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 <cursoragent@cursor.com>
This commit is contained in:
Randall Stillwell
parent
efca3e84c1
commit
d3ea1cbf24
2 changed files with 672 additions and 2 deletions
|
|
@ -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://<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.
|
||||
|
|
|
|||
|
|
@ -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 <run-id> -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://<deployment>.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://<deployment>/?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.
|
||||
Loading…
Reference in a new issue