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_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.

**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:

  ```gdscript
  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.

---

## 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.