policy-bot icon indicating copy to clipboard operation
policy-bot copied to clipboard

Certain merges can lead to ignored commits during evaluation

Open bluekeyes opened this issue 8 months ago • 0 comments

We recently found an internal PR where someone had effectively merged a branch with itself. When processing approvals for the PR, the sortCommits function

https://github.com/palantir/policy-bot/blob/55d042dbd89e1124d53be5ed05c89fbddc901c4b/policy/approval/approve.go#L449-L468

orders commits by their parents, only using the first parent of each commit. Any commits not in that first parent chain are discarded from the result. This works if the second parent is always another branch, but if both parents are present on the PR branch, only exploring the first path can discard commits.

The result was that a user who should have been a contributor was able to approve the PR, because their commits were only reachable through the second parent of a merge commit.

I think we need to update sortCommits to make sure the result always contains every commit in the input. However, I'm not sure yet how to order commits in this situation (e.g. should we evaluate parents depth-first or breadth-first?). This sorting function was introduced as part of #602, so we'll need to make sure than any ordering is valid for the invalidate_on_push option.

bluekeyes avatar Oct 20 '23 17:10 bluekeyes