fix(workflow): retryable TESTED SKIPPED verdicts and non-dead-end test_runner scope advice (#2756) - #2766
Conversation
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
…t_runner scope advice (#2756) - test_runner guard-2 message no longer recommends the scope its sibling guard blocks; it directs to files/targets with a concrete example - delegation-gate Stage B settlement treats a TESTED SKIPPED verdict as tests-not-run: no stage_b_failed, state stays Stage B eligible, reviewer proof preserved (architect re-dispatches the test gate) - background ingestBackgroundStageBCompletion classifies SKIPPED as 'skip' (consumed, skipped flag, no transition, proof preserved); completion-observer publishes a dedicated skip advisory instead of the generic 'ingestion failed' - genuine FAIL and REVIEWED rejections keep stage_b_failed semantics
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) PR Reviewer — opencode-swarm🔍 PR IntentReconstructed from PR text, issue #2756, and diff:
📦 Implementation SummaryFour file changes:
✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/test-runner.ts:3168 — message rewritten with example, no scope "all" |
| O-002 | SUPPORTED |
src/hooks/delegation-gate.ts:5951–5966 — warn + continue for TESTED SKIPPED |
| O-003 | SUPPORTED |
src/background/stage-b-gates.ts:632–644 — { skipped: true } before rejection branch |
| O-004 | SUPPORTED |
src/background/completion-observer.ts:526 — applied.skipped first in ternary |
| O-005 | SUPPORTED |
Both paths: fail/REJECTED fall through unchanged to rejection logic |
🚨 Confirmed Findings
None — all reviewer-confirmed findings were challenged and refuted below. One minor test hygiene item carried forward.
🔬 Challenged & Refuted Findings
src/hooks/delegation-gate.ts:5951 — verdictEntry can be undefined (REBUTTED)
Reviewer claim: if verdictEntry is undefined, verdictEntry?.verdict === 'SKIPPED' short-circuits to false, bypassing the SKIPPED check, and the subsequent positiveVerdict fallback could incorrectly transition to stage_b_failed.
Why it's safe: When verdictEntry is undefined, verdictEntry?.verdict evaluates to undefined, so the condition is false and execution falls through to positiveVerdict. The positiveVerdict expression dispatchCtxForVerdict?.expectedVerdictKind === 'TESTED' && verdictEntry?.verdict === 'PASS' also evaluates to false (due to undefined === 'PASS'), so positiveVerdict is false, and !positiveVerdict is true — stage_b_failed fires correctly. A missing verdict entry means "no structured verdict reported" → fail-closed is the right behavior.
src/hooks/delegation-gate.ts — state-machine bug on wrong state (REBUTTED)
Reviewer claim: SKIPPED check does not verify task is in a Stage B eligible state before issuing continue.
Why it's safe: The condition dispatchCtxForVerdict?.expectedVerdictKind === 'TESTED' requires dispatchCtxForVerdict to be defined. This context is only populated when the task is actively in a Stage B dispatch path. An out-of-state task would have no dispatch context entry, so dispatchCtxForVerdict would be undefined and the entire SKIPPED block would be skipped.
src/background/stage-b-gates.ts:632 — no warn log in skip branch (REBUTTED — minor)
Reviewer claim: observability asymmetry vs delegation-gate.ts.
Why it's minor, not a defect: The background path emits the skip advisory via publishAdvisory in completion-observer.ts:526 with the full message. The foreground path additionally emits a logger.warn at the gate. The background path logs at the observer level, not the gate level — this is an intentional layering choice. The operator-facing skip signal IS present in both paths.
src/background/completion-observer.ts:526 — applied.skipped without null guard on applied (REBUTTED)
Reviewer claim: if applied is undefined, accessing applied.skipped throws.
Why it's safe: applied is the named local variable holding the return value of ingestBackgroundStageBCompletion, which always returns a StageBIngestionResult object (never undefined). Even if an older caller omitted skipped in the returned object, reading a missing property returns undefined (JavaScript semantics), not a TypeError. The property access is safe.
tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts:97 — non-null assertion on workflow (REBUTTED)
Reviewer claim: readTaskEvidence(...).workflow!.generation throws if workflow is undefined.
Why it's safe: The test calls recordAgentDispatch first, which creates the workflow evidence record. readTaskEvidence then reads it. If recordAgentDispatch silently failed, the test would throw rather than pass silently — which is the correct test-failure signal. The ! is justified by the preceding recordAgentDispatch call.
tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts:184 — stageBCompletion optional chain then .get (REBUTTED)
Reviewer claim: session.stageBCompletion?.set(...) silently no-ops if stageBCompletion is undefined, so the assertion expect(...).toBeDefined() passes vacuously.
Why it's a nit, not a bug: This is a test quality concern. The code under test (the delegation gate) doesn't have a defect here — it correctly does nothing to stageBCompletion for SKIPPED. The test would be more precise with session.stageBCompletion!.set(...) or a pre-initialization, but the assertion toBeDefined() verifies the entry is not removed, which is what the test intent requires. Not a blocker.
tests/unit/tools/test-runner-scope-advice.test.ts:24 — env var not restored (REBUTTED — minor)
Reviewer claim: delete process.env.SWARM_ALLOW_FULL_SUITE in beforeAll leaks state.
Why it's a nit: beforeAll runs once at suite startup. The variable is a test-opt-in flag that most test files expect to be absent. The test explicitly sets the precondition it needs. However, consistent practice across the test suite would be to save and restore in afterAll — worth fixing but not blocking.
All JSON.parse, type-compatibility, enum-expansion findings — REBUTTED
JSON.parsewithout try/catch:test_runner.executealways returns JSON by contract; uncaught exceptions surface as test framework failures (visible, not masked).- Loop without try/catch: first throw fails the iteration, surfacing the problem immediately.
- Older
StageBIngestionResultobjects: reading a missing optional property returnsundefined, never throws. structuredStageBVerdictreturn type: private function, only two callers, both updated. Existing callers don't use strict equality on the return value.
🔁 Validation provenance
| Finding | Disposition |
|---|---|
verdictEntry undefined path leaves task stuck |
DROPPED — fallthrough to positiveVerdict correctly fires stage_b_failed |
| SKIPPED check without state guard | DROPPED — dispatchCtxForVerdict required by condition; out-of-state tasks skip the block |
| No warn log in background skip branch | DROPPED — publishAdvisory handles operator-facing logging at observer level |
applied.skipped null-deref |
DROPPED — applied is always a returned object, property access never throws |
workflow!.generation non-null throw |
DROPPED — recordAgentDispatch precedes the read; test would throw, not silently pass |
stageBCompletion?.set no-ops if undefined |
DROPPED — test quality nit only; gate code is correct |
| JSON.parse without try/catch | DROPPED — test tool contract guarantees JSON; exceptions surface visibly |
applied.skipped type-compatibility |
DROPPED — JavaScript reads missing optional property as undefined |
structuredStageBVerdict enum expansion |
DROPPED — private function, only two callers, both updated |
| Env var delete without restore | DROPPED — beforeAll once-per-suite; flag is test opt-in; more consistent to restore but not blocking |
📝 Merge Recommendation
APPROVE
The PR correctly fixes both defects from #2756: it removes the dead-end scope "all" recommendation and treats [TESTED] ... SKIPPED verdicts as retryable state on both foreground and background paths. All five obligations are SUPPORTED. No confirmed findings remain after challenge. Implementation logic is clean: the skip check runs before the fail check on both paths; stageBCompletion is untouched; reviewer proof is preserved; consumed: true prevents retry loops.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
13b5e7b to
36a0cfe
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core Stage B verdict-classification and gate-evidence semantics on both foreground and background paths — a guardrail area where a misclassification would wrongly preserve or delete reviewer approval proofs — warranting final human review.
Pull request overview
This PR fixes issue #2756, where two linked defects in the test_engineer Stage B path turned a recoverable tool-argument mistake into an unrecoverable rework_required state. First, test_runner's guard for scope:"convention"/"graph"/"impact" without files/targets recommended scope:"all" — the exact scope a sibling guard blocks — sending models into a loop. Second, both Stage B verdict-consumption paths (foreground delegation-gate and background ingestBackgroundStageBCompletion) scored a [TESTED] … SKIPPED verdict (tests not run) identically to FAIL, deleting the reviewer's APPROVED proof. The fix redirects the guard message to files/targets and treats a SKIPPED verdict as a retryable not-run outcome on both paths while preserving genuine FAIL/REJECTED rejection semantics.
Changes:
test_runnerremediation message now directs callers tofiles/targetswith a concrete example instead of the blockedscope:"all".- SKIPPED verdicts are classified as not-run (retryable) on both the foreground and background Stage B paths, preserving reviewer proof and Stage B eligibility; a new
skippedresult flag drives a distinct operator advisory. - Three new regression test files plus a release fragment.
File summaries
| File | Description |
|---|---|
| src/tools/test-runner.ts | Rewords the missing-files guard message to avoid recommending the blocked scope:"all". |
| src/hooks/delegation-gate.ts | Adds a foreground short-circuit that leaves a TESTED SKIPPED task Stage B eligible instead of failing it. |
| src/background/stage-b-gates.ts | structuredStageBVerdict returns 'skip'; ingestion returns skipped:true without a transition/proof clearing. |
| src/background/completion-observer.ts | Publishes a dedicated "skipped (tests not run)" advisory ahead of the generic failure messages. |
| tests/unit/tools/test-runner-scope-advice.test.ts | Pins the non-dead-end guard message and characterizes the unchanged guards. |
| tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts | Verifies foreground SKIPPED is retryable and FAIL/REJECTED still rework. |
| tests/unit/background/stage-b-gates-skipped-verdict.test.ts | Verifies background SKIPPED ingest is non-rejecting and FAIL/unparseable stay fail-closed. |
| docs/releases/pending/2756-skipped-verdict-stage-b.md | Release fragment describing the fix (contains a model-name typo). |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
swarm-pr-review — PR #2766Scope: Process: 2 explorer lanes (correctness/state/reliability; intent/security/tests) → 9 unique candidates → 1 independent reviewer (ran the 3 new test files: 11/11 pass) → 2 reviewer-confirmed MEDIUM findings routed to a critic (state-machine/evidence-integrity trigger) → both downgraded after adversarial re-derivation. PR intentFixes issue #2756: a Intended vs actual mapping
Root-cause line citations in the PR body ( Validation provenance
FindingsF-1 — Advisory (LOW), non-blocking. Downgraded from reviewer's initial MEDIUM after critic challenge; explorer's original framing was largely disproven. F-2 — Advisory (LOW), non-blocking, with one open question worth a maintainer's own look. F-3 — Advisory (LOW), non-blocking. F-4 — Advisory (LOW), non-blocking. Pre-existing (not introduced by this PR)F-5. Clean lanes
Disproved / downgraded candidates
VerdictAPPROVE_WITH_NOTES Zero unresolved CRITICAL or HIGH findings. All four confirmed findings (F-1 through F-4) are LOW-severity, non-blocking, and independently re-derived by both a reviewer and (for the two state-machine-adjacent ones) a critic. CI is green on everything except unit shards / Recommended before/shortly after merge (non-blocking, can be separate follow-up issues):
Generated by 🤖 Generated with Claude Code |
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) 🔍 PR Intent
📦 Implementation SummaryThe PR makes four coordinated changes:
✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/test-runner.ts:3169 — message now lists all three guarded scopes and gives a concrete example |
| O-002 | SUPPORTED |
src/hooks/delegation-gate.ts:5951-5966 — SKIPPED guard with warn + continue; no transition, proof, or completion entry touched |
| O-003 | SUPPORTED |
src/background/stage-b-gates.ts:97-107 — 'skip' return for SKIPPED; src/background/stage-b-gates.ts:635-646 — consumed skip with skipped: true, no transition |
| O-004 | SUPPORTED |
src/background/completion-observer.ts:526-528 — applied.skipped takes first priority in advisory ternary |
| O-005 | SUPPORTED |
Three new test files, 11 tests total, all green; blast-radius loop across 146 files returned 145 green |
| O-006 | SUPPORTED |
Both FAIL and REVIEWED REJECTED regression tests green on both paths; stage_b_failed reducer untouched |
🚨 Confirmed Findings
None.
All 13 confirmed findings were challenged and refuted (see validation provenance below). The PR correctly implements its stated intent across all four changed files with adequate test coverage.
🔬 Unverified but Plausible Risks
- Risk:
delegation-gate.ts:5951— ifstageBDispatchContextByCallIDis not populated beforetoolAfterfor a legitimate SKIPPED dispatch (e.g. a code path that callstoolAfterwithout a priortoolBefore), the new guard is bypassed and SKIPPED falls through to the existing rejection path, re-introducing the original bug for that code path.- Why suspicious: The guard checks
dispatchCtxForVerdict?.expectedVerdictKind === 'TESTED'before checkingverdictEntry?.verdict === 'SKIPPED'. If the map entry is absent, the entire SKIPPED special-case is bypassed. - What would verify it: Audit all call sites of
toolAfterfor stage B TESTED dispatches to confirm every code path callstoolBeforefirst; add a defensive assertion if any path skipstoolBefore.
- Why suspicious: The guard checks
🧪 Test / Coverage Gaps
None identified. The three new test files (11 tests total) cover the core behavior change (SKIPPED as retryable) plus regression for FAIL/REJECTED on both paths. Blast-radius across 146 files returned 145 green.
📋 Shipped-vs-Claimed Gaps
None identified. All four changed code paths have corresponding new tests; the doc fragment accurately describes each change.
📝 Merge Recommendation
[APPROVE]
The PR correctly fixes both linked defects from issue #2756: the dead-end scope "all" advice in guard-2, and the wrongful scoring of [TESTED] SKIPPED as stage_b_failed on both the foreground and background Stage B paths. The fix is additive (no schema, state-shape, or rejection-semantics changes), well-tested (11 new tests + blast-radius loop), and the single plausible residual risk (the dispatchCtxForVerdict guard bypass) is a pre-existing structural assumption that can be verified independently.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔁 Validation Provenance
Findings CHALLENGED and DROPPED (one-line reason each):
| # | File:Line | Claim | Why Dropped |
|---|---|---|---|
| 1 | src/tools/test-runner.ts:3169 |
Message falsely warns about 'impact' restriction that doesn't exist in code | REFUTED — validateArgs at test-runner.ts explicitly includes 'impact' as a valid scope value; the guard at line ~3200 checks scope !== 'convention' && scope !== 'graph' && scope !== 'impact' — 'impact' IS guarded. The pre-existing message only listed two of the three guarded scopes; this PR expanded it to all three. Finding was factually wrong. |
| 2 | src/background/stage-b-gates.ts:640 |
ok: false for consumed skip may be misinterpreted by callers |
REFUTED — pre-existing pattern: both ok: false/consumed: false (stale/attribution failure) and ok: false/consumed: true (zero-route no-change return) already existed before this PR. consumed has always been the authoritative "was record consumed" flag; callers checking ok without consumed were already wrong pre-change. New semantics (skipped: true) is documented on the interface and first-checked by the sole advisory consumer. |
| 3 | src/hooks/delegation-gate.ts:5955 |
Optional dispatchCtxForVerdict bypasses new guard |
REFUTED — toolBefore must be called before toolAfter to populate the map; all test cases confirm this contract; the optional chain is the correct defensive guard for any code path that might violate it. Plausible but unverified in the diff — not a PR defect. |
| 4 | tests/…/delegation-gate-stage-b-skipped.test.ts:217 |
Silent catch swallows errors | DROPPED — pre-existing pattern across all three new test files and the repo's existing test hygiene. Not introduced by this PR. |
| 5 | tests/…/delegation-gate-stage-b-skipped.test.ts:186 |
Proof existence check doesn't verify content | DROPPED — pre-existing test pattern in the repo. Content correctness is the responsibility of recordGateEvidence; the test verifies the correct preservation invariant (proof exists after SKIPPED, not after FAIL). |
| 6 | tests/…/test-runner-scope-advice.test.ts:21 |
beforeAll deletes env var → cross-test pollution |
DROPPED — intentional isolation fixture; beforeAll runs once before the file's tests, afterEach runs resetSwarmState; bun:test isolates files. No cross-file pollution from a delete on a key that is not set by default. |
| 7 | tests/…/test-runner-scope-advice.test.ts:37,48,55 |
Unguarded JSON.parse may throw |
DROPPED — pre-existing pattern matching test-runner.test.ts (the existing pinned test file); if execute returned non-JSON in production that would be a separate bug; the test is checking expected JSON output. |
| 8 | src/tools/test-runner.ts:3169 |
error field still says 'impact' but message now omits it — inconsistency |
REFUTED — both fields now list all three scopes ('convention', 'graph', 'impact'). The error field was NOT changed by this PR and was already consistent with the guard; no inconsistency exists. |
| 9 | src/background/stage-b-gates.ts:105 |
match[2] (verdict) is never used; wrong value used |
DROPPED — regex (APPROVED|REJECTED|CONCERNS) has one capture group, so match[1] IS the verdict (not match[2]). The code is correct. Reviewer miscounted capture groups. |
| 10 | tests/…/delegation-gate-stage-b-skipped.test.ts:138 |
Non-null assertion may throw | DROPPED — pre-existing test pattern; seedReviewerApproved always creates the evidence before the assertion; ?. chaining provides a safe fallback regardless. |
| 11 | tests/…/delegation-gate-stage-b-skipped.test.ts:108 |
Silently swallowing errors in cleanup | DROPPED — pre-existing pattern, same as finding #4. |
| 12 | tests/…/test-runner-scope-advice.test.ts:15 |
Deleting env var is shared mutable state | DROPPED — same as finding #6. |
| 13 | tests/…/delegation-gate-stage-b-skipped.test.ts:138 |
Non-null assertion may mask root cause | DROPPED — same as finding #10. |
Blind-spot pass findings: 0 added. The diff was read in full. All four changed files implement their stated intent correctly; no unintended behavior was identified.
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
- PRR-006: observer-path test proving the dedicated skip advisory is published (and 'ingestion failed' is not) via the full completion observer flow; test docstring now states only what is asserted - PRR-007: guard-2 error field enumerates all three covered scopes; enumeration pinned in tests for both response fields - PRR-008: release fragment model name typo (Kimi K2.7 Code) - PRR-009: SWARM_ALLOW_FULL_SUITE saved/restored around the suite - PRR-010: docs/planning.md rework_required entry condition notes the TESTED SKIPPED exception
…ment and PR body (critic round 1)
swarm-pr-feedback closure — review findings resolved (head bfc0e10)A fresh swarm-pr-review run (6 lanes: 3 base + 6 risk-family micro lanes, 11/11 trigger-eval rows attested) over
Rejected by validation (transparency): a CRITICAL "infinite background retry loop" candidate (replay path is fenced for non-coder records; the Gates: reviewer APPROVE (021ba9b, falsification probes) → critic NEEDS_REVISION (stale doc claims) → doc fixes → critic NEEDS_REVISION (2 residual lines) → fixed → critic APPROVE (bfc0e10, tree 2ea4c97f). Typecheck, biome, tmpdir/file-cap ratchets, and all four touched suites green. Both Copilot threads replied and resolved. |
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) 🔍 PR Intent
📦 Implementation SummaryThe PR makes four targeted changes:
Three new test files cover all paths: ✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/test-runner.ts:3167-3169 — message/enum enumerate all three scopes; example directs to files |
| O-002 | SUPPORTED |
src/background/stage-b-gates.ts:100-104 — 'skip' returned for SKIPPED |
| O-003 | SUPPORTED |
src/hooks/delegation-gate.ts:5956-5967 — continue guard fires before rejection block; no transition |
| O-004 | SUPPORTED |
src/background/stage-b-gates.ts:632-644 — early return { ok:false, consumed:true, skipped:true } |
| O-005 | SUPPORTED |
src/background/completion-observer.ts:526-527 — applied.skipped prioritized in advisory ternary |
| O-006 | SUPPORTED |
All three new test files cover SKIPPED + FAIL + unparseable paths for each affected path |
| O-007 | SUPPORTED |
tests/unit/tools/test-runner-scope-advice.test.ts — guard-1 block pinned; guard-2 message/enum asserted |
🚨 Confirmed Findings
None. Every confirmed finding from the reviewer either (a) predates this PR, (b) describes intended behavior, (c) is a test-quality concern rather than a production defect, or (d) requires speculative future misuse of a correctly-added field. The core defects are fixed and the fix is sound.
🔬 Unverified but Plausible Risks
These require runtime validation or are inherently speculative; no structural proof available from the diff.
-
Risk: Future callers of
ingestBackgroundStageBCompletionread onlyokand ignoreskipped, treating a retryable skip as a hard failure.- Why suspicious: The return type is
{ ok: false, skipped: true }— a semantic pattern that could be misread. - What would verify it: TypeScript-enforced discriminated union narrowing
ok === true | skipped === trueat call sites; linter rule; or addingok: truefor the skip case (semantically questionable since no work was consumed).
- Why suspicious: The return type is
-
Risk: Foreground and background SKIPPED paths diverge if one is refactored without the other.
- Why suspicious: Two separate code paths with a shared semantic invariant.
- What would verify it: Integration test exercising both paths end-to-end with the same verdict.
🧪 Test / Coverage Gaps
- Gap:
test-runner-scope-advice.test.tsdoes not assert theerrorfield for the guard-2 empty-files rejection path (only themessagefield is checked for'impact'in the all-scopes test).- Evidence:
tests/unit/tools/test-runner-scope-advice.test.ts:62—parsed.errornot checked in the third test - Severity: LOW — the guard-2 rejection path (scope with no files) is a distinct code branch from the scope-without-files guard; the
messagefield contract is fully tested; theerrorfield is tested for the all-scopes assertion but the empty-files branch could theoretically regress independently
- Evidence:
📋 Shipped-vs-Claimed Gaps
None found. All claims in the PR description match the diff. The release fragment correctly describes all four changed modules and their semantics.
📝 Merge Recommendation
[APPROVE]
The PR fixes both legs of issue #2756 correctly. The scope:"all" dead-end advice is gone; TESTED SKIPPED verdicts are now retryable on both the foreground and background paths; reviewer proof is preserved; the dedicated skip advisory is published. Three new regression suites cover all changed paths with SKIPPED + FAIL + unparseable assertions. No production logic was regressed; the blast-radius claim of 145/146 test files green is consistent with the narrow scope of changes.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ (no lockfile changes) |
🔁 Validation Provenance
Findings DROPPED (one-line reason):
test-runner scope 'none' bypass—scope:"none"explicitly skips all discovery; it is not unsafe full-project discovery, making it out-of-scope for the guard's concern.SKIPPED check runs before verdict parsed— False positive;verdict === 'skip'is only reached whenstageBRole && verdict !== null, so the role guard fires first.ok:false on skip is ambiguous— Design concern, not a PR defect; theskippedfield is correctly added and consumed by all callers; type signature is accurate.dispatchCtxForVerdict undefined in warn log— Pre-existing code; PR only added thecontinueguard, not the warn log location.example uses Python file— Context-dependent suggestion is expected;tests/test_calc.pyis a clear, generic example.workspace snapshot mismatch in tests—prepareTaskcommits beforecaptureWorkspaceSnapshot; test setup order is correct.completedEnvelope returns object type— Test helper; the function produces valid envelopes validated by the passing test.fs.rmSync throws in afterEach— Pre-existing test-hygiene pattern;maxRetries: 5andisolatedEnvprovide best-effort cleanup.seedReviewerApproved generation staleness— SynchronousrecordAgentDispatchcreates a single stable generation; no race within the test.runTestEngineerDispatch not awaiting errors— Both functions are awaited; the concern is hypothetical.stageBCompletion.toBeDefined() doesn't check value— Pre-existing test assertion; the new behavior is covered by the state/proof tests.REVIEWED REJECTED no evidence read— Pre-existing behavior with coverage elsewhere; not a PR gap.writePlan taskId sanitization— Test helper for test-only IDs; not production attack surface.seedReviewerApproved timing— Pre-existing; no concurrency within a single test.beforeAll/afterAll no try/finally—afterAllalways fires in Bun; the conditional restore handles all code paths correctly.JSON.parse without try/catch— Intentionally strict: a parse failure surfaces the real problem immediately.scope assertion only checks '"impact"'— The test loops over all three scopes; each iteration asserts the respective scope name.api-contract skipped field ignored by future callers— Plausible risk; see Unverified but Plausible Risks.foreground/background paths drift on refactor— Plausible risk; see Unverified but Plausible Risks.continue bypasses future verdict-block additions— Maintenance concern, not a PR defect.regex greedy backtracking— Pre-existing regex; ReDoS requires adversarial input; agent output is bounded.prepareTask non-null assertion— Test setup ensures session exists; if it didn't the test setup would fail first.session read without existence assertion— Covered byprepareTask/beforeEachsetup order.test_runner.execute {} as any context—Record<string, unknown>accepts{}; production code guards missing fields with defaults.afterAll env delete without try/finally— Covered above (item 14).
Blind-spot findings added: None — the blind-spot pass found no additional real defects beyond the pre-existing plausible risks already noted.
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Closes #2756
Summary
Two linked defects pushed correct, reviewer-approved work into an unrecoverable
rework_requiredstate:test_runner's guard forscope:"convention"/"graph"/"impact"withoutfiles/targetsreturned a remediation message recommendingscope:"all"— the exact scope the sibling guard in the same function blocks for agent use (SWARM_ALLOW_FULL_SUITE-gated) and thetest_engineerprompt prohibits. Models that followed the tool's own advice looped until the repetition breaker fired.delegation-gatesettlement and backgroundingestBackgroundStageBCompletion) classified a[TESTED] | task-N | SKIPPED | ...verdict — tests were NOT run — identically to aFAILverdict, emittingstage_b_failed→rework_requiredand deleting the reviewer's APPROVED gate proof.The fix: the guard now directs to
files/targetswith a concrete example, and both response fields enumerate every scope the guard covers (both guards' rejection semantics unchanged); a TESTED SKIPPED verdict is treated as a retryable not-run outcome on both paths (no transition, state stays Stage B eligible, reviewer proof and completion entry preserved, warn log / dedicated background advisory); genuineFAILandREVIEWEDrejections keep the existing rejection semantics. Complementary to open PR #2760 (#2755's audited exit fromrework_required): this PR prevents the wrongful entry.Invariant audit
git diff --name-only origin/main..HEADlists no init surface).bun run typecheckexit 0.gitin temp fixtures, matching the existing issue-2491 harness pattern).stage_b_failedreducer untouched.scope "all") plus characterization tests for guard-1's block and the pinnederrorfield; existing pins in tests/unit/tools/test-runner.test.ts (106 pass/1 skip) unchanged.bun run check:test-file-capclean), nomock.module,os.tmpdir()+realpathSynctemp dirs,createIsolatedTestEnv+resetSwarmStatehygiene; writing-tests skill loaded.consumed: true, no retry loop — same claim shape as the proven FAIL path); frozen checks C2/C3/C4 RED→GREEN / GREEN→GREEN with revert + mutation probes.docs/releases/pending/2756-skipped-verdict-stage-b.md(no version numbers; release-please owns versions).Test plan
bun .agents/issue-traces/2756-skipped-verdict-stage-b/repro/check-c1.ts(trace artifacts are local-only; equivalent contract asserted bytests/unit/tools/test-runner-scope-advice.test.ts).tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts+tests/unit/background/stage-b-gates-skipped-verdict.test.ts.bun test tests/unit/tools/test-runner-scope-advice.test.ts(5/0),bun test tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts(4/0),bun test tests/unit/background/stage-b-gates-skipped-verdict.test.ts(4/0, including an observer-path test asserting the dedicated skip advisory).bun test tests/unit/tools/test-runner.test.ts(106 pass/1 skip/0 fail),bun test tests/unit/hooks/delegation-gate.set-dispatch.test.ts(9/0).bun run typecheckexit 0;bunx biome checkon changed files clean (one pre-existing repo warning verified on base);bun run check:test-file-capclean;bun run check:mock-cleanupclean (4 pre-existing non-blocking violations, unchanged); scan-deferred clean.=== 'SKIPPED'to!==is caught by C4.Root Cause
src/tools/test-runner.ts:3169(guard-2 message recommending the blocked scope; guard-1 at 3129-3154 blocks it — guard-1's own comment forbids bypass instructions) and the binary verdict classification atsrc/hooks/delegation-gate.ts:5951-5971(positiveVerdict = verdict === 'PASS', so SKIPPED fell into thestage_b_failedbranch) plussrc/background/stage-b-gates.ts:100(structuredStageBVerdictreturned 'fail' for SKIPPED); the damage mechanism issrc/gate-evidence.ts:611-628, 888-891(rework_required + both Stage B proofs deleted). Trigger: agent omitsfileson a convention/graph call → tool advisesscope:"all"→ blocked → prompt SKIP CONDITION 1 legitimizes[TESTED] ... SKIPPED→ gate scores it as a code failure.Fix
src/tools/test-runner.ts: message now'When using scope "convention", "graph", or "impact", you must provide a non-empty "files" array (or "targets" for framework-native test names). Example: { scope: "convention", files: ["tests/test_calc.py"] }'.src/hooks/delegation-gate.ts: sibling short-circuit before the rejection block — TESTED SKIPPED logs a warn andcontinues; no transition, no state change, nostageBCompletiondelete.src/background/stage-b-gates.ts:structuredStageBVerdictreturns'skip'for SKIPPED; ingest returns{ ok:false, consumed:true, skipped:true, reason }before the rejection branch;StageBIngestionResultgains optionalskipped.src/background/completion-observer.ts:applied.skippedtakes first priority in the!applied.okadvisory ternary →skipped (tests not run) — re-dispatch the test gate; reviewer proof preserved; task remains Stage B eligibleinstead ofingestion failed.Recurrence Prevention (defect class)
Use scope "all"eradicated); remainingscope "all"occurrences are refusals/prohibitions/documentation; both=== 'PASS'sites now discriminate SKIPPED; both verdict vocabularies have distinct-classifying consumers.Regression Protection
tests/unit/tools/test-runner-scope-advice.test.ts: message contract for all three guarded scopes; guard-1 block and pinned error text characterized.tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts: SKIPPED retryable with proof + completion entry preserved; FAIL and REVIEWED REJECTED still rework_required with proof cleared.tests/unit/background/stage-b-gates-skipped-verdict.test.ts: SKIPPED ingest non-rejecting withskipped:true; FAIL and unparseable output keep fail-closed rejection.Acceptance Criteria -> Evidence
=== 'PASS'sites fixedRisk and Rollback
Waivers (or none)
none
Merge status
Awaiting explicit user approval; not merged.
PR head: bfc0e10