mirror of
https://github.com/Sea-Haven-Industries/shoc-frontend-new.git
synced 2026-09-30 06:53:12 +00:00
chore(governance): enforce frontend quality system (#53)
* chore(governance): make React/TS conventions mandatory via executable gates Add AGENTS.md, QUALITY_GATES.md, ARCHITECTURE_AND_CODE_QUALITY.md, and REVIEW_AND_PR_FRAMEWORK.md as the binding conventions and PR review contract for humans and all coding/review agents. Add a single 'npm run verify' command (format + lint + build + test + governance) and 'npm run governance', which runs a dependency-free godfile ratchet (whole-repo, baseline in scripts/governance-baseline.json) and a changed-file maintainability gate (complexity<=20, function<=150, params<=4, depth<=4) via ESLint. Legacy is handled by ratchets, not relaxation: 5 godfiles over 500 lines are grandfathered debt; maintainability thresholds apply to changed TS/TSX (72 legacy violations across ~51 files otherwise). Add a repo-owned 'governance' CI job that runs 'npm run verify' so every gate is guaranteed from this repository, independent of the org reusable workflow. * fix(governance): make frontend ratchets fail closed
This commit is contained in:
parent
33001aac74
commit
4337cc662b
9 changed files with 709 additions and 10 deletions
27
.github/workflows/ci.yaml
vendored
27
.github/workflows/ci.yaml
vendored
|
|
@ -11,6 +11,33 @@ permissions:
|
|||
|
||||
jobs:
|
||||
ci:
|
||||
# Org reusable workflow (Node 24): format check, lint, build, unit tests.
|
||||
uses: Sea-Haven-Industries/.github/.github/workflows/ci-typescript-frontend.yaml@main
|
||||
with:
|
||||
node-version: "24"
|
||||
|
||||
governance:
|
||||
# Repo-owned guarantee that every frontend quality gate runs from this
|
||||
# repository, independent of (and in addition to) the reusable workflow.
|
||||
# `npm run verify` is the single command that chains: format check, lint
|
||||
# (--max-warnings=0), type-check + build, unit tests, then the governance
|
||||
# checks in scripts/governance-check.mjs (godfile ratchet + changed-file
|
||||
# maintainability gate). If the reusable workflow is later confirmed to run
|
||||
# every gate, this job can be slimmed to `npm run governance`.
|
||||
#
|
||||
# GOVERNANCE_BASE points the changed-file gate at the right diff:
|
||||
# PR -> the PR target branch (origin/<base_ref>)
|
||||
# push-> the previous commit on the branch (github.event.before)
|
||||
runs-on: ubuntu-latest
|
||||
env:
|
||||
GOVERNANCE_BASE: ${{ github.event_name == 'pull_request' && format('origin/{0}', github.base_ref) || github.event.before }}
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
fetch-depth: 0
|
||||
- uses: actions/setup-node@v4
|
||||
with:
|
||||
node-version: "24"
|
||||
cache: npm
|
||||
- run: npm ci
|
||||
- run: npm run verify
|
||||
|
|
|
|||
81
AGENTS.md
Normal file
81
AGENTS.md
Normal file
|
|
@ -0,0 +1,81 @@
|
|||
# AGENTS.md — frontend mandatory conventions
|
||||
|
||||
This file is the binding entry point for **every contributor — human or AI coding
|
||||
or review agent** — working in this repository (`seahaven-new-app`, the SHOC
|
||||
frontend). It makes the validated React/TypeScript conventions mandatory and
|
||||
points to the operational documents and executable gates that enforce them.
|
||||
|
||||
Read these before writing or reviewing code. They override generic "best
|
||||
practice" suggestions from any agent or model. This repository contract may
|
||||
strengthen, but never weaken, the workspace-level `AGENTS.md`.
|
||||
|
||||
- [QUALITY_GATES.md](QUALITY_GATES.md) — the executable gates, the single
|
||||
command, and the no-false-pass guarantees.
|
||||
- [ARCHITECTURE_AND_CODE_QUALITY.md](ARCHITECTURE_AND_CODE_QUALITY.md) — the
|
||||
MUST / MUST NOT conventions (conditional rendering, typography, forms, state,
|
||||
data access, security, maintainability) with evidence and thresholds.
|
||||
- [REVIEW_AND_PR_FRAMEWORK.md](REVIEW_AND_PR_FRAMEWORK.md) — the PR review
|
||||
contract (exact-head review, board regression inventory, behavior-based
|
||||
testing, security/performance/Big-O review, no style-only comments).
|
||||
- [docs/FRONTEND_MAINTAINABILITY.md](docs/FRONTEND_MAINTAINABILITY.md) — the
|
||||
detailed rationale for the conditional-rendering and typography rules.
|
||||
|
||||
## One command to run every gate
|
||||
|
||||
```bash
|
||||
npm run verify
|
||||
```
|
||||
|
||||
This chains the full set: Prettier check, ESLint (`--max-warnings=0`), TypeScript
|
||||
build (`tsc -b && vite build`), unit tests (`vitest run`), and the governance
|
||||
checks (`npm run governance`). **Do not claim a task is done until `npm run
|
||||
verify` is green locally.** CI runs the same `npm run verify` in a repo-owned
|
||||
`governance` job, so a green local run mirrors CI.
|
||||
|
||||
## Non-negotiable rules (enforced; do not work around)
|
||||
|
||||
These are already enforced by lint/build or the governance script. Disabling,
|
||||
baselining, or per-line-suppressing them is forbidden (see exceptions below).
|
||||
|
||||
- **No one-sided `cond ? <Element/> : null`** — use `&&` or the `when` prop.
|
||||
- **The left operand of `&&` in JSX must be entirely boolean** — coerce with
|
||||
`Boolean(...)` / an explicit comparison; `{count && ...}` is rejected.
|
||||
- **Shared `Text` for `p` / `h1`–`h6` / error typography** — raw `<p>`/`<h*>` and
|
||||
the `vp-error` class outside `Text` are rejected.
|
||||
- **Zero lint warnings** — `--max-warnings=0` makes a warning a failure; fix it,
|
||||
do not silence it.
|
||||
- **Hooks correctness** — the `react-hooks` recommended rules (including
|
||||
`exhaustive-deps`) run under the zero-warnings gate.
|
||||
- **Godfile ratchet** — no source file may exceed 500 lines. Existing named debt
|
||||
has a frozen per-file cap that may only decrease.
|
||||
- **Changed-file maintainability** — changed TS/TSX must meet
|
||||
`complexity ≤ 20`, function `≤ 150` lines, `≤ 4` params, `≤ 4` depth.
|
||||
|
||||
## How to add / change a convention
|
||||
|
||||
1. Land it **green**: a new or tightened rule must ship with the codebase passing
|
||||
it (a migration in the same change), not as a warning-only backlog.
|
||||
2. If legacy would break, use changed-file enforcement or migrate it. Do not add
|
||||
new baseline debt or raise a frozen cap.
|
||||
3. Document the rule, its threshold, and its evidence in
|
||||
[ARCHITECTURE_AND_CODE_QUALITY.md](ARCHITECTURE_AND_CODE_QUALITY.md).
|
||||
|
||||
## Exceptions and the ADR process
|
||||
|
||||
Exceptions are **not granted by disabling a rule inline**. To deviate:
|
||||
|
||||
1. Record an **Architecture Decision Record** under `docs/adr/`
|
||||
(`NNNN-title.md`: context, decision, consequences, alternatives).
|
||||
2. Existing grandfathered entries may only be removed or have their caps
|
||||
reduced. New entries and cap increases fail the governance gate.
|
||||
3. Get it reviewed like any other change. The ADR + baseline entry is the
|
||||
auditable record; an `eslint-disable` comment is not.
|
||||
|
||||
## Toolchain (do not change without an ADR)
|
||||
|
||||
React 19, TypeScript 6, Vite, Tailwind 4 + MUI, TanStack Query, React Hook Form +
|
||||
Zod, Ky. Node ≥ 22.22.1 (CI runs Node 24); npm 11.16.0 (pinned via
|
||||
`packageManager`, invoked through corepack). **No new dependencies without an
|
||||
ADR** — prefer the libraries already established (see
|
||||
[ARCHITECTURE_AND_CODE_QUALITY.md](ARCHITECTURE_AND_CODE_QUALITY.md) §Forms and
|
||||
data access).
|
||||
125
ARCHITECTURE_AND_CODE_QUALITY.md
Normal file
125
ARCHITECTURE_AND_CODE_QUALITY.md
Normal file
|
|
@ -0,0 +1,125 @@
|
|||
# ARCHITECTURE_AND_CODE_QUALITY.md — MUST / MUST NOT conventions
|
||||
|
||||
Mandatory React/TypeScript conventions for this repository. Each rule lists how
|
||||
it is enforced. Detailed rationale for rendering and typography lives in
|
||||
[docs/FRONTEND_MAINTAINABILITY.md](docs/FRONTEND_MAINTAINABILITY.md). When a rule
|
||||
says "MUST", it is enforced by lint, build, or `npm run governance`; "SHOULD"
|
||||
means it is a review-enforced convention backed by the
|
||||
[PR review contract](REVIEW_AND_PR_FRAMEWORK.md).
|
||||
|
||||
## Conditional rendering
|
||||
|
||||
- **MUST** use `&&` or the `Text` `when` prop for one-sided conditions; never
|
||||
`cond ? <Element/> : null`. (ESLint `no-restricted-syntax`.)
|
||||
- **MUST** make the left operand of `&&` in JSX entirely boolean. Coerce presence
|
||||
with `Boolean(value)` (or `Boolean(a || b)`); preserve numeric/empty-string
|
||||
semantics and TypeScript narrowing with `count > 0`, `value != null`.
|
||||
`{count && <X/>}` renders `0` and is rejected by the type-aware
|
||||
`seahaven/no-non-boolean-jsx-and` rule. (See FRONTEND_MAINTAINABILITY.md for
|
||||
the approved guard forms.)
|
||||
- **MUST NOT** widen typed element-slot props (`icon`, `action`, `actions`) back
|
||||
to `ReactNode`; coerce with `Boolean(prop)` before `&&`.
|
||||
|
||||
## Typography and feedback
|
||||
|
||||
- **MUST** use `Text` from `@/components/ui/text` for paragraphs, headings,
|
||||
descriptions, labels, captions, code, and async feedback. Raw `<p>` and
|
||||
`<h1>`–`<h6>` are rejected. (ESLint `no-restricted-syntax`.)
|
||||
- **MUST** use `Text variant="error"` for the `vp-error` styling; the
|
||||
`vp-error` class on any non-`Text` element is rejected
|
||||
(`seahaven/no-vp-error-outside-text`). Do not compose `vp-error` dynamically to
|
||||
bypass the static check.
|
||||
- **MUST NOT** pass `component`, `role`, or `aria-live` to `Text` — they are
|
||||
omitted from `TextProps` so the variant contract (element, tone, live region)
|
||||
cannot be overridden. Use `as` and `tone`.
|
||||
- **SHOULD** keep the `feedback` (polite `status`) region mounted and toggle text
|
||||
with `when`; mount `error` (assertive `alert`) on demand.
|
||||
|
||||
## Forms and mutations
|
||||
|
||||
- **MUST** use the libraries already established: **React Hook Form** for field
|
||||
registration/lifecycle, **Zod** for validation and inferred types, **TanStack
|
||||
Query** for server reads and mutations (pending/error state, invalidation,
|
||||
retries).
|
||||
- **MUST NOT** add **TanStack Form** alongside React Hook Form for one form. A
|
||||
second form convention increases cognitive and dependency cost. Replacing
|
||||
React Hook Form requires an approved repo-wide migration ADR with measured
|
||||
benefits, a codemod/migration plan, and removal of the superseded dependency.
|
||||
- **SHOULD** keep file upload selection/validation in a focused component and
|
||||
drive upload progress/errors/retry through a TanStack Query mutation, not in a
|
||||
route-sized page.
|
||||
|
||||
## State ownership (bounded)
|
||||
|
||||
- **MUST** keep query loading/error/empty state adjacent to the query result;
|
||||
mutation pending/error state in the component that initiated the mutation.
|
||||
- **MUST** compose focused state components instead of accumulating unrelated
|
||||
booleans in a page. Route pages coordinate sections and navigation; reusable
|
||||
sections own their interaction details.
|
||||
- **SHOULD** render errors inline with accessible feedback (`Text
|
||||
variant="error"`); reserve toasts for cross-page outcomes.
|
||||
|
||||
## Data access
|
||||
|
||||
- **MUST** keep TanStack Query keys **stable and descriptive** (a consistent
|
||||
entity + identity tuple, co-located with the query). Unstable or ad-hoc keys
|
||||
break caching and invalidation. (Review-enforced; a future lint rule is
|
||||
tracked as a gap.)
|
||||
- **MUST NOT** materialize large server collections into client state and then
|
||||
filter/sort them in the component when the server (or a memoized, virtualized
|
||||
layer) should own it. Prefer server-side filtering/pagination; if client-side
|
||||
is required, memoize and avoid re-filtering on every render. (Review-enforced.)
|
||||
- **MUST** invalidate the right query keys after a mutation so caches do not
|
||||
show stale data.
|
||||
|
||||
## Security
|
||||
|
||||
- **MUST NOT** surface raw server error payloads, stack traces, or internal
|
||||
identifiers to end users. Map server errors to a safe user-facing message
|
||||
(e.g. `Text variant="error"`); log the full detail only to controlled channels.
|
||||
(Review-enforced.)
|
||||
- **MUST NOT** bake secrets, tokens, or account-specific values into the build.
|
||||
Only `VITE_*` build-time vars are allowed, and they are baked into the bundle —
|
||||
never put a secret in a `VITE_` variable.
|
||||
|
||||
## Hooks, performance, and Big-O
|
||||
|
||||
- **MUST** satisfy `react-hooks` recommended rules (including `exhaustive-deps`)
|
||||
under the zero-warnings gate.
|
||||
- **SHOULD** avoid `O(n²)` or worse work inside render/hot paths; memoize derived
|
||||
data, key lists stably, and prefer server-side filtering for large sets
|
||||
(reviewers flag algorithmic complexity — see REVIEW_AND_PR_FRAMEWORK.md).
|
||||
- **SHOULD** keep renders pure; derive expensive values with `useMemo`/`useCallback`
|
||||
only when measured to matter (no speculative memoization).
|
||||
|
||||
## Maintainability (measured thresholds)
|
||||
|
||||
Enforced by `npm run governance` ([QUALITY_GATES.md](QUALITY_GATES.md)).
|
||||
These are conservative code-shape proxies for review focus. File length is not
|
||||
proof of a god object, cyclomatic complexity is not runtime Big-O, and neither
|
||||
substitutes for behavior tests, profiling, query-plan evidence, or reviewer
|
||||
judgment.
|
||||
|
||||
| Metric | Threshold | How enforced |
|
||||
| --------------------- | ----------- | ------------------------------------------------------------------------------------------ |
|
||||
| File length (godfile) | ≤ 500 lines | Whole-repo ratchet: any file over the cap not in `scripts/governance-baseline.json` fails. |
|
||||
| Cyclomatic complexity | ≤ 20 | Changed-file ESLint `complexity`. |
|
||||
| Function length | ≤ 150 lines | Changed-file ESLint `max-lines-per-function` (`skipComments`). |
|
||||
| Parameters | ≤ 4 | Changed-file ESLint `max-params`. |
|
||||
| Nesting depth | ≤ 4 | Changed-file ESLint `max-depth`. |
|
||||
|
||||
**Why these are changed-file ratchets, not whole-repo errors:** measured against
|
||||
the current codebase, the maintainability thresholds surface **72 violations
|
||||
across ~51 legacy files** (45 function-length, 24 complexity, 3 params), and
|
||||
**5 files exceed 500 lines**. Applying them repo-wide at `error` would block
|
||||
legacy without a migration. Instead:
|
||||
|
||||
- The **5 oversized files** are explicit grandfathered debt with frozen
|
||||
per-file caps in `scripts/governance-baseline.json`. They may shrink but never
|
||||
grow; new entries and cap increases fail.
|
||||
- The **complexity/function-length/params/depth** rules apply to **changed**
|
||||
TS/TSX on a PR/push, so new and modified code must comply while untouched
|
||||
legacy is not blocked. Bring legacy into compliance when you next touch it.
|
||||
|
||||
These thresholds are starting ratchets: tighten them (lower caps, fewer baseline
|
||||
entries) as debt is paid down. Never raise them to make a change pass.
|
||||
54
QUALITY_GATES.md
Normal file
54
QUALITY_GATES.md
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
# QUALITY_GATES.md — executable frontend gates
|
||||
|
||||
The single command that runs **every** gate, locally and in CI:
|
||||
|
||||
```bash
|
||||
npm run verify
|
||||
```
|
||||
|
||||
`verify` chains: `format:check` → `lint` → `build` (`tsc -b && vite build`) →
|
||||
`test` (`vitest run`) → `governance`. A task is not done until this is green.
|
||||
|
||||
## Gate matrix
|
||||
|
||||
| Gate | Command / rule source | Enforced by | Scope |
|
||||
| ----------------------------------- | ----------------------------------------------------------------------------------------------------------- | ---------------------- | ------------------------------------ |
|
||||
| Formatting | `npm run format:check` (Prettier) | `verify` + lint-staged | Whole repo |
|
||||
| Lint, zero warnings | `npm run lint` → `eslint . --max-warnings=0` | `verify` + CI | Governed TS/TSX (`eslint.config.js`) |
|
||||
| Type-check + production build | `npm run build` → `tsc -b && vite build` | `verify` + CI | Whole app |
|
||||
| Unit tests | `npm test` → `vitest run` | `verify` + CI | `src/test/**`, `config/**/*.test.ts` |
|
||||
| Conditional rendering (no `: null`) | `no-restricted-syntax` in `eslint.config.js` | lint | Governed TSX |
|
||||
| Boolean-only JSX `&&` | `seahaven/no-non-boolean-jsx-and` (type-aware) in `eslint-rules/` | lint | Governed TSX |
|
||||
| Shared `Text` typography | `no-restricted-syntax` (raw `p`/`h1`–`h6`) + `seahaven/no-vp-error-outside-text` | lint | Governed TSX |
|
||||
| Hooks correctness | `eslint-plugin-react-hooks` recommended (incl. `exhaustive-deps`) under zero-warnings | lint | Governed TS/TSX |
|
||||
| Godfile ratchet (file length) | `scripts/governance-check.mjs` + `scripts/governance-baseline.json` | `governance` | `src/**`, `config/**` (non-test) |
|
||||
| Changed-file maintainability | `scripts/governance-check.mjs` → ESLint (`complexity`, `max-lines-per-function`, `max-params`, `max-depth`) | `governance` | Changed TS/TSX vs base ref |
|
||||
|
||||
## No-false-pass guarantees
|
||||
|
||||
- **`--max-warnings=0`** — a warning is a failure. There is no "warning-only"
|
||||
backlog; rules ship green (see AGENTS.md → How to add a convention).
|
||||
- **Type-aware rules fail closed** — `seahaven/no-non-boolean-jsx-and` reports
|
||||
when type services are unavailable rather than silently claiming safety.
|
||||
- **Godfile ratchet is monotonic** — any new file over the cap, new baseline
|
||||
entry, global cap increase, per-file cap increase, or growth beyond a frozen
|
||||
legacy cap fails. Only cap reductions and entry removals are allowed.
|
||||
- **Changed-file maintainability fails closed without a valid base** — in CI the
|
||||
base ref is derived from `GITHUB_BASE_REF` (PR) or `github.event.before`
|
||||
(push). An absent or unresolvable base is a failure, not a pass.
|
||||
|
||||
## Where the gates run
|
||||
|
||||
- **Locally:** `npm run verify`. `lint-staged` (via Husky) re-runs ESLint +
|
||||
Prettier on staged files at commit; commitlint enforces Conventional Commits.
|
||||
- **CI ([`.github/workflows/ci.yaml`](.github/workflows/ci.yaml)):** the org
|
||||
reusable workflow (`ci-typescript-frontend.yaml`, Node 24) runs
|
||||
format/lint/build/tests, **and** a repo-owned `governance` job runs
|
||||
`npm run verify` so the maintainability ratchets are guaranteed from this
|
||||
repository regardless of the reusable workflow.
|
||||
|
||||
## Toolchain pin
|
||||
|
||||
Node ≥ 22.22.1 (CI uses Node 24); npm 11.16.0 via `packageManager` (use
|
||||
`corepack npm …` if your default `npm` is older). The lockfile is
|
||||
`package-lock.json` v3; install with `npm ci`.
|
||||
28
README.md
28
README.md
|
|
@ -95,15 +95,17 @@ The dev proxy expects the `shoc-backend` API at `http://localhost:5141`;
|
|||
override with `VITE_API_TARGET` (e.g. `https://api.dev.seahaven.com` to use
|
||||
the deployed dev API).
|
||||
|
||||
| Command | Description |
|
||||
| ------------------------------------------ | ----------------------------------------------------- |
|
||||
| `npm run dev` | Start Vite dev server on port 3000 |
|
||||
| `npm run build` | Type-check (`tsc -b`) and production build to `dist/` |
|
||||
| `npm run preview` | Preview the production build locally |
|
||||
| `npm test` / `npm run test:watch` | Vitest unit tests (once / watch) |
|
||||
| `npm run test:e2e` / `npm run test:e2e:ui` | Playwright e2e tests (headless / UI mode) |
|
||||
| `npm run lint` / `npm run lint:fix` | ESLint (check / auto-fix) |
|
||||
| `npm run format` / `npm run format:check` | Prettier (write / check) |
|
||||
| Command | Description |
|
||||
| ------------------------------------------ | -------------------------------------------------------- |
|
||||
| `npm run dev` | Start Vite dev server on port 3000 |
|
||||
| `npm run build` | Type-check (`tsc -b`) and production build to `dist/` |
|
||||
| `npm run preview` | Preview the production build locally |
|
||||
| `npm test` / `npm run test:watch` | Vitest unit tests (once / watch) |
|
||||
| `npm run test:e2e` / `npm run test:e2e:ui` | Playwright e2e tests (headless / UI mode) |
|
||||
| `npm run lint` / `npm run lint:fix` | ESLint (check / auto-fix) |
|
||||
| `npm run format` / `npm run format:check` | Prettier (write / check) |
|
||||
| `npm run governance` | Frontend governance checks (godfile + maintainability) |
|
||||
| `npm run verify` | **All gates**: format + lint + build + test + governance |
|
||||
|
||||
Husky + lint-staged run ESLint and Prettier on staged files at commit;
|
||||
commitlint enforces conventional commit messages. Run `npx tsc --noEmit` (or
|
||||
|
|
@ -132,7 +134,13 @@ CI/CD uses the org's reusable workflows (no stored AWS keys — OIDC only):
|
|||
- **CI** ([`.github/workflows/ci.yaml`](.github/workflows/ci.yaml)) — on push
|
||||
and PRs to `main`/`dev`, calls
|
||||
`Sea-Haven-Industries/.github` → `ci-typescript-frontend.yaml` (Node 24):
|
||||
format check, lint, build, tests.
|
||||
format check, lint, build, tests; **and** runs a repo-owned `governance` job
|
||||
that calls `npm run verify` so every gate (including the maintainability
|
||||
ratchets in [`scripts/governance-check.mjs`](scripts/governance-check.mjs)) is
|
||||
guaranteed from this repository. Conventions and gates are documented under
|
||||
[`AGENTS.md`](AGENTS.md), [`QUALITY_GATES.md`](QUALITY_GATES.md),
|
||||
[`ARCHITECTURE_AND_CODE_QUALITY.md`](ARCHITECTURE_AND_CODE_QUALITY.md), and
|
||||
[`REVIEW_AND_PR_FRAMEWORK.md`](REVIEW_AND_PR_FRAMEWORK.md).
|
||||
- **CD** ([`.github/workflows/deploy.yml`](.github/workflows/deploy.yml)) — on
|
||||
push to `dev`, calls `Sea-Haven-Industries/.github` → `cd-cdk.yaml`, which
|
||||
runs `cdk deploy` on `infra/cdk` (stack `shoc-frontend-dev`, `us-east-1`)
|
||||
|
|
|
|||
81
REVIEW_AND_PR_FRAMEWORK.md
Normal file
81
REVIEW_AND_PR_FRAMEWORK.md
Normal file
|
|
@ -0,0 +1,81 @@
|
|||
# REVIEW_AND_PR_FRAMEWORK.md — PR review contract
|
||||
|
||||
Every PR is reviewed against this contract, by humans and by review agents. The
|
||||
goal is high-signal review: catch real behavior, security, performance, and
|
||||
regression problems — not re-lint what the gates already enforce.
|
||||
|
||||
## 1. Review the exact head
|
||||
|
||||
- **MUST** review the diff at the **current PR head**, not a stale checkout.
|
||||
Re-pull before reviewing if new commits landed; stale approvals are dismissed
|
||||
on push by branch protection.
|
||||
- **MUST** read the full diff of every changed file, including renames and
|
||||
generated/mapper code, not only the "interesting" components.
|
||||
|
||||
## 2. Board-backed regression inventory
|
||||
|
||||
- **MUST** check the change against the **Seahaven Jira SH board** inventory —
|
||||
all visible tickets for this area, not only the current ticket. Behavior that
|
||||
is Done / QA-approved / released is **protected scope**.
|
||||
- **MUST** treat a plausible regression against protected behavior as a
|
||||
**Blocker** until disproven with repo evidence (the accepting tests, the linked
|
||||
PR/release, and a targeted check of the changed code paths).
|
||||
- **MUST NOT** approve if board access or the relevant inventory is missing —
|
||||
say so and block rather than infer a pass. A missing ticket link alone is not
|
||||
a blocker, but known acceptance criteria must still be traced.
|
||||
|
||||
The current Jira workflow has no `Ready for QA` transition. Merged work remains
|
||||
in the documented pre-QA status until QA evidence supports `Done`; do not invent
|
||||
a status or mark unverified work Done.
|
||||
|
||||
## 3. Behavior-based testing
|
||||
|
||||
- **MUST** test through **public behavior** (rendered output, user interactions,
|
||||
query/mutation outcomes), not internal implementation details. Prefer
|
||||
`@testing-library` queries and user-event flows; assert what users observe.
|
||||
- **MUST NOT** add tests whose only purpose is to prove a tool (ESLint, the
|
||||
governance script) executes — validate tooling by running the real gates
|
||||
(`npm run verify`), not with assertion-free unit tests.
|
||||
- **SHOULD** cover the meaningful branches of new logic: the happy path, the
|
||||
error/empty state, and any boundary the change introduces.
|
||||
|
||||
## 4. Security review
|
||||
|
||||
- **MUST** confirm no raw server error payloads, stack traces, or internal IDs
|
||||
leak to the UI (see ARCHITECTURE_AND_CODE_QUALITY.md §Security).
|
||||
- **MUST** confirm no secrets/tokens are introduced into the build, and that no
|
||||
`VITE_*` variable carries a secret (it is baked into the bundle).
|
||||
- **SHOULD** check untrusted input is validated (Zod) before use and that
|
||||
dangerously-set HTML / unescaped server strings are not introduced.
|
||||
|
||||
## 5. Performance and Big-O review
|
||||
|
||||
- **MUST** flag algorithmic regressions in hot paths: `O(n²)`+ loops over server
|
||||
collections, re-filtering/sorting on every render, unbounded list rendering
|
||||
without virtualization.
|
||||
- **MUST** confirm TanStack Query keys are stable and that mutations invalidate
|
||||
the correct keys (no stale cache, no redundant refetch storms).
|
||||
- **SHOULD** question speculative `useMemo`/`useCallback` (add when measured) and
|
||||
unstable identities passed to memoized children.
|
||||
|
||||
## 6. No style-only comments
|
||||
|
||||
- **MUST NOT** leave comments that only restate what Prettier or ESLint already
|
||||
enforces (formatting, naming nits the linter catches). Style is settled by the
|
||||
gates; review is for behavior, correctness, security, and architecture.
|
||||
- **MUST** make every comment actionable: tie it to a behavior, a risk, or an
|
||||
evidence-based convention in these docs, and offer a concrete fix or a
|
||||
targeted question. Use GitHub suggestion blocks when safe.
|
||||
|
||||
## 7. Review close-out
|
||||
|
||||
A review is complete when it records, briefly:
|
||||
|
||||
1. Findings ordered by severity (Blocker / Needs-change / Suggestion), or
|
||||
"no findings".
|
||||
2. Open questions and their owner.
|
||||
3. Which of the above checks were run, and any that were skipped (and why).
|
||||
4. Residual risk, if approving.
|
||||
|
||||
Do not write a monolithic review body or a validation transcript into the PR
|
||||
surface; keep comments inline and high-signal.
|
||||
|
|
@ -15,6 +15,8 @@
|
|||
"lint:fix": "eslint . --fix --max-warnings=0",
|
||||
"format": "prettier --write .",
|
||||
"format:check": "prettier --check .",
|
||||
"governance": "node scripts/governance-check.mjs",
|
||||
"verify": "npm run format:check && npm run lint && npm run build && npm test && npm run governance",
|
||||
"prepare": "husky"
|
||||
},
|
||||
"lint-staged": {
|
||||
|
|
|
|||
32
scripts/governance-baseline.json
Normal file
32
scripts/governance-baseline.json
Normal file
|
|
@ -0,0 +1,32 @@
|
|||
{
|
||||
"version": 1,
|
||||
"purpose": "Frozen grandfathered debt inventory for the frontend governance checks. New entries and cap increases fail the governance comparison. Existing caps may only decrease and entries must be removed when compliant.",
|
||||
"maxFileLines": 500,
|
||||
"godfileDebt": [
|
||||
{
|
||||
"path": "src/app/(protected)/workorders/[id].tsx",
|
||||
"maxLines": 797,
|
||||
"reason": "Work-order detail route page; legacy godfile targeted for decomposition into focused state components."
|
||||
},
|
||||
{
|
||||
"path": "src/app/(protected)/workorders/_components/dispatch-detail-modal.tsx",
|
||||
"maxLines": 782,
|
||||
"reason": "Dispatch detail modal; oversized legacy component pending extraction of sections/fields."
|
||||
},
|
||||
{
|
||||
"path": "src/app/v/[token]/dispatch/[id].tsx",
|
||||
"maxLines": 651,
|
||||
"reason": "Vendor-portal dispatch detail route; legacy page pending decomposition."
|
||||
},
|
||||
{
|
||||
"path": "src/app/(protected)/vendors/index.tsx",
|
||||
"maxLines": 612,
|
||||
"reason": "Vendors list route; legacy page with mixed query/filter/table state pending extraction."
|
||||
},
|
||||
{
|
||||
"path": "src/domain/work-orders/mappers/work-order-mapper.ts",
|
||||
"maxLines": 572,
|
||||
"reason": "API<->domain mapper; long but cohesive mapping logic, pending split by responsibility."
|
||||
}
|
||||
]
|
||||
}
|
||||
289
scripts/governance-check.mjs
Normal file
289
scripts/governance-check.mjs
Normal file
|
|
@ -0,0 +1,289 @@
|
|||
import { execFileSync, spawnSync } from "node:child_process";
|
||||
import { readFileSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url));
|
||||
const ROOT = path.resolve(SCRIPT_DIR, "..");
|
||||
const BASELINE_PATH = path.join(SCRIPT_DIR, "governance-baseline.json");
|
||||
|
||||
const MAX_FILE_LINES = 500;
|
||||
const MAINTAINABILITY_RULES = [
|
||||
'complexity: ["error", { "max": 20 }]',
|
||||
'max-lines-per-function: ["error", { "skipComments": true, "max": 150 }]',
|
||||
'max-params: ["error", 4]',
|
||||
'max-depth: ["error", 4]',
|
||||
];
|
||||
const GOVERNED_ROOTS = ["src/", "config/"];
|
||||
const EXCLUDE_DIR = /(^|\/)(mocks|test|__mocks__|node_modules|dist|coverage|e2e)\//;
|
||||
const EXCLUDE_NAME = /\.(mock|test|spec)\.(ts|tsx)$|\.d\.ts$/;
|
||||
|
||||
function isGoverned(relativePath) {
|
||||
return (
|
||||
GOVERNED_ROOTS.some((root) => relativePath.startsWith(root)) &&
|
||||
/\.(ts|tsx)$/.test(relativePath) &&
|
||||
!EXCLUDE_DIR.test(relativePath) &&
|
||||
!EXCLUDE_NAME.test(relativePath)
|
||||
);
|
||||
}
|
||||
|
||||
function gitText(args) {
|
||||
return execFileSync("git", args, { cwd: ROOT, encoding: "utf8" }).trim();
|
||||
}
|
||||
|
||||
function gitLines(args) {
|
||||
return gitText(args).split("\n").filter(Boolean);
|
||||
}
|
||||
|
||||
function governedFiles() {
|
||||
const tracked = gitLines(["ls-files"]);
|
||||
const untracked = gitLines(["ls-files", "--others", "--exclude-standard"]);
|
||||
return [...new Set([...tracked, ...untracked])].filter(isGoverned);
|
||||
}
|
||||
|
||||
function lineCount(relativePath) {
|
||||
const content = readFileSync(path.join(ROOT, relativePath), "utf8");
|
||||
if (content.length === 0) return 0;
|
||||
return content.endsWith("\n") ? content.split("\n").length - 1 : content.split("\n").length;
|
||||
}
|
||||
|
||||
function readBaseline() {
|
||||
return JSON.parse(readFileSync(BASELINE_PATH, "utf8"));
|
||||
}
|
||||
|
||||
function readBaselineAtRef(ref) {
|
||||
try {
|
||||
const content = execFileSync("git", ["show", `${ref}:scripts/governance-baseline.json`], {
|
||||
cwd: ROOT,
|
||||
encoding: "utf8",
|
||||
stdio: ["ignore", "pipe", "ignore"],
|
||||
});
|
||||
return JSON.parse(content);
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
function godfileRatchet(baseRef) {
|
||||
const baseline = readBaseline();
|
||||
const cap = baseline.maxFileLines ?? MAX_FILE_LINES;
|
||||
const debtEntries = new Map(
|
||||
(baseline.godfileDebt ?? []).map((entry) => [entry.path, entry.maxLines]),
|
||||
);
|
||||
const files = governedFiles();
|
||||
|
||||
const newDebt = [];
|
||||
const grownDebt = [];
|
||||
for (const file of files) {
|
||||
const lines = lineCount(file);
|
||||
const debtCap = debtEntries.get(file);
|
||||
if (lines > cap && debtCap === undefined) {
|
||||
newDebt.push({ path: file, lines });
|
||||
} else if (debtCap !== undefined && lines > debtCap) {
|
||||
grownDebt.push({ path: file, lines, maxLines: debtCap });
|
||||
}
|
||||
}
|
||||
|
||||
const stale = [];
|
||||
const remaining = [];
|
||||
for (const [debtPath, maxLines] of debtEntries) {
|
||||
const lines = files.includes(debtPath) ? lineCount(debtPath) : -1;
|
||||
if (lines === -1 || lines <= cap) {
|
||||
stale.push({ path: debtPath, lines });
|
||||
} else {
|
||||
remaining.push({ path: debtPath, lines, maxLines });
|
||||
}
|
||||
}
|
||||
|
||||
const baselineLoosening = [];
|
||||
const baseBaseline = baseRef ? readBaselineAtRef(baseRef) : null;
|
||||
if (baseBaseline) {
|
||||
const baseCap = baseBaseline.maxFileLines ?? MAX_FILE_LINES;
|
||||
if (cap > baseCap) {
|
||||
baselineLoosening.push(`global cap increased from ${baseCap} to ${cap}`);
|
||||
}
|
||||
const baseEntries = new Map(
|
||||
(baseBaseline.godfileDebt ?? []).map((entry) => [entry.path, entry.maxLines]),
|
||||
);
|
||||
for (const [debtPath, maxLines] of debtEntries) {
|
||||
const priorMax = baseEntries.get(debtPath);
|
||||
if (priorMax === undefined) {
|
||||
baselineLoosening.push(`new debt entry: ${debtPath}`);
|
||||
} else if (maxLines > priorMax) {
|
||||
baselineLoosening.push(`cap increased for ${debtPath}: ${priorMax} -> ${maxLines}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
cap,
|
||||
newDebt,
|
||||
grownDebt,
|
||||
stale,
|
||||
remaining,
|
||||
baselineLoosening,
|
||||
comparedBaseline: Boolean(baseBaseline),
|
||||
};
|
||||
}
|
||||
|
||||
function resolveBaseRef() {
|
||||
if (process.env.GOVERNANCE_BASE) return process.env.GOVERNANCE_BASE;
|
||||
if (process.env.GITHUB_BASE_REF) return `origin/${process.env.GITHUB_BASE_REF}`;
|
||||
for (const candidate of ["origin/dev", "origin/main"]) {
|
||||
try {
|
||||
execFileSync("git", ["rev-parse", "--verify", candidate], {
|
||||
cwd: ROOT,
|
||||
encoding: "utf8",
|
||||
stdio: "ignore",
|
||||
});
|
||||
return candidate;
|
||||
} catch {
|
||||
// candidate ref not present locally; try the next
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
function changedGovernedFiles(baseRef) {
|
||||
let mergeBase;
|
||||
try {
|
||||
mergeBase = execFileSync("git", ["merge-base", baseRef, "HEAD"], {
|
||||
cwd: ROOT,
|
||||
encoding: "utf8",
|
||||
stdio: ["ignore", "pipe", "ignore"],
|
||||
}).trim();
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
const diffed = gitLines(["diff", "--name-only", "--diff-filter=AMR", mergeBase, "HEAD"]);
|
||||
const untracked = gitLines(["ls-files", "--others", "--exclude-standard"]);
|
||||
return [...new Set([...diffed, ...untracked])].filter(isGoverned);
|
||||
}
|
||||
|
||||
function maintainabilityGate(files) {
|
||||
if (files.length === 0) {
|
||||
return { skipped: true, reason: "no changed governed TS/TSX files" };
|
||||
}
|
||||
const eslintBin = path.join(ROOT, "node_modules", ".bin", "eslint");
|
||||
const ruleArgs = MAINTAINABILITY_RULES.flatMap((rule) => ["--rule", rule]);
|
||||
const result = spawnSync(
|
||||
eslintBin,
|
||||
[
|
||||
...files,
|
||||
...ruleArgs,
|
||||
"--max-warnings=0",
|
||||
"--no-warn-ignored",
|
||||
"--no-error-on-unmatched-pattern",
|
||||
],
|
||||
{ cwd: ROOT, encoding: "utf8" },
|
||||
);
|
||||
return {
|
||||
skipped: false,
|
||||
status: result.status,
|
||||
stdout: result.stdout?.trim() ?? "",
|
||||
stderr: result.stderr?.trim() ?? "",
|
||||
files,
|
||||
};
|
||||
}
|
||||
|
||||
function plural(count, word) {
|
||||
return `${count} ${word}${count === 1 ? "" : "s"}`;
|
||||
}
|
||||
|
||||
function main() {
|
||||
const failures = [];
|
||||
const baseRef = resolveBaseRef();
|
||||
if (!baseRef) {
|
||||
failures.push(
|
||||
"base ref is required but was not found. Set GOVERNANCE_BASE to a valid commit or fetch origin/dev.",
|
||||
);
|
||||
} else {
|
||||
try {
|
||||
gitText(["merge-base", baseRef, "HEAD"]);
|
||||
} catch {
|
||||
failures.push(
|
||||
`base ref '${baseRef}' cannot be resolved against HEAD. Fetch it or set GOVERNANCE_BASE correctly.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
console.log("─".repeat(64));
|
||||
console.log("godfile ratchet: legacy caps may only shrink");
|
||||
const god = godfileRatchet(baseRef);
|
||||
console.log(
|
||||
` cap: ${god.cap} lines | grandfathered debt: ${plural(god.remaining.length, "file")} | new violations: ${god.newDebt.length}`,
|
||||
);
|
||||
for (const entry of god.remaining) {
|
||||
console.log(` debt ${String(entry.lines).padStart(4)}/${entry.maxLines} ${entry.path}`);
|
||||
}
|
||||
for (const entry of god.newDebt) {
|
||||
console.log(` NEW ${String(entry.lines).padStart(4)} ${entry.path}`);
|
||||
}
|
||||
for (const entry of god.grownDebt) {
|
||||
console.log(` GREW ${String(entry.lines).padStart(4)}/${entry.maxLines} ${entry.path}`);
|
||||
}
|
||||
if (god.newDebt.length > 0) {
|
||||
failures.push(
|
||||
`godfile ratchet: ${plural(god.newDebt.length, "file")} exceed ${god.cap} lines. Refactor them under the cap; new baseline debt is forbidden.`,
|
||||
);
|
||||
}
|
||||
if (god.grownDebt.length > 0) {
|
||||
failures.push(
|
||||
`godfile ratchet: ${plural(god.grownDebt.length, "grandfathered file")} exceeded its frozen line cap.`,
|
||||
);
|
||||
}
|
||||
if (god.baselineLoosening.length > 0) {
|
||||
failures.push(
|
||||
`governance baseline was loosened: ${god.baselineLoosening.join("; ")}. Only cap reductions and entry removals are allowed.`,
|
||||
);
|
||||
}
|
||||
if (!god.comparedBaseline) {
|
||||
console.log(" baseline comparison unavailable (initial adoption or missing base file)");
|
||||
}
|
||||
if (god.stale.length > 0) {
|
||||
console.log(` stale baseline entries (now compliant — remove to ratchet tighter):`);
|
||||
for (const entry of god.stale) {
|
||||
console.log(` stale ${entry.path}`);
|
||||
}
|
||||
}
|
||||
|
||||
console.log("─".repeat(64));
|
||||
if (!baseRef) {
|
||||
console.log("changed-file maintainability gate: FAIL (no valid base ref)");
|
||||
} else {
|
||||
const files = changedGovernedFiles(baseRef);
|
||||
console.log(
|
||||
`changed-file maintainability gate (base: ${baseRef}): ${files === null ? "unresolvable" : plural(files.length, "changed governed file")}`,
|
||||
);
|
||||
if (files === null) {
|
||||
console.log(" failed — base ref could not be resolved against HEAD");
|
||||
} else {
|
||||
const gate = maintainabilityGate(files);
|
||||
if (gate.skipped) {
|
||||
console.log(` skipped — ${gate.reason}`);
|
||||
} else {
|
||||
const clean = gate.status === 0;
|
||||
console.log(
|
||||
` result: ${clean ? "PASS" : "FAIL"} (complexity<=20, function<=150 lines, params<=4, depth<=4)`,
|
||||
);
|
||||
if (!clean) {
|
||||
if (gate.stdout) console.log(gate.stdout);
|
||||
if (gate.stderr) console.log(gate.stderr);
|
||||
failures.push(
|
||||
"changed-file maintainability gate: see ESLint output above. Extract functions/components to meet the thresholds; do not relax the thresholds.",
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
console.log("─".repeat(64));
|
||||
if (failures.length > 0) {
|
||||
console.log(`RESULT: FAIL (${plural(failures.length, "gate")})`);
|
||||
for (const failure of failures) console.log(` - ${failure}`);
|
||||
process.exit(1);
|
||||
}
|
||||
console.log("RESULT: PASS — all governance gates green");
|
||||
}
|
||||
|
||||
main();
|
||||
Loading…
Add table
Reference in a new issue