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 */