Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ dotnet test Cluckwork.sln # 2295 tests as of 2026-09; integrati
- **Container health probe: the `healthcheck` verb (#266).** Dispatched before host build (no DI, DB or config), it GETs `/health/ready` over loopback and exits `0` only on a 2xx — the hardened image ships no curl/wget, and the probe must never report a false green. → [`266-container-health-probe.md`](docs/decisions/266-container-health-probe.md)
- **Container image hardening (#267).** Runtime stage runs non-root (`USER $APP_UID`), all three base images are digest-pinned, and Trivy fails the build on a fixable HIGH/CRITICAL. Keep the full glibc base per #264. → [`267-container-hardening.md`](docs/decisions/267-container-hardening.md)
- **Seed / simulation data is never boot-seeded (#280, #284, #279).** Run it explicitly against an already-base-seeded, non-Production database: `ASPNETCORE_ENVIRONMENT=Development dotnet Cluckwork.Api.dll seed --profile demo|simulation` (unset env → Production → blocked). The verb migrates, seeds, exits — authoritative and fail-loud. Both profiles require an Owner (#500); `simulation` additionally requires `Simulation:CastPassword` and fails closed without it. → [`280-seed-and-simulation.md`](docs/decisions/280-seed-and-simulation.md) · [runbook](docs/runbooks/simulation-fixture-on-a-dev-database.md) for loading the simulation fixture into a local debug database
- **A write-contract change must update its non-CI callers (#394).** Coverage is not uniform: the seeders are covered only to the handler layer, so a **validator-only** tightening is invisible to every seeder test, and `tools/simulation/k6/` plus the Playwright specs are uncovered while a green baseline hides it. **Verify the request/status contract by reading the callers, not by running.** → [`394-write-contract-callers.md`](docs/decisions/394-write-contract-callers.md)
- **A write-contract change must update its non-CI callers (#394).** Coverage is not uniform, and it is not what it was when this rule was written. The seeders are covered only to the handler layer, so a **validator-only** tightening is invisible to every seeder test. `tools/simulation/k6/` is **genuinely uncovered** — `k6-baseline.yml` is `workflow_dispatch` only, and a tolerated-status list means a green run does not mean a landing write. The **Playwright specs are no longer uncovered**: since 2026-08-08 `e2e-smoke.yml` runs the quick suite on every `pull_request` touching `src/**`, `web/**`, `tools/simulation/**` or `deploy/**` — which is every write-contract change by definition — so CI does catch a broken spec there. A docs-only PR still skips it, and `slow`/`canary` stay dispatch-only. **Verify the request/status contract by reading the callers, not by running** — the reading is what still finds the k6 case and what tells you whether a caller's *intent* changed. → [`394-write-contract-callers.md`](docs/decisions/394-write-contract-callers.md)
- **SPA E2E lives in `tools/simulation/ui/` (#277/#385).** Playwright drives the real built SPA over the same `seed --profile simulation` fixture k6 uses; `web/` stays Vitest. Three enforced rules, each of which has caught something: never hardcode a credential, never hardcode English, respect the farm clock. Anything reasoning about `inert` or the accessibility tree must go through CDP (`src/ax.ts`) — Playwright's own APIs do not model `inert` (#501). → [`277-spa-e2e.md`](docs/decisions/277-spa-e2e.md)
- **Any PR that changes what a user sees attaches screenshots, captured from a stack rebuilt at the head under review (#662).** Not only a PR whose *purpose* is visual: a new dialog, a new field, a changed label or an altered state all qualify, because the trigger is what the reader can no longer check from the diff. **Before and after wherever a before exists**, the same viewport and the same scenario in both; after-only where the UI is net-new and there is nothing to compare against. It is the only check in this repo that reads the *rendered* result: on #661 it found the one product defect that four review seats, three CI Playwright runs and the driver's own verification all passed. Three mechanics matter — capture at 1:1, because downscaling destroys exactly the 1px hairlines and 4–5% alpha shadows a styling change is about; rebuild first, because a long-running sim stack serves the bytes it was built from, not the branch under review; and attach the images to the PR with `gh pr comment --attach` (or `gh pr create --attach`), **never** by committing them to a branch — a branch is deletable, so routine cleanup silently breaks the images on a merged PR while the prose around them still claims they exist.
- **Production logs: compact JSON on stdout; base layer argument-free (#404).** The base `appsettings.json` Console entry carries `Name` only — a template left beside the formatter is **silently** bound instead of it. Trace fields differ by format (`@tr`/`@sp` in compact JSON, `{TraceId}`/`{SpanId}` in the Dev template), and compact JSON serializes **every** property, so anything pushed via `BeginScope`/`LogContext` reaches the collector. → [`404-production-logs.md`](docs/decisions/404-production-logs.md)
Expand Down
34 changes: 33 additions & 1 deletion docs/decisions/394-write-contract-callers.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,36 @@

> **Rule** — the one-paragraph version lives in [`AGENTS.md`](../../AGENTS.md); this file is the relocated rationale (what shipped, why the short version was insufficient, what not to break).

**A change to a write contract must update its non-CI callers in the same PR (#394).** Same failure as #370 with a different trigger: adding a required field, tightening a validator, or changing an aggregate's state machine breaks automated callers of that endpoint that no test references. **Coverage is not uniform, and the gaps do not announce themselves — so work out which layer you changed before trusting any of it.** The **seeders are covered, but only to the handler/domain layer**: `DemoSeedTests`, `SimulationSeederTests`, `SeedCommandTests`, `SimulationSeedCommandTests` and `SimulationCrossDayRerunTests` invoke `DemoDataSeeder`/`SimulationDataSeeder` under `dotnet test`, so a fixture violating a new **handler or aggregate** invariant turns a PR **red**, and `SimulationDataSeeder`'s exact-count validation fails **closed** rather than certifying bad data. **A validator-only tightening is invisible to all five.** The seeders construct commands and call `HandleAsync` **directly** (`DemoDataSeeder` contains not one `ValidateAsync` call) while the endpoint runs `ValidateAsync` *before* the handler — so a fixture payload the real API would now reject keeps every seeder test green. If your change lives in a `*Validator`, the seeders prove nothing; push the rule into the handler/aggregate, or accept that CI is silent here. What is **uncovered outright** is `tools/simulation/k6/` and the Playwright specs in `tools/simulation/ui/` (`workflow_dispatch` only, #385) — and **do not expect a run to surface it either**: `bundles.js`'s `dailyEntryScreen` passes `[403, 409, 422]` as *tolerated*, `authedPost`'s `check()` counts a tolerated status as a **pass**, and `noteIfUnexpected` is handed `expected.concat(tolerated)` — so a new validator answering 422 makes the write workload stop landing entirely against a **100% green baseline**. `reset.sh` never runs k6 at all; it boots and seeds and stops. **So verify the request and status contract by reading it, not by running something.** Grep for callers of the endpoint you changed, check each payload still satisfies the new rule, and narrow the tolerated-status list if a status that used to mean "constrained write" now means "broken write". Read what each caller *does* first: `manager.spec.ts` records **and submits**, while `worker.spec.ts` deliberately **saves a draft and never submits** (its rerunnability contract depends on that), so a submit-contract change must not "fix" the worker spec into submitting. Treat a write-contract PR that leaves the uncovered callers behind like a missing test.
**A change to a write contract must update its non-CI callers in the same PR (#394).** Same failure as #370 with a different trigger: adding a required field, tightening a validator, or changing an aggregate's state machine breaks automated callers of that endpoint that no test references. **Coverage is not uniform, and the gaps do not announce themselves — so work out which layer you changed before trusting any of it.** The **seeders are covered, but only to the handler/domain layer**: `DemoSeedTests`, `SimulationSeederTests`, `SeedCommandTests`, `SimulationSeedCommandTests` and `SimulationCrossDayRerunTests` invoke `DemoDataSeeder`/`SimulationDataSeeder` under `dotnet test`, so a fixture violating a new **handler or aggregate** invariant turns a PR **red**, and `SimulationDataSeeder`'s exact-count validation fails **closed** rather than certifying bad data. **A validator-only tightening is invisible to all five.** The seeders construct commands and call `HandleAsync` **directly** (`DemoDataSeeder` contains not one `ValidateAsync` call) while the endpoint runs `ValidateAsync` *before* the handler — so a fixture payload the real API would now reject keeps every seeder test green. If your change lives in a `*Validator`, the seeders prove nothing; push the rule into the handler/aggregate, or accept that CI is silent here. What is **uncovered outright** is `tools/simulation/k6/` (`k6-baseline.yml`, `workflow_dispatch` only) — and **do not expect a run to surface it either**: `bundles.js`'s `dailyEntryScreen` passes `[403, 409, 422]` as *tolerated*, `authedPost`'s `check()` counts a tolerated status as a **pass**, and `noteIfUnexpected` is handed `expected.concat(tolerated)` — so a new validator answering 422 makes the write workload stop landing entirely against a **100% green baseline**. `reset.sh` never runs k6 at all; it boots and seeds and stops. **So verify the request and status contract by reading it, not by running something.** Grep for callers of the endpoint you changed, check each payload still satisfies the new rule, and narrow the tolerated-status list if a status that used to mean "constrained write" now means "broken write". Read what each caller *does* first: `manager.spec.ts` records **and submits**, while `worker.spec.ts` deliberately **saves a draft and never submits** (its rerunnability contract depends on that), so a submit-contract change must not "fix" the worker spec into submitting. Treat a write-contract PR that leaves the uncovered callers behind like a missing test.

## Amendment, 2026-09-11: the Playwright half stopped being true

This rule was written when **both** `tools/simulation/k6/` and the Playwright specs in
`tools/simulation/ui/` were dispatch-only, and the paragraph above said so. Half of that is no longer
the case, and the half that changed is the half a reader is most likely to act on.

`e2e-smoke.yml` runs the quick suite **on `pull_request`** — an owner call dated 2026-08-08, recorded
in that workflow's own header — path-filtered to `src/**`, `web/**`, `tools/simulation/**`,
`deploy/**`, the three root image-build inputs and the workflow file. A write-contract change lives in
`src/` by definition, so **the Playwright specs do run for exactly the class of change this rule
governs**. Roughly 28 specs across the 14 files, in about three minutes. Only `slow` (the real
15-minute token-expiry spec) and `canary` remain dispatch-only, and a docs-only PR skips the job.

`k6-baseline.yml` is still `workflow_dispatch` only. Everything the paragraph above says about the
tolerated-status list hiding a broken write is unchanged and still the sharpest trap here.

**Why this is worth correcting rather than leaving.** The error pointed the safe way — it made you
verify by hand something CI already covered — so nothing shipped broken because of it. The cost was
misdirected effort: on #727 the Playwright half of the caller read was done by hand under the belief
that a green baseline would hide a break, when CI would have caught it. It also flattened the one
caller that genuinely still needs reading into a list of two, which makes it easier to skip.

It is a trap in the other direction too. Anyone who checks `e2e-smoke.yml`, sees Playwright covered,
and concludes the whole bullet is stale will stop reading the k6 callers as well.

**The durable form of the rule is the reading, not the coverage claim.** CI can tell you a spec broke.
It cannot tell you a caller's *intent* changed — which is why `manager.spec.ts` records and submits
while `worker.spec.ts` deliberately never submits, and why a submit-contract change must not "fix"
the worker spec into submitting. That part of this decision is untouched.

Found while shipping #727; filed as #767.
Loading