Repository navigation
Conversation
Codex/Antigravity/Devin hand the managed hook command to CreateProcess as argv[0] rather than running it through cmd.exe, so it must stay a single spawnable token naming a real .cmd. stablyai#8430 broke that with a cmd-builtin compound and failed every hook with exit 1; stablyai#8737 restored the bare path. Nothing currently guards which wrapper each service picks: installer-utils.test.ts tests wrapWindowsCmdHookCommand directly, so a service switching to wrapWindowsHookCommand reintroduces stablyai#8430 untested. Surfaced while evaluating stablyai#8665, which proposed exactly that swap for Codex to fix stablyai#8645 -- already fixed by stablyai#8737 via the bare path. The existing codex-side check is win32-gated and pr.yml runs unit tests on ubuntu only, so it never executes in PR CI. This mocks the managed script path instead of the platform, keeping the real wrappers in play so the contract is asserted on Linux. Covers all three direct-spawn agents plus a shell-launched control (Cursor) so the split is pinned in both directions.
📝 WalkthroughWalkthroughAdds a Vitest suite for Windows agent hook launcher contracts. It mocks Electron paths, the home directory, installer utilities, and platform behavior, then verifies Codex, Devin, and Antigravity commands use directly spawnable 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a695dca-3f09-4c3a-ac90-234c6cc0e831
📒 Files selected for processing (1)
src/main/agent-hooks/windows-direct-spawn-launcher.test.ts
| it('keeps every Antigravity managed command directly spawnable', () => { | ||
| withPlatform('win32', () => new AntigravityHookService().install()) | ||
| const commands = readAntigravityCommands(homeDir) | ||
| expect(commands.length).toBeGreaterThan(0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Seed the Antigravity hooks configuration before installation.
This test creates only homeDir; AntigravityHookService.install() returns an error when .gemini/config/hooks.json is missing, so readAntigravityCommands() then throws while reading a nonexistent file. Create a minimal valid configuration and assert the install status is installed before reading the generated commands.
Based on the supplied AntigravityHookService.install() implementation, missing configuration returns an error.
|
Thanks for the contribution. The Windows direct-spawn hook launcher contract is now covered by tests already on main (windows-direct-cmd-hook-command, windows-batch-hook-launcher, windows-direct-spawn-launcher), so we are closing this test-only PR as redundant. |
Summary
Locks the invariant behind #8430/#8737: Codex, Antigravity and Devin hand the managed hook
commandto CreateProcess asargv[0]— not tocmd.exe— so it must stay a single spawnable token naming a real.cmd. Anything else is unspawnable and fails every lifecycle hook withexit 1.Test-only. No production code changes.
The gap
Nothing currently guards which wrapper each service picks:
installer-utils.test.tstestswrapWindowsCmdHookCommanddirectly, so it stays green no matter which wrapper a service calls.it.skipIf(process.platform !== 'win32'), andpr.ymlruns unit tests onubuntu-latestonly — so it never executes in PR CI.Net effect: a service switching from
wrapWindowsCmdHookCommandtowrapWindowsHookCommandreintroduces #8430 with fully green CI.This surfaced while evaluating #8665, which proposed exactly that swap for Codex to fix #8645. #8645 was already fixed by #8737 via the bare-path form (user-confirmed on Orca 1.4.141), so that swap would have been a pure regression.
Approach
Rather than skipping on non-Windows, this mocks the managed script path (and the disk write) while leaving the real wrappers in play. The platform-sensitive input is the only thing stubbed, so the win32 launcher contract is asserted deterministically on Linux CI.
Per agent, the command is taken from the narrowest available seam —
getCodexManagedHookInstallMaterial(),getDevinManagedCommand(), and Antigravity's installedhooks.json.Each direct-spawn agent asserts the command is the bare managed
.cmdpath, is a singleargv[0]token, and is not a shell launcher. A shell-launched control (Cursor) asserts the complement, so the split is pinned in both directions and "make every agent spawnable" can't pass as a valid refactor.Verification
Each assertion was proven non-vacuous by swapping wrappers locally (reverted, not committed):
wrapWindowsHookCommand(the #8665 change)wrapWindowsHookCommandwrapWindowsHookCommandwrapWindowsCmdHookCommand(reverse)mainAlso confirmed empirically on Windows via
System.Diagnostics.ProcesswithUseShellExecute=false(CreateProcess, no shell): the bare.cmdpath spawns and exits 0, while the encoded PowerShell launcher fails to spawn with "The system cannot find the file specified". Note that a Node-basedspawnSynccheck is misleading here — Node refuses.cmdwithshell:falsesince the CVE-2024-24576 guard, so it errors for both forms.pnpm typecheck— cleanoxlinton the new file — cleannpx vitest run src/main/agent-hooks/windows-direct-spawn-launcher.test.ts— 4 passedagent-hooks/, codex, antigravity, devin, cursor): 5 failures, all pre-existing — an identical control run on unmodifiedorigin/mainwith this file removed produced the same 5 (Windows-local symlink-permission and POSIX-script assertions; they pass on Linux CI).Linux execution is confirmed by this PR's own CI rather than assumed: shard
tests 4/16(ubuntu) logs✓ src/main/agent-hooks/windows-direct-spawn-launcher.test.ts (4 tests), so all four assertions really execute instead of skipping.Notes
Issue #8645 is still open even though #8737 fixed it and a user confirmed the fix on 1.4.141 — worth closing separately.