Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Comprehensive PR Review

PR: #1609 — feat(cli): add Pi as AI provider option in setup wizard and doctor
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-05-07

Summary

The Pi provider addition to archon setup and archon doctor is well-structured: clean injection-based testability, thorough cancellation handling, no type safety violations, no API key leakage. Two HIGH issues should be addressed before merge — an ESM spy interception gap in doctor tests and zero test coverage for a file-mutating function. Three MEDIUM issues are real but non-blocking bugs.

Verdict: REQUEST_CHANGES

🟠 High Issues

H1: existsSync spy in doctor.test.ts likely doesn't intercept the named import

📍 packages/cli/src/commands/doctor.test.ts:113-116 / doctor.ts:8

doctor.ts uses import { existsSync } from 'fs' (named import). The test spies on fsModule.existsSync via a namespace import. setup.ts explicitly documents that this ESM rebinding technique does not work for named imports and provides probeFileExists wrappers to work around it. If the same limitation applies here, the "returns pass when ~/.pi/agent/auth.json exists" test silently tests nothing — it would pass by accident on a machine that has Pi credentials, or fail on CI machines that don't.

// doctor.ts — add exported wrapper before checkPi
export function probeAuthJsonExists(path: string): boolean {
  return existsSync(path);
}

export async function checkPi(env: NodeJS.ProcessEnv): Promise<CheckResult> {
  // ...
  if (probeAuthJsonExists(authJsonPath)) {   // ← use wrapper
    return { label, status: 'pass', message: '~/.pi/agent/auth.json found' };
  }
  // ...
}

// doctor.test.ts — spy on the exported wrapper
import * as doctorModule from './doctor';
authSpy = spyOn(doctorModule, 'probeAuthJsonExists');

H2: writeHomePiModelConfig has zero test coverage

📍 packages/cli/src/commands/setup.ts:1582-1609

writeHomePiModelConfig is exported, mutates a user-owned file (~/.archon/config.yaml), and has three distinct branches — none are tested. Branch 3's quote-escaping (model.replace(/"/g, '\\"')) is untested; branch 1's includes('pi:') check has a known false-positive risk (see LOW issue below). The same function also has the Docker path bug (MEDIUM M1) that tests would have caught.

describe('writeHomePiModelConfig', () => {
  let tmpDir: string;
  let originalHome: string | undefined;

  beforeEach(() => {
    tmpDir = mkdtempSync(join(tmpdir(), 'archon-pi-config-'));
    originalHome = process.env.ARCHON_HOME;
    process.env.ARCHON_HOME = tmpDir;
  });

  afterEach(() => {
    rmSync(tmpDir, { recursive: true, force: true });
    if (originalHome !== undefined) process.env.ARCHON_HOME = originalHome;
    else delete process.env.ARCHON_HOME;
  });

  it('writes fresh assistants.pi.model block when config is empty', () => {
    writeHomePiModelConfig('anthropic/claude-haiku-4-5');
    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
    expect(content).toContain('assistants:');
    expect(content).toContain('pi:');
    expect(content).toContain('model: "anthropic/claude-haiku-4-5"');
  });

  it('skips write when existing config already contains pi:', () => {
    writeFileSync(join(tmpDir, 'config.yaml'), 'assistants:\n  pi:\n    model: "old"\n');
    writeHomePiModelConfig('anthropic/claude-haiku-4-5');
    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
    expect(content).toContain('model: "old"');
    expect(content).not.toContain('claude-haiku-4-5');
  });

  it('shows note and does not write when config has assistants: but no pi:', () => {
    writeFileSync(join(tmpDir, 'config.yaml'), 'assistants:\n  claude:\n    model: sonnet\n');
    writeHomePiModelConfig('openai/gpt-4o');
    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
    expect(content).not.toContain('pi:');
  });

  it('escapes double quotes in model name', () => {
    writeHomePiModelConfig('vendor/"weird-model"');
    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
    expect(content).toContain('\\"weird-model\\"');
  });
});

