Add world management and save hardening

[MichaelFisher1997]

Summary

• add rename, duplicate, delete, and timestamped backup actions to the saved-world library
• add configurable autosave intervals and versioned gameplay-state migration
• harden world IDs, player-state restoration, weather continuity, and corrupt-region handling
• expand storage, UI-flow, and weather verification and update the roadmap

Verification

• redot --editor --headless --path . --quit
• redot --headless --path . --script res://tools/world_storage_verify.gd
• redot --headless --path . --script res://tools/ui_flow_verify.gd
• redot --headless --path . --script res://tools/weather_verify.gd
• redot --headless --path . --script res://tools/frame_pacing_verify.gd
• redot --headless --path . res://tools/player_target_verify.tscn
• live main-menu startup check

[github-actions]

PR Review: #22 — Add world management and save hardening

📋 Summary

No linked issues ("Closes #N" etc.) are referenced in the PR description, so there are no issue requirements to verify beyond the ROADMAP items — the PR checks off "World management UI" and updates two adjacent persistence items, and the implementation does match those claims (rename/duplicate/backup in ui/play_panel.gd, autosave option in Gameplay settings, session-state versioning in game/main.gd, corrupt-region save blocking in world/world_storage.gd).

This PR adds world-library management (rename, duplicate, timestamped backup), a configurable autosave interval with retry-on-failure, versioned gameplay-state migration, and several save-hardening measures (id traversal rejection, creation-over-existing guard, player Y-bounds validation, weather transition persistence). The code is careful, consistently styled, well-commented, and expands three headless verifiers (world_storage_verify, ui_flow_verify, weather_verify) to cover the new behavior; no worker-thread, mesher, water-sim, or worldgen code is touched, so the riskiest invariants of the codebase are unaffected.

📌 Review Metadata

• Reviewed Commit SHA: 3ec932e040e24516cc9aede99990d22a78ebfdaa
• Reviewed PR: Add world management and save hardening #22
• No previous automated reviews exist; nothing to acknowledge as fixed.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

None identified. Specific non-issues verified: duplicate_world passes activate=false so it never hijacks the Continue-Last-World pointer (world/world_storage.gd:225); .tmp files are excluded from copies (world_storage.gd:341); _copy_directory_contents tolerates ERR_ALREADY_EXISTS consistently with _ensure_world_directories (world_storage.gd:335-337); the missing-inventory-key behavior change (game/main.gd:366-367) correctly restores defaults instead of wiping to empty; the LoadBox child reordering in _build_management_controls yields list → manage → rename → confirm → status → footer with mutually exclusive visibility; ui_cancel routing, focus-on-open, and focus-restore for the rename modal all follow the repo's modal contract; all existing create_world call sites remain compatible with the new optional activate parameter.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] world/world_storage.gd:131-133 (with world/world_storage.gd:368-371) - One corrupt region blocks every save for the entire session
Confidence: High
Description: _load_region sets the sticky _region_load_error the first time any region file exists but fails to parse, and flush_dirty_regions() returns it before writing anything. The error is only cleared by _reset_cache() (i.e., open_world/create_world), so within a session it is permanent: no other region is flushed, and flush() never reaches the metadata write, so player state/inventory/time are never saved either. Merely walking near the corrupt region is enough — chunk edit hydration loads the region — the player does not need to edit there.
Impact: A single unreadable region file (disk corruption, truncated file, region written by a newer build) means all progress made after discovery is lost on exit, and the autosave retry loop re-attempts a save that can never succeed. This is a documented, deliberate tradeoff (ROADMAP: "an unreadable region blocks later saves instead of being overwritten as empty terrain") and the previous behavior had its own data-loss mode, but the blast radius here is the whole session rather than the corrupt region.
Suggested Fix: Block only the corrupt region's write and still flush clean regions plus session metadata, or quarantine the unreadable file (rename to .corrupt) after surfacing an explicit, non-repeating warning:

func flush_dirty_regions() -> Error:
	var first_error: Error = _region_load_error
	for region_pos in _dirty_regions.keys():
		if _region_load_failed_regions.has(region_pos):
			continue # never rewrite a region we could not read
		var error := _write_region(region_pos)
		...

