307 lines
13 KiB
Markdown
307 lines
13 KiB
Markdown
# Guards — how to write a check that actually checks
|
|
|
|
```
|
|
Status: Current
|
|
Owner: <who maintains this>
|
|
Last reviewed: <YYYY-MM-DD>
|
|
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.
|
|
|
|
## 8. Find the vacuous ones in one run: comment the whole tree out
|
|
|
|
§1 proves one guard at a time, which is the right thing to do while writing one
|
|
and far too slow for a suite that already exists. The audit that produced these
|
|
rules found six vacuous guards by reading; the sweep below found forty in ninety
|
|
seconds, because it asks every guard the same question at once.
|
|
|
|
The question is the one the failure mode is named after. **What survives when
|
|
the code is moved into a comment?** Every byte a source-grep can see is still
|
|
there; nothing runs.
|
|
|
|
```bash
|
|
# In a throwaway copy — never the working tree.
|
|
cp -a repo /tmp/mutant && cd /tmp/mutant
|
|
# Prefix every line of every source file with the language's comment marker.
|
|
# Shell keeps line 1, so the shebang survives.
|
|
npx vitest run --reporter=json --outputFile=/tmp/survivors.json
|
|
```
|
|
|
|
Then read out the tests that **passed**. Three kinds of answer come back:
|
|
|
|
- **A test that claims to check behaviour.** "Refuses anywhere that is not
|
|
Discord", "checks the guard before doing anything else", "never deletes what
|
|
production is running". These are the findings. Every one is a `toContain`
|
|
against source text whose docblock argues the same rule in the same words.
|
|
- **A negative assertion** — `not.toContain`, or an empty-list expectation over
|
|
a walk. These pass on an empty tree by construction and are not findings on
|
|
their own. They need §4 applied instead: something in the file must prove the
|
|
walk found anything and the pattern still matches.
|
|
- **A structural check with nothing to do with source text** — file sizes,
|
|
counts, a fixture asserting things about itself. Not findings.
|
|
|
|
### The sweep needs §4 applied to itself, and the first one here did not
|
|
|
|
**Run it with the test database UNSET.** This is the trap that matters most,
|
|
because it fails silently and in the flattering direction. With
|
|
`TEST_DATABASE_URL` set, every database-backed file's `beforeAll` throws against
|
|
the commented-out migration runner — and vitest marks every test in a file whose
|
|
`beforeAll` threw as **skipped**, not failed. A skipped test is not a passing
|
|
test, so it never appears in the survivor list. The first sweep run here was done
|
|
that way: 42 files exploded at setup, their pure source-grep describes were never
|
|
asked the question at all, and the answer came back **208 survivors** when the
|
|
real figure was **502**. It undercounted by about 345 — and it undercounted by
|
|
counting "could not check" as "nothing found", which is the exact confusion §4
|
|
exists to forbid.
|
|
|
|
Unset the variable and those files' `beforeAll` returns early instead, so their
|
|
non-database describes still run and still answer.
|
|
|
|
**Prove the instrument before trusting the number.** A sweep that mutates nothing
|
|
reports every test as a survivor; a sweep that deletes everything reports none.
|
|
Assert that code survived the mutation *and* that the mutation happened — one
|
|
guard here measured only how much its stripper removed, so 100% removal read as a
|
|
healthy instrument.
|
|
|
|
**Mutate every file a guard can read**, not just the obvious source tree. A first
|
|
pass left `notices/`, `reporter/` and `radar/` intact and reported forty-four
|
|
working tests as suspects. A later one missed `.forgejo/workflows/*.yml`,
|
|
`src/db/migrations/*.sql`, `*.css` and `.npmrc` — five guards are only
|
|
demonstrably red once those are included.
|
|
|
|
**Write the mutated file from a variable you read first.**
|
|
`open(p, "w").write(f(open(p).read()))` truncates before it reads, so it deletes
|
|
the file rather than commenting it out, and a deletion proves something weaker.
|
|
|
|
The recursion is the lesson worth keeping: the instrument built to find guards
|
|
that cannot fail was itself a guard that could not fail, and it stayed wrong
|
|
until somebody asked it the question it asks everything else.
|
|
|
|
## 9. `toThrow()` cannot tell "refused" from "broken"
|
|
|
|
`expect(() => parse(url)).toThrow()` with no matcher is satisfied by any throw
|
|
at all — including `TypeError: parse is not a function`, which is what a deleted
|
|
export produces. An SSRF boundary asserted this way passes when the validator
|
|
has been removed entirely.
|
|
|
|
```ts
|
|
// Vacuous: green when parseWebhookUrl no longer exists.
|
|
expect(() => parseWebhookUrl("https://evil.example/api/webhooks/1/t")).toThrow();
|
|
|
|
// Names the refusal, so a broken import is a failure rather than a pass.
|
|
expect(() => parseWebhookUrl("https://evil.example/api/webhooks/1/t"))
|
|
.toThrow(InvalidInputError);
|
|
```
|
|
|
|
The same applies to `rejects.toThrow()` on a database probe: name the
|
|
constraint, or a typo in the table name passes as the constraint firing.
|
|
|
|
## 10. A floor over a walk is not a check on the walk
|
|
|
|
`expect(routes.length).toBeGreaterThan(40)` is satisfied by 41, 57 and 500
|
|
equally, and `expect(actionFiles.length).toBeGreaterThanOrEqual(1)` was
|
|
satisfied by a walk that found one of twenty-two files — while every other
|
|
assertion in that file was an `it.each` over the same walk, so the run went
|
|
green having checked almost nothing.
|
|
|
|
When the real set is discoverable, derive it twice and compare:
|
|
|
|
```ts
|
|
const independently = readdirSync(ADMIN_DIR, { recursive: true })
|
|
.map(String)
|
|
.filter((entry) => entry.endsWith("page.tsx"));
|
|
|
|
expect(pages).toHaveLength(independently.length);
|
|
```
|
|
|
|
Neither number is maintained by hand, adding a page fails nothing, and a walk
|
|
that stopped entering `[slug]` directories is a failure rather than a smaller
|
|
green run.
|
|
|
|
## 11. A guard armed by a field is only as armed as the sloppiest writer
|
|
|
|
A guard that reads a field to decide whether to fire is armed only where every
|
|
writer sets that field. The two writers you fixtured set it. The shared helper
|
|
they both call defaults it to empty, and every other writer — present and future
|
|
— writes that default straight past the guard. Nothing fails. Nothing logs. The
|
|
fixtures stay green, because they exercise the writers that get it right.
|
|
|
|
**The test to add is not another one about the guard. It is one about the
|
|
default.** Either the default carries the safe value, or the field is not
|
|
optional. A guard whose arming depends on every caller remembering an argument
|
|
is not armed; it is armed *so far*.
|
|
|
|
## 12. A guard in one caller is a guard over that caller
|
|
|
|
Putting the check inside the function that happens to be in front of you protects
|
|
that path and no other. The next caller reaches the same resource by a different
|
|
route and meets nothing.
|
|
|
|
Count the ways in before deciding where the guard goes. If there is more than
|
|
one, the guard belongs at the resource — or the resource belongs behind one door.
|
|
|
|
## 13. A guard that restates the list is a copy of the list
|
|
|
|
A guard written to stop two lists drifting apart, which contains its own copy of
|
|
one of them, is a third thing to keep in step. It passes by agreeing with itself.
|
|
|
|
```python
|
|
# Wrong: the literal is a second copy, and adding a surface updates neither.
|
|
check("every SURFACE is one the gate can see",
|
|
set(SURFACES) == {"a", "b", "c", "d"})
|
|
|
|
# Right: derive both sides from the thing that decides.
|
|
check("every SURFACE is one the gate can see",
|
|
set(SURFACES) <= set(gate_reads()))
|
|
```
|
|
|
|
**Derive it, or justify the constant.** Ask the daemon, read the config, walk the
|
|
schedule — do not restate it. Where a constant genuinely must be written down,
|
|
say in a comment *where the authority is* and what would make it wrong.
|
|
|
|
This is the narrow form of a rule `docs/WORK_CYCLE.md` requires every plan to
|
|
answer: **hardcode as little as possible.** It is true of guards, and of
|
|
everything else.
|
|
|
|
Two limits, so it does not become a slogan:
|
|
|
|
- **Something has to be the root.** A path, a channel id, a threshold taken from
|
|
a real incident: those *are* the authority, not copies of one. The rule is
|
|
against the **second** copy.
|
|
- **Derive the input, never the assertion.** A probe that derives its own
|
|
expectation from the thing it is testing proves nothing at all.
|
|
|