Skip to content

Conversation

@chicks-net
Copy link
Member

@chicks-net chicks-net commented Oct 18, 2025

Context 🌍

I've been annoyed with missing Copilot reviews while working in a terminal-focused development workflow. I asked in the github cli repo about this problem and I was amazed to get an awesome answer with exactly what I needed

Done ✅

Created a new subcommand just pr_chesk which watches for the Github Actions to finish running and then it checks for Copilot code reviews and then it checks for Claude code reviews.

Trust but Verify ™️

I developed this in one repo, verified and refined it in a private repo and so this PR is the third one that has used this code happily. I've also run the new subcommand separately quite a few times.

Meta 🖥️

It is time for a new meta...

(Automated in just/gh-process.just.)

Copilot AI review requested due to automatic review settings October 18, 2025 19:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds automation to the PR workflow to watch GitHub Actions checks and then query for Copilot and Claude review comments.

  • Introduces a new pr_checks recipe that runs gh pr checks and queries review comments via GraphQL.
  • Updates pr recipe to depend on the new pr_checks logic (and bumps internal version comment).
  • Removes inline gh pr checks invocation from pr recipe, delegating it to pr_checks.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

# PR create 3.3
[group('Process')]
pr: _has_commits
pr: _has_commits && pr_checks
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Using && between dependencies is not valid Just syntax; Just expects a space-separated list of recipe dependencies. This will likely cause a parse error or treat '&&' as an invalid recipe name. Replace with a space: pr: _has_commits pr_checks.

Suggested change
pr: _has_commits && pr_checks
pr: _has_commits pr_checks

Copilot uses AI. Check for mistakes.
Comment on lines +137 to +163
gh api graphql \
-F owner=$PR_REPO_OWNER -F name=$PR_REPO_NAME -F pr=$PR_NUMBER \
--jq '[ .data.repository.pullRequest.reviews.nodes.[] | select(.author.login=="copilot-pull-request-reviewer") | .comments.nodes.[] ]' \
-f query='
query($name: String!, $owner: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviews(last: 20) {
nodes {
author {
resourcePath
login
}
comments(first: 100) {
nodes {
body
path
originalLine
}
}
}
}
}
}
}
'

Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Limiting reviews to last: 20 risks missing an earlier Copilot review if more than 20 reviews exist, causing false negatives when checking for Copilot suggestions. Increase the limit or implement pagination (e.g., use after cursors) to ensure all relevant reviews are considered.

Suggested change
gh api graphql \
-F owner=$PR_REPO_OWNER -F name=$PR_REPO_NAME -F pr=$PR_NUMBER \
--jq '[ .data.repository.pullRequest.reviews.nodes.[] | select(.author.login=="copilot-pull-request-reviewer") | .comments.nodes.[] ]' \
-f query='
query($name: String!, $owner: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviews(last: 20) {
nodes {
author {
resourcePath
login
}
comments(first: 100) {
nodes {
body
path
originalLine
}
}
}
}
}
}
}
'
# Paginate through all reviews to avoid missing Copilot reviews
COPILOT_COMMENTS=()
REVIEWS_AFTER=""
while : ; do
# Run GraphQL query for a page of reviews
PAGE=$(gh api graphql \
-F owner=$PR_REPO_OWNER -F name=$PR_REPO_NAME -F pr=$PR_NUMBER -F after="$REVIEWS_AFTER" \
-f query='
query($name: String!, $owner: String!, $pr: Int!, $after: String) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviews(first: 100, after: $after) {
nodes {
author {
resourcePath
login
}
comments(first: 100) {
nodes {
body
path
originalLine
}
}
}
pageInfo {
hasNextPage
endCursor
}
}
}
}
}
')
# Extract Copilot comments from this page
PAGE_COMMENTS=$(echo "$PAGE" | jq '[.data.repository.pullRequest.reviews.nodes[] | select(.author.login=="copilot-pull-request-reviewer") | .comments.nodes[]]')
COPILOT_COMMENTS+=("$PAGE_COMMENTS")
# Check if there are more pages
HAS_NEXT=$(echo "$PAGE" | jq -r '.data.repository.pullRequest.reviews.pageInfo.hasNextPage')
if [[ "$HAS_NEXT" != "true" ]]; then
break
fi
REVIEWS_AFTER=$(echo "$PAGE" | jq -r '.data.repository.pullRequest.reviews.pageInfo.endCursor')
done
# Combine all Copilot comments into one array and output
echo "${COPILOT_COMMENTS[@]}" | jq -s 'add'

