Repository navigation
fix(pr-validation): keep the breaking-change label after the approval - #919
Conversation
The label was being removed the moment the change was approved, which
gets it backwards: `breaking-change` describes the commits, and the
break does not stop being a break because somebody agreed to it. The
repository's own labels.yml says as much — "Callers must update their
workflow configuration after this change" is a fact about the change,
not a step in a workflow. Removing it erased the record exactly when it
started to matter: now the break is both real and agreed.
Two labels now, expiring at different times:
breaking-change a fact. Applied throughout, removed only
when the breaking commits are gone
awaiting-breaking-approval a state. On while a decision is pending,
off the moment one is made, either way
Found while implementing this: the cleanup for the fact label did not
exist at all. An earlier rewrite of the verdict job dropped it, so a
reword that removed the `!` left `breaking-change` on the pull request
for good. Restored, and now removing both.
awaiting-breaking-approval added to .github/labels.yml. Both are
cosmetic — a repository that never declared them still gets its gate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe breaking-change approval flow now uses a persistent ChangesBreaking-change labels
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (3 skipped: 3… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary fix: preserving the breaking-change label after approval. Description check The description explains the behavior change, affected labels, migration impact, testing, and validation results. It does not include a caller workflow run or related issue, and it marks breaking chan… 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 explains the behavior change, affected labels, migration impact, testing, and validation results. It does not include a caller workflow run or related issue, and it marks breaking changes as none despite documenting a migration case for callers that use label as a temporary marker. These are non-critical because the description is otherwise complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (3 skipped: 3 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.
lerian-studio
added
size/S
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
🔍 Lint Analysis
|
🛡️ 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
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.
From the CodeRabbit review on #919. The label test asserted on the composite's text, which cannot say which state made which call. Moving `--add-label "$LABEL"` into the `waiting` branch would have left every string in place and stopped the fact label from surviving a decision — the one thing this PR exists to fix. `run_reporter_labels(state)` executes the reporter with `gh` stubbed and returns the `gh pr edit` calls. Each state is checked for the fact label being added and never removed, and for the pending label being added only while waiting. Verified by that exact mutation: three subtests fail. Worth saying plainly: I ran this check by hand while implementing, and pasted its output as evidence. Running it once and versioning it are not the same thing, and only the second survives the next refactor. Also documents the behaviour change for callers who set `label` as a temporary waiting marker — move it to `pending_label` and clear `label`. Callers on the default need nothing.
lerian-studio
added
size/M
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
GitHub Actions Shared Workflows
Description
The gate removed the
breaking-changelabel the moment the change was approved. That is backwards: the label describes the commits, and the break does not stop being a break because somebody agreed to it.This repository's own
.github/labels.ymlalready says as much:That is a fact about the change, not a step in a process. Removing it on approval erased the record exactly when it started to matter — the break is now both real and agreed — and a search for
label:breaking-changemonths later would miss every PR that made it through the gate.Two labels, expiring at different times
breaking-changeawaiting-breaking-approvalVerified state by state against the composite:
A second bug, found while implementing this
The cleanup for the fact label did not exist at all. An earlier rewrite of the verdict job dropped it, so a reword that removed the
!leftbreaking-changeon the pull request permanently — the one moment the label genuinely should come off was the one moment nothing happened.Restored, now removing both labels, with a test pinning it to the
state=nonepath.Type of Change
feat: New workflow or new input/output/step in an existing workflowfix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)perf: Performance improvement (e.g. caching, parallelism, reduced steps)refactor: Internal restructuring with no behavior changedocs: Documentation only (README, docs/, inline comments)ci: Changes to self-CI (workflows under.github/workflows/that run on this repo)chore: Dependency bumps, config updates, maintenancetest: Adding or updating testsBREAKING CHANGE: Callers must update their configuration after this PRBreaking Changes
None.
pending_labelis a new optional input with a default;labelkeeps its name and default and only changes when it is removed.Repositories that have not declared
awaiting-breaking-approvalsimply do not get it — labelling is cosmetic throughout and failing to apply one never fails the job. It is added to this repository's.github/labels.ymlhere.Testing
@this-branchor the beta tagLabel behaviour exercised against a stubbed
ghacross all five states — the table above is that run's output.Two tests added: the fact label is never removed by the reporter (no
--remove-label "$LABEL"exists in it), and both labels come off on thestate=nonepath, which is the only place the fact stops being true.actionlint,composite-schema,typosclean; full wiring suite green.Note on provenance: this commit was written on the
feat/one-breaking-change-commentbranch while #918 was merged out from under it, so it ended up pushed with no open PR. It is cherry-picked here unchanged — the four touched files are byte-identical to what was validated there.Related Issues