GeezerVison

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.

Actionable comments posted: 1

Actionable comments posted: 0

The signal handler cleans the temp files but does not terminate the script. In zsh, a handled INT or TERM resumes after the trap; a minimal reproduction prints CLEANED and then CONTINUED_AFTER_INT.

Here that means regen-defs.zsh can continue after Ctrl-C and rewrite the current .def file from partial output. Please have the handler clean up and exit with the matching signal status, and add a small regression check for the interrupted path.

Nice to see the trap actually terminating now, but I think 53a1696 has a side effect: exit went into cleanup, and cleanup is also what replaced the rm at the end of each loop iteration.

# Clean up temporary files
cleanup      # <- ends in `exit`, so the success path exits too

So the run stops after the first .def file instead of continuing through the rest.

Also, trap cleanup INT TERM invokes the function with no arguments — $1 is only set in the TRAPINT()-style form — so exit $((128 + $1)) isn't valid arithmetic on either path.

Splitting the two responsibilities keeps the loop intact and still satisfies the signal-status request:

cleanup() { rm -f "$stdout_file" "$stderr_file"; }
trap 'cleanup; exit 130' INT
trap 'cleanup; exit 143' TERM

With the end-of-iteration call left as plain cleanup. I verified this control flow in bash (loop completes; INT exits 130) — I don't have the environment here, so you'll want to confirm on your side.

Unrelated to the title: e89746c swaps $HOME/Documents/cpplint for $HOME/Projects/cpplint, which just trades one local layout for another. Deriving it would work for everyone:

cpplint="${0:A:h}/cpplint.py"

(set before the cd samples/).

eval also drops the argument boundary provided by the quotes around $cpplint. For example, with:

cpplint='/tmp/cpplint checkout/cpplint.py'
cmd='--flag'
eval uv run "$cpplint" $cmd

the second parse passes /tmp/cpplint and checkout/cpplint.py as separate arguments. The previous non-eval form passed the configured path as one argument. This affects any checkout path containing whitespace, and deriving the path from the script location would still have the same problem if a parent directory contains a space.

Could the command expansion preserve $cpplint as one shell word through the second parse (for example, using zsh-safe quoting), with a regression case for a path containing whitespace? The Python suite is green on both the exact head (222 passed) and the current merge ref (231 passed), but it does not exercise regen-defs.zsh.

Current head e89746c still has two merge blockers.

cleanup() now exits, but the normal success path calls cleanup at the end of every iteration, so the script stops after the first .def. The INT/TERM trap also calls cleanup without a signal-number argument, so $1 is not valid for the exit-status arithmetic.

Separately, eval reparses the command and can split a quoted cpplint path containing whitespace.

Please separate ordinary cleanup from signal-exit handling and preserve the cpplint path as one shell word.

Reviewed current head e89746c. I reproduced the current control-flow and command-construction problems and don't think this head is ready to merge.

The main cleanup issue is that cleanup() now has two incompatible responsibilities:

cleanup() {
  rm "$stdout_file" "$stderr_file"
  exit $((128 + $1))
}

It is installed as the INT/TERM trap, but it is also called directly on the normal success path after each .def file. That normal call supplies no $1, so cleanup should not contain unconditional signal-exit logic.

There is a related trap issue: trap cleanup INT TERM calls the ordinary cleanup function rather than using the special Zsh TRAPINT / TRAPTERM function form where the signal number is provided as $1. The current 128 + $1 assumption therefore does not match the handler form being used.

I also confirmed the eval path-boundary problem. Because the command is reparsed, a cpplint path containing whitespace can be split into multiple argv elements even though "$cpplint" appears quoted in the source.

A cleaner design is to keep normal cleanup side-effect-only, for example:

cleanup() {
  rm -f -- "$stdout_file" "$stderr_file"
}

and give INT/TERM their exit behavior separately. For the eval path, Zsh's (q) parameter-expansion flag is specifically intended to quote a value that will be re-evaluated, so ${(q)cpplint} looks like the appropriate way to preserve that argv boundary if eval remains necessary.

I also don't think changing $HOME/Documents/cpplint to $HOME/Projects/cpplint improves portability; that hunk just exchanges one machine-specific checkout path for another.

I inspected the real .def commands as well. They need word splitting, unquoted glob expansion, and preservation of the existing quoted --exclude=... argument, which explains why simply removing all shell-style parsing is not enough.

I was unable to execute a real Zsh runtime in my local environment, so I am not treating the candidate signal/expansion implementation as fully runtime-verified. Before merging, I would verify under Zsh that:

• two sequential .def files both process normally;
• INT cleans up and terminates with the intended status;
• TERM does the same;
• a cpplint path containing spaces remains one argv element;
• existing glob and quoted-argument behavior remains correct.

Requesting changes for the current cleanup/trap behavior and the eval argv-boundary regression.