has_status bug - PR merged without approval

[rajesh-dhakad]

Hi Team - We encountered a situation where PR merged without meeting the policy bot's criteria.

The policy bot's current status is still Pending but PR has been merged into the master.

Eng has enabled auto merge and rebased the feature branch with master ( merging master into his feature branch ) and the policy bot status changed to Approved and PR got merged - without meeting the criteria of policy bot, you can see in above screenshot - which shows current status of the policy bot.

[bluekeyes]

Hey @rajesh-dhakad, sorry you ran in to this issue. Can you please share your full YAML policy definition, redacting any sensitive team names or other information?

Note that when you view the details page for a pul request, Policy Bot re-evaluates the status using the current policy and commit state, which may be different from the state when the pull request merged. My guess here is that there is some combination of rules in your policy that can allow unintentional approval, but it's hard to confirm that without seeing the full definition.

[rajesh-dhakad]

Hey @bluekeyes - Please see below policy details:

# the high level policy
policy:
  approval:
    - and:
      - db-migration squad approved or self-review passed
      - non-migration files exists - db-migration squad approved or self-review passed
      - non-migration files exists - addition review from respective squad
      - or:
          - the engineering team has approved
          - an admin approved

    requires:
      organizations:
        - "myorg"

# the list of rules
approval_rules:
  - name: the engineering team has approved
    options:
      allow_contributor: true
    requires:
      count: 1
      teams:
        - "myorg/eng"
  - name: an admin approved
    options:
      allow_contributor: true
    requires:
      count: 1
      admins: true
  - name: db-migration squad approved or self-review passed
    if:
      has_status:
        conclusions: ["failed", "success"]
        statuses:
          - "review-migration-pr"
      changed_files:
        paths:
          - "^db-migration-files/.*$"
    requires:
      count: 1
      teams:
        - "myorg/db-team"
  - name: non-migration files exists - db-migration squad approved or self-review passed
    if:
      has_status:
        conclusions: ["failed", "success"]
        statuses:
          - "review-non-migration-pr"
      changed_files:
        paths:
          - "^db-migration-files/.*$"
    requires:
      count: 1
      teams:
        - "myorg/db-team"
  - name: non-migration files exists - addition review from respective squad
    if:
      has_status:
        conclusions: ["failed","success"]
        statuses:
          - "review-non-migration-pr"
      changed_files:
        paths:
          - "^db-migration-files/.*$"
    requires:
      count: 2
      teams:
        - "myorg/eng"

[bluekeyes]

After reviewing your policy, this is expected behavior and is a consequence of your policy and how predicates (the if block) work.

A rule's predicate controls whether or not the rule applies. If the predicate is not currently met, then the rule has the skipped status, which means it does not impact the overall policy status at all (both and and or blocks ignore it.) Here's what I think happened in your case:

1. The engineering team approved the PR
2. The developer updated the PR by merging in master
3. Because the the engineering team has approved rule does not have invalidate_on_push set, the approval from (1) was still valid
4. There was a small delay in posting the review-migration-pr status
5. In the time before this status appeared, Policy Bot evaluated the updated PR. It marked the db-migration squad approved or self-review passed rule as skipped because the predicate status did not exist, which allowed the approved rule from (3) to approve the full policy.

Since you are now viewing the policy after the status was posted, the rule is enabled again and it looks like the overall state is pending.

Here are two possible fixes:

1. Add the invalidate_on_push option to your rules so that approval is required again if the pull request changes

2. Use required conditions instead of predicates. A required condition uses the status as part of the approval, rather than using it to enable or disable the rule. This means the rule remains in the pending state until the status appears:

     - name: db-migration squad approved or self-review passed
       if:
         changed_files:
           paths:
             - "^db-migration-files/.*$"
       requires:
         count: 1
         teams:
           - "myorg/db-team"
         conditions:
           has_status:
             conclusions: ["failed", "success"]
             statuses:
               - "review-migration-pr"

   In this rule, the review-migration-pr status must be present with a failed or success conclusion and a member of the myorg/db-team team must approve for the rule to pass.

[rajesh-dhakad]

Thank you @bluekeyes - Let me test this suggestion.