Code review P4: legibility — contributor docs, import-time side effects documented, dedup, dead code, line endings

[HansGR]

Fourth of five stacked code-review PRs (performed by Anthropic Fable 5). Based on the P3 PR — merge P1–P3 first. This tier's own commits are 2a8cf0d..3a2e6b0.

What's included (7 commits)

• 2a8cf0d — docstrings for the import-time magic (comments only). The three least discoverable mechanisms now explain themselves: args/ parses sys.argv and injects attributes into its own namespace at import time; log/ performs file I/O at import time; objectives/ replaces itself in sys.modules with an Objectives instance. Also documented: Space's class-level shared state, its two-pass label resolution, the instruction-callable expansion, and the event loader's class-naming convention (including that a mismatched class name is silently skipped).
• 04a4c5a — CONTRIBUTING.md (human-facing companion to llms.md/agents.md): requirements (Python 3.9+), the seed-build pipeline phase by phase, the two cardinal rules (RNG call order is part of the seed format; flags are never removed/renamed), how to add a flag module / event / objective condition, and style conventions. README links it.
• 85b6956 — constants/battle_addresses.py: the enemy-level table 0x3b18 was independently hardcoded in four places across three files; now one named constant. 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; we hit exactly this during development and verified the fix).
• c7891d7 — objective condition dedup: _battle_condition.py and _menu_condition.py were identical except for one word in their space descriptions. 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.
• 10ccb1c — sprite rendering dedup: rgb_data/write_ppm/get_ppm each computed the same dimensions and RGB matrix; extracted into one _pose_rgb helper, with rendering tests.
• c0bc8c7 — dead code sweep: commented-out blocks removed (imperial_base.py, espers.py, args/items.py); unused imports removed (kefka_tower.py, starting_gold_items.py); name() added to the two flag modules missing it; import sys hoisted out of error handlers; "averaage" → "average" in two help strings (user-facing help/metadata text only).
• 3a2e6b0 — line endings: 26 files had CRLF endings; all normalized to LF (verified content-identical with git diff --ignore-cr-at-eol), and .gitattributes now enforces LF for source and marks .pal/.bin graphics data binary.

Seed compatibility

Verified byte-identical ROMs (SHA-256) against the pre-change baseline for four flagsets: default, 20-flag heavy, a 4-objective flagset (exercising the deduplicated battle/menu condition classes), and a shuffle flagset. Spoiler logs identical.

https://claude.ai/code/session_01QnF26DB2TYi1hpJE8L6EJr

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive suite of unit tests, replaces several assertions with robust runtime exceptions, refactors shared assembly-based objective conditions, and fixes multiple bugs, such as character run success bit-packing. The code review feedback focuses on improving robustness and performance: it highlights a bug in truncated_discrete_distribution where falsy bounds (like 0) are ignored, suggests upfront validation of minimum/maximum bounds, recommends deterministic deadlock detection using all() instead of retry counters in shuffle_spells and shop shuffling, and advises optimizing list intersection complexity to O(N + M) by utilizing a set.

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.

[gemini-code-assist]

In Python, 0 is falsy. If minimum or maximum is passed as 0, the checks if minimum and if maximum will evaluate to False, causing the bounds to be ignored. Use explicit is not None checks instead.

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

[gemini-code-assist]

If minimum is greater than maximum, the range is impossible, and the loop will run for MAX_ATTEMPTS (10,000 iterations) before raising a ValueError. Validating this upfront avoids unnecessary CPU cycles.

Suggested change

	def truncated_discrete_distribution(mean, stddev, minimum = None, maximum = None):
	import random
	def truncated_discrete_distribution(mean, stddev, minimum = None, maximum = None):
	if minimum is not None and maximum is not None and minimum > maximum:
	raise ValueError(f"truncated_discrete_distribution: minimum ({minimum}) cannot be greater than maximum ({maximum})")
	import random

[gemini-code-assist]

