Merge PR #8144: fix(extensions): preserve unsafe schemas during plugin install (@roboomp)

2026-08-13 01:14:49 +02:00
parent 420998f821 05aea7e8b7
commit 00cabe5513
10 changed files with 258 additions and 51 deletions
@@ -68,10 +68,43 @@ describe("legacy-pi TypeBox remap", () => {
 		};

 		expect(loaded.probe).toBe(TypeBoxShimType);
-		expect(toolWireSchema({ name: "fixture", description: "", parameters: loaded.schema })).toEqual({
+		// `Type.Unsafe` is now a first-class omptype schema (so `Type.Optional`/
+		// `Type.Object` can compose it), so a top-level Unsafe tool param takes the
+		// omptype wire path and is closed like every other tool param. Compare the
+		// JSON-serialized wire — internal memoization stamps are non-serialized.
+		const wire = toolWireSchema({ name: "fixture", description: "", parameters: loaded.schema });
+		expect(JSON.parse(JSON.stringify(wire))).toEqual({
 			type: "object",
 			properties: { path: { type: "string" } },
 			required: ["path"],
+			additionalProperties: false,
+		});
+	});
+
+	it("preserves raw JSON Schema properties passed directly to Type.Object", async () => {
+		const entry = await writeFixtureExtension(
+			[
+				'import { Type } from "typebox";',
+				"export const schema = Type.Object({ cfg: { type: 'string', pattern: '^ok' }, label: Type.Optional(Type.String()) });",
+			].join("\n"),
+		);
+
+		const loaded = (await loadLegacyPiModule(entry)) as {
+			schema: Record<string, unknown> & { safeParse(input: unknown): { success: boolean } };
+		};
+
+		expect(loaded.schema.safeParse({ cfg: "okay" }).success).toBe(true);
+		expect(loaded.schema.safeParse({ cfg: "bad" }).success).toBe(false);
+		expect(loaded.schema.safeParse({ cfg: { type: "string" } }).success).toBe(false);
+		const wire = toolWireSchema({ name: "fixture", description: "", parameters: loaded.schema });
+		expect(JSON.parse(JSON.stringify(wire))).toEqual({
+			type: "object",
+			properties: {
+				cfg: { type: "string", pattern: "^ok" },
+				label: { type: "string" },
+			},
+			required: ["cfg"],
+			additionalProperties: false,
 		});
 	});

@@ -59,6 +59,53 @@ 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("preserves nested Type.Unsafe keywords fromJsonSchema cannot lower", () => {
+		const raw = Type.Unsafe({
+			type: "object",
+			properties: { a: { type: "string" } },
+			patternProperties: { "^x-": { type: "number" } },
+			additionalProperties: false,
+		});
+		const nested = {
+			type: "object",
+			properties: { a: { type: "string" } },
+			patternProperties: { "^x-": { type: "number" } },
+			additionalProperties: false,
+		};
+
+		// The wire schema must keep patternProperties even when the Unsafe schema
+		// is embedded inside Type.Object / Type.Optional — a `.toJsonSchema`
+		// method override would vanish at these nested positions.
+		expect((Type.Object({ cfg: raw }).toJsonSchema().properties as Record<string, unknown>).cfg).toEqual(nested);
+		expect(
+			(Type.Object({ cfg: Type.Optional(raw) }).toJsonSchema().properties as Record<string, unknown>).cfg,
+		).toEqual(nested);
+
+		// Runtime validation still enforces the nested keyword.
+		const object = Type.Object({ cfg: raw });
+		expect(object.safeParse({ cfg: { a: "ok", "x-n": 3 } }).success).toBe(true);
+		expect(object.safeParse({ cfg: { a: "ok", "x-n": "bad" } }).success).toBe(false);
+	});
+
 	it("validates Type.Unsafe draft-07 documents like the wire path", () => {
 		const schema = Type.Unsafe({
 			type: "object",
@@ -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"]);