estimatedReviewTime and files length check do not seem to be working

[matthewparavati]

Describe the bug

estimatedReviewTime and files length check do not seem to be working

We have a few automations that rely on the estimatedReviewTime and files length checks to set some labels and the required number of reviewers.

Our last few reviews created today have not been labeled as expected or had the expected number of required approvals set. They are all being labeled as 1 minute reviews that only require 1 approval. This has occurred on 3 PRs in a row

This is incorrect based on the number of files + I'd expected the estimatedReviewTime to be higher for these PRs

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: 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'

2. The PR URL (as it contains the repo and PR identifiers) PR URL - from screenshots PR URL 2 PR URL 3

3. Describe your PR relevant content

4. Add relevant commit SHA

Expected behavior

• Even accounting for the ignore_files that are set in gitstream.cm

I expect 2 approvals to be required before merging is allowed since the files length should be greater than 7 I expect the estimated time to review to be greater than 1 minute since these PRs are pretty big

Screenshots

Additional context

We have not changed our gitstream.cm config recently and last week this was working fine and as expected

[vim-zz]

Hi, we have a known issue that the estimated review time takes into account the package-lock files, I expect it to be fixed in day or two - do you think that might be the issue?

[matthewparavati]

Hi @vim-zz, hmm maybe but if that were the case, wouldn't the estimate possibly be too long rather than too short? Also, how would that affect our files length check?

[bartkuzio]

Hi, I have the same problem. All PRs are getting 1 min review label no matter how many files are changed.

[matthewparavati]

@bartkuzio, do you have ignore_files set in your gitstream.cm?

I did and just removed them as a test (bc another repo here didn't ignore any files and it showed PRs with non 1 min PR review estimates). Now my PR is labeled as a "~10 min review" and requires 2 approvals, which is expected

@vim-zz @PavelLinearB just fyi, it might be related to having ignore_files set? 🤔

PR URL

[vim-zz]

@EladKohavi please look

[vim-zz]

@bartkuzio @matthewparavati, we have added some logs to better identify the issue. Could you please provide the following information to the support email (support at linearb.io) or directly to me in Slack:

1. Identify a new MR or PR where the estimation seems to be incorrect
2. Share the MR or PR link
3. Share the GitHub Action or GitLab job log
4. Share a screenshot with the estimation label
5. If possible, share a screenshot of the changed files list with the number of changed lines in each file

[matthewparavati]

@vim-zz just to confirm, it needs to be a new PR and not 1 we've pushed changes to since you requested this info?

[vim-zz]

It's better to have a new PR so we are sure you get the latest and starts fresh

tl;dr yes :)

[vim-zz]

@matthewparavati ping me when you send it just to make sure :)

[matthewparavati]

@vim-zz just sent an email over. Should have all the requested info except for the number of changes lines in each file

[vim-zz]

@matthewparavati we fixed an issue there, please see if the issue was resolved on your side

[matthewparavati]

@vim-zz the issue does not seemed resolved here. A new PR opened today is still being marked as ~1 minute review and only requiring 1 approval. I sent some more info via email

[bartkuzio]

@bartkuzio, do you have ignore_files set in your gitstream.cm?

I did and just removed them as a test (bc another repo here didn't ignore any files and it showed PRs with non 1 min PR review estimates). Now my PR is labeled as a "~10 min review" and requires 2 approvals, which is expected

Yes, I did. I removed it now and it fixed the issue. So it's a good workaround for now. Thanks a lot! Btw, for me also explain_code_experts action was broken and it also fixes it, so both problems seem to have the same root cause.

[vim-zz]

The issue has been resolved. There was a regression in the ignore_files support for glob patterns, and another regression in the default exclusion of lock files during review time estimations calc.

[matthewparavati]

Awesome, thanks @vim-zz! I do see it working now! 🥳