Repository: palantir/policy-bot (go)

Issue:
review dismissal question
Hello,

I think I'm currently hitting an edge case introduced in: https://github.com/palantir/policy-bot/pull/577

As described in the bottom of that PR description:

> If a single approval satisfies rules A and B where A is invalidated by pushes and B is not, pushing a new commit will not dismiss the review (because it still satisfies B). This is the intended behavior, but it could confuse people. I think in practice conflicting rules like this are behind predicates.

Most of our rules are "reviews with comments" type of policies where the github review could potentially contain a comment satisfying multiple policy rules. So it is a bit surprising to us to see that a new commit being pushed post approval makes the policy go back to pending after the re-evaluation, but doesn't actually dismiss the github review. This is where in the original implementation, I was thinking the "reason" field could clearly state which candidate is the cause of the review being dismissed. 

Example policy:
```
policy:
  approval:
      - and:
          - domain review
          - platform review

approval_rules:
- name: domain review
  requires:
    count: 1
    teams:
    - "some-org/engineering"
  options:
    ignore_edited_comments: true
    invalidate_on_push: true
    allow_non_author_contributor: true
    methods:
      comments: []
      github_review: true
      github_review_comment_patterns:
        - '(?i)\bdomain\s*lgtm\b'
- name: platform review
  requires:
    count: 1
    teams:
    - "some-org/platform-reviewers"
  options:
    ignore_edited_comments: true
    invalidate_on_push: false
    allow_non_author_contributor: true
    methods:
      comments: []
      github_review: true
      github_review_comment_patterns:
        - '(?i)\bplatform\s*lgtm\b'
```

Is there a way to have it still dismiss the github review commenting the reason with this set up? You mention using predicates, is there a way to refactor this policy to accomplish what we want with predicates?

Pull request:
feat: add server option for strict review dismissals
This pull request introduces a new feature for strict review dismissal in the evaluation context. The most important changes include adding a new option for strict review dismissal and modifying the related logic to respect this new option.

New feature for strict review dismissal:

