fix(persist): only clear dirty after a successful save so a forced Save retries

[itsmiso-ai]

Fixes issue #379 by ensuring forced saves always attempt to write and that failed writes don't clear the dirty flag prematurely.

Fixes #379

Opened by foreman on review GO (workload wl-misospace-windowstead-379).

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — primary route

Recommendation: Approve

This PR correctly fixes issue PR 379. The three-file change is minimal, focused, and directly addresses both the root cause (dirty flag cleared before write completes) and the symptom (second Save reports success without writing).

Change-by-Change Findings

scripts/game_state.gd (+8/-0)

Added _save_game_hook: Callable pointing to _save_game_impl by default. save_game() now delegates to this hook. This enables test injection without touching the filesystem — the cleanest approach for counting invocations.

scripts/main.gd (+10/-2)

Two surgical moves:

1. Guard flip: if not sim.dirty: → if not force and not sim.dirty:. A forced save (explicit Save, recruit, placement, quit) now always reaches GameState.save_game(state), even if a previous failed write left sim.dirty cleared.
2. Reset reorder: sim.dirty = false moved from before the write to after it succeeds. The flag only clears when GameState.save_game(state) returns true.

tests/test_save_failure_surfaced.gd (+78/-1)

Flow 6 exercises the exact failure window from the issue:

• Installs a hook that always returns false and increments a call counter.
• Calls persist(true) twice in sequence and asserts both return false (not the second returning true and lying about success).
• Asserts _save_call_count == 2 — the write was re-attempted on the second click.
• Asserts main.sim.dirty stays true through both failures.
• Asserts no "Game saved" event was pushed.
• Verifies sim.dirty clears only after the hook is restored to return true.

Standards Compliance

Repository standards file (AGENTS.md) is present. Notable applicable conventions:

• Testing traps: Tests use assert_eq(actual, expected, name) with three required arguments — Flow 6 follows this correctly throughout.
• Testing traps: The test avoids the 2-argument assert_eq parse-error trap.
• Save/version migration: Not applicable; this is a behavior fix, not a save format change.

No standards conflicts detected.

Linked Issue Fit

Issue PR 379 acceptance criteria:

1. ✅ persist(force=true) always invokes GameState.save_game(state) — verified by guard flip if not force and not sim.dirty.
2. ✅ sim.dirty = false only on successful write — verified by move after the if not GameState.save_game(state) branch.
3. ✅ Regression test in tests/test_save_failure_surfaced.gd that forces a failed write, calls save_game() again immediately, and asserts GameState.save_game(state) was invoked and "Game saved" was not pushed a second time — Flow 6 covers all three assertions.

Issue PR 379 expected files: scripts/main.gd, tests/test_save_failure_surfaced.gd, optionally scripts/game_state.gd — all three are touched.

Tool Harness Findings

No tool harness output in this corpus.

CI Check Results

All checks passed: Script test suite, macOS validation, and all platform exports (Windows, Web, Linux, macOS) plus headless smoke test. CI is green.

Unknowns / Needs Verification

None — all required evidence is present and verifiable from the corpus.