Skip to content

fix(workflow): complete trusted empty-scope tasks - #2776

Draft
zaxbysauce wants to merge 10 commits into
mainfrom
codex/issue-2763-empty-scope-completion
Draft

zaxbysauce wants to merge 10 commits into
mainfrom
codex/issue-2763-empty-scope-completion

Conversation

@zaxbysauce

@zaxbysauce zaxbysauce commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Closes #2763

Summary

  • Allow an explicitly trusted, generation-zero no-mutation settlement with files_touched: [] to complete without inventing a pre_check_batch receipt.
  • Bind the empty-scope exception to the matching coder-settlement WAL and transition; derive applicable gates consistently for completion, status, the evidence bridge, and phase completion.
  • Pass ledger-replayed task scope into phase-completion inference. Missing or expanded scope revokes stale empty-scope proof; legacy completion diagnostics now report the same required, satisfied, and missing gates as check_gate_status.
  • Preserve fail-closed behavior for malformed or missing settlement scope, accepted mutation, repair generations, stale Stage A evidence, and failing secretscan evidence.

Invariant audit

  • 1 (plugin init): not touched — no initialization or startup-await path changed.
  • 2 (runtime portability): touched — plain TypeScript runtime logic only; bun run build, Node ESM import of dist/index.js, and the gate-portability check passed.
  • 3 (subprocesses): not touched — no production subprocess caller changed.
  • 4 (.swarm containment): touched — evidence remains under existing validated .swarm paths; focused settlement and recovery tests pass.
  • 5 (plan durability): touched — phase inference uses the ledger-replayed plan; terminal authorization re-derives scope under the plan lock. Scope-expansion and omitted-scope regressions pass.
  • 6 (test_runner safety): not touched — validation used bounded, per-file shell test runs; no broad test_runner call.
  • 7 (test writing): touched — bun:test regressions cover empty, omitted, and expanded scope plus legacy gate parity; no new mock.module. No new test file exceeds 500 lines; the existing 522-line gate-status test shrank to 521 under the shrink-only FR-006 ratchet.
  • 8 (session state): not touched — no new session/global state.
  • 9 (guardrails/retry): touched — exception remains bound to exact scope, generation, and WAL proof; ordinary mutation and repair still require applicable gates.
  • 10 (chat/system message): not touched — no chat transform or guidance carrier changed.
  • 11 (tool registration): not touched — no metadata, manifest, barrel, or agent map changed.
  • 12 (release/cache): touched — the issue release-note fragment is included; package version and cache logic are unchanged.

Test plan

  • Frozen issue-tracer acceptance C1–C4: C1/C2 RED→GREEN; C3/C4 GREEN→GREEN, with checkpoint and deferred-work checks recorded in the issue trace.
  • Focused exact-worktree validation: 125 tests passed across 11 files, including gate applicability, phase completion, legacy parity, gate status, Turbo bypass, delegation, bridge/summary, and settlement recovery.
  • bun run typecheck; bun run lint:ci (exit 0; four non-blocking Biome warnings); targeted Biome; git diff --check; bun run build; Node ESM import.
  • Push-protection pattern scan on the follow-up diff: no credential-like matches.
  • Fresh CI and independent review for this PR head: pending; this PR remains a draft until checks and review are complete.

Acceptance criteria → evidence

Acceptance criterion Evidence
AC1: Explicit files_touched: [] with a trusted no-mutation settlement completes after applicable task gates without a fabricated file or Stage A scan. Frozen C1; tests/unit/tools/empty-scope-completion-2763.test.ts; tests/unit/evidence/gate-evidence-2763.test.ts.
AC2: Status and completion diagnostics agree on the applicable gate set. Frozen C2; tests/unit/tools/legacy-gate-parity-2763.test.ts compares required, satisfied, and missing arrays, including a satisfied reviewer receipt and missing pre_check.
AC3: Non-empty scope or accepted mutation retains current-generation pre_check. Frozen C3; existing mutation and scope regressions.
AC4: Malformed inputs, stale or omitted scope, and mutation-bearing tasks remain fail-closed. Frozen C4; tests/unit/tools/phase-complete-doc-only-evidence.test.ts proves empty scope passes while omitted and expanded scope fail.

