fix(watch): harden hash_pane() fallback chain against PATH gaps - #21
Merged
trillium merged 2 commits intoAug 1, 2026
Merged
Conversation
hash_pane() hard-errored on every poll cycle when a watcher's PATH lacked both md5 and md5sum (observed on a secondmate whose runtime PATH omitted both, needing ~3 manual watcher restarts). Add an FM_MD5_SBIN_OVERRIDE test seam for the existing hardcoded /sbin/md5 check, and extend the fallback chain past md5sum to openssl dgst -md5, then shasum, then cksum, then a wc -c last resort that cannot itself be absent. The hash is only compared for change-detection between polls, so any tool that returns a stable token in its first whitespace-delimited field is interchangeable. Covered by tests/fm-hash-pane.test.sh.
….md env var reference
📝 WalkthroughWalkthrough
ChangesHash pane fallback support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@bin/fm-watch.sh`:
- Around line 197-198: Update the final fallback in hash_pane within
bin/fm-watch.sh at lines 197-198 to produce a content-sensitive token, or
explicitly return and handle an unavailable result instead of using wc -c;
update docs/configuration.md at line 442 to accurately describe the selected
behavior, and extend tests/fm-hash-pane.test.sh at lines 113-123 to verify that
equal-length, different inputs yield distinct results when checksum tools are
unavailable.
In `@tests/fm-hash-pane.test.sh`:
- Around line 96-124: Add a dedicated test alongside the existing hash_pane
fallback tests that constructs a fake PATH containing only cksum, invokes
hash_pane with the md5 override unavailable, and verifies successful execution
with the expected normalized checksum output produced by cksum. Keep the test
focused on the cksum tier and mirror the setup and assertions used by
test_hash_pane_falls_back_past_openssl_to_shasum.
- Around line 25-26: Update the sbin-md5 test setup around REAL_SBIN_MD5 and the
FM_MD5_SBIN_OVERRIDE seam so it does not depend on host availability or behavior
of /sbin/md5 or md5. Provide a test-local executable that supports -q, or
feature-probe the resolved executable and skip the affected test when
unavailable; ensure the setup used by the test at lines 66-70 never fails the
entire file on unsupported hosts.
🪄 Autofix (Beta)
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: 9d18c426-b332-4718-8d1d-3be26154ab0a
📒 Files selected for processing (4)
bin/fm-test-run.shbin/fm-watch.shdocs/configuration.mdtests/fm-hash-pane.test.sh
Comment on lines
+197
to
+198
| else | ||
| wc -c | tr -d ' ' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use a content-sensitive final fallback.
wc -c returns only the byte count. Two different pane states with the same length produce the same token, so hash_pane() can miss a poll-to-poll change and suppress required watcher handling.
bin/fm-watch.sh#L197-L198: replace the byte-count fallback with a content-sensitive token, or define and handle an explicit unavailable result.docs/configuration.md#L442-L442: document the selected behavior accurately. Do not describe a byte count as reliable pane hashing.tests/fm-hash-pane.test.sh#L113-L123: assert that two different equal-length inputs produce distinct results when checksum tools are unavailable.
📍 Affects 3 files
bin/fm-watch.sh#L197-L198(this comment)docs/configuration.md#L442-L442tests/fm-hash-pane.test.sh#L113-L123
🤖 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 `@bin/fm-watch.sh` around lines 197 - 198, Update the final fallback in
hash_pane within bin/fm-watch.sh at lines 197-198 to produce a content-sensitive
token, or explicitly return and handle an unavailable result instead of using wc
-c; update docs/configuration.md at line 442 to accurately describe the selected
behavior, and extend tests/fm-hash-pane.test.sh at lines 113-123 to verify that
equal-length, different inputs yield distinct results when checksum tools are
unavailable.
Comment on lines
+25
to
+26
| REAL_SBIN_MD5=/sbin/md5 | ||
| [ -x "$REAL_SBIN_MD5" ] || REAL_SBIN_MD5=$(command -v md5 || true) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the sbin-md5 test independent of host tools.
On hosts without /sbin/md5 or md5, Line 66 fails the whole test file. A resolved md5 path also does not prove that the executable supports -q.
Use a test-local executable that accepts -q, or feature-probe a real executable and skip this case when unavailable. The FM_MD5_SBIN_OVERRIDE seam supports a local fixture.
Also applies to: 66-70
🤖 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/fm-hash-pane.test.sh` around lines 25 - 26, Update the sbin-md5 test
setup around REAL_SBIN_MD5 and the FM_MD5_SBIN_OVERRIDE seam so it does not
depend on host availability or behavior of /sbin/md5 or md5. Provide a
test-local executable that supports -q, or feature-probe the resolved executable
and skip the affected test when unavailable; ensure the setup used by the test
at lines 66-70 never fails the entire file on unsupported hosts.
Comment on lines
+96
to
+124
| test_hash_pane_falls_back_past_openssl_to_shasum() ( | ||
| local home fakebin out expected status | ||
| home="$TMP_ROOT/no-md5-no-openssl" | ||
| mkdir -p "$home" | ||
| source_watch "$home" | ||
| [ -n "$REAL_SHASUM" ] || fail "shasum not available to exercise the fallback tier" | ||
| fakebin=$(make_fakebin "$home/fakebin" "shasum=$REAL_SHASUM") | ||
| status=0 | ||
| out=$(PATH=$fakebin FM_MD5_SBIN_OVERRIDE="$home/no-such-md5" hash_pane <<<"pane text a") || status=$? | ||
| expect_code 0 "$status" "hash_pane exit with md5/md5sum/openssl absent from PATH" | ||
| [ -n "$out" ] || fail "hash_pane produced empty output with md5/md5sum/openssl absent from PATH" | ||
| expected=$(printf 'pane text a\n' | "$REAL_SHASUM" | "$REAL_CUT" -d' ' -f1) | ||
| [ "$out" = "$expected" ] \ | ||
| || fail "hash_pane did not use the shasum fallback tier past openssl (got '$out', wanted '$expected')" | ||
| pass "hash_pane falls back past openssl to shasum and still exits cleanly" | ||
| ) | ||
|
|
||
| test_hash_pane_never_hard_errors_with_no_hash_tool_at_all() ( | ||
| local home fakebin out status | ||
| home="$TMP_ROOT/no-hash-tool" | ||
| mkdir -p "$home" | ||
| source_watch "$home" | ||
| fakebin=$(make_fakebin "$home/fakebin") | ||
| status=0 | ||
| out=$(PATH=$fakebin FM_MD5_SBIN_OVERRIDE="$home/no-such-md5" hash_pane <<<"pane text a") || status=$? | ||
| expect_code 0 "$status" "hash_pane exit with no md5/md5sum/openssl/shasum/cksum on PATH" | ||
| [ -n "$out" ] || fail "hash_pane produced empty output with no hash tool at all on PATH" | ||
| pass "hash_pane never hard-errors even with no hashing tool at all on PATH" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add direct coverage for the cksum tier.
Lines 195-196 in bin/fm-watch.sh add a distinct output conversion path. The tests exercise openssl, shasum, and no-tool behavior, but they never provide only cksum on PATH.
Add a fake-bin case with only cksum available. Assert that hash_pane() returns the expected normalized checksum.
🤖 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/fm-hash-pane.test.sh` around lines 96 - 124, Add a dedicated test
alongside the existing hash_pane fallback tests that constructs a fake PATH
containing only cksum, invokes hash_pane with the md5 override unavailable, and
verifies successful execution with the expected normalized checksum output
produced by cksum. Keep the test focused on the cksum tier and mirror the setup
and assertions used by test_hash_pane_falls_back_past_openssl_to_shasum.
trillium
added a commit
that referenced
this pull request
Aug 1, 2026
* fix(watch): make hash_pane's last-resort tier content-sensitive Address 3 CodeRabbit findings on merged PR #21 (hash_pane PATH-gap fallback chain): - MAJOR: the last-resort tier returned wc -c's raw byte count, so two panes of equal size but different content collided and change detection missed real updates. Replace it with a pure-shell content digest (od byte dump piped through an awk rolling hash) so same-size differing content can no longer collide. - MAJOR: the sbin-md5 test depended on the runner's ambient /sbin/md5 or PATH md5, which is absent on non-BSD hosts. Replace it with a self-contained fake md5 -q stand-in script so the test no longer depends on host tooling. - MINOR: add direct test coverage for the cksum tier. make_fakebin's baseline symlink set grows to include od/awk/cat, which the new last-resort tier and the fake sbin-md5 script both need. * no-mistakes(review): fix stale hash_pane doc and drop unused wc test fixture
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Harden bin/fm-watch.sh's hash_pane() against PATH gaps so it never hard-errors when md5/md5sum aren't on PATH. Root cause: the herdr secondmate hit repeated watcher FAILED cycles because its watcher process's PATH lacked md5/md5sum, so hash_pane() errored on every poll cycle, needing ~3 manual watcher restarts that session. On the dev machine md5, md5sum, and openssl all exist at /opt/homebrew/bin (and in a clean login shell md5 is /sbin/md5, md5sum is /run/current-system/sw/bin/md5sum, openssl is /usr/bin/openssl) - the root cause is a PATH gap in the watcher's runtime environment, not a missing binary. Fix: extended hash_pane()'s fallback chain past md5/(hardcoded /sbin/md5)/md5sum to add openssl dgst -md5, then shasum, then cksum, then a wc -c last resort that cannot itself be absent - so hash_pane always returns a stable hex-ish token and never hard-errors. Also made the existing hardcoded /sbin/md5 check overridable via FM_MD5_SBIN_OVERRIDE so it can be exercised deterministically in tests. hash_pane's output is only compared for poll-to-poll change detection, so any tool returning a stable token in its first whitespace-delimited field is interchangeable - exact hash algorithm/format does not need to match across tools within a single watcher process, since the tool selection is stable for the life of that process. Constraint: this collides with a standing refactor 'Slice 4' (task-tozf) that will rewrite this same watcher script, so the change was kept deliberately minimal and localized to hash_pane() only - no changes to polling logic, beacon, or lifecycle. Added tests/fm-hash-pane.test.sh (4 cases) covering: stable/distinct hashing via the sbin md5 tier, falling back to openssl when md5/md5sum are absent from PATH, falling back further to shasum when openssl is also absent, and never hard-erroring even with no hash tool at all on PATH (verified against a known pre-existing dev-machine quirk where the ambient PATH's 'md5' resolves to a homebrew symlink of GNU gmd5sum lacking -q support, unrelated to this fix). Registered the new test file in bin/fm-test-run.sh's watcher-wake-lock family. Known pre-existing, out-of-scope issue: tests/fm-watch-triage.test.sh fails on at least one dev machine due to that same ambient 'md5 -q' shadowing quirk - reproduced identically on the unmodified base branch before this change, so it is not a regression introduced here and should not be fixed as part of this task.
What Changed
hash_pane()inbin/fm-watch.shwith a longer fallback chain (pastmd5/hardcoded/sbin/md5/md5sum) to tryopenssl dgst -md5, thenshasum, thencksum, then awc -clast resort that can never itself be absent, so the watcher never hard-errors on a poll cycle when no hash tool is onPATH./sbin/md5check overridable via a newFM_MD5_SBIN_OVERRIDEenvironment variable, documented indocs/configuration.md, so the fallback chain can be exercised deterministically in tests.tests/fm-hash-pane.test.sh(4 cases covering stable/distinct hashing and successive fallback tiers) and registered it inbin/fm-test-run.sh'swatcher-wake-locktest family.Risk Assessment
✅ Low: Change is minimal and localized to hash_pane()'s tool-resolution fallback chain; the hash value is only ever compared for byte-equality against a persisted previous value (no callers assume a particular format/length), the override env var preserves default behavior (/sbin/md5) when unset, no set -e/pipefail issues introduced, and the new test file exercises each fallback tier plus the never-hard-error guarantee.
Testing
Ran the new tests/fm-hash-pane.test.sh directly and via bin/fm-test-run.sh (all 4 cases pass, confirming hash_pane's fallback chain through sbin-md5, openssl, shasum, and a wc-based last resort never hard-errors and stays stable/input-sensitive), verified the test is properly registered in the watcher-wake-lock family, and confirmed via a base-commit worktree comparison that the unrelated fm-watch-triage.test.sh failure (ambient PATH md5 -q shadowing quirk) pre-dates this change and is not a regression.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-hash-pane.test.sh (direct run, 4/4 pass)bin/fm-test-run.sh tests/fm-hash-pane.test.sh (via runner, exit=0)bin/fm-test-run.sh --family watcher-wake-lock --list (confirms fm-hash-pane.test.sh registration)bash tests/fm-watch-triage.test.sh on target commit a7d6124 (reproduces pre-existing md5 -q failure)bash tests/fm-watch-triage.test.sh on base commit f988287 in isolated worktree (reproduces identical failure, confirming not a regression)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests