Reflects the merged fix-vercel-deployment-protection-in-ci convoy (PR #17, squash commit9a3e077) in repo documentation. Closes the CI-infra side-effect of P0 #7. Three commits in the convoy: Brief 1 (bypass plumbing per spec), plus two scope expansions discovered during CI validation (shell-injection hardening, and a Decision-A shape correction to drop the cookie-bypass param). .convoys/ship-readiness.md: - Queued convoys: mark fix-vercel-deployment-protection-in-ci as RESOLVED 2026-05-24 with9a3e077. Document the 3-commit reality (365e9f0Brief 1 bypass plumbing,b6f8688shell-injection hardening of the gate Decide step,043a6eedropping &x-vercel-set-bypass-cookie=true), the empirical metrics (wait-action: 10-min timeout -> 194ms; workflow runtime: 10+ min -> 59s), the documented expected red on Playwright smoke (npx playwright test fails because playwright.config.js doesn't exist yet -- adopt-playwright-smoke owns that), and the operator-rotation caveat (R6). AGENTS.md: - Section 7 Deployment: correct the noun "header" -> "query param on wait-action's path:" since that's what actually landed per Decision A. Also document the without-cookie form (the cookie variant 401s through axios's missing cookie jar) and the operator re-seed runbook for token rotation. - Section 7 Deployment: fold in a one-liner about the GitHub Actions ${{ }}-in-shell-is-injection-vector pattern, with the env: + quoted-shell fix shape. Picked Section 7 over a new Gotcha #13 because the existing Gotchas list is dominated by app-level pitfalls (auth, SQL clients, ESLint), and CI YAML hardening is naturally co-located with deployment. .convoys/fix-vercel-deployment-protection-in-ci.md: - frontmatter status: in-progress -> shipped (added shipped: 2026-05-24) - new ## As-shipped section: 3-commit reality, Decision-A shape deviation (we shipped without &x-vercel-set-bypass-cookie=true), empirical timings (194ms wait, 59s total), remaining-red attribution to adopt-playwright-smoke, and the operator-rotation caveat. No changes to: package.json, lib/**, pages/**, components/**, scripts/**, .github/workflows/**, .cursor/rules/**, README.md. Co-authored-by: Cursor <cursoragent@cursor.com>
674 lines
35 KiB
Markdown
674 lines
35 KiB
Markdown
---
|
||
name: fix-vercel-deployment-protection-in-ci
|
||
classification: convoy
|
||
success_metric: |
|
||
`Playwright smoke` and `Screenshot diff` workflows reach their actual
|
||
smoke / capture step on every PR (no more 401-from-Vercel-SSO 10-min
|
||
timeouts). Both workflows complete in < 5 minutes. Failures, when they
|
||
occur, are real assertion failures — not auth failures against the
|
||
preview URL.
|
||
skip:
|
||
- role-design-system-auditor # CI infra only
|
||
- role-a11y-auditor # no a11y surface
|
||
- role-ux-reviewer # no UX surface
|
||
- role-ia-architect # no IA surface
|
||
- browser-smoke # the convoy IS the smoke pipeline
|
||
status: shipped
|
||
created: 2026-05-24
|
||
shipped: 2026-05-24
|
||
parent: ship-readiness
|
||
addresses: P0 #7 (CI infrastructure side-effect)
|
||
depends_on:
|
||
- fix-layout-default-user (shipped — PR #15, ca302a8)
|
||
- PR #16 fix(ci) scoped permissions (shipped — 7e97254)
|
||
---
|
||
|
||
# Fix Vercel Deployment Protection in CI
|
||
|
||
Plumb `VERCEL_AUTOMATION_BYPASS_SECRET` into the `Playwright smoke` and
|
||
`Screenshot diff` workflows so anonymous GitHub Actions runners can
|
||
actually GET the preview URL without hitting Vercel's SSO 401 challenge.
|
||
Without this, both workflows permanently red on every PR — just slower
|
||
red than before PR #16.
|
||
|
||
## Why now
|
||
|
||
PR #16 (`fix(ci): scoped permissions`, squash commit `7e97254`) added
|
||
minimal scoped `permissions:` blocks to `.github/workflows/preview-smoke.yml`
|
||
and `.github/workflows/visual-diff.yml`. That fixed the 5-second 403
|
||
"Resource not accessible by integration" failure both workflows hit when
|
||
trying to call the GitHub deployments API. **However**, with permissions
|
||
correct, both workflows now reach the actual deployment check and fail
|
||
with a different error: a 10-minute timeout from
|
||
`patrickedqvist/wait-for-vercel-preview@v1.3.2`'s subsequent HTTP GET
|
||
against the preview URL, which Vercel returns 401 for because Deployment
|
||
Protection is on (anonymous GitHub-runner request → Vercel SSO challenge).
|
||
Cost: ~10 minutes of runner time per workflow per PR — and zero signal,
|
||
since neither workflow ever reaches its smoke step. This blocks PR #15's
|
||
recurring follow-up convoys (visual-regression baselines, Playwright smoke
|
||
for `adopt-playwright-smoke`) from getting any CI feedback.
|
||
|
||
The bypass token already exists locally as `VERCEL_AUTOMATION_BYPASS_SECRET`
|
||
in `.env.local` (Protection Bypass for Automation, configured in the
|
||
Vercel project). Documented in `AGENTS.md` § 7 — Deployment. The work here
|
||
is plumbing it from operator-supplied repo secret → workflow env →
|
||
`wait-for-vercel-preview`'s `path:` input + the eventual Playwright
|
||
`BASE_URL` so anonymous runner requests bypass the SSO challenge.
|
||
|
||
## Operator action required (BEFORE this convoy can run)
|
||
|
||
This convoy CANNOT proceed without the operator first seeding the secret
|
||
into GitHub Actions. The implementer has nothing to wire up if the
|
||
secret isn't visible to the workflows.
|
||
|
||
1. **Seed the secret:**
|
||
```bash
|
||
gh secret set VERCEL_AUTOMATION_BYPASS_SECRET --body "<value from local .env.local>"
|
||
```
|
||
(The value is whatever `VERCEL_AUTOMATION_BYPASS_SECRET=…` says in
|
||
`.env.local`. Do not paste it anywhere logged. Do not echo it from a
|
||
workflow step.)
|
||
2. **Confirm visibility:**
|
||
```bash
|
||
gh secret list
|
||
```
|
||
Expect to see `VERCEL_AUTOMATION_BYPASS_SECRET` listed alongside the
|
||
existing repo secrets. Note: `gh secret list` shows names only — never
|
||
values — by design.
|
||
3. **Notify the next agent** that steps 1 + 2 are done. The convoy file's
|
||
frontmatter `status:` should flip from `queued` to `in-progress` only
|
||
after this notification.
|
||
|
||
This is the same pattern `npm run setup-db`'s `ADMIN_INITIAL_PASSWORD`
|
||
established (`drop-public-setup` Brief 1, commit `ff80753`): CI / scripts
|
||
that need a secret get an actionable fail-loud error when the secret is
|
||
missing, and the operator seeds it once per environment.
|
||
|
||
## Decisions to ratify with operator
|
||
|
||
Queued; do not pre-decide.
|
||
|
||
1. **Bypass via query param vs. request header.**
|
||
- **Option A — query param.** Append `?x-vercel-protection-bypass=...&x-vercel-set-bypass-cookie=true`
|
||
to the wait-action's `path:` input AND to the Playwright `BASE_URL`.
|
||
The first request sets a `_vercel_jwt` cookie on the runner's
|
||
ephemeral browser context; subsequent same-origin requests reuse it.
|
||
Pro: works with any HTTP client, no custom config in Playwright.
|
||
Con: the bypass token shows up in workflow run logs if any step
|
||
echoes the URL (mitigation: never `echo` or `cat` a URL containing
|
||
the token; log `${{ steps.wait.outputs.url }}` only after stripping
|
||
the query string).
|
||
- **Option B — request header (`x-vercel-protection-bypass: <secret>`).**
|
||
Cleaner — the token never appears in any URL. But requires custom
|
||
HTTP-client config in `playwright.config.js` (`extraHTTPHeaders`)
|
||
AND in `wait-for-vercel-preview` (the action's docs need confirming —
|
||
header support may not be exposed via inputs).
|
||
2. **CI assertion that bypass actually works.** Should we add a step
|
||
that explicitly asserts `200` on the preview URL during the wait-
|
||
action's healthcheck phase, before handing off to Playwright /
|
||
screenshot capture? This would surface bypass-misconfiguration as a
|
||
fast-fail step instead of letting Playwright time out 8 minutes
|
||
later on a different error. Cost: ~5 lines of YAML; benefit: clearer
|
||
failure signal for the next operator-touch event.
|
||
3. **Workflow concurrency cancellation.** The workflows already use
|
||
`concurrency:` keyed on `github.ref`. Confirm that the bypass-token
|
||
wiring doesn't inadvertently break the cancel-stale behavior (e.g.
|
||
if the `secrets.VERCEL_AUTOMATION_BYPASS_SECRET` reference is in a
|
||
`concurrency:` expression, that's a syntax error and the implementer
|
||
should pull it into a job-level `env:` instead).
|
||
|
||
## Scope
|
||
|
||
**In scope:**
|
||
|
||
- `.github/workflows/preview-smoke.yml` — wire the bypass into the
|
||
`wait-for-vercel-preview` step's `path:` input (Option A) OR add the
|
||
bypass header via the action's input shape (Option B, pending
|
||
confirmation that the action exposes header inputs).
|
||
- `.github/workflows/visual-diff.yml` — same treatment as preview-smoke
|
||
(the two workflows have similar shapes; whatever pattern works for one
|
||
should land in both).
|
||
- `playwright.config.js` (when it exists — the `adopt-playwright-smoke`
|
||
convoy ships it) — add `use: { extraHTTPHeaders: { 'x-vercel-protection-bypass': process.env.VERCEL_AUTOMATION_BYPASS_SECRET } }`
|
||
if Decision #1 picks Option B; OR build the BASE_URL with the query
|
||
param (Option A).
|
||
- Any test-setup file or helper that constructs the preview URL for
|
||
`screenshot-diff`-style workflows.
|
||
|
||
**Out of scope:**
|
||
|
||
- Writing new Playwright tests. Test authoring lives in
|
||
`adopt-playwright-smoke`. This convoy only makes the existing smoke
|
||
pipeline reachable.
|
||
- Broadening workflow `permissions:` blocks. PR #16 already landed the
|
||
minimal scope; this convoy should not need to touch them again.
|
||
- Replacing `patrickedqvist/wait-for-vercel-preview` with a different
|
||
action. The action retrieves the URL successfully (confirmed in PR #16's
|
||
run logs); the failure is the subsequent HTTP GET, which is a
|
||
configuration issue, not an action choice. A wholesale action swap is
|
||
a deeper rewrite — separate convoy if/when it's needed.
|
||
- Authoring new visual-regression baselines. The screenshot diff workflow
|
||
has nothing meaningful to compare against today; baseline authoring is
|
||
its own convoy.
|
||
- Disabling Vercel Deployment Protection on the project. Operator may
|
||
prefer to keep protected previews (cheap defense-in-depth against
|
||
preview-URL leakage); this fix lets CI work _around_ the protection
|
||
without weakening it.
|
||
|
||
## Known constraints
|
||
|
||
- **`wait-for-vercel-preview@v1.3.2` `path:` input is supported.** PR
|
||
#16's run logs confirm the action retrieves the URL successfully — the
|
||
subsequent HTTP GET is what fails. The action's `path:` input accepts a
|
||
full path including query string, so Option A (`?x-vercel-protection-bypass=...`)
|
||
is mechanically straightforward. Whether the action exposes a way to
|
||
inject custom request headers (Option B) needs to be confirmed by
|
||
reading the action's source / README before the implementer commits to
|
||
it.
|
||
- **The same secret will need to be plumbed into Playwright's `BASE_URL`
|
||
or into a request header in `playwright.config.js`** when the
|
||
`adopt-playwright-smoke` convoy ships. Coordinating shape now (this
|
||
convoy) vs. shape later (when Playwright lands) saves churn — the
|
||
implementer should pick whichever option keeps both call sites
|
||
consistent.
|
||
- **`npm run setup-db`'s `ADMIN_INITIAL_PASSWORD` is a parallel
|
||
precedent** for "CI needs a secret the operator must seed." Same
|
||
pattern applies: secret is repo-scoped, fail-loud (or fail-noisy) when
|
||
unset, never echoed to logs. See `drop-public-setup` Brief 1
|
||
(commit `ff80753`).
|
||
- **Token rotation.** The Vercel bypass token can be rotated from the
|
||
Vercel dashboard. If/when that happens, the operator must re-seed the
|
||
GitHub secret (`gh secret set ...`). No automation here — this is a
|
||
human responsibility per the same pattern as `JWT_SECRET` rotation.
|
||
|
||
## Acceptance criteria
|
||
|
||
The convoy is shippable when ALL of the following hold:
|
||
|
||
1. `Playwright smoke` workflow reaches its actual smoke step on a fresh
|
||
PR. It either passes (smoke green) OR fails on a real assertion
|
||
(Playwright reports a test failure or a runtime error from the
|
||
smoke spec). It does NOT fail with a 10-min timeout from the
|
||
`wait-for-vercel-preview` step or with a 401 from the preview URL.
|
||
2. `Screenshot diff` workflow reaches its screenshot capture step and
|
||
posts the "Visual Diff" comment to the PR (even if the diff itself
|
||
is empty / first-run / null-baseline). Same constraint: no 10-min
|
||
timeout, no 401.
|
||
3. Both workflows complete in < 5 minutes on a typical PR (the
|
||
pre-PR-16 baseline was ~30 seconds for the workflow body; adding a
|
||
bypass query string or header shouldn't materially affect runtime).
|
||
4. The bypass token does not appear in any workflow run log. Verify by
|
||
downloading the raw log of a passing run and grepping for the token's
|
||
first 8 chars.
|
||
5. Workflow YAML still passes basic actionlint review (`actionlint .github/workflows/*.yml`
|
||
exits 0). PR #16's permissions blocks remain unchanged.
|
||
6. `AGENTS.md` § 7 deployment paragraph (the "Preview protection bypass
|
||
for automation" line) still reflects reality after the change. May
|
||
need a one-sentence update if the implementer picks Option B
|
||
(`x-vercel-protection-bypass` header) vs. Option A (query string).
|
||
|
||
## Anything flagged but not acted on (in advance)
|
||
|
||
These are real findings that the architect / implementer should NOT try
|
||
to solve in this convoy. Each is queued separately if it warrants a fix.
|
||
|
||
- **The `wait-for-vercel-preview` action is no longer maintained** (last
|
||
release Mar 2024; no v2). Could be replaced with a few lines of
|
||
`gh api` + `curl`-loop in the workflow itself. Not in scope here —
|
||
this convoy needs to fix the immediate auth failure, not rewrite the
|
||
wait logic. Queue as `replace-wait-for-vercel-preview` if the action
|
||
ages out further or has a security advisory.
|
||
- **Playwright config doesn't exist yet.** `playwright.config.js`,
|
||
`tests/smoke/`, and `@playwright/test` all land in
|
||
`adopt-playwright-smoke` (P1 #10 step 2 / launch sequence step 10).
|
||
Until that convoy ships, the only `Playwright smoke` workflow body is
|
||
a no-op. This convoy can pre-wire the bypass infrastructure (env var,
|
||
workflow secrets) so `adopt-playwright-smoke` only needs to add the
|
||
test files and the Playwright config — but it can't ship a real
|
||
smoke-pass without that follow-up.
|
||
- **`Screenshot diff` baseline authoring.** Even after this convoy lands,
|
||
the visual-diff workflow has nothing to compare against on its first
|
||
run. That's expected and orthogonal — baseline authoring is a separate
|
||
scope.
|
||
- **Operator-rotation hygiene for `VERCEL_AUTOMATION_BYPASS_SECRET`.**
|
||
Vercel's bypass tokens don't auto-expire. If the team wants a periodic
|
||
rotation policy, that's an ops-runbook concern outside this convoy.
|
||
- **`AGENTS.md` § 7 wording.** The current "Smoke/visual-diff workflows
|
||
pass this header (`x-vercel-protection-bypass`)" line in § 7 is
|
||
aspirational — it describes intent, not what was actually wired. After
|
||
this convoy ships, that line becomes accurate. The doc-writer pass at
|
||
convoy close should reword to past-tense reality. **Also: § 7 says
|
||
"header"; the architect recommendation in Decision A below is the query
|
||
param (the wait-action has no input for custom headers). The doc-writer
|
||
pass MUST correct the noun.**
|
||
|
||
## Decisions (post-IA round)
|
||
|
||
Each decision below routes back to the operator for ratification at
|
||
human gate 1 (per the architect contract). Recommendations are based on
|
||
fresh-checkout evidence the architect gathered before drafting the brief.
|
||
|
||
### A — 2026-05-24: Use query-param-on-`path:` for the wait-action; reserve `extraHTTPHeaders` for the Playwright config that lands in `adopt-playwright-smoke`
|
||
|
||
> Resolves convoy file § "Decisions to ratify with operator" #1 (query
|
||
> param vs. header).
|
||
|
||
**Context.** The convoy file framed this as a clean either/or between
|
||
Option A (query param on `path:`) and Option B (request header via the
|
||
action's input shape). Boot-the-brief revealed the choice is forced for
|
||
the wait step but free for Playwright:
|
||
|
||
- `patrickedqvist/wait-for-vercel-preview@v1.3.2`'s `action.yml`
|
||
exposes inputs `token`, `max_timeout`, `environment`,
|
||
`allow_inactive`, `check_interval`, `vercel_password`, and `path` —
|
||
and **nothing else**. There is no input for custom request headers.
|
||
Option B is mechanically impossible for the wait step without
|
||
forking the action.
|
||
- `action.js:42` consumes `path` via `new URL(path, url)`. Anything
|
||
parseable as a URL path is fine; query strings work verbatim. So
|
||
`path: '/?x-vercel-protection-bypass=…&x-vercel-set-bypass-cookie=true'`
|
||
becomes the URL `https://<deployment>/?x-vercel-protection-bypass=…`
|
||
that axios then GETs.
|
||
- Crucially, the action only echoes `targetUrl` (the bare deployment
|
||
URL — `status.target_url`) in its logs (`action.js:357`, `:363`) and
|
||
sets it as `outputs.url` at `:360`. The `path:` query string is
|
||
**never appended to anything that is logged or set as an output.**
|
||
So passing the secret via `path:` does NOT leak it to workflow logs
|
||
or to downstream steps that consume `${{ steps.vercel.outputs.url }}`.
|
||
- Vercel's docs explicitly support both shapes; the "header is
|
||
recommended" guidance is about URL-in-log leak risk in callers, not
|
||
Vercel's acceptance. For the wait-action the leak risk is structurally
|
||
absent (see above).
|
||
- The future `playwright.config.js` (owned by `adopt-playwright-smoke`)
|
||
CAN and SHOULD use `extraHTTPHeaders` per Vercel's own snippet — the
|
||
config controls its own request shape and the header is cleaner.
|
||
|
||
**Recommendation (needs operator ratification).** Option A for the
|
||
wait-action. Pass the secret to Playwright through `env:` (this convoy
|
||
plumbs the env var; the actual Playwright config is `adopt-playwright-smoke`'s
|
||
job).
|
||
|
||
**If operator prefers Option B uniformly** (i.e. headers everywhere),
|
||
the cost is forking `wait-for-vercel-preview` or replacing it with a
|
||
hand-rolled `gh api` + `curl` poll. That's a larger rewrite and was
|
||
flagged as out-of-scope in the convoy file (§ "Anything flagged but not
|
||
acted on" → `replace-wait-for-vercel-preview`). Recommend keeping it
|
||
out of scope for now.
|
||
|
||
**Routing.** Operator ratifies at gate 1. Default to A unless rejected.
|
||
|
||
### B — 2026-05-24: No extra healthcheck assertion step; tighten `max_timeout` from 600 → 120 instead
|
||
|
||
> Resolves convoy file § "Decisions to ratify with operator" #2 (CI
|
||
> assertion that bypass actually works).
|
||
|
||
**Context.** The convoy file asked whether to add an explicit step that
|
||
asserts `200` on the preview URL before handing off to Playwright /
|
||
screenshot capture.
|
||
|
||
- The wait-action's healthcheck loop (`action.js:25-66`) already does
|
||
exactly this: `axios.get` against `new URL(path, url)`, retry on
|
||
non-2xx, exit on first 2xx, fail the step on timeout. If the bypass
|
||
is misconfigured, the action will time out at `max_timeout` and
|
||
call `core.setFailed('Timeout reached: Unable to connect to <url>')`.
|
||
An extra `curl` step would duplicate this signal.
|
||
- The real ergonomics problem is `max_timeout: 600` (10 minutes). At
|
||
2-second polling intervals (the action's default — confirmed in PR
|
||
#16's run logs: "Attempt N of 300"), a misconfigured bypass burns
|
||
10 minutes of runner time before failing. Vercel preview builds
|
||
typically complete in 30-90s; the deployment is normally already up
|
||
by the time GitHub triggers the workflow.
|
||
|
||
**Recommendation (architect-self-ratifiable; flagging for awareness).**
|
||
No additional assertion step. Lower `max_timeout` from `600` to `120`
|
||
in both workflows. This makes a misconfigured bypass fail in ~2 minutes
|
||
instead of ~10, well inside the convoy's "< 5 minutes" success metric,
|
||
and gives the deployment plenty of headroom for slow builds.
|
||
|
||
**Routing.** Architect ratifies. Operator may override at gate 1 if
|
||
preview builds in this project are known to exceed 120s — observed
|
||
behavior in PR #16's logs (deployment URL retrieved within 1 second of
|
||
job start) suggests the deployment is up well before the wait step
|
||
starts, so 120s is comfortable.
|
||
|
||
### C — 2026-05-24: Confirmed — `concurrency:` block contains no secret reference and stays unchanged
|
||
|
||
> Resolves convoy file § "Decisions to ratify with operator" #3
|
||
> (workflow concurrency cancellation).
|
||
|
||
**Context.** The convoy file flagged the risk that a secret-reference
|
||
inside a `concurrency:` group expression would be a YAML syntax error.
|
||
|
||
- Current `concurrency:` groups: `preview-smoke-${{ github.event.pull_request.number }}`
|
||
and `visual-diff-${{ github.event.pull_request.number }}`. No secret
|
||
reference today.
|
||
- The implementer's plumb-the-secret work lands in: (a) the wait-action
|
||
step's `with: path: ...` input, and (b) the Playwright smoke step's
|
||
`env: VERCEL_AUTOMATION_BYPASS_SECRET: ...` for forward-compat with
|
||
`adopt-playwright-smoke`. Neither location intersects `concurrency:`.
|
||
- Brief acceptance criterion #3 explicitly forbids placing the secret in
|
||
the `concurrency:` group expression.
|
||
|
||
**Recommendation (architect-self-ratifiable).** No change to the
|
||
`concurrency:` blocks; the cancel-stale behavior is preserved as-is.
|
||
|
||
**Routing.** Architect ratifies. No operator action needed.
|
||
|
||
### D — 2026-05-24: Skip Playwright smoke + Screenshot diff on fork PRs (extend `gate:` job) — NEW decision surfaced by Boot-the-brief
|
||
|
||
> Not in the original convoy file's "Decisions to ratify" list. Surfaced
|
||
> by the architect's Boot-the-brief check ("Empty / unset secret" case).
|
||
|
||
**Context.** GitHub Actions silently omits repo secrets on
|
||
`pull_request`-event runs that originate from a fork. The wait-action
|
||
would receive `${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}` as an
|
||
empty string, build the URL `https://<deployment>/?x-vercel-protection-bypass=&x-vercel-set-bypass-cookie=true`,
|
||
get 401 (empty bypass value is not a valid bypass), and time out at
|
||
`max_timeout`. After Decision B (120s timeout), that's still ~2 minutes
|
||
of wasted runner time per fork PR per workflow — net 4 minutes per fork
|
||
PR. The failure signal is "the convoy's fix didn't work" instead of "the
|
||
PR is from a fork and can't access secrets" — a misleading red.
|
||
|
||
tcg-vault is single-maintainer with occasional collaborators (all with
|
||
write access, so their PRs aren't from forks today). Fork PRs are rare.
|
||
But the cost of a one-line gate-job extension is zero, and the value
|
||
is "fork PRs get a clear skip message instead of a 2-minute wait + red."
|
||
|
||
**Recommendation (needs operator ratification).** Extend the existing
|
||
`gate:` step in both workflows to check `github.event.pull_request.head.repo.fork`
|
||
first, BEFORE the existing `pipeline:.*skip.*\bsmoke\b` / `\bvisual\b`
|
||
body-directive check. When `fork == true`, emit a `::notice::`
|
||
explaining why, and `should_run=false`. The actual `smoke` /
|
||
`visual` job stays guarded by `if: needs.gate.outputs.should_run == 'true'`
|
||
unchanged — it just doesn't fire for forks.
|
||
|
||
Verbatim shape baked into the brief:
|
||
|
||
```bash
|
||
if [[ "${{ github.event.pull_request.head.repo.fork }}" == "true" ]]; then
|
||
echo "should_run=false" >> $GITHUB_OUTPUT
|
||
echo "::notice::Smoke skipped on fork PR (bypass secret unavailable to forks)"
|
||
elif echo "${{ github.event.pull_request.body }}" | grep -qE 'pipeline:.*skip.*\bsmoke\b'; then
|
||
...
|
||
```
|
||
|
||
**Alternative (rejected):** add the fork check as an `if:` on the
|
||
`smoke` and `visual` jobs directly. Same effect, but loses the
|
||
`::notice::` annotation that surfaces in the GitHub Actions UI summary —
|
||
silent skip is worse UX than annotated skip.
|
||
|
||
**Side effect.** Until `adopt-playwright-smoke` ships, this convoy's
|
||
fork-PR skip applies to a workflow that already does nothing useful
|
||
(no Playwright config, no `@playwright/test`). The skip is forward-
|
||
looking — once the smoke pipeline becomes real, fork PRs gracefully opt
|
||
out instead of failing.
|
||
|
||
**Routing.** Operator ratifies at gate 1. Default to "yes, skip on
|
||
forks" unless rejected. If rejected, the brief drops the fork check and
|
||
the recommendation in `AGENTS.md` § 7 (the doc-writer pass) should
|
||
document the fork-PR failure mode.
|
||
|
||
## Architecture
|
||
|
||
### File plan
|
||
|
||
| File | Action | Purpose |
|
||
| --- | --- | --- |
|
||
| `.github/workflows/preview-smoke.yml` | modified | Inject bypass-secret query into `path:` of the wait-for-vercel-preview step (Decision A); lower `max_timeout` 600 → 120 (Decision B); extend `gate:` step to skip fork PRs (Decision D); add `VERCEL_AUTOMATION_BYPASS_SECRET` to the Playwright smoke step's `env:` for forward-compat with `adopt-playwright-smoke` |
|
||
| `.github/workflows/visual-diff.yml` | modified | Same shape as preview-smoke: bypass query on `path:`, `max_timeout` 600 → 120, fork-PR gate, `VERCEL_AUTOMATION_BYPASS_SECRET` in the screenshot-capture step's `env:` |
|
||
|
||
### API surface
|
||
|
||
N/A — this convoy modifies CI workflow YAML only. No HTTP routes are
|
||
added, modified, or removed.
|
||
|
||
### Schema diff
|
||
|
||
N/A — no database changes.
|
||
|
||
### Test plan
|
||
|
||
- **No new unit tests.** The change is workflow YAML; vitest does not
|
||
exercise GitHub Actions. The existing 16-test auth-surface suite stays
|
||
green and is unaffected.
|
||
- **Manual validation in the brief's "Manual verification" section:**
|
||
- Acceptance criterion #1 (wait-action exits successfully on the
|
||
convoy's own PR): observe by reading the workflow's run log after
|
||
pushing the convoy branch. Expect `Received success status code`
|
||
within the first few attempts and total wait-step duration < 90s.
|
||
- Acceptance criterion #4 (no bypass secret in workflow logs):
|
||
`gh run download <run-id> -n logs && rg "<first-8-chars-of-secret>"
|
||
logs/` (locally only — never paste the chars into a script or
|
||
commit). Expect zero matches.
|
||
- Acceptance criterion #5 (actionlint validation): document the
|
||
one-line `brew install actionlint` install OR a hermetic Docker
|
||
one-liner; recommended-not-required (no actionlint binary in CI
|
||
today, and gating on it would expand scope). The brief includes the
|
||
exact command.
|
||
- **Smoke / visual jobs themselves still fail** after the brief lands,
|
||
because `@playwright/test` is not installed and `playwright.config.js`
|
||
does not exist — the failure mode shifts from "401 timeout in the
|
||
wait step" (this convoy's target) to "playwright not installed" (the
|
||
`adopt-playwright-smoke` convoy's target). That is the **correct,
|
||
expected end state of this convoy.** Brief acceptance criterion #1
|
||
explicitly accepts a real downstream failure as success, as long as
|
||
the wait-action reaches `Received success status code` first.
|
||
|
||
### Risk list
|
||
|
||
- **R1 — Secret leaks via workflow log.** Even though the wait-action
|
||
itself doesn't echo `path:` (verified — `action.js:357,360,363` only
|
||
emit `targetUrl`, which does NOT include the query string the action
|
||
appended internally), any added `echo "$BASE_URL"` or `run: |` step
|
||
with `set -x` in the same job could expose the secret. Brief calls
|
||
this out and prohibits echoing constructed URLs. Mitigation: keep the
|
||
bypass *only* in `path:` and `env:` — never built into a shell
|
||
variable that a step might print.
|
||
- **R2 — `outputs.url` is the bare deployment URL (already verified) —
|
||
Playwright will need its own injection.** Confirmed via `action.js:360`:
|
||
`core.setOutput('url', targetUrl)` where `targetUrl = status.target_url`.
|
||
The `path:` query is NOT appended. So the BASE_URL Playwright receives
|
||
via `${{ steps.vercel.outputs.url }}` is clean — Playwright must inject
|
||
the bypass itself (via `extraHTTPHeaders` per Vercel's docs). This
|
||
convoy plumbs `VERCEL_AUTOMATION_BYPASS_SECRET` as an env var on the
|
||
step so `adopt-playwright-smoke` can read it from `process.env`.
|
||
- **R3 — `path:` parsing requires leading `/`.** `action.js:42`:
|
||
`new URL(path, url)`. If the implementer writes
|
||
`path: '?x-vercel-protection-bypass=...'` (no leading `/`), the URL
|
||
resolver will combine relative-to-current-document which can drop the
|
||
origin. Brief acceptance criterion explicitly mandates `path: '/?...'`.
|
||
- **R4 — `max_timeout: 120` may be too aggressive for very slow Vercel
|
||
builds.** PR #16's run log evidence (deployment URL retrieved within
|
||
1 second of job start) suggests the deployment is already up by the
|
||
time the workflow triggers. 120s gives ~60 polls at the default 2s
|
||
interval. If a cold-start build legitimately takes > 120s, the
|
||
workflow will time out. Mitigation: operator may override at gate 1
|
||
if recent Vercel build times have been long. Easy revert.
|
||
- **R5 — Fork-PR gate misclassification.** GitHub's
|
||
`github.event.pull_request.head.repo.fork` is a boolean but is
|
||
rendered as the string `"true"` / `"false"` in expression context.
|
||
The brief's shell check uses `[[ ... == "true" ]]`, which is the
|
||
safe comparison.
|
||
- **R6 — Token rotation invalidates CI silently.** If the operator
|
||
rotates the bypass token in the Vercel dashboard but forgets to
|
||
re-seed the GitHub secret, the workflow will start failing with the
|
||
same 401 + timeout it does today. This is documented in convoy file
|
||
§ Known constraints; not preventable from workflow YAML. The doc-
|
||
writer pass should add a one-line note to `AGENTS.md` § 7 listing
|
||
the secret-rotation runbook.
|
||
- **R7 — Concurrency-group cancellation interacts with the bypass URL?
|
||
Confirmed: no.** `concurrency:` uses only `github.event.pull_request.number`;
|
||
no secret reference. Decision C covers this.
|
||
- **R8 — actionlint not in CI.** No workflow validator runs on PRs
|
||
today. The brief recommends a local `actionlint` install for the
|
||
implementer; CI integration is its own scope (queueable as
|
||
`adopt-actionlint`).
|
||
|
||
### Decomposition
|
||
|
||
| Brief # | Title | Files | Depends on | Estimated PR size |
|
||
| --- | --- | --- | --- | --- |
|
||
| 1 | Inject `VERCEL_AUTOMATION_BYPASS_SECRET` into preview-smoke + visual-diff workflows | `.github/workflows/preview-smoke.yml`, `.github/workflows/visual-diff.yml` | none | ~50 LOC YAML diff total |
|
||
|
||
**Why 1 brief and not 2** (one per workflow):
|
||
|
||
- Both files take the **identical shape change** (same wait-action step,
|
||
same `max_timeout` reduction, same gate-job extension, same
|
||
forward-compat env var). The diffs are parallel and best reviewed
|
||
together — PR #16 set the precedent of touching both workflow files
|
||
in a single PR for this exact reason.
|
||
- Splitting into 2 briefs would force two PRs into the same review
|
||
surface, two implementer runs, two convoy-cycle bookings, with zero
|
||
reviewer benefit: the files are independently reverte-able at the
|
||
file level inside a single PR.
|
||
- Total brief LOC is well under the 400-LOC architect ceiling.
|
||
- No cross-brief commitments are needed.
|
||
|
||
If the implementer surfaces a reason the two files must diverge mid-
|
||
flight (e.g. visual-diff needs a different `path:` because it captures
|
||
a deeper page), that's a Decision-letter scope expansion documented in
|
||
this file, not a re-decomposition.
|
||
|
||
### Slice dependencies (multitask-ready)
|
||
|
||
```yaml
|
||
slice_dependencies:
|
||
- brief: 1
|
||
depends_on: []
|
||
files:
|
||
- .github/workflows/preview-smoke.yml
|
||
- .github/workflows/visual-diff.yml
|
||
```
|
||
|
||
Single brief — no parallelization opportunity. Conductor dispatches
|
||
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://<preview>/?x-vercel-protection-bypass=<secret>" | head -1
|
||
→ HTTP/2 200 (works, no cookie needed)
|
||
|
||
curl -sI "https://<preview>/?x-vercel-protection-bypass=<secret>&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 "<new value>"`.
|
||
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`).
|