fix(ci): stop the revert oracle scoring an all-skipped baseline as pass - #4131
Open
BIMvoice wants to merge 4 commits into
Open
fix(ci): stop the revert oracle scoring an all-skipped baseline as pass#4131BIMvoice wants to merge 4 commits into
BIMvoice wants to merge 4 commits into
Conversation
vitest's and node --test's summary lines count a skipped test into the
same `total` as a passed one: `Tests 2 skipped (2)` parses through
parseRunnerOutput to `{ passed: 0, failed: 0, total: 2 }`, exactly the
shape a `describe.skipIf` guard produces when its fixture is absent.
`verdict()` only ever guarded `baseline.total === 0`, so a file that
collected tests but executed none of them was scored `kind: pass` and
used as a green baseline -- and then went on to render an identically
skipped reverted run as an UNOBSERVED "finding about the author's
tests" when nothing was ever measured.
Adds a new `ALL_SKIPPED` kind (`scripts/lib/revert-oracle-all-skipped.mjs`,
split out to keep revert-oracle.mjs at its recorded module-size budget)
for the case `collected > 0 && executed === 0`. It plugs into the
existing verdict machinery rather than a new one: a baseline of this
kind falls through the existing `baseline.kind !== PASS` check into
BASELINE-BROKEN, and a reverted run of this kind falls through to the
existing "unhandled reverted kind" INCONCLUSIVE fallback -- both already
model "nothing can be concluded," and neither is UNOBSERVED or a pass.
A PARTIALLY skipped file (some tests ran, some skipped) is left as an
ordinary PASS: it still has at least one executed assertion with real
evidentiary value, and AGENTS.md's skip-not-throw convention for missing
fixtures depends on that staying unremarkable.
pytest and cargo do not share this gap: parsePython already collapses
an all-skipped pytest run to `total: 0` (routing through the existing
NO_TESTS path), and cargo's summary line does not fold `ignored` into
its total. Only vitest and node --test count skips into `total`.
Verified both directions: disabling the new detection makes two tests
fail (the ALL_SKIPPED-kind test and the BASELINE-BROKEN verdict test);
a genuinely-passing, zero-skip vitest baseline (12 passed, 0 skipped)
still scores PASS/OBSERVED exactly as before.
node --test scripts/lib/revert-oracle*.test.mjs: 100 -> 104 tests, all
passing.
Closes #4108
Part of the capability-gap-vs-finding family tracked by #4109 (not
closed by this PR) -- the fifth instance in that family, this one
narrowly scoped to the all-skipped shape rather than #4109's proposed
structural per-file attribution fix.
Contributor
Claude review - no findings for
|
github-actions
Bot
added
the
llm-reviewed
|
Warning Review limit reached
This review includes 3 billable files and costs up to $0.75. Or wait 3 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe oracle now classifies collected runs with zero executed tests as ChangesAll-skipped classification
Priority: ➖ Normal — Impact reflects medium issue severity. Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The oracle now distinguishes wholly skipped test runs from passing runs, preventing skipped baselines from being treated as valid observations while retaining passing behavior for partially skipped files. No current merge-readiness risk remains. 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This was referenced Sep 8, 2026
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
…ence in the revert oracle #4131 (closes #4108) correctly stops an all-skipped baseline from being scored a pass, but `aggregate()`'s severity walk ranked ALL_SKIPPED above PASS and ASSERTION_FAILURE unconditionally. CI's revert-oracle job installs Python/ifcopenshell only when Python files changed, so a TS-only PR touching even one env-gated all-skip file (e.g. packages/export/src/ifcopenshell-schema-conformance.test.ts) now aggregates to ALL_SKIPPED across the whole run, and verdict() falls into BASELINE-BROKEN/exit 3 -- blocking a PR whose other packages produced real, executed assertions that went red on revert. Adds severityCandidates() in revert-oracle-all-skipped.mjs: when at least one result in the run has kind PASS or ASSERTION_FAILURE, ALL_SKIPPED entries are dropped from the severity walk (their counts/evidence still fold into aggregate()'s totals, unfiltered). With no real evidence anywhere, every result stays a candidate, so a single all-skipped package, or a run where every package is all-skipped, still aggregates to ALL_SKIPPED and still blocks -- #4108's own case is unchanged. Verified both directions with synthetic aggregate()/verdict() calls before fixing: a PASS-plus-ALL_SKIPPED run aggregated to ALL_SKIPPED/BASELINE-BROKEN (exit 3) pre-fix, and to PASS/OBSERVED (exit 0) post-fix; a single- or every-package all-skipped run stayed ALL_SKIPPED/BASELINE-BROKEN (exit 3) in both cases. Mutated to confirm the tests hold: reverting the aggregate() change back to scanning `results` directly turns exactly the three new multi-package tests red; reverting severityCandidates() to unconditionally filter ALL_SKIPPED (dropping the single-signal guard) turns exactly the two single/every all-skipped tests red. All other tests stayed green in both mutations. scripts/lib/revert-oracle.mjs stays within its 528-line module-size budget (now flush against it); the new logic lives in the existing revert-oracle-all-skipped.mjs sibling module instead of growing the budget. node --test scripts/lib/revert-oracle*.test.mjs: 104 -> 108 tests, all passing.
Contributor
Claude review - no findings for
|
github-actions
Bot
added
llm-reviewed
Contributor
Claude review - no findings for
|
github-actions
Bot
added
llm-reviewed
…d-baseline Resolves a textual conflict in scripts/lib/revert-oracle.mjs between this branch's ALL_SKIPPED bucket/severityCandidates work and #4140's inert-path classification: both imports kept, isInertPath wired after the existing TEST_* checks in classifyPath so precedence is preserved.
Contributor
Claude review - no findings for
|
github-actions
Bot
added
llm-reviewed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
vitest's and node --test's summary lines count a skipped test into the same
totalas a passed one: vitest'sTests 2 skipped (2)parses throughparseRunnerOutputto{ passed: 0, failed: 0, total: 2 }— exactly the shape adescribe.skipIfguard produces when its fixture is absent (AGENTS.md requires skipping, not throwing, in that case).verdict()only ever guardedbaseline.total === 0, so a file that collected tests but executed none of them was scoredkind: passand used as a green baseline — matching this issue's own quoted CI output:packages/export (vitest) -> pass (pass 0, fail 0, total 2).Worse than cosmetic: since the baseline was accepted as a legitimate PASS, the oracle went on to score an identically all-skipped reverted run as
UNOBSERVED— "FINDING: ... all N test(s) still PASS. The branch's tests do not observe the branch's change." — a finding about the author's tests, when the truth is that nothing was ever measured.Closes #4108
Part of the capability-gap-vs-finding family tracked by #4109 (referenced, not closed by this PR). This fix is narrowly scoped to #4108's specific all-skipped shape rather than #4109's larger proposed per-file execution attribution.
RED (before the fix)
An all-skipped baseline was scored
pass, and an identically all-skipped reverted run was reported asUNOBSERVED(exit 1) — a finding about the branch's tests, not a report that nothing was measured.Fix
Adds a new
ALL_SKIPPEDkind (scripts/lib/revert-oracle-all-skipped.mjs— split out to keeprevert-oracle.mjsat its recorded module-size budget, the same reasonrevert-oracle-python.mjs,revert-oracle-rust-features.mjsandrevert-oracle-plan-runs.mjsexist) forcollected > 0 && executed === 0(passed === 0 && failed === 0).It plugs into the oracle's existing verdict machinery rather than inventing a parallel one:
ALL_SKIPPEDis notPASS, so it falls through the existingbaseline.kind !== PASScheck intoBASELINE-BROKEN(exit 3) — the same bucketNO_TESTS/LOAD_FAILURE/etc. baselines already use, and neverUNOBSERVED.ALL_SKIPPEDfalls through to the existing "unhandled reverted kind"INCONCLUSIVEfallback (exit 3) — also neverUNOBSERVEDor a pass.KIND_SEVERITY(used byaggregate()across packages) getsALL_SKIPPEDinserted betweenASSERTION_FAILUREandNO_TESTS, so one all-skipped package still poisons an otherwise-green multi-package aggregate.No new exit code was needed —
EXIT_UNOBSERVED(1) stays reserved for genuine findings, and the dispatcher (check-test-revert-oracle.mjs) already collapses every non-OBSERVED/non-UNOBSERVEDverdict toEXIT_INCONCLUSIVE(3), which is exactly the right semantic bucket here.GREEN (after the fix)
Partial-skip decision
A partially skipped file (some tests ran, some were skipped) is left as an ordinary
PASS— it still has at least one executed assertion with real evidentiary value that could have gone red on the revert. Verified:{ passed: 1, failed: 0, total: 2 }still classifies asPASS, and a revert that turns that one real assertion red still scoresOBSERVED. The threshold ispassed === 0 && failed === 0(not "any skip"), matching the issue's explicit instruction not to fail on any skip — AGENTS.md's own skip-not-throw convention for missing fixtures depends on partial skips staying unremarkable.pytest and cargo don't share this gap:
parsePythonalready collapses an all-skipped pytest run tototal: 0(routing through the existingNO_TESTSpath), and cargo's summary line does not foldignoredinto its total at all. Only vitest and node --test count skipped tests intototal.Mutation testing (both directions)
ALL_SKIPPEDbranch inrevert-oracle-all-skipped.mjs):node --test scripts/lib/revert-oracle.test.mjswent from 58/58 passing to 56 pass / 2 fail — theALL_SKIPPED-kind test and theBASELINE-BROKENverdict test both caught it.PASS, and a revert that turns 3 of those 12 red still scoresOBSERVED(exit 0) — no false positive introduced.Test plan
node --test scripts/lib/revert-oracle*.test.mjs: 100 → 104 tests, all passing (0 fail, 0 skipped)node scripts/check-module-size.mjs: OK (revert-oracle.mjsat its 528-line budget, no headroom — no budget raised)node scripts/check-test-wiring.mjs: OKnode scripts/check-source-text-assertions.mjs: OK🤖 Generated with Claude Code
https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
Summary by CodeRabbit