feat(onboard): enforce one OpenShell gateway lifecycle authority#6842
feat(onboard): enforce one OpenShell gateway lifecycle authority#6842souvikDevloper wants to merge 18 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a versioned gateway-management contract, ownership and attachment validation, host runtime probing, and onboarding enforcement that prevents NemoClaw from performing lifecycle effects on externally supervised gateways. Documentation and navigation describe configuration, validation failures, and remediation. ChangesGateway lifecycle authority
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant GatewayState
participant HostRuntime
participant OwnershipRules
Onboarding->>GatewayState: resolve gateway owner
GatewayState->>HostRuntime: collect listener and health evidence
HostRuntime-->>GatewayState: return attachment probe
GatewayState->>OwnershipRules: validate external ownership
OwnershipRules-->>GatewayState: allow attachment or return failure
GatewayState->>Onboarding: record owner metadata and advance
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 4 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
6b6763a to
0298d72
Compare
Canonical onboarding recognized the packaged OpenShell gateway user service or a standalone gateway it started itself. When a platform image supervises the gateway with its own service, neither was detected, so onboarding fell through to the standalone cutover: it reaped the live listener, the platform service restarted it, and the host was left with a persistent bind conflict and no authoritative owner. Make ownership explicit and enforce it before any effect. - Add a public, versioned gateway-management contract declaring the mode, endpoint, state location, expected service/runtime identity, and required capabilities. Parsing is fail-closed: an unknown version, capability, or field is an error, never a silent downgrade to self-management. Unknown fields are rejected so credentials cannot travel through metadata that is persisted and emitted. - Resolve exactly one lifecycle owner per run and guard every gateway lifecycle effect against it. An externally supervised gateway is attached to and validated, never started, stopped, or replaced -- including via the standalone fallback. - Make the gateway FSM handler attach rather than manage when supervision is declared, and fail before any provider, policy, sandbox, or registry effect on an inactive supervisor, an unknown listener, a mismatched identity, or multiple owners. Resume revalidates the owner instead of trusting a recorded complete step. - Guard startDockerDriverGateway itself, covering the rebuild and recovery callers that can also start a gateway. - Report redacted owner, source, and attachment state through machine events. Host-side wiring lives in a new onboard/gateway-host-runtime module, which also takes over getGatewayStartEnv, keeping src/lib/onboard.ts net-neutral per the codebase-growth guardrail. Refs NVIDIA#6576
0298d72 to
9f83c99
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/onboard/gateway-management.ts (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
SUPERVISOR_KINDSfrom a single source of truth, matching the capabilities pattern.
GatewaySupervisorKindis a standalone union whileSUPERVISOR_KINDSis a separately maintainedSet— unlikeSUPPORTED_GATEWAY_CAPABILITIES/GatewayCapabilityin this same file, which derive the type from oneas constarray. If a new kind is added to the union but theSetisn't updated, valid declarations of that kind silently fail closed with no compile-time signal.♻️ Proposed fix
-export type GatewaySupervisorKind = "systemd-system" | "systemd-user" | "external"; +export const SUPPORTED_GATEWAY_SUPERVISOR_KINDS = ["systemd-system", "systemd-user", "external"] as const; +export type GatewaySupervisorKind = (typeof SUPPORTED_GATEWAY_SUPERVISOR_KINDS)[number];-const SUPERVISOR_KINDS = new Set<GatewaySupervisorKind>([ - "systemd-system", - "systemd-user", - "external", -]); +const SUPERVISOR_KINDS = new Set<string>(SUPPORTED_GATEWAY_SUPERVISOR_KINDS);Also applies to: 85-89
🤖 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-management.ts` at line 50, Make GatewaySupervisorKind and SUPERVISOR_KINDS share one source of truth, following the SUPPORTED_GATEWAY_CAPABILITIES/GatewayCapability pattern: define the supervisor kinds in an as-const collection, derive the union type from its elements, and construct the Set from that collection. Update the declarations around GatewaySupervisorKind and SUPERVISOR_KINDS without changing the supported kind values.src/lib/onboard/gateway-host-runtime.ts (2)
84-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
require("node:child_process")flagged by static analysis — likely a false positive here.
spawnSyncImplis invoked with an argument array (["systemctl", ...scope, "is-active", supervisor.serviceName]), not a shell string, so this isn't exploitable OS command injection. Still, using a lazyrequire()instead of a top-levelimport { spawnSync } from "node:child_process"mixes CJS/ESM styles for no apparent reason, sincedeps.spawnSyncImplalready exists as the test seam.♻️ Optional cleanup
+import { spawnSync as defaultSpawnSync } from "node:child_process"; + ... - const spawnSyncImpl = - deps.spawnSyncImpl ?? - (require("node:child_process") as typeof import("node:child_process")).spawnSync; + const spawnSyncImpl = deps.spawnSyncImpl ?? defaultSpawnSync;🤖 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-host-runtime.ts` around lines 84 - 93, Update the spawnSyncImpl initialization in the supervisor status-checking function to use a top-level spawnSync import from node:child_process as its default, while preserving deps.spawnSyncImpl as the test override. Remove the lazy require expression and keep the existing argument-array invocation and result handling unchanged.Source: Linters/SAST tools
137-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant condition.
stableGatewayImageis only non-null whenopenshellVersionis truthy, sostableGatewayImage && openshellVersioncan just checkopenshellVersion.🤖 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-host-runtime.ts` around lines 137 - 147, Simplify the condition in the stable gateway image setup to check only openshellVersion, since stableGatewayImage is derived from it and is non-null whenever it is truthy. Preserve the existing environment assignments and applyOverlayfsAutoFix behavior inside the conditional.docs/deployment/gateway-lifecycle-authority.mdx (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit combined sentences onto separate lines; remove em dashes.
Several paragraphs pack two sentences onto one line (14, 16, 60, 65, 75, 85), and lines 16 and 65 use em dashes.
✏️ Proposed fix (representative lines)
-Platform images sometimes supervise the gateway with their own service. If canonical onboarding does not know that, both layers believe they own the same gateway and the same port: onboarding replaces the live listener, the platform service immediately restarts it, and the host is left with a persistent bind conflict and no authoritative owner. +Platform images sometimes supervise the gateway with their own service. +If canonical onboarding does not know that, both layers believe they own the same gateway and the same port: onboarding replaces the live listener, the platform service immediately restarts it, and the host is left with a persistent bind conflict and no authoritative owner. -The gateway-management contract makes that ownership explicit. NemoClaw either manages the packaged canonical gateway service, or it attaches to a gateway that an external supervisor owns — never both, and it never silently falls back to starting its own gateway when external supervision was declared. +The gateway-management contract makes that ownership explicit. +NemoClaw either manages the packaged canonical gateway service, or it attaches to a gateway that an external supervisor owns, never both, and it never silently falls back to starting its own gateway when external supervision was declared.As per coding guidelines, "Keep one sentence per line in Markdown and MDX source files" and "Avoid filler, hype, rhetorical questions, emoji, em dashes, and unnecessary bold text."
Also applies to: 16-16, 60-60, 65-65, 75-75, 85-85
🤖 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/deployment/gateway-lifecycle-authority.mdx` at line 14, Update the Markdown/MDX paragraphs on the referenced lines so each sentence occupies its own source line. Replace em dashes in the paragraphs around lines 16 and 65 with appropriate punctuation or sentence breaks, while preserving the existing meaning and content.Source: Coding guidelines
🤖 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/gateway-host-runtime.ts`:
- Around line 59-79: Remove the cachedOwner state and recompute the gateway
owner on every getGatewayOwner() call using the current declaration and
hasOpenShellGatewayUserService() result. Keep malformed declaration errors and
the existing resolveGatewayOwner inputs unchanged so assertGatewayStartAllowed()
and FSM ownership checks use fresh state.
In `@src/lib/onboard/machine/handlers/gateway.ts`:
- Around line 278-282: Remove the unused session parameter from
attachToExternallySupervisedGateway and update its call site to stop passing
session; preserve the existing returned session from
deps.recordStepComplete("gateway").
---
Nitpick comments:
In `@docs/deployment/gateway-lifecycle-authority.mdx`:
- Line 14: Update the Markdown/MDX paragraphs on the referenced lines so each
sentence occupies its own source line. Replace em dashes in the paragraphs
around lines 16 and 65 with appropriate punctuation or sentence breaks, while
preserving the existing meaning and content.
In `@src/lib/onboard/gateway-host-runtime.ts`:
- Around line 84-93: Update the spawnSyncImpl initialization in the supervisor
status-checking function to use a top-level spawnSync import from
node:child_process as its default, while preserving deps.spawnSyncImpl as the
test override. Remove the lazy require expression and keep the existing
argument-array invocation and result handling unchanged.
- Around line 137-147: Simplify the condition in the stable gateway image setup
to check only openshellVersion, since stableGatewayImage is derived from it and
is non-null whenever it is truthy. Preserve the existing environment assignments
and applyOverlayfsAutoFix behavior inside the conditional.
In `@src/lib/onboard/gateway-management.ts`:
- Line 50: Make GatewaySupervisorKind and SUPERVISOR_KINDS share one source of
truth, following the SUPPORTED_GATEWAY_CAPABILITIES/GatewayCapability pattern:
define the supervisor kinds in an as-const collection, derive the union type
from its elements, and construct the Set from that collection. Update the
declarations around GatewaySupervisorKind and SUPERVISOR_KINDS without changing
the supported kind values.
🪄 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: bb43b065-6f72-4ba9-b3f0-5d772c1ed1d9
📒 Files selected for processing (11)
docs/deployment/gateway-lifecycle-authority.mdxdocs/index.ymlsrc/lib/onboard.tssrc/lib/onboard/gateway-host-runtime.tssrc/lib/onboard/gateway-management.test.tssrc/lib/onboard/gateway-management.tssrc/lib/onboard/gateway-ownership.test.tssrc/lib/onboard/gateway-ownership.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/onboard/machine/handlers/gateway.tssrc/lib/onboard/machine/initial-flow-phases.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard/gateway-host-runtime.ts (1)
59-79: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStale gateway owner cached for the process lifetime.
cachedOwneris memoized in thecreateGatewayHostRuntime()closure. Once resolved, later calls never re-readNEMOCLAW_GATEWAY_MANAGEMENTorhasOpenShellGatewayUserService(), soassertGatewayStartAllowed()and the FSM ownership checks can act on stale ownership if state changes mid-process (e.g., across rebuild/recovery attempts in the same run). This was flagged on a previous commit of this file and remains unresolved.♻️ Proposed fix
export function createGatewayHostRuntime(deps: GatewayHostRuntimeDeps): GatewayHostRuntime { - let cachedOwner: GatewayOwner | null = null; - /** * Resolve the one gateway lifecycle authority for this process. A malformed * declaration throws instead of degrading to self-management: a host that * meant to hand the gateway to an external supervisor must never silently get * a second NemoClaw-owned gateway on the same port. */ function getGatewayOwner(): GatewayOwner { - if (cachedOwner) return cachedOwner; const loaded = loadGatewayManagementDeclaration(); if (!loaded.ok) { throw new Error(`Invalid gateway management declaration: ${loaded.reason}`); } - cachedOwner = resolveGatewayOwner({ + return resolveGatewayOwner({ declaration: loaded.declaration, hasPackagedService: hasOpenShellGatewayUserService(), }); - return cachedOwner; }🤖 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-host-runtime.ts` around lines 59 - 79, Remove the process-lifetime memoization in createGatewayHostRuntime and make getGatewayOwner re-read loadGatewayManagementDeclaration and hasOpenShellGatewayUserService on every call before resolving ownership. Preserve the existing malformed-declaration error behavior and ensure assertGatewayStartAllowed and FSM ownership checks receive the current GatewayOwner.
🤖 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/gateway-host-runtime.ts`:
- Around line 81-93: Add a short timeout option to the spawnSyncImpl invocation
in isSupervisorUnitActive, alongside the existing UTF-8 encoding configuration.
Preserve the current result.error and result.status === null handling so
timed-out systemctl probes return null.
---
Duplicate comments:
In `@src/lib/onboard/gateway-host-runtime.ts`:
- Around line 59-79: Remove the process-lifetime memoization in
createGatewayHostRuntime and make getGatewayOwner re-read
loadGatewayManagementDeclaration and hasOpenShellGatewayUserService on every
call before resolving ownership. Preserve the existing malformed-declaration
error behavior and ensure assertGatewayStartAllowed and FSM ownership checks
receive the current GatewayOwner.
🪄 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: b1124431-4a4f-4095-8d44-818fd6981cdd
📒 Files selected for processing (11)
docs/deployment/gateway-lifecycle-authority.mdxdocs/index.ymlsrc/lib/onboard.tssrc/lib/onboard/gateway-host-runtime.tssrc/lib/onboard/gateway-management.test.tssrc/lib/onboard/gateway-management.tssrc/lib/onboard/gateway-ownership.test.tssrc/lib/onboard/gateway-ownership.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/onboard/machine/handlers/gateway.tssrc/lib/onboard/machine/initial-flow-phases.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/index.yml
- docs/deployment/gateway-lifecycle-authority.mdx
- src/lib/onboard/machine/initial-flow-phases.test.ts
- src/lib/onboard/gateway-management.test.ts
- src/lib/onboard/machine/handlers/gateway.test.ts
- src/lib/onboard/machine/handlers/gateway.ts
- src/lib/onboard.ts
- src/lib/onboard/gateway-ownership.test.ts
- src/lib/onboard/gateway-ownership.ts
…d endpoint Addresses two blocking findings from the PR review advisor. The ownership guard sat only in startDockerDriverGateway(), so a non-Docker-driver host could still reach `openshell gateway start` through startGatewayWithOptions() or a recovery branch, launching a second gateway on a host whose external supervisor already owns one. Move the guard to the shared start entrypoint and add it to recoverGatewayRuntime() and to startGatewayForRecovery(), which reaches a raw start on the cross-port and non-default-name branches. Attachment also probed the process-global gateway port rather than the declared endpoint, so a declaration naming a different port was assessed against a different local listener. Probe the declared endpoint, and reject a declaration whose port is not the one this process operates, since every downstream consumer is bound to that port. Enforce requiredCapabilities on the effect path instead of only at parse time. OpenShell exposes no gateway capability-discovery endpoint, so capabilities are validated against what this build implements. That is recorded as a source boundary rather than papered over with invented discovery. Refs NVIDIA#6576
|
Thanks — both advisor blockers were real. Fixed in
Per the recommendation, the guard now sits on the shared start entrypoint (
Endpoint: the probe now targets Capabilities: One thing I want to be straight about rather than quietly satisfy: I did not implement gateway-side capability discovery, because OpenShell exposes no endpoint to discover capabilities from. Inventing a probe would be worse than the gap. What the check does honestly do is fail closed when a declaration requires a capability this NemoClaw build does not implement — which is the version-skew case the contract must not let through (newer platform profile, older NemoClaw). It's recorded as an explicit source boundary in If maintainers would rather not ship a partially-verifiable field, dropping
|
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/onboard/gateway-ownership.ts`:
- Around line 244-251: Update the capability-mismatch message in the gateway
ownership declaration flow to use plural wording when listing unsupported
capabilities, while preserving the existing unsupported.join(", ") values and
failure behavior.
🪄 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: 6491f606-50fe-4500-bc6e-c53116df9505
📒 Files selected for processing (9)
docs/deployment/gateway-lifecycle-authority.mdxsrc/lib/onboard.tssrc/lib/onboard/gateway-host-runtime.tssrc/lib/onboard/gateway-ownership.test.tssrc/lib/onboard/gateway-ownership.tssrc/lib/onboard/gateway-recovery.test.tssrc/lib/onboard/gateway-recovery.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/onboard/machine/initial-flow-phases.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/onboard/gateway-ownership.test.ts
- src/lib/onboard.ts
- docs/deployment/gateway-lifecycle-authority.mdx
- src/lib/onboard/gateway-host-runtime.ts
- src/lib/onboard/machine/handlers/gateway.test.ts
| return { | ||
| ok: false, | ||
| code: "capability_unsupported", | ||
| message: | ||
| `The declaration requires gateway capability ${unsupported.join(", ")}, which this NemoClaw ` + | ||
| `build does not provide. Onboarding stops before any effect rather than attaching to a gateway ` + | ||
| `it cannot drive as declared.`, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pluralize the capability-mismatch message.
unsupported.join(", ") can contain multiple capability names, but the message reads "requires gateway capability X, Y" — grammatically awkward for more than one item.
✏️ Proposed wording fix
code: "capability_unsupported",
message:
- `The declaration requires gateway capability ${unsupported.join(", ")}, which this NemoClaw ` +
+ `The declaration requires gateway ${unsupported.length > 1 ? "capabilities" : "capability"} ${unsupported.join(", ")}, which this NemoClaw ` +
`build does not provide. Onboarding stops before any effect rather than attaching to a gateway ` +
`it cannot drive as declared.`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| ok: false, | |
| code: "capability_unsupported", | |
| message: | |
| `The declaration requires gateway capability ${unsupported.join(", ")}, which this NemoClaw ` + | |
| `build does not provide. Onboarding stops before any effect rather than attaching to a gateway ` + | |
| `it cannot drive as declared.`, | |
| }; | |
| return { | |
| ok: false, | |
| code: "capability_unsupported", | |
| message: | |
| `The declaration requires gateway ${unsupported.length > 1 ? "capabilities" : "capability"} ${unsupported.join(", ")}, which this NemoClaw ` + | |
| `build does not provide. Onboarding stops before any effect rather than attaching to a gateway ` + | |
| `it cannot drive as declared.`, | |
| }; |
🤖 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-ownership.ts` around lines 244 - 251, Update the
capability-mismatch message in the gateway ownership declaration flow to use
plural wording when listing unsupported capabilities, while preserving the
existing unsupported.join(", ") values and failure behavior.
wscurran
added
area: onboarding
|
✨ Thanks for the fix, @souvikDevloper. Making gateway ownership explicit with a versioned contract should resolve the persistent bind conflict. Ready for maintainer review. Related open issues:
Related open issues: |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Blocking review on the current head (2d60fe3):\n\n1. The product-scope gate is not satisfied. This PR creates a supported, versioned external-supervisor integration and publishes it as canonical documentation, but #6576, its parent #6401, and the supplying profile #6404 are all still open with needs: design. Before this can be approved, an accepted issue or design decision needs to establish the supported surface and its ownership, lifecycle, compatibility, security, and validation expectations.\n\n2. A valid systemd-owned gateway cannot attach through the current host probe. gateway-host-runtime.ts:123 calls getDockerDriverGatewayPortListenerScan(), whose returned PID set is filtered through Docker-driver process recognition and requireDockerDriverEnv (docker-driver-gateway-port-listener.ts:111-119 and docker-driver-gateway-runtime.ts:451-454). A normal declared systemd-system executable without those markers is discarded and then rejected as unknown_listener before execPath can be compared. Externally supervised attachment needs raw, complete listener enumeration independent of Docker-driver identity, plus a host-runtime test for a real declared systemd listener.\n\n3. The refactor regresses configured gateway ports. gateway-host-runtime.ts imports the module-load GATEWAY_PORT constant and uses it in getGatewayStartEnv(), while onboard.ts maintains the authoritative mutable gateway binding selected for the run. CI reproduces this in test/gateway-start-wait.test.ts: the requested 9443 environment receives 8080 values. Pass the current authoritative port into the host runtime (preferably through an accessor) and test both configured and resumed bindings.\n\n4. The declaration-controlled endpoint is passed directly to the HTTP readiness request, but parseEndpoint accepts arbitrary HTTP(S) hosts. Constrain it to the explicitly supported local gateway origins before any request; add negative coverage for remote, link-local, and metadata-style destinations.\n\n5. The synchronous systemctl is-active probe has no timeout. Add a short timeout and preserve the existing null/unprobeable handling so onboarding cannot hang indefinitely on a wedged supervisor query.\n\nThe branch also currently fails DCO (the PR body has no Signed-off-by declaration), CLI typecheck in the new gateway handler test, the CLI shard above, and the aggregate/E2E gate. Please get the exact-head required checks green after the design and code blockers are resolved.
…red endpoint Addresses the blocking review on 2d60fe3. A valid systemd-owned gateway could not attach. The attachment probe used getDockerDriverGatewayPortListenerScan(), whose PID set is filtered through Docker-driver process recognition and requireDockerDriverEnv. An ordinary systemd-run gateway carries none of those markers, so it was discarded and then rejected as unknown_listener before its execPath could be compared -- externally supervised attachment could never succeed. Add an unfiltered listener enumeration and use it for the ownership probe, keeping the Docker-driver-filtered scan for the callers that need runtime identity. The declaration-controlled endpoint was passed to the HTTP readiness request while parseEndpoint accepted arbitrary HTTP(S) hosts. Constrain the endpoint to the supported local gateway origins so a declaration cannot direct a request at a remote, link-local, or cloud-metadata address. The refactor also regressed configured gateway ports: the host runtime read the module-load GATEWAY_PORT constant, while onboard.ts owns the mutable authoritative binding. Pass the current port through an accessor, resolve the gateway env module lazily so a reloaded environment is honored, and cover both the configured and default bindings. Also from review: - Add a timeout to the synchronous systemctl is-active probe so a wedged supervisor query cannot hang onboarding; unprobeable still reports null. - Recompute the gateway owner per call instead of memoizing it, since onboarding can install the packaged service underneath a cached value. - Fix the CLI typecheck failure in the gateway handler test. - Drop the unused session parameter from the attach path. Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
Thanks for the thorough review, @cv. Points 2–5 were real; all are fixed in 1. Product-scope gate — acknowledged, and I'll defer to you. You're right that #6576, #6401, and #6404 are all still open with I don't think this should be approved ahead of that decision, and I'm not asking you to. Options, in the order I'd suggest:
Tell me which you prefer and I'll do it. I'd rather not add more code to this until that's settled. 2. Systemd-owned gateway could not attach — confirmed, and it made the feature inert. You're exactly right, and this was the worst bug in the PR: Added Coverage: a new 3. Configured gateway port regression — confirmed. The host runtime captured the module-load 4. Endpoint SSRF — confirmed.
5. Also from CodeRabbit: the owner is now recomputed per call rather than memoized (onboarding can install the packaged service underneath a cached value), and the unused CI: DCO sign-off added to the PR body. The CLI typecheck failure ( Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com |
|
may i get the ci now @cv |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Follow-up review on 27e65c0: the earlier implementation issues in points 2-5 are materially addressed, but the new raw-listener path exposes one additional ownership blocker.
The raw scan proves only that some live PID holds the port. In gateway-ownership.ts, identity checks are conditional on supervisor.execPath; because that field is optional, a declaration with execPath null accepts any live local listener that answers the health probe. Even when an executable path is declared, systemctl is-active and an executable-path match do not prove that the listening PID belongs to the declared unit. A separately launched process using the same binary can pass while the named unit is merely active. Bind the listener PID to the declared supervisor identity (for systemd, authoritative unit PID/cgroup evidence), or fail closed when that relationship cannot be established.
The new "real systemd listener" test does not exercise that boundary: it mocks getGatewayPortListenerRawScan(), cannot read /proc/4242/exe, and then overwrites listenerExecPath before evaluation. Add coverage for the raw enumeration and executable lookup, plus the PID-to-supervisor relationship and negative cases for a same-binary PID outside the unit and a missing execPath.
Security review verdict for this head: FAIL on supervisor/listener identity and its regression coverage. The separate product-scope gate also remains unresolved, as acknowledged above.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/gateway-host-runtime.test.ts (1)
36-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe real declared-endpoint readiness branch is never exercised.
createDeps()always injectsprobeGatewayHttpReady, so every test short-circuits beforewaitForDeclaredGatewayHttpReady's endpoint-targeted branch (constructing${owner.endpoint}/and callingisGatewayHttpReady) ingateway-host-runtime.ts(lines 133-139). Production wiring omits this override, so that URL-construction logic — the piece explicitly called out as fixed to "target the declared endpoint" — has no test coverage at all.Add at least one test that omits
probeGatewayHttpReadyand mocksisGatewayHttpReady/waitForGatewayHttpReadyfrom./gateway-http-readinessto assert the constructed URL matches the declared endpoint.As per path instructions, tests under
**/*.test.{ts,js,mts,mjs,cts,cjs}should be reviewed "for behavioral confidence rather than implementation lock-in," and broad mocks that bypass the behavior under test should be flagged.🤖 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-host-runtime.test.ts` around lines 36 - 52, Add a focused test for the real declared-endpoint readiness path in the gateway runtime tests by omitting the createDeps probeGatewayHttpReady override and mocking the gateway-http-readiness isGatewayHttpReady/waitForGatewayHttpReady dependencies. Assert that readiness invokes the check with the declared owner.endpoint plus a trailing slash, while preserving existing test behavior elsewhere.Source: Path instructions
🤖 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-host-runtime.test.ts`:
- Around line 36-52: Add a focused test for the real declared-endpoint readiness
path in the gateway runtime tests by omitting the createDeps
probeGatewayHttpReady override and mocking the gateway-http-readiness
isGatewayHttpReady/waitForGatewayHttpReady dependencies. Assert that readiness
invokes the check with the declared owner.endpoint plus a trailing slash, while
preserving existing test behavior elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95f7c99c-6bc2-44c1-b5ae-2542371698fc
📒 Files selected for processing (9)
src/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-port-listener.tssrc/lib/onboard/docker-driver-gateway-runtime.tssrc/lib/onboard/gateway-host-runtime.test.tssrc/lib/onboard/gateway-host-runtime.tssrc/lib/onboard/gateway-management.test.tssrc/lib/onboard/gateway-management.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/onboard/machine/handlers/gateway.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/onboard/machine/handlers/gateway.ts
- src/lib/onboard/gateway-management.test.ts
- src/lib/onboard.ts
- src/lib/onboard/gateway-management.ts
- src/lib/onboard/machine/handlers/gateway.test.ts
Addresses the follow-up security review on 27e65c0. The raw listener scan proved only that some live PID held the port, not that the PID belonged to the declared unit. Identity was gated on supervisor.execPath, which was optional, so a declaration with execPath absent accepted any live local listener that answered the health probe. Even with execPath declared, `systemctl is-active` plus an executable-path match do not prove the listening PID is the unit's: a separate process running the same binary passes both while the named unit is merely active. Bind the listening PID to the declared supervisor with authoritative cgroup evidence: - Make supervisor.execPath required, so there is always something to check the listening process against. - Read the listener's /proc/<pid>/cgroup and confirm it names the declared systemd unit. A same-binary process in a login session scope or the user slice does not match. When the relationship cannot be established (unreadable cgroup, non-systemd supervisor), attachment fails closed rather than trusting an executable-path match. Regression coverage: - cgroupBelongsToUnit: v1/v2 layouts, user-manager units, a login-session-scope impostor, and a prefix-collision unit. - The host-runtime "real systemd listener" test now drives the probe's own /proc/exe and /proc/cgroup reads and evaluates the probe output directly, instead of overwriting listenerExecPath before evaluation. Added a same-binary-outside-the-unit case and an unreadable-cgroup fail-closed case. - Raw listener enumeration is covered directly: it reports a listener the Docker-driver filter discards, drops dead PIDs, and flags an incomplete scan. Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
Thanks — you're right, and the raw-scan fix traded one hole for a subtler one. Fixed in The gap: the raw scan proved a live PID held the port, not that it was the declared unit's PID. Identity was gated on Fix — authoritative PID→unit binding:
So the new On the test you flagged — you were right that it mocked the scan, couldn't read
138 affected-suite tests pass; Product-scope gate (your point 1): still yours to call, and I'm treating it as unresolved. I don't think this should merge ahead of an accepted design on #6576/#6404. My standing offer: reduce this to just the ownership guard (a safety fix independent of the new public surface) and take the versioned contract to a design issue — or park it entirely. Happy to act on whichever you prefer. Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com |
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/onboard/gateway-ownership.test.ts`:
- Line 252: Rename the suite currently titled “cgroupBelongsToUnit” to a
behavior-oriented description of systemd cgroup ownership matching, without
changing the tests or implementation.
🪄 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: dddcef7b-3f28-4b92-b5ab-64df6af47df5
📒 Files selected for processing (10)
docs/deployment/gateway-lifecycle-authority.mdxsrc/lib/onboard/docker-driver-gateway-port-listener.test.tssrc/lib/onboard/gateway-host-runtime.test.tssrc/lib/onboard/gateway-host-runtime.tssrc/lib/onboard/gateway-management.test.tssrc/lib/onboard/gateway-management.tssrc/lib/onboard/gateway-ownership.test.tssrc/lib/onboard/gateway-ownership.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/onboard/machine/initial-flow-phases.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/onboard/gateway-host-runtime.test.ts
- src/lib/onboard/gateway-management.test.ts
- src/lib/onboard/machine/initial-flow-phases.test.ts
- docs/deployment/gateway-lifecycle-authority.mdx
- src/lib/onboard/machine/handlers/gateway.test.ts
- src/lib/onboard/gateway-ownership.ts
| }); | ||
| }); | ||
|
|
||
| describe("cgroupBelongsToUnit", () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a behavior-oriented suite title.
cgroupBelongsToUnit exposes the private helper name rather than the behavior under test; name the systemd cgroup ownership-matching behavior instead.
Suggested change
-describe("cgroupBelongsToUnit", () => {
+describe("systemd cgroup ownership matching", () => {As per coding guidelines, “Use behavior-oriented test titles.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("cgroupBelongsToUnit", () => { | |
| describe("systemd cgroup ownership matching", () => { |
🤖 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-ownership.test.ts` at line 252, Rename the suite
currently titled “cgroupBelongsToUnit” to a behavior-oriented description of
systemd cgroup ownership matching, without changing the tests or implementation.
Source: Coding guidelines
|
v0.0.88 disposition: deferred under the product-scope gate. This creates and documents a canonical external-supervisor authority and destructive cleanup contract, while #6576, parent #6401, and supplying profile #6404 still lack an accepted decision covering ownership, lifecycle, compatibility, security, and validation. The implementation can be reconsidered after that product authority is recorded. |
|
Following the v0.0.88 disposition: I've posted a proposed design decision on #6576 (#6576 (comment)) covering the five dimensions named in the disposition — ownership, lifecycle, compatibility, security, and validation — scoped inside the #6401 boundary, with the #6404 profile touchpoint pinned in a companion comment there. If maintainers accept or amend it, I'll align this PR to whatever is recorded so it can be re-reviewed against the accepted contract. |
…uctive under external supervision The NVIDIA#6576 review asked for a regression test through the full preflight path: the original defect was two guarded stages followed by an unguarded orphan-cleanup block, a composition class that per-stage unit tests cannot catch. Extract the reuse-reconcile -> stale-cleanup -> orphan-cleanup sequence into onboard/preflight-gateway-sequence.ts, composed exactly as onboard ran it inline and consuming the one authority resolved before the sequence. onboard.ts now makes a single call (net -17 lines). The new suite drives the whole sequence end-to-end: - under external supervision, every hostile input (missing metadata with an orphan-looking container, stale, active-unnamed, foreign-active, stale container plus image drift) crosses the full path with zero destructive calls and an unchanged reuse state; - under NemoClaw ownership, orphan removal, stale-session destruction, and the stage-to-stage reuse-state handoff still behave exactly as the inline sequence did. Refs NVIDIA#6576 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
Acceptance evidence from the July 16 review is now complete in |
The codebase-growth guardrail requires changed test files not to add if statements; fold the two-step inspect behavior into one expression. Refs NVIDIA#6576 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
v0.0.89 revalidation: the later commits and focused sequence test address the destructive externally-supervised preflight cleanup called out in my prior review. The remaining disposition is deferred for product scope, not branch state or a missing test: #6576, #6401, and #6404 still carry |
Superseded by later commits. The remaining blocker is the unresolved product-scope decision recorded in the current maintainer deferral comment.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewing exact head c2e422693ab10b2616efd1b626f66f3a42019d65, I still see three correctness/security-boundary blockers:
-
The owner binding is not durable across resume.
src/lib/onboard/gateway-host-runtime.ts:85-123keepsboundOwneronly in a module closure, while the onboarding session persists no gateway-owner identity. A new-process--resumeafter the declaration is removed or changed binds a fresh NemoClaw-managed owner and can re-enter lifecycle effects instead of recognizing an explicit authority migration. Persist a secret-free owner identity in the run checkpoint/session, compare it before effects on resume, and fail closed on drift. Also scope/reset the in-memory binding for a genuinely new in-process run. Add regression coverage for external -> absent/managed declaration across process-resume boundaries. -
Attachment does not bind downstream OpenShell commands to the validated endpoint.
src/lib/onboard/machine/handlers/gateway.ts:278-308probes and advances without registering or selectingowner.endpoint; meanwhilesrc/lib/onboard.ts:4185-4188forcesOPENSHELL_GATEWAYto the derived NemoClaw name. On a fresh host with no prior registration, or with an ambient sibling selected, the listener can validate while provider/sandbox operations target no gateway or a different gateway. Register/select and verify the exact declared gateway identity before advancing, then test both no-prior-registration and ambient-sibling cases through a downstream mutation boundary. -
Accepted HTTPS declarations cannot pass the production readiness probe.
src/lib/onboard/gateway-management.ts:142-143acceptshttps:, butsrc/lib/onboard/gateway-host-runtime.ts:185-193routes it toisGatewayHttpReady, whose implementation atsrc/lib/onboard/gateway-http-readiness.ts:103-113always usesnode:http; Node rejects an HTTPS URL withERR_INVALID_PROTOCOL, which is caught and reported as not ready. Either reject HTTPS in contract v1 or implement the authenticated HTTPS/gRPC probe and trust material required by the gateway. Cover the accepted HTTPS path without the test-only probe override.
Separately, the product-scope dependency remains open: #6576, #6401, and #6404 still carry needs: design. The proposed ownership/lifecycle/compatibility/security/validation contract should be accepted or amended before this surface is published as canonical behavior.
Persist and revalidate gateway ownership across resume, bind the exact declared endpoint, and reject unsupported HTTPS declarations in contract v1. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
8 participants
Problem
Refs #6576 (parent epic #6401).
Canonical onboarding recognizes two things: the packaged OpenShell gateway user service, or a standalone gateway it starts itself. A platform image that supervises the gateway with its own system service is neither, so
hasOpenShellGatewayUserService()returns false andstartDockerDriverGateway()falls straight through to the standalone cutover — which reaps the live listener while the platform service immediately restarts it. The result is the persistent:8080bind conflict described in the issue, with neither layer authoritative.The root cause is that gateway ownership was implicit: whichever code path ran first became the owner.
What this changes
Ownership becomes explicit and is enforced before any effect.
1. A public, versioned gateway-management contract (
src/lib/onboard/gateway-management.ts)Declares the mode, endpoint, state location, expected service/runtime identity, and required capabilities. It is platform-neutral — no Brev/GCP control-plane concepts in core NemoClaw — and is consumable by the platform profile in #6404 either as a file (
NEMOCLAW_GATEWAY_MANAGEMENT) or as an in-process declaration.{ "version": 1, "mode": "externally-supervised", "endpoint": "http://127.0.0.1:8080", "stateDir": "/var/lib/openshell/gateway", "supervisor": { "kind": "systemd-system", "serviceName": "openshell-gateway.service", "execPath": "/usr/local/bin/openshell-gateway" }, "requiredCapabilities": ["sandbox.create"] }Parsing is fail-closed. An unrecognized version, an unknown capability, or a malformed field is an error — never a silent downgrade to "no declaration", since treating a bad declaration as absent is exactly how a host ends up with two authorities. Unknown fields are rejected rather than ignored, so a credential cannot travel through a surface that gets persisted and emitted.
2. One resolved owner, and effect enforcement (
src/lib/onboard/gateway-ownership.ts)resolveGatewayOwner()establishes exactly one authority per run.assertGatewayEffectAllowed()guardsstart/stop/restart/destroy/replace/standalone-fallbackagainst it: an externally supervised gateway is attached to and validated, never started, stopped, or replaced. Standalone fallback is prohibited outright in that mode.evaluateGatewayAttachment()is a pure function deciding whether attachment is safe, and fails on an inactive supervisor, an absent listener, an unknown listener, an unenumerable listener set, a mismatched executable identity, or multiple owners.3. The gateway FSM handler enforces the owner before all effects and on resume
When supervision is declared, the handler takes an attach-only path that runs no destructive effect and fails before any provider, policy, sandbox, or registry mutation. Resume takes the same path as a fresh run — a recorded
completegateway step is not evidence that the declared owner still holds the port.4.
startDockerDriverGateway()is guarded directlyThat function is the only path that starts a gateway process and is reachable from onboarding, rebuild, and recovery, so the guard sits there too rather than only in the FSM.
5. Redacted ownership in machine events
Owner mode, source, endpoint, and supervisor identity flow into
advanceTometadata viadescribeGatewayOwner(). No credential value is persisted or emitted.When nothing is declared, behavior is unchanged: the packaged service if installed, otherwise standalone — but the owner is now explicit rather than implied.
Scope
This is the upstream contract and enforcement the issue asks for. It deliberately does not include:
brevdev/nemoclaw-imagecutover (Migrate the Brev web wizard from direct OpenShell orchestration to canonical onboarding #6405) and lifecycle parity evidence (Restore canonical lifecycle, policy, provider, and integration semantics on Brev #6406);statusCLI and read-only diagnostics — only machine events are wired here;Happy to split further or fold any of those in if maintainers prefer.
Testing
47 new tests;
npm run typecheck:cliand biome clean.I verified against
upstream/mainthat this introduces no new test failures (the failing suites in my local checkout fail identically on a clean baseline for an unrelated missing dependency).Note the
direct-credential-envrepo check also fails on a cleanupstream/maincheckout here, so it is pre-existing and unrelated.Summary by CodeRabbit
Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com