[hieu-lee]

Problem

The ACP stdio NDJSON framer (makeStrictNdJsonRpcParser in packages/effect-acp/src/protocol.ts) did buffer += chunk; buffer.split("\n") on every stdout read. That flattens and rescans the whole pending buffer each time, so one large JSON-RPC line (for example a big tool result from Cursor, Grok, or another ACP agent) arriving across many 64 KiB pipe reads costs O(n²) time and memory.

Fix

Scan only the incoming chunk for its last newline. A read without a newline is appended to the buffer (a V8 rope, no scan). When a newline arrives, buffer + chunk.slice(0, lastNewline) is split once and the tail after the last newline becomes the new buffer. Behaviour stays the same: same lines in the same order, the buffer is updated before the strict codec can throw on a malformed line, CR/empty-line handling is unchanged, and UTF-8 decoding still uses the streaming TextDecoder.

The existing protocol test now delivers its two inbound frames split across three reads, with one read ending the first frame and starting the second.

Measurements

Harness: Node 24.18, Apple Silicon. The old and new decode bodies run against the real effect/rpc/RpcSerialization jsonRpc() codec. Input is fed as Uint8Array reads of 64 KiB. After 3 warmup runs, each process takes 7 measured runs. Allocated bytes are heap growth plus what each GC reclaimed during the run (v8.GCProfiler). Each scenario ran in 3 interleaved processes; the table shows the median of the per-process medians.

Scenario	Before	After
One 10 MB tool_call_update line (161 reads)	93.4 ms, 838 MB allocated	8.6 ms, 33 MB allocated
One 1 MB line (17 reads)	1.47 ms, 11.8 MB	0.95 ms, 3.3 MB
20k small agent_message_chunk lines (52 reads)	14.4 ms, 19 MB	15.3 ms, 18-19 MB (same within noise)

What limits the new number: one JSON.parse of the 10 MB line (about 3.9 ms by itself), one rope flatten, and the UTF-8 decode. Framing no longer shows up.

Benchmark script (run from packages/effect-acp with node --expose-gc bench.mjs <old|new> <big|big1|deltas>)

// Usage: node --expose-gc acp-framer.bench.mjs <old|new> <scenario>
// Run from packages/effect-acp so `effect` resolves.
import { createRequire } from "node:module";
const require = createRequire(process.cwd() + "/package.json");
const RpcSerialization = await import(require.resolve("effect/rpc/RpcSerialization"));

const makeOld = () => {
  const codec = RpcSerialization.jsonRpc().makeUnsafe();
  const decoder = new TextDecoder();
  let buffer = "";
  return {
    decode: (bytes) => {
      buffer += typeof bytes === "string" ? bytes : decoder.decode(bytes, { stream: true });
      const lines = buffer.split("\n");
      buffer = lines.pop() ?? "";
      return lines.flatMap((line) => codec.decode(line));
    },
  };
};

const makeNew = () => {
  const codec = RpcSerialization.jsonRpc().makeUnsafe();
  const decoder = new TextDecoder();
  let buffer = "";
  return {
    decode: (bytes) => {
      const chunk = typeof bytes === "string" ? bytes : decoder.decode(bytes, { stream: true });
      const lastNewline = chunk.lastIndexOf("\n");
      if (lastNewline === -1) {
        buffer += chunk;
        return [];
      }
      const lines = (buffer + chunk.slice(0, lastNewline)).split("\n");
      buffer = chunk.slice(lastNewline + 1);
      return lines.flatMap((line) => codec.decode(line));
    },
  };
};

const [impl, scenario] = process.argv.slice(2);
const make = impl === "old" ? makeOld : makeNew;
const enc = new TextEncoder();

const chunkBytes = (bytes, size) => {
  const out = [];
  for (let i = 0; i < bytes.length; i += size) out.push(bytes.subarray(i, i + size));
  return out;
};

