Repository navigation
Conversation
|
@psh4607 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughThe wrapper now prefers storing restore-node-options.cjs under $HOME/.claude/cmux when HOME is set, falling back to $TMPDIR otherwise. Tests add a HOME override, validate guard-file placement under HOME, exercise bad-HOME behavior, and verify NODE_OPTIONS survives tmpdir cleanup. ChangesNode Options Guard File Relocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR stores the Claude
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR fixes cmux issue #3463 where the claude wrapper’s NODE_OPTIONS=--require <guard file> can become stale on macOS because the guard file was written under $TMPDIR, which may be cleared between sessions/reboots. The guard file location is moved to a persistent path under $HOME, and regression tests are updated/added to validate the new behavior end-to-end.
Changes:
- Move the NODE_OPTIONS restore guard directory from
${TMPDIR}/cmux-claude-node-optionsto${HOME}/.claude/cmux/cmux-claude-node-options(fallback to${TMPDIR:-/tmp}only if$HOMEis unset). - Sandbox
HOMEby default in the wrapper test harness and update TMPDIR-specific tests to align with the new guard file location. - Add two new behavioral regression tests covering guard file placement and survival across simulated
$TMPDIRcleanup.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Resources/bin/claude |
Writes the NODE_OPTIONS restore guard file under $HOME to avoid macOS $TMPDIR cleanup issues, with fallback when $HOME is unset. |
tests/test_claude_wrapper_hooks.py |
Sandboxes HOME, updates existing tests for the new guard path, and adds #3463 regression tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 `@tests/test_claude_wrapper_hooks.py`:
- Around line 716-766: The test only rmtree's tmp_dir /
"cmux-claude-node-options" which is a no-op for the new wrapper; to faithfully
reproduce `#3463` wipe everything under the temporary $TMPDIR after calling
run_wrapper. In the test function
test_issue_3463_node_options_survives_tmpdir_cleanup, replace the single-path
cleanup using guard_dir_tmpdir with logic that iterates tmp_dir.iterdir() and
removes each entry (file or directory) so the entire tmp_dir contents are
deleted before launching the child Node process, ensuring the guard relocation
under $TMPDIR would be caught.
🪄 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: a480bf7c-7631-4d84-856f-d898c2a10956
📒 Files selected for processing (2)
Resources/bin/claudetests/test_claude_wrapper_hooks.py
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 `@tests/test_claude_wrapper_hooks.py`:
- Around line 318-323: The test helper sets a sandboxed HOME but then calls
env.update(inherited_env) which can restore the real HOME; filter out "HOME"
from inherited_env before updating (e.g., build inherited_env_no_home = {k: v
for k, v in inherited_env.items() if k != "HOME"}) and call
env.update(inherited_env_no_home); keep the current order so setup_env(tmp) may
intentionally override HOME but prevent inherited_env from undoing the sandbox;
reference sandbox_home, env, setup_env, and inherited_env when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 954dad70-5009-4c30-a272-523d1f333a6d
📒 Files selected for processing (1)
tests/test_claude_wrapper_hooks.py
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)
tests/test_claude_wrapper_hooks.py (1)
47-67: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winKeep a way to run the wrapper with
HOMEtruly unset.
home=Nonenow means “create a fake home”, so this helper can no longer exercise the production fallback path whereHOMEis absent and the guard should land under$TMPDIR. An explicit sentinel orunset_homeflag would let you add one regression test for that branch instead of leaving it unreachable from this harness.Also applies to: 184-188
🤖 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` around lines 47 - 67, The helper currently treats home=None as “create a fake home” which prevents testing the production fallback where HOME is truly unset; add an explicit flag (e.g. unset_home: bool = False) to the helper signature and change the resolution logic so that if unset_home is True you leave HOME unset (set resolved_home to None / do not set sandbox_home), otherwise preserve the existing behavior (when home is provided use it, when home is None and unset_home is False create sandbox_home as before). Update uses of resolved_home/sandbox_home and any tests (including the branch referenced at lines ~184-188) to pass unset_home=True when you need to exercise the absent-HOME path. Ensure callers that relied on home=None keep the current fake-home behavior unless they opt into unset_home.
♻️ Duplicate comments (1)
tests/test_claude_wrapper_hooks.py (1)
318-323:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't let
inherited_envoverwrite the sandboxedHOME.
env.update(inherited_env)can restore the caller's realHOMEafter the sandbox is set, which breaks hermeticity and can pull real~/.claudestate into these auth-env tests.💡 Minimal fix
sandbox_home = tmp / "fake-home" sandbox_home.mkdir(parents=True, exist_ok=True) env["HOME"] = str(sandbox_home) if setup_env is not None: env.update(setup_env(tmp)) - env.update(inherited_env) + env.update({key: value for key, value in inherited_env.items() if key != "HOME"})🤖 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` around lines 318 - 323, The test currently sets env["HOME"] = str(sandbox_home) and then does env.update(inherited_env), which can overwrite the sandboxed HOME; to fix, ensure the sandbox HOME wins by either applying inherited_env before setting env["HOME"] or by stripping the HOME key from inherited_env (e.g. pop("HOME", None)) before env.update; adjust the code around sandbox_home, env, and setup_env so the sandboxed HOME is preserved when updating env with inherited_env.
🤖 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 `@tests/test_claude_wrapper_hooks.py`:
- Around line 47-67: The helper currently treats home=None as “create a fake
home” which prevents testing the production fallback where HOME is truly unset;
add an explicit flag (e.g. unset_home: bool = False) to the helper signature and
change the resolution logic so that if unset_home is True you leave HOME unset
(set resolved_home to None / do not set sandbox_home), otherwise preserve the
existing behavior (when home is provided use it, when home is None and
unset_home is False create sandbox_home as before). Update uses of
resolved_home/sandbox_home and any tests (including the branch referenced at
lines ~184-188) to pass unset_home=True when you need to exercise the
absent-HOME path. Ensure callers that relied on home=None keep the current
fake-home behavior unless they opt into unset_home.
---
Duplicate comments:
In `@tests/test_claude_wrapper_hooks.py`:
- Around line 318-323: The test currently sets env["HOME"] = str(sandbox_home)
and then does env.update(inherited_env), which can overwrite the sandboxed HOME;
to fix, ensure the sandbox HOME wins by either applying inherited_env before
setting env["HOME"] or by stripping the HOME key from inherited_env (e.g.
pop("HOME", None)) before env.update; adjust the code around sandbox_home, env,
and setup_env so the sandboxed HOME is preserved when updating env with
inherited_env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2f9a13c-7bbc-425d-b02d-34f589ed6b67
📒 Files selected for processing (1)
tests/test_claude_wrapper_hooks.py
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 `@tests/test_claude_wrapper_hooks.py`:
- Around line 706-715: The TMPDIR guard-file check is too narrow: change the
glob call on guard_dir_tmpdir (where stale_under_tmpdir is built) from
glob("restore-node-options.cjs") to a wider pattern (e.g. glob("*.cjs") or
glob("restore-node-options*.cjs")) so the expect(...) assertion will catch both
the final committed file and any mktemp-style temp artifacts; update the
stale_under_tmpdir construction accordingly so expect(not stale_under_tmpdir,
...) still reports leaked files.
🪄 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: 1718f88d-781a-4c01-95f9-e59563aa0294
📒 Files selected for processing (1)
tests/test_claude_wrapper_hooks.py
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@lawrencecchen @austinywang Conflicts are resolved and this PR is mergeable. Could you review and merge? |
|
This is the right approach and currently the only conflict-free PR of the several open against #3463 — #3541, #3966, #3526, #3278 are all the same durable-location fix, just rotting on merge conflicts. Consolidating on this one and merging would close the issue. Two notes for reviewers:
This has sat unreviewed while the same fix keeps getting reproposed. Could a maintainer review and merge? Happy to help rebase if it drifts. |
7480356 to
7c5ad9e
Compare
…R staleness The claude wrapper writes restore-node-options.cjs under $TMPDIR/cmux-claude-node-options/ and exports NODE_OPTIONS=--require <that path>. macOS clears $TMPDIR (/var/folders/...) between sessions and reboots, but child Node processes spawned later still inherit NODE_OPTIONS referencing the now-deleted path, producing 'Cannot find module' errors. Add two behavioral tests against the real wrapper (not source-text greps): - test_issue_3463_guard_file_lives_under_home_not_tmpdir: asserts the guard file lands under $HOME/.claude/cmux/cmux-claude-node-options/, not under $TMPDIR. - test_issue_3463_node_options_survives_tmpdir_cleanup: simulates macOS clearing $TMPDIR after the wrapper sets NODE_OPTIONS, then spawns a fresh node child with that NODE_OPTIONS and asserts no 'Cannot find module' error. Both tests fail today against the unpatched wrapper, with the 'tmpdir wipe' test reproducing the exact stack trace the issue reporter saw. Also sandbox HOME to a fresh temp dir inside run_wrapper by default, so the upcoming fix (which writes under $HOME) does not pollute the real ~/.claude tree during test runs. Refs manaflow-ai#3463
…ad of $TMPDIR The claude wrapper writes restore-node-options.cjs and exports NODE_OPTIONS=--require <that path>. macOS clears $TMPDIR (/var/folders/...) between sessions and reboots, but child Node processes spawned later still inherit NODE_OPTIONS pointing at the now-deleted file, producing 'Cannot find module' errors on session stop. Move guard_dir from $TMPDIR/cmux-claude-node-options to $HOME/.claude/cmux/cmux-claude-node-options, which is persistent across login sessions and reboots, and co-located with the existing ~/.claude/ data the wrapper already reads. Keep the existing atomic temp-then-mv pattern and cmp -s reuse-if-identical fast path. Fall back to the historical $TMPDIR location only if $HOME is somehow unset, to keep the wrapper non-fatal in unusual environments. Update two existing tests that asserted TMPDIR-specific behaviour: - Rename test_live_socket_tmpdir_failure_skips_node_options_injection to test_live_socket_home_failure_skips_node_options_injection and simulate HOME pointing at a regular file so mkdir fails (same fail-open intent). - Move the stale-mktemp-literal test fixture from $TMPDIR to $HOME so it still exercises the wrapper's mktemp path under the new layout. Closes manaflow-ai#3463
run_wrapper already auto-sandboxes $HOME so the wrapper's NODE_OPTIONS guard file ($HOME/.claude/cmux/cmux-claude-node-options/) cannot land in the developer's real home dir during test runs. The auth-env helper missed the same treatment, so the four tests that go through run_wrapper_auth_env (preserves_third_party_claude_auth_for_fresh_launch, preserves_claude_auth_for_resume_launch, preserves_only_listed_claude_auth_keys, and the subrouter-config-dir normalisation case) executed the real wrapper against the developer's real HOME and silently materialised that guard file there. Apply the same default sandbox in run_wrapper_auth_env. setup_env still overrides HOME for tests that intentionally set up a specific home tree (e.g. the subrouter normalisation case), because env.update(setup_env) runs after the default is set. Refs manaflow-ai#3463
The 'survives_tmpdir_cleanup' test only rmtree'd $TMPDIR/cmux-claude-node-options before. After the fix the wrapper never writes that path, so the rmtree is a no-op and the test only proves 'node spawns successfully with the wrapper's NODE_OPTIONS'. Wipe the whole sandboxed $TMPDIR instead, mirroring how macOS clears /var/folders/... between login sessions. If the wrapper ever regresses to writing the guard file under $TMPDIR, the --require path will be gone after the wipe and node will fail with the same 'Cannot find module' the issue reporter saw, catching the regression. Refs manaflow-ai#3463
The previous commit set HOME to a sandboxed temp dir, but env.update(inherited_env) immediately afterwards could clobber it back to the caller's real HOME. None of today's call sites pass HOME through inherited_env, but a future auth-env regression test that does would silently re-leak the wrapper's NODE_OPTIONS guard file into the dev's real ~/.claude/cmux. setup_env stays as the intentional override path (used by the subrouter normalisation case). Refs manaflow-ai#3463
The previous glob('restore-node-options.cjs') only catches the final
committed filename. Use glob('*.cjs') so the assertion also catches
any leaked mktemp temp files (restore-node-options.cjs.XXXXXX) that a
partial wrapper regression could leave under $TMPDIR/cmux-claude-node-options/.
The TMPDIR subdir is sandboxed and only the wrapper writes \.cjs files
there, so the wider glob has no false-positive risk.
Refs manaflow-ai#3463
7c5ad9e to
a88aa58
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
@debedb Thanks for pointing out the live-session case. I rebased this PR onto the latest The regression test models the live-session scenario by launching the wrapper, deleting the entire sandboxed |
|
@psh4607 Thanks — that answers it, and it answers it in the right place.
For maintainers, the state of this one:
@lawrencecchen @austinywang — could one of you take a look? This has now been open across three months and rebased twice while the same fix keeps getting reproposed by different contributors. Happy to help rebase again if it drifts. |
…30.0) (#333) The launchd-env-sync-session-leak branch carried a "Related" pointer to cmux-node-options-tmpdir-guard that never landed, because the target was only ever a personal skill under ~/.claude/skills - a shadow copy nobody else has. Adding the pointer alone would have shipped a dangling reference in a public repo, so the target comes with it. cmux launches Claude Code with a --require shim whose .cjs lives under $TMPDIR. macOS reaps files there once they go untouched for about three days, which is exactly the window a reboot-surviving cmux session lives past. The file disappears while NODE_OPTIONS still names it, and from then on every node/claude invocation in that session dies on a missing --require target - a failure that reads as a broken toolchain and arrives days after anything changed. The skill carries the stopgap for builds predating manaflow-ai/cmux#3699: a persistent copy under $HOME, and a LaunchAgent that recreates the $TMPDIR file and refreshes its mtime daily, well inside the reap window. The subtle part is why the LaunchAgent can find the right file at all - it runs in the same per-user launchd context as the GUI app, so getconf DARWIN_USER_TEMP_DIR returns the same per-boot temp base cmux used, and nothing has to be hardcoded. The personal copy's closing reference pointed at a memory note with no public counterpart; it is replaced by a pointer back to launchd-env-sync-session-leak, which is the same class of failure from the other direction - a stale value synced into launchd, rather than a guard file reaped out from under one. Versions: bundle 1.130.0; launchd-env-sync-session-leak skill 1.1.0 -> 1.2.0, and its standalone plugin 1.0.0 -> 1.2.0, which had been left behind at 1.0.0 by an earlier skill bump. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # tests/test_cli_claude_teams_env.py
Per review, keep the manaflow-ai#3463 restore module out of Claude's own ~/.claude tree and under a cmux-owned directory, matching ~/.cmux/hooks and ~/.cmux/local-tmux. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I have read the CLA Document v2.2 and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
|
Thanks for working on this. #12022 is now fixed on main by #14814, which moves the Claude |
Summary
Store the Claude
NODE_OPTIONSrestore module under persistent user storage so it survives macOS cleanup of$TMPDIRbetween login sessions or reboots.Both current Claude launch entrypoints now prefer
$HOME/.claude/cmux/cmux-claude-node-optionsand fall back to${TMPDIR:-/tmp}only whenHOMEis unavailable.Current implementation
Resources/bin/cmux-claude-wrapper— uses the persistent HOME-based guard directory.CLI/cmux.swift— applies the same location in theclaude-teamspath, keeping both entrypoints consistent.tests/test_claude_wrapper_hooks.py— sandboxes HOME and covers guard placement, invalid HOME, and TMPDIR cleanup.tests/test_cli_claude_teams_env.py— verifies the Swift CLI environment uses the same persistent location and fails open for unusable HOME.The obsolete
Resources/bin/claudepath was deleted onmainand was not restored during conflict resolution.Verification
Closes #3463