fix(regen-defs): Fix globs and clean tempfiles on INT, TERM #381

aaronliu0130 · 2025-04-24T00:10:19Z

Summary by CodeRabbit

Bug Fixes
- Improved signal handling so temporary output files are reliably cleaned up if the script is interrupted.
Refactor
- Centralized temporary-file cleanup into a single routine and updated how the linting command is invoked to integrate with that cleanup flow.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

regen-defs.zsh

Copilot

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 eval to 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.

regen-defs.zsh

Copilot · 2025-11-28T17:53:43Z

regen-defs.zsh


    # Clean up temporary files
-    rm "$stdout_file" "$stderr_file"
+    cleanup


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 EXIT to 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.

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.

Copilot · 2025-11-28T20:25:52Z

@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.

coderabbitai · 2025-11-28T20:37:34Z

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Adds 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 rm calls with the new cleanup function across the script.

Changes

Cohort / File(s)	Change Summary
Signal handling, cleanup, and cpplint invocation `regen-defs.zsh`	Adds a `cleanup` function to remove temp stdout/stderr files and installs a trap for `INT` and `TERM`; replaces direct `rm` calls with `cleanup` invocations; runs `cpplint` via `eval` (or equivalent) to incorporate the extracted command line/arguments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

Verify the evaluated cpplint command preserves quoting/argument separation and does not introduce shell injection risks.
Confirm the trap fires and cleanup runs on SIGINT/SIGTERM and that all temp files are removed for all exit paths.
Check there are no remaining code paths that call rm directly on the same temp files.

Poem

🐰 I hopped in the shell with a tidy design,
I trap every signal and clear every line.
Temp files are gone with a small, graceful sweep,
cpplint runs tidy — the codebase can sleep. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main changes: fixing globs and adding proper cleanup of temporary files on INT/TERM signals.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch tempreglob

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b32e1f3 and 7abe1d2.

📒 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 cleanup after 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 enabling GLOB_SUBST. This causes the expanded variable value to undergo glob pattern matching and tilde expansion—wildcards and ~ characters in the .def file's first line will be expanded as patterns before being passed to uv run "$cpplint".

This aligns with the PR objective to fix globbing. However, the security concern you raised is legitimate: since .def file content is read directly without validation (line 20) and then expanded with glob patterns enabled (line 33), any glob patterns in .def files will be evaluated by the shell. This is safe only if .def files are from trusted sources. If .def files can be supplied by untrusted users, consider adding validation or escaping mechanisms to prevent unintended pattern expansion.

regen-defs.zsh

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

regen-defs.zsh (2)
26-30: Add -f flag to rm for idempotent cleanup.

The cleanup function should use rm -f instead of plain rm to 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 .def files must be trusted.

Using eval to execute the command line extracted from .def files requires that those files remain under your control. If a .def file is compromised or maliciously modified, arbitrary code could execute. While this is a developer-internal tool and .def files 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7abe1d2 and e12e484.

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

fix(regen-defs): Fix globs and clean tempfiles on INT, TERM

7fe652d

aaronliu0130 requested a review from cclauss April 24, 2025 00:17

cclauss reviewed Apr 24, 2025

View reviewed changes

regen-defs.zsh Show resolved Hide resolved

aaronliu0130 requested a review from Copilot November 28, 2025 17:51

Copilot started reviewing on behalf of aaronliu0130 November 28, 2025 17:51 View session

Copilot finished reviewing on behalf of aaronliu0130 November 28, 2025 17:53

Copilot AI reviewed Nov 28, 2025

View reviewed changes

Copilot AI mentioned this pull request Nov 28, 2025

Confirm tempfile cleanup logic is correct in regen-defs.zsh #415

Closed

use wildcard parameter expansion instead of eval

7abe1d2

I do not think "attackers controlling .def content" is a plausible scenario, but this does seem to be the better tool for the job.

aaronliu0130 temporarily deployed to testpypi November 28, 2025 20:37 — with GitHub Actions Inactive

coderabbitai bot reviewed Nov 28, 2025

View reviewed changes

regen-defs.zsh Show resolved Hide resolved

back to eval—switch to expansion caused all kinds of breakage

e12e484

aaronliu0130 temporarily deployed to testpypi November 28, 2025 23:06 — with GitHub Actions Inactive

coderabbitai bot reviewed Nov 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(regen-defs): Fix globs and clean tempfiles on INT, TERM #381

fix(regen-defs): Fix globs and clean tempfiles on INT, TERM #381

Uh oh!

aaronliu0130 commented Apr 24, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

aaronliu0130 Nov 28, 2025

Uh oh!

Copilot AI commented Nov 28, 2025

Uh oh!

coderabbitai bot commented Nov 28, 2025 •

edited

Loading

Other AI code review bot(s) detected

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(regen-defs): Fix globs and clean tempfiles on INT, TERM #381

Are you sure you want to change the base?

fix(regen-defs): Fix globs and clean tempfiles on INT, TERM #381

Uh oh!

Conversation

aaronliu0130 commented Apr 24, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

aaronliu0130 Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 28, 2025

Uh oh!

coderabbitai bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaronliu0130 commented Apr 24, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 28, 2025 •

edited

Loading