fix(tools/git): the tea diagnostic must not suppress the API fallback
be-coder-08, reviewing #1086, found the diagnostic is not diagnostic-only. At all three call sites it was written as the last command of an && list: declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist and it sits immediately BEFORE the Gitea API fallback. Under `set -e` a failing diagnostic (stderr closed or full) therefore exits the script and the fallback never runs -- a diagnostic that suppresses the recovery path it exists to explain. The asymmetry is what makes it dangerous: the fault only appears when the helper is PRESENT, so the helper-absent path -- the pre-#1086 behaviour -- keeps working and reads as a passing control. Measured on /dev/full: helper present rc=1 fallback NOT reached helper absent rc=0 fallback reached Wrapping in `{ ...; } || true` makes the diagnostic status-neutral, which is what the PR claimed to be in the first place. test-explain-diagnostic-status-neutral.sh probes all four combinations of {helper present, absent} x {stderr ok, failing} for each of the three call sites, and lifts the construct FROM THE SHIPPED FILE rather than restating it -- a probe that retypes the fixed form passes on a build whose real call sites still carry the bare && form. Verified RED on the pre-fix tree (16 failures, behavioural half included) and GREEN here. Enumerated on test:framework-shell. Reported-by: be-coder-08
This commit is contained in:
Mos
@@ -156,7 +156,7 @@ case "$PLATFORM" in
|
||||
exit 0
|
||||
fi
|
||||
echo "Warning: tea issue create failed, trying Gitea API fallback..." >&2
|
||||
declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist
|
||||
{ declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist; } || true
|
||||
fi
|
||||
gitea_issue_create_api
|
||||
;;
|
||||
|
||||
@@ -71,7 +71,7 @@ elif [[ "$PLATFORM" == "gitea" ]]; then
|
||||
exit 0
|
||||
fi
|
||||
echo "Warning: tea issue view failed, trying Gitea API fallback..." >&2
|
||||
declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist
|
||||
{ declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist; } || true
|
||||
fi
|
||||
gitea_issue_view_api
|
||||
else
|
||||
|
||||
@@ -219,7 +219,7 @@ case "$PLATFORM" in
|
||||
exit 0
|
||||
fi
|
||||
echo "Warning: tea pr create failed, trying Gitea API fallback..." >&2
|
||||
declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist
|
||||
{ declare -F explain_tea_user_does_not_exist >/dev/null && explain_tea_user_does_not_exist; } || true
|
||||
gitea_pr_create_api
|
||||
;;
|
||||
*)
|
||||
|
||||
+70
@@ -0,0 +1,70 @@
|
||||
#!/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
|
||||
# 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.
|
||||
#
|
||||
# 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.
|
||||
set -uo pipefail
|
||||
fail=0
|
||||
GIT_DIR_UNDER_TEST="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
|
||||
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)"
|
||||
}
|
||||
|
||||
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
|
||||
}
|
||||
|
||||
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.
|
||||
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"
|
||||
check "$src stderr FAILING / helper present" "$(probe present /dev/full "$src")" "0:yes"
|
||||
check "$src stderr FAILING / helper absent " "$(probe absent /dev/full "$src")" "0:yes"
|
||||
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
|
||||
done
|
||||
|
||||
[ "$fail" -eq 0 ] && echo "OK diagnostic is status-neutral" || echo "FAILED"
|
||||
exit "$fail"
|
||||
Reference in New Issue
Block a user