Recurrence prevention

  • Defect class: completion/status/recovery code independently reconstructing gate applicability from empty or lossy scope instead of trusted, generation-bound evidence.
  • The repository-wide census dispositioned semantic hits; no unresolved in-class residual remains.
  • The guardrail combines a WAL trust-boundary assertion, shared typed gate derivation, terminal-WAL validation, and regressions for scope expansion and legacy parity.

Risk and rollback

  • Risk: medium; this changes durable task evidence and completion decisions, but the exception is limited to authoritative generation-zero empty settlements and remains fail-closed otherwise.
  • Rollback: revert the issue commits; the WAL field is optional, preserving legacy parsing.
  • Residual behavior: an unrelated context-free no-mutation event after settlement clears eligibility and restores pre_check by design.

Environment notes

  • Earlier PR head 01c13fd57d0de2d7babeb877059cb4a701edc2ae had green CI and passed the focused security/adversarial/smoke checks; exact-head checks for this follow-up are pending.
  • The canonical isolated integration run had one skill-optimizer-e2e production-store tripwire failure also reproduced on clean origin/main ([Observability PR 05/23] Isolate hive stores and add exact-ID quarantine with rollback #2033). The restricted-home swarm-model F-07 case likewise failed on clean main; its isolated writable-runtime run passed 19/19.
  • A supplementary co-run of the existing coder-settlement recovery test once reported a production-store tripwire after nine assertions. Its isolated rerun passed 9/9. The platform store was left untouched, and the cause of that co-run signal is not claimed to be pre-existing.

Waivers

None.

Merge status

Awaiting explicit user approval; not merged. The PR remains draft.

PR head: 2d40c56f640b573aa868f5e25fd379d3c57be940

@github-actions

Copy link
Copy Markdown
Contributor

Drift check report

Found 2 drift finding(s): 0 error, 0 warning, 2 notice.

required-check-contract (2)

  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not yet required by the captured ruleset
  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not present for every expected event in captured external workflow evidence

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes safety-critical durable task-completion, terminal WAL replay, and gate-derivation trust-boundary semantics whose fail-closed guarantees warrant final human verification.

Pull request overview

This PR fixes issue #2763, where a verification-only task that honestly declares files_touched: [] could never be marked completed because update_task_status required a pre_check gate that pre_check_batch never records for an empty file list, deadlocking the task in in_progress. It also resolves the reported disagreement between check_gate_status and update_task_status over the required-gate set by routing both (plus the durable evidence bridge and phase completion) through one shared deriveApplicableGateSet derivation. The exception is tightly scoped: it is granted only when an authoritative, generation-0 coder settlement proves an empty declared scope and no accepted mutation, and every other path (plan scope alone, null/malformed/non-empty settlement scope, accepted mutation, later generations, stale Stage A, failing secretscan) remains fail-closed.

Changes:

  • Add a generation-0 noMutationSettlement marker, bound to the trusted coder-settlement WAL, and a shared deriveApplicableGateSet/isReadOnlyNoMutationEligible derivation reused by completion, status, the durable bridge, and hasPassedAllGates.
  • Thread readOnlyNoMutation through the task-terminal transition and terminal WAL schema (with cross-field validation) so a durable, non-forced empty-scope completion replays deterministically.
  • Add extensive positive/negative/malformed/replay/idempotence/secretscan/recovery regression tests and a release fragment.
File summaries
File Description
src/gate-evidence.ts Adds the noMutationSettlement marker, deriveApplicableGateSet, WAL-bound trust adapter, reducer handling, and stricter hasPassedAllGates.
src/tools/check-gate-status.ts Routes required/passed/missing gates and status through the shared derivation and readOnlyNoMutation.
src/tools/update-task-status.ts Uses shared derivation in checkReviewerGate, ignores empty scope for Turbo bypass, and passes resolveTerminal with readOnlyNoMutation.
src/workflow/coder-settlement.ts Emits declared-scope/transition context and hardens idempotent COMMITTED replay against advisory-gate overwrite.
src/workflow/task-terminal.ts Propagates readOnlyNoMutation into the terminal event and WAL, including idempotence comparison.
src/workflow/workflow-wal-schema.ts Adds the optional readOnlyNoMutation field with backward-compat parsing and cross-field validation.
src/evidence/gate-bridge.ts Derives durable gate status via the shared function instead of treating empty required gates as complete.
scripts/retention-registry.data.ts Updates task-workflow-evidence citation line numbers for the shifted symbols.
docs/releases/pending/issue-2763-empty-scope-completion.md Adds the user-visible release fragment.
tests/** Adds and updates unit/integration coverage for the empty-scope, terminal-replay, secretscan, and receiptless paths.
Review details
  • Files reviewed: 20/20 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.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Swarm PR Review — #2776 "fix(workflow): complete trusted empty-scope tasks"

Scope: 37ee0ce8b..ea8dba687 (20 files, +1280/-128). Depth tier M (risk triggers: concurrency-state, api-schema-migrations). Profile B (Claude Code, no controller) — 10 parallel explorer lanes (6 base dimensions + 4 mandatory risk-family micro-lanes), 4 independent reviewer lanes, 1 critic challenge, all bound to pr_head_sha=ea8dba68718067cf7fd3b55d5bd01eedabd1ace9.

Verdict: BLOCK

A reviewer-confirmed, critic-upheld CRITICAL finding (evidence-gate bypass) is present and is not mitigated by an unusual/adversarial precondition — it fires on the PR's own advertised happy path.


Critical finding

F-1 (CRITICAL, confirmed, upheld by critic) — The new "trusted no-mutation" marker is not proof of no mutation

A plan task with files_touched: [] cannot be dispatched to a coder at all unless a FILE: prompt line or a declare_scope call supplies real, non-empty write authority (resolveCoderScopeSources rejects a wholly empty scope — src/scope/scope-binding.ts:954-957). So every coder-settlement whose declaredFiles is [] necessarily had non-empty write authority (src/hooks/guardrails/file-authority.ts:945-957 grants writes from the resolved scope, not from declaredFiles).

At settlement time, the real git-diff-observed mutation set is filtered through isPathWithinDeclaredScope(file, [], dir) (src/scope/path-identity.ts:85-87), which is scopeEntries.some(...) — vacuously false for an empty array. So observedFiles is always [] and accepted = observedFiles.length > 0 is always false, regardless of what the coder actually wrote to disk.

This PR's new bindTrustedNoMutationSettlement (src/gate-evidence.ts:975-1024) trusts exactly the accepted===false && declaredFiles.length===0 combination as proof of no mutation, and mints a noMutationSettlement marker that lets update_task_status(completed) skip pre_check (build/SAST/secretscan requirement) — specifically on reviewer-required phases, which are the default (src/tools/update-task-status.ts:2090-2107).

Net effect: a coder can be dispatched with files_touched: [] (the plan schema's own default when the field is omitted — src/config/plan-schema.ts:81) plus one FILE: line, write to that file, and the task completes with required_gates: [] — no pre_check, no secretscan, no SAST, no reviewer-required QA — with no forced-completion audit stamp (the new path sets neither qaExempt nor forcedCompletion, converting what used to be an auditable forced-completion into a silent one). The same deriveApplicableGateSet/hasPassedAllGates derivation this PR introduces is also consumed by phase_complete (src/tools/phase-complete.ts:287) and the evidence bridge (src/evidence/gate-bridge.ts:62-68 → preflight/diagnose/evidence-summary services), so the bypass propagates into phase completion too (confirmed: tests/unit/evidence/gate-evidence-2763.test.ts:29-52 asserts hasPassedAllGates === true for zero-gate empty-scope evidence).

This is not an adversarial/unusual-plan construction — it is the only way the PR's own advertised path can ever be reached (a files_touched: [] task with genuinely zero scope throws SCOPE_NOT_DECLARED before any settlement exists at all, so the "pure" issue #2763 scenario described in the bug report is, ironically, still not fixed by this PR).

Falsification probe: dispatch a coder for a files_touched: [] task with one FILE: src/x.ts line, have it write src/x.ts, call update_task_status(completed), and assert the task completes with no pre_check gate and no forcedCompletion/qaExempt stamp.

Suggested fix: record the settlement's declaredFiles from the resolved authority scope (binding.files), not the raw plan files_touched, so declaredFiles: [] genuinely implies zero write authority — or reject bindTrustedNoMutationSettlement whenever rawObservedFiles.length > 0, not just the post-filter observedFiles. Add a regression test where the coder writes a FILE:-granted file and completion is refused.

(Reachability/authority-chain traced independently by two separate agents — an initial reviewer and a final critic — reading src/hooks/delegation-gate.ts, src/scope/scope-binding.ts, src/scope/path-identity.ts, src/hooks/guardrails/file-authority.ts, and src/gate-evidence.ts end to end, with a concrete falsification probe on both passes.)


Important non-blockers

F-2 (HIGH, confirmed) — Legacy-migration backfill path is unreachable for its stated purpose (unwired code)

recoverCoderSettlement (src/workflow/coder-settlement.ts:970-986) early-returns for any COMMITTED WAL, so it never reaches the new backfill allowance at :309-318. The three production callers of recoverCoderSettlement therefore cannot use it to backfill a marker onto an already-stuck pre-PR #2763 task. Mitigating context (critic): an ordinary architect re-dispatch (beginCoderSettlement, coder-settlement.ts:402-451) overwrites the stale COMMITTED WAL under a new transitionId and re-enables normal settlement — so the task is not permanently wedged, just not self-healing on deploy alone as the release note implies. Net: the new branch is dead code relative to its own stated intent (project directive #2: "never ship unwired code"). Untested (RP-1, MEDIUM, merge with this finding): the only related test (empty-scope-completion-2763.test.ts:250-282) exercises the marker-present idempotent path, never the marker-absent backfill branch, and that branch also double-increments retryCount/retryHistory when it does run (bounded, capped at 3 — CONC-001/IA-004, LOW-MEDIUM) and emits a duplicate settled audit event without a resumed:true flag (RP-2, LOW).

F-3 (LOW, advisory) — Undocumented check_gate_status reporting-semantics change

check-gate-status.ts's all_passed/status computation now additionally requires workflow.state to be tests_run/complete for all tasks, not just the new empty-scope path (confirmed by the retrofit of check-gate-status-secretscan*.test.ts fixtures). No in-repo programmatic consumer depends on all_passed (it's an agent-facing diagnostic field), so this is advisory, but it's undisclosed in the release fragment and should be called out.


Confirmed non-blocking findings (grouped)

  • Retention-registry citation drift (LOW): scripts/retention-registry.data.ts's citation repoint for the touched row is incomplete — writerCitations/readerCitations were correctly shifted, but writeLimits.citation, schemaVersion, and readBound.citation still point at pre-diff line numbers that now name unrelated code. Invisible to CI (the registry test only checks non-emptiness).
  • "One shared gate derivation" claim is an overstatement (LOW, pre-existing): three hand-rolled gate-derivation blocks in update-task-status.ts (:318-333, :777-816, :1186-1241) and a fourth independent implementation in src/ci/evaluate.ts:524-553 were not migrated to deriveApplicableGateSet. All fail conservatively (no false-green), but they contradict the PR's own "no unresolved in-class residual remains" recurrence-prevention claim.
  • Missing test coverage at the phase_complete and crash-recovery boundaries (LOW/MEDIUM): no test exercises a trusted empty-scope task through phase-complete.ts's tool boundary or through recoverCoderSettlement's crash-recovery mint path — both are real production entry points for the new marker.
  • repair_idle marker-clearing is correct but untested in combination (LOW): the clearing logic at gate-evidence.ts:915-932 is structurally sound (verified: the marker cannot survive into a mutating generation regardless, due to the generation===0 gate), but no test combines repair_idle with noMutationSettlement.
  • Weak fail-closed test assertions (LOW/MEDIUM): empty-scope-completion-2763.test.ts:332-370's negative-path loop only asserts result.success === false for 3 of 4 cases, without checking why — a regression that failed for an unrelated reason would still pass.
  • Silently swallowed exception, no logging (LOW): bindTrustedNoMutationSettlement's catch block (gate-evidence.ts:1015-1020) fails closed correctly but logs nothing, making a transient WAL-read failure indistinguishable from a legitimate refusal.
  • check_gate_status vs update_task_status reporting/enforcement asymmetry (LOW): check_gate_status omits contradictorySignals/routeGateAllowsTask checks that update_task_status performs — narrower, less severe recurrence of the class of bug this PR claims to fix. No enforcement consumer found.
  • New 413-line test file is at 82.6% of the repo's 500-line test-file-cap ratchet (INFO) — currently green, flagged for future headroom.

Pre-existing, not introduced by this PR

  • update_task_status's actual completion write path never consults secretscan evidence at all (only the read-only check_gate_status diagnostic tool does) — identical on base and head; not worsened by this PR, but adjacent to F-1's trust boundary and worth its own issue.
  • A separate, unrelated null-declaredFiles branch in coder-settlement.ts:1210-1211 also forces observed=[] — untouched by this diff.

Disproved / non-findings

  • Explorer claim that "phase completion" isn't wired into the shared derivation at all: disprovedphase-complete.ts:287 calls hasPassedAllGates, which now internally uses the shared derivation (transitively wired, no diff hunk needed there). This nuance matters because it's exactly the path F-1's bypass propagates through.
  • Mixed-version WAL-schema passthrough concern (ASM-001): disproved — the new field is explicitly declared in the schema, so it survives normal (non-legacy) parsing; the passthrough comparison to a same-version sibling schema didn't apply.
  • Retention-registry's 3-line change: confirmed to be legitimately connected (line-citation bookkeeping from this PR's own insertions), not scope creep.
  • The isDuplicateTransition/assertExpectedGeneration replay path: confirmed fail-closed as claimed; the generation guard is enforced upstream before the bypass can matter.

Unverified (flagged, not confirmed or refuted)

  • The PR body's "251 passed, 0 failed" broader 22-file affected/recovery matrix claim was not independently reproduced (the exact file list wasn't available to the reviewing lane); the narrower "183 tests" claim also could not be exactly reproduced in isolation (a different subset totaled 126 in one run), though all tests examined passed.
  • A narrow rollback-timing edge case (a PREPARED-but-not-yet-COMMITTED WAL with the new marker, rolled back to old code mid-window) is plausible but not reproduced; assessed as fail-closed in the stated direction if it does occur.

Validation provenance

  • Explorer lanes (10): intent-architecture, correctness-state, tests-falsifiability, security-trust, reliability-performance, compatibility-delivery (base dimensions) + concurrency-state, api-schema-migrations, test-infrastructure, unclassified-risk (mandatory risk-family micro-lanes). ~37 raw candidates generated.
  • Independent reviewers (4): every candidate individually re-derived from source (not trusted from the explorer's claim), each with a concrete code citation and falsification probe. Classifications: 1 CRITICAL confirmed, 1 HIGH confirmed (later downgraded to MEDIUM by critic, but spawning a new HIGH finding), several MEDIUM/LOW confirmed, several DISPROVED, several PRE_EXISTING, several UNVERIFIED (disclosed above).
  • Critic: independently re-derived the CRITICAL finding's reachability chain from scratch (not trusting the reviewer's citations), strengthened rather than weakened it, corrected an overclaim (secretscan is not bypassed as an overlay — it's just never made a required gate), and surfaced one additional HIGH finding (phase-completion propagation) the reviewers had part-credited to "no consumers" without checking the full diff.
  • PR title/body compliance: the PR's own invariant-audit and acceptance-criteria tables are thorough, but two of its own claims are contradicted by code: (1) the "COMMITTED settlement replay preserves advisory gates without incrementing retry state" claim (invariant 9) is false for the marker-absent backfill branch; (2) the empty-scope exception does not actually reach the literal issue update_task_status(completed) permanently blocked for tasks with files_touched: [] — pre_check gate can never be satisfied #2763 scenario (a task with truly empty scope and no FILE:/declare_scope) — that still throws before any settlement exists.
  • Mergeability: CI was still running required checks (all previously-completed checks green) at review time; no merge conflicts.

Required actions before merge

  1. Fix F-1: bind the trust marker to the actual authority scope, not the raw (possibly-empty) plan declaration; add a test where a coder writes a FILE:-granted file under files_touched: [] and confirm completion is refused.
  2. Either wire the backfill path to an actual production entry point (F-2) or remove it and document that stuck-legacy tasks are recovered via ordinary re-dispatch.
  3. Add test coverage for the phase_complete boundary and the crash-recovery mint path with a trusted empty-scope task.
  4. Fix the retention-registry citation drift and the retry-count double-increment on backfill replay.
  5. Amend the release fragment to disclose the check_gate_status semantics change and correct/remove the "proves no mutation was accepted" claim, replacing it with an accurate description of what the marker actually attests.

🤖 Generated via /swarm-pr-review (Claude Code, Profile B) — 10 explorer lanes, 4 independent reviewers, 1 critic challenge, all bound to ea8dba68718067cf7fd3b55d5bd01eedabd1ace9.

Keep explicit empty plan preflight fail-closed while recording a trusted no-mutation proof only after a clean baseline confirms no child ran. Preserve authoritative FILE scopes, clear stale proofs on new dispatches, and classify raw empty-scope mutations as failed rework.

Review findings from PR #2776 are covered by production-path and mutation regression tests.
Refresh retention citations after the empty-scope settlement hardening.

Issue: #2763
@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

🤖 Multi-Stage PR Review

Pipeline: 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)
Commit reviewed: d1183eec4112


🔍 PR Intent

Reconstructed from PR text, issue #2763, commits, changed tests, and changed interfaces.

  • O-001: Tasks with explicit files_touched: [] and a trusted generation-0 no-mutation coder settlement can complete without pre_check_batch evidence.
  • O-002: check_gate_status and update_task_status(completed) report the same applicable gate set for a given task generation.
  • O-003: Raw workspace mutations against an empty declared scope → rework_required, not a no-mutation settlement.
  • O-004: Non-empty scope, accepted mutation, later repair generations, stale Stage A, and failing secretscan remain fail-closed.
  • O-005: The noMutationSettlement marker is cleared on any new dispatch or accepted mutation.
  • O-006: WAL replay and terminal recovery preserve the invariant.
  • O-007: Existing empty required_gates evidence (no trusted settlement) retains the pre_check obligation.

📦 Implementation Summary

The PR adds a noMutationSettlement block to TaskWorkflowMetadata, persisted only via the coder-settlement WAL at dispatch_no_mutation time for generation-0, empty-declared-scope settlements. A new exported deriveApplicableGateSet() function (used by both check_gate_status and update_task_status) reports an empty gate set when the trusted settlement proof is present; otherwise pre_check is included. The marker is cleared by dispatch_attempted and accepted_mutation. hasUnattributedEmptyScopeMutation() flags mutations against an empty declared scope, routing them to rework_required instead. WAL replay, terminal WAL schema, and terminal recovery are hardened. All gate derivation now routes through one shared function.


✅ / ⚠️ / ❌ Intended vs Actual

Obligation Status Evidence (file:line)
O-001 SUPPORTED src/gate-evidence.ts:640isNoMutationSettlementMetadata; src/gate-evidence.ts:600deriveApplicableGateSet returns empty gate set
O-002 SUPPORTED src/tools/check-gate-status.ts:237 + src/tools/update-task-status.ts:600 — both call deriveApplicableGateSet
O-003 SUPPORTED src/workflow/coder-settlement.ts:178hasUnattributedEmptyScopeMutation; src/workflow/coder-settlement.ts:216 — returns null from scopedObservedFiles
O-004 SUPPORTED src/gate-evidence.ts:730dispatch_attempted clears marker; src/gate-evidence.ts:797accepted_mutation clears marker
O-005 SUPPORTED src/gate-evidence.ts:730 (dispatch_attempted clears), src/gate-evidence.ts:797 (accepted_mutation clears)
O-006 SUPPORTED src/workflow/workflow-wal-schema.ts:489 — WAL state mismatch check; src/workflow/task-terminal.ts:258evidenceMatchesTerminal includes readOnlyNoMutation
O-007 SUPPORTED src/gate-evidence.ts:649!readOnlyNoMutation && !requiredGates.includes('pre_check')pre_check added; src/evidence/gate-bridge.ts:62missingGates updated

🚨 Confirmed Findings

[CRITICAL] reduceTaskWorkflowSnapshot task_completed branch accesses context.requiredGates which is never set on the no-mutation path

  • Location: src/gate-evidence.ts:909
  • Why it matters: When readOnlyNoMutation is true (the fix's primary feature), the reducer throws TypeError: Cannot read properties of undefined (reading 'every') because context.requiredGates is never populated by the dispatch_no_mutation event or bindTrustedNoMutationSettlement. This permanently blocks the exact task the PR intends to unblock.
  • Evidence: dispatch_no_mutation events (from src/workflow/coder-settlement.ts:238) construct context as { declaredFiles, settlementTransitionId } — no requiredGates or gates. bindTrustedNoMutationSettlement at :1001 only adds declaredFiles from the WAL and settlementTransitionId. reduceTaskWorkflowSnapshot at :622 defaults context to {}, so context.requiredGates is undefined.
  • Fix direction: In the task_completed reducer, replace context.requiredGates.every(...) with derivedGates.requiredGates.every(...) where derivedGates = deriveApplicableGateSet(/* reconstruct from evidence */). Alternatively, construct the applicableGates object in updateEvidenceForTransition before calling reduceTaskWorkflowSnapshot and pass it in.

