Add support for allowing skipped checks in has_successful_status

The `has_successful_status` predicate currently requires predicates to
be present and successful in order to pass. But workflows can be run
conditionally - for example only if certain paths change - and it is
currently not very convenient to write a policy which considers such
skipped workflows as passing. The only feasible workaround is to
duplicate the path filters in the policy, and this quickly gets unwieldy
and prone to getting out-of-sync in large repositories.

Here we add direct support for specifying such rules. This is done by
introducing a new alernative form that `has_successful_status`
predicates can take:

```yaml
has_successful_status:
  options:
    skipped_is_success: true
  statuses:
    - "status 1"
    - "status 2"
```

In this mode, we will consider the `skipped` result as acceptable. The
current form:

```yaml
has_successful_status:
  - "status 1"
  - "status 2"
```

remains supported. We have done this by implementing a custom
unmarshaling function to be able to handle both forms.

Closes: #760

README.md

@@ -329,6 +329,18 @@ if:
     - "status-name-2"
     - "status-name-3"
 
+  # "has_successful_status" can be configured to count "skipped" statuses as
+  # successful. This can be useful in combination with path filters where
+  # workflows only run on parts of the tree. They are required to succeed only
+  # if they run.
+  # has_successful_status:
+  #  options:
+  #    skipped_is_success: true
+  #  statuses:
+  #    - "status-name-1"
+  #    - "status-name-2"
+  #    - "status-name-3"
+
   # "has_labels" is satisfied if the pull request has the specified labels
   # applied
   has_labels:

policy/approval/approve.go

@@ -48,6 +48,8 @@ type Options struct {
 
 	RequestReview RequestReview `yaml:"request_review"`
 
+	CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"`
+
 	Methods *common.Methods `yaml:"methods"`
 }
 

policy/approval/approve_test.go

@@ -676,7 +676,7 @@ func TestIsApproved(t *testing.T) {
 		r := &Rule{
 			Requires: Requires{
 				Conditions: predicate.Predicates{
-					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"deploy"},
+					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"deploy"}},
 				},
 			},
 		}
@@ -688,7 +688,7 @@ func TestIsApproved(t *testing.T) {
 		r := &Rule{
 			Requires: Requires{
 				Conditions: predicate.Predicates{
-					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"},
+					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}},
 				},
 			},
 		}
@@ -701,7 +701,7 @@ func TestIsApproved(t *testing.T) {
 			Requires: Requires{
 				Count: 1,
 				Conditions: predicate.Predicates{
-					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"},
+					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}},
 				},
 			},
 		}
@@ -717,7 +717,7 @@ func TestIsApproved(t *testing.T) {
 					Organizations: []string{"everyone"},
 				},
 				Conditions: predicate.Predicates{
-					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"},
+					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}},
 				},
 			},
 		}
@@ -825,7 +825,7 @@ func TestTrigger(t *testing.T) {
 		r := &Rule{
 			Requires: Requires{
 				Conditions: predicate.Predicates{
-					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"},
+					HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}},
 				},
 			},
 		}

policy/approval/parse_test.go

