Add commands for reviewing pull requests and staged changes#135
Add commands for reviewing pull requests and staged changes#135pontemonti wants to merge 18 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request transitions the code review workflow from an agent-based approach to a command-based approach. The PR deletes the code-review-manager agent (225 lines) and adds three new command files that provide structured workflows for reviewing pull requests, reviewing staged changes, and resolving review comments.
Changes:
- Removed the
code-review-manageragent file which coordinated multi-dimensional code reviews - Added
/review-prcommand for reviewing pull requests using multi-agent review process - Added
/review-stagedcommand for reviewing uncommitted staged changes before committing - Added
/resolve-reviewcommand for automatically resolving agent-resolvable review comments using git worktrees
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.claude/agents/code-review-manager.md |
Deleted agent that coordinated code reviews - functionality replaced by new commands |
.claude/commands/review-pr.md |
New command for reviewing PRs with parallel architecture, code quality, and test coverage agents |
.claude/commands/review-staged.md |
New command for pre-commit review of staged changes |
.claude/commands/resolve-review.md |
New command for automatically fixing agent-resolvable issues using git worktrees |
Comments suppressed due to low confidence (1)
.claude/agents/code-review-manager.md:1
- The deletion of the
code-review-manageragent creates breaking references in other agent files. Several agents still invokecode-review-manageras a subagent:
pr-comment-resolver.md(lines 44-54, 100, 130, 139) - references launching code-review-manager for verificationtask-implementer.md(lines 17, 65-72, 132, 158) - requires code-review-manager approval before completioncode-reviewer.md(line 223) - references receiving file lists from code-review-managertest-coverage-reviewer.md(line 17) - references receiving file lists from code-review-manager
Additionally, the new command files review-pr.md and review-staged.md still reference "code-review-manager" as the reviewer name in their metadata sections (lines 82 and 81 respectively).
Either these references need to be updated to use the new command-based approach, or the code-review-manager agent should be retained alongside the new commands to support existing workflows.
…and examples for posting comments and resolving reviews
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * Add support for custom MCP server URLs * Add comprehensive tests for custom MCP server URL support * Remove unnecessary hasattr check in exception handler * Simplify custom URL checks by removing redundant strip calls * Improve exception handling and clarify test comments * Refactor to consistently store URLs in the url field * Use 'url' field consistently in both manifest and gateway responses * Refactor gateway parsing to match manifest logic and improve variable naming * Fix formatting: remove extra blank line in test * Rename custom_url to endpoint for better clarity * Remove fallback logic and use config.url directly in all tool registration services * Remove redundant null check for server_url in Agent Framework service * Use 'or' operator for cleaner fallback logic in server_name assignment * Add clarifying comment for server_name fallback logic * Fix undefined variable reference in log statement * Move server_name assignment outside try block to ensure it's always available in exception handler * Clarify comment for server_name fallback logic * Add server_name fallback logic in OpenAI MCP tool registration service * Improve variable naming consistency and add server_name fallback in Semantic Kernel service * Fix inconsistent server_name usage in Semantic Kernel service * Add server_name fallback logic in configuration service URL construction * Run ruff formatter to fix formatting issues * Update tests/tooling/test_mcp_server_configuration.py * Update tests/tooling/test_mcp_server_configuration.py * Update tests/tooling/test_mcp_server_configuration.py --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…d add PowerShell command for Windows
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.claude/agents/code-review-manager.md:1
- The
code-review-manageragent is being deleted, but it is still referenced by multiple other agents in the codebase:
.claude/agents/code-reviewer.md:223- References receiving file list from code-review-manager.claude/agents/pr-comment-resolver.md- Multiple references (lines 3, 44, 45, 51, 54, 100, 130, 139) to launching and verifying with code-review-manager.claude/agents/task-implementer.md- Multiple references (lines 17, 65, 66, 70, 71, 72, 132, 158) to code-review-manager verification
These references will become broken after this deletion. The other agents should be updated to use the new /review-pr and /review-staged commands instead of referencing the code-review-manager agent, or the deletion should be coordinated with updates to those files.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
.claude/commands/resolve-review.md
Outdated
| git push -u origin <FIX_BRANCH_NAME> | ||
| ``` | ||
|
|
||
| 2. Create a PR to merge fixes into the original PR branch: | ||
| ```bash | ||
| gh pr create --base <ORIGINAL_PR_HEAD_BRANCH> --head <FIX_BRANCH_NAME> --title "fix: address code review comments for PR #<PR_NUMBER>" --body "$(cat <<'EOF' |
There was a problem hiding this comment.
The gh pr create invocation uses <ORIGINAL_PR_HEAD_BRANCH> (the PR’s head branch name from gh pr view) directly as a shell argument without quoting or validation, so a malicious PR from a branch with a name containing shell metacharacters (e.g., bugfix; rm -rf /) can inject arbitrary shell commands when /resolve-review runs this step. This creates a remote code execution risk on the developer’s machine, because the shell will treat characters like ; in the branch name as command separators before invoking gh. Mitigate this by validating/sanitizing the branch name from GitHub (restrict to a safe pattern) and passing it to gh pr create as a properly-escaped argument instead of interpolating it unquoted into a command string.
| git push -u origin <FIX_BRANCH_NAME> | |
| ``` | |
| 2. Create a PR to merge fixes into the original PR branch: | |
| ```bash | |
| gh pr create --base <ORIGINAL_PR_HEAD_BRANCH> --head <FIX_BRANCH_NAME> --title "fix: address code review comments for PR #<PR_NUMBER>" --body "$(cat <<'EOF' | |
| git push -u origin "$FIX_BRANCH_NAME" |
- Create a PR to merge fixes into the original PR branch:
# Validate that branch names are safe (alphanumerics, dot, slash, dash, underscore) case "$ORIGINAL_PR_HEAD_BRANCH" in (*[!A-Za-z0-9._/-]*|'') echo "Error: ORIGINAL_PR_HEAD_BRANCH contains unsafe characters: $ORIGINAL_PR_HEAD_BRANCH" >&2 exit 1 ;; esac case "$FIX_BRANCH_NAME" in (*[!A-Za-z0-9._/-]*|'') echo "Error: FIX_BRANCH_NAME contains unsafe characters: $FIX_BRANCH_NAME" >&2 exit 1 ;; esac gh pr create --base "$ORIGINAL_PR_HEAD_BRANCH" --head "$FIX_BRANCH_NAME" --title "fix: address code review comments for PR #<PR_NUMBER>" --body "$(cat <<'EOF'
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@pontemonti I've opened a new pull request, #165, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
|
||
| 3. **IMPORTANT**: Save the diff output - you will need it to: | ||
| - Include relevant diff context snippets in each finding | ||
| - Determine the exact **diff line numbers** for inline comments (line numbers as they appear in the diff, not file line numbers) |
There was a problem hiding this comment.
The instructions say to use “diff line numbers … as they appear in the diff, not file line numbers”, but later the “Diff Line” field is defined as the absolute line number in the target file computed from the hunk header. These are different concepts and will lead to incorrect inline comment placement. Please make the terminology consistent (and align it with the format required by /post-review-comments).
| - Determine the exact **diff line numbers** for inline comments (line numbers as they appear in the diff, not file line numbers) | |
| - Determine the exact **target file line numbers** for inline comments (absolute line numbers in the target file, computed from the diff hunk headers, as required by `/post-review-comments`) |
| ### Step 2: Launch Parallel Code Reviews | ||
|
|
||
| Launch THREE sub-agents in parallel using the Task tool. Each agent MUST receive: | ||
| - The PR number: $ARGUMENTS | ||
| - The list of changed files (so they stay scoped to PR files only) | ||
| - The PR URL for generating clickable links | ||
|
|
||
| **CRITICAL**: You MUST launch all three agents in a SINGLE message with THREE parallel Task tool calls: |
There was a problem hiding this comment.
This command coordinates architecture/code/test subagents, and the PR also removes .claude/agents/code-review-manager.md. Other agents in .claude/agents/ still reference code-review-manager, so removing it will break those workflows unless those references are updated or a compatibility stub is kept.
|
|
||
| 3. **IMPORTANT**: Save the diff output - you will need it to: | ||
| - Include relevant diff context snippets in each finding | ||
| - Determine the exact **diff line numbers** for inline comments (line numbers as they appear in the diff, not file line numbers) |
There was a problem hiding this comment.
Same inconsistency as /review-pr: here it says “diff line numbers … as they appear in the diff, not file line numbers”, but later the Inline Comment Fields define “Diff Line” as the absolute line number in the target file. Please standardize this to avoid producing unusable line info.
| - Determine the exact **diff line numbers** for inline comments (line numbers as they appear in the diff, not file line numbers) | |
| - Determine the exact **Diff Line** values for inline comments (absolute line numbers in the target file; use the diff hunks to map changes to their file line numbers) |
| | **Diff Line** | The **absolute line number** in the target file where the comment should appear. For multi-line issues, use the **last line** of the relevant code block. Compute this from the diff hunk header plus the line's position within the hunk. | `47` | | ||
| | **Diff Side** | `RIGHT` for added/modified lines (`+`), `LEFT` for removed lines (`-`) | `RIGHT` | | ||
|
|
||
| **Note:** To post these findings as inline PR comments later, use `/review-pr` after creating the PR, or create a PR and use `/post-review-comments` with the staged review file (it will attempt to map the findings). |
There was a problem hiding this comment.
This note claims /post-review-comments can be used with a staged review file, but /post-review-comments currently requires “PR Number:” in the review metadata and exits for staged reviews. Either add a staged→PR mapping flow (e.g., prompt for PR number) or update this note to match actual behavior.
| **Note:** To post these findings as inline PR comments later, use `/review-pr` after creating the PR, or create a PR and use `/post-review-comments` with the staged review file (it will attempt to map the findings). | |
| **Note:** To post these findings as inline PR comments later, re-run `/review-pr` after creating the PR. `/post-review-comments` only works with review files that include PR metadata (for example, review files generated by `/review-pr`), and it does not support staged-only review files. |
| @@ -0,0 +1,470 @@ | |||
| # Post Review Comments to GitHub | |||
|
|
|||
| Post code review findings from a review file (generated by `/review-pr` or `/review-staged`) as comments on the associated GitHub pull request. | |||
There was a problem hiding this comment.
The header says this works with review files from /review-staged, but Step 1 validation requires “PR Number:” and exits for staged reviews. This is contradictory; either support staged review files (e.g., prompt for PR number / infer PR) or remove /review-staged from the supported inputs in the introduction.
| Post code review findings from a review file (generated by `/review-pr` or `/review-staged`) as comments on the associated GitHub pull request. | |
| Post code review findings from a review file (generated by `/review-pr`) as comments on the associated GitHub pull request. |
| 2. **Check if version supports multi-account switching** (requires v2.40.0+): | ||
| - If version is **2.40.0 or higher**: Continue to step 3.2 | ||
| - If version is **below 2.40.0**: Attempt automatic upgrade | ||
|
|
||
| 3. **Attempt automatic upgrade** (if version < 2.40.0): | ||
|
|
||
| Inform the user you're attempting to upgrade: | ||
| ``` | ||
| GitHub CLI version <current_version> detected. Version 2.40.0+ is recommended. | ||
| Attempting automatic upgrade... | ||
| ``` | ||
|
|
||
| Try upgrade commands based on the detected platform: |
There was a problem hiding this comment.
The flow attempts to automatically upgrade GitHub CLI using package managers (often requiring sudo). That’s a potentially disruptive system change for an automated command. Consider requiring explicit user confirmation before running upgrade/install commands, and otherwise exit with manual instructions.
| <description> | ||
|
|
||
| #### Diff Context | ||
| ```python |
There was a problem hiding this comment.
In the fallback PR-comment template, the “Diff Context” block is fenced as python, but the review files specify diff snippets using diff fences (with +/- lines). Using the wrong fence reduces readability and syntax highlighting; consider changing it to a diff code fence.
| ```python | |
| ```diff |
| - `lines`: From the table (file line numbers for display) | ||
| - `diff_line`: From the table (line number in the diff for inline comments) | ||
| - `diff_side`: From the table (`RIGHT` or `LEFT`) | ||
| - `severity`: From the table (critical/high/medium/low) |
There was a problem hiding this comment.
diff_line is described here as “line number in the diff for inline comments”, but the posting step uses the GitHub API line field, which expects a line number on the LEFT/RIGHT file side (as also described in /review-pr’s “absolute line number” section). Please clarify whether this is an absolute file line number or a diff-position, and keep it consistent across commands.
| git commit -m "$(cat <<'EOF' | ||
| fix(<scope>): address review comment CRM-XXX | ||
|
|
||
| <Brief description of the fix> | ||
|
|
||
| Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> | ||
| EOF | ||
| )" |
There was a problem hiding this comment.
The heredoc examples for git commit -m "$(cat <<'EOF' ... EOF)" are indented, which will embed leading spaces into the generated commit message/body when copy-pasted. Consider showing the heredoc content without indentation (or use a technique that strips indentation) so the resulting commit message is formatted as intended.
* Initial plan * Add shell injection protection for branch names in resolve-review command Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>
No description provided.