ptarrant/code_of_conquest_dnd
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(creation-screen): the bonus-skill hole, the theme guard that could not fail, and the strings the player was reading

Browse Source 
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>
This commit is contained in:
Phillip Tarrant
2026-07-13 16:25:12 -05:00
parent 31759ef848
commit 34bf064904
10 changed files with 506 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
client/tests/unit/test_theme_resource.gd
View File

@@ -29,22 +29,56 @@ func test_default_font_is_set():
assert_true(t.default_font is FontFile)
func _guarded_variations() -> Array:
# EVERY variation the builder touches — the stylebox variations (ThemeKeys.ALL)
# AND the six font roles (ThemeKeys.FONT_ROLES), which ALL deliberately excludes
# by its own docstring. Iterating ALL alone left every font, font size and font
# colour in the theme unguarded: change Palette.CREAM (TitleLogo's font colour,
# and nothing else's), skip the rebuild, and the suite stayed green with a stale
# .tres shipped. traps.md #12 — "covers everything" must iterate the full set.
var out: Array = ThemeKeys.ALL.keys()
out.append_array(ThemeKeys.FONT_ROLES.keys())
return out
func test_the_drift_guard_covers_every_variation_the_builder_touches():
# The guard below is only as good as the set it walks. Pin that set to the
# builder's own output: every variation the fresh Theme carries a stylebox, a
# font, a font size or a font colour for MUST be in _guarded_variations(), or
# it can drift unwatched. A new variation added to the builder and forgotten in
# ThemeKeys goes red here rather than sitting unguarded forever.
var fresh: Theme = Builder.build_theme()
var guarded := _guarded_variations()
for variation in fresh.get_type_list():
var touched := (not fresh.get_stylebox_list(variation).is_empty()
or not fresh.get_font_list(variation).is_empty()
or not fresh.get_font_size_list(variation).is_empty()
or not fresh.get_color_list(variation).is_empty())
if not touched:
continue
if fresh.get_type_variation_base(variation) == &"":
continue # a BASE type (the builder styles RichTextLabel directly), not a variation
assert_true(variation in guarded,
"the builder styles the variation %s but no drift guard walks it — add it to ThemeKeys.ALL or ThemeKeys.FONT_ROLES" % variation)
func test_committed_tres_matches_builder():
# Catches "palette/builder changed but nobody regenerated game_theme.tres."
# Preloading the builder script and calling its static build_theme() does
# NOT run _init() (that only fires on .new()), so this is side-effect-free.
#
# Drives its checks off ThemeKeys.ALL so every variation is guarded, not just
# whichever three someone remembered to hardcode here. For each variation:
# every stylebox state the builder actually set (get_stylebox_list) is
# compared property-by-property when it's a StyleBoxFlat, and every font
# colour role the builder actually set (get_color_list) is compared too.
# Nothing the builder never touches is asserted on.
# Drives its checks off ThemeKeys.ALL *and* ThemeKeys.FONT_ROLES so every
# variation is guarded, not just whichever three someone remembered to hardcode
# here — and not just the stylebox half. For each variation: every stylebox
# state, every font, every font size and every font colour the builder actually
# set is compared. Nothing the builder never touches is asserted on.
var fresh: Theme = Builder.build_theme()
var committed: Theme = load(THEME_PATH)
for variation in ThemeKeys.ALL:
for variation in _guarded_variations():
for state in fresh.get_stylebox_list(variation):
assert_true(committed.has_stylebox(state, variation),
"stale game_theme.tres — %s/%s stylebox is missing entirely; re-run build_game_theme.gd" % [variation, state])
var fresh_box: StyleBox = fresh.get_stylebox(state, variation)
var committed_box: StyleBox = committed.get_stylebox(state, variation)
assert_eq(committed_box.get_class(), fresh_box.get_class(),
@@ -74,7 +108,28 @@ func test_committed_tres_matches_builder():
assert_eq(c.corner_radius_bottom_left, f.corner_radius_bottom_left,
"stale game_theme.tres — %s/%s corner_radius_bottom_left drifted from the builder; re-run build_game_theme.gd" % [variation, state])
# The FONT half — the whole reason this guard could not fail against a font
# role. Compare the font's resource_path, not the Resource: `load()` caches,
# so two references to the same .ttf are the same instance and `==` would be
# true even when the builder swapped SERIF for MONO... only if the .tres
# happened to point at the same file anyway. The path is the thing that drifts.
for font_name in fresh.get_font_list(variation):
assert_true(committed.has_font(font_name, variation),
"stale game_theme.tres — %s/%s font is missing entirely; re-run build_game_theme.gd" % [variation, font_name])
var fresh_font: Font = fresh.get_font(font_name, variation)
var committed_font: Font = committed.get_font(font_name, variation)
assert_eq(committed_font.resource_path, fresh_font.resource_path,
"stale game_theme.tres — %s/%s font drifted from the builder; re-run build_game_theme.gd" % [variation, font_name])
for size_name in fresh.get_font_size_list(variation):
assert_true(committed.has_font_size(size_name, variation),
"stale game_theme.tres — %s/%s font size is missing entirely; re-run build_game_theme.gd" % [variation, size_name])
assert_eq(committed.get_font_size(size_name, variation), fresh.get_font_size(size_name, variation),
"stale game_theme.tres — %s/%s font size drifted from the builder; re-run build_game_theme.gd" % [variation, size_name])
for color_name in fresh.get_color_list(variation):
assert_true(committed.has_color(color_name, variation),
"stale game_theme.tres — %s/%s colour is missing entirely; re-run build_game_theme.gd" % [variation, color_name])
var fresh_color: Color = fresh.get_color(color_name, variation)
var committed_color: Color = committed.get_color(color_name, variation)
assert_eq(committed_color, fresh_color,