From 54adbd2b3d78c6b85e346e48b988e6ca1901ec98 Mon Sep 17 00:00:00 2001 From: fred Date: Thu, 27 Aug 2026 09:52:28 -0500 Subject: [PATCH] review fixes: seeded-auth hard gate, ci.yml build serialization, assets 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - M1: E2E_REQUIRE_SEEDED_AUTH=1 in the CI e2e step makes login failures hard failures (loginAs throws, guards disabled, globalSetup refuses a pre-populated DB); auth.spec redirect test asserts outright under the flag; non-admin /admin test is now a real authorization assertion - M2: ci.yml build step depends_on test — never two concurrent turbo builds on the shared workspace - M3: unknown /assets/* paths 404 from the SPA catch-all instead of serving index.html with an immutable cache header; spec arm added - minors: e2e step gets when: *image_build_when, health poll uses GATEWAY_PORT + AbortSignal.timeout, BETTER_AUTH_SECRET generated per run (no literal in tree), failure echoes artifact path, dev-guide documents the gate, stale verify-release comment fixed --- .woodpecker/ci.yml | 7 ++++- .woodpecker/publish.yml | 23 +++++++++++--- apps/gateway/src/spa/serve-spa.e2e.spec.ts | 16 +++++++++- apps/gateway/src/spa/serve-spa.ts | 13 ++++++++ apps/web/e2e/admin.spec.ts | 36 ++++++++++------------ apps/web/e2e/auth.spec.ts | 14 +++------ apps/web/e2e/chat.spec.ts | 7 +++-- apps/web/e2e/global-setup.ts | 14 +++++++-- apps/web/e2e/helpers/auth.ts | 22 ++++++++++--- apps/web/e2e/navigation.spec.ts | 12 ++++++-- apps/web/e2e/projects.spec.ts | 7 +++-- apps/web/e2e/settings.spec.ts | 7 +++-- docs/guides/dev-guide.md | 14 +++++++++ scripts/verify-release.mjs | 4 +-- 14 files changed, 142 insertions(+), 54 deletions(-) diff --git a/.woodpecker/ci.yml b/.woodpecker/ci.yml index a50029b8..4eeafacf 100644 --- a/.woodpecker/ci.yml +++ b/.woodpecker/ci.yml @@ -264,7 +264,12 @@ steps: - *enable_pnpm - pnpm build depends_on: - - typecheck + # after test, not typecheck: turbo gives `test` a ^build dependency, so + # running this step concurrently with test would put two independent + # turbo builds on the same shared-workspace dist/ and turbo cache with + # no cross-process locking — the same serialization invariant + # publish.yml documents for #1411. + - test services: ci-postgres: diff --git a/.woodpecker/publish.yml b/.woodpecker/publish.yml index 1a330302..b656c5db 100644 --- a/.woodpecker/publish.yml +++ b/.woodpecker/publish.yml @@ -416,7 +416,8 @@ steps: # # Image pinned to the @playwright/test version in pnpm-lock.yaml so the # image's bundled browsers match the workspace driver exactly (bump the two - # together). The step installs nothing: it reuses the workspace node_modules + # together). The step installs no workspace packages (corepack does fetch + # the pinned pnpm itself): it reuses the workspace node_modules # from `install` and the dist outputs from `build` — the gateway's runtime # dependency path is pure JS/WASM (PGlite is WASM, postgres-js is pure JS), # so the alpine-installed modules run unchanged under this glibc image. @@ -426,14 +427,20 @@ steps: e2e: image: mcr.microsoft.com/playwright:v1.58.2-noble environment: - # Test-only value for this step's throwaway embedded database; the - # gateway refuses to boot without one. Not a credential. - BETTER_AUTH_SECRET: ci-e2e-throwaway-value GATEWAY_PORT: '14242' PLAYWRIGHT_BASE_URL: http://localhost:14242 + # The database is seeded by Playwright's globalSetup in this step, so + # login failures are real failures: without this flag the suite's + # skip-when-login-fails guards (a live-environment affordance) could + # skip every authenticated spec and go green while proving nothing. + E2E_REQUIRE_SEEDED_AUTH: '1' commands: - corepack enable - | + # Throwaway signing secret for this step's ephemeral embedded database + # (the gateway refuses to boot without one). Generated per run so no + # usable literal lives in the tree. + export BETTER_AUTH_SECRET="$(head -c 32 /dev/urandom | base64)" export WEB_DIST_DIR="$(pwd)/apps/web/dist" if [ ! -f "$WEB_DIST_DIR/index.html" ]; then echo "[e2e] FATAL: $WEB_DIST_DIR/index.html missing — did the build step run?" >&2 @@ -449,7 +456,7 @@ steps: GATEWAY_PID=$! ready=0 for i in $(seq 1 90); do - if node -e "fetch('http://localhost:14242/health').then((r) => process.exit(r.ok ? 0 : 1), () => process.exit(1))"; then + if node -e "fetch('http://localhost:' + process.env.GATEWAY_PORT + '/health', { signal: AbortSignal.timeout(2000) }).then((r) => process.exit(r.ok ? 0 : 1), () => process.exit(1))"; then ready=1 break fi @@ -475,8 +482,14 @@ steps: if [ "$E2E_EXIT" -ne 0 ]; then echo "[e2e] FATAL: Playwright suite failed (exit $E2E_EXIT); gateway log follows" >&2 tail -100 /tmp/gateway.log >&2 + echo "[e2e] browser-side traces/screenshots are under apps/web/test-results/ in the step workspace (not persisted past the pod)" >&2 fi exit "$E2E_EXIT" + # Same filter as the image builds it gates: a merge that publishes no + # image (docs-only on main) pays no browser suite, and a skipped e2e does + # not block anything (skipped-dependency semantics, same as + # publish-next-npm on tag events). + when: *image_build_when depends_on: - build - verify diff --git a/apps/gateway/src/spa/serve-spa.e2e.spec.ts b/apps/gateway/src/spa/serve-spa.e2e.spec.ts index a8b044c9..486182da 100644 --- a/apps/gateway/src/spa/serve-spa.e2e.spec.ts +++ b/apps/gateway/src/spa/serve-spa.e2e.spec.ts @@ -11,7 +11,8 @@ * 3. Unknown backend paths (/api, /mcp, /socket.io) are JSON 404s, never the * SPA page — including with a query string (`/api?x=1`). * 4. Static files are served exactly; hashed /assets/ files get immutable - * cache headers, everything else revalidates (max-age=0). + * cache headers, everything else revalidates (max-age=0), and a missing + * /assets/ file is a 404 — never the SPA fallback. * 5. WEB_DIST_DIR unset disables SPA serving entirely. * 6. WEB_DIST_DIR pointing at a directory without index.html fails at boot. */ @@ -130,6 +131,19 @@ describe('SPA static serving — fixture dist dir', () => { expect(res.headers['cache-control']).toBe('public, max-age=31536000, immutable'); }); + it('missing /assets/ files are 404s, never the SPA page with an immutable header', async () => { + // The exact request a browser with a stale index.html makes after a + // deploy: the old hashed filename. Serving index.html here would poison + // caches with a year-long immutable entry whose body is HTML. + for (const missingAsset of ['/assets/app-old999.js', '/assets/app-old999.js?v=1']) { + const res = await request(app.getHttpServer()).get(missingAsset); + expect(res.status, missingAsset).toBe(404); + expect(res.text, missingAsset).not.toContain('mosaic spa fixture'); + // The 404 carries no cache-control at all; ?? '' keeps the assertion valid. + expect(res.headers['cache-control'] ?? '', missingAsset).not.toContain('immutable'); + } + }); + it('index.html and non-asset files revalidate (no immutable caching)', async () => { for (const revalidating of ['/', '/chat', '/favicon.svg']) { const res = await request(app.getHttpServer()).get(revalidating); diff --git a/apps/gateway/src/spa/serve-spa.ts b/apps/gateway/src/spa/serve-spa.ts index ba91b356..dc3f2cfd 100644 --- a/apps/gateway/src/spa/serve-spa.ts +++ b/apps/gateway/src/spa/serve-spa.ts @@ -75,6 +75,7 @@ export async function mountSpaStatic(app: NestFastifyApplication): Promise // this catch-all; non-GET unmatched requests keep Fastify's stock 404. fastify.get('/*', (req, reply) => { const url = req.raw.url ?? ''; + const pathOnly = url.split('?', 1)[0] ?? url; if (isBackendPath(url)) { // An unknown backend path is an API 404, never the SPA page. void reply.code(404).send({ @@ -84,6 +85,18 @@ export async function mountSpaStatic(app: NestFastifyApplication): Promise }); return; } + if (pathOnly === '/assets' || pathOnly.startsWith('/assets/')) { + // A missing hashed asset — typically a browser holding a stale + // index.html after a deploy — must 404. Falling through to the SPA + // fallback would return index.html as the asset body, and the onSend + // hook above would stamp it with a year-long immutable cache-control. + void reply.code(404).send({ + message: `Asset ${pathOnly} not found`, + error: 'Not Found', + statusCode: 404, + }); + return; + } // sendFile is decorated by @fastify/static; its type augmentation targets // a different fastify copy in the pnpm tree than the Nest adapter's. (reply as unknown as { sendFile: (file: string) => unknown }).sendFile('index.html'); diff --git a/apps/web/e2e/admin.spec.ts b/apps/web/e2e/admin.spec.ts index bac519f0..b0dbafe8 100644 --- a/apps/web/e2e/admin.spec.ts +++ b/apps/web/e2e/admin.spec.ts @@ -1,11 +1,14 @@ import { test, expect } from '@playwright/test'; -import { loginAs, ADMIN_USER, TEST_USER } from './helpers/auth.js'; +import { loginAs, ADMIN_USER, REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; test.describe('Admin page — admin user', () => { test.beforeEach(async ({ page }) => { await loginAs(page, ADMIN_USER.email, ADMIN_USER.password); const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded admin user — skipping admin tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded admin user — skipping admin tests', + ); }); test('admin page loads with the Admin Panel heading', async ({ page }) => { @@ -43,26 +46,19 @@ test.describe('Admin page — non-admin user', () => { test.beforeEach(async ({ page }) => { await loginAs(page, TEST_USER.email, TEST_USER.password); const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded test user — skipping non-admin tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded test user — skipping non-admin tests', + ); }); - test('non-admin visiting /admin sees access denied or is redirected', async ({ page }) => { + test('non-admin visiting /admin never sees the admin panel', async ({ page }) => { await page.goto('/admin'); - // Either redirected away or shown an access-denied message - const onAdmin = page.url().includes('/admin'); - if (onAdmin) { - // Should show some access-denied content rather than the full admin panel - const hasPanel = await page - .getByRole('heading', { name: /admin panel/i }) - .isVisible() - .catch(() => false); - // If heading is visible, the guard allowed access (user may have admin role in this env) - // — not a failure, just informational - if (!hasPanel) { - // access denied message, redirect, or guard placeholder - const url = page.url(); - expect(url).toBeTruthy(); // environment-dependent — no hard assertion - } - } + // Wait for the app shell to render (redirect and access-denied views both + // keep the sidebar), then assert the panel itself is absent. globalSetup + // seeds TEST_USER with role 'member', so this is a real authorization + // assertion, not environment-dependent. + await expect(page.getByRole('img', { name: /mosaic logo/i })).toBeVisible({ timeout: 10_000 }); + await expect(page.getByRole('heading', { name: /admin panel/i })).not.toBeVisible(); }); }); diff --git a/apps/web/e2e/auth.spec.ts b/apps/web/e2e/auth.spec.ts index 93915a38..3fd19e5b 100644 --- a/apps/web/e2e/auth.spec.ts +++ b/apps/web/e2e/auth.spec.ts @@ -1,5 +1,5 @@ import { test, expect } from '@playwright/test'; -import { TEST_USER } from './helpers/auth.js'; +import { REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; // ── Login page ──────────────────────────────────────────────────────────────── @@ -49,18 +49,14 @@ test.describe('Login page', () => { }); test('redirects to /chat after successful login', async ({ page }) => { + // Only meaningful with known-good credentials; against a live environment + // this would just probe someone else's user table. + test.skip(!REQUIRE_SEEDED_AUTH, 'needs seeded credentials (E2E_REQUIRE_SEEDED_AUTH=1)'); await page.goto('/login'); await page.getByLabel('Email').fill(TEST_USER.email); await page.getByLabel('Password').fill(TEST_USER.password); await page.getByRole('button', { name: /sign in/i }).click(); - // Either reaches /chat or shows an error (if credentials are wrong in this env). - // We assert a navigation away from /login, or the alert is shown. - await Promise.race([ - expect(page).toHaveURL(/\/chat/, { timeout: 10_000 }), - expect(page.getByRole('alert')).toBeVisible({ timeout: 10_000 }), - ]).catch(() => { - // Acceptable — environment may not have seeded credentials - }); + await expect(page).toHaveURL(/\/chat/, { timeout: 10_000 }); }); }); diff --git a/apps/web/e2e/chat.spec.ts b/apps/web/e2e/chat.spec.ts index 51d106d5..6b6f99ab 100644 --- a/apps/web/e2e/chat.spec.ts +++ b/apps/web/e2e/chat.spec.ts @@ -1,12 +1,15 @@ import { test, expect } from '@playwright/test'; -import { loginAs, TEST_USER } from './helpers/auth.js'; +import { loginAs, REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; test.describe('Chat page', () => { test.beforeEach(async ({ page }) => { await loginAs(page, TEST_USER.email, TEST_USER.password); // If login failed (no seeded user in env) we may be on /login — skip const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded test user — skipping authenticated tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded test user — skipping authenticated tests', + ); }); test('chat page loads and shows the conversation area', async ({ page }) => { diff --git a/apps/web/e2e/global-setup.ts b/apps/web/e2e/global-setup.ts index 0e0c85e1..02a4ee12 100644 --- a/apps/web/e2e/global-setup.ts +++ b/apps/web/e2e/global-setup.ts @@ -1,5 +1,5 @@ import type { FullConfig } from '@playwright/test'; -import { ADMIN_USER, TEST_USER } from './helpers/auth.js'; +import { ADMIN_USER, REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; /** * Seed the E2E users through the gateway's real APIs (#1445, P6). @@ -10,7 +10,9 @@ import { ADMIN_USER, TEST_USER } from './helpers/auth.js'; * * Against an environment that already has users (needsSetup=false), seeding is * skipped entirely: the specs keep their own skip-when-login-fails guards, so - * a live environment stays usable as a test target without mutation. + * a live environment stays usable as a test target without mutation. Under + * E2E_REQUIRE_SEEDED_AUTH=1 (CI) that state is instead a hard failure and the + * guards are disabled — see helpers/auth.ts. * * On a fresh database, any seeding failure throws and fails the whole run: an * E2E gate whose authenticated suites silently skip would pass while proving @@ -25,6 +27,14 @@ export default async function globalSetup(config: FullConfig): Promise { } const status = (await statusRes.json()) as { needsSetup: boolean }; if (!status.needsSetup) { + if (REQUIRE_SEEDED_AUTH) { + // CI boots the gateway on a fresh HOME-isolated database, so an + // already-populated one means the isolation regressed — refuse to run + // against unknown data rather than skip-and-pass. + throw new Error( + 'E2E_REQUIRE_SEEDED_AUTH=1 but the database already has users — gateway HOME isolation regressed?', + ); + } console.info('[e2e setup] users already exist; skipping seed'); return; } diff --git a/apps/web/e2e/helpers/auth.ts b/apps/web/e2e/helpers/auth.ts index 12163d40..98113b08 100644 --- a/apps/web/e2e/helpers/auth.ts +++ b/apps/web/e2e/helpers/auth.ts @@ -12,17 +12,29 @@ export const ADMIN_USER = { name: 'E2E Admin User', }; +/** + * Set when the database was seeded by global-setup (CI sets it in the + * publish.yml e2e step). Seeded credentials MUST work, so login failures are + * hard failures and the skip-when-login-fails guards are disabled — otherwise + * a login regression would skip every authenticated suite and the gate would + * pass while proving nothing. Unset (a live environment used as a test + * target), the guards stay on and unseeded credentials skip their suites. + */ +export const REQUIRE_SEEDED_AUTH = process.env['E2E_REQUIRE_SEEDED_AUTH'] === '1'; + /** * Fill the login form and submit, then wait for the post-login redirect to - * /chat. On failed login the wait times out and is swallowed: the page stays - * on /login, and the callers' `test.skip(!url.includes('/chat'))` guards see - * that. Without this wait, every guard read page.url() before the redirect - * happened and skipped its suite even when login succeeded (#1445). + * /chat. Under REQUIRE_SEEDED_AUTH a missed redirect throws (failing the + * test). Otherwise the timeout is swallowed: the page stays on /login and the + * callers' `test.skip(...)` guards see that. Without this wait, every guard + * read page.url() before the redirect happened and skipped its suite even + * when login succeeded (#1445). */ export async function loginAs(page: Page, email: string, password: string): Promise { await page.goto('/login'); await page.getByLabel('Email').fill(email); await page.getByLabel('Password').fill(password); await page.getByRole('button', { name: /sign in/i }).click(); - await page.waitForURL(/\/chat/, { timeout: 10_000 }).catch(() => {}); + const redirect = page.waitForURL(/\/chat/, { timeout: 10_000 }); + await (REQUIRE_SEEDED_AUTH ? redirect : redirect.catch(() => {})); } diff --git a/apps/web/e2e/navigation.spec.ts b/apps/web/e2e/navigation.spec.ts index a15819d4..60122f3a 100644 --- a/apps/web/e2e/navigation.spec.ts +++ b/apps/web/e2e/navigation.spec.ts @@ -1,11 +1,14 @@ import { test, expect } from '@playwright/test'; -import { loginAs, TEST_USER } from './helpers/auth.js'; +import { loginAs, REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; test.describe('Sidebar navigation', () => { test.beforeEach(async ({ page }) => { await loginAs(page, TEST_USER.email, TEST_USER.password); const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded test user — skipping authenticated tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded test user — skipping authenticated tests', + ); }); test('sidebar shows the Mosaic brand', async ({ page }) => { @@ -64,7 +67,10 @@ test.describe('Route transitions', () => { test.beforeEach(async ({ page }) => { await loginAs(page, TEST_USER.email, TEST_USER.password); const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded test user — skipping authenticated tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded test user — skipping authenticated tests', + ); }); test('navigating chat → projects → settings → chat works without errors', async ({ page }) => { diff --git a/apps/web/e2e/projects.spec.ts b/apps/web/e2e/projects.spec.ts index 3181e383..b7deacac 100644 --- a/apps/web/e2e/projects.spec.ts +++ b/apps/web/e2e/projects.spec.ts @@ -1,11 +1,14 @@ import { test, expect } from '@playwright/test'; -import { loginAs, TEST_USER } from './helpers/auth.js'; +import { loginAs, REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; test.describe('Projects page', () => { test.beforeEach(async ({ page }) => { await loginAs(page, TEST_USER.email, TEST_USER.password); const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded test user — skipping authenticated tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded test user — skipping authenticated tests', + ); }); test('projects page loads with heading', async ({ page }) => { diff --git a/apps/web/e2e/settings.spec.ts b/apps/web/e2e/settings.spec.ts index 143b435e..d07a84c3 100644 --- a/apps/web/e2e/settings.spec.ts +++ b/apps/web/e2e/settings.spec.ts @@ -1,11 +1,14 @@ import { test, expect } from '@playwright/test'; -import { loginAs, TEST_USER } from './helpers/auth.js'; +import { loginAs, REQUIRE_SEEDED_AUTH, TEST_USER } from './helpers/auth.js'; test.describe('Settings page', () => { test.beforeEach(async ({ page }) => { await loginAs(page, TEST_USER.email, TEST_USER.password); const url = page.url(); - test.skip(!url.includes('/chat'), 'No seeded test user — skipping authenticated tests'); + test.skip( + !REQUIRE_SEEDED_AUTH && !url.includes('/chat'), + 'No seeded test user — skipping authenticated tests', + ); }); test('settings page loads with heading', async ({ page }) => { diff --git a/docs/guides/dev-guide.md b/docs/guides/dev-guide.md index 09830639..4615e28d 100644 --- a/docs/guides/dev-guide.md +++ b/docs/guides/dev-guide.md @@ -212,6 +212,20 @@ Woodpecker `.woodpecker/publish.yml` keeps stable and integration-line artifacts `next` never publishes npm `latest` or Docker `latest`. The next npm publish step verifies that `@mosaicstack/mosaic@next` resolves to the computed prerelease before the pipeline can pass. +### E2E Gate (#1445, P6) + +Trunk publish pipelines run a headless Playwright suite (`e2e` step) before any image publishes: the built gateway `dist` boots on a throwaway embedded PGlite database (isolated via a fresh `HOME`), serves the built SPA bundle through `WEB_DIST_DIR` — the same serving path the gateway image ships — and the suite runs against it inside the pinned `mcr.microsoft.com/playwright` image. `E2E_REQUIRE_SEEDED_AUTH=1` makes login failures hard failures (the skip-when-login-fails guards are a live-environment affordance only). Both image build steps depend on this gate. + +Reproduce locally (Ubuntu-based environments; Fedora's headless-shell rendering is broken): + +```bash +pnpm build +BETTER_AUTH_SECRET="$(head -c 32 /dev/urandom | base64)" GATEWAY_PORT=14242 \ + WEB_DIST_DIR="$PWD/apps/web/dist" HOME="$(mktemp -d)" node apps/gateway/dist/main.js & +E2E_REQUIRE_SEEDED_AUTH=1 PLAYWRIGHT_BASE_URL=http://localhost:14242 \ + pnpm --filter @mosaicstack/web exec playwright test +``` + --- ## Adding New Agent Tools diff --git a/scripts/verify-release.mjs b/scripts/verify-release.mjs index b8b34e4b..aca6957b 100644 --- a/scripts/verify-release.mjs +++ b/scripts/verify-release.mjs @@ -106,8 +106,8 @@ export const STAGES = [ { // RI-N4 (QC-19, card RI-3-002): the typed quality-rails evaluator, invoked // as the implementation of the check it owns instead of a duplicated - // presence loop here. Canonical-only stage (no ci.yml mirror — same shape - // as `build`); runs AFTER build so the evaluator's dist/ exists. Subject + // presence loop here. Canonical-only stage (no ci.yml mirror; `build` + // gained one in #1445); runs AFTER build so the evaluator's dist/ exists. Subject // is this repository (`.` → monorepo subject kind, per-subject check set). name: 'quality-rails', commands: ['node packages/quality-rails/dist/cli.js quality-rails evaluate --project .'],