fix(uninstall): isolate scoped gateway shutdown - #8677
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughThe PR scopes gateway identity and cleanup to each state directory. It validates process and systemd ownership before signaling, fails closed when proof is missing, and expands unit, integration, documentation, and live E2E coverage. ChangesGateway isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant UninstallRunPlan
participant Systemd
participant HostGatewayProcess
participant DefaultGateway
Operator->>UninstallRunPlan: request scoped uninstall
UninstallRunPlan->>Systemd: verify selected service MainPID and namespace
UninstallRunPlan->>HostGatewayProcess: validate and stop selected gateway
HostGatewayProcess-->>UninstallRunPlan: return ownership and port result
UninstallRunPlan-->>Operator: complete or preserve state on failure
DefaultGateway-->>Operator: remain available during survivor probes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 5816159 in the TypeScript / code-coverage/cliThe overall coverage in commit 5816159 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/lib/onboard/host-gateway-process.test.ts (1)
676-734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding scoped cases for a missing PID file and a dead recorded PID.
The table covers the identity mismatch vectors well. Two scoped branches in
stopHostGatewayProcesseshave no coverage here:
- Lines 794-816: the PID file is absent or unreadable while the marker still exists, or the port is still occupied. The expected outcomes are
"scoped gateway PID/runtime evidence is incomplete or invalid"and"gateway port 18080 is occupied without PID-file ownership evidence".- Lines 852-860: the recorded PID is dead but the port remains occupied. The expected outcome is
"recorded PID <pid> is dead but port 18080 remains occupied".Both branches decide whether runtime evidence is deleted, so a regression there would silently remove the operator's evidence. The fixture already supports this through
omitMarker,listenerPids, and theisPortFreedependency, so the cases are cheap to add.🤖 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/host-gateway-process.test.ts` around lines 676 - 734, Add table-driven cases to the existing stopHostGatewayProcesses coverage for an absent or unreadable PID file with the marker present, an occupied port without PID-file ownership evidence, and a dead recorded PID while the port remains occupied. Configure the existing scopedGatewayFixture options and isPortFree dependency to reach each branch, assert the specified ownership-failure messages, and verify runtime evidence is preserved rather than deleted.Source: Path instructions
src/lib/onboard/host-gateway-process.ts (1)
399-606: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting
scopedGatewayOwnershipProofinto staged proof helpers.The function runs about 15 sequential guards across roughly 200 lines and repeats the
{ cmdline, compatContainerName, reason }return shape at each rejection. The logic is correct, but the size makes each guard hard to review and hard to test in isolation.Consider extracting three stages that each return a reason or a value: state-file ownership (lines 413-455), runtime-marker identity (lines 457-492), and process/listener/Docker identity (lines 494-604). A small
reject(reason)helper would also remove the repeated literal return shape.This is a readability improvement only. Do not change the guard order, because the current order proves cheap evidence before it runs
ps,lsof, anddocker.🤖 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/host-gateway-process.ts` around lines 399 - 606, Refactor scopedGatewayOwnershipProof into staged helpers for state-file ownership, runtime-marker identity, and process/listener/Docker identity, preserving the existing guard order and behavior. Have each stage return either its proven value or a rejection reason, and add a small reject helper to centralize the repeated { cmdline, compatContainerName, reason } shape. Keep expensive process, listener, and Docker checks in the final stage after the cheaper evidence checks.Source: Coding guidelines
src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts (1)
23-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
withManagedGatewayAuthorityis duplicated across two uninstall test files. Both files define the same fixture helper. The bodies differ only in thesourcefield of the resolved authority. TheisPortFreedefault and thelsofavailability override are identical, so a future change to the scoped-teardown probe contract must be applied twice.
src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts#L23-L43: move this helper into a shared test-support module and accept thesourcevalue as a parameter.src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts#L21-L38: import the shared helper and pass"standalone"as thesourcevalue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts` around lines 23 - 43, Move withManagedGatewayAuthority from src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts#L23-L43 into a shared test-support module, adding a source parameter while preserving its existing defaults and authority fields. Update src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts#L23-L43 to import and use the shared helper with its current source value, and update src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts#L21-L38 to import it and pass "standalone".src/lib/actions/uninstall/run-plan.ts (3)
1360-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit the scoped teardown into named phases.
removeManagedDefaultGatewayUserServiceScopedspans about 245 lines and holds roughly twenty decision points. It covers target resolution, inactive-unit disable, pre-stop identity proof, drop-in install, post-reload revalidation, the stop, post-stop proof, and finalization.Extract the inactive-unit branch (1434-1492), the identity proof (1494-1552), and the finalization (1577-1603) into named helpers. The control flow already returns
booleanat every exit, so the split needs no behavior change.As per coding guidelines: "Keep function complexity low, follow the Biome configuration".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan.ts` around lines 1360 - 1604, Split removeManagedDefaultGatewayUserServiceScoped into named helpers for the inactive-unit cleanup, pre-stop identity/drop-in validation and stop flow, and post-stop finalization. Preserve the existing boolean outcomes, warnings, rollback behavior, and sequencing; keep target resolution in the main function while extracting the branches corresponding to inactive cleanup, identity proof, and finalization into focused helpers.Source: Coding guidelines
1181-1245: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare one executable-resolution helper instead of two.
runtimeProcessExecutableduplicatesprocessExecutableinsrc/lib/onboard/host-gateway-process.ts(lines 187-207). The two versions already differ: this one omits theps -p <pid> -o comm=fallback and omits thepath.isAbsolutecheck on thelsofvalue.Both versions decide process ownership before a SIGKILL path runs. Two copies can drift and return different verdicts for the same PID. Extract one adapter-owned helper and call it from both modules.
The same applies to
runtimeProcessStartIdentity,scopedGatewayListenerPids, andnormalizedExecutablePath. These are host and process boundary probes, so they belong behind an adapter rather than inside an uninstall action module.As per path instructions: "actions orchestrate, domain modules make pure decisions, adapters own host/process/network boundaries" and "Flag cross-layer cycles, duplicate sources of truth".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan.ts` around lines 1181 - 1245, Extract the host/process boundary helpers runtimeProcessExecutable, runtimeProcessStartIdentity, scopedGatewayListenerPids, and normalizedExecutablePath into one shared adapter-owned module, incorporating the existing processExecutable behavior including the comm= fallback and absolute-path validation. Update both run-plan and host-gateway-process to reuse these shared helpers, removing their local duplicates while preserving their current ownership and port-probing behavior.Source: Path instructions
1104-1110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for the rendered NemoClaw unit template
scripts/lib/openshell-gateway.service.inrenders one argument-freeExecStart, so the uninstall path accepts the installed unit. The fixture does not render this template. Add a test that asserts the managedExecStartremains argument-free.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan.ts` around lines 1104 - 1110, Add test coverage for the rendered NemoClaw unit template, verifying its managed ExecStart contains exactly one absolute, argument-free executable path so declaredGatewayServiceBinary accepts it. Render the template through the existing test fixture or setup path and assert the resulting unit preserves this contract.src/lib/actions/uninstall/run-plan-gateway-service.test.ts (2)
557-625: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd fail-closed cases for executable mismatch and a deleted executable.
The three cases cover listener ownership, fragment ambiguity, and UID mismatch. Two branches of the same ownership gate stay unexercised:
normalizedExecutablePath(processExecutable) !== normalizedExecutablePath(before.execStartPath)(run-plan.ts lines 1501-1502).processExecutable.endsWith(" (deleted)")(run-plan.ts lines 1500 and 1540).Both branches guard the SIGKILL path. The fixture already parameterizes identity, so each case needs one more entry plus a
readProcessExecutableoverride.💚 Proposed additional cases
{ label: "the managed process owner differs from the current user", listenerPid: 41_201, processUid: CURRENT_UID + 1, }, + { + executable: "/usr/bin/some-other-binary", + label: "the running executable differs from the unit ExecStart", + listenerPid: 41_201, + }, + { + executable: "/tmp/openshell-gateway (deleted)", + label: "the running executable was deleted on disk", + listenerPid: 41_201, + },Then read the override in the dependency block:
- readProcessExecutable: () => gatewayBin, + readProcessExecutable: () => identity.executable ?? gatewayBin,As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan-gateway-service.test.ts` around lines 557 - 625, Extend the parameterized identity cases in the uninstall test to cover an executable path mismatch and a process executable ending with “ (deleted)”. Add the corresponding identity data and make the readProcessExecutable dependency return the case-specific executable, ensuring both ownership-gate branches are exercised and the existing fail-closed assertions remain unchanged.Source: Path instructions
164-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
managedSystemdShowfor the synthesized inactive response.Lines 177-189 rebuild the systemd show fixture by hand.
managedSystemdShowalready produces this shape. The two copies already differ (KillSignal=15here,SIGTERMthere). Both values fail the production check, so behavior matches today, but the duplication can drift.♻️ Proposed fix
- const servicePath = getNemoclawOpenShellGatewayUserServicePath(test.home, test.env); - const gatewayBin = `${test.home}/.local/bin/openshell-gateway`; - return ok( - [ - `FragmentPath=${servicePath}`, - `ExecStart={ path=${gatewayBin} ; argv[]=${gatewayBin} ; }`, - "ExecStop=", - "ExecStopPost=", - "ActiveState=inactive", - "MainPID=0", - "Restart=on-failure", - "KillSignal=15", - "KillMode=control-group", - ].join("\n"), - ); + return ok(managedSystemdShow(test, { active: false, mainPid: 0 }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan-gateway-service.test.ts` around lines 164 - 192, Replace the hand-built inactive systemd response in the run callback with the existing managedSystemdShow fixture/helper, passing the same service and gateway context needed by that symbol. Preserve the current conditions that synthesize the response only for the matching systemctl show call with empty successful output, and remove the duplicated field list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts`:
- Around line 167-203: Replace the conditional dispatch logic in both
command-router mocks with signature-keyed lookup tables: update the kill and run
mocks at
src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts:167-203,
preserving events recording, and update the second run mock at
src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts:271-288,
preserving calls recording. Ensure both sites use the same lookup shape while
retaining their existing command responses and assertions.
---
Nitpick comments:
In `@src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts`:
- Around line 23-43: Move withManagedGatewayAuthority from
src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts#L23-L43 into a
shared test-support module, adding a source parameter while preserving its
existing defaults and authority fields. Update
src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts#L23-L43 to import
and use the shared helper with its current source value, and update
src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts#L21-L38 to
import it and pass "standalone".
In `@src/lib/actions/uninstall/run-plan-gateway-service.test.ts`:
- Around line 557-625: Extend the parameterized identity cases in the uninstall
test to cover an executable path mismatch and a process executable ending with “
(deleted)”. Add the corresponding identity data and make the
readProcessExecutable dependency return the case-specific executable, ensuring
both ownership-gate branches are exercised and the existing fail-closed
assertions remain unchanged.
- Around line 164-192: Replace the hand-built inactive systemd response in the
run callback with the existing managedSystemdShow fixture/helper, passing the
same service and gateway context needed by that symbol. Preserve the current
conditions that synthesize the response only for the matching systemctl show
call with empty successful output, and remove the duplicated field list.
In `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 1360-1604: Split removeManagedDefaultGatewayUserServiceScoped into
named helpers for the inactive-unit cleanup, pre-stop identity/drop-in
validation and stop flow, and post-stop finalization. Preserve the existing
boolean outcomes, warnings, rollback behavior, and sequencing; keep target
resolution in the main function while extracting the branches corresponding to
inactive cleanup, identity proof, and finalization into focused helpers.
- Around line 1181-1245: Extract the host/process boundary helpers
runtimeProcessExecutable, runtimeProcessStartIdentity,
scopedGatewayListenerPids, and normalizedExecutablePath into one shared
adapter-owned module, incorporating the existing processExecutable behavior
including the comm= fallback and absolute-path validation. Update both run-plan
and host-gateway-process to reuse these shared helpers, removing their local
duplicates while preserving their current ownership and port-probing behavior.
- Around line 1104-1110: Add test coverage for the rendered NemoClaw unit
template, verifying its managed ExecStart contains exactly one absolute,
argument-free executable path so declaredGatewayServiceBinary accepts it. Render
the template through the existing test fixture or setup path and assert the
resulting unit preserves this contract.
In `@src/lib/onboard/host-gateway-process.test.ts`:
- Around line 676-734: Add table-driven cases to the existing
stopHostGatewayProcesses coverage for an absent or unreadable PID file with the
marker present, an occupied port without PID-file ownership evidence, and a dead
recorded PID while the port remains occupied. Configure the existing
scopedGatewayFixture options and isPortFree dependency to reach each branch,
assert the specified ownership-failure messages, and verify runtime evidence is
preserved rather than deleted.
In `@src/lib/onboard/host-gateway-process.ts`:
- Around line 399-606: Refactor scopedGatewayOwnershipProof into staged helpers
for state-file ownership, runtime-marker identity, and process/listener/Docker
identity, preserving the existing guard order and behavior. Have each stage
return either its proven value or a rejection reason, and add a small reject
helper to centralize the repeated { cmdline, compatContainerName, reason }
shape. Keep expensive process, listener, and Docker checks in the final stage
after the cheaper evidence checks.
🪄 Autofix
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: cca0b901-2410-4bc0-8f8b-9e738d81b6dc
📒 Files selected for processing (13)
src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.tssrc/lib/actions/uninstall/run-plan-gateway-segregation.test.tssrc/lib/actions/uninstall/run-plan-gateway-service.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/onboard/docker-driver-gateway-prelaunch.test.tssrc/lib/onboard/docker-driver-gateway-prelaunch.tssrc/lib/onboard/gateway-process-identity.tssrc/lib/onboard/gateway-process-target-identity.tssrc/lib/onboard/host-gateway-process-target.test.tssrc/lib/onboard/host-gateway-process.test.tssrc/lib/onboard/host-gateway-process.tssrc/lib/tunnel/gateway-port-release-test-helpers.tstest/e2e/live/concurrent-gateway-ports.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/onboard/host-gateway-process.test.ts (1)
768-803: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference the sibling PID by a shared constant instead of the literal
9_990_808.
siblingPidis declared insidescopedGatewayFixtureat Line 96. These tests repeat its numeric value to model "a sibling owns the listener". IfsiblingPidchanges, the literal becomes an unrelated PID. The tests can then still pass while no longer exercising sibling listener ownership. Hoist the PID to a module-level constant and use it in both places.♻️ Proposed refactor
+const SIBLING_PID = 9_990_808; + function scopedGatewayFixture(options: ScopedGatewayFixtureOptions = {}) { const selectedPid = 9_991_880; - const siblingPid = 9_990_808; + const siblingPid = SIBLING_PID;const fixture = scopedGatewayFixture({ - listenerPids: [9_990_808], + listenerPids: [SIBLING_PID], omitMarker: true,const fixture = scopedGatewayFixture({ - listenerPids: [9_990_808], + listenerPids: [SIBLING_PID], portFree: false,🤖 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/host-gateway-process.test.ts` around lines 768 - 803, Hoist the sibling PID used by scopedGatewayFixture into a module-level constant, replace the fixture’s local siblingPid value and both test literals with that shared constant, and preserve the existing sibling-listener ownership scenarios.Source: Path instructions
src/lib/actions/uninstall/run-plan-gateway-service.test.ts (1)
222-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKey the default
systemctl showfallback on an explicit miss, not on an empty successful response.The wrapper substitutes
inactiveSystemdShowwhenever the delegated responder returns status 0 with empty stdout. A test that intentionally returnsok("")for thesystemctl showsignature is silently overridden with inactive metadata. Track whether the caller-suppliedrunhandled the signature instead.♻️ Proposed refactor
run: (command, args, options) => { const signature = commandSignature(command, args); - const response = defaultResponses.get(signature) ?? run(command, args, options); - const defaultSystemdShow = - response.status === 0 && response.stdout === "" - ? defaultSystemdShows.get(signature) - : undefined; - return defaultSystemdShow ?? response; + const seeded = defaultResponses.get(signature); + if (seeded !== undefined) return seeded; + const response = run(command, args, options); + // `run` defaults to `() => ok()`, which signals "unhandled" here. + const unhandled = run === defaultRun && response.status === 0 && response.stdout === ""; + return unhandled ? (defaultSystemdShows.get(signature) ?? response) : response; },
defaultRunis the() => ok()default destructured at Line 172.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan-gateway-service.test.ts` around lines 222 - 230, Update the run wrapper in the test to track whether defaultResponses.get(signature) explicitly matched before delegating to defaultRun. Apply the defaultSystemdShows fallback only when that lookup missed, not when the delegated response has status 0 and empty stdout, so caller-supplied ok("") responses remain unchanged.
🤖 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/actions/uninstall/run-plan-gateway-service.test.ts`:
- Around line 222-230: Update the run wrapper in the test to track whether
defaultResponses.get(signature) explicitly matched before delegating to
defaultRun. Apply the defaultSystemdShows fallback only when that lookup missed,
not when the delegated response has status 0 and empty stdout, so
caller-supplied ok("") responses remain unchanged.
In `@src/lib/onboard/host-gateway-process.test.ts`:
- Around line 768-803: Hoist the sibling PID used by scopedGatewayFixture into a
module-level constant, replace the fixture’s local siblingPid value and both
test literals with that shared constant, and preserve the existing
sibling-listener ownership scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7298a7d5-6bbe-46d6-9a3b-d7efcae2e7b3
📒 Files selected for processing (5)
src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.tssrc/lib/actions/uninstall/run-plan-gateway-service.test.tssrc/lib/onboard/host-gateway-process.test.tssrc/lib/onboard/host-gateway-process.tstest/e2e/live/concurrent-gateway-ports.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/uninstall/run-plan-gateway-process-isolation.test.ts
- test/e2e/live/concurrent-gateway-ports.test.ts
- src/lib/onboard/host-gateway-process.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-8677.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/onboard/host-gateway-process-target.test.ts (3)
185-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific rejection reason in each cross-match case.
Each case asserts only
ownershipFailures?.lengthis 1. Different cases trigger different guards, so a case can pass for the wrong reason and a regression in one guard can be masked by another.Two examples from
stopHostGatewayProcesses:
{ name: "nemoclaw", port: 18080 }is rejected by thecanonicalGatewayTargetMatchesprecondition, before any namespace or PID proof runs.{ name: "nemoclaw", port: 8080 }passes that precondition and is rejected later by the runtime-marker port comparison.The adjacent test at Line 200 already asserts the exact reason string. Apply the same assertion per case so each case proves the guard it names.
Line 121 also pins
isPortFree: () => true, so the "selected gateway port remains occupied after its process stopped" rejection is never exercised. Consider one case that returnsfalseafter the stop.♻️ Proposed test change
it.each([ - ["PID file", { pidFilePid: 9_999_602 }], - ["command line", { cmdline: "openshell-gateway[nemoclaw=nemoclaw;port=8080]" }], - ["gateway name", { name: "nemoclaw", port: 18080 }], - ["gateway port", { name: "nemoclaw", port: 8080 }], - ["loaded namespace", { namespace: "default" }], - ])("fails closed when a sibling cross-matches by %s (`#8663`)", (_case, overrides) => { + [ + "PID file", + { pidFilePid: 9_999_602 }, + "PID 9999602: PID file and runtime marker do not identify the same process", + ], + [ + "command line", + { cmdline: "openshell-gateway[nemoclaw=nemoclaw;port=8080]" }, + "PID 9999601: process command line does not identify the selected gateway name and port", + ], + [ + "gateway name", + { name: "nemoclaw", port: 18080 }, + "scoped cleanup requires one canonical name, port, and PID file", + ], + [ + "gateway port", + { name: "nemoclaw", port: 8080 }, + "PID 9999601: runtime marker does not identify the selected gateway", + ], + [ + "loaded namespace", + { namespace: "default" }, + "PID 9999601: gateway process owner and loaded sandbox namespace cannot be proven", + ], + ])("fails closed when a sibling cross-matches by %s (`#8663`)", (_case, overrides, reason) => { const { kill, pidFile, result } = stopScopedTarget(overrides); expect(result.stopped).toEqual([]); - expect(result.ownershipFailures?.length).toBe(1); + expect(result.ownershipFailures).toEqual([reason]); expect(kill).not.toHaveBeenCalled(); expect(fs.existsSync(pidFile)).toBe(true); });Verify each expected string against the current source before applying.
🤖 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/host-gateway-process-target.test.ts` around lines 185 - 198, Strengthen the parameterized cross-match test around stopScopedTarget and stopHostGatewayProcesses by asserting the exact ownership-failure reason expected for each override, matching the adjacent test’s reason-string assertion and current source text. Also add a case or adjust setup so isPortFree returns false after the process stops, exercising the occupied-port rejection path currently bypassed by the fixed true value.Source: Path instructions
121-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEvery new test pins the gateway port as free, so both port-based fail-closed rejections stay unexecuted.
stopHostGatewayProcessesadds two rejections that depend onisPortFreereturningfalse: "recorded process is dead but its selected port remains occupied" and "selected gateway port remains occupied after its process stopped". Both test files stubisPortFreeto always returntrue, so neither rejection runs in this PR. A regression in either guard would let uninstall report success while a sibling listener is still bound, which is the failure reported in issue#8663.
src/lib/onboard/host-gateway-process-target.test.ts#L121-L121: add a scoped case whereisPortFreereturnsfalseafter the stop, and assert the "port remains occupied after its process stopped" rejection with the PID file preserved.src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts#L26-L26: keepisPortFree: () => trueas the helper default, and add one segregation case that overrides it withisPortFree: () => falseto prove uninstall exits nonzero rather than removing the selected gateway state.🤖 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/host-gateway-process-target.test.ts` at line 121, Add coverage for both port-occupied fail-closed paths: in src/lib/onboard/host-gateway-process-target.test.ts:121-121, add a scoped case where isPortFree returns false after stopping and assert the “port remains occupied after its process stopped” rejection while preserving the PID file; in src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts:26-26, retain isPortFree: () => true as the default and add a segregation case overriding it to false that verifies uninstall exits nonzero without removing the selected gateway state.Source: Path instructions
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove temporary directories created by the test helpers. The helpers create directories with
fs.mkdtempSync, but teardown removes neither the directories nor their remaining files.🤖 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/host-gateway-process-target.test.ts` at line 78, Update the test helpers around stateDir creation with fs.mkdtempSync to track each temporary directory and remove it, including any remaining files, during teardown. Ensure cleanup runs for every directory created by the helpers without changing the test behavior.Source: Learnings
src/lib/actions/uninstall/run-plan-gateway-service.test.ts (1)
128-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the active-
MainPIDbranch and tighten thesystemctlassertion.Two points on this fixture and test.
First, Line 132 always answers the
MainPIDprobe with"0\n". InremoveNemoclawOpenShellGatewayUserService,mainPid === 0skipsprocessUsesStateScopedSandboxNamespacecompletely. No test in this file reaches themainPid > 0branch, which is the guard that proves a running service process belongs to the selected gateway. That guard is a stated objective of this PR. Add a case that returns a non-zeroMainPIDand a process environment whose namespace does not match, then assert the service survives.Second, Line 271 asserts that no
systemctlcommand was recorded. That holds only because the helper at Line 131 intercepts the--property=MainPIDprobe before delegating to the testrun. The source does invokesystemctlfor that probe. Assert the specific signaling command instead, so the test states the real guarantee.♻️ Proposed assertion change
expect(result.exitCode).toBe(1); expect(fs.existsSync(servicePath)).toBe(true); - expect(calls.some(([command]) => command === "systemctl")).toBe(false); + expect( + calls.some(([command, ...args]) => command === "systemctl" && args.includes("disable")), + ).toBe(false);Line 253 also calls
writeGatewayState(test)only for its return path and then overwrites the file. Assigning the path to a local first would make the intent clear.Also applies to: 250-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan-gateway-service.test.ts` around lines 128 - 133, Extend the tests around removeNemoclawOpenShellGatewayUserService with a non-zero MainPID and mismatched process namespace, asserting the selected service is preserved. Update the systemctl assertion to verify the specific signaling command rather than claiming no systemctl invocation occurred. In the test setup, store the path returned by writeGatewayState(test) in a local before overwriting the file.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.
Inline comments:
In `@src/lib/onboard/docker-driver-gateway-config.ts`:
- Around line 66-106: Update the onboarding upgrade/uninstall flow around
hasStateScopedSandboxNamespace to migrate or rewrite an existing legacy
openshell-gateway.toml so its Docker driver sandbox_namespace uses
gatewayIdForStateDir(stateDir) before scoped uninstall. Make the migration
idempotent and preserve valid scoped configurations, then add coverage for
upgrade, restart, resume, and uninstall paths.
In `@src/lib/onboard/host-gateway-process.ts`:
- Around line 214-216: Update the owner validation in the host-gateway identity
check to reject an empty trimmed `owner.stdout` before converting it with
`Number`. Preserve the existing nonzero-status and UID comparison checks,
ensuring `ps` output must contain a value that matches `uid`.
---
Nitpick comments:
In `@src/lib/actions/uninstall/run-plan-gateway-service.test.ts`:
- Around line 128-133: Extend the tests around
removeNemoclawOpenShellGatewayUserService with a non-zero MainPID and mismatched
process namespace, asserting the selected service is preserved. Update the
systemctl assertion to verify the specific signaling command rather than
claiming no systemctl invocation occurred. In the test setup, store the path
returned by writeGatewayState(test) in a local before overwriting the file.
In `@src/lib/onboard/host-gateway-process-target.test.ts`:
- Around line 185-198: Strengthen the parameterized cross-match test around
stopScopedTarget and stopHostGatewayProcesses by asserting the exact
ownership-failure reason expected for each override, matching the adjacent
test’s reason-string assertion and current source text. Also add a case or
adjust setup so isPortFree returns false after the process stops, exercising the
occupied-port rejection path currently bypassed by the fixed true value.
- Line 121: Add coverage for both port-occupied fail-closed paths: in
src/lib/onboard/host-gateway-process-target.test.ts:121-121, add a scoped case
where isPortFree returns false after stopping and assert the “port remains
occupied after its process stopped” rejection while preserving the PID file; in
src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts:26-26, retain
isPortFree: () => true as the default and add a segregation case overriding it
to false that verifies uninstall exits nonzero without removing the selected
gateway state.
- Line 78: Update the test helpers around stateDir creation with fs.mkdtempSync
to track each temporary directory and remove it, including any remaining files,
during teardown. Ensure cleanup runs for every directory created by the helpers
without changing the test behavior.
🪄 Autofix
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: aba65e9d-3f1a-4a73-bbe4-7edc77990702
📒 Files selected for processing (14)
docs/manage-sandboxes/uninstall-nemoclaw.mdxdocs/reference/commands.mdxsrc/lib/actions/uninstall/run-plan-gateway-segregation.test.tssrc/lib/actions/uninstall/run-plan-gateway-service.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/onboard/docker-driver-gateway-config.tssrc/lib/onboard/docker-driver-gateway-env.tssrc/lib/onboard/docker-driver-gateway-launch.test.tssrc/lib/onboard/docker-driver-gateway-launch.tssrc/lib/onboard/gateway-process-identity.tssrc/lib/onboard/gateway-process-target-identity.tssrc/lib/onboard/host-gateway-process-target.test.tssrc/lib/onboard/host-gateway-process.tstest/e2e/live/concurrent-gateway-ports.test.ts
💤 Files with no reviewable changes (1)
- src/lib/onboard/gateway-process-identity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/live/concurrent-gateway-ports.test.ts
| /** Prove that a NemoClaw-owned Docker gateway config uses its state-scoped namespace. */ | ||
| export function hasStateScopedSandboxNamespace(stateDir: string): boolean { | ||
| if (typeof process.getuid !== "function" || typeof fs.constants.O_NOFOLLOW !== "number") { | ||
| return false; | ||
| } | ||
| const configPath = path.join(stateDir, DOCKER_DRIVER_GATEWAY_CONFIG_NAME); | ||
| let descriptor: number | undefined; | ||
| try { | ||
| const state = fs.lstatSync(stateDir); | ||
| descriptor = fs.openSync(configPath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); | ||
| const config = fs.fstatSync(descriptor); | ||
| if ( | ||
| !state.isDirectory() || | ||
| state.isSymbolicLink() || | ||
| !config.isFile() || | ||
| config.nlink !== 1 || | ||
| state.uid !== process.getuid() || | ||
| config.uid !== state.uid || | ||
| config.size > 64 * 1024 | ||
| ) { | ||
| return false; | ||
| } | ||
| const expected = `sandbox_namespace = ${tomlString(gatewayIdForStateDir(stateDir))}`; | ||
| let inDriverTable = false; | ||
| const matches = fs | ||
| .readFileSync(descriptor, "utf-8") | ||
| .split(/\r?\n/) | ||
| .filter((line) => { | ||
| const trimmed = line.trim(); | ||
| if (trimmed.startsWith("[") && trimmed.endsWith("]")) { | ||
| inDriverTable = trimmed === "[openshell.drivers.docker]"; | ||
| return false; | ||
| } | ||
| return inDriverTable && trimmed.startsWith("sandbox_namespace ="); | ||
| }); | ||
| return matches.length === 1 && matches[0]?.trim() === expected; | ||
| } catch { | ||
| return false; | ||
| } finally { | ||
| if (descriptor !== undefined) fs.closeSync(descriptor); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every producer of openshell-gateway.toml and of sandbox_namespace entries.
set -euo pipefail
rg -n --type=ts -C4 'DOCKER_DRIVER_GATEWAY_CONFIG_NAME|openshell-gateway\.toml'
rg -n --type=ts -C3 'sandbox_namespace'
rg -n --type=ts -C6 'function tomlString'Repository: NVIDIA/NemoClaw
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -i 'docker-driver-gateway-config|onboard' src/lib || true
printf '%s\n' '--- tracked references ---'
git ls-files | rg '(^|/)(docker-driver-gateway-config|onboard)([^/]*|/.*)$' || true
rg -n -S -C4 'DOCKER_DRIVER_GATEWAY_CONFIG_NAME|openshell-gateway\.toml|sandbox_namespace|writeDockerDriverGatewayConfig|tomlString' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || trueRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/lib/onboard/docker-driver-gateway-config.ts"
printf '%s\n' '--- implementation outline ---'
ast-grep outline "$file" || true
printf '%s\n' '--- implementation ---'
cat -n "$file"
printf '%s\n' '--- exact symbol references ---'
rg -n --type=ts 'DOCKER_DRIVER_GATEWAY_CONFIG_NAME|writeDockerDriverGatewayConfig|hasStateScopedSandboxNamespace|gatewayIdForStateDir|sandbox_namespace' src/lib test src/commands 2>/dev/null || true
printf '%s\n' '--- config filename references outside TypeScript ---'
rg -n -S 'openshell-gateway\.toml|sandbox_namespace' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' --glob '!agents/**' 2>/dev/null || trueRepository: NVIDIA/NemoClaw
Length of output: 28818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- config and writer tests ---'
for file in \
src/lib/onboard/docker-driver-gateway-config-toml.test.ts \
src/lib/onboard/docker-driver-gateway-config-auth-contract.test.ts \
src/lib/onboard/docker-driver-gateway-launch.test.ts \
src/lib/onboard/docker-driver-gateway-cutover.ts \
src/lib/onboard/docker-driver-gateway-compat.ts \
src/lib/actions/uninstall/run-plan.ts
do
if [ -f "$file" ]; then
printf '\n### %s\n' "$file"
wc -l "$file"
rg -n -C5 'buildDockerDriverGatewayConfigToml|writeDockerDriverGatewayConfig|OPENSHELL_GATEWAY_CONFIG|openshell-gateway\.toml|sandbox_namespace|upgrade|legacy|compat|cutover|rename|copyFile|writeFile' "$file" || true
fi
done
printf '%s\n' '--- all config-builder and file-write call sites ---'
rg -n --type=ts -C3 'buildDockerDriverGatewayConfigToml|writeDockerDriverGatewayConfig|writeFileSync|writeFileAtomic|copyFileSync|renameSync' src/lib/onboard src/lib/actions test/e2e 2>/dev/null | rg -C2 'GatewayConfig|gateway config|openshell-gateway|sandbox_namespace|writeDocker|buildDocker|copyFileSync|renameSync|writeFileSync' || trueRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- direct call sites ---'
rg -n --type=ts 'buildDockerDriverGatewayConfigToml|writeDockerDriverGatewayConfig|prepareDockerDriverGatewayConfigEnv' \
src test --glob '!**/*.map' || true
printf '%s\n' '--- config TOML tests ---'
sed -n '1,260p' src/lib/onboard/docker-driver-gateway-config-toml.test.ts
printf '%s\n' '--- launch tests around namespace assertions ---'
sed -n '80,190p' src/lib/onboard/docker-driver-gateway-launch.test.ts
printf '%s\n' '--- cutover and compatibility source ---'
for file in src/lib/onboard/docker-driver-gateway-cutover.ts src/lib/onboard/docker-driver-gateway-compat.ts; do
printf '\n### %s\n' "$file"
cat -n "$file"
done
printf '%s\n' '--- migration references ---'
rg -n -i -C4 'docker.driver.gateway|gateway config|sandbox namespace|legacy identity|identity upgrade|migration|upgrade' \
docs/security src/lib/onboard test/onboard-gateway-legacy-identity-upgrade-runtime.test.ts \
--glob '!**/*.test.ts' 2>/dev/null || trueRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact builder/writer call sites ---'
rg -n --type=ts 'buildDockerDriverGatewayConfigToml|writeDockerDriverGatewayConfig|prepareDockerDriverGatewayConfigEnv' src test || true
printf '%s\n' '--- config TOML test assertions ---'
rg -n -C8 'sandbox_namespace|gatewayIdForStateDir|buildDockerDriverGatewayConfigToml|writeDockerDriverGatewayConfig|legacy|upgrade' \
src/lib/onboard/docker-driver-gateway-config-toml.test.ts || true
printf '%s\n' '--- launch test assertions ---'
rg -n -C8 'sandbox_namespace|gatewayIdForStateDir|buildDockerDriverGatewayConfigToml|legacy|upgrade' \
src/lib/onboard/docker-driver-gateway-launch.test.ts || true
printf '%s\n' '--- compatibility implementation ---'
wc -l src/lib/onboard/docker-driver-gateway-compat.ts
cat -n src/lib/onboard/docker-driver-gateway-compat.ts
printf '%s\n' '--- gateway upgrade test references ---'
rg -n -C8 'legacy|upgrade|config|namespace|gatewayIdForStateDir|openshell-gateway' \
test/onboard-gateway-legacy-identity-upgrade-runtime.test.ts src/lib/onboard/*gateway*test.ts 2>/dev/null | head -n 500 || trueRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime config preparation ---'
sed -n '240,285p' src/lib/onboard/docker-driver-gateway-env.ts
sed -n '105,155p' src/lib/onboard/docker-driver-gateway-launch.ts
sed -n '1,80p' test/support/openshell-gateway-config-helpers.ts
printf '%s\n' '--- uninstall ownership gate and tests ---'
sed -n '1000,1050p' src/lib/actions/uninstall/run-plan.ts
rg -n -C10 'hasStateScopedSandboxNamespace|sandbox_namespace|legacy|unscoped|state-scoped' \
src/lib/actions/uninstall src/lib/onboard/host-gateway-process-target.test.ts \
src/lib/onboard/docker-driver-gateway-config-auth-contract.test.ts \
src/lib/onboard/docker-driver-gateway-config-toml.test.ts || true
printf '%s\n' '--- OpenShell upgrade path ---'
rg -n -C8 'installOpenshell|needsUpgrade|upgrade|prepareDockerDriverGatewayConfigEnv|writeDockerDriverGatewayConfig' \
src/lib/onboard/openshell-install.ts src/lib/onboard/openshell-pin.ts src/lib/onboard.ts \
src/lib/onboard/docker-driver-gateway-env.ts src/lib/onboard/docker-driver-gateway-launch.ts || trueRepository: NVIDIA/NemoClaw
Length of output: 50371
Migrate legacy gateway TOML before scoped uninstall
openshell-install upgrades OpenShell without rewriting an existing openshell-gateway.toml. A legacy sandbox_namespace = "nemoclaw" therefore fails hasStateScopedSandboxNamespace, and scoped uninstall cannot stop the gateway. Add an idempotent migration or rewrite the file before uninstall. Add coverage for upgrade, restart, resume, and uninstall.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 89-90: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs
.readFileSync(descriptor, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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-config.ts` around lines 66 - 106,
Update the onboarding upgrade/uninstall flow around
hasStateScopedSandboxNamespace to migrate or rewrite an existing legacy
openshell-gateway.toml so its Docker driver sandbox_namespace uses
gatewayIdForStateDir(stateDir) before scoped uninstall. Make the migration
idempotent and preserve valid scoped configurations, then add coverage for
upgrade, restart, resume, and uninstall paths.
| const uid = typeof process.getuid === "function" ? process.getuid() : -1; | ||
| const owner = deps.run("ps", ["-p", String(pid), "-o", "uid="], { env: deps.env }); | ||
| if (owner.status !== 0 || Number(owner.stdout.trim()) !== uid) return false; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject an empty ps uid result explicitly.
Line 216 compares Number(owner.stdout.trim()) !== uid. Number("") evaluates to 0. If ps exits with status 0 but prints nothing, the owner check passes for a root-owned run (uid === 0) without any real ownership evidence. This is the first proof in the scoped identity chain, so it must fail closed.
removeNemoclawOpenShellGatewayUserService in src/lib/actions/uninstall/run-plan.ts already guards the same shape with !inspected.stdout.trim(). Apply the same guard here.
🛡️ Proposed fix
const uid = typeof process.getuid === "function" ? process.getuid() : -1;
const owner = deps.run("ps", ["-p", String(pid), "-o", "uid="], { env: deps.env });
- if (owner.status !== 0 || Number(owner.stdout.trim()) !== uid) return false;
+ const ownerUid = owner.stdout.trim();
+ if (owner.status !== 0 || !ownerUid || Number(ownerUid) !== uid) return false;📝 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.
| const uid = typeof process.getuid === "function" ? process.getuid() : -1; | |
| const owner = deps.run("ps", ["-p", String(pid), "-o", "uid="], { env: deps.env }); | |
| if (owner.status !== 0 || Number(owner.stdout.trim()) !== uid) return false; | |
| const uid = typeof process.getuid === "function" ? process.getuid() : -1; | |
| const owner = deps.run("ps", ["-p", String(pid), "-o", "uid="], { env: deps.env }); | |
| const ownerUid = owner.stdout.trim(); | |
| if (owner.status !== 0 || !ownerUid || Number(ownerUid) !== uid) return false; |
🤖 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/host-gateway-process.ts` around lines 214 - 216, Update the
owner validation in the host-gateway identity check to reject an empty trimmed
`owner.stdout` before converting it with `Number`. Preserve the existing
nonzero-status and UID comparison checks, ensuring `ps` output must contain a
value that matches `uid`.
3 participants
Summary
Generated Docker gateway configurations now assign one state-root-specific OpenShell sandbox namespace to each NemoClaw gateway.
Scoped uninstall proves the selected gateway configuration, running process namespace, PID state, name, and port immediately before signaling that process.
This keeps graceful Docker teardown scoped to the selected gateway, fails closed on ambiguous identity, and preserves full single-gateway uninstall behavior.
Related Issue
Fixes #8663
Changes
Both gateways used the Docker driver's implicit
defaultsandbox namespace, and graceful gateway shutdown stopped every managed container in that namespace.The linked CI evidence showed selected scoped state cleanup before the sibling sandbox destabilized, which ruled against shared NemoClaw state deletion as the cause.
sandbox_namespacefrom the current UID and full gateway state root.A process environment marker joins that namespace to runtime identity, so onboarding restarts a legacy process instead of accepting stale configuration.
The supported Podman schema does not expose this field, so Podman TOML omits it and scoped Podman process teardown fails closed while full single-gateway teardown remains unchanged.
Require an owned PID file and runtime marker, current-user process ownership, the loaded namespace, and exact gateway name and port before signaling.
Revalidate the proof immediately before SIGTERM and any SIGKILL fallback.
Missing or conflicting evidence returns nonzero without a host-wide process sweep or fuzzy match.
MainPIDto load the selected namespace beforesystemctl --user disable --now.Inactive services retain the existing scoped cleanup behavior.
Probe the surviving sandbox phase, listener, scoped
nemoclaw list, and dashboard HTTP three times with delays.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/uninstall-nemoclaw.mdx;docs/reference/commands.mdx; final re-review returnedno-doc-changeDGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run typecheck:cli,npm run checks:repository,npm run docs, and bothnpx prek run --from-ref main --to-ref HEADand the currentorigin/mainrange passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not run. The change is limited to gateway startup and scoped uninstall, and the targeted suites plus repository checks cover the changed boundaries.npm run docsbuilds without warnings (doc changes only)Live E2E
The
concurrent-gateway-portslive E2E is not claimed as passing.apt-getexited with status 100.concurrent-gateway-portsafter the trusted controller permits the target.The documentation build passed with zero errors and two Fern warnings.
Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
Tests