Repository navigation
Fix stale Claude NODE_OPTIONS restore preload - #3966
Closed
austinywang wants to merge 21 commits into
Closed
austinywang wants to merge 21 commits into
austinywang wants to merge 21 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPlaces restore-node-options.cjs in the first writable candidate directory (prefer HOME/.claude/cmux), normalizes/sanitizes NODE_OPTIONS by removing injected restore-module requires and associated heap caps, and updates tests and path recognition across Swift, Bash, Go, and Python. ChangesPersistent NODE_OPTIONS restore-module
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR fixes a macOS session-scoped TMPDIR cleanup race (#3463) by persisting the Claude NODE_OPTIONS restore preload module under
Confidence Score: 5/5Safe to merge; all three code paths apply the fix consistently, atomic writes preserve the restore module across concurrent launches, and the regression tests cover the exact macOS TMPDIR-cleanup scenario the fix targets. The fix is narrow and mechanical — write the restore module to a stable home-directory path before falling back to the session-scoped TMPDIR, and strip stale inherited preloads at startup. The walker logic in all three implementations handles every form of the injected flag and is idempotent. No blocking primitives, actor-isolation issues, or user-facing strings were introduced. No files require special attention. The one-line change in AgentLaunchEnvironmentPolicy.swift is kept in sync with the identical extension in CLI/cmux.swift. Important Files Changed
Reviews (15): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
austinywang
force-pushed
the
issue-3463-node-options-require-tmpdir-stale
branch
from
May 12, 2026 06:12
b0e4272 to
17a02c0
Compare
austinywang
force-pushed
the
issue-3463-node-options-require-tmpdir-stale
branch
from
May 12, 2026 07:37
17a02c0 to
bf0f982
Compare
…ns-require-tmpdir-stale
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 12388-12395: The current logic sets homePath from
environment["HOME"] trimmed or NSHomeDirectory(), but then skips adding a
candidate when that trimmed value is empty, causing an explicit HOME="" to
bypass NSHomeDirectory(); update the assignment for homePath so that if
environment["HOME"] exists but is empty/whitespace you fall back to
NSHomeDirectory(), otherwise use the trimmed value (and if environment["HOME"]
is nil also use NSHomeDirectory()); keep the subsequent check and candidate
append to URL(fileURLWithPath: homePath, isDirectory:
true).appendingPathComponent(".claude", isDirectory:
true).appendingPathComponent("cmux", isDirectory: true) intact.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f26ddbcc-9e7a-413c-aba6-326598629a8a
📒 Files selected for processing (3)
CLI/cmux.swiftResources/bin/claudetests/test_claude_wrapper_hooks.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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)
daemon/remote/cmd/cmuxd-remote/agent_launch.go (1)
420-494: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsolidate duplicated token-walking logic between
cleanedNodeOptionsandnodeOptionsForRestore.The two functions share an identical ~25-line loop covering the empty-tokens guard, the
dropInjectedHeapCapwindow, restore-module require detection (both space and inline forms), and the final default-append. They only diverge in how--max-old-space-sizeis handled (strip vs. normalize-and-keep). A future change to require/heap-cap detection (e.g., a new injected flag or path location) will need to be applied to both copies and is easy to miss.Consider extracting a shared walker parameterized by a heap-cap handler, e.g.:
♻️ Proposed consolidation
type heapCapMode int const ( heapCapStrip heapCapMode = iota heapCapNormalize ) func walkNodeOptions(existing string, mode heapCapMode) string { tokens := strings.Fields(existing) if len(tokens) == 0 { return "" } filtered := make([]string, 0, len(tokens)) dropInjectedHeapCap := false for i := 0; i < len(tokens); i++ { token := tokens[i] if dropInjectedHeapCap && isInjectedNodeHeapCap(tokens, i) { i += nodeHeapCapWidth(tokens, i) - 1 dropInjectedHeapCap = false continue } dropInjectedHeapCap = false if isRequireOption(token) && i+1 < len(tokens) && isCmuxNodeOptionsRestoreModulePath(tokens[i+1]) { i++ dropInjectedHeapCap = true continue } if path, ok := inlineRequireOptionPath(token); ok && isCmuxNodeOptionsRestoreModulePath(path) { dropInjectedHeapCap = true continue } switch mode { case heapCapStrip: if token == "--max-old-space-size" { if i+1 < len(tokens) { i++ } continue } if strings.HasPrefix(token, "--max-old-space-size=") { continue } case heapCapNormalize: if token == "--max-old-space-size" && i+1 < len(tokens) { filtered = append(filtered, "--max-old-space-size="+tokens[i+1]) i++ continue } } filtered = append(filtered, token) } return strings.Join(filtered, " ") } func cleanedNodeOptions(existing string) string { return walkNodeOptions(existing, heapCapStrip) } func nodeOptionsForRestore(existing string) string { return walkNodeOptions(existing, heapCapNormalize) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daemon/remote/cmd/cmuxd-remote/agent_launch.go` around lines 420 - 494, The two functions cleanedNodeOptions and nodeOptionsForRestore duplicate the same token-walking loop; extract that common logic into a single walkNodeOptions(existing string, mode heapCapMode) function (define heapCapMode with values like heapCapStrip and heapCapNormalize) and move the shared empty-tokens guard, dropInjectedHeapCap handling, require/inline require detection, and default append into it; inside the walker branch on mode to either strip both "--max-old-space-size" forms (heapCapStrip) or normalize the space form into "--max-old-space-size=NNN" (heapCapNormalize), then make cleanedNodeOptions and nodeOptionsForRestore thin wrappers that call walkNodeOptions with the appropriate mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@daemon/remote/cmd/cmuxd-remote/agent_launch.go`:
- Around line 420-494: The two functions cleanedNodeOptions and
nodeOptionsForRestore duplicate the same token-walking loop; extract that common
logic into a single walkNodeOptions(existing string, mode heapCapMode) function
(define heapCapMode with values like heapCapStrip and heapCapNormalize) and move
the shared empty-tokens guard, dropInjectedHeapCap handling, require/inline
require detection, and default append into it; inside the walker branch on mode
to either strip both "--max-old-space-size" forms (heapCapStrip) or normalize
the space form into "--max-old-space-size=NNN" (heapCapNormalize), then make
cleanedNodeOptions and nodeOptionsForRestore thin wrappers that call
walkNodeOptions with the appropriate mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 828d2280-8b56-4e3d-952a-01c758f752b5
📒 Files selected for processing (6)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.pytests/test_cli_claude_teams_env.py
…ns-require-tmpdir-stale
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 18999-19023: The loop currently unconditionally drops a split-form
"--max-old-space-size <value>" pair; change that behavior so the pair is only
removed when it was the cmux-injected cap. In the token branch that matches
token == "--max-old-space-size", check shouldDropInjectedHeapCap and call
isInjectedNodeHeapCap(tokens, index: index) (or use nodeHeapCapWidth) to confirm
the following value is the injected cap (e.g., "4096"); if both true, consume
both tokens (index += min(2, tokens.count - index)), clear
shouldDropInjectedHeapCap and continue, otherwise do not consume the value
(advance only past the flag or leave handling to the normal flow) so
user-supplied heap limits are preserved. Ensure you reference and update the
same symbols: shouldDropInjectedHeapCap, isInjectedNodeHeapCap(tokens, index:),
nodeHeapCapWidth(tokens, index:), and the "--max-old-space-size" token check.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12263ec5-9533-4c42-9a23-a791d42aaf20
📒 Files selected for processing (6)
CLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchEnvironmentPolicy.swiftResources/bin/claudecmuxTests/SessionPersistenceTests.swiftdaemon/remote/cmd/cmuxd-remote/agent_launch.gotests/test_claude_wrapper_hooks.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
18997-19029: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract the
NODE_OPTIONSwalker into one helper.Lines 18997-19029 and Lines 19038-19072 now carry the same stateful token-walk logic in two places. That makes the sanitize-on-launch and sanitize-for-restore paths easy to drift apart the next time this parser changes, which is exactly the class of bug this PR is trying to eliminate. Please have both call sites share a single walker and keep the caller-specific
String/String?handling outside it.Also applies to: 19038-19072
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 18997 - 19029, The duplicated stateful token-walking logic that builds a filtered token list (uses variables tokens, index, shouldDropInjectedHeapCap and helper checks isInjectedNodeHeapCap, nodeHeapCapWidth, isRequireOption, inlineRequireOptionPath, isCmuxNodeOptionsRestoreModulePath) should be extracted into a single helper function (e.g. walkAndFilterNodeOptions(tokens: [String]) -> [String]) that encapsulates the while-loop and all token-mutating behavior; replace both duplicate blocks with calls to this helper and keep any caller-specific conversion to String or String? (joining with " " or returning nil/optional) outside the helper so the walker only returns a neutral [String] result for callers to format as needed. Ensure the helper preserves the exact semantics around dropping injected heap caps and rewriting "--max-old-space-size" to "--max-old-space-size=<value>" by using the existing helper predicates (isInjectedNodeHeapCap, nodeHeapCapWidth, isRequireOption, inlineRequireOptionPath, isCmuxNodeOptionsRestoreModulePath).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@daemon/remote/cmd/cmuxd-remote/agent_launch.go`:
- Around line 337-343: claudeNodeOptionsRestoreDirs() can produce a path with
spaces which breaks mergeNodeOptions and walkNodeOptions that use simple
whitespace tokenization; to fix, filter out any candidate paths containing
whitespace in claudeNodeOptionsRestoreDirs() (skip adding home-based path when
strings.ContainsAny(home, " \t\n") is true) so mergeNodeOptions and
walkNodeOptions continue to work unchanged, and add a short comment referencing
mergeNodeOptions and walkNodeOptions so future changes consider quote-aware
parsing if you later need to support spaces.
In `@Resources/bin/claude`:
- Around line 193-244: normalize_node_options_for_restore() breaks paths with
spaces because merge_node_options() emits unquoted --require=$guard_path and
normalize_node_options_for_restore() uses read -r -a (whitespace split). Fix by
either (A) excluding any node_options_restore_dir_candidates() entries that
contain spaces so merge_node_options() never emits those, or (B) make both sides
quote-aware: in merge_node_options() emit the require flag with a shell-safe
quoted value (e.g., use printf '%q' or emit --require="$guard_path" style
quoting when constructing NODE_OPTIONS), and in
normalize_node_options_for_restore() replace the simple read-splitting with a
quote-aware tokenizer (e.g., evaluate the string into an array with shell
word-splitting that preserves quoted segments or use eval to populate tokens) so
--require="path with spaces" stays a single token; update references to
guard_path, merge_node_options, and normalize_node_options_for_restore
accordingly.
In `@tests/test_claude_wrapper_hooks.py`:
- Line 961: The test was renamed to
test_live_socket_preserves_space_separated_heap_cap but main() still calls the
old name test_live_socket_enforces_heap_cap_for_space_separated_flag, causing a
NameError; update the call inside main() to invoke
test_live_socket_preserves_space_separated_heap_cap (or revert the test name)
and ensure any references to the old function name are replaced so the test
runner calls the actual function.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 18997-19029: The duplicated stateful token-walking logic that
builds a filtered token list (uses variables tokens, index,
shouldDropInjectedHeapCap and helper checks isInjectedNodeHeapCap,
nodeHeapCapWidth, isRequireOption, inlineRequireOptionPath,
isCmuxNodeOptionsRestoreModulePath) should be extracted into a single helper
function (e.g. walkAndFilterNodeOptions(tokens: [String]) -> [String]) that
encapsulates the while-loop and all token-mutating behavior; replace both
duplicate blocks with calls to this helper and keep any caller-specific
conversion to String or String? (joining with " " or returning nil/optional)
outside the helper so the walker only returns a neutral [String] result for
callers to format as needed. Ensure the helper preserves the exact semantics
around dropping injected heap caps and rewriting "--max-old-space-size" to
"--max-old-space-size=<value>" by using the existing helper predicates
(isInjectedNodeHeapCap, nodeHeapCapWidth, isRequireOption,
inlineRequireOptionPath, isCmuxNodeOptionsRestoreModulePath).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef499cd3-ee31-442c-ac74-ba92824ebfd8
📒 Files selected for processing (6)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.pytests/test_cli_claude_teams_env.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 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="tests/test_claude_wrapper_hooks.py">
<violation number="1" location="tests/test_claude_wrapper_hooks.py:961">
P2: The test function was renamed but `main()` still calls the old name, which causes a `NameError` and aborts the regression runner.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
austinywang
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
May 18, 2026 06:59
Stale bot review: the heap-cap preservation finding was addressed by af8113e and current HEAD preserves user split-form heap options.
…-3463-node-options-require-tmpdir-stale
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7d9cfc9. Configure here.
…-3463-node-options-require-tmpdir-stale
…ns-require-tmpdir-stale
…ns-require-tmpdir-stale
lawrencecchen
added
the
stale-revisit
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
3 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.
Closes #3463
Summary
Verification
Note
Medium Risk
Touches cross-language agent launch/wrapper logic that rewrites
NODE_OPTIONS, so mistakes could break Claude/Node startup or preserve the wrong flags; changes are localized and backed by new regression tests.Overview
Fixes stale/inherited Claude
NODE_OPTIONSpreloads by persisting the restore module under~/.claude/cmux(with TMPDIR fallback, and skipping HOME paths that are empty/whitespace/contain whitespace) across the Swift CLI (claude-teams/omc), the shellclaudewrapper, andcmuxd-remote.Refactors
NODE_OPTIONShandling to sanitize prior cmux injections: detects--require/-r(including inline=...and quoted paths), drops stale restore-module preloads plus only the paired injected--max-old-space-size=4096, normalizes space-separated heap flags to--max-old-space-size=…, and only exportsCMUX_ORIGINAL_NODE_OPTIONSwhen non-empty.Adds/updates regression tests covering resume-command stripping, home/TMPDIR fallback behavior, persistence after TMPDIR cleanup, and stale-preload cleanup in Swift, Go, and Python harnesses.
Reviewed by Cursor Bugbot for commit 9c71c85. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Persist the Claude NODE_OPTIONS restore preload under
~/.claude/cmuxwith TMPDIR fallback and whitespace-HOME safeguards, and sanitize inherited NODE_OPTIONS to strip cmux restore preloads and only the cmux-injected heap cap while preserving user heap flags; fixes #3463.Bug Fixes
claude-teams), the shellclaudewrapper (at startup and before passthrough), andcmuxd-remote: remove cmux restore preloads from TMPDIR or~/.claude/cmux, handle--require/-rand inline=...(with quotes), drop only the paired injected--max-old-space-size=4096, normalize heap flags to=, and setCMUX_ORIGINAL_NODE_OPTIONSonly when non-empty.~/.claude/cmuxwith TMPDIR fallback; ignore empty or whitespace-containing HOME; survive session TMPDIR cleanup. Tests cover home/TMPDIR placement and fallback, persistence after temp cleanup, inherited stale-preload cleanup (launcher/runtime/child and passthrough), andcmuxd-remotebehavior.Refactors
Written for commit 9c71c85. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes & Improvements
Tests