[MEDIUM] continue in malformed-settlement test case skips the failure assertion

  • Location: tests/unit/tools/empty-scope-completion-2763.test.ts:193
  • Why it matters: The malformed-settlement case calls expect(...).rejects.toThrow(...) then continues before the expect(result.success, candidate.label).toBe(false) assertion at line 265. If settleNoMutation does NOT throw (e.g., WAL schema accepts scope: 'not-an-array'), the test silently passes with no assertion covering that case.
  • Evidence: continue at :193 skips the only assertion in the loop body that checks result.success.
  • Fix direction: After the rejects.toThrow assertion, either assert result.success === false directly for that case, or restructure the loop to have a dedicated assertion for each case type.

🔬 Unverified but Plausible Risks

  • Risk: isNoMutationSettlementMetadata at line 651 checks workflow.state === 'idle' || workflow.state === 'complete'. A task that somehow reaches complete state through another path (e.g., forced completion) and then re-enters dispatch_no_mutation would pass the state check. While repair_idle opens a new generation, the interaction surface between forced completion and the read-only path is complex and deserves a dedicated regression test.
    • What would verify it: A test exercising forced completion followed by a no-mutation dispatch on the same taskId.

🧪 Test / Coverage Gaps

  • Gap: deriveApplicableGateSet and isNoMutationSettlementMetadata have no unit tests in isolation — coverage is through integration tests. The task_completed reducer path for readOnlyNoMutation is exercised by the integration repro scripts but not by a fast unit test.
    • Evidence: tests/unit/evidence/gate-evidence-2763.test.ts and tests/unit/evidence/gate-bridge-2763.test.ts cover gate derivation and bridge but not the full reducer path including task_completed.

