fix(slicer): strip zero-valued filament-index sentinels, and sanitise the preview slice too (issue #3030)

Bambu Studio writes 0 into wall_filament, sparse_infill_filament and
solid_infill_filament to mean "use whichever filament the object is set
to". Bambu Studio and OrcaSlicer 2.4 define these min 0 and accept it;
OrcaSlicer 2.3 and earlier used the 1-based scheme (min 1, default 1)
and reject it with "0 not in range [1.000000,...]". Sidecar images are
version-tagged, so an install can be pinned to one of those builds.

Same shape as the -1 inherit markers from #1201 with a different marker,
so the allowlist becomes a key-to-marker map rather than one global
constant. The buckets must not bleed: a -1 on a filament index is a real
value, and a 0 on a raft field is a setting the user chose.

The key is removed rather than rewritten, which is what makes it safe on
every build. The CLI then uses its own default: 0 where 0 was legal
(unchanged), 1 on the older builds, which is what "the active filament"
means under that scheme.

The preview slice never ran the sanitiser at all, so a file that sliced
fine could still fail its automatic plate preview and fall back to the
painted-face heuristic. It matters more there than in a real slice: the
preview runs on the file's own embedded settings, so there is no
--load-settings pass that could supply a replacement for a field the
range validator has already rejected. That also explains the reported
"same error on a later attempt of an unchanged file" without any second
copy of the keys -- the validator that emits it reads the merged global
config, which per-object model_settings.config overrides never reach.

The sanitiser moves to utils/threemf_tools so the service can use it
without importing a route module, and both preview callers pick it up
from one place. Drops _strip_3mf_embedded_settings and its constant,
which have had no callers since the strip-everything experiment was
reverted.

backend/tests/unit/services/test_slice_preview.py

@@ -495,3 +495,139 @@ class TestPreviewRetriesUnparsableGcode:
         assert len(stub.calls) == 2
         # A failed retry must not be cached — the sidecar may be upgraded.
         assert not slice_preview._preview_cache
+
+
+class _RecordingService:
+    """Succeeds every slice, recording the bytes it was handed."""
+
+    def __init__(self, response_bytes: bytes) -> None:
+        self.response_bytes = response_bytes
+        self.calls: list[bytes] = []
+
+    async def __aenter__(self):
+        return self
+
+    async def __aexit__(self, *exc):
+        return False
+
+    async def slice_without_profiles(self, **kw):
+        self.calls.append(kw["model_bytes"])
+        return SliceResult(
+            content=self.response_bytes,
+            print_time_seconds=0,
+            filament_used_g=0.0,
+            filament_used_mm=0.0,
+        )
+
+
+class TestPreviewSanitisesSentinels:
+    """The preview slices on the file's own embedded settings, so there is no
+    ``--load-settings`` pass to supply a replacement for a field the CLI's
+    range validator has already rejected. Until #3030 this path handed the
+    sidecar raw bytes, so a MakerWorld 3MF carrying Bambu's inherit markers
+    failed before producing any slice_info and the modal fell back to its
+    painted-face heuristic for a file the slicer could have answered exactly.
+    """
+
+    @pytest.mark.asyncio
+    async def test_the_slicer_is_handed_sanitised_bytes(self):
+        original = _make_project_3mf(
+            {
+                "wall_filament": "0",
+                "raft_first_layer_expansion": "-1",
+                "layer_height": "0.2",
+            }
+        )
+        body = _make_sliced_3mf(plate_id=1, filaments=[{"id": "1", "type": "PLA", "color": "#000"}])
+        stub = _RecordingService(body)
+        with patch.object(slice_preview, "SlicerApiService", lambda **kw: stub):
+            result = await get_preview_filaments(
+                kind="library_file",
+                source_id=1,
+                plate_id=1,
+                file_bytes=original,
+                file_name="x.3mf",
+                api_url="http://sidecar",
+            )
+        assert result is not None
+        sent = _settings_of(stub.calls[0])
+        assert "wall_filament" not in sent
+        assert "raft_first_layer_expansion" not in sent
+        # Everything the user actually configured still reaches the slicer.
+        assert sent["layer_height"] == "0.2"
+
+    @pytest.mark.asyncio
+    async def test_a_file_without_sentinels_is_forwarded_untouched(self):
+        original = _make_project_3mf({"layer_height": "0.2", "wall_filament": "2"})
+        body = _make_sliced_3mf(plate_id=1, filaments=[{"id": "1", "type": "PLA", "color": "#000"}])
+        stub = _RecordingService(body)
+        with patch.object(slice_preview, "SlicerApiService", lambda **kw: stub):
+            await get_preview_filaments(
+                kind="library_file",
+                source_id=1,
+                plate_id=1,
+                file_bytes=original,
+                file_name="x.3mf",
+                api_url="http://sidecar",
+            )
+        # Not merely equal: the sanitiser returns the input object when it has
+        # nothing to do, so the common case never pays for a zip rebuild.
+        assert stub.calls[0] is original
+
+    @pytest.mark.asyncio
+    async def test_the_gcode_retry_keeps_the_sanitisation(self):
+        # Both faults at once. The retry derives from the sanitised bytes, so
+        # it must not reintroduce the sentinel while blanking the template --
+        # otherwise the retry trades one CLI rejection for the other.
+        original = _make_project_3mf(
+            {
+                "time_lapse_gcode": "{if timelapse_inline_photo}",
+                "wall_filament": "0",
+                "layer_height": "0.2",
+            }
+        )
+        body = _make_sliced_3mf(plate_id=1, filaments=[{"id": "1", "type": "PLA", "color": "#000"}])
+        stub = _FailThenSucceedService(SlicerApiServerError(_TIMELAPSE_PARSE_ERROR), body)
+        with patch.object(slice_preview, "SlicerApiService", lambda **kw: stub):
+            result = await get_preview_filaments(
+                kind="library_file",
+                source_id=1,
+                plate_id=1,
+                file_bytes=original,
+                file_name="x.3mf",
+                api_url="http://sidecar",
+            )
+        assert result is not None
+        assert len(stub.calls) == 2
+        for sent in stub.calls:
+            assert "wall_filament" not in _settings_of(sent)
+        assert _settings_of(stub.calls[1])["time_lapse_gcode"] == ""
+        assert _settings_of(stub.calls[1])["layer_height"] == "0.2"
+
+    @pytest.mark.asyncio
+    async def test_the_cache_key_still_follows_the_source_file(self):
+        # Sanitising is deterministic, so the key stays the hash of what the
+        # caller read off disk -- a second open of the same file is a hit, not
+        # a second 30-second slice.
+        original = _make_project_3mf({"wall_filament": "0"})
+        body = _make_sliced_3mf(plate_id=1, filaments=[{"id": "1", "type": "PLA", "color": "#000"}])
+        stub = _RecordingService(body)
+        with patch.object(slice_preview, "SlicerApiService", lambda **kw: stub):
+            first = await get_preview_filaments(
+                kind="library_file",
+                source_id=1,
+                plate_id=1,
+                file_bytes=original,
+                file_name="x.3mf",
+                api_url="http://sidecar",
+            )
+            second = await get_preview_filaments(
+                kind="library_file",
+                source_id=1,
+                plate_id=1,
+                file_bytes=original,
+                file_name="x.3mf",
+                api_url="http://sidecar",
+            )
+        assert second == first
+        assert len(stub.calls) == 1

backend/tests/unit/test_project_settings_sentinel_sanitiser.py

@@ -1,4 +1,4 @@
-"""Unit tests for ``_sanitize_project_settings_sentinels`` (#1201).
+"""Unit tests for ``sanitize_project_settings_sentinels`` (#1201, #3030).
 
 MakerWorld 3MFs sliced for the P2S (and potentially other Bambu printers)
 ship ``Metadata/project_settings.config`` entries with ``"-1"`` values on
@@ -22,6 +22,21 @@ re-zip. The slicer then falls back to the supplied ``--load-settings``
 default for the removed keys, while every other entry in the zip stays
 byte-identical.
 
+#3030 added a second sentinel value. ``wall_filament``,
+``sparse_infill_filament`` and ``solid_infill_filament`` are filament indices
+that Bambu Studio writes as ``"0"`` meaning "use the active object/part
+filament". Bambu Studio and OrcaSlicer 2.4.0+ define these ``min 0``, so the
+value is legal there; OrcaSlicer 2.3.x and earlier used the 1-based scheme
+(``min 1``, default ``1``) and answer::
+
+    wall_filament: 0 not in range [1.000000,...]
+
+Sidecar images are version-tagged, so an install can be pinned to one of
+those builds. So the allowlist is a key -> sentinel mapping now rather than
+one global constant, and the two buckets must not bleed into each other: a
+``"-1"`` on a filament index is not a sentinel, and a ``"0"`` on a raft field
+is a value the user chose.
+
 Pinning the contract here rather than via the slicer integration tests
 because the fix is purely about the bytes we hand to the sidecar — no
 slicer mock needed.
@@ -33,9 +48,9 @@ import zipfile
 
 import pytest
 
-from backend.app.api.routes.library import (
-    _PROJECT_SETTINGS_SENTINEL_KEYS,
-    _sanitize_project_settings_sentinels,
+from backend.app.utils.threemf_tools import (
+    PROJECT_SETTINGS_SENTINELS,
+    sanitize_project_settings_sentinels,
 )
 
 
@@ -65,10 +80,10 @@ def _zip_namelist(zip_bytes: bytes) -> list[str]:
 
 
 class TestRemovesSentinelValues:
-    @pytest.mark.parametrize("key", sorted(_PROJECT_SETTINGS_SENTINEL_KEYS))
-    def test_removes_each_allowlisted_key_when_value_is_minus_one(self, key):
-        original = _make_3mf(settings={key: "-1", "layer_height": "0.2"})
-        sanitised = _sanitize_project_settings_sentinels(original)
+    @pytest.mark.parametrize(("key", "sentinel"), sorted(PROJECT_SETTINGS_SENTINELS.items()))
+    def test_removes_each_allowlisted_key_at_its_own_sentinel(self, key, sentinel):
+        original = _make_3mf(settings={key: sentinel, "layer_height": "0.2"})
+        sanitised = sanitize_project_settings_sentinels(original)
 
         cfg = _read_settings(sanitised)
         assert key not in cfg, f"Sentinel key {key!r} should have been removed"
@@ -85,7 +100,7 @@ class TestRemovesSentinelValues:
                 "layer_height": "0.2",
             }
         )
-        sanitised = _sanitize_project_settings_sentinels(original)
+        sanitised = sanitize_project_settings_sentinels(original)
 
         cfg = _read_settings(sanitised)
         assert "raft_first_layer_expansion" not in cfg
@@ -99,7 +114,7 @@ class TestPreservesUnaffectedValues:
         # A user who deliberately configured raft_first_layer_expansion=0 must
         # see that 0 forwarded to the slicer — only literal "-1" gets stripped.
         original = _make_3mf(settings={"raft_first_layer_expansion": "0"})
-        sanitised = _sanitize_project_settings_sentinels(original)
+        sanitised = sanitize_project_settings_sentinels(original)
         assert _read_settings(sanitised)["raft_first_layer_expansion"] == "0"
 
     def test_does_not_touch_non_allowlisted_keys_with_minus_one(self):
@@ -107,7 +122,7 @@ class TestPreservesUnaffectedValues:
         # Some Bambu fields legitimately allow negative values (z_offset,
         # translation, etc.) and a blanket "-1" strip would corrupt those.
         original = _make_3mf(settings={"z_offset": "-1", "layer_height": "0.2"})
-        sanitised = _sanitize_project_settings_sentinels(original)
+        sanitised = sanitize_project_settings_sentinels(original)
 
         cfg = _read_settings(sanitised)
         assert cfg["z_offset"] == "-1"
@@ -117,7 +132,7 @@ class TestPreservesUnaffectedValues:
         # If nothing needs sanitising, return the input identity-equal so
         # the caller's downstream comparisons / hashes don't churn.
         original = _make_3mf(settings={"layer_height": "0.2", "z_offset": "0"})
-        sanitised = _sanitize_project_settings_sentinels(original)
+        sanitised = sanitize_project_settings_sentinels(original)
         assert sanitised is original
 
     def test_does_not_strip_array_value_even_if_includes_minus_one(self):
@@ -128,7 +143,7 @@ class TestPreservesUnaffectedValues:
         # sentinel and removed wholesale. If a future report shows the CLI
         # rejects array-form sentinels, expand this then.
         original = _make_3mf(settings={"raft_first_layer_expansion": ["-1", "0"]})
-        sanitised = _sanitize_project_settings_sentinels(original)
+        sanitised = sanitize_project_settings_sentinels(original)
         cfg = _read_settings(sanitised)
         assert cfg["raft_first_layer_expansion"] == ["-1", "0"]
 
@@ -143,7 +158,7 @@ class TestZipPreservation:
                 "Metadata/_rels/model_settings.rels": "<rels/>",
             },
         )
