fix(bin): parse fm-brief.sh and friends under stock macOS Bash 3.2 - #1196
Open
theaussie86 wants to merge 1 commit into
Open
fix(bin): parse fm-brief.sh and friends under stock macOS Bash 3.2#1196theaussie86 wants to merge 1 commit into
theaussie86 wants to merge 1 commit into
Conversation
theaussie86
force-pushed
the
fm/fm-brief-bash32-parse
branch
from
July 28, 2026 18:41
2a4fefe to
4af1da5
Compare
Owner
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for firstmate#1196 at |
Rebased onto current main and reconciled with two changes that landed in the meantime. - kunchenguid#1093 already restored the fm-brief.sh heredoc captures on main, so this branch keeps only the comment that records why those blocks must stay free of command substitution (issue kunchenguid#166). - kunchenguid#1282 replaced source assertions with behavioral coverage, so the source-shape assertions this branch carried are dropped. The parse sweep is proven by behavior instead: it rejects the exact 3.2-only construct, and it resolves a linted root however the path is spelled. - fm-lint.sh now owns the parse sweep, so the pre-push gate - not only the CI macOS lane - is the earliest boundary that catches a 3.2 parse defect. - The sweep covers every root in the owner inventory rather than the bin roots alone, because the CI macOS lane parses whatever --list-files reports. A tests/ root that only 3.2 rejects would otherwise pass the gate and fail that lane; a new test holds that parity. - fm-bootstrap.sh converts its remaining heredoc capture to the same read -r -d '' form, so prose there can contain apostrophes.
theaussie86
force-pushed
the
fm/fm-brief-bash32-parse
branch
from
July 31, 2026 06:30
4cc3777 to
11f813a
Compare
Owner
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for firstmate#1196 at |
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.
What Changed
$(cat <<'EOF' ...)heredoc captures inbin/fm-brief.shandbin/fm-bootstrap.shtoIFS= read -r -d '' VAR <<'EOF'plus a trailing-newline strip, so prose containing apostrophes no longer breaks the parse under stock macOS Bash 3.2 (bin/fm-brief.shpreviously failed with "unexpected EOF" and exited 2 without scaffolding a brief).bin/fm-lint.sh: when/bin/bashexists it runsbash -noverbin/*.shandbin/backends/*.sh, normalizing$ROOT/and./path prefixes first, so the pre-push lint gate owns the invariant instead of only the macOS CI lane.binscript, and documented the rule in.agents/skills/firstmate-coding-guidelines/SKILL.mdandCONTRIBUTING.md.tests/fm-lint.test.shcoverage for the sweep (guard presence, path-spelling normalization, and a 3.2-only defect probe gated on/bin/bashactually being Bash 3.x, with cleanup via the repo'sFM_TEST_CLEANUP_DIRStrap convention); updatedtests/fm-brief.test.shfor the converted blocks. The 3.2-only probe and the pinned-ShellCheck tests skip on machines without those toolchains, so their runtime behavior is proven by CI's macOS lane.Risk Assessment
✅ Low: The round-3 blocker is correctly resolved by a real version gate plus a bash-version-independent probe that preserves path-normalization coverage on Linux, and every production change on the branch remains behavior-preserving and parse-clean under stock Bash 3.2.57.
Testing
Ran the targeted suites (fm-brief, fm-bootstrap, fm-x-mode, fm-lint) and then reproduced issue #1179 end-to-end on the actual stock macOS /bin/bash 3.2.57: at the base commit fm-brief.sh fails to parse and a real scaffold call exits 2 producing no brief, while at the target commit it parses clean and generates correct briefs for all three delivery modes with intact Definition-of-done text and no leaked heredoc markers; a full 3.2 parse sweep of the 92 bin scripts goes from 1 failure to 0. Only gap: fm-lint's two new sweep-behavior tests self-skip locally because pinned ShellCheck 0.11.0 is not installed, so that sweep's runtime rejection path is proven by CI's macOS lane rather than locally.
Evidence: macOS Bash 3.2 before/after CLI transcript for fm-brief.sh
$ /bin/bash --version | head -1 GNU bash, version 3.2.57(1)-release (arm64-apple-darwin25) ### BEFORE - base commit fa0d85d on stock macOS /bin/bash 3.2 (issue #1179) $ /bin/bash -n bin/fm-brief.sh bin/fm-brief.sh: line 314: unexpected EOF while looking for matching)' bin/fm-brief.sh: line 388: syntax error: unexpected end of file exit: 2 $ /bin/bash bin/fm-brief.sh demo-task-1179 demo-project warn: no registry at $FM_HOME/data/projects.md; defaulting demo-project to no-mistakes off bin/fm-brief.sh: line 314: unexpected EOF while looking for matching)' exit: 2 $ ls $FM_HOME/data/demo-task-1179/ # no brief produced ### AFTER - target commit 2a4fefe, same shell $ /bin/bash -n bin/fm-brief.sh exit: 0 $ /bin/bash bin/fm-brief.sh demo-task-1179 demo-project scaffolded: $FM_HOME/data/demo-task-1179/brief.md (ship, mode=no-mistakes; replace {TASK}) exit: 0 # Definition of done The task is complete only when committed on your branch. When you believe it is complete, appenddone: {summary}to the status file and stop. Firstmate will then instruct you to run /no-mistakes to validate and ship a PR. $ grep -c EOF brief.md # no leaked heredoc marker 0 (direct-PR and local-only DOD branches also scaffold exit 0 with correct wording - see full file)Evidence: Full bin set parse sweep under Bash 3.2 (base vs target)
## base commit fa0d85d FAIL: bin/fm-brief.sh bin/fm-brief.sh: line 314: unexpected EOF while looking for matching `)' bin/fm-brief.sh: line 388: syntax error: unexpected end of file checked 92 files, 1 fail to parse ## target commit 2a4fefe checked 92 files, 0 fail to parseEvidence: Brief generated by fm-brief.sh under Bash 3.2 (target commit)
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (3) ✅
bin/fm-brief.sh:220- Same 3.2 parse trap remains reachable in this very file.HERDR_SECTION=$(cat <<'EOF' ... EOF\n)is prose captured through command substitution, and a quoted delimiter does not help - verified on GNU bash 3.2.57: a body containingit'sinsideX=$(cat <<'EOF' ...)fails with "unexpected EOF while looking for matching''" exactly like the unquoted form. Concrete path: an editor adds "the crew's lab" to the HERDR_SECTION prose,/bin/bash -n bin/fm-brief.shfails again, and fm-brief.sh is unusable on macOS - the identical user-facing failure the fix claims to close. The new CI loop catches it pre-merge, so this is a follow-up rather than a blocker, but the construct-level invariant the added comment asserts ("Keep these blocks free of command substitution so prose can contain apostrophes") holds only for the three DOD blocks. Convert HERDR_SECTION toIFS= read -r -d '' HERDR_SECTION <<'EOF' || trueplus the samebin/fm-bootstrap.sh:691-cadence_body=$(cat <<'EOF' ... )is the same latent construct: its body is comment prose ("Non-X instances have no such file"), so any future apostrophe there makes bin/fm-bootstrap.sh unparseable under stock macOS Bash 3.2. Currently clean, and the new CI parse loop would catch a regression before merge. Same mechanical conversion as fm-brief.sh applies. (The four$(cat <<'BASH'|<<'JQ')sites in bin/fm-fleet-snapshot.sh are code/filter bodies, lower risk of gaining prose apostrophes.).agents/skills/firstmate-coding-guidelines/SKILL.md:98- The new Bash 3.2 parse rule is enforced only in themacos-stock-bashCI job, while the adjacent guideline line names bin/fm-lint.sh as "the single owner of the lint definition ... that CI and the no-mistakes pre-push gate both invoke". bin/fm-lint.sh contains nobash -n/ BASH_VERSION check, so a macOS contributor - who has a real 3.2.57 at /bin/bash locally - gets a green pre-push gate and a red CI. The added SKILL.md text is honest about this ("which CI enforces per file"), so it is a deliberate-looking placement rather than a defect; moving a[ -x /bin/bash ] && /bin/bash -nsweep into fm-lint.sh would make the pre-push gate the earliest boundary that holds the invariant.bin/fm-brief.sh:281- Issue macOS bash 3.2: fm-brief.sh fails to parse at fa0d85d (unexpected EOF, heredoc-in-case); watcher cycles also exit 1 without reason since update #1179 reports two symptoms; this branch fixes only the first. The second ("watcher cycles also exit 1 without reason since update") is not addressed: bin/fm-watch.sh does not invoke bin/fm-brief.sh (its only callers are fm-test-run.sh, fm-subagent-pretool-check.sh, fm-home-seed.sh), so the parse fix cannot explain it, and all bin scripts now parse clean under 3.2.57 - meaning the watcher exit is a distinct cause. Scoping this branch to the parse fix is reasonable; just do not auto-close macOS bash 3.2: fm-brief.sh fails to parse at fa0d85d (unexpected EOF, heredoc-in-case); watcher cycles also exit 1 without reason since update #1179 on merge.🔧 Fix: convert remaining heredoc captures and own Bash 3.2 parse sweep in fm-lint
4 issues (1 warning, 3 infos) still open:
tests/fm-lint.test.sh:73- The probe test writes a deliberately non-parsing script into the tracked source tree at$ROOT/bin/fm-lint-parse-probe.shand relies solely on a happy-pathrm -fat line 82. There is no cleanup trap: the file-scopetrap 'exit 143' HUP INT TERMat line 392 is inside the fake-shellcheck heredoc, not this test, and the repo's own convention (FM_TEST_CLEANUP_DIRS+trap fm_test_cleanup EXIT, used at lines 456-459) is not used here. Concrete failure: Ctrl-C, a lane timeout, or a kill between the write and therm -fleavesbin/fm-lint-parse-probe.shin the working tree, after which everybin/fm-lint.shrun fails (both the new parse sweep and shellcheck), the CIfor script in bin/*.shloop fails, and the junk file is a candidate for accidental commit - recovery is manual. Fix:tmp_dir=$(mktemp -d "$ROOT/bin/.fm-lint-probe.XXXXXX")and put the probe inside it, then register it with the existing FM_TEST_CLEANUP_DIRS/EXIT-trap convention. That still exercises the sweep - thecasepatternbin/*.shmatches across/, sobin/.fm-lint-probe.XXXX/probe.shis swept - while the canonicalbin/*.shglob does not expand into a dot-prefixed subdirectory, so even a leaked probe cannot break lint or CI.bin/fm-lint.sh:165- The sweep filters roots by literal path spelling (case "$path" in bin/*.sh|bin/backends/*.sh)), so only relativebin/...arguments are parsed.bin/fm-lint.sh /repo/bin/fm-brief.shorbin/fm-lint.sh ./bin/fm-brief.sh- both natural under the documentedfm-lint.sh <path>...usage and both produced by shell tab-completion - silently skip the 3.2 check while the run still reports lint clean, so the owner states an invariant it did not test. Not reachable from the enforced paths (.no-mistakes.yamlline 37 and the CIrun: bin/fm-lint.shstep both pass no arguments, giving the relative canonical set), which keeps this informational. Normalize before the case: strip a leading$ROOT/and./from$path.bin/fm-lint.sh:160- The comment says the sweep is a "No-op where no real 3.2 exists" (and the header at line 19-21 says it "skips that sweep silently where /bin/bash is absent"), but the guard is[ -x /bin/bash ], which on Linux is Bash 5. There the loop runs to completion and passes without testing the 3.2 invariant at all - it is a tautology, not a no-op. The behaviour is exactly what was asked for; only the first comment overstates what the guard proves, which matters because the whole point of the change is that a green local gate should not imply a property it did not check. Reword to: no-op where /bin/bash is absent, and a real proof only where /bin/bash is the stock 3.2 shell - the macOS CI job remains the sole real 3.2 enforcement for Linux contributors.CONTRIBUTING.md:48- Doc drift inside the changed area: this line still scopes the owner's definition to "(the shellcheck file set, config, and pinned shellcheck version)", while .agents/skills/firstmate-coding-guidelines/SKILL.md:96 was updated in this commit to "(file set, config, pinned shellcheck version, and the stock Bash 3.2 parse sweep)". The new CONTRIBUTING.md line 97 states the sweep separately, so the two files no longer make the same statement about what fm-lint.sh owns. Add the parse sweep to the line 48 parenthetical.🔧 Fix: harden lint parse sweep probe cleanup and path normalization
1 error still open:
tests/fm-lint.test.sh:78- test_parse_sweep_rejects_a_bash32_only_defect skips only when /bin/bash is missing, but its probe is a Bash-3.2-ONLY parse defect, so the test fails on every Linux lane. Chain: (1) the probe at lines 87-92 isBODY=$(cat <<EOF / the crew's lab / EOF ), which this branch's own tests/fm-brief.test.sh header documents as "Bash 5 later stopped mis-scanning it, while stock macOS Bash 3.2 still does" - that asymmetry is precisely why the macOS CI lane exists; (2) on ubuntu-latest /bin/bash is Bash 5, so[ -x /bin/bash ]at line 78 passes and does not skip; (3) the portable parallel shards install pinned ShellCheck 0.11.0 (.github/workflows/ci.yml lines 49-53 and 81-85), sopinned_readyat line 79 does not skip either; (4) the sweep therefore runs Bash 5, the probe parses fine, PARSE_RC stays 0, fm-lint.sh exits 0, and line 97[ "$rc" -ne 0 ] || failfires. Result: tests/fm-lint.test.sh goes red on the Linux portable test shards and on any Linux contributor's local no-mistakes Test gate, while passing on macOS - reproducing the exact green-on-my-machine / red-on-CI asymmetry this branch set out to remove, just with the platforms swapped. Fix: gate this test on /bin/bash actually being 3.x (e.g.case $(/bin/bash -c 'printf %s "${BASH_VERSION%%.*}"') in 3) ;; *) pass "SKIP (/bin/bash is not stock Bash 3.2)"; return 0 ;; esac), and add a second bash-version-independent probe (an unterminatedif true; thenfails to parse under every bash) carrying the three path-spelling assertions at lines 96-107. Without that second probe the review-2 path-normalization work in bin/fm-lint.sh gets zero coverage anywhere except macOS. Note: theassert_grep 'if [ -x /bin/bash ]; then'at line 59 pins fm-lint.sh's own guard and is correct as-is - only this test's line-78 skip guard is wrong.🔧 Fix: gate 3.2-only lint probe test on real Bash 3.2
✅ Re-checked - no issues remain.
tests/fm-lint.test.sh- tests/fm-lint.test.sh's two new Bash 3.2 parse-sweep behavior tests (parse sweep path-spelling check,Bash 3.2 parse sweep rejection) self-skip locally because pinned ShellCheck 0.11.0 is not resolvable on this machine; installing it would modify system state outside the worktree. The sweep's runtime behavior is therefore only proven by CI's macOS lane. The equivalent invariant was verified manually with/bin/bash -nover the full bin set../tests/fm-brief.test.sh(15 ok, 0 fail)./tests/fm-bootstrap.test.sh(21 ok, 0 fail)./tests/fm-x-mode.test.sh(all ok; covers the fm-bootstrap x-mode cadence_body conversion)./tests/fm-lint.test.sh(all ok; 6 tests SKIP - ShellCheck 0.11.0 not resolvable locally)Manual:/bin/bash -n bin/fm-brief.shat base commit fa0d85d vs target 2a4fefe under real Bash 3.2.57Manual:FM_HOME=<tmp> /bin/bash bin/fm-brief.sh demo-task-1179 demo-projectat base (exit 2, no brief) vs target (exit 0, brief scaffolded)Manual:/bin/bash bin/fm-brief.shfor direct-PR and local-only registry projects, inspecting rendered Definition-of-done sectionsManual:/bin/bash -noverbin/*.sh bin/backends/*.shat both commits (92 files each).github/workflows/ci.yml:308- CI job name at .github/workflows/ci.yml:308 still reads "Stock macOS Bash snapshot compatibility", but the step now parse-checks every bin/.sh and bin/backends/.sh, not just the snapshot consumers. The step's inline comment and CONTRIBUTING.md:96 both describe the widened scope correctly, so no prose is stale; only the job label understates it. Left unchanged because the job name is workflow config, not documentation, and this phase may not alter executable config. Suggested follow-up: rename to "Stock macOS Bash 3.2 parse + snapshot compatibility".✅ **Push** - passed
✅ No issues found.