Code review P1: ROM gitignore protection, CI, unit tests, Space.write overflow fix#158
Code review P1: ROM gitignore protection, CI, unit tests, Space.write overflow fix#158HansGR wants to merge 1 commit into
Conversation
HansGR
requested review from
BriGuy7727,
bootherunner,
ff6wc and
wrjones104
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new RomSpaceError exception to handle memory allocation and out-of-bounds write errors safely, adds a comprehensive suite of unit tests, updates documentation, and implements a mechanism to relocate dried meat from the phantom train shop. A critical issue was identified in data/shops.py where removing a guard clause could lead to an IndexError when attempting to choose from an empty list of shops.
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.
| for index in range(number_shops_with_dried_meat, self.args.shop_dried_meat): | ||
| if not no_dried_meat_shops: | ||
| break | ||
| random_shop = random.choice(no_dried_meat_shops) |
There was a problem hiding this comment.
Removing the if not no_dried_meat_shops: break guard can cause an IndexError: Cannot choose from an empty sequence if self.args.shop_dried_meat is configured to a value greater than the number of available shops in no_dried_meat_shops. The guard should be restored to prevent crashes under certain flag configurations.
for index in range(number_shops_with_dried_meat, self.args.shop_dried_meat):
if not no_dried_meat_shops:
break
random_shop = random.choice(no_dried_meat_shops)There was a problem hiding this comment.
This could be resolved by rebasing to dev I think. Either way, it should be reverted.
wrjones104
left a comment
wrjones104
left a comment
There was a problem hiding this comment.
Everything looks good to me except the dried_meat shop deletion, probably just a carryover from the pre-1.4.3 merge
| for index in range(number_shops_with_dried_meat, self.args.shop_dried_meat): | ||
| if not no_dried_meat_shops: | ||
| break | ||
| random_shop = random.choice(no_dried_meat_shops) |
There was a problem hiding this comment.
This could be resolved by rebasing to dev I think. Either way, it should be reverted.
…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
HansGR
force-pushed
the
code-review-p1-infrastructure
branch
from
3bdbd2b to
2990323
Compare
Yes, I had some dead code in my main branch. I've now updated this and the subsequent ones to be based off dev branch, so it shouldn't be an issue anymore. |
3 participants
First of five stacked PRs from a full code review of v1.4.3, ordered by priority. Merge order: P1 → P2 → P3 → P4 → P5. Each later PR's diff includes its predecessors until they merge.
What's included (1 commit,
3bdbd2b).gitignoreROM protection —*.smc/*.sfc(and case variants) plus generated test artifacts are now ignored. Previously nothing stopped a ROM from being committed.CI workflow (
.github/workflows/ci.yml) — runs on every push/PR:.smc/.sfcfile or any file >512KB is tracked (largest legitimate source file is ~56KB, so no false positives)compileallsyntax check, the unit suite, and awc.py -hsmoke test that exercises all flag parsing. The version matrix would have caught the Python 3.11random.sample(set)breakage class automatically.Unit test suite (
tests/, 65 tests, no ROM required) covering the ROM-independent core: heap allocate/free/reserve/merge, label and 65c816 branch encoding,Spacewrite/clear/labels/conflict detection, compression round-trips, the utils helpers, seed determinism (locking in the "same seed + flags ⇒ same stream" contract), ROM hash validation, and a subprocess CLI test. Run withpython3 -m unittest discover -s tests.Space.write()overflow fix (memory/space.py) — bounds were previously checked afterrom.set_bytes(), so an overflowing write corrupted bytes beyond the end of its space before raising. Bounds are now validated first; a regression test asserts the adjacent byte survives. The error is nowRomSpaceError(newmemory/errors.py), deliberately a subclass ofMemoryErrorwith the identical message format so existing tooling that catches or greps the old error keeps working. Also: two bareexcept:clauses narrowed toexcept TypeError, andagents.md/llms.mdupdated to match.Seed compatibility
No RNG calls added, removed, or reordered. Verified by byte-identical before/after builds (SHA-256 compared) of the same seeds under the default flagset and a 20-flag heavy flagset (
-open+ scaling + character/esper/shop/chest randomization). Note CI cannot roll real seeds (no ROM, legally) — the byte-identical comparison remains a local maintainer step for behavior-relevant changes.https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr