fix(web): report localStorage save success via sentinel completion value (#402)

[itsmiso-ai]

What

Fixes #402.

On the web build, GameState._local_storage_write reported every save as failed: localStorage.setItem(...) returns undefined on success and JavaScriptBridge.eval converts undefined — like a thrown quota/security error — to GDScript null, so the old if result == null: return false could not distinguish success from failure. main.gd persist() therefore kept sim.dirty set, announced "Save failed: colony progress is not being persisted.", and rewrote localStorage every 10 ticks; the Save button always reported failure.

The write eval statement now ends with a fixed sentinel string literal (WRITE_EVAL_SUCCESS_SENTINEL), giving it a deterministic completion value; success is detected by exact match in the new local_storage_write_succeeded decoder. A throw still yields null → false.

Invariants this change keeps, and every other path enforcing the same rules

1. No JS injection into localStorage eval statements (issue [P1] XSS in web build: JavaScriptBridge.eval uses unsanitized key for localStorage ops #291/[P3] No Web export preset or CI coverage for the localStorage/JavaScriptBridge persistence path #316): key/payload must stay JSON-encoded inside JS string literals and cannot extend the eval'd program.
   • build_local_storage_write_eval (scripts/game_state.gd): unchanged JSON encoding of key/payload; the appended ; "windowstead-write-ok" is a fixed literal, not attacker-controllable (evil content is confined inside the JSON string args). Kept by the updated tests/test_local_storage_xss.gd round-trip + single-call-shape tests, which strip only the exact fixed suffix before re-checking the localStorage.setItem(...) call.
   • build_local_storage_read_eval / build_local_storage_remove_eval (scripts/game_state.gd): untouched; their XSS tests unchanged.
2. _local_storage_write returns true only when the write actually happened; false on genuine failure (issue [P3] persist() clears dirty before write succeeds; second Save click after a failure reports success without saving #379 retry semantics).
   • Consumers: _save_game_impl (web branch, SAVE_KEY) → main.gd persist() (~1845) now clears sim.dirty / _persist_failure_announced on web success; main.gd save_game() (~743) Save button reports success only on real writes; both unchanged in code and keep behaving correctly because the bool is now truthful.
   • Other writers of the same localStorage state: the settings writer save_settings (end of scripts/game_state.gd) calls _local_storage_write and discards its result (pre-existing) — filed as follow-up [P3] save_settings discards the web localStorage write result — settings save failures are invisible #409, out of scope here. clear_game's removeItem path ignores eval results; removeItem is idempotent and has no false-success consumer, so no sentinel needed.
   • Desktop writer _write_text_file (FileAccess) and the desktop save/load paths are untouched.
3. No save-format / migration change: setItem receives the exact same JSON.stringify(data) payload byte-for-byte; only the eval wrapper changed. Load, validate_save_schema, and migrate_save paths are untouched (save/version migration stays migration-first — nothing to migrate here).
4. CI SCRIPT ERROR rail: no String(...) conversion is performed on untrusted Variant types — local_storage_write_succeeded uses a typeof(result) == TYPE_STRING guard (String(false)/String(0) throws a runtime script error in Godot 4.7.1, which would fail the suite per the CI log check).

Verification

• Full CI-equivalent local run (godot --headless --path . --script res://tests/<suite>.gd for all 30 suites, Godot 4.7.1-stable, same SHA256 as godot-toolchain.json): all pass, no SCRIPT ERROR / Parse Error lines.
• tests/test_local_storage_xss.gd extended: success-detection asserts (sentinel → true; null, "", "null", arbitrary string, false, 0 → false) without a browser, per the issue's acceptance criteria.
• JS completion-value semantics verified against globalThis.eval behavior (trailing string literal is the completion value; throws propagate).

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3 (anthropic) — primary route

Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.

Findings (2 info)

Severity	Location	Finding
Info	tests/test_local_storage_xss.gd:41	The new flow_write_success_detection_matches_sentinel registers before each flow_export call but after the round-trip and single-call-shape flows; this matches the existing ordering pattern and is correct.
Info	scripts/game_state.gd:688	save_settings (line 688) calls _local_storage_write and discards the bool — pre-existing behavior filed as follow-up PR 409, out of scope for this PR; the PR does not regress this path.

Recommendation

Approve. The fix is targeted, internally consistent, and matches the linked issue's acceptance criteria. The PR swaps an un-decodable eval result (undefined and thrown errors both arriving as GDScript null) for a deterministic sentinel completion value, gates detection behind a typeof == TYPE_STRING guard, and extends the existing browser-free XSS suite to cover the new wiring.

Change-by-change findings

• scripts/game_state.gd (build_local_storage_write_eval, lines 51–61): a new WRITE_EVAL_SUCCESS_SENTINEL := "windowstead-write-ok" constant is appended via JSON.stringify(WRITE_EVAL_SUCCESS_SENTINEL). Because the sentinel is interpolated through JSON.stringify rather than concatenated raw, the produced statement is localStorage.setItem("<key>", "<payload>"); "windowstead-write-ok" — a JSON-encoded string literal appended after a semicolon. The first ( and the last char of the call part still belong to localStorage.setItem(…); attacker-controlled bytes are confined inside the JSON string args as before. Held length is held.
• scripts/game_state.gd (_local_storage_write, lines 73–75): the old if result == null / String(result) cascade is replaced by return local_storage_write_succeeded(result). The old code would have hit String(false)/String(0) if localStorage.setItem ever returned a non-string non-null value through some bridge version; the new code gates on typeof(result) == TYPE_STRING before the comparison, so no untrusted String(...) coercion runs in the common path. Held length is held for the original invariant (true only on real success, false on throws).
• scripts/game_state.gd (local_storage_write_succeeded, lines 81–82): the new static helper. typeof(result) == TYPE_STRING and result == WRITE_EVAL_SUCCESS_SENTINEL correctly distinguishes the sentinel from the previous failure modes (thrown → null → false; empty string → false; literal "null" → false; boolean false → false; 0 → false) and is exercised by flow_write_success_detection_matches_sentinel. Held.
• tests/test_local_storage_xss.gd: WRITE_STATEMENT_SUFFIX := "; \"windowstead-write-ok\"" mirrors the production string (including the surrounding quotes) and is stripped before the existing flow_write_eval_statement_round_trips_evil_key / flow_write_eval_statement_round_trips_evil_payload / flow_eval_statement_has_single_call_shape flows re-check the call shape, so XSS coverage is not weakened — it is hardened by the explicit ends-with assertion added at the top of each round-trip flow. The new flow_write_success_detection_matches_sentinel runs entirely without JavaScriptBridge (it imports the static helpers via GameState.local_storage_write_succeeded(...) and GameState.WRITE_EVAL_SUCCESS_SENTINEL), satisfying the issue's "without a browser" acceptance criterion. Held.

Claim-by-claim verification

• Claim 1 ("these helpers are the only place either quirk lives") — held. Repo grep finds only scripts/game_state.gd:51 (constant), scripts/game_state.gd:60–61 (builder), scripts/game_state.gd:73–75 (writer), scripts/game_state.gd:81–82 (decoder). The only other writer of localStorage state is save_settings at scripts/game_state.gd:685, which already routes through _local_storage_write and is therefore covered by the same fix.
• Claim 2 ("thrown quota/security error arrives as null, so the bare call cannot signal success") — held. The previous if result == null: return false branch (which could not distinguish success from throw) is gone; the new decoder accepts only the exact sentinel string, so a thrown error → null → false.
• Claim 3 ("local_storage_write_succeeded: only the exact sentinel string means success") — held for the seven cases the suite asserts (sentinel → true; null / "" / "null" / "ok" / false / 0 → false). The new test imports the static helper directly, so the assertion is exact-match against the constant, not against a literal string that could drift.
• Claim 4 ("the first ( must be the call-opening paren and the last char of the call part must be the call-closing )") — held for flow_write_eval_statement_round_trips_evil_key, flow_write_eval_statement_round_trips_evil_payload, and the rewritten flow_eval_statement_has_single_call_shape, all of which now strip the fixed suffix before checking call shape. The evil keys tested include a trailing ( (in flow_eval_statement_has_single_call_shape) and complex payloads containing parens/backticks/backslashes, which all still round-trip cleanly.
• Claim 5 ("any attacker-supplied parens are confined to the JSON string args") — held for the keys/payloads tested. The appended suffix is a fixed literal interpolated through JSON.stringify (which escapes any embedded quotes/backslashes/control chars), so no attacker-controlled input reaches the suffix position. The diff additionally removed the old String(result) coercion that was only safe when result was already a string, replacing it with the typeof == TYPE_STRING short-circuit before the comparison.

Sources

• scripts/game_state.gd lines 51, 60–61, 73–75, 81–82, 122, 685, 688 (production change and the other localStorage writer that reuses it).
• scripts/main.gd lines 1832–1861 (persist) and ~743 (save_game) — the only consumers of GameState.save_game/_local_storage_write's bool; both already treat true as success and false as retry-announce, so they behave correctly once the return becomes truthful.
• tests/test_local_storage_xss.gd lines 17–24 (sentinel suffix constant), 41 (flow_write_success_detection_matches_sentinel registered), 87–104 (round-trip flows with the suffix stripped), 126–149 (single-call shape and success-detection flows).
• tests/test_case.gd — confirms the assert_eq(actual, expected, name) 3-arg convention enforced by AGENTS.md is satisfied throughout; assert_true / assert_false accept the 2-arg (condition, name) form the new tests use.
• AGENTS.md — review/implementation notes (migration-first; tests run headless with HOME=/tmp XDG_DATA_HOME=/tmp/.local/share; assert_eq 3-arg trap documented at PR 321).
• CI status for 6e5ef0b (head of this branch): script test suite, release-tag validation, macOS validation, headless smoke, and all four export validations (Web, macOS, Windows, Linux) all succeeded.

Standards Compliance

AGENTS.md's review notes and testing traps are followed — the new test calls assert_eq only with three arguments (no risk of the PR 321 parse-error trap that drops the whole suite), runs against the same headless 4.7.1 toolchain referenced in godot-toolchain.json and .justfile, and uses the preload(...) / load(...) patterns the suite conventions require. The change does not alter save format or migration; setItem still receives the exact same JSON.stringify(data) payload byte-for-byte, so no migration-first work is needed.

Linked Issue Fit

Issue PR 402 asks for: _local_storage_write returns true when setItem succeeds and false on genuine failure; existing XSS guarantees preserved; a regression test that asserts the new logic without a browser; on the web build, a successful save no longer surfaces "Save failed" and clears the dirty flag. The PR delivers all four:

• The new sentinel gives a deterministic completion value distinct from a thrown error; the decoder gates on typeof == TYPE_STRING and exact match.
• XSS guarantees in build_local_storage_write_eval are unchanged (JSON-encoded key/payload) and the test suite now explicitly asserts the call shape after stripping the fixed suffix, with the same evil keys/payloads as before plus the trailing-( key in the single-call-shape flow.
• flow_write_success_detection_matches_sentinel exercises the static decoder directly, with no browser.
• The consumers (persist at ~1845, Save button at ~743) already treat a true return as success — they were correct all along; the bug was the lying input, which is now fixed.

Tool Harness Findings

Fifteen tool calls confirmed:

• The new constant, builder, writer, and decoder all live at the expected line ranges in scripts/game_state.gd.
• The only other writer of the same localStorage state is save_settings at scripts/game_state.gd:685–688 (calls _local_storage_write(SETTINGS_KEY, payload) and discards the result — the pre-existing PR 409 follow-up, out of scope here per the PR body).
• No production code uses String(result) / String(false) / String(0) anywhere in scripts/, so the CI script-error rail concern in the PR description is satisfied.
• scripts/main.gd's persist() (~1832–1861) and save_game() (~743) already implement the desired success/retry semantics; they become correct on the web once _local_storage_write's return is truthful.
• tests/test_local_storage_xss.gd's helper functions (_extract_first_json_arg, _extract_second_json_arg, _find_top_level_comma) operate only on the JSON-encoded args and are reused unmodified — the round-trip flows still prove the key/payload survive JSON.stringify → eval-string → JSON.parse_string.
• tests/test_runner.gd is a separate suite that exercises the desktop path; the web-save XSS regression lives in test_local_storage_xss.gd, which CI runs headlessly per the AGENTS.md instructions.

Unknowns or Needs Verification

None for the verdict. Two items worth noting but not blocking:

• The PR body mentions a follow-up issue PR 409 for save_settings (which discards _local_storage_write's return). That is explicitly out of scope here, and the PR does not regress that path — save_settings continues to ignore the result as before.
• The fix is invisible to headless CI because no CI job runs the web build in a browser (the export-validation job only checks artifacts exist, per the issue body). The CI rail that does fire (tests/test_local_storage_xss.gd) covers the eval statement shape and the success-detection logic without a browser, which is the appropriate coverage for a stateless helper. Runtime browser verification remains a manual concern, matching the pre-existing repo posture.

Requirement trace

1 of 1 requirement(s) not fully traced to enforcement and a test:

• req-6f9f0abbedbf — unverifiable (no valid enforcement location)

Approval withheld: this review's coverage is incomplete — required-check coverage or the tool-loop investigation did not finish, so it is publishing as an advisory comment rather than an approval.