* [`server/handler/eval_options.go`](diffhunk://#diff-4c2f8588bb0dbf74803fffef6bbee50e7680c72899b0972fea6cf0ef80937130R45-R49): Added a new boolean field `StrictReviewDismissal` in the `PullEvaluationOptions` struct to enable unconditional GitHub dismissals for reviews associated with invalidated approval candidates.
* [`server/handler/eval_options.go`](diffhunk://#diff-4c2f8588bb0dbf74803fffef6bbee50e7680c72899b0972fea6cf0ef80937130R100): Updated the `SetValuesFromEnv` function to set the `StrictReviewDismissal` option from environment variables.

Logic modification for review dismissal:

* [`server/handler/eval_context_dismissal.go`](diffhunk://#diff-9a1aa4ae85323f98b833a9551817bf9388a01bfef333c49b28c08927f5512f2fR37-R42): Modified the `dismissStaleReviewsForResult` function to respect the new `StrictReviewDismissal` option, ensuring reviews are only dismissed if the option is enabled or the user is not currently an approver.

Closes #893 

Patch:
diff --git a/README.md b/README.md
index c36ecf1..b96999f 100644
--- a/README.md
+++ b/README.md
@@ -268,7 +268,44 @@ if:
     paths:
       - "^config/.*$"
 
-  # "file_not_deleted" is satisfied if none of the files matching any regular expression
+  # "file_added" is satisfied if any file matching any regular expression in the "paths"
+  # list has been added in the pull request, allowing enforcement of rules when
+  # specific types of files are added. If no matching files are added, the predicate
+  # fails.
+  #
+  # Note: Double-quote strings must escape backslashes while single/plain do not.
+  # See the Notes on YAML Syntax section of this README for more information.
+  file_added:
+    paths:
+      - "^config/.*\\.yaml$"
+      - "^server/.*\\.go$"
+
+  # "file_not_added" is the negation of "file_added". This predicate is
+  # satisfied if none of the files matching any regular expression
+  # in the "paths" list have been added in the pull request. If any matching file
+  # was added, the predicate fails, allowing rules to be skipped that depend on
+  # files not being added.
+  #
+  # Note: Double-quote strings must escape backslashes while single/plain do not.
+  # See the Notes on YAML Syntax section of this README for more information.
+  file_not_added:
+    paths:
+      - "^deprecated/.*$"
+
+  # "file_deleted" is satisfied if any file matching any regular expression in the "paths"
+  # list has been deleted in the pull request, allowing enforcement of rules when
+  # specific types of files are deleted. If no matching files are deleted, the
+  # predicate fails.
+  #
+  # Note: Double-quote strings must escape backslashes while single/plain do not.
+  # See the Notes on YAML Syntax section of this README for more information.
+  file_deleted:
+    paths:
+      - "^legacy/.*\\.js$"
+      - "^deprecated/.*\\.go$"
+
+  # "file_not_deleted" is the negation of "file_deleted". This predicate is
+  # satisfied if none of the files matching any regular expression
   # in the "paths" list have been deleted in the pull request. If any matching file
   # was deleted, the predicate fails, allowing rules that depend on the file's existence
   # to be skipped.
diff --git a/policy/predicate/file.go b/policy/predicate/file.go
index ae8c7db..b012083 100644
--- a/policy/predicate/file.go
+++ b/policy/predicate/file.go
@@ -25,6 +25,14 @@ import (
 	"github.com/pkg/errors"
 )
 
+func getPathStrings(patterns []common.Regexp) []string {
+	var paths []string
+	for _, r := range patterns {
+		paths = append(paths, r.String())
+	}
+	return paths
+}
+
 type ChangedFiles struct {
 	Paths       []common.Regexp `yaml:"paths,omitempty"`
 	IgnorePaths []common.Regexp `yaml:"ignore,omitempty"`
@@ -33,15 +41,8 @@ type ChangedFiles struct {
 var _ Predicate = &ChangedFiles{}
 
 func (pred *ChangedFiles) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
-	var paths, ignorePaths []string
-
-	for _, path := range pred.Paths {
-		paths = append(paths, path.String())
-	}
-
-	for _, ignorePath := range pred.IgnorePaths {
-		ignorePaths = append(ignorePaths, ignorePath.String())
-	}
+	paths := getPathStrings(pred.Paths)
+	ignorePaths := getPathStrings(pred.IgnorePaths)
 
 	predicateResult := common.PredicateResult{
 		ValuePhrase:     "changed files",
@@ -92,11 +93,7 @@ type OnlyChangedFiles struct {
 var _ Predicate = &OnlyChangedFiles{}
 
 func (pred *OnlyChangedFiles) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
-	var paths []string
-
-	for _, path := range pred.Paths {
-		paths = append(paths, path.String())
-	}
+	paths := getPathStrings(pred.Paths)
 
 	predicateResult := common.PredicateResult{
 		ValuePhrase:     "changed files",
@@ -181,25 +178,66 @@ func (pred *NoChangedFiles) Trigger() common.Trigger {
 	return common.TriggerCommit
 }
 
-type FileNotDeleted struct {
+type FileAdded struct {
 	Paths []common.Regexp `yaml:"paths"`
 }
 
-var _ Predicate = &FileNotDeleted{}
+var _ Predicate = &FileAdded{}
 
-func (pred *FileNotDeleted) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
-	var paths []string
-	for _, r := range pred.Paths {
-		paths = append(paths, r.String())
+func (pred *FileAdded) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
+	paths := getPathStrings(pred.Paths)
+	predicateResult := common.PredicateResult{
+		Satisfied:       false,
+		ValuePhrase:     "added files",
+		Values:          []string{},
+		ConditionPhrase: "match path patterns",
+		ConditionValues: paths,
 	}
 
+	changedFiles, err := prctx.ChangedFiles()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to list changed files")
+	}
+
+	addedFiles := []string{}
+	for _, f := range changedFiles {
+		if f.Status == pull.FileAdded {
+			addedFiles = append(addedFiles, f.Filename)
+
+			if anyMatches(pred.Paths, f.Filename) {
+				predicateResult.Satisfied = true
+				predicateResult.Values = []string{f.Filename}
+				predicateResult.Description = f.Filename + " was added"
+				return &predicateResult, nil
+			}
+		}
+	}
+
+	predicateResult.Values = addedFiles
+	predicateResult.Description = "No added files match the specified patterns"
+	return &predicateResult, nil
+}
+
+func (pred *FileAdded) Trigger() common.Trigger {
+	return common.TriggerCommit
+}
+
+type FileNotAdded struct {
+	Paths []common.Regexp `yaml:"paths"`
+}
+
+var _ Predicate = &FileNotAdded{}
+
+func (pred *FileNotAdded) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
+	paths := getPathStrings(pred.Paths)
+
 	predicateResult := common.PredicateResult{
 		Satisfied:         true,
-		ValuePhrase:       "deleted files",
+		ValuePhrase:       "added files",
 		Values:            []string{},
 		ConditionPhrase:   "match path patterns",
-		ReverseSkipPhrase: true,
 		ConditionValues:   paths,
+		ReverseSkipPhrase: true,
 	}
 
 	changedFiles, err := prctx.ChangedFiles()
@@ -207,9 +245,94 @@ func (pred *FileNotDeleted) Evaluate(ctx context.Context, prctx pull.Context) (*
 		return nil, errors.Wrap(err, "failed to list changed files")
 	}
 
-	if len(changedFiles) == 0 {
-		predicateResult.Description = "No files were changed"
-		return &predicateResult, nil
+	addedFiles := []string{}
+	for _, f := range changedFiles {
+		if f.Status == pull.FileAdded {
+			addedFiles = append(addedFiles, f.Filename)
+
+			if anyMatches(pred.Paths, f.Filename) {
+				predicateResult.Satisfied = false
+				predicateResult.Values = []string{f.Filename}
+				predicateResult.Description = f.Filename + " was added"
+				return &predicateResult, nil
+			}
+		}
+	}
+
+	predicateResult.Values = addedFiles
+	predicateResult.Description = "No added files match the specified patterns"
+	return &predicateResult, nil
+}
+
+func (pred *FileNotAdded) Trigger() common.Trigger {
+	return common.TriggerCommit
+}
+
+type FileDeleted struct {
+	Paths []common.Regexp `yaml:"paths"`
+}
+
+var _ Predicate = &FileDeleted{}
+
+func (pred *FileDeleted) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
+	paths := getPathStrings(pred.Paths)
+
+	predicateResult := common.PredicateResult{
+		Satisfied:       false,
+		ValuePhrase:     "deleted files",
+		Values:          []string{},
+		ConditionPhrase: "match path patterns",
+		ConditionValues: paths,
+	}
+
+	changedFiles, err := prctx.ChangedFiles()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to list changed files")
+	}
+
+	deletedFiles := []string{}
+	for _, f := range changedFiles {
+		if f.Status == pull.FileDeleted {
+			deletedFiles = append(deletedFiles, f.Filename)
+
+			if anyMatches(pred.Paths, f.Filename) {
+				predicateResult.Satisfied = true
+				predicateResult.Values = []string{f.Filename}
+				predicateResult.Description = f.Filename + " was deleted"
+				return &predicateResult, nil
+			}
+		}
+	}
+
+	predicateResult.Values = deletedFiles
+	predicateResult.Description = "No deleted files match the specified patterns"
+	return &predicateResult, nil
+}
+
+func (pred *FileDeleted) Trigger() common.Trigger {
+	return common.TriggerCommit
+}
+
+type FileNotDeleted struct {
+	Paths []common.Regexp `yaml:"paths"`
+}
+
+var _ Predicate = &FileNotDeleted{}
+
+func (pred *FileNotDeleted) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
+	paths := getPathStrings(pred.Paths)
+	predicateResult := common.PredicateResult{
+		Satisfied:         true,
+		ValuePhrase:       "deleted files",
+		Values:            []string{},
+		ConditionPhrase:   "match path patterns",
+		ConditionValues:   paths,
+		ReverseSkipPhrase: true,
+	}
+
+	changedFiles, err := prctx.ChangedFiles()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to list changed files")
 	}
 
 	deletedFiles := []string{}
diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go
index 847e435..de28c29 100644
--- a/policy/predicate/predicates.go
+++ b/policy/predicate/predicates.go
@@ -18,6 +18,9 @@ type Predicates struct {
 	ChangedFiles     *ChangedFiles     `yaml:"changed_files,omitempty"`
 	NoChangedFiles   *NoChangedFiles   `yaml:"no_changed_files,omitempty"`
 	OnlyChangedFiles *OnlyChangedFiles `yaml:"only_changed_files,omitempty"`
+	FileAdded        *FileAdded        `yaml:"file_added,omitempty"`
+	FileNotAdded     *FileNotAdded     `yaml:"file_not_added,omitempty"`
+	FileDeleted      *FileDeleted      `yaml:"file_deleted,omitempty"`
 	FileNotDeleted   *FileNotDeleted   `yaml:"file_not_deleted,omitempty"`
 
 	HasAuthorIn             *HasAuthorIn             `yaml:"has_author_in,omitempty"`
@@ -64,6 +67,15 @@ func (p *Predicates) Predicates() []Predicate {
 	if p.OnlyChangedFiles != nil {
 		ps = append(ps, Predicate(p.OnlyChangedFiles))
 	}
+	if p.FileAdded != nil {
+		ps = append(ps, Predicate(p.FileAdded))
+	}
+	if p.FileNotAdded != nil {
+		ps = append(ps, Predicate(p.FileNotAdded))
+	}
+	if p.FileDeleted != nil {
+		ps = append(ps, Predicate(p.FileDeleted))
+	}
 	if p.FileNotDeleted != nil {
 		ps = append(ps, Predicate(p.FileNotDeleted))
 	}
diff --git a/server/handler/eval_context_dismissal.go b/server/handler/eval_context_dismissal.go
index 03543b9..1e3cd38 100644
--- a/server/handler/eval_context_dismissal.go
+++ b/server/handler/eval_context_dismissal.go
@@ -34,9 +34,11 @@ func (ec *EvalContext) dismissStaleReviewsForResult(ctx context.Context, result
 		if d.Candidate.Type != common.ReviewCandidate {
 			continue
 		}
-		// Only dismiss reviews from users who are not currently approvers
-		if approvers[d.Candidate.User] {
-			continue
+		if !ec.Options.StrictReviewDismissal {
+			// Only dismiss reviews from users who are not currently approvers
+			if approvers[d.Candidate.User] {
+				continue
+			}
 		}
 		// Only dismiss reviews once if they're dismissed by multiple rules
 		if alreadyDismissed[d.Candidate.ReviewID] {
diff --git a/server/handler/eval_options.go b/server/handler/eval_options.go
index 19f9663..b6cbe31 100644
--- a/server/handler/eval_options.go
+++ b/server/handler/eval_options.go
@@ -42,6 +42,11 @@ type PullEvaluationOptions struct {
 	// is otherwise private. See the README for details.
 	ExpandRequiredReviewers bool `yaml:"expand_required_reviewers"`
 
+	// StrictReviewDismissal enables sending unconditional GitHub dismissals
+	// for reviews associated with rules of invalidated approval candidates
+	// even if that same approval candidate satifies another rule.
+	StrictReviewDismissal bool `yaml:"strict_review_dismissal"`
+
 	// PostInsecureStatusChecks enables the sending of a second status using just StatusCheckContext as the context,
 	// no templating. This is turned off by default. This is to support legacy workflows that depend on the original
 	// context behaviour, and will be removed in 2.0
@@ -92,6 +97,7 @@ func (p *PullEvaluationOptions) SetValuesFromEnv(prefix string) {
 	setStringPtrFromEnv("SHARED_POLICY_PATH", prefix, &p.SharedPolicyPath)
 	setStringFromEnv("STATUS_CHECK_CONTEXT", prefix, &p.StatusCheckContext)
 	setBoolFromEnv("EXPAND_REQUIRED_REVIEWERS", prefix, &p.ExpandRequiredReviewers)
+	setBoolFromEnv("STRICT_REVIEW_DISMISSAL", prefix, &p.StrictReviewDismissal)
 	setBoolFromEnv("POST_INSECURE_STATUS_CHECKS", prefix, &p.PostInsecureStatusChecks)
 	p.fillDefaults()
 }


Test patch:
diff --git a/policy/predicate/file_test.go b/policy/predicate/file_test.go
index 7417d51..bc19f44 100644
--- a/policy/predicate/file_test.go
+++ b/policy/predicate/file_test.go
@@ -453,6 +453,245 @@ func TestFileNotDeleted(t *testing.T) {
 	})
 }
 
+func TestFileAdded(t *testing.T) {
+	p := &FileAdded{
+		Paths: []common.Regexp{
+			common.NewCompiledRegexp(regexp.MustCompile("app/.*\\.go")),
+			common.NewCompiledRegexp(regexp.MustCompile("server/.*\\.go")),
+		},
+	}
+
+	runFileTests(t, p, []FileTestCase{
+		{
+			"no files",
+			[]*pull.File{},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+		{
+			"matching file added",
+			[]*pull.File{
+				{
+					Filename: "app/client.go",
+					Status:   pull.FileAdded,
+				},
+				{
+					Filename: "other/file.go",
+					Status:   pull.FileAdded,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       true,
+				Values:          []string{"app/client.go"},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+		{
+			"no matching files added",
+			[]*pull.File{
+				{
+					Filename: "other/file1.go",
+					Status:   pull.FileAdded,
+				},
+				{
+					Filename: "other/file2.go",
+					Status:   pull.FileAdded,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{"other/file1.go", "other/file2.go"},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+		{
+			"files exist but modified",
+			[]*pull.File{
+				{
+					Filename: "app/client.go",
+					Status:   pull.FileModified,
+				},
+				{
+					Filename: "server/server.go",
+					Status:   pull.FileModified,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+	})
+}
+
+func TestFileDeleted(t *testing.T) {
+	p := &FileDeleted{
+		Paths: []common.Regexp{
+			common.NewCompiledRegexp(regexp.MustCompile("app/.*\\.go")),
+			common.NewCompiledRegexp(regexp.MustCompile("server/.*\\.go")),
+		},
+	}
+
+	runFileTests(t, p, []FileTestCase{
+		{
+			"no files",
+			[]*pull.File{},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+		{
+			"matching file deleted",
+			[]*pull.File{
+				{
+					Filename: "app/client.go",
+					Status:   pull.FileDeleted,
+				},
+				{
+					Filename: "other/file.go",
+					Status:   pull.FileDeleted,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       true,
+				Values:          []string{"app/client.go"},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+		{
+			"no matching files deleted",
+			[]*pull.File{
+				{
+					Filename: "other/file1.go",
+					Status:   pull.FileDeleted,
+				},
+				{
+					Filename: "other/file2.go",
+					Status:   pull.FileDeleted,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{"other/file1.go", "other/file2.go"},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+		{
+			"files exist but modified",
+			[]*pull.File{
+				{
+					Filename: "app/client.go",
+					Status:   pull.FileModified,
+				},
+				{
+					Filename: "server/server.go",
+					Status:   pull.FileModified,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{},
+				ConditionValues: []string{"app/.*\\.go", "server/.*\\.go"},
+			},
+		},
+	})
+}
+
+func TestFileNotAdded(t *testing.T) {
+	p := &FileNotAdded{
+		Paths: []common.Regexp{
+			common.NewCompiledRegexp(regexp.MustCompile("workflows/.*\\.yaml")),
+			common.NewCompiledRegexp(regexp.MustCompile("actions/.*\\.yaml")),
+		},
+	}
+
+	runFileTests(t, p, []FileTestCase{
+		{
+			"file not changed",
+			[]*pull.File{},
+			&common.PredicateResult{
+				Satisfied:       true,
+				Values:          []string{},
+				ConditionValues: []string{"workflows/.*\\.yaml", "actions/.*\\.yaml"},
+			},
+		},
+		{
+			"files exist and modified",
+			[]*pull.File{
+				{
+					Filename: "workflows/workflow.yaml",
+					Status:   pull.FileModified,
+				},
+				{
+					Filename: "actions/action.yaml",
+					Status:   pull.FileModified,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       true,
+				Values:          []string{},
+				ConditionValues: []string{"workflows/.*\\.yaml", "actions/.*\\.yaml"},
+			},
+		},
+		{
+			"one file added",
+			[]*pull.File{
+				{
+					Filename: "workflows/workflow.yaml",
+					Status:   pull.FileAdded,
+				},
+				{
+					Filename: "actions/action.yaml",
+					Status:   pull.FileModified,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{"workflows/workflow.yaml"},
+				ConditionValues: []string{"workflows/.*\\.yaml", "actions/.*\\.yaml"},
+			},
+		},
+		{
+			"multiple files added",
+			[]*pull.File{
+				{
+					Filename: "workflows/workflow.yaml",
+					Status:   pull.FileAdded,
+				},
+				{
+					Filename: "actions/action.yaml",
+					Status:   pull.FileAdded,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       false,
+				Values:          []string{"workflows/workflow.yaml"},
+				ConditionValues: []string{"workflows/.*\\.yaml", "actions/.*\\.yaml"},
+			},
+		},
+		{
+			"file added not matching",
+			[]*pull.File{
+				{
+					Filename: "some/otherfile.yaml",
+					Status:   pull.FileAdded,
+				},
+			},
+			&common.PredicateResult{
+				Satisfied:       true,
+				Values:          []string{"some/otherfile.yaml"},
+				ConditionValues: []string{"workflows/.*\\.yaml", "actions/.*\\.yaml"},
+			},
+		},
+	})
+}
+
 func TestModifiedLines(t *testing.T) {
 	p := &ModifiedLines{
 		Additions: ComparisonExpr{Op: OpGreaterThan, Value: 100},