Files
F-r-ldraledigplannerare/PLAN_WARNINGS_HANDOFF.md
jacobh2 21f27b29a5 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014jPFUcCJ5AKXmPVC1anhXe
2026-09-20 14:27:06 +02:00

6.8 KiB
Raw Permalink Blame History

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.