Skip to content

[wip] test Qoder code review action#27

Closed
wangxiyu191 wants to merge 8 commits intomainfrom
feature/add-qoder-review
Closed

[wip] test Qoder code review action#27
wangxiyu191 wants to merge 8 commits intomainfrom
feature/add-qoder-review

Conversation

@wangxiyu191
Copy link
Collaborator

No description provided.

Copy link

@qoderai qoderai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Review Summary

Nice addition wiring Qoder into this repo’s PR workflow; this keeps the surface area small by touching only CI and no runtime code.

🛡️ Key Risks & Issues

  • Qoder review rerun trigger: The if condition on the qoder-review job currently compares github.event.action to the string pull_request_review, which is an event name, not an action. Because action here will be values like opened, reopened, or dismissed, the left-hand side of the OR is always true, so the job runs on every dismissed review regardless of who authored it. This can create unnecessary workflow runs and re-trigger Qoder on human review dismissals, adding noise and extra CI usage. We should instead key off github.event_name or separate the logic paths so only dismissals of Qoder’s own reviews retrigger the job.
  • Workflow reruns on updated PRs: The workflow only listens to pull_request events of type opened/reopened plus pull_request_review:dismissed, so pushing new commits to an existing PR won’t automatically retrigger Qoder. If the goal is to keep Qoder’s review aligned with code updates, consider whether it should also run on pull_request synchronize events or another explicit signal.
  • Permissions surface: The job grants id-token: write alongside using secrets.QODER_PERSONAL_ACCESS_TOKEN. If the action does not require OIDC, dropping id-token: write would reduce the blast radius in case the action is ever compromised.

🧪 Verification Advice

  • Open and reopen PRs against main and verify Qoder runs exactly once per event and posts expected reviews.
  • Push new commits to an existing PR and confirm whether the current behavior (no re-run) matches expectations, then re-evaluate triggers if not.
  • Dismiss reviews from qoderai[bot] vs a human reviewer and confirm that only the intended dismissals retrigger the workflow once the condition is fixed.
  • In a non-production environment, temporarily misconfigure or remove QODER_PERSONAL_ACCESS_TOKEN to ensure the action fails clearly without leaking secrets.

💡 Thoughts & Suggestions

  • This workflow is a good, scoped way to introduce automated reviews. Once behavior is stable, you might consider expanding triggers (e.g., to synchronize) or narrowing them (e.g., to specific labels) depending on noise levels and desired coverage.
  • As you iterate, keeping permissions to the minimum needed (dropping id-token if unused, and only adding scopes when required) will help keep the CI surface area tight.

🤖 Generated by QoderView workflow run

Comment on lines +11 to +13
qoder-review:
runs-on: ubuntu-slim
if: ${{ github.event.action != 'pull_request_review' || github.event.review.login == 'qoderai[bot]' }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the intention seems to be "run on any pull_request event" plus "run on pull_request_review:dismissed only when the dismissed review was authored by qoderai[bot]", but the conditional is using github.event.action != 'pull_request_review', which compares an action (like opened, reopened, dismissed) to an event name (pull_request_review). For both of the configured events (pull_request and pull_request_review), github.event.action will never equal the literal string pull_request_review, so the left side of the OR is always true and this job will run on every dismissed review regardless of author. That means dismissing any human reviewer’s review will also retrigger Qoder, which adds noise and extra CI usage. To match the probable intent, we should instead key off github.event_name (e.g. github.event_name != 'pull_request_review' || github.event.review.user.login == 'qoderai[bot]') or split the logic per-event so that only dismissals of Qoder’s own reviews cause a rerun.


🤖 Generated by QoderFix in Qoder

Copy link

@qoderai qoderai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Review Summary

Nice addition to the repo to enable automatic Qoder reviews on PRs. The workflow is generally well structured, pins third-party actions to specific SHAs, and scopes permissions explicitly, which are all solid choices.

🛡️ Key Risks & Issues

  • The if condition on the qoder-review job currently uses github.event.action != 'pull_request_review' || github.event.review.login == 'qoderai[bot]'. Since action values are things like opened, reopened, dismissed, the comparison is always true, and the field for the reviewer login is likely incorrect. This means the job can run on every relevant event and not just when Qoder’s own review is dismissed, which can waste CI minutes and cause confusing behavior. Updating this to branch on github.event_name and github.event.review.user.login will make the behavior match the intent.
  • The job runs on pull_request events only for opened and reopened. It will not re-run automatically when new commits are pushed to an existing PR (the synchronize action), so reviews can become stale unless someone dismisses a review to trigger a rerun. Adding synchronize to the types list or relying on the default event types would make the auto-review better track PR updates.
  • The runner label ubuntu-slim is not a standard GitHub-hosted runner label. Unless you have a self-hosted runner registered with that exact label, the job will never start and the workflow will appear broken. If you intend to use GitHub-hosted runners, something like ubuntu-latest would be safer.
  • The workflow passes a personal access token (QODER_PERSONAL_ACCESS_TOKEN) to the action while also granting id-token: write. It is worth double-checking that the PAT is least-privileged and that id-token: write is actually needed; if the action can rely on fine-grained permissions or OIDC instead of a broad PAT, that would reduce the blast radius of any compromise.

🧪 Verification Advice

  • Open a test PR and confirm that the workflow triggers on open, reopen, and on new commits (synchronize) once you adjust the triggers, and that it re-runs only when Qoder’s review is dismissed if that’s the intended behavior.
  • Verify that the job successfully starts on your chosen runner label in the target organization, and that Qoder is able to post and update reviews and comments as expected using the configured token.

💡 Thoughts & Suggestions

  • You are already pinning both the hide-comment and Qoder actions to specific SHAs, which is great for supply-chain safety. Keeping those pins updated periodically to newer SHAs after review will help you benefit from upstream fixes while still avoiding surprise changes.
  • Given that this workflow is meant to be foundational infrastructure for reviews, it may be worth adding a short internal note or doc about how to interpret its behavior (e.g., when it runs, how to trigger a re-review) for other contributors in the org.

🤖 Generated by QoderView workflow run

jobs:
qoder-review:
runs-on: ubuntu-slim
if: ${{ github.event.action != 'pull_request_review' || github.event.review.login == 'qoderai[bot]' }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current if condition does not behave as intended and can lead to unexpected re-runs and confusion.

Right now it is:

if: ${{ github.event.action != 'pull_request_review' || github.event.review.login == 'qoderai[bot]' }}

A few issues combine here:

  • github.event.action contains values like opened, reopened, dismissed, etc., not pull_request_review, so github.event.action != 'pull_request_review' is always true. That means this job will run for every pull_request event and for every pull_request_review dismissal, regardless of who dismissed the review.
  • For pull_request_review events, the reviewer login is typically at github.event.review.user.login, not github.event.review.login, so the bot-check part likely never works as intended.

As a result, we might end up re-running the Qoder review on any review dismissal, including human reviewers, and we still don’t correctly scope behavior to qoderai[bot].

To better match the intention (run on all PR events and only on review dismissals from Qoder), consider something along the lines of:

if: ${{ github.event_name == 'pull_request' || (github.event_name == 'pull_request_review' && github.event.review.user.login == 'qoderai[bot]') }}

This keeps behavior predictable and ensures we don’t burn CI minutes on unrelated review events.


🤖 Generated by QoderFix in Qoder

@wangxiyu191
Copy link
Collaborator Author

@tair-bot review

@wangxiyu191
Copy link
Collaborator Author

@tair-ci review

@wangxiyu191 wangxiyu191 closed this Mar 2, 2026
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.

1 participant