fix(cli): preserve EROFS errno on share mount RO target by jason-ma-nv · Pull Request #4315 · NVIDIA/NemoClaw

jason-ma-nv · 2026-05-27T11:55:27Z

Summary

nemoclaw <name> share mount /sandbox <target> was reporting "ENOENT: no such file or directory, mkdir '<...>'" when the local target's parent filesystem was read-only, instead of the intended "parent filesystem is read-only" message. Root cause: Node's fs.mkdirSync(path, { recursive: true }) masks EROFS as ENOENT when the leaf is missing on a read-only parent, so the EROFS branch in checkLocalMountWritable never fired.

Related Issue

Fixes #4311.

Changes

src/lib/share-command.ts: when the immediate parent of localMount already exists, call fs.mkdirSync(localMount) without { recursive: true } so the kernel's EROFS errno propagates and the existing EROFS branch reports "parent filesystem is read-only". Treat EEXIST from the non-recursive call as success (the leaf is already present from a prior mount). Fall back to recursive mkdir only when the parent is genuinely missing.
test/share-command-writable.test.ts: four new #4311 tests covering non-recursive vs recursive branch selection, EROFS propagation on an existing parent, and EEXIST idempotency.

Type of Change

Code change (feature, bug fix, or refactor)
Code change with doc updates
Doc only (prose changes, no code sample modifications)
Doc only (includes code sample changes)

Verification

npx prek run --all-files passes
npm test passes
Tests added or updated for new or changed behavior
No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
npm run docs builds without warnings (doc changes only)
Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Jason Ma jama@nvidia.com

Summary by CodeRabbit

Bug Fixes
- Improved directory creation and error reporting so read-only filesystem errors are reported accurately (no longer masked as missing-path errors); also detects when a mount target exists but is not a directory.
Tests
- Added tests covering directory creation behavior and writability checks, including read-only filesystem and existing-non-directory scenarios.

Node's `fs.mkdirSync(path, { recursive: true })` masks EROFS as ENOENT when the leaf is missing on a read-only parent. The recursive call in `checkLocalMountWritable` therefore never hit the EROFS branch, so `nemoclaw <name> share mount /sandbox <target>` reported the misleading "ENOENT: no such file or directory, mkdir '<...>'" instead of the intended "parent filesystem is read-only" message. When the immediate parent already exists, fall back to non-recursive `fs.mkdirSync(localMount)` so the kernel's EROFS errno propagates and the existing EROFS branch fires. Treat EEXIST from the non-recursive call as success (the leaf was already present from a prior mount). Recursive mkdir is still used when the parent is genuinely missing. Fixes #4311. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>

coderabbitai · 2026-05-27T11:58:13Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 92215d10-5e8a-4592-b062-57a1c96df390

📥 Commits

Reviewing files that changed from the base of the PR and between e5db774 and 655abd4.

📒 Files selected for processing (2)

src/lib/share-command.ts
test/share-command-writable.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

src/lib/share-command.ts
test/share-command-writable.test.ts

📝 Walkthrough

Walkthrough

checkLocalMountWritable now checks whether the parent directory exists and uses non-recursive mkdir when it does (so EROFS propagates); it falls back to recursive mkdir only when the parent is missing. Tests were added to verify non-recursive vs recursive mkdir behavior, EROFS mapping, EEXIST handling, and subsequent writability checks.

Changes

EROFS error propagation in directory creation

Layer / File(s)	Summary
Conditional mkdir and EROFS preservation `src/lib/share-command.ts`, `test/share-command-writable.test.ts`	`checkLocalMountWritable` first checks `path.dirname(localMount)`: if the parent exists it calls non-recursive `fs.mkdirSync(localMount)` (so EROFS from a read-only parent surfaces), otherwise it calls `fs.mkdirSync(localMount, { recursive: true })`. Added tests mock `fs.existsSync`, `fs.mkdirSync`, `fs.statSync`, and `fs.accessSync` to validate the non-recursive vs recursive branches, EROFS -> "parent filesystem is read-only" mapping, and EEXIST handling for when an existing path is or is not a directory.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

fix

Suggested reviewers

ericksoa

Poem

