Record exporter: registry through redaction, plus the security re-review - #649
Merged
Conversation
The record exporter must not import a bounded context: it lives at cora.infrastructure so that tach's `depends_on = ["cora.shared"]` ENFORCES the zero-import property its standalone-verifiability claim rests on. But deciding whether a field's annotation is a StrEnum, a str alias, or a value object needs the defining module imported. Resolving at runtime is therefore not available to the code that needs the answer. So resolve at build time and commit the answer as inert data, the way `make openapi-snapshot` already does. The generator lives outside src/ on purpose: nothing shippable can import it, so the exporter cannot quietly acquire the dependency the design forbids. Resolving real type objects rather than annotation strings is what makes this tractable. An AST census left 88 unclassifiable fields across 48 annotation names; runtime resolution collapses plain aliases for free and leaves 2, both genuine modelling questions rather than gaps. The generator ABORTS on anything it cannot classify, because an unrecognised type is a question about the model, not a row to skip. The default is drop. A new event with a bare `str` field is excluded with nobody editing a list, which is the whole fail-closed property; the committed diff is the list of what a published record would disclose, and the drift test fails until someone regenerates and reads it. Two predictions from the design memo came out of the artifact rather than out of argument: FacilityCode resolves to a value object and drops, and LogbookOpened.kind drops because it is a bare `str` with no enum, which is the pressure to type it properly that F5 said it would be. One correction to the review record: PresenceMode and VisitType are NOT published. Both are declared `str` with the enum named only in a comment, so both drop. The round-4 threat model listed them as kept. Its conclusion is unchanged, since full-precision timestamps and ActorKind still compose with a public beamtime schedule, but the premise was wrong and the reviews memo says so now. Refs: project_record_export_v3.md F5, project_record_export_build_brief.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The exporter walks the stream and resolves each *LogbookOpened envelope's kind through one registry to the entries table it opened, per project_record_export_v3.md F0/F2. Eight entries, not six: six resolve a kind from an envelope; entries_run_feed_heartbeats and entries_enclosure_permit_probes have no envelope and are declared here explicitly, since a table with no envelope stays invisible to envelope-driven traversal until something declares it. Order key lives per kind because sampled_at exists on only four of the eight tables. An unknown kind raises rather than returning None or being skipped, and both the fitness test (every *LogbookOpened class under src/cora is named by the registry, and vice versa) and the information_schema integration test (renames defeat AST enumeration; three of these tables have already been renamed) exist to keep the registry from rotting quietly the way the frame memo's own census did. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
export_record captures one pg_snapshot_xmin watermark up front (not re-evaluated per row like the projection worker's catch-up query, so one export sees one consistent snapshot), walks the whole events table under it in (transaction_id, position) order, and on each row whose event_type names one of Step 1's six *LogbookOpened classes, pulls kind/logbook_id straight out of the already-decoded payload and follows it through Step 1's registry into the matching entries rows. Payload UUID/datetime fields are already plain strings by the time they reach jsonb (every to_payload() converts them before the row is written, confirmed against Step 0's own generated disposition table), so F6 rendering only has to touch the outer typed columns asyncpg hands back as uuid.UUID / tz-aware datetime / bytes -- not recurse into payload/metadata. An unknown stream_type refuses immediately rather than being skipped, against a closed set collected from the 42 distinct _STREAM_TYPE literals scattered across the tree (no such registry existed before; Step 2's own acceptance criteria required one). Zero exported rows is an error, not an empty result: a freshly migrated database is not actually empty, since two migrations seed bootstrap events, so the zero-rows test empties events explicitly on its own disposable per-test database rather than relying on that lucky-empty state. Heartbeats and probes are deliberately not walked here: neither has an envelope event to trigger from, and that is written down as an explicit deferral rather than an omission, per the build brief's open question about excluding them as operational telemetry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…usions hash_record covers both tiers of an ExportedRecord in one compute_content_hash call (SHA-256 over DSSE-PAE-wrapped, NFC-normalized, sorted-key JSON) with no exclusions, per F2: re-exporting an unchanged database reproduces the same hash, and any single differing byte in either tier changes it. hash_streams/hash_logbooks hash each tier alone, each pinned to its own payload type from the design's Naming section so identical bodies under different types never collide. Deliberately reuses content_hash's existing canonicalization rather than canonical_json, which is a different recipe: the committed file must be the bytes the hash covers, and cora.infrastructure's tach dependency on cora.shared is exactly what permits this import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
build_manifest is pure: git_commit and watermark are captured by the caller (capture_git_commit, step 2's capture_watermark) and passed in, so the function itself does no I/O. Carries the two profile hashes namable before step 6 exists -- record_hash (H1, step 3) and redaction_profile_hash (H2, step 0's disposition table, computed for the first time here since nothing needed it before) -- plus row_count_by_logbook_kind and max_schema_version_by_event_type as reader-facing sanity checks a reader can recompute and compare against. is_simulated rolls up every observation row's own flag, vacuously True when an export carries none. expansion_digest_presence_by_run answers F8 per run: a run has a pinned digest iff at least one child Procedure was registered via register_procedure_from_recipe (carries RecipeExpansionRecorded on its own stream), derived by cross-referencing ProcedureRegistered.parent_run_id against RecipeExpansionRecorded rows rather than assuming anything about how a run's Procedures were made. Naming-r3 review renamed four names before commit: xmin_watermark -> watermark (parity with capture_watermark), logbook_row_counts -> row_count_by_logbook_kind and has_expansion_digest_by_run -> expansion_digest_presence_by_run (both were dict fields wearing a scalar's naming shape), _str -> _require_str (matches the repo's _require_* assert-and-return convention). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
verify_record_hash.py reimplements compute_content_hash's canonicalization (NFC-normalized, sorted-key JSON, DSSE-PAE wrap, SHA-256) using only the stdlib, at repo-root scripts/ rather than inside apps/api: the whole point is running on a machine that has never installed CORA, and apps/api's ruff/pyright/tach coverage never reaches that directory today, which is the correct trade for a file with zero third-party dependencies by design. Deliberately narrower than cora.shared.content_hash: skips the dataclass/Pydantic/set branches, since an exported record body is already reduced to JSON primitives by the time it reaches this tool (render_row in step 2). A test cross-checks the two implementations byte-for-byte over a corpus including composed/decomposed NFC forms and floats, loading this script via the same importlib bridge tests/unit/deployments/test_beamline_descriptor.py already uses for scripts/ modules. A separate genuine-subprocess test proves a flipped byte in the input file changes the verdict from OK to MISMATCH with a nonzero exit. Naming-r3 review renamed the check subcommand to verify (matching the script's own name and docstring), --expected to --expected-hash (parity with its sibling --payload-type), and json_file to body_file (every other symbol in the file calls this the body). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
redact_record is the fail-closed switch: it computes hash_redaction_profile() itself and compares against a caller-supplied expected hash before touching a single row, refusing under a stale or substituted disposition table rather than redacting silently under the wrong rules. Tier 1 iterates the STORED payload's own keys against Step 0's generated DISPOSITIONS table (never the profile's keys, which is the whole fail-closed property), corrected against an internal wording conflict in the design docs: an event_type entirely absent from the table aborts (the table is stale relative to the code), but a known event type's unlisted field drops (the graceful path for schema evolution, and what "a bare str on a new event drops with nobody editing a list" actually describes). position/version re-index densely; transaction_id maps to a dense monotone int; the five UUID columns token through one shared TokenMap so tier-1/tier-2 joins survive within an export without the surrogate being derivable from its source (never a hash). Tier 2 is hand-transcribed from F5's three text-column tables (28 columns/8 kinds) plus the five jsonb columns (four recursing through a shared generic leaf-rule walker, one dropped whole), since F5's own argument for not generating this table is that 103 columns is small enough to enumerate by hand. The recursion walker never branches on which step_kind or Recipe step type produced a value, which is what kept a real drift (ProcedureActivity.step_kind grew from 3 to 5 values since the design was written, and two unenumerated step kinds write payload shapes the design never saw) from being a correctness problem: unenumerated fields already drop by the same default every other unlisted field drops by. Also built: an unfired-clearance check, catching a declared jsonb string-leaf clearance that never matched a row in a kind present in the export -- most useful as a completeness signal against a realistic fixture (a narrow one, like a single-step-kind rehearsal activity, legitimately fails it, which the redaction acceptance test's fixture was widened to avoid rather than the check being loosened). Naming-r3 review renamed six names before commit: reveal_map -> surrogate_by_source, looks_like_uuid -> is_uuid_shaped, DROPPED -> OMITTED (collided with tier-2's own DROP disposition constant), RECURSE -> BY_VALUE (tier-1's locked, generated vocabulary already calls this dispatch by-value), TIER2_JSONB_DROP_WHOLE -> TIER2_JSONB_DROPPED_COLUMNS, RedactionResult.redacted -> redacted_record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Step 7's single security-lens re-review (project_record_export_v3.md F5's "How to apply" item 7) found hash_redaction_profile() hashing only Step 0's generated tier-1 DISPOSITIONS table. Step 6's hand- authored tier-2 tables (TIER2_DISPOSITIONS, TIER2_JSONB_CLEARED_POINTERS, TIER2_JSONB_DROPPED_COLUMNS) were never part of H2, so redact_record's fail-closed switch could not detect a tier-2 disposition weakened after the fact (e.g. conduit_verdicts.reason DROP -> KEEP): expected_redaction_profile_hash would still match. Widen the hash body to cover all four tables, flattening the two tuple-keyed dicts to "kind/column" string keys for a reviewable canonical form. Regression tests pin that each of the four tables actually moves the hash.
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
project_record_export_v3.md(F0-F8): the kind->table registry, the exporter walking events and following logbook envelopes into entries tables, content hashing (no exclusions), the export manifest, a standalone zero-cora-import verifier, and redaction (disposition-driven, fail-closed, per F5).stron their events and correctly drop).Test plan
make lint,make typecheck,make arch-check(30496 passed, 621 skipped),tach checkall greenrecord_export/covers registry completeness, export ordering, hashing stability/sensitivity, manifest assembly, the standalone verifier byte-for-byte cross-check, and redaction (tier 1 + tier 2 + the fail-closed switch, including the new H2 tier-2 sensitivity tests)