ci-host: reject Percy build when a host-test shard fails#4638
Open
ci-host: reject Percy build when a host-test shard fails#4638
Conversation
Percy auto-approve is enabled on `main`, so a build that finalises without the snapshots from a crashed shard would otherwise lock in a partial baseline — and the next clean build would surface those missing snapshots as "new" diffs on PRs. Capture the build URL/ID from `percy build:finalize`, and follow up with `percy build:reject <id>` whenever `needs.host-test.result` is anything other than `success`. The reject step is gated on the finalize step having actually emitted a build URL so we don't try to reject an empty string when finalize itself fails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3115f31ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`set -e` alone does not fail a step on non-final pipeline elements, so a `percy build:finalize` failure piped through `tee` would have its exit code masked by `tee`'s success. Pre-this-change, the bare `npx percy build:finalize` invocation failed the step immediately; this restores that behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Two temporary changes to verify the new reject-on-failure flow in this PR's own CI run, both clearly marked TEMPORARY (PR #4638): 1. check-percy unconditionally sets percy_needed=true so Percy runs on this workflow-only PR (no UI-relevant files would otherwise trigger it). 2. Shard 1 exits 1 immediately after the test step, so Percy has real snapshots from shards 1-20 but the matrix result is failure. Both blocks must be removed before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts the Host CI workflow to prevent Percy from auto-approving an incomplete baseline when one or more host-test shards fail, by finalizing the Percy build, extracting its build ID, and rejecting the build on shard failure.
Changes:
- Capture Percy build URL/ID from
percy build:finalizeoutput and expose it as step outputs. - Add a follow-up
percy build:reject <build_id>step whenneeds.host-test.resultis notsuccess. - Introduce temporary workflow modifications to force Percy on and to force a shard failure (intended for verification only).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
+44
| # TEMPORARY (PR #4638): force Percy on so the reject-on-failure | ||
| # step has a real build to reject. Remove this block before merge. | ||
| echo "percy_needed=true" >> "$GITHUB_OUTPUT" | ||
| echo "TEMP: Forcing Percy on for reject-on-failure verification" | ||
| exit 0 | ||
|
|
Comment on lines
+220
to
+228
| # TEMPORARY (PR #4638): deliberately fail shard 1 after Percy has | ||
| # already received its snapshots, so the reject-on-failure step in | ||
| # host-percy-finalize has something to reject. Remove before merge. | ||
| - name: TEMP — force shard 1 failure to exercise Percy reject path | ||
| if: matrix.shardIndex == 1 | ||
| run: | | ||
| echo "::warning::TEMP: failing shard 1 deliberately to verify the percy build:reject step" | ||
| exit 1 | ||
|
|
Comment on lines
+342
to
+343
| BUILD_URL=$(grep -oE 'https://percy\.io/[A-Za-z0-9_-]+/[A-Za-z0-9_-]+/builds/[0-9]+' /tmp/percy-finalize.log | tail -1) | ||
| BUILD_ID="${BUILD_URL##*/}" |
The Percy build URL has three path segments between percy.io/ and /builds/ (org/product/project), not two — the previous regex matched nothing and the grep failure propagated through pipefail to fail the whole finalize step before the reject step could run. Allow 1–3 trailing segments in the regex and append `|| true` to the extraction so a parse failure no longer kills the step. Emit a `::warning::` if the build ID can't be parsed; the reject step is already gated on a non-empty build_id, so it'll skip cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Percy auto-approve is enabled on
main, so a build that finalises without the snapshots from a crashed shard would otherwise lock in a partial baseline — and the next clean build would surface those missing snapshots as "new" diffs on PRs. Host shards do crash periodically (OOM, ChunkLoadError before the retry catches it, etc.), so this is a real source of noise now that the branch filter matches.Capture the build URL/ID from
percy build:finalize, and follow up withpercy build:reject <id>wheneverneeds.host-test.resultis anything other thansuccess. The reject step is gated on finalize having actually emitted a build URL so we don't try to reject an empty string when finalize itself fails.Follow-up to #4518 (CS-10954).
Verification I'd appreciate
percy build:finalizestdout via grep — if Percy CLI changes its log format this becomes brittle. There's no documented "give me the build ID" flag onbuild:finalize. An alternative would be queryingGET /api/v1/builds?filter[parallel_nonce]=...but that endpoint isn't documented either. Open to a cleaner discovery path.percy build:rejectexists in@percy/cli(verified via--help) but I haven't manually tested whether it reliably prevents auto-approve. If it doesn't,build:deleteis a more aggressive alternative.Test plan
🤖 Generated with Claude Code