fix(delegation-gate): require explicit critic task attribution

[zaxbysauce]

Closes #2757

Summary

• Require explicit, strict numeric task attribution for the critic family before durable task-gate evidence is recorded; critic attribution no longer falls back to ambient session task state.
• Prevent a non-strict named ID from shadowing a valid numeric TASK: marker while preserving reviewer and test-engineer plan-list routing.
• Cover accepted-mutation re-gating, standalone-marker parsing, architect prompt guidance, shared task-ID fields, Windows-safe fixture cleanup, and plan-free PR-review re-entry with a stale session task ID.
• Add the required pending release-note fragment.

Invariant audit

• 1 (plugin init): not touched — no plugin initialization code changed; bun run build passed.
• 2 (runtime portability): not touched — no runtime loader, export, or bundler configuration changed.
• 3 (subprocesses): not touched — no production subprocess call site changed.
• 4 (.swarm containment): not touched — no runtime state sink or working-directory resolver changed; the new fixture stays under the test temp root and uses bounded cleanup.
• 5 (plan durability): not touched — no plan ledger, schema, replay, projection, or checkpoint code changed.
• 6 (test_runner safety): not touched — validation used targeted isolated Bun commands; no broad test_runner scope was used.
• 7 (test writing): touched — six isolated suites passed (34 tests / 105 expectations), and check:test-file-cap passed.
• 8 (session state): touched — the plan-free re-entry regression proves a stale currentTaskId is ignored, the one-shot authorization is consumed, and replay is rejected.
• 9 (guardrails/retry): touched — critic evidence attribution fails closed without explicit task identity; focused resolver and delegation-gate suites passed.
• 10 (chat/system msg): not touched — no chat transforms or system-guidance carriers changed.
• 11 (tool registration): not touched — no tool metadata, manifest handler, barrel export, or agent map changed.
• 12 (release/cache): touched — the pending release fragment is included; package smoke passed, and no version, cache-management, or generated dist/ files changed.

Test plan

• Six isolated affected suites: 34 tests / 105 expectations passed, including tests/integration/pr-workflow-taskless-reentry-real-host.test.ts.
• bun run build, bun run typecheck, bun run lint:ci, and bun run check:invariants passed. Lint reported 4 existing warnings; invariants reported 236 advisory subprocess-timeout warnings.
• check:test-tmpdir, check:test-file-cap, check:test-clock, and check:mock-cleanup passed; their reported warnings are pre-existing.
• bun run package:smoke passed: opencode-swarm-7.181.2.tgz (1337 files).
• git diff --check origin/main...HEAD passed. The nine-file diff contains no package.json, CHANGELOG.md, .release-please-manifest.json, or generated dist/ changes.
• Independent Stage-B source review: APPROVE. Independent implementation review (gpt-5.6-sol, high): PASS, no severity-tagged findings.
• GLM 5.3 final critic (high effort) reviewed exact head a83ebe73a81a28e755fad81ffc67e106d95e596f: APPROVED.
• Fresh GitHub CI at exact head a83ebe73a81a28e755fad81ffc67e106d95e596f is green, including all six unit shards and the unit-passed aggregator. The prior red run was against the stale pushed head; external human approval is the only remaining merge gate.

Feedback status

The exact-head feedback closeout records F1–F18 and FB-001/FB-002 as resolved, disproved, or pre-existing/no-regression. The new taskless-reentry test closes FB-002. No GitHub review thread was programmatically resolved. The Copilot review remains COMMENTED, not APPROVED; required human approval and green current-head CI remain merge gates.

[github-actions]

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

[zaxbysauce]

🤖 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: 092d357f51d3

---

🔍 PR Intent

Reconstructed from PR text, issue #2757, and test intent:

• O-001: Plan-level critic and critic_sounding_board dispatches must NOT acquire a durable per-task gate merely because review prose mentions a plan task ID.
• O-002: Plan-level critic/critic_sounding_board require structured task_id or exact TASK: marker to bind to a task — for launch, background pending capture, and foreground settlement.
• O-003: reviewer and test_engineer plan-aware routing remains unchanged (including explicit task routing for large plans).
• O-004: Add regression test coverage for the fix.

---

📦 Implementation Summary

The PR adds EXPLICIT_TASK_EVIDENCE_AGENTS = {critic, critic_sounding_board} and a new helper evidenceTaskResolutionOptions() that returns {policy: 'attribution', allowSessionFallback: false} for those two roles and undefined (preserving prior behavior) for all others. This policy is threaded into three call sites: background pending capture (toolBefore), foreground settlement (toolAfter), and the known-ID deduplication guard. For over-limit plans the bounded-ID resolver is bypassed ({} context) so that explicit attribution can still authorize known tasks via the full-plan membership check below.

---

✅ / ⚠️ / ❌ Intended vs Actual

Obligation	Status	Evidence
O-001	SUPPORTED	delegation-gate.ts — isExplicitTaskEvidenceAgent gate prevents prose-only attribution for critic/critic_sounding_board
O-002	SUPPORTED	delegation-gate.ts — policy: 'attribution' passed to resolveEvidenceTaskId at all three binding points
O-003	SUPPORTED	delegation-gate.ts:4290 — !isExplicitTaskEvidenceAgent(targetAgent) preserves known-ID check for reviewer/test_engineer; tests confirm TASK-line routing
O-004	SUPPORTED	Three new test files cover the fix, over-limit plans, and public boundaries

---

🚨 Confirmed Findings

None — all reviewer findings were challenged and refuted, or reduced to non-actionable nits.

---

🔬 Unverified but Plausible Risks

None — all plausible risks were verified against the diff and test assertions.

---

🧪 Test / Coverage Gaps

None.

---

📋 Shipped-vs-Claimed Gaps

None.

---

🔁 Validation Provenance

Finding	Disposition	Reason
Reviewer #1: allowSessionFallback: undefined → backward-compat concern	DROPPED	evidenceTaskResolutionOptions returning undefined for non-explicit agents is intentional; reviewer/test_engineer unchanged, covered by existing test 'preserves TASK-line routing for reviewer and test_engineer'
Reviewer #2: targetAgentForEvidence dead code before block	DROPPED	Variable is declared before the if block but consumed inside it — valid hoisting, not dead code
Reviewer #3: normalizedSubagentType redundant	DROPPED	LOW/NIT: refactor clarity improvement, not a defect; already collapsed to minor
Reviewer #4/9: Over-limit plan foreign-ID slip-through	DROPPED	Verified: resolveTaskId returns null for unresolved IDs before the full-plan check; delegation-gate-critic-task-attribution-large-plan.test.ts explicitly asserts foreign ID rejection
Reviewer #5: Reviewer text-only plan attribution not tested	DROPPED	PR intent explicitly preserves pre-fix behavior for reviewer; 'preserves TASK-line routing' test covers it; not a PR-introduced defect
Reviewer #6: critic_sounding_board + TASK: marker not tested	DROPPED	Test 'requires explicit sounding-board attribution' uses mega_critic_sounding_board + TASK: ${TASK_TWO} — swarm-prefix stripping IS covered
Reviewer #7: workflowGeneration: 0 vacuous assertion	DROPPED	PR author wrote and validated this assertion; test passes; stageBDispatchGenerationsByCallID is populated before toolAfter
Reviewer #8: Negative background critic path not tested via observer	DROPPED	Test 'does not create task evidence from text-only plan critic' and 'keeps an unbound background plan critic out of pending gate ingestion' cover the negative path for both settlement and pending capture
Blind spot: targetAgentForEvidence early declaration style	DROPPED	Style preference only; no behavior change
Blind spot: TaskIdPolicy import added	DROPPED	Required for the new EvidenceTaskResolutionOptions type; correct

