fix(sandbox): replace brew policy preset with first-class brew subcommand#3878
Closed
laitingsheng wants to merge 13 commits into
Closed
fix(sandbox): replace brew policy preset with first-class brew subcommand#3878laitingsheng wants to merge 13 commits into
laitingsheng wants to merge 13 commits into
Conversation
…mand Fixes #3757 Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds sandbox Brew CLI and action handlers, a reusable privileged-exec adapter, removes the built-in ChangesBrew command + privileged exec
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-3878.docs.buildwithfern.com/nemoclaw |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
docs/reference/commands.mdx (2)
704-704: 💤 Low valueUse more direct command behavior descriptions.
Lines 704, 715, 725, and 734 use the anthropomorphic construction "Refuses when..." to describe command failure conditions.
Command documentation typically uses more direct constructions.Suggested rewrites
-Refuses when shields are up — run `nemoclaw <name> shields down` first. +This command fails when shields are up — run `nemoclaw <name> shields down` first.or:
-Refuses when shields are up or when `brew init` has not been run. +Returns an error when shields are up or when `brew init` has not been run.As per coding guidelines: Active voice required when addressing the reader.
Also applies to: 715-715, 725-725, 734-734
🤖 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 `@docs/reference/commands.mdx` at line 704, Replace the anthropomorphic phrasing "Refuses when shields are up — run `nemoclaw <name> shields down` first." (and the analogous lines using "Refuses when...") with an active-voice, direct command description such as "Command fails if shields are up; run `nemoclaw <name> shields down` first." Update each occurrence so the sentence uses active voice and addresses the reader directly rather than personifying the command.
710-710: ⚡ Quick winSplit into one sentence per line.
Line 710 contains two sentences on the same source line, which violates the formatting rule for readable diffs.
Suggested fix
-Homebrew refuses to run as anyone other than the prefix owner, so the agent inside the sandbox cannot invoke `brew install` directly. Bottled binaries dropped by `brew install` (for example `/home/linuxbrew/.linuxbrew/bin/jq`) remain executable from the agent's PATH and survive `shields up`. +Homebrew refuses to run as anyone other than the prefix owner, so the agent inside the sandbox cannot invoke `brew install` directly. +Bottled binaries dropped by `brew install` (for example `/home/linuxbrew/.linuxbrew/bin/jq`) remain executable from the agent's PATH and survive `shields up`.As per coding guidelines: One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line.
🤖 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 `@docs/reference/commands.mdx` at line 710, The line containing "Homebrew refuses to run as anyone other than the prefix owner, so the agent inside the sandbox cannot invoke `brew install` directly. Bottled binaries dropped by `brew install` (for example `/home/linuxbrew/.linuxbrew/bin/jq`) remain executable from the agent's PATH and survive `shields up`." has two sentences on one source line; split them so each sentence is on its own line: place the sentence starting with "Homebrew refuses..." on one line and the sentence starting with "Bottled binaries dropped..." on the next line to satisfy the one-sentence-per-line rule.docs/network-policy/integration-policy-examples.mdx (1)
226-226: ⚡ Quick winRewrite to active voice.
"Homebrew is installed via a first-class subcommand" uses passive voice.
Active voice is required per the style guide.Suggested rewrite:
-Homebrew is installed via a first-class subcommand rather than a network preset: +Install Homebrew via a first-class subcommand rather than a network preset:or:
-Homebrew is installed via a first-class subcommand rather than a network preset: +Use the `brew` subcommand to install Homebrew rather than a network preset:As per coding guidelines: Active voice required. Flag passive constructions.
🤖 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 `@docs/network-policy/integration-policy-examples.mdx` at line 226, Replace the passive sentence "Homebrew is installed via a first-class subcommand rather than a network preset:" with an active-voice construction; for example, rewrite to "Install Homebrew using a first-class subcommand rather than a network preset." Update the sentence in the same paragraph containing the phrase "Homebrew is installed via a first-class subcommand" so the message is active and matches the style guide.src/lib/shields/index.ts (1)
18-20: Run shields lifecycle E2E for this exec-path refactor.Since
src/lib/shields/**now routes privileged execution through a new adapter, run the shields lifecycle E2E job to validate lock/unlock + timer restore behavior on this branch.As per coding guidelines: “E2E test recommendation:
shields-config-e2e— shields lifecycle + config get/set/rotate”.Also applies to: 46-55
🤖 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 `@src/lib/shields/index.ts` around lines 18 - 20, The shield adapter refactor now routes privileged execution through the new adapter (privilegedSandboxExec / rawPrivilegedSandboxExec in src/lib/shields/index.ts) but the PR lacks verification; run the shields lifecycle E2E job "shields-config-e2e" to validate lock/unlock and timer restore behavior and ensure config get/set/rotate still work; if failures appear, update the adapter wiring around privilegedSandboxExec (and any call sites that used rawPrivilegedSandboxExec) to match the new execution path and re-run the E2E until the lifecycle tests pass.src/lib/adapters/sandbox/privileged-exec.test.ts (1)
31-41: ⚡ Quick winAdd a regression test for exact-match precedence regardless of list order.
Current tests don’t cover the case where a prefix match appears before the exact container name; that case should still pick the exact match.
Suggested test case
describe("selectDockerDriverSandboxContainer", () => { + it("prefers exact match even when prefix match appears first", () => { + expect( + selectDockerDriverSandboxContainer( + "demo", + "docker", + "openshell-demo-abc\nopenshell-demo\nopenshell-other", + ), + ).toBe("openshell-demo"); + }); + it("returns the exact docker-driver sandbox container when present", () => {🤖 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 `@src/lib/adapters/sandbox/privileged-exec.test.ts` around lines 31 - 41, Add a regression test for selectDockerDriverSandboxContainer that verifies exact-match precedence even when a prefix match appears earlier in the container list: create a test where the container list contains a prefix (e.g., "openshell-demo-abc") before the exact name ("openshell-demo") and assert the function returns "openshell-demo"; ensure the test uses the same call signature as the existing tests (selectDockerDriverSandboxContainer("demo", "docker", "<list>")) and add it alongside the other tests in privileged-exec.test.ts.
🤖 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 @.github/workflows/nightly-e2e.yaml:
- Around line 1435-1438: The Checkout step currently uses actions/checkout@...
with default credential persistence which leaves the repo token available to
subsequent steps; update the "Checkout" step that calls actions/checkout to
explicitly set with: persist-credentials: false so the GITHUB_TOKEN/credentials
are not left in the working directory for downstream shell steps (look for the
step named "Checkout" that invokes actions/checkout and add persist-credentials:
false to its with block).
In `@src/commands/sandbox/brew.ts`:
- Line 13: The static usage string (static usage) advertises a subcommand token
that doesn't exist; update it to reflect the actual parsed argument
(sandboxName) so help output is accurate—replace the current
["<init|deinit|install|uninstall> <name>"] with a single-name usage (e.g.,
["<sandboxName>"] or ["<name>"]) to match the parsing code that reads
sandboxName in the command handler.
In `@src/lib/actions/sandbox/brew.ts`:
- Around line 118-119: The call to registry.updateSandbox in the brew
init/deinit flows (the updateSandbox(sandboxName, { brewInitialised: true }) and
the corresponding deinit call setting false) ignores its boolean result so the
CLI reports success even if the registry entry was missing; update both places
to check the returned boolean and treat a false result as a failure: when
updateSandbox returns false, log a clear error referencing sandboxName and the
attempted brewInitialised value (use processLogger or console.error), and abort
the command (throw or process.exit with non‑zero) so the stale state isn’t
reported as success.
In `@src/lib/adapters/sandbox/privileged-exec.ts`:
- Around line 28-33: The current container selection logic in the return
expression uses a single find with the predicate name === exact ||
name.startsWith(prefix), which can pick a prefix match that appears earlier than
an exact match; change it to first search for an exact match and return it if
found, otherwise search for the first name that startsWith(prefix) and return
that (or null if none). Update the code around the
containerNames.split("\n").map(...).find(...) expression in privileged-exec.ts
to perform two lookups: one for name === exact, then one for
name.startsWith(prefix).
In `@src/lib/state/registry.ts`:
- Line 43: The new optional field brewInitialised on the registry entries is
never persisted in registerSandbox, so re-registering a sandbox can drop that
flag; update registerSandbox to copy/persist brewInitialised into the stored
entry (when creating a new entry and when merging/updating an existing one) by
reading the incoming value (or retaining the existing value if not provided) and
writing it into the registry object saved by the function; ensure both the
creation branch and the update/merge branch in registerSandbox handle the
brewInitialised property so it survives re-registration.
---
Nitpick comments:
In `@docs/network-policy/integration-policy-examples.mdx`:
- Line 226: Replace the passive sentence "Homebrew is installed via a
first-class subcommand rather than a network preset:" with an active-voice
construction; for example, rewrite to "Install Homebrew using a first-class
subcommand rather than a network preset." Update the sentence in the same
paragraph containing the phrase "Homebrew is installed via a first-class
subcommand" so the message is active and matches the style guide.
In `@docs/reference/commands.mdx`:
- Line 704: Replace the anthropomorphic phrasing "Refuses when shields are up —
run `nemoclaw <name> shields down` first." (and the analogous lines using
"Refuses when...") with an active-voice, direct command description such as
"Command fails if shields are up; run `nemoclaw <name> shields down` first."
Update each occurrence so the sentence uses active voice and addresses the
reader directly rather than personifying the command.
- Line 710: The line containing "Homebrew refuses to run as anyone other than
the prefix owner, so the agent inside the sandbox cannot invoke `brew install`
directly. Bottled binaries dropped by `brew install` (for example
`/home/linuxbrew/.linuxbrew/bin/jq`) remain executable from the agent's PATH and
survive `shields up`." has two sentences on one source line; split them so each
sentence is on its own line: place the sentence starting with "Homebrew
refuses..." on one line and the sentence starting with "Bottled binaries
dropped..." on the next line to satisfy the one-sentence-per-line rule.
In `@src/lib/adapters/sandbox/privileged-exec.test.ts`:
- Around line 31-41: Add a regression test for
selectDockerDriverSandboxContainer that verifies exact-match precedence even
when a prefix match appears earlier in the container list: create a test where
the container list contains a prefix (e.g., "openshell-demo-abc") before the
exact name ("openshell-demo") and assert the function returns "openshell-demo";
ensure the test uses the same call signature as the existing tests
(selectDockerDriverSandboxContainer("demo", "docker", "<list>")) and add it
alongside the other tests in privileged-exec.test.ts.
In `@src/lib/shields/index.ts`:
- Around line 18-20: The shield adapter refactor now routes privileged execution
through the new adapter (privilegedSandboxExec / rawPrivilegedSandboxExec in
src/lib/shields/index.ts) but the PR lacks verification; run the shields
lifecycle E2E job "shields-config-e2e" to validate lock/unlock and timer restore
behavior and ensure config get/set/rotate still work; if failures appear, update
the adapter wiring around privilegedSandboxExec (and any call sites that used
rawPrivilegedSandboxExec) to match the new execution path and re-run the E2E
until the lifecycle tests pass.
🪄 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: d7f8e1fa-76d5-4acc-ab98-ecdd14749e60
📒 Files selected for processing (31)
.github/workflows/nightly-e2e.yamldocs/get-started/quickstart.mdxdocs/network-policy/customize-network-policy.mdxdocs/network-policy/integration-policy-examples.mdxdocs/reference/commands.mdxdocs/reference/network-policies.mdxdocs/security/best-practices.mdxnemoclaw-blueprint/policies/presets/brew.yamlnemoclaw-blueprint/policies/tiers.yamlsrc/commands/sandbox/brew.test.tssrc/commands/sandbox/brew.tssrc/commands/sandbox/brew/deinit.tssrc/commands/sandbox/brew/init.tssrc/commands/sandbox/brew/install.tssrc/commands/sandbox/brew/uninstall.tssrc/lib/actions/sandbox/brew.test.tssrc/lib/actions/sandbox/brew.tssrc/lib/adapters/sandbox/privileged-exec.test.tssrc/lib/adapters/sandbox/privileged-exec.tssrc/lib/cli/command-registry.test.tssrc/lib/sandbox/brew-command-support.tssrc/lib/sandbox/config.tssrc/lib/shields/index.tssrc/lib/state/registry.tstest/e2e/test-brew-command.shtest/onboard-policy-suggestions.test.tstest/onboard-preset-diff.test.tstest/policies.test.tstest/policy-tiers-onboard.test.tstest/policy-tiers.test.tstest/repro-2681-group-writable.test.ts
💤 Files with no reviewable changes (4)
- nemoclaw-blueprint/policies/tiers.yaml
- nemoclaw-blueprint/policies/presets/brew.yaml
- docs/security/best-practices.mdx
- docs/network-policy/customize-network-policy.mdx
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26148315609
|
- brew init now writes /etc/profile.d/nemoclaw-linuxbrew.sh so login shells see the Homebrew prefix on PATH (deinit removes it). - registerSandbox persists brewInitialised so the flag survives recovery and re-registration. - selectDockerDriverSandboxContainer prefers exact match over prefix, so a generated container like openshell-demo-abc cannot intercept commands targeted at openshell-demo. - Tighten the doc claim about PATH (long-running processes started before brew init keep their original PATH) and fix the broken #nemoclaw-name-brew anchor link. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
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 `@src/lib/actions/sandbox/brew.ts`:
- Around line 96-101: The generated profile hook in
src/lib/actions/sandbox/brew.ts uses a single-bracket test with wildcard pattern
matching which treats '*' literally and can duplicate entries; update the guard
that references PROFILE_D_PATH and LINUXBREW_PREFIX so it uses a proper shell
pattern test (e.g. change the condition to use double brackets: [[ -d
${LINUXBREW_PREFIX}/bin && ":${PATH}:" != *":${LINUXBREW_PREFIX}/bin:"* ]] ) or
switch to a case/:$PATH: pattern check to reliably detect presence and only
prepend once; modify the string templates that produce the `if [...] && [...]`
line accordingly.
🪄 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: 61da8faa-4c34-4210-be16-41ff2d1197ae
📒 Files selected for processing (8)
docs/network-policy/integration-policy-examples.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/brew.test.tssrc/lib/actions/sandbox/brew.tssrc/lib/adapters/sandbox/privileged-exec.test.tssrc/lib/adapters/sandbox/privileged-exec.tssrc/lib/state/registry.tstest/e2e/test-brew-command.sh
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.mdx
- Brew action treats updateSandbox returning false as a hard failure on init and deinit, so a missing registry entry no longer reports success with stale state. - Parent brew command usage now reads `<name>` to match the one positional arg it actually parses, instead of advertising a phantom subcommand token. - brew-command-e2e checkout step sets persist-credentials: false to keep the GITHUB_TOKEN off downstream shell steps. - E2E test harness wraps each TC in `|| true` and uses a trap for sandbox teardown, so an early failure no longer skips the remaining cases or the cleanup. - Tighten the install-script assertion to a plain substring match against the full URL, dropping the unescaped-dot regex that CodeQL flagged. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
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)
.github/workflows/nightly-e2e.yaml (1)
1426-1456:⚠️ Potential issue | 🔴 CriticalAdd missing
path_instructionsentry fortest/e2e/test-brew-command.shin.coderabbit.yaml.The new
brew-command-e2ejob is added to.github/workflows/nightly-e2e.yamlwith a corresponding test script attest/e2e/test-brew-command.sh, but this script lacks apath_instructionsentry in.coderabbit.yaml. Per the coding guidelines and the documented requirement in the workflow file itself: "If a new E2E job is added, verify correspondingpath_instructionsentries exist in.coderabbit.yamlfor the source files it covers."Add a
path_instructionsentry fortest/e2e/test-brew-command.shfollowing the pattern of other E2E test scripts (e.g.,test/e2e/test-cloud-onboard-e2e.sh), documenting that it covers thebrew-command-e2ejob and the brew command functionality insrc/commands/sandbox/brew.ts.🤖 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 @.github/workflows/nightly-e2e.yaml around lines 1426 - 1456, Add a path_instructions entry in .coderabbit.yaml for the new test script test/e2e/test-brew-command.sh: create a mapping under path_instructions that lists this file and documents it covers the brew-command-e2e job and the brew command implementation at src/commands/sandbox/brew.ts, following the same structure/fields used by existing E2E entries (e.g., test/e2e/test-cloud-onboard-e2e.sh) so the workflow and coderabbit config stay in sync.
🤖 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 @.github/workflows/nightly-e2e.yaml:
- Around line 1426-1456: Add a path_instructions entry in .coderabbit.yaml for
the new test script test/e2e/test-brew-command.sh: create a mapping under
path_instructions that lists this file and documents it covers the
brew-command-e2e job and the brew command implementation at
src/commands/sandbox/brew.ts, following the same structure/fields used by
existing E2E entries (e.g., test/e2e/test-cloud-onboard-e2e.sh) so the workflow
and coderabbit config stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f714d216-22ab-4c7c-b70a-d6afe1a29a5d
📒 Files selected for processing (5)
.github/workflows/nightly-e2e.yamlsrc/commands/sandbox/brew.tssrc/lib/actions/sandbox/brew.test.tssrc/lib/actions/sandbox/brew.tstest/e2e/test-brew-command.sh
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26153972221
|
…nit from install brew install now accepts --yes/-y. When --yes is set alongside the existing NEMOCLAW_NON_INTERACTIVE=1 env var, install auto-bootstraps Homebrew first if the sandbox has not been initialised yet, instead of refusing. Interactive sessions or scripts that omit either signal keep the explicit-init contract. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
PR Review AdvisorRecommendation: info only This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Advisor execution failed: Could not configure advisor model openai/openai/gpt-5.5 Full advisor summaryPR Review AdvisorBase: PR review advisor failed: Could not configure advisor model openai/openai/gpt-5.5 Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26165857181
|
- Generated /etc/profile.d/nemoclaw-linuxbrew.sh now uses a portable `case` guard instead of `[ != *...* ]`, which under single-bracket test treats `*` as a literal and lets repeated sourcing prepend duplicate prefix entries. - assertBrewInitialised takes an optional extra hint, so brew uninstall no longer dangles the install-only "--yes auto-init" suggestion in its refusal message. Only brew install passes the hint. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26168768350
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26173179557
|
…registry Sandboxes onboarded under prior NemoClaw versions can still carry preset names in `policies` (e.g. legacy `brew`) whose YAML has since been removed from the blueprint. policy-list silently hid them and rebuild restoration attempted to apply them, hard-failing. Add `partitionKnownPresetNames` and `pruneStaleBuiltInPresets` in src/lib/policy/index.ts. The prune helper drops any policy name absent from both `listPresets()` and the sandbox's `customPolicies`, warns to stderr, and rewrites the registry so the warning fires at most once per sandbox. Custom presets are always preserved. Wire two callers: - `listSandboxPolicies` (policy-list display) prunes on read. - Rebuild's "Restoring policy presets" loop partitions the backup manifest's saved preset list, warns about stale entries, and only iterates the known subset. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26174408916
|
- Snapshot restore --to clones most of the source registry entry into the destination, but `/home/linuxbrew` is not part of the snapshot manifest. Force brewInitialised: false on the cloned entry so a brew-bootstrapped source does not fool the destination into thinking Homebrew is present before the operator runs `brew init` on it. - pruneStaleBuiltInPresets now warns that any network rules a removed preset installed may still be live on the gateway and points the user at `nemoclaw <sb> rebuild` for a clean policy state. The active gateway rules can't be cleaned up automatically (the preset YAML is gone) and policy-list/policy-remove cannot surface them after the prune. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26175918454
|
- The container prefix fallback in selectDockerDriverSandboxContainer
could still intercept across sandboxes whose names share a prefix
(e.g. sandbox `demo` resolving to `openshell-demo-prod` when the
exact `openshell-demo` is absent and sandbox `demo-prod` exists).
resolveDockerDriverSandboxContainer now passes the registry's known
sandbox names so the resolver excludes any prefix candidate whose
suffix exact-matches another registered sandbox. Default empty list
preserves existing callers/tests.
- Register `brew-command-e2e` in `.coderabbit.yaml` path_instructions
for `src/lib/actions/sandbox/brew.ts`,
`src/commands/sandbox/brew{,/**}.ts`, and the E2E script itself, per
the nightly-e2e meta-rule that new jobs must have matching coverage
hints in the CodeRabbit config.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
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)
src/lib/actions/sandbox/brew.ts (1)
258-261:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeinit can skip policy cleanup when registry state is stale.
Line 258 returns early when
brewInitialised !== true, so Line 267 policy cleanup never runs. If state is desynced, the sandbox can retain__brew_runtime__network allowances afterdeinit. Please makedeinitalways attempt runtime-policy removal, even in the idempotent path.Also applies to: 267-268
🤖 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 `@src/lib/actions/sandbox/brew.ts` around lines 258 - 261, The early return guarded by entry.brewInitialised in deinit prevents runtime-policy removal and can leave __brew_runtime__ network allowances; change control flow so the runtime-policy cleanup always runs regardless of entry.brewInitialised (either move the policy-removal calls currently near lines 267-268 to before the return or remove the early return and conditionally skip only the Homebrew-specific teardown while always invoking the runtime-policy removal). Reference entry.brewInitialised, sandboxName and the runtime-policy removal/cleanup calls so the deinit path always attempts to remove the __brew_runtime__ policy even when state is stale.
🧹 Nitpick comments (2)
src/lib/actions/sandbox/brew.test.ts (1)
82-95: ⚡ Quick winAdd assertions for runtime-policy apply/remove side effects.
These tests cover bootstrap/deinit execution, but they currently do not assert that policy update commands are actually issued. Since init/deinit now mutate gateway policy, add expectations around
buildPolicyGetCommand,buildPolicySetCommand, andruncalls in the success paths.Also applies to: 246-257
🤖 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 `@src/lib/actions/sandbox/brew.test.ts` around lines 82 - 95, The test for runSandboxBrew("alpha", { kind: "init" }) needs assertions that the gateway policy commands are invoked: after calling runSandboxBrew add expects that buildPolicyGetCommand and buildPolicySetCommand were called with the expected sandbox name ("alpha") and that the mocked run was invoked with the commands returned by those builders; mirror these assertions in the deinit test (the block around lines 246-257) to assert buildPolicyGetCommand/buildPolicySetCommand and run are used for removing the policy as well, using the same mock call inspection pattern as privilegedSandboxExec/mock.calls to verify arguments.docs/security/best-practices.mdx (1)
85-85: ⚡ Quick winUse a sentence instead of a clause colon in the table cell.
Line 85 uses
Landlock layout: no.where the colon does not introduce a list. Please rewrite this as a normal sentence to match docs style.As per coding guidelines: “Colons should only introduce a list. Flag colons used as general punctuation between clauses.”
🤖 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 `@docs/security/best-practices.mdx` at line 85, Replace the table cell clause "Landlock layout: no." with a full sentence that avoids using a colon as punctuation; update the text to something like "Landlock does not support layout." or "Layout is not supported by Landlock." so the cell reads as a normal sentence and matches the document's style; locate the cell containing the exact string "Landlock layout: no." and make the replacement.
🤖 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 `@src/lib/actions/sandbox/brew.ts`:
- Around line 258-261: The early return guarded by entry.brewInitialised in
deinit prevents runtime-policy removal and can leave __brew_runtime__ network
allowances; change control flow so the runtime-policy cleanup always runs
regardless of entry.brewInitialised (either move the policy-removal calls
currently near lines 267-268 to before the return or remove the early return and
conditionally skip only the Homebrew-specific teardown while always invoking the
runtime-policy removal). Reference entry.brewInitialised, sandboxName and the
runtime-policy removal/cleanup calls so the deinit path always attempts to
remove the __brew_runtime__ policy even when state is stale.
---
Nitpick comments:
In `@docs/security/best-practices.mdx`:
- Line 85: Replace the table cell clause "Landlock layout: no." with a full
sentence that avoids using a colon as punctuation; update the text to something
like "Landlock does not support layout." or "Layout is not supported by
Landlock." so the cell reads as a normal sentence and matches the document's
style; locate the cell containing the exact string "Landlock layout: no." and
make the replacement.
In `@src/lib/actions/sandbox/brew.test.ts`:
- Around line 82-95: The test for runSandboxBrew("alpha", { kind: "init" })
needs assertions that the gateway policy commands are invoked: after calling
runSandboxBrew add expects that buildPolicyGetCommand and buildPolicySetCommand
were called with the expected sandbox name ("alpha") and that the mocked run was
invoked with the commands returned by those builders; mirror these assertions in
the deinit test (the block around lines 246-257) to assert
buildPolicyGetCommand/buildPolicySetCommand and run are used for removing the
policy as well, using the same mock call inspection pattern as
privilegedSandboxExec/mock.calls to verify arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 720145e1-7755-4140-acc1-ecfcb31bf52b
📒 Files selected for processing (8)
docs/network-policy/customize-network-policy.mdxdocs/network-policy/integration-policy-examples.mdxdocs/reference/commands.mdxdocs/reference/network-policies.mdxdocs/security/best-practices.mdxnemoclaw-blueprint/policies/integrations/brew.yamlsrc/lib/actions/sandbox/brew.test.tssrc/lib/actions/sandbox/brew.ts
💤 Files with no reviewable changes (1)
- docs/network-policy/customize-network-policy.mdx
✅ Files skipped from review due to trivial changes (3)
- docs/reference/network-policies.mdx
- docs/network-policy/integration-policy-examples.mdx
- docs/reference/commands.mdx
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26182292353
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26182129196
|
The previous docker-driver privileged-exec path used `docker exec --user <name>`, which switches UID but inherits HOME from the calling shell. When the target user was `linuxbrew`, brew's Ruby tried to write to /root/.cache/Homebrew and hit EACCES because it could not access root's home as the linuxbrew user. Drop the `--user` flag and route non-root targets through `runuser -u <name> --` instead — the same wrapper kubectl-path already uses. runuser sets HOME to the target user's pw_dir, so brew, bundler, and similar tools land in the right per-user cache/config directories. Root operations stay flag-free: docker exec lands on the container's USER directive (root in NemoClaw's image), and withUserPrefix is a no-op for root. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26183761287
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26183924572
|
This was referenced May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
brewpolicy preset opened Homebrew network endpoints but never installed the brew CLI, so the documentedbrew install hellovalidation step had no chance of succeeding. This PR deletes the misleading preset and ships a first-classnemoclaw <sandbox> brew {init,install,uninstall,deinit}lifecycle that actually provisions Homebrew (Linuxbrew) inside the sandbox via the privileged exec channel.Related Issue
Fixes #3757
Fixes #1767
Changes
BrewRequestaction layer (src/lib/actions/sandbox/brew.ts) with shields-up refusal, init-required refusal, formula-name validation, and idempotent init/deinit semantics.brew install --yespaired withNEMOCLAW_NON_INTERACTIVE=1auto-runsbrew initfirst when Homebrew is not yet bootstrapped.sandbox brew(help),sandbox brew {init,deinit,install,uninstall}with variadic packages for install/uninstall.privilegedSandboxExecinto a shared adapter (src/lib/adapters/sandbox/privileged-exec.ts) acceptinguser?,stdin?,input?,timeout?. Migrateshields/andsandbox/config/onto it; drop the two local copies. Resolver now prefers exact match over prefix, so a generatedopenshell-demo-abccannot intercept commands targeted atopenshell-demo.brewInitialisedonSandboxEntry(persisted throughregisterSandbox) so the action can refuseinstall/uninstallbefore init, short-circuit a duplicate init, and not be leaked into snapshot-restore clones.brew initwrites/etc/profile.d/nemoclaw-linuxbrew.shso login shells see the linuxbrew prefix on PATH;brew deinitremoves it.nemoclaw-blueprint/policies/presets/brew.yamland dropbrewfromnemoclaw-blueprint/policies/tiers.yaml(Balanced and Open tiers) plus the matching test/doc fixtures.pruneStaleBuiltInPresetsinsrc/lib/policy/index.tsand call it fromlistSandboxPoliciesand the rebuild restoration loop. Sandboxes upgrading from older NemoClaw versions withbrewin theirpolicieslist get the name silently dropped from the registry on the next interaction, with a warning to stderr and a hint to runrebuildfor a clean gateway-policy state (the live gateway rules can't be cleaned in place because the preset YAML is gone). Custom presets are preserved.brew-command-e2ejob to.github/workflows/nightly-e2e.yaml(TC-BREW-01..07 viatest/e2e/test-brew-command.sh) and register it in the workflow_dispatch input list plus all three aggregation lists.docs/reference/commands.mdx,docs/network-policy/*,docs/reference/network-policies.mdx,docs/security/best-practices.mdx,docs/get-started/quickstart.mdxto reflect the new shape..agents/skills/user skills are deliberately untouched — they regenerate fromdocs/during release-prep, not in feature PRs.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Changes
Refactor
Documentation
Tests