fix(guardrails): correct coder mutation guidance - #2770
zaxbysauce wants to merge 3 commits into
Conversation
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
🤖 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 splits a single ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/hooks/guardrails/index.ts:207-210 — new separate set added; both catches updated |
| O-002 | SUPPORTED |
src/hooks/guardrails/index.ts:1267-1273,1338-1344 — actionable guidance replacing /swarm recover |
| O-003 | SUPPORTED |
src/hooks/guardrails/index.ts:1262,1335 — else if still fires for STAGE_A_ATTRIBUTION_MISS_CODES |
| O-004 | SUPPORTED |
tests/unit/hooks/guardrails-durable-stage-a-attribution.test.ts:381-419, guardrails-stage-a-coder-mutation-failure.test.ts:1-84, stage-a-attribution-classification-preserving.test.ts:1-30, stage-a-error-classification.test.ts:82-99 |
| O-005 | SUPPORTED |
docs/releases/pending/guardrails-coder-mutation-guidance-2758.md:1-19 |
🚨 Confirmed Findings
None — all reviewer-confirmed findings were false positives or already addressed by existing test coverage.
🔬 Unverified but Plausible Risks
None — all plausible concerns either (a) have structural proof the risk is blocked, or (b) are already covered by the exhaustive classification guardrail test.
🧪 Test / Coverage Gaps
None found. The exhaustive classification test (stage-a-error-classification.test.ts) mechanically verifies every TASK_WORKFLOW_* code in gate-evidence.ts is either classified or allowlisted, preventing silent regression.
📋 Shipped-vs-Claimed Gaps
None.
📝 Merge Recommendation
APPROVE
The fix is correct, the classification is exhaustive and mechanically enforced, the test coverage is thorough, and every reviewer-confirmed finding was a false positive. The PR fully addresses issue #2758 with a minimal, well-reasoned change.
| 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 DROPPED (one-line reason):
| # | Finding | Reason |
|---|---|---|
| 1 | Silent swallow at line 1262/1338 if code not in either set | null return from stageAWriteErrorCode for non-TASK_WORKFLOW errors is intentional and allowlisted by design; the exhaustive classification test pins every reducer code. |
| 2 | optionalCoderMutationCodes() vacuous pass |
instanceof Set fallback returning new Set() is intentional defensive behaviour; covered by the disjointness + exhaustive tests in stage-a-error-classification.test.ts. |
| 3 | getCoderMutationCodes() vacuous pass |
Same as above — stage-a-error-classification.test.ts:85 explicitly asserts the exact singleton set. |
| 4 | Asymmetric fragility (direct import vs cast) | Not a defect — direct import is the safer pattern, cast is defensive fallback; both are intentional. |
| 5 | Missing inverse non-overlap test | Already covered by stage-a-error-classification.test.ts:88-91 which explicitly asserts no overlap. |
| 6–7 | Message injection via error code interpolation | Error codes come from internal reducer constants; no user-controllable path exists. |
| 8–9 | HIGH severity on cast fragility | Downgraded: existing exhaustive classification tests catch vacuous pass; stage-a-error-classification.test.ts:85 is a concrete structural proof. |
| 10 | Other advisories could mask regression | Minor nit; the pass/fail advisory is present and asserts absence of /swarm recover. |
Blind-spot pass: No additional real defects found. The null path through the catch chain is intentional (non-TASK_WORKFLOW errors are expected and allowlisted).
🔒 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.
There was a problem hiding this comment.
🔵 Needs a closer look
The change modifies advisory routing in the guardrails/retry subsystem (a non-negotiable invariant surface with a history of subtle regressions), so final human sign-off is warranted despite no issues being found.
Pull request overview
This PR fixes issue #2758, where the Stage A guardrail catches in src/hooks/guardrails/index.ts misclassified the reducer's TASK_WORKFLOW_CODER_MUTATION_REQUIRED throw as an attribution miss and advised the architect to run the human-only /swarm recover command. That advice was both wrong (the result was attributed) and non-actionable, causing the architect to waste run time and model spend chasing a non-existent recovery path. The fix separates the coder-mutation-required category from genuine attribution-recovery failures and gives the architect actionable guidance (dispatch a coder for a real change, or mark the task blocked).
The change is verified against the reducer in src/gate-evidence.ts:647-668: stage_a_passed/stage_a_failed throw only TASK_WORKFLOW_CODER_MUTATION_REQUIRED (or a generation-mismatch fencing code), so the newly split coder-mutation branch is the live path while the retained STAGE_A_REQUIRED attribution branch stays defensive at these two catches but remains live in the delegation-gate Stage B surfaces.
Changes:
- Split
TASK_WORKFLOW_CODER_MUTATION_REQUIREDinto a dedicatedSTAGE_A_CODER_MUTATION_REQUIRED_CODESset, disjoint fromSTAGE_A_ATTRIBUTION_MISS_CODES, and route it to distinct advisory/critical-warn guidance in both Stage A catch blocks. - Add regression and classification-non-overlap coverage across three new/updated test files, plus a pass-side
rework_requiredregression. - Add a pending release fragment documenting the guidance correction.
File summaries
| File | Description |
|---|---|
src/hooks/guardrails/index.ts |
Adds STAGE_A_CODER_MUTATION_REQUIRED_CODES, updates the classification doc comment, and routes coder-mutation-required throws to actionable guidance in both Stage A failure/pass catches. |
tests/unit/hooks/stage-a-error-classification.test.ts |
Extends the mechanical classification pin to the new set and adds disjointness/attribution-preservation assertions. |
tests/unit/hooks/stage-a-attribution-classification-preserving.test.ts |
New test confirming TASK_WORKFLOW_STAGE_A_REQUIRED stays in attribution recovery only. |
tests/unit/hooks/guardrails-stage-a-coder-mutation-failure.test.ts |
New real-hook regression for the failure-side coder-mutation guidance. |
tests/unit/hooks/guardrails-durable-stage-a-attribution.test.ts |
Adds a pass-side rework_required regression asserting coder-mutation guidance without recovery advice. |
docs/releases/pending/guardrails-coder-mutation-guidance-2758.md |
Pending release note for the guardrail message correction. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- 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 — #2770
|
zaxbysauce
left a comment
zaxbysauce
left a comment
There was a problem hiding this comment.
Swarm PR review
Verdict: APPROVE_WITH_NOTES (implementation approved; keep the draft while required checks are blocked).
- Confirmed low-severity test-quality gap:
tests/unit/hooks/guardrails-stage-a-coder-mutation-failure.test.ts:73asserts the queued advisory but not the independentcriticalWarnoutput from either Stage A catch. Recommended follow-up: captureconsole.warn/logger output and assert the accepted-coder-mutation guidance plus absence ofNOT attributedand/swarm recover. - Pre-existing delivery blocker: the unchanged
release-owner-guardtwo-dot comparison sees base-only release files aftermainadvanced from the branch fork, sorelease-owner-guard,quality, andunit-passedfail/skip. The exact three-dot PR diff has no release-owned files. Update/rebase the branch and rerun required checks before merging. - All other reviewed candidates were disproved or pre-existing. The four issue obligations are met at exact head
00e1a842aa1d4a53d66d44c521c1e9d1d8362555.
Review artifacts: .agents/issue-traces/2758-guardrails-coder-mutation-guidance/pr-review-2770/final-review.md and findings.jsonl.
00e1a84 to
7afc75e
Compare
7afc75e to
b8f2383
Compare
Feedback closure — PR #2770 (round 2)The requested feedback fixes are complete and verified on
Verification completed:
No review threads were resolved because none were present and no thread-resolution authorization was supplied. The remaining blocker is GitHub approval before merge. |
🤖 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) Phase 0 — Reconstruct IntentO-001: Split Phase 1 — Summarise Actual BehaviourThe PR adds a new Phase 2 — Compare Intended vs Actual
Phase 3 — Review Confirmed Findings[MEDIUM] "Stage A fail catch block pushes advisory only for coder-mutation errors but NOT for attribution-miss errors"Challenge: The diff at
[MEDIUM] "Stage A pass catch block pushes advisory only for coder-mutation errors but NOT for attribution-miss errors"Challenge: Same diff at
[MEDIUM] "Stage A fail catch: session not null-checked before pushAdvisory"Challenge: At both catch sites, [MEDIUM] "Stage A pass catch: session not null-checked before pushAdvisory"Challenge: Same as above — [MEDIUM] "stageAWriteErrorCode returns null/undefined, unclear if legitimate"Challenge: [MEDIUM] "optionalCoderMutationCodes returns empty Set causing vacuous pass"Challenge: The primary assertion in this test ( [MEDIUM] "test for attribution-miss fail-side write failure is missing after rename"Challenge: The test "attribution-miss write failure…" ( [LOW] "test name semantic drift"Challenge: See above — the original name was inaccurate; the rename is correct. [LOW] "find() may return undefined before .includes()"Challenge: Bun's [LOW] "advisory.toContain('before Stage A') mismatch"Challenge: The implementation says "before Stage A can be recorded again". [LOW] "getCoderMutationCodes vacuous pass"Challenge: Same as [LOW] "advisory spam from dual pushAdvisory + criticalWarn"Challenge: Phase 4 — Runtime-Aware False-Positive ControlAll findings about session null-checks, vacuous test assertions, and advisory spam are either structurally guaranteed safe (session), mitigated by companion hard-equality tests (vacuous pass), or dependent on upstream deduplication behavior not in scope of this PR (spam). Phase 5 — Blind-Spot PassPotential: silent swallow of unrecognized error codes. Both catch blocks use: if (code && STAGE_A_CODER_MUTATION_REQUIRED_CODES.has(code)) { … }
else if (code && STAGE_A_ATTRIBUTION_MISS_CODES.has(code)) { … }
// no else — null/unknown codes silently fall throughAn error whose message does not start with a 🔍 PR Intent
📦 Implementation SummaryAdds ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/hooks/guardrails/index.ts:207–210 |
| O-002 | SUPPORTED |
src/hooks/guardrails/index.ts:1265–1275, 1339–1349 |
| O-003 | SUPPORTED |
src/hooks/guardrails/index.ts:1277–1281, 1351–1355 |
| O-004 | SUPPORTED |
guardrails-stage-a-coder-mutation-failure.test.ts, guardrails-durable-stage-a-attribution.test.ts:385–431 |
| O-005 | SUPPORTED |
stage-a-error-classification.test.ts:82–100 |
🚨 Confirmed Findings
None — all 13 confirmed findings were successfully refuted by diff analysis or determined to be pre-existing, non-PR-introduced concerns.
🔬 Unverified but Plausible Risks
- Risk: Unknown error codes (non-
TASK_WORKFLOW_*messages from the reducer or wrapped I/O errors) are silently swallowed in both catch blocks with no logging or advisory.- Why suspicious:
if (code && …)guards meancode === nullfalls through both branches with no else-clause. The exhaustiveness test instage-a-error-classification.test.tscovers known reducer codes only; it does not guard against an unexpected thrown object or I/O error leaking through. - What would verify it: Instrument both catch blocks with an else-branch that logs at
warnlevel (e.g.,[guardrails] Stage A write failed with unrecognized code null); add a test exercising the I/O error path directly.
- Why suspicious:
🧪 Test / Coverage Gaps
- Gap: No integration test exercises the attribution-miss
/swarm recoveradvisory end-to-end through either catch block. The classification is covered bystage-a-error-classification.test.ts(direct unit assertions) but not by an advisory-content integration test.- Evidence:
guardrails-durable-stage-a-attribution.test.tscovers coder-mutation pass/fail paths end-to-end;stage-a-attribution-classification-preserving.test.tscovers attribution-miss classification directly but not through the hook. - Note: This is a pre-existing gap; the PR does not reduce coverage.
- Evidence:
📋 Shipped-vs-Claimed Gaps
None.
📝 Merge Recommendation
[APPROVE]
The PR correctly splits the error classification, routes both catch blocks to category-appropriate messaging, adds comprehensive regression coverage for the coder-mutation path, and preserves attribution-recovery semantics. All 13 confirmed findings were refuted by the diff — they described the pre-fix code state, not PR-introduced defects.
| 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 | ✅ (unchanged) |
🔁 Validation provenance
Confirmed findings DROPPED (13/13):
- F1–F2 (missing advisories on fail/pass): refuted by diff — both branches push advisory
- F3–F4 (null session): refuted —
ensureAgentSessionalways returns non-null; pre-existing structural guarantee - F5 (null code guard): refuted —
&&guard is intentional and correct; pre-existing - F6 (vacuous
optionalCoderMutationCodes): dropped — primary assertion is direct export check; mitigated by hard equality in companion test - F7 (attribution-miss test coverage after rename): dropped — original test always exercised coder-mutation path; rename is accurate; classification covered in companion file
- F8–F14 (test name drift, undefined find, substring mismatch, vacuous
getCoderMutationCodes, advisory spam): all dropped — pre-existing idiom, correct substring semantics, upstream deduplication, or mitigated by companion test
Blind-spot findings added (1):
- Silent swallow of unrecognized error codes — pre-existing latent concern, outside PR scope, noted as unverified but plausible
🔒 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 #2758
Summary
TASK_WORKFLOW_CODER_MUTATION_REQUIREDfrom genuine Stage A attribution-recovery failures in both Stage A guardrail transition catches./swarm recoverguidance with actionable accepted-coder-mutation guidance, while preserving genuine attribution recovery semantics.Invariant audit
bun run buildpassed.dist/remains generated/uncommitted.bun:testfiles use existing fixtures and no newmock.moduleseams;bun run check:test-file-capand focused suite passed.docs/releases/pending/guardrails-coder-mutation-guidance-2758.md; pending-fragment validation passed and no cache code changed.Test plan
bun --smol test tests/unit/hooks/guardrails-durable-stage-a-attribution.test.ts tests/unit/hooks/stage-a-error-classification.test.ts tests/unit/hooks/stage-a-attribution-classification-preserving.test.ts tests/unit/hooks/guardrails-stage-a-coder-mutation-failure.test.ts --timeout 60000— 18 passed, 0 failed.origin/mainand GREEN on this commit; C3 GREEN on both; checkpoint verification OK for all four paths.bun run typecheck— passed.bun run build— passed.bun run lint:ci— passed with four pre-existing warnings in unchanged base files; the two changed test files pass the scoped Biome check.bun run check:invariants— passed with pre-existing advisory findings only.bun run check:test-file-cap— passed.bun run check:pending-fragment— passed.git diff --check— passed.Known caveat
bun run package:smokereached the local-tarball npm-install stage but timed out repeatedly on this Windows host; packaging, dependencies, exports, and generated artifacts are unchanged.The PR branch is now rebased onto current
origin/main(b865ba262f48e1ccfc966c054a5ce323b62a9cdb), so the earlier base-drift condition is addressed. The follow-up head isb8f2383164956ee1364f4059c40d7069c963f65e; required remote checks must be rerun against that exact head. Keep the PR draft until the CI and approval gates are satisfied.