disinto-admin/disinto

Fork 0

fix: fix: vault_request RETURN trap fires prematurely when vault-env.sh is sourced (#773) #904

Merged

dev-qwen merged 1 commit from fix/issue-773 into main

2026-04-16 19:11:44 +00:00

dev-qwen commented

2026-04-16 18:23:19 +00:00

Collaborator

Fixes #773

Changes

Fixes #773 ## Changes

dev-qwen added 1 commit 2026-04-16 18:23:19 +00:00

fix: fix: vault_request RETURN trap fires prematurely when vault-env.sh is sourced (#773 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

ci/woodpecker/pr/secret-scan Pipeline was successful

Details

24d6441e14

review-bot commented

2026-04-16 18:38:19 +00:00

Collaborator

AI Review

Analysis

The PR correctly identifies the root cause: the RETURN trap set before source "$vault_env" fires prematurely when the source triggers a return in the enclosing function. The two explicit rm -f "$tmp_toml" calls added for the early-exit paths (file-not-found and source-failure, lines 138–149) are correct.

Blocker

critical lib/action-vault.sh:155: The replacement trap leaks $tmp_toml on every path except the low-tier branch.

trap 'rm -f "$tmp_toml"' EXIT   # single-quoted — $tmp_toml NOT expanded here

tmp_toml is a local variable (declared at line 129). EXIT fires when the shell exits — long after vault_request() has returned and its locals are gone. At EXIT time $tmp_toml expands to empty string, so rm -f "" is a no-op. The temp file is never removed.

Paths where $tmp_toml is now leaked (none of these have an explicit rm -f and all reach the EXIT-trap zone):

line 160 — validation failure (return 1)
line 231 — SHA lookup failure (return 1)
line 241 — branch creation failure (return 1)
line 262 — file upload failure (return 1)
line 271 — PR creation failure (return 1)
line 315 — success path (return 0)

The low-tier path (line 183–187) is OK because it sets a RETURN trap after the source call; RETURN fires while locals are still in scope.

Required fix

Two equivalent options:

Option A — move the RETURN trap to after the source call (same semantics as before, no premature firing):

# after: FORGE_TOKEN="${_saved_forge_token:-}"
trap 'rm -f "$tmp_toml"' RETURN

Option B — expand $tmp_toml at trap-set time using double quotes (EXIT remains valid):

trap "rm -f '$tmp_toml'" EXIT

Either approach also removes the need for explicit rm -f on the early-error paths (lines 138 and 149), though keeping them is harmless.

Verdict

REQUEST_CHANGES — EXIT trap with single-quoted local variable leaks temp files on all non-early-return paths

Reviewed at 24d6441 | AGENTS.md

## AI Review  ### Analysis The PR correctly identifies the root cause: the RETURN trap set *before* `source "$vault_env"` fires prematurely when the source triggers a return in the enclosing function. The two explicit `rm -f "$tmp_toml"` calls added for the early-exit paths (file-not-found and source-failure, lines 138–149) are correct. ### Blocker **critical** `lib/action-vault.sh:155`: The replacement trap leaks `$tmp_toml` on every path except the low-tier branch. ```bash trap 'rm -f "$tmp_toml"' EXIT # single-quoted — $tmp_toml NOT expanded here ``` `tmp_toml` is a `local` variable (declared at line 129). EXIT fires when the *shell* exits — long after `vault_request()` has returned and its locals are gone. At EXIT time `$tmp_toml` expands to empty string, so `rm -f ""` is a no-op. The temp file is never removed. **Paths where `$tmp_toml` is now leaked** (none of these have an explicit `rm -f` and all reach the EXIT-trap zone): - line 160 — validation failure (`return 1`) - line 231 — SHA lookup failure (`return 1`) - line 241 — branch creation failure (`return 1`) - line 262 — file upload failure (`return 1`) - line 271 — PR creation failure (`return 1`) - line 315 — success path (`return 0`) The low-tier path (line 183–187) is OK because it sets a RETURN trap *after* the source call; RETURN fires while locals are still in scope. ### Required fix Two equivalent options: **Option A** — move the RETURN trap to after the source call (same semantics as before, no premature firing): ```bash # after: FORGE_TOKEN="${_saved_forge_token:-}" trap 'rm -f "$tmp_toml"' RETURN ``` **Option B** — expand `$tmp_toml` at trap-set time using double quotes (EXIT remains valid): ```bash trap "rm -f '$tmp_toml'" EXIT ``` Either approach also removes the need for explicit `rm -f` on the early-error paths (lines 138 and 149), though keeping them is harmless. ### Verdict **REQUEST_CHANGES** — EXIT trap with single-quoted local variable leaks temp files on all non-early-return paths --- *Reviewed at `24d6441` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-16 18:38:19 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — EXIT trap with single-quoted local variable leaks temp files on all non-early-return paths

AI Review: **REQUEST_CHANGES** — EXIT trap with single-quoted local variable leaks temp files on all non-early-return paths

dev-qwen force-pushed fix/issue-773 from 24d6441e14 to f1f60e555c

2026-04-16 18:42:57 +00:00

Compare

review-bot commented

2026-04-16 18:57:16 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

EXIT trap with single-quoted local variable leaks temp files → PARTIAL: trap signal corrected (EXIT→RETURN), but the fix created a new leak on the two pre-trap early-exit paths (see below).

New Issue Introduced by Fix

critical lib/action-vault.sh:138,147: Both early-exit return 1 paths now leak $tmp_toml.

The RETURN trap at line 153 is only active for returns that happen after it is registered. The two pre-source early-exit paths execute before the trap is set:

line 138 — vault_env file not found → return 1 (no trap installed, no rm -f)
line 147 — source "$vault_env" fails → return 1 (no trap installed, no rm -f)

The previous review's Option A description said the explicit rm -f calls on those paths were no longer needed — that was incorrect. The RETURN trap covers only returns occurring after trap ... RETURN executes; it does not retroactively cover earlier returns. Removing both explicit rm -f calls while leaving the trap after those code paths creates exactly the leak the fix was meant to prevent, just on different paths.

Required fix: restore the explicit rm -f "$tmp_toml" before each pre-trap return 1:

if [ ! -f "$vault_env" ]; then
  echo "ERROR: vault-env.sh not found at $vault_env" >&2
  rm -f "$tmp_toml"
  return 1
fi

if ! source "$vault_env"; then
  FORGE_TOKEN="${_saved_forge_token:-}"
  echo "ERROR: failed to source vault-env.sh" >&2
  rm -f "$tmp_toml"
  return 1
fi

The RETURN trap at line 153 then correctly covers all remaining return paths (validation failure, SHA lookup failure, branch creation failure, file upload failure, PR creation failure, success).

Verdict

REQUEST_CHANGES — early-exit paths at lines 138 and 147 leak $tmp_toml — RETURN trap is set after them, and explicit rms were removed

Reviewed at f1f60e5 | Previous: 24d6441 | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - EXIT trap with single-quoted local variable leaks temp files → **PARTIAL**: trap signal corrected (EXIT→RETURN), but the fix created a new leak on the two pre-trap early-exit paths (see below). ### New Issue Introduced by Fix **critical** `lib/action-vault.sh:138,147`: Both early-exit `return 1` paths now leak `$tmp_toml`. The RETURN trap at line 153 is only active for returns that happen *after* it is registered. The two pre-source early-exit paths execute before the trap is set: - **line 138** — `vault_env` file not found → `return 1` (no trap installed, no `rm -f`) - **line 147** — `source "$vault_env"` fails → `return 1` (no trap installed, no `rm -f`) The previous review's Option A description said the explicit `rm -f` calls on those paths were no longer needed — that was incorrect. The RETURN trap covers only returns occurring *after* `trap ... RETURN` executes; it does not retroactively cover earlier returns. Removing both explicit `rm -f` calls while leaving the trap after those code paths creates exactly the leak the fix was meant to prevent, just on different paths. **Required fix**: restore the explicit `rm -f "$tmp_toml"` before each pre-trap `return 1`: ```bash if [ ! -f "$vault_env" ]; then echo "ERROR: vault-env.sh not found at $vault_env" >&2 rm -f "$tmp_toml" return 1 fi ``` ```bash if ! source "$vault_env"; then FORGE_TOKEN="${_saved_forge_token:-}" echo "ERROR: failed to source vault-env.sh" >&2 rm -f "$tmp_toml" return 1 fi ``` The RETURN trap at line 153 then correctly covers all remaining return paths (validation failure, SHA lookup failure, branch creation failure, file upload failure, PR creation failure, success). ### Verdict **REQUEST_CHANGES** — early-exit paths at lines 138 and 147 leak $tmp_toml — RETURN trap is set after them, and explicit rms were removed --- *Reviewed at `f1f60e5` | Previous: `24d6441` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-16 18:57:16 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Re-review (round 2): REQUEST_CHANGES — early-exit paths at lines 138 and 147 leak $tmp_toml — RETURN trap is set after them, and explicit rms were removed

AI Re-review (round 2): **REQUEST_CHANGES** — early-exit paths at lines 138 and 147 leak $tmp_toml — RETURN trap is set after them, and explicit rms were removed

dev-qwen force-pushed fix/issue-773 from f1f60e555c to 96870d9f30

2026-04-16 19:02:55 +00:00

Compare

review-bot commented

2026-04-16 19:11:02 +00:00

Collaborator

AI Re-review (round 3)

Previous Findings\n- Early-exit paths at lines 138 and 147 leak `$tmp_toml` (RETURN trap set after them, explicit rms removed) → FIXED: `rm -f "$tmp_toml"` added before each pre-trap `return 1` (file-not-found at :138, source-failure at :148). RETURN trap at line 155 now correctly covers all post-source return paths.\n\n### New Issues\n\nNone.

Verdict

APPROVE — Both early-exit paths now have explicit rm -f before return 1; RETURN trap at line 155 covers all remaining paths.

Reviewed at 96870d9 | Previous: f1f60e5 | AGENTS.md

## AI Re-review (round 3)  ### Previous Findings\n- Early-exit paths at lines 138 and 147 leak `$tmp_toml` (RETURN trap set after them, explicit rms removed) → **FIXED**: `rm -f "$tmp_toml"` added before each pre-trap `return 1` (file-not-found at :138, source-failure at :148). RETURN trap at line 155 now correctly covers all post-source return paths.\n\n### New Issues\n\nNone. ### Verdict **APPROVE** — Both early-exit paths now have explicit rm -f before return 1; RETURN trap at line 155 covers all remaining paths. --- *Reviewed at `96870d9` | Previous: `f1f60e5` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-16 19:11:02 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 3): APPROVE — Both early-exit paths now have explicit rm -f before return 1; RETURN trap at line 155 covers all remaining paths.

AI Re-review (round 3): **APPROVE** — Both early-exit paths now have explicit rm -f before return 1; RETURN trap at line 155 covers all remaining paths.