Skip to content

feat: record and replay a PTY stream, so a redraw glitch becomes a test - #379

Open
kshivang wants to merge 1 commit into
masterfrom
feat/pty-replay-harness
Open

feat: record and replay a PTY stream, so a redraw glitch becomes a test#379
kshivang wants to merge 1 commit into
masterfrom
feat/pty-replay-harness

Conversation

@kshivang

Copy link
Copy Markdown
Owner

Why

Chasing the intermittent redraw glitch from a screenshot didn't work. I formed three theories about the terminal's wrap and batch logic from one image and disproved all three by reading the code afterwards. A picture shows that something is wrong, not what.

The byte stream is different evidence: it's deterministic. The same bytes must always produce the same grid. So a recording turns "sometimes the wrapped line garbles" into a failing test.

What the capture showed

Every frame of Claude Code's spinner is bracketed in DEC 2026 and rewrites only the characters that changed, using absolute column addressing and no line clear:

CSI 35 C   "th"   CSI 39 G   "nking more with xhigh ef"   CSI 64 G   "ort"

still thinking with xhigh effortthinking more with xhigh effort, rewriting cols 36-37, skipping col 38 (ii), resuming at 39.

It is diffing against its own model of our grid. One cell of disagreement anywhere becomes permanent visible corruption, because nothing ever repaints the line to resync — and it surfaces later, somewhere unrelated to where it started. That is why it looks intermittent, and why the wrap is where it shows rather than necessarily where it's caused.

What's here

1. Recording. BOSSTERM_PTY_LOG=1 starts per-tab capture (or set it to a directory). startFileLogging already existed and had no callers — the facility was there and unreachable, the same shape of dead flag as setEnabled was in this class.

2. A lossless escaper. A literal backslash was written through unescaped, so \e was ambiguous between an escape character and the two characters \ and e. Readable, but not replayable — any Windows path or regex in the output would have replayed as a control sequence. DEL is escaped too.

3. PtyReplay (test sources) parses a recording and feeds it through a real BossEmulator headlessly, returning the grid. Chunk boundaries are preserved rather than concatenated, because a CSI split across two reads is exactly what a recording should be able to re-expose.

How to use it

BOSSTERM_PTY_LOG=1 ./gradlew :bossterm-app:run --no-daemon
# reproduce the glitch, then grab ~/.bossterm/pty-log/<stamp>-<tab>.log

Drop the log in as a fixture, assert the grid, and the glitch is a deterministic regression test.

Verification

The round-trip test goes through the production writeChunkToFile rather than a reimplementation, so the two halves can't drift apart silently. Removing the backslash escape fails it — mutation-checked.

Writing the differential-redraw test also produced a correction worth keeping: I expected "thinking more" and got "thinking moreg". The emulator is right — without a trailing clear the previous frame's tail survives, which is precisely the bug class here. Both behaviours are now pinned, with and without the CSI K the real capture ends on.

Test-only additions plus two small production changes (env wiring, escaper). Full suite green in both modules.

Not claimed

This does not fix the glitch. It makes the next occurrence diagnosable instead of speculative.

Chasing an intermittent redraw glitch from a screenshot does not work. I tried it
this week: I formed three theories about the terminal's wrap and batch logic from
one image, and disproved all three by reading the code afterwards. A picture shows
that something is wrong, not what.

The byte stream is a different kind of evidence. It is deterministic: the same
bytes must always produce the same grid. So a recording turns "sometimes the
wrapped line garbles" into a failing test.

This matters here specifically because of HOW the affected apps redraw. A capture
of Claude Code's spinner shows every frame bracketed in DEC 2026, rewriting only
the characters that changed, with absolute column addressing and no line clear:

  CSI 35 C  "th"  CSI 39 G  "nking more with xhigh ef"  CSI 64 G  "ort"

It is diffing against its own model of our grid. One cell of disagreement anywhere
becomes permanent visible corruption, because nothing repaints the line to resync,
and it surfaces later somewhere unrelated to where it started. Replaying a
recording and comparing grids is how that gets localised to the chunk that caused
it.

