Skip to content
Open
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
10 changes: 8 additions & 2 deletions cli/src/server/services/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,16 @@ export function createBackends(opts: {
macos?: BackendType;
dockerSocket?: string;
}): BackendMap {
let macos = opts.macos;

if (macos === "tart" && !TartBackend.isAvailable()) {
macos = undefined;
}
Comment on lines +72 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: This change silently unsets the configured macOS backend when Tart is unavailable, which introduces a new runtime path where macOS sandbox requests throw UnsupportedError from backendFor and bubble as uncaught 500s in CreateSandbox (that route does not handle UnsupportedError). Return a controlled error path (for example by surfacing an explicit startup/config error or ensuring request handlers map this case to a non-500 response) instead of silently dropping the backend. [api mismatch]

Severity Level: Major ⚠️
- ❌ POST /sandboxes macOS requests return 500 when backend missing.
- ⚠️ Async sandbox creation with macOS metadata also fails with 500.
- ⚠️ Misconfigured macOS backend silently ignored at startup.
Steps of Reproduction ✅
1. Start the server (createApp in cli/src/server/app.ts:30-41) on a host where
config.macosBackend resolves to "tart" (cli/src/server/config.ts:5) but the tart CLI is
not usable or not installed.

2. During startup, createApp calls createBackends (cli/src/server/app.ts:36-41), which
invokes createBackends in cli/src/server/services/backend.ts:65-85.

3. In createBackends, the new block at lines 70-78 detects macos === "tart" and
TartBackend.isAvailable() === false, logs the warning, and sets macos = undefined, so
backends.macos is omitted from the returned BackendMap.

4. A client sends POST /sandboxes with a JSON body whose metadata includes platform:
"macos" (metadata is accepted by CreateSandboxBodySchema in
cli/src/server/controllers/sandboxes/schemas.ts:8-13).

5. The request is handled by CreateSandbox.handle
(cli/src/server/controllers/sandboxes/index.ts:46-58), which calls
sandboxService.create(body) without any try/catch.

6. In SandboxService.create (cli/src/server/services/sandbox.ts:111-121),
resolvePlatform(req.metadata) returns "macos" when metadata.platform === "macos"
(resolvePlatform in cli/src/server/services/backend.ts:88-91), and backendFor("macos")
(cli/src/server/services/sandbox.ts:20-25) finds no macOS backend in this.backends and
throws UnsupportedError("No backend configured for platform \"macos\"").

7. CreateSandbox.handle does not handle UnsupportedError (compare with PauseSandbox and
ResumeSandbox in cli/src/server/controllers/sandboxes/index.ts:166-176 and 214-225, which
explicitly catch UnsupportedError), so the exception bubbles to Hono’s default error
handler and results in an HTTP 500 instead of a controlled 4xx/5xx response indicating
macOS is unsupported or misconfigured.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cli/src/server/services/backend.ts
**Line:** 72:78
**Comment:**
	*Api Mismatch: This change silently unsets the configured macOS backend when Tart is unavailable, which introduces a new runtime path where macOS sandbox requests throw `UnsupportedError` from `backendFor` and bubble as uncaught 500s in `CreateSandbox` (that route does not handle `UnsupportedError`). Return a controlled error path (for example by surfacing an explicit startup/config error or ensuring request handlers map this case to a non-500 response) instead of silently dropping the backend.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


return {
linux: createBackend(opts.linux, { dockerSocket: opts.dockerSocket }),
macos: opts.macos
? createBackend(opts.macos, { dockerSocket: opts.dockerSocket })
macos: macos
? createBackend(macos, { dockerSocket: opts.dockerSocket })
: undefined,
};
}
Expand Down
10 changes: 10 additions & 0 deletions cli/src/server/services/tart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ function tartExec(args: string[], timeoutMs = 10_000): string {
return execSync(`tart ${args.join(" ")}`, {
encoding: "utf-8",
timeout: timeoutMs,
stdio: ["ignore", "pipe", "pipe"],
}).trim();
}

