fix(coding-agent): include provider response in permission reverse-request errors - #3822
fix(coding-agent): include provider response in permission reverse-request errors#3822developjik wants to merge 1 commit into
Conversation
…quest errors The SDK reverse permission provider threw a bare "permission provider returned an invalid response" for both a null/non-object response and an unexpected outcome, discarding the payload the host actually returned. When a host (ACP host, notifications daemon) answers a permission reverse-request with the wrong shape, the only signal was the opaque message with no detail about what was received. Surface a bounded description of the response (the string itself, or a JSON snapshot capped at 500 chars, with a String() fallback for non-serializable values) in both throw sites so the mismatch is immediately diagnosable. - packages/coding-agent/src/sdk/bus/index.ts: describeResponse helper + both throws - packages/coding-agent/CHANGELOG.md: Unreleased Fixed entry
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
GJC GPT-heavy adversarial exact-head review
Terminal verdict: REQUEST_CHANGES
Reviewed head: d0a12788d0ae1b6f44bd503a4bfb9c3abc0c15a3
Current base: dev at f9dffed426a433bf4948bdeb8fb2fe76f25ef5ca (direct parent)
GitHub state: open, non-draft, mergeable/rebaseable, unstable
HIGH — raw provider payloads cross the error boundary without a disclosure policy
packages/coding-agent/src/sdk/bus/index.ts:3648-3662 and 3691-3698 append the reverse-provider result to propagated Error.message values. The helper passes strings through and JSON-serializes object values, so the 500-character cap limits length but does not redact secrets, terminal/Bidi controls, or file content.
The fs claim in the PR description is not safe: an invalid response can still contain the file body in a non-string shape such as { content: { data: "..." } } or { content: ["..."] }, and this change copies that content into the error. Permission providers can likewise echo sensitive request data or credentials. The permission error is awaited without a local sanitizing catch by packages/coding-agent/src/session/agent-session.ts:6634-6642,6697-6716. Nearby SDK failure handling explicitly preserves only a discriminator and fixed message rather than provider text (sdk/bus/index.ts:5890-5892).
Required: keep raw values out of public error text. Report a structured, content-free descriptor (JSON kind, expected schema, allowlisted field names/counts) and put any payload-level diagnostics behind an explicit local, redacted debug sink. Add non-disclosure coverage with token-like and malformed fs payloads.
MEDIUM — the advertised never-throwing helper is not total
describeProviderResponse at sdk/bus/index.ts:3226-3238 calls Object.prototype.toString.call(value) in and after the catch. A value whose toJSON forces fallback and whose Symbol.toStringTag getter throws, or a hostile/revoked Proxy, escapes the helper. A standalone probe matching the exact helper logic produced THREW:tag boom. The test at test/provider-response-description.test.ts:46-56 covers only an ordinary enumerable getter.
Required: after serialization failure return a fixed literal without inspecting the value again; cover throwing Symbol.toStringTag, Proxy/revoked Proxy, and toJSON cases.
MEDIUM — changed production branches are untested
test/provider-response-description.test.ts:4-57 only calls the helper. Removing the interpolation from any of the three throw sites would leave all added tests green. Exercise invalid permission and fs reverse responses through the existing live SDK provider wiring and assert the observable, redacted rejection contract.
MEDIUM — unrelated changelog duplication was introduced
packages/coding-agent/CHANGELOG.md:18-20 adds a third ### Fixed block and repeats the Telegram callback entry already present at lines 13-15. Remove the duplicate and place the SDK note under the existing Unreleased Fixed section.
LOW — truncation can split a surrogate pair
text.slice(0, 500) can retain only the high surrogate of a non-BMP character before appending the ellipsis. Keep the output well-formed at the boundary and add a non-BMP test.
Exact-head CI and verification
- Check runs:
0; legacy statuses:0(pending). Dev CIrun30914334626:action_required.Public site syncrun30914337020:action_required.- No CI action was taken.
- Exact-head focused tests could not execute in the isolated worktree: frozen dependency installation hit the environment's
node-pty/node-gypfailure, and the subsequent target test was blocked by an installed native binary lacking the@gajae-code/natives@0.12.11sentinel. This is not counted as a source test failure.
Contributor ledger / issue disposition
- Admission: admitted as a real diagnostics gap; current base still emits opaque fixed errors. The proposed disclosure mechanism is not admitted.
- Duplicate: no competing implementation or linked closing issue was found; exact error/helper searches identify this PR. The changelog itself contains the unrelated duplicate noted above.
- Current owner path:
developjik:fix/acp-permission-response-clean→ PR #3822 →Yeachan-Heo:dev. Ownership remains withdevelopjikon this same PR/branch; no replacement PR or parallel mutation lane is warranted.
Signature: GJC hostile review | exact-head d0a12788d0ae1b6f44bd503a4bfb9c3abc0c15a3 | base f9dffed426a433bf4948bdeb8fb2fe76f25ef5ca | two independent layofflabs/gpt-5.6-terra architect lanes plus maintainer pass | GitHub actor Yeachan-Heo | REQUEST_CHANGES
VERDICT: REQUEST_CHANGES
2 participants
What
Surface a bounded description of the reverse-provider response in the SDK's permission/fs error messages, so a host that answers a reverse-request with the wrong shape is immediately diagnosable instead of opaque.
Why
The SDK reverse permission/fs provider threw a bare
permission provider returned an invalid response/fs provider returned an invalid read responsefor both a null/non-object response and an unexpected outcome, discarding the payload the host actually returned. When an ACP host or the notifications daemon answers a reverse-request with the wrong shape, the only signal was the opaque message with no detail about what was received.Change
describeProviderResponse(value)helper inpackages/coding-agent/src/sdk/bus/index.ts: strings pass through; other values are JSON-serialized and capped at 500 chars (with…); non-serializable values (circular structures, throwing getters, BigInt) fall back to a never-throwingObject.prototype.toString.calltag.permission provider returned an invalid responsethrow sites and thefs provider returned an invalid read responsesite now append: ${describeProviderResponse(result)}.The fs site only throws when
contentis not a string, so genuine file bodies (strings) return normally and never reach the error — real file content cannot leak through this path.Testing
bun test packages/coding-agent/test/provider-response-description.test.ts→ 8 pass (strings, 500-cap + ellipsis, JSON objects, null/primitives, undefined, circular, self-referential array, throwing getter).tsc --noEmit -p packages/coding-agent/tsconfig.json→ clean.biome checkon changed files → clean.Focused checks per CONTRIBUTING ("focused tests first"); the full
bun checkis left for CI.GJC verdict
Independent architect review (severity-rated): core logic correct, type-safe, robust; ship-with-nits (remaining items are review-polish: an optional fs-site redaction comment and moving the test-only export out of the barrel — non-blocking).
devbun checkpasses (focused checks pass locally; full check pending CI)