shoc-backend/REVIEW_AND_PR_FRAMEWORK.md

# Review and PR Framework (canonical)

> **Status: canonical.** This document owns *who* reviews, in what order, with
> what evidence, and how exceptions are recorded. The rules under review live in
> `ARCHITECTURE_AND_CODE_QUALITY.md`; the gates live in `QUALITY_GATES.md`.

## 1. Exact-head review

Reviews are performed against the **exact HEAD** the PR will merge, not a stale
snapshot:

- Fetch and review the precise merge commit / head SHA. If new commits land
  after review, the review covers only the previously seen diff until re-run.
- Comments cite `file:line` against current HEAD so they are navigable and
  reproducible.
- A "looks good from what I remember" approval is invalid; re-diff before
  approving.

Review order is **business-rule-first, high-signal, low-false-positive**:

1. Correctness of the changed business behavior vs. the acceptance criteria.
2. Regression risk against board-backed protected behavior (§2).
3. Security / data / authorization (§3).
4. Architecture / dependency direction (the `ArchitectureTests` gate already
   covers mechanical direction; review covers *semantic* layering).
5. Performance / complexity evidence (§4).
6. Tests: do they assert behavior through public contracts?
7. Nitpicks / style last, and only if not already caught by `dotnet format`.

## 2. Board-backed regression inventory

A task is incomplete if it implements the current change while regressing
behavior that was already working. Before claiming review/merge readiness:

- Pull the **Seahaven Jira SH board inventory** — all
  visible tickets, not only the current one: key, title, type, status,
  component, acceptance criteria, linked PR/release, and QA evidence.
- Check the change against that inventory. A plausible regression against
  protected (Done / released / QA-approved) behavior is a **Blocker** until
  disproven with repo evidence and targeted validation.
- **Missing board access, incomplete inventory, or missing PR-to-ticket
  traceability is Blocked / NOT READY** — never a pass. Say so explicitly.

If Jira cannot be reached, report the gate as **not-run / blocked**, not green.
The current workflow has no `Ready for QA` transition; merged-but-unverified
work remains in the documented pre-QA status and must not be mislabeled Done.

## 3. Security, data, and authorization evidence

For changes touching auth, data, crypto, external input, or configuration,
review requires evidence (not assertion) for:

- **No exception disclosure** — changed endpoints do not leak exception
  messages, stack traces, or SQL/provider/path/credential/crypto detail to the
  client. `SanitizedErrorsTests` covers the shared boundary; changed handlers
  must route through it.
- **Secret handling** — no secret-bearing field is logged or serialized into a
  response. Structured log templates must not interpolate secrets.
- **Tenant scope** — scope is server-derived and authorization is enforced at
  service entry (§2 of the architecture doc).
- **Authorization** — the service entry trust boundary is intact; controllers
  express HTTP policy and delegate.

Cite the test, log redaction, or code path for each. "We don't think it leaks"
without evidence is a fail.

## 4. Performance and Big-O evidence

For each changed hot path, the PR provides:

- **Complexity** — CPU and memory Big-O for any in-memory step (e.g., dictionary
  reconstruction is `O(n)`; repeated `First`/`Single` scans are `O(n²)`).
- **SQL-backed evidence** — claims that filtering/ordering/paging execute in
  SQL, or that there is a single round trip, are backed by provider query logs,
  `ToQueryString()`, store SQL, or a measured count/plan — **not** inferred from
  LINQ shape.
- **Round trips / loops** — no query-in-loop / save-in-loop where a set-based
  operation preserves semantics; document when a loop is intentional and why.
- **Partial-failure / transaction behavior** — preserved or explicitly changed
  by business decision (see architecture §4/§8).

## 5. Tests

- Tests assert **behavior through public interfaces**, not linter/analyzer
  self-proofs.
- Cover: the happy path, authorization, data contracts, ordering, duplicates,
  missing records, and relevant failure boundaries.
- New I/O paths include cancellation-forwarding evidence (architecture §6).
- A test that only proves "the rule exists" is not a behavior test; rewrite it
  to exercise the product behavior the rule protects.

## 6. Size and "no godfile theater"

- Do not request splits based on line/file counts. A number is not feedback.
- If a unit is too large, name the **concrete second responsibility** or the
  **untestable coupling** and propose the seam. Otherwise size is not a blocker.
- Do not introduce file/line thresholds into gates (architecture §9).

## 7. Exceptions (ADR)

A deviation from a hard rule requires an **Architecture Decision Record** under
`docs/adr/NNNN-title.md` (create the path if absent), referenced from the PR:

- context, decision, consequences;
- the specific rule being excepted;
- expiry/review date.

Suppressions are single-diagnostic and cite the ADR — **no wildcard
suppressions** (no global `[SuppressMessage]`, no `.editorconfig` severity
sweeps, no `#pragma` swaths). See architecture §10.

Infra and application **PRs** stay separate. GitHub Actions owns Elastic
Beanstalk application versions. HCP Terraform owns infrastructure and ignores
`version_label`. A change set that includes both `terraform/` and deployable
application files (`.cs`, `.csproj`, `.razor`, `.ebextensions/`, or the
Elastic Beanstalk package/smoke scripts) fails the isolation check. Workflow,
docs, and gate-script changes may travel with either side.

## 8. PR description contract (minimal)

- **Summary** — what changed and why, in plain language.
- **Changes and value** — grouped by area, each with the value it delivers.
- **Ticket** — the board key(s) when one applies. A missing ticket link is not
  itself a blocker; missing board regression access or contradictory acceptance
  criteria is.
- Link G6 migration evidence if the PR adds/changes a migration.
- Link any ADR relied upon.

Avoid boilerplate: no deployment notes, no validation transcripts, no
"residual-risk" theatre, no AI signatures. The validation story lives in the
check run results and the close-out, not in the PR body.

`.github/PULL_REQUEST_TEMPLATE.md` pre-fills this layout and overrides the org
template, whose Summary / Validation / Tests / Notes headings this repository
does not use. The org `callable-pr-policy` workflow hard-codes those four
headings; it is not wired into this repository, and this layout is the reason.

## 9. Merge readiness

Merge requires: exact-head review done; board-backed regression check pass (or
documented blocked); security/perf evidence present where applicable; CI green
(G1–G5); migration evidence (G6) if touched; no open hard-rule exceptions
without an ADR. Report each as pass / fail / skipped / not-run — never infer a
pass.