# Review Changes > Review code changes with a small team — two senior engineers review independently, debate their findings, then a team lead reports a consolidated list. Local mode compares the current branch against a base branch; PR mode (auto-enabled when a PR URL or number is supplied) offers to post the findings as PR review comments. PR mode is strictly read-only — no file in the working tree is ever modified, and every developer instruction is interpreted as an update to the comments plan. The agent never checks out a branch in the current working directory; if it needs files at another ref, it creates a temporary worktree and removes it before presenting the report. Local mode lets the developer pick findings to fix, and the same two engineers implement them in auto mode. Engineers read the project's AI documentation (architecture first) before reviewing, and explicitly check for reuse violations and unused established practices. - Skill: `morphet81/review-changes` (Agent Skill) - Install (CLI): `npx skillmds@latest add morphet81/review-changes` - Raw SKILL.md: https://api.skillmd.com/api/skills/morphet81/review-changes/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: AI & ML - Author: morphet81 (https://skillmd.com/u/morphet81) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/morphet81/review-changes --- Review code changes with a focused 3-agent team: - Two **senior engineers** each review the changes independently, covering every focus area. - They share findings, **challenge each other**, and converge on a consolidated list. - A **team lead** receives the consolidated list and reports it back to the developer. - **PR mode** (auto-detected when the argument is a PR URL or number): the team lead offers to post the findings as pending PR review comments. - **Local mode**: the team lead shows the findings and asks which to address now. Selected findings are implemented by the same two engineers in auto mode. **Usage:** - `/review-changes` — Local review against `main` - `/review-changes ` — Local review against `` - `/review-changes ` — Review the PR in the current repo - `/review-changes ` — Review a PR by URL (use this for PRs in another repo) **Instructions:** 1. **Parse arguments and determine mode:** `$1` is the only argument. - If `$1` matches a GitHub PR URL (`https://github.com/.../pull/\d+`) → **PR mode**. Extract owner, repo, and PR number from the URL. - If `$1` is a pure number → **PR mode**. Use it as the PR number, and run `gh repo view --json nameWithOwner -q .nameWithOwner` to derive the current repo. For a PR in another repo, pass the full PR URL instead. - Otherwise → **local mode**. Base branch = `$1` if given, else the `baseBranch=...` value from a `.agent` file in the working directory if present, else `main`. > ## ⚠️ PR mode is strictly read-only > > Once the skill is in **PR mode**, neither the team lead nor either engineer may modify any file in the working tree under any circumstances. **Step 9 (Address findings) is skipped entirely in PR mode**, no matter what the developer says. > > Every developer instruction received in PR mode must be interpreted as a **change to the comments plan** — the set of findings staged to be posted on the PR — *not* as a code-fix request. Examples: > > | Developer says… | Interpret as… | > |---|---| > | "Drop #3" | Remove #3 from the comments plan | > | "Make #5 critical" | Change #5's severity to 🔴 Critical in the plan | > | "Add a note about X to #4" | Edit #4's body in the plan | > | "Combine #6 and #7" | Merge those entries in the plan | > | "Fix #2" / "Apply #2" | **Plan update**: ask the developer whether they meant *drop #2 because they'll fix it themselves* or *rephrase #2 to suggest a specific fix*. Never run any code-modifying command. | > > If a developer explicitly insists on fixing code in PR mode, do **not** comply. Reply: "PR mode is read-only. To apply fixes, re-run `/review-changes` without the PR argument while checked out on the PR branch." Then continue refining the comments plan. > ## ⚠️ Never check out a branch in the current working directory > > Applies to **both modes**, at all times. The skill must never change which ref is checked out in the developer's working directory. **Do not** run `git checkout `, `git switch `, `git reset --hard`, or any equivalent that moves the current working tree's HEAD. The developer may have uncommitted work or staged changes — switching branches under them is unacceptable. > > **Local mode:** The current branch is already the one being reviewed; just `git diff ...HEAD`. No checkout needed. > > **PR mode:** The diff comes from `gh pr diff ` and metadata from `gh pr view `. If the engineers need to read additional files at the **PR's state** (e.g., the PR's `CLAUDE.md`, sibling files for convention checks), prefer `gh api repos///contents/?ref=` for a small number of files. If broader directory inspection is required, create a **temporary worktree**: > > ```bash > # Get the PR head SHA > SHA=$(gh pr view --repo --json headRefOid --jq .headRefOid) > # Create a worktree at a temp path, detached at that SHA > WT_PATH=$(mktemp -d -t review-changes-XXXXXX) > rmdir "$WT_PATH" # mktemp pre-creates; git worktree wants a non-existent path > git worktree add --detach "$WT_PATH" "$SHA" > ``` > > Engineers may read files from `$WT_PATH` (read-only — never write to it). **The worktree must be removed before the team lead presents the consolidated report to the developer** (i.e., between step 6 and step 7). Cleanup: > > ```bash > git worktree remove --force "$WT_PATH" > git worktree list # verify it is gone > ``` > > If cleanup fails (locked, busy, or some other error), the team lead must **not** present the report yet — surface the failure to the developer, list the leftover path, and ask them how to proceed. --- ## Local Mode — Gather Changes 2. **Gather branch changes:** - Get the current branch name and verify we are not on the base branch - `git diff ...HEAD` — full diff - `git log ..HEAD --oneline` — commit history - `git diff --name-only ...HEAD` — list of changed files --- ## PR Mode — Gather Context 2. **Gather PR context:** **a) Verify the PR exists and gather metadata:** ```bash gh pr view --repo --json number,title,url,state,baseRefName,headRefName,author,additions,deletions,changedFiles,files ``` - If the PR does not exist or is not open, display an error and **STOP**. - Store the PR metadata for later reference. **b) Fetch the full diff and changed files:** ```bash gh pr diff --repo ``` ```bash gh pr view --repo --json files --jq '.files[].path' ``` - If the diff exceeds ~5000 lines, note this for the engineers so they can prioritise the most impactful changes. **c) Check for a Jira ticket reference:** - Scan the PR title, description, and branch name for a Jira key (pattern: `[A-Z][A-Z0-9]+-[0-9]+`, e.g., `PROJ-123`). - If found, attempt to fetch the ticket details: ```bash acli jira workitem view --fields summary,description,acceptance-criteria --json ``` - If the fetch succeeds, pass the ticket details to both engineers and instruct the team lead to include a **Ticket Compliance** section in the final report. If it fails, note the ticket key but proceed without details. **d) Build the exclusion list and collect unresolved threads:** Findings in the exclusion list must be **excluded** from the final report (no duplication of existing review comments). Additionally, **every unresolved thread will receive a reply** when the lead posts comments (see step 8a). - Existing reviews, inline comments, and general comments — one call: ```bash gh pr view --repo --json reviews,reviewComments,comments ``` Record from the output: - `reviews`: author, state, body - `reviewComments`: file path, line, body, author - `comments`: substantive general PR conversation comments (skip bot comments and acknowledgements) - Unresolved review threads via GraphQL (since `gh pr view` does not expose resolution status): ```bash gh api graphql -f query=' query($owner: String!, $repo: String!, $pr: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr) { reviewThreads(first: 100) { nodes { id isResolved comments(first: 20) { nodes { body path line author { login } } } } } } } } ' -f owner='{owner}' -f repo='{repo}' -F pr={number} ``` Collect all **unresolved** threads with thread `id`, file paths, line numbers, and comment bodies. - Compile the exclusion list with: file path + line (if applicable), summary of the issue raised, author. - Store all unresolved threads separately — they will be replied to in step 8a. --- ## Shared Steps 3. **Spawn the review team:** Use `TeamCreate` with name `review-changes`. Spawn three agents simultaneously via the `Agent` tool (`subagent_type: general-purpose`, `run_in_background: true`, with the team name): | Name | Role | |------|------| | `engineer-1` | Senior Engineer — independent reviewer | | `engineer-2` | Senior Engineer — independent reviewer | | `team-lead` | Team Lead — coordinates the engineers and reports to the developer | **Shared brief for both engineers:** > You are one of two senior engineers reviewing this change set independently. Cover **every focus area** below. Do NOT communicate with the other engineer during your initial review — your value comes from forming an independent opinion. > > **Read the project's AI documentation first.** Before reviewing the diff, look for AI-oriented documentation in the repo. Common locations: > - `CLAUDE.md` (root, and per-directory if present) > - `AGENTS.md` > - `.claude/` and `.cursor/rules/` > - `docs/ai/`, `docs/agents/`, `docs/architecture/` > - `ARCHITECTURE.md`, `README.md` (architecture sections only) > > The documentation is often split across multiple files. **You must read the most important ones — especially anything describing the app's architecture, module boundaries, data flow, or core conventions** — because findings made without that context tend to be wrong. For multi-file doc sets, skim the index/README of the doc directory, then read only the entries relevant to the files you're reviewing. Do **not** read documentation that is clearly unrelated to the changes. If the project provides no AI documentation, skip this step. > > **Scope:** Focus on the changes introduced (the diff). Only review code that was added or modified — do not flag pre-existing issues in surrounding code. When reviewing new files or significant additions, read 2–3 sibling files (same directory, same type) to understand the existing conventions and flag deviations. > > **Only report potential issues.** Do NOT list things that look fine. Every finding must identify a concrete problem, risk, or improvement opportunity. If a focus area looks clean, simply state "No issues found". > > **Focus areas — cover all of them:** > - **Code Quality** — Bugs, edge cases, error handling. Think about data conflicts and overlaps — what happens when two items occupy the same slot, time range, or index? Question assumptions: if code skips or filters items, is the skip logic correct from a domain perspective? > - **Security** — Vulnerabilities: injection, XSS, secrets exposure, auth issues. > - **Performance** — Inefficiencies, bottlenecks, unnecessary allocations, resource usage. > - **Best Practices** — Coding standards, design patterns, conventions, consistency with sibling files. Flag hardcoded values that should use existing constants/variables. Flag dead or unreachable code. > - **Testing** — see the **Test Review Checklist** below. > - **Documentation** — Missing or outdated documentation, changelog needs, inline comment gaps. > > **Test Review Checklist** — read each new or modified test carefully and verify all of: > 1. **Description is generic enough** — describes behaviour, not implementation details (e.g., "shows an error" instead of "renders a red div with class `error-box`"). > 2. **Does not test implementation** — assertions target stable contracts, not styling or internal markup. Example: assert that a button has an `alert` class, not that it is red; assert that an error is announced to the user, not that a specific element exists. > 3. **No duplicates** — flag tests (or near-duplicate tests) that cover the same scenario. > 4. **No cross-component re-testing** — a component that uses another component must not re-test the inner component's behaviour. Each component has its own test suite; the outer test should only verify the integration, not the inner component's internals. > 5. **Description matches assertions** — for every test, verify the assertions actually prove what the description claims. Flag missing assertions (claim made, no assertion for it) and extraneous assertions (assertion not described). Flag silent passes (a test that would still pass if the feature it claims to cover were removed). > > **Reuse & Project Practices Checklist** — before approving any new code, verify all of: > 1. **No reinvention of existing components** — flag any new component, hook, helper, or utility that could have been created by **extending or reusing** an existing one. Grep the relevant directories for similar names, props, or responsibilities before assuming something is new. If the diff genuinely needs a new variant, it must come with a clear architectural rationale (in the code, commit message, PR description, or ticket); otherwise flag it as a reuse violation and recommend extending the existing primitive. > 2. **Established practices are used** — identify the project's higher-order patterns (read the AI documentation and skim sibling files for examples): things like a modal-visibility HOC/hook, a permissioned-button wrapper, a form helper, a request-state hook, a feature-flag gate, etc. Verify the new code uses them. **Example:** if the project provides a HOC or hook for managing modal visibility, any new conditionally-visible modal must use it instead of rolling its own `useState(open)` boilerplate. Flag any new code that bypasses an established practice without justification. > 3. **Existing utilities, constants, and shared variables are reused** — flag inline implementations of behaviour already covered by an existing utility, and hardcoded values that have a named constant or shared variable available. > > **Convention check:** Before reporting findings, scan the directory of each changed file to identify sibling files. Note any conventions (naming, patterns, utilities, shared variables) that the new code should follow but doesn't. > > When finished, message `team-lead` with your findings. For each finding, provide: title, `file:line`, severity (🔴 Critical / 🟡 Warning / 🔵 Suggestion), rationale, suggested fix. State "No issues found in " for any clean focus area. **Working-directory rule (both modes, applies to both engineers):** - **Never run `git checkout`, `git switch`, `git reset --hard`, or any command that changes which ref is checked out in the developer's working directory.** The developer may have uncommitted work. - To read files at a different ref, either use `gh api repos///contents/?ref=` for a few files, or ask the team lead to create a temporary worktree (see the top-level callout). Read from the worktree path; never write to it. **PR mode additions to the engineers' brief:** - **You must not modify the codebase in PR mode.** Do not run any file-editing, write, or shell command that mutates the working tree (including any worktree the lead creates). Your job is solely to read, review, and report findings to `team-lead`. If you are later asked to "fix" or "apply" something while still in PR mode, refuse and remind the lead that PR mode is read-only. - Attach the **exclusion list** with the instruction: "Do NOT report these issues — they have already been raised. For each excluded item that falls in a focus area you reviewed, send the lead a brief assessment: do you agree with the comment, and is the issue still present in the current code?" - Attach the **Jira ticket details** if available, with the instruction: "Also compare the changes against the ticket's requirements and acceptance criteria. Flag any missing or partially-addressed items." **Team Lead brief:** > You coordinate two independent senior engineers reviewing a change set. Your responsibilities: > 1. **Wait** for both engineers to submit their independent findings. > 2. **Share & challenge** — relay each engineer's findings to the other. Instruct each to challenge anything they disagree with (false positive, wrong severity, missing context) and to add anything the other engineer missed within their own analysis. > 3. **Discuss & consolidate** — facilitate a short discussion (one round-trip is usually enough) where the engineers converge on a single, deduplicated list of findings with agreed severities. When they genuinely disagree, keep the finding and mark it **Debated** with both perspectives noted. > 4. **Filter non-issues** — drop anything that concludes with "no action needed", "looks good", etc. Only actionable findings remain. > 5. **Produce the consolidated report** using the format in step 7. > 6. **Clean up any temporary worktree** you created for the engineers (see the working-directory callout at the top of this skill). This must happen **before** you present the report to the developer. If cleanup fails, hold the report and surface the problem. > 7. **Deliver the report** to the developer. In PR mode, also propose posting the findings as PR review comments (step 8a). In local mode, ask which findings to address now (step 8b). > 8. When fixes are required, **dispatch the same two engineers in auto mode** to implement them (step 9). **This applies in local mode only.** > 9. Once all work is done, send `shutdown_request` to both engineers and call `TeamDelete`. > > **Working-directory rule (critical):** Never check out a branch in the developer's working directory, and never instruct an engineer to do so. If files at a different ref are needed, create a temporary worktree with `git worktree add --detach `, share that path with the engineers, and remove the worktree with `git worktree remove --force ` before step 7. > > **PR mode read-only rule (critical):** In PR mode you must never dispatch engineers to modify the codebase. Treat every developer instruction as an update to the comments plan, never as a code-fix request. Do not run any file-editing or shell command that mutates the working tree, and do not allow either engineer to do so. If the developer phrases something ambiguously (e.g., "fix #3"), ask whether they mean "drop #3 because I'll fix it myself" or "rephrase #3 to suggest a fix" — never assume it means modify the code. If the developer insists on fixing code, tell them to re-run `/review-changes` without the PR argument while checked out on the PR branch. 4. **Independent review (parallel):** Both engineers begin reviewing in parallel. Each: - Reads the full diff and changed files - Covers every focus area - Sends findings to `team-lead` - Marks their task completed 5. **Cross-review & challenge:** Once both reports arrive, `team-lead`: - Sends `engineer-1`'s findings to `engineer-2`, and `engineer-2`'s findings to `engineer-1` - Asks each engineer to **challenge** anything they consider a false positive, wrongly-severed, or duplicated, and to **add** anything the other engineer missed within their own analysis - Collects the challenges and rebuttals 6. **Discussion & consolidation:** `team-lead` facilitates a brief discussion where the engineers converge on: - A single deduplicated list of findings - An agreed severity for each (🔴 Critical / 🟡 Warning / 🔵 Suggestion) - Any items the engineers cannot agree on are kept and marked **Debated**, with both perspectives recorded 7. **Team lead reports to the developer:** The team lead presents the consolidated report: ``` ## Summary Brief description of what the changes introduce. ## Changes Reviewed - `path/to/file.ts` — Description of changes - `path/to/other.ts` — Description of changes ## Findings ### 🔴 Critical - **#1** — `file.ts:42` — Title and explanation - **Suggested fix:** … ### 🟡 Warnings - **#2** — `other.ts:15` — Title and explanation - **Suggested fix:** … ### 🔵 Suggestions - **#3** — `file.ts:78` — Title and explanation - **Suggested fix:** … ### ⚖️ Debated *(only if engineers disagreed)* - **#4** — `file.ts:120` — Description - `engineer-1`: … - `engineer-2`: … ## Ticket Compliance *(PR mode, if a Jira ticket was found)* - ✅ Requirement A — addressed in `file.ts` - ⚠️ Requirement B — partially addressed, missing edge case handling - ❌ Requirement C — not addressed in this PR ## Overall Assessment Summary and recommendation. ``` Findings are numbered sequentially across all severity categories so the developer can reference them by number. 8. **Handle the findings:** **a) PR mode — propose posting comments (read-only):** The team lead first presents the consolidated findings as a **comments plan** and **iteratively refines the plan with the developer** before posting anything. Throughout this whole step, no codebase changes are ever made — every developer instruction targets the plan. **Refine loop:** 1. Show the current comments plan (numbered findings, severities, bodies). 2. Ask the developer (via `AskUserQuestion`) what they want to do. Offer at minimum: - `Post all` — post every finding as-is. - `Post a subset` — developer specifies a list of finding numbers (e.g., `1, 3, 5`). - `Edit the plan` — developer specifies changes (drop, rephrase, change severity, merge, split, add a new finding). Apply the edits, then re-show the updated plan and ask again. - `Post nothing` — skip posting and proceed to cleanup. 3. If the developer says something that sounds like a code-fix request (e.g., "fix #3"), do **not** modify the codebase. Ask whether they mean "drop #3 because I'll fix it myself" or "rephrase #3 to suggest a concrete fix", and apply the chosen plan update. If they truly want to apply fixes, tell them: "PR mode is read-only — re-run `/review-changes` without the PR argument while checked out on the PR branch to apply fixes." 4. Loop until the developer chooses `Post all`, `Post a subset`, or `Post nothing`. Once the plan is finalised, post the selected findings to GitHub as follows: - Determine the authenticated user: ```bash gh api user --jq .login ``` - Check for an existing pending review by the user: ```bash gh api graphql -f query=' query($owner: String!, $repo: String!, $pr: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr) { reviews(states: PENDING, first: 10) { nodes { id author { login } } } } } } ' -f owner='{owner}' -f repo='{repo}' -F pr={number} ``` - If a pending review exists for the authenticated user, reuse its `id`. - Otherwise create a new pending review: ```bash gh api graphql -f query=' mutation($prId: ID!) { addPullRequestReview(input: {pullRequestId: $prId}) { pullRequestReview { id } } } ' -f prId='{pullRequest_node_id}' ``` Obtain the PR node ID via: ```bash gh api graphql -f query=' query($owner: String!, $repo: String!, $pr: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr) { id } } } ' -f owner='{owner}' -f repo='{repo}' -F pr={number} ``` - Determine the latest commit SHA: ```bash gh pr view --repo --json commits --jq '.commits[-1].oid' ``` - For each selected finding, post a review thread with a substantive body. **Every comment body MUST start with `## From AI agent` and contain a meaningful explanation — never just a file/line reference.** Format: ``` ## From AI agent **[]** <explanation: what is wrong, why it matters, what the expected behaviour should be> **Suggested fix:** <concrete suggestion> **Confidence:** <High|Medium|Debated> ``` ```bash gh api graphql -f query=' mutation($reviewId: ID!, $body: String!, $path: String!, $line: Int!, $side: DiffSide!) { addPullRequestReviewThread(input: { pullRequestReviewId: $reviewId, body: $body, path: $path, line: $line, side: RIGHT }) { thread { id } } } ' -f reviewId='{review_id}' -f body='{comment_body}' -f path='{file_path}' -F line={line_number} ``` - For range findings, use the last line. - For findings without a specific line, fall back to a top-level review body comment. - Never post an empty body — if you cannot write a meaningful explanation for a finding, skip it and warn the developer. - **Reply to every unresolved thread** collected in step 2d using the engineers' assessments, even when the thread is not duplicated by a new finding. Each reply begins with `## From AI agent` and contains one of: agreement + current status, disagreement + rationale, or partial agreement. ```bash gh api graphql -f query=' mutation($threadId: ID!, $body: String!) { addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { comment { id } } } ' -f threadId='{thread_id}' -f body='{reply_body}' ``` - **Never submit the review programmatically.** Tell the developer: > Comments have been added to your pending review. Please review them and submit the review from the GitHub UI when you're ready. - Print a posting summary: ``` ## Review Comments Posted **PR:** #<number> — <title> **Review:** <"New pending review created" | "Added to your existing pending review"> **New comments posted:** <count> **Replies to existing threads:** <count> ``` **b) Local mode — ask which to address:** The team lead asks the developer (via `AskUserQuestion`) which findings to address now. Accept `all`, `none`, or a list of finding numbers. Continue to step 9 with the selected findings (skip step 9 entirely if the developer selects `none`). 9. **Address the selected findings (LOCAL MODE ONLY, auto mode):** **Skip this step entirely in PR mode**, regardless of what the developer says. PR mode never modifies the codebase — that boundary is enforced at the top of this skill and in every team brief. In local mode, when findings are selected for fixing, the team lead dispatches `engineer-1` and `engineer-2` to implement them in **auto mode**: - Split the selected findings between the two engineers (roughly by file or focus area) so they work in parallel without stepping on each other. - Each engineer implements its assigned fixes directly in the working tree, validating with the relevant tests/lints when sensible. - When both engineers report completion, the team lead summarises what changed and lists any follow-up work the developer should review manually. - **Do not commit.** The developer commits when satisfied. 10. **Cleanup:** - Verify no temporary worktree is still attached: `git worktree list`. Anything created by the team for this review must already have been removed before step 7; if one is somehow still present at this point, remove it now with `git worktree remove --force <path>` and tell the developer. - The team lead sends `shutdown_request` to `engineer-1` and `engineer-2`. - After both confirm, call `TeamDelete`. - Present the final summary to the developer. --- ## Edge Cases - **PR mode is read-only — under no circumstance modify the codebase in PR mode.** All developer input in PR mode targets the comments plan, never the working tree. Step 9 is local-mode only. - **Never check out a branch in the developer's working directory.** Use a temporary worktree if files at another ref are needed, and remove it before presenting the report. If worktree cleanup fails, hold the report and surface the failure. - If the PR diff is empty (PR mode) or there are no commits ahead of base (local mode), report "No changes to review" and **STOP**. - If `gh` is not authenticated (PR mode), display setup instructions and **STOP**. - If the exclusion list is very large (>30 items), summarise it for the engineers by grouping related items. - If the engineers find no new issues beyond the exclusion list, report: "No new issues found beyond the <N> already-raised items in existing reviews." - If the PR URL points to GitHub Enterprise, pass the full URL to `gh` commands; they handle enterprise hosts. - If a GraphQL mutation fails when posting a comment (e.g., the diff has moved), skip that comment, log a warning, and continue with the rest. - If all comment postings fail, inform the developer and suggest they post manually based on the report.