From ecf3d64c1bc4d04663b3bc90b2aec4360d67be56 Mon Sep 17 00:00:00 2001 From: wxr <51250936+wxrbyte@users.noreply.github.com> Date: Wed, 7 Oct 2026 11:58:20 +0800 Subject: [PATCH] fix(eval): normalize judge settings once across preflight and resolution resolveJudgeConfigStatus validated trimmed provider/model values but handed the untrimmed env to resolveJudgeConfig, which only lowercased. Two consequences, both observable: - a whitespace-only FINAGENT_JUDGE_API_KEY passed the preflight with an empty `missing` list, so the client was built with a blank key and only failed at request time; - a provider value with surrounding whitespace (`" anthropic"`) passed the preflight as anthropic, while resolveJudgeConfig fell through to 'openai-compatible' and picked the OpenAI endpoint for an Anthropic model. Trim provider, model, and apiKey in both functions so "configured" means the same thing on both sides. baseUrl is untouched: neither function normalizes it. --- .../src/evaluation/judge-client.test.ts | 85 +++++++++++++++++++ .../shared/src/evaluation/judge-client.ts | 21 +++-- 2 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 packages/shared/src/evaluation/judge-client.test.ts diff --git a/packages/shared/src/evaluation/judge-client.test.ts b/packages/shared/src/evaluation/judge-client.test.ts new file mode 100644 index 00000000..ff8ef809 --- /dev/null +++ b/packages/shared/src/evaluation/judge-client.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it } from 'bun:test'; +import { resolveJudgeConfig, resolveJudgeConfigStatus } from './judge-client.ts'; + +describe('judge config normalization', () => { + it('treats a whitespace-only API key as missing at preflight', () => { + const status = resolveJudgeConfigStatus({ + FINAGENT_JUDGE_PROVIDER: 'anthropic', + FINAGENT_JUDGE_MODEL: 'claude-sonnet-4', + FINAGENT_JUDGE_API_KEY: ' ', + }); + + expect(status.requested).toBe(true); + expect(status.missing).toEqual(['API key (--judge-api-key / FINAGENT_JUDGE_API_KEY)']); + expect(status.config).toBeUndefined(); + }); + + it('resolves the provider the preflight promised when the value carries whitespace', () => { + const env: NodeJS.ProcessEnv = { + FINAGENT_JUDGE_PROVIDER: ' anthropic\n', + FINAGENT_JUDGE_MODEL: 'claude-sonnet-4', + FINAGENT_JUDGE_API_KEY: 'k', + }; + + expect(resolveJudgeConfigStatus(env).missing).toEqual([]); + expect(resolveJudgeConfigStatus(env).config?.provider).toBe('anthropic'); + expect(resolveJudgeConfig(env)?.provider).toBe('anthropic'); + }); + + it('hands trimmed model and API key to the client', () => { + const config = resolveJudgeConfig({ + FINAGENT_JUDGE_PROVIDER: 'openai-compatible', + FINAGENT_JUDGE_MODEL: ' gpt-4o ', + FINAGENT_JUDGE_API_KEY: ' sk-test\n', + }); + + expect(config).toMatchObject({ provider: 'openai-compatible', model: 'gpt-4o', apiKey: 'sk-test' }); + }); + + it('names every missing setting when the configuration is incomplete', () => { + const status = resolveJudgeConfigStatus({ FINAGENT_JUDGE_MODEL: 'gpt-4o' }); + + expect(status.requested).toBe(true); + expect(status.config).toBeUndefined(); + expect(status.missing).toEqual([ + 'provider (--judge-provider / FINAGENT_JUDGE_PROVIDER)', + 'API key (--judge-api-key / FINAGENT_JUDGE_API_KEY)', + ]); + }); + + it('reports "not requested" when every value is blank', () => { + const status = resolveJudgeConfigStatus({ + FINAGENT_JUDGE_PROVIDER: ' ', + FINAGENT_JUDGE_MODEL: '', + FINAGENT_JUDGE_API_KEY: ' ', + }); + + expect(status.requested).toBe(false); + expect(status.missing).toEqual([]); + expect(status.config).toBeUndefined(); + }); + + it('keeps the preflight verdict and the resolved config in agreement', () => { + const cases: NodeJS.ProcessEnv[] = [ + { FINAGENT_JUDGE_PROVIDER: 'anthropic', FINAGENT_JUDGE_MODEL: 'claude', FINAGENT_JUDGE_API_KEY: 'k' }, + { FINAGENT_JUDGE_PROVIDER: ' anthropic ', FINAGENT_JUDGE_MODEL: ' claude ', FINAGENT_JUDGE_API_KEY: ' k ' }, + { FINAGENT_JUDGE_PROVIDER: 'openai-compatible', FINAGENT_JUDGE_MODEL: 'gpt-4o', FINAGENT_JUDGE_API_KEY: ' ' }, + { FINAGENT_JUDGE_PROVIDER: '', FINAGENT_JUDGE_MODEL: 'claude', FINAGENT_JUDGE_API_KEY: 'k' }, + { FINAGENT_JUDGE_PROVIDER: 'anthropic\n', FINAGENT_JUDGE_MODEL: '\tclaude', FINAGENT_JUDGE_API_KEY: 'k\n' }, + ]; + + for (const env of cases) { + const status = resolveJudgeConfigStatus(env); + // A clean preflight must always hand over a usable config, and an + // incomplete one must never do so. + expect(status.config !== undefined).toBe(status.requested && status.missing.length === 0); + } + }); + + it('prefers an explicit override over a blank env value', () => { + const status = resolveJudgeConfigStatus({ FINAGENT_JUDGE_PROVIDER: ' ' }, { provider: 'anthropic', model: 'claude', apiKey: 'k' }); + + expect(status.missing).toEqual([]); + expect(status.config).toMatchObject({ provider: 'anthropic', model: 'claude', apiKey: 'k' }); + }); +}); diff --git a/packages/shared/src/evaluation/judge-client.ts b/packages/shared/src/evaluation/judge-client.ts index 127afb12..5b5a0b0d 100644 --- a/packages/shared/src/evaluation/judge-client.ts +++ b/packages/shared/src/evaluation/judge-client.ts @@ -141,6 +141,15 @@ export interface JudgeConfigResolution { missing: string[]; } +/** + * Judge settings arrive from CLI flags, `process.env`, or a `.env` / secret + * file, so surrounding whitespace is a real input. Normalize once here so the + * preflight below and {@link resolveJudgeConfig} agree on what is configured. + */ +function judgeSetting(value: string | undefined): string { + return (value ?? '').trim(); +} + /** * Like {@link resolveJudgeConfig}, but distinguishes "no judge requested" from * "judge requested but misconfigured". The live preflight (issue #113) needs @@ -151,9 +160,9 @@ export function resolveJudgeConfigStatus( env: NodeJS.ProcessEnv, explicit?: { provider?: string; model?: string; apiKey?: string; baseUrl?: string }, ): JudgeConfigResolution { - const provider = (explicit?.provider ?? env.FINAGENT_JUDGE_PROVIDER ?? '').trim(); - const model = (explicit?.model ?? env.FINAGENT_JUDGE_MODEL ?? '').trim(); - const apiKey = explicit?.apiKey ?? env.FINAGENT_JUDGE_API_KEY ?? ''; + const provider = judgeSetting(explicit?.provider ?? env.FINAGENT_JUDGE_PROVIDER); + const model = judgeSetting(explicit?.model ?? env.FINAGENT_JUDGE_MODEL); + const apiKey = judgeSetting(explicit?.apiKey ?? env.FINAGENT_JUDGE_API_KEY); const requested = provider.length > 0 || model.length > 0 || apiKey.length > 0; if (!requested) return { requested: false, missing: [] }; @@ -175,9 +184,9 @@ export function resolveJudgeConfig( env: NodeJS.ProcessEnv, explicit?: { provider?: string; model?: string; apiKey?: string; baseUrl?: string; headers?: Record }, ): ResolvedJudgeConfig | undefined { - const provider = (explicit?.provider ?? env.FINAGENT_JUDGE_PROVIDER ?? '').toLowerCase(); - const model = explicit?.model ?? env.FINAGENT_JUDGE_MODEL; - const apiKey = explicit?.apiKey ?? env.FINAGENT_JUDGE_API_KEY; + const provider = judgeSetting(explicit?.provider ?? env.FINAGENT_JUDGE_PROVIDER).toLowerCase(); + const model = judgeSetting(explicit?.model ?? env.FINAGENT_JUDGE_MODEL); + const apiKey = judgeSetting(explicit?.apiKey ?? env.FINAGENT_JUDGE_API_KEY); if (!provider || !model || !apiKey) return undefined; const headers = explicit?.headers ?? parseJudgeHeaders(env.FINAGENT_JUDGE_HEADERS); const extra = headers ? { headers } : {};