fix(bin): repair fm-brief.sh parse error and harden set -u array expansion#205
Open
Ballestar wants to merge 5 commits into
Open
fix(bin): repair fm-brief.sh parse error and harden set -u array expansion#205Ballestar wants to merge 5 commits into
Ballestar wants to merge 5 commits into
Conversation
…D heredoc A lone single quote inside the no-mistakes Definition-of-done block, built with DOD=$(cat <<EOF ... EOF), broke bash's $() scanner: it hunted past the closing EOF) for a matching quote, so `bash -n bin/fm-brief.sh` failed with "unexpected EOF while looking for matching '" and no brief could be scaffolded at all. Quoting the heredoc delimiter does not help because the $() scanner still counts the quote. Reword the possessive "no-mistakes' own guidance" to "the guidance from no-mistakes itself" so the block carries no lone apostrophe. The change is minimal and leaves the brief's wording, escaped backticks, and the scout and secondmate code paths untouched. Add tests/fm-brief.test.sh as a regression guard: a bash -n parse check, a check that the no-mistakes DOD renders the guidance line with literal backticks and no leftover apostrophe phrasing, and a check that the scout and secondmate paths still scaffold well-formed briefs. Upstream kunchenguid#166.
Expanding "${arr[@]}" on an empty array under `set -u` throws "unbound
variable" on bash < 4.4 (notably macOS system bash 3.2), while it is a
no-op on bash 4.4+. Two sites expanded arrays that can legitimately be
empty:
- bin/fm-pr-merge.sh: merge_args is empty when the caller passes an
explicit merge method, so the extra-args path failed on bash 3.2.
- bin/fm-spawn.sh: shared_args is empty when no --harness/--model/
--effort/--backend flags are passed, so batch dispatch failed on
bash 3.2.
Both block the no-mistakes test step (which runs `bash tests/*.test.sh`
locally) on macOS, and are pre-existing failures on main. Use the portable
"${arr[@]+"${arr[@]}"}" idiom, which expands to nothing when the array is
empty or unset and to the elements otherwise, under set -u on all bash
versions. Behavior on bash 4.4+ is unchanged.
Found while running the suite for the fm-brief parse fix; the two affected
tests (fm-pr-merge extra-args, fm-spawn-batch dispatch) now pass on bash
3.2 as well as on CI's bash 5.x.
4 tasks
Owner
|
Thanks for the PR! It looks like this branch has a merge conflict with the base branch right now. When you get a chance, could you rebase onto (or merge in) the latest base branch, resolve the conflict, and push? Once GitHub shows the PR as mergeable again, it'll be picked back up for review. Noted for firstmate#205 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
2 participants
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
bin/fm-brief.shcaused by a lone apostrophe inside the definition-of-done heredoc, so the brief scaffold script runs again.bin/fm-pr-merge.shandbin/fm-spawn.shset -u-safe so they no longer fail under nounset.tests/fm-brief.test.shcovering the ship-brief DOD output (with its assertion aligned to the brief's actual "Follow the no-mistakes guidance for the mechanics" wording) and registered it in theCONTRIBUTING.mdtest registry.Risk Assessment
✅ Low: Two narrowly-scoped, correct set -u-safe array-expansion fixes plus a new regression test whose assertions all match the actual script output and available test helpers; the prior wording-mismatch finding is resolved.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
🔧 **Rebase** - 1 issue found → auto-fixed ✅
bin/fm-brief.sh- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Review** - 1 issue found → auto-fixed ✅
tests/fm-brief.test.sh:38- The new regression test asserts the fixed string "Follow the guidance from no-mistakes itself for the mechanics", but bin/fm-brief.sh:211 actually renders "Follow the no-mistakes guidance for the mechanics". assert_grep uses grep -F (literal), so test_ship_brief_no_mistakes_dod will fail on this line, breaking the very code path it is meant to protect. Update the assertion string to match the brief's actual wording ("Follow the no-mistakes guidance for the mechanics").🔧 Fix: align fm-brief test assertion with main DOD wording
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.