[MEDIUM] player/player.gd:99-100 - Restoring rejects positions at or above the world ceiling, silently discarding saved positions
Confidence: Medium
Description: The new guard if restored.y < 0.0 or restored.y >= float(VoxelDefs.WORLD_HEIGHT): return false rejects any save with y ≥ 192. Nothing clamps flight or standing height to the world ceiling (no WORLD_HEIGHT clamp exists in player.gd or the movement sweep), so a player who autosaves while flying above 192 — or standing on a build at the ceiling (feet exactly at y = 192.0) — writes a state that fails validation on next load. Main then takes the fresh-spawn path, losing position, yaw, and spawn point (inventory/slot/time/weather still restore via _restore_session_state).
Impact: Silent respawn-at-world-spawn on reload for players who saved while high in the air; a "hardening" check introduces a new legitimate-state rejection. Impact is limited (position only, no corruption), but it is reachable through normal play.
Suggested Fix: Clamp instead of reject for in-range-adjacent values, reserving rejection for genuinely corrupt data, e.g.:

	if not is_finite(restored.y):
		return false
	restored.y = clampf(restored.y, 0.0, float(VoxelDefs.WORLD_HEIGHT) - 0.2)

Alternatively, clamp fly height in Player._physics_process so saves can never exceed the bound.

ℹ️ Low Priority Suggestions (Optional)

[LOW] world/world_storage.gd:229 - Duplicate name can exceed the 64-character rename cap
Confidence: High
Description: duplicate_world appends " Copy" to the source name without applying the 64-character limit that rename_world enforces. A 64-char source yields a 69-char stored name that (a) can never be re-saved unchanged via rename (rejected as too long) and (b) is silently truncated by the rename LineEdit's max_length = 64 when prefilled (ui/play_panel.gd:552,721), so confirming the dialog renames the world to the truncated string.
Impact: Minor inconsistency in the management UI for max-length names.
Suggested Fix: Trim before appending: var base := String(source_metadata.get("name", "World")); base = base.substr(0, maxi(64 - 5, 1)) then append " Copy".

[LOW] game/main.gd:234-238 - Permanent save failure retries every 5 seconds with a repeated warning and toast
Confidence: High
Description: On failure _autosave_time = maxf(autosave_interval - AUTOSAVE_RETRY_SECONDS, 0.0) schedules a retry in ~5 s. For a transient failure this is good, but for the sticky _region_load_error (or unavailable storage) every retry emits push_warning plus set_status("Save failed...") for the rest of the session — a toast every 5 seconds for hours.
Impact: Log and HUD toast spam; the message stops being informative after the first few occurrences.
Suggested Fix: Distinguish permanent from transient failures (e.g., back off exponentially, or only toast once per session for ERR_FILE_CORRUPT and keep subsequent retries silent).

[LOW] ui/play_panel.gd:604-605 (and :592) - Failure status messages use the success color
Confidence: High
Description: _operation_status is styled with UITheme.CYAN (ui/play_panel.gd:693), so "Could not duplicate this world." / "Could not back up this world." read identically to "Created X." / "Backup saved to X.". The delete confirmation uses UITheme.WARN for destructive/error text.
Impact: Cosmetic; error feedback is not visually distinguishable.
Suggested Fix: Style the label per message (or add a second label) using UITheme.WARN for failures, matching _confirm_label's treatment.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Management operations (rename/duplicate/backup) live in WorldStorage, UI flow in PlayPanel, state migration in Main; each layer keeps its existing duty
Open/Closed	7	Autosave row added purely through the data-driven row definition; storage gained new static operations without modifying existing callers
Liskov Substitution	8	No inheritance changes; WorldStorage static/instance contract stays coherent for all call sites
Interface Segregation	8	Player↔Main still signals-only; PlayPanel preserved its existing signal names/args (create_requested, load_requested)
Dependency Inversion	7	Main/UI still depend on the WorldStorage/GameConfig concretes directly (pre-existing pattern); constants are properly centralized in GameConfig
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 78%

Confidence Breakdown:

