Skip to content

fix(watch): make hash_pane's last-resort tier content-sensitive - #23

Merged
trillium merged 2 commits into
mainfrom
fm/hashpane-crfix-address-coderabbit-findings-on-merged-pr-43
Aug 1, 2026
Merged

fix(watch): make hash_pane's last-resort tier content-sensitive#23
trillium merged 2 commits into
mainfrom
fm/hashpane-crfix-address-coderabbit-findings-on-merged-pr-43

Conversation

@trillium

@trillium trillium commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Intent

Address 3 CodeRabbit findings on the already-merged PR #21 (hash_pane PATH-gap fallback chain) as a forward-fix, branched from current origin/main which includes #21.

  1. MAJOR - content-sensitive final fallback: the last-resort wc -c tier in hash_pane() returned a byte COUNT, not a content hash, so two panes with the same size but different content collided and change-detection missed updates. Fixed by replacing the wc -c last tier with a pure-shell content digest: od byte-dumps the input and an awk rolling hash (h = h*31 + byte, mod a large prime) folds it into a stable, content-sensitive token, printed as hex. This keeps hashing correct even when md5/md5sum/openssl/shasum/cksum are all unavailable.

  2. MAJOR - host-independent test: the FM_MD5_SBIN_OVERRIDE / sbin-md5 test previously required the runner's ambient /sbin/md5 (or a PATH md5 fallback), which is absent on non-BSD CI runners and unreliable across dev hosts (one dev host's ambient md5 turned out to be GNU coreutils md5sum aliased as md5, which doesn't support -q). Fixed by replacing it with a self-contained fake md5 -q stand-in script (make_fake_sbin_md5): it ignores its arguments and echoes stdin back with a fixed tag prefix, so the test only needs stable-per-input/distinct-per-input behavior, not a real hash, and no longer depends on any host tool.

  3. MINOR - cksum coverage: added a direct test (test_hash_pane_falls_back_past_shasum_to_cksum) that isolates the cksum tier (md5/md5sum/openssl/shasum absent) and asserts hash_pane's output matches cksum's own transform, plus a distinctness check.

Constraints: changes are localized to hash_pane() in bin/fm-watch.sh and its test tests/fm-hash-pane.test.sh only. make_fakebin's baseline symlink set was extended to include od/awk/cat, since the new last-resort tier needs od+awk and the fake sbin-md5 script needs cat - these are tools every test host is expected to have, matching the existing cut/tr/wc baseline. The final no-hash-tool test was also strengthened to assert content-sensitivity (same-length, different-content inputs must hash differently), as a regression check for finding 1. All changes follow firstmate-coding-guidelines (this is firstmate's own shared tracked material). tests/fm-hash-pane.test.sh is green (5/5) and bin/fm-lint.sh is clean.

What Changed

  • Replaced hash_pane()'s final fallback tier in bin/fm-watch.sh (used when md5/md5sum/openssl/shasum/cksum are all unavailable) from a raw wc -c byte count to a content-sensitive pure-shell digest built with od byte-dumping and an awk rolling hash, preventing same-size/different-content panes from colliding and masking real changes.
  • Updated docs/configuration.md's FM_MD5_SBIN_OVERRIDE doc comment to describe the new od+awk content-digest last resort instead of the stale wc -c description.
  • Updated tests/fm-hash-pane.test.sh: dropped the now-unused wc test fixture/symlink, added coverage for the cksum fallback tier, and strengthened the no-hash-tool test to assert content-sensitivity (same-length, different-content inputs hash differently).

Risk Assessment

✅ Low: Change is confined to hash_pane()'s last-resort tier and its tests; the od+awk rolling-hash replacement is content-sensitive (uses od -v to avoid line-collapsing, correct modulus arithmetic within double precision), the doc comment matches the new behavior, and the test suite was updated consistently with no leftover stale references to the old wc-based tier.

Testing