📋 Shipped-vs-Claimed Gaps

  • Gap: None. All four acceptance criteria have corresponding test coverage (C1/C2/C3/C4).

📝 Merge Recommendation

[BLOCK]

Two defects: one is a guaranteed TypeError crash on the primary feature path (CRITICAL), one is a silent test gap (MEDIUM).

Check Result
No CRITICAL findings reduceTaskWorkflowSnapshot TypeError on task_completed with readOnlyNoMutation
No unresolved STEALTH_CHANGE
No UNSUPPORTED obligations
Test coverage adequate ⚠️ Missing unit test for task_completed reducer path; malformed-settlement case assertion gap
No hardcoded secrets
All async errors handled
Input validation present
No broken agent role boundaries
Prompt format contracts intact
Lockfile consistent

🔁 Validation Provenance

Finding Source Outcome
context.requiredGates TypeError Reviewer confirmed, verified KEPT (CRITICAL) — traced through bindTrustedNoMutationSettlement and settlementTransitionEvent; context is { declaredFiles, settlementTransitionId }, never requiredGates
accepted: true && settlementFailed: true contradiction Reviewer confirmed DROPPEDaccepted means "mutation occurred"; settlementFailed means "settlement broke"; both true for out-of-scope mutation is by design per PR docs and test at line 95
String match error.message.includes(...) Reviewer confirmed DROPPED — maintenance risk, not a functional defect; other structural guards (WAL assertion, generation check) provide the authorization
continue skips assertion Reviewer confirmed KEPT (MEDIUM)malformed-settlement case skips expect(result.success).toBe(false)
Loop cleanup leak Reviewer confirmed DROPPEDresetSwarmState() before cleanup() in each iteration; stale directory reference is recovered on next iteration
result.success not checked by reason Reviewer confirmed DROPPED — all 4 cases fail identically (no settlement → blocked by pre_check); error reason is deterministic
Unused settlement variable Reviewer confirmed DROPPEDsettlement.evidence is used via transitionTaskWorkflowEvidence; beginCoderSettlementsettleCoderDispatch order is tested in other cases
Blind spot: task_completed reducer context access Blind-spot pass ADDED as CRITICAL above