• Code Quality: 88% (idiomatic, typed, well-commented, follows the repo's UI/storage conventions)
• Completeness: 90% (all PR claims implemented; ROADMAP updated accurately)
• Risk Level: 72% (no worker/determinism/mesher risk; the two mediums are a documented blast-radius tradeoff and a reachable edge-case position rejection)
• Verification: 70% (verifiers listed as run in the PR and the new assertions match the code as read, but the engine is not available on this runner — redot --editor --headless --path . --quit, tools/world_storage_verify.gd, tools/ui_flow_verify.gd, tools/weather_verify.gd, and tools/player_target_verify.tscn should be green before merge; residual risk is around the in-game corrupt-region/autosave path, which only a live run exercises)

Merge Readiness:

• All critical issues resolved (none found)
• SOLID average score >= 6.0 (7.6)
• Overall confidence >= 60% (78%)
• No security concerns (id traversal, overwrite, and path handling are hardened and verifier-covered)
• No unresolved worker-thread, determinism, or data-loss risk (two medium data-loss-adjacent tradeoffs remain: session-wide save blocking on one corrupt region, and ceiling-height position rejection)

Verdict:

MERGE WITH FIXES — Solid, verifier-covered feature work; address the corrupt-region save-blocking blast radius and the y-bound position rejection (or explicitly accept both as documented tradeoffs) before merge.

Machine Readable Verdict

{
  "reviewed_sha": "3ec932e040e24516cc9aede99990d22a78ebfdaa",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 2,
  "overall_confidence_score": 78,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[MichaelFisher1997]

Addressed the automated review in 12f9302:

• isolate unreadable regions so their files remain untouched while unrelated regions and session state continue saving
• preserve legitimate above-ceiling flight and spawn positions, with verifier coverage
• cap duplicated names at the 64-character management limit
• back off failed autosaves to at most one retry every 30 seconds
• style management-operation failures with the warning color

Re-ran the editor parse check plus world storage, UI flow, and player target verifiers locally.

[github-actions]

PR Review: #22 — Add world management and save hardening (re-review after 12f9302)

📋 Summary

No linked issues ("Closes #N" etc.) appear in the PR description; the requirements to verify are the ROADMAP claims, which remain accurately described — the follow-up commit updated the corrupt-region wording to "an unreadable region rejects edits to that region without blocking unrelated regions or session state" (ROADMAP.md:75), which now matches the implementation.

The follow-up commit 12f9302 ("Address persistence review feedback") is a focused response to all five findings of the previous review: unreadable regions are isolated per-region instead of blocking the whole session save, above-ceiling player positions are preserved, duplicated names are capped at 64 characters, autosave retries back off to 30 seconds, and management-operation failures use the warning color. Each fix carries new verifier assertions where feasible, and the commit touches exactly the files needed (no worker-thread, mesher, water-sim, or worldgen code is affected by either commit).

📌 Review Metadata

• Reviewed Commit SHA: 12f93023a3ec05770dc80bfd8b8486f24275539f
• Reviewed PR: Add world management and save hardening #22
• Previous automated review (of 3ec932e) reported 0 critical, 0 high, 2 medium, 3 low issues; this re-review verifies each against the current head.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported).

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high-priority issues have been resolved (none were reported).

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: one corrupt region blocked every save for the entire session (_region_load_error short-circuited flush_dirty_regions()). The current code replaces the sticky error with per-region isolation: _load_region records the region in _unreadable_regions only when its file exists but fails to parse (world/world_storage.gd:367-372), stage_chunk_edits refuses to stage into it so it never becomes dirty (world/world_storage.gd:109-110), and flush_dirty_regions() (world/world_storage.gd:133-144) plus flush() (world/world_storage.gd:120-130) write all other regions and the session metadata. Verified the dirty/unreadable states cannot overlap: a region marked unreadable was never staged, and a dirty region is never evicted or re-read. tools/world_storage_verify.gd:148-154 asserts an unrelated region still flushes OK and the unreadable file's bytes are untouched — this is exactly the suggested fix, and the tradeoff is now documented in ROADMAP.

✅ [FIXED] Previous issue: restoring rejected positions at or above the world ceiling. restore_persistent_state now rejects only restored.y < 0.0 (player/player.gd:99-100), and the spawn restore requires restored_spawn.y >= 0.0 with fallback to the restored position (player/player.gd:110-114), so legitimate above-ceiling flight and spawn states survive reload. tools/player_target_verify.gd:94-101 asserts y=210 position/spawn restore succeeds.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: duplicated names could exceed the 64-character rename cap. duplicate_world now uses "%s Copy" % source_name.substr(0, 59) (world/world_storage.gd:230), capping at 64; tools/world_storage_verify.gd:238-240 asserts a 64-char source yields exactly the capped duplicate name.

✅ [FIXED] Previous issue: permanent autosave failures retried every ~5 seconds with repeated warning/toast. AUTOSAVE_RETRY_SECONDS := 30.0 (game/main.gd:25) and the failure path schedules at most one retry per 30 seconds (game/main.gd:238), matching the suggested backoff.

✅ [FIXED] Previous issue: management failure messages used the success color. _set_operation_status(text, failed := false) styles with UITheme.WARN if failed else UITheme.CYAN (ui/play_panel.gd:757-760), and all failure call sites pass true (ui/play_panel.gd:576,592,605).

[LOW] world/world_storage.gd:109-110 (with world/voxel_world.gd:296-299) - Refused region edits are only surfaced via console warning
Confidence: High
Description: When stage_chunk_edits refuses an unreadable region, VoxelWorld.flush_edit_store() still returns OK and Main._flush_world_save() reports a successful save — the only signal is the push_warning at world/world_storage.gd:372, which never reaches the HUD, and which re-fires each time the unreadable region is evicted from the 16-entry clean cache and re-loaded. A player building inside a corrupt region will silently lose those edits on reload while autosave reports success.
Impact: Limited, in-game-visibility-only data-loss surprise for the corrupt region's edits; the region file itself is preserved and the tradeoff is documented in ROADMAP, so this is the accepted design.
Suggested Fix: Track refused staging (e.g., expose has_unreadable_regions() / a count of dropped regions) and surface it once per session through Main.set_status("Region X could not be saved - its file is corrupt"), ideally deduplicating the repeated push_warning.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Management operations stay in WorldStorage, UI flow in PlayPanel, migration/autosave policy in Main; the fix kept boundaries intact
Open/Closed	7	Autosave row added via the data-driven row table; storage gained static management ops and an activate flag without breaking existing create_world callers
Liskov Substitution	8	No inheritance changes; WorldStorage contract stays coherent, and stage_chunk_edits' new refusal semantics are internal (still void)
Interface Segregation	8	Player↔Main still signals-only; PlayPanel preserved create_requested/load_requested; rename modal follows the repo modal contract (ui_cancel, focus-on-open, focus-restore)
Dependency Inversion	7	Main/UI depend on WorldStorage/GameConfig concretes (pre-existing pattern); constants properly centralized in GameConfig/WorldStorage
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

• Code Quality: 90% (idiomatic typed GDScript, careful validation paths, defense-in-depth id checks at every storage entry point)
• Completeness: 95% (all five review findings fixed with matching verifier assertions; PR and ROADMAP claims match the code)
• Risk Level: 85% (no worker/mesher/water/worldgen changes; remaining risk is the documented corrupt-region tradeoff, now bounded to a single region)
• Verification: 82% (the fix commit's files are covered by the author's re-run set — parse check, world_storage_verify, ui_flow_verify, player_target_verify; weather_verify ran against commit 1 whose weather code commit 2 does not touch. The engine binary is unavailable on this runner, so these were not independently executed here — residual risk around live corrupt-region/autosave behavior remains a live-run concern, not a blocker)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0 (7.6)
• Overall confidence >= 60% (88%)
• No security concerns (traversal ids rejected at create_world/open_world/_read_world_for_management; creation-over-existing guarded; .tmp excluded from copies; verifier-covered)
• No unresolved worker-thread, determinism, or data-loss risk

Verdict:

MERGE — All five previously reported issues are fixed with verifier coverage, and the only remaining finding is a low-priority in-game visibility gap for an already-documented tradeoff.

Machine Readable Verdict

{
  "reviewed_sha": "12f93023a3ec05770dc80bfd8b8486f24275539f",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run

[MichaelFisher1997]

Closed the remaining optional visibility finding in 13fa59c: unreadable regions are exposed by WorldStorage, repeated console warnings are deduplicated, and Main shows one in-game warning per session after a successful save so affected edits are not silently presented as durable. Parse and storage verification pass locally.

[github-actions]

I have everything I need. The new commit is verified against the full files, the previously fixed items are all confirmed at HEAD, and I've checked the surrounding invariants (eviction does not clear _unreadable_regions, so the count is session-stable and the warning dedup is correct; unreadable_region_count() is only read on the main thread).

PR Review: #22 — Add world management and save hardening (re-review after 13fa59c)

📋 Summary

No linked issues ("Closes #N" etc.) appear in the PR description; the requirements to verify remain the ROADMAP claims, which still match the implementation.

The follow-up commit 13fa59c ("Surface unreadable region warnings") closes the single remaining LOW from the previous review: WorldStorage exposes unreadable_region_count(), the repeated per-reload push_warning is deduplicated via a first-warning flag that survives clean-cache eviction (eviction erases _regions/_loaded_regions/_region_access but never _unreadable_regions, world/world_storage.gd:673-689), and Main._flush_world_save shows one in-game toast per session after a successful save (game/main.gd:415-417), with a new verifier assertion (tools/world_storage_verify.gd:149-150). The change is 13 lines across 3 files, touches no worker-thread, mesher, water-sim, or worldgen code, and reads _unreadable_regions only from the main thread.

📌 Review Metadata

• Reviewed Commit SHA: 13fa59ce67c6f58d324f730e06c240d180ba16e1
• Reviewed PR: Add world management and save hardening #22
• Previous automated reviews (of 3ec932e and 12f9302) reported in total 2 medium and 4 low issues; this re-review verifies each against the current head.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported).

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high-priority issues have been resolved (none were reported).

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ All previously reported medium-priority issues have been resolved:

• ✅ [FIXED] (reported at 3ec932e) One corrupt region blocked every save for the entire session — per-region isolation confirmed at HEAD (world/world_storage.gd:109-110, 133-144, 367-378), verifier-covered at tools/world_storage_verify.gd:151-156.
• ✅ [FIXED] (reported at 3ec932e) Restoring rejected positions at/above the world ceiling — confirmed at HEAD (player/player.gd:99, 111-113).

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] (reported at 12f9302) Refused region edits were only surfaced via a repeating console warning. The current code exposes unreadable_region_count() (world/world_storage.gd:151-152), deduplicates the push_warning per region across eviction-driven reloads (world/world_storage.gd:375-378 — the flag lives in _unreadable_regions, which only _reset_cache() clears, so re-loads after eviction stay silent), and Main shows one status toast per session after a successful save (game/main.gd:415-417; _storage_warning_shown is per-Main-instance, and Main is recreated per scene load, so per-session semantics hold). tools/world_storage_verify.gd:149-150 asserts the count is exposed.

