Repository navigation
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped ACP interoperability fix that changes only generated request-ID values, with shared constant usage and targeted tests covering sequencing and response correlation. Existing routing, extension IDs, schemas, and deployment behavior remain unchanged. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOutgoing ACP RPC request IDs now start at ChangesACP RPC request IDs
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established; the change is ready for normal merge checks. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The incremental diff since the previous review adds changes unrelated to issue
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Dismissing prior approval to re-evaluate 268ef9a
Fixes #15263.
Problem
effect-acpstarts typed Effect RPC request ids at2 ** 32in bothAcpClientandAcpAgent. 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 inRequestId.IntId(a KotlinInt) and decodes them withtoInt(), so4294967296fails to decode. The SDK treats the frame as malformed. Older builds may stay silent; current source answers with an error whoseidisnull, which T3 cannot correlate and drops. Either way,initializenever resolves and the provider status probe times out after 60 s ("did not resolve and create a test session"). ACP'sRequestIdisint64, so T3 is within spec; the break is at the signed 32-bit limit.Change
One exported constant,
RPC_REQUEST_ID_START = 2 ** 30inprotocol.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-acponly; no contract, client or adapter change. Every ACP provider shares this transport, so every one benefits, and agents that handleint64ids 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 asLongand echoing a recoverable id there would help other clients too.Verification
Observed result.
AcpClient.makeinitialized and created a session through the repository ACP mock behind a gate script that mimics the Kotlin SDK by silently dropping numeric request ids above2147483647. 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 frommainat 00eb8f6 byte for byte.initialize4294967296, dropped; 10 s timeout in 3/3 runs1073741824, forwarded; success in 0.294–0.326 s, 3/3 runssession/new1073741825; success by 0.300–0.330 s from driver start, 3/3 runsThe gate models the unanswered call, not the current SDK's exact
id: nullerror frame, which T3 drops the same way.Tests. All 73
effect-acptests pass. The client and agent tests assert the first typed id2 ** 30, the next2 ** 30 + 1, extension id1, and that each reply reaches its caller. Agent-side incoming ids0,2 ** 30and2 ** 32are answered while an outbound request is pending, including the same id in opposite directions. With the old constant, both counter tests fail on4294967296versus1073741824. The ACP provider tests and theAcpAdapterV2andAcpRegistryAdapterV2suites pass (542 tests, 4 skipped). Package and server typecheck, targeted lint, format and knip are clean; lockfile unchanged. Two existingAntigravityAdapterV2file-system tests fail identically with either constant (a macOS/varvs/private/vartemp-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.