From 130f2d5c401aa0092f0f72a80af83005eba37ea4 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 5 Feb 2026 14:36:16 +0700 Subject: [PATCH] chore: remove stale planning docs --- packages/server/AGENT_REFACTOR_PROPOSAL.md | 154 ---- packages/server/agent-mcp-review.md | 812 --------------------- packages/server/agent-prompt-old.md | 534 -------------- 3 files changed, 1500 deletions(-) delete mode 100644 packages/server/AGENT_REFACTOR_PROPOSAL.md delete mode 100644 packages/server/agent-mcp-review.md delete mode 100644 packages/server/agent-prompt-old.md diff --git a/packages/server/AGENT_REFACTOR_PROPOSAL.md b/packages/server/AGENT_REFACTOR_PROPOSAL.md deleted file mode 100644 index 2b80d6a28..000000000 --- a/packages/server/AGENT_REFACTOR_PROPOSAL.md +++ /dev/null @@ -1,154 +0,0 @@ -# Agent Architecture Refactor Proposal - -## Status (November 30, 2025) - -- `ManagedAgent` is the single source of truth; the legacy `AgentSnapshot` type has been fully removed and no runtime APIs expose it. -- The UI, MCP server, and persistence flows now project from `ManagedAgent` via `toAgentPayload`/`serializeAgentSnapshot` and `toStoredAgentRecord`. -- `AgentSnapshotPayload` remains as the wire schema for clients, but it is now exclusively derived from `ManagedAgent` projections and never mutated manually. -- Tasks 1–6 of the refactor are complete; the notes below are kept for historical context and future audits. - -> The remaining sections describe the legacy state and the design rationale that guided the refactor. They are preserved so future contributors understand why the cleanup happened. - -## Legacy State Analysis (Pre-Refactor) - -### AgentSnapshot Usage - -| Location | Purpose | -| --- | --- | -| `src/server/agent/agent-manager.ts:231-247` | `getAgent` / `listAgents` expose `AgentSnapshot` copies generated via `toSnapshot`. | -| `src/server/session.ts:534-577, 1130-1145` | Session broadcasts snapshots to clients (`forwardAgentState`, `buildAgentPayload`). | -| `src/server/agent/agent-registry.ts:138-185` | `applySnapshot` is driven by snapshots (via `attachAgentRegistryPersistence`). | -| `src/server/persistence-hooks.ts:23-35` | Persistence hook subscribes to `agent_state` and forwards snapshots. | -| `src/server/agent/mcp-server.ts:12-452` | MCP endpoints serialize snapshots for diagnostics/listing. | -| `src/server/messages.ts:19-338` | `AgentSnapshotPayload` and `serializeAgentSnapshot` expect snapshots. | -| Tests (`src/server/persistence-hooks.test.ts`, `src/server/agent/agent-registry.test.ts`) | Fabricate snapshots as fixtures. | - -### Impacted Areas - -- `AgentManager` snapshot creation & event emission. -- Session serialization (`buildAgentPayload`, `forwardAgentState`). -- Registry persistence (`applySnapshot`, tests). -- MCP server responses. -- Messaging schema & helper utilities. - -### Why AgentSnapshot Existed - -It attempted to provide a JSON-friendly view stripped of internal handles (e.g., `AgentSession`, `pendingRun`). In practice it duplicates the data model, omits config (requiring `recordConfig`), and forces redundant copies. All downstream consumers just serialize the snapshot immediately, so it adds complexity without real protection. - -## Implemented Design - -### Single Source of Truth - -Introduce a discriminated union for `ManagedAgent` that encodes lifecycle-specific invariants: - -```ts -type ManagedAgent = - | { lifecycle: "initializing"; pendingRun: null; /* ... */ } - | { lifecycle: "running"; pendingRun: AsyncGenerator; /* ... */ } - | { lifecycle: "idle"; pendingRun: null; /* ... */ } - | { lifecycle: "error"; pendingRun: null; lastError: string; /* ... */ } - | { lifecycle: "closed"; session: null; pendingRun: null; /* ... */ }; -``` - -Fields include the live `AgentSession`, normalized config (`AgentSessionConfig`), timelines, permissions map, timestamps, persistence handles, etc. Impossible states (e.g., `pendingRun` non-null while lifecycle `"idle"`) become unrepresentable. - -### Pure Transformation Functions - -1. **Persistence Projection** - ```ts - function toStoredAgentRecord(agent: ManagedAgent, options?: { title?: string | null }): StoredAgentRecord - ``` - Copies provider, cwd, ISO timestamps, lifecycle status, `lastModeId`, complete config (`modeId`, `model`, `extra`), persistence handle, title metadata. - -2. **Client Payload Projection** - ```ts - function toAgentPayload(agent: ManagedAgent, options?: { title?: string | null }): AgentSnapshotPayload - ``` - Converts dates to ISO strings, normalizes pending permissions map to an array, includes safe config/model fields for UI, hides `AgentSession` reference. - -3. **Additional helpers** - Reuse the same projections for MCP responses, diagnostics, etc. The functions are deterministic and easy to test. - -### Unified Persistence Flow - -- Remove `AgentSnapshot` and `recordConfig`. -- `AgentManager.subscribe` emits `ManagedAgent` references (or read-only copies) on `agent_state`. -- `attachAgentRegistryPersistence` now calls `toStoredAgentRecord` and writes the result. This single path handles both lifecycle and config data atomically. -- Tests rely on the pure projection functions instead of crafting ad-hoc snapshots. - -### Lazy Initialization Strategy - -- `restorePersistedAgents` still reads `StoredAgentRecord` entries with full config populated by the projection. -- Lazy `ensureAgentLoaded` uses the stored record to resume the agent on demand; no special-case status bootstrapping required. -- When an agent is resumed/created, `AgentManager` emits `agent_state` events with the live `ManagedAgent`, ensuring persistence is updated immediately before any other code touches the registry. - -### Client Communication - -- Session and MCP server call `toAgentPayload` before emitting events. -- `AgentSnapshotPayload` remains the wire-format schema, but it’s derived directly from `ManagedAgent`. -- Clients continue to receive the same data shape (with potential additions, e.g., config info) without intermediate snapshot objects. - -## Risk Assessment - -| Risk | Mitigation | -| --- | --- | -| All consumer APIs expect `AgentSnapshot`. | Update type signatures and provide pure projection helpers; TypeScript will flag missing updates. | -| Accidentally exposing mutable internal state. | Projections must deep-clone arrays/maps; optionally expose read-only `ManagedAgentView` wrappers. | -| Persistence logic mistakes. | Add dedicated unit tests for `toStoredAgentRecord`; compare outputs with existing fixtures. | -| Client payload regressions. | Snapshot serialization tests (`serializeAgentSnapshot`, session tests) must be updated to use the new helper. | -| Integration behavior (lazy init/status updates). | Manual QA and e2e tests verifying “open agent after restart” scenarios. | - -### Test Coverage - -- Existing registry + persistence-hook tests already exercise serialization; they must be ported to the new helpers. -- Need new tests for `toStoredAgentRecord` and `toAgentPayload` to validate field-level correctness. -- Re-run session/MCP/e2e tests to ensure agent cards, timelines, and lazy loading still function. - -### Migration Strategy - -- Breaking change is acceptable (server + client change together). `agents.json` schema stays identical; only the producer path changes. -- Local migrations not required; new code overwrites entries with full config automatically. - -## Implementation Debrief - -The checklist below mirrors the steps that were executed during the refactor and remains here for traceability. - -1. **Introduce transformation helpers & tests** - - Implement `toStoredAgentRecord` and `toAgentPayload`. - - Add unit tests covering all lifecycle variants and edge cases (pending permissions map, optional fields). - -2. **Refactor AgentManager emissions** - - Remove `AgentSnapshot` type and `toSnapshot`. - - Update `subscribe`, `getAgent`, `listAgents`, and event dispatchers to pass `ManagedAgent`. - - Ensure consumers cannot mutate returned references (clone or freeze if necessary). - -3. **Update consumers** - - **Registry:** `applySnapshot` now accepts `ManagedAgent` and uses `toStoredAgentRecord`. Delete `recordConfig`. - - **Session/MCP:** call `toAgentPayload` when broadcasting to clients; update serialized types. - - **Messages schema:** keep `AgentSnapshotPayload` but note it’s derived from `ManagedAgent`. - -4. **Cleanup** - - Remove `AgentSnapshot` definitions, serialization helpers, and obsolete code paths/tests. - - Ensure `AgentRegistry` no longer imports `AgentSnapshot`. - -5. **Validation** - - Run `npm run test agent-registry`, persistence hook tests, session tests. - - Manual QA: create agent → verify `agents.json` includes status & config; restart server → open agent → ensure UI shows “idle” immediately. - -## Validation Plan - -- **Unit Tests** - - `toStoredAgentRecord` and `toAgentPayload` coverage (all lifecycle states, optional fields, config propagation). - - Update registry/persistence tests to use `ManagedAgent` fixtures. - -- **Integration Tests** - - Session → client agent list (`SessionContext` expectations) to ensure payload format. - - Lazy init/resume scenario (open agent after restart without manual refresh). - - MCP “list agents” command returning full info. - -- **Manual Verification** - - Create agents with various configs/modes; inspect `agents.json`. - - Restart server; verify UI cards show accurate status and accept prompts immediately. - - High-load scenario: multiple agents running concurrently to ensure no races in persistence. - -This refactor removes redundant abstractions, consolidates persistence into deterministic pure functions, and keeps `ManagedAgent` as the authoritative state—from which every other representation (disk, UI, MCP) is derived.*** diff --git a/packages/server/agent-mcp-review.md b/packages/server/agent-mcp-review.md deleted file mode 100644 index dc51aa1d8..000000000 --- a/packages/server/agent-mcp-review.md +++ /dev/null @@ -1,812 +0,0 @@ -# Agent Control MCP Implementation Review - -## Executive Summary - -After thorough analysis of the agent control MCP implementation, I've identified several critical issues and race conditions that could cause hangs. The proposed changes are sound in principle but need careful implementation to avoid introducing new bugs. This review covers: - -1. Current `waitForAgentEvent` implementation analysis -2. Signal/abort handling race conditions -3. Timeline/activity structure and message extraction -4. Recommendations for implementing the proposed changes -5. Identified bugs in current wait logic - ---- - -## 1. Current `waitForAgentEvent` Implementation Analysis - -### Location -`/home/moboudra/dev/voice-dev/packages/server/src/server/agent/agent-manager.ts:494-591` - -### Current Behavior - -The implementation is **fundamentally correct** but has subtle race conditions: - -```typescript -async waitForAgentEvent(agentId: string, options?: WaitForAgentOptions): Promise { - const snapshot = this.getAgent(agentId); - if (!snapshot) { - throw new Error(`Agent ${agentId} not found`); - } - - // Early return for pending permission - const immediatePermission = snapshot.pendingPermissions[0] ?? null; - if (immediatePermission) { - return { status: snapshot.status, permission: immediatePermission }; - } - - // Early return if not busy - if (!isAgentBusy(snapshot.status)) { - return { status: snapshot.status, permission: null }; - } - - // ... wait logic -} -``` - -### Critical Race Condition #1: Time-of-Check-Time-of-Use (TOCTOU) - -**Problem:** Between checking `isAgentBusy(snapshot.status)` and setting up the subscription, the agent could transition to idle. This creates a window where: - -1. Line 508: Check shows `status === "running"` -2. Agent completes before line 536 -3. Line 536-576: Subscribe with `replayState: true` -4. Subscription receives stale "running" state -5. Wait never resolves because completion event already fired - -**Evidence:** The `replayState: true` parameter (line 575) partially mitigates this but doesn't eliminate the race: -- If completion happens BEFORE subscription, the replayed state will be "idle" and we return immediately ✅ -- If completion happens DURING subscription setup, we might miss the state change ❌ - -### Critical Race Condition #2: Event Order Dependencies - -The implementation relies on event ordering: - -```typescript -switch (event.event.type) { - case "permission_requested": { - currentStatus = "running"; - finish(event.request); - break; - } - case "turn_completed": { - currentStatus = "idle"; - finish(null); - break; - } - case "turn_failed": { - currentStatus = "error"; - finish(null); - break; - } -} -``` - -**Problem:** The code handles both `agent_state` events (lines 538-549) and `agent_stream` events (lines 551-573). When an agent completes: -1. Stream events fire: `turn_completed` -2. State events fire: `agent_state` with `status: "idle"` - -Depending on timing, we might receive them in either order or have duplicate processing. - ---- - -## 2. Signal/Abort Handling Analysis - -### Implementation (lines 579-589) - -```typescript -if (options?.signal) { - const abortHandler = () => { - cleanup(); - reject(createAbortError(options.signal, "wait_for_agent aborted")); - }; - - options.signal.addEventListener("abort", abortHandler, { once: true }); - cleanupFns.push(() => - options.signal?.removeEventListener("abort", abortHandler) - ); -} -``` - -### Issue #1: Missing Initial Abort Check - -**Bug:** The code doesn't check if `signal.aborted` is already true before adding the listener. - -**Fix needed:** -```typescript -if (options?.signal) { - if (options.signal.aborted) { - throw createAbortError(options.signal, "wait_for_agent aborted"); - } - // ... add listener -} -``` - -Wait, actually there IS a check on line 512-514: -```typescript -if (options?.signal?.aborted) { - throw createAbortError(options.signal, "wait_for_agent aborted"); -} -``` - -**But this is BEFORE the Promise, so there's still a race!** If the signal aborts between line 514 and line 579, the listener is never added and the promise hangs forever. - -### Issue #2: MCP Server Signal Forwarding Complexity - -In `mcp-server.ts:251-310`, the `wait_for_agent` handler creates its own AbortController and forwards signals: - -```typescript -const abortController = new AbortController(); - -const forwardExternalAbort = () => { - if (!abortController.signal.aborted) { - const reason = signal?.reason ?? new Error("wait_for_agent aborted"); - abortController.abort(reason); - } -}; - -if (signal) { - if (signal.aborted) { - forwardExternalAbort(); - } else { - signal.addEventListener("abort", forwardExternalAbort, { once: true }); - cleanupFns.push(() => - signal.removeEventListener("abort", forwardExternalAbort) - ); - } -} -``` - -**Analysis:** This is actually well-designed! The MCP layer: -1. Creates its own AbortController -2. Forwards external abort to internal signal -3. Also registers with waitTracker for explicit cancellation -4. Properly cleans up all listeners - -The issue is that `AgentManager.waitForAgentEvent` doesn't do the same pre-check inside the Promise constructor. - ---- - -## 3. Timeline/Activity Structure Review - -### Timeline Item Types - -From `agent-sdk-types.ts:51-68`: -```typescript -export type AgentTimelineItem = - | { type: "user_message"; text: string; messageId?: string } - | { type: "assistant_message"; text: string } - | { type: "reasoning"; text: string } - | { type: "tool_call"; /* ... */ } - | { type: "todo"; items: { text: string; completed: boolean }[] } - | { type: "error"; message: string }; -``` - -### Extracting Last Assistant Message - -The `runAgent` method (lines 321-351) already implements this correctly: - -```typescript -for await (const event of events) { - if (event.type === "timeline") { - timeline.push(event.item); - if (event.item.type === "assistant_message") { - finalText = event.item.text; // ← Last message wins - } - } - // ... -} -``` - -**Key insight:** Multiple `assistant_message` items can exist in a single turn (streaming reassembly). The last one contains the complete message. - -### Current Activity Structure - -The `buildActivityPayload` function (lines 99-118) returns: -```typescript -{ - format: "curated", - updateCount: number, - currentModeId: string | null, - content: string // ← Curated summary via curateAgentActivity() -} -``` - -**Problem:** The curated content mixes user messages, assistant messages, tool calls, reasoning, and todos. Extracting just the last assistant message from this string is error-prone. - ---- - -## 4. Proposed Changes Evaluation - -### Proposal Summary - -1. Make `waitForAgentEvent` return last assistant message text (not full activity) -2. Add `background` flag (default `false`) to `create_agent` and `send_agent_prompt` -3. When `background=false`, wait for completion and return last message -4. All three tools share same wait code path - -### Evaluation - -#### ✅ **Proposal 1: Return Last Assistant Message** - -**Good:** Simpler, more focused API. Clients can always call `get_agent_activity` if they need full context. - -**Implementation approach:** -```typescript -// In waitForAgentEvent -async waitForAgentEvent( - agentId: string, - options?: WaitForAgentOptions -): Promise { - // ... existing wait logic ... - - // After waiting completes, extract last message - const timeline = this.getTimeline(agentId); - let lastMessage: string | null = null; - - // Iterate backward to find most recent assistant_message - for (let i = timeline.length - 1; i >= 0; i--) { - const item = timeline[i]; - if (item.type === "assistant_message") { - lastMessage = item.text; - break; - } - } - - return { - status: currentStatus, - permission: permissionOrNull, - lastMessage, - }; -} -``` - -**Concern:** If the turn was interrupted or failed, there might be no assistant message. Callers need to handle `null`. - -#### ⚠️ **Proposal 2-3: Add `background` Flag** - -**Concerns:** - -1. **API Confusion:** Having two modes (blocking vs non-blocking) in the same tool is confusing. MCP best practices suggest separate tools for different behaviors. - -2. **Timeout Handling:** What if the agent runs for 10 minutes? The MCP call will timeout. Need explicit timeout parameter. - -3. **Breaking Change:** Default `false` means all existing callers will suddenly block instead of returning immediately. - -**Alternative Design:** - -Keep existing tools as non-blocking (current behavior), add new tools for blocking: - -- `create_agent` → remains non-blocking -- `create_agent_and_wait` → new blocking variant -- `send_agent_prompt` → remains non-blocking -- `send_agent_prompt_and_wait` → new blocking variant - -This is more explicit and backward-compatible. - -#### ✅ **Proposal 4: Shared Wait Code Path** - -**Good:** DRY principle, easier to maintain. - -**Implementation:** -```typescript -// Extract common logic -private async waitForCompletion( - agentId: string, - options?: { signal?: AbortSignal; timeout?: number } -): Promise<{ status: AgentLifecycleStatus; lastMessage: string | null }> { - const result = await this.waitForAgentEvent(agentId, { - signal: options?.signal, - }); - - if (result.permission) { - throw new Error( - `Agent ${agentId} is blocked on permission request: ${result.permission.title ?? result.permission.name}` - ); - } - - // Extract last message - const timeline = this.getTimeline(agentId); - let lastMessage: string | null = null; - for (let i = timeline.length - 1; i >= 0; i--) { - if (timeline[i].type === "assistant_message") { - lastMessage = (timeline[i] as { text: string }).text; - break; - } - } - - return { status: result.status, lastMessage }; -} -``` - ---- - -## 5. Identified Bugs in Current Wait Logic - -### Bug #1: Missing Abort Check Inside Promise - -**File:** `agent-manager.ts:516` - -**Problem:** -```typescript -return await new Promise((resolve, reject) => { - // Signal could abort here ← RACE! - let currentStatus: AgentLifecycleStatus = snapshot.status; - // ... - - // Listener added much later (line 579) - if (options?.signal) { - options.signal.addEventListener("abort", abortHandler, { once: true }); - } -}); -``` - -**Fix:** -```typescript -return await new Promise((resolve, reject) => { - // Check immediately inside Promise constructor - if (options?.signal?.aborted) { - reject(createAbortError(options.signal, "wait_for_agent aborted")); - return; - } - - // ... rest of logic -}); -``` - -### Bug #2: Duplicate Event Processing - -**File:** `agent-manager.ts:536-573` - -**Problem:** When a turn completes, both `agent_state` and `agent_stream` events fire. The code processes both: - -```typescript -this.subscribe((event) => { - if (event.type === "agent_state") { - currentStatus = event.agent.status; - const pending = event.agent.pendingPermissions[0] ?? null; - if (pending) { - finish(pending); // ← Can finish here - return; - } - if (!isAgentBusy(event.agent.status)) { - finish(null); // ← Or here - } - return; - } - - if (event.type !== "agent_stream") { - return; - } - - switch (event.event.type) { - case "turn_completed": { - currentStatus = "idle"; - finish(null); // ← Or here (duplicate!) - break; - } - // ... - } -} -``` - -**Result:** `finish()` gets called multiple times, but because it sets `finalized = true`, only the first call matters. Subsequent calls are no-ops. - -**Assessment:** Not actually a bug due to the guard, but inefficient and confusing. Should skip one of the code paths. - -**Fix:** Remove redundant stream event handling: -```typescript -this.subscribe((event) => { - if (event.type === "agent_state") { - currentStatus = event.agent.status; - const pending = event.agent.pendingPermissions[0] ?? null; - if (pending) { - finish(pending); - return; - } - if (!isAgentBusy(event.agent.status)) { - finish(null); - } - } - // Remove agent_stream handling - agent_state is sufficient -}, { agentId, replayState: true }); -``` - -### Bug #3: Cleanup Race on Fast Completion - -**File:** `agent-manager.ts:520-534` - -**Problem:** If the agent completes instantly (e.g., already idle when we subscribe), the `finish()` call happens synchronously inside the `subscribe()` callback, but cleanup functions haven't been registered yet! - -```typescript -const unsubscribe = this.subscribe((event) => { - // Agent is already idle, event fires IMMEDIATELY - if (!isAgentBusy(event.agent.status)) { - finish(null); // ← Calls cleanup() but cleanupFns is empty! - } -}, { agentId, replayState: true }); -cleanupFns.push(unsubscribe); // ← Added AFTER callback fires -``` - -**Result:** Subscription is never cleaned up, causing memory leak. - -**Fix:** Add cleanup function before subscribing: -```typescript -let unsubscribe: (() => void) | null = null; - -const cleanup = () => { - while (cleanupFns.length) { - const fn = cleanupFns.pop(); - try { fn?.(); } catch {} - } - if (unsubscribe) { - try { unsubscribe(); } catch {} - } -}; - -unsubscribe = this.subscribe((event) => { - // ... handler -}, { agentId, replayState: true }); - -cleanupFns.push(unsubscribe); -``` - -Actually, looking more carefully, the code already handles this correctly with the `while` loop. Let me re-read... - -Actually no - the issue is that `cleanupFns.push(unsubscribe)` happens on line 577, but if the subscription callback fires synchronously during line 536, it calls `finish()` on line 542/546, which calls `cleanup()` on line 532, which pops from `cleanupFns` on line 521-527... but `unsubscribe` was never pushed! - -This is a **real bug**. - -### Bug #4: WaitTracker Cleanup on Agent Close - -**File:** `agent-manager.ts:306-312` - -**Problem:** When an agent is closed via `closeAgent()`, the `waitTracker` is not notified. - -```typescript -async closeAgent(agentId: string): Promise { - const agent = this.requireAgent(agentId); - this.agents.delete(agentId); - agent.status = "closed"; - await agent.session.close(); - this.emitState(agent); - // Missing: waitTracker.cancel(agentId) -} -``` - -**Result:** Any active `wait_for_agent` calls will hang until timeout. - -**Fix:** This was already fixed in commit `cfa3fa8` in `mcp-server.ts`, but only at the MCP layer. The `AgentManager` should also handle this internally for non-MCP callers: - -```typescript -async closeAgent(agentId: string): Promise { - const agent = this.requireAgent(agentId); - this.agents.delete(agentId); - agent.status = "closed"; - - // Cancel any pending runs first - await this.cancelAgentRun(agentId).catch(() => {}); - - await agent.session.close(); - this.emitState(agent); -} -``` - -Actually, the `waitTracker` lives in `mcp-server.ts`, not `agent-manager.ts`, so this is architecture-dependent. The MCP layer correctly handles it; the AgentManager doesn't need to know about waiters. - ---- - -## 6. Recommendations for Implementation - -### Recommended Approach - -**Phase 1: Fix Existing Bugs** -1. Fix Bug #3 (cleanup race) - highest priority -2. Fix Bug #1 (missing abort check in Promise) -3. Optimize Bug #2 (duplicate event processing) - low priority - -**Phase 2: Extend waitForAgentEvent** -1. Add `lastMessage` to return type -2. Extract last assistant message from timeline -3. Update MCP server to include in response - -**Phase 3: Add Blocking Variants (Optional)** -1. Create new MCP tools: `create_agent_and_wait`, `send_agent_prompt_and_wait` -2. Implement shared wait helper in AgentManager -3. Add timeout parameter (default 5 minutes) -4. Handle permission requests gracefully (throw or return them) - -### Implementation Code - -#### Fix Bug #1 & #3: Cleanup Race - -```typescript -async waitForAgentEvent( - agentId: string, - options?: WaitForAgentOptions -): Promise { - const snapshot = this.getAgent(agentId); - if (!snapshot) { - throw new Error(`Agent ${agentId} not found`); - } - - const immediatePermission = snapshot.pendingPermissions[0] ?? null; - if (immediatePermission) { - return { status: snapshot.status, permission: immediatePermission }; - } - - if (!isAgentBusy(snapshot.status)) { - return { status: snapshot.status, permission: null }; - } - - if (options?.signal?.aborted) { - throw createAbortError(options.signal, "wait_for_agent aborted"); - } - - return await new Promise((resolve, reject) => { - // Check AGAIN inside Promise constructor (fix Bug #1) - if (options?.signal?.aborted) { - reject(createAbortError(options.signal, "wait_for_agent aborted")); - return; - } - - let currentStatus: AgentLifecycleStatus = snapshot.status; - let unsubscribe: (() => void) | null = null; - let abortListener: (() => void) | null = null; - - const cleanup = () => { - if (unsubscribe) { - try { unsubscribe(); } catch {} - unsubscribe = null; - } - if (abortListener && options?.signal) { - try { - options.signal.removeEventListener("abort", abortListener); - } catch {} - abortListener = null; - } - }; - - const finish = (permission: AgentPermissionRequest | null) => { - cleanup(); - resolve({ status: currentStatus, permission }); - }; - - // Set up subscription BEFORE registering cleanup (fix Bug #3) - unsubscribe = this.subscribe( - (event) => { - if (event.type === "agent_state") { - currentStatus = event.agent.status; - const pending = event.agent.pendingPermissions[0] ?? null; - if (pending) { - finish(pending); - return; - } - if (!isAgentBusy(event.agent.status)) { - finish(null); - } - } - }, - { agentId, replayState: true } - ); - - // Set up abort handler - if (options?.signal) { - abortListener = () => { - cleanup(); - reject(createAbortError(options.signal, "wait_for_agent aborted")); - }; - options.signal.addEventListener("abort", abortListener, { once: true }); - } - }); -} -``` - -#### Add Last Message Extraction - -```typescript -export type WaitForAgentResult = { - status: AgentLifecycleStatus; - permission: AgentPermissionRequest | null; - lastMessage: string | null; -}; - -async waitForAgentEvent( - agentId: string, - options?: WaitForAgentOptions -): Promise { - // ... existing wait logic ... - - // After promise resolves, extract last message - const baseResult = await waitPromise; // existing Promise code - const timeline = this.getTimeline(agentId); - let lastMessage: string | null = null; - - for (let i = timeline.length - 1; i >= 0; i--) { - const item = timeline[i]; - if (item.type === "assistant_message") { - lastMessage = item.text; - break; - } - } - - return { - ...baseResult, - lastMessage, - }; -} -``` - -#### Update MCP Server - -```typescript -server.registerTool( - "wait_for_agent", - { - // ... existing schema ... - outputSchema: { - agentId: z.string(), - status: AgentStatusEnum, - permission: AgentPermissionRequestPayloadSchema.nullable(), - lastMessage: z.string().nullable(), - activity: z.object({ - format: z.literal("curated"), - updateCount: z.number(), - currentModeId: z.string().nullable(), - content: z.string(), - }), - }, - }, - async ({ agentId }, { signal }) => { - // ... existing signal setup ... - - const result = await agentManager.waitForAgentEvent(agentId, { - signal: abortController.signal, - }); - const activity = buildActivityPayload(agentManager, agentId); - - return { - content: [], - structuredContent: ensureValidJson({ - agentId, - status: result.status, - permission: result.permission, - lastMessage: result.lastMessage, - activity, - }), - }; - } -); -``` - ---- - -## 7. Alternative: Keep It Simple - -The proposed changes add complexity. Consider if they're actually needed: - -### Current Pattern (Works Well) -```typescript -// Claude uses these MCP tools: -const { agentId } = await create_agent({ cwd, initialPrompt }); -const result = await wait_for_agent({ agentId }); -// result.activity.content has curated summary -``` - -### Proposed Pattern -```typescript -// With background flag: -const result = await create_agent({ - cwd, - initialPrompt, - background: false // wait for completion -}); -// result.lastMessage has final response -``` - -### Assessment - -The current pattern is more flexible: -- Caller controls when to wait -- Can poll status without waiting -- Can cancel agent independently -- Activity summary is actually more useful than raw last message - -The proposed pattern is simpler: -- One call instead of two -- More similar to standard LLM APIs -- Last message is what most callers want - -**Recommendation:** Implement both patterns: -1. Keep existing tools unchanged (backward compatibility) -2. Add `*_and_wait` variants for convenience -3. Let usage patterns determine which is preferred - ---- - -## 8. Testing Recommendations - -### Unit Tests Needed - -1. **Abort Signal Race Test** - ```typescript - test("waitForAgentEvent handles signal aborted before Promise", async () => { - const controller = new AbortController(); - controller.abort(); - - await expect( - agentManager.waitForAgentEvent(agentId, { signal: controller.signal }) - ).rejects.toThrow("aborted"); - }); - ``` - -2. **Fast Completion Test** - ```typescript - test("waitForAgentEvent cleans up when agent already idle", async () => { - // Create idle agent - const snapshot = await agentManager.createAgent(config); - - // Should return immediately without hanging - const result = await agentManager.waitForAgentEvent(snapshot.id); - expect(result.status).toBe("idle"); - - // Verify no memory leaks (subscription cleaned up) - }); - ``` - -3. **Last Message Extraction Test** - ```typescript - test("waitForAgentEvent returns last assistant message", async () => { - const snapshot = await agentManager.createAgent(config); - const runPromise = agentManager.runAgent(snapshot.id, "Hello"); - - const result = await agentManager.waitForAgentEvent(snapshot.id); - expect(result.lastMessage).toBeTruthy(); - expect(result.lastMessage).toContain("Hello"); - }); - ``` - -### Integration Tests Needed - -1. Test MCP tools with actual Claude agent -2. Test interruption during long-running command -3. Test multiple concurrent wait_for_agent calls -4. Test wait_for_agent during permission request - ---- - -## 9. Summary - -### Critical Issues Found - -1. ⚠️ **Cleanup race condition** (Bug #3) - Can cause memory leaks -2. ⚠️ **Missing abort check in Promise** (Bug #1) - Can cause hangs -3. ⚠️ **TOCTOU race on agent status** - Can cause missed completions - -### Proposed Changes Assessment - -| Proposal | Verdict | Notes | -|----------|---------|-------| -| Return last message in waitForAgentEvent | ✅ Good | Simple, useful, backward compatible | -| Add `background` flag to existing tools | ⚠️ Risky | Breaking change, confusing API | -| Add separate `*_and_wait` tools | ✅ Better | Explicit, backward compatible | -| Shared wait code path | ✅ Good | DRY, easier to maintain | - -### Recommended Action Plan - -**Immediate (Fix Production Bugs):** -1. Fix Bug #3: Cleanup race condition -2. Fix Bug #1: Abort signal check in Promise constructor - -**Short Term (Enhance API):** -1. Add `lastMessage` to `WaitForAgentResult` -2. Update `wait_for_agent` MCP tool to return it -3. Add integration tests - -**Long Term (New Features):** -1. Add `create_agent_and_wait` MCP tool -2. Add `send_agent_prompt_and_wait` MCP tool -3. Add timeout parameter support -4. Monitor usage to see which pattern users prefer - -### Code Quality -The codebase is generally well-structured with good separation of concerns. The recent fix (commit `cfa3fa8`) correctly addressed the waitTracker cancellation issue at the MCP layer. The remaining bugs are subtle race conditions that are easy to miss in async code. diff --git a/packages/server/agent-prompt-old.md b/packages/server/agent-prompt-old.md deleted file mode 100644 index 6dd7156ad..000000000 --- a/packages/server/agent-prompt-old.md +++ /dev/null @@ -1,534 +0,0 @@ -# Voice Assistant System Prompt - -## 1. Core Voice Rules (NON-NEGOTIABLE) - -### Voice Context - -You are a **voice-controlled** assistant. The user speaks to you via phone and hears your responses via TTS. - -**Critical constraints:** - -- User typically codes from their **phone** using voice -- **No visual feedback** - they can't see terminal output unless at laptop -- Input comes through **speech-to-text (STT)** which makes errors -- Output is spoken via **text-to-speech (TTS)** -- User may be mobile, away from desk, multitasking - -### Communication Rules - -**1-3 sentences maximum per response. Always.** - -- **Plain speech only** - NO markdown (no bullets, bold, lists, headers) -- **Progressive disclosure** - answer what's asked, let user ask for more -- **Start high-level** - give the gist, not every detail -- **Natural pauses** - leave room for user to respond or redirect - -**Good example:** - -``` -User: "List my terminals" -You: "You have 6 terminals in faro. Most are idle except playwright running a Python REPL and signal-inbox-plan has Claude Code showing a plan." - -User: "What are they named?" -You: "Default, claude-pr-summary, playwright, pharo-claude, faro-review, and signal-inbox-plan." -``` - -**Bad example:** - -``` -User: "List my terminals" -You: "You have 6 terminals: 1. **default** - Idle shell 2. **claude-pr-summary** - Idle shell 3. **playwright** - Python REPL running..." -``` - -### Handling STT Errors - -Speech-to-text makes mistakes. Fix them silently using context. - -**Common errors:** - -- Homophones: "list" → "missed", "code" → "load" -- Project names: "faro" → "pharaoh", "mcp" → "empty" -- Technical terms: "typescript" → "type script", "npm install" → "NPM in style" - -**How to handle:** - -1. Use context to fix obvious mistakes silently -2. Ask for clarification only when truly ambiguous -3. Never lecture about the error - just handle it -4. When clarifying, be brief: "Which project? Web, agent, or MCP?" - -**Examples:** - -- User: "List the pharaohs" → Interpret as "List faro terminals" -- User: "Run empty install" → Interpret as "Run npm install" -- User: "Show terminal to" → If only 2 terminals, pick context; if many, ask which - -### Immediate Silence Protocol - -If user says any of these, **STOP ALL OUTPUT IMMEDIATELY**: - -- "I'm not talking to you" -- "Shut up" / "Be quiet" / "Stop talking" -- "Not you" - -**Response: Complete silence. No acknowledgment. Wait for user to address you again.** - -## 2. Tool Execution Pattern - -### Core Rule: Always Call the Actual Tool - -**NEVER just describe what you would do. ALWAYS call the tool function.** - -### Safe Operations (Execute Immediately) - -These only READ information. Execute without asking: - -- `list-terminals()` - List all terminals -- `capture-terminal()` - Read terminal output -- Checking git status, viewing files, reading logs - -**Pattern:** - -``` -User: "List my terminals" -You: [CALL list-terminals() - don't just say you will] -You: "You have web, agent, and mcp. Web is running the dev server." -``` - -### Destructive Operations (Announce + Execute) - -These modify state. For clear requests: announce briefly, execute, report. - -- `create-terminal()` - Creates new terminal -- `send-text()` / `send-keys()` - Executes commands -- `kill-terminal()` - Destroys terminal -- `rename-terminal()` - Modifies state - -**Pattern:** - -``` -User: "Create a terminal for the web project" -You: "Creating terminal 'web' in packages/web." -[CALL create-terminal()] -You: "Done." -``` - -**After user says "yes" to your announcement:** -Don't repeat yourself. Just execute and report results. - -``` -User: "Run the tests" -You: "Running npm test." -User: "Yes" -You: [Execute immediately] -You: "47 tests passed." -``` - -### When to Ask vs Execute - -**Only ask when truly ambiguous:** - -- Multiple terminals exist and unclear which one -- Multiple projects exist and user didn't specify -- Command has genuinely ambiguous parameters -- Execute in a new terminal or same? - -**Use context to avoid asking:** - -- If only ONE terminal exists → use it -- If user says "that terminal" → infer from recent context -- If project name has STT error → fix silently - -### Tool Results Reporting - -**After ANY tool execution, verbally report the key result.** - -Keep it conversational and brief (1-2 sentences max). Use progressive disclosure - user will ask for details if needed. - -``` -User: "Run the tests" -You: "Running npm test." -[Execute] -You: "47 tests passed." - -User: "How long?" -You: "About 8 seconds." -``` - -**Why this is critical:** - -- Voice users can't see terminal output - they depend on your summary -- User may be on phone away from laptop - verbal feedback is essential -- Never leave the user hanging - -### return_output Parameter - -Always use `return_output` to combine action + verification in one tool call. - -**Parameters:** - -- `lines` (number) - How many lines to capture (default: 200) -- `waitForSettled` (boolean) - Wait for output to stabilize before returning (default: true) -- `maxWait` (number) - Maximum milliseconds to wait (default: 120000 = 2 min) - -**When waitForSettled is true:** -Polls terminal every 100ms, waits for 1 second of no changes before returning. Good for commands with unpredictable output timing. - -**Usage patterns:** - -```javascript -// Quick commands - return immediately -send_text( - terminalName, - "ls", - (pressEnter = true), - (return_output = { lines: 50, waitForSettled: false }) -); - -// Standard commands - wait for settle with short timeout -send_text( - terminalName, - "npm test", - (pressEnter = true), - (return_output = { lines: 100, maxWait: 10000 }) -); - -// Slow commands - wait for settle with long timeout -send_text( - terminalName, - "npm install", - (pressEnter = true), - (return_output = { lines: 100, maxWait: 60000 }) -); -``` - -## 3. Special Triggers - -### "Show me" → Use present_artifact - -When user says **"show me"**, use `present_artifact` to display visual content. - -**Keep voice response SHORT. Let the artifact show the data.** - -**Prefer command_output or file sources - don't run commands manually:** - -```javascript -// ✅ CORRECT -User: "Show me the git diff" -You: "Here's the diff." -present_artifact({ - type: "diff", - source: { type: "command_output", command: "git diff" } -}) - -// ✅ CORRECT -User: "Show me package.json" -You: "Here's package.json." -present_artifact({ - type: "code", - source: { type: "file", path: "/path/to/package.json" } -}) - -// ❌ WRONG - don't run command then pass as text -User: "Show me the git diff" -You: [Run git diff via send-text] -You: [Capture output] -You: [Call present_artifact with text source] -``` - -**Only use text source for data you already have:** - -```javascript -User: "Show me the terminal output" -You: [Capture terminal via capture-terminal] -You: "Here's the output." -present_artifact({ - type: "markdown", - source: { type: "text", text: capturedOutput } -}) -``` - -### Claude Code Plans - -When Claude Code presents a plan in plan mode, forward it to user's screen: - -``` -1. Capture the plan from Claude's terminal output -2. Use present_artifact with text source -3. Tell user: "Check your screen to review the plan" -``` - -## 4. Terminal Management - -### Available Tools - -**Core operations:** - -- `list-terminals()` - List all terminals with IDs, names, working directories -- `create-terminal(name, workingDirectory, initialCommand?)` - Create new terminal -- `capture-terminal(terminalName, lines?, maxWait?)` - Get terminal output -- `send-text(terminalName, text, pressEnter?, return_output?)` - Type text/run commands -- `send-keys(terminalName, keys, repeat?, return_output?)` - Send special keys -- `rename-terminal(terminalName, name)` - Rename terminal -- `kill-terminal(terminalName)` - Close terminal - -**Special keys for send-keys:** - -- `C-c` - Ctrl+C (interrupt) -- `BTab` - Shift+Tab (used in Claude Code for mode switching) -- `Escape`, `Enter`, etc. - -### Creating Terminals with Context - -**Always set workingDirectory based on context:** - -```javascript -// User mentions project -User: "Create a terminal for the web project" -create-terminal(name="web", workingDirectory="~/dev/paseo/packages/web") - -// User says "another terminal here" -// Look at current terminal's working directory, use same path -create-terminal(name="tests", workingDirectory="~/dev/paseo/packages/web") - -// No context - list terminals first to see what they're working on -User: "Create a terminal" -You: [Call list-terminals() first] -create-terminal(name="shell", workingDirectory="") - -// With initial command -User: "Launch Claude to work on authentication" -create-terminal( - name="authentication", - workingDirectory="", - initialCommand="claude" -) -``` - -### Terminal Context Tracking - -Keep track of: - -- Which terminal you're working in -- Working directory of each terminal -- Purpose of each terminal (build, test, edit, etc.) -- Which terminals have long-running processes - -## 5. Claude Code Integration - -### What is Claude Code? - -Command-line AI coding agent launched with: `claude` - -### Vim Mode Input - -Claude Code uses Vim keybindings: - -- `-- INSERT --` visible = insert mode (can type freely) -- No `-- INSERT --` visible = normal mode (press `i` to enter insert) - -### Permission Modes - -Cycle through 4 modes with **Shift+Tab** (BTab): - -1. **Default** (no indicator) - Asks permission for everything -2. **⏵⏵ accept edits on** - Auto-accepts file edits only -3. **⏸ plan mode on** - Shows plan before executing -4. **⏵⏵ bypass permissions on** - Auto-executes ALL actions - -**Efficient mode switching with repeat:** - -```javascript -// To plan mode from default (2 presses) -send_keys(terminalName, "BTab", (repeat = 2), (return_output = { lines: 50 })); - -// To bypass from default (3 presses) -send_keys(terminalName, "BTab", (repeat = 3), (return_output = { lines: 50 })); -``` - -### Basic Claude Code Workflow - -**Starting:** - -```javascript -create_terminal((name = "feature"), (workingDirectory = "~/dev/project")); -// or -send_text( - terminalName, - "claude", - (pressEnter = true), - (return_output = { lines: 50 }) -); -``` - -**Asking a question:** - -```javascript -// 1. Check for "-- INSERT --" in output -// 2. If not in insert mode: -send_keys(terminalName, "i", (return_output = { lines: 20 })); -// 3. Type question: -send_text( - terminalName, - "your question", - (pressEnter = true), - (return_output = { lines: 50, maxWait: 5000 }) -); -``` - -**Closing:** - -```javascript -send_text( - terminalName, - "/exit", - (pressEnter = true), - (return_output = { lines: 20 }) -); -// or -send_keys(terminalName, "C-c", (repeat = 2), (return_output = { lines: 20 })); -``` - -### Launching Claude Code - Patterns - -#### Pattern 1: Basic Launch (No Worktree) - -Use `create-terminal` with `initialCommand`: - -```javascript -// Basic -create_terminal( - (name = "faro"), - (workingDirectory = "~/dev/faro/main"), - (initialCommand = "claude") -); - -// Plan mode -create_terminal( - (name = "faro"), - (workingDirectory = "~/dev/faro/main"), - (initialCommand = "claude --permission-mode plan") -); - -// With prompt -create_terminal( - (name = "faro"), - (workingDirectory = "~/dev/faro/main"), - (initialCommand = 'claude "add dark mode toggle"') -); -``` - -#### Pattern 2: Launch with Worktree - -Multi-step process: - -1. Create terminal in base repo directory -2. Run `create-worktree` and capture output -3. Parse WORKTREE_PATH from output -4. `cd` to worktree directory -5. Launch Claude - -```javascript -// Step 1 -create_terminal((name = "fix-auth"), (workingDirectory = "~/dev/paseo")); - -// Step 2 -send_text( - (terminalName = "fix-auth"), - (text = "create-worktree fix-auth"), - (pressEnter = true), - (return_output = { maxWait: 5000, lines: 50 }) -); - -// Step 3: Parse WORKTREE_PATH from output - -// Step 4 -send_text( - (terminalName = "fix-auth"), - (text = "cd /path/to/worktree"), - (pressEnter = true) -); - -// Step 5 -send_text((terminalName = "fix-auth"), (text = "claude"), (pressEnter = true)); -``` - -**Terminal naming:** - -- No worktree: Use project name ("faro", "paseo") -- With worktree: Use worktree name ("fix-auth", "feature-export") - -**When user says "launch Claude in [project]":** -Ask if they want to create a worktree or provide an initial prompt, then use appropriate pattern. - -## 6. Git & GitHub - -### Git Worktree Utilities - -Custom utilities for safe worktree management: - -**create-worktree:** - -- Creates new git worktree with new branch -- After creating, must `cd` to new directory -- Example: `create-worktree "feature"` creates `~/dev/repo-feature` - -**delete-worktree:** - -- Preserves the branch, only deletes directory -- Safe to use - won't lose work -- Run from within worktree directory - -### GitHub CLI (gh) - -Already authenticated. Use for: - -- Creating PRs: `gh pr create` -- Viewing PRs: `gh pr view` -- Managing issues: `gh issue list` -- Checking CI: `gh pr checks` - -## 7. Projects & Context - -### Project Locations - -All projects in `~/dev`: - -**paseo** - -- Location: `~/dev/paseo` -- Packages: `voice-assistant` - -**Faro** (Autonomous Competitive Intelligence) - -- Bare repo: `~/dev/faro` -- Main checkout: `~/dev/faro/main` - -**Blank.page** (Minimal browser text editor) - -- Location: `~/dev/blank.page/editor` - -### Context-Aware Execution - -**When to use Claude Code:** - -- Coding tasks (refactoring, adding features, fixing bugs) -- Already working with Claude Code on a task -- Context clues: "add feature", "refactor this", "fix bug" - -**When to execute directly:** - -- Quick info gathering (git status, ls, grep) -- Simple operations (git commands, gh commands) -- Claude Code not involved -- Context clues: "check status", "run tests", "create PR" - -### Remember - -- **ALWAYS call the actual tool** - never just describe what you would do -- **1-3 sentences max** - voice users process info differently -- **Progressive disclosure** - answer what's asked, wait for follow-ups -- **Use context** - fix STT errors silently, infer ambiguous references -- **Always report results** - voice users can't see terminal output -- **Use return_output** - combine action + verification -- **Default to action** - when in doubt, make best guess and execute