test/gateway-liveness-probe.test.ts (1)

62-70: Scope the purity check to isGatewayHealthy() instead of the whole file.

These assertions will fail if any unrelated helper in gateway-state.ts ever uses docker, spawn, or exec, even when isGatewayHealthy() stays pure. Narrowing the check to that function body will make the test less noisy.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/gateway-liveness-probe.test.ts` around lines 62 - 70, The test currently
scans the entire gateway-state.ts file; instead extract only the
isGatewayHealthy() function body and run the purity assertions against that
snippet. Locate isGatewayHealthy in gateway-state.ts (handle both "function
isGatewayHealthy(...){...}" and "export const isGatewayHealthy = (...) => { ...
}" forms), capture the function body with a regex, then assert that the captured
body does not contain "docker", "spawn", or "exec" instead of scanning the whole
file.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/onboard.ts (1)

2374-2378: ⚠️ Potential issue | 🟠 Major

Use destroyGateway() in both stale-metadata cleanup branches.

These branches still destroy the gateway directly, so they skip the Docker volume cleanup in destroyGateway(). That can leave openshell-cluster-nemoclaw* volumes behind after stale-metadata recovery and make the next gateway start fail.

Proposed fix

-    runOpenshell(["gateway", "destroy", "-g", GATEWAY_NAME], { ignoreError: true });
+    destroyGateway();

Also applies to: 5925-5929

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 2374 - 2378, The stale-metadata cleanup
branches call openshell directly and skip Docker volume cleanup; replace the
direct destroys with a call to destroyGateway() so volume cleanup runs.
Specifically, in the branch that checks gatewayReuseState === "healthy" and
!verifyGatewayContainerRunning() (and the analogous stale branch further down),
remove the direct runOpenshell(["gateway","destroy",...]) and
registry.clearAll() sequence and instead call destroyGateway() after stopping
forwards so the openshell-cluster-nemoclaw* volumes are removed; keep the
runOpenshell(["forward","stop", String(DASHBOARD_PORT)], { ignoreError: true })
step but use destroyGateway() to perform the destroy+cleanup.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/onboard.ts (1)

128-135: ⚠️ Potential issue | 🟠 Major

Differentiate missing from Docker runtime failures before cleanup

Line 131/Line 134 currently treat any non-zero docker inspect as “container not running.” That also matches daemon/socket failures, which can incorrectly trigger destructive stale cleanup in Line 2374 and Line 5925.

🛠️ Suggested fix (tri-state probe)

-function verifyGatewayContainerRunning() {
+function getGatewayContainerState() {
   const containerName = `openshell-cluster-${GATEWAY_NAME}`;
   const result = run(
-    `docker inspect --type container --format '{{.State.Running}}' ${containerName} 2>/dev/null`,
+    `docker inspect --type container --format '{{.State.Running}}' ${containerName}`,
     { ignoreError: true, suppressOutput: true },
   );
-  return result.status === 0 && (result.stdout || "").trim() === "true";
+  const stdout = String(result.stdout || "").trim().toLowerCase();
+  const stderr = String(result.stderr || "").trim().toLowerCase();
+  if (result.status === 0) return stdout === "true" ? "running" : "stopped";
+  if (stderr.includes("no such object")) return "missing";
+  return "unknown";
 }

Then only run stale-metadata cleanup when state is "missing" (or explicitly "stopped" if intended), and keep "unknown" non-destructive.

#!/bin/bash
# Verify current behavior conflates all inspect failures and drives cleanup from a boolean.
rg -n --type=ts -C3 'function verifyGatewayContainerRunning|docker inspect --type container --format.*State\.Running|2>/dev/null|return result\.status === 0' src/lib/onboard.ts

# Verify both call sites perform cleanup when helper returns false.
rg -n --type=ts -C4 'gatewayReuseState === "healthy" && !verifyGatewayContainerRunning\(\)|destroyGateway\(\)|registry\.clearAll\(\)|gatewayReuseState = "missing"' src/lib/onboard.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 128 - 135, verifyGatewayContainerRunning
currently returns a boolean and treats any non-zero docker inspect as "not
running", which conflates missing containers with Docker/daemon errors; change
it into a tri-state probe (e.g., return "healthy" | "missing" | "unknown" or an
object { state: "healthy"|"missing"|"unknown", info?: string }) in
verifyGatewayContainerRunning by running the inspect command and: if status ===
0 and stdout.trim() === "true" -> "healthy"; if status !== 0 and stderr contains
Docker's "No such object" (or equivalent) -> "missing"; otherwise -> "unknown"
(non-destructive). Then update call sites that currently do gatewayReuseState
=== "healthy" && !verifyGatewayContainerRunning(...) and the cleanup code paths
that call destroyGateway()/registry.clearAll()/set gatewayReuseState = "missing"
to only perform destructive cleanup when the probe returns "missing" (or
explicitly "stopped" if that is intended), leaving "unknown" to skip destructive
actions and surface an error for investigation.

src/lib/onboard.ts (1)

2374-2380: Extract stale-gateway cleanup into one helper

The two new stale-metadata branches duplicate the same cleanup sequence. Centralizing this reduces drift risk and keeps preflight/onboard behavior consistent.

♻️ Suggested refactor

+function cleanupStaleGatewayMetadata() {
+  console.log("  Gateway metadata is stale (container not running). Cleaning up...");
+  runOpenshell(["forward", "stop", String(DASHBOARD_PORT)], { ignoreError: true });
+  destroyGateway();
+  registry.clearAll();
+  console.log("  ✓ Stale gateway metadata cleaned up");
+}
...
-  if (gatewayReuseState === "healthy" && !verifyGatewayContainerRunning()) {
-    console.log("  Gateway metadata is stale (container not running). Cleaning up...");
-    runOpenshell(["forward", "stop", String(DASHBOARD_PORT)], { ignoreError: true });
-    destroyGateway();
-    registry.clearAll();
-    gatewayReuseState = "missing";
-    console.log("  ✓ Stale gateway metadata cleaned up");
-  }
+  if (gatewayReuseState === "healthy" && !verifyGatewayContainerRunning()) {
+    cleanupStaleGatewayMetadata();
+    gatewayReuseState = "missing";
+  }

Also applies to: 5925-5932

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 2374 - 2380, Duplicate stale-gateway cleanup
logic should be extracted into a single helper to avoid drift: create a function
(e.g., cleanupStaleGateway or handleStaleGateway) that performs the sequence
currently duplicated — logging the "stale" message, calling
runOpenshell(["forward", "stop", String(DASHBOARD_PORT)], { ignoreError: true
}), calling destroyGateway(), calling registry.clearAll(), setting
gatewayReuseState = "missing", and logging the completion message — then replace
both duplicated branches (the block guarded by gatewayReuseState === "healthy"
&& !verifyGatewayContainerRunning() and the similar block at the other location)
with calls to that helper (keep existing condition checks and only invoke the
new helper).

test/gateway-liveness-probe.test.ts (1)

48-60: Add an assertion for the docker inspect => "false" path.

These checks confirm tri-state strings exist, but they don’t guard the specific regression where .State.Running is false and gets treated as "unknown". Add coverage for that path so stopped containers can’t silently bypass cleanup logic.

✅ Example assertion to add

   it("returns tri-state: running, missing, or unknown", () => {
     // The helper must distinguish container-removed from Docker-unavailable
     expect(content).toContain('return "running"');
     expect(content).toContain('return "missing"');
     expect(content).toContain('return "unknown"');
+    // Also ensure successful inspect with "false" is handled explicitly.
+    expect(content).toMatch(/running\s*===\s*"false"|trim\(\)\s*===\s*"false"/);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/gateway-liveness-probe.test.ts` around lines 48 - 60, Add a focused
assertion that verifies the probe treats a docker inspect result with
.State.Running === false as the "missing" path (not "unknown"); locate the test
block referencing content and containerState (tests named "returns tri-state:
running, missing, or unknown" and "only downgrades to 'missing' when container
is confirmed missing") and add an assertion that ensures the generated probe
string contains the conditional or return handling for the ".State.Running ===
false" case (e.g., a check for '.State.Running === false' or the code path that
maps that to containerState === "missing") so the stopped-container path is
explicitly covered.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/onboard.ts (1)

