Repository navigation
ci: relax visual regression + commit scope checks - #2581
Conversation
Visual regression now skips when all changed files are content-only (markdown anywhere under web/, Astro content collections under web/sites/*/src/content/, free-standing content under web/content/, and static assets under web/sites/*/public/). A new detect-scope job emits the decision; visual-regression gates on it via needs+if. Mixed PRs still run the full regression with the existing blog-baseline pre-refresh path. Drops the commitlint scope-enum allowlist. Scope is still optional, but any value is now accepted — the allowlist produced more friction (wrong scope -> CI block -> amend + force-push) than value (changelog grouping keys off type, not scope). Conventional types and the subject/header rules remain enforced. Updates CLAUDE.md "Commit Message Conventions" and the wheels-bot shared rails so humans and bots get the same loosened guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This PR makes two CI friction reductions: a new detect-scope job that skips visual regression for content-only changes, and removal of the commitlint scope-enum allowlist. Both changes are sensible and well-motivated. The implementation is generally solid, but there is one gap in the stated "safe-by-default" guarantee worth addressing before merge.
Correctness
Job-level failure causes visual-regression to be skipped, not run (.github/workflows/web-deploy.yml, line 183)
needs: detect-scope
if: needs.detect-scope.outputs.content_only != 'true'The PR description claims: "Safe-by-default: any uncertainty in scope detection falls through to 'run the regression.'" The script handles within-script uncertainties correctly — an empty diff, unresolvable SHAs, or an unknown event type all emit content_only=false and exit 0. However, if the detect-scope job itself fails (infra failure, runner OOM, actions/checkout transient error), GitHub Actions' default behavior skips dependent jobs rather than running them. An empty content_only output is enough to satisfy != 'true', but the job-level guard still wins and visual-regression is skipped silently.
On PRs this is recoverable (the failed detect-scope check blocks merge anyway), but on a push to develop it would silently skip regression. If the safety guarantee matters, the fix is:
if: always() && needs.detect-scope.outputs.content_only != 'true'That said, a checkout failure on detect-scope is a runner infrastructure event, and skipping regression in that context is arguably acceptable. Whether to address this is a judgment call — flagging it so the author can decide consciously.
Missing -e in set -uo pipefail (.github/workflows/web-deploy.yml, Compute change scope step)
set -uo pipefailAll other run: blocks in this workflow use set -euo pipefail. The omission of -e here is deliberate — it allows the script to fall through to safe defaults when git diff returns non-zero — but a future maintainer seeing the inconsistency might "fix" it by adding -e and break the fallback behavior. A one-line comment would prevent that:
# -e intentionally omitted: git errors fall through to safe defaults below.
set -uo pipefailDocs
The "When you forget and CI rejects the commit" subsection was removed from CLAUDE.md. The scope-enum drop eliminates the main cause of commitlint failures, so removing the section is reasonable. One minor loss: the git commit --amend -m ... && git push --force-with-lease recipe applies to type/subject violations too, not only scope. Since this is bot documentation as much as human documentation, its absence may mean the bot generates a fix-commit instead of amending on a single-commit PR. Low priority, but worth tracking.
Commits
Single commit: ci: relax visual regression + commit scope checks — valid type (ci), no scope (acceptable), subject 47 chars, not ALL-CAPS. Clean.
Security
The env: routing for all GitHub context values (SHAs, event name) in the detect-scope step is correct. No ${{ }} interpolation inside run: blocks. The blog-baseline commit message is hardcoded ASCII. No concerns.
Verdict: comment — no blocking correctness, cross-engine, or security findings. The job-failure skip gap and the missing -e note are the only items worth deciding on before this merges.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-reasoned. The two findings are real and the verdict of SycophancyNone detected. A's opening summary ("both changes are sensible and well-motivated") is substantiated by the subsequent analysis, not stand-alone praise. Verdict is False positivesNone detected.
Missed issuesNone detected. The diff is CI-and-docs-only. No CFML, no cross-engine concerns, no migration changes, no security surface beyond what A already reviewed ( Verdict alignmentA's ConvergenceAligned. A and I agree on the diagnosis. The two improvements — |
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Tests were not re-run: the changes are CI workflow YAML only and not exercised by The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
Previous ordering ran 'Fail job on real regression' first (which exits 1), which caused Actions to skip the subsequent 'Upload diffs on failure' step even though its 'if:' matched. Net effect: the diagnostic artifact went missing on exactly the runs that needed it. Reorder so the upload runs first, then the explicit-fail step asserts the failure for the check status. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This PR makes two CI friction reductions — a detect-scope job to skip visual regression on content-only changes, and removal of the commitlint scope-enum allowlist — plus a follow-up fix for an upload-artifact ordering bug. The overall implementation is correct and the follow-up commit shows careful attention to GitHub Actions execution semantics.
Correctness
Follow-up commit ordering fix is correct (.github/workflows/web-deploy.yml, lines 301–328)
The second commit (ec03b9aa7) moves "Upload diffs on failure" ahead of "Fail job on real regression." This is correct because:
- The regression step (
id: regress, line 238) usescontinue-on-error: true(line 239). When the regression exits non-zero,steps.regress.outcomebecomes'failure'but the job remains in success state. - With the original ordering (fail before upload), when "Fail job on real regression" ran
exit 1, GitHub Actions flipped the job to failure. Subsequent steps withif: steps.regress.outcome == 'failure'were then skipped because GitHub Actions implicitly prependssuccess()to anyif:expression that does not include an explicit status check function (always(),failure(), etc.). - With the new ordering, "Upload diffs on failure" executes while the job is still in success state, uploads the artifact, then "Fail job on real regression" runs and exits 1.
The comment block added above the upload step accurately describes the reasoning. Fix is mechanically sound.
Job-level failure still silently skips visual-regression (.github/workflows/web-deploy.yml, line 186)
Previously flagged; still applies. The job currently gates as:
needs: detect-scope
if: needs.detect-scope.outputs.content_only != 'true'If detect-scope itself fails (runner OOM, transient checkout error), content_only is empty — which satisfies != 'true' — but GitHub Actions marks dependent jobs as skipped when their dependency job fails, so the regression never runs. On PRs this is visible (the failed check blocks merge). On a push to develop it silently skips regression. The fix is:
if: always() && needs.detect-scope.outputs.content_only != 'true'Non-blocking: a detect-scope infra failure is a runner event, recoverable on re-run. Whether to address this is a judgment call for the author.
Missing -e in set -uo pipefail (.github/workflows/web-deploy.yml, "Compute change scope" step, line 104)
set -uo pipefailEvery other run: block in this workflow uses set -euo pipefail. The omission is intentional — git errors should fall through to safe defaults below — but a future maintainer could "fix" it and break the fallback path. A one-line comment prevents that:
# -e intentionally omitted: git errors fall through to safe defaults below.
set -uo pipefailCommits
cdf0d12f3 ci: relax visual regression + commit scope checks— valid type (ci), no scope (acceptable), 47-char subject, not ALL-CAPS. Clean.ec03b9aa7 ci: upload visual-regression diffs before failing the job— valid type (ci), no scope, 52-char subject. Commit body explains the "why" precisely ("Actions skips subsequent steps even though itsif:matched").
Both commits conform to commitlint.config.js.
Security
All GitHub context values (SHAs, event name) reach the shell via env: block and quoted $VAR access — no ${{ }} interpolation inside run: blocks. The blog-baseline commit message is hardcoded ASCII. No concerns.
Verdict: comment — the follow-up commit is mechanically correct and the whole PR is in good shape. The two open items (job-level skip on detect-scope failure; missing -e annotation) are non-blocking judgment calls, flagged for the author to decide consciously.
Wheels Bot — Reviewer B (round 1)A's review on the current SHA is accurate. Both findings are real and correctly diagnosed. No sycophancy, no false positives, no missed issues. Converging to Note: a prior Address Review round (on SHA SycophancyNone detected. A's opening characterization ("the overall implementation is correct") is earned — the review backs it up with a detailed mechanical analysis of the step-ordering fix and a clear diagnosis of the two remaining gaps. Verdict is False positivesNone detected.
Missed issuesNone detected. The diff is CI workflow YAML plus prose doc updates ( Verdict alignmentA's ConvergenceAligned. A and B agree on both findings and neither disputes the other's analysis. The two concrete changes — add |
The previous blog-post recovery (66827a2) on develop didn't trigger the post-merge auto-refresh, so every subsequent PR was tripping on the stale baseline. Pulling the actual screenshot from this PR's own CI run and committing it as the new baseline clears the trap. This is exactly the failure mode the content-only skip in this PR prevents going forward — once landed, content-only PRs skip the gate entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This PR adds a `detect-scope` job to skip visual regression on content-only changes, drops the commitlint `scope-enum` allowlist, and refreshes the blog baseline that was blocking subsequent PRs. The main workflow and commitlint changes were reviewed at `ec03b9aa7`; this pass covers the new `f756cfaa0` commit (blog baseline refresh) and confirms whether the two previously-flagged items were addressed.
Correctness
New commit (`f756cfaa0`) is correct and well-motivated.
The commit body explains that `develop`'s blog baseline became stale when a prior auto-refresh (`66827a221`) failed to fire post-merge, causing every subsequent PR to trip on the old snapshot. Manually committing the corrected baseline follows the documented process (download the `visual-regression-diffs` artifact → copy `.actual.png` over `web/tests/visual-baselines/.png` → commit). The commit body also correctly notes that once this PR lands, content-only PRs skip the gate entirely, preventing the same failure mode from recurring.
Previously-flagged items remain open.
- Job-level failure silently skips `visual-regression` (`.github/workflows/web-deploy.yml`, `visual-regression` job `if:` condition):
```yaml
if: needs.detect-scope.outputs.content_only != 'true'
```
If `detect-scope` itself fails at the job level (runner OOM, transient checkout error), GitHub Actions marks dependent jobs as skipped rather than running them. An empty `content_only` output satisfies `!= 'true'`, but the dependency failure wins. On PRs this is recoverable (the failed check blocks merge), but on a `push` to `develop` regression is silently skipped. The fix is:
```yaml
if: always() && needs.detect-scope.outputs.content_only != 'true'
```
Still non-blocking; flagged for the author's conscious decision.
- Missing `-e` in `set -uo pipefail` (`.github/workflows/web-deploy.yml`, "Compute change scope" step):
```bash
set -uo pipefail
```
Every other `run:` block in this workflow uses `set -euo pipefail`. The omission is intentional — git errors fall through to safe defaults — but a future maintainer could "fix" it and inadvertently break the fallback path. A one-line comment prevents that:
```bash
-e intentionally omitted: git errors fall through to safe defaults below.
set -uo pipefail
```
Commits
- `cdf0d12f3 ci: relax visual regression + commit scope checks` — `ci` type, no scope, 47-char subject. Conforms to `commitlint.config.js`.
- `ec03b9aa7 ci: upload visual-regression diffs before failing the job` — `ci` type, no scope, 52-char subject. Clean.
- `f756cfaa0 chore(web): refresh blog visual baseline after LuCLI post` — `chore` type, `web` scope (listed as a suggested scope under the new policy), 46-char subject, sentence-case. Clean.
All three commits conform to both the old and new versions of `commitlint.config.js`.
Security
All GitHub context values reach the shell via `env:` block and quoted `$VAR` access — no `${{ }}` interpolation inside `run:` blocks. The baseline update is a binary PNG with no embedded secrets. No concerns.
Verdict: comment — the new commit is routine and clean. The two previously-flagged non-blocking items remain open for the author to decide consciously before merge. No new blocking findings.
Wheels Bot — Address Review (round 2)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1 of the convergence loop on SHA
Note: a prior address-review (round 1, marker on SHA Tests were not re-run: the changes are CI workflow YAML only and not exercised by The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
…eline Two follow-on fixes after the detect-scope grep bug exposed a deeper design issue: 1. The content-only skip in #2581 applied to both PR and push events. For PR events, that's the friction-reducer we wanted. For push events to develop, it silently broke the blog baseline auto-refresh-and-commit dance, which lives inside the visual-regression job. After #2580 merged, the post-merge push couldn't refresh the baseline (detect-scope had crashed; even if it hadn't, content_only=true would have skipped the whole job), so every subsequent non-content PR trips on the stale baseline — exactly what's happening now on this hotfix PR. Fix: tighten the visual-regression `if:` to skip only on PR events with content_only=true. Push events always run, eating the few extra CI minutes so the maintenance contract holds. 2. Refresh blog.png from this PR's CI artifact. The previous baseline was screenshot before #2580 merged and is now 38k pixels off the current develop blog index. Pulling the actual.png from the visual-regression-diffs artifact and committing it here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eline (#2585) Two follow-on fixes after the detect-scope grep bug exposed a deeper design issue: 1. The content-only skip in #2581 applied to both PR and push events. For PR events, that's the friction-reducer we wanted. For push events to develop, it silently broke the blog baseline auto-refresh-and-commit dance, which lives inside the visual-regression job. After #2580 merged, the post-merge push couldn't refresh the baseline (detect-scope had crashed; even if it hadn't, content_only=true would have skipped the whole job), so every subsequent non-content PR trips on the stale baseline — exactly what's happening now on this hotfix PR. Fix: tighten the visual-regression `if:` to skip only on PR events with content_only=true. Push events always run, eating the few extra CI minutes so the maintenance contract holds. 2. Refresh blog.png from this PR's CI artifact. The previous baseline was screenshot before #2580 merged and is now 38k pixels off the current develop blog index. Pulling the actual.png from the visual-regression-diffs artifact and committing it here. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Two CI friction reductions that have been costing us routine recovery commits:
detect-scopejob inspects the diff; if every changed file is markdown (*.md/*.mdx), an Astro content collection (web/sites/*/src/content/**), free-standing content (web/content/**), or a public asset (web/sites/*/public/**), thevisual-regressionjob is skipped entirely. Mixed PRs (content + components/scripts) still run the full regression with the existing blog-baseline pre-refresh path. Safe-by-default: any uncertainty in scope detection falls through to "run the regression."scope-enumrule was producing more friction (wrong scope → CI block → amend + force-push loop) than value (changelogs group by type, not scope). Scope is still optional and the convention is preserved; any value is now accepted. Type / subject / header-length rules unchanged.CLAUDE.md§ "Commit Message Conventions" and.claude/commands/_shared-rails.mdare updated to match so humans andwheels-botget the same loosened guidance.Why now
developshows two(recover failed auto-deploy)commits in the last week from the blog-baseline auto-refresh racing on push. Skipping the regression entirely for content-only changes removes the class of failure.Files
.github/workflows/web-deploy.yml— newdetect-scopejob;visual-regressiongates onneeds.detect-scope.outputs.content_only != 'true'; old inline blog-content detection consolidated into the new job.commitlint.config.js—scope-enumremoved; everything else unchanged.CLAUDE.md— "Commit Message Conventions" section rewritten to reflect optional/unrestricted scope; "Suggested scopes (not enforced)" replaces the old allowlist + path-mapping table + invalid-scopes / forget-and-amend sections..claude/commands/_shared-rails.md— same loosening, restated forwheels-bot.Safety notes
detect-scopedefaults tocontent_only=falseon any uncertainty (event type unknown, diff range unresolvable, empty diff, git error). The only way to skip visual regression is for the job to succeed and explicitly determine every changed file is content. No silent skips on detection failure.env:block + quoted$VARaccess — no${{ }}interpolation insiderun:blocks (per the workflow-injection guidance).Test plan
web/sites/guides/src/content/) and confirmVisual regressionshows as skipped on the checks list..mdxchange + a token tweak inweb/sites/landing/src/pages/index.astroand confirmVisual regressionstill runs.ci:subject; if it lands cleanly through CI, the scope-enum removal is validated end-to-end.🤖 Generated with Claude Code