diff --git a/.convoys/fix-vercel-deployment-protection-in-ci.md b/.convoys/fix-vercel-deployment-protection-in-ci.md index 7baecb0..6fd06b4 100644 --- a/.convoys/fix-vercel-deployment-protection-in-ci.md +++ b/.convoys/fix-vercel-deployment-protection-in-ci.md @@ -13,8 +13,9 @@ skip: - role-ux-reviewer # no UX surface - role-ia-architect # no IA surface - browser-smoke # the convoy IS the smoke pipeline -status: in-progress +status: shipped created: 2026-05-24 +shipped: 2026-05-24 parent: ship-readiness addresses: P0 #7 (CI infrastructure side-effect) depends_on: @@ -545,3 +546,129 @@ serially. Architecture complete. 1 brief created. Estimated PRs: 1. Awaiting human gate 1 (Decisions A + B + D ratification + brief approval) before the implementer runs. + +## As-shipped + +Shipped 2026-05-24 as squash commit `9a3e077` (PR #17). The convoy +shipped in three commits, not one — Brief 1 plus two scope expansions +discovered during PR #17's own CI validation. Capturing the deviation +from the architect's original 1-brief decomposition here so the next +architect / reviewer has the audit trail. + +### Three-commit reality + +1. **`365e9f0` Brief 1 — bypass plumbing per spec.** Both workflows got + the identical shape change architect planned: `wait-for-vercel-preview@v1.3.2`'s + `path:` input now carries `/?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}&x-vercel-set-bypass-cookie=true` + (Decision A's original cookie-variant shape — later corrected in + commit 3); `max_timeout: 600 → 120` (Decision B); the `gate:` job's + Decide step short-circuits on `github.event.pull_request.head.repo.fork == true` + with a `::notice::` annotation, before the existing PR-body skip + directive runs (Decision D); the Playwright/screenshot step exports + `VERCEL_AUTOMATION_BYPASS_SECRET` as `env:` for forward-compat with + `adopt-playwright-smoke`. ~25 LOC in `preview-smoke.yml`, ~15 LOC in + `visual-diff.yml`. +2. **`b6f8688` shell-injection hardening (scope expansion #1).** + Pre-existing latent bug surfaced by PR #17's own CI validation. + Decision D's gate step inlined `${{ github.event.pull_request.body }}` + directly into bash, which broke when the PR body contained shell + metacharacters like `(` or backticks — PR #17's description bit this + with `"unexpected token \`('"` because of phrasing like *"(was: + 10-minute timeout)"*. Every prior Decide-step run was one badly- + formatted PR body away from breaking the gate. Fix is the standard + GitHub Actions hardening pattern: route the body and the fork flag + through the step's `env:` block as `PR_BODY` and `PR_IS_FORK`, then + quote them as `"$PR_BODY"` / `"$PR_IS_FORK"` in the shell condition. + Same change in both workflows (~9 LOC each). This fix is technically + beyond Brief 1's planned scope (which targeted only Vercel-bypass + plumbing) but was bundled into the convoy because the bug actively + blocked Brief 1's success criterion from being validated on PR #17. +3. **`043a6ee` drop `&x-vercel-set-bypass-cookie=true` (scope expansion #2 — + Decision-A shape correction).** Brief 1 used the cookie-variant + shape per the original Decision A wording. PR #17's CI run showed + the wait-action's healthcheck was still 401ing despite the bypass + query being correct. Root cause: with `x-vercel-set-bypass-cookie=true`, + Vercel responds **307 + Set-Cookie** (setting `_vercel_jwt`), but + axios in Node has no cookie jar — it follows the redirect to the + bare URL without the cookie, which then 401s. Operator's local curl + confirmed empirically: + + ``` + curl -sI "https:///?x-vercel-protection-bypass=" | head -1 + → HTTP/2 200 (works, no cookie needed) + + curl -sI "https:///?x-vercel-protection-bypass=&x-vercel-set-bypass-cookie=true" | head -1 + → HTTP/2 307 (the redirect-without-cookie path that breaks axios) + ``` + + For a one-shot healthcheck the per-request bypass query is enough. + The cookie variant stays reserved for the future Playwright config + in `adopt-playwright-smoke` where a real browser cookie jar exists. + An inline comment in `preview-smoke.yml` now explains this so the + next agent doesn't accidentally re-add the cookie param. + +### Decision-A deviation from the original spec + +**The convoy did NOT ship the cookie variant.** Decision A as written +in this file specified `path: '/?x-vercel-protection-bypass=...&x-vercel-set-bypass-cookie=true'` +(quote: "Append `?x-vercel-protection-bypass=...&x-vercel-set-bypass-cookie=true` +to the wait-action's `path:` input"). Commit `043a6ee` corrected this +to the bare `path: '/?x-vercel-protection-bypass=...'` after empirical +evidence (per `curl -sI` above) showed Vercel's cookie-bypass path is +incompatible with axios's no-cookie-jar behavior in the wait-action. +The spec evolved during validation; the convoy file's Decision A text +above is preserved as the original recommendation, but the next +architect should know the as-shipped shape is the cookie-less form. +The cookie variant remains the right call for Playwright's +`extraHTTPHeaders` / cookie-jar-aware future use case (Decision A +already flagged this division). + +### As-shipped metrics (from PR #17's CI run, post-validation) + +- `Wait for Vercel Preview deployment` step elapsed: **194 milliseconds** + (was: 10-minute timeout on every PR before this convoy — a ~3,000× + improvement). Acceptance criterion #3 (workflows complete in < 5 + minutes) crushed by ~50× margin on the wait-step alone. +- `Playwright smoke` workflow total runtime: **59 seconds** (was: 10+ + minutes). Comfortably inside the < 5-minute acceptance threshold. +- Step breakdown: + - `Wait for Vercel Preview deployment` → success in 194ms ✅ + - `npm ci`, `setup-node`, `playwright install` → success ✅ + - `Run smoke tests` → failure (expected — see next section) ❌ +- `Screenshot diff` workflow: **not triggered on PR #17 itself** because + its path filter excludes workflow-only changes. Will fire on the + next PR touching `pages/**` / `components/**` / `styles/**` / + Tailwind / PostCSS config. + +### Remaining expected red + +`Playwright smoke` workflow now reaches `npx playwright test` and +fast-fails because **`playwright.config.js` does not exist** in the +tree yet. That is `adopt-playwright-smoke`'s scope (P1 #10 step 2 / +launch sequence step 10 in `.convoys/ship-readiness.md`), not this +convoy's. Brief 1's acceptance criterion #1 explicitly accepts a real +downstream failure as success, as long as the wait-action reaches +`Received success status code` first — which it does, in 194ms. The +failure mode shifted from "401 timeout in the wait step" (this +convoy's target) to "playwright not installed" (the next convoy's +target). That is the correct end state. + +### Operator-rotation caveat + +R6 in the Risk list. The Vercel bypass token does not auto-expire. If +the operator rotates it from the Vercel dashboard, the workflow will +silently start failing with the same 401 + timeout it did before this +convoy. Re-seed via `gh secret set VERCEL_AUTOMATION_BYPASS_SECRET --body ""`. +No automation; this is a human responsibility (same pattern as +`JWT_SECRET` rotation, documented in `AGENTS.md` § 7). + +### What did NOT change + +- Workflow `permissions:` blocks (PR #16's contracts preserved). +- `concurrency:` groups (Decision C confirmed unchanged). +- Any application code (`pages/**`, `components/**`, `lib/**`, + `scripts/**`). +- Any test file (no new unit tests; vitest does not exercise GitHub + Actions YAML). +- `playwright.config.js` (still does not exist; owned by + `adopt-playwright-smoke`). diff --git a/.convoys/ship-readiness.md b/.convoys/ship-readiness.md index 7224228..f841869 100644 --- a/.convoys/ship-readiness.md +++ b/.convoys/ship-readiness.md @@ -299,7 +299,24 @@ Follow-ups surfaced mid-convoy or mid-PR that didn't fit the original launch seq - **`single-auth-provider`** (priority: P1 quality, also listed as launch sequence step 9). Collapse `lib/auth-context.js` + `lib/admin-auth.js` into `lib/use-auth.js`. Surfaced again as a follow-up in P0 #7 § Flagged-but-deferred (4 pages still import the legacy `useAuth`). - **`cleanup-mobile-nav-dead-props`** (priority: P3 polish). `components/MobileNavigation.js` accepts a dead `user` prop; remove it. Surfaced in P0 #7 § Flagged-but-deferred. May fold into `god-component-split` (P2 #13) if that lands first. - **`bump-eslint-10`** (priority: P2 hygiene; upstream-blocked). Bump ESLint from v9 to v10 once `typescript-eslint` ships a v10-tested release and `eslint-config-next` bundles it. Surfaced in `.convoys/bump-next-js.md` § Decisions D. -- **`fix-vercel-deployment-protection-in-ci`** (priority: P2 CI infra; **operator action required**). PR #16's permissions fix exposed that Vercel Deployment Protection 401s anonymous CI requests, so `Playwright smoke` and `Screenshot diff` now 10-min-timeout instead of 5-second-403. Plumb `VERCEL_AUTOMATION_BYPASS_SECRET` into both workflows + (eventually) Playwright config. Operator must seed the secret into GitHub Actions before the implementer can run. See `.convoys/fix-vercel-deployment-protection-in-ci.md`. Created 2026-05-24. +- **`fix-vercel-deployment-protection-in-ci`** (priority: P2 CI infra) — **RESOLVED 2026-05-24**. + - **Resolved by:** squash commit `9a3e077` (PR #17), comprising three commits, not one. Operator prereq seeded 2026-05-24T20:03:31Z (`gh secret set VERCEL_AUTOMATION_BYPASS_SECRET`; confirmed via `gh secret list`); the implementer dispatch waited on that visibility per the convoy file's "Operator action required" gate. + - **Three-commit reality** (Brief 1 + two scope expansions found during CI validation): + 1. `365e9f0` Brief 1 — bypass plumbing per spec. Both `.github/workflows/preview-smoke.yml` and `.github/workflows/visual-diff.yml` got the same shape change: `wait-for-vercel-preview@v1.3.2`'s `path:` input now carries `/?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}&x-vercel-set-bypass-cookie=true` (Decision A's original cookie-variant shape — later corrected in commit 3); `max_timeout: 600 → 120` (Decision B); the `gate:` job's Decide step short-circuits on `github.event.pull_request.head.repo.fork == true` with a `::notice::` annotation, before the existing PR-body skip directive runs (Decision D); and the Playwright/screenshot step exports `VERCEL_AUTOMATION_BYPASS_SECRET` as `env:` for forward-compat with `adopt-playwright-smoke`. + 2. `b6f8688` shell-injection hardening (latent pre-existing bug surfaced by PR #17's own CI validation). Decision D's gate step inlined `${{ github.event.pull_request.body }}` directly into bash, which broke when the PR body contained shell metacharacters like `(` or backticks. PR #17's own description bit this with `"unexpected token \`('"` because of phrasing like *"(was: 10-minute timeout)"*. Fix is the standard GitHub Actions hardening pattern (their official "Security hardening" guide flags inline `${{ }}` in shell as both a syntax-error risk and a shell-injection vector): route the body and the fork flag through the step's `env:` block as `PR_BODY` and `PR_IS_FORK`, then quote them as `"$PR_BODY"` / `"$PR_IS_FORK"` in the shell condition. Same change in both workflows; ~9 LOC each. Documented in commit message as technically beyond Brief 1's scope but bundled into the convoy because the bug actively blocked Brief 1's success criterion from being validated. + 3. `043a6ee` drop `&x-vercel-set-bypass-cookie=true` from the wait-action `path:` — corrects Decision A's exact shape. With the cookie variant, Vercel responds **307 + Set-Cookie**, and axios in Node has no cookie jar — it follows the redirect to the bare URL without the cookie, which then 401s. Empirically confirmed by operator's local curl: bare `?x-vercel-protection-bypass=X` → HTTP/2 200, while `?x-vercel-protection-bypass=X&x-vercel-set-bypass-cookie=true` → HTTP/2 307 (the broken path). For a one-shot healthcheck the per-request bypass query is enough. The cookie variant stays reserved for the future Playwright config (`adopt-playwright-smoke`) where a real browser cookie jar exists. An inline comment in `preview-smoke.yml` explains this so the next agent doesn't accidentally re-add the cookie param. + - **As-shipped metrics** (from PR #17's CI run, post-validation): + - `Wait for Vercel Preview deployment` step elapsed: **194 milliseconds** (was: 10-minute timeout before this convoy). + - `Playwright smoke` workflow total runtime: **59 seconds** (was: 10+ minutes). + - Step breakdown: `Wait for Vercel Preview deployment` → success in 194ms; `npm ci`, `setup-node`, `playwright install` → success; `Run smoke tests` → **failure** (expected — see next bullet). + - `Screenshot diff` workflow: not triggered on PR #17 itself because its path filter excludes workflow-only changes; will fire on the next PR touching `pages/**` / `components/**` / `styles/**` / Tailwind/PostCSS config. + - **Documented expected red**: `Playwright smoke` now reaches `npx playwright test` and fast-fails because `playwright.config.js` doesn't exist in the tree yet. That is `adopt-playwright-smoke`'s scope (P1 #10 step 2 / launch sequence step 10), not this convoy's. Per the convoy file's Test plan § and Brief 1 acceptance criterion #1, a real downstream failure with the wait-action reaching `Received success status code` first counts as success for this convoy — the failure mode shifted from "401 timeout in the wait step" to "playwright not installed", which is precisely the target state. + - **Operator-rotation caveat** (R6 in the convoy file). The Vercel bypass token does not auto-expire. If/when it's rotated from the Vercel dashboard, the operator must re-seed the GitHub secret via `gh secret set VERCEL_AUTOMATION_BYPASS_SECRET --body ""`. Same human-responsibility pattern as `JWT_SECRET` rotation; not preventable from workflow YAML. No automation here. + - **Flagged-but-deferred** (from the convoy file's "Anything flagged but not acted on" section, unchanged at merge): + 1. `replace-wait-for-vercel-preview` — the wait-action's last release was Mar 2024; could be replaced with a few lines of `gh api` + `curl`-loop. Out of scope for this convoy; queue if the action ages out further or gets a security advisory. + 2. `adopt-playwright-smoke` — owns the actual `playwright.config.js`, `tests/smoke/`, and `@playwright/test` dep. The bypass plumbing here is forward-compat for that convoy (env var available on the smoke step). Listed in P1 #10 step 2 / launch sequence step 10 above. + 3. `Screenshot diff` baseline authoring — orthogonal scope; the visual-diff workflow has nothing to compare against on its first real run. + - **Owns:** `role-architect` (3 Decisions ratified — A query-param, B 120s timeout, D fork-PR skip) → `role-implementer` (Brief 1) + two scope-expansion commits. ## Self-analytics diff --git a/AGENTS.md b/AGENTS.md index a0a4a47..7090114 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,7 +77,8 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560 ## 7. Deployment - **Vercel** auto-deploys `main` and creates Preview deployments for every PR. `vercel.json` and `.vercel/` are committed. CI in `.github/workflows/` runs lint + types (no duplicate build — Vercel handles it). -- **Preview protection bypass for automation.** The project has a Protection Bypass for Automation token exposed locally as `VERCEL_AUTOMATION_BYPASS_SECRET` in `.env.local` (not committed). Smoke/visual-diff workflows pass this header (`x-vercel-protection-bypass`) when hitting password-protected preview URLs. Needed for the queued `adopt-playwright-smoke` convoy; do not log or echo the value. +- **Preview protection bypass for automation.** The project has a Protection Bypass for Automation token exposed locally as `VERCEL_AUTOMATION_BYPASS_SECRET` in `.env.local` (not committed) and seeded into GitHub Actions as a repo secret (`gh secret set VERCEL_AUTOMATION_BYPASS_SECRET`, 2026-05-24). The `preview-smoke` and `visual-diff` workflows pass the bypass as a **query parameter** on `wait-for-vercel-preview@v1.3.2`'s `path:` input — `path: '/?x-vercel-protection-bypass=…'`, bare form, **without** `&x-vercel-set-bypass-cookie=true` (the cookie variant returns 307 + Set-Cookie and axios in Node has no cookie jar, so it 401s on the redirect). The header form (`x-vercel-protection-bypass: `) is reserved for the future `playwright.config.js` that `adopt-playwright-smoke` will ship (Playwright has its own request shape and `extraHTTPHeaders` is cleaner there). Do not log or echo the value. If the operator rotates the token in the Vercel dashboard, re-seed the GitHub secret via `gh secret set VERCEL_AUTOMATION_BYPASS_SECRET --body ""`. See `.convoys/fix-vercel-deployment-protection-in-ci.md`. +- **Shell-injection hardening in workflow YAML.** Never inline `${{ github.event.* }}` directly into a `run:` block — route the value through the step's `env:` block and quote it (`"$VAR_NAME"`) in shell. PR #17's CI validation caught a real syntax error from a PR body containing `(` because the gate-job's Decide step inlined `${{ github.event.pull_request.body }}` straight into bash; commit `b6f8688` swept both `preview-smoke.yml` and `visual-diff.yml` to the `env:` + quoted-shell pattern. This is GitHub's official Security Hardening guidance ("Security hardening for GitHub Actions" → "Using a third-party action"). Apply to any new workflow that reads PR body / title / branch name / commit messages in shell. ## 8. Code graph