fix git pr metadata tmpfile cleanup

2026-06-18 16:16:34 -05:00
parent b0b2c20da0
commit 5cd6c83b9b
3 changed files with 204 additions and 8 deletions
--- a/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md
+++ b/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md
@@ -51,3 +51,48 @@ This repository currently has no root `CHANGELOG.md`; the scratchpad and `docs/T
  - PR #1908: `Dry run: would merge PR #1908 on git.uscllc.com with authenticated Gitea API fallback (base=main, method=squash).`
 - PR: `https://git.mosaicstack.dev/mosaicstack/stack/pulls/518`, branch `fix/t-a292e96f-gitea-pr-metadata`.
 - CI: Recent PR/push pipelines failed before clone/test execution due Woodpecker/Kubernetes PVC API timeout: `dial tcp 10.43.0.1:443: i/o timeout`. No repository test step executed in CI; local targeted verification above remains clean.
+
+## 2026-06-18 — PR #549 functional blocker remediation
+
+### Assignment
+
+Coordinator `mos-claude` assigned remediation for PR #549: fix `packages/mosaic/framework/tools/git/pr-metadata.sh` tmpfile cleanup where an `EXIT` trap references function-local `body_file` after the function returns inside `RAW=$(...)`, producing `body_file: unbound variable` on the authenticated success path and failing to clean up safely on early `set -e` exits.
+
+### Plan
+
+1. Add a non-vacuous Gitea test that exercises `curl_gitea_pull` with stubbed `curl` and `GITEA_TOKEN` instead of `MOSAIC_GITEA_PR_METADATA_RAW_FILE`.
+2. Prove the new test is RED against the current PR head.
+3. Replace the function-local `EXIT` cleanup with robust function-scoped tmpfile cleanup.
+4. Re-run targeted tests, `bash -n`, and review gates; commit and push branch only. Do not merge.
+
+### Constraints / assumptions
+
+- Do not modify prior injection/JSON fixes in `issue-edit`, `issue-assign`, or `milestone-create`.
+- Worker role: do not modify `docs/TASKS.md`; orchestrator remains the single writer.
+- Budget: no explicit token cap provided; keep scope to shell wrapper + targeted regression harness.
+
+### Remediation results
+
+- Rebased `fix/tooling-eval-injection-jq-json` onto `origin/main`; branch was already current.
+- Added a curl-stub regression path that does not use `MOSAIC_GITEA_PR_METADATA_RAW_FILE`, so it exercises `curl_gitea_pull` and its temp body file.
+- RED evidence: copied the new harness next to the pre-fix `HEAD` version of `pr-metadata.sh`; `MOSAIC_TEST_WORK_DIR=$PWD/.mosaic-test-work/pr-metadata-red-work .../test-pr-metadata-gitea.sh` failed with `body_file: unbound variable` on the curl success path.
+- Fix: replaced `EXIT` temp-file cleanup with a `RETURN`-scoped cleanup function that removes the body file while the function-local variable is still in scope, preserves the original return status, and clears the `RETURN` trap.
+- GREEN evidence:
+  - `MOSAIC_TEST_WORK_DIR=$PWD/.mosaic-test-work/pr-metadata-gitea-current packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh` passed.
+  - `bash -n packages/mosaic/framework/tools/git/pr-metadata.sh packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh` passed.
+  - `shellcheck -x -P . -e SC1090 packages/mosaic/framework/tools/git/pr-metadata.sh packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh` passed.
+
+### Review remediation
+
+- Codex review returned one should-fix: the early-exit test used `chmod 000`, which is not root-safe in container CI.
+- Remediation: changed the stubbed 2xx/cat-failure mode to replace the curl output with a broken symlink, which fails deterministically even as root and still validates cleanup via `rm -f -- "$body_file"`.
+
+### Second review remediation
+
+- Codex review found the 2xx `cat "$body_file"` read could be masked under command substitution semantics because the branch returned 0 unconditionally.
+- Remediation: both authenticated 2xx branches now use `cat "$body_file" || return $?` before returning success.
+- Strengthened the broken-symlink test to require the body-read failure and reject the later `Gitea API returned non-JSON` parse-failure path, so the test verifies the helper-level failure propagation rather than eventual downstream failure.
+
+### Final review gate
+
+- Codex review after remediation: approved (`0 blockers, 0 should-fix, 0 suggestions`).
--- a/packages/mosaic/framework/tools/git/pr-metadata.sh
+++ b/packages/mosaic/framework/tools/git/pr-metadata.sh
@@ -56,15 +56,21 @@ curl_gitea_pull() {
    local api_url="$1"
    local token basic_auth raw_code body_file http_code
    body_file=$(mktemp)
-    # Ensure the tmpfile is removed even on early exit (set -e, SIGINT, etc.)
-    trap 'rm -f "$body_file"' EXIT
+
+    # shellcheck disable=SC2329 # Invoked by the RETURN trap below.
+    cleanup_gitea_pull_body() {
+        local status=$?
+        rm -f -- "$body_file"
+        trap - RETURN
+        return "$status"
+    }
+    trap cleanup_gitea_pull_body RETURN

    token=$(get_gitea_token "$HOST" || true)
    if [[ -n "$token" ]]; then
        raw_code=$(curl -sS -w '%{http_code}' -o "$body_file" -H "User-Agent: curl/8" -H "Authorization: token $token" "$api_url" || true)
        if [[ "$raw_code" =~ ^2 ]]; then
-            cat "$body_file"
-            rm -f "$body_file"
+            cat "$body_file" || return $?
            return 0
        fi
        http_code="$raw_code"
@@ -74,8 +80,7 @@ curl_gitea_pull() {
    if [[ -n "$basic_auth" ]]; then
        raw_code=$(curl -sS -w '%{http_code}' -o "$body_file" -u "$basic_auth" -H "User-Agent: curl/8" "$api_url" || true)
        if [[ "$raw_code" =~ ^2 ]]; then
-            cat "$body_file"
-            rm -f "$body_file"
+            cat "$body_file" || return $?
            return 0
        fi
        http_code="$raw_code"
@@ -98,7 +103,6 @@ except Exception:
    message = open(path, encoding="utf-8", errors="replace").read()[:200] or "empty response"
 print(f"Error: Gitea pull request API request failed with HTTP {code}: {message}")
 PY
-    rm -f "$body_file"
    return 1
 }

--- a/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh
+++ b/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh
@@ -7,9 +7,10 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 WORK_DIR="${MOSAIC_TEST_WORK_DIR:-$PWD/.mosaic-test-work/pr-metadata-gitea}"
 REPO_DIR="$WORK_DIR/repo"
 FIXTURE_DIR="$WORK_DIR/fixtures"
+STUB_DIR="$WORK_DIR/stubs"

 rm -rf "$WORK_DIR"
-mkdir -p "$REPO_DIR" "$FIXTURE_DIR"
+mkdir -p "$REPO_DIR" "$FIXTURE_DIR" "$STUB_DIR"

 git -C "$REPO_DIR" init -q
 git -C "$REPO_DIR" remote add origin https://git.uscllc.com/USC/uconnect.git
@@ -56,6 +57,150 @@ cat > "$FIXTURE_DIR/gitea-error.json" <<'JSON'
 {"message": "user does not exist [uid: 0, name: ]", "url": "https://git.uscllc.com/api/swagger"}
 JSON

+cat > "$STUB_DIR/curl" <<'SH'
+#!/usr/bin/env bash
+set -euo pipefail
+
+output_file=""
+while [[ $# -gt 0 ]]; do
+    case "$1" in
+        -o)
+            output_file="$2"
+            shift 2
+            ;;
+        -w|-H|-u)
+            shift 2
+            ;;
+        -s|-S|-sS)
+            shift
+            ;;
+        *)
+            shift
+            ;;
+    esac
+done
+
+if [[ -z "$output_file" ]]; then
+    echo "curl stub expected -o <output_file>" >&2
+    exit 2
+fi
+
+case "${MOSAIC_STUB_CURL_MODE:-success}" in
+    success)
+        cat > "$output_file" <<'JSON'
+{
+  "number": 1910,
+  "title": "Live curl path",
+  "state": "open",
+  "user": {"login": "edith"},
+  "head": {"ref": "fix/live-curl-path"},
+  "base": {"ref": "main"},
+  "html_url": "https://git.example.test/acme/widgets/pulls/1910"
+}
+JSON
+        printf '200'
+        ;;
+    cat-fails-after-2xx)
+        rm -f -- "$output_file"
+        ln -s /nonexistent/pr-metadata-body "$output_file"
+        printf '200'
+        ;;
+    *)
+        echo "unknown MOSAIC_STUB_CURL_MODE=${MOSAIC_STUB_CURL_MODE:-}" >&2
+        exit 2
+        ;;
+esac
+SH
+chmod +x "$STUB_DIR/curl"
+
+assert_tmpdir_empty() {
+    local tmpdir="$1" leftover
+    leftover=$(find "$tmpdir" -mindepth 1 -print -quit)
+    if [[ -n "$leftover" ]]; then
+        echo "Expected tmpfile cleanup, found leftover: $leftover" >&2
+        find "$tmpdir" -mindepth 1 -maxdepth 1 -ls >&2
+        exit 1
+    fi
+}
+
+run_curl_success_case() {
+    local tmpdir="$WORK_DIR/tmp-success" stderr_file="$WORK_DIR/curl-success.stderr"
+    local output status
+    mkdir -p "$tmpdir"
+
+    set +e
+    output=$(cd "$REPO_DIR" && \
+        PATH="$STUB_DIR:$PATH" \
+        TMPDIR="$tmpdir" \
+        GITEA_TOKEN="stub-token" \
+        GITEA_URL="https://git.example.test" \
+        MOSAIC_STUB_CURL_MODE="success" \
+        "$SCRIPT_DIR/pr-metadata.sh" -n 1910 2>"$stderr_file")
+    status=$?
+    set -e
+
+    if [[ "$status" -ne 0 ]]; then
+        echo "Expected curl success path to pass, got status $status" >&2
+        cat "$stderr_file" >&2
+        exit 1
+    fi
+    if grep -q "unbound variable" "$stderr_file"; then
+        echo "curl success path emitted unbound-variable cleanup noise" >&2
+        cat "$stderr_file" >&2
+        exit 1
+    fi
+    assert_tmpdir_empty "$tmpdir"
+
+    PR_METADATA_OUTPUT="$output" python3 - <<'PY'
+import json
+import os
+
+data = json.loads(os.environ["PR_METADATA_OUTPUT"])
+assert data["number"] == 1910, data
+assert data["baseRefName"] == "main", data
+assert data["headRefName"] == "fix/live-curl-path", data
+PY
+}
+
+run_curl_early_exit_cleanup_case() {
+    local tmpdir="$WORK_DIR/tmp-early-exit" stderr_file="$WORK_DIR/curl-early-exit.stderr"
+    local output status
+    mkdir -p "$tmpdir"
+
+    set +e
+    output=$(cd "$REPO_DIR" && \
+        PATH="$STUB_DIR:$PATH" \
+        TMPDIR="$tmpdir" \
+        GITEA_TOKEN="stub-token" \
+        GITEA_URL="https://git.example.test" \
+        MOSAIC_STUB_CURL_MODE="cat-fails-after-2xx" \
+        "$SCRIPT_DIR/pr-metadata.sh" -n 1910 2>"$stderr_file")
+    status=$?
+    set -e
+
+    if [[ "$status" -eq 0 ]]; then
+        echo "Expected unreadable 2xx body path to fail" >&2
+        printf '%s\n' "$output" >&2
+        exit 1
+    fi
+    if grep -q "unbound variable" "$stderr_file"; then
+        echo "curl early-exit path emitted unbound-variable cleanup noise" >&2
+        cat "$stderr_file" >&2
+        exit 1
+    fi
+    if ! grep -q "No such file or directory" "$stderr_file"; then
+        echo "Expected body-read failure from broken symlink path" >&2
+        cat "$stderr_file" >&2
+        exit 1
+    fi
+    if grep -q "Gitea API returned non-JSON" "$stderr_file"; then
+        echo "curl helper masked body-read failure as later JSON parsing failure" >&2
+        cat "$stderr_file" >&2
+        exit 1
+    fi
+    assert_tmpdir_empty "$tmpdir"
+}
+
 run_case() {
    local fixture="$1" expected_number="$2" expected_head="$3"
    local output
@@ -77,6 +222,8 @@ PY
 run_case "$FIXTURE_DIR/gitea-standard.json" 1905 edith/t_39ce717c-authentik-smoke-gate
 run_case "$FIXTURE_DIR/gitea-fallback.json" 1908 fix/fallback-head
 run_case "$FIXTURE_DIR/gitea-refs-pull-label.json" 1908 fix/t_23fa9e1d-portal-health-backend
+run_curl_success_case
+run_curl_early_exit_cleanup_case

 if cd "$REPO_DIR" && MOSAIC_GITEA_PR_METADATA_RAW_FILE="$FIXTURE_DIR/gitea-error.json" "$SCRIPT_DIR/pr-metadata.sh" -n 1909 >/dev/null 2>"$WORK_DIR/error.log"; then
    echo "Expected API error fixture to fail" >&2