From 21f27b29a5e9961f07463b5bd3d1715e60d41e8c Mon Sep 17 00:00:00 2001 From: Jacob Hagstedt Date: Sun, 20 Sep 2026 14:27:06 +0200 Subject: [PATCH] Hand off what the plan-warning fix left open Records why the heartbeat was rejected so it isn't re-litigated, the two verification paths the demo workspace couldn't exercise, and the design questions nobody has ruled on yet -- chiefly whether an evicting sync's proposal should be adopted at all, and that warnings still don't say whether the planner can actually resolve them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014jPFUcCJ5AKXmPVC1anhXe --- PLAN_WARNINGS_HANDOFF.md | 103 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 PLAN_WARNINGS_HANDOFF.md diff --git a/PLAN_WARNINGS_HANDOFF.md b/PLAN_WARNINGS_HANDOFF.md new file mode 100644 index 0000000..67d54c5 --- /dev/null +++ b/PLAN_WARNINGS_HANDOFF.md @@ -0,0 +1,103 @@ +# Handoff — plan warnings in step with the plan + +**Date:** 2026-09-20 · **Commit:** `b76fb74` *Keep plan warnings in step with the plan they describe* + +Historical context, not authoritative spec. Where this disagrees with the code, the code wins. + +--- + +## Why this change happened + +Warnings such as "semesterdagar riskerar att gå förlorade" kept appearing after the planner should +already have resolved them. The question on the table was whether to add a manual "run the planner" +button or put the planner on a ~5-minute heartbeat. + +**Neither was the fix, and the heartbeat was rejected outright.** The planner was already firing +correctly: `frontend/src/context/PlanSyncProvider.tsx` posts `/api/plan/sync` on every `planVersion` +bump, app-wide, coalescing bursts; `Calendar.tsx` calls `runSync` directly for its own mutations. A +heartbeat would have re-run an allocator that had already run, cleared no warning, and — because a +sync can come back `evictionPreview.wouldEvict` — could pop an eviction prompt with no user action +behind it. **Don't reintroduce it.** The warnings were simply wrong, for two independent reasons. + +## What was fixed + +1. **Warning computed before allocation** (`backend/app/services/allocation_service.py`, + `_Ledger._vacation_expiry_warnings`). It read `vacation_forfeiting`, which the loader derives from + *already-committed* usage, while `_assign_tier3` deliberately spends at-risk vacation days first to + avoid exactly that forfeiture — so it warned about days the plan was busy saving. It now subtracts + the vacation days this run actually placed for that `(parent, year)`, clamped at 0. Sound because + the bank is FIFO: a day spent in year Y draws from the oldest (expiring) entry, retiring one + forecast day 1:1. `AnalyticsService`'s carry-forward simulation is still **not** duplicated in the + ledger — keep it that way. +2. **Dashboard never refreshed them.** The proposal came from a one-shot effect guarded by + `proposal === null` with no `planVersion` dependency, while `syncPlan`'s response already carried a + fresh proposal that both call sites discarded. `PlanSyncProvider` now exposes `latestProposal` + + `latestProposalVersion` (monotonic counter), and `usePlanProposal` prefers it once any sync has + completed, keeping `propose()` for the first load. +3. **Manual recompute button** in Settings → "Kalender & språk". It only calls `bumpPlanVersion()`; + the provider does the sync. It is an escape hatch, not the fix. +4. **Partner add/remove now bump `planVersion`.** That gap meant gaining or losing a parent never + re-ran the allocator, though it changes who days can be assigned to. Rename stays cosmetic. + +Verified in the browser: a 2031 warning of 15 days dropped to 12 after three vacation days were +planned, with no page reload. Frontend 1338 tests pass (98.19% stmts / 92.47% branch); backend 893 +pass at 92%. + +--- + +## What is left + +### Verification gaps (nothing known broken — just not exercised live) + +- **The recompute button's success message was never seen in a browser.** The demo workspace's sync + always lands on the eviction branch, so only "inget sparades" was observed. The happy path + (`settings.recomputeSuccess`) and the 10 s timeout path (`settings.recomputeTimeout`) are covered + by unit tests only. Worth a run against a workspace with slack in its pools. +- **Add-partner was not exercised live.** The demo workspace already has a partner, and removing her + is blocked by the app's own data guard; forcing it would have meant destructive edits to a shared + dev DB. The `bumpPlanVersion()` calls were confirmed by source on the same path observed firing. + +### Design questions worth a decision + +- **A sync that evicts writes nothing, but we still adopt its proposal.** `PlanSyncProvider` stores + `result.proposal` even when `evictionPreview.wouldEvict` is true. The warnings then describe a plan + that was never persisted. Defensible (the proposal is still the honest answer to "what would the + planner do"), but it is a real semantic choice that nobody has ruled on. If it turns out to confuse + users, gate the adopt on `!wouldEvict`. +- **The recompute button infers its own result by watching a counter.** `Settings.tsx` captures a + baseline `latestProposalVersion`, waits for it to move past, and falls back to a 10 s timeout. That + exists only because `PlanSyncProvider` *logs* sync errors rather than exposing them. The cleaner + fix is an error/status field on the provider's context; then the button can await a real outcome and + `RECOMPUTE_TIMEOUT_MS` disappears. +- **Warnings don't say whether the planner can help.** Some forfeiture is structurally unresolvable — + `expiring_over_cap`, or simply more earned vacation than there are eligible days in any leave window + before expiry. The planner cannot invent leave days. Today those look identical to resolvable ones. + Splitting the message into "the planner will handle this" vs "you must add leave or accept the loss" + would stop the exact confusion that started this work. +- **The FIFO subtraction is an assumption with no guard.** It holds because a vacation day spent in + year Y always draws from the oldest bank entry. If the bank's ordering policy ever changes, the + subtraction silently over-credits and under-warns. There is no test pinning the FIFO property + itself, only its consequences. + +### Known stale, deliberately out of scope + +- **`useOptimizationTips` has the identical one-shot staleness shape** — `GET /api/tips` fetched once, + no `planVersion` dependency, so tips never refresh after a replan. Flagged in a comment in + `Dashboard.tsx`; not fixed. Same treatment as `usePlanProposal` should work. +- **Stale `member-action-error` banner** persists in the DOM after cancelling the remove-partner + confirmation. Pre-existing, cosmetic, untouched. +- **Demo-data leftovers:** three calendar entries (Jacob · Semester, 1–3 Sep 2031) were added during + browser verification and left in place — removing them is itself a live-data mutation. + +### Repo state at handoff + +- Committed **on `main`**, not a branch: another multi-agent setup was working in this same tree, and + `git checkout -b` would have moved HEAD under them and pulled their commits onto my branch. +- That setup's in-flight work was left entirely unstaged. Three lines of theirs + (`settings.partnerActionErrorMaxReached`, in `Settings.tsx` and both `i18n/*/settings.ts`) were + interleaved with mine in files I had to commit; they were excluded from the index by staging a + filtered blob, so the working tree still holds them, unstaged and intact. +- `backend/tests/unit/test_planning_service_sync.py::TestSyncPreservesPromotedLockedWish` fails. It is + **theirs** — a new red test (135 uncommitted lines) for a pinned-day double-debit bug in + `planning_service`. This change touches only warning output, nothing in the funding path. Don't + "fix" it.