Land the sidebar-issues work from the release candidate onto main #116
No reviewers
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
lz/agent-nexus!116
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "rc/sidebar-96"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Brings the four
sidebar-issuesPRs ontomain. Everything else the RC carried — the #96 stack (#98/#99/#100/#101/#106/#107), #103 and #104, and #105 with its conflict resolved — has already landed onmainthrough its own PR, so this merge is exactly those four and nothing else: +370 −201 across 21 files.Contained here, each already reviewed and merged into the RC:
env_var_countonWorkerDTO, plus the client/server parity guardWhy a merge and not four fresh PRs
They were reviewed and merged against the RC while it was the trial artifact, so re-opening them against
mainwould duplicate a record that already exists. Merging keeps every commit and every merge commit inmain's history rather than squashing them, sogit log mainstill shows the four individually.Verified before merging
rc → mainwas confirmed conflict-free, and the RC was gated after integration: typecheck 4898 files / 0 errors, lint clean, 121 files / 1201 tests.rc/sidebar-96is deleted after this lands. It existed to be a deployable trial with a rollback to:latest, andmainnow carries everything it did.`.left-pane` sets `flex-shrink: 0` for the desktop row layout. Under 768px `.main-split` flips to `flex-direction: column`, so that shrink factor starts applying to the vertical axis: the pane grows to its full content height and is clipped by `.main-split { overflow: hidden }`. `.lp-body { overflow-y: auto }` therefore never gets a constrained height to scroll inside, and everything past the first viewport is unreachable. Give the pane `flex: 1 1 auto; min-height: 0` in the mobile block only. Measured at 390x740 with 12 cards in the list, on the real page: before pane 1488px, lp-body client 1488 == scroll 1488, scrollTop stuck at 0 after pane 683px, lp-body client 683 < scroll 1488, scrolls freely Desktop is unaffected -- at 1280px the pane still computes to 360px with `flex-shrink: 0`, since the media query does not apply there.The card used to claim "running · healthy" the instant container.start() returned, while the entrypoint spent another 30-90s cloning the repo and running `npm install -g @anthropic-ai/claude-code@latest`. Every session action failed in that window with no explanation on screen. Adds a `booting` phase that survives finishProvisioning and is cleared by checkWorkerHealth on the first successful `tmux has-session` probe -- tmux answering IS the definition of usable. While booting, the card shows the last `==> ` marker the entrypoint printed; entrypoint.sh already emits those, so no worker-side change was needed. Adds pull progress. pullWorkerImage already yielded per-layer {id, current, total} and provisionWorker discarded all of it except as a boolean "has anything happened yet" flag. PullProgress folds those into a percentage and a layer/byte line, throttled to one DB write per 750ms (docker emits per layer per chunk) with a forced final write so the bar cannot freeze mid-percentage. Percent is null -- an indeterminate bar -- whenever any layer lacks a known total, rather than showing a number that races upward and jumps back as sizes arrive. Adds Retry on a failed card. It cannot restart anything: provisionWorker tears the container down on every failure path and container_id was never written, so retryWorker re-enters the pipeline from `queued` on the same row, keeping the frozen mounts_json snapshot it was created with. It needs the master key to rebuild the sealed secret block, so it is an operator route and never something convergeSessions could do. The provider is resolved BEFORE the row is claimed, so a retry that fails early leaves the row in `error` and retryable. Two traps handled: - failStuckProvisioning must SPARE `booting`. Nothing in Nexus drives that phase -- the container is started and its entrypoint runs to completion whether or not Nexus is up -- so sweeping it would errored-out every workspace booting across a Nexus restart. Verified live: on a harness restart the two `pulling` rows failed and the `booting` row was untouched. - Session controls are gated on the phase, not just on `running`. Sessions, the shell agent and the view panels all need tmux, so leaving them live during boot just moves the failure one click later. Verified in a browser against a seeded instance: determinate bar at 62% with "4 of 7 layers · 218.0 MB / 351.0 MB", indeterminate bar when a layer size is unknown, the booting card showing its entrypoint marker with no "start agent" button, POST /retry rejecting a non-errored row with 400, and a failed retry leaving the row's original error message intact.Both found by running the real worker image through a real dind daemon, not by the suite. 1. The tag was counted as a layer. Docker's stream-level "Pulling from <repo>" event carries an `id` -- the TAG, not a layer -- so observe() registered it alongside the real layers. Measured against docker 29.6.2: a 5-layer image reported 6. Stream-level statuses are now filtered out. 2. Completion could not be inferred from bytes. A cached layer reports "Already exists" and transfers nothing, so it never reaches a total and was counted as incomplete forever. Layer completion now comes from the terminal statuses ("Pull complete" / "Already exists"), which is what docker actually tells us. A fully cached pull previously read "0 of 6 layers · 0.0 MB"; it now reads "5 of 5 layers", and the byte clause is dropped entirely when nothing was transferred rather than shouting "0.0 MB". 3. Phase detail outlived its phase. advanceProvisionPhase left provision_detail and provision_percent in place, so the card showed "Creating container… 100%" with the pull's byte count under it. Observed live at the pulling→creating transition. Advancing now clears both; the new phase writes its own. Live trace after the fixes, against x.lck.sh/lz/agent-nexus-worker:latest (318 MB, 20 layers) pulled from an empty local store: queued pulling pct=null 1 of 20 layers · 5.3 MB pulling pct=38 15 of 20 layers · 122.5 MB / 318.1 MB pulling pct=100 19 of 20 layers · 318.1 MB / 318.1 MB creating pct=null (no detail) booting Cloning https://x.lck.sh/lz/agent-nexus.git booting Registering forgejo-mcp booting Updating @anthropic-ai/claude-code to latest null (ready -- healthy, tmux up, repo cloned on main) Percent is null only for the first ~10s, while docker is still announcing layer sizes; once every total is known it rises monotonically to 100. The earlier worry that the determinate bar would rarely appear did not hold up.`views` was not the only field on a group's view state that references panel ids — `tabGroups[].panelIds` does as well. It rode through the `{...data}` spread unexamined, so a tab group could keep naming an artifact panel that had just been stripped: a dangling reference persisted by the one function whose job is to persist none. TypeScript could not see it. The local narrowed `PanelViewState` deliberately omitted every optional field of dockview's `GroupPanelViewState`, and its own comment listed `tabGroups` among them — so the field was carried at runtime and invisible at compile time. It is now modelled locally, with the rule stated: model a field here the moment it can name a panel. Latent rather than live — Nexus never creates tab groups, and no saved layout carries the key today. Fixed anyway because a dangling-reference vector inside the module that exists to eliminate them is incoherent whether or not anything currently triggers it. Emptied tab groups are dropped rather than persisted with an empty panelIds array. Verified the three new tests fail against the previous implementation and the fourth (no tab groups present) passes either way.The previous commit shipped a hole I had documented as a deliberate limitation. It was not one, and it sat on the most natural way to try floating at all. Floating a panel calls `removePanel(item, { removeEmptyGroup: true })` and `removeGroup` fires when the source group empties (dockviewComponent.js:1332, :2576). So floating your ONLY panel leaves a childless branch as the grid root. `pruneGridObject` returned null for that, `stripArtifactPanels` returned null, and `saveLayout` then REMOVED the stored layout — wiping the float on the next save. The same silent loss the previous commit set out to fix, reached by a different route, and reachable by the single most obvious gesture. The reasoning I recorded for it ("dockview needs a grid root, not worth risking a fromJSON throw") is factually wrong for 7.0.2. `_doFromJSON` validates only `root.type === 'branch' && Array.isArray(root.data)` (dockviewComponent.js:1937-1939); an empty branch passes, and is exactly what `Gridview.clear()` produces for an empty dock. Verified in a browser: injecting an empty-branch root plus one float restores the float, renders it, and round-trips through three reloads with a forced relayout each time. So `null` now means one thing only — nothing survived ANYWHERE — which is what saveLayout's removeItem branch is actually for. Those two states were conflated. When the grid is empty but floats survive, the grid is emitted as an empty branch, preserving the original root's own fields rather than rebuilding it. The test that asserted the old behaviour is inverted, and joined by the literal post-float state (already-empty branch root) and by the genuine nothing-survives case, so the two are no longer confused. Found by an adversarial review of the previous commit. It was right and I was wrong: I had called this a corner case, and it is the primary path.Three findings from an adversarial review of this branch. **Floats came back in the corner.** dockview hands sizing to ShellManager, whose only size source is a ResizeObserver deferred through requestAnimationFrame — so the component is still 0x0 on the synchronous line where we call fromJSON. That captures 0x0, and the constrainBounds() fromJSON runs at the end clamps every restored float against a zero-height container: clamp(top, -100, 0) is 0, and the same for left. The float lands at the dock origin, and because the clamp writes style.top, a later save persists 0,0 and the position is gone for good. One line fixes it: lay the dock out before restoring. Deferring the restore into a rAF would be a race instead, since dockview's own observer callback is already inside one. Measured, with a negative control first: seeding a float at top 137 / left 211 and reloading rendered it at 0px/0px before the change and at 137px/211px after, stable across reloads with a forced relayout each time. **The whitelist rebuild had no guard, only comments.** `stripArtifactPanels` rebuilds its result from a hand-listed set of SerializedDockview fields, and that is exactly how floats were lost for a release: a field existed, nothing referenced it, no test noticed. The justification for still dropping popoutGroups/edgeGroups was a comment asserting a fact about the UI that nothing enforced. There is now a compile-time assertion over `keyof SerializedDockview`, in the same idiom as the `_exhaustive: never` guards in mounts/validate.ts. Verified it fires by dropping a key from the allowlist: ERROR layout.ts 185:7 Type 'boolean' is not assignable to type '{ UNHANDLED_DOCKVIEW_KEY: "edgeGroups"; }' The next dockview upgrade that adds a serialized field now fails the build and names it, instead of silently discarding operator state. **The narrowed local types are gone.** `GroupPanelViewState` is not re-exported by name, but it is reachable structurally as `NonNullable<SerializedFloatingGroup['data']>` — through the very type this branch already imported. The hand-narrowed copy is what let the tabGroups bug in two commits ago: it omitted the one field that names panel ids, so the spread carried it through invisibly. The hand-rolled SerializedTabGroup mirror goes too; dockview exports it (`export * from 'dockview-core'`). What was a standing promise to mirror the right fields is now structural. Also adds the invariant the reviewer rated above every individual case: whatever else happens, a branch root in produces a branch root out. dockview rejects a non-branch root outright, so it is the single property deciding whether the blob we persist is restorable at all.The guard added in the previous commit was one level too shallow, and the field it missed was live. `maximizedNode` is a POSITIONAL reference — `{ location: number[] }`, an index path into the grid tree — so it is invisible to a panel-id audit. The float branch spread it through with `{ ...float.grid, root }` while the main grid, which rebuilds field by field, dropped it. Pruning shifts or deletes the indices it names, so `Gridview.deserialize` then calls `getNode(location)` and throws 'Invalid location' (gridview.js:767-769): the STRIPPED layout fails to restore where the unstripped one would have. Measured against real dockview 7.0.2 in jsdom, not a fixture. Unreachable in 7.0.2 — `maximize()` early-returns unless the group's location is 'grid', so a float's nested gridview cannot carry the field — but the asymmetry was the bug: two rebuild sites, two different answers, one of them unsafe. Both now drop it. Dropping beats repairing: an index path cannot be corrected after pruning without re-deriving it, and maximize state is not worth that. The deeper problem is why the guard did not catch it. `AssertNoUnhandledKeys` was parameterised over `keyof SerializedDockview`, which pins the TOP-level field set only — while the grid sub-object at the rebuild site is a second hand-listed set, and hand-listed sets are precisely the mechanism that lost floats for a release. Widening to `keyof SerializedDockview['grid']` would not work either: dockview declares that member inline with four fields (dockviewComponent.d.ts:99-105) while `toJSON` assigns it `this.gridview.serialize()` — a full `SerializedGridview` with five (dockviewComponent.js:1832, :1840). The declared type understates what the runtime writes, so a guard over the declared shape is blind by construction. So the second assertion guards `SerializedGridview` itself, the shape actually written. Verified it fires by removing a key: ERROR layout.ts 225:7 Type 'boolean' is not assignable to type '{ UNHANDLED_DOCKVIEW_KEY: "maximizedNode"; }' Found by the same adversarial review, re-run against the previous commit. Its verdict on the rest of that commit: idempotence and purity survive the rewrite, `emptyBranchRoot` holds on both arms, tabGroups pruning reaches both float forms, and the layout-before-restore call has no zero-size mount to trip on.Two different things claimed a card's left edge: status (running / stopped / booting / errored) and identity ("these panels are docked, and this is their tab colour"). Worse than crowding, three of the eight SESSION_PALETTE hues -- --danger, so the two channels could read as each other. Identity moves to a background wash on the ROW (and the dock tab), leaving the border to status. The status dot's fill is never recoloured, so the signals no longer compete for the same pixels. Two alphas, not one: --wash-row 0.10 reads on a 330px sidebar row and disappears entirely on a 40px rail tile, which needs ~0.22. One global value cannot serve both surfaces. sessionColourRgb() is DERIVED from SESSION_PALETTE rather than written out a second time -- a parallel hand-maintained list would drift silently, and the two must agree by construction because they identify the same target. A test pins them together. A wash is a fill, and the sidebar already fills for hover, so hover on a washed row becomes a border change instead of a third fill. Also promotes the .bar track/fill to app.css: QuotaBadge already had the same class name and the same track+<i> idiom, so the card was the second copy, not the first.RAIL. One bordered capsule per workspace: initials on top, then one zero-gap segment per target. The shared border is what makes a session read as belonging to its workspace -- the loose 9px dots said nothing about who owned them, and at 81px² were barely clickable. A segment is 40×24 = 960px². The rail is desktop-only (MainSplit gates it on isDesktop), so the bar is WCAG 2.2's 24×24 minimum, which 24px segments meet exactly and the 9px dot failed outright. The capsule's left border carries STATUS, matching the wash carrying identity everywhere else. A booting workspace gets a progress segment so the one status a collapsed rail must not hide stays visible, and a warn pip marks anything waiting on the operator. HOVER CARD, replacing a one-line `${kind} ${name}` tooltip that told the operator less than the row it replaced -- so collapsing the sidebar cost information, not just space. Three things it had to get right: - opens on focus as well as hover; the segments are buttons and a keyboard user got nothing at all before - portalled to <body>, because the rail scrolls and its overflow clips anything past 52px - ONE timer shared across segments, so sliding down the rail does not re-arm a full delay at every stop PILL. Reaching the dock on mobile used to REQUIRE opening a panel, since openPanel() was showDock()'s only caller: forward always mutated the layout while coming back was free. The pill is the missing free direction. It renders only when something is docked, so it is never a control leading nowhere, and it carries identity-coloured dots rather than a numeral -- the colours say whose panels are waiting, which a count cannot.Two adversarial reviewers over the audit-fix commits. The critical one is verified against real tmux, not reasoned: **`respawn-window -t nexus:<name>` resolves an all-digit name as an INDEX.** tmux tries the window half of a target as an index before it tries it as a name, and `1` / `42` are legal git refs the create form accepts. Measured on 3.3a with windows `0:workspace 1:alpha 2:beta 3:"1"`, `-t nexus:1` resolves to **alpha** — and the `=` exact-match prefix does not change it. So Restart on a session named `1` SIGKILLs alpha's claude, relaunches session 1's agent inside alpha's window (where notify-preview then attributes to the wrong session), and leaves alpha dead permanently: convergeSessions only restores a session whose window is MISSING, and alpha's window is right there. Now resolved to a window ID first. The same hole existed in `deleteSession`'s `kill-window`, older than this PR and worse — it would destroy a bystander's window outright — so that site moves too. Resolving also means a Restart on a session whose window went missing now CREATES it instead of failing, which is the state the panel reports while leaving the button enabled. **Stop left `sessions.status = 'running'`.** That column is what the rail and its hover card read, so a stopped agent showed green and "agent: running" — while the expanded sidebar, which reads the probe, painted the row RED with a "no claude running in <worktree>" line. The operator stops an agent on purpose and one surface says it is fine, another says it crashed. `'stopped'` was already in the union, so this is a write and a guard, no migration. **Restart claimed to resume a conversation it had not resumed.** `resumeFlag` emits nothing for the empty `claude_session_id` that migration 0012 backfilled, so on an upgraded install Restart silently started a fresh conversation under the old toast, and left the row permanently unresumable. It now mints and pins an id the way `openWorkspaceSession` does, and reports `resumed: false` so the toast says which of the two things happened. **Neither action waited for claude.** tmux exiting 0 only means the pane was respawned; the launch line falls through to `exec bash` if claude dies on startup. Both paths now wait — the shape `openWorkspaceSession` already used — and return `{confirmed}`, so "Agent relaunched, but claude has not come up yet" is sayable instead of an unconditional success. The silent-failure pass found four more places the operator is told nothing: `goto` was the one awaited call outside a try (a rejected navigation became an unhandled rejection); "Copy branch name" returned silently when the roster had no branch, leaving the previous clipboard contents to be pasted into a checkout; "New session…" on a booting workspace expanded and scrolled and then found no form; and a failed repo-env fetch rendered as "None set for this repository" — an affirmative claim produced by not knowing, on the pane whose whole job is stating a blast radius. `worker-images` also latched a transient failure for the life of the page, so one bad response hid every capability chip. Two more the reviewers caught in the merged workspace tab: it offered "Stop shell agent" beside "Open workspace shell" (distinct actions, so the new dedupe did not collapse them) — fixed in the builder, since offering to open a running shell was equally wrong on the card; and a session Settings panel restored from localStorage rendered the WORKSPACE pane, complete with a live "Remove workspace…", during the window between listWorkers and listSessions. Verified: the getter on `deps.menuHost` does survive the `$state` proxy (an accessor has no `writable`, so the trap falls through to `Reflect.get` and the getter runs on every read), `lastProbedAt` is per-instance, and `dedupeByAction` keeps its separator. Five new tests cover the numeric name, exact-name matching, the missing-window fallback, the unresumable row and the status write; each was confirmed to go red against the behaviour it describes.Closes a latent bug the review found NEXT DOOR to this PR: `viewItems` in target-menu.ts was a hand-written four-element array — the one per-view list with no compile-time guard. Dock's panel map and SessionTab's icon map are both `Record<DockView, …>`, so a sixth view fails to compile there; added to viewItems it compiled clean and was simply absent from every ⋯ menu, on every surface, with nothing to catch it. Now derived from VIEWS. `needsYouReason` stops enumerating phases. `provision_phase` is nulled on the first successful tmux probe, so non-null already means "still coming up, or failed" — which also covers queued/creating/starting, dropped by the old list while its own comment argued for including them. Nothing left to drift. `PaletteRow` carried each row's target three times: as `action.scope`, as a `scope` field always identical to it, and as a `colourKey` that was `targetKey` of the same thing. One `rowKey(row)` accessor replaces both fields. Drilling is keyed on that too, which deletes the one-caller `sameScope`, a non-null assertion and a guard. `GET /api/previews/pending` returned `{previews}` — the only list GET in the codebase that wraps; all eight others return a bare array. Unwrapped, and the one-off generic in the client goes with it. It also had no test, the only new server surface in this PR without one; now covered for both the fleet-wide pending set and the empty case. The header no longer tests `pathname === '/'` to decide whether to show its Ctrl-K chip. AGENTS.md calls out this exact file for deriving visibility rather than hardcoding a route — a literal there once hid Settings from the operator who needed it. The palette now reports its own mount, so the chip follows what exists rather than where it is mounted. Falls out of that: the Ctrl-K chord is claimed from the browser ONLY on a page that has a palette, verified. Smaller: `groupRows` already returns the shape the identity `.map()` rebuilt; `query.trim()` was recomputed five times beside a `searching` that was used once; the reset effect no longer lists `drill`, which drillInto/drillOut already handle synchronously (an effect would paint one frame on the old row first); GROUP_ORDER is module-private; a dead class attribute and a stray blank line. Reviewed and left alone, with reasons: the fleet-wide read and its absent try/catch (single operator by design, and its GET siblings do the same); PaletteAction's two-member union (a third member is a compile error at the destructure); building the index per open rather than caching.tmux resolves the window half of a `session:window` target as an INDEX first, falling back to a name match only if that fails. Session names are git refs, and `1` / `42` are legal refs the create form accepts, so both remaining name-based targets could act on a different session's window. Measured on tmux 3.3a with windows `0:workspace 1:alpha 2:"1"`: `nexus:1` and `nexus:=1` both resolve to **alpha** (the `=` exact-match prefix does not help), while `@2` is the window actually named `1`. `send-keys -t nexus:1 -l TEXT` was delivered into alpha's pane. - artifacts/tmux-nudge.ts: an operator nudge aimed at the session named `1` was typed into another agent's claude prompt, and Enter pressed. The list-windows call that already verified the window now also carries `#{window_id}`, and the sends target `@N`. The `{delivered:false, reason:'window not found'}` contract is unchanged. - terminal/exec-pty.ts: the browser terminal attached to and displayed the wrong session's pane, so the operator typed into an agent they had not picked. `buildTmuxAttachArgv` stays a pure argv builder and now takes the id; `openTerminalPty` resolves it (one extra exec per terminal-open, not a poll path) and rejects when no window carries the name rather than falling back — handleTerminalUpgradeCore already answers an openPty rejection with `500 failed to open terminal`, so no new failure mode. `VIEW:@ID` still leaves the original `nexus` session's own active-window pointer untouched, so the grouped-session shape is otherwise unchanged. lib/tmux-window.ts holds the shared `-F` format and its parser so the measurement is documented once instead of at each call site.The refcount was the one part of the poll consolidation argued from the code rather than observed. A browser reaches "both subscribers mounted" trivially and "neither mounted" almost never — the only route to it is the lock screen — so the branch that actually clears the interval had never run under test or in a browser. A stranded interval polls forever and surfaces nowhere. Five cases, each negative-controlled by breaking the guard it covers: - drop the `--count === 0` guard, so any leave clears the timer -> "keeps polling while ONE subscriber remains" fails - drop the idempotent-leave guard -> "survives a double leave without stranding the timer" fails - never clearInterval -> "STOPS polling when the last subscriber leaves" fails, "expected 7 to be 1" - drop the `++count === 1` guard, so every subscribe starts a timer -> "runs ONE timer for two subscribers" fails, "expected 6 to be 3" — which is the two-polls-per-tick regression this change removed