fix(ruleset): preserve external pull parameters

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)

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)
+	}
+}

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

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, &current); 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(