review

Files (1)

• SKILL.md

SKILL.md (first lines)

## Read-only — hard constraint

This skill **analyzes and reports**; it never mutates the repository, the PR/MR, or anything external. After you emit the merge recommendation, **STOP** -- unless the **Merge offer** conditions (below) are met.

Never — not for a real bug, not for a blocking defect, not even when there is no human reviewer and "someone has to fix it":
- edit, create, or delete files (no `write`/`edit`-style mutations);
- `git add` / `commit` / `stash` / `reset`, `git checkout` to discard changes, or `git push`;
- `gh pr comment` / `review` / `edit` / `merge` / `create`, or post or edit any PR/MR body, comment, or description;

**Exception:** `gh pr merge` is permitted after a **MERGE** verdict on a PR review in-which the findings are docs/test-related only, but **only** after the user explicitly confirms via `ask_question` (see **Merge offer** below).
- run any other write- or network-mutating shell command.

The only shell permitted is **read-only inspection**: `git diff` / `git show` / `gh pr diff` / `gh repo view`, `grep` / `rg`, and file reads — plus dispatching the review sub-agents. Resolving findings, fixing bugs, resolving merge conflicts, and "making the branch mergeable" are explicitly **out of scope**: a fixable defect is a finding to report (`file:line` + a one-line fix in the `suggestion` field), never a license to act.

## Sub-agent contract
/contract

**Skip for:** lock files (`package-lock.json`, `go.sum`, `yarn.lock`), auto-generated files (`*.generated.*`), pure-docs diffs, vendored deps.

**Resolve target → diff (inline).** The review target argument is: `$ARGUMENT` (empty = review working-tree/HEAD changes). Map this argument to a diff source, then capture the diff text plus a one-line target descriptor for the triage header. Also capture the **reviewed ref** (branch HEAD SHA or equivalent) — this is required for citation verification later:

- `--staged` → `git diff --staged`; reviewed ref = `git write-tree` (snapshots the staged index to a throwaway tree so citations resolve against the staged content under review, not HEAD)
- `--head` or no arg → `git diff HEAD`; reviewed ref = `git stash create` (snapshots worktree + index to a throwaway commit so citations resolve against the content under review; empty output = no local changes → fall back to `git rev-parse HEAD`)
- arg matches `^https?://.*/pull/\d+` (GitHub/GitLab PR URL) → `gh pr diff <url>` (or `glab mr diff`); reviewed ref = head SHA from `gh pr view <url> --json headRefOid -q .headRefOid`; record PR title + base/head refs
- arg matches `^#?\d+$` (bare PR number, optionally `#`-prefixed) → resolve in current repo with `gh pr diff <n>`; reviewed ref = head SHA from `gh pr view <n> --json headRefOid -q .headRefOid`; if `gh` is unavailable or repo has no PR matching, abort with `Asking` (one question: which repo/PR)
- arg matches `^[0-9a-f]{7,40}$` (commit SHA) → `git show <sha>`; reviewed ref = `<sha>`
- arg matches a known ref (`git rev-parse --verify <arg>` succeeds) → `git diff <merge-base>...<arg>` against the repo's default branch; reviewed ref = `git rev-parse <arg>`
- arg is a path or `*.diff`/`*.patch` file → read file contents as the diff; reviewed ref = `unknown (patch file — no live ref available)`
- otherwise → abort with `Asking` naming the ambiguous arg

**Capture stated intent (inline).** A reviewer that sees *what changed* but not *what it was meant to accomplish* cannot judge whether the change does its job — it silently redefines "the spec" as whatever the diff or the repo's global constraints imply, and rubber-stamps. Capture the change's stated intent into a `stated-intent` field passed to every agent:
- `--brief "<text>"` / `--spec <path>` supplied → use that text / file contents verbatim (highest priority).
- PR URL or number → `gh pr view <ref> --json title,body -q '.title + "\n\n" + .body'` (or `glab mr view`); title + description are the intent.
- commit SHA → the full commit message: `git show -s --format=%B <sha>`.
- known ref / branch → the branch's PR body if one exists (`gh pr view <branch> --json body -q .body`), else the commit subjects: `git log --format=%s <merge-base>..<ref>`.
- `--staged` / `--head` / working-tree / patch-file with no `--brief` → set `stated-intent = "(none supplied)"`.

Never fabricate intent. When none is available the value is the literal `(none supplied)`; agents disclose its absence rather than guess.

**Capture prior reviewer feedback (inline, PR targets only).** When the review target is a PR URL or number, fetch existing reviewer feedback from all three GitHub comment stores before dispatching Wave 1:

