Privacy-Period-Tracker/docs/architecture/GUARDS.md

211 lines
8.9 KiB
Markdown
Raw Normal View History

chore: adopt the project template and add the Kotlin/Compose skeleton Period was a bare directory holding one 2,527-line specification, with no git repository, no tracker and no documentation convention. This is the adoption from Projects/Template/START-HERE-New-Project.md, plus a project that compiles so the hooks and future guards have something real to run against. Documents. scaffold.sh created 19 paths, 0 skipped. The specification moved to docs/planning/PRODUCT_PLAN.md unchanged in substance, with a status header; the capitalised Docs/ is gone. Every scaffolded document was filled in for Period. docs/OPERATIONS.md deleted — an offline app is not a deployed service. DOC_TRUST_MAP.md written last, describing what is actually here, including what this project deliberately does not have. Code. Four Gradle modules. domain/cycle and domain/prediction are kotlin("jvm") and cannot see the Android SDK, so the engine is testable without an emulator — 17 tests pass, 12 of them the acceptance cases from PRODUCT_PLAN.md §51. BaselinePredictionEngine is a robust-median prototype and explicitly not the product; it exists so Batch 02's replacement can be shown to be better rather than merely different. Versions verified against their official sources today rather than inherited from the specification's own numbers, which that document asks for: Kotlin 2.4.10, AGP 9.3.1, Gradle 9.7.0, Compose BOM 2026.08.00, Room 2.8.4, Hilt 2.60.1. AGP 9 ships Kotlin built in, so org.jetbrains.kotlin.android is no longer applied. compileSdk is 37 because current AndroidX requires it; targetSdk stays 36, Play's floor from 2026-08-31, and the difference is deliberate. Six scripts taken into scripts/; the rest declined and named in docs/TOOLS.md. Three hooks in .githooks/, with pre-commit adapted to Gradle. closes #1 closes #2
2026-08-18 02:16:47 -05:00
# Guards — how to write a check that actually checks
```
Status: Current
Owner: _null
Last reviewed: 2026-08-18
Governs: structural tests, source-grep assertions, probes, and any check whose
passing is taken as evidence
Review trigger: A guard is found to have been passing while the thing it guards
was broken; a new class of check is added to the suite.
```
A guard that cannot fail is worse than no guard, because it is trusted. Every
rule here was learned by finding one that had been green for months over
something broken.
## 1. Prove the guard fails before you believe it passes
The one discipline that matters most, and it takes thirty seconds:
```bash
cp src/lib/thing.ts /tmp/thing.bak
# break exactly the thing the test protects
sed -i 's/if (body.error)/if (false)/' src/lib/thing.ts
npx vitest run tests/thing.test.ts # expect: exactly one failure
cp /tmp/thing.bak src/lib/thing.ts
npx vitest run tests/thing.test.ts # expect: green again
```
**Exactly one** is the part people skip. If breaking the guard's target fails
three tests, two of them are coincidental and will mask a real regression later.
If it fails none, the guard is decoration — and you have just learned that for
the price of one `sed`.
`scripts/prove-guard.sh` performs exactly this, which removes the two ways it
gets skipped: the restore is a `trap`, so an interrupted run cannot leave the
code broken, and the failure count comes from the runner's own summary rather
than from eyeballing red — one failing test is routinely reported on half a
dozen lines, and counting those calls a clean result six coincidental
failures.
Do this when you write a guard, and again when you change what it guards. A
test written alongside the code it tests has never been observed failing.
## 2. A source-grep guard must tell code from the comment about code
Structural tests that assert a file does *not* contain some pattern will match
the docblock explaining why that pattern is forbidden. So the clearest possible
comment breaks the test, and the obvious fix is to delete the explanation.
Strip comments first:
```ts
const codeOf = (path: string) =>
readFileSync(path, "utf8")
.split("\n")
.filter((line) => !/^\s*(\*|\/\/|\{\/\*)/.test(line))
.join("\n");
expect(codeOf("src/lib/thing.ts")).not.toContain("dangerouslySetInnerHTML");
```
Otherwise the guard quietly punishes documenting the rule it exists to enforce —
which is exactly backwards, because the comment is how the next person learns
the rule at all.
## 3. Pin the behaviour, not the spelling
A guard should fail when the protected behaviour breaks and stay quiet
otherwise. One that asserts on a variable name fails on a rename that changed
nothing.
```ts
// Brittle: breaks when the variable is renamed, while the fallback it protects
// is untouched.
expect(route).toContain("readAsset(project.forgejoRepo");
// Pins the behaviour: the route fetches through the wrapper that tries both
// spellings, and never through the raw reader.
expect(route).toMatch(/readAsset\(\s*\w+,\s*ASSETS\[which\]\s*\)/);
expect(body).not.toContain("readFileBytes(");
```
A guard that fails on changes it does not care about is one people learn to edit
rather than heed, and the edit is usually deletion.
## 4. A negative result is only as good as the probe that produced it
"The check found nothing" and "the check did not run" are different facts, and
they look identical from the outside. Before reporting an absence, prove the
instrument worked:
```bash
# Not this alone — an unreadable file produces the same silence as an unset key
grep -c '^WANTED=' /proc/$PID/environ
# Establish the read succeeded first
tr '\0' '\n' < /proc/$PID/environ | grep -c . # 0 here means "could not read"
```
This is the confident-absence failure one level up: the same trap as a screen
rendering a failed query as a count of zero, applied to your own diagnosis.
## 5. A guard that is often wrong is worse than none
A check with a high false-positive rate trains everybody to skip its output,
including on the day it is right.
One written for this template flagged **684 of 1142** candidates on its first
run. That was not 684 findings, it was a broken heuristic — and shipping it
would have taught its readers that the check is noise. Two rounds of narrowing
brought it to 17 of 363, all of them real.
If a new guard's first run is loud, tune it until it is quiet before anybody
relies on it. Report the false-positive rate you settled at, so the next person
knows what silence is worth.
## 6. Guards belong before the artifact exists
A check that runs after publication catches the problem once it is somewhere it
cannot be taken back from: the tag is in the registry, and refusing the commit
afterwards leaves git with no record of it.
Order the gates so the expensive, irreversible step is last — preconditions,
guards, build, verify the built thing is what was asked for, publish, and record
it last of all.
## 7. When the gate finds something that invalidates the operation, stop
Printing a warning and continuing produces the worst outcome available: the bad
thing happens *and* a reassuring summary appears above it.
The question is not how bad the finding is. It is **whether it invalidates what
the operation claims**:
- A release whose test gate skipped half the suite — a release claims to be
tested. **Refuse.**
- A backup written to a group-readable directory — the backup is still a
backup. **Warn.**
Escape hatches are fine, and they have to be asked for by name, never be the
default, and say plainly what is being given up.
fix: a throw in a background flow no longer kills the process The application scope in PeriodApplication was built with SupervisorJob and no CoroutineExceptionHandler, and ReminderCoordinator launchIns two Room flows on it. SupervisorJob stops a failing child cancelling its siblings; it does not stop the exception, which reaches the thread's default handler and ends the process. That scope is the one that runs with nobody watching. Application.onCreate runs in every process, including the ones WorkManager starts after a reboot and at the daily reminder — no Activity, no screen, nothing to show an error. Both ViewModels already install a handler; the one place a crash is invisible did not. The trigger is real rather than theoretical: repository.forecast runs the prediction engine inside the flow, and Prediction's init block enforces its window invariants with require. Three layers, outermost last: - ReminderCoordinator catches per chain, so one failing collection cannot take the other down. Doing nothing on failure is deliberate — cancelling the schedule would turn a failed read into reminders silently switched off until the user next touched a notification setting. - ReminderWorker returns success and posts nothing when it cannot read what it needs, which is already its behaviour with no history. Cancellation is rethrown rather than swallowed. - The scope handler is a backstop whose only job is that the process lives. It cannot log: checkNoHealthLogging covers this module, and an exception message here can carry a date derived from a cycle. The chains moved into internal functions taking flows so the catch is reachable from a test. CycleRepository is final with an internal constructor, which is right for a data boundary and wrong for faking, and adding a mocking library to reach one catch would have been the worse trade. Proved to fail, per GUARDS.md §1: removing the handler fails exactly one test (ApplicationScopeTest.kt:69), and removing either catch fails exactly its own. GUARDS.md gains §8. prove-guard.sh decides a guard caught the mutation from the runner's exit code, and cannot tell a broken test from a malformed command. Its first use here reported a clean catch when Gradle had actually rejected `:app:test --tests` as an unknown option and run nothing. The same tool's line-counting fallback also means the three documented boundary proofs in architecture/README.md have been exiting 3 rather than 0 since they were written; they now carry the fail pattern that makes them exit 0. closes #45 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-19 03:06:10 -05:00
## 8. A red is not a proof — read what went red
`scripts/prove-guard.sh` decides the guard caught the mutation from the runner's
**exit code**. It cannot tell *"the mutation broke the test"* from *"the command
was malformed and nothing ran"*, and both are non-zero.
Found by using it on the fix for the missing application-scope exception
handler. This looked like a clean proof and was not one:
```bash
bash scripts/prove-guard.sh app/.../PeriodApplication.kt \
' + CoroutineExceptionHandler { _, _ -> }' '' \
./gradlew :app:test --tests '*ApplicationScopeTest*'
```
`:app:test` is AGP's lifecycle task and takes no `--tests` option, so Gradle
failed with `Unknown command-line option '--tests'` in 544 ms. The mutation was
never compiled and the test never ran — and `prove-guard` reported *"the guard
caught it"*. The concrete task is `:app:testDebugUnitTest`; against that, the
same mutation failed exactly one test and the proof was real.
Two rules follow, and the first is the general one:
- **Read the `--- what failed ---` block, every time.** A proof is a proof only
when the *named test* is what went red. An exit code alone cannot distinguish
a caught regression from a typo, and this script is most likely to be run at
the moment you least want to read output — after the code already works.
- **Give it a fail pattern when the runner prints no summary.** The fallback
counts matching log lines, and the default pattern also matches Gradle's
`FAILURE:` and `BUILD FAILED` banners, so one caught violation reads as three
and the script exits 3. Exit 3 is not a pass. The three boundary-guard proofs
in [`README.md`](README.md) carry a `PROVE_GUARD_FAIL_PATTERN` for exactly
this reason; without it they exit 3 while the guard is behaving perfectly,
which is the failure this document exists to prevent — a check whose red you
have learned to ignore.
The uncomfortable part: the documented proofs had been exiting 3 rather than 0
since they were written. Nobody had run them and read the last line.
feat: a guard that every drawable has a night twin The light/dark pairing was held by a @Preview and nothing else. A preview fails no build and nothing runs it, and the KDoc on OnboardingPreviews.kt already said why that matters: "a set of eight where seven have a night variant looks completely fine in light mode." The failure mode is what makes this worth a guard. A missing drawable-night file does not crash, does not warn, and does not fall back to nothing — Android resolves the light drawable and draws it on a dark screen. The only other way to find it is to open that one screen in that one theme, which is how #44 was found and how it sat unnoticed until somebody looked. checkThemedDrawables walks both directions: a light asset with no night twin, and a night asset with no light one. The second is the same defect from the other side and renders as nothing rather than as the wrong picture. Exemptions are a named map with a reason each, rather than a narrowed scope. ic_launcher_monochrome is the only entry: the launcher tints it from the system palette, so a night copy would be a second source of truth for one shape. A scope that only listed today's eight illustrations would not cover tomorrow's, and the defect this guards against is a file somebody forgot. Proved four ways, because prove-guard.sh cannot drive this one — it replaces a string inside a file, and this guard's failure mode is a file that is not there, in a set that is all .webp. GUARDS.md gains section 9 for that class of guard, and the manual recipe from section 1 was run instead: - a night twin deleted -> exactly 1 violation, naming art_welcome - a dark-only asset added -> exactly 1 violation, naming art_orphan - both restored -> green, 64 resources across 5 folder pairs - roots pointed at a folder that does not exist -> "no drawables were found ... not a pass" The fourth is the one worth copying. Refusing to report a pass over an empty observation is itself a thing to prove: a guard that finds nothing and says "clean" is the failure GUARDS.md was written after, and two guards in this project have done exactly that. closes #42 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-19 21:37:43 -05:00
## 9. Some guards cannot be driven by `prove-guard.sh`, and must still be proved
`scripts/prove-guard.sh` breaks a guard's target by **replacing a string inside a
file**. That covers every guard whose subject is code, which was all of them
until `checkThemedDrawables` — whose failure mode is a file that is *not there*.
There is no string to replace in an absent file, and the drawables are `.webp`,
so there is no text in the present ones either. The tool simply does not reach
this class of guard.
That is not permission to skip §1. It means running §1's manual recipe instead,
with the same standard — break exactly one thing, require exactly one failure,
restore, require green again — and writing down what was run. For
`checkThemedDrawables` that was four passes:
```bash
# 1. a night twin is deleted -> 1 violation, naming art_welcome
# 2. a dark-only asset is added -> 1 violation, naming art_orphan
# 3. both restored -> green, 64 resources, 5 folder pairs
# 4. themedResourceRoots pointed at a
# folder that does not exist -> "no drawables were found … not a pass"
```
The fourth is the one worth copying. Every guard here refuses to report a pass
over an empty observation, and that refusal is itself a thing to prove — a guard
that finds nothing and says "clean" is the exact failure this document was
written after.