Name the two quality floors, and unblock a beam gate that could never pass - #721
Merged
Conversation
… pass A consumer reading `Measurement.quality` is choosing between two different questions: can I BELIEVE this value, where only `Bad` disqualifies, or can I ACT on it, where any annotation is a reason to stop. The enum's docstring has said so since it was written, and asked each use site to state which it means. The only vocabulary available for saying it was a raw comparison, and spelled that way the two are indistinguishable on sight: which one is correct depends on whether the consumer records or acts, and on which state the facility chose to alarm. Three independently written consumers answered the believe question with the act test. The hutch permit (2026-08-09), the BLEPS interlock flags (2026-08-23), and now the beam-availability gate. That is the rule-of-three trigger, and a third one-line patch would leave the fourth consumer to re-derive the same answer. So the two questions get names in `cora.shared.quality`, and every one of the ten decision sites now declares which it is asking. `Quality` itself moves there, beside `ReachTier` and for the same reason: consumers in three BCs read it and `cora.shared` is the only module all of them can reach. `measurement.py` re-exports it, so no import elsewhere changed. The beam gate is the live bug the trigger surfaced, and it was worse than a wrong severity. Measured on arcturus: all three configured PVs carry `ZSV=MAJOR` / `OSV=NO_ALARM`, so state 0 alarms and state 1 is silent. On `BeamBlockingM` the polarity is INVERTED, so state 0 is the shutter OPEN. Under the strict floor a shutter had to be silent to be believed, which meant blocking; an open one was discarded as unreadable. `fes_open` and `sbs_open` were structurally incapable of being True, so the gate refused every run in every state of the beamline, and presented it as `RunBeamAvailabilityUnknown` rather than as anything pointing at CORA. The blocked start path was not the whole cost. `witness_safety_envelope` runs the same predicate and records rather than raises, so every witnessed run at 2-BM was stamped `beam_available=false` whatever the shutters were doing: a false fact in the record CORA exists to keep. An architecture fitness function refuses any comparison against a quality literal outside the module that defines the floors, so a fourth consumer picks a name instead of a side. Its allowlist is one entry. Six substrate adapters were in it at first on the assumption that translating a native severity enum needs the literals; it does, but never as a comparison, so those entries were exemptions granted against nothing.
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.
Why
A consumer reading
Measurement.qualityis choosing between two different questions:Baddisqualifies.Uncertainis what a substrate reports when the value is fine but the process it describes is in alarm.Goodonly.The enum's docstring has said so since it was written, and asked each use site to state which it means. The only vocabulary for saying it was a raw comparison, and spelled that way the two are indistinguishable on sight: which is correct depends on whether the consumer records or acts, and on which state the facility chose to alarm.
Three independently written consumers answered the believe question with the act test:
SecureM)That is the rule-of-three trigger. A third one-line patch leaves the fourth consumer to re-derive the same answer.
What
cora.shared.qualitynames the two floors,believableandactionable, and all ten decision sites now declare which they ask:believable: beam-availability lookup (the bug), enclosure permit observer, BLEPS supply observer (verdict + diagnostic), capture baseline reader.actionable: Conductor check steps (×2), Conductor capture assertion, optimizer observation inputs (×2).The six
actionablesites were already correct. Converting them is not cleanup for its own sake: the bug happened because both questions looked identical in source, and leaving half the sites in the old spelling preserves exactly that.Qualitymoves tocora.sharedbesideReachTier, for the same reason that one moved: consumers in three BCs read it andcora.sharedis the only module all of them can reach (depends_on = []).measurement.pyre-exports it, so no import anywhere else changed.The live bug
Measured on arcturus 2026-08-24, from the records' own fields: all three configured beam PVs carry
ZSV=MAJOR/OSV=NO_ALARM, so state 0 alarms and state 1 is silent. OnBeamBlockingMthe polarity is INVERTED, so state 0 is the shutter open.Under the strict floor a shutter had to be silent to be believed, which meant blocking; an open one was discarded as unreadable. So
fes_openandsbs_openwere structurally incapable of beingTrue, and the gate refused every run in every state of the beamline. It surfaced asRunBeamAvailabilityUnknown, which points at the facility rather than at CORA.The blocked start path was not the whole cost.
witness_safety_enveloperuns the same predicate and RECORDS rather than raises, so every witnessed run at 2-BM was stampedbeam_available=falsewhatever the shutters were doing: a false fact in the record CORA exists to keep. 2-BM runs through the witnessed path today, which is why the blocked half had not been hit yet.Believing an alarmed reading is safe here for the reason it is safe in the permit observers: ACIS and the PSS hold the shutters, CORA holds nothing.
Bad(EPICS INVALID) still fails closed.What stops the fourth one
test_quality_floors_are_namedwalks the AST of every tracked module and fails on any comparison against a quality literal. AST rather than text, so the strings in docstrings and log payloads (describing a floor, not applying one) do not register.Its allowlist is one entry, the module that defines the floors. Six substrate adapters were in it at first, on the assumption that translating a native severity enum needs the literals. It does, but never as a comparison: they map with a dict and return a constant, so the check never saw them and those entries were exemptions granted against nothing. Removing them is the difference between an allowlist that describes the guard and one that quietly widens it.
Verification
Full suite, architecture (34,121), pyright, tach all pass. Mutations run:
actionablebelievablecollapsed toactionableThe beam tests now carry the two shapes measured on arcturus rather than invented ones. That mattered: every invented shape in that file missed the defect, because it needs the real combination of an inverted polarity AND an alarm on state 0.
Two notes for the reviewer
A pre-existing failure this PR does not cause.
test_run_debriefer_seed::test_seed_warns_when_an_already_seeded_agent_is_not_promotedfails in a full-suite run. It reproduces on cleanmain(1fb7b9f) from two test files alone:pytest tests/unit/observability/test_handler_spans_via_testclient.py tests/unit/agent/test_run_debriefer_seed.py.configure_loggingrebuildslogging.StreamHandler(sys.stdout)on every call and binds the stream at that moment, so once an earlier test builds an app the root handler points at a stale stdout and the later test'scapsyssees nothing while the line still lands on the real stdout. Deterministic given ordering, not a flake, and untouched by this diff. Worth its own fix.Overlap with #688. That PR adds a docstring paragraph to
permit_status_from_reading, four lines above the line this PR edits, so whichever lands second needs a small rebase. #688 is already stale against main in that same docstring (it carries the pre-#718 DBR_ENUM wording), so it needs one regardless. No semantic conflict: the floor is unchanged, only its spelling.🤖 Generated with Claude Code