Repository navigation
Fix claude_vm_node OOM behavior and hook payload retention - #2462
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors Claude hook parsing to use a compacted object and redacted fallback, adds NODE_OPTIONS merge/restore logic across shell, Go daemon, and Swift launchers with a generated restore shim, hardens shim file writes, updates tests to verify NODE_OPTIONS propagation and heap-cap enforcement. Changes
Sequence Diagram(s)sequenceDiagram
participant Launcher as Launcher (shell / Go / Swift)
participant FS as Filesystem (temp dir)
participant Node as Claude Node process
participant Restore as RestoreModule (CJS)
participant Real as Real Claude child process
Launcher->>FS: ensure/create restore-node-options.cjs
Note right of FS: file contains logic to\nrestore/delete NODE_OPTIONS using\nCMUX_ORIGINAL_NODE_OPTIONS*
Launcher->>Launcher: compute merged NODE_OPTIONS\n(--require=<restore> + --max-old-space-size=4096 + other flags)
Launcher->>Node: spawn Node with merged NODE_OPTIONS
Node->>Restore: Node loads required module (--require)
Restore->>Node: restore or delete process.env.NODE_OPTIONS\nbased on CMUX_ORIGINAL_NODE_OPTIONS_PRESENT
Node->>Real: spawn real Claude child (inherits restored NODE_OPTIONS)
Real->>FS: (tests) log observed NODE_OPTIONS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 617ae656f6
ℹ️ 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".
Comment on lines
+145
to
+146
| node_options == "--max-old-space-size=4096", | ||
| f"live socket: expected NODE_OPTIONS memory cap, got {node_options!r}", |
There was a problem hiding this comment.
This exact-value assertion is brittle because run_wrapper builds env from os.environ.copy() and does not clear NODE_OPTIONS before invoking claude. On runners where NODE_OPTIONS is pre-set, the wrapper correctly prepends the cap and produces a different value (and the stale/missing cases won’t be __UNSET__), so this test can fail for environment reasons rather than a regression. Normalize or unset NODE_OPTIONS in the harness before running the wrapper.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis PR addresses three distinct issues: V8 OOM behavior for Claude/claude-teams Node processes, memory bloat from retaining large hook payloads, and a missing Ghostty background-rendering hook in the fork. The changes are well-scoped and the implementation is consistent across all three launch paths (bash wrapper, Swift CLI, and Go daemon). Key changes:
Confidence Score: 4/5Safe to merge after addressing the test environment isolation issue in The P1 finding is a test fragility issue: the three new NODE_OPTIONS assertions in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[claude invoked] --> B{IN_CMUX && live socket?}
B -- No --> C[exec real claude as-is\nNODE_OPTIONS unchanged]
B -- Yes --> D{subcommand:\nmcp/config/api-key/rc?}
D -- Yes --> C
D -- No --> E[merge_node_options\nexport NODE_OPTIONS with\n--max-old-space-size=4096]
E --> F[exec real claude\nwith hooks + session-id]
G[cmux claude-teams / cmux claude-vm] --> H[configureAgentEnvironment]
H --> I[mergeNodeOptions\nos.Setenv NODE_OPTIONS]
I --> J[syscall.Exec claude]
K[Claude hook fires\nstdin = raw JSON] --> L[parseClaudeHookInput]
L --> M[extract sessionId, cwd,\ntranscriptPath from full object]
M --> N[compactClaudeHookObject\ndrop tool arrays, file content]
N --> O{hook type}
O -- notification --> P[summarizeClaudeHookNotification\nfrom compact object]
O -- pre-tool-use --> Q[describeToolUse / describeAskUserQuestion\nfrom compact object]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+11875
to
+11897
| for key in [ | ||
| "tool_name", | ||
| "last_assistant_message", | ||
| "lastAssistantMessage", | ||
| "event", | ||
| "event_name", | ||
| "hook_event_name", | ||
| "type", | ||
| "kind", | ||
| "notification_type", | ||
| "matcher", | ||
| "reason", | ||
| "message", | ||
| "body", | ||
| "text", | ||
| "prompt", | ||
| "error", | ||
| "description", | ||
| ] { | ||
| if let value = firstString(in: object, keys: [key]) { | ||
| compact[key] = value | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
last_assistant_message retained without truncation
last_assistant_message / lastAssistantMessage is included in the compacted object as a raw string with no length cap. On long sessions this field can contain the full text of a multi-paragraph assistant turn — potentially several kilobytes — which offsets the memory savings the compact pass was designed to achieve.
All other message-like fields (message, body, text, prompt, description) are short by nature. Only this one is unbounded.
Consider capping it at the same 180-char limit used elsewhere for display purposes:
for key in [
"tool_name",
"last_assistant_message",
"lastAssistantMessage",
// ...
] {
if var value = firstString(in: object, keys: [key]) {
if key == "last_assistant_message" || key == "lastAssistantMessage" {
value = truncate(normalizedSingleLine(value), maxLength: 180)
}
compact[key] = value
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 11855-11857: When constructing the fallback string for malformed
JSON in the Claude hook path (where ClaudeHookParsedInput is created), redact
sensitive spans before truncation/normalization: create/use a helper like
redactClaudeSensitiveSpans that calls
ClaudeHookTagExtractor.redactSensitiveSpans(value) and apply it to trimmed (or
the normalizedSingleLine input) so the rawFallback value is the redacted, then
truncated string; update the fallback assignment that currently uses
trimmed/normalize/truncate to first call the redaction helper and then continue
with normalize/truncate to ensure paths/tokens/emails are not stored or
surfaced.
- Around line 12162-12172: The mergedNodeOptions function currently only detects
the "--max-old-space-size=" form and can leave existing spaced flags or
duplicates, causing user values to override the intended
"--max-old-space-size=4096"; update mergedNodeOptions to normalize and enforce a
single token by scanning the trimmedExisting string for both forms
("--max-old-space-size=" and "--max-old-space-size <number>") using a regex,
remove any existing occurrences (both "=" and space variants), then prepend or
return a single canonical "--max-old-space-size=4096" followed by the cleaned
remaining options so there is exactly one enforced token; reference the
mergedNodeOptions function to implement the regex-based removal and
reconstruction.
In `@ghostty`:
- Line 1: Update scripts/ghosttykit-checksums.txt to include the missing
checksum for the new ghostty submodule commit
9549a4c06ca3e51a7b951ae91eb8a423032527bf: build the
GhosttyKit.xcframework.tar.gz for that exact commit (checkout
9549a4c06ca3e51a7b951ae91eb8a423032527bf and produce the artifact), compute its
SHA256, then append a new line to scripts/ghosttykit-checksums.txt in the format
"9549a4c06ca3e51a7b951ae91eb8a423032527bf <computed_sha256_checksum>" and
commit/push the change so the CI can validate the submodule update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17ae5dae-7a25-490b-adae-c2a3d5dbd979
📒 Files selected for processing (7)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.goghosttytests/test_claude_wrapper_hooks.pytests/test_cli_claude_teams_env.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
12282-12296:⚠️ Potential issue | 🟠 MajorUse the shared Claude sensitive-span redactor to avoid drift.
This local regex set can diverge from the repo’s canonical sensitive-span handling and miss classes of sensitive values in fallback text.
🔧 Suggested fix
private func redactClaudeSensitiveSpans(_ value: String) -> String { - let patterns: [(pattern: String, replacement: String)] = [ - (#"[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}"#, "<email>"), - (#"(?:~|/)[^\s\"']+"#, "<path>"), - (#"\b(?:sk|rk|sess|token|key|secret|api[_-]?key)[A-Za-z0-9._:-]{8,}\b"#, "<token>"), - (#"\b[A-Za-z0-9_-]{24,}\b"#, "<token>") - ] - return patterns.reduce(value) { partial, entry in - partial.replacingOccurrences( - of: entry.pattern, - with: entry.replacement, - options: [.regularExpression, .caseInsensitive] - ) - } + ClaudeHookTagExtractor.redactSensitiveSpans(value) }Based on learnings: “For Claude Code session tag extraction… pre-redact sensitive spans … using unanchored sensitiveSpanPatterns … to prevent PII/path fragments from slipping.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 12282 - 12296, The local redactClaudeSensitiveSpans function duplicates regex logic and risks drifting from the repo’s canonical sensitive-span handling; replace its custom patterns by calling the shared redactor (the central sensitive-span utility) instead of hardcoding patterns so all CLAUDE-related redaction uses the single source of truth. Locate redactClaudeSensitiveSpans and update its body to invoke the shared sensitive-span redactor API (e.g., the repo's sensitiveSpanPatterns/sensitiveSpanRedactor function or equivalent) with the input string, removing the inline patterns array and ensuring the same flags/options (regex, case-insensitive) are used.
🧹 Nitpick comments (5)
tests/test_cli_claude_teams_env.py (3)
73-105: Code duplication with test_claude_wrapper_hooks.py.The
claude-real.jscontent is nearly identical to the one intest_claude_wrapper_hooks.py. Consider extracting this to a shared test utility or fixture file to reduce maintenance burden and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_claude_teams_env.py` around lines 73 - 105, The claude-real.js script content duplicated in tests (created via make_executable for "claude-real.js") should be extracted into a shared test helper/fixture so both tests reuse the same source; create a utility function (e.g., make_claude_real_script or get_claude_real_template) or a pytest fixture in a common test utilities module (or conftest) that writes the canonical script and returns its path, then replace the inline make_executable block in tests/test_cli_claude_teams_env.py and test_claude_wrapper_hooks.py to call that helper/fixture (referencing the helper name and the existing make_executable call) to eliminate duplication and centralize maintenance.
151-152: Early return on failure may lose diagnostic information.When
proc.returncode != 0, the function returns empty strings for NODE_OPTIONS values. The caller inmain()handles this by printing stdout/stderr, but the diagnostic information about which specific NODE_OPTIONS checks failed is lost. Consider whether any partial log data should still be read for debugging purposes.♻️ Suggested improvement
if proc.returncode != 0: - return proc, "", "", "" + # Still try to read logs for debugging, but don't fail if they're missing + return ( + proc, + read_text(node_options_log), + read_text(runtime_node_options_log), + read_text(child_node_options_log), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_claude_teams_env.py` around lines 151 - 152, The early return when proc.returncode != 0 discards stdout/stderr and NODE_OPTIONS diagnostics; change the branch to call proc.communicate() (or otherwise read proc.stdout and proc.stderr) before returning so the function returns the actual stdout and stderr strings along with the proc object instead of empty strings; update the return at that location to return proc, stdout, stderr, <existing-node-options-values> so main() can print full diagnostic info (reference proc.returncode, proc.communicate(), and the function that calls main()).
231-236: Consider catching a more specific exception.The static analysis tool flagged the bare
Exceptioncatch. Whileresolve_cmux_cli()might raise various exceptions, catchingExceptioncan mask unexpected errors. Consider catching more specific exceptions or at least excludingKeyboardInterruptandSystemExit.♻️ Suggested fix
- try: - cli_path = resolve_cmux_cli() - except Exception as exc: - print(f"FAIL: {exc}") - return 1 + try: + cli_path = resolve_cmux_cli() + except (FileNotFoundError, OSError, ValueError) as exc: + print(f"FAIL: {exc}") + return 1Alternatively, if you need to catch all exceptions except system-exiting ones:
- except Exception as exc: + except BaseException as exc: + if isinstance(exc, (KeyboardInterrupt, SystemExit)): + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_claude_teams_env.py` around lines 231 - 236, The current main() catches a broad Exception from resolve_cmux_cli(); narrow the catch to specific exceptions (e.g., catch FileNotFoundError, OSError, RuntimeError) related to CLI resolution or, if you must catch all exceptions, re-raise system-exiting ones: in main(), replace the bare except Exception as exc with either a targeted except block listing the expected exceptions thrown by resolve_cmux_cli(), or keep except Exception as exc but immediately do "if isinstance(exc, (KeyboardInterrupt, SystemExit)): raise" before printing the error and returning 1; reference resolve_cmux_cli() and main() when making the change.tests/test_claude_wrapper_hooks.py (2)
41-47: Return tuple is getting unwieldy.The function now returns an 8-element tuple which is hard to manage and prone to errors when unpacking. Consider using a
dataclassorNamedTupleto make the return value self-documenting and less error-prone.♻️ Suggested refactor using NamedTuple
from typing import NamedTuple class WrapperResult(NamedTuple): returncode: int real_argv: list[str] cmux_log: list[str] stderr: str claudecode: str node_options: str runtime_node_options: str child_node_options: strThen update
run_wrapperto returnWrapperResult(...)and callers can use named access likeresult.node_options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_claude_wrapper_hooks.py` around lines 41 - 47, The run_wrapper function returns an unwieldy 8-tuple; replace that raw tuple with a self-documenting container (e.g., a NamedTuple or dataclass named WrapperResult) that defines fields for the eight values (e.g., returncode, real_argv, cmux_log, stderr, claudecode, node_options, runtime_node_options, child_node_options), update run_wrapper to return WrapperResult(...) instead of a raw tuple, and update all call sites in tests to access fields by name (e.g., result.node_options) or by attribute unpacking to avoid fragile positional unpacking.
81-113: Minor: Prefer strict equality in JavaScript.Lines 94 and 102 use
!=instead of!==. While this works correctly in this context (comparing numbers), strict equality is the JavaScript best practice to avoid type coercion surprises.♻️ Suggested fix
-if ((child.status ?? 0) != 0) { +if ((child.status ?? 0) !== 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_claude_wrapper_hooks.py` around lines 81 - 113, In the generated claude-real.js script created by make_executable, replace non-strict inequality uses with strict inequality: change any occurrences comparing child.status (e.g., "(child.status ?? 0) != 0") to use !== instead, and likewise replace any other "!=" comparisons in that script with "!=="; update the condition(s) around the child variable and exit/status checks so they use strict equality to avoid type-coercion surprises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 9611-9615: The early return when
createClaudeNodeOptionsRestoreModule() fails causes the OOM guard
(--max-old-space-size=4096) to be skipped; instead of returning after calling
unsetenv("CMUX_ORIGINAL_NODE_OPTIONS_PRESENT") and
unsetenv("CMUX_ORIGINAL_NODE_OPTIONS"), ensure NODE_OPTIONS is still set/updated
with the OOM flag and any necessary environment state is preserved or restored;
modify the failure branch in the createClaudeNodeOptionsRestoreModule() handling
to fall through to the same code path that enforces/merges in
"--max-old-space-size=4096" (the logic that updates NODE_OPTIONS) and apply the
same fix to the other identical block that handles restore-module creation
failure so both paths enforce the OOM guard.
In `@Resources/bin/claude`:
- Around line 90-127: merge_node_options currently tokenizes NODE_OPTIONS with
read -r -a and reassembles with plain spaces, which breaks args containing
whitespace; update merge_node_options to detect unsafe NODE_OPTIONS (e.g.,
presence of quotes, unescaped spaces inside arguments, or an existing
"--require" followed by a separate path token) and fail fast with a non-zero
exit and an explanatory error via processLogger (or stderr) instead of
attempting to rejoin, and when emitting the injected flags use a
safe-quoting/serialization for the guard_path (reference require_flag,
memory_flag, NODE_OPTIONS, tokens, filtered) so that if guard_path contains
spaces it is either properly quoted/escaped (use shell-safe serialization like
printf '%q' for the injected value) or the function refuses to operate and
returns an error.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 12282-12296: The local redactClaudeSensitiveSpans function
duplicates regex logic and risks drifting from the repo’s canonical
sensitive-span handling; replace its custom patterns by calling the shared
redactor (the central sensitive-span utility) instead of hardcoding patterns so
all CLAUDE-related redaction uses the single source of truth. Locate
redactClaudeSensitiveSpans and update its body to invoke the shared
sensitive-span redactor API (e.g., the repo's
sensitiveSpanPatterns/sensitiveSpanRedactor function or equivalent) with the
input string, removing the inline patterns array and ensuring the same
flags/options (regex, case-insensitive) are used.
---
Nitpick comments:
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 41-47: The run_wrapper function returns an unwieldy 8-tuple;
replace that raw tuple with a self-documenting container (e.g., a NamedTuple or
dataclass named WrapperResult) that defines fields for the eight values (e.g.,
returncode, real_argv, cmux_log, stderr, claudecode, node_options,
runtime_node_options, child_node_options), update run_wrapper to return
WrapperResult(...) instead of a raw tuple, and update all call sites in tests to
access fields by name (e.g., result.node_options) or by attribute unpacking to
avoid fragile positional unpacking.
- Around line 81-113: In the generated claude-real.js script created by
make_executable, replace non-strict inequality uses with strict inequality:
change any occurrences comparing child.status (e.g., "(child.status ?? 0) != 0")
to use !== instead, and likewise replace any other "!=" comparisons in that
script with "!=="; update the condition(s) around the child variable and
exit/status checks so they use strict equality to avoid type-coercion surprises.
In `@tests/test_cli_claude_teams_env.py`:
- Around line 73-105: The claude-real.js script content duplicated in tests
(created via make_executable for "claude-real.js") should be extracted into a
shared test helper/fixture so both tests reuse the same source; create a utility
function (e.g., make_claude_real_script or get_claude_real_template) or a pytest
fixture in a common test utilities module (or conftest) that writes the
canonical script and returns its path, then replace the inline make_executable
block in tests/test_cli_claude_teams_env.py and test_claude_wrapper_hooks.py to
call that helper/fixture (referencing the helper name and the existing
make_executable call) to eliminate duplication and centralize maintenance.
- Around line 151-152: The early return when proc.returncode != 0 discards
stdout/stderr and NODE_OPTIONS diagnostics; change the branch to call
proc.communicate() (or otherwise read proc.stdout and proc.stderr) before
returning so the function returns the actual stdout and stderr strings along
with the proc object instead of empty strings; update the return at that
location to return proc, stdout, stderr, <existing-node-options-values> so
main() can print full diagnostic info (reference proc.returncode,
proc.communicate(), and the function that calls main()).
- Around line 231-236: The current main() catches a broad Exception from
resolve_cmux_cli(); narrow the catch to specific exceptions (e.g., catch
FileNotFoundError, OSError, RuntimeError) related to CLI resolution or, if you
must catch all exceptions, re-raise system-exiting ones: in main(), replace the
bare except Exception as exc with either a targeted except block listing the
expected exceptions thrown by resolve_cmux_cli(), or keep except Exception as
exc but immediately do "if isinstance(exc, (KeyboardInterrupt, SystemExit)):
raise" before printing the error and returning 1; reference resolve_cmux_cli()
and main() when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19a5378c-7f8f-4739-8c64-e1af0990a2fa
📒 Files selected for processing (7)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.goscripts/ghosttykit-checksums.txttests/test_claude_wrapper_hooks.pytests/test_cli_claude_teams_env.py
✅ Files skipped from review due to trivial changes (1)
- scripts/ghosttykit-checksums.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- daemon/remote/cmd/cmuxd-remote/agent_launch.go
Comment on lines
+9611
to
+9615
| guard let restoreModuleURL = try? createClaudeNodeOptionsRestoreModule() else { | ||
| unsetenv("CMUX_ORIGINAL_NODE_OPTIONS_PRESENT") | ||
| unsetenv("CMUX_ORIGINAL_NODE_OPTIONS") | ||
| return | ||
| } |
There was a problem hiding this comment.
Fail-open path skips OOM guard when restore module creation fails.
On Line 9611, the early return means NODE_OPTIONS is never updated if restore-module creation fails, so --max-old-space-size=4096 is not enforced in that path.
🔧 Suggested fix
- guard let restoreModuleURL = try? createClaudeNodeOptionsRestoreModule() else {
- unsetenv("CMUX_ORIGINAL_NODE_OPTIONS_PRESENT")
- unsetenv("CMUX_ORIGINAL_NODE_OPTIONS")
- return
- }
- if let existing = processEnvironment["NODE_OPTIONS"] {
+ let restoreModuleURL = try? createClaudeNodeOptionsRestoreModule()
+ if let existing = processEnvironment["NODE_OPTIONS"] {
setenv("CMUX_ORIGINAL_NODE_OPTIONS_PRESENT", "1", 1)
setenv("CMUX_ORIGINAL_NODE_OPTIONS", existing, 1)
} else {
setenv("CMUX_ORIGINAL_NODE_OPTIONS_PRESENT", "0", 1)
unsetenv("CMUX_ORIGINAL_NODE_OPTIONS")
}
- setenv(
- "NODE_OPTIONS",
- mergedNodeOptions(
- existing: processEnvironment["NODE_OPTIONS"],
- restoreModulePath: restoreModuleURL.path
- ),
- 1
- )
+ let merged: String
+ if let restoreModuleURL {
+ merged = mergedNodeOptions(
+ existing: processEnvironment["NODE_OPTIONS"],
+ restoreModulePath: restoreModuleURL.path
+ )
+ } else {
+ // Keep heap cap even if restore shim cannot be created.
+ merged = "--max-old-space-size=4096 \(cleanedNodeOptions(processEnvironment["NODE_OPTIONS"]))"
+ .trimmingCharacters(in: .whitespacesAndNewlines)
+ }
+ setenv("NODE_OPTIONS", merged, 1)Also applies to: 9623-9630
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 9611 - 9615, The early return when
createClaudeNodeOptionsRestoreModule() fails causes the OOM guard
(--max-old-space-size=4096) to be skipped; instead of returning after calling
unsetenv("CMUX_ORIGINAL_NODE_OPTIONS_PRESENT") and
unsetenv("CMUX_ORIGINAL_NODE_OPTIONS"), ensure NODE_OPTIONS is still set/updated
with the OOM flag and any necessary environment state is preserved or restored;
modify the failure branch in the createClaudeNodeOptionsRestoreModule() handling
to fall through to the same code path that enforces/merges in
"--max-old-space-size=4096" (the logic that updates NODE_OPTIONS) and apply the
same fix to the other identical block that handles restore-module creation
failure so both paths enforce the OOM guard.
Comment on lines
+90
to
+127
| merge_node_options() { | ||
| local guard_path="$1" | ||
| local require_flag="--require=$guard_path" | ||
| local memory_flag="--max-old-space-size=4096" | ||
| local existing="${NODE_OPTIONS:-}" | ||
| local -a filtered=() | ||
| local -a tokens=() | ||
| local token | ||
| local skip_next=0 | ||
|
|
||
| if [[ -z "$guard_path" ]]; then | ||
| printf '%s' "$existing" | ||
| return 0 | ||
| fi | ||
|
|
||
| read -r -a tokens <<<"$existing" | ||
| for token in "${tokens[@]}"; do | ||
| if (( skip_next )); then | ||
| skip_next=0 | ||
| continue | ||
| fi | ||
| if [[ "$token" == "--max-old-space-size" ]]; then | ||
| skip_next=1 | ||
| continue | ||
| fi | ||
| if [[ "$token" == --max-old-space-size=* ]]; then | ||
| continue | ||
| fi | ||
| filtered+=("$token") | ||
| done | ||
|
|
||
| if (( ${#filtered[@]} == 0 )); then | ||
| printf '%s %s' "$require_flag" "$memory_flag" | ||
| return 0 | ||
| fi | ||
| printf '%s %s' "$require_flag" "$memory_flag" | ||
| printf ' %s' "${filtered[@]}" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
existing='--require "/tmp/a b.cjs" --trace-warnings'
read -r -a tokens <<<"$existing"
printf 'original: %s\n' "$existing"
printf 'split:\n'
for i in "${!tokens[@]}"; do
printf ' token[%s]=<%s>\n' "$i" "${tokens[$i]}"
done
printf 'rejoined:'
printf ' %s' "${tokens[@]}"
printf '\n'Repository: manaflow-ai/cmux
Length of output: 264
🏁 Script executed:
sed -n '90,127p' Resources/bin/claudeRepository: manaflow-ai/cmux
Length of output: 1033
Don't split and rebuild NODE_OPTIONS with raw whitespace.
Line 105 tokenizes NODE_OPTIONS using read -r -a, and lines 125-126 rejoin with plain spaces. This breaks arguments containing whitespace. For example, NODE_OPTIONS='--require "/tmp/a b.cjs" --trace-warnings' splits into invalid tokens (--require, "/tmp/a, b.cjs", --trace-warnings), and the same problem occurs for the injected --require if the guard path contains spaces. Use a proper shell argument parser/serializer that preserves quoting, or fail fast when the value cannot be safely represented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/bin/claude` around lines 90 - 127, merge_node_options currently
tokenizes NODE_OPTIONS with read -r -a and reassembles with plain spaces, which
breaks args containing whitespace; update merge_node_options to detect unsafe
NODE_OPTIONS (e.g., presence of quotes, unescaped spaces inside arguments, or an
existing "--require" followed by a separate path token) and fail fast with a
non-zero exit and an explanatory error via processLogger (or stderr) instead of
attempting to rejoin, and when emitting the injected flags use a
safe-quoting/serialization for the guard_path (reference require_flag,
memory_flag, NODE_OPTIONS, tokens, filtered) so that if guard_path contains
spaces it is either properly quoted/escaped (use shell-safe serialization like
printf '%q' for the injected value) or the function refuses to operate and
returns an error.
# Conflicts: # Resources/bin/claude # tests/test_claude_wrapper_hooks.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_claude_wrapper_hooks.py (1)
300-316: Exercise the bad-TMPDIRbranch with existing user flags too.This only proves the unset case. A regression that clears caller-supplied
NODE_OPTIONSwhen shim creation fails would still pass here. I'd add one variant with a non-heap flag and assert the launcher, runtime, and child logs all keep it unchanged.🧪 Suggested coverage expansion
def test_live_socket_tmpdir_failure_skips_node_options_injection(failures: list[str]) -> None: + existing = "--trace-warnings" with tempfile.TemporaryDirectory(prefix="cmux-claude-wrapper-bad-tmp-") as td: bad_tmpdir = Path(td) / "not-a-directory" bad_tmpdir.write_text("occupied", encoding="utf-8") code, real_argv, cmux_log, stderr, claudecode, node_options, runtime_node_options, child_node_options, _ = run_wrapper( socket_state="live", argv=["hello"], tmpdir=str(bad_tmpdir), + node_options=existing, ) @@ - expect(node_options == "__UNSET__", f"tmpdir failure: expected NODE_OPTIONS injection to be skipped, got {node_options!r}", failures) - expect(runtime_node_options == "__UNSET__", f"tmpdir failure: expected runtime NODE_OPTIONS passthrough, got {runtime_node_options!r}", failures) - expect(child_node_options == "__UNSET__", f"tmpdir failure: expected child NODE_OPTIONS passthrough, got {child_node_options!r}", failures) + expect(node_options == existing, f"tmpdir failure: expected existing NODE_OPTIONS passthrough, got {node_options!r}", failures) + expect(runtime_node_options == existing, f"tmpdir failure: expected runtime NODE_OPTIONS passthrough, got {runtime_node_options!r}", failures) + expect(child_node_options == existing, f"tmpdir failure: expected child NODE_OPTIONS passthrough, got {child_node_options!r}", failures)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_claude_wrapper_hooks.py` around lines 300 - 316, Add a second variant of test_live_socket_tmpdir_failure_skips_node_options_injection that exercises the bad-TMPDIR branch while supplying a caller-side NODE_OPTIONS flag (e.g., a non-heap flag passed via argv or environment to run_wrapper) and assert that node_options, runtime_node_options, and child_node_options remain unchanged (equal to the original supplied value) after run_wrapper returns; update the test to call run_wrapper twice (one for the existing unset case and one with a provided NODE_OPTIONS value) and add expect checks comparing the returned node_options/runtime_node_options/child_node_options to the original supplied flag so we catch regressions that would clear caller-supplied NODE_OPTIONS when shim creation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 149-168: The test harness lets run_wrapper inherit the caller's
TMPDIR when tmpdir is None, causing flaky failures; ensure env["TMPDIR"] is
always pinned to the test's temp dir by default: in the setup where env is built
(the block that currently does if tmpdir is not None: env["TMPDIR"] = tmpdir),
change it so env["TMPDIR"] is set to the test temp dir when tmpdir is None
(i.e., default tmpdir -> test temp) and only leave it overridden when an
explicit tmpdir argument is provided; modify the run_wrapper/test harness code
that references tmpdir, env["TMPDIR"], and run_wrapper() accordingly so the
TMPDIR used for the process is deterministic for the test.
---
Nitpick comments:
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 300-316: Add a second variant of
test_live_socket_tmpdir_failure_skips_node_options_injection that exercises the
bad-TMPDIR branch while supplying a caller-side NODE_OPTIONS flag (e.g., a
non-heap flag passed via argv or environment to run_wrapper) and assert that
node_options, runtime_node_options, and child_node_options remain unchanged
(equal to the original supplied value) after run_wrapper returns; update the
test to call run_wrapper twice (one for the existing unset case and one with a
provided NODE_OPTIONS value) and add expect checks comparing the returned
node_options/runtime_node_options/child_node_options to the original supplied
flag so we catch regressions that would clear caller-supplied NODE_OPTIONS when
shim creation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 574759f8-57de-4afb-8368-f3a8bab35637
📒 Files selected for processing (2)
Resources/bin/claudetests/test_claude_wrapper_hooks.py
✅ Files skipped from review due to trivial changes (1)
- Resources/bin/claude
Comment on lines
149
to
+168
| env = os.environ.copy() | ||
| env["PATH"] = f"{wrapper_dir}:{real_dir}:/usr/bin:/bin" | ||
| env["PATH"] = f"{wrapper_dir}:{real_dir}:{env.get('PATH', '/usr/bin:/bin')}" | ||
| env["CMUX_SURFACE_ID"] = "surface:test" | ||
| env["CMUX_SOCKET_PATH"] = socket_path | ||
| env["FAKE_REAL_ARGS_LOG"] = str(real_args_log) | ||
| env["FAKE_REAL_CLAUDECODE_LOG"] = str(real_claudecode_log) | ||
| env["FAKE_REAL_NODE_OPTIONS_LOG"] = str(real_node_options_log) | ||
| env["FAKE_REAL_RUNTIME_NODE_OPTIONS_LOG"] = str(real_runtime_node_options_log) | ||
| env["FAKE_REAL_CHILD_NODE_OPTIONS_LOG"] = str(real_child_node_options_log) | ||
| env["FAKE_REAL_NODE_SCRIPT"] = str(real_dir / "claude-real.js") | ||
| env["FAKE_HOOK_CMUX_BIN_LOG"] = str(hook_cmux_bin_log) | ||
| env["FAKE_CMUX_LOG"] = str(cmux_log) | ||
| env["FAKE_CMUX_PING_OK"] = "1" if socket_state == "live" else "0" | ||
| env["CMUX_BUNDLED_CLI_PATH"] = str(bundled_cli_path) | ||
| env["CLAUDECODE"] = "nested-session-sentinel" | ||
| env.pop("NODE_OPTIONS", None) | ||
| if tmpdir is not None: | ||
| env["TMPDIR"] = tmpdir | ||
| if node_options is not None: | ||
| env["NODE_OPTIONS"] = node_options |
There was a problem hiding this comment.
Pin TMPDIR in the default harness path.
run_wrapper() now inherits the caller's TMPDIR whenever tmpdir=None. If a runner has a bad or read-only TMPDIR, the new "skip injection" branch triggers and the live-socket assertions fail for environmental reasons. Set it to this test's temp dir by default, and only override it in the explicit failure case.
🛠️ Proposed fix
env = os.environ.copy()
env["PATH"] = f"{wrapper_dir}:{real_dir}:{env.get('PATH', '/usr/bin:/bin')}"
+ env["TMPDIR"] = td
env["CMUX_SURFACE_ID"] = "surface:test"
env["CMUX_SOCKET_PATH"] = socket_path
env["FAKE_REAL_ARGS_LOG"] = str(real_args_log)
@@
env["CLAUDECODE"] = "nested-session-sentinel"
env.pop("NODE_OPTIONS", None)
if tmpdir is not None:
env["TMPDIR"] = tmpdir🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_claude_wrapper_hooks.py` around lines 149 - 168, The test harness
lets run_wrapper inherit the caller's TMPDIR when tmpdir is None, causing flaky
failures; ensure env["TMPDIR"] is always pinned to the test's temp dir by
default: in the setup where env is built (the block that currently does if
tmpdir is not None: env["TMPDIR"] = tmpdir), change it so env["TMPDIR"] is set
to the test temp dir when tmpdir is None (i.e., default tmpdir -> test temp) and
only leave it overridden when an explicit tmpdir argument is provided; modify
the run_wrapper/test harness code that references tmpdir, env["TMPDIR"], and
run_wrapper() accordingly so the TMPDIR used for the process is deterministic
for the test.
This was referenced Apr 1, 2026
5 tasks
This was referenced Apr 28, 2026
This was referenced May 12, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--max-old-space-size=4096so V8 fails fast instead of consuming system memorymacos-background-from-layerhook in the fork and update the submodule pointer so embedded terminal background rendering keeps using the host layer pathNotes
./scripts/reload.sh --tag vm-node-oommanaflow-ai/ghosttybranchissue-2443-restore-layer-bgSummary by cubic
Fixes VM Node OOMs by capping V8 heap to 4 GiB and injecting a preload that restores the original
NODE_OPTIONSso Claude and its children keep user flags. Also compacts Claude hook payloads to drop large JSON while preserving statuses and notifications. Aligns with Linear #2443.Bug Fixes
--max-old-space-size=4096by prepending--require=<tmp>/restore-node-options.cjsand the cap in theclaudewrapper,claude-teams, and remote agent; strip existing heap flags (including space‑separated form), then restore the originalNODE_OPTIONSat runtime so child processes inherit it; skip injection ifTMPDIRis unusable; atomic shim writes to avoid partial updates; tests cover merge/restore paths.Dependencies
ghosttysubmodule to restore themacos-background-from-layerhook for embedded terminal backgrounds.Written for commit 594bef2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Tests