From dafc2d9c5f0111c66b1e684b43186508baf5279e Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Fri, 17 Jul 2026 21:58:54 +0200 Subject: [PATCH] relay(issue-62-authstorage): leg 6 status/log (slice 5 done, verify green) --- relays/issue-62-authstorage/log.md | 54 ++++++++++ relays/issue-62-authstorage/status.md | 137 ++++++++++++++++---------- 2 files changed, 141 insertions(+), 50 deletions(-) diff --git a/relays/issue-62-authstorage/log.md b/relays/issue-62-authstorage/log.md index 14aa287..7e89c3b 100644 --- a/relays/issue-62-authstorage/log.md +++ b/relays/issue-62-authstorage/log.md @@ -313,3 +313,57 @@ sessionRoutes (1). `piSessionService.ts` lints clean. Committed `--no-verify` added `piSessionService.ts` (a session-daemon path) to the pending-restart surface; do not clear the note. Handing off to leg 6 (slice 5: tests + testSupport). + +## Leg 6 — slice 5 (tests + testSupport migration) — commit `d0cc55c` + +**What:** Migrated all test doubles + `piSessionService.testSupport.ts` off the +removed `AuthStorage.inMemory` / `ModelRegistry.create|inMemory` surface to the +pi-ai `InMemoryCredentialStore` + async `ModelRuntime.create({ credentials })`. +`npm run verify` is now **fully green** (typecheck + lint + knip + 1390 tests, +2 skipped) — the migration goal (charter criteria 1, 2, 5) is met; only the +changeset (criterion 4) remains for slice 6. + +**Files changed (all `src/server/sessions/`):** +- `piSessionService.testSupport.ts`: new seams `createTestModelRuntime`, + shared `testModelRuntime` (top-level await), `seedCredential`; `fakeRuntime` + and `testModel` moved onto `modelRuntime`; dropped AuthStorage/ModelRegistry. +- `archiveCleanup`/`lifecycle`/`promptQueue`/`spawnSession`/`spawnSubsession`/ + `sessionRoutes` tests: injected `modelRuntime: testModelRuntime` into every + `new PiSessionService(...)` (now required) + imported the shared runtime. +- `promptQueue.test.ts` auth-loss test: live `InMemoryCredentialStore` + + `createTestModelRuntime(credentials)`, driving changes through + `delete`/`seedCredential` + `refresh()` + `applyAuthChange(...)`. +- `warnings.test.ts`: `anthropicSubscriptionWarning` seam is now a temp + `auth.json` read via `readStoredCredential(id, authPath)`; type narrowed. +- `authService.test.ts`: reworked to async `AuthService.create` + + `InMemoryCredentialStore`; awaits async ops; OAuth-complete uses `vi.waitFor`. + +**Decisions:** +- Used a single shared `testModelRuntime` (top-level `await` in testSupport, an + allowed ESM pattern here) for the no-auth catalog case so the many + `PiSessionService` constructions and `fakeRuntime` sessions inject it + synchronously — avoided making `fakeRuntime` itself async (which would have + rippled through ~90 call sites). Auth-dependent tests build a dedicated + per-test runtime via `createTestModelRuntime(credentials)`. +- `anthropicSubscriptionWarning` seam: chose the on-disk temp `auth.json` + + `readStoredCredential` path (matches production exactly) rather than adding a + new injectable credential-read seam. Clean; no intervention needed. +- Made `getLoginProviderOptions` **synchronous** (it did no async work) to + satisfy `require-await`; de-awaited its 2 call sites in `authService.ts` and + the test. Also fixed pre-existing lint debt from earlier slices surfaced now + that lint ran green for the first time: `authRoutes.ts` return-await, + `authService.ts` api-key interaction (`() => Promise.resolve(key)` / + `notify: () => undefined`). +- Verified SDK behavior empirically with throwaway probe scripts (removed) + before writing doubles: providers/models catalog, api-key login persistence, + `getModel`, `hasConfiguredAuth` across store mutations + refresh, and + `readStoredCredential` against a temp auth.json. + +**Verification:** `npm run verify` green. Pre-commit hook (whole-project +typecheck + knip + eslint + related vitest) passed — committed normally +(no `--no-verify` needed since the tree is verify-green). + +**Blockers:** none. **Sessiond-restart-pending note still ACTIVE** — unchanged +this leg but slices 1 + 4 touched session-daemon paths; only the human clears +it after restarting the sessiond service. Handing off to leg 7 (slice 6: +changeset + final verify + cleanup; the finish line — no PR). diff --git a/relays/issue-62-authstorage/status.md b/relays/issue-62-authstorage/status.md index 1f6d385..ed4ec98 100644 --- a/relays/issue-62-authstorage/status.md +++ b/relays/issue-62-authstorage/status.md @@ -1,6 +1,59 @@ # Relay status — issue-62-authstorage ## Current position +Slice 5 (tests + testSupport) complete and committed (`d0cc55c`). +**`npm run verify` is fully GREEN** — typecheck + lint + knip + 1390 tests +pass (2 skipped). All production code and all test/support code are now off the +removed `AuthStorage` / `ModelRegistry.create|inMemory` surface. Goal criteria +1, 2, 5 are met; criteria 3 (dep ranges) was done in slice 0/1; only criterion +4 (changeset) remains — that is slice 6. + +What slice 5 changed (all under `src/server/sessions/`): +- **`piSessionService.testSupport.ts`** (central helper): dropped + `AuthStorage`/`ModelRegistry` imports; added pi-ai `InMemoryCredentialStore`. + New seams: `createTestModelRuntime(credentials?)` (wraps + `ModelRuntime.create({ credentials })`), a shared `testModelRuntime` + (top-level `await createTestModelRuntime()` — the common no-auth catalog + runtime), and `seedCredential(store, providerId, credential)` (writes via the + `CredentialStore.modify` path). `fakeRuntime` session now carries + `modelRuntime: testModelRuntime`; `testModel()` reads + `testModelRuntime.getModel(...)`. +- Threaded `modelRuntime: testModelRuntime` into every `new PiSessionService(...)` + (now a required dep) across `archiveCleanup`/`lifecycle`/`promptQueue`/ + `spawnSession`/`spawnSubsession`/`sessionRoutes` tests, importing + `testModelRuntime` in each. +- **`piSessionService.promptQueue.test.ts`** auth-loss test rewritten: builds a + live `InMemoryCredentialStore` + `createTestModelRuntime(credentials)`, and + simulates auth changes via `credentials.delete/seedCredential` + + `modelRuntime.refresh()` + `applyAuthChange(...)` (matching AuthService's + real refresh-then-emit sequence). Removed the `modelRegistry` dep line. +- **`piSessionService.warnings.test.ts`**: `anthropicSubscriptionWarning` now + reads `readStoredCredential("anthropic", authPath)`, so the test seam is a + temp `auth.json` written per case (helper `anthropicAuthPath(...)`), passed + as the 2nd arg. `SubscriptionSession` type narrowed to + `Pick`. The "no credential" + case points at a temp dir with no auth.json (deterministic). +- **`authService.test.ts`** fully reworked to the async `AuthService.create({ + runtime | agentDir })` + `InMemoryCredentialStore` model. `saveApiKey`/ + `logoutProvider`/`startOAuthLogin` are awaited; OAuth-complete test asserts + `startOptions.runtime === runtime` and uses `vi.waitFor` for the async + refresh; credential assertions go through `credentials.read(...)`. +- Lint fixes surfaced by running lint green for the first time this relay: + `getLoginProviderOptions` made **synchronous** (it did no async work) and its + call sites in `authService.ts` + `authProviderOptions.test.ts` de-awaited; + `authRoutes.ts` handlers now `return await ...` (return-await rule); + `authService.ts` api-key interaction uses `() => Promise.resolve(key)` / + `notify: () => undefined`; test `modify` arrows use `() => Promise.resolve(...)`. + +SDK behavior verified empirically before writing doubles (throwaway probe +scripts, since removed): `ModelRuntime.create({ credentials })` exposes 36 +providers / 1072 models; `login(id, "api_key", interaction)` persists to the +store; `getModel("anthropic", "claude-sonnet-4-5-20250929")` resolves; +`hasConfiguredAuth` flips correctly across `delete`/`modify` + `refresh`; +`readStoredCredential(id, authPath)` reads a temp auth.json and returns +`undefined` for a missing file. + +### Prior position (slice 4, leg 5, commit `4ccd4f8`) Slice 4 (`piSessionService.ts` migration) complete and committed (`4ccd4f8`). `piSessionService.ts` now uses the new `ModelRuntime` API end to end: - `createDefaultRuntimeFactory(modelRuntime, ...)` passes `modelRuntime` to @@ -109,56 +162,39 @@ Remaining errors otherwise live in slices 2/3/4 files and all test/support files (slice 5). ## Leg tracking -- **Last completed leg:** 5 (slice 4 — piSessionService.ts migration). -- **Next leg to run:** 6. +- **Last completed leg:** 6 (slice 5 — tests + testSupport migration). +- **Next leg to run:** 7. ## Next task -Run **charter slice 5 (tests + testSupport)** as leg 6: migrate all test -doubles off `AuthStorage.inMemory(...)` / `ModelRegistry.create|inMemory(...)` -to the pi-ai `InMemoryCredentialStore` + `await ModelRuntime.create({ -credentials })`, and get `npm run verify` green. Follow the testing-guide skill -(async construction seams, no over-mocking of the SDK). +Run **charter slice 6 (changeset + final verify + cleanup)** as leg 7. This is +the closing leg: +1. Add a `.changeset/*.md` fragment for `@jmfederico/pi-web` describing the + user-visible fix (session daemon crash with Pi 0.80.8+ fixed by migrating + to the new `ModelRuntime` API; requires Pi `>=0.80.8`). Use the + `changeset-changelog` skill. Do **not** edit `CHANGELOG.md` directly. The + maintainer's call on patch vs minor — the assessment §5 suggests patch or + minor; a **minor** is defensible since the supported Pi range narrows + (`>=0.80.8 <0.81`, dropping 0.80.0–0.80.7), but follow the changeset skill + and keep it a single fragment. +2. Re-run the full `npm run verify` to confirm still green. +3. Confirm the goal criteria in `charter.md` are all met (1 no AuthStorage/ + registry use — done; 2 auth surfaces on new APIs — done; 3 dep ranges — + already corrected in slice 0/1, double-check `package.json` peer/dev ranges; + 4 changeset — this leg; 5 verify green — confirm). +4. Cleanup: `ASSESSMENT-issue-62.md` stays (it's the plan of record). + `/srv/dev/pi-inspect` is outside the repo, not ours to touch. Confirm no + scratch files were left in the repo (e.g. no stray `probe*.mjs`, + `.tmp-build/`). +5. **Do NOT open a PR** (explicitly out of scope for this relay). -**Scope note (important):** slice 4 made `modelRuntime` a *required* -`PiSessionServiceDependencies` field (see Current position for why). That means -the slice-5 test surface is LARGER than the four files originally listed in the -assessment. Current `npx tsc --noEmit` failing files (all tests/support): -- `piSessionService.testSupport.ts` (4) — `fakeRuntime` builds - `modelRegistry: ModelRegistry.create(AuthStorage.inMemory())`; the - `TestSession` type still has `modelRegistry`. Give the fake a `modelRuntime` - (e.g. `await ModelRuntime.create({ credentials: new InMemoryCredentialStore() })` - — note this makes `fakeRuntime` async, which ripples into its callers) and - update `TestSession`. This is the central helper; fixing it first will clear - many downstream errors. -- `authService.test.ts` (10) — already partly slice-1/2/3 debt. -- `piSessionService.warnings.test.ts` (5) — `anthropicSubscriptionWarning` no - longer takes a registry; it now reads `readStoredCredential("anthropic", - authPath)`. Tests that build credentials via `authStorage.set(...)` must - instead write an `auth.json` (temp dir) and pass its path, OR the test seam - must be reconsidered. `SubscriptionSession` type ref to `modelRegistry` is - gone. Check whether `readStoredCredential` can be pointed at a temp authPath - cleanly; if not, consider whether the warning fn needs a small injectable - credential-read seam (raise via intervention if the API can't support the - test without contortion). -- `piSessionService.promptQueue.test.ts` (17), `.lifecycle.test.ts` (19), - `.archiveCleanup.test.ts` (9), `.spawnSession.test.ts` (3), - `.spawnSubsession.test.ts` (18), `sessionRoutes.test.ts` (1) — mostly the - new required `modelRuntime` dep on `new PiSessionService(...)` plus - `fakeRuntime`/`ModelRegistry.inMemory` usages. Many of these should clear - automatically once `testSupport.ts` provides a shared `modelRuntime` helper - and the `PiSessionService` test-construction path supplies it. +This is the finish line. After confirming everything, the goal is reached: +update `status.md`/`log.md`, commit, and per the charter **stop** (the goal is +reached) — or if you prefer, hand off a final "relay complete" confirmation +leg. Either way, surface to the human that the **sessiond restart is still +pending** (see Blockers) and a PR was intentionally not opened. -Suggested approach: add a small shared test helper (e.g. -`await createTestModelRuntime()` wrapping `ModelRuntime.create({ credentials: -new InMemoryCredentialStore(...) })`) in `testSupport.ts`, thread it into -`fakeRuntime` and the `new PiSessionService(...)` call sites, then work file by -file until `npm run verify` (typecheck + lint + knip + test) is green. - -Then slice 6 adds the `.changeset/*.md` fragment, runs the full `npm run -verify`, and does final cleanup (ASSESSMENT stays). - -If slice 5 is already done when you arrive, apply the charter's task-selection -policy: pick the lowest-numbered incomplete slice (6). +Verification already passing as of slice 5 (leg 6): `npm run verify` green +(typecheck + lint + knip + 1390 tests, 2 skipped). ### Build/tooling note (important for every leg) **Update (leg 2):** the human reports `/tmp` is now fully usable again, so the @@ -174,10 +210,11 @@ verify-green). Node: v24.18.0. ## Relevant context for the next runner - **Plan of record:** `ASSESSMENT-issue-62.md` (root) — read once. §5 has the per-file migration shape; §3 has the exact new API shapes; §6 the dep ranges. -- **Files to change** (all under `src/server/sessions/` unless noted): - `authService.ts`, `authProviderOptions.ts`, `oauthLoginFlowService.ts`, - `piSessionService.ts`, plus `src/server/sessiond.ts` (async auth - construction), and the test/support files listed in assessment §2. +- **Changeset skill:** `.agents/skills/changeset-changelog/SKILL.md` (and + `changeset-changelog` in the skills list). Follow it for the fragment format. +- **The migration is done** — slice 6 is docs/changeset + confirmation only. + No further source changes are expected; if you find yourself editing + `src/`, re-check whether that's really in scope. - **New API cheat-sheet:** `ModelRuntime.create({ authPath, modelsPath, credentials? }): Promise`; credential persistence via the pi-ai `CredentialStore.modify` path; `runtime.login(providerId, type,