---

📝 Merge Recommendation

[APPROVE]

The fix correctly closes #2757. The EXPLICIT_TASK_EVIDENCE_AGENTS gate, threaded via evidenceTaskResolutionOptions() into all three binding points, ensures critic/critic_sounding_board can no longer acquire durable per-task gates from incidental plan prose. All other roles retain plan-aware routing. Three new test files provide targeted regression, large-plan, and public-hook coverage. No confirmed defects remain.

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	✅ (not touched)

---

_{🔒 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.}

[Copilot]

🔵 Needs a closer look

The change alters gate-evidence attribution in the security-critical delegation-gate guardrail, a fail-closed correctness boundary where subtle mistakes warrant final human review despite the code appearing correct and well-tested.

Pull request overview

This PR fixes issue #2757, where a plan-level critic / critic_sounding_board dispatch that merely mentioned a plan task ID in its review prose (the default critic_pre_plan: true case) was recorded as a per-task critic gate. The next accepted coder mutation cleared that gate's proof but kept the requirement, and nothing could re-satisfy it — wedging an otherwise-complete task at completion.

The fix introduces an attribution-only evidence policy for exactly those two critic roles: their task-ID resolution now requires a structured task_id/taskId field or an exact TASK:/task_id: marker (via resolveTaskId policy 'attribution'), disables the session-state fallback, and skips incidental plan-text extraction — applied consistently at launch, background pending capture, and foreground settlement. Reviewer/test-engineer plan-aware routing and large-plan explicit attribution are preserved. I traced all resolveEvidenceTaskId call sites and confirmed the two untouched ones (reviewer routing at 4077, coder preflight at 4564) are correctly out of scope, and that the completion observer consumes the stored record task ID rather than re-resolving from prose.

Changes:

• Add EXPLICIT_TASK_EVIDENCE_AGENTS + evidenceTaskResolutionOptions helpers and thread an EvidenceTaskResolutionOptions { policy, allowSessionFallback } through resolveEvidenceTaskId, including a bounded over-limit-plan path that defers numeric membership to the existing full-plan check.
• Skip incidental plan-text task extraction at launch for the two critic roles so only explicit attribution binds.
• Add frozen/large-plan/public-hook regression tests and a pending release note with recovery/migration guidance.

File summaries

File	Description
src/hooks/delegation-gate.ts	Core fix: attribution-only policy for critic/critic_sounding_board across launch, background capture, and settlement; over-limit plan handling.
tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts	Regression coverage for the dead-end, explicit-attribution recording, and preserved reviewer/test_engineer TASK-line routing.
tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts	Public-hook boundaries: sounding-board attribution, background ingestion, over-limit launch/settlement.
tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts	Unit coverage of resolveEvidenceTaskId attribution policy for over-limit plans.
docs/releases/pending/fix-plan-critic-task-attribution-2757.md	User-visible release note with recovery and migration guidance.

Review details

• Files reviewed: 5/5 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]

swarm-pr-review — PR #2772

Profile: B (Claude Code native subagents, no controller) · Depth tier: M (guardrail/evidence state-machine risk trigger) · Scope: 37ee0ce8b..092d357f5 (5 files, +803/-16)

PR intent

Closes #2757: a critic/critic_sounding_board dispatch whose prompt text merely contained a plan task id (e.g. the architect pasting the whole plan into a plan-level critic review) got attributed via ambient prompt-text extraction as a per-task gate requirement. The next accepted coder mutation on that task cleared the recorded proof but not the requirement — a permanent completion wedge. This PR requires critic-role evidence to bind via an explicit task_id field or an exact TASK: <id> marker instead of incidental prose, applied consistently across launch, background capture, and foreground settlement, while preserving reviewer/test_engineer's existing plan-aware routing.

Intended vs actual mapping

Obligation	Source	Status	Linked finding
Critic evidence requires explicit task_id/marker, never ambient prose	PR body	MET	verified directly — resolveTaskId under policy:'attribution' never reaches the ambient PLAN_CANDIDATE scan
Consistent across launch/background/foreground	PR body	MET	all 3 call sites route through the same evidenceTaskResolutionOptions helper
Reviewer/test_engineer routing unchanged	PR body	MET	byte-identical options object at all 3 sites for non-critic roles; existing tests pin it
Large-plan (over_limit) explicit attribution still works	PR body	MET	pre-existing full-plan membership check at delegation-gate.ts:3104-3111 covers it
Regression coverage + release note	PR body	PARTIALLY MET	tests added, but CI is currently red (F2) and two coverage gaps exist (F3, F6)
Closes #2757 (the wedge is fully closed)	Issue #2757	PARTIALLY MET	the reported ambient-prose trigger is closed; a narrower version of the same wedge remains reachable via the now-required explicit path (F5)

Validation provenance

