[Spec 786] Multi-architect lifecycle, persistence, and UX#822
Merged
Conversation
…t, afx status scope, client path)
…ve-tab MUST, docs surfaces)
…erved 'main', removeArchitect helper, clearRuntime split
…ECT_NAME on shellper auto-restart
…tTerminalsForWorkspace reconnect path + fix stale comment
…ect to /api/state + modal flow tests
…n + loadState collection-aware + afx status enumeration
…t entries + shared TerminalEntry type + status-naming Phase 5 tests
…ree + per-name terminal slots + right-click remove
…n remove + vscode unit tests
….md + CHANGELOG + verify-scenarios.md
…ace + afx-open + afx-tower-stop CHANGELOG + afx status section
…oveArchitect + openArchitect handles stale terminalId
…llyStopping flag Architect's integration CMAP found that killTerminalWithShellper only sends SIGTERM; node-pty's 'exit' event fires later on its own tick. The finally block in stopInstance was clearing the intentionallyStopping flag before the cascaded exit handlers ran, so they read the flag as false and deleted the persisted state.db.architect rows — defeating Phase 3's persistence story. Fix: register a waitForTerminalExit(terminalId) promise BEFORE each kill, then await Promise.all before letting finally clear the flag. 5s per-terminal timeout safety so a stuck process can't block shutdown indefinitely. Applied symmetrically to removeArchitect (race was harmless there due to idempotent setArchitectByName, but keeping the paths symmetric prevents future divergence). Behavioral test exercises real async timing via an EventEmitter mock that emits 'exit' on the next tick — captures the flag at exit-time, asserts it's still true. Without the fix this test would observe false, catching the production failure mode that unit tests with synchronous mocks miss.
…FORE kill loop Codex PR iter-2 caught a second-order race in the same family as the stopInstance one. handleWorkspaceStopAll kills then clears the registry synchronously, but the cascaded architect exit handlers run later and recover the architect name by scanning currentEntry.architects — which was already cleared. So the name lookup returns null, setArchitectByName(name, null) never runs, and stale state.db.architect rows survive what's supposed to be a full wipe. Fix: iterate entry.architects.keys() and explicitly call setArchitectByName(name, null) BEFORE the kill loop. Idempotent — even if the exit handler somehow runs first, the second deletion is a no-op. Regression test pins the ordering at the source level: brace-match handleWorkspaceStopAll, assert the delete loop exists, assert it comes before the kill loop. The existing 'no intentionallyStopping reference' sentinel was insufficient — it only covered half the full-wipe property.
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
Multi-architect feature was underbaked after Specs 755/761/774. This PR closes the lifecycle, persistence, and UX gaps so a user can add, manage, evict, and recover sibling architects with the same fluency as builders.
Closes #786
Closes #764 (folded into Phase 4 — solo-architect tab label restored to
'Architect'at N=1)Changes
Seven phases, each committed independently:
validateArchitectNamereserved'main',removeArchitecthelper,clearRuntimesplit)CODEV_ARCHITECT_NAMEre-injection intower-terminals.tsreconciliation paths)launchInstancereconciliation,stop.ts→clearRuntime)remove-architect+ dashboard UX + Mobile: solo-architect tab label regresses from 'Architect' to 'main' after #762 #764 (CLI + RESTDELETE+ Tower handler + dashboard close button + confirmation modal + active-tab fallback + solo-architect label)architectName/pid/port/terminalId,loadStatecollection-aware,afx statusshows all architects)codev.openArchitectTerminal, newcodev.removeArchitect+ right-click context menu)agent-farm.md,arch.md, CHANGELOG,verify-scenarios.mdwith 12 manual round-trip scenarios)Testing
codev/projects/786-.../verify-scenarios.md(12 scenarios covering headline round-trip, persistence, crash recovery, permanent exit, naming, architect-to-architect, surface enumeration, dashboard UX, VSCode UX, stop-all)CMAP iterations
Spec / Plan / Review
Breaking-ish change
`afx workspace stop` no longer wipes the architect registry (now uses `clearRuntime()`). Callers that wanted the full-wipe behaviour should switch to the dashboard's stop-all (or POST the workspace stop API) or call `clearState()` directly. See CHANGELOG `[Unreleased]` for the full list.