fix(T-071): agents return to their desks across a GL-loss rebuild instead of walking back in

docs/investigations/T-071-floor-reset.md

@@ -0,0 +1,96 @@
+# T-071 — the office floor resets when you switch agents
+
+## What was happening
+
+Settled before this change, and unchanged by it. Chromium caps live WebGL
+contexts per renderer process (~16) and **evicts the oldest** when a new one
+pushes past. The office floor's Pixi context is created at app startup, so it is
+always the oldest alive. Every terminal xterm opens takes another context via
+`@xterm/addon-webgl`. Switching agents opens terminals, crosses the cap, and the
+floor's context is the one that goes.
+
+Pixi reports nothing, so `glRecovery.ts` catches `webglcontextlost`, calls
+`preventDefault()` (without it the context never comes back) and bumps
+`glGeneration` — a dep of the scene effect at `OfficeFloor.tsx:1740`. The whole
+scene is torn down and rebuilt through the ordinary mount path. **That rebuild
+is the recovery working correctly**, and the workers never stop running; the
+problem was purely visual. Because every `Character` was constructed cold, each
+one reappeared at the office door and walked to a desk, and desks were re-dealt
+in whatever order the agents happened to be rebuilt in — so an agent could come
+back to somebody else's chair.
+
+## What changed
+
+`src/renderer/src/scene/office/sceneRestore.ts` (new) — a placement snapshot
+that crosses the rebuild.
+
+- **Capture.** The effect cleanup records, per agent, its seat index and the
+  tile it was actually standing on, into a `useRef`. A ref because the effect
+  *re-runs* on a rebuild: anything in its closure dies with the scene.
+- **Restore.** `addCharacter` seeds each agent from that snapshot instead of the
+  cold-start path — same desk, and `spawnTile` is where it already was rather
+  than the door. `applyState` then snaps it into its seat (`walkToDeskAndSit`
+  short-circuits when you are already on the desk tile) instead of walking it
+  there.
+
+The two halves degrade independently: a desk already claimed by an earlier agent
+this mount still leaves the position worth restoring, and a remembered tile that
+is not walkable on the current map still leaves the desk worth restoring. A
+theme change discards the snapshot outright — seat indices and tiles are indices
+into *one* map, and replaying them onto another would seat agents inside walls.
+
+Seat 0 is Michael's room by rule, so `agent.isGod` always goes through
+`claimSeat` and is never restored.
+
+The module takes plain numbers and predicates — no Pixi, no map renderer, no
+React — so the whole restore policy is testable against a `Map` literal with no
+browser and no GPU. Same seam `glRecovery.ts` already used.
+
+## What this does NOT fix
+
+- **It does not stop the eviction.** The floor still loses its context whenever
+  enough terminals are open, and the scene is still fully rebuilt. This removes
+  the annoyance, not the cause.
+- Everything except desk and position still resets: the coffee economy (clean-cup
+  stock, mugs parked on desks), break-room and errand state, in-flight message
+  envelopes, thought bubbles, and the task-board notes until the next 5s poll.
+- Options deliberately *not* taken, because they are Aaron's call, not a worker's:
+  dropping `@xterm/addon-webgl` (a global change to every user's terminal), or
+  trying to make the floor's context not be the oldest.
+
+## Residual jank
+
+- **A ~1.5s blank floor.** `DEFAULT_REBUILD_DELAY_MS` deliberately waits out the
+  eviction storm — claiming a context straight back just loses it to the next
+  terminal in the same burst. Not touched.
+- **A 0.5s fade-in.** `Character.show()` always fades from alpha 0
+  (`Character.ts:502`). Agents now *materialise at their desks* rather than
+  walking in, but they still fade. Removing that means changing `show()` for
+  every caller, which is not surgical enough to be worth it here.
+- An agent that was on a coffee break is restored at the café and then walks to
+  its desk — a short walk, not the walk-in.
+
+## Un-regressing the recovery
+
+The rebuild cap and the give-up-loudly path are the thing that must not break:
+going blank forever is far worse than an ugly rebuild. They are untouched, and
+`test/office-gl-recovery.test.cjs` (6 tests) still passes unchanged.
+
+The one new risk this change introduces is the capture itself: it runs inside
+the same cleanup that uninstalls the context-loss listener, against characters
+that are being destroyed. If it threw, it would take the cleanup down with it
+and leave a listener on a dead scene — a scene that can resurrect itself, which
+is strictly worse than the reset being fixed. So the uninstall runs **first and
+unconditionally**, the capture is wrapped, and `captureSceneSnapshot` is total by
+construction: a runtime with no character, a getter that throws, or a `NaN`
+position is skipped rather than propagated. `test/office-scene-restore.test.cjs`
+pins that.
+
+## Not verifiable here
+
+This was implemented in a headless worktree with no GPU and no running Electron
+app, so the *live* behaviour — actual WebGL context counts, and how the restored
+floor looks after a real eviction — was not measured. What would measure it: run
+`npm run dev`, open enough agent terminals to cross the cap (~16 contexts;
+DevTools logs `WARNING: Too many active WebGL contexts`), and confirm the floor
+comes back with agents at their desks instead of filing in through the door.

docs/investigations/T-071-qa.md

@@ -0,0 +1,176 @@
+# T-071 QA — the GL-rebuild scene restore, probed
+
+QA of `9f050849` on `agent/worker-t071-retry`. Adds `test/office-scene-restore-qa.test.cjs`
+(16 tests) alongside the author's `test/office-scene-restore.test.cjs` (13). No production
+code changed: **this branch is test-and-docs only.**
+
+Baseline is `main` @ `5300285f` = 594 of 595 (`provider-config.test.cjs` "model picker options
+stay provider-specific", already fixed in PR #9). This branch: **623 of 624, same single
+failure.** Typecheck 0 errors. `glRecovery.ts` re-verified untouched: 0 changed lines
+`5300285f..9f050849`, so the rebuild cap and the give-up-loudly path are un-regressed.
+
+The verdict on the four paths is **correct but untested, now tested** — every one behaves as
+the author claimed. What follows is what is now pinned, six findings that are real but were
+not defects to be manufactured, and an honest list of what a headless worktree cannot settle.
+
+## 1. The capture is total — the highest-severity risk
+
+The capture shares the effect cleanup with the context-loss uninstall, and the author is right
+that a listener stranded on a dead scene is worse than the reset. Both halves hold, and they
+hold independently:
+
+**The ordering.** `(a as any).__glRecovery?.()` is the first statement in the cleanup and is
+unconditional; the capture follows it, inside its own `try`. Nothing downstream of the
+uninstall can strand a listener. This is source structure, not behaviour, so it is pinned by a
+source-order assertion — it goes red when the two are swapped and when the `try` is removed
+(mutations M11, M12 below). A runtime proof would need OfficeFloor mounted under a test
+renderer with Pixi stubbed.
+
+**The totality.** Tried hard to make `captureSceneSnapshot` throw. Every per-entry shape is
+skipped cleanly: a `character` **getter** that throws (not just a throwing `getTilePosition`),
+a throwing `seatIndex` getter, a throwing `tile.x` getter, `getTilePosition` that is not
+callable, a character with no such method, a runtime that is `null`/`undefined`/a primitive, a
+null tile, a string coordinate, an `Infinity` coordinate, and a map key that throws when
+coerced to a property name. A floor of nothing but wreckage still returns a well-formed
+`{ theme, agents: {} }` — which matters, because the cleanup assigns the result to the ref
+unconditionally. (Since F2 it is *merged* onto the ref, so a well-formed empty capture is now
+absorbed rather than destructive — but it must still be well-formed.)
+
+## 2. Seat collision
+
+No ordering of restores double-claims a desk: three agents with remembered seats land on
+exactly their own desks in all three orders tried, with the claims set never exceeding three.
+`isSeatFree` is consulted live rather than cached, so a chair freed part-way through a mount
+is restorable by the agent that remembers it. A desk taken by a snapshot-less agent is not
+stolen back — the rememberer gives up the desk and keeps the position, which is the documented
+independent degradation.
+
+**Seat 0 holds, and it is worth knowing exactly why.** `restorePlacement` has no seat-0 rule of
+its own — it will hand seat 0 to anyone who remembers it. The invariant survives on two
+caller-side facts: `claimSeat` only ever deals seats from index 1, so no ordinary agent's
+snapshot can contain seat 0 to begin with; and the god's remembered *seat* is never restored.
+Both are now asserted, and both mutations (`i = 0`, restoring the god's seat) go red. Before the
+F1 follow-up the second fact was the stronger "the god is off the restore path entirely"; F1
+narrowed it to the seat so the god's position could be restored, which is all seat 0 ever needed.
+
+## 3. Snapshot lifetime
+
+Nothing stale ever leaks forward. Each mount builds its own `runtimes` Map and its cleanup
+closes over that one, so every teardown overwrites the ref; a snapshot from rebuild N cannot
+reach rebuild N+2. An `office → spaceship → office` round trip was checked explicitly: the only
+snapshot alive when office is rebuilt describes the spaceship map, and the theme tag rejects it.
+An agent archived between capture and rebuild leaves a dead entry that is never consulted
+(`syncAgents` only builds agents still in the store) and holds no claim, so its chair is simply
+free. An agent added between them has no entry and walks in cold.
+
+## 4. Degradation
+
+Both halves failing returns `{ seatIndex: null, spawnTile: null }` — an object, not `null`, and
+the caller treats it identically to a cold start: `claimSeat(agent)` for the desk and
+`restored?.spawnTile ?? entrance` for the position. So the both-fail case lands the agent at the
+office door with a freshly dealt desk. Not `0,0`, not off-map. A restored spawn tile is never
+one the floor called unwalkable, and it is a fresh object rather than an alias into the
+snapshot, so the first agent to move cannot corrupt the placement later agents restore from.
+
+## Findings
+
+None of these is a reason to hold the branch. F1 and F2 are the two worth a decision.
+
+**F1 — Michael still replays the walk-in on every rebuild. (medium; scope, not correctness.)**
+`agent.isGod ? null : restorePlacement(...)` excludes the god from *both* halves of the restore.
+Keeping seat 0 Michael's only requires excluding the **seat**; the **position** is orthogonal
+and safe to restore. So on every eviction the one agent most likely to be on screen marches in
+through the door — the exact jank this task exists to remove. Roughly:
+
+```ts
+const restored = restorePlacement(sceneSnapshotRef.current, officeTheme, agent.id, { ... });
+// seat 0 is Michael's by rule, and claimSeat is the one place that rule lives
+const restoredSeat = agent.isGod ? null : restored?.seatIndex;
+```
+
+**APPLIED** in the T-071 follow-up as `seedPlacement` in `sceneRestore.ts` — the god now goes
+through the restore and only his *seat* is nulled. Covered by `test/office-scene-restore-f1f2.test.cjs`;
+the seat-0 source guard in §2 above was retightened onto the new call site.
+
+**F2 — an eviction storm clobbers the snapshot with an empty one. (low-medium; narrow window.)**
+Characters are built asynchronously (`await theme.cast.getFrames`). If a second eviction tears
+the mount down before those resolve, the cleanup captures a `runtimes` Map that is still empty
+and writes `{ theme, agents: {} }` over the good snapshot — so the *next* rebuild is a full cold
+start and everyone walks in. This is a **loss**, not a leak; nothing stale survives. The window
+is narrow (`DEFAULT_REBUILD_DELAY_MS` is 1500ms and frames are usually cached) but this is
+precisely the storm the feature targets. Merging instead of replacing would close it, and is
+safe because a placement is only ever consulted for an agent still in the store:
+
+```ts
+const fresh = captureSceneSnapshot(officeTheme, runtimes);
+sceneSnapshotRef.current = fresh.theme === sceneSnapshotRef.current?.theme
+  ? { theme: fresh.theme, agents: { ...sceneSnapshotRef.current.agents, ...fresh.agents } }
+  : fresh;
+```
+
+**APPLIED** in the T-071 follow-up as `mergeSceneSnapshot` in `sceneRestore.ts`, called from the
+cleanup (still *after* the context-loss uninstall). A theme change discards outright, including
+when the fresh capture is empty.
+
+F3-F6 remain deliberately unapplied: documented, unreachable today.
+
+**F3 — a newcomer can be dealt a desk someone else remembers. (low; degrades gracefully.)**
+`claimSeat` does not know which seats are spoken for, so a snapshot-less agent built before a
+rememberer can take that rememberer's chair; the rememberer then falls back to a different desk
+but does keep its position. Closing it means reserving remembered seats before any
+`addCharacter` runs and releasing the unused ones afterwards — more than a one-liner, and the
+current behaviour is defensible.
+
+**F4 — `finiteInt` does not check integrality. (low; unreachable today.)** A `seatIndex` of
+`1.5` passes every guard; the caller would then `seatClaims.add(1.5)` and index `seatTiles[1.5]`
+as `undefined`, silently making the entrance the agent's "desk". Unreachable because `claimSeat`
+only ever yields integers. `Number.isInteger` for the seat index would make the name true.
+
+**F5 — the totality guarantee is per-entry, not per-iterable. (info; not a defect.)** The
+`for (const [id, rt] of runtimes)` header sits outside the `try`, so a throwing iterator or a
+non-destructurable entry would escape. Unreachable — the caller always passes a real
+`Map<string, Runtime>` — and harmless even then, because the uninstall has already run and the
+call site wraps the capture in `try/catch`. Deliberately **not** asserted with `assert.throws`:
+such a test would go red if someone later widened the guard, i.e. it would forbid an
+improvement.
+
+**F6 — an agent id of `__proto__` is dropped and repoints the prototype of `snapshot.agents`.
+(info; verified benign.)** Every inherited-key lookup yields a value whose `seatIndex`/`tile`
+fail the finite guards, so the caller falls through to `claimSeat` + entrance. Pinned by a test
+so it stays benign.
+
+## What a headless worktree cannot settle
+
+- **That the rebuild looks right** — that an agent materialises at its desk rather than at the
+  door, through the deliberate ~1.5s blank gap and the 0.5s `Character.show()` fade. Needs a
+  running Electron with a GPU: open terminals until Chromium evicts the floor's context and
+  watch the floor. This is the only thing that closes the task's actual acceptance criterion.
+- **The uninstall/capture ordering at runtime** rather than in source (guarded above, but by
+  reading the file). Needs OfficeFloor mounted under a test renderer with Pixi stubbed.
+- **That a remembered tile is genuinely re-occupiable** — that `mapRenderer.isWalkable` agrees
+  with what a real `Character` reports from `getTilePosition()` mid-walk, and that two agents
+  cannot be restored onto the same tile (the restore checks walkability, not occupancy; two
+  agents standing on one tile is cosmetic, but it is unchecked). Needs the real map and Character.
+- **Whether F2 and F3 bite in practice.** Needs the running app: force two evictions inside the
+  rebuild delay for F2, and spawn an agent during a rebuild for F3.
+
+## Break-it record
+
+Every test was proved to bite by mutating the thing it protects. 14 mutations, each reverted:
+
+| # | mutation | red |
+|---|---|---|
+| M1 | capture: drop the `try/catch` | totality (2 tests) |
+| M2 | capture: drop the `!tile` guard | totality |
+| M3 | capture: alias the tile instead of copying | author's "the snapshot is a copy" |
+| M4 | restore: drop `isSeatFree` | 3 seat-collision tests |
+| M5 | restore: drop the theme check | theme round trip |
+| M6 | restore: drop `isWalkable` | both-fail, wall spawn |
+| M7 | restore: alias the spawn tile | "the restored tile is a copy" |
+| M8 | restore: both-fail lands at `0,0` | both-fail, Object-key |
+| M9 | restore: move the seat-0 rule into the module | "no seat-0 rule of its own" |
+| M10 | restore: a missing / empty placement returns a default | 3 lifetime tests |
+| M11 | caller: capture moved BEFORE the uninstall | source-order guard |
+| M12 | caller: capture no longer inside a `try` | source-order guard |
+| M13 | caller: god no longer excluded from the restore | seat-0 contention |
+| M14 | caller: `claimSeat` deals from seat 0 | seat-0 contention |