Keep a damaged save slot visible, and stop it being overwritten

[dmccoystephenson]

Summary

Two ways a damaged save could cost the player their run, both of which ended with the damaged slot being overwritten rather than reported.

A player.json that would not parse (#150) made _read_save_metadata return None. That dropped the slot from list_save_files(), and get_next_available_slot() derives the taken slot numbers from that same filtered list — so the slot vanished from the menu and was handed straight back as Create New Save. Picking the only option offered pointed select_save_slot at the occupied directory, and the next save() destroyed the intact stats.json and timeService.json sitting beside the damaged file.

An empty player.json (#170) failed differently: the os.path.getsize(...) > 0 half of each load guard skipped the read entirely, so loadPlayer() never ran, nothing reached failedLoads, and neither the warning dialogue nor the damaged-... copy added by #169 fired. The player was handed a starting character on the saved calendar and the empty file was overwritten on the first action.

What changed

• _read_save_metadata returns an {"unreadable": True, ...} marker instead of None, so a damaged slot stays listed and stays claimed. Those two halves have to move together: visibility alone would not stop the overwrite, and counting it alone would not tell the player it exists.
• player.json is now read strictly — no size guard, since an empty file is damaged rather than absent. Valid JSON that is not an object is also treated as unreadable, because player_data.get(...) would raise AttributeError, which is not among the caught errors and would take the whole save menu down over one bad slot.
• timeService.json is read tolerantly. A damaged calendar is not a damaged run: the player still loads, and FishE already reports and preserves that damage on load, so the slot stays loadable and the label just falls back to its Day 1 default.
• The save menu lists the slot as Slot N - damaged, cannot be loaded and marks it unpickable with a reason, reusing the unavailableOptions mechanism from Grey out menu options the game would refuse, with the reason #168. Deleting is how the slot is reclaimed, so the reason says so and the delete menu tags that row (damaged).
• The three load guards test existence only, so an empty file reaches the loader, raises JSONDecodeError, and is reported and copied aside by the machinery Stop a damaged save from being destroyed, and say so on screen #169 already added — no new reporting path.

The two mechanisms stay complementary rather than redundant: the menu catches what it can see cheaply (unparseable player.json), while the load path still covers damage the metadata read cannot detect — a save that parses but fails schema validation, or a bad stats.json/timeService.json.

Verification

Reproduced both issues' own repro scenarios against the fixed code. Issue #150's slot (truncated player.json beside intact stats.json/timeService.json):

list_save_files() -> [{'slot': 1, ..., 'metadata': {'unreadable': True, 'reason': 'Unterminated string ...'}}]
get_next_available_slot() -> 2
slot_1 still on disk -> ['player.json', 'stats.json', 'timeService.json']
menu options -> ['Slot 1 - damaged, cannot be loaded', 'Create New Save (Slot 2)', 'Delete a Save File', 'Quit']
unavailable  -> {1: 'this save is damaged - delete it to reuse the slot'}
slot selected -> 2

Issue #170's slot (empty player.json):

failedLoads -> ['player.json (Expecting value: line 1 column 1 (char 0))']
showDialogue called -> 1
damaged backups -> ['damaged-20260806-071420']
backup contents -> ['player.json', 'stats.json', 'timeService.json']

Test plan

• python3 -m compileall -q src tests
• SDL_VIDEODRIVER=dummy SDL_AUDIODRIVER=dummy python3 -m pytest --verbose -vv --cov=src --cov-report=term-missing --cov-report=xml:cov.xml — 791 passed, 97% total coverage; every new branch covered
• Two existing tests that asserted the old behaviour (test_read_save_metadata_corrupted_json, test_read_save_metadata_empty_player_file) were updated to the marker — that behaviour was the bug
• Front-end parity: the change is at the shared showOptions(descriptor, options, unavailable) call site, and all three implementations (UserInterface, PygameUserInterface, WebUserInterface — with ConsoleUserInterface/PyodideUserInterface inheriting) already honour unavailableOptions from Grey out menu options the game would refuse, with the reason #168; web/client.js already greys the row and shows the reason. No front-end-specific code was needed or added.
• Schemas: no Player/Stats/TimeService field was added, renamed or retyped, so no schemas/*.json change applies; cross-checked all three schemas against their *JsonReaderWriter anyway and they are in sync.
• README.md's save-file section updated to match (it described the pre-A save whose player.json is corrupt disappears from the menu and its slot is offered as "Create New Save" #150 menu); PLANNING.md needed no change.
• black/autoflake run per format.sh. Note: this sandbox's black is 23.1.0 and reformats four files this PR does not touch (src/business/export.py, src/location/shop.py, src/npc/villagers.py, tests/ui/test_consoleUserInterface.py) — that churn was reverted and left out. The five files here are black --check clean.

Closes #150
Closes #170

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Self-review: read the full diff against the two issues and the surrounding save/load machinery. No correctness problems found; the notes below record the three judgement calls a later reader is most likely to want to undo, plus one pre-existing observation worth a follow-up issue rather than a change here.

One finding from this pass was already fixed in d8b0b12 rather than left as a comment: the damaged row put the blocker in both the label and the reason, so a front-end that appends the reason rendered Slot 1 - damaged, cannot be loaded (unavailable: this save is damaged - delete it to reuse the slot). The label now only identifies the slot, matching how every other unusable option in the game is built.

Deliberately not changed: the _read_save_metadata marker is not derived from schema validation. Keeping the metadata read cheap and limited to "does this parse" leaves a schema-invalid save loadable, which is correct — #169 already reports and preserves that case on load, and a save whose values are out of range is far more recoverable than one that is not JSON. Coupling SaveFileManager to the reader/writers and schemas/ to catch it in the menu would be a worse trade.

[dmccoystephenson]

Deliberate asymmetry, flagged so it doesn't read as a copy-paste slip against the strict player.json read 20 lines above: this one swallows the error and keeps the slot loadable.

timeService.json is not the run. A slot whose calendar is damaged still holds a readable player, and FishE.loadTimeService already records the failure in failedLoads, reports it through the front-end and copies the slot aside. Marking the slot unreadable here would take a mostly-intact run and lock it out of the menu entirely — strictly worse for the player than losing the day counter.

The cost is that day/time are simply absent, which _selectSaveFile renders with its own Day 1 default, so the label can understate a damaged-calendar slot. That seemed better than refusing to load it.

[dmccoystephenson]

This guard is not defensive padding — it closes a real crash that the original except clause could not catch.

json.load on a player.json holding valid JSON that isn't an object (42, null, "x", or some unrelated file copied over the save) returns a non-dict, and the player_data.get(...) calls below raise AttributeError. That is not in (json.JSONDecodeError, IOError, OSError), so it would propagate out of _read_save_metadata and out of list_save_files — taking the entire save menu down on startup over one bad slot, which is a strictly worse failure than the disappearing slot #150 is about.

Covered by test_read_save_metadata_player_file_that_is_not_an_object, which asserts the listing still works rather than only checking the return value.

[dmccoystephenson]

This branch should be unreachable and is here on purpose; worth justifying rather than leaving as apparently-dead code.

showOptions is contracted not to return an unavailable option's number, and unavailableReasons' all-unavailable fallback cannot fire here because Quit is always selectable. But without a branch, kind == "damaged" falls out of the if/elif chain and the while True simply re-renders the same menu — the player picks the row, and the game appears to ignore them, with nothing said. That is an unexplained hang rather than a visible bug.

I hit exactly this while writing the reproduction script for #150 (a mock returning "1" unconditionally span the menu forever), which is what convinced me not to rely on the contract. A new front-end is the realistic way this gets violated for real, and front-end parity is the most common gap in this repo. Covered by test_selectSaveFile_explains_a_damaged_slot_a_front_end_let_through.

[dmccoystephenson]

Observation, pre-existing and not changed by this PR (the line moved but the behaviour is untouched): metadata["energy"] has no production consumer. _selectSaveFile reads only day, money and fishCount, and the only other reference in the repo is an assertion in test_read_save_metadata_partial_data.

Left alone deliberately — removing it is unrelated to #150/#170 and would widen this diff. Worth a small follow-up issue to either drop it or put it on the slot label, where it would arguably be more useful than the fish count.