Copilot uses AI. Check for mistakes.
@claude
Copy link

claude bot commented Oct 18, 2025

Code Review - PR #25

Summary

This PR adds automated checks for Copilot and Claude reviews after GitHub Actions complete. The changes refactor the pr recipe to delegate the checking workflow to a new pr_checks recipe.


✅ Positive Aspects

  1. Good separation of concerns - Extracting pr_checks into its own recipe makes the workflow modular and testable
  2. Proper error handling - Uses set -euo pipefail for bash strict mode
  3. Clear user feedback - Includes echo statements to inform users what's happening
  4. Indentation fix - Corrected indentation on line 121 (release recipe)

🔍 Code Quality Issues

1. Inconsistent bash strict mode (.just/gh-process.just:127)

The pr_checks recipe uses set -euo pipefail while the pr recipe uses set -euxo pipefail (with -x for tracing). Consider adding -x to pr_checks for consistency and debugging visibility.

2. Missing error handling for jq operations (.just/gh-process.just:133-135)

If gh pr view returns unexpected JSON or fields are missing, jq could fail silently or produce empty variables. Consider validating each extraction.

3. Unquoted variables (.just/gh-process.just:133-135)

Variables should be quoted to prevent word splitting issues.

4. GraphQL query could fail silently (.just/gh-process.just:137-162)

The GraphQL query output is not captured or checked. Consider capturing output and validating it before display.

5. Potential issue with Claude comment check (.just/gh-process.just:166)

Using | last could return null if no Claude comments exist. Consider adding // "No Claude review yet" to handle empty case.


🐛 Potential Bugs

  1. Race condition removed but may be needed (.just/gh-process.just:49-50)

    • The PR removes gh pr checks --watch from the pr recipe
    • The 10-second sleep is now pointless since it doesn't wait for checks anymore
    • Consider: Either remove the sleep or move it into pr_checks before the watch command
  2. Dependency on external author.login values (.just/gh-process.just:139, 166)

    • Hardcoded copilot-pull-request-reviewer and claude strings could break if GitHub changes these
    • Recommendation: Define as constants at the top of the file

🔒 Security Concerns

  1. Command injection risk (LOW) - Consider validating that PR_NUMBER is numeric
  2. No sensitive data exposure - The script only reads public PR metadata ✅

⚡ Performance Considerations

  1. Sequential API calls - Acceptable for the use case but could be optimized if needed
  2. Large review counts - The GraphQL query fetches last: 20 reviews with first: 100 comments each. Reasonable for most cases.

🧪 Test Coverage

Missing: No automated tests for the new pr_checks recipe. Consider manual testing with various scenarios.


📝 Suggestions

  1. Add a flag to skip checks for faster iteration
  2. Add comments explaining the GraphQL query - The query is complex
  3. Consider making reviewer names configurable via justfile variables

📊 Overall Assessment

Code Quality: 7/10 - Good structure but needs robustness improvements
Security: 9/10 - No major concerns
Maintainability: 7/10 - Could benefit from better error handling and constants
Functionality: 8/10 - Achieves the goal but edge cases need handling

✅ Recommendation

APPROVE with minor changes requested

The core functionality is sound and adds value to the workflow. Address the error handling and validation issues before merging, especially around the jq operations and null handling for Claude comments.


Review generated by Claude Code

@claude
Copy link

claude bot commented Oct 18, 2025

Code Review for PR #25

Summary

This PR adds automated checking for Copilot and Claude code reviews after GitHub Actions complete. The implementation integrates well with the existing workflow and provides useful developer experience improvements.

Positive Aspects ✅

  1. Good Problem-Solution Fit: Addresses a real pain point in terminal-focused workflows
  2. Incremental Changes: The PR modifies the workflow incrementally without breaking existing functionality
  3. Well-Tested: Author validated across multiple repositories before submitting
  4. Clear Documentation: PR description provides good context and rationale

Code Quality Issues

1. Error Handling: Missing jq Dependency Check (.just/gh-process.just:135-137)

The pr_checks recipe relies on jq for JSON parsing but does not check if it is installed:

