fix(acp): normalize session/request_permission responses into the SDK decision contract - #23
Closed
snowykr wants to merge 13 commits into
Closed
fix(acp): normalize session/request_permission responses into the SDK decision contract#23snowykr wants to merge 13 commits into
snowykr wants to merge 13 commits into
Conversation
Dev CI run 31102063678 (dev @473eab944) failed because coding-agent shard 7 timed out in "Smithery env trust boundary > uses the built-in endpoints and no key by default" after 5004ms (Bun's default per-test timeout). Root cause: each test spawns a child Bun process (smithery-env-probe.ts) that eagerly parses 9 env/dotfiles (5 shell rc files + 4 .env paths) at module load via $credentialEnv → env.ts:39-50. Locally each test runs ~800ms, but under CI's 7-shard parallel load the child-process spawn + TS compilation + synchronous file I/O can exceed the 5s default. This is a CI flake (resource contention), not a deterministic regression — reproduced 5 pass / 0 fail consistently at dev 473eab9 across 3 local runs (~4s total for all 5 tests). The fix adds a 30s per-test timeout matching the established codebase pattern (agent-session-resilient-retry .test.ts:589,1283 use 30_000 for child-process tests). No production code or test logic changed — only the timeout budget. Lore-id: 7d2e8c1f Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 3x reproduction at dev 473eab9 (5 pass / 0 fail each, ~4s total); post-fix verification (5 pass / 0 fail, ~6s total) Not-tested: CI shard-7 parallel contention (requires CI runner) Supersedes: dev CI run 31102063678 failure evidence
9 tasks
The issue fixture treated one configured model's limits as provider-wide defaults for unrelated discovered ids. Cover same-id YAML precedence, exact-id model overrides, and unknown fallback independently. Lore-id: issue-3954-yaml-discovery-contract Constraint: model configuration remains identity-scoped Rejected: provider-wide cross-model limit inheritance | not supported by schema or runtime merge semantics Confidence: high Scope-risk: narrow Reversibility: easy Tested: focused issue regression and three nearby model-registry discovery checks
snowykr
force-pushed
the
feat/acp-permission-shape
branch
from
August 6, 2026 15:57
7df4fa9 to
b8a544e
Compare
…eachan-Heo#3948) Red-team CLEAR at f9c0fe0. All CI checks success/skipped. Test-only timeout stabilization for Dev CI flake 31102063678.
snowykr
force-pushed
the
feat/acp-permission-shape
branch
from
August 6, 2026 17:23
b8a544e to
1f9bc2c
Compare
…Heo#3961) Red-team MERGE_READY at 460285d. Test-only contract correction for post-Yeachan-Heo#3927 discovery limits (same-id YAML, modelOverrides, UNK fallbacks). No runtime change. Local public surfaces failed pre-checkout on GitHub Actions 5xx only.
Yeachan-Heo
force-pushed
the
feat/acp-permission-shape
branch
2 times, most recently
from
August 6, 2026 19:35
fcc1fc4 to
aaa8253
Compare
…ntract Dev CI at 4f6e860 fails provider-onboarding-wizard-redteam on a stale assertion: it expected "At least one model id is required" while addApiCompatibleProvider throws the post-Yeachan-Heo#3927 public message "At least one model id or model discovery is required." Update the empty-models red-team expectation to the exact intended substring of that contract. No runtime change. Lore-id: 9c4e1a02 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/coding-agent/test/provider-onboarding-wizard-redteam.test.ts (7 pass / 0 fail) Not-tested: full Dev CI shard matrix
…ntract (Yeachan-Heo#3966) Red-team MERGE_READY. Test-only: assert post-Yeachan-Heo#3927 public error 'At least one model id or model discovery is required'. Dev CI 31127515306 success. No runtime change.
Yeachan-Heo
force-pushed
the
feat/acp-permission-shape
branch
from
August 6, 2026 21:42
aaa8253 to
abc3cb1
Compare
Dev CI run 31128319216 (Yeachan-Heo#3855 exact head) failed only when "uses the built-in endpoints and no key by default" hit 30004ms — exactly the 30s per-test cap from Yeachan-Heo#3948. Assertions and provider behavior were green; this is CI contention at the previous budget. Raise each child-process it() timeout from 30_000 to 60_000, matching existing 60s child-process patterns (e.g. acp-session-delete-wire). No assertion or production change. Lore-id: a3f8c201 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: sequential 3x (5 pass/~4s); 6-way parallel contention (6x 5 pass/~4s) Not-tested: full 8-shard Dev CI load shape
…chan-Heo#3967) Red-team MERGE_READY at accba29. Test-only 30s→60s timeout for smithery-env-trust child-process CI contention flake (run 31128319216 @30004ms). Unblocks Yeachan-Heo#3855.
Yeachan-Heo
force-pushed
the
feat/acp-permission-shape
branch
from
August 6, 2026 22:12
abc3cb1 to
3a8f335
Compare
The builtin notify handler always returns {prompt: command.text} for on/off so
it cannot shadow the extension-owned per-session control — whether or not a
lazy/native command is installed. A stale fixture on the startup-cost PR
expected {consumed:true} + "unavailable" for the no-command path and failed
exact-head shard-5.
Add the contract on dev as a minimal baseline so the performance PR does not
absorb unrelated expectation rewrites into its diff.
Lore-id: ntfy-pass-1
Confidence: high
Scope-risk: narrow
Reversibility: reversible
Tested: bun test packages/coding-agent/src/slash-commands/notify-command.test.ts
Not-tested: full coding-agent shard matrix
…nly-pass-through test(coding-agent): lock /notify on|off always-pass-through contract
… decision contract
ACP clients answer `session/request_permission` with the spec-shaped
`RequestPermissionResponse` (`{ outcome: { outcome, optionId } }`), but the SDK
permission-provider contract is the flat decision (`{ outcome, optionId }`).
The ACP reverse bridge returned the raw nested payload, so every
permission-gated tool call (bash, edit, write) under a standards-compliant
client such as Paseo failed with "permission provider returned an invalid
response". Unwrap the outer outcome at the adapter boundary; the flat legacy
shape passes through unchanged.
Lore-id: 2b18a5e0
Constraint: permission decisions must stay fail-closed -- malformed responses still surface the existing invalid-response error
Rejected: accepting both shapes in the SDK bus | duplicates ACP wire knowledge in the generic SDK contract
Confidence: high
Scope-risk: narrow
Reversibility: revert-commit
Tested: nested selected/cancelled and flat shapes via createAcpReverseConnection unit test; live ACP probe against cli.ts acp with a Paseo-shaped nested response
Not-tested: other ACP client implementations beyond Paseo-shaped responses
The response-shape fix was only proven at the reverse helper boundary, leaving the registered provider path, alias, and typed bridge contracts exposed to regression. Add a coupled fail-closed fixture and correct the release note to match the actual guarded tools. Lore-id: pr3953-followup Constraint: keep ACP envelope parsing at createAcpReverseConnection Constraint: SDK permission providers remain flat and fail closed Tested: ACP startup, client bridge, agent-session permission, SDK host permission, coding-agent package check Not-tested: full repository test suite
Yeachan-Heo
force-pushed
the
feat/acp-permission-shape
branch
from
August 6, 2026 22:45
3a8f335 to
5006314
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5006314cb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,56 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
This ACP response-normalization commit also adds /notify routing coverage and changes unrelated provider-discovery, onboarding-wizard, and Smithery-timeout tests. Bundling these independent changes makes the ACP fix harder to review, revert, or cherry-pick safely; move them into separate logical commits as required by the repository contract.
AGENTS.md reference: AGENTS.md:L152-L155
Useful? React with 👍 / 👎.
Owner
Author
|
Closing: identical change merged upstream via Yeachan-Heo#3953. |
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.
Problem
Permission-gated tool calls (
bash,edit,write) fail with "permission provider returned an invalid response" whenever GJC runs as an ACP agent under a standards-compliant ACP client. Reproduced with Paseo 0.3.0-beta.2 (the app's daemon launchescli.ts acpas the provider), affecting every bash/edit/write call once the client answers a permission prompt.Affected environment
gjc ... acp(any ACP client).session/new, prompts the agent to usebash, then answers the incomingsession/request_permissionwith the spec-shaped response. The tool call fails withpermission provider returned an invalid response; expected behavior is that the tool executes.Root cause
The ACP spec (
@agentclientprotocol/sdk, pinned1.3.0) defines the response tosession/request_permissionasRequestPermissionResponse:Paseo (and the official SDK's own fixtures) send exactly this nested shape. GJC's SDK permission provider (
sdk/bus/index.ts) validates the flat decision{ outcome: "selected", optionId }, soresponse.outcomeis an object instead of the expected string and the validation rejects it. The ACP reverse bridge (modes/acp/acp-agent.tscreateAcpReverseConnection) returned the raw wire payload without the unwrap that the siblingacp-client-bridge.tspath already performs.Change
Normalize
session/request_permissionresponses at the ACP adapter boundary (createAcpReverseConnection):{ outcome: { outcome, optionId } }→ SDK contract{ outcome, optionId }(selected / cancelled).Scope is one function plus a regression test and a changelog entry. No public API or behavior change beyond permission prompts resolving correctly.
Validation
test/acp-startup-options.test.ts): nested selected, nested cancelled, and flat passthrough — all pass.acp-startup-options,acp-client-bridge,agent-session-acp-permission,sdk-acp-production-path— all pass.bun --cwd=packages/coding-agent run check(biome + tsc) passes on the final head.echo daemon-permission-ok, answered the permission request with the nested spec shape. Transcript showstoolResult "daemon-permission-ok\n"and a completed turn;permission provider returned an invalid responseappears 0 times.Related work
session/request_permissionchannel and would hit the same shape bug; this change is complementary (fixes the permission response contract) and not a duplicate.permission provider,request_permission).Checklist
dev; CI runs on the final head commit