[Bugfix #1077] Pre-flight agy auth cache to stop the OAuth tab burst

[waleedkadous]

Summary

The gemini consult lane spawned agy before it could know agy was signed out, and every spawn opens an OAuth browser tab. A CMAP round is N independent consult processes, so N tabs stranded — 12-15 in the reported case.

This adds a cross-process auth-state cache consulted before the spawn. Unauthenticated agy is now spawned at most once per TTL window instead of once per consult.

Measured with a fake agy that logs every invocation (spawn counts are observed, not mocked):

Scenario	Before	After
5 parallel consult -m gemini, unauthenticated	5 spawns / 5 tabs	1
5 parallel, authenticated	5	5 (unchanged — every lane is real work)
unauth verdict already cached	5	0

Root Cause

runAgyConsultation (packages/codev/src/commands/consult/index.ts) spawned agy unconditionally, and AGY_OAUTH_MARKERS detection lives in the child's stdout/stderr handler — i.e. strictly after the spawn. Upstream agy opens the OAuth tab before printing the URL those markers match, so the kill can never prevent the tab. The skip semantics were already correct; only the timing was wrong.

A module-level memo cannot help: each consult invocation is a separate OS process, so the state has to be shared through the filesystem.

Amplifiers confirmed in code: CMAP-N dispatches one process per model (porch/next.ts:529-531), builders run 1-3 consult phases each, and the partial-review re-emit (porch/next.ts:544) re-issues the same consult commands.

Fix

New module packages/codev/src/commands/consult/agy-auth-cache.ts:

• Verdict in ~/.cache/codev/agy-auth.json (0600 in a 0700 dir, honours XDG_CACHE_HOME), keyed to the resolved agy binary, written via temp-file + rename so a concurrent reader never sees half a document.
• TTLs: 5 min unauth (the sign-in recovery window), 30 min auth. Both env-overridable.
• A flock-style lock (openSync 'wx', with stale-lock reclaim) elects one prober per cold window. Losers poll the cache, not the lock — the prober holds it for its whole run, which may be minutes, but publishes the verdict within seconds.
• Best-effort credential-mtime hint invalidates a verdict early when agy rewrites its credentials. agy's credential path is undocumented and was not locatable on this machine, so the TTL is the real recovery mechanism and the hint is a bonus.

Two integration points: runAgyConsultation pre-flights before spawn, and doctor.verifyAgy both reports a fresh verdict without probing and records the verdicts its own probe produces.

Deliberate deviation from the issue's proposal

The issue proposes a dedicated --print-timeout 5s probe on a cold cache. Implemented instead: the real consult run doubles as the probe, publishing the verdict as soon as it is knowable (OAuth marker → unauth; real output, or 3s of marker-free silence → auth). This avoids a second agy spawn per TTL window in the authenticated common case, and keeps marker detection in one place rather than duplicating it in a path that could classify differently. The observable guarantee the ACs ask for is unchanged.

Failure bias

Only positive marker evidence records unauth, and a waiter that never sees a verdict proceeds anyway. 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.

Test Plan

packages/codev/src/commands/consult/__tests__/agy-auth-cache.test.ts — 25 tests. The headline case drives the real lane (_runAgyConsultation) against a fake agy that logs every invocation, so a 5-wide unauthenticated burst is asserted to produce exactly one spawn. A companion case asserts 3 spawns with the cache disabled, which is what proves the burst test measures the cache rather than something incidental.

Also covered: per-state TTL expiry, bin-path and credential-mtime invalidation, corrupt/unknown entries, lock election, stale-lock reclaim, sign-in recovery after the unauth TTL, and that an authenticated burst still runs every lane.

• Full suite: 3720 passed, 0 failed (npm test), build green. Both verified again by porch's own fix phase checks.
• Cross-process smoke (5 real consult processes, not in-process): 1 spawn unauthenticated, 5 authenticated with all 5 reviews delivered.
• Recovery verified end-to-end: unauth cached → sign in → wait out the TTL → next call re-probes, finds auth, delivers a review. No manual cache clear.

Note on test isolation

The cache is a user-global side effect. Unpinned, the suite wrote a verdict into the developer's real ~/.cache/codev, and one doctor case's verdict made a sibling case stop spawning the thing it asserted on (one genuine failure, caught and fixed). The consult/doctor suites now pin CODEV_AGY_AUTH_CACHE_DIR into their temp dirs, and the module is inert under VITEST unless a cache dir is explicitly set — the guard is what keeps future tests from silently re-acquiring the hazard. Verified: a full suite run no longer creates ~/.cache/codev.

Scope

Net diff is ~960 lines across 12 files, above BUGFIX's ~300 guideline. The composition: ~310 lines of new module (roughly half comments), 74 lines across the two integration points, ~370 lines of tests, ~58 of docs. Shape is what the guideline actually targets — one new module plus two call sites, no architectural change, no new abstractions imposed on callers. Flagging it rather than quietly ignoring it; happy to split the docs out if you'd prefer a smaller diff.

Notes

• packages/codev/src/commands/doctor.ts also carries the formatBytes alias from the hotfix in hotfix(doctor): resolve formatBytes duplicate-identifier collision from #1242 × #1243 #1248, which arrived via a main merge — not a change of mine.
• Docs: consult.md (mechanism, recovery, env vars), mirrored into codev-skeleton/; CLAUDE.md / AGENTS.md gemini-lane bullet, kept byte-identical.

Fixes #1077

[waleedkadous]

CMAP-3 review

Reviewer	Verdict	Confidence	Key issues
Gemini (agy)	APPROVE	HIGH	None
Codex	APPROVE	HIGH	None
Claude	APPROVE	HIGH	None

Unanimous, no requested changes, so nothing to rebut or fix.

What each reviewer independently called out:

• Gemini — atomic write semantics (temp + rename), 0600/0700 permissions, 'wx' lock with stale reclaim, and fail-open polling "so active review runs are never improperly blocked".
• Codex — confirmed the cross-process framing (in-memory memoization cannot stop parallel bursts) and the fail-open bias. Note: it could not execute Vitest — its sandbox is read-only and Vitest failed creating .vite-temp — so it reviewed by inspection and relied on the reported green build/tests. Those were independently verified here by porch's own fix-phase build + tests checks.
• Claude — verified the burst regression test plus the disabled-cache control case, and the sign-in recovery path.

[waleedkadous]

Architect Integration Review

Excellent work — this is a genuinely well-engineered fix. The single-prober lock election with cache-polling waiters, the fail-open bias, and the spawn-count-observed (not mocked) burst regression test are all exactly right. My two integration lanes agree with your CMAP-3: codex APPROVE (called the module boundary "the right system boundary for a cross-process concern") and claude APPROVE, high confidence (independently verified the two-tree mirroring and CLAUDE/AGENTS byte-identity).

On your two flagged judgment calls:

1. Probe deviation — agreed, your version is better than the issue's proposal. Reusing the real run as the probe avoids an extra spawn on the authenticated path and keeps marker detection in one place. No concern.
2. Scope — agreed, no split needed. ~370 of the lines are tests and ~58 docs; one module + two call sites is BUGFIX-shaped regardless of line count.

One change I'd like before merge, because it protects the headline guarantee:

Let positive OAuth-marker evidence write the cache unconditionally. Today the marker path reports unauth only through preflight.publish, which is once-only. If the grace timer has already fired at 3s (agy slower than 3s to emit its banner — a node CLI doing a network token check makes that plausible), the timer publishes a false auth that lives for 30 minutes, and the real unauth evidence a moment later becomes a no-op. During that window every burst spawns tabs again — the fix silently reverts to the status quo on exactly the machines that need it. Suggested shape: in the marker handler, call recordAgyAuthState('unauth', bin) directly (in addition to the existing publishAuth('unauth') for lock release). That also means non-prober runs that hit the banner publish the verdict, so a misfired grace timer gets corrected by the very next spawn instead of waiting out the TTL. recordAgyAuthState already carries the disabled-guard, so the call is safe everywhere.

Perhaps add one test alongside: grace timer fires auth, marker arrives late, assert the cache ends up unauth.

Everything else is merge-ready.

[waleedkadous]

Requested change pushed — 42cf18e

Good catch, and you were right that this protected the headline guarantee. Marker detection now calls recordAgyAuthState('unauth', bin) directly, so positive evidence wins regardless of arrival order.

Worth noting it was two bugs, not one — the second I had missed as well:

1. The grace-timer race you described. Banner later than the 3s grace → timer publishes auth → real unauth becomes a no-op → false verdict for the full 30-min TTL.
2. Non-prober evidence was discarded entirely. A caller that failed open (another process holding the lock) still spawns and can be the one to see the banner, but publish is inert for non-probers, so it wrote nothing at all. Even with the ordering fixed, a misfired guess would have sat uncorrected until the TTL lapsed rather than being fixed by the next spawn. Writing unconditionally from the marker handler resolves both at once, exactly as you suggested.

This was a real inversion of the fail-safe bias I claimed in the PR body: I wrote that "only positive marker evidence records unauth", but the implementation let a speculative verdict outrank positive evidence purely on timing.

Tests

Two added, and I verified both fail against the previous code before restoring the fix — not just pass with it:

Test	Without fix	With fix
late OAuth banner (3.4s, past the grace period) overwrites the speculative verdict	FAIL (cached auth)	cached unauth
the run that sees the banner is not the prober	FAIL (nothing cached)	cached unauth

Full suite 3722 passed, 0 failed (up 2). Typecheck clean. Cross-process burst smoke re-run after the change: 5 parallel unauthenticated consults → still 1 spawn.

No changes to the docs or the public surface. Gate is still yours.