# 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. --- ## 7. An assertion can pass because one string happens to sit inside another **M4-b.** `assert_string_contains(line, Callings.armor(id))` checked that a calling's detail line mentioned its armour — using the **raw** table value (`"light"`, `"none"`), not the word the card actually prints (`"light armour"`, `"no armour"`). For light/medium/heavy this passed **by accident**: `"light"` is a substring of `"light armour"`. It broke only on the Hedge-Mage, the one calling with `armor: "none"`, because `"none"` is not a substring of `"no armour"`. Delete the Hedge-Mage and the bug goes dormant and silent — nothing else in the roster would ever expose it. The same species, same milestone: `assert_string_contains(line, str(Callings.skill_count(id)))` could not fail for the Reaver — its skill count is 2, and `"2"` is already sitting inside `"d12"` (its hit die). Hardcoding the wrong skill count in the card would still pass, satisfied by a digit in an unrelated number. **The guard:** anchor the assertion to the **full derived phrase** the code actually produces (`CreationCopy.armor_word(id)`, `"picks %d skills" % Callings.skill_count(id)`), never to a bare fragment that might coincidentally be present for the wrong reason. If an assertion would still pass with the code deleted and a different, unrelated number substituted, it is checking overlap, not correctness. --- ## 8. A wrapper can hide the void it was supposed to catch **M4-b.** A §7 sweep asserted every race×calling combination's DM-panel prose was non-empty — the guard meant to catch a blank `ContentDB` leaking onto the one screen that must never show a raw error. But the binder wraps prose in BBCode (`"[i]%s[/i]"`) and the origin panel always emits its own literal connective (`" Now you carry a %s's work — "`) even when every authored fragment and blurb is blank. `"[i][/i]".strip_edges().is_empty()` reads `false` regardless of what content actually rendered. The 28-combination sweep was proving the *wrapper* is non-empty, not that the guard against a blank `ContentDB` held. **The guard:** when a value is always wrapped in a fixed template before display, assert that the render **contains the payload's own string** (and that the payload's own string is itself non-blank) — never just that the wrapped result is non-empty. A non-empty assertion downstream of a non-empty literal proves nothing. --- ## 9. A test that asserts the script's own loop bound, not the artifact **M4-b.** `test_five_ability_cards_never_six` asserted `_ability_cards.size() == 5` — but that array is filled by the binder's own `for i in range(5)`, so it is exactly 5 **by construction**, independent of what the scene actually contains. An `Ab5` node could be added to the `.tscn` and this test stayed green while a sixth ability card — the exact regression §7 exists to prevent — rendered on screen. **The guard:** interrogate the artifact, not the code that reads it. `get_node_or_null("Ab5")` must be null, or `get_child_count()` on the authored container must equal the expected count. A test whose only source of truth is a literal in the same file it is meant to be guarding is not a test of that file. --- ## 10. A guard whose only fixture makes it trivially true **M4-b.** `test_only_the_origin_s_allowed_callings_are_shown` checked that shown calling cards equal the origin's `allowed_callings`. The only origin in the game (the deserter) allows all seven callings, and cards default to visible — so the assertion was `7 == 7` no matter what the hiding logic did. **Deleting the entire hide branch left the test green.** **The guard:** a gating rule needs a fixture that actually gates. Inject a restrictive origin (the injection seam for `origin`/`ContentDB` already existed for exactly this) that allows a strict subset, and assert the excluded cards are absent. A test with only the permissive case in play is not testing the restrictive path at all. --- ## 11. An assertion that was already true before the action under test ran **M4-b.** A re-roll test asserted `"points left: 3"` **after** calling re-roll — but a freshly-constructed draft already reads 3 points left. Removing `spend = {}` from `reroll()` entirely left the test green, because the assertion never depended on the reset actually happening. The same species bit a displayed-scores test harder: it asserted the ability cards show `draft.final()` values, but spent zero points before checking — so `final() == rolled()` and a binder bug that read `rolled` instead of `final` (arguably the single most obvious possible bug in a point-buy panel) passed clean. **The guard:** drive the state away from its default before asserting the reset or the derivation. Spend a point, *then* re-roll and check the pool refilled. Spend a point, *then* check the card shows the spent value, not the rolled one. If the assertion would already hold on a brand-new object with no action taken, the test has not exercised anything. --- ## 12. A "covers everything" claim that covers three things **M4-b.** `test_committed_tres_matches_builder` is the drift guard between the theme builder and the committed `.tres` artifact — the thing that stops "changed the palette, forgot to regenerate" from shipping silently. The plan described it as iterating `ThemeKeys.ALL`. It did not: it checked a hardcoded list of exactly three variation/state pairs, so every variation added since — including all five this milestone added for the creation screen — could drift from its generator with the test still green. **The guard:** when a guard is described as covering "every X," check that it actually enumerates `X.ALL` (or equivalent) rather than a literal list someone wrote down once. And make the failure message name the specific variation and property that drifted — a guard that only says "theme mismatch" does not tell the next person which of forty checks failed. --- ## 13. A node-type sweep that misses the types that matter most **M4-b.** The §7 Luck-invisibility guard swept `find_children("*", "Label", ...)` across the creation screen looking for the word "luck." `RichTextLabel` does **not** extend `Label` in Godot's class hierarchy — and the two `RichTextLabel` nodes on the screen were the only nodes rendering authored prose (the DM origin panel and the calling detail panel), i.e. the single likeliest place for a stray "luck" to leak onto the screen. The sweep also missed every `Button` (whose text is a property, not a child Label) and `LineEdit.placeholder_text`. **The guard:** a sweep for "does this string appear anywhere on screen" must walk `Control` and check every text-bearing property that type can hold (`text`, `.text` on buttons, `placeholder_text`, `bbcode_text`), not one Label subclass. Naming the node type you filtered on is not the same as covering the node types that carry the content you care about. --- ## 14. A test that calls the handler instead of pressing the button **M4-b.** Every interaction test for the creation screen invoked `_on_race_pressed(i)` / `_on_calling_pressed(i)` / etc. directly. **Commenting out `_wire()` entirely — the method that connects every button's `pressed` signal to its handler — left the whole suite green**, because no test ever went through the signal. A mis-bound `bind(i)` (wrong card wired to the wrong index) would have been equally invisible. **The guard:** drive the interaction through the actual node — `button.pressed.emit()` (or, for a real click, `button.pressing`/`gui_input`) — not the handler function. This covers the wiring *and* the index binding for free, and it is the only way a test can tell you a button that looks correct in the editor is inert at runtime. --- ## 15. GUT can skip a test file with a warning, not a failure **M4-b.** A new `class_name`-declaring script (`CreationDraft`, `CreationCopy`) needs Godot's `.godot/` import cache rebuilt before GUT can resolve the global class name from a sibling test file. Until that happens, the test file fails to parse and GUT **silently skips it with a `WARNING`**, not a failure — and the run still prints `All tests passed!`, at the **old** test count. A TDD RED phase against a brand-new global class can therefore be **fake**: you believe you watched the new test fail, but it never ran at all. **The guard:** check the test **count**, every time, not the green banner text. If a change was supposed to add N tests and the total didn't move, the suite lied by omission. `rm -rf .godot && ./run_tests.sh` forces the cache rebuild if a new file seems to be missing. --- ## 16. A test can hang instead of fail **M4-b.** The natural way to write "drain the point-buy pool" in a test is `while draft.points_left() > 0: plus.pressed.emit()`. Run that against a broken `_wire()` (trap 14) and `points_left()` never changes — the loop **never terminates**, and the test runner hangs instead of reporting a failure. **The guard:** bounded `for` loops only, with an explicit iteration cap well above the expected count, and an assertion after the loop that the expected end-state was actually reached. A test that can hang is worse than a test that can silently pass — it costs a human a wall-clock timeout to even learn something is wrong. --- ## 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. - Does an assertion string appear **only as a fragment** of another string? Anchor to the full derived phrase. - Is the value under test **always wrapped** in a fixed template first? Assert on the payload, not the wrapper. - Could the assertion be reading a bound the **test's own setup** guarantees, rather than the artifact under test? - Does the fixture make the guard's condition **trivially true** regardless of the logic it claims to check? - Would the assertion **already hold before the action under test runs**? Drive state away from its default first. - Does "covers everything" actually **iterate the full set**, or a hardcoded sample of it? - Does a node-type sweep account for **every type** that can carry the content, not just the common one? - Does the test **press the node** (emit the real signal), or call the handler directly? - Did the test **count** move by the expected amount — not just the green banner? - Can the test **hang** on a broken precondition? Bound every loop.