diff --git a/packages/coding-agent/src/core/extensions/loader.ts b/packages/coding-agent/src/core/extensions/loader.ts index d560e67c4..36df2da21 100644 --- a/packages/coding-agent/src/core/extensions/loader.ts +++ b/packages/coding-agent/src/core/extensions/loader.ts @@ -256,18 +256,38 @@ function createExtensionAPI( runtime: ExtensionRuntime, cwd: string, eventBus: EventBus, -): ExtensionAPI { +): { api: ExtensionAPI; commit: () => void; discard: () => void } { + const pendingFlagValues = new Map(); + const pendingRuntimeChanges: Array<() => void> = []; + const loadingUnsubscribers: Array<() => void> = []; + let state: "loading" | "active" | "failed" = "loading"; + const assertActive = () => { + if (state === "failed") { + throw new Error(`Extension "${extension.path}" failed to load and its API is no longer active.`); + } + runtime.assertActive(); + }; + const applyRuntimeChange = (change: () => void) => { + if (state === "loading") pendingRuntimeChanges.push(change); + else change(); + }; + const clearPending = () => { + pendingFlagValues.clear(); + pendingRuntimeChanges.length = 0; + loadingUnsubscribers.length = 0; + }; + const api = { // Registration methods - write to extension on(event: string, handler: HandlerFn): void { - runtime.assertActive(); + assertActive(); const list = extension.handlers.get(event) ?? []; list.push(handler); extension.handlers.set(event, list); }, registerTool(tool: ToolDefinition): void { - runtime.assertActive(); + assertActive(); extension.tools.set(tool.name, { definition: tool, sourceInfo: extension.sourceInfo, @@ -276,7 +296,7 @@ function createExtensionAPI( }, registerCommand(name: string, options: Omit): void { - runtime.assertActive(); + assertActive(); extension.commands.set(name, { name, sourceInfo: extension.sourceInfo, @@ -291,7 +311,7 @@ function createExtensionAPI( handler: (ctx: import("./types.ts").ExtensionContext) => Promise | void; }, ): void { - runtime.assertActive(); + assertActive(); extension.shortcuts.set(shortcut, { shortcut, extensionPath: extension.path, ...options }); }, @@ -299,7 +319,7 @@ function createExtensionAPI( name: string, options: { description?: string; type: "boolean" | "string"; default?: boolean | string }, ): void { - runtime.assertActive(); + assertActive(); if (options.default !== undefined && typeof options.default !== options.type) { throw new Error( `Invalid default for flag "${name}": expected ${options.type}, got ${typeof options.default}`, @@ -307,132 +327,156 @@ function createExtensionAPI( } extension.flags.set(name, { name, extensionPath: extension.path, ...options }); if (options.default !== undefined && !runtime.flagValues.has(name)) { - runtime.flagValues.set(name, options.default); + if (state === "loading") { + if (!pendingFlagValues.has(name)) pendingFlagValues.set(name, options.default); + } else { + runtime.flagValues.set(name, options.default); + } } }, registerMessageRenderer(customType: string, renderer: MessageRenderer): void { - runtime.assertActive(); + assertActive(); extension.messageRenderers.set(customType, renderer as MessageRenderer); }, registerMarkdownTransformer(transformer: MarkdownTransformer): void { - runtime.assertActive(); + assertActive(); extension.markdownTransformer = transformer; }, registerEntryRenderer(customType: string, renderer: EntryRenderer): void { - runtime.assertActive(); + assertActive(); extension.entryRenderers ??= new Map(); extension.entryRenderers.set(customType, renderer as EntryRenderer); }, // Flag access - checks extension registered it, reads from runtime getFlag(name: string): boolean | string | undefined { - runtime.assertActive(); + assertActive(); if (!extension.flags.has(name)) return undefined; - return runtime.flagValues.get(name); + return runtime.flagValues.has(name) ? runtime.flagValues.get(name) : pendingFlagValues.get(name); }, // Action methods - delegate to shared runtime sendMessage(message, options): void { - runtime.assertActive(); + assertActive(); runtime.sendMessage(message, options); }, sendUserMessage(content, options): void { - runtime.assertActive(); + assertActive(); runtime.sendUserMessage(content, options); }, appendEntry(customType: string, data?: unknown): void { - runtime.assertActive(); + assertActive(); runtime.appendEntry(customType, data); }, setSessionName(name: string): void { - runtime.assertActive(); + assertActive(); runtime.setSessionName(name); }, getSessionName(): string | undefined { - runtime.assertActive(); + assertActive(); return runtime.getSessionName(); }, setLabel(entryId: string, label: string | undefined): void { - runtime.assertActive(); + assertActive(); runtime.setLabel(entryId, label); }, exec(command: string, args: string[], options?: ExecOptions) { - runtime.assertActive(); + assertActive(); return execCommand(command, args, options?.cwd ?? cwd, options); }, getActiveTools(): string[] { - runtime.assertActive(); + assertActive(); return runtime.getActiveTools(); }, getAllTools() { - runtime.assertActive(); + assertActive(); return runtime.getAllTools(); }, setActiveTools(toolNames: string[]): void { - runtime.assertActive(); + assertActive(); runtime.setActiveTools(toolNames); }, getCommands() { - runtime.assertActive(); + assertActive(); return runtime.getCommands(); }, setModel(model) { - runtime.assertActive(); + assertActive(); return runtime.setModel(model); }, getThinkingLevel() { - runtime.assertActive(); + assertActive(); return runtime.getThinkingLevel(); }, setThinkingLevel(level) { - runtime.assertActive(); + assertActive(); runtime.setThinkingLevel(level); }, registerProvider(providerOrName: Provider | string, config?: ProviderConfig) { - runtime.assertActive(); + assertActive(); if (typeof providerOrName === "string") { if (!config) throw new Error("Provider config is required when registering by name"); - runtime.registerProvider(providerOrName, config, extension.path); + applyRuntimeChange(() => runtime.registerProvider(providerOrName, config, extension.path)); return; } - runtime.registerNativeProvider(providerOrName, extension.path); + applyRuntimeChange(() => runtime.registerNativeProvider(providerOrName, extension.path)); }, unregisterProvider(name: string) { - runtime.assertActive(); - runtime.unregisterProvider(name, extension.path); + assertActive(); + applyRuntimeChange(() => runtime.unregisterProvider(name, extension.path)); }, events: { emit(channel, data) { - runtime.assertActive(); + assertActive(); eventBus.emit(channel, data); }, on(channel, handler) { - runtime.assertActive(); - return runtime.trackEventBusSubscription(eventBus.on(channel, handler)); + assertActive(); + const unsubscribe = runtime.trackEventBusSubscription(eventBus.on(channel, handler)); + if (state === "loading") loadingUnsubscribers.push(unsubscribe); + return unsubscribe; }, }, } as ExtensionAPI; - return api; + return { + api, + commit: () => { + if (state !== "loading") return; + runtime.assertActive(); + for (const [name, value] of pendingFlagValues) { + if (!runtime.flagValues.has(name)) runtime.flagValues.set(name, value); + } + for (const apply of pendingRuntimeChanges) apply(); + state = "active"; + clearPending(); + }, + discard: () => { + if (state !== "loading") return; + state = "failed"; + for (const unsubscribe of loadingUnsubscribers) unsubscribe(); + clearPending(); + }, + }; } function isCurrentCacheToken(cacheToken: ExtensionCacheToken | undefined): cacheToken is ExtensionCacheToken { @@ -498,6 +542,27 @@ function createExtension(extensionPath: string, resolvedPath: string): Extension }; } +async function initializeExtension( + factory: ExtensionFactory, + extensionPath: string, + resolvedPath: string, + cwd: string, + eventBus: EventBus, + runtime: ExtensionRuntime, +): Promise { + const extension = createExtension(extensionPath, resolvedPath); + const load = createExtensionAPI(extension, runtime, cwd, eventBus); + try { + await factory(load.api); + load.commit(); + } catch (error) { + load.discard(); + throw error; + } + time(`${extensionPath} factory`, "extensions"); + return extension; +} + async function loadExtension( extensionPath: string, cwd: string, @@ -514,10 +579,7 @@ async function loadExtension( return { extension: null, error: `Extension does not export a valid factory function: ${extensionPath}` }; } - const extension = createExtension(extensionPath, resolvedPath); - const api = createExtensionAPI(extension, runtime, cwd, eventBus); - await factory(api); - time(`${extensionPath} factory`, "extensions"); + const extension = await initializeExtension(factory, extensionPath, resolvedPath, cwd, eventBus, runtime); return { extension, error: null }; } catch (err) { @@ -536,12 +598,8 @@ export async function loadExtensionFromFactory( runtime: ExtensionRuntime, extensionPath = "", ): Promise { - const extension = createExtension(extensionPath, extensionPath); const resolvedCwd = resolvePath(cwd); - const api = createExtensionAPI(extension, runtime, resolvedCwd, eventBus); - await factory(api); - time(`${extensionPath} factory`, "extensions"); - return extension; + return initializeExtension(factory, extensionPath, extensionPath, resolvedCwd, eventBus, runtime); } /** diff --git a/packages/coding-agent/test/suite/regressions/8423-extension-factory-failure.test.ts b/packages/coding-agent/test/suite/regressions/8423-extension-factory-failure.test.ts new file mode 100644 index 000000000..e6511cbb6 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/8423-extension-factory-failure.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it } from "vitest"; +import { createEventBus } from "../../../src/core/event-bus.ts"; +import { createExtensionRuntime, loadExtensionFromFactory } from "../../../src/core/extensions/loader.ts"; +import type { ExtensionAPI, ProviderConfig } from "../../../src/core/extensions/types.ts"; + +const providerConfig = { + baseUrl: "https://provider.test/v1", + apiKey: "provider-test-key", +} satisfies ProviderConfig; + +describe("issue #8423 extension factory failure", () => { + it("discards runtime changes and disables the failed API", async () => { + const runtime = createExtensionRuntime(); + const eventBus = createEventBus(); + let capturedApi: ExtensionAPI | undefined; + let eventCalls = 0; + let flagDuringLoad: boolean | string | undefined; + + await loadExtensionFromFactory( + (pi) => pi.registerProvider("working-provider", providerConfig), + process.cwd(), + eventBus, + runtime, + "", + ); + await expect( + loadExtensionFromFactory( + (pi) => { + capturedApi = pi; + pi.events.on("factory-failure", () => { + eventCalls++; + }); + pi.registerFlag("failed-flag", { type: "boolean", default: true }); + flagDuringLoad = pi.getFlag("failed-flag"); + pi.unregisterProvider("working-provider"); + pi.registerProvider("failed-provider", providerConfig); + throw new Error("factory failed"); + }, + process.cwd(), + eventBus, + runtime, + "", + ), + ).rejects.toThrow("factory failed"); + + eventBus.emit("factory-failure", undefined); + expect(flagDuringLoad).toBe(true); + expect(runtime.flagValues.has("failed-flag")).toBe(false); + expect(runtime.pendingProviderRegistrations.map(({ name }) => name)).toEqual(["working-provider"]); + expect(eventCalls).toBe(0); + expect(capturedApi).toBeDefined(); + expect(() => capturedApi?.registerFlag("late-flag", { type: "boolean", default: true })).toThrow( + 'Extension "" failed to load and its API is no longer active.', + ); + }); + + it("does not discard a concurrently loaded factory's provider", async () => { + const runtime = createExtensionRuntime(); + const eventBus = createEventBus(); + let releaseFailure!: () => void; + const waitBeforeFailure = new Promise((resolve) => { + releaseFailure = resolve; + }); + const failingLoad = loadExtensionFromFactory( + async (pi) => { + pi.registerProvider("failed-provider", providerConfig); + await waitBeforeFailure; + throw new Error("factory failed"); + }, + process.cwd(), + eventBus, + runtime, + "", + ); + + await loadExtensionFromFactory( + (pi) => pi.registerProvider("working-provider", providerConfig), + process.cwd(), + eventBus, + runtime, + "", + ); + releaseFailure(); + + await expect(failingLoad).rejects.toThrow("factory failed"); + expect(runtime.pendingProviderRegistrations.map(({ name }) => name)).toEqual(["working-provider"]); + }); +});