test_committed_tres_matches_builder has now been "fixed" three times (traps.md #12).
v1 hardcoded 3 checks; v2 iterated ThemeKeys.ALL but missed the six font roles; v3
added a meta-guard specifically to catch "the builder styles a thing guarded by
nothing" — but that meta-guard contained `if fresh.get_type_variation_base(variation)
== &"": continue`, which whitelisted exactly the case it was built to catch. The
builder styles RichTextLabel directly (build_game_theme.gd's _rich_text()), with no
set_type_variation, so it reports base "" and was skipped unconditionally — and
RichTextLabel is what renders every line of DM prose the creation screen shows
(_origin_text, _detail_blurb). Nothing compared its fonts, font size, or colour, nor
the theme-level default_font/default_font_size, against the committed .tres.
Adds ThemeKeys.BASE_TYPES for base Control types the builder styles directly, folds
it into the drift guard's coverage, replaces the meta-guard's continue with an
assertion that names the offending type, and adds the missing theme-level default
font/size comparison. Proved with three reverted breaks: a RichTextLabel font-size
edit, a theme-level default_font_size edit, and an unregistered "GhostRole" variation
— all three now go red and name the drifted property.
Also: strengthens test_creation_copy's error-copy sweep to assert a mapped error
actually reaches its authored line rather than silently falling through to the
generic UNSPOKEN fallback (CreationCopy.UNSPOKEN satisfied all four prior checks,
so a renamed validator string could regress silently); and removes two assertions
that were true by construction / already true before their test's action ran.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Final whole-branch review of M4-b. Six findings.
1. The player could press his way into an invalid draft. can_take_bonus()
refused an already-picked skill; can_pick() did not consider the bonus, so
the guard was one-directional. Take athletics as the human's bonus, pick it
again from the Sellsword's pool, and the draft is one NewGame.validate
rejects — the invalid draft "he did not cause and cannot see" that
set_calling's own comment forbids. Fixed in the DRAFT (the screen owns no
rules): can_pick() now mirrors can_take_bonus(), so the chip goes inert, the
idiom the pool row already uses for a race-granted skill. Recoverable: hand
the bonus back and the chip is live again. Both directions tested.
2. The theme drift guard STILL could not fail. ThemeKeys.ALL is stylebox
variations only, by its own docstring — so the six FONT roles the builder
sets fonts, sizes and colours for were entirely unguarded. Changing
Palette.CREAM and skipping the rebuild shipped a stale game_theme.tres with
the suite green (proven, on the committed test). Adds ThemeKeys.FONT_ROLES
beside ALL (never inside it — ALL's contract is relied on), compares fonts,
font sizes and font colours as well as styleboxes, and names the variation
AND the property that drifted. Plus a meta-guard that walks the builder's own
output and fails if a variation it styles is in neither collection — this is
the second time this guard has been fixed by "enumerate the right set", and
nothing was checking the set.
3. §13: the CTA label piped NewGame.validate's diagnostics straight to the
player. On the game's SECOND SCREEN he read "hedge_mage picks 2 skills, got
0". Now CreationCopy.error_line maps them to authored copy — "name yourself",
"choose two more proficiencies" (the number DERIVED from Callings.skill_count,
never written down), "one more proficiency, any of them — take it" — with an
authored fallback for anything unmapped, because §13 means the unmapped case
still speaks in voice. validate's strings are untouched: they are a contract,
so they are translated in the presentation layer, not rewritten.
4. §2 had no test that fails if the boundary is breached. The emit test compared
the signal to draft.to_creation() — both sides move together. Pins the seven
contract keys exactly, and feeds construct a hostile creation dict carrying an
18 in every stat, asserting the sheet is still roll_attributes(seed) + spend.
Every other test in test_new_game passed with that breach in place.
5. Three holes: the orphan sweep skipped PoolRow (a stray Pool6 rendered unbound
and visible, suite green — the exact bug the test exists to catch, in the one
row it forgot); the §7 sweep forbade "luck" but not Luck.BANDS' descriptors,
which are worse than the number (the player would re-roll until his Luck read
well — calculable Luck); and the DM panel's composition hardcodes the article
"a", so a vowel-initial calling would break it silently — guarded in the
content parity test, which names the template as the reason.
6. The calling card printed "talent second_wind". Four of seven talents carry an
underscore. Humanised, the way skill_label() already does. The test that
pinned the RAW form is moved to the full derived phrase ("talent second wind")
so it still fails if someone hardcodes a talent into the format string.
Every guard was proven by re-breaking the code and watching the named test go
red (docs/traps.md). Suite 302 -> 315, content build green, no new warnings.
Report: .superpowers/sdd/final-review-fix-report.md
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
roll_attributes(seed) is now the single implementation of the die — construct
calls it, and the creation screen (M4-b) will call it to DISPLAY the roll. So
the screen shows real numbers while still only ever handing over a seed (§2).
validate() goes public so the screen can gate its CTA on the real rules.
Neither takes a default parameter (traps.md #2).
The canon-log schema and NewGame code already migrated to race_id/
calling_id + allowed_callings, but two copies of the roster outside
that schema were missed, and no parity test guards them:
- docs/canon-log.md (the cross-boundary contract doc) still documented
the OLD player shape and allowed_classes. Anyone building a player
block from the doc would get a 422.
- .claude/skills/world-building's schema reference still emitted
allowed_classes with two dead calling ids (assassin, priest). Author
a new origin with that skill and it fails origin.schema.json
(additionalProperties: false, allowed_callings required) AND, if that
ever loosened, silently allows zero callings at runtime.
Also:
- add schema tests rejecting a dead class_id field and an unknown
calling_id (priest)
- guard NewGame._validate's container types (spend/skills) so a
malformed JSON round-trip (null spend, string skills) produces a
front-loaded error list instead of a GDScript runtime crash
- extend the no-mechanics-in-content test to db.races, not just
db.callings
- rewrite the roadmap's M4 bullet: the two contract migrations landed;
only the creation screen and title->creation->shell flow remain
client: 250/250. api: 74 passed, 2 skipped. content_build --check: clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
Finding 1 (important): test_same_seed_yields_an_identical_character only
compared two construct() runs against each other, so swapping dex/con in
Attributes.IDS still passed the whole suite while every persisted seed
would silently become a different character. Added a golden-vector test
that pins one fixed seed to its exact rolled attributes and hidden Luck,
so reordering Attributes.IDS or moving the Luck roll fails loudly instead
of silently rewriting every saved character. Proved it: swapped IDS,
watched the golden vector fail, restored, confirmed green.
Also: _validate_spend no longer lets a negative value mask the over-pool
check (continue after each error instead of falling through to `total +=
v`); non-integer spend values (float/bool/string) are now rejected instead
of coerced; a missing or non-integer seed is now a validation error instead
of silently defaulting to 0. GameState's header comment no longer
advertises "stats" it no longer holds.
243 -> 248 tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
construct() takes 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). The whole character is
reproducible from four primitives: seed, race_id, calling_id, spend. The rng
parameter is dropped — the seed is now the only source of randomness.
The roll is 3d6 with a hard floor of 8. Straight 3d6 puts ~9% on a primary the +3
pool cannot rescue, and a character bad at the one thing he is FOR is not grit —
it is the fine §7 forbids, paid for twenty hours.
LCK is unspendable, not merely hidden: spending on it is a validation error.
GameState.stats becomes GameState.sheet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
Three callers (main_window_shell.gd, narrate_harness.gd, npc_harness.gd)
still built LogPlayer with the OLD 3-arg shape (name, class_id,
luck_descriptor) after LogPlayer.new() was migrated to the 4-arg
(name, race_id, calling_id, luck_descriptor) shape. This was invisible
to both grep and the test suite: every ctor param carries a default, so
GDScript compiles a 3-arg call against a 4-arg signature without error —
it just silently mis-binds positionally. "sellsword" landed in the
race_id slot (rejected — not a race), the luck descriptor landed in the
calling_id slot (rejected — not a calling), and luck_descriptor itself
fell back to its default "". Every field in the emitted dict came out
empty, which 422s against the canon-log schema's minLength/enum
constraints. Grepping for class_id can't see this, because there is no
class_id token left anywhere — the bug is purely positional.
Also fixes prompts.py's _article(""), which returned "an" because
Python's "" in "aeiou" is True, producing a doubled-space/wrong-article
digest line when race_id is empty. _describe_player now builds its
descriptor from whichever of race/calling are present and omits the
clause entirely when both are absent.
Adds a schema-parity guard for origin.schema.json's inlined
allowed_callings enum, which duplicated the calling roster with no
runtime check tying it to Callings.IDS. Renames leftover "class"
vocabulary in test names/comments to "calling".
Regression coverage:
- client/tests/unit/test_entities.gd: ctor populates both race_id and
calling_id; a calling passed into the race_id slot (the exact shape
of the three broken call sites) yields an all-empty row.
- api/tests/test_prompts.py: empty-race and empty-race-and-calling
digest rendering, asserting no doubled space and no dangling article.
- client/tests/unit/test_schema_parity.gd: origin.schema.json's
allowed_callings enum matches Callings.IDS.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
class_id is the rulebook's word; the world has callings — that was the whole §17
reconcile. The contract the Narrator reads should say what the world says. There
are no saves (M9) and no deployed clients, so this is as cheap as it will ever be
and strictly more expensive every milestone after.
race_id reaches the log because the AI must be able to describe the player:
api/app/prompts.py rendered 'a sellsword' and now renders 'a beastfolk cutpurse'.
It humanizes the id (the model must never read 'hedge_mage') and picks the right
article ('an elf', not 'a elf').
LogPlayer's hardcoded CLASSES const is deleted — the roster is Callings.IDS. A
parity test reads the JSON Schema from the client and asserts its enum equals the
code, so the two sides of the HTTP boundary cannot drift.
The deserter allowed three callings that no longer exist. He now allows all seven;
a thematic gate is a content decision for the Greywater authoring.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
The three denominations are move tokens, not stored buckets — there is one
integer, so change-making never arises. The deserter is down to 47c: under a
silver, so he cannot cover a bed and the player sees it on turn one.
A parity test guards the code-side ratios against the content-side ids.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8
Whole-branch review flagged that NewGame.construct validated class and
origin refs but not player.name. The canon-log schema requires
player.name with minLength:1, so a missing/blank name previously
returned ok:true and produced a log the API would reject with 422.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
