Skip to content

Commit e452148

Browse files
TheGreatAxiosTheGreatAxios
committed
Wrap subagent span around full child wall
Start the exclusive subagent span before worktree setup and end it after teardown so attribution includes prepare and cleanup, including prepare failures that never reach run().
1 parent a28796d commit e452148

2 files changed

Lines changed: 162 additions & 125 deletions

File tree

‎src/perf/permission-subagent-spans.test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,4 +312,38 @@ describe("subagent spans", () => {
312312
expect(agents[0]!.tags?.subagent_id).toBe("call-fail");
313313
expect(agents[0]!.endNs).toBeDefined();
314314
});
315+
316+
test("opens and closes subagent span when worktree setup fails before run", async () => {
317+
let runEntered = false;
318+
const tool = createTaskTool({
319+
permissionGate: skipGate,
320+
// Not a git repo — createSubAgentWorktree fails before run.
321+
cwd: "/tmp/not-a-git-repo-for-subagent-span",
322+
getWorkdirBase: () => "/tmp/not-a-git-repo-for-subagent-span/.corbits",
323+
provider,
324+
useWorktree: true,
325+
run: async () => {
326+
runEntered = true;
327+
return "## Summary\n\nshould not run\n";
328+
},
329+
});
330+
if (tool.kind !== "full") throw new Error("expected full tool");
331+
332+
const result = await tool.handler(
333+
{
334+
id: "call-wt-fail",
335+
name: "task",
336+
arguments: { description: "Worktree fail", prompt: "Work" },
337+
},
338+
new AbortController().signal,
339+
);
340+
expect(runEntered).toBe(false);
341+
expect(typeof result.content === "string" ? result.content : "").toContain("Error:");
342+
343+
const agents = byName(completed(snapshot()), "subagent");
344+
expect(agents).toHaveLength(1);
345+
expect(agents[0]!.tags?.subagent_id).toBe("call-wt-fail");
346+
expect(agents[0]!.endNs).toBeDefined();
347+
});
315348
});
349+

‎src/subagent/task-tool.ts‎

