fix: resolve Windows native hook session owner PID#3260
Conversation
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Independent maintainer review — PR #3260 @ 8031bec81105d0e0b65668602afb136a74f5fe9c
Read-only adversarial review; branch not mutated.
Bug claim — verified
On win32, readParentPid/readProcessCommand fell through to the Unix ps path, which fails, so resolveSessionOwnerPidFromAncestry collapsed to the transient PowerShell shim PID. classifySessionProcess (src/hooks/session.ts:651) then probes that dead PID after shim exit and returns stale-dead, breaking Stop authorization. The mechanism in the PR description is accurate, not speculative.
Fix design — verified
- One bounded CIM process-table read (
powershell.exe -NoProfile -NonInteractive, 5s timeout, 4MB buffer,windowsHide), parent-chain walk capped at 6 hops, matching the Unix loop bound. - Image-name-first Codex detection (
codex.exe) with shell-name exclusion (powershell/pwsh/cmdadded tolooksLikeShellCommand) correctly rejects the misleading-argument case (pwsh -Command "... codex.exe ..."). IfCommandLineis access-denied,Namestill satisfieslooksLikeCodexProcess— graceful. - Unix ancestry path preserved: injected-dependency branch untouched, and the tightened
looksLikeCodexCommandregex still matches canonical Unix shapes (node /usr/local/bin/codex.js) while excludingcodex-native-hook.jswrappers and.codexhook paths — confirmed by the pre-existing ancestry regression test. - Cost is one CIM call per SessionStart only:
resolveSessionOwnerPidis called solely from the SessionStart branch (codex-native-hook.ts:20230), not per hook event. - Cross-platform regression risk is low: win32 early-returns in
readParentPid/readProcessCommandaffect only the ancestry resolver (sole caller), and failure modes degrade to pre-fix behavior, never worse.
Interaction with merged #3257 fix (#3259, dev head 8f76c22b) — none
#3259 touches src/cli/update*, package-manager-ownership, postinstall. Zero shared files with this PR; merge-base 299f31ad; GitHub reports MERGEABLE.
Verification performed locally at exact head (Linux)
npm run build(tsc): clean.dist/scripts/__tests__/codex-native-hook.test.js: 583/583 pass, including the new Windows lineage test and the Unix wrapper test.dist/hooks/__tests__/session.test.js: 73/73 pass (stale-dead / owner-evidence contract consumer).biome linton both changed files: clean.
Hosted CI status
The CI workflow run for this head is action_required (first-time-contributor fork gate): hosted build/test lanes have not executed. Local runs above are the current evidence; approve the pending workflow run before merge per repo policy.
Blockers
None found.
Noise / non-blocking observations
- On CIM failure or timeout,
resolveSessionOwnerPidsilently falls back toprocess.pid(transient hook process), recreating the originalstale-deadsymptom with no diagnostic. A bounded warning would aid future triage. readWindowsProcessLineage's own JSON parsing (single-record object case, malformed records) is only exercised through a mocked lineage, not directly.- Windows owner identity remains liveness-probe-only (no start-tick/cmdline identity as on Linux); PID reuse by an unrelated process keeps a pointer
usable. Pre-existing platform contract, explicitly out of scope here. - Contributor's
npm testgap on Windows (ask.testexternal-provider stall) is honestly disclosed in the PR body and unrelated to this change.
Verdict: MERGE_READY
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
CI record — exact reviewed head green, rebase required before mergeFollow-up to review 4757337998 (MERGE_READY). The approved hosted CI run has now reached terminal state on the exact reviewed head. Exact-head CI evidence
Merge preconditionThe PR head is one commit behind current Status: exact head — |
GJC maintainer rebase evidence — LOCAL CANDIDATE — PENDING PUSHRebase of PR #3260 prepared locally and verified; the contributor branch and this PR are still at the old head. Nothing has been pushed.
Equivalence evidence
Frozen local checks at exact new head (after locked
|
8031bec to
9142f91
Compare
|
[GJC maintainer rebase push confirmation | session 019f8b39-26e6-7000-8264-82c84947cb27 | actor @Yeachan-Heo] Pushed head confirmed at CI and a fresh exact-head adversarial review are pending. Merge remains held for BOTH a fresh exact-head MERGE_READY review on |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Final verdict — rebased head 9142f91f977bd937df94a4c622ed43c190b3219e
SHA-bound follow-up to review 4757337998 and green record comment 5050171032. Read-only lane; contributor branch not mutated.
Rebase equivalence — verified byte-identical
- Base after rebase:
8f76c22b01fb0314e801ea99c5c87e6347e76166(current dev at time of rebase), confirmed via merge-base. git range-diff 299f31ad..8031bec8 8f76c22b..9142f91f→1: 8031bec8 = 1: 9142f91f: exact patch equality.git patch-id --stableof old vs. rebased diff: identical.- Rebased diff scope unchanged: exactly
src/scripts/codex-native-hook.tsandsrc/scripts/__tests__/codex-native-hook.test.ts, +125/−11. - Tree delta old head → rebased head is exactly the #3259 file set (dev drift); those modules neither overlap with nor are imported by the changed hook/session modules.
- All adversarial findings from the original exact-head review therefore carry over unchanged: bug claim accurate, fix sound, Unix paths preserved, one bounded CIM read per SessionStart only, failure modes degrade to pre-fix behavior at worst, no blockers.
Hosted CI on exact rebased head — terminal green
- Run 29957125573 (
pull_request), head_sha9142f91f977bd937df94a4c622ed43c190b3219e— status completed, conclusion success. - 12 lanes success: Detect CI lanes, Lint, Build dist artifact, Typecheck, Ralplan preflight security (macOS), Coverage Gate (Team Critical), Ralph Persistence Gate, Test (Node 22 / smoke), Test (Node 20 / hooks-notify-platform), Test (Node 20 / team-state-runtime), Test (Node 20 / cli-core-rest), CI Status.
- 5 lanes legitimately skipped by lane classification (Rust x3, Docs, Full Source Build) — no Rust/docs changes in this diff.
- Combined with prior local verification of the byte-identical patch (tsc clean; 583/583 native-hook tests; 73/73 session contract tests; biome clean), the rebased head is fully verified.
Blockers
None. Non-blocking noise unchanged from the original review (silent transient-PID fallback on CIM failure, mocked-only lineage parsing test, pre-existing win32 probe-only identity).
Verdict: MERGE_READY
The PR is still draft; it may now be marked ready for review and merged to dev. This lane does not mutate or merge.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
![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: 9142f91f97
ℹ️ 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".
|
|
||
| function looksLikeShellCommand(command: string): boolean { | ||
| return /(^|[\/\s])(bash|zsh|sh|dash|fish|ksh)(\s|$)/i.test(command); | ||
| return /(^|[\\/\s])(bash|zsh|sh|dash|fish|ksh|powershell(?:\.exe)?|pwsh(?:\.exe)?|cmd(?:\.exe)?)(\s|$)/i.test(command); |
There was a problem hiding this comment.
On Windows the managed hook can launch PowerShell by a quoted absolute path, so Win32_Process.CommandLine may start like "C:\\...\\powershell.exe" -File ...; this pattern only treats powershell.exe/cmd.exe as a shell when whitespace or end-of-string follows the executable name. In fallback cases where no exact codex.exe/codex.js ancestor is matched, looksLikeShellCommand(lineage[0].command) then returns false and the resolver records the transient shim PID instead of lineage[1], recreating the stale-dead Stop owner on those installs.
Useful? React with 👍 / 👎.
2 participants
Summary
On Windows, the native hook ancestry resolver called the Unix
pscommand. When that lookup failed, SessionStart could persist the transient PowerShell hook PID instead of the long-lived Codex PID. Once the shim exited, Stop authorization saw the selected session pointer asstale-dead.The previous command-only Codex detection could also mistake shell arguments or
.codexhook paths for the Codex process.Changes
codex.exefrom PowerShell/cmd arguments that merely mention Codex.codex.exeancestor.Validation
npm run buildnpx biome lint src/scripts/codex-native-hook.ts src/scripts/__tests__/codex-native-hook.test.tscodex.exePIDomx doctor: 15 passed, 4 environment warnings, 0 failednpm test: did not complete on Windows because the existingask.testfake-provider flow invoked an external Claude process and stalled; the run was stopped after isolating that unrelated test path.Checklist
Related
N/A