Give recipes a second, terminal-only closing-step list - #748
Merged
Merged
Conversation
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.
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.
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.
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.
…n 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.
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.
Coverage report
Click to see where and how coverage changed
The report is truncated to 25 files out of 40. To see the full report, please visit the workflow summary page. This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
closing_stepslist the Conductor walks once the main list reaches a real terminal (Completed or Aborted), never on Held, an acquisition halt, or cancellation. Closing steps are isolated from each other (one failing doesn't stop the rest or flipsucceeded).conduct,conduct_or_hold, andconduct_fromall threadclosing_stepsthrough to the Conductor; the three loop-driving slices (conduct_until_converged,conduct_until_advised,conduct_until_advised_from) refuse a closing-bearing recipe outright (422, v1 scope).conduct_fromalso gainsClosingCaptureBeforeBoundaryError: a closing step'sCaptureRefnaming a capture only a pre-boundary main step declares is rejected up front (422), rather than failing deep inside the closing walk's isolation.flat_field's shutter-close (steps 4-5) moved from the main step list intoclosing_steps, so a halt mid-capture still closes the shutter instead of leaving it open.Built on #744 (already merged), which hardened the Conductor's result plumbing to make this addition safe.
Test plan
tests/unit/ tests/architecture/ tests/contract/suite green (52391 passed, 637 skipped)openapi.jsonregenerated, drift test passesmkdocs build --strictclean after the docs update🤖 Generated with Claude Code