diff --git a/core/githubruleset/adoption.go b/core/githubruleset/adoption.go index fdc11f6..ee17f59 100644 --- a/core/githubruleset/adoption.go +++ b/core/githubruleset/adoption.go @@ -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 { diff --git a/core/githubruleset/ruleset.go b/core/githubruleset/ruleset.go index 3f3a60a..f35a81b 100644 --- a/core/githubruleset/ruleset.go +++ b/core/githubruleset/ruleset.go @@ -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 { @@ -451,7 +463,7 @@ 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 { @@ -459,35 +471,60 @@ func applyOwnedState(current githubprovider.RepositoryRulesetState, desired gith 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.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( diff --git a/core/providers/github/mutation_ruleset.go b/core/providers/github/mutation_ruleset.go index 2eafe9d..6dd6bd1 100644 --- a/core/providers/github/mutation_ruleset.go +++ b/core/providers/github/mutation_ruleset.go @@ -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"` @@ -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 { @@ -159,24 +162,23 @@ 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 { @@ -184,17 +186,22 @@ func replaceOwnedRulesetChecks(payload map[string]any, desired []RulesetRule) er 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 @@ -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{ diff --git a/core/providers/github/ruleset_live_shape_test.go b/core/providers/github/ruleset_live_shape_test.go index 632bade..53efb7a 100644 --- a/core/providers/github/ruleset_live_shape_test.go +++ b/core/providers/github/ruleset_live_shape_test.go @@ -55,7 +55,9 @@ 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. @@ -63,7 +65,7 @@ func TestCurrentLiveRulesetShapeIsObservable(t *testing.T) { 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) } @@ -71,6 +73,42 @@ func TestCurrentLiveRulesetShapeIsObservable(t *testing.T) { 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) { diff --git a/core/providers/github/ruleset_observation.go b/core/providers/github/ruleset_observation.go index 1141753..5858a72 100644 --- a/core/providers/github/ruleset_observation.go +++ b/core/providers/github/ruleset_observation.go @@ -238,18 +238,21 @@ 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 @@ -257,6 +260,9 @@ func normalizeRulesetRule(ruleType string, parameters json.RawMessage) (RulesetR 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"`