Skip to content

fix(ai-reviews): try to correct maintainer recognition checks#2091

Draft
novusnota wants to merge 2 commits intomainfrom
closes-1970
Draft

fix(ai-reviews): try to correct maintainer recognition checks#2091
novusnota wants to merge 2 commits intomainfrom
closes-1970

Conversation

@novusnota
Copy link
Copy Markdown
Collaborator

Closes #1970

@mintlify
Copy link
Copy Markdown

mintlify Bot commented Apr 16, 2026

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
mintlify-ton-docs 🟢 Ready View Preview Apr 16, 2026, 10:52 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 16, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 48328f78-0505-4090-8698-4d702faaf95c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch closes-1970

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@novusnota
Copy link
Copy Markdown
Collaborator Author

@coderabbitai suggest alternatives: perhaps, it's better to first collect all info on the user asking for /review, then proceed with the separate job for handling it? What if commander.yml is to be used instead? Outline your suggestions in a single message.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 16, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look at the PR diff and relevant files
gh pr view 2091 --repo ton-org/docs --json files,body,title

Length of output: 273


🏁 Script executed:

#!/bin/bash
# Find workflow files and commander.yml
fd -e yml -e yaml . .github/

Length of output: 219


🏁 Script executed:

#!/bin/bash
# Read the modified pitaya.yml file
cat .github/workflows/pitaya.yml

Length of output: 23914


🏁 Script executed:

#!/bin/bash
# Read commander.yml to understand its structure
cat .github/workflows/commander.yml

Length of output: 4983


@novusnota Here is a structured outline of both directions and a third hybrid option:


Current approach — recap

The pr-review job's if: does not filter by permission at all for /review comments — the job always starts and a step (Check commenter permission) calls gh api /repos/.../collaborators/{commenter}/permission. Subsequent steps are gated via the TRUSTED_COMMENTER env variable. This wastes a runner slot and delays the "real" work for every unauthorized /review comment.


Alternative A — Two-job split (collect info first, then review)

Split the single job into two:

jobs:
  gate:
    if: |
      (github.event_name == 'issue_comment' && github.event.issue.pull_request != null &&
       (github.event.comment.body == '/review' || startsWith(github.event.comment.body, '/review '))) ||
      (github.event_name == 'pull_request_review_comment' &&
       (github.event.comment.body == '/review' || startsWith(github.event.comment.body, '/review '))) ||
      (github.event_name == 'pull_request' && ...)
    runs-on: ubuntu-latest
    outputs:
      trusted: ${{ steps.check.outputs.trusted }}
      pr_number: ${{ steps.check.outputs.pr_number }}
      is_fork: ${{ steps.check.outputs.is_fork }}
      base_ref: ${{ steps.check.outputs.base_ref }}
    steps:
      - name: Collect user info and PR context
        id: check
        env:
          GH_TOKEN: ${{ github.token }}
        uses: actions/github-script@...
        with:
          script: |
            // 1. Resolve PR number
            // 2. Call collaborator permission API
            // 3. Fetch PR metadata (fork, base/head ref)
            // 4. core.setOutput('trusted', 'true'/'false')
            // 5. core.setOutput('pr_number', ...)
            // etc.

  pr-review:
    needs: gate
    if: needs.gate.outputs.trusted == 'true'
    runs-on: ubuntu-latest
    steps:
      # No permission check needed — gate already did it
      - name: Checkout
        ...

Pros:

  • The heavy job (pr-review) never starts for untrusted commenters — no wasted compute.
  • All user + PR context is collected in one place, in a single github-script step (similar to commander.yml).
  • Cleaner separation of concerns.

Cons:

  • Two-job overhead adds ~10–15 s of scheduling latency per /review invocation.
  • needs outputs are strings, so 'true'/'false' comparisons must be explicit.

Alternative B — Adopt the commander.yml pattern

commander.yml already solves this at the job-level if: with no API call:

if: |
  contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association) &&
  (github.event.comment.body == '/review' || startsWith(github.event.comment.body, '/review '))

Then collect all PR context in a single github-script step (just like commander.yml's "Gather PR context in env variables" step), replacing the current multi-shell-step approach.

Pros:

  • No API call at all — GitHub provides author_association on the event payload for free.
  • Job is immediately skipped for untrusted users (no runner slot wasted).
  • Already proven by commander.yml in this very repo; consistent pattern.
  • github-script for context gathering is cleaner than the current multi-line shell heuristic.

Cons:

  • author_association values OWNER | MEMBER | COLLABORATOR do not cover team-derived write access. A repository team member who was granted write via a team (not added directly as a collaborator) will appear as CONTRIBUTOR or NONE and would be rejected. The current pitaya.yml comment explicitly notes: "team-derived write access is recognized as well" — this would be lost.

Alternative C — Hybrid: author_association gate + API fallback (recommended)

Use author_association in the job-level if: to gate the obvious cases, then widen the check inside the job with the collaborator API only when needed:

jobs:
  pr-review:
    if: |
      (github.event_name == 'pull_request' && ...) ||
      (
        (github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') &&
        (github.event.comment.body == '/review' || startsWith(github.event.comment.body, '/review ')) &&
        contains(
          fromJSON('["OWNER", "MEMBER", "COLLABORATOR", "CONTRIBUTOR"]'),
          github.event.comment.author_association
        )
      )
    steps:
      - name: Collect user info, PR context, and verify permission
        id: gate
        uses: actions/github-script@...
        with:
          script: |
            // Quick pass: OWNER/MEMBER/COLLABORATOR → trusted immediately
            // For CONTRIBUTOR: call collaborator permission API to catch team-derived write
            // Set TRUSTED_COMMENTER, PR_NUMBER, IS_FORK, BASE_REF, HEAD_REF via core.exportVariable
      - name: Checkout
        if: env.TRUSTED_COMMENTER == 'true'
        ...

Pros:

  • Job-level if: blocks anonymous and clearly untrusted users cheaply (no runner slot for them).
  • Team-derived write access still works (API call only for CONTRIBUTOR edge case).
  • Single consolidated info-gathering step.

Cons:

  • Still slightly more complex than pure Alternative B.
  • CONTRIBUTOR is a broad bucket (anyone who has opened a PR); you might get occasional API calls from non-maintainers, though these will still correctly return none and be rejected.

Summary table

Job always starts? API call? Team-derived write? Consistent with commander.yml?
Current ✅ Yes (wasteful) ✅ Always ✅ Yes ❌ No
Alt A (two-job) ❌ Gate only ✅ Gate job ✅ Yes Partial
Alt B (author_association) ❌ No ❌ Never ❌ No ✅ Yes
Alt C (hybrid) ❌ No ⚠️ Only for CONTRIBUTOR ✅ Yes Mostly

Recommendation: Alt C gives the best of both worlds — cheap job-level filtering aligned with commander.yml, plus correctness for team-derived write access. Alt A is the cleanest architecturally if you want a strict separation of "who triggered this" from "what to do about it."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Correct maintainer recognition checks in manual AI reviews

1 participant