Dedupe @agentclientprotocol/sdk to fix stale-schema request_permission errors - #156
Merged
Conversation
apps/cli depends on @agentclientprotocol/sdk@^1.1.0 directly, but @acp-kit/core pins ^0.18.0, resolving to a stale 0.18.2 that's actually used at runtime by @acp-kit/core's connection factory. That old schema hard-fails session/request_permission requests from the spawned claude-code-acp bridge that the newer, lenient 1.1.0 schema accepts, causing repeated "Invalid params" errors when editing files. Fixes #148 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior override pinned the deduped version to 1.1.0, matching what apps/cli already depended on directly, but that's not the latest published release. Since the payload shapes causing #148 are driven by Claude Code's own rapidly-updating engine rather than anything pinned in this repo, track the newest sdk release for more schema headroom instead of settling on the first version that happened to fix it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@acp-kit/core's own .d.ts still references @agentclientprotocol/sdk's SessionModelState/ModelInfo types, which were dropped entirely in the 1.x rewrite. tsc resolves the hoisted top-level sdk package for type checking (independent of bun's runtime resolution, which correctly keeps @acp-kit/core on its declared 0.18.x range), so once that resolves to 1.x the type silently degrades to any and leaks into modelsFromSession's .map callback. Mirror the wire shape locally instead of trusting the broken upstream type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe CLI upgrades the ACP SDK, enriches approval request diffs, adds wire error logging with request correlation, and types native session model entries. ChangesACP observability and approval mapping
Session model typing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ACPPeer
participant InteractiveHost
participant WireErrorLogger
participant evlog
ACPPeer->>InteractiveHost: JSON-RPC request
InteractiveHost->>WireErrorLogger: inbound frame
WireErrorLogger->>WireErrorLogger: track request by ID
ACPPeer->>InteractiveHost: outbound error response
InteractiveHost->>WireErrorLogger: error frame
WireErrorLogger->>evlog: log request metadata and error
WireErrorLogger->>InteractiveHost: forward frame
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the conflict in apps/cli/src/core/agents/catalog.ts between main's hasNativeModelSelection routing fix (#153) and this branch's RawSessionModel type-safety fix, keeping both: the native/config-option routing logic from main, with the availableModels array still cast through RawSessionModel since @acp-kit/core's own .d.ts still can't resolve SessionModelState against the 1.x sdk.
@agentclientprotocol/sdk's internal request-error console.error was dropped entirely in the 1.x rewrite (only the notification path still logs) - confirmed by diffing 0.18.2's dist/acp.js against 1.3.0's dist/jsonrpc.js. That means bumping the sdk (previous commits on this branch) traded a loud, truncated error for a completely silent one: session/request_permission failures now just vanish. Add a wireMiddleware on the interactive host that taps every JSON-RPC frame at the transport boundary, before @acp-kit/core's connection does its own parsing/validation, and logs the originating request alongside any error response via evlog instead of console.error so the payload isn't truncated. This sits below @agentclientprotocol/sdk entirely, so it keeps working regardless of future internal changes to the sdk's own (evidently unstable) logging. Lives in its own logger.ts module, separate from interactive.ts's permission/elicitation host wiring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The pending map was keyed only by JSON-RPC id, but ids are only unique per direction: cyrus's own outgoing requests (out) and the agent's incoming requests (in) each have independent id sequences that can collide on the same number. The old deletion check fired on any frame carrying result/error regardless of direction, so a response to one of cyrus's own outgoing requests could evict a still-pending agent request that happened to share the same id, before its real error response arrived - losing the correlation this logger exists to provide. Only ever mutate `pending` from the two frames that actually belong to the in-request/out-response pairing this tracks: set on inbound requests, delete on our own outbound responses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ions
Root cause of the "Invalid params" failures with no visible cause:
mapApprovalRequest() built toolCall.content directly from the raw ACP
diff content item ({type, path, oldText, newText}) and validated it
with ApprovalRequestEventSchema.parse(), whose DiffSchema requires
patch/additions/deletions - fields the ACP protocol's Diff type never
sends. Every file-edit permission request threw a ZodError before
pushing the approval_request UI event, which is why no popup ever
appeared. That ZodError propagated up through @acp-kit/core's
handlePermissionRequest catch/rethrow into the SDK's JSON-RPC
dispatcher, which correctly (if confusingly) reported it as
"-32602 Invalid params" - nothing to do with the SDK version work in
the earlier commits on this branch, which fixed a real but separate
issue.
The other three event mappers in this file (tool.start/update/end)
already call enrichDiffContent() to compute these fields locally from
oldText/newText before validating; mapApprovalRequest() was simply
missing the same call. Confirmed via a full ClientSideConnection +
createSdkConnectionFactory + createInteractiveHost reproduction fed
the real captured wire payload, and via a regression test that fails
with the exact same ZodError on the old code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/core/acp/events.test.ts (1)
75-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the generated diff values.
expect.any(Number)andexpect.any(String)verify only the output types. They do not detect incorrect additions, deletions, or patch content. Assert one addition, one deletion, and the expected changed lines.Suggested assertions
- additions: expect.any(Number), - deletions: expect.any(Number), - patch: expect.any(String), + additions: 1, + deletions: 1, + patch: expect.stringMatching(/-old[\r\n]+\+new/),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/core/acp/events.test.ts` around lines 75 - 87, Strengthen the diff object assertions in the approval_request test by replacing the broad additions, deletions, and patch type checks with exact values: one addition, one deletion, and the expected changed-line content. Keep the existing event, toolCallId, and path assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/core/acp/events.test.ts`:
- Around line 75-87: Strengthen the diff object assertions in the
approval_request test by replacing the broad additions, deletions, and patch
type checks with exact values: one addition, one deletion, and the expected
changed-line content. Keep the existing event, toolCallId, and path assertions
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2330bacc-ada9-4a30-ace3-418f85a8d7cf
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/cli/package.jsonapps/cli/src/core/acp/events.test.tsapps/cli/src/core/acp/events.tsapps/cli/src/core/acp/interactive.tsapps/cli/src/core/acp/logger.test.tsapps/cli/src/core/acp/logger.tsapps/cli/src/core/agents/catalog.tspackage.json
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
1 participant
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.
Summary
apps/clidepends on@agentclientprotocol/sdk@^1.1.0directly, but@acp-kit/core@^0.10.2pins^0.18.0, which was resolving to a separate, stale0.18.2— the schema actually used at runtime by@acp-kit/core's connection factory inapps/cli/src/core/acp/transport.ts.session/request_permissionrequests (unrecognized array items fail the whole request) where the modern schema tolerates them (drops unrecognized items instead). This caused the repeatedInvalid paramserrors when editing files.overridesentry to dedupe@agentclientprotocol/sdkto a single resolved version workspace-wide, bumped to the latest1.3.0for headroom against future drift, since the payload shapes are ultimately driven by Claude Code's own frequently-updating engine rather than anything pinned in this repo.tsc's hoisted resolution at the 1.x sdk:@acp-kit/core's own.d.tsreferencesSessionModelState, a type dropped entirely from@agentclientprotocol/sdk's 1.x rewrite, which silently degraded toanyand leaked intomodelsFromSession.Verification
bun pm why @agentclientprotocol/sdkshows a single resolved1.3.0, used by both@cyrus/cli(direct) and@acp-kit/core(nested).@acp-kit/coreactually imports (ClientSideConnection,ndJsonStream,PROTOCOL_VERSION) are unchanged in1.3.0.bun run check:types(workspace-wide) andapps/cliunit suite (105 tests, including all ACP-specific tests) pass.syncpack lintclean.Fixes #148
Test plan
bun installresolves a single@agentclientprotocol/sdk@1.3.0across the workspacebun run check:typespasses workspace-wideapps/cliunit tests pass (105/105)syncpack lintpassesSummary by CodeRabbit
Bug Fixes
Chores