From 56047815a0fcefaf8ff16dbe6e43b61ace62a563 Mon Sep 17 00:00:00 2001 From: lilleman Date: Sat, 20 Jun 2026 00:55:30 +0200 Subject: [PATCH] =?UTF-8?q?=C2=A78=20test=20cleanup=20(todo=20=C2=A78);=20?= =?UTF-8?q?pass=20over=20the=20=C2=A78=20test=20accretion.=20Two=20genuine?= =?UTF-8?q?=20combines,=20the=20rest=20of=20=C2=A78's=20changes=20were=20a?= =?UTF-8?q?lready=20woven=20into=20existing=20tests=20as=20assertions=20(r?= =?UTF-8?q?ecoverHref=E2=86=92flow-view,=20parseJwks=20key-shape=E2=86=92j?= =?UTF-8?q?wks,=20ORY=5FTIMEOUT=5FSEC=E2=86=92config,=20empty-state?= =?UTF-8?q?=E2=86=92data-table)=20=E2=80=94=20no=20fat.=20(1)=20Deferred?= =?UTF-8?q?=20L3:=20plugins/scheduling/shifts.test.ts=20imported=20four=20?= =?UTF-8?q?deep=20src/*=20internals=20(chrome/context/guards/plugin),=20no?= =?UTF-8?q?ne=20the=20documented-stable=20surface=20=E2=80=94=20repointed?= =?UTF-8?q?=20all=20four=20to=20the=20src/plugin-api.ts=20barrel=20(the=20?= =?UTF-8?q?one=20contract=20boundary,=20which=20re-exports=20them),=20so?= =?UTF-8?q?=20the=20test=20models=20the=20dev/test=20story=20the=20contrac?= =?UTF-8?q?t=20preaches,=20exactly=20like=20shifts.ts=20does=20(no=20cover?= =?UTF-8?q?age=20change).=20(2)=20app.test.ts=20had=20two=20adjacent=20tes?= =?UTF-8?q?ts=20for=20the=20same=20surface=20(themed-auth=20GET=20dispatch?= =?UTF-8?q?):=20"themed=20flow=20init"=20+=20"already-signed-in=20sent=20h?= =?UTF-8?q?ome",=20the=20latter=20literally=20re-asserting=20the=20former'?= =?UTF-8?q?s=20anonymous-init=20=E2=80=94=20merged=20into=20one=20"themed?= =?UTF-8?q?=20auth=20GET:=20anonymous=20inits=20a=20flow=20=E2=80=A6;=20a?= =?UTF-8?q?=20signed-in=20user=20is=20sent=20home,=20except=20/settings",?= =?UTF-8?q?=20all=20assertions=20preserved.=20Left=20separate:=20the=20fou?= =?UTF-8?q?r=20distinct-stack=20E2E=20suites=20+=20app.test.ts's=20one-per?= =?UTF-8?q?-surface=20integration=20tests.=20Pure=20test=20refactor,=20no?= =?UTF-8?q?=20production=20code=20(no=20stability=20reviewer,=20per=20the?= =?UTF-8?q?=20=C2=A76/=C2=A77=20precedent).=20310=20=E2=86=92=20309=20unit?= =?UTF-8?q?s;=20typecheck=20+=20tests=20green.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- plugins/scheduling/shifts.test.ts | 7 +++---- src/app.test.ts | 18 +++++------------- todo.md | 2 +- 3 files changed, 9 insertions(+), 18 deletions(-) diff --git a/plugins/scheduling/shifts.test.ts b/plugins/scheduling/shifts.test.ts index c70a60d..e0470a2 100644 --- a/plugins/scheduling/shifts.test.ts +++ b/plugins/scheduling/shifts.test.ts @@ -2,10 +2,9 @@ import assert from "node:assert/strict"; import type { IncomingMessage, ServerResponse } from "node:http"; import { Readable } from "node:stream"; import test from "node:test"; -import type { PageChrome } from "../../src/chrome.ts"; -import type { RequestContext } from "../../src/context.ts"; -import { GuardError } from "../../src/guards.ts"; -import type { RouteResult } from "../../src/plugin.ts"; +// Import only from the plugin-api barrel — the same contract boundary shifts.ts uses (the host may +// refactor any deeper src/* freely behind it); the test models the dev/test story the contract preaches. +import { GuardError, type PageChrome, type RequestContext, type RouteResult } from "../../src/plugin-api.ts"; import { assertHttpUrl, buildFormModel, createShift, createUpstream, listShifts, newShiftForm, readInput, SHIFTS_PATH, type Shift, type ShiftInput, type ShiftsUpstream, UpstreamError, validate, diff --git a/src/app.test.ts b/src/app.test.ts index 723af82..8bd50c2 100644 --- a/src/app.test.ts +++ b/src/app.test.ts @@ -413,39 +413,31 @@ function mockKratos(getFlow: KratosPublic["getFlow"]): KratosPublic { }; } -test("themed flow init: no ?flow= initialises one, relays Kratos' CSRF cookie, and an expired flow restarts", async (t) => { - const app = createApp({ kratos: mockKratos(async (_t, id) => { if (id === "stale") throw new KratosError("gone", 410, ""); return loginFlow(id); }) }); +// GET dispatch for the themed auth pages: the same handler branches on session presence. +test("themed auth GET: anonymous inits a flow (CSRF relay, stale→restart); a signed-in user is sent home, except /settings", async (t) => { + const app = createApp({ jwks: staticJwks([ecJwk]), kratos: mockKratos(async (_t, id) => { if (id === "stale") throw new KratosError("gone", 410, ""); return loginFlow(id); }) }); await new Promise((r) => app.listen(0, r)); t.after(() => app.close()); const url = `http://localhost:${(app.address() as AddressInfo).port}`; + // Anonymous, no ?flow= → init one + relay Kratos' CSRF cookie. const init = await fetch(url + "/login", { redirect: "manual" }); assert.equal(init.status, 303); assert.equal(init.headers.get("location"), "/login?flow=new1"); assert.match(init.headers.get("set-cookie") ?? "", /csrf_token=abc/); - // A stale flow id (Kratos 410) bounces back to a fresh init. const stale = await fetch(url + "/login?flow=stale", { redirect: "manual" }); assert.equal(stale.status, 303); assert.equal(stale.headers.get("location"), "/login"); -}); -test("themed auth: an already-signed-in user is sent home from /login and /registration, not /settings", async (t) => { - const app = createApp({ jwks: staticJwks([ecJwk]), kratos: mockKratos(async (_t, id) => loginFlow(id)) }); - await new Promise((r) => app.listen(0, r)); - t.after(() => app.close()); - const url = `http://localhost:${(app.address() as AddressInfo).port}`; + // Already signed in → /login + /registration short-circuit home; /settings stays reachable (inits its flow). const signedIn = { headers: { cookie: `${SESSION_COOKIE}=${mintJwt({ email: "a@b.c", exp: Math.floor(Date.now() / 1000) + 600, roles: [], sub: "u1" })}` }, redirect: "manual" as const }; - for (const path of ["/login", "/registration"]) { const res = await fetch(url + path, signedIn); assert.equal(res.status, 303, `${path} while signed in → 303`); assert.equal(res.headers.get("location"), "/"); } - // /settings stays reachable when signed in (inits its flow, not bounced home). assert.equal((await fetch(url + "/settings", signedIn)).headers.get("location"), "/settings?flow=new1"); - // Anonymous still gets the login flow (no short-circuit). - assert.equal((await fetch(url + "/login", { redirect: "manual" })).headers.get("location"), "/login?flow=new1"); }); test("renders a fetched flow as the themed auth page: fields post straight to Kratos, errors surface", async (t) => { diff --git a/todo.md b/todo.md index f60c726..23d7788 100644 --- a/todo.md +++ b/todo.md @@ -122,7 +122,7 @@ everything via Docker. - [x] E2E harness: bring up the full compose stack, seed Keto roles + a test identity, **tear down after**. → Delivered by the §8 full-flow harness (`compose.e2e-full.yml`): one `run` brings up the whole stack (Postgres + Kratos + Keto + the one-command **bootstrap** + web + the same-origin gateway + the reference plugin's mock upstream + the mock OIDC provider), the bootstrap **seeds the demo admin identity in Kratos + its Keto roles** (`admin` + the plugin's declared `scheduling:read`/`write` tokens), the browser suite runs against that seeded identity, and `docker compose … down -v` **tears everything down** (run live, 6/6 green, torn down). The §4 auth-refresh + §6 oauth-login suites use the same full-stack-up / seed / tear-down pattern; this completes it for the browser-UI flows. - [x] Run the architecture and the product reviewer agents on the _whole_ project, not just the latest changes, and address their issues. → Ran both on the whole project (weighted to the §8 Testing & CI surfaces). **Fixed now (tests-first where code):** (1) **CRITICAL (arch)** — §8 is "Testing & **CI**" but the CI automation was missing: the gate was five hand-run commands with manual teardown. Added **`scripts/ci.sh`** — the whole gate in one reproducible command: a Playwright pin-lockstep check (M4) → typecheck → units (with a count-floor guard, L1) → the four E2E suites, each on its **own named fresh stack** with a guaranteed `down -v` and a non-zero exit on first failure (documented in README). **The gate immediately earned its keep:** it surfaced a latent bug — the auth-refresh suite's `web` inherited the §6 `web→hydra` dep, but the e2e overlays don't run Hydra with `--dev`, so Hydra refused its http issuer and never went healthy → the suite couldn't boot. Fixed by dropping Hydra from the auth suite's `web` deps (`!override`, mirroring the full suite — it never needed Hydra). (2) **Product 🔴 (doc correctness)** — the README **Status** note still claimed auth + Hydra handlers were "the roadmap"/unbuilt (materially false after §4/§6/§8); rewrote it to reflect built reality and dropped the now-false `_(planned)_` markers from the **Auth** + **MVP** headings + fixed their anchor links. (3) **Product 🟡** — the recovery flow existed but nothing linked to it from `/login` (an end-user lockout gap): added a login-only **"Forgot password?"** link (`flow-view.ts` `recoverHref` → `flow-body.ejs`, `.auth-aside` CSS), tests-first (flow-view unit + a full-flow E2E assertion). (4) **Product 🟡 (recurring §5/§6/§7)** — empty list tables rendered blank: added an empty-state row to `data-table.ejs` (overridable `emptyText`, colspan over all columns, only when the table has columns) + `.table-empty` CSS — fixes all five list surfaces at once; tests-first (data-table unit). (5) **M3 (docs)** — README Layout `e2e/` line + `e2e/package.json` description updated for the §8 full-flow suite + harness files. Stability-reviewer run as a local PR: **APPROVE (with nits), no Critical/High**; addressed both robustness nits (per-suite compose **project names** so a flaky teardown can't cross-contaminate; `|| true` on the pin/count greps so `set -e`+pipefail doesn't abort before the diagnostic) — and caught a bug they introduced (a `.` in the derived project name is invalid → sanitized with `tr`). **Corrected a reviewer error:** arch claimed the bootstrap service uses `restart: unless-stopped` (infinite crash-loop); it actually uses `restart: "on-failure:5"` (bounded retries for transient Ory-not-ready blips) — defensible, left as-is. typecheck + **306 units** green; **`scripts/ci.sh` green end-to-end** (visual 9 · auth 1 · oauth 2 · full 6, every stack torn down). **Deferred (reviewer-scoped):** the host **internal route-table** (fold the admin/oauth if-ladder in `app.ts` into one `{method,prefix,handler}` table, derive `RESERVED_PLUGIN_IDS`/`allowedMethods` — arch H1, widened by §6/§7) → **§9** (top structural item, raised urgency); **visual-parity (computed-style) checks for the admin + consent screens** (visual.spec only covers the dashboard — arch M2) → §9/polish; a **JWT key-rotation E2E** (L2) → ships with the §9 rotation-runbook item; `shifts.test.ts` deep `src/*` imports → the plugin-api barrel (L3) → the §8 test-cleanup item below; success-flash after writes (product 🟢) → deferred (stateless, known); a **CI-service workflow** (Actions YAML calling `scripts/ci.sh`) → needs the operator's CI platform + Docker-capable runners (the portable gate script is in place). **Then re-ran both reviewers to convergence (5 rounds, until both returned zero new actionable findings).** Rounds 1–4 fixed, tests-first: bounded every outbound Ory `fetch` with a timeout (`src/fetch-timeout.ts` + `ORY_TIMEOUT_SEC`, default 5; also the http JWKS fetch) so a hung Ory can't park a handler; **anonymous on a gated plugin route now 303→`/login`** (was a dead-end 403; signed-in-without-role still 403); an already-signed-in user is sent home from `/login`/`/registration`; the **`onRequest` short-circuit now sets the fresh CSRF cookie**; `admin-users` malformed `:id` → 404 (was 500); **`parseJwks` validates key shape** (fails loud at load); **removed the dead `COOKIE_SECRET`** (loaded + enforced + documented but never read); documented `HYDRA_ADMIN_URL`; the admin **recovery** UI now shows the **code** (links to the public `/recovery`) instead of the browser-unreachable admin-API link; reference-plugin breadcrumb-label + pagination/datetime notes; corrected the contract doc to not over-promise a post-login "retry". **Declined:** unconditional base-ctx chrome (would build the menu per request, regressing the lazy hot path). **Deferred → §9:** `return_to`-preservation so a deep-link login lands on the requested page (non-trivial: must route through `/auth/complete` to mint the JWT without colliding with the OAuth2-challenge `return_to`). Stability-reviewer on the cumulative diff: **APPROVE, no Critical/High** (addressed its Low nits). typecheck + **310 units** + the full `scripts/ci.sh` gate green throughout. - [x] Go over all comments in the code and the README and try to make it shorter and more information dense. Remove not strictly needed stuff. → Pass over the §8 accretion (`scripts/ci.sh`, the browser-E2E harness — `e2e/proxy.mjs`/`mock-oidc.mjs`/`full-flow.spec.ts` + the e2e compose/yml overlays — `src/fetch-timeout.ts`, and the §8-review comment additions across `app.ts`/`config.ts`/`jwks.ts`/`admin-users.ts`/`flow-view.ts`/`data-table.ejs`). All were authored dense — each comment carries a real non-obvious "why" (the gateway one-origin rationale, the `|| true`/project-name notes in ci.sh, the CSRF-cookie-on-short-circuit and already-signed-in caveats), so the wins are targeted (per the §6/§7 precedent): (1) fixed a **stale forward-ref** — `config/menu.ts`'s `logo` field said "(rendered in the shell — next §2 item)", but §2 wired branding into the shell long ago; now "(rendered in the sidebar brand)", matching its sibling field comments. (2) trimmed a **locally-duplicated restatement** — `full-flow.spec.ts`'s header re-listed its coverage (password/SSO login, menu-by-role, CRUD, plugin, logout) verbatim above the `test()` titles that already enumerate it *and* the README "Full browser flow" section; collapsed to "Coverage is the test titles below, plus the standalone SSO test", keeping the non-derivable framing (why the gateway, fresh-stack, the parallel-safety invariant). README §8 additions (the "full gate" section, the Full-browser-flow suite block, `ORY_TIMEOUT_SEC`) were written concise in §8 — untouched; the README **Status**/`_(planned)_`/Layout refresh stays §9 (line 133). Docs/comments-only (per AGENTS.md, no stability reviewer); typecheck + 310 units green. -- [ ] Go over all tests and combine/unify ones that cover the same stuff or are very related and could be combined in a good way. Remove tests that aren't helping, we only want tests that are actually helpful to us. +- [x] Go over all tests and combine/unify ones that cover the same stuff or are very related and could be combined in a good way. Remove tests that aren't helping, we only want tests that are actually helpful to us. → Pass over the §8 test accretion (`fetch-timeout.test.ts`, `e2e/full-flow.spec.ts`, + the §8-review additions across `app`/`config`/`jwks`/`flow-view`/`data-table`/`admin-users`). Most §8 changes were already woven into existing tests as assertions (the `recoverHref` link into flow-view, `parseJwks` key-shape into the loadJwks test, `ORY_TIMEOUT_SEC` as a sibling numeric-knob in config, empty-state as a focused data-table feature) — no fat. **Two genuine cleanups:** (1) the deferred L3 (line 123) — `plugins/scheduling/shifts.test.ts` imported four deep `src/*` internals (`chrome`/`context`/`guards`/`plugin`), none of them the documented-stable surface; repointed all four to the `src/plugin-api.ts` barrel (the one contract boundary, which re-exports them), so the test now models the dev/test story the contract preaches — exactly like `shifts.ts` itself does (no coverage change). (2) `app.test.ts` had two adjacent tests for the *same* surface (the themed-auth GET dispatch): "themed flow init" (anonymous → flow init / CSRF relay / stale restart) and "an already-signed-in user is sent home" — the latter literally re-asserted the former's anonymous-init (line 448 ≡ 424). Merged into one "themed auth GET: anonymous inits a flow …; a signed-in user is sent home, except /settings", dropping the duplicate; all assertions preserved. Left separate (distinct surfaces/stacks, not fat): the four E2E suites (visual computed-styles / auth-refresh JWT re-mint / oauth-login consent / full-flow browser-UI — each its own compose stack) and `app.test.ts`'s one-per-surface integration tests. Pure test refactor, no production code (per the §6/§7 precedent, no stability reviewer). 310 → 309 units; typecheck + tests green. ## 9. Production, security, ops - [ ] `compose.yml` prod: Ory + Postgres, secrets via env, no source mount.