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

.gitattributes

@@ -0,0 +1,7 @@
+# normalize source line endings (checkout and commit as LF)
+*.py text eol=lf
+*.md text eol=lf
+
+# graphics data is binary; never diff or normalize it
+*.pal binary
+*.bin binary

.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

CONTRIBUTING.md

@@ -0,0 +1,100 @@
+# Contributing to Worlds Collide
+
+Worlds Collide is community-maintained. This guide covers what you need to
+know to make changes safely. For deeper architectural reference (memory
+model, assembly hooks, troubleshooting), see [llms.md](llms.md) and
+[agents.md](agents.md).
+
+## Requirements
+
+- Python 3.9+ (CI tests 3.9 through 3.13), standard library only.
+  Exception: the optional PNG conversion tools in `graphics/tools/` need
+  Pillow.
+- An unheadered FFIII US v1.0 ROM (3,145,728 bytes) for generating seeds.
+  **Never commit a ROM** — `.gitignore` and CI both guard against it, for
+  legal reasons.
+
+## Running and testing
+
+```sh
+python3 wc.py -i ff3.smc -o tests/test_output.smc   # roll a seed
+python3 wc.py -h                                     # exercise all flag parsing
+python3 -m unittest discover -s tests                # unit tests (no ROM needed)
+```
+
+Generated artifacts (`.smc`, logs) belong in `tests/`, which is gitignored
+for everything except the unit test sources. CI runs the unit tests, a
+syntax check, and `wc.py -h` on every push/PR.
+
+## The two cardinal rules
+
+1. **Seed reproducibility.** The same seed + flags must produce the same ROM,
+   forever. The flag string seeds Python's global `random`, so the *order and
+   number of RNG calls* is effectively part of the seed format. Reordering
+   loops, filtering before shuffling, adding an early `random.*` call — all
+   of these silently change every seed. When changing code near `random`
+   calls, verify before/after builds of the same seed are byte-identical
+   (`cmp old.smc new.smc`). Behavior-changing fixes are sometimes warranted,
+   but must be deliberate and called out in the PR.
+
+2. **Flags are forever.** Old flagsets must keep working with new versions,
+   so flags are never removed or renamed. New behavior gets a new flag.
+
+## How a seed is built
+
+`wc.py` runs the phases in order (see `wc.py:main`):
+
+1. `import args` — parses the command line (every module in
+   `args/arguments.py`'s group list), builds the canonical flag string, and
+   seeds the RNG. This happens at *import time*; see `args/__init__.py`.
+2. `import log` — creates the spoiler log (also at import time).
+3. `Memory()` — loads and validates the input ROM into `Space.rom`.
+4. `Data()` — reads game data structures (characters, espers, items, …)
+   from the ROM, applies flag-driven randomization (`mod()` methods).
+5. `Events()` — discovers `event/*.py` by naming convention, distributes
+   character/esper/item rewards, rewrites event scripts.
+6. `Menus()`, `Battle()`, `Settings()`, `BugFixes()` — menu rewrites,
+   battle assembly patches, bug fixes.
+7. `data.write()` / `memory.write()` — serialize everything back and write
+   the output ROM.
+
+ROM writes go through the memory model in `memory/space.py`: `Reserve()`
+fixed vanilla ranges, or `Allocate()`/`Write()` dynamic space freed by
+`Free()`. Overflowing a space raises `RomSpaceError` — see agents.md
+("Memory Overflow / Bank Exhaustion") for resolutions.
+
+## Adding things
+
+**A flag:** each module in `args/` implements the same interface, called by
+`args/arguments.py` and the menu/log systems: `parse(parser)` (add argparse
+arguments), `process(args)` (validate/derive values), `flags(args)` (rebuild
+the canonical flag string), `options(args)`, `menu(args)`, `log(args)`, and
+`name()`. Copy an existing small module (e.g. `args/sketch_control.py`) as a
+template, and register new modules in `Arguments.groups`. The flag's
+behavior is then implemented in `data/`, `event/`, etc. by checking
+`self.args.<dest>`.
+
+**An event:** create `event/<location>.py` containing a class whose name is
+the module name with underscores removed (e.g. `mt_kolts.py` → `MtKolts`),
+subclassing `Event`. A mismatched class name is *silently skipped*. If you
+add or remove character/esper reward slots, update
+`CHARACTER_ESPER_ONLY_REWARDS` (see agents.md).
+
+**An objective condition/result:** add the constant in
+`constants/objectives/`, then the implementation under
+`objectives/conditions/` or `objectives/results/`, following an existing
+pair like `quest.py`/`quests.py`.
+
+**Tests:** new ROM-independent logic (pure Python helpers, encodings,
+allocation) should come with unit tests in `tests/test_*.py`.
+
+## Style
+
+- Match the surrounding code. The codebase uses 4-space indents,
+  `snake_case`, and `key = value` spacing in calls.
+- Name magic ROM/WRAM addresses, or comment what they are — future readers
+  cannot grep for `0x3b18` and know it's the enemy level table.
+- Prefer raising descriptive exceptions over `assert` for anything that can
+  fail at seed-generation time (asserts vanish under `python -O`).
+- If your change affects module structure, core APIs, or the patterns
+  documented in `llms.md`/`agents.md`, update those files in the same PR.

README.md

@@ -12,10 +12,18 @@ Complete objectives while searching the worlds for characters, espers, and items
 
 ## Usage
 
+Requires Python 3.9+ and an unheadered FFIII US v1.0 ROM.
+
 ```sh
 $ python3 wc.py -i ffiii.smc
 ```
 
 ```sh
 $ python3 wc.py -h
 ```
+
+## Contributing
+
+See [CONTRIBUTING.md](CONTRIBUTING.md) for how the randomizer is put
+together, how to add flags/events, and the rules that keep old seeds
+reproducible. Never commit ROM files.

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/__init__.py

@@ -1,3 +1,19 @@
+"""Command line arguments, parsed and processed at import time.
+
+The first `import args` runs the full pipeline: parse sys.argv (every flag
+module listed in Arguments.groups), process/validate values, build the
+canonical flag string, seed the global random module (see seed.py), and
+compute the sprite hash. The resulting attributes are then injected into
+this module's namespace so flag values read as plain module attributes:
+
+    import args
+    if args.open_world:
+        ...
+
+Because parsing happens at import, importing this module (or any module
+that imports it, e.g. `log`) requires valid command line arguments —
+at minimum `-i INPUT_FILE`.
+"""
 from args.arguments import Arguments
 arguments = Arguments()
 

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/graphics.py

@@ -198,6 +198,9 @@ def _other_options_log(args):
 
     return log
 
+def name():
+    return "Graphics"
+
 def log(args):
     lcolumn = [""]
     lcolumn.extend(_sprite_palettes_log(args))
@@ -212,4 +215,4 @@ def log(args):
     rcolumn.extend(_character_customization_log(args))
 
     from log import section
-    section("Graphics", lcolumn, rcolumn)
+    section(name(), lcolumn, rcolumn)

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)
 
@@ -258,9 +258,6 @@ def log(args):
     log = [name()]
 
     entries = options(args)
-    '''for entry in entries:
-        log.append(format_option(*entry))
-    '''
     for entry in entries:
         key, value, unique_name = entry
         if key == "Item Rewards":

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):