🐰 A rabbit nudged the mkdir call with care,
So kernels' EROFS no longer hide there,
The parent is checked before we create,
Tests hop in to verify the state,
Now mounts tell the truth — no longer unfair.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: preventing EROFS errno masking when checking if a share mount target's parent filesystem is read-only.
Linked Issues check	✅ Passed	The PR fully addresses issue `#4311` by implementing the suggested fix (option a): using non-recursive mkdir when parent exists and recursive only when parent is missing, allowing EROFS to propagate correctly, plus handling EEXIST idempotency.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing issue `#4311`: the share-command.ts implementation change and its corresponding test coverage are both narrowly focused on the EROFS masking issue.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/4311-share-mount-erofs-recursive-mask

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-05-27T11:58:13Z

E2E Advisor Recommendation

Required E2E: None
Optional E2E: sandbox-operations-e2e, cloud-e2e

Dispatch hint: sandbox-operations-e2e,cloud-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

None.

Optional E2E

sandbox-operations-e2e (high): Closest existing runtime/sandbox lifecycle E2E. It validates live sandbox operations, SSH config access, logs, recovery, and cleanup, but it does not directly exercise nemoclaw share mount or sshfs mount-target writability.
cloud-e2e (medium): Broad install → onboard → sandbox verify → live inference smoke for the default OpenClaw user journey. Useful as a general runtime confidence check for CLI changes, but it does not cover the share mount path directly.

New E2E recommendations

share-mount-local-filesystem-preflight (high): No existing E2E job found that runs nemoclaw <sandbox> share mount with sshfs against a live sandbox and verifies local mount creation, readable sandbox content, clean unmount, and clear failure behavior for an unwritable/read-only mount parent. This PR changes exactly that preflight path.
- Suggested test: Add a share-mount E2E script/job that provisions or reuses a live sandbox, installs sshfs/FUSE as needed, runs nemoclaw <sandbox> share mount --local-mount <tmpdir>, verifies a known /sandbox file through the mount, runs share status and share unmount, and includes a negative case for an unwritable or read-only parent directory.

Dispatch hint

Workflow: E2E / Nightly
jobs input: sandbox-operations-e2e,cloud-e2e

github-actions · 2026-05-27T11:58:14Z

E2E Scenario Advisor Recommendation

Required scenario E2E: None
Optional scenario E2E: None

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

None. No scenario workflow, scenario metadata, scenario runtime, or validation-suite files changed.

Optional scenario E2E

None.

Relevant changed files

None.

github-actions · 2026-05-27T11:59:40Z

PR Review Advisor

Findings: 0 needs attention, 2 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 1 still applies, 0 new items found

Review findings

🛠️ Needs attention

None.

🔎 Worth checking

Source-of-truth review needed: src/lib/share-command.ts checkLocalMountWritable read-only-parent workaround: The advisor marked localized patch analysis as needs_followup.
- Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
- Evidence: src/lib/share-command.ts comments document the Node recursive mkdir EROFS masking and implement the branch; test/share-command-writable.test.ts adds mocked regression tests. The remaining follow-up is the same runtime-validation gap captured in the tests finding.
Add runtime validation for the read-only-parent share mount diagnostic (test/share-command-writable.test.ts:94): The new unit tests prove the helper chooses non-recursive mkdir and maps a mocked EROFS to the intended reason, but they do not exercise the actual Ubuntu/Node read-only bind-mount behavior or the full runShareMount failure path that prints the first stderr line, exits before SSHFS, and cleans up the temp SSH config. Because this bug is specifically about runtime filesystem behavior from Node's recursive mkdir, mocked fs calls leave the source-of-truth workaround only partially validated.
- Recommendation: Add or identify a focused runtime/integration test or manual validation note for an existing read-only parent where the leaf is missing, confirming the CLI reports "parent filesystem is read-only", exits before invoking sshfs, and leaves no temp SSH config behind. This does not need to reference any external E2E job status.
- Evidence: src/lib/share-command.ts:119-139 implements the non-recursive mkdir workaround; test/share-command-writable.test.ts:94-160 adds mocked fs tests for branch selection, EROFS propagation, EEXIST directory, and EEXIST non-directory. The full runShareMount stderr/exit/cleanup behavior remains supported by code inspection rather than a runtime or caller-level test.

🌱 Nice ideas

None.

Since last review details

Current findings:

