Conversation
xterm does not reflow the alternate buffer (or a normal buffer under pre-21376 ConPTY), so after a shrink each line keeps its old length. SerializeAddon walked every non-final row to line.length, so any snapshot taken after a shrink carried stale right-hand cells that wrapped into extra rows on replay; restores repainted that garbage and a differential TUI such as OpenCode never cleared it. Clamp the row walk and the wrap-boundary lookups to the terminal's columns in Orca's addon-serialize source patch, and regenerate the bundles, maps and lockfile hash per docs/reference/xterm-patch-regeneration.md.
Drops the private-terminal casts the casting gate flags; the normal-buffer case now drives a plain pre-21376 ConPTY terminal and its SerializeAddon directly.
…ializing After a non-reflowing shrink a width-2 glyph can have its lead half in the last column and its trailing half past the grid. Serializing the lead half makes the replay wrap it to the next row and shift every row below, so serialize that cell as a blank and keep the row exactly the grid's width. A glyph ending exactly at the edge is unchanged.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe xterm serializer patch changes visible-width cell handling, cursor and row tracking, style output, inverse-styled empty cells, and OSC 8 links. New tests cover snapshots after column shrink, clipped wide glyphs, and trailing background rows. The change also adds a round-trip oracle, differential fuzz tools and tests, a script to build serializer output at a selected git ref, and PTY transcript fixtures with replay validation. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The serializer change is mergeable after normal checks; no actionable issue remains established. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation For [ ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 176066a5-4b8b-4399-a28b-7a2d2d8ed0dc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
config/patches/@xterm__addon-serialize@0.15.0-beta.300.patchconfig/patches/xterm-src/@xterm__addon-serialize@0.15.0-beta.300.src.patchsrc/main/daemon/headless-emulator-shrink-snapshot.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one coverage gap worth a look.
Reviewed changes
This PR clamps every buffer-line length read in Orca's @xterm/addon-serialize source patch to the terminal's current width, so a snapshot taken after a non-reflowing column shrink (the alternate screen always; the normal buffer on pre-21376 ConPTY) describes only the visible grid.
- Width-clamped serialization —
SerializeAddon.tsgainsvisibleCellCount(line, cols)andisClippedWideCell(...); the base serialize loop's per-row end column,_rowEnd's last/second-last cell and_nullCellCountcursor math, and both handler constructors now use the terminal'scols. - Clipped wide glyph — a width-2 cell whose trailing half fell past the grid is serialized as a blank that keeps the row exactly
colswide. - Patch bookkeeping — regenerated compiled bundle/maps and the
pnpm-lock.yamlpatch hash. - Test —
headless-emulator-shrink-snapshot.test.tspins the alt-screen shrink round trip, the normal-buffer non-reflow case, and CJK/emoji cells that straddle the edge versus end exactly at it.
I verified against the installed patched addon that the new suite fails 6/6 on the pre-PR patch (stale . tails wrap into extra rows) and passes 6/6 on this one, that node config/scripts/regenerate-xterm-patches.mjs --check reports the tree in sync, and that a wrapped no-reflow normal buffer round-trips correctly.
ℹ️ Wrapped-line clamp in _rowEnd has no automated guard
The added tests position every row with \x1b[<n>;1H, so nextLine.isWrapped is always false and _rowEnd takes its \r\n branch. The clamp this PR adds to the wrapped branch (thisRowLastChar read at currentLineCells - 1, the _backgroundCell substitution, and the contentCellCount guard on the C/D cursor math) therefore ships untested.
Technical details
# Untested wrapped-line clamp in `_rowEnd`
## Affected sites
- `config/patches/xterm-src/@xterm__addon-serialize@0.15.0-beta.300.src.patch` — `_rowEnd` wrapped branch: `currentLineCells`/`thisRowLastChar` clamp and `contentCellCount`.
- `src/main/daemon/headless-emulator-shrink-snapshot.test.ts` — cases 1 and 2 drive rows with explicit cursor positioning, so `nextLine.isWrapped` is false.
## Required outcome
- A regression guard for the wrapped-line path: a no-reflow shrink of a normal buffer whose content actually wrapped at the old width, serialized and replayed at the narrow width.
- This is the only branch where `currentLine.length - _nullCellCount` could exceed `cols` (the bug the `contentCellCount > 0` guard exists for); nothing currently pins it.
## Open questions for the human
- Is the wrapped branch reachable for the target hosts (pre-21376 ConPTY), or is it considered dead enough to leave uncovered?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
… modules #22452 added src/shared/main-agent-status.ts and src/shared/agent-turn-outcome.ts, which agent-status-types.ts imports, so the session route's closure grew by two local modules (4218 -> 4220). That change was src/shared-only, so its own CI never ran this suite; main has been at 4220 since, and any PR that fires the mobile web app job fails on the stale pin. Measured on 4064653 and on origin/main 3ea15dd.
… replay Seeded VT streams (text, CJK/emoji/combining, SGR, cursor/edit ops, scroll regions, DECAWM/IRM, alt-screen variants, DECSC, and shrink-heavy resizes) drive a source terminal in three modes: reflowing normal buffer, alternate buffer, and a non-reflowing pre-21376 ConPTY normal buffer. At each checkpoint every SerializeAddon build under test serializes it, and each output is replayed into a fresh terminal of the same size and compared cell by cell, plus cursor, active buffer and modes. CI runs 25 seeds per mode against a pinned list of pre-existing divergences, and replays the committed PTY transcripts (the existing agent fixtures plus new vim, less, pico, and OpenCode captures) under four resize schedules. Point ORCA_OLD_SERIALIZE_ADDON at a previous patched build to also check byte identity when no line is wider than the grid, and that no checkpoint regresses.
…t ref config/scripts/build-serialize-addon-at-ref.mjs reverse-applies the patch that produced the installed @xterm/addon-serialize dist, applies the ref's patch, and verifies each step against the patches' blob hashes, so the fuzz can use origin/main (or any fix commit) as its baseline without a second install. ORCA_NEW_SERIALIZE_ADDON swaps in a built dist for the build under test, and a seed-pinned test replays the nine I3 regressions found against origin/main.
The stand-in for a wide glyph clipped by a column shrink came from getNullCell(), whose width is 0. _nextCell skipped it as a wide trailer, and the row-end wrap check counted the width-0 _backgroundCell as content, so a soft wrap after the clipped column was taken as natural and replayed one column early (conpty seed 1149: `abcdefghi中WRAPPED` at 12 -> 10 cols replayed as `abcdefghiR`/`APPED`). Blank the cell in place instead: width 1, no codepoint, its own attributes, so it counts as one empty column and forces the wrap. Differential sweep vs origin/main, 7000 cases per mode: I1 0 byte diffs, seed 1149 fixed; the remaining I3 regressions are the trailing background-row seeds.
…ment The OpenCode transcript carried this machine's lane paths in its footer; replace them with same-length neutral paths so the recorded cursor layout is unchanged.
Without scrollback, _serializeString trims rows after the last content cursor. A row made only of background-colored blanks emits its erase in _rowEnd but never moved that cursor, so two or more such rows at the bottom were dropped (4x3 `r1\r\n\e[48;5;157m\e[J\e[0m\e[3;1H` replayed with the last row blank). Track the erase separately and extend the kept rows to it, except when the cursor is wrap-pending: relative moves back from those rows cannot re-create that state, and doing so regressed normal 508, alt 1425/6647, conpty 4699. Harness: I1 now exempts checkpoints with a background row after the last text row, the one place this fix changes bytes on purpose (scope helpers move to serialize-grid-variant-scope.ts); conpty seed 5 leaves the pinned pre-existing list. Sweep vs origin/main, 7000 cases per mode: I1 0, I3 regressions 0; fixed/both-fail normal 967/3914, alt 2628/8316, conpty 6657/2937 (was 323/4558, 2296/8648, 5466/4128).
… background rows Trailing background-only rows change bytes only when the serialized range has no scrollback (the trimming path) and the cursor is not wrap-pending; checkpoints with scrollback or a wrap-pending cursor are held to byte identity again. The 7000-per-mode sweep against origin/main stays at I1 0 and I3 0.
The serializer now keeps trailing background-only rows, so a painted TUI's screen read as text ends in \r\n\x1b[NX rows. Serialize the same frame with and without them and check the Claude option scrape, the empty-prompt check and the fork transcript read the same thing.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 34b3b79d-23cb-41ae-8b1e-7bfdac5bd9ae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
config/patches/@xterm__addon-serialize@0.15.0-beta.300.patchconfig/patches/xterm-src/@xterm__addon-serialize@0.15.0-beta.300.src.patchconfig/scripts/build-serialize-addon-at-ref.mjssrc/main/daemon/__fixtures__/pty-transcripts/less.meta.jsonsrc/main/daemon/__fixtures__/pty-transcripts/less.txtsrc/main/daemon/__fixtures__/pty-transcripts/nano.meta.jsonsrc/main/daemon/__fixtures__/pty-transcripts/nano.txtsrc/main/daemon/__fixtures__/pty-transcripts/opencode-run.meta.jsonsrc/main/daemon/__fixtures__/pty-transcripts/opencode-run.txtsrc/main/daemon/__fixtures__/pty-transcripts/opencode.meta.jsonsrc/main/daemon/__fixtures__/pty-transcripts/opencode.txtsrc/main/daemon/__fixtures__/pty-transcripts/vim.meta.jsonsrc/main/daemon/__fixtures__/pty-transcripts/vim.txtsrc/main/daemon/serialize-addon-edge-cases.test.tssrc/main/daemon/serialize-grid-cell-descriptors.tssrc/main/daemon/serialize-grid-fuzz-stream.tssrc/main/daemon/serialize-grid-roundtrip.tssrc/main/daemon/serialize-grid-transcript-replay.test.tssrc/main/daemon/serialize-grid-variant-scope.tssrc/main/daemon/serialize-grid.differential.fuzz.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the commits since the prior pullfrog review (ecfdea0a94): two serializer fixes, the wrapping differential test infrastructure, and test-only bookkeeping. I read the full diff (all 23 files), verified the fixes against xterm's CellData/Content layout, and ran the two always-on focused suites — serialize-addon-edge-cases.test.ts (3 tests) and headless-emulator-shrink-snapshot.test.ts (6 tests) — both green.
- Clipped wide glyph now blanks to width 1 —
blankClippedWideCellwritescontent = 1 << 22(width 1, empty codepoint) instead of reusing the buffer's null cell. I confirmed at runtime thatgetNullCell().getWidth()is0in this xterm build and thatWIDTH_SHIFT = 22, so the earlier revision's blank was skipped as a placeholder / read as a wide trailer; the fix is correct. - Trailing background-only rows are no longer trimmed — the serializer records the last
\x1b[<n>Xbackground erase and, in the no-scrollback fixup, advances the content cursor past it unless the cursor is wrap-pending (cursorX < cols). The added regression replays pin the previously-trimmed output on both normal and alternate buffers. - Differential serialize oracle added — seeded VT fuzz (
serialize-grid-fuzz-stream.ts) with an I1/I2/I3 verdict oracle (serialize-grid-roundtrip.ts), cell descriptors, variant scope, a step-drop minimizer, and captured PTY transcript replay under resize schedules. - Baseline builder —
config/scripts/build-serialize-addon-at-ref.mjsreverse-applies the installed patch and applies any git ref's patch with blob-hash checks, so I1/I3 can compare two builds without reinstallingnode_modules. - Wrapped-line clamp guard —
serialize-addon-edge-cases.test.tsreplays the seed-1149 soft wrap, covering the_rowEndwrapped branch the prior review flagged as untested. That feedback is now addressed. - Test bookkeeping — PTY fixtures are scrubbed (no
/Users/, emails, or tokens; cwd neutralized) and the mobile session-closure pin moved 4218 → 4220.
ℹ️ The differential oracle does not run automatically
The strongest check here — I1 byte identity and I3 "no checkpoint regresses" against a prior build — is gated behind it.skipIf(!OLD_ADDON_PATH), and nothing in CI sets ORCA_OLD_SERIALIZE_ADDON. The always-on coverage is the fixed-seed I2 pin (seeds 1-25, installed build only) plus the two edge-case suites, so a regression outside those seeds and not reducible to the pinned cases would land uncaught.
Technical details
# Differential oracle is opt-in
## Affected sites
- `src/main/daemon/serialize-grid.differential.fuzz.test.ts` — the two I1/I3 tests are `it.skipIf(!OLD_ADDON_PATH)`, so they are skipped unless `ORCA_OLD_SERIALIZE_ADDON` is set.
- `config/scripts/build-serialize-addon-at-ref.mjs` — the only documented way to produce that env's value.
- `.github/workflows/*` — no step sets `ORCA_OLD_SERIALIZE_ADDON`.
## Required outcome
- Decide whether the I1/I3 sweep should run in CI (building the `origin/main` addon per run) or stay an on-demand investigative tool.
## Open questions for the human
- Is the fixed-seed I2 pin plus the edge-case replays considered sufficient always-on coverage, with the differential sweep reserved for local/adversarial use?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the two commits pushed after the previous incremental review (15feb5c0b3). Both are test-only refinements of the coverage around the trailing-background-row fix; no production code changed.
- I1 carve-out narrowed —
hasTrailingBackgroundRownow takes the source terminal and returns early unless the serializer's keep gate holds (buffer.length - start <= rowsandcursorX < cols), so the differential oracle asserts byte identity against the old build in every case where the serializer still trims. I verified the gate matches the_rowEndcondition exactly (_firstRow≡start,cursorX < cols). - Reader-agnostic guard added — a renderer test replays a Claude-Code-like alt screen with and without painted background rows and asserts
readClaudeSessionOptionsFromTerminalScreen,agentInputLineCleared, andbuildBoundedSessionTranscriptreturn identical results. I ran it: 1 test, pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
static analysis is red on this head: the repo's anti-slop/no-reflect-get rule rejects the two Reflect.get(terminal, '_core') reads in serialize-grid-roundtrip.ts. This delta is otherwise a clean origin/main merge, so the failure is code the PR added earlier and it is a mechanical fix.
Reviewed changes
This run re-reviewed the commits since the prior pullfrog review (c5ba3a28): a single merge of origin/main, which absorbs main's session-closure pin. Verifying the head surfaced the red check above plus one cross-platform fixture gap.
- Merged
origin/maininto the branch — no product-code change; themobile-web-app-session-terminal-closure.test.mjspin now matches main and drops out of the PR diff.
⚠️ New PTY fixtures are not pinned against line-ending normalization
The five src/main/daemon/__fixtures__/pty-transcripts/*.txt captures carry real CR and CRLF bytes (verified: less.txt, vim.txt, and opencode-run.txt all contain CRLF), and serialize-grid-transcript-replay.test.ts:48 reads them as utf8. A Windows checkout with core.autocrlf=true rewrites those bytes (LF → CRLF, and an existing CRLF → CRCRLF), so the replay tests would diverge from the committed evidence. The repo already guards the older captures against exactly this at .gitattributes:39-42 (/src/main/runtime/__fixtures__/*.txt -text), but the new pty-transcripts/ path is not covered.
Technical details
# PTY transcript fixtures unprotected from CRLF normalization
## Affected sites
- `src/main/daemon/__fixtures__/pty-transcripts/{less,opencode-run,vim}.txt` — hold CR/CRLF exactly as the terminal emitted them
- `src/main/daemon/serialize-grid-transcript-replay.test.ts:48` — `readFileSync(join(dir, file), 'utf8')`, replayed and compared byte-for-byte
- `.gitattributes:39-42` — the existing `-text` pin for `/src/main/runtime/__fixtures__/*.txt`, same rationale
## Required outcome
- A Windows (`core.autocrlf=true`) checkout preserves the transcript bytes, so the replay fixtures stay reproducible across platforms.
## Suggested approach
- Add `/src/main/daemon/__fixtures__/pty-transcripts/*.txt -text` next to the existing transcript rule.ℹ️ Git compatibility failed on an npm-registry ECONNRESET
The other red check, Git compatibility, died in pnpm/setup before any test ran: Error: Could not reach https://registry.npmjs.org/pnpm: fetch failed with ECONNRESET. Environment noise, unrelated to this PR — a re-run should clear it.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ot Reflect.get The fuzz oracle reached xterm's private _core with Reflect.get, which the low-evidence gate rejects. Narrow _core, writeSync and the DECSTBM bounds with in/typeof checks into one named XtermCoreInternals shape instead.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the commit pushed after the prior pullfrog review (4eff4a5a4a), which resolved the red static analysis check.
Reflect.getreplaced with a checked parser — the twoReflect.get(terminal, '_core')reads insrc/main/daemon/serialize-grid-roundtrip.tsare now served byreadXtermCore/readRegion, which narrow_coreviainand validatewriteSyncbefore use. No casts, and the private core is no longer reached through reflection.
Verified locally on this head: anti-slop/no-reflect-get reports 0 errors for the file, pnpm tc:node is clean, and serialize-grid.differential.fuzz.test.ts (the consumer of the refactored helper) passes. The prior Reflect.get thread is resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the single commit pushed after the prior pullfrog review (ccfc5ad580). It closes the last open finding from the 4eff4a5a4a review, with no production-code change.
- PTY transcript fixtures pinned against line-ending normalization —
.gitattributesgains/src/main/daemon/__fixtures__/pty-transcripts/*.txt -text, mirroring the existing/src/main/runtime/__fixtures__/*.txt -textrule so a Windowscore.autocrlf=truecheckout preserves the captured CR/CRLF bytes the replay suites compare against. Verifiedgit check-attr textreportsunsetfor the new fixtures.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
ELI5
When OpenCode's pane gets narrower, xterm still keeps the old, wider rows in memory past the new right edge. When Orca saved a copy of the screen, it saved those hidden leftovers too. Coming back to the worktree redraws the pane from that copy, so the leftovers wrap onto their own rows and the screen looks shredded. OpenCode only redraws cells it thinks changed, so it never cleans them up. This change saves only what fits the current width.
What Changed
Before: open the right sidebar while OpenCode is writing an answer, switch to another worktree, switch back. The pane shows every other row filled with pieces of the old wide screen. It stays like that until the pane gets wider again.
After: the pane comes back exactly as OpenCode drew it.
Mechanism:
@xterm/addon-serializewrote every non-final row out to the row's own length, not toterminal.cols.HeadlessEmulator.getSnapshot, main's model serializer, and the renderer serializer. So a snapshot taken after any narrowing replays stale tails that wrap into junk rows.line.lengthread is clamped tocols. A wide (CJK or emoji) glyph cut in half by the shrink is written as a blank so the row still spans exactlycols. Bundles, maps and the lockfile hash are regenerated perdocs/reference/xterm-patch-regeneration.md;--checkreports them in sync.Why
Linked Issue
Fixes #18178
Visual Proof
N/A for screenshots: in the background-launched harness the WebGL renderer stopped drawing glyphs, so captures show only bands of color. The proof is cell-level instead (see Testing).
Minimal reproduction without Orca (xterm headless + serialize add-on): paint a 135-col alt-screen frame, resize to 48, repaint at 48 without clearing, serialize, replay at 48. On main 4 of 6 rows come back as wrapped
....; with this patch all 6 are correct.Testing
Recipe (by hand, local Mac): close the right sidebar; give OpenCode a prompt with a long answer; while it streams, open the right sidebar; switch to another worktree for ~3 s; switch back.
On main the host model itself matched OpenCode's byte stream exactly (checked against an independent
@xterm/headlessfed a PTY tap). Only the serialized-and-replayed image was wrong. Controls stayed clean on main: OpenCode idle while away (no restore happens), and the sidebar opened after leaving.I manually tested these changes locally (macOS; background-launched dev build driven over CDP)
Automated tests added:
src/main/daemon/headless-emulator-shrink-snapshot.test.ts. It covers:All fail on main's patch and pass here.
pnpm tcandcheck:code-quality:changedare clean, and the xterm contract tests pass. The only failures in the wider daemon/shared/terminal-pane run are therepro-13767real-PTY timeout tests, which fail the same way on main.AI Disclosure
Review
Adversarial review found no regressions. The one hole it confirmed (the wide glyph straddling the edge) is fixed in ecfdea0.
Agent skill upstream boundary
Notes
Checklist
N/Awith reason abovepnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (typecheck and targeted tests local; CI will cover the rest)