1. Inline review comments (anchored to diff lines): `gh api "repos/{owner}/{repo}/pulls/<n>/comments?per_page=100&sort=created&direction=desc"`.
2. Review summary bodies (top-level body per review submission): `gh pr view <n> -R {owner}/{repo} --json reviews -q '.reviews[] | {author: .author.login, state: .state, body: .body, submittedAt: .submittedAt}'`.
3. Conversation comments (issue-level PR comments): `gh pr view <n> -R {owner}/{repo} --json comments -q '.comments[] | {author: .author.login, body: .body, createdAt: .createdAt}'`.

Extract `{owner}/{repo}` from the PR URL argument directly, or for bare-number inputs from `gh pr view <n> --json url -q .url`. Pass `-R {owner}/{repo}` on every `gh pr view` call above so cross-repo PR URLs resolve in the correct repository.

Normalize each source's timestamp field (`created_at` for inline, `submittedAt` for review summaries, `createdAt` for conversation) to a single `timestamp` ISO string during merge. Filter: drop bot/automation comments (author login contains `[bot]` or body is empty/whitespace). Merge all three stores into a single array, sort by timestamp descending, then cap to the **20 most recent** comments, truncated to a combined **4,096 tokens** to prevent context-window bloat on busy PRs. Identify afk's own prior review comments by the `<!-- agent-afk-review -->` marker.

Bundle surviving comments as a **`prior-reviewer-feedback`** block: `[{ source: "inline"|"review"|"conversation", author, body, timestamp, path?, line? }]`. When the PR has no prior comments, set `prior-reviewer-feedback: none`.

For non-PR targets (`--staged`, `--head`, working-tree, patch-file, bare commit SHA, branch with no open PR), skip this step and set `prior-reviewer-feedback: not available — non-PR target`.

**Pre-fetch file contents at reviewed ref (inline).** After capturing `reviewed_ref` and the diff, and before dispatching any Wave 1 agent, the orchestrator (which has Bash) pre-reads every changed file at the reviewed ref and bundles the results for injection into sub-agent prompts. This is required because Wave 1 and Wave 1.5 agents are `research-agent` instances with no shell — they cannot run `git show` themselves.

1. Extract the list of changed files from the diff: `git diff --name-only <base>..<reviewed-ref>` (or parse `--- a/<file>` / `+++ b/<file>` headers from a patch-file diff).
2. For each file, run `git show <reviewed-ref>:<file>` and capture the full output. If the command fails (file does not exist at that ref — e.g. the file was added and the ref predates it, or the ref is `unknown` for a patch-file input), skip that file and note it as `unavailable at ref`.
3. Bundle as a **`prefetched-files`** block: `[{ file, content, status: "ok"|"unavailable" }]`.
4. Include this block verbatim in every Wave 1 and Wave 1.5 agent prompt alongside the diff. Agents verify `file-state` citations against the injected content — they do **not** call `read_file` for ref-anchored verification (the working tree may be on a different branch).

When `reviewed_ref` is `unknown` (patch-file input), skip pre-fetch entirely and note `prefetched-files: none — patch-file input`. Agents fall back to `diff-context` citations only and tag any `file-state` citation `[UNVERIFIED: no live ref]`.

**Triage (inline).** From the resolved diff extract: change type (hotfix | feature | refactor | dep-bump | new-service), files changed, total lines changed, summary. Classify regime: `light` if ≤300 lines or change type is hotfix/dep-bump; `full` otherwise.

