diff options
Diffstat (limited to 'CLAUDE.md')
| -rw-r--r-- | CLAUDE.md | 114 |
1 files changed, 114 insertions, 0 deletions
diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..59b2350 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,114 @@ +# FeeDreader — Agent Instructions + +Personal Android RSS/Atom reader, `org.terst.feedreader`. Built for Peter's own use, greenfield, +hands-on-testing-rounds workflow (sideload a debug build, use it, batch feedback). See `BACKLOG.md` +for open work. + +## Test-driven development is mandatory + +Every change to production code (`app/src/main/`) follows red-green-refactor, in that literal order: + +1. **Write the test first**, for the specific behavior you're about to add or change. Not a test + for the whole feature at the end — the test that would fail without the change you're about to + make. +2. **Run it. Confirm it fails, and fails for the right reason** — an assertion mismatch that + demonstrates the missing behavior, not a compile error, not a typo, not a test that was + accidentally already passing. +3. Only now write the minimal production code to make it pass. +4. Run again. Confirm green. +5. Refactor if needed, keeping it green. +6. Run the full quality gate (below) before committing. + +**If it's discovered that production code was written before, or without, a preceding failing +test** — mid-session, in review, by the pre-commit hook, however it surfaces — **the fix is to +discard the whole diff and redo it properly**, test first. Do not backfill tests onto code that +already exists to make the rule look satisfied after the fact: a test written against working code +tends to just confirm what the code already does, not drive the design or catch what the code +missed. `git diff` to see what's uncommitted, stash or note anything worth keeping conceptually, +then `git checkout -- <files>` (or `git reset` for staged-but-uncommitted work) and start over from +a failing test. This applies to every agent working in this repo, not just whoever wrote the +original rule. + +**Scoping exception:** pure resource/config/manifest changes with no meaningful unit-testable +behavior (an XML theme parent, a manifest permission, a Gradle version bump) don't get a red step — +there's nothing to fail. These are instead covered by the static-analysis gate (lint/detekt) acting +as the regression check. If you're unsure whether something falls in this bucket, default to +writing the test. + +## Quality gate — every commit + +Enforced by `.githooks/pre-commit` (see below; run `git config core.hooksPath .githooks` once per +clone to activate it — this is a manual one-time step per checkout, not automatic, since hooks +aren't cloned by default): + +1. **Build** — `./gradlew compileDebugKotlin` must succeed. +2. **Tests** — `./gradlew testDebugUnitTest` must pass in full. +3. **Static analysis / code-quality audit** — `./gradlew detekt lintDebug` must both come back + clean: zero lint errors (warnings are fine to accumulate but should trend down, not up — see + "periodic checks"), zero new detekt findings beyond the tracked baseline. +4. **Coverage must not decrease** — Jacoco line coverage over `app/src/main/java` excluding + `ui/**` (Compose screens are impractical to unit-test for coverage and would make the metric + meaningless — see `app/build.gradle.kts` Jacoco exclusions) is compared against + `.coverage-baseline`. A commit that drops it fails. + - **Justifiable exceptions exist** — e.g. deleting dead code that happened to be covered, + where the deletion is the point (percentage can dip when the denominator shrinks even though + real risk went down). These aren't silently allowed: set + `COVERAGE_EXCEPTION="reason for the drop"` when committing, which the hook logs and lets + through instead of blocking. Don't reach for this to avoid writing a test you were supposed to + write — it's for cases where the drop itself is the correct outcome. + - A commit that *raises* coverage automatically updates `.coverage-baseline` as part of the + hook. +5. **WebView JavaScript justification** — any new or changed `javaScriptEnabled = true` / + `setJavaScriptEnabled(true)` site must carry a `@SuppressLint("SetJavaScriptEnabled")` plus an + inline comment explaining why (matches the existing pattern in `ReaderExtractor.kt`, + `LinkReaderScreen.kt`, `BrowserScreen.kt`). This app's whole security posture around untrusted + article content rests on JS being off almost everywhere — a new JS-enabled surface is exactly + the kind of thing that should never land silently. +6. **Manifest permission diff** — if `AndroidManifest.xml`'s `<uses-permission>` list changed, the + hook prints a warning requiring the commit message to say why. Doesn't block (permissions + sometimes need to land before the feature using them), but makes it impossible to miss in review. +7. **Secret scan** — a lightweight grep for API-key-shaped strings and common credential patterns + across the staged diff. Blocks on a hit; override only if you're certain it's a false positive + (e.g. a test fixture with an obviously-fake key). +8. **Schema-migration test requirement** — if `AppDatabase.kt`'s `version = N` changed, the diff + must also touch a migration test file (Room's `MigrationTestHelper`, run against a real + pre-migration DB). No mechanism currently exists for this (see Task: Room migration tests below) + — until it does, this check is a documented rule, not yet hook-enforced. + +Never bypass this hook with `--no-verify` without asking first, same as the general git-safety +rules — if a check is wrong or too strict for a specific case, fix the check or use the documented +exception mechanism, don't route around it silently. + +## Periodic checks (not per-commit — too heavy, or need a human/agent read, not a mechanical gate) + +- **Dependency freshness** — `./gradlew lintDebug` already reports outdated dependency versions via + its `GradleDependency` check (no extra plugin needed — the `com.github.ben-manes.versions` / + `io.github.ben-manes.versions` plugin was tried during setup and reproducibly broke + `kspDebugKotlin` in this environment, so it's deliberately not used here). Batch-upgrade + periodically from those findings rather than drip-updating one dependency per unrelated commit. +- **Dead-code / reachability sweep** — the kind of audit that found the orphaned `/tabs/meals`-style + routes on doot and the dead DAO methods here: re-run after a significant feature batch, not every + commit. Trace every nav route, DAO method, and setting to a real caller. +- **Room migration correctness** — beyond the per-commit "a test exists" check above, periodically + verify the full migration chain (v1→latest) runs clean against a realistic seeded DB, not just the + latest single-step migration in isolation. +- **Detekt/lint baseline review** — baselines exist so new code doesn't inherit old debt as a blocker, + but a stale baseline just hides debt forever. Periodically review what's in it and pay some down. + +## Why these specific checks + +Every check above traces back to something that actually happened in this codebase, not a generic +checklist: +- The lint zero-errors gate exists because `themes.xml`'s theme parent silently required API 29 + against a minSdk of 26 — a real crash risk on real devices that sat unnoticed until an audit found + it. +- The WebView-JS-justification check exists because this app's core security property (JS off for + untrusted article content) depends entirely on that staying true at every callsite, and one + existing site (`ReaderWebView.kt`) already didn't carry the same justification pattern as the + other three. +- The dead-code sweep exists because three DAO methods (`ArticleDao.delete`, + `FilterRuleDao.getAll`, `ArticleDao.markAllReadForFeed`) were superseded by newer methods and left + behind, undetected until an audit. +- The migration-test requirement exists because `AppDatabase` is on its 11th schema version with + zero migration tests, several of which are one-off hardcoded data repairs — exactly the kind of + change most likely to corrupt a real, non-reproducible production DB if wrong. |