const toolResult = (bytes) =>
  JSON.stringify({
    jsonrpc: "2.0",
    method: "session/update",
    params: {
      sessionId: "s1",
      update: {
        sessionUpdate: "tool_call_update",
        toolCallId: "t1",
        status: "completed",
        content: [{ type: "content", content: { type: "text", text: "x".repeat(bytes) } }],
      },
    },
  }) + "\n";

const delta = (i) =>
  JSON.stringify({
    jsonrpc: "2.0",
    method: "session/update",
    params: {
      sessionId: "s1",
      update: {
        sessionUpdate: "agent_message_chunk",
        content: { type: "text", text: `token ${i} ` },
      },
    },
  }) + "\n";

let chunks;
if (scenario === "big") {
  // One 10 MB tool result arriving in 64 KiB pipe reads.
  chunks = chunkBytes(enc.encode(toolResult(10 * 1024 * 1024)), 64 * 1024);
} else if (scenario === "big1") {
  chunks = chunkBytes(enc.encode(toolResult(1024 * 1024)), 64 * 1024);
} else {
  // 20k streaming deltas, packed into 64 KiB reads (lines straddle reads).
  let s = "";
  for (let i = 0; i < 20000; i++) s += delta(i);
  chunks = chunkBytes(enc.encode(s), 64 * 1024);
}

const run = () => {
  const p = make();
  let n = 0;
  for (const c of chunks) n += p.decode(c).length;
  return n;
};

import { GCProfiler } from "node:v8";
// Allocated bytes = heap growth plus whatever each GC reclaimed during the window.
const measureAlloc = (fn) => {
  globalThis.gc();
  const profiler = new GCProfiler();
  profiler.start();
  const before = process.memoryUsage().heapUsed;
  const t0 = performance.now();
  const value = fn();
  const ms = performance.now() - t0;
  const after = process.memoryUsage().heapUsed;
  const { statistics } = profiler.stop();
  let reclaimed = 0;
  for (const gc of statistics) {
    const used = (s) => s.usedHeapSize;
    reclaimed += used(gc.beforeGC.heapStatistics) - used(gc.afterGC.heapStatistics);
  }
  return { ms, bytes: after - before + reclaimed, value };
};
for (let i = 0; i < 3; i++) run();
const times = [];
const allocs = [];
let sink = 0;
for (let i = 0; i < 7; i++) {
  const r = measureAlloc(run);
  sink += r.value;
  times.push(r.ms);
  allocs.push(r.bytes);
}
const med = (a) => [...a].sort((x, y) => x - y)[Math.floor(a.length / 2)];
console.log(
  JSON.stringify({ impl, scenario, chunks: chunks.length, medianMs: +med(times).toFixed(2), medianAllocMB: +(med(allocs) / 1048576).toFixed(1), sink }),
);

Validation

• vp test run packages/effect-acp/src: 64 passed.
• Mutation check: dropping the carried buffer makes the extended test fail.
• vp run check:changed: clean (format, lint, knip, typecheck of 14 packages, tests).
• Differential fuzz, kept out of the repo: 20,000 random inputs through the old and new framers, comparing every read's output and any thrown error. Inputs mixed JSON-RPC lines, empty lines, CR-terminated lines, malformed JSON, multibyte, emoji and U+2028 text, random 1-64 byte read boundaries, and both string and Uint8Array reads. 0 mismatches.

Not checked: a live ACP agent session. This is a pure framing change below the agent protocol.

Authored with Claude Opus 5.5 via the Claude Code harness in T3 Code.

🤖 Generated with Claude Code

[macroscopeapp]

Approvability

Verdict: Approved at e9d21fa

Macroscope's review found this PR approvable — ACP input framing now avoids repeatedly splitting large unfinished lines. Existing strict decoding and connection-error handling remain in place, with no change to product defaults or protocol behavior.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: 9826dbb
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5ffb0a6b-e3fb-4825-b960-2d299fc830c3

Commits

Reviewing files that changed from the base of the PR and between 9826dbb and e9d21fa.

Files selected for processing: 2

• packages/effect-acp/src/protocol.test.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; 9 remain after this review.

Walkthrough

The NDJSON parser now processes complete newline-terminated frames and retains incomplete trailing data. The notification test feeds two encoded frames in chunks that split frames and cross a frame boundary.

Changes: NDJSON decoding

Layer / File(s)	Summary
Decode complete frames packages/effect-acp/src/protocol.ts, packages/effect-acp/src/protocol.test.ts	The decoder processes complete newline-terminated frames and retains the trailing partial line. The test feeds two encoded notifications in chunks that split frames and cross the boundary between frames.

Priority: Low

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

Change: Refactor

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to e9d21

The parser change is mergeable; covered framing scenarios preserve the existing protocol behavior.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title "perf(effect-acp): frame large ACP stdout lines in linear time" matches the main change: the NDJSON framer in protocol.ts now scans only each incoming chunk for its last newline instead of r…
Description check		The description follows the template's main sections: Problem, Change, Verification, and a note on the agent used. The Problem section explains the O(n²) rescanning issue with a concrete reproduction …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "perf(effect-acp): frame large ACP stdout lines in linear time" matches the main change: the NDJSON framer in protocol.ts now scans only each incoming chunk for its last newline instead of rescanning the whole pending buffer, which removes the O(n²) cost for large lines. It uses a conventional commit prefix and names the package and the performance goal. It is specific and concise, and it does not include file lists or vague terms.

Full details: Description check

Explanation

The description follows the template's main sections: Problem, Change, Verification, and a note on the agent used. The Problem section explains the O(n²) rescanning issue with a concrete reproduction scenario. The Change section explains the new chunk-scanning approach and its preserved behaviors. Verification is detailed, with benchmark tables, test counts, a mutation check, differential fuzzing, and a note on what was not checked. The Scope and approval section is missing. The template asks for a linked triaged issue or maintainer-approved discussion, or an explanation of why a small focused fix qualifies without one. The author does not provide either. The description also does not state the CONTRIBUTING.md read-through. These gaps are non-critical relative to the overall completeness of the description, so the check passes, but the author should add the approval or qualification explanation.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[hieu-lee]

@juliusmarminge ready for review. ACP stdout framing is now linear in line size (10 MB line: 93 ms to 8.6 ms, 838 MB to 33 MB allocated; many small deltas unchanged). Differential fuzz and existing tests pass. CI workflows on this fork PR show action_required, so they are waiting on a maintainer to approve the run. CodeRabbit and Macroscope are done with no open findings.

[maria-rcks]

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

Verified head e9d21fab026b0910cdb97ce30e2c29abf127f4aa against github/main at 5732bbee397f0a2b0e61d0c2e432f01dca6df3dc on Linux. Installed both with vp i and ran one real dev server at a time.

The running server used the local ACP provider with official codex-acp 2.5.0, real Codex gpt-6.1-sol, and the official filesystem MCP server. The same prompt read the real httpApiScalar.ts source on both revisions. No mocks or synthetic payloads.

A temporary observational loader timed the real parser's decode calls, including UTF-8 decoding and JSON parsing. The resulting ACP tool-result message was 1,048,617 bytes:

revision	stdout reads	total decode time
main	21	38.37 ms
PR	20	4.59 ms

These are single live observations, not an end-to-end speedup estimate. Both turns completed. Two follow-up turns submitted through the real web composer also completed, including git commands and unicode responses. Native Codex completed an adjacent tool-call check on both revisions.

All 64 existing effect-acp tests passed remotely through blacksmith-check. CI reports 31 successful checks. No code changes were needed.

command transcript and measurement details

Not verified

• The claimed 10 MiB latency/allocation numbers: this real Codex path bounded the tool-result frame to about 1 MiB.
• Native desktop/mobile clients, macOS/Windows, and other ACP providers. Malformed input was covered by existing tests, not injected into a live provider.
• An initial large git log stress run on main streamed many small updates and hit the default Node heap limit. That separate failure was not investigated or fixed. The bounded source-read comparison used the same 8 GiB heap ceiling on both revisions.