fix(acp): child/subagent session permission requests no longer hang forever - #37902
Open
aiken884 wants to merge 1 commit into
Open
fix(acp): child/subagent session permission requests no longer hang forever#37902aiken884 wants to merge 1 commit into
aiken884 wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
…rmission 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 anomalyco#12133 ("Permission requests from child sessions do not get forwarded via ACP and hangs forever"). That issue was closed by anomalyco#13222, but anomalyco#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 anomalyco#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 <noreply@anthropic.com>
Copy link
Copy Markdown
|
Independent confirmation from another ACP client integration. Environment:
I reproduced this deterministically with: OPENCODE_PERMISSION='{"*":"ask","task":"allow"}' \
opencode acp --print-logsThe ACP client:
task: allow ensures that the parent task itself does not require permission. The wildcard ask ensures that the child read does. Observed on both 1.15.10 and 1.17.11:
Result from both runs: {
"childReadAskObserved": true,
"parentPermissionRequestCount": 0,
"childPermissionRequestCount": 0,
"promptSettledBeforeCancel": false,
"routing": "missing-from-acp-wire"
}This independently confirms that #12133 is still reproducible and was not fixed by #13222. The resolveAncestor() approach in this PR targets the exact failing path. It would also be useful to keep coverage for nested subagents and simultaneous permission requests from multiple children sharing the same ACP parent. |
Sorry, something went wrong.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Closes #12133
Type of change
What does this PR do?
Child sessions spawned server-side by the task tool are never registered in the ACP session store (only session/new/load/resume/fork register one). When such a session raises a permission.asked event, acp/permission.ts's Handler.process() calls session.tryGet() on that unregistered id, gets undefined, and just returns — the underlying Permission.ask Deferred is never resolved, so under an "ask" ruleset the whole session/prompt call hangs forever.
This is exactly #12133. That issue shows as closed via #13222, but #13222's actual diff (util/git.ts, snapshot/index.ts) fixes a Windows-only git-subprocess stdin-inheritance deadlock — it never touches acp/permission.ts or acp/session.ts. The PR description lists several "ACP hangs forever" issues together and #12133 looks like it got swept into that list without the actual root cause being addressed. I hit the same hang on current dev with a real subprocess.
Fix: resolveAncestor() (new, in acp/session.ts) walks the SDK's parentID chain for a session id that isn't in the ACP store, until it finds an ancestor that is registered. permission.ts's Handler.process() uses this instead of a bare tryGet(), so a child session's permission ask still routes through the connection. If no ancestor can be resolved at all, it now replies with an active reject instead of silently returning, so the Deferred is never left hanging either way.
How did you verify your code works?
Added test/cli/acp/child-session-permission.test.ts: drives a real opencode acp subprocess through a task-tool delegation under an edit:"ask" ruleset and asserts the session/prompt call resolves instead of hanging. Confirmed it fails (20s timeout) on dev without this change, passes (~2.5s) with it.
Also reproduced this against real acpx + a real model outside the test suite: with a strict permission policy, the model chose on its own to delegate a file-write to a Task subagent, and the whole invocation hung past a 90s timeout with the same symptom.
Ran the full existing acp + permission suite (249 tests) — all pass, no regressions. tsgo --noEmit clean.
Checklist