feat(equipment): PIDINST read closure (slice E.1)#34
Merged
Conversation
Closes slice D Lock 14: Asset PIDINST records are now reachable
over HTTP + MCP, and Slice C's OwnerStateNotAvailableError stops
firing for owner-populated Assets through the production read
route.
Asset state extension: commissioned_at + decommissioned_at fields
folded from existing AssetRegistered.occurred_at (genesis
Commissioned lifecycle) and AssetDecommissioned.occurred_at. No
new events. Atlas migration 20260603130000 adds three columns to
proj_equipment_asset_summary (lifecycle timestamps + reserved
persistent_id JSONB for slice F).
Feature slice features/get_asset_pidinst/ with query.py (sibling
6/6 read-slice convention), _view_assembler.py calling bare
load_asset + load_model + load_family functions (matches
get_asset_integration_view/handler.py precedent), handler.py,
route.py with PidinstRecordResponse Pydantic mirror, and tool.py
exposing MCP per BC slice-contract convention. Route
GET /assets/{asset_id}/pidinst returns 200 / 404 / 409 (state) /
422 (view prep) / 500 (defensive). Two shared exception handlers
wire the four routable PidinstSerializationError subclasses.
Response body uses BC-uniform {"detail": str(exc)} shape
(structured error body deferred to D-ERRBODY).
Tests (all green):
- Handler-level integration suite at
tests/integration/test_get_asset_pidinst_handler_postgres.py
(12 scenarios per memo section 8).
- HTTP-level contract suite at
tests/contract/test_get_asset_pidinst_endpoint.py (4 orthogonal
route-layer assertions; a regression dropping the
exception-handler tuple would silently 500 here).
- Architecture fitness: status_map_is_complete (L15, AST-walks
add_exception_handler Call nodes), assembler_uses_loaders (L17),
closure_proof_suite_is_present (R4).
- 8 assembler + 4 handler + 4 projection + 2 bootstrap unit tests.
The slice's earlier memo posture of "no MCP exposure in E.1" was
wrong: every sibling read slice ships tool.py + tools-registration
per the BC slice-contract fitness. tool.py mirrors
get_asset_integration_view/tool.py shape.
Memo: project_asset_persistent_id_design.md (23 Locks, 17 Defers).
Three rounds of Stage 3 gate review applied; the round-3 review on
the implementation surfaced the no-MCP and structured-error-body
misalignments which this commit corrects.
Slice F follows for the write path (Asset.persistent_id + assign
mutation + DataCite mint adapter; gated on facility credentials).
Slice E.Fixture follows in parallel for the Fixture-tier PIDINST
sibling per the session-2026-06-04 discussion. Also captured:
project_fixture_configuration_hash_followup.md notes the Fixture
content-hash idea for Federation BC cross-facility configuration
sharing.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
xmap
force-pushed
the
asset-persistent-id-slice-e
branch
from
4df9af6 to
73e7c95
Compare
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Merged
5 tasks
xmap
added a commit
that referenced
this pull request
Jun 5, 2026
…w-ups (#39) * fix(equipment): Asset evolver carries commissioned_at + decommissioned_at through AssetOwnerRemoved + AssetAttachedToFixture The Asset evolver's Critical Invariant docstring requires every transition arm to preserve commissioned_at and decommissioned_at from prior state. Two arms violated the invariant by omitting both fields from their Asset(...) construction. Replaying a stream that included either event silently wiped both timestamps to None, corrupting PIDINST Property 11 lifecycle dates on any Asset whose owners changed or that was attached to a Fixture after commissioning. Same silent-data-loss shape as the Sub-Stage B.6 bug previously caught on the fixture_id field. That earlier fix added fixture_id to the preserve-fields list but did not catch the parallel commissioned_at / decommissioned_at omissions on these two arms, which were folded in later via the PIDINST read closure work. Surfaced by the PseudoAxis Stage-1 design's implementation-readiness gate review (round 2) as an out-of-scope finding while staging the partition_rule threading work. Two targeted regression tests pin the carry-forward: - test_evolve_asset_owner_removed_preserves_lifecycle_timestamps - test_evolve_asset_attached_to_fixture_preserves_lifecycle_timestamps Both assert that prior commissioned_at / decommissioned_at survive the respective evolver transitions. Mirror the existing test_evolve_<transition>_preserves_<field> pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(equipment): widen Asset evolver lifecycle-date carry-forward to matrix + AST fitness Follow-up to 493aad0bb (`fix(equipment): Asset evolver carries commissioned_at + decommissioned_at through AssetOwnerRemoved + AssetAttachedToFixture`). That surgical fix pinned the two originally-broken arms with arm-specific regression tests. This commit adds two complementary layers of coverage so the bug class cannot re-emerge on a future evolver arm. Layer 1: full carry-forward matrix in test_asset_lifecycle_dates_evolver.py. Mirrors the existing per-field parametrize convention (test_evolve_mutation_preserves_drawing, ..._model_id, ..._alternate_identifiers) and extends it to both PIDINST lifecycle dates across all 17 non-writer arms, explicitly including the owner_added / owner_removed / fixture_attached / fixture_detached arms whose absence from the older parametrize lists is what allowed the original bug to ship undetected. Writer-arm behavior (AssetRegistered as genesis writer; AssetDecommissioned as terminal writer) is pinned in two dedicated tests. Layer 2: AST-based architecture fitness in test_asset_evolver_lifecycle_dates_carry_forward.py. Parses evolver.py, walks every `case <EventName>(...):` arm in `evolve`, and asserts the inner `Asset(...)` call contains both `commissioned_at=prior.commissioned_at` and `decommissioned_at=prior.decommissioned_at` unless the arm is one of the documented writers. The structural check catches arms that no behavior test was written for; the behavior matrix catches regressions in arms that exist today. Sanity-checked by reverting the original evolver patch and confirming both layers report the violations. Surfaced by the Equipment-BC balance audit (post-ship review of slice E.1 PIDINST work) as the highest-priority recommendation: protects PR #34 from corrupting on any Asset that has owners removed or is attached to a fixture. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(equipment): move _assembly_content_hash.py into the assembly aggregate dir Files at the aggregates/ level (one directory above the per-aggregate subdirs) signal "shared by multiple aggregates." _drawing.py and _placement.py earn that placement with consumers across Mount, Frame, and several siblings. _assembly_content_hash.py did not: its only consumers are define_assembly/decider.py, version_assembly/decider.py, and its own test. Living at the shared-helper level was misleading and would have nudged future designers (notably the Fixture-tier content-hash follow-up tracked in project_fixture_configuration_hash_followup) toward extending this module rather than building a sibling inside aggregates/fixture/. Move: apps/api/src/cora/equipment/aggregates/_assembly_content_hash.py -> apps/api/src/cora/equipment/aggregates/assembly/_content_hash.py Touches the two import sites (define_assembly + version_assembly deciders), the test file's import, and the hardcoded path entry in test_no_assembly_asset_level_literal.py. The PAYLOAD_TYPE constants and the function names are unchanged, so no event-shape or hash drift. The source docstring is rewritten to explain the single-aggregate scope and to flag the future Fixture-tier sibling expectation explicitly. Surfaced by the Equipment-BC balance audit as the lowest-effort honest-architecture fix: zero fold-cost, zero cross-BC ripple, and removes a load-bearing-looking signal that wasn't load-bearing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(equipment): factor Assembly per-id family lookup into find_missing_families_per_id define_assembly and version_assembly each opened the family stream fan-out inline: build the referenced FamilyId set, await asyncio.gather over load_family for each id, zip the results, and collect the missing ids into a frozenset before threading into context. The two implementations were near-identical (10-ish lines each, two formatting variants on the same logic). Extracting the lookup into a named helper in family/read.py shrinks each handler by ~6 lines, removes the asyncio import + asyncio.gather doctring mention from both, and earns the rule of three the moment a third per-id caller appears. Scope deliberately narrower than the Equipment-BC balance audit recommendation: - Model handlers (define_model, add_model_family) keep their inline `list_all_family_ids` + set-difference. They are monkeypatched at the handler-module level (`handler.list_all_family_ids`) in 15 test files; routing the lookup through a helper would invalidate every one of those patches and force a 15-file stub- shape rewrite for a 2-line per-handler win. Revisit when those tests get migrated to patch at the read-module location. - Asset handlers (register_asset, add_asset_family) keep their inline `load_model` calls. register_asset uses the loaded model only for an existence check with elaborate INFO logging on the not-found path; add_asset_family loads the model AND reads `model.declared_family_ids` for a subset gate. The two share a surface shape (`if model is None: raise ModelNotFoundError(id)`) but their downstream use differs enough that a single helper would either hide the asymmetry or carry an awkward parameter surface. The unused sibling helper `find_missing_families_in_registry` (bulk-SQL strategy for Model-side callers) was authored, then deleted before commit per the project's defer-until-trigger discipline: speculative helpers without a current caller are noise. When the Model-side test surface rework lands, that helper can ship alongside it in one commit. Surfaced by the Equipment-BC balance audit (Rec #3). The audit's original prescription targeted 6 handlers and proposed two strategy-variant helpers in family/read.py + matching helpers in model/read.py. Reading the actual code revealed the Model side has a test-shape coupling the audit did not see and the Asset side isn't a duplication at all. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(equipment): log PidinstRecordInvariantError at the application handler before re-raising PidinstRecordInvariantError is intentionally NOT wired to a custom HTTP handler per Lock 11 of project_asset_persistent_id_design: it is a server-bug backstop, not a routable client-facing error, and FastAPI's default 500 is the locked outcome (enforced by tests/architecture/test_get_asset_pidinst_status_map_is_complete.py which both pins the 4 routable PIDINST errors AND asserts this one is NOT registered). The locked policy left a real observability gap: FastAPI's default 500 returns the plain-text body `Internal Server Error` (not the BC-uniform `{"detail": str(exc)}` shape every other Equipment error uses) and prints the stack trace to stderr via traceback, NOT through the project's structlog `_log` channel. Sentry / Loki / structured monitoring sees nothing actionable: no asset_id, no correlation_id, no principal_id, no JSON fields. The naive fix (wrap construction inside `to_pidinst_record`) was rejected: that module is governed by L22 / L28 purity (tests/architecture/test_pidinst_serializer_purity.py) which forbids I/O accumulation including logging. The serializer must stay a pure transform. The right placement is the application boundary: the `get_asset_pidinst` handler wraps the `to_pidinst_record(view)` call in a try/except for `PidinstRecordInvariantError`, emits a structured `_log.exception("get_asset_pidinst.pidinst_record_invariant", ...)` with asset_id + principal_id + correlation_id + invariant reason, then re-raises. FastAPI's default 500 still fires, the body shape is unchanged, the locked policy is preserved, and operators get the structured trail they need to diagnose a recurrence. The handler-level placement also matches the existing convention for error-path logging in non-pure modules (infrastructure/idempotency_pruner.py:97, infrastructure/auth/bearer_auth_middleware.py:277, infrastructure/projection/worker.py:209). Three docstrings updated to point at the new arrangement: - _pidinst_serializer.to_pidinst_record notes that observability lives in the application handler and explains why (L22). - get_asset_pidinst/handler.py updates the 500-line in the error-routing table to cite L11 explicitly and to point at the in-handler log. - get_asset_pidinst/route.py mirrors the handler docstring's 500-line for the route-layer reader. New unit test test_handler_logs_and_reraises_on_pidinst_record_invariant_error monkeypatches to_pidinst_record to raise the invariant error, runs the handler inside structlog.testing.capture_logs(), and asserts both the re-raise AND the structured-log emission (event name, asset_id, principal_id, correlation_id, reason, log_level=error). Surfaced by the Equipment-BC balance audit (Rec #5). The audit's original prescription was to register a custom HTTP handler mapping PidinstRecordInvariantError to a structured 500; that recommendation would have failed the existing fitness test the audit did not see. The deeper investigation isolated the genuine observability gap and a placement that respects every locked constraint (Lock 11, L22, both fitness tests). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- 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
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
Closes slice D Lock 14: Asset PIDINST records are now reachable over HTTP + MCP, and Slice C's
OwnerStateNotAvailableErrorstops firing for owner-populated Assets through the production read route. The slice C serializer (PR #30) is no longer dead code on main.What ships
commissioned_at+decommissioned_atfolded from existingAssetRegistered.occurred_at(genesis Commissioned lifecycle) andAssetDecommissioned.occurred_at. No new events.20260603130000_add_asset_summary_persistent_id.sql: three new columns onproj_equipment_asset_summary(lifecycle timestamps + reservedpersistent_id JSONBfor slice F).features/get_asset_pidinst/withquery.py+_view_assembler.py(bareload_asset+load_model+load_family, matchingget_asset_integration_viewprecedent) +handler.py+route.py(withPidinstRecordResponsePydantic mirror) +tool.py(MCP per BC slice-contract convention).GET /assets/{asset_id}/pidinstreturns 200 / 404 / 409 (Owner/Manufacturer state) / 422 (LandingPage/AssetName view-prep) / 500 (defensivePidinstRecordInvariantError). Two shared exception handlers wire the four routablePidinstSerializationErrorsubclasses via the existing_handle_cannot_transition-style registration loop inequipment/routes.py._bootstrap.py: missing landing-page-template raisesRuntimeErrorat startup.cora.infrastructure.config.Settings:facility_publisher+landing_page_template.Tests (all green)
tests/integration/test_get_asset_pidinst_handler_postgres.py(12 scenarios per memo §8: 6 happy + 4 negative + 2 closure-proof).tests/contract/test_get_asset_pidinst_endpoint.py(4 orthogonal route-layer assertions; a regression dropping the exception-handler tuple would silently 500 here).status_map_is_complete(L15, AST-walksadd_exception_handlerCall nodes),assembler_uses_loaders(L17, imports + no-SQL),closure_proof_suite_is_present(R4, both suites must exist with required test counts).Notes
tool.py+ tools-registration per the BC slice-contract fitness.tool.pymirrorsget_asset_integration_view/tool.pyshape.{"detail": str(exc)}shape (structured{"code": ..., "asset_id": ...}deferred to a future BC-wide slice per memo Defer D-ERRBODY).commissioned_at.Design memo
project_asset_persistent_id_design.md(23 Locks, 17 Defers including D-ETAG, D-ASSIGN, D-ERRBODY). Three rounds of Stage 3 gate review applied (memo round 1, memo round 2 with regression fixes, implementation round with this commit's corrections).Follow-ups
Asset.persistent_idfield +assign_persistent_idmutation + DataCite mint adapter +DoiMinterport. Gated on facility credentials.project_fixture_configuration_hash_followup.mdcaptures the Fixture content-hash idea (realization-hash vs configuration-hash) for Federation BC cross-facility configuration sharing; not designed, not scheduled.Test plan
uv run ruff check+format --checkcleanuv run pyright src/cora/equipment/0 errorsuv run tach checkclean