🟡 Medium Issues

M1: writeHomePiModelConfig writes to wrong path in Docker

📍 packages/cli/src/commands/setup.ts:1582-1609

The function uses setup.ts's local getArchonHome() (line ~200), which returns ~/.archon unconditionally. The canonical version in @archon/paths returns /.archon inside Docker. doctor.ts correctly imports from @archon/paths. Inside Docker, writeHomePiModelConfig silently writes Pi model config to ~/.archon/config.yaml — a path that doesn't exist in the Docker mount — so Pi model config is never applied at runtime.

// setup.ts imports — add getArchonHome from @archon/paths
import { getArchonEnvPath, getRepoArchonEnvPath, getArchonHome } from '@archon/paths';

export function writeHomePiModelConfig(model: string): void {
  const home = getArchonHome();    // ← now uses @archon/paths (Docker-aware)
  // ... rest unchanged
}

M2: checkPi and checkExistingConfig false positive for Claude-only users

📍 packages/cli/src/commands/doctor.ts:89-117 / setup.ts:407-412

PI_API_KEY_VARS includes ANTHROPIC_API_KEY. A Claude user with DEFAULT_AI_ASSISTANT=claude and ANTHROPIC_API_KEY set will have hasApiKey=true, bypassing the skip gate — checkPi returns pass ("ANTHROPIC_API_KEY is set") even though Pi has never been configured. Similarly, checkExistingConfig.hasPi becomes true for Claude users, so setup re-runs display "Pi: Configured" incorrectly.

// doctor.ts
const isDefault = env.DEFAULT_AI_ASSISTANT === 'pi';
// Only count a shared key (e.g. ANTHROPIC_API_KEY) as Pi-evidence when Pi is the configured provider.
const hasApiKey = isDefault && PI_API_KEY_VARS.some(v => (env[v] ?? '').trim().length > 0);

if (!isDefault && !hasApiKey) {
  return { label, status: 'skip', message: 'Pi not configured' };
}

M3: writeHomePiModelConfig branch 1 docstring says "user-edited" — should say "idempotent"

📍 packages/cli/src/commands/setup.ts:1577

The JSDoc describes branch 1 as "skip (user-edited; don't overwrite)" but the check fires any time pi: appears — including on Archon's own re-run after a previous successful write. "User-edited" implies it only fires when the file was hand-edited, which could mislead a maintainer into adding force-overwrite logic for Archon-generated blocks and breaking idempotency.

/**
 * Write the Pi model ref to `~/.archon/config.yaml` so Pi knows which backend
 * to use by default. Three branches:
 *   1. File already contains `pi:` — skip (idempotent; avoids duplicate blocks
 *      on re-runs or when the user has already configured this manually).
 *   2. File contains `assistants:` but no `pi:` — show a manual `note()`
 *      because we can't safely splice into existing YAML indentation.
 *   3. Otherwise — append a fresh `assistants: pi: model:` block.
 */

🟢 Low Issues

✅ What's Good

• checkPiModule injection pattern is excellent — the loader default parameter avoids mock.module('@archon/providers') pollution, exactly matching the codebase's Bun mock isolation constraints.
• Non-fatal degradation: writeHomePiModelConfig failure is caught, shown to the user, and doesn't abort setup after the env write succeeded.
• API keys never logged: piApiKey collected via password() (masked), written only to .env, never in any log call.
• Cancellation paths are thorough — both select and password prompts handle isCancel correctly.
• Bidirectional "keep in sync" notes between PI_BACKENDS and PI_API_KEY_VARS explicitly name the other file — better than a generic pointer.
• continueWithoutPi confirm defaults to true — good UX, doesn't block setup on a non-critical check failure.
• docs-web addition is accurate — no gap between feature and user-facing docs.

Reviewed by Archon comprehensive-pr-review workflow
Full artifacts: /Users/rasmus/.archon/workspaces/coleam00/Archon/artifacts/runs/3211ffc8ad767223b53d1c80f988d6db/review/

