Fix Claude NODE_OPTIONS restore cache path - #3541
Closed
austinywang wants to merge 43 commits into
Closed
austinywang wants to merge 43 commits into
austinywang wants to merge 43 commits into
Conversation
The restore-node-options.cjs guard module is written under
${TMPDIR}/cmux-claude-node-options/ in three call sites (the bash
wrapper, the Swift main app, and the cmuxd-remote daemon). On macOS
the launchd job com.apple.periodic-daily reaps /var/folders/.../T/
after ~3 days of no access, but NODE_OPTIONS=--require=<that path>
lives for the entire Claude session. Once the file is reaped, every
node child process spawned by Claude hooks crashes with
MODULE_NOT_FOUND, breaking long-running sessions.
The previous attempt at a fix (#2983) moved the guard to
~/Library/Application Support/cmux/node-options/, but Greptile flagged
that path contains a space, and node splits NODE_OPTIONS on whitespace,
so the --require=<path> flag is truncated at the first space and
fails immediately on every macOS launch.
This change picks a no-whitespace cache path on each platform:
- $XDG_CACHE_HOME/cmux/cmux-claude-node-options/ (when set)
- ~/Library/Caches/com.cmuxterm.app/cmux-claude-node-options/ (macOS)
- ~/.cache/cmux/cmux-claude-node-options/ (Linux fallback)
Library/Caches is the Apple-recommended location for regenerable
runtime data and is not periodically swept. The Go daemon uses
os.UserCacheDir() with a TempDir fallback. The bash wrapper's cache
selection respects XDG_CACHE_HOME first so callers (including tests)
can override the path without setting HOME.
Existing snapshot-resume tests in cmuxTests/SessionPersistenceTests.swift
intentionally still hardcode /tmp/... paths -- they verify that legacy
on-disk session snapshots get their stale --require=/tmp/... flags
stripped on resume, which remains correct.
…etic tests - daemon/remote/cmd/cmuxd-remote/agent_launch.go: on Darwin, use com.cmuxterm.app/cmux-claude-node-options to match the bash wrapper and Swift launcher so all three share one cache tree (Greptile P2, CodeRabbit nit). - Resources/bin/claude: refuse to use XDG_CACHE_HOME if it contains whitespace, falling back to the OS default. Otherwise a user's XDG_CACHE_HOME with a space would re-trigger the same node split-on-whitespace failure that sank #2983 (Greptile P2). - tests/test_claude_wrapper_hooks.py: always pin XDG_CACHE_HOME to a per-test path so tests are hermetic instead of touching the host's real ~/.cache or ~/Library/Caches when no override is provided (CodeRabbit nit).
Merged current origin/main into the PR branch and resolved the wrapper test conflict by keeping both the XDG-cache override coverage and the hook-disable coverage. The restore-module cache path is now an explicit cross-launcher contract instead of each launcher independently trusting its platform cache helper. Constraint: Node splits NODE_OPTIONS on whitespace, so --require paths must be absolute and whitespace-free. Constraint: macOS launchers must converge on ~/Library/Caches/com.cmuxterm.app/cmux-claude-node-options. Rejected: Keep honoring XDG_CACHE_HOME on macOS | it diverges from the Swift launcher and makes launch order decide the shim location. Rejected: Accept relative XDG_CACHE_HOME values | Node preload resolution would become cwd-dependent. Confidence: high Scope-risk: narrow Tested: git diff --check Tested: ./scripts/reload.sh --tag fix-node-options-restore-cache-dir --launch Not-tested: Local test suites per repo policy; CI pending after push. Co-authored-by: OmX <omx@oh-my-codex.dev>
…ode-options-restore-cache-dir
|
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:
📝 WalkthroughWalkthroughMoves the Claude ChangesPersistent Claude NODE_OPTIONS restore cache
Sequence Diagram(s)sequenceDiagram
actor TestHarness
participant Wrapper
participant CLI
participant Daemon
participant FS as Filesystem
TestHarness->>Wrapper: start wrapper (HOME/XDG overrides)
Wrapper->>CLI: request restore-module path
CLI->>Daemon: compute cache root via claudeNodeOptionsCacheDir()
Daemon->>FS: validate candidate path (whitespace/quotes, symlink, owner)
alt safe path
Daemon->>FS: mkdir (0700), write shim temp (0600), mv -> restore-node-options.cjs (0600)
else fallback needed
Daemon->>FS: ensure /var/tmp/cmux-<uid> base (no symlink, owned by uid), mkdir, write shim
end
Daemon-->>CLI: return safe require-path
CLI-->>Wrapper: inject --require=<safe path> into NODE_OPTIONS
Wrapper->>Wrapper: launch Node with injected --require
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
Greptile SummaryThis PR moves the Claude
Confidence Score: 5/5Safe to merge; all three runtimes produce the same cache root, path-safety boundary is enforced at every exit point, and the previous round's findings have been addressed. The cache-path selection logic is consistent across Swift, bash, and Go: identical fallback hierarchy, same whitespace/quote rejection predicate, and the same final defensive check before interpolating the path into NODE_OPTIONS. Symlink and ownership validation in the fallback path is correct. The shim writer now re-applies the requested mode on every code path including cache hits and the replaceItemAt branch. Test coverage spans the full platform matrix and permission re-hardening. No new logic defects were found beyond what prior review rounds already caught and the author addressed. No files require special attention at this point; CLI/CMUXCLI+ClaudeNodeOptions.swift and Resources/bin/claude carry the most security-sensitive logic but were reviewed in depth across multiple rounds. Important Files Changed
Reviews (28): Last reviewed commit: "fix: align node options fallback validat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 11633-11637: The code currently reads environment["HOME"] into
homePath and builds cacheRoot directly, which causes a hard-fail when HOME
contains whitespace; change the logic so if the resolved homePath contains any
whitespace you deterministically use the system caches directory instead (e.g.
FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first) and
then append "com.cmuxterm.app" to that URL, leaving the original behavior when
HOME is whitespace-free; apply the same fix for the other occurrence referenced
(the block around the symbols assigning cacheRoot at the second site).
In `@Resources/bin/claude`:
- Line 177: The guard that checks for whitespace in the guard_dir variable
currently does a silent "return 1" when whitespace is found; update that check
to print a short diagnostic to stderr (e.g., using printf or echo to >&2) before
returning so callers can see why NODE_OPTIONS injection was skipped; modify the
line with [[ "$guard_dir" != *[[:space:]]* ]] || return 1 to instead emit a
warning like "echo 'warning: guard_dir contains whitespace; skipping
NODE_OPTIONS injection' >&2" and then return 1, referencing the guard_dir
variable in the message for clarity.
🪄 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: cb546bf1-dcd5-4384-ac4a-d485a0d7b266
📒 Files selected for processing (5)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.py
Reviewer feedback exposed a remaining representable bad state: the preferred persistent cache root could still inherit whitespace from HOME, making Node split the --require path. This keeps the preferred platform cache path when it is safe, but gives all launchers the same deterministic /var/tmp/cmux fallback when the preferred path would be unsafe. Constraint: NODE_OPTIONS --require paths are split on whitespace by Node and cannot be quoted through this environment variable. Constraint: Normal macOS homes must still use ~/Library/Caches/com.cmuxterm.app/cmux-claude-node-options. Rejected: Use FileManager user caches as the fallback for spaced HOME | it resolves under the same spaced home and does not remove the unsafe state. Confidence: high Scope-risk: narrow Directive: Keep Swift, bash, and Go cache-root fallback rules aligned whenever this path contract changes. Tested: git diff --check Tested: ./scripts/reload.sh --tag fix-node-options-restore-cache-dir --launch Not-tested: Local test suites per repo policy; CI will validate pushed branch. Co-authored-by: OmX <omx@oh-my-codex.dev>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Resources/bin/claude (2)
190-204: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMode
0644on the restore module is correct, but consider0600to harden against tampering on the/var/tmpfallback.
restore-node-options.cjsis--require-loaded into the Claude process, so any user that can write to it gains code execution in Claude. On the per-user cache (~/Library/Caches/...,~/.cache/...) the parent dir already restricts access, so0644is fine. On the shared/var/tmp/cmux/...fallback the wider read perms are unnecessary (only the current user invokesclaude) and0600would shrink the surface a bit. Combine with the/var/tmpownership concern flagged separately.🤖 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 `@Resources/bin/claude` around lines 190 - 204, Change the file-permission bit used when creating the restore module from 0644 to 0600 to reduce tampering risk (update the chmod invocation that currently reads chmod 0644 "$temp_path" || { ... } to chmod 0600 "$temp_path" || { ... }); keep the existing failure cleanup (rm -f "$temp_path") and return behavior unchanged so the temporary restore-node-options.cjs remains writable/readable only by the creating user, mitigating the /var/tmp fallback exposure.
188-190:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
/var/tmp/cmuxfallback creates unmitigated RCE risk — both shell wrapper and Go daemon lack ownership validation.Because
NODE_OPTIONS=--require=$guard_pathcauses Node to execute whatever sits at that path, an attacker can pre-create/var/tmp/cmux(world-writable on Linux with sticky bit) owned by themselves before this script runs. Both implementations will then:
- Succeed silently with
mkdir -p(no error if dir already exists, even if owned by another user)- Write the restore module to the attacker-controlled directory
- Have Claude load and execute the attacker's code
Neither the shell wrapper (
Resources/bin/claude, lines 188–190) nor the Go daemon (agent_launch.go,claudeNodeOptionsCacheRoot(), lines 354–375) validates directory ownership before using the fallback.Fix by one of:
- Check ownership: refuse the fallback if
/var/tmp/cmuxexists and is not owned by the current user- Namespace per-user: use
/var/tmp/cmux-$(id -u)/...and enforce0700permissions- Fail safely: skip the fallback (return error) rather than write to a potentially shared location
🤖 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 `@Resources/bin/claude` around lines 188 - 190, The fallback directory usage in Resources/bin/claude (the mkdir -p "$guard_dir" and temp_path="$(mktemp "$guard_dir/restore-node-options.cjs.XXXXXX")" logic that writes restore-node-options.cjs) is unsafe; update the script to either validate ownership or use a per-user, restricted directory: check if "$guard_dir" exists and ensure it is owned by the current UID (refuse/return error if not), or create a per-user path like "$guard_dir-$(id -u)" with mode 0700 before calling mktemp, and in all cases avoid silently writing to an existing world-writable/shared directory (i.e., if ownership check fails, return a non-zero error instead of proceeding to set NODE_OPTIONS).daemon/remote/cmd/cmuxd-remote/agent_launch.go (1)
318-352: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDead error-handling branch and swallowed
homeDirErr.
claudeNodeOptionsCacheRootnever returns a non-nil error (every branch returns a path withnil), so theif err != nilblock at lines 330–335 is unreachable. The practical consequence is that whenos.UserHomeDir()fails,homeDirErris silently dropped: the function falls through to/var/tmp/cmux, writes the module there, and the user gets no diagnostic about why their per-user cache wasn't used. Either changeclaudeNodeOptionsCacheRootto return(string, error)meaningfully (propagatinghomeDirErrwhen it had to fall back), or drop theerrorreturn entirely and surfacehomeDirErrhere as a warning.♻️ Proposed simplification
- homeDir, homeDirErr := os.UserHomeDir() - if homeDirErr != nil { - homeDir = "" - } - cacheRoot, err := claudeNodeOptionsCacheRoot(runtime.GOOS, os.Getenv("XDG_CACHE_HOME"), homeDir) - if err != nil { - if homeDirErr != nil { - return "", fmt.Errorf("could not determine home directory for NODE_OPTIONS cache: %w", homeDirErr) - } - return "", err - } + homeDir, homeDirErr := os.UserHomeDir() + if homeDirErr != nil { + homeDir = "" + fmt.Fprintf(os.Stderr, "cmux: warning: could not determine home directory for NODE_OPTIONS cache: %v; using /var/tmp fallback\n", homeDirErr) + } + cacheRoot := claudeNodeOptionsCacheRoot(runtime.GOOS, os.Getenv("XDG_CACHE_HOME"), homeDir)And drop the
errorreturn onclaudeNodeOptionsCacheRoot(update test expectations accordingly).🤖 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 318 - 352, The function ensureClaudeNodeOptionsRestoreModule currently swallows homeDirErr because claudeNodeOptionsCacheRoot always returns a nil error; fix by making claudeNodeOptionsCacheRoot return only (string) (remove the error return) and then explicitly check homeDirErr in ensureClaudeNodeOptionsRestoreModule: if homeDirErr != nil return "", fmt.Errorf("could not determine home directory for NODE_OPTIONS cache: %w", homeDirErr) before proceeding to use cacheRoot; update any callers/tests of claudeNodeOptionsCacheRoot accordingly so the dead error-handling branch is removed and the original homeDirErr is surfaced.
🤖 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 11543-11547: ClaudeNodeOptionsCachePathError currently returns a
static errorDescription which hides which path failed; update the
ClaudeNodeOptionsCachePathError struct to accept the offending path (e.g., a
String or URL) and include that path in errorDescription, then update each throw
site (the places that currently throw ClaudeNodeOptionsCachePathError when
validating HOME, XDG_CACHE_HOME, or the /var/tmp fallback) to instantiate
ClaudeNodeOptionsCachePathError with the actual root.path/fallback string so the
error message surfaces the exact rejected path.
- Around line 11673-11683: The fallback cache root in
claudeNodeOptionsFallbackCacheRoot currently uses a shared /var/tmp/cmux path
which allows cross-user race/symlink attacks; change
claudeNodeOptionsFallbackCacheRoot to append a per-user unique directory (e.g.,
based on getuid() or the current user's UID) so the path becomes
/var/tmp/cmux/<uid>/... instead of a global directory, create that per-user
directory with restrictive permissions (mode 0700), and before reusing it verify
it is owned by the current UID and is not a symlink; apply the identical per-UID
directory scheme and ownership/permission checks in the corresponding writers in
Resources/bin/claude and daemon/remote/cmd/cmuxd-remote/agent_launch.go so all
three components agree on the safe path.
In `@daemon/remote/cmd/cmuxd-remote/agent_launch.go`:
- Around line 340-343: The whitespace check for dir duplicates the guard already
performed by claudeNodeOptionsCacheRoot; either remove this unreachable
duplicate (the pathContainsWhitespace check around dir) or move/document the
responsibility to a single place: keep the validation inside
claudeNodeOptionsCacheRoot and delete the extra check here, or retain it but add
a clear comment stating it is defensive redundancy; update references to the dir
variable and the pathContainsWhitespace call in agent_launch.go accordingly so
only one canonical validation exists.
- Around line 354-375: The Linux/non-darwin fallback in
claudeNodeOptionsCacheRoot currently returns "/var/tmp/cmux", which causes the
caller (which appends "cmux" + "cmux-claude-node-options") to produce a doubled
"cmux/cmux" segment; change the non-darwin fallback return to "/var/tmp" (i.e.,
return the parent temp dir) so that subsequent joins yield
"/var/tmp/cmux/cmux-claude-node-options" and align with the bash wrapper and
Swift CLI; keep the existing whitespace checks and use the same helpers
(pathContainsWhitespace) and branch structure, only adjust the final return
value for the non-darwin case.
In `@daemon/remote/cmd/cmuxd-remote/tmux_compat_test.go`:
- Around line 416-464: Add a test case to TestClaudeNodeOptionsCacheRoot that
covers the degenerate input claudeNodeOptionsCacheRoot("linux", "", "") (both
XDG_CACHE_HOME empty and homeDir empty) and assert it returns the fallback
filepath.Join("/var", "tmp", "cmux"); locate the test function
TestClaudeNodeOptionsCacheRoot and append a block creating linuxEmptyRoot, err
:= claudeNodeOptionsCacheRoot("linux", "", "") with the usual err check and a
comparison expecting "/var/tmp/cmux" to lock in current fallback behavior.
- Around line 416-464: The TestClaudeNodeOptionsCacheRoot contains six
sequential checks that should be converted into a table-driven set of subtests
to isolate failures and make output clearer: create a slice of test cases (name,
goos, xdg, home, want, expectError) and iterate with for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) { got, err :=
claudeNodeOptionsCacheRoot(tc.goos, tc.xdg, tc.home); check error vs
tc.expectError; compare got to tc.want with t.Fatalf on mismatch }) }; keep the
existing expected strings and reuse the same claudeNodeOptionsCacheRoot
reference so each scenario (mac, mac fallback, linux XDG, linux fallback, linux
relative fallback, linux unsafe home fallback) is a separate subtest and can be
run/filtered individually.
In `@Resources/bin/claude`:
- Around line 165-186: The final whitespace re-check (the if test matching
"$guard_dir" == *[[:space:]]*) is effectively unreachable because every branch
that sets guard_dir already normalizes or falls back to a whitespace-free path;
add a short comment immediately above that if-statement explaining it's a
defensive sanity check (defense-in-depth) to catch future branches that might
forget whitespace handling, referencing the guard_dir variable and the
"*[[:space:]]*" pattern so readers understand why it exists and that current
branches guarantee no hit.
In `@tests/test_claude_wrapper_hooks.py`:
- Line 771: The test runner calls an undefined function
test_live_socket_whitespace_cache_path_skips_node_options_injection in main();
replace that call with the correct existing test name
test_live_socket_whitespace_home_uses_safe_cache_fallback (or define the missing
function if intended) so main() invokes a defined test; update the invocation in
the main() test list to refer to
test_live_socket_whitespace_home_uses_safe_cache_fallback (or add a new test
function with the expected name) to eliminate the NameError.
---
Outside diff comments:
In `@daemon/remote/cmd/cmuxd-remote/agent_launch.go`:
- Around line 318-352: The function ensureClaudeNodeOptionsRestoreModule
currently swallows homeDirErr because claudeNodeOptionsCacheRoot always returns
a nil error; fix by making claudeNodeOptionsCacheRoot return only (string)
(remove the error return) and then explicitly check homeDirErr in
ensureClaudeNodeOptionsRestoreModule: if homeDirErr != nil return "",
fmt.Errorf("could not determine home directory for NODE_OPTIONS cache: %w",
homeDirErr) before proceeding to use cacheRoot; update any callers/tests of
claudeNodeOptionsCacheRoot accordingly so the dead error-handling branch is
removed and the original homeDirErr is surfaced.
In `@Resources/bin/claude`:
- Around line 190-204: Change the file-permission bit used when creating the
restore module from 0644 to 0600 to reduce tampering risk (update the chmod
invocation that currently reads chmod 0644 "$temp_path" || { ... } to chmod 0600
"$temp_path" || { ... }); keep the existing failure cleanup (rm -f "$temp_path")
and return behavior unchanged so the temporary restore-node-options.cjs remains
writable/readable only by the creating user, mitigating the /var/tmp fallback
exposure.
- Around line 188-190: The fallback directory usage in Resources/bin/claude (the
mkdir -p "$guard_dir" and temp_path="$(mktemp
"$guard_dir/restore-node-options.cjs.XXXXXX")" logic that writes
restore-node-options.cjs) is unsafe; update the script to either validate
ownership or use a per-user, restricted directory: check if "$guard_dir" exists
and ensure it is owned by the current UID (refuse/return error if not), or
create a per-user path like "$guard_dir-$(id -u)" with mode 0700 before calling
mktemp, and in all cases avoid silently writing to an existing
world-writable/shared directory (i.e., if ownership check fails, return a
non-zero error instead of proceeding to set NODE_OPTIONS).
🪄 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: 8dabb062-fa0e-4911-b43b-268d9dce8db5
📒 Files selected for processing (5)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.py
The whitespace-safe /var/tmp fallback must not become a shared writable code-loading location. This changes the fallback contract to a per-UID private directory, validates ownership before use, fixes the Go full-dir assembly, and wires the renamed wrapper regression into the test runner. Constraint: NODE_OPTIONS --require executes the restore module in Claude subprocesses, so fallback directories must not be attacker-owned or shared. Constraint: Swift, bash, and Go must continue to agree on the final restore-module path shape for each platform. Rejected: Keep /var/tmp/cmux as a global fallback | allows pre-created shared directories to influence a code-loaded path. Confidence: high Scope-risk: narrow Directive: Future fallback changes must preserve per-user ownership checks and avoid root-plus-subdir recomposition drift. Tested: git diff --check Tested: ./scripts/reload.sh --tag fix-node-options-restore-cache-dir --launch Not-tested: Local test suites per repo policy; CI will validate pushed branch. Co-authored-by: OmX <omx@oh-my-codex.dev>
The per-user fallback refactor used named return values in claudeNodeOptionsCacheDir, so reassignment must use = instead of :=. This keeps the path contract unchanged while restoring the remote daemon test build. Constraint: Keep the prior security fix intact; this is only a compile correction. Confidence: high Scope-risk: narrow Tested: git diff --check Tested: ./scripts/reload.sh --tag fix-node-options-restore-cache-dir --launch Not-tested: Local test suites per repo policy; CI remote-daemon-tests will validate. Co-authored-by: OmX <omx@oh-my-codex.dev>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resources/bin/claude (1)
232-254:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnhandled
catwrite failure can silently overwrite the guard module with an empty file.If the heredoc write fails (e.g., disk-full),
catexits non-zero but there is no error check, so execution continues.chmod 0600 "$temp_path"succeeds on the empty file,cmp -ssees a size mismatch with the existing$guard_path, andmv -f "$temp_path" "$guard_path"replaces the valid module with an empty file. Claude then runs with a broken--require=<path>for the rest of the session.🐛 Proposed fix
- cat >"$temp_path" <<'EOF' + if ! cat >"$temp_path" <<'EOF' const hadOriginalNodeOptions = process.env.CMUX_ORIGINAL_NODE_OPTIONS_PRESENT === "1"; if (hadOriginalNodeOptions) { process.env.NODE_OPTIONS = process.env.CMUX_ORIGINAL_NODE_OPTIONS ?? ""; } else { delete process.env.NODE_OPTIONS; } delete process.env.CMUX_ORIGINAL_NODE_OPTIONS; delete process.env.CMUX_ORIGINAL_NODE_OPTIONS_PRESENT; EOF + then + rm -f "$temp_path" + return 1 + fi🤖 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 `@Resources/bin/claude` around lines 232 - 254, The heredoc write to temp_path can fail and leave an empty file that gets moved over guard_path; fix this by checking the exit status of the cat heredoc write (the block that writes to "$temp_path") and if it fails remove "$temp_path" and return 1 before continuing; keep the existing cleanup pattern used after chmod (i.e., on failure rm -f "$temp_path" and return 1) and apply it immediately after the heredoc write so the subsequent chmod/cmp/mv steps never run on a failed write involving temp_path.
🤖 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 11645-11649: The code uses environment["HOME"] ??
NSHomeDirectory() which treats an empty HOME as set; change the logic to treat
an empty string as unset so the fallback NSHomeDirectory() is used (e.g., check
environment["HOME"] for nil or empty before using it) and apply the same change
where the same pattern appears (the other branch around the second occurrence).
Update the code that computes homePath and any uses that derive preferredRoot so
they only use environment["HOME"] when it's non-empty.
In `@daemon/remote/cmd/cmuxd-remote/tmux_compat_test.go`:
- Around line 416-491: Add a table case to TestClaudeNodeOptionsCacheDir to
cover the darwin branch when homeDir == "": update the tests slice in
TestClaudeNodeOptionsCacheDir by inserting a case named like "mac empty home
fallback" with goos "darwin", xdgCacheHome "", homeDir "", wantDir
filepath.Join("/var/tmp", "cmux-501", "com.cmuxterm.app",
"cmux-claude-node-options") and wantFallback filepath.Join("/var/tmp",
"cmux-501"); this will explicitly exercise the fallback path in
claudeNodeOptionsCacheDir for mac when no HOME is available.
In `@Resources/bin/claude`:
- Around line 164-166: The ownership check in node_options_path_owner_uid() must
try GNU-format first then BSD-format; change the command to run stat -c '%u'
"$1" 2>/dev/null || stat -f '%u' "$1" 2>/dev/null so that on Linux the -c format
is used and on BSD/macOS the -f fallback runs; ensure stderr is still redirected
to /dev/null for both calls.
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 582-593: The linter flags S108 for the two assertions that compare
require_path to a literal "/var/tmp/cmux-..." even though they are not
file-creation calls; update the two equality-check lines in
tests/test_claude_wrapper_hooks.py (the expect(...) calls that compare
require_path for macOS and non-macOS) to append a noqa suppression comment (#
noqa: S108) to each literal-containing expression so Ruff will ignore the false
positive for those require_path comparisons.
---
Outside diff comments:
In `@Resources/bin/claude`:
- Around line 232-254: The heredoc write to temp_path can fail and leave an
empty file that gets moved over guard_path; fix this by checking the exit status
of the cat heredoc write (the block that writes to "$temp_path") and if it fails
remove "$temp_path" and return 1 before continuing; keep the existing cleanup
pattern used after chmod (i.e., on failure rm -f "$temp_path" and return 1) and
apply it immediately after the heredoc write so the subsequent chmod/cmp/mv
steps never run on a failed write involving temp_path.
🪄 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: 8c29a0b6-1443-49c0-ae3b-e5ddc9cfd427
📒 Files selected for processing (5)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.py
The restore preload is injected through NODE_OPTIONS without shell quoting, so cache directory selection must reject every path character that Node treats as option syntax. The fallback owner probe also now tries GNU stat first so Linux fallback ownership checks do not capture failed BSD-probe stdout. Constraint: NODE_OPTIONS --require paths are assembled as a single unquoted option value. Rejected: Quote the --require path in NODE_OPTIONS | Node parses NODE_OPTIONS itself and quote handling is the source of this failure class. Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag fix-node-options-restore-cache-dir; bash -n Resources/bin/claude; python3 -m py_compile tests/test_claude_wrapper_hooks.py; git diff --check Not-tested: Local regression tests were not executed per repository testing policy; CI should run the Python wrapper and remote daemon coverage.
CodeRabbit caught two follow-up issues on the parser-safe path change: Swift treated an exported empty HOME as a real cache root input, and the Python assertions now intentionally contain /var/tmp literals that Ruff flags as S108. Constraint: Swift, bash, and Go cache-root selection should agree for empty or unsafe HOME/XDG inputs. Rejected: Remove the /var/tmp assertions | they lock the per-UID fallback path that prevents shared-directory preload attacks. Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag fix-node-options-restore-cache-dir; python3 -m py_compile tests/test_claude_wrapper_hooks.py; git diff --check Not-tested: Local test execution was skipped per repository testing policy; CI will run the relevant lint/test jobs.
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)
Resources/bin/claude (1)
251-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCached
guard_pathpermissions are not re-tightened to 0600 on the cmp-match path.When
guard_pathalready exists from a prior install andcmp -smatches, we justrm -f "$temp_path"and return — the existing file keeps whatever mode it had (e.g., a 0644 file written by an earlier version of this wrapper before the 0600 hardening). The Go counterpart inagent_launch.goalways runsos.Chmod(restoreModulePath, 0600)afterwriteShimIfChanged, so bash diverges from Go on cache hits.🔒 Proposed fix: enforce 0600 on the cached file too
if [[ -f "$guard_path" ]] && cmp -s "$temp_path" "$guard_path"; then rm -f "$temp_path" || return 1 + chmod 0600 "$guard_path" || return 1 else mv -f "$temp_path" "$guard_path" || { rm -f "$temp_path" return 1 } fi🤖 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 `@Resources/bin/claude` around lines 251 - 258, When the existing cached file at guard_path matches temp_path (the cmp -s branch) we currently just rm -f temp_path and leave guard_path's mode as-is; change that to enforce chmod 0600 on guard_path before removing temp_path and return a non-zero status if the chmod fails. In other words, in the cmp-match branch (the if [[ -f "$guard_path" ]] && cmp -s "$temp_path" "$guard_path" block) run chmod 0600 "$guard_path" and check its exit code (return 1 on failure) prior to rm -f "$temp_path", mirroring the hardening behavior applied after the mv -f path.
♻️ Duplicate comments (2)
CLI/cmux.swift (1)
11652-11652:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
HOME=""as unset in both cache-root branches.Line 11652 and Line 11670 still use
environment["HOME"] ?? NSHomeDirectory(). An exported emptyHOMEslips through here, andURL(fileURLWithPath: "")can resolve relative to the current working directory instead of the user cache. That makes the Swift path selection diverge from the Go/bash implementations this PR is trying to align with.Suggested fix
- let homePath = environment["HOME"] ?? NSHomeDirectory() + let homePath = environment["HOME"].flatMap { $0.isEmpty ? nil : $0 } ?? NSHomeDirectory() @@ - let homePath = environment["HOME"] ?? NSHomeDirectory() + let homePath = environment["HOME"].flatMap { $0.isEmpty ? nil : $0 } ?? NSHomeDirectory()Also applies to: 11670-11670
🤖 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` at line 11652, The code uses environment["HOME"] ?? NSHomeDirectory() which treats an empty HOME as set; change the logic in both cache-root branches where homePath is computed (the variable named homePath and the code paths handling cache root selection) to treat an empty string as unset — i.e., if environment["HOME"] is nil or empty, fall back to NSHomeDirectory(); otherwise use the non-empty environment value to construct the URL so URL(fileURLWithPath: "") is never created from an empty HOME.tests/test_claude_wrapper_hooks.py (1)
614-614:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuppress Ruff
S108false positives on/var/tmpassertion literals.Line 614, Line 620, and Line 648 are equality assertions (not temp-dir creation), but Ruff still reports
S108and can fail lint/CI.🔧 Proposed fix
- require_path == f"/var/tmp/cmux-{os.getuid()}/com.cmuxterm.app/cmux-claude-node-options/restore-node-options.cjs", + require_path == f"/var/tmp/cmux-{os.getuid()}/com.cmuxterm.app/cmux-claude-node-options/restore-node-options.cjs", # noqa: S108 f"whitespace HOME fallback: expected macOS fallback cache path, got {require_path!r}", failures, ) @@ - require_path == f"/var/tmp/cmux-{os.getuid()}/cmux-claude-node-options/restore-node-options.cjs", + require_path == f"/var/tmp/cmux-{os.getuid()}/cmux-claude-node-options/restore-node-options.cjs", # noqa: S108 f"whitespace HOME fallback: expected Linux fallback cache path, got {require_path!r}", failures, ) @@ - require_path == f"/var/tmp/cmux-{os.getuid()}/cmux-claude-node-options/restore-node-options.cjs", + require_path == f"/var/tmp/cmux-{os.getuid()}/cmux-claude-node-options/restore-node-options.cjs", # noqa: S108 f"quoted HOME fallback: expected Linux fallback cache path, got {require_path!r}", failures, )Also applies to: 620-620, 648-648
🤖 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 `@tests/test_claude_wrapper_hooks.py` at line 614, Ruff is flagging S108 on literal "/var/tmp/..." equality assertions in the tests even though they are not creating temp dirs; update the three assertion lines that compare test variables (e.g., require_path == f"/var/tmp/cmux-{os.getuid()}/com.cmuxterm.app/cmux-claude-node-options/restore-node-options.cjs" and the two similar equality assertions) to append a per-line suppression comment ("# noqa: S108") so Ruff ignores these false positives while keeping the assertions intact.
🤖 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 `@Resources/bin/claude`:
- Around line 251-258: When the existing cached file at guard_path matches
temp_path (the cmp -s branch) we currently just rm -f temp_path and leave
guard_path's mode as-is; change that to enforce chmod 0600 on guard_path before
removing temp_path and return a non-zero status if the chmod fails. In other
words, in the cmp-match branch (the if [[ -f "$guard_path" ]] && cmp -s
"$temp_path" "$guard_path" block) run chmod 0600 "$guard_path" and check its
exit code (return 1 on failure) prior to rm -f "$temp_path", mirroring the
hardening behavior applied after the mv -f path.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Line 11652: The code uses environment["HOME"] ?? NSHomeDirectory() which
treats an empty HOME as set; change the logic in both cache-root branches where
homePath is computed (the variable named homePath and the code paths handling
cache root selection) to treat an empty string as unset — i.e., if
environment["HOME"] is nil or empty, fall back to NSHomeDirectory(); otherwise
use the non-empty environment value to construct the URL so URL(fileURLWithPath:
"") is never created from an empty HOME.
In `@tests/test_claude_wrapper_hooks.py`:
- Line 614: Ruff is flagging S108 on literal "/var/tmp/..." equality assertions
in the tests even though they are not creating temp dirs; update the three
assertion lines that compare test variables (e.g., require_path ==
f"/var/tmp/cmux-{os.getuid()}/com.cmuxterm.app/cmux-claude-node-options/restore-node-options.cjs"
and the two similar equality assertions) to append a per-line suppression
comment ("# noqa: S108") so Ruff ignores these false positives while keeping the
assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d16c39bf-d549-4889-8b85-d6719528a6b2
📒 Files selected for processing (5)
CLI/cmux.swiftResources/bin/claudedaemon/remote/cmd/cmuxd-remote/agent_launch.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests/test_claude_wrapper_hooks.py
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 `@CLI/cmux.swift`:
- Around line 11543-11548: ClaudeNodeOptionsCachePathError is used for multiple
failure causes but its errorDescription always says "unsafe for --require",
which misleads triage; update the type and its use in
claudeNodeOptionsFallbackCacheRoot/pathIsUnsafeForNodeOptions so the error
message reflects the actual rejection reason (e.g., "unsafe for --require" for
pathIsUnsafeForNodeOptions, "fallback base is a symlink" for symlink check, and
"owned by different UID" for ownership check). Add a small enum or string reason
field to ClaudeNodeOptionsCachePathError (or distinct error cases) and set it at
each call site (claudeNodeOptionsFallbackCacheRoot and
pathIsUnsafeForNodeOptions) so errorDescription returns the correct, specific
message for each failure mode.
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 605-625: Add assertions that the restored shim file and its parent
directories have hardened permissions: use os.stat and stat.S_IMODE on
require_path to assert file mode == 0o600 and on its containing directories
(os.path.dirname(require_path) and its parent) to assert dir mode == 0o700; add
these checks next to the existing expect(...) calls that validate require_path,
runtime_node_options, and child_node_options so failures surface in the same
test, and mirror the same assertions in the other fallback test block around the
640-653 region.
- Around line 210-213: The harness currently always sets env["XDG_CACHE_HOME"],
preventing tests from exercising the "unset" branch; modify the assignment in
the test setup that references env, xdg_cache_home and tmp so that when
xdg_cache_home is a special sentinel value (e.g. a constant UNSET or a specific
string marker) the code leaves env without the "XDG_CACHE_HOME" key, when
xdg_cache_home is None it sets the deterministic tmp / "xdg-cache" path, and
otherwise it sets env["XDG_CACHE_HOME"] to the provided xdg_cache_home value;
update any tests to pass that sentinel to simulate an actually missing
environment variable.
🪄 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: 4d75f3ae-b9cc-4d3f-bf3a-25cf43c39914
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_claude_wrapper_hooks.py
CodeRabbit flagged that the fallback restore-module path still had ambiguous diagnostics, an unchecked heredoc write, and missing permission coverage. This keeps the per-user fallback path while making each rejection reason specific, hardening fallback intermediate directories, and locking the wrapper expectations into executable coverage. Constraint: PR review feedback requires high and medium findings to be addressed before merge Constraint: Repo policy forbids local test execution; use syntax checks locally and CI for tests Rejected: Skip older CodeRabbit summary findings wholesale | still-valid current findings were present in inline medium feedback Confidence: medium Scope-risk: moderate Tested: bash -n Resources/bin/claude; python3 -m py_compile tests/test_claude_wrapper_hooks.py; git diff --check; gofmt on changed Go files Not-tested: Local test suites per repo policy; pending GitHub/CircleCI checks
…ode-options-restore-cache-dir
![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 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ode-options-restore-cache-dir
Contributor
Author
|
Addressed the Greptile fallback hardening finding in 2be7750: Swift now validates the /var/tmp fallback base with lstat before any chmod, and the bash wrapper now performs the post-mkdir symlink/owner checks before chmod as well. I did not run local tests per handoff instructions; CI is the validation path. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 32d60d8. Configure here.
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
4 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.
Based on #3278, with maintainer-side conflict resolution against current
origin/main.Summary
NODE_OPTIONSrestore shim to a persistent cache path instead of$TMPDIR.~/Library/Caches/com.cmuxterm.app/cmux-claude-node-options/restore-node-options.cjs.NODE_OPTIONS --require=<path>never receives paths containing whitespace.XDG_CACHE_HOME, otherwise fall back to~/.cache/cmux.Validation
git pull origin maincompleted and conflicts are resolved on this branch.git diff --checkpassed../scripts/reload.sh --tag fix-node-options-restore-cache-dir --launchpassed.Notes
a667bf3, so this same-repo PR carries the updated branch frommanaflow-ai/cmux.Note
Medium Risk
Changes how NODE_OPTIONS is built and where preload files live across Claude launch paths; mistakes could break Node startup or weaken cache permissions, but behavior is heavily tested and scoped to restore-module placement.
Overview
Fixes long-running Claude sessions breaking when macOS reaps the NODE_OPTIONS restore preload from temp dirs by writing
restore-node-options.cjsto a persistent per-user cache instead of$TMPDIR/NSTemporaryDirectory()/os.TempDir().Swift, the
Resources/bin/claudewrapper, andcmuxd-remotenow pick the same cache roots (macOS~/Library/Caches/com.cmuxterm.app/…, Linux absolute safeXDG_CACHE_HOMEor~/.cache/cmux, else/var/tmp/cmux-<uid>/…), reject paths with whitespace or quotes for unquoted--require=<path>, and harden fallbacks with 0700 dirs, 0600 module files, ownership checks, and symlink rejection. Logic moves intoCMUXCLI+ClaudeNodeOptions.swiftwith sharedCMUXCLIShimWriter; a broken$TMPDIRno longer skips injection when the cache path still works.Regression tests cover cache selection, unsafe env fallbacks, and permission re-hardening on cache hits.
Reviewed by Cursor Bugbot for commit db7a877. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Chores
Tests
Summary by cubic
Moves the Claude NODE_OPTIONS restore preload from temp dirs to a persistent, per-user cache with whitespace‑safe paths and strict permissions across Swift, bash, and Go to stop long‑running session crashes. Aligns fallback validation across launchers with owner checks and symlink rejection before chmod.
~/Library/Caches/com.cmuxterm.app/cmux-claude-node-options; Linux uses absolute, safeXDG_CACHE_HOME/cmux/…, else~/.cache/cmux/…; empty/unsafe HOME/XDG fall back to per‑UID/var/tmp/cmux-<uid>/…(Swift may use the system caches root if HOME is unsafe).--require=<path>must be free of whitespace/quotes; validate fallback base and directory chain ownership; deny symlinks; create dirs0700and module0600from the start; re‑apply modes on cache hits; guard heredoc writes; owner checks support GNU/BSDstat; avoid$TMPDIR.--require=<path>is always whitespace‑free; helpers moved toCMUXCLI+ClaudeNodeOptions.swift; shim writers accept explicit modes; tests cover path selection, fallbacks, and permission re‑hardening.Written for commit db7a877. Summary will update on new commits.