feat: repair recorded-time assignment and identity-scoped people kernel - #17
Conversation
Require known_at on portfolio validation so a closed recorded interval does not inflate FTE. Scope historical resolution per identity, attach assignments to a covering employment, version employment and position status separately, and keep adapter errors free of HR identifiers. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Document recorded-time assignment integrity, versioned employment and position anchors, identity-scoped queries, and the APA 7th sources that justify multilevel staffing and bitemporal reconstruction. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
PR #5 already owns ADR 0005 for purpose-bound PostgreSQL persistence. Keep the stacked tree free of colliding decision numbers. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Do not merge this draft at 2fb7f2de versus pinned base 53d929a. GitHub rejected REQUEST_CHANGES on this head (own-PR rule). CodeRabbit CLI 0.7.3 is installed here; coderabbit auth login --agent timed out (not_authenticated), so this is a kernel employment-truth review, not a CodeRabbit result.
Independent reviewers: keep #17 Draft. The next action is the successor kernel on this stack — status-aware coverage, write-time composition, and rehire-versus-dual classification — then rebase onto live feat/foundation-product-baseline after #8. Do not transfer this review or any check to another head.
What holds
- Timezone-aware
known_atandwas_known_athide closed recorded rows. employment_record_idplusnumeric(5,4)allocation.- Identity-scoped historical resolution; mixed people are not one fact.
- Visible
A→B→Aorganization cycle fails closed at one coordinate. - Relink and allocation errors omit HR identifiers.
What a payroll clerk still gets wrong
- Coverage answers the wrong question.
validate_assignment_employment_coverageresolves one version atassignment.period.effective_fromand then asks that single interval to contain the whole assignment.employment_status_codeis never read.covers_effective_intervalreturns true wheneffective_to is None.active[Jan 1, Jun 1) +leave[Jun 1, Dec 31) on the same employment rejects a Jan–Dec assignment. Parental leave looks like a missing employment.- Open-ended
activeplus a laterterminatedaccepts a Jan–Dec assignment (fail open). A terminated person stays staffed. - A
terminated-only version whose dates contain the assignment is accepted.
- Rehire versus dual is a field, not an invariant. The concurrent-employment test only asserts two IDs differ. Leave the prior employment open, insert a second “rehire,” attach a post-rehire assignment to the old id — coverage accepts it. The clerk cannot tell two live relationships from a closed-then-reopened one.
- Portfolio does not compose with employment truth. The FTE sweep never calls coverage. An assignment whose person is not the named employment’s person still passes
validate_assignment_portfolio. - Merge is dirty.
mergeable_state=dirtyagainstfeat/foundation-product-baseline.git merge-treeconflicts inARCHITECTURE.md,CHANGELOG.md,docs/DATA_MODEL.md, and other docs. Live foundationassignment_recordstill has noemployment_record_id.
Residual
PositionVersionRecordis modeled and never consulted; a closed seat can still be assigned.- No hierarchy history walker; a cycle on another effective day is missed if the caller only checks “today.”
- 100% statement/branch coverage of the current predicates does not recover the true employment parameter a clerk would use.
Next action
Do not approve or merge #17 at this head. Implement status-aware endpoint coverage, write-time portfolio+coverage composition, explicit dual-versus-rehire classification, and closed-position rejection on a successor head. Keep #3 Draft until that repair is integrated and the stack is rebased onto the live foundation after #8. Independent review, OpenCode, Noema, Strix, SAST, and Security Scan remain required on the exact successor SHA.
Sent by Cursor Automation: Fix Issues
| for version in versions | ||
| if version.employment_record_id == assignment.employment_record_id | ||
| ], | ||
| effective_on=assignment.period.effective_from, |
There was a problem hiding this comment.
This resolves one employment version at assignment.period.effective_from only. A clerk recording parental leave (active then leave on the same employment_record_id) is told the employment does not cover a person who is still employed. Walk every effective endpoint of the assignment and of matching versions. Each day in the assignment interval must resolve to exactly one visible version whose employment_status_code is assignment-eligible (active, leave). Reject terminated and fail closed on a gap or overlap.
|
|
||
| if self.effective_from > other.effective_from: | ||
| return False | ||
| if self.effective_to is None: |
There was a problem hiding this comment.
covers_effective_interval returns true whenever self.effective_to is None. Combined with start-day-only coverage, an open active version plus a later terminated version accepts a full-year assignment. Keep this helper as geometry only. Coverage must not treat an open interval as permission to ignore later status slices.
| events_by_position: dict[UUID, list[tuple[date, int, Decimal]]] = defaultdict(list) | ||
| for assignment in visible: | ||
| _append_allocation_events( | ||
| events_by_person, assignment.person_record_id, assignment |
There was a problem hiding this comment.
The write-time FTE sweep keys only person_record_id and position_record_id. It never checks that the named employment belongs to that person and never runs coverage. An assignment can charge FTE to the wrong person and still pass. Add a write-time composer that rejects assignment.person_record_id != employment.person_record_id and runs coverage for every visible row before allocation is accepted.
| (first, second), | ||
| known_at=datetime(2026, 3, 1, tzinfo=timezone.utc), | ||
| ) | ||
| self.assertEqual(first.employment_record_id, EMPLOYMENT_ID) |
There was a problem hiding this comment.
This only asserts two employment IDs differ at 0.5+0.5. That does not distinguish rehire from dual. Add clerk-visible cases: leave-spanning same-employment assignment accepted; open active + later terminated rejected past the term date; terminated-only cover rejected; prior employment still open + second “rehire” classified as dual, not rehire; assignment after a terminal close cannot keep the old id.
| ## Decision | ||
|
|
||
| 1. `validate_assignment_portfolio()` requires timezone-aware `known_at` and counts only rows visible at that instant. `validate_assignment_portfolio_history()` repeats the check at every recorded endpoint before a write. | ||
| 2. `AssignmentRecord` names the durable `employment_record_id`. Coverage validation requires the visible employment version to contain the assignment's effective interval. |
There was a problem hiding this comment.
Decision 2 overclaims. Requiring one visible version interval to contain the assignment is the predicate that rejects Worker→Leave→Worker and fail-opens past termination. Replace this with status-aware coverage across the assignment interval, and state that rehire requires a prior terminal close while overlapping live employments are dual.
|
Do not merge this head. The clerk-visible coverage, dual-versus-rehire, and write-time composition repair is on successor PR #19 ( |
|
Closing as superseded by PR #19. Fresh ancestry proof shows #19 head |
2 participants
Dependency
Stacked on the bitemporal people kernel (PR #3) and the foundation baseline (PR #8). Review and merge #8 first. Keep #3 Draft until it is rebased onto the live foundation head and this repair is integrated. Do not transfer predecessor checks or approvals.
What changed
Repairs the buyer-visible holes that blocked PR #3 from becoming the persistence/API dependency:
validate_assignment_portfolio()now requires timezone-awareknown_atand ignores rows that were not visible at that knowledge timevalidate_assignment_portfolio_history()checks every recorded endpoint before a writeAssignmentRecordnamesemployment_record_idand rejects ratios that cannot persist asnumeric(5,4)A→B→Afails closedTDD evidence
RED buyer-visible cases were added first in
test_buyer_visible_invariants.py, then the kernel was implemented. Localcoverage.py7.13.3 reports exact 100% statement/branch coverage (243 statements, 64 branches, 0 missed, 0 partial). Installed-wheel smoke importsorgmetra-domain0.2.0 withpy.typed.Next action
employment_record_id, employment/position version tables, and recorded-time gist exclusions before embedding this kernel.CONFLICTINGagainst the foundation branch.Independent review, OpenCode, Noema, Strix, SAST, and Security Scan remain required.