[LOW] game/main.gd:415-417 - With Autosave Off, the warning is effectively never visible
Confidence: High
Description: The toast fires only inside the successful-save branch of _flush_world_save(). This PR also adds an "Off" autosave option (autoload/game_config.gd: AUTOSAVE_INTERVAL_VALUES[0] == 0), and with autosave off the only flushes happen on exit paths — _on_new_world/_on_quit_game/_exit_tree (game/main.gd:199-200, 822-832) — where the HUD toast either never renders (scene change/quit immediately after) or is about to be torn down. A player with autosave disabled who builds in a damaged region gets no visible signal.
Impact: Minor: the default 30 s autosave surfaces the toast mid-session as intended, and the corrupt region's file is still preserved plus logged via push_warning; only the Off setting loses the visible feedback.
Suggested Fix: Also surface the warning once when an unreadable region is first encountered during play, e.g. after VoxelWorld hydration reports the region (or on the first _flush_world_save attempt regardless of autosave), rather than gating solely on a completed save:

	if save_error == OK:
		GameConfig.active_world_metadata = _world_storage.metadata.duplicate(true)
		if not _storage_warning_shown and _world_storage.unreadable_region_count() > 0:
			_storage_warning_shown = true
			if autosave_interval > 0.0 or not get_tree().paused:
				set_status("A damaged world region is read-only; edits there cannot be saved")

