Repository navigation
fix(hooks): exit Windows Devin/Grok skip without draining stdin - #24909
inforanger wants to merge 1 commit into
Conversation
The Windows managed hook routes the Devin-imports-Claude and Grok-replay skips to the `orca_agent_hook_drain_stdin` epilogue, which parks in `more.com` until stdin reaches EOF. When the caller abandons the pipe instead of closing it — the same stablyai#11549 class this file already guards against for missing Orca env — the drain child never returns: the hook is stranded until the caller's timeout, and because `more.com` inherits the hook's stdout handle it keeps that pipe open after a timeout-kill, so the agent's read never sees EOF. Exit the skip paths without owning stdin instead. The payload is discarded on those paths anyway, matching the rule the env guards already follow (buildWindowsHookEnvironmentGuardLines), and removing the last in-script jumps to `more.com`. - hook-script.ts: DEVIN skip `goto :drain` -> `exit /b 0` - grok-replay-guard.ts: Windows Grok skip `goto :drain` -> `exit /b 0` - update the lifecycle + grok tests to pin exit-without-drain The shared drain epilogue itself is left intact (the missing-managed-script launcher case and the lifecycle contract still reference it); a bounded Windows drain, mirroring the POSIX deadline reader, is a larger follow-up. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — the skip change is correct; two minor notes.
Reviewed changes
- Devin-import skip exits instead of draining — the Windows Claude impl now uses
exit /b 0, so a Devin session no longer parks inmore.comwhen the caller abandons stdin. - Grok-replay skip exits instead of draining — the shared Windows guard builder now exits, which affects both the Claude and Cursor scripts that consume it.
- Tests pin the new behavior — the Devin lifecycle assertion and the Windows Grok guard constant now require exit-without-drain and would fail on a revert (not theatre).
ℹ️ statusline-script.ts still routes an unbounded more.com drain
The PR body says rotating the skips off the drain removes "the last in-script jumps to more.com", but statusline-script.ts:28 still does if "%ORCA_PANE_KEY%"=="" goto :orca_agent_hook_drain_stdin. That is the same unbounded read-to-EOF shape this PR removes from the skips; if the statusline caller can abandon stdin, it carries the identical #11549 hang. It is pre-existing and outside this PR's stated scope, but worth confirming the statusline path is safe before treating the hang class as fully closed.
Technical details
# Remaining unbounded drain in statusline-script.ts
## Affected sites
- `src/main/claude/statusline-script.ts:28` — `if "%ORCA_PANE_KEY%"=="" goto :orca_agent_hook_drain_stdin`
jumps to `buildWindowsHookStdinDrainEpilogue()`, whose `more.com` reads stdin to EOF.
- The `CLAUDE_JOB_DIR` guard directly above it (`:26`) was already changed to `exit /b 0` for the
same abandoned-stdin reason, so the two guards in one script now disagree on the contract.
## Required outcome
- Decide whether the pane-key-missing statusline path can have its stdin abandoned. If it can, it
needs the same exit-without-owning-stdin treatment — the payload is likewise discarded there.
## Open questions for the human
- Does Claude Code's statusLine command close stdin after writing, or can it abandon the pipe?ℹ️ Nitpicks
src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts:303-304— the comment still says "claude-hook-impl.cmdstill jumps to it from the Devin-imports-.claude skip", which is exactly the jump this PR removed. The assertion is fine; only its rationale is now stale.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)📝 WalkthroughWalkthroughThe Windows Grok replay guard and Claude Devin-project skip path now exit with status 0 instead of routing to the stdin-drain label. The changes remove the unused label imports and update tests for the exit behavior and guard ordering. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to The Windows skip paths now avoid waiting for stdin, but their tests do not verify that behavior with an open pipe. The change appears mergeable; adding that focused coverage would help prevent the hook hang from returning. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The changed skip paths return without starting a stdin-draining child process. Their selection conditions, successful response, and access controls remain unchanged. No material security risk was identified in the changed behavior. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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.
🧹 Nitpick comments (1)
src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts (1)
314-325: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd abandoned-stdin coverage for both Windows skip branches.
The Windows live test invokes every local script with closed stdin. The Claude and Grok tests only inspect generated text. Add live cases for
DEVIN_PROJECT_DIRandGROK_HOOK_EVENTthat bypass the earlier Orca guards, usestdin: 'abandon', and assert exit code0.The existing text checks catch a literal
goto :orca_agent_hook_drain_stdinrollback, but they do not validate EOF-independent runtime behavior.Suggested fix
for (const fileName of mainScripts) { const scriptPath = join(hooksDir, fileName) const executable = fileName.endsWith('.cmd') @@ expect(WRITER_BROKEN_BY_EARLY_EXIT, `${fileName} stdin error`).toContain(error.code) } } + for (const [fileName, extraEnv] of [ + ['claude-hook.cmd', { DEVIN_PROJECT_DIR: 'C:\\devin' }], + ['grok-hook.cmd', { GROK_HOOK_EVENT: 'Stop' }] + ] as const) { + const abandoned = await runHookProcess( + 'cmd.exe', + ['/d', '/c', join(hooksDir, fileName)], + hookEnvironment({ + ORCA_AGENT_HOOK_PORT: '59999', + ORCA_AGENT_HOOK_TOKEN: 'token', + ORCA_PANE_KEY: 'tab:leaf', + ...extraEnv + }), + 'abandon' + ) + expect(abandoned.exitCode, `${fileName} abandoned-stdin exit code`).toBe(0) + } + const missingScript = 'C:\\missing\\orca-hook.cmd'
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d3f18da0-23c3-4a6b-b1c5-6d2ac155b5ec
📒 Files selected for processing (4)
src/main/agent-hooks/grok-replay-guard.test.tssrc/main/agent-hooks/grok-replay-guard.tssrc/main/agent-hooks/managed-hook-stdin-lifecycle.test.tssrc/main/claude/hook-script.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
…din (#33470) Windows managed hook skip paths (Devin/Grok/Cursor replay guards, and orca-agent-hook.exe) exit immediately instead of draining stdin through more.com, so a held-open writer can no longer hang the hook. Fixes #24907 Supersedes #24909 Co-authored-by: inforanger <29155427+inforanger@users.noreply.github.com>
|
Thanks @inforanger — your commit is carried in #33470 (merged, with your co-author credit), rebased onto current main and extended to the Cursor replay guard and |
ELI5
On Windows, some Orca hook scripts "skip out" by handing stdin to
more.comandwaiting for it to finish reading. If the program that was writing the hook
payload never closes its end, that wait never ends: the hook hangs, and a leftover
more.comkeeps the pipeline open so the agent waits too. On macOS/Linux the sameskip already returns immediately, so this just makes Windows match.
What Changed
The two Windows no-op skip paths now exit without owning stdin:
src/main/claude/hook-script.ts: the Devin-imports-Claude skipif not "%DEVIN_PROJECT_DIR%"=="" goto :orca_agent_hook_drain_stdin→
if not "%DEVIN_PROJECT_DIR%"=="" exit /b 0src/main/agent-hooks/grok-replay-guard.ts: the Windows Grok-replay skipif not "%GROK_HOOK_EVENT%"=="" goto :orca_agent_hook_drain_stdin→
if not "%GROK_HOOK_EVENT%"=="" exit /b 0managed-hook-stdin-lifecycle.test.tsandgrok-replay-guard.test.tsto pin exit-without-drain for those skips.
Before (user experience): a Devin-or-Grok session on Windows outside an Orca pane
could park in
more.comuntil the agent's hook timeout, and a killed hook left alive child that kept the agent's stdout pipe open. After: those skips return
immediately, with no child and no held pipe.
The shared
buildWindowsHookStdinDrainEpilogue()itself is unchanged (themissing-managed-script launcher and the lifecycle contract still reference it).
Why
This is the same hang class the file already documents as #11549: a
read-to-EOF drain deadlocks when the caller abandons stdin rather than closing it.
The POSIX side already solved it with a deadline-bounded reader
(
POSIX_HOOK_JSON_STDIN_READER, 5s first-byte / 1.5s idle); Windows has no bound.Rotating the skip paths off the drain removes the last in-script jumps to
more.comand matches the rule the Orca env guards already follow(
buildWindowsHookEnvironmentGuardLinesexits rather than parking). The payload isdiscarded on these paths anyway, so nothing is lost by not reading it.
Alternatives considered:
fix, but a larger change touching the shared contract + launcher + contract
tests. Filed as the follow-up; the skip-path fix is small and self-contained.
more.com:more.comhas no timeoutflag, and spawning a bounded reader on Windows re-opens the EDR/perf questions
the POSIX prelude carefully avoids.
Linked Issue
Fixes #24907
Visual Proof
N/A — no UI or interaction change; the generated
.cmdtext changes only.Testing
pnpm exec vitest run --config config/vitest.config.ts --pool=forks --maxWorkers=1 --no-file-parallelism src/main/agent-hooks/grok-replay-guard.test.ts src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts→ 2 files passed, 5 passed / 11 skipped. The two tests this PR changed ran
and passed: "exits immediately when Orca env is missing and keeps drain for
other failures" (Windows managed hook stdin structure) and the Windows Grok
guard test.
pnpm run tc:node→ clean.oxlintandoxfmt --checkon the four changed files → clean.Platform tested: Windows (generated
.cmdassertions). The POSIX templates areuntouched by this change.
AI Disclosure
Implemented with Claude Code (Anthropic), driven by a human operator who
reproduced the hang on Windows and reviewed the diff and test results.
Review
Scope is limited to the Windows skip routing. The drain epilogue and the
missing-script launcher are intentionally left as-is.
Agent skill upstream boundary
N/A — no agent-skill changes.
🤖 Generated with Claude Code