From daae0e48e0251499a407802e0379b9085a6b1e9b Mon Sep 17 00:00:00 2001 From: Anton Date: Sat, 3 Oct 2026 23:29:50 +0300 Subject: [PATCH] fix(config): name the thresholds that are actually in force MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #631 ## Problem The report describes a day of work lost to a config typo: `modelMaxLimits` / `modelMinLimits` placed at the top level and per-model entries written as `compress["provider/model"]`, neither of which is in the schema. The plugin printed one `Unknown keys` line, then ran on the silent fallback of 100% of `model.limit.context`, so nudges fired at ~30k instead of the intended ~76k. Two things made that undiagnosable: 1. **The warning is emitted before the merge.** `showConfigWarnings` was called per config layer, *before* `mergeLayer` ran for that layer, so the effective values simply did not exist yet. This is why the warning could not name them. 2. **The thresholds are never logged anywhere.** `DCP initialized` logged only `strategies`. Since the failure mode of a schema mismatch is *wrong thresholds* rather than an error, and `getConfig` runs once at startup, there was no way to see what the plugin had decided without attaching a debugger. ## Change - `showConfigWarnings` now takes collected per-layer diagnostics and the **merged** config, and is called once at the end of `getConfig`. The toast therefore lists the thresholds that are in force, not the ones the offending layer tried to set. It also says the config is read once and OpenCode must be restarted, which was the other half of the report. - `describeEffectiveThresholds(config)` renders absolute limits as tokens and percentage limits as `"35%" of the model context window`. Percentages cannot be evaluated until the host reports the window, so the configured form is what the user needs to see. - `DCP initialized` now logs `thresholds` and the restart note, so the resolved values are in the debug log from the first line. ## Where the helper lives `describeEffectiveThresholds` moved to a new `lib/thresholds.ts`. `config.ts` imports `jsonc-parser/lib/esm/main.js`, whose published ESM entry has no named `parse` export — that is why `tsup.config.ts` already bundles `jsonc-parser`, and why no test could ever import `config.ts` at runtime (every existing test uses `import type`, which is erased). A pure formatting helper has no business pulling in a JSON parser, so this makes it testable at all. ## Verification `tests/config-warning.test.ts`, seven cases. All fail on the current implementation (the file cannot even load, since the helper does not exist): - absolute thresholds render as tokens - percentage thresholds say what they are relative to - a mixed configuration reports each form separately - the description uses the real schema keys and not the misplaced ones - `DCP initialized` logs the thresholds and the restart note - the warning is emitted *after* the merge and is no longer emitted per layer - the helper stays importable without the JSONC parser Full suite 131 passing, `tsc --noEmit` clean, `tsup` + declarations build, `prettier --check` clean. `verify-package.mjs` still passes its file, package.json and import-graph checks. --- index.ts | 7 +++ lib/config.ts | 97 +++++++++++++++++++++-------------- lib/thresholds.ts | 29 +++++++++++ tests/config-warning.test.ts | 98 ++++++++++++++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 37 deletions(-) create mode 100644 lib/thresholds.ts create mode 100644 tests/config-warning.test.ts 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) +})