Conversation
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds a shared GNU/BSD ChangesPortable stat handling
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
Thirteen scripts chose between BSD `stat -f <fmt>` and GNU `stat -c <fmt>` by branching on `uname`. That is the wrong discriminator: a Darwin kernel routinely resolves `stat` to GNU coreutils (nix-darwin, or Homebrew coreutils ahead of /usr/bin on PATH), so the kernel's name does not predict the binary's dialect. When it guesses wrong the failure is silent, not loud. GNU's `-f` is not "format" - it is --file-system. `stat -f %m <path>` treats %m as an extra operand, stats the FILESYSTEM of <path>, and prints a multi-line apfs dump. Downstream numeric checks then fail permanently: the remote-job readiness probe never reports ready (robots-xw8p), lock reaping refuses every abandoned lock, and the pending-reply signature pins at 'unreadable' so no missed reply is ever detected. The `stat -f ... || stat -c ...` fallback form does not help and is worse - GNU exits 0 on the dump, so the `||` never fires and the correct call never runs. Add bin/fm-stat-lib.sh as the single owner. It feature-detects the binary once per process and caches the answer, so the hot callers (fm_path_mtime runs inside 0.2s confirm and 0.5s attach polls) stay fork-free after the first probe. The probe order is load-bearing: it tries `stat -c %s /` FIRST, because `-c` on BSD is an unknown option that writes nothing to stdout and exits non-zero, while `-f` on GNU succeeds with junk. Only `-c`-first cannot be fooled by either flavor. Named accessors (fm_stat_mtime, _mode, _device, _inode, _links, _size, _identity, _signature, _fingerprint) keep the letter pairs in one place, and the single-value ones gate on a bare unsigned integer so a third flavor would fail cleanly instead of leaking a stray token into arithmetic. Converted call sites: fm-x-lib.sh (4 fns, plus the :421 ordering trap), fm-watch.sh, fm-pr-lib.sh (4 fns), fm-supervise-daemon.sh, fm-lock-lib.sh, fm-wake-lib.sh, fm-remote-inherit.sh, fm-remote-inherit-push.sh, fm-remote-file.sh, fm-classify-lib.sh, fm-pending-reply-lib.sh, fm-backlog-receive.sh, and fm-remote-job-lib.sh - the last so the fix from robots-xw8p, whose branch never landed, is not left as a fourteenth private copy of the same decision. bin/fm-test-run.sh is deliberately NOT converted. Its one site is already GNU-first, which is the safe ordering, and its own test copies just that file into a fixture repo, so it cannot source a sibling. A comment now pins the ordering so nobody "fixes" it into the broken direction. Eight further uname-keyed sites exist that this ticket's survey missed; they are filed as robots-ldiu rather than folded in here, because two of them need fake-`stat` fixture surgery that does not belong in a mechanical refactor. Test fixtures that hand-list a partial bin/ gain the new sibling: without it the remote-job worker fails to start and every fm-wake-lib.sh consumer aborts on a missing source. Refs: robots-e8x5, robots-xw8p, robots-ldiu
PR #102 made bin/fm-pr-lib.sh and bin/fm-lock-lib.sh source the new leaf bin/fm-stat-lib.sh, each resolving the sibling from its own unresolved BASH_SOURCE directory. fm-gotmp.test.sh builds a fake FM_HOME/bin by symlinking each sibling the real fm-teardown.sh sources, but not the new fm-stat-lib.sh, so the nested source failed against the fake bin and teardown exited non-zero under set -e ("teardown exited non-zero with a valid tasktmp"), failing CI on the #102 run. Add the fm-stat-lib.sh symlink to both fake-bin fixtures in this file (make_fake_root and the inline builder in test_teardown_skips_gracefully_without_tasktmp); the second would have tripped the same failure once the first test stopped aborting the suite.
…sion-lib, fix backend fixture
e906b9b to
ea4123f
Compare
trillium
changed the title
…dering
Three comments asserted that GNU `stat -f <fmt> <path>` "exits 0 with a
filesystem dump, so the fallback never runs". That is inverted, and it is the
load-bearing justification for GNU-first probe ordering — a maintainer who
tests the claim, finds it false, and concludes the `||` fires cleanly would
flip to BSD-first and reintroduce the exact defect this branch fixes.
Measured (GNU coreutils 9.7 ahead of /usr/bin on Darwin):
stat -f %m README.md -> rc=1, 227 bytes of apfs
dump ALREADY on stdout
stat -f %m F 2>/dev/null || stat -c %Y F ... -> rc=0, len=238
(dump + real mtime)
stat -c %Y F 2>/dev/null || stat -f %m F ... -> rc=0, len=10 (correct)
/usr/bin/stat -c %s / -> rc=1, stdout EMPTY
GNU's `-f` is --file-system and takes no format operand: it writes the dump to
STDOUT and only THEN exits 1. The `||` DOES fire; `2>/dev/null` silences only
stderr, so the fallback's correct integer is APPENDED to the dump already in
the pipe. The hazard is STDOUT POLLUTION AT rc=0, never a suppressed exit
status.
GNU-first is required because only BSD rejects CLEANLY — non-zero with nothing
on stdout — so its discarded probe cannot contribute output. Comments now match
the corrected bin/fm-stat-lib.sh header. Comment-only; no behavior change.
Fixes robots-qyim.
7e7bd9f corrected three of the comments asserting that GNU `stat -f <fmt> <path>` "exits 0 with a filesystem dump" so the `||` "never fires". Two sites carrying the same inverted claim were missed. This finishes the sweep; the three sites 7e7bd9f already fixed are left exactly as it wrote them, because its wording is correct and equivalent and rewording it would be diff noise on a branch with 13 green CI shards. Why they were missed, since it generalises: a line-oriented grep for the phrase cannot match bin/fm-watch.sh, where it wraps across lines 99-100 ("...and the `||` never" / "fires, so arithmetic..."). Any prose sweep over comments has to join lines before matching. bin/fm-watch.sh:99 - comment-only, same correction as the other three: GNU's `-f` is --file-system, so it writes the dump to STDOUT and only THEN exits 1. The `||` DOES fire; that is the trap, not the escape. A single `$(... || ...)` captures both commands, so the fallback's correct integer lands appended to the dump at overall rc=0, and the arithmetic under `set -u` aborts on a stray token and silently kills the watcher mid-cycle. tests/fm-watch-triage.test.sh:68 - NOT comment-only, and not cosmetic. The comment was wrong in a second way ("writes a partial filesystem dump on Linux" - wrong platform; the break is GNU-coreutils-on-Darwin), and the helper underneath it was still live-buggy: if [ "$(uname)" = Darwin ]; then stat -f %m "$1"; else stat -c %Y "$1"; fi That is the original robots-e8x5 defect verbatim. On this host - GNU coreutils ahead of /usr/bin on PATH, Darwin kernel - it takes the Darwin arm, runs GNU `stat -f %m`, and returns an apfs dump as an "mtime" into test arithmetic. Rewritten to the GNU-first probe the other standalone caller already uses (bin/fm-test-run.sh): BSD rejects `-c` with a usage error on stderr and writes NOTHING to stdout, so its discarded probe cannot pollute the pipe. Verified: helper returns a clean integer under a GNU-shadowed PATH and under a BSD-only PATH; tests/fm-watch-triage.test.sh passes 21/21 (exit=0); shellcheck -x clean; the bin/ hunk is provably comment-only (diff with comment lines filtered is empty). Refs: robots-qyim, robots-e8x5, robots-xw8p
|
@coderabbitai review |
|
|
Superseded by #104, closing.
Every remaining One thing to carry forward, and it is worth stating plainly: this branch's version of the Measured here on GNU coreutils 9.7: rc is 1. GNU takes the operands in order — The distinction is not cosmetic. The real failure is that the fallback runs and appends its correct integer to the dump already in the same command substitution, so the caller gets a multi-line non-integer at overall rc=0 — invisible to error handling, fatal to the arithmetic after it. Reverting The code on main is correct either way (feature-detect, never chain), and probing This branch's header text is being ported back onto main in a follow-up PR. Tracking ticket: robots-6qo0. |
…#132) * fix(comments): GNU stat -f exits 1 after dumping, so the || does fire Six sites state the mechanism backwards. They claim GNU `stat -f <fmt> <path>` exits 0 and that the `||` in a `stat -f ... || stat -c ...` chain therefore never fires, leaving the correct call unrun. Measured, GNU coreutils 9.7: $ gstat -f %m /tmp ; echo rc=$? File: "/tmp" ID: 100000d0000001a Namelen: ? Type: apfs Block size: 4096 Fundamental block size: 4096 rc=1 rc is 1. GNU takes the operands in order: the format string fails as an operand and the path prints 223 bytes of filesystem dump to stdout, so the exit is non-zero and the `||` does fire. The failure is not that the fallback never runs - it is that the fallback runs and appends its correct integer to the dump already sitting in the same command substitution. The caller receives a multi-line non-integer at overall rc=0: invisible to error handling, fatal to the arithmetic after it. This matters beyond wording. The stated reason is why nobody may reintroduce the chain, and bin/fm-stat-lib.sh is the single owner of that explanation, so its header being wrong is the worst of the six. It also contradicted bin/fm-busy-event.sh:110 on the same tree, which already said "so the `||` does fire". The probe order is unchanged and still correct, but the reason is restated: `-c` goes first because GNU's `-f` pollutes stdout before failing, so a `-f`-first probe would contribute junk even when it fails - not because the `||` fails to fire. tests/fm-remote-job.test.sh carried the error twice: once in prose, and once in the GNU shim itself, whose -f arm printed the dump and exited 0. A fixture labelled "GNU coreutils shape" that disagrees with the binary it stands in for teaches the next reader the wrong mechanism. It now exits 1. The suite still passes, which also confirms the correct code path never reaches that arm. bin/fm-test-run.sh's chain is GNU-FIRST and therefore correct code; only its justifying comment was wrong, so that site is comment-only. Reported in robots-qyim, which #102 fixed on its branch (7e7bd9f, ffa1936); #104 was what merged and it carried the pre-fix prose, plus a new instance in fm-stat-lib.sh itself. * no-mistakes(review): correct remaining inverted GNU stat exit-status comments --------- Co-authored-by: Trillium Smith <trillium@macbookpro>
1 participant
Intent
The developer dispatched a scout task to reconcile two diverged git histories in the firstmate repository using git-imerge, as one arm of a two-tool bake-off against git-town. The goal was to merge origin/main (commit 80058d2, containing an upstream merge, Muse adapter, Relay rename, remote-secondmates, and trace-context work) into the captain's local feature-rich line (commit aa8b69a, containing beads backend, --account multi-account launcher, Parlay enrollment, persona.md, watcher beacon-based idle detection, fm-send --raw, decision-holds, and more), producing a superset branch imerge-result that preserves all local features while gaining all upstream changes. The developer required that every conflict be resolved by intent—preserving both sides' functionality rather than blindly taking one side—and that the result never be pushed or turned into a PR. The final deliverable was a written report documenting the merge process, conflict blocks, superset verification results, and an honest assessment of whether git-imerge's incremental model was useful and whether the result could be trusted as the reconciled main.
What Changed
Final changed paths and statuses:
Risk Assessment
✅ Low: All four Round 1 fixes were correctly applied — probe guards, corrected comment, supervision-lib migration, and backend fixture — with no new defects introduced and no remaining reachable failure paths within scope.
Testing
Exercised the core behavioral fix (binary probe instead of uname-based dialect dispatch) directly via 10 focused assertions all passing, and validated 4 modified test fixtures with the new fm-stat-lib.sh dependency. The one pre-existing flaky failure in fm-remote-job ("queue time consumed the second job's execution timeout") reproduces identically on the base commit and is not a regression.
Evidence: stat-dialect behavioral test
ok - dialect probe returns valid flavor: 'bsd' ok - FM_STAT_DIALECT_OVERRIDE=gnu honored ok - FM_STAT_DIALECT_OVERRIDE=bsd honored ok - invalid override exits non-zero ok - fm_stat_mtime /tmp = 1770268403 (integer epoch seconds) ok - fm_stat_size /tmp = 11 (integer bytes) ok - BSD stat: -c %s / rejects cleanly (no stdout pollution, so probe order is safe) ok - fm_stat_identity /tmp = '16777233:1152921500312561831' (device:inode pair) ok - _FM_STAT_DIALECT cached after direct call: 'bsd' ok - fm_stat_mtime works on a real file: before=1786134240 after=1786134241 --- Total: 10 | Passed: 10 | Failed: 0Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed ✅
bin/fm-busy-event.sh:105- fm-busy-event.sh:105 still usesstat -f %m ... || stat -c %Y ..., the exact anti-pattern the PR header explicitly calls 'worse' than uname-based. On GNU-on-Darwin,stat -f %m "$LOCK"exits 0 with a multi-line filesystem dump; the||never fires;mtimegets non-integer text;age=$((now - mtime))then fails under arithmetic or produces garbage, preventing the stale-lock reap in the busy-event arm loop. This file was not touched by the diff and is pre-existing, but the same authorized failure (wrong stat on GNU-on-Darwin) remains reachable through it.bin/fm-supervision-lib.sh:19- fm-supervision-lib.sh:19-22 still uses uname-based stat dialect detection. On GNU-on-Darwin (nix-darwin or Homebrew coreutils ahead of /usr/bin),stat -f %mexits 0 with an apfs filesystem dump;[ -n "$m" ]passes;age=$(( $(date +%s) - m ))fails under arithmetic or computes 0;FM_SUP_WATCHER_FRESHmay stay false or be set incorrectly, causing false guard fires. Same class as robots-e8x5, still reachable in this unchanged file.bin/fm-stat-lib.sh:46- The _FM_STAT_DIALECT cache only persists within a subshell and does not propagate to the parent shell. Everyfm_stat_mtime/fm_stat_fmtcall invoked via command substitution (result=$(fm_stat_mtime ...)) creates a new subshell where the cache is empty, so thestat -c %s /probe re-runs on every call. The header comment ('cached in _FM_STAT_DIALECT after the first probe... forking a probe per call is a measurable cost') overstates the benefit. Functionally correct, but the per-call fork is not eliminated in the common caller pattern.bin/fm-config-inherit-lib.sh:113- bin/fm-config-inherit-lib.sh:113 and bin/fm-startup-memory-budget-lib.sh:25 retain uname-based stat dialect selection (fm_inherit_file_link_count/fm_startup_memory_budget_link_count), two more pre-existing call sites with the same failure mode on GNU-on-Darwin. Not introduced by this diff but worth noting for follow-up sweep.🔧 Fix: guard set-e probes, fix comment, migrate supervision-lib, fix backend fixture
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash /var/folders/.../stat-dialect-probe-test.sh— 10 direct behavioral assertions on fm-stat-lib.sh: dialect probe, override mechanism, fm_stat_mtime/size/identity, probe order safety, caching, and mtime change tracking — all passbin/fm-test-run.sh tests/fm-remote-entrypoint.test.sh tests/fm-gotmp.test.sh— both pass (fm-stat-lib.sh added to fixture sets)bin/fm-test-run.sh tests/fm-afk-return.test.sh— 5/5 pass (fm-stat-lib.sh added to install_runner)bin/fm-test-run.sh tests/fm-remote-job.test.sh— pre-existing failure confirmed on base commit df7e4a9c (not a regression)Base-commit baseline:git checkout df7e4a9c -- tests/fm-remote-job.test.sh bin/fm-remote-job-lib.sh && bin/fm-test-run.sh tests/fm-remote-job.test.sh— same failure reproduced, confirming no regression✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
New Features
Documentation
Tests