mirror of
https://github.com/Sea-Haven-Industries/shoc-backend.git
synced 2026-09-30 09:33:13 +00:00
77c3016c9d
Some checks are pending
Validate and deploy / Validate deployable source bundle (push) Waiting to run
Validate and deploy / Deploy shoc-backend-dev through Terraform (push) Blocked by required conditions
Validate and deploy / Deploy shoc-backend-staging to Elastic Beanstalk (push) Blocked by required conditions
* feat(deploy): move dev application CD through Terraform GitHub creates the immutable Elastic Beanstalk version; HCP Terraform is the only UpdateEnvironment caller via a guarded version_label run. * fix: add permissions block for dependency-review workflow Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * fix(terraform): stop pinning the generated dev instance SG --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
136 lines
6.4 KiB
Markdown
136 lines
6.4 KiB
Markdown
# 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.
|
||
|
||
Do not mix deployable application changes with Terraform or CDK changes. The
|
||
first Terraform-owned application-CD change is the allowed exception because it
|
||
introduces `release_version_label`. Later PRs must keep those diffs separate.
|
||
|
||
## 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.
|