Skip to content

feat: add bitemporal people domain kernel - #3

Closed
seonghobae wants to merge 46 commits into
feat/foundation-product-baselinefrom
feat/bitemporal-people-core
Closed

feat: add bitemporal people domain kernel#3
seonghobae wants to merge 46 commits into
feat/foundation-product-baselinefrom
feat/bitemporal-people-core

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

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-domain package:

  • non-empty half-open effective-time and system-recorded-time value object
  • timezone-aware recorded-time validation
  • identity-scoped historical fact resolution at one effective-date/knowledge-time coordinate
  • fail-closed ambiguity for overlapping visible versions of the requested durable identity, without conflating other identities
  • durable PersonRecord identity anchor with mutable names moved into bitemporal PersonNameRecord facts
  • durable organization/job identity anchors with separate bitemporal OrganizationUnitVersionRecord and JobProfileVersionRecord facts
  • distinct employment, organization, job, position, and assignment concepts
  • organization hierarchy history with immediate self-parent rejection
  • positions that reference durable organization/job identities rather than one historical description
  • multiple simultaneous assignments validated at an explicit timezone-aware knowledge time so superseded recorded versions do not inflate FTE
  • append-only and idempotent candidate-to-worker linkage with bounded identifier-free conflict errors
  • explicit domain error types
  • PEP 561 py.typed marker and installed-wheel smoke
  • package README, changelog, architecture/ADR/traceability/test-strategy updates

The 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

  • Non-finite assignment allocations fail with stable domain errors.
  • Assignment over-allocation errors omit person IDs, dates, and exact allocation values.
  • Candidate relink conflicts omit candidate/person UUIDs.
  • Assignment portfolio validation rejects naive known_at, filters by recorded visibility, and proves the A / A' / B retroactive-correction boundary so superseded rows are excluded.
  • Historical resolution requires an identity selector, ignores simultaneously visible facts for other identities, and still fails closed on duplicate visible versions of the requested identity.
  • The active manifest now tracks 50 artifacts, including the assignment-security regression file that was previously omitted from stack integrity evidence.
  • Focused local semantic assertions for the changed assignment, temporal-resolution, and candidate-link contracts are GREEN. Hosted exact-head checks remain authoritative.

Quality contract

python -m pip install --only-binary=:all: --require-hashes -r requirements/ci.txt
./scripts/run_domain_quality.sh
python tests/validate_repository.py

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

Buyer-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.

@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 29082305-0c81-49ff-9cd2-2febdae9c269

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@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 main; findings should be tied to the current source head, not predecessor-check state.

@cursor cursor 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.

Stale comment

This draft is not merge-ready. Head c707ae0 still treats superseded assignment rows as current FTE, leaks candidate/person identifiers on relink, and resolves mixed identities as one ambiguous fact. GitHub also reports CONFLICTING 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.

Blocking defects

  1. 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-aware known_at, include only rows visible at that knowledge time, and add a regression for the A / A' / B correction triple.
  2. Candidate relink error leaks HR identifiers. CandidateWorkerRelinkError embeds 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.
  3. 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.
  4. 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_to boundary, timezone-equal knowledge time, assignment without covering employment, concurrent employments, same-position double assignment, org cycle A→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.

Open in Web View Automation 

Sent by Cursor Automation: fix all

Comment thread packages/orgmetra-domain/src/orgmetra_domain/assignment.py
Comment on lines +18 to +21
assignment_record_id: UUID
person_record_id: UUID
position_record_id: UUID
allocation_ratio: Decimal

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two buyer-visible holes sit on this record:

  1. 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.
  2. allocation_ratio is an unconstrained Decimal after the finite (0, 1] check. This head accepts Decimal('0.00001'). Live foundation stores numeric(5,4). Persist rounds to 0.0000 and 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.

Comment thread packages/orgmetra-domain/src/orgmetra_domain/candidate.py
Comment thread packages/orgmetra-domain/src/orgmetra_domain/temporal.py
class EmploymentRecord:
"""Represent one effective-dated employment relationship for a person."""

employment_record_id: UUID

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread docs/TRACEABILITY.md
|---|---|---|---|---|
| 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 |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread docs/TEST_STRATEGY.md Outdated

## Active domain-kernel test evidence

The stacked implementation runs 17 behavioral domain tests plus three repository-contract tests. It proves:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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_to boundary (known_at == recorded_to must not see the closed row)
  • known_at in +09:00 equal to a UTC recorded instant

Coverage can stay 100% while these paths stay untested. Parameterize effective bounds and add those three cases.

Comment thread packages/orgmetra-domain/tests/test_domain.py Outdated

@cursor cursor 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.

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.
  • CandidateWorkerRelinkError no longer embeds UUIDs.
  • resolve_bitemporal_fact(..., identity_of=, identity=) fails closed only for one identity.

Still blocking on this head

  1. No employment_record_id. Assignments cannot distinguish rehire from dual employment.
  2. Ratios are not numeric(5,4). Decimal('0.00001') constructs and will round to an invalid persist value.
  3. Employment/position still collapse identity and version. A retroactive status correction looks like a second employment or a new seat.
  4. Coverage is absent. Even after those fields land, do not resolve one employment version at assignment start only. PR #17 at 2fb7f2de did that and accepted assignments past termination while rejecting legal leave slices. Walk every effective-day boundary; active and leave cover, terminated does not.
  5. 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.

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

assignment_record_id: UUID
person_record_id: UUID
position_record_id: UUID
allocation_ratio: Decimal

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Closing as superseded by PR #21. Fresh ancestry proof shows #21 head 8a8fce96d27988b43b85079fd6baaea4abf8641e is 2 commits ahead of and 0 commits behind this PR's exact head cf557c9f9aa0e0885ad1754b0596ab611439e51d, with the merge base exactly equal to this PR head. #21 therefore preserves this bitemporal people-domain history and adds the later durable employment/position anchors plus status-aware assignment coverage regressions. PR #19 is a divergent competing successor and is not being treated as equivalent. Reviews/checks do not transfer; the surviving domain lane must be reconciled against the canonical protected-base foundation before integration.

@seonghobae seonghobae closed this Aug 16, 2026
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