Repository navigation
fix(pr-validation): scope, cancellation and the commit-list cap - #932
Merged
Merged
Conversation
Radagast's High on #926. Neither the approval job nor the preflight carried an in-scope condition; they gated on has-breaking-changes alone. Before #918 that was survivable because out-of-scope normalisation forced the detection to false. #918 started preserving the caller's real result — correctly, so the acknowledgement could still be reported — and removed the only thing keeping Environment work out of scope. A breaking pull request excluded by a branch rule, a skip label or an ignored author therefore waited on an Environment it had no business touching, or failed because that Environment could not be read, and the verdict then discarded the answer and returned success. Scope now gates everything the Environment touches: the approval job, the preflight, the CODEOWNERS lookup that exists to create it, and the reuse lookup. The acknowledgement report keeps running outside scope, which is the thing #918 was right about.
Radagast's High on #926. #912 treated every cancelled approval as supersession. The reasoning was sound — a run can be replaced without the head moving, because editing the description queues a new run on the same commit — but it was too wide. A cancellation with nothing replacing it reported success for a revision nobody approved, while result=failure and approved=false sat in the outputs saying the opposite. Supersession is now established, not assumed: either the head moved on, or a newer run exists for this branch to carry the decision. Neither leaves the cancellation unresolved, which does not pass. Exercised against a stubbed gh: cancelled with no newer run resolves to unresolved; cancelled with a newer run and cancelled with a moved head both resolve to superseded; approval, rejection and moved-head paths are unchanged.
Radagast's High on #926, and the one I would not have found. The commits endpoint returns at most 250 commits, pagination or not. The resolver paginated and trusted the result, so a break living past the cap read as "no breaking change" — and with approval reuse it is worse: an approved break inside the window plus an omitted one outside leaves the fingerprint unchanged, so an approval is honoured for a change it never covered. The returned count is now compared against the pull request's declared total, and a mismatch is refused rather than answered. The caller fails closed on it: detection-succeeded goes false and the verdict blocks. Refusing rather than reconstructing the history is deliberate. A gate that cannot see the whole change should say so; guessing from a subset is how it ends up approving one break while blind to another. The cost is that pull requests above 250 commits cannot be gated automatically, which is the honest answer rather than a confident wrong one. Two cases added. The suite also needed `|| capped_status=$?` on the assignment, since `set -e` takes the script down on a substitution that exits non-zero — the same trap flagged on #915 and sidestepped in `run`, walked into again in a new block.
Contributor
🔍 Lint Analysis
|
lerian-studio
added
size/S
Contributor
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
Contributor
🛡️ CodeQL Analysis ResultsLanguages analyzed: ✅ No security issues found. 🔍 View full scan logs | 🛡️ Security tab |
This comment has been minimized.
This comment has been minimized.
lerian-studio
added
the
review-ready
WalkthroughThe approval workflow now gates approval-related steps by pull-request scope and checks for newer runs before classifying a cancelled approval as superseded. The commit resolver verifies that the returned commit list matches the pull request’s declared commit count. ChangesApproval workflow
Commit enumeration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title is concise and accurately identifies the three main fixes: scope handling, cancellation handling, and the commit-list cap. Description check The description is complete and directly matches the changes. It includes the change summary, type, breaking-change assessment, testing details, and validation results. The caller workflow run was not… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and directly matches the changes. It includes the change summary, type, breaking-change assessment, testing details, and validation results. The caller workflow run was not performed and the Related Issues section is not populated, but both are clearly represented or non-critical.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
bedatty
added a commit
that referenced
this pull request
Oct 19, 2026
`actions/runs?branch=` returns runs of every workflow on the branch, and the siblings from the same push start later with higher ids — so any cancelled approval found a "newer run" and passed. Measured on test/final-bang: a run of Self — Repository Routines sat above the gate's own run id, which restores the exact false success #932 set out to remove. The check now resolves the current run's `workflow_id` and queries that workflow's runs. The test stub answers only the scoped URL, so a regression to the broad query reads as "no newer run" and fails the supersession cases instead of passing them silently. Also documents the 250-commit refusal in the composite README.
lerian-studio
added
size/M
`actions/runs?branch=` returns runs of every workflow on the branch, and the siblings from the same push start later with higher ids — so any cancelled approval found a "newer run" and passed. Measured on test/final-bang: a run of Self — Repository Routines sat above the gate's own run id, which restores the exact false success #932 set out to remove. The check now resolves the current run's `workflow_id` and queries that workflow's runs. The test stub answers only the scoped URL, so a regression to the broad query reads as "no newer run" and fails the supersession cases instead of passing them silently. Also documents the 250-commit refusal in the composite README.
bedatty
force-pushed
the
fix/gate-scope-cancellation-commit-cap
branch
from
October 19, 2026 16:19
8f8632f to
76edec7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A head branch can carry more than one pull request — same branch, different bases — so scoping the query to the gate's own workflow still let another pull request's run stand in for this one's and pass a cancellation as superseded. The filter now also matches the run's `.pull_requests`, falling back to the branch scope when the field comes back empty, which is what runs from forks look like. The `gh` stub now executes the real `--jq` against a real payload. The branch-wide query survived a full round of review because the stub returned the count directly and never ran the filter — the query is the thing under test, so it has to actually run. Verified by removing the filter: test_a_newer_run_of_another_pull_request_is_not_supersession fails.
Contributor
|
@coderabbitai review |
7 of 8 tasks
|
This was referenced Oct 19, 2026
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.
GitHub Actions Shared Workflows
Description
The three High findings from Radagast's review of #926. One commit each.
1. The Environment gate ran outside its own scope
Neither the approval job nor the preflight carried an
in-scopecondition — they gated onhas-breaking-changesalone.Before #918 that was survivable, because out-of-scope normalisation forced the detection to
false. #918 started preserving the caller's real result — correctly, so the acknowledgement could still be reported — and removed the only thing keeping Environment work out of scope.A breaking pull request excluded by a branch rule, a skip label or an ignored author therefore waited on an Environment it had no business touching, or failed because that Environment could not be read, and the verdict then discarded the answer and returned success.
Scope now gates the approval job, the preflight, the CODEOWNERS lookup that exists to create the Environment, and the reuse lookup. The acknowledgement report keeps running outside scope, which is the part #918 got right.
2. A cancellation passed enforcement with nothing replacing it
#912 treated every cancelled approval as supersession. The reasoning was sound — a run can be replaced without the head moving, since editing the description queues a new run on the same commit — but too wide. A cancellation with nothing replacing it reported success for a revision nobody approved, while
result=failureandapproved=falsesat in the outputs saying the opposite.Supersession is now established rather than assumed: either the head moved on, or a newer run exists for this branch to carry the decision. Neither leaves it
unresolved, which does not pass.3. A verdict from a truncated commit list
The commits endpoint returns at most 250 commits, pagination or not. The resolver paginated and trusted what came back.
A break living past the cap read as "no breaking change". With approval reuse it is worse: an approved break inside the window plus an omitted one outside leaves the fingerprint unchanged, so an approval is honoured for a change it never covered.
The returned count is now compared against the pull request's declared total, and a mismatch is refused rather than answered. The caller fails closed on it.
Refusing rather than reconstructing history is deliberate, and it is a trade: pull requests above 250 commits can no longer be gated automatically. That is the honest answer rather than a confident wrong one — but if you would rather have complete-history detection, say so and I will take that route instead.
Type of Change
fix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)Breaking Changes
None for callers — no input, output or job name changes.
Three behaviour changes, all corrections: out-of-scope pull requests no longer wait on the Environment; an unexplained cancellation no longer passes; a pull request above 250 commits now fails the gate instead of being waved through.
Testing
@this-branchor the beta tagVerdict logic exercised against a stubbed
ghfor every cancellation path, plus approval, rejection and moved-head to confirm no regression. The commit cap has two new cases — a truncated list is refused, a complete one is answered — with the refusal verified by mutation.Three existing tests failed and were updated, which is the part worth reading: they pinned
cancelled → supersededunconditionally. They were encoding the bug, not guarding against it.Suites: 95 wiring, 22 + 32 + 22 + 60 shell.
actionlint,composite-schema,shellcheck,typosclean.Related Issues