Repository navigation
fix(lefthook): keep the check-affected pre-push job parseable on Windows - #11360
Conversation
On Windows, lefthook passes a multi-line run body to sh -c wrapped in double quotes without escaping the ones inside it, so sh got a cut-off script, exited 2, and blocked every push. The body now uses no double quotes; the merge-base SHA and mktemp path hold no spaces. Closes #11356 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJUn7KoThXoYQoxfvdCVi7
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7882ed2005
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| git archive $mb -- .claude/hooks/check-affected-pre-push.sh lib/hook-utils.sh 2>/dev/null | tar -x -C $tmp 2>/dev/null | ||
| [ -f $tmp/.claude/hooks/check-affected-pre-push.sh ] || exit 0 |
There was a problem hiding this comment.
When TMPDIR contains spaces, these unquoted $tmp expansions silently skip the pre-push checks. mktemp --help explicitly says it uses $TMPDIR by default; it does not guarantee a space-free path. In the check-affected job, word splitting makes tar -C receive only part of the path, and the subsequent file test fails and exits 0. Reproducing with a space-containing TMPDIR and an archived check that exits 1 blocks correctly before this commit but passes without executing the check afterward. Preserve the temporary path as a single argument while addressing the Windows quoting issue.
Useful? React with 👍 / 👎.
|
claude-security-review reviewed No security findings. The only changed file is I checked it against what the job gates. It runs check code taken from the merge-base (
critical-findings: 0 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if ! mb=$(git merge-base HEAD refs/remotes/origin/main 2>/dev/null); then | ||
| echo 'check-affected pre-push: skipped, no origin/main merge-base to take the check from; CI checks it.' >&2 | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
The new comment says "mktemp paths hold no spaces", but mktemp -d builds its path from $TMPDIR, which the user controls. If TMPDIR contains a space, say /Users/a b/tmp, then rm -rf $tmp splits into two arguments and deletes /Users/a plus a second path. The unquoted $tmp on lines 26-28 breaks the same way, but those only fail the job. This rm -rf deletes the wrong directory.
The risk is small, but the guard needs no double quotes. After line 24, add case $tmp in *' '*) rm -rf -- "..." ... is not possible without quotes, so use case $tmp in *[[:space:]]*) exit 0;; esac before the trap. That skips the job instead of deleting the wrong path, which fits the job's existing 'anything else passes' rule. The comment should also say that TMPDIR is assumed to have no spaces.
claude-lane: claude-review
| @@ -15,17 +15,19 @@ pre-push: | |||
| # commit too. | |||
There was a problem hiding this comment.
Issue #11356 asks to "Keep the run: body free of double quotes", and nothing in the repository fails if one comes back. .claude/hooks/check-affected-pre-push.test.sh covers only the hook script, not this YAML body. A later edit that re-quotes "$mb" or "$tmp" would pass CI, pass a Linux or macOS push, and re-break every Windows push. The only guard is the comment.
Add a small check that parses lefthook.yml and asserts that no run: value of a job containing a newline contains ". This could be a case in an existing lefthook or hook test, such as lib/git-hooks/install.test.mjs.
claude-lane: claude-review
|
claude-review reviewed The fix is correct for the reported failure. The Two findings, both advisory:
Closing issue #11356 was read. Its other criterion, to keep taking check code from the merge-base, is met, and No security, cost or manifest items. The change touches no |
|
PR body contract — issue linkage This PR body does not yet satisfy the issue-linkage contract:
Edit the body and this comment updates itself on the next run. |
github-actions
Bot
added
the
needs-issue-linkage
Summary
Fixes #11356. On Windows, the
check-affectedpre-push job exited 2 before running any check and blocked every push from this repo.Cause
Lefthook 2.2.2 on Windows runs a multi-line
run:body assh.exe -c "<body>"and does not escape the double quotes inside the body. Windows argument parsing ends the-cstring at the first inner quote, soshreported-c: line 2: unexpected EOF while looking for matching ')'.Fix
The job body now contains no double quotes: single quotes for the echo text and the
trap, and unquoted$mb(a commit SHA) and$tmp(amktemp -dpath). Neither can contain spaces. A comment on the job records why. The merge-base rule is unchanged: check code still comes from the merge-base, never from the pushed branch.Verification
lefthook run pre-push --job check-affected --forcepassed (exit 0). Before the fix it failed with the parse error.check-affectedpassed in 26 s.🤖 Generated with Claude Code
https://claude.ai/code/session_01UJUn7KoThXoYQoxfvdCVi7