Fix Report — automated self-review pass

Commit 33b0c0e7 addresses all HIGH and MEDIUM findings plus actionable LOW items from the review.

H1 — existsSync spy doesn't intercept named import (FIXED)

Added export function probeAuthJsonExists(path: string): boolean wrapper to doctor.ts, used it inside checkPi, and updated doctor.test.ts to spy on doctorModule.probeAuthJsonExists instead of fsModule.existsSync. Consistent with the probeFileExists pattern already in setup.ts.

H2 — writeHomePiModelConfig had zero test coverage (FIXED)

Added a 6-case describe('writeHomePiModelConfig') suite to setup.test.ts covering: fresh write when config absent, fresh write when config is empty, idempotent skip when pi: already present, note-only path when assistants: exists but no pi:, double-quote escaping, and a regression guard for the api: false positive.

M1 — writeHomePiModelConfig used wrong getArchonHome in Docker (FIXED)

writeHomePiModelConfig now calls pathsGetArchonHome (from @archon/paths) instead of the local getArchonHome(), so Docker environments (/.archon) are handled correctly.

M2 — checkPi false positive for Claude-only users (FIXED)

hasApiKey in checkPi is now only true when DEFAULT_AI_ASSISTANT === 'pi'. A Claude-only user with ANTHROPIC_API_KEY set no longer gets a spurious pass. Two regression tests added.

M3 — writeHomePiModelConfig docstring said "user-edited" (FIXED)

Branch-1 description updated to "idempotent; avoids duplicate blocks on re-runs or when the user has already configured this manually."

L1 — includes('pi:') false positive (FIXED)

Replaced with /^\s*pi\s*:/m.test(existing) so api: no longer matches.

L2 — log.warn consistency (FIXED)

Changed to log.warning(...) to match every other call site in setup.ts.

L3 — writeHomePiModelConfig catch omitted err.code (FIXED)

Error message now includes the errno code (e.g. EACCES) matching neighboring catch blocks.

L4 — Mixed Claude+Pi test missing section header assertion (FIXED)

Added expect(content).toContain('# Pi Authentication') to the mixed-setup test.

L5 — checkPiModule docstring (FIXED)

Replaced "mirrors the Claude binary check pattern" with "serves the same doctor-step role as checkClaudeBinary."

Skipped

L6 (CLAUDE.md comment) — doc-only, out of scope for this PR.

Validation: type-check ✓ · lint ✓ (0 warnings) · tests ✓ (0 failures)

🔍 Comprehensive PR Review

PR: #1609 — feat(cli): add Pi as AI provider option in setup wizard and doctor
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-05-09

Summary

This PR adds Pi as a first-class option in archon setup and archon doctor. The implementation is clean, follows established codebase patterns, and ships with 19 new test cases covering all major business-logic branches. All CI checks pass. No correctness bugs, no security issues, no type-safety violations.

Verdict: APPROVE (with minor fixes recommended)

🟡 Medium Issues (Recommended Fixes)

1. checkPiModule docstring claims "doctor-step role" — inaccurate

📍 packages/cli/src/commands/setup.ts (checkPiModule JSDoc)

The docstring says checkPiModule "serves the same doctor-step role as checkClaudeBinary". But checkClaudeBinary runs via doctorCommand in doctor.ts. checkPiModule only runs during the setup wizard — it is never called from doctorCommand. This will mislead future developers searching for where Pi's health check runs in archon doctor.

/**
 * Verify the Pi npm module is loadable. Pi is bundled as a transitive dep of
 * `@archon/providers` so this should always pass, but catching broken compiled
 * builds at setup time is preferable to a silent runtime failure.
 *
 * The `loader` parameter is injected in tests so we don't need
 * `mock.module()` on `@archon/providers` (which would pollute other tests).
 */

2. checkPiModule swallows non-Error throws and emits no structured log

📍 packages/cli/src/commands/setup.ts (checkPiModule catch block)

