mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-10-27 04:30:59 +00:00
fix: preserve foreground fork reports until launch completes (#1431)
A foreground fork can stream a copied `tool_result` for the Agent call that launched it. That placeholder completed the call and removed its tracked state, so the real report arriving later was dropped. Ignore only results whose tool ID equals the message's parent tool ID, keeping the launching call open for its actual report. Keep other results in the same message and discard the message-level `tool_use_result` sidecar when its launch result was removed, so it cannot be attributed to a remaining tool result. Regression coverage exercises plain and Air clients, root and nested launch results, and a mixed message containing a genuine nested Agent result. Fixes #1389. Validation: - [Linux CI](https://github.com/agentclientprotocol/claude-agent-acp/actions/runs/40217593917) passes the full suite: 3,304 passed, 32 skipped (57 test files passed, 4 skipped). All PR checks are green. - `npm run check` and `npm run build` pass. - `vitest run src/tests/acp-agent.test.ts --maxWorkers=1`: 712 passed, 10 skipped. The four new regression cases fail on the original code and pass with the fix. - Eight actual-SDK runs against localhost fake gateways complete successfully without model calls: plain and Air foreground forks, native Air child sessions, nested forks with the background option omitted or false, the SDK's fork-inside-fork rejection, a background launch receipt, and history replay. Foreground reports arrive exactly once and no launch-result tracking warning remains. The local full-suite run encountered 19 failures in `session-index.test.ts` and then stopped progressing, so it was interrupted. A standalone recursive-file-watcher probe also emits `EMFILE` on this machine. The successful Linux CI run above provides the complete-suite verification. Review dependencies (updated 26 October): | Related proposal | Relationship to this PR | | --- | --- | | [#1408](https://github.com/agentclientprotocol/claude-agent-acp/pull/1408) — targeted session lookup | Independent. Both orders merge cleanly onto the checked main and produce the same tree. | | [#1422](https://github.com/agentclientprotocol/claude-agent-acp/pull/1422) — resumed subagents | Complementary, with no dependency on this PR. It now conflicts with upstream main in `src/acp-agent.ts`; its author needs to resolve that conflict, integrate the [separately shared AIR routing patch](https://github.com/agentclientprotocol/claude-agent-acp/pull/1422#issuecomment-6401008862) and complete its validation. | <details> <summary>Additional local integration validation — 25 October</summary> A local combination of the recorded main, #1408, #1431, #1422 and the already-reviewed routing patch passed 199 focused foreground/resume/native-subagent checks (647 other cases filtered or skipped), TypeScript build, and two replays of previously captured SDK frames for AIR and standard subagents. The two captured-frame replays were independently repeated. No new model calls were made. The combination is a private test tree, not a published change. It does not replace full CI for #1422 or live Air acceptance. The routing patch needed three-way application against that recorded main because its surrounding source had moved; it applied cleanly that way. The current-main conflict in #1422 means this historical result does not establish today’s combined compatibility. Checked main `739dde3`, #1408 `e3b3aa4`, #1431 `8f96f46`, #1422 `dd3bf2c`. The two orders of #1408 and #1431 produce tree `1716d7e`; the full local test tree is `61c0447`. Refresh this evidence if these inputs change. </details> Co-authored-by: Codex <codex@openai.com>
This commit is contained in:
Slapoguzov Alexander
committed by
GitHub
GitHub
parent
c26e640915
commit
e369479f93
@@ -8200,6 +8200,7 @@ export class ClaudeAcpAgent {
|
||||
}
|
||||
|
||||
let content: typeof message.message.content;
|
||||
let toolUseResult = message.type === "user" ? message.tool_use_result : undefined;
|
||||
if (
|
||||
message.type === "assistant" &&
|
||||
(message.parent_tool_use_id === null || forwardsSubagentText())
|
||||
@@ -8220,6 +8221,20 @@ export class ClaudeAcpAgent {
|
||||
(item) => item.type !== "text" && item.type !== "thinking",
|
||||
);
|
||||
streamedBlocksOf(message.parent_tool_use_id).length = 0;
|
||||
} else if (
|
||||
message.parent_tool_use_id !== null &&
|
||||
Array.isArray(message.message.content)
|
||||
) {
|
||||
// A foreground fork inherits a placeholder result for its own
|
||||
// launch. Only the launching side can complete that Agent call
|
||||
// with the fork's actual report.
|
||||
const launchId = message.parent_tool_use_id;
|
||||
content = message.message.content.filter(
|
||||
(block) => block.type !== "tool_result" || block.tool_use_id !== launchId,
|
||||
);
|
||||
// The message-level sidecar cannot describe a remaining result
|
||||
// after its launch result was removed.
|
||||
if (content.length !== message.message.content.length) toolUseResult = undefined;
|
||||
} else {
|
||||
content = message.message.content;
|
||||
}
|
||||
@@ -8237,7 +8252,7 @@ export class ClaudeAcpAgent {
|
||||
);
|
||||
const backgroundBashTask = backgroundBashTaskFromToolResult(
|
||||
content,
|
||||
message.tool_use_result,
|
||||
toolUseResult,
|
||||
session.toolUseCache,
|
||||
);
|
||||
if (backgroundBashTask) await asyncTasks.taskBackgrounded(backgroundBashTask);
|
||||
@@ -8266,7 +8281,7 @@ export class ClaudeAcpAgent {
|
||||
asyncTasks,
|
||||
backgroundBashTask,
|
||||
);
|
||||
const resumedAgentId = resumedNativeSubagentId(message.tool_use_result);
|
||||
const resumedAgentId = resumedNativeSubagentId(toolUseResult);
|
||||
if (resumedAgentId) {
|
||||
const resume = sendMessageResume(
|
||||
session.toolUseCache,
|
||||
@@ -8307,7 +8322,7 @@ export class ClaudeAcpAgent {
|
||||
emittedToolCalls: session.emittedToolCalls,
|
||||
toolCallFields: toolCallFieldsOf(session),
|
||||
messageId: messageIdForGrouping(message),
|
||||
toolUseResult: message.type === "user" ? message.tool_use_result : undefined,
|
||||
toolUseResult,
|
||||
// On the wire since CLI 2.1.216 but not in SDKUserMessage's
|
||||
// type, hence the cast. Validated by parseToolResultMeta.
|
||||
toolResultMeta:
|
||||
|
||||
@@ -15964,6 +15964,132 @@ describe("usage_update computation", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("foreground fork tool results", () => {
|
||||
it.each([
|
||||
["plain", {}, null],
|
||||
["AIR", AIR_CLIENT_CAPABILITIES, null],
|
||||
["plain nested", {}, "toolu_outer"],
|
||||
["AIR nested", AIR_CLIENT_CAPABILITIES, "toolu_outer"],
|
||||
] as const)(
|
||||
"waits for the launching side's result for a %s client",
|
||||
async (_, capabilities, parent) => {
|
||||
const updates: SessionNotification[] = [];
|
||||
const logger = { log: vi.fn(), error: vi.fn() };
|
||||
const agent = new ClaudeAcpAgent(
|
||||
{
|
||||
sessionUpdate: async (notification: SessionNotification) => {
|
||||
updates.push(notification);
|
||||
},
|
||||
} as unknown as AcpClient,
|
||||
logger,
|
||||
);
|
||||
await initializeClient(agent, capabilities);
|
||||
const input = new Pushable<any>();
|
||||
const assistant = (content: unknown[], parentToolUseId: string | null) => ({
|
||||
type: "assistant",
|
||||
parent_tool_use_id: parentToolUseId,
|
||||
uuid: randomUUID(),
|
||||
session_id: "test-session",
|
||||
message: {
|
||||
id: randomUUID(),
|
||||
role: "assistant",
|
||||
model: "claude-sonnet-4-6",
|
||||
content,
|
||||
usage: successfulResultMessage().usage,
|
||||
},
|
||||
});
|
||||
const user = (content: unknown[], parentToolUseId: string | null) => ({
|
||||
type: "user",
|
||||
parent_tool_use_id: parentToolUseId,
|
||||
uuid: randomUUID(),
|
||||
session_id: "test-session",
|
||||
message: { role: "user", content },
|
||||
});
|
||||
const toolResult = (id: string, text: string) => ({
|
||||
type: "tool_result",
|
||||
tool_use_id: id,
|
||||
content: [{ type: "text", text }],
|
||||
});
|
||||
async function* messages() {
|
||||
const { value } = await input[Symbol.asyncIterator]().next();
|
||||
yield userEcho(value);
|
||||
yield assistant(
|
||||
[
|
||||
{
|
||||
type: "tool_use",
|
||||
id: "toolu_fork",
|
||||
name: "Agent",
|
||||
input: {
|
||||
subagent_type: "fork",
|
||||
run_in_background: false,
|
||||
prompt: "Inspect the parser",
|
||||
},
|
||||
},
|
||||
],
|
||||
parent,
|
||||
);
|
||||
// The fork inherits a result for its own launch. It cannot complete
|
||||
// that call: the launching side sends the actual report later.
|
||||
yield user([toolResult("toolu_fork", "Fork started — that fork is you.")], "toolu_fork");
|
||||
yield assistant(
|
||||
[
|
||||
{
|
||||
type: "tool_use",
|
||||
id: "toolu_child",
|
||||
name: "Agent",
|
||||
input: { subagent_type: "general-purpose", prompt: "Read the parser" },
|
||||
},
|
||||
],
|
||||
"toolu_fork",
|
||||
);
|
||||
// Keep results of tools the fork itself called, even in a message
|
||||
// that also carries the copied launch result.
|
||||
yield {
|
||||
...user(
|
||||
[
|
||||
toolResult("toolu_fork", "Fork started — processing in background"),
|
||||
toolResult("toolu_child", "child report"),
|
||||
],
|
||||
"toolu_fork",
|
||||
),
|
||||
// Filtering must not attribute the inherited launch sidecar to
|
||||
// the one result that remains in this message.
|
||||
tool_use_result: {
|
||||
status: "completed",
|
||||
content: [{ type: "text", text: "inherited launch report" }],
|
||||
},
|
||||
};
|
||||
yield user([toolResult("toolu_fork", "fork report")], parent);
|
||||
yield successfulResultMessage();
|
||||
yield { type: "system", subtype: "session_state_changed", state: "idle" };
|
||||
}
|
||||
agent.sessions["test-session"] = mockSessionState({ query: wrapQuery(messages()), input });
|
||||
|
||||
await expect(
|
||||
agent.prompt({ sessionId: "test-session", prompt: [{ type: "text", text: "Inspect it" }] }),
|
||||
).resolves.toMatchObject({ stopReason: "end_turn" });
|
||||
|
||||
const completed = updates.flatMap(({ update }) =>
|
||||
update.sessionUpdate === "tool_call_update" && update.status === "completed"
|
||||
? [{ id: update.toolCallId, content: update.content, rawOutput: update.rawOutput }]
|
||||
: [],
|
||||
);
|
||||
expect(completed.map(({ id }) => id)).toEqual(["toolu_child", "toolu_fork"]);
|
||||
expect(completed.map(({ content }) => content)).toEqual([
|
||||
[{ type: "content", content: { type: "text", text: "child report" } }],
|
||||
[{ type: "content", content: { type: "text", text: "fork report" } }],
|
||||
]);
|
||||
if (!("_meta" in capabilities)) {
|
||||
expect(completed.map(({ rawOutput }) => rawOutput)).toEqual([
|
||||
[{ type: "text", text: "child report" }],
|
||||
[{ type: "text", text: "fork report" }],
|
||||
]);
|
||||
}
|
||||
expect(logger.error.mock.calls.flat().join("\n")).not.toContain("wasn't tracked");
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe("assembled assistant text fallback", () => {
|
||||
const ZERO_USAGE = {
|
||||
input_tokens: 0,
|
||||
|
||||
Reference in New Issue
Block a user