diff --git a/backend/app/api/routes/library.py b/backend/app/api/routes/library.py index a690d5987..adb8ec2f8 100644 --- a/backend/app/api/routes/library.py +++ b/backend/app/api/routes/library.py @@ -94,6 +94,7 @@ from backend.app.utils.threemf_tools import ( extract_embedded_presets_from_3mf, extract_nozzle_mapping_from_3mf, extract_project_filaments_from_3mf, + sanitize_project_settings_sentinels, select_plate_gcode_name, supports_enabled_in_config, ) @@ -3492,142 +3493,6 @@ async def get_library_file_filament_requirements( } -_STRIPPABLE_3MF_CONFIGS = frozenset( - { - # Settings dump used by --load-settings validation; the CLI tries to - # match its sentinel values (`prime_tower_brim_width: -1`, empty - # arrays) against the supplied profile and rejects out-of-range. - "Metadata/project_settings.config", - # Per-object settings overrides referencing the source plate's - # filament IDs / printer IDs. When the user picks a different - # printer / filament triplet, the IDs no longer resolve and the - # CLI exits non-zero on input validation. - "Metadata/model_settings.config", - # Slicer-version + plate-config + filament-mapping snapshot from - # the original slice. Includes the original printer model and - # filament references; mismatches against `--load-settings` - # consistently surfaced as `Slicer CLI failed (500)` for every - # 3MF in production. Removing it lets the CLI build a fresh slice - # plan from the supplied profile triplet. - "Metadata/slice_info.config", - # Multi-part / split-mesh metadata referencing object IDs from the - # original slice. Strip for the same reason — preserves the geometry - # in `3D/3dmodel.model` while dropping the orphan references. - "Metadata/cut_information.xml", - } -) - - -def _strip_3mf_embedded_settings(zip_bytes: bytes) -> bytes: - """Remove embedded slicer-config metadata from a 3MF. - - Bambuddy supplies the slicer profile triplet via the sidecar's - ``--load-settings`` path; the 3MF's embedded settings would otherwise be - validated by the CLI first and can fail with sentinel-value range - checks (`prime_tower_brim_width: -1 not in range`, etc.) regardless of - what we pass via ``--load-settings``. Stripping the embedded configs - forces the CLI to use the supplied profiles only. Geometry - (``3D/3dmodel.model``), thumbnails, color, and multi-part data inside - the 3MF are preserved. - - The set of strippable filenames is centralised in - ``_STRIPPABLE_3MF_CONFIGS`` — see that constant for the per-file - rationale. Project-settings alone wasn't enough: real-world Bambu - Studio 3MFs cross-reference printer / filament IDs from the other - metadata configs, and any single leftover triggered the validation - failure that made every profile-driven slice fall back to embedded - settings. - """ - from io import BytesIO - - src = BytesIO(zip_bytes) - dst = BytesIO() - with zipfile.ZipFile(src, "r") as zin, zipfile.ZipFile(dst, "w", zipfile.ZIP_DEFLATED) as zout: - for item in zin.infolist(): - if item.filename in _STRIPPABLE_3MF_CONFIGS: - continue - zout.writestr(item, zin.read(item.filename)) - return dst.getvalue() - - -# Keys in ``Metadata/project_settings.config`` that BambuStudio writes ``"-1"`` -# to when the user wants the value inherited from the parent process preset. -# The CLI's ``StaticPrintConfig`` validator runs against the embedded settings -# *before* ``--load-settings`` overrides apply, so a sentinel ``"-1"`` trips -# the field's lower-bound range check and the CLI exits non-zero before our -# profile triplet is ever consulted (#1201 — MakerWorld P2S models). -# -# Allowlisted (rather than "strip every '-1' value") because some fields -# legitimately accept negative numbers (z_offset, translation values, etc.) -# and a blanket strip would silently corrupt those. -# -# Add new entries here as more reports surface — the slicer's error message -# names the offending field directly (`: -1 not in range [...]`). -_PROJECT_SETTINGS_SENTINEL_KEYS = frozenset( - { - # Reported in #1201 (MakerWorld P2S 3MFs). - "raft_first_layer_expansion", - "tree_support_wall_count", - # Cited in the strip-experiment comment block above as a known sentinel - # case from earlier reports. - "prime_tower_brim_width", - } -) - - -def _sanitize_project_settings_sentinels(zip_bytes: bytes) -> bytes: - """Strip ``"-1"`` inherit-from-parent sentinels from the 3MF's - ``Metadata/project_settings.config`` so the slicer CLI's range validator - accepts the file (#1201). - - Removes only allowlisted keys (see ``_PROJECT_SETTINGS_SENTINEL_KEYS``) - when their value is exactly ``"-1"``. The rest of the config — and every - other entry in the zip — is preserved byte-for-byte. Unlike the earlier - full-strip experiment (see ``_strip_3mf_embedded_settings`` and the - cautionary comment in ``_run_slicer_with_fallback``) this leaves - ``StaticPrintConfig`` initialisation intact: the file is still present, - still parses, and the slicer falls back to the supplied - ``--load-settings`` value for the removed key. - - Returns the original bytes unchanged when no sanitisation is needed - (input isn't a valid zip, no ``project_settings.config``, no allowlisted - sentinels present, or any other parse failure) so the caller can pass - the result on without further checks. - """ - from io import BytesIO - - try: - with zipfile.ZipFile(BytesIO(zip_bytes), "r") as zin: - if "Metadata/project_settings.config" not in zin.namelist(): - return zip_bytes - try: - config = json.loads(zin.read("Metadata/project_settings.config").decode("utf-8")) - except (json.JSONDecodeError, UnicodeDecodeError): - return zip_bytes - if not isinstance(config, dict): - return zip_bytes - removed = [key for key in _PROJECT_SETTINGS_SENTINEL_KEYS if config.get(key) == "-1"] - if not removed: - return zip_bytes - for key in removed: - config.pop(key, None) - patched = json.dumps(config) - logger.info( - "3MF sanitiser: removed sentinel '-1' for keys %s — slicer will use --load-settings defaults", - sorted(removed), - ) - dst = BytesIO() - with zipfile.ZipFile(dst, "w", zipfile.ZIP_DEFLATED) as zout: - for item in zin.infolist(): - if item.filename == "Metadata/project_settings.config": - zout.writestr(item, patched) - else: - zout.writestr(item, zin.read(item.filename)) - return dst.getvalue() - except (zipfile.BadZipFile, OSError): - return zip_bytes - - def _patch_process_bed_type(process_json: str, bed_type: str) -> str: """Overwrite ``curr_bed_type`` in a process-profile JSON before forwarding to the slicer sidecar. @@ -4022,13 +3887,14 @@ async def _run_slicer_with_fallback( is_3mf = model_filename.lower().endswith(".3mf") primary_bytes = model_bytes if is_3mf: - # Strip "-1" inherit-from-parent sentinels from - # Metadata/project_settings.config so the CLI's StaticPrintConfig - # range validator accepts the file (#1201). Surgical — keeps the - # config present, just removes the offending keys; the supplied - # --load-settings (and the fallback's embedded values for keys we - # didn't touch) still drive the slice. - primary_bytes = _sanitize_project_settings_sentinels(primary_bytes) + # Strip inherit/unset sentinels from Metadata/project_settings.config + # so the CLI's StaticPrintConfig range validator accepts the file + # (#1201, #3030). Surgical — keeps the config present, just removes + # the offending keys; the supplied --load-settings (and the fallback's + # embedded values for keys we didn't touch) still drive the slice. + # The preview-slice path applies the same sanitiser in + # ``slice_preview.get_preview_filaments``. + primary_bytes = sanitize_project_settings_sentinels(primary_bytes) # #2622: the process settings the file's designer moved off the stock # preset. Read once — the support patch below needs to know which of diff --git a/backend/app/services/slice_preview.py b/backend/app/services/slice_preview.py index 6d6d4e059..34308887e 100644 --- a/backend/app/services/slice_preview.py +++ b/backend/app/services/slice_preview.py @@ -14,10 +14,13 @@ we don't need to thread the user's profile triplet through here. That choice also protects the numbers — overriding the process preset drops the project's own support configuration, which loses whole slots from the answer. -The one thing that can defeat those embedded settings is a custom G-code -template written by a Studio newer than the sidecar, which fails to parse -before any slice_info exists. That case gets one retry with the offending -template blanked; see ``_blank_custom_gcode``. +Two things can defeat those embedded settings. A custom G-code template +written by a Studio newer than the sidecar fails to parse before any +slice_info exists; that case gets one retry with the offending template +blanked, see ``_blank_custom_gcode``. And Bambu Studio writes inherit/unset +markers into ``project_settings.config`` that some slicer builds' range +validator rejects outright, so the same sanitiser the real slice runs is +applied here too, see ``sanitize_project_settings_sentinels``. Results are cached by ``(kind, source_id, plate_id, content_hash)`` so repeat opens on the same plate are instant. LRU eviction keeps the cache @@ -42,6 +45,7 @@ from backend.app.services.slicer_api import ( SlicerApiError, SlicerApiService, ) +from backend.app.utils.threemf_tools import sanitize_project_settings_sentinels logger = logging.getLogger(__name__) @@ -203,13 +207,17 @@ async def get_preview_filaments( Uses the file's embedded settings (``slice_without_profiles``) since the slot mapping is a model property, independent of any user-picked profile - triplet. A slice killed by an unparsable custom G-code template is retried - once with that template blanked, still on the file's own settings. + triplet. Those settings are sentinel-sanitised first (#1201, #3030). A + slice killed by an unparsable custom G-code template is retried once with + that template blanked, still on the file's own settings. Returns ``None`` when the preview slice fails — the caller should fall back to whatever heuristic it has (typically the project_filaments + painted-face approach in ``threemf_tools``). """ + # Hash the file as it was given to us, not as it is sent: the key + # identifies the source file, and sanitising is deterministic, so folding + # it in would only make two names for one thing. h = _content_hash(file_bytes) key: _PreviewCacheKey = (kind, source_id, plate_id, h) cached = _preview_cache.get(key) @@ -231,6 +239,17 @@ async def get_preview_filaments( # while the slicer is visibly working. svc_kwargs = {} if timeout_seconds is None else {"timeout_seconds": timeout_seconds} + # Same sanitiser the real slice runs (#1201, #3030). It matters more + # here, not less: this path slices on the file's own embedded + # settings, so there is no --load-settings pass that could supply a + # replacement for a field the CLI's range validator has already + # rejected. Without it a MakerWorld 3MF carrying Bambu's inherit + # markers fails before producing any slice_info, and the modal falls + # back to its painted-face heuristic for a file the slicer could have + # answered exactly. Applied before the G-code retry below so that + # retry inherits it rather than reintroducing the markers. + slice_bytes = sanitize_project_settings_sentinels(file_bytes) + async def _slice(model_bytes: bytes): async with SlicerApiService(base_url=api_url, **svc_kwargs) as svc: return await svc.slice_without_profiles( @@ -242,7 +261,7 @@ async def get_preview_filaments( ) try: - result = await _slice(file_bytes) + result = await _slice(slice_bytes) except SlicerApiError as e: # One retry, and only for a custom-G-code template the sidecar # cannot parse — a file from a Studio newer than the sidecar. The @@ -259,7 +278,7 @@ async def get_preview_filaments( retry_bytes = None option = _unparsable_gcode_option(str(e)) if option is not None: - retry_bytes = _blank_custom_gcode(file_bytes, option) + retry_bytes = _blank_custom_gcode(slice_bytes, option) if retry_bytes is None: logger.warning( "Preview slice failed for %s/%s plate %s: %s", diff --git a/backend/app/utils/threemf_tools.py b/backend/app/utils/threemf_tools.py index 9a8215a2c..f2c1fe12b 100644 --- a/backend/app/utils/threemf_tools.py +++ b/backend/app/utils/threemf_tools.py @@ -1767,3 +1767,125 @@ def extract_plate_extruder_set_from_3mf(zf: zipfile.ZipFile, plate_id: int) -> s used.update(_scan_paint(path)) break return used + + +# Keys in ``Metadata/project_settings.config`` that Bambu Studio writes an +# "inherit / unset" marker into, mapped to the marker it uses for that key. +# The slicer CLI's ``StaticPrintConfig`` validator runs against the embedded +# settings *before* ``--load-settings`` overrides apply, so a marker the CLI's +# own range check rejects makes it exit non-zero before our profile triplet is +# ever consulted. +# +# There are two markers because there are two conventions, and which one a +# given CLI rejects depends on the build: +# +# "-1" -- inherit from the parent process preset (#1201, MakerWorld P2S +# 3MFs). ``raft_first_layer_expansion`` and ``tree_support_wall_count`` are +# min 0 in every OrcaSlicer to date, so those still fail on the current +# sidecar; ``prime_tower_brim_width`` gained min -1 in Orca 2.4.2 and now +# passes there, but not on older builds. +# +# "0" -- "use the active object/part filament", the default Bambu Studio +# writes for the three feature-filament indices (#3030). Bambu Studio and +# OrcaSlicer 2.4.0+ both define these min 0, so 0 is legal there; OrcaSlicer +# 2.3.x and earlier still 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. +# +# Removing the key rather than rewriting it is what makes this safe on every +# build: the CLI then falls back to its own compiled default, which is 0 on +# the builds where 0 was legal (so nothing changes) and 1 on the older ones, +# which is what "the active filament" means under that scheme. +# +# Allowlisted (rather than "strip every marker-shaped value") because some +# fields legitimately take the marker value -- z_offset, translations, and any +# feature index a user really did set to a first filament -- and a blanket +# strip would silently corrupt those. +# +# Add new entries as reports surface: the slicer names the offending field +# directly, e.g. ``: not in range [...]``. +PROJECT_SETTINGS_SENTINELS: dict[str, str] = { + # Reported in #1201 (MakerWorld P2S 3MFs). + "raft_first_layer_expansion": "-1", + "tree_support_wall_count": "-1", + # Known sentinel case from earlier reports, cited in #1201. + "prime_tower_brim_width": "-1", + # Reported in #3030 (MakerWorld 3MF, OrcaSlicer sidecar). + "wall_filament": "0", + "sparse_infill_filament": "0", + "solid_infill_filament": "0", +} + +PROJECT_SETTINGS_PATH = "Metadata/project_settings.config" + + +def _is_sentinel(value: object, sentinel: str) -> bool: + """Does ``value`` carry ``sentinel``, whether stored as text or a number? + + Bambu Studio writes every ``project_settings.config`` value as a string, + but a 3MF that has been round-tripped through another tool can carry the + same field as a JSON number. ``bool`` is excluded explicitly: it is an + ``int`` subclass in Python, and ``str(False)`` would otherwise never match + anyway -- the exclusion is there so a future numeric sentinel like ``0`` + cannot be matched by ``False``. + """ + if isinstance(value, bool): + return False + if isinstance(value, (str, int)): + return str(value) == sentinel + return False + + +def sanitize_project_settings_sentinels(zip_bytes: bytes) -> bytes: + """Strip inherit/unset sentinels from a 3MF's ``project_settings.config`` + so the slicer CLI's range validator accepts the file (#1201, #3030). + + Removes only allowlisted keys (see ``PROJECT_SETTINGS_SENTINELS``) and only + when the value is exactly that key's sentinel. The rest of the config -- + and every other entry in the zip -- is preserved byte-for-byte. Unlike a + whole-file strip this leaves ``StaticPrintConfig`` initialisation intact: + the file is still present, still parses, and the slicer falls back to the + supplied ``--load-settings`` value, or to its own default, for the removed + key. + + Returns the original bytes unchanged when no sanitisation is needed (input + isn't a valid zip, no ``project_settings.config``, no allowlisted sentinels + present, or any other parse failure) so the caller can pass the result on + without further checks. + """ + from io import BytesIO + + try: + with zipfile.ZipFile(BytesIO(zip_bytes), "r") as zin: + if PROJECT_SETTINGS_PATH not in zin.namelist(): + return zip_bytes + try: + config = json.loads(zin.read(PROJECT_SETTINGS_PATH).decode("utf-8")) + except (json.JSONDecodeError, UnicodeDecodeError): + return zip_bytes + if not isinstance(config, dict): + return zip_bytes + removed = { + key: sentinel + for key, sentinel in PROJECT_SETTINGS_SENTINELS.items() + if _is_sentinel(config.get(key), sentinel) + } + if not removed: + return zip_bytes + for key in removed: + config.pop(key, None) + patched = json.dumps(config) + logger.info( + "3MF sanitiser: removed inherit sentinels %s - slicer will use its defaults for those keys", + sorted(f"{key}={sentinel}" for key, sentinel in removed.items()), + ) + dst = BytesIO() + with zipfile.ZipFile(dst, "w", zipfile.ZIP_DEFLATED) as zout: + for item in zin.infolist(): + if item.filename == PROJECT_SETTINGS_PATH: + zout.writestr(item, patched) + else: + zout.writestr(item, zin.read(item.filename)) + return dst.getvalue() + except (zipfile.BadZipFile, OSError): + return zip_bytes diff --git a/backend/tests/unit/services/test_slice_preview.py b/backend/tests/unit/services/test_slice_preview.py index 029e07f96..d35e16e2d 100644 --- a/backend/tests/unit/services/test_slice_preview.py +++ b/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 diff --git a/backend/tests/unit/test_project_settings_sentinel_sanitiser.py b/backend/tests/unit/test_project_settings_sentinel_sanitiser.py index 74ba9d0bc..363a3c031 100644 --- a/backend/tests/unit/test_project_settings_sentinel_sanitiser.py +++ b/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": "", }, ) - 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", "") 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", "") 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