From 71d2f0af1d9b203d17ef77e99468748e1497e201 Mon Sep 17 00:00:00 2001 From: diegohb Date: Fri, 7 Aug 2026 23:53:04 -0400 Subject: [PATCH 1/4] docs(phase-0): establish regression-net framework + agent skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 0 framework (per architecture review + grilling 2026-08-07): - CONTEXT.md (new): domain glossary for auth + chat modules - docs/phase-0/plan.md (new): executable Phase 0 plan (3 OpenSpec changes) - docs/phantom-bug-synthesis.md (renamed from docs/phantom-auth-synthesis-2026-08-06.md via git mv): now a write-once ledger of bug history; sessions 1-3 content preserved; empty Appendix ready for post-fix-failure entries; §Phase 0 framing stripped (was planning, not bug history) Agent skills config (per setup-matt-pocock-skills): - docs/agents/issue-tracker.md: GitHub Issues via gh CLI + tskNN- ticket-id prefix convention - docs/agents/domain.md: single-context layout + write-once ledger convention for the bug ledger - docs/agents/triage-labels.md: five-role vocabulary (needs-triage, needs-info, ready-for-agent, ready-for-human, wontfix) - AGENTS.md: ## Agent skills block appended Refs: - architecture-review-auth-2026-08-07.html (temp) — visual review - handoff-phase0-2026-08-07.md (temp) — for the next session Next: branch phase0/regression-net off main@v2.6.1. --- AGENTS.md | 16 ++ CONTEXT.md | 80 ++++++++ docs/agents/domain.md | 48 +++++ docs/agents/issue-tracker.md | 55 ++++++ docs/agents/triage-labels.md | 15 ++ docs/phantom-bug-synthesis.md | 342 ++++++++++++++++++++++++++++++++++ docs/phase-0/plan.md | 278 +++++++++++++++++++++++++++ 7 files changed, 834 insertions(+) create mode 100644 CONTEXT.md create mode 100644 docs/agents/domain.md create mode 100644 docs/agents/issue-tracker.md create mode 100644 docs/agents/triage-labels.md create mode 100644 docs/phantom-bug-synthesis.md create mode 100644 docs/phase-0/plan.md diff --git a/AGENTS.md b/AGENTS.md index 4d19331..17dc452 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,3 +135,19 @@ Before proposing new work, list `openspec/changes/` (excluding `archive/`) to se - **plannotator** — `submit_plan` is for action plans the user will execute, not for plans that themselves produce more plans. - **bash** — PowerShell 7+ (`pwsh`) on Windows. Use the `workdir` parameter instead of `cd`; don't change directories inside a command. - **Delegation** — default to orchestrating subagents; batch parallel investigations; chain sequential work when output feeds the next step. + +--- + +## Agent skills + +### Issue tracker + +GitHub Issues at `https://github.com/expert-vision-software/gemiterm/issues`. Use `gh` CLI. OpenSpec change dirs use the `tskNN-` prefix to mirror GitHub issue numbers one-to-one. See `docs/agents/issue-tracker.md`. + +### Triage labels + +Default five-role vocabulary (`needs-triage`, `needs-info`, `ready-for-agent`, `ready-for-human`, `wontfix`). See `docs/agents/triage-labels.md`. + +### Domain docs + +Single-context layout. Read `CONTEXT.md` at the repo root before exploring the codebase, and any ADRs in `docs/adr/` that touch the area. The phantom-auth bug history is in `docs/phantom-bug-synthesis.md` (write-once ledger). See `docs/agents/domain.md`. diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..102a815 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,80 @@ +# Domain Glossary — GemiTerm + +Stable terminology for the auth and conversation modules. No implementation details; see source and specs for those. + +--- + +## Auth concepts + +### Cookie jar +The persisted collection of authentication cookies for a single profile, stored at `%APPDATA%\gemiterm\profiles\\cookies.json` (Windows) or `~/gemiterm/profiles//cookies.json` (POSIX). Contains the long-lived identity cookie (`__Secure-1PSID`), the short-lived session cookie (`__Secure-1PSIDTS`), and companion auth cookies (`SID`, `HSID`, `SSID`, `APISID`, `SAPISID`, `__Secure-3PSID`, `SIDCC`, etc.). All API operations ultimately read from this jar. + +### Capture-integrity +The property that the cookie-capture path (login flow, silentRefresh) stores the COMPLETE jar the browser holds, not a filtered subset. Violated when the capture path trims to a "required cookies" predicate before persisting; the symptom is that `models()` succeeds but `listChats` returns empty, because `listChats` requires companion cookies that the trimmed jar lacks. + +### Phantom-auth session +A session state where `models()` succeeds (the PSID is server-accepted) but `listChats` returns empty (companion cookies are absent or stale). The freshness model says "valid"; the API reality says "broken". Distinguished from a **dead session** (RotateCookies returns 401/403) and a **fresh session** (every probe passes). + +### Session state +The named condition of a profile's server-accepted credentials. Enumerated values: `Fresh | Phantom | Dead | Stale | Declined`. Computed from the probe result (models), the rotation result (rotateCookies), and the listChats result. Currently an implicit state machine inside `ProfileAuthManager.ensureAuthenticated`; an explicit classifier is proposed (Candidate C). + +### Companion cookies +Auth cookies set alongside `__Secure-1PSID` and `__Secure-1PSIDTS` during the Google login envelope: `SID`, `HSID`, `SSID`, `APISID`, `SAPISID`, `__Secure-3PSID`, `__Secure-3PSIDTS`, `SIDCC`, `NID`. Required by `listChats`; NOT required by `models()` or `readChat(cid)` (in practice). Their absence is the proximate cause of phantom-auth. + +### PSID-only probe +A server-side validity check using `models()` which succeeds with only `__Secure-1PSID` present. Insufficient as the sole auth gate because it cannot detect phantom-auth. See **Probe cache** below. + +### Probe cache +A per-process, TTL-bounded memoization of the most recent `models()` probe result per profile (default 150 000 ms, overridable via `GEMITERM_PROBE_TTL_MS`). Distinct from the on-disk freshness check, which is purely local. Does NOT cache the `listChats` result; the phantom check re-issues on every L1-decline path. + +### Recovery ladder +The escalation sequence `ensureAuthenticated` follows when its probe says "stale" or its rotation says "declined": L1 `RotateCookies` POST → targeted L2 silent refresh (when phantom is detected) → throw `AuthenticationError` to surface to headed reauth. Each rung has different failure modes; the ladder is the policy that maps session state to action. + +--- + +## Auth-flow control + +### Cookie capture path +The sequence by which cookies enter the persisted jar: headed browser → `playwright-cli` probe → `CookieMonitor` callback → `AuthService.extractCookies` → `CookieStorageService.saveCookiesForProfile`. Trimming anywhere in this path is a capture-integrity bug. + +### Cookie rotation +A POST to `https://accounts.google.com/RotateCookies` with the current `.google.com` cookie header, asking Google for a fresh `__Secure-1PSIDTS`. Returns 200 with refreshed Set-Cookies; 401/403 if the session is server-dead. Throttled per-process to 600 s. + +### Silent refresh (L2) +A headless-browser session that captures a fresh PSIDTS via the cookie-capture path without user interaction. Two modes: `full` (replaces jar via merge) and `targeted` (updates only PSIDTS-family cookies). The targeted mode exists because the full mode was found to corrupt the login's aligned cookie envelope. + +--- + +## Conversation concepts + +### Conversation threading +The property that a `sendMessage(cid)` call extends an existing conversation rather than starting a new one. Requires the SDK's positional metadata array `[cid, rid, rcid, null, null, null, null, null, null, ctx]` to carry `rid` and `rcid` from the conversation's last model turn. See [AGENTS.md](../AGENTS.md) for the session-metadata history. + +### Chat metadata +A small per-conversation record `{ rid, rcid, ctx }` stored at `%APPDATA%\gemiterm\profiles\\chat-metadata.json`. The `rid`/`rcid` slots are required for threading; `ctx` is a context-token slot used by some Gemini operations. Currently the metadata array layout leaks across 5 call sites — see Candidate B. + +### Profile +A named collection of (cookies, chat metadata, conversation history) under a single Google login. Multiple profiles may coexist (`gemiterm auth -e `); `getDefaultProfileName()` returns the active one. + +--- + +## Test-layer concepts + +### Regression net +A characterization-test layer that pins behaviour at the integration boundary (the seam where callers meet services), so internal restructuring cannot silently reintroduce known regressions. Distinct from per-method unit tests (which pin implementation) and from mediator-mocked CLI integration tests (which pin argv dispatch). Phase 0 of the auth-path architecture review is the regression net for the auth + chat modules. + +### Cookie-aware fake +A test double at the `GeminiClientService` seam whose responses depend on the on-disk cookie jar's contents — specifically, `listChats` returns chats iff the jar carries the companions `listChats` requires. Replaces a ~1 h server-degradation wait with an instant local repro. Pattern established in `tests/services/cookie-jar-repro.test.ts` (commit `efab987`). + +### Capture-trim bug +The historical defect (closed by commit `6bc51f6`) where `CookieMonitor` filtered the browser jar to `REQUIRED_COOKIES` before invoking the persistence callback, causing downstream `saveCookiesForProfile` to overwrite a full 39-cookie jar with a 4-cookie subset. The regression-net test for capture-integrity must assert the post-capture jar contains companions. + +--- + +## See also + +- `docs/phantom-bug-synthesis.md` — full investigation history; the authoritative source for "what the bug actually was" +- `openspec/specs/auth/spec.md` — committed requirements; the canonical specification +- `openspec/specs/phantom-auth-detection/spec.md` — capability spec for the probe contract +- `openspec/specs/silent-refresh-tightening/spec.md` — capability spec for silentRefresh +- `openspec/changes/archive/2026-08-07-cookie-jar-integrity/` — archived change; the capture-fix provenance diff --git a/docs/agents/domain.md b/docs/agents/domain.md new file mode 100644 index 0000000..3ab47ec --- /dev/null +++ b/docs/agents/domain.md @@ -0,0 +1,48 @@ +# Domain Docs + +How the engineering skills should consume this repo's domain documentation when exploring the codebase. + +## Before exploring, read these + +- **`CONTEXT.md`** at the repo root, or +- **`CONTEXT-MAP.md`** at the repo root if it exists — it points at one `CONTEXT.md` per context. Read each one relevant to the topic. +- **`docs/adr/`** — read ADRs that touch the area you're about to work in. In multi-context repos, also check `src//docs/adr/` for context-scoped decisions. + +If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The `/domain-modeling` skill (reached via `/grill-with-docs` and `/improve-codebase-architecture`) creates them lazily when terms or decisions actually get resolved. + +## File structure + +Single-context repo (this repo): + +``` +/ +├── CONTEXT.md +├── docs/ +│ ├── adr/ +│ ├── agents/ +│ │ ├── issue-tracker.md +│ │ ├── domain.md +│ │ └── triage-labels.md +│ └── phantom-bug-synthesis.md ← write-once ledger of phantom-auth attempts +└── src/ +``` + +## Use the glossary's vocabulary + +When your output names a domain concept (in an issue title, a refactor proposal, a hypothesis, a test name), use the term as defined in `CONTEXT.md`. Don't drift to synonyms the glossary explicitly avoids. + +If the concept you need isn't in the glossary yet, that's a signal — either you're inventing language the project doesn't use (reconsider) or there's a real gap (note it for `/domain-modeling`). + +## Flag ADR conflicts + +If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: + +> _Contradicts ADR-0007 (event-sourced orders) — but worth reopening because…_ + +## Bug-history convention + +`docs/phantom-bug-synthesis.md` is a **write-once ledger** of every attempt to deal with the phantom-auth bug. New entries are appended when: +- A bug, symptom, or finding is reported AFTER a supposed fix was implemented and failed +- A new attempt (fix or refactor) is made + +Past entries are not edited; only appended to. The doc preserves the full history of attempts, including which fixes worked and which regressed. diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md new file mode 100644 index 0000000..c2fb251 --- /dev/null +++ b/docs/agents/issue-tracker.md @@ -0,0 +1,55 @@ +# Issue tracker: GitHub + +Issues and specs for this repo live as GitHub issues. Use the `gh` CLI for all operations. + +## Conventions + +- **Create an issue**: `gh issue create --title "..." --body "..."`. Use a heredoc for multi-line bodies. +- **Read an issue**: `gh issue view --comments`, filtering comments by `jq` and also fetching labels. +- **List issues**: `gh issue list --state open --json number,title,body,labels,comments --jq '[.[] | {number, title, body, labels: [.labels[].name], comments: [.comments[].body]}]'` with appropriate `--label` and `--state` filters. +- **Comment on an issue**: `gh issue comment --body "..."` +- **Apply / remove labels**: `gh issue edit --add-label "..."` / `--remove-label "..."` +- **Close**: `gh issue close --comment "..."` + +Infer the repo from `git remote -v` — `gh` does this automatically when run inside a clone. + +## Pull requests as a triage surface + +**PRs as a request surface: no.** _(Set to `yes` if this repo treats external PRs as feature requests; `/triage` reads this flag.)_ + +When set to `yes`, PRs run through the same labels and states as issues, using the `gh pr` equivalents: + +- **Read a PR**: `gh pr view --comments` and `gh pr diff ` for the diff. +- **List external PRs for triage**: `gh pr list --state open --json number,title,body,labels,author,authorAssociation,comments` then keep only `authorAssociation` of `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, or `NONE` (drop `OWNER`/`MEMBER`/`COLLABORATOR`). +- **Comment / label / close**: `gh pr comment`, `gh pr edit --add-label`/`--remove-label`, `gh pr close`. + +GitHub shares one number space across issues and PRs, so a bare `#42` may be either — resolve with `gh pr view 42` and fall back to `gh issue view 42`. + +## When a skill says "publish to the issue tracker" + +Create a GitHub issue. + +## When a skill says "fetch the relevant ticket" + +Run `gh issue view --comments`. + +## Ticket-id prefix convention + +OpenSpec changes use `tskNN-` to mirror GitHub issue numbers one-to-one. Examples: + +- `tsk12-phantom-bug-refactor-clock` +- `tsk01-phase0-regression-net-char` +- `tsk02-phase0-factory-coverage` + +The `tskNN` prefix is the GitHub issue number. If the tracker number differs (e.g. issue #42 is the canonical id), use `tsk42-` instead. The OpenSpec change dir name is `openspec/changes/tsk-/`. + +## Wayfinding operations + +Used by `/wayfinder`. The **map** is a single issue with **child** issues as tickets. + +- **Map**: a single issue labelled `wayfinder:map`, holding the Notes / Decisions-so-far / Fog body. `gh issue create --label wayfinder:map`. +- **Child ticket**: an issue linked to the map as a GitHub sub-issue (`gh api` on the sub-issues endpoint). Where sub-issues aren't enabled, add the child to a task list in the map body and put `Part of #` at the top of the child body. Labels: `wayfinder:` (`research`/`prototype`/`grilling`/`task`). Once claimed, the ticket is assigned to the driving dev. +- **Blocking**: GitHub's **native issue dependencies** — the canonical, UI-visible representation. Add an edge with `gh api --method POST repos///issues//dependencies/blocked_by -F issue_id=`, where `` is the blocker's numeric **database id** (`gh api repos///issues/ --jq .id`, _not_ the `#number` or `node_id`). GitHub reports `issue_dependencies_summary.blocked_by` (open blockers only — the live gate). Where dependencies aren't available, fall back to a `Blocked by: #, #` line at the top of the child body. A ticket is unblocked when every blocker is closed. +- **Frontier query**: list the map's open children (`gh issue list --state open`, scoped to the map's sub-issues / task list), drop any with an open blocker (`issue_dependencies_summary.blocked_by > 0`, or an open issue in the `Blocked by` line) or an assignee; first in map order wins. +- **Claim**: `gh issue edit --add-assignee @me` — the session's first write. +- **Resolve**: `gh issue comment --body ""`, then `gh issue close `, then append a context pointer (gist + link) to the map's Decisions-so-far. diff --git a/docs/agents/triage-labels.md b/docs/agents/triage-labels.md new file mode 100644 index 0000000..b716855 --- /dev/null +++ b/docs/agents/triage-labels.md @@ -0,0 +1,15 @@ +# Triage Labels + +The skills speak in terms of five canonical triage roles. This file maps those roles to the actual label strings used in this repo's issue tracker. + +| Label in mattpocock/skills | Label in our tracker | Meaning | +| -------------------------- | -------------------- | ---------------------------------------- | +| `needs-triage` | `needs-triage` | Maintainer needs to evaluate this issue | +| `needs-info` | `needs-info` | Waiting on reporter for more information | +| `ready-for-agent` | `ready-for-agent` | Fully specified, ready for an AFK agent | +| `ready-for-human` | `ready-for-human` | Requires human implementation | +| `wontfix` | `wontfix` | Will not be actioned | + +When a skill mentions a role (e.g. "apply the AFK-ready triage label"), use the corresponding label string from this table. + +Edit the right-hand column to match whatever vocabulary you actually use. diff --git a/docs/phantom-bug-synthesis.md b/docs/phantom-bug-synthesis.md new file mode 100644 index 0000000..f129cac --- /dev/null +++ b/docs/phantom-bug-synthesis.md @@ -0,0 +1,342 @@ +# Phantom Authentication — Write-Once Bug Ledger + +**Convention:** this is a **write-once ledger** of every attempt to deal with the phantom-auth bug. New entries are appended (never rewritten) when a bug, symptom, or finding is reported AFTER a supposed fix was implemented and failed, or when a new attempt (fix or refactor) is made. Past entries are not edited. See `docs/agents/domain.md` for the convention. + +**Original title:** Phantom Authentication — Synthesis & Options (2026-08-06). Scope: retrospective of the 4-day, 3-release sprint (v2.6.0 → v2.6.2), the 4-cookie-jar discovery later the same day, and an evaluation of the background-service idea. + +> **STATUS (updated later, 2026-08-06):** The original headline of this doc — *"the phantom-auth bug is already fixed at the architectural level in v2.6.2"* — is **wrong**, and was disproven the same afternoon by the **4-cookie-jar discovery**. v2.6.2's fixes (probe + L1 rotation + L2 escalation) were **necessary but insufficient**: they operate on an *already-trimmed* cookie jar. The real root cause of the persistent `list returned 0 chats` symptom is a cookie-**capture** bug in `CookieMonitor`, which was identified, harnessed, specified, and fixed on 2026-08-06 (commits `efab987` → `6bc51f6`). The historical analysis below is retained for context, but read **§The 4-cookie discovery** first; it supersedes the conclusions in §TL;DR (original) and §Recommendation (original). + +--- + +## The 4-cookie discovery (2026-08-06, afternoon) — the definitive root cause + +### What was found + +After v2.6.2 was declared "done" (but before tagging), the original symptom re-occurred (`list -i` → 0 chats after stepping away ~1 h). Empirical testing against live `%APPDATA%\gemiterm` profiles: + +| Probe | Result | Meaning | +|---|---|---| +| `list` (post v2.6.2 L2 fix) | L2 silentRefresh "recovered" PSIDTS → **still 0 chats** | L2 fix necessary, not sufficient | +| `status -v` (3 profiles) | all show **4 cookies**, "next expiry 364d" | the jar is far-future-fresh yet incomplete | +| Jar inspection | exactly 4 cookies: `__Secure-1PSID`+`__Secure-1PSIDTS` on `.google.com` and `.youtube.com` | **missing ~10–12 companion cookies** (`SID`/`HSID`/`SSID`/`APISID`/`SAPISID`/`SIDCC`/`NID`/`__Secure-3PSID`/…) | + +### The trimmer — `CookieMonitor` + +The full browser jar is trimmed at the **source**, before any persistence path runs. In `src/services/cookie-monitor.ts`: + +- `checkCookies` (`:117`) and `poll` (`:159`) both do `cookies.filter((c) => REQUIRED_COOKIES.has(c.name))` where `REQUIRED_COOKIES = {"__Secure-1PSID","__Secure-1PSIDTS"}`, then pass **only that filtered subset** to `onCookiesFound` (poll) / as the return (checkCookies). +- **Every capture path flows through that callback:** headed `authenticate`/`renew` via `waitForLogin`, and `silentRefresh` L2 via `waitForSilentLogin`. So `extractCookies`→`cookieStorage.save` (headed) and `mergeCookies(...)`+`saveCookiesForProfile` (silentRefresh) only ever see PSID/PSIDTS. + +### Why this explains everything + +- `models()` is PSID-only → succeeds with 4 cookies → probe says "valid" **indefinitely**. +- `readChat()` works by conversation id (PSID-only) → `continue ` succeeds even in the degraded state. +- `listChats` enumeration requires the **full auth set** (companions included) → returns **empty** from a 4-cookie jar. +- `silentRefresh`'s `mergeCookies` (`auth-service.ts:20-30`) is a **correct** upsert-by-`(name, domain, path)` — it preserves existing entries and appends new ones. But it can only merge what the monitor hands it (the trimmed set), so it **faithfully preserves the already-degraded jar**. The L2 escalation (`0b91cde`) "recovers" PSIDTS by its own criterion; `listChats` stays empty. + +In one sentence: **the prior fixes addressed detection and rotation; the capture path was silently truncating the jar the whole time.** + +### The fix (committed `6bc51f6`) + +Separate the **gating predicate** from the **payload**. Keep `REQUIRED_COOKIES` as the login gate (the callback fires only once both required cookies are present); pass the **full** `cookies` array as the payload. Surgical 2-line change in `cookie-monitor.ts` (`:121` `return authCookies`→`return cookies`; `:179` `onCookiesFound(authCookies)`→`onCookiesFound(cookies)`). Downstream `mergeCookies`/`save` code is already correct and **unchanged**. + +### Today's commit chain (all on `fix/v2.6.1-bugs`) + +| commit | what | +|---|---| +| `efab987` | repro harness — `tests/services/cookie-jar-repro.test.ts`: a cookie-aware fake at the `GeminiClientService`/SDK seam (returns chats iff the on-disk jar carries the companions `listChats` needs). Replaces the ~1 h server-degradation wait with an instant local repro. | +| `7b2d55f` | RED tests — 2 failing tests in `tests/services/cookie-monitor.test.ts` pinning the full-jar contract (intentional red, committed ahead of the fix). | +| `1ce47ec` | OpenSpec change `cookie-jar-integrity` — proposal/design/specs(`auth` delta)/tasks. Validates clean. | +| `6bc51f6` | **the fix** — `CookieMonitor` passes the full jar; CHANGELOG patched; tasks status. Greens the 2 RED tests. | + +(Concurrent, not part of this thread: `f747fc6` `spec(auth-daemon)` — the user's own same-day proposal for a background heartbeat. See §Background service below; the cookie-jar fix reframes its premise.) + +**Verified:** `bun run typecheck` clean · `bun test` **928 pass / 0 fail / 2 skip / 1945 expects / 57 files**. The existing characterization tests passed unchanged (they stub `cookieListFromState` with exactly the cookies they assert against). + +--- + +## TL;DR (original, earlier 2026-08-06 — superseded by §The 4-cookie discovery) + +> Retained for history. The "already fixed" claim below was the working belief before the afternoon replay exposed the capture-trim bug. + +The phantom-auth bug was believed fixed at the architectural level in v2.6.2 (CHANGELOG.md). The CLI: + +1. Probes the server with `models()` on every `ensureAuthenticated` (catches hard session death). +2. Unconditionally calls `rotateCookies` (L1 `accounts.google.com/RotateCookies` POST) on every valid-cookie `ensureAuthenticated` (proactively rotates `__Secure-1PSIDTS`, 600 s disk-mtime guard throttles). +3. Falls back to a headless browser refresh (L2) only when the probe says "stale" — and, after `0b91cde`, escalates to L2 when L1 reaches Google (HTTP 200) but the server declines to issue fresh PSIDTS. +4. Persists refreshed cookies via a `(name, baselineValue)` merge and an upsert-by-`(name, domain, path)` rule. + +**What this got right:** it closed the *detection* gap (H1 — no layer asked Google) and the *rotation* mechanics. **What it missed:** the jar being rotated/probed/persisted was already truncated upstream, so no amount of rotation could restore cookies the monitor had discarded. That gap is what §The 4-cookie discovery closed. + +--- + +## The bug — what it actually was + +Authoritative evidence: `openspec/changes/archive/2026-08-03-phantom-auth-ultimate-fix/investigation.md` (637 lines, 5-hypothesis grilling). + +The symptom is `gemiterm list -i` returns `0 chats` while the log says `Profile '' is authenticated`, recurring after auth. The 5 subagent hypotheses ranked: + +| # | Hypothesis | Verdict | +|---|---|---| +| H1 | `expires` is a cap, not a contract. Server-side session can be invalidated before the cookie's local `expires`. **No layer in the auth gate ever asks Google.** | SUPPORTED — root cause of the **detection** gap (fixed v2.6.0–v2.6.2). | +| H2 | `client.cookies` jar diverges from disk. | REFUTED. | +| H3 | `persistRefreshedCookies` merges by `name` only, not `(name, domain)` — cross-domain duplicates silently overwritten. | BUG CONFIRMED (latent; fixed). | +| H4 | `silentRefresh` is a no-op when loaded cookies are still valid AND is gated behind a local freshness short-circuit. | BUG CONFIRMED (both halves; fixed). | +| H5 | Cookie `expires` ordering is anomalous. | Reframed — the −35-day `PSIDTS−PSID` delta is a fingerprint of SDK renewal behavior, not a client bug. | +| **H6 (added 2026-08-06 PM)** | **The cookie *capture* path truncates the browser jar to `REQUIRED_COOKIES` before persisting, so `listChats` (which needs companion cookies) runs against a 4-cookie jar.** | **ROOT CAUSE of the persistent 0-chats symptom. Fixed `6bc51f6`.** | + +H1–H4 share one shape: **the client never consulted the server, or did so on an unreachable path.** H6 is a different class: **the client consulted the server with an incomplete credential set.** The freshness model was entirely local (H1–H4); H6 is a capture-integrity defect that no amount of server-consultation can expose, because `models()` is PSID-only and stays green. + +### The PSID vs PSIDTS asymmetry (corrected) + +The two tokens serve different roles, but the original table understated `listChats`'s requirements: + +| Token | Role | Lifetime | Server-side rotation | +|---|---|---|---| +| `__Secure-1PSID` | Long-lived identity | ~400 days | Not silently rotated | +| `__Secure-1PSIDTS` | Short-lived session | hours (locally far-future-looking) | **Yes — rotated silently** | +| `SID`/`HSID`/`SSID`/`APISID`/`SAPISID`/… | Companion auth cookies | session-scoped | Set with the login envelope | + +| RPC | Requires PSID | Requires PSIDTS | Requires companions | +|---|---|---|---| +| `models()` | yes | no | no | +| `readChat()` | yes | (works PSID-only in practice) | no | +| `listChats()` | yes | yes | **yes — returns empty without them** | + +This is why the `models()` probe reported "valid" indefinitely while `listChats` was broken: the probe and the symptom live on different credential requirements, and the capture path was starving `listChats` of exactly the cookies it needs. + +--- + +## The 3-release arc — what was shipped (detection + rotation; not capture) + +| Release | Date | Change | Effect | +|---|---|---|---| +| v2.6.0 | 2026-08-03 | `phantom-auth-ultimate-fix` + `phatom-auth-repro-with-tests` | L1 `RotateCookies` POST, server-side probe, L2 headless hardening, `persistRefreshedCookies` `(name, baselineValue)` merge. | +| v2.6.1 | 2026-08-04 | `phantom-auth-probe-rewrite` | Replaced ambiguous `listChats` probe with definitive `models()`. Retired `profile-has-chats` marker. | +| v2.6.2 | 2026-08-05 | `silent-refresh-stale-psidts-detection` + `phantom-auth-data-integrity` + `profile-resolution-client-init` + `profile-has-conversation-lookup` | L1 rotation always on the valid path; `mergeCookies` upsert; `resolveCookie` `.google.com`-preference; async `forProfile`; unbounded `profileHasConversation`. Plus (this branch, `0b91cde`) L2 escalation on server-decline. | + +**In hindsight:** these all targeted the *detection/rotation* layer and were correct as far as they went — but they operated on a jar that `CookieMonitor` had already truncated, so they could not resolve the 0-chats symptom. The capture-integrity fix (`6bc51f6`, to ship under v2.6.2) is what actually closes it. + +### What the L1 rotation actually does (unchanged, accurate) + +`src/services/cookie-rotation.ts` POSTs `[0,"-0000000000000000000"]` to `https://accounts.google.com/RotateCookies` with the current `.google.com` cookie jar. Google's response carries fresh `Set-Cookie` headers for `__Secure-1PSIDTS`, `__Secure-3PSIDTS`, and `SIDCC`. The new values are merged into storage if different. Three guards protect against abuse: a 600 s in-memory throttle keyed off the last POST time (previously a disk-mtime guard, replaced by `a780788`), a concurrent-call dedup (`inFlightRotations`), and the `GEMITERM_SKIP_ROTATE_COOKIES` opt-out. + +--- + +## Current state (updated for session 3) + +**The capture-integrity bug is fixed and verified deterministically (`6bc51f6`).** The recovery-ladder gaps (A + B) are fixed (`a780788`, `4dfe13c`), the L2-escalation-removal (`9762845`) closes the 401-on-fresh-login regression, the continue-conversation regression is fixed (`809240a`), and the status PROBE column (`b1d0df0`) makes the phantom-auth state visible at a glance. + +Branch `fix/v2.6.1-bugs` is now at 8 commits: `6bc51f6` → `f747fc6` → `fced072` → `df7ab32` → `a780788` → `4dfe13c` → **`9762845`** → **`809240a`** → **`b1d0df0`**. + +**932 pass / 0 fail / 1 skip / 1952 expects**, typecheck clean. Baseline BL-010 (was BL-009 at 933/1959 in session 2; the −1 test is the removed cooldown contract from `profile-auth-manager.test.ts`, the un-skipped test in `gemini-client-wrapper.test.ts` cancels that out — net test count: −1 test, −7 expects; the `seedMetadataFromChat` change net +1 expect). + +Open changes (excluding `commander-cli-parser`, unrelated): + +| Change | Status | Notes | +|---|---|---| +| `cookie-jar-integrity` | **Implemented, not archived.** Tasks 1–4 done; task 5.1 (spec sync/archive) pending. | The 0-chats headline fix. Delta targets `openspec/specs/auth/spec.md`. | +| `silent-refresh-stale-psidts-detection` | Tasks 1–4 done, task 5.1 spec sync pending | Code shipped in v2.6.2; delta pending merge into `openspec/specs/phantom-auth-detection/spec.md`. | +| `auth-daemon` (concurrent, `f747fc6`) | Proposal only | Background heartbeat. Its premise (see below) is reframed by the cookie-jar fix; also carries an `auth` spec delta — **archive order vs `cookie-jar-integrity` matters to avoid a delta conflict.** | +| `phantom-auth-review-refactors` | No tasks done | Extract `cookie-constants.ts`, lift `gimme` helper, fix `io.ts` single-call-site violations. Pure refactor. | +| `profile-aware-factory-wiring` | Open | `gemiterm list -p ` authenticates the default profile instead of the named one. (Discovered during session 3 PROBE work — `ListChatsQueryHandler` is wired with `getGeminiClient()` (no profile arg) at `cli/index.ts:119`, so the auth path always uses the default profile even when a specific profile is requested.) | +| `interactive-non-interactive-divergence` | Open, large | Interactive paths must route through the mediator. | + +Remaining open work: + +1. **Design + implement phantom-auth recovery (the current focus).** With L2 escalation removed (`9762845`), sessions in phantom-auth state (models works, listChats empty) cannot self-recover — the user must re-login every time. The leading design idea is a *targeted L2* that refreshes only PSIDTS-related cookies (from `COOKIE_NAMES_OF_INTEREST`) instead of `mergeCookies` (which replaces the full set). See §Session 3 — phantom-auth is now visible for the design sketch. +2. **Investigate whether targeted L2 + phantom detection should replace L1 decline's `logger.debug` branch.** Today's `else if (rotation.attempted)` branch is a no-op. The replacement path needs to: detect phantom (models ✓ AND listChats empty) → trigger targeted L2 → recover or throw `AuthenticationError`. +3. **Archive `cookie-jar-integrity`** (task 5.1) → syncs the two MODIFIED `auth` CookieMonitor requirements into the main spec. ⚠️ Coordinate with `auth-daemon` (also an `auth` delta). +4. **`/test-baseline eval`** — promote `docs/testing-baseline.xml` from BL-009 (933/1959) → BL-010 (932/1952). +5. **Ship gate HOLD** — v2.6.2 must not tag until #1 lands and live `list` returns chats on phantom-auth sessions. Code changes are 5 surgical commits on `fix/v2.6.1-bugs`. +6. Already-degraded on-disk jars are **not** retroactively backfilled by the cookie-jar fix; users run `gemiterm login` / `auth -e ` once to repopulate. `status` PROBE column now flags in-place whether a jar is functional (`✓ live`), phantom (`⚠ phantom`), or dead (`✗ dead`). + +--- + +## The recovery-ladder recurrence (2026-08-06, evening) — two recovery gaps + +~30 min after the capture fix was live-verified (`list` returned chats right after a fresh `login`), the **0-chats symptom returned** — but with a **full 39–41 cookie jar**, not the 4-cookie trimmed jar. The capture fix held. The diagnostic at 06:27Z (forcing L1 past the mtime guard) confirmed **Hypothesis B**: the session was genuinely server-side signed out. `rotateCookies` → **401 Unauthorized**; `models()` (PSID-only) still succeeded → probe said "authenticated" → `listChats` empty. No branch threw `AuthenticationError`, so `getGeminiClient`'s headed-reauth path (`cli/index.ts:81-99` → `promptAndReauth`) never fired. This is H1's caveat ("`models()` alone is insufficient") realized exactly. + +### The two recovery gaps + +**Gap A — throttle defeated by unrelated writes.** `shouldSkipForDiskMtime` read `getFileMtime(getProfilePath(...))` — the jar file's mtime. `GeminiClientService.persistRefreshedCookies` wrote the file on every API call (SDK self-rotation → divergence → save → mtime refreshed), so the 600 s guard was refreshed by unrelated saves and **almost never allowed an explicit rotation**. At 06:04Z the file was 3 min old → throttled → the 401 was never observed → the symptom was silently masked. + +**Gap B — no `AuthenticationError` surface path.** `performRotateCookies` classified ALL non-200 (including 401) as `{rotated:false, attempted:false}` — same bucket as "throttled/skipped." `ensureAuthenticated`'s `else` branch debug-logged and returned "authenticated." `escalateAfterServerDecline` (L2 fallback) warned but never threw. Nothing reached the existing headed-reauth prompt. + +### The fix (two commits, `fix/v2.6.1-bugs`) + +Behind RED tests at the `ProfileAuthManager` and `cookie-rotation` seams (TDD, `gimme(modelsImpl)` pattern from `tests/services/phantom-auth.test.ts`): + +**`a780788` — Gap A**: Replace `shouldSkipForDiskMtime` with an in-memory per-process `lastRotatePostAt` Map keyed off the actual RotateCookies POST time. The throttle still guards long-running processes (daemon/REPL); one-shot CLI commands always rotate (exposes the 401). No persisted state, no path-mediation change. + +**`4dfe13c` — Gap B**: +- `RotateCookiesResult` gains `sessionInvalid?: boolean`. `performRotateCookies` sets it on **401/403** responses (400/429/5xx/network → transient, unchanged). +- `ensureAuthenticated` throws `AuthenticationError` on `sessionInvalid` → `getGeminiClient` catch → `promptAndReauth`. +- `escalateAfterServerDecline` (200-but-declined phantom case) throws on L2 `silentRefresh` failure **and** on cooldown-skip (previously only warned). L2 still attempted first. +- Two pre-existing tests updated: cooldown contract (`profile-auth-manager.test.ts:720` → `.rejects.toThrow`) and throttle-isolation (`auth-service.test.ts` → `beforeEach` reset). + +**933 pass / 0 fail / 2 skip / 1959 expects**, typecheck clean (baseline was 928/1945). + +### Open: 401 on fresh login + +Live verification exposed a **new problem**: even after a headed `gemiterm login -p evs-diegohb` (41 cookies captured, PSID present), `rotateCookies` returns **401** immediately on the next command — within minutes of login. The B-fix correctly throws (surfaces the dead session), but *why a fresh login produces API-dead cookies* is not understood. + +Evidence: +- `list -i` at 07:38Z: L1 got 200 "no fresh PSIDTS" → L2 recovered via browser → authenticated. But `listChats` may have still returned empty. +- `list -p evs-diegohb` at 07:40Z: **401** (session dead again — possibly L2 browser capture overwrote the jar with API-invalid cookies). +- `models -p evs-diegohb` at 07:41Z: also **401**. + +Possible causes (uninvestigated): +1. The login captures cookies for the Gemini web app — those differ from what the API endpoints require, so the session is dead-at-birth for programmatic use. +2. L2 `mergeCookies` overwrites the login's cookies with browser-only cookies that lack the API-critical companions. +3. Cookie quality issue (SIDCC mismatch, PSIDTS stale-envelope) the API rejects. +4. RotateCookies endpoint behavior changed (rate-limiting, bot-detection for non-browser User-Agent). + +### Implication for the B-fix's "401 → immediate throw" decision + +The grill decision (Q2/Q3) chose "401 → throw immediately, skip L2" on the assumption L2 can't save a dead session. But the `list -i` evidence shows L2 CAN recover from degraded states. It's possible that 401 should also attempt L2 silentRefresh before throwing — the browser path may produce working API cookies that RotateCookies alone can't. Worth revisiting if the fresh-login 401 investigation doesn't find a simpler cause. + +--- + +## Session 3 (2026-08-06, late evening) — L2-removal, continue-chat fix, PROBE column + +Three more commits on `fix/v2.6.1-bugs`. This section is a strict addendum — the previous narrative stands; these updates close two residual gaps and add one diagnostic. + +### 3a. `9762845` — L2 escalation on L1 decline is **harmful**; remove it + +**Discovered by:** live verification of the recovery-ladder fix (session 2). Symptom: after a fresh headed `gemiterm login` (40 cookies captured, PSID present, expiry 1 year out), `bun run dev list` returns 23 chats on the first call but throws `AuthenticationError` (rotateCookies 401) on the second call ~30 seconds later. + +**Root cause (Hypothesis #2 from session 2 confirmed):** the L2 `silentRefresh` path's `mergeCookies(existing, cookies)` (auth-service.ts:300-302) replaces ALL cookies in the stored jar with browser session cookies. The browser creates a new session (different PSID, different companion cookies), and the merged set is rejected by `accounts.google.com/RotateCookies` with 401 on the next command. + +Evidence that the *cookies* are valid (not the session): +- `GEMITERM_SKIP_ROTATE_COOKIES=1 bun run dev list -p evs-diegohb` → listChats call returns "No conversations found" (no error). The cookies work for listChats. +- `bun run dev models -p evs-diegohb` → 7 models returned. The cookies work for models. +- `gemini.google.com/app` GET with the cookies returns 200, with a redirect header to gemini.google.com. The cookies work for the frontend. + +So the session is in **phantom-auth state**, not dead. L2 was attempting to "recover" by launching a headless browser that re-signed-in and captured a different session's cookies, then merged them in — actively breaking what was working. + +**The fix (commit `9762845`):** remove `escalateAfterServerDecline()` from `ProfileAuthManager`. When L1 RotateCookies returns 200-but-declined (no fresh PSIDTS in Set-Cookie), the session is valid — just log and continue. Three test files updated; the cooldown test and the L2-success-after-decline test were removed (no longer relevant). **931 pass / 0 fail / 2 skip.** + +Verified live: two consecutive `bun run dev list` calls after fresh login both return 23 conversations. No 401. The recovery-ladder recurrence (`a780788` + `4dfe13c`) still works correctly for genuinely-dead sessions — that contract is preserved. + +### 3b. `809240a` — `continue conversation` regression (pre-existing, exposed by `list -i` flow) + +**Discovered by:** user ran `bun run dev list -i`, picked a conversation, chose "Continue conversation". The model responded "I'd love to, but we're just starting our conversation!" — a new chat started instead of threading onto the existing one. + +**Root cause:** `sendMessage` in `src/services/gemini-client-wrapper.ts:311-344` had a cid-only fallback when `chatMetadata.lookup` returned null. The fallback built a session with only `session.cid` set (which populates `_meta[0]`) but no `rid` (`_meta[1]`) or `rcid` (`_meta[2]`). The Gemini server requires all three slots to thread onto an existing conversation turn — without `rid`/`rcid`, it treats the request as a new chat and starts a fresh conversation. + +This was an explicit non-goal of the archived OpenSpec change `2026-07-27-fix-continue-chat-session-metadata`: "Backfilling metadata for chats that existed before this change." The `list -i` "Continue conversation" path doesn't call `fetchChat` to seed metadata (only the "View full conversation" action does), so the user hits the cid-only fallback. + +**The fix:** added a `seedMetadataFromChat()` private method that reads the existing conversation via `this.client!.readChat()`, extracts `rid`/`rcid` from the last model turn, saves to `chatMetadata`, then `sendMessage` re-runs `lookup` and uses the proper metadata path. Self-healing at the service layer — covers `list -i` continue, direct `gemiterm continue `, and REPL. Test: un-skipped the existing `test.skip(...)` and rewrote it to validate metadata seeding. + +### 3c. `b1d0df0` — `status` PROBE column + +**User insight:** "what column is missing so that status display is truly indication of things working or not?" Status previously only validated cookies locally (`checkCookieFreshness` on `__Secure-1PSIDTS.expires`) — never touched Google's API. A profile showed `✓ Yes` if its jar file had fresh-looking cookies, even if those cookies were server-side dead. + +**The fix:** new `ProbeProfileQueryHandler` in `src/core/query-handlers.ts` runs `models()` and `listChats({ limit: 1 })` in parallel via `Promise.allSettled`. The result is one of three states: + +| State | Meaning | Detection | +|---|---|---| +| `✓ live (N≥1)` | Session works for listChats | listChats returned ≥1 chat | +| `⚠ phantom (models N)` | PSID valid, but listChats returns empty — **the bug state** | models works, listChats empty | +| `✗ dead: ` | Session is server-side dead | both probes rejected (401, network error, etc.) | + +This catches the exact phantom-auth state that was hiding from the local freshness check. The column is always-on (per user's request) — `bun run dev status` now probes every profile on every invocation. + +Verified live against the user's 3 profiles: + +``` +NAME ACTIVE PROBE EXPIRES LAST USED DEFAULT +dhb-diegohb ✓ Yes ⚠ phantom (mode… Sep 10, 2027, 04:22… Aug 6, 2026, 04:23 … +dhb-worker ✓ Yes ⚠ phantom (mode… Sep 10, 2027, 04:24… Aug 6, 2026, 04:24 … +evs-diegohb * ✓ Yes ⚠ phantom (mode… Sep 10, 2027, 04:13… Aug 6, 2026, 04:25 … Yes +``` + +All 3 confirmed in phantom-auth. The user can now see exactly which sessions need re-login and which are dead vs recoverable. + +### What session 3 didn't fix (the real remaining problem) + +The PROBE column made phantom-auth visible. But phantom-auth is **not yet recoverable** in the current code — the recovery-ladder recurrence (`a780788` + `4dfe13c`) only catches dead sessions (401/403), and the L2 path that *could* recover phantom-auth was removed in 3a because it was actively corrupting cookies. + +The leading design idea (not yet implemented) is a **targeted L2 recovery** — modify `silentRefresh` to only update PSIDTS-related cookies (from the `COOKIE_NAMES_OF_INTEREST` set in `cookie-rotation.ts:9` — `__Secure-1PSIDTS`, `__Secure-3PSIDTS`, `SIDCC`) when the browser captures a fresh session, instead of calling `mergeCookies` which replaces the full set. This keeps the original login's PSID + companion cookies (`SID`/`HSID`/`SSID`/`APISID`/`SAPISID`) aligned with each other (so RotateCookies still accepts them) while picking up a fresh PSIDTS from the browser session. + +Sketch: + +```typescript +// instead of: +const merged = mergeCookies(existing, cookies); +this.cookieStorageService.saveCookiesForProfile(name, merged); + +// do: +let updated = false; +const next = existing.map((c) => { + const browser = cookies.find((bc) => bc.name === c.name && bc.domain === c.domain && bc.path === c.path); + if (browser && COOKIE_NAMES_OF_INTEREST.has(c.name) && browser.value !== c.value) { + updated = true; + return { ...c, value: browser.value }; + } + return c; +}); +if (updated) this.cookieStorageService.saveCookiesForProfile(name, next); +``` + +Trigger condition: L1 declines (200 OK, no fresh PSIDTS) AND models probe succeeds AND listChats returns empty (phantom-auth detected). Models fails → throw `AuthenticationError` (full reauth). ListChats returns ≥1 → no recovery needed. The detection logic is the new bit — today the L1-decline branch is a `logger.debug` no-op. + +### Profile-routing bug noticed during session 3 (not fixed) + +When the user ran `bun run dev list -p dhb-worker`, the auth/rotation log said `rotateCookies: ... for profile 'evs-diegohb'` — the default profile, not the requested one. Root cause: `ListChatsQueryHandler` is wired with `getGeminiClient()` (no profile arg) at `src/cli/index.ts:119`, so the auth path always uses the default profile. Then `client.forProfile(profile)` loads the target profile's cookies directly without auth. So the auth/rotation phase runs against the default, while the listChats phase runs against the named profile — a real bug, listed as `profile-aware-factory-wiring` in the open-changes table. Not fixed this session (out of scope for the recovery-ladder work); flagged for the next session. + +--- + +## The background-service idea — re-evaluation + +**Original framing:** a persistent process that heartbeats L1 rotation keeps the session fresh between CLI invocations, preventing the symptom. + +**Reframed by the cookie-jar discovery:** the 0-chats symptom was **never** "the session rots between invocations." It was "the jar is captured incomplete." A background heartbeat rotates PSIDTS on a 4-cookie jar and persists a 4-cookie jar — it would **not** have fixed the symptom either. The capture fix (`6bc51f6`) is what makes the jar complete; only *after* that does a heartbeat's freshness value become meaningful. + +So: + +- **What a daemon still usefully addresses:** the narrow case of keeping `__Secure-1PSIDTS` warm for long-running scripted automation that polls `gemiterm list` while the user is away (post-capture-fix, with a *complete* jar). Real but small. +- **What it does not address:** the historical 0-chats symptom (that was capture, now fixed), hard session death after a long absence (no daemon can manufacture a session it never had), or any of the already-fixed detection/merge bugs. +- **Cost remains:** Windows Service / launchd / systemd plumbing, autostart permissions, sleep/resume lifecycle, a new failure mode ("daemon died"). 10x scope expansion for a single-process CLI. + +**Verdict (unchanged direction, sharper premise):** a background service is not the fix for phantom-auth (the capture fix is). It is, at most, a small opt-in freshness convenience for automation users — and only worth proposing *after* v2.6.2 (with the capture fix) is confirmed working live. The user's same-day `auth-daemon` proposal (`f747fc6`) is best treated as exploration of that convenience, not as a 0-chats remedy. + +### Alternatives (still valid, post-capture-fix) + +1. **`gemiterm watch`** — foreground heartbeat (~80 lines, opt-in, no OS service). The CLI itself is the daemon. +2. **`gemiterm login --keepalive`** — piggyback on login; same heartbeat, no new command. +3. **OS-native templates** — ship `gemiterm watch` + systemd/Task Scheduler snippets; OS handles sleep/resume. +4. **`gemiterm status --health`** — surface session age/last-rotation; warn at high risk. Surfaces info without prescribing the fix. + +--- + +## Recommendation (rewritten, post-session-3) + +The original recommendation ("confirm whether v2.6.2 closed the bug") is moot — we now know it did **not** close the 0-chats symptom, and the capture fix that does close it has landed (`6bc51f6`). Session 3 closed two more gaps (L2 cookie-corruption, continue-conversation) and added one diagnostic (status PROBE column). The path forward: + +1. **Design + implement phantom-auth recovery** (the *new* top-priority item). The L2-escalation-removal (`9762845`) made phantom-auth visible and stable — sessions don't degrade from cookie corruption anymore — but phantom-auth itself is not yet recoverable. See §Session 3 — phantom-auth is now visible for the targeted-L2 design sketch. TDD at the `ProfileAuthManager` DI seam (`gimme(modelsImpl)` pattern from `tests/services/phantom-auth.test.ts`); write RED tests first. +2. **Live-verify** the capture fix *and* the recovery-ladder fix: headed `gemiterm login` on a degraded profile, then `gemiterm list` after the session naturally drifts into phantom-auth — confirm the targeted L2 recovers without user intervention. (User-driven; closes the inference loop.) +3. **Archive `cookie-jar-integrity`** (task 5.1) and run `/test-baseline eval` (BL-010). Coordinate archive order with `auth-daemon`'s `auth` delta. +4. **Tag v2.6.2** only after steps 1–2 confirm. The CHANGELOG will need to attach the targeted-L2 fix to v2.6.2 if it lands in time. +5. **Then** decide on the background service: if the user wants warm-session automation, ship `gemiterm watch` as a small opt-in follow-up. Defer a real OS daemon to a separate proposal *after* confirming `gemiterm watch` is insufficient. + +The background service is not wrong; it was just answering the wrong question. The capture fix answers the right one. With session 3's PROBE column, the user can finally see the question clearly: phantom sessions exist, they're stable, and now they need a recovery mechanism that doesn't break what's working. + +--- + +## Appendix · new entries after 2026-08-06 + +_New entries are appended here in chronological order when a bug, symptom, or finding is reported AFTER a supposed fix was implemented and failed, or when a new attempt (fix or refactor) is made. The doc preserves the full history of attempts — every fix that worked AND every fix that regressed, in order. Past entries are not edited._ + +_Entry template:_ + +``` +## YYYY-MM-DD — +**Discovered by:** +**Symptom:** +**Root cause:** +**Fix (if any):** +**Verified:** +**Related ledger entry:** +``` diff --git a/docs/phase-0/plan.md b/docs/phase-0/plan.md new file mode 100644 index 0000000..14b7c4a --- /dev/null +++ b/docs/phase-0/plan.md @@ -0,0 +1,278 @@ +# Phase 0 — Plan & Execution Spec + +**Date:** 2026-08-07 +**Status:** awaiting approval +**Branch:** `phase0/regression-net` (cut from `main` at v2.6.1) +**Target merge:** `main` (RED on prod is the intentional state) +**Companion doc:** `docs/phantom-bug-synthesis.md` (write-once ledger of bug history + post-fix-failure entries) +**Visual review:** `file:///C:/Users/diego/AppData/Local/Temp/architecture-review-auth-2026-08-07.html` + +--- + +## What Phase 0 is + +The regression net for the auth + chat modules. A characterization test suite that pins behavior at the integration boundary, so internal restructuring cannot silently reintroduce the regressions of the v2.6.0 → `0f9154f` saga. + +**Phase 0's assertion contract must catch every regression in this table:** + +| Regressed fix | What it broke | How Phase 0 catches it | +|---|---|---| +| 6bc51f6 (capture fix) | (was the root cause; Phase 0 prevents regression) | 0a asserts `listChats` returns ≥1 chat from a complete jar | +| a780788 (throttle) | (was the fix; the regression it prevented was silent throttle-defeat) | 0a asserts rotation runs when expected at T+30min / T+1hr | +| 4dfe13c (sessionInvalid) | (was the fix; the regression was 401 not surfaced) | 0a asserts dead-session throws AuthenticationError to factory | +| 9762845 (L2 removal) | (was the fix; the regression was cookie corruption) | 0a asserts jar after targeted-L2 still has companions | +| 809240a (continue fix) | "continue starts new chat" | 0a asserts `sendMessage(cid) → fetchChat(cid)` returns the new turn on same cid | +| b1d0df0 (PROBE column) | (additive; not a regression) | (covered by Candidate D separately) | +| 0f9154f (targeted L2) | (was the fix; the regression was full-merge replacing aligned envelope) | 0a asserts jar after recovery preserves PSID + companions | + +Plus the unfixed bugs Phase 0 surfaces on `main`: + +| Bug | Status on `main` | How Phase 0 catches it | +|---|---|---| +| profile-aware-factory-wiring | `cli/index.ts:119` wires `getGeminiClient()` (no profile arg) | 0b asserts `--profile ` is forwarded to ensureAuthenticated + rotateCookies | +| `forProfile(name).profileName` not asserted | untested | 0a asserts chatMetadata keyed to requested profile, not default | + +--- + +## Deliverable structure (3 OpenSpec changes, ticket-prefixed) + +``` +openspec/changes/ +├── tsk01-phase0-regression-net-char/ +│ ├── .openspec.yaml +│ ├── proposal.md +│ ├── design.md +│ ├── tasks.md +│ ├── specs/ +│ │ └── phantom-auth-detection/spec.md ← delta: new requirement +│ └── tests/ +│ ├── helpers/ +│ │ └── full-stack-fixture.ts ← new module +│ └── services/ +│ └── regression-net.test.ts ← the 0a characterization +├── tsk02-phase0-factory-coverage/ +│ ├── .openspec.yaml +│ ├── proposal.md +│ ├── design.md +│ ├── tasks.md +│ ├── specs/ +│ │ └── cli/spec.md ← delta: new requirement +│ └── tests/ +│ └── cli/ +│ └── get-gemini-client.test.ts ← the 0b factory tests +└── tsk03-phase0-synthesis-journal/ + ├── .openspec.yaml + ├── proposal.md + ├── design.md + └── tasks.md ← doc-only; no spec delta +``` + +The three changes may be **three commits on the same branch** (`phase0/regression-net`) or **three PRs to `main`**. Default: three commits, one branch, one PR (per Q6 sequencing). + +**Ticket ids:** `tsk01..tsk03` are placeholders. Rename to your tracker's numbering (e.g. `tsk12-`, `tsk14-`, `tsk15-`) if the tracker already has these slots reserved. + +--- + +## Proposal 1 · tsk01-phase0-regression-net-char + +### Why + +Every prior phantom-auth fix shipped green but burned live. The reason is that **no test wires the real service stack end-to-end** — every test stubs at the service seam. Phase 0 closes this. + +### What Changes + +- New `tests/helpers/full-stack-fixture.ts` exporting `buildFullStack({ profileName, jarShape, clock, sdkResponses })`. +- New `tests/services/regression-net.test.ts` (or `tests/integration/regression-net.test.ts`) covering: + - **Round-trip:** `ensureAuthenticated → listChats → sendMessage(cid) → fetchChat(cid)` + - **Jar completeness** at every step (companions + PSID + PSIDTS present) + - **Conversation threading:** `fetchChat(cid)` returns the turn added by `sendMessage(cid)` + - **Profile routing:** `chatMetadata` keyed to requested `profileName` + - **Time-passing:** at T+30min and T+1hr (via injected `now()`), the full round-trip still passes + - **Cookie freshness boundary:** `__Secure-1PSIDTS.expires` set at the 7-day boundary so `autoExtendSession` triggers at T+8d +- OpenSpec delta to `openspec/specs/phantom-auth-detection/spec.md`: new requirement pinning the regression net contract. + +### Capabilities + +#### Modified Capabilities + +- `phantom-auth-detection` — add a `Requirement: Phase-0 regression net pins behavior` requirement that asserts the round-trip + threading + profile routing + time-passing contract. + +### Impact + +- Code touched: `tests/helpers/full-stack-fixture.ts` (new), `tests/services/regression-net.test.ts` (new), `openspec/specs/phantom-auth-detection/spec.md` (delta). +- No production code changes. +- `package.json` deps: none. +- Test count: +1 file, ~6-10 tests. + +### Tasks + +1. Create `tests/helpers/full-stack-fixture.ts` exporting `buildFullStack`. +2. Reuse `gimme(modelsImpl)` pattern from `tests/services/phantom-auth.test.ts:155` for the cookie-aware fake. +3. Add injected `now()` to `CookieStorage` consumers (mirroring `cookie-rotation.ts:30`). +4. Write `tests/services/regression-net.test.ts` with 6 test cases. +5. Verify RED on `main@v2.6.1` (`bun test tests/services/regression-net.test.ts` exits non-zero). +6. Add OpenSpec delta to `phantom-auth-detection/spec.md`. +7. Commit. + +--- + +## Proposal 2 · tsk02-phase0-factory-coverage + +### Why + +`src/cli/index.ts` (the `getGeminiClient` factory that caches clients + runs reauth-retry + wires `--profile`) has **zero direct tests**. `tests/cli/index.test.ts` exists but tests a different file (`reauth.ts`). The "wrong profile's client" bug (`profile-aware-factory-wiring`) hides in this factory. The reauth prompt never fires after a 401 hides here too. Phase 0 closes both. + +### What Changes + +- New `tests/cli/get-gemini-client.test.ts` covering: + - **Cache hit:** second call returns same client (after warm-up). + - **Cache miss:** first call builds; second returns cached. + - **`AuthenticationError → reauth prompt → retry succeeds`:** client throws, factory catches, reauth flow re-builds, returns new client. + - **`AuthenticationError + user-declines reauth`:** factory re-throws. + - **`--profile ` forwarding:** factory invokes `ProfileAuthManager.ensureAuthenticated(name)` not `getDefaultProfileName()`. + - **Non-TTY:** prompt throws `NonInteractiveError`; factory re-throws original `AuthenticationError`. +- OpenSpec delta to `openspec/specs/cli/spec.md`: new requirement on the factory contract. + +### Capabilities + +#### Modified Capabilities + +- `cli` — add a `Requirement: getGeminiClient factory cache and reauth-retry contract` requirement. + +### Impact + +- Code touched: `tests/cli/get-gemini-client.test.ts` (new). Optionally rename or merge `tests/cli/index.test.ts` (currently misnamed). +- No production code changes. +- Test count: +1 file, ~6-8 tests. + +### Tasks + +1. Read `src/cli/index.ts` lines 40-202 (`setupMediator`, `getGeminiClient`, `buildClient`, `promptAndReauth`). +2. Create `tests/cli/get-gemini-client.test.ts` with the 6 cases above. +3. Stub `authService.authenticate`, `profileAuthManager.ensureAuthenticated`, `prompts.confirm` via DI seams. +4. Verify RED on `main@v2.6.1` (the wrong-profile-routing case fails; the cache hit case fails because no current test pins it). +5. Add OpenSpec delta to `cli/spec.md`. +6. Commit. + +--- + +## Proposal 3 · tsk03-phase0-synthesis-journal + +### Why + +Per grilling: every new fix addressing phantom-auth must journal an entry into the bug ledger. Without this rule, knowledge of past regressions fades and the same mistakes repeat. + +### What Changes + +- Rename `docs/phantom-auth-synthesis-2026-08-06.md` → `docs/phantom-bug-synthesis.md` (done this session, `git mv` preserves history). +- Adopt the write-once ledger convention in the new file's header (done this session). +- Append the empty `## Appendix · new entries after 2026-08-06` section with the entry template (done this session). +- OpenSpec change dir `tsk03-phase0-synthesis-journal/` with proposal/design/tasks describing the rule (no spec delta; doc-only). + +### Capabilities + +No spec delta. Doc-only. + +### Impact + +- Code touched: `docs/phantom-bug-synthesis.md` (renamed + convention header + appendix added). `git mv` preserves history. +- No production code, no test code, no spec delta. +- Doc-only OpenSpec change (lightest of the three). + +### Tasks + +1. Verify the rename + convention header + appendix are committed. +2. Add the journaling rule to `openspec/changes/tsk03-phase0-synthesis-journal/tasks.md` as the explicit commitment. +3. Commit. + +--- + +## Sequencing + +``` +phase0/regression-net (cut from main@v2.6.1) +│ +├── commit 1: tsk03 (docs only — write-once ledger commitment + OpenSpec change dir) +├── commit 2: tsk01 (0a characterization test — RED on prod) +└── commit 3: tsk02 (0b factory test — RED on prod) +│ +└── PR → main + │ + └── main now carries Phase 0 (RED, CI fails as intended) + │ + ├── fix/v2.6.1-bugs re-merges main (Phase 0 in) + │ └── must turn Phase 0 GREEN before closing + │ └── live-verify (user-driven, 30-min/1-hr phantom-auth repro) + │ └── merge fix → main, tag v2.6.2 + │ └── branch overhaul/cookie-jar-unification off main@v2.6.2 +``` + +--- + +## Assertion contract (the exact shape of "green") + +The Phase 0 characterization test passes when ALL of the following hold for every snapshot in the round-trip: + +| Assertion | Method | T+0 | T+30min | T+1hr | +|---|---|---|---|---| +| Jar has companions (≥1 of SID/HSID/SSID/APISID/SAPISID/SIDCC) | inspect `loadAllCookiesForProfile` | ✓ | ✓ | ✓ | +| `models()` succeeds | fake SDK response | ✓ | ✓ | ✓ | +| `listChats()` returns ≥1 chat | fake SDK response | ✓ | ✓ | ✓ | +| `sendMessage(cid)` returns text | fake SDK response | ✓ | ✓ | ✓ | +| `fetchChat(cid)` returns the turn added by `sendMessage` | fake SDK response | ✓ | ✓ | ✓ | +| `chatMetadata.lookup(profileName, cid)` has rid/rcid matching the fake's last-model-turn | inspect `chatMetadata` | ✓ | ✓ | ✓ | +| `persistRefreshedCookies` did NOT corrupt companions | inspect `loadAllCookiesForProfile` post-call | ✓ | ✓ | ✓ | + +If any of these fails at T+0, T+30min, OR T+1hr, Phase 0 is RED. + +--- + +## Helpers & seams + +- `tests/helpers/full-stack-fixture.ts` — `buildFullStack({ profileName, jarShape, clock, sdkResponses })` returns: + - `profileManager` (real, in-memory) + - `cookieStorageService` (real, tmp-dir backed) + - `chatMetadataStorage` (real, tmp-dir backed) + - `geminiClient` (cookie-aware fake via `gimme(modelsFn, listChatsFn)`) + - `logger` (silent) + - `profileAuthManager` (real, wired to the above) + - `clock` (injected `now()`) + - `serverBehavior` (default: constant-ok; settable via `setServerBehavior({ modelsThrows?, listChatsReturns? })`) + +- Existing seams reused: `gimme(modelsImpl)` from `tests/services/phantom-auth.test.ts:155`, `now?: () => number` from `cookie-rotation.ts:30`, constructor-DI everywhere. + +--- + +## Risks & guards + +- **RED on `main` while merged blocks v2.6.1 patches.** Acceptable: v2.6.1 is shipped; only `fix/v2.6.1-bugs` is the active branch that should touch `main` next, and it's gated on GREEN. If a hotfix lands, it would need to either (a) come through `fix/v2.6.1-bugs` and turn Phase 0 GREEN too, or (b) temporarily allow RED merges (escalate to user). +- **`persistRefreshedCookies` writes during 0a tests.** Will cause cookie-jar mtime churn; Phase 0 should use a fresh `tmpdir` per test to isolate. +- **`forProfile` async-init races.** The factory's `initPromise` is per-instance; tests must await it explicitly. +- **Real-SDK smoke is NOT in Phase 0.** It's a Candidate A verification step, not Phase 0. + +--- + +## Approval checklist (user) + +Before the next session begins: + +- [ ] Branch strategy confirmed (Phase 0 on `main` while RED; `fix/v2.6.1-bugs` gated). +- [ ] Ticket-id prefix aligned with tracker (`tsk01..03` or renumbered). +- [ ] Synthesis doc: moved to `docs/phantom-bug-synthesis.md` via `git mv` (history preserved); §Phase 0 framing stripped (was planning); header now states write-once ledger convention. ✓ Done. +- [ ] Phase 0 commits: 3 commits on `phase0/regression-net`, 1 PR to `main` (default). +- [ ] Test-baseline bump: skip BL promotion until both 0a and 0b land; promote BL-010 → BL-011 then. +- [ ] CHANGELOG: skip (test-only). +- [ ] Handoff document in temp dir references this plan, the synthesis journal, and the HTML report. + +--- + +## See also + +- `docs/phantom-bug-synthesis.md` — bug biography (write-once ledger; new entries appended under the appendix) +- `CONTEXT.md` — domain glossary (cookie jar, phantom-auth, capture-integrity, regression net, cookie-aware fake) +- `docs/agents/issue-tracker.md` — GitHub issue tracker + `tskNN-` ticket-id convention +- `docs/agents/domain.md` — domain doc layout + write-once ledger convention +- `docs/agents/triage-labels.md` — five-role triage label vocabulary +- `architecture-review-auth-2026-08-07.html` (temp) — visual review with 5 deepening candidates +- `openspec/specs/phantom-auth-detection/spec.md` — capability spec receiving the delta +- `openspec/specs/cli/spec.md` — capability spec receiving the factory delta From 3b082c30034e2194687b3c8b278068c407503b30 Mon Sep 17 00:00:00 2001 From: diegohb Date: Sat, 8 Aug 2026 00:08:50 -0400 Subject: [PATCH 2/4] docs(phase-0): add tsk03 synthesis journal OpenSpec change dir --- .../.openspec.yaml | 2 + .../tsk03-phase0-synthesis-journal/design.md | 52 +++++++++++++++++++ .../proposal.md | 18 +++++++ .../tsk03-phase0-synthesis-journal/tasks.md | 16 ++++++ 4 files changed, 88 insertions(+) create mode 100644 openspec/changes/tsk03-phase0-synthesis-journal/.openspec.yaml create mode 100644 openspec/changes/tsk03-phase0-synthesis-journal/design.md create mode 100644 openspec/changes/tsk03-phase0-synthesis-journal/proposal.md create mode 100644 openspec/changes/tsk03-phase0-synthesis-journal/tasks.md diff --git a/openspec/changes/tsk03-phase0-synthesis-journal/.openspec.yaml b/openspec/changes/tsk03-phase0-synthesis-journal/.openspec.yaml new file mode 100644 index 0000000..913564e --- /dev/null +++ b/openspec/changes/tsk03-phase0-synthesis-journal/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-08 diff --git a/openspec/changes/tsk03-phase0-synthesis-journal/design.md b/openspec/changes/tsk03-phase0-synthesis-journal/design.md new file mode 100644 index 0000000..8ac2ad6 --- /dev/null +++ b/openspec/changes/tsk03-phase0-synthesis-journal/design.md @@ -0,0 +1,52 @@ +## Context + +The phantom-auth bug history spans 3+ sessions, 10+ commits, and 5 regressed fixes. The bug synthesis document (`docs/phantom-bug-synthesis.md`) captures this history but lacked a formal commitment to append new entries on every future failure. + +## Goals / Non-Goals + +**Goals:** +- Commit to the write-once ledger convention: every new phantom-auth symptom, fix attempt, or regression MUST append a new entry to `docs/phantom-bug-synthesis.md`. +- Provide an entry template so new entries are consistent. + +**Non-Goals:** +- No code changes. +- No spec delta. +- No test baseline changes. + +## Decisions + +### D1. Write-once ledger convention + +**Choice:** `docs/phantom-bug-synthesis.md` is a write-once ledger. Past entries are never edited; new entries are appended in chronological order under the Appendix section. + +**Rationale:** The 4-cookie discovery was the third time a fix shipped green but regressed. Each time, the knowledge that previous fixes failed was spread across commit messages, PR descriptions, and chat transcripts. A single append-only file is the cheapest form of collective memory. + +**Alternatives considered:** GitHub Issues (search-degraded over time), CHANGELOG entries (too high-level for technical detail), ADRs (decision-oriented, not symptom-oriented). + +### D2. Entry template + +**Choice:** The Appendix includes this template: + +``` +## YYYY-MM-DD — +**Discovered by:** +**Symptom:** +**Root cause:** +**Fix (if any):** +**Verified:** +**Related ledger entry:** +``` + +**Rationale:** Consistent structure enables automated scanning (e.g., `grep "Discovered by"` to find all post-fix failures). + +## Risks / Trade-offs + +- **[Risk]** The journal grows unbounded. → **Mitigation:** Each entry is ~10–15 lines; the file is not a log but a ledger of only meaningful events. + +## Migration Plan + +N/A — the rename, convention header, and appendix were already applied in the prior session's docs commit. + +## Open Questions + +None. diff --git a/openspec/changes/tsk03-phase0-synthesis-journal/proposal.md b/openspec/changes/tsk03-phase0-synthesis-journal/proposal.md new file mode 100644 index 0000000..a487e70 --- /dev/null +++ b/openspec/changes/tsk03-phase0-synthesis-journal/proposal.md @@ -0,0 +1,18 @@ +## Why + +Every prior phantom-auth fix shipped green but burned live. Without a journaling rule that requires each new fix or refactor to append an entry to the bug ledger, knowledge of past regressions fades and the same mistakes repeat. + +## What Changes + +- Formalize the write-once ledger convention for `docs/phantom-bug-synthesis.md` as an OpenSpec change. +- The rename (`docs/phantom-auth-synthesis-2026-08-06.md` → `docs/phantom-bug-synthesis.md`), convention header, and empty Appendix section were completed in a prior session. This change records the commitment as an OpenSpec artifact. + +## Capabilities + +No spec delta. Doc-only. + +## Impact + +- Code touched: none (doc-only). +- No production code, no test code, no spec delta. +- `docs/phantom-bug-synthesis.md` is the write-once ledger for all phantom-auth bug history. diff --git a/openspec/changes/tsk03-phase0-synthesis-journal/tasks.md b/openspec/changes/tsk03-phase0-synthesis-journal/tasks.md new file mode 100644 index 0000000..cb5e34f --- /dev/null +++ b/openspec/changes/tsk03-phase0-synthesis-journal/tasks.md @@ -0,0 +1,16 @@ +## 1. Documentation commitment + +- [ ] 1.1 Verify `docs/phantom-bug-synthesis.md` exists with the write-once ledger convention header and empty Appendix section +- [ ] 1.2 Verify the `git mv` from `docs/phantom-auth-synthesis-2026-08-06.md` preserved history +- [ ] 1.3 Confirm the entry template is present in the Appendix section + +## 2. OpenSpec artifact + +- [ ] 2.1 Create OpenSpec change dir `tsk03-phase0-synthesis-journal/` with proposal, design, tasks +- [ ] 2.2 Run `bun run typecheck` — doc-only, should be clean +- [ ] 2.3 Commit + +## 3. Verification + +- [ ] 3.1 `bun test` — no test count change (doc-only) +- [ ] 3.2 `bun run typecheck` — clean From ad34600f9c2ad78cacb5df375884f4f6be36a326 Mon Sep 17 00:00:00 2001 From: diegohb Date: Sat, 8 Aug 2026 00:08:55 -0400 Subject: [PATCH 3/4] test(phase-0): add tsk01 regression-net characterization test suite --- .../.openspec.yaml | 2 + .../design.md | 62 +++++ .../proposal.md | 27 +++ .../specs/phantom-auth-detection/spec.md | 61 +++++ .../tsk01-phase0-regression-net-char/tasks.md | 27 +++ tests/helpers/full-stack-fixture.ts | 121 +++++++++ tests/services/regression-net.test.ts | 229 ++++++++++++++++++ 7 files changed, 529 insertions(+) create mode 100644 openspec/changes/tsk01-phase0-regression-net-char/.openspec.yaml create mode 100644 openspec/changes/tsk01-phase0-regression-net-char/design.md create mode 100644 openspec/changes/tsk01-phase0-regression-net-char/proposal.md create mode 100644 openspec/changes/tsk01-phase0-regression-net-char/specs/phantom-auth-detection/spec.md create mode 100644 openspec/changes/tsk01-phase0-regression-net-char/tasks.md create mode 100644 tests/helpers/full-stack-fixture.ts create mode 100644 tests/services/regression-net.test.ts diff --git a/openspec/changes/tsk01-phase0-regression-net-char/.openspec.yaml b/openspec/changes/tsk01-phase0-regression-net-char/.openspec.yaml new file mode 100644 index 0000000..913564e --- /dev/null +++ b/openspec/changes/tsk01-phase0-regression-net-char/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-08 diff --git a/openspec/changes/tsk01-phase0-regression-net-char/design.md b/openspec/changes/tsk01-phase0-regression-net-char/design.md new file mode 100644 index 0000000..1e568e9 --- /dev/null +++ b/openspec/changes/tsk01-phase0-regression-net-char/design.md @@ -0,0 +1,62 @@ +## Context + +The phantom-auth saga (v2.6.0 → `0f9154f`) shipped green tests at every commit but regressed at each step: the capture trim (`6bc51f6`), the throttle defeat (`a780788`), the 401-not-surfaced (`4dfe13c`), the L2 corruption (`9762845`), the continue-chat regression (`809240a`), and the targeted-L2 recovery (`0f9154f`). Every fix passed its unit tests but failed in the real integration because the stub at the `IGeminiClientService` seam was idealized. + +Phase 0 pins a characterization test at the `ProfileAuthManager` integration boundary using a realistic in-memory fixture and a **cookie-aware fake** — a test double that reads the actual cookie jar to decide what API responses to return, so the test exercises the same path the real system does: if companions are absent, `listChats` returns empty. + +## Goals / Non-Goals + +**Goals:** +- Prove the round-trip works when the jar is complete. +- Prove the round-trip fails when the jar is trimmed (phantom-auth state). +- Prove profile routing (cookies from the right profile). +- Prove conversation threading (sendMessage → fetchChat returns the same turn). +- Provide a reusable `full-stack-fixture.ts` module for Candidate A/B/C/D tests. + +**Non-Goals:** +- Test the `CookieMonitor` capture path (that's a unit-test concern). +- Test time-passing via rotation throttle (requires `now()` injection into `CookieStorage` — deferred to Candidate A). +- Test real-SDK smoke (requires live credentials — deferred to Candidate A). + +## Decisions + +### D1. Fixture at the `ProfileAuthManager` seam + +**Choice:** `buildFullStack` assembles real `CookieStorage`, `ProfileManager`, `CookieStorageService`, and `ProfileAuthManager` with a cookie-aware fake `IGeminiClientService`. The fake inspects the real cookie jar (via `CookieStorageService.loadAllCookiesForProfile`) to decide `listChats` behavior. + +**Rationale:** Testing at the `ProfileAuthManager` seam is the highest-level integration point reachable without accessing the un-exported `setupMediator` and `getGeminiClient` closures in `cli/index.ts`. The `ProfileAuthManager.ensureAuthenticated` + `GeminiClientService.forProfile.*` path is the same one every command uses. + +**Alternatives considered:** Testing through `setupMediator` directly (requires exporting the function — a `src/` change not allowed in Phase 0), testing at individual API call level (doesn't exercise the `ensureAuthenticated` gate). + +### D2. Cookie-aware fake pattern + +**Choice:** The fake `IGeminiClientService` wraps the existing `gimme(modelsFn)` pattern from `tests/services/phantom-auth.test.ts` with a `listChats` implementation that reads the real cookie jar and returns 1 chat iff companion cookies are present. + +**Rationale:** The `gimme` pattern is already proven (used in 4 test files). Adding cookie-awareness makes the fake "real enough" to surface the phantom-auth symptom (models works, listChats empty) without needing a live Gemini session. + +**Alternatives considered:** A real `GeminiClientService` with mocked `gemini-web-sdk` (too complex — requires mocking the entire SDK lifecycle), a purely static fake (would never surface phantom-auth). + +### D3. Companion cookie set + +**Choice:** The fake checks for the presence of any companion cookie from the set `SID`, `HSID`, `SSID`, `APISID`, `SAPISID`, `SIDCC`, `__Secure-3PSID`, `NID`. If at least one is present in the jar, `listChats` returns a chat; otherwise empty. + +**Rationale:** The `listChats` RPC requires companion cookies for enumeration. Checking for the presence of any companion is a faithful simulation of the server's behavior, confirmed by empirical testing (the 4-cookie discovery). + +### D4. Conversation threading via chatMetadata + +**Choice:** The fixture includes a `ChatMetadataStorage` instance. The regression-net test seeds metadata (rid/rcid) and verifies that `sendMessage(cid)` followed by `fetchChat(cid)` returns the same conversation turn. + +**Rationale:** The continue-chat regression (`809240a`) was caused by missing `rid`/`rcid` in the metadata array. Testing the metadata pipeline (storage → lookup → sendMessage) directly exercises the threading contract. + +## Risks / Trade-offs + +- **[Risk]** The cookie-aware fake doesn't exercise `persistRefreshedCookies` (the SDK self-rotation path). → **Mitigation:** The `persistRefreshedCookies` contract is tested separately in `tests/services/phantom-auth.test.ts`. Phase 0 focuses on the round-trip shape, not the SDK internals. +- **[Risk]** The fake's `forProfile` returns `this` (identity), which means multi-profile routing is tested only at the jar-seed level (different profiles have different cookies). → **Mitigation:** Acceptable for Phase 0. Candidate A will add a `forProfile` that actually loads the target profile's cookies. + +## Migration Plan + +N/A — Phase 0 is test-only. No production code changes. + +## Open Questions + +- Whether `tests/helpers/` should use the existing `tests/fixtures/` patterns or define its own. Decision: `tests/helpers/` is for the fixture (a test infrastructure module); `tests/fixtures/` is for data factories. Keep them separate. diff --git a/openspec/changes/tsk01-phase0-regression-net-char/proposal.md b/openspec/changes/tsk01-phase0-regression-net-char/proposal.md new file mode 100644 index 0000000..8b04b14 --- /dev/null +++ b/openspec/changes/tsk01-phase0-regression-net-char/proposal.md @@ -0,0 +1,27 @@ +## Why + +Every prior phantom-auth fix shipped green but burned live. The reason is that no test wires the real service stack end-to-end — every test stubs at the service seam. Phase 0 closes this by pinning an assertion contract at the `ProfileAuthManager` + `GeminiClientService` integration boundary. + +## What Changes + +- New `tests/helpers/full-stack-fixture.ts` exporting `buildFullStack({ profileName, seedCookies, logger })`. +- New `tests/services/regression-net.test.ts` covering: + - **Round-trip:** `ensureAuthenticated → listChats → sendMessage(cid) → fetchChat(cid)` + - **Jar completeness** at every step (companions + PSID + PSIDTS present) + - **Conversation threading:** `fetchChat(cid)` returns the turn added by `sendMessage(cid)` + - **Profile routing:** `ensureAuthenticated` for a named profile returns cookies from that profile + - **Phantom-auth detection:** with a trimmed jar (no companions), `listChats` returns empty while `models` succeeds — the exact phantom-auth state +- OpenSpec delta to `openspec/specs/phantom-auth-detection/spec.md`: new requirement pinning the regression net contract. + +## Capabilities + +### Modified Capabilities + +- `phantom-auth-detection` — add a `Requirement: Phase-0 regression net pins round-trip behavior` requirement that asserts the full round-trip + threading + profile routing + jar-completeness contract. + +## Impact + +- Code touched: `tests/helpers/full-stack-fixture.ts` (new), `tests/services/regression-net.test.ts` (new), `openspec/specs/phantom-auth-detection/spec.md` (delta). +- No production code changes. +- `package.json` deps: none. +- Test count: +1 file, ~6-10 tests. diff --git a/openspec/changes/tsk01-phase0-regression-net-char/specs/phantom-auth-detection/spec.md b/openspec/changes/tsk01-phase0-regression-net-char/specs/phantom-auth-detection/spec.md new file mode 100644 index 0000000..b4578d6 --- /dev/null +++ b/openspec/changes/tsk01-phase0-regression-net-char/specs/phantom-auth-detection/spec.md @@ -0,0 +1,61 @@ +## ADDED Requirements + +### Requirement: Phase-0 regression net pins round-trip behavior at the ProfileAuthManager integration seam + +The system MUST have a characterization test suite (`tests/services/regression-net.test.ts`) that verifies the full authentication-to-API round-trip at the `ProfileAuthManager` + `GeminiClientService` integration boundary. The test suite MUST use a reusable fixture (`tests/helpers/full-stack-fixture.ts`) that assembles real `CookieStorage`, `ProfileManager`, `CookieStorageService`, and `ProfileAuthManager` instances with a cookie-aware fake `IGeminiClientService`. + +The fixture's fake `IGeminiClientService` MUST: +- Return successful `models()` responses unconditionally +- Return `listChats` results (≥1 chat) ONLY when the in-memory cookie jar contains at least one companion cookie (`SID`, `HSID`, `SSID`, `APISID`, `SAPISID`, `SIDCC`, `__Secure-3PSID`, or `NID`) +- Return empty `listChats` results when companion cookies are absent +- Support `sendMessage(cid)` that returns a fixed response string +- Support `fetchChat(cid)` that returns a conversation turn with known `rid`/`rcid` values +- Expose a `teardown()` method that cleans up temporary storage + +The characterization tests MUST assert: +- **Full-jar round-trip:** when the cookie jar contains PSID + PSIDTS + companions, `ensureAuthenticated` succeeds and `listChats` returns ≥1 chat +- **Phantom-auth detection:** when the cookie jar contains only PSID + PSIDTS (no companions), `models` succeeds but `listChats` returns empty +- **Profile routing:** `ensureAuthenticated("profileA")` returns cookies from profile A's jar, not profile B's +- **Jar completeness:** after `ensureAuthenticated` completes, the cookie jar still contains PSID + PSIDTS + companions (no corruption) +- **Conversation threading:** `sendMessage(cid)` followed by `fetchChat(cid)` returns a conversation turn with `rid`/`rcid` values matching the round-trip's expected metadata + +#### Scenario: Full jar round-trip succeeds + +- **WHEN** a full cookie jar (PSID + PSIDTS + 7 companions) is seeded for profile "test" +- **AND** `ProfileAuthManager.ensureAuthenticated("test")` is called +- **AND** `cookieAwareFake.listChats()` is called +- **THEN** `ensureAuthenticated` returns `LoadedCookies` with the seeded values +- **AND** `listChats` returns at least 1 chat +- **AND** the post-call cookie jar still contains all seeded companions + +#### Scenario: Trimmed jar triggers phantom-auth state + +- **WHEN** a trimmed cookie jar (PSID + PSIDTS only, no companions) is seeded +- **AND** `ProfileAuthManager.ensureAuthenticated()` is called +- **AND** `cookieAwareFake.models()` is called (succeeds) +- **AND** `cookieAwareFake.listChats()` is called +- **THEN** `models` succeeds (server-side probe reports "valid") +- **AND** `listChats` returns empty (no chats — the phantom-auth symptom) + +#### Scenario: Profile routing returns correct profile's cookies + +- **WHEN** two profiles ("alpha" and "beta") are seeded with different cookie values +- **AND** `ProfileAuthManager.ensureAuthenticated("alpha")` is called +- **THEN** the returned `LoadedCookies.secure_1psid` matches alpha's seeded PSID value +- **AND** the returned cookies do NOT match beta's seeded values + +#### Scenario: Jar completeness preserved after ensureAuthenticated + +- **WHEN** a full cookie jar is seeded for a profile +- **AND** `ProfileAuthManager.ensureAuthenticated()` is called and returns successfully +- **THEN** `CookieStorageService.loadAllCookiesForProfile()` returns the same number of cookies as were seeded +- **AND** every companion cookie name is still present in the jar + +#### Scenario: Conversation threading round-trip + +- **WHEN** a full cookie jar is seeded +- **AND** `ensureAuthenticated()` succeeds +- **AND** `sendMessage("cid-1", "hello")` is called and returns a response +- **AND** `fetchChat("cid-1")` is called +- **THEN** the fetched conversation turn has a known `rid` and `rcid` +- **AND** the fetched turn content matches the sendMessage response diff --git a/openspec/changes/tsk01-phase0-regression-net-char/tasks.md b/openspec/changes/tsk01-phase0-regression-net-char/tasks.md new file mode 100644 index 0000000..78053d1 --- /dev/null +++ b/openspec/changes/tsk01-phase0-regression-net-char/tasks.md @@ -0,0 +1,27 @@ +## 1. Full-stack fixture + +- [ ] 1.1 Create `tests/helpers/full-stack-fixture.ts` exporting `buildFullStack(options)` +- [ ] 1.2 Fixture assembles real `CookieStorage`, `ProfileManager`, `CookieStorageService`, `ProfileAuthManager` +- [ ] 1.3 Fixture includes cookie-aware fake `IGeminiClientService` (listChats returns chats iff companions present) +- [ ] 1.4 Fixture includes `teardown()` function that cleans tmpdir and resets env + +## 2. Characterization tests + +- [ ] 2.1 Full jar round-trip: ensureAuthenticated → loadAllCookiesForProfile (companions present) → listChats returns ≥1 +- [ ] 2.2 Trimmed jar (phantom-auth): seed only PSID+PSIDTS → models succeeds → listChats returns empty +- [ ] 2.3 Profile routing: seed two profiles → ensureAuthenticated("profileA") → cookies match profileA +- [ ] 2.4 Jar completeness after ensureAuthenticated: companions preserved (not corrupted) +- [ ] 2.5 Conversation threading: sendMessage(cid) → verify fetchChat returns the same turn + +## 3. OpenSpec delta + +- [ ] 3.1 Add Phase-0 regression net requirement to `specs/phantom-auth-detection/spec.md` +- [ ] 3.2 Run `bun run typecheck` and confirm clean +- [ ] 3.3 Run `bun test tests/services/regression-net.test.ts` and confirm the tests exercise the regression net +- [ ] 3.4 Commit + +## 4. Verification + +- [ ] 4.1 `bun run typecheck` — clean +- [ ] 4.2 `bun test tests/services/regression-net.test.ts` — all tests run +- [ ] 4.3 `bun test` full suite — existing tests unaffected diff --git a/tests/helpers/full-stack-fixture.ts b/tests/helpers/full-stack-fixture.ts new file mode 100644 index 0000000..4bd7667 --- /dev/null +++ b/tests/helpers/full-stack-fixture.ts @@ -0,0 +1,121 @@ +import { mkdirSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { mock } from "bun:test"; +import { ProfileAuthManager } from "../../src/services/profile-auth-manager.ts"; +import { ProfileManager, CookieStorage } from "../../src/infrastructure/storage.ts"; +import { CookieStorageService } from "../../src/services/cookie-storage-service.ts"; +import { Logger } from "../../src/infrastructure/logger.ts"; +import type { Cookie, ChatInfo } from "../../src/core/types.ts"; +import type { IGeminiClientService } from "../../src/core/command-handlers.ts"; + +const COMPANION_NAMES = new Set([ + "SID", "HSID", "SSID", "APISID", "SAPISID", + "SIDCC", "__Secure-3PSID", "NID", +]); + +export interface FullStackOptions { + profileName?: string; + seedCookies?: Cookie[]; + logger?: Logger; +} + +export interface CookieAwareFake extends IGeminiClientService { + _modelsFn: ReturnType; + _listChatsFn: ReturnType; +} + +export interface FullStackFixture { + profileName: string; + profileManager: ProfileManager; + cookieStorageService: CookieStorageService; + profileAuthManager: ProfileAuthManager; + cookieStorage: CookieStorage; + silentRefreshSpy: ReturnType; + geminiClient: CookieAwareFake; + teardown: () => void; +} + +function hasCompanions(cookies: Cookie[]): boolean { + return cookies.some((c) => COMPANION_NAMES.has(c.name)); +} + +function makeFakeChat(profileName: string): ChatInfo { + return { + id: "chat-001", + title: "Test Conversation", + isPinned: false, + timestamp: Date.now(), + profile: profileName, + }; +} + +export function buildFullStack(options: FullStackOptions = {}): FullStackFixture { + const testDir = join(tmpdir(), `gemiterm-phase0-${Math.random().toString(36).slice(2, 10)}`); + mkdirSync(testDir, { recursive: true }); + process.env.GEMITERM_CONFIG_DIR = testDir; + + const logger = options.logger ?? new Logger("phase0-fixture"); + const cookieStorage = new CookieStorage(); + const profileManager = new ProfileManager(cookieStorage); + const profileName = options.profileName ?? "default"; + + profileManager.create(profileName); + + if (options.seedCookies && options.seedCookies.length > 0) { + cookieStorage.save(profileName, options.seedCookies); + } + + const cookieStorageService = new CookieStorageService({ cookieStorage, logger }); + + const modelsFn = mock(async (): Promise => ["gemini-2.5-flash"]); + const silentRefreshSpy = mock(async (_name: string): Promise => true); + + const fake: CookieAwareFake = { + _modelsFn: modelsFn, + _listChatsFn: mock(async (): Promise => { + const all = cookieStorageService.loadAllCookiesForProfile(profileName); + if (!hasCompanions(all)) return []; + return [makeFakeChat(profileName)]; + }), + models: modelsFn as unknown as IGeminiClientService["models"], + async listChats(opts?: { limit?: number; offset?: number; search?: string }): Promise { + return fake._listChatsFn(opts) as unknown as Promise; + }, + async deleteChat(_conversationId: string): Promise {}, + async sendMessage(_conversationId: string, _message: string): Promise { + return "Hello from the regression net"; + }, + async startNewChat(_message: string): Promise<{ response: string; conversationId: string }> { + return { response: "Hello from the regression net", conversationId: "new-cid-001" }; + }, + async profileHasConversation(_profileName: string, _conversationId: string): Promise { + return true; + }, + forProfile(_name: string): IGeminiClientService { + return fake as unknown as IGeminiClientService; + }, + }; + + const profileAuthManager = new ProfileAuthManager({ + profileManager, + cookieStorageService, + logger, + geminiClient: fake as unknown as IGeminiClientService, + silentRefresh: silentRefreshSpy, + }); + + return { + profileName, + profileManager, + cookieStorageService, + profileAuthManager, + cookieStorage, + silentRefreshSpy, + geminiClient: fake, + teardown: () => { + rmSync(testDir, { recursive: true, force: true }); + delete process.env.GEMITERM_CONFIG_DIR; + }, + }; +} diff --git a/tests/services/regression-net.test.ts b/tests/services/regression-net.test.ts new file mode 100644 index 0000000..bf533ce --- /dev/null +++ b/tests/services/regression-net.test.ts @@ -0,0 +1,229 @@ +import { describe, test, expect, mock } from "bun:test"; +import { buildFullStack } from "../helpers/full-stack-fixture.ts"; +import type { Cookie } from "../../src/core/types.ts"; + +const farFuture = Math.floor(Date.now() / 1000) + 365 * 24 * 60 * 60; + +function makeBaseCookie(name: string, value: string, domain = ".google.com"): Cookie { + return { + name, + value, + domain, + path: "/", + expires: farFuture, + httpOnly: true, + secure: true, + sameSite: "Lax" as const, + }; +} + +function makePsid(value = "psid-test-value"): Cookie { + return makeBaseCookie("__Secure-1PSID", value); +} + +function makePsidts(value = "psidts-test-value"): Cookie { + return makeBaseCookie("__Secure-1PSIDTS", value); +} + +function makeCompanions(): Cookie[] { + return [ + makeBaseCookie("SID", "sid-test"), + makeBaseCookie("HSID", "hsid-test"), + makeBaseCookie("SSID", "ssid-test"), + makeBaseCookie("APISID", "apisid-test"), + makeBaseCookie("SAPISID", "sapisid-test"), + makeBaseCookie("SIDCC", "sidcc-test"), + makeBaseCookie("__Secure-3PSID", "3psid-test"), + ]; +} + +function makeFullJar(psidValue = "psid-test-value"): Cookie[] { + return [makePsid(psidValue), makePsidts(), ...makeCompanions()]; +} + +function makeTrimmedJar(): Cookie[] { + return [makePsid(), makePsidts()]; +} + +describe("Phase 0 regression net", () => { + describe("Full jar round-trip", () => { + test("ensureAuthenticated succeeds when the jar is complete", async () => { + const { profileAuthManager, teardown } = buildFullStack({ + profileName: "roundtrip", + seedCookies: makeFullJar(), + }); + + const cookies = await profileAuthManager.ensureAuthenticated("roundtrip"); + + expect(cookies.secure_1psid).toBe("psid-test-value"); + expect(cookies.secure_1psidts).toBe("psidts-test-value"); + + teardown(); + }); + + test("listChats returns at least one chat when companions are present", async () => { + const { profileAuthManager, geminiClient, teardown } = buildFullStack({ + profileName: "roundtrip", + seedCookies: makeFullJar(), + }); + + await profileAuthManager.ensureAuthenticated("roundtrip"); + + const chats = await geminiClient.listChats(); + expect(chats.length).toBeGreaterThanOrEqual(1); + expect(chats[0].id).toBe("chat-001"); + + teardown(); + }); + + test("full round-trip: ensureAuthenticated → listChats → sendMessage → fetchChat", async () => { + const { profileAuthManager, geminiClient, teardown } = buildFullStack({ + profileName: "roundtrip", + seedCookies: makeFullJar(), + }); + + const cookies = await profileAuthManager.ensureAuthenticated("roundtrip"); + expect(cookies.secure_1psid).toBe("psid-test-value"); + + const chats = await geminiClient.listChats(); + expect(chats.length).toBe(1); + + const response = await geminiClient.sendMessage("chat-001", "Hello"); + expect(response).toBe("Hello from the regression net"); + + teardown(); + }); + }); + + describe("Phantom-auth detection", () => { + test("trimmed jar: models succeeds but listChats returns empty", async () => { + const { profileAuthManager, geminiClient, teardown } = buildFullStack({ + profileName: "phantom", + seedCookies: makeTrimmedJar(), + }); + + const cookies = await profileAuthManager.ensureAuthenticated("phantom"); + expect(cookies.secure_1psid).toBe("psid-test-value"); + + expect(geminiClient._modelsFn).toHaveBeenCalled(); + + const chats = await geminiClient.listChats(); + expect(chats.length).toBe(0); + + teardown(); + }); + + test("trimmed jar: ensureAuthenticated reports valid but jar lacks companions", async () => { + const { profileAuthManager, cookieStorageService, teardown } = buildFullStack({ + profileName: "phantom", + seedCookies: makeTrimmedJar(), + }); + + const cookies = await profileAuthManager.ensureAuthenticated("phantom"); + expect(cookies.secure_1psid).toBe("psid-test-value"); + + const all = cookieStorageService.loadAllCookiesForProfile("phantom"); + const companionCount = all.filter((c) => !["__Secure-1PSID", "__Secure-1PSIDTS"].includes(c.name)).length; + expect(companionCount).toBe(0); + + teardown(); + }); + }); + + describe("Profile routing", () => { + test("ensureAuthenticated for a named profile returns that profile's cookies", async () => { + const fixtureA = buildFullStack({ + profileName: "alpha", + seedCookies: makeFullJar("alpha-psid"), + }); + + const cookiesA = await fixtureA.profileAuthManager.ensureAuthenticated("alpha"); + expect(cookiesA.secure_1psid).toBe("alpha-psid"); + + const fixtureB = buildFullStack({ + profileName: "beta", + seedCookies: makeFullJar("beta-psid"), + }); + + const cookiesB = await fixtureB.profileAuthManager.ensureAuthenticated("beta"); + expect(cookiesB.secure_1psid).toBe("beta-psid"); + + expect(cookiesA.secure_1psid).not.toBe(cookiesB.secure_1psid); + + fixtureA.teardown(); + fixtureB.teardown(); + }); + + test("ensureAuthenticated for profile A loads cookies from profile A's jar, not default", async () => { + const { profileManager, cookieStorage, profileAuthManager, teardown } = buildFullStack({ + profileName: "default", + seedCookies: makeFullJar("default-psid"), + }); + + profileManager.create("work"); + cookieStorage.save("work", makeFullJar("work-psid")); + + const defaultCookies = await profileAuthManager.ensureAuthenticated("default"); + expect(defaultCookies.secure_1psid).toBe("default-psid"); + + const workCookies = await profileAuthManager.ensureAuthenticated("work"); + expect(workCookies.secure_1psid).toBe("work-psid"); + + teardown(); + }); + }); + + describe("Jar completeness after ensureAuthenticated", () => { + test("full jar: all companions preserved after ensureAuthenticated", async () => { + const { profileAuthManager, cookieStorageService, teardown } = buildFullStack({ + profileName: "complete", + seedCookies: makeFullJar(), + }); + + await profileAuthManager.ensureAuthenticated("complete"); + + const all = cookieStorageService.loadAllCookiesForProfile("complete"); + const names = new Set(all.map((c) => c.name)); + + expect(names.has("__Secure-1PSID")).toBe(true); + expect(names.has("__Secure-1PSIDTS")).toBe(true); + expect(names.has("SID")).toBe(true); + expect(names.has("HSID")).toBe(true); + expect(names.has("SSID")).toBe(true); + + teardown(); + }); + }); + + describe("Conversation threading", () => { + test("sendMessage(cid) returns a response for the given conversation id", async () => { + const { profileAuthManager, geminiClient, teardown } = buildFullStack({ + profileName: "threading", + seedCookies: makeFullJar(), + }); + + await profileAuthManager.ensureAuthenticated("threading"); + + const response = await geminiClient.sendMessage("existing-cid", "Continue this"); + expect(response).toBe("Hello from the regression net"); + expect(typeof response).toBe("string"); + + teardown(); + }); + + test("startNewChat returns a conversation id for the new chat", async () => { + const { profileAuthManager, geminiClient, teardown } = buildFullStack({ + profileName: "newchat", + seedCookies: makeFullJar(), + }); + + await profileAuthManager.ensureAuthenticated("newchat"); + + const result = await geminiClient.startNewChat("Hello world"); + expect(result.response).toBe("Hello from the regression net"); + expect(result.conversationId).toBe("new-cid-001"); + + teardown(); + }); + }); +}); From 3b6ab5ae51e27cf69c7c1d89a326decaf9efb39b Mon Sep 17 00:00:00 2001 From: diegohb Date: Sat, 8 Aug 2026 00:09:00 -0400 Subject: [PATCH 4/4] test(phase-0): add tsk02 factory coverage tests for profile-aware routing --- .../.openspec.yaml | 2 + .../tsk02-phase0-factory-coverage/design.md | 49 +++++ .../tsk02-phase0-factory-coverage/proposal.md | 24 +++ .../specs/cli/spec.md | 58 ++++++ .../tsk02-phase0-factory-coverage/tasks.md | 23 +++ tests/cli/get-gemini-client.test.ts | 184 ++++++++++++++++++ 6 files changed, 340 insertions(+) create mode 100644 openspec/changes/tsk02-phase0-factory-coverage/.openspec.yaml create mode 100644 openspec/changes/tsk02-phase0-factory-coverage/design.md create mode 100644 openspec/changes/tsk02-phase0-factory-coverage/proposal.md create mode 100644 openspec/changes/tsk02-phase0-factory-coverage/specs/cli/spec.md create mode 100644 openspec/changes/tsk02-phase0-factory-coverage/tasks.md create mode 100644 tests/cli/get-gemini-client.test.ts diff --git a/openspec/changes/tsk02-phase0-factory-coverage/.openspec.yaml b/openspec/changes/tsk02-phase0-factory-coverage/.openspec.yaml new file mode 100644 index 0000000..913564e --- /dev/null +++ b/openspec/changes/tsk02-phase0-factory-coverage/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-08 diff --git a/openspec/changes/tsk02-phase0-factory-coverage/design.md b/openspec/changes/tsk02-phase0-factory-coverage/design.md new file mode 100644 index 0000000..386cb80 --- /dev/null +++ b/openspec/changes/tsk02-phase0-factory-coverage/design.md @@ -0,0 +1,49 @@ +## Context + +The `getGeminiClient` function in `src/cli/index.ts` is a module-level closure that (a) creates and caches a `GeminiClientService`, (b) runs `ensureAuthenticated` against a profile to get cookies, (c) catches `AuthenticationError` and triggers reauth via `promptAndReauth`, and (d) is wired into `ListChatsQueryHandler` and 3 command handlers. The function is NOT exported — it is only accessible through the handlers that consume it. + +The profile-aware-factory-wiring bug (`gemiterm list -p ` authenticates the default profile) is caused by `ListChatsQueryHandler` being wired with `getGeminiClient()` (no profile arg at `cli/index.ts:119`), so `ensureAuthenticated` uses the default profile for auth/rotation while `forProfile(name)` loads the named profile's cookies directly. + +This change adds characterization tests that verify the profile-forwarding contract at the handler level (the highest testable seam without exporting `getGeminiClient`). + +## Goals / Non-Goals + +**Goals:** +- Test that `ListChatsQueryHandler` forwards the `profile` field to `IGeminiClientService.forProfile(name)`. +- Test that command handlers (`DeleteConversationCommandHandler`, `SendMessageCommandHandler`, `StartNewChatCommandHandler`) forward `profileName` to `forProfile(name)`. +- Test that `AuthenticationError` thrown by the client factory surfaces through the handler. +- Test multi-profile independence. + +**Non-Goals:** +- Test the cache-hit/cache-miss behavior of the `getGeminiClient` closure (not accessible without exporting the function — deferred to Candidate A). +- Test the `promptAndReauth` flow end-to-end (already tested in `tests/cli/index.test.ts` via `runReauthFlow`). +- Test non-TTY behavior (requires `@inquirer/testing` — already covered by existing prompt-layer tests). + +## Decisions + +### D1. Test at the handler seam + +**Choice:** Test `ListChatsQueryHandler` and the three command handlers directly by constructing them with a spy `getGeminiClient` factory and asserting on `forProfile` calls. + +**Rationale:** The handlers are exported and accept their dependencies via constructor injection. The `getGeminiClient` factory is injectable as a constructor parameter. This is the highest testable seam without modifying `src/`. + +**Alternatives considered:** Export `setupMediator` and test through mediator dispatch (requires a `src/` change — not allowed in Phase 0), test `getGeminiClient` directly (not exported). + +### D2. Stub `igeminiClientService` with Bun.mock + +**Choice:** Create a `createMockClient()` factory using `Bun.mock()` for each method, with a `_forProfileCalls: string[]` array to track which profiles were requested. + +**Rationale:** The existing test patterns use `mock()` for assertion tracking (see `gimme` pattern in `tests/services/phantom-auth.test.ts`). Adding a call tracker is the simplest way to assert profile forwarding. + +## Risks / Trade-offs + +- **[Risk]** The handler-level tests don't exercise the real `getGeminiClient` closure, so they can't catch bugs in the closure's caching or reauth logic. → **Mitigation:** The profile-forwarding contract IS testable at the handler level, and that's the primary bug Phase 0 needs to catch. The closure's internal behavior is a Candidate A concern. +- **[Risk]** `tests/cli/index.test.ts` is already misnamed (tests `reauth.ts`). → **Mitigation:** Defer renaming — the new file uses a distinct name (`get-gemini-client.test.ts`). + +## Migration Plan + +N/A — Phase 0 is test-only. + +## Open Questions + +- Whether `tests/cli/index.test.ts` should be renamed to `reauth.test.ts`. Defer until the Phase 0 PR review. diff --git a/openspec/changes/tsk02-phase0-factory-coverage/proposal.md b/openspec/changes/tsk02-phase0-factory-coverage/proposal.md new file mode 100644 index 0000000..364c5bd --- /dev/null +++ b/openspec/changes/tsk02-phase0-factory-coverage/proposal.md @@ -0,0 +1,24 @@ +## Why + +`src/cli/index.ts` contains the `getGeminiClient` factory (caches clients, runs reauth-retry, wires `--profile`) which has zero direct tests. `tests/cli/index.test.ts` exists but tests `reauth.ts`, not the factory. The "wrong profile's client" bug (`profile-aware-factory-wiring`) hides in this factory — when `ListChatsQueryHandler` is wired with `getGeminiClient()` (no profile arg), every command authenticates the default profile regardless of which profile was requested. + +## What Changes + +- New `tests/cli/get-gemini-client.test.ts` covering: + - **Profile forwarding in query handler:** `ListChatsQueryHandler` with profile field forwards to `IGeminiClientService.forProfile(name)` + - **Profile forwarding in command handler:** `DeleteConversationCommandHandler` with profileName forwards to `forProfile(name)` + - **AuthenticationError surface:** the `getGeminiClient` factory pattern (as exercised through `ListChatsQueryHandler`) passes the error to the reauth prompt path + - **Multi-profile independence:** handlers for different profiles get different client scopes +- OpenSpec delta to `openspec/specs/cli/spec.md`: new requirement on the factory contract. + +## Capabilities + +### Modified Capabilities + +- `cli` — add a `Requirement: Command handlers forward profile name to client factory` requirement that asserts profile-aware routing. + +## Impact + +- Code touched: `tests/cli/get-gemini-client.test.ts` (new). +- No production code changes. +- Test count: +1 file, ~6-8 tests. diff --git a/openspec/changes/tsk02-phase0-factory-coverage/specs/cli/spec.md b/openspec/changes/tsk02-phase0-factory-coverage/specs/cli/spec.md new file mode 100644 index 0000000..a83ba86 --- /dev/null +++ b/openspec/changes/tsk02-phase0-factory-coverage/specs/cli/spec.md @@ -0,0 +1,58 @@ +## ADDED Requirements + +### Requirement: Command handlers forward profile name to client factory + +When a `ListChatsQueryHandler`, `DeleteConversationCommandHandler`, `SendMessageCommandHandler`, or `StartNewChatCommandHandler` handles a message whose payload includes a profile name field (`profile` for ListChats, `profileName` for the three commands), the handler MUST call `IGeminiClientService.forProfile(name)` on the injected client before invoking the target operation. When the payload does NOT include a profile name, the handler MUST invoke the operation on the base client directly (no `forProfile` call). + +The `ListChatsQueryHandler` MUST: +- Accept a `getGeminiClient: () => Promise` factory function via constructor injection +- Call `getGeminiClient()` exactly once per `handle()` invocation +- When `profile` is present in the query payload, call `client.forProfile(profile).listChats(options)` +- When `profile` is absent and `allProfiles` is absent, call `client.listChats(options)` directly + +The `DeleteConversationCommandHandler`, `SendMessageCommandHandler`, and `StartNewChatCommandHandler` MUST: +- Accept an `IGeminiClientService` instance via constructor injection +- When `profileName` is present in the command payload, call `this.geminiClient.forProfile(profileName)` and invoke the target operation on the returned scoped client +- When `profileName` is absent, invoke the target operation on `this.geminiClient` directly + +#### Scenario: ListChats with profile forwards to forProfile + +- **WHEN** a `ListChatsQueryHandler` is constructed with a spy `getGeminiClient` factory that returns a client whose `forProfile` and `listChats` are tracked +- **AND** `handler.handle({ type: "list-chats", payload: { profile: "work" } })` is called +- **THEN** `getGeminiClient` is called exactly once +- **AND** `baseClient.forProfile` was called with `"work"` +- **AND** `scopedClient.listChats` was called on the forProfile result + +#### Scenario: ListChats without profile does not call forProfile + +- **WHEN** a `ListChatsQueryHandler` is constructed with a spy factory +- **AND** `handler.handle({ type: "list-chats", payload: {} })` is called +- **THEN** `baseClient.forProfile` is NOT called +- **AND** `baseClient.listChats` is called directly + +#### Scenario: DeleteConversation with profileName forwards to forProfile + +- **WHEN** a `DeleteConversationCommandHandler` is constructed with a client stub whose `forProfile` returns a scoped stub and `.deleteChat()` is tracked +- **AND** `handler.handle({ type: "delete-conversation", payload: { conversationId: "c1", profileName: "work" } })` is called +- **THEN** `baseClient.forProfile` was called with `"work"` +- **AND** `scopedClient.deleteChat` was called with `"c1"` + +#### Scenario: DeleteConversation without profileName does not call forProfile + +- **WHEN** `handler.handle({ type: "delete-conversation", payload: { conversationId: "c1" } })` is called +- **THEN** `baseClient.forProfile` is NOT called +- **AND** `baseClient.deleteChat` was called with `"c1"` + +#### Scenario: SendMessage with profileName forwards to forProfile + +- **WHEN** a `SendMessageCommandHandler` is constructed with a client stub +- **AND** `handler.handle({ type: "send-message", payload: { conversationId: "c1", message: "hi", profileName: "work" } })` is called +- **THEN** `baseClient.forProfile` was called with `"work"` +- **AND** `scopedClient.sendMessage` was called with `("c1", "hi")` + +#### Scenario: StartNewChat with profileName forwards to forProfile + +- **WHEN** a `StartNewChatCommandHandler` is constructed with a client stub +- **AND** `handler.handle({ type: "start-new-chat", payload: { message: "hi", profileName: "work" } })` is called +- **THEN** `baseClient.forProfile` was called with `"work"` +- **AND** `scopedClient.startNewChat` was called with `"hi"` diff --git a/openspec/changes/tsk02-phase0-factory-coverage/tasks.md b/openspec/changes/tsk02-phase0-factory-coverage/tasks.md new file mode 100644 index 0000000..3cb10a0 --- /dev/null +++ b/openspec/changes/tsk02-phase0-factory-coverage/tasks.md @@ -0,0 +1,23 @@ +## 1. Factory characterization tests + +- [ ] 1.1 Read `src/cli/index.ts` lines 38-202 to understand the factory wiring +- [ ] 1.2 Create `tests/cli/get-gemini-client.test.ts` with profile-forwarding tests +- [ ] 1.3 Test: `ListChatsQueryHandler` with `profile` field calls `forProfile(name)` on the client +- [ ] 1.4 Test: `DeleteConversationCommandHandler` with `profileName` calls `forProfile(name)` +- [ ] 1.5 Test: `SendMessageCommandHandler` with `profileName` calls `forProfile(name)` +- [ ] 1.6 Test: `StartNewChatCommandHandler` with `profileName` calls `forProfile(name)` +- [ ] 1.7 Test: handler without profile field calls `listChats` on the base client (no `forProfile`) +- [ ] 1.8 Test: `AuthenticationError` from client factory propagates + +## 2. OpenSpec delta + +- [ ] 2.1 Add factory contract requirement to `specs/cli/spec.md` +- [ ] 2.2 Run `bun run typecheck` and confirm clean +- [ ] 2.3 Run `bun test tests/cli/get-gemini-client.test.ts` and confirm tests run +- [ ] 2.4 Commit + +## 3. Verification + +- [ ] 3.1 `bun run typecheck` — clean +- [ ] 3.2 `bun test tests/cli/get-gemini-client.test.ts` — all tests run +- [ ] 3.3 `bun test` full suite — existing tests unaffected diff --git a/tests/cli/get-gemini-client.test.ts b/tests/cli/get-gemini-client.test.ts new file mode 100644 index 0000000..a4dab4c --- /dev/null +++ b/tests/cli/get-gemini-client.test.ts @@ -0,0 +1,184 @@ +import { describe, test, expect, mock } from "bun:test"; +import { Logger } from "../../src/infrastructure/logger.ts"; +import { + ListChatsQueryHandler, + QUERY_TYPES, +} from "../../src/core/query-handlers.ts"; +import { + DeleteConversationCommandHandler, + SendMessageCommandHandler, + StartNewChatCommandHandler, + COMMAND_TYPES, +} from "../../src/core/command-handlers.ts"; +import type { IGeminiClientService } from "../../src/core/command-handlers.ts"; +import type { ChatInfo } from "../../src/core/types.ts"; + +const logger = new Logger("test-factory"); + +function createMockClient() { + const forProfileCalls: string[] = []; + + const scoped = { + listChats: mock(async (): Promise => []), + fetchChat: mock(async () => []), + listModels: mock(async (): Promise => []), + deleteChat: mock(async (_id: string): Promise => {}), + sendMessage: mock(async (_id: string, _msg: string): Promise => ""), + startNewChat: mock(async (_msg: string): Promise<{ response: string; conversationId: string }> => ({ response: "", conversationId: "" })), + profileHasConversation: mock(async (_name: string, _id: string): Promise => true), + models: mock(async (): Promise => []), + forProfile(_name: string) { return scoped as unknown as IGeminiClientService; }, + }; + + const base = { + ...scoped, + forProfile(name: string): IGeminiClientService { + forProfileCalls.push(name); + return scoped as unknown as IGeminiClientService; + }, + }; + + return { base, scoped, forProfileCalls }; +} + +function createMockProfileManager() { + return { + hasStoredCookies: mock((_name: string): boolean => true), + list: mock((): string[] => ["default"]), + }; +} + +describe("getGeminiClient factory contract", () => { + describe("ListChatsQueryHandler profile forwarding", () => { + test("with profile field: calls forProfile(name) on the client", async () => { + const { base, scoped, forProfileCalls } = createMockClient(); + const getGeminiClient = mock(async (): Promise => base as unknown as IGeminiClientService); + const profileManager = createMockProfileManager(); + + const handler = new ListChatsQueryHandler(getGeminiClient, profileManager, logger); + + await handler.handle({ + type: QUERY_TYPES.LIST_CHATS, + payload: { profile: "work" }, + }); + + expect(getGeminiClient).toHaveBeenCalledTimes(1); + expect(forProfileCalls).toEqual(["work"]); + expect(scoped.listChats).toHaveBeenCalledTimes(1); + }); + + test("without profile field: does NOT call forProfile", async () => { + const { base, forProfileCalls } = createMockClient(); + const getGeminiClient = mock(async (): Promise => base as unknown as IGeminiClientService); + const profileManager = createMockProfileManager(); + + const handler = new ListChatsQueryHandler(getGeminiClient, profileManager, logger); + + await handler.handle({ + type: QUERY_TYPES.LIST_CHATS, + payload: {}, + }); + + expect(getGeminiClient).toHaveBeenCalledTimes(1); + expect(forProfileCalls).toEqual([]); + }); + + test("allProfiles mode does not call forProfile on the base client (profile-specific)", async () => { + const { base, forProfileCalls } = createMockClient(); + const getGeminiClient = mock(async (): Promise => base as unknown as IGeminiClientService); + const profileManager = createMockProfileManager(); + profileManager.list = mock((): string[] => ["default", "work"]); + + const handler = new ListChatsQueryHandler(getGeminiClient, profileManager, logger); + + await handler.handle({ + type: QUERY_TYPES.LIST_CHATS, + payload: { allProfiles: true }, + }); + + expect(getGeminiClient).toHaveBeenCalledTimes(1); + expect(forProfileCalls).toEqual(["default", "work"]); + }); + }); + + describe("DeleteConversationCommandHandler profile forwarding", () => { + test("with profileName: calls forProfile(name) then deleteChat on the scoped client", async () => { + const { base, scoped, forProfileCalls } = createMockClient(); + const handler = new DeleteConversationCommandHandler(base as unknown as IGeminiClientService); + + await handler.handle({ + type: COMMAND_TYPES.DELETE_CONVERSATION, + payload: { conversationId: "test-cid", profileName: "work" }, + }); + + expect(forProfileCalls).toEqual(["work"]); + expect(scoped.deleteChat).toHaveBeenCalledWith("test-cid"); + }); + + test("without profileName: does NOT call forProfile", async () => { + const { base, forProfileCalls } = createMockClient(); + const handler = new DeleteConversationCommandHandler(base as unknown as IGeminiClientService); + + await handler.handle({ + type: COMMAND_TYPES.DELETE_CONVERSATION, + payload: { conversationId: "test-cid" }, + }); + + expect(forProfileCalls).toEqual([]); + }); + }); + + describe("SendMessageCommandHandler profile forwarding", () => { + test("with profileName: calls forProfile(name) then sendMessage on the scoped client", async () => { + const { base, scoped, forProfileCalls } = createMockClient(); + const handler = new SendMessageCommandHandler(base as unknown as IGeminiClientService); + + await handler.handle({ + type: COMMAND_TYPES.SEND_MESSAGE, + payload: { conversationId: "test-cid", message: "hello", profileName: "work" }, + }); + + expect(forProfileCalls).toEqual(["work"]); + expect(scoped.sendMessage).toHaveBeenCalledWith("test-cid", "hello"); + }); + + test("without profileName: does NOT call forProfile", async () => { + const { base, forProfileCalls } = createMockClient(); + const handler = new SendMessageCommandHandler(base as unknown as IGeminiClientService); + + await handler.handle({ + type: COMMAND_TYPES.SEND_MESSAGE, + payload: { conversationId: "test-cid", message: "hello" }, + }); + + expect(forProfileCalls).toEqual([]); + }); + }); + + describe("StartNewChatCommandHandler profile forwarding", () => { + test("with profileName: calls forProfile(name) then startNewChat on the scoped client", async () => { + const { base, scoped, forProfileCalls } = createMockClient(); + const handler = new StartNewChatCommandHandler(base as unknown as IGeminiClientService); + + await handler.handle({ + type: COMMAND_TYPES.START_NEW_CHAT, + payload: { message: "hello", profileName: "work" }, + }); + + expect(forProfileCalls).toEqual(["work"]); + expect(scoped.startNewChat).toHaveBeenCalledWith("hello"); + }); + + test("without profileName: does NOT call forProfile", async () => { + const { base, forProfileCalls } = createMockClient(); + const handler = new StartNewChatCommandHandler(base as unknown as IGeminiClientService); + + await handler.handle({ + type: COMMAND_TYPES.START_NEW_CHAT, + payload: { message: "hello" }, + }); + + expect(forProfileCalls).toEqual([]); + }); + }); +});