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..9f07b8d77 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). + + 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) => ( - 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 +344,7 @@ export function PullRequestView({ onClick={() => setEdit(mergeEditFor(method, detail))} > {MERGE_LABELS[method]}, edit message… + {bypass && " (bypasses rules)"} ))} @@ -442,7 +471,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..1b77940a4 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,120 @@ 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) + // 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, + ) + 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..5114bfc09 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,71 @@ 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 + } + // 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) +} + +// 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") }