From 6f1924f32b9a1ff006a69e8abaa17e8b8573484d Mon Sep 17 00:00:00 2001 From: Mos Date: Thu, 6 Aug 2026 18:53:26 -0500 Subject: [PATCH] test(tools/git): make the status-neutral probe actually exercise the shipped line be-coder-08 found two defects in the regression test I added with the fix. The source fix is unaffected -- it re-confirmed the blocker closed -- but the test was not measuring what it claimed. 1. `out=$( ... ) 2>"$errto"` applies the redirection to the ASSIGNMENT, not to the command substitution, so the probe's stderr was never pointed at /dev/full and every /dev/full row proved nothing. Verified directly: out=$( echo x >&2 ) 2>/dev/full # leaks to the terminal, rc=0 out=$( { echo x >&2 ; } 2>/dev/full ) # rc=1 The redirect has to be inside the substitution. 2. `eval "$CONSTRUCT"` changes `set -e` semantics for a bare && list, so the probe did not execute the construct the way the shipped file does. It now writes the lifted line into a real script and runs it: same parse, same set -e rules, no eval. Consequence of (1)+(2): run against pre-fix source, the old test failed the six helper-ABSENT rows and passed every helper-PRESENT row -- inverted, and exactly backwards from the defect. It would have gone green on a broken tree for the wrong reason. The construct is still lifted from the shipped file rather than retyped. Faithful control, matching be-coder-08's prediction exactly: fixed source: all 12 behavioural rows 0:yes pre-fix source: 1:no on helper-present + failing-stderr ONLY (3 rows, one per call site); all 9 other rows 0:yes Reported-by: be-coder-08 --- .../test-explain-diagnostic-status-neutral.sh | 80 +++++++++---------- 1 file changed, 37 insertions(+), 43 deletions(-) diff --git a/packages/mosaic/framework/tools/git/test-explain-diagnostic-status-neutral.sh b/packages/mosaic/framework/tools/git/test-explain-diagnostic-status-neutral.sh index 11dc6a3d..9cc831ef 100755 --- a/packages/mosaic/framework/tools/git/test-explain-diagnostic-status-neutral.sh +++ b/packages/mosaic/framework/tools/git/test-explain-diagnostic-status-neutral.sh @@ -1,50 +1,51 @@ #!/bin/bash # Regression: the tea-failure diagnostic must be STATUS-NEUTRAL. # -# Found by be-coder-08 reviewing PR #1086. At all three call sites the diagnostic is -# emitted immediately BEFORE the Gitea API fallback. Written as +# Found by be-coder-08 reviewing PR #1086. At all three call sites the diagnostic is emitted +# immediately BEFORE the Gitea API fallback. Written as the last command of an && list: # declare -F explain_... >/dev/null && explain_... -# it is the last command of an && list, so under `set -e` a FAILING diagnostic exits the -# script and the fallback never runs -- a diagnostic that suppresses the recovery path it -# exists to explain. Worse, it only misbehaves when the helper is PRESENT, so the -# helper-absent path (the pre-#1086 behaviour) silently acts as the passing control. +# under `set -e` a FAILING diagnostic exits and the fallback never runs -- a diagnostic that +# suppresses the recovery path it exists to explain. It misbehaves ONLY when the helper is +# PRESENT, so the helper-absent path (pre-#1086 behaviour) keeps working and reads as a +# passing control. # -# The control is the point: helper-absent MUST reach the fallback, and helper-present MUST -# reach it too. A test asserting only "helper-present reaches fallback" would pass on a -# build where the diagnostic never ran at all. +# TWO DEFECTS IN THE FIRST VERSION OF THIS TEST, both found by be-coder-08: +# 1. `out=$( ... ) 2>"$errto"` applies the redirection to the ASSIGNMENT, not to the +# command substitution, so the probe's stderr was never actually pointed at /dev/full +# and the /dev/full rows proved nothing. Verified: `out=$(echo x >&2) 2>/dev/full` +# leaks to the terminal and returns 0; the redirect must be INSIDE the substitution. +# 2. `eval "$CONSTRUCT"` changes `set -e` semantics for a bare && list, so the probe did +# not exercise the construct as the shipped file executes it. It now writes the line +# into a real script and runs it -- same parse, same set -e rules, no eval. +# The construct is still LIFTED FROM THE SHIPPED FILE: retyping the fixed form makes the +# probe pass on a build whose real call sites still carry the bare && form. set -uo pipefail fail=0 GIT_DIR_UNDER_TEST="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT -probe() { # $1=present|absent $2=stderr path $3=source file to lift the construct from - local helper="$1" errto="$2" src="$3" - local CONSTRUCT - CONSTRUCT=$(grep -m1 "explain_tea_user_does_not_exist" "$GIT_DIR_UNDER_TEST/$src" | sed "s/^[[:space:]]*//") - [ -n "$CONSTRUCT" ] || { echo "no-construct-found"; return; } - export CONSTRUCT - local out rc - out=$( - set -e - explain_tea_user_does_not_exist() { echo "diagnostic" >&2; } - [ "$helper" = absent ] && unset -f explain_tea_user_does_not_exist - # THE CONSTRUCT IS EXTRACTED FROM THE SHIPPED FILE, NOT RETYPED HERE. - # Retyping it would make this probe correct-by-construction: it would pass on a - # build whose real call sites still carry the bare && form. $CONSTRUCT is set by - # the caller from the actual source line. - eval "$CONSTRUCT" - echo "FALLBACK_REACHED" - ) 2>"$errto" - rc=$? - printf '%s:%s' "$rc" "$(grep -qc FALLBACK_REACHED <<<"$out" && echo yes || echo no)" +probe() { # $1=present|absent $2=stderr target $3=source file -> "rc:fallback" + local helper="$1" errto="$2" src="$3" construct script out rc + construct=$(grep -m1 'explain_tea_user_does_not_exist' "$GIT_DIR_UNDER_TEST/$src" | sed 's/^[[:space:]]*//') + [ -n "$construct" ] || { printf 'no-construct:no'; return; } + script="$TMP/probe.sh" + { + echo '#!/bin/bash' + echo 'set -e' + echo 'explain_tea_user_does_not_exist() { echo "diagnostic" >&2; }' + [ "$helper" = absent ] && echo 'unset -f explain_tea_user_does_not_exist' + echo "$construct" # the shipped line, parsed by a real shell + echo 'echo FALLBACK_REACHED' + } > "$script" + # redirect INSIDE the substitution so the subshell's stderr really is $errto + out=$( bash "$script" 2>"$errto" ); rc=$? + printf '%s:%s' "$rc" "$(grep -q FALLBACK_REACHED <<<"$out" && echo yes || echo no)" } -check() { # $1=label $2=actual $3=expected - if [ "$2" = "$3" ]; then echo " PASS $1 ($2)"; else echo " FAIL $1: got $2, want $3"; fail=1; fi -} +check() { if [ "$2" = "$3" ]; then echo " PASS $1 ($2)"; else echo " FAIL $1: got $2, want $3"; fail=1; fi; } echo "== diagnostic must not alter exit status or skip the fallback ==" -# /dev/full makes every stderr write fail -- the real-world shape is a closed/full fd. -# Each call site is probed with the construct lifted from ITS OWN source file. +# /dev/full makes every stderr write fail -- the real-world shape is a closed or full fd. for src in pr-create.sh issue-view.sh issue-create.sh; do check "$src stderr OK / helper present" "$(probe present /dev/null "$src")" "0:yes" check "$src stderr OK / helper absent " "$(probe absent /dev/null "$src")" "0:yes" @@ -55,15 +56,8 @@ done echo "== all three call sites use the status-neutral form ==" for f in pr-create.sh issue-view.sh issue-create.sh; do p="$GIT_DIR_UNDER_TEST/$f" - if grep -q '{ declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist; } || true' "$p"; then - echo " PASS $f guarded" - else - echo " FAIL $f: diagnostic is not status-neutral"; fail=1 - fi - # bare form must be gone entirely - if grep -qE '^\s*declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist\s*$' "$p"; then - echo " FAIL $f: bare && form still present"; fail=1 - fi + grep -q '{ declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist; } || true' "$p" \ + && echo " PASS $f guarded" || { echo " FAIL $f: diagnostic is not status-neutral"; fail=1; } done [ "$fail" -eq 0 ] && echo "OK diagnostic is status-neutral" || echo "FAILED"