Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion biome.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
"noExcessiveCognitiveComplexity": {
"level": "error",
"options": {
"maxAllowedComplexity": 245
"maxAllowedComplexity": 244
}
}
},
Expand Down
31 changes: 19 additions & 12 deletions src/lib/actions/sandbox/rebuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,24 @@ function hookOutputsFromBuildSteps(
return { outputs };
}

function countActiveSandboxSessionsForRebuild(sandboxName: string): number {
const opsBinRebuild = resolveOpenshell();
// Source boundary: active-session detection depends on host process listing
// and the OpenShell binary being installed. A failed/unavailable detector is
// not evidence of active sessions, and rebuild's safety preflights still run
// before destructive work. Keep the prior fail-open prompt behavior here;
// remove this fallback only if session detection becomes a required, typed
// OpenShell API that can distinguish "zero sessions" from "unavailable".
if (!opsBinRebuild) return 0;

try {
const sessionResult = getActiveSandboxSessions(sandboxName, createSessionDeps(opsBinRebuild));
return sessionResult.detected ? sessionResult.sessions.length : 0;
} catch {
return 0;
}
}

async function reapplyMessagingManifestAfterOpenClawDoctor(
sandboxName: string,
plan: SandboxMessagingPlan | null,
Expand Down Expand Up @@ -347,18 +365,7 @@ export async function rebuildSandbox(
: (_msg: string, code = 1) => process.exit(code);

// Active session detection — enrich the confirmation prompt if sessions are active
let rebuildActiveSessionCount = 0;
const opsBinRebuild = resolveOpenshell();
if (opsBinRebuild) {
try {
const sessionResult = getActiveSandboxSessions(sandboxName, createSessionDeps(opsBinRebuild));
if (sessionResult.detected) {
rebuildActiveSessionCount = sessionResult.sessions.length;
}
} catch {
/* non-fatal */
}
}
const rebuildActiveSessionCount = countActiveSandboxSessionsForRebuild(sandboxName);

const sb = registry.getSandbox(sandboxName) as any;
if (!sb) {
Expand Down
92 changes: 76 additions & 16 deletions test/rebuild-credential-preflight.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ function createFixture(opts: {
messagingPlanChannels?: string[] | null;
dockerBuildExitCode?: number;
providerRegistered?: boolean;
activeSessionCount?: number | null;
}) {
const {
sandboxName = "my-assistant",
Expand All @@ -92,6 +93,7 @@ function createFixture(opts: {
messagingPlanChannels = null,
dockerBuildExitCode = 0,
providerRegistered = true,
activeSessionCount = 0,
} = opts;
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-2273-"));
tmpFixtures.push(tmpDir);
Expand Down Expand Up @@ -248,6 +250,21 @@ process.exit(0);
{ mode: 0o755 },
);

// ── Fake ps for active SSH session detection ──────────────────
const activeSessionLines = Array.from(
{ length: activeSessionCount ?? 0 },
(_, index) => `${9000 + index} ssh openshell-${sandboxName}`,
).join("\n");
fs.writeFileSync(
path.join(tmpDir, "ps"),
`#!/usr/bin/env node
if (${activeSessionCount === null ? "true" : "false"}) process.exit(1);
process.stdout.write(${JSON.stringify(activeSessionLines)} + (${JSON.stringify(activeSessionLines)} ? "\\n" : ""));
process.exit(0);
`,
{ mode: 0o755 },
);

// ── Fake Docker ───────────────────────────────────────────────
// Hermes rebuild forces a base-image build before backup/delete.
// This fixture only exercises rebuild session state, so Docker succeeds.
Expand Down Expand Up @@ -301,24 +318,24 @@ process.exit(0);
function runRebuild(
fixture: ReturnType<typeof createFixture>,
extraEnv: Record<string, string> = {},
options: { yes?: boolean; input?: string } = {},
) {
return spawnSync(
process.execPath,
[path.join(REPO_ROOT, "bin", "nemoclaw.js"), fixture.sandboxName, "rebuild", "--yes"],
{
cwd: REPO_ROOT,
encoding: "utf-8",
env: {
HOME: fixture.tmpDir,
PATH: fixture.tmpDir + ":" + NODE_BIN + ":/usr/bin:/bin",
NEMOCLAW_NON_INTERACTIVE: "1",
NEMOCLAW_NO_CONNECT_HINT: "1",
NO_COLOR: "1",
...extraEnv,
},
timeout: 30_000,
const argv = [path.join(REPO_ROOT, "bin", "nemoclaw.js"), fixture.sandboxName, "rebuild"];
if (options.yes !== false) argv.push("--yes");
return spawnSync(process.execPath, argv, {
cwd: REPO_ROOT,
encoding: "utf-8",
input: options.input,
env: {
HOME: fixture.tmpDir,
PATH: fixture.tmpDir + ":" + NODE_BIN + ":/usr/bin:/bin",
NEMOCLAW_NON_INTERACTIVE: "1",
NEMOCLAW_NO_CONNECT_HINT: "1",
NO_COLOR: "1",
...extraEnv,
},
);
timeout: 30_000,
});
}

function registryHasSandbox(fixture: ReturnType<typeof createFixture>): boolean {
Expand All @@ -334,6 +351,49 @@ function registryHasSandbox(fixture: ReturnType<typeof createFixture>): boolean

describe("Issue #2273: atomic rebuild", () => {
describe("Layer 2: preflight credential check", () => {
it("prints active SSH session warning before interactive confirmation", {
timeout: 60_000,
}, () => {
const f = createFixture({
activeSessionCount: 2,
savedCredential: {
key: "NVIDIA_INFERENCE_API_KEY",
value: "nvapi-test-key-for-rebuild",
},
});

const result = runRebuild(f, {}, { yes: false, input: "n\n" });
const output = (result.stderr || "") + (result.stdout || "");

expect(result.status).toBe(0);
expect(output).toContain("Active SSH sessions detected (2 connections)");
expect(output).toContain("terminate all active sessions with a Broken pipe error");
expect(output).toContain("Proceed? [y/N]:");
expect(output).toContain("Cancelled.");
Comment on lines +369 to +372

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert output ordering, not just presence, for warning vs confirmation prompt.

On Lines 369-372, the test checks both strings exist but does not verify the warning appears before Proceed? [y/N]:, so the intended sequencing can regress undetected.

Suggested test assertion update
       expect(output).toContain("Active SSH sessions detected (2 connections)");
       expect(output).toContain("terminate all active sessions with a Broken pipe error");
       expect(output).toContain("Proceed? [y/N]:");
+      expect(output.indexOf("Active SSH sessions detected (2 connections)")).toBeLessThan(
+        output.indexOf("Proceed? [y/N]:"),
+      );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(output).toContain("Active SSH sessions detected (2 connections)");
expect(output).toContain("terminate all active sessions with a Broken pipe error");
expect(output).toContain("Proceed? [y/N]:");
expect(output).toContain("Cancelled.");
expect(output).toContain("Active SSH sessions detected (2 connections)");
expect(output).toContain("terminate all active sessions with a Broken pipe error");
expect(output).toContain("Proceed? [y/N]:");
expect(output.indexOf("Active SSH sessions detected (2 connections)")).toBeLessThan(
output.indexOf("Proceed? [y/N]:"),
);
expect(output).toContain("Cancelled.");
🤖 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/rebuild-credential-preflight.test.ts` around lines 369 - 372, The test
assertions on lines 369-372 in test/rebuild-credential-preflight.test.ts use
multiple toContain() checks that only verify string presence but not their
ordering. To ensure the warning messages appear before the confirmation prompt
as intended, replace the individual toContain() assertions with a single
assertion that verifies the substring order—for example, check that the index of
"Active SSH sessions detected (2 connections)" and "terminate all active
sessions with a Broken pipe error" appear before the index of "Proceed? [y/N]:"
in the output string, or use a regex or index-based approach to enforce the
correct sequence.

expect(output).not.toContain("Backing up sandbox state");
});

it("omits active SSH warning when detection is unavailable", {
timeout: 60_000,
}, () => {
const f = createFixture({
activeSessionCount: null,
savedCredential: {
key: "NVIDIA_INFERENCE_API_KEY",
value: "nvapi-test-key-for-rebuild",
},
});

const result = runRebuild(f, {}, { yes: false, input: "n\n" });
const output = (result.stderr || "") + (result.stdout || "");

expect(result.status).toBe(0);
expect(output).not.toContain("Active SSH");
expect(output).toContain("Proceed? [y/N]:");
expect(output).toContain("Cancelled.");
expect(output).not.toContain("Backing up sandbox state");
});

it("aborts rebuild BEFORE destroying sandbox when credential is missing", {
timeout: 60_000,
}, () => {
Expand Down