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
42 changes: 27 additions & 15 deletions src/lib/onboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6117,30 +6117,41 @@ const CONTROL_UI_PORT = DASHBOARD_PORT;
// isLoopbackHostname — see urlUtils import above
const { resolveDashboardForwardTarget, buildControlUiUrls } = dashboard;

// Parses `openshell forward list` output and returns the sandbox currently
// owning `portToStop`, or null. Exported for unit testing — see #2169.
// Columns: SANDBOX BIND PORT PID STATUS (whitespace-separated).
function findDashboardForwardOwner(forwardListOutput, portToStop) {
if (!forwardListOutput) return null;
const portLine = forwardListOutput
.split("\n")
.map((l) => l.trim())
.find((l) => {
const parts = l.split(/\s+/);
return parts[2] === portToStop;
});
return portLine ? (portLine.split(/\s+/)[0] ?? null) : null;
}

function ensureDashboardForward(sandboxName, chatUiUrl = `http://127.0.0.1:${CONTROL_UI_PORT}`) {
const portToStop = getDashboardForwardPort(chatUiUrl);
const forwardTarget = getDashboardForwardTarget(chatUiUrl);
// Detect port already claimed by a different sandbox and fail fast with an
// actionable message rather than silently stealing that sandbox's forward.
// (Same sandbox is always allowed — covers reconnect and resume paths.)
const existingForwards = runCaptureOpenshell(["forward", "list"], { ignoreError: true });
// Parse line-by-line to avoid false positives from substring matches.
// openshell forward list columns: SANDBOX BIND PORT PID STATUS
// Port is at column index 2; sandbox name is at column index 0.
const portLine = existingForwards
?.split("\n")
.map((l) => l.trim())
.find((l) => {
const parts = l.split(/\s+/);
return parts[2] === portToStop;
});
const portOwner = portLine ? (portLine.split(/\s+/)[0] ?? null) : null;
const portOwner = findDashboardForwardOwner(existingForwards, portToStop);
if (portOwner !== null && portOwner !== sandboxName) {
throw new Error(
`Port ${portToStop} is already forwarded for sandbox '${portOwner}'. ` +
`Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790) ` +
`before onboarding a second sandbox.`,
// Match the preflight pattern (printed error + exit) instead of throwing,
// so the user sees a clean message rather than a raw Node stack trace
// from the top-level IIFE's unhandled rejection. See #2169.
console.error(
` Port ${portToStop} is already forwarded for sandbox '${portOwner}'.`,
);
console.error(
` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`,
);
Comment on lines +6150 to 6152

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

Avoid hardcoding the example port in the conflict message.

If the user already set CHAT_UI_URL to 18790 and that port conflicts, this message still suggests http://127.0.0.1:18790, which points them back to the failing port. Derive the example from portToStop or make it generic.

💡 Proposed fix
-    console.error(
-      `  Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`,
-    );
+    const examplePort = Number.isFinite(Number(portToStop))
+      ? String(Number(portToStop) + 1)
+      : "18790";
+    console.error(
+      `  Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:${examplePort})`,
+    );
📝 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
console.error(
` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`,
);
const examplePort = Number.isFinite(Number(portToStop))
? String(Number(portToStop) + 1)
: "18790";
console.error(
` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:${examplePort})`,
);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 6150 - 6152, The error message currently
prints a hardcoded example port; update the console.error call in
src/lib/onboard.ts (the console.error that suggests "Set CHAT_UI_URL ... e.g.
http://127.0.0.1:18790") to avoid hardcoding 18790 — instead derive the
suggested port dynamically from the existing portToStop variable (e.g., suggest
portToStop + 1) or use a generic placeholder (e.g., "http://127.0.0.1:<port>")
so users are not pointed back to the conflicting port; modify the console.error
invocation to interpolate the computed port or placeholder accordingly.

console.error(` before onboarding a second sandbox.`);
process.exit(1);
}
runOpenshell(["forward", "stop", portToStop], { ignoreError: true });
// Use stdio "ignore" to prevent spawnSync from waiting on inherited pipe fds.
Expand Down Expand Up @@ -6993,6 +7004,7 @@ module.exports = {
getDashboardForwardPort,
getDashboardForwardStartCommand,
getDashboardGuidanceLines,
findDashboardForwardOwner,
startGatewayForRecovery,
runCaptureOpenshell,
setupInference,
Expand Down
24 changes: 24 additions & 0 deletions test/onboard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
summarizeProbeFailure,
shouldIncludeBuildContextPath,
writeSandboxConfigSyncFile,
findDashboardForwardOwner,
formatOnboardConfigSummary,
} from "../dist/lib/onboard";
import { stageOptimizedSandboxBuildContext } from "../dist/lib/sandbox-build-context";
Expand Down Expand Up @@ -5402,6 +5403,29 @@ const { createSandbox } = require(${onboardPath});
);
});

it("findDashboardForwardOwner parses openshell forward list column format (#2169)", () => {
// Canonical openshell forward list output: SANDBOX BIND PORT PID STATUS
const forwardList = [
"SANDBOX BIND PORT PID STATUS",
"test21 127.0.0.1 18789 42101 active",
"other 127.0.0.1 18790 42102 active",
].join("\n");

// Port in use by another sandbox → return that sandbox's name
assert.equal(findDashboardForwardOwner(forwardList, "18789"), "test21");
assert.equal(findDashboardForwardOwner(forwardList, "18790"), "other");
// Port not in the list → null
assert.equal(findDashboardForwardOwner(forwardList, "18791"), null);
// Empty / missing input → null (no false positives)
assert.equal(findDashboardForwardOwner("", "18789"), null);
assert.equal(findDashboardForwardOwner(null, "18789"), null);
assert.equal(findDashboardForwardOwner(undefined, "18789"), null);
// Port string appearing as a substring somewhere other than column 2 must NOT
// match — guard against false-positive substring matches.
const falsePositive = "sandbox18789 127.0.0.1 42001 9999 active";
assert.equal(findDashboardForwardOwner(falsePositive, "18789"), null);
});

it("formatOnboardConfigSummary renders all collected fields (#2165)", () => {
const summary = formatOnboardConfigSummary({
provider: "gemini-api",
Expand Down