diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index d97207a0..ce0880b6 100644 --- a/docs/agent-coordination.md +++ b/docs/agent-coordination.md @@ -708,4 +708,135 @@ handoff *visible and its failure loud* (§2), not a history feature. ## 6. How the found defects map onto the path -*(deferred — this is the re-rank, and it is explicitly not to be started yet)* +*(the re-rank. Written 2026-08-07 after the MVP was built and proven running, so it is ranked against +what the mechanism turned out to need, not against what it was predicted to need. `workspace-panel-todo.md` +is ordered by defect severity; this section says which of those defects the **objective** actually cares +about. Where the two disagree, this section wins for prioritisation and the todo keeps the severity note.)* + +### 6.1 The headline: most of the panel defect list is not on this path + +That is the finding, and it follows directly from Q1 and §1.16. **The mechanism is server-side; the +panels are a window onto it.** An agent's identity, its session, its transcript and its inbox all live in +Postgres and on the sidecar's disk. So a panel can remount, re-render, lose its React state, be dragged +across the dashboard, or not be rendered at all — and the work continues. Whole sections of the todo that +rank high on severity rank near-zero here. + +The corollary, and it is the useful half: the defects that *do* matter are almost all the same defect +wearing four hats — **a write that silently does not persist.** Panel identity is the one piece of +coordination state that lives in the layout jsonb rather than in a table of its own, so every silent +persistence failure in this list is now a path by which a panel forgets which agent it is. + +### 6.2 Tier A — on the critical path + +**A1. Stop swallowing persist failures.** (§1, third item — `state/src/useDashboardState.ts:46`, +`.catch(() => {})`.) *The single highest-value item in the whole list against this objective.* The +panel's agent name is written through this path. A swallowed 500 leaves the optimistic cache correct, so +the panel shows its name, answers to its name, and **forgets it on the next reload** — the failure is +invisible for exactly as long as the human is not looking, which is the entire window this project +exists to serve. §2 requires *loud failure*; this is the loudest silence in the codebase. + +**A2. The PATCH dispatcher's missing `else`.** (§2, first item.) The server half of A1. `ws-layout-*` is +matched today so panel `config` does persist — verified, the demo dashboard round-tripped with +`config: {agentName: …}` intact — but a chain of `if (…) continue` with no fallback means the *next* key +family added for coordination is a silent no-op that returns 200. Add the 400. + +**A3. Validate the layout on read, and fix the `'[]'` default.** (§4, items 2 and 3.) Panel `config` is +new unvalidated jsonb inside an already-unvalidated tree. `normalizeLayout` calling `.children.map` on a +stored `[]` throws, and with no error boundary (A4) that is a white screen where the dashboard should be. + +**A4. Add an error boundary around `WorkspaceRenderer`.** (§4, item 1 — the repo has none, anywhere.) +Ranked here for one specific reason: this is the **last mile of the whole objective.** Work proceeds +overnight; the owner opens the dashboard at 07:00 to see it. A malformed row turning that into a white +screen loses the result at the only moment it is consumed. The work itself is safe — it is in +`chat_session_events` — which is exactly why the recovery path must offer "restore default layout" +rather than requiring SQL. + +**A5. The resize-debounce items.** (§5.5, first two.) Both are lost-update bugs against the layout, and +the layout is now where a panel's identity lives. The 500 ms timer closing over a pre-drag tree does not +merely resurrect a deleted panel — it **re-writes an older `config`**, so a panel that was just named +reverts to anonymous. Concurrent same-key writes clobbering has the same effect from a window resize. +The prescribed fix (an updater-form `setValue` + cleanup) fixes identity loss as a consequence. + +**A6. Test `layout-utils.ts`.** (§9.) Promoted from "cheapest high-value target" to critical path, and +the reason is mine to own: `e588524` put agent identity inside those pure functions — +`setContents`, `swapPanels`, `movePanel`, `setPanelConfig`, `collectPanelConfigs`, `newPanelFrom` — and +shipped them with **no tests**. A regression in `swapPanels` now silently swaps two agents' identities +between panels, which is worse than any layout glitch it could previously cause. 214 lines of pure +functions over a serialisable tree; there is no excuse. + +**A7. Two windows must not disagree about the roster.** (§5.5, "the cache is never invalidated" — +`staleTime: Infinity`, no `invalidateQueries` anywhere, and every PATCH already returns a fresh state +blob the client throws away.) Q1 makes the dashboard *a window onto server-side work*. Two windows onto +the same work that permanently diverge, and neither told, is a direct contradiction of that. Cheap: +consume the response that is already being computed. + +### 6.3 Tier B — needed for the surface, not for the mechanism + +**B1. `dashboardId` is a bag whose format is parsed by its consumers.** (§5.9, third item.) Promote — +and note a hazard this document's own MVP introduced. The agent address book is keyed on `dashboardId`, +which is literally `workspace.key` (`WorkspaceView.tsx:163`) — so the demo dashboard's agents are stored +under `ws-layout-agent-mvp`. **Renaming a screen key, or changing that derivation, silently orphans +every agent row on that dashboard.** The rows survive, the sessions survive, and the panels can no longer +find them. The prescribed fix — give the context `{kind, id}` instead of the raw key — is now load-bearing +rather than tidy. Not Tier A only because it is stable today and the failure requires someone to change +the derivation. + +**B2. Panel lifecycle — but the ranking inverts.** (§5.1.) Against the terminal-orphan objective this was +"the highest-value change here." Against *this* objective the priority is the opposite one: **closing a +chat panel must never destroy the agent.** §5 Q2 records the hazard precisely — the explicit `disconnect` +path calls `clearClaudeSession`, which destroys the `sessionKey → claudeSessionId` pointer and orphans the +transcript, whereas idle reaping deliberately does not. So what is wanted from `onClose` here is a +*guarantee that nothing rides that path*, not an eager cleanup hook. Build the hook for the terminal by +all means; do not let a chat panel be wired into it without deciding that question first. A panel is a +pointer, and closing a window should not delete what it points at. + +**B3. `normalizeLayout` as framework, not convention.** (§5.4.) Matters for one consequence: a panel +whose appType is allow-listed but no longer in the registry renders, on a `locked` screen, as an +unrecoverable empty box. A chat panel in that state is a *visible* agent the human cannot reach — though +note its peers still can, because the mechanism does not go through the browser. Real, but a display +failure over a live agent rather than a lost one. + +**B4. The mobile collapse decision.** (§6 of the todo, first item.) Genuinely undecided against this +objective, and worth putting to the owner rather than guessing: *"I want to be able to sleep at night"* +raises the obvious question of whether the 03:00 check-in happens on a phone. If yes, a user-created +dashboard rendering only its left column forever is a Tier A problem wearing a mobile hat. If the answer +is "I check on the laptop, and mobile web is being retired for the native app" — which is what +`CLAUDE.md` currently says — it stays Tier C. **One question, one answer, large ranking swing.** + +### 6.4 Tier C — orthogonal to this objective, keep them ranked by severity + +Not dismissals. These are real defects and several are severe; they simply do not touch agents +coordinating with each other, and should be scheduled on their own merits rather than pulled forward by +this project. + +- **§1's Running Shells items and §5.1's pty follow-ups.** The terminal orphan leak is its own worthwhile + problem. It is not this one. +- **§3, multi-user correctness.** Ranks on its own timer (a second member creating a dashboard), which is + unrelated to this path. +- **§5.2, the remount table.** *The largest downgrade in this re-rank.* A remount used to threaten + whatever the panel was holding; a panel now holds nothing. A chat panel that remounts re-runs + `resume-cursor` from its stored cursor and replays the durable log — it costs latency, and §2 declares + latency free. Fix these for the interaction quality they are actually about; do not fix them for this. +- **§5.3, drag-to-move.** *The second-largest downgrade, and it was on the critical path when the north + star was written* (§1.5, P3: "dragging a panel would silently sever its session binding"). Two things + disarmed it. Q3 made the **name** the address and the panel id merely where it currently lives; and + `e588524` made `swapPanels`/`movePanel` carry `{appType, config}` as one unit, so the name travels with + the panel. `useAgentPanel` resolves by name and re-anchors the row's `panelId` afterwards. The + finish-or-delete decision is still open and still worth taking — it is ~180 lines of dead code — but it + is no longer gating anything. +- **§5.6 registry, §5.7 effect hygiene, §5.8 navigation, §5.10 channel hygiene, §8 dead code.** All + orthogonal. Two notes in passing: §5.10's observation that `preview:refresh` and `chat:active-session` + are written by `ChatPanelWrapper` and read by nobody survives the MVP rewrite unchanged — I kept both + writes rather than widen the diff. And §5.8 is not a prerequisite here; the mechanism never goes + through a channel, because it never goes through the browser at all. + +### 6.5 What the re-rank did *not* find, and that is the result + +No defect in `workspace-panel-todo.md` blocked building the MVP. It was built, and it ran unattended, on +the framework as it stands. The framework needed exactly one addition — per-panel config that survives a +move — and that is now `e588524`. + +So the honest summary is: **this list is not the bottleneck, and it should stop being treated as the +prerequisite queue for the objective.** Tier A is seven items, five of which are the same "a write +silently did not land" defect, and all seven are small. Everything else in the list should be scheduled +for its own reasons, on its own timetable, by severity — which is what the file was already ordered by. diff --git a/docs/workspace-panel-todo.md b/docs/workspace-panel-todo.md index e9a491e0..6ce02d6f 100644 --- a/docs/workspace-panel-todo.md +++ b/docs/workspace-panel-todo.md @@ -7,9 +7,27 @@ Move anything fully settled to §7. **How the framework actually works is documented separately, in `workspace-panels.md`** — read that first if you are new to it. This file is only the defect list and the work queue. -**Why any of it matters is in `agent-coordination.md`** — the north star. This list is currently ordered -by defect severity; it is to be **re-ranked against that objective**, and until that happens an item's -position here says nothing about its importance to the goal. +**Why any of it matters is in `agent-coordination.md`** — the north star. + +**This list stays ordered by defect severity. The re-rank against the objective lives in +`agent-coordination.md` §6** *(done 2026-08-07)*, and the two orderings deliberately disagree — read §6 +before deciding what to pick up next. Its conclusions in one paragraph: + +- **Tier A, on the critical path:** the swallowed persist failure (§1), the PATCH dispatcher's missing + `else` (§2), validate-on-read + the `'[]'` layout default + an error boundary (§4), the two + resize-debounce lost-update items (§5.5), the never-invalidated cache (§5.5), and tests for + `layout-utils.ts` (§9). Five of the seven are one defect — *a write that silently did not land* — and + they matter because panel identity now lives in the layout jsonb. +- **Tier B:** `dashboardId`-as-a-bag (§5.9) — the agent address book is keyed on it; panel lifecycle + (§5.1), but inverted: the requirement is that closing a chat panel must *not* destroy the agent; + `normalizeLayout` as framework (§5.4); and the mobile-collapse decision (§6), which swings on one + unanswered question. +- **Tier C, orthogonal:** everything else — including **§5.2 (remounts) and §5.3 (drag-to-move), both + large downgrades.** A panel is now a pointer to a server-side session, so a remount costs a replay, and + the drag hazard is disarmed by resolving identity by name (`e588524` + `bc82086`). +- **And the result that matters: no item in this file blocked building the MVP.** It was built and ran + unattended on the framework as it stands. Stop treating this list as the prerequisite queue for the + objective. Findings and full reasoning: `COMMS/workspace-panel-framework-analysis-2026-08-07.md`. Every `file:line` below was opened; DB claims were run against live `officer_dev`. Paths are relative to @@ -42,6 +60,10 @@ Both are two-line fixes, and without them you cannot tell whether any later fix - [ ] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is `.catch(() => { })`. Every 500 in this document is invisible because of it — the optimistic cache keeps the UI correct until reload. At minimum log; better, surface a toast and roll the cache back. + **Tier A1 — the single highest-value item in this file against the objective** + (`agent-coordination.md` §6.2). A panel's `config.agentName` is written through this path, so a + swallowed 500 leaves a panel that shows its name, answers to its name, and forgets it on reload: + invisible for exactly as long as nobody is looking, which is the window the whole project serves. --- @@ -58,6 +80,9 @@ Both are two-line fixes, and without them you cannot tell whether any later fix never killed. (Tmux's own state survives via `new-session -A -s off-`; the shell running `tmux attach` does not.) Two parts: add the three families, **and** add a fallback `else` that 400s on an unknown key. + **Tier A2** — the server half of A1. `ws-layout-*` *is* matched, so panel `config` persists today + (verified: the `agent-mvp` layout round-trips with `config: {agentName: …}` intact). The missing + `else` means the next key family added for coordination is a silent no-op returning 200. - [ ] **`ws-terminals-{id}: null` on a live dashboard is a 500.** Same file, `:61-66` — the `ws-layout-*` branch has a `value === null` → `deleteDashboard` case (`:42`); the terminals @@ -115,6 +140,12 @@ these. ## 4. Resilience — one bad row is currently a white screen +*The first three items are Tier A (`agent-coordination.md` §6.2, A3–A4). This is the last mile of the +objective: work proceeds overnight, and the human's only act is to open the dashboard and look. A +malformed row turning that into a white screen loses the result at the one moment it is consumed — and +`config` added a new unvalidated field to an already-unvalidated tree. The work itself is safe in +`chat_session_events`, which is why recovery must offer "restore default layout" rather than SQL.* + - [ ] **Add an error boundary.** `grep -rln "componentDidCatch\|getDerivedStateFromError\|ErrorBoundary\|errorElement" src` returns **nothing** across the whole repo. Wrap `WorkspaceRenderer` at minimum, with a reset that offers "restore default layout". Today the only recovery from a malformed stored layout is SQL. @@ -168,6 +199,13 @@ these. remount."* It closes the terminal orphan leak as a consequence rather than as a special case, and the same gap affects every panel holding a server-side resource. + **The requirement inverts for a chat panel** *(2026-08-07)* — do not wire one into this hook + without deciding first. `agent-coordination.md` §5 Q2: idle reaping deliberately leaves the + `sessionKey → claudeSessionId` pointer intact, but the explicit `disconnect` path calls + `clearClaudeSession`, which destroys it and orphans the transcript. A panel is a pointer to a + server-side session; **closing the window must not delete what it points at.** What coordination + wants from `onClose` is a guarantee that nothing rides the disconnect path, not an eager cleanup. + - [ ] **Then: kill the pty on real close.** Once the hook exists, `TerminalWrapper` / `CommandTerminalWrapper` / `HostTerminalWrapper` can `DELETE /terminal/_officer/sessions/:id` and drop the map entry — the thing they each explain they cannot currently do. @@ -181,6 +219,12 @@ these. ### 5.2 Stop the avoidable remounts +*Tier C — **the largest downgrade in the re-rank** (`agent-coordination.md` §6.4). A remount used to +threaten whatever the panel was holding; a panel now holds nothing. A chat panel that remounts re-runs +`resume-cursor` against the durable log and replays, costing latency, which §2 of the north star +declares free. Fix these for the interaction quality they are genuinely about — scroll position, media +playback, transcodes — not as a prerequisite for agent coordination.* + A panel's React identity is its position plus `key={child.node.id}` on its **nearest ancestor group slot** (`WorkspaceRenderer.tsx:178,204`) — the panel's own id is a key nowhere. That's the root cause of the whole table: @@ -213,9 +257,14 @@ the whole table: `DragOverlay.tsx` (99 lines, imported by nothing), `LayoutEditor.tsx` (51, imported by nothing), `movePanel` + `insertPanel` + `DropPosition` (`layout-utils.ts:142-169`), and three context fields. ~180 lines. - **If finishing it:** `movePanel` currently mints a *brand-new* panel id (`:151-154`) and carries - only `appType`, so it destroys all panel-keyed state and **silently drops `fitContent`**. Fix both - before re-enabling. + **If finishing it:** `movePanel` mints a *brand-new* panel id, so it destroys all panel-keyed + state, and `PanelContents` (`layout-utils.ts:229`) carries only `{appType, config}` — so a move + **silently drops `fitContent` and `zoom`**. Fix before re-enabling. + **Downgraded 2026-08-07** — this was on the critical path in `agent-coordination.md` §1.5 (P3: + "dragging a panel would silently sever its session binding") and no longer is. Q3 made the agent's + *name* the address and the panel id merely where it lives, and `e588524` made move and swap carry + `config` with the panel. `useAgentPanel` resolves by name and re-anchors the row afterwards. Still + worth deciding — it is dead code — but it gates nothing. See `agent-coordination.md` §6.4. ### 5.4 Make `normalizeLayout` framework, not convention @@ -237,6 +286,12 @@ the whole table: ### 5.5 Persistence hygiene +*Three Tier A items live here (`agent-coordination.md` §6.2, A5 and A7). The two lost-update bugs are +worse than described now that identity is in the layout: a debounce timer closing over a pre-drag tree +does not just resurrect a deleted panel, it **re-writes an older `config`**, so a panel that was just +named reverts to anonymous. And the never-invalidated cache means two windows onto the same server-side +work disagree permanently about the roster, with neither told — a direct contradiction of §5 Q1.* + - [ ] **The resize debounce can resurrect a deleted panel.** `WorkspaceRenderer.tsx:108-124` holds a 500 ms timer in a ref with **no `useEffect`, therefore no cleanup**, and its callback closes over the layout as it was when the drag began. Drag a splitter, remove a panel within 500 ms → the timer @@ -351,7 +406,11 @@ touch the framework half, so the abstraction holds in one direction; the leak is (`WorkspaceView.tsx:163`). Consumers reverse-engineer meaning from its shape: `Chat/ChatPanelWrapper.tsx:51-56` does `dashboardId === 'email' || dashboardId === 'screens/email'` → email context, and `!startsWith('screens/')` → dashboard context — **so renaming a screen key - silently changes the agent's system context**. `TerminalWrapper.tsx:13-16` regexes it; + silently changes the agent's system context**. **Worse since 2026-08-07:** the agent address book + (`agent_panels.dashboard_id`, via `useAgentPanel`) is keyed on this same string, so changing the + derivation orphans every named agent on that dashboard — the rows and sessions survive and the + panels can no longer find them. Promoted to Tier B in `agent-coordination.md` §6.3. + `TerminalWrapper.tsx:13-16` regexes it; `HostTerminalWrapper.tsx:12` doesn't (§2). Give the context the parsed facts (`{ kind: 'screen'|'dashboard', id }`) instead of the raw key, and the three parsers collapse. - [ ] **`WorkspaceLayout.tsx:36` silently omits `root`, `initialFilePath`, `defaultFileSort`** — apps @@ -413,6 +472,15 @@ Cheap to fix, and prerequisites for the 5.8 migration rather than alternatives t *(move items here with the commit and a one-line resolution)* +- [x] **Panels can carry per-panel settings, and keep them through a move.** *(`e588524`, branch + `agent-coordination-mvp`)* — `LayoutPanel.config`, opaque to the framework, exposed as + `usePanelConfig(panelId)`. The substantive half is in `layout-utils.ts`: `swapPanels` and + `movePanel` now carry `{appType, config}` as one unit via a `PanelContents` type, where before + they carried only `appType` and therefore reset *any* per-panel state on a drag. This is the one + framework change the agent-coordination MVP needed; it is what lets a panel remember which named + agent it is. Follow-ons: `PanelContents` still drops `fitContent` and `zoom` (§5.3), and the + mutators are still untested (§9). + --- ## 8. Dead code sweep @@ -445,3 +513,8 @@ Low priority, but each line here is a line someone will read and believe. is 214 lines of pure functions over a serialisable tree — the cheapest high-value test target in the codebase, and every item in 5.2 and 5.5 is a regression test waiting to be written. Start here before the mutator work, not after. + **Promoted to the critical path 2026-08-07** (`agent-coordination.md` §6.2, A6). `e588524` put + *agent identity* inside those functions — `setContents`, `swapPanels`, `movePanel`, + `setPanelConfig`, `collectPanelConfigs`, `newPanelFrom` — and shipped them with no tests. A + regression in `swapPanels` now silently swaps two agents' identities between panels, which is a + worse failure than any layout glitch the same bug could previously cause.