🔒 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.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

🤖 Multi-Stage PR Review

Pipeline: 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)
Commit reviewed: 01c13fd57d0d


PR Reviewer — opencode-swarm

🔍 PR Intent

Reconstructed from PR text, issue #2763, and diff:

  • O-001: Tasks with files_touched: [] and a generation-0 no-mutation coder settlement must be completable without a pre_check gate.
  • O-002: check_gate_status and update_task_status(completed) must report the same applicable gate set (converging the historically divergent implementations).
  • O-003: Raw workspace mutations against an empty declared scope must be treated as failed mutations, not as no-mutation proof.
  • O-004: The no-mutation exception must be fail-closed: plan scope alone, malformed scope, accepted mutation, later generations, stale Stage A, and failing secretscan must all retain pre_check.

📦 Implementation Summary

The PR introduces a generation-0 no-mutation settlement path: when a coder dispatch with an empty declared scope produces no observed mutations, a trusted noMutationSettlement marker is persisted in the task evidence. A new deriveApplicableGateSet() function is the single gate-derivation source shared by both check_gate_status and update_task_status, and it reports pre_check as missing unless (a) the marker exists, (b) the workflow state is idle/complete, (c) generation is 0, and (d) the current plan scope is also empty. The hasUnattributedEmptyScopeMutation() guard prevents mutations within an empty declared scope from being silently filtered into a no-mutation proof.


