diff --git a/.github/workflows/architecture-quality.yml b/.github/workflows/architecture-quality.yml index ada851d..14ce54c 100644 --- a/.github/workflows/architecture-quality.yml +++ b/.github/workflows/architecture-quality.yml @@ -2,7 +2,6 @@ name: Architecture and changed-file quality on: pull_request: - branches: [main, dev] permissions: contents: read @@ -21,31 +20,10 @@ jobs: with: dotnet-version: "8.0.x" - - name: Restore - run: dotnet restore SeaHavenIndustries.sln - - - name: Verify architecture boundaries - run: >- - dotnet test - Api.SeaHavenIndustries.Tests/Api.SeaHavenIndustries.Tests.csproj - --no-restore - --filter FullyQualifiedName~ArchitectureTests - - - name: Verify formatting of changed C# files + # CI and local run the same complete repository gate. + - name: Repository quality gate shell: bash - run: | - mapfile -t files < <( - git diff --name-only --diff-filter=ACMR \ - "${{ github.event.pull_request.base.sha }}" \ - "${{ github.event.pull_request.head.sha }}" \ - -- '*.cs' - ) - - if (( ${#files[@]} == 0 )); then - exit 0 - fi - - dotnet format SeaHavenIndustries.sln \ - --no-restore \ - --verify-no-changes \ - --include "${files[@]}" + env: + BASE_REF: ${{ github.event.pull_request.base.sha }} + HEAD_REF: ${{ github.event.pull_request.head.sha }} + run: bash scripts/governance-check.sh diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..03e95fe --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,70 @@ +# AGENTS.md — SeaHaven backend (repo-specific delta) + +This file is the **repo-specific delta** for the Seahaven backend. It layers on +top of the operator/workspace agent baseline and does not repeat it. This file +may add stricter backend rules but may never weaken the workspace baseline. + +## Canonical governance documents (precedence) + +1. `ARCHITECTURE_AND_CODE_QUALITY.md` — *what* the rules mean (canonical; + supersedes the older `BACKEND_ARCHITECTURE.md`, which is retained only as + historical reference). +2. `QUALITY_GATES.md` — *how* rules are enforced (commands + CI mapping). +3. `REVIEW_AND_PR_FRAMEWORK.md` — *who* reviews, in what order, with what + evidence. + +When these conflict with each other, the one that *owns* the topic wins +(meaning → architecture doc; execution → quality gates; process → review +framework). When unsure, ask; do not silently pick. + +## Architecture in one line + +```text +Controller -> I{Feature}Service -> I{Feature}DataService -> ApplicationDbContext +``` + +- Controllers depend on `I{Feature}Service` only — never `DbContext`/EF/concrete + services/data services. +- Business services depend on feature-specific data/query/command interfaces — + never `DbContext`, never `IConfiguration`, never a generic repository. +- Data services own EF; EF/`DbContext` lives only on the documented + infrastructure allowlist (data services, `ApplicationDbContext`, migrations, + Identity/composition, transactions inside data services). +- One commit convention: `SaveChangesAsync(CancellationToken)` inside the owning + data service is the atomic boundary; explicit transactions stay inside data + services. +- Tenant scope is server-derived; authorization is enforced at service entry. +- No HTTP response leaks exception/stack/SQL/credential/crypto detail; secrets + are never logged. + +Full detail: `ARCHITECTURE_AND_CODE_QUALITY.md`. + +## Running the complete gate (local == CI) + +```bash +bash scripts/governance-check.sh +``` + +The command restores, verifies architecture and changed-file formatting, builds +Release, and runs the full test suite. CI (`architecture-quality` workflow) +calls the same script. See `QUALITY_GATES.md`. + +## Hard rules (deviation needs an ADR; no wildcard suppressions) + +- EF allowlist, dependency direction, no client exception disclosure, + server-derived tenant scope, one commit convention, cancellation forwarding + (verified, not just signed). +- Suppress a single diagnostic with a cited ADR only — never global sweeps. +- See `ARCHITECTURE_AND_CODE_QUALITY.md` §10 for the ADR exception process. + +## Do not touch without explicit instruction + +- Product behavior, database **migrations**, secrets, dependency versions, and + files unrelated to the current task. +- The `.ai-config-kit-sidecar-write-scope` marker file is a transient + authorization artifact — **never commit it**. + +## Reporting + +State every changed file with a one-line summary. Report each gate as +pass / fail / skipped / not-run with evidence; never infer a pass from silence. diff --git a/ARCHITECTURE_AND_CODE_QUALITY.md b/ARCHITECTURE_AND_CODE_QUALITY.md new file mode 100644 index 0000000..54b35a1 --- /dev/null +++ b/ARCHITECTURE_AND_CODE_QUALITY.md @@ -0,0 +1,273 @@ +# Architecture and Code Quality (canonical) + +> **Status: canonical.** This document owns the *meaning* of the backend's +> architecture and code-quality rules. On any conflict with the older +> `BACKEND_ARCHITECTURE.md`, **this document governs**; that file is retained +> only as historical reference pending removal. +> +> Companion documents: +> - `QUALITY_GATES.md` — *how* each rule is enforced (commands + CI mapping). +> - `REVIEW_AND_PR_FRAMEWORK.md` — *who* reviews, in what order, with what +> evidence, and how exceptions are recorded. +> - `AGENTS.md` — precedence and repo-specific agent delta. + +## 1. Layering model + +The backend is a feature-oriented service boundary over Entity Framework Core: + +```text +HTTP controller -> I{Feature}Service -> I{Feature}DataService -> ApplicationDbContext +``` + +The interfaces are **architectural seams**, not a mandate to wrap every class or +every EF method. Add an interface when it separates HTTP, business, +persistence, or external I/O responsibility *and* enables behavior-focused +testing through the public contract. + +| Layer | May depend on | May not depend on | +|-------|---------------|-------------------| +| API controller | `I{Feature}Service`, `ILogger`, framework types | `DbContext`, concrete services, concrete data services, raw SQL, business workflow code | +| Business service (`SeaHaven.Services`) | `I{Feature}DataService`, `IOptions`, `ILogger`, pure ports (`IServicePorts`, `IDispatchEmailPort`, …) | `DbContext`, `IConfiguration`, concrete data services, a generic repository | +| Data service (`SeaHaven.DataServices`) | `ApplicationDbContext`, EF Core | Business decisions, HTTP concerns | +| Composition / infrastructure | `Program.cs`, `*Module.cs`, migrations, Identity | Being depended on by controllers or business services | + +### 1.1 Controllers + +- Parse HTTP input, authorize the caller, invoke a business-service interface, + and map the result to the established HTTP contract. +- Depend **only on `I{Feature}Service` interfaces** — never on `DbContext`, EF, + concrete service implementations, or concrete data-service implementations. +- Contain no persistence queries and no business workflows. +- Map known failures explicitly; let the centralized exception boundary handle + unexpected failures (see §5). + +### 1.2 Business services + +- Own validation, business decisions, orchestration, and **transaction intent** + (the *what* of semantic atomicity — see §4). +- Depend on **feature-specific data/query/command interfaces**, never on a + generic repository and never on `DbContext`. +- Use `IOptions` for typed configuration. Never inject `IConfiguration`. +- Keep feature responsibilities cohesive. Split a service when it has + independently changing business reasons — **not** because it crossed an + arbitrary line count (see §9, "no godfile threshold theater"). + +### 1.3 Data services + +- Own EF Core query shape, persistence, batching, and **transaction mechanics** + (the *how* of commits — see §4). +- Accept `CancellationToken` on new I/O methods and propagate it (see §6). +- Use `AsNoTracking()` for read-only queries. +- Project only required columns for list/read models; include full graphs only + when the caller needs them. +- Page unbounded collections at the database. +- Avoid query-in-loop / save-in-loop patterns; prefer set-based reads and + batched writes **while preserving the workflow's failure and transaction + semantics**. +- Do **not** introduce a generic repository over EF Core. Feature data services + expose intent-revealing operations. + +### 1.4 Infrastructure ownership and the EF allowlist + +EF Core / `DbContext` is owned by approved infrastructure only. The explicit +allowlist of places that may touch EF directly: + +- `SeaHaven.DataServices.Implementation.*` (feature data services). +- `Data.SeaHavenIndustries` — the `ApplicationDbContext`, entity configuration, + and **migrations** (see §7). +- `SeaHaven.DataServices.DependencyInjection.DataServicesModule` — context + registration and lifetime. +- Identity / authentication composition in `Program.cs` and the DI modules + (`AddDbContext`, `AddIdentity`, token/options wiring). +- Transaction mechanics inside data services when semantic atomicity requires + it (see §4). + +Anything outside this allowlist that references EF / `DbContext` is an +architecture violation. New allowlist entries require an ADR (see §10). + +## 2. Tenant scope and authorization + +- **Tenant scope is server-derived.** Filtering by tenant/customer/owner is + applied in the data access layer from authenticated principal claims, never + from client-supplied request bodies or query strings. +- **Authorization remains at service entry.** A business service's public method + is the trust boundary: it authorizes the caller and resolves the effective + tenant scope before doing work. Controllers express HTTP-level authorization + (`[Authorize]`, route/policy) and then delegate; they do not re-implement + service-level authorization. +- Data services receive an already-resolved, server-derived scope; they must not + invent authorization rules of their own. + +## 3. Dependency-direction enforcement (mechanical) + +`ArchitectureTests` (in `Api.SeaHavenIndustries.Tests`) enforces mechanical +dependency direction by reflection over constructor parameters: + +1. Controllers do not inject `DbContext`, concrete data services, or concrete + service implementations. +2. Business services do not inject `DbContext` or `IConfiguration`. +3. Business services depend on interfaces for their data/persistence + dependencies, not on concrete data-service or service implementations. +4. **Discovery must not silently pass on an empty set.** Each invariant asserts + the discovered type set is non-empty, so the gate **fails** if reflection + ever returns zero controllers/services (e.g., after a namespace move). A + passing run therefore means "real types were checked," not "nothing was + found." + +These tests cover *dependency direction*. Behavior lives in behavior tests +through public interfaces. Do **not** add tests merely to prove a linter or +analyzer itself works — test the product behavior the rule protects. Do not +claim full runtime properties from constructor reflection; it proves wiring +shape, not execution behavior. + +## 4. Transaction semantics and the one commit convention + +- **Services own semantic atomicity.** A business service decides which set of + operations constitutes a unit of work and expresses that intent. +- **Scoped infrastructure owns transaction mechanics.** The actual commit + boundary is `DbContext.SaveChangesAsync(...)` and lives **inside a data + service**. A service must not sprinkle independent `SaveChanges` calls across + data-service boundaries in a way that breaks the semantic unit. +- **The repo documents one commit convention:** a single + `SaveChangesAsync(CancellationToken)` inside the owning data service is the + atomic commit boundary. This is the current convention observed across the + data-service layer. +- When a unit of work genuinely requires multiple statements to be all-or- + nothing, use EF Core's transaction (`IDbContextTransaction`) **inside the data + service**. Explicit transactions are infrastructure (EF allowlist), owned by + data services, never by controllers or business services. +- Do not "optimize" atomicity away: collapsing several semantically independent + saves into one final save changes partial-success behavior and is a business + decision, not mechanical cleanup. + +## 5. Error disclosure and structured logging (security) + +- **No HTTP response may expose** arbitrary exception messages, stack traces, + or SQL/provider/path/credential/crypto detail. Known failures are mapped to + explicit public messages; unexpected failures are funneled through the + centralized boundary (`SanitizedErrors`), which returns a stable message plus + a correlation reference and never echoes internal exception text. +- **Correlated structured internal logs** record the original exception at + `Error` level with a correlation id, so incidents are traceable without + leaking detail to the client. +- **Secrets are never logged.** Configuration values, tokens, connection + strings, and credentials are treated as redacted; structured log templates + must not interpolate secret-bearing fields. +- The `SanitizedErrorsTests` guard the public-facing contract (stable message, + correlation id present, original exception text absent, original logged at + `Error`). + +## 6. Cancellation + +- New async I/O methods accept a `CancellationToken` where the public contract + permits and **propagate it** to EF Core / downstream I/O. +- **Do not falsely claim propagation.** A `CancellationToken` parameter that is + ignored, swallowed, or replaced with `default`/`CancellationToken.None` at the + call site is a defect, not a courtesy. The signature is a contract. +- Enforcement combines **behavior tests** (assert the token reaches the data + service / EF call for changed I/O paths) and **analyzers** where available. + Either is acceptable evidence; silence is not. For changed paths, review must + confirm forwarding, not merely the presence of the parameter. + +## 7. Database migrations + +Migration gates require **actual-provider** evidence, not reflection or +assumptions: + +- **Actual-provider discovery** — the migration provider is the configured + database provider (e.g., SQL Server / Npgsql), discovered from the runtime + configuration, not inferred from package references. +- **Snapshot consistency** — `ModelSnapshot` must be consistent with the model + after the migration set (`dotnet ef ... --output` / script diff with no + phantom operations). +- **Empty-db apply** — applying the full migration history to an empty database + must reproduce the current model with no errors. +- **Supported-prior upgrade** — upgrading from each still-supported prior state + applies cleanly. +- **No untested rollback claims.** Do not assert "down/rollback is supported" + unless a downgrade is actually exercised against the real provider; otherwise + state rollback as **not guaranteed**. EF "EnsureDeleted"/re-create is not a + migration rollback. + +## 8. Query performance and complexity + +For each changed hot path, document and test: + +- input-size variables; +- CPU and memory complexity (Big-O) for any in-memory reconstruction; +- database round trips; +- whether filtering, ordering, and paging execute **in SQL** vs. in memory; +- whether tracking is necessary; +- whether repeated work can be hoisted out of loops; +- the partial-failure and transaction behavior. + +Distinguish two classes of finding: + +- **Analyzable anti-patterns** — query-in-loop / save-in-loop, unbounded load + followed by in-memory `Where`, repeated `First`/`Single` scans over a result + (prefer dictionary/hash-set reconstruction, normally `O(n)` vs `O(n²)`). These + can be flagged by inspection. +- **SQL-backed evidence** — actual paging/filtering/sort location, query count, + and plan shape must be backed by provider query logs, `ToQueryString()`, + store-generated SQL, or a plan/count measurement. Do **not** assert "executes + in SQL" or "single round trip" from LINQ shape alone. + +Do not optimize by weakening correctness: batching all writes into one final +save can change partial-success behavior; adding a transaction can change +locking/retry semantics. Both are business decisions. + +## 9. Size, complexity, and "no godfile theater" + +- We do **not** enforce arbitrary line-count or file-size thresholds as quality + gates. A number is not an architecture. +- A file is too large when it has **independently changing responsibilities** + (low cohesion) or **untestable coupling**, not when it crosses a count. + Split on seams, not on a counter; describe the seam in the PR. +- Gates that operate on size must name the *consequence* (e.g., "two unrelated + features in one service") and be backed by review, not by a magic threshold. + +## 10. Exceptions and ADRs + +Some rules here are hard (EF allowlist, no client exception leakage, server- +derived tenant scope). A deviation from a hard rule requires an **Architecture +Decision Record** under `docs/adr/` (create the path if absent): + +- A short `NNNN-title.md` stating context, decision, consequences, and the + specific rule being excepted. +- The excepted rule and an expiry/review date. +- Reference from the PR that relies on it. + +Without an ADR, the rule stands as written. Explicit, documented exceptions +only — **no wildcard suppressions** (no global `[SuppressMessage]`, no +`.editorconfig` `dotnet_diagnostic.*.severity = none` sweeps, no +`#pragma` swaths). A suppression must name the single diagnostic and cite the +ADR/justification. + +## 11. Configuration + +Configuration is bound once during composition in +`SeaHaven.Services/DependencyInjection/ServicesModule.cs`. Business services +receive typed options (`FrontendOptions`, `JwtOptions`, `ApprovalsOptions`, +`VendorPortalOptions`, …). Defaults must preserve current behavior. Startup +validation is introduced only when every deployed environment is known to +provide the required value. + +## 12. Review checklist (quick reference) + +- Controller depends only on `I{Feature}Service` abstractions. +- Business logic is in a cohesive feature service. +- EF operations are behind a feature data-service abstraction. +- No raw `IConfiguration` in a business service. +- No unbounded load + in-memory filtering. +- No query/save loop where a set-based operation preserves semantics. +- Read-only EF queries use no tracking. +- New async I/O accepts and **propagates** cancellation where the contract + permits (verified, not just signed). +- Errors do not leak internal exception details; secrets are not logged. +- Tenant scope is server-derived; authorization enforced at service entry. +- Commit convention respected (single `SaveChangesAsync` in the owning data + service; transactions only in data services when required). +- Tests cover behavior, authorization, data contracts, ordering, duplicates, + missing records, and relevant failure boundaries. +- Changed behavior checked against the authoritative ticket board (see + `REVIEW_AND_PR_FRAMEWORK.md`). Missing board access blocks readiness/merge. diff --git a/Api.SeaHavenIndustries.Tests/ArchitectureTests.cs b/Api.SeaHavenIndustries.Tests/ArchitectureTests.cs index b636e67..8624cca 100644 --- a/Api.SeaHavenIndustries.Tests/ArchitectureTests.cs +++ b/Api.SeaHavenIndustries.Tests/ArchitectureTests.cs @@ -73,4 +73,25 @@ public class ArchitectureTests violations.Should().BeEmpty( "business services must depend on data-service interfaces and typed options, never on DbContext or IConfiguration"); } + + [Fact] + public void BusinessServices_DependOnInterfaces_NotConcreteServiceOrDataServiceImplementations() + { + // Guards the feature-specific interface boundary: a business service must + // depend on I{Feature}DataService / I{Feature}Service abstractions, never on + // concrete service or data-service implementations (no generic-repository + // theater and no coupling to persistence implementations). + BusinessServiceImplementations.Should().NotBeEmpty( + "business services must be discovered for this invariant to be meaningful"); + + var violations = ParametersOf(BusinessServiceImplementations) + .Where(p => IsConcreteServiceImplementation(p.ParameterType) + || IsConcreteDataService(p.ParameterType)) + .Select(p => $"{p.Member.DeclaringType!.Name} depends on {p.ParameterType.FullName} ({p.Name})") + .ToList(); + + violations.Should().BeEmpty( + "business services must depend on interfaces (I{Feature}Service / I{Feature}DataService), " + + "never on concrete service or data-service implementations"); + } } diff --git a/QUALITY_GATES.md b/QUALITY_GATES.md new file mode 100644 index 0000000..e7b5c84 --- /dev/null +++ b/QUALITY_GATES.md @@ -0,0 +1,125 @@ +# Quality Gates (canonical) + +> **Status: canonical.** This document owns *how* the architecture and +> code-quality rules are enforced: the gate list, the local command, the CI +> mapping, and the pass/fail/skip reporting semantics. The *meaning* of each +> rule lives in `ARCHITECTURE_AND_CODE_QUALITY.md`. +> +> **CI and local run the same check.** Every gate below is executed by +> `scripts/governance-check.sh`. The `architecture-quality` workflow calls that +> script so a green run means the same thing locally and in CI. Do not add a +> check to CI that is not also runnable locally through the script (and vice +> versa). + +## Gate inventory + +| # | Gate | Protects (architecture §) | Local command | CI step | +|---|------|---------------------------|---------------|---------| +| G1 | Restore | build integrity | `dotnet restore SeaHavenIndustries.sln` | `architecture-quality` → `governance-check.sh` | +| G2 | Architecture boundary tests | §1, §3 (dependency direction) | `dotnet test Api.SeaHavenIndustries.Tests ... --filter FullyQualifiedName~ArchitectureTests --no-restore` | same script | +| G3 | Changed-file formatting | §1 (conventions) | `dotnet format SeaHavenIndustries.sln --no-restore --verify-no-changes --include ` | same script | +| G4 | Release build | compile correctness | `dotnet build SeaHavenIndustries.sln -c Release --no-restore` | `architecture-quality` and `ci` | +| G5 | Full test suite | behavior | `dotnet test SeaHavenIndustries.sln -c Release --no-build` | `architecture-quality` and `ci` | +| G6 | Migration gates | §7 | see §"Migration gates" below | on-demand / release | +| G7 | Cancellation forwarding | §6 | behavior tests on changed I/O paths + analyzer | review-enforced on changed paths | +| G8 | Error disclosure | §5 | `SanitizedErrorsTests` (part of G5) | `ci` | +| G9 | Board-backed regression | review framework | `REVIEW_AND_PR_FRAMEWORK.md` inventory | review-enforced | + +## How to run locally + +```bash +# Complete local repository gate (mirrors the architecture-quality workflow): +bash scripts/governance-check.sh + +# By default it compares against the default branch for changed-file formatting. +# Override the comparison point: +BASE_REF=origin/dev bash scripts/governance-check.sh +BASE_REF=main bash scripts/governance-check.sh +``` + +The script: +1. `dotnet restore` (G1) +2. runs the `ArchitectureTests` filter with `--no-restore` (G2) +3. computes changed `.cs` files vs `BASE_REF` (default `origin/dev`) and runs + `dotnet format --verify-no-changes --include ...` (G3). When there are no + changed C# files it skips G3 with an explicit "skipped: no changed C#" line. +4. builds the complete solution in Release with no restore (G4). +5. runs the complete solution test suite in Release with no rebuild (G5). + +## Migration gates (G6) + +Provider-real, not reflection-derived. Run against the configured provider: + +- **Actual-provider discovery** — confirm the runtime provider matches the + configured connection (e.g., SQL Server / Npgsql) from configuration, not + from package guesses. +- **Snapshot consistency** — `dotnet ef migrations script --idempotent` (or a + model diff) shows no phantom operations; `ModelSnapshot` matches the model. +- **Empty-db apply** — `dotnet ef database update` against a fresh empty + database applies the full history cleanly. +- **Supported-prior upgrade** — upgrade from each still-supported prior + migration applies cleanly. +- **Rollback** — report **not guaranteed** unless a real downgrade was + exercised against the provider. Never claim "EF can roll it back" untested. + +Migration gates are run on-demand and gated for releases; they are not in the +per-PR fast path because they require a database. A PR that adds/changes a +migration must document the G6 evidence in the PR description. + +## Cancellation forwarding (G7) + +For changed async I/O paths, the requirement (§6) is **verified forwarding**, +not a signature checkbox. Acceptable evidence, any one of: + +- a behavior test asserting the `CancellationToken` reaches the data service / + EF call; or +- an analyzer diagnostic confirming propagation on the changed method. + +A parameter that exists but is swallowed/`default`-ed is a **fail**. If an +analyzer is unavailable for a path, a behavior test is required; silence is not +acceptable. Review enforces this on changed paths. + +## Architecture discovery must fail on zero (G2 invariants) + +Each reflection invariant in `ArchitectureTests` asserts the discovered set is +non-empty. A future namespace move that makes reflection return zero types will +**fail the gate** rather than silently pass. The current invariants: + +1. Controllers inject neither `DbContext`, concrete data services, nor concrete + service implementations. +2. Business services inject neither `DbContext` nor `IConfiguration`. +3. Business services depend on interfaces (not concrete service/data-service + implementations) for their data and persistence dependencies. + +## What this does not claim + +- Reflection over constructors proves **wiring shape**, not runtime behavior. + Runtime/semantic properties (cancellation actually cancelling, a query + executing in SQL, a transaction actually committing atomically) are proven by + behavior tests or provider evidence, never by reflection. +- A passing script proves G1–G5 ran in sequence. It does not prove the + provider-backed or review-owned gates G6–G9. + +## Pass / fail / skip / not-run reporting + +Every gate result is reported as exactly one of: + +- **pass** — ran and succeeded, with evidence. +- **fail** — ran and failed; cite the failing test/command and output. +- **skipped** — intentionally not run for this change (e.g., no changed C# for + G3); name the gate and the reason. +- **not-run** — could not run (environment/dependency missing); name the + blocker. + +A PR is not "green" if any applicable gate is fail, skipped-without-justification, +or not-run. Never infer a pass from silence. + +## Adding or changing a gate + +- Any new executable check goes into `scripts/governance-check.sh` **and** the + `architecture-quality` workflow together (or into the full CI for G4/G5-type + checks). One source of truth for local and CI. +- A new rule documents its meaning in `ARCHITECTURE_AND_CODE_QUALITY.md` and its + command/mapping here. +- Explicit, documented exceptions only — no wildcard suppressions (see + `ARCHITECTURE_AND_CODE_QUALITY.md` §10). diff --git a/REVIEW_AND_PR_FRAMEWORK.md b/REVIEW_AND_PR_FRAMEWORK.md new file mode 100644 index 0000000..04b70dc --- /dev/null +++ b/REVIEW_AND_PR_FRAMEWORK.md @@ -0,0 +1,132 @@ +# 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. + +## 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. diff --git a/scripts/governance-check.sh b/scripts/governance-check.sh new file mode 100755 index 0000000..4175287 --- /dev/null +++ b/scripts/governance-check.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# +# governance-check.sh — local/CI parity governance gate for the Seahaven backend. +# +# Runs G1 restore, G2 ArchitectureTests, G3 changed-file formatting, G4 Release +# build, and G5 full tests exactly as the `architecture-quality` workflow does. +# A green run here means the same thing locally and in that CI workflow. +# +# Usage: +# bash scripts/governance-check.sh +# BASE_REF=origin/dev bash scripts/governance-check.sh +# BASE_REF= HEAD_REF= bash scripts/governance-check.sh +set -euo pipefail + +SOLUTION="SeaHavenIndustries.sln" +ARCH_TEST_PROJECT="Api.SeaHavenIndustries.Tests/Api.SeaHavenIndustries.Tests.csproj" + +log() { printf '\n\033[1m== %s ==\033[0m\n' "$1"; } +ok() { printf '\033[32mPASS\033[0m %s\n' "$1"; } +bad() { printf '\033[31mFAIL\033[0m %s\n' "$1"; } + +if [[ -n "${DOTNET_BIN:-}" ]]; then + DOTNET="$DOTNET_BIN" +elif command -v dotnet >/dev/null 2>&1; then + DOTNET="$(command -v dotnet)" +elif [[ -x "$HOME/.dotnet/dotnet" ]]; then + DOTNET="$HOME/.dotnet/dotnet" +else + bad "dotnet is unavailable; set DOTNET_BIN or install the repository SDK." + exit 1 +fi + +# Comparison point for changed-file formatting. Default to the dev integration +# branch locally; CI overrides BASE_REF/HEAD_REF with the PR base/head SHAs. +BASE_REF="${BASE_REF:-origin/dev}" +HEAD_REF="${HEAD_REF:-HEAD}" + +# Resolve the base ref before using it for a diff. +if ! git rev-parse --verify --quiet "${BASE_REF}^{commit}" >/dev/null; then + bad "G3: BASE_REF '${BASE_REF}' does not resolve to a commit (run: git fetch origin)." + exit 1 +fi + +log "G1: restore" +"$DOTNET" restore "$SOLUTION" +ok "G1: restore" + +log "G2: architecture boundary tests (dependency direction)" +"$DOTNET" test "$ARCH_TEST_PROJECT" \ + --no-restore \ + --filter "FullyQualifiedName~ArchitectureTests" \ + --nologo +ok "G2: ArchitectureTests" + +log "G3: changed-file formatting (${BASE_REF}..${HEAD_REF})" +changed_cs=() +while IFS= read -r f; do + changed_cs+=("$f") +done < <( + git diff --name-only --diff-filter=ACMR "${BASE_REF}" "${HEAD_REF}" -- '*.cs' +) + +if (( ${#changed_cs[@]} == 0 )); then + printf '\033[33mSKIP\033[0m G3: no changed C# files between %s..%s\n' "${BASE_REF}" "${HEAD_REF}" +else + printf ' checking %d changed C# file(s)\n' "${#changed_cs[@]}" + "$DOTNET" format "$SOLUTION" \ + --no-restore \ + --verify-no-changes \ + --include "${changed_cs[@]}" + ok "G3: changed-file formatting" +fi + +log "G4: Release build" +"$DOTNET" build "$SOLUTION" -c Release --no-restore --nologo +ok "G4: Release build" + +log "G5: full test suite" +"$DOTNET" test "$SOLUTION" -c Release --no-build --nologo +ok "G5: full test suite" + +log "governance-check: all required repository gates passed"