Instead of using a retry counter (stalled_picks) and a hard limit of 10,000 attempts, we can deterministically check if all available espers already know the spell using all(). This avoids wasting RNG calls and CPU cycles on futile random selections when a deadlock occurs.

Suggested change

	stalled_picks = 0
	MAX_STALLED_PICKS = 10000
	while len(spells) > 0:
	if not esper_indices or stalled_picks > MAX_STALLED_PICKS:
	raise RuntimeError(f"shuffle_spells: cannot place {len(spells)} remaining spells, "
	f"{len(esper_indices)} espers have open slots")
	esper_index = random.choice(esper_indices)
	esper = self.espers[esper_index]
	if not esper.has_spell(spells[-1].id):
	stalled_picks = 0
	spell = spells.pop()
	if self.args.esper_spells_shuffle_random_rates:
	esper.add_spell(spell.id, random.choice(Esper.LEARN_RATES))
	else:
	esper.add_spell(spell.id, spell.rate)
	if esper.spell_count == spell_counts[esper_index]:
	esper_indices.remove(esper_index)
	else:
	stalled_picks += 1
	while len(spells) > 0:
	if not esper_indices:
	raise RuntimeError(f"shuffle_spells: cannot place {len(spells)} remaining spells, no espers have open slots")
	if all(self.espers[i].has_spell(spells[-1].id) for i in esper_indices):
	raise RuntimeError(f"shuffle_spells: cannot place {len(spells)} remaining spells, all {len(esper_indices)} espers with open slots already know {spells[-1].id}")
	esper_index = random.choice(esper_indices)
	esper = self.espers[esper_index]
	if not esper.has_spell(spells[-1].id):
	spell = spells.pop()
	if self.args.esper_spells_shuffle_random_rates:
	esper.add_spell(spell.id, random.choice(Esper.LEARN_RATES))
	else:
	esper.add_spell(spell.id, spell.rate)
	if esper.spell_count == spell_counts[esper_index]:
	esper_indices.remove(esper_index)

[gemini-code-assist]

Similar to the esper spell shuffling, we can deterministically check if all available shops already stock the item using all(). This eliminates the need for stalled_picks and avoids wasting RNG calls.

Suggested change

	stalled_picks = 0
	MAX_STALLED_PICKS = 10000
	while len(items) > 0:
	if not shop_indices or stalled_picks > MAX_STALLED_PICKS:
	raise RuntimeError(f"shops shuffle: cannot place {len(items)} remaining items, "
	f"{len(shop_indices)} shops have open slots")
	shop_index = random.choice(shop_indices)
	shop = type_shops[shop_type][shop_index]
	if not shop.contains(items[-1]):
	stalled_picks = 0
	item = items.pop()
	shop.append(item)
	if shop.item_count == item_counts[shop_index]:
	shop_indices.remove(shop_index)
	else:
	stalled_picks += 1
	while len(items) > 0:
	if not shop_indices:
	raise RuntimeError(f"shops shuffle: cannot place {len(items)} remaining items, no shops have open slots")
	if all(type_shops[shop_type][i].contains(items[-1]) for i in shop_indices):
	raise RuntimeError(f"shops shuffle: cannot place {len(items)} remaining items, all {len(shop_indices)} shops with open slots already stock {items[-1]}")
	shop_index = random.choice(shop_indices)
	shop = type_shops[shop_type][shop_index]
	if not shop.contains(items[-1]):
	item = items.pop()
	shop.append(item)
	if shop.item_count == item_counts[shop_index]:
	shop_indices.remove(shop_index)

[gemini-code-assist]

Converting lst2 to a set first improves the lookup complexity from O(M) to O(1) per element, reducing the overall complexity of the intersection from O(N * M) to O(N + M). This is a standard performance best practice in Python.

Suggested change

	def intersection(lst1, lst2):
	lst3 = [value for value in lst1 if value in lst2]
	return lst3
	def intersection(lst1, lst2):
	set2 = set(lst2)
	return [value for value in lst1 if value in set2]