mirror of
https://github.com/Sea-Haven-Industries/shoc-backend.git
synced 2026-09-30 06:03:12 +00:00
163 lines
8.9 KiB
Markdown
163 lines
8.9 KiB
Markdown
# 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` locally. The `architecture` job in `ci.yml`
|
||
> calls that script (skipping G4/G5, which the `Build and test` job owns). Do
|
||
> not add a check to CI that is not also runnable locally through the script
|
||
> (and vice versa), except G4/G5 which the local script still runs.
|
||
|
||
## Gate inventory
|
||
|
||
| # | Gate | Protects (architecture §) | Local command | CI step |
|
||
|---|------|---------------------------|---------------|---------|
|
||
| G1 | Restore | build integrity | `dotnet restore SeaHavenIndustries.sln` | `ci.yml` `architecture` → `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` | `ci.yml` `Build and test` (local script still runs it) |
|
||
| G5 | Full test suite | behavior | `dotnet test SeaHavenIndustries.sln -c Release --no-build` | `ci.yml` `Build and test` (local script still runs it) |
|
||
| 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.yml` `Build and test` |
|
||
| G9 | Board-backed regression | review framework | `REVIEW_AND_PR_FRAMEWORK.md` inventory | review-enforced |
|
||
| G10 | Terraform import plan safety | live infrastructure adoption | `python scripts/test-terraform-import-plan-check.py` | `ci.yml` `architecture` → `governance-check.sh` |
|
||
| G11 | Terraform static validation | import configuration integrity | commands below | `ci.yml` `architecture` → `governance-check.sh` |
|
||
| G13 | App/Terraform isolation | separate delivery lanes | `python3 scripts/check_app_terraform_isolation.py` | `ci.yml` `architecture` (live classifier: PR + local) |
|
||
|
||
## How to run locally
|
||
|
||
```bash
|
||
# Complete local repository gate (G1–G5, G10, G11, G13):
|
||
bash scripts/governance-check.sh
|
||
|
||
# By default it compares against the default branch for changed-file formatting.
|
||
# Override the comparison point:
|
||
BASE_REF=origin/main 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/main`) 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).
|
||
6. verifies that the Terraform plan guard rejects create, delete, replacement,
|
||
unmanaged resource types, and updates not allowlisted by exact address (G10).
|
||
7. runs the release-tag, commit-check, and isolation unit tests.
|
||
8. runs Terraform fmt/validate for `live/dev` and `live/staging` (G11).
|
||
9. rejects a diff that contains both `terraform/` and deployable application
|
||
files (G13). Live G13 skips on `merge_group` and `push`. The architecture
|
||
job in CI sets `GOVERNANCE_SKIP_BUILD_TEST=1` so steps 4–5 run only locally
|
||
and in the Build and test job.
|
||
|
||
G10 permits only exact approved resource address/type pairs for the
|
||
environment-owned boundary: Elastic
|
||
Beanstalk environment, IAM role/inline policy/managed-policy attachment/
|
||
instance profile, Secrets Manager secret metadata, and Route 53 record.
|
||
SSM `/shoc-backend/<env>/deploy/*` parameters are created after adoption and
|
||
are not part of the import allowlist. Initial mode permits no update.
|
||
Controlled mode requires one
|
||
`--allow-update-address` argument per reviewed in-place update. Every invocation
|
||
also requires `--environment dev` or `--environment staging`; an empty or
|
||
incomplete environment plan fails.
|
||
|
||
G11 runs `terraform fmt -check -recursive terraform`, `terraform init -backend=false
|
||
-input=false -lockfile=readonly`, and `terraform validate` for both `live/dev`
|
||
and `live/staging` from `governance-check.sh` on every CI event, including
|
||
merge groups. Org-baseline CloudFormation owns the HCP role substrate, and Terraform
|
||
owns the environment GitHub deploy roles, so no backend CDK or bootstrap root
|
||
remains in the matrix. HCP plan/apply roles stay in org-baseline; this
|
||
repository never manages `hcptf-*` roles.
|
||
|
||
The former G12 version-only HCP apply guard is not part of the repository gate.
|
||
|
||
G13 fails when the same diff contains both `terraform/` and deployable
|
||
application files. Workflow, documentation, and gate-script changes may share
|
||
a PR with either side.
|
||
|
||
## 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 local script proves G1–G5, G10, G11, and G13 ran (G4/G5 are skipped
|
||
in the CI architecture job). 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` job in `ci.yml` together (or into the Build and test job for
|
||
G4/G5-type checks). One source of truth for local and CI. The required
|
||
merge-queue check is `ci-complete`.
|
||
- 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).
|