fix: a test-harness crash reads as a pass in three release/heredoc suites (#476, #477, #478)

changelog.d/476.fixed.md

@@ -0,0 +1 @@
+- **A `check_release_tree.py` crash no longer reads as a pass** (#476). `tests/test-release-branch-437.sh` and `tests/test-release-branch-461.sh` decided pass/fail by checking whether the output had any `^FAIL ` line, so a non-zero exit with no FAIL line at all -- a traceback, not a reported offender -- took the same branch as a genuinely clean run. The two outcomes are now told apart.

changelog.d/477.fixed.md

@@ -0,0 +1 @@
+- **The release-branch equivalence test now builds the ref the workflow actually publishes** (#477). `tests/test-release-branch-461.sh` hard-coded `--ref HEAD`, while `.github/workflows/release-branch.yml` builds and publishes `$SOURCE_REF`. On a `workflow_dispatch` run with an older `ref=` input the two diverge, and the equivalence step was silently checking the wrong tree. It now reads `$SOURCE_REF` from the environment, falling back to `HEAD` for a local/manual run.

changelog.d/478.fixed.md

@@ -0,0 +1 @@
+- **`detect_heredoc_lines()` in `tests/test-no-heredocs-459.sh` no longer confuses a read failure with a clean file** (#478). An `awk` failure (a vanished file, a permissions problem) produced the same empty string as a file that was genuinely scanned and found clean. `awk`'s own exit status is now captured separately from `grep`'s, so an unreadable file is reported distinctly instead of silently passing.

tests/test-no-heredocs-459.sh

@@ -61,9 +61,41 @@ HEREDOC_PATTERN="(^|[^<])<<-?[[:space:]]*[${Q1}${Q2}]?[A-Za-z0-9_][A-Za-z0-9_]*"
 # heredoc, comment lines excluded. Blanks comment lines rather than deleting them
 # (awk) before the grep -nE pass, so the line numbers grep reports stay the file's
 # own.
+#
+# #478: awk's own exit status is captured BEFORE the grep -nE pass, not folded
+# into the pipeline's "|| true". A file that vanishes, loses read permission,
+# or otherwise makes awk fail produces the exact same empty string as a file
+# that was genuinely read and found clean -- the caller could not tell "no
+# heredoc here" from "could not read this file at all". Return 2 on an awk
+# failure so a caller can tell the two apart instead of treating both as the
+# same empty, clean result.
+#
+# Self-review caught a second-order version of the same bug this fix exists
+# to close: an earlier draft merged awk's stderr into $filtered via "2>&1" on
+# EVERY run, including a successful one. A non-fatal stderr warning from a
+# different awk implementation (gawk on a Linux CI runner vs. the BSD/mawk
+# this was likely tested against) would then have spliced extra text into
+# the very data grep -nE scans afterward, shifting the line numbers it
+# reports for a real hit -- a corruption #478's own pre-fix code never had,
+# because stderr used to pass straight through untouched. awk's stderr is
+# now discarded on success and never enters the grep pipe at all; only the
+# exit status crosses the function boundary.
 detect_heredoc_lines() {
-  awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" \
-    | grep -nE -- "$HEREDOC_PATTERN" || true
+  local filtered awk_err
+  # stdout and stderr captured separately: $filtered only ever holds awk's
+  # stdout (what the grep pass below scans), while $awk_err is read only on
+  # the failure path -- the second self-review round caught that discarding
+  # stderr outright on failure too lost the diagnostic text (what awk
+  # actually said), not just the risk the first round fixed (stderr noise
+  # splicing into a SUCCESSFUL run's scanned data).
+  if ! filtered=$(awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" 2> "$TMPD/awk-err.$$"); then
+    awk_err=$(cat "$TMPD/awk-err.$$" 2> /dev/null)
+    rm -f "$TMPD/awk-err.$$"
+    echo "AWK_READ_FAILED($1): $awk_err" >&2
+    return 2
+  fi
+  rm -f "$TMPD/awk-err.$$"
+  printf '%s\n' "$filtered" | grep -nE -- "$HEREDOC_PATTERN" || true
 }
 
 echo "=== DETECTOR SELF-TEST: a planted fixture, both directions ==="
@@ -120,6 +152,25 @@ for ln in $FALSE_POSITIVE_LINES; do
 done
 [ "$ANY_FALSE_POSITIVE" -eq 0 ] && ok "NEGATIVE: comments, quoted \"<< \" tokens, and here-strings (<<<) are not flagged"
 
+# #478: an unreadable/vanished file must render distinguishably from a clean
+# one -- both used to produce the same empty string. Positive control (a
+# genuine read failure IS reported) paired with the clean-file negative
+# control already proven above, so a harness that reports nothing for
+# everything cannot pass this pair.
+detect_heredoc_lines "$TMPD/does-not-exist-478.sh" > /dev/null 2> /dev/null
+UNREADABLE_RC=$?
+if [ "$UNREADABLE_RC" -eq 2 ]; then
+  ok "POSITIVE: a vanished/unreadable file is reported as a read failure (rc=2), not silently 'clean'"
+else
+  bad "POSITIVE: a vanished/unreadable file should return rc=2, got rc=$UNREADABLE_RC -- indistinguishable from a clean file"
+fi
+
+detect_heredoc_lines "$FIXTURE" > /dev/null 2> /dev/null
+CLEAN_RC=$?
+[ "$CLEAN_RC" -eq 0 ] \
+  && ok "NEGATIVE: a real, readable file still returns rc=0 (the read-failure path does not fire on a clean read)" \
+  || bad "NEGATIVE: a readable file should return rc=0, got rc=$CLEAN_RC"
+
 echo ""
 echo "=== LIVE SWEEP: every tracked file under scripts/ ==="
 
@@ -129,16 +180,26 @@ if [ -z "$SCRIPTS_FILES" ]; then
   bad "git ls-files scripts returned nothing -- the sweep below would pass for the wrong reason"
 else
   VIOLATIONS=""
+  UNREADABLE=""
   while IFS= read -r f; do
     [ -n "$f" ] || continue
-    HITS="$(detect_heredoc_lines "$REPO/$f")"
+    HITS="$(detect_heredoc_lines "$REPO/$f" 2> /dev/null)"
+    DETECT_RC=$?
+    if [ "$DETECT_RC" -eq 2 ]; then
+      UNREADABLE="$UNREADABLE
+$f"
+      continue
+    fi
     [ -n "$HITS" ] || continue
     VIOLATIONS="$VIOLATIONS
 $f:
 $HITS"
   done <<< "$SCRIPTS_FILES"
 
-  if [ -z "$VIOLATIONS" ]; then
+  if [ -n "$UNREADABLE" ]; then
+    bad "could not scan every tracked file under scripts/ -- the sweep below is incomplete, not clean" \
+      "$UNREADABLE"
+  elif [ -z "$VIOLATIONS" ]; then
     ok "no tracked file under scripts/ opens a real here-document"
   else
     bad "a real here-document was found under scripts/ -- the directory validator holds on this" \

tests/test-release-branch-437.sh

@@ -33,6 +33,48 @@ bad() {
   [ $# -eq 0 ] || echo "    $*"
 }
 
+# classify_check_result RC OUTPUT -- "clean", "crash" or "offenders". A non-zero
+# RC with no "^FAIL " line anywhere in OUTPUT is check_release_tree.py crashing
+# (a traceback, not a reported offender) -- #476. Used by this file's real
+# check below and by its own self-test, so the self-test exercises the exact
+# function the real flow uses rather than a restatement of it. DUPLICATED
+# (not sourced from a common file) in test-release-branch-461.sh -- these two
+# are standalone suites with no shared library, so keep both copies in sync
+# by hand if this function's logic ever changes.
+classify_check_result() {
+  local rc="$1" out="$2" fails
+  if [ "$rc" -eq 0 ]; then
+    echo "clean"
+    return
+  fi
+  fails=$(printf '%s\n' "$out" | grep '^FAIL ' || true)
+  if [ -z "$fails" ]; then
+    echo "crash"
+  else
+    echo "offenders"
+  fi
+}
+
+echo "=== DETECTOR SELF-TEST: classify_check_result (#476) ==="
+RESULT=$(classify_check_result 0 "")
+[ "$RESULT" = "clean" ] \
+  && ok "classify_check_result: rc=0 -> clean" \
+  || bad "classify_check_result: rc=0 should be clean, got '$RESULT'"
+
+CRASH_OUT='Traceback (most recent call last):
+  File "check_release_tree.py", line 42, in <module>
+    raise ZeroDivisionError
+ZeroDivisionError'
+RESULT=$(classify_check_result 1 "$CRASH_OUT")
+[ "$RESULT" = "crash" ] \
+  && ok "classify_check_result: rc!=0, no FAIL lines -> crash (#476 -- this used to read as clean)" \
+  || bad "classify_check_result: rc!=0 with no FAIL lines should be crash, got '$RESULT'"
+
+RESULT=$(classify_check_result 1 "FAIL something.md: offender")
+[ "$RESULT" = "offenders" ] \
+  && ok "classify_check_result: rc!=0, FAIL line(s) present -> offenders" \
+  || bad "classify_check_result: rc!=0 with a FAIL line should be offenders, got '$RESULT'"
+
 echo "=== harness guard: the three scripts and the config exist here ==="
 for f in "$BUILD" "$CHECK" "$SMOKE" "$CONFIG"; do
   [ -f "$f" ] || {
@@ -187,17 +229,36 @@ _KNOWN_ALLOWED_TOOLS_HOLDS=""
 if [ "$BUILD_RC" -eq 0 ]; then
   CHECK_OUT=$(python3 "$CHECK" "$TREE" --config "$CONFIG" 2>&1)
   CHECK_RC=$?
-  if [ "$CHECK_RC" -eq 0 ]; then
-    ok "check_release_tree.py exits 0"
-  else
-    UNEXPECTED=$(printf '%s\n' "$CHECK_OUT" | grep '^FAIL ' | sed 's/^FAIL //' \
-      | grep -vFxf <(printf '%s\n' "$_KNOWN_ALLOWED_TOOLS_HOLDS") || true)
-    if [ -z "$UNEXPECTED" ]; then
-      ok "check_release_tree.py's only offender(s) are the known, already-filed ALLOWED_TOOLS_BROAD hold on commands/doctor.md, commands/init.md and commands/stats.md"
-    else
-      bad "check_release_tree.py reported (an) UNEXPECTED offender(s)" "$UNEXPECTED"
-    fi
-  fi
+  case "$(classify_check_result "$CHECK_RC" "$CHECK_OUT")" in
+    clean)
+      ok "check_release_tree.py exits 0"
+      ;;
+    crash)
+      # #476: rc != 0 with no "^FAIL " line at all is check_release_tree.py
+      # crashing (a traceback), not a reported offender -- the old
+      # UNEXPECTED-emptiness-only check took the "known holds" ok branch here
+      # too, so a crash read as a clean pass.
+      bad "check_release_tree.py exited $CHECK_RC with no FAIL lines -- it crashed rather than finishing the check" "$CHECK_OUT"
+      ;;
+    offenders)
+      UNEXPECTED=$(printf '%s\n' "$CHECK_OUT" | grep '^FAIL ' | sed 's/^FAIL //' \
+        | grep -vFxf <(printf '%s\n' "$_KNOWN_ALLOWED_TOOLS_HOLDS") || true)
+      # Self-review on #476/#477/#478: _KNOWN_ALLOWED_TOOLS_HOLDS is currently
+      # empty (the comment above explains why -- #439 resolved the one hold it
+      # used to pin), so this "ok" branch cannot actually be reached by
+      # anything today; filtering a non-empty $UNEXPECTED against an empty
+      # exclusion list changes nothing. The old wording named the #439 hold by
+      # file, which read as describing a live exception that no longer
+      # exists. Kept generic (naming the variable rather than a specific,
+      # resolved hold) so it stays accurate if this set is ever populated
+      # again for a future pinned, already-filed exception.
+      if [ -z "$UNEXPECTED" ]; then
+        ok "check_release_tree.py's only offender(s) are already covered by the pinned, already-filed exception(s) in \$_KNOWN_ALLOWED_TOOLS_HOLDS"
+      else
+        bad "check_release_tree.py reported (an) UNEXPECTED offender(s)" "$UNEXPECTED"
+      fi
+      ;;
+  esac
 else
   bad "check skipped -- the build above did not produce a tree"
 fi

tests/test-release-branch-461.sh

@@ -30,6 +30,75 @@ bad() {
   [ $# -eq 0 ] || echo "    $*"
 }
 
+# classify_check_result RC OUTPUT -- "clean", "crash" or "offenders". A non-zero
+# RC with no "^FAIL " line anywhere in OUTPUT is check_release_tree.py crashing
+# (a traceback, not a reported offender) -- #476. Used by this file's real
+# check below and by its own self-test, so the self-test exercises the exact
+# function the real flow uses rather than a restatement of it. DUPLICATED
+# (not sourced from a common file) in test-release-branch-437.sh -- these two
+# are standalone suites with no shared library, so keep both copies in sync
+# by hand if this function's logic ever changes.
+classify_check_result() {
+  local rc="$1" out="$2" fails
+  if [ "$rc" -eq 0 ]; then
+    echo "clean"
+    return
+  fi
+  fails=$(printf '%s\n' "$out" | grep '^FAIL ' || true)
+  if [ -z "$fails" ]; then
+    echo "crash"
+  else
+    echo "offenders"
+  fi
+}
+
+echo "=== DETECTOR SELF-TEST: classify_check_result and REF selection (#476, #477) ==="
+
+RESULT=$(classify_check_result 0 "")
+[ "$RESULT" = "clean" ] \
+  && ok "classify_check_result: rc=0 -> clean" \
+  || bad "classify_check_result: rc=0 should be clean, got '$RESULT'"
+
+CRASH_OUT='Traceback (most recent call last):
+  File "check_release_tree.py", line 42, in <module>
+    raise ZeroDivisionError
+ZeroDivisionError'
+RESULT=$(classify_check_result 1 "$CRASH_OUT")
+[ "$RESULT" = "crash" ] \
+  && ok "classify_check_result: rc!=0, no FAIL lines -> crash (#476 -- this used to read as clean)" \
+  || bad "classify_check_result: rc!=0 with no FAIL lines should be crash, got '$RESULT'"
+
+RESULT=$(classify_check_result 1 "FAIL something.md: offender")
+[ "$RESULT" = "offenders" ] \
+  && ok "classify_check_result: rc!=0, FAIL line(s) present -> offenders" \
+  || bad "classify_check_result: rc!=0 with a FAIL line should be offenders, got '$RESULT'"
+
+(
+  unset SOURCE_REF
+  REF="${SOURCE_REF:-HEAD}"
+  [ "$REF" = "HEAD" ]
+) && ok "REF selection: no \$SOURCE_REF set -> HEAD (local/manual run)" \
+  || bad "REF selection: with no \$SOURCE_REF, REF should fall back to HEAD"
+
+(
+  SOURCE_REF="v0.1.0"
+  REF="${SOURCE_REF:-HEAD}"
+  [ "$REF" = "v0.1.0" ]
+) && ok "REF selection: \$SOURCE_REF=v0.1.0 -> builds that ref, not HEAD (#477 -- workflow_dispatch with an older ref)" \
+  || bad "REF selection: with \$SOURCE_REF set, REF should be that ref, not HEAD"
+
+# The two checks above only prove bash's own "${VAR:-default}" semantics, not
+# that THIS SCRIPT still wires $REF into the real build call below -- they
+# would keep passing even if the build line were reverted to a hardcoded
+# "--ref HEAD" (exactly the #477 regression). Grep this script's own source
+# for the build invocation actually passing "$REF" (sourced from
+# $SOURCE_REF), so a revert to a literal HEAD fails this self-test.
+if grep -qE -- '--ref[[:space:]]+"\$REF"' "$0"; then
+  ok "REF selection: the build invocation passes \$REF, not a hardcoded --ref HEAD (#477 regression guard)"
+else
+  bad "REF selection: the build invocation no longer passes \$REF -- #477 may have regressed back to a hardcoded --ref HEAD"
+fi
+
 for f in "$BUILD" "$CHECK" "$CONFIG"; do
   [ -f "$f" ] || {
     echo "FAIL: harness guard -- $f does not exist, every assertion below is vacuous"
@@ -45,8 +114,19 @@ TMPD=$(mktemp -d 2> /dev/null || mktemp -d -t jiteq)
 trap 'rm -rf "$TMPD"' EXIT
 TREE="$TMPD/release-tree"
 
-echo "=== build: HEAD builds a release tree, and check_release_tree.py is clean ==="
-BUILD_OUT=$(python3 "$BUILD" --repo "$REPO" --ref HEAD --out "$TREE" --config "$CONFIG" 2>&1)
+# #477: the release-branch workflow builds and publishes $SOURCE_REF (the
+# workflow_dispatch input, or the pushed tag on a tag push) -- not necessarily
+# HEAD. On a tag push the checkout's HEAD IS that tag, so the two happen to
+# agree; on workflow_dispatch with an older ref= input, HEAD is the dispatching
+# branch (main) and this step used to build and check THAT tree while the
+# workflow published a different one built from $SOURCE_REF, leaving the
+# equivalence check silently pointed at the wrong commit. $SOURCE_REF is a
+# job-level env var in the workflow, so it is already in this step's
+# environment when CI runs this script; a local/manual run has no such var and
+# keeps building HEAD, as before.
+REF="${SOURCE_REF:-HEAD}"
+echo "=== build: $REF builds a release tree, and check_release_tree.py is clean ==="
+BUILD_OUT=$(python3 "$BUILD" --repo "$REPO" --ref "$REF" --out "$TREE" --config "$CONFIG" 2>&1)
 BUILD_RC=$?
 if [ "$BUILD_RC" -eq 0 ]; then
   ok "build_release_tree.py exits 0"
@@ -58,15 +138,18 @@ else
 fi
 CHECK_OUT=$(python3 "$CHECK" "$TREE" --config "$CONFIG" 2>&1)
 CHECK_RC=$?
-# Same pinned, already-filed exception test-release-branch-437.sh carries (#439,
-# narrowed allowed-tools grants on the three command scripts) -- not this issue's
-# subject, so it is excluded from "unexpected" the same way there.
-UNEXPECTED=$(printf '%s\n' "$CHECK_OUT" | grep '^FAIL ' || true)
-if [ "$CHECK_RC" -eq 0 ] || [ -z "$UNEXPECTED" ]; then
-  ok "check_release_tree.py reports no offender in the built tree"
-else
-  bad "check_release_tree.py reported offender(s)" "$UNEXPECTED"
-fi
+case "$(classify_check_result "$CHECK_RC" "$CHECK_OUT")" in
+  clean)
+    ok "check_release_tree.py reports no offender in the built tree"
+    ;;
+  crash)
+    bad "check_release_tree.py exited $CHECK_RC with no FAIL lines -- it crashed rather than finishing the check" "$CHECK_OUT"
+    ;;
+  offenders)
+    UNEXPECTED=$(printf '%s\n' "$CHECK_OUT" | grep '^FAIL ' || true)
+    bad "check_release_tree.py reported offender(s)" "$UNEXPECTED"
+    ;;
+esac
 
 echo ""
 echo "=== the compiler's own two guarantees, on planted input (#461 release audit) ==="

trap.d/477.sha-not-threaded-across-jobsteps.md

@@ -0,0 +1,30 @@
+Observed during self-review of the #477 fix (second Explore spawn, strict
+finding-format re-ask). The fix (`REF="${SOURCE_REF:-HEAD}"` in
+tests/test-release-branch-461.sh, reading the job-level $SOURCE_REF env var)
+closes the exact gap #477 named -- a hard-coded `--ref HEAD` diverging from
+what the workflow actually publishes on a workflow_dispatch run with an
+older `ref=` input -- but it narrows rather than fully closes the class.
+
+In `.github/workflows/release-branch.yml`, the "Build the release tree" step
+resolves `$SOURCE_REF` to a frozen `sha=$(git rev-parse --verify
+"${SOURCE_REF}^{commit}")` and builds from that sha. The later "Equivalence
+test the compiled scripts (#461)" step (test-release-branch-461.sh) is never
+handed that resolved sha -- it independently re-reads `$SOURCE_REF` and
+re-resolves it itself inside build_release_tree.py. If a dispatch's `ref=`
+input names a moving ref (a branch, not a tag or sha) and that ref moves
+between the two steps, the equivalence test can still build and check a
+different commit than the one the first step already built and `publish`
+will later push -- the same class of silent-wrong-tree bug #477 described,
+with a narrower window (between two steps in one job run) instead of none.
+
+Not acted on in the #476/#477/#478 lane: closing this fully means threading
+the already-resolved `sha` from the "Build the release tree" step's own
+`steps.build.outputs.sha` into the equivalence-test step (a job `outputs:`
+wire-up plus a CLI flag or env var on test-release-branch-461.sh), which is
+a workflow-level change beyond what #477 asked for and risks widening this
+lane's diff into territory #477's own issue text did not scope.
+
+Worth curation's attention: whether this is worth a follow-up issue, and
+whether `workflow_dispatch`'s `ref=` input is realistically ever a moving
+branch name in practice for this repo (the input's own description says
+"Tag (or sha)", which would make this window theoretical rather than live).