docs: make review checklist advisory and risk-based (#260)

Co-authored-by: PR Review <review@example.invalid>

Author: Yif-Yang

docs/contributing.md

@@ -75,9 +75,9 @@ mkdocs serve  # Preview at http://localhost:8000
 
 ## Code Review Guidelines
 
-We maintain a [code review & contribution guide](review_guidelines.md) distilled
-from maintainer review feedback. Reviewers and contributors should use it as a
-shared checklist before merging.
+See the [code review checklist](review_guidelines.md) for common considerations
+that may help reviewers and contributors. Apply the items relevant to the scope
+and risk of each change.
 
 ## License
 

docs/review_guidelines.md

@@ -1,80 +1,78 @@
 # Code Review & Contribution Guidelines
 
-> Distilled from maintainer review feedback on this repository, these are a
-> **shared baseline for both contributors and reviewers**. They are especially
-> useful early on — following the pre-submission self-check avoids the common
-> blockers that typically surface in a first review round — and they give
-> everyone a common vocabulary for what "ready to merge" means here.
-> These are **advisory guidance, not a hard policy**: reviewers and maintainers
-> apply them with judgment, and a PR that doesn't need a given check should not
-> be blocked on it.
+> This checklist summarizes common considerations for contributors and
+> reviewers. It is advisory, not an exhaustive merge policy: apply the items
+> relevant to the scope and risk of a change, and use maintainer judgment for
+> merge decisions.
 
 ## Review approach
 
-- **Recheck the current HEAD** — never review a stale version of the branch.
-- **Switch to the target branch before probing it** — the same file path can
-  differ across branches; probing from another branch reads an old copy and
-  produces false positives/negatives.
+- **Review the current PR HEAD** — confirm the exact revision before probing or
+  testing, and use the target branch as the comparison baseline.
 - **Reproduce the failure against the real version and real behavior**, not an
   imagined input.
 - **Check the real third-party contract** — verify the actual CLI/SDK flags and
   behavior a dependency supports, rather than assuming.
-- **Run focused + full test suites** and report the counts as evidence
-  (e.g. "99 passed / 1 skipped").
-- **Report blockers as "N blockers" with exact file:line and one sentence on why
-  it matters.**
-- When acknowledging, **point to concrete fixable spots** and distinguish
-  "useful but blocked" from "unusable".
+- **Run tests proportionate to the change** — include focused regression tests
+  and, when practical, the relevant broader suite; report the commands and
+  results.
+- **Separate blockers from suggestions** and make feedback actionable by citing
+  the relevant behavior or location and explaining the impact.
 
 ## Pre-submission self-check
 
-- [ ] Every supported version of a declared dependency range actually works
-      (version-adapt APIs; add a real build/launch smoke test per version).
+- [ ] The declared dependency range matches supported behavior; test
+      representative boundary versions when compatibility differs or the range
+      changes.
 - [ ] Error fallbacks fire only for the *expected* failure; other errors
       re-raise; existing targets fail closed; no out-of-bounds mutation.
-- [ ] Sensitive data is redacted **structurally** (mapping-key aware), on **every
-      output boundary** (json/md/stdout/file), while keeping debuggable content.
-      Secret-key detection uses **endswith** on the normalized key, not substring
-      (so `token_count` / `token_budget` diagnostics are preserved).
-- [ ] Shared mutable state (cache, accounting, errors) goes through a **locked**
-      helper; one caller's failure does not clobber another's success; accounting
-      is call-local; transient failures are not cached.
-- [ ] A root-cause fix to a shared function/helper verifies **all callers and
-      subclasses** (via `super()` inheritance) are covered, and gives new threads
-      a safe default; the test asserts the **quantity itself** so it can fail on
-      the old bug.
-- [ ] Concurrency tests use a **Barrier** to force overlap; degraded inputs are
-      covered (prefixed stderr, multiline, reentrancy), not just the happy path.
-- [ ] Third-party contracts are validated against the real version; smoke tests
-      exercise real calls, not just `--version`.
-- [ ] PR hygiene: original author attribution preserved, rebased on latest main,
-      no unrelated stacked commits, focused scope.
+- [ ] Sensitive data is redacted at every relevant output boundary while
+      preserving useful diagnostics. Prefer structural redaction for structured
+      data, and test both secret-key variants and non-secret lookalikes such as
+      `token_count`.
+- [ ] Shared mutable state is synchronized with a mechanism appropriate to the
+      implementation; failures and accounting remain isolated per operation,
+      and cache behavior is explicit.
+- [ ] A bug fix considers all affected callers and subclasses and includes a
+      regression test that fails before the fix and verifies externally
+      observable behavior.
+- [ ] Concurrency tests coordinate execution deterministically enough to
+      exercise the race (for example with barriers, events, or controlled hooks)
+      and cover relevant failure inputs.
+- [ ] Third-party behavior is checked against supported versions. Use an
+      integration smoke test when practical, or a faithful offline contract test
+      when live calls are unsuitable.
+- [ ] PR hygiene: focused scope, no unrelated commits, appropriate attribution,
+      and a sufficiently current base to assess conflicts and integration.
 - [ ] Domain math (stats/research/numeric) is correct and the formula verified.
 - [ ] Config/role routing is consistent (optimizer/target/judge are role-aware;
       matching model fields accompany backend changes).
 - [ ] Data isolation: test/eval data never leaks back into training; holdout/test
       sets are explicitly excluded.
-- [ ] Trust boundaries: external paths/ids are bound to a verified manifest/hash;
-      fail-closed validation.
+- [ ] External paths and identifiers are validated at the appropriate trust
+      boundary; invalid or untrusted inputs are rejected safely.
 - [ ] Features wired end-to-end (not just primitives).
 - [ ] Deliberate resilience/fallback contracts are respected, not "cleaned up";
       deprecated options retire cleanly rather than being re-purposed.
-- [ ] Reuse existing infrastructure over reinventing it.
+- [ ] Prefer existing infrastructure when it fits the requirement; introduce
+      new abstractions when they provide a clear benefit.
 - [ ] Sensitive content is redacted **before** reaching downstream consumers;
       resource operations are bounded.
-- [ ] Mutating endpoints are authenticated and CSRF-protected.
-- [ ] Backward compatibility preserved; the same contract behaves identically
-      across backends.
+- [ ] Mutating endpoints enforce appropriate authorization; browser endpoints
+      that rely on ambient credentials include CSRF protection or an equivalent
+      defense.
+- [ ] Avoid unintended compatibility regressions across supported backends;
+      document and test intentional backend-specific differences.
 - [ ] Filesystem boundaries handled (cross-drive paths, empty home, path
       normalization, symlinks).
-- [ ] First mergeable slice submitted; follow-ups as independent PRs — each with
-      its own tests.
+- [ ] Keep each PR a coherent, reviewable slice with tests appropriate to its
+      behavior; split unrelated follow-up work into separate PRs.
 
 ## Notes
 
-- The most common review root cause is **validating the shape you imagined
-  rather than the shape reality produces**. Prefer running the real entry point
-  once with a realistic input before writing assertions.
-- When a failure is reported, find and fix the **root cause in the shared
-  function/helper** (one fix covers all callers) — but align the fix with the
-  codebase's existing conventions to avoid over- or under-correction.
+- A recurring review failure mode is validating an assumed shape rather than
+  the behavior the real entry point produces. When practical, exercise the real
+  entry point with representative input before writing assertions.
+- Fix failures at the narrowest layer that correctly covers the affected
+  callers; check neighboring callers and follow existing conventions to avoid
+  over- or under-correction.