Slice 17: auto-ingest a witnessed Run's scan file over SSH - #682
Merged
Conversation
The 2-BM pilot recorded 133 witnessed Runs and zero Datasets: ingest_scan is a POST route / MCP tool a human invokes, tenable when scans were supervised one at a time but not once vertical_lens_series.sh runs 510 unattended. CaptureScanIngestor sweeps terminated witnessed Runs whose capture path resolved and issues the existing IngestScan command for each, attributed to a new pinned agent principal, behind its own kill switch (capture_scan_ingestor_enabled). The scan files live on the detector host (tomdet), not on CORA's own: measured, pulling one ~24 GB file over the 1 GbE link takes roughly twice the scan cadence, so the bytes stay put and the read/digest runs remotely over SSH, shipping back only a JSON verdict. The locator is untrusted (sourced from a Channel-Access-writable PV), so it never reaches argv, travels over the SSH child's stdin, and is confined by the same resolve_confined_file_uri check the local adapters already use, enforced on whichever host actually holds the bytes. Gate review (4-agent panel: architecture / test-coverage / cross-BC consistency / security specialist) found 2 P0s, both fixed here: - Head-of-line blocking: the original one-oldest-candidate-per-tick design let one permanently-stuck run (a missing binding, an unparseable timestamp) block every newer run forever. tick() now tries up to 10 candidates per tick, excluding each one it gives up on, stopping at the first success or the first systemic failure. - A previously-empty, previously-inviolate cross-BC-SQL architecture fitness test didn't fire on an earlier full-suite run because the new file was untracked at the time (the AST scanner walks tracked files only) -- allowlisted with a citation, the sanctioned escape hatch for a query neither BC's own store can express alone. A third boundary violation surfaced only at commit time from tach-check: cora.api depending on cora.data.aggregates to catch IngestScan's own composed errors is the identical blessed-cross-BC-access pattern already granted to five other BCs' aggregates for the same reason; added with the same citation convention. Also fixed: a PII leak (the vaulted observed_path reaching log lines across four call sites); the never-raise contract broken on a malformed remote response; a missing startup authorization probe and per-tick log spam on a standing denial; several untested settings validators and the new boot-gate branch; two test-naming fitness violations; and a handful of P2s. 34 new tests added in the fix pass (76 total for the slice). One item is a genuine policy decision rather than a code fix: auto-ingest puts the vaulted (personal-data) observed_path onto an immutable event via ingest_scan's existing RegisterDataset/RegisterDistribution deciders, unconditionally, the moment the switch goes on. Not new to this slice (a human could already do this via the POST route) but now automatic; recorded as open in the design plan for a decision before switch-on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CI's diff-cover put the slice at 87.4% (50 of 398 new lines), below the 90% floor. Every uncovered line was a real branch rather than dead code, so the fix is tests, not a lowered gate: - capture_scan_ingestor_lifespan's whole ENABLED path (the startup grant probe, candidate-lookup selection, spawned sweep task, cancellation on exit) had never run in a test. Only the disabled no-op branch was covered, which means the enabled path would have run for the first time on the deployment. - _sweep_loop's two arms: a tick that raises must be logged and the loop must keep sweeping (the candidate QUERY sits outside tick()'s own guard, so a database blip surfaces there), while cancellation arriving mid-tick must end the loop instead. Swallowing the second as if it were the first would hang shutdown forever. - _ssh_probe's five never-raise arms: a missing locator, a failed ssh launch, a nonzero exit, unparseable stdout, and valid JSON of the wrong shape. These run inside a sweep tick, so an escaping OSError or JSONDecodeError would take the loop down with it. - _remote_scan_probe's _main entrypoint, which is the actual contract with the SSH client (one request line in, exactly one JSON line out, always exit 0). It was previously asserted only by inspection; a stray print or a second line would silently corrupt every response. - The Unrecognized and Unreachable serialization arms, the malformed binding-UUID guard, the no-pool candidate-lookup fallback, and _parse_iso's two undateable-scan arms (2-BM writes start_date as a free-form string, so an aborted scan or a version skew can yield something datetime.fromisoformat refuses). Renamed one new test that tripped the test-name outcome fitness rule: "refuses" is not a recognized outcome verb after an input clause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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
The 2-BM pilot has recorded 133 witnessed Runs and zero Datasets.
ingest_scanis a POST route plus an MCP tool that a human invokes, by design:run/aggregates/run/capture_path.pykeeps the observed path unredacted precisely so an operator can read it to locate the file. That was tenable when scans were supervised one at a time. It is not now, withvertical_lens_series.shrunning 510 unattended scans (~24 GB each, ~2 min apart, ~12 TB per series).CaptureScanIngestorsweeps terminated witnessed Runs whose capture path resolved and issues the existingIngestScancommand for each, attributed to a new pinned agent principal, behind its own kill switch.The bytes do not move
The scan files live on the detector host (
tomdet), not on CORA's. Measured 2026-08-18:A mount falls behind ~2x permanently and saturates the same link the witness uses for EPICS. So the computation moves to the bytes instead: the read and the digest run remotely over SSH and ship back a ~200-byte JSON verdict. This is cheap because tomdet already runs CORA's own venv (python 3.13.12, h5py 3.16.0).
Two round trips (
describe, thenchecksum) are kept deliberately separate. Collapsing them would make the handler's changed-under-read guard agree by construction, and therefore decorative.The locator is untrusted
full_file_namecomes from2bmSP2:HDF1:FullFileName_RBV, writable by anyone with Channel Access. Today CORA only ever opens that string; this slice makes it reach a remote host, so:resolve_confined_file_uri(realpath + commonpath) the local adapters already use, run on whichever host actually holds the bytes;sshargv withBatchMode=yes, a connect timeout with a 1 s floor, and--before the host.A sweep, not a terminal hook
The slice-17 memo said "fires off the witnessed terminal". A ~30 s remote probe inline in
_record_outcomewould stall the single sequential witness loop for 30 s of every 120 s, and missing a BEGUN is the one failure this system cannot afford._run_witness.pyis not touched at all. A sweep is also restart-safe with no queue and no in-memory state, and it backfills runs that already have a resolved path.Gate review
4-agent panel (architecture / test-coverage / cross-BC consistency / security specialist, the last declared for the untrusted-locator-to-SSH surface). 2 P0s, both fixed here:
tick()now tries up to 10 candidates, excluding each one it gives up on, stopping at the first success or the first systemic failure.tracked_python_files()only. Allowlisted with a citation, the escape hatch the test itself names for a query neither BC's own store can express alone.A third boundary violation surfaced only at commit time from
tach-check:cora.apidepending oncora.data.aggregatesto catchIngestScan's own composed errors is the identical blessed-cross-BC-access pattern already granted to five other BCs for the same reason.Also fixed: a PII leak (the vaulted
observed_pathreaching log lines across four call sites); a broken never-raise contract on a malformed remote response; a missing startup authorization probe plus per-tick log spam on a standing denial; untested settings validators and the new boot-gate branch; two test-naming fitness violations; and several P2s. 34 tests added in the fix pass, 76 total for the slice.Risk
Inert on merge.
capture_scan_ingestor_enableddefaults off, and the boot gate refuses to start if it is on withoutcapture_path_recording_enabled.Every green test on the SSH path uses a fake invoker or a loopback; the loopback does exercise real h5py against a real HDF5 file, so the composition is proven, but no byte has yet crossed to tomdet. Live verification against the real host, and the operator's consent to run a read-only probe there, are tracked outside this PR.
Open before switch-on, not before merge
Auto-ingest puts the vaulted
observed_path(personal data: 2-BM's directory layout embeds a surname and proposal number) onto immutableDatasetRegistered.uri/DistributionRegistered.urievents. Not new to this slice, since a human could already do this via the POST route, but now automatic and unconditional. Recorded as a policy decision owed before the switch goes on.🤖 Generated with Claude Code