Slice 12: genesis baseline, read a witnessed Run's PVs once at BEGUN - #665
Merged
Conversation
Adds capture_baseline_pvs, a sibling of capture_watch_pvs shaped code -> channel_name -> PV rather than code -> role -> PV: baseline needs on the order of twenty PVs per code (scan geometry plus beamline conditions TomoScan never reports), which does not fit the closed role vocabulary capture_watch_pvs already dispatches on by name. Adds the fourth independent kill switch, capture_baseline_recording_enabled, mirroring capture_progress_recording_enabled's shape. Neither is wired to anything yet; the boot guard and reader land in follow-up commits.
…_recording_enabled Fourth kill switch, same shape as capture_progress_recording_enabled's existing gate: a baseline read has no promoted Run to attach to without recording turned on.
A separate principal from RunWitness and CaptureProgressFeeder,
mirroring why CaptureProgressFeeder got its own: an operator can
revoke baseline-writing without blinding either the witness or the
progress feeder, and actor_id tells the three runtimes' rows apart.
Opens the ba5e ("BASE") mnemonic UUID block. Not wired to any runtime
yet; the reader itself lands next.
Reads every channel declared for a capture_code exactly once and appends whatever survives as one AppendObservations batch with sampling_procedure="baseline". Three skip rules per reading: Bad quality, no substrate time, non-numeric value; a MAJOR/MINOR-collapsed Uncertain quality is kept, mirroring the alarm-vs-fault split already shipped elsewhere. Every failure (a dead PV, an uncoercible reading, or the append call itself) is caught and logged, mirroring CaptureProgressFeeder's posture: a baseline read must never prevent or unwind the promotion that triggers it. Not wired into RunWitnessRecorder yet.
RunWitnessRecorder._promote calls the configured baseline reader exactly once, right after a successful promotion, gated on the fourth kill switch (capture_baseline_recording_enabled) read directly off settings the same way run_witness_recording_enabled already is. A baseline-read failure is caught and logged, never unwinding the promotion that already committed. run_witness_lifespan builds the reader when capture_baseline_pvs is declared, requiring record_witnessed_run, control_port, and append_observations, mirroring the existing capture_progress_recording_enabled gate shape.
Seeds the CaptureBaselineReader agent alongside RunWitness and CaptureProgressFeeder, and passes the shared ControlPort plus capture_baseline_pvs into run_witness_lifespan so a deployment that declares CAPTURE_BASELINE_PVS gets a real reader without any other wiring change.
Baseline channels have no per-role decoder in production (every channel is treated identically), so the sweep reports kind/value/units with verdict n/a, except a non-numeric value is flagged BAD -- the one defect checkable ahead of a real append attempt, since Observation.value is float. Reuses finite_float from _capture_observer so this can never drift from what CaptureBaselineReader itself accepts. Baseline lines are tagged group="baseline" and rendered with a "baseline:" role prefix so a human reading the combined report can tell the two groups apart.
"Scan aborted" -> "Aborted" was never a real mapping target:
decarlof/tomoscan@master never calls ScanStatus.put("Scan aborted"),
verified by extracting every ScanStatus.put() call across
tomoscan.py, tomoscan_2bm.py, and tomoscan_pso.py (an Aborted phase
reaches the recorder through the separate abort role instead). Two
literals upstream DOES write, "Error writing configuration" and
"Config File Write Error", are absent from the example and stay that
way: UNRECOGNIZED is the honest answer until an operator decides what
phase, if any, a config-write failure maps to.
Comment-only change, no behavior difference.
…seline() Two findings from the naming reviewer: 1. _PvReport.role was populated with a channel_name for baseline lines, which are not roles; renamed to pv_key, the field's actual structural meaning across both groups. 2. CaptureBaselineReader.read_baseline stuttered at every call site (self._baseline_reader.read_baseline(...)). Renamed to the bare read(), matching CaptureProgressFeeder's bare-verb methods (offer, flush). No behavior change.
…rently Gate review findings: Security (moderate): PostgresObservationStore.append writes an entire batch in one executemany call, so a single reading whose units exceeds the DB's 64-char CHECK bound would fail EVERY other reading in the same promotion's batch, not just the offending channel -- silently undermining this module's own per-PV-independence guarantee. Reject an oversized units string per-reading, ahead of the batch, mirroring the existing non-numeric-value rejection. Architecture (moderate): read() looped sequentially over every declared channel inside RunWitnessRecorder._promote, itself on run_witness_loop's single consumer path, so a slow or partially-unreachable control system could block the loop from reacting to the next lifecycle observation for the sum of every PV's own timeout. _read_one already never raises, so a plain asyncio.gather over the per-channel reads is safe and removes the sequential blocking without changing any failure-posture guarantee.
1. Pin that capture_baseline_recording_enabled is read fresh on every promotion, not cached at RunWitnessRecorder construction: flips the flag on a live Settings object between two promotions on the same recorder instance and asserts the second one alone reads the baseline. 2. Pin the F2 preflight's baseline verdict contrast with the real reader: a Bad-quality or timestamp-less reading still reports ok=True / verdict="n/a" in the preflight sweep, since only non-numeric is checkable ahead of a real append attempt.
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.
Summary
Closes the conditions-snapshot gap identified in the pre-live plan: when a capture promotes to a witnessed Run,
CaptureBaselineReaderreads a deployment-declared set of PVs ONCE through the existingControlPortand writes them asObservationrows via the existingAppendObservationscommand, withsampling_procedure="baseline". No new event, no new projection, no migration — the Plan's declared defaults stay ineffective_parametersas declared intent (D1 stands), and the baseline rows carry the observed truth.capture_baseline_pvs(dict[str, dict[str, str]], code ->channel_name-> PV), a sibling ofcapture_watch_pvswith an open inner-key vocabulary since baseline needs ~20 PVs per code and doesn't fit the closed role set. Reasoning is written into the setting's own comment.capture_baseline_recording_enabled, defaulting False, refusing to boot when True withoutrun_witness_recording_enabled(mirrors the existingcapture_progress_recording_enabledgate).CaptureBaselineReader(cora/api/_capture_baseline_reader.py) reads every declared channel concurrently, skips a reading for Bad quality, absent substrate time, non-numeric value, or oversizedunits, and appends whatever survives as one batch. Every failure is caught and logged — a baseline read must never prevent or unwind the promotion that triggered it.CaptureBaselineReader, a separate principal from RunWitness and CaptureProgressFeeder, so baseline-writing can be revoked without blinding either.capture_watch_preflight.py) extended to sweepcapture_baseline_pvstoo: no per-channel decoder, so it reportskind/value/unitswith verdictn/a, flagging only a non-numeric value as BAD.CAPTURE_STATUS_PHASESexample, which listed"Scan aborted": "Aborted", a literaldecarlof/tomoscan@masternever actually writes.Energy has no located readback PV and is deliberately left undeclared (not guessed).
Gate review
Ran a 3-baseline + security-specialist panel before the final commits. All four lenses came back clean except two real findings, both fixed:
PostgresObservationStore.appendwrites a whole batch in oneexecutemany; an oversizedunitsstring would have failed every other reading in the same promotion's batch, not just the offending channel. Now rejected per-reading, ahead of the batch (READING_UNITS_MAX_LENGTH).run_witness_loop's single consumer path. Now concurrent viaasyncio.gather(_read_onenever raises, so this is safe).A naming review (naming-r3-reviewer) also caught and fixed two issues:
_PvReport.rolewas misnamed for baseline lines (renamed topv_key), andCaptureBaselineReader.read_baselinestuttered at its call site (renamed to bareread, matchingCaptureProgressFeeder'soffer/flush).Test plan
make test-noio— 47,876 passed, 630 skippedmake test-db— 1,196 passed, 1 skippedmake lint/make typecheck— clean across the full treecreate_app()capture_watch_preflightrun against the live 2-BM host onceioc2bmb1is reachable (currently down per the pre-live plan's testing note)🤖 Generated with Claude Code