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)