Three parts:

1. BOSSTERM_PTY_LOG=1 starts per-tab recording (or set it to a directory).
   startFileLogging already existed and had NO callers - the facility was there
   and unreachable, the same shape of dead flag as setEnabled was in this class.

2. The escaper is now lossless. A literal backslash was written through unescaped,
   so "\e" in output was ambiguous between an escape character and the two
   characters \ and e - readable, but not replayable. Any Windows path or regex in
   the output would have replayed as a control sequence. DEL is escaped too.

3. PtyReplay (test sources) parses a recording and feeds it through a real
   BossEmulator headlessly, returning the grid. Chunk boundaries are preserved
   rather than concatenated, because a CSI split across two reads is exactly the
   kind of thing a recording should be able to re-expose.

The round-trip test goes through the production writeChunkToFile rather than a
reimplementation of it, so the two halves cannot drift apart silently; removing
the backslash escape fails it.

Writing the differential-redraw test also produced a small correction worth
keeping: I expected "thinking more" and got "thinking moreg". The emulator is
right - without a trailing clear the previous frame's tail survives, which is
precisely the bug class here. Both behaviours are now pinned, with and without the
CSI K the real capture ends on.
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review: record and replay a PTY stream (1/3 — the blocking one)

The framing is right, and the framing is the valuable part: a byte stream is deterministic evidence and a screenshot isn't, so turning a recording into a fixture is exactly the right move for a differential-redraw bug. Three things worth calling out as genuinely good before the findings:

  • The round-trip test goes through the production writeChunkToFile instead of a reimplementation. That's the difference between a test that pins behaviour and a test that pins a copy of it.
  • chunkBoundariesDoNotSplitAControlSequence asserts whole == split rather than a hardcoded grid, so it stays true as the emulator evolves.
  • "thinking moreg" being pinned rather than fixed is the correct call, and the comment explaining why is the kind of thing that stops a future reader from "fixing" it.

Findings across three comments, most consequential first.


1. The recording is not lossless for non-ASCII — FileWriter uses the platform default charset

DebugDataCollector.kt:435

fileLogWriter = PrintWriter(FileWriter(file, false), true)

FileWriter(File, Boolean) encodes with the JVM default charset. Every module here is jvmToolchain(17), so JEP 400 (UTF-8 by default) does not apply — on JDK 17 the default comes from the platform locale. The reader side is file.readText(), which is unconditionally UTF-8 (readText(charset = Charsets.UTF_8)).

So the two halves disagree wherever the platform default isn't UTF-8:

  • Windows (windows-1252): the box-drawing and spinner glyphs in the test's own payload are written as ?.
  • Linux/CI with LANG unset or LANG=C (JDK 17 → ANSI_X3.4-1968): same, and aRecordingRoundTripsThroughTheProductionEscaper should fail on its own unicode payload there. If the suite is only ever run under a UTF-8 locale, that's a green test guarding a broken invariant.

This isn't hypothetical for the bug being chased: Claude Code's spinner glyphs and box-drawing are precisely the non-ASCII characters that get destroyed, and a replayed grid full of ? looks like an emulator bug. That's the failure mode PtyReplayTest's own header warns is the worst possible one for a debugging aid.

fileLogWriter = PrintWriter(FileWriter(file, Charsets.UTF_8, false), true)

