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

[HansGR]

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)

• 66f5612 — LabelPointer 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.
• 6005d31 — docs/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

[gemini-code-assist]

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.

[wrjones104]

PR #162 Review — Code Review P5: Polish

1. PR Summary & Grade

• Overall Grade: B (Minor Tweaks Needed)
• Executive Summary: The P5-specific payload (4 commits: 66f5612..c4da72f) is genuinely excellent — a correct off-by-one branch-encoding fix, defensive size validation on graphics loads, accurate format docs, and annotation-only type hints, all with tests and verified byte-identical ROMs. The only thing standing between this and an A is mechanical: the branch is CONFLICTING against dev because it replays the P1–P4 history that has already been squash-merged into dev (via #158/#159/#161). It needs a rebase onto current dev, not a code rework.

2. Must Change (Critical / Blockers)

None identified in the P5 code itself. The four P5 commits are logically sound (I verified the branch-encoding math and the graphics constants/imports by hand), and all 84 unit tests pass locally.

There is one merge-readiness blocker, but it is not a code defect:

• Location: Whole branch vs. base dev.
• Issue: mergeStateStatus: DIRTY / mergeable: CONFLICTING. The branch is 21 ahead / 17 behind dev. Conflicts are in memory/heap.py, memory/space.py, utils/compression.py, utils/intersection.py, tests/test_label.py, event/events.py, event/kefka_tower.py, data/espers.py, data/shops.py, args/*, instruction/field/instructions.py, agents.md, llms.md.
• Why: These conflicts come entirely from the stacked P1–P4 tiers, which were already merged into dev under different (squashed) commit hashes. I confirmed every file the P5 commits actually author (memory/label.py, graphics/palette_file.py, graphics/sprite_file.py, data/character_palettes.py, data/character_sprites.py, tests/test_graphics_files.py, docs/SPRITE_FORMAT.md, seed.py, valid_rom_file.py) is conflict-free against current dev. Only the type-hints commit touches two already-merged files (heap.py, space.py), which will conflict trivially.
• Suggested Fix: Rebase the branch onto current dev and drop the already-merged P1–P4 commits:

git rebase --onto origin/dev 3a2e6b0 code-review-p5-polish

(3a2e6b0 is the last pre-P5 commit; this replays only 66f5612..c4da72f.) After the rebase the diff should shrink to just the ~10 P5 files, and the 4017/2908 line churn — which is almost all inherited P1–P4 content — should largely disappear.

3. Could Change (Low Impact / Optional)

• memory/label.py — consider a brief inline note on the [-127, 128] asymmetry. The code comment explains the (distance − 1) derivation well, but the asymmetric bound (offset range [-128, 127] → distance range [-127, 128]) is exactly the kind of thing a future reader will second-guess. The boundary tests document it, but a one-line "(asymmetric because the encoded offset is distance−1)" at the check itself would help.
• graphics/palette_file.py / sprite_file.py — the not data or len(data) % N != 0 guard is slightly redundant. An empty file has len(data) == 0, and 0 % N == 0, so not data is what actually rejects empties — which is correct and intentional (you have a test for it). Fine as-is; just noting the not data clause is load-bearing, so keep it.
• data/character_palettes.py / character_sprites.py — the exact-size ValueError messages are good; consider mentioning the slot/character (e.g. include character/palette_index) so a bad community submission points the author at which portrait slot mismatched, not just the byte count.
• utils/intersection.py still ends with no trailing newline (pre-existing) — a harmless nit the type-hint edit touched but didn't fix.

4. General Notes & QA Check

• Praise:

  1. The branch-encoding fix is genuinely sharp work — it caught two latent bugs (distance-0 falling through to 0x00, and the asymmetric off-by-one that silently emitted −128 as a forward branch), collapsed both arms into one correct (distance − 1) % 256 expression, improved the error message, and added boundary tests that pin the exact [-127, 128] range. The "no current branch hits this, verified byte-identical" framing is exactly the right way to land a latent-bug fix.
  2. The graphics validation is appropriately surgical: it hardens the community-submission failure mode (malformed .pal/.bin silently corrupting ROM data) while explicitly preserving the deliberate pad/truncate behavior for character sprites. Knowing which loads should be strict vs. lenient shows real understanding of the surrounding code.

• Manual QA Testing Plan:

  1. Custom graphics happy path: Generate a seed with -cpal/-cpor using well-formed community sprite/palette files and diff the output ROM SHA-256 against a pre-PR baseline — should be byte-identical.
  2. Malformed-file rejection: Feed a truncated .bin (e.g. 32*4 + 1 bytes) and an odd-sized .pal into -cpal/-cpor; confirm you get a ValueError naming the file and expected size, not a corrupted ROM or a silent success.
  3. Wrong-size portrait/palette on the seed path: Point a portrait/palette load at a file that's the wrong exact size for its slot; confirm it raises with a clear message rather than emitting a garbled portrait.

• Questions for Contributor:

  1. This PR's body says "merge P1–P4 first," but dev already contains #158/#159/#161 (the P1–P4 tiers, squash-merged). Is the intent to rebase P5 directly onto current dev now, or is there a remaining tier not yet merged?
  2. For the seed-path portrait/palette loads you made exact-size-strict — are there any known community assets that intentionally ship a differently-sized portrait (relying on the old no-size-handling behavior), or is exact-size a safe universal invariant for those slots?

Bottom line: The P5 code is A-grade and its own files merge cleanly onto dev. Rebase to drop the already-merged P1–P4 history, and this is ready.