fix(coding-agent): handle clipboard helper failures - #1252
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Apocrathia
left a comment
There was a problem hiding this comment.
no injection to fix here, text always went through stdin. retitle and add a changelog?
jonaowen
left a comment
There was a problem hiding this comment.
The exact-head change closes both unsafe shell interpolation and the unowned wl-copy lifecycle: fixed argv with shell:false, bounded synchronous fallbacks, async spawn/error/stdin/exit observation, forced timeout kill, and success only after terminal status. The tests gate hostile stdin, async launch failure, nonzero exit, timeout kill, and fallback ordering. I found no blocker at 64744d9975a6e4174f20b8e644b7cecc3cf8478e.
Address PR #1252 review by detaching wl-copy, settling at the timeout even when close never arrives, and retaining late error listeners. Add behavioral coverage for success, child lifetime release, timeout fallback, and stdin-only fallbacks.
Summary
shell: false, while keeping clipboard text on stdinwl-copysuccess, detach it from the CLI lifetime, and force a bounded failure result even if a killed child never emitscloseBehavior
Clipboard text already traveled through stdin, so this is not an injection fix. The change removes unnecessary shell parsing and fixes lifecycle/error ownership: failed, signaled, or hung Wayland helpers now fall back to X11 or OSC 52 instead of being assumed successful or holding the CLI open.
Validation
npx tsx ../../node_modules/vitest/dist/cli.js --run test/clipboard.test.tsfrompackages/coding-agent— 10 passednpm run checkgit diff --checkBehavioral coverage includes native and synchronous helper success, fixed argv/stdin X11 fallback, Wayland launch failure, clean success, nonzero exit fallback, child unref ownership, and a timeout that settles without any
closeevent.Provenance
Extracted from independently authored commits
a3ba5dba6e8c280caa20cdca88f057810c35bb1aand16f5e83cc04bfdb465494f82f0e82006aa829c2cin #1159. Kernel/RLM cancellation remains separate.Review focus
Please verify fixed argv/stdin data flow, child and timer ownership, late-event safety, and fallback ordering across Wayland, X11, and OSC 52.
Note
Low Risk
Localized clipboard utility changes with preserved fallback order; main behavioral shift is correctly failing over when
wl-copyhangs or errors instead of silently succeeding.Overview
Hardens clipboard fallbacks by replacing
execSyncwithspawnSync/spawnandshell: false, passing clipboard text only via stdin so hostile pasted content cannot be interpreted by a shell.On Linux Wayland,
copyToWaylandClipboardnow awaitswl-copy, requires a clean exit, and SIGKILLs after 5s if the helper hangs; failures then continue the existing chain (X11xclip→xsel→ OSC 52) instead of treating a fire-and-forgetwl-copyas success. Platform helpers return boolean success so fallbacks run when a tool fails, exits non-zero, or is signaled.Tests in
clipboard.test.tscover Wayland spawn/close/error, timeout cleanup, and xclip → xsel fallback with injection-shaped input.Reviewed by Cursor Bugbot for commit f30271c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix
copyToClipboardto handle unresponsive or failing Wayland clipboard helpercopyToWaylandClipboardnow spawnswl-copywith shell disabled, writes text via stdin, and waits for a clean exit (code 0, null signal) before reporting success.falseifwl-copyis unresponsive, preventing a silent false-positive.wl-copyattempts now fall back to X11 tools (xclip/xsel) or OSC 52, instead of continuing as if the copy succeeded.pbcopy,clip,termux-clipboard-set,xclip,xsel) are switched fromexecSynctospawnSyncwithshell: falseand stdin-only input.wl-copylaunch (without waiting for completion) will now wait up to 5s and may fall back to X11 or OSC 52.Macroscope summarized f30271c.