From ecc7fb90d81d1af6a3cdb629ad2263f2339252c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 08:40:33 +0000 Subject: [PATCH] rank the panel defects against the objective, not against severity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-rank the north star deferred, done now that the MVP is built and running — so it is ranked against what the mechanism turned out to need. The finding is that most of the list is not on this path. The mechanism is server-side and a panel is a pointer to it, so a remount, a re-render or a drag costs a replay, not a session. Section 5.2 and 5.3 are large downgrades; 5.3 was on the critical path when the north star was written and is disarmed by resolving identity by name. What is left is small and mostly one defect wearing four hats: a write that silently does not land. Panel identity lives in the layout jsonb now, so the swallowed persist catch, the dispatcher's missing else and the two debounce lost-updates each become a panel that forgets which agent it is — invisibly, for exactly as long as nobody is looking. Also corrects two items the MVP made stale, and promotes layout-utils tests: e588524 put agent identity inside those mutators and shipped them untested. Co-Authored-By: Claude Opus 5 --- docs/agent-coordination.md | 133 ++++++++++++++++++++++++++++++++++- docs/workspace-panel-todo.md | 87 +++++++++++++++++++++-- 2 files changed, 212 insertions(+), 8 deletions(-) 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.