record what landed: A1, A2, the tests, and what tier A has left

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-07 08:57:13 +00:00
co-authored by Claude Opus 5
parent f4ed7401da
commit 30eef86972
+69 -7
View File
@@ -18,6 +18,11 @@ before deciding what to pick up next. Its conclusions in one paragraph:
resize-debounce lost-update items (§5.5), the never-invalidated cache (§5.5), and tests for 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 `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. they matter because panel identity now lives in the layout jsonb.
**Done as of 2026-08-07:** §1's `.catch(() => {})` (`70c2f08`), all of §2 and §3's missing `userId`
predicate (`f4ed740`), §9 (`85452d1`). **Left in Tier A: §4 entirely** — validate-on-read, the `'[]'`
layout default, and the error boundary — **and §5.5's two debounce items plus the stale cache.** §4 is
now the largest remaining piece and the one the objective actually ends on: work proceeds overnight and
the human's only act is to open the dashboard and look at it.
- **Tier B:** `dashboardId`-as-a-bag (§5.9) — the agent address book is keyed on it; panel lifecycle - **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; (§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 `normalizeLayout` as framework (§5.4); and the mobile-collapse decision (§6), which swings on one
@@ -57,19 +62,32 @@ Both are two-line fixes, and without them you cannot tell whether any later fix
field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field that distinguishes an field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field that distinguishes an
orphan (`clients: 0`) from a live shell. Add it to the type and render it. orphan (`clients: 0`) from a live shell. Add it to the type and render it.
- [ ] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is - [x] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is
`.catch(() => { })`. Every 500 in this document is invisible because of it — the optimistic cache `.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. 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** **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 (`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: 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. invisible for exactly as long as nobody is looking, which is the window the whole project serves.
**Resolved `70c2f08`** — both catches now `console.error` and raise a toast naming the key, and
roll the cache back. The rollback is a **compare-and-swap**: it only reverts if the cache still
holds exactly what that call wrote. Writes to one key overlap freely (a window resize fires one
per group), and a blind rollback over a later successful write would turn one failure into two.
**Still swallowed, and not through this hook:** the three direct `client.patch('/dashboards', …)`
calls in `apps/Dashboards/DashboardListApp.tsx:93` and `DashboardPreview.tsx:323,326,356` — dashboard
create, rename and delete. Same defect, different call site; they bypass `useDashboardState`
entirely. A failed delete there leaves a dashboard that reappears on reload.
--- ---
## 2. Live data loss ## 2. Live data loss
- [ ] **The PATCH dispatcher silently drops three key families in active use.** **All four resolved in `f4ed740`** — they were one defect with four faces, so they were fixed as one
change. Each item keeps its diagnosis below and records what was done. Verified against the live server:
an unknown key 400s, the three prefixes round-trip through a GET, a null on a live dashboard is a no-op,
and the rename sequence leaves `workspaces` with no zombie.
- [x] **The PATCH dispatcher silently drops three key families in active use.**
`servers/api/dashboards/dashboards.ts:26-88` is a chain of `if (match) { …; continue; }` that ends `servers/api/dashboards/dashboards.ts:26-88` is a chain of `if (match) { …; continue; }` that ends
with **no `else`** — unmatched keys are dropped and the request returns 200 with a fresh state blob. with **no `else`** — unmatched keys are dropped and the request returns 200 with a fresh state blob.
Matched: `workspaces`, `ws-layout-*`, `ws-terminals-*`, `ws-host-terminals-*`, `screens/*`. Matched: `workspaces`, `ws-layout-*`, `ws-terminals-*`, `ws-host-terminals-*`, `screens/*`.
@@ -83,13 +101,23 @@ Both are two-line fixes, and without them you cannot tell whether any later fix
**Tier A2** — the server half of A1. `ws-layout-*` *is* matched, so panel `config` persists today **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 (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. `else` means the next key family added for coordination is a silent no-op returning 200.
**Resolved.** The three maps live in a new `dashboards.panel_state` jsonb bag keyed by prefix
(`{tmux: {panelId: sessionId}, …}`), with `dashboard_defaults.panel_state` for panels on a screen.
A bag rather than three columns because the fourth prefix should not need a schema change; an
allow-list rather than a regex because `ws-<prefix>-<id>` cannot be split without one — both halves
may contain dashes. `PANEL_STATE_PREFIXES` in `servers/api/dashboards/dashboards.ts` must stay in
step with the `statePrefix` props in `apps/Terminal/index.tsx`, and **the new fallback `else` — a
400 naming the key — is what tells you when it does not.** Living on the dashboard row also means
the maps are deleted with it.
- [ ] **`ws-terminals-{id}: null` on a live dashboard is a 500.** Same file, `:61-66` — the - [x] **`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 `ws-layout-*` branch has a `value === null``deleteDashboard` case (`:42`); the terminals
branches do not. A null falls to the UPDATE branch and sets a `NOT NULL` column branches do not. A null falls to the UPDATE branch and sets a `NOT NULL` column
(`databases/officer_db/src/queries/dashboards.ts:70`) → 23502. (`databases/officer_db/src/queries/dashboards.ts:70`) → 23502.
**Resolved.** A null on either terminals branch is now a no-op: it means "forget this key", and it
only ever arrives paired with `ws-layout-{id}: null` on a rename, by which point the row is gone.
- [ ] **`HostTerminalWrapper` never strips the prefix, so it mints phantom dashboards.** *(found - [x] **`HostTerminalWrapper` never strips the prefix, so it mints phantom dashboards.** *(found
2026-08-07, latent — `dashboards` is still 0 rows)* 2026-08-07, latent — `dashboards` is still 0 rows)*
`apps/Terminal/HostTerminalWrapper.tsx:12` is `` `ws-host-terminals-${dashboardId}` `` with **no `apps/Terminal/HostTerminalWrapper.tsx:12` is `` `ws-host-terminals-${dashboardId}` `` with **no
regex**, while its sibling `TerminalWrapper.tsx:13-14` correctly matches `^ws-layout-(.+)$` first. regex**, while its sibling `TerminalWrapper.tsx:13-14` correctly matches `^ws-layout-(.+)$` first.
@@ -101,13 +129,22 @@ Both are two-line fixes, and without them you cannot tell whether any later fix
Two apps deriving a state key from the same string by two different rules is the actual defect; Two apps deriving a state key from the same string by two different rules is the actual defect;
the id-shaped-string-as-a-bag problem behind it is §5.9. the id-shaped-string-as-a-bag problem behind it is §5.9.
Latent only because `officerdev/terminal-host` is in no default layout and hidden from the picker. Latent only because `officerdev/terminal-host` is in no default layout and hidden from the picker.
**Resolved.** All three wrappers now call `terminalStateKey(prefix, dashboardId)`
(`apps/Terminal/state-key.ts`) — `TerminalWrapper`'s rule, extracted, with the reasoning beside it.
Belt and braces on the server: the terminals and panel-state branches use a new `updateDashboard`
that will not INSERT, and **404 on an unknown id instead of creating one**. §5.9 is still the real
fix; this closes the hole a mis-derived key could fall through.
- [ ] **Renaming a dashboard resurrects it as a zombie row.** - [x] **Renaming a dashboard resurrects it as a zombie row.**
`apps/Dashboards/DashboardPreview.tsx:316-318` PATCHes `ws-layout-old: null` *and* `apps/Dashboards/DashboardPreview.tsx:316-318` PATCHes `ws-layout-old: null` *and*
`ws-terminals-old: null` together. The first deletes the row; the second then calls `ws-terminals-old: null` together. The first deletes the row; the second then calls
`upsertDashboard`, finds nothing, and **re-INSERTs it** with `name = id`. The old slug reappears in `upsertDashboard`, finds nothing, and **re-INSERTs it** with `name = id`. The old slug reappears in
`workspaces` on the next GET as a duplicate. Fixed by the item above, but verify this specific `workspaces` on the next GET as a duplicate. Fixed by the item above, but verify this specific
sequence after fixing. sequence after fixing.
**Resolved, and verified as the specific sequence:** create `zombie-test`, then PATCH
`{ws-layout-renamed: …, ws-layout-zombie-test: null, ws-terminals-zombie-test: null,
ws-host-terminals-zombie-test: null}` in one body. `workspaces` comes back as
`['agent-mvp', 'renamed']`.
--- ---
@@ -129,12 +166,17 @@ these.
Fix: composite PK `(user_id, id)`, or uuid ids. Composite PKs have a known drizzle re-diff quirk Fix: composite PK `(user_id, id)`, or uuid ids. Composite PKs have a known drizzle re-diff quirk
(see `databases/CLAUDE.md` → "Composite keys") — harmless churn, but read the plan. (see `databases/CLAUDE.md` → "Composite keys") — harmless churn, but read the plan.
- [ ] **`upsertDashboard`'s UPDATE has no `userId` predicate.** - [x] **`upsertDashboard`'s UPDATE has no `userId` predicate.**
`databases/officer_db/src/queries/dashboards.ts:73` — `databases/officer_db/src/queries/dashboards.ts:73` —
`db.update(dashboards).set(set).where(eq(dashboards.id, id))`. The `existing` lookup above it *is* `db.update(dashboards).set(set).where(eq(dashboards.id, id))`. The `existing` lookup above it *is*
scoped, so it cannot reach another user's row today, but it is a non-transactional read-then-write. scoped, so it cannot reach another user's row today, but it is a non-transactional read-then-write.
**It becomes a live cross-user overwrite the moment the PK above is made composite.** **It becomes a live cross-user overwrite the moment the PK above is made composite.**
Do both in one change or the first fix opens the second. Do both in one change or the first fix opens the second.
**Resolved `f4ed740`, incidentally.** `upsertDashboard` is now `updateDashboard` (scoped by
`(userId, id)`, `RETURNING` to say whether it matched) with an INSERT only when that returns
nothing — so the read-then-write is gone as well as the missing predicate. **The composite-PK item
above is still open**, and is still the one that matters; this just no longer opens a second hole
when it lands.
--- ---
@@ -481,6 +523,16 @@ Cheap to fix, and prerequisites for the 5.8 migration rather than alternatives t
agent it is. Follow-ons: `PanelContents` still drops `fitContent` and `zoom` (§5.3), and the agent it is. Follow-ons: `PanelContents` still drops `fitContent` and `zoom` (§5.3), and the
mutators are still untested (§9). mutators are still untested (§9).
- [x] **A write that does not land no longer looks like one that did.** *(`70c2f08` + `f4ed740`, branch
`agent-coordination-mvp`)* — Tier A1 and A2 of the re-rank, and between them the whole of §2 and
the second half of §3. Client: `useDashboardState` rolls back and toasts instead of `.catch(() => {})`.
Server: the PATCH dispatcher gained the three missing key families, a 400 on anything unmatched, a
404 rather than an INSERT for an unknown dashboard id, and a null that means "forget" rather than
"write NULL into a NOT NULL column". Framework: one rule for deriving a terminal's state key
(`apps/Terminal/state-key.ts`) where there had been three.
The remaining member of that family is the three direct `client.patch('/dashboards', …)` calls in
`apps/Dashboards/` — see §1.
--- ---
## 8. Dead code sweep ## 8. Dead code sweep
@@ -509,7 +561,7 @@ Low priority, but each line here is a line someone will read and believe.
## 9. Tests ## 9. Tests
- [ ] **There are none.** No `*.test.*` exists under any `Workspace*` or `*Screen*` path. `layout-utils.ts` - [x] **There are none.** No `*.test.*` exists under any `Workspace*` or `*Screen*` path. `layout-utils.ts`
is 214 lines of pure functions over a serialisable tree — the cheapest high-value test target in 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 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. before the mutator work, not after.
@@ -518,3 +570,13 @@ Low priority, but each line here is a line someone will read and believe.
`setPanelConfig`, `collectPanelConfigs`, `newPanelFrom` — and shipped them with no tests. A `setPanelConfig`, `collectPanelConfigs`, `newPanelFrom` — and shipped them with no tests. A
regression in `swapPanels` now silently swaps two agents' identities between panels, which is 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. worse failure than any layout glitch the same bug could previously cause.
**Resolved `85452d1`** — `layout-utils.test.ts`, 47 tests, 99.56% lines / 100% functions, the
first test under any `Workspace*` path. It paid for itself immediately: `setApp` was preserving
`config` whenever the new `appType` was non-null, so switching a panel from chat to terminal handed
the terminal the chat's `{agentName}` to read as its own settings — the exact leak the comment
beside it claimed to prevent. Not reachable through the UI (the only route out of an app is
`onClearApp` → `null`), but `setApp` is exported from the barrel. Fixed in the same commit.
Two tests are deliberately written to the *current* behaviour and marked `KNOWN GAP` so they fail
the day it changes: a move drops `zoom` and `fitContent` (§5.3).
**Still untested:** the other eleven files of the framework — `WorkspaceView`, `WorkspaceRenderer`,
`PanelSlot`, `normalizeLayout`. §5.5's debounce lost-updates live there, not in `layout-utils`.