feat: add trap cleanup for all mktemp temp files (t135.9) #801
Conversation
Add trap EXIT/RETURN cleanup for mktemp files to prevent temp file leaks on unexpected exits. Scripts modified: - supervisor-helper.sh (4 locations) - setup.sh (3 locations) - aidevops.sh, cron-helper.sh, matrix-dispatch-helper.sh - add-skill-helper.sh, full-loop-helper.sh, objective-runner-helper.sh - coderabbit-collector-helper.sh, coderabbit-task-creator-helper.sh - schema-validator-helper.sh, sops-helper.sh, agent-test-helper.sh
Complete trap EXIT/RETURN cleanup coverage for all mktemp usages. Previous commit covered 14 scripts; this adds traps to 12 more files that were missed — loop-common.sh, quality-fix.sh, ralph-loop-helper.sh, site-crawler-helper.sh, verify-run-helper.sh, test-supervisor-state-machine.sh, and 6 archive scripts.
WalkthroughThis PR systematically adds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and reliability of the codebase by implementing comprehensive temporary file cleanup across numerous shell scripts. By systematically adding Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to improve the robustness of the scripts by ensuring temporary files are always cleaned up, adhering to the repository's style guide (line 33: 'Temp files must have trap cleanup (RETURN or EXIT)'). The changes are consistent and correctly use trap with RETURN for function-scoped cleanup and EXIT for script-scoped cleanup. I've found a few minor areas for improvement:
- In several files, an explicit
rmcommand remains after atrapwas added, making thermredundant. - In the test script
test-supervisor-state-machine.sh, multipletrap ... EXITcommands are defined, where only the last one will be active. Consolidating them into a singletrapat the top of the script would be cleaner.
My detailed comments provide suggestions to address these points. Overall, this is a solid and valuable contribution.
| tmp_file=$(mktemp) | ||
| trap 'rm -f "$tmp_file"' RETURN | ||
| jq --arg name "$name" '.skills = [.skills[] | select(.name != $name)]' "$SKILL_SOURCES" > "$tmp_file" && mv "$tmp_file" "$SKILL_SOURCES" | ||
| rm -f "$tmp_file" |
There was a problem hiding this comment.
The explicit rm -f "$tmp_file" is now redundant. The trap you've added on line 439 will handle the cleanup of the temporary file automatically when the register_skill function exits, regardless of success or failure. You can safely remove this line.
| tmp=$(mktemp) | ||
| trap 'rm -f "$tmp"' RETURN | ||
| jq '. + {"type": "module"}' "$TOOL_DIR/package.json" > "$tmp" && mv "$tmp" "$TOOL_DIR/package.json" | ||
| rm -f "$tmp" |
There was a problem hiding this comment.
The explicit rm -f "$tmp" is now redundant. The trap you've added on line 64 will handle the cleanup of the temporary file automatically when the install_deps function exits. You can safely remove this line.
| trap 'rm -f "$tmp_deb"' RETURN | ||
| curl -fsSL "$latest_url" -o "$tmp_deb" | ||
| sudo dpkg -i "$tmp_deb" | ||
| rm -f "$tmp_deb" |
There was a problem hiding this comment.
The explicit rm -f "$tmp_deb" is now redundant. The trap you've added on line 107 will handle the cleanup of the temporary file automatically when the cmd_install function exits. You can safely remove this line.
|
|
||
| # Test: run_verify_checks with a real VERIFY.md | ||
| VERIFY_TEST_DIR=$(mktemp -d) | ||
| trap 'rm -rf "$WORKTREE_TEST_DIR" "$TEST_DIR" "$VERIFY_TEST_DIR" "$CLAIM_TEST_DIR"' EXIT |
There was a problem hiding this comment.
While this trap correctly covers all temporary directories, the previous trap commands on lines 59 and 1046 are still present in the file. Each new trap command for the EXIT signal overwrites the previous one, making the earlier ones dead code. To avoid confusion and improve maintainability, you should remove the now-redundant traps on lines 59 and 1046, leaving only this consolidated one. Ideally, this single trap should be placed at the top of the script to ensure it's defined once before any temporary directories are created.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.agents/scripts/supervisor-helper.sh:
- Around line 3367-3369: The RETURN trap for cleaning up mktemp (e.g.,
probe_tmpfile created before running ("${probe_cmd[@]}" > "$probe_tmpfile" 2>&1)
&) is clobbering existing RETURN traps and will fail with set -u when the temp
var goes out of scope; fix by saving the previous RETURN trap (e.g.,
prev_trap=$(trap -p RETURN)), installing a new RETURN trap that removes the temp
file and then restores/evaluates the saved prev_trap (or clears it if empty),
and ensure this pattern is applied to every mktemp/trap pair
(probe_tmpfile/probe_cmd and the other mktemp uses noted) so the original RETURN
trap is preserved/restored after cleanup.
In `@setup.sh`:
- Line 2590: The RETURN trap for deploy_aidevops_agents currently gets
overwritten (trap 'rm -rf "$tmp_preserve"' RETURN then later trap 'rm -rf
"$tmp_file"' RETURN), which can leak tmp files if an early return occurs; fix by
creating a single cleanup function (e.g., cleanup_deploy_aidevops_agents) that
removes both "$tmp_preserve" and "$tmp_file" safely (checking for non-empty
variables and existence), set one trap 'trap cleanup_deploy_aidevops_agents
RETURN', and remove the separate inline trap calls so all temporary-file cleanup
is centralized and cannot be silently replaced.
🧹 Nitpick comments (2)
.agents/scripts/site-crawler-helper.sh (1)
478-486: Trap set inside a loop — works but is subtly fragile.Because
trap '...' RETURNis function-scoped (not loop-iteration-scoped), each iteration overwrites the previous trap. This is safe today because:
$new_queueresolves at trap-fire time (single quotes) → targets the latest file.- Each iteration's file is consumed by
mvon line 486, so the trap is effectively a no-op at function return.- No other
RETURNtrap exists incrawl_with_crawl4ai.If a second
mktemp+ trap were added to this function in the future, one cleanup would be silently lost. Consider noting this with a brief comment for the next maintainer.aidevops.sh (1)
1486-1490: Secondtrap … RETURNsilently replaces the first withincmd_upgrade_planning.Bash only keeps one handler per signal per scope. Line 1553's trap overwrites Line 1488's trap, so if the function were to exit after Line 1553 without hitting the explicit
rm -fon Line 1570, only$plans_content_filewould be cleaned up —$tasks_filecleanup would be lost.In the current code flow this is safe because
$tasks_fileis explicitly removed on Line 1505 before the second trap is set. However, the pattern is fragile: any future edit that reorders these blocks or adds an early return between them could silently leak a temp file.A more robust approach is to accumulate both files in the second trap:
♻️ Combine both cleanups into a single trap
At Line 1488, keep the original trap:
tasks_file=$(mktemp) trap 'rm -f "$tasks_file"' RETURNAt Line 1553, extend the trap to cover both files:
plans_content_file=$(mktemp) - trap 'rm -f "$plans_content_file"' RETURN + trap 'rm -f "$tasks_file" "$plans_content_file"' RETURN
rm -fon an already-deleted file is a harmless no-op, so this is safe even when$tasks_filewas cleaned up earlier on Line 1505.Also applies to: 1551-1554
| probe_tmpfile=$(mktemp) | ||
| trap 'rm -f "$probe_tmpfile"' RETURN | ||
| ("${probe_cmd[@]}" > "$probe_tmpfile" 2>&1) & |
There was a problem hiding this comment.
Avoid leaking RETURN traps (can break set -u on later returns)
These trap 'rm -f "$tmp"' RETURN lines persist beyond the function, clobber any existing RETURN traps and, with set -u, will later error when the local temp var is out of scope. That can hard-exit the supervisor unexpectedly. Please chain/restore the prior RETURN trap (or clear it) in each block. Apply the same pattern to all newly added mktemp traps.
✅ Suggested pattern (chain + restore the previous RETURN trap)
- probe_tmpfile=$(mktemp)
- trap 'rm -f "$probe_tmpfile"' RETURN
+ local _prev_return_trap
+ _prev_return_trap=$(trap -p RETURN)
+ probe_tmpfile=$(mktemp)
+ trap 'rm -f "$probe_tmpfile"; eval "$_prev_return_trap"' RETURN
@@
- probe_result=$(cat "$probe_tmpfile" 2>/dev/null || true)
- rm -f "$probe_tmpfile"
+ probe_result=$(cat "$probe_tmpfile" 2>/dev/null || true)
+ rm -f "$probe_tmpfile"
+ eval "$_prev_return_trap"Also applies to: 3397-3399, 4374-4376, 7412-7414, 7947-7949
🤖 Prompt for AI Agents
In @.agents/scripts/supervisor-helper.sh around lines 3367 - 3369, The RETURN
trap for cleaning up mktemp (e.g., probe_tmpfile created before running
("${probe_cmd[@]}" > "$probe_tmpfile" 2>&1) &) is clobbering existing RETURN
traps and will fail with set -u when the temp var goes out of scope; fix by
saving the previous RETURN trap (e.g., prev_trap=$(trap -p RETURN)), installing
a new RETURN trap that removes the temp file and then restores/evaluates the
saved prev_trap (or clears it if empty), and ensure this pattern is applied to
every mktemp/trap pair (probe_tmpfile/probe_cmd and the other mktemp uses noted)
so the original RETURN trap is preserved/restored after cleanup.
| print_info "Clean mode: removing stale files from $target_dir (preserving ${preserved_dirs[*]})" | ||
| local tmp_preserve | ||
| tmp_preserve="$(mktemp -d)" | ||
| trap 'rm -rf "$tmp_preserve"' RETURN |
There was a problem hiding this comment.
Second RETURN trap silently replaces the first in deploy_aidevops_agents.
In bash, trap ... RETURN replaces the previous RETURN trap — it does not stack. Line 2590 sets cleanup for $tmp_preserve, and line 2683 overwrites it with cleanup for $tmp_file. This is safe today because $tmp_preserve is explicitly removed at line 2615 before the second trap is set. However, it's fragile — a future refactor inserting code between lines 2590–2615 that causes an early return after the second trap is set would silently leak $tmp_preserve.
Consider consolidating into a single trap that covers both, or using an explicit cleanup function.
💡 One approach: accumulate cleanup in a helper
+ # Track all temp paths for cleanup
+ local -a _tmp_cleanup=()
+ trap 'rm -rf "${_tmp_cleanup[@]}"' RETURN
+
...
local tmp_preserve
tmp_preserve="$(mktemp -d)"
- trap 'rm -rf "$tmp_preserve"' RETURN
+ _tmp_cleanup+=("$tmp_preserve")
...
local tmp_file
tmp_file=$(mktemp)
- trap 'rm -f "$tmp_file"' RETURN
+ _tmp_cleanup+=("$tmp_file")Also applies to: 2683-2683
🤖 Prompt for AI Agents
In `@setup.sh` at line 2590, The RETURN trap for deploy_aidevops_agents currently
gets overwritten (trap 'rm -rf "$tmp_preserve"' RETURN then later trap 'rm -rf
"$tmp_file"' RETURN), which can leak tmp files if an early return occurs; fix by
creating a single cleanup function (e.g., cleanup_deploy_aidevops_agents) that
removes both "$tmp_preserve" and "$tmp_file" safely (checking for non-empty
variables and existence), set one trap 'trap cleanup_deploy_aidevops_agents
RETURN', and remove the separate inline trap calls so all temporary-file cleanup
is centralized and cannot be silently replaced.
|
Closing: duplicate of PR #800 which was already merged. The RETURN trap clobbering feedback from CodeRabbit is valid and should be addressed in a follow-up task. |
1 participant
Summary
trap 'rm -f "$var"' RETURNcleanup for all remaining mktemp usages that lacked trap protectionChanges
Active scripts (7 files):
loop-common.sh— 3 locations (receipt tracking, attempt tracking, task blocking)quality-fix.sh— 3 locations (return statement fixer, positional params, string analysis)ralph-loop-helper.sh— 1 location (iteration increment)site-crawler-helper.sh— 1 location (queue processing)verify-run-helper.sh— 1 location (verify status update)Test files (1 file):
test-supervisor-state-machine.sh— consolidated EXIT trap to cover all 4 mktemp -d dirsArchive scripts (6 files):
_archive/add-missing-returns.sh,fix-common-strings.sh,fix-s131-default-cases.sh,fix-sc2155-simple.sh,fix-shellcheck-critical.sh,fix-string-literals.shVerification
bash -nsyntax checkRETURNfor function-scoped cleanup,EXITfor script-level cleanupSummary by CodeRabbit