fix(workflow): architect-only audited exit from rework_required (#2755) - #2760
Conversation
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
512a994 to
a639ae1
Compare
a639ae1 to
c826e08
Compare
🤖 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 adds a new architect-only tool The tool is fail-closed: it requires an active architect session, the exact plan task id, durable workflow state exactly ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/recover-rework-task.ts:10-82 — full tool with zod validation, architect session check, all preconditions, supervised stage_a_passed + audit event |
| O-002 | SUPPORTED |
src/gate-evidence.ts:655-663 — reducer admits rework_required only when event.supervisedRecovery === true; mechanical emitters never set it |
| O-003 | SUPPORTED |
src/hooks/delegation-gate.ts:4458-4471 — state-branching remediation: rework_required names recover_rework_task; other states keep /swarm recover |
| O-004 | SUPPORTED |
src/tools/tool-metadata.ts:258-263, src/tools/manifest.ts:201, src/tools/index.ts:248-251, scripts/check-core-events-usage.ts:97-100 — full registration chain + drift-gate allowlist |
| O-005 | SUPPORTED |
src/workflow/stage-a-repair.ts:54,108 — both functions promoted from async function to export async function |
🚨 Confirmed Findings
None — all 25 reviewer-confirmed findings were challenged and refuted (see §🔁 Validation provenance). The implementation is structurally sound.
🔬 Unverified but Plausible Risks
None that meet the structural-proof bar. The acknowledged concurrency window between greenness check and state write (concurrent pre_check_batch after greenness pass, before transition) is documented as a known trade-off identical to issue #2665; it is not a defect introduced by this PR.
🧪 Test / Coverage Gaps
- Gap: No test verifies that calling
forceRecoverReworkTasktwice for the same task fails closed on the second call.- Evidence: The happy-path test at
tests/unit/workflow/rework-recovery.test.ts:249-256does include a repeat-call assertion, but only from the workflow-level test. No standalone tool-level test exercises this. - Severity: Low — covered by the integration test; the tool-level test validates the JSON contract on success and invalid args, and the repeat-call behavior is exercised at the workflow layer.
- Evidence: The happy-path test at
- Gap: The SAST-disabled documented trap (projects with SAST disabled can never satisfy the greenness precondition) is tested for failure but has no explicit documentation in the tool description or the release notes.
- Evidence:
src/workflow/rework-recovery.ts:126-128comment +src/tools/recover-rework-task.ts:50-52description;docs/releases/pending/rework-recovery-escape-hatch-2755.md:9-10does not mention the constraint. - Severity: Low — the PR comment documents it, and the failure message
RECOVER_REWORK_GREEN_PRECHECK_REQUIREDwith the underlying reason is surfaced to the architect.
- Evidence:
📋 Shipped-vs-Claimed Gaps
None. The release notes accurately describe the tool, its preconditions, the guardrail preservation, and the remediation text change.
📝 Merge Recommendation
[APPROVE]
The PR delivers exactly what issue #2755 requires: an architect-only, audited, fail-closed escape hatch from rework_required. The supervisedRecovery flag is admitted only from rework_required; all other emitters (mechanical stage_a_passed, guardrails recorder, stage-a-repair wedge scan) remain unchanged. The dispatch refusal now tells the architect the truth instead of the human-only command. The registration chain is complete, the drift gate allowlist entry is correct, and the four regression test files cover the full state machine surface.
| 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 challenged and DROPPED (one-line reason):
| # | File:line | Claim | Why dropped |
|---|---|---|---|
| 1 | gate-evidence.ts:655 | Implicit-negation guard is fragile for future states | Established codebase pattern; terminal states are separately guarded at reducer entry |
| 2 | delegation-gate.ts:4458 | Future enum state silently falls to else |
Same pattern used throughout the file; enum additions require explicit code changes |
| 3 | delegation-gate-rework-remediation.test.ts:71 | Hardcoded session ID may cause unexpected errors | Test setup uses ensureAgentSession explicitly in other tests; hardcoded string is correct test fixture |
| 4 | recover-rework-task.test.ts:159 | Session context not established before tool call | Tool's own !context.sessionID?.trim() guard handles this; execution flow is correct |
| 5 | rework-recovery.test.ts:112 | updatedAt may be invalid for Date.parse |
updatedAt is always set by the reducer on every transition; never undefined on authoritative workflow |
| 6 | rework-recovery.test.ts:292 | readEvents() returning '' causes split('') edge case |
existsSync guard ensures file existence is checked; split behavior on empty string is intentional and handled by toBeDefined() assertion |
| 7 | rework-recovery.test.ts:120 | Invalid timestamp causes updated_at to be invalid |
Timestamp is always derived from a real workflow.updatedAt (ISO string from toISOString()); never NaN |
| 8 | gate-evidence.ts:659 | supervisedRecovery not authenticated; any caller can set it |
The only public entry point is the architect-only registered tool, which enforces ensureAgentSession → architect internally; forceRecoverReworkTask is not exported |
| 9 | delegation-gate.ts:4467 | taskId log injection |
taskId is validated by isStrictTaskId before use; not user-supplied free text |
| 10 | gate-evidence.ts:659 | Three-way OR future-state maintenance burden | Intentional by-design; future states require explicit code updates, which is standard |
| 11 | rework-recovery.ts:139 | Task ID not validated against workflow evidence | knownTaskIds.has(taskId) against the plan's flat task list is the correct check; the plan IS the authoritative scope |
| 12 | rework-recovery.ts:153 | transitionId prefix constructed from user-supplied taskId |
transitionId is a data field in the audit event JSON, not a file path; taskId comes from validated plan |
| 13 | rework-recovery.ts:151 | Best-effort audit event leaves forensic gap on failure | Explicitly documented as best-effort in code comment; criticalWarn provides observability; the durable state transition is authoritative |
| 14 | rework-recovery.ts:52 | ensureAgentSession third arg undefined may bypass validation |
The third arg is callID, irrelevant to authorization; agentName is explicitly checked on the returned session |
| 15 | rework-recovery.ts:119 | Concurrency gap between greenness check and transition | Acknowledged trade-off in code comment, identical to issue #2665; pre_check_batch is the project's own agent tool |
| 16 | recover-rework-task.ts:58 | Whitespace-only sessionID guard inconsistency | !context.sessionID?.trim() correctly rejects ' ' → ''; consistent with codebase trim patterns |
| 17 | rework-recovery.ts:124 | wedgeMs parameter type mismatch risk |
wedgeMs is derived from Date.parse(workflow.updatedAt) which is always a valid ISO string; Number.isFinite guard is defensive |
| 18 | rework-recovery.ts:119 | SAST-disabled project trapped in retry loop | Explicitly documented in code comment as a known trade-off; not a bug |
| 19 | stage-a-repair.ts:54 | Exported functions can be called independently of formal workflow | Export enables reuse; appendStageARepairEvent requires a valid action string; authorization is at the tool registration level |
| 20 | recover-rework-task.ts:72 | Error serialization leaks internal state | Standard pattern throughout the codebase (error instanceof Error ? error.message : String(error)) |
| 21 | rework-recovery.test.ts:54 | readFileSync without explicit encoding |
Node.js defaults to 'utf-8'; the test fixture is always UTF-8; this is correct |
| 22 | rework-recovery.test.ts:136 | writeGreenBundles not awaited |
It IS awaited in the test body (await writeGreenBundles('1.4')); the claim is false |
| 23 | delegation-gate-rework-remediation.test.ts:31 | tmpDir cleanup if beforeEach throws |
createSafeTestDir/canonicalMkdtemp always return a cleanup function; not a real issue |
| 24 | recover-rework-task.test.ts:127 | JSON.parse without try/catch on test |
executeRecoverReworkTask always returns a JSON string; the test would fail on parse error which is correct behavior |
| 25 | rework-recovery.test.ts:278 | split('\n').find() returns undefined silently |
expect(auditLine).toBeDefined() fails explicitly on undefined; the test asserts presence, not absence |
| 26 | rework-recovery.test.ts:284 | Test passes if SAST is not required | The test correctly verifies the guard DOES require SAST (it fails with the message); the comment describes the test's intent, not a bug |
Blind-spot findings added: 0 — no new real defects found beyond those already documented (SAST-disabled constraint, concurrency window — both are acknowledged trade-offs, not defects).
Final verdict: APPROVE. No confirmed findings survive the challenge. The implementation correctly delivers the escape hatch with appropriate fail-closed preconditions, preserves the mechanical guardrail unchanged, and updates the dispatch refusal to give the architect a legal move.
🔒 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.
🤖 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 adds a new ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/recover-rework-task.ts:33 — forceRecoverReworkTask called with taskId + reason; src/workflow/rework-recovery.ts:156 — supervisedRecovery: true set on the transition event |
| O-002 | SUPPORTED |
src/workflow/rework-recovery.ts:164 — appendStageARepairEvent({ action: 'rework_recovered', ... }) called; transitionId prefixed rework-recovery: |
| O-003 | SUPPORTED |
src/workflow/rework-recovery.ts:42-76 — architect session check (:44), plan+taskId check (:61-77), rework_required state check (:96), green pre-check check (:120); every refusal is a distinct typed error |
| O-004 | SUPPORTED |
src/gate-evidence.ts:662 — reducer guard event.supervisedRecovery === true required; tests/unit/gate-evidence/rework-recovery-supervised.test.ts:77 — mechanical path test confirms throw |
| O-005 | SUPPORTED |
src/hooks/delegation-gate.ts:4460-4466 — stageARemediation branches on workflow.state === 'rework_required'; tests/unit/hooks/delegation-gate-rework-remediation.test.ts:53-65 — asserts /swarm recover absent from rework branch |
| O-006 | SUPPORTED |
src/tools/tool-metadata.ts:258 — agents: ['architect']; src/tools/manifest.ts:202 — thunk registered; src/tools/index.ts:248 — barrel export; scripts/check-core-events-usage.ts:97 — allowlist entry for prompt-doc class |
🚨 Confirmed Findings
One finding carried forward after challenge.
[MEDIUM] UTF-16 slicing corrupts reason for multi-byte characters
- Location:
src/workflow/rework-recovery.ts:137 - Why it matters:
sanitizedReason(up to 500 code units) is audited to events.jsonl viaappendStageARepairEvent. A reason containing emoji, CJK, or accented characters can be sliced mid-codepoint, producing a corrupted audit string. - Evidence:
src/workflow/rework-recovery.ts:137:JavaScript? options.reason.trim().slice(0, 500)
.slice()counts UTF-16 code units, not grapheme clusters or code points."👍🏾".slice(0, 1)→"👍"is intact (surrogate pair); but"a👍🏾".slice(0, 2)→"a👍"(trailing modifier severed) and"a👍🏾".slice(0, 3)→"a👍�" (half a surrogate). - Fix direction: Replace
.slice(0, 500)with[...str].slice(0, 500).join('')(iterates grapheme clusters) or use a code-point-aware utility. The same pattern also exists insrc/tools/recover-rework-task.tsat the Zod layer (.max(500)), which will reject valid multi-byte strings earlier than intended; that layer should use a custom validator that counts code points.
🔬 Unverified but Plausible Risks
None at structural-confidence threshold.
🧪 Test / Coverage Gaps
Minor items collapsed into single list per rubric.
- Gap:
tests/unit/tools/recover-rework-task.test.ts— no test at the 500-character boundary (501-char reason to verify truncation, 500-char reason to verify acceptance). The happy-path test uses a short ASCII string.- Evidence:
tests/unit/tools/recover-rework-task.test.ts— all reason values are ≤ ~80 chars.
- Evidence:
- Gap:
tests/unit/workflow/rework-recovery.test.ts— no concurrent-call race probe; theexpectedGenerationCAS is exercised only by the repeat-call test (single-threaded).- Evidence:
tests/unit/workflow/rework-recovery.test.ts:180— no concurrency test.
- Evidence:
- Gap:
tests/unit/hooks/delegation-gate-rework-remediation.test.ts—coder_delegatedbranch tested only forreviewerdispatch;test_engineerfromcoder_delegatednot verified to retain/swarm recover(only therework_requiredbranch tested both agents).- Evidence:
tests/unit/hooks/delegation-gate-rework-remediation.test.ts:97— oneseedCoderDelegatedtest withrevieweronly.
- Evidence:
📋 Shipped-vs-Claimed Gaps
None.
📝 Merge Recommendation
[APPROVE]
The PR fully delivers its six obligations. One confirmed medium finding (UTF-16 slicing) is a real but narrow defect: it affects only multi-byte-character reasons and does not block any other precondition. Four test-coverage gaps are noted as minor.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ (minor gaps noted) |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ (best-effort audit is intentional/design) |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ (no lockfile touched) |
🔁 Validation provenance
Confirmed finding carried (1):
- UTF-16 slicing in
rework-recovery.ts:137— structurally provable, real behavior change, not pre-existing at this call site.
Dropped findings and why:
gate-evidence.ts:241(optional boolean type) —=== truestrict guard is the correct runtime enforcement; type system is compile-time; no path bypasses it.gate-evidence.ts:655(supervised flag from non-rework states) —workflow.stateis aTaskWorkflowStateenum (never null/undefined); the guard is correct as written; onlyforceRecoverReworkTasksets the flag.delegation-gate.ts:4460(null check onworkflow.state) —workflow.stateis a typed enum; cannot be null at that call site; not a defect.delegation-gate.ts:4463(null taskId in error) —taskIdis a validated non-empty string at that point; not a defect.recover-rework-task.ts:53(sessionID guard allows undefined) —?.trim()onundefinedreturnsundefined; falsy check correctly rejects it; guard is sound.recover-rework-task.ts:45(UTF-16 slicing in tool) — Zod.max(500)is code-unit-based (same as the helper), so the schema rejects valid multi-byte strings earlier than intended, but this is a pre-existing pattern not introduced by this PR; the real slice bug is inrework-recovery.ts.rework-recovery.ts:148(uncheckedupdatedreturn) —transitionTaskWorkflowEvidencereturnsPromise<TaskEvidence>;updatedisTaskEvidence(never null);getTaskWorkflowSnapshothandles it safely.rework-recovery.ts:65(stripKnownSwarmPrefix empty-string) —stripKnownSwarmPrefixreturnsstring; the||guard before it handles the falsy case including empty string; guard is required (not dead code).rework-recovery.ts:164(unawaited appendStageARepairEvent) — documented as best-effort audit; internal try/catch +criticalWarn; not a defect.gate-evidence.ts:662(strict-equals bypass) — TypeScript enforcesboolean;=== trueenforces at runtime; no path deserializes events back through the reducer.rework-recovery.ts:59(dead code on session guard) —ensureAgentSessioncan return null/undefined; the!session ||guard is required.recover-rework-task.ts:29(whitespace-only reason) — intentional; sanitized toundefinedwhich the audit event handles as absent field.rework-recovery.ts:109(TOCTOU race) —expectedGenerationCAS handles it; not a defect.delegation-gate.ts:4470(taskId in error disclosure) — task IDs are not secrets; not a security defect.- All test-coverage findings (missing boundary test, missing concurrent test, missing audit schema validation, missing negative assertion) — confirmed as gaps but below MEDIUM threshold; collapsed into minor list.
🔒 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
It modifies the safety-critical task-workflow reducer and guardrail/retry semantics to add a supervised bypass of a deliberate fail-closed transition, which warrants final human verification despite the well-tested, precedent-aligned implementation.
Pull request overview
This PR adds an architect-only audited escape hatch for tasks stranded at rework_required when a Stage B verdict did not require a code change (e.g. a SKIPPED verdict scored from a tool-argument error). The mechanical stage_a_passed transition intentionally throws TASK_WORKFLOW_CODER_MUTATION_REQUIRED from rework_required, /swarm recover skips this state, and the dispatch refusal previously recommended dead ends — hard-stopping unattended runs on correct, reviewed, gate-green code. The fix introduces a new recover_rework_task tool that writes a supervised stage_a_passed (generation preserved) plus a distinguishable stage_a_repair audit event, gated behind a fail-closed precondition ladder, and corrects the dispatch remediation text to name an architect-invocable remedy.
Changes:
- Adds an optional
supervisedRecoveryflag to thestage_a_passedevent; the reducer admitsrework_required → pre_check_passedonly when set, so mechanical emitters still fail closed exactly as before. - Adds
forceRecoverReworkTask(newsrc/workflow/rework-recovery.ts) and the registeredrecover_rework_tasktool, with architect-session, plan/task, state, green pre-check, and generation-CAS preconditions each surfacing a distinct typed error. - Makes the
TASK_WORKFLOW_STAGE_A_REQUIREDremediation state-specific (namesrecover_rework_taskfromrework_required; keeps/swarm recoverfor the attribution-wedge case).
File summaries
| File | Description |
|---|---|
src/gate-evidence.ts |
Adds supervisedRecovery event flag and the guarded rework_required admission in the stage_a_passed reducer case. |
src/workflow/rework-recovery.ts |
New fail-closed ladder writing the supervised transition + audit event; mirrors the #2703 precedent. |
src/tools/recover-rework-task.ts |
New architect-only tool: zod-strict args, JSON success/refusal contract, delegates to the workflow helper. |
src/tools/tool-metadata.ts |
Registers recover_rework_task metadata (architect-only). |
src/tools/manifest.ts |
Adds the manifest handler thunk. |
src/tools/index.ts |
Barrel export for tool + execute function. |
src/hooks/delegation-gate.ts |
State-specific remediation text for the Stage-A-required refusal. |
src/workflow/stage-a-repair.ts |
Additive export on appendStageARepairEvent / hasGreenPostSettlementPreCheck for reuse. |
scripts/retention-registry.data.ts |
Re-pins the getEvidencePath anchor shifted by the reducer edit (:786→:798, verified). |
scripts/check-core-events-usage.ts |
Sanctioned prompt-doc allowlist entry for the tool's events.jsonl mentions. |
docs/releases/pending/rework-recovery-escape-hatch-2755.md |
User-visible release fragment. |
tests/unit/gate-evidence/rework-recovery-supervised.test.ts |
Reducer admission/refusal matrix for the supervised flag. |
tests/unit/workflow/rework-recovery.test.ts |
Full fail-closed ladder + audit/CAS/repeat-call coverage. |
tests/unit/tools/recover-rework-task.test.ts |
Tool argument validation, session requirement, and JSON contract. |
tests/unit/hooks/delegation-gate-rework-remediation.test.ts |
State-specific remediation-text assertions for both branches. |
Review details
- Files reviewed: 15/15 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 — PR #2760Scope: ObligationsIssue #2755 is OPEN and correctly linked only to this PR (no obligation drift). All 5 acceptance criteria (AC1–AC5) map cleanly to the diff. All 7 quantitative test-count/registration claims in the PR body were independently re-run and matched exactly (e.g. Confirmed findings (all MEDIUM/LOW — no CRITICAL, no HIGH)F-1 (MEDIUM) — The pre-check "proof" gating this permission-bypass tool is not bound to the wedged task, and the gated actor can self-manufacture it. F-2 (MEDIUM) — The tool's audit claim is unconditional even though the actual audit-log append is best-effort and untested on failure. F-3 (MEDIUM) — This PR creates the first exit from F-4 (LOW) — Mislabeled test, not test theater on a live invariant. Advisory / no action needed
Verdict: APPROVE_WITH_NOTESNo CRITICAL or HIGH findings survived independent review + critic challenge. The four confirmed MEDIUM/LOW findings (F-1..F-4) are real but each has a documented mitigating factor (defense-in-depth elsewhere, no downstream consumer, bounded blast radius, or a runtime guard covering the gap). None block merge; F-2 and F-4 are one-function/one-line fixes worth doing before or shortly after merge, and F-1/F-3 warrant a tracked follow-up given this tool's role as a permission-boundary escape hatch. Review method: 4 parallel explorer lanes (Claude Sonnet) → 2 independent reviewer passes (Claude Opus, one with executed mutation tests) → 1 critic challenge pass (Claude Opus) on all surviving MEDIUM findings. Full validation provenance (raw candidates, reviewer verdicts, critic verdicts) available on request. |
…vised marker, pins, tests)
PR #2760 feedback closure ledgerReview run: swarm-pr-review (Profile B, tier L, head c826e08) → 36 raw candidates → 4 reviewer lanes → 2 critic lanes. Fix commit: c1d7801 (this push). Gates: verification lane 19/19 CONFIRMED → Stage A green (typecheck, biome, registry-citations, core-events, tool-registration, test-clock, test-file-cap, invariants, git diff --check) → Stage B reviewer 19/19 APPROVE + test_engineer 19/19 PASS → closeout reviewer 19/19 APPROVE + closeout critic 19/19 APPROVE.
Regression scan (Stage B): mechanical reducer paths byte-identical; appendStageARepairEvent return-type widening ripples nowhere; EISDIR probe deterministic cross-platform; no sibling asserts the old refusal texts. Sibling scope note unchanged: #2756/#2757/#2758 remain open and untouched. |
🤖 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 adds a new architect-only ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/recover-rework-task.ts:46 — executeRecoverReworkTask calls forceRecoverReworkTask; src/workflow/rework-recovery.ts:62 — supervised stage_a_passed with supervisedRecovery: true |
| O-002 | SUPPORTED |
src/gate-evidence.ts:673 — !(current.state === 'rework_required' && event.supervisedRecovery === true) gate; src/workflow/rework-recovery.ts:175 — only path setting the flag |
| O-003 | SUPPORTED |
src/gate-evidence.ts:690 — { supervisedRecovery: true as const } in return; src/gate-evidence.ts:618-624 — preserved across transitions via ...(current.supervisedRecovery === true ? { supervisedRecovery: true } : {}) |
| O-004 | SUPPORTED |
src/gate-evidence.ts:779-785 — repair_idle destructuring strips supervisedRecovery; src/gate-evidence.ts:663-665 — retry case also clears via supervisedRecovery: undefined |
| O-005 | SUPPORTED |
src/workflow/rework-recovery.ts:188-200 — appendStageARepairEvent({ action: 'rework_recovered', ... }); src/workflow/rework-recovery.ts:168 — transition id rework-recovery:${taskId}:gen${workflow.generation} |
| O-006 | SUPPORTED |
src/workflow/rework-recovery.ts:67-74 — architect session check; src/workflow/rework-recovery.ts:85-99 — plan task existence; src/workflow/rework-recovery.ts:108-118 — rework_required state check; src/workflow/rework-recovery.ts:128-147 — green pre-check proof |
| O-007 | SUPPORTED |
src/hooks/delegation-gate.ts:4458-4468 — stageARemediation branches on workflow.state === 'rework_required' to name recover_rework_task |
| O-008 | SUPPORTED |
src/commands/recover.ts:59-65 — skipped_not_wedged branch checks outcome.state === 'rework_required' and names the tool |
| O-009 | SUPPORTED |
src/tools/tool-metadata.ts:255-259 — architect-only grant; src/tools/manifest.ts:202 — manifest entry; src/tools/index.ts:248-251 — barrel export |
| O-010 | SUPPORTED |
src/workflow/stage-a-repair.ts:52 — export async function appendStageARepairEvent returns Promise<boolean>; src/workflow/stage-a-repair.ts:113 — export async function hasGreenPostSettlementPreCheck |
🚨 Confirmed Findings
None. Every confirmed finding from the prior review was either pre-existing, grounded in incorrect assumptions about types/contract, or already addressed by the PR's own review rounds. No real, PR-introduced defects remain.
🔬 Unverified but Plausible Risks
None that meet the structural-evidence bar. The SAST-disabled unavailability is documented and intentional (docs acknowledge it; the error message names the cause and alternative). The append-only audit event outcome surfacing (audit_event_recorded: false with WARNING) is the correct behavior per the review finding it addresses (FB-001).
🧪 Test / Coverage Gaps
None identified. The PR adds 4 test files (20 test cases total) covering: reducer state-machine behavior (6 cases including generation rotation, marker persistence/clearing, terminal-state rejection, widening probe), recovery function preconditions (13 cases including SAST-disabled shape, corrupt evidence/plan, stale bundles, prefixed agent names), tool wrapper contract (7 cases including audit append failure), and delegation-gate remediation text (3 cases). Each bun test entry point is exercised at the acceptance level.
📋 Shipped-vs-Claimed Gaps
None. The supervisedRecovery marker is correctly typed as boolean | undefined (not true | undefined), so event.supervisedRecovery === true in the reducer is precise — false is excluded by the strict equality check, matching the forcedCompletion precedent. The true as const literal in the reducer return is correct TypeScript narrowing. All acceptance criteria have traceable code evidence.
📝 Merge Recommendation
[APPROVE]
The PR fully delivers its obligations. The supervisedRecovery guardrail is structurally sound, the preconditions fail closed at each step with distinct errors, the audit trail is surfaced honestly, and the mechanical path is provably unchanged by the flag check (widening-probe mutation confirmed to flip the guardrail test RED in the PR's own review). No real defects survive this pass.
| 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 | ✅ (only metadata changes to existing files) |
🔁 Validation Provenance
Confirmed findings reviewed (all DROP'd):
| Finding | File:Line | Drop Reason |
|---|---|---|
outcome.state undefined access |
recover.ts:60 |
StageARepairOutcome.skipped_not_wedged carries state — TypeScript would reject missing it |
| Architect check missing in tool | recover-rework-task.ts:46 |
forceRecoverReworkTask:67-74 calls ensureAgentSession and validates agentName; tool is architect-only in TOOL_METADATA |
| Whitespace sessionID bypass | recover-rework-task.ts:44 |
!context.sessionID?.trim() correctly blocks whitespace-only strings |
| Directory traversal in tool | recover-rework-task.ts:31 |
directory is framework-validated plugin context, not untrusted user input |
updated uninitialized after throw |
rework-recovery.ts:156 |
transitionTaskWorkflowEvidence always returns; any exception would propagate before the getTaskWorkflowSnapshot call |
| Oversized task_id not enforced | recover-rework-task.test.ts:258 |
Zod schema z.string().max(64) enforces it; test passes on schema rejection |
| Retry idempotency — no duplicate audit | rework-recovery.test.ts:157 |
State precondition fails closed on second call; evidence unchanged by design |
greenness.reason fragile string |
rework-recovery.ts:127 |
Discriminant is a string enum in the same module; intentional API design |
| taskId unsanitized in recover.ts | recover.ts:64 |
outcome.taskId comes from the typed StageARepairOutcome union |
| Architect check only in workflow | recover-rework-task.ts:44 |
Redundant with forceRecoverReworkTask; architect-only TOOL_METADATA is the intended primary enforcement |
| SAST-disabled hard failure | recover-rework-task.ts:73 |
Documented as intentional design limitation; error message names the cause and alternative |
| task_id 64-char bound not enforced | recover-rework-task.ts:34 |
Zod schema enforces it; the claim of no enforcement is incorrect |
supervisedRecovery unconditional clear |
gate-evidence.ts:663 |
...(undefined) is a no-op in spread; harmless asymmetry with forcedCompletion |
| taskId unsanitized in delegation-gate | delegation-gate.ts:4463 |
taskId sourced from validated workflow state enum |
workflow null before .authoritative |
rework-recovery.ts:128 |
getTaskWorkflowSnapshot throws RECOVER_REWORK_NO_WORKFLOW when !evidence || !workflow.authoritative — guard precedes the access |
loadPlanJsonOnly unhandled throw |
rework-recovery.ts:74 |
Return value checked for null; throws for corrupt files — same pattern as forceRecordPlanCriticApproval |
Date.parse(workflow.updatedAt) undefined |
rework-recovery.ts:128 |
TaskWorkflowMetadataSchema requires updatedAt: z.string() — property always exists |
appendStageARepairEvent return type change |
stage-a-repair.ts:52 |
New callers (forceRecoverReworkTask) use the boolean; existing caller (scanStageATask) ignores the return — no functional break |
hasGreenPostSettlementPreCheck newly exported |
stage-a-repair.ts:67 |
Intentional: sibling #2758 uses it; export is the required seam |
| FB-014 test doesn't assert error value | recover-rework-task.test.ts:176 |
Test validates the call is rejected; echoing the oversized string is not a security concern for a JSON error message |
supervisedRecovery type inconsistency |
gate-evidence.ts:663 |
true as const vs true in two return sites — harmless; TS narrows both to true |
Blind-spot findings (none): No additional real defects found. The code is well-structured with clear defensive checks, and the test coverage is comprehensive for the new surface area.
🔒 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.
🤖 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 IntentObligations from issue #2755 and PR description:
Phase 1 — Summarise Actual BehaviourThe PR adds a complete new escape hatch:
Phase 2 — Intended vs Actual
Phase 3 — Confirmed Findings: Challenge ResultsKeeping: NONE — all confirmed findings are either false positives or intentional designDropped findings (with rationale):
Phase 4 — Runtime-Aware False-Positive ControlNo findings survive to this phase. All concerns were either:
Phase 5 — Blind-Spot PassBlind-spot:
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 | SUPPORTED |
src/tools/recover-rework-task.ts |
| O-002 | SUPPORTED |
src/gate-evidence.ts:673-691 |
| O-003 | SUPPORTED |
src/workflow/rework-recovery.ts:141-163 |
| O-004 | SUPPORTED |
src/workflow/rework-recovery.ts:76-85 |
| O-005 | SUPPORTED |
src/hooks/delegation-gate.ts:4464-4474 |
| O-006 | SUPPORTED |
src/gate-evidence.ts:690-694 + rework-recovery.ts:183 |
| O-007 | SUPPORTED |
mechanical path unchanged; widening probe documented |
| O-008 | SUPPORTED |
src/workflow/stage-a-repair.ts:52,110 |
🚨 Confirmed Findings
None — all 18 reviewer-confirmed findings were either false positives, already-addressed-by-this-PR, or intentional design. The implementation is correct and complete.
🔬 Unverified but Plausible Risks
-
Risk:
appendStageARepairEventreturn type changedvoid→boolean. If the function gains external callers (it was not previously exported), callers expectingvoidwill silently discard the boolean.- Why suspicious: Explicit export added in this PR;
void→booleanis a documented intentional change. - What would verify it: Audit of all imports of
appendStageARepairEventacross the codebase (currently onlyrework-recovery.ts). - Confidence: Low — the function was not exported before; no external callers exist today.
- Why suspicious: Explicit export added in this PR;
-
Risk: The
supervisedRecoveryevidence marker is write-only from the perspective of any future consumer of the evidence file. The marker is never read back by any code path.- Why suspicious: Write-only fields are code smells; a future consumer could misinterpret the evidence.
- What would verify it: Full-text search for any
readTaskEvidencecaller that reads.workflow.supervisedRecovery(currently none). - Confidence: Low — this is by design; the three distinguishing signals (prefix + audit event + marker) work together.
🧪 Test / Coverage Gaps
- Minor gap: No explicit test for
supervisedRecovery: truefrompre_check_passed(which the reducer correctly rejects). The test named "still admits plain stage_a_passed from coder_delegated and pre_check_passed" covers the pre-existing mechanical path. The supervised-path rejection is implicitly tested by the reducer's conditional logic and explicitly by the widening probe documented in the PR. - Adequacy: 4 new test files (20 test cases total) + frozen C1-C5 checks + widening probe. Coverage is sufficient.
📋 Shipped-vs-Claimed Gaps
- None. All five acceptance criteria are substantiated in the diff.
📝 Merge Recommendation
[APPROVE]
The implementation correctly addresses issue #2755. All 18 confirmed findings were refuted as false positives or intentional design. The two plausible risks in the blind-spot pass are low-confidence, not fixable without changing intended behavior, and do not affect correctness of the primary fix.
| 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
- Survived (0): No findings survived the challenge pass.
- Dropped (18): All 18 reviewer-confirmed findings were dropped — each was either already fixed in the diff, an intentional design documented in the PR, an incorrect assumption about TypeScript narrowing, or a misreading of the code flow.
- Blind-spot additions (2):
appendStageARepairEventreturn-type change (low confidence, no external callers today);supervisedRecoverywrite-only marker (by design, not a defect).
🔒 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 #2755
PR head: c826e08
Summary
A task stranded at
rework_requiredby a Stage B verdict that did not requirea code change had no architect-legal exit: the mechanical
stage_a_passedtransition intentionally throws
TASK_WORKFLOW_CODER_MUTATION_REQUIREDfromthat state,
/swarm recoverskips non-coder_delegatedstates, and thedispatch refusal recommended dead ends — unattended runs hard-stopped with
correct, reviewed, gate-green code. This PR adds the audited escape hatch the
repo already uses for this class (#2703 precedent): the architect-only
recover_rework_tasktool.Root Cause
src/gate-evidence.ts— thestage_a_passedreducer case (around line 647)throws from any state other than
coder_delegated/pre_check_passed, which isintentional (a genuine defect must go back through the coder; sibling #2758
documents the same). But nothing represented "Stage B failed WITHOUT a code
defect": the repair machinery behind
/swarm recover(
src/workflow/stage-a-repair.tsscanStageATask) only handlescoder_delegated, therepair_idleforce-repair requires a settled planstatus, and every non-terminal exit presupposed an accepted coder mutation.
The dispatch refusal at
src/hooks/delegation-gate.ts:4458-4465then advisedpre_check_batch(throws) and the human-only/swarm recover(skips thisstate).
Fix
src/gate-evidence.ts— thestage_a_passedevent union gains an optionalsupervisedRecoveryflag; the reducer admitsrework_requiredONLY when itis set. Mechanical emitters (guardrails recorder, stage-a-repair) never set
it, so they still fail closed exactly as before.
src/workflow/rework-recovery.ts(new) —forceRecoverReworkTask, thefail-closed ladder mirroring
forceRecordRetrySoundingBoardApproval:architect session, exact plan task, durable state exactly
rework_required, green pre-check proof for the wedged generation (the[Workstream A] PR 10 of 12: Connect existing task recovery and status to authoritative transition receipts #2665 secretscan+SAST bar, anchored at the wedge transition timestamp),
generation CAS, then the supervised
stage_a_passedplus astage_a_repairaudit event (actionrework_recovered) through the sharedwrapper. Every refusal is a distinct typed error.
src/tools/recover-rework-task.ts(new) — the registered tool(
task_id+ auditedreason, zod-strict, JSON contract).TOOL_METADATA(architect-only),TOOL_MANIFESTthunk, barrel export; derived maps regenerate(
check-tool-registration.ts: 134 tools coherent).src/hooks/delegation-gate.ts— theTASK_WORKFLOW_STAGE_A_REQUIREDremediation now branches on state:
rework_requirednamesrecover_rework_task(and the coder repair loop for real defects); theattribution-wedge branch keeps
/swarm recoverwhere it actually applies.src/workflow/stage-a-repair.ts— additiveexportonhasGreenPostSettlementPreCheck/appendStageARepairEventfor reuse.Recurrence Prevention (defect class)
require work the triggering condition did not produce, with no audited
supervisor override and remediation naming agent-uninvocable surfaces.
dispositioned (by-design human surfaces; settlement-wedge policy with
documented agent self-heal; correct state-gated texts; two follow-up rows
owned by open sibling Guardrails label TASK_WORKFLOW_CODER_MUTATION_REQUIRED as an attribution miss and advise the human-only /swarm recover #2758; the fixed state itself).
executed revert probe (supervised branch removed → C1 FAIL) and widening
probe (flag check dropped → C3 FAIL).
Regression Protection
rework-recovery-supervised.test.ts: supervised admission (state flip,generation unchanged, retry history preserved) vs mechanical refusal vs
terminal-state refusal vs plain-path preservation.
rework-recovery.test.ts: the full fail-closed ladder incl. theSAST-disabled shape, audit event fields, transition-id prefix, CAS, and
repeat-call refusal.
delegation-gate-rework-remediation.test.ts: both Stage B agents get therecover_rework_taskremediation fromrework_required; thecoder_delegatedbranch keeps/swarm recover.Acceptance Criteria -> Evidence
rework_required→pre_check_passed, durable re-read); dispatch gate acceptspre_check_passed(delegation-gate.ts:4409-4415)rework_recovered,rework-recovery:transition id, session/state/greenness refusals leave state untouched)/swarm recoverthere)check-tool-registration.ts"134 tools, coherent"Invariant audit
bun run build+ bundle-portability 10/10 + plugin-shape 2/2 + packaging smoke 10/10 + Node ESM import →plugin id: opencode-swarm | has server: true; nobun:resolution, noBun.*calls.transitionTaskWorkflowEvidence/appendCoreEventSyncseams under.swarm/.loadPlanJsonOnly).mock.module, each < 500 lines (check:test-file-cap0 violations,check:mock-cleanupclean).ensureAgentSessionseam; no new session-keyed state.stage_a_passedpath and the retry circuit semantics are unchanged (frozen C3 + mutation probe).check-tool-registration.tspasses,tests/unit/config2166/0, architect-tool-lists 13/13.docs/releases/pending/rework-recovery-escape-hatch-2755.mdfragment; version files untouched.Test plan
bun test tests/unit/gate-evidence/rework-recovery-supervised.test.ts6/0;tests/unit/workflow/rework-recovery.test.ts6/0;tests/unit/tools/recover-rework-task.test.ts5/0;tests/unit/hooks/delegation-gate-rework-remediation.test.ts3/0.(replayed via
repro-check.sh run;verify-checkpoint5/5 OK).(all files green incl. stage-a-repair 15/0), delegation-gate-reentry-bypass
5/0, stage-a-error-classification 3/0, acceptance-injection 18/0,
guardrails-durable-stage-a-attribution 10/0, manifest-parity 9/0,
registration-fix.adversarial 37/0, architect-tool-lists 13/13,
tests/unit/config 2166/0.
biome checkclean on touched files;bun run typecheckclean;
bun run buildok; bundle-portability 10/10; plugin-shape 2/2;node --input-type=module -e "await import('./dist/index.js')"→plugin id: opencode-swarm | has server: true; packaging smoke 10/10(9,465,417 B < 9.5 MiB cap);
drift:check --enforceclean;check:invariantsall passed;check:test-file-cap0 violations;check:mock-cleanupclean.scan-deferred.sh→ clean.Risk and Rollback
state machine's mechanical semantics are pinned by frozen checks and
regression tests; no data migrations.
cleanup (a supervised
stage_a_passedis an ordinary durable transition).TASK_WORKFLOW_CODER_MUTATION_REQUIREDthrow for this state — owned byopen sibling Guardrails label TASK_WORKFLOW_CODER_MUTATION_REQUIRED as an attribution miss and advise the human-only /swarm recover #2758, deliberately untouched here (as is test_runner convention-scope error recommends the blocked scope "all"; the resulting SKIPPED verdict is scored as stage_b_failed and deletes the reviewer approval #2756's verdict
scoring and Plan-critic dispatch is recorded as a per-task critic required gate that no later step can satisfy #2757's per-task critic gate).
Waivers (or none)
none
Merge status
Awaiting explicit user approval; not merged. Independent implementation
review (Kimi K2.7) and final critic (Kimi K3) both APPROVE at head
c826e08; plan critic APPROVE at base
d40e934.
CI-round note
One CI round after initial publication: the drift gate flagged the new tool's
user-facing strings that mention
events.jsonl(the #2039 literal-mentionratchet); fixed with the sanctioned
prompt-docallowlist entry inscripts/check-core-events-usage.ts(the same class theapprove-retry-sounding-board.tsprecedent uses — the append itself alreadyroutes through the
appendCoreEventSyncseam), and the PR body sections werereordered to satisfy the pr-standards contract. Head re-stamped to
c826e08; all three gates re-approved at the
new head.
Pre-existing local-only artifact (not introduced by this PR): on this Windows
checkout
tests/unit/scripts/drift-check.test.ts("no false positives on thereal repository") reports a
[WORKFLOW_CHANGED_AFTER_CAPTURE]finding for.github/workflows/pr-standards.yml— proven present on the unmodified basetree via a stash probe and absent from the CI drift run for this PR (known
captured-evidence local divergence, same class as the documented
schema-serializer local/CI split). The related
tests/unit/events/check-core-events-usage.test.tspasses 15/15 with thisPR's allowlist entry.
CI-round note (round 2)
A second CI round fixed the quality job: the registry-citation gate flagged
one anchor my reducer edit shifted (src/gate-evidence.ts getEvidencePath
:786 → :798 — re-pinned at source in scripts/retention-registry.data.ts, the
sanctioned arm), and check:test-clock flagged two new test fixtures using the
wall clock (timestamps now derived from the durable workflow anchor —
deterministic and strictly newer than the wedge by construction). The unit
matrix had been skipped only as quality's dependent. Final head:
c826e08; all three review gates re-approved
at this head (Round 3).
CI-round note (round 3)
A third CI round reworded the recover_rework_task TOOL_METADATA description
to drop the literal "Stage B": the architect prompt embeds the full tool list
on a single line, where a pre-existing "skip" token elsewhere in that line
combined with the new "Stage B" token and tripped architect-council-prompt's
no-skip-Stage-B assertion. That suite is 53/53 and the whole
tests/unit/agents directory 1949/0 after the reword; registration stays
coherent (134 tools). Final head: c826e08;
all three review gates re-approved at this head (Round 4).