mosaicstack/stack
1
0
Fork 1
Code Issues 179 Pull Requests 9 Actions Packages Projects Releases 15 Wiki Activity

test(tools/git): stop the fail-closed test escaping its sandbox; cover the API path
ci/woodpecker/pr/ci Pipeline was successful
Details

Browse Source 
rev-974 (#1085 review 130) found three blockers. This closes the first two.

[1] SANDBOX ESCAPE. The test ran under `set -uo pipefail` with unchecked mkdir,
    calls-log redirect and `cd "$REPO_DIR"`, then prepended a possibly-nonexistent
    $MOCK_BIN to PATH -- while `git remote add origin` names the REAL repository.
    rev-974 forced setup failure with an unwritable AGENT_WORK_ROOT and the test
    continued past every error, ran `git init` in its CALLER's directory, added the
    real origin, and invoked its target:

        TARGET_REACHED args=-i 42 -c closing note

    With the committed target that is the real, provider-mutating issue-close.sh.
    ShellCheck flagged the unguarded cd as SC2164 independently.

    Now: `set -euo pipefail`, every setup step checked with a legible reason, and
    assert_mocked() proves BOTH `tea` and `curl` resolve inside $MOCK_BIN before any
    target invocation. Control: unwritable AGENT_WORK_ROOT -> rc=1 at mkdir, target
    never reached.

[2] THE API/no-login BRANCH HAD NO DISCRIMINATING COVERAGE. The mock always returned
    a tea login, so `gitea_issue_comment_api || fail-closed` was never executed.
    rev-974 replaced the whole fallback contract with an unconditional close --
    silently dropping the comment -- and the committed test still passed rc=0.

    Added three cases asserting the POSTCONDITION (which HTTP calls happened, in what
    order) rather than that a command ran: comment POST fails -> no PATCH and non-zero;
    comment succeeds -> strictly POST,PATCH; no comment requested -> PATCH only, never
    a POST. The curl mock now records method and URL. Control: replaying rev-974's
    contract destruction now fails with "API path: no comment POST attempted".

Two self-inflicted traps hit while adding `set -e`, both the same family as the #1086
defect be-coder-08 found, and both silent:
  - `grep -q X "$CALLS" && fail "..."` -- the ABSENT case (grep rc=1, the PASSING case
    for a must-not-appear assertion) is the last command of an && list and terminates
    the script with no message. All four converted to if-blocks.
  - `run_target ...; rc=$?` -- the function's non-zero RETURN trips set -e in the CALLER
    before rc is read; run_target's internal `set +e` protects the target, not the
    caller. All five call sites now `rc=0; run_target ... || rc=$?`.

Controls:
  unchanged main                  -> rc=1 "used 'tea issue comment'"
  fallback contract destroyed     -> rc=1 "API path: no comment POST attempted"
  unwritable AGENT_WORK_ROOT      -> rc=1 at setup, target never invoked
  fixed source                    -> rc=0

[3] remains open: with MOSAIC_GIT_IDENTITY=rev-974 the wrapper resolves
    GITEA_LOGIN_NAME=mosaicstack-mos, so one principal holds but the operation is
    attributed to Mos rather than the requested seat. That changes identity resolution
    shared by every wrapper in this directory and is not folded in here.

Reported-by: rev-974
This commit is contained in:
Mos
2026-08-07 00:33:57 -05:00
parent 2bf610c9ba
commit f214613680
1 changed files with 106 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/mosaic/framework/tools/git/test-issue-close-fail-closed.sh
+106 -35
View File
@@ -1,17 +1,26 @@
#!/usr/bin/env bash #!/usr/bin/env bash
# Regression: issue-close.sh must NOT close an issue when the closing comment # Regression: issue-close.sh must NOT close an issue when the closing comment could not
# could not be posted, and comment+close must be made by ONE principal. # be posted, and comment+close must be made by ONE principal.
# #
# Guards two defects fixed together (see #1081): # Guards two defects fixed together (see #1081):
# 1. `tea issue comment` is not a subcommand -- tea exposes comments as the # 1. `tea issue comment` is not a subcommand -- tea exposes comments as the TOP-LEVEL
# TOP-LEVEL `tea comment`. The old call always failed, was unchecked, and # `tea comment`. The old call always failed, was unchecked, and the issue closed
# the issue closed anyway, losing the record of WHY it was closed. # anyway, losing the record of WHY it was closed.
# 2. Routing the comment through the token-authenticated API helper while the # 2. Routing the comment through the token-authenticated API helper while the close
# close used --login would attribute one operation to two principals. # used --login would attribute one operation to two principals.
# #
# Fully offline: `tea` and `curl` are mocked onto PATH, the repo is a throwaway # SAFETY (rev-974, #1085 review 130): this test previously ran under `set -uo pipefail`
# `git init`, everything lives under $SANDBOX, removed on EXIT. # with unchecked mkdir/redirect/cd, then prepended a possibly-nonexistent $MOCK_BIN to
set -uo pipefail # PATH -- while `git remote add origin` names the REAL repository. Forcing setup failure
# with an unwritable AGENT_WORK_ROOT made it `git init` in its CALLER's directory and
# invoke the real, provider-mutating issue-close.sh. Setup now fails closed, and both
# `tea` and `curl` are asserted to resolve INSIDE $MOCK_BIN before any target run.
set -euo pipefail
# NOTE: with `set -e`, `grep -q X && fail "..."` is a trap -- the ABSENT case (grep rc=1,
# which is the PASSING case for a must-not-appear assertion) is the last command of an &&
# list and silently terminates the script with no message. Every must-not-appear check
# below is therefore an if-block. This is the same set -e + &&-list defect be-coder-08
# found in #1086, reintroduced here by adding `set -e` for the sandbox-safety fix.
WORK_ROOT="${AGENT_WORK_ROOT:-${TMPDIR:-/tmp}}" WORK_ROOT="${AGENT_WORK_ROOT:-${TMPDIR:-/tmp}}"
SANDBOX="$WORK_ROOT/issue-close-fail-closed-test-$$" SANDBOX="$WORK_ROOT/issue-close-fail-closed-test-$$"
@@ -24,56 +33,118 @@ TARGET="$SCRIPT_DIR/issue-close.sh"
[ -f "$TARGET" ] || { echo "FAIL: issue-close.sh not found beside this test"; exit 1; } [ -f "$TARGET" ] || { echo "FAIL: issue-close.sh not found beside this test"; exit 1; }
fail() { echo "FAIL: $*"; exit 1; } fail() { echo "FAIL: $*"; exit 1; }
mkdir -p "$MOCK_BIN" "$REPO_DIR"; : > "$CALLS" # Every setup step is checked. Under `set -e` these abort; the explicit || fail keeps the
cd "$REPO_DIR"; git init -q # reason legible instead of a bare non-zero exit.
git remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git mkdir -p "$MOCK_BIN" "$REPO_DIR" || fail "setup: cannot create sandbox under $WORK_ROOT"
: > "$CALLS" || fail "setup: cannot write calls log at $CALLS"
cd "$REPO_DIR" || fail "setup: cannot cd into $REPO_DIR"
git init -q || fail "setup: git init failed"
git remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git || fail "setup: git remote add failed"
export PATH="$MOCK_BIN:$PATH" CALLS export PATH="$MOCK_BIN:$PATH" CALLS
export GITEA_LOGIN="git.mosaicstack.dev"
export GITEA_URL="https://git.mosaicstack.dev" export GITEA_URL="https://git.mosaicstack.dev"
export GITEA_TOKEN="redacted-test-token" export GITEA_TOKEN="redacted-test-token"
cat > "$MOCK_BIN/curl" <<'EOF' cat > "$MOCK_BIN/curl" <<'EOF'
#!/bin/bash #!/bin/bash
printf 'curl %q ' "$@" >> "$CALLS"; printf '\n' >> "$CALLS"; exit 0 method=GET; url=""
while [ $# -gt 0 ]; do
case "$1" in
-X) method="$2"; shift 2 ;;
http*|https*) url="$1"; shift ;;
*) shift ;;
esac
done
printf 'curl %s %s\n' "$method" "$url" >> "$CALLS"
[ "${MOCK_CURL_FAIL:-}" = "1" ] && [ "$method" = "POST" ] && exit 22
exit 0
EOF EOF
chmod +x "$MOCK_BIN/curl" chmod +x "$MOCK_BIN/curl"
mk_tea() { # $1 = exit code for `tea comment` mk_tea() { # $1 = exit code for a comment attempt; $2 = login list (empty => no login)
cat > "$MOCK_BIN/tea" <<EOF local rc="$1" login="${2-}"
cat > "$MOCK_BIN/tea" <<EOF
#!/bin/bash #!/bin/bash
printf 'tea %s\n' "\$*" >> "$CALLS" printf 'tea %s\n' "\$*" >> "$CALLS"
if [[ "\$*" == *"login list"* ]]; then if [[ "\$*" == *"login list"* ]]; then
echo '[{"name":"git.mosaicstack.dev","url":"https://git.mosaicstack.dev"}]'; exit 0 printf '%s\n' '${login}'; exit 0
fi fi
# Fail ANY comment attempt -- both the correct top-level \`tea comment\` and the # Fail ANY comment attempt -- both the correct top-level \`tea comment\` and the broken
# broken \`tea issue comment\` -- so the test exercises the DEFECT on an unfixed # \`tea issue comment\` -- so an unfixed script exercises the DEFECT rather than tripping
# script rather than failing on a setup assertion. # a setup assertion.
if [[ "\$1" == "comment" || ( "\$1" == "issue" && "\$2" == "comment" ) ]]; then exit $1; fi if [[ "\$1" == "comment" || ( "\$1" == "issue" && "\$2" == "comment" ) ]]; then exit $rc; fi
exit 0 exit 0
EOF EOF
chmod +x "$MOCK_BIN/tea" chmod +x "$MOCK_BIN/tea"
}
LOGIN_JSON='[{"name":"git.mosaicstack.dev","url":"https://git.mosaicstack.dev"}]'
# The mocks must be the ones that run. Without this, a failed setup silently falls through
# to the real tea/curl and the "test" mutates the real provider.
assert_mocked() {
local w
for w in tea curl; do
p=$(command -v "$w" || true)
[ -n "$p" ] || fail "SAFETY: $w does not resolve at all"
case "$p" in
"$MOCK_BIN"/*) : ;;
*) fail "SAFETY: $w resolves to $p, OUTSIDE the sandbox -- refusing to invoke the target" ;;
esac
done
} }
# ── 1. NEGATIVE (the regression): comment fails => must NOT close, must exit non-zero run_target() { # never let a target failure abort the test; we assert on rc
mk_tea 1; : > "$CALLS" # Call sites MUST use `rc=0; run_target ... || rc=$?` -- a bare `run_target ...; rc=$?`
bash "$TARGET" -i 42 -c "closing note" >/dev/null 2>&1; rc=$? # lets the non-zero RETURN trip set -e in the CALLER before rc is ever read.
grep -qE 'tea (issue )?comment' "$CALLS" || fail "no comment attempt at all -- setup did not reach the tea branch" set +e; bash "$TARGET" "$@" >/dev/null 2>&1; local rc=$?; set -e; return $rc
grep -q 'tea issue close' "$CALLS" && fail "ISSUE CLOSED AFTER THE COMMENT FAILED -- the regression" }
# ── tea path ────────────────────────────────────────────────────────────────────────
# 1. NEGATIVE (the regression): comment fails => must NOT close, must exit non-zero
mk_tea 1 "$LOGIN_JSON"; : > "$CALLS"; assert_mocked
rc=0; run_target -i 42 -c "closing note" || rc=$?
grep -qE 'tea (issue )?comment' "$CALLS" || fail "no comment attempt -- setup did not reach the tea branch"
if grep -q 'tea issue close' "$CALLS"; then fail "ISSUE CLOSED AFTER THE COMMENT FAILED -- the regression"; fi
[ "$rc" -ne 0 ] || fail "comment failed but issue-close exited 0 -- FAIL-OPEN" [ "$rc" -ne 0 ] || fail "comment failed but issue-close exited 0 -- FAIL-OPEN"
# ── 2. POSITIVE (rule MET must pass): comment succeeds => close proceeds, exit 0 # 2. POSITIVE: comment succeeds => close proceeds, exit 0
mk_tea 0; : > "$CALLS" mk_tea 0 "$LOGIN_JSON"; : > "$CALLS"; assert_mocked
bash "$TARGET" -i 42 -c "closing note" >/dev/null 2>&1; rc=$? rc=0; run_target -i 42 -c "closing note" || rc=$?
[ "$rc" -eq 0 ] || fail "comment succeeded but issue-close exited $rc" [ "$rc" -eq 0 ] || fail "comment succeeded but issue-close exited $rc"
grep -q 'tea issue close' "$CALLS" || fail "issue not closed even though the comment succeeded" grep -q 'tea issue close' "$CALLS" || fail "issue not closed even though the comment succeeded"
# ── 3. must use top-level `tea comment`, never `tea issue comment` # 3. must use top-level `tea comment`, never `tea issue comment`
grep -q 'tea issue comment' "$CALLS" && fail "used 'tea issue comment' -- not a valid subcommand" if grep -q 'tea issue comment' "$CALLS"; then fail "used 'tea issue comment' -- not a valid subcommand"; fi
# ── 4. ONE PRINCIPAL: comment and close must carry the SAME --login # 4. ONE PRINCIPAL: comment and close must carry the SAME --login
c=$(grep -m1 '^tea comment' "$CALLS" | grep -o -- '--login [^ ]*' | awk '{print $2}') c=$(grep -m1 '^tea comment' "$CALLS" | grep -o -- '--login [^ ]*' | awk '{print $2}')
k=$(grep -m1 '^tea issue close' "$CALLS" | grep -o -- '--login [^ ]*' | awk '{print $2}') k=$(grep -m1 '^tea issue close' "$CALLS" | grep -o -- '--login [^ ]*' | awk '{print $2}')
[ -n "$c" ] || fail "comment carried no --login" [ -n "$c" ] || fail "comment carried no --login"
[ "$c" = "$k" ] || fail "MIXED PRINCIPALS: comment=$c close=$k" [ "$c" = "$k" ] || fail "MIXED PRINCIPALS: comment=$c close=$k"
# ── no-login / API fallback path ────────────────────────────────────────────────────
# rev-974: the delta also adds fail-closed behaviour to this branch, and the suite never
# reached it -- replacing the whole fallback contract with an unconditional close still
# passed. These assert the POSTCONDITION (which HTTP calls happened, in what order),
# not merely that a command ran.
# 5. no login + comment FAILS => POST attempted, NO PATCH, non-zero
mk_tea 0 ""; : > "$CALLS"; assert_mocked
rc=0; MOCK_CURL_FAIL=1 run_target -i 42 -c "closing note" || rc=$?
grep -q 'curl POST' "$CALLS" || fail "API path: no comment POST attempted"
if grep -q 'curl PATCH' "$CALLS"; then fail "API path: ISSUE CLOSED (PATCH) AFTER THE COMMENT POST FAILED"; fi
[ "$rc" -ne 0 ] || fail "API path: comment failed but exited 0 -- FAIL-OPEN"
# 6. no login + comment SUCCEEDS => POST strictly BEFORE PATCH, exit 0
mk_tea 0 ""; : > "$CALLS"; assert_mocked
rc=0; run_target -i 42 -c "closing note" || rc=$?
[ "$rc" -eq 0 ] || fail "API path: comment succeeded but exited $rc"
order=$(grep -oE 'curl (POST|PATCH)' "$CALLS" | awk '{print $2}' | paste -sd, -)
[ "$order" = "POST,PATCH" ] || fail "API path: expected POST,PATCH -- got '${order:-<none>}'"
# 7. no login + NO comment => PATCH only, never a POST
mk_tea 0 ""; : > "$CALLS"; assert_mocked
rc=0; run_target -i 42 || rc=$?
[ "$rc" -eq 0 ] || fail "API path: no-comment close exited $rc"
if grep -q 'curl POST' "$CALLS"; then fail "API path: posted a comment when none was requested"; fi
grep -q 'curl PATCH' "$CALLS" || fail "API path: issue not closed when no comment was requested"
echo "issue-close.sh fail-closed + single-principal regression passed" echo "issue-close.sh fail-closed + single-principal regression passed"