✅ / ⚠️ / ❌ Intended vs Actual

Obligation Status Evidence
O-001 SUPPORTED src/gate-evidence.ts:653–688deriveApplicableGateSet skips pre_check for trusted no-mutation evidence
O-002 SUPPORTED src/evidence/gate-bridge.ts:61, src/tools/check-gate-status.ts:250–268 — both call deriveApplicableGateSet
O-003 SUPPORTED src/workflow/coder-settlement.ts:165–182hasUnattributedEmptyScopeMutation forces accepted=true / settlementFailed=true for raw mutations on empty scope
O-004 SUPPORTED src/gate-evidence.ts:619–660isNoMutationSettlementMetadata requires exact generation-0 + empty scope + idle/complete state

🚨 Confirmed Findings

None. All three reviewer-confirmed findings were false positives; see §🔁 Validation Provenance.


🔬 Unverified but Plausible Risks

None. The empty-scope path is gated by five independent conditions (WAL trust-boundary, generation, settlement metadata, current plan scope, workflow state) and the recovery path also carries the settlementFailed flag.


🧪 Test / Coverage Gaps

  • Gap: No test covers the case where a no-mutation settlement is recorded, then the plan's files_touched is expanded mid-session before completion, and completion is subsequently attempted.
    • Evidence: The test 'revokes the no-mutation exception when the current plan later gains scope' covers checkReviewerGate but not executeUpdateTaskStatus with the terminal-WAL path.
    • Severity: Low — the readCurrentTaskDeclaredFiles call in update-task-status.ts:604–608 is present; this is a missing regression test rather than a structural gap.

