Repository navigation
fix(hooks): give the Codex team-hook dispatcher a Windows command (#1209) - #1210
Conversation
…ncent#1209) On Windows, Codex runs hook commands with PowerShell, and the built-in entries have carried their PowerShell form in commandWindows since Tencent#1074. The team-hook dispatcher entries never did: getTeamHookDispatch Command renders the POSIX wrapper on every platform, so on Windows reconcileCodexDispatchers writes an entry whose command is a quoted Git Bash path (or bare bash, which resolves to the WSL launcher) - a ParserError in Codex's PowerShell, so team hooks never run. The dispatcher entry now carries a commandWindows beside its unchanged POSIX command, following the same contract as the built-ins: the entry stays byte-identical across platforms and removable by the same command match. The Windows form is deliberately not the built-in fail-open shape (2>$null; exit 0): a dispatcher must pass what its bash form passes - the exit status (2 blocks the run) and stderr - so nothing is redirected and the exit code rides $LASTEXITCODE. The PowerShell semantics (exit-code passthrough, stderr not swallowed) could not be run on macOS, where pwsh is absent; the POSIX form is exercised end-to-end by the dispatcher e2e.
|
No findings in the specified diff. The PR description documents sufficient testing: 12/12 dispatcher E2E tests through the built CLI, including a real PowerShell execution remains unverified, but the author explicitly identifies that limitation; it is not blocking under the review rules. No PR code was run, built, or installed. |
STiFLeR7
left a comment
STiFLeR7
left a comment
There was a problem hiding this comment.
Summary. On Windows, Codex runs hook commands through PowerShell, but the team-hook dispatcher only ever wrote its POSIX bash -lc "..." form (unlike the built-in hook entries, which got a commandWindows sibling in #1074). PowerShell rejects that as a ParserError, so team hooks silently never ran on Windows. This PR adds commandWindows to the dispatcher entry, carrying teamai hook-dispatch <event> --tool codex --team-hooks; exit $LASTEXITCODE — deliberately not the built-ins' fail-open shape (2>$null; exit 0), since a dispatcher has to pass through the block exit code (2) and stderr, not swallow them.
Verified. (Windows 11, real PowerShell 7 and Windows PowerShell 5.1, not a mock)
You flagged in the PR body that the PowerShell semantics couldn't be exercised on macOS (no pwsh) and asked for a Windows run to confirm end to end — ran that directly:
- Built a
.cmdshim standing in for a real npm-installedteamaibinary (same shim shape Windows gives any globally-installed Node CLI) that writes to stderr and exits 2, then ran it through the exact dispatcher line:faketeamai hook-dispatch Stop --tool codex --team-hooks; exit $LASTEXITCODE. - Confirmed both halves of the claim for real: the stderr message passed through uninterrupted (not swallowed), and the final process exit code was
2, matching the block contract described in the PR. - Also spot-checked the unit test's claim directly against
builtin-hooks.ts/codex-team-hooks.ts:commandWindowsonly gets attached to the dispatcher entry (getTeamHookDispatchCommandWindows), thecommandfield stays the POSIX form on every platform per #1074's contract, andisCodexTeamHookDispatcher's removal match still readscommandonly — so the entry stays recognizable and removable exactly as before.
Findings. None — I went in looking for a defect and didn't find one. The design choice (not reusing the built-ins' fail-open shape) is correct and well-reasoned: 2>$null; exit 0 would genuinely discard the block message and the exit code, which is precisely the bug this PR is fixing.
Not covered / unverified. I didn't run this through a real Codex install end-to-end (only the PowerShell snippet in isolation plus the source-level checks above); didn't test on PowerShell Core on Linux/macOS since the dispatcher's Windows path only matters on Windows anyway.
Verdict. Comment (resolving your explicit "please confirm on Windows" ask, not a formal approval). Confirmed: the stderr-and-exit-code contract works exactly as described on real Windows PowerShell, both 5.1 and 7.
|
Flagging for the maintainers before this goes further: @natinli's field-level finding on #1209 — Codex's hook config struct ( Two things this PR can still claim, and one it can't:
If the durable fix is a platform-neutral bare |
|
@kittimzhe Checked Codex's source: it does read |
A correction to my comment above. Codex does read
Suggested change: drop the tail, update the comments, and replace the PowerShell record with one from Codex on Windows, including the Codex version. The command has no quotes, so a This PR should own the dispatcher's |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Blocking until the changes in #1210 (comment) land. Codex runs Windows hooks under cmd.exe, so the commandWindows tail and its PowerShell record need reworking.
) Codex runs hook commands through %COMSPEC% /C on Windows — cmd.exe, not PowerShell (Tencent#1215) — so the '; exit $LASTEXITCODE' tail was never a command: it reached hook-dispatch as junk arguments (accepted and ignored, so the entry worked by accident). cmd /C already exits with the program's exit code and passes stderr through, so the Windows form is just the bare dispatch, without the POSIX form's bash wrapper that cmd cannot resolve. Comments and tests updated to the real shell; verification record to be re-run under cmd /C with the Codex version noted.
Per the split agreed in review: the dispatcher entry belongs to Tencent#1210, so this PR no longer rewrites it — reconcileCodexDispatchers, getTeamHookDispatchCommand and DISPATCHER_COMMAND are back to main's state. What remains is the built-in half: the bare portable command plus the cmd.exe commandWindows (6124e9c).
|
Agreed on the split — this PR owns the dispatcher's Windows side, and #1231 is now built-ins only (dispatcher hunks stripped there). Reworked per your reading (34f2559): the |
|
No findings in the specified diff. The PR description documents sufficient testing: 12/12 dispatcher E2E tests through the built CLI, including a real The earlier pass raised no findings, so none require resolution. No PR code was run, built, or installed. |
|
Windows record for the reworked dispatcher entry ( Environment
The string under test
No quotes, no tail, no redirect — so a plain ResultsRun from a real
Exit-code passthroughThe property the bare form depends on, verified separately under cmd: So dropping the tail is safe in the sense that matters: Caveats on what this covers
One suggestion for the PR textThe doc comment could state why no tail is needed, since the previous form's tail looked deliberate: |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
The rework covers the changes requested in #1210 (comment):
- The tail is gone.
commandWindowsis nowteamai hook-dispatch <event> --tool codex --team-hooks, andcmd /Cruns it with the exit code and stderr intact. - The comments and test names now describe cmd.exe instead of PowerShell.
- @natinli's record (#1210 (comment)) runs that exact string under
cmd /Con Windows 11, with Codex 0.142.5 on PATH, and showscmd /Creturning a child's exit 2.
Checked locally at 34f2559 on macOS: npx tsc --noEmit is clean and codex-team-hooks.test.ts passes 7/7. After npm run build, codex-team-hook-dispatcher-915.test.ts passes 12/12. The branch still merges cleanly with current main. The trust pass needs no change for the new field, because teamaiCodexHooks already reads commandWindows from the file. Codex on Windows therefore lists the dispatcher under an id teamai asks it to trust.
Findings
- [P2 non-blocking] The PR body still describes the first commit. The Change section shows the
; exit $LASTEXITCODEform and says Codex runs hooks with PowerShell, and Verification says PowerShell could not be run. A squash merge turns that text into the commit message. Please change it to the bare cmd form and paste or link natinli's Windows record in Verification. - [P3 nit]
src/codex-team-hooks.ts:209-212says the entry "keeps the POSIX form on every platform, like the built-in entries (#1074)". The dispatcher'scommandcomes fromgetHookShellCommand(), which returns the machine's Git Bash path on Windows, while the built-ins use barebash. Both are POSIX forms, but the bytes differ. This is harmless here because~/.codex/hooks.jsonbelongs to one user and is never committed, but "like the built-in entries" is inaccurate. The body's "stays byte-identical across platforms" makes the same claim.
Not covered
Nobody has run this through Codex itself on Windows, and no real team hook has returned exit 2 through the npm teamai.cmd shim under cmd /C. natinli's record checks the two parts separately: the command shape, and cmd's exit-code passthrough. That is enough to merge; a Windows user report can confirm the full path.
Verdict: Approve. This replaces my earlier request for changes.
Tencent#1210 pinned the POSIX `bash -lc` rendering in codex-team-hooks.test.ts, and Tencent#1221 put that file on the Windows test list. On Windows the command renders the Git Bash path, and the Windows list guard rejects a pinned POSIX rendering in a file that does not pin the platform. Every Lint & Test leg and the Windows job on main fail.
…15% (#1283) * fix(test): pin the platform in the Codex dispatcher render test #1210 pinned the POSIX `bash -lc` rendering in codex-team-hooks.test.ts, and #1221 put that file on the Windows test list. On Windows the command renders the Git Bash path, and the Windows list guard rejects a pinned POSIX rendering in a file that does not pin the platform. Every Lint & Test leg and the Windows job on main fail. * perf(ci): shard the internal E2E run The full-surface E2E job ran every E2E file on one runner under the fixture-repo lock, so it set the pace of every main push and internal PR. Only e2e.test.ts uses the fixture repo: run that file alone under the lock, and run the rest in the three credential-free shards on every event. * perf(test): shorten the exclude lock retry and driver poll in tests Unit tests that hold the exclude lock wait out updateFileLocked's whole retry budget, and the workflow driver tests wait out its 2 s poll. Production keeps both values; the test files that pay for them shorten them through _setExcludeLockRetryMs and _setDriverPollMs. * perf(git-exclude): skip git for a directory that does not exist findMcpGitExcludes asks where each agent's config directory is excluded, including directories not created yet (.codex, .kiro/settings, .trae). For each one locateExclude started git in a missing cwd, which always failed, and the null answer is not cached, so every call paid for it again. The answer is the same null; it now comes without the process. mcp-reconcile.test.ts and uninstall.test.ts start 35% fewer git processes (10918 to 7115) and run 13% and 9% faster locally. * perf(ci): collect coverage only on the leg that uploads it Five of the six unit legs ran with --coverage and discarded the report: only Ubuntu on Node 20 uploads it, and no threshold reads it. In a paired run on one macOS runner, coverage cost about 67 s of a 550-810 s leg.
What
On Windows, Codex runs hook commands with PowerShell. Built-in entries have carried their PowerShell form in
commandWindowssince #1074; the team-hook dispatcher entries never did —getTeamHookDispatchCommandrenders the POSIX wrapper on every platform, so on WindowsreconcileCodexDispatcherswrites an entry whosecommandis a quoted Git Bash path (or barebash, which resolves to the WSL launcher). Either is aParserErrorin Codex's PowerShell, so the team hooks never run — the remaining half of #1075/#1127 (#1209).Change
The dispatcher entry now carries a
commandWindowsbeside its unchanged POSIXcommand, following the built-ins' contract (#1074): the entry stays byte-identical across platforms, and stays removable by the same command match (isCodexTeamHookDispatcherreadscommandonly).The Windows form is deliberately not the built-in fail-open shape (
2>$null; exit 0):A dispatcher must pass what its bash form passes — the exit status (2 blocks the run) and stderr — so nothing is redirected and the exit code rides
$LASTEXITCODE. (The sketch on the issue redirects stderr with2>$null; that would discard the block message.)Verification
codex-team-hooks.test.ts): reconcile writes the dispatcher entry with the POSIXcommandand the block-preservingcommandWindows; a second run writes the same bytes.codex-team-hook-dispatcher-915.test.ts), 12/12 through the built CLI: after a realpull, every dispatcher entry in~/.codex/hooks.jsoncarries the expectedcommandWindows, and the POSIX form still dispatches with exit code and stderr.hooks,pull-codex-trust,muse), tsc and lint clean.pwshis absent — @natinli a run on your Windows machine would confirm the fix end to end.Fixes #1209