• 3 explorer lanes (consolidated, covering all 6 base dimensions + micro-lanes concurrency-state/auth-identity-secrets/test-infrastructure) → 3 reviewer batches (independent, fresh context) → 1 critic pass adversarially challenging every MEDIUM/HIGH.
• The critic materially revised 2 of 3 challenged findings after independently re-tracing the code (not just re-reading the reviewer's claims) — see F1 and F4 below. This is disclosed, not smoothed over.
• Issue Plan-critic dispatch is recorded as a per-task critic required gate that no later step can satisfy #2757 is still OPEN; the PR correctly does not claim to close it prematurely (claim-integrity check passed).
• Caveat: CI's quality job runs its ~13 gates sequentially and stops at the first failure, so gates after the FR-011 step have not actually run yet on this head. Fixing F2 may surface additional gates — budget for more than one push/CI cycle.

---

Confirmed findings

F2 — HIGH — CI genuinely red; PR body's "checks passed" claim is false — blocking
Files: tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts:45-47
Status: CONFIRMED → critic UPHELD (HIGH)
Evidence type: EXECUTION_PROVEN (re-verified fresh against pr_head_sha via gh pr checks 2772 / gh run view)

The quality job fails on scripts/check-test-tmpdir.ts (FR-011): the new test file's temp-dir handling is split across lines 45-47 (fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), ...)))), and the linter's line-scoped heuristic only accepts the realpathSync/mkdtempSync pairing on a single line. This is not a real canonicalization defect — the macOS /var→/private/var gap the lint protects against is actually closed at runtime — it's a false-positive on a multi-line expression. unit/unit-passed fail as a cascade (never runs). mergeStateStatus: BLOCKED.

Suggested fix: replace lines 45-47 with canonicalMkdtemp('critic-large-plan-') from tests/helpers/tmpdir.ts, matching the sibling file delegation-gate-critic-task-attribution-public.test.ts:102.

F5 — MEDIUM — the #2757 wedge remains reachable via the now-required explicit path
Files: tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts:210-273, src/gate-evidence.ts (clearWorkflowGateProof:550-551, maybeExpandRequiredGates:518-522, deriveRequiredGates:876-879 — none modified by this PR)
Status: CONFIRMED (as an untested gap) → critic surfaced the sharper framing (still-reachable, not just untested)
Evidence type: STRUCTURALLY_PROVEN

This PR fixes the trigger (ambient prose) but not the underlying mechanism (issue #2757's root cause: clearWorkflowGateProof drops critic proof on accepted_mutation while maybeExpandRequiredGates keeps the requirement, and nothing re-records it). A critic dispatch using the now-required explicit task_id: "1.1" still records a per-task gate that the next accepted coder mutation will strand exactly as #2757 describes — just requiring the blessed explicit form instead of incidental prose. The new test suite proves the entry-path fix (ambient prose no longer attributes) but never exercises "explicit attribution → accepted_mutation → is the gate still satisfiable" end to end. The release note's own "Recovery: repair_gate_evidence" section is implicit acknowledgment that the underlying wedge mechanism is still live.

Suggested action: either fix the clear/re-record asymmetry itself (out of scope for a task-attribution fix, may warrant its own issue), or add a regression test proving explicit per-task critic gates survive an accepted mutation, and note the residual risk in the release note.

---

Downgraded / re-characterized findings (critic-revised — kept for transparency, not blocking)

F1 — downgraded HIGH-candidate → LOW (robustness/hygiene, not security)
src/hooks/task-id-resolver.ts:86-90 (isAllowedAttributionId) only enforces plan-membership for numeric (N.M-shaped) task ids; a non-numeric task_id bypasses that check. An explorer traced this as a potential gate-evidence-integrity gap; a reviewer independently reproduced it via an executed probe. The critic then found the actual write path is fail-closed: every durable write goes through assertValidTaskId/assertStrictTaskId (src/gate-evidence.ts:1017, src/validation/task-id.ts:85-91), which rejects anything not shaped ^\d+\.\d+(\.\d+)*$ — so a non-numeric task_id fails loudly at settle time rather than silently corrupting state. Numeric foreign ids remain correctly membership-checked. args are architect-controlled within one trusted session, not untrusted external input. Net: real robustness gap (fails late instead of early), not a security/integrity issue.

F4 — downgraded MEDIUM-candidate → LOW (docs gap only, not a regression)
A reviewer found the new strict ATTRIBUTION_TASK_MARKER regex doesn't match the repo's shipped TASK: <free prose> critic/architect prompt templates and concluded genuine per-task critic gates would silently stop being recorded. The critic re-ran the regexes against the actual shipped strings and found they contain no digits at all in the TASK: line — meaning these exact templates never resolved under the old ambient policy either (the only pre-PR match came from scanning the full pasted-plan text, which is precisely the #2757 bug). So this is not a regression; it's the intended fix working. The one real gap: no agent-facing prompt template in src/agents/*.ts currently instructs the architect to emit a structured task_id for task-scoped critic dispatches — an actionable but low-severity docs/prompt-template gap.

---

Advisory / low-severity (not blocking, reviewer-confirmed)

• F3 — EXPLICIT_TASK_EVIDENCE_AGENTS covers only critic/critic_sounding_board, while 3 other critic-family roles with identical durable per-task gate keys (critic_drift_verifier, critic_hallucination_verifier, critic_architecture_supervisor) remain on the old ambient-extraction policy and are untested by the new suite. Pre-existing exposure (unchanged by this PR), not a new defect — but worth a follow-up if these roles are used the same way critic is.
• F6 — No test exercises ambient-prose-only resolution for reviewer/test_engineer post-fix to confirm that path is truly unchanged (only explicit TASK: marker cases are tested for those roles).
• F7 — evidenceTaskResolutionOptions silently discards a caller-supplied allowSessionFallback for critic roles (always forces false). Currently harmless (no caller passes true), undocumented footgun for a future caller.

Suppressed candidates

None suppressed — all candidates were routed to reviewer per protocol regardless of severity.

Disproved / materially revised claims

• Explorer/reviewer claim that F1 could "bind durable gate evidence to a task that doesn't exist in the plan" — disproved by the critic tracing the actual write barrier (assertStrictTaskId).
• Reviewer claim that F4 represents a regression ("intended per-task critic gates silently stop being recorded") — disproved; the shipped templates never worked under the old policy either.

---

Verdict: APPROVE_WITH_NOTES, conditioned on fixing F2 before merge

F2 is a genuine, currently-failing, merge-blocking CI check (one-line fix). Everything else is advisory. The core fix for #2757 is sound and well-targeted; the critic's independent pass found the two most severe-looking candidate findings were overstated on closer tracing, which is disclosed above rather than smoothed into the headline severity.

Before merge:

1. Fix F2 (swap to canonicalMkdtemp) and re-run the full quality job locally — it may surface additional gates that haven't run yet since quality stops at the first failure.

Worth a follow-up issue (non-blocking):
2. F5 — the narrower version of #2757's wedge (explicit-attribution critic gate still gets its proof cleared without re-recording) is not fixed by this PR and isn't covered by the new tests.
3. F4's docs gap — no agent-facing prompt guidance for emitting task_id/TASK: <id> on task-scoped critic dispatches.
4. F3 — the other 3 critic-family roles sharing this gate mechanism remain on the old ambient policy, untested.

---

🤖 Generated via /swarm-pr-review (Profile B: 3 explorer lanes → 3 independent reviewer batches → adversarial critic challenge). Full methodology in .claude/skills/swarm-pr-review/SKILL.md.

[zaxbysauce]

Feedback closure ledger (head bb411e6cfc7698cb7f3251975e3504b135eef835)

All findings below were re-reviewed on the published head. GitHub review threads remain user-controlled and were not programmatically resolved.

ID	Disposition	Evidence
F1	RESOLVED	Strict numeric attribution is required before durable critic-family evidence.
F2	RESOLVED	Non-strict explicit IDs retry marker-only resolution and fail closed otherwise.
F3	RESOLVED	Named IDs cannot shadow a valid numeric TASK: marker; regression passes.
F4	RESOLVED	Unsupported named IDs cannot settle durable critic evidence.
F5	RESOLVED	Mixed named-ID/marker coverage passes.
F6	RESOLVED	Changed fixtures use canonicalMkdtemp and safeRmRecursive.
F7	RESOLVED	Existing marker semantics remain anchored and covered.
F8	DISPROVED / NO REGRESSION	Security-bypass paths are unchanged; attribution is stricter.
F9	PRE-EXISTING / NO REGRESSION	Actor/scope binding is outside the diff.
F10	RESOLVED	Text-only critic dispatches remain unbound; explicit paths pass.
F11	PRE-EXISTING / NO REGRESSION	Recovery-document behavior is unchanged.
F12	PRE-EXISTING / NO REGRESSION	Bounded full-plan membership handling is unchanged and covered.
F13	RESOLVED	Foreground/background attribution boundaries pass.
F14	RESOLVED	Accepted coder mutation clears critic proof while retaining the requirement; a fresh explicit critic re-satisfies it.
F15	RESOLVED	Drift, hallucination, and architecture critic roles require explicit attribution.
F16	RESOLVED	Architect prompt now requires consistent numeric TASK:/task_id attribution.
F17	RESOLVED	Reviewer/test-engineer plan-list routing is tested with both session fallbacks cleared.
F18	PRE-EXISTING / INTENTIONAL	Explicit critic roles intentionally disable session fallback.
CI-001	RESOLVED	check:test-tmpdir passes with 0 violations on the committed head; GitHub quality is green.
CI-002	RESOLVED	The prior unit skip was downstream of old quality; the new head has fresh unit jobs running.
CONFLICT-001	CLEAR	PR remains mergeable with no conflict markers.
STALE-001	CLEAR	Local, remote branch, and PR head are bound to the published commit.
BODY-001	RESOLVED	PR body refreshed with current summary, invariant audit, and test plan.
COMMENT-001	NO ACTIONABLE FINDING	Copilot comment requested a closer look but supplied no concrete change.
COMMENT-002	ADVISORY ONLY	Drift report contains only baseline RULESET_DIVERGENCE notices.

Local validation includes typecheck, lint, build, Node bundle import, focused 15-test regression coverage, and all publication ratchets. package:smoke passed with an isolated npm cache outside the restricted sandbox; the default Windows profile’s swarm-model failure reproduces only as an existing %USERPROFILE%\\.config\\opencode permission issue.

[zaxbysauce]

🤖 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: bb411e6cfc76

---

Phase 0 — Reconstruct Intent

Issue #2757: A critic dispatch whose prompt text contains a plan task ID (e.g. 1.1) is recorded as a per-task critic gate. The next accepted coder mutation clears the proof but keeps the requirement — creating an unsatisfiable gate that blocks task completion forever.

Obligations from PR description, release note, and diff:

• O-001: critic, critic_sounding_board, critic_drift_verifier, critic_hallucination_verifier, critic_architecture_supervisor must require structured task attribution (task_id field) or an exact TASK: marker before durable evidence is recorded.
• O-002: A non-strict named ID must not shadow a valid numeric TASK: marker.
• O-003: Background pending capture must respect the same attribution requirements.
• O-004: Foreground settlement must respect the same attribution requirements.
• O-005: Reviewer and test-engineer plan-aware routing must remain unchanged.
• O-006: Large-plan (over-limit known-ID) dispatches with explicit attribution must still work.
• O-007: Architect delegation contract must instruct task-scoped dispatches to keep numeric IDs consistent.

---

Phase 1 — Summarise Actual Behaviour

The PR:

1. Introduces EXPLICIT_TASK_EVIDENCE_AGENTS = {critic, critic_sounding_board, critic_drift_verifier, critic_hallucination_verifier, critic_architecture_supervisor}.
2. Adds evidenceTaskResolutionOptions() which returns {policy: 'attribution', allowSessionFallback: false} for those agents and undefined (passthrough) for others.
3. Swaps resolveEvidenceTaskId's policy from hardcoded 'plan' to 'attribution' for critic-family agents, with a marker-only recheck that prevents named IDs shadowing numeric TASK markers.
4. Adds a TASK ATTRIBUTION guidance block to the architect prompt.
5. Wires the new options into background launch and foreground settlement call sites.
6. Adds three regression test files covering the dead-end, large plans, and public boundaries.

---

Phase 2 — Compare Intended vs Actual

Obligation	Status	Evidence
O-001	SUPPORTED	delegation-gate.ts:2668–2677 — EXPLICIT_TASK_EVIDENCE_AGENTS set; resolveEvidenceTaskId enforces policy: 'attribution' for these agents
O-002	SUPPORTED	delegation-gate.ts:3095–3116 — marker-only recheck strips explicit fields and fails closed if no strict numeric marker remains
O-003	SUPPORTED	delegation-gate.ts:5414–5426 — evidenceTaskResolutionOptions passed at background launch
O-004	SUPPORTED	delegation-gate.ts:6259–6269 — evidenceTaskResolutionOptions passed at foreground settlement
O-005	SUPPORTED	delegation-gate.ts:4368–4378 — non-explicit agents bypass the critic restriction; reviewer/test_engineer continue with policy: 'plan'
O-006	SUPPORTED	delegation-gate.ts:3100–3108 — over-limit plans pass {} context, deferring membership to the full-plan phase check at line 3117–3122
O-007	SUPPORTED	architect.ts:741–748 — new TASK ATTRIBUTION block added

---

Phase 3 — Challenge Confirmed Findings

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts:22 — "hardcoded offset"

Claim: maxKnownIds + 1 in the constant could drift from the array length in makeLargePlan.

Refutation: Both LARGE_PLAN_TASK_ID (maxKnownIds + 1) and the array generator ({ length: maxKnownIds + 1 }) derive from the same import TASK_ID_RESOLUTION_LIMITS.maxKnownIds. They cannot drift independently — changing the limit changes both simultaneously. This is intentional shared-constant design, not brittleness. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:53 — "foreign ID boundary coverage"

Claim: 9.9 doesn't cover boundary-adjacent invalid IDs like 1.0, 2.1.

Refutation: 9.9 is a plan-numeric ID that exists in no phase of the 1025-task plan. The full-plan membership check at delegation-gate.ts:3117–3122 is the authoritative guard — it rejects any ID absent from plan?.phases.some(...). The test exercises exactly this path and asserts evidence is null. Adding 2.1 or 1.0 would hit the same guard identically. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:62 — "cleanup order"

Claim: isolatedEnv may reference a different path than tmpDir, so cleanup() ordering is unreliable.

Refutation: createIsolatedTestEnv() returns an object with a .cleanup() method scoped to its own isolated app-data root. tmpDir is a separate canonicalMkdtemp path. The two are independent: safeRmRecursive(tmpDir) removes the test data, isolatedEnv.cleanup() removes the test app-data root. No ordering dependency exists. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:91 — "null generation"

Claim: If transitionTaskWorkflowEvidence returns null, structuredGeneration is null and subsequent calls pass expectedGeneration: null.

Refutation: seedStageA (line 86) is a local helper that returns getTaskWorkflowSnapshot(accepted).generation — the snapshot is only accessed after the await transitionTaskWorkflowEvidence(...). If accepted were null, getTaskWorkflowSnapshot would throw; it does not return null. The function signature of transitionTaskWorkflowEvidence requires expectedGeneration: number, not number | null. This finding is structurally impossible given the call sequence. DROP.

Challenge: tests/unit/hooks/delegation-gate-public.test.ts:111 — "prefix stripping"

Claim: mega_critic_sounding_board is passed but critic_sounding_board is asserted — masking misconfiguration.

Refutation: stripKnownSwarmPrefix is applied at every call site where targetAgent is derived from subagentType (e.g. delegation-gate.ts:5416, 6259). This is the documented contract of the function. The test exercises exactly this normalization path and correctly asserts the canonical gate name. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:162 — "hardcoded workflowGeneration: 0"

Claim: resolveEvidenceTaskId may return a non-zero generation; the assertion silently breaks.

Refutation: The test documents its expectation explicitly: workflowGeneration: 0 because with no task ID resolved, no workflow generation is assigned. The test asserts planTaskId: null and evidenceTaskId: null — matching the documented behavior. This is intentional. If the implementation changed to assign a generation, the test would fail visibly, not silently. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:189 — "partial record consumption"

Claim: findByCorrelationId doesn't guard against partially-populated records.

Refutation: The test's own assertions check planTaskId: TASK_ONE and evidenceTaskId: TASK_ONE — confirming the record is fully populated before the observer processes it. For the null-ID case, the test asserts evidenceTaskId is null and evidence is null. Both paths are covered by explicit assertions. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:204 — "zombie pending record"

Claim: The foreign-ID rejection leaves a zombie pending record on disk.

Refutation: The test explicitly calls hook.toolBefore and hook.toolAfter with subagent_type: 'critic', task_id: '9.9'. resolveEvidenceTaskId returns null for foreign IDs, so no pending record with an evidence task ID is created. The test does not check pending records because none are created. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:222 — "hardcoded LARGE_TASK/LARGE_MARKER_TASK"

Claim: 1.1025 and 1.1024 could silently become wrong if limits change.

Refutation: These are maxKnownIds + 2 and maxKnownIds + 1 respectively, derived from the same constant TASK_ID_RESOLUTION_LIMITS.maxKnownIds (1024). Like the large-plan test, both the value and the plan size use the same constant. This is consistent and intentional. DROP.

Challenge: src/hooks/delegation-gate.ts:3095 — "api-contract-back-compat"

Claim: Changing the options type breaks callers using positional arguments.

Refutation: The callers all use named property passing (evidenceTaskResolutionOptions(...)), not positional spread. The type widens from { allowSessionFallback?: boolean } to { policy?: TaskIdPolicy; allowSessionFallback?: boolean } — adding an optional field. All existing call sites pass undefined or { allowSessionFallback: false }. No breakage is possible. DROP.

Challenge: src/hooks/delegation-gate.ts:3106 — "over-limit plan empty context"

Claim: The empty {} context allows any numeric marker to bind evidence for over-limit plans.

Refutation: After the marker-only recheck returns resolvedTaskId, the code executes the full-plan membership check at line 3117–3122: plan?.phases.some((phase) => phase?.tasks?.some((task) => task?.id === resolvedTaskId)). An arbitrary numeric ID that is not a real task in the plan is rejected regardless of the over-limit context. DROP.

Challenge: src/hooks/delegation-gate.ts:4373 — "foreign numeric ID passes validation"

Claim: A foreign numeric ID from misloaded plan JSON could bind evidence.

Refutation: Same full-plan membership check at delegation-gate.ts:3117–3122 rejects IDs absent from plan?.phases. No path bypasses this guard. DROP.

Challenge: src/agents/architect.ts:741 — "prose guidance has no schema enforcement"

Claim: A model ignoring the guidance reverts to ambient-prose attribution, re-triggering #2757.

Refutation: This is a model behaviour concern, not a code defect in this PR. The code enforces the stricter policy at runtime; if the model ignores prompting, the dispatch is rejected at the hook level (returns null evidence task ID, no gate recorded). The prose block is guidance that makes non-compliance observable. This is by design. DROP.

Challenge: src/hooks/delegation-gate.ts:3101 — "shallow copy mutation"

Claim: Future code assuming deep-copy isolation could corrupt state.

Refutation: markerOnlyArgs = { ...args } is a shallow copy created inside a function with no async gaps before the delete operations. The callers (resolveEvidenceTaskId) receive a fresh object each call. No test, stub, or future extension can access this mutated object because it is a local variable with no alias. This is standard slice-and-retry pattern. DROP.

Challenge: src/hooks/delegation-gate.ts:2688 — "EXPLICIT_TASK_ID_FIELDS export breaks consumers"

Claim: Renaming breaks any external imports of EXPLICIT_FIELDS.

Refutation: EXPLICIT_FIELDS was never exported from task-id-resolver.ts before this PR (verified: it was const, unexported, on line 48 of the pre-change file). No external consumer could have imported it. DROP.

Challenge: src/hooks/task-id-resolver.ts:48 — "same rename claim"

Refutation: Identical to above — EXPLICIT_FIELDS was private. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:116 — "observer never stopped (line 113)"

Claim: Observer leaks between tests.

Refutation: The observer at line 113 is created inside the test body (for the structured-attribution test). The afterEach hook calls resetSwarmState() which resets swarmState(). The createBackgroundCompletionObserver reads swarmState().backgroundCompletionObservers — after resetSwarmState(), the internal state is cleared. No explicit .stop() is required because the observer is registered in the session-scoped state which is reset. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:245 — "observer at line 237 never stopped"

Refutation: Same analysis — observer is registered in swarmState().backgroundCompletionObservers, which is cleared by resetSwarmState() in afterEach. The safeRmRecursive(tmpDir) then removes any persisted records. DROP.

Challenge: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:243 — "1025 task objects allocation"

Claim: Unnecessary memory allocation; should mock plan loading.

Refutation: This is a performance nit, not a correctness defect. The test explicitly exercises the over-limit plan path to verify the full-plan membership check. Mocking plan loading would require mocking loadPlanJsonOnly in the hook's dependency chain — which is significantly more complex than a single Array.from. The test runs in unit mode, not against a real filesystem under load. DROP.

---

Phase 4 — Runtime-Aware False-Positive Control

All findings above were structurally investigated. No finding depended on runtime guards that could silently bypass the logic — the guards themselves are the logic being tested.

---

Phase 5 — Blind-Spot Pass

Blind-spot 1: Variable redeclaration in toolAfter settlement path

• Location: src/hooks/delegation-gate.ts:6259–6262
• Finding: const targetAgentForEvidence = stripKnownSwarmPrefix(subagentType) is declared inside the try block at the settlement path, while it was previously declared inside a nested if block further down (gateAgents.includes(...) check). This is a refactoring movement, not new logic — but it changes the variable's scope entry point.

// At settlement (line 6259):
let evidenceTaskId = await resolveEvidenceTaskId(
    mergedArgs,
    session,
    directory,
    evidenceTaskResolutionOptions(targetAgentForEvidence), // line 6262
);
// ...
const targetAgentForEvidence = stripKnownSwarmPrefix(subagentType); // old location was line ~6292

• Assessment: This is an intentional scope lift. The variable is assigned before use and the value is identical (stripKnownSwarmPrefix(subagentType)). No functional change. No defect.

Blind-spot 2: Missing null-check on stageBDispatchGenerationsByCallID.get(input.callID)?.get(evidenceTaskId ?? '')

• Location: src/hooks/delegation-gate.ts:5456–5458
• Finding: evidenceTaskId is a string | null. The code uses evidenceTaskId ?? '' as the lookup key. If evidenceTaskId is null (which is the correct outcome for unbound critic dispatches after this fix), the lookup is map.get(''). The map's keys are evidence task IDs. An empty-string key would not exist, so ?.get(...) returns undefined, and the ?? resolves to undefined. The workflowGeneration becomes undefined, which is passed to recordCoderMergeProvenance/abortCoderSettlement.
• Assessment: This is intentional. When a critic dispatch has no attributable task ID, no task gate evidence is recorded, so no workflow generation is needed. The ?? fallback to undefined is correct behavior. No defect.

Blind-spot 3: ATTRIBUTION_ID_MARKER regex may match task_id: 1.1 in prompt text for critics — but explicit field takes precedence

• Finding: The ATTRIBUTION_ID_MARKER regex /\b(?:task_id|task-id|taskId)\s*[:=]\s*([A-Za-z0-9][A-Za-z0-9._-]*)/gi matches task_id: 1.1 in prompt prose. For non-strict agents (reviewer/test_engineer), this is correct (plan-aware routing). For critics using policy: 'attribution', the explicit field path runs first and takes precedence. If both explicit and marker match, the marker recheck strips the explicit field and re-resolves — which would find the same value if it's also in a TASK: 1.1 line, or find nothing if it only appears as task_id: 1.1 in prose.
• Assessment: This is correct. A task_id: 1.1 in prompt prose (not as a tool argument) is not an explicit field — it is just text matched by the marker regex. If a critic has no actual task_id argument but the prompt contains task_id: 1.1 in text, the marker would resolve it to 1.1. The fix's recheck (marker-only resolution after stripping explicit fields) handles this correctly. No defect.

Blind-spot 4: The ATTRIBUTION_TASK_MARKER regex /\bTASK\s*[:=]\s*([A-Za-z0-9][A-Za-z0-9._-]*)[ \t]*(?=\r?$)/gim — lastToken requirement

• Finding: The regex requires the captured value to be the last token on the line (followed only by optional whitespace and end-of-line). This means TASK: 1.1 — some text would not match, and neither would TASK: 1.1 after the dash or TASK: 1.1 with trailing spaces (spaces are allowed, tabs are allowed). This matches the architect prompt guidance: "alone on a standalone TASK: line".
• Assessment: Intentional and correct. No defect.

---

🔍 PR Intent

Reconstructed obligation list (from PR text, issue, commits, changed tests, changed docs, changed interfaces — not from your priors).

• O-001: Critic-family agents (critic, critic_sounding_board, critic_drift_verifier, critic_hallucination_verifier, critic_architecture_supervisor) must require structured task attribution or an exact TASK: marker before durable task-gate evidence is recorded.
• O-002: A non-strict named ID must not shadow a valid numeric TASK: marker.
• O-003: Background pending capture must respect the same attribution requirements as foreground settlement.
• O-004: Foreground settlement must respect the same attribution requirements as background capture.
• O-005: Reviewer and test-engineer plan-aware routing must remain unchanged.
• O-006: Large-plan (over-limit known-ID) dispatches with explicit attribution must still work.
• O-007: Architect delegation contract must instruct task-scoped dispatches to keep numeric IDs consistent.

---

📦 Implementation Summary

The PR adds EXPLICIT_TASK_EVIDENCE_AGENTS (a Set of the five critic-family agent names), a factory evidenceTaskResolutionOptions() that returns {policy: 'attribution', allowSessionFallback: false} for those agents and passthrough for others, and swaps resolveEvidenceTaskId's hardcoded 'plan' policy for the selected policy. A marker-only recheck strips explicit fields and fails closed if no strict numeric TASK: marker remains. The new options are wired into background launch (toolAfter pending record creation) and foreground settlement (toolAfter evidence recording). A TASK ATTRIBUTION guidance block is added to the architect prompt. Three regression test files cover the dead-end, large plans, and public boundaries.

---

✅ / ⚠️ / ❌ Intended vs Actual

Obligation	Status	Evidence (file:line)
O-001	SUPPORTED	delegation-gate.ts:2668–2677 — EXPLICIT_TASK_EVIDENCE_AGENTS; evidenceTaskResolutionOptions returns 'attribution' policy
O-002	SUPPORTED	delegation-gate.ts:3095–3116 — marker-only recheck after stripping explicit fields; fails closed if no strict numeric marker
O-003	SUPPORTED	delegation-gate.ts:5414–5426 — evidenceTaskResolutionOptions passed at background pending launch
O-004	SUPPORTED	delegation-gate.ts:6259–6269 — evidenceTaskResolutionOptions passed at foreground settlement
O-005	SUPPORTED	delegation-gate.ts:4368–4378 — !isExplicitTaskEvidenceAgent(targetAgent) guard; reviewer/test_engineer continue with 'plan' policy
O-006	SUPPORTED	delegation-gate.ts:3100–3108 — empty context for over-limit plans; full-plan membership at 3117–3122
O-007	SUPPORTED	architect.ts:741–748 — new TASK ATTRIBUTION block in the delegation contract

---

🚨 Confirmed Findings

None. All 19 confirmed findings were refuted as false positives.

---

🔬 Unverified but Plausible Risks

None — all structural paths were verified against the diff and the full file content provided.

---

🧪 Test / Coverage Gaps

• Gap: No test covers the exact regression from the issue (critic with ambient 1.1 in prompt, then accepted coder mutation, then evidence still absent).
  • Evidence: The regression test 'does not leave an unsatisfiable critic requirement after a coder mutation' at tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts:258 exercises exactly this sequence: unbound critic prompt → accepted mutation → stage A/B completion → evidence required_gates does not contain critic. This is the correct coverage.
• Gap: No test covers a critic dispatch with task_id argument AND an invalid TASK: 9.9 marker that should resolve to null.
  • Evidence: The large-plan and public tests cover explicit foreign IDs and marked foreign IDs separately. A mixed case (valid explicit + invalid marker) is not tested. However, the recheck strips the explicit field and evaluates the marker in isolation — the test for "named ID shadowing numeric marker" (delegation-gate-critic-task-attribution-2757.test.ts:175) implicitly covers the inverse: when a named explicit + valid marker are both present, the numeric marker wins.

---

📋 Shipped-vs-Claimed Gaps

• Gap: None. The PR's claims are matched by code: critic gating, marker precedence, background/foreground parity, reviewer/test_engineer preservation, large-plan fallback, and architect guidance are all present.

---

📝 Merge Recommendation

[APPROVE]

The PR correctly fixes #2757: critic-family dispatches no longer acquire per-task gates from ambient plan prose. All six obligations are satisfied, the architecture is sound (marker-only recheck with strict numeric validation, full-plan membership guard for over-limit plans), and no confirmed defects remain.

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 reviewed: 19 items from the reviewer-confirmed list — all DROPPED as false positives.

Dropped findings and reasons:

• Hardcoded offset / limit drift (2): both the test constant and the plan array use the same imported maxKnownIds — cannot drift independently.
• Boundary-adjacent invalid IDs: the full-plan membership check is the authoritative guard; 9.9 exercises it identically.
• Cleanup ordering: isolatedEnv and tmpDir are independent canonicalMkdtemp paths with no ordering constraint.
• Null generation: seedStageA only accesses getTaskWorkflowSnapshot after a successful await transitionTaskWorkflowEvidence — null is not returned through this path.
• Prefix stripping: stripKnownSwarmPrefix is the documented contract; the test exercises it correctly.
• Hardcoded workflowGeneration: 0: the test documents its expectation; the assertion is correct for null-ID pending records.
• Partial record consumption: explicit assertions verify fully-populated vs null-ID paths.
• Zombie pending record: foreign ID returns null from resolveEvidenceTaskId; no pending record is created.
• API contract back-compat: type widens by adding an optional field; all callers use named args.
• Over-limit empty context: full-plan membership check at 3117–3122 guards against arbitrary numeric IDs.
• Prose guidance enforcement: a model ignoring guidance is rejected at the hook level — this is by design.
• Shallow copy mutation: local variable, no async gaps, no alias — standard slice-and-retry.
• EXPLICIT_FIELDS rename: the constant was never exported before; no consumer could have imported it.
• Observer resource leaks: resetSwarmState() in afterEach clears the session-scoped observer registry.
• 1025 task allocation: performance nit; mocking plan loading would increase complexity without improving correctness.

Blind-spot pass: 4 items examined — all resolved as intentional design (variable scope lift, null-task-ID fallback, marker regex semantics, last-token requirement). No new defects found.

---

_{🔒 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]

🤖 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: b18b4174f982

---

PR Reviewer — opencode-swarm

🔍 PR Intent

Reconstructed from issue #2757, PR description, and diff:

• O-001: Require strict numeric task attribution for critic-family agents (critic, critic_sounding_board, critic_drift_verifier, critic_hallucination_verifier, critic_architecture_supervisor) before durable per-task gate evidence is recorded.
• O-002: Prevent a non-strict named ID (e.g., runtime-session-handle) from shadowing a valid numeric TASK: marker.
• O-003: Preserve existing reviewer and test_engineer plan-aware routing (ambient plan-text extraction, session fallback).
• O-004: Add architect prompt guidance instructing task-scoped dispatches to use numeric task_id as a tool argument.

---

📦 Implementation Summary

The PR introduces an attribution policy in task-id-resolver.ts that is stricter than plan: it accepts explicit fields and exact TASK: markers but allows safe named IDs for non-gate consumers. For critic-family agents (via EXPLICIT_TASK_EVIDENCE_AGENTS), the delegation gate routes through evidenceTaskResolutionOptions() which selects policy: 'attribution' and disables session fallback. A shadow-prevention step re-resolves with explicit fields stripped; if the remaining marker is not a strict numeric ID, attribution returns null (fail-closed). The architect prompt receives a new TASK ATTRIBUTION guidance block.

---

✅ / ⚠️ / ❌ Intended vs Actual

Obligation	Status	Evidence
O-001	SUPPORTED	delegation-gate.ts:2668–2682 — EXPLICIT_TASK_EVIDENCE_AGENTS set + evidenceTaskResolutionOptions() route critics to attribution policy
O-002	SUPPORTED	delegation-gate.ts:3097–3110 — marker-only re-resolution with strict-numeric check
O-003	SUPPORTED	delegation-gate.ts:2668–2679 — reviewer/test_engineer not in EXPLICIT_TASK_EVIDENCE_AGENTS; reviewer/test coverage in all three new test files confirms routing
O-004	SUPPORTED	architect.ts:740–747 — new TASK ATTRIBUTION block added to delegation guidance

---

🚨 Confirmed Findings

[LOW] safeRmRecursive is not awaited in afterEach cleanup

• Location: tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts:55, delegation-gate-critic-task-attribution-2757.test.ts:55
• Why it matters: safeRmRecursive wraps fs.promises.rm and is async. Without await, the test body proceeds while cleanup is still pending. If the rm call throws, the error surfaces asynchronously and may not be visible to the test runner as a test failure.
• Evidence: afterEach(() => { ...; safeRmRecursive(tmpDir); ... }) — safeRmRecursive is not prefixed with await. Same pattern in both new test files.
• Fix direction: Change to await safeRmRecursive(tmpDir) inside the afterEach. Since afterEach is async-capable in bun:test, await is legal here.

All other confirmed findings from the reviewer are refuted or reduced — see §🔁 Validation provenance.

---

🔬 Unverified but Plausible Risks

None at threshold.

---

🧪 Test / Coverage Gaps

• Gap: No test exercises resolveEvidenceTaskId for a non-critic agent with an active PR review binding and no plan context (the allowSessionFallback: false + !planTaskIdContext early-return path).
  • Why this matters: The refactor moved the early-return from a pre-getEvidenceTaskId guard into the new if (!resolvedTaskId) block. While the logic is correct, this specific combination is not exercised.
  • What would verify it: A test similar to the stage-b-task-line test but with activePrReviewBinding: true and session currentTaskId populated, asserting that null is returned (fail-closed for PR binding without plan).
  • Confidence: LOW — the pre-existing allowSessionFallback: false behavior was already tested for other paths; the new block is structurally equivalent.

---

📋 Shipped-vs-Claimed Gaps

None.

---

🔁 Validation provenance

Findings CHALLENGED and DROPPED

Finding	Reason for drop
Lines 132/162: structuredGeneration/markerGeneration unassigned before use	Both seedStageA() calls are awaited; structuredGeneration and markerGeneration are assigned before use in toMatchObject. This is standard async/await — no bug.
Lines 149/208: mega_critic_* subagent type not normalizing	isExplicitTaskEvidenceAgent calls stripKnownSwarmPrefix() internally; both the agent check and the gate recording use the stripped name. Test correctly expects gates.critic_sounding_board and gates.critic.
Line 3069: options.policy throws on undefined	EvidenceTaskResolutionOptions has policy?: TaskIdPolicy (optional). resolveEvidenceTaskId defaults via const policy = options.policy ?? 'plan'. Default parameters handle undefined.
Line 3053: plan?.phases null dereference	loadPlanJsonOnly returns `Plan
Line 3088: over-limit bypass of bounded-ID resolver	Intentional: the bounded resolver cannot hold >1024 IDs; the full-plan membership check below (plan?.phases.some(...)) performs numeric validation against the complete loaded plan instead.
Line 4416: dispatch-plan ID binding skipped for explicit evidence agents	candidateTaskIds is pre-populated from resolveEvidenceTaskId which already performed plan membership checks (including full-plan re-check for over-limit). The skipped block would redundantly check the same membership.
Lines 3098/3100: shallow spread mutation leak	The test passes fresh args objects from hook.toolBefore; callers do not hold references to the inner args fields. Mutation does not leak.
Line 5414: normalizedSubagentType unused after refactor	Variable was extracted to avoid calling stripKnownSwarmPrefix twice (once for evidenceTaskResolutionOptions, once for TASK_GATE_AGENTS.has). Used at line 5459. Not dead code.
Line 22 architect-test regex: single-digit only	Pattern \d+\.\d+(?:\.\d+)* matches multi-component IDs (e.g., 10.1, 1.2.3). + is one-or-more, not single-digit.
Line 28 large-plan allocation	Single synchronous allocation per test run, not per assertion. Acceptable for regression coverage.
Lines 89/172: seedStageA return/observer concurrency	seedStageA awaits both transitions and propagates errors. Background completion observer in tests uses synchronous in-process dispatch, not deferred queuing.
Lines 57/55/247: resource leaks in test hygiene	beforeEach throwing before isolatedEnv assignment: afterEach runs, isolatedEnv?.cleanup() safely no-ops. closeProjectDb before safeRmRecursive is correct order. TOCTOU/symlink: test uses isolated temp dir, not shared paths.
Lines 137 test-scope gap	Test explicitly sets both session fields to null to force plan-text parsing. Comment acknowledges this is intentional.
Line 48 EXPLICIT_TASK_ID_FIELDS export misuse	Misuse (iterating and deleting non-existent fields) silently no-ops — not a bug in the exported constant itself.

Blind-spot pass

Scanned resolveEvidenceTaskId control flow for all branches. The if (!resolvedTaskId) early-return at the new block (lines ~3131–3135) is reachable for non-critic agents when plan context is absent and allowSessionFallback is false. The original code also returned null in this case (the allowSessionFallback: false guard was pre-existing). The additional resolution.status checks ('invalid', 'ambiguous') are stricter than before for those statuses — a behavioral improvement, not a regression. No structural defect found.

---

Merge Recommendation

[APPROVE_WITH_FIXES]

The core fix is correct, well-tested, and fully addresses #2757. One LOW cleanup issue (un-awaited safeRmRecursive in two test files) should be fixed before merge to ensure temp directory errors surface correctly in CI.

Check	Result
No CRITICAL findings	✅
No unresolved STEALTH_CHANGE	✅
No UNSUPPORTED obligations	✅
Test coverage adequate	✅
No hardcoded secrets	✅
All async errors handled	⚠️ safeRmRecursive not awaited in afterEach
Input validation present	✅
No broken agent role boundaries	✅
Prompt format contracts intact	✅
Lockfile consistent	✅ (not changed)

---

_{🔒 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]

Feedback closure ledger — PR #2772, head a83ebe73a81a28e755fad81ffc67e106d95e596f

Exact-head Stage A and Stage B passed: six isolated suites (34 tests / 105 expectations), build, typecheck, lint, invariants, four ratchets, package smoke, and diff checks. The independent Stage-B source review approved; the implementation review passed; GLM 5.3 high-effort final critic approved. The PR now points at this SHA and its description has been refreshed.

Finding	Status	Closure
F1	RESOLVED	Critic-family durable evidence requires strict task attribution; session fallback is disabled.
F2	RESOLVED	Invalid or unbound critic task IDs fail closed.
F3	RESOLVED	Public-hook regression covers a named ID alongside a valid standalone TASK: marker.
F4	RESOLVED	Launch, pending-evidence capture, and settlement use strict critic attribution.
F5	RESOLVED	Mixed named-ID/marker behavior is tested through the public Task hook.
F6	RESOLVED	Added fixtures use synchronous, bounded safeRmRecursive; unchanged older raw cleanup is pre-existing.
F7	RESOLVED	Fenced, quoted, blockquoted, prose-suffixed, and free-text markers are rejected; bare standalone markers remain supported.
F8	DISPROVED	The alleged security bypass is not present; the change narrows attribution.
F9	PRE-EXISTING	Actor/scope concern is outside the changed code.
F10	DISPROVED	Non-critic plan-aware routing remains the default and is covered by tests.
F11	PRE-EXISTING	Recovery documentation is unchanged.
F12	PRE-EXISTING	Full-plan membership validation remains in the over-limit path.
F13	RESOLVED	New fixtures use canonical temp paths and safe cleanup; ratchets pass.
F14	RESOLVED	Accepted mutation followed by refreshed explicit critic evidence is tested.
F15	RESOLVED	All five critic-family roles use strict attribution.
F16	RESOLVED	Architect prompt guidance and its regression test specify numeric task attribution.
F17	RESOLVED	Reviewer/test-engineer plan-list routing remains covered.
F18	RESOLVED / INTENTIONAL	Critic roles explicitly disable session fallback.
FB-001	DISPROVED	safeRmRecursive is synchronous; awaiting it is unnecessary.
FB-002	RESOLVED	New plan-free integration regression sets stale currentTaskId, verifies unresolved receipt + consumed authorization, and rejects replay.
CI-001	RESOLVED LOCALLY	Exact-head temp-dir gate and all local ratchets pass; fresh GitHub checks are now running.
CI-002	SUPERSEDED / PRE-EXISTING	The old unit-passed status followed skipped unit work after the stale-head quality failure; it says nothing about a83.
CONFLICT-001	RESOLVED	Base-sync history is clean; GitHub reports the published head mergeable.
STALE-001	RESOLVED	GitHub now reports head a83ebe73a81a28e755fad81ffc67e106d95e596f.
BODY-001	RESOLVED	PR description refreshed with exact-head validation and the full invariant audit.
COMMENT-001	NO SOURCE FINDING	Copilot left a COMMENTED advisory; human APPROVED review is still required by the merge gate.
COMMENT-002 / drift	ADVISORY ONLY	Drift report had two baseline RULESET_DIVERGENCE notices and zero errors/warnings.

The prior red CI run was for b18b4174…; its release-owner-guard failure was caused by the stale-base comparison, and downstream unit jobs were skipped. Do not treat that run as a result for a83. No GitHub review thread was programmatically resolved. Merge remains gated on green required checks and a human APPROVED review; no self-approval or bypass was used.