fix(tui): a copied NUL byte no longer panics the TUI on Windows - #876
Conversation
syscall.StringToUTF16 PANICS rather than returning an error when the string contains a NUL — documented standard-library behaviour — and github.com/atotto/clipboard's Windows path calls it directly. So one NUL anywhere in a copied transcript selection killed the whole program: "zero: tui error: program was killed: program experienced a panic", reported from a real Windows machine. The NUL reaches the clipboard because ansi.Strip removes escape SEQUENCES and leaves C0 bytes untouched, and the transcript carries them from two independent directions: tool output (a binary read, git's -z NUL-separated listings — which this repo itself parses in files_git_sweep.go) and Zero's own card protocol, whose row prefixes are literally "\x00command-card\x00" and "\x00plan-card\x00". Sanitised at the WRITE rather than at each source, because the sources are unbounded: any tool result a user can select from is one, and a fix per source is a fix the next tool re-breaks. The same pass protects the OSC52 fallback, where a stray control byte terminates the escape sequence early and silently corrupts the copy instead of crashing — which is likely why this went unnoticed off Windows, where the identical NUL produces a bad copy rather than a dead process. \n and \t survive: a multi-line selection is the normal case and both are legal clipboard content. Fixes Gitlawb#875
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughClipboard selections now remove unsafe control characters before native clipboard and OSC52 writes. New tests cover NUL bytes from transcript sources and verify that readable content, newlines, and tabs remain intact. ChangesClipboard safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. Good diagnosis and the right place to fix it.
I checked the structural question, which is whether anything can still reach a clipboard write unsanitised: three callers of copyTranscriptSelectionCmd (composer.go:253, model.go:4741, transcript_selection.go:1458), both write sites inside it, and the sanitise happens before the closure so the OSC52 fallback is covered too. Nothing bypasses it. Stripping at the write rather than per source is the correct call, and your reasoning for it is the argument I would have made.
One nit, and it is the tests rather than the fix. They exercise clipboardSafeText directly, so they pin the helper and not the wiring. I deleted the text = clipboardSafeText(text) line at the call site and both tests still passed, which means a later refactor can drop it and reintroduce the panic with CI green. Worth a seam that lets a test drive copyTranscriptSelectionCmd and assert no NUL reaches the native write, even something as small as taking the clipboard writer as a parameter.
Not blocking, since I verified the wiring by hand this time. But this exact shape has bitten us three times recently, most memorably #866, which shipped as a complete no-op with six green tests because they all called the helper instead of the path.
Minor question rather than a request: \r is stripped along with the other C0 bytes. On Windows the clipboard convention is CRLF, so if any transcript row carries CRLF a paste will arrive LF-only. Probably irrelevant since the rows look LF-native, but you are closer to that code than me.
|
@jatmn this one needs a third approval and it is small. It is a crash fix: a copied NUL takes the whole TUI down on Windows, because I approved it, and I checked the coverage question by hand rather than reading the diff alone: three callers of One nit is already on the PR so you do not spend time rediscovering it: the tests exercise |
…compactions Two blocking review findings from the draft review. Control bytes reached the terminal. redact() scrubbed secrets but not control characters, and the title and structural fields (name, toolCallId, role) skipped it entirely, so an imported title or message carrying ESC or NUL forged a picker row or corrupted a transcript line — the #835/#876 class, on strictly more attacker-influenced input. redact() now composes a stripControl pass (C0 except tab/newline, DEL, C1), and every rendered string — including the title at the import chokepoint — routes through control-stripping. The activity summary was emitted as EventCompaction, whose payload it did not satisfy. RehydrateEvents restructures the transcript around the last EventCompaction; a summary with no CompactableEvents/CompactedThroughSequence is hoisted to the front of the transcript on resume. It is now an assistant EventMessage, which still passes promptContextEvents (the resume digest) but carries none of that replay-side contract. A payload marker keeps it distinguishable from a translated turn, so filters and the digest can tell a Zero-generated summary from the foreign transcript. Also: the import-tag comment now matches ImportTag's actual output. Tests: regression coverage for both fixes, mutation-checked (removing the control strip surfaces the surviving byte; the summary type is asserted not to be EventCompaction). Existing tests updated for the new summary shape via a shared NoteEventIsSummary marker rather than the old EventCompaction type check.
…compactions Two blocking review findings from the draft review. Control bytes reached the terminal. redact() scrubbed secrets but not control characters, and the title and structural fields (name, toolCallId, role) skipped it entirely, so an imported title or message carrying ESC or NUL forged a picker row or corrupted a transcript line — the #835/#876 class, on strictly more attacker-influenced input. redact() now composes a stripControl pass (C0 except tab/newline, DEL, C1), and every rendered string — including the title at the import chokepoint — routes through control-stripping. The activity summary was emitted as EventCompaction, whose payload it did not satisfy. RehydrateEvents restructures the transcript around the last EventCompaction; a summary with no CompactableEvents/CompactedThroughSequence is hoisted to the front of the transcript on resume. It is now an assistant EventMessage, which still passes promptContextEvents (the resume digest) but carries none of that replay-side contract. A payload marker keeps it distinguishable from a translated turn, so filters and the digest can tell a Zero-generated summary from the foreign transcript. Also: the import-tag comment now matches ImportTag's actual output. Tests: regression coverage for both fixes, mutation-checked (removing the control strip surfaces the surviving byte; the summary type is asserted not to be EventCompaction). Existing tests updated for the new summary shape via a shared NoteEventIsSummary marker rather than the old EventCompaction type check.
…compactions Two blocking review findings from the draft review. Control bytes reached the terminal. redact() scrubbed secrets but not control characters, and the title and structural fields (name, toolCallId, role) skipped it entirely, so an imported title or message carrying ESC or NUL forged a picker row or corrupted a transcript line — the #835/#876 class, on strictly more attacker-influenced input. redact() now composes a stripControl pass (C0 except tab/newline, DEL, C1), and every rendered string — including the title at the import chokepoint — routes through control-stripping. The activity summary was emitted as EventCompaction, whose payload it did not satisfy. RehydrateEvents restructures the transcript around the last EventCompaction; a summary with no CompactableEvents/CompactedThroughSequence is hoisted to the front of the transcript on resume. It is now an assistant EventMessage, which still passes promptContextEvents (the resume digest) but carries none of that replay-side contract. A payload marker keeps it distinguishable from a translated turn, so filters and the digest can tell a Zero-generated summary from the foreign transcript. Also: the import-tag comment now matches ImportTag's actual output. Tests: regression coverage for both fixes, mutation-checked (removing the control strip surfaces the surviving byte; the summary type is asserted not to be EventCompaction). Existing tests updated for the new summary shape via a shared NoteEventIsSummary marker rather than the old EventCompaction type check. Origin-Session: local-13d543 | Claude Code | 2 prompts Origin-Snapshot: a939509c08a8
…oreign title cannot repaint the picker All four raised by CodeRabbit on ad57dd3. Two are real defects in this PR's own feature, not test issues. ## The feature was invisible to the user it exists for newSessionPicker gave up on an EMPTY local history: metas, err := m.sessionStore.ListResumable() if err != nil || len(metas) == 0 { return nil } Foreign sessions are discovered independently of the store, so the person with no Zero sessions at all — someone who just installed it and wants to carry on work another agent started — got nil before discovery ran. The import path was reachable only after they had already done by hand the thing it exists to save them. A failed read is still a reason to give up; an empty one is not. Emptiness is now decided after combining both sources, in pickerFromParts, split out so that decision is testable without a session store on disk. ## A foreign title reached the terminal unfiltered registry.go strips control bytes when a session is IMPORTED, and its comment names this picker row as the reason (#835/#876). But the picker lists a session BEFORE anything is imported, reading the title straight out of the other agent's transcript — so the vector that comment describes was the one path the stripping did not cover. An escape repaints the rows above, a carriage return hides the rest of the label, a NUL can truncate the row. sanitizePickerLabel drops control bytes and keeps the printable text: a title that is merely unusual must stay readable, because the row is how the user recognises their own work. ## The live-store test printed the developer's own sessions t.Errorf("incomplete index entry: %+v", session) That walks the REAL store, so a failure put the user's session titles, working directories and file paths into the test output and into any log or pasted report carrying it. It now names which fields are empty, which is the whole diagnostic — the missing value is by definition not the interesting part. NOT taken from the same comment: gating the live-store tests behind an explicit opt-in. @Vasanthdev2004 asked for the opposite in the review this branch is answering — a skip "would also stop it finding anything, so I would rather have the fixture" — and the tests now only report. The data leak was the substantive half and it is fixed. ## The symlink test asserted the weaker half of its property It checked only that Discover and Read AGREE, which passes in two opposite worlds: both correctly refusing a path reached through a symlink, and both happily following it out of the store. Containment is now asserted directly — "sneaky" must not be listed and must not be readable — and agreement is kept afterwards, since that is what the original fast path broke. Not taken, out of scope: three findings in internal/tui/model.go, which this branch does not touch (CodeRabbit marks them "outside diff"). They look real — particularly toolResultSessionPayload persisting displayPreview for a redacted result — and deserve their own issue rather than a drive-by in a draft. Mutations: restoring the len(metas) == 0 return makes the new-user test fail; removing sanitizePickerLabel lets all four control bytes through. Pre-existing here and unrelated: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-13d543 | Claude Code | 4 prompts Origin-Snapshot: 8781ce78fdbe
…compactions Two blocking review findings from the draft review. Control bytes reached the terminal. redact() scrubbed secrets but not control characters, and the title and structural fields (name, toolCallId, role) skipped it entirely, so an imported title or message carrying ESC or NUL forged a picker row or corrupted a transcript line — the #835/#876 class, on strictly more attacker-influenced input. redact() now composes a stripControl pass (C0 except tab/newline, DEL, C1), and every rendered string — including the title at the import chokepoint — routes through control-stripping. The activity summary was emitted as EventCompaction, whose payload it did not satisfy. RehydrateEvents restructures the transcript around the last EventCompaction; a summary with no CompactableEvents/CompactedThroughSequence is hoisted to the front of the transcript on resume. It is now an assistant EventMessage, which still passes promptContextEvents (the resume digest) but carries none of that replay-side contract. A payload marker keeps it distinguishable from a translated turn, so filters and the digest can tell a Zero-generated summary from the foreign transcript. Also: the import-tag comment now matches ImportTag's actual output. Tests: regression coverage for both fixes, mutation-checked (removing the control strip surfaces the surviving byte; the summary type is asserted not to be EventCompaction). Existing tests updated for the new summary shape via a shared NoteEventIsSummary marker rather than the old EventCompaction type check. Origin-Session: local-13d543 | Claude Code | 2 prompts Origin-Snapshot: a939509c08a8
…oreign title cannot repaint the picker All four raised by CodeRabbit on ad57dd3. Two are real defects in this PR's own feature, not test issues. ## The feature was invisible to the user it exists for newSessionPicker gave up on an EMPTY local history: metas, err := m.sessionStore.ListResumable() if err != nil || len(metas) == 0 { return nil } Foreign sessions are discovered independently of the store, so the person with no Zero sessions at all — someone who just installed it and wants to carry on work another agent started — got nil before discovery ran. The import path was reachable only after they had already done by hand the thing it exists to save them. A failed read is still a reason to give up; an empty one is not. Emptiness is now decided after combining both sources, in pickerFromParts, split out so that decision is testable without a session store on disk. ## A foreign title reached the terminal unfiltered registry.go strips control bytes when a session is IMPORTED, and its comment names this picker row as the reason (#835/#876). But the picker lists a session BEFORE anything is imported, reading the title straight out of the other agent's transcript — so the vector that comment describes was the one path the stripping did not cover. An escape repaints the rows above, a carriage return hides the rest of the label, a NUL can truncate the row. sanitizePickerLabel drops control bytes and keeps the printable text: a title that is merely unusual must stay readable, because the row is how the user recognises their own work. ## The live-store test printed the developer's own sessions t.Errorf("incomplete index entry: %+v", session) That walks the REAL store, so a failure put the user's session titles, working directories and file paths into the test output and into any log or pasted report carrying it. It now names which fields are empty, which is the whole diagnostic — the missing value is by definition not the interesting part. NOT taken from the same comment: gating the live-store tests behind an explicit opt-in. @Vasanthdev2004 asked for the opposite in the review this branch is answering — a skip "would also stop it finding anything, so I would rather have the fixture" — and the tests now only report. The data leak was the substantive half and it is fixed. ## The symlink test asserted the weaker half of its property It checked only that Discover and Read AGREE, which passes in two opposite worlds: both correctly refusing a path reached through a symlink, and both happily following it out of the store. Containment is now asserted directly — "sneaky" must not be listed and must not be readable — and agreement is kept afterwards, since that is what the original fast path broke. Not taken, out of scope: three findings in internal/tui/model.go, which this branch does not touch (CodeRabbit marks them "outside diff"). They look real — particularly toolResultSessionPayload persisting displayPreview for a redacted result — and deserve their own issue rather than a drive-by in a draft. Mutations: restoring the len(metas) == 0 return makes the new-user test fail; removing sanitizePickerLabel lets all four control bytes through. Pre-existing here and unrelated: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-13d543 | Claude Code | 4 prompts Origin-Snapshot: 8781ce78fdbe
Fixes #875
What was wrong
syscall.StringToUTF16panics rather than returning an error when handed a string containing a NUL — documented standard-library behaviour — andgithub.com/atotto/clipboard's Windows path calls it directly. One NUL anywhere in a copied transcript selection therefore killed the whole program:The NUL gets through because
ansi.Stripremoves escape sequences and leaves C0 bytes untouched. The transcript carries NULs from two independent directions:git's-zNUL-separated listings, which this repo itself parses infiles_git_sweep.go:134"\x00command-card\x00"and"\x00plan-card\x00"The fix
Sanitise once at the write, not per source. The sources are unbounded — any tool result a user can select from is one — so a fix per source is a fix the next tool re-breaks.
clipboardSafeTextdrops C0 controls (keeping\nand\t, which are legal clipboard content and the normal case for a multi-line selection), applied before both destinations.The OSC52 fallback needed it too: a stray control byte terminates the escape sequence early and silently corrupts the copy. That is very likely why this went unnoticed off Windows — the identical NUL produces a bad copy there rather than a dead process.
Verification
TestNulNeverReachesTheClipboard— four real sources (command-card prefix, plan-card prefix, binary tool output,git -zlisting); asserts no NUL survives and that readable content is not dropped, so this sanitises rather than truncates.TestClipboardSanitisingKeepsLayout— clean multi-line text with tabs is returned byte-identical; other C0 bytes are removed.clipboardSafeTextcall makes the panic-guard test fail.make fmt-check,go vet ./...,go test ./..., release build + smoke,git diff --checkall clean.GOOS=windows go build ./...andGOOS=windows go vetclean.Two pre-existing failures on
mainare unrelated and unaffected:TestRunDoctorFormatsRedactedProviderDiagnosticsandTestRunDoctorConnectivityProbesProviderfail identically with this change stashed.I could not execute the Windows path itself (no Windows machine); the crash mechanism is pinned by the standard library's own documented contract and the reported stack, and the sanitiser is proven by the tests above.
Summary by CodeRabbit