Repository navigation
Align code with confirmed design V1.0: bonus role segregation, one live plan per period, "adjusted" marker - #12
Merged
Conversation
…one live plan per period, adjusted marker) Confirmed design V1.0 chapter 10 items 6 / 10 / 14 (customer sign-off 2026-09-02) were not yet enforced in code: - Bonus role segregation (item 6): registering a bonus now requires the HR reviewer position and approving/rejecting requires the HR head position, so an HR reviewer cannot approve what they registered and an HR head cannot register. The decision is a pure function (requiredBonusPosition) — the single source of truth for the segregation rule. - One live version per assessment period (item 10): publishing is refused when another plan with the same period type + year + period number is already published; closed and archived plans do not block. Period comparison and the refusal message live in pure functions (samePeriod / findPublishedConflict). - "Adjusted" marker (item 14 = A): kpi_entry_line gains is_adjusted and adjust_type_applied, written by the adjustment hook when an approved adjustment lands (both source and result adjustments). The marker shows in the entry-line list view and in the sheet's inline grid, and every indicator row of the result breakdown JSON now carries is_adjusted. The aggregate engine header now cites design V1.0 chapter 8 table 6 instead of the pre-sign-off "undecided / inferred" wording; the algorithm is unchanged. Unit tests cover all three rules; e2e adds T58-T64: two bonus position counter-examples, duplicate publish in the same period (plus success after closing the live version), the adjusted marker on the line and in the result breakdown, and a branch checker's direct value edit being refused. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
docs/04 now compares against the confirmed design V1.0 (chapter references instead of open-question numbers), records the three newly enforced items as their own rows (adjusted marker, one live version per period, branch checker cannot edit values), drops every "pending decision" note that the 2026-09-02 sign-off settled, and recounts the statistics line so it matches the status column. One new deviation is recorded honestly: the HR reviewer / HR head positions have read scope only, so bonus registration and approval are reachable by an administrator alone until their record-level write scope is decided. docs/05 is rewritten on the dev-test report template: verdict page with the case count, environment table carrying the tested commit SHA and how the port / process / revision were cross-checked, a seven-column plan-and-result table covering all 66 e2e cases, per-case detail for the seven added cases, a problem record with reproduction steps and a workaround, and the verbatim output of this run's `pnpm verify` and `pnpm e2e`. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
…wn plans Ruling of 2026-09-02, option A. Plan publish now writes two more classes of per-plan sharing rule, using the platform's `position` recipient type (ADR-0090 D3 — the rule evaluator expands a position into its holders), so the HR reviewer and HR head positions get edit on every entry sheet and every adjustment of that plan. Bonuses need no rule of their own: they are master-detail children of the entry sheet, and the record-level check reads the master. The new rules ride the existing machinery unchanged — criteria carry the plan id, names carry the plan prefix, publish reconciles them, and a closed or archived plan downgrades them from edit to read with everything else. Declaration now matches enforcement: the department reporter permission set no longer declares create on kpi_bonus, since only the HR reviewer may register one. Rule count per plan grows by four, so the e2e expectation is derived again from the plan's configuration rather than adjusted by hand. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
docs/04 turns row 17 back to fully implemented: the deviation recorded in the first round (HR positions had read scope only) is fixed and retested with real position accounts, and the statistics line is recounted against the status column (26 fully implemented, 1 deviation left: no load test yet). docs/05 is refreshed from the second run: verdict is now a pass over 72 cases, the environment table carries the retested commit and the rebuild caveat, the plan-and-result table covers the six added cases, each of them has its own detail section, and the problem record closes items 1 and 2 as fixed-and- retested while adding the stale-build finding that cost the first attempt of this round. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
… grid Review round 2. An explicit `inlineColumns` REPLACES the column set the platform derives from the child object, so declaring one to surface the "adjusted" marker silently dropped six columns from the entry grid — the required source assignment among them. All nineteen columns of `kpi_entry_line` (every field except the master-detail one) are declared again, in the field-definition order the derivation used, with the two new columns last. The three verbose ones — the calculation trace, the last adjustment and the indicator reference — are collapsed with the platform's own `defaultHidden` rather than dropped, so the column picker still reaches them. Also from the review, both small: - The same-period publish check queried plans through the system context with no tenant filter, so a published plan of another organization could block a publish. The organization is now read off the persisted plan row and applied as an equality condition, matching how the sharing service anchors its rules; the pure decision compares tenants too, and falls back to the period-only verdict when either side carries no organization — a missing column must not turn into a silent pass. - The HR-position rationale moved into the JSDoc of `planSharingIntents` instead of sitting between that JSDoc and the signature. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
docs/05 gains problem record 4 (the entry grid lost six derived columns when the explicit column list was introduced), closed as fixed and retested on 6ba334f with the built metadata checked column by column, and is refreshed from this round's run: 68 unit tests, the retested commit and its cross-checks, and the verbatim verify / e2e output. docs/04 row 25 now states that the grid columns are declared explicitly and that no derived column was lost — the three verbose ones are collapsed, not dropped. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
…eriod Review round 3. The tenant guard added last round narrowed the candidate query with an `organization_id` equality condition. Equality does not match NULL, so a published plan whose organization is empty — which is exactly what the seeded and imported rows look like — was filtered out before the decision ran, and the "fall back to the period-only verdict when either side has no organization" branch could never see it. Same-period uniqueness became data-dependent. Candidates are now taken by status alone and the tenant decision stays entirely inside the pure function, where it is already covered: two plans that both carry an organization must match, and a missing organization on either side falls back to the period comparison. A unit case pins the empty-organization candidate, and an e2e case publishes a second plan into the seeded plan's period — the seeded plan carries no organization and still blocks it. Also from the review: the inline-grid comment now says the first seventeen columns are the fields that existed before this change, rather than describing them as the object's full field set. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
docs/05 gains problem record 5 — the organization equality condition added last round hid published plans that carry no organization, which is exactly how the seeded and imported rows look — closed as fixed and retested on 69a33c6. The new case T71 has its own detail section, including the reverse verification: putting the filter back turns T71 red (the duplicate publish succeeds) while T61 stays green, and restoring turns the whole suite green again, with the restored file's blob hash matching HEAD. The environment table also warns that the bundler strips comments, so a comment string cannot serve as a staleness probe. docs/04 row 26 now states that candidates are taken by status alone and the tenant decision lives in the pure function, with T71 as evidence. Part of #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work item #10 (also refreshes the compliance checklist tracked as #7). Baseline: main @ 8cce30c; merges cleanly onto fbf64da.
What changed
Three items of the confirmed design V1.0 (customer sign-off 2026-09-02, chapter 10) were not yet enforced in code:
requiredBonusPositioninsrc/hooks/bonus.hook.ts) — the single source of truth for the rule. Administrators (org owner/admin,kpi_admin) keep the existing passthrough.PlanPublishHookrefuses to publish when another plan with the sameperiod_type+year+period_nois already published; closed and archived plans are history and do not block. Period comparison and the refusal message are pure functions (samePeriod/findPublishedConflict/planPeriodConflictMessage).kpi_entry_linegainsis_adjusted(boolean, read-only, default false) andadjust_type_applied(select, read-only), written by the adjustment hook when an approved adjustment lands — both source and result adjustments are marked. The marker shows in the entry-line list view and in the sheet's inline grid, and every indicator row of the resultbreakdownJSON now carriesis_adjusted.src/lib/aggregate.tsheader now cites design V1.0 chapter 8 table 6 instead of the pre-sign-off "undecided / inferred" wording. The algorithm is unchanged.Docs:
docs/04is re-based on the confirmed design (chapter references, new rows, statistics line recounted against the status column),docs/05is rewritten on the dev-test report template with the tested commit SHA, a seven-column plan-and-result table over every e2e case, per-case detail for the added ones, and the verbatim output of the run.Review round 1 — HR positions get record-level write scope (ruling of 2026-09-02, option A)
The first round could not drive acceptance criteria 1 and 2 with real HR accounts: entry sheets are private, and record-level write access came only from the sharing rules plan publish writes for subject units, checking branches and supervising leaders, so an HR reviewer could read every sheet but write nothing under one. The ruling settled it — grant the scope — and this round implements it:
src/services/sharing-service.tsnow emits two more classes of per-plan rule using the platform'spositionrecipient type (ADR-0090 D3: the evaluator expands a position into its holders):kpi_hr_reviewerandkpi_hr_headeach getediton the plan'skpi_entry_sheetandkpi_adjustment. Bonuses need no rule of their own — they are master-detail children of the sheet and the record-level check reads the master.edittoreadwith everything else. The per-plan rule count grows by four, and the e2e expectation is derived again from the plan's configuration (5 × 4 + 3 + 2 + 2 × 2 = 29).kpi_dept_reporter_setno longer declares create onkpi_bonus;kpi_hr_head_setkeeps no create.Everything the ruling asked to verify is proven by real position accounts, not the administrator: an HR reviewer registers a bonus (T65) and is refused when approving it (T66); an HR head approves it and the sheet's final score is recomputed 103 → 105 (T67) but cannot register (T68); the HR reviewer rejects and then approves at the HR review step (T69); and after the plan is closed the same account still reads the sheet but can no longer write (T70).
Review round 2 — the inline grid keeps every derived column
inlineColumnsreplaces the column set the platform derives from the child object, so declaring one just to surface the "adjusted" marker silently dropped six columns from the entry grid. All nineteen columns are declared again in field-definition order with the two new columns last; the three verbose ones are collapsed withdefaultHiddenrather than dropped. The built metadata was read back: 19 columns, 3 default-hidden, none missing, none unknown.planSharingIntents.Review round 3 — the tenant guard no longer hides organization-less plans
organization_idby equality in the candidate query; equality does not match NULL, and seeded or imported plans carry no organization, so those published plans were dropped before the decision ran. Candidates are now taken bystatus: 'published'alone and the tenant decision stays inside the pure function (both organizations present must match; either side missing falls back to the period comparison). A unit case pins the empty-organization candidate.dup=200) while T61 stays green; restoring the file returns the suite to 73/73.Left untouched deliberately: the concurrent-publish window on the period check, and the relationship between personal-item scores and the adjusted marker (recorded in the review).
Verification (commit 69a33c6, port 3110, database
.objectstack/issue-10.db)pnpm verify— validate ✓ (17 objects / 184 fields, 41 author-time rules), typecheck 0 errors, vitest 68/68.pnpm e2e— 73/73 pass, 0 fail (T58–T64 round 1, T65–T70 round 2, T71 round 3).dist/cleared and rebuilt; port 3110 the only bound app port; the listening process's cwd is the task worktree; worktree HEAD was the tested commit with a clean tree.docs/04statistics line reads 26 fully implemented, 1 deviation (no load test yet — escalated to the trial-run phase of the design's schedule), 0 unimplemented, and matches its status column.Does not touch
docs/00–docs/03,README.md,CLAUDE.mdor.github/(owned by the parallel item #9).Review gate: round 1 NOT MERGEABLE (1 should-fix), round 2 NOT MERGEABLE (1 new should-fix from the round-1 fix), round 3 MERGEABLE at 0252185; dispatcher collection check passed at every round.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb