Preserve ACP catalogs when explicit model discovery fails

[bb-slop-cop]

🚨 SLOP COP 🚨 · new-issue-autopilot

Human comments

What was wrong

An explicit ACP model-list command that failed, timed out, or produced no usable catalog fell through to a successful synthetic-default response when the bridge's process-local cache was empty. The server correctly treated that response as authoritative and replaced its last good catalog. Maintenance runtime idle shutdown makes an empty bridge cache a normal possibility.

Verified report: https://get-bb.github.io/reports/issues/5311.html

What changed

Reject explicit CLI model discovery when neither a fresh nor cached catalog is available, using the existing JSON-RPC error path. Preserve the synthetic default for agents without a configured listing and preserve same-command last-good cache reuse. No public protocol, schema, host wire fields, stored data, dependency, packaging, or release changes.

The patch changes 51 total text lines: 47 additions and four deletions across two files, entirely in the existing ACP bridge subsystem. Five lines are production additions; 46 changed lines are tests.

How you verified

• Trusted base: fe1a02d7b994866cfd477e3ad7c0bc85fa525002 (origin/main). Repeated by the same agent in a second clean trusted checkout.
• Added regression tests before production changes. On unchanged production, unsuccessful exit, unusable output, and a real 30-second timeout fail because the expected error is absent; same-command last-good cache preservation passes. Both clean runs produce three failures and one pass.
• pnpm exec turbo run test lint typecheck --filter=@bb/provider-bridge-acp: all tasks pass; 388 tests pass, four intentionally skip, across 19 files. This includes all focused regression cases after the guard.
• pnpm exec turbo run test --filter=@bb/server -- test/providers/provider-model-catalog-store.test.ts: 18 tests pass, including retaining the persisted last good catalog after failed refreshes.
• Frozen installs and full Turbo builds pass in both clean checkouts. git diff --check passes; no binary changes. No open linked PR existed before opening this one.

Fixes #5311

AGENT GENERATED

[thomaswillner]

This fixes provider-bridge-acp, but provider-bridge-acp-next (added in #5266) has the same fallthrough at the end of handleModelList. When a model-list command is configured and returns no catalog, it still sends an acp-default-only result, so ACP-next users still see the catalog collapse described in #5311.

Could this PR also cover provider-bridge-acp-next? The same guard works there, placed after the CLI-catalog branch and with the same message:

  if (params.listCommand) {
    throw new Error(
      "ACP model list command failed to provide a model catalog.",
    );
  }

Verified on main @ 59a1fcc, package @bb/provider-bridge-acp-next, vitest -t "model/list|catalog":

• Without the guard: 6 failed, 8 passed. The failures are exit non-zero, no model lines, timeout, SIGKILL, >1 MiB stdout (with a valid model line before the overflow), and a changed command key that must not reuse another command's cache. Each fails on the error assertion.
• With the guard: 14 passed. tsc --noEmit and oxlint are clean.
• Mutation (guard disabled): the same 6 fail.
• Unchanged behaviour:
  • missing executable keeps its ENOENT error;
  • auth-required keeps its recovery error;
  • with no list command, the synthetic acp-default fallback is unchanged;
  • the same-command last-good cache still answers;
  • a listed acp-default - <name> line is treated as a real catalog.

Note for the timeout test in ACP-next: on unguarded code the fallthrough also awaits declaredOptions(), which probes the hanging program for up to another 30 s. The synthetic result therefore arrives at about 60 s, and the test needs a wait of about 70 s (a 40 s wait fails on "Timed out waiting", not on the assertion). With the guard it answers at about 30 s.

Full diff (bridge.ts + bridge.test.ts)

diff --git a/packages/provider-bridge-acp-next/src/bridge/bridge.test.ts b/packages/provider-bridge-acp-next/src/bridge/bridge.test.ts
index 607daf2..a46f61c 100644
--- a/packages/provider-bridge-acp-next/src/bridge/bridge.test.ts
+++ b/packages/provider-bridge-acp-next/src/bridge/bridge.test.ts
@@ -1259,10 +1259,116 @@ describe("acp bridge", () => {
     });
   });
 
