From a851d3587c3c0df3d6342c38897070a4a9d7b6a9 Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Fri, 12 Jun 2026 02:36:09 -0700 Subject: [PATCH] =?UTF-8?q?fix(products):=20conservative=20dialect=20detec?= =?UTF-8?q?tion=20=E2=80=94=20canonical-distinctive=20columns=20veto=20Sho?= =?UTF-8?q?pify=20(SLICE-8=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses adversarial-review findings: a canonical file carrying a stray Shopify-signature name (e.g. SEO Title) no longer misdetects as Shopify and silently drops canonical Type / corrupts the weight unit (kg->g). A canonical-distinctive column (Description/Variant Cost/Variant Weight/Google Product Category/Variant Volume/Tax ID/Position) now vetoes detection, so detection leans conservative (under-detection warns; over-detection corrupts). Also closes the dual-named-column shadow (Body (HTML)+Description) and the stale-weight-unit-on-clear case. Tests cover all three. Co-Authored-By: Claude Fable 5 --- backend/app/domains/products/codec.py | 10 +++-- .../app/domains/products/dialect_shopify.py | 30 +++++++++++-- .../tests/test_products_dialect_shopify.py | 43 +++++++++++++++++++ docs/products-domain.md | 15 ++++--- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/backend/app/domains/products/codec.py b/backend/app/domains/products/codec.py index 7cc7a9a..41b476e 100644 --- a/backend/app/domains/products/codec.py +++ b/backend/app/domains/products/codec.py @@ -73,9 +73,13 @@ def parse_csv(data: bytes) -> ParsedFile: c: (raw[col_index[c]].strip() if col_index[c] < len(raw) else "") for c in col_index } - # Shopify grams carry an implicit unit; canonical needs it explicit (§6.5.1). - if dialect == "shopify" and cells.get("Variant Weight"): - cells["Variant Weight Unit"] = "g" + # Shopify grams carry an implicit unit; canonical needs it explicit. In a + # Shopify file "Variant Weight" can only come from the Variant Grams rename + # (a canonical-named Variant Weight would have vetoed Shopify detection), so + # weight and unit move together: a value -> "g"; a cleared grams (present-but- + # empty) clears the unit too, never leaving a stale unit (§6.5.1). + if dialect == "shopify" and "Variant Weight" in cells: + cells["Variant Weight Unit"] = "g" if cells["Variant Weight"] else "" rows.append(Row(line_number=reader.line_num, cells=cells)) except csv.Error: raise FileRejected("not_csv", "This file isn't readable as CSV.") from None diff --git a/backend/app/domains/products/dialect_shopify.py b/backend/app/domains/products/dialect_shopify.py index 1ad8b68..baa48c7 100644 --- a/backend/app/domains/products/dialect_shopify.py +++ b/backend/app/domains/products/dialect_shopify.py @@ -38,12 +38,36 @@ _SIGNATURE: frozenset[str] = frozenset( } ) +# Canonical-distinctive columns — names Shopify renames away (so a real Shopify export +# never carries them) plus canonical-only columns Shopify has no equivalent for. Their +# presence proves the file is canonical and VETOES Shopify detection: detection must +# lean conservative, because under-detection is safe (Shopify-named columns are warned +# as not-imported) while over-detection corrupts (canonical Type / weight-unit are +# dropped). This closes the misdetection hazard — a canonical file that merely contains +# a stray signature-named column (e.g. `SEO Title`) is never misread as Shopify (§7.4). +_CANONICAL_SIGNATURE: frozenset[str] = frozenset( + { + "Description", + "Variant Cost", + "Variant Weight", + "Google Product Category", + "Variant Volume", + "Variant Volume Unit", + "Variant Tax ID 1", + "Variant Tax ID 2", + "Variant Position", + } +) + def is_shopify_header(header: list[str]) -> bool: - """True iff the header carries a Shopify-only signature column. Otherwise the - file is treated as canonical — shared columns map identically, so an ambiguous - header never misparses (§7.4).""" + """True iff the header carries a Shopify-only signature column AND no + canonical-distinctive column. A canonical signal vetoes Shopify detection so an + ambiguous or hybrid header is treated as canonical — where Shopify-named columns + are warned as not-imported rather than silently remapped (never misparses, §7.4).""" cols = {h.strip() for h in header} + if cols & _CANONICAL_SIGNATURE: + return False if cols & _SIGNATURE: return True return any(c.startswith("Google Shopping / ") for c in cols) diff --git a/backend/tests/test_products_dialect_shopify.py b/backend/tests/test_products_dialect_shopify.py index d888672..3c4e570 100644 --- a/backend/tests/test_products_dialect_shopify.py +++ b/backend/tests/test_products_dialect_shopify.py @@ -76,3 +76,46 @@ def test_shopify_fixture_maps_exhaustively(): ): assert col in parsed.unknown_columns, col assert "Variant Compare At Price" not in first + + +# --- Detection robustness (review findings #1, #2): bias conservative ---------------- + + +def test_canonical_distinctive_column_vetoes_shopify_detection(): + # A canonical file carrying a stray Shopify-signature name (`SEO Title`) stays + # canonical because it also has canonical-distinctive columns — no misparse, + # no dropped Type, no corrupted weight unit (review finding #2). + header = ["Handle", "Title", "Type", "Variant Weight", "Variant Weight Unit", "SEO Title"] + assert is_shopify_header(header) is False + + +def test_dual_named_file_stays_canonical_and_warns_shopify_name(): + # A malformed file with BOTH `Body (HTML)` and canonical `Description`: the + # canonical column vetoes detection, so `Body (HTML)` is honestly warned as an + # unknown column rather than silently shadowing `Description` (review finding #1). + data = ( + b"Handle,Title,Body (HTML),Description,Variant Price\n" + b"mug,Moon Mug,FROM_BODY,FROM_DESC,18.00\n" + ) + parsed = parse_csv(data) + assert parsed.dialect == "canonical" + assert parsed.rows[0].cells["Description"] == "FROM_DESC" + assert "Body (HTML)" in parsed.unknown_columns + + +def test_real_shopify_export_still_detected(): + # The conservative veto must not break a genuine Shopify export (no canonical- + # distinctive columns present). + parsed = parse_csv(_FIXTURE.read_bytes()) + assert parsed.dialect == "shopify" + + +def test_shopify_grams_clear_clears_weight_unit_together(): + # An empty Variant Grams (a clear) clears the synthesized unit too — weight and + # unit move together, never a stale unit (review finding #3). + data = b"Handle,Title,Body (HTML),Variant Grams\nmug,Moon Mug,

