Skip to content

fix(tunnel): release NemoClaw gateway port on stop (#5968) - #5988

Merged
ericksoa merged 29 commits into
mainfrom
fix/5968-stop-releases-gateway-port
Jul 3, 2026
Merged

fix(tunnel): release NemoClaw gateway port on stop (#5968)#5988
ericksoa merged 29 commits into
mainfrom
fix/5968-stop-releases-gateway-port

Conversation

@yimoj

@yimoj yimoj commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Legacy nemoclaw stop now releases its managed host gateway port without disrupting another registered sandbox that shares the gateway. Canonical nemoclaw tunnel stop remains tunnel-only and preserves the shared gateway. The onboard start/reuse path also enforces a single verified gateway listener before reuse or replacement, closing both symptoms in #5968 without breaking repeatable tunnel lifecycle.

Related Issue

Fixes #5968

Changes

  • Resolve the selected sandbox's persisted gateway binding fail-closed, emit a concise operator warning when registry lookup fails, and preserve the host gateway while another registered sandbox shares that canonical gateway.
  • Signal only port-observed, cmdline-verified gateway processes; PID-file state alone can never target another worktree's same-named gateway. Remediation remains PID-scoped and never recommends a host-wide kill.
  • Confirm release with both authoritative listener inspection and a bounded loopback bind probe, so hidden/root-owned or unrecorded listeners cannot be reported as released.
  • Extract the start/reuse cutover into an injectable lifecycle boundary: complete listener enumeration is required for reuse, duplicate cleanup is port-scoped, and fresh launch requires an independent post-reap bind proof.
  • Preserve tunnel stop's documented tunnel-only contract while making the deprecated stop command explicitly opt into gateway release; update command help and generated agent-variant docs.
  • Split release/listener/cutover responsibilities into focused modules and focused test files, remove the fragile Module._load child harness, and add resolver, fail-closed, shared-ownership, command-boundary, cutover, real-process rebind, and tunnel-lifecycle coverage.
  • Run the cutover and real-process regressions unconditionally on the macOS 26 CI runner, even when Docker is unavailable, and cap fallback bind probes at 20 attempts.
  • Keep creation serialized at the public boundary: onboard() holds the existing atomic cross-process filesystem lock (openSync(..., "wx")) through gateway reconciliation and launch, with a child-process regression proving a second CLI process is rejected; the strict post-reap bind check and OS bind exclusivity cover recovery commands and external processes outside that lock.
  • Keep the bind proof in a short-lived child because the stop API is synchronous while Node reports in-process net.Server bind results asynchronously; poll authoritative listener state at most 20 times, then invoke the independent bind subprocess exactly once.
  • Exercise the integration project on Linux in every ordinary CLI coverage shard as well as running the two gateway lifecycle regressions explicitly on macOS.
  • Preserve the dynamic gateway-port semantics introduced by the MCP stack: listener discovery resolves the current port on every scan, with a mutation regression proving it does not freeze the pre-rebuild value.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: destructive actions are registry-scoped, port-scoped, cmdline-verified, fail closed on ambiguous state, and covered through the public start/stop boundaries plus a real-process rebind test.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior — 167 tests across resolver/release, single-spawn bind confirmation, shell-free listener discovery, dynamic port-listener resolution, cross-process onboard locking, host reaper, runtime identity, shared ownership, legacy-vs-tunnel command routing, cutover decisions, real-process rebind, and tunnel E2E-support helpers.
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Additional exact-head local validation: npm run build:cli, npm run typecheck, npm run lint, agent-variant docs sync, test-size/title/project/import checks, source-shape check, and scripts/generate-platform-docs.py --check.


Signed-off-by: Yimo Jiang yimoj@nvidia.com

`nemoclaw stop` (the deprecated alias for `tunnel stop`) only stopped the
in-sandbox channels and the host-side cloudflared tunnel. On macOS the
OpenShell gateway runs as a host `openshell-gateway` process bound to the
gateway port (default 8080), and nothing in the stop path stopped it — so
the port stayed occupied after `nemoclaw stop` and a fresh onboard /
port-conflict recovery could not re-bind it.

`stopAll` now releases the NemoClaw-managed gateway port via a new
`gateway-port-release` helper. It reuses the shared host-gateway stopper
(`stopHostGatewayProcesses`) rather than an ad-hoc pkill: it stops the
recorded gateway process (pid file) plus any duplicate/orphan gateway
squatting the same port (discovered with `lsof`, covering the reporter's
`host-process=2` case), then polls the port for release. The sweep is
scoped to the resolved gateway port and gated on the openshell-gateway
cmdline, so a different worktree's gateway or an unrelated process is never
torn down. The remediation warning fires only when a matched gateway
process resists stopping, so a Docker-published port held by docker-proxy
is not mislabeled.

Release is best-effort: a stop never fails because gateway teardown hit an
edge case.

Fixes #5968

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds managed gateway port release on stop, Docker-driver prelaunch reaping and cutover orchestration, plus tests, CI, and docs updates.

Changes

Gateway Lifecycle

Layer / File(s) Summary
Release resolution and orchestration
src/lib/tunnel/gateway-port-release.ts, src/lib/tunnel/gateway-port-resolution.ts, src/lib/tunnel/gateway-port-confirmation.ts, src/lib/tunnel/gateway-port-listeners.ts
Defines gateway port resolution, listener scanning, confirmation polling, and managed port release with scoped stop and result reporting.
Release tests and runtime coverage
src/lib/tunnel/gateway-port-release-test-helpers.ts, src/lib/tunnel/gateway-port-release*.test.ts, test/tunnel-gateway-port-release-runtime.test.ts, src/lib/tunnel/service-command.test.ts, src/lib/tunnel/services-gateway-ownership.test.ts, src/commands/simple-global-oclif-adapters.test.ts, test/cli/tunnel-command.test.ts
Adds deterministic helpers plus unit, fail-closed, lifecycle, wiring, and runtime tests for gateway-port release behavior.
Stop command wiring
src/lib/tunnel/service-command.ts, src/lib/tunnel/services.ts, src/commands/stop.ts, docs/reference/commands*.mdx
Extends stop command options, passes gateway-port release through stop dispatch, wires stop-time release into service shutdown, and updates legacy stop metadata and docs.

Docker-driver Cutover

Layer / File(s) Summary
Prelaunch reaping and port-listener helpers
src/lib/onboard/docker-driver-gateway-port-listener.ts, src/lib/onboard/docker-driver-gateway-prelaunch.ts, src/lib/onboard/docker-driver-gateway-runtime.ts, src/lib/onboard/docker-driver-gateway-runtime.test.ts, src/lib/onboard/host-gateway-process.ts
Adds Docker-driver listener scanning, prelaunch reaping helpers, runtime delegation, supporting tests, and host-gateway process comments.
Docker-driver cutover orchestration
src/lib/onboard/docker-driver-gateway-cutover.ts, src/lib/onboard.ts, test/onboard-gateway-prelaunch-cutover.test.ts
Implements the cutover flow and threads the new scan-based startup path through onboarding orchestration and tests.
Docs and CI updates
.github/workflows/macos-e2e.yaml, ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, test/cli/tunnel-command.test.ts
Updates macOS integration test coverage and internal line references in docs and matrix files.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runStopCommand
  participant stopAll
  participant releaseManagedGatewayPort
  participant resolveStopGatewayPort
  participant listeningGatewayPids
  participant stopHostGatewayProcesses
  participant confirmGatewayPortReleased

  runStopCommand->>stopAll: stopAll({ sandboxName, releaseGatewayPort })
  stopAll->>releaseManagedGatewayPort: releaseGatewayPortForStop(sandboxName)
  releaseManagedGatewayPort->>resolveStopGatewayPort: resolve port
  resolveStopGatewayPort-->>releaseManagedGatewayPort: port or null
  releaseManagedGatewayPort->>listeningGatewayPids: scan listeners
  releaseManagedGatewayPort->>stopHostGatewayProcesses: stop scoped PIDs
  stopHostGatewayProcesses-->>releaseManagedGatewayPort: stopped / failed
  releaseManagedGatewayPort->>confirmGatewayPortReleased: confirm released
  confirmGatewayPortReleased-->>releaseManagedGatewayPort: released / remaining
Loading
sequenceDiagram
  participant startDockerDriverGateway
  participant createDockerDriverGatewayRuntimeHelpers
  participant runDockerDriverGatewayCutover
  participant reapHostGatewayBeforeLaunchOrFail
  participant reapDuplicateHostGatewaysExceptOrFail

  startDockerDriverGateway->>createDockerDriverGatewayRuntimeHelpers: build helpers
  startDockerDriverGateway->>runDockerDriverGatewayCutover: run cutover
  runDockerDriverGatewayCutover->>reapHostGatewayBeforeLaunchOrFail: reap before launch
  runDockerDriverGatewayCutover->>reapDuplicateHostGatewaysExceptOrFail: reap duplicates on reuse
  runDockerDriverGatewayCutover-->>startDockerDriverGateway: reused or launch
Loading

Possibly related issues

Possibly related PRs

  • NVIDIA/NemoClaw#3441: Both PRs modify the Docker-driver gateway startup/orchestration path in src/lib/onboard.ts.
  • NVIDIA/NemoClaw#3657: Overlaps with src/lib/tunnel/services.ts stop-time wiring and gateway-process handling.

Suggested labels: bug-fix, refactor

Suggested reviewers: ericksoa, jyaunches

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address the macOS stop-time port leak and add release/rebind regression coverage, matching #5968's core objective.
Out of Scope Changes check ✅ Passed No clearly unrelated code changes stand out; the docs, tests, workflow, and onboarding updates all support the gateway-port release fix.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: releasing the NemoClaw gateway port when stopping tunnel services.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/5968-stop-releases-gateway-port

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/5968-stop-releas... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5968-stop-releas... 7281e94 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/5968-stop-releas... branch is 69%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5968-stop-releas... 7281e94 +/-
src/lib/shields...nsition-lock.ts 87%
src/lib/actions...all/run-plan.ts 81%
src/lib/state/o...oard-session.ts 78%
src/lib/onboard/preflight.ts 77%
src/lib/state/sandbox.ts 74%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/shields/index.ts 68%
src/lib/actions...licy-channel.ts 60%
src/lib/policy/index.ts 60%
src/lib/onboard.ts 22%

Updated July 03, 2026 22:24 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@yimoj

yimoj commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

E2E verification (real worktree CLI)

The issue is a host openshell-gateway process left bound to the gateway port after nemoclaw stop (observed on macOS). Reproduced hermetically on Linux by standing up a process whose argv0 basename is openshell-gateway (what hostGatewayCmdlineMatches keys on) bound to a unique port 15968, then running the real built CLI. Isolated HOME/state dir and a per-issue port (no shared sandbox touched).

===== BEFORE STOP =====
fake gateway pid=3065956  argv0=/tmp/.../openshell-gateway /tmp/.../listen.js
lsof :15968 -> 3065956

===== RUN: node ./bin/nemoclaw.js stop =====
[services] cloudflared was not running
Stopped host openshell-gateway process 3065956
Released NemoClaw gateway port 15968 (stopped host process 3065956).
[services] All services stopped.
(exit=0)

===== AFTER STOP =====
lsof :15968 -> ''
gateway pid 3065956 reaped
pid file exists: no

===== SQUATTER REBIND CHECK =====
REBIND OK: port 15968 is free

This exercises the full new path: lsof discovery of the listener, openshell-gateway cmdline gate, stopHostGatewayProcesses TERM→KILL, pid-file cleanup, and post-stop port-release confirmation. The cross-platform unit suite (gateway-port-release.test.ts) covers the registry-port resolution, per-port state dir, orphan/duplicate stop, quiet no-op, sudo-remediation warn, and lsof-absent fallback. The macos-e2e CI job validates the original macOS host-process scenario directly.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: No advisor follow-up required beyond maintainer review.
Open items: 0 required · 0 warnings · 0 suggestions · 0 test follow-ups
Since last review: 0 prior items resolved · 0 still apply · 0 new items found

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: onboard-resume, onboard-repair, cloud-onboard, full-e2e, tunnel-lifecycle, concurrent-gateway-ports, gateway-drift-preflight, gateway-guard-recovery, macos-e2e
Optional E2E: sandbox-operations, hermes-e2e, wsl-e2e

Dispatch hint: onboard-resume,onboard-repair,cloud-onboard,full-e2e,tunnel-lifecycle,concurrent-gateway-ports,gateway-drift-preflight,gateway-guard-recovery

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • onboard-resume (high): Required by the onboarding resume rule because gateway prelaunch/cutover and src/lib/onboard.ts orchestration changes can affect partial onboarding resume state and live slice recovery.
  • onboard-repair (high): Required by the onboarding resume rule because duplicate/stale gateway repair and managed gateway recovery paths must be validated live, not only with unit or integration tests.
  • cloud-onboard (high): The changed onboarding gateway launch/cutover path can affect full hosted onboarding with real provider credentials and sandbox creation.
  • full-e2e (high): Broad live user-flow coverage is required because the PR changes onboarding, gateway readiness, sandbox lifecycle, and stop/start semantics used by the default OpenClaw path.
  • tunnel-lifecycle (high): Directly covers tunnel start/stop lifecycle behavior affected by the changed distinction between tunnel stop and legacy full stop releasing the managed gateway port.
  • concurrent-gateway-ports (medium): The PR changes gateway port listener identification, ownership checks, duplicate reaping, and port cutover. Concurrent gateway port coverage is required for those collision/ownership paths.
  • gateway-drift-preflight (medium): The runtime drift and stale gateway handling in src/lib/onboard.ts and Docker-driver gateway helpers changed; this E2E verifies fail-closed handling around stale gateway state.
  • gateway-guard-recovery (high): Gateway health/recovery guidance can be affected by duplicate gateway reaping, port ownership changes, and gateway cutover behavior.
  • macos-e2e (high): The PR modifies .github/workflows/macos-e2e.yaml and host gateway lifecycle code that is platform-sensitive. Run the macOS E2E workflow to validate the new gateway lifecycle regressions and the full macOS path when Docker is available.

Optional E2E

  • sandbox-operations (high): Useful adjacent confidence for sandbox command/lifecycle behavior after changing managed gateway stop/release and service ownership code.
  • hermes-e2e (high): Optional because the user-facing docs and stop semantics mention nemohermes/Hermes behavior; run if maintainers want confidence that shared gateway lifecycle changes do not regress Hermes onboarding.
  • wsl-e2e (high): Optional platform confidence for host gateway and port listener behavior on another non-Linux-native host path.

New E2E recommendations

  • legacy stop gateway port release (high): Existing live tunnel lifecycle coverage may not explicitly assert the new compatibility split: nemoclaw tunnel stop preserves the managed host gateway, while deprecated nemoclaw stop releases it. Add a focused live E2E if tunnel-lifecycle does not already assert both commands against the same running sandbox.
    • Suggested test: Add a live E2E scenario that onboards a sandbox, records the managed gateway port/listener, runs nemoclaw tunnel stop and asserts the gateway remains available, then runs deprecated nemoclaw stop and asserts the managed gateway port is released.
  • Docker-driver gateway prelaunch cutover (medium): The PR adds integration coverage, but a live E2E that exercises duplicate host gateway reaping and cutover with real OpenShell binaries would better protect the production boundary.
    • Suggested test: Add a live E2E scenario that seeds a stale or duplicate host OpenShell gateway listener, runs onboarding/resume, and asserts NemoClaw reaps only the unsafe duplicate and cuts over to the correct managed gateway without breaking the sandbox.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: onboard-resume,onboard-repair,cloud-onboard,full-e2e,tunnel-lifecycle,concurrent-gateway-ports,gateway-drift-preflight,gateway-guard-recovery

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: tunnel-lifecycle-vitest
Optional Vitest E2E scenarios: concurrent-gateway-ports-vitest

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=tunnel-lifecycle-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: medium

Required Vitest E2E scenarios

  • tunnel-lifecycle-vitest: Changes the tunnel service stop path and adds gateway-port release during stop; the tunnel lifecycle live Vitest job exercises real NemoClaw tunnel start/status/stop cleanup boundaries through the scenario workflow.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=tunnel-lifecycle-vitest

Optional Vitest E2E scenarios

  • concurrent-gateway-ports-vitest: Adjacent coverage for the per-port gateway binding/isolation risk in the new gateway-port release helper: it onboards multiple sandboxes with distinct gateway ports and verifies one gateway remains healthy after operations on another sandbox.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=concurrent-gateway-ports-vitest

Relevant changed files

  • src/lib/tunnel/gateway-port-release.ts
  • src/lib/tunnel/services.ts

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-6: defaultProbePortFree spawns Node child process per stop invocation; then add or justify PRA-T1.
Open items: 2 required · 22 warnings · 3 suggestions · 8 test follow-ups
Since last review: 2 prior items resolved · 9 still apply · 5 new items found

Action checklist

  • PRA-6 Fix: defaultProbePortFree spawns Node child process per stop invocation in src/lib/tunnel/gateway-port-confirmation.ts:42
  • PRA-7 Fix: Creation-path singleton enforcement (Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2) lacks integration test for SAME-port concurrent onboard in src/lib/onboard.ts:2144
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/tunnel/gateway-port-confirmation.ts:42 defaultProbePortFree spawnSync
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/tunnel/gateway-port-confirmation.ts:12 PORT_FREE_PROBE_SCRIPT minified inline
  • PRA-3 Resolve or justify: Source-of-truth review needed: src/lib/onboard/host-gateway-process.ts:84 defaultCommandExists sh -c command -v
  • PRA-4 Resolve or justify: Source-of-truth review needed: src/lib/onboard/docker-driver-gateway-cutover.ts:35 DockerDriverGatewayCutoverDeps interface (16 methods)
  • PRA-5 Resolve or justify: Source-of-truth review needed: src/lib/onboard/docker-driver-gateway-prelaunch.ts:1 prelaunch reaping workaround
  • PRA-8 Resolve or justify: Monolith test file at 1377 lines — exceeds growth threshold in src/lib/state/onboard-session.test.ts:1377
  • PRA-9 Resolve or justify: defaultCommandExists uses sh -c 'command -v' despite trusted literal in src/lib/onboard/host-gateway-process.ts:84
  • PRA-10 Resolve or justify: Peer registry corruption error lacks actionable nemoclaw destroy guidance in src/lib/tunnel/gateway-stop.ts:103
  • PRA-11 Resolve or justify: NODE_DEBUG console.error lacks nemoclaw destroy remediation hint in src/lib/tunnel/gateway-stop.ts:112
  • PRA-12 Resolve or justify: Port listener helpers re-exported through runtime facade instead of direct import in src/lib/onboard/docker-driver-gateway-runtime.ts:25
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Linux E2E workflow missing gateway lifecycle regression tests
  • PRA-T7 Add or justify test follow-up: Runtime validation test skipped on Windows and when lsof absent — no alternative coverage
  • PRA-T8 Add or justify test follow-up: confirmGatewayPortReleased maxAttempts bound (20) not explicitly tested as a regression guard
  • PRA-15 In-scope improvement: ReleaseGatewayPortResult.scanned field lacks clarifying JSDoc comment in src/lib/tunnel/gateway-port-release.ts:50
  • PRA-16 In-scope improvement: PORT_FREE_PROBE_SCRIPT minified inline string reduces readability in src/lib/tunnel/gateway-port-confirmation.ts:12
  • PRA-21 In-scope improvement: Deprecated stop command description could be clearer about what it does vs tunnel stop in src/commands/stop.ts:14

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-5 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-6 Required security src/lib/tunnel/gateway-port-confirmation.ts:42 Replace spawnSync-based probe with synchronous net.createServer().listen() attempt in the main process. Since confirmGatewayPortReleased is synchronous, use a one-shot server that binds, immediately closes, and returns success/failure. Follow the canBind() pattern from test/tunnel-gateway-port-release-runtime.test.ts:112.
PRA-7 Required acceptance src/lib/onboard.ts:2144 Add integration test proving no two host gateways can bind the same port under concurrent onboard invocations. Spawn two CLI processes attempting to onboard with same NEMOCLAW_GATEWAY_PORT and verify only one acquires lock and binds port. If not feasible in this PR, create follow-up issue with acceptance criteria and update Issue #5968 to 'Refs #5968'.
PRA-8 Resolve/justify architecture src/lib/state/onboard-session.test.ts:1377 Extract new test helpers into separate files under test/helpers/ or src/lib/state/onboard-session-*.test.ts. Follow the pattern established by onboard-session-cross-process-lock.test.ts. At minimum, justify why new tests cannot be modularized.
PRA-9 Resolve/justify security src/lib/onboard/host-gateway-process.ts:84 Replace with pure-JS PATH search matching the pattern in gateway-port-listeners.ts:35 (defaultGatewayReleaseCommandExists). This also resolves the source-of-truth concern from previous review.
PRA-10 Resolve/justify correctness src/lib/tunnel/gateway-stop.ts:103 Update warn message to include: 'Consider nemoclaw <name> destroy to clear stale entries.' Also consider adding registry validation on write to prevent corrupt entries.
PRA-11 Resolve/justify correctness src/lib/tunnel/gateway-stop.ts:112 Add the same destroy remediation hint to the NODE_DEBUG output, or ensure the warn message (with hint) is always shown alongside the debug output.
PRA-12 Resolve/justify architecture src/lib/onboard/docker-driver-gateway-runtime.ts:25 Update onboard.ts to import port listener helpers directly from docker-driver-gateway-port-listener.ts. Remove the re-exports from docker-driver-gateway-runtime.ts.
PRA-13 Resolve/justify tests .github/workflows/macos-e2e.yaml:45 Add a step to the Linux E2E workflow (or a unit test job) that runs the gateway lifecycle regression tests: npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts. The integration project in vitest.config.ts already exists.
PRA-14 Resolve/justify architecture src/lib/onboard/docker-driver-gateway-cutover.ts:35 Evaluate consolidating cutover dependencies. Consider having cutover use runtime module helpers directly, or create a shared facade. At minimum, document why each dep is needed and cannot be sourced from runtime module.
PRA-15 Improvement correctness src/lib/tunnel/gateway-port-release.ts:50 Add JSDoc comment to the 'scanned' field: 'true when lsof scan was attempted and completed (exit code 0 or 1), false when lsof unavailable or scan failed.'
PRA-16 Improvement architecture src/lib/tunnel/gateway-port-confirmation.ts:12 Convert to multi-line template literal with comments explaining the net.createServer bind logic. Or extract to src/lib/tunnel/probe-port-free.cjs and load via fs.readFileSync. (Also resolved by PRA-5 fix which removes the script entirely.)
PRA-17 Resolve/justify tests test/tunnel-gateway-port-release-runtime.test.ts:1 Add a conditional unit test that mocks lsof/spawnSync to cover the same logic on Windows, OR document why Windows is not a supported gateway host platform (NemoClaw targets Linux/macOS).
PRA-18 Resolve/justify security src/lib/onboard/docker-driver-gateway-port-listener.ts:75 Add stricter validation: limit max PIDs parsed (e.g., 100), validate each PID is within reasonable range (1-4194304), handle unexpected output formats gracefully.
PRA-19 Resolve/justify correctness src/lib/tunnel/gateway-port-release.ts:115 Clarify control flow: when lsof fails (status >1), skip the stop call entirely and return early with released=false, scanned=false. Makes fail-closed behavior more explicit.
PRA-20 Resolve/justify acceptance src/lib/onboard.ts:1986 Add test (unit or integration) verifying that when a Docker container gateway is running on a port, the host-process gateway cannot also bind that port (and vice versa). May require Docker in CI.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-6 Required — defaultProbePortFree spawns Node child process per stop invocation

  • Location: src/lib/tunnel/gateway-port-confirmation.ts:42
  • Category: security
  • Problem: defaultProbePortFree uses spawnSync(process.execPath, ['-e', PORT_FREE_PROBE_SCRIPT, port]) to prove a port is free. This spawns a short-lived Node child process on every 'nemoclaw stop' call. While the current code only calls it once per releaseManagedGatewayPort invocation (not 20 times as previously thought), it still creates unnecessary subprocess overhead and a minor DoS vector under automated CI or repeated stop calls.
  • Impact: Subprocess spawn overhead accumulates in CI; could be abused as minor DoS vector. Eliminates PORT_FREE_PROBE_SCRIPT inline script which reduces auditability.
  • Required action: Replace spawnSync-based probe with synchronous net.createServer().listen() attempt in the main process. Since confirmGatewayPortReleased is synchronous, use a one-shot server that binds, immediately closes, and returns success/failure. Follow the canBind() pattern from test/tunnel-gateway-port-release-runtime.test.ts:112.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read src/lib/tunnel/gateway-port-confirmation.ts lines 42-55. Confirm defaultProbePortFree no longer uses spawnSync with PORT_FREE_PROBE_SCRIPT. The function should use net.createServer().listen(port, '127.0.0.1') with a callback that closes and returns true/false.
  • Missing regression test: Add test mocking spawnSync to verify it is NOT called when releaseManagedGatewayPort invokes probePortFree. Existing test 'runs one bind proof for one managed gateway release' in gateway-port-release-fail-closed.test.ts covers call count.
  • Done when: The required change is committed and verification passes: Read src/lib/tunnel/gateway-port-confirmation.ts lines 42-55. Confirm defaultProbePortFree no longer uses spawnSync with PORT_FREE_PROBE_SCRIPT. The function should use net.createServer().listen(port, '127.0.0.1') with a callback that closes and returns true/false.
  • Evidence: defaultProbePortFree at line 42 uses spawnSync; PORT_FREE_PROBE_SCRIPT at line 12 is minified inline string; confirmGatewayPortReleased at line 82 calls probePortFree once after listener polling.

PRA-7 Required — Creation-path singleton enforcement (Issue #5968 clause 2) lacks integration test for SAME-port concurrent onboard

  • Location: src/lib/onboard.ts:2144
  • Category: acceptance
  • Problem: Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2 requires 'gateway must be shared (exactly one instance across container + host-process modes)'. The PR implements singleton enforcement via acquireOnboardLock() filesystem lock (openSync 'wx') and post-reap bind check, but no integration test proves two concurrent 'nemoclaw onboard' invocations for the SAME port cannot both succeed. The existing E2E test concurrent-gateway-ports.test.ts tests DIFFERENT ports (8080 and 18080).
  • Impact: Without this test, the singleton guarantee for the critical concurrent creation path is unproven. Race conditions could allow two gateways to bind the same port.
  • Required action: Add integration test proving no two host gateways can bind the same port under concurrent onboard invocations. Spawn two CLI processes attempting to onboard with same NEMOCLAW_GATEWAY_PORT and verify only one acquires lock and binds port. If not feasible in this PR, create follow-up issue with acceptance criteria and update Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 to 'Refs [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968'.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search test/ for concurrent onboard test with SAME gateway port. Confirm it spawns two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT and verifies only one acquires lock and binds port.
  • Missing regression test: Integration test: concurrent onboard for same port enforces singleton — spawn two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT, verify only one acquires lock and binds port.
  • Done when: The required change is committed and verification passes: Search test/ for concurrent onboard test with SAME gateway port. Confirm it spawns two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT and verifies only one acquires lock and binds port.
  • Evidence: acquireOnboardLock() at onboard-session.ts:671 uses openSync('wx') atomic create. startDockerDriverGateway in onboard.ts:2068 holds lock across gateway creation. Post-reap bind check in cutover adds second boundary. concurrent-gateway-ports.test.ts tests DIFFERENT ports only.
Review findings by urgency: 2 required fixes, 22 items to resolve/justify, 3 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/tunnel/gateway-port-confirmation.ts:42 defaultProbePortFree spawnSync

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: gateway-port-release-fail-closed.test.ts: 'runs one bind proof for one managed gateway release'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: defaultProbePortFree at line 42 uses spawnSync; PORT_FREE_PROBE_SCRIPT at line 12 is the child code; confirmGatewayPortReleased at line 82 calls probePortFree once after listener polling

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/tunnel/gateway-port-confirmation.ts:12 PORT_FREE_PROBE_SCRIPT minified inline

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: N/A
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PORT_FREE_PROBE_SCRIPT at lines 12-30 is a minified single-line template literal implementing net.createServer bind logic

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/onboard/host-gateway-process.ts:84 defaultCommandExists sh -c command -v

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Unit test for commandExists replacement verifying correct behavior for 'pgrep' and non-existent commands
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: defaultCommandExists at line 84 uses sh -c with JSON.stringify. defaultGatewayReleaseCommandExists at gateway-port-listeners.ts:35 uses process.env.PATH.split + fs.accessSync

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/onboard/docker-driver-gateway-cutover.ts:35 DockerDriverGatewayCutoverDeps interface (16 methods)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/onboard-gateway-prelaunch-cutover.test.ts mocks all deps
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: DockerDriverGatewayCutoverDeps defines 16 methods including isDockerDriverGatewayProcessAlive, getDockerDriverGatewayRuntimeDrift, reapHostGatewayBeforeLaunchOrFail, reapDuplicateHostGatewaysExceptOrFail, etc. — many exist in runtime module.

PRA-5 Resolve/justify — Source-of-truth review needed: src/lib/onboard/docker-driver-gateway-prelaunch.ts:1 prelaunch reaping workaround

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/onboard-gateway-prelaunch-cutover.test.ts covers reaping behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Header comment lines 1-30 explains historical problem and workaround. No explicit removal condition stated.

PRA-8 Resolve/justify — Monolith test file at 1377 lines — exceeds growth threshold

  • Location: src/lib/state/onboard-session.test.ts:1377
  • Category: architecture
  • Problem: The onboard-session.test.ts file is a monolith at 1377 lines. Previous review flagged 34 lines of growth exceeding the 20-line threshold. While the cross-process lock test was correctly placed in a separate file (onboard-session-cross-process-lock.test.ts), the main test file remains a monolith that should be modularized.
  • Impact: Maintainability risk; new test helpers should be extracted to separate files rather than adding to this monolith.
  • Recommended action: Extract new test helpers into separate files under test/helpers/ or src/lib/state/onboard-session-*.test.ts. Follow the pattern established by onboard-session-cross-process-lock.test.ts. At minimum, justify why new tests cannot be modularized.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run wc -l src/lib/state/onboard-session.test.ts. Verify new test helpers added in this PR are in separate files.
  • Missing regression test: N/A — architecture/maintainability concern, not functional regression.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run wc -l src/lib/state/onboard-session.test.ts. Verify new test helpers added in this PR are in separate files.
  • Evidence: File is 1377 lines. Previous review noted growth from 1376 to 1410 (34 lines). Cross-process lock test correctly isolated in separate file.

PRA-9 Resolve/justify — defaultCommandExists uses sh -c 'command -v' despite trusted literal

  • Location: src/lib/onboard/host-gateway-process.ts:84
  • Category: security
  • Problem: defaultCommandExists uses defaultRun('sh', ['-c', `command -v ${JSON.stringify(command)} >/dev/null 2>&1`]) to check if a command exists. The command is always an internal trusted literal ('pgrep'), and JSON.stringify provides quoting, but this still spawns a shell process unnecessarily. A pure-JS PATH search using process.env.PATH.split(path.delimiter) and fs.accessSync(path.join(dir, command), fs.constants.X_OK) would be simpler, faster, and avoid shell invocation.
  • Impact: Unnecessary shell process spawn; attack surface increase (though minimal with trusted literal). Pure-JS alternative already exists in gateway-port-listeners.ts:35 as defaultGatewayReleaseCommandExists.
  • Recommended action: Replace with pure-JS PATH search matching the pattern in gateway-port-listeners.ts:35 (defaultGatewayReleaseCommandExists). This also resolves the source-of-truth concern from previous review.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/host-gateway-process.ts lines 80-90. Confirm defaultCommandExists no longer uses sh -c and instead uses PATH split + fs.accessSync.
  • Missing regression test: Unit test for commandExists replacement verifying it correctly finds 'pgrep' on PATH and returns false for non-existent commands.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/host-gateway-process.ts lines 80-90. Confirm defaultCommandExists no longer uses sh -c and instead uses PATH split + fs.accessSync.
  • Evidence: defaultCommandExists at line 84 uses sh -c with JSON.stringify. defaultGatewayReleaseCommandExists at gateway-port-listeners.ts:35 uses pure-JS PATH search.

PRA-10 Resolve/justify — Peer registry corruption error lacks actionable nemoclaw destroy guidance

  • Location: src/lib/tunnel/gateway-stop.ts:103
  • Category: correctness
  • Problem: When a corrupt peer registry entry makes gateway ownership ambiguous, the warning message says 'repair the sandbox registry and retry' but doesn't mention 'nemoclaw <name> destroy' as a remediation. Users may not know how to repair the registry.
  • Impact: Operators facing registry corruption get unactionable error message, prolonging outage.
  • Recommended action: Update warn message to include: 'Consider nemoclaw <name> destroy to clear stale entries.' Also consider adding registry validation on write to prevent corrupt entries.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-stop.ts lines 100-110. Confirm warn message includes 'Consider nemoclaw <name> destroy to clear stale entries.'
  • Missing regression test: Test verifying the warning message contains the destroy remediation hint when gateway-stop catches a registry error.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-stop.ts lines 100-110. Confirm warn message includes 'Consider nemoclaw <name> destroy to clear stale entries.'.
  • Evidence: gateway-stop.ts:103 warns 'repair the sandbox registry and retry' without destroy hint. NODE_DEBUG output at line 112 also lacks it.

PRA-11 Resolve/justify — NODE_DEBUG console.error lacks nemoclaw destroy remediation hint

  • Location: src/lib/tunnel/gateway-stop.ts:112
  • Category: correctness
  • Problem: The NODE_DEBUG=nemoclaw:gateway diagnostic output shows the stack trace but doesn't include the 'Consider nemoclaw <name> destroy' remediation hint that the warn message should have.
  • Impact: Debug output for operators explicitly debugging gateway teardown lacks actionable remediation.
  • Recommended action: Add the same destroy remediation hint to the NODE_DEBUG output, or ensure the warn message (with hint) is always shown alongside the debug output.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-stop.ts lines 110-115. Confirm the debug output or accompanying warn includes the destroy remediation.
  • Missing regression test: Test with NODE_DEBUG=nemoclaw:gateway verifying destroy hint appears in output when registry error occurs.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-stop.ts lines 110-115. Confirm the debug output or accompanying warn includes the destroy remediation.
  • Evidence: gateway-stop.ts:112 logs error stack via console.error when NODE_DEBUG includes nemoclaw:gateway, but no remediation hint.

PRA-12 Resolve/justify — Port listener helpers re-exported through runtime facade instead of direct import

  • Location: src/lib/onboard/docker-driver-gateway-runtime.ts:25
  • Category: architecture
  • Problem: docker-driver-gateway-runtime.ts re-exports getDockerDriverGatewayPortListenerPid, getDockerDriverGatewayPortListenerScan, isDockerDriverGatewayPortListener from docker-driver-gateway-port-listener.ts. This creates unnecessary indirection. onboard.ts should import port listener helpers directly from docker-driver-gateway-port-listener.ts.
  • Impact: Maintenance burden; unclear dependency graph; harder to trace code flow.
  • Recommended action: Update onboard.ts to import port listener helpers directly from docker-driver-gateway-port-listener.ts. Remove the re-exports from docker-driver-gateway-runtime.ts.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check imports in src/lib/onboard.ts for port listener helpers. Verify they come from docker-driver-gateway-port-listener.ts, not docker-driver-gateway-runtime.ts.
  • Missing regression test: N/A — architecture/refactoring concern.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check imports in src/lib/onboard.ts for port listener helpers. Verify they come from docker-driver-gateway-port-listener.ts, not docker-driver-gateway-runtime.ts.
  • Evidence: docker-driver-gateway-runtime.ts lines 25-30 re-export three functions from docker-driver-gateway-port-listener.ts. onboard.ts imports from runtime facade.

PRA-13 Resolve/justify — Linux E2E workflow missing gateway lifecycle regression tests

  • Location: .github/workflows/macos-e2e.yaml:45
  • Category: tests
  • Problem: The macOS E2E workflow now runs gateway lifecycle regression tests (tunnel-gateway-port-release-runtime.test.ts and onboard-gateway-prelaunch-cutover.test.ts) via the new 'Run gateway lifecycle regressions' step. However, the Linux E2E workflow (.github/workflows/e2e.yaml) does not have an equivalent step. These tests validate critical gateway port release logic and should run on Linux CI as well.
  • Impact: Gateway lifecycle regressions may go undetected on Linux, the primary platform.
  • Recommended action: Add a step to the Linux E2E workflow (or a unit test job) that runs the gateway lifecycle regression tests: npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts. The integration project in vitest.config.ts already exists.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read .github/workflows/e2e.yaml. Confirm it has a step running npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts
  • Missing regression test: CI job running gateway lifecycle regression tests on Linux.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read .github/workflows/e2e.yaml. Confirm it has a step running npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts.
  • Evidence: macos-e2e.yaml lines 45-48 added the step. e2e.yaml has no equivalent.

PRA-14 Resolve/justify — DockerDriverGatewayCutoverDeps interface duplicates runtime module logic (16 methods)

  • Location: src/lib/onboard/docker-driver-gateway-cutover.ts:35
  • Category: architecture
  • Problem: The DockerDriverGatewayCutoverDeps interface defines 16 methods, many duplicating helpers from docker-driver-gateway-runtime.ts and host-gateway-process.ts. This creates a large dependency surface that's hard to maintain. The cutover logic should use runtime module helpers directly, or a shared facade should be created.
  • Impact: Maintenance burden; duplication risk; unclear ownership of logic.
  • Recommended action: Evaluate consolidating cutover dependencies. Consider having cutover use runtime module helpers directly, or create a shared facade. At minimum, document why each dep is needed and cannot be sourced from runtime module.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/docker-driver-gateway-cutover.ts lines 35-80. Compare each dep method with docker-driver-gateway-runtime.ts and host-gateway-process.ts exports. Identify which can be sourced from existing modules.
  • Missing regression test: N/A — architecture/refactoring concern.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/docker-driver-gateway-cutover.ts lines 35-80. Compare each dep method with docker-driver-gateway-runtime.ts and host-gateway-process.ts exports. Identify which can be sourced from existing modules.
  • Evidence: DockerDriverGatewayCutoverDeps has 16 methods including isDockerDriverGatewayProcessAlive, isGatewayHealthy, getDockerDriverGatewayRuntimeDrift, logDockerDriverGatewayRestart, registerDockerDriverGatewayEndpoint, isDockerDriverGatewayHttpReady, verifySandboxBridgeGatewayReachableOrExit, readGatewayHealth, rememberDockerDriverGatewayPid, reapDuplicateHostGatewaysExceptOrFail, reapHostGatewayBeforeLaunchOrFail, isGatewayPortAvailable, reportUntrustedGatewayPort, reportMissingGatewayBinary, log.

PRA-17 Resolve/justify — Runtime validation test skipped on Windows and when lsof absent — no alternative coverage

  • Location: test/tunnel-gateway-port-release-runtime.test.ts:1
  • Category: tests
  • Problem: The runtime validation test uses it.skipIf(!posix || !hasLsof) which skips on Windows and when lsof is absent. There's no conditional unit test that mocks lsof/spawnSync to cover the same logic on Windows. Since NemoClaw targets Linux/macOS as host platforms, this may be acceptable but should be documented or covered.
  • Impact: No test coverage for gateway port release logic on Windows or in environments without lsof.
  • Recommended action: Add a conditional unit test that mocks lsof/spawnSync to cover the same logic on Windows, OR document why Windows is not a supported gateway host platform (NemoClaw targets Linux/macOS).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if there's a unit test mocking lsof/spawnSync for gateway port release on Windows. Read test/tunnel-gateway-port-release-runtime.test.ts header comments.
  • Missing regression test: Mock-based unit test covering gateway port release logic on Windows (without requiring lsof).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if there's a unit test mocking lsof/spawnSync for gateway port release on Windows. Read test/tunnel-gateway-port-release-runtime.test.ts header comments.
  • Evidence: test/tunnel-gateway-port-release-runtime.test.ts uses it.skipIf(!posix || !hasLsof) at line 38. posix = process.platform !== 'win32'; hasLsof checks lsof -v.

PRA-18 Resolve/justify — lsof output parsing trusts lsof -ti format without bounds validation

  • Location: src/lib/onboard/docker-driver-gateway-port-listener.ts:75
  • Category: security
  • Problem: getDockerDriverGatewayPortListenerScan parses lsof -ti output with parseListenerPids which splits on newlines and parses integers. While it filters for positive integers, there's no max PID count limit, no PID range validation (1-4194304), and no handling of unexpected output formats. A malicious or corrupted lsof binary could output excessive data.
  • Impact: Defense-in-depth gap; potential for resource exhaustion or logic errors if lsof output is malformed.
  • Recommended action: Add stricter validation: limit max PIDs parsed (e.g., 100), validate each PID is within reasonable range (1-4194304), handle unexpected output formats gracefully.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/docker-driver-gateway-port-listener.ts lines 60-80. Verify parseListenerPids has bounds checking and input validation.
  • Missing regression test: Test with malformed lsof output (non-numeric, negative, extremely large PIDs, excessive line count) verifying graceful handling.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/docker-driver-gateway-port-listener.ts lines 60-80. Verify parseListenerPids has bounds checking and input validation.
  • Evidence: parseListenerPids at line 60 splits on \r?\n, maps to parseInt, filters Number.isInteger && pid > 0. No max count, no upper bound.

PRA-19 Resolve/justify — releaseManagedGatewayPort continues with empty PID list after lsof failure instead of early return

  • Location: src/lib/tunnel/gateway-port-release.ts:115
  • Category: correctness
  • Problem: When lsof exits with status >1 (genuine error), releaseManagedGatewayPort sets scanFailed=true and scanned=false, but then calls stopHostGatewayProcesses with empty pids list and usePidFile=false. This is correct fail-closed behavior but the control flow could be clearer with an early return.
  • Impact: Code clarity; maintainers may misread the fail-closed intent.
  • Recommended action: Clarify control flow: when lsof fails (status >1), skip the stop call entirely and return early with released=false, scanned=false. Makes fail-closed behavior more explicit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-port-release.ts lines 100-130. Verify lsof failure path returns early without calling stopHostGatewayProcesses.
  • Missing regression test: Test in gateway-port-release-fail-closed.test.ts already covers this: 'warns and refuses unsafe pid-file cleanup when lsof exits with a real failure'.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-port-release.ts lines 100-130. Verify lsof failure path returns early without calling stopHostGatewayProcesses.
  • Evidence: Lines 115-125: after lsof failure, scanFailed=true, then stopHostGatewayProcesses called with pids: [] and usePidFile: false.

PRA-20 Resolve/justify — Issue #5968 clause 1 — gateway shared across container + host-process modes not explicitly tested

  • Location: src/lib/onboard.ts:1986
  • Category: acceptance
  • Problem: Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 1 requires 'gateway must be shared (exactly one instance across container + host-process modes)'. The cutover logic handles host-process gateway reuse/replacement, but there's no test proving a container gateway and host-process gateway cannot both bind the same port simultaneously.
  • Impact: Container vs host-process gateway port conflict scenario untested; potential for both to bind same port.
  • Recommended action: Add test (unit or integration) verifying that when a Docker container gateway is running on a port, the host-process gateway cannot also bind that port (and vice versa). May require Docker in CI.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search test/ for container-vs-host gateway port conflict test. Verify it tests both directions.
  • Missing regression test: Test proving container gateway and host-process gateway cannot both bind same port.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search test/ for container-vs-host gateway port conflict test. Verify it tests both directions.
  • Evidence: Cutover logic in docker-driver-gateway-cutover.ts handles host-process reuse/replacement. No test for container gateway coexistence.

PRA-22 Resolve/justify — prelaunchReapFailureMessage recommends sudo kill -9 without process verification step

  • Location: src/lib/onboard/docker-driver-gateway-prelaunch.ts:100
  • Category: security
  • Problem: prelaunchReapFailureMessage constructs 'sudo kill -9 ${pids}' from failed PIDs. These PIDs come from stopHostGatewayProcesses which cmdline-gates on 'openshell-gateway', so they should be gateway processes. However, if a PID was recycled between the stop attempt and message construction, the kill could target an unrelated process. The message should include a verification step.
  • Impact: Potential for accidental kill of unrelated process if PID recycled (low probability but possible).
  • Recommended action: Update message to: 'Run: sudo kill -9 ${pids} (verify with: ps -p ${pids} -o pid,cmd)'. This lets the user verify the processes before killing.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/docker-driver-gateway-prelaunch.ts lines 95-105. Confirm the remediation message includes a verification command.
  • Missing regression test: Test verifying the failure message includes process verification guidance.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/docker-driver-gateway-prelaunch.ts lines 95-105. Confirm the remediation message includes a verification command.
  • Evidence: prelaunchReapFailureMessage at lines 95-105 constructs 'sudo kill -9 ${result.failed.join(' ')}' without verification step.

PRA-23 Resolve/justify — confirmGatewayPortReleased maxAttempts bound (20) not explicitly tested as a regression guard

  • Location: src/lib/tunnel/gateway-port-confirmation.test.ts:25
  • Category: tests
  • Problem: The confirmGatewayPortReleased function caps listener polling at 20 attempts (maxAttempts: 20 at line 82). The test 'caps failed listener inspections at twenty without spawning a bind probe' verifies this, but it's not explicitly documented as a regression guard for the DoS bound.
  • Impact: If maxAttempts is increased, the DoS bound weakens without a test failure.
  • Recommended action: Add explicit test comment or assertion documenting that maxAttempts=20 is a security bound limiting subprocess spawn attempts. The existing test covers the behavior but the intent should be explicit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-port-confirmation.test.ts lines 20-30. Confirm test asserts listeningPids called exactly 20 times and probePortFree not called.
  • Missing regression test: Existing test covers this but should be explicitly labeled as security bound test.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-port-confirmation.test.ts lines 20-30. Confirm test asserts listeningPids called exactly 20 times and probePortFree not called.
  • Evidence: gateway-port-confirmation.ts:82 has maxAttempts: 20. gateway-port-confirmation.test.ts:25 expects listeningPids called 20 times.

PRA-24 Resolve/justify — Source-of-truth: prelaunch reaping is a localized workaround for missing atomic gateway replacement

  • Location: src/lib/onboard/docker-driver-gateway-prelaunch.ts:1
  • Category: architecture
  • Problem: The prelaunch reaping logic (reapHostGatewayBeforeLaunch, reapDuplicateHostGatewaysExcept) handles the case where a gateway replacement doesn't cleanly terminate the old process before spawning a new one. This is a workaround for the lack of atomic gateway replacement in OpenShell. The invalid state is 'two host gateways bound to same port'. Source boundary: OpenShell doesn't expose atomic gateway replacement. Source fix would require OpenShell to support graceful handoff. Regression test: test/onboard-gateway-prelaunch-cutover.test.ts tests reaping behavior. Removal condition: when OpenShell provides atomic gateway replacement or NemoClaw moves to container-only gateway.
  • Impact: Complexity in NemoClaw to compensate for missing OpenShell capability.
  • Recommended action: Document this as a known workaround with removal condition. No code change needed in this PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docker-driver-gateway-prelaunch.ts header comment. Confirm it explains the workaround nature and removal condition.
  • Missing regression test: test/onboard-gateway-prelaunch-cutover.test.ts covers reaping behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docker-driver-gateway-prelaunch.ts header comment. Confirm it explains the workaround nature and removal condition.
  • Evidence: Header comment at lines 1-30 explains the historical problem and that this reuses stopHostGatewayProcesses reaper. No explicit removal condition stated.

PRA-25 Resolve/justify — Source-of-truth: DockerDriverGatewayCutoverDeps interface is a large localized abstraction

  • Location: src/lib/onboard/docker-driver-gateway-cutover.ts:35
  • Category: architecture
  • Problem: The 16-method DockerDriverGatewayCutoverDeps interface is a large dependency injection surface that duplicates runtime module logic. Invalid state: cutover logic needs access to drift detection, health checks, reaping functions that exist in other modules but aren't composed cleanly. Source boundary: cutover is a new orchestration layer between runtime and prelaunch modules. Source fix: compose from existing modules rather than defining large interface. Regression test: test/onboard-gateway-prelaunch-cutover.test.ts mocks all deps. Removal condition: when cutover uses runtime module helpers directly via a shared facade.
  • Impact: Maintenance burden; duplication risk; unclear ownership.
  • Recommended action: Document why each dep is needed and cannot be sourced from runtime module. Plan consolidation in follow-up.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docker-driver-gateway-cutover.ts lines 35-80. Compare each dep with runtime module exports.
  • Missing regression test: N/A — architecture concern.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docker-driver-gateway-cutover.ts lines 35-80. Compare each dep with runtime module exports.
  • Evidence: 16-method interface duplicating isDockerDriverGatewayProcessAlive, getDockerDriverGatewayRuntimeDrift, reapHostGatewayBeforeLaunchOrFail, etc. from runtime and host-gateway-process modules.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-15 Improvement — ReleaseGatewayPortResult.scanned field lacks clarifying JSDoc comment

  • Location: src/lib/tunnel/gateway-port-release.ts:50
  • Category: correctness
  • Problem: The 'scanned' field in ReleaseGatewayPortResult indicates whether lsof scan was attempted and completed. It's true when lsof scan was attempted and completed (exit code 0 or 1), false when lsof unavailable or scan failed. This distinction is important for callers but not documented.
  • Impact: Callers may misinterpret the field meaning, leading to incorrect logic.
  • Suggested action: Add JSDoc comment to the 'scanned' field: 'true when lsof scan was attempted and completed (exit code 0 or 1), false when lsof unavailable or scan failed.'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read src/lib/tunnel/gateway-port-release.ts lines 45-55. Confirm JSDoc comment on 'scanned' field.
  • Missing regression test: N/A — documentation concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: ReleaseGatewayPortResult interface at lines 45-55 has 'scanned: boolean' field with no JSDoc.

PRA-16 Improvement — PORT_FREE_PROBE_SCRIPT minified inline string reduces readability

  • Location: src/lib/tunnel/gateway-port-confirmation.ts:12
  • Category: architecture
  • Problem: The PORT_FREE_PROBE_SCRIPT constant is a minified inline template literal that implements the child process bind probe. It's hard to read and maintain. Should be formatted as multi-line with comments, extracted to a separate file, or eliminated entirely by the PRA-5 fix.
  • Impact: Reduced auditability and maintainability of the bind probe logic.
  • Suggested action: Convert to multi-line template literal with comments explaining the net.createServer bind logic. Or extract to src/lib/tunnel/probe-port-free.cjs and load via fs.readFileSync. (Also resolved by PRA-5 fix which removes the script entirely.)
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read src/lib/tunnel/gateway-port-confirmation.ts lines 12-30. Verify script is formatted readably or extracted/eliminated.
  • Missing regression test: N/A — code quality concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PORT_FREE_PROBE_SCRIPT at lines 12-30 is a minified single-line template literal.

PRA-21 Improvement — Deprecated stop command description could be clearer about what it does vs tunnel stop

  • Location: src/commands/stop.ts:14
  • Category: correctness
  • Problem: The deprecated 'nemoclaw stop' command description says 'Deprecated: use 'nemoclaw tunnel stop' for tunnel-only shutdown. This legacy command also releases the managed host gateway port.' This is good but could explicitly state that 'nemoclaw stop' = 'tunnel stop' + 'gateway port release' to make the difference crystal clear.
  • Impact: User confusion about the difference between the two stop commands.
  • Suggested action: Update description to: 'Stop tunnel services and release the managed host gateway port. Use "nemoclaw tunnel stop" to stop only tunnel services (cloudflared) while preserving the shared gateway.'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read src/commands/stop.ts lines 10-18. Verify description clearly distinguishes the two commands.
  • Missing regression test: N/A — documentation/UX concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: stop.ts lines 10-18: summary 'Deprecated full stop (also releases the managed gateway port)', description mentions both but could be more explicit.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Integration test: concurrent onboard for same port enforces singleton — spawn two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT, verify only one acquires lock and binds port (PRA-T1, blocks merge). Runtime/sandbox/infrastructure paths need behavioral runtime validation. New modules (docker-driver-gateway-cutover, docker-driver-gateway-port-listener, docker-driver-gateway-prelaunch, gateway-port-release, gateway-stop) exercise real process management, lsof parsing, PID signaling, and port binding. Unit tests mock these boundaries well, but integration/runtime tests are needed for: SAME-port concurrent onboard (Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2), container vs host-process gateway conflict (clause 1), Windows/lsof-absent coverage, and malformed lsof output.
  • PRA-T2 Runtime validation — Integration test: container gateway and host-process gateway cannot both bind same port — verify mutual exclusion across gateway modes (PRA-T2, blocks merge). Runtime/sandbox/infrastructure paths need behavioral runtime validation. New modules (docker-driver-gateway-cutover, docker-driver-gateway-port-listener, docker-driver-gateway-prelaunch, gateway-port-release, gateway-stop) exercise real process management, lsof parsing, PID signaling, and port binding. Unit tests mock these boundaries well, but integration/runtime tests are needed for: SAME-port concurrent onboard (Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2), container vs host-process gateway conflict (clause 1), Windows/lsof-absent coverage, and malformed lsof output.
  • PRA-T3 Runtime validation — Mock-based unit test: gateway port release logic on Windows without lsof — mock lsof/spawnSync to cover same branches (PRA-T3). Runtime/sandbox/infrastructure paths need behavioral runtime validation. New modules (docker-driver-gateway-cutover, docker-driver-gateway-port-listener, docker-driver-gateway-prelaunch, gateway-port-release, gateway-stop) exercise real process management, lsof parsing, PID signaling, and port binding. Unit tests mock these boundaries well, but integration/runtime tests are needed for: SAME-port concurrent onboard (Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2), container vs host-process gateway conflict (clause 1), Windows/lsof-absent coverage, and malformed lsof output.
  • PRA-T4 Runtime validation — Unit test: malformed lsof output handling in port listener scan — non-numeric, negative, huge PIDs, excessive line count (PRA-T4). Runtime/sandbox/infrastructure paths need behavioral runtime validation. New modules (docker-driver-gateway-cutover, docker-driver-gateway-port-listener, docker-driver-gateway-prelaunch, gateway-port-release, gateway-stop) exercise real process management, lsof parsing, PID signaling, and port binding. Unit tests mock these boundaries well, but integration/runtime tests are needed for: SAME-port concurrent onboard (Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2), container vs host-process gateway conflict (clause 1), Windows/lsof-absent coverage, and malformed lsof output.
  • PRA-T5 Runtime validation — Unit test: destroy remediation hint in gateway-stop warnings — verify 'Consider nemoclaw <name> destroy' appears (PRA-T5). Runtime/sandbox/infrastructure paths need behavioral runtime validation. New modules (docker-driver-gateway-cutover, docker-driver-gateway-port-listener, docker-driver-gateway-prelaunch, gateway-port-release, gateway-stop) exercise real process management, lsof parsing, PID signaling, and port binding. Unit tests mock these boundaries well, but integration/runtime tests are needed for: SAME-port concurrent onboard (Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2), container vs host-process gateway conflict (clause 1), Windows/lsof-absent coverage, and malformed lsof output.
  • PRA-T6 Linux E2E workflow missing gateway lifecycle regression tests — Add a step to the Linux E2E workflow (or a unit test job) that runs the gateway lifecycle regression tests: npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts. The integration project in vitest.config.ts already exists.
  • PRA-T7 Runtime validation test skipped on Windows and when lsof absent — no alternative coverage — Add a conditional unit test that mocks lsof/spawnSync to cover the same logic on Windows, OR document why Windows is not a supported gateway host platform (NemoClaw targets Linux/macOS).
  • PRA-T8 confirmGatewayPortReleased maxAttempts bound (20) not explicitly tested as a regression guard — Add explicit test comment or assertion documenting that maxAttempts=20 is a security bound limiting subprocess spawn attempts. The existing test covers the behavior but the intent should be explicit.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/tunnel/gateway-port-confirmation.ts:42 defaultProbePortFree spawnSync

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: gateway-port-release-fail-closed.test.ts: 'runs one bind proof for one managed gateway release'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: defaultProbePortFree at line 42 uses spawnSync; PORT_FREE_PROBE_SCRIPT at line 12 is the child code; confirmGatewayPortReleased at line 82 calls probePortFree once after listener polling

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/tunnel/gateway-port-confirmation.ts:12 PORT_FREE_PROBE_SCRIPT minified inline

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: N/A
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PORT_FREE_PROBE_SCRIPT at lines 12-30 is a minified single-line template literal implementing net.createServer bind logic

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/onboard/host-gateway-process.ts:84 defaultCommandExists sh -c command -v

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Unit test for commandExists replacement verifying correct behavior for 'pgrep' and non-existent commands
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: defaultCommandExists at line 84 uses sh -c with JSON.stringify. defaultGatewayReleaseCommandExists at gateway-port-listeners.ts:35 uses process.env.PATH.split + fs.accessSync

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/onboard/docker-driver-gateway-cutover.ts:35 DockerDriverGatewayCutoverDeps interface (16 methods)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/onboard-gateway-prelaunch-cutover.test.ts mocks all deps
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: DockerDriverGatewayCutoverDeps defines 16 methods including isDockerDriverGatewayProcessAlive, getDockerDriverGatewayRuntimeDrift, reapHostGatewayBeforeLaunchOrFail, reapDuplicateHostGatewaysExceptOrFail, etc. — many exist in runtime module.

PRA-5 Resolve/justify — Source-of-truth review needed: src/lib/onboard/docker-driver-gateway-prelaunch.ts:1 prelaunch reaping workaround

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/onboard-gateway-prelaunch-cutover.test.ts covers reaping behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Header comment lines 1-30 explains historical problem and workaround. No explicit removal condition stated.

PRA-6 Required — defaultProbePortFree spawns Node child process per stop invocation

  • Location: src/lib/tunnel/gateway-port-confirmation.ts:42
  • Category: security
  • Problem: defaultProbePortFree uses spawnSync(process.execPath, ['-e', PORT_FREE_PROBE_SCRIPT, port]) to prove a port is free. This spawns a short-lived Node child process on every 'nemoclaw stop' call. While the current code only calls it once per releaseManagedGatewayPort invocation (not 20 times as previously thought), it still creates unnecessary subprocess overhead and a minor DoS vector under automated CI or repeated stop calls.
  • Impact: Subprocess spawn overhead accumulates in CI; could be abused as minor DoS vector. Eliminates PORT_FREE_PROBE_SCRIPT inline script which reduces auditability.
  • Required action: Replace spawnSync-based probe with synchronous net.createServer().listen() attempt in the main process. Since confirmGatewayPortReleased is synchronous, use a one-shot server that binds, immediately closes, and returns success/failure. Follow the canBind() pattern from test/tunnel-gateway-port-release-runtime.test.ts:112.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read src/lib/tunnel/gateway-port-confirmation.ts lines 42-55. Confirm defaultProbePortFree no longer uses spawnSync with PORT_FREE_PROBE_SCRIPT. The function should use net.createServer().listen(port, '127.0.0.1') with a callback that closes and returns true/false.
  • Missing regression test: Add test mocking spawnSync to verify it is NOT called when releaseManagedGatewayPort invokes probePortFree. Existing test 'runs one bind proof for one managed gateway release' in gateway-port-release-fail-closed.test.ts covers call count.
  • Done when: The required change is committed and verification passes: Read src/lib/tunnel/gateway-port-confirmation.ts lines 42-55. Confirm defaultProbePortFree no longer uses spawnSync with PORT_FREE_PROBE_SCRIPT. The function should use net.createServer().listen(port, '127.0.0.1') with a callback that closes and returns true/false.
  • Evidence: defaultProbePortFree at line 42 uses spawnSync; PORT_FREE_PROBE_SCRIPT at line 12 is minified inline string; confirmGatewayPortReleased at line 82 calls probePortFree once after listener polling.

PRA-7 Required — Creation-path singleton enforcement (Issue #5968 clause 2) lacks integration test for SAME-port concurrent onboard

  • Location: src/lib/onboard.ts:2144
  • Category: acceptance
  • Problem: Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 2 requires 'gateway must be shared (exactly one instance across container + host-process modes)'. The PR implements singleton enforcement via acquireOnboardLock() filesystem lock (openSync 'wx') and post-reap bind check, but no integration test proves two concurrent 'nemoclaw onboard' invocations for the SAME port cannot both succeed. The existing E2E test concurrent-gateway-ports.test.ts tests DIFFERENT ports (8080 and 18080).
  • Impact: Without this test, the singleton guarantee for the critical concurrent creation path is unproven. Race conditions could allow two gateways to bind the same port.
  • Required action: Add integration test proving no two host gateways can bind the same port under concurrent onboard invocations. Spawn two CLI processes attempting to onboard with same NEMOCLAW_GATEWAY_PORT and verify only one acquires lock and binds port. If not feasible in this PR, create follow-up issue with acceptance criteria and update Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 to 'Refs [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968'.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search test/ for concurrent onboard test with SAME gateway port. Confirm it spawns two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT and verifies only one acquires lock and binds port.
  • Missing regression test: Integration test: concurrent onboard for same port enforces singleton — spawn two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT, verify only one acquires lock and binds port.
  • Done when: The required change is committed and verification passes: Search test/ for concurrent onboard test with SAME gateway port. Confirm it spawns two nemoclaw onboard processes with same NEMOCLAW_GATEWAY_PORT and verifies only one acquires lock and binds port.
  • Evidence: acquireOnboardLock() at onboard-session.ts:671 uses openSync('wx') atomic create. startDockerDriverGateway in onboard.ts:2068 holds lock across gateway creation. Post-reap bind check in cutover adds second boundary. concurrent-gateway-ports.test.ts tests DIFFERENT ports only.

PRA-8 Resolve/justify — Monolith test file at 1377 lines — exceeds growth threshold

  • Location: src/lib/state/onboard-session.test.ts:1377
  • Category: architecture
  • Problem: The onboard-session.test.ts file is a monolith at 1377 lines. Previous review flagged 34 lines of growth exceeding the 20-line threshold. While the cross-process lock test was correctly placed in a separate file (onboard-session-cross-process-lock.test.ts), the main test file remains a monolith that should be modularized.
  • Impact: Maintainability risk; new test helpers should be extracted to separate files rather than adding to this monolith.
  • Recommended action: Extract new test helpers into separate files under test/helpers/ or src/lib/state/onboard-session-*.test.ts. Follow the pattern established by onboard-session-cross-process-lock.test.ts. At minimum, justify why new tests cannot be modularized.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run wc -l src/lib/state/onboard-session.test.ts. Verify new test helpers added in this PR are in separate files.
  • Missing regression test: N/A — architecture/maintainability concern, not functional regression.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run wc -l src/lib/state/onboard-session.test.ts. Verify new test helpers added in this PR are in separate files.
  • Evidence: File is 1377 lines. Previous review noted growth from 1376 to 1410 (34 lines). Cross-process lock test correctly isolated in separate file.

PRA-9 Resolve/justify — defaultCommandExists uses sh -c 'command -v' despite trusted literal

  • Location: src/lib/onboard/host-gateway-process.ts:84
  • Category: security
  • Problem: defaultCommandExists uses defaultRun('sh', ['-c', `command -v ${JSON.stringify(command)} >/dev/null 2>&1`]) to check if a command exists. The command is always an internal trusted literal ('pgrep'), and JSON.stringify provides quoting, but this still spawns a shell process unnecessarily. A pure-JS PATH search using process.env.PATH.split(path.delimiter) and fs.accessSync(path.join(dir, command), fs.constants.X_OK) would be simpler, faster, and avoid shell invocation.
  • Impact: Unnecessary shell process spawn; attack surface increase (though minimal with trusted literal). Pure-JS alternative already exists in gateway-port-listeners.ts:35 as defaultGatewayReleaseCommandExists.
  • Recommended action: Replace with pure-JS PATH search matching the pattern in gateway-port-listeners.ts:35 (defaultGatewayReleaseCommandExists). This also resolves the source-of-truth concern from previous review.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/host-gateway-process.ts lines 80-90. Confirm defaultCommandExists no longer uses sh -c and instead uses PATH split + fs.accessSync.
  • Missing regression test: Unit test for commandExists replacement verifying it correctly finds 'pgrep' on PATH and returns false for non-existent commands.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/host-gateway-process.ts lines 80-90. Confirm defaultCommandExists no longer uses sh -c and instead uses PATH split + fs.accessSync.
  • Evidence: defaultCommandExists at line 84 uses sh -c with JSON.stringify. defaultGatewayReleaseCommandExists at gateway-port-listeners.ts:35 uses pure-JS PATH search.

PRA-10 Resolve/justify — Peer registry corruption error lacks actionable nemoclaw destroy guidance

  • Location: src/lib/tunnel/gateway-stop.ts:103
  • Category: correctness
  • Problem: When a corrupt peer registry entry makes gateway ownership ambiguous, the warning message says 'repair the sandbox registry and retry' but doesn't mention 'nemoclaw <name> destroy' as a remediation. Users may not know how to repair the registry.
  • Impact: Operators facing registry corruption get unactionable error message, prolonging outage.
  • Recommended action: Update warn message to include: 'Consider nemoclaw <name> destroy to clear stale entries.' Also consider adding registry validation on write to prevent corrupt entries.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-stop.ts lines 100-110. Confirm warn message includes 'Consider nemoclaw <name> destroy to clear stale entries.'
  • Missing regression test: Test verifying the warning message contains the destroy remediation hint when gateway-stop catches a registry error.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-stop.ts lines 100-110. Confirm warn message includes 'Consider nemoclaw <name> destroy to clear stale entries.'.
  • Evidence: gateway-stop.ts:103 warns 'repair the sandbox registry and retry' without destroy hint. NODE_DEBUG output at line 112 also lacks it.

PRA-11 Resolve/justify — NODE_DEBUG console.error lacks nemoclaw destroy remediation hint

  • Location: src/lib/tunnel/gateway-stop.ts:112
  • Category: correctness
  • Problem: The NODE_DEBUG=nemoclaw:gateway diagnostic output shows the stack trace but doesn't include the 'Consider nemoclaw <name> destroy' remediation hint that the warn message should have.
  • Impact: Debug output for operators explicitly debugging gateway teardown lacks actionable remediation.
  • Recommended action: Add the same destroy remediation hint to the NODE_DEBUG output, or ensure the warn message (with hint) is always shown alongside the debug output.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-stop.ts lines 110-115. Confirm the debug output or accompanying warn includes the destroy remediation.
  • Missing regression test: Test with NODE_DEBUG=nemoclaw:gateway verifying destroy hint appears in output when registry error occurs.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-stop.ts lines 110-115. Confirm the debug output or accompanying warn includes the destroy remediation.
  • Evidence: gateway-stop.ts:112 logs error stack via console.error when NODE_DEBUG includes nemoclaw:gateway, but no remediation hint.

PRA-12 Resolve/justify — Port listener helpers re-exported through runtime facade instead of direct import

  • Location: src/lib/onboard/docker-driver-gateway-runtime.ts:25
  • Category: architecture
  • Problem: docker-driver-gateway-runtime.ts re-exports getDockerDriverGatewayPortListenerPid, getDockerDriverGatewayPortListenerScan, isDockerDriverGatewayPortListener from docker-driver-gateway-port-listener.ts. This creates unnecessary indirection. onboard.ts should import port listener helpers directly from docker-driver-gateway-port-listener.ts.
  • Impact: Maintenance burden; unclear dependency graph; harder to trace code flow.
  • Recommended action: Update onboard.ts to import port listener helpers directly from docker-driver-gateway-port-listener.ts. Remove the re-exports from docker-driver-gateway-runtime.ts.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check imports in src/lib/onboard.ts for port listener helpers. Verify they come from docker-driver-gateway-port-listener.ts, not docker-driver-gateway-runtime.ts.
  • Missing regression test: N/A — architecture/refactoring concern.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check imports in src/lib/onboard.ts for port listener helpers. Verify they come from docker-driver-gateway-port-listener.ts, not docker-driver-gateway-runtime.ts.
  • Evidence: docker-driver-gateway-runtime.ts lines 25-30 re-export three functions from docker-driver-gateway-port-listener.ts. onboard.ts imports from runtime facade.

PRA-13 Resolve/justify — Linux E2E workflow missing gateway lifecycle regression tests

  • Location: .github/workflows/macos-e2e.yaml:45
  • Category: tests
  • Problem: The macOS E2E workflow now runs gateway lifecycle regression tests (tunnel-gateway-port-release-runtime.test.ts and onboard-gateway-prelaunch-cutover.test.ts) via the new 'Run gateway lifecycle regressions' step. However, the Linux E2E workflow (.github/workflows/e2e.yaml) does not have an equivalent step. These tests validate critical gateway port release logic and should run on Linux CI as well.
  • Impact: Gateway lifecycle regressions may go undetected on Linux, the primary platform.
  • Recommended action: Add a step to the Linux E2E workflow (or a unit test job) that runs the gateway lifecycle regression tests: npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts. The integration project in vitest.config.ts already exists.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read .github/workflows/e2e.yaml. Confirm it has a step running npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts
  • Missing regression test: CI job running gateway lifecycle regression tests on Linux.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read .github/workflows/e2e.yaml. Confirm it has a step running npx vitest run --project integration test/tunnel-gateway-port-release-runtime.test.ts test/onboard-gateway-prelaunch-cutover.test.ts.
  • Evidence: macos-e2e.yaml lines 45-48 added the step. e2e.yaml has no equivalent.

PRA-14 Resolve/justify — DockerDriverGatewayCutoverDeps interface duplicates runtime module logic (16 methods)

  • Location: src/lib/onboard/docker-driver-gateway-cutover.ts:35
  • Category: architecture
  • Problem: The DockerDriverGatewayCutoverDeps interface defines 16 methods, many duplicating helpers from docker-driver-gateway-runtime.ts and host-gateway-process.ts. This creates a large dependency surface that's hard to maintain. The cutover logic should use runtime module helpers directly, or a shared facade should be created.
  • Impact: Maintenance burden; duplication risk; unclear ownership of logic.
  • Recommended action: Evaluate consolidating cutover dependencies. Consider having cutover use runtime module helpers directly, or create a shared facade. At minimum, document why each dep is needed and cannot be sourced from runtime module.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/docker-driver-gateway-cutover.ts lines 35-80. Compare each dep method with docker-driver-gateway-runtime.ts and host-gateway-process.ts exports. Identify which can be sourced from existing modules.
  • Missing regression test: N/A — architecture/refactoring concern.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/docker-driver-gateway-cutover.ts lines 35-80. Compare each dep method with docker-driver-gateway-runtime.ts and host-gateway-process.ts exports. Identify which can be sourced from existing modules.
  • Evidence: DockerDriverGatewayCutoverDeps has 16 methods including isDockerDriverGatewayProcessAlive, isGatewayHealthy, getDockerDriverGatewayRuntimeDrift, logDockerDriverGatewayRestart, registerDockerDriverGatewayEndpoint, isDockerDriverGatewayHttpReady, verifySandboxBridgeGatewayReachableOrExit, readGatewayHealth, rememberDockerDriverGatewayPid, reapDuplicateHostGatewaysExceptOrFail, reapHostGatewayBeforeLaunchOrFail, isGatewayPortAvailable, reportUntrustedGatewayPort, reportMissingGatewayBinary, log.

PRA-15 Improvement — ReleaseGatewayPortResult.scanned field lacks clarifying JSDoc comment

  • Location: src/lib/tunnel/gateway-port-release.ts:50
  • Category: correctness
  • Problem: The 'scanned' field in ReleaseGatewayPortResult indicates whether lsof scan was attempted and completed. It's true when lsof scan was attempted and completed (exit code 0 or 1), false when lsof unavailable or scan failed. This distinction is important for callers but not documented.
  • Impact: Callers may misinterpret the field meaning, leading to incorrect logic.
  • Suggested action: Add JSDoc comment to the 'scanned' field: 'true when lsof scan was attempted and completed (exit code 0 or 1), false when lsof unavailable or scan failed.'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read src/lib/tunnel/gateway-port-release.ts lines 45-55. Confirm JSDoc comment on 'scanned' field.
  • Missing regression test: N/A — documentation concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: ReleaseGatewayPortResult interface at lines 45-55 has 'scanned: boolean' field with no JSDoc.

PRA-16 Improvement — PORT_FREE_PROBE_SCRIPT minified inline string reduces readability

  • Location: src/lib/tunnel/gateway-port-confirmation.ts:12
  • Category: architecture
  • Problem: The PORT_FREE_PROBE_SCRIPT constant is a minified inline template literal that implements the child process bind probe. It's hard to read and maintain. Should be formatted as multi-line with comments, extracted to a separate file, or eliminated entirely by the PRA-5 fix.
  • Impact: Reduced auditability and maintainability of the bind probe logic.
  • Suggested action: Convert to multi-line template literal with comments explaining the net.createServer bind logic. Or extract to src/lib/tunnel/probe-port-free.cjs and load via fs.readFileSync. (Also resolved by PRA-5 fix which removes the script entirely.)
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read src/lib/tunnel/gateway-port-confirmation.ts lines 12-30. Verify script is formatted readably or extracted/eliminated.
  • Missing regression test: N/A — code quality concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PORT_FREE_PROBE_SCRIPT at lines 12-30 is a minified single-line template literal.

PRA-17 Resolve/justify — Runtime validation test skipped on Windows and when lsof absent — no alternative coverage

  • Location: test/tunnel-gateway-port-release-runtime.test.ts:1
  • Category: tests
  • Problem: The runtime validation test uses it.skipIf(!posix || !hasLsof) which skips on Windows and when lsof is absent. There's no conditional unit test that mocks lsof/spawnSync to cover the same logic on Windows. Since NemoClaw targets Linux/macOS as host platforms, this may be acceptable but should be documented or covered.
  • Impact: No test coverage for gateway port release logic on Windows or in environments without lsof.
  • Recommended action: Add a conditional unit test that mocks lsof/spawnSync to cover the same logic on Windows, OR document why Windows is not a supported gateway host platform (NemoClaw targets Linux/macOS).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if there's a unit test mocking lsof/spawnSync for gateway port release on Windows. Read test/tunnel-gateway-port-release-runtime.test.ts header comments.
  • Missing regression test: Mock-based unit test covering gateway port release logic on Windows (without requiring lsof).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if there's a unit test mocking lsof/spawnSync for gateway port release on Windows. Read test/tunnel-gateway-port-release-runtime.test.ts header comments.
  • Evidence: test/tunnel-gateway-port-release-runtime.test.ts uses it.skipIf(!posix || !hasLsof) at line 38. posix = process.platform !== 'win32'; hasLsof checks lsof -v.

PRA-18 Resolve/justify — lsof output parsing trusts lsof -ti format without bounds validation

  • Location: src/lib/onboard/docker-driver-gateway-port-listener.ts:75
  • Category: security
  • Problem: getDockerDriverGatewayPortListenerScan parses lsof -ti output with parseListenerPids which splits on newlines and parses integers. While it filters for positive integers, there's no max PID count limit, no PID range validation (1-4194304), and no handling of unexpected output formats. A malicious or corrupted lsof binary could output excessive data.
  • Impact: Defense-in-depth gap; potential for resource exhaustion or logic errors if lsof output is malformed.
  • Recommended action: Add stricter validation: limit max PIDs parsed (e.g., 100), validate each PID is within reasonable range (1-4194304), handle unexpected output formats gracefully.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/onboard/docker-driver-gateway-port-listener.ts lines 60-80. Verify parseListenerPids has bounds checking and input validation.
  • Missing regression test: Test with malformed lsof output (non-numeric, negative, extremely large PIDs, excessive line count) verifying graceful handling.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/onboard/docker-driver-gateway-port-listener.ts lines 60-80. Verify parseListenerPids has bounds checking and input validation.
  • Evidence: parseListenerPids at line 60 splits on \r?\n, maps to parseInt, filters Number.isInteger && pid > 0. No max count, no upper bound.

PRA-19 Resolve/justify — releaseManagedGatewayPort continues with empty PID list after lsof failure instead of early return

  • Location: src/lib/tunnel/gateway-port-release.ts:115
  • Category: correctness
  • Problem: When lsof exits with status >1 (genuine error), releaseManagedGatewayPort sets scanFailed=true and scanned=false, but then calls stopHostGatewayProcesses with empty pids list and usePidFile=false. This is correct fail-closed behavior but the control flow could be clearer with an early return.
  • Impact: Code clarity; maintainers may misread the fail-closed intent.
  • Recommended action: Clarify control flow: when lsof fails (status >1), skip the stop call entirely and return early with released=false, scanned=false. Makes fail-closed behavior more explicit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read src/lib/tunnel/gateway-port-release.ts lines 100-130. Verify lsof failure path returns early without calling stopHostGatewayProcesses.
  • Missing regression test: Test in gateway-port-release-fail-closed.test.ts already covers this: 'warns and refuses unsafe pid-file cleanup when lsof exits with a real failure'.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/tunnel/gateway-port-release.ts lines 100-130. Verify lsof failure path returns early without calling stopHostGatewayProcesses.
  • Evidence: Lines 115-125: after lsof failure, scanFailed=true, then stopHostGatewayProcesses called with pids: [] and usePidFile: false.

PRA-20 Resolve/justify — Issue #5968 clause 1 — gateway shared across container + host-process modes not explicitly tested

  • Location: src/lib/onboard.ts:1986
  • Category: acceptance
  • Problem: Issue [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 clause 1 requires 'gateway must be shared (exactly one instance across container + host-process modes)'. The cutover logic handles host-process gateway reuse/replacement, but there's no test proving a container gateway and host-process gateway cannot both bind the same port simultaneously.
  • Impact: Container vs host-process gateway port conflict scenario untested; potential for both to bind same port.
  • Recommended action: Add test (unit or integration) verifying that when a Docker container gateway is running on a port, the host-process gateway cannot also bind that port (and vice versa). May require Docker in CI.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search test/ for container-vs-host gateway port conflict test. Verify it tests both directions.
  • Missing regression test: Test proving container gateway and host-process gateway cannot both bind same port.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search test/ for container-vs-host gateway port conflict test. Verify it tests both directions.
  • Evidence: Cutover logic in docker-driver-gateway-cutover.ts handles host-process reuse/replacement. No test for container gateway coexistence.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

The codebase-growth-guardrails check forbids adding `if` statements in
changed test files. Rewrite the lsofResponder helper to branch with a
ternary instead, keeping behavior identical.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/tunnel/gateway-port-release.test.ts`:
- Around line 61-65: The new conditional in the test helper is unnecessary and
is causing CI issues; remove the `if (command !== "lsof") return ok();` branch
from the `run` stub in `gateway-port-release.test.ts` so the helper always
follows the injected `lsof` probe behavior. Keep the existing response selection
logic using `state.calls`, `responses`, and `ok()` unchanged so the test
coverage remains the same while eliminating the branch.

In `@src/lib/tunnel/gateway-port-release.ts`:
- Around line 229-237: The release confirmation in gateway-port-release.ts is
treating a failed listening probe as success by coercing null from
listeningPids() into an empty array inside the waitUntil predicate. Update the
logic around waitUntil and the released/remaining handling so that a lsof error
does not count as a released port; only consider the port released when
listeningPids() returns a real empty list, and preserve null/error cases so they
can be retried or surfaced instead of being mistaken for success.
- Around line 121-129: The fallback in gateway port resolution is swallowing
malformed sandbox binding errors and incorrectly returning the process-wide
default port. Update resolveGatewayPortFromName() handling in
gateway-port-release so it only falls back when no sandbox entry exists or the
lookup is genuinely absent, and let resolveSandboxGatewayName() failures
propagate to the caller. Keep the boundary handling in stopAll({ sandboxName })
so invalid persisted bindings fail the sandbox-specific release instead of
retargeting the default gateway.

In `@src/lib/tunnel/services.ts`:
- Around line 629-630: The gateway-port release in services.ts should not fall
back to the process-wide default when no sandbox identity is available. Update
the stopAll({ pidDir }) flow around releaseManagedGatewayPort so it only
releases a managed gateway port when sandboxName was resolved, and otherwise
skip the release entirely rather than passing {}. Use the existing sandboxName
check near the releaseManagedGatewayPort call to keep the teardown scoped to the
pidDir-selected service.
🪄 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: 5b87f353-171b-4cf0-91be-92852de2a7a9

📥 Commits

Reviewing files that changed from the base of the PR and between c6113be and d291bdb.

📒 Files selected for processing (4)
  • src/lib/tunnel/gateway-port-release.test.ts
  • src/lib/tunnel/gateway-port-release.ts
  • src/lib/tunnel/services.test.ts
  • src/lib/tunnel/services.ts

Comment thread src/lib/tunnel/gateway-port-release.test.ts Outdated
Comment thread src/lib/tunnel/gateway-port-release.ts Outdated
Comment thread src/lib/tunnel/gateway-port-release.ts Outdated
Comment thread src/lib/tunnel/services.ts Outdated
…stop

Addresses PR Review Advisor PRA-1/PRA-2: `resolveStopGatewayPort` previously
caught any error from `resolveSandboxGatewayName` and fell back to the
process-wide `GATEWAY_PORT`. For a corrupt or tampered registry row that
silently retargeted the destructive stop path (pid-file derivation, lsof
scan, signal delivery) at the default gateway — potentially another
sandbox's or worktree's `openshell-gateway`.

Now resolution returns null when a sandbox *has* a persisted gateway binding
that fails validation, and `releaseManagedGatewayPort` skips the destructive
path entirely (no lsof, no stopHostGatewayProcesses) with a warning. The
legacy/no-registry fallback to `GATEWAY_PORT` is preserved only for a missing
entry or a legacy entry with no gateway fields, mirroring the fail-closed
contract of `resolveSandboxGatewayName`.

Adds regression tests: fail-closed port resolution, invalid-binding skip
(no default-port cleanup), non-matching listener left alone without sudo
remediation, and lsof real-failure pid-file fallback.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yimoj

yimoj commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Response to PR Review Advisor (run on d291bdbd4)

Thanks — PRA-1/PRA-2 were valid and are now fixed in commit on top.

PRA-1 / PRA-2 — fail-closed on invalid persisted gateway binding (fixed)

resolveStopGatewayPort no longer swallows a validation error and returns GATEWAY_PORT. It now mirrors the fail-closed contract of resolveSandboxGatewayName:

  • Invalid state: a registry entry whose gatewayName/gatewayPort is corrupt or tampered (e.g. out-of-range port), which resolveSandboxGatewayName already throws on.
  • Source boundary: the persisted sandbox registry row, resolved through resolveSandboxGatewayName.
  • Behavior: when the entry exists but fails validation, resolveStopGatewayPort returns null and releaseManagedGatewayPort skips the destructive path entirely (no pid-file derivation, no lsof, no stopHostGatewayProcesses) and emits a warning. The legacy/no-registry fallback to GATEWAY_PORT is kept only for a missing entry or a legacy entry with no gateway fields (where resolveSandboxGatewayName returns the base nemoclaw name → port 8080).
  • Removal condition: none — this is a permanent invariant, not a localized workaround.

Test follow-ups

  • PRA-T8 / PRA-T1 (added): fails closed (null) when the persisted gateway binding is invalid and does not fall back to the default port when the persisted gateway binding is invalid — asserts no stopHostGatewayProcesses call, no lsof, skipped=true, and a warning.
  • PRA-T2 (added): leaves a non-matching listener alone without sudo pkill remediationlsof returns a PID the stopper classifies as skippedNonMatchingPids; asserts no remediation warning.
  • PRA-T3 (added): warns and falls back to pid-file cleanup when lsof exits with a real failurelsof status > 1 surfaces a warning and still delegates to the pid-file stopper.
  • PRA-T4 / PRA-T5 / PRA-T6 (real-CLI runtime validation): a hermetic real-worktree CLI transcript is posted above — a process whose argv0 is openshell-gateway bound to a unique port, then node ./bin/nemoclaw.js stop, which reaps it and confirms a squatter can rebind. This exercises the full lsof → cmdline-gate → TERM/KILL → port-free path. The macos-e2e CI job covers the original macOS host-process scenario directly; a maintainer can also run it on the macOS E2E host.
  • PRA-T7 (justified as out of scope / follow-up): proving the start/onboard singleton invariant (no two live host gateways before stop) is the duplicate-creation path, a separate axis from this stop-time fix. This PR reaps duplicate/orphan listeners at stop; the creation-side invariant is tracked separately and intentionally not widened here to keep scope tight.

…nfirm probe

Addresses two CodeRabbit findings:

- services.ts: `stopAll` now only releases the gateway when a sandbox identity
  was resolved. With no sandbox name the port resolver would fall back to the
  process-wide default gateway port — not tied to the selected pidDir — which
  could tear down another worktree's default gateway. Skip the release instead.
- gateway-port-release.ts: a transient `lsof` failure during the confirmation
  poll previously coerced `null` to `[]`, reporting the port as released
  without ever confirming it was free. The poll now tracks a failed probe and
  refuses to report released on an lsof error.

Updates/adds tests: stop skips release when no sandbox name resolves, and a
failed confirmation probe is not reported as a released port.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yimoj

yimoj commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Re: PRA-3 / PRA-4

  • PRA-3 (acceptance covered only by mocked lsof/stopper): added a real worktree-CLI transcript to the PR description (Verification section) — it stands up a host process whose argv0 is openshell-gateway bound to a unique port, runs node ./bin/nemoclaw.js stop, and confirms the process is reaped and a squatter can rebind. This exercises the unmocked lsof → cmdline-gate → TERM/KILL → port-free path. The macos-e2e CI job covers the original macOS host-process scenario.
  • PRA-4 (singleton only addressed as stop-time cleanup): intentional scope. This issue ([macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968) is the stop-time release of the gateway port. Preventing two live host gateways from being created during start/onboard is a separate axis (the duplicate-creation path) and is deliberately out of scope here to keep the change tight. This PR reaps duplicate/orphan listeners at stop, which resolves the reporter's host-process=2-after-stop symptom; the creation-side singleton invariant is a follow-up.

Addresses PR Review Advisor PRA-3: `lazyGetSandbox` previously caught a
registry read error and returned null, which `resolveStopGatewayPort` then
treated as a clean "no entry" and fell back to destructive default-port
cleanup. A corrupt/unreadable registry now propagates and is handled as a
fail-closed skip (no lsof, no stopHostGatewayProcesses), the same as an
invalid persisted binding.

Adds tests for the lookup-throws path at both the resolver and
releaseManagedGatewayPort layers; widens the skip warning to cover
"invalid or unreadable".

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yimoj

yimoj commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Advisor items — consolidated status (head f1bb81321)

Fixed in code (fail-closed source-of-truth):

  • Invalid persisted gateway binding → resolveStopGatewayPort returns null; release is skipped (no lsof, no stopHostGatewayProcesses). No default-port fallback.
  • Registry lookup failure (corrupt/unreadable registry that throws) → also fails closed and skips, instead of being treated as a clean "no entry".
  • stopAll only releases when a sandbox identity is resolved (no {}/default-port path that could hit another worktree).
  • Confirmation poll: a transient lsof error no longer counts as "released".

Tests added for each: invalid binding (resolver + release), lookup-throws (resolver + release), confirm-probe failure, non-matching listener (no remediation), lsof real-failure fallback, no-sandbox skip.

Justified (no code change):

  • Real-runtime acceptance (mocked lsof/stopper): a real worktree-CLI transcript is in the PR description (node ./bin/nemoclaw.js stop reaps a host openshell-gateway and a squatter rebinds). macos-e2e CI covers the macOS host-process scenario.
  • Singleton (creation-side): preventing two live host gateways from being created at start/onboard is a separate axis from this stop-time release and is intentionally out of scope for [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968; this PR reaps duplicate/orphan listeners at stop. Tracked as follow-up.

@yimoj

yimoj commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Advisor items (refreshed numbering on f1bb81321) — resolution map

All hard CI gates are green (incl. macos-e2e, codebase-growth-guardrails, both PR Review Advisor checks, CodeRabbit, dco-check). Mapping the current advisor IDs:

  • PRA-1 (host gateway remains bound after stop): this is the fix's core purpose — stopAll now releases the host openshell-gateway port. Verified by the real-CLI transcript in the PR description and by macos-e2e.
  • PRA-2 (duplicate/orphan host-process=2 cleanup): handled — lsof discovers every listener on the resolved port and the shared stopper reaps each cmdline-matched gateway, so duplicates squatting the port are cleared at stop.
  • PRA-3 / PRA-5 (legacy/no-registry fallback → default port for a named-but-unregistered sandbox): intentional and bounded. A resolved sandbox name with no registry entry is the legacy single-sandbox case; the default gateway port is the only defensible target, and the sweep is still cmdline-gated to a real openshell-gateway process (a non-matching listener is never touched) and usePgrepFallback:false. Invalid bindings and registry read failures already fail closed. Multi-sandbox safety is preserved because the no-sandbox path skips release entirely.
  • PRA-4 (singleton at creation): out of scope for [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 (stop-time release). Preventing two host gateways from being created during start/onboard is a separate axis; this PR reaps duplicates at stop. Follow-up.
  • PRA-6 / PRA-T1–T8 (mocked-only acceptance / runtime validation): a real worktree-CLI transcript (node ./bin/nemoclaw.js stop) is in the PR's Verification section, and macos-e2e exercises the reporter's macOS host-process scenario.

No code-actionable items remain; the above are non-binding warnings for maintainer consideration.

@wscurran wscurran added area: networking DNS, proxy, TLS, ports, host aliases, or connectivity area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jun 29, 2026
@wscurran

Copy link
Copy Markdown
Contributor

…ndbox

resolveStopGatewayPort() coerced a *named* stop whose registry entry was
absent to the process-wide GATEWAY_PORT, so `nemoclaw stop --sandbox
<unknown>` could scan and signal the default `openshell-gateway` that
belongs to another sandbox/worktree — the same hazard stopAll() already
avoids for the no-sandbox path. Fail closed (skip) instead, while still
honoring a real legacy entry (e.g. `{}` → base `nemoclaw` → default port)
and the explicit no-sandbox-name default-cleanup call. Addresses the PR
Review Advisor PRA-5 security finding.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@yimoj

yimoj commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Advisor response (head 290199a6a)

Both advisor passes ran against f1bb813. Below is a per-item disposition. One security finding (PRA-5) is now fixed in code; the rest are addressed as deliberate, scoped design with justification. All real CI lanes (growth-guardrails, static-checks, cli-tests, macos-e2e, self-hosted gateway-isolation/port-overrides E2E, CodeQL, DCO) are green.

Fixed

  • GPT-5.5 PRA-5 (security) — named sandbox with an absent registry entry fell back to default-port cleanup. Confirmed and fixed in 290199a6a. resolveStopGatewayPort() now fails closed (skips) when a named sandbox has no registry entry, instead of coercing to the process-wide GATEWAY_PORT. This closes the exact hole in the safety guard this PR introduced: stopAll() already refuses default-port cleanup for the no-sandbox path because the default port is not tied to the selected pidDir; the named-but-unknown case had the same hazard (nemoclaw stop --sandbox <unknown> could signal another worktree's default openshell-gateway). The fallback is still preserved for the two safe cases the advisor called out: a real legacy entry with no gateway fields (e.g. {} → base nemoclaw → default port) and a direct no-sandbox-name "release the default gateway" call. New regression test skips default-port cleanup for a named sandbox whose registry entry is absent asserts no lsof probe, no stopHostGatewayProcesses() call, and port: null.

Resolve/justify — accepted with rationale

  • GPT-5.5 PRA-4 (Required, acceptance) / PRA-T8 — pre-stop "exactly one gateway instance" not enforced. Scoped intentionally. The issue's blocking symptom (and the failing test T5883720-port-conflict-recovery) is that nemoclaw stop never released the port, so a squatter could not rebind — that is fixed here, plus stop now reaps lsof-discovered duplicate/orphan gateways, covering the reporter's observed host-process=2 at teardown. Preventing a duplicate host gateway from being created during onboard/start is a separate concern in the gateway reuse/binding path (issue Fix Plan item 6: "investigate duplicate host-process start path…"), not the stop lifecycle this PR changes. The PR fixes the reporter's flow end-to-end; any residual pre-stop double-spawn is tracked as follow-up and does not block this teardown fix.

  • GPT-5.5 PRA-1/PRA-2/PRA-3 & Nemotron PRA-11/PRA-12 (architecture, "source-of-truth review"). These are generic localized-patch prompts; the requested fields are already concrete: the invalid state is a corrupt/missing/tampered gateway binding at stop time; the source boundary is the registry write path (onboard/gateway registration), which is the right place to validate ports 1–65535 at write time; this PR is the defensive read-time fail-closed mirror of resolveSandboxGatewayName; regression tests exist (gateway-port-release.test.ts invalid-binding/registry-throw/absent-entry cases); removal condition — the read-time skip can be relaxed once registry write-time validation guarantees a valid binding. The change is at the correct boundary (the stop lifecycle is exactly what failed to release the port); it does not hide an invalid state — it surfaces a warning and skips destructively touching an untrusted port.

  • Nemotron PRA-4/PRA-5 (scope, "monolith growth +34/+22 lines"). The repo's actual growth guardrail (CI / Codebase Growth Guardrails) is green; the advisor's "20-line blocker" is its own heuristic, not the project budget. New logic already lives in a dedicated module (gateway-port-release.ts); the services.ts delta is the minimal call-site wiring (best-effort release behind the if (sandboxName) guard), and the two services.test.ts tests assert that stopAll() actually wires to releaseManagedGatewayPort() only when a sandbox is resolved — integration coverage that the unit suite for the helper cannot provide. Kept deliberately.

  • Nemotron PRA-6/PRA-7/PRA-12 (logging). The catch-all in stopAll() is the intended best-effort contract: a stop must never fail because gateway teardown hit an edge case. Fail-closed resolveStopGatewayPort() catches already surface a user-facing warn(...) explaining the skip. Adding NODE_DEBUG=nemoclaw:gateway stack logging is a reasonable nice-to-have but out of scope for this teardown fix; not required for correctness.

  • Nemotron PRA-8 (repair command in warning). The suggested nemoclaw gateway reset --sandbox <name> command does not exist in the CLI, so it would be misleading guidance. The warning already names the sandbox and tells the operator to resolve the registry entry and re-run stop, which is the correct generic action.

  • Nemotron PRA-10 (single-use constants). DEFAULT_CONFIRM_TIMEOUT_MS / DEFAULT_CONFIRM_POLL_INTERVAL_MS are kept as named constants because they are also the documented defaults backing the public confirmTimeoutMs/confirmPollIntervalMs options; inlining 2000/100 would lose that intent.

Tests — runtime validation (PRA-6/PRA-9/PRA-T1T7)

The mocked unit tests intentionally cover branch decisions (fail-closed, lsof error, confirmation-probe failure, non-matching listener, per-port state dir, sudo remediation, quiet no-op, and now absent-named-entry skip). Real process/port runtime validation is covered by: (1) the self-hosted test-e2e-gateway-isolation and test-e2e-port-overrides CI lanes (green), and (2) the committed local real-CLI E2E in the PR description — a host process whose argv0 is openshell-gateway bound to an isolated per-issue port, reaped by the real built node ./bin/nemoclaw.js stop, after which a squatter rebinds. Spawning a real openshell-gateway inside the fast cli Vitest lane would touch host processes/ports and is deliberately kept out of unit tests.

…lease

Commit the runtime/integration test the advisor asked for (PRA-2 / the
runtime-validation test follow-ups): instead of mocks, it launches a real
process whose argv0 basename is `openshell-gateway` (the identity the host
stopper cmdline-gates on) bound to an isolated non-default port with an
isolated HOME/state dir, runs the REAL releaseManagedGatewayPort (real
spawnSync/ps/kill/stopper), then proves a fresh process can immediately
rebind the freed port — the exact #5968 macOS failure mode.

The fake gateway is launched via a short-lived launcher that exits so the
gateway orphans to init (avoiding an unreaped zombie under the synchronous,
event-loop-blocked release call). POSIX-gated via it.skipIf and written
branch-free to satisfy the changed-test conditionals budget.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@yimoj

yimoj commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Runtime validation added (head 7d29390a1)

Following up on the recurring runtime-validation ask (GPT-5.5 PRA-2 / Nemotron PRA-9 and the PRA-T* runtime test follow-ups): I've now committed an automated runtime test rather than relying only on the local E2E transcript — test/tunnel-gateway-port-release-runtime.test.ts (runs in the green integration Vitest project via cli-test-shards).

It does exactly what the advisor specified, with no mocks:

  • launches a real process whose argv0 basename is openshell-gateway (the identity the host stopper cmdline-gates on), bound to an isolated non-default port (reserved via :0) with an isolated HOME/state dir — so it never touches a real user gateway;
  • runs the real releaseManagedGatewayPort (real spawnSync/ps/kill/stopHostGatewayProcesses, only the registry lookup + HOME are stubbed/isolated);
  • asserts the recorded gateway pid is stopped, the port is released, and a fresh process can immediately rebind the freed port — the exact [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 macOS failure mode.

Implementation notes: the fake gateway is launched through a short-lived launcher that exits, so it orphans to init (avoiding an unreaped zombie under the synchronous release call); it is POSIX-gated with it.skipIf and written branch-free to satisfy the changed-test conditionals budget. This supersedes the "existing maintained runtime coverage" portion of my earlier justification.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: onboard-resume, onboard-repair, concurrent-gateway-ports, ubuntu-repo-docker-post-reboot-recovery
Optional E2E targets: None

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=concurrent-gateway-ports
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-docker-post-reboot-recovery

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • onboard-resume: Changes touch central onboarding orchestration and Docker-driver gateway prelaunch/cutover/runtime behavior. The onboarding resume rule requires the live onboard-resume job for resume paths that can be affected by gateway launch and persisted onboarding state.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume
  • onboard-repair: The same onboarding and Docker-driver gateway changes can affect repair/backstop execution from persisted sessions, so repair coverage is required alongside resume rather than optional.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair
  • concurrent-gateway-ports: Gateway port listener, release, ownership, stop, and multi-gateway service changes should be exercised by the focused concurrent gateway ports live job.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=concurrent-gateway-ports
  • ubuntu-repo-docker-post-reboot-recovery: Docker-driver gateway prelaunch, cutover, runtime identity, host gateway process, and status/recovery behavior changes map directly to the live-supported post-reboot recovery target.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-docker-post-reboot-recovery

Optional E2E targets

  • None.

Relevant changed files

  • src/commands/stop.ts
  • src/lib/onboard.ts
  • src/lib/onboard/docker-driver-gateway-cutover.ts
  • src/lib/onboard/docker-driver-gateway-port-listener.ts
  • src/lib/onboard/docker-driver-gateway-prelaunch.ts
  • src/lib/onboard/docker-driver-gateway-runtime.ts
  • src/lib/onboard/host-gateway-process.ts
  • src/lib/tunnel/gateway-port-confirmation.ts
  • src/lib/tunnel/gateway-port-listeners.ts
  • src/lib/tunnel/gateway-port-release.ts
  • src/lib/tunnel/gateway-port-resolution.ts
  • src/lib/tunnel/gateway-stop.ts
  • src/lib/tunnel/service-command.ts
  • src/lib/tunnel/services.ts

resolveStopGatewayPort() ignored an out-of-range explicit `port` override
(via `isValidPort` being false) and silently fell through to the sandbox
binding / default `GATEWAY_PORT`. An explicitly provided but invalid port is
a caller error, so distinguish it from an absent override and fail closed
(return null → releaseManagedGatewayPort skips/warns), consistent with the
fail-closed handling already used for invalid registry state. Addresses the
PR Review Advisor PRA-1 correctness finding.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@yimoj

yimoj commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

PRA-1 (invalid explicit port override) fixed (head 8258720dd)

Confirmed and fixed. resolveStopGatewayPort() previously let an out-of-range explicit port override fall through isValidPort() to the sandbox binding / default GATEWAY_PORT. It now distinguishes an absent override from a present-but-invalid one and fails closed (nullreleaseManagedGatewayPort skips and warns), exactly as it already does for invalid registry state — the advisor's suggested shape. New unit cases assert { port: 70000 } and { port: 0, sandboxName } both resolve to null.

Acceptance-clause test follow-ups (PRA-T1–T5): now covered by the committed runtime test added in 7d29390a1 (test/tunnel-gateway-port-release-runtime.test.ts), which proves the issue's acceptance criteria directly — after the real release path runs, the gateway port is released and a fresh process rebinds it immediately (the #5968 "port not released / squatter cannot rebind" symptom).

All real CI lanes are green (growth-guardrails, static-checks, cli-tests incl. the new runtime test, macos-e2e, self-hosted gateway-isolation/port-overrides E2E, CodeQL, DCO).

@yimoj

yimoj commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Disposition of remaining advisor items (head 8258720dd)

All real CI lanes are green across the last three pushes. Every genuinely actionable advisor finding has been fixed in code this round; the remaining items are scope/justify warnings, now addressed as follows.

  • PRA-2 (Required, acceptance — "duplicate-gateway not fixed at the creation source"). Taken the advisor's sanctioned option: narrowed the scope in the PR description (new "Scope and acceptance" section). This PR fixes the blocking [macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop #5968 symptom (stop releases the port; squatter rebinds — proven by the new runtime test) and reaps duplicate/orphan host gateways at stop. Enforcing "exactly one instance" at gateway creation is a property of the onboard/start gateway-launch/reuse path — a separate sensitive lifecycle (issue Fix-Plan item 6) — deliberately out of scope for this stop-teardown change. Maintainers can convert the keyword to Refs #5968 if they want the issue held open for the creation-source singleton.
  • PRA-1 (source-of-truth — duplicate/orphan host gateway listeners). Invalid state = a corrupt/missing/tampered gateway binding (or out-of-range override) at stop time; source boundary = the registry write path (onboard/gateway registration) which should validate the port at write time; this PR is the defensive read-time fail-closed mirror, now consistent across all untrusted-input cases (registry throw, invalid binding, absent named entry, invalid explicit override); regression tests cover each; removal condition = relax once write-time validation guarantees a valid binding. It surfaces a warn and skips rather than hiding state.
  • PRA-T1PRA-T8 (runtime validation / acceptance clauses). Covered by the committed test/tunnel-gateway-port-release-runtime.test.ts (real process, real release path, real rebind) for the port-release acceptance criteria. The acceptance clauses tied to the creation-source singleton are scoped out per PRA-2 above.

This is a partial-vs-full-fix scope judgment that only a human maintainer can finalize; the advisor is explicitly non-binding and defers the merge decision. No further code changes are planned unless a maintainer requests the creation-source lifecycle work as part of this PR.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/macos-e2e.yaml (1)

73-78: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider a step-level timeout for the real-process integration step.

This step runs test/tunnel-gateway-port-release-runtime.test.ts, which spawns a real openshell-gateway-like process and probes port release via lsof/polling. If cleanup ever hangs (e.g., zombie process not reaped), this step could consume the full 30-minute job timeout, delaying failure signal for the rest of the job. A tighter timeout-minutes on just this step would fail fast.

💡 Optional: add a step timeout
       - name: Run gateway lifecycle regressions
+        timeout-minutes: 10
         run: >-
           npx vitest run --project integration
           test/tunnel-gateway-port-release-runtime.test.ts
           test/onboard-gateway-prelaunch-cutover.test.ts
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/macos-e2e.yaml around lines 73 - 78, The real-process
integration step in the macOS workflow can hang and consume the full job
timeout, so add a step-level timeout to the “Run gateway lifecycle regressions”
step that runs the integration vitest command. Update the workflow step that
executes test/tunnel-gateway-port-release-runtime.test.ts and
test/onboard-gateway-prelaunch-cutover.test.ts so it fails fast if cleanup or
process reaping stalls, while leaving the rest of the job unaffected.
🤖 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 @.github/workflows/macos-e2e.yaml:
- Around line 73-78: The real-process integration step in the macOS workflow can
hang and consume the full job timeout, so add a step-level timeout to the “Run
gateway lifecycle regressions” step that runs the integration vitest command.
Update the workflow step that executes
test/tunnel-gateway-port-release-runtime.test.ts and
test/onboard-gateway-prelaunch-cutover.test.ts so it fails fast if cleanup or
process reaping stalls, while leaving the rest of the job unaffected.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: dc2f46c7-143c-4384-915c-7a9ac00604ff

📥 Commits

Reviewing files that changed from the base of the PR and between 5719408 and d737769.

📒 Files selected for processing (4)
  • .github/workflows/macos-e2e.yaml
  • ci/platform-matrix.json
  • docs/inference/inference-options.mdx
  • docs/reference/platform-support.mdx
✅ Files skipped from review due to trivial changes (1)
  • ci/platform-matrix.json

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ❌ Some jobs failed

Run: 28681019834
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,concurrent-gateway-ports,tunnel-lifecycle
Summary: 2 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
concurrent-gateway-ports ✅ success
tunnel-lifecycle ❌ failure

Failed jobs: tunnel-lifecycle. Check run artifacts for logs.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ⚠️ Run cancelled — no signal

Run: 28681570067
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: (selector rejected by workflow validation)
Summary: 0 passed, 0 failed, 71 cancelled, 0 skipped

Job Result
agent-turn-latency ⚠️ cancelled
bedrock-runtime-compatible-anthropic ⚠️ cancelled
brave-search ⚠️ cancelled
channels-add-remove ⚠️ cancelled
channels-stop-start ⚠️ cancelled
cloud-inference ⚠️ cancelled
cloud-onboard ⚠️ cancelled
common-egress-agent ⚠️ cancelled
concurrent-gateway-ports ⚠️ cancelled
credential-migration ⚠️ cancelled
credential-sanitization ⚠️ cancelled
cron-preflight-inference-local ⚠️ cancelled
device-auth-health ⚠️ cancelled
diagnostics ⚠️ cancelled
docs-validation ⚠️ cancelled
double-onboard ⚠️ cancelled
full-e2e ⚠️ cancelled
gateway-drift-preflight ⚠️ cancelled
gateway-guard-recovery ⚠️ cancelled
gateway-health-honest ⚠️ cancelled
gpu-double-onboard ⚠️ cancelled
gpu-e2e ⚠️ cancelled
hermes-dashboard ⚠️ cancelled
hermes-discord ⚠️ cancelled
hermes-e2e ⚠️ cancelled
hermes-gpu-startup ⚠️ cancelled
hermes-inference-switch ⚠️ cancelled
hermes-slack ⚠️ cancelled
inference-routing ⚠️ cancelled
issue-2478-crash-loop-recovery ⚠️ cancelled
issue-4434-tui-unreachable-inference ⚠️ cancelled
issue-4462-scope-upgrade-approval ⚠️ cancelled
jetson-nvmap-gpu ⚠️ cancelled
kimi-inference-compat ⚠️ cancelled
launchable-smoke ⚠️ cancelled
live ⚠️ cancelled
messaging-compatible-endpoint ⚠️ cancelled
messaging-providers ⚠️ cancelled
model-router-provider-routed-inference ⚠️ cancelled
network-policy ⚠️ cancelled
ollama-auth-proxy ⚠️ cancelled
onboard-negative-paths ⚠️ cancelled
onboard-repair ⚠️ cancelled
onboard-resume ⚠️ cancelled
openclaw-discord-pairing ⚠️ cancelled
openclaw-inference-switch ⚠️ cancelled
openclaw-skill-cli ⚠️ cancelled
openclaw-slack-pairing ⚠️ cancelled
openclaw-tui-chat-correlation ⚠️ cancelled
openshell-gateway-auth-contract ⚠️ cancelled
openshell-gateway-upgrade ⚠️ cancelled
openshell-version-pin ⚠️ cancelled
overlayfs-autofix ⚠️ cancelled
rebuild-hermes ⚠️ cancelled
rebuild-hermes-stale-base ⚠️ cancelled
rebuild-openclaw ⚠️ cancelled
sandbox-operations ⚠️ cancelled
sandbox-rebuild ⚠️ cancelled
sandbox-rlimits-connect ⚠️ cancelled
sandbox-survival ⚠️ cancelled
security-posture ⚠️ cancelled
sessions-agents-cli ⚠️ cancelled
shields-config ⚠️ cancelled
skill-agent ⚠️ cancelled
snapshot-commands ⚠️ cancelled
spark-install ⚠️ cancelled
state-backup-restore ⚠️ cancelled
telegram-injection ⚠️ cancelled
token-rotation ⚠️ cancelled
tunnel-lifecycle ⚠️ cancelled
upgrade-stale-sandbox ⚠️ cancelled

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ⚠️ Some jobs cancelled — partial pass

Run: 28683405046
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 2 passed, 0 failed, 5 cancelled, 0 skipped

Job Result
cloud-onboard ⚠️ cancelled
concurrent-gateway-ports ⚠️ cancelled
double-onboard ⚠️ cancelled
full-e2e ⚠️ cancelled
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ⚠️ cancelled

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/onboard-gateway-prelaunch-cutover.test.ts (1)

160-173: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Test doesn't actually exercise drift-based exclusion.

cleanupPids is always portListenerPids regardless of driftPids (see runDockerDriverGatewayCutover), so this test passes for the same reason as the preceding "never includes an unobserved pid-file process" test (line 149) — the pid-file PID (4242) is excluded because it's unobserved by the listener scan, not because of drift. The title implies drift causes exclusion, but drift status has no bearing on cleanupPids scoping in this code path.

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 `@test/onboard-gateway-prelaunch-cutover.test.ts` around lines 160 - 173, The
test in onBoard-gateway-prelaunch-cutover should not claim drift-based exclusion
when it never exercises that path. Update the "also excludes a drifted pid-file
process from port-scoped cleanup" case around makeHarness/run so it explicitly
validates the behavior driven by driftPids or rename the test to match the
actual unobserved-PID exclusion being asserted. Make sure the assertion and
setup in harness.events and cleanupPids/portListenerPids align with the behavior
under test, rather than passing for the same reason as the previous pid-file
exclusion test.

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 `@docs/reference/commands-nemohermes.mdx`:
- Line 1614: The Markdown/MDX text in the `nemohermes stop` description combines
multiple sentences on one line, violating the one-sentence-per-line guideline.
Update the affected prose in the `nemohermes stop` documentation block so each
sentence is on its own line, keeping the same wording but splitting it into
separate lines for the full stop behavior, tunnel services, and managed host
gateway port details.

---

Nitpick comments:
In `@test/onboard-gateway-prelaunch-cutover.test.ts`:
- Around line 160-173: The test in onBoard-gateway-prelaunch-cutover should not
claim drift-based exclusion when it never exercises that path. Update the "also
excludes a drifted pid-file process from port-scoped cleanup" case around
makeHarness/run so it explicitly validates the behavior driven by driftPids or
rename the test to match the actual unobserved-PID exclusion being asserted.
Make sure the assertion and setup in harness.events and
cleanupPids/portListenerPids align with the behavior under test, rather than
passing for the same reason as the previous pid-file exclusion test.
🪄 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: 9e09a135-6ec4-43ab-8698-c4a2cf167db2

📥 Commits

Reviewing files that changed from the base of the PR and between d737769 and 826271f.

📒 Files selected for processing (25)
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/commands/simple-global-oclif-adapters.test.ts
  • src/commands/stop.ts
  • src/lib/onboard.ts
  • src/lib/onboard/docker-driver-gateway-cutover.ts
  • src/lib/onboard/docker-driver-gateway-port-listener.ts
  • src/lib/onboard/docker-driver-gateway-prelaunch.test.ts
  • src/lib/onboard/docker-driver-gateway-prelaunch.ts
  • src/lib/onboard/docker-driver-gateway-runtime.test.ts
  • src/lib/onboard/docker-driver-gateway-runtime.ts
  • src/lib/tunnel/gateway-port-confirmation.ts
  • src/lib/tunnel/gateway-port-listeners.ts
  • src/lib/tunnel/gateway-port-release-fail-closed.test.ts
  • src/lib/tunnel/gateway-port-release-lifecycle.test.ts
  • src/lib/tunnel/gateway-port-release-test-helpers.ts
  • src/lib/tunnel/gateway-port-release.ts
  • src/lib/tunnel/gateway-port-resolution.ts
  • src/lib/tunnel/service-command.test.ts
  • src/lib/tunnel/service-command.ts
  • src/lib/tunnel/services-gateway-ownership.test.ts
  • src/lib/tunnel/services.ts
  • test/cli/tunnel-command.test.ts
  • test/onboard-gateway-prelaunch-cutover.test.ts
  • test/tunnel-gateway-port-release-runtime.test.ts
✅ Files skipped from review due to trivial changes (2)
  • src/commands/simple-global-oclif-adapters.test.ts
  • docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/onboard/docker-driver-gateway-prelaunch.test.ts
  • test/tunnel-gateway-port-release-runtime.test.ts

Comment thread docs/reference/commands-nemohermes.mdx Outdated
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ⚠️ Some jobs cancelled — partial pass

Run: 28683481077
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 6 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
concurrent-gateway-ports ✅ success
double-onboard ⚠️ cancelled
full-e2e ✅ success
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ✅ success

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ⚠️ Some jobs cancelled — partial pass

Run: 28683889853
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 5 passed, 0 failed, 2 cancelled, 0 skipped

Job Result
cloud-onboard ⚠️ cancelled
concurrent-gateway-ports ✅ success
double-onboard ⚠️ cancelled
full-e2e ✅ success
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ✅ success

ericksoa added 2 commits July 3, 2026 14:45
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ⚠️ Some jobs cancelled — partial pass

Run: 28684349737
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 6 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
concurrent-gateway-ports ✅ success
double-onboard ⚠️ cancelled
full-e2e ✅ success
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ✅ success

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ⚠️ Some jobs cancelled — partial pass

Run: 28684756831
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 2 passed, 0 failed, 5 cancelled, 0 skipped

Job Result
cloud-onboard ⚠️ cancelled
concurrent-gateway-ports ⚠️ cancelled
double-onboard ⚠️ cancelled
full-e2e ⚠️ cancelled
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ⚠️ cancelled

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ⚠️ Some jobs cancelled — partial pass

Run: 28685245581
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 5 passed, 0 failed, 2 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
concurrent-gateway-ports ⚠️ cancelled
double-onboard ⚠️ cancelled
full-e2e ✅ success
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ✅ success

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ✅ All requested jobs passed

Run: 28685614903
Workflow ref: fix/5968-stop-releases-gateway-port
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,full-e2e,concurrent-gateway-ports,tunnel-lifecycle,double-onboard,gateway-health-honest,gateway-drift-preflight
Summary: 7 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
concurrent-gateway-ports ✅ success
double-onboard ✅ success
full-e2e ✅ success
gateway-drift-preflight ✅ success
gateway-health-honest ✅ success
tunnel-lifecycle ✅ success

@ericksoa ericksoa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at exact head 7281e94.

Verified 7/7 exact-head E2E scenarios, 41 passing PR checks with no failures or pending checks, 29/29 verified commits, zero unresolved review threads, and a clean conflict/overlap audit against current main.

Maintainer rationale for the non-binding Nemotron findings: the single 2-second-capped child bind probe is required by the synchronous stop API because Node reports in-process bind success/failure asynchronously; it runs once only after listener scans clear and fails closed. The global atomic onboard lock is port-independent, is held across gateway creation, and is proven across processes; the post-reap bind gate separately covers non-participating processes.

@ericksoa
ericksoa merged commit 8aeb719 into main Jul 3, 2026
120 of 123 checks passed
@ericksoa
ericksoa deleted the fix/5968-stop-releases-gateway-port branch July 3, 2026 22:32
@ericksoa ericksoa mentioned this pull request Jul 4, 2026
21 tasks
ericksoa added a commit that referenced this pull request Jul 4, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [#6020](#6020) and
[#5876](#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [#6251](#6251) and
[#5989](#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [#6232](#6232),
[#6082](#6082),
[#6219](#6219),
[#6214](#6214),
[#6215](#6215),
[#6230](#6230), and
[#6260](#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [#6166](#6166),
[#6254](#6254),
[#6265](#6265),
[#6164](#6164), and
[#6017](#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [#6150](#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [#6234](#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [#6129](#6129),
[#5987](#5987),
[#5955](#5955), and
[#6220](#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [#5963](#5963),
[#6050](#6050),
[#6094](#6094),
[#6238](#6238),
[#5988](#5988),
[#6235](#6235),
[#6181](#6181), and
[#5986](#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [#6200](#6200),
[#6248](#6248),
[#6168](#6168),
[#6270](#6270), and
[#5649](#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages and routes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…DIA#5988)

Release the managed host gateway port for the selected sandbox without disrupting registered peers that share the gateway. Make listener discovery, release confirmation, and replacement cutover fail closed, and cover the lifecycle with exact-head unit, integration, macOS, and live E2E validation.

Fixes NVIDIA#5968

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [NVIDIA#6020](NVIDIA#6020) and
[NVIDIA#5876](NVIDIA#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [NVIDIA#6251](NVIDIA#6251) and
[NVIDIA#5989](NVIDIA#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [NVIDIA#6232](NVIDIA#6232),
[NVIDIA#6082](NVIDIA#6082),
[NVIDIA#6219](NVIDIA#6219),
[NVIDIA#6214](NVIDIA#6214),
[NVIDIA#6215](NVIDIA#6215),
[NVIDIA#6230](NVIDIA#6230), and
[NVIDIA#6260](NVIDIA#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [NVIDIA#6166](NVIDIA#6166),
[NVIDIA#6254](NVIDIA#6254),
[NVIDIA#6265](NVIDIA#6265),
[NVIDIA#6164](NVIDIA#6164), and
[NVIDIA#6017](NVIDIA#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [NVIDIA#6150](NVIDIA#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [NVIDIA#6234](NVIDIA#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [NVIDIA#6129](NVIDIA#6129),
[NVIDIA#5987](NVIDIA#5987),
[NVIDIA#5955](NVIDIA#5955), and
[NVIDIA#6220](NVIDIA#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [NVIDIA#5963](NVIDIA#5963),
[NVIDIA#6050](NVIDIA#6050),
[NVIDIA#6094](NVIDIA#6094),
[NVIDIA#6238](NVIDIA#6238),
[NVIDIA#5988](NVIDIA#5988),
[NVIDIA#6235](NVIDIA#6235),
[NVIDIA#6181](NVIDIA#6181), and
[NVIDIA#5986](NVIDIA#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [NVIDIA#6200](NVIDIA#6200),
[NVIDIA#6248](NVIDIA#6248),
[NVIDIA#6168](NVIDIA#6168),
[NVIDIA#6270](NVIDIA#6270), and
[NVIDIA#5649](NVIDIA#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages and routes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: networking DNS, proxy, TLS, ports, host aliases, or connectivity area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[macOS][Sandbox] gateway does not release port 8080 promptly after nemoclaw stop

4 participants