-        sanitised = _sanitize_project_settings_sentinels(original)
+        sanitised = sanitize_project_settings_sentinels(original)
         assert sanitised is not original
 
         names = _zip_namelist(sanitised)
@@ -171,12 +186,12 @@ class TestDefensiveFallbacks:
         # routing decides whether 3MF sanitisation runs anyway, but
         # defending here means a misrouted call can't corrupt the bytes.
         garbage = b"not a zip file"
-        assert _sanitize_project_settings_sentinels(garbage) is garbage
+        assert sanitize_project_settings_sentinels(garbage) is garbage
 
     def test_returns_original_when_settings_config_absent(self):
         # 3MF without an embedded project_settings.config — nothing to do.
         original = _make_3mf(settings=None)
-        assert _sanitize_project_settings_sentinels(original) is original
+        assert sanitize_project_settings_sentinels(original) is original
 
     def test_returns_original_on_malformed_json(self):
         # Settings file present but not valid JSON. We don't risk rebuilding
@@ -187,7 +202,7 @@ class TestDefensiveFallbacks:
             zf.writestr("3D/3dmodel.model", "<model/>")
             zf.writestr("Metadata/project_settings.config", "{not valid json")
         original = buf.getvalue()
-        assert _sanitize_project_settings_sentinels(original) is original
+        assert sanitize_project_settings_sentinels(original) is original
 
     def test_returns_original_when_settings_root_is_not_a_dict(self):
         # Real-world configs are objects, but defend against an array root