Ran the project's tests/fm-hash-pane.test.sh suite (all 5 tests pass, including the new cksum-tier test and the strengthened no-hash-tool content-sensitivity check), then independently exercised hash_pane() by hand in a bash script (sourcing bin/fm-watch.sh) with PATH stripped to only cut/tr/od/awk/cat, proving the last-resort tier is now content-sensitive (distinct hashes for same-length different-content input) and still stable for repeated identical input — directly confirming the MAJOR finding's fix. The other two findings (host-independent sbin-md5 test via a fake stand-in script, and dedicated cksum-tier coverage) are exercised by the passing test suite itself.

Evidence: Manual hash_pane content-sensitivity check script and output

Simulating a watcher whose PATH lacks every real hash tool: hash('pane text a') = b1c0d0ef hash('pane text b') = b1c0d10e hash('pane text a') again = b1c0d0ef PASS: last-resort tier is content-sensitive and stable

#!/bin/bash
# Manual end-to-end demonstration of hash_pane()'s content-sensitive
# last-resort tier (no md5/md5sum/openssl/shasum/cksum on PATH).
set -eu

REPO_ROOT=$1
FAKEBIN=$2
HOME_DIR=$3

mkdir -p "$FAKEBIN" "$HOME_DIR"
for t in cut tr od awk cat; do
  ln -sf "$(command -v "$t")" "$FAKEBIN/$t"
done

export FM_HOME="$HOME_DIR"
. "$REPO_ROOT/bin/fm-watch.sh" 2>/dev/null || true

echo "Simulating a watcher whose PATH lacks every real hash tool:"
out_a=$(PATH="$FAKEBIN" FM_MD5_SBIN_OVERRIDE="$FAKEBIN/no-such-md5" hash_pane <<EOF
pane text a
EOF
)
out_b=$(PATH="$FAKEBIN" FM_MD5_SBIN_OVERRIDE="$FAKEBIN/no-such-md5" hash_pane <<EOF
pane text b
EOF
)
out_a2=$(PATH="$FAKEBIN" FM_MD5_SBIN_OVERRIDE="$FAKEBIN/no-such-md5" hash_pane <<EOF
pane text a
EOF
)

echo "hash('pane text a')       = $out_a"
echo "hash('pane text b')       = $out_b"
echo "hash('pane text a') again = $out_a2"

if [ "$out_a" = "$out_b" ]; then
  echo "FAIL: same-length different-content inputs collided (pre-fix wc -c behavior)"
  exit 1
fi
if [ "$out_a" != "$out_a2" ]; then
  echo "FAIL: same input produced different hashes (not stable)"
  exit 1
fi
echo "PASS: last-resort tier is content-sensitive and stable"

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 2 issues found → auto-fixed ✅
  • ⚠️ docs/configuration.md:442 - The FM_MD5_SBIN_OVERRIDE doc comment still says hash_pane's last tier is 'a wc -c last resort that cannot itself be absent' — that's the exact byte-count behavior this PR just fixed. The comment wasn't updated to describe the new od+awk content-sensitive fallback, so it now documents the pre-fix (buggy) behavior.
  • ℹ️ tests/fm-hash-pane.test.sh:24 - REAL_WC and its make_fakebin symlink are dead weight now: hash_pane() no longer uses wc in any tier (the last resort switched from wc -c to od | awk), so resolving/symlinking wc in every test's fakebin no longer serves a purpose.

🔧 Fix: fix stale hash_pane doc and drop unused wc test fixture
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-hash-pane.test.sh (5/5 passing: sbin-md5, openssl, shasum, cksum, and no-hash-tool tiers)
  • Manual script exercising hash_pane() directly with PATH stripped of md5/md5sum/openssl/shasum/cksum: confirmed 'pane text a' and 'pane text b' (same length, different content) now hash to distinct values (b1c0d0ef vs b1c0d10e), and identical input reproduces the same hash — demonstrating finding 1's fix (the old wc -c tier would have returned the identical byte count 12 for both, masking a real pane change)
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

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.
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@trillium, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 484e1b2c-ede4-470b-9664-146b28d08f34

📥 Commits

Reviewing files that changed from the base of the PR and between 1350b11 and 5a54c4f.

📒 Files selected for processing (3)
  • bin/fm-watch.sh
  • docs/configuration.md
  • tests/fm-hash-pane.test.sh

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@trillium
trillium merged commit 1e11ba8 into main Aug 1, 2026
10 of 11 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.

1 participant