Skip to content

fix(buffer): retain output by line and enforce buffer limits - #70

Open
dhaern wants to merge 1 commit into
shekohex:mainfrom
dhaern:omos/fix-pty-buffer-lines
Open

dhaern wants to merge 1 commit into
shekohex:mainfrom
dhaern:omos/fix-pty-buffer-lines

Conversation

@dhaern

@dhaern dhaern commented Oct 5, 2026

Copy link
Copy Markdown

Summary

RingBuffer splits the whole buffer string every time its length, a read or a search is needed, never reads the documented PTY_MAX_BUFFER_LINES, cuts the oldest line in the middle when it trims, and reports UTF-16 units as bytes. This PR stores output as lines and makes both documented limits work.

In plain terms: the plugin keeps the latest output of each terminal so the agent can read it back. That storage redid a lot of work for every new chunk of output, which slowed down commands that print a lot, and one of the two documented size limits was never applied. Now it keeps output line by line, which is much faster, and both limits work as the README describes.

Problems fixed

  • Cost. length, read() and search() each run split('\n') over the full buffer, and the exit notification looks for its last line with one read() per line. Feeding 16 MiB in 4,096 character chunks and reading length after each chunk takes 1,616 ms on main (median of three runs) and 16 ms here.
  • Limits. PTY_MAX_BUFFER_LINES is documented in the README and in the pty_read description, but nothing reads it. PTY_MAX_BUFFER_SIZE is read but not documented.
  • Trimming. The size limit keeps the last N characters, so the oldest retained line is usually cut in the middle. With a 10 character budget, appending line1\nline2\nline3\nline4 left ine3\nline4; it now leaves line4.
  • byteLength returns buffer.length, which counts UTF-16 code units: 3 for á😀, whose UTF-8 size is 6.
  • Negative offsets. OutputManager.read with an offset below zero returned that offset and hasMore: true on a ten line buffer, and the next page hint that search prints was built from the unclamped value.

Changes

  • Output is kept as an array of lines, so counting lines is constant time. Discarded lines are trimmed in one batch, and reads use the native slice.
  • Both limits apply, and whole lines are discarded when either one is exceeded. Only a single line larger than the size limit is cut, and it keeps its tail. A missing, invalid or non-positive limit falls back to the default (1,000,000 characters, 50,000 lines).
  • byteLength counts UTF-8 bytes.
  • Read and search clamp negative offsets, including the next page hint that search prints.
  • The exit notification finds the last non-empty line with a single read.
  • RingBuffer.flush() was an exported no-op. It is removed together with its only caller.
  • The README documents PTY_MAX_BUFFER_SIZE and how the two limits interact, and the pty_read description names both.

Behavior changes

  • With the default limits, a session that prints many short lines now stops at 50,000 lines. That limit fits under the character limit only if lines average 20 characters or fewer, newline included. Before, only the character limit applied.
  • A line that alone exceeds the size limit is kept as its tail while it is the newest line. If a blank line follows it, the long line is dropped and only the blank line remains.
  • refactor: shorten tool descriptions to focus on when-to-use instead of how-to-use #49 shortens src/plugin/pty/tools/read.txt. This PR edits the line of that file that describes the limits, so whichever of the two lands second needs a small manual merge.

Validation

Four new tests and one rewritten test cover the line limit, whole line trimming, the tail of an oversized line, UTF-8 byte length and negative offsets. All of them fail on main except the oversized line test, which passes on both and guards the new trimming. Removing the offset clamp makes its test fail again.

bun test over test/*.test.ts, without the live and npm-pack suites, gives 188 passing. bun run typecheck, bun run lint and bunx biome format . are clean. I did not run the Playwright e2e suite locally. The only e2e assertion that compares byteLength with the raw length uses ASCII output, where both values still match.

Benchmark script
// bun bench.ts /absolute/path/to/src/plugin/pty/buffer.ts
const { RingBuffer } = await import(process.argv[2]!)
const chunk = `${'x'.repeat(127)}\n`.repeat(32)
const totalChars = 16 * 1024 * 1024
const runsMs: number[] = []
let sink = 0
for (let run = 0; run < 3; run++) {
  const buffer = new RingBuffer()
  const start = performance.now()
  for (let written = 0; written < totalChars; written += chunk.length) {
    buffer.append(chunk)
    sink += buffer.length
  }
  runsMs.push(performance.now() - start)
}
console.log(runsMs, sink)

Runs on main: 1,684 / 1,616 / 1,495 ms. Runs here: 18.9 / 16.1 / 15.2 ms.

Diff

Production: -13 lines (+43/-56) across buffer.ts, notification-manager.ts, output-manager.ts, session-lifecycle.ts and tools/read.ts.
Tests: +37 lines.
Docs: +3 lines (README.md, tools/read.txt).

Store retained output as lines so counts are constant-time and reads and
searches avoid splitting the full buffer. Trim discarded lines in one batch
and find the exit-notification tail with a single read. Normalize negative
read and search offsets, including the tool's next-page hint.

Intentional contract changes: honor PTY_MAX_BUFFER_LINES alongside
PTY_MAX_BUFFER_SIZE, retaining whole lines until either limit is reached;
only a single oversized line is sliced to its tail. Report byteLength as
UTF-8 bytes instead of UTF-16 code units. Remove the exported no-op
RingBuffer.flush(). Invalid or non-positive limits fall back to the
existing defaults.

An oversized final line that ends with a newline stays visible to pty_read
and exit notifications. Reads use native slice pagination, and the
line-density explanation lives in the README rather than the tool prompt.
@dhaern dhaern changed the title fix(pty): retain output by line and enforce buffer limits fix(buffer): retain output by line and enforce buffer limits Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant