feat: add bitemporal people domain kernel - #3
Conversation
|
Important Review skippedDraft detected. 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 |
seonghobae
left a comment
There was a problem hiding this comment.
Two current-head correctness/security defects need regression-first repair before this kernel can become the persistence/API dependency. First, AssignmentRecord.__post_init__ compares Decimal values without rejecting non-finite inputs; Decimal('NaN') can raise InvalidOperation instead of the stable domain error, while non-finite allocations must fail closed. Second, validate_assignment_portfolio() embeds person_record_id, the exact allocation, and effective date in AllocationExceededError. That domain exception is intended to cross adapters, so the message unnecessarily discloses an HR identifier and sensitive schedule detail. Add tests for NaN/±Infinity and for zero UUID/date leakage, then reject all non-finite ratios and return a bounded generic allocation error. Also reconcile the durable person anchor with ADR-0003: the accepted schema system-versions person_record; either model its recorded interval explicitly or document why creation/retirement knowledge time is owned outside this package, with a contract test preventing silent loss.
seonghobae
left a comment
There was a problem hiding this comment.
Package-evidence mismatch on the current head: the PR body says ./scripts/run_domain_quality.sh requires an installed-wheel smoke, but the script stops after source-tree tests/docstrings/repository tests. It never builds a wheel, verifies orgmetra_domain/py.typed inside the artifact, installs the wheel, or imports it outside the source tree. Add a regression contract for these steps, build with the pinned local toolchain, inspect the wheel contents, install it into an isolated target/venv, and run a smoke import from a directory that cannot resolve packages/orgmetra-domain/src. Keep the PR description and exact CI evidence aligned with what actually executes.
seonghobae
left a comment
There was a problem hiding this comment.
Version-identity mismatch introduced on this head: OrganizationUnitRecord says the same organization_unit_id can be reused by successive bitemporal versions, and JobProfileRecord makes the same promise for job_profile_id, but neither type has a separate version-record identifier. The protected foundation schema also makes those IDs primary keys, so two recorded versions cannot coexist. Choose one truthful contract before persistence adapts: either introduce durable organization_unit / job_definition anchors plus separate version records with their own UUIDs and non-overlap rules, or define these current IDs as immutable version identities and add a separate lineage/anchor reference. Add retroactive-correction tests proving the old recorded version remains addressable and positions/criteria reference the intended version. Do not document ID reuse that the model and database cannot represent.
|
@coderabbitai review Please review the current exact head of this dependency-stacked bitemporal domain slice. Keep the PR Draft until the foundation branch is merged and this PR is retargeted to |
There was a problem hiding this comment.
Stale comment
This draft is not merge-ready. Head
c707ae0still treats superseded assignment rows as current FTE, leaks candidate/person identifiers on relink, and resolves mixed identities as one ambiguous fact. GitHub also reportsCONFLICTINGagainstfeat/foundation-product-baseline(PR #8).Do not merge, do not retarget, and do not transfer predecessor checks or approvals. Keep this PR Draft until #8 merges and this stack is rebased onto the live protected baseline with fresh exact-head review and checks.
Blocking defects
- Assignment portfolio ignores recorded time.
validate_assignment_portfolio()sums every row by effective date only. A legal retroactive correction (close the old recorded interval, open a corrected row) is rejected as over-allocation. Adapters that persist history will block normal HR corrections. Require timezone-awareknown_at, include only rows visible at that knowledge time, and add a regression for the A / A' / B correction triple.- Candidate relink error leaks HR identifiers.
CandidateWorkerRelinkErrorembeds both UUIDs. The allocation error was already made generic for adapter safety; this path was not. Return a bounded message and assert neither UUID appears.- Resolver is not identity-scoped.
resolve_bitemporal_fact()fails closed as soon as two people have a visible name at the same coordinate. Live gist exclusions are per identity. Require an identity getter or per-entity helper so ambiguity means one identity has two visible versions.- Stack is stale. This tree still vendors a pre-tenant schema snapshot while live foundation already has tenant, name, and version tables. Rebase onto current #8 after it merges; do not treat this head as persistence truth.
Required before any merge attempt
- Rebase onto the live foundation baseline and resolve contract drift.
- Recorded-time assignment validation plus the correction regression.
- Generic candidate-relink error with zero UUID leakage.
- Identity-scoped historical resolution.
- Realistic HRIS tests: effective-dated name change,
recorded_toboundary, timezone-equal knowledge time, assignment without covering employment, concurrent employments, same-position double assignment, org cycleA→B→A.- Fresh exact-head quality on the retargeted PR.
Employment/position still collapse identity and version, and assignments still skip
employment_record_id. Those are buyer-visible holes. Fix them in this kernel or an immediately following revision; do not freeze the weaker model as accepted persistence truth without an explicit ADR.Independent review, OpenCode, Noema, Strix, SAST, and Security Scan remain required. This comment does not approve and does not replace those gates.
Sent by Cursor Automation: fix all
| assignment_record_id: UUID | ||
| person_record_id: UUID | ||
| position_record_id: UUID | ||
| allocation_ratio: Decimal |
There was a problem hiding this comment.
Two buyer-visible holes sit on this record:
- There is no
employment_record_id. Rehire (employment 1 closed, employment 2 open) and dual employment cannot say which relationship an assignment belongs to. Headcount and "who is this assignment under?" will be wrong. Add the field and reject an assignment that has no covering employment over the effective interval. allocation_ratiois an unconstrainedDecimalafter the finite(0, 1]check. This head acceptsDecimal('0.00001'). Live foundation storesnumeric(5,4). Persist rounds to0.0000and then fails> 0. Reject any ratio that is not finite with at most four decimal places.
Next action: add those two contract tests, then tighten __post_init__ and the portfolio validator.
| class EmploymentRecord: | ||
| """Represent one effective-dated employment relationship for a person.""" | ||
|
|
||
| employment_record_id: UUID |
There was a problem hiding this comment.
EmploymentRecord still uses one id as both durable identity and version identity. A retroactive status correction needs a new row; because the id is the identity, that looks like a second employment. PositionRecord has the same collapse, and AssignmentRecord pins to position_record_id.
Org/job on this head already use durable anchors plus version ids. Shipping this kernel freezes the weaker model for employment, position, and assignment.
Choose one truthful contract before persistence adapts: durable id + version id + non-overlap rules, or document persistence-only corrections with a contract test at the ADR-0004 bar used for PersonRecord. Immediate self-parent is rejected below; cycle A→B→A is not.
Next action: do not treat this shape as accepted persistence truth without an explicit ADR and tests.
| |---|---|---|---|---| | ||
| | Separate person/employment/job/position/assignment | Core bounded contexts | `person_record`, `employment_record`, `job_profile`, `position_record`, `assignment_record` | schema/domain tests | ADR-0001 | | ||
| | Separate person/employment/organization/job/position/assignment | Core bounded contexts + `orgmetra-domain` active PR | `person_record`, `employment_record`, `organization_unit`, `job_profile`, `position_record`, `assignment_record` | `test_domain.py` record and assignment tests | ADR-0001 | | ||
| | Durable organization/job identity with bitemporal descriptive history | People Core + `OrganizationUnitRecord` / `OrganizationUnitVersionRecord` / `JobProfileRecord` / `JobProfileVersionRecord` active PR | `organization_unit`, `organization_unit_version`, `job_profile`, `job_profile_version` | `RecordValidationTests` | ADR-0001, ADR-0003 | |
There was a problem hiding this comment.
GitHub mergeability is CONFLICTING. This stacked tree still vendors the pre-tenant schema snapshot (display_name on person_record, no version tables) and hashes it. Live foundation already has tenant_record, person_name_record, organization_unit_version, and job_profile_version.
TRACEABILITY must name the live tables this kernel can actually implement. After rebase, rewrite the person-lifecycle contract test and the integrity manifest. Add a contract that domain types remain implementable against the live schema (numeric(5,4), tenant FKs).
Next action: rebase onto current #8 after it merges. Do not transfer predecessor checks or approvals.
|
|
||
| ## Active domain-kernel test evidence | ||
|
|
||
| The stacked implementation runs 17 behavioral domain tests plus three repository-contract tests. It proves: |
There was a problem hiding this comment.
Stale inventory. This says "17 behavioral domain tests plus three repository-contract tests." The current package has more tests than that, and it still omits the buyer-visible HRIS cases that are missing: recorded-time assignment correction, identity-scoped resolution, effective-dated name change, covering employment, org cycle, and numeric(5,4) quantization.
100% statement/branch coverage of existing code does not prove those paths exist.
Next action: rewrite this paragraph from the new inventory after the blocking tests land.
| person_record_id=PERSON_ID, | ||
| display_name=display_name, | ||
| period=BitemporalPeriod( | ||
| effective_from=date(2026, 1, 1), |
There was a problem hiding this comment.
name_record() hardcodes effective_from=2026-01-01 and effective_to=None. These tests only cover correction of the same effective interval.
Missing realistic cases:
- effective-dated legal name change (old interval closes, new interval opens)
- exact
recorded_toboundary (known_at == recorded_tomust not see the closed row) known_atin+09:00equal to a UTC recorded instant
Coverage can stay 100% while these paths stay untested. Parameterize effective bounds and add those three cases.
There was a problem hiding this comment.
This draft is still not merge-ready. Head cf557c9 fixed recorded-time FTE, generic candidate-relink errors, and identity-scoped resolution. GitHub still reports dirty against feat/foundation-product-baseline (PR #8).
Do not merge, do not retarget, and do not transfer predecessor checks or approvals. Keep this PR Draft until #8 merges and this stack is rebased onto the live protected baseline with fresh exact-head review and checks.
Addressed on this head
validate_assignment_portfolio(..., known_at=)ignores rows that were not visible at that knowledge time.CandidateWorkerRelinkErrorno longer embeds UUIDs.resolve_bitemporal_fact(..., identity_of=, identity=)fails closed only for one identity.
Still blocking on this head
- No
employment_record_id. Assignments cannot distinguish rehire from dual employment. - Ratios are not
numeric(5,4).Decimal('0.00001')constructs and will round to an invalid persist value. - Employment/position still collapse identity and version. A retroactive status correction looks like a second employment or a new seat.
- Coverage is absent. Even after those fields land, do not resolve one employment version at assignment start only. PR #17 at
2fb7f2dedid that and accepted assignments past termination while rejecting legal leave slices. Walk every effective-day boundary;activeandleavecover,terminateddoes not. - Stack is stale. Rebase onto current #8 after it merges.
Successor
#21 implements the remaining kernel contract, including exclusive/concurrent rehire, same-seat uniqueness, A→B→A cycle rejection, and the buyer-visible name-change / timezone cases. Prefer #21 over merging #3 or #17 at their current heads.
Independent review, OpenCode, Noema, Strix, SAST, and Security Scan remain required. This comment does not approve and does not replace those gates.
Sent by Cursor Automation: Fix Issues
| assignment_record_id: UUID | ||
| person_record_id: UUID | ||
| position_record_id: UUID | ||
| allocation_ratio: Decimal |
There was a problem hiding this comment.
Still missing on this head: employment_record_id and numeric(5,4) quantization. A rehire cannot say which employment an assignment belongs to, and Decimal('0.00001') still constructs. Repair is #21 — do not treat this field list as persistence truth.
| class EmploymentRecord: | ||
| """Represent one effective-dated employment relationship for a person.""" | ||
|
|
||
| employment_record_id: UUID |
There was a problem hiding this comment.
EmploymentRecord still uses one id as both durable identity and version identity. Leave/termination/rehire corrections need a new row without looking like a second employment. PositionRecord has the same collapse. Repair is #21, which also walks every effective day so a later terminated slice cannot hide behind the start-date status.
|
Closing as superseded by PR #21. Fresh ancestry proof shows #21 head |


Dependency
Stacked on #8. Review and merge #8 first, then retarget this PR to the protected default branch and obtain fresh exact-head review/check evidence. Do not transfer predecessor checks or approvals. Keep this PR Draft until that dependency order is satisfied.
What changed
Adds the first executable Orgmetra product slice as the independently importable
orgmetra-domainpackage:PersonRecordidentity anchor with mutable names moved into bitemporalPersonNameRecordfactsOrganizationUnitVersionRecordandJobProfileVersionRecordfactspy.typedmarker and installed-wheel smokeThe package intentionally owns no web framework, database driver, authentication, LLM, or psychometric arithmetic. Future services embed these invariants and mirror them transactionally in PostgreSQL. Mathematical and psychometric production compute remains Rust-first in its owning product.
Regression-first evidence
known_at, filters by recorded visibility, and proves the A / A' / B retroactive-correction boundary so superseded rows are excluded.Quality contract
The gate requires exact 100% owned production statement/branch coverage, 100% public docstrings, installed-wheel smoke, immutable action pins, and repository supply-chain contracts.
Exact candidate
cf557c9f9aa0e0885ad1754b0596ab611439e51dfeat/foundation-product-baselinebase_pr: 8Buyer-visible behavior
The kernel can answer a core HRIS audit question without silently rewriting history: “What fact for this durable identity was effective on this business date, given what Orgmetra knew at this knowledge time?” A retroactive correction preserves the earlier answer before the correction was recorded and returns the corrected fact afterward. Assignment capacity is evaluated against the same knowledge-time concept, so superseded historical rows do not double-count FTE.
Remaining dependency action
After #8 merges: retarget this PR to the protected default branch, integrate the exact protected baseline, repair any resulting contract drift, add the remaining realistic HRIS regressions required by review, and require fresh independent review plus exact-head required checks before merge.