Code review P2: small verified bug fixes (zero seed impact)#159
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests under the tests/ directory, updates documentation, and refactors memory handling by introducing a specific RomSpaceError to prevent out-of-bounds ROM corruption. It also addresses several bugs, such as correcting the character run success setter logic, removing duplicate functions, and refining shop item swapping. Feedback on the changes highlights a critical issue in data/shops.py where removing a safety check on no_dried_meat_shops could result in an IndexError if the list becomes empty.
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 safety check if not no_dried_meat_shops: break can lead to an IndexError when self.args.shop_dried_meat is configured to be larger than the number of available shops (e.g., due to exclusions or empty shops). It is highly recommended to restore this guard to prevent potential crashes during seed generation.
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 branch was not correctly rebased to dev prior to the pull request, which caused this issue. This and all other pull requests in this series should now be correctly rebased to dev.
…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
8fbdb37 to
c8b1d38
Compare
Second of five stacked code-review PRs. Based on the P1 infrastructure PR — merge that first; until then this diff includes P1's commit. This tier's own commits are
7573784..8fbdb37.What's included (5 commits, one per fix)
7573784—data/character.py: invertedinit_run_successsetter. It storedvalue - MAX_RUN_SUCCESS(negative) where the getter expectsMAX_RUN_SUCCESS - value; a set/get round-trip returned the wrong value and the negative raw value would corrupt the bit-packed byte atinit_data[21]. Latent: nothing calls the setter today, so no seeds are affected — but the first feature to use it would silently corrupt character init data. Regression tests added.2fdc731—objectives/objective.py:NameErrorin a failure path. The assert message referenced baresuplex_train_quest_nameinstead ofcls.suplex_train_quest_name, so a failed quest lookup crashed withNameErrorinstead of the intended diagnostic. Now an explicitRuntimeError(also survivespython -O).8359042—args/challenges.py: dead code. An unreachablereturn opts(optswas never defined) and a duplicate definition of_format_spells_log_entrieswhose first copy was silently shadowed. The surviving definition is the one that already ran, so no behavior change.59d8092—event/whelk.py: dead check.self.reward.type == RewardType.NONEcould never be true:Reward.typeis initialized to PythonNoneandRewardType.NONEis never assigned anywhere (verified by grep).8fbdb37— three silent-failure fixes:utils/compression.pyprinted an error and returned data with a truncated size header when output exceeded 65535 bytes (silent corruption for the consumer) — now raisesValueError, with a test;metadata/flag_metadata_writer.pyhadif type(group_title):which is always true (a type object is truthy) — replaced with explicit str/None/callable dispatch preserving the manifest output exactly;battle/scaling.pylabeled the xp/gp routine "scale hp/mp" in its space description (debug output only).Seed compatibility
Verified byte-identical ROMs (SHA-256) against the pre-change baseline for: the default flagset, a 20-flag heavy flagset, and a
-rls ultima,quakebuild specifically exercising the modifiedchallenges.pylog path. Flag-metadata JSON output is also byte-identical. Spoiler logs identical.https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr