fix(provider-verifier): make the gate always report, so it CAN be required - #2631
Merged
Merged
Conversation
…uired Prerequisite for adding `verifier-gate` to branch protection. Adding it first would have blocked every PR in the repository. GitHub is explicit about this: "If a workflow is skipped due to path filtering, branch filtering or a commit message, then checks associated with that workflow will remain in a Pending state. A pull request that requires those checks to be successful will be blocked from merging." "You should not use path or branch filtering to skip workflow runs if the workflow is required." -- docs.github.com, Troubleshooting required status checks provider-verifier.yml had on.pull_request.paths with 4 patterns. Required, that means every PR touching none of them sits on "Waiting for status to be reported" forever -- the same repo-wide block #2623's cross-workflow `needs:` would have caused, by a different route. The distinction that makes the fix work is in the same doc: a skipped WORKFLOW never reports, but a skipped JOB reports success. So the path decision moves from the trigger into the job. The job now always runs and always reports; a `changed` step diffs base..head and sets an output, and the working steps carry `if: steps.changed.outputs.relevant == 'true'`. Path detection is a plain `run:` step, not dorny/paths-filter -- that action is not on this repo's Actions allowlist (github_owned_allowed plus 18 named patterns), so it would fail at setup. Verified the matching logic against the cases that matter: Pmoves-MiniMax-Provider-Verifier/verify.py -> true Pmoves-MiniMax-Provider-Verifier/src/a/b.py -> true (** semantics) pmoves/tools/provider_verifier_gate.py -> true .github/workflows/provider-verifier.yml -> true README.md -> false README.md + the gate helper together -> true pmoves/tools/provider_verifier_gate_helper.py -> false (near-miss) NOT DONE HERE, and it must come after this lands: adding `verifier-gate` to the required list. Sequencing matters -- required-then-fix blocks the repo, fix-then- required does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 76c1faa2bc
ℹ️ 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".
Two tests asserted on.pull_request.paths, which this PR removes. Rewritten
rather than deleted -- the guarantee they encoded is still the right one, it
just moved from the trigger into the job:
test_workflow_paths_filter_covers_submodule -> test_job_condition_covers_submodule
test_workflow_paths_filter_covers_helper -> test_job_condition_covers_helper
Both now assert the `changed` step's case arms, so a future edit that drops
submodule or helper coverage still fails. Plus two new ones:
test_workflow_has_no_paths_filter
the removal is now itself asserted, with GitHub's reasoning quoted, so
nobody reintroduces a paths: filter on a workflow meant to be required.
test_working_steps_are_gated_on_the_condition
both halves matter: unconditional steps would run the gate on every PR in
the repo, and a conditional JOB would stop reporting. The shape is a job
that always runs with work that does not.
20 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ow diff
The first detection attempt used `git diff base..head` with `HEAD~1` as
fallback. Checkout is fetch-depth 1, so neither resolves — the run died with:
fatal: ambiguous argument 'HEAD~1': unknown revision or path not in the
working tree
##[error]Process completed with exit code 128
Deepening the clone would mean fetching full history for a monorepo with 72
submodules to answer a question the PR files API answers in one call. Switched
to `gh api repos/\{owner}/\{repo}/pulls/N/files`, with `pull-requests: read`
added alongside the existing `issues: write`.
Fails OPEN TOWARD RUNNING: no PR context, or an API call that returns nothing,
sets relevant=true. An unanswerable question runs the gate rather than silently
skipping it — the opposite default would be a gate that reports success because
it could not tell whether it applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 19, 2026
POWERFULMOVES
added a commit
that referenced
this pull request
Aug 20, 2026
…out (#2641) #2631 made this gate always report, which is what lets it become a required check. Measuring an actual skip run showed the cost of doing that: run 32294521610, PR #2634 (compose files only) Set up job 19:42:46 -> 19:42:47 1s Checkout repo (with submodules) 19:42:47 -> 19:52:02 9m15s Does this PR touch the verifier ... 19:52:02 -> 19:52:04 2s Set up Python skipped Install the verifier's runtime deps skipped Run the static gate skipped Post PR comment on FAIL skipped Nine minutes of `submodules: recursive` across 72 submodules, then two seconds to conclude the PR was irrelevant, then nothing. As a REQUIRED check that is paid by every pull request in the repo, and almost none of them touch the verifier surface. The detection step reads no working-tree state. It asks the PR files API and names the repo explicitly via github.repository, so it runs fine on a bare runner. Moved it to first, and gated the checkout on its result. Ordering is now: detect (ungated) -> checkout -> python -> gate -> comment, with every step after the first carrying the same relevance condition. A non-relevant PR should now finish in seconds instead of ~9.5 minutes, with the same reported conclusion. test_detection_runs_before_checkout encodes this so it cannot silently regress: it asserts detect precedes checkout, that the checkout IS conditional (otherwise the cost is unchanged), and that the detection step is NOT conditional (it is what decides). Verified it can fail -- against the previous ordering it reports "found detect at 1 and checkout at 0". 21 passed. Also removes a duplicated comment paragraph left behind by #2631. Co-authored-by: Claude Opus 5 <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
1 participant
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.
Prerequisite for making
verifier-gatea required check. Adding it first would have blocked every PR in the repository.Mavis proposed adding
Provider Verifier Gate / verifier-gateto branch protection. I checked that against GitHub's own guidance before doing it, and the proposal as written breaks the repo.What the docs say
provider-verifier.ymlhadon.pull_request.pathswith four patterns. Required, that means every PR touching none of them sits on "Waiting for status to be reported" forever — the same repo-wide block #2623's cross-workflowneeds:would have caused, arrived at by a different route.The fix
The distinction that makes it work is in the same doc: a skipped workflow never reports, but a skipped job reports success.
So the path decision moves from the trigger into the job. The job always runs and always reports; a
changedstep diffsbase..headand sets an output, and the working steps carryif: steps.changed.outputs.relevant == 'true'.Detection is a plain
run:step rather thandorny/paths-filter— that action is not on this repo's Actions allowlist (github_owned_allowedplus 18 named patterns), so it would fail at setup, which is the failure mode #2622 was about.Verified
Sequencing
Adding the required check comes after this lands. Required-then-fix blocks the repo; fix-then-required does not. Same shape as the JuiceFS cutover in #2613 — the mitigation has to be in place before the thing that depends on it.