Skip to content

fix(ruleset): own pull request merge controls #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rldyourmnd merged 1 commit into from
Aug 22, 2026
Merged

fix(ruleset): own pull request merge controls #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/githubruleset/adoption.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ func PlanAdoption(state githubprovider.RepositoryRulesetState, observedAt time.T
return AdoptionPlan{}, errors.New("ruleset adoption requires a fresh full privileged observation")
}
plan := AdoptionPlan{SchemaVersion: 1, RulesetID: state.ID, ObservedAt: observedAt.UTC(), ObservedDigest: state.WritableDigest,
OwnedPaths: []string{"/enforcement", "/rules/required_status_checks"},
ExternalPaths: []string{"/bypass_actors", "/conditions", "/rules/pull_request", "/rules/*"},
OwnedPaths: []string{"/enforcement", "/rules/pull_request", "/rules/required_status_checks"},
ExternalPaths: []string{"/bypass_actors", "/conditions", "/rules/*"},
UnknownPolicy: "preserve-or-refuse", Status: "observation-only"}
digest, err := canonicaljson.Digest(plan)
if err != nil {
Expand Down
65 changes: 51 additions & 14 deletions core/githubruleset/ruleset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,18 @@ func normalizeDesired(value githubprovider.RepositoryRuleset) (githubprovider.Re
len(rule.RequiredStatusChecks) != 0 {
return githubprovider.RepositoryRuleset{}, errors.New("GitHub pull-request rule is invalid")
}
if len(rule.AllowedMergeMethods) == 0 {
rule.AllowedMergeMethods = []string{"merge", "rebase", "squash"}
}
methods := append([]string(nil), rule.AllowedMergeMethods...)
sort.Strings(methods)
for methodIndex, method := range methods {
if (method != "merge" && method != "rebase" && method != "squash") ||
(methodIndex > 0 && methods[methodIndex-1] == method) {
return githubprovider.RepositoryRuleset{}, errors.New("GitHub pull-request merge methods are invalid")
}
}
rule.AllowedMergeMethods = methods
case "required_status_checks":
if len(rule.RequiredStatusChecks) == 0 || len(rule.RequiredStatusChecks) > 50 ||
rule.RequiredApprovingReviewCount != 0 {
Expand Down Expand Up @@ -451,43 +463,68 @@ func ownedStateEqual(current githubprovider.RepositoryRulesetState, desired gith
if desired.Enforcement == "" && current.Enforcement != "active" {
return false
}
return reflect.DeepEqual(requiredChecks(current.Rules), requiredChecks(desired.Rules))
return reflect.DeepEqual(ownedRules(current.Rules), ownedRules(desired.Rules))
}

func applyOwnedState(current githubprovider.RepositoryRulesetState, desired githubprovider.RepositoryRuleset) githubprovider.RepositoryRulesetState {
current.Enforcement = desired.Enforcement
if current.Enforcement == "" {
current.Enforcement = "active"
}
owned := requiredChecks(desired.Rules)
result := make([]githubprovider.RulesetRule, 0, len(current.Rules)+1)
replaced := false
owned := ownedRulesByType(desired.Rules)
result := make([]githubprovider.RulesetRule, 0, len(current.Rules)+len(owned))
for _, rule := range current.Rules {
if rule.Type == "required_status_checks" {
if owned != nil {
result = append(result, *owned)
if rule.Type == "required_status_checks" || rule.Type == "pull_request" {
if replacement, exists := owned[rule.Type]; exists {
result = append(result, replacement)
}
replaced = true
delete(owned, rule.Type)
continue
}
result = append(result, rule)
}
if owned != nil && !replaced {
result = append(result, *owned)
for _, typeName := range []string{"pull_request", "required_status_checks"} {
if replacement, exists := owned[typeName]; exists {
result = append(result, replacement)
}
}
sort.Slice(result, func(i, j int) bool { return result[i].Type < result[j].Type })
current.Rules = result
return current
}

func requiredChecks(rules []githubprovider.RulesetRule) *githubprovider.RulesetRule {
func ownedRules(rules []githubprovider.RulesetRule) []githubprovider.RulesetRule {
byType := ownedRulesByType(rules)
result := make([]githubprovider.RulesetRule, 0, len(byType))
for _, typeName := range []string{"pull_request", "required_status_checks"} {
if rule, exists := byType[typeName]; exists {
result = append(result, rule)
}
}
return result
}

func ownedRulesByType(rules []githubprovider.RulesetRule) map[string]githubprovider.RulesetRule {
result := map[string]githubprovider.RulesetRule{}
for _, rule := range rules {
if rule.Type == "required_status_checks" {
if rule.Type == "required_status_checks" || rule.Type == "pull_request" {
copy := rule
return &copy
copy.ExternalParameters = nil
copy.OpaqueParameters = nil
copy.AllowedMergeMethods = append([]string(nil), copy.AllowedMergeMethods...)
sort.Strings(copy.AllowedMergeMethods)
result[copy.Type] = copy
}
}
return nil
return result
}

func requiredChecks(rules []githubprovider.RulesetRule) *githubprovider.RulesetRule {
rule, exists := ownedRulesByType(rules)["required_status_checks"]
if !exists {
return nil
}
return &rule
}

func stateEvidence(
Expand Down
57 changes: 33 additions & 24 deletions core/providers/github/mutation_ruleset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,16 @@ type RequiredStatusCheck struct {
}

type RulesetRule struct {
Type string `json:"type"`
RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`
DismissStaleReviewsOnPush bool `json:"dismiss_stale_reviews_on_push,omitempty"`
RequireCodeOwnerReview bool `json:"require_code_owner_review,omitempty"`
RequiredReviewThreadResolution bool `json:"required_review_thread_resolution,omitempty"`
RequiredStatusChecks []RequiredStatusCheck `json:"required_status_checks,omitempty"`
StrictRequiredStatusChecksPolicy bool `json:"strict_required_status_checks_policy,omitempty"`
Type string `json:"type"`
RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`
DismissStaleReviewsOnPush bool `json:"dismiss_stale_reviews_on_push,omitempty"`
RequireCodeOwnerReview bool `json:"require_code_owner_review,omitempty"`
RequiredReviewThreadResolution bool `json:"required_review_thread_resolution,omitempty"`
RequireLastPushApproval bool `json:"require_last_push_approval,omitempty"`
RequireExtraApprovalForUnattributedChanges bool `json:"require_extra_approval_for_unattributed_changes,omitempty"`
AllowedMergeMethods []string `json:"allowed_merge_methods,omitempty"`
RequiredStatusChecks []RequiredStatusCheck `json:"required_status_checks,omitempty"`
StrictRequiredStatusChecksPolicy bool `json:"strict_required_status_checks_policy,omitempty"`
// OpaqueParameters preserves externally managed or provider-new rule
// parameters. GDS never synthesizes this field for rules it owns.
OpaqueParameters json.RawMessage `json:"opaque_parameters,omitempty"`
Expand Down Expand Up @@ -88,7 +91,7 @@ func (mutator *RepositoryMutator) UpsertDefaultBranchRuleset(
if payload["enforcement"] == "" {
payload["enforcement"] = "active"
}
if err := replaceOwnedRulesetChecks(payload, ruleset.Rules); err != nil {
if err := replaceOwnedRules(payload, ruleset.Rules); err != nil {
return RulesetSummary{}, MutationMeta{}, err
}
} else {
Expand Down Expand Up @@ -159,42 +162,46 @@ func (mutator *RepositoryMutator) UpsertDefaultBranchRuleset(
}, meta, nil
}

func replaceOwnedRulesetChecks(payload map[string]any, desired []RulesetRule) error {
func replaceOwnedRules(payload map[string]any, desired []RulesetRule) error {
rawRules, ok := payload["rules"].([]any)
if !ok {
return rulesetStageFieldFailure(
RulesetStageExternalFieldMerge, "preserved-rules-not-a-list", "rules",
)
}
var owned map[string]any
owned := map[string]map[string]any{}
for _, rule := range desired {
if rule.Type == "required_status_checks" {
owned = rulesetRulePayload(rule)
if rule.Type == "required_status_checks" || rule.Type == "pull_request" {
owned[rule.Type] = rulesetRulePayload(rule)
}
}
// Do not add to a provider-controlled length when computing allocation
// capacity. append grows the bounded decoded slice safely if the owned rule
// was not present in the observation.
result := make([]any, 0, len(rawRules))
replaced := false
for _, value := range rawRules {
rule, ok := value.(map[string]any)
if !ok {
return rulesetStageFieldFailure(
RulesetStageExternalFieldMerge, "preserved-rule-not-an-object", "rules[]",
)
}
if rule["type"] == "required_status_checks" {
if owned != nil {
result = append(result, owned)
typeName, _ := rule["type"].(string)
if typeName == "required_status_checks" || typeName == "pull_request" {
if replacement, exists := owned[typeName]; exists {
result = append(result, replacement)
delete(owned, typeName)
} else {
result = append(result, rule)
}
replaced = true
continue
}
result = append(result, rule)
}
if owned != nil && !replaced {
result = append(result, owned)
for _, typeName := range []string{"pull_request", "required_status_checks"} {
if replacement, exists := owned[typeName]; exists {
result = append(result, replacement)
}
}
payload["rules"] = result
return nil
Expand Down Expand Up @@ -275,11 +282,13 @@ func rulesetRulePayload(rule RulesetRule) map[string]any {
switch rule.Type {
case "pull_request":
payload["parameters"] = map[string]any{
"required_approving_review_count": rule.RequiredApprovingReviewCount,
"dismiss_stale_reviews_on_push": rule.DismissStaleReviewsOnPush,
"require_code_owner_review": rule.RequireCodeOwnerReview,
"required_review_thread_resolution": rule.RequiredReviewThreadResolution,
"require_last_push_approval": false,
"required_approving_review_count": rule.RequiredApprovingReviewCount,
"dismiss_stale_reviews_on_push": rule.DismissStaleReviewsOnPush,
"require_code_owner_review": rule.RequireCodeOwnerReview,
"required_review_thread_resolution": rule.RequiredReviewThreadResolution,
"require_last_push_approval": rule.RequireLastPushApproval,
"require_extra_approval_for_unattributed_changes": rule.RequireExtraApprovalForUnattributedChanges,
"allowed_merge_methods": rule.AllowedMergeMethods,
}
case "required_status_checks":
payload["parameters"] = map[string]any{
Expand Down
42 changes: 40 additions & 2 deletions core/providers/github/ruleset_live_shape_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,22 +55,60 @@ func TestCurrentLiveRulesetShapeIsObservable(t *testing.T) {
}
// Owned fields stay typed and exact.
if pullRequest.RequiredApprovingReviewCount != 0 || !pullRequest.DismissStaleReviewsOnPush ||
pullRequest.RequireCodeOwnerReview || !pullRequest.RequiredReviewThreadResolution {
pullRequest.RequireCodeOwnerReview || !pullRequest.RequiredReviewThreadResolution ||
pullRequest.RequireLastPushApproval || pullRequest.RequireExtraApprovalForUnattributedChanges ||
!reflect.DeepEqual(pullRequest.AllowedMergeMethods, []string{"merge"}) {
t.Fatalf("owned pull_request fields were not typed exactly: %#v", pullRequest)
}
// Externally managed fields are preserved rather than rejected or dropped.
var external map[string]json.RawMessage
if err := json.Unmarshal(pullRequest.ExternalParameters, &external); err != nil {
t.Fatalf("external pull_request parameters were not preserved: %v", err)
}
for _, key := range []string{"required_reviewers", "dismissal_restriction", "allowed_merge_methods"} {
for _, key := range []string{"required_reviewers", "dismissal_restriction"} {
if _, present := external[key]; !present {
t.Errorf("external pull_request parameter %q was dropped", key)
}
}
if _, leaked := external["required_approving_review_count"]; leaked {
t.Error("an owned field was also recorded as external")
}
for _, key := range []string{"require_last_push_approval", "require_extra_approval_for_unattributed_changes", "allowed_merge_methods"} {
if _, leaked := external[key]; leaked {
t.Errorf("owned pull_request parameter %q was also recorded as external", key)
}
}
}

func TestPullRequestOwnedParametersRoundTripExactly(t *testing.T) {
rule, err := normalizeRulesetRule("pull_request", json.RawMessage(`{
"required_approving_review_count":0,
"dismiss_stale_reviews_on_push":true,
"require_code_owner_review":false,
"required_review_thread_resolution":true,
"require_last_push_approval":false,
"require_extra_approval_for_unattributed_changes":true,
"allowed_merge_methods":["merge"],
"required_reviewers":[]
}`))
if err != nil {
t.Fatal(err)
}
if !rule.RequireExtraApprovalForUnattributedChanges ||
!reflect.DeepEqual(rule.AllowedMergeMethods, []string{"merge"}) {
t.Fatalf("owned pull-request parameters were not decoded: %#v", rule)
}
payload := rulesetRulePayload(RulesetRule{
Type: "pull_request", DismissStaleReviewsOnPush: true,
RequiredReviewThreadResolution: true,
RequireExtraApprovalForUnattributedChanges: false,
AllowedMergeMethods: []string{"merge"},
})
parameters := payload["parameters"].(map[string]any)
if parameters["require_extra_approval_for_unattributed_changes"] != false ||
!reflect.DeepEqual(parameters["allowed_merge_methods"], []string{"merge"}) {
t.Fatalf("owned pull-request parameters were not encoded: %#v", parameters)
}
}

func TestOwnedRulesetUpdatePreservesEveryExternalFieldByteForByte(t *testing.T) {
Expand Down
18 changes: 12 additions & 6 deletions core/providers/github/ruleset_observation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,25 +238,31 @@ func normalizeRulesetRule(ruleType string, parameters json.RawMessage) (RulesetR
}
case "pull_request":
var value struct {
RequiredApprovingReviewCount int `json:"required_approving_review_count"`
DismissStaleReviewsOnPush bool `json:"dismiss_stale_reviews_on_push"`
RequireCodeOwnerReview bool `json:"require_code_owner_review"`
RequiredReviewThreadResolution bool `json:"required_review_thread_resolution"`
RequireLastPushApproval bool `json:"require_last_push_approval"`
RequiredApprovingReviewCount int `json:"required_approving_review_count"`
DismissStaleReviewsOnPush bool `json:"dismiss_stale_reviews_on_push"`
RequireCodeOwnerReview bool `json:"require_code_owner_review"`
RequiredReviewThreadResolution bool `json:"required_review_thread_resolution"`
RequireLastPushApproval bool `json:"require_last_push_approval"`
RequireExtraApprovalForUnattributedChanges bool `json:"require_extra_approval_for_unattributed_changes"`
AllowedMergeMethods []string `json:"allowed_merge_methods"`
}
external, err := decodeOwnedRuleParameters(parameters, &value, []string{
"required_approving_review_count", "dismiss_stale_reviews_on_push",
"require_code_owner_review", "required_review_thread_resolution",
"require_last_push_approval",
"require_extra_approval_for_unattributed_changes", "allowed_merge_methods",
})
if err != nil || value.RequireLastPushApproval {
if err != nil {
return RulesetRule{}, fmt.Errorf("GitHub pull-request ruleset parameters are unsupported")
}
rule.ExternalParameters = external
rule.RequiredApprovingReviewCount = value.RequiredApprovingReviewCount
rule.DismissStaleReviewsOnPush = value.DismissStaleReviewsOnPush
rule.RequireCodeOwnerReview = value.RequireCodeOwnerReview
rule.RequiredReviewThreadResolution = value.RequiredReviewThreadResolution
rule.RequireLastPushApproval = value.RequireLastPushApproval
rule.RequireExtraApprovalForUnattributedChanges = value.RequireExtraApprovalForUnattributedChanges
rule.AllowedMergeMethods = append([]string(nil), value.AllowedMergeMethods...)
case "required_status_checks":
var value struct {
RequiredStatusChecks []RequiredStatusCheck `json:"required_status_checks"`
Expand Down