From 77d57593123dce9e639fff14df567cad513fe9a3 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 15:08:21 +0200 Subject: [PATCH 1/4] fix: a test-harness crash reads as a pass in three release/heredoc suites (#476, #477, #478) test-release-branch-437.sh and -461.sh decided pass/fail from whether the output had a "^FAIL " line, so a non-zero exit with no FAIL line at all -- check_release_tree.py crashing, not reporting an offender -- took the same branch as a clean run (#476). Both now classify "clean" / "crash" / "offenders" as three distinct outcomes via a shared classify_check_result(). test-release-branch-461.sh also hard-coded --ref HEAD while the workflow builds and publishes $SOURCE_REF; on workflow_dispatch with an older ref= input the two diverge and the equivalence step silently checked the wrong tree (#477). It now reads $SOURCE_REF from the environment, falling back to HEAD for a local/manual run. detect_heredoc_lines() in test-no-heredocs-459.sh folded awk's exit status into a trailing "|| true" before the grep pass, so an unreadable/vanished file produced the same empty string as a genuinely clean one (#478). awk's own rc is now captured separately and returned as 2, with a positive/negative control pair and a live-sweep path that reports an unscanned file rather than silently treating it as clean. Self-tests added for all three; each was confirmed red against the old logic before the fix and green after. Co-Authored-By: Claude Sonnet 5 --- changelog.d/476.fixed.md | 1 + changelog.d/477.fixed.md | 1 + changelog.d/478.fixed.md | 1 + tests/test-no-heredocs-459.sh | 51 ++++++++++++++++-- tests/test-release-branch-437.sh | 72 +++++++++++++++++++++---- tests/test-release-branch-461.sh | 90 ++++++++++++++++++++++++++++---- 6 files changed, 190 insertions(+), 26 deletions(-) create mode 100644 changelog.d/476.fixed.md create mode 100644 changelog.d/477.fixed.md create mode 100644 changelog.d/478.fixed.md diff --git a/changelog.d/476.fixed.md b/changelog.d/476.fixed.md new file mode 100644 index 00000000..d2ee350c --- /dev/null +++ b/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. diff --git a/changelog.d/477.fixed.md b/changelog.d/477.fixed.md new file mode 100644 index 00000000..c0da8a8a --- /dev/null +++ b/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. diff --git a/changelog.d/478.fixed.md b/changelog.d/478.fixed.md new file mode 100644 index 00000000..2329827a --- /dev/null +++ b/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. diff --git a/tests/test-no-heredocs-459.sh b/tests/test-no-heredocs-459.sh index 01971a51..e07ea6a0 100755 --- a/tests/test-no-heredocs-459.sh +++ b/tests/test-no-heredocs-459.sh @@ -61,9 +61,23 @@ 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. detect_heredoc_lines() { - awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" \ - | grep -nE -- "$HEREDOC_PATTERN" || true + local filtered awk_rc + filtered=$(awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" 2>&1) + awk_rc=$? + if [ "$awk_rc" -ne 0 ]; then + echo "AWK_READ_FAILED($1): $filtered" >&2 + return 2 + fi + printf '%s\n' "$filtered" | grep -nE -- "$HEREDOC_PATTERN" || true } echo "=== DETECTOR SELF-TEST: a planted fixture, both directions ===" @@ -120,6 +134,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 +162,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" \ diff --git a/tests/test-release-branch-437.sh b/tests/test-release-branch-437.sh index 4977d359..f4478320 100755 --- a/tests/test-release-branch-437.sh +++ b/tests/test-release-branch-437.sh @@ -33,6 +33,46 @@ 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. Shared 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. Mirrors the +# same function in test-release-branch-461.sh. +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 + 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 +227,27 @@ _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) + 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 + ;; + esac else bad "check skipped -- the build above did not produce a tree" fi diff --git a/tests/test-release-branch-461.sh b/tests/test-release-branch-461.sh index ac963cb7..218db15e 100755 --- a/tests/test-release-branch-461.sh +++ b/tests/test-release-branch-461.sh @@ -30,6 +30,60 @@ 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. Shared 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. +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 + 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" + for f in "$BUILD" "$CHECK" "$CONFIG"; do [ -f "$f" ] || { echo "FAIL: harness guard -- $f does not exist, every assertion below is vacuous" @@ -45,8 +99,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 +123,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) ===" From edb689326c816a93570f38aeaa13fab80fee1e53 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 15:40:02 +0200 Subject: [PATCH 2/4] fix: self-review follow-up on #476/#477/#478 -- non-vacuous #477 guard, accurate duplication comment Explore's self-review of 77d5759 found two things: - The #477 self-test only re-proved bash's own "${VAR:-default}" semantics in an isolated subshell, never touching the $REF variable the real build line actually uses -- it would keep passing even if that line reverted to a hardcoded "--ref HEAD" (the #477 regression itself). Added a third self-test that greps this script's own source for the build invocation passing "$REF", confirmed it fails against the old hardcoded line and passes against the current one. - classify_check_result() is independently duplicated in both test-release-branch-437.sh and test-release-branch-461.sh (no common sourced file between these two standalone suites), but the comment in each said "shared" / "mirrors" as if one sourced the other. Corrected both comments to say DUPLICATED and flag that the two copies must be kept in sync by hand. oss:auditor's self-review additionally flagged tests/test-no-heredocs-459.sh as apparently unwired from CI -- checked against .github/workflows/tests.yml and tests/run-all.sh (which globs test-*.sh with no exclusion list) and found this finding was wrong: the file does run on all three CI platforms via the `hooks` job's `bash tests/run-all.sh` step. No action taken; not logged to trap.d. Co-Authored-By: Claude Sonnet 5 --- tests/test-release-branch-437.sh | 8 +++++--- tests/test-release-branch-461.sh | 19 +++++++++++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/tests/test-release-branch-437.sh b/tests/test-release-branch-437.sh index f4478320..dedff64a 100755 --- a/tests/test-release-branch-437.sh +++ b/tests/test-release-branch-437.sh @@ -35,10 +35,12 @@ bad() { # 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. Shared by this file's real +# (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. Mirrors the -# same function in test-release-branch-461.sh. +# 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 diff --git a/tests/test-release-branch-461.sh b/tests/test-release-branch-461.sh index 218db15e..1ad09adb 100755 --- a/tests/test-release-branch-461.sh +++ b/tests/test-release-branch-461.sh @@ -32,9 +32,12 @@ bad() { # 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. Shared by this file's real +# (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. +# 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 @@ -84,6 +87,18 @@ RESULT=$(classify_check_result 1 "FAIL something.md: offender") ) && 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" From 0acd3add3ca88747fa843108011ff569c03496fd Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 15:49:27 +0200 Subject: [PATCH 3/4] fix: second self-review round -- stop merging awk stderr into the scanned stream, correct stale offender message A re-spawned Explore review of edb6893 found three things: - detect_heredoc_lines() in test-no-heredocs-459.sh captured awk's stderr into the same text grep -nE later scans, even on success ("2>&1" applied unconditionally). A non-fatal stderr warning from a different awk implementation (gawk on Linux CI vs. the BSD/mawk this was likely tested against) could splice extra text into the data being line-numbered, corrupting the real heredoc hits it reports -- a failure mode the #478 fix itself introduced. Fixed: stderr is now discarded on success (2>/dev/null) and only ever reported (to the caller's own stderr) on the failure path; nothing from awk's stderr enters the grep pipe. - The "offenders" ok-message in test-release-branch-437.sh's case block named the #439 ALLOWED_TOOLS_BROAD hold by file, but _KNOWN_ALLOWED_TOOLS_HOLDS has been empty since #439 resolved that hold -- the message described a specific, no-longer-live exception, inside the exact hunk this lane's own #476 fix touched (the if/else -> case restructuring) without correcting it. Reworded to name the variable generically so it stays accurate whether or not that set is ever populated again. - A third finding (the #477 fix narrows, but does not fully close, a cross-job-step race if a workflow_dispatch ref= input is a moving branch name rather than a tag/sha) is a workflow-level design question beyond what #477 asked for -- logged to trap.d/477.sha-not-threaded-across-jobsteps.md for curation rather than fixed in this lane. Confirmed test-no-heredocs-459.sh and test-release-branch-437.sh still pass (437's 2 failures are the same pre-existing, environment-only `claude plugin validate --strict` mismatch confirmed unrelated to this diff by running the suite against unmodified main). Co-Authored-By: Claude Sonnet 5 --- tests/test-no-heredocs-459.sh | 19 ++++++++---- tests/test-release-branch-437.sh | 11 ++++++- .../477.sha-not-threaded-across-jobsteps.md | 30 +++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 trap.d/477.sha-not-threaded-across-jobsteps.md diff --git a/tests/test-no-heredocs-459.sh b/tests/test-no-heredocs-459.sh index e07ea6a0..be575c86 100755 --- a/tests/test-no-heredocs-459.sh +++ b/tests/test-no-heredocs-459.sh @@ -69,12 +69,21 @@ HEREDOC_PATTERN="(^|[^<])<<-?[[:space:]]*[${Q1}${Q2}]?[A-Za-z0-9_][A-Za-z0-9_]*" # 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() { - local filtered awk_rc - filtered=$(awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" 2>&1) - awk_rc=$? - if [ "$awk_rc" -ne 0 ]; then - echo "AWK_READ_FAILED($1): $filtered" >&2 + local filtered + if ! filtered=$(awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" 2> /dev/null); then + echo "AWK_READ_FAILED($1)" >&2 return 2 fi printf '%s\n' "$filtered" | grep -nE -- "$HEREDOC_PATTERN" || true diff --git a/tests/test-release-branch-437.sh b/tests/test-release-branch-437.sh index dedff64a..272c39c7 100755 --- a/tests/test-release-branch-437.sh +++ b/tests/test-release-branch-437.sh @@ -243,8 +243,17 @@ if [ "$BUILD_RC" -eq 0 ]; then 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 the known, already-filed ALLOWED_TOOLS_BROAD hold on commands/doctor.md, commands/init.md and commands/stats.md" + 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 diff --git a/trap.d/477.sha-not-threaded-across-jobsteps.md b/trap.d/477.sha-not-threaded-across-jobsteps.md new file mode 100644 index 00000000..4bcc22b3 --- /dev/null +++ b/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). From 3a38b1477380ca6d939a9d311db5594848ef2d65 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 15:54:23 +0200 Subject: [PATCH 4/4] fix: third self-review round -- restore awk's diagnostic text on the read-failure path A second-pass Explore review of 0acd3ad (scoped to that diff alone, since it touched 3 files and tripped the needs-second-pass threshold) found that discarding awk's stderr via 2>/dev/null applied on BOTH the success and the failure path. The success-path redirect is the fix that round closed (stderr noise must never splice into $filtered, which grep -nE scans); the failure-path redirect was collateral -- it regressed the diagnostic versus the pre-#478 code, which used to report awk's actual stderr text ("awk: can't open file ..."). The generic "AWK_READ_FAILED($1)" message carried no indication of why. Fixed by capturing stdout and stderr into two separate places: $filtered (awk's stdout only, used by the grep pass exactly as before) and a scratch file under $TMPD read back only on the failure branch. Confirmed the real error text ("awk: can't open file ...") now appears in the diagnostic again while the success path is unchanged. tests/test-no-heredocs-459.sh: 6/0. Co-Authored-By: Claude Sonnet 5 --- tests/test-no-heredocs-459.sh | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/test-no-heredocs-459.sh b/tests/test-no-heredocs-459.sh index be575c86..8eefac92 100755 --- a/tests/test-no-heredocs-459.sh +++ b/tests/test-no-heredocs-459.sh @@ -81,11 +81,20 @@ HEREDOC_PATTERN="(^|[^<])<<-?[[:space:]]*[${Q1}${Q2}]?[A-Za-z0-9_][A-Za-z0-9_]*" # now discarded on success and never enters the grep pipe at all; only the # exit status crosses the function boundary. detect_heredoc_lines() { - local filtered - if ! filtered=$(awk '{ if ($0 ~ /^[[:space:]]*#/) print ""; else print }' "$1" 2> /dev/null); then - echo "AWK_READ_FAILED($1)" >&2 + 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 }