feat(equipment): register_fixture rejects orphan bound assets (INV-4)#44
Merged
Conversation
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>
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>
…ction 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>
…cts-decommissioned # Conflicts: # apps/api/openapi.json # apps/api/src/cora/equipment/aggregates/assembly/state.py # apps/api/src/cora/equipment/features/register_fixture/context.py # apps/api/src/cora/equipment/features/register_fixture/decider.py # apps/api/src/cora/equipment/features/register_fixture/handler.py # apps/api/src/cora/equipment/features/register_fixture/route.py # apps/api/src/cora/equipment/routes.py # apps/api/tests/integration/test_register_fixture_handler_postgres.py # apps/api/tests/unit/equipment/test_register_fixture_decider.py
… 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>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
3 tasks
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>
…s for INV-4 Companion to 71bb940: two more Fixture-PIDINST integration test files from PR #45 (Phase 2 write path) need the same install-before- register treatment to satisfy the new INV-4 orphan guard. - `test_get_fixture_pidinst_after_assign.py`: `_seed_asset_with_owner_and_model` - `test_assign_fixture_persistent_id_tool.py`: inline asset setup in `_seed_fixture` Both gain a call to `install_existing_asset_into_fresh_mount` right after their `register_asset` step, slot_code suffixed with the asset's uuid for in-test uniqueness. All 21 PIDINST tests (5 files) now pass locally; previous CI run on this branch failed 10 of the 11 from these two files. 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
FixtureAssetNotInstalledError-- every Asset inslot_asset_bindingsmust currently be installed in some Mount beforeregister_fixturecan materialize the compositionWhy
install_asset -> register_fixture, never the reverseNotable shape
RegisterFixtureContext.mount_id_by_asset_id: dict[UUID, UUID | None] | None; whole-Noneshort-circuits the guard for pool-less test paths (matchesinstall_asset/decommission_assetprecedent), per-entryNonesignals an orphanasyncio.gather:assembly_state_task+ per-Assetload_assetgather + newmount_ids_futureoverload_asset_locationcallsseed_installed_assethelper intests/integration/_equipment_helpers.py; 5 integration files rippled to install-then-registerTest plan
FixtureAssetNotInstalledErrorend-to-end against Postgres withoutinstall_asset