From 9a3e077d405981d98ca9a38a5610347d599e9032 Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Sun, 24 May 2026 16:26:22 -0500 Subject: [PATCH] fix(ci): plumb VERCEL_AUTOMATION_BYPASS_SECRET into preview-smoke + visual-diff (#17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * architect: plan fix-vercel-deployment-protection-in-ci convoy (1 brief) Flip convoy status queued -> in-progress and append the architect output per the role-architect contract: - Decisions (post-IA round) A/B/C/D with recommendations + rationale, routed back to operator for gate-1 ratification (A and D especially). - Architecture section: file plan, risk list (R1-R8), test plan, decomposition (1 brief, justified), slice_dependencies block. - brief-1: inject VERCEL_AUTOMATION_BYPASS_SECRET into both workflows via query-param-on-path (Decision A), tighten max_timeout 600 -> 120 (Decision B), extend gate to skip fork PRs (Decision D), and forward the secret as an env var to the Playwright/visual step for forward- compat with adopt-playwright-smoke. Boot-the-brief findings preempted in the brief: - wait-for-vercel-preview@v1.3.2 source confirms only `targetUrl` (the bare deployment URL) is ever logged or emitted as outputs.url; the bypass query in `path:` is structurally invisible to logs and to downstream consumers of ${{ steps.vercel.outputs.url }}. - The action exposes no custom-header input -- Option B is mechanically impossible for the wait step without forking the action. - `path:` MUST begin with a leading `/` because the action parses it via `new URL(path, url)`. - PR #16 run logs confirm the 401 timeout failure mode the convoy targets. - Playwright config doesn't exist yet (adopt-playwright-smoke owns it); this brief only plumbs the env var. No workflow YAML, no Playwright config, no AGENTS.md changes in this commit -- those land in the implementer phase per the architect contract. Awaiting human gate 1 (Decisions A + B + D ratification + brief approval) before implementer dispatch. Co-authored-by: Cursor * fix(ci): plumb VERCEL_AUTOMATION_BYPASS_SECRET into preview-smoke + visual-diff (Brief 1 of fix-vercel-deployment-protection-in-ci) Closes the CI-infra half of P0 #7's follow-up. PR #16 (squash commit 7e97254) added scoped permissions to both workflows but exposed that Vercel Deployment Protection 401s anonymous GitHub-runner requests, causing both Playwright smoke and Screenshot diff to time out at 10 minutes on every PR. This brief plumbs the bypass secret end-to-end so the wait-action's healthcheck reaches 200. Per architect Decision A (.convoys/fix-vercel-deployment-protection-in-ci.md): - preview-smoke.yml + visual-diff.yml: wait-for-vercel-preview's `path:` input now carries the bypass as a query parameter `?x-vercel-protection-bypass=${{ secrets.* }}&x-vercel-set-bypass-cookie=true`. The action only logs the bare targetUrl (verified in action.js:357,360,363) so the secret stays out of workflow logs. Per Decision B: - max_timeout: 600 -> 120. PR #16 evidence shows Vercel previews are up within seconds of job start; 120s gives ample headroom and surfaces misconfigurations in ~2 minutes instead of ~10. Per Decision D (NEW -- surfaced by Boot-the-brief): - gate: job's Decide step now checks github.event.pull_request.head.repo.fork FIRST. Forks lack repo secrets, so they would otherwise burn ~4 minutes per PR on a misleading 401. The fork-check emits `::notice::` and short- circuits before the existing skip-via-PR-body directive runs. Forward-compat for adopt-playwright-smoke: - Both workflows' Playwright/screenshot-capture step now exports VERCEL_AUTOMATION_BYPASS_SECRET as env. The actual Playwright config consumes it via extraHTTPHeaders in adopt-playwright-smoke's brief. BASE_URL stays as the bare \${{ steps.vercel.outputs.url }} (no query string) so it remains safe to echo. Verification: - YAML parses (js-yaml load on both files: preview-smoke jobs [gate, smoke]; visual-diff jobs [gate, visual]) - actionlint not run (binary not installed locally); recommend installing for future PRs. Future adopt-actionlint convoy could add it to CI. - No `set -x`, `echo`, `cat`, or `printf` of the secret or bypass URL in any modified step - permissions: and concurrency: blocks unchanged (PR #16 contracts preserved) Co-authored-by: Cursor * fix(ci): route github.event.pull_request.body through env: to avoid shell injection Pre-existing latent bug surfaced by PR #17's CI run. The Decide step's inline `${{ github.event.pull_request.body }}` interpolation pastes arbitrary PR-body text directly into a bash script. When the body contains shell metacharacters (parens, pipes, backticks, redirections, etc.) the resulting script either errors out at YAML-load time OR — worse — executes attacker-controlled shell. This bit PR #17 with a real syntax error ("unexpected token `('") because the PR body contains parenthesized phrases like "(was: 10-minute timeout)". Every Decide-step run in this repo has been one badly-formatted PR body away from breaking the gate. Fix: forward `github.event.pull_request.body` and `github.event.pull_request.head.repo.fork` through the step's `env:` block as `PR_BODY` and `PR_IS_FORK`, then quote them in shell (`"$PR_BODY"`, `"$PR_IS_FORK"`). The env-var path leaves the values as plain strings rather than syntactically embedded code, which is the standard GitHub Actions hardening pattern (see GitHub's "Security hardening for GitHub Actions" → "Using a third-party action"). Same change in both workflows; ~9 LOC each. This fix is technically beyond Brief 1's scope (which targeted only Vercel-bypass plumbing) but is added in this convoy because the bug actively blocks Brief 1's success criterion from being validated on PR #17. Documented in the convoy file's "Anything flagged but not acted on" follow-up pass. Co-authored-by: Cursor * fix(ci): drop x-vercel-set-bypass-cookie=true from wait-action path The wait-action's healthcheck was still 401ing despite the bypass query being correct. Root cause: with `x-vercel-set-bypass-cookie=true`, Vercel returns 307 + Set-Cookie (`_vercel_jwt`), but axios in Node has no cookie jar — it follows the redirect to the bare URL without the cookie, which then 401s. Local verification (run by operator): 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 (adopt-playwright-smoke) where a real browser cookie jar exists. Added an inline comment in preview-smoke.yml explaining this so the next agent doesn't accidentally re-add the cookie param. Co-authored-by: Cursor --------- Co-authored-by: Cursor --- .../fix-vercel-deployment-protection-in-ci.md | 312 ++++++++++++++- ...-inject-bypass-secret-into-ci-workflows.md | 362 ++++++++++++++++++ .github/workflows/preview-smoke.yml | 25 +- .github/workflows/visual-diff.yml | 15 +- 4 files changed, 708 insertions(+), 6 deletions(-) create mode 100644 .convoys/fix-vercel-deployment-protection-in-ci/brief-1-inject-bypass-secret-into-ci-workflows.md diff --git a/.convoys/fix-vercel-deployment-protection-in-ci.md b/.convoys/fix-vercel-deployment-protection-in-ci.md index 2ed0afd..7baecb0 100644 --- a/.convoys/fix-vercel-deployment-protection-in-ci.md +++ b/.convoys/fix-vercel-deployment-protection-in-ci.md @@ -13,7 +13,7 @@ skip: - role-ux-reviewer # no UX surface - role-ia-architect # no IA surface - browser-smoke # the convoy IS the smoke pipeline -status: queued +status: in-progress created: 2026-05-24 parent: ship-readiness addresses: P0 #7 (CI infrastructure side-effect) @@ -236,4 +236,312 @@ to solve in this convoy. Each is queued separately if it warrants a fix. 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. + 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:///?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 ')`. + 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:///?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 -n logs && rg "" + 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. diff --git a/.convoys/fix-vercel-deployment-protection-in-ci/brief-1-inject-bypass-secret-into-ci-workflows.md b/.convoys/fix-vercel-deployment-protection-in-ci/brief-1-inject-bypass-secret-into-ci-workflows.md new file mode 100644 index 0000000..39d20c5 --- /dev/null +++ b/.convoys/fix-vercel-deployment-protection-in-ci/brief-1-inject-bypass-secret-into-ci-workflows.md @@ -0,0 +1,362 @@ +--- +convoy: fix-vercel-deployment-protection-in-ci +brief_number: 1 +depends_on: [] +files: + - .github/workflows/preview-smoke.yml + - .github/workflows/visual-diff.yml +--- + +# Brief 1: Inject `VERCEL_AUTOMATION_BYPASS_SECRET` into the preview-smoke + visual-diff workflows so the wait-for-vercel-preview healthcheck passes against protected Vercel previews + +## Goal (1 sentence) + +Plumb `${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}` into `wait-for-vercel-preview`'s `path:` input as a query parameter (Decision A) in both workflows, tighten `max_timeout` from 600 → 120 (Decision B), extend the `gate:` job to skip fork PRs (Decision D), and forward the same secret as an env var to the Playwright smoke / screenshot-capture step so `adopt-playwright-smoke` finds the surface pre-wired — without ever leaking the secret to a workflow log or to `${{ steps.vercel.outputs.url }}`. + +## Files in scope (do not edit anything else) + +- `.github/workflows/preview-smoke.yml` — modified. +- `.github/workflows/visual-diff.yml` — modified. + +**Files explicitly out of scope** (do not touch even if it seems related): + +- `playwright.config.js` — does not exist yet; `adopt-playwright-smoke` owns it. +- `tests/smoke/app.smoke.spec.ts` — already exists as a stub but stays in `tests/smoke/`; `adopt-playwright-smoke` owns it. +- `tests/visual/` — does not exist yet; `adopt-playwright-smoke` owns it. +- `AGENTS.md` § 7 — the wording correction (header → query param; queued → wired) is the doc-writer pass at convoy close, NOT this brief. +- `package.json` — no new deps. `@playwright/test` is not in scope here. +- Any other workflow in `.github/workflows/` (e.g. `ci.yml`) — out of scope. + +## Conventions to follow + +- **Decision A (`.convoys/fix-vercel-deployment-protection-in-ci.md` § Decisions post-IA round).** The wait-action receives the bypass via query param on `path:`. The header form is reserved for the future `playwright.config.js`. +- **Decision B (same file).** `max_timeout: 600` → `max_timeout: 120` in both workflows. Operator may override at gate 1 if Vercel builds have been slow recently. +- **Decision C (same file).** Do NOT place `${{ secrets.* }}` inside any `concurrency:` group expression. GitHub Actions YAML parser rejects secret refs in `concurrency:` and the workflow fails to load. The `concurrency:` blocks stay unchanged. +- **Decision D (same file).** Extend the existing `gate:` step's `Decide` shell script to check `github.event.pull_request.head.repo.fork` FIRST. When the PR comes from a fork, emit `::notice::` and set `should_run=false`. Same shape in both workflows. +- **No-go zones (`.cursor/rules/no-go-zones.mdc`).** None of the files in scope are in the no-go list. Do not edit anything outside `.github/workflows/preview-smoke.yml` and `.github/workflows/visual-diff.yml` in this brief. +- **Secret-handling discipline:** + - NEVER `echo`, `cat`, or `printf` a URL or env var that contains `${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}` or `$VERCEL_AUTOMATION_BYPASS_SECRET`. + - NEVER use `set -x` in any `run:` step in either workflow (it would echo every command including ones that interpolate secrets). + - NEVER assign the constructed URL (the one with the bypass query) to a shell variable that is later printed. + - The wait-action's own logs are safe (see Boot-the-brief finding #2 below). The risk is in YOUR additions, not in the action. + - GitHub Actions auto-masks values that match registered secrets in workflow logs. That is a backstop, not a primary defense. Do not rely on it to redact full URLs. +- **Style match.** PR #16 (`fix(ci): scoped permissions for preview-smoke + visual-diff workflows`, squash commit `7e97254`) is the precedent for touching both workflow files in the same PR. Follow its diff shape: same change applied to both files, with workflow-specific differences (smoke vs visual-diff naming, the `pull-requests: write` permission only on visual-diff) preserved as-is. + +## Acceptance criteria + +### `.github/workflows/preview-smoke.yml` + +- [ ] **Wait-action step (currently lines 60-65) becomes:** + +```yaml + - name: Wait for Vercel Preview deployment + id: vercel + uses: patrickedqvist/wait-for-vercel-preview@v1.3.2 + with: + token: ${{ secrets.GITHUB_TOKEN }} + max_timeout: 120 + path: /?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}&x-vercel-set-bypass-cookie=true +``` + + Three changes relative to the current step: + + 1. `max_timeout: 600` → `max_timeout: 120` (Decision B). + 2. New `path:` input — **MUST begin with a leading `/`**. `action.js:42` parses this via `new URL(path, url)`; a missing leading `/` makes the URL resolver produce a path relative to the wrong base. Verbatim `/` then `?` then the two query params. + 3. The `x-vercel-set-bypass-cookie=true` segment is REQUIRED, not optional. It causes Vercel to set a `_vercel_jwt` cookie on the response so any follow-up same-origin requests (e.g. Playwright's `page.goto` redirects) reuse the bypass without needing the query string again. + + Do NOT: + - Move the `path:` line above `max_timeout:` (no semantic difference, but match the verbatim order so the diff stays minimal). + - Use single quotes around the `path:` value. YAML treats the unquoted form as a plain string; quoting introduces escape-handling questions. Leave it unquoted. + - Add an `env:` block to this step. The wait-action does not read `process.env.VERCEL_AUTOMATION_BYPASS_SECRET`; it consumes the input only. + +- [ ] **`gate:` job's `Decide` step (currently lines 41-49) becomes:** + +```yaml + - name: Decide + id: check + run: | + 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 + echo "should_run=false" >> $GITHUB_OUTPUT + echo "::notice::Smoke skipped via pipeline directive" + else + echo "should_run=true" >> $GITHUB_OUTPUT + fi +``` + + Three changes relative to current: + + 1. New `if [[ "${{ ... fork }}" == "true" ]]` branch FIRST. It must come before the body-directive check so fork PRs short-circuit out without parsing the PR body. + 2. The shell comparison is `== "true"` (a string compare against the literal string `"true"`). `github.event.pull_request.head.repo.fork` is rendered as the string `"true"` or `"false"` in expression context — NOT as a bare boolean (Risk R5 in the convoy's architecture risk list). + 3. The notice mentions "fork PR" explicitly; this surfaces in the GitHub Actions UI summary so a reader scanning a PR can immediately see why smoke didn't run. + + Do NOT: + - Replace the body-directive check (`pipeline:.*skip.*\bsmoke\b`). That gate is still useful for non-fork PRs that legitimately want to skip smoke (e.g. doc-only PRs). + - Move the `gate:` step's `if:` higher — `if: needs.gate.outputs.should_run == 'true'` on the `smoke:` job is the correct gate; it stays. + +- [ ] **Playwright smoke step (currently lines 77-80) becomes:** + +```yaml + - name: Run smoke tests + run: npx playwright test --project=smoke + env: + BASE_URL: ${{ steps.vercel.outputs.url }} + VERCEL_AUTOMATION_BYPASS_SECRET: ${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} +``` + + One change relative to current: + + 1. New `VERCEL_AUTOMATION_BYPASS_SECRET: ${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}` line in the step's `env:` block. This is forward-compat plumbing — the `playwright.config.js` that `adopt-playwright-smoke` ships will read `process.env.VERCEL_AUTOMATION_BYPASS_SECRET` and inject the bypass via `extraHTTPHeaders` per Vercel's snippet. + 2. **Do NOT modify `BASE_URL`.** It stays `${{ steps.vercel.outputs.url }}`. The wait-action's `outputs.url` is the bare deployment URL — the bypass query string is NOT appended (confirmed by reading `action.js:360`). Playwright will inject the bypass via headers; the URL must stay clean so the headers actually apply on every request (Playwright re-applies `extraHTTPHeaders` per request, including redirects). + 3. Until `adopt-playwright-smoke` ships, this step will FAIL because `@playwright/test` is not installed. That's the documented end state of THIS brief (see acceptance criterion #1 at the bottom of this section); do not try to fix it here. + +- [ ] **All other lines in `.github/workflows/preview-smoke.yml` stay byte-for-byte identical to the current file** — including: + - The `name:` line. + - The full `on:` block (PR types, target branch). + - The full `concurrency:` block (Decision C — no secret reference). + - The full `permissions:` block (PR #16 already landed the minimal scope; do not touch). + - The `gate:` job's `name:`, `runs-on:`, `outputs:`, the existing `actions/checkout@v4` step in the `smoke:` job, `actions/setup-node@v4`, `npm ci`, `npx playwright install --with-deps chromium`, and the `Upload Playwright report on failure` step. + - The leading multi-line comment block at the top of the file (lines 1-11). Update text is the doc-writer's job, not the implementer's. + +### `.github/workflows/visual-diff.yml` + +- [ ] **Wait-action step (currently lines 56-61) becomes:** + +```yaml + - name: Wait for Vercel Preview deployment + id: vercel + uses: patrickedqvist/wait-for-vercel-preview@v1.3.2 + with: + token: ${{ secrets.GITHUB_TOKEN }} + max_timeout: 120 + path: /?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}&x-vercel-set-bypass-cookie=true +``` + + Identical to the preview-smoke version (same wait-action, same input shape, same secret). The shared shape is intentional — Decision A applies to both workflows. + +- [ ] **`gate:` job's `Decide` step (currently lines 38-45) becomes:** + +```yaml + - id: check + run: | + if [[ "${{ github.event.pull_request.head.repo.fork }}" == "true" ]]; then + echo "should_run=false" >> $GITHUB_OUTPUT + echo "::notice::Visual diff skipped on fork PR (bypass secret unavailable to forks)" + elif echo "${{ github.event.pull_request.body }}" | grep -qE 'pipeline:.*skip.*\bvisual\b'; then + echo "should_run=false" >> $GITHUB_OUTPUT + echo "::notice::Visual diff skipped via pipeline directive" + else + echo "should_run=true" >> $GITHUB_OUTPUT + fi +``` + + Same shape as preview-smoke's gate, with two text-only differences: + + 1. The body-directive regex is `\bvisual\b` (was `\bsmoke\b` in preview-smoke). Matches the existing convention in the current file. + 2. The two `::notice::` strings say "Visual diff" instead of "Smoke" — matches the workflow's name. + + Note: the current `visual-diff.yml` `Decide` step is missing the `name:` field (the current file is `- id: check` directly). Preserve that style — do not add a `name:` here just because preview-smoke has one. The diff stays minimal. + +- [ ] **Screenshot-capture step (currently lines 71-75) becomes:** + +```yaml + - name: Capture screenshots (PR) + run: npx playwright test --project=visual --update-snapshots=none + env: + BASE_URL: ${{ steps.vercel.outputs.url }} + VERCEL_AUTOMATION_BYPASS_SECRET: ${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} + continue-on-error: true +``` + + Same change as preview-smoke's smoke step — add `VERCEL_AUTOMATION_BYPASS_SECRET` to `env:`. Do NOT remove `continue-on-error: true` (the visual-diff workflow's design is to upload diffs even when tests fail; that behavior stays). + +- [ ] **All other lines stay byte-for-byte identical** — including: + - `name:`, `on:` (paths-only trigger), `concurrency:`, `permissions:` (note: visual-diff has `pull-requests: write` because of the comment-on-PR step; do not change this). + - The leading multi-line comment block at the top of the file (lines 1-5). + - The `actions/upload-artifact@v4` and `actions/github-script@v7` (comment-on-PR) steps. + +### Cross-file checks (apply to both YAML files) + +- [ ] **No `${{ secrets.* }}` reference appears inside the `concurrency:` block** in either file. Grep before committing: `rg 'secrets\.' .github/workflows/preview-smoke.yml .github/workflows/visual-diff.yml` should return exactly two matches per file (one in the wait-action's `path:`, one in the Playwright/visual step's `env:`). Three or more matches per file means a stray secret reference snuck somewhere; investigate. +- [ ] **No `echo`, `printf`, `cat`, or `set -x` references the bypass-bearing URL or env var.** Grep: `rg 'echo|printf|cat|set -x' .github/workflows/preview-smoke.yml .github/workflows/visual-diff.yml` — every match should be an existing line untouched by this brief (the `gate:` job's `echo "should_run=..." >> $GITHUB_OUTPUT` lines are fine; they don't echo URLs or secrets). +- [ ] **Diff hygiene.** `git diff main..HEAD -- .github/workflows/` should show only the changes specified above. No whitespace-only changes elsewhere. No unrelated edits. Total diff is expected to be ~50 LOC across both files (~20 LOC per file changed, plus a few context lines). + +### Acceptance criterion #1 — end-state behavior + +After this brief lands on the convoy branch and a Vercel preview deployment is published for the PR: + +- [ ] **The wait-action's healthcheck exits successfully.** In the `Playwright smoke` job's run log, expect a single line near the end of the `Wait for Vercel Preview deployment` step: + ``` + Received success status code + ``` + This means the bypass query made it through to Vercel and the protected preview returned 2xx. +- [ ] **The wait-action step takes < 90 seconds wall time** (typical: < 10s if the deployment is already up, which is the usual case based on PR #16's logs). +- [ ] **The subsequent `Run smoke tests` step (preview-smoke) or `Capture screenshots (PR)` step (visual-diff) is REACHED**, even though it will fail because `@playwright/test` is not installed. The expected failure mode is roughly: + ``` + npm ERR! could not determine executable to run + npm error code ENOENT + npm error path .../node_modules/@playwright/test + ``` + OR a `npx`-driven download that succeeds but then fails on the missing config. Either is acceptable for this brief — the success metric is "wait-action passed and Playwright step was reached," not "Playwright passed." +- [ ] **Total job runtime is < 5 minutes.** The convoy file's success metric. Now-correctly-passing wait step (~10s) + reached-but-failing Playwright step (~30-90s) is well inside 5 minutes. +- [ ] **The bypass secret value does NOT appear in any line of the run log.** Verify after the run: + ```bash + gh run download -n logs + # Then locally — NEVER commit this script — check that the secret's + # first 8 chars do not appear in the downloaded logs. GitHub Actions + # also auto-masks; this is belt-and-suspenders. + rg "$(head -c 8 <<< "$VERCEL_AUTOMATION_BYPASS_SECRET")" logs/ || echo "OK — bypass not in logs" + ``` + Expected: `OK — bypass not in logs`. + +## Manual verification (in addition to the workflow run on push) + +Run these in order. Paste relevant output (with secrets redacted) into the PR description. + +- [ ] **Local YAML lint.** Install actionlint (one-time): + ```bash + brew install actionlint + ``` + Then validate: + ```bash + actionlint .github/workflows/preview-smoke.yml .github/workflows/visual-diff.yml + ``` + Expected: zero output, exit code 0. If actionlint flags anything, + read the message — most actionlint warnings are real (shellcheck + embedded). Investigate before commit. If `brew` is unavailable, the + binary is downloadable from the actionlint releases page; recommended + but not strictly required by acceptance criterion (the workflow YAML + is small enough to eyeball). + +- [ ] **Local `gh` dry-run check.** Verify the secret is still seeded + (operator says it is, but confirm before pushing): + ```bash + gh secret list | grep VERCEL_AUTOMATION_BYPASS_SECRET + ``` + Expected: one line showing the secret name and an `Updated` timestamp. + +- [ ] **Push the branch and observe the first workflow run.** From the + convoy branch (`convoy/fix-vercel-deployment-protection-in-ci`): + ```bash + git push -u origin HEAD + ``` + Then watch the Preview-smoke and Visual-diff workflows. The first + Vercel preview deploy on this PR is the one to scrutinize. Expect: + - Wait-action step logs `target url » https://.vercel.app` + (note: no query string in this log — that's the action's safe log + of the bare deployment URL). + - Within a few seconds, `Received success status code`. + - Step exits 0. + - Next step (`Set up Node`, `npm ci`, etc.) runs. + - Eventually fails at the Playwright step — that's the expected end + state of THIS brief. + +- [ ] **Re-run validation.** Click "Re-run jobs" on the same run. Expect + identical behavior — the wait-action's healthcheck issues fresh + axios GETs on every iteration (no caching), so re-runs are + idempotent (Risk R-not-listed-because-confirmed-OK). + +- [ ] **Bypass-log-leak validation** (as documented above under acceptance + criterion #1, final bullet). + +## Boot-the-brief findings (preempted by the architect; do not re-investigate) + +### Finding 1 — Wait-action source: `path:` is consumed via `new URL(path, url)` + +`patrickedqvist/wait-for-vercel-preview@v1.3.2` at `action.js:42`: + +```js +let checkUri = new URL(path, url); +await axios.get(checkUri.toString(), { headers }); +``` + +Parsed as a URL relative to `url` (the deployment URL). Query strings +work verbatim. **MUST begin with `/`** or the URL resolver produces +unexpected paths. + +### Finding 2 — Wait-action source: bypass secret never appears in the action's logs + +Action source emits exactly three `console.log` calls that include URL +content: + +1. `action.js:357` — `console.log('target url »', targetUrl)`. `targetUrl` + is `status.target_url` (the bare deployment URL from the GitHub + Deployments API). **No `path:` is appended.** ✅ Safe. +2. `action.js:363` — `console.log('Waiting for a status code 200 from: ${targetUrl}')`. Same `targetUrl`. ✅ Safe. +3. `action.js:53-54` — `console.log('GET status: ${e.response.status}. Attempt ${i} of ${iterations}')`. Only the HTTP status code, no URL. ✅ Safe. + +The bypass query string lives ONLY in the internal `checkUri` axios +call. **This means: passing the bypass via `path:` is structurally +safe from log leakage by the action itself.** Your remaining job is to +not add any echo/print step in the workflow YAML that constructs a URL +with the bypass. + +### Finding 3 — `outputs.url` is the bare URL (no bypass query) + +`action.js:360`: `core.setOutput('url', targetUrl)` — `targetUrl` does +NOT include `path:`. So `${{ steps.vercel.outputs.url }}` downstream is +clean. **This is why the Playwright `BASE_URL` env var stays bare:** the +future `playwright.config.js` will inject the bypass via +`extraHTTPHeaders`, NOT by reconstructing a URL with the query string. + +### Finding 4 — Empty/unset secret on fork PRs + +GitHub Actions silently omits repo secrets on `pull_request`-event runs +from forks. If the fork-PR gate (Decision D) is NOT added, fork PRs +would build `https:///?x-vercel-protection-bypass=&x-vercel-set-bypass-cookie=true`, +get 401, and time out at `max_timeout` (120s after Decision B, but +still 4 minutes total wasted per fork PR across both workflows). The +fork-PR gate in Decision D prevents this entirely. + +### Finding 5 — `max_timeout: 600` is excessive + +PR #16's failed run (`gh run view 26370087240`) shows the wait-action +retrieved the deployment URL within 1 second (`target url » ...` at +T+1s relative to job start). The healthcheck then 401-looped for +~600s. With a working bypass, the first axios GET would have succeeded +within 2 seconds. **120s is plenty of headroom.** Decision B applies. + +### Finding 6 — Concurrency expression is safe + +Current `concurrency:` blocks: + +- preview-smoke: `group: preview-smoke-${{ github.event.pull_request.number }}` +- visual-diff: `group: visual-diff-${{ github.event.pull_request.number }}` + +No secret reference. Adding the secret via the wait-action's `with:` +and the Playwright step's `env:` does NOT touch `concurrency:`. The +cancel-stale-runs behavior is preserved. Decision C applies. + +### Finding 7 — `tests/smoke/app.smoke.spec.ts` is a `.ts` file in a JS-only repo + +Out of scope for this brief — flagged for `adopt-playwright-smoke`, +which will own both the Playwright config and the JS/TS decision for +its test files. Do NOT rename or edit it here. + +### Finding 8 — actionlint is not installed locally for the implementer + +Not a blocker. The brief recommends installing it for local validation +(see Manual verification), but the absence of actionlint in CI today +means it's a recommended-not-required check. A future `adopt-actionlint` +convoy can add it to CI; this brief stays focused on the bypass plumb. + +## Out of scope (do not do these) + +- [ ] No new Playwright tests or `playwright.config.js`. +- [ ] No `@playwright/test` install. +- [ ] No `package.json` or `package-lock.json` changes. +- [ ] No edits to any other workflow YAML (`ci.yml`, etc.). +- [ ] No edits to `AGENTS.md` § 7 (the wording correction header → query param is the doc-writer pass after this convoy closes). +- [ ] No edits to `.cursor/rules/*.mdc`. +- [ ] No new vitest tests (the existing 16-test suite remains green and is unrelated to this convoy). +- [ ] No replacement of `patrickedqvist/wait-for-vercel-preview` with another action or a hand-rolled `gh api` + `curl` poll loop. That's `replace-wait-for-vercel-preview` (queued, separate scope) and is explicitly out of scope per the convoy file. +- [ ] No tightening or loosening of the `permissions:` blocks in either workflow — PR #16 landed the minimal scope. +- [ ] No `setup-node@v4` version bump, no `actions/checkout@v4` bump, no `actions/upload-artifact@v4` bump. Those are general dependency-bump scope, not this convoy's. +- [ ] No `max_timeout` change beyond the 600 → 120 specified by Decision B. +- [ ] No echo of the constructed URL or BASE_URL in any step. Even for "debugging." If a debug echo is needed during local iteration, remove it before committing. + +## Rationale (≤3 sentences) + +The wait-action's healthcheck loop is the de-facto bypass-works assertion; injecting the bypass via `path:` query is the only mechanism the action exposes (its `action.yml` has no custom-header input), and the action's source confirms the query never leaks to logs or `outputs.url`. Combining query-param-on-wait with header-form-on-Playwright (deferred to `adopt-playwright-smoke` via the plumb-the-env-var step) keeps both call sites idiomatic for their respective HTTP clients. The fork-PR gate and the `max_timeout` reduction are small operator-quality-of-life refinements that make a 10-minute failure into a 2-minute (or zero-minute) failure when something does go wrong. diff --git a/.github/workflows/preview-smoke.yml b/.github/workflows/preview-smoke.yml index a4f7a73..2f65fc8 100644 --- a/.github/workflows/preview-smoke.yml +++ b/.github/workflows/preview-smoke.yml @@ -40,8 +40,20 @@ jobs: steps: - name: Decide id: check + env: + # Route PR body + fork flag through env vars instead of inline + # ${{ }} interpolation. Direct ${{ github.event.pull_request.body }} + # in a shell command pastes arbitrary user-controlled text (parens, + # backticks, pipes, heredocs) directly into the script — both a + # syntax-error risk AND a shell-injection vector. Quoting the env + # vars below makes both safe. + PR_BODY: ${{ github.event.pull_request.body }} + PR_IS_FORK: ${{ github.event.pull_request.head.repo.fork }} run: | - if echo "${{ github.event.pull_request.body }}" | grep -qE 'pipeline:.*skip.*\bsmoke\b'; then + if [[ "$PR_IS_FORK" == "true" ]]; then + echo "should_run=false" >> $GITHUB_OUTPUT + echo "::notice::Smoke skipped on fork PR (bypass secret unavailable to forks)" + elif echo "$PR_BODY" | grep -qE 'pipeline:.*skip.*\bsmoke\b'; then echo "should_run=false" >> $GITHUB_OUTPUT echo "::notice::Smoke skipped via pipeline directive" else @@ -62,7 +74,15 @@ jobs: uses: patrickedqvist/wait-for-vercel-preview@v1.3.2 with: token: ${{ secrets.GITHUB_TOKEN }} - max_timeout: 600 + max_timeout: 120 + # NOTE: do NOT add `&x-vercel-set-bypass-cookie=true` here. Vercel + # responds to that with a 307 + Set-Cookie (`_vercel_jwt`), but + # axios in Node has no cookie jar — the cookie is dropped before + # the followup request, which then 401s. For this one-shot + # healthcheck the bare bypass query is enough; the cookie variant + # belongs in the future Playwright config where the browser does + # have a cookie jar. + path: /?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} - uses: actions/setup-node@v4 with: @@ -78,6 +98,7 @@ jobs: run: npx playwright test --project=smoke env: BASE_URL: ${{ steps.vercel.outputs.url }} + VERCEL_AUTOMATION_BYPASS_SECRET: ${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} - name: Upload Playwright report on failure if: failure() diff --git a/.github/workflows/visual-diff.yml b/.github/workflows/visual-diff.yml index eec3606..f6771e9 100644 --- a/.github/workflows/visual-diff.yml +++ b/.github/workflows/visual-diff.yml @@ -36,8 +36,16 @@ jobs: should_run: ${{ steps.check.outputs.should_run }} steps: - id: check + env: + # See preview-smoke.yml for the rationale: ${{ github.event.* }} + # inlined into shell is a syntax-error + injection vector. + PR_BODY: ${{ github.event.pull_request.body }} + PR_IS_FORK: ${{ github.event.pull_request.head.repo.fork }} run: | - if echo "${{ github.event.pull_request.body }}" | grep -qE 'pipeline:.*skip.*\bvisual\b'; then + if [[ "$PR_IS_FORK" == "true" ]]; then + echo "should_run=false" >> $GITHUB_OUTPUT + echo "::notice::Visual diff skipped on fork PR (bypass secret unavailable to forks)" + elif echo "$PR_BODY" | grep -qE 'pipeline:.*skip.*\bvisual\b'; then echo "should_run=false" >> $GITHUB_OUTPUT echo "::notice::Visual diff skipped via pipeline directive" else @@ -58,7 +66,9 @@ jobs: uses: patrickedqvist/wait-for-vercel-preview@v1.3.2 with: token: ${{ secrets.GITHUB_TOKEN }} - max_timeout: 600 + max_timeout: 120 + # See preview-smoke.yml for the no-`set-bypass-cookie` rationale. + path: /?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} - uses: actions/setup-node@v4 with: @@ -72,6 +82,7 @@ jobs: run: npx playwright test --project=visual --update-snapshots=none env: BASE_URL: ${{ steps.vercel.outputs.url }} + VERCEL_AUTOMATION_BYPASS_SECRET: ${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} continue-on-error: true - name: Upload screenshots + diffs