From 6125700c393d1d119fea7d65d11c2c96718b76c6 Mon Sep 17 00:00:00 2001
From: Hongming Wang <hongmingwang.rabbit@users.noreply.github.com>
Date: Tue, 5 May 2026 04:21:26 -0700
Subject: [PATCH] test(e2e): plug /tmp scratch leaks in 3 shell E2E tests + add
 CI lint gate (RFC #2873 iter 2)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Three shell E2E tests created scratch files via `mktemp` but never
deleted them on early exit (assertion failure, SIGINT, errexit). Each
CI run leaked ~10-100 KB of /tmp into the runner; over ~200 runs/week
that's 20+ MB of accumulated cruft.

## Files

- **test_chat_attachments_e2e.sh** — was missing both trap and rm;
  added per-run TMPDIR_E2E with `trap rm -rf … EXIT INT TERM`.
- **test_notify_attachments_e2e.sh** — had a `cleanup()` for the
  workspace but didn't include the TMPF; only an unconditional
  `rm -f` at the bottom (line 233) which doesn't fire on early exit.
  Extended cleanup() to also rm the scratch + dropped the redundant
  trailing rm.
- **test_chat_attachments_multiruntime_e2e.sh** — `round_trip()`
  function had per-call `rm -f` only on the success path; failure
  paths leaked. Switched to script-level TMPDIR_E2E + trap; per-call
  rm dropped (the trap handles every return path including SIGINT).

Pattern: `mktemp -d -t prefix-XXX` for the dir, `mktemp <full-template>`
for files (portable across BSD/macOS + GNU coreutils — `-p` is
GNU-only and breaks Mac local-dev runs).

## Regression gate

New `tests/e2e/lint_cleanup_traps.sh` asserts every `*.sh` that calls
`mktemp` also has a `trap … EXIT` line in the file. Wired into the
existing Shellcheck (E2E scripts) CI step. Verified locally: passes
on the fixed state, fails-loud when one of the 3 fixes is reverted.

## Verification

  - shellcheck --severity=warning clean on all 4 touched files
  - lint_cleanup_traps.sh passes on the post-fix tree (6 mktemp users,
    all have EXIT trap)
  - Negative test: revert one fix → lint exits 1 with file:line +
    suggested fix pattern in the error message (CI-grokkable
    ::error file=… annotation)
  - Trap fires on SIGTERM mid-run (smoke-tested on macOS BSD mktemp)
  - Trap fires on `exit 1` (smoke-tested)

## Bars met (7-axis)

  - SSOT: trap pattern documented in lint message (one rule, one fix)
  - Cleanup: this IS the cleanup hygiene fix
  - 100% coverage: lint catches future regressions across all
    `tests/e2e/*.sh` files, not just the 3 fixed today
  - File-split: N/A (no files split)
  - Plugin / abstract / modular: N/A (test infra, not product code)

Iteration 2 of RFC #2873.
---
 .github/workflows/ci.yml                      |  8 ++++
 tests/e2e/lint_cleanup_traps.sh               | 40 +++++++++++++++++++
 tests/e2e/test_chat_attachments_e2e.sh        | 11 ++++-
 .../test_chat_attachments_multiruntime_e2e.sh | 20 ++++++++--
 tests/e2e/test_notify_attachments_e2e.sh      | 14 ++++++-
 5 files changed, 87 insertions(+), 6 deletions(-)
 create mode 100755 tests/e2e/lint_cleanup_traps.sh

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index a642e1a3..f2512cb9 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -272,6 +272,14 @@ jobs:
           find tests/e2e infra/scripts -type f -name '*.sh' -print0 \
             | xargs -0 shellcheck --severity=warning
 
+      - if: needs.changes.outputs.scripts == 'true'
+        name: Lint cleanup-trap hygiene (RFC #2873)
+        # Asserts every shell E2E test that calls `mktemp` also installs
+        # an EXIT trap. Catches the /tmp-leak class — a missing trap
+        # silently leaks scratch into CI runners (~10-100KB per run).
+        # See tests/e2e/lint_cleanup_traps.sh for the rule + fix pattern.
+        run: bash tests/e2e/lint_cleanup_traps.sh
+
       - if: needs.changes.outputs.scripts == 'true'
         name: Run E2E bash unit tests (no live infra)
         # Pure-bash unit tests for E2E helper libs (lib/*.sh). These pin
diff --git a/tests/e2e/lint_cleanup_traps.sh b/tests/e2e/lint_cleanup_traps.sh
new file mode 100755
index 00000000..a20d9a6f
--- /dev/null
+++ b/tests/e2e/lint_cleanup_traps.sh
@@ -0,0 +1,40 @@
+#!/usr/bin/env bash
+# lint_cleanup_traps.sh — regression gate for the OSS-shape program's
+# "all E2E tests must have proper cleanup" bar (RFC #2873).
+#
+# Asserts: every shell file under tests/e2e/ that calls `mktemp` ALSO
+# installs an `EXIT` trap somewhere in the file. The trap is the
+# minimum-viable guarantee that scratch files won't leak when an
+# assertion or curl exits the script non-zero.
+#
+# Why this lints (instead of the test runner enforcing): shell scripts
+# can't easily be wrapped by an outer harness without breaking the
+# `WSID=… ./test_x.sh` invocation contract. Static gate is the cheap
+# defense.
+#
+# Usage:
+#   tests/e2e/lint_cleanup_traps.sh
+#
+# Exits non-zero if any test_*.sh has unmatched mktemp/trap. CI invokes
+# it from the existing Shellcheck (E2E scripts) workflow.
+
+set -euo pipefail
+
+cd "$(dirname "$0")"
+
+violations=0
+for f in test_*.sh; do
+  if grep -qE '\bmktemp\b' "$f"; then
+    if ! grep -qE 'trap[[:space:]]+.*EXIT' "$f"; then
+      echo "::error file=tests/e2e/$f::has 'mktemp' but no 'trap … EXIT' — scratch will leak when test exits non-zero. Pattern: TMPDIR_E2E=\$(mktemp -d -t prefix-XXX); trap 'rm -rf \"\$TMPDIR_E2E\"' EXIT INT TERM"
+      violations=$((violations + 1))
+    fi
+  fi
+done
+
+if [ "$violations" -gt 0 ]; then
+  echo "::error::$violations shell E2E file(s) leak scratch on early exit. See above."
+  exit 1
+fi
+
+echo "✓ all $(grep -lE '\bmktemp\b' test_*.sh | wc -l | tr -d ' ') shell E2E files with mktemp also install an EXIT trap"
diff --git a/tests/e2e/test_chat_attachments_e2e.sh b/tests/e2e/test_chat_attachments_e2e.sh
index 76f46706..3d352f05 100755
--- a/tests/e2e/test_chat_attachments_e2e.sh
+++ b/tests/e2e/test_chat_attachments_e2e.sh
@@ -22,6 +22,13 @@ set -euo pipefail
 WSID="${WSID:?WSID=<workspace-id> required}"
 BASE="${BASE:-http://localhost:8080}"
 
+# Per-run scratch dir collected under one trap so every mktemp leak path
+# (assertion failure, SIGINT, exit non-zero) is plugged. Pre-fix this test
+# created a /tmp/hermes-e2e-XXXXXX.txt and never deleted it — ~10 KB ×
+# every CI run leaked into the runner. RFC #2873 cleanup-hygiene PR.
+TMPDIR_E2E=$(mktemp -d -t chat-attachments-e2e-XXXXXX)
+trap 'rm -rf "$TMPDIR_E2E"' EXIT INT TERM
+
 log() { printf "\n=== %s ===\n" "$*"; }
 
 log "Preflight: workspace online?"
@@ -29,7 +36,9 @@ STATUS=$(curl -s "$BASE/workspaces/$WSID" | python3 -c 'import json,sys;print(js
 [ "$STATUS" = "online" ] || { echo "workspace not online ($STATUS)"; exit 1; }
 
 log "Step 1 — Upload a text file via /chat/uploads"
-TEST_FILE=$(mktemp -t hermes-e2e-XXXXXX.txt)
+# `mktemp <full-template>` is portable across BSD (macOS) + GNU; -p is
+# GNU-only and breaks local dev runs on Mac.
+TEST_FILE=$(mktemp "$TMPDIR_E2E/hermes-e2e-XXXXXX.txt")
 echo "secret code: $(openssl rand -hex 4)-$(openssl rand -hex 4)" > "$TEST_FILE"
 EXPECTED=$(cat "$TEST_FILE" | awk '{print $NF}')
 UPLOAD=$(curl -s -X POST "$BASE/workspaces/$WSID/chat/uploads" -F "files=@$TEST_FILE")
diff --git a/tests/e2e/test_chat_attachments_multiruntime_e2e.sh b/tests/e2e/test_chat_attachments_multiruntime_e2e.sh
index 9601132e..3d93e186 100755
--- a/tests/e2e/test_chat_attachments_multiruntime_e2e.sh
+++ b/tests/e2e/test_chat_attachments_multiruntime_e2e.sh
@@ -24,6 +24,15 @@ set -uo pipefail
 BASE="${BASE:-http://localhost:8080}"
 fails=0
 
+# Per-run scratch dir collected under one trap so every per-runtime
+# round_trip mktemp leak path (assertion failure, SIGINT, exit
+# non-zero, function early-return between mktemp and rm) is plugged.
+# Pre-fix, round_trip's `rm -f "$test_file"` only fired on the success
+# path inside the function — every test_failure path before the rm
+# leaked the scratch into /tmp permanently. RFC #2873 cleanup-hygiene PR.
+TMPDIR_E2E=$(mktemp -d -t mr-attachments-e2e-XXXXXX)
+trap 'rm -rf "$TMPDIR_E2E"' EXIT INT TERM
+
 has_patch_in_container() {
   local container="$1"
   # Signal that platform helpers are available AND wired into the
@@ -74,12 +83,16 @@ print(f"executor: claude-code monkey-patch active ({name})")
 round_trip() {
   local label="$1" wsid="$2"
   local test_file expected upload uri payload reply reply_text
-  test_file=$(mktemp -t e2e-mr-XXXX.txt)
+  # Scratch goes under TMPDIR_E2E; the script-level trap rm -rf's the
+  # whole dir on exit, so per-file rm calls are unnecessary AND make
+  # error paths leak when forgotten.
+  # `mktemp <full-template>` is portable across BSD (macOS) + GNU; -p is GNU-only.
+  test_file=$(mktemp "$TMPDIR_E2E/e2e-mr-${label}-XXXX.txt")
   expected="secret-$(openssl rand -hex 6)"
   echo "$expected" > "$test_file"
   upload=$(curl -s -X POST "$BASE/workspaces/$wsid/chat/uploads" -F "files=@$test_file")
   uri=$(echo "$upload" | python3 -c 'import json,sys;print(json.load(sys.stdin)["files"][0]["uri"])' 2>/dev/null)
-  [ -z "$uri" ] && { echo "FAIL $label: upload returned no URI: $upload"; rm -f "$test_file"; return 1; }
+  [ -z "$uri" ] && { echo "FAIL $label: upload returned no URI: $upload"; return 1; }
   payload=$(URI="$uri" python3 -c '
 import json, os
 uri = os.environ["URI"]
@@ -103,7 +116,8 @@ try:
 except Exception as exc:
     print(f"(parse failed: {exc})")
 ' 2>&1)
-  rm -f "$test_file"
+  # $test_file lives under TMPDIR_E2E; the script-level trap rm -rf's
+  # the dir on exit, covering every return path including SIGINT.
 
   if echo "$reply_text" | grep -qF "$expected"; then
     echo "PASS $label round-trip: agent quoted $expected"
diff --git a/tests/e2e/test_notify_attachments_e2e.sh b/tests/e2e/test_notify_attachments_e2e.sh
index 6957c7e3..2a5f12a7 100755
--- a/tests/e2e/test_notify_attachments_e2e.sh
+++ b/tests/e2e/test_notify_attachments_e2e.sh
@@ -29,11 +29,20 @@ FAIL=0
 WSID=""
 
 cleanup() {
+  # Workspace teardown — best-effort, ignore errors so an unrelated CP
+  # outage doesn't shadow a real test failure.
   if [ -n "$WSID" ]; then
     curl -s -X DELETE "$BASE/workspaces/$WSID?confirm=true" > /dev/null || true
   fi
+  # /tmp scratch — pre-fix only ran on success path (the unconditional
+  # rm at the bottom of the script). Trap-based path lets the file leak
+  # whenever the script exits non-zero before reaching the rm. RFC #2873
+  # cleanup-hygiene PR.
+  if [ -n "${TMPF:-}" ]; then
+    rm -f "$TMPF"
+  fi
 }
-trap cleanup EXIT
+trap cleanup EXIT INT TERM
 
 assert() {
   local label="$1"
@@ -230,7 +239,8 @@ for r in rows:
   assert "stored URI matches uploaded URI" "$STORED_URI" "$URI"
 fi
 
-rm -f "$TMPF"
+# $TMPF cleanup happens via the trap-cleanup function above — covers
+# both the success path and any early exit / SIGINT.
 
 echo ""
 echo "=== Results: $PASS passed, $FAIL failed ==="