ci(studio): require before and after captures on studio and player prs

[miguel-heygen]

What changes

A required-check candidate: any PR that changes packages/studio or packages/player must show the behaviour in its description. The body needs a Before and an After section, each with at least one attached image or video (a user-attachments asset, or an image/video link). Only the PR description counts; captures in comments are not read. The failure text names each missing piece and prints the exact gh pr edit --attach command.

A PR may instead add a ## No visible change section, accepted only when the diff under those two packages is under 20 changed lines and touches no .tsx, .css or .html file; the failure says which condition did not hold.

Check name for the ruleset: Studio and player captures (workflow PR captures). It runs on every PR to main, with no path filter, so it can be required; the script passes when neither package changed. It re-runs when the description is edited.

Shape

• scripts/check-pr-captures.mjs: pure parseSections, hasMedia, parseNumstat, evaluate; the diff is read with --numstat -z --no-renames; an unreachable base exits 2.
• Comments, fenced code and setext headings are normalized first so an image in a comment or code block cannot satisfy a section, and an After nested under Before cannot share its image.
• Trigger shape follows ci.yml; body, PR number, base ref and head sha reach the script through env, never interpolated into run. Tests are added to test:scripts.

Gate ownership

.github/CODEOWNERS lists the script, its test and the workflow, so a PR cannot change its own gate without a code owner. Enforcing it needs "Require review from Code Owners" in the ruleset.

Open

• Closed: a .test.tsx or .spec.tsx-only change now counts as non-visual for No visible change (component files still do not).
• Closed: an embedded image (![..](url) or <img src>) counts without a file extension; a plain link to a page still does not.
• Closed: the live Actions run was exercised on this PR (the check passes here because it touches neither package); a fork PR was not exercised.
• Host and comment handling hardened: attachment links are matched on the parsed hostname (github.com) and path, and HTML comments are stripped until none remain, an unclosed one hiding the rest of the body as GitHub does.

What I measured

node --test scripts/check-pr-captures.test.mjs: 29 pass. Run against a real Studio PR description and diff it exits 0; against an empty body it fails with the attach command. A copy of the script under a path with a space still runs (entry guard).

[somanshreddy]

Reviewed at 0798a5a7. Genuinely well-built check — the design and the test suite are both strong. One real red gate (Fallow), and I traced its actual cause because it changes the fix.

Verified sound

• The gate is designed to be safely required. The workflow runs on every PR with no path filter, and the script no-ops when neither packages/studio nor packages/player changed (evaluate returns ok on files.length === 0) — so it can become a required check without blocking unrelated PRs.
• edited re-triggers it. on: pull_request: types: [..., edited] means adding the captures to the body re-runs the check — it sidesteps the "body edits don't re-trigger, you must push" evidence-at-push-time trap that bites the HFI evidence gate.
• It fails loud and reads the right source. assert-free but process.exit(1) on problems / exit(2) when the diff can't be computed (no silent skip); --head $HEAD_SHA + --base origin/$BASE_REF with fetch-depth: 0 diffs correctly regardless of the checked-out merge commit; body comes from github.event.pull_request.body (current on every event).
• The bypass vectors are actually closed and tested. normalize() strips HTML comments + fenced code and converts setext→ATX before matching, and the 26 tests hit the real exported functions (not mocks) across the evasions that matter: media in an HTML comment, in a code fence, in the wrong section, one image shared via a nested ### After, setext headings, qualifier headings (## After: on passes, ## After the merge fails), and ## Before-and-after correctly not matching Before. Binary files spend the full no-visible-change budget. It's wired into CI (package.json adds check-pr-captures.test.mjs to test:scripts), so it runs and can fail.
• Security posture is clean: PR body is regex-matched only, execFileSync runs git with fixed args (no shell), workflow perms are contents: read + pull-requests: read.

Blocking — Fallow audit is RED, and the cause is coverage visibility, not complexity

