feat(longitudinal): persist immutable observation evidence - #248
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthrough종단 관찰 PostgreSQL 스키마와 영속성 모듈을 추가했습니다. 관찰 및 멤버십 데이터를 불변 상태로 저장하고, 정확한 중복 재생과 충돌 재생을 구분합니다. 테넌트 범위 조회, 손상 이력 검증, 오류 처리를 구현했습니다. Changes종단 관찰 저장
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This change adds durable PostgreSQL storage for immutable longitudinal observations, but the current database constraints can still persist contract-invalid references and inconsistent clock-anomaly metadata, causing later replay failures or corrupt evidence interpretation. These correctness issues should be fixed or explicitly accepted before merge; documentation and error-message follow-ups remain bounded. Sequence Diagram(s)sequenceDiagram
participant Caller
participant PersistenceAPI
participant PostgreSQL
participant ObservationTable
participant MembershipTable
Caller->>PersistenceAPI: 관찰 레코드 저장
PersistenceAPI->>PostgreSQL: READ COMMITTED 확인
PersistenceAPI->>ObservationTable: 관찰 헤더 INSERT 또는 기존 행 조회
PersistenceAPI->>MembershipTable: 멤버십 공유 INSERT 또는 비교
PostgreSQL-->>PersistenceAPI: 제약 및 재생 결과
PersistenceAPI-->>Caller: Inserted, Duplicate 또는 오류
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Schema-integrity tests dropped one shared schema in parallel, so CREATE TABLE could not resolve longitudinal_reference_is_valid. Give each contract its own schema, serialize with the existing test lock, and keep rustfmt on the membership replay assertion. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Updated the original branch to exact head |
|
@opencode-agent Please review only exact head |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head review recheck at c6b0583: the previously reported longitudinal observation issues are already addressed in the branch. The migration now uses the immutable search_path-pinned Unicode numeric reference validator for every reference column, enforces the clock/anomaly relation in both CHECK and insert-trigger paths, and the Rustdoc/test-harness findings are reconciled. Real PostgreSQL evidence passed: schema-integrity 4/4, persistence 9/9, replay-coverage 8/8. Also passed cargo fmt --check, clippy -D warnings, and rustdoc -D warnings. No additional code change is required for the stale findings; the PR still requires exact-head independent review and terminal protected checks. |
|
Correction to the previous note: the module Rustdoc, |
Preserve immutable longitudinal-observation PostgreSQL persistence on top of protected main 4499d9c. The reconciliation keeps both public modules, retains the longitudinal persistence migration/tests, and carries forward the more tolerant malformed-framing test harness while preserving the request-bound scoring-engine adapter. No gate is weakened and no force push is required.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed the current Devin finding on exact head |
…260818 Resolve CHANGELOG union (keep Active PR #248 bullet plus merged-main bullets, dedupe superseded export-transport copy), keep PR's tenant-scoped longitudinal unique tuple in ERD (matches migrations/0031_longitudinal_observation.sql), modernize stale Active PR #287 TRACEABILITY ref to merged #287 truth.
| CREATE OR REPLACE FUNCTION longitudinal_reference_is_valid(reference_value text) | ||
| RETURNS boolean | ||
| LANGUAGE sql | ||
| IMMUTABLE | ||
| PARALLEL SAFE | ||
| SET search_path = pg_catalog | ||
| AS $longitudinal_reference$ | ||
| WITH reference_character AS ( | ||
| SELECT substr(reference_value, character_index, 1) AS character_text | ||
| FROM generate_series(1, character_length(reference_value)) AS character_index | ||
| ), | ||
| reference_classification AS ( | ||
| SELECT | ||
| character_text, | ||
| ascii(character_text) <@ '{[48,58),[178,180),[185,186),[188,191),[1632,1642),[1776,1786),[1984,1994),[2406,2416),[2534,2544),[2548,2554),[2662,2672),[2790,2800),[2918,2928),[2930,2936),[3046,3059),[3174,3184),[3192,3199),[3302,3312),[3416,3423),[3430,3449),[3558,3568),[3664,3674),[3792,3802),[3872,3892),[4160,4170),[4240,4250),[4969,4989),[5870,5873),[6112,6122),[6128,6138),[6160,6170),[6470,6480),[6608,6619),[6784,6794),[6800,6810),[6992,7002),[7088,7098),[7232,7242),[7248,7258),[8304,8305),[8308,8314),[8320,8330),[8528,8579),[8581,8586),[9312,9372),[9450,9472),[10102,10132),[11517,11518),[12295,12296),[12321,12330),[12344,12347),[12690,12694),[12832,12842),[12872,12880),[12881,12896),[12928,12938),[12977,12992),[42528,42538),[42726,42736),[43056,43062),[43216,43226),[43264,43274),[43472,43482),[43504,43514),[43600,43610),[44016,44026),[65296,65306),[65799,65844),[65856,65913),[65930,65932),[66273,66300),[66336,66340),[66369,66370),[66378,66379),[66513,66518),[66720,66730),[67672,67680),[67705,67712),[67751,67760),[67835,67840),[67862,67868),[68028,68030),[68032,68048),[68050,68096),[68160,68169),[68221,68223),[68253,68256),[68331,68336),[68440,68448),[68472,68480),[68521,68528),[68858,68864),[68912,68922),[68928,68938),[69216,69247),[69405,69415),[69457,69461),[69573,69580),[69714,69744),[69872,69882),[69942,69952),[70096,70106),[70113,70133),[70384,70394),[70736,70746),[70864,70874),[71248,71258),[71360,71370),[71376,71396),[71472,71484),[71904,71923),[72016,72026),[72688,72698),[72784,72813),[73040,73050),[73120,73130),[73184,73194),[73552,73562),[73664,73685),[74752,74863),[90416,90426),[92768,92778),[92864,92874),[93008,93018),[93019,93026),[93552,93562),[93824,93847),[94196,94199),[118000,118010),[119488,119508),[119520,119540),[119648,119673),[120782,120832),[123200,123210),[123632,123642),[124144,124154),[124401,124411),[125127,125136),[125264,125274),[126065,126124),[126125,126128),[126129,126133),[126209,126254),[126255,126270),[127232,127245),[130032,130042)}'::int4multirange | ||
| AS is_numeric | ||
| FROM reference_character | ||
| ) | ||
| SELECT | ||
| reference_value IS NOT NULL | ||
| AND reference_value <> '' | ||
| AND left(reference_value, 1) !~ '[[:space:]]' | ||
| AND right(reference_value, 1) !~ '[[:space:]]' | ||
| AND reference_value !~ '[[:cntrl:]]' | ||
| AND NOT COALESCE( | ||
| bool_or(is_numeric) | ||
| AND bool_and( | ||
| is_numeric | ||
| OR character_text = ANY ( | ||
| ARRAY[ | ||
| '+', | ||
| '-', | ||
| '.', | ||
| ',', | ||
| 'e', | ||
| 'E', | ||
| U&'\066B', | ||
| U&'\066C', | ||
| U&'\FF0E', | ||
| U&'\FF0C' | ||
| ] | ||
| ) | ||
| ), | ||
| FALSE | ||
| ) | ||
| FROM reference_classification; | ||
| $longitudinal_reference$; |
There was a problem hiding this comment.
📝 Info: DB reference validator and Rust normalized_reference have subtle divergences that fail closed
The database longitudinal_reference_is_valid function (0031_longitudinal_observation.sql) reproduces Rust's char::is_numeric boundary via a hardcoded Unicode 17 int4multirange and rejects leading/trailing whitespace and [[:cntrl:]]. Rust's normalized_reference (src/reference.rs:11-35) uses char::is_control, which covers Unicode Cc beyond ASCII, so the two do not match perfectly on exotic control code points. Because persist references (other than tenant) originate from already-validated domain records, and any DB CHECK rejection surfaces as a fail-closed Database error, this divergence does not cause silently accepted bad data. Noting for awareness that the two validators are maintained in parallel and could drift on future Unicode updates.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if rows.len() != 1 { | ||
| return Err(LongitudinalObservationPersistenceError::ConflictingReplay); | ||
| } | ||
| let row = &rows[0]; | ||
| let anomaly_code = clock_anomaly_code(record.clock_anomaly()).map(str::to_owned); | ||
| let exact_header = row.get::<_, String>(0) == record.observation_record_ref() | ||
| && row.get::<_, String>(1) == tenant_ref | ||
| && row.get::<_, String>(2) == record.enrollment_ref() | ||
| && row.get::<_, String>(3) == record.source_system_ref() | ||
| && row.get::<_, String>(4) == record.source_observation_ref() | ||
| && row.get::<_, String>(5) == record.construct_ref() | ||
| && row.get::<_, String>(6) == record.measure_ref() | ||
| && row.get::<_, i64>(7) == postgres_u64(record.validity_start_at_unix_ms())? | ||
| && row.get::<_, i64>(8) == postgres_u64(record.validity_end_at_unix_ms())? | ||
| && row.get::<_, i64>(9) == postgres_u64(record.recorded_at_unix_ms())? | ||
| && row.get::<_, i64>(10) == postgres_u64(record.received_at_unix_ms())? | ||
| && row.get::<_, i64>(11) == postgres_u64(record.ingested_at_unix_ms())? | ||
| && row.get::<_, String>(12) == record.timezone_name() | ||
| && row.get::<_, i16>(13) == record.utc_offset_minutes() | ||
| && row.get::<_, Option<String>>(14) == anomaly_code; | ||
| if !exact_header { | ||
| return Err(LongitudinalObservationPersistenceError::ConflictingReplay); | ||
| } |
There was a problem hiding this comment.
📝 Info: classify_existing OR-query fails closed on ambiguous or cross-tenant matches
The replay classifier selects rows matching either the global observation record ref OR the tenant-scoped source identity (postgres_longitudinal_observation.rs) and returns ConflictingReplay unless exactly one row matches and every immutable header field (including tenant) is identical. I verified the column indices (0-14) line up with the SELECT list, that the source-identity clause is tenant-scoped so the same source tuple in a different tenant does not alias, and that a record-ref match with a differing tenant yields ConflictingReplay. The logic is sound and fail-closed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| -- A membership-only trigger cannot observe an observation whose membership vector is empty, | ||
| -- because no child INSERT exists to fire it. Defer the same invariant from the parent INSERT so | ||
| -- the complete vector can be inserted in the transaction while a header-only commit still fails. | ||
| DROP TRIGGER IF EXISTS longitudinal_observation_membership_total_check ON longitudinal_observation; | ||
| CREATE CONSTRAINT TRIGGER longitudinal_observation_membership_total_check | ||
| AFTER INSERT ON longitudinal_observation | ||
| DEFERRABLE INITIALLY DEFERRED | ||
| FOR EACH ROW EXECUTE FUNCTION enforce_longitudinal_membership_total(); | ||
|
|
||
| DROP TRIGGER IF EXISTS longitudinal_membership_total_check ON longitudinal_membership_share; | ||
| CREATE CONSTRAINT TRIGGER longitudinal_membership_total_check | ||
| AFTER INSERT ON longitudinal_membership_share | ||
| DEFERRABLE INITIALLY DEFERRED | ||
| FOR EACH ROW EXECUTE FUNCTION enforce_longitudinal_membership_total(); No newline at end of file |
There was a problem hiding this comment.
📝 Info: Deferred membership-total trigger on the parent header closes the empty-vector gap
The membership-sum invariant is enforced by a CONSTRAINT TRIGGER (INITIALLY DEFERRED) attached to BOTH longitudinal_observation (AFTER INSERT) and longitudinal_membership_share (AFTER INSERT) (0031_longitudinal_observation.sql). Attaching it to the parent is what makes a header-only commit (no membership rows) fail at COMMIT rather than silently persist, since a child-only trigger could never fire for an empty vector. The duplicate-replay path returns before inserting the header, so no trigger is queued for idempotent replays. Behavior matches the schema-integrity test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if inserted == 0 { | ||
| return classify_existing(transaction, tenant_ref, record); | ||
| } |
There was a problem hiding this comment.
📝 Info: READ COMMITTED requirement is load-bearing for replay classification
persist_longitudinal_observation inserts with ON CONFLICT DO NOTHING and, when inserted == 0, calls classify_existing to distinguish an exact duplicate from a conflicting replay (postgres_longitudinal_observation.rs). This is only correct because require_read_committed (postgres_longitudinal_observation.rs) rejects stricter isolation: under READ COMMITTED, ON CONFLICT DO NOTHING blocks on a concurrent uncommitted conflicting insert and, after it commits, the subsequent SELECT takes a fresh snapshot and sees the winner. Under SERIALIZABLE/REPEATABLE READ the classify SELECT would use the transaction-start snapshot and could miss the committed winner, so rejecting those levels is deliberate rather than a bug. Worth confirming the calling code always runs this in READ COMMITTED transactions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| CREATE OR REPLACE FUNCTION enforce_longitudinal_membership_total() | ||
| RETURNS trigger | ||
| LANGUAGE plpgsql | ||
| AS $$ | ||
| DECLARE | ||
| total_weight bigint; | ||
| BEGIN | ||
| SELECT COALESCE(SUM(weight_parts_per_10_000), 0) | ||
| INTO total_weight | ||
| FROM longitudinal_membership_share | ||
| WHERE observation_record_ref = NEW.observation_record_ref; | ||
| IF total_weight <> 10000 THEN | ||
| RAISE EXCEPTION 'longitudinal membership shares must sum to 10000' | ||
| USING ERRCODE = '23514'; | ||
| END IF; | ||
| RETURN NULL; | ||
| END; | ||
| $$; | ||
|
|
||
| -- A membership-only trigger cannot observe an observation whose membership vector is empty, | ||
| -- because no child INSERT exists to fire it. Defer the same invariant from the parent INSERT so | ||
| -- the complete vector can be inserted in the transaction while a header-only commit still fails. | ||
| DROP TRIGGER IF EXISTS longitudinal_observation_membership_total_check ON longitudinal_observation; | ||
| CREATE CONSTRAINT TRIGGER longitudinal_observation_membership_total_check | ||
| AFTER INSERT ON longitudinal_observation | ||
| DEFERRABLE INITIALLY DEFERRED | ||
| FOR EACH ROW EXECUTE FUNCTION enforce_longitudinal_membership_total(); | ||
|
|
||
| DROP TRIGGER IF EXISTS longitudinal_membership_total_check ON longitudinal_membership_share; | ||
| CREATE CONSTRAINT TRIGGER longitudinal_membership_total_check | ||
| AFTER INSERT ON longitudinal_membership_share | ||
| DEFERRABLE INITIALLY DEFERRED | ||
| FOR EACH ROW EXECUTE FUNCTION enforce_longitudinal_membership_total(); No newline at end of file |
There was a problem hiding this comment.
📝 Info: Deferred membership-total invariant depends on trigger being enabled
The 10,000 basis-point total is enforced only by the DEFERRABLE INITIALLY DEFERRED constraint triggers (0031_longitudinal_observation.sql), fired from both the parent INSERT and each child INSERT. There is no CHECK-level backstop for this aggregate (unlike immutability and anomaly, which have both a CHECK and a trigger). If an operator disables these triggers (as the corrupt-history test does at postgres_longitudinal_observation_persistence.rs), header-only or unbalanced rows can be committed. This is by design for the restore/repair scenario and the loader fails closed via CorruptHistory, but it is worth noting the total invariant is trigger-only, not constraint-backed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| - Exact source identity is unique on `(tenant_ref, enrollment_ref, source_system_ref, source_observation_ref)`. Exact replay is idempotent; source rebinding or stored-evidence mutation fails closed. The same source tuple may repeat under a different tenant only with a different observation-record identity. UPDATE, DELETE, and TRUNCATE guards preserve the logical ERD's immutable-evidence semantics. | ||
| - Real PostgreSQL tests exercise persistence/reload, tenant isolation, replay/conflict classification, source-identity rebinding rejection, schema integrity, membership totals, immutability, and missing-schema failure. No direct Gyeot or TEPP database access is introduced. | ||
|
|
||
| This is an explicit logical-to-physical reconciliation: the physical split preserves one logical observation record with zero-or-more explicit membership-share evidence rows; it does not create a second collection or analysis kernel. Durable longitudinal enrollment, HTTP transport, live Gyeot/TEPP adapters, recovery acceptance, and research-release registration remain Target. |
There was a problem hiding this comment.
📝 Info: AS_BUILT prose describes membership rows as 'zero-or-more' but the schema requires at least one
AS_BUILT_SCHEMA.md describes the physical split as preserving "one logical observation record with zero-or-more explicit membership-share evidence rows." The enforced invariant actually requires the shares to total exactly 10,000, i.e. at least one row; a header-only commit fails (verified by postgres_longitudinal_observation_schema_integrity.rs). The 'zero-or-more' phrasing understates the enforced cardinality. Minor documentation imprecision rather than a functional defect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| CONSTRAINT longitudinal_observation_anomaly_check CHECK ( | ||
| (clock_anomaly_code IS NULL AND recorded_at_unix_ms <= received_at_unix_ms) | ||
| OR ( | ||
| clock_anomaly_code = 'recorded_after_received' | ||
| AND recorded_at_unix_ms > received_at_unix_ms | ||
| ) | ||
| ) |
There was a problem hiding this comment.
📝 Info: Anomaly-code mapping stays consistent with domain flagging and CHECK constraint
The domain sets clock_anomaly = RecordedAfterReceived iff recorded_at > received_at (src/longitudinal_observation.rs:374-375), clock_anomaly_code maps that one-to-one (postgres_longitudinal_observation.rs), and the SQL longitudinal_observation_anomaly_check enforces exactly (code IS NULL AND recorded <= received) OR (code='recorded_after_received' AND recorded > received). These three definitions agree for the boundary case recorded == received (no anomaly), so persisted records never violate the constraint and require_clock_anomaly_code on load stays consistent. No defect found.
Was this helpful? React with 👍 or 👎 to provide feedback.
Resolved conflicts: - CHANGELOG.md: union Added bullets; Active PR #248/#287 rewritten as merged - docs/TRACEABILITY.md: main's module/migration truth plus #224 reload row; fixed mangled tree annotation - docs/adr/0015: unioned references with consistent APA lettering - AS_BUILT_SCHEMA/ERD/UML: main's merged truth (#58/#77/#218/#232/#264) plus Active PR #224 reload evidence - src/postgres_item_delivery.rs: kept exact_reference/stored_sequence/reconstruct_error helpers; unioned doc contract - tests: restored concurrency-test imports
Resolved conflicts: - src/lib.rs: alphabetical union of new modules (account_link_write, anonymous_*, postgres_longitudinal_observation, postgres_participant_identity_link) - .gitignore: kept /.netlify entry - CHANGELOG: unioned Added bullets; merged #248/#287 labels modernized - TRACEABILITY: main's merged-truth paragraphs plus Active PR #206 row with stale predecessor lanes noted as superseded - ERD: PR #206's updated participant_identity_link bullet retained
Resolved conflicts: - CHANGELOG: unioned Added bullets; merged #248/#287 labels modernized - TRACEABILITY: main's merged-truth paragraphs/tree plus Active PR #194 reload rows; stale #168 preference dropped - ERD: main's data-rights (#77 modernized to merged) and assessment_session bullets plus Active PR #194 scoring_request reload bullet - doctoring/standards-and-evidence: main's APA-lettered PostgreSQL references (2026a SELECT, 2026b Transaction isolation); in-text citation relettered
Resolved conflicts: - Renumbered 0020_item_delivery_instrument_version.sql to 0032 (0020 reserved by open PR #284 response-event slice); all references updated - src docs: unioned instrument-version and canonical-spelling contract sentences - CHANGELOG: unioned bullets; merged #248/#287 labels modernized - TRACEABILITY/AS_BUILT_SCHEMA/ERD: main merged truth plus Active PR rows for the instrument-version persistence slice
2 participants
Why
Protected
mainnow contains the normalized longitudinal observation contract from #235, including validity/source/platform clocks, civil-time evidence, source identity, and explicit multiple-membership shares. That evidence is still in-memory only after process restart. This slice adds product-owned PostgreSQL durability without moving Gyeot collection or TEPP temporal/multiple-membership analysis into Psychometrics Commons.TDD
RED first:
e36af18bc5a768372bd9d3f4c708b791447ea180adds a real PostgreSQL contract for a Seoul observation with two membership shares, exact replay, source-identity rebinding rejection, database immutability, isolation, and missing-schema failure.GREEN implementation adds:
migrations/0031_longitudinal_observation.sqlwith immutablelongitudinal_observationandlongitudinal_membership_shareevidence;(enrollment_ref, source_system_ref, source_observation_ref);55000;src/postgres_longitudinal_observation.rswithREAD COMMITTED, exact replay classification, numeric-range checks, and fail-closed conflicting replay.The branch started from exact protected main
46142cdbbe5dd5e900a926b70c700adf1878088aafter #235 merged.Verification
Intended exact-head evidence:
cargo test --test postgres_longitudinal_observation_persistencecargo test --lib postgres_longitudinal_observationThis execution environment does not expose a Rust/PostgreSQL toolchain, so GitHub exact-head CI is the executable acceptance boundary; no local pass is claimed.
Scope / ownership
Psychometrics Commons persists consented normalized ingestion evidence only. Gyeot remains collection owner. TEPP remains temporal/event/multilevel/multiple-membership analysis owner. No scientific kernel is copied and there is no cross-service database access.
Out of scope
Do not merge until the unchanged exact head satisfies every live required check, zero valid unresolved findings, and qualifying independent non-author last-push approval. Never self-approve.
Summary by CodeRabbit
새로운 기능
오류 처리 및 안정성
테스트