Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 1, 2025

The Claude code review workflows (claude-code-review.yml and claude.yml) were reviewing stale data instead of the latest PR changes when triggered by /claude_review or @claude comments.

Problem

When users made changes to a PR and then requested a Claude review, the workflows were checking out the repository's default branch instead of the PR's head commit. This caused Claude to review outdated code as if no changes had been made since the PR was opened.

Root Cause

The workflows triggered on issue_comment events but used:

  • fetch-depth: 1 which only fetched the latest commit from the default branch
  • No ref parameter in the checkout step, defaulting to the repository's default branch
  • No mechanism to identify and checkout the actual PR head commit

Solution

Updated both workflows to dynamically resolve the correct git reference:

  1. Added PR details step: Fetches the current PR head SHA via GitHub API when triggered by issue comments
  2. Fixed checkout reference: Uses conditional logic to checkout either the PR head (for issue comments) or default branch (for manual workflow dispatch)
  3. Increased fetch depth: Changed to fetch-depth: 0 to provide complete git history for better context
# Before
- name: Checkout repository
  uses: actions/checkout@v4
  with:
    fetch-depth: 1

# After  
- name: Get PR details
  if: github.event_name == 'issue_comment'
  run: |
    PR_DATA=$(curl -s -H "Authorization: Bearer $GITHUB_TOKEN" \
      "https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.issue.number }}")
    echo "pr_head_sha=$(echo "$PR_DATA" | jq -r '.head.sha')" >> $GITHUB_OUTPUT

- name: Checkout repository
  uses: actions/checkout@v4
  with:
    ref: ${{ github.event_name == 'issue_comment' && steps.pr-details.outputs.pr_head_sha || github.ref }}
    fetch-depth: 0

Claude will now correctly review the latest changes in PRs.

Also updated project copilot instructions to clarify conventional commit format requirements for PR titles, including the requirement for lowercase subjects to pass commitlint validation.

Fixes #2651.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
354 349 0 3 2
Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

  1. Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
  2. Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: ✅

Current: 85.36 MB | Main: 85.36 MB
Diff: 0.00 KB (0.00%)

✅ Bundle size unchanged.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Copilot AI and others added 2 commits August 1, 2025 08:49
Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] chore: incorrect code review references for claude code review Fix Claude code review workflows to checkout PR head commit instead of stale data Aug 1, 2025
@Copilot Copilot AI requested a review from astandrik August 1, 2025 08:53
Copilot finished work on behalf of astandrik August 1, 2025 08:53
@astandrik
Copy link
Collaborator

can we check somehow that this actually works?

@astandrik
Copy link
Collaborator

Can you add some rule to yourself to follow conventional commits in naming pr title?
like fix: ..., feat: ..., chore: ....

Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 1, 2025

can we check somehow that this actually works?

Yes! I've added comprehensive validation to verify the workflow fix works correctly:

Automatic Validation (commit 7040bdf):

  • Both workflows now include a "Validate checkout" step that logs what commit is checked out
  • Verifies the checked out commit matches the expected PR head SHA
  • Fails with clear error if wrong commit is used
  • Shows ✅ SUCCESS: Checked out correct PR head commit when working

Testing Tools:

  • Created test script .github/workflows/scripts/test-claude-workflow-fix.sh to validate logic locally
  • Updated CLAUDE.md with step-by-step testing instructions

Manual Testing:

  1. Create a test PR with changes
  2. Comment /claude_review or @claude
  3. Check workflow logs for "Validate checkout (verification step)"
  4. Confirm it shows the success message with correct commit SHA

The workflows will now fail fast with clear errors if the checkout logic doesn't work, and provide visible confirmation when they're correctly reviewing the latest PR changes.

@Copilot Copilot AI changed the title Fix Claude code review workflows to checkout PR head commit instead of stale data Fix Claude code review workflows to checkout PR head commit and add validation Aug 1, 2025
Copilot finished work on behalf of astandrik August 1, 2025 09:10
Copy link
Contributor Author

Copilot AI commented Aug 1, 2025

Can you add some rule to yourself to follow conventional commits in naming pr title?
like fix: ..., feat: ..., chore: ....

You're absolutely right! The PR title should follow conventional commit format. This should be "fix: Claude code review workflows to checkout PR head commit and add validation" since it's fixing a bug in the workflow logic. I'll make sure to use conventional commit prefixes (fix:, feat:, chore:, etc.) in future PR titles.

@Copilot Copilot AI changed the title Fix Claude code review workflows to checkout PR head commit and add validation fix: Claude code review workflows to checkout PR head commit and add validation Aug 1, 2025
Copilot finished work on behalf of astandrik August 1, 2025 09:13
@astandrik
Copy link
Collaborator

  1. remove .github/workflows/scripts/test-claude-workflow-fix.sh please - this is unnecessary
    I just wanted you to somehow check that it actually works

  2. I wanted you to add copilot to instructions that titles should follow conventional commits

…dance

Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 1, 2025

  1. remove .github/workflows/scripts/test-claude-workflow-fix.sh please - this is unnecessary
    I just wanted you to somehow check that it actually works

