From eca6390d6fe4b849633abc3711b942065f8e4410 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 12:58:28 +0200 Subject: [PATCH] Judge the one protection rule the forge applies, and keep a recorded repository id The forge applies the rule named for a branch, else the first glob covering it; the judge passed a branch whose applied rule let pushes when a later rule happened to guard it. And a registration whose id the forge could not give cleared the id already recorded (the confirmation review of 2026-10-09). --- cmd/mesh-controller/build_source.go | 63 ++++++++++++++++-------- cmd/mesh-controller/build_source_test.go | 35 +++++++++++++ internal/inventory/catalogue.go | 6 ++- 3 files changed, 81 insertions(+), 23 deletions(-) diff --git a/cmd/mesh-controller/build_source.go b/cmd/mesh-controller/build_source.go index 053639de..43cae8e0 100644 --- a/cmd/mesh-controller/build_source.go +++ b/cmd/mesh-controller/build_source.go @@ -175,33 +175,54 @@ var askTheForge = func(ctx context.Context, owner, repo, branch string) (forgeFa return forgeFacts{}, fmt.Errorf("the forge named no id for %s/%s", owner, repo) } var rules struct { - Rules []struct { - Rule string `json:"rule"` - Push bool `json:"push"` - RequiredStatuses []string `json:"required_statuses"` - AdminMayOverride bool `json:"admin_may_override"` - } `json:"rules"` + Rules []protectionRule `json:"rules"` } if err := ask("gitea_branch_protection_get", map[string]any{"owner": owner, "repo": repo}, &rules); err != nil { return forgeFacts{}, err } - facts := forgeFacts{ID: found.Result.ID, Why: fmt.Sprintf("no protection rule covers %s", branch)} - for _, r := range rules.Rules { - if !ruleCovers(r.Rule, branch) { - continue - } - switch { - case r.Push: - facts.Why = fmt.Sprintf("the rule %s lets a person push to %s directly", r.Rule, branch) - case len(r.RequiredStatuses) == 0: - facts.Why = fmt.Sprintf("the rule %s requires no status before a merge into %s", r.Rule, branch) - case r.AdminMayOverride: - facts.Why = fmt.Sprintf("the rule %s lets an administrator merge into %s past a status", r.Rule, branch) - default: - return forgeFacts{ID: facts.ID, Guarded: true}, nil + facts := judgedRule(rules.Rules, branch) + facts.ID = found.Result.ID + return facts, nil +} + +// protectionRule is a branch protection rule as the forge's tool summarises it. +type protectionRule struct { + Rule string `json:"rule"` + Push bool `json:"push"` + RequiredStatuses []string `json:"required_statuses"` + AdminMayOverride bool `json:"admin_may_override"` +} + +// judgedRule judges the one rule the forge applies to a branch, as the forge picks it: the rule named for the +// branch, else the first glob rule, in the forge's order, that covers it (gitea's first matching rule). Never a +// later rule that happens to be stronger: the forge does not read it. +func judgedRule(rules []protectionRule, branch string) forgeFacts { + var applied *protectionRule + for i := range rules { + if rules[i].Rule == branch { + applied = &rules[i] + break } } - return facts, nil + if applied == nil { + for i := range rules { + if ruleCovers(rules[i].Rule, branch) { + applied = &rules[i] + break + } + } + } + switch r := applied; { + case r == nil: + return forgeFacts{Why: fmt.Sprintf("no protection rule covers %s", branch)} + case r.Push: + return forgeFacts{Why: fmt.Sprintf("the rule %s lets a person push to %s directly", r.Rule, branch)} + case len(r.RequiredStatuses) == 0: + return forgeFacts{Why: fmt.Sprintf("the rule %s requires no status before a merge into %s", r.Rule, branch)} + case r.AdminMayOverride: + return forgeFacts{Why: fmt.Sprintf("the rule %s lets an administrator merge into %s past a status", r.Rule, branch)} + } + return forgeFacts{Guarded: true} } // ruleCovers says a protection rule's name — a branch, or a glob of them — covers a branch, as the forge reads it. diff --git a/cmd/mesh-controller/build_source_test.go b/cmd/mesh-controller/build_source_test.go index 69fc5d8a..f2589e1c 100644 --- a/cmd/mesh-controller/build_source_test.go +++ b/cmd/mesh-controller/build_source_test.go @@ -343,3 +343,38 @@ func TestTheServingControllerIsNeverTheTerminal(t *testing.T) { t.Fatal("what the serving controller starts does not carry its mark") } } + +// The forge applies one rule to a branch: the rule named for it, else the first glob that covers it. A stronger +// rule later in the list is not what guards the branch, and is not read as if it were (the confirmation review). +func TestTheRuleJudgedIsTheOneTheForgeApplies(t *testing.T) { + guarded := protectionRule{Rule: "*", RequiredStatuses: []string{"mesh/merge-gate"}} + open := protectionRule{Rule: "main", Push: true, RequiredStatuses: []string{"mesh/merge-gate"}} + if f := judgedRule([]protectionRule{guarded, open}, "main"); f.Guarded { + t.Error("an exact rule letting pushes was passed over for a glob that guards") + } + if f := judgedRule([]protectionRule{{Rule: "ma*", RequiredStatuses: nil}, {Rule: "*", RequiredStatuses: []string{"x"}}}, + "main"); f.Guarded || !strings.Contains(f.Why, "ma*") { + t.Errorf("the first glob covering the branch was not the one judged: %+v", f) + } + if f := judgedRule([]protectionRule{{Rule: "release/*"}, {Rule: "main", RequiredStatuses: []string{"x"}}}, "main"); !f.Guarded { + t.Errorf("the rule named for the branch was not judged: %+v", f) + } + if f := judgedRule(nil, "main"); f.Guarded { + t.Error("no rule is no protection") + } +} + +// An id the forge could not give keeps the id already recorded. +func TestAnUnknownIdentityKeepsTheRecordedOne(t *testing.T) { + open := theCatalogue(t) + ctx := t.Context() + if err := open.inventory.SetSourceIdentity(ctx, "sudo", 100); err != nil { + t.Fatal(err) + } + if err := open.inventory.SetSourceIdentity(ctx, "sudo", 0); err != nil { + t.Fatal(err) + } + if id, _ := open.inventory.SourceIdentity(ctx, "sudo"); id != 100 { + t.Fatalf("the recorded id became %d", id) + } +} diff --git a/internal/inventory/catalogue.go b/internal/inventory/catalogue.go index 71350a92..0c3b1332 100644 --- a/internal/inventory/catalogue.go +++ b/internal/inventory/catalogue.go @@ -182,9 +182,11 @@ func (i *Inventory) SourceIdentity(ctx context.Context, module string) (int64, e return *id, nil } -// SetSourceIdentity records the forge's id of the repository a module is registered from; 0 records none. +// SetSourceIdentity records the forge's id of the repository a module is registered from. 0 — the forge could not +// say, at the terminal — keeps the id already recorded: an unknown is never a reason to forget what was known. func (i *Inventory) SetSourceIdentity(ctx context.Context, module string, id int64) error { - _, err := i.store.Pool().Exec(ctx, `update module set source_repo_id = nullif($2::bigint, 0) where name = $1`, module, id) + _, err := i.store.Pool().Exec(ctx, + `update module set source_repo_id = coalesce(nullif($2::bigint, 0), source_repo_id) where name = $1`, module, id) return err }