From aa0a7b5fa2f87f6b639b17bcd93ee1ad5aad03e3 Mon Sep 17 00:00:00 2001 From: Mos Date: Fri, 7 Aug 2026 05:59:12 +0000 Subject: [PATCH] fix(tools/git): issue-close.sh silently dropped the closing comment (#1085) --- .woodpecker/ci.yml | 4 + .../mosaic/framework/tools/git/issue-close.sh | 23 ++- .../tools/git/test-issue-close-fail-closed.sh | 150 ++++++++++++++++++ 3 files changed, 175 insertions(+), 2 deletions(-) create mode 100755 packages/mosaic/framework/tools/git/test-issue-close-fail-closed.sh diff --git a/.woodpecker/ci.yml b/.woodpecker/ci.yml index c02f3f1c..63fb349d 100644 --- a/.woodpecker/ci.yml +++ b/.woodpecker/ci.yml @@ -46,6 +46,10 @@ steps: # [0] of the pnpm chain, so severing that chain would silence it together # with everything it guards; this direct line keeps one instrument running. - bash packages/mosaic/framework/tools/quality/scripts/check-test-enumeration.sh + # Hermetic regression for issue-close.sh (#1081): mocks tea/curl onto PATH + # and sandboxes a throwaway git repo, so it resolves no real credentials and + # joins CI directly rather than the exclusions file. + - bash packages/mosaic/framework/tools/git/test-issue-close-fail-closed.sh # Blocking gate (#791): a framework upgrade must never write or delete an # operator-owned path. The HARD GATE proves an unanticipated operator sentinel diff --git a/packages/mosaic/framework/tools/git/issue-close.sh b/packages/mosaic/framework/tools/git/issue-close.sh index 646c8b09..3d014a59 100755 --- a/packages/mosaic/framework/tools/git/issue-close.sh +++ b/packages/mosaic/framework/tools/git/issue-close.sh @@ -91,13 +91,32 @@ elif [[ "$PLATFORM" == "gitea" ]]; then GITEA_LOGIN_NAME=$(get_gitea_login || true) if [[ -n "$GITEA_LOGIN_NAME" ]]; then if [[ -n "$COMMENT" ]]; then - tea issue comment "$ISSUE_NUMBER" "$COMMENT" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME" + # `tea issue comment` is NOT a subcommand -- tea 0.11.x lists only + # list/create/edit/reopen/close under `tea issue`. Comments are the + # TOP-LEVEL `tea comment`, which takes the same --repo/--login flags. + # The old call therefore always failed, was unchecked, and the script + # closed the issue anyway, losing the record of WHY. + # + # Use `tea comment` rather than the API helper so the comment and the + # close are made by the SAME principal ($GITEA_LOGIN_NAME). Routing the + # comment through the token-authenticated helper here would attribute the + # comment to the token holder and the close to the tea login -- two + # principals for one operation. + tea comment "$ISSUE_NUMBER" "$COMMENT" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME" || { + echo "Error: failed to post comment on #$ISSUE_NUMBER -- NOT closing (fail closed)." >&2 + exit 1 + } fi tea issue close "$ISSUE_NUMBER" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME" else echo "No tea login configured for $(get_remote_host); using authenticated Gitea API fallback." >&2 if [[ -n "$COMMENT" ]]; then - gitea_issue_comment_api + # Fail closed here too: an unchecked comment lets the issue close without its + # audit trail, which is the same defect as the tea path above. + gitea_issue_comment_api || { + echo "Error: failed to post comment on #$ISSUE_NUMBER -- NOT closing (fail closed)." >&2 + exit 1 + } fi gitea_issue_close_api fi diff --git a/packages/mosaic/framework/tools/git/test-issue-close-fail-closed.sh b/packages/mosaic/framework/tools/git/test-issue-close-fail-closed.sh new file mode 100755 index 00000000..fdc774dd --- /dev/null +++ b/packages/mosaic/framework/tools/git/test-issue-close-fail-closed.sh @@ -0,0 +1,150 @@ +#!/usr/bin/env bash +# Regression: issue-close.sh must NOT close an issue when the closing comment could not +# be posted, and comment+close must be made by ONE principal. +# +# Guards two defects fixed together (see #1081): +# 1. `tea issue comment` is not a subcommand -- tea exposes comments as the TOP-LEVEL +# `tea comment`. The old call always failed, was unchecked, and the issue closed +# anyway, losing the record of WHY it was closed. +# 2. Routing the comment through the token-authenticated API helper while the close +# used --login would attribute one operation to two principals. +# +# SAFETY (rev-974, #1085 review 130): this test previously ran under `set -uo pipefail` +# with unchecked mkdir/redirect/cd, then prepended a possibly-nonexistent $MOCK_BIN to +# 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}}" +SANDBOX="$WORK_ROOT/issue-close-fail-closed-test-$$" +MOCK_BIN="$SANDBOX/bin"; REPO_DIR="$SANDBOX/repo"; CALLS="$SANDBOX/calls.log" +cleanup() { rm -rf "$SANDBOX"; } +trap cleanup EXIT + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TARGET="$SCRIPT_DIR/issue-close.sh" +[ -f "$TARGET" ] || { echo "FAIL: issue-close.sh not found beside this test"; exit 1; } +fail() { echo "FAIL: $*"; exit 1; } + +# Every setup step is checked. Under `set -e` these abort; the explicit || fail keeps the +# reason legible instead of a bare non-zero exit. +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 GITEA_URL="https://git.mosaicstack.dev" +export GITEA_TOKEN="redacted-test-token" + +cat > "$MOCK_BIN/curl" <<'EOF' +#!/bin/bash +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 +chmod +x "$MOCK_BIN/curl" + +mk_tea() { # $1 = exit code for a comment attempt; $2 = login list (empty => no login) + local rc="$1" login="${2-}" + cat > "$MOCK_BIN/tea" <> "$CALLS" +if [[ "\$*" == *"login list"* ]]; then + printf '%s\n' '${login}'; exit 0 +fi +# Fail ANY comment attempt -- both the correct top-level \`tea comment\` and the broken +# \`tea issue comment\` -- so an unfixed script exercises the DEFECT rather than tripping +# a setup assertion. +if [[ "\$1" == "comment" || ( "\$1" == "issue" && "\$2" == "comment" ) ]]; then exit $rc; fi +exit 0 +EOF + 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 +} + +run_target() { # never let a target failure abort the test; we assert on rc + # Call sites MUST use `rc=0; run_target ... || rc=$?` -- a bare `run_target ...; rc=$?` + # lets the non-zero RETURN trip set -e in the CALLER before rc is ever read. + set +e; bash "$TARGET" "$@" >/dev/null 2>&1; local rc=$?; set -e; return $rc +} + +# ── 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" + +# 2. POSITIVE: comment succeeds => close proceeds, exit 0 +mk_tea 0 "$LOGIN_JSON"; : > "$CALLS"; assert_mocked +rc=0; run_target -i 42 -c "closing note" || 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" + +# 3. must use top-level `tea comment`, never `tea issue comment` +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 +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}') +[ -n "$c" ] || fail "comment carried no --login" +[ "$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:-}'" + +# 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"