diff --git a/AGENTS.md b/AGENTS.md index d70bab1da..a483432ac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -339,7 +339,11 @@ Use sequential numbering with descriptive names (no leading zeros): **DEFAULT BEHAVIOR**: Consultation is ENABLED by default with: - **Gemini** via the **Antigravity CLI (`agy`)** for deep analysis (the retired Gemini CLI's replacement; OAuth/subscription, agy's default model — no pinned model id). Skips non-blockingly - if `agy` is missing/unauthenticated. + if `agy` is missing/unauthenticated. An unauthenticated `agy` is spawned **at most once per TTL + window** rather than once per consult: the verdict is cached across processes in + `~/.cache/codev/agy-auth.json`, because each spawn opens an OAuth browser tab before Codev can + detect the missing login (#1077). Sign in with `agy` in any terminal and the lane recovers on its + own; see `codev/resources/commands/consult.md` for the TTL/opt-out env vars. - **GPT-5.4 Codex** (gpt-5.4-codex) for coding and architecture perspective To disable: User must explicitly say "without multi-agent consultation" diff --git a/CLAUDE.md b/CLAUDE.md index d70bab1da..a483432ac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -339,7 +339,11 @@ Use sequential numbering with descriptive names (no leading zeros): **DEFAULT BEHAVIOR**: Consultation is ENABLED by default with: - **Gemini** via the **Antigravity CLI (`agy`)** for deep analysis (the retired Gemini CLI's replacement; OAuth/subscription, agy's default model — no pinned model id). Skips non-blockingly - if `agy` is missing/unauthenticated. + if `agy` is missing/unauthenticated. An unauthenticated `agy` is spawned **at most once per TTL + window** rather than once per consult: the verdict is cached across processes in + `~/.cache/codev/agy-auth.json`, because each spawn opens an OAuth browser tab before Codev can + detect the missing login (#1077). Sign in with `agy` in any terminal and the lane recovers on its + own; see `codev/resources/commands/consult.md` for the TTL/opt-out env vars. - **GPT-5.4 Codex** (gpt-5.4-codex) for coding and architecture perspective To disable: User must explicitly say "without multi-agent consultation" diff --git a/codev-skeleton/resources/commands/consult.md b/codev-skeleton/resources/commands/consult.md index dd2260127..72a06cd1c 100644 --- a/codev-skeleton/resources/commands/consult.md +++ b/codev-skeleton/resources/commands/consult.md @@ -170,6 +170,32 @@ Configure auth: - Gemini (`agy`): **OAuth / subscription** — run `agy` once and sign in (no API key). If `agy` is missing or unauthenticated, the gemini lane skips non-blockingly (the run proceeds without it). +#### agy auth-state cache (unauthenticated tab burst) + +An unauthenticated `agy` opens an OAuth browser tab *before* it prints the URL +Codev detects, so the lane cannot suppress the tab after spawning — only by not +spawning. Since a CMAP round is several independent `consult` processes, this used +to strand one tab per consult (#1077). + +Codev keeps a shared auth verdict in `~/.cache/codev/agy-auth.json` (0600, +honours `XDG_CACHE_HOME`). The first call probes; the rest read the verdict and +short-circuit, so **an unauthenticated agy is spawned at most once per TTL +window** instead of once per consult. A file lock keeps parallel consults from +probing simultaneously. `codev doctor`'s agy check shares the same cache. + +Verdicts expire on their own — sign in with `agy` in another terminal and the lane +recovers within the unauth TTL, no cache clearing required. The cache only ever +suppresses a lane on positive "signed out" evidence; anything ambiguous falls +through to a normal spawn. + +| Variable | Default | Purpose | +|---|---|---| +| `CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS` | `300000` (5 min) | How long a "signed out" verdict is trusted — the sign-in recovery window. | +| `CODEV_AGY_AUTH_CACHE_TTL_AUTH_MS` | `1800000` (30 min) | How long a "signed in" verdict is trusted. | +| `CODEV_AGY_AUTH_CACHE_WAIT_MS` | `6000` | How long a consult waits for another process's in-flight probe before proceeding anyway. | +| `CODEV_AGY_AUTH_CACHE_DIR` | `~/.cache/codev` | Cache location (mainly for tests). | +| `CODEV_AGY_AUTH_CACHE_DISABLE` | unset | `1` restores the always-spawn behaviour. | + ### Claude auth: subscription vs. metered API `consult -m claude` runs on the Claude Agent SDK. When `CLAUDE_CODE_OAUTH_TOKEN` diff --git a/codev/projects/bugfix-1077-consult-pre-flight-auth-state-/status.yaml b/codev/projects/bugfix-1077-consult-pre-flight-auth-state-/status.yaml new file mode 100644 index 000000000..585907425 --- /dev/null +++ b/codev/projects/bugfix-1077-consult-pre-flight-auth-state-/status.yaml @@ -0,0 +1,17 @@ +id: bugfix-1077 +title: consult-pre-flight-auth-state- +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-07-25T13:15:16.670Z' + approved_at: '2026-07-25T13:23:32.434Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-07-25T12:49:13.351Z' +updated_at: '2026-07-25T13:23:32.435Z' +pr_ready_for_human: false diff --git a/codev/resources/commands/consult.md b/codev/resources/commands/consult.md index dd2260127..72a06cd1c 100644 --- a/codev/resources/commands/consult.md +++ b/codev/resources/commands/consult.md @@ -170,6 +170,32 @@ Configure auth: - Gemini (`agy`): **OAuth / subscription** — run `agy` once and sign in (no API key). If `agy` is missing or unauthenticated, the gemini lane skips non-blockingly (the run proceeds without it). +#### agy auth-state cache (unauthenticated tab burst) + +An unauthenticated `agy` opens an OAuth browser tab *before* it prints the URL +Codev detects, so the lane cannot suppress the tab after spawning — only by not +spawning. Since a CMAP round is several independent `consult` processes, this used +to strand one tab per consult (#1077). + +Codev keeps a shared auth verdict in `~/.cache/codev/agy-auth.json` (0600, +honours `XDG_CACHE_HOME`). The first call probes; the rest read the verdict and +short-circuit, so **an unauthenticated agy is spawned at most once per TTL +window** instead of once per consult. A file lock keeps parallel consults from +probing simultaneously. `codev doctor`'s agy check shares the same cache. + +Verdicts expire on their own — sign in with `agy` in another terminal and the lane +recovers within the unauth TTL, no cache clearing required. The cache only ever +suppresses a lane on positive "signed out" evidence; anything ambiguous falls +through to a normal spawn. + +| Variable | Default | Purpose | +|---|---|---| +| `CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS` | `300000` (5 min) | How long a "signed out" verdict is trusted — the sign-in recovery window. | +| `CODEV_AGY_AUTH_CACHE_TTL_AUTH_MS` | `1800000` (30 min) | How long a "signed in" verdict is trusted. | +| `CODEV_AGY_AUTH_CACHE_WAIT_MS` | `6000` | How long a consult waits for another process's in-flight probe before proceeding anyway. | +| `CODEV_AGY_AUTH_CACHE_DIR` | `~/.cache/codev` | Cache location (mainly for tests). | +| `CODEV_AGY_AUTH_CACHE_DISABLE` | unset | `1` restores the always-spawn behaviour. | + ### Claude auth: subscription vs. metered API `consult -m claude` runs on the Claude Agent SDK. When `CLAUDE_CODE_OAUTH_TOKEN` diff --git a/codev/state/bugfix-1077_thread.md b/codev/state/bugfix-1077_thread.md new file mode 100644 index 000000000..1b0d2f707 --- /dev/null +++ b/codev/state/bugfix-1077_thread.md @@ -0,0 +1,127 @@ +# bugfix-1077 — agy auth-state pre-flight cache + +Issue #1077: consult's gemini lane spawns `agy` before it can know agy is +unauthenticated. Each spawn opens an OAuth browser tab (upstream agy behavior), +so an N-wide CMAP burst strands N tabs. The existing `AGY_OAUTH_MARKERS` kill +fires *after* the tab is already open. + +## Investigate + +- Confirmed the code path: `packages/codev/src/commands/consult/index.ts` + `runAgyConsultation` → `spawn(bin, args)` unconditionally; marker detection is + in the stdout/stderr `watch()` handler, i.e. post-spawn (index.ts:834-873). +- Confirmed the amplifiers named in the issue: + - CMAP-N dispatch emits one `consult -m ` process per model + (`porch/next.ts:529-531`), gemini lane = 1 agy spawn per round. + - Partial-review re-emit (`porch/next.ts:544+`) re-issues the same consult + commands, so a malformed/racy gemini skip causes another agy spawn. +- `doctor.ts:472 verifyAgy()` probes agy on every `codev doctor` run — same + tab-opening cost, separate entry point. +- Fix is cross-process, not in-process: each consult is its own OS process + (`bin/consult.js`), so a module-level memo would not help. Needs a + filesystem-backed cache + lock, as the issue proposes. +- agy credential path: not discoverable on this machine. `~/.gemini/antigravity-cli/` + holds settings/state but no credentials/token file; nothing in the login + keychain under "Antigravity". So mtime-based invalidation is best-effort over + a candidate list, and the TTL is the real recovery window (the issue + anticipates exactly this fallback). +- Scope: 1 new module + 2 integration points + tests. Well within BUGFIX. + +### Reproduced + +Fake unauthenticated agy (`CODEV_AGY_BIN` → script that logs each invocation, +then prints an `accounts.google.com/o/oauth2` banner on stderr like real agy), +5 parallel `consult -m gemini --prompt`: + +``` +=== AGY SPAWNS (== browser tabs): 5 +``` + +All 5 correctly emitted the non-blocking COMMENT skip — *after* spawning. That +is the bug precisely: the semantics are right, the timing is too late. + +### Detour: main's build was broken + +`doctor.ts` imported `formatBytes` from two modules (TS2300), landed via PR #1243. +I fixed it locally to unblock, then the architect asked me to drop it — they are +shipping it as a separate fast PR. Reverted; will merge main once that lands. +My branch stays scoped to the auth cache. + +### Design decision: the real run IS the probe + +The issue proposes a dedicated `--print-timeout 5s` probe when the cache is cold. +Implemented instead: the lock holder's *real* consult run doubles as the probe, +publishing the auth verdict as soon as it is knowable. Rationale: + +- No extra agy spawn per TTL window in the authenticated (common) case — a + dedicated probe would mean probe-spawn + real-spawn. +- Reuses the already-tested `AGY_OAUTH_MARKERS` detection instead of duplicating + marker logic in a second code path that could classify differently. +- Same observable guarantee the ACs ask for: at most 1 agy spawn per TTL window + when unauthenticated. + +Waiters poll the cache (they do NOT block on the lock, which the holder may hold +for minutes) and **fail open** — an undecided wait proceeds with the spawn. The +bias is deliberate: only positive marker evidence records `unauth`, so the +failure mode is "status-quo tab" and never "silently skip a working lane". + +`doctor.verifyAgy` becomes both consumer and producer of the same cache. + +## Fix + +Three commits: fix, tests, docs (+ a merge of main for the hotfix). + +Verified with a fake agy (`CODEV_AGY_BIN`) that logs every invocation, so spawn +counts are observed rather than mocked. Real 5-process bursts: + +| Scenario | Spawns | Wanted | +|---|---|---| +| unauthenticated, 5 parallel `consult -m gemini` | **1** | 1 | +| authenticated, 5 parallel | **5** | 5 (every lane is real work) | +| unauth verdict already cached | **0** | 0 | +| cache disabled, 3 calls | **3** | 3 (pre-fix behaviour) | + +Sign-in recovery also verified end-to-end: unauth cached → flip the fake to +authenticated → wait out the TTL → next call re-probes, finds `auth`, delivers a +review. No manual cache clear. + +### Test-pollution trap worth remembering + +The cache lives at a **user-global** path, so the first full-suite run wrote a +verdict into my own `~/.cache/codev` — and worse, a verdict recorded by one +doctor case made a sibling case stop spawning the thing it asserted on (one real +test failure, `doctor.test.ts` "passes the probe text immediately after --print"). + +Two-part fix: the consult/doctor suites pin `CODEV_AGY_AUTH_CACHE_DIR` into their +temp dirs, and the module itself is inert under `VITEST` unless a cache dir is +explicitly set. The guard is what makes this safe for *future* tests — otherwise +every new test that reaches `doctor()` silently re-acquires the hazard. + +Full suite: 3720 passed, 0 failed, and `~/.cache/codev` is not created by a test run. + +## Architect integration review — one real defect + +Both architect lanes APPROVE, and both my judgment calls (probe deviation, scope) +were accepted. One pre-merge change requested, and it was a genuine bug that +inverted the fail-safe bias I had *claimed* in the PR body: + +The lane guesses `auth` after a marker-free grace period (3s) so waiters are not +stuck behind a long review. That guess went through the once-only `publish`, so a +banner arriving later than the grace period left the guess standing — caching +`auth` for the full 30-min TTL and re-opening a tab on every consult in that +window. The fix silently reverted to the status quo on exactly the slow machines +that need it most. + +Investigating it surfaced a **second** case the architect had not named and I had +also missed: `publish` is inert for non-probers, so a caller that failed open, +spawned, and saw the banner wrote *nothing at all*. Even with ordering fixed, a +misfired guess would sit uncorrected until the TTL lapsed instead of being +corrected by the next spawn. + +Both fixed by recording `unauth` directly from the marker handler (42cf18e8). +Both new tests confirmed to FAIL against the previous code before restoring the +fix — a test that only passes with the fix proves much less. Suite now 3722. + +Lesson worth carrying: I wrote "only positive marker evidence records unauth" as a +design invariant, but never tested the *ordering* that invariant depends on. A +stated invariant with no test pinning it is a comment, not a property. diff --git a/packages/codev/src/__tests__/consult.test.ts b/packages/codev/src/__tests__/consult.test.ts index ac8cde623..8ff853e74 100644 --- a/packages/codev/src/__tests__/consult.test.ts +++ b/packages/codev/src/__tests__/consult.test.ts @@ -731,6 +731,11 @@ describe('consult command', () => { agyBin = path.join(testBaseDir, 'agy-fake'); fs.writeFileSync(agyBin, '#!/bin/sh\n'); process.env.CODEV_AGY_BIN = agyBin; + // The lane consults a cross-process auth cache before spawning (#1077). + // Pin it inside the per-test dir: otherwise a verdict recorded by one case + // changes whether the next one spawns at all, and the run would write into + // the developer's real ~/.cache/codev. + process.env.CODEV_AGY_AUTH_CACHE_DIR = path.join(testBaseDir, 'agy-auth-cache'); fs.mkdirSync(path.join(testBaseDir, 'codev', 'roles'), { recursive: true }); fs.writeFileSync( path.join(testBaseDir, 'codev', 'roles', 'consultant.md'), @@ -741,6 +746,7 @@ describe('consult command', () => { afterEach(() => { delete process.env.CODEV_AGY_BIN; + delete process.env.CODEV_AGY_AUTH_CACHE_DIR; }); async function loadAgy() { diff --git a/packages/codev/src/__tests__/doctor.test.ts b/packages/codev/src/__tests__/doctor.test.ts index 9292e9084..192d2df7d 100644 --- a/packages/codev/src/__tests__/doctor.test.ts +++ b/packages/codev/src/__tests__/doctor.test.ts @@ -717,15 +717,24 @@ describe('doctor command', () => { describe('AI model verification (Issue #128)', () => { const testBaseDir = path.join(tmpdir(), `codev-doctor-ai-test-${Date.now()}`); let originalCwd: string; + let savedAgyCacheDir: string | undefined; beforeEach(() => { originalCwd = process.cwd(); fs.mkdirSync(path.join(testBaseDir, 'codev', 'consult-types'), { recursive: true }); + // verifyAgy shares the consult lane's auth cache (#1077): pin it to a + // per-test directory so these cases neither read a verdict left by a + // sibling case (which would suppress the probe they assert on) nor write + // into the developer's real ~/.cache/codev. + savedAgyCacheDir = process.env.CODEV_AGY_AUTH_CACHE_DIR; + process.env.CODEV_AGY_AUTH_CACHE_DIR = path.join(testBaseDir, 'agy-auth-cache'); process.chdir(testBaseDir); }); afterEach(() => { process.chdir(originalCwd); + if (savedAgyCacheDir === undefined) delete process.env.CODEV_AGY_AUTH_CACHE_DIR; + else process.env.CODEV_AGY_AUTH_CACHE_DIR = savedAgyCacheDir; if (fs.existsSync(testBaseDir)) { fs.rmSync(testBaseDir, { recursive: true }); } diff --git a/packages/codev/src/commands/consult/__tests__/agy-auth-cache.test.ts b/packages/codev/src/commands/consult/__tests__/agy-auth-cache.test.ts new file mode 100644 index 000000000..196725b3e --- /dev/null +++ b/packages/codev/src/commands/consult/__tests__/agy-auth-cache.test.ts @@ -0,0 +1,391 @@ +/** + * Regression tests for the agy auth-state pre-flight cache (Issue #1077). + * + * The bug: an unauthenticated `agy` opens an OAuth browser tab *before* printing + * the OAuth URL that the gemini lane detects, so post-spawn detection cannot stop + * the tab. A CMAP burst is N separate consult processes, so N tabs landed + * (12-15 observed in the wild). + * + * The lane's guarantee, pinned below: across a burst against an unauthenticated + * agy, **agy is spawned at most once per TTL window**. Every other call decides + * from the shared filesystem cache and never spawns. + * + * These tests use a fake `agy` (via `CODEV_AGY_BIN`) that records every + * invocation to a log file — so "did we spawn?" is asserted against observed + * process starts, not mocks. Because the cache and lock live entirely on disk, + * concurrent in-process calls exercise exactly the same coordination path that + * separate `consult` processes do. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { + checkCachedAgyAuth, + recordAgyAuthState, + clearAgyAuthCache, + preflightAgyAuth, + waitForAgyAuthState, + agyAuthCacheDir, +} from '../agy-auth-cache.js'; +import { _runAgyConsultation } from '../index.js'; + +/** Env keys this suite manipulates; restored wholesale after each test. */ +const ENV_KEYS = [ + 'CODEV_AGY_BIN', + 'CODEV_AGY_AUTH_CACHE_DIR', + 'CODEV_AGY_AUTH_CACHE_DISABLE', + 'CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS', + 'CODEV_AGY_AUTH_CACHE_TTL_AUTH_MS', + 'CODEV_AGY_AUTH_CACHE_WAIT_MS', + 'FAKE_AGY_LOG', + 'FAKE_AGY_MODE', + 'FAKE_AGY_BANNER_DELAY_MS', +] as const; + +let dir: string; +let savedEnv: Record; + +/** Absolute path of the fake agy binary for the current test. */ +let fakeAgy: string; +/** File the fake agy appends to on every invocation. */ +let spawnLog: string; + +/** + * A stand-in for the `agy` CLI. `FAKE_AGY_MODE=unauth` reproduces the upstream + * behaviour this issue is about: print the OAuth banner, then sit there waiting + * for an interactive login that can never complete headlessly. + */ +const FAKE_AGY_SOURCE = `#!/usr/bin/env node +const fs = require('node:fs'); +fs.appendFileSync(process.env.FAKE_AGY_LOG, process.pid + '\\n'); +if (process.argv[2] === '--version') { console.log('1.0.10-fake'); process.exit(0); } +if (process.env.FAKE_AGY_MODE === 'unauth' || process.env.FAKE_AGY_MODE === 'unauth-slow') { + // 'unauth-slow' emits the banner *after* the lane's marker-free grace period, + // the case that used to leave a false 'auth' verdict cached. + const delay = process.env.FAKE_AGY_MODE === 'unauth-slow' ? Number(process.env.FAKE_AGY_BANNER_DELAY_MS) : 0; + setTimeout(() => { + process.stderr.write('Please visit https://accounts.google.com/o/oauth2/auth?client_id=fake\\n'); + }, delay); + setTimeout(() => process.exit(1), 30000); // hang like real agy; the lane kills us +} else { + process.stdout.write('---\\nVERDICT: APPROVE\\nSUMMARY: ok\\nCONFIDENCE: HIGH\\n---\\n'); + process.exit(0); +} +`; + +/** Invocations of the fake agy so far — i.e. how many browser tabs would exist. */ +function spawnCount(): number { + if (!fs.existsSync(spawnLog)) return 0; + return fs.readFileSync(spawnLog, 'utf-8').split('\n').filter(Boolean).length; +} + +function setMode(mode: 'auth' | 'unauth'): void { + process.env.FAKE_AGY_MODE = mode; +} + +beforeEach(() => { + savedEnv = {}; + for (const k of ENV_KEYS) savedEnv[k] = process.env[k]; + + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'agy-auth-cache-test-')); + fakeAgy = path.join(dir, 'agy'); + fs.writeFileSync(fakeAgy, FAKE_AGY_SOURCE, { mode: 0o755 }); + spawnLog = path.join(dir, 'spawns.log'); + fs.writeFileSync(spawnLog, ''); + + process.env.CODEV_AGY_BIN = fakeAgy; + process.env.CODEV_AGY_AUTH_CACHE_DIR = path.join(dir, 'cache'); + process.env.FAKE_AGY_LOG = spawnLog; + delete process.env.CODEV_AGY_AUTH_CACHE_DISABLE; +}); + +afterEach(() => { + for (const k of ENV_KEYS) { + if (savedEnv[k] === undefined) delete process.env[k]; + else process.env[k] = savedEnv[k]; + } + fs.rmSync(dir, { recursive: true, force: true }); + vi.restoreAllMocks(); +}); + +describe('agy auth-state cache', () => { + it('reports nothing on a cold cache', () => { + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('round-trips both states', () => { + recordAgyAuthState('unauth', fakeAgy); + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + recordAgyAuthState('auth', fakeAgy); + expect(checkCachedAgyAuth(fakeAgy)).toBe('auth'); + }); + + it('writes the cache file 0600 in a 0700 directory', () => { + recordAgyAuthState('unauth', fakeAgy); + const cacheFile = path.join(agyAuthCacheDir(), 'agy-auth.json'); + expect(fs.statSync(cacheFile).mode & 0o777).toBe(0o600); + expect(fs.statSync(agyAuthCacheDir()).mode & 0o777).toBe(0o700); + }); + + it('expires an unauth verdict once its TTL lapses', async () => { + process.env.CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS = '20'; + recordAgyAuthState('unauth', fakeAgy); + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + await new Promise((r) => setTimeout(r, 40)); + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('applies the auth and unauth TTLs independently', async () => { + process.env.CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS = '20'; + process.env.CODEV_AGY_AUTH_CACHE_TTL_AUTH_MS = '60000'; + recordAgyAuthState('auth', fakeAgy); + await new Promise((r) => setTimeout(r, 40)); + // Past the *unauth* TTL but well inside the auth one. + expect(checkCachedAgyAuth(fakeAgy)).toBe('auth'); + }); + + it('ignores a verdict recorded for a different agy binary', () => { + recordAgyAuthState('unauth', '/some/other/agy'); + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('ignores a verdict whose credential-mtime hint no longer matches', () => { + // Simulates the user completing OAuth: agy rewrites its credentials, so the + // recorded hint goes stale and the verdict must not be trusted. + fs.mkdirSync(agyAuthCacheDir(), { recursive: true }); + fs.writeFileSync( + path.join(agyAuthCacheDir(), 'agy-auth.json'), + JSON.stringify({ state: 'unauth', checkedAt: Date.now(), agyBinPath: fakeAgy, agyAuthMtime: 12345 }), + ); + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('treats a corrupt cache file as cold rather than throwing', () => { + fs.mkdirSync(agyAuthCacheDir(), { recursive: true }); + fs.writeFileSync(path.join(agyAuthCacheDir(), 'agy-auth.json'), '{not json'); + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('rejects an entry with an unrecognised state', () => { + fs.mkdirSync(agyAuthCacheDir(), { recursive: true }); + fs.writeFileSync( + path.join(agyAuthCacheDir(), 'agy-auth.json'), + JSON.stringify({ state: 'maybe', checkedAt: Date.now(), agyBinPath: fakeAgy, agyAuthMtime: null }), + ); + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('goes inert when disabled, neither reading nor writing', () => { + recordAgyAuthState('unauth', fakeAgy); + process.env.CODEV_AGY_AUTH_CACHE_DISABLE = '1'; + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + recordAgyAuthState('auth', fakeAgy); // must not overwrite + delete process.env.CODEV_AGY_AUTH_CACHE_DISABLE; + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + }); + + it('clears the cache on request', () => { + recordAgyAuthState('unauth', fakeAgy); + clearAgyAuthCache(); + expect(checkCachedAgyAuth(fakeAgy)).toBeNull(); + }); + + it('stops waiting and returns null when no verdict arrives', async () => { + const started = Date.now(); + expect(await waitForAgyAuthState(fakeAgy, 150)).toBeNull(); + expect(Date.now() - started).toBeGreaterThanOrEqual(140); + }); + + it('returns the verdict as soon as another process publishes it', async () => { + setTimeout(() => recordAgyAuthState('unauth', fakeAgy), 50); + expect(await waitForAgyAuthState(fakeAgy, 3000)).toBe('unauth'); + }); +}); + +describe('agy auth pre-flight', () => { + it('elects exactly one prober among concurrent callers', async () => { + // Nothing publishes a verdict here, so the losers ride out the fail-open + // wait — shorten it so the test does not sit through the real one. + process.env.CODEV_AGY_AUTH_CACHE_WAIT_MS = '150'; + const results = await Promise.all([ + preflightAgyAuth(fakeAgy), + preflightAgyAuth(fakeAgy), + preflightAgyAuth(fakeAgy), + ]); + // Losers fall through to the (short) wait, then fail open — so they all say + // "proceed", but only one of them is on the hook to publish a verdict. + expect(results.filter((r) => r.isProber)).toHaveLength(1); + results.forEach((r) => r.release()); + }); + + it('skips without probing when a fresh unauth verdict exists', async () => { + recordAgyAuthState('unauth', fakeAgy); + const pre = await preflightAgyAuth(fakeAgy); + expect(pre.action).toBe('skip'); + expect(pre.reason).toMatch(/cached/); + expect(pre.isProber).toBe(false); + }); + + it('proceeds without probing when a fresh auth verdict exists', async () => { + recordAgyAuthState('auth', fakeAgy); + const pre = await preflightAgyAuth(fakeAgy); + expect(pre.action).toBe('proceed'); + expect(pre.isProber).toBe(false); + }); + + it('releases the lock so the next caller can probe', async () => { + const first = await preflightAgyAuth(fakeAgy); + expect(first.isProber).toBe(true); + first.release(); + const second = await preflightAgyAuth(fakeAgy); + expect(second.isProber).toBe(true); + second.release(); + }); + + it('reclaims a lock abandoned by a crashed prober', async () => { + const lock = path.join(agyAuthCacheDir(), 'agy-auth.lock'); + fs.mkdirSync(agyAuthCacheDir(), { recursive: true }); + fs.writeFileSync(lock, '99999 0\n'); + // Backdate past the staleness threshold (60s). + const old = new Date(Date.now() - 5 * 60 * 1000); + fs.utimesSync(lock, old, old); + + const pre = await preflightAgyAuth(fakeAgy); + expect(pre.isProber).toBe(true); + pre.release(); + }); + + it('is inert when the cache is disabled', async () => { + process.env.CODEV_AGY_AUTH_CACHE_DISABLE = '1'; + recordAgyAuthState('unauth', fakeAgy); // no-op while disabled + const pre = await preflightAgyAuth(fakeAgy); + expect(pre.action).toBe('proceed'); + expect(pre.isProber).toBe(false); + }); +}); + +describe('gemini lane burst behaviour (#1077 regression)', () => { + /** Silence the lane's review/skip output so test logs stay readable. */ + function muteStdout() { + vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + vi.spyOn(console, 'error').mockImplementation(() => {}); + } + + async function runLane(n: number): Promise { + await Promise.all( + Array.from({ length: n }, (_, i) => + _runAgyConsultation('review this', 'be terse', dir, path.join(dir, `review-${i}.txt`)), + ), + ); + } + + it('spawns agy at most once across a 5-wide burst when unauthenticated', async () => { + setMode('unauth'); + muteStdout(); + + await runLane(5); + + // The heart of #1077: one spawn means one browser tab, not five. + expect(spawnCount()).toBe(1); + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + }, 30_000); + + it('still emits a non-blocking COMMENT skip for every call it short-circuits', async () => { + setMode('unauth'); + muteStdout(); + + await runLane(5); + + // A skipped lane must never silently produce nothing — porch aggregates these. + for (let i = 0; i < 5; i++) { + const review = fs.readFileSync(path.join(dir, `review-${i}.txt`), 'utf-8'); + expect(review).toContain('VERDICT: COMMENT'); + expect(review).toMatch(/Gemini lane skipped/); + } + }, 30_000); + + it('does not spawn at all once the unauth verdict is cached', async () => { + setMode('unauth'); + muteStdout(); + recordAgyAuthState('unauth', fakeAgy); + + await runLane(3); + + expect(spawnCount()).toBe(0); + }, 30_000); + + it('re-probes after the unauth TTL lapses, recovering once the user signs in', async () => { + process.env.CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS = '50'; + setMode('unauth'); + muteStdout(); + + await runLane(1); + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + + // User completes OAuth in another window; the stale verdict times out. + setMode('auth'); + await new Promise((r) => setTimeout(r, 80)); + + await runLane(1); + expect(checkCachedAgyAuth(fakeAgy)).toBe('auth'); + const review = fs.readFileSync(path.join(dir, 'review-0.txt'), 'utf-8'); + expect(review).toContain('VERDICT: APPROVE'); + }, 30_000); + + it('runs every lane in a burst when authenticated (no reviewer dropped)', async () => { + setMode('auth'); + muteStdout(); + + await runLane(5); + + // Each call is real work and must reach agy; the cache must not suppress any. + expect(spawnCount()).toBe(5); + for (let i = 0; i < 5; i++) { + expect(fs.readFileSync(path.join(dir, `review-${i}.txt`), 'utf-8')).toContain('VERDICT: APPROVE'); + } + }, 30_000); + + it('lets a late OAuth banner overwrite the speculative auth verdict', async () => { + // The lane guesses `auth` after a marker-free grace period so waiters are not + // stuck behind a long review. If agy is merely slow to print its banner, that + // guess is wrong — and left standing it would cache `auth` for the full auth + // TTL, re-opening a tab on every consult in that window. Positive marker + // evidence must win regardless of arrival order. + setMode('unauth-slow'); + process.env.FAKE_AGY_BANNER_DELAY_MS = '3400'; // > the lane's 3s grace + muteStdout(); + + await runLane(1); + + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + }, 30_000); + + it('records the verdict even when the run that sees the banner is not the prober', async () => { + // A caller that fails open (another process holds the lock) still spawns, so + // it can be the one to observe the banner. Dropping that evidence would leave + // a misfired guess uncorrected until the TTL lapsed. + fs.mkdirSync(agyAuthCacheDir(), { recursive: true }); + fs.writeFileSync(path.join(agyAuthCacheDir(), 'agy-auth.lock'), '99999 0\n'); + process.env.CODEV_AGY_AUTH_CACHE_WAIT_MS = '200'; // fail open promptly + setMode('unauth'); + muteStdout(); + + await runLane(1); + + expect(checkCachedAgyAuth(fakeAgy)).toBe('unauth'); + }, 30_000); + + it('keeps spawning per call when the cache is disabled (pre-#1077 behaviour)', async () => { + process.env.CODEV_AGY_AUTH_CACHE_DISABLE = '1'; + setMode('unauth'); + muteStdout(); + + await runLane(3); + + // The escape hatch genuinely restores the old always-spawn path — this is + // also the assertion that proves the burst test above measures the cache. + expect(spawnCount()).toBe(3); + }, 30_000); +}); diff --git a/packages/codev/src/commands/consult/agy-auth-cache.ts b/packages/codev/src/commands/consult/agy-auth-cache.ts new file mode 100644 index 000000000..16a6159cc --- /dev/null +++ b/packages/codev/src/commands/consult/agy-auth-cache.ts @@ -0,0 +1,344 @@ +/** + * Cross-process auth-state cache for the Antigravity CLI (`agy`) — Issue #1077. + * + * ## Why this exists + * + * An unauthenticated `agy --print` opens an OAuth browser tab *before* it prints + * the OAuth URL that `AGY_OAUTH_MARKERS` detection keys off. So by the time the + * gemini consult lane recognises "not signed in" and kills the child, the tab is + * already on screen. A CMAP burst runs N consults as N separate OS processes, so + * N tabs land — 12-15 stranded tabs were observed in the wild. + * + * A module-level memo cannot help: each `consult` invocation is its own process. + * The state has to be shared through the filesystem. + * + * ## The protocol + * + * 1. Read `~/.cache/codev/agy-auth.json`. A *fresh* entry decides immediately: + * `unauth` → skip without spawning; `auth` → spawn as usual. + * 2. Cold or expired cache → race for an exclusive lock. Exactly one process + * wins and becomes the **prober**: it spawns agy for real and publishes the + * verdict the moment it is knowable (see `runAgyConsultation`). + * 3. Losers do **not** block on the lock — the prober may hold it for minutes on + * a real review. They poll the cache for a few seconds and act on the verdict + * as soon as it appears. + * + * ## Fail-open bias + * + * Only positive OAuth-marker evidence records `unauth`. A waiter that never sees + * a verdict proceeds with its spawn. Both choices push failures toward "one + * unnecessary browser tab" (the status quo) and away from "silently skipped a + * working reviewer", which would corrupt a CMAP round. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import { homedir } from 'node:os'; + +export type AgyAuthState = 'auth' | 'unauth'; + +/** Cache entry persisted to `agy-auth.json`. */ +interface AgyAuthCacheEntry { + state: AgyAuthState; + /** epoch ms of the probe that produced `state`. */ + checkedAt: number; + /** Binary the verdict applies to — a different binary invalidates it. */ + agyBinPath: string; + /** Newest mtime across agy's candidate credential files, or null if none found. */ + agyAuthMtime: number | null; +} + +/** + * `unauth` expires fast: a user who signs in elsewhere should get the lane back + * without clearing anything by hand. `auth` lasts longer because re-probing a + * working agy is pure overhead. + */ +const DEFAULT_TTL_UNAUTH_MS = 5 * 60 * 1000; +const DEFAULT_TTL_AUTH_MS = 30 * 60 * 1000; + +/** How long a waiter polls for the prober's verdict before failing open. */ +const DEFAULT_WAIT_MS = 6000; +const WAIT_POLL_INTERVAL_MS = 100; + +/** + * A lock older than this is treated as abandoned (prober crashed / was killed + * mid-run) and reclaimed. Generous enough to cover a prober that is still + * deciding, short enough that a crash does not wedge the lane for a whole TTL. + */ +const LOCK_STALE_MS = 60 * 1000; + +/** + * Files agy plausibly writes when an OAuth login completes. Purely a *hint*: a + * changed mtime invalidates the cache early, so a sign-in is picked up before + * the TTL lapses. agy's credential location is not documented and has moved + * between releases, so this list is best-effort by design and every entry is + * optional — when none exist, `agyAuthMtime` is null and the TTL is the only + * recovery window (which is sufficient on its own). + * + * Deliberately excludes general settings/state files: those churn for unrelated + * reasons, and a spurious invalidation costs a browser tab. + */ +const AGY_CREDENTIAL_CANDIDATES = [ + ['.antigravity', 'credentials.json'], + ['Library', 'Application Support', 'Antigravity', 'credentials.json'], + ['.gemini', 'antigravity-cli', 'credentials.json'], + ['.gemini', 'antigravity-cli', 'oauth_creds.json'], + ['.gemini', 'oauth_creds.json'], + ['.gemini', 'google_accounts.json'], +]; + +function envInt(name: string, fallback: number): number { + const raw = process.env[name]; + if (!raw) return fallback; + const parsed = Number.parseInt(raw, 10); + return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback; +} + +function ttlFor(state: AgyAuthState): number { + return state === 'unauth' + ? envInt('CODEV_AGY_AUTH_CACHE_TTL_UNAUTH_MS', DEFAULT_TTL_UNAUTH_MS) + : envInt('CODEV_AGY_AUTH_CACHE_TTL_AUTH_MS', DEFAULT_TTL_AUTH_MS); +} + +/** + * Opt-out escape hatch: restores the pre-#1077 always-spawn behaviour. + * + * Also inert under a test runner unless the suite names a cache directory. This + * cache is a *user-global* side effect: without the guard, any test that reaches + * `doctor()` or the gemini lane would record a verdict into the developer's real + * `~/.cache/codev` and, worse, later tests would silently take the cached branch + * and stop spawning what they assert on. Suites that mean to exercise the cache + * set `CODEV_AGY_AUTH_CACHE_DIR` and get the full behaviour. + */ +export function agyAuthCacheDisabled(): boolean { + const raw = process.env.CODEV_AGY_AUTH_CACHE_DISABLE; + if (raw === '1' || raw === 'true') return true; + return Boolean(process.env.VITEST) && !process.env.CODEV_AGY_AUTH_CACHE_DIR; +} + +/** + * Cache directory. `CODEV_AGY_AUTH_CACHE_DIR` keeps tests off the real user + * cache; otherwise XDG_CACHE_HOME, else `~/.cache`. + */ +export function agyAuthCacheDir(): string { + const override = process.env.CODEV_AGY_AUTH_CACHE_DIR; + if (override) return override; + const xdg = process.env.XDG_CACHE_HOME; + return path.join(xdg && xdg.startsWith('/') ? xdg : path.join(homedir(), '.cache'), 'codev'); +} + +function cacheFilePath(): string { + return path.join(agyAuthCacheDir(), 'agy-auth.json'); +} + +function lockFilePath(): string { + return path.join(agyAuthCacheDir(), 'agy-auth.lock'); +} + +/** Newest mtime across the candidate credential files; null when none exist. */ +function currentAgyAuthMtime(): number | null { + let newest: number | null = null; + for (const parts of AGY_CREDENTIAL_CANDIDATES) { + try { + const stat = fs.statSync(path.join(homedir(), ...parts)); + const mtime = stat.mtimeMs; + if (newest === null || mtime > newest) newest = mtime; + } catch { + // Absent or unreadable — this list is a hint, not a requirement. + } + } + return newest; +} + +function readEntry(): AgyAuthCacheEntry | null { + try { + const parsed = JSON.parse(fs.readFileSync(cacheFilePath(), 'utf-8')) as unknown; + if (!parsed || typeof parsed !== 'object') return null; + const e = parsed as Partial; + if (e.state !== 'auth' && e.state !== 'unauth') return null; + if (typeof e.checkedAt !== 'number' || !Number.isFinite(e.checkedAt)) return null; + if (typeof e.agyBinPath !== 'string') return null; + return { + state: e.state, + checkedAt: e.checkedAt, + agyBinPath: e.agyBinPath, + agyAuthMtime: typeof e.agyAuthMtime === 'number' ? e.agyAuthMtime : null, + }; + } catch { + // Missing, truncated, or concurrently-rewritten file — treat as cold. + return null; + } +} + +/** + * The cached auth state for `agyBinPath`, or null when there is nothing + * trustworthy to act on (cold, expired, different binary, or a credential file + * changed since the probe). + */ +export function checkCachedAgyAuth(agyBinPath: string): AgyAuthState | null { + if (agyAuthCacheDisabled()) return null; + const entry = readEntry(); + if (!entry) return null; + // A reinstall or a CODEV_AGY_BIN change means the verdict describes a + // different binary than the one we are about to run. + if (entry.agyBinPath !== agyBinPath) return null; + if (Date.now() - entry.checkedAt >= ttlFor(entry.state)) return null; + // Credentials moved under us → someone signed in (or out); re-probe. + if (entry.agyAuthMtime !== currentAgyAuthMtime()) return null; + return entry.state; +} + +/** + * Persist a verdict. Written via a temp file + rename so a concurrent reader + * never observes a half-written JSON document. + */ +export function recordAgyAuthState(state: AgyAuthState, agyBinPath: string): void { + if (agyAuthCacheDisabled()) return; + const entry: AgyAuthCacheEntry = { + state, + checkedAt: Date.now(), + agyBinPath, + agyAuthMtime: currentAgyAuthMtime(), + }; + try { + const dir = agyAuthCacheDir(); + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + const tmp = path.join(dir, `agy-auth.${process.pid}.tmp`); + fs.writeFileSync(tmp, JSON.stringify(entry), { mode: 0o600 }); + fs.renameSync(tmp, cacheFilePath()); + } catch { + // A cache we cannot write is a missed optimisation, never a failed consult. + } +} + +/** Test/diagnostic helper: drop the cache file and any lock. */ +export function clearAgyAuthCache(): void { + for (const p of [cacheFilePath(), lockFilePath()]) { + try { fs.unlinkSync(p); } catch { /* already gone */ } + } +} + +/** + * Try to become the prober. Returns a release function on success, null when + * another process holds the lock. An abandoned lock (older than LOCK_STALE_MS) + * is reclaimed once. + */ +function acquireProbeLock(): (() => void) | null { + const lockPath = lockFilePath(); + const attempt = (): boolean => { + try { + fs.mkdirSync(agyAuthCacheDir(), { recursive: true, mode: 0o700 }); + // 'wx' fails when the file exists — the atomic test-and-set we need. + const fd = fs.openSync(lockPath, 'wx', 0o600); + fs.writeSync(fd, `${process.pid} ${Date.now()}\n`); + fs.closeSync(fd); + return true; + } catch { + return false; + } + }; + + if (attempt()) return () => { try { fs.unlinkSync(lockPath); } catch { /* already gone */ } }; + + try { + if (Date.now() - fs.statSync(lockPath).mtimeMs > LOCK_STALE_MS) { + fs.unlinkSync(lockPath); + if (attempt()) return () => { try { fs.unlinkSync(lockPath); } catch { /* already gone */ } }; + } + } catch { + // Lock vanished mid-inspection (holder finished) — fall through to waiting. + } + return null; +} + +/** + * Poll for the prober's verdict. Resolves with the state as soon as one lands, + * or null if the wait elapses first. + * + * Polls the *cache* rather than waiting on the lock: the prober holds the lock + * for its entire agy run, which can be minutes, but publishes its verdict within + * seconds. + */ +export async function waitForAgyAuthState( + agyBinPath: string, + timeoutMs = envInt('CODEV_AGY_AUTH_CACHE_WAIT_MS', DEFAULT_WAIT_MS), +): Promise { + const deadline = Date.now() + timeoutMs; + for (;;) { + const state = checkCachedAgyAuth(agyBinPath); + if (state) return state; + if (Date.now() >= deadline) return null; + await new Promise((r) => setTimeout(r, WAIT_POLL_INTERVAL_MS)); + } +} + +export interface AgyAuthPreflight { + /** `skip` → do not spawn agy at all; `proceed` → spawn. */ + action: 'skip' | 'proceed'; + /** Skip reason, suitable for a user-facing message. Set iff action is `skip`. */ + reason?: string; + /** + * True when this process won the probe lock. Such a process MUST report what + * it learns via `publish` — every other process is waiting on it. + */ + isProber: boolean; + /** + * Record a verdict and release the lock. Idempotent, and a no-op for + * non-probers, so callers can invoke it from every exit path unconditionally. + */ + publish: (state: AgyAuthState) => void; + /** Release the lock without recording anything (abnormal exits). Idempotent. */ + release: () => void; +} + +const SKIP_REASON_CACHED_UNAUTH = + 'agy unauthenticated (cached); run `agy` once to sign in'; + +/** + * Decide whether to spawn agy, before spawning it. + * + * Callers that receive `proceed` with `isProber: true` are on the hook to + * `publish` what the spawn reveals; see the module header for the full protocol. + */ +export async function preflightAgyAuth(agyBinPath: string): Promise { + const inert: AgyAuthPreflight = { + action: 'proceed', + isProber: false, + publish: () => {}, + release: () => {}, + }; + if (agyAuthCacheDisabled()) return inert; + + const cached = checkCachedAgyAuth(agyBinPath); + if (cached === 'unauth') { + return { ...inert, action: 'skip', reason: SKIP_REASON_CACHED_UNAUTH }; + } + if (cached === 'auth') return inert; + + const release = acquireProbeLock(); + if (release) { + let done = false; + const finish = (state?: AgyAuthState) => { + if (done) return; + done = true; + if (state) recordAgyAuthState(state, agyBinPath); + release(); + }; + return { + action: 'proceed', + isProber: true, + publish: (state) => finish(state), + release: () => finish(), + }; + } + + // Another process is probing. Wait briefly for its verdict rather than adding + // a spawn (and a browser tab) of our own. + const observed = await waitForAgyAuthState(agyBinPath); + if (observed === 'unauth') { + return { ...inert, action: 'skip', reason: SKIP_REASON_CACHED_UNAUTH }; + } + // `auth`, or no verdict in time: proceed. Failing open keeps a slow or crashed + // prober from silently dropping a reviewer that would have worked. + return inert; +} diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 28d3bfc0a..ca6fc7eb8 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -21,6 +21,7 @@ import { getResolver, GitRefResolver, type ArtifactResolver } from '../porch/art import { MetricsDB } from './metrics.js'; import { extractUsage, extractReviewText, type SDKResultLike, type UsageData } from './usage-extractor.js'; import { executeForgeCommandSync } from '../../lib/forge.js'; +import { preflightAgyAuth, recordAgyAuthState, type AgyAuthState } from './agy-auth-cache.js'; // Content reference — resolved artifact content with a display label interface ContentRef { @@ -635,6 +636,14 @@ const AGY_PRINT_TIMEOUT = '5m'; // passed to `agy --print-timeou const AGY_TIMEOUT_MS = 6 * 60 * 1000; // Codev-owned hard cap (> agy's own timeout) // OAuth banner appears before any review text; only scan the early stream. const AGY_MARKER_SCAN_LIMIT = 8192; +/** + * How long a prober waits, marker-free, before publishing `auth` to the shared + * cache (#1077). The OAuth banner is the very first thing an unauthenticated agy + * emits, so silence this far in is good evidence we are signed in — and it lets + * the processes waiting on our verdict get moving instead of stalling behind a + * review that may run for minutes. + */ +const AGY_AUTH_GRACE_MS = 3000; // agy's own print-timeout message: on an agentic task that outruns --print-timeout, // it returns this (often with a "monitoring the task" note) instead of a review. // Treat it as a non-response → non-blocking skip rather than a garbage "review". @@ -788,6 +797,21 @@ async function runAgyConsultation( return; } + // Pre-flight the shared auth cache BEFORE spawning (#1077). An unauthenticated + // agy opens a browser tab before it prints the OAuth URL we detect below, so + // post-spawn detection cannot prevent the tab — only not spawning can. Across a + // CMAP burst this collapses N stranded tabs into at most one per TTL window. + const preflight = await preflightAgyAuth(bin); + if (preflight.action === 'skip') { + const reason = preflight.reason ?? 'agy unauthenticated (cached)'; + const content = agySkipContent(reason); + process.stdout.write(content); + writeConsultOutput(outputPath, content); + recordAgyMetrics(metricsCtx, startTime, 0, reason); + console.error(`\n[gemini (agy) skipped without spawning: ${reason}]`); + return; + } + // agy has no system-prompt flag — fold the role into the prompt (hermes precedent). // Prepend strict constraint directive to force agy to remain on-task, prevent exploratory actions // that lead to wandering off-task, and avoid unrelated local skills/directories (Issue #1032). @@ -841,12 +865,28 @@ async function runAgyConsultation( let scanBuf = ''; let settled = false; + // When we hold the probe lock, other consult processes are polling the cache + // for our verdict — publish it as soon as it is knowable, and always release + // the lock, whatever this run turns into. + const graceTimer = preflight.isProber + ? setTimeout(() => preflight.publish('auth'), AGY_AUTH_GRACE_MS) + : null; + const publishAuth = (state?: AgyAuthState) => { + if (graceTimer) clearTimeout(graceTimer); + if (state) preflight.publish(state); + else preflight.release(); + }; + const settleSkip = (reason: string, exitCode = 0) => { if (settled) return; settled = true; clearTimeout(timer); try { proc.kill('SIGTERM'); } catch { /* already gone */ } cleanup(); + // No-op if a verdict was already published (e.g. the OAuth-marker path); + // otherwise this just releases the lock — a timeout or a spawn failure is + // not evidence either way about authentication. + publishAuth(); const content = agySkipContent(reason); process.stdout.write(content); writeConsultOutput(outputPath, content); @@ -865,6 +905,19 @@ async function runAgyConsultation( if (scanBuf.length < AGY_MARKER_SCAN_LIMIT) { scanBuf += buf.toString('utf-8'); if (AGY_OAUTH_MARKERS.some((m) => scanBuf.includes(m))) { + // The one place we have positive proof of being signed out, so it is + // written unconditionally rather than through the once-only publish: + // - If our own grace timer already guessed `auth` (agy took longer + // than AGY_AUTH_GRACE_MS to emit its banner), that guess would + // otherwise stand for the full auth TTL and every burst in that + // window would spawn tabs again. + // - A non-prober that failed open and spawned holds real evidence + // too; discarding it would leave a misfired guess uncorrected + // until the TTL lapsed, instead of on the very next spawn. + // recordAgyAuthState carries its own disabled-guard, so this is safe + // on every path. + recordAgyAuthState('unauth', bin); + publishAuth('unauth'); settleSkip('agy not authenticated — run `agy` once to sign in (OAuth)', 1); } } @@ -883,6 +936,9 @@ async function runAgyConsultation( cleanup(); const raw = Buffer.concat(outChunks).toString('utf-8').trim(); if (code !== 0 || raw.length === 0 || raw.includes(AGY_NONRESPONSE_MARKER)) { + // A broken run tells us nothing about auth — release without a verdict + // and let the next call re-probe. + publishAuth(); const reason = code !== 0 ? `agy exited with code ${code}` : raw.includes(AGY_NONRESPONSE_MARKER) @@ -896,6 +952,8 @@ async function runAgyConsultation( resolve(); return; } + // A real review is conclusive proof agy is signed in. + publishAuth('auth'); // Plain-text stdout IS the review. process.stdout.write(raw); writeConsultOutput(outputPath, raw); diff --git a/packages/codev/src/commands/doctor.ts b/packages/codev/src/commands/doctor.ts index c7c961153..a635f0873 100644 --- a/packages/codev/src/commands/doctor.ts +++ b/packages/codev/src/commands/doctor.ts @@ -29,6 +29,7 @@ import { STALE_BACKUP_AGE_DAYS, } from '../lib/migration-backup-audit.js'; import { resolveAgyBin, AGY_OAUTH_MARKERS } from './consult/index.js'; +import { checkCachedAgyAuth, recordAgyAuthState } from './consult/agy-auth-cache.js'; import { AGENT_FARM_DIR } from '@cluesmith/codev-core/constants'; import { measureSessionLogs, @@ -468,11 +469,24 @@ function checkAgy(): CheckResult { * unauthenticated agy reports "needs login" promptly (it would otherwise print * the URL and wait ~30s) — rather than stalling `codev doctor` for the full * auth wait. Always resolves (never throws). + * + * Shares the consult lane's auth cache (#1077): a fresh verdict is reported + * without probing, so `codev doctor` on an unauthenticated agy does not open yet + * another OAuth browser tab. When it does probe, it records the result for the + * consult lane's benefit. */ function verifyAgy(): Promise { const bin = resolveAgyBin(); if (!bin) return Promise.resolve({ status: 'skip', version: 'not installed', note: AGY_INSTALL_HINT }); + const cached = checkCachedAgyAuth(bin); + if (cached === 'unauth') { + return Promise.resolve({ status: 'fail', version: 'needs login', note: 'run `agy` once to sign in (OAuth)' }); + } + if (cached === 'auth') { + return Promise.resolve({ status: 'ok', version: 'operational (cached)' }); + } + return new Promise((resolve) => { const proc = spawn(bin, ['--print-timeout', '20s', '--print', 'Reply with just OK'], { stdio: ['ignore', 'pipe', 'pipe'], @@ -502,6 +516,8 @@ function verifyAgy(): Promise { scan += s; // Fast path: OAuth URL appears immediately on an unauthenticated run. if (AGY_OAUTH_MARKERS.some((m) => scan.includes(m))) { + // Share the verdict so a concurrent CMAP burst skips without spawning. + recordAgyAuthState('unauth', bin); finish({ status: 'fail', version: 'needs login', note: 'run `agy` once to sign in (OAuth)' }); } } @@ -511,8 +527,14 @@ function verifyAgy(): Promise { proc.on('error', () => finish({ status: 'fail', version: 'error', note: 'run `agy` to verify sign-in' })); proc.on('close', (code) => { const text = out.join('').trim(); - if (code === 0 && text.length > 0) finish({ status: 'ok', version: 'operational' }); - else finish({ status: 'fail', version: 'not responding', note: 'run `agy` to verify sign-in' }); + if (code === 0 && text.length > 0) { + recordAgyAuthState('auth', bin); + finish({ status: 'ok', version: 'operational' }); + } else { + // Neither authenticated nor proven signed-out — record nothing, so the + // consult lane re-probes rather than skipping a lane that may work. + finish({ status: 'fail', version: 'not responding', note: 'run `agy` to verify sign-in' }); + } }); }); }