diff --git a/CLAUDE.md b/CLAUDE.md index 91c208e..e440017 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -559,6 +559,12 @@ contracts (§12), and Failure UX (§13). - **When a request conflicts with this document, say so.** Do not silently comply. This file is the argument; if it is wrong, change the file first. - **When adding a system, state which side of §2 it falls on.** State or text. If it is both, it is two systems. - **Prefer deleting scope.** The POC's value is answering one question fast. +- **A green suite is evidence only if the test can fail.** Before accepting a fix that + claims to guard a regression, re-break the code and *watch the new test fail*. This + project has shipped a regression test that passed against its own bug, a determinism + test that compared two runs that changed together, and an arity change that + miscompiled silently past a clean grep. Read **[docs/traps.md](docs/traps.md)** before + writing tests — it is six ways this codebase has already fooled a green suite. ### Git workflow diff --git a/docs/README.md b/docs/README.md index 6c64af5..b771f0c 100644 --- a/docs/README.md +++ b/docs/README.md @@ -5,7 +5,9 @@ Anything affecting **both** client and api, plus planning and roadmap. Side-spec Put here: - [`roadmap.md`](roadmap.md) — phases, POC scope, what's next +- [`traps.md`](traps.md) — **bugs we have already shipped, and the guards that catch them.** Read before writing tests: every one of them passed a green suite. - [`adr/`](adr/) — Architecture Decision Records +- [`canon-log.md`](canon-log.md) — the cross-boundary contract (§11) - Shared contracts — canon log schema, tag syntax, retry policy (§11, §12) - Anything spanning the HTTP boundary diff --git a/docs/roadmap.md b/docs/roadmap.md index 9972bb3..dd9df3a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -75,6 +75,7 @@ Each screen is recreated as a Godot 4.7 Control-node scene against the mockups ( ### M4 — Character creation *Content it consumes → [BOM](../content/roadmap.md#2-bill-of-materials): calling blurbs + the Bloodsworn's patron choice (fixed); origins (variable). **~4 pieces.*** +- ✅ **Creation model (M4-a)** — the rules a character is made of, no UI ([spec](superpowers/specs/2026-07-12-creation-model-design.md)). Race/calling/skill **mechanics** are static tables in code (`client/scripts/rules/`); their **blurbs** are hand-written content (`content/world/{callings,races}/`, loaded by `ContentDB`, parity-tested), so the content BOM does not block the engine. `CharacterSheet` **stores only what was rolled or chosen** (attributes, race, calling, skills, current hp/mp) and **derives** the rest (`max_hp`, `ac`, `save`, `skill_bonus`, `spell_dc`) — so no derived number can drift from its inputs, and M5's level curve is a function change rather than a stored-field migration. **`NewGame.construct` carries a seed, not a stat block:** it rebuilds the RNG and rolls the attributes itself, so it never trusts a number handed to it (§2), and the same seed yields an identical character, hidden Luck included (§10) — pinned by a golden-vector test. The roll is **3d6 with a hard floor of 8** (straight 3d6 puts ~9% on a primary the +3 pool cannot rescue; a character bad at the one thing he is *for* is the "fine" §7 forbids). **LCK is unspendable, not merely hidden.** Open seams marked honestly: `ac()` is `10 + DEX` until armor exists (M7), `max_mp()` is a `TUNABLE` placeholder (M5 owns the curve), L1 talents are id stubs (M5). **Live-proven end-to-end** (2026-07-12): a beastfolk cutpurse built by the pipeline → canon log → schema validation → `Player: Vexcca, a beastfolk cutpurse` in the digest (no numeric Luck, no stats, no snake_case id) → real narration from qwen3.5. 250 client tests, 76 api (live layer included). *§2: state · goal: the character exists in code.* - ✅ **Contract migration** — the two contract migrations this milestone owed have **landed**: the canon log's `player` block is now `{name, race_id, calling_id, luck_descriptor}` (the Narrator/NPCs describe the player as e.g. "a beastfolk cutpurse"), and an origin's build-constraints key is now `allowed_callings` (`content/origins/deserter.json` lists all seven real calling ids; the pre-migration key and its two dead class ids are gone from the codebase). Schemas, docs, fixtures, and the world-building skill's content template all agree. - ○ **Character creation UI (M4-b)** — the mock's screen over our proven model: race + calling cards, six stats with **LCK hidden entirely** (§7), 3d6 floors, the **+3 additive-only spend-up** pool (§8, matches the `character-creation-stats` model), live origin blurb + nameplate. **The class-name reconcile is settled** — all seven callings are named (Sellsword · **Reaver** · Cutpurse · Trapper · Hedge-Mage · Bonesetter · **Bloodsworn**; [races/classes spec](superpowers/specs/2026-07-11-races-and-classes-design.md) §4). Feeds new-game canon-log construction (already built, and already emits `race_id` + `calling_id` per the migration above). **⛨ saga:** `NewGame.construct` takes `creation` as a plain `Dictionary` — keep it that way. The creation *scene* produces that dict but must not become the only thing that can (a saga synthesizes it and skips the UI). *§2: state · depends on the creation model (M0) + Theme · goal: build a character.* - ○ **Title → creation → shell flow (M4-c)** — wire the title screen's *new game* path into character creation and on into the M3 shell, without hardcoding a `new game → creation → world` linear flow (the saga's fourth entry path, §M3, must still be addable later). *§2: n/a (flow) · depends on M4-b + the Title screen (M3) · goal: a playable start-to-shell loop.* diff --git a/docs/traps.md b/docs/traps.md new file mode 100644 index 0000000..9948052 --- /dev/null +++ b/docs/traps.md @@ -0,0 +1,129 @@ +# 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.