From 6833070a570ffba52116a6c25e3a4f0127ecb551 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Sat, 9 May 2026 15:08:55 -0400 Subject: [PATCH] fix(server): project /config response to JSON-safe at the HTTP boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors #26550 (Provider.toPublicInfo): internal runtime objects may legitimately carry function/symbol/bigint/undefined values, but the HTTP API contract is JSON-safe data matching the typed schema. Plugin `config:` hooks are documented to mutate cfg in place (registering agents/providers); that contract is preserved. The projection happens in ConfigHttpApi.get only, so internal consumers keep raw cfg and HTTP clients see a clean response. Active reproducer asserts the actual product invariant — GET /config returns a JSON-safe body even when a plugin attached a function to cfg. Three trigger-output reproducers stay skipped: per-hook design needed because the documented mutation contract makes a blanket clone unsafe and `tool.parameters` is a live Effect Schema with function-valued own keys that recursive in-place scrub would corrupt. --- .../instance/httpapi/handlers/config.ts | 7 +- packages/opencode/src/util/json-safe.ts | 9 + .../test/plugin/mutation-repro.test.ts | 232 ++++++++++++++++++ 3 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 packages/opencode/src/util/json-safe.ts create mode 100644 packages/opencode/test/plugin/mutation-repro.test.ts diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/config.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/config.ts index 3d0e8a06c0..7d8cc09edd 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/config.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/handlers/config.ts @@ -1,6 +1,7 @@ import { Config } from "@/config/config" import { Provider } from "@/provider/provider" import * as InstanceState from "@/effect/instance-state" +import { toJsonSafe } from "@/util/json-safe" import { Effect } from "effect" import { HttpApiBuilder } from "effect/unstable/httpapi" import { InstanceHttpApi } from "../api" @@ -12,7 +13,11 @@ export const configHandlers = HttpApiBuilder.group(InstanceHttpApi, "config", (h const configSvc = yield* Config.Service const get = Effect.fn("ConfigHttpApi.get")(function* () { - return yield* configSvc.get() + // Plugin `config` hooks may attach non-JSON-safe values (function, + // symbol, undefined, bigint) to the live config. Project a JSON-safe + // copy at the HTTP boundary so the response matches the typed schema + // (mirrors Provider.toPublicInfo in #26550). + return toJsonSafe(yield* configSvc.get()) }) const update = Effect.fn("ConfigHttpApi.update")(function* (ctx) { diff --git a/packages/opencode/src/util/json-safe.ts b/packages/opencode/src/util/json-safe.ts new file mode 100644 index 0000000000..1ebb4b6d8f --- /dev/null +++ b/packages/opencode/src/util/json-safe.ts @@ -0,0 +1,9 @@ +export function toJsonSafe(value: T): T { + return JSON.parse( + JSON.stringify(value, (_, v) => { + if (typeof v === "function" || typeof v === "symbol" || v === undefined) return undefined + if (typeof v === "bigint") return v.toString() + return v + }), + ) +} diff --git a/packages/opencode/test/plugin/mutation-repro.test.ts b/packages/opencode/test/plugin/mutation-repro.test.ts new file mode 100644 index 0000000000..fa3ebbbb4d --- /dev/null +++ b/packages/opencode/test/plugin/mutation-repro.test.ts @@ -0,0 +1,232 @@ +/** + * Reproducer tests for plugin-hook mutation bugs analogous to #26546 + * (gemini-auth-plugin / `toPublicInfo`, fixed by #26550). + * + * Status: + * - Finding 4 (plugin.config): FIXED in this PR by projecting cfg + * through `toJsonSafe` at the HTTP boundary in + * `ConfigHttpApi.get`. The active reproducer below asserts the + * HTTP `/config` response, not Config.Service.get(); internal cfg + * is allowed to carry whatever plugins put there, the public API + * is the contract that matters. + * - Findings 1-3 (tool.definition, messages.transform, system.transform): + * reproducers SKIPPED. The `output`-mutation contract is documented + * public API (see packages/web/src/content/docs/plugins.mdx); blanket + * clone-and-return breaks plugins that mutate by reference, and + * blanket in-place scrub corrupts shared Effect Schema instances + * in `tool.parameters`. Per-hook design needed. + */ +import { afterAll, describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" +import path from "path" +import { pathToFileURL } from "url" +import { ModelID, ProviderID } from "../../src/provider/schema" +import { provideTmpdirInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" + +const disableDefault = process.env.OPENCODE_DISABLE_DEFAULT_PLUGINS +process.env.OPENCODE_DISABLE_DEFAULT_PLUGINS = "1" + +const { Plugin } = await import("../../src/plugin/index") +const { Config } = await import("../../src/config/config") +const { Server } = await import("../../src/server/server") + +const it = testEffect(Layer.mergeAll(Plugin.defaultLayer, Config.defaultLayer, CrossSpawnSpawner.defaultLayer)) + +afterAll(() => { + if (disableDefault === undefined) { + delete process.env.OPENCODE_DISABLE_DEFAULT_PLUGINS + return + } + process.env.OPENCODE_DISABLE_DEFAULT_PLUGINS = disableDefault +}) + +function withProject(source: string, self: Effect.Effect | ((dir: string) => Effect.Effect)) { + return provideTmpdirInstance((dir) => + Effect.gen(function* () { + const file = path.join(dir, "plugin.ts") + yield* Effect.all( + [ + Effect.promise(() => Bun.write(file, source)), + Effect.promise(() => + Bun.write( + path.join(dir, "opencode.json"), + JSON.stringify( + { + $schema: "https://opencode.ai/config.json", + plugin: [pathToFileURL(file).href], + }, + null, + 2, + ), + ), + ), + ], + { discard: true, concurrency: 2 }, + ) + return yield* typeof self === "function" ? self(dir) : self + }), + ) +} + +/** True if `value` contains no function values anywhere in its (mutable, + * enumerable) tree — i.e. survives a round-trip through JSON.stringify + * without silently losing fields. */ +function isJsonSafe(value: unknown, seen = new WeakSet()): boolean { + if (typeof value === "function") return false + if (value === null || typeof value !== "object") return true + if (seen.has(value as object)) return true + seen.add(value as object) + if (Array.isArray(value)) return value.every((v) => isJsonSafe(v, seen)) + for (const key of Object.keys(value as Record)) { + if (!isJsonSafe((value as Record)[key], seen)) return false + } + return true +} + +describe("plugin hook mutation reproducers (analog of #26546)", () => { + // ------------------------------------------------------------------------- + // Finding 1: tool.definition — packages/opencode/src/tool/registry.ts:325 + // Plugin can attach function values to `output.parameters`; downstream LLM + // tool-use serialization drops them, producing malformed tool schemas. + // ------------------------------------------------------------------------- + it.live.skip("tool.definition: output stays JSON-safe after plugin mutation", () => + withProject( + [ + "export default async () => ({", + ' "tool.definition": (_input, output) => {', + " // Simulate a misbehaving plugin attaching a function value", + " // (e.g. trying to inject a custom validator, runtime hook, etc.)", + " output.parameters = {", + " ...((output.parameters as any) ?? {}),", + " __pluginFn: () => 'side effect',", + " }", + " },", + "})", + "", + ].join("\n"), + Effect.gen(function* () { + const plugin = yield* Plugin.Service + const output: { description: string; parameters: any } = { + description: "test tool", + parameters: { type: "object", properties: {} }, + } + yield* plugin.trigger("tool.definition", { toolID: "test_tool" }, output) + // Post-fix contract: opencode must hand the plugin a defensive + // copy and re-scrub the result, so function-valued mutations + // never leak back to the tool definition pipeline. + expect(isJsonSafe(output)).toBe(true) + }), + ), + ) + + // ------------------------------------------------------------------------- + // Finding 2: experimental.chat.messages.transform + // Call sites: session/prompt.ts:1566 and session/compaction.ts:407. + // The hook contract is the same; one test covers both. + // ------------------------------------------------------------------------- + it.live.skip("experimental.chat.messages.transform: output stays JSON-safe after plugin mutation", () => + withProject( + [ + "export default async () => ({", + ' "experimental.chat.messages.transform": (_input, output) => {', + " output.messages = [", + " ...output.messages,", + " // Plugin attaches a function in a message-shaped object;", + " // downstream MessageV2.toModelMessagesEffect / persistence", + " // will drop the field, corrupting message ordering or", + " // failing schema validation.", + " { id: 'plug', role: 'assistant', toolCall: () => 'oops' } as any,", + " ]", + " },", + "})", + "", + ].join("\n"), + Effect.gen(function* () { + const plugin = yield* Plugin.Service + const output = { messages: [] as any[] } + yield* plugin.trigger("experimental.chat.messages.transform", {}, output) + // Post-fix contract: messages handed back to the prompt / + // compaction pipeline must be JSON-safe. The same hook fires from + // both session/prompt.ts:1566 and session/compaction.ts:407. + expect(isJsonSafe(output)).toBe(true) + }), + ), + ) + + // ------------------------------------------------------------------------- + // Finding 3: experimental.chat.system.transform — agent/agent.ts:452 + // Plugin can attach function-valued entries to the system prompt array. + // ------------------------------------------------------------------------- + it.live.skip("experimental.chat.system.transform: output stays JSON-safe after plugin mutation", () => + withProject( + [ + "export default async () => ({", + ' "experimental.chat.system.transform": (_input, output) => {', + " // Plugin pushes a non-string value onto system. After JSON", + " // round-trip (e.g. provider request body), the function is", + " // dropped, leaving an empty / malformed prompt entry.", + " ;(output.system as any[]).push(() => 'dynamic prompt')", + " },", + "})", + "", + ].join("\n"), + Effect.gen(function* () { + const plugin = yield* Plugin.Service + const output: { system: unknown[] } = { system: ["initial"] } + yield* plugin.trigger( + "experimental.chat.system.transform", + { + model: { + providerID: ProviderID.anthropic, + modelID: ModelID.make("claude-sonnet-4-6"), + }, + }, + output, + ) + // Post-fix contract: the system prompt array opencode keeps + // forwarding to the LLM provider must contain only JSON-safe + // primitives. + expect(isJsonSafe(output)).toBe(true) + }), + ), + ) + + // ------------------------------------------------------------------------- + // Finding 4: plugin.config() — plugin/index.ts:235 + // The plugin's `config` hook receives the LIVE Config.Info object and can + // mutate it. Downstream `/config/get` and any JSON serialization drop the + // function-valued fields, but the in-memory state stays corrupted. + // ------------------------------------------------------------------------- + it.live("plugin.config(): GET /config response stays JSON-safe after plugin mutation", () => + withProject( + [ + "export default async () => ({", + " config: (cfg) => {", + " // Misbehaving plugin attaches a function-valued field to the", + " // shared config object. Internal state is allowed to carry it,", + " // but the HTTP API (and any other JSON boundary) must project", + " // it out so callers see the typed schema.", + " ;(cfg as any).__pluginFn = () => 'mutated'", + " },", + "})", + "", + ].join("\n"), + (dir) => + Effect.gen(function* () { + const plugin = yield* Plugin.Service + yield* plugin.init() + const headers = { "x-opencode-directory": dir } + const response = yield* Effect.promise(() => Promise.resolve(Server.Default().app.request("/config", { headers }))) + expect(response.status).toBe(200) + const body = (yield* Effect.promise(() => response.json())) as Record + // Post-fix contract: HTTP /config is the boundary. Whatever the + // live in-memory cfg looks like, the wire response must be + // JSON-safe and match the typed schema. + expect(isJsonSafe(body)).toBe(true) + expect("__pluginFn" in body).toBe(false) + }), + ), + ) +})