Conversation
The WSL port left every macOS branch "unchanged by construction" -- an argument, not a test result, and the plan that produced it said so. This is the test result. A macos-latest (arm64) job, sibling of the linux one, same conventions: pinned checkout SHA, persist-credentials off, and nvim pinned to the same v0.11.0 tarball checked against the same release shasum.txt, so a job that goes red is the code and not the editor. brew installs fzf and nothing else; each omission is spelled out in the file, but the load-bearing one is coreutils -- installing it would put gtimeout, not timeout, on PATH, and taking the perl leg is the entire point. It is not the linux job with a different logo. It is the only place bash is 3.2, pgrep/ps/sed/awk are the BSD ones, and tests/bin/timeout actually runs its perl alarm fallback: on Linux the shim always finds a real timeout(1) and hands over, so the branch that file was WRITTEN for had never executed in CI. tests/run.sh: probe the pasteboard on macOS instead of assuming it. The preflight skipped Darwin entirely, on "macOS always has pbcopy". True about presence, and nothing about capability -- the pasteboard lives in a per-session pbs, so over ssh or on a CI runner pbcopy is right there on PATH with nothing to talk to, and the six suite_config.sh cases that assert register '+' would fail looking like six mapping regressions. That is the same mistake the xclip branch already stopped making, so it is now made once, in one shape, on both platforms: write with pbcopy, read with pbpaste, believe the exit code. That probe needs `timeout`, which macOS does not have, so run.sh now hoists tests/bin onto PATH the way every suite already does for itself -- which also makes docs/testing.md's claim that it does so true. Also: the case count was 556 in six places and is 558. suite_options.sh went 84 -> 86 when the port added the wsl_quit_fallback assertions and nothing else moved with it. Measured, not counted by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe test harness now validates macOS clipboard access. CI adds a macOS arm64 job with pinned Neovim and platform-specific checks. Documentation records the 558-case suite, clipboard findings, and expanded platform coverage. ChangesCross-platform testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI as macOS CI
participant Neovim
participant Homebrew
participant tests_run as tests/run.sh
participant demo as demo.sh
CI->>Neovim: download and verify pinned arm64 release
CI->>Homebrew: install fzf
CI->>tests_run: run test suite
tests_run->>tests_run: probe macOS clipboard
CI->>demo: validate syntax and secs() output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/run.sh (1)
58-60: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRequire a round-trip match before accepting
CLIP.Line 58 writes a marker, but Line 59 discards the
pbpasteoutput. Exit codes alone do not establish that the marker can be read back. Compare the returned bytes withxbefore assigningCLIP.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/run.sh` around lines 58 - 60, Update the clipboard detection condition around pbcopy and pbpaste to capture and compare pbpaste’s output with the marker x before assigning CLIP="pbcopy"; retain the existing timeout and failure handling so CLIP is only accepted when the write/read round trip matches exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/run.sh`:
- Around line 58-60: Update the pbcopy/pbpaste capability probe in tests/run.sh
to preserve the user’s existing pasteboard contents: capture the previous
payload before writing the probe value, then restore it through cleanup
regardless of whether pbpaste succeeds or fails. Keep the CLIP assignment
behavior unchanged when the probe passes.
---
Nitpick comments:
In `@tests/run.sh`:
- Around line 58-60: Update the clipboard detection condition around pbcopy and
pbpaste to capture and compare pbpaste’s output with the marker x before
assigning CLIP="pbcopy"; retain the existing timeout and failure handling so
CLIP is only accepted when the write/read round trip matches exactly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c0c19403-4aa4-4043-899f-d269df9607dd
📒 Files selected for processing (6)
.github/workflows/tests.ymlREADME.mdTODO.mddocs/testing.mddocs/wsl.mdtests/run.sh
CodeRabbit, on the new Darwin branch: the probe writes "x" to the system pasteboard and never puts back what was there. Fair -- and its own suggested fix, capture-and-restore, would be worse than the bug, because pbpaste yields text, so the round-trip silently flattens an image, RTF or a file promise to nothing. Its other suggestion is the right one. macOS serves four named pasteboards from the same pbs, so `-pboard find` proves reachability exactly as well and leaves the general one alone. The register cases still destroy your clipboard later in the run -- but only if you actually run them, and `./tests/run.sh timer` should not cost you a copied password. No equivalent below: wl-copy offers only clipboard and primary, and primary is the user's too, so the Linux branch still writes, as its own comment already owned. Verified with the real preflight cut out of run.sh and a pbcopy stub that exits non-zero unless asked for the find pasteboard: silent as written, and one WARNING when the same code is aimed at the general pasteboard -- so the check is not vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
cubic, P3: -pboard find is non-destructive to the clipboard but not side-effect free -- it leaves a stray "x" as the system find string, which Preview and some search fields honour. Correct. The objection that stopped me restoring the GENERAL board does not apply here: the find board holds a search string, which is text by definition, so a pbpaste round trip cannot flatten an image or RTF the way it would there. So save it and put it back. The probe now costs nothing at all. Not the read-only probe cubic offered as the alternative: that drops the write half, and writing is what the register cases actually depend on. While in here, compare by VALUE rather than by exit code. Write "x", read "x" back -- that is the property the register cases need, and two processes exiting 0 is not it. Verified against a file-backed fake find board: a stub whose write succeeds and whose read returns something else now produces a warning, where the old exit-code test called it a working pasteboard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…aught The macOS job ran and 522 of 558 cases passed. The 36 that did not were the whole timer suite, which died before its first case: suite_timer.sh: line 52: syntax error near unexpected token `;;' case "$(ps -p "$p" -o comm= 2>/dev/null)" in python*|*/python*) ... Nothing to do with pgrep, ps, or the timer. /bin/bash on macOS is 3.2, and 3.2 finds the end of a $( ) by scanning for a balancing paren instead of parsing. This heredoc lives inside $( ), and a case pattern contributes a lone ')' -- so the scan ended on the pattern, the remainder of the line was read as a command, and `;;` was a syntax error. The fix is the optional open paren POSIX allows on a case pattern, which balances the count. Bash 4+, zsh, and the meaning of the pattern are all unaffected. The comment above it says so, because it looks exactly like something worth tidying away. Reproduced and verified on real bash 3.2.57 (docker bash:3.2): the old spelling gives that error at that line, the new one parses. Every other shell script in the repo was swept the same way -- 20 files, all clean, so this was the only one. This is the entire argument for the macOS job in one commit. The bug was invisible to the Linux gate, to bash -n on this box, and to four review bots, and it disabled 36 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The macOS job earned its keep on its first runRun 31125892967 is the first one Actions let through. 522 of 558 passed. The 36 that did not were the entire timer suite, and it died before its first case: Nothing to do with
The fix is the optional open paren POSIX allows on a case pattern — Verified on real bash 3.2, not by reasoning about it
Then swept every shell script in the repo the same way — What the run also settled, in our favourThree risks were called out in the description. Two are now answered:
Linux gate after the fix: 557/558, the one failure being This bug was invisible to the Linux gate, to |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… term cubic, P3: the restore ran unconditionally, and FIND_WAS reports nothing about whether the save worked. If pbpaste hit the 5s timeout on a merely SLOW pbs, FIND_WAS collapsed to empty and the restore then wrote that empty value over a find term that was there all along -- the exact data loss the save/restore was added to prevent. Right, and reproducible. With a stub whose first read hangs: OLD code, slow pbs -> board is now '' NEW code, slow pbs -> board is now 'my search term' The fix removes a variable rather than adding one: put the save in the if, and never write to a board you have not proved you can put back. Reading first is free because a failed read is already the answer -- if pbs will not answer a paste it is not a working provider, CLIP stays empty, the warning fires, and nothing was written. Four cases, each starting from a board holding "my search term": healthy pbs warn=0 board=my search term pbs unreachable (read broken) warn=1 board=my search term pbs write broken warn=1 board=my search term SLOW pbs -- cubic's case warn=1 board=my search term Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t was docs/testing.md claimed these cases "pass when run alone (10/10, 3/3, 3/3)" and told you that a solo failure means a real bug. Both halves are wrong, and the second one is the harmful half -- it sends you hunting a regression that is not there. Bigger sample: ctrlc_visual_charwise_exclusive, alone in a loop, failed 2 times in 30. Same signature as always, buffer correct and '+' empty. Isolation lowers the rate by roughly 5x. It does not reach zero. So the advice is now "run it five times and read it as a rate": five passes is the flake, five failures is a bug, one-in-five is the flake again. The 10/10 sample that produced the old claim was simply too small to meet a 2-in-30 event. Nothing about the flake itself changed and nothing is papered over. It is still WSL-only, still not the provider, still predates the port, and CI has still never hit it -- both jobs, both platforms. Found because the gate flaked on this PR twice on different cases, and the documented "re-run it once" check disagreed with itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…lower rate cubic, on the paragraph I added one commit ago. It is right and the error was mine: I compared a PER-CASE rate (2/30) against a PER-RUN rate (1 in 3) as though they shared a denominator, and called the difference a load effect. They do not share a denominator. A full run bets the same 6.7% six times: 1 - (1 - 2/30)^6 = 34% vs the observed ~1 in 3 Those agree, so there is nothing left for "load" to account for -- and the "factor of five" I wrote was just the six-cases multiplier wearing a disguise. The page now says so, and drops the load explanation entirely rather than demoting it, since the data never supported it in either version. The same arithmetic also explains the 10/10 sample this page used to rest on: at 6.7%, ten clean runs happen 50% of the time regardless, and three clean runs 81% of the time. That was a coin flip being read as a proof. Reworked the verdict guidance to match. Consistency is the signal: five failures out of five is a bug (1.3e-06 by chance), one or two out of five is the flake, and five passes is the WEAKEST reading of the three -- it happens 71% of the time with the flake present, so it is consistent with the flake rather than evidence against it. The old text implied it was conclusive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Why
The WSL port merged with every macOS branch marked "unchanged by construction". That is an argument, not a test result — the plan that produced it said so in as many words. The Linux job proves the port; nothing proves the thing it was ported around.
This is the test result.
What
A
macosjob onmacos-latest(arm64). Sibling oflinux, same conventions: pinnedactions/checkoutSHA,persist-credentials: false, and nvim pinned to the same v0.11.0 tarball checked against the same releaseshasum.txtthe Linux job's checksum came from — so "which nvim did the gate run against" keeps one answer, and a macOS-only failure is the code rather than the editor.brew install fzfand nothing else. Every omission is argued in the file; the load-bearing one is coreutils — installing it would putgtimeout, nottimeout, onPATH, so the shim would not find it anyway, and taking the perl leg is the entire point.Not one Intel leg: those runner labels are on GitHub's deprecation path, so it would be a check that needs re-pinning rather than one that keeps working.
This job reaches code the Linux job structurally cannot
tests/bin/timeoutperl alarmfallback. On Linux the shim finds a realtimeout(1)andexecs it — so the branch that file was written for has never executed in CI#!/bin/bashsuitedrill.sh,suite_timer.shpgrep/ps, and BSDsed/awkfor thesecs()check that exists to dropbctests/run.sh— probe the pasteboard, don't assume itThe preflight skipped Darwin outright, on "macOS always has pbcopy, so this is silent there". True about presence, and says nothing about capability: the pasteboard lives in a per-session
pbs, so over ssh or on a CI runnerpbcopyis right there onPATHwith nothing to talk to. The sixsuite_config.shcases that assert register+would then fail looking like six unexplained mapping regressions.That is precisely the mistake the
xclipbranch already stopped making ("a display name is not a display"). Now it is made once, in one shape, on both platforms: write withpbcopy, read back withpbpaste, believe the exit code.The probe needs
timeout, which macOS has not got — sorun.shnow hoiststests/binontoPATHthe wayrun_test.shand every suite already do for themselves. Side effect: it makesdocs/testing.md's existing claim thatrun.shdoes this true.Drive-by: the case count was wrong
556 in six places; it is 558.
suite_options.shwent 84 → 86 when the port added thewsl_quit_fallback_*assertions and no count moved with it. Measured from a run, not counted by hand — the docs table now sums to 558 as well.Verified locally
./tests/run.sh→ ALL SUITES PASSED, 558, 0 failures, 14/14 suitesworkflow YAML parses;
bash -non all 8 inlinerun:blocksthe Darwin arm proven able to fail — the real preflight cut out of
run.sh(not a copy pasted into a test) run under a stubbeduname/pbcopy/pbpaste, four ways:pbcopyfailspbcopyok,pbpastedeadpbcopyhangs 300sthe perl
timeoutleg exercised on a PATH with no realtimeout: stdin passes through the pipe, and a hang is still boundedWhat is not verified: macOS itself. There is no Mac here — that is the whole reason this job exists. The first
macosrun is partly discovery. Three things are most likely to come back red and are called out so they get recognised rather than re-diagnosed:pgrep -fvs the timer's ~2KB argv (36 cases) —_drill_timer_pidsrests on "pgrep matches the whole command line, while ps truncates", measured on Linux; BSDpgrepreadsKERN_PROCARGS2bounded bykern.argmaxMerging only at
linux558 andmacos558.Summary by cubic
Adds a
macos-latestCI job to run the full gate on macOS and hardens the pasteboard probe to be non-destructive, value-checked, and save-gated. Fixes a bash 3.2 parsing bug in the timer suite; suite count is 558.New Features
macosjob onmacos-latest(arm64) alongsidelinux.actions/checkoutand thenvimv0.11.0 tarball (checksum-verified);persist-credentials: false.fzfvia Homebrew; skippedcoreutilsto exercise the perltimeoutpath.demo.shsecs()under BSDsed/awkand bash 3.2.Bug Fixes
tests/run.shprobespbcopy/pbpasteon-pboard find, gates the write on a successful save, restores it, and checks round-trip by value to avoid clobbering the clipboard and prevent data loss on slow pasteboards.tests/binontoPATHsotimeoutis available during preflight; clearer “no working clipboard” warnings (including macOS session guidance).suite_timer.shon bash 3.2 by balancing the case-pattern(inside$(...), unblocking all 36 timer cases on macOS.suite_options.shis 86; corrected WSL clipboard flake math and guidance to read results as rates and consistency, not load or single-run verdicts.Written for commit 415c92b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
Adds a macOS CI gate alongside Linux, exercising platform-specific shell, utility, clipboard, and timeout behavior against the pinned Neovim release.
macos-latest.Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
Reviews (8): Last reviewed commit: "Correct the flake arithmetic: isolation ..." | Re-trigger Greptile