Code review P3: robustness — loud failures instead of hangs, silent skips, and bare AssertionErrors

.github/workflows/ci.yml

@@ -0,0 +1,47 @@
+name: CI
+
+on:
+  push:
+  pull_request:
+
+jobs:
+  rom-guard:
+    name: ROM safety guard
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Fail if ROM or oversized files are tracked
+        run: |
+          set -e
+          rom_files=$(git ls-files | grep -iE '\.(smc|sfc)$' || true)
+          if [ -n "$rom_files" ]; then
+            echo "::error::ROM files must never be committed (legal requirement):"
+            echo "$rom_files"
+            exit 1
+          fi
+          # largest legitimate source file is ~56KB; anything over 512KB is suspect
+          big_files=$(git ls-files -z | xargs -0 du -b 2>/dev/null | awk '$1 > 524288 {print $2 " (" $1 " bytes)"}')
+          if [ -n "$big_files" ]; then
+            echo "::error::Unexpectedly large files tracked (possible ROM or binary data):"
+            echo "$big_files"
+            exit 1
+          fi
+
+  test:
+    name: Tests (Python ${{ matrix.python-version }})
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        python-version: ["3.9", "3.10", "3.11", "3.12", "3.13"]
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: ${{ matrix.python-version }}
+      - name: Syntax check all modules
+        run: python -m compileall -q .
+      - name: Run unit tests
+        run: python -m unittest discover -s tests -v
+      - name: CLI smoke test
+        run: python wc.py -h

.gitignore

@@ -3,4 +3,14 @@ __pycache__/
 $py.class
 *-objective.json
 *-flag.json
