chore: disable automatic PR AI review - #120
Merged
Merged
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| @@ -1,8 +1,6 @@ | |||
| name: Aictrl Review | |||
|
|
|||
| on: | |||
There was a problem hiding this comment.
🟠 Dispatch-only mode broken: empty PR_BASE_REF skips review.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #120, .github/workflows/code-review.yml:3-4):
Problem: Dispatch-only mode broken: empty PR_BASE_REF skips review
Detail: Removing the `pull_request` trigger leaves `workflow_dispatch` as the only way to run this workflow, but the manual path is broken: `PR_BASE_REF: ${{ github.event.pull_request.base.ref }}` (line 21) is empty on dispatch (unlike PR_SHA and the concurrency group, it has no `||` fallback, and the script's empty-PR_NUMBER recovery branch recovers only PR_NUMBER, not the base ref). The changed-files gate then runs `git diff --name-only "origin/$PR_BASE_REF...$PR_SHA"` → invalid refspec `origin/...<sha>` → git diff fails → CODE_CHANGES empty → step outputs skip=true ("No actual code changes detected"). Net effect: after this PR the workflow can never complete a review — every run, automatic or manual, silently no-ops.
Suggested fix: Recover the base ref for dispatch runs alongside PR_NUMBER, e.g. inside the existing `if [ -z "$PR_NUMBER" ]` branch add: `PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName')` and `echo "PR_BASE_REF=$PR_BASE_REF" >> $GITHUB_ENV`; or give the env a fallback: `PR_BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }}`. Then verify a manual dispatch on an open PR actually reaches the "Run Aictrl Review" step.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Removing the pull_request trigger leaves workflow_dispatch as the only way to run this workflow, but the manual path is broken: PR_BASE_REF: ${{ github.event.pull_request.base.ref }} (line 21) is empty on dispatch (unlike PR_SHA and the concurrency group, it has no || fallback, and the script's empty-PR_NUMBER recovery branch recovers only PR_NUMBER, not the base ref). The changed-files gate then runs git diff --name-only "origin/$PR_BASE_REF...$PR_SHA" → invalid refspec origin/...<sha> → git diff fails → CODE_CHANGES empty → step outputs skip=true ("No actual code changes detected"). Net effect: after this PR the workflow can never complete a review — every run, automatic or manual, silently no-ops.
| @@ -1,8 +1,6 @@ | |||
| name: Aictrl Review | |||
|
|
|||
| on: | |||
There was a problem hiding this comment.
🟡 Auto PR review disabled with no compensating gate.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #120, .github/workflows/code-review.yml:3):
Problem: Auto PR review disabled with no compensating gate
Detail: After this change no PR to main/master gets an automated AI review unless someone with write access manually dispatches the workflow, and fork-based contributor PRs cannot be auto-reviewed at all. The PR title says the disabling is intentional, but no compensating control or re-enable tracking is visible: if this is temporary, reference the tracking issue in the commit/PR; if permanent, document the replacement gate (e.g. required status check, scheduled dispatch, or an org-level policy) so merges to main/master don't silently lose review coverage.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
After this change no PR to main/master gets an automated AI review unless someone with write access manually dispatches the workflow, and fork-based contributor PRs cannot be auto-reviewed at all. The PR title says the disabling is intentional, but no compensating control or re-enable tracking is visible: if this is temporary, reference the tracking issue in the commit/PR; if permanent, document the replacement gate (e.g. required status check, scheduled dispatch, or an org-level policy) so merges to main/master don't silently lose review coverage.
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 1 · ⚪ 0 · 0/2 resolved
🤖 Fix all 2 open findings with your agent📋 Out-of-diff findings (2)
Reviewed 1 files · 0 inline · view all 2 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
Review response — PR #120Verified and fixed both findings against review SHA Issues addressed (pushed to this PR)
Review claims verified false (no change needed)None. Not addressed hereNone. |
| echo "Found PR #$PR_NUMBER for branch $BRANCH" | ||
| echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV | ||
|
|
||
| PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName') |
There was a problem hiding this comment.
🟡 Unvalidated base branch flows into agent prompt.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #120, .github/workflows/code-review.yml:48-53):
Problem: Unvalidated base branch flows into agent prompt
Detail: baseRefName is chosen by the PR author (any existing repo branch; git allows `$`, backticks, `;` in ref names) and is exported verbatim to GITHUB_ENV, then interpolated into the review-agent prompt at line 153 (`base: ${PR_BASE_REF}`) that drives an agent allowed to run `gh *` with a GITHUB_TOKEN holding pull-requests/issues write. Later shell uses are quoted, so this is a prompt-injection channel rather than command injection, and the branch must already exist in the repo — but the agent-facing prompt is unvalidated input.
Suggested fix: Validate before export, e.g. `git rev-parse --verify "origin/$PR_BASE_REF" >/dev/null || exit 1` and/or an allowlist regex such as `^[A-Za-z0-9._/-]+$` before writing PR_BASE_REF to $GITHUB_ENV.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
baseRefName is chosen by the PR author (any existing repo branch; git allows $, backticks, ; in ref names) and is exported verbatim to GITHUB_ENV, then interpolated into the review-agent prompt at line 153 (base: ${PR_BASE_REF}) that drives an agent allowed to run gh * with a GITHUB_TOKEN holding pull-requests/issues write. Later shell uses are quoted, so this is a prompt-injection channel rather than command injection, and the branch must already exist in the repo — but the agent-facing prompt is unvalidated input.
PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName')
if [ -z "$PR_BASE_REF" ]; then
echo "Could not determine the base branch for PR #$PR_NUMBER."
exit 1
fi
echo "PR_BASE_REF=$PR_BASE_REF" >> $GITHUB_ENV| on: | ||
| pull_request: | ||
| branches: [main, master] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🟡 Fork PRs can no longer be AI-reviewed, even manually.
Suggested change
| workflow_dispatch: | |
| If fork coverage matters, accept a PR number/URL via workflow_dispatch inputs and resolve the head SHA with `gh pr view --json headRefOid` instead of relying on the dispatched ref; otherwise document the fork gap in the policy comment. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #120, .github/workflows/code-review.yml:7):
Problem: Fork PRs can no longer be AI-reviewed, even manually
Detail: workflow_dispatch can only select refs that exist in the base repo, so PRs from forks (head branch lives only in the contributor's fork) cannot be dispatched against, and the resolve step's `gh pr list --head $GITHUB_REF_NAME` (line 39) can never match a fork head. External contributions therefore lose AI review entirely — contradicting the PR body's claim that "reviews can still be run manually when needed".
Suggested fix: If fork coverage matters, accept a PR number/URL via workflow_dispatch inputs and resolve the head SHA with `gh pr view --json headRefOid` instead of relying on the dispatched ref; otherwise document the fork gap in the policy comment.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
workflow_dispatch can only select refs that exist in the base repo, so PRs from forks (head branch lives only in the contributor's fork) cannot be dispatched against, and the resolve step's gh pr list --head $GITHUB_REF_NAME (line 39) can never match a fork head. External contributions therefore lose AI review entirely — contradicting the PR body's claim that "reviews can still be run manually when needed".
# the merge-quality controls.
on:
workflow_dispatch:
concurrency:
group: aictrl-review-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true| echo "Found PR #$PR_NUMBER for branch $BRANCH" | ||
| echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV | ||
|
|
||
| PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName') |
There was a problem hiding this comment.
🟡 gh pr view failure bypasses base-ref guard under bash -e.
--- a/.github/workflows/code-review.yml
+++ b/.github/workflows/code-review.yml
@@ -45,7 +45,7 @@
echo "Found PR #$PR_NUMBER for branch $BRANCH"
echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV
- PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName')
+ PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName' 2>/dev/null || true)
if [ -z "$PR_BASE_REF" ]; then
echo "Could not determine the base branch for PR #$PR_NUMBER."
exit 1🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #120, .github/workflows/code-review.yml:48):
Problem: gh pr view failure bypasses base-ref guard under bash -e
Detail: The step runs under GitHub Actions' default `bash -e`, so if `gh pr view` exits non-zero (rate limit, transient 5xx, token issue) the assignment at line 48 aborts the step immediately — the `[ -z "$PR_BASE_REF" ]` guard with its "Could not determine the base branch" message is unreachable for real gh failures and only fires if gh succeeds but prints empty output, which is near-impossible for baseRefName. Inconsistent with the sibling lookup at line 39 which uses `2>/dev/null || true`.
Suggested fix: Append `2>/dev/null || true` inside the command substitution so the existing `-z` guard handles both gh failure and empty output and exits with the intended message.
Suggested patch:
--- a/.github/workflows/code-review.yml
+++ b/.github/workflows/code-review.yml
@@ -45,7 +45,7 @@
echo "Found PR #$PR_NUMBER for branch $BRANCH"
echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV
- PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName')
+ PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName' 2>/dev/null || true)
if [ -z "$PR_BASE_REF" ]; then
echo "Could not determine the base branch for PR #$PR_NUMBER."
exit 1
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The step runs under GitHub Actions' default bash -e, so if gh pr view exits non-zero (rate limit, transient 5xx, token issue) the assignment at line 48 aborts the step immediately — the [ -z "$PR_BASE_REF" ] guard with its "Could not determine the base branch" message is unreachable for real gh failures and only fires if gh succeeds but prints empty output, which is near-impossible for baseRefName. Inconsistent with the sibling lookup at line 39 which uses 2>/dev/null || true.
echo "Found PR #$PR_NUMBER for branch $BRANCH"
echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV
PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq '.baseRefName')
if [ -z "$PR_BASE_REF" ]; then
echo "Could not determine the base branch for PR #$PR_NUMBER."
exit 1
fi
echo "PR_BASE_REF=$PR_BASE_REF" >> $GITHUB_ENV| @@ -1,8 +1,9 @@ | |||
| name: Aictrl Review | |||
|
|
|||
| # Policy: AI review is opt-in and advisory. Maintainers dispatch this workflow for | |||
There was a problem hiding this comment.
⚪ Policy comment cites CodeQL checks absent from the repo.
Suggested change
| # Policy: AI review is opt-in and advisory. Maintainers dispatch this workflow for | |
| Drop "and CodeQL" ("maintainer review plus the standard CI checks remain the merge-quality controls") or verify and reference the org-level CodeQL default setup explicitly. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #120, .github/workflows/code-review.yml:3-5):
Problem: Policy comment cites CodeQL checks absent from the repo
Detail: The new policy comment claims "maintainer review plus the standard CI and CodeQL checks remain the merge-quality controls", but no CodeQL workflow or config exists in the repository (.github/workflows contains only ci.yml, code-review.yml, publish.yml; ci.yml has no CodeQL job). Unless CodeQL runs via org-level default setup, the comment misleads contributors about the actual merge gates.
Suggested fix: Drop "and CodeQL" ("maintainer review plus the standard CI checks remain the merge-quality controls") or verify and reference the org-level CodeQL default setup explicitly.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new policy comment claims "maintainer review plus the standard CI and CodeQL checks remain the merge-quality controls", but no CodeQL workflow or config exists in the repository (.github/workflows contains only ci.yml, code-review.yml, publish.yml; ci.yml has no CodeQL job). Unless CodeQL runs via org-level default setup, the comment misleads contributors about the actual merge gates.
name: Aictrl Review
# Policy: AI review is opt-in and advisory. Maintainers dispatch this workflow for
# PRs that need it; maintainer review plus the standard CI and CodeQL checks remain
# the merge-quality controls.
on:
workflow_dispatch:
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 4 · ⚪ 2 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 1 files · 0 inline · view all 6 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
Review response — PR #120Verified and fixed all six findings against review SHA Issues addressed (pushed to this PR)
Review claims verified false (no change needed)None. Not addressed hereNone. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pull_requesttrigger from the Aictrl Review workflowworkflow_dispatchso reviews can still be run manually when neededValidation
git diff --check