From cbbdaae53d9c82d9882e045ba763b273004c5c8e Mon Sep 17 00:00:00 2001 From: Willem van den Ende Date: Mon, 26 Oct 2026 17:47:04 +0000 Subject: [PATCH] doc: plan for internal code health sensors Records the plan for bringing coverage floors, CRAP scores, credo strict, sobelow, boundary and mutation testing to firehose, framed as the "internal verification" counterpart to the existing Playwright / conn / LiveView tests. Ordered in phases because the fast/push split (make verify + pre-push hook, cheap precommit) is the container the other sensors wire into. Includes the steering principles that made the stickies run work (behavior not state, functional core/imperative shell, delete dead code, leave defensive branches uncovered deliberately) and the deferred decisions (CRAP threshold below 30, boundary.spec.check, Playwright in the Lens pass). --- doc/improve-verification.md | 233 ++++++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 doc/improve-verification.md diff --git a/doc/improve-verification.md b/doc/improve-verification.md new file mode 100644 index 0000000..7f39b5d --- /dev/null +++ b/doc/improve-verification.md @@ -0,0 +1,233 @@ +# Improve verification for firehose + +Plan for bringing the "internal code health" sensor stack to firehose, based on +what worked in stickies (Oct 2026) and +[Velasco's ExCrap blog post](https://www.germanvelasco.com/blog/i-wrote-excrap-without-looking-at-the-code). + +Two kinds of verification (the post's frame): + +1. **External** — is the product what we wanted? Visible: the site, the blogs. + Firehose already has decent coverage here: Playwright suites for the app + (`make playwright*`), controller/LiveView tests. +2. **Internal** — is the codebase healthy and evolving well? Invisible unless + we install **sensors and signals**: coverage, CRAP scores, credo, sobelow, + mutation testing, boundary checks. This plan is about the sensors. + +## Where firehose differs from stickies + +- **Two OTP apps.** `app/` (Phoenix, the Firehose site) and `blogex/` + (publishable library, NimblePublisher-based blog engine, no DB). Every sensor + must run per-app; the root `Makefile` orchestrates (`make test` already runs + both). +- **blogex is a library** (has `package`/`docs` config). Quality bar is + higher; it's also mostly *pure* code (post parsing, link validation, feeds, + SEO) — the ideal mutation-testing target, like stickies' `lib/cardgame/domain`. +- **blogex has no mix aliases at all** (no precommit, no format in aliases). + `app` has a `precommit` alias (compile --warnings-as-errors, deps.unlock, + format, test). +- **A gitea remote already exists** — unlike stickies at the start, a pre-push + hook will actually fire on every push from day one. +- Already present: credo dep in `app` (default config, not tuned/strict); + nothing else yet. No coverage config, no boundary/ex_crap/sobelow/muex in + either app. Current suites green: app 178 tests, blogex 156. +- `AGENTS.md` has a **zero-defects policy** ("never continue when there are + errors") — the fast/push split must be written to fit that policy: Lens + failures are triaged and reported, never ignored (see triage classes below). + +## Phases + +Order matters: the first yak is the *container* the other sensors wire into +(same conclusion as stickies — `pre-push-verification-loop-and-lens-review-practice`). + +### Phase 1 — Fast/push split (the container) + +Keep `precommit` cheap and fast (commit freely); move heavy checks to a +**Lens pass** = `make verify` + `.githooks/pre-push`. + +- [ ] Root `Makefile`: `verify` target that loops over `app` and `blogex`, + running the heavy checks cheapest-first (credo → sobelow → coverage+CRAP). +- [ ] `.githooks/pre-push` + `core.hooksPath=.githooks` (wire into `make`/setup). + Fail loudly with a summary; trivially skippable via `git push --no-verify`. +- [ ] `blogex/mix.exs`: add an `aliases` block with `precommit` + (compile --warnings-as-errors, format, test) so both apps share the loop. +- [ ] AGENTS.md: write the **Lens practice** down — review is a separate pass + with its own context; don't chase hook nits inline while building; file a + yak instead. Add the **triage classes** for Lens failures: + *real bug* (fix inline) / *environment-dependent test* (the test is the + bug — make it deterministic by construction, no ambient state as + fixtures) / *style or design nit* (note or file a yak). Reconcile with + the zero-defects policy: "never continue silently" ≠ "fix nits inline"; + it means report, triage, decide. +- [ ] Watch hook runtime: `app`'s `test` alias drops/creates/migrates the DB + every run. If the pre-push run re-tests, suite time doubles. Consider a + `test.crap` alias reusing an exported coverage run (stickies: + `test --cover --export-coverage default` + `test.coverage` + `crap`). + +### Phase 2 — Coverage floor + exclusions policy + +- [ ] Add `test_coverage: [threshold: N]` to **both** `mix.exs` files + (blogex first — it's pure and should be near-perfect; app likely lower). + Run coverage once to see actual numbers before picking the floor. +- [ ] Make the **exclusions policy decision early**, not later (in stickies + this came late, commit `af12c2d`): exclude framework-owned glue + (`Firehose.Repo`, `ErrorHTML/JSON`, `Telemetry`, `Mailer`) — counting it + inflates the floor without reflecting app behavior. +- [ ] Rule from stickies: the threshold is a **floor, ratcheted up as yaks + land** — never lowered. + +### Phase 3 — Behavior tests for the gaps (the bulk of the work) + +Run per-app coverage, list the uncovered modules, file yaks per area +(stickies: "behavior tests: boundary shell", "…: web LiveViews", "…: +DevToolsPage"). See *Steering the model on missing tests* below for the +principles — this is where most steering happened. + +Expected gaps in firehose, by module type: + +- `blogex/*` — pure library, should be drivable to very high coverage through + behavior tests (post → rendered HTML, invalid links → errors, feed output). +- `app/lib/firehose/accounts*` — context behavior (stickies did this for + MultiTenancy: narrate from the actors' view). +- `firehose_web/live/*` and controllers — outcome tests through + `Phoenix.LiveViewTest` / conn tests, not HTML-string assertions. +- Editorial features (`editor_dashboard_live`, `microprints_live`) — likely the + thinnest coverage; behavior-test them like stickies' DevToolsPage. + +- [ ] Coverage-driven yak list per app; work them one by one. +- [ ] After each yak: **ratchet the floor** up to the new total + small margin. + +### Phase 4 — CRAP scores with ex_crap + +- [ ] Add `{:ex_crap, "~> 0.1", only: [:dev, :test], runtime: false}` to both + apps. **Sandbox gotcha:** `mix deps.get` cannot fetch new hex packages + from inside the sandbox — edit `mix.exs`, then ask the user to run + `mix deps.get` outside; stop there. Same for every dep below. +- [ ] Wire `mix crap` into the Lens pass (needs the coverage export from + Phase 1's `test.crap` alias). +- [ ] Baseline at default `--max-score 30`. Fix offenders the stickies way: + * pure data refactor where the complexity is real logic (9-branch + `case` → compile-time lookup map); + * for shell glue (LiveView `handle_event`), **close coverage gaps + instead of refactoring** — a fully-covered complexity-22 function + scores 24.48, under the default threshold. +- [ ] CRAP formula reminder: `comp^2*(1-cov)^3 + comp`; coverage ≥ ~75% of a + comp-22 function clears threshold 30. + +### Phase 5 — Credo strict, tuned + +- [ ] Tune `.credo.exs` (both apps; blogex has none): **defaults, zero + findings under `--strict` at introduction** — the config is the policy. + Deliberate deviation: disable `Credo.Check.Refactor.CyclomaticComplexity` + in favor of CRAP scores (a wide `case` with good coverage is not a + problem; that was the original itch behind ExCrap). +- [ ] Runs in the Lens pass, not precommit. +- [ ] When strict flags code: refactor along **pure-data lines** (lookup + tables, small pure classifiers, pure helpers that return values) — never + split orchestration/shell code into async calls or scattered processes + just to satisfy a metric (user, 2026-10-10: "I don't want an agent to + arbitrarily break them down, but respect our design principles"). + +### Phase 6 — Sobelow + +- [ ] Add to the Lens pass for `app` (Phoenix surface). blogex is a library + without endpoints; decide whether to run it there anyway — in ex_crap's + own history sobelow caught a `String.to_atom` in a library, so it's not + strictly web-only value. +- [ ] **Fail when sobelow is not installed** (stickies: user explicitly + removed the skip — a silently-skipped check is a lying sensor). +- [ ] Fix findings or add per-check skips *with rationale comments* + (stickies: `Config.CSP` skipped because the plug is invisible to sobelow). + +### Phase 7 — Boundary + +Different shape here than in stickies: + +- The **app/blogex split already enforces the biggest boundary** via the mix + dep (`{:blogex, path: "../blogex"}`): app code can only use blogex's public + API. The library direction is naturally clean. +- [ ] Inside `app`: declare boundaries for `Firehose` (contexts), + `FirehoseWeb` (exports Endpoint/Telemetry), and make **`Firehose.Repo` a + top-level sibling boundary with `deps: []`**, removed from `Firehose`'s + exports — so web→Repo calls become compile-time violations, not + convention (stickies commits `35f2eff`, `337a0c8`). +- [ ] Enable the boundary compiler in `:dev` **and** `:test` so precommit's + `compile --warning-as-errors` gates on violations. Watch for + unclassified-module warnings from test support modules (declare an + ignored top-level boundary for them, as stickies did for `CardgameTest`). +- [ ] External apps whitelist: `[:ecto, :phoenix, :plug, :phoenix_live_view]` + with per-module lists where boundary splits them. +- [ ] *Optional, not used in stickies:* Velasco's `boundary.spec.check` / + `boundary.spec.accept` tasks — an agent-proof sensor that fails when the + boundary definition itself changed without human approval. Cheap to add + once boundaries exist; worth considering given firehose's zero-defects + culture. + +### Phase 8 — Mutation testing (muex) + +- [ ] Add muex (dev-only, `runtime: false`) + `mix mutate` / `mix mutate.fast` + aliases. **Scope tightly to pure code**: `blogex/lib/**` (link_validator, + seo, feed, post parsing) and any pure modules in `app` — stickies scoped + to `lib/cardgame/{domain,events}` and left shell code alone. +- [ ] Curated mutator set: `arithmetic,boolean,comparison,conditional, + function_call,literal,return_value` (excludes macro/use-based mutators — + in ex_crap itself, `use Boundary` removals produced false-positive + survivors). +- [ ] `--fail-at 0`: mutation testing stays a **sensor, not a gate**; documented + false positives (guard `and`↔`or` mutants, removed log lines) are noted, + not chased. +- [ ] Genuine survivors → new tests that pin the behavior (e.g. per-field + error tests pinning chained guards). + +### Phase 9 — Ratchets, decisions, handoff + +- [ ] Ratchet coverage floors after each behavior yak lands. +- [ ] Decide (with the user, don't assume): CRAP `--max-score` below 30? + boundary.spec.check adoption? Playwright in the Lens pass or kept as a + separate external-verification loop? +- [ ] "Landing the plane": after each session with code changes, run + `make precommit` always; run `make verify` (or push, which fires the + hook) before handing off. + +## Steering the model on missing tests (what made stickies work) + +The sensors tell you *where* tests are missing; the steering below is what +made the resulting tests good rather than line-chasing. Recorded from the +stickies session history (Oct 10–12) so it can be pasted into firehose +sessions or this repo's AGENTS.md: + +1. **Behavior, not state.** Narrate tests from the actors' view — "host starts + a team, members join, roles change, members leave" — and assert *outcomes* + (the next actor's experience), not internal struct/assign shapes. Pin + current behavioral decisions explicitly when a test does so (e.g. "same-name + teams cannot both exist"). +2. **Functional core, imperative shell.** Details of persistence (how joining + works internally) stay out of most tests; the test drives the public API of + the core. GenServers/stores get *outcome* tests (persistence failure → state + unchanged; restart → replay), not process-internals tests. +3. **Don't test dead code — delete it.** Coverage gaps that are unreachable + mount branches (plug guarantees the precondition) are removed with a comment + stating the contract, not tested. +4. **Leave genuinely defensive branches uncovered, deliberately.** Store + failure paths only reachable by DB failure, fallbacks the router guarantees + — chasing them with artificial mocks tests lines, not behavior. Note them + in the yak context so the next ratchet isn't confused. +5. **Refactor along pure-data lines when a metric flags code**; never split a + linear shell script into async tasks to satisfy a metric. A flat lookup + `case` that is genuinely clearer than a table may stay — note it, move on. +6. **Don't chase Lens nits inline** while building; file a yak, let the Lens + pass handle them. +7. **Sandbox:** new hex deps need the user to run `mix deps.get` outside the + sandbox (read-only `~/.hex/packages`). Never work around it. + +## Suggested yak list (for `yx` in firehose) + +1. Fast/push split: make verify + pre-push hook + Lens practice in AGENTS.md +2. Coverage floors + framework-glue exclusions policy (both apps) +3. Behavior tests: blogex library (probably 2–3 yaks by area) +4. Behavior tests: accounts context +5. Behavior tests: firehose_web LiveViews/controllers +6. CRAP scores with ex_crap (both apps) +7. Credo strict tuned config (both apps) +8. Sobelow in the Lens pass +9. Boundary: Repo as sibling boundary, web→Repo forbidden +10. Mutation testing with muex (blogex + pure app code)