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
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user