Fix ESC k title payload rendering#176
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
cec12d2 to
793c828
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 793c828e24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
793c828 to
2d6d616
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d6d616414
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Change-Id: Ifcd1e3a10786423d8d08284f44d11433440b68b1 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
2d6d616 to
8f2d24d
Compare
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
Summary
Fixes #153.
ESC k <text> ESC \/ESC k <text> BEL) into Ghostty's ignore-string parser state so title payload bytes are consumed instead of rendered.Uint8Arraywrites, and split writes across parser boundaries.Validation
bun run fmtbun run lintbun run typecheckbun testbun run buildfmt,lint,type check,test, andbuildall passed on commitcec12d2051.Dogfooding
gh-imagerequires aGH_SESSION_TOKENor browseruser_sessioncookie.📋 Implementation Plan
Implementation Plan: #153
Issue summary
GitHub issue
coder/ghostty-web#153is open and accepted with labelstriage:done,bug, andaccepted.Problem: the GNU screen/tmux title sequence
ESC k <text> ESC \\is currently parsed as an unimplemented one-byteESC kaction. The parser returns to ground state, so<text>is printed into the visible terminal grid, while the trailingESC \\is consumed as a no-op string terminator.Expected behavior for the issue reproduction:
Actual behavior today:
Verified context and constraints
ESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text #153 was read withgh issue view 153 --repo coder/ghostty-web --commentsand--json.onTitleChangeis explicitly optional/bonus and should not be added for this minimal fix.lib/terminal.tswrites data tothis.wasmTerm!.write(data), which enters the Ghostty Zig terminal stream;ghostty/src/terminal/stream.zigwarns for unimplementedESC kafter the parser dispatches it as an ordinary ESC action.scripts/build-wasm.shthroughpatches/ghostty-wasm-api.patch; the implementation should update that patch rather than leaving direct edits in the submodule.ghostty-vt.wasmis currently ignored, not tracked (git ls-files --error-unmatch ghostty-vt.wasmreturns no match;git check-ignore ghostty-vt.wasmmatches). A local WASM rebuild is required for validation, but the generated rootghostty-vt.wasmshould not be committed unless repository policy changes.diegosouzapw/ghostty-web@31ed228fixed the symptom with a TypeScript pre-filter and also included an unrelated renderer change. Do not port the renderer change. Do not use a stateless TypeScript pre-filter: it is fragile for split writes and duplicates parser responsibility in JS.Recommended approach
Fix this in the Ghostty parser state table by making
ESC kenter an ignore string state instead of dispatching as a normal ESC action.Use
State.dcs_ignoreas the sink state:ESCmoves the parser from.groundto.escape.kin.escapemoves to.dcs_ignorewith.noneaction..dcs_ignorewith no print/dispatch action.ESCinside the ignore string uses the existing anywhere transition to.escape.\\dispatches as ST/no-op and returns to.ground.This preserves parser state across separate
term.write()calls, avoids TypeScript allocations/scanning, and keeps the fix close to the terminal parser behavior that caused the bug.Files and symbols to touch
Required
patches/ghostty-wasm-api.patchAdd a patch hunk for
ghostty/src/terminal/parse_table.ziginsidegenTable()'s// escapestate block.The minimal hunk should override the existing
range(&result, 0x60, 0x7E, source, .ground, .esc_dispatch);entry for byte0x6B('k') by adding a latersingle(...)entry:single()overwrites prior entries in the generated table, so placing this after the broad0x60...0x7Erange is sufficient and keeps the change small. If a reviewer prefers explicit non-overlap, split the broad range into0x60...0x6Aand0x6C...0x7Einstead of relying on overwrite behavior.lib/terminal.test.tscreateIsolatedTerminalfromlib/test-helpers.ts.term.wasmTerm?.getLine(y)andtrimEnd()s it.Optional only if it materially improves review confidence
ghostty/src/terminal/Parser.zig, verifyingESC kenters.dcs_ignoreandESC \\returns to.ground.Implementation phases and quality gates
Phase 1 — Add parser patch
Update
patches/ghostty-wasm-api.patchwith theparse_table.zighunk described above.Run a patch-only check:
If the patch does not apply, inspect current
ghostty/src/terminal/parse_table.zigcontext and adjust only the hunk context, not the behavior.Quality gate: patch applies cleanly to the submodule.
Phase 2 — Rebuild local WASM for validation
Ensure Zig is available. If
zigis missing but mise has the expected toolchain, use:PATH="$HOME/.local/share/mise/installs/zig/0.15.2/bin:$PATH" bun run build:wasmOtherwise:
Confirm
scripts/build-wasm.shapplies the patch, buildsghostty/zig-out/bin/ghostty-vt.wasm, copiesghostty-vt.wasmto the repo root, then reverses the patch and removes generated submodule files.Check that the submodule is clean after the build:
Quality gate: WASM build succeeds and
git -C ghostty status --shortis empty.Phase 3 — Add regression tests
Add focused tests in
lib/terminal.test.tsfor the visible-grid contract, without requiring title events.Minimum cases:
Issue reproduction, string write
Write:
'\x1bk/tmp\x1b\\\r\n\x1bkls\x1b\\demo.txt\r\n'Assert row 0 is
''and row 1 is'demo.txt'.Issue reproduction,
Uint8Arraywritenew TextEncoder().encode(...)for the same bytes.''and row 1 is'demo.txt'.Split/chunked writes
Split across parser boundaries, for example:
Assert
/tmpdoes not appear and visible content after the terminator still appears normally.Use the repository's existing write-settling/render-settling pattern if needed before reading rows, so the regression does not become flaky.
Adjacent behavior stays intact
onTitleChangetests. Do not add a requirement thatESC kfiresonTitleChange; that is outside the accepted minimal scope.Quality gate: the new regression tests fail before the parser patch and pass after rebuilding WASM with the parser patch.
Phase 4 — Automated validation
Run validation progressively:
If
bun testhangs after reporting results, capture the pass/fail summary before terminating, because this repository has a known Bun completion-hang gotcha.Quality gate: all required checks pass, or any environmental blocker is reported with exact command output and the narrowest passing subset.
Dogfooding and reviewable evidence
Because this is terminal UI/interactive behavior, dogfood with browser evidence in addition to automated tests.
Setup
Start the dev server with Vite, not a plain static server:
Open
http://localhost:8000/demo/with browser automation. Prefer theagent-browserskill for repeatable interaction.If the main demo requires the PTY server for input, start it separately:
Exercise the bug sequence
Use whichever path is least invasive and does not require committing demo changes. Prefer direct terminal writes over typing shell commands, because shell echo can make screenshots ambiguous:
Best: from browser DevTools/agent-browser console, write directly to the terminal instance if it is exposed.
If the terminal instance is not exposed, use a throwaway local page/script or temporary console hook during dogfooding only; do not commit demo-only exposure changes.
Fallback: use the PTY shell only if direct writes are unavailable:
Expected visible result: no
/tmporlstitle payload appears in the terminal grid; only intended visible text such asdemo.txtappears after the title sequence terminator.Evidence to capture
unimplemented ESC action: ESC kwarning during the dogfood run, if logs are available.attach_file/GitHub upload tooling as appropriate.Acceptance criteria
ESC k <text> ESC \\payload is consumed and never rendered as visible grid text.''and row 1'demo.txt'for both string andUint8Arraywrites.term.write()calls without leaking title payload.unimplemented ESC action: ESC kfor the fixed sequence.onTitleChangetests continue to pass.lib/terminal.ts.bun run fmt && bun run lint && bun run typecheck && bun test && bun run build.Explicit non-goals
onTitleChangepropagation forESC kin this issue.Out-of-scope issue protocol during implementation
If a separate bug, flaky behavior, missing feature, or nice-to-have is discovered while implementing:
ESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text #153 patch.ESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text #153 or the current PR when relevant.ESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text #153, and label itneeds-triage.ESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text #153 implementation scope.Advisor review status
Approved by advisor. Non-blocking advisor refinements were incorporated: use existing test settling patterns to avoid flake, prefer direct
term.write(...)dogfooding over shell echo for clear screenshots, and note the optional explicit range-split alternative if reviewers dislike relying on table overwrite behavior.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh