Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions docs/releases/pending/fix-plan-critic-task-attribution-2757.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Plan-critic task attribution fix (#2757)

## What changed

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

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`.
8 changes: 8 additions & 0 deletions src/agents/architect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
110 changes: 90 additions & 20 deletions src/hooks/delegation-gate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,11 @@ import {
toTaskIdPlanContextOptions,
} from './plan-task-id-context.js';
import {
EXPLICIT_TASK_ID_FIELDS,
resolveDelegatedPlanTaskId,
resolveTaskId,
TASK_ID_RESOLUTION_LIMITS,
type TaskIdPolicy,
} from './task-id-resolver.js';

export { resolveDelegatedPlanTaskId } from './task-id-resolver.js';
Expand Down Expand Up @@ -2666,6 +2668,34 @@ const TASK_GATE_AGENTS = new Set([
'sme',
]);

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 {
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;
Expand Down Expand Up @@ -3026,10 +3056,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.
Expand All @@ -3039,7 +3069,7 @@ async function resolveEvidenceTaskId(
args: Record<string, unknown> | undefined,
session: AgentSessionState,
directory: string,
options: { allowSessionFallback?: boolean } = {},
options: EvidenceTaskResolutionOptions = {},
): Promise<string | null> {
// Shared bounded resolution first; session fallback is allowed only when the
// resolver had no safe plan context and therefore made no authoritative
Expand All @@ -3058,22 +3088,58 @@ async function resolveEvidenceTaskId(

if (args) {
try {
const resolution = resolveTaskId(args, {
policy: 'plan',
...(planTaskIdContext
? toTaskIdPlanContextOptions(planTaskIdContext)
: {}),
});
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 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)) {
// 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,
resolutionOptions,
);
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' ||
Expand Down Expand Up @@ -4306,12 +4372,15 @@ export function createDelegationGateHook(
args,
stageBSession,
directory,
activePrReviewBinding ? { allowSessionFallback: false } : undefined,
evidenceTaskResolutionOptions(
targetAgent,
activePrReviewBinding ? false : undefined,
),
);
const candidateTaskIds = new Set<string>();
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),
Expand Down Expand Up @@ -5349,10 +5418,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 &&
Expand Down Expand Up @@ -5388,9 +5460,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 ?? '')
Expand Down Expand Up @@ -6208,10 +6278,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
Expand Down Expand Up @@ -6239,8 +6311,6 @@ export function createDelegationGateHook(
'explorer',
'sme',
];
const targetAgentForEvidence =
stripKnownSwarmPrefix(subagentType);
if (gateAgents.includes(targetAgentForEvidence)) {
if (
targetAgentForEvidence === 'reviewer' ||
Expand Down
78 changes: 69 additions & 9 deletions src/hooks/task-id-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>;
/** The caller observed a valid plan whose task-ID cardinality exceeded the bound. */
planContextOverLimit?: boolean;
Expand All @@ -45,7 +51,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',
Expand All @@ -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 (
Expand Down Expand Up @@ -177,7 +231,7 @@ export function resolveTaskId(
}

const explicit = new Set<string>();
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 };
Expand Down Expand Up @@ -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];
Expand All @@ -296,8 +353,11 @@ export function resolveTaskId(
}
if (hasInvalidRawMarker) return { status: 'invalid', input: 'marker' };
const marked = new Set<string>();
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];
Expand Down
Loading
Loading