From 4fdc7e71c76e45a9120767e0fe47e9c2db8f3f15 Mon Sep 17 00:00:00 2001 From: Valentin PLANES Date: Sun, 13 Sep 2026 19:19:28 +0200 Subject: [PATCH 1/2] Skip objects whose bodies won't parse instead of failing the load A modded save serialises property shapes this parser has never seen. One such object sank the whole save: the payload/index build latched the first re-parse error and returned Err, so 106 Modular Load Balancer buildables out of 37,958 objects produced "Failed to load save: object re-parse failed during index build: ... String decode failure". The mod's mIndexMapping is a Map whose LBBalancerIndexing value is a native struct written as 12 raw bytes, not a tagged property list. StructProperty and ArrayProperty already fall back to opaque bytes for unknown modded structs; the map-value path had no such fallback and parsed those bytes as a property name/type pair. Rather than teach the parser one more shape, stop treating an unparsable body as fatal. Every collector already skips a None object, and buildings render from the header (typePath/position/rotation), so all 159 balancers still draw -- only their body-derived contents are missing. The set of skipped objects is reported on the payload as `unreadableObjects` and noted in the load status line. Edits stay strictly checked: the failure set rides on MapIndex (so it survives the CBOR handoff to the lean worker) and finish_edit refuses an edit that makes a previously-parsable object stop parsing -- that is this editor corrupting the save, which the rebuild still exists to catch. Co-Authored-By: Claude Opus 5 (1M context) --- map/static/map/data.js | 31 ++++++++- rust_parser/core/src/mapdata/index.rs | 13 +++- rust_parser/core/src/mapdata/mod.rs | 43 ++++++++++--- rust_parser/core/src/mapdata/scan.rs | 92 ++++++++++++++++++++++----- rust_parser/tauri/src/session.rs | 20 ++++++ rust_parser/wasm/src/lib.rs | 23 +++++++ 6 files changed, 192 insertions(+), 30 deletions(-) diff --git a/map/static/map/data.js b/map/static/map/data.js index 11d5cbb..d9d4285 100644 --- a/map/static/map/data.js +++ b/map/static/map/data.js @@ -17,6 +17,31 @@ loadStatus.textContent = text; } + // "Loaded: ()", plus a note when the parser had to skip + // objects. Skipping is normal on modded saves -- the parser doesn't know + // every mod's property layout -- and buildables still draw, from their + // headers, so the note explains a real but narrow gap (their contents and + // details) rather than announcing a failure. + function loadedStatus(payload) { + var text = "Loaded: " + payload.sessionName + " (" + payload.saveDatetime + ")"; + var skipped = payload.unreadableObjects; + if (!skipped || !skipped.count) { + return text; + } + var plural = skipped.count === 1 ? "" : "s"; + text += " — " + skipped.count + " object" + plural + + " could not be read (usually modded buildings); " + + (skipped.count === 1 ? "it still shows" : "they still show") + + " on the map, without their contents"; + // The type paths and byte offsets are the useful half of a bug report, + // but far too much for the status line -- console only. + if (skipped.samples && skipped.samples.length) { + console.warn("Skipped " + skipped.count + " unreadable object(s):"); + skipped.samples.forEach(function(sample) { console.warn(" " + sample); }); + } + return text; + } + function showProgress(phase, percent) { progressBar.style.display = "block"; progressFill.style.width = Math.max(0, Math.min(100, percent)) + "%"; @@ -275,7 +300,7 @@ restorePinnedSelection(pinnedSelection); } Analytics.saveLoaded("file", file.size, performance.now() - startedAt); - setStatus("Loaded: " + payload.sessionName + " (" + payload.saveDatetime + ")"); + setStatus(loadedStatus(payload)); }) .catch(function(error) { hideProgress(); @@ -324,7 +349,7 @@ if (pinnedSelection) { restorePinnedSelection(pinnedSelection); } - setStatus("Loaded: " + payload.sessionName + " (" + payload.saveDatetime + ")"); + setStatus(loadedStatus(payload)); }) .catch(function(error) { hideProgress(); @@ -418,7 +443,7 @@ restorePinnedSelection(pinnedSelection); } Analytics.saveLoaded("url", bytes, performance.now() - startedAt); - setStatus("Loaded: " + payload.sessionName + " (" + payload.saveDatetime + ")"); + setStatus(loadedStatus(payload)); }) .catch(function(error) { hideProgress(); diff --git a/rust_parser/core/src/mapdata/index.rs b/rust_parser/core/src/mapdata/index.rs index 3bc4c92..4245397 100644 --- a/rust_parser/core/src/mapdata/index.rs +++ b/rust_parser/core/src/mapdata/index.rs @@ -12,7 +12,7 @@ use super::geometry::{project_xy, world_z_to_meters}; use super::jsonval::jnum; use super::names::readable_label; use super::props; -use super::scan::{SaveScan, Slot}; +use super::scan::{ParseFailures, SaveScan, Slot}; use crate::extract::find_prop; use crate::store::*; use indexmap::IndexMap; @@ -120,6 +120,13 @@ pub struct MapIndex { pub item_location_index: IndexMap, Vec<(Vec, i64)>>, pub dimensional_depot_by_item: IndexMap, pub static_item_locations: IndexMap>, + /// Objects the build could not re-parse and therefore skipped (modded + /// property shapes, overwhelmingly). Carried on the index so it survives + /// the CBOR handoff to the lean worker, which never runs a build of its + /// own yet still has to diff this set when an edit rebuilds (see + /// SaveSession::finish_edit). `default` keeps older blobs loadable. + #[serde(default)] + pub parse_failures: ParseFailures, } impl MapIndex { @@ -417,6 +424,10 @@ impl MapIndex { item_location_index, dimensional_depot_by_item, static_item_locations, + // Last field for a reason: every collector above has run, so this + // snapshot covers the whole build (the payload build shares the + // scan and runs first -- see build_all_json). + parse_failures: scan.parse_failures(), } } } diff --git a/rust_parser/core/src/mapdata/mod.rs b/rust_parser/core/src/mapdata/mod.rs index f2bb275..7fc603c 100644 --- a/rust_parser/core/src/mapdata/mod.rs +++ b/rust_parser/core/src/mapdata/mod.rs @@ -305,12 +305,6 @@ fn build_payload_json_with_scan( cb(done, BUILD_STEP_COUNT); } } - // On-demand re-parses can't fail on bytes that already parsed, but once - // the pipeline is lean this build validates edited bodies -- fail loud - // rather than emit a payload with a silently-skipped object. - if let Some(e) = scan.parse_error() { - return Err(format!("object re-parse failed during payload build: {e}")); - } out.push(b'}'); Ok(out) } @@ -334,10 +328,41 @@ pub fn build_all_json( collectors::STEP_ORDER.to_vec(), Some(&mut tick), )?; + // Objects whose bodies would not re-parse are skipped, not fatal: a + // modded save serializes property shapes this parser has never seen, and + // one such object must not sink a 40k-object save. Every collector + // already skips a None object, and buildings render off the header + // (position/rotation/typePath), so a skipped buildable still appears on + // the map -- only its body-derived detail is missing. The set rides out + // on the index; the edit paths diff it to keep catching real corruption. let map_index = index::MapIndex::build_with_scan(&scan); - if let Some(e) = scan.parse_error() { - return Err(format!("object re-parse failed during index build: {e}")); - } + // The payload is serialized before the index is built, but objects the + // INDEX build could not read belong in the same report -- so splice the + // key in here, where both halves are in hand, rather than teach every + // collector about it. + let mut payload = payload; + append_unreadable_report(&mut payload, &map_index.parse_failures); tick(BUILD_STEP_COUNT, BUILD_STEP_COUNT); Ok((payload, map_index)) } + +/// Add `"unreadableObjects": {count, samples}` to a finished payload object, +/// so the frontend can tell the user what the map is missing. Absent +/// entirely on a save that parsed cleanly -- which is nearly all of them. +fn append_unreadable_report(payload: &mut Vec, failures: &scan::ParseFailures) { + if failures.is_empty() { + return; + } + debug_assert_eq!(payload.last(), Some(&b'}')); + payload.pop(); + if payload.last() != Some(&b'{') { + payload.push(b','); + } + let report = serde_json::json!({ + "count": failures.len(), + "samples": failures.samples, + }); + payload.extend_from_slice(b"\"unreadableObjects\":"); + payload.extend_from_slice(report.to_string().as_bytes()); + payload.push(b'}'); +} diff --git a/rust_parser/core/src/mapdata/scan.rs b/rust_parser/core/src/mapdata/scan.rs index 920014e..b7896e8 100644 --- a/rust_parser/core/src/mapdata/scan.rs +++ b/rust_parser/core/src/mapdata/scan.rs @@ -6,7 +6,7 @@ use super::consts::{GAME_STATE_TYPE_PATH_SUBSTRING, LIGHTWEIGHT_BUILDABLE_SUBSYSTEM_TYPE_PATH}; use crate::extract::InstanceSlots; use crate::store::*; -use indexmap::IndexMap; +use indexmap::{IndexMap, IndexSet}; use std::cell::{OnceCell, RefCell}; use std::collections::HashMap; @@ -44,11 +44,49 @@ pub struct SaveScan<'a> { hard_drives: OnceCell, depot_contents: OnceCell, catalog_drops: OnceCell>, - /// First on-demand re-parse failure seen during a build. Re-parsing bytes - /// that already parsed cannot fail, but once the pipeline is lean the - /// build IS the deep validation of an edited body -- so build_all_json - /// turns a latched error into Err instead of emitting a wrong payload. - parse_error: RefCell>, + /// Objects whose body failed to re-parse during a build. Modded saves + /// serialize property shapes this parser does not know, so a failure here + /// is normal content rather than corruption: the collectors already skip + /// a None object, and the build reports the set instead of failing. The + /// edit paths turn it back into a hard error for objects that parsed + /// before an edit and stopped after it (see finish_edit) -- that IS + /// corruption, and the build is still its deep validation. + parse_failures: RefCell, +} + +/// Objects a build could not re-parse, identified by instanceName so the set +/// survives an edit that shifts slot indices. +#[derive(Default, Clone, serde::Serialize, serde::Deserialize)] +pub struct ParseFailures { + /// instanceName of each unparsable object, deduplicated, in first-seen + /// order (one object is re-parsed by several collectors). + pub names: IndexSet>, + /// The first few failures verbatim, for the user-facing report. Capped: + /// a save where everything fails must not build a 600k-string list. + pub samples: Vec, +} + +/// How many failure messages `ParseFailures::samples` keeps. +const MAX_FAILURE_SAMPLES: usize = 5; + +impl ParseFailures { + pub fn is_empty(&self) -> bool { + self.names.is_empty() + } + + pub fn len(&self) -> usize { + self.names.len() + } + + /// Failures present in `self` but not in `baseline` -- an edit's new + /// breakage, as opposed to mod content that never parsed to begin with. + pub fn newly_failing<'s>(&'s self, baseline: &ParseFailures) -> Vec<&'s [u8]> { + self.names + .iter() + .filter(|n| !baseline.names.contains(*n)) + .map(|n| n.as_slice()) + .collect() + } } impl<'a> SaveScan<'a> { @@ -86,7 +124,7 @@ impl<'a> SaveScan<'a> { hard_drives: OnceCell::new(), depot_contents: OnceCell::new(), catalog_drops: OnceCell::new(), - parse_error: RefCell::new(None), + parse_failures: RefCell::new(ParseFailures::default()), } } @@ -144,22 +182,42 @@ impl<'a> SaveScan<'a> { } /// Re-parse the object at `slot` from its byte span (owned; identical to - /// the eager parse). Returns None on failure, latching the first error so - /// the build can fail cleanly. Replaces the old `object()` borrow into the - /// resident model, so the builder never needs `parsed_objects()`. + /// the eager parse). Returns None on failure, recording the object so the + /// build can report what it skipped. Replaces the old `object()` borrow + /// into the resident model, so the builder never needs `parsed_objects()`. pub fn parse_object(&self, slot: Slot) -> Option { match self.store.parse_object_at(slot.0, slot.1) { Ok(object) => Some(object), Err(e) => { - let mut latched = self.parse_error.borrow_mut(); - if latched.is_none() { - *latched = Some(format!("object at {slot:?}: {e}")); - } + self.record_parse_failure(slot, &e.msg); None } } } + /// Identity is the instanceName, not the slot: an edit that splices bytes + /// renumbers slots, and the edit paths diff these sets across that. + fn record_parse_failure(&self, slot: Slot, msg: &str) { + let name = match self.header(slot) { + Header::Actor(a) => a.instance_name, + Header::Component(c) => c.instance_name, + }; + let mut failures = self.parse_failures.borrow_mut(); + if !failures.names.insert(name.bytes(self.data()).to_vec()) { + return; // already recorded -- several collectors parse one object + } + if failures.samples.len() < MAX_FAILURE_SAMPLES { + let type_path = match self.header(slot) { + Header::Actor(a) => a.type_path, + Header::Component(c) => c.class_name, + }; + failures.samples.push(format!( + "{} at {slot:?}: {msg}", + String::from_utf8_lossy(type_path.bytes(self.data())) + )); + } + } + /// scan.objectsByInstanceName.get(name), re-parsed on demand. pub fn parse_object_by_name(&self, name: &[u8]) -> Option { let slot = *self.by_instance_name.get(name)?; @@ -181,9 +239,9 @@ impl<'a> SaveScan<'a> { .as_ref() } - /// The first on-demand re-parse error latched during the build, if any. - pub fn parse_error(&self) -> Option { - self.parse_error.borrow().clone() + /// Objects this build could not re-parse (empty on a clean save). + pub fn parse_failures(&self) -> ParseFailures { + self.parse_failures.borrow().clone() } pub fn header_by_name(&self, name: &[u8]) -> Option<&'a Header> { diff --git a/rust_parser/tauri/src/session.rs b/rust_parser/tauri/src/session.rs index 90af65f..1126de9 100644 --- a/rust_parser/tauri/src/session.rs +++ b/rust_parser/tauri/src/session.rs @@ -20,6 +20,25 @@ use sav_core::object::ClassTables; use sav_core::save_header::SaveFileInfo; use sav_core::store::SaveStore; +/// The build tolerates objects it cannot re-parse (modded property shapes), +/// but an object that parsed BEFORE an edit and stops after it is this +/// editor corrupting the save -- the rebuild is that check. Refuse the edit +/// rather than keep a payload built from a damaged body. +fn check_no_new_parse_failures(before: Option<&MapIndex>, after: &MapIndex) -> Result<(), String> { + // No baseline (a session recovering from a failed edit) -- nothing to + // diff against, so don't invent a failure. + let Some(before) = before else { return Ok(()) }; + let broken = after.parse_failures.newly_failing(&before.parse_failures); + if broken.is_empty() { + return Ok(()); + } + Err(format!( + "edit aborted: {} object(s) no longer parse after this edit (e.g. {}) -- the save was left unchanged", + broken.len(), + String::from_utf8_lossy(broken[0]), + )) +} + const SESSION_LOST: &str = "No usable save state (a failed edit was not recovered) -- reload the save file"; @@ -96,6 +115,7 @@ impl AppSession { let mut build_progress = |current: u64, total: u64| progress(2, current, total); let (payload_json, index) = mapdata::build_all_json(&new_store, Some(&mut build_progress))?; + check_no_new_parse_failures(self.index.as_ref(), &index)?; self.store = Some(new_store); self.index = Some(index); Ok(payload_json) diff --git a/rust_parser/wasm/src/lib.rs b/rust_parser/wasm/src/lib.rs index 0257a53..3889523 100644 --- a/rust_parser/wasm/src/lib.rs +++ b/rust_parser/wasm/src/lib.rs @@ -135,6 +135,28 @@ pub struct SaveSession { info: sav_core::save_header::SaveFileInfo, } +/// The build tolerates objects it cannot re-parse (modded property shapes), +/// but an object that parsed BEFORE an edit and stops after it is this +/// editor corrupting the save -- the rebuild is that check. Refuse the edit +/// rather than hand back a payload built from a damaged body. +fn check_no_new_parse_failures( + before: Option<&MapIndex>, + after: &MapIndex, +) -> Result<(), JsError> { + // No baseline (a session recovering from a failed edit) -- nothing to + // diff against, so don't invent a failure. + let Some(before) = before else { return Ok(()) }; + let broken = after.parse_failures.newly_failing(&before.parse_failures); + if broken.is_empty() { + return Ok(()); + } + Err(JsError::new(&format!( + "edit aborted: {} object(s) no longer parse after this edit (e.g. {}) -- the save was left unchanged", + broken.len(), + String::from_utf8_lossy(broken[0]), + ))) +} + const SESSION_LOST: &str = "No usable save state (a failed edit was not recovered) -- reload the save file"; @@ -166,6 +188,7 @@ impl SaveSession { let (payload_json, index) = mapdata::build_all_json(&new_store, Some(&mut build_progress)) .map_err(|e| JsError::new(&e))?; + check_no_new_parse_failures(self.index.as_ref(), &index)?; self.store = Some(Arc::new(new_store)); self.index = Some(index); Ok(payload_json) From 8976659aa0871d115d7d1138d25d427324763d5c Mon Sep 17 00:00:00 2001 From: Valentin PLANES Date: Sun, 13 Sep 2026 19:53:37 +0200 Subject: [PATCH 2/2] Scope the edit gate precisely and explain unreadable-object edit errors Two follow-ups from testing the skip on the reporter's save. The edit gate now lives on MapIndex and only rejects objects that EXISTED and parsed before the edit. Copy/paste splices an object's bytes under a fresh instanceName, so duplicating an unreadable modded buildable produces a new unreadable object; the earlier name-set diff read that as corruption and blocked a working operation. A mis-computed splice damaging a neighbour -- the case the check exists for -- still trips it. Editor ops parse their target eagerly, so editing an unreadable object now reports "String decode failure at offset 24369724 of length 3670015". That was unreachable before (the save did not load at all) and says nothing about what the user did. All fourteen op call sites reach an object through apply::fetch, so one wrap there names the object and explains it can be viewed but not edited. Adds tests/unparsable_objects.rs: a clobbered body must not fail the build, must be reported on the payload, must still render as a building, and the edit gate must fire on exactly the newly-broken pre-existing objects. Co-Authored-By: Claude Opus 5 (1M context) --- map/static/map/data.js | 7 +- rust_parser/core/src/editor/apply.rs | 19 ++- rust_parser/core/src/mapdata/index.rs | 37 +++++ rust_parser/core/tests/unparsable_objects.rs | 158 +++++++++++++++++++ rust_parser/tauri/src/session.rs | 21 +-- rust_parser/wasm/src/lib.rs | 24 +-- 6 files changed, 217 insertions(+), 49 deletions(-) create mode 100644 rust_parser/core/tests/unparsable_objects.rs diff --git a/map/static/map/data.js b/map/static/map/data.js index d9d4285..0f0d780 100644 --- a/map/static/map/data.js +++ b/map/static/map/data.js @@ -28,11 +28,8 @@ if (!skipped || !skipped.count) { return text; } - var plural = skipped.count === 1 ? "" : "s"; - text += " — " + skipped.count + " object" + plural + - " could not be read (usually modded buildings); " + - (skipped.count === 1 ? "it still shows" : "they still show") + - " on the map, without their contents"; + text += " — " + skipped.count + " object" + (skipped.count === 1 ? "" : "s") + + " could not be read (usually modded); shown on the map without contents"; // The type paths and byte offsets are the useful half of a bug report, // but far too much for the status line -- console only. if (skipped.samples && skipped.samples.length) { diff --git a/rust_parser/core/src/editor/apply.rs b/rust_parser/core/src/editor/apply.rs index acf2857..0a7d963 100644 --- a/rust_parser/core/src/editor/apply.rs +++ b/rust_parser/core/src/editor/apply.rs @@ -421,8 +421,25 @@ pub(crate) fn actor_slots_of_types(store: &SaveStore, candidates: &[&str]) -> Ve /// One object, re-parsed on demand from its span (identical to the eagerly /// parsed model; StrRefs point into the same `store.data`). +/// +/// A body this parser cannot read is no longer fatal at load -- the map +/// skips such an object and still draws it from its header -- so edits are +/// where the user now meets one, and the raw "String decode failure at +/// offset N" says nothing about what they did or what to do. Every editor +/// op reaches an object through here, so one wrap covers all of them. pub(crate) fn fetch(store: &SaveStore, li: usize, oi: usize) -> PResult { - store.parse_object_at(li, oi) + store.parse_object_at(li, oi).map_err(|e| { + let name = match &store.levels[li].headers[oi] { + Header::Actor(a) => a.instance_name, + Header::Component(c) => c.instance_name, + }; + perr!( + "cannot edit {}: this object is stored in a format this app cannot read \ + (usually a mod's own data), so it can be viewed on the map but not edited [{}]", + String::from_utf8_lossy(name.bytes(&store.data)), + e.msg, + ) + }) } // --------------------------------------------------------------------------- diff --git a/rust_parser/core/src/mapdata/index.rs b/rust_parser/core/src/mapdata/index.rs index 4245397..a35dd47 100644 --- a/rust_parser/core/src/mapdata/index.rs +++ b/rust_parser/core/src/mapdata/index.rs @@ -130,6 +130,43 @@ pub struct MapIndex { } impl MapIndex { + /// The edit-path gate, `self` being the index rebuilt after an edit. + /// + /// A build tolerates objects it cannot re-parse (modded property shapes + /// this parser has never seen), but an object that EXISTED and parsed + /// before an edit and stops parsing after it is this editor corrupting + /// the save -- a mis-computed splice damaging its neighbours looks + /// exactly like that -- so the rebuild stays that check and the edit is + /// refused. + /// + /// Deliberately scoped to objects present before the edit. Copy/paste + /// splices an object's bytes under a fresh instanceName, so pasting an + /// unreadable modded buildable legitimately produces a new unreadable + /// object; treating that as corruption would block a working operation. + /// Newly created objects are the editor's own output and are covered by + /// the editor round-trip tests instead. + /// + /// `before` is None for a session with nothing to diff against (one + /// recovering from a failed edit), which must not read as "all new". + pub fn reject_new_parse_failures(&self, before: Option<&MapIndex>) -> Result<(), String> { + let Some(before) = before else { return Ok(()) }; + let broken: Vec<&[u8]> = self + .parse_failures + .newly_failing(&before.parse_failures) + .into_iter() + .filter(|name| before.by_instance_name.contains_key(*name)) + .collect(); + if broken.is_empty() { + return Ok(()); + } + Err(format!( + "edit aborted: {} object(s) that parsed before this edit no longer do \ + (e.g. {}) -- the save was left unchanged", + broken.len(), + String::from_utf8_lossy(broken[0]), + )) + } + /// scan.headersByInstanceName.get(name) -- the Header, or None. pub fn header_by_name<'a>(&self, store: &'a SaveStore, name: &[u8]) -> Option<&'a Header> { self.by_instance_name.get(name).map(|&(li, oi)| &store.levels[li].headers[oi]) diff --git a/rust_parser/core/tests/unparsable_objects.rs b/rust_parser/core/tests/unparsable_objects.rs new file mode 100644 index 0000000..98ed7c6 --- /dev/null +++ b/rust_parser/core/tests/unparsable_objects.rs @@ -0,0 +1,158 @@ +//! A body this parser cannot read must not sink the whole save. +//! +//! Modded saves serialise property shapes sav_core has never seen (the case +//! that prompted this: Modular Load Balancers stores a native 12-byte struct +//! as a MapProperty value, where the parser expects a tagged property list). +//! The build skips such an object and reports it instead of failing, and the +//! object still draws on the map because buildings render off the header. + +use sav_core::level::parse_full_save_lean; +use sav_core::mapdata; +use sav_core::mapdata::index::MapIndex; +use sav_core::mapdata::scan::ParseFailures; +use sav_core::object::ClassTables; +use sav_core::store::{Header, SaveStore}; +use std::path::PathBuf; + +fn load_lean(name: &str) -> SaveStore { + let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../map/uploads").join(name); + let bytes = std::fs::read(path).expect("test save present"); + parse_full_save_lean(&bytes, &ClassTables::embedded(), None).unwrap() +} + +const SAVE: &str = "All_080726-163150.sav"; + +/// Find a buildable whose body parses today, and return its slot, type path +/// and instance name. Buildable so the "still renders" half is testable. +fn a_parsable_buildable(store: &SaveStore) -> ((usize, usize), String, String) { + for (li, level) in store.levels.iter().enumerate() { + for (oi, header) in level.headers.iter().enumerate() { + let Header::Actor(actor) = header else { continue }; + let type_path = actor.type_path.to_string(&store.data); + if !type_path.contains("/Build_") || store.parse_object_at(li, oi).is_err() { + continue; + } + return ((li, oi), type_path, actor.instance_name.to_string(&store.data)); + } + } + panic!("no parsable buildable in {SAVE}"); +} + +/// 0xFF over the whole body span: every length prefix in it becomes absurd, +/// so the property walk fails wherever it first looks. The headers (and the +/// recorded spans) are untouched -- exactly the shape of the real bug, where +/// the header is fine and only the body is unreadable. +fn clobber_body(store: &mut SaveStore, (li, oi): (usize, usize)) { + let (off, len) = store.levels[li].object_spans[oi]; + store.data[off..off + len as usize].fill(0xFF); + assert!( + store.parse_object_at(li, oi).is_err(), + "clobbered body still parsed -- the test corrupts nothing", + ); +} + +#[test] +fn clean_save_reports_no_unreadable_objects() { + let store = load_lean(SAVE); + let (payload, index) = mapdata::build_all_json(&store, None).expect("build"); + assert!(index.parse_failures.is_empty()); + // Absent entirely rather than a zero count: nearly every save is clean, + // and the frontend keys the notice off the key's presence. + let payload: serde_json::Value = serde_json::from_slice(&payload).unwrap(); + assert!(payload.get("unreadableObjects").is_none()); +} + +#[test] +fn unreadable_body_is_skipped_and_reported_not_fatal() { + let mut store = load_lean(SAVE); + let (slot, type_path, instance_name) = a_parsable_buildable(&store); + clobber_body(&mut store, slot); + + // The whole point: this used to be Err. + let (payload, index) = mapdata::build_all_json(&store, None).expect("build must not fail"); + + assert_eq!(index.parse_failures.len(), 1); + assert!(index.parse_failures.names.contains(instance_name.as_bytes())); + + let payload: serde_json::Value = serde_json::from_slice(&payload).unwrap(); + let report = &payload["unreadableObjects"]; + assert_eq!(report["count"], 1); + assert!( + report["samples"][0].as_str().unwrap().contains(&type_path), + "sample should name the class: {report}", + ); +} + +/// The map keeps the building: position/rotation/typePath live in the header, +/// which parses fine -- only the body-derived detail is lost. +#[test] +fn unreadable_buildable_still_renders() { + let store = load_lean(SAVE); + let (slot, type_path, instance_name) = a_parsable_buildable(&store); + let short_name = instance_name.rsplit('.').next().unwrap().to_string(); + + let mut broken = load_lean(SAVE); + clobber_body(&mut broken, slot); + let (payload, _) = mapdata::build_all_json(&broken, None).expect("build"); + let payload: serde_json::Value = serde_json::from_slice(&payload).unwrap(); + + let rendered = payload["buildingCategories"] + .as_array() + .unwrap() + .iter() + .flat_map(|category| category["types"].as_array().unwrap()) + .filter(|bucket| bucket["typePath"] == type_path.as_str()) + .any(|bucket| { + bucket["ids"] + .as_array() + .unwrap() + .iter() + .any(|id| id.as_str() == Some(short_name.as_str())) + }); + assert!(rendered, "{instance_name} vanished from the map after its body failed to parse"); +} + +/// Edits stay strictly checked: an object that existed and parsed before an +/// edit must still parse after it. Pre-existing failures and objects the +/// edit itself created are not that. +#[test] +fn edit_gate_rejects_only_newly_broken_existing_objects() { + // A save holding two objects, one of them unreadable modded content. + let mut before = MapIndex::build(&load_lean(SAVE)); + before.by_instance_name.clear(); + for name in [b"modded_thing".as_slice(), b"was_fine".as_slice()] { + before.by_instance_name.insert(name.to_vec(), (0, 0)); + } + before.parse_failures = ParseFailures::default(); + before.parse_failures.names.insert(b"modded_thing".to_vec()); + + let after = |failures: &[&[u8]]| { + let mut index = MapIndex::build(&load_lean(SAVE)); + index.parse_failures = ParseFailures::default(); + for name in failures { + index.parse_failures.names.insert(name.to_vec()); + } + index + }; + + // Unchanged, and "the edit removed the broken object", are both fine. + assert!(after(&[b"modded_thing"]).reject_new_parse_failures(Some(&before)).is_ok()); + assert!(after(&[]).reject_new_parse_failures(Some(&before)).is_ok()); + + // An object that parsed before this edit and no longer does: corruption. + let err = after(&[b"modded_thing", b"was_fine"]) + .reject_new_parse_failures(Some(&before)) + .unwrap_err(); + assert!(err.contains("was_fine"), "{err}"); + assert!(!err.contains("modded_thing"), "pre-existing failure reported as new: {err}"); + + // Pasting an unreadable buildable splices its bytes under a fresh name -- + // a new unreadable object, not damage to anything that existed. + assert!(after(&[b"modded_thing", b"pasted_copy"]) + .reject_new_parse_failures(Some(&before)) + .is_ok()); + + // No baseline (a session recovering from a failed edit) must not read as + // "everything is new". + assert!(after(&[b"was_fine"]).reject_new_parse_failures(None).is_ok()); +} diff --git a/rust_parser/tauri/src/session.rs b/rust_parser/tauri/src/session.rs index 1126de9..455e7ac 100644 --- a/rust_parser/tauri/src/session.rs +++ b/rust_parser/tauri/src/session.rs @@ -20,25 +20,6 @@ use sav_core::object::ClassTables; use sav_core::save_header::SaveFileInfo; use sav_core::store::SaveStore; -/// The build tolerates objects it cannot re-parse (modded property shapes), -/// but an object that parsed BEFORE an edit and stops after it is this -/// editor corrupting the save -- the rebuild is that check. Refuse the edit -/// rather than keep a payload built from a damaged body. -fn check_no_new_parse_failures(before: Option<&MapIndex>, after: &MapIndex) -> Result<(), String> { - // No baseline (a session recovering from a failed edit) -- nothing to - // diff against, so don't invent a failure. - let Some(before) = before else { return Ok(()) }; - let broken = after.parse_failures.newly_failing(&before.parse_failures); - if broken.is_empty() { - return Ok(()); - } - Err(format!( - "edit aborted: {} object(s) no longer parse after this edit (e.g. {}) -- the save was left unchanged", - broken.len(), - String::from_utf8_lossy(broken[0]), - )) -} - const SESSION_LOST: &str = "No usable save state (a failed edit was not recovered) -- reload the save file"; @@ -115,7 +96,7 @@ impl AppSession { let mut build_progress = |current: u64, total: u64| progress(2, current, total); let (payload_json, index) = mapdata::build_all_json(&new_store, Some(&mut build_progress))?; - check_no_new_parse_failures(self.index.as_ref(), &index)?; + index.reject_new_parse_failures(self.index.as_ref())?; self.store = Some(new_store); self.index = Some(index); Ok(payload_json) diff --git a/rust_parser/wasm/src/lib.rs b/rust_parser/wasm/src/lib.rs index 3889523..d3116e1 100644 --- a/rust_parser/wasm/src/lib.rs +++ b/rust_parser/wasm/src/lib.rs @@ -135,28 +135,6 @@ pub struct SaveSession { info: sav_core::save_header::SaveFileInfo, } -/// The build tolerates objects it cannot re-parse (modded property shapes), -/// but an object that parsed BEFORE an edit and stops after it is this -/// editor corrupting the save -- the rebuild is that check. Refuse the edit -/// rather than hand back a payload built from a damaged body. -fn check_no_new_parse_failures( - before: Option<&MapIndex>, - after: &MapIndex, -) -> Result<(), JsError> { - // No baseline (a session recovering from a failed edit) -- nothing to - // diff against, so don't invent a failure. - let Some(before) = before else { return Ok(()) }; - let broken = after.parse_failures.newly_failing(&before.parse_failures); - if broken.is_empty() { - return Ok(()); - } - Err(JsError::new(&format!( - "edit aborted: {} object(s) no longer parse after this edit (e.g. {}) -- the save was left unchanged", - broken.len(), - String::from_utf8_lossy(broken[0]), - ))) -} - const SESSION_LOST: &str = "No usable save state (a failed edit was not recovered) -- reload the save file"; @@ -188,7 +166,7 @@ impl SaveSession { let (payload_json, index) = mapdata::build_all_json(&new_store, Some(&mut build_progress)) .map_err(|e| JsError::new(&e))?; - check_no_new_parse_failures(self.index.as_ref(), &index)?; + index.reject_new_parse_failures(self.index.as_ref()).map_err(|e| JsError::new(&e))?; self.store = Some(Arc::new(new_store)); self.index = Some(index); Ok(payload_json)