-
Notifications
You must be signed in to change notification settings - Fork 13k
feat(skills): improve async-pr-review workflow and logging #21790
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
902311f
feat(skills): add async-pr-review skill
mattKorwel c1b640a
refactor(skills): optimize async-pr-review for agentic workflow
mattKorwel c3272a5
refactor(skills): dynamic paths and worktree scoping for async-pr-review
mattKorwel a49a502
feat(skills): improve async-pr-review workflow and logging
mattKorwel 4fbf969
Merge remote-tracking branch 'origin/main' into feat/update-async-pr-…
mattKorwel dcb7dd7
feat(skills): add macOS notifications to async-pr-review
mattKorwel 3f7523b
Merge branch 'main' into feat/update-async-pr-review
mattKorwel ee5d325
feat(skills): improve async-pr-review test running and diff analysis
mattKorwel ccf29e5
fix(skills): remove full test suite fallback in async review
mattKorwel 9f46923
fix(skills): handle pending CI checks in async pr review test step
mattKorwel fb0a1e1
Update .gemini/skills/async-pr-review/SKILL.md
mattKorwel 0c8b8bd
feat(skills): migrate async-pr-review to use policy.toml
mattKorwel 739de66
fix(skills): secure and complete async-pr-review policy.toml
mattKorwel 6e79770
fix(skills): restrict gh pr commands to read-only in policy.toml
mattKorwel 0052657
feat(skills): enrich async-pr-review policy with core unix and read-o…
mattKorwel 078ad35
Merge branch 'main' into feat/update-async-pr-review
mattKorwel 17f3266
Apply suggestion from @mattKorwel
mattKorwel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| --- | ||
| name: async-pr-review | ||
| description: Trigger this skill when the user wants to start an asynchronous PR review, run background checks on a PR, or check the status of a previously started async PR review. | ||
| --- | ||
|
|
||
| # Async PR Review | ||
|
|
||
| This skill provides a set of tools to asynchronously review a Pull Request. It will create a background job to run the project's preflight checks, execute Gemini-powered test plans, and perform a comprehensive code review using custom prompts. | ||
|
|
||
| This skill is designed to showcase an advanced "Agentic Asynchronous Pattern": | ||
| 1. **Native Background Shells vs Headless Inference**: While Gemini CLI can natively spawn and detach background shell commands (using the `run_shell_command` tool with `is_background: true`), a standard bash background job cannot perform LLM inference. To conduct AI-driven code reviews and test generation in the background, the shell script *must* invoke the `gemini` executable headlessly using `--approval-mode=yolo`. This offloads the AI tasks to independent worker agents. | ||
| 2. **Dynamic Git Scoping**: The review scripts avoid hardcoded paths. They use `git rev-parse --show-toplevel` to automatically resolve the root of the user's current project. | ||
| 3. **Ephemeral Worktrees**: Instead of checking out branches in the user's main workspace, the skill provisions temporary git worktrees in `.gemini/tmp/async-reviews/pr-<number>`. This prevents git lock conflicts and namespace pollution. | ||
| 4. **Agentic Evaluation (`check-async-review.sh`)**: The check script outputs clean JSON/text statuses for the main agent to parse. The interactive agent itself synthesizes the final assessment dynamically from the generated log files. | ||
|
|
||
| ## Workflow | ||
|
|
||
| 1. **Determine Action**: Establish whether the user wants to start a new async review or check the status of an existing one. | ||
| * If the user says "start an async review for PR #123" or similar, proceed to **Start Review**. | ||
| * If the user says "check the status of my async review for PR #123" or similar, proceed to **Check Status**. | ||
|
|
||
| ### Start Review | ||
|
|
||
| If the user wants to start a new async PR review: | ||
|
|
||
| 1. Ask the user for the PR number if they haven't provided it. | ||
| 2. Execute the `async-review.sh` script, passing the PR number as the first argument. Be sure to run it with the `is_background` flag set to true to ensure it immediately detaches. | ||
| ```bash | ||
| .gemini/skills/async-pr-review/scripts/async-review.sh <PR_NUMBER> | ||
| ``` | ||
| 3. Inform the user that the tasks have started successfully and they can check the status later. | ||
|
|
||
| ### Check Status | ||
|
|
||
| If the user wants to check the status or view the final assessment of a previously started async review: | ||
|
|
||
| 1. Ask the user for the PR number if they haven't provided it. | ||
| 2. Execute the `check-async-review.sh` script, passing the PR number as the first argument: | ||
| ```bash | ||
| .gemini/skills/async-pr-review/scripts/check-async-review.sh <PR_NUMBER> | ||
| ``` | ||
| 3. **Evaluate Output**: Read the output from the script. | ||
| * If the output contains `STATUS: IN_PROGRESS`, tell the user which tasks are still running. | ||
| * If the output contains `STATUS: COMPLETE`, use your file reading tools (`read_file`) to retrieve the contents of `final-assessment.md`, `review.md`, `pr-diff.diff`, `npm-test.log`, and `test-execution.log` files from the `LOG_DIR` specified in the output. | ||
| * **Final Assessment**: Read those files, synthesize their results, and give the user a concise recommendation on whether the PR builds successfully, passes tests, and if you recommend they approve it based on the review. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,240 @@ | ||
| #!/bin/bash | ||
|
|
||
| notify() { | ||
| local title="$1" | ||
| local message="$2" | ||
| local pr="$3" | ||
| # Terminal escape sequence | ||
| printf "\e]9;%s | PR #%s | %s\a" "$title" "$pr" "$message" | ||
| # Native macOS notification | ||
| if [[ "$(uname)" == "Darwin" ]]; then | ||
| osascript -e "display notification \"$message\" with title \"$title\" subtitle \"PR #$pr\"" | ||
| fi | ||
| } | ||
|
|
||
| pr_number=$1 | ||
| if [[ -z "$pr_number" ]]; then | ||
| echo "Usage: async-review <pr_number>" | ||
| exit 1 | ||
| fi | ||
|
|
||
| base_dir=$(git rev-parse --show-toplevel 2>/dev/null) | ||
| if [[ -z "$base_dir" ]]; then | ||
| echo "❌ Must be run from within a git repository." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Use the repository's local .gemini/tmp directory for ephemeral worktrees and logs | ||
| pr_dir="$base_dir/.gemini/tmp/async-reviews/pr-$pr_number" | ||
| target_dir="$pr_dir/worktree" | ||
| log_dir="$pr_dir/logs" | ||
|
|
||
| cd "$base_dir" || exit 1 | ||
|
|
||
| mkdir -p "$log_dir" | ||
| rm -f "$log_dir/setup.exit" "$log_dir/final-assessment.exit" "$log_dir/final-assessment.md" | ||
|
|
||
| echo "🧹 Cleaning up previous worktree if it exists..." | tee -a "$log_dir/setup.log" | ||
| git worktree remove -f "$target_dir" >> "$log_dir/setup.log" 2>&1 || true | ||
| git branch -D "gemini-async-pr-$pr_number" >> "$log_dir/setup.log" 2>&1 || true | ||
| git worktree prune >> "$log_dir/setup.log" 2>&1 || true | ||
|
|
||
| echo "📡 Fetching PR #$pr_number..." | tee -a "$log_dir/setup.log" | ||
| if ! git fetch origin -f "pull/$pr_number/head:gemini-async-pr-$pr_number" >> "$log_dir/setup.log" 2>&1; then | ||
| echo 1 > "$log_dir/setup.exit" | ||
| echo "❌ Fetch failed. Check $log_dir/setup.log" | ||
| notify "Async Review Failed" "Fetch failed." "$pr_number" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ! -d "$target_dir" ]]; then | ||
| echo "🧹 Pruning missing worktrees..." | tee -a "$log_dir/setup.log" | ||
| git worktree prune >> "$log_dir/setup.log" 2>&1 | ||
| echo "🌿 Creating worktree in $target_dir..." | tee -a "$log_dir/setup.log" | ||
| if ! git worktree add "$target_dir" "gemini-async-pr-$pr_number" >> "$log_dir/setup.log" 2>&1; then | ||
| echo 1 > "$log_dir/setup.exit" | ||
| echo "❌ Worktree creation failed. Check $log_dir/setup.log" | ||
| notify "Async Review Failed" "Worktree creation failed." "$pr_number" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "🌿 Worktree already exists." | tee -a "$log_dir/setup.log" | ||
| fi | ||
| echo 0 > "$log_dir/setup.exit" | ||
|
|
||
| cd "$target_dir" || exit 1 | ||
|
|
||
| echo "🚀 Launching background tasks. Logs saving to: $log_dir" | ||
|
|
||
| echo " ↳ [1/5] Grabbing PR diff..." | ||
| rm -f "$log_dir/pr-diff.exit" | ||
| { gh pr diff "$pr_number" > "$log_dir/pr-diff.diff" 2>&1; echo $? > "$log_dir/pr-diff.exit"; } & | ||
|
|
||
| echo " ↳ [2/5] Starting build and lint..." | ||
| rm -f "$log_dir/build-and-lint.exit" | ||
| { { npm run clean && npm ci && npm run format && npm run build && npm run lint:ci && npm run typecheck; } > "$log_dir/build-and-lint.log" 2>&1; echo $? > "$log_dir/build-and-lint.exit"; } & | ||
|
|
||
| # Dynamically resolve gemini binary (fallback to your nightly path) | ||
| GEMINI_CMD=$(which gemini || echo "$HOME/.gcli/nightly/node_modules/.bin/gemini") | ||
|
|
||
| echo " ↳ [3/5] Starting Gemini code review..." | ||
| rm -f "$log_dir/review.exit" | ||
| { "$GEMINI_CMD" --approval-mode=yolo /review-frontend "$pr_number" > "$log_dir/review.md" 2>&1; echo $? > "$log_dir/review.exit"; } & | ||
|
|
||
| echo " ↳ [4/5] Starting automated tests (waiting for build and lint)..." | ||
| rm -f "$log_dir/npm-test.exit" | ||
| { | ||
| while [ ! -f "$log_dir/build-and-lint.exit" ]; do sleep 1; done | ||
| if [ "$(cat "$log_dir/build-and-lint.exit")" == "0" ]; then | ||
| gh pr checks "$pr_number" > "$log_dir/ci-checks.log" 2>&1 | ||
| ci_status=$? | ||
|
|
||
| if [ "$ci_status" -eq 0 ]; then | ||
| echo "CI checks passed. Skipping local npm tests." > "$log_dir/npm-test.log" | ||
| echo 0 > "$log_dir/npm-test.exit" | ||
| elif [ "$ci_status" -eq 8 ]; then | ||
| echo "CI checks are still pending. Skipping local npm tests to avoid duplicate work. Please check GitHub for final results." > "$log_dir/npm-test.log" | ||
| echo 0 > "$log_dir/npm-test.exit" | ||
| else | ||
| echo "CI checks failed. Failing checks:" > "$log_dir/npm-test.log" | ||
| gh pr checks "$pr_number" --json name,bucket -q '.[] | select(.bucket=="fail") | .name' >> "$log_dir/npm-test.log" 2>&1 | ||
|
|
||
| echo "Attempting to extract failing test files from CI logs..." >> "$log_dir/npm-test.log" | ||
| pr_branch=$(gh pr view "$pr_number" --json headRefName -q '.headRefName' 2>/dev/null) | ||
| run_id=$(gh run list --branch "$pr_branch" --workflow ci.yml --json databaseId -q '.[0].databaseId' 2>/dev/null) | ||
|
|
||
| failed_files="" | ||
| if [[ -n "$run_id" ]]; then | ||
| failed_files=$(gh run view "$run_id" --log-failed 2>/dev/null | grep -o -E '(packages/[a-zA-Z0-9_-]+|integration-tests|evals)/[a-zA-Z0-9_/-]+\.test\.ts(x)?' | sort | uniq) | ||
| fi | ||
|
|
||
| if [[ -n "$failed_files" ]]; then | ||
| echo "Found failing test files from CI:" >> "$log_dir/npm-test.log" | ||
| for f in $failed_files; do echo " - $f" >> "$log_dir/npm-test.log"; done | ||
| echo "Running ONLY failing tests locally..." >> "$log_dir/npm-test.log" | ||
|
|
||
| exit_code=0 | ||
| for file in $failed_files; do | ||
| if [[ "$file" == packages/* ]]; then | ||
| ws_dir=$(echo "$file" | cut -d'/' -f1,2) | ||
| else | ||
| ws_dir=$(echo "$file" | cut -d'/' -f1) | ||
| fi | ||
| rel_file=${file#$ws_dir/} | ||
|
|
||
| echo "--- Running $rel_file in workspace $ws_dir ---" >> "$log_dir/npm-test.log" | ||
| if ! npm run test:ci -w "$ws_dir" -- "$rel_file" >> "$log_dir/npm-test.log" 2>&1; then | ||
| exit_code=1 | ||
| fi | ||
| done | ||
| echo $exit_code > "$log_dir/npm-test.exit" | ||
| else | ||
| echo "Could not extract specific failing files. Skipping full local test suite as it takes too long. Please check CI logs manually." >> "$log_dir/npm-test.log" | ||
| echo 1 > "$log_dir/npm-test.exit" | ||
| fi | ||
| fi | ||
| else | ||
| echo "Skipped due to build-and-lint failure" > "$log_dir/npm-test.log" | ||
| echo 1 > "$log_dir/npm-test.exit" | ||
| fi | ||
| } & | ||
|
|
||
| echo " ↳ [5/5] Starting Gemini test execution (waiting for build and lint)..." | ||
| rm -f "$log_dir/test-execution.exit" | ||
| { | ||
| while [ ! -f "$log_dir/build-and-lint.exit" ]; do sleep 1; done | ||
| if [ "$(cat "$log_dir/build-and-lint.exit")" == "0" ]; then | ||
| "$GEMINI_CMD" --approval-mode=yolo "Analyze the diff for PR $pr_number using 'gh pr diff $pr_number'. Instead of running the project's automated test suite (like 'npm test'), physically exercise the newly changed code in the terminal (e.g., by writing a temporary script to call the new functions, or testing the CLI command directly). Verify the feature's behavior works as expected. IMPORTANT: Do NOT modify any source code to fix errors. Just exercise the code and log the results, reporting any failures clearly. Do not ask for user confirmation." > "$log_dir/test-execution.log" 2>&1; echo $? > "$log_dir/test-execution.exit" | ||
| else | ||
| echo "Skipped due to build-and-lint failure" > "$log_dir/test-execution.log" | ||
| echo 1 > "$log_dir/test-execution.exit" | ||
| fi | ||
| } & | ||
|
|
||
| echo "✅ All tasks dispatched!" | ||
| echo "You can monitor progress with: tail -f $log_dir/*.log" | ||
| echo "Read your review later at: $log_dir/review.md" | ||
|
|
||
| # Polling loop to wait for all background tasks to finish | ||
| tasks=("pr-diff" "build-and-lint" "review" "npm-test" "test-execution") | ||
| log_files=("pr-diff.diff" "build-and-lint.log" "review.md" "npm-test.log" "test-execution.log") | ||
|
|
||
| declare -A task_done | ||
| for t in "${tasks[@]}"; do task_done[$t]=0; done | ||
|
|
||
| all_done=0 | ||
| while [[ $all_done -eq 0 ]]; do | ||
| clear | ||
| echo "==================================================" | ||
| echo "🚀 Async PR Review Status for PR #$pr_number" | ||
| echo "==================================================" | ||
| echo "" | ||
|
|
||
| all_done=1 | ||
| for i in "${!tasks[@]}"; do | ||
| t="${tasks[$i]}" | ||
|
|
||
| if [[ -f "$log_dir/$t.exit" ]]; then | ||
| exit_code=$(cat "$log_dir/$t.exit") | ||
| if [[ "$exit_code" == "0" ]]; then | ||
| echo " ✅ $t: SUCCESS" | ||
| else | ||
| echo " ❌ $t: FAILED (exit code $exit_code)" | ||
| fi | ||
| task_done[$t]=1 | ||
| else | ||
| echo " ⏳ $t: RUNNING" | ||
| all_done=0 | ||
| fi | ||
| done | ||
|
|
||
| echo "" | ||
| echo "==================================================" | ||
| echo "📝 Live Logs (Last 5 lines of running tasks)" | ||
| echo "==================================================" | ||
|
|
||
| for i in "${!tasks[@]}"; do | ||
| t="${tasks[$i]}" | ||
| log_file="${log_files[$i]}" | ||
|
|
||
| if [[ ${task_done[$t]} -eq 0 ]]; then | ||
| if [[ -f "$log_dir/$log_file" ]]; then | ||
| echo "" | ||
| echo "--- $t ---" | ||
| tail -n 5 "$log_dir/$log_file" | ||
| fi | ||
| fi | ||
| done | ||
|
|
||
| if [[ $all_done -eq 0 ]]; then | ||
| sleep 3 | ||
| fi | ||
| done | ||
|
|
||
| clear | ||
| echo "==================================================" | ||
| echo "🚀 Async PR Review Status for PR #$pr_number" | ||
| echo "==================================================" | ||
| echo "" | ||
| for t in "${tasks[@]}"; do | ||
| exit_code=$(cat "$log_dir/$t.exit") | ||
| if [[ "$exit_code" == "0" ]]; then | ||
| echo " ✅ $t: SUCCESS" | ||
| else | ||
| echo " ❌ $t: FAILED (exit code $exit_code)" | ||
| fi | ||
| done | ||
| echo "" | ||
|
|
||
| echo "⏳ Tasks complete! Synthesizing final assessment..." | ||
| if ! "$GEMINI_CMD" --approval-mode=yolo -p "Read the review at $log_dir/review.md, the automated test logs at $log_dir/npm-test.log, and the manual test execution logs at $log_dir/test-execution.log. Summarize the results, state whether the build and tests passed based on $log_dir/build-and-lint.exit and $log_dir/npm-test.exit, and give a final recommendation for PR $pr_number." > "$log_dir/final-assessment.md" 2>&1; then | ||
| echo $? > "$log_dir/final-assessment.exit" | ||
| echo "❌ Final assessment synthesis failed!" | ||
| echo "Check $log_dir/final-assessment.md for details." | ||
| notify "Async Review Failed" "Final assessment synthesis failed." "$pr_number" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo 0 > "$log_dir/final-assessment.exit" | ||
| echo "✅ Final assessment complete! Check $log_dir/final-assessment.md" | ||
| notify "Async Review Complete" "Review and test execution finished successfully." "$pr_number" |
65 changes: 65 additions & 0 deletions
65
.gemini/skills/async-pr-review/scripts/check-async-review.sh
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| #!/bin/bash | ||
| pr_number=$1 | ||
|
|
||
| if [[ -z "$pr_number" ]]; then | ||
| echo "Usage: check-async-review <pr_number>" | ||
| exit 1 | ||
| fi | ||
|
|
||
| base_dir=$(git rev-parse --show-toplevel 2>/dev/null) | ||
| if [[ -z "$base_dir" ]]; then | ||
| echo "❌ Must be run from within a git repository." | ||
| exit 1 | ||
| fi | ||
|
|
||
| log_dir="$base_dir/.gemini/tmp/async-reviews/pr-$pr_number/logs" | ||
|
|
||
| if [[ ! -d "$log_dir" ]]; then | ||
| echo "STATUS: NOT_FOUND" | ||
| echo "❌ No logs found for PR #$pr_number in $log_dir" | ||
| exit 0 | ||
| fi | ||
|
|
||
| tasks=( | ||
| "setup|setup.log" | ||
| "pr-diff|pr-diff.diff" | ||
| "build-and-lint|build-and-lint.log" | ||
| "review|review.md" | ||
| "npm-test|npm-test.log" | ||
| "test-execution|test-execution.log" | ||
| "final-assessment|final-assessment.md" | ||
| ) | ||
|
|
||
| all_done=true | ||
| echo "STATUS: CHECKING" | ||
|
|
||
| for task_info in "${tasks[@]}"; do | ||
| IFS="|" read -r task_name log_file <<< "$task_info" | ||
|
|
||
| file_path="$log_dir/$log_file" | ||
| exit_file="$log_dir/$task_name.exit" | ||
|
|
||
| if [[ -f "$exit_file" ]]; then | ||
| exit_code=$(cat "$exit_file") | ||
| if [[ "$exit_code" == "0" ]]; then | ||
| echo "✅ $task_name: SUCCESS" | ||
| else | ||
| echo "❌ $task_name: FAILED (exit code $exit_code)" | ||
| echo " Last lines of $file_path:" | ||
| tail -n 3 "$file_path" | sed 's/^/ /' | ||
| fi | ||
| elif [[ -f "$file_path" ]]; then | ||
| echo "⏳ $task_name: RUNNING" | ||
| all_done=false | ||
| else | ||
| echo "➖ $task_name: NOT STARTED" | ||
| all_done=false | ||
| fi | ||
| done | ||
|
|
||
| if $all_done; then | ||
| echo "STATUS: COMPLETE" | ||
| echo "LOG_DIR: $log_dir" | ||
| else | ||
| echo "STATUS: IN_PROGRESS" | ||
| fi |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.