diff --git a/plugins/corbits-skills/skills/review/SKILL.md b/plugins/corbits-skills/skills/review/SKILL.md index d64b551b2..cdc58f97e 100644 --- a/plugins/corbits-skills/skills/review/SKILL.md +++ b/plugins/corbits-skills/skills/review/SKILL.md @@ -1,346 +1,78 @@ --- name: review description: Review a branch or pull request diff and report findings. +argument-hint: "[branch | PR number | PR URL]" --- -# Code Review +# Review -Use this skill when performing code reviews or pull request reviews. +Classify the target, resolve the base, dispatch only the lenses it warrants, verify, and report. Findings only: do not implement fixes. Requires a git repo. Run `git rev-parse --show-toplevel`; if it fails, stop and tell the operator to `git init` first. -## Classify, Then Recommend a Selected Fleet +## 1. Classify -First classify the review target, then recommend only the fleet the -target warrants. Do not fan out a default wide fleet. This skill does -not route the fleet — the primary (the dispatch director) dispatches; -the classification below tells it which lenses the target warrants. +- **Diff:** a branch, PR, or path. The common case. +- **Topic:** an architecture or approach question. +- **Interview:** only when the review object is genuinely missing. Never as ritual. -Classify the review target as one of: +The primary dispatches with `spawn_agent`, one target per wave: `reviewer` always, `planner` when architecture, API, or approach is at stake, `designer` or `warden` only when the touched files warrant that lens (UI surface, security-sensitive code). Do not fan out a default wide fleet. -- Diff: a branch, PR, path, or post-PR work. -- Topic: an architecture or approach question. -- Interview: only when the review object or base is genuinely missing. - Never run interview as ritual. +## 2. Resolve the target and base -Then the primary dispatches the warranted lenses with `spawn_agent`, one target per wave: +For a PR (number, URL, or branch), review from a worktree, never the local checkout: -- Reviewer always. -- Planner when architecture, API, or approach is at stake. -- Designer or Warden only when the touched files warrant that lens - (UI surface, or security-sensitive code). +1. `gh pr view --json headRefName,baseRefName,number,url`. Stop and ask if it does not exist. +2. `git fetch origin `, then create a worktree with `use_skill("git-worktrees")`. +3. Use the PR's base as the diff base. -When the target is a PR (number, URL, or branch), review it from a worktree, never from the local checkout: - -1. Resolve the PR and its head branch with `gh pr view --json headRefName,baseRefName,number,url`. Stop and ask if it does not exist. -2. `git fetch origin `, then create a worktree for it with `use_skill("git-worktrees")`. -3. Review there. Use the PR's base as the diff base. -4. Post the finished review on the PR (see the GitHub posting rules), then remove the worktree. +For a local branch, take the base from its open PR (`gh pr view --json baseRefName`), else from upstream tracking, else the merge base with `origin/`. If none is clear, ask; a wrong base pulls in out-of-scope changes or hides in-scope ones. If any step fails, stop and ask instead of working around it. -## Post the Review on GitHub - -When the branch has an open GitHub PR, post the finished review on it. A review that only lives in chat is not done. Posting delivers decided findings; do not reopen the analysis while writing the body. - -Post when the operator asked to review a PR, when `/implement`'s whole-branch gate is running for a branch with an open PR, or when a PR exists and the review's purpose is a record on it. Do not post a private local read. - -Use a real review for the verdict, `--approve`, `--comment`, or `--request-changes`: - -```bash -gh pr review --comment --body-file -``` - -Body shape, short and in present tense: - -```markdown -## Review · - - - -### Findings - -- `path/to/file.ts:12` — concrete problem and why it matters - -### Notes - -- Only load-bearing context. -``` - -A clean review says "No findings." after the one-line description. Every finding has a `path:line` and a concrete failure mode; severity is the review action, not adjectives. No throat-clearing, no praise to soften a finding, no journey narration, no emoji, no "nit:" taste. When more than one lens produced a distinct judgment (reviewer, planner waiver, warden), each posts its own labelled review, and only the lens that owns the merge verdict may approve. Paste the review URL back to the operator. Posting does not change Linear state. - -## Base Branch Determination - -Before reviewing, you must determine the correct base branch. Use these methods -in order of reliability: - -### Method 1: Check for Associated PR/MR - -If the branch has an open pull request or merge request, use the PR's target: - -```bash -# GitHub -gh pr view --json baseRefName --jq '.baseRefName' 2>/dev/null - -# GitLab -glab mr view --output json 2>/dev/null | jq -r '.target_branch' -``` - -### Method 2: Check Upstream Tracking - -If the branch tracks a remote branch that itself branched from somewhere: - -```bash -git config --get branch..merge -``` - -### Method 3: Find Merge Base with Default Branches - -Attempt to find a common ancestor with likely default branches: - -```bash -# Check if main or master exists and find merge-base -for base in main master develop; do - if git rev-parse --verify "origin/$base" >/dev/null 2>&1; then - merge_base=$(git merge-base "origin/$base" HEAD) - echo "$base: $merge_base" - fi -done -``` - -This helps identify which branch HEAD diverged from, but does not guarantee -the intent of the branch author. - -### When Base Cannot Be Determined - -If none of the above methods provide a clear answer, do not assume. Ask the user: - -> I cannot determine which branch this was based off. What branch should I use -> as the base for comparison? Common options include `main`, `master`, or -> `develop`. - -Proceeding with an incorrect base will cause the review to include out-of-scope -changes or miss in-scope changes. - -## Scope Determination - -Focus only on commits that are contained within the branch being reviewed. -Use `git diff ...HEAD` to determine what is in scope. - -```bash -# List commits on the branch -git log --oneline ..HEAD - -# Get the full diff for review -git diff ...HEAD - -# Get diff for a specific file -git diff ...HEAD -- -``` - -## Pre-existing Code - -If a bug, convention violation, or inconsistent naming existed in code before -the branch, do not treat it as a problem in the review. This includes cases -where refactored code retains a pre-existing type name, variable name, or -pattern that does not match current conventions. - -Only evaluate the changes introduced by the branch. - -## Convention Compliance +## 3. Scope -All new logic, patterns, and naming introduced by the branch should follow -the project's established conventions. Pre-existing code that appears in -the diff due to refactoring is exempt from this requirement. +Review only what `git diff ...HEAD` shows. Pre-existing bugs, names, and patterns in touched code are not findings. New logic, names, and patterns follow project conventions (`AGENTS.md`, and the `typescript` skill for TypeScript). Ask for tests only for your own logic, integration points, error paths, and edge cases, never for what a library already guarantees. -If reviewing TypeScript code, consider loading the `typescript-conventions` -skill for detailed guidance on type patterns, naming, and idioms. +When delegating, give the sub-agent `git diff ...HEAD -- `, not whole files. If it needs a full file for context, name the modified line ranges and say only those are in scope. -## Delegating to Sub-agents +## 4. Reviewer-of-record checks -When delegating file review to sub-agents, provide the output of -`git diff ...HEAD -- ` rather than the full file contents. +The agent whose verdict ships runs these itself and reads the raw output. They are not delegable; a delegate collapses `Bin 0 -> 8181 bytes` on a `.ts` file into noise. -Sub-agents that receive full files cannot distinguish branch changes from -pre-existing code and will flag out-of-scope issues. +- `git diff ...HEAD --stat`: look for `Bin` markers on files that should be text, and files outside the stated scope. +- `git log --oneline ..HEAD`: commits must match the ticket's scope. +- Commit messages: each subject and body follows `CONTRIBUTING.md`, and each diff matches its message. Scan `git log ..HEAD --format='%s'` and `--format='%b'` (pipe through `awk 'length > 72'` for length). Flag a missing or unknown type, ticket ids, status tags, file paths in subjects, trailing punctuation, vague subjects, references to other commits or to review conversation. -If full files must be provided for context, explicitly instruct the sub-agent -which line ranges were modified by the branch and that only those ranges are -in scope. +Deeper work (file-by-file behavior, architecture, coherence) can go to sub-agents. -## Test Coverage Philosophy +## 5. Findings -Do not request additional test coverage for functionality provided by external -libraries. Focus test coverage requests on: +Raise what helps the author. Flag real defects and architecture that would constrain future work. Skip hypothetical problems in cases the code need not handle, stylistic nits, and vague remarks like "could be cleaner". -- Business logic and domain-specific validation -- Integration points between components -- Error handling paths and edge cases -- Custom algorithms and data transformations +- Every finding has a `path:line` and a concrete failure mode. Severity is the review action, not adjectives. +- Describe the branch as it stands now, in present tense, from `git diff ...HEAD`, not the journey or the PR description. +- Write to a colleague: propose rather than command, say why, describe consequences instead of "wrong" or "broken", and suggest the one-liner when the fix is small. +- **Cite the check.** Every affirmative claim ("tests pass", "no regressions") names the command or file range that proved it. Otherwise run the check, strike the claim, or narrow it to what you examined ("read `lock.go:42-68`, no acquire-while-holding cycle there"). -Trust well-maintained libraries to do their job. If a library's behavior needs -testing, that suggests reconsidering whether to use that library. +## 6. Report and post -## Signal Over Noise - -A review should help the author, not burden them. Do not raise issues that are -not important to the code being reviewed. Specifically: - -- Do not flag hypothetical problems in unlikely scenarios that the code does - not need to handle today or in the near future. -- Do flag architectural choices that would limit or constrain future - implementations, even if they work fine today. -- Do not leave minor stylistic nits that have no impact on correctness, - readability, or maintainability. -- Do not make vague observations like "this could be cleaner" without a - concrete reason it matters. - -If something should be fixed, say so directly. If it's not worth fixing, don't -bring it up. - -## Comment Tone - -Review comments are posted under your name to a human author. Write them -as you would speak to a colleague: - -- Frame suggestions as questions or proposals, not commands. - "Could we extract this?" not "Extract this." -- Explain the *why* -- "so that X" or "because Y" -- not just the *what*. -- Avoid words like "wrong", "broken", "bad". Describe the consequence - instead: "this will produce incorrect values when..." not "this is wrong." -- If the fix is small, suggest it concretely. A one-liner suggestion is - more helpful than a paragraph explaining what to change. -- Do not soften feedback to the point of obscuring it. The author should - understand whether something needs to change or is merely a thought. - -## Describe the Branch As It Stands - -Review comments and PR descriptions must describe the branch's *current* diff against the base, not the journey that produced it. The merged result is what ships; intermediate states, earlier review rounds, and pre-iteration code are not part of the artifact. - -Re-derive the description from `git diff ...HEAD` every time. Do not trust the existing PR description — it may have been written before the branch was iterated on, and reviewers reading it now should see what is true now. - -Bad: "I refactored the retry logic to use exponential backoff and removed the old fixed-delay path." -Good: "The HTTP client retries transient failures with exponential backoff." - -Bad (review comment): "This used to call `foo()` but now calls `bar()` — did you mean to..." -Good (review comment): "`bar()` is being called here without checking its return value." - -Past-tense framing rots: as the branch evolves through review, descriptions of "what was changed" stop matching the diff. Present-tense framing of "what the code now does" stays correct as long as the diff is correct. - -## Cite the Check - -A review report may include affirmative verification claims — "tests pass," "messages are clean," "no regressions," "convention compliance verified." When a review says something is *verified*, the reader takes that as a checked fact. If the check was never run, the report is dishonest. The dishonesty does not surface until a human reviewer finds the issue the agent claimed did not exist. - -**For every affirmative claim in your review, you must be able to cite the specific check that proved it.** A check is a concrete artifact: a command whose output you read, a tool invocation whose results you inspected, a file/line range you examined for a specific pattern. "I considered it" and "it looked fine to me" are not checks. - -When you cannot cite a check, do one of: - -1. **Run the check** and cite it. -2. **Strike the claim** from the report. -3. **Narrow the claim to what you actually examined.** "Did not observe race conditions in `lock.go:42-68`" is honest; "no race conditions in the new locking code" is not. - -Some properties — subtle concurrency bugs, performance pathologies, security gaps — cannot be fully verified by a single check. Honest narrowing is better than a dishonest absolute: say what you looked at and what you looked for, and do not claim absence beyond that boundary. - -**Bad → Good:** - -- "All commit messages are clean." → cite the specific audit command (see *Commit-Message Style Audit*), or strike. -- "Tests pass." → "`npm test` exited 0," or strike. -- "Convention compliance verified." → "diffed naming and error-handling shape in the new handlers against `src/api/user.ts` and `src/api/billing.ts`; same `Result` return pattern, same `assert`-style guards," or strike. -- "No race conditions." → "read `lock.go:42-68`, traced lock acquisition order; no acquire-while-holding cycles in those lines. Did not analyze interactions with `pool.go` or callers outside the diff." - -## Reviewer-of-Record Checks - -"Cite the Check" requires that affirmative claims have backing. This rule narrows it: for the checks below, the backing must be the reviewer-of-record's own eyes on raw output. The reviewer-of-record is the agent whose verdict ships — when a workflow delegates the deeper read to a subagent, the orchestrator remains the reviewer-of-record and these checks stay with them. - -Their value is in catching unknown-unknowns. A delegate asked for "a punch list of findings" returns things that fit the punch-list shape; `Bin 0 -> 8181 bytes` on a `.ts` file does not look like a finding, it looks like stat noise, and gets collapsed away. Only direct inspection preserves the signal. - -**Reviewer-of-record must run, in-session, and read the raw output:** - -- `git diff ...HEAD --stat` — scan for `Bin` markers on any file you did not expect to be binary (source code, markdown, config) and for files outside the branch's stated scope. -- `git log --oneline ..HEAD` — confirm the commits on the branch match the issue's scope; unexpected or off-topic commits are stop conditions. -- The subject and body audits enumerated under *Commit-Message Style Audit*. That section is the canonical command catalog and pattern list; this section's contribution is the delegation rule — those audits are reviewer-of-record, not subagent work. - -Delegating the deeper read (file-by-file behavioral review, architectural analysis, commit-message coherence) to a subagent is fine and often valuable for context isolation. Delegating the checks above is not — their value is the raw output landing in front of the reviewer-of-record's eyes. - -## Commit-Message Coherence - -Each commit's message is a claim about what the commit contains. Verify that -claim. For every commit on the branch, read the message, read the diff -(`git show `), and confirm that the two match. - -If a commit contains material changes that are unrelated to its message — -files touched that have nothing to do with the stated purpose, unmentioned -behavioral changes, or new functionality that the message does not describe — -flag it. Undescribed changes hiding inside a commit undermine the reviewability -and auditability of the history. - -This includes whitespace or formatting changes to code that is not otherwise -being modified. Drive-by cleanup belongs in its own commit — its presence in -an unrelated commit is itself a problem. - -Do not require that every modified line be individually narrated in the message. -The message is a summary. But if someone reading only the message would be -surprised by what the diff actually contains, that is a problem worth raising. - -## Commit-Message Style Audit - -Coherence (above) checks that each message accurately describes its diff. Style audit checks that each message conforms to the project's commit-message rules (see the `style` skill, which is the canonical source for the prefix-family list and other rules). The two are independent; both have to be run. - -Each check below is reviewer-of-record territory (see *Reviewer-of-Record Checks*) — run them in-session and read the raw output yourself. Do not delegate them; a subagent asked to "verify style compliance" will return a generic "looks fine" read with no audit trail. - -**Subject-line audits.** Most checks scan the output of: +Summarize with the findings. When the target has an open GitHub PR, post the finished review on it; a review that only lives in chat is not done. Post when the operator asked for a PR review, when `/implement`'s whole-branch gate runs on a branch with an open PR, or when a record on the PR is the purpose. Do not post a private local read, and do not reopen the analysis while writing the body. ```bash -git log ..HEAD --format='%s' -``` - -Scan for: - -- **Subject-form violations.** Project convention is Conventional Commits: `(): ` with type from `feat`, `fix`, `perf`, `refactor`, `test`, `docs`, `build`, `ci`, `chore`, `style`. Flag a subject with no type (`Anthropic adapter: handle 429s`, `Add retry logic`), an unrecognized type, a ticket ID (`INTR-79:`), or a status tag (`WIP:`, `[urgent]`). Do **not** flag `feat:`/`fix:`-style prefixes themselves — those are the convention. -- **Filename or path references.** Tokens that look like file paths or extensions (`server.ts`, `INFERENCE.md`, `src/foo/bar.py`). The diff lists what changed; subjects describe the change, not the file. -- **Trailing punctuation.** Subjects ending with `.`, `!`, or `?`. -- **Vague subjects.** "Update code," "Fix bug," "Misc changes," "Address review." - -For the length limit, use a length-aware filter so the check is not eyeball-counting: - -```bash -git log ..HEAD --format='%s' | awk 'length > 72' +gh pr review --comment --body-file ``` -Empty output is clean. Any line returned is an over-72-character subject violation. +Use `--approve` or `--request-changes` for the verdict. Shape: -**Body audits.** Most checks scan the output of: - -```bash -git log ..HEAD --format='%b' -``` - -Scan for: +```markdown +## Review · -- **External tracker references.** Linear/Jira/GitHub IDs (`INTR-79`, `JIRA-1234`, `#456`, `Closes XXX-99`). The commit must explain itself. -- **References to other commits in the series.** "as discussed in the previous commit," "the next commit wires this up," "see also abc1234." A commit describes its own state, not the branch's trajectory. -- **References to PR review comments or session conversation.** Ephemeral context the future reader cannot access. + -For body-line length, use the same length-aware filter: +### Findings -```bash -git log ..HEAD --format='%b' | awk 'length > 72' +- `path/to/file.ts:12` — concrete problem and why it matters ``` -Empty output is clean. Any line returned is an over-72-character body line. - -Affirmative claims about these audits must cite the specific command whose output proved them — see **Cite the Check**. - -## Review Checklist - -Items marked *(reviewer-of-record)* must be run by the agent whose verdict ships — see *Reviewer-of-Record Checks*. Do not delegate them to a subagent. Unmarked items are delegable. - -1. Determine the base branch using the methods in "Base Branch Determination" -2. Run `git log --oneline ..HEAD` to understand the scope *(reviewer-of-record)* -3. Run `git diff ...HEAD --stat` to see which files changed *(reviewer-of-record)* -4. Review each changed file, focusing only on lines modified by the branch -5. For every commit on the branch, verify: - - Diff matches the commit message (see "Commit-Message Coherence") - - Subject and body pass the style audit (see "Commit-Message Style Audit") *(reviewer-of-record)* -6. Check that new code follows project conventions -7. Summarize findings with specific file:line references; cite the check behind any affirmative claim (see "Cite the Check") +A clean review says "No findings." after the one line. No throat-clearing, praise to soften a finding, emoji, or "nit:" taste. When more than one lens produced a distinct judgment (reviewer, planner waiver, warden), each posts its own labelled review, and only the lens that owns the merge verdict may approve. Paste the review URL back to the operator. Posting does not change Linear state. Remove the PR worktree afterward.