diff --git a/docs/plugin-manager-installer-plumbing.md b/docs/plugin-manager-installer-plumbing.md index a9329bf91..a2f69a709 100644 --- a/docs/plugin-manager-installer-plumbing.md +++ b/docs/plugin-manager-installer-plumbing.md @@ -109,7 +109,7 @@ Malformed `package.json` JSON is a hard failure at read time; malformed manifest - `[a,b]`: validates each feature exists in manifest features map - `[]`: empty feature list - bare spec: `null` (use defaults policy later in loader) -7. Validate declared extension entries (`#validateInstalledExtensions`): each manifest `extensions` entry must resolve on disk and import to a factory function. On failure, roll back the install — restore the previous `plugins/package.json`, remove the freshly installed package, and restore any prior version from a backup taken before `bun install` — then abort. +7. Validate declared extension entries (`#validateInstalledExtensions`): each manifest `extensions` entry must resolve on disk, import to a factory function, and initialize successfully against a throwaway registration surface. On failure, roll back the install — restore the previous `plugins/package.json`, remove the freshly installed package, and restore any prior version from a backup taken before `bun install` — then abort. 8. Upsert lockfile runtime state: `{ version, enabledFeatures, enabled: true }`. ### Update semantics diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a4d66e8d2..279ed138a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the legacy TypeBox facade rejecting `Type.Optional(Type.Unsafe(...))`, losing optional object properties when raw schemas were present, and plugin installs accepting extension factories that fail during initialization ([#8143](https://github.com/can1357/oh-my-pi/issues/8143)). + ## [17.2.12] - 2026-08-08 ### Fixed diff --git a/packages/coding-agent/src/extensibility/legacy-typebox.ts b/packages/coding-agent/src/extensibility/legacy-typebox.ts index 7270aaebd..50ff0b6c7 100644 --- a/packages/coding-agent/src/extensibility/legacy-typebox.ts +++ b/packages/coding-agent/src/extensibility/legacy-typebox.ts @@ -1,9 +1,5 @@ -import { - type ObjectOpts, - Type as OmpType, - type TypeBuilder as OmpTypeBuilder, - type TUnsafe, -} from "@oh-my-pi/omptype/typebox"; +import { fromJsonSchema } from "@oh-my-pi/omptype"; +import { Type as OmpType, type TypeBuilder as OmpTypeBuilder, type TUnsafe } from "@oh-my-pi/omptype/typebox"; import { upgradeJsonSchemaTo202012, validateJsonSchemaValue } from "@oh-my-pi/pi-ai/utils/schema"; export * from "@oh-my-pi/omptype/typebox"; @@ -25,11 +21,14 @@ interface SafeParseFailure { error: ValidationFailure; } -type LegacyUnsafeSchema = Record & - TUnsafe & { - __validator(data: unknown): T | ValidationFailure; - safeParse(input: unknown): SafeParseSuccess | SafeParseFailure; - }; +type LegacyUnsafeSchema = TUnsafe & { + __validator(data: unknown): T | ValidationFailure; + safeParse(input: unknown): SafeParseSuccess | SafeParseFailure; +}; + +function isValidationFailure(result: T | ValidationFailure): result is ValidationFailure { + return typeof result === "object" && result !== null && VALIDATION_FAILURE in result; +} function defineHidden(target: object, key: PropertyKey, value: unknown): void { Object.defineProperty(target, key, { @@ -40,8 +39,8 @@ function defineHidden(target: object, key: PropertyKey, value: unknown): void { } function unsafe(jsonSchema: Record = {}): LegacyUnsafeSchema { - const schema = { ...jsonSchema } as LegacyUnsafeSchema; - const upgradedSchema = upgradeJsonSchemaTo202012(jsonSchema); + const document = { ...jsonSchema }; + const upgradedSchema = upgradeJsonSchemaTo202012(document); const validate = (data: unknown): T | ValidationFailure => { const result = validateJsonSchemaValue(upgradedSchema, data); if (result.success) return data as T; @@ -54,47 +53,20 @@ function unsafe(jsonSchema: Record = {}): LegacyUn defineHidden(failure, VALIDATION_FAILURE, true); return failure; }; + const schema = fromJsonSchema(upgradedSchema).narrow((data, ctx) => { + const result = validate(data); + return isValidationFailure(result) ? ctx.mustBe(result.message) : true; + }) as unknown as LegacyUnsafeSchema; + defineHidden(schema, "toJsonSchema", () => document); defineHidden(schema, "__validator", validate); defineHidden(schema, "safeParse", (input: unknown): SafeParseSuccess | SafeParseFailure => { const result = validate(input); - return typeof result === "object" && result !== null && VALIDATION_FAILURE in result - ? { success: false, error: result } - : { success: true, data: result as T }; + return isValidationFailure(result) ? { success: false, error: result } : { success: true, data: result }; }); return schema; } -const object = ((properties: Record, opts?: ObjectOpts) => { - let hasRawProperty = false; - for (const key in properties) { - if (typeof properties[key] !== "function") { - hasRawProperty = true; - break; - } - } - if (!hasRawProperty) return OmpType.Object(properties as Parameters[0], opts); - - const propertySchemas: Record = {}; - const required: string[] = []; - for (const key in properties) { - const property = properties[key]; - propertySchemas[key] = - typeof property === "function" && "toJsonSchema" in property - ? (property as { toJsonSchema(): Record }).toJsonSchema() - : property; - required.push(key); - } - const document: Record = { type: "object", properties: propertySchemas, required }; - if (opts?.additionalProperties !== undefined) { - document.additionalProperties = - typeof opts.additionalProperties === "function" - ? opts.additionalProperties.toJsonSchema() - : opts.additionalProperties; - } - return unsafe(document); -}) as typeof OmpType.Object; - -export const Type: OmpTypeBuilder = { ...OmpType, Object: object, Unsafe: unsafe } as unknown as OmpTypeBuilder; +export const Type = { ...OmpType, Unsafe: unsafe } as unknown as OmpTypeBuilder; export type TypeBuilder = OmpTypeBuilder; const legacyTypeBox: { Type: OmpTypeBuilder } = { Type }; diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 564f31a60..e4959e20f 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -11,10 +11,9 @@ import { isEnoent, logger, } from "@oh-my-pi/pi-utils"; -import { withHostGuard } from "../utils"; +import { loadExtensions } from "../extensions/loader"; import { refreshBunGitCache } from "./bun-git-cache"; import { type GitSource, parseGitUrl } from "./git-url"; -import { installLegacyPiSpecifierShim, loadLegacyPiModule } from "./legacy-pi-compat"; import { resolvePluginManifestEntries } from "./loader"; import { getInstalledPluginsRegistryPath, readInstalledPluginsRegistry } from "./marketplace/registry"; import { parsePluginId } from "./marketplace/types"; @@ -95,14 +94,6 @@ function findGitPackageName(source: GitSource, deps: Record): st return undefined; } -function hasDefaultExport(value: unknown): value is { default?: unknown } { - return typeof value === "object" && value !== null && "default" in value; -} - -function hasExtensionFactoryExport(module: unknown): boolean { - return typeof module === "function" || (hasDefaultExport(module) && typeof module.default === "function"); -} - interface PluginPackageSnapshot { readonly actualName: string; readonly packagePath: string; @@ -372,17 +363,9 @@ export class PluginManager { } if (loadable.length > 0) { - installLegacyPiSpecifierShim(); - for (const extensionPath of loadable) { - try { - const module = await withHostGuard(() => loadLegacyPiModule(extensionPath)); - if (!hasExtensionFactoryExport(module)) { - errors.push(`${extensionPath}: extension does not export a valid factory function`); - } - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - errors.push(`${extensionPath}: ${message}`); - } + const result = await loadExtensions(loadable, this.#cwd); + for (const failure of result.errors) { + errors.push(`${failure.path}: ${failure.error}`); } } diff --git a/packages/coding-agent/test/extensibility/typebox-shim.test.ts b/packages/coding-agent/test/extensibility/typebox-shim.test.ts index ade6fc715..4ad5f877d 100644 --- a/packages/coding-agent/test/extensibility/typebox-shim.test.ts +++ b/packages/coding-agent/test/extensibility/typebox-shim.test.ts @@ -59,6 +59,25 @@ describe("pi.typebox compatibility shim", () => { ).toThrow('Validation failed for tool "unsafe-schema"'); }); + it("composes optional Type.Unsafe schemas without making sibling properties required", () => { + const schema = Type.Object({ + kind: Type.Unsafe({ type: "string", enum: ["question"] }), + mode: Type.Optional(Type.Unsafe({ type: "string", enum: ["overlay", "inline"] })), + label: Type.Optional(Type.String()), + }); + + const document = schema.toJsonSchema(); + expect(document.required).toEqual(["kind"]); + expect(document).toMatchObject({ + properties: { + mode: { type: "string", enum: ["overlay", "inline"] }, + label: { type: "string" }, + }, + }); + expect(schema.safeParse({ kind: "question", mode: "overlay" }).success).toBe(true); + expect(schema.safeParse({ kind: "question", mode: "other" }).success).toBe(false); + }); + it("validates Type.Unsafe draft-07 documents like the wire path", () => { const schema = Type.Unsafe({ type: "object", diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts index 1621954ce..e4efcd125 100644 --- a/packages/coding-agent/test/plugin-install-validation.test.ts +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -126,6 +126,48 @@ describe("PluginManager.install load validation", () => { expect(result.path).toBe(path.join(pluginsNodeModules, "pi-figma-remote-auth")); }); + test("rejects and rolls back an install when the extension factory throws", async () => { + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd).toEqual(["bun", "install", "factory-failure-plugin"]); + + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { + name: "omp-plugins", + private: true, + dependencies: { "factory-failure-plugin": "1.0.0" }, + }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "factory-failure-plugin", { + version: "1.0.0", + source: 'export default function() { throw new Error("factory-time failure"); }\n', + }); + })(); + + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + await expect(new PluginManager(tmpRoot).install("factory-failure-plugin")).rejects.toThrow( + /factory-time failure/, + ); + + const pluginsPackage = await Bun.file(pluginsPkgJson).json(); + expect(pluginsPackage.dependencies ?? {}).toEqual({}); + expect(await Bun.file(path.join(pluginsNodeModules, "factory-failure-plugin", "package.json")).exists()).toBe( + false, + ); + }); + test("rejects an install whose extension entry cannot resolve its dependencies", async () => { vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { expect(cmd).toEqual(["bun", "install", "broken-plugin"]);