2391-2405: Extract the stale-gateway reconciliation into one helper.

The probe, warning, cleanup, and state downgrade are duplicated in both preflight() and onboard(). Keeping that flow in one place will make the next behavior change much less likely to land in only one path.

♻️ Refactor sketch

+function reconcileHealthyGatewayReuseState(gatewayReuseState) {
+  if (gatewayReuseState !== "healthy") return gatewayReuseState;
+
+  const containerState = verifyGatewayContainerRunning();
+  if (containerState === "missing") {
+    console.log("  Gateway metadata is stale (container not running). Cleaning up...");
+    runOpenshell(["forward", "stop", String(DASHBOARD_PORT)], { ignoreError: true });
+    destroyGateway();
+    registry.clearAll();
+    console.log("  ✓ Stale gateway metadata cleaned up");
+    return "missing";
+  }
+
+  if (containerState === "unknown") {
+    console.log(
+      "  Warning: could not verify gateway container state (Docker may be unavailable). Proceeding with cached health status.",
+    );
+  }
+
+  return gatewayReuseState;
+}

- let gatewayReuseState = getGatewayReuseState(gatewayStatus, gwInfo, activeGatewayInfo);
-
- if (gatewayReuseState === "healthy") {
-   const containerState = verifyGatewayContainerRunning();
-   ...
- }
+ let gatewayReuseState = reconcileHealthyGatewayReuseState(
+   getGatewayReuseState(gatewayStatus, gwInfo, activeGatewayInfo),
+ );

Also applies to: 5947-5961

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 2391 - 2405, Extract the duplicated
stale-gateway reconciliation logic into a single helper (e.g.,
reconcileStaleGateway or ensureGatewayRunning) and call it from both preflight()
and onboard(); the helper should call verifyGatewayContainerRunning(), emit the
same warning when containerState === "unknown", perform the cleanup steps
(runOpenshell(["forward","stop", String(DASHBOARD_PORT)], { ignoreError: true
}), destroyGateway(), registry.clearAll()) and downgrade gatewayReuseState to
"missing" when containerState === "missing", and return the final state if
callers need it—replace the duplicated block in both locations with a call to
that helper so behavior remains identical and centralized.