code_of_conquest_dnd/docs/traps.md

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_errors 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 +15s 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.

It took three fixes, and that is the actual lesson.

• Fix 1 replaced the hardcoded three with ThemeKeys.ALL. Still could not fail: ALL, by its own docstring, holds stylebox variations only — it deliberately excludes the six font roles. Change a palette colour used only by a font role, skip the regen, ship a stale theme, green.

• Fix 2 added FONT_ROLES and iterated both. Still could not fail: the builder also styles some base types directly (RichTextLabel's font, italics font, size and colour), and those are in neither set. RichTextLabel is what renders every line of DM prose on the creation screen.

• Fix 2 also added a meta-guard — a test that walks the builder's own output and fails if it styles anything that no set covers. That guard was written specifically to make a third miss impossible. It contained this:

  if fresh.get_type_variation_base(variation) == &"":
      continue        # a BASE type, not a variation
  

  The meta-guard's one exemption was exactly the gap it existed to catch.

The guard: when a guard is described as covering "every X," check that it enumerates X.ALL (or equivalent) rather than a literal list someone wrote down once — and then check what X.ALL actually contains, because a set's name is not its contents. 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.

And the harder lesson: a guard-of-a-guard with an exemption in it is not a guard. If you write a meta-test to prove a set is complete, every continue and every if … : return in it is a hole you are cutting on purpose. Assert on the exempted case instead of skipping it, or you have built the very thing you were trying to prevent, one level up, where nobody will look for it.

---

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.

---

17. Asserting against a value the engine derives from the thing under test

M4-b, the F6 pass. The race cards were authored 108px tall, and their text — the trait line — visibly rendered below the card's bottom border in the running game. The guard written for it read:

var needed := box.get_combined_minimum_size().y   # 114
assert_lte(needed, box.size.y)                    # box.size.y is ALSO 114

It passed. Not against the fix — against the bug. The card was put back to its overflowing 108px and the suite stayed green, 29/29.

The reason is a Godot rule that is easy to forget: Control.size is clamped UP to get_combined_minimum_size(). The Box is anchored inside the card, so a 108px card leaves it 82px of room — but the Box does not become 82px tall. It reports 114, its own minimum, and quietly overflows the card's border. So the assertion compared 114 against 114: needed <= box.size.y is not a size check at all, it is x <= x. It could never have failed, for any card, at any height.

The guard: assert against the container that actually clips — the card — not against the child, whose size the engine has already reconciled with the very number you are testing:

var available: float = card.size.y - box.offset_top + box.offset_bottom  # 82
assert_lte(needed, available)                                            # 114 <= 82 -> RED

The general shape: if the engine derives B from A, then assert(A <= B) is a tautology, not a test. Layout, min-size, and any auto-fitted value are all A-and-B. Reach for a number the engine computed from a different source — the parent, the authored constant, the viewport — or the assertion is checking that arithmetic works.

Related: this is trap 11's family (an assertion already true before the action ran), but it is worse — this one is true by construction, so no amount of driving state can redeem it.

Corollary, same fix: geometry assertions are meaningless before the container tree has sorted. On the frame add_child() runs, a race card reports 102px wide and its autowrapped blurb reports a 2508px minimum height. await get_tree().process_frame twice, or the guard fails for a reason that has nothing to do with the bug.

---

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.
• Is the assertion's right-hand side derived by the engine from its left-hand side (Control.size vs get_combined_minimum_size())? Then it is x <= x. Measure against the parent that clips, not the child that reports.
• Is it a geometry assertion? Let the container tree sort first, or it measures an unlaid-out node.