From 22d0e4f5374c22873f957b938d9630c4161575c6 Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 3 Sep 2026 16:42:24 +0200 Subject: [PATCH 1/2] Add Definition of Done Codifies the quality bar already implicit in the CI gates and existing code patterns (crate-boundary SRP, per-domain db modules, test comment style) so it's explicit and reviewable rather than tribal knowledge. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PqTJmazHBQK878vjQ4JnnF --- DEFINITION_OF_DONE.md | 76 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 DEFINITION_OF_DONE.md diff --git a/DEFINITION_OF_DONE.md b/DEFINITION_OF_DONE.md new file mode 100644 index 0000000..f26494b --- /dev/null +++ b/DEFINITION_OF_DONE.md @@ -0,0 +1,76 @@ +# Definition of Done — feedsignal + +A change is done when all of the following hold, not just when it compiles. + +## Single Responsibility + +- Each module/function does one thing. If a PR description needs "and" to + describe what a function does, split it. +- Crate boundaries stay honest: `core` has no I/O (see `crates/core` — + domain models + scoring/affinity math only); `db` only talks to SQLite; + `feeds` only fetches/parses; `llm` only talks to Ollama. A change that + makes `core` reach for a DB handle or `db` parse RSS is a sign the code + landed in the wrong crate. +- Within `db`, one file per domain concern (`feeds.rs`, `articles.rs`, + `reading_events.rs`, `affinities.rs`) — a new table gets its own file, + not a growing `queries.rs`. + +## DRY, but not premature + +- No copy-pasted logic across call sites — extract a shared function once + a second real call site exists, not in anticipation of one. +- No new abstraction (trait, generic helper, config knob) unless at least + two concrete cases need it today. + +## Testing + +- New logic in `core` (scoring, affinity math) ships with unit tests — + that crate is the one place pure functions make this cheap, and it's + already the pattern (`crates/core/src/affinity.rs`). +- Test rationale goes in a `///` doc comment above `#[test] fn`, not + inline in the body. +- A bug fix includes a regression test that fails on the old code. +- `cargo test --workspace --exclude feedsignal-web` passes locally before + pushing (CI runs this; `feedsignal-web` itself currently has no test + target). + +## Static checks (all of CI, not a subset) + +- `cargo fmt --check` clean. +- `cargo clippy --workspace --exclude feedsignal-web --all-targets -- -D warnings` clean. +- `cargo clippy -p feedsignal-web --no-default-features --features server --all-targets -- -D warnings` clean. +- `cargo clippy -p feedsignal-web --no-default-features --features web --target wasm32-unknown-unknown -- -D warnings` clean. +- `cargo audit` clean, or a documented reason if it can't be (e.g. an + unfixed transitive advisory). + +## No half-finished work + +- No `TODO`s standing in for a decision the PR itself was supposed to + make (a `TODO` marking genuinely deferred, ticketed follow-up work is + fine — see `LlmConfig`'s config-story TODO). +- No dead code kept "just in case" — if it's unused, delete it. Library- + surface exceptions (e.g. `dioxus-primitives`-derived components) get an + explicit `#[allow(dead_code)]` with a one-line reason, matching the + existing pattern in `components/button/component.rs`. +- No commented-out code left in. + +## Correctness at the boundary + +- New `#[server]` fns validate/trim input the way `services::feeds::subscribe` + does (`anyhow::ensure!` on empty URL) — don't trust the browser. +- DB writes that must be atomic with respect to a failure path (e.g. + "fetch before writing so a bad URL leaves no half-registered row") get + a comment explaining why, like `subscribe`'s doc comment does. +- Migrations are additive/reversible — no editing a shipped migration file. + +## Docs + +- A non-obvious "why" (a workaround, a chosen threshold, a design + tradeoff) gets a comment; "what" is left to the code. +- `README.md`'s architecture section gets updated if the change adds a + crate, a pipeline stage, or a scoring input. + +## PR hygiene + +- Diff is scoped to the stated task — no drive-by refactors bundled in. +- `Cargo.lock` changes match what the dependency change actually requires. -- 2.45.2 From 4846ab3bd626090ec34c616ef8b7df9fbfd8cc53 Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 3 Sep 2026 16:55:52 +0200 Subject: [PATCH 2/2] Broaden Definition of Done testing rule beyond core Require unit tests for non-trivial logic project-wide (db query/filter logic, non-passthrough server::services code), not just core's pure functions, per feedback that the original scope was too narrow. Thin passthrough wrappers stay exempt, and genuinely DB/network-bound cases must be called out explicitly rather than silently skipped. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PqTJmazHBQK878vjQ4JnnF --- DEFINITION_OF_DONE.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/DEFINITION_OF_DONE.md b/DEFINITION_OF_DONE.md index f26494b..c3113ad 100644 --- a/DEFINITION_OF_DONE.md +++ b/DEFINITION_OF_DONE.md @@ -24,9 +24,16 @@ A change is done when all of the following hold, not just when it compiles. ## Testing -- New logic in `core` (scoring, affinity math) ships with unit tests — - that crate is the one place pure functions make this cheap, and it's - already the pattern (`crates/core/src/affinity.rs`). +- All non-trivial logic ships with unit tests wherever practical — not + just `core`'s pure functions (scoring, affinity math — the existing + pattern in `crates/core/src/affinity.rs`), but query/filter logic in + `db` and non-passthrough logic in `web`'s `server::services`. "Non-trivial" + excludes thin wrappers (a `#[server]` fn or service function that's just + `db.some_query(args).await?` with no branching of its own) and glue code + whose only failure mode is a compiler error. +- When a DB/network dependency makes a case genuinely impractical to unit + test (e.g. a Diesel query's SQL correctness), say so in the PR rather + than silently skipping it — that's a real gap to track, not a pass. - Test rationale goes in a `///` doc comment above `#[test] fn`, not inline in the body. - A bug fix includes a regression test that fails on the old code. -- 2.45.2