fix(cli): address Pi provider review findings in doctor and setup

Wirasm · Wirasm · commit 33b0c0e754af · 2026-05-07T18:03:31.000+03:00
- Add `probeAuthJsonExists` wrapper to doctor.ts so the existsSync call
  can be properly spied in tests (ESM named-import rebinding limitation,
  consistent with `probeFileExists` pattern in setup.ts)

- Fix `checkPi` false positive: shared keys like ANTHROPIC_API_KEY no
  longer count as Pi evidence unless DEFAULT_AI_ASSISTANT=pi; Claude-only
  users no longer see a spurious "pass" or "Pi: Configured" status

- Fix `writeHomePiModelConfig` to use `pathsGetArchonHome` from
  @archon/paths instead of the local `getArchonHome()`, so Docker
  environments (/.archon) are handled correctly

- Replace `includes('pi:')` substring check with `/^\s*pi\s*:/m` regex
  to prevent false positives from substrings like `api:`

- Fix `log.warn` → `log.warning` for consistency with rest of setup.ts

- Add `err.code` to the Pi model config write-failure warning message

- Update `checkPiModule` docstring to avoid "mirrors Claude binary check
  pattern" phrasing that misled maintainers

- Update `writeHomePiModelConfig` branch-1 docstring from "user-edited"
  to "idempotent" to accurately describe the skip condition

- Add `writeHomePiModelConfig` test suite covering all three branches,
  quote escaping, and the api:/pi: regex fix

- Update checkPi tests to spy on `probeAuthJsonExists` via doctorModule
  instead of fsModule.existsSync; add regression tests for M2 false positive

- Add `# Pi Authentication` section header assertion to mixed Claude+Pi
  generateEnvContent test
diff --git a/packages/cli/src/commands/doctor.test.ts b/packages/cli/src/commands/doctor.test.ts
@@ -11,7 +11,6 @@ import { describe, it, expect, spyOn, afterEach, beforeEach } from 'bun:test';
 import { tmpdir } from 'os';
 import { join } from 'path';
 import { mkdirSync, rmSync } from 'fs';