**Concurrency floor — declared, conditional, and enforced.** *Through synthesis*, a full-regime review peaks at **2 concurrent sub-agent sessions** (Wave 1's two dimension agents) and dispatches **3 in total** (Wave 1 ×2, then Wave 2 ×1, sequential). Wave 1.5 runs inline in the orchestrator and dispatches nothing. **No wave nests a child**: the sub-agents are shell-less by design, and nothing in this skill requires them to run a command, so none of them needs to nest a `git-investigator` to comply. If you add a requirement here that needs a shell, you have silently doubled this floor — put that requirement in Wave 1.5 instead.

**The post-synthesis tail is the conditional half of that budget.** A review that surfaces a `critical`/`high` finding (or one whose `blocking` value departs from the default table — see **Post-synthesis** below) invokes `/shadow-verify`, which dispatches one verifier per claim in parallel, so the whole-run budget is **peak 2–3 concurrent, 4–6 total (1–3 verifiers)**, and it lands on exactly the high-stakes reviews most likely to hit a rate ceiling. Bound it: **at most 3 claims in a single round, no repeat rounds**, and hand the verifiers Wave 1.5's manifest so each re-derives the *claim* instead of re-locating evidence Wave 1.5 already pinned at the ref. Wave 1.5 verifies that a citation is real; shadow-verify re-derives whether the inference drawn from it holds — never substitute one for the other.

**Wave 1 — Full review (regime=full, 2 parallel agents, `subagent_type: "research-agent"`).** Dispatch:
- **security · api-compat** — contracts, auth, injection, breaking changes, secret exposure. Set `id_prefix: "security & api-compat"` on this Agent dispatch.
- **correctness · spec-compliance · test-coverage · perf-observability** — logic bugs, regressions, whether the change satisfies its **stated intent** (unmet requirement or unrequested scope creep), missing tests, hot-path perf, logging gaps. Set `id_prefix: "correctness & coverage"` on this Agent dispatch.

**Budget: set `max_tool_use_iterations: 80` on both Wave 1 dispatches** (or `max_tool_rounds: 80` per node if you dispatch them as `compose` nodes). The `research-agent` default of 50 is too tight for this wave. Each agent covers several dimensions of a diff over 300 lines, and measured Wave 1 workers on the default exhausted their budget 25–36% of the time, which ends in a tools-stripped wind-down built on partial evidence. Raising the round ceiling does not change the concurrency floor above (still 2 concurrent, 3 total).

Each agent receives: full diff + file tree + triage header + **reviewed ref (SHA)** + the **stated intent** (what the change is meant to accomplish, or `(none supplied)`), the **`prior-reviewer-feedback`** block (when available), the severity rubric, **the `blocking` default table plus its overrides and assignment-order invariant**, and the finding schema. The blocking rules are not optional context: the finding schema mandates a `blocking` value per finding, so an agent that receives the schema without the table is being told to emit a field whose assignment rules it was never given.

**Citation requirement (enforced per agent).** Wave 1 agents cite from the diff and from the **`prefetched-files` block injected by the orchestrator**. They do **not** call `read_file` for ref-anchored verification (the working tree may be on a different branch) and do **not** run git. Centralized verification runs in Wave 1.5, which checks every `blocking`/`critical`/`high` citation **and every `file-state` citation at any severity** against the same pre-fetched content, then drops fabricated ones. Each agent must:
1. State the reviewed ref it was given in each finding: `ref: <sha>`.
2. Classify every citation as `diff-context` (line visible in the diff hunk) or `file-state` (line in the post-merge file, not visible in the hunk). For `file-state` citations, look up the line in the injected `prefetched-files` content. If the file's status is `unavailable` in the bundle, tag the citation `[UNVERIFIED: file unavailable at ref]` so Wave 1.5 knows it could not be pre-fetched.
3. Never paraphrase or reconstruct a line from memory. If the line is not visible in the diff and not present in the pre-fetched content, cite it as `file-state [UNVERIFIED: file unavailable at ref]` — do not invent the content.

**Invariant — why Wave 1 does not run git.** `research-agent` has no shell (`tools: Read, Grep, Glob, WebFetch, WebSearch, Agent(git-investigator)` — no `Bash`, and that single `Agent(...)` entry is the nesting path this rule closes), so a mandatory `git show` forces it to dispatch a nested `git-investigator` purely to run one command. That doubles the concurrent session count of *every* Wave 1 agent, and is the structural cause of the rate-limit cascade in #726. Ref-anchored verification is therefore performed once, centrally, by a shell-capable actor — never N times by shell-less ones. Do not reintroduce a per-agent re-read here.

Banned words: "ensure", "consider", "may", "could". No `file:line` citation → omit the finding.

**Spec-compliance assessment (mandatory framing for the spec-compliance dimension).** Judge the diff against the `stated-intent` — not against the diff's own apparent goals, and not against the repo's global constraints:
- `stated-intent` present → flag (a) **unmet intent**: a requirement named in the intent with no implementing change, and (b) **scope creep**: a substantive behavior change the intent does not call for. Cite the unmet clause for gaps; cite `file:line` for creep.
- `stated-intent` is `(none supplied)` → do **not** assess spec-compliance and do **not** substitute the global constraints for the spec. Emit exactly one line: `unverified — spec-compliance not assessed: no stated intent supplied (pass --brief/--spec, or review a PR/commit)`. Silently treating the diff or the constraints as "the spec" is the precise failure this rule prevents.

**api-compat reachability pre-check (mandatory before surfacing any breaking-change finding).**
For every symbol flagged as a breaking change, search production source files with the `Grep` tool — which needs no shell, so this check never forces a nested dispatch — for imports or usages of that symbol, excluding `*.test.*`, `*.spec.*`, `__tests__/`, `__mocks__/`, `/test/`, `/tests/`. Decision table:
- Zero production importers → downgrade finding to `nit`, append `[UNVERIFIED: no production importers]`, set confidence `low`.
- One or more production importers → severity stands; include one importer path as evidence.

If the `Grep` tool is unavailable, tag the finding `[UNVERIFIED: reachability not checked]` and downgrade one severity tier.

**Absence-claim grounding (mandatory for any claim that something does not exist).** Before emitting a finding of the form "no test covers X", "no handler validates Y", "no caller invokes Z", "X is not tested": search the production tree with the `Grep` tool — which needs no shell, so this check never forces a nested dispatch — for plausible match strings. Decision table:
- Zero matches → finding stands; cite the search pattern in the evidence field.
- One or more matches → emit `unverified — absence claim refuted by <path>:<line>` instead of the finding.
- Grep tooling unavailable or claim cannot be reduced to a pattern → tag finding `[UNVERIFIED: absence not checked]` and downgrade one severity tier.

This is the agent's first-line self-check; **Wave 1.5 Check B** independently re-verifies any surviving absence claims against the reviewed ref as a backstop.

**Wave 1 — Light review (regime=light, 1 agent, `subagent_type: "research-agent"`).** Set `id_prefix: "light review"` on this Agent dispatch. Single agent covers all dimensions (including spec-compliance). Same `stated-intent` input, `prior-reviewer-feedback` block, rubric, schema, and citation requirement. Through synthesis, the light regime peaks at **1 concurrent sub-agent session** and dispatches **2 in total** (Wave 1 ×1, then Wave 2 ×1, sequential). Its conditional post-synthesis `/shadow-verify` tail dispatches 1–3 verifiers in parallel when qualifying findings surface, making the whole-run budget **peak 1–3 concurrent, 3–5 total (1–3 verifiers)**; the same bound of at most 3 claims in one round with no repeat rounds applies.

**Wave 1.5 — Citation + absence-claim verification (INLINE — run by the orchestrator, dispatches nothing).** Run after Wave 1 returns, before Wave 2 synthesis. The orchestrator already holds exactly the read-only shell this verification needs (`git show` / `git diff` / `gh pr diff` / `grep` / `rg` — see the shell grant above), so running it inline costs **zero** additional sessions and zero nesting. A shell-less sub-agent here would have to nest a `git-investigator` to run the very commands the orchestrator can already run. Two independent checks:

**Check A — Citation verification.** Extracts (a) every `file:line` citation from any `blocking`, `critical`, or `high` finding, **and (b) every citation tagged `file-state` at any severity** — `medium`, `low`, and `nit` included — across all Wave 1 results. Both sets are checked, because Wave 1 never re-reads at the ref independently: an unverified `file-state` citation in a `low` finding is exactly as fabricable as one in a `high` finding, and quoting a line absent from the reviewed change is a defect at every tier. For each citation, looks up the file in the **`prefetched-files` block** (already captured by the orchestrator before Wave 1 ran) and checks whether the quoted evidence snippet actually appears at that line — not in main, not in diff context alone. If the file's status is `unavailable` in the bundle, falls back to `git show <reviewed-ref>:<file>` (the orchestrator has shell). Classifies each citation as:
- `verified` — content matches what is actually at that line on the reviewed ref.
- `diff-only` — line appears in the diff hunk but no longer exists at the reviewed ref HEAD (e.g., deleted block). Finding must be downgraded: the issue may already be resolved.
- `fabricated` — line does not exist at the reviewed ref and was not in the diff hunk; the evidence snippet is unverifiable. Finding is **dropped** from the report.

**Check B — Absence-claim verification.** Extracts every **absence claim** across all Wave 1 results **at any severity** — `medium`, `low`, and `nit` included, for the same reason Check A checks every `file-state` citation: Wave 1's absence grounding is a self-check by an agent that never re-read at the ref, so an unverified absence claim in a `low` finding is exactly as fabricable as one in a `high` finding. These are claims of the form "no test covers X", "no handler validates Y", "no caller invokes Z", "X is not tested", "Y has no validation". Citations are not required for absence claims, so Check A cannot catch them; they need their own gate. For each, identify the asserted-absent symbol, test name, or behavior, then run `git grep -n <pattern> <reviewed-ref>` (or `rg --no-heading -n <pattern>` if outside a git context) across the production tree (exclude the same paths as the api-compat reachability check: tests, mocks, fixtures, when the claim is about production code). Classify as:
- `confirmed-absent` — zero matches in the asserted scope; finding stands.
- `false-absent` — one or more matches in the asserted scope; finding is **dropped** (the asserted-absent entity exists at the reviewed ref). Name the matching path(s) in the dropped-findings manifest.
- `grep-unavailable` — tooling missing, symbol ambiguous, or absence claim cannot be reduced to a grep pattern; finding tagged `[UNVERIFIED: absence not checked]` and downgraded one severity tier.

Returns a combined verification manifest: `[{type: citation|absence, claim, status, finding_id, evidence?}]`. Findings classified `fabricated` (citation) or `false-absent` (absence) are excluded from Wave 2 input. `diff-only` citations are passed to Wave 2 with a `⚠ diff-only citation — line absent at the reviewed ref` annotation and auto-downgraded one severity tier. `grep-unavailable` absence claims are passed through with their `[UNVERIFIED]` tag intact.

From griffinwork40/agent-framework, Apache-2.0.

More from griffinwork40/agent-framework