Source-of-truth review needed: src/lib/share-command.ts checkLocalMountWritable read-only-parent workaround: The advisor marked localized patch analysis as needs_followup.
- Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
- Evidence: src/lib/share-command.ts comments document the Node recursive mkdir EROFS masking and implement the branch; test/share-command-writable.test.ts adds mocked regression tests. The remaining follow-up is the same runtime-validation gap captured in the tests finding.
Add runtime validation for the read-only-parent share mount diagnostic (test/share-command-writable.test.ts:94): The new unit tests prove the helper chooses non-recursive mkdir and maps a mocked EROFS to the intended reason, but they do not exercise the actual Ubuntu/Node read-only bind-mount behavior or the full runShareMount failure path that prints the first stderr line, exits before SSHFS, and cleans up the temp SSH config. Because this bug is specifically about runtime filesystem behavior from Node's recursive mkdir, mocked fs calls leave the source-of-truth workaround only partially validated.
- Recommendation: Add or identify a focused runtime/integration test or manual validation note for an existing read-only parent where the leaf is missing, confirming the CLI reports "parent filesystem is read-only", exits before invoking sshfs, and leaves no temp SSH config behind. This does not need to reference any external E2E job status.
- Evidence: src/lib/share-command.ts:119-139 implements the non-recursive mkdir workaround; test/share-command-writable.test.ts:94-160 adds mocked fs tests for branch selection, EROFS propagation, EEXIST directory, and EEXIST non-directory. The full runShareMount stderr/exit/cleanup behavior remains supported by code inspection rather than a runtime or caller-level test.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

coderabbitai

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 `@src/lib/share-command.ts`:
- Around line 131-133: The EEXIST fast-path in the catch block for localMount
incorrectly treats any existing file as a successful mount; update the handler
to stat/lstat the path (localMount) when errno === "EEXIST" and only swallow the
error if the target exists and is a directory (isDirectory() true); if it exists
but is not a directory, rethrow the error (or convert to a clear error) so the
later writable check doesn't report writable for an invalid mount target.

🪄 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: CHILL

Plan: Enterprise

Run ID: f0d33560-5bbc-4473-bd3e-1a0a8a6e1c7c

📥 Commits

Reviewing files that changed from the base of the PR and between d312144 and e5db774.

📒 Files selected for processing (2)

src/lib/share-command.ts
test/share-command-writable.test.ts

Address coderabbitai review on #4315: swallowing EEXIST from the non-recursive `fs.mkdirSync` was a regression. If `localMount` is an existing regular file, the prior recursive `mkdirSync` also threw EEXIST but it surfaced through the generic catch as an error; the new EEXIST fast-path silently fell through to `accessSync`, which passes on a writable file and returned `{ writable: true }` — a false positive that would let SSHFS try to mount over a regular file. On EEXIST, stat the path and only swallow the error if the existing entry is a directory; otherwise return a clear "mount target exists and is not a directory" reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>

cv

Reviewed according to the PR Advisor rubric: no blocking findings; acceptance for #4311 is covered by the narrow code change and targeted unit tests. The remaining runtime-validation note is non-blocking.

jason-ma-nv self-assigned this May 27, 2026

coderabbitai Bot reviewed May 27, 2026

View reviewed changes

Comment thread src/lib/share-command.ts

jason-ma-nv added the v0.0.53 Release target label May 27, 2026

Merge branch 'main' into fix/4311-share-mount-erofs-recursive-mask

85d0ea8

cv approved these changes May 27, 2026

View reviewed changes

cv enabled auto-merge (squash) May 27, 2026 15:48

cv disabled auto-merge May 27, 2026 15:57

Merge remote-tracking branch 'origin/main' into update-pr-4315

5554244

cv enabled auto-merge (squash) May 27, 2026 16:06

cv merged commit e139dbc into main May 27, 2026
26 checks passed

coderabbitai Bot mentioned this pull request May 27, 2026

fix(cli): sandbox status JSON, openshell resolve, policy descriptions, and tunnel command #4326

Open

wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026

Conversation

jason-ma-nv commented May 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Type of Change

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Advisor Recommendation

E2E Recommendation Advisor

Required E2E

Optional E2E

New E2E recommendations

Dispatch hint

Uh oh!

github-actions Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Scenario Advisor Recommendation

E2E Scenario Advisor

Required scenario E2E

Optional scenario E2E

Relevant changed files

Uh oh!

github-actions Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor

🛠️ Needs attention

🔎 Worth checking

🌱 Nice ideas

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jason-ma-nv commented May 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 27, 2026 •

edited

Loading

github-actions Bot commented May 27, 2026 •

edited

Loading

github-actions Bot commented May 27, 2026 •

edited

Loading

github-actions Bot commented May 27, 2026 •

edited

Loading