Skip to content

fix(tui): sanitize clipboard text - #44197

Open
heymaaz wants to merge 1 commit into
anomalyco:v2from
heymaaz:copy-nul-output
Open

fix(tui): sanitize clipboard text#44197
heymaaz wants to merge 1 commit into
anomalyco:v2from
heymaaz:copy-nul-output

Conversation

@heymaaz

@heymaaz heymaaz commented Aug 22, 2026

Copy link
Copy Markdown

Issue for this PR

Fixes #44198

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

OpenTUI rejects clipboard text containing NUL characters, so shell output with NUL bytes caused /copy to fail. This strips NUL characters at the shared TUI clipboard boundary and adds a regression test, covering transcript, message, and selection copy paths.

How did you verify your code works?

  • Ran the TUI test suite: 748 passed, 5 skipped.
  • Ran the repository typecheck: 32 packages passed.
  • Opened the affected session with dev:live, ran /copy, and pasted the complete 669 KB transcript with Cmd+V.

Screenshots / recordings

Before

Screen.Recording.2026-08-22.at.4.44.08.PM.mov

After

Screen.Recording.2026-08-22.at.4.47.02.PM.mov

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Copilot AI lite review requested due to automatic review settings August 22, 2026 15:33
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the needs:compliance This means the issue will auto-close after 2 hours. label Aug 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes TUI clipboard writes failing when session text contains NUL (\0) characters by sanitizing clipboard text at the shared adapter boundary before handing it to OpenTUI, and adds a regression test to prevent recurrence.

Changes:

  • Strip NUL characters from clipboard text in createClipboardAdapter.write before calling @opentui/core’s writeText.
  • Add a regression test ensuring NUL-bearing text is written without NULs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/tui/src/clipboard.ts Sanitizes clipboard write input by removing NUL characters before delegating to OpenTUI.
packages/tui/test/clipboard.test.ts Adds a regression test validating NUL characters are removed prior to write.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Aug 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

Reasonable defensive fix and correctly placed — sanitizing once inside the shared write() (packages/tui/src/clipboard.ts:54) covers every route, and the test (packages/tui/test/clipboard.test.ts:105-116) pins it. Two small points:

  1. Worth documenting why NUL breaks a write. If the terminal route base64-encodes payloads for OSC 52, embedded NULs are harmless there, so the failure presumably comes from a path that emits text verbatim (or from native bridge APIs that treat NUL as a terminator, C-string style). A one-line comment naming the actual breaking consumer would keep a future refactor from dropping the sanitizer as "obviously unnecessary". It also clarifies whether other control characters (e.g. lone \x07 BEL) need similar treatment for that route.

  2. Silent mutation tradeoff: replaceAll("\0", "") alters content without any signal. Fine given NULs are almost never intentional in copied text, but consider a debug-level log when characters are removed so users diagnosing odd paste output have a trail.

Otherwise good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants