Five ACP integration papercuts: an unanswered request_permission hangs the turn forever, session/close returns -32601, and three config traps

[chyroc]

Summary

Five small things we hit while integrating DSH over ACP into a multi-tenant platform. Each cost us real debugging time, each has a one-line fix or a one-paragraph doc note, and none deserves its own thread. Filing together; happy to split if you prefer.

Versions: @deepseek-ai/dsh-acp 0.1.1-rc.2, @agentclientprotocol/sdk 0.25.1.

1. An unanswered session/request_permission blocks the turn forever

This is the one that actually hurt — it presents as "session/prompt never returns," with no error and nothing in the log.

Before a tool call the bridge sends session/request_permission to the client (packages/acp/acp/src/index.ts:271-285) and awaits the response with no timeout. A client that doesn't implement the reverse request — easy to miss, since the method is a client obligation — hangs indefinitely on its first tool-using turn.

Fix we'd suggest: document it prominently as a hard client obligation in packages/acp/acp/README.md (the protocol-contract table lists the method but not the consequence of ignoring it), and ideally add a config for non-interactive deployments — a bounded wait that falls back to the existing fail-closed unavailable outcome would have turned our silent hang into a clear denial.

Our workaround: we always answer. Our permission model lives at a different layer (tools are externalized to a separate sandboxed executor), so a second in-DSH prompt would be redundant — we answer allow-once unconditionally and enforce policy where the tools actually run.

2. session/close is not implemented → -32601

closeSession is an optional method on the SDK's Agent interface, dispatched at dist/acp.js:86-92:

