From efc3a52947e9f0909569bf23dc9c5d1544f6aec3 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Mon, 27 Apr 2026 09:42:24 -0700 Subject: [PATCH] fix(sessions_spawn): tolerate ACP-only fields for subagent runtime Preserve contributor credit and land the narrowed sessions_spawn ACP-field handling with follow-up transcript redaction and ACP resume ownership hardening. Targeted Blacksmith validation passed for the touched sessions/ACP tests. --- CHANGELOG.md | 2 +- docs/tools/acp-agents.md | 4 +- src/agents/acp-spawn.test.ts | 88 +++++++++++++++++++ src/agents/acp-spawn.ts | 80 +++++++++++++++++ ...sion-transcript-repair.attachments.test.ts | 79 +++++++++++++++++ src/agents/session-transcript-repair.ts | 30 ++++++- src/agents/tools/sessions-spawn-tool.test.ts | 30 +++++-- src/agents/tools/sessions-spawn-tool.ts | 4 +- 8 files changed, 302 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f505cf86a4..641405b1629 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -240,7 +240,7 @@ Docs: https://docs.openclaw.ai - Diagnostics/OTEL: capture privacy-safe model-call request payload bytes, streamed response bytes, first-response latency, and total duration in diagnostic events, plugin hooks, stability snapshots, and OTEL model-call spans/metrics without logging raw model content. Fixes #33832. Thanks @wwh830. - Logging: write validated diagnostic trace context as top-level `traceId`, `spanId`, `parentSpanId`, and `traceFlags` fields in file-log JSONL records so traced requests and model calls are easier to correlate in log processors. Refs #40353. Thanks @liangruochong44-ui. - Logging/sessions: apply configured redaction patterns to persisted session transcript text and accept escaped character classes in safe custom redaction regexes, so transcript JSONL no longer keeps matching sensitive text in the clear. Fixes #42982. Thanks @panpan0000. -- Agents/sessions: let `sessions_spawn runtime="subagent"` ignore ACP-only `streamTo` and `resumeSessionId` fields while keeping ACP passthrough and documenting `streamTo` as ACP-only. Fixes #43556 and #63120; covers #56326, #61724, #64714, and #67248. Thanks @skernelx, @damselem, @Br1an67, @Mintalix, @IsaacAPerez, @vvitovec, and @Sanjays2402. +- Agents/sessions: let `sessions_spawn runtime="subagent"` ignore ACP-only `streamTo` and `resumeSessionId` fields while keeping ACP passthrough and documenting `streamTo` as ACP-only. Fixes #43556 and #63120; covers #56326, #61724, #64714, and #67248; carries forward #68397, #65282, #58686, #56342, and #40102. Thanks @skernelx, @damselem, @Br1an67, @Mintalix, @IsaacAPerez, @vvitovec, @Sanjays2402, @shenkq97, and @1034378361. - Providers/Ollama: honor `/api/show` capabilities when registering local models so non-tool Ollama models no longer receive the agent tool surface, and keep native Ollama thinking opt-in instead of enabling it by default. Fixes #64710 and duplicate #65343. Thanks @yuan-b, @netherby, @xilopaint, and @Diyforfun2026. - Image tool/media: honor `tools.media.image.timeoutSeconds` and matching per-model image timeouts in explicit image analysis, including the MiniMax VLM fallback path, so slow local vision models are not capped by hardcoded 30s/60s aborts. Fixes #67889; supersedes #67929. Thanks @AllenT22 and @alchip. - Providers/Ollama: read larger custom Modelfile `PARAMETER num_ctx` values from `/api/show` so auto-discovered Ollama models with expanded context no longer stay pinned to the base model context. Fixes #68344. Thanks @neeravmakwana. diff --git a/docs/tools/acp-agents.md b/docs/tools/acp-agents.md index fce4f87641e..e2793298378 100644 --- a/docs/tools/acp-agents.md +++ b/docs/tools/acp-agents.md @@ -676,7 +676,9 @@ background work. The delivery path depends on that shape. Notes: - - `resumeSessionId` requires `runtime: "acp"` — returns an error if used with the sub-agent runtime. + - `resumeSessionId` only applies when `runtime: "acp"`; the default sub-agent runtime ignores this ACP-only field. + - `streamTo` only applies when `runtime: "acp"`; the default sub-agent runtime ignores this ACP-only field. + - `resumeSessionId` is a host-local ACP/harness resume id, not an OpenClaw channel session key; OpenClaw still checks ACP spawn policy and target agent policy before dispatch, while the ACP backend or harness owns authorization for loading that upstream id. - `resumeSessionId` restores the upstream ACP conversation history; `thread` and `mode` still apply normally to the new OpenClaw session you are creating, so `mode: "session"` still requires `thread: true`. - The target agent must support `session/load` (Codex and Claude Code do). - If the session id is not found, the spawn fails with a clear error — no silent fallback to a new session. diff --git a/src/agents/acp-spawn.test.ts b/src/agents/acp-spawn.test.ts index d581fcf8a9a..2e2986fd8b2 100644 --- a/src/agents/acp-spawn.test.ts +++ b/src/agents/acp-spawn.test.ts @@ -3,6 +3,7 @@ import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { AcpInitializeSessionInput } from "../acp/control-plane/manager.types.js"; +import type { SessionEntry } from "../config/sessions/types.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { __testing as sessionBindingServiceTesting, @@ -726,6 +727,93 @@ describe("spawnAcpDirect", () => { expect(transcriptCalls[1]?.threadId).toBe("child-thread"); }); + it("allows ACP resume IDs recorded for the requester session", async () => { + const resumeSessionId = "codex-inner-resume"; + hoisted.loadSessionStoreMock.mockReturnValue({ + "agent:codex:acp:owned": { + sessionId: "sess-owned", + updatedAt: Date.now(), + spawnedBy: "agent:main:main", + acp: { + backend: "acpx", + agent: "codex", + runtimeSessionName: "codex", + identity: { + state: "resolved", + source: "ensure", + agentSessionId: resumeSessionId, + acpxSessionId: "acpx-owned", + lastUpdatedAt: Date.now(), + }, + mode: "oneshot", + state: "idle", + lastActivityAt: Date.now(), + }, + } satisfies SessionEntry, + }); + + const result = await spawnAcpDirect( + { + task: "Resume owned ACP session", + agentId: "codex", + resumeSessionId, + }, + { + agentSessionKey: "agent:main:main", + }, + ); + + expectAcceptedSpawn(result); + expect(hoisted.initializeSessionMock).toHaveBeenCalledWith( + expect.objectContaining({ + resumeSessionId, + }), + ); + }); + + it("rejects ACP resume IDs not recorded for the requester session", async () => { + hoisted.loadSessionStoreMock.mockReturnValue({ + "agent:codex:acp:other": { + sessionId: "sess-other", + updatedAt: Date.now(), + spawnedBy: "agent:other:main", + acp: { + backend: "acpx", + agent: "codex", + runtimeSessionName: "codex", + identity: { + state: "resolved", + source: "ensure", + agentSessionId: "codex-inner-other", + acpxSessionId: "acpx-other", + lastUpdatedAt: Date.now(), + }, + mode: "oneshot", + state: "idle", + lastActivityAt: Date.now(), + }, + } satisfies SessionEntry, + }); + + const result = await spawnAcpDirect( + { + task: "Resume other ACP session", + agentId: "codex", + resumeSessionId: "codex-inner-other", + }, + { + agentSessionKey: "agent:main:main", + }, + ); + + expect(result).toMatchObject({ + status: "forbidden", + errorCode: "resume_forbidden", + }); + expect(hoisted.initializeSessionMock).not.toHaveBeenCalled(); + expect(hoisted.callGatewayMock).not.toHaveBeenCalled(); + }); + it("passes model and thinking overrides into ACP session initialization", async () => { const result = await spawnAcpDirect( { diff --git a/src/agents/acp-spawn.ts b/src/agents/acp-spawn.ts index b58e18786e6..266ea458b46 100644 --- a/src/agents/acp-spawn.ts +++ b/src/agents/acp-spawn.ts @@ -128,6 +128,7 @@ export const ACP_SPAWN_ERROR_CODES = [ "acp_disabled", "requester_session_required", "runtime_policy", + "resume_forbidden", "subagent_policy", "thread_required", "target_agent_required", @@ -843,6 +844,72 @@ function resolveAcpSpawnStreamPlan(params: { }; } +function sessionEntryMatchesAcpResumeSessionId( + entry: SessionEntry | undefined, + resumeSessionId: string, +): boolean { + const identity = entry?.acp?.identity; + return ( + normalizeOptionalString(identity?.agentSessionId) === resumeSessionId || + normalizeOptionalString(identity?.acpxSessionId) === resumeSessionId + ); +} + +function sessionEntryIsOwnedByRequester(params: { + sessionKey: string; + entry: SessionEntry | undefined; + requesterSessionKey: string; +}): boolean { + return ( + params.sessionKey === params.requesterSessionKey || + normalizeOptionalString(params.entry?.spawnedBy) === params.requesterSessionKey || + normalizeOptionalString(params.entry?.parentSessionKey) === params.requesterSessionKey + ); +} + +function validateAcpResumeSessionOwnership(params: { + cfg: OpenClawConfig; + targetAgentId: string; + requesterSessionKey?: string; + resumeSessionId?: string; +}): { ok: true } | { ok: false; error: string } { + const resumeSessionId = normalizeOptionalString(params.resumeSessionId); + if (!resumeSessionId) { + return { ok: true }; + } + const requesterSessionKey = normalizeOptionalString(params.requesterSessionKey); + if (!requesterSessionKey) { + return { + ok: false, + error: "sessions_spawn resumeSessionId requires an active requester session context.", + }; + } + + const storePath = resolveStorePath(params.cfg.session?.store, { agentId: params.targetAgentId }); + const sessionStore = loadSessionStore(storePath); + for (const [sessionKey, entry] of Object.entries(sessionStore)) { + if (!sessionEntryMatchesAcpResumeSessionId(entry, resumeSessionId)) { + continue; + } + if ( + sessionEntryIsOwnedByRequester({ + sessionKey, + entry, + requesterSessionKey, + }) + ) { + return { ok: true }; + } + break; + } + + return { + ok: false, + error: + "sessions_spawn resumeSessionId is only allowed for ACP sessions previously recorded for this requester. Omit resumeSessionId to start a fresh ACP session.", + }; +} + async function initializeAcpSpawnRuntime(params: { cfg: OpenClawConfig; sessionKey: string; @@ -1156,6 +1223,19 @@ export async function spawnAcpDirect( error: subagentEnvelopeState.error, }); } + const resumeAuthorization = validateAcpResumeSessionOwnership({ + cfg, + targetAgentId, + requesterSessionKey: requesterInternalKey, + resumeSessionId: params.resumeSessionId, + }); + if (!resumeAuthorization.ok) { + return createAcpSpawnFailure({ + status: "forbidden", + errorCode: "resume_forbidden", + error: resumeAuthorization.error, + }); + } const { effectiveStreamToParent } = resolveAcpSpawnStreamPlan({ spawnMode, requestThreadBinding, diff --git a/src/agents/session-transcript-repair.attachments.test.ts b/src/agents/session-transcript-repair.attachments.test.ts index 7f823a8cee7..7428325378e 100644 --- a/src/agents/session-transcript-repair.attachments.test.ts +++ b/src/agents/session-transcript-repair.attachments.test.ts @@ -119,4 +119,83 @@ describe("sanitizeToolCallInputs redacts sessions_spawn attachments", () => { }); expect(JSON.stringify(out)).not.toContain(secret); }); + + it("redacts ACP-only routing fields from arguments and input payloads", () => { + const argumentResumeSessionId = "ACP_ARGUMENT_SESSION_ID_SHOULD_NOT_PERSIST"; // pragma: allowlist secret + const inputResumeSessionId = "ACP_INPUT_SESSION_ID_SHOULD_NOT_PERSIST"; // pragma: allowlist secret + const input = castAgentMessages([ + { + role: "assistant", + content: [ + { + type: "toolCall", + id: "call_4", + name: "sessions_spawn", + arguments: { + task: "do thing", + resumeSessionId: argumentResumeSessionId, + streamTo: "parent", + }, + }, + { + type: "toolUse", + id: "call_5", + name: "sessions_spawn", + input: { + task: "do other thing", + resumeSessionId: inputResumeSessionId, + streamTo: "parent", + }, + }, + ], + }, + ]); + + const out = sanitizeToolCallInputs(input); + const msg = out[0] as { content?: unknown[] }; + const argumentTool = (msg.content?.[0] ?? null) as { + arguments?: { resumeSessionId?: string; streamTo?: string }; + } | null; + const inputTool = (msg.content?.[1] ?? null) as { + input?: { resumeSessionId?: string; streamTo?: string }; + } | null; + + expect(argumentTool?.arguments?.resumeSessionId).toBe("__OPENCLAW_REDACTED__"); + expect(argumentTool?.arguments?.streamTo).toBe("__OPENCLAW_REDACTED__"); + expect(inputTool?.input?.resumeSessionId).toBe("__OPENCLAW_REDACTED__"); + expect(inputTool?.input?.streamTo).toBe("__OPENCLAW_REDACTED__"); + expect(JSON.stringify(out)).not.toContain(argumentResumeSessionId); + expect(JSON.stringify(out)).not.toContain(inputResumeSessionId); + }); + + it("redacts ACP-only routing fields with non-string payloads", () => { + const nestedResumeSessionId = "ACP_NESTED_SESSION_ID_SHOULD_NOT_PERSIST"; // pragma: allowlist secret + const input = castAgentMessages([ + { + role: "assistant", + content: [ + { + type: "toolUse", + id: "call_6", + name: "sessions_spawn", + input: { + task: "do nested thing", + resumeSessionId: { value: nestedResumeSessionId }, + streamTo: ["parent"], + }, + }, + ], + }, + ]); + + const out = sanitizeToolCallInputs(input); + const msg = out[0] as { content?: unknown[] }; + const tool = (msg.content?.[0] ?? null) as { + input?: { resumeSessionId?: string; streamTo?: string }; + } | null; + + expect(tool?.input?.resumeSessionId).toBe("__OPENCLAW_REDACTED__"); + expect(tool?.input?.streamTo).toBe("__OPENCLAW_REDACTED__"); + expect(JSON.stringify(out)).not.toContain(nestedResumeSessionId); + }); }); diff --git a/src/agents/session-transcript-repair.ts b/src/agents/session-transcript-repair.ts index 5e651603e80..4532f86f073 100644 --- a/src/agents/session-transcript-repair.ts +++ b/src/agents/session-transcript-repair.ts @@ -80,6 +80,28 @@ function redactSessionsSpawnAttachmentsArgs(value: unknown): unknown { return { ...rec, attachments: next }; } +function redactSessionsSpawnAcpArgs(value: unknown): unknown { + if (!value || typeof value !== "object") { + return value; + } + const rec = value as Record; + const next = { ...rec }; + let changed = false; + + for (const key of ["resumeSessionId", "streamTo"] as const) { + if (Object.hasOwn(rec, key)) { + next[key] = REDACTED_SESSIONS_SPAWN_ATTACHMENT_CONTENT; + changed = true; + } + } + + return changed ? next : value; +} + +function redactSessionsSpawnArgs(value: unknown): unknown { + return redactSessionsSpawnAcpArgs(redactSessionsSpawnAttachmentsArgs(value)); +} + function redactSessionsSpawnAttachment(item: unknown): Record { const next: Record = { content: REDACTED_SESSIONS_SPAWN_ATTACHMENT_CONTENT, @@ -113,10 +135,10 @@ function sanitizeToolCallBlock(block: RawToolCallBlock): RawToolCallBlock { return { ...(block as Record), name: normalizedName } as RawToolCallBlock; } - // Redact large/sensitive inline attachment content from persisted transcripts. - // Apply redaction to both `.arguments` and `.input` properties since block structures can vary - const nextArgs = redactSessionsSpawnAttachmentsArgs(block.arguments); - const nextInput = redactSessionsSpawnAttachmentsArgs(block.input); + // Redact sensitive sessions_spawn payload fields from persisted transcripts. + // Apply redaction to both `.arguments` and `.input` properties since block structures can vary. + const nextArgs = redactSessionsSpawnArgs(block.arguments); + const nextInput = redactSessionsSpawnArgs(block.input); if (nextArgs === block.arguments && nextInput === block.input && !nameChanged) { return block; } diff --git a/src/agents/tools/sessions-spawn-tool.test.ts b/src/agents/tools/sessions-spawn-tool.test.ts index 0a28d82fd90..4e4f1ef8ada 100644 --- a/src/agents/tools/sessions-spawn-tool.test.ts +++ b/src/agents/tools/sessions-spawn-tool.test.ts @@ -73,7 +73,7 @@ describe("sessions_spawn tool", () => { const schema = tool.parameters as { properties?: { runtime?: { enum?: string[] }; - resumeSessionId?: unknown; + resumeSessionId?: { description?: string }; streamTo?: { description?: string }; }; }; @@ -93,7 +93,7 @@ describe("sessions_spawn tool", () => { const schema = tool.parameters as { properties?: { runtime?: { enum?: string[] }; - resumeSessionId?: unknown; + resumeSessionId?: { description?: string }; streamTo?: { description?: string }; }; }; @@ -103,7 +103,15 @@ describe("sessions_spawn tool", () => { expect(schema.properties?.runtime?.enum).toEqual(["subagent", "acp"]); expect(schema.properties?.resumeSessionId).toBeDefined(); expect(schema.properties?.streamTo).toBeDefined(); - expect(schema.properties?.streamTo?.description).toContain('Requires runtime="acp"'); + expect(schema.properties?.resumeSessionId?.description).toContain("ACP-only resume target"); + expect(schema.properties?.resumeSessionId?.description).toContain( + 'ignored for runtime="subagent"', + ); + expect(schema.properties?.resumeSessionId?.description).toContain( + "already recorded for this requester", + ); + expect(schema.properties?.streamTo?.description).toContain("ACP-only stream target"); + expect(schema.properties?.streamTo?.description).toContain('ignored for runtime="subagent"'); }); it("hides ACP runtime affordances when the ACP backend is unhealthy", () => { @@ -507,14 +515,16 @@ describe("sessions_spawn tool", () => { ); }); - it("ignores resumeSessionId without runtime=acp", async () => { + it("ignores ACP-only fields for subagent spawns", async () => { const tool = createSessionsSpawnTool({ agentSessionKey: "agent:main:main", }); const result = await tool.execute("call-guard", { + runtime: "subagent", task: "resume prior work", resumeSessionId: "7f4a78e0-f6be-43fe-855c-c1c4fd229bc4", + streamTo: "parent", }); expect(result.details).toMatchObject({ @@ -526,11 +536,14 @@ describe("sessions_spawn tool", () => { expect.objectContaining({ task: "resume prior work", }), - expect.any(Object), + expect.objectContaining({ + agentSessionKey: "agent:main:main", + }), ); expect(hoisted.spawnSubagentDirectMock.mock.calls[0]?.[0]).not.toHaveProperty( "resumeSessionId", ); + expect(hoisted.spawnSubagentDirectMock.mock.calls[0]?.[0]).not.toHaveProperty("streamTo"); expect(hoisted.spawnAcpDirectMock).not.toHaveBeenCalled(); }); @@ -559,14 +572,14 @@ describe("sessions_spawn tool", () => { expect(hoisted.spawnSubagentDirectMock).not.toHaveBeenCalled(); }); - it('ignores streamTo when runtime is not "acp"', async () => { + it('ignores streamTo when runtime is omitted and defaults to "subagent"', async () => { const tool = createSessionsSpawnTool({ agentSessionKey: "agent:main:main", }); const result = await tool.execute("call-3b", { - runtime: "subagent", task: "analyze file", + resumeSessionId: "7f4a78e0-f6be-43fe-855c-c1c4fd229bc4", streamTo: "parent", }); @@ -582,6 +595,9 @@ describe("sessions_spawn tool", () => { }), expect.any(Object), ); + expect(hoisted.spawnSubagentDirectMock.mock.calls[0]?.[0]).not.toHaveProperty( + "resumeSessionId", + ); expect(hoisted.spawnSubagentDirectMock.mock.calls[0]?.[0]).not.toHaveProperty("streamTo"); }); diff --git a/src/agents/tools/sessions-spawn-tool.ts b/src/agents/tools/sessions-spawn-tool.ts index 842d3343e30..ce2636e3b9f 100644 --- a/src/agents/tools/sessions-spawn-tool.ts +++ b/src/agents/tools/sessions-spawn-tool.ts @@ -154,12 +154,12 @@ function createSessionsSpawnToolSchema(params: { acpAvailable: boolean }) { resumeSessionId: Type.Optional( Type.String({ description: - 'Resume an existing agent session by its ID (e.g. a Codex session UUID from ~/.codex/sessions/). Requires runtime="acp". The agent replays conversation history via session/load instead of starting fresh.', + 'ACP-only resume target. Only meaningful with runtime="acp"; ignored for runtime="subagent". Use only an ACP/harness session ID already recorded for this requester so the ACP backend replays conversation history instead of starting fresh.', }), ), streamTo: optionalStringEnum(SESSIONS_SPAWN_ACP_STREAM_TARGETS, { description: - 'Stream ACP run output to the parent session. Requires runtime="acp"; omit for runtime="subagent".', + 'ACP-only stream target. Only meaningful with runtime="acp"; ignored for runtime="subagent". Use "parent" to stream the ACP turn back to the requester instead of tracking it as a background sessions_spawn run.', }), } : {}),