fix(onboard): gate the packaged gateway service on the version window - #8098
Merged
Conversation
NemoClaw prefers the upstream package-managed `openshell-gateway.service` whenever its unit file exists. That unit hard-codes an absolute ExecStart under `/usr/bin`, so when preflight finds a packaged OpenShell outside the blueprint version window and reinstalls a supported build into the user-local bin directory, the service still starts the packaged binary. Onboarding then drives a gateway it has already classified as unsupported: on a host carrying OpenShell 0.0.91 the gateway never reaches health and onboarding aborts, while the same host succeeds once the package is removed. Adopt the packaged unit only when the packaged `openshell-gateway` binary falls inside the blueprint range, and otherwise report it once and fall through to NemoClaw's own managed service. The check sits in the single resolver every consumer already routes through, so startup, status and teardown authority all agree on which service is eligible. A packaged binary whose version cannot be read is still adopted, keeping hosts that work today unchanged. Fixes #8094 Signed-off-by: yanyunl1991 <yanyunl@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:
📝 WalkthroughWalkthroughLinux onboarding validates the packaged OpenShell gateway’s effective executable path and version before adoption. Known incompatible versions and untrusted binaries are rejected. Supported or indeterminate versions remain adoptable, with fallback to the NemoClaw-managed service when required. ChangesGateway version gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LinuxServiceResolver
participant checkUpstreamGatewayVersion
participant PackagedGateway
participant NemoClawManagedService
LinuxServiceResolver->>LinuxServiceResolver: validate systemd ExecStart identity
LinuxServiceResolver->>checkUpstreamGatewayVersion: check packaged gateway version
checkUpstreamGatewayVersion->>PackagedGateway: read and parse executable version
PackagedGateway-->>checkUpstreamGatewayVersion: supported, unsupported, or unknown
checkUpstreamGatewayVersion-->>LinuxServiceResolver: return verdict
LinuxServiceResolver->>NemoClawManagedService: select fallback when package service is rejected
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8098.docs.buildwithfern.com/nemoclaw |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 6737a54 in the TypeScript / code-coverage/cliThe overall coverage in commit 6737a54 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
6 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard/docker-driver-gateway-service.ts (1)
189-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFold
spawnSyncImplinto thePicklist instead of intersecting it separately.
spawnSyncImplalready exists onOpenShellGatewayUserServiceOptions(line 48). Add it to thePickunion instead of re-declaring its type in a separate intersection member; this avoids two places to keep in sync if the field's type ever changes.♻️ Proposed refactor
export function checkUpstreamGatewayVersion( opts: Pick< OpenShellGatewayUserServiceOptions, - "env" | "existsSync" | "getUpstreamGatewayVersion" | "getUpstreamGatewayVersionBounds" - > & { spawnSyncImpl?: SpawnSyncLike } = {}, + | "env" + | "existsSync" + | "getUpstreamGatewayVersion" + | "getUpstreamGatewayVersionBounds" + | "spawnSyncImpl" + > = {}, ): UpstreamGatewayVersionVerdict {🤖 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/docker-driver-gateway-service.ts` around lines 189 - 194, Update the parameter type of checkUpstreamGatewayVersion to include "spawnSyncImpl" in the existing Pick list, and remove the separate intersection member that redeclares SpawnSyncLike.
🤖 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/onboard/docker-driver-gateway-service-version-gate.test.ts`:
- Around line 38-47: Remove the three `if (verdict.supported) return` guards in
the affected test cases and replace their type-narrowing role with a TypeScript
assertion on `verdict` after the existing
`expect(verdict.supported).toBe(false)` checks. Keep the assertions and message
expectations unchanged so all test bodies remain linear.
---
Nitpick comments:
In `@src/lib/onboard/docker-driver-gateway-service.ts`:
- Around line 189-194: Update the parameter type of checkUpstreamGatewayVersion
to include "spawnSyncImpl" in the existing Pick list, and remove the separate
intersection member that redeclares SpawnSyncLike.
🪄 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: 58773904-034f-461e-8f16-d640bb0aba35
📒 Files selected for processing (3)
docs/reference/architecture.mdxsrc/lib/onboard/docker-driver-gateway-service-version-gate.test.tssrc/lib/onboard/docker-driver-gateway-service.ts
The repository growth guardrail rejects newly added `if` statements in test files. Assert the rejected verdicts with `toMatchObject` instead of narrowing the result union with an early return. Signed-off-by: yanyunl1991 <yanyunl@nvidia.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
24 tasks
24 tasks
cv
previously requested changes
Aug 3, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Security review for commit ab31092bb4bb713c709df15097e15df7d8666ec6 — FAIL
This commit is not approval-ready. Required GitHub checks pass for its recorded base, but the branch is behind current main and the implementation has two correctness and security-boundary defects.
- Secrets and credentials — PASS. No credential material is added.
- Input validation and data sanitization — PASS. Version commands use argument arrays, and version bounds are validated by existing helpers.
- Authentication and authorization — PASS. No authentication or authorization surface changes.
- Dependencies and third-party libraries — PASS. No dependency changes.
- Error handling and logging — WARNING. Version-probe failures return
nulland permit adoption without preserving an actionable explanation that version evidence was unavailable. - Cryptography and data protection — PASS. No cryptographic behavior changes.
- Configuration and security headers — WARNING. The implementation deliberately adopts a managed gateway when its version cannot be determined. Because that gateway holds credentials and enforces policy, retaining this behavior needs explicit acceptance if it remains.
- Security testing — FAIL. Tests do not bind the checked version to the effective systemd
ExecStart, do not cover both candidate binary paths existing, and do not cover the existing development-channel exception. - System security — FAIL.
getOpenShellGatewayUserServiceBinaryPathsorders/usr/local/binbefore/usr/bin, andcheckUpstreamGatewayVersionchecks the first existing path. The resolver then adopts the unit from that verdict, while the effectiveExecStartis parsed only later at lines 509–539. With both binaries installed, NemoClaw can approve one binary while systemd starts a below-minimum binary.
The comparison at lines 202–205 also always rejects a version above the blueprint maximum. Existing preflight preserves recognized development builds through shouldAllowOpenshellAboveBlueprintMax; the service resolver needs the same policy and a regression test.
The stated #8103 recovery prerequisite is unresolved: PR #8118 explicitly leaves the managed-MCP destroy deadlock outside its scope. The PR description also has no Documentation Writer Review receipt even though documentation changes.
Smallest safe next step: complete the #8103 recovery path, bind version validation to the effective systemd ExecStart, preserve the development-channel policy, add the missing tests, refresh from current main, and complete the independent documentation writer review. Then rely on required GitHub checks for the updated commit.
15 tasks
senthilr-nv
reviewed
Aug 3, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Security review for f1dc0936206b221c3770bd481011e8a84742ed92 on base 8c542b214f15561f71b262523d80a61ed4a5e9c7 — FAIL
The effective-ExecStart correction closes the previously reported binary-selection and development-channel defects. The change is not safe to merge yet because it can expose the unresolved #8103 recovery deadlock for existing sandboxes whose recorded gateway owner changes from packaged-service to standalone.
| # | Category | Severity | File:Line | Description | Recommendation |
|---|---|---|---|---|---|
| 1 | System security | High | src/lib/onboard/docker-driver-gateway-service.ts:483 |
Declining an out-of-window package gateway can change the live owner to standalone. Issue #8103 documents that existing sandboxes with recorded packaged-service authority can then lose every sanctioned recovery path when managed MCP state is present. Its linked PR #8118 explicitly leaves that case incomplete. |
Complete the accepted #8103 recovery prerequisite first. Then merge current upstream/main into this branch, rerun exact-head validation and reviews, and obtain new exact-head human approval. |
- Secrets and credentials — PASS. No secret material, credential values, or new credential transport is added. The diff-aware gitleaks hook passed.
- Input validation and data sanitization — PASS. The gateway version command uses an argument array, the version parser accepts only a numeric semantic-version core, and the probed path comes from a trusted effective systemd identity.
- Authentication and authorization — PASS. No authentication or authorization contract changes. Existing service identity and trusted-path checks remain in force before lifecycle effects.
- Dependencies and third-party libraries — PASS. No package, image, download, or dependency changes.
- Error handling and logging — WARNING, accepted. An unreadable or unparsable version remains eligible without a new warning. This is an explicit, documented compatibility decision that preserves existing behavior and mirrors CLI preflight; service identity validation still fails closed. No follow-up is required for this issue-scoped fix.
- Cryptography and data protection — PASS. No cryptographic or persisted-data protection behavior changes.
- Configuration and security headers — WARNING, accepted.
NEMOCLAW_OPENSHELL_CHANNEL=devcontinues to allow a recognized above-maximum development build. This is the existing supported channel policy, now covered by a regression test. Browser security headers are outside this change. - Security testing — PASS. Focused coverage binds validation to effective
ExecStart, covers both candidate binaries existing, below-minimum, stable above-maximum, development above-maximum, in-window, unknown version, marked-service fallback, and warn-once behavior. The focused suite passed 52/52. - System security — FAIL. The version and trust boundary is now correct, but the unresolved #8103 authority-transition recovery failure blocks safe merge sequencing.
Files reviewed: docs/reference/architecture.mdx, src/lib/onboard/docker-driver-gateway-service.ts, and src/lib/onboard/docker-driver-gateway-service-version-gate.test.ts.
Product scope is separately approved for this exact head/base because the PR restores an existing supported Linux onboarding path under QA issue #8094 and creates no new supported product surface. That approval does not waive the #8103 correctness blocker.
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/onboard/docker-driver-gateway-service.ts (1)
486-511: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReject an untrusted upstream systemd identity before version gating
When
validateSystemdServiceIdentityreturnstrustFailure: true, do not convert the failure tonull.checkUpstreamGatewayVersion(null, ...)returnssupported: true, so the resolver adopts a foreign unit without warning. Decline the upstream unit and continue to the NemoClaw-managed fallback. Add a regression test for an untrustedFragmentPathorExecStart.🤖 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/docker-driver-gateway-service.ts` around lines 486 - 511, Update the upstream service resolution around validateSystemdServiceIdentity and checkUpstreamGatewayVersion so trustFailure immediately rejects the upstream unit instead of passing null and treating it as supported. Preserve version gating for trusted identities, continue to the NemoClaw-managed fallback, and add a regression test covering an untrusted FragmentPath or ExecStart.
🤖 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/onboard/docker-driver-gateway-service.ts`:
- Around line 486-511: Update the upstream service resolution around
validateSystemdServiceIdentity and checkUpstreamGatewayVersion so trustFailure
immediately rejects the upstream unit instead of passing null and treating it as
supported. Preserve version gating for trusted identities, continue to the
NemoClaw-managed fallback, and add a regression test covering an untrusted
FragmentPath or ExecStart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0ae86c3-db6b-462a-9ed9-2f1aefd1a8a3
📒 Files selected for processing (3)
docs/reference/architecture.mdxsrc/lib/onboard/docker-driver-gateway-service-version-gate.test.tssrc/lib/onboard/docker-driver-gateway-service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/docker-driver-gateway-service-version-gate.test.ts
- docs/reference/architecture.mdx
senthilr-nv
reviewed
Aug 3, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Security review for revision a71433ed93a2d1b298ace547fe05c7cee5945467 on base d756d15d867cdf91a90cad81971a8a29febe3002 — FAIL
The effective-ExecStart correction closes the previously reported binary-selection and development-channel defects. The change is not safe to merge yet because it can expose the unresolved #8103 recovery deadlock for existing sandboxes whose recorded gateway owner changes from packaged-service to standalone.
| # | Category | Severity | File:Line | Description | Recommendation |
|---|---|---|---|---|---|
| 1 | System security | High | src/lib/onboard/docker-driver-gateway-service.ts:485 |
Declining an out-of-window package gateway can change the live owner to standalone. Issue #8103 documents that existing sandboxes with recorded packaged-service authority can then lose every sanctioned recovery path when managed MCP state is present. Its linked PR #8118 remains open with changes requested and explicitly leaves that case incomplete. |
Complete the accepted #8103 recovery prerequisite first. Then merge current upstream/main into this branch, rerun revision-specific validation and reviews, and obtain new SHA-specific human approval. |
- Secrets and credentials — PASS. No secret material, credential values, or new credential transport is added. The diff-aware gitleaks hook passed.
- Input validation and data sanitization — PASS. The gateway version command uses an argument array, the version parser accepts only a numeric semantic-version core, and the probed path comes from a trusted effective systemd identity. Positive evidence of a foreign unit or executable now rejects the upstream service before version gating.
- Authentication and authorization — PASS. No authentication or authorization contract changes. Existing service identity and trusted-path checks remain in force before lifecycle effects.
- Dependencies and third-party libraries — PASS. No package, image, download, or dependency changes.
- Error handling and logging — WARNING, accepted. An unreadable or unparsable version remains eligible without a new warning. This is an explicit, documented compatibility decision that preserves existing behavior and mirrors CLI preflight; service identity validation still fails closed. No follow-up is required for this issue-scoped fix.
- Cryptography and data protection — PASS. No cryptographic or persisted-data protection behavior changes.
- Configuration and security headers — WARNING, accepted.
NEMOCLAW_OPENSHELL_CHANNEL=devcontinues to allow a recognized above-maximum development build. This is the existing supported channel policy, now covered by a regression test. Browser security headers are outside this change. - Security testing — PASS. Focused coverage binds validation to effective
ExecStart, covers both candidate binaries existing, below-minimum, stable above-maximum, development above-maximum, in-window, unknown version, untrusted identity rejection, marked-service fallback, and warn-once behavior. The focused suite passed 53/53. - System security — FAIL. The version and trust boundary is now correct, but the unresolved #8103 authority-transition recovery failure blocks safe merge sequencing.
Files reviewed: docs/reference/architecture.mdx, src/lib/onboard/docker-driver-gateway-service.ts, src/lib/onboard/docker-driver-gateway-service-version-gate.test.ts, and src/lib/onboard/docker-driver-gateway-service.test.ts.
Product scope is separately approved for revision a71433ed93a2d1b298ace547fe05c7cee5945467 on base d756d15d867cdf91a90cad81971a8a29febe3002 because the PR restores an existing supported Linux onboarding path under QA issue #8094 and creates no new supported product surface. That approval does not waive the #8103 correctness blocker.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
senthilr-nv
reviewed
Aug 3, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Security review for revision 5a981c3ed3e5a16425e04933472178d821fa2c6f on base 03fbf5ebf1dfc09506c5fefb1debe4b8766fde57 — FAIL
The effective-ExecStart correction closes the previously reported binary-selection and development-channel defects. The change is not safe to merge yet because it can expose the unresolved #8103 recovery deadlock for existing sandboxes whose recorded gateway owner changes from packaged-service to standalone.
| # | Category | Severity | File:Line | Description | Recommendation |
|---|---|---|---|---|---|
| 1 | System security | High | src/lib/onboard/docker-driver-gateway-service.ts:485 |
Declining an out-of-window package gateway can change the live owner to standalone. Issue #8103 documents that existing sandboxes with recorded packaged-service authority can then lose every sanctioned recovery path when managed MCP state is present. Its linked PR #8118 remains open with changes requested and explicitly leaves that case incomplete. |
Complete the accepted #8103 recovery prerequisite first. Then merge current upstream/main into this branch, rerun revision-specific validation and reviews, and obtain new SHA-specific human approval. |
- Secrets and credentials — PASS. No secret material, credential values, or new credential transport is added. The diff-aware gitleaks hook passed.
- Input validation and data sanitization — PASS. The gateway version command uses an argument array, the version parser accepts only a numeric semantic-version core, and the probed path comes from a trusted effective systemd identity. Positive evidence of a foreign unit or executable now rejects the upstream service before version gating.
- Authentication and authorization — PASS. No authentication or authorization contract changes. Existing service identity and trusted-path checks remain in force before lifecycle effects.
- Dependencies and third-party libraries — PASS. No package, image, download, or dependency changes.
- Error handling and logging — WARNING, accepted. An unreadable or unparsable version remains eligible without a new warning. This is an explicit, documented compatibility decision that preserves existing behavior and mirrors CLI preflight; service identity validation still fails closed. No follow-up is required for this issue-scoped fix.
- Cryptography and data protection — PASS. No cryptographic or persisted-data protection behavior changes.
- Configuration and security headers — WARNING, accepted.
NEMOCLAW_OPENSHELL_CHANNEL=devcontinues to allow a recognized above-maximum development build. This is the existing supported channel policy, now covered by a regression test. Browser security headers are outside this change. - Security testing — PASS. Focused coverage binds validation to effective
ExecStart, covers both candidate binaries existing, below-minimum, stable above-maximum, development above-maximum, in-window, unknown version, untrusted identity rejection, marked-service fallback, and warn-once behavior. The focused suite passed 53/53. - System security — FAIL. The version and trust boundary is now correct, but the unresolved #8103 authority-transition recovery failure blocks safe merge sequencing.
Files reviewed: docs/reference/architecture.mdx, src/lib/onboard/docker-driver-gateway-service.ts, src/lib/onboard/docker-driver-gateway-service-version-gate.test.ts, and src/lib/onboard/docker-driver-gateway-service.test.ts.
Product scope is separately approved for revision 5a981c3ed3e5a16425e04933472178d821fa2c6f on base 03fbf5ebf1dfc09506c5fefb1debe4b8766fde57 because the PR restores an existing supported Linux onboarding path under QA issue #8094 and creates no new supported product surface. That approval does not waive the #8103 correctness blocker.
23 tasks
senthilr-nv
reviewed
Aug 4, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Security review for revision 62e231b48cebc381e38b49978b7dce9db6fd6cb1 on base 3fb4ac1d37bec961f494ca39faf996c990b9d06b — FAIL
The version and service-identity checks remain correct after the refresh. Approval is blocked because declining a package service can expose the open #8103 recovery deadlock for an existing sandbox with recorded packaged-service authority and managed MCP state.
| # | Category | Severity | File:Line | Description | Recommendation |
|---|---|---|---|---|---|
| 1 | System security | High | src/lib/onboard/docker-driver-gateway-service.ts:485 |
A rejected package gateway can change the resolved lifecycle authority to standalone. Issue #8103 documents that managed MCP state can then block rebuild, recreate, and destroy recovery paths. PR #8118 explicitly excludes that managed MCP recovery case. |
Complete the #8103 managed MCP recovery path before approval. Refresh this PR afterward and rerun revision-specific evidence. |
- Secrets and credentials — PASS. The four-file diff adds no secret material or new credential transport.
- Input validation and data sanitization — PASS. The implementation validates the effective systemd unit and executable, uses argument arrays, and accepts only a numeric semantic-version core.
- Authentication and authorization — PASS. No authentication or authorization contract changes. Positive evidence of a foreign unit or executable rejects the package service before lifecycle effects.
- Dependencies and third-party libraries — PASS. The PR diff adds no dependency, image, package, or download.
- Error handling and logging — WARNING, accepted. A failed identity query or unreadable version preserves eligibility. This compatibility behavior is documented and keeps positive trust failures fail closed.
- Cryptography and data protection — PASS. No cryptographic or persisted-data protection behavior changes.
- Configuration and security headers — WARNING, accepted.
NEMOCLAW_OPENSHELL_CHANNEL=devpreserves the existing above-maximum development-build policy. Browser headers are outside this change. - Security testing — PASS. Focused tests cover both package binaries, trusted and untrusted identity, version boundaries, development builds, unknown versions, fallbacks, and warn-once behavior. The refreshed suite passed 53/53.
- System security — FAIL. The trust boundary is correct, but the resulting authority transition can expose the unresolved #8103 managed MCP recovery failure.
Files reviewed: docs/reference/architecture.mdx, src/lib/onboard/docker-driver-gateway-service.ts, src/lib/onboard/docker-driver-gateway-service-version-gate.test.ts, and src/lib/onboard/docker-driver-gateway-service.test.ts.
Product scope is separately approved for this revision and base because the PR restores the existing supported Linux onboarding path described by #8094 and creates no new supported surface. That decision does not waive the #8103 security blocker.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
reviewed
Aug 4, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Verdict
PASS for revision 790b035ff against base 3fb4ac1d3. The recovery change permits only the one-way NemoClaw-managed packaged-service to standalone transition at the transactional rebuild boundary. The replacement journal persists the selected standalone authority before managed MCP, provider, or sandbox mutation. Credential mutation, ordinary teardown, declaration changes, reverse migration, target changes, and all other authority drift remain fail-closed. No actionable security finding remains in the reviewed diff.
Findings Table
No findings.
Detailed Analysis
- Secrets and Credentials — PASS. No secret, credential, token, key, or credential-bearing fixture was added. Commit hooks passed gitleaks.
- Input Validation and Data Sanitization — PASS. The migration accepts no new user input. It requires the canonical gateway target and complete owner equality after substituting only the source discriminator.
- Authentication and Authorization — PASS. Provider and credential authority checks are unchanged. Credential mutation continues to reject the migration.
- Dependencies and Third-Party Libraries — PASS. No dependency or artifact pin changed.
- Error Handling and Logging — PASS. The reported recovery path no longer raises the authority-drift error. Other drift continues to abort before gateway effects without exposing secrets.
- Cryptography and Data Protection — PASS. No cryptographic or persisted sensitive-data contract changed. The journaled authority remains secret-free.
- Configuration and Security Headers — PASS. No network, container, permission, endpoint, header, or policy configuration changed.
- Security Testing — PASS. Regression tests cover the permitted one-way migration and rejection of ordinary teardown, credential mutation, reverse migration, and declaration changes. The journal test confirms the standalone owner is persisted. Focused validation passed 94/94 tests.
- System Security — PASS. Both owners must remain
nemoclaw-managed; gateway name, port, endpoint, state directory, supervisor, and required capabilities must match. Onlypackaged-servicetostandaloneis accepted, and only for rebuild. The journal is written before MCP teardown, provider mutation, or sandbox deletion.
Files Reviewed
docs/deployment/gateway-lifecycle-authority.mdxdocs/reference/architecture.mdxsrc/lib/actions/sandbox/rebuild-recreate-journal.test.tssrc/lib/actions/sandbox/rebuild-recreate-journal.tssrc/lib/onboard/docker-driver-gateway-service-version-gate.test.tssrc/lib/onboard/docker-driver-gateway-service.test.tssrc/lib/onboard/docker-driver-gateway-service.tssrc/lib/onboard/gateway-teardown-authority.test.tssrc/lib/onboard/gateway-teardown-authority.ts
Validation: focused tests 94/94; CLI and plugin builds; CLI type-check; repository checks; docs build and routes; pre-commit, commit-message, and pre-push hooks; diff check. Product scope is approved independently for the accepted #8094 compatibility fix and #8103 recovery path; no new supported integration or product surface is created.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
reviewed
Aug 4, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Security review: PASS for current revision 6737a54e5 on base a55620150.
- Secrets and credentials: PASS — no secret material or credential-handling expansion.
- Injection and command execution: PASS — version probing is limited to the effective executable path already validated against trusted package paths; arguments are fixed.
- Authentication and authorization: PASS — no authentication or permission boundary changes.
- Network exposure and SSRF: PASS — no new listener, route, egress, or URL-input surface.
- Filesystem and path safety: PASS — systemd unit and executable identities must match trusted normalized paths; foreign identities are declined.
- Sandbox and privilege boundaries: PASS — no privilege, capability, mount, or sandbox-policy expansion.
- Supply chain and dependencies: PASS — no dependency or artifact-source changes.
- Denial of service and resource use: PASS — the additional pre-start version probe is bounded by the existing 10-second timeout.
- Lifecycle and state integrity: PASS — the rebuild exception is one-way, requires full owner equality apart from
packaged-servicetostandalone, and is checkpointed before managed MCP, provider, or sandbox mutation. Startup rechecks the package gateway version after daemon reload and identity validation, before stop, enable, or restart.
No actionable security finding remains. Product scope is independently approved by accepted issues #8094 and #8103; this repairs existing onboarding and rebuild behavior and creates no new supported surface.
Collaborator
|
Disposition: the stale CodeRabbit linked-issue warning for #8103 is resolved. Revision 790b035 added the one-way packaged-service-to-standalone rebuild authority migration and regression coverage; revision 6737a54 retains it on the refreshed base. The current advisor reports 0 blockers, 0 warnings, and 0 suggestions. |
senthilr-nv
approved these changes
Aug 4, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed Files changed for revision 6737a54. All findings are addressed and required validation passes. Approved.
senthilr-nv
dismissed
cv’s stale review
August 4, 2026 05:41
All requested changes addressed in revision 6737a54; validation refreshed.
senthilr-nv
added
bug-fix
cv
added a commit
that referenced
this pull request
Aug 4, 2026
…8118) <!-- markdownlint-disable MD041 --> ## Summary `resolveGatewayEffectAuthority` refuses gateway effects when a sandbox's recorded gateway owner no longer matches the live one. Three callers let that refusal escape as an uncaught exception, so the CLI crashed with a raw Node stack trace instead of naming the migration. This PR raises the refusal as a typed error and reports it at those boundaries. Related report: issue 8103. This PR implements only the accepted reporting subset and does not close the remaining managed MCP recovery scope. ## Reproduction Executed on our DGX Spark aarch64 test host (GB10 GPU), Ubuntu 24.04, against `main` at `4cd4d64fe`. ```bash nemoclaw onboard --name repro-8103 --non-interactive --yes # records the live authority # reconstruct the pre-migration recording: source packaged-service, live standalone nemoclaw repro-8103 rebuild --yes nemoclaw repro-8103 destroy --yes --cleanup-gateway ``` **How the drift was produced.** The reporter's sandbox was onboarded while the packaged `openshell-gateway.service` was authoritative, then the managed gateway migrated to standalone. On our test host the packaged gateway service cannot start at all for an unrelated reason (its `EnvironmentFile` supplies a quoted `DOCKER_HOST`, so the value reaches the process with the quotes and the socket URL is invalid), so onboarding against it is not possible here. The recorded authority was therefore set to `packaged-service` directly in the onboarding checkpoint after a normal onboard, which reproduces the recorded-vs-live drift this issue is about while leaving the sandbox, gateway, and registry real. **Observed on `main` (before fix)** `rebuild --yes`: ```text Error: Gateway lifecycle authority changed since onboarding (nemoclaw@8080:nemoclaw-managed:packaged-service -> nemoclaw@8080:nemoclaw-managed:standalone). Changing authority requires a fresh onboarding run; gateway teardown will not perform gateway effects. at resolveGatewayEffectAuthority (.../dist/lib/onboard/gateway-teardown-authority.js:80:15) at resolveGatewayTeardownAuthority (.../dist/lib/onboard/gateway-teardown-authority.js:87:12) at openRebuildRecreateJournal (.../dist/lib/actions/sandbox/rebuild-recreate-journal.js:73:88) ... Node.js v22.22.2 ``` `destroy --yes --cleanup-gateway` crashed the same way from `cleanupGatewayAfterLastSandbox`. Both exited `1`. **Observed on `fix/...` (after fix)** — same host, same commit, same drifted state `rebuild --yes` (exit `1`, zero stack-trace lines): ```text ✓ State backed up (12 directories, 1 files) Refusing sandbox rebuild because the gateway lifecycle authority could not be revalidated. Gateway lifecycle authority changed since onboarding (nemoclaw@8080:nemoclaw-managed:packaged-service -> nemoclaw@8080:nemoclaw-managed:standalone). Changing authority requires a fresh onboarding run; gateway teardown will not perform gateway effects. Re-run onboarding to bind the current gateway authority before retrying. ``` `destroy --yes --cleanup-gateway` (exit `0`, zero stack-trace lines, registry empty afterwards): ```text Refusing shared gateway cleanup because the gateway lifecycle authority could not be revalidated. Gateway lifecycle authority changed since onboarding (...packaged-service -> ...standalone). ... Re-run onboarding to bind the current gateway authority before retrying. The shared NemoClaw gateway was left running. ✓ Sandbox 'repro-8103' destroyed ``` The verification script asserts the built `dist/` actually contains the change before running, because an earlier attempt silently tested an unpatched build. ## Analysis `resolveGatewayEffectAuthority` (`src/lib/onboard/gateway-teardown-authority.ts`) threw plain `Error`s. `credentials/command-support.ts` was the only caller that caught them, so `credentials add`/`reset` already reported the refusal cleanly. The remaining callers did not: - `rebuild-recreate-journal.ts` — reached from `rebuild --yes` - `onboard-recreate-journal.ts` — reached from `onboard --recreate-sandbox` - `destroy-gateway.ts` — reached from the final-sandbox gateway cleanup There is no process-level error boundary in the CLI entry, so each of those escaped as an uncaught exception and printed a Node stack trace. ## Fix - `GatewayAuthorityError` is a distinct type, so boundaries recognise the refusal without matching message text. All four refusal sites in the resolver raise it; an invalid management declaration deliberately keeps its own type because it is a different failure class. - `gatewayAuthorityFailureLines` is the single source of the wording `credentialsGatewayAuthorityFailureLines` already used. The remedy line is binary-agnostic so this module takes no branding dependency, which the source-architecture budget counts against every consumer. - The catches live in the two modules that already import the resolver, so no new import edges are added. `rebuild-recreate-journal` hands the lines back through a new `onAuthorityRefusal` callback and the pipeline routes it to its existing `bail`. - Shared gateway cleanup reports and returns instead of throwing. The sandbox and its registry entry are already gone at that point, so refusing the optional last step must not undo the removal. **The #6576 contract is preserved.** The catch sits at the resolver call, before any gateway effect, so nothing touches the gateway before authority is proven — the existing test asserting no side effects still passes unchanged, and a new case pins the same guarantee for the typed refusal. Any failure that is not an authority refusal still aborts outright. ## Scope This does not close issue 8103. The report also describes `destroy --yes` failing in MCP-bridge teardown with managed MCP servers present; that path raises `McpBridgeError`, a different root cause this PR does not touch. Our repro had no managed MCP servers and `destroy --yes` without `--cleanup-gateway` already succeeded on `main`, so that half is unverified here and left for a separate change. ## Interaction with #8098 #8098 makes NemoClaw decline a packaged gateway service whose binary is outside the blueprint version window, moving the resolved owner from `packaged-service` to `standalone`. That is exactly the drift this PR reports on, so landing this first (or together) keeps existing sandboxes on a working path. ## Changes - `src/lib/onboard/gateway-teardown-authority.ts`: add `GatewayAuthorityError` and `gatewayAuthorityFailureLines`; raise the typed error from every refusal site. - `src/lib/actions/sandbox/destroy-gateway.ts`: report an authority refusal and skip cleanup instead of throwing. - `src/lib/actions/sandbox/rebuild-recreate-journal.ts`: surface the refusal through an `onAuthorityRefusal` callback. - `src/lib/actions/sandbox/rebuild-pipeline.ts`: route that callback to the existing `bail`. - tests: regression coverage for the typed refusal, the shared wording, and the preserved no-side-effect guarantee. ## Type of Change - [x] 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Existing gateway lifecycle authority and sandbox destroy documentation already describes the unchanged fail-closed authority and optional cleanup boundaries; this PR replaces raw stack traces with existing-pattern operator guidance. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Exact-head nine-category security review at `27a3a1f13c7be9b6a7ab87a73585b99ec6de11e4` against base `e9fb170a2e3e4c4dcfa19e398232f71b8f2bace6`: PASS with no findings. Typed refusals remain fail closed, unrelated and malformed-declaration errors still propagate, diagnostics contain no credential material, and no gateway effect runs before authority is proven. Base commit #8145 changes separate Docker-context inference validation and does not interact with these typed errors. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Reviewed the complete eight-file diff at `27a3a1f13` against base `e9fb170a2e3e4c4dcfa19e398232f71b8f2bace6`, stable patch ID `2a1a9ce90bda5c03abf2788d381656fb42c301da`, and the NemoClaw writing guide, controlled word list, documentation contributor guide, and shared documentation-writing review contract. The change replaces raw Node.js stack traces with existing-pattern operator guidance when gateway lifecycle authority cannot be revalidated during rebuild, `onboard --recreate-sandbox`, or final shared-gateway cleanup. It adds no command, flag, default, configuration, schema, workflow, or supported product surface. Existing documentation already states that authority drift requires fresh onboarding and prevents gateway effects, while command documentation describes final shared-gateway cleanup after sandbox and registry deletion. Base commit #8145 adds credential-free Windows Ollama validation from Docker's network context in separate adapter and inference-selection files. It does not throw or consume `GatewayAuthorityError` and does not interact with the changed command-boundary handling. Exact-head validation passed: CLI and plugin builds; 6 focused files and 104 tests; `npm run validate:pr`; and `git diff --check`. No blocking findings. - Agent: Codex Desktop documentation writer subagent <!-- docs-review-head-sha: 27a3a1f --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## Verification - [x] Exact-head maintainer validation at `27a3a1f13`: 6 focused files / 104 tests passed; CLI and plugin builds passed; `npm run validate:pr` passed; `git diff --check` passed. - [x] PR description includes a `Signed-off-by:` line and every current commit appears as `Verified` in GitHub. - [x] `npx prek run` passes on the changed files - [x] `npm test` passes (640 files / 7228 tests under `src/lib/onboard`, `src/lib/actions`, `src/lib/credentials`) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Platform scope: reproduced and verified on aarch64 (DGX Spark), matching the reporter's environment. The changed code is platform-independent; x86_64 was not exercised. ## AI Disclosure - [x] AI-assisted — tool: Claude Code Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway teardown handling when ownership or authority cannot be confirmed. * Shared gateways remain running when authority checks fail, preventing unintended cleanup. * Sandbox rebuilds now report authority refusals clearly and stop safely. * CLI errors provide actionable details without exposing stack traces. * Unrelated errors continue to surface normally. * Gateway authority failure messages are standardized for invalid or changed authorization details. * **Tests** * Added coverage for authority changes, credential updates, invalid targets, declined checkpoints, malformed errors, and safe cleanup behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: yanyunl1991 <yanyunl@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
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
Linux onboarding now adopts the package-managed OpenShell gateway only when the effective binary selected by systemd is compatible with the blueprint version window.
It declines a known incompatible or untrusted package service, rechecks the version before changing service state, and continues through the marked-service or standalone fallback instead of pairing a supported CLI with an unsupported gateway.
Existing sandboxes can recover through a transactional rebuild when that compatibility decision moves NemoClaw-managed authority from the recorded package service to the standalone service; other authority changes remain fail-closed.
Related Issue
Closes #8094.
Closes #8103.
Changes
ExecStart, probe that binary, and compare its version withmin_openshell_versionandmax_openshell_version.NEMOCLAW_OPENSHELL_CHANNEL=dev; keep an undetermined version eligible to preserve existing behavior.systemctl --user daemon-reloadand identity validation, before stop, enable, or restart; this resolves advisor finding PRA-1 and is protected by the startup race regression test.packaged-servicetostandalonetransition at the transactional rebuild journal, before managed MCP teardown, provider mutation, or sandbox deletion.docs/reference/architecture.mdxanddocs/deployment/gateway-lifecycle-authority.mdxwith the adoption, pre-start validation, fallback, and recovery contracts.6737a54e5081449e4af06d3af3311cc9120307b4on basea5562015029fd8cdbebdce5664e8b8bfda9d6ba8. This fixes the existing supported Linux onboarding and managed recovery paths accepted in DGX Spark Playbook: [NemoClaw] express onboarding fails when a different OpenShell is already installed (aborts at inference setup; should detect/align/coexist) #8094 and [DGX Spark][Sandbox] rebuild throws uncaught 'Gateway lifecycle authority changed' and deadlocks all recovery for a sandbox onboarded across the gateway-service migration #8103. It creates no new integration, recipe, image, third-party stack, or supported product surface. Existing NemoClaw ownership, OpenShell compatibility bounds, security validation, transactional rebuild, and managed MCP preservation remain the support boundary.Type of Change
Quality Gates
6737a54e5081449e4af06d3af3311cc9120307b4on basea5562015029fd8cdbebdce5664e8b8bfda9d6ba8; all nine categories pass with no findings. Trusted systemd paths bound version probing, the pre-start check runs before service-state mutation, and the recovery migration is one-way with complete owner equality apart from the source discriminator.Documentation Writer Review
docs-updateddocs/reference/architecture.mdxanddocs/deployment/gateway-lifecycle-authority.mdx; independently reviewed all nine changed files against the writing guide, controlled terms, security meaning, and current base.npm run docs, route validation for 66 guarded pages, andgit diff --checkpassed. Fern reported 0 errors and two existing suppressed warnings. OpenClaw, Hermes, and Deep Agents generated variants are synchronized.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged, and no DGX Station review is claimed.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — all sixteen displayed commits are GitHub Verified at revision6737a54e5081449e4af06d3af3311cc9120307b4on basea5562015029fd8cdbebdce5664e8b8bfda9d6ba8.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — canonical-base diff-aware pre-commit, commitlint, and pre-push stages passed; the non-force push also passed normal pre-push hooks.vitestpassed 92/92 focused gateway-service, authority, journal, and rebuild tests after refresh;npm run build:cli,npm --prefix nemoclaw run build,npm run typecheck:cli,npm --prefix nemoclaw run typecheck, andnpm run checks:repositorypassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; this focused onboarding authority and recovery change is covered at its resolver, transaction-journal, rebuild, service, compatibility, and pre-start mutation boundaries. GitHub required CI is authoritative.npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and Fern's two existing suppressed warnings.Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Signed-off-by: Senthil Ravichandran senthilr@nvidia.com