Agents view: architect-aware group-by axis (VSCode)

codev/projects/1104-vscode-merge-architects-builde/1104-review-iter1-rebuttals.md

@@ -0,0 +1,69 @@
+# PIR #1104 — Review iteration 1 rebuttals
+
+Consultation verdicts: **codex = REQUEST_CHANGES**, **claude = COMMENT**.
+
+PIR is single-pass (`max_iterations: 1`): this is not re-reviewed by the models. Real
+defects below are fixed + regression-tested; plan-deviation findings are rebutted because they
+were deliberate decisions the human made at the `dev-approval` gate. All of this is escalated to
+the human at the `pr` gate (the only remaining reviewer of these dispositions).
+
+## Codex (REQUEST_CHANGES)
+
+Codex's first four points all reduce to "the implementation deviates from the approved plan."
+That is true and **intentional**: the plan described a *nested adaptive architect tier*, and at
+the `dev-approval` gate the human reviewed it running and explicitly redirected the design to a
+*flat 3-way group-by axis*. The review file documents this pivot (Summary + "Things to Look At");
+the builder thread records the live decisions. PIR's `dev-approval` gate exists precisely to let
+"seeing it running changes the design" happen — so a plan/implementation divergence here is the
+gate working as intended, not a regression.
+
+1. **"Flat axis instead of nested architect-rooted tree."** Deliberate. The human directed
+   "pivot to the flat 3-way toggle" after reviewing the running nested tree (duplication with
+   Workspace > Architects, single-architect collapse awkwardness, and an icon-collision problem
+   drove it). The nested-tier code was fully removed. **No change** — rebutted.
+
+2. **"Childless architects dropped (should stay as interactive leaf rows)."** Deliberate. The
+   human's exact objection to the nested tree was that childless architects duplicated
+   Workspace > Architects. In the flat model a group exists only when it owns work, so childless
+   architects naturally don't appear — which is the requested behavior. The full roster remains
+   in Workspace > Architects. **No change** — rebutted.
+
+3. **"View id renamed `codev.builders` → `codev.agents` (plan said keep it)."** Deliberate and
+   human-directed ("rename the codev.builders ID to codev.agents for consistency"). All internal
+   `when`-clauses, `createTreeView`, and tests were updated together; there are no external
+   consumers of the id (it's our own extension). The only cost is that a user who manually
+   repositioned the old view returns to the default position once — acceptable, and the human
+   chose it knowingly. **No change** — rebutted.
+
+4. **"Add Architect `+` missing from the Agents title bar."** Deliberate. The human explicitly
+   rejected an Agents title-bar `+`: a `+` there is ambiguous (add-builder vs add-architect), so
+   Add Architect stays on Workspace > Architects only. **No change** — rebutted.
+
+5. **"No-workspace `/api/overview` branch returns a payload missing `recentlyClosed` /
+   `architects`, violating the declared contract."** **Valid — real defect, fixed.** `OverviewData`
+   now declares `architects` required ("never undefined"), but the early `if (!workspaceRoot)`
+   return in `handleOverview` emitted only `{ builders, pendingPRs, backlog }`. Fixed to emit all
+   collection fields empty (`recentlyClosed: []`, `architects: []`). Added a regression assertion
+   to the existing "returns empty data when no workspace is known" test in `tower-routes.test.ts`
+   (now checks `recentlyClosed`/`architects` === `[]`). Pinning test fails without the fix.
+
+## Claude (COMMENT)
+
+Both of Claude's points are real doc-staleness bugs introduced by my own pivot (not plan
+deviations). Both fixed.
+
+1. **"README.md Agents description still describes the retired nested tier."** **Valid — fixed.**
+   The paragraph ("builders nest under the architect… passive architect appears as a leaf row…")
+   predated the pivot. Rewritten to describe the flat 3-way group-by axis and the
+   architects-with-work-only behavior.
+
+2. **"`buildersGroupBy` setting description references a 'pressed' button state that doesn't
+   exist."** **Valid — fixed.** VS Code toolbar buttons have no pressed state (the very lesson this
+   PR recorded). Rewritten to describe the single cycling group-by button (icon shows the next
+   axis; cycles stage → area → architect).
+
+## Net
+
+- 3 real fixes (1 code + regression test, 2 docs), committed on the PR branch.
+- 4 plan-deviation findings rebutted as deliberate `dev-approval`-gate decisions.
+- Escalated to the human at the `pr` gate per PIR single-pass design.

codev/projects/1104-vscode-merge-architects-builde/status.yaml

@@ -0,0 +1,29 @@
+id: '1104'
+title: vscode-merge-architects-builde
+protocol: pir
+phase: review
+plan_phases: []
+current_plan_phase: null
+gates:
+  plan-approval:
+    status: approved
+    requested_at: '2026-06-27T12:00:20.441Z'
+    approved_at: '2026-06-27T12:13:50.827Z'
+  dev-approval:
+    status: approved
+    requested_at: '2026-06-27T12:39:09.461Z'
+    approved_at: '2026-06-28T01:14:44.858Z'
+  pr:
+    status: pending
+    requested_at: '2026-06-28T01:29:33.253Z'
+iteration: 1
+build_complete: true
+history: []
+started_at: '2026-06-27T11:55:09.921Z'
+updated_at: '2026-06-28T01:29:33.254Z'
+pr_history:
+  - phase: review
+    pr_number: 1106
+    branch: builder/pir-1104
+    created_at: '2026-06-28T01:20:31.045Z'
+pr_ready_for_human: true

codev/resources/arch.md

@@ -1096,6 +1096,7 @@ The VS Code extension (`packages/vscode`) is a thin client over Tower's existing
 - **Startup CLI preflight (#791)**: On `activate()` the extension verifies the `codev` CLI is installed and at least its own `package.json` version (`codev --version`, resolved like `resolveAfxPath`, cached per session, 400ms-bounded, fire-and-forget so activation never blocks). Missing → `Get started with Codev` walkthrough; outdated → upgrade notification; either dismissed → CLI-dependent commands no-op with one "run setup" toast. Commands register through two helpers — `reg` (unguarded) and `regCli` (guarded) — so the registrar name *is* the guard policy (no separate list). Preflight also sets the `codev.cliReady` context key, which drives the walkthrough's Verify-step completion. Lives in `src/preflight/` (`preflight-core.ts` pure + unit-tested, `preflight.ts` vscode glue).
 - **Markdown Preview / artifact-canvas host integration (#859)**: The first integration of the shared `@cluesmith/codev-artifact-canvas` React surface into a host. A read-only `CustomTextEditor` (`codev.markdownPreview`, `priority: "option"` so it never replaces the default `.md` editor; selector scoped to `**/codev/{specs,plans,reviews}/**/*.md`) renders an artifact and lets a reviewer add comments by hovering a block and clicking `+`. It is the extension's **first bundled React webview**: a *second* esbuild entry (`esbuild.js`, browser/IIFE, bundles react/react-dom/the canvas + emits `markdown-preview.css`), type-checked by a dedicated `tsconfig.webview.json` (DOM libs) since the host `tsconfig.json` excludes the browser dir. Host↔webview bridge (`markdown-preview/preview-provider.ts`, HTML in a sibling `preview-template.ts` per #920): the host posts **raw** document text + parsed markers; the webview mounts `<ArtifactCanvas>`; `onAddComment(line)` → host `showInputBox` → `WorkspaceEdit` → the round-trip goes through the file text. **Cross-cutting invariants this established**: (1) the on-disk REVIEW-marker convention (`<!-- REVIEW(@author): text -->`, a marker annotates the nearest non-marker line above it) lives in `@cluesmith/codev-core/review-markers` so every host (vscode now, dashboard later) writes/parses identical bytes — the editor Comments-API path (`comments/plan-review.ts`) shares it. (2) The canvas renderer runs markdown-it with **`html: true` + DOMPurify as the sole guard** (#1042 amends spec-945 D7: safe static HTML renders, scripts/handlers/`javascript:` stripped, document JS never executes) and **strips full-line HTML comments before block parsing** with a cleaned→original line map (#1036), so markers never render as text and never split a multi-line block while `data-line` stays accurate.
 - **Builders diff-review: navigation + active-file sync (#1060/#1066)**: The "current builder/file" in a diff review is **derived from the active editor, not stored**. The diff-inject registry (`diff-inject-codelens.ts`) maps each right-side worktree fsPath → `{ builderId, relPath, hunks }`; because a worktree-absolute path is unique per builder, two builders that changed the same relative path stay distinct. Everything keys off `getDiffInjectEntry(activeEditor.fsPath)`: cross-file keyboard nav (`codev.diffNextFile`/`diffPreviousFile`, #1060) and the Builders-tree active-file reveal (#1066). Navigation walks **one builder's** changed-file list (it never crosses builders) in the **visible tree order** — depth-first via `flattenTreeOrder(buildFilePathTree(...))` in tree-view mode, raw git `--name-status` order in flat mode — and **wraps** at both ends (`computeNavTarget` modulo) to match VSCode's built-in hunk navigation, which also wraps. The reveal (#1066, Explorer-style, gated by `codev.buildersAutoReveal`) requires two pieces of TreeView groundwork: file rows carry a stable `<builderId>::<relPath>` id and `BuildersProvider.getParent` reconstructs the full chain (file → compacted folder(s) → builder → group), since `reveal` matches by id and walks parents while the subtree is still collapsed. It fires on **both** the active-editor change AND the diff-inject registry change, because a programmatic diff open registers its entry *after* the editor activates (same dual-trigger the lens context-key sync uses).
+- **Agents view + group-by axes (#1104)**: The sidebar's primary work view is **Agents** (view id `codev.agents`, formerly `codev.builders` — only the user-facing id/label changed; internal symbols like `BuildersProvider` and the `codev.buildersGroupBy` setting keep their names). It groups in-flight builders by **exactly one of three axes** (`stage` | `area` | `architect`), each a `BuilderGrouping` strategy in `views/builder-grouping.ts`; `architect` groups by `spawnedByArchitect` (null folds into `main`, matching the affinity router) and a childless architect produces no group, so the work view shows owners-of-work while the *full* roster stays in Workspace > Architects. The axis is a single toolbar button (see the no-`toggled` lesson). The architect roster the Add-Architect flow needs rides on **`/api/overview` as `architects: ArchitectState[]`** (additive wire field), built by a shared `liveArchitects(entry, manager)` helper in `tower-routes.ts` reused by the `/api/state` dashboard-state path so the two payloads can't drift — `liveArchitects` (running-session set, from `terminal_sessions`) is distinct from state.ts's `getArchitects` (persisted `architect` table). `Codev: Add Architect` is conversational: it resolves `main` from that roster and routes a request via `sendMessage('architect:main', …)` rather than creating the architect directly.
 - **Running-Tower version probe (#983)**: A second preflight dimension catches the case the CLI check structurally can't — an `npm install -g` upgrade that updated the on-disk binary but left **Tower running stale in-memory code**. Tower exposes read-only `GET /api/version` (`{ version, startedAt }`, wire type `TowerVersionInfo` in `codev-types`, served from `RouteContext` so it reports the *running* process's version, not the disk binary; unauthenticated like `/health`). On each `connected` transition the extension probes it (`TowerClient.getVersion()`, returning the raw `{ status }` so the preflight distinguishes a 404 "Tower too old to report" from an unreachable Tower). Divergence fires **only on `running < installedCLI`** — the case a restart actually fixes; running-vs-extension is left to #791 (a restart can't load code that isn't installed). The toast offers a `Restart Tower` action (`afx tower stop && afx tower start`, local host only — safe to self-invoke because #991 scoped `afx tower stop` to the listening process; remote hosts get informational wording). The two async inputs (installed-CLI version, running-Tower version) are reconciled against the startup race by re-probing once the CLI check resolves. Decision/wording logic is pure + unit-tested in `preflight-core.ts` (`decideTowerStatus`, `towerDivergenceMessage`).
 
 ## Repository Dual Nature

codev/resources/lessons-learned.md

@@ -335,6 +335,8 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated
 - [From #1053] When styling rendered markdown, **prose and code want opposite overflow behavior.** Long unbreakable tokens in prose (a long inline-code span, URL, or file path) overflow the column and force a *page-level* horizontal scrollbar unless you set `overflow-wrap: break-word` on the prose container — but `pre` and `table` must NOT wrap (wrapping code corrupts it; a wide table shouldn't reflow), so they opt out with their own `overflow: auto` to scroll *within their block*. The correct outcome is: prose never causes page scroll; code/tables scroll locally. (github-markdown-css uses exactly this split.)
 - [From #1053] Watch `em`-on-`em` compounding when sizing nested rendered elements. Sizing both `pre` and `code` to `0.85em` made fenced code (`pre code`) ~72% (0.85 × 0.85); the fix is to size the chip (`code`) and the block (`pre`) but **reset the inner `pre code` to `font-size: inherit`** (plus reset its chip background/padding, since code inside a fence is plain text, not an inline chip). Anchor `em` chains to a single px root and reset where they'd stack.
 - [From #841] When a TreeView row's **display label is transformed for presentation** (here architect names rendered UPPERCASE for visual consistency) so it no longer equals the canonical identifier, any command that targets the row must read the identifier from a **stable channel** — set `item.id = <prefix>-<rawId>` and have the command strip the prefix back off — **not** re-derive it from `arg.label`. The label-as-identifier shortcut works only while the label *is* the identifier; the moment a cosmetic transform diverges them, `arg.label` silently sends the wrong target downstream (e.g. a DELETE for `WEB`, a name the server knows only as `web`). The right-click context menu passes the whole `TreeItem`, so `item.id` is the natural carrier. This also future-proofs against a later switch to a `displayName` field.
+- [From #1104] **VS Code view-title toolbar buttons have no pressed/selected state.** The menu contribution schema (verified against `menusExtensionPoint.ts`) supports only `command`/`alt`/`when`/`group` — there is **no `toggled` property**, and an extension cannot override the workbench's toolbar colors. A toolbar action's icon is also fixed to its command. So to express a one-of-N selection (e.g. the Agents group-by axis), don't chase a pressed highlight: use a **single button whose icon/title reflect state**, implemented as N show/hide commands swapped by a `when`-clause on a context key (the pattern VS Code itself uses for View-as-Tree/List). Make the button an **action** — show the *next* state (what clicking applies), not the current one — and let the view content itself (group headers) be the primary "what am I looking at" signal. Don't overload the view title text with the state either; it truncates and clutters.
+- [From #1104] **A custom tree-row SVG renders at 16px — author for that size, and verify by rendering, not by eyeballing a large preview.** A radial 8-arm octopus looked great at 200px and collapsed into an indistinct asterisk at 16px (eyes vanished entirely). Rasterize the candidate (e.g. Playwright screenshot) on both light and dark backgrounds at *actual* 16px before shipping. Tree `iconPath` SVGs are **not** theme-tinted by VS Code, so ship a monochrome **light/dark pair** using the theme-foreground colors (`#1f1f1f` / `#cccccc`, matching the existing `codev-light`/`codev-dark`), never a hardcoded brand color; eyes/holes via a `<mask>` so they contrast on either background.
 
 ## Documentation