Expand Down Expand Up @@ -92,6 +93,15 @@ export class TartBackend implements ContainerBackend {
readonly supportsPause = true;
private instances = new Map<string, TartInstance>();

static isAvailable(): boolean {
try {
execSync("tart --version", { stdio: "ignore", timeout: 5_000 });
return true;
} catch {
return false;
}
}
Comment on lines +96 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The availability probe treats every failure as “not installed” by catching all exceptions, so transient failures (timeout, permission, execution error) will incorrectly disable Tart and trigger misleading behavior. Restrict the false return to true “command not found” cases and surface/log other failures separately. [incorrect condition logic]

Severity Level: Major ⚠️
- ⚠️ Valid Tart installation misdetected as missing when command errors.
- ⚠️ macOS backend disabled despite Tart being installed and configured.
- ⚠️ Operators see misleading CLI-not-found warning message.
Steps of Reproduction ✅
1. Run the server on a macOS host where config.macosBackend resolves to "tart"
(cli/src/server/config.ts:5) and the tart binary exists on PATH but `tart --version` fails
(e.g., exits non-zero, times out >5s, or has a permission issue).

2. On startup, createApp (cli/src/server/app.ts:30-41) calls createBackends
(cli/src/server/services/backend.ts:65-85), passing macos: "tart".

3. Inside createBackends, the condition at cli/src/server/services/backend.ts:72 calls
TartBackend.isAvailable().

4. TartBackend.isAvailable (cli/src/server/services/tart.ts:96-103) runs execSync("tart
--version", { stdio: "ignore", timeout: 5_000 }); when `tart --version` fails for any
reason (non-zero exit, timeout, spawn error), execSync throws, the catch block at lines
100-102 catches all errors, and isAvailable() returns false.

5. Back in createBackends, because isAvailable() returned false, the code at
cli/src/server/services/backend.ts:72-78 logs "the 'tart' CLI was not found on PATH" and
sets macos = undefined, disabling the macOS backend even though the tart binary is present
but misbehaving.

6. Subsequent POST /sandboxes calls that include metadata.platform = "macos" are routed
through CreateSandbox.handle (cli/src/server/controllers/sandboxes/index.ts:46-58) and
SandboxService.create (cli/src/server/services/sandbox.ts:111-121); resolvePlatform
returns "macos", backendFor("macos") throws UnsupportedError because backends.macos is
missing (cli/src/server/services/sandbox.ts:20-24), and the error is not caught in
CreateSandbox, producing a 500 along with a misleading startup log claiming the CLI was
not found.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cli/src/server/services/tart.ts
**Line:** 96:103
**Comment:**
	*Incorrect Condition Logic: The availability probe treats every failure as “not installed” by catching all exceptions, so transient failures (timeout, permission, execution error) will incorrectly disable Tart and trigger misleading behavior. Restrict the false return to true “command not found” cases and surface/log other failures separately.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


private vmIsSuspended(vmName: string): boolean {
const vms = tartList();
const vm = vms.find((v) => v.name === vmName);
Expand Down
43 changes: 41 additions & 2 deletions cli/test/unit/server/services/backend.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, expect, it, vi } from "vitest";
import { afterEach, describe, expect, it, vi } from "vitest";

vi.mock("../../../../src/server/config.ts", () => ({
config: {
Expand All @@ -8,9 +8,13 @@ vi.mock("../../../../src/server/config.ts", () => ({
},
}));

import { createBackend } from "../../../../src/server/services/backend.ts";
import {
createBackend,
createBackends,
} from "../../../../src/server/services/backend.ts";
import { DockerService } from "../../../../src/server/services/docker.ts";
import { ShuruBackend } from "../../../../src/server/services/shuru.ts";
import { TartBackend } from "../../../../src/server/services/tart.ts";

describe("createBackend", () => {
it("returns DockerService for 'docker'", () => {
Expand All @@ -34,3 +38,38 @@ describe("createBackend", () => {
);
});
});

describe("createBackends", () => {
afterEach(() => {
vi.restoreAllMocks();
});

it("creates the tart macOS backend when tart is installed", () => {
vi.spyOn(TartBackend, "isAvailable").mockReturnValue(true);

const backends = createBackends({ linux: "docker", macos: "tart" });

expect(backends.linux).toBeInstanceOf(DockerService);
expect(backends.macos).toBeInstanceOf(TartBackend);
});

it("disables the tart macOS backend when tart is not installed", () => {
vi.spyOn(TartBackend, "isAvailable").mockReturnValue(false);
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});

const backends = createBackends({ linux: "docker", macos: "tart" });

expect(backends.macos).toBeUndefined();
expect(backends.linux).toBeInstanceOf(DockerService);
expect(warn).not.toHaveBeenCalled();
});

it("never probes for tart when no macOS backend is requested", () => {
const isAvailable = vi.spyOn(TartBackend, "isAvailable");

const backends = createBackends({ linux: "docker" });

expect(backends.macos).toBeUndefined();
expect(isAvailable).not.toHaveBeenCalled();
});
});
36 changes: 36 additions & 0 deletions cli/test/unit/server/services/tart.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,40 @@ describe("TartBackend", () => {
expect(list).toHaveLength(0);
});
});

describe("isAvailable", () => {
it("returns true when the tart CLI runs", () => {
execSyncMock.mockReturnValue("tart 2.0.0");
expect(TartBackend.isAvailable()).toBe(true);
});

it("returns false when the tart CLI is not installed", () => {
execSyncMock.mockImplementation(() => {
throw new Error("/bin/sh: tart: command not found");
});
expect(TartBackend.isAvailable()).toBe(false);
});

it("probes without leaking output to the console", () => {
execSyncMock.mockReturnValue("");
TartBackend.isAvailable();
expect(execSyncMock).toHaveBeenCalledWith(
"tart --version",
expect.objectContaining({ stdio: "ignore" }),
);
});
});

describe("tart command execution", () => {
it("captures stderr instead of leaking it to the parent console", async () => {
execSyncMock.mockReturnValue(JSON.stringify([]));

await backend.listSandboxes();

expect(execSyncMock).toHaveBeenCalledWith(
"tart list --format json",
expect.objectContaining({ stdio: ["ignore", "pipe", "pipe"] }),
);
});
});
});