Skip to content

fix(longitudinal): reject reused Commons observation identities - #262

Closed
seonghobae wants to merge 8 commits into
mainfrom
fix/longitudinal-record-identity-20260820
Closed

seonghobae wants to merge 8 commits into
mainfrom
fix/longitudinal-record-identity-20260820

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

Why

Protected main treats observation_record_ref as the opaque Commons identity of one immutable longitudinal observation, but the in-memory LongitudinalObservationSet only enforces uniqueness of the Gyeot source tuple (enrollment_ref, source_system_ref, source_observation_ref). Two distinct source observations can therefore reuse the same Commons record identity and both be accepted. That contradicts the identity contract and the PostgreSQL persistence slice in #248, where observation_record_ref is a unique persisted record identity.

What

  • Reject a distinct source observation that attempts to reuse an already accepted observation_record_ref.
  • Preserve exact source replay semantics: an exact replay still returns the first immutable record, while a same-source/different-evidence replay remains IdempotencyConflict.
  • Add a distinct ObservationIdentityConflict error so operators can distinguish a Commons identity collision from a source idempotency conflict.
  • Keep Gyeot collection and TEPP analysis ownership unchanged; no persistence, HTTP, scoring, or psychometric kernel change is introduced.

TDD lineage

  • RED 2fc3a36b779895d1be2e972bcfa27b44f1964520 adds a contract that references the missing identity-conflict behavior and therefore cannot compile on the protected-main implementation.
  • GREEN a2fe87dad1ffa429d79aa9ebb5c488688693ea28 adds the narrow domain guard and operator-facing error.

Compatibility with #248

#248 does not modify src/longitudinal_observation.rs or this focused contract file. Its PostgreSQL schema already treats observation_record_ref as the persisted observation identity. This PR closes the in-memory/domain mismatch without copying persistence or TEPP/Gyeot responsibilities.

Required evidence before merge

Do not merge until the unchanged exact head passes live Runtime CI, exact line/branch coverage, rustfmt/clippy/rustdoc, security/SAST/SBOM/provenance, zero valid unresolved findings, and qualifying independent non-author review. Never self-approve.

Summary by CodeRabbit

  • 버그 수정
    • 서로 다른 원천 관찰이 동일한 관찰 레코드 식별자를 재사용하면 저장을 거부하도록 검증을 강화했습니다.
    • 충돌 발생 시 명확한 오류가 표시되며, 유효하지 않은 관찰은 데이터 집합에 추가되지 않습니다.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

LongitudinalObservationSet::ingest가 서로 다른 source observation의 중복 observation_record_ref를 저장 전에 거부합니다. 새 ObservationIdentityConflict 오류와 설명을 추가하고, 오류 및 집합 크기를 검증하는 회귀 테스트를 추가했습니다.

Changes

관측 레코드 식별자 충돌

Layer / File(s) Summary
식별자 충돌 오류 계약
src/longitudinal_observation.rs
LongitudinalObservationError::ObservationIdentityConflict variant와 오류 메시지를 추가했습니다. ingest 문서에 고유한 Commons 관측 레코드 식별자 요구사항을 반영했습니다.
수집 중복 검증 및 회귀 테스트
src/longitudinal_observation.rs, tests/longitudinal_observation_record_identity.rs
ingest가 기존 observation_record_ref와 중복되면 오류를 반환하고 저장하지 않습니다. 테스트가 두 번째 수집의 오류 메시지와 집합 크기를 검증합니다.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to a2fe8

This PR adds a localized validation for reused observation identities while preserving replay behavior. Only minor test-strengthening and documentation follow-up remain, with no actionable merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 재사용된 Commons 관측 식별자를 거부하는 주요 변경을 정확하고 간결하게 설명합니다.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/longitudinal-record-identity-20260820

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Fixed the current-head Runtime CI formatting failure with a minimal formatting-only commit.

  • Previous head: d759ecec80614509888237d7782f493dbdcff3da
  • New exact head: 64d88c70
  • Cause: cargo fmt --all -- --check rejected a wrapped assertion and missing final newline in tests/longitudinal_observation_record_identity.rs.
  • Local evidence: cargo fmt --all -- --check passed; cargo test -q --test longitudinal_observation_record_identity passed (1 test).

This is validation evidence, not an approval. The required remote checks must complete for the new exact head.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Additional local validation on exact code after the formatting commit: cargo clippy --all-targets -- -D warnings passed. The remote required checks remain the authoritative gate for head 64d88c70.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please review only the current exact head 64d88c7 against protected main 5544149. The formatting-only Runtime CI failure was corrected at exact head 64d88c7; please review only this head against protected main 5544149. Do not transfer conclusions from the superseded d759ece head.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please review only exact head 31df209059ca1be7b93fb869025fc1c8b139fefb against protected main 5544149c. The current head preserves the accepted record after rejecting a distinct source observation that reuses its Commons identity, and includes the traceability mapping. Use only same-head checks and code.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Reconciled exact head 64d88c7079459490f979cb60f8b8ec569ecf510a to 31df209059ca1be7b93fb869025fc1c8b139fefb without force-pushing.

  • Added the traceability mapping for the Commons observation_record_ref uniqueness invariant; enrollment persistence, PostgreSQL, HTTP, Gyeot, and TEPP integration remain Target.
  • Fresh validation on the new head: cargo fmt --all -- --check, git diff --check, documentation contracts (10 + 1), identity regression (1), longitudinal time contract (10), and cargo clippy --all-targets -- -D warnings passed.

This is validation evidence, not approval.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please review only exact head 31df209059ca1be7b93fb869025fc1c8b139fefb against protected base 5544149ca5dc55d2bfc3402cc59c03c44830de5f. Validate the longitudinal observation identity correction, temporal/multiple-membership preservation, tenant and consent boundaries, and exact replay behavior. This request is not an approval.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Current-base correction: protected main is exact head 503a4e6. Please review only PR #262 head 31df209 against that current protected main; the PR metadata may still show the older ancestor base 5544149. Do not transfer conclusions from superseded heads or stale-base reviews.

Preserve the #262 longitudinal observation identity guard and its focused traceability/test evidence on top of current protected main. The #258 Rust toolchain-refresh paths are disjoint, so this reconciliation retains both change sets without force-pushing or changing longitudinal behavior beyond the existing PR delta.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Current-head correction: PR #262 is now at exact head 8cd3ae2 after a concurrent branch update. Please review only this current head against protected main 503a4e6; ignore earlier review requests for superseded heads and do not require a force push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant