fix(result): require durable supersession predecessors - #331
Conversation
|
Warning Review limit reachedNext included review available in 31 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 (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough결과 스냅샷의 Changes결과 스냅샷 무결성
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to The persistence API now enforces durable supersession predecessors, but its public documentation still does not clearly define that requirement for callers. This is a bounded integration risk; the PR is otherwise mergeable with explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant Caller
participant persist_result_snapshot
participant PostgreSQL
Caller->>persist_result_snapshot: ResultSnapshot 저장 요청
persist_result_snapshot->>PostgreSQL: result_snapshot_ref 존재 여부 조회
alt 기존 스냅샷 존재
PostgreSQL-->>persist_result_snapshot: 기존 행 반환
persist_result_snapshot-->>Caller: ConflictingReplay 또는 동일 재생 결과 반환
else 새 스냅샷
persist_result_snapshot->>PostgreSQL: 선행 스냅샷 조회
alt 선행 스냅샷 존재
PostgreSQL-->>persist_result_snapshot: 선행 행 반환
persist_result_snapshot->>PostgreSQL: 스냅샷 삽입
PostgreSQL-->>Caller: 저장 성공
else 선행 스냅샷 없음
PostgreSQL-->>persist_result_snapshot: 조회 결과 없음
persist_result_snapshot-->>Caller: InvalidSupersession 반환
end
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head96e22211691efed5700ef8637549d1d30d60367e. -
Head SHA:
96e22211691efed5700ef8637549d1d30d60367e -
Workflow run: 32700481093
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: postgres_result_supersession_predecessor.rs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: postgres_result_supersession_predecessor.rs"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: postgres_result_supersession_predecessor.rs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: postgres_result_supersession_predecessor.rs"]
R2 --> V2["targeted test run"]
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Dismissed as predecessor-head evidence only. This REQUEST_CHANGES was explicitly submitted for head 96e22211691efed5700ef8637549d1d30d60367e and workflow run 32700481093. Current PR head is 64262302c230546cd1a43a56d94b090464fafa95; exact-head coverage-evidence and opencode-review are successful, and all current review threads are resolved. This dismissal does not approve the PR or transfer old evidence; all remaining unchanged-head checks and two qualifying independent approvals remain mandatory.
| IF EXISTS ( | ||
| WITH RECURSIVE supersession_lineage AS ( | ||
| SELECT | ||
| result_snapshot_ref AS start_ref, | ||
| supersedes_ref AS current_ref, | ||
| ARRAY[result_snapshot_ref]::text[] AS visited_refs | ||
| FROM result_snapshot | ||
| WHERE supersedes_ref IS NOT NULL | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT | ||
| lineage.start_ref, | ||
| predecessor.supersedes_ref, | ||
| lineage.visited_refs || predecessor.result_snapshot_ref | ||
| FROM supersession_lineage AS lineage | ||
| JOIN result_snapshot AS predecessor | ||
| ON predecessor.result_snapshot_ref = lineage.current_ref | ||
| WHERE lineage.current_ref IS NOT NULL | ||
| AND NOT predecessor.result_snapshot_ref = ANY(lineage.visited_refs) | ||
| ) | ||
| SELECT 1 | ||
| FROM supersession_lineage | ||
| WHERE current_ref IS NOT NULL | ||
| AND current_ref = ANY(visited_refs) | ||
| ) THEN | ||
| RAISE EXCEPTION 'result snapshot supersession lineage must be acyclic' | ||
| USING ERRCODE = '23514'; | ||
| END IF; |
There was a problem hiding this comment.
📝 Info: Cycle-detection CTE is quadratic in chain length
The recursive lineage CTE seeds one path per superseding row and walks each to its root, producing O(N^2) intermediate rows for a linear chain of length N, and it runs on every migration reapply. Fine for short lineages; a large table with long chains could make reapply slow.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if result_snapshot_exists(transaction, snapshot_ref)? { | ||
| return classify_existing_snapshot(transaction, snapshot, created_at, schema_version); | ||
| } | ||
|
|
||
| validate_supersession_predecessor(transaction, snapshot.supersedes_ref())?; |
There was a problem hiding this comment.
📝 Info: Extra existence SELECT before every insert
persist_result_snapshot now runs result_snapshot_exists before every insert, adding a round trip to the common new-insert path. The concurrent race stays covered: the insert keeps ON CONFLICT DO NOTHING and falls through to classify_existing_snapshot when inserted==0. Intentional, so existing identities classify before predecessor validation.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Why
The original protected-main base
3bb873f02d2e1639be49e2bc9ac998c158b48d3daccepted an immutableResultSnapshotwhosesupersedes_refcould name a result that did not exist when the successor was inserted. Because result rows are immutable, that left a dangling lineage edge and made a forward-reference cycle constructible through an insufficiently guarded persistence path.The first failing boundary is Psychometrics Commons-owned result persistence, not scoring math:
persist_result_snapshotinserted the caller-supplied predecessor reference without proving that predecessor was already visible, and migration0007_result_snapshot.sqlhad no equivalent direct-SQL guard.TDD / RCA lineage
5abdcefcb5b824332ef10d671d6fb67fd3c14d40adds a real PostgreSQL contract requiring a missing supersession predecessor to fail before any result row is stored.0225202de8929d91068bc17e8410e10245a0c2c2adds typed application-level predecessor validation withInvalidSupersessionbefore insert.04d1f399eab9344a77de87631310ac8df9079336adds aBEFORE INSERTpredecessor trigger and makes migration reapplication fail closed on pre-existing dangling or cyclic supersession evidence.7fb300c89e6704e413e02ce316d95c470612180dcovers the typed application failure, raw-SQL bypass attempt, migration reapplication over simulated historical cyclic evidence, and database-error propagation from predecessor lookup.92d52a3a0c940e6f03a13d3ae5f0435016cb1cd5preserves the existing named self-supersession CHECK as the classifier instead of having the predecessor trigger misreport a self-reference as a missing predecessor.d9051429008d74dc9d78389b454529f1bde47057proves the fixed-schema integration test mutex is process-local and therefore invisible to a second PostgreSQL session.3d35065b5db17b5ef12831013fe415bde8d0b807replaces that mutex with a database-session advisory lock, so concurrent Cargo/test processes sharingTEST_DATABASE_URLcannot race the result-supersession schema.eb9cf192633cac288b4dfb2319249f228779af1eis a regular two-parent merge of this exact product/test tree with protectedmain@8c6b433fc27678d772759720ca5325d3c3f23b4a; no rebase, force-push, or history rewrite was used.The falsifiable acceptance hypothesis is that every newly inserted supersession edge references a different predecessor row already visible to the writer before the successor insert. A predecessor inserted earlier in the same transaction is intentionally visible and valid; a predecessor still uncommitted in a sibling transaction is rejected fail-closed. The ordering constraint prevents forward references and cycle construction, while migration reapplication rejects pre-existing dangling or cyclic lineage.
Current exact head
eb9cf192633cac288b4dfb2319249f228779af1eis directly ahead of protectedmain@8c6b433fc27678d772759720ca5325d3c3f23b4awith zero commits behind. The exact diff remains limited to:migrations/0007_result_snapshot.sqlsrc/postgres_result_snapshot.rstests/postgres_result_supersession_predecessor.rsExact-head Runtime CI, Security Scan, SAST Semgrep, SPDX SBOM evidence, and supply-chain provenance were freshly queued after the reconciliation; queued evidence is not passing.
Architecture / documentation impact
This implements the already accepted ADR-0010 rule that corrections create immutable superseding results and supersession-chain integrity is validated. It changes enforcement only: no bounded-context ownership, public/admin operation, lifecycle state, logical entity/cardinality, serialized result shape, or psychometric publication rule changes. Existing ADR-0010 / TRD / UML / ERD semantics therefore remain unchanged; this PR remains IMPLEMENTED_ON_ACTIVE_PR evidence only until this exact head lands on protected
main.Scope
This is result-persistence lineage integrity plus the reliability of its real-PostgreSQL acceptance fixture only. It does not recompute psychometrics, change result observations, require same-session or same-participant supersession semantics, import the stale current-result reload implementation from #157, change public result transport, or touch another bounded context/database. #157 remains a separate reload/read-path concern.
Merge discipline
Do not merge until the unchanged exact head passes Runtime CI including the real PostgreSQL contracts, exact owned statement/branch coverage, rustfmt/Clippy/rustdoc, Security/SAST, SPDX SBOM, supply-chain provenance, has zero valid unresolved findings, and satisfies the live qualifying independent non-author/non-last-pusher approval requirement. The historical OpenCode REQUEST_CHANGES review applies to predecessor head
96e22211691efed5700ef8637549d1d30d60367e; it is not passing evidence for this new head and must not be treated as current approval. Pending, queued, skipped, cancelled, absent, stale, predecessor, synthetic, or model-only evidence is not passing. Never self-approve.Summary by CodeRabbit