📋 Shipped-vs-Claimed Gaps

None.


📝 Merge Recommendation

[APPROVE]

The PR correctly implements all four obligations. The fix is precise, fail-closed by construction, and the five-condition gate (WAL trust-boundary + generation + settlement metadata + current plan scope + workflow state) provides defence-in-depth against false-positive read-only completions.

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

Confirmed findings CHALLENGED and DROPPED:

Finding File:Line Drop reason
Idempotence asymmetry (undefined vs false) task-terminal.ts:313 Both existingWal.readOnlyNoMutation === true and terminal.readOnlyNoMutation === true evaluate to false when the field is undefined or false. undefined !== true is false, which equals false !== true — idempotence holds. The WAL omit-when-false pattern is also intentional (optional boolean). FALSE POSITIVE.
Idempotence asymmetry (WAL write omit-when-false) task-terminal.ts:386 Same root cause as above. The omit-when-false pattern is intentional and correct for an optional boolean field; it does not create a mismatch with the idempotence check. FALSE POSITIVE.
Empty declared scope skips scope validation workflow-wal-schema.ts:277 When declaredFiles === [], the filter condition short-circuits and passes all candidate paths. This is correct by design: (a) scopedObservedFiles returns null for empty scopes in normal flow, so raw paths never reach this validator; (b) the recovery path intentionally preserves raw observed files (for audit) and the hasUnattributedEmptyScopeMutation guard in settleCoderDispatch already forces accepted=true + settlementFailed=true, routing the workflow to rework_required. The validation bypass is harmless here. FALSE POSITIVE.

Blind-spot findings added: None — all obligations are verified by the existing test matrix (104 focused tests + 39 compatibility tests + 117 gate-status assertions + 4 acceptance criteria fixtures).


🔒 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update_task_status(completed) permanently blocked for tasks with files_touched: [] — pre_check gate can never be satisfied

3 participants