feat(core): persist bitemporal position reporting relationships - #106
feat(core): persist bitemporal position reporting relationships#106seonghobae wants to merge 30 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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}
| BEGIN | ||
| -- Graph validity is a tenant-wide invariant. Concurrent opposite edge | ||
| -- inserts can otherwise both validate before either commits. A transaction | ||
| -- advisory lock serializes graph mutations for one tenant while still | ||
| -- allowing different tenants to progress independently. This trigger is | ||
| -- VOLATILE, so the queries after lock acquisition use current committed | ||
| -- state under PostgreSQL READ COMMITTED semantics. | ||
| PERFORM pg_catalog.pg_advisory_xact_lock( | ||
| pg_catalog.hashtextextended( | ||
| 'orgmetra_position_reporting:' || NEW.tenant_record_id::text, | ||
| 0 | ||
| ) | ||
| ); |
There was a problem hiding this comment.
📝 Info: Cycle detection depends on VOLATILE snapshot behavior
Concurrency safety in enforce_position_reporting_scope rests on the trigger being VOLATILE, so the recursive graph query re-snapshots after the advisory lock and sees the committed opposite edge under READ COMMITTED. Correct as written, but fragile: changing the function to STABLE would silently break serialization.
Was this helpful? React with 👍 or 👎 to provide feedback.
| with_tenant "${TENANT_ID}" "${DATABASE_URL}" -v ON_ERROR_STOP=1 <<SQL | ||
| INSERT INTO position_record ( | ||
| tenant_record_id, position_record_id, organization_unit_id, job_profile_id | ||
| ) VALUES | ||
| ('${TENANT_ID}', '${POSITION_X}', '${ORG_ID}', '${JOB_ID}'), | ||
| ('${TENANT_ID}', '${POSITION_Y}', '${ORG_ID}', '${JOB_ID}'); | ||
| INSERT INTO position_record_version ( | ||
| tenant_record_id, position_record_version_id, position_record_id, | ||
| position_status_code, effective_from | ||
| ) VALUES | ||
| ('${TENANT_ID}', '${POSITION_X_VERSION}', '${POSITION_X}', 'active', DATE '2026-01-01'), | ||
| ('${TENANT_ID}', '${POSITION_Y_VERSION}', '${POSITION_Y}', 'active', DATE '2026-01-01'); | ||
| INSERT INTO position_reporting_relationship_record ( | ||
| tenant_record_id, position_reporting_relationship_record_id, | ||
| subordinate_position_record_id, relationship_type_code | ||
| ) VALUES | ||
| ('${TENANT_ID}', '${REL_X}', '${POSITION_X}', 'solid_line'), | ||
| ('${TENANT_ID}', '${REL_Y}', '${POSITION_Y}', 'solid_line'); | ||
| SQL |
There was a problem hiding this comment.
📝 Info: Tests share DB state and must run in fixed order
test_position_reporting_review_binding_postgres.sh and test_position_reporting_concurrency_postgres.sh reuse tenant, org, job, and version rows inserted by test_position_reporting_persistence_postgres.sh on the shared Postgres service. They only pass in the workflow's fixed step order; run in isolation they fail on missing prerequisites.
Was this helpful? React with 👍 or 👎 to provide feedback.
| set +e | ||
| (with_tenant "${TENANT_ID}" "${DATABASE_URL}" -v ON_ERROR_STOP=1 -c "${sql_x}" >"${log_x}" 2>&1) & | ||
| pid_x=$! | ||
| sleep 0.5 | ||
| with_tenant "${TENANT_ID}" "${DATABASE_URL}" -v ON_ERROR_STOP=1 -c "${sql_y}" >"${log_y}" 2>&1 | ||
| status_y=$? | ||
| wait "${pid_x}" | ||
| status_x=$? | ||
| set -e |
There was a problem hiding this comment.
📝 Info: Concurrency test winner is timing-dependent
The test assumes the backgrounded X transaction grabs the advisory lock within the 0.5s head start before Y runs. On a slow runner Y could win the lock first, flipping which transaction hits the cycle error and producing a spurious failure. The 2s pg_sleep makes this unlikely but not guaranteed.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| PR #95 owns the in-memory pre-mutation review packet. This persistence slice does not copy or modify that branch; it accepts the review digest plus immutable application audit evidence as the handoff boundary. A later authorized host adapter may translate a verified review packet into the database command, but direct cross-service SQL is out of scope. | ||
|
|
||
| Canonical `docs/DATA_MODEL.md` / `docs/ERD.md` are intentionally not edited in this stacked slice while independent active database PRs also own those high-conflict documents. Integration must reconcile the accepted relationship tables into canonical data-model documentation after dependency ordering is resolved. |
There was a problem hiding this comment.
🔍 Canonical data-model docs not updated for new tables
The two new tables are not reflected in docs/DATA_MODEL.md or docs/ERD.md; the ADR defers this to integration to avoid conflicts with concurrent database PRs. Reviewer should confirm reconciliation happens at integration so the canonical data-model docs stay current with the schema.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Buyer-visible gap
PR #94 defines governed Position→Position solid-line reporting snapshots but intentionally leaves durable persistence/mutation out of scope. This stacked Orgmetra-only lane adds normalized bitemporal persistence without deriving supervision from Person, Assignment, or organization-unit parentage.
Governed persistence contract
position_reporting_relationship_recordowns stable(tenant, subordinate Position, solid_line)identity andposition_reporting_relationship_versionowns manager Position plus effective/business time and PostgreSQL-owned system-recorded time. Persisted versions require same-tenant Position references, distinct human reviewer/applying actor, immutable review/application audit+outbox correlation, staffable PositionVersion coverage for both endpoints across the full effective interval, cycle-free graph state, immutable history/TRUNCATE resistance, and FORCE RLS. Person/Assignment/worker identity, compensation, ratings, assessment output, and free-form HR text are absent. RLS is defense in depth and does not replace purpose-bound application authorization.Repair sequence
78e092353be310f2bcefd30c1f7439b5ed1b235eestablished database-owned recorded time, one durable solid-line anchor per subordinate, same-tenant scope, self-report/cycle rejection, immutable history, audited human-reviewed application, and FORCE-RLS acceptance criteria.209259120d10b1cd18ab1e6184297f5af4730ffcadded the normalized relations and guards.2e421d8927516dd4b67e469c25ef9cf12107ca34made self-report/cycle regressions transaction-independent.566b74570e479735bbc2fedae7c6b56eb35ff3d9,fc1fb92ad208abdc895b1f94a8d637531e126d2b,affa260b1e94e0040fdfcc59f7bcdad5117bd9a4, andcfef3e27258104b2de448580777271e8ac7d16bf: the audit event'sorgmetraevidencemust equal the reviewed-evidence digest and stored application evidence must equal the immutable audit envelope digest exactly.69923053bb8f39fbe9ea876c33443c38b5ef4b63demonstrated that concurrent opposite edges need serialization;df13397ded4dfa6b605b7ded7cfb7e4468d3f0fcadded a transaction-scoped tenant graph advisory lock before graph reads.8f0a874761bcf1f84e7d0544fabf9394178773bashowed persistence accepted Position anchors with no staffable PositionVersion coverage. Root repairfacfff9d91243fe06639d60a978cb5072cfec64faddedposition_reporting_has_staffable_coverage(...), using PostgreSQL 16 multirange containment so same-tenantactive/openPositionVersion ranges must cover the relationship's entire effective interval.61611908b49a5ff74f2b7f10a2883a2572070b54added active PositionVersion fixtures for the X/Y race endpoints. Exact-head Position Reporting Persistence Quality run32688382859, job97317512146, completed GREEN and proved exact checkout, deterministic provenance, primary persistence regressions, immutable review binding, two-session cycle prevention, and a clean checkout.2af25408d95ac9bb362f318d218af507c0861f8badded a valid immutable audit/outbox event whose purpose/actor/evidence/subject/result/time all matched the requested mutation but whose governedorgmetrareasonwasunrelated_change. The RED-head hosted run32692438893was cancelled after the branch advanced and is not claimed as terminal RED evidence.ef8c7776b2520e6839c14da8122aacfe3f6ad1e4now requiresorgmetrareason = approved_reporting_line_changein the authoritative insert guard. ADR 0106 and traceability were aligned in8ede14cc7430d83955c38c867d1521989a557ea7and current headc35ad114edbce7a4ebafcea793748493f1346351.Current exact state
Current exact head:
c35ad114edbce7a4ebafcea793748493f1346351.Parent base: #94
feat/position-reporting-hierarchy@3f67182bb3065f2fc8fd974bfdd75a390d8a8fdc.GitHub reports this PR open, Draft, and mergeable against its stack base.
Exact-current-head Position Reporting Persistence Quality run
32692576503is terminal GREEN. It is lane-local stack evidence only: #106 still lacks post-parent-integration Foundation/SAST/Security/Recovery/product evidence against fresh protecteddevelop, and no predecessor #94 check or review is transferred. Fresh submitted-review and review-thread queries are empty.Stack and merge governance
This PR is intentionally stacked on #94. #94 must integrate first; its checks/reviews do not transfer. After #94 integrates, retarget #106 to fresh
develop, reconcile migration/document ordering, refetch exact head/base/rules/reviews/threads, and rerun all applicable Foundation/SAST/Security/Recovery/product evidence on the resulting head before review readiness.PR #95 remains the separate Orgmetra owner for the pre-mutation review packet; this branch consumes only reviewed-evidence digest plus immutable application audit correlation and does not rewrite #95. No dedicated-writer dependency repository or cross-service application table is modified.