x

,\n" + parsed = parse_csv(data) + assert parsed.dialect == "shopify" + cells = parsed.rows[0].cells + assert cells["Variant Weight"] == "" + assert cells["Variant Weight Unit"] == "" diff --git a/docs/products-domain.md b/docs/products-domain.md index 65a6d05..3f55409 100644 --- a/docs/products-domain.md +++ b/docs/products-domain.md @@ -72,11 +72,16 @@ mapping (mirrored by `tests/test_products_dialect_shopify.py` and - **Detection** — `is_shopify_header()` returns `shopify` iff the header carries a Shopify-only *signature* column (`Body (HTML)`, `Variant Grams`, `Cost per item`, - `Gift Card`, a `Google Shopping / …` column, …). Everything else is `canonical`. - Detection is by signature, not an exact header set, so extra market/region columns - don't defeat it; an ambiguous header (only columns common to both dialects) falls - through to `canonical`, which maps those shared columns identically — so it never - misparses (§7.4). + `Gift Card`, a `Google Shopping / …` column, …) **and no canonical-distinctive + column**. A canonical-distinctive name — one Shopify renames away (`Description`, + `Variant Cost`, `Variant Weight`, `Google Product Category`) or a canonical-only + column Shopify lacks (`Variant Volume`, `Variant Tax ID 1/2`, `Variant Position`) — + **vetoes** Shopify detection. Detection is by signature, not an exact header set, so + extra market/region columns don't defeat it; the veto biases it conservative, because + under-detection is safe (Shopify-named columns are warned as not-imported) while + over-detection corrupts (canonical `Type` / `Variant Weight Unit` would be dropped). + So a hybrid or ambiguous header falls through to `canonical`, which maps shared + columns identically and warns the rest — it never misparses (§7.4). - **Mapping** — `map_shopify_header(header)` returns `(mapped, not_imported)`: `mapped[i]` is the canonical name for column `i` (or `None` when it has no canonical home), and `not_imported` is the warning list surfaced as the draft's