Skip to content

Code review P3: robustness — loud failures instead of hangs, silent skips, and bare AssertionErrors#160

Open
HansGR wants to merge 10 commits into
ff6wc:devfrom
HansGR:code-review-p3-robustness
Open

Code review P3: robustness — loud failures instead of hangs, silent skips, and bare AssertionErrors#160
HansGR wants to merge 10 commits into
ff6wc:devfrom
HansGR:code-review-p3-robustness

Conversation

@HansGR

@HansGR HansGR commented Jun 13, 2026

Copy link
Copy Markdown

Third of five stacked code-review PRs (performed by Anthropic Fable 5, June, 2026). Based on the P2 PR — merge P1 and P2 first. This tier's own commits are 9e69bc4..471fbd8.

What's included (4 commits)

  • 9e69bc4 — validation asserts → informative exceptions in seed-generation paths. Asserts vanish under python -O and give users a bare AssertionError when they fire. Converted: the choose_reward gating-deadlock check (event/event_reward.py, the failure documented in agents.md — now names the reward types involved), the CHARACTER_ESPER_ONLY_REWARDS count validation (event/events.py), Space.clear() size mismatch (now reports both sizes), Set/ClearEventBit range checks (now also reject negative bits, which would corrupt the opcode), and args/starting_party.py user-input asserts (now parser.error() like other arg modules).
  • 7556821 — guards on unbounded retry loops. Three rejection-sampling loops could spin forever if their progress assumption broke: esper spell shuffle (data/espers.py) and shop shuffle (data/shops.py) now raise a descriptive error after 10,000 consecutive failed picks (false-trigger probability when progress is possible: ~(26/27)^10000 ≈ 10⁻¹⁶³) or when the candidate pool empties; the character-gating loop (event/events.py) raises a clear "deadlock" error where it previously crashed with a bare TypeError; utils/truncated_discrete_distribution.py's unbounded recursion became a capped loop — impossible bounds now raise ValueError instead of recursing forever (test added). The guards consume no RNG and the success paths are unchanged.
  • 72473eb — bare except: → specific exceptions in six args/ modules (ValueError for int conversion, AttributeError for non-string menu values), so real bugs in menu generation can no longer be swallowed.
  • 471fbd8 — Pillow import guard + UTF-8 log. The PNG conversion tools import PIL, contradicting the documented "standard library only" requirement — a missing install now produces a clear message, and agents.md documents the optional dependency. The spoiler log encoding was platform-dependent (cp1252 on Windows); now explicitly UTF-8.

Seed compatibility

Verified byte-identical ROMs (SHA-256) against the pre-change baseline for three flagsets, including -ess -sisr 25 which runs through both modified shuffle loops. Spoiler logs identical.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr

claude added 10 commits June 12, 2026 21:11
…e overflow corruption

Priority 1 items from the v1.4.3 code review:

