feat(variant-axis): registry + resolvers + back-compat synthesis (B.1)#572
Open
Blb3D wants to merge 16 commits into
Open
feat(variant-axis): registry + resolvers + back-compat synthesis (B.1)#572Blb3D wants to merge 16 commits into
Blb3D wants to merge 16 commits into
Conversation
12-task TDD plan for the variant-axis-registry workstream. Each task follows write-failing-test → run → minimal impl → run → commit. Spawned from docs/plans/variant-axis-generalization.md §4. Key choices: - synthesized legacy axis_selections key = "__legacy__" sentinel (we don't know the original RoutingOperationMaterial.id from a flat legacy record; never persisted, only read-side) - swap_material_variant refactor reuses ComponentTemplateResolver list_options for child-of-current membership check; no widening - mrp.py, frontend, alembic, write-path all out of scope (B.2/C.1) Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 706e457c-d7ef-40ae-a1e5-5f5c1c7aac42
Contributor
WalkthroughThis PR introduces a registry-based variant axis resolver architecture. New Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as variant_service
participant Reader as reader
participant Registry as axis_registry
participant Resolver as MaterialColorResolver<br/>ComponentTemplateResolver
participant DB as Database
Client->>Service: sync_routing_to_variants(template)
Service->>Reader: read_axis_selections(variant_metadata)
Reader->>Reader: normalize to v2
Service->>Reader: compute_axis_count(meta_v2)
Reader-->>Service: axis_count
Service->>Reader: enforce_axis_cap(meta_v2)
Reader-->>Service: validated_axis_count
loop per routing_material
Service->>Registry: get(axis_type)
Registry-->>Service: resolver instance
Service->>Resolver: resolve_to_component(db, value)
Resolver->>DB: query Product by value
DB-->>Resolver: active Product or 404
Resolver-->>Service: resolved component
Service->>Service: swap variable material
end
Service-->>Client: updated Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR spans ~15 files with systematic but varied changes: new registry/resolver pattern (medium complexity), multiple resolver implementations following similar contracts (repetitive but logic-dense), service refactoring with per-axis delegation (requires tracing flow), and extensive test infrastructure. While individual files follow consistent patterns, the heterogeneous mix of registry mechanics, database queries, error handling paths, and legacy synthesis logic demands separate reasoning for each component. The presence of detailed plan documentation and comprehensive fixtures reduces friction but does not eliminate the need to validate protocol adherence, resolver correctness, and integration soundness. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Council Results0 tests 0 ✅ 0s ⏱️ Results for commit cf5ddbb. ♻️ This comment has been updated with latest results. |
Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 54ff2693-7376-448c-bcc2-cb82b473bf38
Adds the dataclass that resolvers return from list_options() and
expands the Protocol to include the three required methods
(list_options, resolve_to_component, synthesize_legacy).
Also tightens the register() docstring to make last-write-wins
semantics explicit for production callers (per Task 0 code review I-2).
Python 3.13 fix: type_name class-var annotation requires a sentinel
default value ("") to appear in dir(AxisTypeResolver); bare annotation
without a default is only in __annotations__, not dir().
Co-authored-by: Claude <claude@anthropic.com>
Agent-Session: dc5dd29c-b1bd-4aab-ab08-84fac726a8c9
…omponent Lifts variant_service._find_material_product logic into the registry. synthesize_legacy is a placeholder; Task 3 implements it. Adds fixtures: material_type_pla, color_black, supply_product_pla_black, fg004_template_with_material_color_axis. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: e39cd6e3-6521-4e1a-a5c3-67f71151d8f3
Implements the back-compat synthesis: legacy {material_type_id, color_id, ...}
records get lifted into v2 axis_selections in memory on read. Synthesized
records use the '__legacy__' sentinel key because the original
RoutingOperationMaterial.id isn't known from a flat record; callers must
accept this and not persist synthesized output.
Also folds in Task 2 review carry-forwards:
- TODO(pre-existing) marker for MaterialColor.active filter omission
- defensive empty-string for color_hex when None
- tighter assertions on list_options test
Co-authored-by: Claude <claude@anthropic.com>
Agent-Session: d49de754-802c-484b-b38e-5fa3bea07959
Reject any non-None schema_version that isn't 1 (instead of only rejecting 2). Closes the gap where a future schema_version=3 record carrying material_type_id/color_id would silently be synthesized into v2. Also folds in Task 3 review minor items: - docstring sentence explaining hardcoded label='Color' - test assertion on axis_count==1 - test assertion that __legacy__ sentinel key is present - new test pinning the future-version reject behavior Co-authored-by: Claude <claude@anthropic.com> Agent-Session: defc8888-ae1b-49e2-99cd-c27d110dfb80
…oduct_id, no item_type branching The centerpiece of B.1's generalization: variants are resolved as active children via parent_product_id, with NO item_type branching (Rule 1 from strategic plan §2). Same code path works for manufactured, component, and supply templates. Also adds the test that pins Rule 1: same resolver invocation against a manufactured template and a supply template both succeed. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 067b4807-e3c8-47af-835e-b679a156672a
Adds component_template_with_children fixture (item_type='component') and extends test_resolver_does_not_branch_on_item_type to iterate over all three: manufactured, supply, AND component. The 'component' case was implicitly covered by other fixtures but not explicitly named in the Rule 1 enforcement test. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 0e0c7d78-5c1a-4f85-b7c0-49afc35d7858
…is_count Single read path for variant_metadata + configuration JSONB. Lifts legacy flat shape into v2 via the material_color resolver's synthesize_legacy. Variants written before B.1 (no schema_version) are treated as v1 per the strategic plan §3.4. compute_axis_count walks nested axis_selections to count depth-aware (used by Task 6's cap enforcement). Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 2ff66a06-2fb5-4e92-85ba-4d536c6d138a
Depth-aware counting via compute_axis_count. Soft cap warns; hard cap raises HTTPException(400). Per the strategic plan §3.5 + the locked decisions in §2. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 0a1f90a8-d24c-40dd-bb4e-2a6070b11d42
…alColorResolver Replaces the inline query with a thin shim that calls the registry's material_color resolver. The legacy 404 error message is preserved character-for-character (the resolver was lifted from this exact function in Task 2). Existing variant_service tests stay green — no behavior change, only the code path moves into the registry. Step 1 of refactoring variant_service to delegate through the registry. Task 8 will refactor sync_routing_to_variants for per-line resolution. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: a551f327-e966-4468-a319-a4647e67d4b6
…o_variants Replaces the flat material+color lookup with registry-driven per-axis resolution. Each variable line in the variant's routing is resolved via the resolver matched to its axis_selections entry (keyed by RoutingOperationMaterial.id). Legacy variants (no schema_version, just material_type_id+color_id at top level) are read through read_axis_selections, which synthesizes a single __legacy__ entry; that entry's target applies to all is_variable lines (preserving today's single-axis behavior). Mixed-axis variants (1 material_color line + 1 component_template line + 2 fixed lines) now produce variants with both axes resolved correctly and fixed lines preserved verbatim -- Rule 3 from paragraph 2 of the strategic plan. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 4aae5d2b-45ad-4df5-b9a7-d3fa70920053
…a registry Delegates the child-of-current membership check to ComponentTemplateResolver.list_options. Same semantics (parent_product_id == current AND active=True), same 400 error message preserved character-for-character. The 8 existing B0 swap tests stay green as the refactor guard. Also folds in two Task 8 review carry-forwards: - Make pla_placeholder inactive in the mixed-axis fixture so the resolver has exactly one valid match (eliminates fixture-order fragility). - Log a warning when sync_routing_to_variants encounters a non-integer axis_selections key (instead of silently swallowing the ValueError). Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 96c5e86a-ce9f-4802-b4e9-8213da3e7160
…canary The 2-deep test demonstrates: customer can pick ball variant AND ball's filament finish independently, resolved by walking both axes via separate resolver calls. The 6-deep perf canary times 6 sequential resolves and either logs or asserts based on VARIANT_AXIS_PERF_THRESHOLD_MS env var. CI runs in log-only mode (no env var = no flake). Local benchmarks can set the env to enforce a target. Per strategic plan §4 + §8 mitigation for the O(N²) recursion-blowup risk. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 6e93d4e3-83f3-46a0-8d7a-3388a968cc5b
…sis.py The strongest correctness check in B.1: loads every variant in the DB, resolves the same leaf via legacy _find_material_product AND via the new registry path (synthesize_legacy + resolve_to_component), asserts identical results. Verified against dev DB: 4 templates, 30 variants checked, 0 skipped, 0 mismatches. Pytest wrapper runs the script as a subprocess so CI catches any mismatch immediately. Local pre-merge: run against dev DB to verify no production variant has drifted under the refactor. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: 2a653882-4400-4543-8550-7beb88af2ba0
- AxisOption: explicit __hash__ = None + docstring note (frozen but not hashable due to dict fields). Adds two tests pinning the contract: immutability and not-hashable. - test_protocol_requires_three_methods: informative assertion message showing missing methods on failure. - read_axis_selections: log warning when material_color resolver isn't registered (silent KeyError fallback was a hidden production failure mode). Move logger/constants to module top so logger is available to all functions in the module. All deferred items from earlier code reviews now closed. Co-authored-by: Claude <claude@anthropic.com> Agent-Session: f89cb086-affb-4f21-aee7-bdc09889d942
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
backend/tests/services/variant_axis/test_reader.py (1)
6-6: Avoid hardcoding cap constants in tests.Mirror the production constants directly so the tests fail when behavior changes (instead of silently diverging).
Suggested patch
-from app.services.variant_axis.reader import read_axis_selections, compute_axis_count, enforce_axis_cap +from app.services.variant_axis.reader import ( + read_axis_selections, + compute_axis_count, + enforce_axis_cap, + AXIS_CAP_SOFT, + AXIS_CAP_HARD, +) @@ -SOFT = 4 -HARD = 6 +SOFT = AXIS_CAP_SOFT +HARD = AXIS_CAP_HARDAlso applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/services/variant_axis/test_reader.py` at line 6, The tests hardcode cap numbers; change them to import and use the production cap constant instead so tests track behavior changes — import the constant used by enforce_axis_cap/compute_axis_count (e.g., AXIS_SELECTION_CAP or the cap constant defined in app.services.variant_axis) and replace the hardcoded literals in the test cases that call read_axis_selections, compute_axis_count, and enforce_axis_cap (including the occurrences around lines 72-73) with that imported constant.backend/tests/conftest.py (1)
776-809: Recommended: extract a fixture helper for routing/op/material scaffolding.This repeated setup block is now everywhere; one helper would cut maintenance cost and make new axis tests faster to author (and less likely to drift).
♻️ Refactor sketch
+def _make_single_op_routing_with_material( + db, *, template_id, work_center_id, op_code, op_name, + material_product_id, qty, unit, is_variable=True, route_code, route_name +): + from app.models.manufacturing import Routing, RoutingOperation, RoutingOperationMaterial + routing = Routing(product_id=template_id, code=route_code, name=route_name, is_active=True) + db.add(routing); db.flush() + op = RoutingOperation( + routing_id=routing.id, work_center_id=work_center_id, sequence=10, + operation_code=op_code, operation_name=op_name, + run_time_minutes=Decimal("30"), setup_time_minutes=Decimal("5"), + ) + db.add(op); db.flush() + rom = RoutingOperationMaterial( + routing_operation_id=op.id, + material_product_id=material_product_id, + quantity_per_unit=qty, + unit=unit, + is_variable=is_variable, + ) + db.add(rom); db.flush() + return routing, op, romAlso applies to: 878-908, 982-1011, 1076-1105, 1169-1198, 1264-1293, 1449-1512, 1638-1667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 776 - 809, Extract a reusable test fixture function (e.g., make_routing_with_operation_and_material) that encapsulates creation and db.add/db.flush for Routing, RoutingOperation and RoutingOperationMaterial (the routing, op, and variable_material objects), accept parameters for product_id/template id, work_center_id, supply component id(s), sequence, run_time_minutes, setup_time_minutes and is_variable/quantity/unit, and return the created instances; replace the repeated blocks that instantiate Routing, add op (RoutingOperation) and add variable_material (RoutingOperationMaterial) in tests with calls to this helper and ensure it uses the same db session passed into tests to call db.add and db.flush so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/variant_axis/reader.py`:
- Around line 27-44: The current reader logic normalizes any meta with
schema_version != 2 to an empty envelope, which silently accepts unknown future
schema_version values; update the code in the reader where meta and
meta.get("schema_version") are inspected so that if schema_version is present
and >2 (or otherwise unsupported) the function raises a clear exception (e.g.,
ValueError or a custom UnsupportedSchemaVersion) instead of returning
{"schema_version":2,...}; retain the existing fallback behavior only for
schema_version == 1 by continuing to attempt material_color resolution via
registry.get("material_color") and
mc.synthesize_legacy(variant_metadata_legacy=meta), but if schema_version is >2
raise with a message referencing meta and schema_version so callers (like
sync_routing_to_variants) cannot silently drop data.
In `@backend/app/services/variant_axis/registry.py`:
- Around line 34-41: Currently register(resolver: AxisTypeResolver) silently
replaces an existing entry in _REGISTRY, causing import-order-dependent
behavior; change register to fail-fast by raising a clear error if
resolver.type_name already exists in _REGISTRY, and provide a separate test-only
helper (e.g., register_for_tests or an explicit allow_replace=True argument
guarded by a TESTING flag) that permits replacement for test setup/teardown;
update references to call the new test helper in tests only and keep production
code using the strict register to prevent silent duplicate registration.
In `@backend/app/services/variant_service.py`:
- Around line 460-475: The code currently trusts
resolver.resolve_to_component(...) as both existence and authorization; instead,
after resolving target from axis_registry.get(...).resolve_to_component(db,
value=sel["value"]) validate the saved selection against the resolver's allowed
options by calling resolver.list_options(..., routing_material=<matching
template line or template identifier for this variant>) and ensure the resolved
target is present in that returned options set before accepting it into
resolved_per_line (for both "__legacy__" and numeric sel_key); if the target is
not listed, log a warning (include variant.sku, sel_key and sel["type"]) and
skip the selection to avoid accepting stale/tampered variant_metadata.
In `@backend/tests/conftest.py`:
- Around line 646-653: The fixtures in conftest.py are constructing MaterialType
and Color with attributes (e.g., code, base_material, process_type, density,
base_price_per_kg) that don't exist on the current ORM models; open
backend/app/models/material.py, confirm the actual fields on the MaterialType
and Color classes, and update every fixture construction (instances of
MaterialType and Color in conftest.py at the reported locations) to only pass
valid model attributes or rename/move those test values into the correct fields;
if the tests actually require those extra attributes, either add corresponding
nullable fields to the ORM models (MaterialType/Color) or add a factory/helper
that maps the test keys to the model's real field names so constructors no
longer throw at runtime.
- Line 1713: The synthetic inner key generation (inner_rom_id) currently
produces only ~1000 distinct ints via 99_998 + int(uid[:4], 16) % 1000 which is
collision-prone; change it to a guaranteed non-overlapping namespace (for
example derive a deterministic negative integer or a namespaced string) so it
cannot clash with real IDs—replace the expression that sets inner_rom_id
(currently using int(uid[:4], 16) % 1000) with a deterministic transformation
such as a negative number based on uid or a prefixed string key (e.g., using uid
directly as part of "synthetic-{uid}") and keep the rest of the code expecting
inner_rom_id consistent with that type.
In `@backend/tests/scripts/test_verify_variant_synthesis.py`:
- Around line 15-21: The subprocess.run call in test_verify_variant_synthesis.py
can hang indefinitely; update the call that invokes
subprocess.run([sys.executable, "scripts/verify_variant_synthesis.py"], ...) to
include a reasonable timeout parameter (e.g., timeout=60) and handle
TimeoutExpired by failing the test or capturing output; specifically modify the
invocation wrapped in the result variable and add try/except around
subprocess.run to catch subprocess.TimeoutExpired and call pytest.fail or assert
with the captured stdout/stderr to make CI deterministic.
In `@backend/tests/services/variant_axis/test_registry.py`:
- Line 1: The module-level docstring is misleading: importing registry via
app.services.variant_axis triggers package __init__ side effects and may already
register resolvers, so update the top docstring to reflect that resolvers can be
registered on import (e.g., "Registry contract tests — resolvers may be
registered on import due to package init side effects") and mention that tests
assume import-time registration; edit the string literal at the top of the test
file to this clearer wording so the comment matches actual behavior.
---
Nitpick comments:
In `@backend/tests/conftest.py`:
- Around line 776-809: Extract a reusable test fixture function (e.g.,
make_routing_with_operation_and_material) that encapsulates creation and
db.add/db.flush for Routing, RoutingOperation and RoutingOperationMaterial (the
routing, op, and variable_material objects), accept parameters for
product_id/template id, work_center_id, supply component id(s), sequence,
run_time_minutes, setup_time_minutes and is_variable/quantity/unit, and return
the created instances; replace the repeated blocks that instantiate Routing, add
op (RoutingOperation) and add variable_material (RoutingOperationMaterial) in
tests with calls to this helper and ensure it uses the same db session passed
into tests to call db.add and db.flush so behavior is unchanged.
In `@backend/tests/services/variant_axis/test_reader.py`:
- Line 6: The tests hardcode cap numbers; change them to import and use the
production cap constant instead so tests track behavior changes — import the
constant used by enforce_axis_cap/compute_axis_count (e.g., AXIS_SELECTION_CAP
or the cap constant defined in app.services.variant_axis) and replace the
hardcoded literals in the test cases that call read_axis_selections,
compute_axis_count, and enforce_axis_cap (including the occurrences around lines
72-73) with that imported constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6688683b-4a3f-4b85-babd-9dba71fa2dfc
📒 Files selected for processing (19)
backend/app/services/production_order_service.pybackend/app/services/variant_axis/__init__.pybackend/app/services/variant_axis/component_template.pybackend/app/services/variant_axis/material_color.pybackend/app/services/variant_axis/reader.pybackend/app/services/variant_axis/registry.pybackend/app/services/variant_axis/types.pybackend/app/services/variant_service.pybackend/scripts/verify_variant_synthesis.pybackend/tests/conftest.pybackend/tests/scripts/__init__.pybackend/tests/scripts/test_verify_variant_synthesis.pybackend/tests/services/variant_axis/__init__.pybackend/tests/services/variant_axis/test_component_template.pybackend/tests/services/variant_axis/test_material_color.pybackend/tests/services/variant_axis/test_reader.pybackend/tests/services/variant_axis/test_registry.pybackend/tests/test_variant_service_mixed_axis.pydocs/plans/variant-axis-registry-b1.md
Comment on lines
+27
to
+44
| if meta.get("schema_version") == 2: | ||
| return meta | ||
| # Try material_color synthesis (the only legacy shape we know about). | ||
| # Note: synthesize_legacy itself rejects schema_version != 1 (after Task 3 | ||
| # cleanup), so passing a future schema_version here returns None safely. | ||
| try: | ||
| mc = registry.get("material_color") | ||
| except KeyError: | ||
| logger.warning( | ||
| "material_color resolver not registered — legacy variant_metadata " | ||
| "cannot be synthesized; returning empty envelope. Check that " | ||
| "app.services.variant_axis.material_color is imported at startup." | ||
| ) | ||
| return {"schema_version": 2, "axis_selections": {}, "axis_count": 0} | ||
| synthesized = mc.synthesize_legacy(variant_metadata_legacy=meta) | ||
| if synthesized is not None: | ||
| return synthesized | ||
| return {"schema_version": 2, "axis_selections": {}, "axis_count": 0} |
Contributor
There was a problem hiding this comment.
Unknown schema versions should fail loud, not evaporate.
If meta carries schema_version >= 3, this returns the same empty envelope as None. Callers such as sync_routing_to_variants() then silently drop every saved axis and proceed with template defaults, which is the kind of forward-compat failure that turns into data drift. Please raise on unsupported schema versions, or otherwise propagate them explicitly, instead of normalizing them to “no selections”.
🛠️ Minimal direction
def read_axis_selections(meta: dict | None) -> dict:
@@
if not meta:
return {"schema_version": 2, "axis_selections": {}, "axis_count": 0}
if meta.get("schema_version") == 2:
return meta
+ schema_ver = meta.get("schema_version")
+ if schema_ver not in (None, 1):
+ raise HTTPException(
+ status_code=400,
+ detail=f"Unsupported variant_metadata schema_version={schema_ver}",
+ )
# Try material_color synthesis (the only legacy shape we know about).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/variant_axis/reader.py` around lines 27 - 44, The
current reader logic normalizes any meta with schema_version != 2 to an empty
envelope, which silently accepts unknown future schema_version values; update
the code in the reader where meta and meta.get("schema_version") are inspected
so that if schema_version is present and >2 (or otherwise unsupported) the
function raises a clear exception (e.g., ValueError or a custom
UnsupportedSchemaVersion) instead of returning {"schema_version":2,...}; retain
the existing fallback behavior only for schema_version == 1 by continuing to
attempt material_color resolution via registry.get("material_color") and
mc.synthesize_legacy(variant_metadata_legacy=meta), but if schema_version is >2
raise with a message referencing meta and schema_version so callers (like
sync_routing_to_variants) cannot silently drop data.
Comment on lines
+34
to
+41
| def register(resolver: AxisTypeResolver) -> None: | ||
| """Register a resolver under its type_name. | ||
|
|
||
| If a resolver with the same type_name is already registered, it is | ||
| replaced (last-write-wins). Tests rely on this for setup/teardown | ||
| isolation; production code should not register the same type_name twice. | ||
| """ | ||
| _REGISTRY[resolver.type_name] = resolver |
Contributor
There was a problem hiding this comment.
Avoid import-order roulette on duplicate registrations.
register() always does last-write-wins, so a second resolver for the same type_name silently replaces the first one. That is fine for tests, but it makes production behavior depend on import order and contradicts the B.1 contract that duplicates should fail fast outside test setup. Please make the default strict and keep replacement behind an explicit test-only escape hatch/helper.
♻️ One way to split prod vs test semantics
-def register(resolver: AxisTypeResolver) -> None:
+def register(resolver: AxisTypeResolver, *, replace: bool = False) -> None:
@@
- _REGISTRY[resolver.type_name] = resolver
+ if resolver.type_name in _REGISTRY and not replace:
+ raise RuntimeError(f"Resolver already registered: {resolver.type_name}")
+ _REGISTRY[resolver.type_name] = resolver🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/variant_axis/registry.py` around lines 34 - 41,
Currently register(resolver: AxisTypeResolver) silently replaces an existing
entry in _REGISTRY, causing import-order-dependent behavior; change register to
fail-fast by raising a clear error if resolver.type_name already exists in
_REGISTRY, and provide a separate test-only helper (e.g., register_for_tests or
an explicit allow_replace=True argument guarded by a TESTING flag) that permits
replacement for test setup/teardown; update references to call the new test
helper in tests only and keep production code using the strict register to
prevent silent duplicate registration.
Comment on lines
+460
to
+475
| for sel_key, sel in axis_selections.items(): | ||
| try: | ||
| resolver = axis_registry.get(sel["type"]) | ||
| target = resolver.resolve_to_component(db, value=sel["value"]) | ||
| except (KeyError, HTTPException) as e: | ||
| logger.warning( | ||
| "Variant %s: cannot resolve axis %s (type=%s): %s", | ||
| variant.sku, sel_key, sel.get("type"), e, | ||
| ) | ||
| .first() | ||
| ) | ||
| continue | ||
| if sel_key == "__legacy__": | ||
| resolved_per_line["__legacy_target__"] = target | ||
| else: | ||
| try: | ||
| resolved_per_line[int(sel_key)] = target | ||
| except ValueError: |
Contributor
There was a problem hiding this comment.
Validate stored selections before swapping components.
This path now trusts resolve_to_component() as if it were also an authorization check for the axis value, but it is only an existence lookup. For both new resolvers, the allowed domain is narrower than “any active product”: material_color is constrained by the MaterialColor rows, and component_template is constrained by parent_product_id. A stale or tampered variant_metadata entry can therefore point sync at an unrelated active product and silently rewrite the routing. Please validate each saved selection against resolver.list_options(..., routing_material=<matching template line>) before accepting it. Right now the JSON has a little too much command authority.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/variant_service.py` around lines 460 - 475, The code
currently trusts resolver.resolve_to_component(...) as both existence and
authorization; instead, after resolving target from
axis_registry.get(...).resolve_to_component(db, value=sel["value"]) validate the
saved selection against the resolver's allowed options by calling
resolver.list_options(..., routing_material=<matching template line or template
identifier for this variant>) and ensure the resolved target is present in that
returned options set before accepting it into resolved_per_line (for both
"__legacy__" and numeric sel_key); if the target is not listed, log a warning
(include variant.sku, sel_key and sel["type"]) and skip the selection to avoid
accepting stale/tampered variant_metadata.
Comment on lines
+646
to
+653
| mt = MaterialType( | ||
| code=f"PLA_BASIC_TEST_{uid}", | ||
| name="PLA Basic Test", | ||
| base_material="PLA", | ||
| process_type="FDM", | ||
| density=1.24, | ||
| base_price_per_kg=20.00, | ||
| ) |
Contributor
There was a problem hiding this comment.
MaterialType/Color fixtures use attributes not present in the shown ORM schema.
backend/app/models/material.py:17-33 and backend/app/models/material.py:75-91 show MaterialType/Color without code, base_material, process_type, density, base_price_per_kg. If this is the active model, these constructors will error at runtime.
🛠️ Proposed fix direction
- mt = MaterialType(
- code=f"PLA_BASIC_TEST_{uid}",
- name="PLA Basic Test",
- base_material="PLA",
- process_type="FDM",
- density=1.24,
- base_price_per_kg=20.00,
- )
+ mt = MaterialType(
+ name=f"PLA Basic Test {uid}",
+ description="Test fixture material type",
+ is_active=True,
+ )
- color = Color(
- code=f"BLK_TEST_{uid}",
- name="Black Test",
- hex_code="#000000",
- )
+ color = Color(
+ name=f"Black Test {uid}",
+ hex_code="#000000",
+ is_active=True,
+ )Also applies to: 665-669, 721-728, 736-736, 1333-1340, 1344-1348, 1670-1677, 1681-1685
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/conftest.py` around lines 646 - 653, The fixtures in
conftest.py are constructing MaterialType and Color with attributes (e.g., code,
base_material, process_type, density, base_price_per_kg) that don't exist on the
current ORM models; open backend/app/models/material.py, confirm the actual
fields on the MaterialType and Color classes, and update every fixture
construction (instances of MaterialType and Color in conftest.py at the reported
locations) to only pass valid model attributes or rename/move those test values
into the correct fields; if the tests actually require those extra attributes,
either add corresponding nullable fields to the ORM models (MaterialType/Color)
or add a factory/helper that maps the test keys to the model's real field names
so constructors no longer throw at runtime.
|
|
||
| # Synthetic inner key — not a real RoutingOperationMaterial row; the test | ||
| # calls resolvers directly and never touches sync_routing_to_variants. | ||
| inner_rom_id = 99_998 + int(uid[:4], 16) % 1000 |
Contributor
There was a problem hiding this comment.
Synthetic inner axis key range is collision-prone.
Line 1713 only gives ~1000 possible values; in long suites it can collide with real IDs or other synthetic keys and cause flaky assertions. Prefer a guaranteed non-overlapping domain (e.g., negative ints or namespaced string keys).
🛠️ Safer key generation
- inner_rom_id = 99_998 + int(uid[:4], 16) % 1000
+ # keep synthetic keys outside DB-generated positive integer ids
+ inner_rom_id = -int(uid[:8], 16)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| inner_rom_id = 99_998 + int(uid[:4], 16) % 1000 | |
| # keep synthetic keys outside DB-generated positive integer ids | |
| inner_rom_id = -int(uid[:8], 16) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/conftest.py` at line 1713, The synthetic inner key generation
(inner_rom_id) currently produces only ~1000 distinct ints via 99_998 +
int(uid[:4], 16) % 1000 which is collision-prone; change it to a guaranteed
non-overlapping namespace (for example derive a deterministic negative integer
or a namespaced string) so it cannot clash with real IDs—replace the expression
that sets inner_rom_id (currently using int(uid[:4], 16) % 1000) with a
deterministic transformation such as a negative number based on uid or a
prefixed string key (e.g., using uid directly as part of "synthetic-{uid}") and
keep the rest of the code expecting inner_rom_id consistent with that type.
Comment on lines
+15
to
+21
| result = subprocess.run( | ||
| [sys.executable, "scripts/verify_variant_synthesis.py"], | ||
| cwd=str(backend_dir), | ||
| capture_output=True, | ||
| text=True, | ||
| env=os.environ.copy(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Add a timeout to prevent CI from hanging forever.
Right now the subprocess can block indefinitely. A bounded timeout makes failures deterministic instead of wedging the pipeline.
Suggested patch
result = subprocess.run(
[sys.executable, "scripts/verify_variant_synthesis.py"],
cwd=str(backend_dir),
capture_output=True,
text=True,
env=os.environ.copy(),
+ timeout=180,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result = subprocess.run( | |
| [sys.executable, "scripts/verify_variant_synthesis.py"], | |
| cwd=str(backend_dir), | |
| capture_output=True, | |
| text=True, | |
| env=os.environ.copy(), | |
| ) | |
| result = subprocess.run( | |
| [sys.executable, "scripts/verify_variant_synthesis.py"], | |
| cwd=str(backend_dir), | |
| capture_output=True, | |
| text=True, | |
| env=os.environ.copy(), | |
| timeout=180, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/scripts/test_verify_variant_synthesis.py` around lines 15 - 21,
The subprocess.run call in test_verify_variant_synthesis.py can hang
indefinitely; update the call that invokes subprocess.run([sys.executable,
"scripts/verify_variant_synthesis.py"], ...) to include a reasonable timeout
parameter (e.g., timeout=60) and handle TimeoutExpired by failing the test or
capturing output; specifically modify the invocation wrapped in the result
variable and add try/except around subprocess.run to catch
subprocess.TimeoutExpired and call pytest.fail or assert with the captured
stdout/stderr to make CI deterministic.
| @@ -0,0 +1,68 @@ | |||
| """Registry contract tests — no resolvers registered yet.""" | |||
Contributor
There was a problem hiding this comment.
Docstring is stale/misleading about registration state.
Importing registry through app.services.variant_axis triggers package init side effects, so resolvers may already be registered.
Suggested wording tweak
-"""Registry contract tests — no resolvers registered yet."""
+"""Registry contract tests (may run with default resolvers pre-registered)."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Registry contract tests — no resolvers registered yet.""" | |
| """Registry contract tests (may run with default resolvers pre-registered).""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/variant_axis/test_registry.py` at line 1, The
module-level docstring is misleading: importing registry via
app.services.variant_axis triggers package __init__ side effects and may already
register resolvers, so update the top docstring to reflect that resolvers can be
registered on import (e.g., "Registry contract tests — resolvers may be
registered on import due to package init side effects") and mention that tests
assume import-time registration; edit the string literal at the top of the test
file to this clearer wording so the comment matches actual behavior.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Workstream B.1 from
docs/plans/variant-axis-generalization.md(PR #563, merged on main).Generalizes the variant axis from hardcoded material+color to a registry-dispatched resolver system, with read-only back-compat synthesis of legacy variant_metadata. Zero migrations. Zero behavior changes for end users. The 8 existing B0 swap tests stay green throughout — they're the refactor guard.
What changed
New package:
app/services/variant_axis/registry.py— Protocol-based resolver registry (last-write-wins for tests; production: first registration wins, duplicate raises)types.py—AxisOptionfrozen dataclass (immutable + not-hashable)material_color.py—MaterialColorResolver(preserves today's behavior + legacy synthesis)component_template.py—ComponentTemplateResolver(children of parent_product_id, NO item_type branching — Rule 1)reader.py—read_axis_selections,compute_axis_count,enforce_axis_cap(soft 4 / hard 6, depth-aware)Refactored services (preserve existing function signatures):
variant_service._find_material_product→ 5-line registry shimvariant_service.sync_routing_to_variants→ per-line axis resolution via registryproduction_order_service.swap_material_variant→ child-of-current via registry's list_options membershipPre-merge correctness guard:
scripts/verify_variant_synthesis.py— runs every existing variant through both legacy and registry paths, asserts identical leaf components. Pytest wrapper attests/scripts/test_verify_variant_synthesis.pyruns in CI.Coverage matrix rows implemented (per strategic plan §2)
test_material_color.pytest_component_template.pytest_resolver_does_not_branch_on_item_typeRule 1 enforcementtest_variant_service_mixed_axis.pytest_resolve_recursive_2_deepsync_routing_to_variants(full coverage in C.1's test suite)Three Rules from §2
if item_type ==branches in any new service code; verified by grep + by thetest_resolver_does_not_branch_on_item_typetest exercising manufactured, supply, and component item_types.AxisOption.labelis a plain str built from data; no hardcoded "Color" outside the__legacy__synthesis path (where the legacy shape carried no label and "Color" is the historical UI label).sync_routing_to_variants'sif t_mat.is_variable and target:guard ensuresis_variable=Falselines retain their templatecomponent_idverbatim. Mixed-axis test asserts this.Pre-merge correctness checks
scripts/verify_variant_synthesis.pyagainst dev DB: 0 mismatches across 30 variantsThree deferred items (from earlier code reviews) — all closed
register()docstring — applied in commit 7fd39e6: "Last-write-wins for tests" tightened to explicit production semantics.__hash__ = None+ two pinning tests (immutability + not-hashable).read_axis_selectionssilent KeyError — applied in final commit cf5ddbb:logger.warning(...)replaces silent fallback; moved logger/constants to module top for consistent availability.Out of scope (per strategic plan §9)
create_variantwrites the v2 axis_selections shape)VariantAxisGrid.jsx)configuration+ SO→PO substitutionTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores