fix(coding-agent): avoid blocking footer branch refresh and isolate invalid extension provider registrations closes #2418 closes #2431

packages/coding-agent/test/extensions-runner.test.ts

@@ -607,6 +607,29 @@ describe("ExtensionRunner", () => {
 	});
 
 	describe("provider registration", () => {
+		it("bindCore ignores invalid queued registrations and reports extension error", () => {
+			const runtime = createExtensionRuntime();
+			runtime.registerProvider(
+				"broken-provider",
+				{
+					streamSimple: (() => {
+						throw new Error("should not run");
+					}) as any,
+				},
+				"/tmp/broken-extension.ts",
+			);
+
+			const runner = new ExtensionRunner([], runtime, tempDir, sessionManager, modelRegistry);
+			const errors: string[] = [];
+			runner.onError((error) => errors.push(`${error.extensionPath}: ${error.error}`));
+
+			expect(() => runner.bindCore(extensionActions, extensionContextActions)).not.toThrow();
+			expect(errors).toEqual([
+				'/tmp/broken-extension.ts: Provider broken-provider: "api" is required when registering streamSimple.',
+			]);
+			expect(() => modelRegistry.refresh()).not.toThrow();
+		});
+
 		it("pre-bind unregister removes all queued registrations for a provider", () => {
 			const runtime = createExtensionRuntime();
 

packages/coding-agent/test/footer-data-provider.test.ts

@@ -1,4 +1,4 @@
-import { spawnSync } from "child_process";
+import { execFile, spawnSync } from "child_process";
 import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs";
 import { tmpdir } from "os";
 import { join } from "path";
@@ -7,6 +7,28 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 let resolvedBranch = "main";
 
 vi.mock("child_process", () => ({
+	execFile: vi.fn(
+		(
+			_command: string,
+			args: readonly string[],
+			_options: unknown,
+			callback: (error: Error | null, stdout: string, stderr: string) => void,
+		) => {
+			if (args[1] === "symbolic-ref") {
+				setTimeout(
+					() =>
+						callback(
+							resolvedBranch ? null : new Error("detached"),
+							resolvedBranch ? `${resolvedBranch}\n` : "",
+							"",
+						),
+					0,
+				);
+				return;
+			}
+			setTimeout(() => callback(new Error("unsupported"), "", ""), 0);
+		},
+	),
 	spawnSync: vi.fn((_command: string, args: readonly string[]) => {
 		if (args[1] === "symbolic-ref") {
 			return { status: resolvedBranch ? 0 : 1, stdout: resolvedBranch ? `${resolvedBranch}\n` : "", stderr: "" };
@@ -55,7 +77,7 @@ function createReftableWorktree(tempDir: string): WorktreeFixture {
 	return { worktreeDir, reftableDir };
 }
 
-async function waitFor(condition: () => boolean, timeoutMs = 2000): Promise<void> {
+async function waitFor(condition: () => boolean, timeoutMs = 3000): Promise<void> {
 	const startedAt = Date.now();
 	while (!condition()) {
 		if (Date.now() - startedAt > timeoutMs) {
@@ -74,6 +96,7 @@ describe("FooterDataProvider reftable branch detection", () => {
 		tempDir = mkdtempSync(join(tmpdir(), "footer-data-provider-"));
 		resolvedBranch = "main";
 		vi.mocked(spawnSync).mockClear();
+		vi.mocked(execFile).mockClear();
 	});
 
 	afterEach(() => {
@@ -156,8 +179,10 @@ describe("FooterDataProvider reftable branch detection", () => {
 			provider.onBranchChange(onBranchChange);
 
 			writeFileSync(join(reftableDir, "tables.list"), "1\n");
-			await waitFor(() => vi.mocked(spawnSync).mock.calls.length > 0);
+			await waitFor(() => vi.mocked(execFile).mock.calls.length === 1);
 
+			expect(vi.mocked(execFile)).toHaveBeenCalledTimes(1);
+			expect(vi.mocked(spawnSync)).not.toHaveBeenCalled();
 			expect(provider.getGitBranch()).toBe("main");
 			expect(onBranchChange).not.toHaveBeenCalled();
 		} finally {
@@ -165,6 +190,27 @@ describe("FooterDataProvider reftable branch detection", () => {
 		}
 	});
 
+	it("debounces rapid reftable updates into a single async refresh", async () => {
+		const { worktreeDir, reftableDir } = createReftableWorktree(tempDir);
+		process.chdir(worktreeDir);
+
+		const provider = new FooterDataProvider();
+		try {
+			expect(provider.getGitBranch()).toBe("main");
+			vi.mocked(execFile).mockClear();
+
+			writeFileSync(join(reftableDir, "tables.list"), "1\n");
+			writeFileSync(join(reftableDir, "tables.list"), "2\n");
+			writeFileSync(join(reftableDir, "tables.list"), "3\n");
+			await waitFor(() => vi.mocked(execFile).mock.calls.length === 1);
+			await new Promise((resolve) => setTimeout(resolve, 650));
+
+			expect(vi.mocked(execFile)).toHaveBeenCalledTimes(1);
+		} finally {
+			provider.dispose();
+		}
+	});
+
 	it("updates the cached branch when the reftable directory changes", async () => {
 		const { worktreeDir, reftableDir } = createReftableWorktree(tempDir);
 		process.chdir(worktreeDir);
@@ -173,17 +219,16 @@ describe("FooterDataProvider reftable branch detection", () => {
 		try {
 			expect(provider.getGitBranch()).toBe("main");
 			resolvedBranch = "foo";
+			const onBranchChange = vi.fn();
+			provider.onBranchChange(onBranchChange);
 
-			await new Promise<void>((resolve, reject) => {
-				const timeout = setTimeout(() => reject(new Error("Timed out waiting for branch change")), 2000);
-				provider.onBranchChange(() => {
-					clearTimeout(timeout);
-					resolve();
-				});
-				writeFileSync(join(reftableDir, "tables.list"), "1\n");
-			});
+			writeFileSync(join(reftableDir, "tables.list"), "1\n");
+			await waitFor(() => vi.mocked(execFile).mock.calls.length === 1);
+			await waitFor(() => provider.getGitBranch() === "foo");
 
+			expect(vi.mocked(execFile)).toHaveBeenCalledTimes(1);
 			expect(provider.getGitBranch()).toBe("foo");
+			expect(onBranchChange).toHaveBeenCalledTimes(1);
 		} finally {
 			provider.dispose();
 		}

packages/coding-agent/test/model-registry.test.ts

@@ -707,6 +707,65 @@ describe("ModelRegistry", () => {
 	});
 
 	describe("dynamic provider lifecycle", () => {
+		test("failed registerProvider does not persist invalid streamSimple config", () => {
+			const registry = new ModelRegistry(authStorage, modelsJsonPath);
+
+			expect(() =>
+				registry.registerProvider("broken-provider", {
+					streamSimple: (() => {
+						throw new Error("should not run");
+					}) as any,
+				}),
+			).toThrow('Provider broken-provider: "api" is required when registering streamSimple.');
+
+			expect(() => registry.refresh()).not.toThrow();
+		});
+
+		test("failed registerProvider does not remove existing provider models", () => {
+			const registry = new ModelRegistry(authStorage, modelsJsonPath);
+
+			registry.registerProvider("demo-provider", {
+				baseUrl: "https://provider.test/v1",
+				apiKey: "TEST_KEY",
+				api: "openai-completions",
+				models: [
+					{
+						id: "demo-model",
+						name: "Demo Model",
+						reasoning: false,
+						input: ["text"],
+						cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+						contextWindow: 128000,
+						maxTokens: 4096,
+					},
+				],
+			});
+
+			expect(registry.find("demo-provider", "demo-model")).toBeDefined();
+
+			expect(() =>
+				registry.registerProvider("demo-provider", {
+					baseUrl: "https://provider.test/v2",
+					apiKey: "TEST_KEY",
+					models: [
+						{
+							id: "broken-model",
+							name: "Broken Model",
+							reasoning: false,
+							input: ["text"],
+							cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+							contextWindow: 128000,
+							maxTokens: 4096,
+						},
+					],
+				}),
+			).toThrow('Provider demo-provider, model broken-model: no "api" specified.');
+
+			expect(registry.find("demo-provider", "demo-model")).toBeDefined();
+			expect(() => registry.refresh()).not.toThrow();
+			expect(registry.find("demo-provider", "demo-model")).toBeDefined();
+		});
+
 		test("unregisterProvider removes custom OAuth provider and restores built-in OAuth provider", () => {
 			const registry = new ModelRegistry(authStorage, modelsJsonPath);