From 4c05946c1799a81dad5095fa305dca6215c4f764 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Sat, 22 Aug 2026 09:25:05 +0500 Subject: [PATCH] fix(ruleset): preserve external pull parameters --- core/githubruleset/ruleset.go | 3 ++ core/githubruleset/ruleset_test.go | 17 +++++++++++ core/providers/github/mutation_ruleset.go | 35 +++++++++++++++++++++++ core/providers/github/mutation_test.go | 25 ++++++++++++---- 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/core/githubruleset/ruleset.go b/core/githubruleset/ruleset.go index f35a81b..92af58e 100644 --- a/core/githubruleset/ruleset.go +++ b/core/githubruleset/ruleset.go @@ -476,6 +476,9 @@ func applyOwnedState(current githubprovider.RepositoryRulesetState, desired gith for _, rule := range current.Rules { if rule.Type == "required_status_checks" || rule.Type == "pull_request" { if replacement, exists := owned[rule.Type]; exists { + if rule.Type == "pull_request" { + replacement.ExternalParameters = append(json.RawMessage(nil), rule.ExternalParameters...) + } result = append(result, replacement) } delete(owned, rule.Type) diff --git a/core/githubruleset/ruleset_test.go b/core/githubruleset/ruleset_test.go index b114203..9e8b578 100644 --- a/core/githubruleset/ruleset_test.go +++ b/core/githubruleset/ruleset_test.go @@ -233,3 +233,20 @@ func TestVisibleStateCanonicalizesPreservedRawParameters(t *testing.T) { t.Fatalf("unparseable external parameters were altered: %s", got) } } + +func TestApplyOwnedStatePreservesPullRequestExternalParameters(t *testing.T) { + external := json.RawMessage(`{"required_reviewers":[],"dismissal_restriction":{"enabled":false}}`) + current := githubprovider.RepositoryRulesetState{Enforcement: "active", Rules: []githubprovider.RulesetRule{{ + Type: "pull_request", RequireExtraApprovalForUnattributedChanges: true, + AllowedMergeMethods: []string{"merge", "squash", "rebase"}, ExternalParameters: external, + }}} + desired := githubprovider.RepositoryRuleset{Enforcement: "active", Rules: []githubprovider.RulesetRule{{ + Type: "pull_request", AllowedMergeMethods: []string{"merge"}, + }}} + updated := applyOwnedState(current, desired) + if len(updated.Rules) != 1 || updated.Rules[0].RequireExtraApprovalForUnattributedChanges || + !reflect.DeepEqual(updated.Rules[0].AllowedMergeMethods, []string{"merge"}) || + !reflect.DeepEqual(updated.Rules[0].ExternalParameters, external) { + t.Fatalf("owned pull update lost external parameters or desired controls: %#v", updated.Rules) + } +} diff --git a/core/providers/github/mutation_ruleset.go b/core/providers/github/mutation_ruleset.go index 6dd6bd1..7d7a8d2 100644 --- a/core/providers/github/mutation_ruleset.go +++ b/core/providers/github/mutation_ruleset.go @@ -189,6 +189,11 @@ func replaceOwnedRules(payload map[string]any, desired []RulesetRule) error { typeName, _ := rule["type"].(string) if typeName == "required_status_checks" || typeName == "pull_request" { if replacement, exists := owned[typeName]; exists { + if typeName == "pull_request" { + if err := preserveExternalPullParameters(rule, replacement); err != nil { + return err + } + } result = append(result, replacement) delete(owned, typeName) } else { @@ -207,6 +212,36 @@ func replaceOwnedRules(payload map[string]any, desired []RulesetRule) error { return nil } +func preserveExternalPullParameters(observed, desired map[string]any) error { + observedParameters, ok := observed["parameters"].(map[string]any) + if !ok { + return rulesetStageFieldFailure( + RulesetStageExternalFieldMerge, "preserved-pull-parameters-not-an-object", "rules/pull_request", + ) + } + desiredParameters, ok := desired["parameters"].(map[string]any) + if !ok { + return rulesetStageFieldFailure( + RulesetStageExternalFieldMerge, "desired-pull-parameters-not-an-object", "rules/pull_request", + ) + } + owned := map[string]bool{ + "required_approving_review_count": true, + "dismiss_stale_reviews_on_push": true, + "require_code_owner_review": true, + "required_review_thread_resolution": true, + "require_last_push_approval": true, + "require_extra_approval_for_unattributed_changes": true, + "allowed_merge_methods": true, + } + for key, value := range observedParameters { + if !owned[key] { + desiredParameters[key] = value + } + } + return nil +} + func validateRepositoryRuleset(ruleset RepositoryRuleset) error { // Each rejected field is named, so a contract that a live reconcile refuses // points at the exact declaration to correct rather than at the whole diff --git a/core/providers/github/mutation_test.go b/core/providers/github/mutation_test.go index 9770d20..52fe956 100644 --- a/core/providers/github/mutation_test.go +++ b/core/providers/github/mutation_test.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "reflect" "strings" "sync/atomic" "testing" @@ -268,14 +269,22 @@ func TestRepositoryRulesetUpdatePreservesExternallyManagedPayload(t *testing.T) "bypass_actors":[{"actor_id":7,"actor_type":"Team","bypass_mode":"always"}], "conditions":{"ref_name":{"include":["~DEFAULT_BRANCH"],"exclude":["refs/heads/vendor/**"]}}, "rules":[ - {"type":"pull_request","parameters":{"required_approving_review_count":2,"require_last_push_approval":true,"allowed_merge_methods":["squash"]}}, + {"type":"pull_request","parameters":{"required_approving_review_count":2,"require_last_push_approval":true,"allowed_merge_methods":["squash"],"required_reviewers":[],"dismissal_restriction":{"enabled":false,"allowed_actors":[]}}}, {"type":"provider_future_rule","parameters":{"opaque":{"keep":true}}}, {"type":"required_status_checks","parameters":{"required_status_checks":[{"context":"old"}],"strict_required_status_checks_policy":false,"do_not_enforce_on_create":true}} ]}`)} - desired := RepositoryRuleset{ID: 9, Name: "gds-main", Target: "branch", Enforcement: "active", Rules: []RulesetRule{{ - Type: "required_status_checks", RequiredStatusChecks: []RequiredStatusCheck{{Context: "generated / required"}}, - StrictRequiredStatusChecksPolicy: true, - }}} + desired := RepositoryRuleset{ID: 9, Name: "gds-main", Target: "branch", Enforcement: "active", Rules: []RulesetRule{ + { + Type: "pull_request", DismissStaleReviewsOnPush: true, + RequiredReviewThreadResolution: true, + RequireExtraApprovalForUnattributedChanges: false, + AllowedMergeMethods: []string{"merge"}, + }, + { + Type: "required_status_checks", RequiredStatusChecks: []RequiredStatusCheck{{Context: "generated / required"}}, + StrictRequiredStatusChecksPolicy: true, + }, + }} if _, _, err := repository.UpsertDefaultBranchRuleset(context.Background(), desired, ¤t); err != nil { t.Fatal(err) } @@ -291,6 +300,12 @@ func TestRepositoryRulesetUpdatePreservesExternallyManagedPayload(t *testing.T) rules[2].(map[string]any)["type"] != "required_status_checks" { t.Fatalf("rule order/external rules changed: %#v", rules) } + pullParameters := rules[0].(map[string]any)["parameters"].(map[string]any) + if pullParameters["require_extra_approval_for_unattributed_changes"] != false || + !reflect.DeepEqual(pullParameters["allowed_merge_methods"], []any{"merge"}) || + pullParameters["required_reviewers"] == nil || pullParameters["dismissal_restriction"] == nil { + t.Fatalf("pull-request owned/external merge is not lossless: %#v", pullParameters) + } } func mutationTestMutator(