Repository navigation
Conversation
Fixes #11496. The fast-path gate from #7131 whitelists [A-Za-z0-9_.:/~-], so any home directory with non-ASCII characters (CJK, Cyrillic, accented profiles) - and any path with spaces - falls back to the encoded PowerShell launcher, tripling per-hook overhead (measured 66ms vs 207ms per wrapper call; PreToolUse and PostToolUse both fire on every tool call). Since #8430 the fast path wraps the script path in single quotes, which keeps spaces, metacharacters, and non-ASCII literal. The gate now only needs to exclude control characters. Leave WINDOWS_CMD_SAFE_PATH untouched: Codex/Antigravity/Devin spawn the hook as argv[0], so that whitelist is load-bearing (#8430/#8737).
📝 WalkthroughWalkthroughGit Bash hook path handling now uses a Unicode-aware safety check that permits whitespace and excludes control characters and parser-sensitive metacharacters. Tests cover quoted paths, Unicode segments, embedded single quotes, shell execution, and stdin draining. Unsafe paths still use the encoded launcher. Managed-hook tests assert the exact quoted command and POSIX stdin-drain fallback. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryThis PR widens the
Confidence Score: 5/5Safe to merge; the change is a minimal regex relaxation in the Windows Git Bash branch only, and the quoting layer that neutralizes all shell-special characters was already in place. The widened regex is applied to bashPath before it is passed to quotePosixShellString, which wraps the path in POSIX single quotes and escapes embedded single quotes with the standard POSIX trick. Single-quoted strings in bash suppress all shell expansion, so spaces, &, $, CJK characters, and other non-control characters are all handled safely. Control characters (Unicode Cc: U+0000-U+001F, U+007F, U+0080-U+009F) are still rejected and fall back to the PowerShell encoded launcher, covered by a test. WINDOWS_CMD_SAFE_PATH for Codex/Devin argv[0] spawn is untouched. Test coverage across unit and end-to-end layers is thorough. Files Needing Attention: No files require special attention; the only changed production code is the single-line regex in installer-utils.ts.
|
| Filename | Overview |
|---|---|
| src/main/agent-hooks/installer-utils.ts | Core change: WINDOWS_GIT_BASH_SAFE_PATH widened to /^\P{Cc}+$/u; wrapWindowsGitBashHookCommand logic and quotePosixShellString are unchanged and correct. |
| src/main/agent-hooks/installer-utils.test.ts | Tests updated to cover fast-path for spaces, metacharacters, non-ASCII, single-quote escaping, and control-character fallback; coverage is comprehensive. |
| src/main/claude/hook-service.test.ts | End-to-end #6078 test correctly updated to assert the quoted fast-path command; unused WINDOWS_POWERSHELL_LAUNCHER matcher removed. |
Reviews (1): Last reviewed commit: "fix(claude): widen Git Bash hook fast pa..." | Re-trigger Greptile
The quoting contract this fix relies on had only string-equality coverage; every executable Git Bash assertion is win32-only and PR CI runs tests on Linux, so a dropped quote would ship green. Run the generated command through /bin/sh for spaced, metacharacter, non-ASCII, and single-quote homes, plus the missing-script drain. Also drop the stale claim that the Claude launcher stays PowerShell-encoded.
Bash's single quotes are not the last parser. Executing a .cmd makes Git Bash re-launch it through cmd.exe (COMSPEC), which parses the path again with the quoting already consumed. Measured on Git Bash 2.55 / Windows: a home containing & ^ ( ) ; , = or %VAR% exits 1 on every hook call, where the encoded launcher runs it correctly. ! is safe today but expands under a registry-enabled DelayedExpansion, so it is gated too. Spaces, quotes, and non-ASCII (CJK, Cyrillic, accented) profiles are unaffected and keep the fast path — verified on a real ACP=1252 host, which also closes the PR's open question about non-UTF-8 code pages.
The premise the fast path rests on - that Claude's Git Bash runner can execute a forward-slash .cmd directly - was dropped when the gate comment was rewritten, leaving no way to reconstruct why the exclusion set is what it is. Restore it and state the bash -> cmd.exe hand-off explicitly. Also stop calling '!' a cmd.exe metacharacter (it is inert without DelayedExpansion), stop implying the POSIX execution block covers Windows, and assert tmpdir passes the gate so a gated tmpdir cannot masquerade as a quoting failure.
|
Thanks for this — the diagnosis is right, and the measurement in #11496 is genuinely useful. I've pushed three commits to your branch so this is landable; details below so nothing is a surprise. The one behavior change I had to make. I tested the widened gate on a real Windows host (Git Bash 2.55, ANSI code page 1252) and hit a case the quoting argument doesn't cover: bash's single quotes aren't the last parser. Running a
So the gate is now Your actual fix is intact. CJK, Cyrillic, and accented profiles still take the fast path — confirmed on that same code-page-1252 machine, which also answers the CP949/CP936 question you flagged in your notes. Good instinct flagging it; it turned out fine. Tests. I added cases that execute the generated command through a real shell rather than asserting the string. Every existing Git Bash assertion is Two things I deliberately left out of this PR so it stays scoped to one change:
One gap worth stating plainly: there's no Windows vitest job in PR CI, so the cmd.exe half of this is pinned by a hand-verified denylist rather than an automated gate. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/agent-hooks/installer-utils.test.ts (1)
583-587: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the explanatory test comments.
Keep only the parser-chain or test-invariant detail needed to explain each case. Remove measured behavior and repeated rationale from the comments.
As per coding guidelines, comments must be concise, limited to non-obvious information, and preferably one line.
Also applies to: 622-626, 635-636, 644-646
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e5653c4-d641-47b3-af90-cb6da16626d8
📒 Files selected for processing (3)
src/main/agent-hooks/installer-utils.test.tssrc/main/agent-hooks/installer-utils.tssrc/main/claude/hook-service.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/agent-hooks/installer-utils.ts
- src/main/claude/hook-service.test.ts
…-path-non-ascii # Conflicts: # src/main/agent-hooks/installer-utils.test.ts # src/main/agent-hooks/installer-utils.ts # src/main/claude/hook-service.test.ts
Windows low-spec QA — PR #11497 (Grok)Commit validated: Tree vs main parent
Superseding runtime-home hook (on main / this tree)
Focused tests (Windows)Unrelated env failures when running full installer-utils without filter: 3 symlink EPERM (no Developer Mode), 3 default 5s timeouts that pass at 30s. Real Git Bash path checkExecuted generated command under
Electron identity (CDP 9341, isolated userData){"name":"Orca: 58db93e90b","isDev":true,"devLabel":"pr-11497-grok-windows-qa @ 58db93e90b","devBranch":"58db93e90b","devWorktreeName":"pr-11497-grok-windows-qa","devRepoRoot":"C:\\Users\\neil\\orca\\workspaces\\orca\\pr-11497-grok-windows-qa"}Navigated Settings → Agents; Agent status hooks toggle visible and ON. Console: 0 errors (dev CSP/tooltip warnings only). Screenshots (not committed)Settings → Agents with Agent status hooks:
Gist: https://gist.github.com/OrcaWin/cda9707c6beaa9d45645e2b7e0a17b37 RecommendationClose as superseded / do not land — empty diff vs main; original fix is preserved by the runtime-home hook path already on main. No code changes made during this validation. Note: dev Electron on this host hits Superseding fix: #13442 — fix(claude): make managed hook paths portable |
|
Thanks for the thoughtful report, performance measurements, and implementation — the diagnosis here was correct and genuinely useful. A broader runtime-home solution landed independently in #13442 and now covers this non-ASCII Windows-home case while preserving the safe fast path. After updating this branch from |
Fixes #11496.
Summary
On Windows, a home directory containing non-ASCII characters (CJK, Cyrillic, accented
profiles) fails the
WINDOWS_GIT_BASH_SAFE_PATHwhitelist, so every managed Claude hookfalls back to the PowerShell
-EncodedCommandlauncher — roughly 640 ms vs 190 ms ofhook overhead per tool call (PreToolUse + PostToolUse both fire; measured 66.2 ms vs
207.4 ms per wrapper call, n=10, details in #11496).
Since #8430 the fast path wraps the script path in
quotePosixShellString(), so thewhitelist's original concern (bash splitting on spaces) is already handled by the
quoting. This PR relaxes the gate to reject only control characters:
so non-ASCII profiles — and paths with spaces or shell metacharacters — take the same
fast path as ASCII ones. User-visible effect: lower per-tool-call hook latency for
affected users; the generated command in
~/.claude/settings.jsonbecomes the quotedif [ -f '…' ]form instead of the encoded PowerShell launcher.WINDOWS_CMD_SAFE_PATHis deliberately untouched: Codex/Antigravity/Devin spawn thehook command as
argv[0]via CreateProcess rather than through a shell, so thatwhitelist is load-bearing (#8430 / #8737). Only the Git Bash branch goes through a shell,
where the quoting provides the safety.
Screenshots
No visual change — this only affects the generated hook command string in
~/.claude/settings.json.Testing
pnpm lintpnpm typecheckpnpm testpnpm buildTest changes:
installer-utils.test.ts(wrapWindowsGitBashHookCommand): kept the ASCIIexact-string case;
Jane Doe(spaces) andalice & bob(metacharacters) now assertthe exact quoted fast-path command instead of the PowerShell fallback; added
exact-string cases for a non-ASCII home directory and a single-quote path (
O'Brien);added a control-character case asserting the encoded-launcher fallback still applies.
claude/hook-service.test.ts: the end-to-end [Bug]: Codex hooks fail on Windows when user path contains spaces #6078 spaces test now asserts the exactquoted fast-path command; removed the now-unused
WINDOWS_POWERSHELL_LAUNCHERmatcher.pnpm testnote (Windows 11 26200): every test file importinginstaller-utilsorclaude/hook-settingspasses (10 files, 144 tests). The full suite reports 179environment-dependent failures in 54 files on my machine (Windows symlink privileges,
pty/daemon spawn); the only failing file intersecting this change's import graph is
installer-utils.test.ts, whose 3writeHooksJsonsymlink tests fail identically on apristine checkout (symlink creation needs elevation/Developer Mode on Windows).
AI Review Report
Reviewed with Claude Code. What it checked:
(
getManagedCommandinsrc/main/claude/hook-settings.ts); macOS/Linux usewrapPosixHookCommand, untouched. No shortcuts, labels, or renderer behavior involved.remote-hook-service-installers.test.ts, which exercises the shared wrapper path,passes.
Codex/Antigravity/Devin (
wrapWindowsCmdHookCommand) and the POSIX wrapper areuntouched.
changes behavior.
What it flagged, and what changed as a result: an earlier draft used
/^[^\x00-\x1F\x7F]+$/, which trips oxlint'sno-control-regex; switched to theequivalent Unicode-property form
/^\P{Cc}+$/u(which also covers C1 controls). It alsocaught that the end-to-end #6078 test still asserted the PowerShell fallback for spaced
paths and had to be updated alongside the unit tests.
Security Audit
quotePosixShellString(single quotes; embedded'escaped as'\''). Single quotessuppress all POSIX shell expansion, so spaces, metacharacters, and non-ASCII stay
literal.
PowerShell launcher (covered by a test).
claude-hook.cmd, same drain-on-missingbehavior; only the wrapper form changes for paths the whitelist previously excluded.
Notes
CP949/CP936 defaults; the commit that introduced the gate (801effa) and its tests
indicate spaces/metacharacters were the concern rather than encoding, but flagging for
reviewer judgment.