Skip to content

fix(acp): request ids stay within signed 32-bit so Kotlin agents answer - #15374

Open
Mnigos wants to merge 2 commits into
pingdotgg:mainfrom
Mnigos:acp-rpc-request-ids-int32
Open

Mnigos wants to merge 2 commits into
pingdotgg:mainfrom
Mnigos:acp-rpc-request-ids-int32

Conversation

@Mnigos

@Mnigos Mnigos commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #15263.

Problem

effect-acp starts typed Effect RPC request ids at 2 ** 32 in both AcpClient and AcpAgent. That offset came with the original ACP support (#1355) to keep typed ids above extension request ids, which count up from 1. The Kotlin ACP SDK, which Junie and other ACP Registry agents use, stores numeric ids in RequestId.IntId (a Kotlin Int) and decodes them with toInt(), so 4294967296 fails to decode. The SDK treats the frame as malformed. Older builds may stay silent; current source answers with an error whose id is null, which T3 cannot correlate and drops. Either way, initialize never resolves and the provider status probe times out after 60 s ("did not resolve and create a test session"). ACP's RequestId is int64, so T3 is within spec; the break is at the signed 32-bit limit.

Change

One exported constant, RPC_REQUEST_ID_START = 2 ** 30 in protocol.ts, now seeds both counters. Response routing is unchanged: replies are matched against pending extension requests first, and any other safe-integer id goes to the RPC client, independent of where the counter starts. The new start leaves about 1.07 billion extension ids below it and about 1.07 billion typed ids per connection before leaving signed int32.

Scope and approval

Triaged in the triage comment, which prescribes: "Start the RPC counter inside the signed 32-bit range (for example 2 ** 30, as you suggested), in both the client and the agent." packages/effect-acp only; no contract, client or adapter change. Every ACP provider shares this transport, so every one benefits, and agents that handle int64 ids see no behaviour change apart from smaller id values.

The root limitation is upstream: the Kotlin SDK decodes ids as Int (serializer) and answers malformed requests with a null id (malformed response). Decoding as Long and echoing a recoverable id there would help other clients too.

Verification

Observed result. AcpClient.make initialized and created a session through the repository ACP mock behind a gate script that mimics the Kotlin SDK by silently dropping numeric request ids above 2147483647. Each state ran three times. "Before" is the same code with the old starting constant; reversing only the constant extraction reproduces the three production files from main at 00eb8f6 byte for byte.

Frame Before After
initialize id 4294967296, dropped; 10 s timeout in 3/3 runs id 1073741824, forwarded; success in 0.294–0.326 s, 3/3 runs
session/new not reached id 1073741825; success by 0.300–0.330 s from driver start, 3/3 runs

The gate models the unanswered call, not the current SDK's exact id: null error frame, which T3 drops the same way.

Tests. All 73 effect-acp tests pass. The client and agent tests assert the first typed id 2 ** 30, the next 2 ** 30 + 1, extension id 1, and that each reply reaches its caller. Agent-side incoming ids 0, 2 ** 30 and 2 ** 32 are answered while an outbound request is pending, including the same id in opposite directions. With the old constant, both counter tests fail on 4294967296 versus 1073741824. The ACP provider tests and the AcpAdapterV2 and AcpRegistryAdapterV2 suites pass (542 tests, 4 skipped). Package and server typecheck, targeted lint, format and knip are clean; lockfile unchanged. Two existing AntigravityAdapterV2 file-system tests fail identically with either constant (a macOS /var vs /private/var temp-path mismatch) and are unrelated.

Not checked: real Junie (its registry archive plus extracted binary is about 770 MB, not fetched), authentication or chat, UI readiness, counter exhaustion past a billion requests, and cross-platform runtime behavior.

Implemented with Claude Code (Claude Opus 5.5, coordinated by Claude Fable 5.1); tests, independent review and the observed result by GPT-6 Astra via Codex.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 785a691

Macroscope's review found this PR approvable — This is a narrowly scoped ACP interoperability fix that changes only generated request-ID values, preserving the existing RPC flow and extension-ID separation. Both client and agent paths are covered by tests, with no product-default, schema, deployment, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bba7e74a-cecd-480d-99e6-3107405eb1c1
📥 Commits

Reviewing files that changed from the base of the PR and between 785a691 and 268ef9a.

📒 Files selected for processing (5)
  • packages/effect-acp/src/agent.test.ts
  • packages/effect-acp/src/agent.ts
  • packages/effect-acp/src/client.test.ts
  • packages/effect-acp/src/client.ts
  • packages/effect-acp/src/protocol.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Outgoing ACP RPC request IDs now start at 2 ** 30 in both the client and agent. Tests check that generated IDs remain within signed int32 limits, increment as expected, and remain distinct from extension-request IDs. Other tests verify handling of incoming initialize IDs.

Changes

ACP RPC request IDs

Layer / File(s) Summary
Set and verify the RPC request ID range
packages/effect-acp/src/protocol.ts, packages/effect-acp/src/client.ts, packages/effect-acp/src/agent.ts, packages/effect-acp/src/client.test.ts, packages/effect-acp/src/agent.test.ts
The protocol exports RPC_REQUEST_ID_START as 2 ** 30. The client and agent use it to initialize their RPC request ID counters. Tests check generated IDs and increments, separation from extension IDs, and preservation of incoming initialize IDs.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 268ef

No actionable merge-blocking issue is established; the change is ready for normal merge checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 268ef

The smaller starting value improves compatibility without changing which operations agents can invoke or how ordinary responses are assigned to callers. No material security regression was identified. The counters still have finite compatibility and separation margins on exceptionally long-lived connections.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The direct exposure is request correlation within each ACP transport, including session calls and permission requests. The inspected change adds no cross-transport state or broader authority.

Trust Boundaries and Controls

  • inferred — A peer's supplied response ID still passes through the existing identity and routing rules. Lowering the generated seed does not itself authorize a new handler or bypass a method boundary.

Resilience and Maintainability Implications

  • observed — Extension completion removes pending ownership, interruption removes the pending entry, and termination clears and fails pending extension requests while notifying typed RPC consumers. These containment mechanisms are unchanged by the seed adjustment.

Hardening Proposals

  • proposed — If billion-request connection lifetimes must be supported, enforce allocation bounds and drain or replace the transport before exhaustion. Avoid wrapping IDs while requests remain pending, and test ownership at both namespace and signed-int32 limits.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The incremental diff since the previous review adds changes unrelated to issue #15263. Examples include new review-tool configuration in .coderabbit.config.ts and CI or release workflow changes in `… Remove the unrelated review-tool and workflow changes from this pull request, or move them to a separate pull request with their own scope.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the ACP request-ID fix and its purpose: keeping IDs within signed 32-bit range so Kotlin agents can respond.
Description check ✅ Passed The description covers the problem, change, scope and approval, and verification. It reports specific test results and limits, and links the triaged approval.
Linked Issues check ✅ Passed Issue #15263 requires ACP RPC IDs within signed int32 range while avoiding extension-ID collisions. The current change sets RPC_REQUEST_ID_START to 2 ** 30 and uses it in both AcpClient and `Acp…
Approvability ✅ Passed PASS. The diff changes only packages/effect-acp/src/agent.ts, client.ts, and protocol.ts, plus their tests. It moves locally generated RPC IDs into the signed 32-bit range to fix ACP interoperab…
Full details: Out of Scope Changes check

Explanation

The incremental diff since the previous review adds changes unrelated to issue #15263. Examples include new review-tool configuration in .coderabbit.config.ts and CI or release workflow changes in .github/workflows/ci.yml, .github/workflows/deploy-relay.yml, and .github/workflows/release.yml. These changes do not support the ACP request-ID fix.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ACP agents built on the Kotlin ACP SDK (e.g. Junie) never become ready: request ids start at 2^32

1 participant