The Fallow audit check fails with 4 fallow/high-crap-score findings on scripts/check-pr-captures.mjs (evaluate:112 flagged critical, CRAP 156). But the raw cyclomatic complexity of all four is under the threshold — <arrow>:42 = 5, parseNumstat:79 = 6, evaluate:112 = 12, main:137 = 6, threshold 30. CRAP = comp²·(1−coverage)³ + comp, so those scores (30 / 42 / 156 / 42) are exactly the coverage = 0 values; at full coverage they'd be 5 / 6 / 12 / 6, all passing. So fallow is measuring ~0 coverage on this script even though the 26-test suite exercises evaluate/parseNumstat/parseSections/hasMedia heavily — its node --test coverage isn't reaching the audit.

So the fix isn't "refactor evaluate" — it's to get fallow to count check-pr-captures.test.mjs's coverage. If that's not wired for scripts/*.test.mjs in this repo (the sibling check-docs-catalog.mjs carries a // fallow-ignore-next-line complexity, which suggests fallow doesn't see these node:test suites), a scoped suppression matching that precedent is the pragmatic path — with the caveat that main() (the CLI wrapper) is the one function that genuinely isn't unit-covered, so it's the only one where the score reflects real risk rather than a measurement gap. Either way the gate must go green before a stamp.

Also pending, not yet green: Build, Perf: *, CLI smoke, Analyze — normal long-runners, just not landed.

Nit (non-blocking)

When a ## No visible change section is present but doesn't qualify, evaluate appends both the "No visible change" does not apply: … messages and the no "Before"/"After" heading messages. Slightly redundant, but it does tell the author both routes are unsatisfied — leave it if intentional.

Once Fallow is green (coverage counted or a precedent-matching suppression) and the pending checks land, this is a clean approve — the check itself is one of the better-tested gate scripts in this batch.

Codex disabled per workspace cap — manual pass only.

[miguel-heygen]

Independent review findings, and what changed:

• Fence tracking toggled on any three backticks or tildes, ignoring the character and length. Fixed: it now follows CommonMark (same character, at least as long, no text on the closer, at most 3 spaces of indent), with tests.
• Four mutants survived (CRLF, the Before/After title anchor, the media-path anchor, the attachment-path anchor). Added tests; all now fail when broken.
• No visible change accepts any diff under 20 lines with no .tsx, .css or .html, so a small .ts change ships without captures. This is the intended exemption; left as is, pending a maintainer decision on its breadth.
• The check runs from the PR's own copy of the script under pull_request, so a PR can edit the gate. Left as is; a fix means pull_request_target or a CODEOWNERS entry, which is a maintainer call.
• Re-running a red job reuses the original event body; editing the description triggers a fresh run via edited. Noted, not changed.
• No merge_group trigger: add it if a merge queue is enabled.

[somanshreddy]

Re-checked at a71a0b6e. My hold reason is resolved — narrowing to just the content-validating Test job.

• Fallow audit is green now — the coverage-driven CRAP findings I held on (evaluate/parseNumstat/etc. flagged only because fallow measured ~0 coverage) are cleared. Skills: manifest in sync / project-native lint + mirror / Test: runtime contract all pass; zero failure-conclusion checks at this head.
• Still pending: Test (in-progress) — this is the job that runs test:scripts → check-pr-captures.test.mjs, i.e. the PR's own 26 tests. That's the content-validating check for a CI-check PR, so I'll flip to approve once it lands green rather than stamp with it pending. The other pending checks (Analyze, Render on windows, Smoke: global install, studio lanes) are peripheral long-runners.

My substantive review carries: the captures check is well-built — 26 adversarial tests (comment/fence/wrong-section/nested/setext/qualifier bypasses), edited re-trigger, no-op on irrelevant PRs so it can be required, real lint codes. Once Test is green it's a clean approve.

Codex disabled per workspace cap — manual pass only.

[somanshreddy]

Flipping to APPROVE at 08da89dc. My hold reason (the Fallow audit's coverage-driven CRAP findings) is cleared, and the whole run is now green: Fallow, Preflight, Test (runs the 26-test check-pr-captures.test.mjs), and Tests on windows-latest: studio-core all pass — 0 failure conclusions, 0 pending. The only #4131-specific delta since my last pass is the benign CODEOWNERS entry protecting the check's own files (the rest was #4138 rebased in from main). My review carries: 26 adversarial tests, edited re-trigger, no-op on irrelevant PRs so it can be required, real lint codes. Approving.

Codex disabled per workspace cap — manual pass only.