-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add code-review task (initial commit) (testing) #21103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4001fe5
c6b85ec
2fea873
84ccad8
d94e9df
724e8b1
96e6afd
647b610
c7c96e9
c5fe6c5
96c66d6
62bf201
ca4dd32
dcaedbd
ff8d037
a26b00f
595278f
50dc5c0
8ab152f
3d1dd32
5d9492b
32e54e8
7ba4bdf
f0eaa46
d6cdd8f
50281ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,336 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This workflow performs AI-powered code review on PRs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # It creates a Coder Task that uses AI to analyze PR changes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # review code quality, identify issues, and post committable suggestions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The AI agent posts a single review with inline comments using GitHub's | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # native suggestion syntax, allowing one-click commits of suggested changes. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Triggered by: Adding the "code-review" label to a PR, or manual dispatch. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Required secrets: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # - DOC_CHECK_CODER_URL: URL of your Coder deployment (shared with doc-check) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # - DOC_CHECK_CODER_SESSION_TOKEN: Session token for Coder API (shared with doc-check) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: AI Code Review | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| types: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| - labeled | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| workflow_dispatch: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| inputs: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| pr_url: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Pull Request URL to review" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| required: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| template_preset: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Template preset to use" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| required: false | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| default: "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| code-review: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: AI Code Review | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| (github.event.label.name == 'code-review' || github.event_name == 'workflow_dispatch') && | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| (github.event.pull_request.draft == false || github.event_name == 'workflow_dispatch') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout-minutes: 30 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| CODER_URL: ${{ secrets.DOC_CHECK_CODER_URL }} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| CODER_SESSION_TOKEN: ${{ secrets.DOC_CHECK_CODER_SESSION_TOKEN }} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| contents: read # Read repository contents and PR diff | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| pull-requests: write # Post review comments and suggestions | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| actions: write # Create workflow summaries | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps: | |
| steps: | |
| - name: Validate required secrets | |
| run: | | |
| if [[ -z "${{ secrets.CODE_REVIEW_CODER_URL }}" ]]; then | |
| echo "::error::CODE_REVIEW_CODER_URL secret is not set" | |
| exit 1 | |
| fi | |
| if [[ -z "${{ secrets.CODE_REVIEW_CODER_SESSION_TOKEN }}" ]]; then | |
| echo "::error::CODE_REVIEW_CODER_SESSION_TOKEN secret is not set" | |
| exit 1 | |
| fi | |
| - name: Determine PR Context |
| steps: | |
| steps: | |
| - name: Validate required secrets | |
| run: | | |
| if [[ -z "${{ secrets.CODE_REVIEW_CODER_URL }}" ]]; then | |
| echo "::error::CODE_REVIEW_CODER_URL secret is not set" | |
| exit 1 | |
| fi | |
| if [[ -z "${{ secrets.CODE_REVIEW_CODER_SESSION_TOKEN }}" ]]; then | |
| echo "::error::CODE_REVIEW_CODER_SESSION_TOKEN secret is not set" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing error handling directives. Shell scripts should fail fast on errors to prevent cascading failures.
Without set -euo pipefail, if any command fails (like accessing an undefined variable), the script continues executing, potentially with invalid state.
| run: | | |
| run: | | |
| set -euo pipefail | |
| echo "Using template preset: ${INPUTS_TEMPLATE_PRESET}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing set -euo pipefail at the start of the script. While doc-check.yaml doesn't have this either, it's a best practice for shell scripts with multiple commands to fail fast on errors and catch undefined variables. This is especially important here since we're doing URL validation and extraction.
| set -euo pipefail | |
| set -euo pipefail | |
| echo "Using template preset: ${INPUTS_TEMPLATE_PRESET}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: The GitHub API call for user ID lacks error context and doesn't validate the response.
If the API returns an empty or invalid response, GITHUB_USER_ID will be set to an empty string, which could cause issues downstream. The error message is good, but we should validate the result.
| if ! GITHUB_USER_ID=$(gh api "users/${GITHUB_ACTOR}" --jq '.id'); then | |
| if ! GITHUB_USER_ID=$(gh api "users/${GITHUB_ACTOR}" --jq '.id') || [[ -z "${GITHUB_USER_ID}" ]]; then | |
| echo "::error::Failed to get GitHub user ID for actor ${GITHUB_ACTOR}" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Error message could be more helpful by suggesting what the user should check.
| echo "::error::Failed to get GitHub user ID for actor ${GITHUB_ACTOR}" | |
| echo "::error::Failed to get GitHub user ID for actor ${GITHUB_ACTOR}. Verify the user exists and has access to this repository." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: The PR URL regex validation doesn't enforce the URL must end after the PR number, allowing invalid URLs like https://github.com/owner/repo/pull/123/extra.
This could lead to incorrect PR number extraction or unexpected behavior. Adding an end-of-string anchor $ ensures the URL format is strictly validated.
| if [[ ! "${INPUTS_PR_URL}" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then | |
| # Validate PR URL format | |
| if [[ ! "${INPUTS_PR_URL}" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then | |
| echo "::error::Invalid PR URL format: ${INPUTS_PR_URL}" | |
| echo "::error::Expected format: https://github.com/owner/repo/pull/NUMBER" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: [NITPICK] Regex allows trailing content after PR number (e.g., https://github.com/owner/repo/pull/123/files would pass validation but fail PR extraction).
Suggestion: Anchor the regex to end of string for stricter validation.
| if [[ ! "${INPUTS_PR_URL}" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then | |
| if [[ ! "${INPUTS_PR_URL}" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+/?$ ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: URL validation doesn't verify that the PR is from the current repository. A user could provide a PR URL from a different repo (e.g., https://github.com/other-org/other-repo/pull/123), which would cause the checkout step to fail or review the wrong PR.
| # Validate PR URL format | |
| if [[ ! "${INPUTS_PR_URL}" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then | |
| echo "::error::Invalid PR URL format: ${INPUTS_PR_URL}" | |
| echo "::error::Expected format: https://github.com/owner/repo/pull/NUMBER" | |
| exit 1 | |
| fi | |
| # Validate PR URL format and repository | |
| if [[ ! "${INPUTS_PR_URL}" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then | |
| echo "::error::Invalid PR URL format: ${INPUTS_PR_URL}" | |
| echo "::error::Expected format: https://github.com/owner/repo/pull/NUMBER" | |
| exit 1 | |
| fi | |
| # Verify the URL is for the current repository | |
| EXPECTED_REPO_PREFIX="https://github.com/${{ github.repository }}/pull/" | |
| if [[ ! "${INPUTS_PR_URL}" =~ ^${EXPECTED_REPO_PREFIX}[0-9]+$ ]]; then | |
| echo "::error::PR URL must be from the current repository: ${{ github.repository }}" | |
| echo "::error::Got: ${INPUTS_PR_URL}" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Variable should be quoted to safely handle URLs with special characters or spaces.
While URLs shouldn't have spaces, defensive quoting is a shell scripting best practice.
| ISSUE_URL="${INPUTS_PR_URL/\/pull\//\/issues\/}" | |
| ISSUE_URL="${INPUTS_PR_URL/\/pull\//\/issues\/}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important: Variable should be quoted to safely handle special characters in URLs.
| ISSUE_URL="${INPUTS_PR_URL/\/pull\//\/issues\/}" | |
| ISSUE_URL="${INPUTS_PR_URL/\/pull\//\/issues\/}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: sed command can fail silently, leaving PR_NUMBER empty if URL doesn't match pattern.
Impact: Empty PR_NUMBER causes cryptic failures in downstream git commands (git fetch, gh api calls) with confusing error messages like "pull//head" or "pull undefined/reviews".
| PR_NUMBER=$(echo "${INPUTS_PR_URL}" | sed -n 's|.*/pull/\([0-9]*\)$|\1|p') | |
| PR_NUMBER=$(echo "${INPUTS_PR_URL}" | sed -n 's|.*/pull/\([0-9]*\)$|\1|p') | |
| if [[ -z "${PR_NUMBER}" ]]; then | |
| echo "::error::Failed to extract PR number from URL: ${INPUTS_PR_URL}" | |
| exit 1 | |
| fi | |
| echo "pr_number=${PR_NUMBER}" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Variable should be quoted for consistency and safety with special characters.
| ISSUE_URL="${GITHUB_EVENT_PR_HTML_URL/\/pull\//\/issues\/}" | |
| ISSUE_URL="${GITHUB_EVENT_PR_HTML_URL/\/pull\//\/issues\/}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important: Variable should be quoted for consistency and safety.
| ISSUE_URL="${GITHUB_EVENT_PR_HTML_URL/\/pull\//\/issues\/}" | |
| ISSUE_URL="${GITHUB_EVENT_PR_HTML_URL/\/pull\//\/issues\/}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing error handling directives for this shell script block.
| run: | | |
| run: | | |
| set -euo pipefail | |
| echo "owner=${{ github.repository_owner }}" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important: Missing set -euo pipefail for consistency with other shell script blocks.
| run: | | |
| run: | | |
| set -euo pipefail | |
| echo "owner=${{ github.repository_owner }}" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing set -euo pipefail at the start of the script block.
Impact: Without these flags, undefined variables or command failures won't cause the step to fail. This could lead to silent failures where empty values are written to GITHUB_OUTPUT.
| echo "owner=${REPO_OWNER}" >> "${GITHUB_OUTPUT}" | |
| run: | | |
| set -euo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing error handling with set -euo pipefail at the start of this script block.
The "Determine PR Context" step (line 62) has proper error handling, but this "Extract repository info" step doesn't. If REPO_OWNER or REPO_NAME are empty, the workflow would continue with empty values rather than failing fast.
Impact: Silent failures could lead to malformed task prompts or API calls.
| run: | | |
| echo "owner=${REPO_OWNER}" >> "${GITHUB_OUTPUT}" | |
| echo "repo=${REPO_NAME}" >> "${GITHUB_OUTPUT}" | |
| run: | | |
| set -euo pipefail | |
| echo "owner=${REPO_OWNER}" >> "${GITHUB_OUTPUT}" | |
| echo "repo=${REPO_NAME}" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing error handling directives. This is especially important for the heredoc block that follows.
| run: | | |
| run: | | |
| set -euo pipefail | |
| echo "Building code review prompt for PR #${PR_NUMBER}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing set -euo pipefail at the start of the script block.
Impact: Without these flags, if PR_NUMBER is undefined or the heredoc fails, the step could succeed with an incomplete or malformed prompt, causing the Coder task to receive invalid instructions.
| echo "Building code review prompt for PR #${PR_NUMBER}" | |
| run: | | |
| set -euo pipefail | |
| echo "Building code review prompt for PR #${PR_NUMBER}" |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The bash heredoc syntax is missing proper indentation handling. The <<EOF should be <<-EOF to strip leading tabs, making the code more readable:
| TASK_PROMPT=$(cat <<EOF | |
| TASK_PROMPT=$(cat <<-EOF |
| TASK_PROMPT=$(cat <<EOF | |
| TASK_PROMPT=$(cat <<-EOF |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The heredoc marker at line 334 and line 336 use different markers (EOFOUTPUT). While this works, it would be more maintainable to use consistent naming. Also, the heredoc at line 119 uses EOF while this uses EOFOUTPUT, creating inconsistency:
| echo "task_prompt<<EOFOUTPUT" | |
| echo "${TASK_PROMPT}" | |
| echo "EOFOUTPUT" | |
| echo "task_prompt<<EOF_TASK_PROMPT" | |
| echo "${TASK_PROMPT}" | |
| echo "EOF_TASK_PROMPT" |
| echo "task_prompt<<EOFOUTPUT" | |
| echo "${TASK_PROMPT}" | |
| echo "EOFOUTPUT" | |
| echo "task_prompt<<EOF_TASK_PROMPT" | |
| echo "${TASK_PROMPT}" | |
| echo "EOF_TASK_PROMPT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best Practice: Consider pinning to a specific commit SHA rather than the main branch for better reproducibility and security.
Using ref: main means the workflow behavior can change unexpectedly if the action is updated. Pinning to a commit SHA ensures consistent behavior and allows you to review changes before updating.
| uses: actions/checkout@1af3b93b6815bc44a9784bd300feb67ff0d1eeb3 # v6.0.0 | |
| ref: main # TODO: Pin to commit SHA for reproducibility (e.g., abc123def456...) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow checks out an external action from the main branch (line 345), which could introduce supply chain security risks if that branch is compromised. Consider pinning to a specific commit SHA or tag version for better security:
| ref: main | |
| ref: v1.0.0 # or a specific commit SHA |
| ref: main | |
| ref: v1.0.0 # or a specific commit SHA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Pin to commit SHA instead of main for reproducibility. Using main means workflow behavior can change unexpectedly.
| ref: main | |
| ref: main # TODO: Pin to specific commit SHA for reproducibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Missing error handling directives for consistency with other steps.
| github-user-id: ${{ steps.determine-context.outputs.github_user_id }} | |
| run: | | |
| set -euo pipefail | |
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Missing set -euo pipefail for consistency.
| run: | | |
| run: | | |
| set -euo pipefail | |
| { |
Uh oh!
There was an error while loading. Please reload this page.