From 49a9bca9f6d68e22c76e3bad58ef78625194a102 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Tue, 21 Jul 2026 14:49:57 +0800 Subject: [PATCH] fix: validate compaction config pairs --- docs/config-catalog.md | 2 +- packages/compact/compact-basic/src/config.ts | 26 +++++++--- .../compact-basic/tests/compact-basic.spec.ts | 51 ++++++++++++++++++- .../tests/compact-loop-repro.spec.ts | 1 - .../tests/loader-composition.spec.ts | 17 ++++++- packages/llm/token-meter/src/index.ts | 4 +- packages/llm/token-meter/src/types.ts | 2 +- .../llm/token-meter/tests/token-meter.spec.ts | 9 +++- 8 files changed, 96 insertions(+), 16 deletions(-) diff --git a/docs/config-catalog.md b/docs/config-catalog.md index 55550cbe4a..28d7932fb0 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -1059,7 +1059,7 @@ Source: [`packages/context/time-context/src/index.ts:19`](../packages/context/ti ```ts config-catalog /** Token-meter plugin configuration; the fixed estimator has no settings. */ -export type TokenMeterConfig = object +export type TokenMeterConfig = Record ``` Source: [`packages/llm/token-meter/src/types.ts:10`](../packages/llm/token-meter/src/types.ts) diff --git a/packages/compact/compact-basic/src/config.ts b/packages/compact/compact-basic/src/config.ts index 90adb5b5f7..b954537af8 100644 --- a/packages/compact/compact-basic/src/config.ts +++ b/packages/compact/compact-basic/src/config.ts @@ -248,17 +248,29 @@ function validatePolicy( assertNonNegativeInteger(`${name}.maxOverflowRetries`, maxOverflowRetries) } - const summarizationProvider = config.summarizationProvider - const summarizationModel = config.summarizationModel - if (summarizationProvider !== undefined && typeof summarizationProvider !== 'string') { + validateSummarizationPair(config, name) +} + +/** Require one scope to omit, clear, or replace the summarization target as a pair. */ +function validateSummarizationPair( + config: CompactPolicyConfig | Record, + name: string, +): void { + const provider = config.summarizationProvider + const model = config.summarizationModel + if (provider !== undefined && typeof provider !== 'string') { throw new Error(`${name}.summarizationProvider must be a string`) } - if (summarizationModel !== undefined && typeof summarizationModel !== 'string') { + if (model !== undefined && typeof model !== 'string') { throw new Error(`${name}.summarizationModel must be a string`) } - if (((summarizationProvider ?? '').length === 0) - !== ((summarizationModel ?? '').length === 0)) { - throw new Error(`${name}: summarizationProvider and summarizationModel must both be empty or both be non-empty`) + if (provider === undefined && model === undefined) return + if (provider === undefined || model === undefined + || (provider.length === 0) !== (model.length === 0)) { + throw new Error( + `${name}: summarizationProvider and summarizationModel must be set together ` + + 'as an empty or non-empty pair', + ) } } diff --git a/packages/compact/compact-basic/tests/compact-basic.spec.ts b/packages/compact/compact-basic/tests/compact-basic.spec.ts index cd1ebd2bd1..e6cc897576 100644 --- a/packages/compact/compact-basic/tests/compact-basic.spec.ts +++ b/packages/compact/compact-basic/tests/compact-basic.spec.ts @@ -308,6 +308,41 @@ describe('compact configuration and defaults', () => { }) }) + it('inherits, clears, and replaces the summarization target as a pair', () => { + const config = resolveConfig({ + summarizationProvider: 'default-provider', + summarizationModel: 'default-model', + modelPolicies: [ + { provider: 'inherit-provider', model: MODEL }, + { + provider: 'clear-provider', + model: MODEL, + summarizationProvider: '', + summarizationModel: '', + }, + { + provider: 'replace-provider', + model: MODEL, + summarizationProvider: 'replacement-provider', + summarizationModel: 'replacement-model', + }, + ], + }) + + expect(resolveTargetPolicy(config, { provider: 'inherit-provider', model: MODEL })) + .toMatchObject({ + summarizationProvider: 'default-provider', + summarizationModel: 'default-model', + }) + expect(resolveTargetPolicy(config, { provider: 'clear-provider', model: MODEL })) + .toMatchObject({ summarizationProvider: '', summarizationModel: '' }) + expect(resolveTargetPolicy(config, { provider: 'replace-provider', model: MODEL })) + .toMatchObject({ + summarizationProvider: 'replacement-provider', + summarizationModel: 'replacement-model', + }) + }) + it('validates common values and pressure-policy invariants', () => { const bad = [ [{ maxTokens: 0 }, /maxTokens/], @@ -316,8 +351,10 @@ describe('compact configuration and defaults', () => { [{ auto: 'yes' }, /auto must be a boolean/], [{ summarizationProvider: 1 }, /summarizationProvider must be a string/], [{ summarizationModel: 1 }, /summarizationModel must be a string/], - [{ summarizationProvider: MODEL }, /must both be empty or both be non-empty/], - [{ summarizationModel: MODEL }, /must both be empty or both be non-empty/], + [{ summarizationProvider: MODEL }, /must be set together/], + [{ summarizationModel: MODEL }, /must be set together/], + [{ summarizationProvider: '' }, /must be set together/], + [{ summarizationModel: '' }, /must be set together/], [{ thresholdRatio: 0 }, /number in \(0, 1\]/], [{ thresholdRatio: 1.1 }, /number in \(0, 1\]/], [{ retainRatio: 0.9 }, /retainRatio \(0.9\) must be less than the resolved thresholdRatio \(0.8\)/], @@ -333,6 +370,16 @@ describe('compact configuration and defaults', () => { [{ modelPolicies: [{ provider: MODEL, model: 1 }] }, /model must be a non-empty string/], [{ modelPolicies: [{ provider: MODEL, model: '' }] }, /model must be a non-empty string/], [{ modelPolicies: [{ provider: MODEL, model: MODEL, summarizationProvider: 1 }] }, /summarizationProvider must be a string/], + [{ + summarizationProvider: 'default-provider', + summarizationModel: 'default-model', + modelPolicies: [{ provider: MODEL, model: MODEL, summarizationModel: '' }], + }, /modelPolicies\[0\].*must be set together/], + [{ + summarizationProvider: 'default-provider', + summarizationModel: 'default-model', + modelPolicies: [{ provider: MODEL, model: MODEL, summarizationProvider: '' }], + }, /modelPolicies\[0\].*must be set together/], [{ modelPolicies: [{ provider: MODEL, model: MODEL, retainRatio: 0.2, retainTokens: 100 }] }, /mutually exclusive/], [ { modelPolicies: [{ provider: MODEL, model: MODEL, thresholdRatio: 0.1 }] }, diff --git a/packages/compact/compact-basic/tests/compact-loop-repro.spec.ts b/packages/compact/compact-basic/tests/compact-loop-repro.spec.ts index 9528683d2b..3326bd130c 100644 --- a/packages/compact/compact-basic/tests/compact-loop-repro.spec.ts +++ b/packages/compact/compact-basic/tests/compact-loop-repro.spec.ts @@ -133,7 +133,6 @@ async function harness(toolSteps: number): Promise<{ ctx: Context; compact: Repr auto: true, thresholdRatio: 0.5, retainTokens: 50, - summarizationModel: '', maxTokens: 8192, compactionRetries: 1, }) diff --git a/packages/compact/compact-basic/tests/loader-composition.spec.ts b/packages/compact/compact-basic/tests/loader-composition.spec.ts index c80f12c4bb..74f8423e52 100644 --- a/packages/compact/compact-basic/tests/loader-composition.spec.ts +++ b/packages/compact/compact-basic/tests/loader-composition.spec.ts @@ -85,7 +85,7 @@ describe('real Loader composition', () => { context = new Context() await expect(context.plugin(TokenMeterService, { contextWindow: 4096, - })).rejects.toThrow(/TokenMeterConfig: unknown key "contextWindow"/) + } as never)).rejects.toThrow(/TokenMeterConfig: unknown key "contextWindow"/) }) it('rejects stale compact-basic config after Schemastery normalization', async () => { @@ -110,4 +110,19 @@ describe('real Loader composition', () => { }], })).rejects.toThrow(/modelPolicies\[0\]: retainRatio \(0.2\).*thresholdRatio \(0.1\)/) }) + + it('rejects an incomplete model-policy summarization pair during plugin load', async () => { + context = new Context() + await context.plugin(LlmService) + await context.plugin(TokenMeterService) + await expect(context.plugin(BasicCompactService, { + summarizationProvider: 'default-provider', + summarizationModel: 'default-model', + modelPolicies: [{ + provider: 'test-provider', + model: 'test-model', + summarizationModel: '', + }], + })).rejects.toThrow(/modelPolicies\[0\].*must be set together/) + }) }) diff --git a/packages/llm/token-meter/src/index.ts b/packages/llm/token-meter/src/index.ts index 2fdfb064db..e17b0c06b6 100644 --- a/packages/llm/token-meter/src/index.ts +++ b/packages/llm/token-meter/src/index.ts @@ -80,7 +80,9 @@ declare module 'cordis' { /** Replay owner for one service-wide estimator and isolated per-session folds. */ export class TokenMeterService extends Service { - static Config: z = z.object({}) + // Schemastery preserves untrusted loader keys on an empty object schema; + // the public type excludes settings while validateConfigKeys rejects them. + static Config: z = z.object({}) as unknown as z private readonly states = new WeakMap() diff --git a/packages/llm/token-meter/src/types.ts b/packages/llm/token-meter/src/types.ts index 33aa3d4bb5..255425b639 100644 --- a/packages/llm/token-meter/src/types.ts +++ b/packages/llm/token-meter/src/types.ts @@ -7,7 +7,7 @@ import type { TokenUsage } from '@deepseek-ai/dsh-llm' /** Token-meter plugin configuration; the fixed estimator has no settings. */ -export type TokenMeterConfig = object +export type TokenMeterConfig = Record /** The baseline from which a signed surface delta produces current pressure. */ export type TokenMeasurementBaseline = diff --git a/packages/llm/token-meter/tests/token-meter.spec.ts b/packages/llm/token-meter/tests/token-meter.spec.ts index dc7d0c6e2e..13e196477a 100644 --- a/packages/llm/token-meter/tests/token-meter.spec.ts +++ b/packages/llm/token-meter/tests/token-meter.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, expectTypeOf, it } from 'vitest' import { Context } from 'cordis' import { CallId } from '@deepseek-ai/dsh-llm' import type { ContentBlock, Message, TokenUsage } from '@deepseek-ai/dsh-llm' @@ -87,10 +87,15 @@ function expectSurfaceTotal(measurement: TokenMeasurement): void { } describe('TokenMeterService configuration and registration', () => { + it('exposes an empty public configuration type', () => { + expectTypeOf<{}>().toExtend() + expectTypeOf<{ contextWindow: number }>().not.toExtend() + }) + it.each(['models', 'contextWindow', 'contextWidow'])( 'rejects stale or unknown top-level config key %s', (key) => { - expect(() => meter({ [key]: {} })) + expect(() => meter({ [key]: {} } as unknown as TokenMeterConfig)) .toThrow(`TokenMeterConfig: unknown key "${key}"`) }, )