[Spec 823] Multi-architect coordination: builder attribution, messaging docs, builder thread, VSCode add-refresh#824
Merged
Merged
Conversation
…MUST + delivery rationale + sse.ts dependency)
…commit/retention + removeArchitect hedge)
…mit default reconciliation + spawn.ts path typo)
…ttern + signature refactor + test harness)
…ution Adds spawnedByArchitect field to OverviewBuilder/BuilderOverview types, populated by extending the existing overview SQL enrichment block in overview.ts. Drops the WHERE issue_number IS NOT NULL clause so soft-mode builders also enrich (iter-1 Gemini finding). BuilderCard renders an inline attribution span '<id> · <architect-name>' when architectCount > 1, with a hover-tooltip carrying the full 'spawned by ...' text. WorkView threads architectCount = state.architects?.length ?? 0 (null-safe) to each card. CSS keeps the N=1 baseline intact via min-width: 60px on .builder-col-id, allowing the cell to grow naturally at N>1. 6 new BuilderCard tests + 4 new overview SQL enrichment tests cover the attribution rendering matrix and the soft-mode enrichment path.
…(N=1/2/3) + strengthen N=1 baseline assertion (iter-1 CMAP) Iter-1 Codex REQUEST_CHANGES: plan called for Playwright N=1/N=2/N=3 visual coverage and a stronger N=1 baseline parity check. Both addressed: (1) New Playwright test at packages/codev/src/agent-farm/__tests__/e2e/spec-823-builder-attribution.test.ts mocks /api/state + /api/overview to render builder rows at three architect cardinalities. Asserts: N=1 has zero .builder-attribution spans (baseline parity), N=2 renders attribution only on non-null spawnedByArchitect rows (legacy null row excluded), N=3 renders six spans across three sibling architects, and the N=1→N=2 layout transition does not collapse the table. Follows the same /api/state mocking pattern as architect-pane-layout.test.ts (#766 regression). (2) BuilderCard.test.tsx N=1 baseline test now also asserts: id cell's textContent equals exactly '#823' (no extra whitespace, no trailing separator), and idCell.children.length === 0 (no <span> child inserted). Strengthens the plan's 'matches the pre-823 baseline' requirement beyond just absence-of-attribution.
…instruction Adds a 'Thread file' section to both codev-skeleton/roles/builder.md (source of truth for external adopters; copied to packages/codev/skeleton/roles/builder.md at build) and codev/roles/builder.md (project-local copy). Both files updated atomically and remain byte-identical (diff confirms). The section instructs every builder to maintain codev/state/<builder-id>_thread.md as a free-text markdown log relative to their worktree. Resolution rule (basename of pwd), directory creation (Write tool's mkdir -p semantics), what/when to write (free-form, phase boundaries), discovery (in-flight via .builders/<id>/codev/state/, post-merge via codev/state/ on main), and commit/retention (default COMMIT, rare opt-out only when noise) are all spelled out. No porch hooks, no schema enforcement. copy-skeleton build step verified: packages/codev/skeleton/roles/builder.md contains the new section (1 match for 'Thread file').
…essaging primitives Adds a new 'Inter-agent messaging' section to CLAUDE.md and AGENTS.md (byte-identical), with equivalent adopter-friendly content in codev-skeleton/templates/CLAUDE.md and codev-skeleton/templates/AGENTS.md. Extends codev/resources/commands/agent-farm.md afx send section with the architect:<name> form, an inter-architect example, and a 'Discovering active agents' subsection that points at codev/state/<builder-id>_thread.md. Documents the four addressing forms (<builder-id>, architect, architect:<name>, <workspace>:architect), the architect-vs-builder distinction on architect:<name> per the spoofing check at tower-messages.ts:213-218, and a concrete main → architect:ob-refine sibling-architect example. Acceptance criteria all pass: 'architect:<name>' appears in 5/5 files; 'spoofing' in 5/5; 'codev/state/' in 5/5; CLAUDE.md and AGENTS.md byte-identical. copy-skeleton build step verified: skeleton/templates/CLAUDE.md and skeleton/templates/AGENTS.md both carry the new section.
…ns (afx send architect main-fallback accuracy + explicit ls codev/state/ post-merge discovery) Codex iter-1 REQUEST_CHANGES — two findings addressed: (1) agent-farm.md afx send architect 'Named target' bullet said non-builder senders route to 'main'. Verified actual code at tower-messages.ts:281-348 falls back to the first registered architect when 'main' is absent. Repo-root CLAUDE.md/AGENTS.md already documented this correctly; agent-farm.md was the outlier. Now reads: 'routes to the architect named main if present, else the first registered architect'. (2) Post-merge thread discovery had no explicit ls command. Plan called for pointing readers to both in-flight (ls .builders/*/codev/state/*.md) AND post-merge (ls codev/state/) discovery. All five files now spell out the post-merge path explicitly: 'list with ls codev/state/ and read with cat codev/state/<builder-id>_thread.md from the main checkout.' copy-skeleton verified: both shipped templates carry the updated content.
…E event + VSCode subscriber
Tower-side: handleAddArchitect and handleRemoveArchitect both accept ctx: RouteContext, and emit ctx.broadcastNotification({ type: 'architects-updated', body: JSON.stringify({ workspace }), workspace }) on success. No emit on failure. No emit from launchInstance (subscribers re-fetch on VSCode activate). Payload mirrors worktree-config-updated's shape exactly (per worktree-config-watcher.ts:60-65).
VSCode-side: WorkspaceProvider's existing connectionManager.onSSEEvent subscriber now matches BOTH worktree-config-updated and architects-updated envelope types via a single shared callback (single parse, two checks). Fires changeEmitter unconditionally — no workspace filter at the SSE layer (mirrors existing pattern; WorkspaceProvider is workspace-scoped at construction).
5 new tower-routes tests assert: add emits on success, add does NOT emit on failure, remove emits on success, remove does NOT emit on failure, payload carries workspace path. 4 new vscode workspace tests assert: subscriber matches both envelope types, single onSSEEvent call (no duplicate subscriber), JSON.parse safety preserved, no workspace filter at the SSE layer.
verify-scenarios.md Scenario 11 updated: the 'tree may not refresh automatically' caveat is replaced with 'refreshes automatically within ~1s' — Phase 4 closes the gap noted at #786 PR-iter-3.
… all remove paths + runtime behavior tests
iter-1 Codex REQUEST_CHANGES — two findings addressed:
(1) Other remove paths weren't emitting architects-updated. Found two: handleWorkspaceRoutes DELETE /api/architects/:name (dashboard close-button + confirmation modal flow, tower-routes.ts:1486-1499) and handleWorkspaceTabDelete DELETE /api/tabs/architect:<name> (mobile TabBar close, :2140-2157). Both already had ctx in scope; added the same ctx.broadcastNotification emit pattern on success only.
(2) VSCode test was source-text grep only; reviewers wanted runtime behavior coverage. Added packages/vscode/src/__tests__/workspace-sse-subscriber.test.ts — vi.mock('vscode') sets up a fake EventEmitter; ConnectionManager is mocked to capture the onSSEEvent callback; WorkspaceProvider is instantiated and the changeEmitter.fire is spied. 9 new tests exercise:
- exactly one onSSEEvent subscription registered
- fires on architects-updated envelope
- fires on worktree-config-updated envelope (#786 regression)
- does NOT fire on unrelated types (overview-changed, builder-spawned, heartbeat, notification)
- does NOT throw and does NOT fire on malformed JSON
- does NOT fire when envelope.type is missing
- fires regardless of envelope.workspace value (no workspace filter at SSE layer)
- fires on multiple sequential events (no de-duplication)
- refresh() fires changeEmitter directly (#786 imperative refresh regression)
The existing source-grep tests in workspace.test.ts are kept as cheap structural guards.
iter-1 Gemini REQUEST_CHANGES (test harness): the plan iter-1 Codex 'NOT vitest' guidance was over-specific — packages/vscode/src/__tests__/ IS a vitest harness per vitest.config.ts (added in #786 Phase 6 with the explicit comment 'mock the vscode module entirely'). The new behavior tests follow that intended pattern.
…thread file Review covers all 4 phases: spec compliance, deviations from plan (test harness reframing, expanded emit sites, CLAUDE.md insertion-point), lessons learned (CMAP discipline paid off, rebase was clean, plan-claim verification matters), technical debt (vscode mock factory deferred, thread accumulation deferred to MAINTAIN), consultation feedback (5 spec iters + 2 plan iters + 4 impl iters all summarized with no findings rejected), flaky tests (session-manager + scrollController, both pre-existing). Also includes the spir-823 thread file at codev/state/spir-823_thread.md per Phase 2's own MUST — this builder maintained a thread throughout the project as proof-of-concept for the cohort's situational-awareness surface. Default commit/retention rule applies.
arch.md: Added Spec 823 Phase 4 to the VSCode extension section (architects-updated SSE event closes the prior CLI-add-while-VSCode-open stale-tree gap). Added a new 'Tower SSE Event Conventions' subsection documenting the JSON-envelope-on-data-with-no-event-name pattern that worktree-config-updated, architects-updated, and builder-spawned share — previously inferrable from worktree-config-watcher.ts + workspace.ts but not named in arch docs. lessons-learned.md: Added two entries to the 3-Way Reviews section: - Reviewers can hallucinate code patterns when summarizing unfamiliar files (the spec iter-1 Claude SSE-pattern mis-read that propagated to the plan before plan-iter-1 caught it). - Plan-claimed test harnesses can be wrong (the 'vscode-test, NOT vitest' guidance that turned out to be over-specific against the actual repo state).
… paths (Codex COMMENT) Codex's review iter-1 COMMENT-level finding: the two newly added emit paths from Phase 4 iter-1 corrections (workspace-scoped dashboard DELETE /workspace/<encoded>/api/architects/:name and mobile tab-close DELETE /workspace/<encoded>/api/tabs/architect:<name>) weren't directly tested. The top-level /api/workspaces/<encoded>/architects/... routes were tested in handleAddArchitect/handleRemoveArchitect; the workspace-scoped variants weren't. Added 4 tests: - handleWorkspaceRoutes DELETE /api/architects/:name emits architects-updated on success - ...does NOT emit on failure (e.g. Cannot remove main) - handleWorkspaceTabDelete /api/tabs/architect:<name> emits architects-updated on success (with 204 status check) - ...does NOT emit on failure (e.g. architect not found) Tower-routes test suite: 80/80 pass (was 76, +4).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #786. Four coherent deliverables that finish the multi-architect coordination story:
state.architects.length > 1, each builder card renders#<id> · <architect-name>(hover-tooltip carries the full "spawned by …" text). N=1 dashboard byte-identical to pre-823.codev/state/<builder-id>_thread.mdas a free-text narrative log. No porch hooks, no schema. Instruction lives in bothcodev-skeleton/roles/builder.md(source of truth for external adopters) andcodev/roles/builder.md(project-local copy).architects-updatedSSE notification from all FOUR successful architect-mutation paths (handleAddArchitect, handleRemoveArchitect, handleWorkspaceRoutes DELETE, handleWorkspaceTabDelete). VSCode'sWorkspaceProviderextends its existingconnectionManager.onSSEEventcallback to match the new envelope type alongsideworktree-config-updated. Closes the gap noted at Multi-architect feature is underbaked — gaps in lifecycle, persistence, and UX #786 PR-iter-3 (Scenario 11).Closes #823.
Changes
Four phases, each a single atomic commit on this branch:
spawnedByArchitectfield onOverviewBuilder+BuilderOverview; SQL enrichment update (dropWHERE issue_number IS NOT NULLso soft-mode rows enrich);BuilderCardconditional render;WorkViewprop threading; CSS (min-widthnot fixedwidthon.builder-col-id); 6 unit tests + 4 overview enrichment tests + 4 Playwright visual smoke scenarios.copy-skeletonvalidation confirms shipped artifact carries the change.copy-skeletonvalidation confirms.ctx.broadcastNotification; VSCode subscriber extends to match both envelope types via single shared callback. 5 Tower-routes tests + 4 source-grep tests + 9 runtime-behavior tests (vi.mock('vscode', …)pattern).Architecture docs updated:
codev/resources/arch.md— added Phase 4 to the VSCode multi-architect section; new "Tower SSE Event Conventions" subsection documents the JSON-envelope-on-data-with-no-event-name pattern.codev/resources/lessons-learned.md— two new entries on reviewer hallucinations and plan-claimed test harnesses.The #786 verify-scenarios.md Scenario 11 wording updated to note the gap is now closed.
Testing
Unit + integration:
porch donepasses consistently across all four phases.Playwright visual smoke (
spec-823-builder-attribution.test.ts): 4 scenarios covering N=1/N=2/N=3 + layout transition. Mocks/api/stateand/api/overviewviapage.route()(same pattern as #766'sarchitect-pane-layout.test.ts).CMAP iterations
WHEREclause excluding soft-mode rows, spoofing-check semantics, CLI flag form, verify-scenarios path, SSE workspace scoping, skeleton MUST promotions, internal-contradiction fixes, SSE-subscriber pattern hallucination.ls codev/state/command. Both addressed.:1489+:2148and weak source-grep-only test coverage. Both addressed (emit added to all 4 paths; newworkspace-sse-subscriber.test.tswith 9 runtime tests).All rebuttals at
codev/projects/823-multi-architect-coordination-b/823-*-iter*-rebuttals.md. No findings rejected.Rebase note
PR #822 (#786) merged to main mid-implementation. Builder branch was created from pre-#786 main; phases 1-3 (#786-independent) landed first, then the branch was rebased onto current main before Phase 4 (which depends on #786's
removeArchitectroute handler). Force-push with--force-with-leaseper architect approval. Rebase resolved cleanly (auto-mergedapi.ts,index.css,agent-farm.md).Spec / Plan / Review
codev/specs/823-multi-architect-coordination-b.mdcodev/plans/823-multi-architect-coordination-b.mdcodev/reviews/823-multi-architect-coordination-b.mdcodev/state/spir-823_thread.mdPre-existing flakes (not from this PR)
packages/codev/src/terminal/__tests__/session-manager.test.ts(6 failures): integration tests need real shellper binary; environment-dependent.packages/dashboard/__tests__/scrollController.test.ts(1 failure): noted in Multi-architect feature is underbaked — gaps in lifecycle, persistence, and UX #786 PR description too.Neither relates to Spec 823. Porch's build+tests gate passes.