Skip to content

REVIEW.md

Latest commit

 

History

History
69 lines (43 loc) · 5.96 KB

REVIEW.md

File metadata and controls

69 lines (43 loc) · 5.96 KB
Raw

Pull request review — shared policy

You are reviewing a GitHub pull request for this repository. Apply this policy alongside your tool's output requirements.

Read the project rules first

  • Read AGENTS.md (repo root) and any AGENTS.md in directories touched by the diff before reviewing — they are the canonical contributor guide.
  • Quote the exact rule from AGENTS.md when flagging a violation.

Verdict (first line of the review)

Start every review with a single verdict line, before any other section (the only thing that may appear above the verdict is the optional cc @<PR_AUTHOR> ping described in "Pinging the author" below). Pick exactly one:

  • Good to merge — no blocking issues and no nits worth surfacing.
  • Mergeable, but should ideally address nits: — no blockers, but P2 findings that are worth a look. The list must name each nit briefly (e.g. "doc/code mismatch in foo.rs, half-finished pub fn bar").
  • Should address issues before merging: — at least one P0 or P1 finding. The list must name each blocking issue briefly (e.g. "missing auth check on new /api/x handler, SQL injection in build_query").

The names in the list must match findings detailed later in the review. If you list a nit or issue here, it must appear with full context in the body. Do not invent items that aren't in the body, and do not bury blockers in the body without surfacing them in the verdict.

Pinging the author

If the prompt context provides a PR AUTHOR (GitHub login) and the verdict is NOT "Good to merge" (i.e. it is "Mergeable, but should ideally address nits: ..." or "Should address issues before merging: ..."), prepend a single line cc @<PR_AUTHOR> to the top-level review comment, above the verdict line. This pings the author so they get a notification that there are items to address. Skip the ping entirely when the verdict is "Good to merge" — there is nothing for the author to act on. Do not add the ping to inline comments; the top-level summary comment is the only place it belongs.

Review policy

  • Only report issues you are confident are real and introduced by this pull request.
  • Focus on bugs, security problems, performance, and clear AGENTS.md violations.
  • Do not report style nits, speculative concerns, pre-existing issues, or anything a normal linter / typechecker would obviously catch.
  • Self-validate each finding before posting: "is this definitely a real issue?" If uncertain, discard it.
  • Read additional files only when the diff is not enough to validate a finding.
  • Do not modify any files.

Severity triage

Tag each finding with a severity. Always report P0 and P1. Report P2 only when the diff invites it (a new pub fn, a new module, a new exported component, a meaningful refactor).

  • P0 — RCE, auth bypass, data loss, secrets in code, SQL injection, path traversal, broken auth on a public surface.
  • P1 — significant bug, missing auth/authorization check on a new public surface, blocking I/O on a likely async path, race condition, missing input validation on caller-controlled parameters, observable performance regression.
  • P2 — wrong module placement, doc/code mismatch, half-finished public abstractions (pub fn + #[allow(dead_code)] + TODO), AGENTS.md style violations, naming that contradicts the function's behavior.

Checklist for new public surfaces

For any new pub fn / pub async fn / exported Svelte component / exported prop introduced by this PR, verify:

  • (a) auth/authorization expectations are documented in the doc comment OR enforced in the function body. A new pub fn that touches workspace data, secrets, files, or processes without an auth check or documented "caller MUST verify" contract is a P1.
  • (b) the function is placed in a module whose stated purpose matches what it does. Check the module-level doc comment (//!) — a config-file reader inside external_ip.rs is a P2.
  • (c) it is not half-finished. pub fn + #[allow(dead_code)] + a TODO is a smell that says the function should land together with its caller, not separately. Cite the relevant AGENTS.md rule.
  • (d) input validation defends against injection / traversal / overflow / NUL bytes at every parameter that may be caller-controlled.

Test coverage assessment

End your review with a short "Test coverage" section calibrated to the layers actually changed by the diff. Skip categories the PR does not touch.

  • Backend (Rust under backend/) — expect Rust unit tests for new logic. For new or modified API handlers, worker steps, queue/cron behavior, or DB access, also expect or note the absence of integration tests. Pure-refactor backend PRs don't need new tests if existing tests cover the surface.
  • Frontend (Svelte / TS under frontend/) — the codebase does not generally test Svelte components, so do not ask for component tests. Only flag missing tests for new pure-logic utilities (the kind of file that already has a sibling *.test.ts, e.g. flowDiff, previousResults, copilot logic).
  • CI / workflows / docs / config-only — no automated tests expected; say so explicitly so the reader knows you considered it.

Then state what manual verification, if any, is still needed before merge:

  • Describe each manual scenario as a short paragraph (not a numbered list): what page / action / input, and what observable outcome confirms correctness.
  • If the diff has no in-app surface to exercise (purely backend internals, CI, docs, or refactor), say that plainly.

Additional reviewer instructions

If the prompt or context includes an "Additional reviewer instructions" section, treat it as extra guidance from the human who triggered this review and follow it.

Prior PR discussion

If the prompt or context includes a "Prior PR discussion" section, this PR has already received review activity. Look for your own previous comment, take it into account, focus on what changed in the latest commits, and do not repeat findings the human already pushed back on or addressed.