Give a Procedure hold claims, so a fault and a pause don't share one bit - #801
Merged
Conversation
…ne is `ProcedureStatus.HELD` is one bit. That was adequate while a hold had a single author, and became a fault once four independent concerns could each park the same conduct: two operator surfaces, a recoverable step failure, and a steering driver going non-ACTIVE. A second holder arriving at an already-Held Procedure is refused and records nothing, and the first holder's resume then restarts the conduct with the second's cause unenforced. Which concern survives depends only on arrival order. Run hit this exact fault, fixed it with cause-scoped claims, and wrote it up in project_hold_claim_ordering_fault. This ports the aggregate half of that design rather than inventing a second one: `ProcedureHeld` carries `claim_id` + a closed `cause`, `ProcedureResumed` carries `released_claim_id`, an audit-only `ProcedureHoldClaimReleased` discharges a claim without resuming, and claims fold onto `Procedure.hold_claims`. Causes are the three concerns that actually park a conduct today, not Run's four: `operator`, `step-fault`, `driver-stand-down`. The last two are separate because they are discharged by different things, the equipment recovering versus the driver being reinstated, so they must be able to hold at once. Nothing changes behaviour yet. No decider enforces resume-only-when-last and no writer passes a cause, so every hold still folds to the single legacy claim and every resume still clears it. That is deliberate: this slice makes the aggregate able to represent the truth, and the enforcement follows in its own slice with its own review. Legacy streams replay to exactly their old one-bit meaning. A claimless hold folds to LEGACY_CLAIM_ID, repeated claimless holds collapse rather than accumulate, and a bare resume clears everything. Two guards earned their keep. The evolver carry-forward fitness test caught the new state field immediately, which is the drift catcher working as designed; its registry is updated with the arms that write the field. And the generated record-export disposition table needed regenerating, so the new keys are dispositioned exactly as Run's are rather than diverging. The release arm uses `replace` rather than a hand-listed constructor, deliberately: re-listing seventeen fields to change one is how a field gets silently dropped, which is the bug class this slice's own field nearly fell into. Verified by mutation: making a release clear every claim fails three of the new tests, including the one pinning that a Procedure stays Held while another concern holds it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
S1 let a Procedure record which concerns hold it. This makes the deciders act on that record, which is the half that changes behaviour. Holding: `Held` becomes a legal starting status, so a second concern can record its intent on an already-parked conduct. That was previously refused outright, which is the fault: the second holder's cause went unrecorded and the first holder's resume then restarted the conduct with it unenforced. The guard moves from "is this Procedure un-held" to "is THIS CONCERN already holding it", so the alternation the old rule protected survives, scoped to the concern rather than the Procedure. Resuming: the caller discharges its own claim and the decider picks. Last active claim, emit ProcedureResumed and run. Others still holding, emit the audit-only ProcedureHoldClaimReleased and stay Held. No claim of its own, raise ProcedureHoldClaimsRemainError naming who is still holding, so the caller learns which concern to address rather than only that it was refused. Causes are not reachable from the wire. `cause` defaults to `operator` and neither the REST route nor the MCP tool exposes it, so a caller cannot label a machine-parked conduct an operator pause and inherit an operator's authority to clear it. The Conductor will set its own causes when the writers are wired; that is the next slice. Legacy holds stay resumable. A hold placed before claims existed has no owner, so no derived id matches it and every Procedure held at that moment would be stuck forever. An operator resume owns the legacy claim in addition to its own, since the operator is the authority that could always clear such a hold; a machine concern deliberately does not, so a step fault clearing cannot silently adopt a hold nobody can attribute. THREE TESTS CHANGED THAT PINNED THE OLD RULE, and they are the ones worth looking at rather than the payload churn: test_decide_rejects_non_running_status asserted "re-holding a Held one too" raises, and the two property tests derived their permitted set from `_HOLDABLE_SOURCES = (RUNNING,)`. All three encoded exactly the behaviour this commit deliberately changes, the same way seven Run tests did before its hold-claim fix. The replacement protection is the per-claim guard, covered directly. Two fitness tests earned their keep again: the decider-docstring invariants block caught both new raises going undocumented, and the generated disposition table needed regenerating for the release event's decision link. Verified by mutation: making a resume ignore other holders fails the test pinning that a discharge leaves the conduct Held. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ator clear it
S1 gave a Procedure somewhere to record which concerns hold it, and S2 made
the deciders act on that record. Every hold still folded to a single claim
because no writer passed a cause. This wires the Conductor's two.
Passing the causes alone would have wedged the system, which the change had
to answer before it could ship. `conduct_or_hold` parks a conduct on a
recoverable step failure; `_hold_driver_stood_down` parks a steered loop
whose driver was switched off. Under S2's rule that each concern discharges
its own claim, neither hold could ever be discharged: nothing in CORA
re-establishes a faulted step or reinstates a driver, and an operator's
resume derives a different claim id. Every fault-parked conduct would have
become permanently unresumable. Reproduced at the decider before writing
the fix:
WEDGED: ProcedureHoldClaimsRemainError ... still held by step-fault.
The wedge exposes a distinction the claim algebra had been carrying
implicitly. A hold claim is one of two things. An AUTHORITY claim is held
on behalf of a rule the holder enforces, and letting anyone else clear it
is the bypass it exists to prevent: Run's ratification and kill-switch
claims are these, which is why `resume_run` refuses an operator outright.
An ATTENTION claim is the opposite. The Conductor is reporting a conduct it
cannot itself un-stick, the claim's job is to say what an operator would be
resuming past, and an operator resuming IS the discharge.
So `ATTENTION_HOLD_CAUSES` records which Procedure causes are which, and an
operator's resume clears its own claim, a legacy claim, and every attention
claim. The set is declared rather than assumed, with
`test_every_hold_cause_is_classified` failing until a newly added cause is
put on one side or the other. "May an operator clear this" is the question
a new concern exists to raise, and inheriting an answer is how it would go
unasked.
Clearing several claims at once needed the emission to change. The caller's
own claim rides the `ProcedureResumed`; each claim it clears on another
concern's behalf gets its own `ProcedureHoldClaimReleased` first. A resume
that answers three concerns then records three discharges rather than one
status change that silently absorbed two of them.
WHAT DOES NOT CHANGE TODAY: every resume in the system is an operator
resume, since `cause` defaults to `operator` and no wire surface exposes
it, and an operator may clear every Procedure cause. So
`ProcedureHoldClaimsRemainError` does not fire in production. It is the
guard that keeps a future machine resumer from clearing a hold it never
placed. What changes now is that a machine hold is RECORDED at all: before
this, a conduct faulting on an already-held Procedure filed its fault under
the operator's claim, was refused as a duplicate, and the cause went
nowhere.
A GUARD, because the widening rests on something outside the algebra.
`operator` is the DEFAULT cause, so it means "the caller named no other
concern", and nothing checks what kind of thing the caller is. Every
unmarked resume traces to a person today only because no agent is granted
`ResumeProcedure`; grant one tomorrow and it defaults into `operator` and
inherits authority over every machine hold with nobody having decided that.
`test_no_agent_is_granted_resume_procedure` puts that where it can be seen,
in the grant table rather than in a docstring, and says what to do instead
if an agent ever needs to resume.
The same pressure is why a machine is already an operator in one place:
`_experiment_coordinator` has an in-process `HoldProcedure` grant and sets
no cause, so an agent's own decision is filed as "a person asked".
A DELIBERATE DIVERGENCE FROM RUN, since the two now differ and a reader
will ask. `resume_run` exposes `cause` on both the REST route and the MCP
tool, added because a kill-switch hold otherwise had no exit but abort_run
and its destroyed beamtime; on a Run, naming the cause is how a person says
which claim they are clearing. `resume_procedure` does not expose it, and
an operator's ownership of attention claims does that job instead. Both let
a person un-stick a machine-parked thing. Run's records the intent more
precisely, this one takes one call. The difference is defensible because no
Procedure claim is an authority claim, and it stops being defensible the
moment one is, which is what the classification test exists to catch.
One interaction is deliberately left alone. `_experiment_coordinator` still
issues a follow-on hold under the default operator cause, so on the Held
path it now records a mislabelled operator claim where it previously
raised. That whole call is removed by 187c55455cc on
`fix-coordinator-holds-aborted-procedure`, which found it invalid on every
path that reaches it; wiring a cause into code being deleted would only
conflict with that fix, and `steer_experiment` has no production caller.
THREE INTEGRATION TESTS CHANGED, and one of them was already broken. The
two `conduct_from` soft-IOC tests pin the exact event stream, which now
carries the release between the hold and the resume; that path parking and
resuming against a real IOC and Postgres is the end-to-end proof the wedge
is gone. The third, `test_a_person_and_an_agent_hold_through_one_gate_and_
the_record_agrees`, compares two Procedures' ProcedureHeld payloads field by
field after stripping what must differ. S2 began writing a claim_id derived
from the procedure id, so it differs for the same reason procedure_id does,
and nothing stripped it: that test has been red since 479b9ae12d9 and was
reported green on a test count that did not cover it. Fixed by asserting the
field rather than dropping it, so both paths still have to file a hold under
the same cause, differing only in which Procedure the claim is on.
Verified by mutation, five separately: dropping the operator's ownership of
attention claims, dropping either Conductor cause, dropping the extra
release events, granting an agent `ResumeProcedure`, and deriving a claim
from a fixed id instead of the Procedure's each fail tests that name the
behaviour rather than the shape.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ek for both The ProcedureWatcher clocked every Held procedure against one week-long window because `status` is one bit and cannot say WHICH concern is holding. That window exists for a real case: a deliberate operator pause (a bakeout, waiting on beam, waiting on a collaborator) legitimately runs for days, and an hour-long window false-flagged every one of them. But `Held` never meant "deliberate pause". The Conductor parks a conduct on a recoverable step fault or a stood-down steering driver, and that is the opposite case: nothing in CORA will move it until a person comes, so a week of silence is precisely wrong for it. The watcher could not tell the two apart, so it gave every hold the benefit of the doubt and the case most worth surfacing was the one it hid longest. Now that a hold records its cause, it does not have to. The long window is not retired, it is narrowed to what its own config comment always described. What changed is the question that selects it. THE RULE IS POSITIVE EVIDENCE, not absence of a fault. The long window applies only when `hold_causes` is non-empty AND every cause in it is `operator`. An empty set does not qualify, and neither does `legacy-unscoped`: a Held row this watcher cannot vouch for takes the short window, because treating "no evidence" as "deliberate" is exactly how the case worth catching would go on hiding. Being wrong costs one advisory Decision a person can ignore, which is the cheap direction for a flag-only watcher. Two consequences worth stating: a deployment carrying pre-claim holds gets one flag per procedure, which is the right answer to "nobody can say why this is held"; and an operator pausing a conduct does not vouch for a fault that arrived alongside it, so one non-operator cause loses the long window. The causes reach the watcher through the read model. `hold_causes text[]` is denormed onto `proj_operation_procedure_summary`, maintained by the Held (append) / HoldClaimReleased (remove) / Resumed and three terminal (clear) arms. Denorming the CAUSES rather than a precomputed "needs attention" flag keeps the classification in `ATTENTION_HOLD_CAUSES`: baking it into the projection would leave old rows silently wrong the day that set changes, and it makes "held by what" answerable on a plain list read. The alternative considered was folding the aggregate per candidate, which `_run_supervisor` already does for Runs; it was rejected for costing a full stream fold per held procedure per tick and for leaving the fact invisible to every other reader. The append is deduplicated by cause rather than a bare `array_append`, since a re-delivered `ProcedureHeld` must not stack a second copy. That mirrors the aggregate, where `_with_claim` is idempotent on an already-active claim. A pre-claim hold carries no cause and folds to `LEGACY_CAUSE` there, so it lands as that string here too rather than as a Held row claiming nothing holds it. The migration backfills currently-Held rows the same way. THE UNIT TESTS COULD NOT HAVE CAUGHT A BROKEN ARRAY UPDATE: the projection's tests mock the connection, so this SQL had never executed. The Postgres integration test now drives the whole chain instead, a conduct parking itself, a person pausing the same conduct, causes accumulating oldest-first, and one resume draining both. ONE MISS WORTH RECORDING. The architecture suite passed 51,665 tests with `EXPECTED_SCHEMA_VERSION` stale, because `test_schema_version_pin` reads `tracked_migration_files()` and the new migration was still untracked. The integration gate, which reads the directory, failed 15 tests and errored 8 on one root cause. The pin's guard was never wrong; running it before staging was. Confirmed by reverting the pin with the file tracked, which fails it immediately. PRE-COMMIT REJECTED THE FIRST ATTEMPT, on a boundary worth recording rather than working around. `cora.api` may not import `cora.operation.aggregates`, and the watcher was reaching in for `HOLD_CAUSE_OPERATOR` to restate the rule locally. The rule is domain knowledge about causes, so it moved to `is_deliberate_pause` beside the causes it classifies, where whoever adds the next cause will see it; the watcher applies it rather than repeating it. That still needs the dependency, which is now declared with its reason alongside the six `*.aggregates` edges `cora.api` already carries, including the `cora.access.aggregates` one this same runtime uses for `Actor.active`. Stating the rule as positive evidence rather than as "no attention cause is present" also settles a case that does not exist yet: a cause that is neither `operator` nor an attention claim, which is what an authority claim on a Procedure would be. Such a hold is not somebody taking a break, and under the negative phrasing it would have inherited a pause's latitude by being unclassified. Verified by mutation: treating an empty cause set as a deliberate pause, and selecting the window by status again, each fail tests that name the behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Split out of `port-procedure-hold-claims`, which mixed this with an
unrelated tach-audit/port-pinning thread (that half is #800). This half
stands on its own.
The gap
A paused Procedure said `HELD` and nothing else: one bit, not who paused
it or why. That collapsed two situations a beamline needs told apart:
week; the ProcedureWatcher's staleness window assumed exactly this.
without a person, and a week of silence before anyone is told is wrong.
Worse, a Held Procedure that then hit a fault had nowhere to record the
second cause: "already held" refused the parking attempt outright, and
the fault vanished. An operator resuming afterwards walked straight back
into the unfixed fault.
What changes
does (`hold_claims.py`, append/dedupe on `Held`, remove on
`HoldClaimReleased`).
Procedure, instead of silently discharging a hold nobody cleared.
can clear that claim directly rather than the watcher inferring it from
a stopwatch.
Conductor-parked hold a separate, week-shorter staleness window than a
deliberate operator pause. Both are recorded and both are cleared
explicitly when the person resumes.
Denormalized onto `proj_operation_procedure_summary` as `hold_causes`
(migration `20260911193339_procedure_summary_add_hold_causes.sql`) rather
than a precomputed flag, because the "needs attention" classification
(`ATTENTION_HOLD_CAUSES`) can change independent of the data; baking it in
would leave old rows silently wrong the day that set does. A row Held
before this shipped backfills to `LEGACY_CAUSE` rather than an empty
array claiming nothing holds it.
Migration note
This branch's migration was originally timestamped
`20260910100000`, which sorted before `20260910222120_init_proj_data_shortfall_summary.sql`
(landed on main via #798 while this branch sat unpushed). Renamed to
`20260911193339` so it lands after the current tip, and regenerated
`atlas.sum` with `atlas migrate hash` rather than hand-editing the
checksum file. `EXPECTED_SCHEMA_VERSION` updated to match.
Verification
projection tests) run in CI; no local Docker available in this session
to pre-run them.
🤖 Generated with Claude Code