fix(review): honour the configured adapter, and let review be pinned apart from the rest (AGT-4292)

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 <newname>` 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());
+  });
+});

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' };
+}

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<typeof import('../core/config.js')>()),
+  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,15 +468,94 @@ 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 });
     // The human verdict block must not have gone to stdout as well.
     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(