Skip to content

fix + feat: surface activation failures; builder terminal lifecycle improvements#682

Merged
waleedkadous merged 4 commits into
mainfrom
feature/vscode-ext-improvements
Apr 23, 2026
Merged

fix + feat: surface activation failures; builder terminal lifecycle improvements#682
waleedkadous merged 4 commits into
mainfrom
feature/vscode-ext-improvements

Conversation

@amrmelsayed
Copy link
Copy Markdown
Collaborator

@amrmelsayed amrmelsayed commented Apr 19, 2026

Four commits on this branch:

1. [Spec 0602] feat: emit and handle builder-spawned SSE event (1c6285e)

Tower broadcasts a builder-spawned notification from handleTerminalCreate after a new builder terminal is registered, carrying terminalId, roleId, and workspacePath. The VS Code extension subscribes via the existing SSE wiring, filters by active workspace, dedups by terminalId, and dispatches based on the new codev.autoOpenBuilderTerminal setting:

  • notify (default): show a toast with an Open Terminal button
  • auto: open the terminal in the right editor group immediately
  • off: ignore (sidebar still updates via overview-changed)

Older clients silently ignore the new event; older Tower instances never emit it. No version handshake required.

Sample

image

2. fix: surface architect spawn failures from workspace activation (ae427a8)

launchInstance used to swallow architect spawn errors and return success, so clients saw "workspace activated" even when the architect never started. It now returns { success: false, error, adopted } and logs at ERROR. The VS Code extension's ConnectionManager.activateWorkspace surfaces activation failures via vscode.window.showErrorMessage in addition to writing them to the output channel.

Why

Reproduced on a workspace with a legacy af-config.json: extension logged "Workspace activated" successfully, then the user got the generic "No architect terminal found" warning when opening the architect. Tower's log had the real cause — af-config.json is no longer supported. Run 'codev update' ... — thrown from buildArchitectArgs and swallowed by the outer catch.

Two layers of silent failure combined:

  1. tower-instances.ts:470-473 — outer catch logged WARN but still returned success: true.
  2. connection-manager.ts:147-159 — activation errors only went to the Codev output channel, which users rarely open.

Sample

image

3. [Spec 0602] fix: dispose stale builder terminal on re-spawn (6566d82)

When a builder is re-spawned (same roleId, new Tower terminalId), TerminalManager.openBuilder previously focused the cached terminal — which still pointed at the dead Tower session. The user saw a disconnected terminal even though Tower had a live session at the new id.

Now openBuilder compares the cached terminal's id against the incoming terminalId; on mismatch it closes the PTY, disposes the VS Code terminal, and opens a fresh one against the new session. Same-id case still just focuses (no flicker for benign repeats).

4. [Spec 0602] feat: clickable builder names in terminal output (0af4b9e)

Adds a TerminalLinkProvider that matches Codev builder role names (builder-spir-153, builder-bugfix-42, etc.) in any terminal and opens the corresponding builder terminal on click.

Resolves the role to a live terminalId via TowerClient.getWorkspaceState and delegates to TerminalManager.openBuilder (which handles stale terminals correctly thanks to commit 3). Works in the architect's own terminal — when Claude prints a list of builders, each name becomes a one-click target.

Sample

image

Test plan

  • pnpm --filter @cluesmith/codev test -- tower-instances tower-routes — 2473 passed, 13 skipped, 0 failed.
  • Manual: built extension, reopened a workspace with legacy af-config.json. Toast appears: Codev: Workspace activation failed: Failed to create architect terminal: af-config.json is no longer supported. Run 'codev update' to migrate to .codev/config.json.
  • Sanity: reopen a healthy workspace and confirm no toast appears and the architect opens normally.
  • Builder auto-open: spawn a builder via afx spawn, confirm the three modes (notify/auto/off) behave as described.
  • Clickable builder names: in any terminal, type or paste builder-spir-153 (or any active builder role); confirm the name is underlined and Cmd-click opens that builder's terminal.

Tower broadcasts a builder-spawned notification from handleTerminalCreate
after a new builder terminal is registered, carrying terminalId, roleId,
and workspacePath.

The VS Code extension subscribes via the existing SSE wiring, filters
by active workspace, dedups by terminalId, and dispatches based on the
new codev.autoOpenBuilderTerminal setting:
  - notify (default): show a toast with an Open Terminal button
  - auto: open the terminal in the right editor group immediately
  - off: ignore (sidebar still updates via overview-changed)

Older clients silently ignore the new event; older Tower instances
never emit it. No version handshake required.
tower-instances.ts used to swallow architect spawn errors and report
launchInstance success, so clients (dashboard, afx, VS Code extension)
saw "workspace activated" even when the architect never started. The
real cause — e.g. legacy af-config.json, or claude missing from
Tower's PATH — stayed buried in ~/.agent-farm/tower.log.

- launchInstance now returns { success: false, error, adopted } when
  architect spawn throws, and logs at ERROR.
- The VS Code extension's ConnectionManager.activateWorkspace shows
  the error as a toast via showErrorMessage in addition to logging.
- Tests cover the new failure contract at both the launchInstance and
  /api/workspaces/:path/activate route levels.
@amrmelsayed amrmelsayed changed the title fix: surface architect spawn failures from workspace activation fix + feat: surface activation failures; auto-open builder terminals Apr 19, 2026
When a builder is re-spawned (same roleId, new Tower terminalId),
TerminalManager.openBuilder previously focused the cached terminal —
which still pointed at the dead Tower session. The user saw a
disconnected terminal even though Tower had a live session at the
new id.

