Slice 14a: give a witnessed Run its experiment identity - #667
Merged
Conversation
Adds Settings.capture_experiment_identity_pvs (code -> closed role -> PV, for proposal_number/esaf_number/esaf_doi) and a sixth, independent kill switch, capture_experiment_identity_recording_enabled, gated at boot on run_witness_recording_enabled exactly like the baseline and capture-path switches before it. A public-resolvability check (DataCite search + the upstream dmagic/APS-DM-SDK source) found ESAFDOINumber is populated from an internal, authenticated APS API with no public DOI-registry record, so it is not confirmed as a genuinely resolvable DOI. All three PVs therefore vault together; none rides an event, so this slice adds no event field, no disposition, and needs no schema gate review. No behavior change yet: the PVs are declared and the switch exists, but nothing reads or writes through them until the vault module and reader land in the next commits.
A sibling table to run_capture_path, not a generalization of it: a capture path and a proposal/ESAF number are different kinds of fact, even though both share the PII-vault-shaped posture (mutable side table keyed on run_id, RLS+FORCE, never referenced from an event). Each of proposal_number/esaf_number/esaf_doi is independently nullable with its own *_observed_at (the substrate's own reading time), because a deployment may configure fewer than three roles, or one PV may read a real value while a sibling still reads "Unknown". No tombstone placeholder on the read side (unlike observed_capture_path): none of these three values is personal data, so a plain None is already honest. Bumps EXPECTED_SCHEMA_VERSION to this migration. Still inert: nothing writes to this table until the reader lands in the next commit.
Mirrors CaptureBaselineReader's shape (one read-every-role-and-write- once call, never raises, per-role failure independence) but simpler: writes directly to the run_experiment_identity vault, not through AppendObservations, so it needs no principal and no authz call. resolved_identity_text is the single place Trap 1 (the substrate's "Unknown" placeholder, and an empty string, must read as absent, not as a plausible value) is decided; capture_watch_preflight will import it directly in the next commit so its own verdict can never drift. Trap 2 (these PVs persist across beamtimes with no in-band staleness signal) is not solved here -- it can't be, per the design memo -- but each accepted value is paired with its own substrate produced_at so a reader downstream can see how old it is. Still unwired: nothing calls this reader yet.
…_run RunWitnessRecorder._promote now calls _read_experiment_identity right after _read_baseline, same defense-in-depth wrapper, gated on both a reader being configured (main.py wires one whenever capture_experiment_identity_pvs is declared) and the sixth kill switch. run_witness_lifespan gains capture_experiment_identity_pvs + experiment_identity_store params and builds the reader exactly like CaptureBaselineReader (a distinct reader object, since it does its own ControlPort reads, unlike capture_path_store's handed-straight-through style). get_run.bind now also takes experiment_identity_store; RunView, the REST route, and the MCP tool each grow six fields (proposal_number, esaf_number, esaf_doi, each paired with its own *_observed_at). No tombstone placeholder here, unlike observed_capture_path: none of these three values is personal data, so a plain None is already honest, and capture_code already tells a Conducted Run apart from a Witnessed Run with nothing recorded yet. Regenerates the OpenAPI snapshot for the six new RunResponse fields.
The single highest-value thing this preflight can say about the proposal/ESAF/ESAF-DOI PVs: the substrate's own "Unknown" placeholder must never look like a healthy, generic string. The identity sweep's verdict now distinguishes unknown / empty / text(len=N), all OK, from non-text, the only BAD outcome -- dispatched through resolved_identity_text (exported alongside ABSENT_IDENTITY_LITERAL from the reader module) so the preflight's verdict can never drift from what ExperimentIdentityReader actually vaults. None of the three roles is personal data, so the raw value prints unredacted; the same defensive path-shape redaction as the baseline sweep still guards against a full_file_name PV being misdeclared here.
proposal_number was already pre-emptively denylisted in slice 13. Adding the other two makes this fitness test the mechanism that enforces the vault-not-event decision against a future agent who reaches for RunStarted's payload, even though none of the three fields is personal data: the decision rests on the auto-harvest / no-operator-gesture asymmetry, not on PII specifically.
- ExperimentIdentityReader -> CaptureExperimentIdentityReader: the class dropped the Capture prefix its own file, log events, and Settings key all carry (R2, matching CaptureBaselineReader's file-to-class correspondence). - esaf_doi -> esaf_doi_number (column, dataclass field, ROLE_ESAF_DOI -> ROLE_ESAF_DOI_NUMBER): restores the shared <qualifier>_number skeleton the other two fields use, and stops implying a claimed scheme (CORA's PersistentIdentifier already reserves DOI as a resolvable-identifier scheme, which this slice explicitly did not confirm ESAFDOINumber to be). - Bare "identity" truncated the family noun on the reader's public surface (identity_pvs, resolved_identity_text, the preflight's group="identity" and its verdict/report-key wiring) and collided with cora.shared.identity's own vocabulary (ActorId, principal identity). Widened to experiment_identity throughout. - ABSENT_IDENTITY_LITERAL -> UNKNOWN_EXPERIMENT_IDENTITY_LITERAL: the old name asserted "absent" for a constant whose value is the string "Unknown"; the new name agrees with its own value, mirroring UNOBSERVED_CAPTURE_PATH's name-matches-value discipline. - Preflight verdict "unknown" -> "placeholder": the report's own status role already uses "unrecognized" for a different failure (CORA's decoder rejected the literal, vs. the substrate never populating it); "unknown" would have read as a synonym. Mechanical rename; no behavior change. Regenerates the OpenAPI snapshot and the migration checksum for the column rename.
- RunWitnessRecorder._promote awaited _read_baseline then _read_experiment_identity sequentially, doubling the single-consumer loop's stall time on a promotion when both switches are on, even though every module in this ladder stresses that this path "must not block the loop". Both readers already catch every failure internally, so a plain asyncio.gather is safe. - get_run's handler paid the same sequential cost on capture_path_store and experiment_identity_store; same fix. - resolved_experiment_identity_text matched the substrate's "Unknown" placeholder by exact case. The exact casing is an observed IOC default, not a wire-level guarantee; match case-insensitively so a differently-cased variant still reads as "not populated" rather than as a real value. - Simplified RunView's six repeated "experiment_identity is not None else None" ternaries by hoisting the guard to the concurrent-read site instead of the composition.
The full-suite run (not scoped to slice-14a-touched files) caught what
my targeted test runs couldn't: an exact dict-equality assertion on
GET /runs/{run_id}'s response body, which the six new fields now fail
by being present. Adds them at None, mirroring the existing
capture_code/observed_capture_path comment: a Conducted Run never
touches the run_experiment_identity vault either.
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
Gives a witnessed Run a proposal / ESAF / ESAF-DOI experiment identity, so an operator can tell which experiment a recorded scan belongs to, without auto-harvesting a re-identifying fact onto an immutable event.
RecordWitnessedRunhas no operator behind it the waystart_run.external_refsdoes, so writing these PVs ontoRunStartedwould be CORA auto-harvesting off an unauthenticated channel with no operator gesture, and events are immutable/INSERT-only.run_experiment_identitymirrors slice 13'srun_capture_pathshape (a sibling table, not a generalization: a capture path and a proposal/ESAF number are different kinds of fact).dmagic/APS-DM-SDK source:ESAFDOINumberis populated viaEsafApsDbApi.getStationEsafById, an internal authenticated APS API, not a DOI registration agency; a DataCite search returned zero APS ESAF records. Unconfirmed as a genuinely resolvable public identifier, so it vaults alongside the other two — no event-schema change, no disposition entry, no naming review on a scheme literal.resolved_experiment_identity_text: the substrate's own"Unknown"placeholder (matched case-insensitively) and an empty string both read as ABSENT, never as a plausible value; each accepted field is paired with its own substrateproduced_atso staleness is visible rather than guessed at (these PVs persist across beamtimes with no in-band freshness signal — a staff question, not a code question).capture_experiment_identity_recording_enabled), gated at boot onrun_witness_recording_enabledexactly like the five before it.capture_watch_preflightextended with a third sweep whose verdict distinguishesplaceholder/empty/text(len=N), so an unpopulated PV is visible before the recording switch flips rather than reading as a healthy value.test_run_events_carry_no_pii.pydeny-list withesaf_number/esaf_doi_number(proposal_numberwas already there), making that fitness test the mechanism enforcing this decision against a future agent who reaches for the event payload.ExperimentIdentityReader→CaptureExperimentIdentityReader,esaf_doi→esaf_doi_number, widened bareidentity→experiment_identitythroughout (collided withcora.shared.identityvocabulary),ABSENT_IDENTITY_LITERAL→UNKNOWN_EXPERIMENT_IDENTITY_LITERAL, preflight verdictunknown→placeholder.RunWitnessRecorder._promote's baseline+identity reads,get_run's two vault lookups) and the case-sensitivity gap above.Slice 14b (
User*PVs) stays blocked on D0's research, per the task; not touched.Test plan
make lint— cleanmake typecheck— cleanmake test-noio— 47998 passed, 630 skippedmake test-db— 1210 passed, 1 skippedget_runsurfacesecurity-reviewskill mis-scoped its own diff against a stale base pulling in ~14 unrelated slices; redid it by hand against the realhttps-origin/main..HEADdiff — no findings)🤖 Generated with Claude Code