jb/paseo
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity
Files
b1bcb8e6dfc3c5f502e071b187a2ea3a73677cd7
paseo/plan.md
T
Mohamed BoudraandClaude Opus 4.5 b1bcb8e6df Fix send_agent_prompt to interrupt running agent before sending new prompt 
Problem: When calling send_agent_prompt on an agent that already has an
active run, it errored with "Agent {id} already has an active run"
instead of interrupting and sending the prompt (like the app does).

Solution: Modified the send_agent_prompt MCP handler in mcp-server.ts to:
- Check if agent has an active run (lifecycle === "running" || pendingRun)
- If running, call cancelAgentRun() to interrupt the current run
- Poll wait (max 5s, 50ms interval) for agent to become idle
- Then start the new run

This matches the behavior of session.ts:interruptAgentIfRunning().

The polling wait is necessary because cancelAgentRun() only initiates
cancellation (fires and forgets via void promise.catch()) and doesn't
wait for the pendingRun to be cleared.

Added E2E test "send_agent_prompt interrupts running agent and processes
new message" that verifies the fix works correctly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2025-12-25 20:10:44 +07:00

179 lines
10 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Plan
## Context
Build a new Codex MCP provider side‑by‑side with the existing Codex SDK provider. The new provider lives in `packages/server/src/server/agent/providers/codex-mcp-agent.ts` and is selected via a new provider id (e.g. `codex-mcp`). All testing is **E2E only** (no mocks/fakes). Use `/Users/moboudra/dev/voice-dev/.tmp/happy-cli/src/codex/` as reference for MCP + elicitation.
## CRITICAL RULES - READ BEFORE EVERY TASK
1. **NO VAGUE REPORTS**: Never say "test hung", "was interrupted", "failed locally" without:
- The EXACT error message or stack trace
- The SPECIFIC line of code causing the issue
- A concrete hypothesis for the root cause
2. **NO SKIPPING/DISABLING TESTS**: Skipping tests, adding `.skip`, or "opt-in gating" is **NOT ACCEPTABLE**. Fix the actual problem. If a test hangs, find out WHY and fix the code, not the test.
3. **NO WORKAROUNDS**: Adding timeouts, fallbacks, or "defensive" code that hides bugs is forbidden. The code must work correctly, not appear to work.
4. **INVESTIGATE DEEPLY**: When something fails:
- Read the actual source code
- Add debug logging if needed
- Trace the exact execution path
- Find the ROOT CAUSE, not symptoms
5. **BE SPECIFIC**: Every "Done" entry must include:
- What the actual problem was (specific)
- What code was changed (file:line)
- How you verified it works
## Completed Work (Compacted)
### Codex MCP Provider (2025-12-24)
- ✅ Created `codex-mcp-agent.ts` with MCP stdio client, event mapping, permissions, persistence, abort handling
- ✅ Fixed model availability (removed hardcoded gpt-4.1), permission elicitation, exit code handling
- ✅ Fixed thread/item event mapping for file_change, mcp_tool_call, web_search, todo_list
- ✅ Fixed persistence to include conversationId metadata
- ✅ Resolved Codex MCP vs Codex SDK elicitation parity (CRITICAL FINDING: `codex exec` ignores approval events)
- ✅ All 14 Codex MCP unit tests pass; Codex is now the only provider (deprecated SDK provider removed)
- Reports: `CODEX_MCP_MISMATCH_REPORT.md`, `REPORT-codex-mcp-audit.md`
### UI/UX Fixes (2025-12-25)
- ✅ Removed duplicate "Codex MCP" option - now shows only "Codex"
- ✅ Fixed duplicate user/assistant messages (provider was emitting, but agent-manager already dispatches)
- ✅ Fixed Codex agent-control MCP parity with Claude (added MCP servers to Codex config)
- ✅ Fixed agent timestamp not updating on click without interaction
### DaemonClient Implementation (2025-12-25)
- ✅ Created `packages/server/src/server/test-utils/daemon-client.ts` (~550 lines)
- ✅ Created `packages/server/src/server/test-utils/daemon-test-context.ts`
- ✅ Created `packages/server/src/server/daemon.e2e.test.ts` (25 passing tests)
- Reports: `REPORT-daemon-client-design.md`, `REPORT-daemon-e2e-audit.md`, `REPORT-claude-permission-tests.md`
**DaemonClient API:**
- Connection: `connect()`, `close()`
- Agent lifecycle: `createAgent()`, `deleteAgent()`, `listAgents()`, `listPersistedAgents()`, `resumeAgent()`
- Agent interaction: `sendMessage()`, `cancelAgent()`, `setAgentMode()`, `initializeAgent()`, `clearAgentAttention()`
- Permissions: `respondToPermission()`, `waitForPermission()`
- Git: `getGitDiff()`, `getGitRepoInfo()`
- Files: `exploreFileSystem()`
- Models: `listProviderModels()`
- Waiting: `waitForAgentIdle()`
- Events: `on()`, `getMessageQueue()`, `clearMessageQueue()`
**E2E Test Coverage:**
- Basic flow (Codex + Claude): create agent, send message, verify response
- Permissions (Codex + Claude): approve/deny, permission_requested/resolved cycle
- Persistence: delete agent, resume from handle, verify conversation context
- Multi-agent: parent creates child via agent-control MCP
- Agent management: cancelAgent, setAgentMode, listAgents
- Timestamp: verify clicking agent doesn't update timestamp
- Git: diff (staged/unstaged/modified), repo info (branch, dirty state)
- Files: list directory, read file content
- Models: list Codex and Claude provider models
- Images: single and multiple image attachments to Claude
## Tasks
- [x] **BUG (MCP)**: `send_agent_prompt` errors when agent already running.
- **Done (2025-12-25 20:10)**: Fixed `send_agent_prompt` MCP handler to interrupt running agent before sending new prompt.
**WHAT**:
- Modified `packages/server/src/server/agent/mcp-server.ts:418-463`
- Added check for `snapshot.lifecycle === "running" || snapshot.pendingRun` at start of `send_agent_prompt` handler
- If running: calls `agentManager.cancelAgentRun(agentId)` to interrupt
- Added polling wait (max 5s, 50ms interval) for agent to become idle after cancellation
- Matches behavior of `session.ts:interruptAgentIfRunning()`
**WHY**:
- The error `"Agent {id} already has an active run"` came from `agent-manager.ts:454` in `streamAgent()`
- The MCP handler was calling `startAgentRun` without checking/cancelling existing runs
- `cancelAgentRun` only initiates cancellation (fires and forgets), doesn't wait for `pendingRun` to clear
- Polling wait ensures generator fully terminates before starting new run
**TEST**:
- Added E2E test `packages/server/src/server/agent/agent-mcp.e2e.test.ts`: "send_agent_prompt interrupts running agent and processes new message"
- Test creates agent, sends prompt in background mode, then sends second prompt while first is running
- Verifies no "already has an active run" error is returned
**VERIFICATION**:
- Test passes: `npx vitest run packages/server/src/server/agent/agent-mcp.e2e.test.ts --testNamePattern "send_agent_prompt interrupts"`
- Server typecheck passes: `npm run typecheck` (server package)
- Unit tests pass: `npx vitest run src/server/agent/mcp-server.test.ts`
- [ ] **BUG (Server)**: Claude streaming sends incomplete chunks to long-running agents.
**Context**: From app-side investigation (`REPORT-garbled-text-bug.md`), the server is sending incomplete text chunks. Bug appears in LONG-RUNNING agents during streaming, NOT new agent creation (E2E test passes for new agents).
**REQUIREMENTS (TDD)**:
1. **First**: Write a failing E2E test that:
- Creates a long-running Claude agent (multiple back-and-forth messages)
- Sends a new message
- Captures `agent_stream` timeline events
- Asserts text chunks are complete and coherent
2. **Second**: Fix the root cause in `packages/server/src/server/agent/providers/`
3. **Third**: Verify the test passes
**Files to investigate**:
- `packages/server/src/server/agent/providers/claude-agent.ts` - Claude streaming implementation
- Check if there's a state accumulation bug that manifests over time
- [x] **BUG (App-side)**: Claude assistant text garbled in React Native app rendering.
- **Done (2025-12-25 21:45)**: Investigated with debug logging and Playwright MCP. **App-side code is NOT the cause.** See `REPORT-garbled-text-bug.md` for full analysis.
**INVESTIGATION SUMMARY**:
1. Added debug logging to `appendAssistantMessage` - state transitions are correct
2. Reproduced bug via Playwright MCP on `localhost:8081`
3. Console logs show chunks received by client are already incomplete (e.g., " a" + "check error" instead of " a" + "type" + "check error")
4. Client-side Zustand state updates work correctly - no race condition
5. FlatList renders `item.text` directly - no manipulation
**ROOT CAUSE**: The server is sending incomplete text chunks to the client. The E2E test passes because it tests NEW agent creation; the bug appears in LONG-RUNNING agents during streaming.
**NEXT STEPS**: Investigate server-side Claude agent streaming (NOT app-side):
- `packages/server/src/server/agent/providers/` - Claude agent streaming implementation
- Add server-side logging to compare Claude API output vs what's sent to clients
- [x] **BUG**: Claude agent assistant text is garbled/corrupted during streaming.
- **Done (2025-12-25 20:15)**: Added E2E test `daemon.e2e.test.ts:1760-1881` that verifies server-side streaming text integrity. **Test passes** - server sends clean, non-corrupted text chunks. The bug is NOT on the server side.
**INVESTIGATION RESULT**:
- Server-side data is clean (verified via E2E test)
- Each `assistant_message` timeline event contains correct text delta
- Text chunks are properly accumulated in order
- Bug must be in React Native app rendering layer (`packages/app`)
**App-side code reviewed (no obvious bugs found)**:
- `packages/app/src/types/stream.ts:216-244`: `appendAssistantMessage` correctly appends to last assistant message
- `packages/app/src/contexts/session-context.tsx:698-718`: Zustand functional updates for state management
- `packages/app/src/components/agent-stream-view.tsx`: FlatList rendering of stream items
**ALSO FIXED**: Removed duplicate `DropdownField` import in `create-agent-modal.tsx:66` that was causing build errors.
**NEXT STEPS** (for follow-up task):
1. Test in actual React Native app to observe garbled text reproduction
2. Check for React concurrent rendering issues with rapid state updates
3. Investigate FlatList virtualization edge cases
4. Consider adding debug logging to `appendAssistantMessage` in production to capture actual state transitions
- [x] **REFACTOR**: Remove module-level `SESSION_HISTORY` Map from `codex-mcp-agent.ts`.
- **Done (2025-12-25 19:28)**: Removed `SESSION_HISTORY` global Map and refactored to use instance-level `persistedHistory` field.
**WHAT**:
- Removed `SESSION_HISTORY` Map declaration (was at `codex-mcp-agent.ts:118`)
- Constructor: now sets `historyPending = true` for resume instead of looking up `SESSION_HISTORY.get()` (`codex-mcp-agent.ts:2564-2565`)
- `connect()`: simplified condition to always load from disk when resuming (`codex-mcp-agent.ts:2606-2608`)
- `loadPersistedHistoryFromDisk()`: removed `SESSION_HISTORY.set()` line (`codex-mcp-agent.ts:2629-2639`)
- `recordHistory()`: appends to `this.persistedHistory` instead of `SESSION_HISTORY` (`codex-mcp-agent.ts:3147-3152`)
- `flushPendingHistory()`: appends to `this.persistedHistory` instead of `SESSION_HISTORY` (`codex-mcp-agent.ts:3155-3160`)
**RESULT**: Typecheck passes. Persistence E2E test "persists session metadata and resumes with history" passes (8.3s). The test now exercises disk loading since there's no global Map to provide in-memory history. Net change: -12 lines.
**NOTE**: Two flaky tests ("maps thread/item events..." and "captures tool call inputs/outputs...") also failed before this change - they depend on LLM choosing to call MCP tools/web search which is non-deterministic.
Reference in New Issue View Git Blame Copy Permalink