Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 47 additions & 11 deletions .github/workflows/code-review.yml
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
name: Aictrl Review

# Policy: AI review is opt-in and advisory. Maintainers dispatch this workflow for

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

# PRs that need it; maintainer review plus the standard CI checks remain the
# merge-quality controls. Set pr_number to review a fork PR or a PR other than the
# one associated with the selected base-repository ref.
on:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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.

pull_request:
branches: [main, master]
workflow_dispatch:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

inputs:
pr_number:
description: PR number to review (required for fork PRs)
required: false
type: string

concurrency:
group: aictrl-review-${{ github.event.pull_request.number || github.ref }}
group: aictrl-review-${{ inputs.pr_number || github.ref }}
cancel-in-progress: true

jobs:
Expand All @@ -18,10 +25,7 @@ jobs:
pull-requests: write
issues: write
env:
PR_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
PR_NUMBER: ${{ github.event.pull_request.number }}
PR_BASE_REF: ${{ github.event.pull_request.base.ref }}
IS_MANUAL: ${{ github.event_name == 'workflow_dispatch' }}
PR_NUMBER: ${{ inputs.pr_number }}
steps:
- name: Checkout repository
uses: actions/checkout@v6
Expand All @@ -33,6 +37,11 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
if [ -n "$PR_NUMBER" ] && ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then
echo "PR number must contain digits only."
exit 1
fi

if [ -z "$PR_NUMBER" ]; then
BRANCH="${GITHUB_HEAD_REF:-${GITHUB_REF_NAME}}"
PR_NUMBER=$(gh pr list --head "$BRANCH" --json number --jq '.[0].number' 2>/dev/null || true)
Expand All @@ -42,9 +51,32 @@ jobs:
exit 0
fi
echo "Found PR #$PR_NUMBER for branch $BRANCH"
echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV
fi

PR_DATA=$(gh pr view "$PR_NUMBER" --json baseRefName,headRefOid --jq '[.baseRefName, .headRefOid] | @tsv' 2>/dev/null || true)
IFS=$'\t' read -r PR_BASE_REF PR_SHA <<< "$PR_DATA"

if [ -z "$PR_BASE_REF" ] || [ -z "$PR_SHA" ]; then
echo "Could not determine the base branch and head SHA for PR #$PR_NUMBER."
exit 1
fi
if ! [[ "$PR_BASE_REF" =~ ^[A-Za-z0-9._/-]+$ ]]; then
echo "PR #$PR_NUMBER has an unsafe base branch name."
exit 1
fi
if ! git rev-parse --verify "refs/remotes/origin/$PR_BASE_REF^{commit}" >/dev/null; then
echo "Base branch '$PR_BASE_REF' does not resolve to an origin commit."
exit 1
fi
if ! [[ "$PR_SHA" =~ ^[0-9a-f]{40}$ ]]; then
echo "PR #$PR_NUMBER has an invalid head SHA."
exit 1
fi

echo "PR_NUMBER=$PR_NUMBER" >> "$GITHUB_ENV"
echo "PR_BASE_REF=$PR_BASE_REF" >> "$GITHUB_ENV"
echo "PR_SHA=$PR_SHA" >> "$GITHUB_ENV"

echo "Checking if SHA $PR_SHA was already reviewed..."
REVIEW_COMMENTS=$(gh pr view "$PR_NUMBER" --json comments --jq '.comments[].body' | grep -c "Reviewed SHA:" || true)
LAST_REVIEW_SHA=$(gh pr view "$PR_NUMBER" --json comments --jq '.comments[].body' | grep -o "Reviewed SHA: [a-f0-9]\{40\}" | tail -n 1 | cut -d' ' -f3)
Expand All @@ -55,14 +87,18 @@ jobs:
exit 0
fi

if [ "$REVIEW_COMMENTS" -ge 2 ] && [ "$IS_MANUAL" != "true" ]; then
echo "PR already has $REVIEW_COMMENTS AI reviews. Skipping (use workflow_dispatch to force)."
if [ "$REVIEW_COMMENTS" -ge 2 ]; then
echo "PR already has $REVIEW_COMMENTS AI reviews. Skipping because the review cap is 2."
echo "skip=true" >> $GITHUB_OUTPUT
exit 0
fi

echo "Checking changed files..."
CODE_CHANGES=$(git diff --name-only "origin/$PR_BASE_REF...$PR_SHA" | grep -E '\.(ts|js|json|sh|yml|yaml)$' | grep -vE '^docs/|.*\.md$' || true)
if ! CHANGED_FILES=$(gh pr diff "$PR_NUMBER" --name-only); then
echo "Could not load changed files for PR #$PR_NUMBER."
exit 1
fi
CODE_CHANGES=$(printf '%s\n' "$CHANGED_FILES" | grep -E '\.(ts|js|json|sh|yml|yaml)$' | grep -vE '^docs/|.*\.md$' || true)

if [ -z "$CODE_CHANGES" ]; then
echo "No actual code changes detected. Skipping review."
Expand Down