diff --git a/index.ts b/index.ts index 02216b33..035f2148 100644 --- a/index.ts +++ b/index.ts @@ -1,5 +1,6 @@ import type { Plugin } from "@opencode-ai/plugin" import { getConfig } from "./lib/config" +import { describeEffectiveThresholds } from "./lib/thresholds" import { createCompressMessageTool, createCompressRangeTool } from "./lib/compress" import { compressDisabledByOpencode, @@ -40,8 +41,14 @@ const server: Plugin = (async (ctx) => { // logger.info("Secure mode detected, configured client authentication") } + // The config is read once, here. Logging the thresholds that are actually in + // force is the only way to tell a typo in dcp.jsonc from intended settings, + // because a schema mismatch degrades to wrong thresholds rather than failing + // loudly (#631). logger.info("DCP initialized", { strategies: config.strategies, + thresholds: describeEffectiveThresholds(config), + note: "config is read once at startup; restart OpenCode after editing it", }) startAutoUpdate(ctx, config.autoUpdate) diff --git a/lib/config.ts b/lib/config.ts index 792b7ca3..b2d26347 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -1,7 +1,8 @@ -import { readFileSync, writeFileSync, existsSync, mkdirSync, statSync } from "fs" +import { readFileSync, writeFileSync, existsSync, mkdirSync, statSync } from "fs" import { join, dirname } from "path" import { homedir } from "os" import { parse } from "jsonc-parser/lib/esm/main.js" +import { CONFIG_REQUIRES_RESTART_NOTE, describeEffectiveThresholds } from "./thresholds" import type { PluginInput } from "@opencode-ai/plugin" type ConfigContext = Pick & { @@ -618,49 +619,56 @@ export function validateConfigTypes(config: Record): ValidationErro return errors } +interface ConfigLayerDiagnostics { + configPath: string + isProject: boolean + invalidKeys: string[] + typeErrors: ValidationError[] +} + +function formatLayerDiagnostics(diagnostics: ConfigLayerDiagnostics[]): string[] { + const messages: string[] = [] + + for (const layer of diagnostics) { + const parts: string[] = [] + if (layer.invalidKeys.length > 0) { + const keyList = layer.invalidKeys.slice(0, 3).join(", ") + const suffix = + layer.invalidKeys.length > 3 ? ` (+${layer.invalidKeys.length - 3} more)` : "" + parts.push(`Unknown keys: ${keyList}${suffix}`) + } + for (const err of layer.typeErrors.slice(0, 2)) { + parts.push(`${err.key}: expected ${err.expected}, got ${err.actual}`) + } + if (layer.typeErrors.length > 2) { + parts.push(`(+${layer.typeErrors.length - 2} more type errors)`) + } + if (parts.length > 0) { + messages.push(`${layer.configPath}\n${parts.join("\n")}`) + } + } + + return messages +} + function showConfigWarnings( ctx: ConfigContext, - configPath: string, - configData: Record, - isProject: boolean, + diagnostics: ConfigLayerDiagnostics[], + config: PluginConfig, ): void { - const invalidKeys = getInvalidConfigKeys(configData) - const typeErrors = validateConfigTypes(configData) - - if (invalidKeys.length === 0 && typeErrors.length === 0) { + if (diagnostics.length === 0) { return } + const isProject = diagnostics.some((layer) => layer.isProject) const configType = isProject ? "project config" : "config" - const messages: string[] = [] - - if (invalidKeys.length > 0) { - const keyList = invalidKeys.slice(0, 3).join(", ") - const suffix = invalidKeys.length > 3 ? ` (+${invalidKeys.length - 3} more)` : "" - messages.push(`Unknown keys: ${keyList}${suffix}`) - } - - if (typeErrors.length > 0) { - for (const err of typeErrors.slice(0, 2)) { - messages.push(`${err.key}: expected ${err.expected}, got ${err.actual}`) - } - if (typeErrors.length > 2) { - messages.push(`(+${typeErrors.length - 2} more type errors)`) - } - } + const body = [ + ...formatLayerDiagnostics(diagnostics), + describeEffectiveThresholds(config), + CONFIG_REQUIRES_RESTART_NOTE, + ].join("\n\n") - setTimeout(() => { - try { - ctx.client.tui.showToast({ - body: { - title: `DCP: ${configType} warning`, - message: `${configPath}\n${messages.join("\n")}`, - variant: "warning", - duration: 7000, - }, - }) - } catch {} - }, 7000) + scheduleParseWarning(ctx, `DCP: ${configType} warning`, body) } const defaultConfig: PluginConfig = { @@ -989,6 +997,7 @@ export function getConfig(ctx: ConfigContext): PluginConfig { { path: configPaths.configDir, name: "configDir config", isProject: true }, { path: configPaths.project, name: "project config", isProject: true }, ] + const diagnostics: ConfigLayerDiagnostics[] = [] for (const layer of layers) { if (!layer.path) { @@ -1009,9 +1018,23 @@ export function getConfig(ctx: ConfigContext): PluginConfig { continue } - showConfigWarnings(ctx, layer.path, result.data, layer.isProject) + const invalidKeys = getInvalidConfigKeys(result.data) + const typeErrors = validateConfigTypes(result.data) + if (invalidKeys.length > 0 || typeErrors.length > 0) { + diagnostics.push({ + configPath: layer.path, + isProject: layer.isProject, + invalidKeys, + typeErrors, + }) + } + config = mergeLayer(config, result.data) } + // Emitted after the merge, so the warning can name the thresholds that are + // actually in force rather than the ones the offending layer tried to set. + showConfigWarnings(ctx, diagnostics, config) + return config } diff --git a/lib/thresholds.ts b/lib/thresholds.ts new file mode 100644 index 00000000..6cf91b59 --- /dev/null +++ b/lib/thresholds.ts @@ -0,0 +1,29 @@ +import type { PluginConfig } from "./config" + +// A percentage threshold cannot be turned into a token count until the host +// reports the model context window, so the configured form is what the user needs +// to see. A warning that lists only the offending keys leaves the most damaging +// failure mode - wrong thresholds rather than an error - invisible (#631). +// +// This lives apart from config.ts so it stays importable without pulling in the +// JSONC parser, whose published ESM entry has no named `parse` export and is only +// usable once tsup bundles it. +export function describeEffectiveThresholds(config: PluginConfig): string { + const describe = (value: unknown): string => { + if (typeof value === "number") { + return `${value} tokens` + } + if (typeof value === "string") { + return value.endsWith("%") ? `"${value}" of the model context window` : `"${value}"` + } + return String(value) + } + + return ( + `Effective thresholds: compress.minContextLimit=${describe(config.compress.minContextLimit)}, ` + + `compress.maxContextLimit=${describe(config.compress.maxContextLimit)}` + ) +} + +export const CONFIG_REQUIRES_RESTART_NOTE = + "Config is read once at startup - restart OpenCode after editing it." diff --git a/tests/config-warning.test.ts b/tests/config-warning.test.ts new file mode 100644 index 00000000..58e4900c --- /dev/null +++ b/tests/config-warning.test.ts @@ -0,0 +1,98 @@ +import assert from "node:assert/strict" +import test from "node:test" +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs" +import { join } from "node:path" +import { tmpdir } from "node:os" +import type { PluginConfig } from "../lib/config" + +const root = mkdtempSync(join(tmpdir(), "dcp-config-warning-")) +process.env.XDG_CONFIG_HOME = join(root, "config") +process.env.OPENCODE_CONFIG_DIR = join(root, "config", "opencode") +process.env.XDG_DATA_HOME = join(root, "data") +test.after(() => rmSync(root, { recursive: true, force: true })) + +const { describeEffectiveThresholds, CONFIG_REQUIRES_RESTART_NOTE } = + await import("../lib/thresholds") + +function configWith(overrides: Record): PluginConfig { + return { + compress: { + minContextLimit: 50000, + maxContextLimit: 100000, + ...overrides, + }, + } as unknown as PluginConfig +} + +// A percentage cannot be turned into a token count until the host reports the +// model context window. Reporting only "Unknown keys" left the user unable to +// tell that their thresholds had silently fallen back to a different value. +test("absolute thresholds are reported in tokens", () => { + const line = describeEffectiveThresholds(configWith({})) + assert.match(line, /compress\.minContextLimit=50000 tokens/) + assert.match(line, /compress\.maxContextLimit=100000 tokens/) +}) + +test("percentage thresholds say what they are relative to", () => { + const line = describeEffectiveThresholds( + configWith({ minContextLimit: "35%", maxContextLimit: "60%" }), + ) + assert.match(line, /compress\.minContextLimit="35%" of the model context window/) + assert.match(line, /compress\.maxContextLimit="60%" of the model context window/) +}) + +test("a mixed configuration reports each form separately", () => { + const line = describeEffectiveThresholds( + configWith({ minContextLimit: "35%", maxContextLimit: 100000 }), + ) + assert.match(line, /minContextLimit="35%" of the model context window/) + assert.match(line, /maxContextLimit=100000 tokens/) +}) + +test("the description names the keys the schema actually uses", () => { + // #631's reporter put modelMaxLimits/modelMinLimits at the top level and + // per-model entries under compress["provider/model"], which is not the + // schema. The message has to point at the real keys. + const line = describeEffectiveThresholds(configWith({})) + assert.doesNotMatch(line, /modelMaxLimits/) + assert.doesNotMatch(line, /modelMinLimits/) + assert.match(line, /^Effective thresholds: /) +}) + +test("the plugin logs the thresholds it is running with", async () => { + const source = await import("node:fs").then((fs) => + fs.readFileSync(join(import.meta.dirname, "..", "index.ts"), "utf8"), + ) + assert.match(source, /thresholds: describeEffectiveThresholds\(config\)/) + assert.match(source, /restart OpenCode after editing it/) +}) + +test("the config warning mentions the restart requirement", async () => { + assert.match( + CONFIG_REQUIRES_RESTART_NOTE, + /Config is read once at startup - restart OpenCode after editing it\./, + ) + const source = await import("node:fs").then((fs) => + fs.readFileSync(join(import.meta.dirname, "..", "lib", "config.ts"), "utf8"), + ) + assert.match(source, /CONFIG_REQUIRES_RESTART_NOTE/) + // The warning is emitted after the merge, so it can report what is in force. + const mergeIndex = source.indexOf("config = mergeLayer(config, result.data)") + const warnIndex = source.indexOf("showConfigWarnings(ctx, diagnostics, config)") + assert.ok(mergeIndex > 0 && warnIndex > mergeIndex, "warning must be emitted after the merge") + // Diagnostics are collected per layer rather than warned per layer. + assert.doesNotMatch(source, /showConfigWarnings\(ctx, layer\.path/) +}) + +test("thresholds stay importable without the JSONC parser", async () => { + // config.ts pulls in jsonc-parser, whose published ESM entry has no named + // `parse` export and only works once tsup bundles it. Keeping the helper in + // its own module is what makes it testable at all. + const source = await import("node:fs").then((fs) => + fs.readFileSync(join(import.meta.dirname, "..", "lib", "thresholds.ts"), "utf8"), + ) + assert.doesNotMatch(source, /jsonc-parser/) + // A type-only import of PluginConfig is erased at runtime and is fine; a + // value import would drag the parser back in. + assert.doesNotMatch(source, /^import \{[^}]*\} from "\.\/config"$/m) +})