mirror of
https://github.com/Sea-Haven-Industries/shoc-backend.git
synced 2026-10-06 04:12:14 +00:00
chore(governance): enforce backend architecture and quality gates (#31)
* docs(governance): add canonical governance docs, unified quality-gate script, and CI parity - ARCHITECTURE_AND_CODE_QUALITY.md: canonical layering, EF allowlist, transaction/commit convention, cancellation, migrations, error disclosure, Big-O/perf, ADR exceptions (supersedes BACKEND_ARCHITECTURE.md) - QUALITY_GATES.md: gate inventory + pass/fail/skip semantics; local==CI - REVIEW_AND_PR_FRAMEWORK.md: exact-head review, board-backed regression inventory, security/perf evidence, ADR exceptions, no godfile theater - AGENTS.md: repo-specific delta + precedence pointers - scripts/governance-check.sh: unified G1 restore + G2 ArchitectureTests + G3 changed-file format (portable bash) - .github/workflows/architecture-quality.yml: call the same local script - ArchitectureTests.cs: add business-service interface-dependency invariant * fix(governance): make backend gate complete * fix(ci): enforce governance on every pull request
This commit is contained in:
Alexandre Brandizzi
GitHub
parent
7d245eb717
commit
833fb816ee
7 changed files with 709 additions and 28 deletions
34
.github/workflows/architecture-quality.yml
vendored
34
.github/workflows/architecture-quality.yml
vendored
|
|
@ -2,7 +2,6 @@ name: Architecture and changed-file quality
|
||||||
|
|
||||||
on:
|
on:
|
||||||
pull_request:
|
pull_request:
|
||||||
branches: [main, dev]
|
|
||||||
|
|
||||||
permissions:
|
permissions:
|
||||||
contents: read
|
contents: read
|
||||||
|
|
@ -21,31 +20,10 @@ jobs:
|
||||||
with:
|
with:
|
||||||
dotnet-version: "8.0.x"
|
dotnet-version: "8.0.x"
|
||||||
|
|
||||||
- name: Restore
|
# CI and local run the same complete repository gate.
|
||||||
run: dotnet restore SeaHavenIndustries.sln
|
- name: Repository quality gate
|
||||||
|
|
||||||
- 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
|
|
||||||
shell: bash
|
shell: bash
|
||||||
run: |
|
env:
|
||||||
mapfile -t files < <(
|
BASE_REF: ${{ github.event.pull_request.base.sha }}
|
||||||
git diff --name-only --diff-filter=ACMR \
|
HEAD_REF: ${{ github.event.pull_request.head.sha }}
|
||||||
"${{ github.event.pull_request.base.sha }}" \
|
run: bash scripts/governance-check.sh
|
||||||
"${{ 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[@]}"
|
|
||||||
|
|
|
||||||
70
AGENTS.md
Normal file
70
AGENTS.md
Normal file
|
|
@ -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.
|
||||||
273
ARCHITECTURE_AND_CODE_QUALITY.md
Normal file
273
ARCHITECTURE_AND_CODE_QUALITY.md
Normal file
|
|
@ -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<T>`, `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<T>` 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.
|
||||||
|
|
@ -73,4 +73,25 @@ public class ArchitectureTests
|
||||||
violations.Should().BeEmpty(
|
violations.Should().BeEmpty(
|
||||||
"business services must depend on data-service interfaces and typed options, never on DbContext or IConfiguration");
|
"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");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
125
QUALITY_GATES.md
Normal file
125
QUALITY_GATES.md
Normal file
|
|
@ -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 <changed .cs>` | 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).
|
||||||
132
REVIEW_AND_PR_FRAMEWORK.md
Normal file
132
REVIEW_AND_PR_FRAMEWORK.md
Normal file
|
|
@ -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.
|
||||||
82
scripts/governance-check.sh
Executable file
82
scripts/governance-check.sh
Executable file
|
|
@ -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=<base-sha> HEAD_REF=<head-sha> 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"
|
||||||
Loading…
Add table
Reference in a new issue