- .gitignore: exclude *.smc/*.sfc ROM files (legal requirement) and
  generated test artifacts in tests/
- CI (.github/workflows/ci.yml): syntax check, unit tests, and wc.py -h
  smoke test on Python 3.9-3.13, plus a guard job that fails if any
  ROM-like or oversized file is ever committed
- tests/: new unit test suite (65 tests) covering the ROM-independent
  core: heap allocation/free/reserve, label and branch encoding,
  Space write/clear/conflict handling, compression round-trips,
  flatten/intersection/weighted_random/shuffle_if, seeding, ROM
  validation, and CLI parsing
- memory/space.py: validate bounds BEFORE writing in Space.write() so
  an overflowing write can no longer corrupt bytes beyond the end of
  its space; replace bare except clauses with except TypeError
- memory/errors.py: new RomSpaceError (subclasses MemoryError for
  backward compatibility) raised by Space.write() and Heap.allocate()
  instead of the builtin MemoryError
- agents.md / llms.md: updated error signature and test documentation

No RNG calls are added, removed, or reordered: seed output is unchanged.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
The setter stored (value - MAX_RUN_SUCCESS), a negative number, while the
getter computes (MAX_RUN_SUCCESS - raw). A set/get round trip returned the
wrong value and the negative raw value would corrupt the bit-packed byte at
init_data[21] on write.

Latent bug: nothing calls the setter today, so generated seeds are
unaffected; the first feature to use it would have silently corrupted
character init data. Regression tests added.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
The assertion message referenced bare suplex_train_quest_name instead of
cls.suplex_train_quest_name, so a failed lookup raised NameError instead of
the intended diagnostic. Use an explicit RuntimeError so the check also
survives python -O.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
Delete an unreachable 'return opts' (opts was never defined) after the
options() return statement, and the first of two definitions of
_format_spells_log_entries, which was silently shadowed by the second.
No behavior change: the surviving definition is the one that already ran.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
Reward.type is initialized to Python None and RewardType.NONE is never
assigned anywhere, so the early-return could never trigger. By the time
mod() runs, all rewards have been assigned a real type.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
…, scaling description

- utils/compression.py: compress() printed an error and returned data with
  a truncated size header when output exceeded 65535 bytes, which would
  silently corrupt whatever consumed it; raise ValueError instead
  (regression test added)
- metadata/flag_metadata_writer.py: 'if type(group_title):' was always
  true (a type object is truthy); replace with an explicit str/None/callable
  dispatch that preserves the existing manifest output exactly
- battle/scaling.py: the xp/gp scaling routine was labeled 'scale hp/mp'
  in its space description (debug/error output only, not ROM content)

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
…tion paths

Asserts are stripped under python -O and give users a bare AssertionError
when they fire. Converted to explicit checks raising RuntimeError/ValueError
with context, and to parser.error() for user input validation:

- event/event_reward.py: the documented gating-deadlock failure in
  choose_reward() now reports which reward types were requested
- event/events.py: the CHARACTER_ESPER_ONLY_REWARDS count validation
- memory/space.py: clear() size mismatch now reports both sizes
- instruction/field/instructions.py: Set/ClearEventBit range checks
  (also reject negative bits, which would corrupt the opcode)
- args/starting_party.py: duplicate/too-many start characters now produce
  a clean CLI error like other arg modules instead of AssertionError

No control-flow change when checks pass; seed output unchanged.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
Three rejection-sampling loops could spin forever if their progress
assumption broke (all remaining candidates already hold the next
item/spell, or no gated slot is unlockable):

- data/espers.py shuffle_spells and data/shops.py shuffle: raise after
  10000 consecutive failed picks or when the candidate pool empties
- event/events.py character_gating_mod: raise a descriptive error when no
  unfilled character slot is unlocked (previously crashed with TypeError)
- utils/truncated_discrete_distribution.py: convert unbounded recursion to
  a capped loop; impossible bounds now raise ValueError (test added)

Guards consume no RNG and the success paths are unchanged, so seed
output is identical.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
- starting_gold_items.py: int() conversion failures are ValueError
- misc_magic/espers/scaling/lores/items menu(): .replace() on a non-string
  entry value raises AttributeError; catching everything could hide real
  bugs in menu generation

(misc_magic.py has CRLF line endings, preserved as-is.)

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
- graphics/tools/png_{portrait,sprite}.py import PIL, contradicting the
  documented 'standard library only' requirement; a missing install now
  produces a clear message instead of a bare ImportError. agents.md
  updated to document the optional dependency.
- log/__init__.py: spoiler log encoding was platform-dependent (cp1252 on
  Windows); now explicitly utf-8.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive suite of unit tests, improves error handling by replacing bare assertions and generic exceptions with specific errors, and fixes several bugs, including an incorrect calculation for character run success and potential infinite loops in randomizer shufflers. However, the review identified two critical issues: first, a return opts statement was accidentally deleted in args/challenges.py, which will break menu and metadata generation; second, the truthiness checks for bounds in utils/truncated_discrete_distribution.py will fail when a bound is set to 0, so explicit is not None checks should be used instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread args/challenges.py
Comment on lines 117 to 118
]

return opts
def _format_spells_log_entries(spell_ids):
from constants.spells import id_spell
spell_entries = []
for i, spell_id in enumerate(spell_ids):
spell_entries.append(("", id_spell[spell_id], f"rls_{i}"))
return spell_entries

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The return opts statement was accidentally removed from the options function while deleting the duplicate _format_spells_log_entries definition. This will cause options(args) to implicitly return None, breaking menu/metadata generation.

Suggested change
]
return opts
def _format_spells_log_entries(spell_ids):
from constants.spells import id_spell
spell_entries = []
for i, spell_id in enumerate(spell_ids):
spell_entries.append(("", id_spell[spell_id], f"rls_{i}"))
return spell_entries
]
return opts

Comment on lines +12 to +15
if minimum and result < minimum:
continue
if maximum and result > maximum:
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using if minimum and if maximum to check if bounds are specified will fail if minimum or maximum is 0, because 0 is falsy in Python. This would cause the bounds check to be skipped when 0 is used as a bound. Use is not None to safely check for bounds.

Suggested change
if minimum and result < minimum:
continue
if maximum and result > maximum:
continue
if minimum is not None and result < minimum:
continue
if maximum is not None and result > maximum:
continue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants