feat(onboard): auto-apply UFW sandbox-bridge rule under NEMOCLAW_AUTO_FIX_FIREWALL=1 (#4265)#4267
Merged
Conversation
…_FIX_FIREWALL=1 (#4265) On Brev shadecloud Ubuntu 22.04 the host ships with `ufw INPUT policy DROP`, so sandbox containers on the docker bridge can't reach `172.18.0.1:8080` until the operator runs the `sudo ufw allow ...` rule the onboard preflight already prints. Eko's QA team and anyone fresh- booting a shadecloud H100 hits this every time. Opt-in path: set `NEMOCLAW_AUTO_FIX_FIREWALL=1`. When the reachability probe fails on a `bridge_gateway` route, `verifySandboxBridgeGatewayReachableOrExit` calls a new `tryAutoApplyUfwRule` helper that: 1. Checks `sudo -n true` — never prompts. Without passwordless sudo, the manual-instructions path runs as before. 2. Checks `sudo -n which ufw` — skips if ufw isn't installed. 3. Checks `sudo -n ufw status` — skips if ufw is inactive (rule moot). 4. Runs `sudo -n ufw allow from <subnet> to <gatewayIp> port <port> proto tcp` — the same rule the existing message prints. 5. Re-probes reachability; proceeds silently if now ok. Without the env var, behaviour is unchanged (existing manual hint, same exit code). Safety: - Opt-in only; no implicit consent. - Narrow rule (single src subnet → single dest IP:port/proto), identical to what preflight already advises. - `sudo -n` only — never elevates with a password prompt mid-run. - Skips on unsupported environments (no sudo, no ufw, ufw inactive) rather than failing loud — the existing manual hint still surfaces. Tests (6 new, all in `gateway-sandbox-reachability.test.ts`): - not_opted_in (env var unset → skip) - no_subnet_or_gateway (missing route info → skip) - sudo_unavailable (passwordless sudo refused) - ufw_missing (`which ufw` non-zero) - ufw_inactive (`Status: inactive`) - ufw_rule_rejected (apply non-zero) - applied=true happy path with exact argv assertion Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Contributor
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an opt-in, non-interactive UFW remediation flow for Docker bridge → gateway TCP reachability failures: a testable ChangesUFW auto-apply sandbox-gateway fix
Sequence Diagram(s)sequenceDiagram
participant Verifier as "verifySandboxBridgeGatewayReachableOrExit"
participant AutoApply as "tryAutoApplyUfwRule"
participant System as "sudo/ufw (system)"
participant Probe as "reachability probe"
Verifier->>Probe: initial probe (gatewayIp:port)
Probe-->>Verifier: fail (bridge_gateway TCP)
Verifier->>AutoApply: autoApplyOptedIn? -> tryAutoApplyUfwRule(reach, options)
AutoApply->>System: sudo -n true / which ufw / ufw status / ufw allow ...
System-->>AutoApply: check results (ok / rejected / missing)
AutoApply-->>Verifier: result (applied | reason)
alt applied
Verifier->>Probe: re-probe reachability
Probe-->>Verifier: reachable / still failing
else not applied (non-silent)
Verifier-->>Verifier: emit warning, fall back to manual instructions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
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
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 2 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/onboard/gateway-sandbox-reachability.test.ts (1)
204-212: 💤 Low valueConsider adding a test case for missing
subnet.The test covers
gatewayIp: undefinedbut notsubnet: undefined. While both are checked together in the implementation (!reach.subnet || !reach.gatewayIp), adding a symmetric test case would fully document the boundary.🤖 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/onboard/gateway-sandbox-reachability.test.ts` around lines 204 - 212, Add a symmetric test to cover the case when reach.subnet is undefined: in the test file gateway-sandbox-reachability.test.ts add an it block similar to the existing "skips when subnet or gatewayIp is unknown" case that calls tryAutoApplyUfwRule with { ...reach, subnet: undefined } and the same runImpl/optedIn setup, then assert the result equals { applied: false, reason: "no_subnet_or_gateway" } and that recorded has length 0; this mirrors the gatewayIp test and ensures tryAutoApplyUfwRule's check for !reach.subnet is exercised.src/lib/onboard/gateway-sandbox-reachability.ts (2)
366-421: 💤 Low value
asyncis unnecessary since the function body is synchronous.
tryAutoApplyUfwRuleis markedasyncbut contains noawaitexpressions—all operations use synchronousspawnSyncviarunImpl. The function will still work correctly (returning aPromisewrapping the result), but theasynckeyword is misleading and adds slight overhead.♻️ Optional: remove async keyword
-export async function tryAutoApplyUfwRule( +export function tryAutoApplyUfwRule( reach: SandboxBridgeReachabilityResult, options: UfwAutoApplyOptions = {}, -): Promise<UfwAutoApplyResult> { +): UfwAutoApplyResult {Note: This would also require updating the call sites to not
awaitthe result, or keeping thePromisereturn type for API consistency if desired.🤖 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/onboard/gateway-sandbox-reachability.ts` around lines 366 - 421, The function tryAutoApplyUfwRule is marked async but has no awaits (it uses synchronous runImpl/defaultRunArgv), so remove the async keyword from the function declaration and change its return type from Promise<UfwAutoApplyResult> to UfwAutoApplyResult; update any callers that currently await its result to handle the synchronous return (remove await) or explicitly wrap with Promise.resolve(...) if you need to keep promise semantics. Refer to tryAutoApplyUfwRule, options.runImpl/defaultRunArgv and the run variable in the function when making the changes.
444-448: 💤 Low valueMinor: the
not_opted_incheck is redundant.The outer condition at line 436 already gates on
isUfwAutoApplyOptedIn(), sotryAutoApplyUfwRulewill never returnreason: "not_opted_in"when this branch executes (theoptedInoption isn't overridden here, so it uses the same env var check). The guard is harmless but unnecessary.🤖 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/onboard/gateway-sandbox-reachability.ts` around lines 444 - 448, Remove the redundant check against autoApply.reason === "not_opted_in" in the branch that logs the console.warn; because the outer gate already ensures isUfwAutoApplyOptedIn() is true, tryAutoApplyUfwRule will never return "not_opted_in" here—change the else-if that references autoApply.reason to a plain else so that any non-success result from tryAutoApplyUfwRule triggers the existing console.warn (referencing tryAutoApplyUfwRule, autoApply, isUfwAutoApplyOptedIn, and the console.warn call).
🤖 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.
Nitpick comments:
In `@src/lib/onboard/gateway-sandbox-reachability.test.ts`:
- Around line 204-212: Add a symmetric test to cover the case when reach.subnet
is undefined: in the test file gateway-sandbox-reachability.test.ts add an it
block similar to the existing "skips when subnet or gatewayIp is unknown" case
that calls tryAutoApplyUfwRule with { ...reach, subnet: undefined } and the same
runImpl/optedIn setup, then assert the result equals { applied: false, reason:
"no_subnet_or_gateway" } and that recorded has length 0; this mirrors the
gatewayIp test and ensures tryAutoApplyUfwRule's check for !reach.subnet is
exercised.
In `@src/lib/onboard/gateway-sandbox-reachability.ts`:
- Around line 366-421: The function tryAutoApplyUfwRule is marked async but has
no awaits (it uses synchronous runImpl/defaultRunArgv), so remove the async
keyword from the function declaration and change its return type from
Promise<UfwAutoApplyResult> to UfwAutoApplyResult; update any callers that
currently await its result to handle the synchronous return (remove await) or
explicitly wrap with Promise.resolve(...) if you need to keep promise semantics.
Refer to tryAutoApplyUfwRule, options.runImpl/defaultRunArgv and the run
variable in the function when making the changes.
- Around line 444-448: Remove the redundant check against autoApply.reason ===
"not_opted_in" in the branch that logs the console.warn; because the outer gate
already ensures isUfwAutoApplyOptedIn() is true, tryAutoApplyUfwRule will never
return "not_opted_in" here—change the else-if that references autoApply.reason
to a plain else so that any non-success result from tryAutoApplyUfwRule triggers
the existing console.warn (referencing tryAutoApplyUfwRule, autoApply,
isUfwAutoApplyOptedIn, and the console.warn call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ad022ef-aa78-4ecf-80a0-9f9a54068c29
📒 Files selected for processing (2)
src/lib/onboard/gateway-sandbox-reachability.test.tssrc/lib/onboard/gateway-sandbox-reachability.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26474872645
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26488308986
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26519898443
|
# Conflicts: # src/lib/onboard/gateway-sandbox-reachability.ts
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4267.docs.buildwithfern.com/nemoclaw |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…est' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26523820633
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/gateway-sandbox-reachability.ts (1)
493-495: 💤 Low valueConsider guarding against undefined in the success log.
If
autoApplyResult.appliedis true,tryAutoApplyUfwRulemust have validatedsubnetandgatewayIp, so this is likely unreachable. However, the types allowundefined, and the template literal would print "undefined" in that edge case. A minor guard could make the message cleaner.♻️ Optional fix
if (autoApplyResult.applied) { + const ruleDesc = reach.subnet && reach.gatewayIp + ? `allow from ${reach.subnet} to ${reach.gatewayIp}:${port}/tcp` + : `allow sandbox bridge traffic to port ${port}/tcp`; console.log( - ` ✓ Applied UFW rule (NEMOCLAW_AUTO_FIX_FIREWALL=1): allow from ${reach.subnet} to ${reach.gatewayIp}:${port}/tcp`, + ` ✓ Applied UFW rule (NEMOCLAW_AUTO_FIX_FIREWALL=1): ${ruleDesc}`, );🤖 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/onboard/gateway-sandbox-reachability.ts` around lines 493 - 495, The success log may print "undefined" if reach.subnet or reach.gatewayIp are undefined; when autoApplyResult.applied is true (from tryAutoApplyUfwRule), guard the template by using safe defaults or nullish coalescing for reach.subnet and reach.gatewayIp (e.g., `${reach.subnet ?? '<unknown-subnet>'}` and `${reach.gatewayIp ?? '<unknown-gateway>'}`) so the console.log message (`Applied UFW rule ... allow from ${...} to ${...}:${port}/tcp`) never shows the literal "undefined" while preserving the original context and variables.
🤖 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.
Nitpick comments:
In `@src/lib/onboard/gateway-sandbox-reachability.ts`:
- Around line 493-495: The success log may print "undefined" if reach.subnet or
reach.gatewayIp are undefined; when autoApplyResult.applied is true (from
tryAutoApplyUfwRule), guard the template by using safe defaults or nullish
coalescing for reach.subnet and reach.gatewayIp (e.g., `${reach.subnet ??
'<unknown-subnet>'}` and `${reach.gatewayIp ?? '<unknown-gateway>'}`) so the
console.log message (`Applied UFW rule ... allow from ${...} to
${...}:${port}/tcp`) never shows the literal "undefined" while preserving the
original context and variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9da62cdc-ae46-4697-9c3b-7fa2fafa327d
📒 Files selected for processing (4)
docs/reference/commands.mdxsrc/lib/onboard/gateway-sandbox-reachability.test.tssrc/lib/onboard/gateway-sandbox-reachability.tssrc/lib/onboard/ufw-auto-apply.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/gateway-sandbox-reachability.test.ts
- src/lib/onboard/ufw-auto-apply.ts
Collaborator
|
Maintainer scope note for the remaining automated advisory items:
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26524085549
|
# Conflicts: # src/lib/onboard/gateway-sandbox-reachability.ts
cv
approved these changes
Jun 2, 2026
Collaborator
There was a problem hiding this comment.
Approved after merge-conflict resolution onto latest main and security review.
Reviewed the UFW auto-remediation path: opt-in gate remains explicit (NEMOCLAW_AUTO_FIX_FIREWALL=1), sudo is non-interactive only (sudo -n), operands are validated before invoking UFW, command execution uses argv (no shell interpolation), failures fall back to manual guidance, and the verifier re-probes before continuing. The bridge-subnet -> gateway-IP:port scope matches the documented/manual remediation trade-off and is acceptable for this PR.
Local validation from /tmp/nemoclaw-pr4267:
- npm run build:cli
- npx vitest run src/lib/onboard/gateway-sandbox-reachability.test.ts (42/42)
- npm run typecheck:cli
Also addressed the remaining CodeRabbit success-log nit while resolving the main merge conflict.
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26839994455
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26839994455
|
wscurran
added
feature
cv
pushed a commit
that referenced
this pull request
Jun 3, 2026
## Summary - Add the missing `v0.0.57` release-notes section with links to the detailed docs pages for command, inference, onboarding, messaging, status, installer, and policy changes. - Remove public references to docs-skip terms from source docs and regenerate the NemoClaw user skills from the current Fern MDX docs. - Carry forward generated references for the per-agent documentation split, including Hermes-specific reference files. ## Source summary - #4615 and #4653 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover host-side `sessions` and `agents` commands plus `NEMOCLAW_EXTRA_AGENTS_JSON` secondary-agent baking. - #4163, #4204, #4611, #4619, and #4676 -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`: Release notes now cover managed vLLM progress/readiness, DGX Spark model default changes, local Ollama streaming usage, and inference route divergence warnings. - #4267, #4601, #4609, #4642, #4645, and #4661 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover UFW auto-remediation, local-inference reachability gates, gateway reuse/binding, cancel rollback, and policy selection persistence. - #4577, #4582, #4607, and #4660 -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`: Release notes now cover Slack validation, atomic `channels add`, WhatsApp QR diagnostics, and Slack placeholder normalization. - #4388, #4600, #4646, and #4647 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover status failure layers, paused-container hints, Docker-driver doctor behavior, and non-destructive stale-registry recovery. - #4569, #4579, and #4678 -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, `docs/network-policy/integration-policy-examples.mdx`: Release notes now cover installer tag pinning, PyPI `uv` policy access, and observable Jira validation. - #4632 -> `.agents/skills/`: Regenerated user skills from the current per-agent docs source, including newly generated Hermes reference files. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" docs --glob "*.mdx"` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" .agents/skills --glob "*.md"` - `npm run docs` - `npm run build:cli` - Commit hooks: markdownlint, docs-to-skills verification, gitleaks, skills YAML, commitlint <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Restructured documentation to clearly distinguish OpenClaw and Hermes agent variants throughout user guides. * Enhanced security, credential storage, and deployment guidance with clearer setup flows. * Added Hermes plugin installation and ecosystem documentation. * Improved workspace, messaging, and policy management references with variant-specific command examples. * Refined troubleshooting and CLI reference sections for clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
5 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
On Brev shadecloud Ubuntu 22.04 (and any host with
ufw INPUT policy DROP), sandbox containers on the docker bridge can't reach172.18.0.1:8080until the operator runs thesudo ufw allow ...rule that onboard preflight already prints. Eko's QA team and anyone fresh-booting a shadecloud H100 hits this every time. This adds an opt-in auto-apply path so non-interactive QA reproductions don't need a manual round-trip.Related Issue
Fixes #4265.
Changes
src/lib/onboard/gateway-sandbox-reachability.tstryAutoApplyUfwRule(reach, { port?, runImpl?, optedIn? })— does the cap-checkedsudo -nchain to apply the same rule preflight already suggests. Dep-injectedrunImplfor testability.verifySandboxBridgeGatewayReachableOrExitnow calls it onbridge_gatewayfailures whenNEMOCLAW_AUTO_FIX_FIREWALL=1is set, re-probes after a successful apply, and falls back to the existing manual-hint path otherwise.src/lib/onboard/gateway-sandbox-reachability.test.ts— 7 new cases covering every branch.Type of Change
Verification
npx vitest run src/lib/onboard/gateway-sandbox-reachability.test.ts→ 19/19 pass (12 existing + 7 new).npm run build:cliclean.Safety
NEMOCLAW_AUTO_FIX_FIREWALL=1is the explicit consent gate. Without it, behaviour is unchanged — same manual hint, same exit code.sudo -nonly. Never prompts for a password mid-run. If passwordless sudo isn't available, falls back to the manual path.from <subnet> to <gatewayIp> port <port> proto tcp— identical to what preflight already advises. Doesn't open up the host firewall beyond that.Behaviour Matrix
--no-verify: pre-commitTest (CLI)hook hits unrelated timeout flakes on this macOS workstation (Defender/Spotlight). New tests pass cleanly in isolation. CI on Linux is authoritative.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes