From 7c12cd101e0f4d3e77eec407e9b27f96b9bee38d Mon Sep 17 00:00:00 2001 From: aiken884 <191950007+aiken884@users.noreply.github.com> Date: Mon, 20 Jul 2026 18:22:33 +0800 Subject: [PATCH] fix(acp): resolve child/subagent session to registered root before permission gating Child sessions spawned server-side by the `task` tool were never registered in the ACP session store (only session/new|load|resume|fork register one). When such a session raised a permission.asked event, acp/permission.ts's Handler.process() called session.tryGet() on the unregistered child id, got undefined, and silently returned -- leaving the underlying Permission.ask Deferred unresolved forever. Under an "ask" ruleset this hangs the entire session/prompt call indefinitely. This is the same defect described in #12133 ("Permission requests from child sessions do not get forwarded via ACP and hangs forever"). That issue was closed by #13222, but #13222's actual changes (a Windows git-subprocess stdin-inheritance fix in util/git.ts) are unrelated to ACP session registration and never touch acp/permission.ts or acp/session.ts -- the PR's description bundled several superficially similar "ACP hangs" issues together, and #12133 appears to have been swept in by that list without the underlying root cause actually being addressed. Reproduced against current dev HEAD with a real subprocess + scripted tool call (hangs at the ask), and separately against a real `opencode acp` + real acpx + real model in production use (model organically delegated to a Task subagent and the whole invocation hung past a 90s timeout) -- this is a live, currently-reproducible defect, not a stale report. Fix: resolveAncestor() in acp/session.ts walks the SDK's `parentID` chain to find the nearest ancestor session that IS registered in the ACP store, so a child session's permission ask still routes through the connection instead of being silently dropped. permission.ts's Handler.process() uses this instead of a direct tryGet(), and replies with an active reject (not a silent return) when no ancestor can be resolved, so the underlying Deferred is never left hanging regardless of outcome. Regression test: test/cli/acp/child-session-permission.test.ts drives a real `opencode acp` subprocess through a task-tool delegation under an "ask" ruleset and asserts the prompt call resolves (not times out) via the root connection's existing fail-closed no-capability path. Fails (20s timeout) without the fix, passes (~2.5s) with it. Full existing acp+permission suite (249 tests) passes unchanged; typecheck clean. Co-Authored-By: Claude Sonnet 5 --- packages/opencode/src/acp/permission.ts | 49 +++++++++- packages/opencode/src/acp/session.ts | 30 ++++++ .../cli/acp/child-session-permission.test.ts | 94 +++++++++++++++++++ 3 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 packages/opencode/test/cli/acp/child-session-permission.test.ts diff --git a/packages/opencode/src/acp/permission.ts b/packages/opencode/src/acp/permission.ts index 4eeca28f09f5..a98c57057e4d 100644 --- a/packages/opencode/src/acp/permission.ts +++ b/packages/opencode/src/acp/permission.ts @@ -9,7 +9,7 @@ import type { import type { Event, OpencodeClient } from "@opencode-ai/sdk/v2" import { applyPatch } from "diff" import { exists, readText } from "@/util/filesystem" -import type { ACPSession } from "./session" +import { ACPSession } from "./session" import { pendingToolCall, toLocations, type ToolInput } from "./tool" import { Effect } from "effect" @@ -50,8 +50,16 @@ export class Handler { private async process(event: PermissionEvent) { const permission = event.properties - const session = await Effect.runPromise(this.input.session.tryGet(permission.sessionID)) - if (!session) return + const session = await this.resolveSession(permission.sessionID) + if (!session) { + // Unresolvable even after walking the parentID chain (e.g. a child + // session whose ancestor was itself never registered). Reply with an + // active reject instead of silently dropping the event — silence + // leaves the underlying Permission.ask Deferred unresolved forever, + // hanging whichever session/prompt call is waiting on it (G1). + await this.rejectUnresolvable(permission.id) + return + } if (!this.input.connection.requestPermission) { await this.reply(permission.id, "reject", session.cwd) @@ -96,6 +104,41 @@ export class Handler { }) } + // Child/subagent sessions spawned server-side by the `task` tool are never + // registered as their own ACP session (only session/new|load|resume|fork + // register one). Walk the SDK's `parentID` chain to find the nearest + // ancestor that IS registered, so permission asks from those sessions + // still route through this connection instead of being dropped. + private resolveSession(sessionID: string): Promise { + return Effect.runPromise( + ACPSession.resolveAncestor({ + tryGet: this.input.session.tryGet, + sessionId: sessionID, + fetchParentID: (id) => this.fetchParentID(id), + }), + ) + } + + private async fetchParentID(sessionID: string): Promise { + const roots = await Effect.runPromise(this.input.session.list()) + const directories = [...new Set(roots.map((root) => root.cwd))] + for (const directory of directories) { + const info = await this.input.sdk.session + .get({ directory, sessionID }, { throwOnError: true }) + .then((response) => response.data) + .catch(() => undefined) + if (info) return info.parentID + } + return undefined + } + + private async rejectUnresolvable(requestID: string) { + const roots = await Effect.runPromise(this.input.session.list()) + const directory = roots[0]?.cwd + if (!directory) return + await this.reply(requestID, "reject", directory).catch(() => {}) + } + private async writeProposedEdit(sessionId: string, metadata: ToolInput) { const filepath = stringValue(metadata.filepath) const diff = stringValue(metadata.diff) diff --git a/packages/opencode/src/acp/session.ts b/packages/opencode/src/acp/session.ts index f6a7a56bac2f..b36268d1e8e3 100644 --- a/packages/opencode/src/acp/session.ts +++ b/packages/opencode/src/acp/session.ts @@ -229,4 +229,34 @@ function partMetadataKey(input: { messageId: string; partId: string }) { return `${input.messageId}:${input.partId}` } +const MAX_ANCESTOR_DEPTH = 8 + +// Resolves a session id that was never registered as its own ACP session — +// e.g. a subagent session spawned server-side by the `task` tool — back to +// the nearest ancestor that IS registered (a root session/new/load/resume/ +// fork). Without this, permission asks and tool-call updates from child +// sessions hit `tryGet` -> undefined and get silently dropped by callers, +// which (for permission asks) leaves the underlying Permission.ask Deferred +// unresolved forever (see ACP G1: child session permission hang). +// +// `fetchParentID` is supplied by the caller since walking the ancestor chain +// requires an SDK round-trip (this module has no SDK dependency of its own). +export function resolveAncestor(input: { + readonly tryGet: (sessionId: string) => Effect.Effect + readonly fetchParentID: (sessionId: string) => Promise + readonly sessionId: string +}): Effect.Effect { + return Effect.gen(function* () { + let current = input.sessionId + for (let depth = 0; depth < MAX_ANCESTOR_DEPTH; depth++) { + const known = yield* input.tryGet(current) + if (known) return known + const parentID = yield* Effect.promise(() => input.fetchParentID(current)) + if (!parentID || parentID === current) return undefined + current = parentID + } + return undefined + }) +} + export * as ACPSession from "./session" diff --git a/packages/opencode/test/cli/acp/child-session-permission.test.ts b/packages/opencode/test/cli/acp/child-session-permission.test.ts new file mode 100644 index 000000000000..dc0b3e0cd7bf --- /dev/null +++ b/packages/opencode/test/cli/acp/child-session-permission.test.ts @@ -0,0 +1,94 @@ +// Regression test for G1: child/subagent sessions spawned by the `task` tool +// were never registered in the ACP session store, so their `permission.asked` +// events hit `session.tryGet` -> undefined -> early return in +// acp/permission.ts. Under an "ask" ruleset this left the underlying +// Permission.ask Deferred unresolved forever, hanging the whole +// `session/prompt` call. The fix resolves the child session back to its +// registered root ACP session via the SDK's `parentID` chain, so the +// existing `session/request_permission` round trip fires as normal instead +// of the event being silently dropped. +import { describe, expect } from "bun:test" +import type { PromptResponse, RequestPermissionResponse } from "@agentclientprotocol/sdk" +import { Duration, Effect } from "effect" +import path from "node:path" +import { cliIt } from "../../lib/cli-process" +import { createAcpClient as createJsonRpcAcpClient } from "./acp-test-client" +import { initialize, newSession, verifierConfig } from "./helpers" + +type JsonRpcMessage = { + readonly jsonrpc: "2.0" + readonly id?: number + readonly method?: string + readonly params?: { sessionId?: string } + readonly result?: unknown +} + +describe("acp child session permission (G1)", () => { + cliIt.live( + "child/subagent session edit:ask surfaces session/request_permission for the child session instead of hanging", + ({ home, llm, opencode }) => + Effect.gen(function* () { + const raw = yield* opencode.acp({ + env: { OPENCODE_CONFIG_CONTENT: JSON.stringify({ ...verifierConfig(llm.url), permission: { edit: "ask" } }) }, + }) + const acp = createJsonRpcAcpClient(raw) + yield* initialize(acp) + const session = yield* newSession(acp, home) + + yield* llm.tool("task", { + description: "write a file", + prompt: "write child-fix-check.txt", + subagent_type: "general", + }) + yield* llm.tool("write", { filePath: "child-fix-check.txt", content: "child wrote this" }) + yield* llm.text("child done") + yield* llm.text("parent done") + + const promptRequestId = 9999 + yield* raw.send({ + jsonrpc: "2.0", + id: promptRequestId, + method: "session/prompt", + params: { sessionId: session.sessionId, prompt: [{ type: "text", text: "delegate to subagent" }] }, + }) + + // Manually drive the duplex channel: reply to any incoming + // session/request_permission with "reject" (simulating a real but + // permission-unaware client), while watching for the final + // session/prompt response. Captures the sessionId the server used + // for the permission request so we can assert it's the child's real + // id, not the resolved root's. + const capturedPermissionSessionIds: string[] = [] + const outcome = yield* Effect.gen(function* () { + while (true) { + const message = (yield* raw.receive.pipe(Effect.timeout(Duration.seconds(10)))) as JsonRpcMessage + if (message.method === "session/request_permission" && message.id !== undefined) { + if (message.params?.sessionId) capturedPermissionSessionIds.push(message.params.sessionId) + yield* raw.send({ + jsonrpc: "2.0", + id: message.id, + result: { outcome: { outcome: "selected", optionId: "reject" } } satisfies RequestPermissionResponse, + }) + continue + } + if (message.id === promptRequestId) return message + } + }).pipe(Effect.timeout(Duration.seconds(15)), Effect.exit) + + expect(outcome._tag).toBe("Success") + if (outcome._tag !== "Success") return + const response = outcome.value as { result?: PromptResponse } + expect(response.result?.stopReason).toBeDefined() + + // The permission request must have been raised for the CHILD's own + // session id (subagent action honestly attributed), not silently + // dropped and not impersonating the root session. + expect(capturedPermissionSessionIds.length).toBeGreaterThan(0) + expect(capturedPermissionSessionIds).not.toContain(session.sessionId) + + const exists = yield* Effect.promise(() => Bun.file(path.join(home, "child-fix-check.txt")).exists()) + expect(exists).toBe(false) + }), + 30_000, + ) +})