fix: preserve unicode in Windows clipboard fallback#388
Conversation
| // clip.exe is always available on Windows. Unicode handling is | ||
| // imperfect (system locale encoding) but good enough for a fallback. | ||
| void execFileNoThrow('clip', [], opts) | ||
| // PowerShell's Set-Clipboard preserves Unicode text more reliably than |
There was a problem hiding this comment.
I can't sign off on this as-is because the real Windows codepage path still fails. On this Windows shell, chcp defaults to 437, and piping Привет мир into the exact PowerShell pattern used here ($text = [Console]::In.ReadToEnd(); Set-Clipboard -Value ) still round-trips as ?????? ???. So the tests prove that we switched from clip.exe to PowerShell, but they do not prove that Unicode is actually preserved in the reported locale/codepage scenario. We need either a codepage-independent handoff here or a test/repro that exercises the actual Windows stdin encoding boundary rather than only asserting the command name and JS input string.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head 7f432fe87df28da4eb13bde4ae7a8a4be12f0f53.
I still can't approve this because the proposed Windows clipboard fallback does not actually verify the reported Unicode/codepage failure mode.
Current blocker:
-
The new
powershell Set-Clipboardpath still mojibakes Unicode when stdin comes through the real Windows console codepage boundary.
On this Windows shell,chcpdefaults to437, and pipingПривет мирinto the same pattern used here:powershell -NoProfile -NonInteractive -Command '$text = [Console]::In.ReadToEnd(); Set-Clipboard -Value $text; [Console]::WriteLine($text)'still round-trips as
?????? ???.So the current tests prove that we switched from
clip.exeto PowerShell and that the JS-side input string is Unicode, but they do NOT prove that Unicode survives the actual Windows stdin/codepage boundary from the issue report. That is the part this PR needs to fix.
Fresh verification on this head:
- direct repro on Windows shell: PowerShell
Set-Clipboardstdin path still outputs?????? ???forПривет мир bun test src/ink/termio/osc.test.ts src/ink/parse-keypress.test.ts-> 9 passbun run scripts/build.ts-> success
So the focused tests/build are green, but I can still reproduce the exact class of Unicode breakage this PR claims to fix.
|
Hi @Vasanthdev2004, I pushed a follow-up fix that addresses the requested changes on this PR. Updated approach:
Verification on the updated head:
If you have time, please take another look and let me know whether this version resolves the remaining concern. PR head: |
| const originalEnv = { ...process.env } | ||
| const originalPlatform = process.platform | ||
|
|
||
| const generateTempFilePathMock = mock(() => '/tmp/openclaude-clipboard.txt') |
There was a problem hiding this comment.
The code change itself is headed the right way, but these new Windows-specific tests are currently broken on Windows. On this machine, �un test src/ink/termio/osc.test.ts src/ink/parse-keypress.test.ts fails both Windows clipboard tests because the mocked temp path here is '/tmp/openclaude-clipboard.txt', and Test-Path '/tmp' is false in this Windows environment. That means the async writeFile() step fails before the mocked PowerShell call ever happens, so the assertions at lines 50-79 never observe the command they expect. The mock path needs to be valid on Windows (or the file write needs to be mocked) before this regression coverage is trustworthy.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head 54e6df58eb04d9d0d9765bf9faecc8537d8ad908.
The implementation itself looks materially better than the previous revision:
- the Windows fallback no longer pipes Unicode through the console stdin/codepage boundary
- direct CP437 verification on this machine now works with the same UTF-8 temp-file read strategy the PR uses
- current shell
chcp->437 - UTF-8 temp-file PowerShell round-trip for
Привет мир->0J/RgNC40LLQtdGCINC80LjRgA==(correct UTF-8 base64)
- current shell
I still can't approve this head yet because the new Windows regression tests are currently broken on Windows.
Current blocker:
-
src/ink/termio/osc.test.tshardcodes the mocked temp path as'/tmp/openclaude-clipboard.txt', which is not valid in this Windows environment.
On this machine:Test-Path '/tmp'->Falsebun test src/ink/termio/osc.test.ts src/ink/parse-keypress.test.tsfails both Windows clipboard tests
The failure mode matches the fixture problem: the async
writeFile()step fails before the mocked PowerShell call is ever observed, so the test assertions never see the command they expect.
Fresh verification on this head:
- direct CP437 temp-file repro now preserves Unicode correctly
bun test src/ink/termio/osc.test.ts src/ink/parse-keypress.test.ts-> 7 pass, 2 failbun run scripts/build.ts-> successbun run smoke-> success
So the code change is on the right track, but I still wouldn't merge it until the new Windows-focused tests are made valid and actually pass on Windows.
|
Hi @Vasanthdev2004, I pushed one more follow-up on top of the temp-file fix to address the Windows test fixture problem you called out. What changed in this update:
Verification on the updated head:
Current PR head: If you have time, please take another look and let me know whether this clears the remaining blocker. |
There was a problem hiding this comment.
The Windows fallback now writes clipboard contents to a plaintext temp file before invoking PowerShell, which introduces a new at-rest exposure path for potentially sensitive clipboard data. If the process crashes or is interrupted, that data can linger on disk.\n\nThe new test file is also a bit flaky: it relies on a 0ms timer to wait for an unobserved async IIFE and uses a real filesystem path in the repo root, so it can race or leave artifacts under load.\n\nSuggested fix: keep the payload in-memory or on stdin with an explicit UTF-8 encoding path, and make the test await a deterministic signal or fully mock fs/tempfile behavior.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Rechecked the latest head 1137b9a037ded5cae55c507252ad7635cf3f281e thoroughly.
What I verified:
- the Windows fallback no longer sends Unicode through the console stdin/codepage boundary; it now hands PowerShell a UTF-8 temp-file read path instead
- the remaining Windows test-fixture issue is fixed: the mocked path is now host-valid and the module mocks stay active for the full
osc.test.tsrun - targeted verification on this head passed locally after wiring the worktree to the repo dependencies:
bun test src/ink/termio/osc.test.ts src/ink/parse-keypress.test.tsbun run scripts/build.tsbun run smoke- repeated
bun test src/ink/termio/osc.test.ts20x with no failures
I don't see a remaining blocker on the current PR head, so approving.
anandh8x
dismissed
their stale review
Superseded by my later approval on the same PR head.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head 1137b9a037ded5cae55c507252ad7635cf3f281e.
The previous blocker is fixed on this revision.
What I verified on this head:
- the Windows fallback still avoids the console stdin/codepage boundary by writing UTF-8 to a temp file and having PowerShell read it as UTF-8 before
Set-Clipboard - direct CP437 verification on this machine still works:
- shell
chcpis437 - UTF-8 temp-file PowerShell round-trip for
Привет мир->0J/RgNC40LLQtdGCINC80LjRgA==(correct UTF-8 base64)
- shell
- the Windows test fixture is now valid on Windows and the focused tests pass:
bun test src/ink/termio/osc.test.ts src/ink/parse-keypress.test.ts-> 9 pass
- repeated
bun test src/ink/termio/osc.test.ts5x -> all pass bun run scripts/build.ts-> successbun run smoke-> success- no leftover mocked clipboard temp file was present after the test run
I didn't find a remaining blocker on the current head.
* fix: preserve unicode in Windows clipboard fallback * fix: avoid Windows clipboard stdin codepage issues * test: fix Windows clipboard temp path fixture
4 participants
Summary
src/ink/termio/osc.tswith PowerShellSet-Clipboardso fullscreen selection copy preserves non-ASCII textTesting
\"/home/gnanasekaran/.bun/bin/bun\" test src/ink/termio/osc.test.ts\"/home/gnanasekaran/.bun/bin/bun\" test src/ink/parse-keypress.test.ts\"/home/gnanasekaran/.bun/bin/bun\" run scripts/build.tsManual Verification
Fixes #386