Phillip Tarrant
b4b72e472e
Six bugs this project shipped into a branch, what let each through, and the guard that now catches it. They share a shape: the test passed, and the test was wrong. - a regression test that constructed its own correct object, so reverting the bug left the suite green - GDScript default params turning an arity change into a silent miscompile, invisible to a grep for the renamed field (grep cannot see positional args) - a determinism test comparing two runs that would change together - a closed move vocabulary that was not bounded, because the model could repeat a tag and choose an amount in unary - a roster written down in four places with only one pair guarded — including a shipped authoring skill that would have emitted origins allowing zero callings - generated content that has a client consumer Linked from CLAUDE.md §18 and docs/README.md. Also records M4-a on the roadmap with its live end-to-end proof. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
130 lines
6.4 KiB
Markdown
130 lines
6.4 KiB
Markdown
# Traps
|
|
|
|
Bugs this project has **already** shipped into a branch, what let them through, and the
|
|
guard that now catches them. Read before writing tests; each entry is a class of mistake,
|
|
not a one-off.
|
|
|
|
The pattern in almost all of them: **the test passed, and the test was wrong.** A green
|
|
suite is evidence only if the test can fail.
|
|
|
|
---
|
|
|
|
## 1. A regression test that cannot fail against the bug is not a regression test
|
|
|
|
**M4-a.** `LogPlayer.new()` changed from `(name, class_id, luck_descriptor)` to
|
|
`(name, race_id, calling_id, luck_descriptor)`. Three callers kept the old 3-argument
|
|
shape. The fix added a test — which constructed its *own* correct `LogPlayer` and asserted
|
|
the constructor worked. You could revert the broken call site and the suite stayed green.
|
|
|
|
**The guard:** a regression test must assert on **the thing that was broken**, not on a
|
|
fresh correct instance of the same type. Here, that meant asserting on the seed log the
|
|
*call site* builds (`test_main_window_shell.gd`), not on a `LogPlayer` the test made.
|
|
|
|
**Prove it.** Before accepting a fix, re-break the code, run the suite, and *watch the new
|
|
test fail*. If it doesn't, the test is decoration. This is now the standard for any fix
|
|
that claims to guard a regression.
|
|
|
|
---
|
|
|
|
## 2. GDScript default parameters turn an arity change into a silent miscompile
|
|
|
|
**M4-a.** Every parameter of `LogPlayer._init` has a default, so
|
|
`LogPlayer.new("Vexcca", "sellsword", "Fortune spits on you")` — the *old* 3-arg call —
|
|
compiled without a warning against the *new* 4-arg signature. `"sellsword"` bound to
|
|
`race_id` (rejected → `""`), the Luck descriptor bound to `calling_id` (rejected → `""`),
|
|
and `luck_descriptor` defaulted to `""`. The live shell and both proving scenes emitted a
|
|
player block of four empty strings — which fails the very JSON Schema that migration had
|
|
just tightened. Two `push_error`s fired into a log nobody was reading.
|
|
|
|
**Why it survived review:** the migration was verified with
|
|
`grep -rn "class_id" client api docs content`, which came back clean. **Grep cannot see
|
|
positional arguments.** The field name was gone; the *call sites* were not.
|
|
|
|
**The guard:** when changing a function's arity or parameter order, grep for the
|
|
**callers** (`grep -rn "LogPlayer.new("`), never for the field name. And if the type is
|
|
constructed in a scene or harness that no unit test touches, that is exactly where the
|
|
bug will live — write the test that loads it.
|
|
|
|
---
|
|
|
|
## 3. A determinism test that compares two runs tests nothing
|
|
|
|
**M4-a.** Character creation is seeded: the same seed must always produce the same
|
|
character (charter §10). The test built the character twice and asserted the two were
|
|
equal. Both runs change *together* — so reordering `Attributes.IDS` from
|
|
`str,dex,con,fth,mag` to anything else would have silently rewritten **every seeded
|
|
character in every future save**, with the suite fully green.
|
|
|
|
**The guard:** pin the **stream**, not the symmetry. `test_golden_vector_pins_the_rng_stream`
|
|
hardcodes the exact five attribute values *and the exact hidden Luck* for a fixed seed. It
|
|
was proven to fail against a reordered `Attributes.IDS`.
|
|
|
|
Any RNG-order contract (creation now; combat seeding at M5; saves at M9) needs a golden
|
|
vector. Two-runs-agree is not a determinism test.
|
|
|
|
---
|
|
|
|
## 4. A closed vocabulary is not bounded if the model can repeat a tag
|
|
|
|
**Currency.** Charter §6 gives NPCs eight moves and *no quantity argument*, deliberately:
|
|
the model picks from a closed vocabulary and never invents a number (§2). But
|
|
`TagExtractor` returns **every** occurrence of a tag, `MoveValidator` is a pure membership
|
|
test, and `MoveApplier` looped and applied each one. So a model emitting
|
|
`[MOVE: accept_item(copper)]` forty-seven times drained a 47-copper purse, and ten
|
|
`[MOVE: give_item(gold)]` tags minted 100,000c.
|
|
|
|
**The model chose the amount, in unary.** Rejecting a quantity argument bought nothing;
|
|
repetition smuggled the integer back in.
|
|
|
|
**The guard:** a move applies **at most once per reply**, keyed on `name(id)`. And
|
|
`adjust_disposition`'s `MAX_DELTA` now caps the reply's **net** swing — it used to clamp
|
|
per *tag*, so three `+15`s moved standing by 45, the exact "wholesale swing" its own
|
|
comment forbids.
|
|
|
|
**The general lesson:** when the defence is "the model can only pick from a list," ask what
|
|
happens if it picks the *same item N times*. Validation that is a membership test is not a
|
|
rate limit.
|
|
|
|
---
|
|
|
|
## 5. One fact, two homes — and only one of them is guarded
|
|
|
|
**M4-a.** The calling roster (`sellsword`, `reaver`, …) needs to be known by the client
|
|
(`Callings.IDS`), the JSON Schema the API validates against, the *origin* schema, and the
|
|
content blurb files. A parity test guarded code ↔ canon-log schema. It did **not** guard:
|
|
|
|
- `docs/canon-log.md` — *the* cross-boundary contract doc, still documenting `class_id`.
|
|
- `.claude/skills/world-building/references/schema.md` — a **shipped authoring tool** whose
|
|
job is to emit content in the exact format the build consumes. It still told authors to
|
|
write `allowed_classes`. Any origin authored with it would have been rejected by the
|
|
schema, and at runtime `NewGame` would have read `allowed_callings` → `[]` → **every
|
|
calling forbidden.**
|
|
|
|
**The guard:** when a roster or enum exists in more than one place, enumerate **every**
|
|
copy — including prose docs and tooling — and add a parity test per machine-readable copy.
|
|
Ask specifically: *is there a doc, a skill, or a template that also writes this down?*
|
|
|
|
---
|
|
|
|
## 6. Generated content has a client consumer
|
|
|
|
**Duncarrow purge.** `content/world/**` is emitted by `content_build`, but it is also
|
|
**read by the client** (`content_db.gd` via the harnesses, plus `test_content_db.gd`).
|
|
Deleting generated content is therefore a *client* change, not a content change, and will
|
|
break client tests.
|
|
|
|
**The guard:** after any content change, run `PYTHONPATH=tools python3 -m content_build
|
|
--check`, then `git diff --stat dev...HEAD -- content/world content/server` and confirm
|
|
nothing changed that you did not intend.
|
|
|
|
---
|
|
|
|
## The checklist this all reduces to
|
|
|
|
- Can the new test **fail**? Re-break the code and watch it. If it can't fail, it isn't a test.
|
|
- Changed an **arity or parameter order**? Grep the *callers*, not the field name.
|
|
- Anything **seeded**? Pin the stream with a golden vector.
|
|
- Anything the **model can emit**? Ask what happens if it emits it *twice*.
|
|
- Any fact written down **twice**? Guard every copy — including docs and tooling.
|
|
- Touched **content**? Check the generated tree and the build.
|