Lines changed: 128 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -494,144 +494,147 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool {
494494
let worktreeCwd: string | undefined;
495495
let worktreeStashBaseline: readonly string[] | null = [];
496496
let worktreeHeadAtCreate: string | undefined;
497-
if (deps.useWorktree === true) {
498-
const worktreePath = join(deps.getWorkdirBase(), "worktrees", generateSessionId());
499-
try {
500-
const worktree = await createSubAgentWorktree(deps.cwd, worktreePath);
501-
worktreeCwd = worktree.path;
502-
worktreeStashBaseline = worktree.stashBaseline;
503-
worktreeHeadAtCreate = worktree.headAtCreate;
504-
} catch (err) {
505-
// Admit already happened and the strip session may be "running" —
506-
// release the ledger slot and fail the session so a worktree setup
507-
// error never burns turn-budget budget or leaves a ghost row.
508-
const message =
509-
err instanceof WorktreeError
510-
? err.message
511-
: `sub-agent worktree setup failed: ${err instanceof Error ? err.message : String(err)}`;
512-
briefLedger.release(fingerprint);
513-
if (session !== undefined) deps.sessions?.fail(session.id, message);
514-
signal.removeEventListener("abort", onParentAbort);
515-
return taskToolResult(call.id, `Error: ${message}`);
516-
}
517-
}
518-
// Cleanup runs once the sub-agent's report is ready, regardless of
519-
// outcome, so a cancelled or failed run's worktree is still reclaimed
520-
// (or preserved with a notice) rather than leaked.
521-
const finishWithWorktree = async (result: ToolResult): Promise<ToolResult> => {
522-
if (worktreeCwd === undefined) return result;
523-
const cleanup = await cleanupSubAgentWorktree(deps.cwd, worktreeCwd, {
524-
stashBaseline: worktreeStashBaseline,
525-
...(worktreeHeadAtCreate !== undefined ? { headAtCreate: worktreeHeadAtCreate } : {}),
526-
});
527-
if (cleanup.status === "preserved") {
528-
return { ...result, content: `${result.content}\n\n${cleanup.notice}` };
529-
}
530-
return result;
531-
};
532-
497+
// Full child wall: worktree setup → run → teardown (CL-5170 exclusive share).
498+
const turnId = currentTurnId();
499+
const subagentSpanId = start("subagent", {
500+
...(turnId !== null && turnId.length > 0 ? { parentId: turnId } : {}),
501+
tags: {
502+
subagent_id: call.id,
503+
...(turnId !== null && turnId.length > 0 ? { turn_id: turnId } : {}),
504+
},
505+
});
533506
try {
534-
const params: RunSubAgentParams = {
535-
...sandbox,
536-
cwd: worktreeCwd ?? deps.cwd,
537-
workdirBase: deps.getWorkdirBase(),
538-
provider,
539-
...(tier !== undefined ? { tier } : {}),
540-
...(settings !== undefined ? { settings } : {}),
541-
...(catalog !== undefined ? { catalog } : {}),
542-
description,
543-
...(context !== undefined && context.length > 0 ? { context } : {}),
544-
prompt,
545-
...(goals.length > 0 ? { goals } : {}),
546-
...(intent !== undefined ? { intent } : {}),
547-
...(successCriteria.length > 0 ? { successCriteria } : {}),
548-
...(doNot.length > 0 ? { doNot } : {}),
549-
...(reportFocus !== undefined && reportFocus.length > 0 ? { reportFocus } : {}),
550-
signal: childCtl.signal,
551-
...(recordEvent !== undefined ? { onEvent: recordEvent } : {}),
552-
...(deps.onProgress !== undefined ? { onProgress: deps.onProgress } : {}),
553-
...(capabilities !== undefined ? { capabilities } : {}),
554-
...(systemPromptRole !== undefined ? { systemPromptRole } : {}),
555-
...(orchestrator
556-
? { orchestrator: true, nestedDispatch: nestedDispatch! }
557-
: {}),
558-
// Nested workers (installed by an orchestrator that already holds a
559-
// slot) reuse the parent slot rather than acquiring their own.
560-
...(deps.allowOrchestrator === false ? { nested: true } : {}),
561-
maxTurns: resolvedMaxTurns,
562-
...(deps.deadlineMs !== undefined ? { deadlineMs: deps.deadlineMs } : {}),
563-
};
564-
const turnId = currentTurnId();
565-
const subagentSpanId = start("subagent", {
566-
...(turnId !== null && turnId.length > 0 ? { parentId: turnId } : {}),
567-
tags: {
568-
subagent_id: call.id,
569-
...(turnId !== null && turnId.length > 0 ? { turn_id: turnId } : {}),
570-
},
571-
});
572-
const result = await run(params).finally(() => {
573-
end(subagentSpanId);
574-
});
575-
// Operator cancel may race after run resolves. Keep strip status cancelled
576-
// when requested, but never discard a returned body (including salvage).
577-
const wasCancelled =
578-
childCtl.signal.aborted ||
579-
(session !== undefined &&
580-
deps.sessions?.get(session.id)?.status === "cancelled");
581-
const salvage = classifyBriefSalvage(result);
582-
briefLedger.recordOutcome(fingerprint, salvage);
583-
const hintOptions = {
584-
dispatchCount,
585-
turnBudgetStopAfterDispatches: TURN_BUDGET_STOP_AFTER_DISPATCHES,
507+
if (deps.useWorktree === true) {
508+
const worktreePath = join(deps.getWorkdirBase(), "worktrees", generateSessionId());
509+
try {
510+
const worktree = await createSubAgentWorktree(deps.cwd, worktreePath);
511+
worktreeCwd = worktree.path;
512+
worktreeStashBaseline = worktree.stashBaseline;
513+
worktreeHeadAtCreate = worktree.headAtCreate;
514+
} catch (err) {
515+
// Admit already happened and the strip session may be "running" —
516+
// release the ledger slot and fail the session so a worktree setup
517+
// error never burns turn-budget budget or leaves a ghost row.
518+
const message =
519+
err instanceof WorktreeError
520+
? err.message
521+
: `sub-agent worktree setup failed: ${err instanceof Error ? err.message : String(err)}`;
522+
briefLedger.release(fingerprint);
523+
if (session !== undefined) deps.sessions?.fail(session.id, message);
524+
signal.removeEventListener("abort", onParentAbort);
525+
return taskToolResult(call.id, `Error: ${message}`);
526+
}
527+
}
528+
// Cleanup runs once the sub-agent's report is ready, regardless of
529+
// outcome, so a cancelled or failed run's worktree is still reclaimed
530+
// (or preserved with a notice) rather than leaked.
531+
const finishWithWorktree = async (result: ToolResult): Promise<ToolResult> => {
532+
if (worktreeCwd === undefined) return result;
533+
const cleanup = await cleanupSubAgentWorktree(deps.cwd, worktreeCwd, {
534+
stashBaseline: worktreeStashBaseline,
535+
...(worktreeHeadAtCreate !== undefined ? { headAtCreate: worktreeHeadAtCreate } : {}),
536+
});
537+
if (cleanup.status === "preserved") {
538+
return { ...result, content: `${result.content}\n\n${cleanup.notice}` };
539+
}
540+
return result;
586541
};
587-
if (wasCancelled) {
588-
if (
589-
session !== undefined &&
590-
deps.sessions?.get(session.id)?.status === "running"
591-
) {
592-
deps.sessions.cancel(session.id, cancelReason(childCtl.signal));
542+
543+
try {
544+
const params: RunSubAgentParams = {
545+
...sandbox,
546+
cwd: worktreeCwd ?? deps.cwd,
547+
workdirBase: deps.getWorkdirBase(),
548+
provider,
549+
...(tier !== undefined ? { tier } : {}),
550+
...(settings !== undefined ? { settings } : {}),
551+
...(catalog !== undefined ? { catalog } : {}),
552+
description,
553+
...(context !== undefined && context.length > 0 ? { context } : {}),
554+
prompt,
555+
...(goals.length > 0 ? { goals } : {}),
556+
...(intent !== undefined ? { intent } : {}),
557+
...(successCriteria.length > 0 ? { successCriteria } : {}),
558+
...(doNot.length > 0 ? { doNot } : {}),
559+
...(reportFocus !== undefined && reportFocus.length > 0 ? { reportFocus } : {}),
560+
signal: childCtl.signal,
561+
...(recordEvent !== undefined ? { onEvent: recordEvent } : {}),
562+
...(deps.onProgress !== undefined ? { onProgress: deps.onProgress } : {}),
563+
...(capabilities !== undefined ? { capabilities } : {}),
564+
...(systemPromptRole !== undefined ? { systemPromptRole } : {}),
565+
...(orchestrator
566+
? { orchestrator: true, nestedDispatch: nestedDispatch! }
567+
: {}),
568+
// Nested workers (installed by an orchestrator that already holds a
569+
// slot) reuse the parent slot rather than acquiring their own.
570+
...(deps.allowOrchestrator === false ? { nested: true } : {}),
571+
maxTurns: resolvedMaxTurns,
572+
...(deps.deadlineMs !== undefined ? { deadlineMs: deps.deadlineMs } : {}),
573+
};
574+
const result = await run(params);
575+
// Operator cancel may race after run resolves. Keep strip status cancelled
576+
// when requested, but never discard a returned body (including salvage).
577+
const wasCancelled =
578+
childCtl.signal.aborted ||
579+
(session !== undefined &&
580+
deps.sessions?.get(session.id)?.status === "cancelled");
581+
const salvage = classifyBriefSalvage(result);
582+
briefLedger.recordOutcome(fingerprint, salvage);
583+
const hintOptions = {
584+
dispatchCount,
585+
turnBudgetStopAfterDispatches: TURN_BUDGET_STOP_AFTER_DISPATCHES,
586+
};
587+
if (wasCancelled) {
588+
if (
589+
session !== undefined &&
590+
deps.sessions?.get(session.id)?.status === "running"
591+
) {
592+
deps.sessions.cancel(session.id, cancelReason(childCtl.signal));
593+
}
594+
const reported = appendSubAgentParentHints(result, hintOptions);
595+
return finishWithWorktree(
596+
taskToolResult(call.id, `Sub-agent "${description}" reported:\n\n${reported}`),
597+
);
593598
}
599+
if (session !== undefined) deps.sessions?.complete(session.id, result);
594600
const reported = appendSubAgentParentHints(result, hintOptions);
595601
return finishWithWorktree(
596602
taskToolResult(call.id, `Sub-agent "${description}" reported:\n\n${reported}`),
597603
);
598-
}
599-
if (session !== undefined) deps.sessions?.complete(session.id, result);
600-
const reported = appendSubAgentParentHints(result, hintOptions);
601-
return finishWithWorktree(
602-
taskToolResult(call.id, `Sub-agent "${description}" reported:\n\n${reported}`),
603-
);
604604

605-
} catch (err) {
606-
if (
607-
isSubAgentCancelError(err, childCtl.signal) ||
608-
(session !== undefined &&
609-
deps.sessions?.get(session.id)?.status === "cancelled")
610-
) {
611-
briefLedger.recordOutcome(fingerprint, "cancelled");
605+
} catch (err) {
612606
if (
613-
session !== undefined &&
614-
deps.sessions?.get(session.id)?.status === "running"
607+
isSubAgentCancelError(err, childCtl.signal) ||
608+
(session !== undefined &&
609+
deps.sessions?.get(session.id)?.status === "cancelled")
615610
) {
616-
deps.sessions.cancel(session.id, cancelReason(childCtl.signal));
611+
briefLedger.recordOutcome(fingerprint, "cancelled");
612+
if (
613+
session !== undefined &&
614+
deps.sessions?.get(session.id)?.status === "running"
615+
) {
616+
deps.sessions.cancel(session.id, cancelReason(childCtl.signal));
617+
}
618+
return finishWithWorktree(taskToolResult(call.id, cancelledSubAgentMessage(description)));
617619
}
618-
return finishWithWorktree(taskToolResult(call.id, cancelledSubAgentMessage(description)));
620+
// Run never produced a body — undo the admit so turn-budget retry budget
621+
// is not burned by auth/provider crashes.
622+
briefLedger.release(fingerprint);
623+
const authMessage = formatSubAgentTaskAuthFailureMessage(description, err);
624+
const message =
625+
authMessage !== null
626+
? `Error: ${authMessage}`
627+
: `Error: sub-agent "${description}" failed: ${err instanceof Error ? err.message : String(err)}`;
628+
const sessionError = err instanceof Error ? err.message : String(err);
629+
// fail() prefixes "Error:" on the transcript report entry — pass bare text.
630+
const failReason = authMessage ?? sessionError;
631+
if (session !== undefined) deps.sessions?.fail(session.id, failReason);
632+
return finishWithWorktree(taskToolResult(call.id, message));
633+
} finally {
634+
signal.removeEventListener("abort", onParentAbort);
619635
}
620-
// Run never produced a body — undo the admit so turn-budget retry budget
621-
// is not burned by auth/provider crashes.
622-
briefLedger.release(fingerprint);
623-
const authMessage = formatSubAgentTaskAuthFailureMessage(description, err);
624-
const message =
625-
authMessage !== null
626-
? `Error: ${authMessage}`
627-
: `Error: sub-agent "${description}" failed: ${err instanceof Error ? err.message : String(err)}`;
628-
const sessionError = err instanceof Error ? err.message : String(err);
629-
// fail() prefixes "Error:" on the transcript report entry — pass bare text.
630-
const failReason = authMessage ?? sessionError;
631-
if (session !== undefined) deps.sessions?.fail(session.id, failReason);
632-
return finishWithWorktree(taskToolResult(call.id, message));
633636
} finally {
634-
signal.removeEventListener("abort", onParentAbort);
637+
end(subagentSpanId);
635638
}
636639
},
637640
});

0 commit comments

Comments
 (0)