fix: address PR review feedback + typecheck failure

- 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 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-02 20:57:25 -07:00
parent b843305144
commit 0c43ed42cf
3 changed files with 39 additions and 15 deletions

View File

@@ -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();

View File

@@ -86,7 +86,20 @@ function runNpmInstall(): Promise<InstallResult> {
});
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 => {

View File

@@ -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<string, unknown>) => ({ 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 () => {