-  it("falls back to the synthetic model when the list command prints no models", async () => {
-    const emptyId = sendModelList({ modelLines: "no model lines here" });
-    expect((await waitForResponse(emptyId)).result).toMatchObject({
-      models: [{ id: "acp-default", isDefault: true }],
+  it.each([
+    ["exits unsuccessfully", "process.exit(2)"],
+    ["prints no models", "process.stdout.write('no model lines here')"],
+    ["times out", "setInterval(() => {}, 1000)"],
+    ["is killed by a signal", "process.kill(process.pid, 'SIGKILL')"],
+    [
+      "exceeds the output buffer",
+      "process.stdout.write('kept-model - Kept Model\\n' + 'x'.repeat(2 * 1024 * 1024))",
+    ],
+  ])(
+    "fails model/list when the configured command %s without a cached catalog",
+    async (_failure, script) => {
+      const failingId = sendModelList({
+        agent: { command: process.execPath, args: ["-e", script] },
+        modelListArgs: ["--"],
+      });
+      const response = await waitFor(
+        () => findResponse(failingId),
+        `response ${failingId}`,
+        70_000,
+      );
+      expect(response.error?.message).toBe(
+        "ACP model list command failed to provide a model catalog.",
+      );
+      expect(response.result).toBeUndefined();
+    },
+    75_000,
+  );
+
+  it("keeps the last good CLI catalog when the same command later fails", async () => {
+    const scriptPath = join(workspaceDir, "model-list.cjs");
+    writeFileSync(
+      scriptPath,
+      "process.stdout.write('cached-model - Cached Model\\n')",
+    );
+    const launch = {
+      agent: { command: process.execPath, args: [scriptPath] },
+      modelListArgs: ["--"],
+    };
+    const first = await waitForResponse(sendModelList(launch));
+    expect(first.result).toMatchObject({
+      models: [{ id: "cached-model", displayName: "Cached Model" }],
+    });
+
+    writeFileSync(scriptPath, "process.exit(2)");
+    const second = await waitForResponse(sendModelList(launch));
+    expect(second.error).toBeUndefined();
+    expect(second.result).toEqual(first.result);
+  });
+
+  it("does not serve another command's cached catalog when the command changes", async () => {
+    const scriptPath = join(workspaceDir, "model-list-keyed.cjs");
+    writeFileSync(
+      scriptPath,
+      "process.stdout.write('keyed-model - Keyed Model\\n')",
+    );
+    const first = await waitForResponse(
+      sendModelList({
+        agent: { command: process.execPath, args: [scriptPath] },
+        modelListArgs: ["--"],
+      }),
+    );
+    expect(first.result).toMatchObject({ models: [{ id: "keyed-model" }] });
+
+    writeFileSync(scriptPath, "process.exit(2)");
+    const changed = await waitForResponse(
+      sendModelList({
+        agent: { command: process.execPath, args: [scriptPath] },
+        modelListArgs: ["--", "--changed-flag"],
+      }),
+    );
+    expect(changed.error?.message).toBe(
+      "ACP model list command failed to provide a model catalog.",
+    );
+    expect(changed.result).toBeUndefined();
+  });
+
+  it("returns a listed acp-default line as a real catalog, not the synthetic fallback", async () => {
+    const scriptPath = join(workspaceDir, "model-list-default.cjs");
+    writeFileSync(
+      scriptPath,
+      "process.stdout.write('acp-default - Listed Default\\n')",
+    );
+    const response = await waitForResponse(
+      sendModelList({
+        agent: { command: process.execPath, args: [scriptPath] },
+        modelListArgs: ["--"],
+      }),
+    );
+    expect(response.error).toBeUndefined();
+    expect(response.result).toMatchObject({
+      models: [{ id: "acp-default", displayName: "Listed Default" }],
+    });
+  });
+
+  it("returns a legitimate single-entry catalog as a result, not an error", async () => {
+    const scriptPath = join(workspaceDir, "model-list-single.cjs");
+    writeFileSync(
+      scriptPath,
+      "process.stdout.write('only-model - Only Model\\n')",
+    );
+    const response = await waitForResponse(
+      sendModelList({
+        agent: { command: process.execPath, args: [scriptPath] },
+        modelListArgs: ["--"],
+      }),
+    );
+    expect(response.error).toBeUndefined();
+    expect(response.result).toMatchObject({
+      models: [{ id: "only-model", displayName: "Only Model" }],
     });
   });
 
diff --git a/packages/provider-bridge-acp-next/src/bridge/bridge.ts b/packages/provider-bridge-acp-next/src/bridge/bridge.ts
index 996654b..c052902 100644
--- a/packages/provider-bridge-acp-next/src/bridge/bridge.ts
+++ b/packages/provider-bridge-acp-next/src/bridge/bridge.ts
@@ -3379,6 +3379,11 @@ async function handleModelList(
     );
     return;
   }
+  if (params.listCommand) {
+    throw new Error(
+      "ACP model list command failed to provide a model catalog.",
+    );
+  }
   const sessionDiscoveredModels =
     params.listCommand === undefined && params.agent
       ? await loadSessionDiscoveredModels(

Refs #5311.

AGENT GENERATED: Claude Code, on behalf of @thomaswillner (reporter of #5311). Tests were run locally, not in CI.

[thomaswillner]

The ACP-next extension is ready as a single commit on top of main @ 59a1fcc:
thomaswillner@6901eec (branch thomaswillner:fix/5311-acp-next-model-list-failure).

It touches only packages/provider-bridge-acp-next/src/bridge/{bridge.ts,bridge.test.ts}. It should cherry-pick cleanly onto this PR's branch:

git fetch https://github.com/thomaswillner/bb.git fix/5311-acp-next-model-list-failure
git cherry-pick 6901eec977aec84d32e768759f810781131c2fab

I'm not on APPROVED_CONTRIBUTORS, so I haven't opened a separate PR. Maintainers or slop-cop, please take it into #5313 or a follow-up, whichever you prefer.

AGENT GENERATED: Claude Code, on behalf of @thomaswillner.