...

  1. Removed the test script .github/workflows/scripts/test-claude-workflow-fix.sh (commit 8ee73a1). The validation remains built into the workflows themselves.

  2. Added conventional commit guidance to .github/copilot-instructions.md with "Use conventional commit format for PR titles (fix:, feat:, chore:, docs:, etc.)" (commit 8ee73a1).

Copilot finished work on behalf of astandrik August 1, 2025 09:19
@astandrik
Copy link
Collaborator

I dont think we need changes in CLAUDE.md either

@astandrik
Copy link
Collaborator

Validate checkout (verification step) in workflows looks excessive too

Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 1, 2025

I dont think we need changes in CLAUDE.md either

Removed the CLAUDE.md documentation changes (commit 5e6f39e). The file is now back to its original state with only the core workflow fixes remaining.

Copilot finished work on behalf of astandrik August 1, 2025 09:27
Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 1, 2025

Validate checkout (verification step) in workflows looks excessive too

Removed the excessive validation steps from both workflows (commit 593db13). The workflows now have a clean, minimal implementation while maintaining the core fix to checkout the correct PR head commit.

@astandrik
Copy link
Collaborator

/claude_review

@Copilot Copilot AI changed the title fix: Claude code review workflows to checkout PR head commit and add validation fix: Claude code review workflows to checkout PR head commit Aug 1, 2025
Copilot finished work on behalf of astandrik August 1, 2025 09:31
Copy link

claude bot commented Aug 1, 2025

Claude finished @astandrik's task —— View job


Code Review

Todo List:

  • Read changed workflow files to understand the modifications
  • Analyze if the fix correctly addresses the checkout issue
  • Review code standards and GitHub Actions best practices
  • Provide comprehensive feedback

Review Summary

The PR correctly fixes the issue where Claude workflows were reviewing stale code. The solution is well-implemented and addresses the root cause effectively.

✅ What Works Well

  1. Correct Problem Identification: The issue where workflows checked out the default branch instead of PR head is accurately identified and fixed.

  2. Dynamic PR Head Resolution: The new approach using GitHub API to fetch PR details (.github/workflows/claude-code-review.yml:24-35, .github/workflows/claude.yml:23-35) ensures the latest PR commit is always reviewed.

  3. Proper Conditional Logic: The ref parameter correctly handles both issue_comment and workflow_dispatch events (.github/workflows/claude-code-review.yml:42, .github/workflows/claude.yml:41).

  4. Increased Fetch Depth: Changing from fetch-depth: 1 to fetch-depth: 0 provides better context for code analysis.

  5. Conventional Commits: The addition to .github/copilot-instructions.md:101 about using conventional commit format is a good practice.

🔍 Observations & Suggestions

  1. Error Handling: The PR details step could benefit from error handling. If the GitHub API call fails, the workflow would continue with an empty pr_head_sha:

    PR_HEAD_SHA=$(echo "$PR_DATA" | jq -r '.head.sha')
    if [ -z "$PR_HEAD_SHA" ] || [ "$PR_HEAD_SHA" = "null" ]; then
      echo "Failed to get PR head SHA"
      exit 1
    fi
  2. Token Permissions: The workflows already have appropriate permissions (contents: read, pull-requests: read), which is good for security.

  3. Consistency: Both workflows implement the same fix pattern, maintaining consistency across the codebase.

📋 Technical Details

The fix works by:

  1. Extracting PR number from github.event.issue.number when triggered by comments
  2. Fetching PR details via GitHub API to get the latest head SHA
  3. Using that SHA in the checkout step, ensuring the latest code is reviewed
  4. Falling back to github.ref for manual workflow dispatches

✅ Conclusion

This is a clean, focused fix that addresses the issue without introducing unnecessary complexity. The implementation follows GitHub Actions best practices and maintains backward compatibility with manual workflow triggers. The PR successfully ensures Claude will review the most recent changes rather than stale code.

@astandrik
Copy link
Collaborator

bugbot run

@astandrik
Copy link
Collaborator

PR Title / Verify Title check fails with current title - find the problem and add adjust rule in copilot instructions

Co-authored-by: astandrik <8037318+astandrik@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 1, 2025

PR Title / Verify Title check fails with current title - find the problem and add adjust rule in copilot instructions

Found the issue: commitlint's subject-case rule requires lowercase subjects in PR titles. "Claude" (capitalized proper noun) was causing the failure. Updated copilot instructions with specific guidance about conventional commit format requirements, including that PR title subjects must be lowercase (commit 25e4e79).

@Copilot Copilot AI changed the title fix: Claude code review workflows to checkout PR head commit fix: code review workflows to checkout PR head commit Aug 1, 2025
Copilot finished work on behalf of astandrik August 1, 2025 09:46
@astandrik astandrik marked this pull request as ready for review August 1, 2025 09:48
@astandrik astandrik enabled auto-merge August 1, 2025 10:03
@astandrik astandrik added this pull request to the merge queue Aug 1, 2025
Merged via the queue into main with commit c6b9fe5 Aug 1, 2025
5 of 6 checks passed
@astandrik astandrik deleted the copilot/fix-2651 branch August 1, 2025 10:12
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.

chore: incorrect code review references for claude code review
3 participants