fix: update ACP SDK large frame handling - #1496
Conversation
📝 WalkthroughWalkthroughThe ACP backend now points to a newer SDK pseudo-version, increases the default inbound notification queue capacity, and adds tests covering unset, invalid, and bounded ChangesACP transport update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
Claude finished @carlosflorencio's task in 47s —— View job Review Todos
FindingsNo blockers or suggestions. This PR is a pure dependency pin bump:
There's no new or changed logic in this repository to test — the actual fix (chunked large-line handling in the JSON-RPC scanner) lives in the upstream fork ( Summary
Verdict: Ready to merge |
|
OpenCode review complete OpenCode model: No suggestions found for commit |
|
| Filename | Overview |
|---|---|
| apps/backend/go.mod | Bumps the ACP SDK fork replace directive to the commit containing chunked large-line JSON-RPC reads; straightforward dependency update. |
| apps/backend/go.sum | Updated checksums for the new ACP SDK fork commit; no concerns. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go | Raises acpNotifQueueDefault from 16,384 to 131,072 (matching acpNotifQueueMax); the struct-level comment for notifQueue still references the old "1024-slot" SDK default which is now stale. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_queue_test.go | New test file covering all branches of acpNotifQueueCapacity(); logic and assertions are correct. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as ACP Agent
participant SDK as ACP Go SDK (chunked reader)
participant NQ as SDK Inbound Queue (131072 slots)
participant IQ as notifQueue (4096 slots)
participant W as Update Worker
participant App as Kandev App
Agent->>SDK: Large JSON-RPC line during session/load replay
Note over SDK: Chunked read assembles full line safely
SDK->>NQ: Enqueue notification (non-blocking)
Note over NQ: Raised from 16384 to 131072 slots
NQ->>IQ: Update handler enqueues (nanosecond)
IQ->>W: Worker drains in FIFO order
W->>App: sendUpdate(AgentEvent)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent as ACP Agent
participant SDK as ACP Go SDK (chunked reader)
participant NQ as SDK Inbound Queue (131072 slots)
participant IQ as notifQueue (4096 slots)
participant W as Update Worker
participant App as Kandev App
Agent->>SDK: Large JSON-RPC line during session/load replay
Note over SDK: Chunked read assembles full line safely
SDK->>NQ: Enqueue notification (non-blocking)
Note over NQ: Raised from 16384 to 131072 slots
NQ->>IQ: Update handler enqueues (nanosecond)
IQ->>W: Worker drains in FIFO order
W->>App: sendUpdate(AgentEvent)
Reviews (3): Last reviewed commit: "fix: update ACP SDK large frame handling" | Re-trigger Greptile
115a01c to
7048dd5
Compare
|
Claude finished @carlosflorencio's task in 1m 54s —— View job Review Todos
SummaryPosted a findings comment per the code-review skill. No blockers — 1 minor (non-blocking) suggestion about Verdict: Ready to merge |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
FindingsReviewed No blockers. Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge The dependency bump pulls in the upstream chunked large-line fix ( |
7048dd5 to
2fc7d84
Compare
|
Claude finished @carlosflorencio's task in 2m 0s —— View job I'll analyze this and get back to you. |
| if got := acpNotifQueueCapacity(); got != acpNotifQueueDefault { | ||
| t.Fatalf("default queue capacity = %d, want %d", got, acpNotifQueueDefault) | ||
| } | ||
| if got := acpNotifQueueDefault; got != acpNotifQueueMax { |
There was a problem hiding this comment.
OpenCode: Brittle constant-comparison assertion in 'default uses max capacity' subtest
Lines 12-14 assert acpNotifQueueDefault == acpNotifQueueMax, which is a compile-time constant comparison rather than a behavior check. If either constant is intentionally changed (e.g., a future maintainer raises acpNotifQueueMax to 262144 to give operators headroom, or lowers the default to 65536 once a hot path is profiled), this test will fail even though acpNotifQueueCapacity() still returns the correct default. The first assertion (lines 9-11) already covers the actual contract — the function returns acpNotifQueueDefault when the env is unset. Drop the second if got := acpNotifQueueDefault; got != acpNotifQueueMax block, or move the relationship to a code-level invariant (e.g., a var _ = ... line or a comment) so it doesn't trip the test suite.
| } | ||
| }) | ||
|
|
||
| t.Run("in-range env is honored", func(t *testing.T) { |
There was a problem hiding this comment.
OpenCode: 'in-range env is honored' subtest only exercises a single mid-range value
The subtest at lines 37-43 only tests KANDEV_ACP_NOTIF_QUEUE=4096, which coincidentally equals the unrelated notifQueueCapacity constant (the internal buffered channel) defined in adapter.go:59. A reader skimming the test could misread 4096 as a special value tied to the internal channel. Add one or two additional in-range cases (e.g., 8192, 32768, and a value just below acpNotifQueueMax like 131071) to (a) make the test resilient to the chosen number and (b) verify the function honors any value in [acpNotifQueueMin, acpNotifQueueMax], not just the one picked. The 'env is clamped' subtest already covers both ends, so the gap is specifically the middle of the range.
| // thousands of notifications before the response, so default to the configured | ||
| // ceiling instead of requiring operators to know about KANDEV_ACP_NOTIF_QUEUE. | ||
| // The SDK still bounds memory to (capacity * avg notification size). | ||
| const acpNotifQueueDefault = 131072 |
There was a problem hiding this comment.
OpenCode: 8x default queue size increase has no operator-facing observability or log on first use
acpNotifQueueDefault jumped from 16384 to 131072 (8x). The Initialize call at line 347-352 already emits a debug log with the resolved capacity, which is good, but the new default equals acpNotifQueueMax — meaning operators who hit memory pressure now have no env-var headroom to reduce the buffer (clamping prevents going below acpNotifQueueMin=1024, and the default already sits at the top of the range). Consider either (a) raising acpNotifQueueMax to a higher ceiling (e.g., 262144) so operators can opt into a larger queue when truly needed, or (b) lowering the default back toward the middle of the range (e.g., 32768 or 65536) and keeping the max as a knob for the rare long-replay case. The comment at lines 61-65 acknowledges the memory bound but does not surface the new default's parity with the max, which is a meaningful behavioral change worth flagging in the constant's doc comment.
FindingsReviewed the full diff for this PR ( Scope: SDK fork bump ( Tests:
This closes the gap flagged earlier by the automated OpenCode review (missing in-range and non-positive coverage). No blockers found:
Note: the unrelated Summary
Verdict: Ready to merge |
1 participant
Long ACP session resumes can emit JSON-RPC lines larger than the SDK scanner limit and notification bursts larger than Kandev’s previous inbound queue default, causing the peer to disconnect before
session/loadcompletes. Updating to the fixed Kandev ACP SDK fork and raising the default ACP notification queue lets long sessions resume without requiring a special environment override.Important Changes
KANDEV_ACP_NOTIF_QUEUEas a clamped override.Validation
go test ./...in/root/.kandev/tasks/acp-go-sdk-fixgo test ./internal/agentctl/server/adapter/transport/acpmake -C apps/backend testSDK PR: kdlbs/acp-go-sdk#1
Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.