-wip/
+wip/
+
+# ROM files must never be committed (legal requirement)
+*.smc
+*.sfc
+*.SMC
+*.SFC
+
+# generated test artifacts (seed logs, manifests) - see agents.md "Test Data Isolation"
+tests/*.txt
+tests/*.json

agents.md

@@ -12,6 +12,8 @@ This document is written for autonomous AI coding agents (such as Antigravity, S
 ### 1.1 Python Dependencies
 The randomizer runs entirely on Python 3 with the standard library. No external pip installations are required.
 
+Exception: the optional developer tools `graphics/tools/png_portrait.py` and `graphics/tools/png_sprite.py` (PNG-to-sprite conversion) require Pillow (`pip install Pillow`). The randomizer itself never imports it.
+
 ### 1.2 Target ROM File
 To verify ROM modifications and successfully run a seed generation test, a valid Super Nintendo FF6 ROM file is required:
 - **Filename**: ff3.smc (located in the workspace root).
@@ -24,9 +26,16 @@ To verify ROM modifications and successfully run a seed generation test, a valid
 
 ### 1.3 Test Data Isolation
 > [!IMPORTANT]
-> **TEST DATA DIRECTORY RULE**: All test output data (including modified test ROMs, debug log text files, or API metadata manifests generated during your test runs) **MUST** be isolated and placed inside a `tests` directory in the workspace root (e.g., `tests/test_output.smc`, `tests/test_output.txt`).
+> **TEST DATA DIRECTORY RULE**: All test output data (including modified test ROMs, debug log text files, or API metadata manifests generated during your test runs) **MUST** be isolated and placed inside the `tests` directory in the workspace root (e.g., `tests/test_output.smc`, `tests/test_output.txt`).
 > - Do not write test output files directly to the root directory.
-> - Ensure the `tests` directory is created before running commands (e.g. `mkdir -Force tests`).
+> - The `tests` directory also contains the committed unit test suite (`tests/test_*.py`); generated artifacts there are excluded from git via `.gitignore` (`*.smc`, `*.sfc`, `tests/*.txt`, `tests/*.json`).
+
+### 1.4 Unit Tests
+The `tests/` directory contains a unit test suite for the ROM-independent logic (memory allocation, label/branch encoding, compression, seeding, CLI parsing). It requires no ROM file:
+```powershell
+python3 -m unittest discover -s tests -v
+```
+Run this after any change to `memory/`, `utils/`, `seed.py`, or `valid_rom_file.py`, and add tests for new pure-Python logic. The same suite runs in CI (`.github/workflows/ci.yml`) on every push and pull request, along with `python -m compileall`, `wc.py -h`, and a guard that fails if any ROM-like or oversized file is ever committed.
 
 ---
 
@@ -64,8 +73,9 @@ During development, agents commonly trigger three specific errors. Here is how t
 ### 3.1 Memory Overflow / Bank Exhaustion
 **Error Signature**:
 ```text
-MemoryError: Not enough room in space "custom event toggle": Next (0xc0f124) > End (0xc0f100). Diff: 36
+memory.errors.RomSpaceError: Not enough room in space "custom event toggle": Next (0xc0f124) > End (0xc0f100). Diff: 36
 ```
+(`RomSpaceError` subclasses `MemoryError`, so older code or docs referring to `MemoryError` still apply. The overflow is detected *before* any bytes are written, so the ROM buffer is never corrupted by an overflowing write.)
 **Cause**: You have written more bytes of assembly instructions or static data than the target `Reserve` range fits, or have exceeded the size of a dynamically requested `Allocate` block in that bank.
 **Resolution**:
 1. Check the size parameters in your dynamic allocation: `Allocate(Bank.C0, size, "description")`. Increase `size` to support your assembly array length.

args/challenges.py

@@ -115,14 +115,6 @@ def options(args):
         ("Remove Learnable Spells", args.remove_learnable_spell_ids, "remove_learnable_spell_ids"),
         ("No Saves", args.no_saves, "no_saves"),
     ]
-
-    return opts
-def _format_spells_log_entries(spell_ids):
-    from constants.spells import id_spell
-    spell_entries = []
-    for i, spell_id in enumerate(spell_ids):
-        spell_entries.append(("", id_spell[spell_id], f"rls_{i}"))
-    return spell_entries
 
 def _format_spells_log_entries(spell_ids):
     from constants.spells import id_spell

args/espers.py

@@ -204,8 +204,8 @@ def menu(args):
             if key == "Equipable":
                 value = value.replace("Random", "")
             entries[index] = (key, value, unique_name)
-        except:
-            pass
+        except AttributeError:
+            pass # value is not a string (e.g. a submenu), leave entry unchanged
     return (name(), entries)
 
 def log(args):

args/items.py

@@ -247,8 +247,8 @@ def menu(args):
             value = value.replace("Original + Random", "Original + ")
             value = value.replace("Shuffle + Random", "Shuffle + ")
             entries[index] = (key, value, unique_name)
-        except:
-            pass
+        except AttributeError:
+            pass # value is not a string (e.g. a submenu), leave entry unchanged
 
     return (name(), entries)
 

args/lores.py

@@ -86,8 +86,8 @@ def menu(args):
                 value = value.replace("Random Value ", "")
                 value = value.replace("Random Percent ", "")
             entries[index] = (key, value, unique_name)
-        except:
-            pass
+        except AttributeError:
+            pass # value is not a string (e.g. a submenu), leave entry unchanged
     return (name(), entries)
 
 def log(args):

args/misc_magic.py

@@ -53,8 +53,8 @@ def menu(args):
                 value = value.replace("Random Value ", "")
                 value = value.replace("Random Percent ", "")
             entries[index] = (key, value, unique_name)
-        except:
-            pass
+        except AttributeError:
+            pass # value is not a string (e.g. a submenu), leave entry unchanged
     return (name(), entries)
 
 def log(args):

args/scaling.py

@@ -310,8 +310,8 @@ def menu(args):
             value = value.replace("Characters + Espers", "C + E")
             value = value.replace("Bosses + Dragons", "B + D")
             entries[index] = (key, value, unique_name)
-        except:
-            pass
+        except AttributeError:
+            pass # value is not a string (e.g. a submenu), leave entry unchanged
     return (name(), entries)
 
 def log(args):

args/starting_gold_items.py

@@ -55,23 +55,23 @@ def __init__(self, _nid, min, max):
             item_id = 0
             try:
                 item_id = int(values[index])
-            except:
+            except ValueError:
                 args.parser.error(f"start-items: Failed to convert value into an int '{values[index]}'")
             if item_id < 0 or item_id >= 255:
                 args.parser.error(f"start-items: '{item_id}' is an invalid value for an item id. It must be between 0-254")
 
             min = 0
             try:
                 min = int(values[index + 1])
-            except:
+            except ValueError:
                 args.parser.error(f"start-items: Failed to convert value into an int '{values[index+1]}'")
             if min < 0 or min > 99:
                 args.parser.error(f"start-items: '{min}' is an invalid min for an item. It must be between 0 and 99")
 
             max = 0
             try:
                 max = int(values[index + 2])
-            except:
+            except ValueError:
                 args.parser.error(f"start-items: Failed to convert value into an int '{values[index+2]}'")
             if max <= 0 or max > 99:
                 args.parser.error(f"start-items: '{max}' is an invalid count for an item. It must be between 1-99")

args/starting_party.py

@@ -35,10 +35,12 @@ def process(args):
         args.start_chars = ["random"]
     else:
         # ensure only 4 starting characters and no duplicates (except random)
-        assert len(args.start_chars) <= 4
+        if len(args.start_chars) > 4:
+            args.parser.error("starting-party: at most 4 starting characters allowed")
         start_chars_found = set()
         for char in args.start_chars:
-            assert (char == "random" or char == "randomngu" or char not in start_chars_found)
+            if char not in ("random", "randomngu") and char in start_chars_found:
+                args.parser.error(f"starting-party: duplicate starting character '{char}'")
             start_chars_found.add(char)
 
 def flags(args):

battle/scaling.py

@@ -185,7 +185,7 @@ def scale_xp_gp_mod(self):
             asm.STA(0xea, asm.DIR),
             asm.JMP(self.scale_value, asm.ABS),
         ]
-        space = Write(Bank.C2, src, "scale hp/mp")
+        space = Write(Bank.C2, src, "scale xp/gp")
         self.scale_xp_gp = space.start_address
 
         if args.xp_gp_scaling:

data/character.py

@@ -85,7 +85,7 @@ def init_run_success(self, value):
         if value < self.MIN_RUN_SUCCESS or value > self.MAX_RUN_SUCCESS:
             raise ValueError(f"Character.init_run_success setter: invalid value {value}")
 
-        self._init_run_success = value - self.MAX_RUN_SUCCESS
+        self._init_run_success = self.MAX_RUN_SUCCESS - value
 
     # initial level of characters is 3
     # when new character is recruited, their level is set to the average of all other recruited characters + init_level_factor

data/espers.py

@@ -101,17 +101,29 @@ def shuffle_spells(self):
 
         random.shuffle(spell_counts)
 
+        # if every esper with open slots already knows the next spell, the loop
+        # below can no longer make progress; fail loudly instead of hanging.
+        # the guards consume no rng so seed output is unchanged
+        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
 
     def randomize_spells(self):
         for esper in self.espers:

data/shops.py

@@ -69,14 +69,26 @@ def shuffle(self):
 
             random.shuffle(item_counts)
 
+            # if every shop with open slots already stocks the next item, the loop
+            # below can no longer make progress; fail loudly instead of hanging.
+            # the guards consume no rng so seed output is unchanged
+            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
 
     def random_tiered(self):
         def get_item(item_type, exclude = None):

event/event_reward.py

@@ -49,7 +49,9 @@ def choose_reward(possible_types, characters, espers, items):
 
     # tried all possible_rewards and none were available
     # probably running out of chars and espers and need to make item rewards possible for more events
-    assert(item_possible)
+    if not item_possible:
+        raise RuntimeError(f"choose_reward: no rewards available for types {possible_types}: "
+                           "characters/espers are exhausted and this slot does not allow items")
     return (items.get_good_random(), RewardType.ITEM)
 
 # Documentation from AtmaTek:

event/events.py

@@ -126,6 +126,10 @@ def character_gating_mod(self, events, name_event):
                     unlocked_slot_iterations.append(slot_iterations[slot])
 
             # pick slot for the next character weighted by number of iterations each slot has been available
+            if not unlocked_slots:
+                raise RuntimeError("character gating deadlock: "
+                                   f"{self.characters.get_available_count()} characters left to assign "
+                                   "but no unfilled character slots are currently unlocked")
             slot_index = weighted_reward_choice(unlocked_slot_iterations, iteration)
             slot = unlocked_slots[slot_index]
             slot.id = self.characters.get_random_available()
@@ -171,4 +175,5 @@ def validate(self, events):
         for event in events:
             char_esper_checks += [r for r in event.rewards if r.possible_types == (RewardType.CHARACTER | RewardType.ESPER)]
 
-        assert len(char_esper_checks) == CHARACTER_ESPER_ONLY_REWARDS, f"Number of char/esper only checks changed - Check usages of CHARACTER_ESPER_ONLY_REWARDS and ensure no breaking changes. Expected: {CHARACTER_ESPER_ONLY_REWARDS}, Actual: {len(char_esper_checks)}"
+        if len(char_esper_checks) != CHARACTER_ESPER_ONLY_REWARDS:
+            raise RuntimeError(f"Number of char/esper only checks changed - Check usages of CHARACTER_ESPER_ONLY_REWARDS and ensure no breaking changes. Expected: {CHARACTER_ESPER_ONLY_REWARDS}, Actual: {len(char_esper_checks)}")

event/whelk.py

@@ -16,9 +16,6 @@ def init_event_bits(self, space):
         )
 
     def mod(self):
-        if self.reward.type == RewardType.NONE:
-            return
-
         self.dialog_mod()
         self.entrance_event_mod()
         self.cleanup_mod()

graphics/tools/png_portrait.py

@@ -57,7 +57,11 @@ def write_sprite(output_prefix, sprite, tile_indices):
         output.write(bytes(sprite.data))
 
 def convert(image_path):
-    from PIL import Image
+    try:
+        from PIL import Image
+    except ImportError:
+        raise ImportError("this developer tool requires the Pillow library (pip install Pillow); "
+                          "the randomizer itself does not need it") from None
     image = Image.open(image_path)
 
     import os

graphics/tools/png_sprite.py

@@ -98,7 +98,11 @@ def write_sprite(output_prefix, sprite, tile_indices):
         output.write(bytes(sprite.data))
 
 def convert(image_path):
-    from PIL import Image
+    try:
+        from PIL import Image
+    except ImportError:
+        raise ImportError("this developer tool requires the Pillow library (pip install Pillow); "
+                          "the randomizer itself does not need it") from None
     image = Image.open(image_path)
 
     import os

instruction/field/instructions.py

@@ -626,7 +626,8 @@ def __str__(self):
 class SetEventBit(_Instruction):
     def __init__(self, event_bit):
         self.event_bit = event_bit
-        assert self.event_bit <= 0x6ff
+        if not 0 <= self.event_bit <= 0x6ff:
+            raise ValueError(f"SetEventBit: invalid event bit {hex(self.event_bit)}, must be within 0x000-0x6ff")
 
         opcode = 0xd0 + (self.event_bit // 0x100) * 2
         arg = self.event_bit & 0xff
@@ -638,7 +639,8 @@ def __str__(self):
 class ClearEventBit(_Instruction):
     def __init__(self, event_bit):
         self.event_bit = event_bit
-        assert self.event_bit <= 0x6ff
+        if not 0 <= self.event_bit <= 0x6ff:
+            raise ValueError(f"ClearEventBit: invalid event bit {hex(self.event_bit)}, must be within 0x000-0x6ff")
 
         opcode = 0xd1 + (self.event_bit // 0x100) * 2
         arg = self.event_bit & 0xff

llms.md

@@ -183,6 +183,8 @@ All generated test data, output ROMs, log text files, and metadata manifests gen
   - Output test ROM: `tests/test_output.smc`
   - Output test Log: `tests/test_output.txt`
   - Manifest file: `tests/test_manifest.json`
+- **Unit Tests**: `tests/` also contains the committed unit test suite (`tests/test_*.py`) covering ROM-independent logic (memory allocation, label/branch encoding, compression, seeding, CLI parsing). Run it with `python3 -m unittest discover -s tests -v`; it requires no ROM file. CI (`.github/workflows/ci.yml`) runs it on every push/PR, plus a guard that rejects committed ROM-like files. Generated artifacts in `tests/` are gitignored (`*.smc`, `*.sfc`, `tests/*.txt`, `tests/*.json`); test source files are tracked.
+- **Memory Overflow Errors**: Space overflows raise `memory.errors.RomSpaceError` (a `MemoryError` subclass). Bounds are validated *before* writing, so an overflowing `space.write(...)` never corrupts adjacent ROM bytes.
 
 ---
 

log/__init__.py

@@ -8,7 +8,8 @@
     import sys
     logging.basicConfig(stream = sys.stdout, filemode = 'w', level = logging.INFO, format = "%(message)s")
 else:
-    logging.basicConfig(filename = log_file, filemode = 'w', level = logging.INFO, format = "%(message)s")
+    # explicit utf-8 so log content is identical across platforms (notably Windows)
+    logging.basicConfig(filename = log_file, filemode = 'w', level = logging.INFO, format = "%(message)s", encoding = "utf-8")
 
 hash = ', '.join([entry.name for entry in args.sprite_hash])
 import time