Skip to content

fix(fm-brief): remove heredoc-in-command-substitution that breaks bash 3.2 #989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sandersjdd3 wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −59
Open

fix(fm-brief): remove heredoc-in-command-substitution that breaks bash 3.2 #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 57 additions & 44 deletions bin/fm-brief.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,13 +217,15 @@ HERDR_SECTION=$(printf '%s\n' \
'Never bypass the helper, even for a read-only lifecycle probe or cleanup after failure.' \
'The captain fleet uses the running `default` session.')
else
HERDR_SECTION=$(cat <<'EOF'
# Herdr lifecycle declaration - NOT ENABLED
**HARD SAFETY GATE:** this scaffold cannot inspect the task text that replaces `{TASK}` later.
If the task will start, stop, delete, restart, profile, or otherwise drive Herdr lifecycle behavior, stop and regenerate the brief with `--herdr-lab` before dispatch.
Do not add Herdr lifecycle commands to this unguarded brief by hand.
EOF
)
# Built with printf rather than HERDR_SECTION=$(cat <<'EOF' ...): a heredoc body
# nested in $(...) trips the bash 3.2 apostrophe parser bug (issue #958), and a
# quoted delimiter does not save it. printf is not wrapped in $(...) at parse time.
# shellcheck disable=SC2016 # single quotes are deliberate: backtick-wrapped literals must reach the reading agent verbatim, not expand at scaffold time.
HERDR_SECTION=$(printf '%s\n' \
'# Herdr lifecycle declaration - NOT ENABLED' \
'**HARD SAFETY GATE:** this scaffold cannot inspect the task text that replaces `{TASK}` later.' \
'If the task will start, stop, delete, restart, profile, or otherwise drive Herdr lifecycle behavior, stop and regenerate the brief with `--herdr-lab` before dispatch.' \
'Do not add Herdr lifecycle commands to this unguarded brief by hand.')
fi