@@ -35,6 +35,9 @@ func TestParsePolicy(t *testing.T) {
 - and:
   - rule6
   - rule7
+  - or:
+    - rule8
+    - rule9
 `
 
 	ruleText := `
@@ -67,6 +70,16 @@ func TestParsePolicy(t *testing.T) {
       enabled: true
   requires:
     admins: true
+- name: rule8
+  if:
+    has_successful_status:
+      - status1
+- name: rule9
+  if:
+    has_successful_status:
+      statuses: [status2, status3]
+      options:
+        skipped_is_success: true
 `
 
 	var policy Policy
@@ -119,6 +132,16 @@ func TestParsePolicy(t *testing.T) {
 						&RuleRequirement{
 							rule: rules[6],
 						},
+						&OrRequirement{
+							requirements: []common.Evaluator{
+								&RuleRequirement{
+									rule: rules[7],
+								},
+								&RuleRequirement{
+									rule: rules[8],
+								},
+							},
+						},
 					},
 				},
 			},

policy/predicate/status.go

@@ -23,30 +23,67 @@ import (
 	"github.com/pkg/errors"
 )
 
-type HasSuccessfulStatus []string
+type hasSuccessfulStatusOptions struct {
+	SkippedIsSuccess bool `yaml:"skipped_is_success"`
+}
+
+type HasSuccessfulStatus struct {
+	Options hasSuccessfulStatusOptions
+	Statuses []string `yaml:"statuses"`
+}
+
+func NewHasSuccessfulStatus(statuses []string) *HasSuccessfulStatus {
+	return &HasSuccessfulStatus{
+		Statuses: statuses,
+	}
+}
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface for HasSuccessfulStatus.
+// This allows the predicate to be specified as either a list of strings or with options.
+func (pred *HasSuccessfulStatus) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	// Try to unmarshal as a list of strings first
+	statuses := []string{}
+	if err := unmarshal(&statuses); err == nil {
+		pred.Statuses = statuses
+
+		return nil
+	}
 
-var _ Predicate = HasSuccessfulStatus([]string{})
+	// If that fails, try to unmarshal as the full structure
+	type rawHasSuccessfulStatus HasSuccessfulStatus
+	return unmarshal((*rawHasSuccessfulStatus)(pred))
+}
+
+var _ Predicate = HasSuccessfulStatus{}
 
 func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
 	statuses, err := prctx.LatestStatuses()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to list commit statuses")
+	}
+
+	allowedStatusConclusions := map[string]struct{}{
+		"success": {},
+	}
 
 	predicateResult := common.PredicateResult{
 		ValuePhrase:     "status checks",
 		ConditionPhrase: "exist and pass",
 	}
 
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to list commit statuses")
+	if pred.Options.SkippedIsSuccess {
+		predicateResult.ConditionPhrase += " or are skipped"
+		allowedStatusConclusions["skipped"] = struct{}{}
 	}
 
 	var missingResults []string
 	var failingStatuses []string
-	for _, status := range pred {
+	for _, status := range pred.Statuses {
 		result, ok := statuses[status]
 		if !ok {
 			missingResults = append(missingResults, status)
 		}
-		if result != "success" {
+		if _, allowed := allowedStatusConclusions[result]; !allowed {
 			failingStatuses = append(failingStatuses, status)
 		}
 	}
@@ -65,7 +102,7 @@ func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context
 		return &predicateResult, nil
 	}
 
-	predicateResult.Values = pred
+	predicateResult.Values = pred.Statuses
 	predicateResult.Satisfied = true
 	return &predicateResult, nil
 }

policy/predicate/status_test.go

@@ -16,6 +16,7 @@ package predicate
 
 import (
 	"context"
+	"slices"
 	"testing"
 
 	"github.com/palantir/policy-bot/policy/common"
@@ -24,10 +25,21 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func keysSorted[V any](m map[string]V) []string {
+	r := make([]string, 0, len(m))
+
+	for k := range m {
+		r = append(r, k)
+	}
+
+	slices.Sort(r)
+	return r
+}
+
 func TestHasSuccessfulStatus(t *testing.T) {
-	p := HasSuccessfulStatus([]string{"status-name", "status-name-2"})
+	p := HasSuccessfulStatus{Statuses: []string{"status-name", "status-name-2"}}
 
-	runStatusTestCase(t, p, []StatusTestCase{
+	commonTestCases := []StatusTestCase{
 		{
 			"all statuses succeed",
 			&pulltest.Context{
@@ -38,7 +50,6 @@ func TestHasSuccessfulStatus(t *testing.T) {
 			},
 			&common.PredicateResult{
 				Satisfied: true,
-				Values:    []string{"status-name", "status-name-2"},
 			},
 		},
 		{
@@ -79,6 +90,18 @@ func TestHasSuccessfulStatus(t *testing.T) {
 				Values:    []string{"status-name-2"},
 			},
 		},
+		{
+			"a status does not exist, the other status is skipped",
+			&pulltest.Context{
+				LatestStatusesValue: map[string]string{
+					"status-name-2": "skipped",
+				},
+			},
+			&common.PredicateResult{
+				Satisfied: false,
+				Values:    []string{"status-name"},
+			},
+		},
 		{
 			"multiple statuses do not exist",
 			&pulltest.Context{},
@@ -87,7 +110,53 @@ func TestHasSuccessfulStatus(t *testing.T) {
 				Values:    []string{"status-name", "status-name-2"},
 			},
 		},
-	})
+	}
+
+	okOnlyIfSkippedAllowed := []StatusTestCase{
+		{
+			"a status is skipped",
+			&pulltest.Context{
+				LatestStatusesValue: map[string]string{
+					"status-name":   "success",
+					"status-name-2": "skipped",
+				},
+			},
+			&common.PredicateResult{
+				Satisfied: false,
+				Values:    []string{"status-name-2"},
+			},
+		},
+		{
+			"all statuses are skipped",
+			&pulltest.Context{
+				LatestStatusesValue: map[string]string{
+					"status-name":   "skipped",
+					"status-name-2": "skipped",
+				},
+			},
+			&common.PredicateResult{
+				Satisfied: false,
+				Values:    []string{"status-name", "status-name-2"},
+			},
+		},
+	}
+
+	// Run tests with skipped statuses counting as failures
+	runStatusTestCase(t, p, commonTestCases)
+	runStatusTestCase(t, p, okOnlyIfSkippedAllowed)
+
+	// Run tests with skipped statuses counting as successes
+	p.Options.SkippedIsSuccess = true
+
+	for i := 0; i < len(commonTestCases); i++ {
+		commonTestCases[i].name += ", but skipped statuses are allowed"
+	}
+	for i := 0; i < len(okOnlyIfSkippedAllowed); i++ {
+		okOnlyIfSkippedAllowed[i].name += ", but skipped statuses are allowed"
+		okOnlyIfSkippedAllowed[i].ExpectedPredicateResult.Satisfied = true
+	}
+	runStatusTestCase(t, p, commonTestCases)
+	runStatusTestCase(t, p, okOnlyIfSkippedAllowed)
 }
 
 type StatusTestCase struct {
@@ -100,6 +169,15 @@ func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) {
 	ctx := context.Background()
 
 	for _, tc := range cases {
+		// If the test case expects the predicate to be satisfied, we always
+		// expect the values to contain all the statuses. Doing this here lets
+		// us use the same testcases when we allow and don't allow skipped
+		// statuses.
+		if tc.ExpectedPredicateResult.Satisfied {
+			statuses, _ := tc.context.LatestStatuses()
+			tc.ExpectedPredicateResult.Values = keysSorted(statuses)
+		}
+
 		t.Run(tc.name, func(t *testing.T) {
 			predicateResult, err := p.Evaluate(ctx, tc.context)
 			if assert.NoError(t, err, "evaluation failed") {