Now openBuilder compares the cached terminal's id against the
incoming terminalId; on mismatch it closes the PTY, disposes the
VS Code terminal, and opens a fresh one against the new session.
Adds a TerminalLinkProvider that matches Codev builder role names
(builder-spir-153, builder-bugfix-42, etc.) in any terminal and opens
the corresponding builder terminal on click.

Resolves the role to a live terminalId via TowerClient.getWorkspaceState
and delegates to TerminalManager.openBuilder. Works in the architect's
own terminal — when Claude prints a list of builders, each name is now
a one-click target.
@amrmelsayed amrmelsayed changed the title fix + feat: surface activation failures; auto-open builder terminals fix + feat: surface activation failures; builder terminal lifecycle improvements Apr 19, 2026
@amrmelsayed amrmelsayed marked this pull request as ready for review April 19, 2026 02:56
@waleedkadous
Copy link
Copy Markdown
Contributor

Thanks for this — the activation-failure surfacing alone is a real UX win, and the builder-spawned / click-to-open / stale-terminal improvements together make the extension feel much more coherent. Three reviewers (Codex, Claude, Gemini) did full integration passes.

Two things I'd like to see addressed before merge:

  1. SSE rate-limiter can drop builder-spawned events. sse-client.ts coalesces dispatches behind pendingRefresh — if another SSE event is already pending when a builder-spawned arrives, the toast and auto-open path is skipped (the sidebar still updates via the coalesced overview-changed, but the new UX is lost). Codex and Claude both flagged this. Suggestion: dispatch payload-carrying events out-of-band from the refresh coalescer, or exempt builder-spawned from drop-on-pending. This is the first SSE event with a unique payload that can't just be replayed via overview-changed, so worth addressing now rather than later.

  2. TerminalLinkProvider — confirm TowerClient.getWorkspaceState() is not called inside provideTerminalLinks. That hook runs on every terminal paint; calling Tower there would spam state requests. Safer: underline all role-name matches blindly during render and only resolve the live terminalId in handleTerminalLink (click handler). Worth a quick grep to confirm the current placement. (Gemini.)

Non-blocking follow-ups:

  1. Terminal race in openBuilderonDidCloseTerminal for a disposed stale terminal can fire after the replacement is registered under the same key, dropping it. One-line guard: if (t === terminal && this.terminals.get(mapKey)?.terminal === terminal). (Claude.)
  2. VS Code logic untestedBuilderSpawnHandler, link-provider, stale-terminal replacement lack automated coverage. The parsing/dedup/workspace-filtering in BuilderSpawnHandler is pure enough to unit-test without the full VS Code API.
  3. Normalize workspace paths in BuilderSpawnHandler — Tower passes raw workspacePath, so symlink/path-form mismatches could cause events to be silently ignored.
  4. Error toast on SSE reconnect — if the workspace is persistently broken (e.g. legacy af-config.json), the activation-failure toast may re-fire each reconnect. Probably fine but worth watching. (Gemini.)

Everything else checked out: launchInstance behavior change is correct and all three callsites handle it; error chain traces end-to-end; SSE additive wire change is backward-safe.

Copy link
Copy Markdown
Contributor

@waleedkadous waleedkadous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — concerns above are pre-merge asks, not blockers on my end. Merge when you've addressed #1 and confirmed #2.

@waleedkadous waleedkadous merged commit 965a7e6 into main Apr 23, 2026
6 checks passed
waleedkadous added a commit that referenced this pull request May 11, 2026
chore(vscode): reliability + UX cleanup, PR #682 follow-up, bugfixes #718/#728
timeleft-- pushed a commit to MachineWisdomAI/codev that referenced this pull request May 12, 2026
The 1s SSE-layer rate limiter coalesced multiple events into a single
deferred dispatch with the most-recent event's data, silently dropping
intermediate events. With the new `builder-spawned` payload event, this
manifested as toast/auto-open being skipped when an `overview-changed`
arrived in the same window (PR cluesmith#682 review cluesmith#1).

Removing the throttle entirely instead of exempt-listing payload types:
the consumers already self-throttle where it matters (overview-data has
a `loading` guard, tree views fire EventEmitters that VS Code batches),
so the SSE-layer coalescing was solving a problem that wasn't there
while creating a class of dropped-event bugs.
timeleft-- pushed a commit to MachineWisdomAI/codev that referenced this pull request May 12, 2026
…e-race on stale-replace

When openBuilder disposes a stale terminal and immediately registers a
replacement under the same mapKey, the stale terminal's onDidCloseTerminal
event can fire after registration — unmapping the live replacement (PR
cluesmith#682 review cluesmith#3).

Identity-check the current map entry before deleting; pty.close() still
runs against the closed-over stale pty.
timeleft-- pushed a commit to MachineWisdomAI/codev that referenced this pull request May 12, 2026
…ce paths + add unit coverage

`payload.workspacePath !== active` was a strict string compare, so
trailing-slash and `..`-segment variants from Tower silently dropped
events (PR cluesmith#682 review cluesmith#5). Switch to `path.resolve` on both sides.
Symlink realpath is intentionally out of scope — Tower passes canonical
paths and realpath would add sync FS I/O on every event.

Adds a test file covering parsing/dedup/path-normalization/mode-dispatch
(PR cluesmith#682 review cluesmith#4), the only handler surface that's pure enough to
unit-test without spinning up the extension host.
timeleft-- pushed a commit to MachineWisdomAI/codev that referenced this pull request May 12, 2026
…r purity

Regression pin for PR cluesmith#682 review cluesmith#2: provideTerminalLinks fires on
every terminal paint so it must stay pure. The current code already
matches Waleed's preferred design (Tower lookup lives in
handleTerminalLink), but a unit test guards against future refactors
that pull connection state into the render path.

Also covers the obvious regex cases (single match, multi-match per
line, non-builder text, lastIndex reset across calls) — review cluesmith#4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants