Umbrella: V2 architecture refactor — shell primitives, composition root, state reactivity, transport adapter

[BorisTyshkevich]

docs/ADR-0004-ui-shell.md decided, on 2026-08-03, to reject the Preact migration (#578, moot) and instead invest in a small set of shared vanilla shell primitives, since the measured evidence showed the Preact treatment costing +330 lines (+26% over a fair vanilla control) and +7.7 KB gzip with no simplification win. That decision opened four refactor issues (#586, #587, #588, #589) plus a separate transport-layer proposal (#585) that this umbrella sequences alongside two more issues surfaced by the same 2026-08-03 review (#590, #591) and a lock-in issue (#592) into one ordered execution plan for the /ship skill: one PR per phase, one session per phase, with a human merge gate between every phase.

Phases

• Phase 1 — refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586: SurfaceLifecycle primitive + docked right-inspector slot. Collapses six copy-pasted overlay lifecycles into one, and gives .main-row a real inspectorHost slot — unblocks Unify cell detail, rows and documentation in a foldable desktop right inspector #488 (foldable right inspector), which is built explicitly on top of this issue's skeleton.
• Phase 2 — refactor(shell): a side-panel registry so adding a navigation panel is a one-file change #587: side-panel registry, so adding a left-nav panel is a one-file change instead of touching the shell in five-plus places. Unblocks Add a drag-foldable desktop left navigation with rail and focused drawers #487 (left navigation); salvages the proven nav-sections.ts registry design from feat/nav-section-registry-487p2 (PR feat(#487): navigation section registry (phase 2) #573) rather than re-deriving it.
• Phase 3 — refactor(state): fail-closed decoders for persisted domain records #591 (fail-closed decoders): small, independent, low-risk — a good early win that doesn't depend on or block anything else in this sequence.
• Phase 4 — refactor(app): decompose the composition root along four seams #588: decompose the composition root (src/ui/app.ts, createApp spans ~3,000 lines) along four seams, plus a fifth extraction replacing the as App cast with typed staged construction.
• Phase 5 — refactor(dashboard): extract the tile gesture controller and a pure repaint plan #589: extract the dashboard tile gesture controller and a pure repaint plan out of renderDashboard's one 46-nested-function closure.
• Phase 6 — refactor(state): make the committed workspace aggregate reactive and retire dashboardTreeRevision #590 (reactive workspace aggregate): retire dashboardTreeRevision by making the committed workspace aggregate itself reactive. Sequenced after refactor(app): decompose the composition root along four seams #588 so this touches a smaller composition-root surface once the workspace-persistence extraction (workspace-session.ts) has already moved out of app.ts.
• Phase 7 — ADR-0005: adopt @clickhouse/client-web behind the SQL Browser transport adapter #585 (ADR-0005): @clickhouse/client-web transport spike. Independent of the shell track — has its own validation gates (precision, streaming, auth/epoch, cancellation, bundle) and may conclude "Rejected"; that outcome still completes the phase, it just means the repository keeps the current separated custom transport from Phase 1 of that issue's own delivery plan.
• Phase 8 — build(guardrails): extend check-boundaries to lock in the shell primitives #592 (guardrail lock-in): extend build/check-boundaries.mjs to mechanically forbid the six-copies-of-an-overlay problem from regrowing. Runs last because it locks in what phases 1–2 established.

Ground rules

• Each phase is exactly one PR.
• A phase's own issue body (Goal/Context/Tests/Acceptance criteria) is the contract for that PR — this umbrella does not restate or override it.
• Every phase must leave main green under the full gate set: npm test and tsc --noEmit and npm run check:arch (check-boundaries.mjs). Note: this repo's npm test alone does not run pretest in every invocation context, so npm test passing by itself is not sufficient evidence — run the full gate explicitly.
• No phase starts before the previous phase has merged.
• Add a drag-foldable desktop left navigation with rail and focused drawers #487 and Unify cell detail, rows and documentation in a foldable desktop right inspector #488 are feature work, not refactor work, and are deliberately not part of this umbrella — they follow on the primitives this umbrella delivers (refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 for Unify cell detail, rows and documentation in a foldable desktop right inspector #488, refactor(shell): a side-panel registry so adding a navigation panel is a one-file change #587 for Add a drag-foldable desktop left navigation with rail and focused drawers #487) once those primitives exist.

Related

• ADR-0004 (docs/ADR-0004-ui-shell.md)
• Roadmap to V2 (professional UI redesign) #582 — V2 roadmap (this umbrella sequences the roadmap's refactor track)
• Add a drag-foldable desktop left navigation with rail and focused drawers #487 — left navigation (follow-on feature work, not in this umbrella)
• Unify cell detail, rows and documentation in a foldable desktop right inspector #488 — right inspector (follow-on feature work, not in this umbrella)
• createState decodes a corrupt editorPct/sideSplitPct/cellDrawerPx/docPanePx to NaN #570 — CLOSED (Not Planned); same defect family as refactor(state): fail-closed decoders for persisted domain records #591 but not resumed by it

[BorisTyshkevich]

Ship log — #593

Branch model: one PR per phase off fresh main, phases strictly sequential, with a human
merge gate between every phase.

This is exactly what this umbrella's ground rules specify ("Each phase is exactly one PR",
"No phase starts before the previous phase has merged").

Run mode history:

• Phase 1 ran attended. (It began as /ship 593 unattended; the owner switched it to
  attended before phase 1 reached its gate, so phase 1 stopped for explicit merge approval and
  its ChatGPT review was a second opinion that stopped early on a clean pass.)

• Phases 2–8 run unattended (/ship 593 unattended, owner-invoked 2026-08-04) as
  coordinator + per-phase worker subagents — but with the merge step withheld from the
  coordinator. Owner instruction, same day, after phase 2's plan was settled: "stop at merge
  gate after posting PR and running all chatgpt reviews. I will merge myself."

  So each phase runs unattended through plan → implement → review → PR → the full three-pass
  ChatGPT sequence (all three passes, not stopping early on a clean one, per the unattended
  contract), and then stops. The coordinator never runs gh pr merge. The owner merges.
  The unattended automatic-merge proof therefore does not apply; what the coordinator owes at
  the gate is the PR link, the three ChatGPT comment URLs, the findings-applied/dismissed
  disposition, the head SHA, and CI state.

  Because this umbrella's ground rules also say "no phase starts before the previous phase has
  merged", the run genuinely pauses at every phase gate until the owner merges — 7 gates, not 1.

This umbrella's ground rules still win over the unattended default. references/unattended.md
would normally use one integration branch and one PR for a whole run, but it justifies that only
by the absence of a human to merge intermediate PRs — and in unattended mode the coordinator
merges. So #593's recorded ground rules stand unchanged: each phase is exactly one PR, and no
phase starts before the previous phase has merged.

Gate for every phase (per this issue's ground rules): npm test and npm run check:types
and npm run check:arch and npm run build, each run explicitly — .npmrc sets
ignore-scripts=true here, so a green npm test does not imply pretest ran. UI-visible
phases add npx playwright test --project=chromium --project=webkit; happy-dom evaluates no CSS
layout, so no unit test can stand in for a geometric claim.

Per-phase context discipline: each phase's implementation runs in a fresh worker subagent with
its own context window and an isolated worktree. A worktree is pinned to the commit it was
created from, so a push to main mid-phase is invisible to the in-flight worker — origin/main
gets merged into the phase branch, and the full gate re-run, before the PR opens.

Phase	Issue	Status	PR
1 — SurfaceLifecycle primitive + docked right-inspector slot	#586	shipped	#596
2 — side-panel registry	#587	shipped	#600
3 — fail-closed decoders	#591	shipped	#601
4 — decompose the composition root	#588	shipped	#604
5 — dashboard tile gesture controller + pure repaint plan	#589	in review (NOT certified)	#608
6 — reactive workspace aggregate	#590	—	—
7 — @clickhouse/client-web transport spike (ADR-0005)	#585	—	—
8 — guardrail lock-in in check-boundaries.mjs	#592	—	—

Phase 1 — handoff

• Landed (PR refactor(#586): SurfaceLifecycle primitive and docked right-inspector slot (phase 1) #596, head dfd9141): src/ui/surface-lifecycle.ts and
  src/ui/inspector-host.ts as new primitives; a docked inspectorHost + inspectorResize in
  .main-row as real layout siblings of queryHost/dashboardHost; a 'rightInspector'
  splitter axis; cell drawer, rows viewer and Reference all migrated onto both primitives;
  isTopDrawer, the .cd-backdrop probes/CSS, the 'docPane' axis and drawer.ts's stateKey
  plumbing deleted; cellDrawerPx/docPanePx collapsed into one rightInspectorPx.
  All 7 of refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586's acceptance criteria met.

• Decisions taken (not in the issue body):

  • Docked surfaces are non-modal — they do not acquire the keyboard owner, because
    shortcuts.ts bails entirely when ownsKeyboard(app), so the pre-refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 modal cell drawer
    disabled every app shortcut while open. Consequence: app.ts's Query↔Dashboard transition,
    sign-out and connection-scope teardown now close whichever surface occupies the dock
    (closeInspector), not only Reference.
  • The dock holds exactly one occupant — opening Cell while Rows is showing replaces Rows
    rather than stacking. Close-before-mount ordering is asserted, so an outgoing teardown can
    never clobber incoming content.
  • AC5 landed at 2 escape policies, not 1 — 'always' for Cell/Rows, 'focus-inside' for
    Reference, both backed by SurfaceLifecycle's escapePolicy parameter, which AC5 permits as
    "an explicitly justified small number". dialog-shell.ts/popover.ts/detached-view.ts and
    the global close-overlay shortcut were left alone as out of scope.
  • The width clamp became dock-aware — the inherited flat 92vw ceiling was safe for an
    overlay but starves the centre once the panel takes real layout space, so the ceiling now
    reserves a 320px centre minimum plus sidebar and handles, and recomputes on unfold and window
    resize. It only ever changes the displayed width; the persisted preference is never narrowed.

• Deviations from the spec: net production delta was +670/−277 against the issue's estimate
  of "~250–300 removed, ~120 added". Verified as comment weight in this repo's dense style, not
  scope creep — a search for every Unify cell detail, rows and documentation in a foldable desktop right inspector #488 non-goal found only a comment disclaiming that future
  work.

• Deferred to Unify cell detail, rows and documentation in a foldable desktop right inspector #488 (owner decision at the phase-1 gate — recorded in Unify cell detail, rows and documentation in a foldable desktop right inspector #488's body under
  "Inherited from refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 — two deliberate deferrals", covered by its acceptance criteria 9–11):

  1. escapePolicy: 'always' on docked Cell/Rows while non-modal, so Escape with focus in the
     editor closes the panel instead of reaching the global running-query cancel. Cannot be fixed
     in isolation: 'focus-inside' also needs auto-focus-into-panel-on-open, or "click a cell,
     immediately press Escape" breaks — and that focus contract is Unify cell detail, rows and documentation in a foldable desktop right inspector #488's, not this phase's.
  2. Mobile is full-screen but non-modal. styles.css turns .inspector-host into
     position: fixed; inset: 0 below the mobile breakpoint while acquiring no keyboard owner, so
     hardware-keyboard shortcuts stay live behind a visually full-screen panel. Pre-refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 the
     mobile drawer was full-screen and modal, so this is a real mobile behaviour change
     against one of refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586's own non-goals. Knowingly shipped, not missed.

• Review history: an internal read-only review pass, then a ChatGPT pass-1 review that found
  four real defects — all four verified against the code before being accepted. Three fixed
  (8539d81): an abandoned resize drag surviving a mid-drag close and persisting a stray width;
  the unsafe 92vw ceiling; no re-clamp on unfold (plus two comments asserting the opposite of what
  the code did); and a compat chain that let a corrupt canonical value block a valid legacy one
  and yield NaN. The fourth is the deferral above. Disposition comment:
  refactor(#586): SurfaceLifecycle primitive and docked right-inspector slot (phase 1) #596 (comment)

• ChatGPT pass 2 (head dfd9141) found four more real findings, all verified against the
  code; by owner decision none is fixed in this PR, because none is a regression against
  pre-refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 behaviour (the surface used to be a position: fixed overlay, so there was no centre
  minimum to violate and the sidebar width did not interact with it at all):

  • refactor(shell): the docked inspector's centre minimum is not held between 769px and ~1073px #597 — the centre minimum is not held when the row cannot fit both 320px minima. clamp
    is Math.max(lo, Math.min(hi, v)), so a sub-320 ceiling makes the inspector's floor win and
    the centre absorb the shortfall. Unprotected band: 769–901px with the default sidebar,
    769–1073px with dragValue's 420px maximum. splitters.ts:88-91 justifies this by
    delegating to the full-screen mobile override, which ends at 768px — so that justification is
    false, and inspector-dock-layout.spec.js:125 asserts > 200 rather than CENTRE_MIN_PX, so
    it cannot enforce the invariant either. Resolving it is a product call touching Add a drag-foldable desktop left navigation with rail and focused drawers #487/Unify cell detail, rows and documentation in a foldable desktop right inspector #488.
  • refactor(shell): a live sidebar drag leaves the docked inspector's width stale #598 — dragCtx.apply's 'col' branch never re-clamps an open inspector, so a live
    sidebar drag leaves the centre stale (≈246px at 1280px with a 600px inspector).
  • refactor(state): fail-closed decoders for persisted domain records #591 (folded in) — firstValidPx validates with parseInt, so "1e3"→1 and "0x10"→0
    still beat a valid legacy key. Fail-closed decoding of persisted values is that issue's own
    subject.
  • Unify cell detail, rows and documentation in a foldable desktop right inspector #488 — pass-1 finding 3 restated; already deferred by owner decision, unchanged.
  • build(guardrails): extend check-boundaries to lock in the shell primitives #592 (folded in, from my own review, not ChatGPT's) — HANDLE_PX = 7 and
    styles.css's width: 7px are independent declarations of the same layout constant with
    nothing linking them; a CSS-only edit silently invalidates reservedPx. Mechanically
    forbidding that regrowth is phase 8's purpose.

  Disposition comment:
  refactor(#586): SurfaceLifecycle primitive and docked right-inspector slot (phase 1) #596 (comment)

• Lesson worth carrying into later phases: two assertions in this phase had become
  unfalsifiable — .cd-backdrop was deleted repo-wide by AC3, so both a Playwright
  locator('.cd-backdrop').count() probe and a unit querySelector('.cd-backdrop') passed
  unconditionally. A deletion-shaped acceptance criterion silently rots every test that asserted
  the deleted thing's absence. When a phase deletes a symbol, grep the test tree for it too.
  Relatedly — and this is the phase's strongest signal — three separate comments asserted
  behaviour the code did not have: state.ts:655 and splitters.ts:36 (both fixed in
  8539d81) and splitters.ts:88-91 (still present, tracked in refactor(shell): the docked inspector's centre minimum is not held between 769px and ~1073px #597). Two of the three were
  written by the fix for an earlier finding, so the drift is not confined to the original
  implementation. Phases 2–8 are all comment-heavy refactors of documented decisions; treat every
  invariant stated in prose as a claim to verify, exactly as AC6 intends.

• Follow-ups filed: refactor(dashboard): drop the vestigial DashboardApp.prefs field left by #586 #595 (inbox, vestigial DashboardApp.prefs, natural home phase 5 /
  refactor(dashboard): extract the tile gesture controller and a pure repaint plan #589), refactor(shell): the docked inspector's centre minimum is not held between 769px and ~1073px #597 and refactor(shell): a live sidebar drag leaves the docked inspector's width stale #598 (inbox, the two shell findings above), plus inherited-item sections
  appended to refactor(state): fail-closed decoders for persisted domain records #591 (phase 3) and build(guardrails): extend check-boundaries to lock in the shell primitives #592 (phase 8). Phase 2 (refactor(shell): a side-panel registry so adding a navigation panel is a one-file change #587) is unaffected by all of them.

Phase 2 — handoff

• Landed (PR refactor(#587): a side-panel registry so adding a navigation panel is a one-file change (phase 2) #600, head c830a96): src/core/side-panels.ts (a pure as const satisfies
  manifest — every id/pane/persisted-key union derives from it via typeof — plus the fail-closed
  decodeSidePanelKey load-boundary decoder) and src/ui/side-panel-registry.ts (persistent
  per-panel hosts built once and never rebuilt, a mount-once/activate-per-transition
  MountedSidePanel lifecycle, pane-scoped showPanel, and one tab-row renderer shared by both
  panes). renderUpperRoleTabs, SidebarUpperHandle.showRole, WorkbenchStateSlice.sidePanel,
  WorkbenchHooks.renderSavedHistory and AppDom's
  savedList/savedSearch/savedTabsRow are all deleted. All 7 of refactor(shell): a side-panel registry so adding a navigation panel is a one-file change #587's acceptance criteria
  met, two of them in an adapted form (below).

• Decisions taken (not in the issue body):

  • The registry owns BOTH panes for real, not adopt-and-delegate. The originally planned
    design (adopt buildSidebarUpper's hosts, delegate exposure to upper.showRole — which is what
    the salvaged Add a drag-foldable desktop left navigation with rail and focused drawers #487-phase-2 branch did) was rejected at the plan stage after the third-party
    review showed it breaks AC1 and AC5: upperRole was a closed hand-written union in state.ts,
    so adding an upper panel would still have required a state.ts edit, and the upper labels/icons
    would have stayed in sidebar-upper.ts. Full genericization was chosen because it satisfies the
    criteria as written; the alternative (scope the registry to the lower pane) would have meant
    amending owner-recorded acceptance criteria, which an unattended run may not do on its own.
    sidebar-upper.ts still builds the two upper bodies and registers them; it no longer owns
    their tab-row vocabulary. upperRole is now Signal<UpperPanelId>, derived.
  • The upper pane's two hosts keep their existing upper-role-host[data-role=…] DOM via the
    registry's optional host? field, because tests/e2e/dashboard-tree.spec.js reads those
    selectors directly and predates this registry (Add a Databases/Dashboards switcher and dashboard hierarchy tree #426). Library/History get fresh generic hosts.
    This avoided an unrelated e2e selector churn for zero behavioural gain.
  • state.libraryFilter stays ONE shared field, not split per panel — splitting it is Add a drag-foldable desktop left navigation with rail and focused drawers #487
    phase 3's job and an explicit non-goal here. Because persistent hosts mean an inactive panel
    keeps a live search input in the DOM, the salvage branch's ownership guard was ported
    (ownsTheList, implemented here as !host.hidden) so a stale hidden input cannot rewrite the
    shared filter or repaint the other panel's list. Verified equivalent to the salvage's
    activeSection(app) === section: host.hidden is mutated in exactly three places, all inside
    showPanel, always in sync with the registry's own active-panel map.
  • The 'library' ↔ 'saved' bridge is NEW work here, not a re-land. AC6 speaks of "retaining"
    it, but main had no bridge at all — a raw unvalidated localStorage read — so an unrecognised
    stored value silently painted the History body with neither tab visually active. It now decodes
    fail-closed to the documented default. Phase 3 / refactor(state): fail-closed decoders for persisted domain records #591 must not re-implement this (noted on
    refactor(state): fail-closed decoders for persisted domain records #591 and in the CHANGELOG).
  • Downgrade-safe by construction: the registry id 'library' is never itself persisted
    (Library writes exactly 'saved'), asserted negatively at the raw saveStr seam, so reverting
    this PR needs no data rollback.

• Adapted acceptance criteria (deliberate, recorded in ADR-0004's refactor(shell): a side-panel registry so adding a navigation panel is a one-file change #587 addendum):

  1. AC5's "(one file)" is two files — src/application/ may not import src/ui/
     (check-boundaries.mjs:40, and :12 counts import type too) and pure logic belongs in
     src/core/. AC5's actual prohibition is met in full: adding a panel touches none of
     app-shell.ts, app-preferences.ts, state.ts, workbench-session.ts.
  2. Mount-once-per-shell, not once per activation — the issue's Tests wording contradicts the
     persistent-host decision its own AC6 makes binding. AC6 wins.

  AC4 landed literally, better than planned: app-preferences.ts's save became generic over a
  PreferenceValues map, so prefs.save('sidePanel', 'library') is a compile error rather than a
  runtime discipline. (My plan had wrongly concluded AC4 was unmeetable because save(name, value: unknown) has no value union — true of the old signature, but the signature itself could change.)

• Review history: two independent plan reviews before any code (one blocked the plan — see the
  both-panes decision above), then a read-only diff review that found a user-visible bug a fully
  green gate could not see: showPanel was single-pass, so a History → Library switch rendered
  Library before History's deactivate cleared the shared filter, painting Library filtered by
  text the user never typed there. Library → History worked only by luck of registration order, and
  the pinned "clears the filter when switching tabs" test covered only that lucky direction. Now
  two-pass — the same close-before-mount invariant refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 established for the docked inspector — with a
  mirror test plus an ordering test; I sabotage-verified that reverting to one pass turns exactly
  those two tests red. Three further findings fixed in c830a96: a downgrade-safety test whose
  @ts-expect-error line still executed the forbidden saveStr(…, 'library') write while
  asserting only a call count; activation-freshness proven only against injected fake panels rather
  than the real Library/History defs; and three comments citing a "17 call sites" figure (real: 10)
  that came from the plan and was invalidated by the implementation.

• Lesson for later phases: the plan's own numbers are a drift source. "17 call sites" was my
  estimate, copied verbatim into three code comments and wrong by the time it shipped. Phases 4–6 are
  large extractions whose plans will quote counts of call sites and line spans — recompute every such
  figure against the code at commit time, not at plan time.

• Gate: check:schemas, check:examples, check:arch (186 files / 9 rules, no violations),
  check:types, npm test (206 files / 6769 tests, 0 coverage-threshold errors),
  npm run build, all run explicitly; Playwright chromium+webkit 414 passed / 0 failed.

• Follow-ups filed: test(e2e): resize-then-measure race makes inspector-dock-layout and tile-open-workbench flaky under parallel runs #599 (inbox) — a pre-existing resize-then-measure race in refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586's own e2e
  spec (setViewportSize immediately followed by boundingBox() with no wait) that makes two specs
  flaky under parallel runs. Not introduced by this phase.

Phase 2 — ChatGPT three-pass review (PR #600)

Run per the owner's unattended instruction to run all three passes rather than stopping on a
clean one. Every finding below was verified against the real code before being accepted; none was
dismissed — all 8 were real, and all 8 are fixed.

Pass	Head reviewed	Verdict	Findings
1	c830a96	request changes	2 major, 1 minor
2	c80b120	request changes	2 major, 1 minor
3	8eb774b	request changes	1 major, 1 minor

• Pass 1 (comment):
  app-shell.ts imported and listed all four concrete panel defs, so adding a panel did touch a
  file AC5 forbids — and the source-contract guard never examined app-shell.ts, so it passed
  vacuously. Fixed with buildProductionSidePanelRegistry, which required extracting
  side-panel-registry.types.ts to avoid a module-graph cycle. Also: UpperPanelId/LowerPanelId
  were hand-written literal allowlists (a manifest row would not extend them) → derived via
  PanelIdInPane<P>; and accessibleLabel was required but never emitted.
• Pass 2 (comment):
  the pass-1 AC5 fix had moved the defect, not closed it — app-shell.ts still composed
  schemaPane from upper.databasesHost/upper.dashboardsHost by name, so a future upper panel
  would have received a tab and a host that was never inserted into the document. Fixed by
  composing from registry.entries, guarded by a runtime host.isConnected assertion over every
  registered entry (chosen over another source-text grep because it catches the whole detached-node
  class for any future panel). Also: the pass-1 aria-label fix was itself an a11y regression —
  an explicit aria-label on a button replaces the content-derived accessible name, silently
  dropping the live .side-count ("Databases · 3") from what assistive tech announces. Reverted,
  with a regression test asserting a counted tab exposes no aria-label and its text includes the
  count; accessibleLabel stays (AC6 mandates it) with an honest comment on why it must not be
  emitted on a labelled, counted tab.
• Pass 3 (comment):
  confirmed all three pass-2 fixes closed, then found that SIDE_PANELS is documented as canonical
  while buildProductionSidePanelRegistry hard-coded four factories — a real crash path, since a
  future lower-pane row would be resolved by decodeSidePanelKey and then hit entry()'s
  throw new Error('unknown panel id'). Fixed with an exhaustive
  Record<SidePanelId, factory> mapped over the manifest, so a missing factory is now a compile
  error (TS2741, sabotage-verified), plus a parity test that the registry's {id, pane} sequence
  equals the manifest's. Minor: the literal-allowlist guard matched only single quotes, so a
  double-quoted allowlist evaded it (this repo has no linter enforcing quote style — verified: no
  lint deps, no lint scripts).

Pass 3 did not certify clean, and the fixes for its two findings landed after it (96ff31c,
6c4df30). The review script permits at most three passes per conversation, so those two fixes carry
no third-party re-review — recorded here because it is material to the merge decision. Both were
verified locally by the coordinator and sabotage-checked.

Comment-accuracy tally for this phase: five comments asserted things the code did not do, and
they were corrected as found — a "17 call sites" count that came from the plan and was invalidated by
the implementation; a fabricated claim that an "uncalled function body" idiom had precedent here; an
AC5 claim the code contradicted; an overstated type-test guarantee; and a false "a multi-line type
alias would slip past this regex" claim inside the test written to catch inaccurate declarations.
Three of the five were written by the fix for an earlier finding. Phases 4–6 are large extractions
whose plans will quote call-site counts and line spans — recompute every such figure at commit time,
never at plan time.

Final state: head 6c4df30, 12 commits, 27 files, +2210/−312. Local gate: check:schemas,
check:examples, check:arch (186 files / 9 rules), check:types, npm test (206 files / 6777
tests, 0 coverage-threshold errors), npm run build; Playwright chromium+webkit 414 passed / 0
failed.

Phase 2 — review round 4 (owner-relayed, after the three-pass cap)

One further Major finding arrived after pass 3, so it could not be re-reviewed in the same
conversation (the script permits three passes). Verified against the code and fixed in 84288cd.

Manifest uniqueness was assumed by every lookup and enforced by nothing. The two mechanisms that
look like they would catch a duplicate id both cannot:

• Record<SidePanelId, factory> cannot — a TypeScript union collapses duplicates, so a second
  manifest row reusing an existing id requires no additional key;
• the manifest-parity test cannot — a duplicate appears on both sides of its toEqual, so the
  sequences still match. Its own comment claimed exact equality proved "no duplicates"; that was
  false and is corrected.

The breakage a duplicate causes is concrete, not theoretical: byId keeps only the last entry;
the initial normalize loop leaves both hosts visible (each one's id equals its pane's default
active id); and showPanel skips every candidate whose id equals its target, so it can never hide
the shadowed sibling — a permanently double-rendered pane. Duplicate persisted keys are the same
shape: sidePanelKeyFor, decodeSidePanelKey and lowerIdForKey are all first-match
PANELS.find(...), so one row becomes unreachable.

Fix: buildSidePanelRegistry throws on a duplicate def id at construction — placed there rather
than in the manifest because that seam also accepts arbitrary injected defs (the AC5 fake-panel
proof, the e2e fixture) that are not manifest-backed. Plus three invariants asserted directly in
side-panels.test.ts: ids unique, defined persisted keys unique, and a persisted key present for
exactly the lower-pane rows (which is what makes sidePanelKeyFor's non-null assertion sound).
Sabotage-verified: removing the guard fails exactly "throws on a duplicate panel id rather than
silently shadowing one entry".

Two process notes worth carrying forward:

1. A fix for a review finding is this run's single largest defect source — now four times over:
   the pass-1 AC5 fix relocated the defect into the same file's composition; the pass-1
   accessibleLabel fix caused an a11y regression; three of six drifted comments were written by an
   earlier fix; and this round's own uniqueness test shipped a tsc error (TS2339 — reading
   persistedKey off the as const element union) that the first gate run caught. Saved as
   [[review-fixes-relocate-the-defect]].
2. Do not trust a compound command's harness exit code. (a && b && …) > log; echo EXIT=$? run in
   the background reported success while the gate had actually aborted at check:types, because the
   reported code belonged to the trailing echo. Write the exit code into the log and grep for it.

Comment-accuracy tally for this phase is now six, the last two both inside guards' own prose.

#599 flake baselined against main (2026-08-04). After the round-4 fix, a full local e2e run
failed inspector-dock-layout.spec.js:147 (413 passed / 1 failed). Re-running that spec three times
on the phase branch: 2 passed, 1 failed. Re-running the same spec four times on main at
2aa9708, without this branch: 2 of 4 failed. So the flake is pre-existing, slightly more
frequent on main than on the branch, and #587 is not implicated — "fails on my branch" alone would
not have shown that. Chromium+webkit e2e is otherwise 413–414 passed / 0 real failures across six
runs during this phase, and CI's own e2e job is green at every pushed head.

Phase 2 — closeout (this session, before starting phase 3)

PR #600 was already MERGED (b0ba89b, 2026-08-04T14:23:55Z) by the owner per this log's recorded
merge-gate instruction, but the coordinator-owned reconciliation from per-issue-cycle.md step 5 had
not yet run. Done now: closed #587 as completed (all 7 acceptance criteria met, 2 adapted per
ADR-0004 — see the phase 2 handoff above), ticked phases 1 and 2 in #593's ## Phases checklist.
CHANGELOG's phase 2 entry was already present under [Unreleased] from the phase-2 session, so no
further edit was needed there.

Phase 3 (#591, fail-closed decoders) starts now, per the umbrella's ground rule that no phase starts
before the previous one has merged — satisfied as of b0ba89b.

Phase 3 — handoff

• Landed (PR refactor(#591): fail-closed decoders for persisted domain records (phase 3) #601, head 509ca20): src/core/state-codec.ts — five fail-closed
  decoders (decodeStoredVarValues, decodeStoredFilterActive, decodeStoredRecentMap,
  decodeStoredVarRecentDisabled, decodeStoredHistory) following the
  decodeStoredSavedQueries pattern; HistoryEntry moved there and re-exported from
  state.ts (same ResultSort precedent); all five createState call sites replaced;
  pushHistory's literal cap replaced by the shared HISTORY_MAX_ENTRIES constant. Plus
  the inherited refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 item: firstValidPx now requires a complete trimmed
  optionally-signed decimal integer, rejecting '420px'/'1e3'/'0x10'/'12abc'. All 4
  acceptance criteria + both inherited-item checkboxes met, nothing deferred.

• Decisions taken (not in the issue body):

  • RecentValueEntry/RecentMap's local duplicate interfaces in state.ts were also
    converted to re-exports from core/recent-values.ts (optional cleanup in the plan,
    taken because no external importer used the state.js copies) — same
    single-authority reasoning as the HistoryEntry move, not itself required by refactor(state): fail-closed decoders for persisted domain records #591's
    acceptance criteria.

• Deviations from the spec: none — the implementation worker followed the plan
  exactly; no scope or design changes.

• Process change bundled into this PR (owner-requested, not part of refactor(state): fail-closed decoders for persisted domain records #591's own
  scope): skills/ship/references/per-issue-cycle.md's plan-review budget now defaults
  Medium-risk units to a chatgpt-review plan-mode pass (previously "none by default").
  This phase's own plan went unreviewed under the old rule and turned out sound, but the
  owner asked for the stronger default going forward — same escape hatch High already
  has if ChatGPT is unreachable.

• Review history: a Fable-model planning pass produced the plan (risk classified
  Medium, full invariant map, 8 sabotage cases) before any code — this predates the
  process change above, so it did not itself get a chatgpt-review plan pass. A sonnet
  worker implemented + tested + executed and reverted all 8 sabotage cases. The
  coordinator independently re-verified: re-ran the full gate from a clean checkout
  (green), confirmed state-codec.ts at 100/100/100/100 via raw lcov data (it doesn't
  print its own row in the truncated text-summary view — a reporting quirk, not a
  coverage gap), and independently re-executed one sabotage case (reverting the
  history call site to a raw cast) to confirm the wiring test — not just the worker's
  say-so — actually catches it. Given the plan was already reviewed pre-implementation
  and the mandatory 3-pass ChatGPT PR review is next, no additional internal readiness
  review was run (would duplicate the ChatGPT pass).

• Gate: check:types, check:arch, check:schemas, check:examples, npm test
  (207 files, 6810+ tests, 0 coverage-threshold errors), npm run build, all green,
  independently re-run by the coordinator. Playwright chromium+webkit: 418 tests, 414
  passed, 4 skipped (touch-gated), 0 failed.

• Follow-ups filed: none — this phase closes clean against its own contract with no
  newly discovered out-of-scope issues.

ChatGPT three-pass review pending (owner instruction: run all three, not stopping early
on a clean pass). This entry will be updated with the review disposition, then the
coordinator stops at the merge gate per this run's standing instruction — the owner
merges.

Phase 3 — ChatGPT three-pass review (PR #601)

Run per the owner's standing unattended instruction for this run: run all three
passes rather than stopping early on a clean one. Pass 1 found real findings (not
clean), so all three passes ran as part of the normal fix-and-reverify cycle, not as
extra ritual passes over an already-certified head.

Pass	Head reviewed	Verdict	Findings
1	509ca20	changes requested	2 major
2	8cd85af	changes requested	1 major
3	812eaa4	no actionable findings	—

Every finding was verified against the real repository (Node reproductions, not just
taking ChatGPT's word) before being accepted; all 3 were real, and all 3 are fixed:

• Pass 1 (comment):
  (1) decodeStoredRecentMap built byName via an imperative byName[name] = filtered
  bracket assignment — a persisted name of "__proto__" hits Object.prototype's
  inherited setter instead of creating an own property, silently losing the entry from
  Object.keys while corrupting the result's prototype chain. Verified with a Node
  repro. Fixed (8cd85af) by rebuilding via Object.fromEntries, matching the pattern
  decodeStoredVarValues/decodeStoredFilterActive already used in the same file.
  (2) firstValidPx's regex confirmed complete decimal-digit syntax but never checked
  the parseInt result was finite — parseInt('9'.repeat(400), 10) returns
  Infinity, which both bypasses a valid legacy fallback (checked first in precedence
  order) and reaches clamp(x, 320, Infinity) unclamped, producing a literal Infinity
  pixel width in production. Verified with a Node repro. Fixed (8cd85af) by adding a
  Number.isFinite check after parseInt.
• Pass 2 (comment):
  the pass-1 fix for decodeStoredRecentMap closed the decode-side hazard but left
  the identical bug on the write side — src/core/recent-values.ts's
  enforceTotalCap (untouched by this phase's own file list, but the same defect class)
  used the same byName[name] = list bracket assignment, and — worse than the decoder
  case — name here is a live SQL variable name a user can type directly, so this was
  reachable in normal application usage, not only via corrupted localStorage. Verified
  with a Node repro of the exact 100→101-entry cap-transition scenario. Fixed (812eaa4)
  with the same Object.fromEntries rebuild. While writing the reproduction test, the
  implementing worker found two further instances beyond ChatGPT's own scope in the
  same file: recordRecent's m.byName[name] || [] and visibleRecents's bracket read
  both resolve a bare "__proto__" read through the inherited accessor getter (a
  truthy object, not undefined), so || [] never fires and the caller crashes on
  .filter of a non-array — a real, user-reachable crash, not just a data-loss risk;
  and clearRecent's name in m.byName walks the prototype chain too, so
  "__proto__" in {} is true even with zero recorded history, breaking the documented
  same-reference no-op contract. All three fixed with Object.hasOwn, this codebase's
  existing guard idiom elsewhere. recordRecent's own write
  ({ ...m.byName, [name]: list }) was checked and confirmed already safe — an
  object-literal computed property uses CreateDataPropertyOrThrow, not the inherited
  setter — and left alone with a comment explaining why, so it isn't re-litigated later.
• Pass 3 (comment):
  reproduced the exact pass-2 scenario end-to-end (survives the cap eviction, retains
  Object.prototype, evicts the actual lowest-sequence entry, survives a JSON
  round-trip) and found no further issues. Certified: reviewed SHA 812eaa4 equals
  the current PR head, CI green at that head, no accepted actionable finding remaining.

Lesson repeated from phase 2: this phase's single largest source of additional
defects was, again, fixing an earlier finding — the pass-1 fix established a safe
rebuild pattern in one file; the mirror instance a review pass later found sat in an
unrelated file the phase never touched, and turned out to be more severely reachable
(live user input, not just corrupted storage) than the original. See
[[fix-one-boundary-check-the-mirror]] and [[review-fixes-relocate-the-defect]] — worth
treating as a standing checklist item for phases 4-6, which touch large, cooperating
modules where this shape (prototype-unsafe keyed writes on user-controlled names) could
recur.

CI note: the e2e job failed twice at head 8cd85af before passing on a third
rerun — same signature as the already-tracked #599 flake
(inspector-dock-layout.spec.js:147, "Expected: 320, Received: 500"). Confirmed
unrelated to this phase's diff (touches only state.ts's decoders, nothing in the
resize/splitter code path) via 4 local isolated reruns of the same spec (3 passed, 1
failed — consistent with the flake's already-documented ~25-50% local failure rate). A
fix for #599 itself was started in parallel (separate branch/PR, not part of this
phase) once it started actively costing CI reliability on this very PR.

Final state: head 812eaa4, 3 commits (d9fdc4f, 8cd85af, 812eaa4) plus one
unrelated bundled skill-process commit (509ca20, see below), 9 files touched net
across the phase. Local gate (check:types, check:arch, check:schemas,
check:examples, npm test, npm run build) independently re-run and green by the
coordinator at every pushed head; Playwright chromium+webkit 418 tests / 414 passed / 4
skipped (touch-gated) / 0 failed at the phase's initial head.

Bundled process change (owner-requested, not #591's own scope): this PR also
carries 509ca20, editing skills/ship/references/per-issue-cycle.md so Medium-risk
units default to a chatgpt-review plan-mode pass (previously "none by default") —
same escape hatch High already has if ChatGPT is unreachable. Requested by the owner
mid-run after asking why this phase's own (sound, but unreviewed) plan hadn't gotten
one.

Coordinator stops here per this run's standing instruction (recorded in the Run
mode history above): the PR is certified, CI is green at the exact certified head, but
the coordinator does not merge — the owner merges PR #601 themselves.

Follow-ups filed: none new from this phase itself. #599 (flake) was already
on the books from phase 2; a fix is in progress on its own branch, separate PR, not
part of #591's scope.

Phase 3 — closeout (this session, before starting phase 4)

PR #601 was MERGED (merge commit e5d17b7, mergedAt 2026-08-04T16:01:57Z, head
812eaa4) by the owner per this log's recorded merge-gate instruction, but the
coordinator-owned reconciliation from per-issue-cycle.md step 5 had not yet run.
Done now: closed #591 as completed (all 4 acceptance criteria + the inherited #586 item
met — #591 (comment)),
ticked phase 3 in #593's ## Phases checklist. The phase-3 CHANGELOG entry was already
present under [Unreleased] from the phase-3 session, so no further edit was needed
there.

Phase 4 (#588, decompose the composition root) starts now, per the umbrella's ground
rule that no phase starts before the previous one has merged — satisfied as of
e5d17b7. Note: PR #602 (test/e2e-resize-race-599, fixing the #599 flake noted in
phases 2 and 3) also merged to main in the interim (merge 51bb190); it is outside
this umbrella's phase sequence (a pre-existing flake fix, not one of the 8 phases) and
needs no reconciliation here.

Phase 4 — handoff

• Landed (PR refactor(#588): decompose the createApp composition root (phase 4) #604, head ca92c0b): src/ui/app.ts 3,246 → 2,226 lines; App
  interface 112 → 105 required members. Five sequential gate-green commits (one per
  wave): W1 variable-strip.ts (NEW module, not variable-bar.ts — that port is
  deliberately adapter-facing per Rename shared VariableBar activation port to caller-neutral names #478, verified during plan revision), W2
  popover.ts's createAnchoredPopovers + save-controller.ts + keyboard-owner.ts
  hoist, W3 workspace-session.ts (queue/tokens/broadcast/refresh/listeners/
  beforeunload/provisioning — applyCommittedWorkspace deliberately stays in
  app.ts, see below), W4 surface-navigation.ts + 3 type relocations
  (WorkspaceChangedMessage/SurfaceCommandPort/DashboardFocusOutcome/
  WorkspaceRouteStatus), W5 typed staged construction (no as App cast — a missing
  member is now tsc error TS2739, caught one real bug this way: App.editingLibrary
  had never been initialized). Plus a coordinator-caught e2e-fixture fix (0d5e511)
  and a reconcile commit (ca92c0b: CHANGELOG, .wiki/Architecture.md +
  .wiki/Source-Map.md, 6 orphaned imports, 2 stale comments).

• Decisions taken (not in the issue body, all verified against the code by a plan
  revision pass after an independent ChatGPT plan review found the first draft's design
  gaps):

  • applyCommittedWorkspace stays in app.ts, not moved to workspace-session.ts
    as the issue's "zero DOM" framing implied — it does real UI orchestration
    (renderTabs, tree-click cancellation, lost-selection route rewrite). The session
    owns queue/tokens/broadcast/refresh/listeners/beforeunload/provisioning only, and
    calls it as an injected hook.
  • Variable-strip extraction target is a NEW src/ui/workbench/variable-strip.ts,
    not the issue-suggested variable-bar.ts — that module's VariableBarApp port is
    deliberately adapter-facing (Rename shared VariableBar activation port to caller-neutral names #478) with neutral names and a private combo-field
    type; Workbench state doesn't fit it without breaking that contract.
  • Three type relocations out of src/ui/ into src/application/main-surface.ts
    (SurfaceCommandPort/DashboardFocusOutcome/WorkspaceRouteStatus) and a new
    declaration in workspace-session.ts (WorkspaceChangedMessage) — required
    because src/application/* cannot import src/ui/, including type-only imports
    (check-boundaries.mjs), and these types previously lived there. Re-export shims
    keep every existing importer compiling unchanged.

• Deviations from the spec:

  • AC1 ("shrinks toward ≈1,800 lines") landed at 2,226 — above the plan's own
    ≈1,950±100 forecast band. Flagged as directional in the plan; reported honestly in
    the PR rather than claiming the forecast landed.
  • AC5 ("fewer files for a new stateful button") is demonstrated at 3 files
    (module + app.ts wiring + app.types.ts entry) by the variable-strip.ts/
    save-controller.ts pattern — down from the old 4–5. The plan's fully
    self-contained 2-file pattern (a button owning its own element, zero
    AppDom/WorkbenchShellDeps touch) is a designed target for the next new button,
    not yet exhibited by this PR's own extractions. Stated precisely in the PR rather
    than overclaimed.
  • One pre-existing defect deliberately not fixed: anchoredPopover.close() (now
    in popover.ts) can clobber a newer popover sharing the same dom ref slot — moved
    verbatim from app.ts, not introduced by this phase. Filed as anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 (inbox),
    pinned by a characterization test.
  • One genuine bookkeeping improvement, not a behavior change: variable-strip.ts's
    deferral state (varStripSig/etc.) is now keyed to strip-element identity instead
    of the old app.dom-reset coupling, closing a latent sign-out/sign-in remount gap.

• Review history:

  • Plan, fable model at high reasoning effort: full plan with a 23-row invariant
    map.
  • One independent ChatGPT plan-mode review (required for High risk) found 9 real
    gaps: an internally-contradictory Wave 5 construction algorithm (spreading
    pre-built member records into the literal that creates the object those records'
    factories supposedly need), an under-specified module boundary that would have
    leaked UI orchestration into src/application/, an undesigned AC5, 3 weak
    sabotage cases, a hidden behavior change in the popover close, a port-design
    contradiction with variable-bar.ts, 7 missed ordering invariants, and a
    test-migration rule gap. A second fable-model verification pass checked every
    finding against the actual code (not just the plan's prose) and produced a revised
    plan closing all 9.
  • Implementation: 5 sequential sonnet subagents (one per wave — all touch
    app.ts, so no parallelism), each producing its own gate-green commit with new
    tests at coverage floors and sabotage-verified invariant tests. The coordinator
    independently re-verified every wave (git diff/git log against the self-report,
    full local gate re-run from a clean state) rather than trusting the self-report.
  • e2e regression caught by the coordinator, not any subagent: two Playwright
    fixtures (import-example-dashboard.html, oauth-document-recovery/index.html)
    called the old flat app.rewriteWorkspaceRoute/app.syncSqlRoute directly —
    production call sites were repointed by the implementing waves but these two
    raw-ESM fixtures were missed (the same footgun this repo's process docs already
    name: "an App-shape change must update the fixture __app construction in
    tests/e2e/*.html, not just the specs"). Fixed in 0d5e511.
  • One targeted readiness review (fable model, read-only): spot-checked ~12 of the
    30 claimed invariants against the real code (all confirmed enforced as claimed),
    confirmed member-disposition and dependency-boundary claims by direct grep/re-run,
    found 3 minor cleanup items (6 orphaned imports, 2 stale comments, missing
    CHANGELOG/wiki reconciliation) — all fixed in ca92c0b.

• Gate: check:types, check:arch (191 files / 9 rules, 0 violations),
  check:schemas, check:examples, npm test (212 files, 6,971 tests, 0
  coverage-threshold errors), npm run build, all green at the final head, run
  explicitly by the coordinator (not inferred from a worker's report). Playwright
  chromium+webkit: 414 passed / 0 failed.

• Follow-ups filed: anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 (inbox) — the pre-existing anchoredPopover.close()
  stale-clobber defect described above.

Phase 4 — ChatGPT three-pass review (PR #604)

Run per the owner's standing unattended instruction for this run: run all three
passes rather than stopping early on a clean one.

Pass	Head reviewed	Verdict	Findings
1	ca92c0b	changes requested	2 major, 1 minor
2	15f798a	no actionable findings	—
3	15f798a (unchanged)	no actionable findings	—

Every finding was verified against the real repository before being accepted; all 3
were real, and all 3 are fixed/disclosed:

• Pass 1 (comment):
  (1) Major — variable-strip.ts's rebuild signature folds in each enum field's
  option count but not option identity (c.enumOptions.length, not the values
  themselves), so a same-cardinality option-set change (e.g. ['a','b'] → ['c','d']
  via a background column reload) does not trigger a rebuild and the dropdown goes
  stale. Verified via git show origin/main:src/ui/app.ts that this line predates the
  refactor(app): decompose the composition root along four seams #588 phase-4 branch entirely — a second undocumented preserved defect, not
  introduced by this PR, which had claimed anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 was the sole one. Disposition:
  disclosed, not fixed (a pure structural extraction is not the place for a
  behavior change) — documented in a code comment at the signature site and filed as
  Workbench var-strip rebuild signature ignores enum option identities (only counts them) #605; the PR description corrected from "the sole… defect" to "two… defects."
  (2) Major — AC5's PR-description claim that a new stateful toolbar button now
  takes "3 files" was unsupported: workbench-shell.ts still builds and inserts the
  pre-existing runBtn/saveBtn/varStrip DOM elements directly — the extraction
  moved each element's render/update logic into its own controller, not the
  element's construction/insertion. Verified against workbench-shell.ts:132/136/160.
  A genuinely new button under this pattern still touches 4 files (controller +
  workbench-shell.ts + app.ts + app.types.ts) — still a real reduction from the
  old 4–5, but not the 3-file path claimed. Fixed: PR description corrected to state
  the demonstrated 4-file path honestly. (3) Minor — app.types.ts's header
  comment still described the removed const app = {} as App construction mechanism.
  Fixed (15f798a).
• Pass 2 (comment):
  reviewed head 15f798a, confirmed all three pass-1 findings correctly dispositioned,
  zero new findings. Comment-only diff since pass 1 (no executable-code or test
  changes) — re-verified via full local gate + CI before this pass ran.
• Pass 3 (comment):
  run per this run's standing instruction to always complete three passes rather than
  stop at a clean pass 2 — reviewed the identical head 15f798a (zero commits since
  pass 2), confirmed zero regressions possible from a zero-delta head, exact-head CI
  green across unit tests/coverage/build/bundle/Docker smoke/Playwright. Certified:
  reviewed SHA 15f798a equals the current PR head, no accepted actionable finding
  remaining.

CI note: one transient infrastructure failure at head 15f798a — the docker-smoke
job's docker/dockerfile:1 base-image pull hit 500 Internal Server Error from
auth.docker.io, unrelated to this PR's diff (test/e2e/bundle/size all passed
in the same run). Confirmed transient via gh run rerun --failed, which passed clean
on retry with no code change.

Final state: head 15f798a, 7 commits (5 waves + 1 coordinator e2e-fixture fix +
1 ChatGPT pass-1 fix), 28 files touched net across the phase. Local gate
(check:types, check:arch, check:schemas, check:examples, npm test, npm run build) independently re-run and green by the coordinator at every pushed head;
Playwright chromium+webkit 414 passed / 0 failed.

Coordinator stops here per this run's standing instruction (recorded in the Run
mode history above): the PR is certified, CI is green at the exact certified head, but
the coordinator does not merge — the owner merges PR #604 themselves.

Follow-ups filed: #603 (inbox, carried from phase 1's ChatGPT review — the
anchoredPopover stale-close defect, moved verbatim into this phase's popover.ts, not
fixed here). #605 (inbox, new this phase — the var-strip rebuild-signature gap
above).

Phase 4 — closeout (this session, before starting phase 5)

PR #604 was already MERGED (15f798a, 2026-08-04T21:12:12Z) by the owner per this
log's recorded merge-gate instruction, but the coordinator-owned reconciliation from
per-issue-cycle.md step 5 had not yet run. Done now: closed #588 as completed (all 6
acceptance criteria met — 2 of them, AC1 and AC5, landed in a weaker/honestly-stated
form rather than the plan's exact target, both already disclosed in the PR and in the
handoff above — #588 (comment)).
## Phases checkbox for phase 4 in this issue ticked itself automatically on issue
close (GitHub's task-list-to-linked-issue feature), so no manual edit was needed there.
The phase-4 CHANGELOG entry was already present under [Unreleased] from the phase-4
session, so no further edit was needed there either.

Phase 5 (#589, dashboard tile gesture controller + pure repaint plan) starts now, per
the umbrella's ground rule that no phase starts before the previous one has merged —
satisfied as of 15f798a's merge to main.

Phase 5 — handoff

• Landed (PR refactor(#589): extract dashboardRepaintPlan + createTileGestureController out of renderDashboard (phase 5) #608, head 7e645eb): src/dashboard/application/dashboard-repaint-plan.ts
  (repaint-decision arbitration — dashboardRepaintPlan plus six granular
  plan* functions it composes) and src/ui/dashboard-tile-gestures.ts
  (createTileGestureController, the drag/resize gesture controller). Three
  sequential waves (repaint plan → gesture controller → test migration/shrink
  audit), then four fix rounds: one internal readiness review, three ChatGPT
  PR-review passes. All 6 of refactor(dashboard): extract the tile gesture controller and a pure repaint plan #589's acceptance criteria addressed; AC4 is a
  disclosed partial (below).

• Decisions taken (not in the issue body):

  • The plan-review cycle ran twice before any code was written. A first
    ChatGPT plan-mode review found 9 real gaps (a false "one gesture at a
    time" invariant claim, batched-vs-staged memo commits, underspecified
    seed values, untested persistence-array parity, a raw signature mutation
    crossing the extraction boundary, unlocked getter call-time semantics,
    non-idempotent cleanup, an imprecise signature-shape claim, unsafe wave
    parallelism) — a second fable-model pass verified all 9 against the real
    code (not just the plan's prose) and produced a revised plan closing all
    9 before implementation started.
  • Two pre-existing latent defects, found by reading the real code during
    planning, are deliberately preserved, not fixed, and filed separately
    per the refactor(app): decompose the composition root along four seams #588/anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 precedent: Dashboard tile gestures: mixed drag/resize concurrency has no cross-gesture guard #606 (mixed drag/resize gestures aren't
    mutually exclusive today — no cross-gesture guard, last-writer-wins
    cancel slot, no pointerId filtering) and Dashboard tile drag: pointerdown-time engine snapshot can disagree with live engine reads mid-gesture #607 (a drag snapshots the
    active engine once at pointerdown for its reflow path but reads it live
    elsewhere in the same gesture). Both are pinned by characterization tests
    written FIRST against the unmodified pre-extraction code and kept green
    with zero assertion changes across the extraction — the standard this
    umbrella has used since phase 1's Unify cell detail, rows and documentation in a foldable desktop right inspector #488-deferral precedent.
  • dashboardRepaintPlan is decomposed into six granular pure functions
    (planRepublishFlow/planBarRebuild/planOptionsPush/
    planLabelRefresh/planPersist/planStructuralRebuild), a design that
    only emerged during the ChatGPT PR-review cycle (below) — the issue's own
    text names only the singular dashboardRepaintPlan, and AC2 is satisfied
    by keeping that name as a composed, independently-tested convenience
    surface while production code calls the six functions directly, proven
    equivalent by a dedicated test.

• Deviations from the spec:

  • AC4 ("closure shrinks by roughly 500–700 lines") lands below the
    band: dashboard.ts 3,228 → 2,772 lines (-456, -14.1%). The wave-3
    measurement was -509 (within band); a subsequent ChatGPT-review
    correctness fix (restoring compute/apply interleaving across all six
    repaint decisions, so a throw computing a later decision can never
    strand an earlier one's already-decided side effect — see pass 2 below)
    added back ~53 lines of explicit per-decision orchestration in
    dashboard.ts. Not reversible without reintroducing the bug it fixed;
    reported honestly in the PR rather than restated to fit the band, per
    this umbrella's own phase-4 precedent (AC1's ≈1,800 target similarly
    landed short, disclosed rather than forced).
  • AC5 ("dashboard.ts's DOM-driven test file shrinks") landed
    narrowly: 6,487 → 6,484 lines (-3 net) — waves 1–2 first grew it +205 for
    new characterization/consumption tests, wave 3 retired ~208 lines of
    now-redundant DOM-simulation assertions once the new modules' direct unit
    tests covered the same ground more thoroughly.

• Review history — the deepest ChatGPT-review cycle this umbrella has run
  so far. All three passes were used (per this run's standing instruction
  to always run all three, not stop early on a clean one) — but unlike prior
  phases, passes 1, 2, AND 3 all found real, accepted findings, so this
  phase's head is genuinely not certified, not merely "certified after
  the full three-pass ritual."

  Pass	Head reviewed	Findings	Disposition
  1	dd60761	3 (1 real ordering bug, 2 test-coverage gaps)	All 3 accepted, fixed in 0a7823d
  2	0a7823d	4 (1 P2 architectural, 3 P3)	All 4 accepted, fixed in 934dc56
  3	934dc56	3 (1 P2 protocol-drift risk, 2 P3)	2 fixed in 9b95d47 (not re-reviewed — ceiling reached); 1 disclosed as the AC4 shortfall, not a code fix

  The single recurring defect class across all three passes — a throw
  computing a later repaint decision stranding an earlier one's
  already-decided side effect unapplied, because the extraction had batched
  computation ahead of application instead of preserving the pre-extraction
  code's interleaved compute-then-apply-then-compute-next order — first
  surfaced narrowly (the memo.mobile commit specifically, pass 1), was
  diagnosed as a general architectural gap by pass 2 (fixed by decomposing
  the planner into six granular functions dashboard.ts calls and applies
  individually), and left one more residual instance (the composed
  dashboardRepaintPlan vs. the production six-call sequence becoming two
  independently-maintained protocols that could silently diverge) that pass
  3 caught and a protocol-equivalence test closed. This is the same defect
  class the internal readiness review had already found and fixed once
  before the PR even opened (the engine-switch throw-path instance, in
  dd60761) — a single root cause recurring in different forms across four
  separate review passes is the strongest signal from this phase: an
  extraction that separates "compute a decision" from "apply a decision"
  will keep reproducing this bug shape at every seam until every one of
  those seams is individually re-verified against the pre-extraction
  interleaving, not just the memo-commit points.

  Full per-finding detail (file:line citations, sabotage-verification
  results, and the coordinator's independent re-verification of every
  finding against the real repository before acceptance) is in PR refactor(#589): extract dashboardRepaintPlan + createTileGestureController out of renderDashboard (phase 5) #608's
  description and its three review comments:
  pass 1,
  pass 2,
  pass 3.

• Gate: check:types, check:arch, check:schemas, check:examples,
  npm test (215 files, 7,074 tests, 100/100/97.13/100 global coverage, every
  per-file floor cleared), npm run build, all independently re-run by the
  coordinator (not inferred from any worker's or review's self-report) at
  every one of the 7 pushed heads. Playwright chromium+webkit: 414 passed / 4
  skipped / 0 failed (measured at wave 3's head; unaffected by the
  test-only/orchestration-only commits that followed).

• Follow-ups filed: Dashboard tile gestures: mixed drag/resize concurrency has no cross-gesture guard #606 (inbox, mixed gesture concurrency), Dashboard tile drag: pointerdown-time engine snapshot can disagree with live engine reads mid-gesture #607
  (inbox, engine snapshot-vs-live inconsistency) — both pre-existing,
  deliberately preserved, pinned by characterization tests.

• Certification: FAILED — this is a safety failure under the unattended
  contract, not an approval gate with human override available. Three
  ChatGPT passes ran; pass 3 (the last allowed) found real findings. Per
  references/unattended.md's hard rule, three passes is a failure ceiling,
  not a ritual, and there is no unattended-mode override for "findings remain
  after the ceiling." The coordinator does not merge PR refactor(#589): extract dashboardRepaintPlan + createTileGestureController out of renderDashboard (phase 5) #608. CI is green
  at the final head (7e645eb) and two of pass 3's three findings are fixed
  (unverified by a further ChatGPT pass); the third (the AC4 shortfall) is a
  disclosed, non-code-defect fact, not something to fix. The owner must
  review PR refactor(#589): extract dashboardRepaintPlan + createTileGestureController out of renderDashboard (phase 5) #608 directly — read the "ChatGPT review — all 3 passes" section
  of its description for the full disposition — and decide whether to merge
  as-is, request further work, or run a manual fourth review pass themselves
  outside this run's ceiling.

Phase 6 (#590, reactive workspace aggregate) is blocked until phase 5
merges, per the umbrella's ground rule that no phase starts before the
previous one has merged. This run stops here.