review

name

review

description

Project-aware code review - run pywatershed's mechanical convention checks on the diff, then the built-in code-review skill for correctness, and merge both into one findings-only report written to a .md file in the repo root. Use when the user invokes /review, optionally with a PR number, branch, or effort level (low/medium/high/max), e.g. "/review 407 high". Also supports a blind second pass and fusing two reviews into one comprehensive report.

Project-aware review

Two layers over one diff, merged into one report: (A) mechanical checks of pywatershed's conventions — run as greps/scripts so they are identical from review to review — and (B) the harness's built-in code-review skill for correctness bugs and cleanups.

Ground rules

• Findings only. Never apply fixes as part of a review; fixes are a separate, discussed step afterwards.
• The review assumes the test suite and the pre-commit hooks are run outside the review. Do not run tests or re-run anything pre-commit covers; the assumed items are listed as bullets up-front in step 2.
• Writes: the report file only. The merged report is written to a markdown file (step 2 fixes the path; step 6 writes it). Invoking /review is permission for that one write. Creating, mutating, or destroying anything else — including scratch files, notes, and fixes — requires asking the human AND receiving explicit permission first. This is the hard line; hold it for sub-agents too.
• No machine-specific absolute paths in anything committed — not in the report, not in this file. The check-security pre-commit hook (.github/scripts/check_security.py) fails the commit on them. Refer to tools and environments generically (conda env list, then <env>/bin/<tool>) and keep real paths in chat.
• Running code: read-only verification is expected. Running code that is not redundant with the project's CI and static checkers is fine and preferred over guessing — python -c "import ..." to enumerate types, a quick static check, reproducing a suspected crash to confirm a finding. Do not run the test suite or anything pre-commit covers. A finding verified by execution is worth more than a finding reasoned about; say which one it is in the report.
• Diff acquisition: REST first, read-only git is also allowed. Prefer the public GitHub REST API for PR targets (step 1). Read-only git — diff, log, status against the target — is permitted without asking; invoking /review is that permission, and it extends to the agents the built-in review spawns. Any git command that writes, moves, or discards state is still forbidden.
• Invoking /review is permission for the agents the built-in code-review skill spawns.
• You cannot launch /code-review ultra yourself (cloud-run, billed, human-triggered only) — hand the human the command instead.

Procedure

1. Preflight: tooling and one consolidated poll. Do this before any checks, so nothing is discovered mid-run.

   Diff source, in order of preference:

   • Public GitHub REST API (preferred for a PR target). Needs no auth and no local tooling: https://api.github.com/repos/DOI-USGS/pywatershed/pulls/<N> and .../pulls/<N>/files?per_page=100. Follow redirects (curl -sSL) — the repo was renamed from EC-USGS and the API answers 301 Moved Permanently without them. The files response carries a per-file patch; it is omitted for very large files, which are the generated test_data/ fixtures you want excluded anyway. Filter those and the .nc binaries out before reviewing and say how many lines you dropped.
   • Local git. git diff <base>...HEAD works offline and is the only option for a branch with no PR. Caveat worth stating in the report: a diff against upstream/develop is only as fresh as the last fetch of that remote-tracking ref, so a stale ref silently yields a stale diff. Confirm the local branch tip matches the PR head (read .git/refs/heads/<branch>, compare to the API's head_sha) before treating a branch review as a PR review.
   • gh (treat as unavailable for PR data). Its GraphQL paths fail against this org with Resource protected by organization SAML enforcement, and that authorization is deliberately not being pursued — so gh pr diff and gh pr create are out. Use REST for the diff and the web UI to open a PR. If you need gh for something else, probe PATH first and then the conda environments (conda env list, then <env>/bin/gh); if it is missing, ask rather than improvising. Never block on gh when REST will do.

   Then poll once, in a single question set, for everything that is the human's call:

   • the effort level (see step 5 for why it matters);
   • the report path, whenever it is ambiguous or already taken (see step 2);
   • the target, if the invocation left it unclear.

   Never proceed on a default for an unanswered question. If the human answers some questions and not others — including answering with free text that does not map to any option — treat the rest as unanswered and re-ask those alone. Repeat until every blocking question has a real selection. The defaults documented in this file are for non-interactive runs, not a licence to skip confirmation.

Second pass and fusing

A second review of the same target is worth running when the diff is large or high-stakes. Run it blind: do not read the existing report before or during the pass. A review handed the prior findings anchors on them — it spends its budget confirming and refuting what is already there and inherits the same blind spots. Agreement between two genuinely independent passes is real signal; agreement with a report you just read is not.

• The second report goes to review_<target>_second.md (confirm the name in the step 1 poll, since review_<target>.md is taken).
• Afterwards, offer to fuse. The fused report goes to review_<target>_fused.md and supersedes both inputs, so it must be self-contained — the human should not need to open the other two.

A fused report needs, beyond the merged findings:

• A provenance tag on every finding — found by both passes (highest confidence), or by one only. Note which pass, and record each pass's head SHA and date, since the two may have reviewed different commits.
• Explicit reconciliation of disagreements. Where the passes conflict, say which one is right, why, and what the residual finding is after correction. Do not quietly drop the loser; a reader who saw the first report needs to know it was superseded. Own it plainly when the later pass was the wrong one.
• A coverage note on how the passes divided — if one went wide on regressions in untouched code and the other deep on the class hierarchy, say so, because it tells the human whether a third pass would find more.
• A suggested order of attack at the end, grouping fixes that belong in one change (a bug and the test gap that hid it) rather than listing findings in severity order again.