Marcos
087834565d
Add per-review + per-comment token accounting, surfaced only when a PR carries the new AI-USAGE label (on top of the existing AI-REVIEW trigger). opencode_review: - run_opencode now uses `--format json`; parse_opencode_events reconstructs the assistant text from `text` events and sums tokens/cost/steps from every `step_finish` event (tolerant of noise / missing fields). - run() measures duration_s around the opencode call and returns (text, usage). - changed_files(diff) extracts the `+++ b/` paths; the brief now lists them under a "Changed files" focus block so the agent grounds findings in the diff's neighbourhood instead of unbounded whole-repo walks. ai_review: - format_usage_section renders a `## AI usage` block: measured totals (in/out/reasoning/cache/cost/steps/duration), the whole-repo scope note, and an attributed per-finding table. Per-comment counts are output tokens split by each finding's body weight — labelled "attributed" since one model pass produces all findings. - inline_comment_body appends `🪙 ~N tok (X% · attributed output)` when attribution is present. - review_pr gains report_usage; compute_attribution stashes _tok_attrib/_tok_pct. - format_review_body inserts the usage section between summary and findings. webhook_server: - Fire on every pull_request action except `closed` (denylist, was an allowlist) — the AI-REVIEW gate + sha dedupe keep this safe. - AI-USAGE label detection + PRAGENT_USAGE_ALWAYS env drive report_usage. .opencode factory + review-methodology skill: new "Ground findings in context" step — read callers/imports/sibling functions per changed file (1-3 files per finding), no unbounded walks. Tests: parse_opencode_events (text+usage sum, malformed tolerance, none-usage), changed_files, compute_attribution math, inline 🪙 line, format_usage_section totals/table/cost, format_review_body ordering. 68 passing. Co-Authored-By: Claude <noreply@anthropic.com>
66 lines
2.8 KiB
Markdown
66 lines
2.8 KiB
Markdown
---
|
||
name: review-methodology
|
||
description: pragent review methodology — severity rubric, what to report vs skip, anchoring rules, and how to honor repo focus. Load this before reviewing a PR.
|
||
---
|
||
|
||
# pragent review methodology
|
||
|
||
## Severity rubric
|
||
|
||
- **critical** — exploitable security bug, data loss/corruption, or a crash on
|
||
a normal input path. Must fix before merge.
|
||
- **high** — correctness bug on a real input path, broken contract, or a
|
||
missing test for security/error behavior. Should fix before merge.
|
||
- **medium** — likely bug on an edge case, missing test for changed logic, or a
|
||
risky pattern that isn't broken yet. Worth fixing.
|
||
- **low** — minor risk, stale expectation, or a defensive improvement. Nice to
|
||
have.
|
||
|
||
## Report vs skip
|
||
|
||
**Report:** correctness bugs, security problems, risky changes, missing tests
|
||
for changed behavior, breaking API/contract changes, N+1/O(n²) in hot paths.
|
||
|
||
**Skip:** praise, nitpicks, pure formatting/style, personal preference,
|
||
speculative "what if" without a concrete trigger, anything already covered in
|
||
`prior_reviews`.
|
||
|
||
Cap at ~15 findings, highest severity first. Quality over quantity — an empty
|
||
findings list for a clean diff is a correct result.
|
||
|
||
## Anchoring (for inline comments)
|
||
|
||
Each finding's `line` MUST be a line that exists in the POST-CHANGE version of
|
||
`path`:
|
||
- a **context** line (unchanged, shown with a leading space in the diff), or
|
||
- an **added** line (shown with a leading `+`).
|
||
|
||
Never anchor on a **removed** (`-`) line — it has no post-change line number.
|
||
If you're unsure of the exact line, use the closest context line you CAN see in
|
||
the diff. A misanchored finding becomes a summary bullet instead of an inline
|
||
comment, so correct anchoring is what gets a finding shown inline with its
|
||
suggested-fix code block (language-highlighted) rather than demoted to a bullet.
|
||
|
||
## Ground each finding in context
|
||
|
||
Don't flag a hunk in isolation. For each changed file, read its callers,
|
||
imports, sibling functions, and type definitions (the repo is checked out at
|
||
the head sha), and make sure the finding holds against how the change is
|
||
actually used. Keep it bounded — 1–3 related files per finding, no unbounded
|
||
whole-repo walks.
|
||
|
||
## Honoring repo config
|
||
|
||
If `.pr-review.json` is present, honor it:
|
||
- `focus` — weight these areas higher, but never ignore a critical issue
|
||
outside them.
|
||
- `exclude_paths` — skip findings in these paths.
|
||
- `languages` — hint to the primary languages; pick matching linters.
|
||
- `instructions` — house conventions / compliance language; treat as binding
|
||
reviewer rules.
|
||
|
||
## Linters are a signal, not the verdict
|
||
|
||
Run the repo's own typecheck/lint on changed files, but translate their output
|
||
into human findings — a raw `TS2322` is not a review comment. Correlate
|
||
diagnostics with the diff; ignore diagnostics in files the PR didn't touch. |