if [ "$KIND" = scout ]; then
Expand Down Expand Up @@ -285,51 +287,15 @@ case "$MODE" in
direct-PR)
SETUP2=""
RULE1='1. Never push to the default branch (push only your `fm/'"$ID"'` branch). Never merge a PR.'
DOD=$(cat <<EOF
# Definition of done
This project ships **direct-PR**: you raise the PR yourself, without the no-mistakes pipeline.
The task is complete only when committed on your branch.
When it is implemented and committed, push your branch and open a PR with \`gh-axi\`, then append \`done: PR {url}\` to the status file and stop.
Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome.
EOF
)
;;
local-only)
SETUP2=""
RULE1="1. Never push to any remote and never open a PR. Work only on your \`fm/$ID\` branch; firstmate handles the merge into local \`main\`."
DOD=$(cat <<EOF
# Definition of done
This project ships **local-only**: no remote, no PR, no pipeline.
The task is complete only when committed on your branch \`fm/$ID\`. Do NOT push, do NOT open a PR, do NOT merge.
Keep your branch a clean fast-forward onto the current default branch - if \`main\` has advanced, rebase onto it so the eventual merge stays a fast-forward.
When it is implemented and committed, append \`done: ready in branch fm/$ID\` to the status file and stop.
The configured merge authority approves the ready branch, then firstmate merges it into local \`main\` through the guarded fast-forward path.
EOF
)
;;
*) # no-mistakes (default)
SETUP2="
2. Run \`no-mistakes doctor\`; if it reports the repo is not initialized here, run \`no-mistakes init\`."
RULE1='1. Never push to the default branch. Never merge a PR.'
DOD=$(cat <<EOF
# Definition of done
The task is complete only when committed on your branch.
When you believe it is complete, append \`done: {summary}\` to the status file and stop.
Firstmate will then instruct you to run /no-mistakes to validate and ship a PR.

You drive no-mistakes by responding to its gates, not by implementing fixes.
Follow the guidance no-mistakes itself provides for the mechanics: it loads when you invoke /no-mistakes, and \`no-mistakes axi run --help\` plus the \`help\` lines in each \`axi\` response are authoritative and version-matched to the installed binary.
Do not hand-edit, commit, or fix findings yourself while a run is active - the pipeline applies every fix.

Two firstmate-specific rules layer on top of that guidance:
- ask-user findings are never yours to answer: escalate to firstmate (rule 6) and stop.
Firstmate applies the authority contract in its \`AGENTS.md\` and obtains any required captain decision.
When the decision comes back, feed it to the gate with \`no-mistakes axi respond\` and let the pipeline apply it - do not route the question to "the user" or implement the fix yourself.
- Avoid \`--yes\`: it would silently bypass firstmate's authority check and any required captain escalation.

After /no-mistakes reports CI green (the CI-ready return point - do not wait for it to keep monitoring in the background until merge), append \`done: PR {url} checks green\` and stop. You are finished.
EOF
)
;;
esac

Expand Down Expand Up @@ -382,6 +348,53 @@ For anything the codebase already shows, prefer a pointer to the authoritative f
If you touch a project \`AGENTS.md\` that lacks \`## Maintaining this file\`, add that short self-governance section from \`$FM_ROOT/bin/fm-ensure-agents-md.sh\` in the same pass.
Keep it proportionate: skip \`AGENTS.md\` edits for trivial tasks that produced no durable project knowledge.

$DOD
EOF

# The mode-specific definition of done is appended with a plain redirection heredoc
# rather than a VAR=$(cat <<EOF ...) command substitution: bash 3.2 (stock macOS
# /bin/bash) has a parser bug that misreads a lone apostrophe inside a heredoc body
# nested in $(...) as an unterminated single-quoted string and fails with
# "unexpected EOF while looking for matching `''" (issue #958). A plain heredoc
# redirection is not wrapped in $(...) and parses cleanly on 3.2.
case "$MODE" in
direct-PR)
cat >> "$BRIEF" <<EOF
# Definition of done
This project ships **direct-PR**: you raise the PR yourself, without the no-mistakes pipeline.
The task is complete only when committed on your branch.
When it is implemented and committed, push your branch and open a PR with \`gh-axi\`, then append \`done: PR {url}\` to the status file and stop.
Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome.
EOF
;;
local-only)
cat >> "$BRIEF" <<EOF
# Definition of done
This project ships **local-only**: no remote, no PR, no pipeline.
The task is complete only when committed on your branch \`fm/$ID\`. Do NOT push, do NOT open a PR, do NOT merge.
Keep your branch a clean fast-forward onto the current default branch - if \`main\` has advanced, rebase onto it so the eventual merge stays a fast-forward.
When it is implemented and committed, append \`done: ready in branch fm/$ID\` to the status file and stop.
The configured merge authority approves the ready branch, then firstmate merges it into local \`main\` through the guarded fast-forward path.
EOF
;;
*) # no-mistakes (default)
cat >> "$BRIEF" <<EOF
# Definition of done
The task is complete only when committed on your branch.
When you believe it is complete, append \`done: {summary}\` to the status file and stop.
Firstmate will then instruct you to run /no-mistakes to validate and ship a PR.

You drive no-mistakes by responding to its gates, not by implementing fixes.
Follow the guidance no-mistakes itself provides for the mechanics: it loads when you invoke /no-mistakes, and \`no-mistakes axi run --help\` plus the \`help\` lines in each \`axi\` response are authoritative and version-matched to the installed binary.
Do not hand-edit, commit, or fix findings yourself while a run is active - the pipeline applies every fix.

Two firstmate-specific rules layer on top of that guidance:
- ask-user findings are never yours to answer: escalate to firstmate (rule 6) and stop.
Firstmate applies the authority contract in its \`AGENTS.md\` and obtains any required captain decision.
When the decision comes back, feed it to the gate with \`no-mistakes axi respond\` and let the pipeline apply it - do not route the question to "the user" or implement the fix yourself.
- Avoid \`--yes\`: it would silently bypass firstmate's authority check and any required captain escalation.

After /no-mistakes reports CI green (the CI-ready return point - do not wait for it to keep monitoring in the background until merge), append \`done: PR {url} checks green\` and stop. You are finished.
EOF
;;
esac
echo "scaffolded: $BRIEF (ship, mode=$MODE; replace {TASK})"
60 changes: 45 additions & 15 deletions tests/fm-brief.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
#!/usr/bin/env bash
# Behavior tests for bin/fm-brief.sh.
#
# Regression coverage for the heredoc-in-command-substitution parse bug (issue
# #166): each ship-mode branch builds its Definition-of-done text with
# `VAR=$(cat <<EOF ... EOF)`. Bash's lexer tracks quote state through the
# heredoc body while it scans for the matching `)` of the command
# substitution, so a single unescaped apostrophe anywhere in that body breaks
# parsing of the *entire rest of the script* - `bash -n` fails, not just the
# generated brief. A plain `cat > file <<EOF ... EOF` (not wrapped in `$(...)`)
# is unaffected, so the secondmate charter block does not need this guard.
# Regression coverage for the heredoc-in-command-substitution parse bug (issues
# #166 and #958). A `VAR=$(cat <<EOF ... EOF)` builder wraps a heredoc body in a
# command substitution. Bash's lexer tracks quote state through that body while
# it scans for the matching `)`, so a single unescaped apostrophe anywhere in
# the body breaks parsing of the *entire rest of the script* - `bash -n` fails,
# not just the generated brief. Stock macOS bash 3.2 is stricter still: it trips
# on the apostrophe even under a quoted `<<'EOF'` delimiter, which bash 4+ parses
# cleanly - so a modern-bash `bash -n` alone (e.g. on CI) does not catch it.
# The durable fix removes the command-substitution-around-heredoc pattern from
# fm-brief.sh entirely: the Definition-of-done text is appended with a plain
# `cat >> file <<EOF` redirection (not wrapped in `$(...)`), and the unguarded
# Herdr section is built with `printf`. This suite guards that structurally
# (the pattern must not return) and by parsing under the oldest bash available.
set -u

# shellcheck source=tests/lib.sh
Expand All @@ -19,14 +24,38 @@ BRIEF_HOME="$TMP_ROOT/home"
mkdir -p "$BRIEF_HOME/data"

# The script itself must always parse. This is the direct regression test for
# issue #166: a stray apostrophe in any of the three DOD heredoc bodies
# (no-mistakes/direct-PR/local-only) breaks `bash -n` on the whole file.
# issues #166 and #958: a stray apostrophe in a DOD or Herdr heredoc body breaks
# `bash -n` on the whole file. We parse under every distinct bash we can find,
# because the #958 variant only surfaces on stock macOS bash 3.2 - a modern-bash
# `bash -n` (e.g. CI) passes the exact input that breaks a stock-macOS firstmate.
test_script_parses() {
local out rc
out=$(bash -n "$ROOT/bin/fm-brief.sh" 2>&1); rc=$?
expect_code 0 "$rc" "bash -n bin/fm-brief.sh must parse cleanly (got: $out)"
[ -z "$out" ] || fail "bash -n bin/fm-brief.sh emitted unexpected output: $out"
pass "fm-brief.sh: bash -n succeeds"
local bash_bin out rc seen=""
for bash_bin in bash /bin/bash /usr/bin/bash /opt/homebrew/bin/bash /usr/local/bin/bash; do
command -v "$bash_bin" >/dev/null 2>&1 || continue
# Resolve to a real path so we do not parse the same binary twice.
local resolved; resolved=$(command -v "$bash_bin")
case " $seen " in *" $resolved "*) continue ;; esac
seen="$seen $resolved"
out=$("$bash_bin" -n "$ROOT/bin/fm-brief.sh" 2>&1); rc=$?
expect_code 0 "$rc" "$bash_bin -n bin/fm-brief.sh must parse cleanly (got: $out)"
[ -z "$out" ] || fail "$bash_bin -n bin/fm-brief.sh emitted unexpected output: $out"
done
pass "fm-brief.sh: bash -n succeeds on every available bash (incl. stock 3.2)"
}

# Version-independent guard on the durable fix: the command-substitution-around-
# heredoc pattern that caused #958 must not return. `bash -n` under a modern bash
# would not catch its reintroduction, so pin it structurally instead. A heredoc
# fed to a plain `cat > "$FILE"` / `cat >> "$FILE"` redirection is fine; only a
# heredoc wrapped in `$(...)` (e.g. `VAR=$(cat <<EOF`) is the hazard.
test_no_heredoc_in_command_substitution() {
local hits
# Match code lines only; a comment line (optionally indented) that names the
# pattern to explain the fix is not a reintroduction.
hits=$(grep -nE '=\$\(cat[[:space:]]*<<' "$ROOT/bin/fm-brief.sh" | grep -vE '^[0-9]+:[[:space:]]*#' || true)
[ -z "$hits" ] || fail "fm-brief.sh reintroduced the \$(cat <<EOF ...) pattern that breaks bash 3.2 (#958):
$hits"
pass "fm-brief.sh: no heredoc wrapped in command substitution"
}

test_help_includes_entire_header() {
Expand Down Expand Up @@ -383,6 +412,7 @@ test_scout_and_secondmate_scaffold() {
}

test_script_parses
test_no_heredoc_in_command_substitution
test_help_includes_entire_header
test_ship_modes_generate_clean_briefs
test_faster_paths_use_configured_authority_without_stacked_review
Expand Down
Loading