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.

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.