feat(equipment): Asset.model_id binding + cross-BC subset invariant (closes Lock 6 + Lock 9)#18
Merged
Merged
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Closes Lock 6 + Lock 9 deferral from the Model design memo via the
locked Stage 1 design (project_asset_model_binding_design.md).
Implementation is purely additive on the Asset BC: one optional
field, one optional event payload key, one nullable projection
column, no new slices, no new event types.
Asset gains optional model_id: UUID | None = None set ONCE at
register_asset time (per Lock A; rebind via decommission +
re-register, no bind_asset_to_model slice in v1).
Cross-BC subset invariant Model.declared_families subset-of
Asset.family_ids enforced at add_asset_family when the Asset has
a bound Model. At register_asset the bind is permissive (load to
verify Model exists, but no subset assertion at genesis because
Asset.family_ids starts empty; subset would be vacuous). NOT
enforced at remove_asset_family per Lock B (eventual-consistency
posture; surface violations at next forward transition). No
cascade-validation when Model.add_model_family fires (Lock C);
no refusal of Deprecated-Model binding (Lock D, Family-deprecation
posture extended).
New error class AssetModelMismatch in aggregates/asset/state.py
carries (asset_id, model_id, declared_families, asset_family_ids)
with both sets verbatim in the message. Wired into routes.py
cannot_transition_cls tuple (HTTP 409). ModelNotFoundError
(HTTP 404) raised cross-BC if the supplied model_id does not
resolve.
Atlas migration is single forward-only ALTER TABLE
proj_equipment_asset_summary ADD COLUMN model_id UUID + partial
index WHERE model_id IS NOT NULL. No event upcasting required;
legacy AssetRegistered events fold to Asset(model_id=None, ...)
via payload.get("model_id") per the additive-state convention,
mirroring the Asset.drawing precedent.
Slice modifications (2 sites, NO new slices):
- features/register_asset/{command,decider,handler,route,tool}.py
- features/add_asset_family/handler.py
Aggregate + projection + wiring:
- aggregates/asset/{state,events,evolver,__init__}.py
- projections/asset.py
- routes.py: AssetModelMismatch in cannot_transition_cls
Tests: 28 files modified/added covering decider + paired PBT +
handler + REST contract + MCP contract + Postgres integration +
projection unit + projection integration tiers. Two drift-catcher
allowlists shrunk as a side effect:
- test_no_em_dashes.py: register_asset/command.py dropped
- test_decider_changes_require_paired_pbt.py: register_asset.decider
dropped (new paired PBT lands today)
Verification: 2991 Asset + Model + architecture tests pass; 14490
architecture tests pass; ruff clean; pyright 0/0/0; tach clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Stage 3 gate review (7 axes; baseline + standards-alignment specialist + security + migration-safety + design-memo conformance) returned GO_WITH_NITS for the Asset.model_id implementation: 0 P0, 3 P1 across 2 axes. This commit lands all 3 P1 fixes. P1-1 (TEST COVERAGE): missing contract test for the canonical idempotency-key collision case across model_id. The cross-BC hash_command includes model_id (RegisterAsset is a frozen dataclass; canonical hash covers every field) but the only same-key/different-body test in test_register_asset_idempotency varied name, not model_id. A future change excluding optional fields from the canonical hash, or a normalizer regression on UUID | None, would silently let two distinct Model bindings collide on the same key and return the wrong cached asset_id. Added test_post_assets_same_key_different_model_id_returns_422 with a local two-id load_model stub. P1-2 (TEST COVERAGE): AssetModelMismatch class name did not end in Error, suppressed via noqa: N818. Two architecture fitness gates silently skipped it (test_state_error_naming _taxonomy._error_class_names and test_routes_completeness ._bc_error_classes both filter on the Error suffix). A future refactor that dropped the class from routes.py cannot_transition_cls would degrade the cross-BC subset gate from 409 to an unmapped 500 with no drift catcher firing. Renamed AssetModelMismatch to AssetModelMismatchError across 9 files (src + tests); dropped the noqa: N818. P1-3 (ARCHITECTURE): docstring rename to phantom symbols Method.needs.families / Method.needs.supplies in 5 files. The actual Method aggregate field is needed_family_ids (state.py :285) and needed_supplies (state.py:325); Method.needs does not exist anywhere in the codebase. Reverted the 5 docstring edits. Tests: 3625 Asset + Model + architecture tests pass, 0 failures. ruff clean, pyright 0/0/0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
b7a3d61 to
b4b9756
Compare
xmap
added a commit
that referenced
this pull request
Jun 2, 2026
…dentity)
Closes the instance-tier identity gap per the locked Stage 1
memo project_asset_alternate_identifiers_design.md. Purely
additive: Asset gains alternate_identifiers: frozenset[
AlternateIdentifier]; AssetRegistered payload gains optional
alternate_identifiers key; 2 NEW targeted-mutation slices
(add_asset_alternate_identifier + remove_asset_alternate_
identifier) mirror the add_asset_port / remove_asset_port
precedent exactly; 1 forward-only Atlas migration adds JSONB
column + partial GIN index.
AlternateIdentifier is a flat (kind, value) VO. kind is a
closed StrEnum (SerialNumber | InventoryNumber | Other)
verbatim from PIDINST v1.0 property 13 alternateIdentifierType
controlled vocabulary. value is a trimmed 1-200 char string;
operators may enter serial numbers like "PCO-EDGE-5.5-001234"
or inventory tags like "INV-2026-001"; CORA stays opaque on
the value semantics.
Frozenset enforces no-duplicate (kind, value) pairs on the
same Asset at the aggregate level. NO cross-Asset uniqueness
check in v1 (operator-curation discipline; revisit if pilot
ops demand a global SerialNumber registry).
New error classes (3) in aggregates/asset/state.py:
- InvalidAlternateIdentifierValueError (422)
- AssetAlternateIdentifierAlreadyPresentError (409)
- AssetAlternateIdentifierNotPresentError (409)
Wired into routes.py cannot_transition_cls + validation
tuples. Both new slices ship full REST + MCP surface with
contract + integration tests; 2 deciders ship paired
Hypothesis PBTs.
Slice modifications:
- register_asset/{command,decider,route,tool}.py: optional
alternate_identifiers parameter passed through to
AssetRegistered event
- features/add_asset_alternate_identifier/{...}.py: NEW
slice, targeted-mutation, strict-not-idempotent (mirrors
add_asset_port precedent)
- features/remove_asset_alternate_identifier/{...}.py: NEW
slice, mirror of add slice
- aggregates/asset/{state,events,evolver,__init__}.py:
field + 2 new events + VO + closed enum + 3 error classes
- projections/asset.py: 3 new event handlers write JSONB
column with canonical-sorted ordering
- routes.py + wire.py + tools.py: 3 errors + 2 routers + 2
handlers + 2 MCP tools
Atlas migration 20260603100000_add_asset_summary_alternate_
identifiers.sql: ALTER TABLE ADD COLUMN JSONB NOT NULL
DEFAULT '[]'::jsonb + partial GIN index WHERE
jsonb_array_length(alternate_identifiers) > 0 for future
find-by-serial queries. Forward-only; no event upcasting.
Cross-BC binding: NONE. The slice is intra-Asset; no
load_model / load_family preflight. Simpler than the prior
Asset.model_id slice which needed cross-BC family_lookup.
PIDINST instance-tier identity is now complete:
- catalog tier (manufacturer + part_number): Model
- instance binding: Asset.model_id (PR #18)
- instance identifier (SerialNumber + InventoryNumber):
Asset.alternate_identifiers (this PR)
This unblocks the PIDINST DOI mint serializer slice (next).
One trivial post-implementation fix: the projection
integration test for the default-empty-array case asserted
the JSON string "[]" but asyncpg deserializes JSONB to a
Python list; assertion updated to accept both shapes (some
asyncpg codec paths return a string).
Tests: 3142 Asset + Model + Alternate Identifier +
architecture tests pass; 0 failures. ruff clean, pyright
0/0/0, tach clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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
Closes the Lock 6 + Lock 9 deferral from the Model design memo via the locked Stage 1 design (
project_asset_model_binding_design.md). Purely additive on the Asset BC: one optional field, one optional event payload key, one nullable projection column, no new slices, no new event types.Assetgains optionalmodel_id: UUID | None = Noneset ONCE atregister_assettime (per Lock A; rebind via decommission + re-register, nobind_asset_to_modelslice in v1).Cross-BC subset invariant
Model.declared_families ⊆ Asset.family_idsenforced atadd_asset_familywhen the Asset has a bound Model. Atregister_assetthe bind is permissive (load to verify Model exists, but no subset assertion at genesis because Asset.family_ids starts empty; subset would be vacuous). NOT enforced atremove_asset_familyper Lock B (eventual-consistency posture). No cascade-validation whenModel.add_model_familyfires (Lock C); no refusal of Deprecated-Model binding (Lock D, Family-deprecation posture extended).Commits (2)
a22ef3b64Implementation: 2 slice mods (register_asset+add_asset_family) +AssetModelMismatchErrorclass + 1 Atlas migration + projection update + 28 test files. ~2200 LOC.b7a3d614eGate-review P1 fixes: idempotency-key + model_id contract test +AssetModelMismatch→AssetModelMismatchErrorrename (drift-catcher visibility) + revert 5 phantomMethod.needs.familiesdocstring renames.Verification
20260602110000_add_asset_summary_model.sqlis forward-only ADD COLUMN + partial indexWHERE model_id IS NOT NULL.AssetModelMismatchErrorcorrectly visible to both fitness gates (_error_class_names+_bc_error_classes); drift catcher will fire on future regressions.Design-memo conformance
All 9 Locks (A through I) honored:
bind_asset_to_modelsliceadd_asset_family; permissive atregister_asset; NOT atremove_asset_familyAssetModelMismatchErrorcarries both sets verbatim; 409 viacannot_transition_clsmodel_idkey (mirrorsAsset.drawingprecedent)append_streamsAll Anti-hooks respected (no append_streams, no validate at remove_asset_family, no cascade, no
bind_asset_to_modelslice, no frozenset[UUID] for model_id, no in-state subset invariant, no Deprecated-Model refusal).Deferred (tracked as Watch items in the design memo)
bind_asset_to_modelslice (Option C) — fires on first re-cataloging operator requestModelDeprecatedForNewAssetsoft refusal — revisit at pilot operator surfaceAsset.alternate_identifiers, is the parallel slice)Test plan
POST /assetswithmodel_id+POST /assets/{id}/familiessubset gateNote on main CI
Main has been CI-red for several commits due to a pre-existing
conduct_procedureOperation BC test bug (unrelated to this PR; admin-merged at #17 with the same scope-isolation reasoning). This PR inherits that failure. The 2 failing tests are NOT in our diff and the rest of CI is green.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🤖 Generated with Claude Code