fix(regen-defs): Fix globs and clean tempfiles on INT, TERM #381
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds signal handling to the regen-defs.zsh script to clean up temporary files when interrupted, and uses eval to fix glob expansion in cpplint commands. The script processes .def test fixture files that contain cpplint commands with glob patterns (like src/*) and their expected outputs.
- Added trap handler for INT and TERM signals to clean up temporary files on interruption
- Changed command execution to use
evalto properly expand glob patterns in the commands - Refactored cleanup logic into a dedicated function
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Clean up temporary files | ||
| rm "$stdout_file" "$stderr_file" | ||
| cleanup |
There was a problem hiding this comment.
The cleanup function is only called on successful completion of a loop iteration (line 51), but not when errors occur earlier. If uv run fails or if any operation between lines 23-48 causes the script to exit, the temporary files will not be cleaned up.
Additionally, the trap handler won't catch normal exit conditions (like the exit calls on lines 6 and 11). Consider:
- Adding
EXITto the trap signals:trap cleanup INT TERM EXIT - Moving the trap setup earlier in the script to catch early exits
Note: If you add EXIT to the trap, you may need to prevent double-cleanup on line 51.
There was a problem hiding this comment.
I don't think a command in between can cause termination of the entire script, and as mentioned trapping EXIT can get a bit messy.
|
@aaronliu0130 I've opened a new pull request, #415, to work on those changes. Once the pull request is ready, I'll request review from you. |
I do not think "attackers controlling .def content" is a plausible scenario, but this does seem to be the better tool for the job.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a cleanup function and installs an INT/TERM trap to remove temporary stdout/stderr files; switches cpplint invocation to use an evaluated/executed command line; replaces direct Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
regen-defs.zsh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (3.x, windows-latest)
- GitHub Check: build-test (3.10, windows-latest)
🔇 Additional comments (2)
regen-defs.zsh (2)
51-51: Cleanup function call is well-placed.Calling
cleanupafter successful iteration completion ensures temp files are removed on the normal (non-signal) path. Combined with the trap handler, this provides cleanup in both success and interruption scenarios.
33-33: Zsh syntax${(~)cmd}is correct for enabling glob and tilde expansion.The syntax
${(~)cmd}is indeed the correct zsh parameter expansion flag for enablingGLOB_SUBST. This causes the expanded variable value to undergo glob pattern matching and tilde expansion—wildcards and~characters in the.deffile's first line will be expanded as patterns before being passed touv run "$cpplint".This aligns with the PR objective to fix globbing. However, the security concern you raised is legitimate: since
.deffile content is read directly without validation (line 20) and then expanded with glob patterns enabled (line 33), any glob patterns in.deffiles will be evaluated by the shell. This is safe only if.deffiles are from trusted sources. If.deffiles can be supplied by untrusted users, consider adding validation or escaping mechanisms to prevent unintended pattern expansion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
regen-defs.zsh (2)
26-30: Add-fflag tormfor idempotent cleanup.The cleanup function should use
rm -finstead of plainrmto prevent errors if the temporary files don't exist and to make cleanup idempotent. This is especially important for signal handlers, which may be invoked at unexpected times.Apply this diff:
- cleanup() { - rm "$stdout_file" "$stderr_file" - } + cleanup() { + rm -f "$stdout_file" "$stderr_file" + }This aligns with the best practice discussed in prior review feedback for robust cleanup handlers.
33-33: Add a security note documenting that.deffiles must be trusted.Using
evalto execute the command line extracted from.deffiles requires that those files remain under your control. If a.deffile is compromised or maliciously modified, arbitrary code could execute. While this is a developer-internal tool and.deffiles are repo-controlled, it's good practice to document this assumption.Apply this diff to add a clarifying comment:
+ # Note: eval is used to enable glob expansion. .def files must be trusted; + # do not run this script on untrusted .def file input. eval uv run "$cpplint" $cmd > "$stdout_file" 2> "$stderr_file"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
regen-defs.zsh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (3.10, windows-latest)
- GitHub Check: build-test (3.x, windows-latest)
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.