Skip to content

fix(bin): prevent inbox note loss on macOS - #2793

Closed
mremond wants to merge 3 commits into
kunchenguid:mainfrom
mremond:fm/inbox-sed-macos
Closed

fix(bin): prevent inbox note loss on macOS#2793
mremond wants to merge 3 commits into
kunchenguid:mainfrom
mremond:fm/inbox-sed-macos

Conversation

@mremond

@mremond mremond commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

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.

  1. 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.

  2. 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 note really writes a record readable by list, 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, list shows 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 -i entirely 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 = Darwin branch 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-XXXXXX file 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 own fm-inbox: diagnostic, states "nothing was queued", and removes the staging file. This is why the diff adds explicit || die guards to steps that set -e already covered - that redundancy is deliberate, because set -e gives 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_say already 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. Explicit rm -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 -i was the only genuinely macOS-broken usage. All ~40 stat -c sites are inside uname = Darwin branches; timeout is owned by bin/fm-timeout-lib.sh; readlink -f is deliberately avoided in bin/fm-cursor-lib.sh; every sha256sum has a command -v guard. Four sites were inspected and DELIBERATELY LEFT ALONE, per the brief's "name the ones you leave and why":

  • bin/fm-x-lib.sh:421 and bin/fm-busy-event.sh:106 use the BSD-first 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.
  • bin/fm-test-run.sh:1648 is GNU-first with a BSD fallback, and bin/fm-test-isolation-proof.sh:212 probes stat -f before 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 mv PATH shim) leaving no staging file; drain --ack; and the empty-note refusal. The mv shim 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:

  • original sed -i id rewrite restored -> round-trip test fails ("note failed on a writable home", sed: 1: ... invalid command code f)
  • unguarded mktemp restored -> "the failure carried no fm-inbox diagnostic" fails
  • failed publish skips staging cleanup -> "a failed publish left 1 staging file(s) in the inbox" fails
    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

  • Write inbox records once with their final identifier, removing the GNU-only sed -i rewrite that broke note capture on macOS.
  • Report capture failures with clear fm-inbox diagnostics, remove staging files, and avoid publishing partial records or depositing wakes for failed notes.
  • Add seven inbox behavior tests to the stock macOS Bash CI job and document cross-platform conventions for 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

$ FM_HOME=<isolated-home> bin/fm-inbox.sh note texte
queued 1787399839-hNup1H
  texte
  firstmate will pick this up at its next check.
exit=0

$ FM_HOME=<isolated-home> bin/fm-inbox.sh list
1787399839-hNup1H
    texte

$ persisted note record
id=1787399839-hNup1H
at=2026-08-22T11:57:19Z
source=text
--
texte

$ persisted wake entry
1787399839	1	check	inbox:1787399839-hNup1H	check: captain inbox note 1787399839-hNup1H - texte

$ FM_HOME=<read-only-inbox-home> bin/fm-inbox.sh note "must not vanish"
mktemp: mkstemp failed on /var/folders/yk/hpylg6r976qgbphydj66gt9h0000gn/T/no-mistakes-evidence/01M0MMAKDXXGB9QRWAW19BR92K/e2e-failure-home/state/inbox/.staging-ONuIgL: Permission denied
fm-inbox: cannot open a staging file in /var/folders/yk/hpylg6r976qgbphydj66gt9h0000gn/T/no-mistakes-evidence/01M0MMAKDXXGB9QRWAW19BR92K/e2e-failure-home/state/inbox; nothing was queued
exit=1
$ remaining inbox entries
$ wake file exists
no
Evidence: Original-defect reproduction and mutation proof

Source: Original-defect reproduction and mutation proof

Mutation proof on macOS (Darwin 24.6.0)

Fixture: current tests/fm-inbox.test.sh and tests/lib.sh, current
bin/fm-wake-lib.sh, and bin/fm-inbox.sh restored from base commit
1231b6ae7fd4c5ff7e94d2f5ca2159536c4c41cb.

$ FM_HOME=<isolated-home> <base fm-inbox.sh> note texte
base_exit=1
base_staging=1
sed: 1: "/var/folders/yk/hpylg6r ...": invalid command code f

The original command was already non-zero, but exposed only BSD sed jargon and
left one orphaned staging file with no published note.

$ /bin/bash <isolated-fixture>/tests/fm-inbox.test.sh
mutation_exit=1
not ok - note failed on a writable home
--- output ---
sed: 1: "/var/folders/yk/hpylg6r ...": invalid command code f

Result: reintroducing the original GNU-only `sed -i` implementation makes the
new behavioral test fail at the end-user note-capture path on BSD/macOS sed.

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 own fm-inbox: diagnostic, states ‘nothing was queued’, and removes the staging file.” The changed line still runs the second date unguarded. If date -u succeeds but date +%s fails, set -e exits after mktemp, 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 call die.
  • ⚠️ tests/fm-inbox.test.sh:41 - The regression test only catches the original sed -i mutation 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.sh
  • Manual macOS FM_HOME=&lt;isolated-home&gt; bin/fm-inbox.sh note texte, then list and inspection of the persisted note and notification
  • Manual denied-write check using a mode-0500 inbox, verifying exit 1, readable fm-inbox: diagnostic, no record/staging litter, and no notification
  • Mutation check using base-commit bin/fm-inbox.sh with current tests/fm-inbox.test.sh, verifying failure under BSD sed
  • Bounded bin/ portability inventory using rg and contextual inspection of retained stat sites
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (4): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile

@mremond
mremond force-pushed the fm/inbox-sed-macos branch from c18d5ef to 1eac645 Compare August 23, 2026 19:23
`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
mremond force-pushed the fm/inbox-sed-macos branch from 1eac645 to bb47ec0 Compare August 23, 2026 20:29
Comment thread bin/fm-inbox.sh Outdated
@mremond

mremond commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #2857, which landed the same fix: the note id is computed before the file is written and the sed -i second pass is removed entirely, so there is no GNU-only rewrite step left to fail on macOS.

Closing this in favour of that one. Thanks.

@mremond mremond closed this Aug 24, 2026
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