feat(equipment): Fixture PIDINST read closure (Phase 1)#43
Merged
Conversation
First slice of Fixture-tier PIDINST integration. Ships the read path
end-to-end without touching the write path: operators can fetch a
Fixture's PIDINST record at GET /fixtures/{fixture_id}/pidinst (and via
the matching MCP tool), with full PIDINST 1.0 property cascade from
bound Assets.
WHY: Asset-tier PIDINST shipped at PR #38 covers the device tier (BODC
convention), but operators speak of "instruments" as composite Fixtures
(HZB convention). Without Fixture-tier records, the only PID an operator
can mint for an imaging instrument is a per-detector DOI, not the
beamline-as-a-whole DOI that publications and Commons search actually
key on. This slice closes the read path so operators can preview the
record before Phase 2 ships the assign mutation.
Surface added:
- Fixture.persistent_id field as data-substrate (no event yet; mirrors
slice E.1's Asset.commissioned_at pattern; Phase 2 ships the event +
assign slice)
- FixturePidinstView + FixtureComponentRef value objects in the kernel
- to_fixture_pidinst_record serializer (sibling to to_pidinst_record;
shares PidinstRecord kernel + StrEnums + invariants per Lock 1 from
Stage 0)
- 5-class FixturePidinstSerializationError taxonomy inheriting from
cross-tier PidinstSerializationError base; all five registered as HTTP
handlers in routes.py
- View assembler walks bound Assets one level deep with the union
cascade for owners (dedupe by name+identifier, sort by name per Lock
7) and the model-mediated cascade for manufacturers (load Model per
bound Asset, gather Model.manufacturer, dedupe, sort)
- Unminted bound Assets skipped in components and surfaced in
Description block only per Lock 11 and the L27 revision
- get_fixture_pidinst feature slice (handler + route + MCP tool)
mirroring the get_asset_pidinst sibling byte-for-byte
Locks honored from Stage 0 + Stage 1: L1 (separate serializer + shared
kernel), L2 (resourceTypeGeneral=Instrument), L3 (HasComponent native
with substitution at the future slice-6 renderer; verified by the L3
deep-dive memo), L6 (read endpoint matches Asset E.1 path convention),
L7 (owners union policy), L9 revised (manufacturers model-mediated),
L11 (one-level depth, no auto bidirectional linking).
Deferred and explicitly out of scope for Phase 1:
- FixturePersistentIdAssigned event + assign_fixture_persistent_id
slice (Phase 2)
- Production DataCite adapter with HasPart substitution (Phase 3)
- Configuration-hash for cross-facility convergence (Federation BC
trigger; see project_fixture_configuration_hash_followup)
Scale: 24 files, 3284 insertions / 191 deletions. 29 unit + 10
integration + 1 contract + 1 fitness new tests; 16,674 architecture
fitness tests still green; adversarial 8-claim 3-vote refutation
returned 24/24 confirmed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
xmap
added a commit
that referenced
this pull request
Jun 5, 2026
The three Fixture-PIDINST integration test files landed via PRs #43 + #45 after this branch was created, so the install-then-register choreography this branch enforces was not applied to their fixture setups. Their tests register an Asset and then immediately call register_fixture binding that Asset, which trips the new INV-4 orphan guard (FixtureAssetNotInstalledError) added in this branch. Adds a sibling helper `install_existing_asset_into_fresh_mount` in `tests/integration/_equipment_helpers.py` that activates an already-registered Asset, registers a fresh Frame + Mount under uuid4 ids, and installs the Asset. Use this when the test already needed to register the Asset itself (to bind a model_id or seed an owner) and now needs to satisfy INV-4 before calling register_fixture. Companion to the existing seed_installed_asset helper that handles the no-model / no-owner case from scratch. Each of the three PIDINST test files calls the new helper right after its register_asset step. Slot codes use the asset's uuid as a suffix to keep them unique across helper calls within the same test run. All 10 PIDINST tests now pass locally. Pre-this-fix the CI run on this branch was failing 6 of them with FixtureAssetNotInstalledError. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
xmap
added a commit
that referenced
this pull request
Jun 5, 2026
…#44) * feat(equipment): register_fixture rejects Decommissioned bound assets Adds a single cross-aggregate guard: every Asset referenced by `slot_asset_bindings` must NOT be Decommissioned. Mirrors the sibling AssetCannotAttachToFixtureError precondition at attach-time and rejects at register-time so the operator does not register a Fixture that would inevitably fail later at attach_asset_to_fixture (Fixture is single-event-genesis and cannot be amended). - New FixtureAssetNotAttachableError carries the sorted-first offending asset_id and current lifecycle string, mirroring the FixtureAssetNotFoundError deterministic-error precedent in the same decider. - RegisterFixtureContext gains a `lifecycle_by_asset_id` dict populated from the existing per-Asset load_asset gather (no extra round-trip, no new projection). Default empty dict means decider-only unit tests that exercise other invariants leave the guard inactive (mirrors family_ids_by_asset_id's relaxed default). - Decider guard ordering: existence (FixtureAssetNotFoundError) -> NEW lifecycle (FixtureAssetNotAttachableError) -> unknown-slot -> cardinality -> family-mismatch -> param-overrides. Operator sees the most actionable error first. - Route + OpenAPI 409 description list the new cause; routes.py wires the new error into the existing _handle_cannot_transition family alongside its Asset-BC mirror. Zero existing tests break because every existing happy-path test uses Active assets. Two new decider unit tests (guard fires with sorted-first determinism + empty-dict short-circuit), plus one integration test exercising the full handler stack end-to-end against Postgres (register_asset -> decommission_asset -> register_fixture -> FixtureAssetNotAttachableError). Closes INV-5 from the Fixture+Mount+Asset alignment plan; first half of Slice 3 (INV-4 orphan-binding guard deferred to Slice 3b to keep this slice ripple-free). Slice 3a in the Option A sequence. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(equipment): register_fixture rejects orphan bound assets (INV-4) Adds the second cross-aggregate guard from Slice 3 (3a shipped the Decommissioned-lifecycle half). Every Asset referenced by `slot_asset_bindings` must currently be installed in some Mount; operators that try to register a Fixture for un-racked assets are rejected with `FixtureAssetNotInstalledError` instead of silently materializing an unbacked composition. Closes INV-4 from the Fixture+Mount+Asset alignment plan: the contract becomes install_asset -> register_fixture, never the reverse. - New FixtureAssetNotInstalledError carries the sorted-first orphan asset_id (matches sibling FixtureAssetNotFoundError / FixtureAssetNotAttachableError deterministic-error precedent). - RegisterFixtureContext gains a `mount_id_by_asset_id` field shaped as `dict[UUID, UUID | None] | None`. Whole-None disables the guard for the pool-less test path (matches install_asset / decommission_asset short-circuit convention); per-entry None signals an orphan and fires the guard. - Handler kicks off three concurrent I/O streams via asyncio: the existing `assembly_state_task` + per-Asset `load_asset` gather, plus a new `mount_ids_future` returned by `asyncio.gather` over `load_asset_location` calls. `asyncio.gather` already schedules its children concurrently, so storing the future (without await) is the canonical way to start a gather alongside other work. - Decider guard ordering: existence -> NEW Decommissioned (3a) -> NEW orphan (3b) -> unknown-slot -> cardinality -> family-mismatch -> param-overrides. Decommissioned check fires before orphan when both apply (lifecycle is the more fundamental constraint). - Route + OpenAPI 409 description list the new cause; routes.py wires the new error into _handle_cannot_transition. Test ripple (5 integration files updated to install-then-register): factored a shared `seed_installed_asset` helper into `tests/integration/_equipment_helpers.py` that registers Frame + Mount + Asset, activates and installs the Asset, drains projections, and returns the ids. Each rippled test calls the helper before register_fixture, so the outer FixedIdGenerator id pool only budgets for the post-seed work. Asset version assertions bumped +1 to account for the AssetActivated event the helper emits. Three new decider unit tests (orphan fires with sorted-first determinism, pool-None short-circuit, ordering vs Decommissioned) plus one new integration test that exercises the full handler stack end-to-end against Postgres without installing the Asset first and asserts FixtureAssetNotInstalledError fires. Slice 3b in the Option A sequence; gate-reviewed 4/5/5 (DDD-fit should-fix on concurrency folded in here). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(architecture): make_inmemory_kernel may not be called from production code Adds a parametrized AST fitness test that scans every git-tracked .py file for direct calls to `make_inmemory_kernel(...)` and asserts each call site is either the definition module `src/cora/infrastructure/deps.py` or under `tests/`. `make_inmemory_kernel` builds a Kernel with `pool=None`. Three cross-aggregate guards short-circuit on `pool=None` for the pool-less test path: install_asset (since 5g), decommission_asset (slice 1), register_fixture (slice 3a). If a future production module accidentally wires the in-memory primitive (alternate entrypoint, MCP-only deployment, lazy-pool refactor), every one of those guards silently disables with zero test failure. This is the "load-bearing assumption with no fitness" risk the slice 3a / 3b gate-review adversarial flagged, and the rule-of-three trigger the `feedback_pool_none_short_circuit_rule_of_three.md` memo captured. Test shape mirrors `test_kernel_construction_single_site.py`: textual `make_inmemory_kernel(` filter + AST `ast.Call` walk + a `_PRODUCTION_ALLOWLIST` with one entry (deps.py) + a bulk allowance for any path under `tests/` + a drift-catcher that the allowlist file exists. Mutation-checked: dropping a one-liner `make_inmemory_kernel(...)` call site at `src/cora/_pool_none_regression.py` makes the test fail at the exact line; removing it returns the suite to green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * review(equipment): address PR #44 second-pass findings (concurrency + determinism + audit-tag strip + docs) Independent review of the INV-4 (orphan-bound-asset) guard surfaced five should-fix findings across correctness, test coverage, naming conventions, and docs. Four addressed here; the fifth (contract-tier pool=None bypass) is closed by the sibling commit adding a `make_inmemory_kernel` production-call-sites architecture fitness. Concurrency (handler.py): the three I/O streams that load the assembly, the per-Asset state, and per-Asset mount location used a hand-rolled `asyncio.create_task` + bare `asyncio.gather` future with sequential awaits. If any one of them raised, the others were never awaited and continued running in the background, surfacing as 'task exception never retrieved' warnings and holding pool connections after the handler had already returned an error. Rewrapped in `asyncio.TaskGroup` so a failure in any child cancels the siblings deterministically before the handler unwinds. Sorted-first determinism (test_register_fixture_decider*.py): the existing test for the orphan guard used a single asset_id, which made sorting trivially identity-preserving. A regression from `sorted(..., key=str)[0]` to `next(iter(...))` would have passed silently. Added a deterministic two-line test with three concrete UUIDs whose dict insertion order disagrees with their stringified sort order, plus a Hypothesis property test parameterized over frozenset[UUID] of 2-5 orphans that asserts the raised id equals `sorted(orphan_ids, key=str)[0]` regardless of iteration order. Naming hygiene (CLAUDE.md hard rule): stripped 14 occurrences of the audit-plan tags 'INV-4' and 'slice 3b' from source and test files. The durable WHY (cross-aggregate guard requiring referenced Assets to be currently installed in some Mount) was already stated alongside each tag, so the strip is mechanical and the prose survives. Module docs (docs/architecture/modules/equipment/index.md): the RegisterFixture error taxonomy listed only the pre-existing causes; both FixtureAssetNotAttachable (lifecycle guard, pre-existing miss from slice 3a) and FixtureAssetNotInstalled (install-required guard added by this slice) are now first-class entries. The register_fixture invariants prose at the top of the doc also gained the lifecycle + install preconditions that the decider now enforces. Tests after fix: 21/21 register_fixture decider tests pass (incl. 1 new example + 1 new PBT); 1726 equipment unit + integration tests pass; 16911 architecture fitnesses pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(equipment): install bound Assets in PIDINST fixture tests for INV-4 The three Fixture-PIDINST integration test files landed via PRs #43 + #45 after this branch was created, so the install-then-register choreography this branch enforces was not applied to their fixture setups. Their tests register an Asset and then immediately call register_fixture binding that Asset, which trips the new INV-4 orphan guard (FixtureAssetNotInstalledError) added in this branch. Adds a sibling helper `install_existing_asset_into_fresh_mount` in `tests/integration/_equipment_helpers.py` that activates an already-registered Asset, registers a fresh Frame + Mount under uuid4 ids, and installs the Asset. Use this when the test already needed to register the Asset itself (to bind a model_id or seed an owner) and now needs to satisfy INV-4 before calling register_fixture. Companion to the existing seed_installed_asset helper that handles the no-model / no-owner case from scratch. Each of the three PIDINST test files calls the new helper right after its register_asset step. Slot codes use the asset's uuid as a suffix to keep them unique across helper calls within the same test run. All 10 PIDINST tests now pass locally. Pre-this-fix the CI run on this branch was failing 6 of them with FixtureAssetNotInstalledError. 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
First slice of Fixture-tier PIDINST integration. Ships the read path end-to-end without touching the write path: operators can fetch a Fixture's PIDINST record at
GET /fixtures/{fixture_id}/pidinst(and via the matching MCP tool), with full PIDINST 1.0 property cascade from bound Assets.Fixture.persistent_idfield as data-substrate (no event yet; mirrors slice E.1'sAsset.commissioned_atpattern)FixturePidinstView+FixtureComponentRefvalue objects in the kernelto_fixture_pidinst_recordserializer (sibling toto_pidinst_record; shares thePidinstRecordkernel + StrEnums + invariants per Stage 0 Lock 1)FixturePidinstSerializationErrortaxonomy inheriting from cross-tierPidinstSerializationErrorbase; all five registered as HTTP handlers inroutes.pyModel.manufacturer, dedupe, sort)get_fixture_pidinstfeature slice (handler + route + MCP tool) mirroring theget_asset_pidinstsibling byte-for-byteWhy
Asset-tier PIDINST shipped at PR #38 covers the device tier (BODC convention), but operators speak of "instruments" as composite Fixtures (HZB convention). Without Fixture-tier records, the only PID an operator can mint for an imaging instrument is a per-detector DOI, not the beamline-as-a-whole DOI that publications and Commons search actually key on. This slice closes the read path so operators can preview the record before Phase 2 ships the assign mutation.
Stage 0 locks honored
resourceTypeGeneral=InstrumentDeferred to later phases
FixturePersistentIdAssignedevent +assign_fixture_persistent_idsliceScale + verification
24 files, 3284 insertions / 191 deletions. 29 unit + 10 integration + 1 contract + 1 architecture-fitness new tests. 16,674 architecture fitness tests still green. Adversarial 8-claim 3-vote refutation returned 24/24 confirmed (unminted-Asset skipping, owners union shape, model-mediated manufacturers, data-substrate-only
persistent_id, match/case no-default arm, noFixtureDoiMinterport created, slice dir carries SUBJECT noun, all 5 errors have HTTP handlers).Test plan
Generated with Claude Code