From 0997039965ec67379a1ae450160775ba3f9b00d6 Mon Sep 17 00:00:00 2001
From: JD Sanders <jd.sanders@hudl.com>
Date: Fri, 24 Jul 2026 11:26:21 -0500
Subject: [PATCH] fix(fm-brief): remove heredoc-in-command-substitution that
 breaks bash 3.2 (#958)

fm-brief.sh failed to parse under stock macOS bash 3.2.57, so no brief could
be scaffolded on a default macOS firstmate. Bash 3.2 misreads a lone apostrophe
inside a heredoc body nested in $(...) as an unterminated single-quoted string
and aborts with "unexpected EOF while looking for matching `)'". #945 added a
line containing "firstmate's" to a DOD=$(cat <<EOF ...) block, triggering it.
Bash 4+ parses the body literally, so CI and modern-bash dev machines passed.

Durable fix, not just a reword: eliminate the VAR=$(cat <<EOF ... EOF) pattern.
The mode-specific Definition of done is appended with a plain cat >> redirection
heredoc (not wrapped in $(...)), and the unguarded Herdr section is built with
printf like its --herdr-lab counterpart. Both quoted and unquoted heredoc
delimiters trip the 3.2 bug, so the quoted Herdr <<'EOF' was a latent trap too.
Brief content is byte-for-byte unchanged across all modes; only the shell
construction changed.

Regression test parses fm-brief.sh under every available bash (the 3.2 variant
is invisible to a modern-bash bash -n) and adds a version-independent structural
guard that the command-substitution-around-heredoc pattern does not return.
---
 bin/fm-brief.sh        | 101 +++++++++++++++++++++++------------------
 tests/fm-brief.test.sh |  60 ++++++++++++++++++------
 2 files changed, 102 insertions(+), 59 deletions(-)

diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh
index 00ea34ddab..54fd93998a 100755
--- a/bin/fm-brief.sh
+++ b/bin/fm-brief.sh
@@ -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
@@ -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
 
@@ -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})"
diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh
index 74690eab42..7870ed0362 100755
--- a/tests/fm-brief.test.sh
+++ b/tests/fm-brief.test.sh
@@ -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
@@ -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() {
@@ -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