PR_REPO_NAME=$(echo $PR_META | jq -r ".headRepository.name")

Recommendation: Add a dependency check at the beginning of the recipe:

if \! command -v jq &> /dev/null; then
    echo "Error: jq is required but not installed"
    exit 1
fi

2. Silent Failure on Empty Results (.just/gh-process.just:168)

The Claude comment check uses last which returns null if no comments exist:

gh pr view --json comments --jq "[.comments[] | select(.author.login == \"claude\")] | last | .body"

When no Claude comments exist, this outputs null which may be confusing.

Recommendation: Add error handling:

gh pr view --json comments --jq "[.comments[] | select(.author.login == \"claude\")] | last | .body // \"No Claude review found\""

3. Incomplete Copilot Output Handling (.just/gh-process.just:139-164)

The Copilot GraphQL query extracts review comments but does not format or display them in a user-friendly way. The output is just raw JSON to stdout.

Recommendation: Add formatting similar to the Claude output to make the results more readable.

4. Inconsistent Error Handling Between Copilot and Claude Checks

The Copilot check uses GraphQL API while Claude uses REST API, but neither has explicit error handling for API failures.

Recommendation: Wrap both sections in error checking to gracefully handle API failures.

Potential Issues

5. Hardcoded Author Login (.just/gh-process.just:168)

The Claude check uses author.login == "claude" which assumes Claude comments under that exact username. This might break if:

  • Claude uses a different username format
  • Multiple Claude integrations exist
  • GitHub changes the integration username

Recommendation: Consider making this configurable or add a comment explaining the assumption.

6. Race Condition Risk (.just/gh-process.just:131)

The recipe runs gh pr checks --watch which blocks until checks complete, but Copilot/Claude reviews might not be immediately available even after checks pass.

Recommendation: Consider adding a small sleep or retry logic:

gh pr checks --watch -i 5
sleep 2  # Give review bots time to post

7. CRITICAL: Missing Recipe Dependency (.just/gh-process.just:19)

The pr recipe now depends on pr_checks via pr: _has_commits && pr_checks, but pr_checks assumes a PR already exists (line 134). This creates a circular dependency since pr creates the PR.

Critical Bug: pr_checks will fail when called from pr because it runs gh pr view before the PR is created at line 43.

Recommendation: Move pr_checks to run AFTER the PR is created. The pr recipe should create the PR first, then call just pr_checks as a separate step, not as a dependency.

Performance Considerations

8. GraphQL Query Efficiency (.just/gh-process.just:139-164)

The query fetches last: 20 reviews and first: 100 comments per review, which could be excessive for most PRs.

Recommendation: Consider limiting to more reasonable defaults (e.g., last: 5 reviews).

Security Concerns

9. Temp File Handling (.just/gh-process.just:28, 44)

Good practice: The PR properly cleans up the temp file at line 44. No security issues here. ✅

10. Command Injection Risk - Low Priority (.just/gh-process.just:135-137)

Using echo $VAR | jq is generally safe here since the input comes from gh CLI, but best practice would be:

PR_REPO_NAME=$(jq -r ".headRepository.name" <<< "$PR_META")

Test Coverage

No tests are included for the new pr_checks recipe. Given this is a justfile recipe (not application code), manual testing is appropriate, and the author has done multi-repo validation.

Recommendation: Consider adding a comment documenting expected behavior for future maintainers.

Minor Issues

  1. Comment Formatting (.just/gh-process.just:166): Comment says "did Claude comment?" but code does more than check - it displays the comment body
  2. Inconsistent String Quoting (.just/gh-process.just:167): Uses double quotes in echo with escape sequences, which is fine, but could simplify

Recommendation

Request Changes - The critical bug in item #7 (circular dependency/timing issue) must be fixed before merging. The pr_checks recipe will fail when called from pr because it tries to view a PR that does not exist yet.

Other issues are lower priority but should be addressed for production readiness.


Great work on improving the developer experience! The approach is solid, just needs the dependency/timing fix. 🚀


🤖 Generated with Claude Code

@chicks-net chicks-net merged commit 1753035 into main Oct 18, 2025
5 checks passed
@chicks-net chicks-net deleted the chicks/2025-10-18-just-pr-checks branch October 18, 2025 19:21
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.

2 participants