From fe705fe618d929e0b5ca472e292db88f72c4c616 Mon Sep 17 00:00:00 2001 From: Nessar Date: Sun, 18 Oct 2026 21:49:55 +0200 Subject: [PATCH] fix(planning): keep counterfactual moves off the shared final plan Since assigning a date shares unchanged activations with the parent state, counterfactual analysis edited an activation it shared with the final plan, so a later alternative date could inherit an earlier one's dates and be judged eligible. It now replaces the moved activation. Every counterfactual over seeds 280000-289999 matches the engine before the sharing step again. When every one of several obligation solutions fails earned-entitlement chronology, the planner again reports the obligation conflict, as when every preview failed. No seeded request reaches that path, so the frozen engine is unchanged. Also marks the ranking's known limit as a shortcut and fixes stale documentation found in review. Part of #302. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- docs/benchmark/README.md | 3 +- docs/benchmark/issue-302.md | 13 ++++- docs/planner-cancellation.md | 2 +- .../leave-planner-explanation.spec.ts | 47 +++++++++++++++++++ src/app/services/leave-planner.core.ts | 22 ++++++--- 6 files changed, 78 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ccc5353..94499ae 100644 --- a/README.md +++ b/README.md @@ -253,7 +253,7 @@ The workflow runs on branch/tag pushes, PR opening/reopening/synchronization/edi Jobs that run project commands install Bun 1.4.3 with `scripts/install-bun.sh` (a checksum-verified download, no npm) and run their steps with a `PATH` that holds Bun but no Node. The job containers keep Node only for Gitea's checkout and artifact actions, which are JavaScript actions the runner executes itself; see the [activation guide](docs/gitea-actions.md#runners-and-permissions). -Functional tests run with `bun test --isolate`; coverage is line-level only (Bun reports no branch or function data). The separate required benchmark job runs without coverage in a container limited to 3 GiB (JavaScriptCore has no heap-size flag, so this replaces the former 2 GiB Node heap cap) and keeps its 240-second hang guard, one warm-up and two measured repetitions of all seven scenarios. [#78](https://gitea.nessar.fr/nessar/vac-optimizer/issues/78) restores this gate after a complete local pass in 298.6 seconds; see [the measurements and hardware limits](docs/benchmark/issue-78.md). Benchmark success is required for image publication and the aggregate `required` job. The narrow local timing margin still needs confirmation on the CI runner. +Functional tests run with `bun test --isolate`; coverage is line-level only (Bun reports no branch or function data). The separate required benchmark job runs without coverage in a container limited to 3 GiB (JavaScriptCore has no heap-size flag, so this replaces the former 2 GiB Node heap cap) and keeps its 240-second hang guard, one warm-up and two measured repetitions of all seven scenarios. [#78](https://gitea.nessar.fr/nessar/vac-optimizer/issues/78) restored this gate after a complete local pass in 298.6 seconds; see [the measurements and hardware limits](docs/benchmark/issue-78.md). After [#302](docs/benchmark/issue-302.md) the local protocol takes about 41 seconds. Benchmark success is required for image publication and the aggregate `required` job. The lowered hang guard and tightened multipliers still need confirmation on the CI runners. The `required` job aggregates successful validation and expected image publication, accepting the image job's intentional skip for fork PRs and manual runs. Image jobs require successful validation and are limited to pushes and same-repository PRs. Registry credentials are confined to the publishing step. diff --git a/docs/benchmark/README.md b/docs/benchmark/README.md index 31e56f2..49b0d2d 100644 --- a/docs/benchmark/README.md +++ b/docs/benchmark/README.md @@ -6,7 +6,8 @@ unchanged Node acceptance protocol on the documented development laptop: **298.6 seconds**, including one warm-up and two measured repetitions of all seven scenarios, without coverage, with one worker and a 2 GiB Node heap. -The full protocol keeps a 240-second timeout only as a hang guard (see +After [#302](issue-302.md#benchmark-protocol), the Bun protocol takes about 41 +seconds, and its timeout is a 240-second hang guard (see [Relative CI gate](#relative-ci-gate)). All expected outcomes, placement counts, final stages and determinism checks pass. The fixture inputs are unchanged; the test now prints its existing report. diff --git a/docs/benchmark/issue-302.md b/docs/benchmark/issue-302.md index 0c8479f..7305bd6 100644 --- a/docs/benchmark/issue-302.md +++ b/docs/benchmark/issue-302.md @@ -6,6 +6,15 @@ identical to the frozen engine at `a871f8f`. The [frozen-engine differential check](README.md#frozen-engine-differential-check), including its cancelled asynchronous comparisons, passes after every step. +That check does not compare counterfactual analyses. Code review found that the +last exact step, which shares unchanged activations between states, let +counterfactual analysis edit an activation it shared with the final plan, so a +later alternative date could inherit an earlier one's dates. A follow-up commit +replaces that activation instead of editing it. Afterwards, every +counterfactual of every plan placement for seeds 280000 to 289999, 8,750 +analyses, matched the engine before that step; the same comparison found 4 +differences with the step alone. + ## Per-step measurements Each step was timed against its parent in the same session, on the development @@ -146,7 +155,9 @@ objectives then the tie-breaks of each obligation-only plan, with enumeration order breaking equal ranks. It runs the preference stages once, on the first solution. If that run ends with earned leave available only after its final merged time-off period, the stages restart on the next solution, and a -conflict is reported only when every solution fails. +conflict is reported only when every solution fails. With several solutions, +that is the obligation conflict the previews reported when every preview +failed. ### Timing diff --git a/docs/planner-cancellation.md b/docs/planner-cancellation.md index 1df8558..1509d6c 100644 --- a/docs/planner-cancellation.md +++ b/docs/planner-cancellation.md @@ -16,7 +16,7 @@ User cancellation is distinct from replacement by another calculation (`stale-ca The asynchronous calculation retains a copied incumbent as soon as the obligation search finds a jointly valid solution, before that entire stage has completed. It also considers accepted placements and every feasible evaluated quality candidate, including candidates rejected by a stage's narrower comparator. Each candidate is reconciled for earned entitlement availability and checked against all obligation rules before retention. The existing full lexicographic comparator chooses the best available incumbent. -Retention reuses quality metrics already computed for an evaluated candidate. An intermediate state with more unplaced at-risk leave, or worse preferences at equal unplaced leave, is rejected using that objective prefix alone. This avoids rescoring dominated states without skipping a candidate that could improve the retained plan. +Retention reuses the objective vector already computed for an evaluated candidate. An intermediate state with more unplaced at-risk leave, or worse preferences at equal unplaced leave, is rejected using that objective prefix alone. This avoids rescoring dominated states without skipping a candidate that could improve the retained plan. Preference occurrence dates and patterns are calculated once for the run's immutable calendar and rules. Each comparison still checks the candidate's actual placements against those patterns; candidate scores are not cached. diff --git a/src/app/services/leave-planner-explanation.spec.ts b/src/app/services/leave-planner-explanation.spec.ts index 881e642..2a35ef9 100644 --- a/src/app/services/leave-planner-explanation.spec.ts +++ b/src/app/services/leave-planner-explanation.spec.ts @@ -495,6 +495,53 @@ describe('planner explanation and alternative dates', () => { ).toBeUndefined(); }); + it('judges each alternative producer date against the unchanged final plan', async () => { + const calculation = createPlanningCalculation({ + horizon: { start: '2026-04-20', end: '2026-05-31' }, + balances: [balance('cp', 'cp', 2, '2026-12-31', '2026-04-20')], + rules: [ + { + id: 'friday', + type: 'if_then', + force: 'preference', + dayOfWeek: 5, + placeDays: 1, + placeType: 'cp', + placement: 'auto', + rewardDays: 1, + rewardType: 'rtt', + }, + { + id: 'consumer', + type: 'time_window', + force: 'obligation', + startDate: '2026-05-12', + endDate: '2026-05-12', + placeDays: 1, + placeType: 'rtt', + }, + ], + }); + const outcome = await calculation.outcome; + if (outcome.kind !== 'Plan') throw new Error('Expected a plan.'); + const producer = outcome.placements.find( + (placement) => placement.date === '2026-04-30' && placement.ruleIds.includes('friday'), + ); + expect(producer).toBeDefined(); + if (!producer) return; + + const analysis = await calculation.analyzeCounterfactual(producer); + if (analysis.kind !== 'Counterfactual') throw new Error('Expected a counterfactual.'); + // 2026-05-04 is evaluated first; the move to 2026-05-11 must not inherit its dates. + expect(analysis.results.find((result) => result.date === '2026-05-04')?.status).toBe( + 'eligible', + ); + expect(analysis.results.find((result) => result.date === '2026-05-11')).toMatchObject({ + status: 'ineligible', + reason: { code: 'unavailable' }, + }); + }); + it('rejects an at-risk move that would delay already-earned leave past its consumer', async () => { const calculation = createPlanningCalculation({ horizon: { start: '2024-04-25', end: '2024-05-08' }, diff --git a/src/app/services/leave-planner.core.ts b/src/app/services/leave-planner.core.ts index bd430fd..a3ca016 100644 --- a/src/app/services/leave-planner.core.ts +++ b/src/app/services/leave-planner.core.ts @@ -4523,7 +4523,7 @@ function* calculatePlanningSteps( ); let rankedSolutions = obligationSolutions; if (obligationSolutions.length > 1) { - // Rank obligation-only plans; the preference stages run once, on the best-ranked. + // shortcut: preferences run on the best-ranked obligation-only plan only, so a lower-ranked one that would end better is missed (ADR 0010); revisit if the comparison record grows. const ranked: { state: State; objectives: PlanningObjective[] }[] = []; for (const candidate of obligationSolutions) { yield; @@ -4585,6 +4585,9 @@ function* calculatePlanningSteps( yield true; return finish(checkpoint!, state); } + // As when every preview failed, several failing solutions report the obligation conflict. + if (rankedSolutions.length > 1) + return finish(obligationConflict(obligationRequirements, context)); return finish({ kind: 'PlanningConflict', conflictingRuleIds: state.earnedActivations.map((activation) => activation.ruleId), @@ -5109,13 +5112,18 @@ export function analyzeCounterfactualInternal( continue; } candidate.placements.get(date)!.supporting = selected.supporting; + // The candidate shares its activations with the baseline, so replace them rather than edit them. if (removeDate && removeDate !== date) - for (const activation of candidate.earnedActivations) { - if (!selected.ruleIds.includes(activation.ruleId)) continue; - activation.dates = activation.dates.map((activationDate) => - activationDate === removeDate ? date : activationDate, - ); - } + candidate.earnedActivations = candidate.earnedActivations.map((activation) => + selected.ruleIds.includes(activation.ruleId) + ? { + ...activation, + dates: activation.dates.map((activationDate) => + activationDate === removeDate ? date : activationDate, + ), + } + : activation, + ); const finalized = finalizeEarnedState(candidate, context); if (!finalized) { results.push({