Architecture review fixes: game rules off the event bus, save v4, game-over screens - #13
Merged
Merged
Conversation
Two game-over screen bugs from the architecture review (F4a, F4b), both from the UI guessing what a GAME_OVER meant: - On death the room scene reappeared for ~2.6s before the drain. The UI hears COMBAT_ENDED before the engine sets GAME_OVER, so the is_in_game_over() guard never fired, and the GAME_OVER reset called end_battle() again. The UI now reads the event's own data: defeat keeps the battle on screen and starts play_death immediately. - F5 and the post-win "new game"/"restore" choices showed the GAME OVER card 2.6s later over whatever came next. GAME_OVER now carries reason "defeat" or "restart"; only a defeat shows the card, and a restart returns to the title screen. - The death card said "Unknown Sysadmin": the reset cleared the player view it reads. The name is kept for the card. Tests drive the real engine and TUI together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The event bus logs and swallows a listener's exception, and ViewBuilder returns a placeholder view when a build fails. Both are right for the game (a panel degrades instead of crashing) and wrong for tests, where a crashed handler left a log line and a green test. EventBus takes strict=True (class default strict_by_default) and ViewBuilder has raise_errors; tests/conftest.py turns both on for every test. The game keeps the forgiving defaults. The full suite still passes strict, so no crash was hiding today; test_strict_mode pins that the switches stay wired. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Loading a save quietly changed the run (review findings F1, F2): - Player.to_dict never wrote equipped_armor or status_effects, so a load unequipped your armor (mitigation 0) and dropped active effects. Armor is re-equipped on load so mitigation is recomputed from the item rather than trusted from the file. - The run's difficulty was not saved, and mounting the UI reset it from user settings, so a Hard run reloaded as Medium. The save now records it and both load paths restore it; the UI no longer touches it. Save format is v4. v3 saves still load: medium difficulty, no armor, no effects. Also: everything the game opens (content, saves, logs, settings) is relative to the working directory, and some of it at import time. Launched from another directory the game could not find its content and left empty data/ and saves/ folders behind. main.py now moves to the repo before importing the game. test_every_player_field_survives_a_round_trip walks every Player attribute rather than listing them, so the next field added without save support fails there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Choosing "r" after a win restored the save but rebuilt the command handler without its event subscriptions (review finding F3): walking into a room with an enemy started no fight, NPCs said nothing after combat and tutorial combat hints stopped. It also never told the UI the run had started (no GAME_STARTED / ROOM_ENTERED) and left the state alone. Both load paths now go through _enter_loaded_run(), so neither can skip a step. Its failure fallback called ui.display_message, which the Textual UI did not implement (UIProtocol declares it); it does now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CommandHandler used to subscribe to ROOM_ENTERED to run the arrival rules (fled enemies come back, a hostile starts a fight). That tied game rules to event wiring (review F6, and the root of F3): - a load path that skipped the subscription silently turned encounters off; - UI-only refreshes (`ls -a` revealing a directory, the post-victory redraw) re-ran the encounter check. Now CommandHandler.arrive() is called directly wherever the player enters a room: cd, flee, new game, load. announce_room() is the UI-only notification, used by ls -a and the post-victory redraw. Nothing in the game listens to ROOM_ENTERED any more. cd and flee now show the room before any fight starts (they used to start the fight first, so the room text landed in the combat log), matching new game. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Four listeners reacted to COMBAT_ENDED (UI, engine, tutorial, handler) and the game's correctness depended on the order they had happened to subscribe in, across three files (review F5): the engine had to run before the handler on a flee, and the handler re-subscribed itself for every fight and unsubscribed mid-dispatch. CombatSession now takes on_start/on_end callbacks. It still emits COMBAT_STARTED/COMBAT_ENDED for observers (UI, tutorial), then calls CommandHandler.end_combat(outcome), which runs one fixed sequence: the engine's state hook (combat over, or game over on a death), then game over / flee relocation / victory check. The engine hands its hooks to every handler through _new_command_handler(), so no run can come up without them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A kill emitted ENEMY_DEFEATED; the handler's listener awarded loot and removed the enemy, and the removal emitted ENEMY_DEFEATED again (with a different payload) from inside its own handling, plus a nested ALL_ENEMIES_DEFEATED. Only a dedupe set kept loot from being rolled twice (review F7). CombatSession now calls CommandHandler.on_kill(enemy_id) directly after announcing the kill: loot, removal, and the NPC post-combat line once the room is clear. GameWorld.remove_enemy_from_room no longer emits anything (GameWorld no longer needs a bus), ALL_ENEMIES_DEFEATED is gone, and ENEMY_DEFEATED fires once per kill, for the UI. Tests that faked ENEMY_DEFEATED to trigger loot now call on_kill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The game-over and post-win screen's "n" and "r" choices were sent to the engine as a GAME_OVER event with an "action" field, so one event meant both "you died" (for the UI) and "start over" (for the engine). That double meaning is what put a GAME OVER card over the next screen. GameFlow now takes the engine's start-new-game and restore callbacks (handed over through _new_command_handler) and calls them directly. The engine no longer listens to GAME_OVER; it is emitted only by the engine (death, F5 restart) for the UI. Also: test_a_kill_is_announced_once forces the attack to land; attacks have an accuracy roll, so the test failed whenever it missed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
config/dev_config.py imports the optional, gitignored config/settings.py inside a try/except ImportError, but mypy still reported it as a missing module, so `make check` failed on any fresh clone or CI checkout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each of these was reachable from nowhere, or listened for an event nothing emitted. All deleted rather than finished: - ROOM_CHANGED and the first-visit NPC greeting it would have triggered (never emitted; NPCs greet on `talk`). The post-combat NPC line, which does run, is now _npc_speaks_after_combat. - DELAYED_ROOM_REFRESH (cat stopped re-listing the room), UI_ERROR, and GAME_SAVED with the engine's _on_save_requested and the TUI's save_current_game (no callers; `save` is the save path). - The WAITING_FOR_NAME flow (_handle_name_input, pending names): the name is asked in TUTORIAL_NAME_INPUT. Also the LOADING, SAVING, PAUSED and EXIT states, which nothing entered, and PlayerState. - StateManager helpers and the combat context nothing read; the engine's _restart_new_game and end_game; CombatSession's no-op status hook and health-bar builder; CommandHandler.create_health_bar. - UIProtocol methods the engine never calls (update_inventory, update_stats, update_exits, update_player_name, display_game_over, save_current_game) and the unused GameEngineProtocol, plus their implementations in the TUI and HeadlessUI. - The settings file's difficulty key and SettingsManager.set_difficulty / apply_all: difficulty belongs to the run since save v4. test_save_no_recursion is deleted with GAME_SAVED: the save storm it guarded needed that event. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…enforced Review option 5 / decision D2: - The engine routed input on the "game_state" the UI stamped into each COMMAND_ENTERED, so the UI decided what mode the game was in. It now routes on its own StateManager; senders only send the command text. - The transition table disagreed with the game: two transitions every player made (class pick -> name entry, Load Game from the menu) were flagged, and states nothing entered were listed. It now lists exactly the transitions the game makes. - Validation was warn-only. An invalid transition now raises InvalidTransitionError. Because the engine's flow methods catch broad exceptions and fall back to the menu, tests/conftest.py also records every rejection and fails the test even when one was swallowed. That guard found test_classes jumping from the menu straight to class selection; it now walks the real path, as do the difficulty-picker and name tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`take`, `equip` and consumables sent PLAYER_STATS_CHANGED / PLAYER_INVENTORY_CHANGED themselves, and the engine then sent both again after the command, so the UI rebuilt each panel twice. The engine's post-command refresh is now the only sender outside combat (which still sends a frame per turn). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The engine built its world from per-collection loaders that caught every error and returned an empty collection, and the linker's integrity checks only ran in `python -m engine.validate`. Broken content could start a game with a missing or half-connected world. _load_content now calls engine.content.link(load_all(DATA_DIR)): the same checks validate runs, at every start and load. A dangling reference or broken room tree raises DataLoadError at start. DATA_DIR is a class attribute so tests can point it at a scratch copy. Attacks, abilities, tutorial hints and difficulty multipliers are still read as raw YAML by combat.py, data_loader and difficulty.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventBus/EventType (src/events.py) and the view models (src/viewmodels/view_models.py) are the contract between the game and any frontend: what gets announced, and the shape of what the UI draws. They now live in engine/ (engine/events.py, engine/view_models.py), the part the Strangler plan keeps, and import nothing from src/. - engine/api.py and engine/headless no longer import src.events. - Both modules are now covered by engine/'s mypy-strict and ruff checks (dropped from the per-file src/ lists). ruff's fixes are annotation style only (dict/list/X | None) plus line wrapping. - ViewBuilder stays in src/: it reads domain objects, so it is game code, not contract. No re-export shims: every import was updated (36 files). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
New players didn't notice the pinned Echo panel above the input. When the first hint appears (the panel goes from hidden to shown), its border and background pulse in the warning colour three times over about two seconds, then settle. Later steps update the panel without flashing. With reduce motion on, the highlight is held steady for the same time instead of pulsing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works through the architecture review's to-do list: bug fixes first, then moving game rules off the event bus, then clean-up. 15 commits plus a merge of the latest
dev.What changes for players
cdand flee show the room before a fight starts, instead of the room text landing in the combat log.Structure
CommandHandler.arrive), a fight ending (end_combat), a kill (on_kill) and the game-over screen's "n"/"r" are direct calls in a fixed order. Events only tell observers (UI, tutorial). This removes the bug class behind the restore bug: a load path that forgot to subscribe turned encounters off.InvalidTransitionError.load_all()+link(), so a broken reference stops the game at start instead of loading an empty or half-connected world.EventBus/EventTypeand the view models moved intoengine/(engine/events.py,engine/view_models.py). All 36 importing files were updated; there are no shims.Removed (never ran)
ROOM_CHANGED(and the first-visit NPC greeting it would have triggered),DELAYED_ROOM_REFRESH,UI_ERROR,GAME_SAVED.WAITING_FOR_NAMEflow and theLOADING/SAVING/PAUSED/EXITstates.UIProtocolmethods andGameEngineProtocol.difficultykey.The full list is in commit
b53bc19; reverting that one commit restores all of it.Tests and tooling
tests/conftest.pymakes the bus re-raise listener errors, makes ViewBuilder raise instead of returning placeholders, and fails any test in which a state transition was rejected, even if the game swallowed it.Playerfield (walks the object, so the next forgotten field fails), the game-over screens with the real engine and TUI together, arrival/combat/kill order, restore after win, the state machine, and the Echo flash.make checkworks on a fresh clone. mypy no longer needs the gitignoredconfig/settings.py.make check: ruff and mypy clean, 405 passed, content validates.Worth a look by hand
Not done
engine/api.pystill importssrc.game_engine.🤖 Generated with Claude Code