Stop the conduct machinery from silently dropping fields - #744
Merged
Merged
Conversation
Four wrapper terminals rebuilt a ConductorResult by hand-copying a field list, so every field the list omitted was silently dropped. All four omitted substrate_writes; three also omitted actuation_kind. The held arm is the one that matters. A Held Procedure is parked mid-flight waiting for an operator decision, which is precisely when "what did CORA leave set" is the question, and it reported nothing at all. Yesterday's 2-BM conduct is the concrete case: a halt returns from the step loop, so a recipe's own closing steps never run and whatever was opened stays open. Use dataclasses.replace, which cannot drift again by construction and was already the idiom fifteen lines above the conduct_from site. That site now copies merged_result rather than result, so its complete- rejected arm reports the same folded kind its success arm does and the response agrees with the terminal event. Each of the four is pinned by a test verified by mutation: reverting one site fails that site's test and no other. The convergence and decide loops drop the ledger too, deliberately left alone. Their observer is per-execute, so a correct answer there is a union across passes rather than a field copy, and that is a design question for the teardown memo.
RecipeCheckStep.timeout_s promises it "rides expansion and the determinism hash like any other authored value, so a recipe whose deadline changed is a different recipe". Neither step serializer emitted it and _step_from_payload never read it back, so the promise was false twice over: two recipes differing only in their deadline hashed identically, and a resumed conduct rebuilt the check with no wait at all and re-ran it as an instantaneous read. That second one is the dangerous half. A deadline exists because the value takes time to arrive; a check that stops waiting reads the pre-move state and passes or fails on it. Emit the key only when set. Every recipe authored before the field existed has None, so its wire form stays byte-identical and its pinned steps_hash stays valid; a test hashes a deadlineless check against the literal three-key form to pin that. Recipes that do carry a deadline change hash, which is the point. Verified by mutation: dropping the key from either serializer fails that serializer's test and no other. The HTTP conduct body still has no timeout_s arm, so a deadline remains recipe-authored only. Left alone; widening the wire is a separate call.
Three fields have now shipped silently dropped by a hand-written copy, two of them this week: substrate_writes and actuation_kind off the Conductor's wrapper terminals, and CheckStep.timeout_s off both step serializers. Each was invisible to a green suite. The step-serializer round-trips already asserted `rebuilt == instance`, which looks like full fidelity but only exercises a field the representative actually SETS. Every representative left its optional fields at their defaults, so a serializer that dropped one rebuilt the default and compared equal. That is precisely how timeout_s got through, and why ComputeStep.capture_name needed a hand-written test of its own. So require the representatives to set every defaulted field, and the existing round-trips gain their teeth for free: with timeout_s set, re-dropping it from step_to_payload now fails the round-trip test that previously stayed green. Recipe had no evolver carry-forward guard at all, unlike Procedure and Run, so a new field would be wiped by the RecipeVersioned and RecipeDeprecated arms on replay with nothing failing. Add the twin, stricter than Procedure's: only `status` is structural, so id, name and capability_id are checked as real carry-forwards. Both arms already pass; this is prevention, and the aggregate is where the next additive field lands. Verified by mutation: unsetting a representative field, re-dropping timeout_s, dropping a prior.* thread, and adding an unregistered field each fail the intended guard and no other.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
conduct_until_converged and conduct_until_advised had the same field-drop bug #744 fixed on conduct() / conduct_or_hold() / conduct_from(): five branches built a fresh ConductorResult next to a `result` (or `merged`) already in scope, copying only completed_count and measurements, so substrate_writes / artifacts / outputs silently read empty on an abort or complete-rejected terminal that followed real hardware writes. Two more branches (_abort_unconverged_cap, _abort_absolute_ceiling) take an Optional last_result and had already threaded completed_count/measurements through a None-safe ternary but not the rest of the ledger. Found while scoping the ConductorResult construction-site guard for the upcoming closing-steps field: writing the guard meant reading every construction site, which is exactly how the four wrapper-level instances surfaced in #744 too. Four of the six byte-frozen convergence-loop methods changed as a result; the digest pin is updated deliberately in this commit per its own docstring.
…ound Three field-drops have now shipped from the same shape: a fresh ConductorResult built next to an existing result already in scope, hand-copying only some of its fields. #744 fixed four sites by reading the file; the prior commit found five more the same way. That process does not scale to the next reviewer, and it is exactly the guard the closing-steps design needs before it adds a closing_failures field to this type. Registers every direct ConductorResult(...) construction (14 remain after both fixup commits; replace() sites are complete by construction and need no entry) against the exact set of fields it is known to leave at their default, with the reason. A construction that newly drops something, drops something different, or a stale registry entry from a renamed/removed site, both fail loud. Adding a field to ConductorResult will make every registered site "wrong" the moment that field is not threaded, forcing a decision at each one rather than a silent default.
…re has ConductOrHoldProcedureResult and ConductFromProcedureResult never carried measurements / artifacts / outputs / substrate_writes at all, despite both docstrings claiming to mirror ConductProcedureResult: conduct_or_hold_procedure/handler.py hand-built its result from six fields when the Conductor handed back nine (the fifth field-drop instance in this tree), and conduct_from_procedure's dataclass never had the fields to drop in the first place. Worst arm on each: a Held Procedure's closing steps have not run, and an acquisition halt leaves the Procedure Running with a setpoint re-driven, so substrate_writes is exactly the ledger an operator needs on precisely these two outcomes. Also fixes the #740 MCP omission for real this time: conduct_procedure's own _ToolResult never carried substrate_writes despite the REST route having it since #740, proven by grepping the tool result construction directly. All three slices now carry substrate_writes on REST response + MCP tool, sharing one `substrate_writes_to_wire` helper (a third inlined copy would have been the next field-drop-by-copy site). measurements / artifacts / outputs stop at the application-layer dataclasses for now (matching conduct_procedure's existing wire scope, which never surfaced them either) -- extending the wire surface to all three fields is a separate, unscoped decision. openapi.json regenerated (two additive substrate_writes properties).
The conduct verb-family is the one place an MCP tool hand-copies a REST response into a separate _ToolResult class instead of returning the response model directly (failure needs a plain dict on the MCP side, a typed sub-model on the REST side). That hand-copy is exactly the shape #740 shipped from: conduct_procedure's own _ToolResult omitted substrate_writes for a full cycle after the REST route gained it, and no test compared the two field sets to notice. Discovers every _ToolResult under cora.operation.features by walking the filesystem (six today) rather than a hardcoded list, finds its sibling route.py Response class, and asserts the field NAMES match (types may differ; that divergence is the documented reason _ToolResult exists at all). Verified by mutation: dropping substrate_writes from conduct_procedure's _ToolResult again reproduces the exact #740 diff and fails loud.
4 tasks
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.
Three fields have shipped silently dropped by a hand-written copy, two of them
this week. Each was invisible to a green suite. This fixes them and adds the
two guards that would have caught them.
Groundwork for a conduct teardown phase: that work adds a field to exactly this
machinery, and the Recipe side had no carry-forward guard at all.
Keep the write ledger on the arms an operator triages from
Four wrapper terminals rebuilt a
ConductorResultby hand-copying a field list.All four omitted
substrate_writes; three also omittedactuation_kind.The held arm is the one that matters. A Held Procedure is parked mid-flight
waiting for an operator decision, which is exactly when "what did CORA leave
set" is the question, and it reported nothing at all.
dataclasses.replaceinstead, which cannot drift again by construction. Theconduct_fromsite now copiesmerged_result, so its complete-rejected armreports the same folded kind its success arm does.
Let a check's deadline reach the hash that decides recipe identity
RecipeCheckStep.timeout_sdocuments that it "rides expansion and thedeterminism hash like any other authored value". Neither step serializer emitted
it and
_step_from_payloadnever read it back, so two recipes differing only intheir deadline hashed identically, and a resumed conduct rebuilt the check with
no wait and re-ran it as an instantaneous read.
Emitted only when set, so every recipe authored before the field existed keeps
its wire form and its pinned
steps_hash. A test hashes a deadlineless checkagainst the literal three-key form to pin that.
Guard the class
The step-serializer round-trips asserted
rebuilt == instance, which looks likefull fidelity but only exercises a field the representative SETS. Every
representative left its optional fields at defaults, so a dropped field rebuilt
the default and compared equal. Requiring representatives to set every defaulted
field gives the existing round-trips their teeth back.
Recipehad no evolver carry-forward guard, unlikeProcedureandRun. Addedthe twin, stricter: only
statusis structural.Verification
Every change verified by mutation, not just by passing: reverting each of the
four result sites fails that site's test and no other; re-dropping
timeout_sfrom either serializer fails that serializer's test; unsetting a representative
field and adding an unregistered Recipe field each fail the intended guard.
No wire change, no OpenAPI change, no event-payload change, no deployment step.
Deliberately out of scope: the convergence and decide loops drop the ledger too.
Their observer is per-
execute, so a correct answer there is a union acrosspasses rather than a field copy, which is a design question for the teardown memo.
🤖 Generated with Claude Code