Merge pull request #2876 from Molecule-AI/refactor/shell-e2e-tmp-cleanup

test(e2e): plug /tmp scratch leaks + add CI lint gate (RFC #2873 iter 2)

.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

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"

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")

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"

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 ==="