From 15ada05346bfaf9cd07bd5ef9191d84012d33a43 Mon Sep 17 00:00:00 2001 From: unohee Date: Thu, 10 Sep 2026 21:00:54 +0900 Subject: [PATCH] fix(review): honour the configured adapter, and let review be pinned apart from the rest (AGT-4292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openswarm review` read the config file for Linear settings and nothing else, so `runReviewer` fell through to the registry default — 'codex' — whatever the operator had configured. The daemon honours `adapter:`; the standalone CLI did not. Someone running `adapter: codex-responses` with an OpenRouter key had no way to review on openrouter. Review is also the one role worth pinning separately. It is the second opinion, so running it on the same provider as the work it checks is a correlated failure, and `reviewAdapter:` exists so that choice does not force every other role onto another provider. Precedence, most specific first: --adapter > OPENSWARM_REVIEW_ADAPTER > config.reviewAdapter > config.adapter > built-in default. An unknown name throws rather than falling through to a lower-precedence source, and names the valid options the way the registry's own error does — the operator asked for something specific and would otherwise silently get a different provider. A flag or the environment variable short-circuits before config is read. That is not an optimisation: `loadConfig` is not a pure read, it engages `enableHumanSurfaceReadOnly()` and the sandbox executor wiring, and a plain `openswarm review` never triggered either before this existed. When config IS consulted and the caller asked for JSON, `console` is silenced around the call, because `loadConfig` logs where it loaded from straight to stdout, in front of the JSON document. `ADAPTER_NAMES` is a leaf module rather than a re-export. Importing the adapter registry for a string check pulls in `codex.ts`'s module-scope `promisify(execFile)` and breaks eleven tests that mock `node:child_process`; putting it on `core/config.ts` breaks a test that mocks that module. Nothing in the source links the copy to the registry, so a test asserts they hold the same names — drift otherwise surfaces as a config that fails validation for an adapter the registry supports. Two things the review of this change found that it does NOT fix, both filed rather than folded in, because bundling an unrelated root cause into an adapter-resolution change is the mistake that cost three review rounds elsewhere today: AGT-4298 — `mcpClient.ts` calls `loadConfig()` unguarded during tool auto-discovery, so `review --json | jq` is still broken for any review that uses tools. Predates this change. Every test here stubs `deps.review`, so no test in the suite runs that path. AGT-4299 — `review --max` still resolves via `opts.adapter ?? activeAdapter()` and ignores `reviewAdapter:`, while `review --debug` reports the key as honoured. `reviewAdapter` is also documented nowhere. The stdout guard test was rewritten because it passed for the wrong reason: it spied on `process.stdout.write` while vitest intercepts `console.*` above that spy, so the pollution never reached its capture. It now captures both and asserts `JSON.parse` of the whole thing, which is what `jq` does. Review: layer 2 (independent subagent), two rounds. Round 1 found the stdout regression and the false-green guard; round 2 confirmed those fixed and found AGT-4298/4299 by running the real CLI. Mutants: removing the console suppression kills 2 tests, not propagating --json 2, removing the short-circuit 2, a phantom name in ADAPTER_NAMES 1. tsc --noEmit exit 0 · src/cli/ + src/core/ 1055 passed, exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- src/cli/reviewAdapter.test.ts | 86 ++++++++++++++++++++++++++++ src/cli/reviewAdapter.ts | 81 ++++++++++++++++++++++++++ src/cli/reviewCommand.test.ts | 104 +++++++++++++++++++++++++++++++++- src/cli/reviewCommand.ts | 73 +++++++++++++++++++++++- src/core/adapterNames.ts | 25 ++++++++ src/core/config.ts | 14 ++++- src/core/types.ts | 5 ++ 7 files changed, 384 insertions(+), 4 deletions(-) create mode 100644 src/cli/reviewAdapter.test.ts create mode 100644 src/cli/reviewAdapter.ts create mode 100644 src/core/adapterNames.ts diff --git a/src/cli/reviewAdapter.test.ts b/src/cli/reviewAdapter.test.ts new file mode 100644 index 00000000..7399be84 --- /dev/null +++ b/src/cli/reviewAdapter.test.ts @@ -0,0 +1,86 @@ +// Precedence is the whole behaviour here, and it was previously unobservable: +// `openswarm review` read the config file for Linear and nothing else, so the +// reviewer ran on the registry default no matter what an operator configured +// (AGT-4292). These pin the order, and pin that a typo fails loudly instead of +// quietly selecting a different provider. + +import { describe, expect, it } from 'vitest'; + +import { ADAPTER_NAMES } from '../core/adapterNames.js'; +import { resolveReviewAdapter } from './reviewAdapter.js'; + +const known = (name: string) => ['codex', 'codex-responses', 'openrouter', 'claude'].includes(name); + +describe('resolveReviewAdapter', () => { + it('prefers the flag over everything else', () => { + expect(resolveReviewAdapter({ + flag: 'openrouter', env: 'claude', configReview: 'codex', configDefault: 'codex-responses', + }, known)).toEqual({ name: 'openrouter', source: 'flag' }); + }); + + it('falls to the environment when there is no flag', () => { + expect(resolveReviewAdapter({ + env: 'openrouter', configReview: 'codex', configDefault: 'codex-responses', + }, known)).toEqual({ name: 'openrouter', source: 'env' }); + }); + + it('prefers reviewAdapter over adapter, so review can differ from the rest', () => { + // The point of the key: a second opinion on the same provider as the work + // it checks is a correlated failure. + expect(resolveReviewAdapter({ + configReview: 'openrouter', configDefault: 'codex-responses', + }, known)).toEqual({ name: 'openrouter', source: 'config.reviewAdapter' }); + }); + + it('follows the installation adapter when review is not pinned', () => { + expect(resolveReviewAdapter({ configDefault: 'codex-responses' }, known)) + .toEqual({ name: 'codex-responses', source: 'config.adapter' }); + }); + + it('leaves the registry default alone when nothing is configured', () => { + // Undefined, not a guessed name: the caller passes this straight to + // `getAdapter`, whose own default is the correct fallback. + expect(resolveReviewAdapter({}, known)).toEqual({ source: 'built-in default' }); + }); + + it('ignores blank and whitespace-only values instead of treating them as a choice', () => { + // An unset shell variable arrives as '' — that must not outrank config. + expect(resolveReviewAdapter({ env: '', configDefault: 'codex' }, known)) + .toEqual({ name: 'codex', source: 'config.adapter' }); + expect(resolveReviewAdapter({ env: ' ', configDefault: 'codex' }, known)) + .toEqual({ name: 'codex', source: 'config.adapter' }); + }); + + it('trims a value rather than rejecting it', () => { + expect(resolveReviewAdapter({ flag: ' openrouter ' }, known)) + .toEqual({ name: 'openrouter', source: 'flag' }); + }); + + it('refuses an unknown name instead of silently using a lower-precedence one', () => { + // The dangerous case: a typo in the flag would otherwise fall through to + // config and run the review on a provider the operator did not ask for, + // with nothing on screen to say so. + expect(() => resolveReviewAdapter({ flag: 'openrouterr', configDefault: 'codex' }, known)) + .toThrow(/openrouterr.*flag/); + expect(() => resolveReviewAdapter({ configReview: 'nope' }, known)) + .toThrow(/config\.reviewAdapter/); + }); +}); + +describe('ADAPTER_NAMES stays in step with the real registry', () => { + it('lists exactly the adapters the registry has', async () => { + // `adapterNames.ts` is a copy of the registry's key set, kept separate + // because importing the registry for a string check pulls in `codex.ts`'s + // module-scope `promisify(execFile)` and breaks every test that mocks + // `node:child_process`. Nothing in the source links the two lists, so this + // is the link: drift otherwise surfaces only at runtime, as a config that + // fails Zod validation for an adapter the registry supports, or as + // `--adapter ` throwing "Unknown review adapter" for a name that + // would have worked. + // + // This file does not mock `node:child_process`, so it is one of the few + // places the registry can be imported without paying that cost. + const { listAdapterNames } = await import('../adapters/index.js'); + expect([...listAdapterNames()].sort()).toEqual([...ADAPTER_NAMES].sort()); + }); +}); diff --git a/src/cli/reviewAdapter.ts b/src/cli/reviewAdapter.ts new file mode 100644 index 00000000..c84014e7 --- /dev/null +++ b/src/cli/reviewAdapter.ts @@ -0,0 +1,81 @@ +// ============================================ +// OpenSwarm — which adapter reviews a change (AGT-4292) +// ============================================ +// +// `openswarm review` read the config file for Linear settings and for nothing +// else, so `runReviewer` fell through to the module default — 'codex' — no +// matter what the operator had configured. The daemon honours `adapter:`; the +// standalone CLI did not, which is the CLI-vs-daemon capability gap this repo +// keeps rediscovering. +// +// Review is also the one role an operator may reasonably want to pin +// separately: it is the second opinion, so running it on the same provider as +// the work it checks is a correlated failure. `reviewAdapter` exists so that +// choice does not force the whole daemon onto another provider. +// +// Pure and separately testable: the caller needs a loaded config and a live +// adapter registry, neither of which says anything about precedence. + +/** Where a review adapter can come from, most specific first. */ +export interface ReviewAdapterSources { + /** `--adapter` on the command line. */ + flag?: string; + /** OPENSWARM_REVIEW_ADAPTER — a per-shell override that needs no config edit. */ + env?: string; + /** `reviewAdapter:` in config — pins review without moving every other role. */ + configReview?: string; + /** `adapter:` in config — what the rest of this installation uses. */ + configDefault?: string; +} + +export interface ReviewAdapterChoice { + /** The adapter to use, or undefined to leave the registry default in place. */ + name?: string; + /** + * Which source won, for the debug line. + * + * `config.adapter` is reported for a value that came from the config schema's + * own default as well as one the operator typed — `AdapterNameSchema` has + * `.default('codex')`, so `config.adapter` is truthy whenever any config file + * parses at all. `built-in default` therefore only appears when config could + * not be loaded. The two spellings mean the same adapter; the label is a hint + * about where to look, not a claim about what was written. + */ + source: 'flag' | 'env' | 'config.reviewAdapter' | 'config.adapter' | 'built-in default'; +} + +/** + * Pick the review adapter. + * + * `isKnown` is injected rather than imported so an unknown name is reported + * here instead of failing later inside the adapter registry with no context + * about where the bad value came from. + */ +export function resolveReviewAdapter( + sources: ReviewAdapterSources, + isKnown: (name: string) => boolean, + known: readonly string[] = [], +): ReviewAdapterChoice { + const candidates: [ReviewAdapterChoice['source'], string | undefined][] = [ + ['flag', sources.flag], + ['env', sources.env], + ['config.reviewAdapter', sources.configReview], + ['config.adapter', sources.configDefault], + ]; + for (const [source, raw] of candidates) { + const name = raw?.trim(); + if (!name) continue; + // An unknown name must not silently fall through to a lower-precedence + // source: the operator asked for something specific and would otherwise + // get a different provider with no indication. + if (!isKnown(name)) { + // Name the value, where it came from, AND what would have worked. The + // registry's own error lists the alternatives; an operator who hits this + // one first should not have to go looking for that list. + const options = known.length > 0 ? `. Available: ${known.join(', ')}` : ''; + throw new Error(`Unknown review adapter "${name}" (from ${source})${options}`); + } + return { name, source }; + } + return { source: 'built-in default' }; +} diff --git a/src/cli/reviewCommand.test.ts b/src/cli/reviewCommand.test.ts index 27c9b533..611a8f5d 100644 --- a/src/cli/reviewCommand.test.ts +++ b/src/cli/reviewCommand.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, expect, it, onTestFinished, vi } from 'vitest'; import { buildReviewWorkerResult, formatReviewOutput, @@ -16,6 +16,17 @@ import type { ReviewResult } from '../agents/agentPair.js'; const getChangedFilesMock = vi.fn(async () => ['x.ts']); vi.mock('../support/gitTracker.js', () => ({ getChangedFiles: getChangedFilesMock })); +// `loadConfig` is not a pure read — it engages process-wide toggles +// (human-surface read-only, sandbox executor wiring) and logs to stdout. Before +// AGT-4292 a plain `openswarm review` never called it, so this spy is how the +// tests below can tell "resolved without touching config" from "loaded config +// and then discarded it". (AGT-4292) +const loadConfigMock = vi.hoisted(() => vi.fn(() => ({ adapter: 'codex', reviewAdapter: undefined }))); +vi.mock('../core/config.js', async (importOriginal) => ({ + ...(await importOriginal()), + loadConfig: loadConfigMock, +})); + describe('buildReviewWorkerResult (INT-1955)', () => { it('synthesizes a WorkerResult from changed files', () => { const wr = buildReviewWorkerResult(['a.ts', 'b.ts']); @@ -434,11 +445,21 @@ describe('runReviewCommand machine-readable output (INT-3102)', () => { it('--json writes the verdict to stdout and keeps prose off it', async () => { // Mixing the human report into stdout would break `review --json | jq`. + // + // `console.log` is captured as well as `process.stdout.write`, and that is + // the point: vitest intercepts `console` ABOVE the stdout spy, so anything + // written that way never reached the array this test parses. AGT-4292 + // added a `loadConfig()` call on this path, which logs "Config loading + // from …" and two credential warnings straight to stdout, in front of the + // JSON document — and this test stayed green through all of it. const stdout: string[] = []; + const push = (chunk: unknown) => { stdout.push(String(chunk)); }; const write = vi.spyOn(process.stdout, 'write').mockImplementation((chunk: any) => { - stdout.push(String(chunk)); + push(chunk); return true; }); + const consoleLog = vi.spyOn(console, 'log').mockImplementation((...args) => push(args.join(' '))); + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation((...args) => push(args.join(' '))); const logs: string[] = []; try { await runReviewCommand( @@ -447,8 +468,12 @@ describe('runReviewCommand machine-readable output (INT-3102)', () => { ); } finally { write.mockRestore(); + consoleLog.mockRestore(); + consoleWarn.mockRestore(); } + // Parsing the WHOLE capture is the assertion. `JSON.parse` on a document + // with anything in front of it throws, which is exactly what `jq` does. const parsed = JSON.parse(stdout.join('')); expect(parsed).toMatchObject({ schemaVersion: 1, decision: 'revise', gateRan: true }); expect(parsed.findings[0]).toMatchObject({ file: 'src/auth.ts', line: 42 }); @@ -456,6 +481,81 @@ describe('runReviewCommand machine-readable output (INT-3102)', () => { expect(logs.join('\n')).not.toContain('Decision: REVISE'); }); + it('keeps stdout parseable even when a config file is present to be loaded', async () => { + // The regression above was reachable only when `loadConfig()` actually + // found something to say. With no flag and no env var the resolution falls + // through to config, which is the common path for a CI `review --json`. + const prevFlag = process.env.OPENSWARM_REVIEW_ADAPTER; + delete process.env.OPENSWARM_REVIEW_ADAPTER; + onTestFinished(() => { + if (prevFlag === undefined) delete process.env.OPENSWARM_REVIEW_ADAPTER; + else process.env.OPENSWARM_REVIEW_ADAPTER = prevFlag; + }); + + const stdout: string[] = []; + const push = (chunk: unknown) => { stdout.push(String(chunk)); }; + const write = vi.spyOn(process.stdout, 'write').mockImplementation((chunk: any) => { + push(chunk); + return true; + }); + const consoleLog = vi.spyOn(console, 'log').mockImplementation((...args) => push(args.join(' '))); + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation((...args) => push(args.join(' '))); + try { + await runReviewCommand( + { json: true }, + { getChangedFiles: async () => ['x.ts'], review: reviewed, startProgress: () => null, log: () => {} }, + ); + } finally { + write.mockRestore(); + consoleLog.mockRestore(); + consoleWarn.mockRestore(); + } + + expect(() => JSON.parse(stdout.join(''))).not.toThrow(); + }); + + it('does not read config when the flag already decided the adapter', async () => { + // The short-circuit is not an optimisation. `loadConfig` flips process-wide + // toggles that a review run had nothing to do with before this resolution + // existed, so "load it and then throw the answer away" is a behaviour + // change dressed as a no-op. + loadConfigMock.mockClear(); + await runReviewCommand( + { adapter: 'claude' }, + { getChangedFiles: async () => ['x.ts'], review: reviewed, startProgress: () => null, log: () => {} }, + ); + expect(loadConfigMock).not.toHaveBeenCalled(); + }); + + it('does not read config when the environment already decided it', async () => { + const prev = process.env.OPENSWARM_REVIEW_ADAPTER; + process.env.OPENSWARM_REVIEW_ADAPTER = 'openrouter'; + onTestFinished(() => { + if (prev === undefined) delete process.env.OPENSWARM_REVIEW_ADAPTER; + else process.env.OPENSWARM_REVIEW_ADAPTER = prev; + }); + loadConfigMock.mockClear(); + await runReviewCommand( + {}, + { getChangedFiles: async () => ['x.ts'], review: reviewed, startProgress: () => null, log: () => {} }, + ); + expect(loadConfigMock).not.toHaveBeenCalled(); + }); + + it('does read config when nothing higher-precedence decided it', async () => { + // Guard the guard: without this the two negatives above would also pass if + // the config path were removed outright. + const prev = process.env.OPENSWARM_REVIEW_ADAPTER; + delete process.env.OPENSWARM_REVIEW_ADAPTER; + onTestFinished(() => { if (prev !== undefined) process.env.OPENSWARM_REVIEW_ADAPTER = prev; }); + loadConfigMock.mockClear(); + await runReviewCommand( + {}, + { getChangedFiles: async () => ['x.ts'], review: reviewed, startProgress: () => null, log: () => {} }, + ); + expect(loadConfigMock).toHaveBeenCalled(); + }); + it('still prints the human report when --json is absent', async () => { const logs: string[] = []; await runReviewCommand( diff --git a/src/cli/reviewCommand.ts b/src/cli/reviewCommand.ts index a73dffa2..19aaf424 100644 --- a/src/cli/reviewCommand.ts +++ b/src/cli/reviewCommand.ts @@ -395,6 +395,70 @@ export interface ReviewCommandOptions { /** * Run the review flow. Injectable deps keep it testable without git/network. */ + +/** + * Resolve the reviewer's adapter from flag, environment and config. + * + * Config is loaded lazily and failures are swallowed: a malformed or absent + * config must not stop a review that was going to use the built-in default + * anyway. (AGT-4292) + * + * Two things this must not do on the way. + * + * It must not read config it does not need. `loadConfig` is not a pure read — + * it engages process-wide toggles (`enableHumanSurfaceReadOnly`, the sandbox + * executor wiring). Before this resolution existed, a plain `openswarm review` + * never called it, so a higher-precedence answer has to short-circuit rather + * than load config and then discard it. + * + * And it must not write to stdout when the caller asked for JSON. `loadConfig` + * logs where it loaded from and warns about absent Discord/Linear credentials, + * straight to stdout — which lands in front of the JSON document and makes + * `openswarm review --json | jq` fail to parse. `cli.ts` already solves this + * for telemetry by silencing `console` around the call; the same applies here. + * + * That fixes THIS call site and not the contract as a whole. `mcpClient.ts` + * calls `loadConfig()` unguarded during tool auto-discovery, so a review that + * actually uses tools still prints config lines in front of the JSON — a + * defect that predates this resolution and is tracked as AGT-4298. The + * silencing here is a third ad hoc copy of a pattern that belongs in + * `loadConfig` itself; AGT-4298 collapses all three. + * + * The restore is not reentrancy-safe: each call captures whatever `console.log` + * currently is and blind-restores it, so two crossed, non-nested windows would + * leave the patched function in place. No caller reaches that today — `cli.ts` + * invokes this once per process and `prProcessor.ts` never passes `json` — but + * a future concurrent caller needs a nesting counter, not this. + */ +async function resolveConfiguredReviewAdapter(flag?: string, quiet = false) { + const { resolveReviewAdapter } = await import('./reviewAdapter.js'); + // A leaf module, not the adapter registry and not config: see adapterNames.ts. + const { ADAPTER_NAMES, isConfiguredAdapterName } = await import('../core/adapterNames.js'); + const env = process.env.OPENSWARM_REVIEW_ADAPTER; + // A flag or an env var already decides it. Loading config here would buy + // nothing and cost the side effects above. + if (flag?.trim() || env?.trim()) { + return resolveReviewAdapter({ flag, env }, isConfiguredAdapterName, ADAPTER_NAMES); + } + let configReview: string | undefined; + let configDefault: string | undefined; + const originalLog = console.log; + const originalWarn = console.warn; + try { + if (quiet) { console.log = () => undefined; console.warn = () => undefined; } + const { loadConfig } = await import('../core/config.js'); + const config = loadConfig(); + configReview = config.reviewAdapter; + configDefault = config.adapter; + } catch { /* no config, or unreadable — flag and env still apply */ } finally { + console.log = originalLog; + console.warn = originalWarn; + } + return resolveReviewAdapter( + { flag, env, configReview, configDefault }, isConfiguredAdapterName, ADAPTER_NAMES, + ); +} + export async function runReviewCommand( opts: ReviewCommandOptions = {}, deps: { @@ -458,6 +522,12 @@ export async function runReviewCommand( }); if (history.context) log(`Loaded prior review log context for ${changed.length} changed file(s).`); + // Honour the configured adapter. Without this the reviewer used the registry + // default regardless of config, so `adapter:`/`reviewAdapter:` were dead for + // this command. (AGT-4292) + const adapterChoice = await resolveConfiguredReviewAdapter(opts.adapter, opts.json === true); + if (opts.debug && adapterChoice.name) log(`Reviewer adapter: ${adapterChoice.name} (${adapterChoice.source})`); + const review = deps.review ?? (async (wr: WorkerResult, c: string, onLog?: (line: string) => void) => { @@ -469,7 +539,7 @@ export async function runReviewCommand( : 'Review the current working-tree changes for correctness, bugs, and follow-ups.', workerResult: wr, projectPath: c, - adapterName: opts.adapter as never, + adapterName: adapterChoice.name as never, mode: 'direct', priorReviewContext: history.context, readOnly: opts.readOnly, @@ -613,3 +683,4 @@ export async function runReviewCommand( } } } +// tmp comment 2026년 9월 10일 목요일 20시 43분 03초 KST diff --git a/src/core/adapterNames.ts b/src/core/adapterNames.ts new file mode 100644 index 00000000..cb3b4279 --- /dev/null +++ b/src/core/adapterNames.ts @@ -0,0 +1,25 @@ +// ============================================ +// OpenSwarm — adapter names, with nothing attached (AGT-4292) +// ============================================ +// +// Its own module because the two obvious homes both cost more than a string +// list should. `adapters/index.ts` imports every adapter, and `codex.ts` calls +// `promisify(execFile)` at module scope — so validating one name there breaks +// any test that mocks `node:child_process`. `core/config.ts` is lighter but is +// widely mocked, and a new export on it makes every one of those mocks +// incomplete. +// +// A leaf with no imports is mockable by nobody and breaks nothing. + +/** Adapter names as configuration and the CLI accept them. */ +export const ADAPTER_NAMES = [ + 'codex', 'codex-responses', 'gpt', 'local', 'lmstudio', + 'openrouter', 'atlascloud', 'claude', 'cc-router', 'cursor', +] as const; + +export type AdapterName = (typeof ADAPTER_NAMES)[number]; + +/** True when `name` is an adapter configuration would accept. */ +export function isConfiguredAdapterName(name: string): name is AdapterName { + return (ADAPTER_NAMES as readonly string[]).includes(name); +} diff --git a/src/core/config.ts b/src/core/config.ts index 374b34cf..b0dc1b62 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -6,6 +6,7 @@ import { readFileSync, existsSync } from 'node:fs'; import { join, resolve } from 'node:path'; import { homedir } from 'node:os'; import { z } from 'zod'; +import { ADAPTER_NAMES } from './adapterNames.js'; import YAML from 'yaml'; import type { SwarmConfig, AgentSession, LongRunningMonitorConfig, ConflictResolverConfig, McpConfig } from './types.js'; import { setTimeWindowConfig, DEFAULT_TIME_WINDOW } from '../support/timeWindow.js'; @@ -45,7 +46,7 @@ function getConfigSearchPaths(): string[] { const DEFAULT_HEARTBEAT_INTERVAL = 30 * 60 * 1000; // 30 minutes const DEFAULT_GITHUB_CHECK_INTERVAL = 5 * 60 * 1000; // 5 minutes -const AdapterNameSchema = z.enum(['codex', 'codex-responses', 'gpt', 'local', 'lmstudio', 'openrouter', 'atlascloud', 'claude', 'cc-router', 'cursor']); +const AdapterNameSchema = z.enum(ADAPTER_NAMES); // Zod Schemas @@ -514,6 +515,13 @@ const DailyReporterConfigSchema = z.object({ const RawConfigSchema = z.object({ adapter: AdapterNameSchema.default('codex'), + /** + * Adapter for `openswarm review`, when it should differ from `adapter`. + * Review is a second opinion, so running it on the same provider as the work + * it checks is a correlated failure — and that provider's quota is the one + * already spent. Omit to follow `adapter`. (AGT-4292) + */ + reviewAdapter: AdapterNameSchema.optional(), language: z.enum(['en', 'ko']).default('en'), discord: DiscordConfigSchema, notifications: NotificationsSchema, @@ -665,6 +673,10 @@ function parseConfigFile(path: string): unknown { function transformConfig(raw: RawConfig): SwarmConfig { return { adapter: raw.adapter, + // Hand-picked mapping: a key added to the schema alone never reaches a + // caller. That is how AGT-4122 shipped, and how this key first shipped + // dead. (AGT-4292) + reviewAdapter: raw.reviewAdapter, language: raw.language, discordToken: raw.discord?.token ?? '', discordChannelId: raw.discord?.channelId ?? '', diff --git a/src/core/types.ts b/src/core/types.ts index b1b27c81..66db2356 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -127,6 +127,11 @@ export type McpConfig = { export type SwarmConfig = { /** Default CLI adapter */ adapter?: 'codex' | 'codex-responses' | 'gpt' | 'local' | 'lmstudio' | 'openrouter' | 'atlascloud' | 'claude' | 'cc-router' | 'cursor'; + /** + * Adapter for `openswarm review` only. Omit to follow `adapter`. + * See src/cli/reviewAdapter.ts. (AGT-4292) + */ + reviewAdapter?: 'codex' | 'codex-responses' | 'gpt' | 'local' | 'lmstudio' | 'openrouter' | 'atlascloud' | 'claude' | 'cc-router' | 'cursor'; /** UI language: 'en' | 'ko' (default: 'en') */ language: 'en' | 'ko'; /** Discord bot token */