fix(bin): prevent inbox note loss on macOS - #2793
Closed
mremond wants to merge 3 commits into
Closed
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (4): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
mremond
force-pushed
the
fm/inbox-sed-macos
branch
from
August 23, 2026 19:23
c18d5ef to
1eac645
Compare
`fm-inbox.sh note` wrote the record with an `id=PENDING` placeholder and patched it afterwards with `sed -i "<expr>" "<file>"`. That is GNU syntax: BSD/macOS sed reads the argument after `-i` as the backup SUFFIX, so it took the expression for a suffix and the path for a script and refused. Every note taken on macOS was lost, and `list` showed an empty inbox. Write the record once with its final id. The id is derived from the mktemp staging name, so it is already known before the record is written; no in-place rewrite is needed on either platform, and the second pass that could half-succeed is gone rather than made conditional. The worse half was the reporting. The command already exited non-zero through `set -e`, but it said only `sed: 1: ...: invalid command code m`, named nothing the captain could act on, and left an orphaned staging file in the inbox. An out-of-band capture surface that loses what it was handed has to say so: every step that can fail now reports through the script's own `fm-inbox:` diagnostic, states that nothing was queued, and removes the staging file, so a refused note leaves no record, no litter, and no wake for a note that does not exist. tests/fm-inbox.test.sh pins the round trip rather than the exit status: a `note` that reports success must leave a record `list` reads back, carrying the reported id and no placeholder, and a `note` that cannot write must be readable, non-zero, and leave nothing behind. CONTRIBUTING.md gains the portability rule the rest of bin/ already follows.
mremond
force-pushed
the
fm/inbox-sed-macos
branch
from
August 23, 2026 20:29
1eac645 to
bb47ec0
Compare
Contributor
Author
|
Superseded by #2857, which landed the same fix: the note id is computed before the file is written and the Closing this in favour of that one. Thanks. |
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
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
The captain's out-of-band capture surface, bin/fm-inbox.sh, wrote NO note at all on macOS. Repair it.
Reproduction, made on 2026-08-22 on freshly merged commit 1231b6a, a single command:
bin/fm-inbox.sh note 'texte'
-> sed: 1: "/Users/...": invalid command code m
-> bin/fm-inbox.sh list -> (inbox empty)
TWO DEFECTS, NOT ONE. Do not repair only the first.
THE CAUSE, line 184:
sed -i "s/^id=PENDING$/id=$id/" "$tmp". That is GNU syntax. BSD/macOS sed reads the argument after -i as the backup SUFFIX: it therefore takes the expression for a suffix and the file path for a script. The repo targets macOS AND Linux; the correction must work on both, without trading one platform for the other. A rewrite that does not use -i at all is probably simpler and safer than a per-platform branch - the captain left the judgment to me, but asked me to say why.THE MORE SERIOUS DEFECT, and the one that counts: the command FAILED and said nothing clear. It printed a cryptic sed message, wrote no note, and the captain could have believed his note was filed. An out-of-band capture that silently loses what it is handed is worse than no capture at all. Verify what the exit code returns today, and make a write failure a VISIBLE FAILURE: nothing half-published, a readable message, a non-zero exit code.
LOOK FOR THE SAME PATTERN ELSEWHERE: this file may not be the only one in bin/ using GNU-only syntax. Inventory them, fix those genuinely broken on macOS, and NAME in the report the ones left alone and why. Do not widen beyond bin/.
TESTS: the repo has a tests/ directory with one test per script. Check whether a test already exists for fm-inbox.sh; if not, write one. The expected evidence is that a
notereally writes a record readable bylist, and that the identifier is really substituted there - not merely that the command exits zero. PROVE BY MUTATION: reintroduce the defect and show the test falls.CONSTRAINT: touch nothing under projects/.
DEFINITION OF DONE
On this machine,
bin/fm-inbox.sh note 'texte'writes the note,listshows it, and the wake is deposited. A write failure is visible and non-zero. The test exists and bites. The other GNU-only usages in bin/ are inventoried. Green PR.--- DECISIONS AND TRADEOFFS MADE WHILE DOING THE WORK (deliberate; not accidents in the diff) ---
DECISION 1 - remove
sed -ientirely rather than branch on uname, and drop the two-pass write.The id is derived from the mktemp staging filename, so it is ALREADY KNOWN before the record is written. The placeholder-then-patch design never had a reason to exist. So the record is now written ONCE with its final id: no -i, no platform branch, and one fewer step that can half-succeed. A
uname = Darwinbranch would have preserved a purposeless second pass. This is the "say why" the captain asked for.DECISION 2 - a correction to the brief's premise, deliberately kept in the change and in the commit message.
The brief assumed the exit code might be zero. It is not: the command already exited 1 through
set -euo pipefail. I verified this before changing anything. So defect 2 was NOT a wrong exit code - it was that the failure printed only raw sed jargon (sed: 1: "/Users/...": invalid command code m), named nothing the captain could act on, and left an orphaned.staging-XXXXXXfile in the inbox directory. The fix therefore targets attribution, readability and cleanup, not the exit status: every step that can fail now reports through the script's ownfm-inbox:diagnostic, states "nothing was queued", and removes the staging file. This is why the diff adds explicit|| dieguards to steps thatset -ealready covered - that redundancy is deliberate, becauseset -egives a correct exit code with an unusable message.DECISION 3 - the write is an explicit &&-chain rather than a plain command group.
{ printf...; printf...; } >"$tmp"reports only the LAST command's status, so a disk that fills mid-record would publish a truncated note. Chaining every write with && makes the group's status the first failure. This is a deliberate hardening of the same "nothing half-published" requirement, not incidental restyling.DECISION 4 - the failure paths remove the staging file explicitly instead of using a trap.
An EXIT trap in queue_note would have been shorter, but
cmd_sayalready installs its own EXIT trap for the audio temp files and calls queue_note; a trap here would silently override it and leak the captain's audio files. Explicitrm -f "$tmp"on each failure path avoids that regression.DECISION 5 - GNU-only sweep of bin/, deliberately NOT widened into a Linux-correctness sweep.
sed -iwas the only genuinely macOS-broken usage. All ~40stat -csites are insideuname = Darwinbranches;timeoutis owned by bin/fm-timeout-lib.sh;readlink -fis deliberately avoided in bin/fm-cursor-lib.sh; everysha256sumhas acommand -vguard. Four sites were inspected and DELIBERATELY LEFT ALONE, per the brief's "name the ones you leave and why":stat -f ... || stat -c ...form that bin/fm-watch.sh:116 and bin/fm-fleet-snapshot.sh:827 explicitly warn against. They are CORRECT on macOS (the BSD arm wins). The suspect platform is Linux, which I could not run here, and both validate the resulting digits afterwards. Fixing them is a Linux-correctness change outside this macOS repair.stat -fbefore using it. Both correct on both platforms.These are intentionally absent from the diff. Do not flag their absence as an oversight.
DECISION 6 - the test pins the ROUND TRIP, not the exit status, because the brief demanded exactly that.
tests/fm-inbox.test.sh did not exist. It has seven cases: the note/list round trip; the substituted id with no PENDING placeholder left plus the matching wake; a multiline body with shell metacharacters; an unwritable inbox failing readably and non-zero with no record, no litter and no wake; a failed publish (via a failing
mvPATH shim) leaving no staging file; drain --ack; and the empty-note refusal. Themvshim exists specifically to make the "no staging litter" assertion non-vacuous - without it, mktemp fails first and no staging file would ever have existed to leak.MUTATION PROOF, run before committing, three separate reintroductions each biting a DIFFERENT assertion:
sed -iid rewrite restored -> round-trip test fails ("note failed on a writable home", sed: 1: ... invalid command code f)All seven pass on the fix.
DECISION 7 - two lines added to CONTRIBUTING.md, deliberately scoped.
The whole of bin/ already follows the macOS/Linux portability convention, but nobody had ever written it down, which is how this slip got in. Two sentences were added to the existing
bin/bullet under "Repo conventions" - patching existing language rather than adding a section, per the firstmate-coding-guidelines one-owner and size-discipline rules. It points at bin/fm-supervision-lib.sh as the branch example and at bin/fm-watch.sh / bin/fm-fleet-snapshot.sh as the existing owners of the BSD-first-fallback hazard rather than restating it. AGENTS.md was deliberately NOT touched: a bash portability rule is not needed by every session, so it belongs on the contributor surface, not in the always-loaded agent job description.VERIFICATION ALREADY RUN LOCALLY: bin/fm-lint.sh clean (shellcheck 0.11.0 + actionlint 1.7.12, which I had to install locally as it was missing); bin/fm-doc-audience-check.sh ok (73 surfaces, 266 links); bin/fm-test-run.sh --check-coverage ok (total=155); tests/fm-inbox.test.sh, tests/fm-test-run.test.sh, tests/fm-documentation-audiences.test.sh and tests/fm-lint.test.sh all pass.
What Changed
sed -irewrite that broke note capture on macOS.fm-inboxdiagnostics, remove staging files, and avoid publishing partial records or depositing wakes for failed notes.bin/scripts.Risk Assessment
✅ Low: Captain, the macOS portability fix is well-bounded, preserves atomic publication, visibly handles pre-publication failures, and adds behavioral coverage on the affected platform.
Testing
The supplied baseline reported clean lint, documentation, coverage registration, and adjacent tests; this phase independently passed the focused inbox suite, demonstrated the complete note/list/persisted-notification path and visible write failure on macOS, and proved the regression test fails when the original GNU-only implementation is restored. CLI transcripts and persisted state were captured; no screenshot was appropriate for this CLI-only change.
Evidence: macOS note/list and write-failure transcript
Source: macOS note/list and write-failure transcript
Evidence: Original-defect reproduction and mutation proof
Source: Original-defect reproduction and mutation proof
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
bin/fm-inbox.sh:184- Required criterion: “every step that can fail now reports through the script's ownfm-inbox:diagnostic, states ‘nothing was queued’, and removes the staging file.” The changed line still runs the seconddateunguarded. Ifdate -usucceeds butdate +%sfails,set -eexits aftermktemp, leaving.staging-*behind with no script-owned diagnostic, no record, and no wake. Guard the epoch read at this pre-publish boundary, clean$tmp, and calldie.tests/fm-inbox.test.sh:41- The regression test only catches the originalsed -imutation on BSD/macOS; restored GNU syntax succeeds in the Linux behavior lanes. The macOS CI job syntax-checks this file but does not execute it, so the exact defect can return while CI remains green. Run this focused test in the stock macOS job as well as the existing Linux lane.🔧 Fix: Guard inbox epoch reads and test on macOS
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
/bin/bash tests/fm-inbox.test.shManual macOSFM_HOME=<isolated-home> bin/fm-inbox.sh note texte, thenlistand inspection of the persisted note and notificationManual denied-write check using a mode-0500 inbox, verifying exit 1, readablefm-inbox:diagnostic, no record/staging litter, and no notificationMutation check using base-commitbin/fm-inbox.shwith currenttests/fm-inbox.test.sh, verifying failure under BSD sedBoundedbin/portability inventory usingrgand contextual inspection of retainedstatsites✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.