Two issues: (1) if the loader rejects with a non-Error value, .message is undefined and the caller renders the generic fallback 'module load failed'; (2) no structured log is emitted — a compiled-binary failure leaves no Pino breadcrumb.

} catch (err) {
  const message = err instanceof Error ? err.message : String(err);
  getLog().warn({ err }, 'setup.pi_module_load_failed');
  return { ok: false, error: message };
}

3. checkPiModule tests belong in setup.test.ts, not doctor.test.ts

📍 packages/cli/src/commands/doctor.test.ts:190-205

checkPiModule is defined and exported from setup.ts. Its 2 tests live in doctor.test.ts via a cross-module import. Project convention is one test file per source file — future developers will look in setup.test.ts first.

Fix: Move the describe('checkPiModule', ...) block to setup.test.ts and remove the cross-import from doctor.test.ts. No mock isolation conflicts prevent this.

4. checkExistingConfig.hasPi detection diverges from checkPi gate — undocumented

📍 packages/cli/src/commands/setup.ts (checkExistingConfig, hasPi line)

hasPi triggers on any Pi backend API key (ANTHROPIC_API_KEY, OPENROUTER_API_KEY, etc.) regardless of DEFAULT_AI_ASSISTANT. A Claude-only user with ANTHROPIC_API_KEY in ~/.archon/.env will see "Pi: Configured" in the setup re-run summary. checkPi in doctor.ts correctly gates on DEFAULT_AI_ASSISTANT=pi and documents this explicitly. The divergence is undocumented.

Decision needed: Is the API-key-only detection intentional for the setup summary?

// Detection is intentionally API-key-only (no DEFAULT_AI_ASSISTANT=pi check)
// so that re-runs after partial configs still surface Pi. Doctor's checkPi
// uses the stricter DEFAULT_AI_ASSISTANT=pi gate to avoid false passes for
// Claude users who share the same key env vars.
hasPi: PI_BACKENDS.some(b => hasEnvValue(content, b.envVar)),

🟢 Low Issues

✅ What's Good

• probeAuthJsonExists spy-wrapper pattern: Correctly applied to sidestep ESM named-import rebinding. Consistent with the existing probeFileExists pattern.
• YAML corruption guard in writeHomePiModelConfig: The /^\s*pi\s*:/m regex correctly prevents the api: false-positive. Explicit test guards this edge case.
• checkPi gating on DEFAULT_AI_ASSISTANT=pi: Correctly avoids false-pass for Claude-only users who have ANTHROPIC_API_KEY set.
• Cross-reference comments on PI_BACKENDS / PI_API_KEY_VARS: Both constants carry explicit cross-reference comments naming the counterpart. Exactly right for drift-prone parallel data structures.
• 19 new tests, all real assertions: Tests use actual temp dirs (no writeFileSync mocking), exercise real edge cases, and document design decisions inline.
• Regression guard test: The "Claude-only user with ANTHROPIC_API_KEY" test is named and commented — permanent documentation of the M2 design decision.
• Type safety: No any types, no eslint-disable, all new functions have explicit return types.
• selectedProviders array built after module-check: Elegantly handles the case where hasPi is cleared mid-flow when the user selects "Continue without Pi".

📋 Suggested Follow-up Issues

Reviewed by Archon comprehensive-pr-review workflow · 5 specialized agents
Full artifacts: ~/.archon/workspaces/coleam00/Archon/artifacts/runs/e2ed11f61a3f2606d1acccc81e8ee9e8/review/

Fix Report — Review Findings Addressed

All 4 MEDIUM and actionable LOW findings from the code review have been resolved in commit 69dec8c.

Fixes Applied

Added a Pino lazy logger (getLog()) to setup.ts following the established doctor.ts pattern.

Validation

• ✅ Type check: all packages pass
• ✅ Lint: 0 warnings
• ✅ Tests: CLI 131 passed (pre-existing @archon/workflows failure is unrelated to this PR)