From 5cd6c83b9bd4f095309abab72c72031d7825f6af Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 18 Jun 2026 16:16:34 -0500 Subject: [PATCH] fix git pr metadata tmpfile cleanup --- .../t-a292e96f-gitea-pr-metadata.md | 45 ++++++ .../mosaic/framework/tools/git/pr-metadata.sh | 18 ++- .../tools/git/test-pr-metadata-gitea.sh | 149 +++++++++++++++++- 3 files changed, 204 insertions(+), 8 deletions(-) diff --git a/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md b/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md index 7ae4dfd..b12cddb 100644 --- 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`). diff --git a/packages/mosaic/framework/tools/git/pr-metadata.sh b/packages/mosaic/framework/tools/git/pr-metadata.sh index 7e8d1fc..5521150 100755 --- 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 } diff --git a/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh b/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh index e98b2e7..41ebefa 100755 --- 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 " >&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