Required reviewer not being removed if automation not run

[matthewparavati]

Describe the bug

We have an automation (do_not_merge) to block merging if the "Do Not Merge" label is added. When the label is added, a required reviewer is set, and the PR is blocked from merging. When removing the label, the required reviewer is not removed and the PR is still blocked from merging.

To Reproduce

Steps to reproduce the behavior:

1. The .cm automation file

gitstream.cm

# -*- mode: yaml -*-

manifest:
  version: 1.0

automations:
  estimated_time_to_review:
    if:
      - true
    run:
      - action: add-label@v1
        args:
          label: "~{{ calc.etr }} min review"
          color: {{ 'E94637' if (calc.etr >= 20) else ('FBBD10' if (calc.etr >= 5) else '36A853') }}
  needs_one_review:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ approvals.zero }}
    run:
      - action: add-label@v1
        args: 
          label: ⏳ Waiting for 1 reviewer
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  needs_two_reviews:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ approvals.ltTwo }}
    run:
      - action: add-label@v1
        args: 
          label: {{ '⏳ Waiting for 2 reviewers' if (approvals.zero) else '⏳ Waiting for 1 reviewer' }}
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  deleted:
    if:
      - {{ has.deleted_files }}
    run: 
      - action: add-label@v1
        args:
          label: 🗑️ Deleted files
          color: '#DF9C04'
  missing_sc_ticket:
    if:
      - {{ not sc_ticket.comment }}
    run: 
      - action: add-label@v1
        args:
          label: 🎫 Missing SC Ticket
          color: '#FBCA02'
  unresolved_threads:
    if:
      - {{ pr.status == 'open' }}
      - {{ pr.unresolved_threads > 0 }}
    run:
      - action: add-label@v1
        args:
          label: 🚨 Unresolved thread(s)
          color: '#EC560D'
  sensitive_file_change_review:
    if: 
      - {{ has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  two_reviews:
    if: 
      - {{ is.long_review or has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  do_not_merge:
    if:
      - {{ has.do_not_merge_label }}
    run:
      - action: require-reviewers@v1
        args:
          reviewers: [john-dugan]
          also_assign: false
  ready_to_merge_one_approval_pr:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtZero }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  ready_to_merge_two_approval_pr:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtOne }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  
calc:
  etr: {{ branch | estimatedReviewTime }}
has:
  deleted_files: {{ source.diff.files | map(attr='new_file') | match(term='/dev/null') | some }}
  sensitive_files: {{ files | match(list=sensitive) | some }}
  do_not_merge_label: {{ pr.labels | match(term='Do not merge') | some }}
is:
  safe_change: {{ (source.diff.files | isFormattingChange) or (files | allDocs) or (files | allTests) or (files | allImages) }}
  quick_review: {{ files | length <= 7 and calc.etr <= 5 }}
  long_review: {{ files | length > 7 or calc.etr > 5 }}
approvals:
  zero: {{ pr.approvals | length == 0 }}
  gtZero: {{ pr.approvals | length > 0 }}
  gtOne: {{ pr.approvals | length > 1 }}
  ltTwo: {{ pr.approvals | length < 2 }}
sc_ticket:
  comment: {{ pr.comments | match(attr='content', term='Shortcut Story') | some }}
sensitive:
  - App.tsx
  - AppRoot.tsx
config:
  ignore_files:
    - 'yarn.lock'
    - 'ios/*.lock'
    - 'android/*.lock'
    - 'src/graphql/schema.json'
    - 'src/__generated__/types.ts'
    - 'src/i18n/messages/*.json'
    - '__tests__/**/*.snap'

2. PR Url

3. Describe your PR relevant content

4. Add relevant commit SHA

Expected behavior

When the "do not merge" label is removed from a PR, then the required reviewer is removed, since the require-reviewers action did not run and the PR is no longer blocked from merging.

Screenshots

Expected results when adding "do not merge"

Unexpected result after removing "do not merge" where PR is still blocked from merging

Additional context

Previous issue discussing this implementation

This was working fine for a bit but more recently, after removing the label, the PR is still blocked because of the required reviewer that was previously added. I've tried to update the automation with the beta also_assign argument but that didn't resolve this issue

[BenLloydPearson]

@matthewparavati Thanks for reporting this issue and providng a link to a related PR. Would you mind also sharing the CM file you're using for this automation? This will help us debug the problem and get back to you.

Thanks!

[matthewparavati]

@BenLloydPearson I've added it as a part of the collapsible text section gitstream.cm in my initial report. Is that fine?

[BenLloydPearson]

Perfect! Thank you! We'll dig into this and get back to you.

[vim-zz]

@matthewparavati currently the action require-reviewers@v1 is not managed, meaning it is not being re-evaluated unless there is a new commit

[matthewparavati]

@vim-zz thanks for the response. The require-reviewers@v1 action was previously managed, correct? and this behavior was changed recently?

Is there a different action I should/could use instead to block merging if a label is added and then allow merging when the label is removed?

[vim-zz]

The request-changes is managed and can be used to block PRs: https://docs.gitstream.cm/automation-actions/#request-changes

[matthewparavati]

@vim-zz Thanks, I tried updating to using the request-changes action. It correctly blocked the PR once we added the label but when we removed the label, "request-changes" still shows as a failing check and blocks the PR for some devs. I assume if the gitstream review is resolved and the action is not run, then PR should no longer be blocked from merging, correct?

PR with failing check

gitstream.cm

# -*- mode: yaml -*-

manifest:
  version: 1.0

automations:
  estimated_time_to_review:
    if:
      - true
    run:
      - action: add-label@v1
        args:
          label: "~{{ calc.etr }} min review"
          color: {{ 'E94637' if (calc.etr >= 20) else ('FBBD10' if (calc.etr >= 5) else '36A853') }}
  needs_one_review:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ approvals.zero }}
    run:
      - action: add-label@v1
        args: 
          label: ⏳ Waiting for 1 reviewer
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  needs_two_reviews:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ approvals.ltTwo }}
    run:
      - action: add-label@v1
        args: 
          label: {{ '⏳ Waiting for 2 reviewers' if (approvals.zero) else '⏳ Waiting for 1 reviewer' }}
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  deleted:
    if:
      - {{ has.deleted_files }}
    run: 
      - action: add-label@v1
        args:
          label: 🗑️ Deleted files
          color: '#DF9C04'
  missing_sc_ticket:
    if:
      - {{ not sc_ticket.comment }}
    run: 
      - action: add-label@v1
        args:
          label: 🎫 Missing SC Ticket
          color: '#FBCA02'
  unresolved_threads:
    if:
      - {{ pr.status == 'open' }}
      - {{ pr.unresolved_threads > 0 }}
    run:
      - action: add-label@v1
        args:
          label: 🚨 Unresolved thread(s)
          color: '#EC560D'
  sensitive_file_change_review:
    if: 
      - {{ has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  two_reviews:
    if: 
      - {{ is.long_review or has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  do_not_merge:
    if:
      - {{ has.do_not_merge_label }}
    run:
      - action: request-changes@v1
        args:
          comment: |
            Merging is blocked while "Do Not Merge" label is added
  ready_to_merge_one_approval_pr:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtZero }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  ready_to_merge_two_approval_pr:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtOne }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  
calc:
  etr: {{ branch | estimatedReviewTime }}
has:
  deleted_files: {{ source.diff.files | map(attr='new_file') | match(term='/dev/null') | some }}
  sensitive_files: {{ files | match(list=sensitive) | some }}
  do_not_merge_label: {{ pr.labels | match(term='Do not merge') | some }}
is:
  safe_change: {{ (source.diff.files | isFormattingChange) or (files | allDocs) or (files | allTests) or (files | allImages) }}
  quick_review: {{ files | length <= 7 and calc.etr <= 5 }}
  long_review: {{ files | length > 7 or calc.etr > 5 }}
approvals:
  zero: {{ pr.approvals | length == 0 }}
  gtZero: {{ pr.approvals | length > 0 }}
  gtOne: {{ pr.approvals | length > 1 }}
  ltTwo: {{ pr.approvals | length < 2 }}
sc_ticket:
  comment: {{ pr.comments | match(attr='content', term='Shortcut Story') | some }}
sensitive:
  - App.tsx
  - AppRoot.tsx
config:
  ignore_files:
    - 'yarn.lock'
    - 'ios/*.lock'
    - 'android/*.lock'
    - 'src/graphql/schema.json'
    - 'src/__generated__/types.ts'
    - 'src/i18n/messages/*.json'
    - '__tests__/**/*.snap'

[matthewparavati]

@vim-zz @BenLloydPearson thoughts on the above? Is it easier/better to create a new issue?

[vim-zz]

@matthewparavati this will be addressed as a roadmap item

[codfish]

@vim-zz any insight in what the ETA is for this feature/fix? For context, here's a snippet of our use case which seems pretty similar to the authors. We just have to comment it out for now cause it doesn't resolve itself so it blocks devs without manual intervention so not a great experience.

  enforce_labels:
    if:
      - {{ pr.draft == false }}
      - {{ pr.approvals | length >= 2 }}
      - {{ pr.labels | match(regex=r/^(needs .*|do not merge)$/i) }}
    run:
      - action: request-changes@v1
        args:
          comment: |
            Could you please flip ....

Also curious what the end solution will look like? Will you just programmatically dismiss the review?

[PavelLinearB]

Hi @matthewparavati and @codfish, Converting require-reviewers to a managed action is a roadmap item we plan to address in 2025. Currently, check status are updated only after commits. The plan is to make this action managed and remove the failed status when the condition no longer applies