From 0c43ed42cfb2dde7c0c41a377ae460b3bf1bceea Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 2 Jul 2026 20:57:25 -0700 Subject: [PATCH] fix: address PR review feedback + typecheck failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - DesktopLaunchGate: the chooser onPick("local") path still called the removed applyServerBaseUrl helper (broke Typecheck and the desktop local chooser flow) — call navigateToLocalRuntimeOrigin. Also re-read shell.getState() immediately before the self-healing start check so a stale mount snapshot doesn't fire a redundant setDesktopMode("local") on normal boots (Greptile). - register-fn-binary-routes: on Windows the npm install runs under a shell, so a timeout's child.kill only stopped cmd.exe and left npm.cmd/node running — kill the whole process tree via taskkill /T on win32 (CodeRabbit). - local-server.test: align with the new resolver contract — assert the runtime never auto-registers the root project and starts engine-less when no projects exist (CodeRabbit). Co-Authored-By: Claude Opus 4.8 --- .../app/components/DesktopLaunchGate.tsx | 10 +++++-- .../src/routes/register-fn-binary-routes.ts | 15 +++++++++- .../src/__tests__/local-server.test.ts | 29 +++++++++++-------- 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/packages/dashboard/app/components/DesktopLaunchGate.tsx b/packages/dashboard/app/components/DesktopLaunchGate.tsx index 5d4f6d8d14..fc937a8b5d 100644 --- a/packages/dashboard/app/components/DesktopLaunchGate.tsx +++ b/packages/dashboard/app/components/DesktopLaunchGate.tsx @@ -123,8 +123,14 @@ export function DesktopLaunchGate({ children }: PropsWithChildren) { * running or starting, actively (re)start it via setDesktopMode("local") — idempotent * and awaits startup — before polling, so the gate can never wait for a runtime nobody * launched. + * + * Re-read the runtime state immediately before deciding: the `state` snapshot was captured + * at mount and may not yet reflect a runtime main already started, which would fire a + * redundant setDesktopMode("local") on normal boots. */ - const rt = state.localRuntime; + const freshState = await shell.getState(); + if (cancelled) return; + const rt = freshState.localRuntime; if (rt?.state !== "running" && rt?.state !== "starting") { try { await shell.setDesktopMode("local"); @@ -225,7 +231,7 @@ export function DesktopLaunchGate({ children }: PropsWithChildren) { await shell.setDesktopMode(mode); if (mode === "local") { const { baseUrl } = await waitForLocalRuntime(shell); - applyServerBaseUrl(baseUrl); + navigateToLocalRuntimeOrigin(baseUrl); return; } await shell.openConnectionManager(); diff --git a/packages/dashboard/src/routes/register-fn-binary-routes.ts b/packages/dashboard/src/routes/register-fn-binary-routes.ts index 17980a0e1b..7e5e1a8308 100644 --- a/packages/dashboard/src/routes/register-fn-binary-routes.ts +++ b/packages/dashboard/src/routes/register-fn-binary-routes.ts @@ -86,7 +86,20 @@ function runNpmInstall(): Promise { }); const timer = setTimeout(() => { timedOut = true; - try { child.kill("SIGKILL"); } catch { /* ignore */ } + /* + * FNXC:CliBinaryInstall 2026-07-03-05:00: + * With shell:true on Windows the spawned child is cmd.exe; killing it leaves the underlying + * npm.cmd/node running in the background. Kill the whole process tree via taskkill /T so a + * timed-out install can't keep running detached. POSIX has no shell wrapper here, so SIGKILL + * on the child suffices. + */ + try { + if (process.platform === "win32" && typeof child.pid === "number") { + spawn("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }).on("error", () => {}); + } else { + child.kill("SIGKILL"); + } + } catch { /* ignore */ } }, INSTALL_TIMEOUT_MS); const append = (target: "stdout" | "stderr", chunk: Buffer): void => { diff --git a/packages/desktop/src/__tests__/local-server.test.ts b/packages/desktop/src/__tests__/local-server.test.ts index 6503afbc26..e672504df5 100644 --- a/packages/desktop/src/__tests__/local-server.test.ts +++ b/packages/desktop/src/__tests__/local-server.test.ts @@ -39,7 +39,9 @@ const mocks = vi.hoisted(() => { init: vi.fn(async () => undefined), close: vi.fn(async () => undefined), getProjectByPath: vi.fn(async () => ({ id: "project-1", name: "Repo", path: "/repo", status: "active" })), - listProjects: vi.fn(async () => []), + // Default: an operator who already onboarded a project. resolveDesktopRuntimePrimaryProject + // picks the first one; the runtime NEVER auto-registers the runtime root. + listProjects: vi.fn(async () => [{ id: "project-1", name: "Repo", path: "/repo", status: "active" }]), registerProject: vi.fn(async ({ path, name }: { path: string; name: string }) => ({ id: "project-1", name, path, status: "initializing" })), updateProject: vi.fn(async (id: string, patch: Record) => ({ id, name: "Repo", path: "/repo", status: patch.status ?? "active" })), }; @@ -102,7 +104,8 @@ describe("DesktopLocalServerManager", () => { expect(manager.getPort()).toBe(4545); expect(manager.getState().status).toBe("ready"); expect(mocks.engineManager.startAll).toHaveBeenCalledTimes(1); - expect(mocks.centralCore.getProjectByPath).toHaveBeenCalledWith("/repo"); + // No auto-registration of the runtime root; the primary engine is the first existing project. + expect(mocks.centralCore.registerProject).not.toHaveBeenCalled(); expect(mocks.engineManager.ensureEngine).toHaveBeenCalledWith("project-1"); expect(mocks.createServer).toHaveBeenCalledWith( expect.anything(), @@ -152,20 +155,22 @@ describe("DesktopLocalServerManager", () => { expect(manager.getState()).toMatchObject({ status: "error", error: "server failed" }); }); - it("registers an active runtime-root project when no projects exist", async () => { - mocks.centralCore.getProjectByPath.mockResolvedValueOnce(undefined); + it("never auto-registers a project and starts engine-less when no projects exist", async () => { + mocks.centralCore.listProjects.mockResolvedValueOnce([]); const { DesktopLocalServerManager } = await import("../local-server.ts"); const manager = new DesktopLocalServerManager("/repo"); - await manager.start(); + const runtime = await manager.start(); - expect(mocks.centralCore.registerProject).toHaveBeenCalledWith({ - path: "/repo", - name: "repo", - isolationMode: "in-process", - }); - expect(mocks.centralCore.updateProject).toHaveBeenCalledWith("project-1", { status: "active" }); - expect(mocks.engineManager.ensureEngine).toHaveBeenCalledWith("project-1"); + // Fresh install: the runtime must NOT create a project for its root; the dashboard onboards. + expect(mocks.centralCore.registerProject).not.toHaveBeenCalled(); + expect(mocks.engineManager.ensureEngine).not.toHaveBeenCalled(); + // The server still starts (engine-less) so the dashboard can render its onboarding empty state. + expect(runtime.port).toBe(4545); + expect(mocks.createServer).toHaveBeenCalledWith( + expect.anything(), + expect.not.objectContaining({ engine: expect.anything() }), + ); }); it("returns existing runtime when start is called twice", async () => {