case schema.AGENT_METHODS.session_close: {
    if (!agent.closeSession) { throw RequestError.methodNotFound(method); }

The bridge implements initialize, authenticate, newSession, prompt and cancel (src/index.ts:290-439) but not closeSession, so the wire method session/close (dist/schema/index.js:21) returns -32601. README.md:81 documents this as intended — "one connection releases all of its sessions; per-session close is not implemented" — and for the in-repo subagent client, where connection lifetime equals session lifetime, that's a perfectly coherent choice.

It's a poor fit for a long-lived host serving many sessions over one connection: the only way to release one session's resources is to tear down the connection and every other session with it. Sessions are already individually disposable — SessionRecord.dispose exists at src/index.ts:88 and quiesce already calls it per record (:493) — so a minimal closeSession looks like it could reuse the existing per-record teardown almost directly.

Our workaround: we call session/close, treat -32601 as success, and rely on an idle-timeout reaper to bound resource growth.

3. Default maxTokens of 256000 is rejected outright by common gateways

DEFAULT_MAX_TOKENS = 256_000 (packages/llm/llm-deepseek/src/adapter.ts:134, applied at src/index.ts:366). Many OpenAI-compatible gateways cap max_tokens well below that — the one we deploy behind caps at 32768 — and reject the request rather than clamping. For comparison, the sibling adapter llm-pi-ai defaults to 32_768 (packages/llm/llm-pi-ai/src/config.ts:64).

The failure surfaces as a generic provider error, so the first guess is credentials or model id, not a default ceiling.

Fix we'd suggest: a lower default, or map the provider's "max_tokens too large" rejection to a message naming maxTokens and the configured value. The latter is probably more valuable — the number is deployment-specific, but the diagnostic is universal.

Our workaround: pin maxTokens per model in our profile patch layer.

4. sandbox-policy.mode and approval.policy must be changed together

permission-presets validates that the composed sandbox + approval defaults hit exactly one preset, and otherwise fails the whole plugin-tree load (packages/interaction/permission-presets/src/index.ts:214):

permission: composed sandbox and approval defaults match no preset; configure defaultPreset explicitly

Change only one of the two and startup fails. The coupling is not obvious from either plugin's own config, and the error names defaultPreset — a third row you may not have touched. The official example does thread one env var through both (examples/acp-agent/cordis.yml, the sandbox-policy and approval rows), which is exactly the right pattern; it just isn't called out as a rule anywhere, so it reads as incidental.

Fix we'd suggest: one sentence in the permission-presets README — "these two move together; drive both from one value or set defaultPreset explicitly." The error message is already good once you know what it means.

Our workaround: we drive both from a single environment variable rather than patching either row.

5. acp-demo collides with the persistence rows inherited from bundle/base

acp-demo mounts its own JSONL persistence and SQLite query engine (packages/examples/acp-demo/src/index.ts:123 and :134). bundle/base ships rows with the same service identities — session-persistence-jsonl (packages/bundle/base/cordis.patch.yml:98) and session-query-sqlite (:117) — which headless inherits. Composing acp-demo onto a headless-derived profile therefore fails at startup on duplicate service registration until both base rows are disabled.

Worth noting the official examples/acp-agent/cordis.yml sidesteps this entirely by composing from the plugin rows directly rather than layering on headless — so the supported path is fine. The trap is specifically for people who start from headless (reasonable, if you want a non-interactive base) and add ACP on top.

Fix we'd suggest: either note in the acp-demo README that it owns persistence and must not be layered over a bundle that already provides it, or make its internal mounts conditional on the services being absent.

Our workaround: our patch layer disables session-persistence-jsonl and session-query-sqlite.

A related note on hmr, and a correction to our own first guess

We disable hmr in containers because cordis-plugin-hmr watches recursively from root: ['.'] (packages/bundle/base/cordis.patch.yml:19-22), and in a container with a low fs.inotify.max_user_instances a recursive watch over a whole profile tree can exhaust watcher instances.

We had assumed this was an ACP-composition gap, and it is not: bundle/headless already disables hmr (packages/bundle/headless/cordis.patch.yml:14-15), and acp-demo's module doc explicitly states "No logger, no hmr — stdout stays pure" (packages/examples/acp-demo/src/index.ts:111). Both correct already. Mentioning it only in case a note in the container/deployment docs would help others who reach for a base-derived profile where the row is still live.

Offer

Happy to send PRs for any of these — the closeSession implementation and the doc notes are the ones we could turn around fastest. And happy to be corrected if any of the five is working as intended.

[denial123789]

The permission item has a useful conformance rule for ACP clients: every live reverse request must receive exactly one terminal selected or cancelled response within a client-owned deadline. Closing a dialog or timing out local UI without sending that response still leaves the bridge awaiting the request.

A bounded client flow should:

1. retain the exact JSON-RPC request id and offered option ids;
2. decide from authenticated predeclared policy or an honest generic UI;
3. validate that the selected option came from that same request;
4. send cancellation on deadline, dismissal, shutdown, or missing policy; and
5. invalidate late/duplicate answers after prompt cancellation or Session disposal.

Blanket allow-once is safe only if a separate authoritative executor really owns every effect. If the same DSH tool still performs local effects, auto-allow removes a real boundary rather than eliminating redundant UI.

I updated the ACP permission client guide with the hang trace, timeout state machine, and conformance gates: https://github.com/sandbaseai/deepseek-harness-handbook/blob/main/docs/en/integrations/acp-permission-request-ui.md#an-unanswered-reverse-request-hangs-the-tool-using-turn

Disclosure: I maintain the SandBase community handbook.

[weijiafu14]

Item 1 is cheaper to fix than your write-up suggests, and I think the report can be sharpened in one place. I read the handler and the approval types on the same version line.

The fail-closed outcome you are asking for already exists in the vocabulary.

export type ApprovalOutcome = 'allowed-once' | 'rejected' | 'cancelled' | 'unavailable'

and every consumer is already forced to handle all four — core/tools/src/index.ts ends its switch with assertNever(outcome, 'ApprovalOutcome'), and the sandbox escalation path has a distinct verbatim text for it ("requires approval, but no approval channel is available"). So a timeout does not need a new outcome, a new config shape, or any change outside the bridge: it needs the existing string returned from one place.

And that one place currently has neither a timeout nor a catch. The whole handler is:

ctx.on('approval/request', (request, next) => {
  const record = ownedRecord(request.agent)
  if (record === undefined || request.callId === undefined) return next()
  return conn.requestPermission({ … }).then(({ outcome }) => {
    if (outcome.outcome === 'cancelled') return 'cancelled'
    return outcome.optionId === 'allow-once' ? 'allowed-once' : 'rejected'
  })
})

No .catch(), no race, no deadline. So the change is a bounded race around that promise resolving to 'unavailable' — plus a .catch(), which brings me to the sharpening.

Worth pinning down: a client that does not implement the method and a client that implements it and never answers are probably two different failures, and your report merges them. You wrote that a client which does not implement the reverse request hangs indefinitely. But an unimplemented JSON-RPC method should come back as -32601, which would reject conn.requestPermission(...) — and since there is no .catch(), that rejection propagates into the waterfall rather than hanging. If that is what happens, the unimplemented case fails loudly (badly, but loudly) and only the silent client hangs.

I could not confirm which it is without your client, and it changes what to ask for: a rejection wants a .catch() mapping to 'unavailable'; a silent non-answer wants the deadline. The handler has neither today, so both belong in the fix — but the doc note you are asking for should say the right thing, and "not implementing it hangs your turn" may be scarier and less accurate than "implementing it and not answering hangs your turn."

Cross-link worth making: #4708 asks for server→client user-interaction requests on the SDK JSON-RPC wire, which does not have them at all. Read together, the two threads are the same gap from opposite sides: ACP has the request direction and no deadline semantics; the SDK wire has neither. Your item 1 is precisely the failure mode that arrives with that capability, so whoever specs the SDK-wire version should treat the bounded-wait requirement as part of the spec rather than a follow-up. I said as much over there and am pointing back here.

On your allow-once-unconditionally workaround: @denial123789's caution is the right one and I will not repeat it. I will only add that it is load-bearing on a claim about your deployment (a separate authoritative executor owns every effect) that a reader of this thread might copy without the same architecture. Worth one sentence in the post saying so.

Item 2 (session/close → -32601) reads correct to me as well, including your observation that SessionRecord.dispose already exists and quiesce already calls it per record — so the minimal implementation really does look like exposing existing teardown.

Interest disclosure: I maintain a third-party DSH plugin. We do not touch the ACP bridge and could not fix any of this; the above is source verification against the same version line plus one cross-thread pointer.