From 3f8bdde9c6e3df94457a55fb6ef85bd4fa0e692c Mon Sep 17 00:00:00 2001 From: omartelo Date: Fri, 31 Jul 2026 10:50:10 -0300 Subject: [PATCH 1/2] feat: merge past a rule that is holding a pull request back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ruleset can hold a pull request that has nothing wrong with it. Measured on a real one: approved, three checks passed, MERGEABLE, no conflicts, and mergeStateStatus BLOCKED — because the repository requires commit messages to match a Conventional Commits pattern, and the message the squash would write does not. Nothing on the pull request says that, and nothing lich showed came close: the toast said "it has conflicts or an unmet requirement", so the reading was conflicts a pull request did not have. There was also nothing to do about it from here. gh checks mergeStateStatus itself and refuses before it ever calls GitHub, so the "edit message" flow — the one thing that would fix a message-pattern rule — died at the same client-side check as the plain merge. Three changes, smallest first. gh writes one sentence for two very different failures and puts the difference in its tail. gherror.go matched only "not mergeable" and claimed conflicts either way, which is the wrong half of the time. Match the tails: "the merge commit cannot be cleanly created" is a conflict, "the base branch policy prohibits the merge" is a rule. The bare match stays last for a wording neither one catches. BranchRules already fetched the whole ruleset payload and threw away everything but allowed_merge_methods. The commit-scoped pattern rules are in that same answer, so keep them: message, author email and committer email, with the operator and the pattern each one tests. Where GitHub answers BLOCKED and one exists, the Merge button's hover and the failure name what the base branch requires. Branch and tag name patterns are left out on purpose — they gate a push, never a merge, and naming them would send the reader after the wrong rule. It is a note, never a verdict. GitHub publishes no "why is this BLOCKED", so the honest answer is where to look. Re-running the pattern here would be a guess dressed as an answer, in a regex dialect that is not the one GitHub evaluates. And the merge itself takes --admin, GitHub's own administrator override — which is also the only way gh calls GitHub at all once a rule holds the merge, so a real rule violation finally reaches the screen. The menu offers it only on BLOCKED and BEHIND, which is gh's own allowsAdminOverride: an override skips a rule, it does not resolve a conflict or finish a draft. Where it is offered, the ordinary items are gone rather than shown beside it — gh has already refused those from the client, so a menu carrying both would put the dead click next to the working one. Without the privilege the dead click stays, because it is what carries gh's refusal and the rule note back. Whether the account may bypass is read, not assumed: `gh repo view --json viewerPermission`, and only ADMIN counts. This is the one answer in the merge gate that fails closed. Everything else there would rather offer an option GitHub turns down than drop one it would have taken, because a refusal at least says what happened — but a bypass reads as a privilege you hold, so one that is not there turns GitHub's refusal into what looks like a broken button. A ruleset can name any role as a bypass actor, so an account that may bypass without administering the repository does not get the option; reading each ruleset's bypass list is the upgrade path. A failed permission read costs the bypass and nothing else — the rules are worth drawing on their own. The tests that changed did so because the contract did: mergeArgs took a parameter, and the gherror case asserted the single sentence that no longer exists. fakeGH grew per-call answers and a call history, which the two-call read needs. Not smoke-tested in task dev: seeing the bypass menu needs a pull request sitting at BLOCKED, and this repository has none. The parser was run against the real ruleset payload that prompted this. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 27 ++++ .../src/components/pulls/PullRequestView.tsx | 39 +++-- frontend/src/lib/api-types.ts | 20 +++ frontend/src/lib/pulls/merge-gate.test.ts | 134 +++++++++++++++++- frontend/src/lib/pulls/merge-gate.ts | 64 ++++++++- frontend/src/lib/rpc.ts | 8 +- internal/project/branchrules.go | 94 +++++++++++- internal/project/branchrules_test.go | 92 +++++++++++- internal/project/gherror.go | 16 +++ internal/project/gherror_test.go | 19 ++- internal/project/pr.go | 22 ++- internal/project/pr_test.go | 61 ++++++-- 12 files changed, 545 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af2f43c6f..fad618b20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **Merge past a rule that is holding a pull request back.** Once GitHub answers + that a rule on the base branch stands in the way, gh refuses the merge without + ever calling GitHub — so there was nothing lich could do with that pull request + but leave for the browser. The Merge menu now offers the same merge as a + bypass, GitHub's own administrator override, and says so in every label it + offers. It appears only where an override can actually help — a rule holding + the merge, or a base branch that has moved — never over a conflict or a draft, + which no override resolves, and never for an account that does not administer + the repository, so a bypass on offer is one that will go through. + +### Changed + +- **A refused merge says which kind of refusal it was.** gh writes one sentence + for two very different failures and puts the difference in its tail; lich read + only the opening and claimed conflicts either way, sending you to look for + conflicts a pull request did not have. A conflict now says so, and a rule on + the base branch says that instead. +- **A merge blocked by a commit rule names the rule.** An approved pull request + with green checks and no conflicts could still be refused, with nothing on + screen accounting for it: the cause was a ruleset testing the *commit message* + the merge would write, which no field of a pull request mentions. Where the + base branch carries one, the Merge button and the failure now name what it + requires — the message, author or committer email patterns, and what each one + expects. + ## [0.23.0] - 2026-07-30 ### Added diff --git a/frontend/src/components/pulls/PullRequestView.tsx b/frontend/src/components/pulls/PullRequestView.tsx index deb5c57cf..11cff6afe 100644 --- a/frontend/src/components/pulls/PullRequestView.tsx +++ b/frontend/src/components/pulls/PullRequestView.tsx @@ -25,7 +25,12 @@ import { subscribePendingReview, } from "@/lib/pulls/pending-review-store" import { conversationCount, conversationTimeline } from "@/lib/pulls/conversation-timeline" -import { allowedMergeMethods, mergeBlockedReason } from "@/lib/pulls/merge-gate" +import { + allowedMergeMethods, + canAdminOverride, + mergeBlockedReason, + mergeRuleNote, +} from "@/lib/pulls/merge-gate" import { useBranchRules } from "@/lib/pulls/use-branch-rules" import { cn, errorText } from "@/lib/utils" import { Markdown } from "@/components/Markdown" @@ -128,15 +133,20 @@ export function PullRequestView({ const blocked = mergeBlockedReason(detail) const methods = allowedMergeMethods(rules) const editableMethods = methods.filter((method) => method !== "rebase") + const ruleNote = mergeRuleNote(detail, rules) + const bypass = canAdminOverride(detail, rules) - const merge = async (method: MergeMethod, subject = "", body = "") => { + const merge = async (method: MergeMethod, subject = "", body = "", admin = false) => { setMerging(true) try { - await ProjectService.MergePullRequest(path, detail.number, method, subject, body) + await ProjectService.MergePullRequest(path, detail.number, method, subject, body, admin) setEdit(null) onMerged() } catch (err: unknown) { - toast.error(`Merge failed: ${errorText(err)}`) + // gh's refusal names a kind of cause, never the rule behind it; the note + // is the only thing on screen that can, so it rides along when there is + // one to say. + toast.error([`Merge failed: ${errorText(err)}`, ruleNote].filter(Boolean).join(" ")) } finally { setMerging(false) } @@ -281,11 +291,11 @@ export function PullRequestView({ blocking, the same spot carries what would make you think twice — red CI that no rule requires, which GitHub merges over without a word. */} - + + } /> + {/* Every item bypasses, or none does — never a menu offering + both. Once GitHub answers BLOCKED or BEHIND gh refuses a + plain merge from the client without ever calling GitHub, so + where the bypass is available the ordinary items are already + dead and keeping them would put the click that cannot work + beside the one that can. Without the privilege the dead + click stays, on purpose: it is what carries gh's refusal and + the rule note back (merge-gate.ts). */} {methods.map((method) => ( - void merge(method)}> + void merge(method, "", "", bypass)} + > {MERGE_LABELS[method]} + {bypass && ", bypass rules"} ))} {/* Rebase replays the branch's own commits, so gh takes no @@ -316,6 +338,7 @@ export function PullRequestView({ onClick={() => setEdit(mergeEditFor(method, detail))} > {MERGE_LABELS[method]}, edit message… + {bypass && " (bypasses rules)"} ))} @@ -442,7 +465,7 @@ export function PullRequestView({ merging={merging} onChange={setEdit} onCancel={() => setEdit(null)} - onConfirm={() => void merge(edit.method, edit.subject, edit.body)} + onConfirm={() => void merge(edit.method, edit.subject, edit.body, bypass)} /> )} diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 11805e4f1..84adbe384 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -43,6 +43,26 @@ export interface BranchRules { * about it", which is also what an unreadable answer reports — so an empty * list must always widen the menu, never empty it. */ allowedMergeMethods: string[] | null + /** The rules that read a commit's own text. Empty means "no rule of this + * kind" — never "nothing blocks this merge". */ + commitPatterns: CommitPattern[] | null + /** This account administers the repository, and so may merge past the rules + * above. The one field here that fails closed: anything short of a clear yes + * is false, so the bypass is hidden rather than offered and refused. */ + viewerCanBypass: boolean +} + +/** internal/project.CommitPattern — one ruleset rule testing the text of a + * commit rather than the state of a pull request. The one class of rule + * nothing on a pull request hints at, and so the one worth naming on screen. */ +export interface CommitPattern { + /** What the rule reads: "message" | "author email" | "committer email". */ + target: string + /** GitHub's spelling: regex | starts_with | ends_with | contains. */ + operator: string + pattern: string + /** The pattern is what the text must *not* be. */ + negate: boolean } /** internal/project.CheckItem — one check of the PR's rollup, for the Checks tab. */ diff --git a/frontend/src/lib/pulls/merge-gate.test.ts b/frontend/src/lib/pulls/merge-gate.test.ts index 8b90885f3..2f72a49d2 100644 --- a/frontend/src/lib/pulls/merge-gate.test.ts +++ b/frontend/src/lib/pulls/merge-gate.test.ts @@ -1,6 +1,20 @@ import { describe, expect, it } from "vitest" -import { allowedMergeMethods, mergeBlockedReason } from "./merge-gate" -import type { PullRequestDetail } from "@/lib/api-types" +import { + allowedMergeMethods, + canAdminOverride, + mergeBlockedReason, + mergeRuleNote, +} from "./merge-gate" +import type { BranchRules, PullRequestDetail } from "@/lib/api-types" + +const rules = (over: Partial = {}): BranchRules => ({ + allowedMergeMethods: null, + commitPatterns: null, + viewerCanBypass: false, + ...over, +}) + +const admin = (over: Partial = {}) => rules({ viewerCanBypass: true, ...over }) const detail = (over: Partial = {}): PullRequestDetail => ({ number: 130, @@ -116,11 +130,11 @@ describe("mergeBlockedReason", () => { describe("allowedMergeMethods", () => { it("narrows the menu to what the base branch's ruleset accepts", () => { - expect(allowedMergeMethods({ allowedMergeMethods: ["squash"] })).toEqual(["squash"]) + expect(allowedMergeMethods(rules({ allowedMergeMethods: ["squash"] }))).toEqual(["squash"]) }) it("keeps the menu's own order, not the ruleset's", () => { - expect(allowedMergeMethods({ allowedMergeMethods: ["rebase", "squash"] })).toEqual([ + expect(allowedMergeMethods(rules({ allowedMergeMethods: ["rebase", "squash"] }))).toEqual([ "squash", "rebase", ]) @@ -134,9 +148,115 @@ describe("allowedMergeMethods", () => { // The call failed, or has not answered yet. expect(allowedMergeMethods(null)).toEqual(all) // The branch has no ruleset, or one that says nothing about merging. - expect(allowedMergeMethods({ allowedMergeMethods: [] })).toEqual(all) - expect(allowedMergeMethods({ allowedMergeMethods: null })).toEqual(all) + expect(allowedMergeMethods(rules({ allowedMergeMethods: [] }))).toEqual(all) + expect(allowedMergeMethods(rules({ allowedMergeMethods: null }))).toEqual(all) // A ruleset naming only methods this build cannot ask gh for. - expect(allowedMergeMethods({ allowedMergeMethods: ["fast_forward"] })).toEqual(all) + expect(allowedMergeMethods(rules({ allowedMergeMethods: ["fast_forward"] }))).toEqual(all) + }) +}) + +describe("canAdminOverride", () => { + // The two gh itself allows --admin on; anything else and gh refuses the flag + // as well, so offering it would trade one dead click for another. + it("offers the bypass on the states GitHub lets an administrator past", () => { + expect(canAdminOverride(detail({ mergeStateStatus: "BLOCKED" }), admin())).toBe(true) + expect(canAdminOverride(detail({ mergeStateStatus: "BEHIND" }), admin())).toBe(true) + }) + + // The rest of this module widens on an unknown; this one hides. A bypass + // reads as a privilege you hold, so offering one that is not there turns + // GitHub's refusal into what looks like a broken button. + it("hides the bypass from an account that does not administer the repository", () => { + const blocked = detail({ mergeStateStatus: "BLOCKED" }) + expect(canAdminOverride(blocked, rules())).toBe(false) + // The rules have not answered yet, or the read failed. + expect(canAdminOverride(blocked, null)).toBe(false) + }) + + it("does not offer it where nothing is in the way", () => { + expect(canAdminOverride(detail(), admin())).toBe(false) + expect(canAdminOverride(detail({ mergeStateStatus: "UNSTABLE" }), admin())).toBe(false) + }) + + // An override skips a rule; it does not resolve a conflict or finish a draft. + it("does not offer it for what an override cannot fix", () => { + expect( + canAdminOverride(detail({ mergeStateStatus: "DIRTY", mergeable: "CONFLICTING" }), admin()), + ).toBe(false) + expect(canAdminOverride(detail({ isDraft: true, mergeStateStatus: "BLOCKED" }), admin())).toBe( + false, + ) + expect( + canAdminOverride(detail({ state: "MERGED", mergeStateStatus: "BLOCKED" }), admin()), + ).toBe(false) + }) +}) + +describe("mergeRuleNote", () => { + const pattern = { + target: "message", + operator: "regex", + pattern: "^(feat|fix): .+", + negate: false, + } + + // The case the note exists for: approved, green, no conflict, still BLOCKED. + it("names the commit rule governing the base branch", () => { + expect( + mergeRuleNote( + detail({ mergeStateStatus: "BLOCKED", reviewDecision: "APPROVED", baseRefName: "develop" }), + rules({ commitPatterns: [pattern] }), + ), + ).toBe("A ruleset governs develop: every commit message must match ^(feat|fix): .+.") + }) + + it("reads a negated rule as the prohibition it is", () => { + expect( + mergeRuleNote( + detail({ mergeStateStatus: "BLOCKED" }), + rules({ + commitPatterns: [ + { target: "author email", operator: "ends_with", pattern: "@ex.com", negate: true }, + ], + }), + ), + ).toBe("A ruleset governs main: every commit author email must not end with @ex.com.") + }) + + it("names every rule when the branch carries more than one", () => { + const note = mergeRuleNote( + detail({ mergeStateStatus: "BLOCKED" }), + rules({ + commitPatterns: [ + pattern, + { target: "committer email", operator: "contains", pattern: "@", negate: false }, + ], + }), + ) + expect(note).toContain("message must match ^(feat|fix): .+") + expect(note).toContain("committer email must contain @") + }) + + // An operator GitHub adds after this build shipped still gets its pattern on + // screen — a slightly wrong verb beats no note at all. + it("falls back to a verb for an operator it has never seen", () => { + expect( + mergeRuleNote( + detail({ mergeStateStatus: "BLOCKED" }), + rules({ commitPatterns: [{ ...pattern, operator: "SOMETHING_NEW" }] }), + ), + ).toContain("message must match ^(feat|fix): .+") + }) + + it("says nothing when GitHub is not holding the merge back", () => { + expect(mergeRuleNote(detail(), rules({ commitPatterns: [pattern] }))).toBeNull() + }) + + // A rule note is not a verdict: no rule of this kind never means "unblocked". + it("says nothing when no rule of this kind is known", () => { + const blocked = detail({ mergeStateStatus: "BLOCKED" }) + expect(mergeRuleNote(blocked, null)).toBeNull() + expect(mergeRuleNote(blocked, rules())).toBeNull() + expect(mergeRuleNote(blocked, rules({ commitPatterns: [] }))).toBeNull() }) }) diff --git a/frontend/src/lib/pulls/merge-gate.ts b/frontend/src/lib/pulls/merge-gate.ts index f2866c9b9..543145538 100644 --- a/frontend/src/lib/pulls/merge-gate.ts +++ b/frontend/src/lib/pulls/merge-gate.ts @@ -1,4 +1,4 @@ -import type { BranchRules, MergeMethod, PullRequestDetail } from "@/lib/api-types" +import type { BranchRules, CommitPattern, MergeMethod, PullRequestDetail } from "@/lib/api-types" // Every method lich knows how to ask gh for, in the order the menu offers them. const EVERY_METHOD: MergeMethod[] = ["squash", "merge", "rebase"] @@ -65,3 +65,65 @@ export function mergeBlockedReason(detail: PullRequestDetail): string | null { } return null } + +// The merge states an administrator override gets past, which is gh's own +// allowsAdminOverride — pass --admin on anything else and gh refuses from the +// client anyway. A conflict is absent on purpose: the override skips a rule, it +// does not compute a merge that has no answer. +const OVERRIDABLE = ["BLOCKED", "BEHIND"] + +// canAdminOverride reports whether the Merge menu should offer to bypass the +// rules on the base branch: something has to be in the way that an override +// actually clears, and the account has to hold the privilege. +// +// This is the one place the menu does *not* widen on an unknown. Everything +// else here would rather offer an option GitHub turns down than drop one it +// would have taken, because a refusal at least says what happened — but a +// bypass is read as a permission you have, so offering one that is not there +// makes GitHub's refusal look like a broken button. Absent is the honest shape +// of "not yours"; the backend answers the same way (branchrules.go). +export function canAdminOverride(detail: PullRequestDetail, rules: BranchRules | null): boolean { + if (!rules?.viewerCanBypass) { + return false + } + if (detail.state !== "OPEN" || detail.isDraft) { + return false + } + return OVERRIDABLE.includes(detail.mergeStateStatus) +} + +// GitHub's pattern operators in the words a sentence needs. An operator this +// build has never seen falls back to "match": the pattern beside it still says +// more than no note at all. +const PATTERN_VERBS: Record = { + regex: "match", + starts_with: "start with", + ends_with: "end with", + contains: "contain", +} + +function describePattern(rule: CommitPattern): string { + const verb = PATTERN_VERBS[rule.operator] ?? "match" + return `${rule.target} must ${rule.negate ? "not " : ""}${verb} ${rule.pattern}` +} + +// mergeRuleNote names what governs the base branch when GitHub answers BLOCKED +// and nothing on the pull request accounts for it — the approved, green, +// conflict-free case, where the cause is a rule read against the commit the +// merge would write and no field on screen mentions it. +// +// It is a note, never a verdict: it names the rules that exist, not the one +// that fired. GitHub publishes no "why is this BLOCKED", so the honest answer +// is where to look — and re-running the pattern here would be a guess dressed +// as an answer, in a regex dialect that is not the one GitHub evaluates. +export function mergeRuleNote(detail: PullRequestDetail, rules: BranchRules | null): string | null { + if (detail.mergeStateStatus !== "BLOCKED") { + return null + } + const patterns = rules?.commitPatterns ?? [] + if (patterns.length === 0) { + return null + } + const clauses = patterns.map(describePattern).join("; ") + return `A ruleset governs ${detail.baseRefName}: every commit ${clauses}.` +} diff --git a/frontend/src/lib/rpc.ts b/frontend/src/lib/rpc.ts index e2eb571ab..393aa4b31 100644 --- a/frontend/src/lib/rpc.ts +++ b/frontend/src/lib/rpc.ts @@ -148,14 +148,18 @@ export const ProjectService = { /** One open PR in full (title, body, checks): the given number, or 0 for the checkout's own branch. */ PullRequestDetail: (path: string, number: number) => call("project.PullRequestDetail", [path, number]), - /** Merge a PR on GitHub (0 = the checkout's branch). The backend allow-lists the method. */ + /** Merge a PR on GitHub (0 = the checkout's branch). The backend allow-lists + * the method. `admin` merges with administrator privileges — GitHub's own + * bypass of the rules on the base branch, and the only way gh calls GitHub at + * all once a rule holds the merge back. */ MergePullRequest: ( path: string, number: number, method: MergeMethod, subject: string, body: string, - ) => call("project.MergePullRequest", [path, number, method, subject, body]), + admin: boolean, + ) => call("project.MergePullRequest", [path, number, method, subject, body, admin]), /** A PR's reviews, its own comments and its review threads, in one call. The * number is required — the query addresses a PR, never "the branch's". */ PullRequestConversation: (path: string, number: number) => diff --git a/internal/project/branchrules.go b/internal/project/branchrules.go index 84f9ceb7e..cdb93c982 100644 --- a/internal/project/branchrules.go +++ b/internal/project/branchrules.go @@ -19,24 +19,63 @@ type BranchRules struct { // Empty means the branch has no rule about it and all three stand — which // is also what a repository with no rulesets at all reports. AllowedMergeMethods []string `json:"allowedMergeMethods"` + // CommitPatterns are the rules that read a commit's own text. Empty is the + // common case and means only "no rule of this kind", never "nothing blocks". + CommitPatterns []CommitPattern `json:"commitPatterns"` + // ViewerCanBypass is whether this account administers the repository, and so + // may merge past the rules above. It is the one answer here that must fail + // *closed*: an unreadable one hides an action rather than offering one that + // cannot work, because a bypass offered to an account that has none reads as + // a broken button, not as a permission it never had. + ViewerCanBypass bool `json:"viewerCanBypass"` } -// ghBranchRule is one entry of the rules list; only the pull_request rule -// carries anything this reads. +// CommitPattern is one ruleset rule that tests the text of a commit rather than +// the state of a pull request. It earns its own field because it is the one +// class of rule nothing on a pull request hints at: the checks pass, the review +// lands, no conflict exists, and GitHub still answers BLOCKED — because the +// message the merge *would* write breaks a pattern nobody can see from here. +type CommitPattern struct { + // Target is the part the rule reads, already in the words the screen uses: + // "message", "author email", "committer email". + Target string `json:"target"` + // Operator is GitHub's own: regex | starts_with | ends_with | contains. + Operator string `json:"operator"` + Pattern string `json:"pattern"` + // Negate flips the test — the pattern is then what the text must *not* be. + Negate bool `json:"negate"` +} + +// commitPatternTargets maps the rule types that test commit text onto the word +// the screen puts in the sentence. GitHub has pattern rules for branch and tag +// names too; they gate a push, never a merge, so they are left out. +var commitPatternTargets = map[string]string{ + "commit_message_pattern": "message", + "commit_author_email_pattern": "author email", + "committer_email_pattern": "committer email", +} + +// ghBranchRule is one entry of the rules list; the pull_request rule and the +// commit pattern rules are all this reads. type ghBranchRule struct { Type string `json:"type"` Parameters struct { AllowedMergeMethods []string `json:"allowed_merge_methods"` + Operator string `json:"operator"` + Pattern string `json:"pattern"` + Negate bool `json:"negate"` } `json:"parameters"` } -// BranchRules reads the rules a repository's rulesets apply to one branch. +// BranchRules reads the rules a repository's rulesets apply to one branch, and +// whether this account may merge past them. // // A branch nothing governs answers with an empty list, and so does a token that // may not read the rules — both reach the screen as "no rule known", which is // the same shape as a repository that has none. That is deliberate: this read // exists to *narrow* what the merge menu offers, so failing to answer must -// leave the menu as wide as it was, never empty. +// leave the menu as wide as it was, never empty. The bypass is the exception +// and runs the other way; see ViewerCanBypass and viewerAdministers. func (s *Service) BranchRules(path, branch string) (*BranchRules, error) { if branch == "" { return nil, fmt.Errorf("a branch is required") @@ -46,7 +85,37 @@ func (s *Service) BranchRules(path, branch string) (*BranchRules, error) { if err != nil { return nil, err } - return parseBranchRules(out), nil + rules := parseBranchRules(out) + rules.ViewerCanBypass = s.viewerAdministers(path) + return rules, nil +} + +// viewerAdministers reports whether the account gh uses for this checkout +// administers the repository — GitHub's own bar for the merge override. +// +// A second call, because nothing on a pull request or in the rules answers it. +// It costs one small read per base branch, which the screen caches, and buys +// the thing a click cannot: an action that is *absent* rather than offered and +// refused. Every failure — the call, the decode, a repository gh cannot see — +// is that same absence, never an error: the rules this rides along with are +// worth drawing on their own. +// +// Only ADMIN counts. A ruleset can name any role as a bypass actor, so an +// account that may bypass without administering the repository does not get +// the option here; reading each ruleset's bypass list to find out is the +// upgrade path. +func (s *Service) viewerAdministers(path string) bool { + out, err := s.gh(prReadTimeout, path, "repo", "view", "--json", "viewerPermission") + if err != nil { + return false + } + var repo struct { + ViewerPermission string `json:"viewerPermission"` + } + if err := json.Unmarshal(out, &repo); err != nil { + return false + } + return repo.ViewerPermission == "ADMIN" } // parseBranchRules keeps what the merge menu asks for. An answer this cannot @@ -58,10 +127,21 @@ func parseBranchRules(out []byte) *BranchRules { if err := json.Unmarshal(out, &rules); err != nil { return &BranchRules{} } + parsed := &BranchRules{} for _, rule := range rules { if rule.Type == "pull_request" && len(rule.Parameters.AllowedMergeMethods) > 0 { - return &BranchRules{AllowedMergeMethods: rule.Parameters.AllowedMergeMethods} + parsed.AllowedMergeMethods = rule.Parameters.AllowedMergeMethods + } + // A pattern with nothing to test against explains nothing, so it is + // dropped rather than shown as an empty requirement. + if target, ok := commitPatternTargets[rule.Type]; ok && rule.Parameters.Pattern != "" { + parsed.CommitPatterns = append(parsed.CommitPatterns, CommitPattern{ + Target: target, + Operator: rule.Parameters.Operator, + Pattern: rule.Parameters.Pattern, + Negate: rule.Parameters.Negate, + }) } } - return &BranchRules{} + return parsed } diff --git a/internal/project/branchrules_test.go b/internal/project/branchrules_test.go index f3fdba6f2..a2056357d 100644 --- a/internal/project/branchrules_test.go +++ b/internal/project/branchrules_test.go @@ -9,7 +9,7 @@ func TestParseBranchRules(t *testing.T) { t.Run("a ruleset that narrows the merge methods", func(t *testing.T) { out := []byte(`[ {"type":"deletion","parameters":null}, - {"type":"commit_message_pattern","parameters":{"pattern":"^(feat|fix): .+"}}, + {"type":"commit_message_pattern","parameters":{"operator":"regex","pattern":"^(feat|fix): .+"}}, {"type":"pull_request","parameters":{"allowed_merge_methods":["squash"],"required_approving_review_count":1}} ]`) rules := parseBranchRules(out) @@ -18,6 +18,36 @@ func TestParseBranchRules(t *testing.T) { } }) + // The rule that sends an approved, green, conflict-free pull request to + // BLOCKED. Reading it is the whole reason the screen can name a cause. + t.Run("commit pattern rules are kept with what they test", func(t *testing.T) { + out := []byte(`[ + {"type":"commit_message_pattern","parameters":{"operator":"regex","pattern":"^(feat|fix): .+","negate":false,"name":"Conventional Commits"}}, + {"type":"committer_email_pattern","parameters":{"operator":"ends_with","pattern":"@example.com","negate":true}} + ]`) + got := parseBranchRules(out).CommitPatterns + want := []CommitPattern{ + {Target: "message", Operator: "regex", Pattern: "^(feat|fix): .+"}, + {Target: "committer email", Operator: "ends_with", Pattern: "@example.com", Negate: true}, + } + if !slices.Equal(got, want) { + t.Errorf("patterns = %+v, want %+v", got, want) + } + }) + + // A branch name pattern governs pushes, not merges: naming it beside the + // rules that do block would send the reader after the wrong rule. + t.Run("rules that gate a push rather than a merge are left out", func(t *testing.T) { + out := []byte(`[ + {"type":"branch_name_pattern","parameters":{"operator":"starts_with","pattern":"feat/"}}, + {"type":"tag_name_pattern","parameters":{"operator":"starts_with","pattern":"v"}}, + {"type":"commit_message_pattern","parameters":{"operator":"contains","pattern":""}} + ]`) + if got := parseBranchRules(out).CommitPatterns; len(got) != 0 { + t.Errorf("patterns = %+v, want none", got) + } + }) + t.Run("a branch nothing governs leaves every method standing", func(t *testing.T) { if got := parseBranchRules([]byte(`[]`)).AllowedMergeMethods; len(got) != 0 { t.Errorf("methods = %v, want none named", got) @@ -49,8 +79,64 @@ func TestBranchRulesFlow(t *testing.T) { t.Fatalf("unexpected error: %v", err) } want := []string{"api", "repos/{owner}/{repo}/rules/branches/feat%2FIMPEX-728"} - if !slices.Equal(gh.args, want) { - t.Errorf("args = %v, want %v", gh.args, want) + if len(gh.history) == 0 || !slices.Equal(gh.history[0], want) { + t.Errorf("args = %v, want %v", gh.history, want) + } + }) + + t.Run("administering the repository is what opens the bypass", func(t *testing.T) { + gh := &fakeGH{outs: [][]byte{[]byte(`[]`), []byte(`{"viewerPermission":"ADMIN"}`)}} + rules, err := withGH(gh).BranchRules("/repo", "develop") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !rules.ViewerCanBypass { + t.Error("an administering account should be offered the bypass") + } + want := []string{"repo", "view", "--json", "viewerPermission"} + if len(gh.history) != 2 || !slices.Equal(gh.history[1], want) { + t.Errorf("second call = %v, want %v", gh.history, want) + } + }) + + // The bypass is the one answer here that fails closed: offered to an account + // that does not have it, it reads as a broken button rather than a + // permission it never held. + t.Run("anything short of an answer keeps the bypass hidden", func(t *testing.T) { + for _, second := range []string{ + `{"viewerPermission":"WRITE"}`, + `{"viewerPermission":"MAINTAIN"}`, + `{}`, + `not json`, + ``, + } { + gh := &fakeGH{outs: [][]byte{[]byte(`[]`), []byte(second)}} + rules, err := withGH(gh).BranchRules("/repo", "develop") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rules.ViewerCanBypass { + t.Errorf("permission answer %q offered the bypass", second) + } + } + }) + + // The rules are worth drawing even when the permission read fails, so the + // failure hides the bypass and nothing else. + t.Run("a failed permission read costs the bypass, not the rules", func(t *testing.T) { + gh := &fakeGH{ + outs: [][]byte{[]byte(`[{"type":"pull_request","parameters":{"allowed_merge_methods":["squash"]}}]`)}, + errs: []error{nil, testError("gh could not complete the request.")}, + } + rules, err := withGH(gh).BranchRules("/repo", "develop") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rules.ViewerCanBypass { + t.Error("a failed permission read must not offer the bypass") + } + if want := []string{"squash"}; !slices.Equal(rules.AllowedMergeMethods, want) { + t.Errorf("methods = %v, want %v", rules.AllowedMergeMethods, want) } }) diff --git a/internal/project/gherror.go b/internal/project/gherror.go index 4407722c0..5e1ea7e62 100644 --- a/internal/project/gherror.go +++ b/internal/project/gherror.go @@ -58,6 +58,22 @@ var ghFailures = []toolFailure{ {"none of the git remotes", "This checkout's git remotes do not point at GitHub."}, {"no git remotes found", "This checkout has no git remote to look up on GitHub."}, {"not a git repository", "This folder is not a git repository."}, + // gh refuses a merge before it calls GitHub, and its one sentence carries + // the reason in the tail: these two match that tail, because "not mergeable" + // alone cannot tell a conflict from a rule and named the wrong one half the + // time. The bare match stays last for a wording neither one catches. + { + "merge commit cannot be cleanly created", + "This pull request conflicts with its base branch — merge the base in and resolve them first.", + }, + { + "base branch policy prohibits the merge", + "A rule on the base branch blocks this merge. Its ruleset on GitHub says which one.", + }, + { + "rule violations found", + "A ruleset on the base branch refused this merge — GitHub named the rules it broke; lich's log has them.", + }, {"not mergeable", "GitHub will not merge this pull request yet — it has conflicts or an unmet requirement."}, {"protected branch", "A branch protection rule on GitHub blocks this merge."}, {"required status check", "GitHub requires a status check that has not passed yet."}, diff --git a/internal/project/gherror_test.go b/internal/project/gherror_test.go index a64bc25a0..c9242333a 100644 --- a/internal/project/gherror_test.go +++ b/internal/project/gherror_test.go @@ -57,11 +57,26 @@ func TestGHMessage(t *testing.T) { errTest, nil, "This checkout's git remotes do not point at GitHub.", }, + // gh writes one sentence for both refusals and puts the reason in its + // tail, so these two must not collapse into each other: only one of them + // is a conflict, and the other is fixed somewhere else entirely. { - "an unmergeable pull request", + "a conflicting pull request", "Pull request is not mergeable: the merge commit cannot be cleanly created.", errTest, nil, - "GitHub will not merge this pull request yet — it has conflicts or an unmet requirement.", + "This pull request conflicts with its base branch — merge the base in and resolve them first.", + }, + { + "a pull request a rule holds back", + "Pull request is not mergeable: the base branch policy prohibits the merge.", + errTest, nil, + "A rule on the base branch blocks this merge. Its ruleset on GitHub says which one.", + }, + { + "GitHub's own answer when a bypass merge is refused", + "Rule violations found: - Commit message must match the required pattern", + errTest, nil, + "A ruleset on the base branch refused this merge — GitHub named the rules it broke; lich's log has them.", }, { "gh missing, reported by exec", diff --git a/internal/project/pr.go b/internal/project/pr.go index b361234b6..318cac8e0 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -265,11 +265,13 @@ var mergeMethods = map[string]string{ // (squash|merge|rebase) — the given number, or the PR of the branch checked out // at path when number is zero. A non-empty subject overrides the commit message // (the dock's "edit message" flow); an empty subject leaves gh's default, the -// quick merge. It does not pass --delete-branch: lich's worktree removal owns -// that cleanup, and deleting a checked-out worktree's branch is trouble. gh's -// stderr is surfaced on failure (not mergeable, branch protection, gh missing). -func (s *Service) MergePullRequest(path string, number int, method, subject, body string) error { - args, err := mergeArgs(number, method, subject, body) +// quick merge. admin merges with administrator privileges, GitHub's own bypass +// of the rules on the base branch. It does not pass --delete-branch: lich's +// worktree removal owns that cleanup, and deleting a checked-out worktree's +// branch is trouble. gh's stderr is surfaced on failure (not mergeable, branch +// protection, gh missing). +func (s *Service) MergePullRequest(path string, number int, method, subject, body string, admin bool) error { + args, err := mergeArgs(number, method, subject, body, admin) if err != nil { return err } @@ -284,7 +286,12 @@ func (s *Service) MergePullRequest(path string, number int, method, subject, bod // commits — rebase replays the branch's own commits, so gh rejects a message // there — and an empty subject means gh's default message. body rides along with // a subject and may itself be empty. -func mergeArgs(number int, method, subject, body string) ([]string, error) { +// +// --admin is what makes gh call GitHub at all on a governed branch: without it +// gh refuses from the client the moment mergeStateStatus is BLOCKED or BEHIND, +// so no rule violation ever reaches the screen. Whether this account may +// actually bypass is GitHub's answer to give, not this one's to guess. +func mergeArgs(number int, method, subject, body string, admin bool) ([]string, error) { flag, ok := mergeMethods[method] if !ok { return nil, fmt.Errorf("unknown merge method %q", method) @@ -293,6 +300,9 @@ func mergeArgs(number int, method, subject, body string) ([]string, error) { if subject != "" && method != "rebase" { args = append(args, "--subject", subject, "--body", body) } + if admin { + args = append(args, "--admin") + } return args, nil } diff --git a/internal/project/pr_test.go b/internal/project/pr_test.go index e377004be..02f72a174 100644 --- a/internal/project/pr_test.go +++ b/internal/project/pr_test.go @@ -282,7 +282,7 @@ func TestToCommits(t *testing.T) { func TestMergeArgs(t *testing.T) { // An unrecognised method must be refused before any gh shell-out. - if _, err := mergeArgs(0, "force", "", ""); err == nil { + if _, err := mergeArgs(0, "force", "", "", false); err == nil { t.Error("expected an error for an unknown merge method") } @@ -292,42 +292,57 @@ func TestMergeArgs(t *testing.T) { method string subject string body string + admin bool want []string }{ - {"quick squash", 0, "squash", "", "", []string{"pr", "merge", "--squash"}}, - {"quick merge", 0, "merge", "", "", []string{"pr", "merge", "--merge"}}, - {"quick rebase", 0, "rebase", "", "", []string{"pr", "merge", "--rebase"}}, + {"quick squash", 0, "squash", "", "", false, []string{"pr", "merge", "--squash"}}, + {"quick merge", 0, "merge", "", "", false, []string{"pr", "merge", "--merge"}}, + {"quick rebase", 0, "rebase", "", "", false, []string{"pr", "merge", "--rebase"}}, { "squash with edited message", - 0, "squash", "title (#1)", "details", + 0, "squash", "title (#1)", "details", false, []string{"pr", "merge", "--squash", "--subject", "title (#1)", "--body", "details"}, }, { "empty body still passes both flags", - 0, "merge", "subject only", "", + 0, "merge", "subject only", "", false, []string{"pr", "merge", "--merge", "--subject", "subject only", "--body", ""}, }, { "rebase ignores an edited message", - 0, "rebase", "unused", "unused", + 0, "rebase", "unused", "unused", false, []string{"pr", "merge", "--rebase"}, }, // The selector sits ahead of the flags — gh reads a trailing number as // an argument to whichever flag precedes it. { "a numbered pull request is selected before the flags", - 42, "squash", "", "", + 42, "squash", "", "", false, []string{"pr", "merge", "42", "--squash"}, }, { "the selector survives an edited message", 42, "squash", "title (#42)", "", + false, []string{"pr", "merge", "42", "--squash", "--subject", "title (#42)", "--body", ""}, }, + { + "a bypass merge carries --admin", + 42, "squash", "", "", true, + []string{"pr", "merge", "42", "--squash", "--admin"}, + }, + { + "a bypass merge keeps an edited message", + 0, "squash", "refactor(dashboard): fix", "", true, + []string{ + "pr", "merge", "--squash", + "--subject", "refactor(dashboard): fix", "--body", "", "--admin", + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := mergeArgs(tt.number, tt.method, tt.subject, tt.body) + got, err := mergeArgs(tt.number, tt.method, tt.subject, tt.body, tt.admin) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -368,21 +383,37 @@ func (e testError) Error() string { return string(e) } // fakeGH stands in for the gh CLI: it records the invocation and replays a // canned result, so the pull request flows run without a GitHub remote. +// +// out and err answer every call. A flow that makes more than one gh call — and +// needs a different answer from each — sets outs/errs instead, one entry per +// call in order; a call past the end of either falls back to out/err. history +// keeps every invocation, while args stays the last one. type fakeGH struct { - out []byte - err error + out []byte + outs [][]byte + err error + errs []error calls int timeout time.Duration dir string token string args []string + history [][]string } func (f *fakeGH) run(timeout time.Duration, dir, token string, args ...string) ([]byte, error) { + out, err := f.out, f.err + if f.calls < len(f.outs) { + out = f.outs[f.calls] + } + if f.calls < len(f.errs) { + err = f.errs[f.calls] + } f.calls++ f.timeout, f.dir, f.token, f.args = timeout, dir, token, args - return f.out, f.err + f.history = append(f.history, args) + return out, err } // withGH builds a Service whose gh calls land on the fake instead of the CLI. @@ -476,7 +507,7 @@ func TestPullRequestBadgeRunsThroughTheRunner(t *testing.T) { func TestMergePullRequestFlow(t *testing.T) { t.Run("the built args reach gh", func(t *testing.T) { gh := &fakeGH{} - if err := withGH(gh).MergePullRequest("/repo", 0, "squash", "", ""); err != nil { + if err := withGH(gh).MergePullRequest("/repo", 0, "squash", "", "", false); err != nil { t.Fatalf("unexpected error: %v", err) } if want := []string{"pr", "merge", "--squash"}; !slices.Equal(gh.args, want) { @@ -489,7 +520,7 @@ func TestMergePullRequestFlow(t *testing.T) { t.Run("an unknown method never reaches gh", func(t *testing.T) { gh := &fakeGH{} - if err := withGH(gh).MergePullRequest("/repo", 0, "force", "", ""); err == nil { + if err := withGH(gh).MergePullRequest("/repo", 0, "force", "", "", false); err == nil { t.Error("expected an error for an unknown merge method") } if gh.calls != 0 { @@ -499,7 +530,7 @@ func TestMergePullRequestFlow(t *testing.T) { t.Run("gh's refusal surfaces", func(t *testing.T) { gh := &fakeGH{err: errors.New("Pull request is not mergeable")} - err := withGH(gh).MergePullRequest("/repo", 0, "merge", "", "") + err := withGH(gh).MergePullRequest("/repo", 0, "merge", "", "", false) if err == nil { t.Fatal("expected an error") } From 928c21fae11f881cb2bdcf7cd9c162cd934c6093 Mon Sep 17 00:00:00 2001 From: omartelo Date: Fri, 31 Jul 2026 11:18:43 -0300 Subject: [PATCH 2/2] fix: never offer a bypass over a conflict a stale merge state hides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canAdminOverride read mergeStateStatus alone, so BLOCKED reported over a conflict — the case mergeBlockedReason already reads both fields for — took the bypass past every guard: the Merge button went live, the menu read "Squash and merge, bypass rules", and the click failed on a conflict no override resolves. That is the dead click the fail-closed design exists to remove, and the shipped promise is "never over a conflict or a draft, which no override resolves". Read mergeable too, as the rest of the gate does. Also names what the menu's all-or-nothing rule costs: BLOCKED is what GitHub answers for any governed base branch, rules met or not, so an administrator's ordinary approved merge goes out with --admin and GitHub records it as an override. gh leaves no other way through, and nothing on screen or in the code said so. Co-Authored-By: Claude Opus 5 --- frontend/src/components/pulls/PullRequestView.tsx | 8 +++++++- frontend/src/lib/pulls/merge-gate.test.ts | 5 +++++ frontend/src/lib/pulls/merge-gate.ts | 6 ++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/pulls/PullRequestView.tsx b/frontend/src/components/pulls/PullRequestView.tsx index 11cff6afe..9f07b8d77 100644 --- a/frontend/src/components/pulls/PullRequestView.tsx +++ b/frontend/src/components/pulls/PullRequestView.tsx @@ -318,7 +318,13 @@ export function PullRequestView({ dead and keeping them would put the click that cannot work beside the one that can. Without the privilege the dead click stays, on purpose: it is what carries gh's refusal and - the rule note back (merge-gate.ts). */} + the rule note back (merge-gate.ts). + + The consequence to know: BLOCKED is what GitHub answers for + any governed base branch, rules met or not, so an + administrator's ordinary approved merge goes out with + --admin here and GitHub records it as an override. gh leaves + no other way through. */} {methods.map((method) => ( { expect( canAdminOverride(detail({ mergeStateStatus: "DIRTY", mergeable: "CONFLICTING" }), admin()), ).toBe(false) + // The conflict a stale merge state hides — BLOCKED is on the override list, + // and only the second field says there is nothing to override. + expect( + canAdminOverride(detail({ mergeStateStatus: "BLOCKED", mergeable: "CONFLICTING" }), admin()), + ).toBe(false) expect(canAdminOverride(detail({ isDraft: true, mergeStateStatus: "BLOCKED" }), admin())).toBe( false, ) diff --git a/frontend/src/lib/pulls/merge-gate.ts b/frontend/src/lib/pulls/merge-gate.ts index 543145538..5114bfc09 100644 --- a/frontend/src/lib/pulls/merge-gate.ts +++ b/frontend/src/lib/pulls/merge-gate.ts @@ -89,6 +89,12 @@ export function canAdminOverride(detail: PullRequestDetail, rules: BranchRules | if (detail.state !== "OPEN" || detail.isDraft) { return false } + // Both fields again, for the reason mergeBlockedReason reads both: GitHub + // answers BLOCKED over a conflict while its merge state is still stale, and a + // bypass offered there is the dead click this whole gate exists to remove. + if (detail.mergeable === "CONFLICTING") { + return false + } return OVERRIDABLE.includes(detail.mergeStateStatus) }