diff --git a/packages/coding-agent/src/core/extensions/loader.ts b/packages/coding-agent/src/core/extensions/loader.ts index 2f7ace30..0dd00fe6 100644 --- a/packages/coding-agent/src/core/extensions/loader.ts +++ b/packages/coding-agent/src/core/extensions/loader.ts @@ -141,8 +141,8 @@ export function createExtensionRuntime(): ExtensionRuntime { pendingProviderRegistrations: [], // Pre-bind: queue registrations so bindCore() can flush them once the // model registry is available. bindCore() replaces both with direct calls. - registerProvider: (name, config) => { - runtime.pendingProviderRegistrations.push({ name, config }); + registerProvider: (name, config, extensionPath = "") => { + runtime.pendingProviderRegistrations.push({ name, config, extensionPath }); }, unregisterProvider: (name) => { runtime.pendingProviderRegistrations = runtime.pendingProviderRegistrations.filter((r) => r.name !== name); @@ -271,11 +271,11 @@ function createExtensionAPI( }, registerProvider(name: string, config: ProviderConfig) { - runtime.registerProvider(name, config); + runtime.registerProvider(name, config, extension.path); }, unregisterProvider(name: string) { - runtime.unregisterProvider(name); + runtime.unregisterProvider(name, extension.path); }, events: eventBus, diff --git a/packages/coding-agent/src/core/extensions/runner.ts b/packages/coding-agent/src/core/extensions/runner.ts index 65234bb4..695e1cf3 100644 --- a/packages/coding-agent/src/core/extensions/runner.ts +++ b/packages/coding-agent/src/core/extensions/runner.ts @@ -271,11 +271,20 @@ export class ExtensionRunner { this.getSystemPromptFn = contextActions.getSystemPrompt; // Flush provider registrations queued during extension loading - for (const { name, config } of this.runtime.pendingProviderRegistrations) { - if (providerActions?.registerProvider) { - providerActions.registerProvider(name, config); - } else { - this.modelRegistry.registerProvider(name, config); + for (const { name, config, extensionPath } of this.runtime.pendingProviderRegistrations) { + try { + if (providerActions?.registerProvider) { + providerActions.registerProvider(name, config); + } else { + this.modelRegistry.registerProvider(name, config); + } + } catch (err) { + this.emitError({ + extensionPath, + event: "register_provider", + error: err instanceof Error ? err.message : String(err), + stack: err instanceof Error ? err.stack : undefined, + }); } } this.runtime.pendingProviderRegistrations = []; diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 76960667..aac7c684 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -1307,15 +1307,15 @@ export type SetLabelHandler = (entryId: string, label: string | undefined) => vo export interface ExtensionRuntimeState { flagValues: Map; /** Provider registrations queued during extension loading, processed when runner binds */ - pendingProviderRegistrations: Array<{ name: string; config: ProviderConfig }>; + pendingProviderRegistrations: Array<{ name: string; config: ProviderConfig; extensionPath: string }>; /** * Register or unregister a provider. * * Before bindCore(): queues registrations / removes from queue. * After bindCore(): calls ModelRegistry directly for immediate effect. */ - registerProvider: (name: string, config: ProviderConfig) => void; - unregisterProvider: (name: string) => void; + registerProvider: (name: string, config: ProviderConfig, extensionPath?: string) => void; + unregisterProvider: (name: string, extensionPath?: string) => void; } /** diff --git a/packages/coding-agent/src/core/footer-data-provider.ts b/packages/coding-agent/src/core/footer-data-provider.ts index 1338d963..2014307f 100644 --- a/packages/coding-agent/src/core/footer-data-provider.ts +++ b/packages/coding-agent/src/core/footer-data-provider.ts @@ -1,4 +1,4 @@ -import { spawnSync } from "child_process"; +import { type ExecFileException, execFile, spawnSync } from "child_process"; import { existsSync, type FSWatcher, readFileSync, statSync, watch } from "fs"; import { dirname, join, resolve } from "path"; @@ -47,7 +47,7 @@ function findGitPaths(): GitPaths | null { } /** Ask git for the current branch. Returns null on detached HEAD or if git is unavailable. */ -function resolveBranchWithGit(repoDir: string): string | null { +function resolveBranchWithGitSync(repoDir: string): string | null { const result = spawnSync("git", ["--no-optional-locks", "symbolic-ref", "--quiet", "--short", "HEAD"], { cwd: repoDir, encoding: "utf8", @@ -57,11 +57,35 @@ function resolveBranchWithGit(repoDir: string): string | null { return branch || null; } +/** Ask git for the current branch asynchronously. Returns null on detached HEAD or if git is unavailable. */ +function resolveBranchWithGitAsync(repoDir: string): Promise { + return new Promise((resolvePromise) => { + execFile( + "git", + ["--no-optional-locks", "symbolic-ref", "--quiet", "--short", "HEAD"], + { + cwd: repoDir, + encoding: "utf8", + }, + (error: ExecFileException | null, stdout: string) => { + if (error) { + resolvePromise(null); + return; + } + const branch = stdout.trim(); + resolvePromise(branch || null); + }, + ); + }); +} + /** * Provides git branch and extension statuses - data not otherwise accessible to extensions. * Token stats, model info available via ctx.sessionManager and ctx.model. */ export class FooterDataProvider { + private static readonly WATCH_DEBOUNCE_MS = 500; + private extensionStatuses = new Map(); private cachedBranch: string | null | undefined = undefined; private gitPaths: GitPaths | null | undefined = undefined; @@ -69,6 +93,10 @@ export class FooterDataProvider { private reftableWatcher: FSWatcher | null = null; private branchChangeCallbacks = new Set<() => void>(); private availableProviderCount = 0; + private refreshTimer: ReturnType | null = null; + private refreshInFlight = false; + private refreshPending = false; + private disposed = false; constructor() { this.gitPaths = findGitPaths(); @@ -78,7 +106,7 @@ export class FooterDataProvider { /** Current git branch, null if not in repo, "detached" if detached HEAD */ getGitBranch(): string | null { if (this.cachedBranch === undefined) { - this.cachedBranch = this.resolveGitBranch(); + this.cachedBranch = this.resolveGitBranchSync(); } return this.cachedBranch; } @@ -120,6 +148,11 @@ export class FooterDataProvider { /** Internal: cleanup */ dispose(): void { + this.disposed = true; + if (this.refreshTimer) { + clearTimeout(this.refreshTimer); + this.refreshTimer = null; + } if (this.headWatcher) { this.headWatcher.close(); this.headWatcher = null; @@ -135,23 +168,66 @@ export class FooterDataProvider { for (const cb of this.branchChangeCallbacks) cb(); } - private refreshGitBranch(): void { - const nextBranch = this.resolveGitBranch(); - if (this.cachedBranch !== undefined && this.cachedBranch !== nextBranch) { - this.cachedBranch = nextBranch; - this.notifyBranchChange(); - return; + private scheduleRefresh(): void { + if (this.disposed) return; + if (this.refreshTimer) { + clearTimeout(this.refreshTimer); } - this.cachedBranch = nextBranch; + this.refreshTimer = setTimeout(() => { + this.refreshTimer = null; + void this.refreshGitBranchAsync(); + }, FooterDataProvider.WATCH_DEBOUNCE_MS); } - private resolveGitBranch(): string | null { + private async refreshGitBranchAsync(): Promise { + if (this.disposed) return; + if (this.refreshInFlight) { + this.refreshPending = true; + return; + } + + this.refreshInFlight = true; + try { + const nextBranch = await this.resolveGitBranchAsync(); + if (this.disposed) return; + if (this.cachedBranch !== undefined && this.cachedBranch !== nextBranch) { + this.cachedBranch = nextBranch; + this.notifyBranchChange(); + return; + } + this.cachedBranch = nextBranch; + } finally { + this.refreshInFlight = false; + if (this.refreshPending && !this.disposed) { + this.refreshPending = false; + this.scheduleRefresh(); + } + } + } + + private resolveGitBranchSync(): string | null { try { if (!this.gitPaths) return null; const content = readFileSync(this.gitPaths.headPath, "utf8").trim(); if (content.startsWith("ref: refs/heads/")) { const branch = content.slice(16); - return branch === ".invalid" ? (resolveBranchWithGit(this.gitPaths.repoDir) ?? "detached") : branch; + return branch === ".invalid" ? (resolveBranchWithGitSync(this.gitPaths.repoDir) ?? "detached") : branch; + } + return "detached"; + } catch { + return null; + } + } + + private async resolveGitBranchAsync(): Promise { + try { + if (!this.gitPaths) return null; + const content = readFileSync(this.gitPaths.headPath, "utf8").trim(); + if (content.startsWith("ref: refs/heads/")) { + const branch = content.slice(16); + return branch === ".invalid" + ? ((await resolveBranchWithGitAsync(this.gitPaths.repoDir)) ?? "detached") + : branch; } return "detached"; } catch { @@ -168,7 +244,7 @@ export class FooterDataProvider { try { this.headWatcher = watch(dirname(this.gitPaths.headPath), (_eventType, filename) => { if (!filename || filename.toString() === "HEAD") { - this.refreshGitBranch(); + this.scheduleRefresh(); } }); } catch { @@ -181,7 +257,7 @@ export class FooterDataProvider { if (existsSync(reftableDir)) { try { this.reftableWatcher = watch(reftableDir, () => { - this.refreshGitBranch(); + this.scheduleRefresh(); }); } catch { // Silently fail if we can't watch diff --git a/packages/coding-agent/src/core/model-registry.ts b/packages/coding-agent/src/core/model-registry.ts index ca9b09b8..514fe832 100644 --- a/packages/coding-agent/src/core/model-registry.ts +++ b/packages/coding-agent/src/core/model-registry.ts @@ -569,8 +569,9 @@ export class ModelRegistry { * If provider has oauth: registers OAuth provider for /login support. */ registerProvider(providerName: string, config: ProviderConfigInput): void { - this.registeredProviders.set(providerName, config); + this.validateProviderConfig(providerName, config); this.applyProviderConfig(providerName, config); + this.registeredProviders.set(providerName, config); } /** @@ -589,6 +590,30 @@ export class ModelRegistry { this.refresh(); } + private validateProviderConfig(providerName: string, config: ProviderConfigInput): void { + if (config.streamSimple && !config.api) { + throw new Error(`Provider ${providerName}: "api" is required when registering streamSimple.`); + } + + if (!config.models || config.models.length === 0) { + return; + } + + if (!config.baseUrl) { + throw new Error(`Provider ${providerName}: "baseUrl" is required when defining models.`); + } + if (!config.apiKey && !config.oauth) { + throw new Error(`Provider ${providerName}: "apiKey" or "oauth" is required when defining models.`); + } + + for (const modelDef of config.models) { + const api = modelDef.api || config.api; + if (!api) { + throw new Error(`Provider ${providerName}, model ${modelDef.id}: no "api" specified.`); + } + } + } + private applyProviderConfig(providerName: string, config: ProviderConfigInput): void { // Register OAuth provider if provided if (config.oauth) { @@ -601,13 +626,10 @@ export class ModelRegistry { } if (config.streamSimple) { - if (!config.api) { - throw new Error(`Provider ${providerName}: "api" is required when registering streamSimple.`); - } const streamSimple = config.streamSimple; registerApiProvider( { - api: config.api, + api: config.api!, stream: (model, context, options) => streamSimple(model, context, options as SimpleStreamOptions), streamSimple, }, @@ -624,20 +646,9 @@ export class ModelRegistry { // Full replacement: remove existing models for this provider this.models = this.models.filter((m) => m.provider !== providerName); - // Validate required fields - if (!config.baseUrl) { - throw new Error(`Provider ${providerName}: "baseUrl" is required when defining models.`); - } - if (!config.apiKey && !config.oauth) { - throw new Error(`Provider ${providerName}: "apiKey" or "oauth" is required when defining models.`); - } - // Parse and add new models for (const modelDef of config.models) { const api = modelDef.api || config.api; - if (!api) { - throw new Error(`Provider ${providerName}, model ${modelDef.id}: no "api" specified.`); - } // Merge headers const providerHeaders = resolveHeaders(config.headers); @@ -657,7 +668,7 @@ export class ModelRegistry { name: modelDef.name, api: api as Api, provider: providerName, - baseUrl: config.baseUrl, + baseUrl: config.baseUrl!, reasoning: modelDef.reasoning, input: modelDef.input as ("text" | "image")[], cost: modelDef.cost, diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index b32409fa..c2f2ab5b 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -674,8 +674,13 @@ export async function main(args: string[]) { // Apply pending provider registrations from extensions immediately // so they're available for model resolution before AgentSession is created - for (const { name, config } of extensionsResult.runtime.pendingProviderRegistrations) { - modelRegistry.registerProvider(name, config); + for (const { name, config, extensionPath } of extensionsResult.runtime.pendingProviderRegistrations) { + try { + modelRegistry.registerProvider(name, config); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(chalk.red(`Extension "${extensionPath}" error: ${message}`)); + } } extensionsResult.runtime.pendingProviderRegistrations = []; diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index 38398a54..61c2fc4e 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/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(); diff --git a/packages/coding-agent/test/footer-data-provider.test.ts b/packages/coding-agent/test/footer-data-provider.test.ts index f9bc4ac2..c3d90532 100644 --- a/packages/coding-agent/test/footer-data-provider.test.ts +++ b/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 { +async function waitFor(condition: () => boolean, timeoutMs = 3000): Promise { 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((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(); } diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts index 010e9fd2..ae54e49d 100644 --- a/packages/coding-agent/test/model-registry.test.ts +++ b/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);