diff --git a/client/docs/README.md b/client/docs/README.md index feb472f..6e258ed 100644 --- a/client/docs/README.md +++ b/client/docs/README.md @@ -17,7 +17,15 @@ Cross-cutting design (anything touching both client and api) goes in the root [` Screens are **authored editor-first** — the layout lives in the `.tscn`, not in code. See ADR [0001](../../docs/adr/0001-editor-first-ui-scenes.md) for the why; this is the how. Reference: `scenes/shell/{MainWindowShell,SystemDock,NarrationBook}.tscn` -+ `scripts/ui/shell/*.gd`. ++ `scripts/ui/shell/*.gd`, and `scenes/creation/CharacterCreation.tscn` + +`scripts/ui/creation/{character_creation,creation_draft,creation_copy}.gd` (M4-b). + +- **Clickable cards are Buttons with `mouse_filter = 2` on every child.** A card that + holds a name, a blurb and a trait line is a `Button` (Button-base variation, so + `normal`/`hover`/`pressed` actually skin it) with a `VBoxContainer` of Labels inside. + Every child needs `mouse_filter = 2` (IGNORE) or the Label swallows the click and the + card never toggles. `pressed` on a `toggle_mode` Button *is* the "chosen" state — no + script-side stylebox swapping. - **The `.tscn` owns the tree.** Author every node in the editor: containers, panels, labels, buttons, art-slot placeholders. Assign the shared theme on the diff --git a/docs/roadmap.md b/docs/roadmap.md index dd9df3a..626e5f5 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -71,13 +71,13 @@ Each screen is recreated as a Godot 4.7 Control-node scene against the mockups ( ### M3 — Visual foundation & shell ✅ - ✅ **Shared `Theme`** — palette tokens, the three font families, and reusable styleboxes (parchment card, dark panel, primary CTA, tab, item tile, tag/chip) from the mock README as a Godot `Theme` resource. Every screen pulls from it; build it first so the look is one system. Merged to `dev`. *§2: n/a (presentation) · depends on nothing · goal: one consistent visual identity.* - ✅ **Main Window shell (2a)** — the exploration HUD: isometric world-view slot + the permanent DM "narration book" (wired to the proven `/dm/narrate` loop + considering-state), the turn-order rail, minimap slot, the slide-in system dock (Inventory/Character/Quest/Map/Party/Spellbook toggles), and the bottom command bar (HP/MP/gold, consumables, End Turn). The frame everything else lives in. **Established the editor-first UI-scene convention** (ADR [0001](adr/0001-editor-first-ui-scenes.md); how-to in `client/docs/README.md`) and added Button-base `DockButton`/`ParchmentButton` + a base `RichTextLabel` prose style to the theme. *§2: state (client owns the HUD, consumes DM text) · depends on the DM loop (M2) · goal: the game's main screen, on screen.* -- ○ **Title screen** — new game / continue / load / settings; the entry point. Author it editor-first (ADR 0001). **⛨ saga:** a fourth entry path (*begin the next campaign of an existing saga*) lands later — don't build it, just don't hardcode the flow as `new game → creation → world` so it can't be added. *§2: n/a · depends on the Theme · goal: first impression, into the world.* +- ✅ **Title screen** — new game / continue / load / settings; the entry point. Author it editor-first (ADR 0001). Built and committed (`a77bf03`, `f071392`); this entry was stale — the roadmap had not caught up and M4-c would have been misled into thinking it still had to build one. **⛨ saga:** a fourth entry path (*begin the next campaign of an existing saga*) lands later — don't build it, just don't hardcode the flow as `new game → creation → world` so it can't be added. *§2: n/a · depends on the Theme · goal: first impression, into the world.* ### 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.* +- ✅ **Character creation UI (M4-b)** — the mock's screen over M4-a's model: four race cards and seven nameplate calling cards (blurb + mechanics shown only for the *chosen* calling), the **+3 additive spend pool** with `−` flooring at the rolled base, proficiency chips gated by the chosen calling's pool (plus the Human bonus-skill row), five ability cards — **never six, and the word "luck" appears nowhere on the screen** (§7) — and the DM origin panel composing race + calling fragments. Five mock/model reconciles settled in the spec's §4 (skill picking added, seven callings not five, blurb-on-chosen-only, `+`/`−` both present, the portrait `‹ ›` arrows cut for lacking art). **Architecture:** a pure, node-free `CreationDraft` (`RefCounted`) holds every rule — changing calling clears stale skill picks, changing race drops a now-granted pick, re-roll clears the spend and only the spend, `errors()` delegates to `NewGame.validate` rather than keeping a second copy of the rules; `CreationCopy` is a pure static formatter that derives every display string (hit die, armour word, saves, skill count) from the `Races`/`Callings`/`Skills` tables, so retuning a calling retunes its card without touching a string literal; the `.tscn` owns the layout per ADR 0001 and `character_creation.gd` only binds. **The §2 property holds without the screen having to be trustworthy:** the screen shows its five numbers by calling the same newly-public `NewGame.roll_attributes(seed)` that `construct` calls internally — the screen never hands a number to anything, only a seed and four choices; `construct` rolls the identical attributes again, itself, off the same seed. Re-roll is unlimited (the floor of 8 already prevents a dead primary), and every re-roll silently re-rolls the hidden LCK draw riding the same RNG stream — the screen says nothing about this, ever. Emits `creation_confirmed(creation: Dictionary)`; does not call `construct` itself, so a saga can synthesize the same Dictionary later and skip the scene entirely. **Test count: 250 → 302.** **The honest seams:** three rounds of adversarial review found ten guard-shaped assertions across this milestone that could not fail against the bug they named (substring collisions, a BBCode wrapper masking an empty ContentDB, a test asserting the script's own loop bound, a trivial origin fixture, assertions already true before the action under test ran, a theme-drift guard checking three hardcoded variations instead of all of them, a `Label` sweep that missed `RichTextLabel`, and tests that called handlers directly instead of pressing nodes) — all rewritten against the actual artifact and reverted-and-reconfirmed-red per `docs/traps.md`'s standard; the new species are folded into that file. *§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.* ### M5 — Tactical combat (gridless) diff --git a/docs/traps.md b/docs/traps.md index 9948052..c71b09f 100644 --- a/docs/traps.md +++ b/docs/traps.md @@ -119,6 +119,178 @@ 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. @@ -127,3 +299,19 @@ nothing changed that you did not intend. - 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.