fix: Windows ENOENT in claw.js + session-end.js subprocess corruption guard#1500
fix: Windows ENOENT in claw.js + session-end.js subprocess corruption guard#1500pangerlkr wants to merge 2 commits intoaffaan-m:mainfrom
Conversation
…ffaan-m#1469) Fixes two bugs: 1. claw.js askClaude(): spawnSync('claude') fails with ENOENT on Windows because claude is installed as claude.cmd and Node cannot resolve .cmd extensions without shell:true. Added shell: process.platform === 'win32' to fix. 2. session-end.js: added ECC_SUMMARY_SUBPROCESS guard to prevent subprocess sessions from corrupting parent session .tmp files.
…affaan-m#1494) Add check to skip execution in AI-summary subprocess
📝 WalkthroughWalkthroughTwo small fixes: Windows shell compatibility for the Claude subprocess spawn in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 SummaryFix 1 (
Confidence Score: 4/5Fix 1 is safe to merge; Fix 2 is a no-op because the guard env var is never set. The Windows ENOENT fix is clean and consistent. However, the subprocess corruption guard (Fix 2) depends on ECC_SUMMARY_SUBPROCESS=1 being injected at spawn time — which no existing code does — so the claimed fix is inoperative. This is a P1 correctness issue on the stated purpose of Fix 2. scripts/hooks/session-end.js — the guard env var is never set, making Fix 2 a no-op Important Files Changed
Sequence DiagramsequenceDiagram
participant Hook as Stop Hook (Claude Code)
participant SE as session-end.js
participant SS as session-summary.js (missing)
participant Claude as claude -p subprocess
Hook->>SE: fires on session stop
SE->>SE: check ECC_SUMMARY_SUBPROCESS (never '1')
SE->>SE: write .tmp session file ✓
SE->>SS: (would spawn claude -p with ECC_SUMMARY_SUBPROCESS=1)
Note over SS,Claude: session-summary.js does not exist in repo
Claude-->>Hook: subprocess fires Stop hook again
Hook->>SE: fires session-end.js again
SE->>SE: ECC_SUMMARY_SUBPROCESS still not set → guard never fires
SE->>SE: overwrites .tmp with prompt text (corruption persists)
Reviews (1): Last reviewed commit: "fix(hooks): guard session-end.js against..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| async function main() { | ||
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | ||
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); |
There was a problem hiding this comment.
ECC_SUMMARY_SUBPROCESS is checked here but is never assigned '1' anywhere in this repository. A search across all hook scripts confirms no call site passes this variable when spawning claude -p. Additionally, session-summary.js (the file described in the PR as the spawn site) does not exist in the codebase. As written, the guard will never trigger and the corruption described in #1494 is not actually prevented.
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | ||
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); |
There was a problem hiding this comment.
The comment uses 4-space indentation while the guard statement uses 2-space indentation, breaking the uniform 2-space style used in the rest of main().
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | |
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); | |
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | |
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| env: { ...process.env, CLAUDECODE: '' }, | ||
| timeout: 300000, | ||
| timeout: 300000, |
There was a problem hiding this comment.
There is a trailing space after the comma on this line.
| timeout: 300000, | |
| timeout: 300000, |
![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: 9e533ab986
ℹ️ 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".
| env: { ...process.env, CLAUDECODE: '' }, | ||
| timeout: 300000, | ||
| timeout: 300000, | ||
| shell: process.platform === 'win32', |
There was a problem hiding this comment.
Running spawnSync('claude', args, { shell: process.platform === 'win32' }) makes askClaude() execute through cmd.exe on Windows while args contains fullPrompt, which is built from untrusted session history and user input. If that text includes shell metacharacters (for example &, |, redirections, or quote-breaking content), it can be interpreted by the shell and run arbitrary commands locally instead of being passed verbatim to Claude. This security regression is introduced by enabling shell for the same code path that handles raw prompt text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR targets two hook/script reliability issues: improving Windows compatibility for NanoClaw’s claude subprocess invocation and preventing session summary file overwrites when session-end.js runs inside an AI-summary subprocess context.
Changes:
scripts/claw.js: run theclaudesubprocess withshell: trueon Windows to avoidspawn claude ENOENT.scripts/hooks/session-end.js: add an env-flag early-exit guard to skip session file writes in summary subprocesses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| scripts/hooks/session-end.js | Adds an env-gated early exit intended to prevent .tmp session file overwrites from subprocesses. |
| scripts/claw.js | Enables Windows shell execution for the claude subprocess to fix PATH resolution of .cmd shims. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | ||
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); |
There was a problem hiding this comment.
ECC_SUMMARY_SUBPROCESS is not set anywhere else in this repo (search under scripts/ shows only this reference), so this guard will never trigger in the corruption scenario unless an external spawner is updated to export the env var. If the goal is to fix #1494 within this codebase, consider implementing a guard that can be derived from data session-end already has (e.g., make the session filename incorporate the transcript/session UUID) or add the env var in the code that spawns the summary subprocess (if that code lives in this repo).
| } | ||
|
|
||
| async function main() { | ||
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files |
There was a problem hiding this comment.
Indentation is inconsistent here (the new comment line is indented differently than the surrounding block). Please align it with the file’s existing 2-space indentation to keep formatting consistent.
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | |
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files |
|
|
||
| async function main() { | ||
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | ||
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); |
There was a problem hiding this comment.
This introduces a new early-exit path, but there’s no test coverage for it (no existing tests reference ECC_SUMMARY_SUBPROCESS). Given the extensive session-end.js test suite, add a regression test that sets ECC_SUMMARY_SUBPROCESS=1 and asserts the hook exits 0 without writing/updating a session file.
| const result = spawnSync('claude', args, { | ||
| encoding: 'utf8', | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| env: { ...process.env, CLAUDECODE: '' }, | ||
| timeout: 300000, | ||
| timeout: 300000, | ||
| shell: process.platform === 'win32', | ||
| }); |
There was a problem hiding this comment.
On Windows, shell: true routes execution through cmd.exe, which has a much smaller command-line length limit than CreateProcess. Since fullPrompt includes the entire session history (unbounded unless the user runs /compact), longer sessions can start failing on Windows even after ENOENT is fixed. Consider enabling shell only as a fallback (e.g., retry with shell when the non-shell spawn fails with ENOENT) or otherwise avoiding cmd.exe for long prompt payloads.
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| env: { ...process.env, CLAUDECODE: '' }, | ||
| timeout: 300000, | ||
| timeout: 300000, |
There was a problem hiding this comment.
There’s trailing whitespace after the comma on the timeout: 300000, line; please remove it to avoid churn and keep formatting clean.
| timeout: 300000, | |
| timeout: 300000, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/claw.js (1)
94-100:⚠️ Potential issue | 🟠 MajorAdd model name validation to prevent command injection on Windows.
The
modelparameter (line 340) is set directly from user input without validation and passed tospawnSyncas an argv withshell: trueon Windows. A user could inject shell metacharacters (e.g.,/model "x" & malicious & "y") which cmd.exe would interpret. Additionally,fullPromptis user-controlled and passed as argv.Validate the model parameter before calling
askClaude()with a pattern like/^[a-zA-Z0-9._:/-]+$/. If stdin input support is available, consider passingfullPromptvia theinputoption instead of argv to further reduce the attack surface, especially on Windows where shell parsing is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/claw.js` around lines 94 - 100, Validate the user-supplied model string before passing it to askClaude()/spawnSync by enforcing a strict pattern (e.g., /^[a-zA-Z0-9._:/-]+$/) on the variable named model and reject or sanitize values that do not match; when on Windows (where spawnSync is invoked with shell: true) ensure only validated model values are used as an argv element for the 'claude' command. Also avoid putting large or user-controlled fullPrompt on the argv: when stdin is supported, pass fullPrompt via spawnSync's input option instead of as an argument to reduce shell parsing risk, and keep env/CLAUDECODE and timeout behavior unchanged. Use the symbols model, fullPrompt, askClaude(), spawnSync and the CLAUDEDECODE-related invocation to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/claw.js`:
- Around line 94-100: Validate the user-supplied model string before passing it
to askClaude()/spawnSync by enforcing a strict pattern (e.g.,
/^[a-zA-Z0-9._:/-]+$/) on the variable named model and reject or sanitize values
that do not match; when on Windows (where spawnSync is invoked with shell: true)
ensure only validated model values are used as an argv element for the 'claude'
command. Also avoid putting large or user-controlled fullPrompt on the argv:
when stdin is supported, pass fullPrompt via spawnSync's input option instead of
as an argument to reduce shell parsing risk, and keep env/CLAUDECODE and timeout
behavior unchanged. Use the symbols model, fullPrompt, askClaude(), spawnSync
and the CLAUDEDECODE-related invocation to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f5a396c-bd00-4be4-a52d-41b6134320d4
📒 Files selected for processing (2)
scripts/claw.jsscripts/hooks/session-end.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/session-end.js">
<violation number="1" location="scripts/hooks/session-end.js:182">
P1: `ECC_SUMMARY_SUBPROCESS` is checked here but is never set to `'1'` anywhere in this repository. Without a corresponding assignment in the code that spawns `claude -p` for summarization, this guard will never trigger and the session file corruption described in #1494 remains unfixed. Either add the env var export at the spawn site, or use a detection mechanism that doesn't require coordination with external code (e.g., checking for a distinguishing characteristic of subprocess invocations).</violation>
</file>
<file name="scripts/claw.js">
<violation number="1" location="scripts/claw.js:99">
P2: Enabling shell execution on Windows routes the user-controlled prompt through cmd.exe, allowing shell expansion of prompt content and potential injection/corruption.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| async function main() { | ||
| // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files | ||
| if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0); |
There was a problem hiding this comment.
P1: ECC_SUMMARY_SUBPROCESS is checked here but is never set to '1' anywhere in this repository. Without a corresponding assignment in the code that spawns claude -p for summarization, this guard will never trigger and the session file corruption described in #1494 remains unfixed. Either add the env var export at the spawn site, or use a detection mechanism that doesn't require coordination with external code (e.g., checking for a distinguishing characteristic of subprocess invocations).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/session-end.js, line 182:
<comment>`ECC_SUMMARY_SUBPROCESS` is checked here but is never set to `'1'` anywhere in this repository. Without a corresponding assignment in the code that spawns `claude -p` for summarization, this guard will never trigger and the session file corruption described in #1494 remains unfixed. Either add the env var export at the spawn site, or use a detection mechanism that doesn't require coordination with external code (e.g., checking for a distinguishing characteristic of subprocess invocations).</comment>
<file context>
@@ -178,6 +178,8 @@ function mergeSessionHeader(content, today, currentTime, metadata) {
async function main() {
+ // Skip if running inside an AI-summary subprocess to prevent overwriting parent session files
+ if (process.env.ECC_SUMMARY_SUBPROCESS === '1') process.exit(0);
// Parse stdin JSON to get transcript_path
let transcriptPath = null;
</file context>
| env: { ...process.env, CLAUDECODE: '' }, | ||
| timeout: 300000, | ||
| timeout: 300000, | ||
| shell: process.platform === 'win32', |
There was a problem hiding this comment.
P2: Enabling shell execution on Windows routes the user-controlled prompt through cmd.exe, allowing shell expansion of prompt content and potential injection/corruption.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/claw.js, line 99:
<comment>Enabling shell execution on Windows routes the user-controlled prompt through cmd.exe, allowing shell expansion of prompt content and potential injection/corruption.</comment>
<file context>
@@ -95,7 +95,8 @@ function askClaude(systemPrompt, history, userMessage, model) {
env: { ...process.env, CLAUDECODE: '' },
- timeout: 300000,
+ timeout: 300000,
+ shell: process.platform === 'win32',
});
</file context>
What Changed
This PR fixes two independent bugs:
Fix 1 —
scripts/claw.jsWindows ENOENT (closes #1469)askClaude()callsspawnSync('claude', args, {...})withoutshell: true. On Windows,claudeis installed asclaude.cmd— a batch wrapper — and Node'sspawncannot resolve.cmdextensions via PATH without shell mode. This causesspawn claude ENOENTandaskClaude()returns an error string.Change: Added
shell: process.platform === 'win32'to thespawnSyncoptions inaskClaude(). This mirrors the exact pattern already used inscripts/hooks/mcp-health-check.jsafter PR #1456, and only activates on Windows.Fix 2 —
scripts/hooks/session-end.jssubprocess corruption (closes #1494)When
session-summary.jsspawnsclaude -p ...as a subprocess, the subprocess also fires the Stop hook chain includingsession-end.js. Both parent and subprocess compute the same session filename, so the subprocess overwrites the parent's valid summary with the AI-summarization prompt text.Change: Added an early exit guard at the top of
main()insession-end.js:This prevents any subprocess that sets
ECC_SUMMARY_SUBPROCESS=1from writing.tmpfiles. This is a safe, additive change — it does not affect any real user session.Why This Change
claw.jsas an interactive REPL.~/.claude/sessions/*.tmpfiles that causes incorrect session summaries.Testing Done
shell: process.platform === 'win32'pattern is consistent with existing mcp-health-check.js fixnode tests/run-all.js)Summary by cubic
Fixes Windows spawn errors in
scripts/claw.jsand prevents subprocesses from overwriting session summaries. Restores REPL usage on Windows and avoids silent.tmpcorruption.spawnSync('claude', ...)withshell: process.platform === 'win32'soclaude.cmdresolves via PATH.scripts/hooks/session-end.js, exit early whenECC_SUMMARY_SUBPROCESS=1to skip writes and avoid overwriting the parent session.Written for commit 9e533ab. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores