You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
pstack watch-pr: BLOCKED merge state with green CI reports ready — intentional, or gate it? #731
assessGitHubMerge treats mergeStateStatus: "BLOCKED" with a non-failing rollup as allowed, so a PR that GitHub blocks for a non-CI reason (e.g. missing required review) is classified ready.
gateReason blocks CHANGES_REQUESTED only, so REVIEW_REQUIRED is echoed into the proof but does not prevent ready.
Why I am asking rather than sending a patch
The types and the readiness truth table in policy.test.ts encode BLOCKED + PENDING -> allowed, and ignoring GitHub's merge state in favour of our own rollup looks deliberate. But BLOCKED + SUCCESS is unspecified and inherits allowed from the same Exclude<RollupState, "ERROR" | "FAILURE">, and that row is the one where required review hides.
So: is trusting the rollup over GitHub's merge state intentional?
If yes, then there is a missing truth-table row worth pinning explicitly, and callers need to know that ready can mean "CI clean, non-CI merge gate possibly unmet".
If no, two fixes suggest themselves:
A. Keep the distrust rule, block readiness: when mergeStateStatus === "BLOCKED" and the head rollup is settled (SUCCESS / EXPECTED / null), return a gate blocker — e.g. add "merge-state-blocked" to MergeGateReason and have gateReason report it when ci.kind === "ci-clean" and ci.github.mergeStateStatus === "BLOCKED". ci-clean stays truthful; only readiness changes.
B. Gate required review: block REVIEW_REQUIRED in gateReason exactly like CHANGES_REQUESTED. Smallest change, but it changes behaviour for every PR that merely awaits an approval.
Either way the truth table should gain the missing ["BLOCKED", "SUCCESS", ...] row.
Hey @BowmanStephen, this one is intentional as written: policy.test.ts pins ["BLOCKED", "PENDING", "allowed"] and types.ts models the BLOCKED + green rollup case as basis: "rollup", so ready there means "nothing in CI stands in the way", with required reviews treated as the human's step rather than the watcher's. Whether that should be surfaced differently (a distinct blocked-by-review outcome, or gating ready on mergeStateStatus !== "BLOCKED") is a product call for the pstack owners, so I have flagged the question to them rather than filing it as a defect. If you have a case where the current classification caused a wrong action downstream, that would be the thing to add here.
Activity
deanrie commentedon Oct 20, 2026
Hey @BowmanStephen, this one is intentional as written:
policy.test.tspins["BLOCKED", "PENDING", "allowed"]andtypes.tsmodels theBLOCKED+ green rollup case asbasis: "rollup", soreadythere means "nothing in CI stands in the way", with required reviews treated as the human's step rather than the watcher's. Whether that should be surfaced differently (a distinctblocked-by-reviewoutcome, or gatingreadyonmergeStateStatus !== "BLOCKED") is a product call for the pstack owners, so I have flagged the question to them rather than filing it as a defect. If you have a case where the current classification caused a wrong action downstream, that would be the thing to add here.