Skip to content

Commit 9b1946b

Browse files
fix(policy): reevaluate approval status promptly (#13)
1 parent ae5ef21 commit 9b1946b

13 files changed

Lines changed: 490 additions & 31 deletions

config/policy-bot.example.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,11 @@ sessions:
156156
# # environment variable.
157157
# pending_as_failure: false
158158
#
159+
# # How long to coalesce rapid status/check/workflow events for the same PR
160+
# # before running the trailing evaluation. Can also be set by the
161+
# # POLICYBOT_OPTIONS_STATUS_DEBOUNCE_WINDOW environment variable.
162+
# status_debounce_window: 5s
163+
#
159164
# # Default options to use for all appoval rules processed by the server.
160165
# # Policies can override these values with their own defaults or in individual
161166
# # rules. This is equivalent to the "approval_defaults" option in a policy and

policy/approval/approve.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,18 @@ type Requires struct {
4343
Conditions predicate.Predicates `yaml:"conditions,omitempty"`
4444
}
4545

46+
func (r Requires) needsApprovalCandidates() bool {
47+
return r.Count > 0 || r.Actors.Codeowners
48+
}
49+
4650
type Defaults struct {
4751
Options *Options `yaml:"options,omitempty"`
4852
}
4953

5054
func (r *Rule) Trigger() common.Trigger {
5155
t := common.TriggerCommit
5256

53-
if r.Requires.Count > 0 {
57+
if r.Requires.needsApprovalCandidates() {
5458
m := r.Options.GetMethods()
5559
if len(m.GetComments()) > 0 || len(m.GetCommentPatterns()) > 0 {
5660
t |= common.TriggerComment
@@ -213,7 +217,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []
213217
func (r *Rule) isApprovedByActors(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, []*common.Candidate, []common.OwnershipGroupResult, error) {
214218
log := zerolog.Ctx(ctx)
215219

216-
if r.Requires.Count <= 0 && !r.Requires.Actors.Codeowners {
220+
if !r.Requires.needsApprovalCandidates() {
217221
log.Debug().Msg("rule requires no approvals")
218222
return true, nil, nil, nil
219223
}
@@ -306,7 +310,7 @@ func (r *Rule) isApprovedByConditions(ctx context.Context, prctx pull.Context) (
306310
// FilteredCandidates returns the potential approval candidates and any
307311
// candidates that should be dimissed due to rule options.
308312
func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, []*common.Dismissal, error) {
309-
if r.Requires.Count <= 0 && !r.Requires.Actors.Codeowners {
313+
if !r.Requires.needsApprovalCandidates() {
310314
return nil, nil, nil
311315
}
312316

policy/approval/approve_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -874,6 +874,20 @@ func TestTrigger(t *testing.T) {
874874
assert.True(t, r.Trigger().Matches(common.TriggerReview), "expected %s to match %s", r.Trigger(), common.TriggerReview)
875875
})
876876

877+
t.Run("triggerReviewForCodeownersWithoutCount", func(t *testing.T) {
878+
r := &Rule{
879+
Options: Options{
880+
Defaults: &defaultOptions,
881+
},
882+
Requires: Requires{
883+
Actors: common.Actors{Codeowners: true},
884+
},
885+
}
886+
887+
assert.True(t, r.Trigger().Matches(common.TriggerCommit), "expected %s to match %s", r.Trigger(), common.TriggerCommit)
888+
assert.True(t, r.Trigger().Matches(common.TriggerReview), "expected %s to match %s", r.Trigger(), common.TriggerReview)
889+
})
890+
877891
t.Run("triggerReviewForGithubReviewCommentPatterns", func(t *testing.T) {
878892
r := &Rule{
879893
Options: Options{
@@ -928,6 +942,40 @@ func TestTrigger(t *testing.T) {
928942
})
929943
}
930944

945+
func TestRequiresNeedsApprovalCandidates(t *testing.T) {
946+
tests := map[string]struct {
947+
requires Requires
948+
expected bool
949+
}{
950+
"no approval requirements": {
951+
requires: Requires{},
952+
expected: false,
953+
},
954+
"explicit approval count": {
955+
requires: Requires{Count: 1},
956+
expected: true,
957+
},
958+
"codeowners without count": {
959+
requires: Requires{Actors: common.Actors{Codeowners: true}},
960+
expected: true,
961+
},
962+
"conditions only": {
963+
requires: Requires{
964+
Conditions: predicate.Predicates{
965+
HasStatus: predicate.NewHasStatus([]string{"ci/test"}, nil),
966+
},
967+
},
968+
expected: false,
969+
},
970+
}
971+
972+
for name, test := range tests {
973+
t.Run(name, func(t *testing.T) {
974+
assert.Equal(t, test.expected, test.requires.needsApprovalCandidates())
975+
})
976+
}
977+
}
978+
931979
func TestIsPendingOnConditionsOnly(t *testing.T) {
932980
tests := map[string]struct {
933981
rule Rule

server/config_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// Copyright 2026 Palantir Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package server
16+
17+
import (
18+
"testing"
19+
"time"
20+
21+
"github.com/palantir/policy-bot/server/handler"
22+
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
24+
)
25+
26+
func TestParseConfigStatusDebounceWindow(t *testing.T) {
27+
t.Setenv("POLICYBOT_ENV_PREFIX", "TEST_POLICYBOT_")
28+
29+
cfg, err := ParseConfig([]byte(`options:
30+
status_debounce_window: 3s
31+
`))
32+
require.NoError(t, err)
33+
34+
assert.Equal(t, 3*time.Second, cfg.Options.StatusDebounceWindow)
35+
}
36+
37+
func TestParseConfigStatusDebounceWindowDefault(t *testing.T) {
38+
t.Setenv("POLICYBOT_ENV_PREFIX", "TEST_POLICYBOT_")
39+
40+
cfg, err := ParseConfig([]byte(`options: {}`))
41+
require.NoError(t, err)
42+
43+
assert.Equal(t, handler.DefaultDebounceWindow, cfg.Options.StatusDebounceWindow)
44+
}

server/handler/base.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,14 @@ func (b *Base) PreparePRContext(ctx context.Context, installationID int64, pr *g
106106
}
107107

108108
func (b *Base) NewEvalContext(ctx context.Context, installationID int64, loc pull.Locator) (*EvalContext, error) {
109+
return b.newEvalContext(ctx, installationID, loc, nil)
110+
}
111+
112+
func (b *Base) newEvalContextWithConfig(ctx context.Context, installationID int64, loc pull.Locator, fetchedConfig FetchedConfig) (*EvalContext, error) {
113+
return b.newEvalContext(ctx, installationID, loc, &fetchedConfig)
114+
}
115+
116+
func (b *Base) newEvalContext(ctx context.Context, installationID int64, loc pull.Locator, fetchedConfig *FetchedConfig) (*EvalContext, error) {
109117
client, err := b.NewInstallationClient(installationID)
110118
if err != nil {
111119
return nil, err
@@ -122,17 +130,13 @@ func (b *Base) NewEvalContext(ctx context.Context, installationID int64, loc pul
122130
return nil, err
123131
}
124132

125-
// Start background fetches for commonly needed data
126-
// This overlaps API calls with the config fetch below
127-
if ghctx, ok := prctx.(*pull.GitHubContext); ok {
128-
ghctx.Prefetch()
129-
}
130-
131133
baseBranch, _ := prctx.Branches()
132134
owner := prctx.RepositoryOwner()
133135
repository := prctx.RepositoryName()
134136

135-
fetchedConfig := b.ConfigFetcher.ConfigForRepositoryBranch(ctx, client, owner, repository, baseBranch)
137+
if fetchedConfig == nil {
138+
fetchedConfig = github.Ptr(b.ConfigFetcher.ConfigForRepositoryBranch(ctx, client, owner, repository, baseBranch))
139+
}
136140

137141
return &EvalContext{
138142
Client: client,
@@ -142,7 +146,7 @@ func (b *Base) NewEvalContext(ctx context.Context, installationID int64, loc pul
142146
PublicURL: b.BaseConfig.PublicURL,
143147

144148
PullContext: prctx,
145-
Config: fetchedConfig,
149+
Config: *fetchedConfig,
146150
}, nil
147151
}
148152

0 commit comments

Comments
 (0)