Skip to content

Code review P5: polish — branch-encoding edge cases, graphics file validation, format docs, type hints#162

Open
HansGR wants to merge 21 commits into
ff6wc:devfrom
HansGR:code-review-p5-polish
Open

Code review P5: polish — branch-encoding edge cases, graphics file validation, format docs, type hints#162
HansGR wants to merge 21 commits into
ff6wc:devfrom
HansGR:code-review-p5-polish

Conversation

@HansGR

@HansGR HansGR commented Jun 13, 2026

Copy link
Copy Markdown

Fifth and final stacked code-review PR (by Anthropic Fable 5.0). Based on the P4 PR — merge P1–P4 first. This tier's own commits are 66f5612..c4da72f.

What's included (4 commits)

  • 66f5612LabelPointer branch-encoding edge cases. Two latent bugs in BRANCH_RELATIVE (no current branch hits them; verified by byte-identical builds): a branch distance of 0 fell through unhandled and encoded 0x00 instead of 0xff, and the range check was off by one on both ends — distance +128 (encodable as offset 127) was rejected while distance −128 (not encodable) was accepted and silently emitted as a forward branch. Both arms collapse to (distance − 1) % 256; the error message now reports the actual distance and valid range. Boundary tests added.
  • a6a23e3 — size validation for palette/sprite/portrait files. A malformed .pal/.bin (e.g. a bad community sprite submission) previously flowed silently into the ROM data. PaletteFile/SpriteFile now require whole colors/tiles, and the seed-path portrait and palette loads (which had no size handling at all) require the exact size of the slot they replace, raising a ValueError that names the file. Character sprites keep their deliberate pad/truncate behavior. All shipped graphics files are well-formed, so output is unchanged. Tests added.
  • 6005d31docs/SPRITE_FORMAT.md: the BGR15 color encoding, the SNES 4bpp planar tile packing (verified against the decoder in graphics/sprite_tile.py), expected file sizes, and the steps to add a custom sprite — so sprite authors no longer need to reverse-engineer the code.
  • c4da72f — type hints (annotation-only) on the core surface newcomers hit first: Space/Reserve/Allocate/Free/Write/Read, Heap, Label/LabelPointer, seed.py, valid_rom_file.py, and the utils helpers. Python 3.9-compatible.

Seed compatibility

Verified byte-identical ROMs (SHA-256) against the pre-change baseline for four flagsets: default, 20-flag heavy, objectives, and a custom-graphics flagset (-cpal/-cpor) that specifically exercises the newly validated load paths. Full unit suite: 84 tests passing.

After this series

The review's one deliberately deferred item is refactoring the import-time side effects (args/log/objectives/battle-package ROM writes) into explicit initialization — documented in P4 and worth its own design discussion.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr

claude added 21 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
The three least discoverable mechanisms in the codebase now have module
docstrings: args/ parses sys.argv and injects attributes at import time,
log/ performs file I/O at import time, and objectives/ replaces itself in
sys.modules with an Objectives instance. Also documented: Space's
class-level shared state and its two-pass label resolution, the
instruction-callable expansion, and the event loader's class-naming
convention (including the silent-skip behavior for mismatched names).

Comments/docstrings only; no code change.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
Human-facing companion to llms.md/agents.md: requirements, how a seed is
built phase by phase, the seed-reproducibility and flags-are-forever
rules, how to add a flag/event/objective, and style conventions. README
now links it and states the Python version requirement.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
0x3b18 (enemy level table) was independently hardcoded in battle/scaling.py,
battle/load_enemy_level.py, and twice in data/enemy_script_custom_commands.py.
New constants/battle_addresses.py is the single source of truth for it and
the per-battle scale-level variables (0x3ecc-0x3ece).

The constants live in constants/ rather than battle/ deliberately: importing
anything from the battle package executes battle/__init__.py, whose imports
perform rom writes at import time. An import from data/ would run those
writes during the data phase and shift every later dynamic allocation
(~15k bytes of pointer changes). constants/ is side-effect free.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
_battle_condition.py and _menu_condition.py were identical except for the
word battle/menu in their space descriptions. The implementation now lives
once in _asm_condition.py; the two modules subclass it setting only
condition_type. The variants remain distinct classes deliberately:
_CachedFunction caches written routines per class, so merging them would
change the rom layout. Space descriptions are preserved exactly.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
rgb_data, write_ppm, and get_ppm each computed the same dimensions and rgb
matrix; extracted into one _pose_rgb helper. Also removes the two unused
'import graphics.poses' statements. Rendering tests added.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
- remove commented-out code: imperial_base.py branch alternatives (and the
  constant only they referenced), espers.py alternative Raiden dialog,
  items.py disabled log loop
- remove unused imports: kefka_tower.py 'import args' (events use
  self.args), starting_gold_items.py 'import random'
- args/graphics.py and args/objectives.py now define name() like the other
  31 flag modules, used at their existing section() calls
- objectives.py: hoist 'import sys' out of the two error handlers
- scaling.py: fix 'averaage' -> 'average' in two help strings (user-facing
  help/metadata text only; flag values and seed output unaffected)

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
26 files had CRLF line endings (verified content-identical with
git diff --ignore-cr-at-eol). .gitattributes now enforces LF for source
and marks .pal/.bin graphics data binary so it is never diffed or
normalized.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
Two latent bugs in BRANCH_RELATIVE encoding (no current branch hits them,
verified by byte-identical builds):
- a branch distance of 0 (branch to own operand) fell through unhandled
  and encoded 0x00 instead of 0xff
- the range check was off by one on both ends: distance +128 (encodable
  as offset 127) was rejected, while distance -128 (not encodable) was
  accepted and silently emitted as a forward branch

Both arms collapse to the single expression (distance - 1) % 256, and the
error message now reports the actual distance and valid range. Boundary
tests added.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
A malformed .pal/.bin (e.g. a bad community sprite submission) previously
flowed silently into the rom data. Loads now raise ValueError naming the
file and the expected size: PaletteFile/SpriteFile require whole
colors/tiles, and the seed-path portrait and palette loads require the
exact size of the slot they replace (character sprites already pad or
truncate deliberately and are unchanged). All shipped graphics files are
well-formed, so output is unchanged; verified with a custom -cpal/-cpor
build. Tests added.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
docs/SPRITE_FORMAT.md specifies the BGR15 color encoding, the SNES 4bpp
planar tile packing (verified against graphics/sprite_tile.py), expected
file sizes, and the steps to add a custom sprite — so sprite authors no
longer need to reverse-engineer the code.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr
Annotation-only change covering the public surface newcomers hit first:
Space/Reserve/Allocate/Free/Write/Read, Heap, Label/LabelPointer,
seed_rng/generate_seed, valid_rom_file, and the utils helpers.

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 unit test suite covering ROM-independent logic, improves error handling by replacing assertions with descriptive exceptions, and adds file size validation for graphics and palette files. It also refactors the objective conditions to share a common assembly-based implementation and fixes a branch relative offset calculation bug. A review comment identifies a high-severity issue in the newly rewritten truncated_discrete_distribution utility, where boundary checks are bypassed if minimum or maximum is set to 0 due to falsy evaluation.

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 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 guard the boundary checks will fail if either limit is explicitly set to 0 (or 0.0), as 0 is falsy in Python. This causes the boundary check to be silently bypassed, potentially returning out-of-bounds values.

Instead, explicitly check if the limits are not None.

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