-import * as fsModule from 'fs';
 import * as git from '@archon/git';
 import {
   checkClaudeBinary,
@@ -25,6 +24,7 @@ import {
   doctorCommand,
   type DatabaseDeps,
 } from './doctor';
+import * as doctorModule from './doctor';
 import { checkPiModule } from './setup';
 
 describe('checkClaudeBinary', () => {
@@ -110,14 +110,18 @@ describe('checkGhAuth', () => {
 });
 
 describe('checkPi', () => {
-  let existsSpy: ReturnType<typeof spyOn<typeof fsModule, 'existsSync'>>;
+  // Spy on the exported `probeAuthJsonExists` wrapper rather than `fsModule.existsSync`.
+  // Named imports from 'fs' cannot be intercepted by spying on the namespace object
+  // due to ESM rebinding — the wrapper pattern (same as `probeFileExists` in setup.ts)
+  // is the correct way to make this testable.
+  let authJsonSpy: ReturnType<typeof spyOn<typeof doctorModule, 'probeAuthJsonExists'>>;
 
   beforeEach(() => {
-    existsSpy = spyOn(fsModule, 'existsSync');
+    authJsonSpy = spyOn(doctorModule, 'probeAuthJsonExists');
   });
 
   afterEach(() => {
-    existsSpy.mockRestore();
+    authJsonSpy.mockRestore();
   });
 
   it('returns skip when Pi is not configured', async () => {
@@ -128,14 +132,14 @@ describe('checkPi', () => {
   });
 
   it('returns pass when ~/.pi/agent/auth.json exists', async () => {
-    existsSpy.mockReturnValue(true);
+    authJsonSpy.mockReturnValue(true);
     const result = await checkPi({ DEFAULT_AI_ASSISTANT: 'pi' });
     expect(result.status).toBe('pass');
     expect(result.message).toContain('auth.json');
   });
 
   it('returns pass when a Pi API key env var is set', async () => {
-    existsSpy.mockReturnValue(false);
+    authJsonSpy.mockReturnValue(false);
     const result = await checkPi({
       DEFAULT_AI_ASSISTANT: 'pi',
       ANTHROPIC_API_KEY: 'sk-ant-test',
@@ -145,17 +149,26 @@ describe('checkPi', () => {
   });
 
   it('returns fail when DEFAULT_AI_ASSISTANT=pi but no auth found', async () => {
-    existsSpy.mockReturnValue(false);
+    authJsonSpy.mockReturnValue(false);
     const result = await checkPi({ DEFAULT_AI_ASSISTANT: 'pi' });
     expect(result.status).toBe('fail');
     expect(result.message).toContain('pi /login');
   });
 
-  it('returns pass when a Pi API key is set even when Pi is not the default', async () => {
-    existsSpy.mockReturnValue(false);
+  it('returns skip for Claude-only users who have ANTHROPIC_API_KEY but Pi is not default', async () => {
+    // Regression guard for M2: shared keys like ANTHROPIC_API_KEY must not be treated
+    // as Pi evidence unless DEFAULT_AI_ASSISTANT=pi.
+    authJsonSpy.mockReturnValue(false);
+    const result = await checkPi({ ANTHROPIC_API_KEY: 'sk-ant-test' });
+    expect(result.status).toBe('skip');
+    expect(result.message).toContain('not configured');
+  });
+
+  it('returns skip for users with OPENROUTER_API_KEY set but Pi not configured as default', async () => {
+    authJsonSpy.mockReturnValue(false);
     const result = await checkPi({ OPENROUTER_API_KEY: 'or-key' });
-    expect(result.status).toBe('pass');
-    expect(result.message).toContain('OPENROUTER_API_KEY');
+    expect(result.status).toBe('skip');
+    expect(result.message).toContain('not configured');
   });
 });
 
diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts
@@ -86,10 +86,22 @@ export async function checkGhAuth(env: NodeJS.ProcessEnv): Promise<CheckResult>
   }
 }
 
+/**
+ * Thin wrapper around `existsSync` so tests can spy on it by name without
+ * fighting ESM named-import rebinding limitations.  Matches the `probeFileExists`
+ * pattern in `setup.ts`.
+ */
+export function probeAuthJsonExists(path: string): boolean {
+  return existsSync(path);
+}
+
 export async function checkPi(env: NodeJS.ProcessEnv): Promise<CheckResult> {
   const label = 'Pi provider';
   const isDefault = env.DEFAULT_AI_ASSISTANT === 'pi';
-  const hasApiKey = PI_API_KEY_VARS.some(v => (env[v] ?? '').trim().length > 0);
+  // Only treat a shared key (e.g. ANTHROPIC_API_KEY) as Pi evidence when Pi is
+  // actually configured as the default — otherwise Claude-only users who happen
+  // to have ANTHROPIC_API_KEY set would get a false-positive "pass" here.
+  const hasApiKey = isDefault && PI_API_KEY_VARS.some(v => (env[v] ?? '').trim().length > 0);
 
   // Skip for users without Pi configured — same pattern as checkGhAuth.
   if (!isDefault && !hasApiKey) {
@@ -99,7 +111,7 @@ export async function checkPi(env: NodeJS.ProcessEnv): Promise<CheckResult> {
   // Pi reads OAuth credentials from ~/.pi/agent/auth.json (written by `pi /login`)
   // or API key env vars; either path is sufficient.
   const authJsonPath = join(homedir(), '.pi', 'agent', 'auth.json');
-  if (existsSync(authJsonPath)) {
+  if (probeAuthJsonExists(authJsonPath)) {
     return { label, status: 'pass', message: '~/.pi/agent/auth.json found' };
   }
 
diff --git a/packages/cli/src/commands/setup.test.ts b/packages/cli/src/commands/setup.test.ts
@@ -2,7 +2,7 @@
  * Tests for setup command utility functions
  */
 import { describe, it, expect, beforeEach, afterEach, spyOn, mock } from 'bun:test';
-import { existsSync, readFileSync, mkdirSync, writeFileSync, rmSync } from 'fs';
+import { existsSync, readFileSync, mkdirSync, mkdtempSync, writeFileSync, rmSync } from 'fs';
 import { join } from 'path';
 import { tmpdir } from 'os';
 import {
@@ -15,6 +15,7 @@ import {
   writeScopedEnv,
   serializeEnv,
   resolveScopedEnvPath,
+  writeHomePiModelConfig,
 } from './setup';
 import * as setupModule from './setup';
 import { copyArchonSkill } from './skill';
@@ -390,6 +391,7 @@ CODEX_ACCOUNT_ID=account1
       expect(content).toContain('CLAUDE_USE_GLOBAL_AUTH=true');
       expect(content).toContain('OPENROUTER_API_KEY=or-key');
       expect(content).toContain('DEFAULT_AI_ASSISTANT=claude');
+      expect(content).toContain('# Pi Authentication');
     });
   });
 
@@ -802,3 +804,71 @@ describe('writeScopedEnv (#1303)', () => {
     expect(result.preservedKeys).not.toContain('API_KEY');
   });
 });
+
+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('writes fresh block when config does not exist yet', () => {
+    // No config.yaml pre-created — function must create it.
+    writeHomePiModelConfig('openai/gpt-4o');
+    expect(existsSync(join(tmpDir, 'config.yaml'))).toBe(true);
+    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
+    expect(content).toContain('pi:');
+    expect(content).toContain('model: "openai/gpt-4o"');
+  });
+
+  it('skips write when existing config already contains pi: (idempotent)', () => {
+    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('does not write pi: block when config has assistants: but no pi: (shows note instead)', () => {
+    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');
+    // Should NOT have injected a pi: key — only a note() was shown.
+    expect(content).not.toContain('pi:');
+  });
+
+  it('escapes double quotes in the model name', () => {
+    writeHomePiModelConfig('vendor/"weird-model"');
+    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
+    expect(content).toContain('\\"weird-model\\"');
+  });
+
+  it('does not false-positive on api: substring (regex guard for includes bug)', () => {
+    // A config with `api:` but no `pi:` should fall through to the append branch,
+    // not the idempotent-skip branch.
+    writeFileSync(join(tmpDir, 'config.yaml'), '# config\napi:\n  key: abc\n');
+    writeHomePiModelConfig('openai/gpt-4o');
+    const content = readFileSync(join(tmpDir, 'config.yaml'), 'utf-8');
+    expect(content).toContain('pi:');
+  });
+});
diff --git a/packages/cli/src/commands/setup.ts b/packages/cli/src/commands/setup.ts
@@ -46,6 +46,7 @@ import { getRegisteredProviders } from '@archon/providers';
 import {
   getArchonEnvPath as pathsGetArchonEnvPath,
   getRepoArchonEnvPath as pathsGetRepoArchonEnvPath,
+  getArchonHome as pathsGetArchonHome,
 } from '@archon/paths';
 
 // =============================================================================
@@ -512,8 +513,8 @@ async function collectPiConfig(): Promise<{
 
 /**
  * Verify the Pi npm module is loadable. Pi is bundled as a transitive dep of
- * `@archon/providers` so this should always pass, but it mirrors the Claude
- * binary check pattern and catches broken compiled builds.
+ * `@archon/providers` so this should always pass, but it serves the same
+ * doctor-step role as `checkClaudeBinary` and catches broken compiled builds.
  *
  * The `loader` parameter is injected in tests so we don't need
  * `mock.module()` on `@archon/providers` (which would pollute other tests).
@@ -1574,18 +1575,22 @@ export function bootstrapProjectConfig(projectPath: string): BootstrapProjectCon
 /**
  * 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 (user-edited; don't overwrite).
+ *   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.
  */
 export function writeHomePiModelConfig(model: string): void {
-  const home = getArchonHome();
+  // Use the paths-package version of getArchonHome so Docker (/.archon) is
+  // handled correctly — the local getArchonHome() always returns ~/.archon.
+  const home = pathsGetArchonHome();
   mkdirSync(home, { recursive: true });
   const configPath = join(home, 'config.yaml');
   const existing = existsSync(configPath) ? readFileSync(configPath, 'utf-8') : '';
 
-  if (existing.includes('pi:')) {
+  // Use a regex to avoid false positives from substrings like `api:`.
+  if (/^\s*pi\s*:/m.test(existing)) {
     log.info(
       `Pi model already present in ${configPath} — edit assistants.pi.model manually to change.`
     );
@@ -2050,7 +2055,9 @@ export async function setupCommand(options: SetupOptions): Promise<void> {
     } catch (err) {
       // Non-fatal: env write already succeeded, so the user can hand-edit
       // ~/.archon/config.yaml later. Surface the error so it's not silent.
-      log.warn(`Could not write Pi model config: ${(err as NodeJS.ErrnoException).message}`);
+      const e = err as NodeJS.ErrnoException;
+      const code = e.code ? ` (${e.code})` : '';
+      log.warning(`Could not write Pi model config: ${e.message}${code}`);
     }
   }
 
