Skip to content

fix: honor clipboard tool exit status - #48

Merged
Prateekgaur merged 1 commit into
KlaatAI:mainfrom
Ayush7614:fix/clipboard-exit-status
Jul 23, 2026
Merged

fix: honor clipboard tool exit status#48
Prateekgaur merged 1 commit into
KlaatAI:mainfrom
Ayush7614:fix/clipboard-exit-status

Conversation

@Ayush7614

@Ayush7614 Ayush7614 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes copyToClipboard() reporting success when the clipboard tool is missing or exits non-zero (closes copyToClipboard() reports success even when the copy actually failed #17).
  • Moves the helper to src/utils/clipboard.ts with injectable spawnSync for unit tests covering darwin/win32/linux failure paths.
  • REPL callers keep showing a clear failure message when copy fails.

Test plan

  • bun run typecheck
  • bun test
  • bun run build
  • On a machine without xclip/xsel (or with a mocked failure), confirm Ctrl+Y shows the failure message

copyToClipboard now returns false when pbcopy/clip/xclip/xsel are
missing or exit non-zero, instead of always reporting success.
@github-actions

Copy link
Copy Markdown
Contributor

🤖 KlaatAI Review Bot (powered by Klaatu, advisory only — a maintainer makes the real call)

Issue match
Yes, this fully addresses #17. The new src/utils/clipboard.ts checks both r.error and r.status === 0 on every platform branch (darwin/win32/linux), matching the issue's suggested fix. The helper is also moved out of repl.ts and given an injectable spawn parameter, which is a reasonable refactor that didn't break callers (the boolean contract is preserved).

Test coverage
src/utils/clipboard.test.ts covers the main paths: darwin success/non-zero/ENOENT, win32 success/non-zero, and linux xclip→xsel fallback incl. both-fail. Gaps worth noting:

  • win32 branch has no ENOENT test (only status 0/1).
  • No test for signal-killed child (status === null, no error) — the ok() helper's (r.status ?? 1) === 0 behavior on this path is unexercised.
  • No assertion that the input text actually reaches the spawned process (i.e. that the mock receives the expected input).

Correctness concerns

  • repl.ts previously imported spawnSync from child_process for the inline helper; that import is no longer shown used in the diff. Worth confirming it isn't now dead — if nothing else in repl.ts uses spawnSync, the import should be removed.
  • Linux fallback is now slightly broader than the original (which only fell back on r.error): it now also retries xsel when xclip exits non-zero with no error. Matches the issue's suggestion and is arguably better, but worth a maintainer nod.
  • The try/catch wrapping copyToClipboard is largely defensive — spawnSync rarely throws, it returns errors in r.error. Harmless, but the catch swallows real bugs.

Verdict
Ready to merge after the human reviewer confirms the spawnSync import in repl.ts is still needed elsewhere in the file; the missing win32-ENOENT and signal-kill test cases are nice-to-have, not blockers.

This is an automated review to help triage faster, not a gate. Nothing here blocks merging.

@Prateekgaur
Prateekgaur merged commit 92bc293 into KlaatAI:main Jul 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

copyToClipboard() reports success even when the copy actually failed

2 participants