From 8613184317083c6488fbd8fa1539ac2b9f038676 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sat, 29 Aug 2026 17:41:23 -0500 Subject: [PATCH 1/7] Give Recipe a second, optional step list to close on closing_steps: tuple[RecipeStep, ...] = () on Recipe, RecipeDefined, and RecipeVersioned: the steps a future Conductor change will walk after the main list ends on a real terminal (Completed or Aborted), never on a Held pause. Additive and optional by design (unlike `steps`, no non-emptiness check) since most recipes will have none. RecipeVersioned replaces it wholesale alongside steps (a version can fix a broken closing step too); RecipeDeprecated carries it forward for audit. Registered in the evolver carry-forward guard so a future arm that forgets it fails loud instead of silently wiping it on replay. The event payload gets a new top-level "closing" key (not nested inside "steps"): gen_record_dispositions.py classifies one entry per dataclass field, so two fields sharing a wire key would silently overwrite each other's disposition in that lookup -- a genuinely separate field, wire-renamed via the existing _OVERRIDE_WIRE_KEYS mechanism, is what the generator's actual mechanics support cleanly. Verified by mutation: a from_stored that assumes "closing" is always present breaks on a payload from before this field existed. --- .../record_export/_dispositions.py | 30 +++++++++ .../cora/recipe/aggregates/recipe/events.py | 15 ++++- .../cora/recipe/aggregates/recipe/evolver.py | 13 ++-- .../cora/recipe/aggregates/recipe/state.py | 9 +++ .../test_recipe_evolver_carry_forward.py | 2 + .../tests/unit/recipe/test_recipe_events.py | 65 +++++++++++++++++++ .../tests/unit/recipe/test_recipe_evolver.py | 45 +++++++++++++ apps/api/tools/gen_record_dispositions.py | 5 ++ 8 files changed, 179 insertions(+), 5 deletions(-) diff --git a/apps/api/src/cora/infrastructure/record_export/_dispositions.py b/apps/api/src/cora/infrastructure/record_export/_dispositions.py index 060bcfdfdff..fca5510928f 100644 --- a/apps/api/src/cora/infrastructure/record_export/_dispositions.py +++ b/apps/api/src/cora/infrastructure/record_export/_dispositions.py @@ -1493,6 +1493,21 @@ }, "RecipeDefined": { "capability_id": "token:uuid", + "closing": { + "address": "drop:text", + "capture_name": "drop:text", + "command": "drop:text", + "criterion": "drop:opaque", + "input_uris": "by-value", + "name": "drop:text", + "output_ref_name": "drop:text", + "output_uri": "drop:text", + "parameters": "drop:opaque", + "params": "drop:opaque", + "timeout_s": "keep:number", + "value": "by-value", + "verify": "keep:number", + }, "name": "drop:text", "occurred_at": "keep:time", "recipe_id": "token:uuid", @@ -1532,6 +1547,21 @@ "steps_hash": "drop:text", }, "RecipeVersioned": { + "closing": { + "address": "drop:text", + "capture_name": "drop:text", + "command": "drop:text", + "criterion": "drop:opaque", + "input_uris": "by-value", + "name": "drop:text", + "output_ref_name": "drop:text", + "output_uri": "drop:text", + "parameters": "drop:opaque", + "params": "drop:opaque", + "timeout_s": "keep:number", + "value": "by-value", + "verify": "keep:number", + }, "occurred_at": "keep:time", "recipe_id": "token:uuid", "steps": { diff --git a/apps/api/src/cora/recipe/aggregates/recipe/events.py b/apps/api/src/cora/recipe/aggregates/recipe/events.py index 28d9719ea65..61c382a2955 100644 --- a/apps/api/src/cora/recipe/aggregates/recipe/events.py +++ b/apps/api/src/cora/recipe/aggregates/recipe/events.py @@ -63,6 +63,9 @@ class RecipeDefined: Status is implicit (`Defined`); the evolver sets it. All declarative fields are present in the genesis payload. + + `closing_steps` is additive and optional (default `()`); see + `Recipe.closing_steps`. """ recipe_id: UUID @@ -70,6 +73,7 @@ class RecipeDefined: capability_id: UUID steps: tuple[RecipeStep, ...] occurred_at: datetime + closing_steps: tuple[RecipeStep, ...] = () @dataclass(frozen=True) @@ -80,13 +84,14 @@ class RecipeVersioned: full step sequence REPLACES wholesale (a new version IS a new declaration). `name` and `capability_id` are PRESERVED across versions; re-binding to a different Capability requires a new - Recipe. + Recipe. `closing_steps` replaces wholesale too, alongside `steps`. """ recipe_id: UUID version_tag: str steps: tuple[RecipeStep, ...] occurred_at: datetime + closing_steps: tuple[RecipeStep, ...] = () @dataclass(frozen=True) @@ -129,6 +134,7 @@ def to_payload(event: RecipeEvent) -> dict[str, Any]: capability_id=capability_id, steps=steps, occurred_at=occurred_at, + closing_steps=closing_steps, ): return { "recipe_id": str(recipe_id), @@ -136,18 +142,21 @@ def to_payload(event: RecipeEvent) -> dict[str, Any]: "capability_id": str(capability_id), "steps": steps_to_dict(steps), "occurred_at": occurred_at.isoformat(), + "closing": steps_to_dict(closing_steps), } case RecipeVersioned( recipe_id=recipe_id, version_tag=version_tag, steps=steps, occurred_at=occurred_at, + closing_steps=closing_steps, ): return { "recipe_id": str(recipe_id), "version_tag": version_tag, "steps": steps_to_dict(steps), "occurred_at": occurred_at.isoformat(), + "closing": steps_to_dict(closing_steps), } case RecipeDeprecated( recipe_id=recipe_id, @@ -189,6 +198,9 @@ def from_stored(stored: StoredEvent) -> RecipeEvent: capability_id=UUID(payload["capability_id"]), steps=steps_from_dict(payload["steps"]), occurred_at=datetime.fromisoformat(payload["occurred_at"]), + # `.get`, not `["closing"]`: a stored event from before + # closing steps existed has no such key. + closing_steps=steps_from_dict(payload.get("closing", {"steps": []})), ), extra=_PAYLOAD_PARSE_EXTRA, ) @@ -200,6 +212,7 @@ def from_stored(stored: StoredEvent) -> RecipeEvent: version_tag=payload["version_tag"], steps=steps_from_dict(payload["steps"]), occurred_at=datetime.fromisoformat(payload["occurred_at"]), + closing_steps=steps_from_dict(payload.get("closing", {"steps": []})), ), extra=_PAYLOAD_PARSE_EXTRA, ) diff --git a/apps/api/src/cora/recipe/aggregates/recipe/evolver.py b/apps/api/src/cora/recipe/aggregates/recipe/evolver.py index 0351c472136..e215c916f5b 100644 --- a/apps/api/src/cora/recipe/aggregates/recipe/evolver.py +++ b/apps/api/src/cora/recipe/aggregates/recipe/evolver.py @@ -16,11 +16,12 @@ ## Replace vs preserve on each arm -- `RecipeVersioned` REPLACES `steps` with the new event's tuple (a - new version IS a new declaration). PRESERVES `name`, +- `RecipeVersioned` REPLACES `steps` AND `closing_steps` with the new + event's tuples (a new version IS a new declaration, and a version + can fix a broken closing step too). PRESERVES `name`, `capability_id`, and `replaced_by_recipe_id`. - `RecipeDeprecated` PRESERVES all declarative fields (steps, - capability_id, name, version) and ADDS the + closing_steps, capability_id, name, version) and ADDS the `replaced_by_recipe_id` pointer. Operators reading a deprecated Recipe still see what it declared (audit-critical). @@ -54,6 +55,7 @@ def evolve(state: Recipe | None, event: RecipeEvent) -> Recipe: name=name, capability_id=capability_id, steps=steps, + closing_steps=closing_steps, ): _ = state # genesis event; prior state ignored return Recipe( @@ -62,8 +64,9 @@ def evolve(state: Recipe | None, event: RecipeEvent) -> Recipe: capability_id=capability_id, steps=steps, status=RecipeStatus.DEFINED, + closing_steps=closing_steps, ) - case RecipeVersioned(version_tag=version_tag, steps=steps): + case RecipeVersioned(version_tag=version_tag, steps=steps, closing_steps=closing_steps): prior = require_state(state, "RecipeVersioned") return Recipe( id=prior.id, @@ -73,6 +76,7 @@ def evolve(state: Recipe | None, event: RecipeEvent) -> Recipe: status=RecipeStatus.VERSIONED, version=version_tag, replaced_by_recipe_id=prior.replaced_by_recipe_id, + closing_steps=closing_steps, ) case RecipeDeprecated(replaced_by_recipe_id=replaced_by_recipe_id): prior = require_state(state, "RecipeDeprecated") @@ -84,6 +88,7 @@ def evolve(state: Recipe | None, event: RecipeEvent) -> Recipe: status=RecipeStatus.DEPRECATED, version=prior.version, replaced_by_recipe_id=replaced_by_recipe_id, + closing_steps=prior.closing_steps, ) case _: # pragma: no cover # exhaustiveness guard assert_never(event) diff --git a/apps/api/src/cora/recipe/aggregates/recipe/state.py b/apps/api/src/cora/recipe/aggregates/recipe/state.py index 7621ebdf505..58d75f75cb1 100644 --- a/apps/api/src/cora/recipe/aggregates/recipe/state.py +++ b/apps/api/src/cora/recipe/aggregates/recipe/state.py @@ -233,6 +233,14 @@ class Recipe: one is deprecated with replacement. None on Deprecated-without-replacement and on Defined/Versioned. LOINC `MAP_TO` precedent matching `Capability.replaced_by_capability_id`. + + `closing_steps` is an additive, OPTIONAL second step list (default + `()`, no non-emptiness check): steps the Conductor walks after + `steps` ends on a real terminal (Completed or Aborted), never on a + Held pause. Authored, never derived; unlike `steps`, an empty + `closing_steps` is the common case, not an error. Replaced wholesale + by `version_recipe` alongside `steps`. See + [[project_conduct_closing_steps_design]]. """ id: UUID @@ -242,6 +250,7 @@ class Recipe: status: RecipeStatus = RecipeStatus.DEFINED version: str | None = None replaced_by_recipe_id: UUID | None = None + closing_steps: tuple[RecipeStep, ...] = () def __post_init__(self) -> None: if not self.steps: diff --git a/apps/api/tests/architecture/test_recipe_evolver_carry_forward.py b/apps/api/tests/architecture/test_recipe_evolver_carry_forward.py index cf39ceb1617..8e22b95a61f 100644 --- a/apps/api/tests/architecture/test_recipe_evolver_carry_forward.py +++ b/apps/api/tests/architecture/test_recipe_evolver_carry_forward.py @@ -58,6 +58,8 @@ "capability_id": frozenset(), # A new version replaces the step list wholesale; that IS the event. "steps": frozenset({"RecipeVersioned"}), + # Same rationale: a version can fix a broken closing step too. + "closing_steps": frozenset({"RecipeVersioned"}), "version": frozenset({"RecipeVersioned"}), "replaced_by_recipe_id": frozenset({"RecipeDeprecated"}), } diff --git a/apps/api/tests/unit/recipe/test_recipe_events.py b/apps/api/tests/unit/recipe/test_recipe_events.py index 900393a2ada..dc50107f940 100644 --- a/apps/api/tests/unit/recipe/test_recipe_events.py +++ b/apps/api/tests/unit/recipe/test_recipe_events.py @@ -85,6 +85,37 @@ def test_to_payload_recipe_versioned_serializes_version_tag_and_steps() -> None: assert "capability_id" not in payload # capability_id preserved on state, not in payload +@pytest.mark.unit +def test_to_payload_recipe_defined_serializes_closing_steps_under_closing_key() -> None: + rid, cid = uuid4(), uuid4() + defn = RecipeDefined( + recipe_id=rid, + name="R", + capability_id=cid, + steps=_steps(), + occurred_at=_NOW, + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + payload = to_payload(defn) + assert payload["closing"] == { + "steps": [{"kind": "setpoint", "address": "dev:shutter", "value": 0.0, "verify": False}] + } + + +@pytest.mark.unit +def test_to_payload_recipe_versioned_serializes_closing_steps_under_closing_key() -> None: + rid = uuid4() + ver = RecipeVersioned( + recipe_id=rid, + version_tag="v2", + steps=_steps(), + occurred_at=_NOW, + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + payload = to_payload(ver) + assert payload["closing"]["steps"] + + @pytest.mark.unit def test_to_payload_recipe_deprecated_serializes_replaced_by_or_none() -> None: rid, succ = uuid4(), uuid4() @@ -114,6 +145,40 @@ def test_from_stored_round_trips_recipe_versioned() -> None: assert rebuilt == original +@pytest.mark.unit +def test_from_stored_round_trips_recipe_defined_with_non_empty_closing_steps() -> None: + """Equality alone proves nothing about a field left at its default (`()`); + this pins a NON-default closing_steps surviving the round trip.""" + rid, cid = uuid4(), uuid4() + original = RecipeDefined( + recipe_id=rid, + name="R", + capability_id=cid, + steps=_steps(), + occurred_at=_NOW, + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + stored = _stored("RecipeDefined", to_payload(original)) + rebuilt = from_stored(stored) + assert rebuilt == original + assert isinstance(rebuilt, RecipeDefined) + assert rebuilt.closing_steps == (RecipeSetpointStep(address="dev:shutter", value=0.0),) + + +@pytest.mark.unit +def test_from_stored_recipe_defined_tolerates_a_pre_feature_payload_missing_closing_key() -> None: + """A RecipeDefined stored before closing steps existed has no "closing" key + at all; from_stored must default it to empty, not raise KeyError.""" + rid, cid = uuid4(), uuid4() + original = _make_defined(rid, cid) + payload = to_payload(original) + del payload["closing"] + stored = _stored("RecipeDefined", payload) + rebuilt = from_stored(stored) + assert isinstance(rebuilt, RecipeDefined) + assert rebuilt.closing_steps == () + + @pytest.mark.unit def test_from_stored_round_trips_recipe_deprecated_with_replacement() -> None: rid, succ = uuid4(), uuid4() diff --git a/apps/api/tests/unit/recipe/test_recipe_evolver.py b/apps/api/tests/unit/recipe/test_recipe_evolver.py index 3fc9431da07..2cf715121f4 100644 --- a/apps/api/tests/unit/recipe/test_recipe_evolver.py +++ b/apps/api/tests/unit/recipe/test_recipe_evolver.py @@ -82,6 +82,51 @@ def test_recipe_versioned_replaces_steps_wholesale_and_preserves_identity() -> N assert state2.name == state.name +@pytest.mark.unit +def test_recipe_defined_folds_closing_steps() -> None: + event = _defined(closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),)) + state = evolve(None, event) + assert state.closing_steps == (RecipeSetpointStep(address="dev:shutter", value=0.0),) + + +@pytest.mark.unit +def test_recipe_defined_defaults_closing_steps_to_empty() -> None: + state = evolve(None, _defined()) + assert state.closing_steps == () + + +@pytest.mark.unit +def test_recipe_versioned_replaces_closing_steps_wholesale() -> None: + rid = uuid4() + state = evolve( + None, + _defined(recipe_id=rid, closing_steps=(RecipeSetpointStep(address="dev:a", value=1.0),)), + ) + new_closing = (RecipeSetpointStep(address="dev:b", value=2.0),) + state2 = evolve( + state, + RecipeVersioned( + recipe_id=rid, + version_tag="v1", + steps=state.steps, + occurred_at=_NOW, + closing_steps=new_closing, + ), + ) + assert state2.closing_steps == new_closing + + +@pytest.mark.unit +def test_recipe_deprecated_preserves_closing_steps_for_audit() -> None: + rid = uuid4() + state = evolve( + None, + _defined(recipe_id=rid, closing_steps=(RecipeSetpointStep(address="dev:a", value=1.0),)), + ) + state2 = evolve(state, RecipeDeprecated(reason="Superseded", recipe_id=rid, occurred_at=_NOW)) + assert state2.closing_steps == state.closing_steps + + @pytest.mark.unit def test_recipe_deprecated_preserves_steps_and_capability_id_for_audit() -> None: rid, cid, succ = uuid4(), uuid4(), uuid4() diff --git a/apps/api/tools/gen_record_dispositions.py b/apps/api/tools/gen_record_dispositions.py index 8a0005b2efa..37ecfe56e1f 100644 --- a/apps/api/tools/gen_record_dispositions.py +++ b/apps/api/tools/gen_record_dispositions.py @@ -77,6 +77,11 @@ ("SealOnlineKeyRotated", "facility_code"): "facility_id", ("SealRepublishingStarted", "facility_code"): "facility_id", ("SealRepublishingCompleted", "facility_code"): "facility_id", + # `to_payload` writes the closing-steps list under the short wire key + # "closing" (matching the operation-BC vocabulary the design settled + # on: "closing steps", not "teardown"), not the dataclass field name. + ("RecipeDefined", "closing_steps"): "closing", + ("RecipeVersioned", "closing_steps"): "closing", } # A field's TYPE-DRIVEN disposition is occasionally the wrong call for From 99df6fa808e9ccd66553c6fbde39cd4de4d6bb0d Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:22:44 -0500 Subject: [PATCH 2/7] Let operators author, validate, and read back a recipe's closing steps DefineRecipe / VersionRecipe carry closing_steps end to end: REST + MCP request bodies, both deciders, both handlers. Both handlers validate the CONCATENATED main-plus-closing walk in one pass, not two separate passes -- validate_output_refs' one-sink rule asserts at end-of-call, so a second pass starting cold would falsely reject a valid chained-compute recipe. One walk also gives "closing may read a main capture, not the reverse" for free, with no new concept. get_recipe's REST response and MCP output gain closing_steps too, for read-side symmetry: an operator inspecting a recipe should see its closing list, not have to infer it. openapi.json regenerated (three additive properties). Verified by mutation: reverting either handler's concatenated walk back to steps-only reproduces exactly the gap a bad BindingRef in closing_steps alone would otherwise slip through. --- apps/api/openapi.json | 20 +++++++- .../recipe/features/define_recipe/command.py | 7 ++- .../recipe/features/define_recipe/decider.py | 2 + .../recipe/features/define_recipe/handler.py | 12 +++-- .../recipe/features/define_recipe/route.py | 10 ++++ .../recipe/features/define_recipe/tool.py | 15 ++++++ .../cora/recipe/features/get_recipe/route.py | 6 ++- .../cora/recipe/features/get_recipe/tool.py | 2 + .../recipe/features/version_recipe/command.py | 7 ++- .../recipe/features/version_recipe/decider.py | 2 + .../recipe/features/version_recipe/handler.py | 8 ++-- .../recipe/features/version_recipe/route.py | 8 ++++ .../recipe/features/version_recipe/tool.py | 14 ++++++ .../contract/test_get_recipe_endpoint.py | 26 +++++++++++ .../contract/test_get_recipe_mcp_tool.py | 18 ++++++++ .../unit/recipe/test_define_recipe_handler.py | 46 +++++++++++++++++++ .../recipe/test_version_recipe_handler.py | 38 +++++++++++++++ 17 files changed, 231 insertions(+), 10 deletions(-) diff --git a/apps/api/openapi.json b/apps/api/openapi.json index 57dd18eb922..033e1917f8d 100644 --- a/apps/api/openapi.json +++ b/apps/api/openapi.json @@ -6645,6 +6645,12 @@ "title": "Capability Id", "type": "string" }, + "closing_steps": { + "additionalProperties": true, + "description": "Optional wire-format closing-step sequence, same shape as `steps`: `{steps: [...]}`. The Conductor walks these after `steps` ends on a real terminal (Completed or Aborted), never on a Held pause. Empty by default; most recipes have none.", + "title": "Closing Steps", + "type": "object" + }, "name": { "description": "Display name for the new Recipe.", "maxLength": 200, @@ -11846,13 +11852,18 @@ "type": "string" }, "RecipeResponse": { - "description": "Read-side DTO at the API boundary.\n\nCarries primitives, not domain VOs. `status` is the StrEnum's\nstring value (Defined / Versioned / Deprecated). `version` is\nthe operator-supplied label of the most recent `version_recipe`\ncall (null until first version). `steps` is the wire-format\ndict (BindingRef sentinels serialize as `{__binding__: name}`).\n`replaced_by_recipe_id` is null on Defined / Versioned /\nDeprecated-without-replacement; populated when a deprecation\nsupplied a successor pointer. `created_at` / `versioned_at` /\n`deprecated_at` are projection-sourced lifecycle timestamps\n(Path C); see module docstring for null semantics.", + "description": "Read-side DTO at the API boundary.\n\nCarries primitives, not domain VOs. `status` is the StrEnum's\nstring value (Defined / Versioned / Deprecated). `version` is\nthe operator-supplied label of the most recent `version_recipe`\ncall (null until first version). `steps` is the wire-format\ndict (BindingRef sentinels serialize as `{__binding__: name}`).\n`replaced_by_recipe_id` is null on Defined / Versioned /\nDeprecated-without-replacement; populated when a deprecation\nsupplied a successor pointer. `created_at` / `versioned_at` /\n`deprecated_at` are projection-sourced lifecycle timestamps\n(Path C); see module docstring for null semantics. `closing_steps`\nmirrors `steps`' wire shape; empty for the common case of a recipe\nwith no closing steps.", "properties": { "capability_id": { "format": "uuid", "title": "Capability Id", "type": "string" }, + "closing_steps": { + "additionalProperties": true, + "title": "Closing Steps", + "type": "object" + }, "created_at": { "anyOf": [ { @@ -11939,6 +11950,7 @@ "status", "version", "steps", + "closing_steps", "replaced_by_recipe_id" ], "title": "RecipeResponse", @@ -17230,6 +17242,12 @@ "VersionRecipeRequest": { "description": "Body for `POST /recipes/{recipe_id}/version`.", "properties": { + "closing_steps": { + "additionalProperties": true, + "description": "Replacement closing-step sequence, same shape as `steps` (wholesale replace alongside it). Empty by default.", + "title": "Closing Steps", + "type": "object" + }, "steps": { "additionalProperties": true, "description": "Replacement step sequence for the new version (wholesale replace; the prior steps are dropped). BindingRef sentinels are re-validated against the CURRENT Capability.parameters_schema.", diff --git a/apps/api/src/cora/recipe/features/define_recipe/command.py b/apps/api/src/cora/recipe/features/define_recipe/command.py index 27b9bbeae35..33feabb8409 100644 --- a/apps/api/src/cora/recipe/features/define_recipe/command.py +++ b/apps/api/src/cora/recipe/features/define_recipe/command.py @@ -27,8 +27,13 @@ @dataclass(frozen=True) class DefineRecipe: - """Define a new Recipe against an existing Capability.""" + """Define a new Recipe against an existing Capability. + + `closing_steps` is additive and optional (default `()`); see + `Recipe.closing_steps`. + """ name: str capability_id: UUID steps: tuple[RecipeStep, ...] + closing_steps: tuple[RecipeStep, ...] = () diff --git a/apps/api/src/cora/recipe/features/define_recipe/decider.py b/apps/api/src/cora/recipe/features/define_recipe/decider.py index 0c50d7fa52c..3a0c012b278 100644 --- a/apps/api/src/cora/recipe/features/define_recipe/decider.py +++ b/apps/api/src/cora/recipe/features/define_recipe/decider.py @@ -50,6 +50,7 @@ def decide( name=name, capability_id=command.capability_id, steps=command.steps, + closing_steps=command.closing_steps, ) return [ RecipeDefined( @@ -58,5 +59,6 @@ def decide( capability_id=command.capability_id, steps=command.steps, occurred_at=now, + closing_steps=command.closing_steps, ) ] diff --git a/apps/api/src/cora/recipe/features/define_recipe/handler.py b/apps/api/src/cora/recipe/features/define_recipe/handler.py index 5369c5b0dee..c1634a7ec42 100644 --- a/apps/api/src/cora/recipe/features/define_recipe/handler.py +++ b/apps/api/src/cora/recipe/features/define_recipe/handler.py @@ -110,9 +110,15 @@ async def handler( capability = await load_capability(deps.event_store, command.capability_id) if capability is None: raise CapabilityNotFoundError(command.capability_id) - validate_recipe_steps_against_capability_schema(command.steps, capability.parameters_schema) - validate_capture_refs(command.steps) - validate_output_refs(command.steps) + # ONE concatenated walk, not two: `validate_output_refs`'s one-sink + # rule asserts at end-of-call, so validating closing_steps as a + # second pass starting cold would falsely reject a valid + # chained-compute recipe. This also gives "closing may read a main + # capture, not the reverse" for free, with no new concept. + all_steps = command.steps + command.closing_steps + validate_recipe_steps_against_capability_schema(all_steps, capability.parameters_schema) + validate_capture_refs(all_steps) + validate_output_refs(all_steps) new_id = deps.id_generator.new_id() now = deps.clock.now() diff --git a/apps/api/src/cora/recipe/features/define_recipe/route.py b/apps/api/src/cora/recipe/features/define_recipe/route.py index 730a8b4073b..7a76ddf20ce 100644 --- a/apps/api/src/cora/recipe/features/define_recipe/route.py +++ b/apps/api/src/cora/recipe/features/define_recipe/route.py @@ -44,6 +44,15 @@ class DefineRecipeRequest(BaseModel): "`{__binding__: name}` to reference a Capability parameter." ), ) + closing_steps: dict[str, Any] = Field( + default_factory=lambda: {"steps": []}, + description=( + "Optional wire-format closing-step sequence, same shape as " + "`steps`: `{steps: [...]}`. The Conductor walks these after " + "`steps` ends on a real terminal (Completed or Aborted), never " + "on a Held pause. Empty by default; most recipes have none." + ), + ) class DefineRecipeResponse(BaseModel): @@ -111,6 +120,7 @@ async def post_recipes( name=body.name, capability_id=body.capability_id, steps=steps_from_dict(body.steps), + closing_steps=steps_from_dict(body.closing_steps), ), principal_id=principal_id, correlation_id=cid, diff --git a/apps/api/src/cora/recipe/features/define_recipe/tool.py b/apps/api/src/cora/recipe/features/define_recipe/tool.py index e4e17138fe7..6d12ff6844b 100644 --- a/apps/api/src/cora/recipe/features/define_recipe/tool.py +++ b/apps/api/src/cora/recipe/features/define_recipe/tool.py @@ -64,6 +64,18 @@ async def define_recipe_tool( # pyright: ignore[reportUnusedFunction] ), ), ], + closing_steps: Annotated[ + dict[str, Any] | None, + Field( + default=None, + description=( + "Optional closing-step sequence, same shape as `steps`. " + "The Conductor walks these after `steps` ends on a real " + "terminal (Completed or Aborted), never on a Held pause. " + "Omit or pass None for no closing steps (the common case)." + ), + ), + ] = None, ) -> DefineRecipeOutput: handler = get_handler() recipe_id = await handler( @@ -71,6 +83,9 @@ async def define_recipe_tool( # pyright: ignore[reportUnusedFunction] name=name, capability_id=capability_id, steps=steps_from_dict(steps), + closing_steps=steps_from_dict( + closing_steps if closing_steps is not None else {"steps": []} + ), ), principal_id=get_mcp_principal_id(ctx), correlation_id=current_correlation_id(), diff --git a/apps/api/src/cora/recipe/features/get_recipe/route.py b/apps/api/src/cora/recipe/features/get_recipe/route.py index 0b7dbb9f6ab..a9337df948d 100644 --- a/apps/api/src/cora/recipe/features/get_recipe/route.py +++ b/apps/api/src/cora/recipe/features/get_recipe/route.py @@ -46,7 +46,9 @@ class RecipeResponse(BaseModel): Deprecated-without-replacement; populated when a deprecation supplied a successor pointer. `created_at` / `versioned_at` / `deprecated_at` are projection-sourced lifecycle timestamps - (Path C); see module docstring for null semantics. + (Path C); see module docstring for null semantics. `closing_steps` + mirrors `steps`' wire shape; empty for the common case of a recipe + with no closing steps. """ id: UUID @@ -55,6 +57,7 @@ class RecipeResponse(BaseModel): status: str version: str | None steps: dict[str, Any] + closing_steps: dict[str, Any] replaced_by_recipe_id: UUID | None created_at: datetime | None = None versioned_at: datetime | None = None @@ -111,6 +114,7 @@ async def get_recipes( status=recipe.status.value, version=recipe.version, steps=steps_to_dict(recipe.steps), + closing_steps=steps_to_dict(recipe.closing_steps), replaced_by_recipe_id=recipe.replaced_by_recipe_id, created_at=timestamps.created_at if timestamps is not None else None, versioned_at=timestamps.versioned_at if timestamps is not None else None, diff --git a/apps/api/src/cora/recipe/features/get_recipe/tool.py b/apps/api/src/cora/recipe/features/get_recipe/tool.py index e3a795ad3fa..ff3109ff406 100644 --- a/apps/api/src/cora/recipe/features/get_recipe/tool.py +++ b/apps/api/src/cora/recipe/features/get_recipe/tool.py @@ -34,6 +34,7 @@ class RecipeOutput(BaseModel): status: str version: str | None steps: dict[str, Any] + closing_steps: dict[str, Any] replaced_by_recipe_id: UUID | None created_at: datetime | None = None versioned_at: datetime | None = None @@ -73,6 +74,7 @@ async def get_recipe_tool( # pyright: ignore[reportUnusedFunction] status=recipe.status.value, version=recipe.version, steps=steps_to_dict(recipe.steps), + closing_steps=steps_to_dict(recipe.closing_steps), replaced_by_recipe_id=recipe.replaced_by_recipe_id, created_at=timestamps.created_at if timestamps is not None else None, versioned_at=timestamps.versioned_at if timestamps is not None else None, diff --git a/apps/api/src/cora/recipe/features/version_recipe/command.py b/apps/api/src/cora/recipe/features/version_recipe/command.py index 2390b3464d4..6297c6bba2e 100644 --- a/apps/api/src/cora/recipe/features/version_recipe/command.py +++ b/apps/api/src/cora/recipe/features/version_recipe/command.py @@ -19,8 +19,13 @@ @dataclass(frozen=True) class VersionRecipe: - """Issue a new version label + replacement steps for an existing Recipe.""" + """Issue a new version label + replacement steps for an existing Recipe. + + `closing_steps` is additive and optional (default `()`); it + REPLACES wholesale alongside `steps`. See `Recipe.closing_steps`. + """ recipe_id: UUID version_tag: str steps: tuple[RecipeStep, ...] + closing_steps: tuple[RecipeStep, ...] = () diff --git a/apps/api/src/cora/recipe/features/version_recipe/decider.py b/apps/api/src/cora/recipe/features/version_recipe/decider.py index 0d7eeffb80a..5c0bb701696 100644 --- a/apps/api/src/cora/recipe/features/version_recipe/decider.py +++ b/apps/api/src/cora/recipe/features/version_recipe/decider.py @@ -61,6 +61,7 @@ def decide( name=state.name, capability_id=state.capability_id, steps=command.steps, + closing_steps=command.closing_steps, ) return [ RecipeVersioned( @@ -68,5 +69,6 @@ def decide( version_tag=trimmed, steps=command.steps, occurred_at=now, + closing_steps=command.closing_steps, ) ] diff --git a/apps/api/src/cora/recipe/features/version_recipe/handler.py b/apps/api/src/cora/recipe/features/version_recipe/handler.py index 8bc540602f8..3f53717dfe7 100644 --- a/apps/api/src/cora/recipe/features/version_recipe/handler.py +++ b/apps/api/src/cora/recipe/features/version_recipe/handler.py @@ -116,9 +116,11 @@ async def handler( capability = await load_capability(deps.event_store, state.capability_id) if capability is None: raise CapabilityNotFoundError(state.capability_id) - validate_recipe_steps_against_capability_schema(command.steps, capability.parameters_schema) - validate_capture_refs(command.steps) - validate_output_refs(command.steps) + # ONE concatenated walk; see define_recipe/handler.py for why not two. + all_steps = command.steps + command.closing_steps + validate_recipe_steps_against_capability_schema(all_steps, capability.parameters_schema) + validate_capture_refs(all_steps) + validate_output_refs(all_steps) domain_events = decide(state=state, command=command, now=now) diff --git a/apps/api/src/cora/recipe/features/version_recipe/route.py b/apps/api/src/cora/recipe/features/version_recipe/route.py index 7d39b596df6..e304861a7ac 100644 --- a/apps/api/src/cora/recipe/features/version_recipe/route.py +++ b/apps/api/src/cora/recipe/features/version_recipe/route.py @@ -42,6 +42,13 @@ class VersionRecipeRequest(BaseModel): "are re-validated against the CURRENT Capability.parameters_schema." ), ) + closing_steps: dict[str, Any] = Field( + default_factory=lambda: {"steps": []}, + description=( + "Replacement closing-step sequence, same shape as `steps` " + "(wholesale replace alongside it). Empty by default." + ), + ) def _get_handler(request: Request) -> Handler: @@ -99,6 +106,7 @@ async def post_recipes_version( recipe_id=recipe_id, version_tag=body.version_tag, steps=steps_from_dict(body.steps), + closing_steps=steps_from_dict(body.closing_steps), ), principal_id=principal_id, correlation_id=cid, diff --git a/apps/api/src/cora/recipe/features/version_recipe/tool.py b/apps/api/src/cora/recipe/features/version_recipe/tool.py index 8dc2c6da6a8..74fcb4ee5f9 100644 --- a/apps/api/src/cora/recipe/features/version_recipe/tool.py +++ b/apps/api/src/cora/recipe/features/version_recipe/tool.py @@ -63,6 +63,17 @@ async def version_recipe_tool( # pyright: ignore[reportUnusedFunction] ), ), ], + closing_steps: Annotated[ + dict[str, Any] | None, + Field( + default=None, + description=( + "Optional replacement closing-step sequence, same shape " + "as `steps` (wholesale replace alongside it). Omit or " + "pass None for no closing steps." + ), + ), + ] = None, ) -> VersionRecipeOutput: handler = get_handler() await handler( @@ -70,6 +81,9 @@ async def version_recipe_tool( # pyright: ignore[reportUnusedFunction] recipe_id=recipe_id, version_tag=version_tag, steps=steps_from_dict(steps), + closing_steps=steps_from_dict( + closing_steps if closing_steps is not None else {"steps": []} + ), ), principal_id=get_mcp_principal_id(ctx), correlation_id=current_correlation_id(), diff --git a/apps/api/tests/contract/test_get_recipe_endpoint.py b/apps/api/tests/contract/test_get_recipe_endpoint.py index d1d27cc1411..5c4505fdaa9 100644 --- a/apps/api/tests/contract/test_get_recipe_endpoint.py +++ b/apps/api/tests/contract/test_get_recipe_endpoint.py @@ -44,6 +44,32 @@ def test_get_recipe_200_returns_full_recipe_response() -> None: assert body["steps"]["steps"][0]["kind"] == "setpoint" +@pytest.mark.contract +def test_get_recipe_surfaces_closing_steps() -> None: + with TestClient(create_app()) as client: + cap = client.post("/capabilities", json=_capability()).json() + body = _recipe_for(cap["capability_id"]) + body["closing_steps"] = { + "steps": [ + {"kind": "setpoint", "address": "dev:shutter", "value": 0.0, "verify": False}, + ], + } + recipe = client.post("/recipes", json=body).json() + response = client.get(f"/recipes/{recipe['recipe_id']}") + assert response.status_code == 200 + resp_body = response.json() + assert resp_body["closing_steps"]["steps"][0]["address"] == "dev:shutter" + + +@pytest.mark.contract +def test_get_recipe_defaults_closing_steps_to_empty() -> None: + with TestClient(create_app()) as client: + cap = client.post("/capabilities", json=_capability()).json() + recipe = client.post("/recipes", json=_recipe_for(cap["capability_id"])).json() + response = client.get(f"/recipes/{recipe['recipe_id']}") + assert response.json()["closing_steps"]["steps"] == [] + + @pytest.mark.contract def test_get_recipe_404_when_recipe_missing() -> None: with TestClient(create_app()) as client: diff --git a/apps/api/tests/contract/test_get_recipe_mcp_tool.py b/apps/api/tests/contract/test_get_recipe_mcp_tool.py index 9d5697b49f7..4653fe45f14 100644 --- a/apps/api/tests/contract/test_get_recipe_mcp_tool.py +++ b/apps/api/tests/contract/test_get_recipe_mcp_tool.py @@ -78,3 +78,21 @@ def test_mcp_get_recipe_tool_returns_structured_recipe_state() -> None: assert body["id"] == recipe_id assert body["status"] == "Defined" assert body["capability_id"] == capability_id + + +@pytest.mark.contract +def test_mcp_get_recipe_tool_surfaces_closing_steps_defined_via_mcp() -> None: + with TestClient(create_app()) as client: + session_headers = open_session(client) + cap_result = _call_tool(client, session_headers, "define_capability", _capability_args(), 2) + capability_id = cap_result["structuredContent"]["capability_id"] + args = _recipe_args(capability_id) + args["closing_steps"] = { + "steps": [{"kind": "setpoint", "address": "dev:shutter", "value": 0.0}], + } + recipe_result = _call_tool(client, session_headers, "define_recipe", args, 3) + recipe_id = recipe_result["structuredContent"]["recipe_id"] + result = _call_tool(client, session_headers, "get_recipe", {"recipe_id": recipe_id}, 4) + assert result["isError"] is False + body = result["structuredContent"] + assert body["closing_steps"]["steps"][0]["address"] == "dev:shutter" diff --git a/apps/api/tests/unit/recipe/test_define_recipe_handler.py b/apps/api/tests/unit/recipe/test_define_recipe_handler.py index a8533fff57c..974c1998029 100644 --- a/apps/api/tests/unit/recipe/test_define_recipe_handler.py +++ b/apps/api/tests/unit/recipe/test_define_recipe_handler.py @@ -192,6 +192,52 @@ async def test_handler_raises_binding_unknown_parameter_when_schema_missing_key( ) +@pytest.mark.unit +async def test_handler_raises_binding_unknown_parameter_for_a_bad_ref_in_closing_steps() -> None: + """A bad BindingRef in closing_steps must be caught too, not just in steps + -- proving the handler validates the CONCATENATED walk, not steps alone.""" + _, deps = await _build_seeded_deps( + parameters_schema={"type": "object", "properties": {"angle": {"type": "number"}}} + ) + handler = define_recipe.bind(deps) + + with pytest.raises(RecipeBindingReferencesUnknownParameterError): + await handler( + DefineRecipe( + name="R", + capability_id=_CAPABILITY_ID, + steps=(RecipeSetpointStep(address="dev:x", value=BindingRef("angle")),), + closing_steps=( + RecipeSetpointStep(address="dev:shutter", value=BindingRef("enrgy")), + ), + ), + principal_id=_PRINCIPAL_ID, + correlation_id=_CORRELATION_ID, + ) + + +@pytest.mark.unit +async def test_handler_persists_closing_steps_on_the_recipe() -> None: + store, deps = await _build_seeded_deps() + handler = define_recipe.bind(deps) + + recipe_id = await handler( + DefineRecipe( + name="R", + capability_id=_CAPABILITY_ID, + steps=(RecipeSetpointStep(address="dev:x", value=1.0),), + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ), + principal_id=_PRINCIPAL_ID, + correlation_id=_CORRELATION_ID, + ) + assert recipe_id == _NEW_ID + recipe_events, _ = await store.load("Recipe", _NEW_ID) + assert recipe_events[0].payload["closing"]["steps"] == [ + {"kind": "setpoint", "address": "dev:shutter", "value": 0.0, "verify": False} + ] + + @pytest.mark.unit async def test_handler_raises_empty_recipe_steps_when_command_steps_empty() -> None: _, deps = await _build_seeded_deps() diff --git a/apps/api/tests/unit/recipe/test_version_recipe_handler.py b/apps/api/tests/unit/recipe/test_version_recipe_handler.py index 555018904c8..05130614b2e 100644 --- a/apps/api/tests/unit/recipe/test_version_recipe_handler.py +++ b/apps/api/tests/unit/recipe/test_version_recipe_handler.py @@ -86,6 +86,28 @@ async def test_handler_appends_recipe_versioned_event() -> None: assert events[1].payload["version_tag"] == "v1" +@pytest.mark.unit +async def test_handler_persists_closing_steps_on_the_new_version() -> None: + store, deps = await _build_seeded_deps() + handler = version_recipe.bind(deps) + + await handler( + VersionRecipe( + recipe_id=_RECIPE_ID, + version_tag="v1", + steps=(RecipeSetpointStep(address="dev:x", value=2.0),), + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ), + principal_id=_PRINCIPAL_ID, + correlation_id=_CORRELATION_ID, + ) + + events, _ = await store.load("Recipe", _RECIPE_ID) + assert events[1].payload["closing"]["steps"] == [ + {"kind": "setpoint", "address": "dev:shutter", "value": 0.0, "verify": False} + ] + + @pytest.mark.unit async def test_handler_raises_unauthorized_on_deny() -> None: _, deps = await _build_seeded_deps(deny=True) @@ -180,3 +202,19 @@ async def test_handler_re_validates_binding_refs_against_capability_schema() -> principal_id=_PRINCIPAL_ID, correlation_id=_CORRELATION_ID, ) + + # A bad ref in closing_steps alone (main steps are valid) must ALSO + # raise -- proving the handler validates the CONCATENATED walk. + with pytest.raises(RecipeBindingReferencesUnknownParameterError): + await handler( + VersionRecipe( + recipe_id=_RECIPE_ID, + version_tag="v2", + steps=(RecipeSetpointStep(address="dev:x", value=BindingRef("angle")),), + closing_steps=( + RecipeSetpointStep(address="dev:shutter", value=BindingRef("enrgy")), + ), + ), + principal_id=_PRINCIPAL_ID, + correlation_id=_CORRELATION_ID, + ) From 0712b7ec708f5944e76db22d4aa9cead624e0a17 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sun, 30 Aug 2026 03:52:58 -0500 Subject: [PATCH 3/7] Expand, hash, and pin closing steps alongside the main list Registration (register_procedure_from_recipe) now expands recipe.closing_steps through the same determinism gate as recipe.steps: overflow and the double-expand comparison both range over the combined count, and steps_hash becomes ONE pin over main-plus-closing (a new steps_to_wire_with_closing composing helper tags closing entries; empty closing_steps reproduces today's hash byte-for-byte, so no existing pinned expansion is invalidated). Mid-conduct replay (_conduct_preparation._re_expand_steps) re-expands and verifies the same combined hash, then resolve_and_pin_conduct_steps runs pseudoaxis expansion over the closing list too and pins the result onto a new ResolvedStepsRecorded.resolved_closing_steps field -- kept SEPARATE from resolved_steps, not flattened, so an operator- supplied conduct_from boundary can never land inside the closing region. The four call sites (conduct_procedure, conduct_or_hold_procedure, conduct_until_converged, conduct_until_advised) now receive a (steps, closing_steps) pair; closing_steps is intentionally unused past this commit; _run_closing is what consumes it. Disposition table regenerated for the new field. Verified by mutation: reverting the overflow/determinism/hash composition, or the mid-conduct re-expansion, each reproduces a real gap the corresponding new test catches. --- .../record_export/_dispositions.py | 1 + .../cora/operation/_conduct_preparation.py | 49 +++-- .../operation/_recipe_expansion/__init__.py | 5 +- .../operation/_recipe_expansion/_expand.py | 25 ++- .../operation/_recipe_expansion/_replay.py | 14 +- .../operation/aggregates/procedure/events.py | 15 ++ .../conduct_or_hold_procedure/handler.py | 5 +- .../features/conduct_procedure/handler.py | 5 +- .../features/conduct_until_advised/handler.py | 6 +- .../conduct_until_converged/handler.py | 6 +- .../register_procedure_from_recipe/decider.py | 31 ++-- .../test_conduct_procedure_handler.py | 75 +++++++- .../operation/test_record_resolved_steps.py | 30 +++ ..._register_procedure_from_recipe_decider.py | 171 +++++++++++++++++- 14 files changed, 400 insertions(+), 38 deletions(-) diff --git a/apps/api/src/cora/infrastructure/record_export/_dispositions.py b/apps/api/src/cora/infrastructure/record_export/_dispositions.py index fca5510928f..64eca74b4ae 100644 --- a/apps/api/src/cora/infrastructure/record_export/_dispositions.py +++ b/apps/api/src/cora/infrastructure/record_export/_dispositions.py @@ -1584,6 +1584,7 @@ "ResolvedStepsRecorded": { "occurred_at": "keep:time", "procedure_id": "token:uuid", + "resolved_closing_steps": "drop:opaque", "resolved_steps": "drop:opaque", "step_count": "keep:number", }, diff --git a/apps/api/src/cora/operation/_conduct_preparation.py b/apps/api/src/cora/operation/_conduct_preparation.py index d68e692dc72..afe450e8608 100644 --- a/apps/api/src/cora/operation/_conduct_preparation.py +++ b/apps/api/src/cora/operation/_conduct_preparation.py @@ -66,6 +66,7 @@ def decide_resolved_steps_recorded( resolved_steps: Sequence[Mapping[str, Any]], *, now: datetime, + resolved_closing_steps: Sequence[Mapping[str, Any]] = (), ) -> list[ResolvedStepsRecorded]: """Pin the resolved step list iff the Procedure is pre-conduct (Defined). @@ -77,16 +78,22 @@ def decide_resolved_steps_recorded( normal lifecycle failure, preserving the conduct route's failures-in-body contract instead of raising a fresh HTTP error here. Kept as a pure function so the decision is unit-testable without an event store. + + `resolved_closing_steps` is the SAME resolution applied to the Recipe's + closing steps, pinned separately (not flattened into `resolved_steps`) + so a resume boundary can never land inside the closing region. """ if state is None or state.status is not ProcedureStatus.DEFINED: return [] steps = tuple(dict(step) for step in resolved_steps) + closing_steps = tuple(dict(step) for step in resolved_closing_steps) return [ ResolvedStepsRecorded( procedure_id=state.id, resolved_steps=steps, - step_count=len(steps), + step_count=len(steps) + len(closing_steps), occurred_at=now, + resolved_closing_steps=closing_steps, ) ] @@ -102,16 +109,22 @@ async def resolve_and_pin_conduct_steps( principal_id: UUID, correlation_id: UUID, causation_id: UUID | None, -) -> tuple[Step, ...]: +) -> tuple[tuple[Step, ...], tuple[Step, ...]]: """Resolve the final conduct step list + pin it as `ResolvedStepsRecorded`. The shared pre-Conductor work for `conduct` / `conduct_or_hold`: recipe re-expansion (recipe-driven Procedures) -> pseudoaxis constituent - expansion (Run-phase Procedures) -> pin. Returns the resolved steps to - hand to the Conductor. `command_name` rides the pinned event's metadata. + expansion (Run-phase Procedures) -> pin. Returns `(steps, closing_steps)` + to hand to the Conductor. `command_name` rides the pinned event's + metadata. + + A legacy (non-recipe-driven) Procedure has no closing steps: `caller_steps` + is an inline list with no separate closing half, so `closing_steps` is + always `()` on that path. """ + closing_steps: tuple[Step, ...] = () if procedure.recipe_id is not None: - steps = await _re_expand_steps( + steps, closing_steps = await _re_expand_steps( procedure_id=procedure.id, recipe_id=procedure.recipe_id, caller_steps=caller_steps, @@ -146,13 +159,21 @@ def _resolve_constituents(asset_id: UUID) -> tuple[UUID, ...]: # Pre-Conductor PseudoAxis expansion: rewrite any virtual-axis SetpointStep # into N sequential constituent SetpointSteps so the Conductor's dispatch # loop walks the constituents in declared order. ActionStep / CheckStep - # pass through unchanged ([[project-pseudoaxis-design]] v3). + # pass through unchanged ([[project-pseudoaxis-design]] v3). Closing steps + # get the SAME expansion, or a pseudoaxis closing setpoint would reach the + # Conductor unresolved. steps = await expansion_port.expand_pseudoaxis( steps, event_store=deps.event_store, correlation_id=correlation_id, constituent_resolver=constituent_resolver, ) + closing_steps = await expansion_port.expand_pseudoaxis( + closing_steps, + event_store=deps.event_store, + correlation_id=correlation_id, + constituent_resolver=constituent_resolver, + ) # Pin the resolved step list (after recipe + pseudoaxis expansion) BEFORE # conducting, so a future resume replays this exact list. The helper emits @@ -163,6 +184,7 @@ def _resolve_constituents(asset_id: UUID) -> tuple[UUID, ...]: procedure, tuple(step_to_payload(step) for step in steps), now=deps.clock.now(), + resolved_closing_steps=tuple(step_to_payload(step) for step in closing_steps), ) if resolved_steps_events: _, current_version = await deps.event_store.load( @@ -187,7 +209,7 @@ def _resolve_constituents(asset_id: UUID) -> tuple[UUID, ...]: ], ) - return steps + return steps, closing_steps async def _re_expand_steps( @@ -198,7 +220,7 @@ async def _re_expand_steps( stored_events: list[StoredEvent], event_store: EventStore, expansion_port: RecipeExpander, -) -> tuple[Step, ...]: +) -> tuple[tuple[Step, ...], tuple[Step, ...]]: """Run the recipe-replay gate per [[project-run-procedure-replay-design]]. Six steps: reject non-empty caller steps -> find_recipe_expansion_record @@ -209,7 +231,8 @@ async def _re_expand_steps( propagates from helper) -> load_capability + reject Deprecated (raise ProcedureBoundCapabilityDeprecatedError, symmetric to start_run's RunBoundPlanDeprecatedError) -> verify_bindings_hash -> - expand -> verify_steps_hash -> return the re-expanded tuple. + expand (both `recipe.steps` and `recipe.closing_steps`) -> + verify_steps_hash (one combined pin) -> return `(steps, closing_steps)`. """ if list(caller_steps): raise ProcedureStepsForbiddenForRecipeDrivenError(procedure_id) @@ -244,6 +267,8 @@ async def _re_expand_steps( raise ProcedureBoundCapabilityDeprecatedError(procedure_id, recipe.capability_id) verify_bindings_hash(procedure_id, pins) - expanded = expansion_port.expand(recipe.steps, dict(pins.bindings)) - verify_steps_hash(procedure_id, expanded, pins) - return expanded + bindings_dict = dict(pins.bindings) + expanded = expansion_port.expand(recipe.steps, bindings_dict) + expanded_closing = expansion_port.expand(recipe.closing_steps, bindings_dict) + verify_steps_hash(procedure_id, expanded, pins, closing_steps=expanded_closing) + return expanded, expanded_closing diff --git a/apps/api/src/cora/operation/_recipe_expansion/__init__.py b/apps/api/src/cora/operation/_recipe_expansion/__init__.py index 501ba47062b..970e5ededca 100644 --- a/apps/api/src/cora/operation/_recipe_expansion/__init__.py +++ b/apps/api/src/cora/operation/_recipe_expansion/__init__.py @@ -7,7 +7,8 @@ that replay / verify that resolution: - `_expand`: the pure `expand(steps, bindings) -> Step list` substitution kernel - (+ `steps_to_wire` / `canonical_json_bytes` for provenance hashing). + (+ `steps_to_wire` / `steps_to_wire_with_closing` / `canonical_json_bytes` + for provenance hashing). - `_replay`: locate + verify the genesis `RecipeExpansionRecorded` provenance on the `conduct_procedure` path (re-expand and compare pinned hashes). - `_resolved_steps_replay`: locate the pinned `ResolvedStepsRecorded` provenance @@ -21,6 +22,7 @@ canonical_json_bytes, expand, steps_to_wire, + steps_to_wire_with_closing, ) from cora.operation._recipe_expansion._replay import ( MismatchField, @@ -43,6 +45,7 @@ "find_resolved_steps_record", "pins_from_payload", "steps_to_wire", + "steps_to_wire_with_closing", "verify_bindings_hash", "verify_steps_hash", ] diff --git a/apps/api/src/cora/operation/_recipe_expansion/_expand.py b/apps/api/src/cora/operation/_recipe_expansion/_expand.py index 9928410f668..00a254f0d7e 100644 --- a/apps/api/src/cora/operation/_recipe_expansion/_expand.py +++ b/apps/api/src/cora/operation/_recipe_expansion/_expand.py @@ -229,4 +229,27 @@ def steps_to_wire(steps: tuple[Step, ...]) -> list[dict[str, Any]]: return [_step_to_wire(step) for step in steps] -__all__ = ["canonical_json_bytes", "expand", "steps_to_wire"] +def steps_to_wire_with_closing( + steps: tuple[Step, ...], closing_steps: tuple[Step, ...] +) -> list[dict[str, Any]]: + """Canonical hash form for main + closing steps COMBINED, one pin. + + Closing entries are tagged `"closing": true` by THIS composing + function, not inside `_step_to_wire` (which receives a bare `Step` + with no partition context). An empty `closing_steps` returns + EXACTLY `steps_to_wire(steps)` with nothing appended, so every + recipe that has never used closing steps keeps its existing + `steps_hash` byte-identical -- same precedent as `CheckStep.timeout_s` + being emitted only when set (see + `test_a_deadlineless_check_hashes_the_same_as_it_always_did`). + """ + main = steps_to_wire(steps) + if not closing_steps: + return main + closing = steps_to_wire(closing_steps) + for entry in closing: + entry["closing"] = True + return main + closing + + +__all__ = ["canonical_json_bytes", "expand", "steps_to_wire", "steps_to_wire_with_closing"] diff --git a/apps/api/src/cora/operation/_recipe_expansion/_replay.py b/apps/api/src/cora/operation/_recipe_expansion/_replay.py index 14bd64666cc..2c548d17cf1 100644 --- a/apps/api/src/cora/operation/_recipe_expansion/_replay.py +++ b/apps/api/src/cora/operation/_recipe_expansion/_replay.py @@ -24,7 +24,7 @@ from uuid import UUID from cora.infrastructure.ports.event_store import StoredEvent -from cora.operation._recipe_expansion._expand import steps_to_wire +from cora.operation._recipe_expansion._expand import steps_to_wire_with_closing from cora.operation.aggregates.procedure import ( RecipeExpansionRecordNotFoundError, RecipeExpansionReplayMismatchError, @@ -121,6 +121,8 @@ def verify_steps_hash( procedure_id: UUID, steps: tuple[Step, ...], pins: RecipeExpansionPins, + *, + closing_steps: tuple[Step, ...] = (), ) -> None: """Verify the re-expanded steps still hash to the recorded `steps_hash`. @@ -129,8 +131,16 @@ def verify_steps_hash( produces different output for the same input than at write time); runs AFTER `verify_bindings_hash` because steps drift downstream of bindings is a confusing diagnostic. + + `closing_steps` composes into the SAME pinned hash (one pin, no + aliasing): `steps_hash` was written over `steps_to_wire_with_closing` + at registration time, so replay must recompute the same combined + form. Defaults to `()`, which reproduces the pre-closing-steps hash + exactly for every recipe that has never used the field. """ - recomputed = hashlib.sha256(canonical_json_bytes(steps_to_wire(steps))).hexdigest() + recomputed = hashlib.sha256( + canonical_json_bytes(steps_to_wire_with_closing(steps, closing_steps)) + ).hexdigest() if recomputed != pins.steps_hash: raise RecipeExpansionReplayMismatchError(procedure_id, "steps") diff --git a/apps/api/src/cora/operation/aggregates/procedure/events.py b/apps/api/src/cora/operation/aggregates/procedure/events.py index c90a8e6dbc8..6b74119a05a 100644 --- a/apps/api/src/cora/operation/aggregates/procedure/events.py +++ b/apps/api/src/cora/operation/aggregates/procedure/events.py @@ -590,6 +590,13 @@ class ResolvedStepsRecorded: rewrite. `step_count` is a denorm for cheap read-side checks, mirror of `RecipeExpansionRecorded.step_count`. + `resolved_closing_steps` is the SAME resolution applied to the + Recipe's `closing_steps`, kept in a SEPARATE field rather than + flattened onto `resolved_steps`: a flat pin would let an + operator-supplied `conduct_from` boundary land inside the closing + region, and `execute_from` would replay closing steps as main steps + at absolute indices. Empty by default (additive, optional). + Provenance-only: the evolver leaves Procedure state unchanged when this event arrives (mirrors `RecipeExpansionRecorded`). """ @@ -598,6 +605,7 @@ class ResolvedStepsRecorded: resolved_steps: tuple[Mapping[str, Any], ...] step_count: int occurred_at: datetime + resolved_closing_steps: tuple[Mapping[str, Any], ...] = () # Discriminated union of every event the Procedure aggregate emits. @@ -882,12 +890,14 @@ def to_payload(event: ProcedureEvent) -> dict[str, Any]: resolved_steps=resolved_steps, step_count=step_count, occurred_at=occurred_at, + resolved_closing_steps=resolved_closing_steps, ): return { "procedure_id": str(procedure_id), "resolved_steps": [dict(step) for step in resolved_steps], "step_count": step_count, "occurred_at": occurred_at.isoformat(), + "resolved_closing_steps": [dict(step) for step in resolved_closing_steps], } case _: # pragma: no cover # exhaustiveness guard assert_never(event) @@ -1143,6 +1153,11 @@ def _build_resumed() -> ProcedureResumed: resolved_steps=tuple(dict(step) for step in payload["resolved_steps"]), step_count=int(payload["step_count"]), occurred_at=datetime.fromisoformat(payload["occurred_at"]), + # `.get`, not `["resolved_closing_steps"]`: a stored event + # from before closing steps existed has no such key. + resolved_closing_steps=tuple( + dict(step) for step in payload.get("resolved_closing_steps", []) + ), ), ) case _: diff --git a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py index eeceb1a166d..d629b472cc9 100644 --- a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py +++ b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py @@ -121,7 +121,10 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - steps = await resolve_and_pin_conduct_steps( + # `_closing_steps`: resolved and pinned onto ResolvedStepsRecorded + # above, but not yet handed to the Conductor -- that lands with + # _run_closing. See [[project_conduct_closing_steps_design]]. + steps, _closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, diff --git a/apps/api/src/cora/operation/features/conduct_procedure/handler.py b/apps/api/src/cora/operation/features/conduct_procedure/handler.py index e8aebd2d0ae..e31d795a68f 100644 --- a/apps/api/src/cora/operation/features/conduct_procedure/handler.py +++ b/apps/api/src/cora/operation/features/conduct_procedure/handler.py @@ -139,7 +139,10 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - steps = await resolve_and_pin_conduct_steps( + # `_closing_steps`: resolved and pinned onto ResolvedStepsRecorded + # above, but not yet handed to the Conductor -- that lands with + # _run_closing. See [[project_conduct_closing_steps_design]]. + steps, _closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, diff --git a/apps/api/src/cora/operation/features/conduct_until_advised/handler.py b/apps/api/src/cora/operation/features/conduct_until_advised/handler.py index ee8f75c9b72..8918ec4b213 100644 --- a/apps/api/src/cora/operation/features/conduct_until_advised/handler.py +++ b/apps/api/src/cora/operation/features/conduct_until_advised/handler.py @@ -134,7 +134,11 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - steps = await resolve_and_pin_conduct_steps( + # `_closing_steps` is unused here by design: this loop refuses a + # closing-bearing Recipe outright (v1 scope; see + # [[project_conduct_closing_steps_design]]), so it never reaches a + # non-empty value past that guard. + steps, _closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, diff --git a/apps/api/src/cora/operation/features/conduct_until_converged/handler.py b/apps/api/src/cora/operation/features/conduct_until_converged/handler.py index c788a40b207..877ba49cf97 100644 --- a/apps/api/src/cora/operation/features/conduct_until_converged/handler.py +++ b/apps/api/src/cora/operation/features/conduct_until_converged/handler.py @@ -127,7 +127,11 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - steps = await resolve_and_pin_conduct_steps( + # `_closing_steps` is unused here by design: this loop refuses a + # closing-bearing Recipe outright (v1 scope; see + # [[project_conduct_closing_steps_design]]), so it never reaches a + # non-empty value past that guard. + steps, _closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, diff --git a/apps/api/src/cora/operation/features/register_procedure_from_recipe/decider.py b/apps/api/src/cora/operation/features/register_procedure_from_recipe/decider.py index f2b6ee21003..b22adfd91a7 100644 --- a/apps/api/src/cora/operation/features/register_procedure_from_recipe/decider.py +++ b/apps/api/src/cora/operation/features/register_procedure_from_recipe/decider.py @@ -50,7 +50,7 @@ from typing import Any from uuid import UUID -from cora.operation._recipe_expansion import canonical_json_bytes, steps_to_wire +from cora.operation._recipe_expansion import canonical_json_bytes, steps_to_wire_with_closing from cora.operation.aggregates.procedure import ( PROCEDURE_KIND_MAX_LENGTH, RECIPE_EXPANSION_STEP_MAX, @@ -78,15 +78,19 @@ from cora.shared.json_schema_validation import validate_values_against_schema -def _hash_steps(steps: tuple[Step, ...]) -> str: +def _hash_steps(steps: tuple[Step, ...], closing_steps: tuple[Step, ...]) -> str: """Content-address the expanded Step tuple per memo §RecipeExpansionRecorded. Hashing the expanded steps (not the unexpanded Recipe template) pins what the Conductor will actually execute, so a Recipe re-version that produces equivalent expanded steps still hashes - identically. + identically. One combined pin over main + closing (no aliasing): + an empty `closing_steps` reproduces the pre-closing-steps hash + byte-for-byte, so no existing pinned expansion is invalidated. """ - return hashlib.sha256(canonical_json_bytes(steps_to_wire(steps))).hexdigest() + return hashlib.sha256( + canonical_json_bytes(steps_to_wire_with_closing(steps, closing_steps)) + ).hexdigest() def _hash_bindings(bindings: Mapping[str, Any]) -> str: @@ -136,16 +140,17 @@ def decide( raise InvalidProcedureIterationCapError(cap) steps_first = expansion_port.expand(recipe.steps, bindings_dict) - if len(steps_first) > RECIPE_EXPANSION_STEP_MAX: - raise RecipeExpansionOverflowError( - step_count=len(steps_first), cap=RECIPE_EXPANSION_STEP_MAX - ) + closing_first = expansion_port.expand(recipe.closing_steps, bindings_dict) + total_count = len(steps_first) + len(closing_first) + if total_count > RECIPE_EXPANSION_STEP_MAX: + raise RecipeExpansionOverflowError(step_count=total_count, cap=RECIPE_EXPANSION_STEP_MAX) - # Determinism check: re-expand and compare. The port wraps a pure - # function; any divergence is a server-side bug in the port or the + # Determinism check: re-expand both lists and compare. The port wraps a + # pure function; any divergence is a server-side bug in the port or the # recipe body. steps_second = expansion_port.expand(recipe.steps, bindings_dict) - if steps_first != steps_second: + closing_second = expansion_port.expand(recipe.closing_steps, bindings_dict) + if steps_first != steps_second or closing_first != closing_second: raise RecipeExpansionDeterminismError(recipe.id) return [ @@ -169,9 +174,9 @@ def decide( capability_version=capability.version, bindings=bindings_dict, expansion_port_version=expansion_port.version, - steps_hash=_hash_steps(steps_first), + steps_hash=_hash_steps(steps_first, closing_first), bindings_hash=_hash_bindings(bindings_dict), - step_count=len(steps_first), + step_count=total_count, occurred_at=now, ), ] diff --git a/apps/api/tests/unit/operation/test_conduct_procedure_handler.py b/apps/api/tests/unit/operation/test_conduct_procedure_handler.py index d57829ab94e..f0f10433080 100644 --- a/apps/api/tests/unit/operation/test_conduct_procedure_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_procedure_handler.py @@ -503,7 +503,7 @@ def test_conduct_procedure_request_default_is_empty_step_list() -> None: import hashlib # noqa: E402 -from cora.operation._recipe_expansion import steps_to_wire # noqa: E402 +from cora.operation._recipe_expansion import steps_to_wire_with_closing # noqa: E402 from cora.operation.aggregates.procedure import ( # noqa: E402 ProcedureBoundCapabilityDeprecatedError, ProcedureNotFoundError, @@ -599,6 +599,7 @@ async def _seed_recipe_driven_procedure( *, bindings: dict[str, object] | None = None, recipe_steps: tuple[RecipeSetpointStep, ...] | None = None, + recipe_closing_steps: tuple[RecipeSetpointStep, ...] = (), expansion_port_version: str = "v2-pseudoaxis-aware", bindings_hash_override: str | None = None, steps_hash_override: str | None = None, @@ -625,6 +626,7 @@ async def _seed_recipe_driven_procedure( capability_id=capability_id, steps=rsteps, occurred_at=_NOW, + closing_steps=recipe_closing_steps, ) await store.append( stream_type="Recipe", @@ -653,8 +655,14 @@ async def _seed_recipe_driven_procedure( SetpointStep(address=s.address, value=s.value) # type: ignore[arg-type] for s in rsteps ) + expanded_closing_for_hash: tuple[Step, ...] = tuple( + SetpointStep(address=s.address, value=s.value) # type: ignore[arg-type] + for s in recipe_closing_steps + ) expected_steps_hash = hashlib.sha256( - canonical_json_bytes(steps_to_wire(expanded_for_hash)) + canonical_json_bytes( + steps_to_wire_with_closing(expanded_for_hash, expanded_closing_for_hash) + ) ).hexdigest() registered = ProcedureRegistered( procedure_id=procedure_id, @@ -678,7 +686,7 @@ async def _seed_recipe_driven_procedure( expansion_port_version=expansion_port_version, steps_hash=steps_hash_override or expected_steps_hash, bindings_hash=bindings_hash_override or expected_bindings_hash, - step_count=len(rsteps), + step_count=len(rsteps) + len(recipe_closing_steps), occurred_at=_NOW, ) procedure_events.append(recorded) # type: ignore[arg-type] @@ -852,6 +860,67 @@ async def test_recipe_driven_handler_with_steps_drift_raises_steps_mismatch() -> assert exc.value.mismatch_field == "steps" +@pytest.mark.unit +async def test_conduct_procedure_recipe_with_closing_steps_pins_them_re_expanded() -> None: + """A recipe-driven conduct re-expands recipe.closing_steps too, and pins + the result onto ResolvedStepsRecorded.resolved_closing_steps.""" + procedure_id = uuid4() + recipe_id = uuid4() + store = InMemoryEventStore() + await _seed_recipe_driven_procedure( + store, + procedure_id, + recipe_id, + recipe_closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + conductor = _FakeConductor(result=ConductorResult(procedure_id=procedure_id, completed_count=1)) + handler = _bind_handler(store, conductor) + + await handler( + ConductProcedure(procedure_id=procedure_id, steps=()), + principal_id=uuid4(), + correlation_id=uuid4(), + ) + + stored, _ = await store.load(stream_type="Procedure", stream_id=procedure_id) + recorded = [ + event + for event in (from_stored(s) for s in stored) + if isinstance(event, ResolvedStepsRecorded) + ] + assert len(recorded) == 1 + expected_closing = SetpointStep(address="dev:shutter", value=0.0) + assert recorded[0].resolved_closing_steps == (step_to_payload(expected_closing),) + assert recorded[0].step_count == 2 # 1 main + 1 closing + + +@pytest.mark.unit +async def test_recipe_driven_handler_with_closing_steps_present_still_verifies_the_hash() -> None: + """A wrong pinned hash must still be caught when closing_steps is + non-empty -- proves threading closing_steps through verify_steps_hash + does not accidentally short-circuit verification.""" + procedure_id = uuid4() + recipe_id = uuid4() + store = InMemoryEventStore() + await _seed_recipe_driven_procedure( + store, + procedure_id, + recipe_id, + recipe_closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + steps_hash_override="0" * 64, + ) + conductor = _FakeConductor(result=ConductorResult(procedure_id=procedure_id, completed_count=0)) + handler = _bind_handler(store, conductor) + with pytest.raises(RecipeExpansionReplayMismatchError) as exc: + await handler( + ConductProcedure(procedure_id=procedure_id, steps=()), + principal_id=uuid4(), + correlation_id=uuid4(), + ) + assert exc.value.procedure_id == procedure_id + assert exc.value.mismatch_field == "steps" + + @pytest.mark.unit async def test_conduct_procedure_with_unregistered_procedure_raises_procedure_not_found_error() -> ( None diff --git a/apps/api/tests/unit/operation/test_record_resolved_steps.py b/apps/api/tests/unit/operation/test_record_resolved_steps.py index 58ff49f549b..be96fcb9b99 100644 --- a/apps/api/tests/unit/operation/test_record_resolved_steps.py +++ b/apps/api/tests/unit/operation/test_record_resolved_steps.py @@ -73,6 +73,36 @@ def test_decide_records_resolved_steps_for_defined_procedure() -> None: assert event.occurred_at == _NOW +@pytest.mark.unit +def test_decide_records_resolved_closing_steps_in_a_separate_field() -> None: + """resolved_closing_steps is NOT flattened into resolved_steps: a flat pin + would let a conduct_from boundary land inside the closing region.""" + _, registered = _registered() + state = fold([registered]) + steps = ({"kind": "setpoint", "address": "2bma:rot", "value": 1.0, "verify": False},) + closing = ({"kind": "setpoint", "address": "2bma:shutter", "value": 0.0, "verify": False},) + + events = decide_resolved_steps_recorded(state, steps, now=_NOW, resolved_closing_steps=closing) + + assert len(events) == 1 + event = events[0] + assert event.resolved_steps == steps + assert event.resolved_closing_steps == closing + assert event.step_count == 2 # 1 main + 1 closing, mirrors RecipeExpansionRecorded + + +@pytest.mark.unit +def test_decide_defaults_resolved_closing_steps_to_empty() -> None: + _, registered = _registered() + state = fold([registered]) + steps = ({"kind": "setpoint", "address": "a", "value": 1.0, "verify": False},) + + event = decide_resolved_steps_recorded(state, steps, now=_NOW)[0] + + assert event.resolved_closing_steps == () + assert event.step_count == 1 + + @pytest.mark.unit def test_decide_records_nothing_when_state_is_none() -> None: steps = ({"kind": "setpoint", "address": "a", "value": 1.0, "verify": False},) diff --git a/apps/api/tests/unit/operation/test_register_procedure_from_recipe_decider.py b/apps/api/tests/unit/operation/test_register_procedure_from_recipe_decider.py index 8c2f277f581..acd59b7af03 100644 --- a/apps/api/tests/unit/operation/test_register_procedure_from_recipe_decider.py +++ b/apps/api/tests/unit/operation/test_register_procedure_from_recipe_decider.py @@ -1,5 +1,6 @@ """Unit tests for the `register_procedure_from_recipe` slice's pure decider.""" +import hashlib from collections.abc import Mapping from datetime import UTC, datetime from typing import Any @@ -7,7 +8,7 @@ import pytest -from cora.operation._recipe_expansion import expand +from cora.operation._recipe_expansion import canonical_json_bytes, expand, steps_to_wire from cora.operation.adapters.in_memory_recipe_expander import ( InMemoryRecipeExpander, ) @@ -111,6 +112,167 @@ def test_decide_emits_registered_plus_recipe_expansion_recorded() -> None: assert prov.step_count == 1 +@pytest.mark.unit +def test_decide_hashes_closing_steps_into_the_same_steps_hash_pin() -> None: + """A recipe whose only difference is closing_steps must hash differently + (one combined pin, no aliasing) -- proves closing_steps actually reaches + the hash, not just step_count.""" + cap = _capability() + plain = _recipe(cap.id) + with_closing = Recipe( + id=plain.id, + name=plain.name, + capability_id=plain.capability_id, + steps=plain.steps, + status=plain.status, + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + + plain_events = decide( + state=None, + command=_cmd(plain.id), + recipe=plain, + capability=cap, + expansion_port=InMemoryRecipeExpander(), + now=_NOW, + new_id=uuid4(), + ) + closing_events = decide( + state=None, + command=_cmd(with_closing.id), + recipe=with_closing, + capability=cap, + expansion_port=InMemoryRecipeExpander(), + now=_NOW, + new_id=uuid4(), + ) + plain_prov = plain_events[1] + closing_prov = closing_events[1] + assert isinstance(plain_prov, RecipeExpansionRecorded) + assert isinstance(closing_prov, RecipeExpansionRecorded) + assert plain_prov.steps_hash != closing_prov.steps_hash + assert closing_prov.step_count == 2 # 1 main + 1 closing + + +@pytest.mark.unit +def test_decide_empty_closing_steps_hashes_identically_to_no_closing_field() -> None: + """The migration-claim precedent applied to closing_steps: an empty + closing list must hash EXACTLY like a recipe that never had the field, + so no existing pinned expansion is invalidated by this feature.""" + cap = _capability() + recipe = _recipe(cap.id) + assert recipe.closing_steps == () + + events = decide( + state=None, + command=_cmd(recipe.id), + recipe=recipe, + capability=cap, + expansion_port=InMemoryRecipeExpander(), + now=_NOW, + new_id=uuid4(), + ) + prov = events[1] + assert isinstance(prov, RecipeExpansionRecorded) + expanded = InMemoryRecipeExpander().expand(recipe.steps, {}) + legacy_hash = hashlib.sha256(canonical_json_bytes(steps_to_wire(expanded))).hexdigest() + assert prov.steps_hash == legacy_hash + + +@pytest.mark.unit +def test_decide_raises_overflow_when_combined_count_exceeds_cap() -> None: + """Overflow must range over steps + closing_steps combined, not steps alone.""" + cap = _capability() + recipe = Recipe( + id=uuid4(), + name=RecipeName("R"), + capability_id=cap.id, + steps=(RecipeSetpointStep(address="dev:x", value=1.0),), + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + + class _FakeSplitOverflowPort: + version = "v1" + + def expand( + self, + steps: tuple[RecipeStep, ...], + bindings: Mapping[str, Any], + ) -> tuple[Step, ...]: + _ = bindings + from cora.operation.conductor import SetpointStep + + # Main list alone is well under cap; closing list alone pushes + # the COMBINED count over it. + first = steps[0] if steps else None + is_main = ( + len(steps) == 1 + and isinstance(first, RecipeSetpointStep) + and first.address == "dev:x" + ) + size = 5 if is_main else 10_000 + return tuple(SetpointStep(address=f"x:{i}", value=i) for i in range(size)) + + with pytest.raises(RecipeExpansionOverflowError) as exc: + decide( + state=None, + command=_cmd(recipe.id), + recipe=recipe, + capability=cap, + expansion_port=_FakeSplitOverflowPort(), # type: ignore[arg-type] + now=_NOW, + new_id=uuid4(), + ) + assert exc.value.step_count == 10_005 + assert exc.value.cap == 10_000 + + +@pytest.mark.unit +def test_decide_raises_determinism_error_on_closing_steps_divergence() -> None: + """A port that diverges only on the closing-list expansion must still + trip the determinism gate, not just a main-list divergence.""" + cap = _capability() + recipe = Recipe( + id=uuid4(), + name=RecipeName("R"), + capability_id=cap.id, + steps=(RecipeSetpointStep(address="dev:x", value=1.0),), + closing_steps=(RecipeSetpointStep(address="dev:shutter", value=0.0),), + ) + + class _FakeNonDeterministicClosingPort: + version = "v1" + _calls = 0 + + def expand( + self, + steps: tuple[RecipeStep, ...], + bindings: Mapping[str, Any], + ) -> tuple[Step, ...]: + from cora.operation.conductor import SetpointStep + + first = steps[0] if steps else None + if ( + len(steps) == 1 + and isinstance(first, RecipeSetpointStep) + and first.address == "dev:shutter" + ): + self._calls += 1 + return (SetpointStep(address="dev:shutter", value=float(self._calls)),) + return expand(steps, bindings) + + with pytest.raises(RecipeExpansionDeterminismError): + decide( + state=None, + command=_cmd(recipe.id), + recipe=recipe, + capability=cap, + expansion_port=_FakeNonDeterministicClosingPort(), # type: ignore[arg-type] + now=_NOW, + new_id=uuid4(), + ) + + @pytest.mark.unit def test_decide_records_patience_cap_on_event() -> None: cap = _capability() @@ -252,7 +414,12 @@ def expand( steps: tuple[RecipeStep, ...], bindings: Mapping[str, Any], ) -> tuple[Step, ...]: - _ = steps, bindings + # Overflow is isolated to the main list; an empty input (the + # closing_steps call, since big_recipe has none) must expand to + # empty, or the combined count double-counts a fixed-size fake. + _ = bindings + if not steps: + return () from cora.operation.conductor import SetpointStep return tuple(SetpointStep(address=f"x:{i}", value=i) for i in range(10_001)) From 387494a0e7ede8045ac25857220fb2073d140e44 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sun, 30 Aug 2026 05:11:00 -0500 Subject: [PATCH 4/7] Walk a recipe's closing steps on every real conduct terminal The Conductor now runs _run_closing after execute() reaches a real terminal (Completed or Aborted) in conduct, conduct_or_hold, and conduct_from, isolating each closing step's failure so one bad step never blocks the rest and never flips succeeded. Held, acquisition halts, and cancellation all still skip closing entirely. Closing's journal writes go through append_activities, which accepts entries only while the Procedure is Running. That means the walk has to happen BEFORE the terminal complete_procedure/abort_procedure call, not after -- the initial implementation had this backwards, which would have silently rejected every closing-step journal entry once the FSM had already left Running. All three wrappers, and their docstrings, now run closing first and attempt the terminal transition last, so a subsequent rejection still keeps the closing ledger that already ran. ConductorResult gains closing_failures; every one of the 14 registered construction sites in test_conductor_result_construction_sites.py omits it correctly, since none of them runs after a closing walk. --- apps/api/src/cora/operation/conductor.py | 487 ++++++++++++++++-- ...est_conductor_result_construction_sites.py | 54 +- .../tests/unit/operation/test_conductor.py | 477 +++++++++++++++++ 3 files changed, 951 insertions(+), 67 deletions(-) diff --git a/apps/api/src/cora/operation/conductor.py b/apps/api/src/cora/operation/conductor.py index bfaef71ce16..63e8bdb371f 100644 --- a/apps/api/src/cora/operation/conductor.py +++ b/apps/api/src/cora/operation/conductor.py @@ -967,18 +967,33 @@ class ConductorResult: """Every address this conduct wrote, in first-write order, last value. Present on the success and the failure construction alike, because a - conduct changes the world either way. It earns its place on the - FAILURE one: a halt returns from the step loop, so a recipe's own - closing steps do not run, and this is then the list of what was left - set with nothing having put it back. Without it an operator has to - reconstruct that from the step journal at exactly the moment they - are least able to. + conduct changes the world either way. On a TERMINAL outcome (Completed + or Aborted) this INCLUDES whatever the recipe's own closing steps wrote: + it is no longer only "what was left set with nothing having put it + back" -- a closing-only address now appears here too, since closing + steps write through the SAME `_ActuationObserver`. On a Held pause + (closing does not run) it is still exactly that: the list of what CORA + left set, unrestored, because a later `conduct_from` resumes against + this exact state. Without it an operator has to reconstruct that from + the step journal at exactly the moment they are least able to. It reports what CORA WROTE, not what it changed: a write whose value already matched the PV still appears. Deciding otherwise would mean reading every address back, which is a second round of substrate traffic to answer a question the operator did not ask. """ + closing_failures: tuple[ConductorFailure, ...] = () + """Every closing step that failed, in walked order; empty on success. + + Closing steps are ISOLATED from each other and from `failure`: one + failing is recorded and the next still runs, and a closing failure + never changes `succeeded` (that bit reports the MAIN steps only, + per [[project_conduct_closing_steps_design]]). Empty whenever closing + did not run at all (Held, an acquisition halt, cancellation, a + Procedure with no `closing_steps`) -- indistinguishable from "closing + ran and every step passed"; a caller that needs to tell the two apart + reads the recipe's `closing_steps` list length instead. + """ @property def succeeded(self) -> bool: @@ -1050,10 +1065,16 @@ def substrate_writes(self) -> Mapping[str, WriteValue]: CORA does not restore what it sets (see `acquisitions`' prior / applied recording for why a tidy-up write would clobber a - concurrent client), so on a HALT this is the list of things left - as CORA put them, with the recipe's own closing steps unrun. An - operator reading a failed conduct should not have to reconstruct - that from the step journal. + concurrent client). A wrapper that runs closing steps (`conduct`, + `conduct_or_hold`'s terminal arms, `conduct_from`'s terminal arms) + merges `_run_closing`'s own observer's writes on top of this map + before returning the final result -- on a TERMINAL outcome this is + no longer only "what was left with nothing having put it back"; it + also includes what closing put back. On a HELD pause (closing does + not run) it is still exactly the pre-closing meaning: the list of + things left as CORA put them. An operator reading a conduct's + result should not have to reconstruct either case from the step + journal. Reads are deliberately absent: reading changes nothing, and mixing them in would bury the writes that actually need @@ -1246,6 +1267,7 @@ async def execute_from( policy: ResumePolicy = ResumePolicy.RE_ESTABLISH, causation_id: UUID | None = None, surface_id: UUID = NIL_SENTINEL_ID, + captures: dict[str, Any] | None = None, ) -> ConductorResult: """Resume a halted conduct by REPLAYING the pinned resolved steps from `boundary`. @@ -1293,7 +1315,11 @@ async def execute_from( # boundary is not replayed, so a CaptureRef into it fails loud rather # than resolving against stale data. Persisting captures across a hold # (seed this dict from a ValueCaptured event) is the deferred resume leg. - captures: dict[str, Any] = {} + # `captures` is caller-owned like `execute()`'s (None creates a fresh + # dict): `conduct_from` passes one in so `_run_closing` can share + # whatever the replay tail deposits. + if captures is None: + captures = {} # The artifact bus is likewise empty on resume and never filled here: a # ComputeStep reached during replay halts for an operator decision (it is # never dispatched), so nothing deposits and no OutputRef resolves against @@ -1375,6 +1401,88 @@ async def execute_from( outputs=dict(outputs), ) + async def _run_closing( + self, + closing_steps: Sequence[Step], + *, + procedure_id: UUID, + principal_id: UUID, + correlation_id: UUID, + causation_id: UUID | None, + surface_id: UUID, + captures: dict[str, Any], + ) -> tuple[tuple[ConductorFailure, ...], Mapping[str, WriteValue]]: + """Walk `closing_steps` after a conduct reaches a REAL terminal. + + Called by `conduct`, `conduct_or_hold` (its two terminal arms only: + complete-success and abort), and `conduct_from` (its complete and + abort arms only) -- never on a Held pause, an acquisition halt, or + cancellation. See [[project_conduct_closing_steps_design]] for the + full six-row disposition table this enforces. + + ISOLATED per step, unlike the main list: one step failing is + recorded and the WALK CONTINUES; this method itself NEVER raises, + so it cannot mask whatever the caller does next (return a result, + or re-raise an already-in-flight exception). Runs in its own + `step_index` space starting at 0 (see `_Envelope.closing`). + + `captures` is the conduct's OWN captures bus, passed in by + reference and shared with the main list (a closing step MAY read a + value the main list captured; the reverse is never true since + closing runs after). Returns `(closing_failures, substrate_writes)` + for the caller to fold onto its `ConductorResult`: closing gets its + own fresh `_ActuationObserver`, so its writes are NOT already + reflected in the main list's `substrate_writes` and the caller + must merge them explicitly. + + An empty `closing_steps` is the common case (no Recipe authors + one): short-circuits before touching the ControlPort at all. + """ + if not closing_steps: + return (), {} + envelope = _Envelope( + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + closing=True, + ) + observer = _ActuationObserver(self._control_port) + outputs: dict[str, ArtifactRef] = {} + compute = _ComputeAccumulator() + failures: list[ConductorFailure] = [] + for index, step in enumerate(closing_steps): + try: + with with_dispatch_correlation_id(correlation_id): + failure = await self._dispatch( + step, + index=index, + envelope=envelope, + port=observer, + captures=captures, + outputs=outputs, + compute=compute, + ) + except Exception as exc: + # "Never raises" is a GUARANTEE, not an intention: a closing + # step can raise for the same reasons a main step's dispatch + # can (a buggy action body, an unexpected port error class), + # and closing is exactly the walk that must survive one bad + # step to still attempt the rest. Convert to a recorded + # failure instead of propagating. + failure = ConductorFailure( + step_index=index, + source_kind=_closing_step_kind(step), + target=_closing_step_target(step), + error_class=type(exc).__name__, + message=str(exc), + ) + if failure is not None: + failures.append(failure) + # ISOLATED: no halt-on-failure. The next closing step always runs. + return tuple(failures), observer.substrate_writes + async def conduct( self, *, @@ -1384,6 +1492,7 @@ async def conduct( steps: Sequence[Step], causation_id: UUID | None = None, surface_id: UUID = NIL_SENTINEL_ID, + closing_steps: Sequence[Step] = (), ) -> ConductorResult: """Drive the full Procedure lifecycle: start -> execute -> complete | abort. @@ -1404,6 +1513,23 @@ async def conduct( (the Procedure stays Running and the operator must reconcile via state inspection). + `closing_steps` runs via `_run_closing` on a REAL terminal only: once + `execute()` reaches Completed-bound success or a step failure, and + BEFORE the corresponding `complete_procedure` / `abort_procedure` + call -- closing's own journal writes go through `append_activities`, + which accepts entries only while the Procedure is `Running` + (`_OPEN_STATUSES = {RUNNING}`); attempting them after the FSM has + already left Running would reject every one of them. So the terminal + FSM write is genuinely LAST here, not first: closing physically runs + (and its outcome is durably journaled) regardless of whether the + SUBSEQUENT `complete_procedure` call itself is rejected. Also runs + before the best-effort abort on an unhandled exception from + `execute()` (the literal TomoScan shape this design cites -- see the + outer `except Exception` below). NOT run on cancellation + (`abort_orphan_on_cancel` already re-raises before any return here). + See [[project_conduct_closing_steps_design]] for the full + disposition table. + Requires `start_procedure` + `complete_procedure` + `abort_procedure` handlers to have been supplied at __init__. Raises `RuntimeError` if any of the three is missing; this @@ -1454,23 +1580,72 @@ async def conduct( # then re-raises so the caller's task still sees the cancellation. No # ConductorResult exists on cancellation, so the observed kind is # unrecoverable and the abort records None (a Dataset off a cancelled - # conduct carries no proven kind). + # conduct carries no proven kind). Cancellation does NOT run closing: + # a cancellation is typically loop teardown, the worst moment to + # start driving hardware, and this re-raises before any return below. abort_procedure = self._abort_procedure - async with abort_orphan_on_cancel( - lambda: abort_procedure( - AbortProcedure(procedure_id=procedure_id, reason="cancelled mid-execute"), - **envelope_kwargs, - ) - ): - result = await self.execute( + captures: dict[str, Any] = {} + try: + async with abort_orphan_on_cancel( + lambda: abort_procedure( + AbortProcedure(procedure_id=procedure_id, reason="cancelled mid-execute"), + **envelope_kwargs, + ) + ): + result = await self.execute( + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + steps=steps, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + except asyncio.CancelledError: + raise + except Exception: + # The literal TomoScan shape: execute() deliberately lets a + # non-port exception propagate (a programmer error, not a + # ConductorFailure), and closing must still run before it + # leaves -- BEFORE the best-effort abort, so closing's own + # journal writes land while the Procedure is still Running (see + # the docstring). Runs for its RECORDING side-effect alone here, + # since there is no ConductorResult to attach closing_failures + # to when the caller is about to see this exception, not a + # return value. + with contextlib.suppress(Exception): + await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + with contextlib.suppress(Exception): + await abort_procedure( + AbortProcedure( + procedure_id=procedure_id, reason="unhandled exception during execute" + ), + **envelope_kwargs, + ) + raise + if result.succeeded: + closing_failures, closing_writes = await self._run_closing( + closing_steps, procedure_id=procedure_id, principal_id=principal_id, correlation_id=correlation_id, - steps=steps, causation_id=causation_id, surface_id=surface_id, + captures=captures, + ) + result = replace( + result, + closing_failures=closing_failures, + substrate_writes={**result.substrate_writes, **closing_writes}, ) - if result.succeeded: try: await self._complete_procedure( CompleteProcedure( @@ -1492,8 +1667,10 @@ async def conduct( except Exception as exc: # `replace`, not a fresh ConductorResult: a hand-copied # field list silently drops whatever the caller forgets to - # list, and `substrate_writes` is exactly the field an - # operator needs on a completion that itself failed. + # list. Closing already ran (above) and its ledger survives + # here even though complete_procedure itself was REJECTED -- + # closing is what physically happened; the FSM label is a + # separate, subsequent fact that failed to record. return replace( result, failure=ConductorFailure( @@ -1505,11 +1682,26 @@ async def conduct( ), ) return result - # execute failed; attempt abort with a derived reason. Best-effort: - # if abort itself fails, surface the original step failure since - # that is what the caller needs to triage. + # execute failed. Closing runs BEFORE the best-effort abort (its + # journal writes need the Procedure still Running); abort itself is + # best-effort regardless: if it fails, surface the original step + # failure since that is what the caller needs to triage. failure = result.failure assert failure is not None # not result.succeeded implies failure + closing_failures, closing_writes = await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + result = replace( + result, + closing_failures=closing_failures, + substrate_writes={**result.substrate_writes, **closing_writes}, + ) reason = _derive_failure_reason(failure) with contextlib.suppress(Exception): await self._abort_procedure( @@ -1535,6 +1727,7 @@ async def conduct_or_hold( steps: Sequence[Step], causation_id: UUID | None = None, surface_id: UUID = NIL_SENTINEL_ID, + closing_steps: Sequence[Step] = (), ) -> ConductorResult: """Drive the lifecycle like `conduct()`, but PAUSE to Held on a recoverable failure. @@ -1555,6 +1748,18 @@ async def conduct_or_hold( - lifecycle failures (start / complete rejected) and a mid-execute `CancelledError` keep `conduct()`'s behavior verbatim (no hold). + `closing_steps` runs on exactly TWO of the arms above: complete- + success, and the non-recoverable-failure abort -- and runs BEFORE + the corresponding `complete_procedure` / `abort_procedure` call, for + the same reason as `conduct()`: closing's own journal writes need + the Procedure still Running. It does NOT run on Held (a hold is a + pause whose resume replays `steps[boundary:]` against the state the + pre-boundary steps established; closing first would tear that + down), on a failed hold attempt (the Procedure is left Running, not + terminal), or on cancellation. A subsequent complete-rejection does + NOT undo a closing walk that already ran. See + [[project_conduct_closing_steps_design]]. + Requires `start_procedure` + `complete_procedure` + `abort_procedure` + `hold_procedure` handlers at __init__; raises `RuntimeError` (a wiring bug) otherwise. @@ -1601,23 +1806,67 @@ async def conduct_or_hold( # Mirror conduct(): a mid-execute cancellation best-effort aborts so the # FSM is not orphaned in Running, then re-raises. A cancellation is not a # recoverable step failure, so it aborts rather than pausing to Held. + # Closing does NOT run here either, for the same reason as conduct(). abort_procedure = self._abort_procedure - async with abort_orphan_on_cancel( - lambda: abort_procedure( - AbortProcedure(procedure_id=procedure_id, reason="cancelled mid-execute"), - **envelope_kwargs, - ) - ): - result = await self.execute( + captures: dict[str, Any] = {} + try: + async with abort_orphan_on_cancel( + lambda: abort_procedure( + AbortProcedure(procedure_id=procedure_id, reason="cancelled mid-execute"), + **envelope_kwargs, + ) + ): + result = await self.execute( + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + steps=steps, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + except asyncio.CancelledError: + raise + except Exception: + # Mirror conduct()'s raised-exception handling: closing runs + # BEFORE the best-effort abort (its journal writes need the + # Procedure still Running), for its recording side-effect alone + # (no ConductorResult to attach closing_failures to when + # re-raising). + with contextlib.suppress(Exception): + await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + with contextlib.suppress(Exception): + await abort_procedure( + AbortProcedure( + procedure_id=procedure_id, reason="unhandled exception during execute" + ), + **envelope_kwargs, + ) + raise + actuation_kind = result.actuation_kind.value if result.actuation_kind is not None else None + if result.succeeded: + closing_failures, closing_writes = await self._run_closing( + closing_steps, procedure_id=procedure_id, principal_id=principal_id, correlation_id=correlation_id, - steps=steps, causation_id=causation_id, surface_id=surface_id, + captures=captures, + ) + result = replace( + result, + closing_failures=closing_failures, + substrate_writes={**result.substrate_writes, **closing_writes}, ) - actuation_kind = result.actuation_kind.value if result.actuation_kind is not None else None - if result.succeeded: try: await self._complete_procedure( CompleteProcedure(procedure_id=procedure_id, actuation_kind=actuation_kind), @@ -1626,6 +1875,8 @@ async def conduct_or_hold( except _LIFECYCLE_RERAISE: raise except Exception as exc: + # Closing already ran (above) and survives a SUBSEQUENT + # complete_procedure rejection; see conduct()'s twin comment. return replace( result, failure=ConductorFailure( @@ -1661,13 +1912,31 @@ async def conduct_or_hold( if held_ok: # A Held Procedure is parked mid-flight awaiting an operator # decision, which is exactly when `substrate_writes` has to - # survive: it is the list of what CORA left set, and the - # recipe's own closing steps did not run. + # survive: it is the list of what CORA left set. NO closing: + # a hold is a pause, not a terminal; a later conduct_from + # resumes against the world the main steps established, and + # closing first would tear that down. return replace(result, failure=failure, held=True) return result # Non-recoverable step failure (action): best-effort abort, exactly # like conduct(). Holding would strand a Procedure whose replay tail - # starts with an interrupted acquisition. + # starts with an interrupted acquisition. Closing runs BEFORE the + # abort attempt (its journal writes need the Procedure still + # Running), regardless of whether the abort itself then succeeds. + closing_failures, closing_writes = await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + result = replace( + result, + closing_failures=closing_failures, + substrate_writes={**result.substrate_writes, **closing_writes}, + ) with contextlib.suppress(Exception): await self._abort_procedure( AbortProcedure( @@ -2707,6 +2976,7 @@ async def conduct_from( prior_actuation_kind: str | None = None, causation_id: UUID | None = None, surface_id: UUID = NIL_SENTINEL_ID, + closing_steps: Sequence[Step] = (), ) -> ConductorResult: """Resume a Held Procedure and REPLAY its pinned resolved steps from `boundary`. @@ -2744,6 +3014,19 @@ async def conduct_from( Running with partial replay history, the same posture as the acquisition-halt branch (the operator reconciles). See [[project_resumable_conduct_design]] Tier 1. + + `closing_steps` runs via `_run_closing` on exactly TWO of the three + terminals: the clean-tail complete, and the genuine-step-failure + abort -- and runs BEFORE the corresponding `complete_procedure` / + `abort_procedure` call, same reason as `conduct()`: closing's own + journal writes need the Procedure still Running. NOT on the + acquisition halt (the Procedure stays Running, not terminal -- + closing there would foreclose the very redo-fresh-vs-reseed + decision being handed back), and NOT on a raised exception that is + itself a cancellation (mirrors this method's existing + no-abort-on-cancel posture). A subsequent complete-rejection does + NOT undo a closing walk that already ran. See + [[project_conduct_closing_steps_design]]. """ if ( self._resume_procedure is None @@ -2768,15 +3051,39 @@ async def conduct_from( ResumeProcedure(procedure_id=procedure_id, re_establishment_boundary=boundary), **envelope_kwargs, ) - result = await self.execute_from( - procedure_id=procedure_id, - principal_id=principal_id, - correlation_id=correlation_id, - steps=steps, - boundary=boundary, - causation_id=causation_id, - surface_id=surface_id, - ) + captures: dict[str, Any] = {} + try: + result = await self.execute_from( + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + steps=steps, + boundary=boundary, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + except asyncio.CancelledError: + raise + except Exception: + # Mirror conduct()'s raised-exception handling for the literal + # TomoScan shape. Unlike conduct(), no best-effort abort precedes + # it here -- this method already leaves a mid-replay cancellation + # un-aborted by design (the operator reconciles), and the same + # posture applies to any other raised exception: the Procedure + # stays Running, closing runs for its recording side-effect + # alone, then the exception propagates unchanged. + with contextlib.suppress(Exception): + await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + raise # Fold the pre-hold conduct's kind (carried on the Held procedure, # passed in by the handler) with the replay tail's observed kind, so a # boundary>0 resume past a simulated prefix does not complete as @@ -2793,8 +3100,24 @@ async def conduct_from( actuation_kind=(ActuationKind(actuation_kind) if actuation_kind is not None else None), ) if result.succeeded: - # Clean tail (incl. empty tail): auto-complete, threading the - # merged observed kind onto ProcedureCompleted (Data BC gate carrier). + # Clean tail (incl. empty tail): closing runs BEFORE the + # auto-complete attempt (its journal writes need the Procedure + # still Running), threading the merged observed kind onto + # ProcedureCompleted (Data BC gate carrier). + closing_failures, closing_writes = await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + merged_result = replace( + merged_result, + closing_failures=closing_failures, + substrate_writes={**merged_result.substrate_writes, **closing_writes}, + ) try: await self._complete_procedure( CompleteProcedure(procedure_id=procedure_id, actuation_kind=actuation_kind), @@ -2806,7 +3129,8 @@ async def conduct_from( # `merged_result`, not `result`: the merged kind is what the # terminal event carried, so the response has to agree with it # on the complete-rejected arm exactly as it does on the - # success arm below. + # success arm below. Closing already ran (above) and survives + # this SUBSEQUENT rejection; see conduct()'s twin comment. return replace( merged_result, failure=ConductorFailure( @@ -2820,6 +3144,9 @@ async def conduct_from( return merged_result if is_acquisition_halt(result.failure): # Halt-for-operator: leave the Procedure Running; no transition. + # NO closing: the Procedure is not terminal, and closing here + # would foreclose the redo-fresh-vs-reseed decision being handed + # back to the operator. # RESIDUAL: the replay tail's observed kind is NOT persisted here # (no terminal event), so a later manual complete/abort -- which # SETs actuation_kind from the command, not merges -- could stamp @@ -2827,10 +3154,26 @@ async def conduct_from( # this method closes; the design-memo second-writer hazard, aligned # with the Tier-2 acquisition-decomposition deferral. return merged_result - # Genuine step failure: best-effort abort (if abort itself fails, the - # original step failure is what surfaces). Mirrors conduct(). + # Genuine step failure: closing runs BEFORE the best-effort abort + # (its journal writes need the Procedure still Running); abort + # itself is best-effort regardless (if it fails, the original step + # failure is what surfaces). Mirrors conduct(). failure = result.failure assert failure is not None # not succeeded + not halt -> failure + closing_failures, closing_writes = await self._run_closing( + closing_steps, + procedure_id=procedure_id, + principal_id=principal_id, + correlation_id=correlation_id, + causation_id=causation_id, + surface_id=surface_id, + captures=captures, + ) + merged_result = replace( + merged_result, + closing_failures=closing_failures, + substrate_writes={**merged_result.substrate_writes, **closing_writes}, + ) with contextlib.suppress(Exception): await self._abort_procedure( AbortProcedure( @@ -4146,13 +4489,18 @@ async def _record( `index` is the step's zero-based position in the conducted step list; it rides the payload as `step_index` so a future resume can map a recorded outcome back to its position in the pinned - resolved step list. + resolved step list. `envelope.closing` (`_run_closing`'s walk) + adds a `"closing": true` marker: closing runs in its OWN + `step_index` space starting at 0, so the index alone would + collide with a main step's row. """ payload: dict[str, Any] = {**body, "step_index": index, "result": result} if error_class is not None: payload["error_class"] = error_class if message is not None: payload["message"] = message + if envelope.closing: + payload["closing"] = True sampled_at = self._clock.now() entry = ActivityInput( event_id=self._id_generator.new_id(), @@ -4176,6 +4524,13 @@ class _Envelope: Internal helper; avoids passing six args to every helper method. Frozen so accidental mutation mid-execute is a type error. + + `closing` marks a `_run_closing` walk: `_record` tags the journal row + `"closing": true` when set, disambiguating a closing step's outcome + from a main step's in the SAME activity log (closing runs in its own + `step_index` space starting at 0, so the index alone does not + disambiguate). Threaded via the envelope rather than a `_record` + kwarg so none of its other call sites need touching. """ procedure_id: UUID @@ -4183,6 +4538,7 @@ class _Envelope: correlation_id: UUID causation_id: UUID | None surface_id: UUID + closing: bool = False @dataclass @@ -4441,6 +4797,29 @@ def _derive_failure_reason(failure: ConductorFailure) -> str: return reason[:REASON_MAX_LENGTH] +def _closing_step_kind(step: Step) -> str: + """`_run_closing`'s defensive fallback: classify a step for a synthetic + `ConductorFailure` when `_dispatch` raised instead of returning one.""" + if isinstance(step, SetpointStep): + return _STEP_KIND_SETPOINT + if isinstance(step, ActionStep): + return _STEP_KIND_ACTION + if isinstance(step, CaptureStep): + return _STEP_KIND_CAPTURE + if isinstance(step, ComputeStep): + return _STEP_KIND_COMPUTE + return _STEP_KIND_CHECK + + +def _closing_step_target(step: Step) -> str: + """Sibling of `_closing_step_kind`: the failure's `target` field.""" + if isinstance(step, ActionStep): + return step.name + if isinstance(step, ComputeStep): + return " ".join(step.command) + return step.address + + def _measurement_to_dict(reading: Measurement) -> dict[str, Any]: """JSON-clean projection of `Measurement` for the step payload. diff --git a/apps/api/tests/architecture/test_conductor_result_construction_sites.py b/apps/api/tests/architecture/test_conductor_result_construction_sites.py index fe5671dd2c9..b6c22e98eb1 100644 --- a/apps/api/tests/architecture/test_conductor_result_construction_sites.py +++ b/apps/api/tests/architecture/test_conductor_result_construction_sites.py @@ -57,29 +57,54 @@ _REQUIRED_FIELDS = frozenset({"procedure_id", "completed_count"}) #: Pre-`start_procedure` lifecycle rejections: no step has run, so the entire -#: ledger is correctly empty. +#: ledger is correctly empty. `closing_failures` too: closing has not run. _PRE_START_OMISSIONS = frozenset( - {"actuation_kind", "artifacts", "held", "measurements", "outputs", "substrate_writes"} + { + "actuation_kind", + "artifacts", + "closing_failures", + "held", + "measurements", + "outputs", + "substrate_writes", + } ) #: Registry of every direct `ConductorResult(...)` site, keyed by #: (enclosing function name, 1-indexed occurrence within that function in #: source order), mapped to the EXACT set of fields it leaves at their #: default. See the module docstring for what this protects. +#: +#: `closing_failures` was added to EVERY entry below in one pass when the +#: field landed: none of these 14 sites runs `_run_closing` (that only +#: happens in the wrapper methods' terminal RETURN via `replace()`, which +#: this test never inspects), so every site correctly omits it. This is the +#: exact forcing function the module docstring describes -- the guard failed +#: at every registered site the moment the field existed, and the fix at +#: each site was "yes, still correctly omitted," not a code change. _EXPECTED_OMISSIONS: dict[tuple[str, int], frozenset[str]] = { # execute(): per-step and final results are built straight from local # data (the actuation observer, the compute accumulator, a running # count), never copied from a stale prior ConductorResult. `held` is - # correctly absent: execute() itself never holds anything. - ("execute", 1): frozenset({"held"}), - ("execute", 2): frozenset({"failure", "held"}), + # correctly absent: execute() itself never holds anything. Closing only + # ever runs from a wrapper's terminal branch, never inside execute(). + ("execute", 1): frozenset({"closing_failures", "held"}), + ("execute", 2): frozenset({"closing_failures", "failure", "held"}), # execute_from(): an ActionStep/ComputeStep halt-for-operator or a step # failure returns before any ComputeStep could run, so measurements / # artifacts / outputs are correctly empty; a resume replay never holds. - ("execute_from", 1): frozenset({"artifacts", "held", "measurements", "outputs"}), - ("execute_from", 2): frozenset({"artifacts", "held", "measurements", "outputs"}), - ("execute_from", 3): frozenset({"artifacts", "held", "measurements", "outputs"}), - ("execute_from", 4): frozenset({"artifacts", "failure", "held", "measurements"}), + ("execute_from", 1): frozenset( + {"artifacts", "closing_failures", "held", "measurements", "outputs"} + ), + ("execute_from", 2): frozenset( + {"artifacts", "closing_failures", "held", "measurements", "outputs"} + ), + ("execute_from", 3): frozenset( + {"artifacts", "closing_failures", "held", "measurements", "outputs"} + ), + ("execute_from", 4): frozenset( + {"artifacts", "closing_failures", "failure", "held", "measurements"} + ), # Pre-start lifecycle failures: start_procedure itself was rejected, so # no step ever ran. ("conduct", 1): _PRE_START_OMISSIONS, @@ -88,19 +113,22 @@ ("conduct_until_advised", 1): _PRE_START_OMISSIONS, # _abort_unconverged_cap / _abort_absolute_ceiling thread the last pass's # ledger through a None-safe ternary (2026-08-29 fix); `held` is the only - # remaining gap, correctly: neither loop-top abort ever holds. - ("_abort_unconverged_cap", 1): frozenset({"held"}), - ("_abort_absolute_ceiling", 1): frozenset({"held"}), + # remaining gap, correctly: neither loop-top abort ever holds. Closing + # steps are v1-refused for the loop slices, so closing_failures is + # correctly always empty here too. + ("_abort_unconverged_cap", 1): frozenset({"closing_failures", "held"}), + ("_abort_absolute_ceiling", 1): frozenset({"closing_failures", "held"}), # conduct_until_advised_from(): a frontier brain fault before any pass ran # (no execute() call yet, so no ledger to carry), and a resume-straight- # to-Stop synthetic placeholder fed into _complete_advised (same reason). ("conduct_until_advised_from", 1): frozenset( - {"artifacts", "held", "measurements", "outputs", "substrate_writes"} + {"artifacts", "closing_failures", "held", "measurements", "outputs", "substrate_writes"} ), ("conduct_until_advised_from", 2): frozenset( { "actuation_kind", "artifacts", + "closing_failures", "failure", "held", "measurements", diff --git a/apps/api/tests/unit/operation/test_conductor.py b/apps/api/tests/unit/operation/test_conductor.py index b7060075855..cd71f21a2c4 100644 --- a/apps/api/tests/unit/operation/test_conductor.py +++ b/apps/api/tests/unit/operation/test_conductor.py @@ -2159,6 +2159,329 @@ async def test_conduct_complete_rejection_still_reports_what_was_left_set() -> N assert result.actuation_kind is ActuationKind.PHYSICAL +# --- closing steps ------------------------------------------------------- +# +# Pins the disposition table from [[project_conduct_closing_steps_design]]: +# closing runs whenever the main list reaches a Completed-bound success or +# a step failure (including a raised exception, the literal TomoScan +# shape) -- and runs BEFORE the corresponding complete_procedure / +# abort_procedure call, since closing's own journal writes need the +# Procedure still Running. Skipped on Held, an acquisition halt, and +# cancellation. A SUBSEQUENT complete_procedure rejection does not undo a +# closing walk that already ran. + + +@pytest.mark.unit +async def test_conduct_success_runs_closing_steps_and_merges_substrate_writes() -> None: + port, _ = _routed_port("2bma:main", "2bma:closing") + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(4)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:main", value=1.0),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is True + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:main": 1.0, "2bma:closing": 0.0} + + +@pytest.mark.unit +async def test_conduct_execute_failure_aborts_then_still_runs_closing_steps() -> None: + """A halted main list still runs closing after the best-effort abort -- + the shutter-open case this feature exists for.""" + port, _ = _routed_port("2bma:closing") # "2bma:missing" is NOT connected + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=abort, + ids=[uuid4() for _ in range(4)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:missing", value=1.0),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is False + assert len(abort.calls) == 1 + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:closing": 0.0} + + +@pytest.mark.unit +async def test_conduct_closing_step_failure_is_isolated_and_does_not_change_succeeded() -> None: + """One closing step failing records it and the NEXT closing step still + runs; a closing failure never changes `succeeded` (that bit reports the + main steps only).""" + port, _ = _routed_port("2bma:main", "2bma:closing_ok") # "2bma:closing_bad" unconnected + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(6)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:main", value=1.0),), + closing_steps=( + SetpointStep(address="2bma:closing_bad", value=0.0), + SetpointStep(address="2bma:closing_ok", value=0.0), + ), + ) + + assert result.succeeded is True # main steps succeeded; closing failure is separate + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].target == "2bma:closing_bad" + # The SECOND closing step still ran despite the first failing (isolated). + assert dict(result.substrate_writes) == {"2bma:main": 1.0, "2bma:closing_ok": 0.0} + + +@pytest.mark.unit +async def test_conduct_closing_step_that_raises_is_caught_not_propagated() -> None: + """`_run_closing` NEVER raises, even against a bug that would otherwise + propagate out of `_dispatch` (mirrors `_run_action`'s own non-port + exceptions propagating in the MAIN list): a raising closing step becomes + a recorded failure, and conduct() returns normally instead of raising.""" + + async def buggy(_ctx: ActionContext) -> Mapping[str, Any]: + raise RuntimeError("closing bug") + + registry = InMemoryActionRegistry({"buggy": buggy}) + port, _ = _routed_port("2bma:main") + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(6)], + registry=registry, + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:main", value=1.0),), + closing_steps=(ActionStep(name="buggy"),), + ) + + assert result.succeeded is True + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].error_class == "RuntimeError" + assert result.closing_failures[0].target == "buggy" + + +@pytest.mark.unit +async def test_conduct_cancellation_does_not_run_closing_steps() -> None: + """A cancellation re-raises before any closing walk is attempted.""" + + class _CountingCancellingPort: + write_calls = 0 + + async def read(self, _address: str) -> Measurement: # pragma: no cover # unused + raise NotImplementedError + + async def write(self, *_args: Any, **_kwargs: Any) -> None: + type(self).write_calls += 1 + raise asyncio.CancelledError + + def subscribe(self, _address: str) -> AsyncIterator[Measurement]: # pragma: no cover + raise NotImplementedError + + _CountingCancellingPort.write_calls = 0 + appender = _FakeAppendStep() + conductor = Conductor( + control_port=_CountingCancellingPort(), # type: ignore[arg-type] + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([]), + start_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=_FakeLifecycleHandler(), + ) + with pytest.raises(asyncio.CancelledError): + await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="any", value=1.0),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + # Exactly ONE write attempt (the main step's, which raised): closing was + # never dispatched, so it never reached the port at all. + assert _CountingCancellingPort.write_calls == 1 + + +@pytest.mark.unit +async def test_conduct_raised_exception_still_runs_closing_before_reraising() -> None: + """The literal TomoScan shape: an unhandled exception from an action body + must not skip closing. Best-effort abort fires first (closing runs on a + real terminal), closing runs, then the ORIGINAL exception propagates + unchanged.""" + + async def buggy(_ctx: ActionContext) -> Mapping[str, Any]: + raise RuntimeError("oops") + + registry = InMemoryActionRegistry({"buggy": buggy}) + port, inner = _routed_port("2bma:closing") + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=abort, + ids=[uuid4() for _ in range(4)], + registry=registry, + ) + + with pytest.raises(RuntimeError, match="oops"): + await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(ActionStep(name="buggy"),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + assert len(abort.calls) == 1 + assert "unhandled exception" in abort.calls[0].command.reason + landed = await inner.read("2bma:closing") + assert landed.value == 0.0 + + +@pytest.mark.unit +async def test_conduct_complete_rejected_still_reports_closing_that_already_ran() -> None: + """Closing runs BEFORE complete_procedure is even attempted (its journal + writes need the Procedure still Running -- append_activities accepts + entries only in Running). So a SUBSEQUENT complete_procedure rejection + does not undo the physical fact that closing already happened; the + ledger and any closing_failures still surface.""" + port, _ = _routed_port("2bma:shutter", "2bma:closing") + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(raises=RuntimeError("complete rejected")), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(4)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:shutter", value=1),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is False + assert result.failure is not None + assert result.failure.target == "complete" + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:shutter": 1, "2bma:closing": 0.0} + + +@pytest.mark.unit +async def test_conduct_or_hold_held_procedure_does_not_run_closing_steps() -> None: + """A hold is a pause: a later conduct_from resumes against the state the + main steps established, so closing (which would tear that down) must + not run.""" + port, inner = _routed_port("2bma:shutter", "2bma:closing") + inner.set_reading("2bma:rot:rbv", _good_reading(12.5)) + appender = _FakeAppendStep() + hold = _FakeLifecycleHandler() + conductor = _conductor_hold_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + hold=hold, + ids=[uuid4() for _ in range(4)], + ) + + result = await conductor.conduct_or_hold( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=( + SetpointStep(address="2bma:shutter", value=1), + CheckStep(address="2bma:rot:rbv", criterion=EqualsCriterion(expected=45.0)), + ), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.held is True + assert len(hold.calls) == 1 + assert dict(result.substrate_writes) == {"2bma:shutter": 1} # NOT "2bma:closing" + + +@pytest.mark.unit +async def test_conduct_or_hold_non_recoverable_failure_aborts_then_runs_closing_steps() -> None: + """An acquisition (action) failure is non-recoverable: abort, not hold -- + and closing runs on that abort, exactly like conduct().""" + registry = InMemoryActionRegistry({}) # "collect" is unregistered -> UnknownActionError + port, inner = _routed_port("2bma:closing") + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + hold = _FakeLifecycleHandler() + conductor = Conductor( + control_port=port, + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + action_registry=registry, + start_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=abort, + hold_procedure=hold, + ) + + result = await conductor.conduct_or_hold( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(ActionStep(name="collect"),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.held is False + assert result.succeeded is False + assert len(abort.calls) == 1 + assert hold.calls == [] + landed = await inner.read("2bma:closing") + assert landed.value == 0.0 + + @pytest.mark.unit async def test_conduct_or_hold_complete_rejection_still_reports_what_was_left_set() -> None: """conduct_or_hold's complete arm carries the same obligation as conduct's.""" @@ -2188,6 +2511,37 @@ async def test_conduct_or_hold_complete_rejection_still_reports_what_was_left_se assert result.actuation_kind is ActuationKind.PHYSICAL +@pytest.mark.unit +async def test_conduct_or_hold_complete_rejected_still_reports_closing_that_already_ran() -> None: + """Same correction as conduct()'s twin: closing runs before the + complete_procedure attempt, so a subsequent rejection does not undo it.""" + port, _ = _routed_port("2bma:shutter", "2bma:closing") + appender = _FakeAppendStep() + conductor = _conductor_hold_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(raises=RuntimeError("complete rejected")), + abort=_FakeLifecycleHandler(), + hold=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(4)], + ) + + result = await conductor.conduct_or_hold( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:shutter", value=1),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is False + assert result.failure is not None + assert result.failure.target == "complete" + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:shutter": 1, "2bma:closing": 0.0} + + @pytest.mark.unit async def test_conduct_or_hold_held_procedure_reports_what_was_left_set() -> None: """A Held Procedure is parked mid-flight awaiting an operator decision, @@ -2253,3 +2607,126 @@ async def test_conduct_from_complete_rejection_reports_the_merged_kind_and_ledge assert result.failure.target == "complete" assert dict(result.substrate_writes) == {"2bma:shutter": 1} assert result.actuation_kind is ActuationKind.PHYSICAL + + +@pytest.mark.unit +async def test_conduct_from_complete_rejected_still_reports_closing_that_already_ran() -> None: + """Same correction as conduct()'s twin: closing runs before the + complete_procedure attempt, so a subsequent rejection does not undo it.""" + port, _ = _routed_port("2bma:shutter", "2bma:closing") + appender = _FakeAppendStep() + conductor = Conductor( + control_port=port, + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + resume_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(raises=RuntimeError("complete rejected")), + abort_procedure=_FakeLifecycleHandler(), + ) + + result = await conductor.conduct_from( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:shutter", value=1),), + boundary=0, + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is False + assert result.failure is not None + assert result.failure.target == "complete" + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:shutter": 1, "2bma:closing": 0.0} + + +@pytest.mark.unit +async def test_conduct_from_clean_tail_completes_with_closing_steps_applied() -> None: + port, _ = _routed_port("2bma:shutter", "2bma:closing") + appender = _FakeAppendStep() + conductor = Conductor( + control_port=port, + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + resume_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=_FakeLifecycleHandler(), + ) + + result = await conductor.conduct_from( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:shutter", value=1),), + boundary=0, + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is True + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:shutter": 1, "2bma:closing": 0.0} + + +@pytest.mark.unit +async def test_conduct_from_acquisition_halt_does_not_run_closing_steps() -> None: + """The replay tail hits an ActionStep: halt-for-operator, no transition. + Closing must not foreclose the redo-fresh-vs-reseed decision.""" + port, _ = _routed_port("2bma:closing") + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + conductor = Conductor( + control_port=port, + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + resume_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=abort, + ) + + result = await conductor.conduct_from( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(ActionStep(name="collect"),), + boundary=0, + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is False + assert result.failure is not None + assert result.failure.error_class == "AcquisitionResumeRequiresOperator" + assert abort.calls == [] + assert dict(result.substrate_writes) == {} # closing never touched the port + + +@pytest.mark.unit +async def test_conduct_from_genuine_failure_aborts_then_runs_closing_steps() -> None: + port, _ = _routed_port("2bma:closing") # "2bma:missing" is NOT connected + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + conductor = Conductor( + control_port=port, + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + resume_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=abort, + ) + + result = await conductor.conduct_from( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:missing", value=1.0),), + boundary=0, + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + + assert result.succeeded is False + assert len(abort.calls) == 1 + assert result.closing_failures == () + assert dict(result.substrate_writes) == {"2bma:closing": 0.0} From 8b6a178167b08c720699c7d932c4fc020488d0aa Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sun, 30 Aug 2026 06:23:50 -0500 Subject: [PATCH 5/7] Thread closing_steps through the three conduct slices and refuse it in loops conduct_procedure / conduct_or_hold_procedure / conduct_from_procedure now pass resolve_and_pin_conduct_steps's resolved closing_steps into their Conductor.conduct*() calls instead of discarding it -- Commit 6a's _run_closing had no caller handing it anything to walk until this landed. conduct_from also gains ClosingCaptureBeforeBoundaryError: a closing step's CaptureRef naming a capture only a pre-boundary main step declares would otherwise resolve against nothing (captures start empty on resume) and fail deep inside _run_closing's per-step isolation, silently converting a should-be-loud gap into a recorded closing failure. Checked up front instead, 422, before any FSM event fires. The three loop-driving slices (conduct_until_converged, conduct_until_advised, conduct_until_advised_from) refuse a closing-bearing Recipe outright via the new UnsupportedClosingStepsError (422): a loop that re-walks one pass block repeatedly has no defined place to run a once-per-conduct closing walk. The resume-direction check reads resolved_closing_steps off the already-pinned record rather than re-loading the Recipe, since conduct_until_advised's own forward call already pinned it. Finally, ConductorResult.closing_failures rides all the way to the wire: the three conduct-family commands, REST responses, and MCP tool results all gain the field, mirroring substrate_writes's existing shape so the tool/response parity fitness test covers it for free. --- apps/api/openapi.json | 28 +++- apps/api/src/cora/operation/_conduct_wire.py | 14 +- apps/api/src/cora/operation/errors.py | 55 ++++++ .../conduct_from_procedure/command.py | 8 + .../conduct_from_procedure/handler.py | 50 +++++- .../features/conduct_from_procedure/route.py | 15 +- .../features/conduct_from_procedure/tool.py | 8 + .../conduct_or_hold_procedure/command.py | 7 + .../conduct_or_hold_procedure/handler.py | 7 +- .../conduct_or_hold_procedure/route.py | 10 ++ .../conduct_or_hold_procedure/tool.py | 7 + .../features/conduct_procedure/command.py | 17 +- .../features/conduct_procedure/handler.py | 7 +- .../features/conduct_procedure/route.py | 22 ++- .../features/conduct_procedure/tool.py | 8 + .../features/conduct_until_advised/handler.py | 18 +- .../conduct_until_advised_from/handler.py | 8 + .../conduct_until_converged/handler.py | 15 +- apps/api/src/cora/operation/routes.py | 12 ++ .../test_conduct_from_procedure_endpoint.py | 68 ++++++++ .../test_conduct_until_advised_endpoint.py | 70 ++++++++ .../test_conduct_from_procedure_handler.py | 84 +++++++++- .../test_conduct_or_hold_procedure_handler.py | 156 ++++++++++++++++++ .../test_conduct_procedure_handler.py | 6 + ...test_conduct_until_advised_from_handler.py | 29 +++- .../test_conduct_until_converged_handler.py | 130 ++++++++++++++- 26 files changed, 815 insertions(+), 44 deletions(-) diff --git a/apps/api/openapi.json b/apps/api/openapi.json index 033e1917f8d..4563889f095 100644 --- a/apps/api/openapi.json +++ b/apps/api/openapi.json @@ -3874,6 +3874,14 @@ ], "title": "Actuation Kind" }, + "closing_failures": { + "description": "Every closing step that failed. Always empty on `acquisition_halt=True`. Never flips `succeeded`.", + "items": { + "$ref": "#/components/schemas/ConductorFailureResponse" + }, + "title": "Closing Failures", + "type": "array" + }, "completed_count": { "title": "Completed Count", "type": "integer" @@ -3986,6 +3994,14 @@ ], "title": "Actuation Kind" }, + "closing_failures": { + "description": "Every closing step that failed. Always empty on `held=True`: closing runs only on a real terminal, never on Held. Never flips `succeeded`.", + "items": { + "$ref": "#/components/schemas/ConductorFailureResponse" + }, + "title": "Closing Failures", + "type": "array" + }, "completed_count": { "title": "Completed Count", "type": "integer" @@ -4097,6 +4113,14 @@ ], "title": "Actuation Kind" }, + "closing_failures": { + "description": "Every closing step that failed. Isolated from `failure`: a closing failure never flips `succeeded`, and one closing step failing does not stop the rest of the closing walk. Empty when the recipe has no closing steps, or all ran clean.", + "items": { + "$ref": "#/components/schemas/ConductorFailureResponse" + }, + "title": "Closing Failures", + "type": "array" + }, "completed_count": { "title": "Completed Count", "type": "integer" @@ -4137,7 +4161,7 @@ } ] }, - "description": "Every control address this conduct wrote, in first-write order, carrying the last value written to each. CORA does not restore what it sets, and a halt returns at the failing step without running the recipe's remaining steps, so on a failed conduct this is what was left set with nothing having put it back. Reports what was WRITTEN, not what changed: a write whose value already matched the address still appears.", + "description": "Every control address this conduct wrote, in first-write order, carrying the last value written to each. Includes the recipe's closing steps, which run on a real terminal (Completed or Aborted). Reports what was WRITTEN, not what changed: a write whose value already matched the address still appears.", "title": "Substrate Writes", "type": "object" }, @@ -40965,7 +40989,7 @@ "description": "Procedure is not in `Held` status, OR its parent Run is itself `Held` (off-diagonal guard)." }, "422": { - "description": "Path parameter or request body failed schema validation." + "description": "Path parameter or request body failed schema validation, OR a closing step's CaptureRef names a capture only a pre-boundary main step declares (ClosingCaptureBeforeBoundaryError)." }, "500": { "content": { diff --git a/apps/api/src/cora/operation/_conduct_wire.py b/apps/api/src/cora/operation/_conduct_wire.py index 6ed2d4f00ee..4ce11aeb919 100644 --- a/apps/api/src/cora/operation/_conduct_wire.py +++ b/apps/api/src/cora/operation/_conduct_wire.py @@ -22,7 +22,7 @@ tuple for the in-process Conductor. """ -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from typing import Annotated, Any, Literal, cast from pydantic import BaseModel, Field @@ -192,3 +192,15 @@ def substrate_writes_to_wire( `result_to_wire` (conduct / conduct_or_hold / conduct_from) now that a third call site would otherwise repeat it.""" return {k: list(v) if isinstance(v, tuple) else v for k, v in substrate_writes.items()} + + +def closing_failures_to_wire( + closing_failures: Sequence[ConductorFailure], +) -> list[ConductorFailureResponse]: + """Project `ConductorResult.closing_failures` onto its JSON wire shape. + + Reuses `failure_to_wire` per entry -- a closing failure has the exact + same shape as the main-walk `failure`, just isolated into its own list + rather than halting the walk. Empty by default: most conducts have no + closing steps at all, or every closing step ran clean.""" + return [failure_to_wire(f) for f in closing_failures] diff --git a/apps/api/src/cora/operation/errors.py b/apps/api/src/cora/operation/errors.py index d62bdeb6c5d..8c03a50a52e 100644 --- a/apps/api/src/cora/operation/errors.py +++ b/apps/api/src/cora/operation/errors.py @@ -13,6 +13,8 @@ (documented in CONTRIBUTING.md "BC-application-layer errors"). """ +from uuid import UUID + class UnauthorizedError(Exception): """The Authorize port denied the command.""" @@ -151,6 +153,59 @@ def __init__(self, reason: str) -> None: self.reason = reason +class ClosingCaptureBeforeBoundaryError(Exception): + """A closing step's `CaptureRef` names a capture only the pre-boundary + main steps declare. + + `conduct_from` starts the per-conduct `captures` dict EMPTY: only the + main steps from `boundary` onward re-run and can deposit into it (the + same "fails loud rather than resolving against stale data" contract + `execute_from` already holds for a main-step `CaptureRef`). Closing + steps always run in full regardless of `boundary`, so a closing + `CaptureRef` whose only declaring `CaptureStep` / capturing + `ComputeStep` sits before `boundary` would resolve against nothing + during THIS resume and fail as `UnresolvedCaptureRef` -- but inside + `_run_closing`'s per-step isolation, that failure is recorded and the + walk continues, silently converting a should-be-loud gap into a + recorded-and-continue one. Checked up front instead, so the operator + sees a 422 naming the missing capture and can pick a boundary at or + before the declaring step, rather than a closing_failures entry after + the fact. + """ + + def __init__(self, capture_name: str, boundary: int) -> None: + super().__init__( + f"closing step references capture {capture_name!r}, which only a " + f"pre-boundary main step (boundary={boundary}) declares; resume " + "starts captures empty, so this closing step would never see it" + ) + self.capture_name = capture_name + self.boundary = boundary + + +class UnsupportedClosingStepsError(Exception): + """A loop-driving conduct slice refuses a closing-bearing Recipe (v1 scope). + + `conduct_until_converged` / `conduct_until_advised` / `conduct_until_advised_from` + each re-walk ONE pass block repeatedly (loop-top abort, per-iteration + re-expansion); `_run_closing` runs once, on a real conduct terminal, and + has no defined place in a loop that may never terminate the way `conduct` + /`conduct_or_hold` do. Rather than silently drop the Recipe's closing + steps or guess when to run them, these three slices refuse the request + up front: well-formed, but this Recipe cannot be driven by a loop slice + until closing-in-a-loop is designed. Mapped to HTTP 422 (operator- + correctable: use `conduct` / `conduct_or_hold` for this Recipe, or + author a closing-less variant for loop-driven conduct). + """ + + def __init__(self, procedure_id: UUID) -> None: + super().__init__( + f"procedure {procedure_id} is bound to a Recipe with closing_steps, " + "which loop-driving conduct slices do not support" + ) + self.procedure_id = procedure_id + + class CheckFailedError(Exception): """A `CheckStep` either read a non-Good quality or its criterion did not match. diff --git a/apps/api/src/cora/operation/features/conduct_from_procedure/command.py b/apps/api/src/cora/operation/features/conduct_from_procedure/command.py index 841847cd52a..c821723a4ed 100644 --- a/apps/api/src/cora/operation/features/conduct_from_procedure/command.py +++ b/apps/api/src/cora/operation/features/conduct_from_procedure/command.py @@ -57,3 +57,11 @@ class ConductFromProcedureResult: artifacts: tuple[ArtifactRef, ...] = () outputs: Mapping[str, ArtifactRef] = field(default_factory=dict[str, ArtifactRef]) substrate_writes: Mapping[str, WriteValue] = field(default_factory=dict[str, WriteValue]) + closing_failures: tuple[ConductorFailure, ...] = () + """Every closing step that failed, threaded from `ConductorResult.closing_failures`. + + Always empty on `acquisition_halt=True`: closing runs only on a real + terminal (complete / abort), never on the halt-for-operator branch that + leaves the Procedure Running. Isolated from `failure`: never flips + `succeeded`. + """ diff --git a/apps/api/src/cora/operation/features/conduct_from_procedure/handler.py b/apps/api/src/cora/operation/features/conduct_from_procedure/handler.py index e2693b5effd..b5269d65300 100644 --- a/apps/api/src/cora/operation/features/conduct_from_procedure/handler.py +++ b/apps/api/src/cora/operation/features/conduct_from_procedure/handler.py @@ -64,18 +64,48 @@ ResolvedStepsRecordNotFoundError, load_procedure_with_events, ) -from cora.operation.conductor import Conductor, is_acquisition_halt, steps_from_payload -from cora.operation.errors import UnauthorizedError +from cora.operation.conductor import ( + CaptureStep, + ComputeStep, + Conductor, + SetpointStep, + Step, + is_acquisition_halt, + steps_from_payload, +) +from cora.operation.errors import ClosingCaptureBeforeBoundaryError, UnauthorizedError from cora.operation.features.conduct_from_procedure.command import ( ConductFromProcedure, ConductFromProcedureResult, ) +from cora.recipe.aggregates.recipe.body import CaptureRef _COMMAND_NAME = "ConductFromProcedure" _log = get_logger(__name__) +def _capture_names_declared_from(steps: tuple[Step, ...], boundary: int) -> frozenset[str]: + """Capture names a main step at or after `boundary` deposits. + + Only these are guaranteed to be populated during THIS resume: `captures` + starts empty and the tail from `boundary` is all that re-runs.""" + declared: set[str] = set() + for step in steps[boundary:]: + if isinstance(step, (CaptureStep, ComputeStep)) and step.capture_name is not None: + declared.add(step.capture_name) + return frozenset(declared) + + +def _closing_capture_refs(closing_steps: tuple[Step, ...]) -> frozenset[str]: + """Capture names a closing step's `SetpointStep.value` reads via `CaptureRef`.""" + return frozenset( + step.value.capture_name + for step in closing_steps + if isinstance(step, SetpointStep) and isinstance(step.value, CaptureRef) + ) + + class Handler(Protocol): """Callable interface every conduct_from_procedure handler implements.""" @@ -157,6 +187,7 @@ async def handler( if record is None: raise ResolvedStepsRecordNotFoundError(command.procedure_id) steps = steps_from_payload(record.payload["resolved_steps"]) + closing_steps = steps_from_payload(record.payload.get("resolved_closing_steps", ())) # Upper-bound guard: a boundary PAST the pinned step count would replay # an empty tail and silently auto-complete with nothing re-driven. The @@ -167,11 +198,25 @@ async def handler( if command.re_establishment_boundary > len(steps): raise InvalidProcedureReEstablishmentBoundaryError(command.re_establishment_boundary) + # A closing CaptureRef must resolve against THIS resume's captures, + # which start empty and fill only from `boundary` onward -- a name + # only a pre-boundary main step declares would otherwise fail deep + # inside _run_closing's per-step isolation instead of up front. + # See ClosingCaptureBeforeBoundaryError. + missing_captures = _closing_capture_refs(closing_steps) - _capture_names_declared_from( + steps, command.re_establishment_boundary + ) + if missing_captures: + raise ClosingCaptureBeforeBoundaryError( + sorted(missing_captures)[0], command.re_establishment_boundary + ) + result = await conductor.conduct_from( procedure_id=command.procedure_id, principal_id=principal_id, correlation_id=correlation_id, steps=steps, + closing_steps=closing_steps, boundary=command.re_establishment_boundary, # The pre-hold conduct's observed kind (folded onto the Held # Procedure) so the terminal event reflects the FULL provenance, @@ -207,6 +252,7 @@ async def handler( artifacts=result.artifacts, outputs=result.outputs, substrate_writes=result.substrate_writes, + closing_failures=result.closing_failures, ) return handler diff --git a/apps/api/src/cora/operation/features/conduct_from_procedure/route.py b/apps/api/src/cora/operation/features/conduct_from_procedure/route.py index ac662429ae7..a9ec1605935 100644 --- a/apps/api/src/cora/operation/features/conduct_from_procedure/route.py +++ b/apps/api/src/cora/operation/features/conduct_from_procedure/route.py @@ -28,6 +28,7 @@ ) from cora.operation._conduct_wire import ( ConductorFailureResponse, + closing_failures_to_wire, failure_to_wire, substrate_writes_to_wire, ) @@ -79,6 +80,13 @@ class ConductFromProcedureResponse(BaseModel): "setpoints re-driven and nothing having put them back." ), ) + closing_failures: list[ConductorFailureResponse] = Field( + default_factory=list[ConductorFailureResponse], + description=( + "Every closing step that failed. Always empty on " + "`acquisition_halt=True`. Never flips `succeeded`." + ), + ) def result_to_wire(result: ConductFromProcedureResult) -> ConductFromProcedureResponse: @@ -95,6 +103,7 @@ def result_to_wire(result: ConductFromProcedureResult) -> ConductFromProcedureRe failure=failure_to_wire(result.failure) if result.failure is not None else None, actuation_kind=result.actuation_kind, substrate_writes=substrate_writes_to_wire(result.substrate_writes), + closing_failures=closing_failures_to_wire(result.closing_failures), ) @@ -131,7 +140,11 @@ def _get_handler(request: Request) -> Handler: ), }, status.HTTP_422_UNPROCESSABLE_CONTENT: { - "description": "Path parameter or request body failed schema validation.", + "description": ( + "Path parameter or request body failed schema validation, OR " + "a closing step's CaptureRef names a capture only a " + "pre-boundary main step declares (ClosingCaptureBeforeBoundaryError)." + ), }, status.HTTP_500_INTERNAL_SERVER_ERROR: { "model": ErrorResponse, diff --git a/apps/api/src/cora/operation/features/conduct_from_procedure/tool.py b/apps/api/src/cora/operation/features/conduct_from_procedure/tool.py index 4623b8ec25a..42e7117b1c1 100644 --- a/apps/api/src/cora/operation/features/conduct_from_procedure/tool.py +++ b/apps/api/src/cora/operation/features/conduct_from_procedure/tool.py @@ -42,6 +42,13 @@ class _ToolResult(BaseModel): "acquisition halt too." ), ) + closing_failures: list[dict[str, Any]] = Field( + default_factory=list[dict[str, Any]], + description=( + "Every closing step that failed. Always empty on an acquisition " + "halt. Never flips `succeeded`." + ), + ) def register(mcp: FastMCP, *, get_handler: Callable[[], Handler]) -> None: @@ -97,4 +104,5 @@ async def conduct_from_procedure_tool( # pyright: ignore[reportUnusedFunction] failure=wire.failure.model_dump() if wire.failure is not None else None, actuation_kind=wire.actuation_kind, substrate_writes=wire.substrate_writes, + closing_failures=[f.model_dump() for f in wire.closing_failures], ) diff --git a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/command.py b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/command.py index 232af681cb1..e1624ebec26 100644 --- a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/command.py +++ b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/command.py @@ -57,3 +57,10 @@ class ConductOrHoldProcedureResult: artifacts: tuple[ArtifactRef, ...] = () outputs: Mapping[str, ArtifactRef] = field(default_factory=dict[str, ArtifactRef]) substrate_writes: Mapping[str, WriteValue] = field(default_factory=dict[str, WriteValue]) + closing_failures: tuple[ConductorFailure, ...] = () + """Every closing step that failed, threaded from `ConductorResult.closing_failures`. + + Always empty on a `held=True` outcome: closing runs only on a real + terminal (Completed / Aborted), never on Held. Isolated from `failure`: + a closing failure never flips `succeeded`. + """ diff --git a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py index d629b472cc9..7839f492be6 100644 --- a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py +++ b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/handler.py @@ -121,10 +121,7 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - # `_closing_steps`: resolved and pinned onto ResolvedStepsRecorded - # above, but not yet handed to the Conductor -- that lands with - # _run_closing. See [[project_conduct_closing_steps_design]]. - steps, _closing_steps = await resolve_and_pin_conduct_steps( + steps, closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, @@ -143,6 +140,7 @@ async def handler( causation_id=causation_id, surface_id=surface_id, steps=steps, + closing_steps=closing_steps, ) _log.info( @@ -168,6 +166,7 @@ async def handler( artifacts=result.artifacts, outputs=result.outputs, substrate_writes=result.substrate_writes, + closing_failures=result.closing_failures, ) return handler diff --git a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/route.py b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/route.py index e8946b12430..afad9856c50 100644 --- a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/route.py +++ b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/route.py @@ -37,6 +37,7 @@ STEP_BATCH_MAX, ConductorFailureResponse, StepRequest, + closing_failures_to_wire, failure_to_wire, step_from_wire, substrate_writes_to_wire, @@ -92,6 +93,14 @@ class ConductOrHoldProcedureResponse(BaseModel): "have not run, so this is what was left set." ), ) + closing_failures: list[ConductorFailureResponse] = Field( + default_factory=list[ConductorFailureResponse], + description=( + "Every closing step that failed. Always empty on `held=True`: " + "closing runs only on a real terminal, never on Held. Never " + "flips `succeeded`." + ), + ) def result_to_wire(result: ConductOrHoldProcedureResult) -> ConductOrHoldProcedureResponse: @@ -107,6 +116,7 @@ def result_to_wire(result: ConductOrHoldProcedureResult) -> ConductOrHoldProcedu failure=failure_to_wire(result.failure) if result.failure is not None else None, actuation_kind=result.actuation_kind, substrate_writes=substrate_writes_to_wire(result.substrate_writes), + closing_failures=closing_failures_to_wire(result.closing_failures), ) diff --git a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/tool.py b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/tool.py index 615dcdbffc2..9c13a8db3fd 100644 --- a/apps/api/src/cora/operation/features/conduct_or_hold_procedure/tool.py +++ b/apps/api/src/cora/operation/features/conduct_or_hold_procedure/tool.py @@ -44,6 +44,12 @@ class _ToolResult(BaseModel): "outcome too: a paused Procedure's closing steps have not run." ), ) + closing_failures: list[dict[str, Any]] = Field( + default_factory=list[dict[str, Any]], + description=( + "Every closing step that failed. Always empty on `held=True`. Never flips `succeeded`." + ), + ) def register(mcp: FastMCP, *, get_handler: Callable[[], Handler]) -> None: @@ -92,4 +98,5 @@ async def conduct_or_hold_procedure_tool( # pyright: ignore[reportUnusedFunctio failure=wire.failure.model_dump() if wire.failure is not None else None, actuation_kind=wire.actuation_kind, substrate_writes=wire.substrate_writes, + closing_failures=[f.model_dump() for f in wire.closing_failures], ) diff --git a/apps/api/src/cora/operation/features/conduct_procedure/command.py b/apps/api/src/cora/operation/features/conduct_procedure/command.py index a41859f17d5..f1a4c4c47d5 100644 --- a/apps/api/src/cora/operation/features/conduct_procedure/command.py +++ b/apps/api/src/cora/operation/features/conduct_procedure/command.py @@ -75,10 +75,15 @@ class ConductProcedureResult: substrate_writes: Mapping[str, WriteValue] = field(default_factory=dict[str, WriteValue]) """Every address the conduct wrote, in first-write order, last value. - Threaded from `ConductorResult.substrate_writes`. On a HALT this is - what CORA left set: the step loop returns at the failing step, so a - recipe's own closing steps never run and nothing restores what - earlier steps changed. Surfacing it on the response means an - operator reads it in the same breath as the failure, instead of - reconstructing it from the step journal. + Threaded from `ConductorResult.substrate_writes`. Includes the Recipe's + closing steps, which run on a real terminal (Completed or Aborted) and + merge their own writes into this ledger. + """ + closing_failures: tuple[ConductorFailure, ...] = () + """Every closing step that failed, threaded from `ConductorResult.closing_failures`. + + Isolated from `failure`: a closing failure never flips `succeeded`, and + one closing step failing does not stop the rest of the closing walk. + Empty when the bound Recipe has no closing steps, or every closing step + ran clean. """ diff --git a/apps/api/src/cora/operation/features/conduct_procedure/handler.py b/apps/api/src/cora/operation/features/conduct_procedure/handler.py index e31d795a68f..6ce0cf440d6 100644 --- a/apps/api/src/cora/operation/features/conduct_procedure/handler.py +++ b/apps/api/src/cora/operation/features/conduct_procedure/handler.py @@ -139,10 +139,7 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - # `_closing_steps`: resolved and pinned onto ResolvedStepsRecorded - # above, but not yet handed to the Conductor -- that lands with - # _run_closing. See [[project_conduct_closing_steps_design]]. - steps, _closing_steps = await resolve_and_pin_conduct_steps( + steps, closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, @@ -161,6 +158,7 @@ async def handler( causation_id=causation_id, surface_id=surface_id, steps=steps, + closing_steps=closing_steps, ) _log.info( @@ -184,6 +182,7 @@ async def handler( artifacts=result.artifacts, outputs=result.outputs, substrate_writes=result.substrate_writes, + closing_failures=result.closing_failures, ) return handler diff --git a/apps/api/src/cora/operation/features/conduct_procedure/route.py b/apps/api/src/cora/operation/features/conduct_procedure/route.py index 865acc5e59e..08e6ddaf946 100644 --- a/apps/api/src/cora/operation/features/conduct_procedure/route.py +++ b/apps/api/src/cora/operation/features/conduct_procedure/route.py @@ -45,6 +45,7 @@ STEP_BATCH_MAX, ConductorFailureResponse, StepRequest, + closing_failures_to_wire, failure_to_wire, step_from_wire, substrate_writes_to_wire, @@ -94,12 +95,20 @@ class ConductProcedureResponse(BaseModel): default_factory=dict[str, int | float | bool | str | list[Any]], description=( "Every control address this conduct wrote, in first-write " - "order, carrying the last value written to each. CORA does " - "not restore what it sets, and a halt returns at the failing " - "step without running the recipe's remaining steps, so on a " - "failed conduct this is what was left set with nothing having " - "put it back. Reports what was WRITTEN, not what changed: a " - "write whose value already matched the address still appears." + "order, carrying the last value written to each. Includes the " + "recipe's closing steps, which run on a real terminal " + "(Completed or Aborted). Reports what was WRITTEN, not what " + "changed: a write whose value already matched the address " + "still appears." + ), + ) + closing_failures: list[ConductorFailureResponse] = Field( + default_factory=list[ConductorFailureResponse], + description=( + "Every closing step that failed. Isolated from `failure`: a " + "closing failure never flips `succeeded`, and one closing " + "step failing does not stop the rest of the closing walk. " + "Empty when the recipe has no closing steps, or all ran clean." ), ) @@ -116,6 +125,7 @@ def result_to_wire(result: ConductProcedureResult) -> ConductProcedureResponse: failure=failure_to_wire(result.failure) if result.failure is not None else None, actuation_kind=result.actuation_kind, substrate_writes=substrate_writes_to_wire(result.substrate_writes), + closing_failures=closing_failures_to_wire(result.closing_failures), ) diff --git a/apps/api/src/cora/operation/features/conduct_procedure/tool.py b/apps/api/src/cora/operation/features/conduct_procedure/tool.py index 8a57a5ccd18..5a71b291541 100644 --- a/apps/api/src/cora/operation/features/conduct_procedure/tool.py +++ b/apps/api/src/cora/operation/features/conduct_procedure/tool.py @@ -43,6 +43,13 @@ class _ToolResult(BaseModel): "it back." ), ) + closing_failures: list[dict[str, Any]] = Field( + default_factory=list[dict[str, Any]], + description=( + "Every closing step that failed. Never flips `succeeded`; empty " + "when the recipe has no closing steps, or all ran clean." + ), + ) def register(mcp: FastMCP, *, get_handler: Callable[[], Handler]) -> None: @@ -89,4 +96,5 @@ async def conduct_procedure_tool( # pyright: ignore[reportUnusedFunction] failure=wire.failure.model_dump() if wire.failure is not None else None, actuation_kind=wire.actuation_kind, substrate_writes=wire.substrate_writes, + closing_failures=[f.model_dump() for f in wire.closing_failures], ) diff --git a/apps/api/src/cora/operation/features/conduct_until_advised/handler.py b/apps/api/src/cora/operation/features/conduct_until_advised/handler.py index 8918ec4b213..9d40e552979 100644 --- a/apps/api/src/cora/operation/features/conduct_until_advised/handler.py +++ b/apps/api/src/cora/operation/features/conduct_until_advised/handler.py @@ -42,7 +42,11 @@ load_procedure_with_events, ) from cora.operation.conductor import Conductor -from cora.operation.errors import SteeringWireMismatchError, UnauthorizedError +from cora.operation.errors import ( + SteeringWireMismatchError, + UnauthorizedError, + UnsupportedClosingStepsError, +) from cora.operation.features.conduct_until_advised.command import ( ConductUntilAdvised, ConductUntilAdvisedResult, @@ -134,11 +138,7 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - # `_closing_steps` is unused here by design: this loop refuses a - # closing-bearing Recipe outright (v1 scope; see - # [[project_conduct_closing_steps_design]]), so it never reaches a - # non-empty value past that guard. - steps, _closing_steps = await resolve_and_pin_conduct_steps( + steps, closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, @@ -149,6 +149,12 @@ async def handler( correlation_id=correlation_id, causation_id=causation_id, ) + # v1 scope: a loop that re-walks one pass block repeatedly has no + # defined place to run _run_closing. Refuse outright rather than + # silently drop the Recipe's closing steps or guess when to run + # them. See [[project_conduct_closing_steps_design]]. + if closing_steps: + raise UnsupportedClosingStepsError(command.procedure_id) llm_calls: list[SteeringLlmCall] = [] decide_port = build_decide_port( diff --git a/apps/api/src/cora/operation/features/conduct_until_advised_from/handler.py b/apps/api/src/cora/operation/features/conduct_until_advised_from/handler.py index 20733b13f4b..c293af28fa8 100644 --- a/apps/api/src/cora/operation/features/conduct_until_advised_from/handler.py +++ b/apps/api/src/cora/operation/features/conduct_until_advised_from/handler.py @@ -66,6 +66,7 @@ from cora.operation.errors import ( SteeringWireMismatchError, UnauthorizedError, + UnsupportedClosingStepsError, ) from cora.operation.features.conduct_until_advised_from.command import ( ConductUntilAdvisedFrom, @@ -175,6 +176,13 @@ async def handler( if record is None: raise ResolvedStepsRecordNotFoundError(command.procedure_id) steps = steps_from_payload(record.payload["resolved_steps"]) + # v1 scope, mirroring conduct_until_advised's forward-direction refusal: + # the original pinned record already carries resolved_closing_steps + # when the bound Recipe has any (conduct_until_advised's own + # resolve_and_pin_conduct_steps call pinned it), so no fresh Recipe + # load is needed here. See [[project_conduct_closing_steps_design]]. + if record.payload.get("resolved_closing_steps"): + raise UnsupportedClosingStepsError(command.procedure_id) # RECONSTRUCT the brain's history from the self-describing outcome rows # (each carries its own point + measurements), so this is a sort-then-map diff --git a/apps/api/src/cora/operation/features/conduct_until_converged/handler.py b/apps/api/src/cora/operation/features/conduct_until_converged/handler.py index 877ba49cf97..a8bb8005b04 100644 --- a/apps/api/src/cora/operation/features/conduct_until_converged/handler.py +++ b/apps/api/src/cora/operation/features/conduct_until_converged/handler.py @@ -44,7 +44,7 @@ load_procedure_with_events, ) from cora.operation.conductor import Conductor -from cora.operation.errors import UnauthorizedError +from cora.operation.errors import UnauthorizedError, UnsupportedClosingStepsError from cora.operation.features.conduct_until_converged.command import ( ConductUntilConverged, ConductUntilConvergedResult, @@ -127,11 +127,7 @@ async def handler( if procedure is None: raise ProcedureNotFoundError(command.procedure_id) - # `_closing_steps` is unused here by design: this loop refuses a - # closing-bearing Recipe outright (v1 scope; see - # [[project_conduct_closing_steps_design]]), so it never reaches a - # non-empty value past that guard. - steps, _closing_steps = await resolve_and_pin_conduct_steps( + steps, closing_steps = await resolve_and_pin_conduct_steps( deps, command_name=_COMMAND_NAME, procedure=procedure, @@ -142,6 +138,13 @@ async def handler( correlation_id=correlation_id, causation_id=causation_id, ) + # v1 scope: a loop that re-walks one pass block repeatedly has no + # defined place to run _run_closing (it runs once, on a real conduct + # terminal). Refuse outright rather than silently drop the Recipe's + # closing steps or guess when to run them. See + # [[project_conduct_closing_steps_design]]. + if closing_steps: + raise UnsupportedClosingStepsError(command.procedure_id) # The command's explicit cap overrides the registered one only when # supplied; otherwise the loop honors the Procedure's declared diff --git a/apps/api/src/cora/operation/routes.py b/apps/api/src/cora/operation/routes.py index 6073d3feb18..1d9dbc00ce3 100644 --- a/apps/api/src/cora/operation/routes.py +++ b/apps/api/src/cora/operation/routes.py @@ -78,6 +78,7 @@ ) from cora.operation.errors import ( AssetNotPseudoAxisError, + ClosingCaptureBeforeBoundaryError, PartitionRuleNotFoundError, PseudoAxisCommandOutsideRangeError, PseudoAxisConstituentDispatchError, @@ -87,6 +88,7 @@ PseudoAxisSingularityExceededError, SteeringWireMismatchError, UnauthorizedError, + UnsupportedClosingStepsError, ) from cora.operation.features import ( abort_procedure, @@ -365,6 +367,16 @@ def register_operation_routes(app: FastAPI) -> None: # setpoints (the Conductor's pre-FSM wire guard). Well-formed request, # unprocessable against the recipe; operator aligns the space + retries. SteeringWireMismatchError, + # conduct_from: a closing step's CaptureRef names a capture only a + # pre-boundary main step declares -- resume starts captures empty, so + # this resume would never populate it. Operator picks a boundary at + # or before the declaring step, or drops the closing CaptureRef. + ClosingCaptureBeforeBoundaryError, + # The three loop-driving conduct slices (conduct_until_converged, + # conduct_until_advised, conduct_until_advised_from) refuse a + # closing-bearing Recipe outright: _run_closing has no defined place + # in a loop that re-walks one pass block repeatedly (v1 scope). + UnsupportedClosingStepsError, ): app.add_exception_handler(unprocessable_cls, _handle_unprocessable) # Server-side determinism bugs / data corruption: HTTP 500. Distinct diff --git a/apps/api/tests/contract/test_conduct_from_procedure_endpoint.py b/apps/api/tests/contract/test_conduct_from_procedure_endpoint.py index 378a720bd71..1a2c9ebbc1e 100644 --- a/apps/api/tests/contract/test_conduct_from_procedure_endpoint.py +++ b/apps/api/tests/contract/test_conduct_from_procedure_endpoint.py @@ -172,6 +172,74 @@ def test_post_conduct_from_returns_400_for_boundary_past_step_count() -> None: assert response.status_code == 400 +def _register_from_recipe_with_closing_capture(client: TestClient) -> UUID: + """Recipe whose closing step reads a capture only its SECOND main step + declares. The failing setpoint at index 0 is recoverable (Held); the + CaptureStep at index 1 never runs during that pass but is still part of + the pinned resolved steps.""" + cap = client.post( + "/capabilities", + json={ + "code": "cora.capability.closing_capture_recipe", + "name": "ClosingCaptureCap", + "required_affordances": [], + "executor_shapes": ["Procedure"], + }, + ).json() + recipe = client.post( + "/recipes", + json={ + "name": "closing capture recipe", + "capability_id": cap["capability_id"], + "steps": { + "steps": [ + {"kind": "setpoint", "address": "2bma:unconnected", "value": 1.0}, + {"kind": "capture", "address": "2bma:readback", "capture_name": "a_readback"}, + ], + }, + "closing_steps": { + "steps": [ + { + "kind": "setpoint", + "address": "2bma:shutter", + "value": {"__capture__": "a_readback"}, + }, + ], + }, + }, + ).json() + registered = client.post( + "/procedures/from-recipe", + json={ + "name": "closing capture procedure", + "kind": "bakeout", + "target_asset_ids": [], + "parent_run_id": None, + "recipe_id": recipe["recipe_id"], + "bindings": {}, + }, + ) + assert registered.status_code == 201, registered.text + return UUID(registered.json()["procedure_id"]) + + +@pytest.mark.contract +def test_post_conduct_from_returns_422_for_closing_capture_before_boundary() -> None: + """A closing CaptureRef naming a capture only a pre-boundary main step + declares is rejected up front: resume starts captures empty, so a + boundary that skips the declaring step would never populate it.""" + with TestClient(create_app()) as client: + pid = _register_from_recipe_with_closing_capture(client) + held = client.post(f"/procedures/{pid}/conduct-or-hold", json={"steps": []}) + assert held.status_code == 200, held.text + assert held.json()["held"] is True + # boundary == 2 (both main steps done): skips the CaptureStep entirely. + response = client.post( + f"/procedures/{pid}/conduct-from", json={"re_establishment_boundary": 2} + ) + assert response.status_code == 422, response.text + + @pytest.mark.contract def test_post_conduct_from_aborts_on_a_genuine_step_failure() -> None: """Replaying a tail whose setpoint still fails (unconnected address) aborts: diff --git a/apps/api/tests/contract/test_conduct_until_advised_endpoint.py b/apps/api/tests/contract/test_conduct_until_advised_endpoint.py index b31b3902e25..52a21ff7a34 100644 --- a/apps/api/tests/contract/test_conduct_until_advised_endpoint.py +++ b/apps/api/tests/contract/test_conduct_until_advised_endpoint.py @@ -191,6 +191,76 @@ def _register_from_steered_recipe(client: TestClient) -> UUID: return UUID(registered.json()["procedure_id"]) +def _register_from_steered_recipe_with_closing_steps(client: TestClient) -> UUID: + """Same as `_register_from_steered_recipe`, plus a non-empty closing_steps + on the Recipe -- the shape conduct_until_advised must refuse (v1 scope: + a loop that re-walks one pass block has no defined place to run + _run_closing).""" + cap = client.post( + "/capabilities", + json={ + "code": "cora.capability.steered_align_recipe_closing", + "name": "SteeredAlignClosingCap", + "required_affordances": [], + "executor_shapes": ["Method", "Procedure"], + }, + ).json() + recipe = client.post( + "/recipes", + json={ + "name": "steered align recipe with closing steps", + "capability_id": cap["capability_id"], + "steps": { + "steps": [ + { + "kind": "compute", + "command": ["tomopy", "find_center"], + "input_uris": ["file:///data/19bm/align/theta_pair.h5"], + "output_uri": None, + "parameters": {}, + "capture_name": _OBJECTIVE_NAME, + }, + { + "kind": "setpoint", + "address": "19bm:sample_rotary_theta", + "value": {"__steering__": _AXIS}, + "verify": False, + }, + ], + }, + "closing_steps": { + "steps": [ + {"kind": "setpoint", "address": "19bm:shutter", "value": 0.0}, + ], + }, + }, + ).json() + registered = client.post( + "/procedures/from-recipe", + json={ + "name": "steered align from recipe with closing", + "kind": "rotation_center_characterization", + "target_asset_ids": [], + "parent_run_id": None, + "recipe_id": recipe["recipe_id"], + "bindings": {}, + }, + ) + assert registered.status_code == 201, registered.text + return UUID(registered.json()["procedure_id"]) + + +@pytest.mark.contract +def test_post_conduct_until_advised_refuses_a_closing_bearing_recipe() -> None: + """v1 scope: a loop-driving slice has no defined place to run + _run_closing, so it refuses a closing-bearing Recipe outright (422), + before any FSM event fires.""" + with TestClient(create_app()) as client: + pid = _register_from_steered_recipe_with_closing_steps(client) + run = client.post(_PATH.format(pid=pid), json=_body()) + assert run.status_code == 422, run.text + + @pytest.mark.contract def test_post_conduct_until_advised_steered_recipe_executes_over_the_wire() -> None: """A SteeringRef-authored recipe drives conduct_until_advised over the wire. diff --git a/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py b/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py index 5ff999a6193..ca4d0292fc5 100644 --- a/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_from_procedure_handler.py @@ -38,8 +38,15 @@ load_procedure, to_payload, ) -from cora.operation.conductor import ActionStep, Conductor, SetpointStep, Step, step_to_payload -from cora.operation.errors import UnauthorizedError +from cora.operation.conductor import ( + ActionStep, + CaptureStep, + Conductor, + SetpointStep, + Step, + step_to_payload, +) +from cora.operation.errors import ClosingCaptureBeforeBoundaryError, UnauthorizedError from cora.operation.features import ( abort_procedure, append_activities, @@ -55,6 +62,8 @@ Handler as ConductFromHandler, ) from cora.operation.ports.control_port import ActuationKind, ControlPort +from cora.operation.ports.measurement import Measurement +from cora.recipe.aggregates.recipe.body import CaptureRef from cora.run.aggregates.run import RunHeld, RunStarted from cora.run.aggregates.run import event_type_name as run_event_type_name from cora.run.aggregates.run import to_payload as run_to_payload @@ -100,6 +109,7 @@ async def _seed_held_with_steps( store: InMemoryEventStore, *, steps: Sequence[Step], + closing_steps: Sequence[Step] = (), procedure_id: UUID = _PROCEDURE_ID, parent_run_id: UUID | None = None, held_actuation_kind: str | None = None, @@ -108,6 +118,7 @@ async def _seed_held_with_steps( (the pinned resolved steps) + Started + Held. `held_actuation_kind` is the kind the pre-hold conduct observed (carried on ProcedureHeld).""" resolved = tuple(step_to_payload(s) for s in steps) + resolved_closing = tuple(step_to_payload(s) for s in closing_steps) events = [ ProcedureRegistered( procedure_id=procedure_id, @@ -120,7 +131,8 @@ async def _seed_held_with_steps( ResolvedStepsRecorded( procedure_id=procedure_id, resolved_steps=resolved, - step_count=len(resolved), + resolved_closing_steps=resolved_closing, + step_count=len(resolved) + len(resolved_closing), occurred_at=_PRIOR, ), ProcedureStarted(procedure_id=procedure_id, occurred_at=_PRIOR), @@ -182,6 +194,10 @@ async def _status(store: InMemoryEventStore) -> ProcedureStatus: return state.status +def _good_reading(value: float) -> Measurement: + return Measurement(value=value, kind="Scalar", quality="Good", produced_at=_NOW) + + async def _call(handler: ConductFromHandler, boundary: int) -> ConductFromProcedureResult: return await handler( ConductFromProcedure(procedure_id=_PROCEDURE_ID, re_establishment_boundary=boundary), @@ -444,3 +460,65 @@ async def test_conduct_from_folds_pre_hold_actuation_kind_into_completion() -> N assert state is not None assert state.status is ProcedureStatus.COMPLETED assert state.actuation_kind == ActuationKind.HYBRID.value + + +@pytest.mark.unit +async def test_clean_tail_resume_also_runs_pinned_closing_steps() -> None: + """resolved_closing_steps is read off the pinned record and handed to + Conductor.conduct_from, not just parsed and discarded.""" + store = InMemoryEventStore() + port = InMemoryControlPort() + port.simulate_connect("2bma:a") + port.simulate_connect("2bma:shutter") + await _seed_held_with_steps( + store, + steps=(SetpointStep(address="2bma:a", value=1.0),), + closing_steps=(SetpointStep(address="2bma:shutter", value=0.0),), + ) + deps = _deps(store) + result = await _call(_make_conduct_from(deps, port), 0) + + assert result.succeeded is True + assert (await port.read("2bma:shutter")).value == 0.0 + assert result.substrate_writes == {"2bma:a": 1.0, "2bma:shutter": 0.0} + + +@pytest.mark.unit +async def test_raises_closing_capture_before_boundary_when_only_prefix_declares_it() -> None: + """A closing CaptureRef naming a capture only a PRE-boundary main step + declares would resolve against nothing (captures start empty on resume) -- + rejected up front rather than surfacing as a closing_failures entry.""" + store = InMemoryEventStore() + await _seed_held_with_steps( + store, + steps=( + CaptureStep(address="2bma:a", capture_name="a_readback"), + SetpointStep(address="2bma:b", value=2.0), + ), + closing_steps=(SetpointStep(address="2bma:shutter", value=CaptureRef("a_readback")),), + ) + deps = _deps(store) + with pytest.raises(ClosingCaptureBeforeBoundaryError) as exc: + await _call(_make_conduct_from(deps, InMemoryControlPort()), 1) # skips the CaptureStep + assert exc.value.capture_name == "a_readback" + assert exc.value.boundary == 1 + + +@pytest.mark.unit +async def test_closing_capture_declared_at_boundary_is_accepted() -> None: + """The same capture_name, declared AT the boundary (re-run this resume), + is fine: it will be populated before the closing step reads it.""" + store = InMemoryEventStore() + port = InMemoryControlPort() + port.set_reading("2bma:a", _good_reading(1.0)) + port.simulate_connect("2bma:shutter") + await _seed_held_with_steps( + store, + steps=(CaptureStep(address="2bma:a", capture_name="a_readback"),), + closing_steps=(SetpointStep(address="2bma:shutter", value=CaptureRef("a_readback")),), + ) + deps = _deps(store) + result = await _call(_make_conduct_from(deps, port), 0) + + assert result.succeeded is True + assert (await port.read("2bma:shutter")).value == 1.0 diff --git a/apps/api/tests/unit/operation/test_conduct_or_hold_procedure_handler.py b/apps/api/tests/unit/operation/test_conduct_or_hold_procedure_handler.py index a3971dcc27e..ae9a7033192 100644 --- a/apps/api/tests/unit/operation/test_conduct_or_hold_procedure_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_or_hold_procedure_handler.py @@ -13,6 +13,7 @@ - unknown procedure -> ProcedureNotFoundError """ +import hashlib from collections.abc import Sequence from dataclasses import dataclass from datetime import UTC, datetime @@ -24,6 +25,7 @@ from cora.infrastructure.event_envelope import to_new_event from cora.infrastructure.kernel import Kernel from cora.infrastructure.routing import NIL_SENTINEL_ID +from cora.operation._recipe_expansion import steps_to_wire_with_closing from cora.operation.adapters.in_memory_control_port import InMemoryControlPort from cora.operation.adapters.in_memory_recipe_expander import InMemoryRecipeExpander from cora.operation.aggregates.procedure import ( @@ -32,6 +34,7 @@ ProcedureRegistered, ProcedureStarted, ProcedureStatus, + RecipeExpansionRecorded, event_type_name, load_procedure, to_payload, @@ -62,6 +65,10 @@ Handler as ConductOrHoldHandler, ) from cora.operation.features.hold_procedure.command import HoldProcedure +from cora.recipe.aggregates.recipe import RecipeDefined, RecipeSetpointStep +from cora.recipe.aggregates.recipe import event_type_name as recipe_event_type_name +from cora.recipe.aggregates.recipe import to_payload as recipe_to_payload +from cora.shared.canonical_json import canonical_json_bytes from tests.unit._helpers import build_deps as _build_deps_shared _NOW = datetime(2026, 6, 21, 12, 0, 0, tzinfo=UTC) @@ -70,6 +77,110 @@ _CORRELATION_ID = UUID("01900000-0000-7000-8000-0000000000aa") +# --- recipe-driven closing_steps wiring --------------------------------- +# +# A single test proving `resolve_and_pin_conduct_steps`'s returned +# `closing_steps` actually reaches `Conductor.conduct_or_hold`, not just +# `ResolvedStepsRecorded` (a real Conductor + real port catches a dropped +# kwarg a fake conductor's call-recording would also catch, but this pins +# the ACTUAL closing walk executing, matching this file's real-Conductor +# style for everything else). + + +async def _seed_recipe_driven_defined( + store: InMemoryEventStore, + *, + recipe_steps: tuple[RecipeSetpointStep, ...], + recipe_closing_steps: tuple[RecipeSetpointStep, ...], +) -> None: + """Seed a recipe-driven, Defined Procedure: a Recipe stream + the + ProcedureRegistered + RecipeExpansionRecorded genesis block + `register_procedure_from_recipe` emits. No Capability stream needed: + `load_capability` returning None (unseeded) is treated as "not + deprecated", the same as a real, non-deprecated Capability.""" + recipe_id = uuid4() + capability_id = uuid4() + bindings: dict[str, object] = {} + recipe_event = RecipeDefined( + recipe_id=recipe_id, + name="R", + capability_id=capability_id, + steps=recipe_steps, + closing_steps=recipe_closing_steps, + occurred_at=_NOW, + ) + await store.append( + stream_type="Recipe", + stream_id=recipe_id, + expected_version=0, + events=[ + to_new_event( + event_type=recipe_event_type_name(recipe_event), + payload=recipe_to_payload(recipe_event), + occurred_at=_NOW, + event_id=uuid4(), + command_name="seed", + correlation_id=_CORRELATION_ID, + principal_id=_PRINCIPAL_ID, + ), + ], + ) + expanded: tuple[Step, ...] = tuple( + SetpointStep(address=s.address, value=s.value) # type: ignore[arg-type] + for s in recipe_steps + ) + expanded_closing: tuple[Step, ...] = tuple( + SetpointStep(address=s.address, value=s.value) # type: ignore[arg-type] + for s in recipe_closing_steps + ) + steps_hash = hashlib.sha256( + canonical_json_bytes(steps_to_wire_with_closing(expanded, expanded_closing)) + ).hexdigest() + bindings_hash = hashlib.sha256(canonical_json_bytes(bindings)).hexdigest() + events = [ + ProcedureRegistered( + procedure_id=_PROCEDURE_ID, + name="P", + kind="bakeout", + target_asset_ids=(), + parent_run_id=None, + capability_id=capability_id, + recipe_id=recipe_id, + occurred_at=_NOW, + ), + RecipeExpansionRecorded( + procedure_id=_PROCEDURE_ID, + recipe_id=recipe_id, + recipe_version=None, + capability_id=capability_id, + capability_version=None, + bindings=bindings, + expansion_port_version="v2-pseudoaxis-aware", + steps_hash=steps_hash, + bindings_hash=bindings_hash, + step_count=len(recipe_steps) + len(recipe_closing_steps), + occurred_at=_NOW, + ), + ] + await store.append( + stream_type="Procedure", + stream_id=_PROCEDURE_ID, + expected_version=0, + events=[ + to_new_event( + event_type=event_type_name(event), # type: ignore[arg-type] + payload=to_payload(event), # type: ignore[arg-type] + occurred_at=_NOW, + event_id=uuid4(), + command_name="seed", + correlation_id=_CORRELATION_ID, + principal_id=_PRINCIPAL_ID, + ) + for event in events + ], + ) + + @dataclass class _LenientIds: """Conductor id_generator that never exhausts (markers double appends).""" @@ -395,3 +506,48 @@ async def test_complete_rejected_records_lifecycle_failure() -> None: assert result.failure is not None assert result.failure.source_kind == "lifecycle" assert result.failure.target == "complete" + + +@pytest.mark.unit +async def test_recipe_driven_clean_run_also_runs_pinned_closing_steps() -> None: + """resolve_and_pin_conduct_steps's returned closing_steps must reach + Conductor.conduct_or_hold, not just ResolvedStepsRecorded -- a dropped + kwarg here would leave every recipe's closing steps silently unwalked.""" + store = InMemoryEventStore() + port = InMemoryControlPort() + port.simulate_connect("2bma:a") + port.simulate_connect("2bma:shutter") + await _seed_recipe_driven_defined( + store, + recipe_steps=(RecipeSetpointStep(address="2bma:a", value=1.0),), + recipe_closing_steps=(RecipeSetpointStep(address="2bma:shutter", value=0.0),), + ) + result = await _call(_make_conduct_or_hold(_deps(store), port), ()) + + assert result.succeeded is True + assert result.held is False + assert await _status(store) is ProcedureStatus.COMPLETED + assert (await port.read("2bma:shutter")).value == 0.0 + assert result.substrate_writes == {"2bma:a": 1.0, "2bma:shutter": 0.0} + assert result.closing_failures == () + + +@pytest.mark.unit +async def test_recipe_driven_clean_run_isolates_a_failing_closing_step() -> None: + """closing_failures threads onto ConductOrHoldProcedureResult -- a + failing closing step (unconnected shutter address) is isolated: it + neither flips `succeeded` nor blocks the main walk's own success.""" + store = InMemoryEventStore() + port = InMemoryControlPort() + port.simulate_connect("2bma:a") # shutter left unconnected: the closing write fails + await _seed_recipe_driven_defined( + store, + recipe_steps=(RecipeSetpointStep(address="2bma:a", value=1.0),), + recipe_closing_steps=(RecipeSetpointStep(address="2bma:shutter", value=0.0),), + ) + result = await _call(_make_conduct_or_hold(_deps(store), port), ()) + + assert result.succeeded is True + assert await _status(store) is ProcedureStatus.COMPLETED + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].error_class == "ControlNotConnectedError" diff --git a/apps/api/tests/unit/operation/test_conduct_procedure_handler.py b/apps/api/tests/unit/operation/test_conduct_procedure_handler.py index f0f10433080..a2ea988b8b9 100644 --- a/apps/api/tests/unit/operation/test_conduct_procedure_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_procedure_handler.py @@ -126,6 +126,7 @@ class _ConductCall: causation_id: UUID | None surface_id: UUID steps: Sequence[Step] + closing_steps: Sequence[Step] = () @dataclass @@ -142,6 +143,7 @@ async def conduct( principal_id: UUID, correlation_id: UUID, steps: Sequence[Step], + closing_steps: Sequence[Step] = (), causation_id: UUID | None = None, surface_id: UUID = NIL_SENTINEL_ID, ) -> ConductorResult: @@ -153,6 +155,7 @@ async def conduct( causation_id=causation_id, surface_id=surface_id, steps=steps, + closing_steps=closing_steps, ) ) return self.result @@ -891,6 +894,9 @@ async def test_conduct_procedure_recipe_with_closing_steps_pins_them_re_expanded assert len(recorded) == 1 expected_closing = SetpointStep(address="dev:shutter", value=0.0) assert recorded[0].resolved_closing_steps == (step_to_payload(expected_closing),) + # Pinned is not enough: the Conductor must actually receive them, or + # _run_closing never walks them. + assert conductor.calls[0].closing_steps == (expected_closing,) assert recorded[0].step_count == 2 # 1 main + 1 closing diff --git a/apps/api/tests/unit/operation/test_conduct_until_advised_from_handler.py b/apps/api/tests/unit/operation/test_conduct_until_advised_from_handler.py index 88491b2e294..015a6e0125d 100644 --- a/apps/api/tests/unit/operation/test_conduct_until_advised_from_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_until_advised_from_handler.py @@ -67,7 +67,7 @@ Step, step_to_payload, ) -from cora.operation.errors import UnauthorizedError +from cora.operation.errors import UnauthorizedError, UnsupportedClosingStepsError from cora.operation.features import ( abort_procedure, append_activities, @@ -217,6 +217,7 @@ async def _seed_held_steered( open_pass: bool = False, extra_outcome: tuple[int, float, float] | None = None, procedure_id: UUID = _PROCEDURE_ID, + resolved_closing_steps: tuple[Step, ...] = (), ) -> None: """Land a conducted-then-Held steered Procedure with recorded closed passes. @@ -233,6 +234,7 @@ async def _seed_held_steered( open pass. It is recovered + re-fed by the resume. """ resolved = tuple(step_to_payload(s) for s in _steered_block()) + resolved_closing = tuple(step_to_payload(s) for s in resolved_closing_steps) events: list[ProcedureEvent] = [ ProcedureRegistered( procedure_id=procedure_id, @@ -245,7 +247,8 @@ async def _seed_held_steered( ResolvedStepsRecorded( procedure_id=procedure_id, resolved_steps=resolved, - step_count=len(resolved), + resolved_closing_steps=resolved_closing, + step_count=len(resolved) + len(resolved_closing), occurred_at=_PRIOR, ), ProcedureStarted(procedure_id=procedure_id, occurred_at=_PRIOR), @@ -624,6 +627,28 @@ async def test_resume_authz_deny_raises_unauthorized() -> None: with pytest.raises(UnauthorizedError): await _call(_make_conduct_from(deps, port, compute, outcome_store)) + + +@pytest.mark.unit +async def test_resume_refuses_a_closing_bearing_pinned_record() -> None: + """v1 scope, mirroring conduct_until_advised's forward-direction refusal: + the pinned record already carries resolved_closing_steps when the bound + Recipe has any, so no fresh Recipe load is needed to reject here.""" + store = InMemoryEventStore() + port = InMemoryControlPort() + port.simulate_connect(_MOTOR_ADDR) + compute = InMemoryComputePort() + outcome_store = InMemoryOutcomeStore() + await _seed_held_steered( + store, + outcome_store, + closed=[(3.0, 2.0)], + resolved_closing_steps=(SetpointStep(address="2bma:shutter", value=0.0),), + ) + deps = _deps(store) + + with pytest.raises(UnsupportedClosingStepsError): + await _call(_make_conduct_from(deps, port, compute, outcome_store)) assert await _status(store) is ProcedureStatus.HELD diff --git a/apps/api/tests/unit/operation/test_conduct_until_converged_handler.py b/apps/api/tests/unit/operation/test_conduct_until_converged_handler.py index fd5480cb000..02b994c3b2a 100644 --- a/apps/api/tests/unit/operation/test_conduct_until_converged_handler.py +++ b/apps/api/tests/unit/operation/test_conduct_until_converged_handler.py @@ -13,6 +13,7 @@ - result_to_wire serializes success + failure """ +import hashlib from collections.abc import Sequence from dataclasses import dataclass, field from datetime import UTC, datetime @@ -26,9 +27,11 @@ from cora.infrastructure.ports.clock import FakeClock from cora.infrastructure.ports.id_generator import UUIDv7Generator from cora.infrastructure.routing import NIL_SENTINEL_ID +from cora.operation._recipe_expansion import steps_to_wire_with_closing from cora.operation.adapters.in_memory_recipe_expander import InMemoryRecipeExpander from cora.operation.aggregates.procedure import ( ProcedureRegistered, + RecipeExpansionRecorded, event_type_name, to_payload, ) @@ -40,18 +43,115 @@ Step, WithinToleranceCriterion, ) -from cora.operation.errors import UnauthorizedError +from cora.operation.errors import UnauthorizedError, UnsupportedClosingStepsError from cora.operation.features.conduct_until_converged.command import ( ConductUntilConverged, ConductUntilConvergedResult, ) from cora.operation.features.conduct_until_converged.handler import bind from cora.operation.features.conduct_until_converged.route import result_to_wire +from cora.recipe.aggregates.recipe import RecipeDefined, RecipeSetpointStep +from cora.recipe.aggregates.recipe import event_type_name as recipe_event_type_name +from cora.recipe.aggregates.recipe import to_payload as recipe_to_payload +from cora.shared.canonical_json import canonical_json_bytes _NOW = datetime(2026, 6, 24, 12, 0, 0, tzinfo=UTC) _CRITERION = WithinToleranceCriterion(expected=0.0, tolerance=0.5) +async def _seed_recipe_driven_procedure_with_closing_steps( + store: InMemoryEventStore, procedure_id: UUID +) -> None: + """Seed a recipe-driven, Defined Procedure whose Recipe carries a + non-empty closing_steps -- the shape conduct_until_converged must + refuse (v1 scope). No Capability stream needed: `load_capability` + returning None is treated as "not deprecated".""" + recipe_id = uuid4() + capability_id = uuid4() + recipe_steps = (RecipeSetpointStep(address="dev:x", value=1.0),) + recipe_closing_steps = (RecipeSetpointStep(address="dev:shutter", value=0.0),) + recipe_event = RecipeDefined( + recipe_id=recipe_id, + name="R", + capability_id=capability_id, + steps=recipe_steps, + closing_steps=recipe_closing_steps, + occurred_at=_NOW, + ) + await store.append( + stream_type="Recipe", + stream_id=recipe_id, + expected_version=0, + events=[ + to_new_event( + event_type=recipe_event_type_name(recipe_event), + payload=recipe_to_payload(recipe_event), + occurred_at=_NOW, + event_id=uuid4(), + command_name="seed", + correlation_id=uuid4(), + causation_id=None, + principal_id=uuid4(), + ), + ], + ) + expanded: tuple[Step, ...] = tuple( + SetpointStep(address=s.address, value=s.value) # type: ignore[arg-type] + for s in recipe_steps + ) + expanded_closing: tuple[Step, ...] = tuple( + SetpointStep(address=s.address, value=s.value) # type: ignore[arg-type] + for s in recipe_closing_steps + ) + steps_hash = hashlib.sha256( + canonical_json_bytes(steps_to_wire_with_closing(expanded, expanded_closing)) + ).hexdigest() + bindings_hash = hashlib.sha256(canonical_json_bytes({})).hexdigest() + events = [ + ProcedureRegistered( + procedure_id=procedure_id, + name="P", + kind="bakeout", + target_asset_ids=(), + parent_run_id=None, + capability_id=capability_id, + recipe_id=recipe_id, + occurred_at=_NOW, + ), + RecipeExpansionRecorded( + procedure_id=procedure_id, + recipe_id=recipe_id, + recipe_version=None, + capability_id=capability_id, + capability_version=None, + bindings={}, + expansion_port_version="v2-pseudoaxis-aware", + steps_hash=steps_hash, + bindings_hash=bindings_hash, + step_count=len(recipe_steps) + len(recipe_closing_steps), + occurred_at=_NOW, + ), + ] + await store.append( + stream_type="Procedure", + stream_id=procedure_id, + expected_version=0, + events=[ + to_new_event( + event_type=event_type_name(event), # type: ignore[arg-type] + payload=to_payload(event), # type: ignore[arg-type] + occurred_at=_NOW, + event_id=uuid4(), + command_name="seed", + correlation_id=uuid4(), + causation_id=None, + principal_id=uuid4(), + ) + for event in events + ], + ) + + async def _seed_procedure( store: InMemoryEventStore, procedure_id: UUID, @@ -281,3 +381,31 @@ def test_result_to_wire_serializes_cap_abort_failure() -> None: assert wire.failure is not None assert wire.failure.error_class == "ConvergenceIterationCapReached" assert wire.failure.step_index is None + + +@pytest.mark.unit +async def test_handler_refuses_a_closing_bearing_recipe() -> None: + """v1 scope: a loop-driving slice has no defined place to run + _run_closing, so it refuses rather than silently dropping the Recipe's + closing steps. The Conductor is never invoked.""" + procedure_id = uuid4() + store = InMemoryEventStore() + await _seed_recipe_driven_procedure_with_closing_steps(store, procedure_id) + conductor = _FakeConductor(result=ConductorResult(procedure_id=procedure_id, completed_count=0)) + handler = bind( + _deps(_FakeAuthz(), store), # type: ignore[arg-type] + conductor=conductor, # type: ignore[arg-type] + expansion_port=InMemoryRecipeExpander(), + ) + with pytest.raises(UnsupportedClosingStepsError) as exc: + await handler( + ConductUntilConverged( + procedure_id=procedure_id, + convergence_capture_name="offset", + criterion=_CRITERION, + ), + principal_id=uuid4(), + correlation_id=uuid4(), + ) + assert exc.value.procedure_id == procedure_id + assert conductor.calls == [] From 51e8853ae07f4e151969501b55e2ff10ec981185 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sun, 30 Aug 2026 06:28:39 -0500 Subject: [PATCH 6/7] Move flat_field's shutter-close into closing_steps, document the concept flat_field's steps 4-5 (close the shutter, verify closed) were just the tail of the main list: a halt at step 3 left the shutter open with nothing having run to close it. They now live in the recipe's closing_steps, which the Conductor walks on any real terminal (Completed or Aborted), not only a clean run. dark_field needs no change: its shutter-close is already step 1 of its main list, so it already ends in a safe state. Adds a glossary entry for closing steps alongside the rest of the Recipe ladder vocabulary. --- docs/deployments/2-bm/recipes.md | 9 +++++++-- docs/reference/glossary.md | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/deployments/2-bm/recipes.md b/docs/deployments/2-bm/recipes.md index dde33e483ed..32b8d94124d 100644 --- a/docs/deployments/2-bm/recipes.md +++ b/docs/deployments/2-bm/recipes.md @@ -54,8 +54,13 @@ Both momentary-command questions are now answered by the same run: writing `1` t | 1 | setpoint | `S02BM-PSS:SBS:OpenEPICSC` (StationShutter, open command) | `1` (verify) | | 2 | check | `S02BM-PSS:SBS:BeamBlockingM` (StationShutter, status read-back) | `== "OFF"` (open, inverted polarity) | | 3 | action | `collect` | `{ detector: "2bmSP1:", repetitions: <>, dwell: <> }` | -| 4 | setpoint | `S02BM-PSS:SBS:CloseEPICSC` (StationShutter, close command) | `1` (verify, return to safe state) | -| 5 | check | `S02BM-PSS:SBS:BeamBlockingM` (StationShutter, status read-back) | `== "ON"` (closed) | + +**Closing steps** (see [glossary](../../reference/glossary.md#recipe-ladder)): walked once `steps` above reaches a real terminal (Completed or Aborted), so the shutter closes even if the capture halts partway through instead of only on a clean run. + +| # | Step | Address | Value / params | +| --- | --- | --- | --- | +| 1 | setpoint | `S02BM-PSS:SBS:CloseEPICSC` (StationShutter, close command) | `1` (verify, return to safe state) | +| 2 | check | `S02BM-PSS:SBS:BeamBlockingM` (StationShutter, status read-back) | `== "ON"` (closed) | **Precondition:** the sample is out of the beam path. This is an operator assertion, not a CORA setpoint (CORA does not drive the sample out automatically). diff --git a/docs/reference/glossary.md b/docs/reference/glossary.md index a0686e215d1..99d62f80572 100644 --- a/docs/reference/glossary.md +++ b/docs/reference/glossary.md @@ -98,6 +98,7 @@ Watch-only (not adopted as a glossary term, see [Deferred](../stack/deferred.md) - **Run.** An execution of a Plan. FSM `RunStatus`: `Running`, `Held`, `Completed`, `Aborted`, `Stopped`, `Truncated`. - **Recipe.** *(Recipe BC)* The executable body: a deployment-bound, ordered tuple of templated steps referencing one Capability by `capability_id`, which expands into the flat Conductor step list once an operator supplies parameter bindings. Sits BESIDE the ladder rather than on a rung of it: Method stays the technique-class contract and Plan stays the Asset-bound binding, while Recipe carries the steps and iterates faster than the Capability above it. Steps change only through `version_recipe` (they replace wholesale, and the prior version stays readable), so a Recipe body cannot be silently mutated. - **RecipeStep.** *(Recipe BC, closed union)* Five templated arms, each the twin of one Conductor `Step`: `RecipeSetpointStep`, `RecipeActionStep`, `RecipeCheckStep`, `RecipeCaptureStep`, `RecipeComputeStep`. A step position that holds a value the Recipe does not itself carry uses one of four typed sentinels, which differ in when they resolve and who produces the value: `BindingRef` (expansion time, from the operator's frozen bindings), `CaptureRef` (execute time, from a reading an earlier step deposited), `SteeringRef` (execute time, from the coordinate the decide loop seeds each pass), `OutputRef` (execute time, from an artifact an earlier compute step produced). Substitution is typed, never textual `${var}`. The union carries no branch, loop, or fork arm: control flow lives outside the body, in the conduct verb (`conduct_until_converged`, `conduct_until_advised`) or in a `Held` transition an operator resumes. See the [Recipe module](../architecture/modules/recipe/index.md#step-kinds-and-value-substitution). +- **Closing steps.** *(Recipe BC / Conductor)* A Recipe's second, optional step list (`closing_steps`, same five `RecipeStep` arms as `steps`). The Conductor walks it once `steps` reaches a real terminal, Completed or Aborted, never on a `Held` pause, an acquisition halt, or cancellation. Each closing step is isolated: one failing does not stop the rest of the walk or flip the conduct's own `succeeded`, and every failure lands in `closing_failures` on the result. Exists so a Recipe can return equipment to a safe state (close a shutter, verify closed) even when the main step list halted early. `dark_field` already ends safe (its shutter-close is step 1 of `steps`); `flat_field` moves its own shutter-close + verify into `closing_steps` so a halt mid-capture still closes the shutter. - **Logbook.** Append-only narrative log on a Run or Decision. Used for OTel `gen_ai.*` reasoning entries on Decisions. ## Agents From f6b6deff9eddef9d547136bb2eefee26e5c794f5 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Sun, 30 Aug 2026 09:51:06 -0500 Subject: [PATCH 7/7] Cover the raised-exception and step-kind branches diff-cover flagged Six new unit tests, no source changes: conduct_or_hold's and conduct_from's raised-exception paths (mirroring conduct()'s own, which was already tested) had no test at all, and _closing_step_kind / _closing_step_target's non-ActionStep branches were only ever exercised by the ActionStep arm, leaving Setpoint/Capture/Compute/ Check dead in coverage. PR #748's diff-cover gate (90% hard floor) caught the gap. --- .../tests/unit/operation/test_conductor.py | 232 ++++++++++++++++++ 1 file changed, 232 insertions(+) diff --git a/apps/api/tests/unit/operation/test_conductor.py b/apps/api/tests/unit/operation/test_conductor.py index cd71f21a2c4..082f18502e1 100644 --- a/apps/api/tests/unit/operation/test_conductor.py +++ b/apps/api/tests/unit/operation/test_conductor.py @@ -80,7 +80,9 @@ from cora.operation.conductor import ( ActionContext, ActionStep, + CaptureStep, CheckStep, + ComputeStep, Conductor, ConductorFailure, ConductorResult, @@ -2298,6 +2300,137 @@ async def buggy(_ctx: ActionContext) -> Mapping[str, Any]: assert result.closing_failures[0].target == "buggy" +class _RaisingPort: + """Every `read`/`write` raises a non-`_CONTROL_ERRORS` exception, so + `_run_setpoint`/`_run_check`/`_run_capture`'s own `except _CONTROL_ERRORS` + does not catch it -- it propagates to `_run_closing`'s classification + fallback (`_closing_step_kind`/`_closing_step_target`).""" + + async def read(self, address: str) -> Measurement: + raise RuntimeError("port read bug") + + async def write(self, *_args: Any, **_kwargs: Any) -> None: + raise RuntimeError("port write bug") + + def subscribe(self, address: str) -> AsyncIterator[Measurement]: # pragma: no cover # unused + raise NotImplementedError + + +@pytest.mark.unit +async def test_conduct_closing_setpoint_that_raises_classifies_as_setpoint() -> None: + """`_closing_step_kind`'s SetpointStep branch: a closing SetpointStep + whose write raises a non-port exception is still recorded, tagged + `source_kind="setpoint"`, `target=
`.""" + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + _RaisingPort(), + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(6)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(), + closing_steps=(SetpointStep(address="2bma:closing", value=1.0),), + ) + + assert result.succeeded is True + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].source_kind == "setpoint" + assert result.closing_failures[0].target == "2bma:closing" + assert result.closing_failures[0].error_class == "RuntimeError" + + +@pytest.mark.unit +async def test_conduct_closing_capture_that_raises_classifies_as_capture() -> None: + """`_closing_step_kind`'s CaptureStep branch.""" + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + _RaisingPort(), + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(6)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(), + closing_steps=(CaptureStep(address="2bma:closing", capture_name="x"),), + ) + + assert result.succeeded is True + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].source_kind == "capture" + assert result.closing_failures[0].target == "2bma:closing" + + +@pytest.mark.unit +async def test_conduct_closing_check_that_raises_classifies_as_check() -> None: + """`_closing_step_kind`'s default arm (CheckStep, the only Step kind not + named by an earlier `isinstance` branch).""" + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + _RaisingPort(), + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(6)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(), + closing_steps=(CheckStep(address="2bma:closing", criterion=EqualsCriterion(expected=1)),), + ) + + assert result.succeeded is True + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].source_kind == "check" + assert result.closing_failures[0].target == "2bma:closing" + + +@pytest.mark.unit +async def test_conduct_closing_compute_that_raises_classifies_as_compute() -> None: + """`_closing_step_kind`/`_closing_step_target`'s ComputeStep branches. + No `compute_port` wired, so `_run_compute` raises immediately -- the + loudest, simplest way to exercise this arm without a real substrate.""" + port = InMemoryControlPort() + appender = _FakeAppendStep() + conductor = _conductor_full_lifecycle( + port, + appender, + start=_FakeLifecycleHandler(), + complete=_FakeLifecycleHandler(), + abort=_FakeLifecycleHandler(), + ids=[uuid4() for _ in range(6)], + ) + + result = await conductor.conduct( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(), + closing_steps=(ComputeStep(command=("tomopy", "recon"), input_uris=(), output_uri=None),), + ) + + assert result.succeeded is True + assert len(result.closing_failures) == 1 + assert result.closing_failures[0].source_kind == "compute" + assert result.closing_failures[0].target == "tomopy recon" + + @pytest.mark.unit async def test_conduct_cancellation_does_not_run_closing_steps() -> None: """A cancellation re-raises before any closing walk is attempted.""" @@ -2410,6 +2543,46 @@ async def test_conduct_complete_rejected_still_reports_closing_that_already_ran( assert dict(result.substrate_writes) == {"2bma:shutter": 1, "2bma:closing": 0.0} +@pytest.mark.unit +async def test_conduct_or_hold_raised_exception_still_runs_closing_before_reraising() -> None: + """Mirrors conduct()'s twin: an unhandled exception from an action body + must not skip closing. Best-effort abort fires after closing (closing's + journal writes need the Procedure still Running), then the ORIGINAL + exception propagates unchanged.""" + + async def buggy(_ctx: ActionContext) -> Mapping[str, Any]: + raise RuntimeError("oops") + + registry = InMemoryActionRegistry({"buggy": buggy}) + port, inner = _routed_port("2bma:closing") + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + conductor = Conductor( + control_port=port, + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + action_registry=registry, + start_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=abort, + hold_procedure=_FakeLifecycleHandler(), + ) + + with pytest.raises(RuntimeError, match="oops"): + await conductor.conduct_or_hold( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(ActionStep(name="buggy"),), + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + assert len(abort.calls) == 1 + assert "unhandled exception" in abort.calls[0].command.reason + landed = await inner.read("2bma:closing") + assert landed.value == 0.0 + + @pytest.mark.unit async def test_conduct_or_hold_held_procedure_does_not_run_closing_steps() -> None: """A hold is a pause: a later conduct_from resumes against the state the @@ -2641,6 +2814,65 @@ async def test_conduct_from_complete_rejected_still_reports_closing_that_already assert dict(result.substrate_writes) == {"2bma:shutter": 1, "2bma:closing": 0.0} +@pytest.mark.unit +async def test_conduct_from_raised_exception_still_runs_closing_before_reraising() -> None: + """Mirrors conduct()'s twin for the resume path: an unhandled exception + from a replayed step still runs closing (for its recording side-effect), + then the ORIGINAL exception propagates. Unlike conduct()/conduct_or_hold, + NO best-effort abort precedes it: a mid-replay raised exception leaves + the Procedure Running, the same posture as an acquisition halt, for the + operator to reconcile. An ActionStep can't trigger this here -- + execute_from halts-for-operator on one rather than running it -- so the + raise comes from the replayed SetpointStep's write instead, on a port + that raises for every address except the closing one.""" + + class _RaisingExceptForClosing: + def __init__(self, safe: InMemoryControlPort, safe_address: str) -> None: + self._safe = safe + self._safe_address = safe_address + + async def read(self, address: str) -> Measurement: + if address == self._safe_address: + return await self._safe.read(address) + raise RuntimeError("port read bug") + + async def write(self, address: str, value: Any, **kwargs: Any) -> None: + if address == self._safe_address: + await self._safe.write(address, value, **kwargs) + return + raise RuntimeError("oops") + + def subscribe(self, address: str) -> AsyncIterator[Measurement]: + raise NotImplementedError + + safe = InMemoryControlPort() + safe.simulate_connect("2bma:closing") + appender = _FakeAppendStep() + abort = _FakeLifecycleHandler() + conductor = Conductor( + control_port=_RaisingExceptForClosing(safe, "2bma:closing"), + append_step=appender, + clock=FakeClock(_FIXED_NOW), + id_generator=_SequenceIdGenerator([uuid4() for _ in range(4)]), + resume_procedure=_FakeLifecycleHandler(), + complete_procedure=_FakeLifecycleHandler(), + abort_procedure=abort, + ) + + with pytest.raises(RuntimeError, match="oops"): + await conductor.conduct_from( + procedure_id=uuid4(), + principal_id=uuid4(), + correlation_id=uuid4(), + steps=(SetpointStep(address="2bma:main", value=1.0),), + boundary=0, + closing_steps=(SetpointStep(address="2bma:closing", value=0.0),), + ) + assert abort.calls == [] + landed = await safe.read("2bma:closing") + assert landed.value == 0.0 + + @pytest.mark.unit async def test_conduct_from_clean_tail_completes_with_closing_steps_applied() -> None: port, _ = _routed_port("2bma:shutter", "2bma:closing")