From 092d357f51d3ee2de3ed21672626f8a8f0d3c4c0 Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 12:50:31 -0500 Subject: [PATCH 1/5] fix(delegation-gate): require explicit critic task attribution --- .../fix-plan-critic-task-attribution-2757.md | 21 ++ src/hooks/delegation-gate.ts | 72 +++- ...-gate-critic-task-attribution-2757.test.ts | 328 ++++++++++++++++++ ...critic-task-attribution-large-plan.test.ts | 121 +++++++ ...ate-critic-task-attribution-public.test.ts | 277 +++++++++++++++ 5 files changed, 803 insertions(+), 16 deletions(-) create mode 100644 docs/releases/pending/fix-plan-critic-task-attribution-2757.md create mode 100644 tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts create mode 100644 tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts create mode 100644 tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts diff --git a/docs/releases/pending/fix-plan-critic-task-attribution-2757.md b/docs/releases/pending/fix-plan-critic-task-attribution-2757.md new file mode 100644 index 000000000..85f55856b --- /dev/null +++ b/docs/releases/pending/fix-plan-critic-task-attribution-2757.md @@ -0,0 +1,21 @@ +# Plan-critic task attribution fix (#2757) + +## What changed + +Plan-level `critic` and `critic_sounding_board` dispatches no longer acquire a +durable per-task gate merely because review prose mentions a plan task ID. +Those roles now require structured task attribution or an exact task marker at +launch, background pending capture, and foreground settlement. Reviewer and +test-engineer plan-aware routing is unchanged, including explicit task routing +for large plans. + +## Recovery + +Existing projects with already-orphaned critic gate evidence should use the +audited `repair_gate_evidence` recovery path. The fix does not rewrite durable +evidence automatically. + +## Migration + +No configuration change is required. Task-scoped critic dispatches must carry a +structured task ID or an exact task marker such as `TASK: 1.1`. diff --git a/src/hooks/delegation-gate.ts b/src/hooks/delegation-gate.ts index e54e6dc18..c7e9e4a46 100644 --- a/src/hooks/delegation-gate.ts +++ b/src/hooks/delegation-gate.ts @@ -131,6 +131,7 @@ import { resolveDelegatedPlanTaskId, resolveTaskId, TASK_ID_RESOLUTION_LIMITS, + type TaskIdPolicy, } from './task-id-resolver.js'; export { resolveDelegatedPlanTaskId } from './task-id-resolver.js'; @@ -2666,6 +2667,31 @@ const TASK_GATE_AGENTS = new Set([ 'sme', ]); +const EXPLICIT_TASK_EVIDENCE_AGENTS = new Set([ + 'critic', + 'critic_sounding_board', +]); + +function isExplicitTaskEvidenceAgent(targetAgent: string): boolean { + return EXPLICIT_TASK_EVIDENCE_AGENTS.has(stripKnownSwarmPrefix(targetAgent)); +} + +type EvidenceTaskResolutionOptions = { + policy?: TaskIdPolicy; + allowSessionFallback?: boolean; +}; + +function evidenceTaskResolutionOptions( + targetAgent: string, + allowSessionFallback?: boolean, +): EvidenceTaskResolutionOptions | undefined { + if (isExplicitTaskEvidenceAgent(targetAgent)) { + return { policy: 'attribution', allowSessionFallback: false }; + } + if (allowSessionFallback === false) return { allowSessionFallback: false }; + return undefined; +} + export function canRunWhileTaskAwaitsCompletion(input: { directory: string | undefined; normalizedTool: string; @@ -3026,10 +3052,10 @@ async function getEvidenceTaskId( } /** - * Resolves the correct task ID for evidence recording by chaining: - * 1. Explicit task_id in direct args (structured field) - * 2. Prompt-text extraction via resolveDelegatedPlanTaskId (plan-aware) - * 3. Session-state fallback via getEvidenceTaskId + * Resolves the correct task ID for evidence recording by chaining the selected + * resolver policy with an optional session-state fallback. Most roles retain + * plan-aware prompt resolution; task-gated critic roles select attribution + * policy and disable the fallback so only structured IDs or exact markers bind. * * This fixes parallel evidence recording where multiple reviewer/test_engineer * agents are dispatched for different tasks from the same architect session. @@ -3039,7 +3065,7 @@ async function resolveEvidenceTaskId( args: Record | undefined, session: AgentSessionState, directory: string, - options: { allowSessionFallback?: boolean } = {}, + options: EvidenceTaskResolutionOptions = {}, ): Promise { // Shared bounded resolution first; session fallback is allowed only when the // resolver had no safe plan context and therefore made no authoritative @@ -3058,11 +3084,21 @@ async function resolveEvidenceTaskId( if (args) { try { + const policy = options.policy ?? 'plan'; + // A plan over the shared bounded-ID limit can still authorize an + // explicitly attributed critic task. The full plan has already been + // loaded above, so defer numeric membership validation to the existing + // full-plan check below instead of handing the bounded resolver an + // over-limit context that intentionally rejects numeric markers. + const planContextOptions = + policy === 'attribution' && planTaskIdContext?.status === 'over_limit' + ? {} + : planTaskIdContext + ? toTaskIdPlanContextOptions(planTaskIdContext) + : {}; const resolution = resolveTaskId(args, { - policy: 'plan', - ...(planTaskIdContext - ? toTaskIdPlanContextOptions(planTaskIdContext) - : {}), + policy, + ...planContextOptions, }); if (resolution.status === 'resolved') { if ( @@ -4306,12 +4342,15 @@ export function createDelegationGateHook( args, stageBSession, directory, - activePrReviewBinding ? { allowSessionFallback: false } : undefined, + evidenceTaskResolutionOptions( + targetAgent, + activePrReviewBinding ? false : undefined, + ), ); const candidateTaskIds = new Set(); if (resolvedTaskId) candidateTaskIds.add(resolvedTaskId); const dispatchPlan = await loadPlanJsonOnly(directory); - if (dispatchPlan) { + if (dispatchPlan && !isExplicitTaskEvidenceAgent(targetAgent)) { const knownIds = new Set( dispatchPlan.phases.flatMap((phase) => phase.tasks.map((task) => task.id), @@ -5349,10 +5388,13 @@ export function createDelegationGateHook( } if (subagentSessionId) { const mergedArgs = { ...(storedArgs ?? {}), ...directArgs }; + const normalizedSubagentType = + stripKnownSwarmPrefix(subagentType); const evidenceTaskId = await resolveEvidenceTaskId( mergedArgs, session, directory, + evidenceTaskResolutionOptions(normalizedSubagentType), ); const scope = session.declaredCoderScope && @@ -5388,9 +5430,7 @@ export function createDelegationGateHook( evidenceTaskId, workspace: fallbackWorkspace, taskChangeContext, - workflowGeneration: TASK_GATE_AGENTS.has( - stripKnownSwarmPrefix(subagentType), - ) + workflowGeneration: TASK_GATE_AGENTS.has(normalizedSubagentType) ? stageBDispatchGenerationsByCallID .get(input.callID) ?.get(evidenceTaskId ?? '') @@ -6193,10 +6233,12 @@ export function createDelegationGateHook( let coderSettleTaskId: string | null = null; try { const mergedArgs = { ...(storedArgs ?? {}), ...directArgs }; + const targetAgentForEvidence = stripKnownSwarmPrefix(subagentType); let evidenceTaskId = await resolveEvidenceTaskId( mergedArgs, session, directory, + evidenceTaskResolutionOptions(targetAgentForEvidence), ); // Issue #2214 belt: the toolBefore scope preflight may have // resolved the task via sources resolveEvidenceTaskId lacks @@ -6224,8 +6266,6 @@ export function createDelegationGateHook( 'explorer', 'sme', ]; - const targetAgentForEvidence = - stripKnownSwarmPrefix(subagentType); if (gateAgents.includes(targetAgentForEvidence)) { if ( targetAgentForEvidence === 'reviewer' || diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts new file mode 100644 index 000000000..b1c52b280 --- /dev/null +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts @@ -0,0 +1,328 @@ +/** Regression coverage for issue #2757's plan-critic task attribution dead-end. */ + +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import * as path from 'node:path'; +import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; +import { findByCorrelationId } from '../../../src/background/pending-delegations'; +import type { Plan } from '../../../src/config/plan-schema'; +import { closeProjectDb } from '../../../src/db/project-db'; +import { + getTaskWorkflowSnapshot, + hasPassedAllGates, + readTaskEvidence, + transitionTaskWorkflowEvidence, +} from '../../../src/gate-evidence'; +import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; +import { ensureAgentSession, resetSwarmState } from '../../../src/state'; +import { createIsolatedTestEnv } from '../../helpers/isolated-test-env'; +import { canonicalMkdtemp } from '../../helpers/tmpdir'; +import { makeConfig } from './_delegation-gate-helpers'; + +const TASK_ID = '1.1'; + +function makePlan(): Plan { + return { + schema_version: '1.0.0', + title: 'Critic attribution regression', + swarm: 'mega', + current_phase: 1, + phases: [ + { + id: 1, + name: 'Implementation', + status: 'pending', + tasks: [ + { + id: TASK_ID, + phase: 1, + status: 'pending', + size: 'small', + description: 'Implement the issue fix', + depends: [], + files_touched: ['src/example.ts'], + }, + ], + }, + ], + }; +} + +function writePlan(directory: string): void { + writeFileSync( + path.join(directory, '.swarm', 'plan.json'), + JSON.stringify(makePlan(), null, 2), + 'utf8', + ); +} + +async function seedStageA(sessionID: string): Promise { + const accepted = await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'accepted_mutation', + agentType: 'coder', + expectedGeneration: 0, + transitionId: 'seed-coder', + }); + const generation = getTaskWorkflowSnapshot(accepted).generation; + await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'stage_a_passed', + expectedGeneration: generation, + transitionId: 'seed-stage-a', + }); + const session = ensureAgentSession(sessionID, 'architect', tmpDir); + session.currentTaskId = TASK_ID; + session.taskWorkflowStates.set(TASK_ID, 'pre_check_passed'); + return generation; +} + +async function settleCritic( + hook: ReturnType, + sessionID: string, + callID: string, + args: Record, +): Promise { + await hook.toolBefore({ tool: 'Task', sessionID, callID }, { args }); + await hook.toolAfter( + { tool: 'Task', sessionID, callID, args }, + { output: 'VERDICT: APPROVED\nThe review is complete.' }, + ); +} + +let tmpDir: string; +let isolatedEnv: ReturnType | undefined; + +beforeEach(() => { + // Keep user-scoped stores under a test-owned app-data root. The delegation + // hook exercises durable evidence and route state, so this also prevents a + // test run from touching the developer's real config/data directories. + isolatedEnv = createIsolatedTestEnv(); + resetSwarmState(); + tmpDir = canonicalMkdtemp('dg-critic-attribution-'); + mkdirSync(path.join(tmpDir, '.opencode'), { recursive: true }); + mkdirSync(path.join(tmpDir, '.swarm'), { recursive: true }); + writePlan(tmpDir); +}); + +afterEach(() => { + resetSwarmState(); + closeProjectDb(tmpDir); + rmSync(tmpDir, { + recursive: true, + force: true, + maxRetries: 5, + retryDelay: 100, + }); + isolatedEnv?.cleanup(); + isolatedEnv = undefined; +}); + +describe('delegation-gate — regression: plan-level critic attribution (#2757)', () => { + it('does not create task evidence from a text-only plan critic prompt', async () => { + const sessionID = 'plan-critic-unbound'; + const session = ensureAgentSession(sessionID, 'architect', tmpDir); + // The session may be actively working on 1.1, but plan-level critic + // dispatches still need explicit attribution rather than this fallback. + session.currentTaskId = TASK_ID; + session.taskWorkflowStates.set(TASK_ID, 'pre_check_passed'); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + // Before the fix, plan-aware text extraction treated the sole `1.1` in + // this plan-level prompt as task attribution, then recorded a per-task + // critic requirement that could outlive the coder mutation. + await settleCritic(hook, sessionID, 'critic-unbound', { + subagent_type: 'critic', + prompt: + 'MODE: CRITIC-GATE\nReview the complete plan before implementation. The plan includes 1.1.', + }); + + expect(await readTaskEvidence(tmpDir, TASK_ID)).toBeNull(); + }); + + it('keeps an unbound background plan critic out of pending gate ingestion', async () => { + const sessionID = 'background-plan-critic-unbound'; + const session = ensureAgentSession(sessionID, 'architect', tmpDir); + // A current task must not turn free-form plan prose into a Stage-B binding. + session.currentTaskId = TASK_ID; + session.taskWorkflowStates.set(TASK_ID, 'pre_check_passed'); + const hook = createDelegationGateHook( + makeConfig({ + hooks: { + background_subagents: true, + background_pending_timeout_minutes: 30, + }, + }), + tmpDir, + ); + const args = { + subagent_type: 'critic', + background: true, + prompt: + 'MODE: CRITIC-GATE\nReview the whole plan before work begins. The only task is 1.1.', + }; + + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'background-critic-unbound' }, + { args }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'background-critic-unbound', + args, + }, + { + state: 'running', + output: + 'Background critic started', + metadata: { background: true, jobId: 'background-critic-job' }, + }, + ); + + const observer = createBackgroundCompletionObserver({ + config: { enabled: true }, + directory: tmpDir, + }); + await observer.event({ + event: { + type: 'message.part.updated', + properties: { + part: { + type: 'text', + synthetic: true, + sessionID, + text: + '\n' + + 'VERDICT: APPROVED\n', + }, + }, + }, + }); + + const record = findByCorrelationId(tmpDir, 'background-critic-child'); + expect(record?.status).toBe('completed'); + expect(record?.planTaskId).toBeNull(); + expect(record?.evidenceTaskId).toBeNull(); + expect(record?.workflowGeneration).toBeUndefined(); + expect(await readTaskEvidence(tmpDir, TASK_ID)).toBeNull(); + }); + + it('records critic evidence when the dispatch carries explicit task_id attribution', async () => { + const sessionID = 'critic-explicit-task'; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, sessionID, 'critic-explicit', { + subagent_type: 'critic', + task_id: TASK_ID, + prompt: + 'MODE: CRITIC-GATE\nReview the implementation plan before execution.', + }); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.required_gates).toContain('critic'); + expect(evidence?.gates.critic).toBeDefined(); + }); + + it('does not leave an unsatisfiable critic requirement after a coder mutation', async () => { + const sessionID = 'plan-critic-orphan'; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + // Before the fix, this plan-level critic settlement created `critic` as a + // task requirement. A succeeding coder mutation intentionally clears its + // proof, but cannot satisfy the stale requirement again. + await settleCritic(hook, sessionID, 'critic-orphan', { + subagent_type: 'critic', + prompt: + 'MODE: CRITIC-GATE\nReview the whole plan. Its only implementation task is 1.1.', + }); + const accepted = await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'accepted_mutation', + agentType: 'coder', + expectedGeneration: 0, + transitionId: 'coder-after-plan-critic', + }); + const generation = getTaskWorkflowSnapshot(accepted).generation; + await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'stage_a_passed', + expectedGeneration: generation, + transitionId: 'stage-a-after-plan-critic', + }); + await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'stage_b_completed', + gate: 'reviewer', + sessionId: sessionID, + routeComplete: false, + expectedGeneration: generation, + transitionId: 'reviewer-after-plan-critic', + }); + await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'stage_b_completed', + gate: 'test_engineer', + sessionId: sessionID, + routeComplete: true, + expectedGeneration: generation, + transitionId: 'test-engineer-after-plan-critic', + }); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.required_gates).not.toContain('critic'); + expect(evidence?.gates.critic).toBeUndefined(); + expect(await hasPassedAllGates(tmpDir, TASK_ID)).toBe(true); + }); + + it('preserves TASK-line routing for reviewer and test_engineer', async () => { + const sessionID = 'stage-b-task-line'; + const generation = await seedStageA(sessionID); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + const reviewerArgs = { + subagent_type: 'reviewer', + prompt: `TASK: ${TASK_ID}\nACCEPTANCE: review the exact task and report APPROVED`, + }; + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'reviewer-task-line' }, + { args: reviewerArgs }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'reviewer-task-line', + args: reviewerArgs, + }, + { + output: `[REVIEWED] | task-${TASK_ID} | APPROVED | exact task approved`, + }, + ); + + const testEngineerArgs = { + subagent_type: 'test_engineer', + prompt: `TASK: ${TASK_ID}\nACCEPTANCE: test the exact task and report PASS`, + }; + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'test-engineer-task-line' }, + { args: testEngineerArgs }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'test-engineer-task-line', + args: testEngineerArgs, + }, + { output: `[TESTED] | task-${TASK_ID} | PASS | exact task passed` }, + ); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.gates.reviewer).toBeDefined(); + expect(evidence?.gates.test_engineer).toBeDefined(); + expect(getTaskWorkflowSnapshot(evidence)).toMatchObject({ + generation, + state: 'tests_run', + authoritative: true, + }); + expect(await hasPassedAllGates(tmpDir, TASK_ID)).toBe(true); + }); +}); diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts new file mode 100644 index 000000000..0eeef3dd7 --- /dev/null +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts @@ -0,0 +1,121 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { _internals as delegationGateInternals } from '../../../src/hooks/delegation-gate'; +import { TASK_ID_RESOLUTION_LIMITS } from '../../../src/hooks/task-id-resolver'; +import { ensureAgentSession, resetSwarmState } from '../../../src/state'; +import { safeRmRecursive } from '../../helpers/safe-test-dir'; + +const { resolveEvidenceTaskId } = delegationGateInternals; +const LARGE_PLAN_TASK_ID = `1.${TASK_ID_RESOLUTION_LIMITS.maxKnownIds + 1}`; + +function makeLargePlan(): Record { + return { + schema_version: '1.0.0', + title: 'Large plan critic attribution', + swarm: 'test', + current_phase: 1, + phases: [ + { + id: 1, + name: 'Implementation', + status: 'in_progress', + tasks: Array.from( + { length: TASK_ID_RESOLUTION_LIMITS.maxKnownIds + 1 }, + (_, index) => ({ + id: `1.${index + 1}`, + phase: 1, + status: 'pending', + size: 'small', + description: `Task ${index + 1}`, + depends: [], + files_touched: [], + }), + ), + }, + ], + }; +} + +let directory: string; + +beforeEach(() => { + resetSwarmState(); + directory = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'critic-large-plan-')), + ); + fs.mkdirSync(path.join(directory, '.swarm'), { recursive: true }); + fs.writeFileSync( + path.join(directory, '.swarm', 'plan.json'), + JSON.stringify(makeLargePlan()), + 'utf8', + ); +}); + +afterEach(() => { + resetSwarmState(); + safeRmRecursive(directory); +}); + +describe('critic task attribution with over-limit plans', () => { + test('preserves explicit and exact-marker IDs from the full plan', async () => { + const session = ensureAgentSession( + 'critic-large-plan', + 'architect', + directory, + ); + session.currentTaskId = '1.1'; + const options = { + policy: 'attribution' as const, + allowSessionFallback: false, + }; + + expect( + await resolveEvidenceTaskId( + { task_id: LARGE_PLAN_TASK_ID }, + session, + directory, + options, + ), + ).toBe(LARGE_PLAN_TASK_ID); + expect( + await resolveEvidenceTaskId( + { prompt: `TASK: ${LARGE_PLAN_TASK_ID}` }, + session, + directory, + options, + ), + ).toBe(LARGE_PLAN_TASK_ID); + }); + + test('rejects explicit and marked IDs absent from the full plan', async () => { + const session = ensureAgentSession( + 'critic-large-plan-foreign', + 'architect', + directory, + ); + session.currentTaskId = '1.1'; + const options = { + policy: 'attribution' as const, + allowSessionFallback: false, + }; + + expect( + await resolveEvidenceTaskId( + { task_id: '9.9' }, + session, + directory, + options, + ), + ).toBeNull(); + expect( + await resolveEvidenceTaskId( + { prompt: 'TASK: 9.9' }, + session, + directory, + options, + ), + ).toBeNull(); + }); +}); diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts new file mode 100644 index 000000000..8c4d30b3b --- /dev/null +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts @@ -0,0 +1,277 @@ +/** Public-hook guardrails for issue #2757 critic task attribution. */ + +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import * as path from 'node:path'; +import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; +import { findByCorrelationId } from '../../../src/background/pending-delegations'; +import type { Plan } from '../../../src/config/plan-schema'; +import { closeProjectDb } from '../../../src/db/project-db'; +import { + getTaskWorkflowSnapshot, + hasPassedAllGates, + readTaskEvidence, + transitionTaskWorkflowEvidence, +} from '../../../src/gate-evidence'; +import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; +import { ensureAgentSession, resetSwarmState } from '../../../src/state'; +import { createIsolatedTestEnv } from '../../helpers/isolated-test-env'; +import { canonicalMkdtemp } from '../../helpers/tmpdir'; +import { makeConfig } from './_delegation-gate-helpers'; + +const TASK_ONE = '1.1'; +const TASK_TWO = '1.2'; +const LARGE_TASK = '1.1025'; +const LARGE_MARKER_TASK = '1.1024'; + +function makeTask(id: string): Plan['phases'][number]['tasks'][number] { + return { + id, + phase: 1, + status: 'pending', + size: 'small', + description: `Implement ${id}`, + depends: [], + files_touched: [`src/task-${id.replaceAll('.', '-')}.ts`], + }; +} + +function makePlan(taskIds: string[]): Plan { + return { + schema_version: '1.0.0', + title: 'Critic attribution public-hook coverage', + swarm: 'mega', + current_phase: 1, + phases: [ + { + id: 1, + name: 'Implementation', + status: 'pending', + tasks: taskIds.map(makeTask), + }, + ], + }; +} + +function writePlan(directory: string, plan: Plan): void { + writeFileSync( + path.join(directory, '.swarm', 'plan.json'), + JSON.stringify(plan, null, 2), + 'utf8', + ); +} + +async function settleCritic( + hook: ReturnType, + sessionID: string, + callID: string, + args: Record, +): Promise { + await hook.toolBefore({ tool: 'Task', sessionID, callID }, { args }); + await hook.toolAfter( + { tool: 'Task', sessionID, callID, args }, + { state: 'completed', output: 'VERDICT: APPROVED\nReview complete.' }, + ); +} + +async function seedStageA(sessionID: string, taskId: string): Promise { + const accepted = await transitionTaskWorkflowEvidence(tmpDir, taskId, { + type: 'accepted_mutation', + agentType: 'coder', + expectedGeneration: 0, + transitionId: `seed-coder:${taskId}`, + }); + const generation = getTaskWorkflowSnapshot(accepted).generation; + await transitionTaskWorkflowEvidence(tmpDir, taskId, { + type: 'stage_a_passed', + expectedGeneration: generation, + transitionId: `seed-stage-a:${taskId}`, + }); + const session = ensureAgentSession(sessionID, 'architect', tmpDir); + session.currentTaskId = taskId; + session.taskWorkflowStates.set(taskId, 'pre_check_passed'); + return generation; +} + +let tmpDir: string; +let isolatedEnv: ReturnType | undefined; + +beforeEach(() => { + isolatedEnv = createIsolatedTestEnv(); + resetSwarmState(); + tmpDir = canonicalMkdtemp('dg-critic-public-'); + mkdirSync(path.join(tmpDir, '.opencode'), { recursive: true }); + mkdirSync(path.join(tmpDir, '.swarm'), { recursive: true }); +}); + +afterEach(() => { + resetSwarmState(); + closeProjectDb(tmpDir); + rmSync(tmpDir, { + recursive: true, + force: true, + maxRetries: 5, + retryDelay: 100, + }); + isolatedEnv?.cleanup(); + isolatedEnv = undefined; +}); + +describe('delegation-gate critic attribution public boundaries', () => { + it('requires explicit sounding-board attribution and preserves generation binding', async () => { + writePlan(tmpDir, makePlan([TASK_ONE, TASK_TWO])); + const unboundSession = ensureAgentSession( + 'sounding-board-unbound', + 'architect', + tmpDir, + ); + unboundSession.currentTaskId = TASK_ONE; + unboundSession.taskWorkflowStates.set(TASK_ONE, 'pre_check_passed'); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, 'sounding-board-unbound', 'unbound', { + subagent_type: 'critic_sounding_board', + prompt: 'Review the whole plan; its only implementation task is 1.1.', + }); + expect(await readTaskEvidence(tmpDir, TASK_ONE)).toBeNull(); + + const structuredGeneration = await seedStageA( + 'sounding-board-structured', + TASK_ONE, + ); + await settleCritic(hook, 'sounding-board-structured', 'structured', { + subagent_type: 'critic_sounding_board', + task_id: TASK_ONE, + prompt: 'Retry the exact task and return its approved verdict.', + }); + const structuredEvidence = await readTaskEvidence(tmpDir, TASK_ONE); + expect(structuredEvidence?.gates.critic_sounding_board?.agent).toBe( + 'critic_sounding_board', + ); + expect(getTaskWorkflowSnapshot(structuredEvidence)).toMatchObject({ + generation: structuredGeneration, + authoritative: true, + }); + + const markerGeneration = await seedStageA( + 'sounding-board-prefixed', + TASK_TWO, + ); + await settleCritic(hook, 'sounding-board-prefixed', 'marker', { + subagent_type: 'mega_critic_sounding_board', + prompt: `TASK: ${TASK_TWO}`, + }); + const markerEvidence = await readTaskEvidence(tmpDir, TASK_TWO); + expect(markerEvidence?.gates.critic_sounding_board?.agent).toBe( + 'critic_sounding_board', + ); + expect(getTaskWorkflowSnapshot(markerEvidence)).toMatchObject({ + generation: markerGeneration, + authoritative: true, + }); + }); + + it('records an explicitly bound background critic through completion ingestion', async () => { + writePlan(tmpDir, makePlan([TASK_ONE])); + const sessionID = 'background-critic-explicit'; + const hook = createDelegationGateHook( + makeConfig({ + hooks: { background_subagents: true }, + }), + tmpDir, + ); + const args = { + subagent_type: 'mega_critic', + task_id: TASK_ONE, + background: true, + prompt: 'Review this exact task and return a trusted verdict.', + }; + + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'background-call' }, + { args }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'background-call', + args, + }, + { + state: 'running', + output: + 'started', + metadata: { background: true, jobId: 'background-critic-job' }, + }, + ); + + const pending = findByCorrelationId(tmpDir, 'background-critic-child'); + expect(pending).toMatchObject({ + planTaskId: TASK_ONE, + evidenceTaskId: TASK_ONE, + workflowGeneration: 0, + status: 'pending', + }); + + const observer = createBackgroundCompletionObserver({ + config: { enabled: true }, + directory: tmpDir, + }); + await observer.event({ + event: { + type: 'message.part.updated', + properties: { + part: { + type: 'text', + synthetic: true, + sessionID, + text: + '\n' + + 'VERDICT: APPROVED\n', + }, + }, + }, + }); + + const completed = findByCorrelationId(tmpDir, 'background-critic-child'); + expect(completed?.status).toBe('consumed'); + expect(completed?.planTaskId).toBe(TASK_ONE); + expect(completed?.evidenceTaskId).toBe(TASK_ONE); + const evidence = await readTaskEvidence(tmpDir, TASK_ONE); + expect(evidence?.gates.critic).toBeDefined(); + expect(await hasPassedAllGates(tmpDir, TASK_ONE)).toBe(true); + }); + + it('uses real launch and settlement boundaries for over-limit plans', async () => { + const taskIds = Array.from( + { length: 1025 }, + (_, index) => `1.${index + 1}`, + ); + writePlan(tmpDir, makePlan(taskIds)); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, 'large-plan-valid', 'large-valid', { + subagent_type: 'critic', + task_id: LARGE_TASK, + prompt: 'Review the explicitly selected task.', + }); + const explicitEvidence = await readTaskEvidence(tmpDir, LARGE_TASK); + expect(explicitEvidence?.gates.critic).toBeDefined(); + expect(await hasPassedAllGates(tmpDir, LARGE_TASK)).toBe(true); + + await settleCritic(hook, 'large-plan-marker', 'large-marker', { + subagent_type: 'critic', + prompt: `TASK: ${LARGE_MARKER_TASK}`, + }); + const markerEvidence = await readTaskEvidence(tmpDir, LARGE_MARKER_TASK); + expect(markerEvidence?.gates.critic).toBeDefined(); + + await settleCritic(hook, 'large-plan-foreign', 'large-foreign', { + subagent_type: 'critic', + task_id: '9.9', + prompt: 'Review the explicitly selected task.', + }); + expect(await readTaskEvidence(tmpDir, '9.9')).toBeNull(); + }); +}); From b18b4174f982c49164f7bf214fd47107374af12d Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 17:59:21 -0500 Subject: [PATCH 2/5] fix(delegation-gate): harden critic task attribution Close the feedback loop by enforcing strict task identity for critic-family evidence, preserving reviewer/test-engineer plan routing, and documenting the delegation contract. Add regression coverage for mixed IDs, mutation re-gating, Windows-safe fixtures, and prompt guidance. --- .../fix-plan-critic-task-attribution-2757.md | 17 +- src/agents/architect.ts | 8 + src/hooks/delegation-gate.ts | 30 +++- src/hooks/task-id-resolver.ts | 4 +- .../architect-task-attribution-prompt.test.ts | 30 ++++ ...-gate-critic-task-attribution-2757.test.ts | 145 +++++++++++++++++- ...critic-task-attribution-large-plan.test.ts | 6 +- ...ate-critic-task-attribution-public.test.ts | 10 +- 8 files changed, 221 insertions(+), 29 deletions(-) create mode 100644 tests/unit/agents/architect-task-attribution-prompt.test.ts diff --git a/docs/releases/pending/fix-plan-critic-task-attribution-2757.md b/docs/releases/pending/fix-plan-critic-task-attribution-2757.md index 85f55856b..a74be9ec1 100644 --- a/docs/releases/pending/fix-plan-critic-task-attribution-2757.md +++ b/docs/releases/pending/fix-plan-critic-task-attribution-2757.md @@ -2,12 +2,17 @@ ## What changed -Plan-level `critic` and `critic_sounding_board` dispatches no longer acquire a -durable per-task gate merely because review prose mentions a plan task ID. -Those roles now require structured task attribution or an exact task marker at -launch, background pending capture, and foreground settlement. Reviewer and -test-engineer plan-aware routing is unchanged, including explicit task routing -for large plans. +Plan-level critic-family dispatches no longer acquire a durable per-task gate +merely because review prose mentions a plan task ID. `critic`, +`critic_sounding_board`, `critic_drift_verifier`, `critic_hallucination_verifier`, +and `critic_architecture_supervisor` now require structured task attribution or +an exact task marker at launch, background pending capture, and foreground +settlement. A non-strict named ID cannot shadow a valid numeric marker. Reviewer +and test-engineer plan-aware routing is unchanged, including explicit task +routing for large plans. + +The architect delegation contract now instructs task-scoped dispatches to keep +the numeric plan ID consistent across the `TASK:` line and `task_id` argument. ## Recovery diff --git a/src/agents/architect.ts b/src/agents/architect.ts index af574a4f9..f15101212 100644 --- a/src/agents/architect.ts +++ b/src/agents/architect.ts @@ -737,6 +737,14 @@ Mutation delegations are performed by calling the **Task** tool. Read-only advis All delegations MUST follow the receiving agent's INPUT FORMAT exactly. Do NOT invent fields, omit required fields, or force one agent's schema onto another. Every delegation MUST begin with the agent name, include \`TASK:\`, and include \`SKILLS:\` when that agent prompt supports skills. Do NOT add conversational preamble before the agent prefix. Begin directly with the agent name. +TASK ATTRIBUTION: For task-scoped delegations, put the exact numeric plan task ID +alone on a standalone \`TASK:\` line (for example, \`TASK: 1.1\`) and put the objective +on the following line. When the Task arguments support an explicit field, set +\`task_id\` to the same numeric value as a tool argument (not as prompt prose). Keep +the numeric ID consistent across the TASK line, \`task_id\`, and any acceptance text. +Plan-level critics and other project-wide reviews must omit task attribution rather +than guessing from ambient prose or session state. + {{AGENT_PREFIX}}[agent] TASK: [single objective] [agent-specific fields required by that agent's INPUT FORMAT] diff --git a/src/hooks/delegation-gate.ts b/src/hooks/delegation-gate.ts index c7e9e4a46..bb95faf5c 100644 --- a/src/hooks/delegation-gate.ts +++ b/src/hooks/delegation-gate.ts @@ -128,6 +128,7 @@ import { toTaskIdPlanContextOptions, } from './plan-task-id-context.js'; import { + EXPLICIT_TASK_ID_FIELDS, resolveDelegatedPlanTaskId, resolveTaskId, TASK_ID_RESOLUTION_LIMITS, @@ -2670,6 +2671,9 @@ const TASK_GATE_AGENTS = new Set([ const EXPLICIT_TASK_EVIDENCE_AGENTS = new Set([ 'critic', 'critic_sounding_board', + 'critic_drift_verifier', + 'critic_hallucination_verifier', + 'critic_architecture_supervisor', ]); function isExplicitTaskEvidenceAgent(targetAgent: string): boolean { @@ -3101,15 +3105,37 @@ async function resolveEvidenceTaskId( ...planContextOptions, }); if (resolution.status === 'resolved') { + let resolvedTaskId = resolution.taskId; + if (policy === 'attribution' && !isStrictTaskId(resolvedTaskId)) { + // The generic attribution resolver intentionally accepts safe named + // IDs for non-gate consumers. Durable task-gate evidence is stricter: + // retry marker-only attribution so a named explicit value cannot + // shadow a valid numeric TASK marker, then fail closed otherwise. + const markerOnlyArgs = { ...args }; + for (const field of EXPLICIT_TASK_ID_FIELDS) { + delete markerOnlyArgs[field]; + } + const markerResolution = resolveTaskId(markerOnlyArgs, { + policy, + ...planContextOptions, + }); + if ( + markerResolution.status !== 'resolved' || + !isStrictTaskId(markerResolution.taskId) + ) { + return null; + } + resolvedTaskId = markerResolution.taskId; + } if ( planTaskIdContext?.status === 'over_limit' && !plan?.phases.some((phase) => - phase?.tasks?.some((task) => task?.id === resolution.taskId), + phase?.tasks?.some((task) => task?.id === resolvedTaskId), ) ) { return null; } - return resolution.taskId; + return resolvedTaskId; } if ( resolution.status === 'invalid' || diff --git a/src/hooks/task-id-resolver.ts b/src/hooks/task-id-resolver.ts index 7e1154ad0..bbd75935b 100644 --- a/src/hooks/task-id-resolver.ts +++ b/src/hooks/task-id-resolver.ts @@ -45,7 +45,7 @@ export type TaskIdPlanContextOptions = Pick< >; const TEXT_FIELDS = ['prompt', 'description', 'task', 'input'] as const; -const EXPLICIT_FIELDS = [ +export const EXPLICIT_TASK_ID_FIELDS = [ 'plan_task_id', 'planTaskId', 'task_id', @@ -177,7 +177,7 @@ export function resolveTaskId( } const explicit = new Set(); - for (const field of EXPLICIT_FIELDS) { + for (const field of EXPLICIT_TASK_ID_FIELDS) { const raw = input[field]; if (raw === undefined || raw === null) continue; if (typeof raw !== 'string') return { status: 'invalid', input: field }; diff --git a/tests/unit/agents/architect-task-attribution-prompt.test.ts b/tests/unit/agents/architect-task-attribution-prompt.test.ts new file mode 100644 index 000000000..9783ca986 --- /dev/null +++ b/tests/unit/agents/architect-task-attribution-prompt.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'bun:test'; +import { createArchitectAgent } from '../../../src/agents/architect'; +import { resolveTaskId } from '../../../src/hooks/task-id-resolver'; + +describe('Architect prompt — task attribution guidance', () => { + it('requires numeric task identity on task-scoped delegations', () => { + const prompt = createArchitectAgent('test-model').config.prompt ?? ''; + + expect(prompt).toContain('TASK ATTRIBUTION'); + expect(prompt).toContain( + 'alone on a standalone `TASK:` line (for example, `TASK: 1.1`)', + ); + expect(prompt).not.toContain('TASK: 1.1 —'); + expect(prompt).toContain( + 'task_id` to the same numeric value as a tool argument', + ); + expect(prompt).toContain('Plan-level critics'); + + const shippedExample = /for example, `(TASK: \d+\.\d+(?:\.\d+)*)`/.exec( + prompt, + )?.[1]; + expect(shippedExample).toBe('TASK: 1.1'); + expect( + resolveTaskId( + { prompt: shippedExample }, + { policy: 'attribution', knownPlanTaskIds: new Set(['1.1']) }, + ), + ).toEqual({ status: 'resolved', taskId: '1.1', source: 'marker' }); + }); +}); diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts index b1c52b280..5dc4a4a5c 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts @@ -1,7 +1,7 @@ /** Regression coverage for issue #2757's plan-critic task attribution dead-end. */ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; -import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, writeFileSync } from 'node:fs'; import * as path from 'node:path'; import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; import { findByCorrelationId } from '../../../src/background/pending-delegations'; @@ -13,9 +13,13 @@ import { readTaskEvidence, transitionTaskWorkflowEvidence, } from '../../../src/gate-evidence'; -import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; +import { + createDelegationGateHook, + _internals as delegationGateInternals, +} from '../../../src/hooks/delegation-gate'; import { ensureAgentSession, resetSwarmState } from '../../../src/state'; import { createIsolatedTestEnv } from '../../helpers/isolated-test-env'; +import { safeRmRecursive } from '../../helpers/safe-test-dir'; import { canonicalMkdtemp } from '../../helpers/tmpdir'; import { makeConfig } from './_delegation-gate-helpers'; @@ -106,12 +110,7 @@ beforeEach(() => { afterEach(() => { resetSwarmState(); closeProjectDb(tmpDir); - rmSync(tmpDir, { - recursive: true, - force: true, - maxRetries: 5, - retryDelay: 100, - }); + safeRmRecursive(tmpDir); isolatedEnv?.cleanup(); isolatedEnv = undefined; }); @@ -224,6 +223,64 @@ describe('delegation-gate — regression: plan-level critic attribution (#2757)' expect(evidence?.gates.critic).toBeDefined(); }); + it('does not let a named ID shadow a valid TASK marker (F3)', async () => { + const session = ensureAgentSession( + 'critic-mixed-attribution', + 'architect', + tmpDir, + ); + const resolved = await delegationGateInternals.resolveEvidenceTaskId( + { + task_id: 'runtime-session-handle', + prompt: `MODE: CRITIC-GATE\nTASK: ${TASK_ID}`, + }, + session, + tmpDir, + { policy: 'attribution', allowSessionFallback: false }, + ); + + expect(resolved).toBe(TASK_ID); + }); + + it('re-satisfies explicit critic evidence after an accepted coder mutation (F14)', async () => { + const sessionID = 'critic-explicit-rerun'; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, sessionID, 'critic-before-mutation', { + subagent_type: 'critic', + task_id: TASK_ID, + prompt: 'MODE: CRITIC-GATE\nReview the explicit task before execution.', + }); + const beforeMutation = await readTaskEvidence(tmpDir, TASK_ID); + expect(beforeMutation?.gates.critic).toBeDefined(); + + const accepted = await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'accepted_mutation', + agentType: 'coder', + expectedGeneration: getTaskWorkflowSnapshot(beforeMutation).generation, + transitionId: 'coder-after-explicit-critic', + }); + const generation = getTaskWorkflowSnapshot(accepted).generation; + expect(accepted.required_gates).toContain('critic'); + expect(accepted.gates.critic).toBeUndefined(); + + await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'stage_a_passed', + expectedGeneration: generation, + transitionId: 'stage-a-before-explicit-critic-rerun', + }); + await settleCritic(hook, sessionID, 'critic-after-mutation', { + subagent_type: 'critic', + task_id: TASK_ID, + prompt: + 'MODE: CRITIC-GATE\nRe-review the explicit task after the coder mutation.', + }); + + const afterRerun = await readTaskEvidence(tmpDir, TASK_ID); + expect(afterRerun?.gates.critic).toBeDefined(); + }); + it('does not leave an unsatisfiable critic requirement after a coder mutation', async () => { const sessionID = 'plan-critic-orphan'; ensureAgentSession(sessionID, 'architect', tmpDir); @@ -325,4 +382,76 @@ describe('delegation-gate — regression: plan-level critic attribution (#2757)' }); expect(await hasPassedAllGates(tmpDir, TASK_ID)).toBe(true); }); + + it('preserves ambient plan-text routing for reviewer and test_engineer (F17)', async () => { + const sessionID = 'stage-b-plan-text'; + await seedStageA(sessionID); + const session = ensureAgentSession(sessionID, 'architect', tmpDir); + // Force this regression through the plan-text parser rather than the + // session fallback that is intentionally available to non-critic gates. + session.currentTaskId = null; + session.lastCoderDelegationTaskId = null; + const hook = createDelegationGateHook(makeConfig(), tmpDir); + const reviewerArgs = { + subagent_type: 'reviewer', + prompt: + 'Implementation plan excerpt:\n- [ ] 1.1: Review the implementation plan.\nACCEPTANCE: DONE = review complete.', + }; + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'reviewer-plan-text' }, + { args: reviewerArgs }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'reviewer-plan-text', + args: reviewerArgs, + }, + { + output: `[REVIEWED] | task-${TASK_ID} | APPROVED | plan text preserved`, + }, + ); + const testEngineerArgs = { + subagent_type: 'test_engineer', + prompt: + 'Implementation plan excerpt:\n- [ ] 1.1: Test the implementation plan.', + }; + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'test-engineer-plan-text' }, + { args: testEngineerArgs }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'test-engineer-plan-text', + args: testEngineerArgs, + }, + { output: `[TESTED] | task-${TASK_ID} | PASS | plan text preserved` }, + ); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.gates.reviewer).toBeDefined(); + expect(evidence?.gates.test_engineer).toBeDefined(); + }); + + it('requires explicit attribution for every critic-family role (F15)', async () => { + const roles = [ + 'critic_drift_verifier', + 'critic_hallucination_verifier', + 'critic_architecture_supervisor', + ] as const; + for (const role of roles) { + const sessionID = `unbound-${role}`; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + await settleCritic(hook, sessionID, `call-${role}`, { + subagent_type: role, + prompt: `MODE: CRITIC-GATE\nReview the plan; task 1.1 is mentioned in prose.`, + }); + } + + expect(await readTaskEvidence(tmpDir, TASK_ID)).toBeNull(); + }); }); diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts index 0eeef3dd7..1a2502c54 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts @@ -1,11 +1,11 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; -import * as os from 'node:os'; import * as path from 'node:path'; import { _internals as delegationGateInternals } from '../../../src/hooks/delegation-gate'; import { TASK_ID_RESOLUTION_LIMITS } from '../../../src/hooks/task-id-resolver'; import { ensureAgentSession, resetSwarmState } from '../../../src/state'; import { safeRmRecursive } from '../../helpers/safe-test-dir'; +import { canonicalMkdtemp } from '../../helpers/tmpdir'; const { resolveEvidenceTaskId } = delegationGateInternals; const LARGE_PLAN_TASK_ID = `1.${TASK_ID_RESOLUTION_LIMITS.maxKnownIds + 1}`; @@ -42,9 +42,7 @@ let directory: string; beforeEach(() => { resetSwarmState(); - directory = fs.realpathSync( - fs.mkdtempSync(path.join(os.tmpdir(), 'critic-large-plan-')), - ); + directory = canonicalMkdtemp('critic-large-plan-'); fs.mkdirSync(path.join(directory, '.swarm'), { recursive: true }); fs.writeFileSync( path.join(directory, '.swarm', 'plan.json'), diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts index 8c4d30b3b..2ca906cf0 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts @@ -1,7 +1,7 @@ /** Public-hook guardrails for issue #2757 critic task attribution. */ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; -import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, writeFileSync } from 'node:fs'; import * as path from 'node:path'; import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; import { findByCorrelationId } from '../../../src/background/pending-delegations'; @@ -16,6 +16,7 @@ import { import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; import { ensureAgentSession, resetSwarmState } from '../../../src/state'; import { createIsolatedTestEnv } from '../../helpers/isolated-test-env'; +import { safeRmRecursive } from '../../helpers/safe-test-dir'; import { canonicalMkdtemp } from '../../helpers/tmpdir'; import { makeConfig } from './_delegation-gate-helpers'; @@ -107,12 +108,7 @@ beforeEach(() => { afterEach(() => { resetSwarmState(); closeProjectDb(tmpDir); - rmSync(tmpDir, { - recursive: true, - force: true, - maxRetries: 5, - retryDelay: 100, - }); + safeRmRecursive(tmpDir); isolatedEnv?.cleanup(); isolatedEnv = undefined; }); From 445262c2cf49c2f05194c9655b571f2a4f34a5cd Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 17:59:21 -0500 Subject: [PATCH 3/5] fix(delegation-gate): harden critic task attribution Close the feedback loop by enforcing strict task identity for critic-family evidence, preserving reviewer/test-engineer plan routing, and documenting the delegation contract. Add regression coverage for mixed IDs, mutation re-gating, Windows-safe fixtures, and prompt guidance. --- .../fix-plan-critic-task-attribution-2757.md | 17 ++- src/agents/architect.ts | 8 + src/hooks/delegation-gate.ts | 30 +++- src/hooks/task-id-resolver.ts | 4 +- .../architect-task-attribution-prompt.test.ts | 30 ++++ ...-gate-critic-task-attribution-2757.test.ts | 137 +++++++++++++++++- ...critic-task-attribution-large-plan.test.ts | 6 +- ...ate-critic-task-attribution-public.test.ts | 10 +- 8 files changed, 214 insertions(+), 28 deletions(-) create mode 100644 tests/unit/agents/architect-task-attribution-prompt.test.ts diff --git a/docs/releases/pending/fix-plan-critic-task-attribution-2757.md b/docs/releases/pending/fix-plan-critic-task-attribution-2757.md index 85f55856b..a74be9ec1 100644 --- a/docs/releases/pending/fix-plan-critic-task-attribution-2757.md +++ b/docs/releases/pending/fix-plan-critic-task-attribution-2757.md @@ -2,12 +2,17 @@ ## What changed -Plan-level `critic` and `critic_sounding_board` dispatches no longer acquire a -durable per-task gate merely because review prose mentions a plan task ID. -Those roles now require structured task attribution or an exact task marker at -launch, background pending capture, and foreground settlement. Reviewer and -test-engineer plan-aware routing is unchanged, including explicit task routing -for large plans. +Plan-level critic-family dispatches no longer acquire a durable per-task gate +merely because review prose mentions a plan task ID. `critic`, +`critic_sounding_board`, `critic_drift_verifier`, `critic_hallucination_verifier`, +and `critic_architecture_supervisor` now require structured task attribution or +an exact task marker at launch, background pending capture, and foreground +settlement. A non-strict named ID cannot shadow a valid numeric marker. Reviewer +and test-engineer plan-aware routing is unchanged, including explicit task +routing for large plans. + +The architect delegation contract now instructs task-scoped dispatches to keep +the numeric plan ID consistent across the `TASK:` line and `task_id` argument. ## Recovery diff --git a/src/agents/architect.ts b/src/agents/architect.ts index af574a4f9..f15101212 100644 --- a/src/agents/architect.ts +++ b/src/agents/architect.ts @@ -737,6 +737,14 @@ Mutation delegations are performed by calling the **Task** tool. Read-only advis All delegations MUST follow the receiving agent's INPUT FORMAT exactly. Do NOT invent fields, omit required fields, or force one agent's schema onto another. Every delegation MUST begin with the agent name, include \`TASK:\`, and include \`SKILLS:\` when that agent prompt supports skills. Do NOT add conversational preamble before the agent prefix. Begin directly with the agent name. +TASK ATTRIBUTION: For task-scoped delegations, put the exact numeric plan task ID +alone on a standalone \`TASK:\` line (for example, \`TASK: 1.1\`) and put the objective +on the following line. When the Task arguments support an explicit field, set +\`task_id\` to the same numeric value as a tool argument (not as prompt prose). Keep +the numeric ID consistent across the TASK line, \`task_id\`, and any acceptance text. +Plan-level critics and other project-wide reviews must omit task attribution rather +than guessing from ambient prose or session state. + {{AGENT_PREFIX}}[agent] TASK: [single objective] [agent-specific fields required by that agent's INPUT FORMAT] diff --git a/src/hooks/delegation-gate.ts b/src/hooks/delegation-gate.ts index c7e9e4a46..bb95faf5c 100644 --- a/src/hooks/delegation-gate.ts +++ b/src/hooks/delegation-gate.ts @@ -128,6 +128,7 @@ import { toTaskIdPlanContextOptions, } from './plan-task-id-context.js'; import { + EXPLICIT_TASK_ID_FIELDS, resolveDelegatedPlanTaskId, resolveTaskId, TASK_ID_RESOLUTION_LIMITS, @@ -2670,6 +2671,9 @@ const TASK_GATE_AGENTS = new Set([ const EXPLICIT_TASK_EVIDENCE_AGENTS = new Set([ 'critic', 'critic_sounding_board', + 'critic_drift_verifier', + 'critic_hallucination_verifier', + 'critic_architecture_supervisor', ]); function isExplicitTaskEvidenceAgent(targetAgent: string): boolean { @@ -3101,15 +3105,37 @@ async function resolveEvidenceTaskId( ...planContextOptions, }); if (resolution.status === 'resolved') { + let resolvedTaskId = resolution.taskId; + if (policy === 'attribution' && !isStrictTaskId(resolvedTaskId)) { + // The generic attribution resolver intentionally accepts safe named + // IDs for non-gate consumers. Durable task-gate evidence is stricter: + // retry marker-only attribution so a named explicit value cannot + // shadow a valid numeric TASK marker, then fail closed otherwise. + const markerOnlyArgs = { ...args }; + for (const field of EXPLICIT_TASK_ID_FIELDS) { + delete markerOnlyArgs[field]; + } + const markerResolution = resolveTaskId(markerOnlyArgs, { + policy, + ...planContextOptions, + }); + if ( + markerResolution.status !== 'resolved' || + !isStrictTaskId(markerResolution.taskId) + ) { + return null; + } + resolvedTaskId = markerResolution.taskId; + } if ( planTaskIdContext?.status === 'over_limit' && !plan?.phases.some((phase) => - phase?.tasks?.some((task) => task?.id === resolution.taskId), + phase?.tasks?.some((task) => task?.id === resolvedTaskId), ) ) { return null; } - return resolution.taskId; + return resolvedTaskId; } if ( resolution.status === 'invalid' || diff --git a/src/hooks/task-id-resolver.ts b/src/hooks/task-id-resolver.ts index 7e1154ad0..bbd75935b 100644 --- a/src/hooks/task-id-resolver.ts +++ b/src/hooks/task-id-resolver.ts @@ -45,7 +45,7 @@ export type TaskIdPlanContextOptions = Pick< >; const TEXT_FIELDS = ['prompt', 'description', 'task', 'input'] as const; -const EXPLICIT_FIELDS = [ +export const EXPLICIT_TASK_ID_FIELDS = [ 'plan_task_id', 'planTaskId', 'task_id', @@ -177,7 +177,7 @@ export function resolveTaskId( } const explicit = new Set(); - for (const field of EXPLICIT_FIELDS) { + for (const field of EXPLICIT_TASK_ID_FIELDS) { const raw = input[field]; if (raw === undefined || raw === null) continue; if (typeof raw !== 'string') return { status: 'invalid', input: field }; diff --git a/tests/unit/agents/architect-task-attribution-prompt.test.ts b/tests/unit/agents/architect-task-attribution-prompt.test.ts new file mode 100644 index 000000000..9783ca986 --- /dev/null +++ b/tests/unit/agents/architect-task-attribution-prompt.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'bun:test'; +import { createArchitectAgent } from '../../../src/agents/architect'; +import { resolveTaskId } from '../../../src/hooks/task-id-resolver'; + +describe('Architect prompt — task attribution guidance', () => { + it('requires numeric task identity on task-scoped delegations', () => { + const prompt = createArchitectAgent('test-model').config.prompt ?? ''; + + expect(prompt).toContain('TASK ATTRIBUTION'); + expect(prompt).toContain( + 'alone on a standalone `TASK:` line (for example, `TASK: 1.1`)', + ); + expect(prompt).not.toContain('TASK: 1.1 —'); + expect(prompt).toContain( + 'task_id` to the same numeric value as a tool argument', + ); + expect(prompt).toContain('Plan-level critics'); + + const shippedExample = /for example, `(TASK: \d+\.\d+(?:\.\d+)*)`/.exec( + prompt, + )?.[1]; + expect(shippedExample).toBe('TASK: 1.1'); + expect( + resolveTaskId( + { prompt: shippedExample }, + { policy: 'attribution', knownPlanTaskIds: new Set(['1.1']) }, + ), + ).toEqual({ status: 'resolved', taskId: '1.1', source: 'marker' }); + }); +}); diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts index b1c52b280..0e9d9f8a7 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts @@ -1,7 +1,7 @@ /** Regression coverage for issue #2757's plan-critic task attribution dead-end. */ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; -import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, writeFileSync } from 'node:fs'; import * as path from 'node:path'; import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; import { findByCorrelationId } from '../../../src/background/pending-delegations'; @@ -16,6 +16,7 @@ import { import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; import { ensureAgentSession, resetSwarmState } from '../../../src/state'; import { createIsolatedTestEnv } from '../../helpers/isolated-test-env'; +import { safeRmRecursive } from '../../helpers/safe-test-dir'; import { canonicalMkdtemp } from '../../helpers/tmpdir'; import { makeConfig } from './_delegation-gate-helpers'; @@ -106,12 +107,7 @@ beforeEach(() => { afterEach(() => { resetSwarmState(); closeProjectDb(tmpDir); - rmSync(tmpDir, { - recursive: true, - force: true, - maxRetries: 5, - retryDelay: 100, - }); + safeRmRecursive(tmpDir); isolatedEnv?.cleanup(); isolatedEnv = undefined; }); @@ -224,6 +220,61 @@ describe('delegation-gate — regression: plan-level critic attribution (#2757)' expect(evidence?.gates.critic).toBeDefined(); }); + it('does not let a named ID shadow a valid TASK marker (F3)', async () => { + const sessionID = 'critic-mixed-attribution'; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, sessionID, 'critic-mixed-attribution', { + subagent_type: 'critic', + task_id: 'runtime-session-handle', + prompt: `MODE: CRITIC-GATE\nTASK: ${TASK_ID}`, + }); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.required_gates).toContain('critic'); + expect(evidence?.gates.critic).toBeDefined(); + }); + + it('re-satisfies explicit critic evidence after an accepted coder mutation (F14)', async () => { + const sessionID = 'critic-explicit-rerun'; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, sessionID, 'critic-before-mutation', { + subagent_type: 'critic', + task_id: TASK_ID, + prompt: 'MODE: CRITIC-GATE\nReview the explicit task before execution.', + }); + const beforeMutation = await readTaskEvidence(tmpDir, TASK_ID); + expect(beforeMutation?.gates.critic).toBeDefined(); + + const accepted = await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'accepted_mutation', + agentType: 'coder', + expectedGeneration: getTaskWorkflowSnapshot(beforeMutation).generation, + transitionId: 'coder-after-explicit-critic', + }); + const generation = getTaskWorkflowSnapshot(accepted).generation; + expect(accepted.required_gates).toContain('critic'); + expect(accepted.gates.critic).toBeUndefined(); + + await transitionTaskWorkflowEvidence(tmpDir, TASK_ID, { + type: 'stage_a_passed', + expectedGeneration: generation, + transitionId: 'stage-a-before-explicit-critic-rerun', + }); + await settleCritic(hook, sessionID, 'critic-after-mutation', { + subagent_type: 'critic', + task_id: TASK_ID, + prompt: + 'MODE: CRITIC-GATE\nRe-review the explicit task after the coder mutation.', + }); + + const afterRerun = await readTaskEvidence(tmpDir, TASK_ID); + expect(afterRerun?.gates.critic).toBeDefined(); + }); + it('does not leave an unsatisfiable critic requirement after a coder mutation', async () => { const sessionID = 'plan-critic-orphan'; ensureAgentSession(sessionID, 'architect', tmpDir); @@ -325,4 +376,76 @@ describe('delegation-gate — regression: plan-level critic attribution (#2757)' }); expect(await hasPassedAllGates(tmpDir, TASK_ID)).toBe(true); }); + + it('preserves ambient plan-text routing for reviewer and test_engineer (F17)', async () => { + const sessionID = 'stage-b-plan-text'; + await seedStageA(sessionID); + const session = ensureAgentSession(sessionID, 'architect', tmpDir); + // Force this regression through the plan-text parser rather than the + // session fallback that is intentionally available to non-critic gates. + session.currentTaskId = null; + session.lastCoderDelegationTaskId = null; + const hook = createDelegationGateHook(makeConfig(), tmpDir); + const reviewerArgs = { + subagent_type: 'reviewer', + prompt: + 'Implementation plan excerpt:\n- [ ] 1.1: Review the implementation plan.\nACCEPTANCE: DONE = review complete.', + }; + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'reviewer-plan-text' }, + { args: reviewerArgs }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'reviewer-plan-text', + args: reviewerArgs, + }, + { + output: `[REVIEWED] | task-${TASK_ID} | APPROVED | plan text preserved`, + }, + ); + const testEngineerArgs = { + subagent_type: 'test_engineer', + prompt: + 'Implementation plan excerpt:\n- [ ] 1.1: Test the implementation plan.', + }; + await hook.toolBefore( + { tool: 'Task', sessionID, callID: 'test-engineer-plan-text' }, + { args: testEngineerArgs }, + ); + await hook.toolAfter( + { + tool: 'Task', + sessionID, + callID: 'test-engineer-plan-text', + args: testEngineerArgs, + }, + { output: `[TESTED] | task-${TASK_ID} | PASS | plan text preserved` }, + ); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.gates.reviewer).toBeDefined(); + expect(evidence?.gates.test_engineer).toBeDefined(); + }); + + it('requires explicit attribution for every critic-family role (F15)', async () => { + const roles = [ + 'critic_drift_verifier', + 'critic_hallucination_verifier', + 'critic_architecture_supervisor', + ] as const; + for (const role of roles) { + const sessionID = `unbound-${role}`; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + await settleCritic(hook, sessionID, `call-${role}`, { + subagent_type: role, + prompt: `MODE: CRITIC-GATE\nReview the plan; task 1.1 is mentioned in prose.`, + }); + } + + expect(await readTaskEvidence(tmpDir, TASK_ID)).toBeNull(); + }); }); diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts index 0eeef3dd7..1a2502c54 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-large-plan.test.ts @@ -1,11 +1,11 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; -import * as os from 'node:os'; import * as path from 'node:path'; import { _internals as delegationGateInternals } from '../../../src/hooks/delegation-gate'; import { TASK_ID_RESOLUTION_LIMITS } from '../../../src/hooks/task-id-resolver'; import { ensureAgentSession, resetSwarmState } from '../../../src/state'; import { safeRmRecursive } from '../../helpers/safe-test-dir'; +import { canonicalMkdtemp } from '../../helpers/tmpdir'; const { resolveEvidenceTaskId } = delegationGateInternals; const LARGE_PLAN_TASK_ID = `1.${TASK_ID_RESOLUTION_LIMITS.maxKnownIds + 1}`; @@ -42,9 +42,7 @@ let directory: string; beforeEach(() => { resetSwarmState(); - directory = fs.realpathSync( - fs.mkdtempSync(path.join(os.tmpdir(), 'critic-large-plan-')), - ); + directory = canonicalMkdtemp('critic-large-plan-'); fs.mkdirSync(path.join(directory, '.swarm'), { recursive: true }); fs.writeFileSync( path.join(directory, '.swarm', 'plan.json'), diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts index 8c4d30b3b..2ca906cf0 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-public.test.ts @@ -1,7 +1,7 @@ /** Public-hook guardrails for issue #2757 critic task attribution. */ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; -import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, writeFileSync } from 'node:fs'; import * as path from 'node:path'; import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; import { findByCorrelationId } from '../../../src/background/pending-delegations'; @@ -16,6 +16,7 @@ import { import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; import { ensureAgentSession, resetSwarmState } from '../../../src/state'; import { createIsolatedTestEnv } from '../../helpers/isolated-test-env'; +import { safeRmRecursive } from '../../helpers/safe-test-dir'; import { canonicalMkdtemp } from '../../helpers/tmpdir'; import { makeConfig } from './_delegation-gate-helpers'; @@ -107,12 +108,7 @@ beforeEach(() => { afterEach(() => { resetSwarmState(); closeProjectDb(tmpDir); - rmSync(tmpDir, { - recursive: true, - force: true, - maxRetries: 5, - retryDelay: 100, - }); + safeRmRecursive(tmpDir); isolatedEnv?.cleanup(); isolatedEnv = undefined; }); From ecc69a46e0a386113e3178606701bdeda799a4de Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 21:23:34 -0500 Subject: [PATCH 4/5] fix(delegation-gate): reject quoted critic task markers --- src/hooks/delegation-gate.ts | 16 ++-- src/hooks/task-id-resolver.ts | 74 +++++++++++++++++-- ...-gate-critic-task-attribution-2757.test.ts | 36 +++++++++ 3 files changed, 113 insertions(+), 13 deletions(-) diff --git a/src/hooks/delegation-gate.ts b/src/hooks/delegation-gate.ts index afb21d5c6..2a415a2ca 100644 --- a/src/hooks/delegation-gate.ts +++ b/src/hooks/delegation-gate.ts @@ -3100,10 +3100,14 @@ async function resolveEvidenceTaskId( : planTaskIdContext ? toTaskIdPlanContextOptions(planTaskIdContext) : {}; - const resolution = resolveTaskId(args, { + const resolutionOptions = { policy, ...planContextOptions, - }); + // Durable critic gates accept only structured IDs or a bare TASK + // marker; quoted and example text is not dispatch attribution. + standaloneTaskMarkerOnly: policy === 'attribution', + }; + const resolution = resolveTaskId(args, resolutionOptions); if (resolution.status === 'resolved') { let resolvedTaskId = resolution.taskId; if (policy === 'attribution' && !isStrictTaskId(resolvedTaskId)) { @@ -3115,10 +3119,10 @@ async function resolveEvidenceTaskId( for (const field of EXPLICIT_TASK_ID_FIELDS) { delete markerOnlyArgs[field]; } - const markerResolution = resolveTaskId(markerOnlyArgs, { - policy, - ...planContextOptions, - }); + const markerResolution = resolveTaskId( + markerOnlyArgs, + resolutionOptions, + ); if ( markerResolution.status !== 'resolved' || !isStrictTaskId(markerResolution.taskId) diff --git a/src/hooks/task-id-resolver.ts b/src/hooks/task-id-resolver.ts index bbd75935b..722bc8a24 100644 --- a/src/hooks/task-id-resolver.ts +++ b/src/hooks/task-id-resolver.ts @@ -32,6 +32,12 @@ export type TaskIdResolution = export interface ResolveTaskIdOptions { policy: TaskIdPolicy; + /** + * Restrict text-derived attribution to bare, standalone TASK lines. This is + * used for durable critic evidence, where quoted/example text is not proof + * that a critic was dispatched for a task. Structured ID fields still win. + */ + standaloneTaskMarkerOnly?: boolean; knownPlanTaskIds?: ReadonlySet; /** The caller observed a valid plan whose task-ID cardinality exceeded the bound. */ planContextOverLimit?: boolean; @@ -63,10 +69,58 @@ const ATTRIBUTION_ID_MARKER = // separately handles numeric IDs embedded in prose. const ATTRIBUTION_TASK_MARKER = /\bTASK\s*[:=]\s*([A-Za-z0-9][A-Za-z0-9._-]*)[ \t]*(?=\r?$)/gim; +const ATTRIBUTION_TASK_MARKER_STANDALONE = + /^TASK\s*[:=]\s*([A-Za-z0-9][A-Za-z0-9._-]*)[ \t]*\r?$/gim; const ATTRIBUTION_ID_MARKER_RAW = /\b(?:task_id|task-id|taskId)\s*[:=][ \t]*([^\s]*)/gi; const ATTRIBUTION_TASK_MARKER_RAW = /\bTASK\s*[:=][ \t]*([^\s]+)[ \t]*(?=\r?$)/gim; +const ATTRIBUTION_TASK_MARKER_RAW_STANDALONE = + /^TASK\s*[:=][ \t]*([^\s]+)[ \t]*\r?$/gim; + +/** Remove Markdown code/quote blocks before treating free text as evidence. */ +function stripUntrustedAttributionMarkdown(text: string): string { + const lines = text.split(/\r?\n/); + const kept: string[] = []; + let fence: { marker: '`' | '~'; length: number } | undefined; + let inBlockQuote = false; + + for (const line of lines) { + if (fence) { + const close = line.match(/^[ \t]{0,3}(`+|~+)[ \t]*$/); + if ( + close && + close[1][0] === fence.marker && + close[1].length >= fence.length + ) { + fence = undefined; + } + continue; + } + + const open = line.match(/^[ \t]{0,3}(`{3,}|~{3,})(.*)$/); + if (open && !(open[1][0] === '`' && open[2].includes('`'))) { + fence = { marker: open[1][0] as '`' | '~', length: open[1].length }; + continue; + } + + if (/^[ \t]{0,3}>/.test(line)) { + inBlockQuote = true; + continue; + } + if (inBlockQuote) { + if (line.trim() === '') { + inBlockQuote = false; + } else { + // Markdown allows lazy continuation lines within a block quote. + continue; + } + } + kept.push(line); + } + + return kept.join('\n'); +} function isSafeAttributionId(value: string): boolean { return ( @@ -270,12 +324,15 @@ export function resolveTaskId( const textSelection = select(textCandidates, 'text'); if (textSelection) return textSelection; } else { + const markerTextFields = options.standaloneTaskMarkerOnly + ? textFields.map(stripUntrustedAttributionMarkdown) + : textFields; let hasInvalidRawMarker = false; - for (const rawMarker of [ - ATTRIBUTION_ID_MARKER_RAW, - ATTRIBUTION_TASK_MARKER_RAW, - ]) { - for (const text of textFields) { + const rawMarkers = options.standaloneTaskMarkerOnly + ? [ATTRIBUTION_TASK_MARKER_RAW_STANDALONE] + : [ATTRIBUTION_ID_MARKER_RAW, ATTRIBUTION_TASK_MARKER_RAW]; + for (const rawMarker of rawMarkers) { + for (const text of markerTextFields) { rawMarker.lastIndex = 0; for (const match of text.matchAll(rawMarker)) { const value = match[1]; @@ -296,8 +353,11 @@ export function resolveTaskId( } if (hasInvalidRawMarker) return { status: 'invalid', input: 'marker' }; const marked = new Set(); - for (const marker of [ATTRIBUTION_ID_MARKER, ATTRIBUTION_TASK_MARKER]) { - for (const text of textFields) { + const markers = options.standaloneTaskMarkerOnly + ? [ATTRIBUTION_TASK_MARKER_STANDALONE] + : [ATTRIBUTION_ID_MARKER, ATTRIBUTION_TASK_MARKER]; + for (const marker of markers) { + for (const text of markerTextFields) { marker.lastIndex = 0; for (const match of text.matchAll(marker)) { const value = match[1]; diff --git a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts index 0e9d9f8a7..5ab587e55 100644 --- a/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts +++ b/tests/unit/hooks/delegation-gate-critic-task-attribution-2757.test.ts @@ -236,6 +236,42 @@ describe('delegation-gate — regression: plan-level critic attribution (#2757)' expect(evidence?.gates.critic).toBeDefined(); }); + for (const [context, prompt] of [ + ['fenced code', `\`\`\`text\nTASK: ${TASK_ID}\n\`\`\``], + ['Markdown blockquote', `> TASK: ${TASK_ID}`], + ['quoted text', `"TASK: ${TASK_ID}"`], + ['prose-suffixed marker', `Review this prompt: TASK: ${TASK_ID}`], + ['free-text task_id marker', `task_id: ${TASK_ID}`], + ] as const) { + it(`does not record critic evidence from ${context} TASK text (F7)`, async () => { + const sessionID = `critic-untrusted-${context.replaceAll(' ', '-')}`; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, sessionID, sessionID, { + subagent_type: 'critic', + prompt, + }); + + expect(await readTaskEvidence(tmpDir, TASK_ID)).toBeNull(); + }); + } + + it('records critic evidence from a bare standalone TASK marker (F7)', async () => { + const sessionID = 'critic-bare-task-marker'; + ensureAgentSession(sessionID, 'architect', tmpDir); + const hook = createDelegationGateHook(makeConfig(), tmpDir); + + await settleCritic(hook, sessionID, sessionID, { + subagent_type: 'critic', + prompt: `Review the exact task.\nTASK: ${TASK_ID}`, + }); + + const evidence = await readTaskEvidence(tmpDir, TASK_ID); + expect(evidence?.required_gates).toContain('critic'); + expect(evidence?.gates.critic).toBeDefined(); + }); + it('re-satisfies explicit critic evidence after an accepted coder mutation (F14)', async () => { const sessionID = 'critic-explicit-rerun'; ensureAgentSession(sessionID, 'architect', tmpDir); From a83ebe73a81a28e755fad81ffc67e106d95e596f Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 23:43:25 -0500 Subject: [PATCH 5/5] test(pr-review): cover taskless reentry fallback --- ...orkflow-taskless-reentry-real-host.test.ts | 127 +++++++++++++++++- 1 file changed, 126 insertions(+), 1 deletion(-) diff --git a/tests/integration/pr-workflow-taskless-reentry-real-host.test.ts b/tests/integration/pr-workflow-taskless-reentry-real-host.test.ts index c1adcc1c2..43183a7ad 100644 --- a/tests/integration/pr-workflow-taskless-reentry-real-host.test.ts +++ b/tests/integration/pr-workflow-taskless-reentry-real-host.test.ts @@ -6,8 +6,11 @@ import { createDelegationGateHook } from '../../src/hooks/delegation-gate.js'; import { _test_exports, activatePrWorkflow, + hasActivePrReviewReentryAuthorization, + readPrReviewReentryBindingContext, } from '../../src/hooks/pr-workflow-gate.js'; import { issuePrReviewReentryAuthorization } from '../../src/pr-review/authorization.js'; +import { readReviewRouteReceipt } from '../../src/review/routing-enforcement.js'; import { ensureAgentSession, resetSwarmState, @@ -18,6 +21,7 @@ import { bootKnowledgeHost, createKnowledgeProject, } from '../helpers/knowledge-real-host.js'; +import { safeRmRecursive } from '../helpers/safe-test-dir.js'; const SESSION_ID = 'pr-workflow-taskless-reentry'; const HEAD_SHA = 'abc123'; @@ -154,6 +158,127 @@ describe('PR workflow taskless re-entry stays standalone-only', () => { } }); + test('plan-free re-entry ignores stale session task IDs and consumes authorization', async () => { + const staleSessionID = 'pr-workflow-stale-task-reentry'; + const staleTaskId = '1.1'; + const planFreeDirectory = createKnowledgeProject(); + try { + // This fixture deliberately does not boot the host or write an approved plan. + // Declare the temp directory as its own project root so an ancestor .swarm + // (for example, the developer profile) cannot block route-receipt writes. + mkdirSync(path.join(planFreeDirectory, '.git'), { recursive: true }); + await activatePrWorkflow(planFreeDirectory, staleSessionID, 'PR_REVIEW', { + prHeadSha: HEAD_SHA, + }); + expect( + await readPrReviewReentryBindingContext( + planFreeDirectory, + staleSessionID, + ), + ).toMatchObject({ prHeadSha: HEAD_SHA }); + const issued = await issuePrReviewReentryAuthorization( + planFreeDirectory, + staleSessionID, + { prHeadSha: HEAD_SHA, role: 'reviewer' }, + ); + expect(issued.role).toBe('reviewer'); + expect( + await hasActivePrReviewReentryAuthorization( + planFreeDirectory, + staleSessionID, + { role: 'reviewer' }, + ), + ).toBe(true); + + const session = ensureAgentSession( + staleSessionID, + 'architect', + planFreeDirectory, + ); + expect(session.taskWorkflowStates.size).toBe(0); + session.currentTaskId = staleTaskId; + + const hook = createDelegationGateHook( + { hooks: { delegation_gate: true } } as PluginConfig, + planFreeDirectory, + ); + const args = { + subagent_type: 'reviewer', + prompt: + 'Review the bound PR without a plan task.\nACCEPTANCE: report the review result.', + }; + await hook.toolBefore( + { + tool: 'Task', + sessionID: staleSessionID, + callID: 'stale-task-reviewer', + }, + { args }, + ); + expect('task_id' in args).toBe(false); + expect( + await hasActivePrReviewReentryAuthorization( + planFreeDirectory, + staleSessionID, + { role: 'reviewer' }, + ), + ).toBe(false); + expect( + await readPrReviewReentryBindingContext( + planFreeDirectory, + staleSessionID, + ), + ).toMatchObject({ prHeadSha: HEAD_SHA }); + expect( + await readReviewRouteReceipt({ + projectRoot: planFreeDirectory, + sessionId: staleSessionID, + taskId: staleTaskId, + }), + ).toBeNull(); + expect( + await readReviewRouteReceipt({ + projectRoot: planFreeDirectory, + sessionId: staleSessionID, + taskId: 'unresolved-stale-task-reviewer', + }), + ).toMatchObject({ taskId: 'unresolved-stale-task-reviewer' }); + await hook.toolAfter( + { + tool: 'Task', + sessionID: staleSessionID, + callID: 'stale-task-reviewer', + args, + }, + { + status: 'completed', + text: `[REVIEWED] | task-${staleTaskId} | APPROVED | stale-task bait`, + }, + ); + + expect(session.currentTaskId).toBe(staleTaskId); + expect(session.taskWorkflowStates.size).toBe(0); + expect( + await readPrReviewReentryBindingContext( + planFreeDirectory, + staleSessionID, + ), + ).toMatchObject({ prHeadSha: HEAD_SHA }); + await expect( + hook.toolBefore( + { + tool: 'Task', + sessionID: staleSessionID, + callID: 'stale-task-reviewer-replay', + }, + { args }, + ), + ).rejects.toThrow(/TASK_WORKFLOW_STAGE_A_REQUIRED/); + } finally { + safeRmRecursive(planFreeDirectory); + } + }); + test('taskless re-entry refuses to guess when task workflow state exists', async () => { await activatePrWorkflow(directory, SESSION_ID, 'PR_REVIEW', { prHeadSha: HEAD_SHA, @@ -271,7 +396,7 @@ describe('PR workflow taskless re-entry stays standalone-only', () => { } finally { await Promise.allSettled([...swarmState.pendingRehydrations]); swarmState.pendingRehydrations.clear(); - rmSync(pendingDirectory, { recursive: true, force: true }); + safeRmRecursive(pendingDirectory); } }); });