# STAGE 3 INDEPENDENT AUDIT — SAVE_INTEGRITY_PASS (commit `eecf825`) > Protocol 8 Stage 3. Auditor: Opus. Branch `dev`. Method: started from the DIFF and the RUNNING CODE, not the implementer's report. Every claim below was reproduced with the actual test harness / real boot, never taken on the commit message's word. Working tree restored clean after every probe. ## VERDICT: ✅ **PASS** This is the highest-bar surface in the project (the save write/read path) and it clears the bar. The two claims that matter most were both independently verified true, the migrate-parity fix is safe and non-regressive, and the persist() layer is correctly fail-safe. Findings below are minor and non-blocking. --- ## ⭐ THE TWO THAT MATTER ### (1) Is the save-survival test GENUINELY REAL? — **YES. I made it go red.** I did the red-then-green myself rather than trusting the commit message's claim of it. - **Baseline green:** ran `node tests/save-survival.mjs` on the committed code — all 35 checks pass, including "PATH1a … all 44 durable field-keys present, all 44 probed values intact". - **Injected a real field drop:** added `state.karma = 0;` immediately after the v8 fast-path merge in `_hydrateStateFromStorage()` (`js/ui/ui-core.js`), simulating a save-field lost to a default-reset. - **It went RED and NAMED the field:** ``` [FAIL] PATH1a save->reload (CURRENT) — field "karma" mismatch. expected=199 actual=0 [FAIL] PATH1b save->reload (MATURE active FNV) — field "karma" mismatch. expected=198 actual=0 ``` PATH3 (v7) correctly stayed green because my injection was on the v8 fast-path, not the v7 path — i.e. the detection is precise and path-specific, not a blanket pass/fail. - **Reverted; back to green.** Working tree confirmed clean (`git status` empty). The test is real for the right reasons: - It seeds fixtures via `addInitScript(localStorage.setItem('robco_v8', …))` and boots the **actual `file://index.html`** through the real `_hydrateStateFromStorage` / `#fileInput` import paths — not a `vm` sandbox and not a direct `migrateState()` call (Suites 12/46 already do that shallow thing). - The durable-field key set is sourced live from `window._defaultState` on every run (`snap.defaultStateKeys`), so a newly-added field is auto-covered — verified this is what `assertFieldsSurvive` iterates. - Comparison is a real recursive `deepEqual` on **parsed fields**, never serialized text. - The completeness-bearing paths (1a/1b/3) pass the **entire fixture** as `expected`, so every one of the 44 fields is value-checked (a default-reset is caught, not just an outright drop). The weaker paths (1c malformed / 2 import / 4 offline) do presence + targeted value checks and are honestly documented as such in-code. One structural nuance worth recording (not a defect): because the v8 fast-path uses a default-base spread, a "dropped" field reappears in `state` as its default, so the **presence** check alone can't catch a drop — the **value** check against the non-default fixture is what carries the weight. That is exactly how PATH1a/1b/3 are built, so the guarantee holds. ### (2) Is the "no data was lost on the pre-fix import path" claim TRUE? — **YES, verified empirically. The original bug was cosmetic, not data-loss.** I did not take this on trust (this is the same shape as the registry-leak "no corruption" claim that turned out false). I reproduced the pre-fix behavior: - Temporarily removed the migrate-parity block from `_writeImportedContainer` (`js/core/state.js`) and re-ran PATH2 (the real `#fileInput` import of the asymmetry fixture): ``` [PASS] PATH2 import(asymmetry) — every durable field-key present post-import [PASS] PATH2 import(asymmetry) — real campaign data (inventory, quests) survives the import [FAIL] PATH2 — campaignMode NOT normalized (got "ironman-bogus") [FAIL] PATH2 — mapView NOT normalized (got "bogus-huge") [FAIL] PATH2 — legacy faction key NOT pruned (factions.wgs={"fame":75,"infamy":0}) ``` - **Interpretation:** on the pre-fix path, **no durable field was dropped and no real campaign data was lost.** The only failures are the three *normalizations* — a bogus `campaignMode`/`mapView` left un-tidied, and a retired-faction key left **un-pruned** (i.e. *more* data retained, not less). The mechanism guarantees this: the fast-path's default-base spread can't drop a top-level field, and `migrateState` only adds/coerces/prunes — it never drops a valid field. So the "non-destructive, no data lost" line is **true and defensible** — the opposite outcome to the registry-leak precedent. The fix is a genuine parity-hardening (normalize the one arrival path that didn't), not a cover for a data-loss bug. --- ## Q3 — Did the migrate-parity fix BREAK anything? **No.** - **Mirrors cloud.js exactly.** The added block is byte-for-byte the same shape as `cloud.js` `loadCloudSave`/pull (`migrateState(data.version || '1.0', sanitized.campaigns[ctx])` per campaign). Protocol 22 honored: it extends the **one** shared container-write core (`_writeImportedContainer`), does not fork a second migrator. - **Other load paths untouched.** The diff only touches `_writeImportedContainer`; cloud/slot/backup migrate calls are unchanged. - **`migrateState` is idempotent** (only adds/coerces), so a normal same-version import is not over-migrated — PATH2 confirms real data survives and PATH1a/1b confirm current saves round-trip byte-clean. - **Shared with the bundle path.** `_writeImportedContainer` is also called by `applyBundleData` (full-backup restore), so bundle imports now migrate too. `buildFullBundle` stamps `version: APP_VERSION` and `robco_v8: {activeContext, campaigns}`, so the block iterates correctly and idempotently — safe. - **No new crash on malformed import.** I tested the one edge PATH2 doesn't cover: importing a *malformed* container (garbage-string `factions`, array `padBindings`, out-of-range `rads`) through the real `#fileInput`. Result: **no page error**, good data (inventory + manual note) survived, and the fix *improved* handling (`campaignMode`→standard, `mapView`→auto, `padBindings`→proper object, `rads`→1000). `migrateState`'s `s.factions.strip = {…}` on a string primitive is a silent no-op because `state.js` is a non-strict global script (not an ES module) — confirmed it does not throw. ## Q4 — persist() layer: **safe and correct.** - **Never throws into boot.** `_requestPersistentStorage()` is feature-detected, fully wrapped (outer `try/catch` + inner `Promise.catch`), and called **un-awaited** from `window.onload` after hydrate. The "absent `navigator.storage`" sub-case test passes with zero page errors — a storage check cannot become its own outage (Protocol 33 class). - **Denied path tested for real**, not just the happy path: Playwright overrides `persist()`/`persisted()` → false, and the test asserts `robco_storage_persisted === 'denied'` **and** the banner renders. The GRANTED sub-case asserts the banner does **not** render. Both pass. - **MetaStore key correct.** `robco_storage_persisted` is declared in `META_MANIFEST` (type `string`, default `''`, owner `ui-core.js`) — a device pref, **not** campaign state. It needs no `state.js`/`autoImportState`/sanitize entry (those are for campaign fields). The two-store-boundary test confirms it never leaks into `robco_v8`. - **Banner render (Protocol 10/17).** Rendered the denied banner at 360px and 412px directly via Playwright: `docOverflow = 0` at both widths (no horizontal overflow), `min-height 44px` tap target, `role="alert"`. Font is 0.82rem (~13px) — acceptable because it is a text banner, not a focusable input (Protocol 17's ≥16px rule targets inputs to prevent focus-zoom); it matches the `.update-banner` sibling precedent it was cloned from. --- ## ALSO VERIFIED - **Test count.** Node runner reports **ALL 3222 TESTS PASSED** (229 suites). Net +1 vs parent (3221→3222): the new Node test is **217.7b** (storage-warning-banner inert-template guard). Counts are synced across `CLAUDE.md`, `README.md`, `RULES.md`, `ARCHITECTURE.md`, and the `CHANGELOG` `[Unreleased]` header (`Tests: 3222/3222 | Cache: …-r29`). - **The claimed "saveToSlot fail-loud regression test" is real** — it lives in `save-survival.mjs` (the browser gate, lines ~1116-1169), not the Node runner, consistent with the commit message. It mocks a total IDB+LS write failure and asserts `saveToSlot()` warns loudly and leaves the prior slot intact. Both assertions pass. (I initially flagged the +1-vs-+2 discrepancy; resolved — the second "test" is a browser check, correctly not counted in 3222.) - **No assertion was weakened.** The two allowlist edits are legitimate, not loosening: `window.onload` line cap `57→58` tracks the real newly-added named call `_requestPersistentStorage()`; Suite 217.5's banner allowlist gains `.storage-warning-banner` (a genuinely new, reviewed banner). **No save/migrate/import assertion was loosened** — I specifically hunted for that. - **Fail-loud is real.** The `beforeunload` flush `setItem` is now wrapped in `try/catch` with a `console.error` (was unguarded). The `saveState()` quota path and the `saveToSlot()` total-failure path are both exercised and assert warn-loudly + prior-value-intact. - **Protocol 1:** `CACHE_NAME` bumped r28→r29 (served files changed). **Protocol 38:** the fix is game-agnostic (iterates `Object.keys(campaigns)`); the fixture's `FNV_FACTIONS`/`FO3_FACTIONS` literals are test-fixture data (concrete example saves), explicitly justified against the `render-check.mjs` precedent — acceptable. **Protocol 41:** repo clean, no junk. My own two scratch probes were deleted; `git status` empty. - **CHANGELOG accuracy.** The persist() `[Unreleased]` "Added" entry is plain-English and true to the running app. --- ## MINOR OBSERVATIONS (non-blocking — for the record, not a bounce) 1. **Garbage-*string* `factions` is still not coerced, even through the import path.** `sanitizeImportedContainer` only rebuilds `factions` when it's already an object, and `migrateState` can't coerce a string (its `s.factions.strip=…` is a silent no-op in non-strict mode). This is a **pre-existing** gap (documented as a NOTE in PATH1c), **unchanged** by this commit, and not a realistic corruption (no writer produces a string there). If ever hardened, the right place is `sanitizeImportedContainer` (coerce non-object `factions` → `{}` so `migrateState` rebuilds it) — a future item, not this unit's regression. 2. **`save-survival.mjs` is ESLint-ignored**, same as its browser-harness siblings (`browser-shared.mjs`, `test-html-check.mjs`). Consistent with precedent, but it means 1284 lines of test code get no lint; it is instead exercised by running in the gate. 3. **MATURE `eventLog` is 40 entries**, not "near the 1000 cap" as the plan's wording suggested. Immaterial to coverage. 4. **`library/TEST_CATALOG.md` left unsynced** — the commit message flags this openly (gitignored, local-only, hand-synced class). Acceptable and honestly disclosed, not a silent skip. --- ## DONE-CRITERIA CHECK (Protocol 26) | Acceptance criterion | Status | |---|---| | Behavioural test boots real fixtures through real boot path | ✅ verified (red-then-green reproduced) | | Full durable-field inventory sourced from `_defaultState`, parsed-field compare | ✅ | | Six paths (save→reload · import · v7→migrate · offline · failed-import · conflict) | ✅ all present & green | | Two-store boundary asserted | ✅ | | Migrate-parity verdict stated + fix extends shared path + regression-locked | ✅ (verdict: non-destructive; fix = parity-hardening) | | `persist()` fire-and-forget, never throws, records state | ✅ | | Denied path tested for real, banner only on denied | ✅ | | Fail-loud audited & locked | ✅ | | Full gate green, counts synced, cache bumped, docs updated, game-agnostic, no fork | ✅ | **Recommendation: PASS. The change is live-safe on `dev` and the save-survival gate is a genuine, self-sourcing regression net — not a false certificate.**