Skip to content

fix(types): make TerminalSnapshot.cursorVisible required, document DECSTR limit - #38

Merged
ThomasK33 merged 2 commits into
mainfrom
fix/snapshot-followups-36
Oct 30, 2026
Merged

fix(types): make TerminalSnapshot.cursorVisible required, document DECSTR limit#38
ThomasK33 merged 2 commits into
mainfrom
fix/snapshot-followups-36

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Oct 29, 2026

Copy link
Copy Markdown
Member

Closes #36

Why

Snapshots from the native binding always carry cursorVisible, but the type marked it optional, so every reader had to handle undefined. Separately, a DECSTR soft reset (ESC[!p) leaves the cursor hidden, and nothing said so.

What changed

  • TerminalSnapshot.cursorVisible is now required (boolean), in src/types.ts and the src/README.md contract.
  • The README says the pinned Ghostty ignores DECSTR and that RIS (ESC c) still resets. A test pins that behavior.

Scope

Tradeoffs

Blast Radius

Code that builds its own TerminalSnapshot must now set cursorVisible. coder/agent-tty does this in assertNativeSnapshot and will get TS2741 when it upgrades. That is the line where it currently drops the field. Readers are unaffected.

Verification

  • npm run build, typecheck, test (68 passed) and smoke pass locally.
  • I packed main and this branch and compiled a strict consumer against each. const v: boolean = snapshot().cursorVisible fails with TS2322 on main and compiles here. A TerminalSnapshot literal without the field compiles on main and fails with TS2741 here.
  • Through the package, ESC[?25l ESC[7m ESC[!p leaves cursorVisible: false and inverse: true. A following ESC c gives cursorVisible: true and no attributes.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33
fix(types): make TerminalSnapshot.cursorVisible required
2c784f5 
The native binding always sets it, and no published release has the
optional form, so the type now says what callers get.

_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_

Change-Id: I19f990d6a9dedbcf1f7ee49d4f1658768bcc37e1
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33
docs: document that the pinned Ghostty ignores DECSTR
f082b91 
A test pins the current behavior, so moving the Ghostty pin past the
upstream DECSTR commit (#37) fails it and flags the README note.

_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_

Change-Id: Idd55a075938efaf6faaff08c4cd144947a2dae07
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33 ThomasK33 self-assigned this Oct 29, 2026
@ThomasK33

ThomasK33 commented Oct 29, 2026

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

ThomasK33 commented Oct 29, 2026

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T23:03:38.063628Z f082b91 Manual request
🔒 Security Review ✅ Completed 2026-10-29T23:04:10.297798Z f082b91 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: f082b91571

ℹ️ 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".

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: f082b91571

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33

ThomasK33 commented Oct 29, 2026

Copy link
Copy Markdown
Member Author

PASS

Independent verification of PR #38 by a verifier that did not write the code. I built and packed each commit with the verify-libghostty-vt-node skill (launch.sh, then doctor.sh exit 0 on all three), installed the tarball into a scratch consumer, and drove it by package name.

  • Head: f082b9157129a612fa6cff5bd9d36fe3f0a5517c
  • Parent (merge-base with main): e9e20d581d2dca3f6785f6793f372150b67a2a5a
  • Merge with main: local merge commit 6d5ba7ee7540e9e9f5ee7712848a131416ff6398 = main 8e3d303017a7bfb5bcec0c64326dd2d3385dea1c + head. It merged with no conflicts. I did not push it.
  • Toolchain: Node v26.12.1 (also under npm exec), tsc Version 5.9.3, @types/node 25.9.8. Ghostty pin 48ccec182a932c2ec04c344d45a5fc553861cb13 on all three.
Claim Parent e9e20d5 Head f082b91 Merge 6d5ba7e
1a. const v: boolean = term.snapshot().cursorVisible under --strict exit 2, TS2322 ('boolean | undefined' is not assignable to type 'boolean') exit 0 exit 0
1b. Hand-built TerminalSnapshot without cursorVisible exit 0 exit 2, TS2741 (Property 'cursorVisible' is missing) exit 2, TS2741
1c. Control: hand-built snapshot with cursorVisible: true exit 0 exit 0 exit 0
1d. TerminalSnapshot["cursorVisible"] is exactly boolean exit 2, TS2322 exit 0 exit 0
1e. Packed dist/types.d.ts cursorVisible?: boolean; cursorVisible: boolean; cursorVisible: boolean;
2. Runtime: true by default in 3 snapshot views, false after ESC[?25l, true after ESC[?25h pass pass pass
3a. After DECSTR, a hidden cursor stays hidden pass pass pass
3b. After DECSTR, SGR 1, 2, 3, 4, 7, 8, 9 and colors 31/42 stay set pass pass pass
3c. RIS shows the cursor and clears every SGR attribute and color, also right after DECSTR pass pass pass
3d. Packed README carries the DECSTR paragraph absent present present
4. vitest ignores the DECSTR soft reset with the pinned Ghostty not present, 67 passed ✓, 68 passed ✓, 69 passed
5. src/README.md lists cursorVisible cursorVisible?; cursorVisible; cursorVisible;

Each typecheck ran tsc --noEmit --strict --module nodenext --moduleResolution nodenext --types node --typeRoots <checkout>/node_modules/@types <file>.mts inside the consumer, against the installed package.

The runtime journey (11 checks) gave the same output on parent, head and merge, check by check. So the runtime did not change. npm run build:native, npm run typecheck and npx vitest run exited 0 on all three. vitest skipped 0 tests.

The new test detects a reset. I replaced ESC[!p with ESC c in the test input on a scratch copy. The test then failed with expected true to be false. I restored the file. The test comment names Ghostty f473f31 ("terminal: support DECSTR"). GitHub's compare API reports it 2119 commits ahead of the pin.

Edge case. DECSTR prints nothing, also when a write splits it. ESC[?25l ESC[7m ESC[!p X leaves visible text "X\n", one cell, and the cursor at row 0, col 1. Splitting the sequence across two feed() calls (ESC[!, then pX) gives the same text and keeps cursorVisible: false and inverse: true on X. Splitting it mid-CSI across four Uint8Array chunks also gives "X\n" and cursorVisible: false. Parent, head and merge agree.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: xhigh

@ThomasK33
ThomasK33 merged commit 74f3bc6 into main Oct 30, 2026
5 checks passed
@ThomasK33 ThomasK33 mentioned this pull request Oct 30, 2026
Merged
ThomasK33 added a commit that referenced this pull request Oct 30, 2026
@ThomasK33
Release v0.1.0-beta.2 (#44)
35a554a 
## Why
Ship the changes merged since `0.1.0-beta.1` to npm. They include mode-aware mouse encoding (#12), the new snapshot attributes and `cursorVisible` (#17, #38), and Ghostty `v1.4.0` with DECSTR support (#43).

## What changed
- `package.json` and `package-lock.json` go from `0.1.0-beta.1` to `0.1.0-beta.2`. The branch name matches the version, so `publish.yml` creates `v0.1.0-beta.2` after merge.
- The `release-changelog` workflow adds the CHANGELOG entry to this branch.
- `communique.toml` raises `max_tokens` to 16384. With Communique 1.0.3's default of 4096, generating these notes failed twice with "submit_release_notes was malformed 3 times". The next run succeeded.
- `prepare-release-changelog.mjs` now detects Communique's `## [vX](url) - date` heading. Before, every push regenerated the entry over hand edits.
- I corrected the generated entry. `cursorVisible` and `encodeMouse` first ship in this release, so they are listed as Added, not as changes or fixes.

## Blast Radius
The merge publishes `0.1.0-beta.2` to npm under the `beta` dist-tag. `latest` stays on `0.1.0-beta.0`. Consumers that build a `TerminalSnapshot` themselves must now set `cursorVisible` (#38).

## Verification
- `RELEASE_BRANCH_NAME=release/v0.1.0-beta.2 node .github/scripts/prepare-release-changelog.mjs origin/main` prints `should_generate=true` and `tag_name=v0.1.0-beta.2`. `resolve-npm-publish-tag.mjs` prints `beta`.
- `npm run typecheck` and `npm test` (73 passed).
- Three independent verifier passes, the last PASS+NOTES on the head. They check every CHANGELOG bullet against the `v0.1.0-beta.1` tag and confirm that the packed tarball loads its addon from `prebuilds/`.
- The local linux-x64 prebuild steps from `mise run release-prebuilds` pass: `build:libghostty`, `build`, `build:prebuild`, `verify:prebuilds` and `npm pack --dry-run`.

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ThomasK33 ThomasK33

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Snapshot follow-ups from #17: required cursorVisible, DECSTR soft reset

1 participant

@ThomasK33