feedsignal/DEFINITION_OF_DONE.md

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

• 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.
• 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 TODOs 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.