fix(scm): don't downgrade an UNSTABLE PR to blocked when CI is failing#2404
Open
abhay-codes07 wants to merge 1 commit into
Open
fix(scm): don't downgrade an UNSTABLE PR to blocked when CI is failing#2404abhay-codes07 wants to merge 1 commit into
abhay-codes07 wants to merge 1 commit into
Conversation
The observer mergeability derivation composes state in a documented priority order (doc.go): DIRTY -> conflicting, BLOCKED -> blocked, UNSTABLE -> unstable, then changes_requested / CI failing -> blocked. UNSTABLE (rule 3) is meant to win over changes_requested (5) and CI failing (6): UNSTABLE means the PR IS mergeable but has a failing/pending NON-required check (a required failure yields BLOCKED). Both mergeabilityObservation (adapters/scm/github) and mergeabilityFromProviderFacts (observe/scm) checked draft/CI/review and returned blocked before ever reaching the UNSTABLE branch at the bottom. Because an UNSTABLE PR's status rollup is FAILURE (so ci=failing), the UNSTABLE branch was effectively dead for the common case, and a genuinely mergeable PR was persisted as Mergeability=blocked — contradicting the package spec and the sibling mergeabilityFromGraphQL, which switches on state first. Move the UNSTABLE check above the draft/CI/review blockers in both functions, matching the spec and the GraphQL sibling. Adds a regression test to each package.
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.
Closes #2401.
What
The observer's mergeability derivation downgraded an
UNSTABLEPR toblockedwhenever CI was failing (or changes were requested), even thoughUNSTABLEmeans the PR is mergeable (it has a failing/pending non-required check; a required failure yieldsBLOCKED). BothmergeabilityObservation(adapters/scm/github) and its byte-identical twinmergeabilityFromProviderFacts(observe/scm) checked draft/CI/review and returnedblockedbefore ever reaching theUNSTABLEbranch at the bottom.Why it's wrong
doc.godocuments the priority order (first rule wins):BLOCKED(2) →UNSTABLE(3) → changes_requested (5) → CI failing (6).UNSTABLEmust win over CI-failing and changes-requested. The siblingmergeabilityFromGraphQL(provider.go) already implements this — it switches onstatefirst. Because anUNSTABLEPR's status rollup isFAILURE(→ci = failing), the bottomUNSTABLEbranch was effectively dead for the common case, and a genuinely mergeable PR was persisted asMergeability = blocked.Change
Move the
UNSTABLEcheck above the draft/CI/review blockers in both functions, matchingdoc.goand the GraphQL sibling. Existing behavior is otherwise unchanged.Tests
TestSCMMergeabilityUnstableNotDowngradedByBlockers,TestMergeabilityFromProviderFactsUnstableNotDowngraded) coveringUNSTABLE+ failing CI andUNSTABLE+ changes-requested. Both fail before the change (blocked) and pass after (unstable).go test ./internal/adapters/scm/github/... ./internal/observe/scm/...green;go vetand gofmt clean.-raceleft to CI (no 64-bit CGO locally).cc @harshitsinghbhandari — followed doc.go's stated order and the GraphQL sibling; happy to adjust if a failing CI is intended to always read as blocked.