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).
This commit is contained in:
Willem van den Ende
2026-10-26 17:47:04 +00:00
parent cee64b9f74
commit cbbdaae53d
+233
View File
@@ -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)