(or simply call set_status from the point where stage_chunk_edits first refuses an edit).

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	WorldStorage owns the unreadable-region ledger and a read-only accessor; Main owns when/how to surface it to the player
Open/Closed	7	New query method added without modifying existing callers; autosave row added purely through the data-driven row table
Liskov Substitution	8	No inheritance changes; WorldStorage contract stays coherent across instance and static use
Interface Segregation	8	Player↔Main still signals-only; PlayPanel preserved create_requested/load_requested; rename modal follows the modal contract (ui_cancel routing at ui/play_panel.gd:128-133, focus-on-open/focus-restore at :560,570)
Dependency Inversion	7	Main/UI depend on the WorldStorage/GameConfig concretes (pre-existing pattern); constants centralized
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 90%

Confidence Breakdown:

• Code Quality: 90% (typed, idiomatic, minimal-surface fix; the dedup flag placement correctly exploits the eviction lifecycle)
• Completeness: 96% (all six findings across two review rounds addressed with verifier coverage where feasible)
• Risk Level: 87% (no worker/mesher/water/worldgen changes; the corrupt-region tradeoff is bounded, documented in ROADMAP, and now player-visible in the default configuration)
• Verification: 83% (author re-ran the parse check and world_storage_verify for this commit, and the new assertion matches the code as read; main.gd changes are a toast plus a bool flag with low blast radius. The engine is unavailable on this runner, so these were not independently executed — residual risk is live-run behavior of the corrupt-region path only)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0 (7.6)
• Overall confidence >= 60% (90%)
• No security concerns (traversal ids rejected at every storage entry point; creation-over-existing guarded; .tmp excluded from copies; verifier-covered)
• No unresolved worker-thread, determinism, or data-loss risk

Verdict:

MERGE — Every finding from both prior reviews is fixed with verifier coverage; the only new observation is a minor visibility gap for the non-default Autosave Off setting.

Machine Readable Verdict

{
  "reviewed_sha": "13fa59ce67c6f58d324f730e06c240d180ba16e1",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 90,
  "recommendation": "MERGE"
}

opencode session  |  github run