fix(acp): bound unread raw notification retention#19585

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: e6e2ab0
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: a1f4ba89-f021-44e1-be4a-7de5d2d11825

Reviewing files that changed from the base of the PR and between 32b32d4bab222150026deade08599649dc1adddb and 5d1fa0afa30651e8fced9e143bab094f9860ef42.

• packages/effect-acp/src/protocol.test.ts
• packages/effect-acp/src/protocol.ts

Security Blast Radius

• inferred — A connected ACP peer controls notification content and can influence memory retained by its protocol instance. The PR reduces that instance's unread raw backlog without introducing another peer entrypoint or broader authority. Many connections or individually oversized notifications can still exceed the nominal character budget in aggregate.

Security Findings and Attack Paths

• observed — The remaining ability to supply an arbitrarily large notification or accumulate an unterminated parser line predates this PR. The base already retained large decoded notifications by count and accumulated incomplete input without a line-size cap; these are residual exposure facts, not introduced findings.

Trust Boundaries and Controls

• observed — Line-length metadata affects retention only. Notification schema decoding and request-method routing remain in place, and typed handlers receive notifications independently of raw eviction. The cited public agent change is documentation; the cited protocol-test range exercises termination rather than creating a production entrypoint.

Explanation

The title "fix(acp): bound unread raw notification retention" names the main change: the raw ACP notification backlog is now capped by entries and character budget. It follows the conventional commit style the template requests. It is specific and matches the changeset, which bounds the unread raw queue in protocol.ts and documents the behavior in agent.ts and client.ts.

Explanation

The description follows the template's main sections: Problem, Change, Scope and approval, and Verification. The Problem section states the bug and its memory impact. The Change section explains the bounded backlog, the retained newest entry, and the end-of-connection behavior. Scope and approval links issue #18799 and explains why the change is focused. Verification lists the tests, typecheck, lint, and synthetic memory measurements, and it states that the measurements are not from a live provider session. The template asks for a triaged issue or maintainer approval comment. The description links the issue but does not include an explicit approval comment. The objectives note that maintainer comments exist, but they do not show that approval was given for this scope. The template also asks for the model and harness used, and the description includes both. The sections are complete overall. The missing explicit approval reference is a minor gap.

Explanation

Active direct target: [#18799]. Its coding requirements are: (1) cap unread raw notifications at 32 entries; (2) cap unread JSON line text at about 16 MiB; (3) keep the newest notification even when it alone exceeds the budget; (4) deliver every notification to a reader that keeps up; (5) keep typed handling intact. Evidence at the reviewed head, packages/effect-acp/src/protocol.ts: the parser returns each decoded message with lineLength: line.length (UTF-16 code units). bufferRawNotification offers to the rawNotifications queue, then evicts oldest entries while size > 32, or while size > 1 and the tracked length > 16,777,216. The newest entry is never evicted. dispatchNotification still calls the typed onNotification path after buffering. Termination and the scope finalizer call Queue.end, so the stream drains and completes. The unit choice (UTF-16 code units, not heap bytes) matches the PR statement that this is not a strict heap ceiling. Test coverage is taken from the change summary and the prior assessment; the test file was not re-read in this review. The implementation requirements are met.

Explanation

All changes support [#18799]. protocol.ts adds the size-aware raw buffer, line-length metadata, and queue termination. The parser now applies normalizeAcpJsonRpcError inside decode; this is a move of existing logic, and the logged and routed messages are unchanged. The incoming decoded-message log now checks options.logIncoming before calling logProtocol; logProtocol already returned early for incoming events when that flag was off, so behavior is unchanged. The agent.ts and client.ts edits only document the raw-stream contract. The test additions cover the requested behavior. No provider capability, wire schema, or client-flow change appears in the reviewed diff.

• Create a new PR