Repository navigation
Keep Claude NODE_OPTIONS restore module out of temp cleanup - #3526
austinywang wants to merge 52 commits into
Conversation
The Claude wrapper regression now runs the restore-module setup twice with a simulated live cmux socket. It deletes the first restore file before the second launch, so CI can prove the writer recreates the module and that the path is not tied to TMPDIR cleanup. Constraint: Swift tests are authored but not run locally per repository policy. Rejected: Python-only coverage | the issue asks for a Swift regression test around the restore module lifecycle. Confidence: high Scope-risk: narrow Tested: Not run locally per repository policy. Not-tested: CI execution of the new XCTest.
The restore preload is referenced by inherited NODE_OPTIONS, so it must live in storage with the same lifetime as the app session. The wrapper, Swift CLI, and remote daemon now write the module under Application Support-style cmux/node-options storage and still rewrite it on every launch path before NODE_OPTIONS is emitted. Application Support contains a space on macOS, so the generated --require value is quoted for Node's NODE_OPTIONS parser. The resume sanitizers now tokenize quoted NODE_OPTIONS values and strip both the old TMPDIR path and the new durable path. Constraint: macOS temp directories can be cleaned while Claude and child Node processes are still alive. Constraint: Local test suites are not run in this repo; verification uses syntax/static checks plus the required tagged reload build. Rejected: Caches directory | still OS-managed and can be purged under pressure. Rejected: Inline --eval bootstrap | broader launch-contract change than needed once durable storage and self-healing writes are in place. Confidence: high Scope-risk: moderate Directive: Do not move NODE_OPTIONS preload files back under TMPDIR or another purgeable directory. Tested: bash -n Resources/bin/claude; python3 -m py_compile tests/test_claude_wrapper_hooks.py tests/test_cli_claude_teams_env.py; git diff --check Not-tested: Swift/XCTest, Python integration, and Go tests locally per repository policy.
|
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:
📝 WalkthroughWalkthroughMove NODE_OPTIONS restore-module creation from ephemeral TMPDIR into a durable per-user Application Support directory and centralize tokenization, quoting, and restore-path detection into a new CMUXNodeOptions package used by Swift, Go, and the shell wrapper; update tests for quoted paths and durable restore behavior. ChangesNODE_OPTIONS Durability & Tokenization Refactor
sequenceDiagram
participant Wrapper as claude wrapper (shell)
participant CLI as cmux CLI (Swift)
participant Daemon as cmuxd agent (Go)
participant FS as Filesystem (Application Support)
participant Child as Node child processes
Wrapper->>FS: ensure restore module exists under ~/Library/Application Support/cmux/node-options
CLI->>FS: write restore-node-options.cjs via NodeOptionsSupport.claudeRestoreDirectory(...)
Daemon->>FS: ensure restore module via claudeNodeOptionsRestoreDir()
Wrapper->>Wrapper: tokenize NODE_OPTIONS (shlex/tokenizer)
CLI->>CLI: tokenize/join via NodeOptionsSupport.tokens/joinedTokens
Daemon->>Daemon: tokenize/join via nodeOptionsTokens/joinNodeOptionsTokens
Wrapper->>Child: spawn child with NODE_OPTIONS="--require=<quoted restore> --max-old-space-size=4096 ..."
Child->>FS: require restore-node-options.cjs (durable path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryMoves
Confidence Score: 5/5Safe to merge; the durable-path change is well-contained and the self-healing recreate-on-launch guard prevents the class of breakage that motivated this PR. The implementation is consistent across all three launchers. The heap-cap removal that was previously unconditional is now correctly paired to the cmux-injected require token, preserving user-supplied values. Directory preparation includes symlink checks and writability probes. The shared CMUXNodeOptions package consolidates the production tokenizer. Regression tests cover durable-path selection, stale-preload recreation, quoted paths, and resume-time sanitization. No files require special attention. Important Files Changed
Reviews (41): Last reviewed commit: "Align NODE_OPTIONS restore normalization..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 11782-11811: The file exceeds the allowed size budget because
three new pure-logic helpers (claudeNodeOptionsRestoreDirectory,
nodeOptionsRequirePath, nodeOptionsTokens) and a parallel NODE_OPTIONS tokenizer
in RestorableAgentSession duplicate reusable logic; extract these helpers and
the tokenizer into a new SwiftPM package target (e.g., SharedNodeOptions or
AgentUtils) and make CLI/cmux.swift and Sources/RestorableAgentSession.swift
depend on that package: move the implementations of
claudeNodeOptionsRestoreDirectory, nodeOptionsRequirePath, nodeOptionsTokens and
the NODE_OPTIONS tokenizer into the package, update both callers to import the
new module, and remove the duplicated code from the app target so CI file-length
budget is satisfied.
- Around line 15396-15406: The function nodeOptionsRequirePath currently builds
charactersRequiringQuotes from whitespace, backslash and double-quote only, so
paths containing a single-quote (apostrophe) are not wrapped and later break
tokenization; update nodeOptionsRequirePath to include the single-quote
character (') in the charactersRequiringQuotes set so any path with an
apostrophe is quoted, keep the existing escaping logic (no special escaping
needed for a literal apostrophe inside the surrounding double quotes) and return
the quoted/escaped string as before to ensure nodeOptionsTokens and
isInjectedNodeOptionsRequire continue to work correctly.
🪄 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: 090e0d61-73a8-498a-87d3-2c9cf4accf6d
📒 Files selected for processing (9)
CLI/cmux.swiftResources/bin/claudeSources/RestorableAgentSession.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SessionPersistenceTests.swiftdaemon/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
👮 Files not reviewed due to content moderation or server errors (7)
- cmuxTests/SessionPersistenceTests.swift
- daemon/remote/cmd/cmuxd-remote/tmux_compat_test.go
- daemon/remote/cmd/cmuxd-remote/agent_launch.go
- Sources/RestorableAgentSession.swift
- Resources/bin/claude
- cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift
- tests/test_cli_claude_teams_env.py
CodeRabbit and the workflow guard both flagged that the fix duplicated NODE_OPTIONS tokenization between the CLI and app resume path while also pushing CLI/cmux.swift over its size budget. Move the shared parsing, quoting, durable restore-directory resolution, and restore-module path detection into a small SwiftPM package consumed by the app, CLI, and regression tests. Also quote restore paths containing apostrophes so tokenizer round-trips remain valid. Constraint: CI enforces Swift file/package boundary budgets for CLI/cmux.swift. Rejected: Keep a second private tokenizer in RestorableAgentSession.swift | this leaves future parser fixes split across production targets. Confidence: high Scope-risk: narrow Directive: Keep NODE_OPTIONS parsing and restore-module path detection in CMUXNodeOptions when adding new Swift callers. Tested: bash -n Resources/bin/claude; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; python3 -m py_compile tests/test_claude_wrapper_hooks.py tests/test_cli_claude_teams_env.py; swift package describe --package-path Packages/CMUXNodeOptions; swift package describe --type json; git diff --check Not-tested: Local Swift, Go, and Python integration tests per repository policy; CI will run them.
The workflow guard tracks growth in large Swift files, and the new regressions crossed two existing file budgets. Split the wrapper and resume NODE_OPTIONS coverage into focused test files under the threshold while leaving the behavior assertions unchanged. Constraint: workflow-guard-tests enforces .github/swift-file-length-budget.tsv. Confidence: high Scope-risk: narrow Directive: Add new regression coverage in focused files when adjacent test files are already budget-constrained. Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; git diff --check Not-tested: Local Swift test execution per repository policy; CI will run the test suite.
Quoted NODE_OPTIONS values can contain whitespace and words that look like flags. The launchers need to tokenize the environment value as an option string, not as raw whitespace fields, before filtering their own heap cap. Constraint: Review feedback identified the Go and bash launch paths as still using whitespace splitting. Rejected: Treat Application Support as a special case | quoted NODE_OPTIONS can contain arbitrary user paths and escaped characters. Confidence: high Scope-risk: narrow Directive: Keep NODE_OPTIONS filtering quote-aware in every launcher surface. Tested: Not run locally per repository testing policy.
The Go remote daemon and bash wrapper now parse NODE_OPTIONS with quote and backslash awareness before filtering cmux's heap cap, then re-quote tokens when writing the value back. This keeps existing quoted require paths intact across every launcher surface. Constraint: Cursor Bugbot found the non-Swift launchers still used whitespace splitting after the Swift path moved to quote-aware tokenization. Rejected: Special-case Application Support paths | NODE_OPTIONS can contain arbitrary quoted user paths, so the parser owns the invariant. Confidence: high Scope-risk: narrow Directive: Do not use strings.Fields or read -a for NODE_OPTIONS filtering; preserve quoted option tokens by construction. Tested: gofmt; bash -n Resources/bin/claude; python3 -m py_compile tests/test_claude_wrapper_hooks.py; git diff --check Not-tested: Runtime Go/Python regression tests not run locally per repository testing policy.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Packages/CMUXNodeOptions/Sources/CMUXNodeOptions/NodeOptionsSupport.swift`:
- Around line 84-92: The function isCmuxRestoreModulePath currently uses
path.contains(...) which matches substrings and can false-positive; change it to
inspect trailing path components instead. After confirming lastPathComponent ==
restoreModuleFilename, build the standardized URL's pathComponents (from the
same URL used to get path) and check that the components' suffix equals the
exact sequences for managed installs (e.g. ["cmux-claude-node-options",
restoreModuleFilename] or ["cmux","node-options", restoreModuleFilename]) rather
than using contains; adjust isCmuxRestoreModulePath to return true only when one
of those suffix matches.
In `@Resources/bin/claude`:
- Around line 126-135: The node_options_restore_dir function returns
CMUX_NODE_OPTIONS_RESTORE_DIR verbatim which can be relative or start with ~ and
cause MODULE_NOT_FOUND; modify node_options_restore_dir to expand a leading ~ to
$HOME and then convert any non-absolute path into an absolute path (e.g. via
realpath/readlink -f if available, otherwise prefix with the current working
directory and normalize) before trimming a trailing slash and returning it; if
expansion/absolutization fails, return non-zero so callers know the override is
invalid.
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 47-51: The function restore_require_and_remaining uses " ".join
which loses original quoting (causing split_node_options to mis-parse paths with
spaces); change restore_require_and_remaining to reconstitute the remaining
flags using shlex.join (or equivalent shlex.quote join) instead of " ".join so
quoted tokens keep their quotes; update references to split_node_options and any
callers expecting the original quoting preserved (e.g., in tests like
test_live_socket_preserves_quoted_existing_require_path) to use the output from
restore_require_and_remaining unchanged.
🪄 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: 6a4f9be5-9242-4081-9c7b-fe840a511538
📒 Files selected for processing (13)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojPackage.swiftPackages/CMUXNodeOptions/Package.swiftPackages/CMUXNodeOptions/Sources/CMUXNodeOptions/NodeOptionsSupport.swiftResources/bin/claudeSources/RestorableAgentSession.swiftcmuxTests/AgentResumeNodeOptionsTests.swiftcmuxTests/ClaudeWrapperNodeOptionsRestoreModuleTests.swiftcmuxTests/SessionPersistenceTests.swiftdaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.py
💤 Files with no reviewable changes (1)
- cmuxTests/SessionPersistenceTests.swift
The follow-up review found a few remaining boundary issues around restore-path classification, override normalization, test-side quoting, and the Xcode package link. This commit tightens those boundaries without changing the durable Application Support lifecycle decision. Constraint: Post-CI CodeRabbit and Cursor reviews requested these changes before merge Rejected: Leave override paths verbatim | relative or tilde paths can still produce MODULE_NOT_FOUND under a different child cwd Confidence: high Scope-risk: narrow Directive: Keep restore module detection component-based and keep CMUXNodeOptions linked only to targets that import it Tested: bash -n Resources/bin/claude; python3 -m py_compile tests/test_claude_wrapper_hooks.py; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; swift package describe --package-path Packages/CMUXNodeOptions; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; git diff --check Not-tested: Runtime/unit tests not run locally per repository testing policy
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 `@cmuxTests/ClaudeWrapperNodeOptionsRestoreModuleTests.swift`:
- Around line 162-185: The helper bindUnixSocket(at:) currently uses XCTAssert*
checks (socket, path length, bindResult, Darwin.listen) which only record
failures and allow execution to continue; change these to throwing guards so the
helper fails fast: replace the socket() check on fd with a guard that throws a
descriptive error if fd < 0, validate utf8.count < maxPathLength with a guard
that throws if violated, and after calling bind (bindResult) and Darwin.listen
check their return values with guards that throw on non-zero results; use the
same local names (fd, addr, utf8, maxPathLength, bindResult) and have
bindUnixSocket(at:) propagate the thrown error to callers so tests stop at the
real failure instead of continuing with invalid state.
🪄 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: 9ef07ce1-47b7-4736-a0d1-3457c374922c
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojPackages/CMUXNodeOptions/Sources/CMUXNodeOptions/NodeOptionsSupport.swiftResources/bin/claudecmuxTests/ClaudeWrapperNodeOptionsRestoreModuleTests.swifttests/test_claude_wrapper_hooks.py
The latest review pass found two test-support issues: one helper still rejoined shell tokens without quoting, and the Unix socket setup helper could keep running after a setup failure. This keeps the regression scaffolding deterministic without changing production behavior. Constraint: Post-CI Cursor and CodeRabbit feedback requested these fixes before launch/merge. Rejected: Leave the XCTest assertions in setup code | assertion failures do not stop helper execution and can obscure the real setup error. Confidence: high Scope-risk: narrow Directive: Keep NODE_OPTIONS test helpers quote-aware across Python test files. Tested: python3 -m py_compile tests/test_cli_claude_teams_env.py tests/test_claude_wrapper_hooks.py; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; git diff --check; rg '" "\.join\(tokens\[1:\]\)' tests Not-tested: Runtime/unit tests not run locally per repository testing policy
Dismissed after all CodeRabbit actionable comments from this stale review were addressed and resolved in later commits; latest CodeRabbit check is passing on 09f3a01.
The NODE_OPTIONS package and the Rovo/Pasteboard package work landed on parallel branches and touched the same SwiftPM and Xcode project sections. The conflict resolution keeps the build graph target-scoped instead of choosing one side: SwiftPM receives both local packages, the app target keeps NodeOptions plus the Rovo/Pasteboard products, and both NodeOptions and Rovo test sources remain registered. Constraint: PR must merge current origin/main without dropping either branch's package products or tests Rejected: Accept either side of the project file conflict | would silently unlink the other branch's package/test additions Confidence: high Scope-risk: narrow Directive: Resolve future package metadata conflicts by target ownership and product union, not by side selection Tested: plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; swift package describe; git diff --check; git diff --cached --check; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Local Swift test suite per repository policy; CI will run tests Co-authored-by: OmX <omx@oh-my-codex.dev>
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 `@Resources/bin/claude`:
- Around line 190-225: The node_options_restore_dir function allows
CMUX_NODE_OPTIONS_RESTORE_DIR to be an arbitrary path which can produce a
restore file without the expected suffix, so change the logic in
node_options_restore_dir (the function handling CMUX_NODE_OPTIONS_RESTORE_DIR)
to ensure the returned directory always ends with a sanitizer-recognized suffix:
append or replace the tail so it ends with either "cmux/node-options"
(preferred) or the legacy "cmux-claude-node-options" if needed; perform this
normalization after expanding tildes and converting to an absolute path, strip
trailing slashes as currently done, and then ensure the final printed value
includes the required suffix so the resume/environment sanitizers can reliably
identify and strip the injected restore preloads.
🪄 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: c3a3fab2-44b6-4661-ac4e-9a59fd08b01f
📒 Files selected for processing (9)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojPackage.swiftResources/bin/claudeSources/RestorableAgentSession.swiftcmuxTests/ClaudeWrapperNodeOptionsRestoreModuleTests.swiftcmuxTests/SessionPersistenceTests.swifttests/test_claude_wrapper_hooks.pytests/test_cli_claude_teams_env.py
💤 Files with no reviewable changes (1)
- cmuxTests/SessionPersistenceTests.swift
There was a problem hiding this comment.
No issues found across 15 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
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 1edefd8. Configure here.

Summary
NODE_OPTIONSrestore preload from macOS temp storage intoLibrary/Application Support/cmux/node-optionsfor the shell wrapper, Swift CLI path, and remote daemon launcher.restore-node-options.cjsevery timeNODE_OPTIONSis computed, so a missing/deleted preload repairs itself on the next Claude launch.Why
The bug is a lifecycle mismatch: child processes inherit
NODE_OPTIONS=--require=<file>, but the old file lived under$TMPDIR, which macOS can clean while the session is still alive. Once the file disappeared, every child Node process failed to load it and flooded the session.Tests
bash -n Resources/bin/claudepython3 -m py_compile tests/test_claude_wrapper_hooks.py tests/test_cli_claude_teams_env.pygit diff --checkNODE_OPTIONSpoints atLibrary/Application Support/cmux/node-options/restore-node-options.cjs, Node runtime sees restoredNODE_OPTIONS, and deleting the file is repaired on the next wrapper invocation../scripts/reload.sh --tag issue-3512-node-options-tmpdir --launch.Note: local Swift tests were not run per repo policy; CI should run them.
Closes #3512
Note
Medium Risk
Touches Claude launch env handling across Swift CLI, shell wrapper, and remote daemon; mistakes could break agent startup or propagate incorrect
NODE_OPTIONSin user shells. Changes are well-covered by new Swift/Go/Python regression tests but still impact a critical launch path.Overview
Moves the Claude
NODE_OPTIONSrestore preload from temp directories to a durable per-user location (~/Library/Application Support/cmux/node-options), with a validated, secure temp fallback and optional override viaCMUX_NODE_OPTIONS_RESTORE_DIR.Centralizes and hardens
NODE_OPTIONSmerging/sanitization: quote-aware tokenization, correct quoting of--requirepaths (spaces/backslashes), stripping only cmux-injected restore--requireentries (legacy + durable) and the paired--max-old-space-size=4096, and preserving user-specified flags (including user heap caps).CMUX_ORIGINAL_NODE_OPTIONS*is now set only when the restored value is non-empty.Introduces a new Swift package
CMUXNodeOptionsused by the CLI andCMUXAgentLaunch, updates resume/env sanitizers accordingly, and adds focused regression tests (Swift unit tests plus expanded Go and Python wrapper/claude-teams tests) for directory selection, stale preload cleanup, and tricky quoting/escaping cases.Reviewed by Cursor Bugbot for commit 4a3aac3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Moves the Claude
NODE_OPTIONSrestore preload out of temp into a durable per‑user path and rebuilds it on each launch. Aligns quote‑aware sanitization and restore normalization across Swift, Bash, and Go so only cmux‑injected flags are removed and user options are preserved (addresses #3512).CMUX_NODE_OPTIONS_RESTORE_DIRto a managed suffix; hardened validation; component‑aware detection of current/legacy paths; remote daemon uses user config dir; resilient to bad/missingTMPDIR/HOME.CMUXNodeOptions, Bash, and Go: preserves spaces/double quotes/apostrophes/backslashes; re‑quotes on join; strips cmux--require(legacy + durable) and only the immediately paired--max-old-space-size=4096; preserves user heap caps; Bash 3.2 empty‑array guards; setsCMUX_ORIGINAL_NODE_OPTIONS*only when the normalized original is non‑empty; guard aligned across CLI and resume.Written for commit 4a3aac3. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests