Repository navigation
Conversation
An output event with data "" passes the event schema, but ghostty-web's
terminal.write('') throws RangeError: offset is out of bounds. A replay
that met one failed with it. The harness page now drops empty chunks in
write() and writeBatch(): the write callback goes to the last chunk that
is written, and a flush of empty chunks writes nothing.
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
An output event with empty data changes no pixel, but a timed replay still gave it a place on the schedule and put its empty chunk in a flush. Its timestamp opened a flush window, ended a flush before an input, a marker, an exit or a resize, and moved the max-speed split points, so a WebM export of a log with such an event had other frames than the export of the log without it. The replay now skips the event: it counts as applied for lastSeq and the progress reports, and it has no other effect. The page tests now cover an empty chunk in the first call of a flush above 1000 chunks, the painted-grapheme note of a flush of empty chunks, and the missing frame wait. The seeded twin test scrolls.
# Conflicts: # src/renderer/ghosttyWeb/README.md
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
An output event with
data""passes the event schema (OutputEventPayloadSchemaisdata: z.string()), the event log codec andEventLog.append. Two things go wrong when a replay meets one.ghostty-web throws. The page calls
terminal.write(''), and ghostty-web 0.4.0 throwsRangeError: offset is out of bounds. Onorigin/mainthese commands fail on a log with such an event:origin/mainscreenshotRPC_ERROR(running), exit 10REPLAY_ERROR(stopped)--renderer ghostty-web snapshotRPC_ERROR(running), exit 10REPLAY_ERROR(stopped)record export --format webmEXPORT_ERRORThe default
snapshotandwaituse libghostty-vt, which already takes an empty chunk. They use ghostty-web only when libghostty-vt is unavailable.record export --format asciicastuses no renderer, and--format svgreplays through libghostty-vt. Both exited 0 onorigin/mainin my run.The timed replay gives the event a place on the schedule. After the page stops throwing,
replayWithTiming(the WebM export) still treats the empty event like any output. Its timestamp opens a flush window. In recorded and accelerated mode it forces a flush and a frame hold before the next input, marker, exit or resize. In max-speed mode it moves the split points. The video of a log with an empty event then differs from the video of the same log without it (more states, longer video).A session that agent-tty records itself never holds such an event today:
RunCompletionCoordinator, and its threeappendOutputcalls all checklength > 0.RunCompletionSentinelScanner.feed('')returns no pieces.net.SocketwithsetEncoding, and Node drops a decoded chunk that is empty. I wrote the bytesE2,82,ACof one character in three PTY writes, an empty write, and a lone lead byte (python3script on a real session). The log held one€event and no empty event.pty.onDatadata without that check, so an older log can only hold an empty event if node-pty emitted one. I did not find a way to make it do that. I did not test the Windows ConPTY reader.So the logs that hit this are hand-made, from another tool, or from an older host. They are valid under the schema, so the renderer has to take them. I kept the log format and the validation as they are. Rejecting these logs breaks every existing log of this shape, and the renderer is the only part that cannot take the value.
What
src/renderer/ghosttyWeb/embeddedHarnessHtml.ts: the page drops an empty chunk before it reachesterminal.write. This is the only place that callsterminal.write, so every caller of the page is covered, includingreplayToand the host's live render.writeBatchdoes not hold an empty chunk. The write callback (which resolves the call one frame later) goes to the last chunk that is written, so a flush that ends with an empty chunk still resolves. A call that ends a flush of only empty chunks writes nothing and waits for no frame, and its painted-grapheme note still runs.writereturns for an empty string. Nothing in the backend callswriteany more (GhosttyWebBackend.writeBridgehas no caller), but it has the same defect in the same file.src/renderer/ghosttyWeb/backend.ts:replayWithTimingtakes an output event with empty data out of the recording. The loop checks seq order, then skips the event before it reads the timestamp, so the event has no place on the schedule, opens and ends no flush, and adds nothing to the recorded span that sets the max-speed split distance. The replay is the replay of the log without the event.highestProcessedSeq, solastSeqand every later progress report reach its seq. Nothing else reports for it, because it does no work.replayTohas no schedule, so it is unchanged. It sends the empty chunks to the page, which drops them.src/renderer/ghosttyWeb/README.md: the page sentences (after thediscardHeldChunks()sentence), the timed replay sentence and the timestamp assertion line.src/renderer/ghosttyWeb/harness.htmlis not changed. No code reads it (it is not incopy-renderer-assets.mjs, and the backend servesEMBEDDED_HARNESS_HTMLat the/harness.htmlURL). It has nowriteBatch, so it was already out of step.Merge with main
Commit
074b3a31merges main9b26e448(it holds #365 and #367) into this branch. Onlysrc/renderer/ghosttyWeb/README.mdconflicted: #365 added a line about the one viewport read next to the lines this branch changes, and the merge keeps both. On the merge commit, format check, lint, typecheck and the unit tests pass (149 files, 2,432 tests), andrenderer-backend.test.tspasses 47 of 47 with--maxWorkers=1 --retry=0. Three unit tests (doctor>returns unique passing checks across environment and renderer groups,lineDiff>handles a huge one-sided middle without allocating the DP table,registerBuiltinPresets>registers agent-tty-smoke and makes repeated registration a no-op) hit the 5 s test timeout at host load 218 and passed alone at load 51.Validation
Unit tests (
ghosttyWebBackend.test.ts, 31 new): for each of 9 placements of an empty output event and each of the 3 timing modes (27 tests), the steps and times of the replay (flushes, resizes, notes, final read, in a fake-timer replay with a fake page) equal those of the same log without the event. The placements are before an input, before a resize, before an exit, between two outputs of one flush, between two outputs of two flushes, before the first output, after the last output, far after the last output (it spreads the frame budget over its time) and before each of 20 inputs. Four more tests pin the accelerated and max-speed repros,lastSeqwith the progress reports, and a log of only empty events.Integration tests (
renderer-backend.test.ts, describeempty output chunks, 11 tests, all through the real renderer):replayTowith an empty chunk in the middle, at the start, as the last chunk, as every chunk, as the only chunk of the call that ends a flush of 1001 chunks, and inside the first call of such a flush.write('').requestAnimationFramein the page, expects the empty flush to settle, and expects a flush that wrote a chunk to stay pending.replayWithTimingwith empty events around two resizes.Mutation of
backend.ts(vitest -t "output events that have empty data", 31 tests):highestProcessedSeqlastSeqand progress)Mutation of
embeddedHarnessHtml.ts(vitest -t "empty output chunks", 11 tests, archive copy):origin/mainfile)flush.length > 0check)as every chunk, note, no frame)writecheckdirect page write)length > 1)chunk.length > 0 || !endsFlush)inside the first call of a flush of 1001 chunks)notes the painted graphemes)without waiting for a frame)Real CLI on isolated
AGENT_TTY_HOMEfolders, on a log that a real host wrote. A host-only preload (NODE_OPTIONS=--import) wrapsEventLog.prototype.append, so the host appends one""output event after each output event and two beforeexit, through the validated writer. The log holds 9 records, 5 of them"". The same Home and session served both trees. Commands on a stopped session unless noted:origin/mainsnapshot --json(libghostty-vt)--renderer ghostty-web snapshot --format textREPLAY_ERRORscreenshot --jsonREPLAY_ERRORrecord export --format webmEXPORT_ERROR--renderer ghostty-web snapshotRPC_ERRORRangeErrorscreenshotRPC_ERRORRangeErrorOn this branch the
--renderer ghostty-web snapshottext (without theSession IDandCaptured At Seqlines) and the screenshot SHA-256cbd557008eea0f327920bea5809e4145cef0e46e9877295a07e2de2e6f19d905equal those of a control session with the same program and no empty events. The running session gave the same SHA-256.Real CLI WebM export of a crafted stopped session (a copy of an exited real session with a new id and a log of controlled timestamps, read by the CLI through the log codec), each cell 5 alternating runs at host load 50 to 80, before this change (
54ec1153) and after it. Frames are the distinct frames thatffmpeg mpdecimatekeeps, and the video length is the median:All exports exited 0. The numbers are noisy at this load. The point is that the with-event runs match the without-event runs only after this change.
Gates:
npm run format:check,npm run lint,npm run typecheck,npx vitest run test/unit(149 files, 2432 tests, all passed),check.sh --drift-onlypasses. Integration and e2e files that reach the page (--maxWorkers=1 --retry=0, one file per run):renderer-backend(44 of 44, with the 11 empty-chunk tests),wait-render(31),record-export(17),ghostty-web-crash(4),empty-session-capture(11),host-renderer-rpc(9), e2eexport-fixtures(5),screenshot-out(9),screenshot-missing-glyphs(1),screen-hash(6),cross-backend-screen-hash(6),slow-replay(5),backend-selection(7),record-diff(10),artifact-manifest-race(3),batch(11),mouse(15),run(15),event-log(5),cli(77),lifecycle(73), and the other 9 e2e files (batch-fixture,hello-prompt,large-snapshot,libghostty-vt-renderer,renderer-errors,renderer-slice,resize-demo,scrollback-demo,unicode-grid). All passed.Two files passed only on a second run of the file:
backend-selection(3 tests:defaults semantic snapshot rendering to libghostty-vt when available,does not leak automatic renderer defaults into PTY environments,threads --renderer ghostty-web through live snapshot RPC paths) andcli(5 tests: the twodoctortests,uses --home instead of AGENT_TTY_HOMEand the twoconfig.jsontests). The first run was at host load 40 to 85, and each failure was a test timeout (STACK_TRACE_ERROR, 5.3 to 13 s against the 5 s limit), never an assertion about output. Both files passed alone (7 of 7 and 77 of 77).Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh