harden codex mcp probe and tidy tests

Author: charlesvien

packages/agent/src/adapters/claude/claude-agent.resume-model.test.ts

@@ -3,6 +3,7 @@ import * as os from "node:os";
 import * as path from "node:path";
 import type { AgentSideConnection } from "@agentclientprotocol/sdk";
 import { afterAll, beforeEach, describe, expect, it, vi } from "vitest";
+import { DEFAULT_GATEWAY_MODEL } from "../../gateway-models";
 
 type SdkQueryHandle = {
   interrupt: ReturnType<typeof vi.fn>;
@@ -153,40 +154,43 @@ describe("ClaudeAcpAgent session model on resume", () => {
     },
   );
 
-  it("does not call setModel for a new session on the default model", async () => {
-    const agent = makeAgent();
-
-    await agent.newSession({
-      cwd,
-      mcpServers: [],
-      _meta: { taskRunId: "run-3" },
-    });
-
-    // New sessions already pass options.model to the SDK at spawn.
-    expect(createdQueries).toHaveLength(1);
-    expect(createdQueries[0].setModel).not.toHaveBeenCalled();
-  });
-
-  // Guards the desync that surfaced as "picked gpt-5.5, session ran Opus": a
-  // Codex model id paired with the Claude adapter must not silently masquerade
-  // as a deliberate Opus session. It falls back AND warns.
-  it("warns and falls back to the default model when a Codex model reaches the Claude adapter", async () => {
+  // New sessions pass the model to the SDK at spawn, never via setModel. The
+  // Codex-model row guards the desync that surfaced as "picked gpt-5.5, session
+  // ran Opus": a non-Anthropic id on the Claude adapter must fall back to the
+  // default AND warn rather than silently masquerade as a deliberate Opus run.
+  it.each([
+    {
+      name: "uses the gateway default and never calls setModel without a requested model",
+      model: undefined,
+      expectsWarn: false,
+    },
+    {
+      name: "warns and falls back to the default when a Codex model reaches the Claude adapter",
+      model: "gpt-5.5",
+      expectsWarn: true,
+    },
+  ])("newSession $name", async ({ model, expectsWarn }) => {
     const agent = makeAgent();
     const warnSpy = vi.spyOn(agent.logger, "warn");
 
     const response = await agent.newSession({
       cwd,
       mcpServers: [],
-      _meta: { taskRunId: "run-codex-on-claude", model: "gpt-5.5" },
+      _meta: { taskRunId: "run-new", ...(model ? { model } : {}) },
     });
 
+    expect(createdQueries[0].setModel).not.toHaveBeenCalled();
     expect(getModelConfigOption(response)?.currentValue).toBe(
-      "claude-opus-4-8",
-    );
-    expect(warnSpy).toHaveBeenCalledWith(
-      expect.stringContaining("Non-Anthropic model"),
-      expect.objectContaining({ requestedModel: "gpt-5.5" }),
+      DEFAULT_GATEWAY_MODEL,
     );
+    if (expectsWarn) {
+      expect(warnSpy).toHaveBeenCalledWith(
+        expect.stringContaining("Non-Anthropic model"),
+        expect.objectContaining({ requestedModel: model }),
+      );
+    } else {
+      expect(warnSpy).not.toHaveBeenCalled();
+    }
   });
 
   // The timeout *message* (RequestError "... timed out after ...") is covered

packages/agent/src/adapters/codex/settings.test.ts

@@ -15,28 +15,40 @@ describe("CodexSettingsManager MCP server names", () => {
   // server, so the spawn emitted `mcp_servers.<name>.env.enabled=false`, which
   // sets a boolean on codex's string-typed env map. codex-acp then rejected the
   // whole config, crashed the session, and the host silently ran Claude/Opus.
-  it("collapses nested mcp_servers sub-tables to the parent server name", () => {
-    expect(
-      serverNamesFor(
-        [
-          "[mcp_servers.node_repl]",
-          'command = "node"',
-          "[mcp_servers.node_repl.env]",
-          'FOO = "bar"',
-          "[mcp_servers.other]",
-          'command = "x"',
-        ].join("\n"),
-      ),
-    ).toEqual(["node_repl", "other"]);
-  });
-
-  it("keeps the inner name for a quoted dotted server key", () => {
-    expect(
-      serverNamesFor(['[mcp_servers."my.server"]', 'command = "x"'].join("\n")),
-    ).toEqual(["my.server"]);
-  });
-
-  it("returns no servers when none are declared", () => {
-    expect(serverNamesFor('model = "gpt-5.5"')).toEqual([]);
+  it.each([
+    {
+      name: "collapses a nested sub-table to its parent server and dedupes",
+      toml: [
+        "[mcp_servers.node_repl]",
+        'command = "node"',
+        "[mcp_servers.node_repl.env]",
+        'FOO = "bar"',
+        "[mcp_servers.other]",
+        'command = "x"',
+      ],
+      expected: ["node_repl", "other"],
+    },
+    {
+      name: "collapses a deeply nested sub-table to its parent server",
+      toml: ["[mcp_servers.foo.bar.baz]", 'k = "v"'],
+      expected: ["foo"],
+    },
+    {
+      name: "keeps the inner name for a double-quoted dotted server key",
+      toml: ['[mcp_servers."my.server"]', 'command = "x"'],
+      expected: ["my.server"],
+    },
+    {
+      name: "keeps the inner name for a single-quoted dotted server key",
+      toml: ["[mcp_servers.'my.server']", 'command = "x"'],
+      expected: ["my.server"],
+    },
+    {
+      name: "returns no servers when none are declared",
+      toml: ['model = "gpt-5.5"'],
+      expected: [],
+    },
+  ])("$name", ({ toml, expected }) => {
+    expect(serverNamesFor(toml.join("\n"))).toEqual(expected);
   });
 });

packages/agent/src/adapters/codex/spawn.test.ts

@@ -0,0 +1,40 @@
+import { describe, expect, it, vi } from "vitest";
+import { Logger } from "../../utils/logger";
+
+const spawnMock = vi.hoisted(() => vi.fn());
+vi.mock("node:child_process", () => ({ spawn: spawnMock }));
+
+const { spawnCodexProcess } = await import("./spawn");
+
+function makeFakeChild() {
+  return {
+    stdin: { destroy: vi.fn() },
+    stdout: { on: vi.fn(), destroy: vi.fn() },
+    stderr: { on: vi.fn(), destroy: vi.fn() },
+    on: vi.fn(),
+    kill: vi.fn(),
+    pid: 1234,
+  };
+}
+
+describe("spawnCodexProcess MCP disable args", () => {
+  it("disables bare-key servers but skips names codex's -c parser cannot express", () => {
+    spawnMock.mockReturnValue(makeFakeChild());
+
+    spawnCodexProcess({
+      logger: new Logger({ debug: false }),
+      settings: {
+        mcpServerNames: ["simple", "with-dash", "my.server", "weird name"],
+      },
+    });
+
+    const args: string[] = spawnMock.mock.calls[0][1];
+    expect(args).toContain("mcp_servers.simple.enabled=false");
+    expect(args).toContain("mcp_servers.with-dash.enabled=false");
+    // A dotted or otherwise non-bare name would emit an override codex rejects,
+    // which crashes the whole session, so it is skipped (the server stays
+    // enabled, which is harmless).
+    expect(args.some((arg) => arg.includes("my.server"))).toBe(false);
+    expect(args.some((arg) => arg.includes("weird name"))).toBe(false);
+  });
+});

packages/ui/src/features/task-detail/hooks/usePreviewConfig.ts

@@ -59,10 +59,9 @@ export function usePreviewConfig(
     // resolving the model. Otherwise lastUsedModel and lastUsedAdapter read as
     // their pre-hydration defaults, the restore below is skipped, and the
     // selector silently falls back to the server default (Opus for Claude).
-    if (!hasHydrated) {
-      setIsLoading(true);
-      return;
-    }
+    // isLoading initializes to true, so the picker stays loading until hydration
+    // lands and the fetch below resolves.
+    if (!hasHydrated) return;
 
     abortRef.current?.abort();
     const abort = new AbortController();

packages/workspace-server/src/services/agent/agent.test.ts

@@ -278,7 +278,7 @@ describe("AgentService", () => {
       );
     });
 
-    it("passes identical MCP servers regardless of adapter", async () => {
+    it("passes identical MCP servers to both adapters when all servers are reachable", async () => {
       await service.startSession({
         ...baseSessionParams,
         taskRunId: "run-claude",

packages/workspace-server/src/services/agent/agent.ts

@@ -993,7 +993,7 @@ When creating pull requests, add the following footer at the end of the PR descr
     url: string;
     headers: Array<{ name: string; value: string }>;
   }): Promise<boolean> {
-    const PROBE_TIMEOUT_MS = 4_000;
+    const PROBE_TIMEOUT_MS = 2_000;
     try {
       const headers: Record<string, string> = {
         "content-type": "application/json",
@@ -1017,7 +1017,14 @@ When creating pull requests, add the following footer at the end of the PR descr
         }),
         signal: AbortSignal.timeout(PROBE_TIMEOUT_MS),
       });
-      await response.body?.cancel();
+      // Release the body without draining it. A cancel rejection (e.g. an
+      // already-disturbed stream) is a cleanup detail, not a reachability
+      // signal, so it must not flip the result to unreachable.
+      try {
+        await response.body?.cancel();
+      } catch {
+        // ignore body cleanup failures
+      }
       // Any HTTP response means the endpoint is reachable. codex-acp only treats
       // transport failures (connection refused, DNS, timeout) as fatal; HTTP or
       // JSON-RPC error responses are handled gracefully.