@@ -198,4 +213,69 @@ class TestDefensiveFallbacks:
             zf.writestr("3D/3dmodel.model", "<model/>")
             zf.writestr("Metadata/project_settings.config", "[]")
         original = buf.getvalue()
-        assert _sanitize_project_settings_sentinels(original) is original
+        assert sanitize_project_settings_sentinels(original) is original
+
+
+class TestTheTwoSentinelBucketsDoNotBleed:
+    """Each key has exactly one sentinel, and the other bucket's value is a
+    real setting on it. Getting this wrong in either direction is worse than
+    the bug: strip a filament index that says ``-1`` and the slice loses a
+    value nothing will put back; strip a raft field that says ``0`` and a
+    user who deliberately turned the raft expansion off gets the preset's
+    default instead."""
+
+    @pytest.mark.parametrize("key", ["wall_filament", "sparse_infill_filament", "solid_infill_filament"])
+    def test_a_filament_index_of_minus_one_is_left_alone(self, key):
+        original = _make_3mf(settings={key: "-1"})
+        assert sanitize_project_settings_sentinels(original) is original
+
+    @pytest.mark.parametrize("key", ["raft_first_layer_expansion", "tree_support_wall_count", "prime_tower_brim_width"])
+    def test_a_zero_on_a_minus_one_key_is_left_alone(self, key):
+        original = _make_3mf(settings={key: "0"})
+        assert sanitize_project_settings_sentinels(original) is original
+
+    def test_a_mixed_config_removes_exactly_the_sentinels(self):
+        original = _make_3mf(
+            settings={
+                # sentinels, both buckets
+                "raft_first_layer_expansion": "-1",
+                "wall_filament": "0",
+                # same keys' non-sentinel values, crossed over
+                "tree_support_wall_count": "0",
+                "sparse_infill_filament": "-1",
+                # an explicit filament pick
+                "solid_infill_filament": "2",
+                "layer_height": "0.2",
+            }
+        )
+        cfg = _read_settings(sanitize_project_settings_sentinels(original))
+        assert "raft_first_layer_expansion" not in cfg
+        assert "wall_filament" not in cfg
+        assert cfg["tree_support_wall_count"] == "0"
+        assert cfg["sparse_infill_filament"] == "-1"
+        assert cfg["solid_infill_filament"] == "2"
+        assert cfg["layer_height"] == "0.2"
+
+
+class TestNumericValuesAreRecognised:
+    """Bambu Studio writes every value as a string, but a 3MF round-tripped
+    through another tool can carry the same field as a JSON number. The
+    slicer's validator reads the deserialised int either way, so the
+    sanitiser has to as well."""
+
+    def test_an_integer_sentinel_is_removed(self):
+        original = _make_3mf(settings={"wall_filament": 0, "layer_height": "0.2"})
+        cfg = _read_settings(sanitize_project_settings_sentinels(original))
+        assert "wall_filament" not in cfg
+        assert cfg["layer_height"] == "0.2"
+
+    def test_an_integer_non_sentinel_survives(self):
+        original = _make_3mf(settings={"wall_filament": 2})
+        assert sanitize_project_settings_sentinels(original) is original
+
+    def test_false_is_not_a_zero_sentinel(self):
+        # bool is an int subclass in Python. A config that stores a flag as
+        # JSON ``false`` under one of these names must not be mistaken for
+        # the numeric 0 sentinel.
+        original = _make_3mf(settings={"wall_filament": False})
+        assert sanitize_project_settings_sentinels(original) is original