(JDK 11+ overload. Worth doing the same at :516 in exportToFile while you're there.) Then add a locale-independent assertion — read the bytes back and compare against the UTF-8 encoding — so the round-trip test can't pass by locale accident.

2. The recording captures the bytes but not the geometry

replay(chunks, width, height) makes the caller supply the grid size, and nothing in the log says what it was. For a wrap glitch — which is what this PR exists to chase — the column count is the single most load-bearing variable, so a fixture whose width is guessed can silently fail to reproduce, or reproduce a different bug. Worse, a mid-session resize changes wrap behaviour and doesn't appear in the stream at all, so any recording spanning a window drag is unreplayable in principle.

Two small additions would close it: write cols x rows into the header startFileLogging already emits, and record a resize marker when the PTY is resized. parseLog can then return the initial geometry alongside the chunks, and replay can apply resizes at the right chunk boundary.

@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review 2/3 — wiring, lifecycle, and the log itself

3. The third collector construction site is missed

DebugDataCollector is constructed in three places in TabController, and only two got the hook:

line function hooked
558 createTab yes
945 createSessionForSplit yes
1192 createTabWithPreConnect no

No in-repo caller today (it's the embedder-facing SSH pre-connect API), so impact is low — but the failure is silent: BOSSTERM_PTY_LOG=1 produces no file for those tabs and nothing says why. Given the repo's taste for drift-proofing registries (AttachTargetCoverageTest), the durable fix is a single private factory in TabController that all three paths call, so a fourth path can't miss it either.

4. Two tabs created in the same second overwrite each other's recording

val safe = tabLabel.replace(Regex("[^A-Za-z0-9._-]"), "_").take(40)
val stamp = SimpleDateFormat("yyyyMMdd-HHmmss").format(Date())

createTab's tabId defaults to null and the UI path doesn't pass one, so tabLabel is the literal "tab" for essentially every tab. With a second-granularity stamp, any two tabs created in the same second (session restore, or Ctrl+T twice) resolve to the same path. startFileLogging opens with FileWriter(file, false), which truncates — and the first tab's PrintWriter stays open and keeps writing at its own offset, so you get one file interleaving two sessions with a NUL-padded hole. A corrupt fixture that looks exactly like corrupt terminal output.

An AtomicInteger sequence in the name (or millis + counter) fixes it. Separately: since the label is almost always "tab", consider deriving the name from something that actually distinguishes tabs.

5. BOSSTERM_PTY_LOG=0 turns logging on

Anything that isn't 1/true is treated as a directory path, so BOSSTERM_PTY_LOG=0, false, off, no all start recording into a relative directory named ./0, ./false, … The idiomatic reading of FOO=0 is "off". Worth handling the falsey set explicitly, and requiring an absolute path (or resolving against user.home) so a typo can't scatter directories into the CWD.

6. Failures are swallowed with no diagnostic

runCatching { startFileLogging("$dir/$stamp-$safe.log") }

A read-only directory, a bad path, a full disk — all indistinguishable from "the env var didn't take". For a diagnostic facility the one thing you can't afford is being undiagnosable. Add .onFailure { System.err.println("WARN: PTY log …") }, and print the resolved path on success so the user doesn't have to reverse-engineer it from the KDoc.

7. Nothing ever stops the logging — one leaked FD per closed tab

stopFileLogging() has no callers outside startFileLogging's own reset and the new tests. TerminalTab.dispose() (TerminalTab.kt:481) tears down listeners, the write channel, the scope and the display, but never the collector's writer — so with logging on, every closed tab leaks an open PrintWriter and the === Log Ended === footer is never written. runCatching { debugCollector?.stopFileLogging() } in dispose() covers it.

Related: the log is unbounded. chunks/snapshots are ring-buffered precisely because unbounded growth is a known hazard in this class, but the file has no cap — a yes loop or a chatty TUI left overnight fills the disk. A size cap that stops and says so is enough; it doesn't need rotation.

8. The recording is a credential capture, written with the default umask

USR< records user input verbatim, so anything typed at a prompt — including a password at a sudo/ssh prompt, which is unechoed and therefore only visible in the recording — lands in a plaintext file, alongside every token and API key that crossed the PTY in the output. ~/.bossterm/pty-log/*.log is created with the default umask, world-readable on most systems.

The repo already treats this class of file carefully — voice.json is deliberately chmod 600 — and the reasoning applies more strongly here. Suggest Files.setPosixFilePermissions(…, rw-------) on creation (guarded for Windows), plus a sentence in the KDoc and AGENTS.md saying out loud that a recording is secret-bearing. Given the intended workflow is literally "drop the log in as a fixture", that warning earns its keep: a fixture committed to the repo is a secret committed to the repo.

@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review 3/3 — smaller things, and the verdict

Dangling KDoc. The new doc block was inserted between startFileLogging's existing KDoc and the function it documented. startFileLogging now has no doc comment, and its @param filePath / @throws IOException block sits above startFileLoggingIfRequested documenting nothing. Move the new function and its doc below startFileLogging.

The tag strings are duplicated and can drift. writeChunkToFile maps ChunkSource"PTY>", PtyReplay.LINE re-encodes the same four tags in a regex, and "PTY>" is hardcoded a third time in replay's filter. Add a fifth ChunkSource and the parser silently drops those lines. One internal mapping in DebugDataCollector used by both sides, plus Chunk.source: ChunkSource instead of String, makes the drift impossible rather than merely unlikely — the same shape as the shouldCaptureState extraction this file already argues for.

startFileLoggingIfRequested has no test. It's the one piece of new production logic, and it's untestable as written because it reads System.getenv and formats the path inline. Extracting internal fun resolveLogPath(envValue: String?, tabLabel: String, now: Date): String? would let you pin "1" → default dir, "true", an explicit dir, blank → null, the falsey set from #5, and the sanitizer (../../etc/x.._.._etc_x, take(40)). That's the argument the file already makes for shouldCaptureState: "a pure function so that mistake is testable rather than only visible by hand."

Replay fidelity gaps. Production does terminal.setCharacterEncoding(settings.characterEncoding) and BossEmulator(dataStream, terminal, settings.allowKittyFileTransfers); PtyReplay uses the 2-arg constructor with default encoding, and builds a TerminalTextBuffer with no history configuration. A glitch that depends on encoding, Kitty transfers, or scroll-into-history may therefore not reproduce. Not blocking — but since the harness's whole value is "replay is faithful", these are worth either wiring through or stating as known limits in the KDoc. (The batch wiring itself matches TabController:683 exactly — good.)

trimEnd() hides trailing-space divergence. Stale trailing spaces are a real differential-redraw artifact — they're what a missing CSI K leaves when the new frame is shorter and the old tail was blank — and trimEnd() makes them invisible to any assertion. The "moreg" case only caught it because the leftover happened to be a g. Consider returning untrimmed rows and letting callers trim, or offering both.

Last chunk's trailing partial grapheme is dropped. BlockingTerminalDataStream.append buffers an incomplete grapheme until the next append, and close() doesn't flush it — so if a recording's final chunk ends mid-grapheme, replay silently loses those characters. Harmless in most fixtures, confusing in exactly the one where it matters.

Autoflush on the PTY reader thread. PrintWriter(…, autoFlush = true) means a flush per chunk inside synchronized(this) on the reader thread. It's an opt-in debug path so the cost is fine — but it perturbs the timing of the intermittent bug you're trying to catch, which is worth a KDoc line so a future reader doesn't lose a day to "it stops happening when I record it."

Style nit. private companion object landed in the middle of the property declarations; convention puts it at the end of the class body.

Things I checked that are fine: the line regex can't be confused by payload content (all newlines are escaped, so one chunk is exactly one line); unescape's \xNN substring can't go out of bounds, since the i == s.lastIndex guard means i + 2 <= length; the chunk-then-sentinel ordering in close() is FIFO-safe with a single consumer, so replay is deterministic rather than racy; and setEnabled still has no callers, so enabled stays true and recording isn't accidentally gated behind debug mode.


Verdict: the direction is right and the tests are well-chosen. #1 is the one I'd treat as blocking — it silently falsifies the PR's central "lossless / replays byte-for-byte" claim on Windows and under a non-UTF-8 CI locale, and it fails in the most misleading way possible. #2#4 are cheap and materially affect whether a captured fixture is usable at all. The rest can land as follow-ups.

Reviewed statically — I couldn't run :compose-ui:desktopTest in this environment, so the charset failure in #1 is reasoned from the toolchain and API contracts rather than observed. Running the round-trip test under LANG=C should confirm it in seconds.

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