shoc-frontend-new/REVIEW_AND_PR_FRAMEWORK.md

REVIEW_AND_PR_FRAMEWORK.md — PR review contract

Every PR is reviewed against this contract, by humans and by review agents. The goal is high-signal review: catch real behavior, security, performance, and regression problems — not re-lint what the gates already enforce.

1. Review the exact head

• MUST review the diff at the current PR head, not a stale checkout. Re-pull before reviewing if new commits landed; stale approvals are dismissed on push by branch protection.
• MUST read the full diff of every changed file, including renames and generated/mapper code, not only the "interesting" components.

2. Board-backed regression inventory

• MUST check the change against the Seahaven Jira SH board inventory — all visible tickets for this area, not only the current ticket. Behavior that is Done / QA-approved / released is protected scope.
• MUST treat a plausible regression against protected behavior as a Blocker until disproven with repo evidence (the accepting tests, the linked PR/release, and a targeted check of the changed code paths).
• MUST NOT approve if board access or the relevant inventory is missing — say so and block rather than infer a pass. A missing ticket link alone is not a blocker, but known acceptance criteria must still be traced.

The current Jira workflow has no Ready for QA transition. Merged work remains in the documented pre-QA status until QA evidence supports Done; do not invent a status or mark unverified work Done.

3. Behavior-based testing

• MUST test through public behavior (rendered output, user interactions, query/mutation outcomes), not internal implementation details. Prefer @testing-library queries and user-event flows; assert what users observe.
• MUST NOT add tests whose only purpose is to prove a tool (ESLint, the governance script) executes — validate tooling by running the real gates (npm run verify), not with assertion-free unit tests.
• SHOULD cover the meaningful branches of new logic: the happy path, the error/empty state, and any boundary the change introduces.

4. Security review

• MUST confirm no raw server error payloads, stack traces, or internal IDs leak to the UI (see ARCHITECTURE_AND_CODE_QUALITY.md §Security).
• MUST confirm no secrets/tokens are introduced into the build, and that no VITE_* variable carries a secret (it is baked into the bundle).
• SHOULD check untrusted input is validated (Zod) before use and that dangerously-set HTML / unescaped server strings are not introduced.

5. Performance and Big-O review

• MUST flag algorithmic regressions in hot paths: O(n²)+ loops over server collections, re-filtering/sorting on every render, unbounded list rendering without virtualization.
• MUST confirm TanStack Query keys are stable and that mutations invalidate the correct keys (no stale cache, no redundant refetch storms).
• SHOULD question speculative useMemo/useCallback (add when measured) and unstable identities passed to memoized children.

6. No style-only comments

• MUST NOT leave comments that only restate what Prettier or ESLint already enforces (formatting, naming nits the linter catches). Style is settled by the gates; review is for behavior, correctness, security, and architecture.
• MUST NOT put Jira issue keys or ticket titles in source comments, JSDoc, or test names (for example (SH-183)). Ticket identity belongs in the PR, commit message, and branch — not in the code. Flag and request removal if a diff adds them.
• MUST make every comment actionable: tie it to a behavior, a risk, or an evidence-based convention in these docs, and offer a concrete fix or a targeted question. Use GitHub suggestion blocks when safe.

7. Review close-out

A review is complete when it records, briefly:

1. Findings ordered by severity (Blocker / Needs-change / Suggestion), or "no findings".
2. Open questions and their owner.
3. Which of the above checks were run, and any that were skipped (and why).
4. Residual risk, if approving.

Do not write a monolithic review body or a validation transcript into the PR surface; keep comments inline and high-signal.

Infra and application PRs stay separate. GitHub Actions owns SPA content (deploy-web.yaml). HCP Terraform owns the bucket and CloudFront. A change set that includes both terraform/ and deployable application files (src/, public/, pages/, config/, index.html, Vite/tsconfig, or .env*) fails G13. 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; "None" otherwise.
• 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.