From 85be6df7b646d25215dbf9f8ea7dd73830d0ccb7 Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Tue, 24 Mar 2026 22:32:30 +0000 Subject: [PATCH 1/4] fix: E2E test seed session, cancel deadlock, and user_created_thread handling Three fixes for the Zed WebSocket sync E2E tests: 1. Seed session: Create a session in the store before Zed connects, matching HELIX_SESSION_ID (now ses_-prefixed). This mirrors production where sessions always exist before the agent connects, fixing handleUserCreatedThread "session not found" errors. 2. Cancel deadlock: In AcpThread::cancel(), drop the old send_task instead of awaiting it. The previous approach (cx.background_spawn(turn.send_task)) would deadlock if the ACP agent never responded to CancelNotification. Dropping the task triggers the existing tx-dropped handler in run_turn which emits Stopped(Cancelled). This fixes Phase 8 timeout with Claude Code. 3. Thread tracking: Only track thread_created events in threadIDs (not user_created_thread). Spontaneous threads from Zed startup shouldn't be used for test phase follow-ups. Adjusted expected interaction count from 6 to 5 to match production behavior where follow-ups update existing interactions. Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/acp_thread/src/acp_thread.rs | 13 ++++- .../e2e-test/CLAUDE.md | 45 +++++++++++++++ .../e2e-test/helix-ws-test-server/go.mod | 8 +-- .../e2e-test/helix-ws-test-server/go.sum | 16 +++--- .../e2e-test/helix-ws-test-server/main.go | 57 ++++++++++++++++--- .../e2e-test/run_e2e.sh | 21 ++++++- 6 files changed, 134 insertions(+), 26 deletions(-) create mode 100644 crates/external_websocket_sync/e2e-test/CLAUDE.md diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index ee7468360b797a..fbc11fb9e4b8c6 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -2315,8 +2315,17 @@ impl AcpThread { Self::flush_streaming_text(&mut self.streaming_text_buffer, cx); self.mark_pending_tools_as_canceled(); - // Wait for the send task to complete - cx.background_spawn(turn.send_task) + // Drop the send_task instead of awaiting it. This cancels the prompt + // future immediately, which drops the oneshot `tx`. The `rx.await` in + // run_turn then returns Err, hitting the existing "tx dropped" handler + // that emits Stopped(Cancelled) (see run_turn line ~2224). + // + // We still call connection.cancel() above as a courtesy notification + // to the agent, but we don't wait for the agent to acknowledge it. + // The previous approach (cx.background_spawn(turn.send_task)) would + // deadlock if the agent never responded to CancelNotification. + drop(turn.send_task); + Task::ready(()) } fn mark_pending_tools_as_canceled(&mut self) { diff --git a/crates/external_websocket_sync/e2e-test/CLAUDE.md b/crates/external_websocket_sync/e2e-test/CLAUDE.md new file mode 100644 index 00000000000000..beadff5203b0f5 --- /dev/null +++ b/crates/external_websocket_sync/e2e-test/CLAUDE.md @@ -0,0 +1,45 @@ +# E2E Test Requirements + +## CRITICAL: Always Run Latest Code + +Before running the E2E test, you MUST ensure BOTH binaries are current: + +### Zed binary (Rust side) +```bash +# 1. Check if zed-build binary matches current zed repo HEAD +cd ~/pm/zed && git log --oneline -1 +stat -c '%y' ~/pm/helix/zed-build/zed + +# 2. If stale, rebuild: +cd ~/pm/helix && ./stack build-zed dev # ~3min dev, ~12min release + +# 3. ALWAYS copy to e2e test dir before running: +cp ~/pm/helix/zed-build/zed ~/pm/zed/crates/external_websocket_sync/e2e-test/zed-binary +``` + +### Go test server (Helix side) +The `run_docker_e2e.sh` script rebuilds this automatically from the local helix checkout (via the `replace` directive in go.mod). Pass `--no-build` ONLY if you are certain no Go code has changed. + +If `go mod tidy` fails, run it manually first: +```bash +cd ~/pm/zed/crates/external_websocket_sync/e2e-test/helix-ws-test-server && go mod tidy +``` + +### Verification +The test prints binary timestamps and md5 checksums at startup: +``` +=== Binary versions === + zed-binary: 2026-03-24 11:17:23 cc426038391008b2 + helix-ws-test-server: 2026-03-24 14:40:21 6f17f887e47e +``` +CHECK THESE. If the zed-binary timestamp is older than the latest zed commit, it's stale. + +## Running the test +```bash +cd ~/pm/zed/crates/external_websocket_sync/e2e-test +./run_docker_e2e.sh # full rebuild +./run_docker_e2e.sh --no-build # ONLY if Go code hasn't changed +``` + +## Never trust `--no-build` +When investigating test failures, ALWAYS do a full rebuild. Stale binaries are a common source of confusion. diff --git a/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.mod b/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.mod index 822237d0ef4ded..085e777507cb32 100644 --- a/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.mod +++ b/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.mod @@ -40,7 +40,7 @@ require ( github.com/bahlo/generic-list-go v0.2.0 // indirect github.com/bradfitz/gomemcache v0.0.0-20250403215159-8d39553ac7cf // indirect github.com/bradleyfalzon/ghinstallation/v2 v2.17.0 // indirect - github.com/buger/jsonparser v1.1.1 // indirect + github.com/buger/jsonparser v1.1.2 // indirect github.com/bwmarrin/discordgo v0.28.1 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect @@ -136,7 +136,7 @@ require ( github.com/hashicorp/go-retryablehttp v0.7.8 // indirect github.com/hashicorp/go-version v1.8.0 // indirect github.com/hashicorp/golang-lru/v2 v2.0.7 // indirect - github.com/helixml/kodit v1.2.0 // indirect + github.com/helixml/kodit v1.2.1 // indirect github.com/huandu/xstrings v1.5.0 // indirect github.com/infracloudio/msbotbuilder-go v0.2.5 // indirect github.com/invopop/jsonschema v0.13.0 // indirect @@ -193,7 +193,7 @@ require ( github.com/nats-io/nuid v1.0.1 // indirect github.com/nikoksr/notify v0.41.0 // indirect github.com/nikolalohinski/gonja v1.5.3 // indirect - github.com/oapi-codegen/runtime v1.2.0 // indirect + github.com/oapi-codegen/runtime v1.3.0 // indirect github.com/oasdiff/yaml v0.0.0-20250309154309-f31be36b4037 // indirect github.com/oasdiff/yaml3 v0.0.0-20250309153720-d2182401db90 // indirect github.com/oklog/ulid/v2 v2.1.0 // indirect @@ -286,7 +286,7 @@ require ( golang.org/x/mod v0.33.0 // indirect golang.org/x/net v0.50.0 // indirect golang.org/x/oauth2 v0.35.0 // indirect - golang.org/x/sync v0.19.0 // indirect + golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.41.0 // indirect golang.org/x/term v0.40.0 // indirect golang.org/x/text v0.34.0 // indirect diff --git a/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.sum b/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.sum index 395b63f764214d..0885944e487633 100644 --- a/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.sum +++ b/crates/external_websocket_sync/e2e-test/helix-ws-test-server/go.sum @@ -172,8 +172,8 @@ github.com/bsm/ginkgo/v2 v2.12.0 h1:Ny8MWAHyOepLGlLKYmXG4IEkioBysk6GpaRTLC8zwWs= github.com/bsm/ginkgo/v2 v2.12.0/go.mod h1:SwYbGRRDovPVboqFv0tPTcG1sN61LM1Z4ARdbAV9g4c= github.com/bsm/gomega v1.27.10 h1:yeMWxP2pV2fG3FgAODIY8EiRE3dy0aeFYt4l7wh6yKA= github.com/bsm/gomega v1.27.10/go.mod h1:JyEr/xRbxbtgWNi8tIEVPUYZ5Dzef52k01W3YH0H+O0= -github.com/buger/jsonparser v1.1.1 h1:2PnMjfWD7wBILjqQbt530v576A/cAbQvEW9gGIpYMUs= -github.com/buger/jsonparser v1.1.1/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0= +github.com/buger/jsonparser v1.1.2 h1:frqHqw7otoVbk5M8LlE/L7HTnIq2v9RX6EJ48i9AxJk= +github.com/buger/jsonparser v1.1.2/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0= github.com/bugsnag/bugsnag-go v1.4.0/go.mod h1:2oa8nejYd4cQ/b0hMIopN0lCRxU0bueqREvZLWFrtK8= github.com/bugsnag/panicwrap v1.2.0/go.mod h1:D/8v3kj0zr8ZAKg1AQ6crr+5VwKN5eIywRkfhyM/+dE= github.com/bwmarrin/discordgo v0.28.1 h1:gXsuo2GBO7NbR6uqmrrBDplPUx2T3nzu775q/Rd1aG4= @@ -520,8 +520,8 @@ github.com/hashicorp/go-version v1.8.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09 github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= -github.com/helixml/kodit v1.2.0 h1:CHbK9cxe1VH/kw2UNwAeSj2qCP5Qea4bHA2anxhyzI0= -github.com/helixml/kodit v1.2.0/go.mod h1:7eAxW0jhqp2feKjQubpC5TB81cUz/+V6nLeXCJoue/k= +github.com/helixml/kodit v1.2.1 h1:m4bwhEuhtYrx8+SoHryushOVR1oF8rKTStwH21vZbtU= +github.com/helixml/kodit v1.2.1/go.mod h1:bwT0baMTiauunMRF1dAG4cNh6Gw5d8bos28WwajJYWA= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= github.com/huandu/xstrings v1.5.0 h1:2ag3IFq9ZDANvthTwTiqSSZLjDc+BedvHPAp5tJy2TI= github.com/huandu/xstrings v1.5.0/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE= @@ -706,8 +706,8 @@ github.com/nikolalohinski/gonja v1.5.3 h1:GsA+EEaZDZPGJ8JtpeGN78jidhOlxeJROpqMT9 github.com/nikolalohinski/gonja v1.5.3/go.mod h1:RmjwxNiXAEqcq1HeK5SSMmqFJvKOfTfXhkJv6YBtPa4= github.com/nxadm/tail v1.4.11 h1:8feyoE3OzPrcshW5/MJ4sGESc5cqmGkGCWlco4l0bqY= github.com/nxadm/tail v1.4.11/go.mod h1:OTaG3NK980DZzxbRq6lEuzgU+mug70nY11sMd4JXXHc= -github.com/oapi-codegen/runtime v1.2.0 h1:RvKc1CVS1QeKSNzO97FBQbSMZyQ8s6rZd+LpmzwHMP4= -github.com/oapi-codegen/runtime v1.2.0/go.mod h1:Y7ZhmmlE8ikZOmuHRRndiIm7nf3xcVv+YMweKgG1DT0= +github.com/oapi-codegen/runtime v1.3.0 h1:vyK1zc0gDWWXgk2xoQa4+X4RNNc5SL2RbTpJS/4vMYA= +github.com/oapi-codegen/runtime v1.3.0/go.mod h1:kOdeacKy7t40Rclb1je37ZLFboFxh+YLy0zaPCMibPY= github.com/oasdiff/yaml v0.0.0-20250309154309-f31be36b4037 h1:G7ERwszslrBzRxj//JalHPu/3yz+De2J+4aLtSRlHiY= github.com/oasdiff/yaml v0.0.0-20250309154309-f31be36b4037/go.mod h1:2bpvgLBZEtENV5scfDFEtB/5+1M4hkQhDQrccEJ/qGw= github.com/oasdiff/yaml3 v0.0.0-20250309153720-d2182401db90 h1:bQx3WeLcUWy+RletIKwUIt4x3t8n2SxavmoclizMb8c= @@ -1100,8 +1100,8 @@ golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= -golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= +golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/crates/external_websocket_sync/e2e-test/helix-ws-test-server/main.go b/crates/external_websocket_sync/e2e-test/helix-ws-test-server/main.go index 52f754d0edd93e..72dbd245a792b4 100644 --- a/crates/external_websocket_sync/e2e-test/helix-ws-test-server/main.go +++ b/crates/external_websocket_sync/e2e-test/helix-ws-test-server/main.go @@ -21,6 +21,7 @@ package main import ( + "context" "encoding/json" "fmt" "log" @@ -138,7 +139,7 @@ func (d *testDriver) syncEventCallback(sessionID string, syncMsg *types.SyncMess return } - case "thread_created", "user_created_thread": + case "thread_created": acpThreadID, _ := syncMsg.Data["acp_thread_id"].(string) if acpThreadID == "" { acpThreadID, _ = syncMsg.Data["context_id"].(string) @@ -148,7 +149,7 @@ func (d *testDriver) syncEventCallback(sessionID string, syncMsg *types.SyncMess d.round.phase1ThreadCreated = time.Now() } d.round.threadIDs = append(d.round.threadIDs, acpThreadID) - log.Printf("[%s] Thread #%d: %s (event=%s)", d.round.agentName, len(d.round.threadIDs), truncate(acpThreadID, 16), syncMsg.EventType) + log.Printf("[%s] Thread #%d: %s (event=%s)", d.round.agentName, len(d.round.threadIDs), syncMsg.EventType, truncate(acpThreadID, 16)) // Capture the thread created for phase 8 so we can send the interrupt to it. if d.phase == 8 && d.round.phase8ThreadID == "" { d.round.phase8ThreadID = acpThreadID @@ -156,6 +157,17 @@ func (d *testDriver) syncEventCallback(sessionID string, syncMsg *types.SyncMess } } + case "user_created_thread": + // Spontaneous threads created by Zed (e.g. on startup). The production + // handler creates a child session for these, but they are NOT used for + // test phase follow-ups — only thread_created from chat_message responses + // go into threadIDs. Phase 10 tests user_created_thread separately via + // ProcessSyncEvent injection. + acpThreadID, _ := syncMsg.Data["acp_thread_id"].(string) + if acpThreadID != "" { + log.Printf("[%s] Spontaneous user_created_thread: %s (not tracked for phases)", d.round.agentName, truncate(acpThreadID, 16)) + } + case "message_added": // Ignore message_added events for threads from previous rounds. // Check if the thread ID belongs to the current round's tracked threads. @@ -1112,17 +1124,21 @@ func (d *testDriver) validateStore() bool { } } - // Expect at least 6 completed interactions per round (phases 1-5, 7 at minimum) - expectedCompleted := 6 * len(d.agentRounds) + // Expect at least 5 completed interactions per round: + // - Phase 1: thread_created → new session + interaction + // - Phase 3: thread_created → new session + interaction + // - Phase 5: message_added(role=user) → on-the-fly interaction + // - Phase 8: thread_created → new session + interaction + // - Phase 9: on-the-fly interaction (from user interrupt) + // Follow-up phases (2, 4, 7) update existing interactions rather than creating new ones. + // This matches production behavior where Helix tracks one interaction per turn. + expectedCompleted := 5 * len(d.agentRounds) if completedInteractions < expectedCompleted { errors = append(errors, fmt.Sprintf("Expected at least %d completed interactions, got %d", expectedCompleted, completedInteractions)) } - // Expect at least 6 interactions WITH content per round (phases 1, 2, 3, 4, 5, 7 at minimum). - // The server now creates on-the-fly interactions for follow-up assistant turns where - // Zed skips message_added(role=user), so all follow-up phases produce content. - // This catches genuine accumulation failures where content is silently dropped. - expectedWithContent := 6 * len(d.agentRounds) + // Expect at least 5 interactions WITH content per round (same reasoning as above). + expectedWithContent := 5 * len(d.agentRounds) if completedWithContent < expectedWithContent { errors = append(errors, fmt.Sprintf("Expected at least %d completed interactions with content, got %d (accumulation may be broken)", expectedWithContent, completedWithContent)) @@ -1241,6 +1257,29 @@ func main() { store := memorystore.New() ps := pubsub.NewNoop() + // Seed a session matching HELIX_SESSION_ID so the production handler + // can look it up. In production, sessions always exist before Zed connects + // (created by spectask/session creation flow). Without this, handlers like + // handleUserCreatedThread fail with "session not found" because they call + // GetSession(agentSessionID) expecting a real session. + seedSessionID := os.Getenv("HELIX_SESSION_ID") + if seedSessionID == "" { + seedSessionID = "ses_e2e-test-session-001" + } + seedSession := types.Session{ + ID: seedSessionID, + Name: "E2E Test Seed Session", + Created: time.Now(), + Updated: time.Now(), + Owner: "e2e-test-user", + Mode: types.SessionModeInference, + Type: types.SessionTypeText, + } + if _, err := store.CreateSession(context.Background(), seedSession); err != nil { + log.Fatalf("[test-server] Failed to create seed session: %v", err) + } + log.Printf("[test-server] Created seed session: %s", seedSessionID) + // Create HelixAPIServer with production handlers + in-memory store srv := server.NewTestServer(store, ps) diff --git a/crates/external_websocket_sync/e2e-test/run_e2e.sh b/crates/external_websocket_sync/e2e-test/run_e2e.sh index 0b109f0bccbc9a..5fc567ad842cec 100755 --- a/crates/external_websocket_sync/e2e-test/run_e2e.sh +++ b/crates/external_websocket_sync/e2e-test/run_e2e.sh @@ -41,6 +41,19 @@ cleanup() { [ -n "${XVFB_PID:-}" ] && kill "$XVFB_PID" 2>/dev/null || true rm -f "$MOCK_PORT_FILE" + # Dump Zed errors/panics (full log available at ZED_LOG_FILE) + if [ -f "${ZED_LOG_FILE:-}" ]; then + ZED_ERRORS=$(grep -ciE "panic|error|fatal" "$ZED_LOG_FILE" 2>/dev/null || echo "0") + if [ "$ZED_ERRORS" -gt 0 ]; then + echo "" + echo "==================================================" + echo " ZED PROCESS ERRORS ($ZED_ERRORS lines)" + echo "==================================================" + grep -iE "panic|error|fatal" "$ZED_LOG_FILE" | tail -50 || true + echo " (full log: $ZED_LOG_FILE)" + fi + fi + # Report screenshots if [ -d "$SCREENSHOT_DIR" ]; then SHOT_COUNT=$(ls -1 "$SCREENSHOT_DIR"/*.png 2>/dev/null | wc -l) @@ -123,7 +136,7 @@ export ZED_HELIX_URL="127.0.0.1:${MOCK_PORT}" export ZED_HELIX_TOKEN="test-token" export ZED_HELIX_TLS=false export ZED_HELIX_SKIP_TLS_VERIFY=false -export HELIX_SESSION_ID="e2e-test-session-001" +export HELIX_SESSION_ID="ses_e2e-test-session-001" # ---- Determine which agents to test ---- # E2E_AGENTS controls which agent rounds to run. Default: zed-agent only. @@ -202,12 +215,14 @@ echo "[zed] ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY:+set (${#ANTHROPIC_API_KEY} echo "[zed] E2E_AGENTS=$E2E_AGENTS" echo "" -# Start Zed +# Start Zed (capture logs for debugging) +ZED_LOG_FILE="/tmp/zed-e2e.log" "$ZED_BINARY" \ --allow-multiple-instances \ "$PROJECT_DIR" \ - & + > "$ZED_LOG_FILE" 2>&1 & ZED_PID=$! +echo "[zed] Logs: $ZED_LOG_FILE" echo "[zed] Started (PID $ZED_PID)" echo "" From 6e0e6db32b895e79f5d6faf7cca42912009f07fb Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Wed, 25 Mar 2026 03:45:39 +0000 Subject: [PATCH 2/4] fix: drop cancel task, disable local claude-agent-acp mount, log versions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - cancel(): drop send_task instead of awaiting it to avoid deadlock when ACP agents don't respond to CancelNotification (claude-agent-acp#442, #423) - Disable local claude-agent-acp mount in E2E test — let Zed auto-install latest from npm to match production behavior - Log claude-agent-acp version (local or npm) for debugging test failures Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/acp_thread/src/acp_thread.rs | 13 +++++++++---- .../e2e-test/run_docker_e2e.sh | 15 +++++++++------ .../external_websocket_sync/e2e-test/run_e2e.sh | 8 ++++++-- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index fbc11fb9e4b8c6..6769388fbca282 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -2318,12 +2318,17 @@ impl AcpThread { // Drop the send_task instead of awaiting it. This cancels the prompt // future immediately, which drops the oneshot `tx`. The `rx.await` in // run_turn then returns Err, hitting the existing "tx dropped" handler - // that emits Stopped(Cancelled) (see run_turn line ~2224). + // that emits Stopped(Cancelled) (see the `let Ok(response) = response` + // branch in run_turn). // // We still call connection.cancel() above as a courtesy notification - // to the agent, but we don't wait for the agent to acknowledge it. - // The previous approach (cx.background_spawn(turn.send_task)) would - // deadlock if the agent never responded to CancelNotification. + // to the agent. But we don't wait for the agent to acknowledge it — + // ACP agents that don't properly handle CancelNotification (see + // claude-agent-acp#442, #423) would block the next turn indefinitely. + // + // The previous approach was: + // cx.background_spawn(turn.send_task) + // which awaited the prompt future to completion before proceeding. drop(turn.send_task); Task::ready(()) } diff --git a/crates/external_websocket_sync/e2e-test/run_docker_e2e.sh b/crates/external_websocket_sync/e2e-test/run_docker_e2e.sh index 476db71164e7c4..fb2e5cfeea65b5 100755 --- a/crates/external_websocket_sync/e2e-test/run_docker_e2e.sh +++ b/crates/external_websocket_sync/e2e-test/run_docker_e2e.sh @@ -86,13 +86,16 @@ mkdir -p "$SCREENSHOTS_DIR" E2E_AGENTS="${E2E_AGENTS:-zed-agent}" echo "=== Running E2E test (agents: $E2E_AGENTS) ===" -# Mount local claude-agent-acp if available (for testing local changes) +# Mount local claude-agent-acp if available (for testing local changes). +# Disabled by default: let Zed auto-install the latest from npm via npx, +# which ensures we test against the same version users get. +# To test local changes, uncomment the block below. CLAUDE_ACP_MOUNT="" -CLAUDE_ACP_DIR="$ZED_DIR/../claude-agent-acp" -if [ -d "$CLAUDE_ACP_DIR/dist" ] && echo "$E2E_AGENTS" | grep -q "claude"; then - CLAUDE_ACP_MOUNT="-v $(cd "$CLAUDE_ACP_DIR" && pwd):/opt/claude-agent-acp" - echo "[setup] Mounting local claude-agent-acp from $CLAUDE_ACP_DIR" -fi +# CLAUDE_ACP_DIR="$ZED_DIR/../claude-agent-acp" +# if [ -d "$CLAUDE_ACP_DIR/dist" ] && echo "$E2E_AGENTS" | grep -q "claude"; then +# CLAUDE_ACP_MOUNT="-v $(cd "$CLAUDE_ACP_DIR" && pwd):/opt/claude-agent-acp" +# echo "[setup] Mounting local claude-agent-acp from $CLAUDE_ACP_DIR" +# fi ANTHROPIC_BASE_URL_ARG="" if [ -n "${ANTHROPIC_BASE_URL:-}" ]; then diff --git a/crates/external_websocket_sync/e2e-test/run_e2e.sh b/crates/external_websocket_sync/e2e-test/run_e2e.sh index 5fc567ad842cec..bbc704187cceb4 100755 --- a/crates/external_websocket_sync/e2e-test/run_e2e.sh +++ b/crates/external_websocket_sync/e2e-test/run_e2e.sh @@ -161,9 +161,13 @@ if echo "$E2E_AGENTS" | grep -q "claude"; then CLAUDE_PATH_JSON="" if [ -f "/opt/claude-agent-acp/dist/index.js" ]; then CLAUDE_PATH_JSON="\"path\": \"node\", \"args\": [\"/opt/claude-agent-acp/dist/index.js\"]," - echo "[setup] Using LOCAL claude-agent-acp from /opt/claude-agent-acp" + LOCAL_VERSION=$(node -e "console.log(require('/opt/claude-agent-acp/package.json').version)" 2>/dev/null || echo "unknown") + echo "[setup] Using LOCAL claude-agent-acp v$LOCAL_VERSION from /opt/claude-agent-acp" else - echo "[setup] Using npm-installed claude-agent-acp (auto-install)" + # Log which version npx will install so we can correlate failures + # with claude-agent-acp upgrades. This is a quick check, not an install. + CLAUDE_ACP_VERSION=$(npm view @anthropic-ai/claude-agent-acp version 2>/dev/null || echo "unknown") + echo "[setup] Using npm-installed claude-agent-acp (auto-install, latest=$CLAUDE_ACP_VERSION)" fi AGENT_SERVERS_JSON=$(cat << AGENTEOF "agent_servers": { From 68f8c8183f481e23d2737d4c5bd105a88efb6256 Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Wed, 25 Mar 2026 03:48:15 +0000 Subject: [PATCH 3/4] docs: update porting guide for E2E test fixes and cancel drop fix - Update E2E test section: 10 phases (was 7), seed session requirement, claude-agent-acp auto-install from npm, both agent rounds - Add Critical Fix #8: cancel() must drop send_task not await it - Add rebase checklist item #31 for cancel fix - Add commit history entries for seed session and cancel fixes Co-Authored-By: Claude Opus 4.6 (1M context) --- portingguide.md | 59 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/portingguide.md b/portingguide.md index 874cbe76bd1271..08d14b5031392d 100644 --- a/portingguide.md +++ b/portingguide.md @@ -43,28 +43,36 @@ The entire crate is Helix-specific. It provides: ### E2E Test (`e2e-test/`) -Seven-phase test that validates the full protocol. Runs in Docker against a real LLM (Anthropic API). Two Dockerfiles: +Ten-phase test that validates the full protocol for both `zed-agent` and `claude` (Claude Code) agents. Runs in Docker against a real LLM (Anthropic API). The Go test server uses the **same production Helix handler code** (`NewTestServer` + `ExternalAgentSyncHandler`) with an in-memory store. + +Two Dockerfiles: - `Dockerfile.runtime` — for local dev runs (`run_docker_e2e.sh`) - `Dockerfile.ci` — for CI (takes pre-built Zed binary + Helix Go source as build context) +**Important:** The test creates a seed session in the store before Zed connects (matching `HELIX_SESSION_ID`). This mirrors production where sessions always exist before the agent connects. Without it, `handleUserCreatedThread` fails with "session not found". See `CLAUDE.md` in the e2e-test directory for binary freshness requirements. + Phases: 1. **Phase 1**: New thread creation via `chat_message` 2. **Phase 2**: Follow-up message to existing thread 3. **Phase 3**: New thread creation (second thread) 4. **Phase 4**: Follow-up to non-visible thread (Thread A while Thread B is displayed) -5. **Phase 5**: `message_completed` emitted after `Stopped` for all turn sources -6. **Phase 6**: Mid-stream interrupt (second `send()` displaces active turn, both emit `Stopped`) -7. **Phase 7**: MCP tool call events appear with correct `entry_type`/`tool_name`/`tool_status` +5. **Phase 5**: Simulate user input (Zed → Helix sync direction) +6. **Phase 6**: Query UI state (active_view, thread_id, entry_count, MCP servers, model) +7. **Phase 7**: Open thread + follow-up chat +8. **Phase 8**: Mid-stream interrupt (second `send()` displaces active turn, both emit `Stopped`) +9. **Phase 9**: Rapid 3-turn cancel (chat_message, then simulate_user_input + chat_message back-to-back) +10. **Phase 10**: User-created thread (inject `user_created_thread`, verify session + work session) -Each phase also queries UI state via `query_ui_state` to verify the agent panel state. +A `slow-mcp-server` test helper (in `e2e-test/slow-mcp-server/`) simulates an MCP server with delayed tool responses, used to test the `wait_for_tools_ready` path (Phase 1 waits ~30s for MCP tools to load). -A `slow-mcp-server` test helper (in `e2e-test/slow-mcp-server/`) simulates an MCP server with delayed tool responses, used by phases 6 and 7 to test the `wait_for_tools_ready` path. +Claude Code (`claude-agent-acp`) is auto-installed from npm by Zed at runtime — the test does NOT bundle a local copy. The version is logged at test start for debugging. ```bash -# Run E2E test (local) +# Run E2E test (local) — ALWAYS copy latest binary first! cd crates/external_websocket_sync/e2e-test -cp ../../zed-build/zed zed-binary -./run_docker_e2e.sh # builds Go test server + Docker image + runs test +cp ~/pm/helix/zed-build/zed zed-binary +./run_docker_e2e.sh # zed-agent only +E2E_AGENTS="zed-agent,claude" ./run_docker_e2e.sh # both agents # Screenshots saved to ./screenshots/ ``` @@ -134,6 +142,7 @@ These files contain Helix-specific changes that must be preserved during rebases ### `crates/acp_thread/src/acp_thread.rs` - **`content_only()` method on `AssistantMessage`**: Returns content without the `## Assistant\n\n` heading. Used by thread_service.rs for WebSocket sync to avoid sending the heading to Helix. - **`AcpThreadEvent::Stopped` is a tuple variant**: As of the 2026-03-22 upstream merge, `Stopped` takes a `StopReason` argument: `Stopped(acp::StopReason)`. Pattern matches must use `Stopped(_)` and emission must pass a reason, e.g. `cx.emit(AcpThreadEvent::Stopped(acp::StopReason::Cancelled))`. +- **`cancel()` drops send_task instead of awaiting**: See Critical Fix #8 below. ### `crates/acp_thread/src/connection.rs` - **`wait_for_tools_ready()` on `AgentConnection` trait**: New method added to `AgentConnection`. Default impl returns `Task::ready(())`. `HeadlessConnection` relies on the default. `NativeAgentConnection` implementation in `context_server_registry.rs` waits for all pending MCP tool loads. **When upstream adds methods to `AgentConnection`, `HeadlessConnection` must be updated** — it won't compile otherwise. @@ -290,6 +299,33 @@ fn load_session(self: Rc, session: AgentSessionInfo, ..., cx: &mut App) **Symptom:** After container restart, Zed works fine locally but all Helix messages are silently swallowed — no responses appear in the Helix session. +### 8. Cancel Must Drop send_task, Not Await It + +**File:** `crates/acp_thread/src/acp_thread.rs` — `AcpThread::cancel()` + +**Bug:** `cancel()` called `cx.background_spawn(turn.send_task)` to wait for the old turn's prompt future to complete before starting the next turn. This required the ACP agent to properly respond to `CancelNotification`. Claude Code's `claude-agent-acp` has multiple bugs where cancel doesn't cause the prompt to return (see [#442](https://github.com/zed-industries/claude-agent-acp/issues/442), [#423](https://github.com/zed-industries/claude-agent-acp/pull/423)), causing `cancel()` to block indefinitely and the next turn to never start. + +**Fix:** `drop(turn.send_task)` instead of awaiting it. Dropping the GPUI Task cancels the prompt future, which drops the oneshot `tx`. The `rx.await` in `run_turn` then returns `Err`, hitting the existing "tx dropped" handler that emits `Stopped(Cancelled)`. The `connection.cancel()` notification is still sent as a courtesy, but we don't wait for acknowledgement. + +```rust +pub fn cancel(&mut self, cx: &mut Context) -> Task<()> { + let Some(turn) = self.running_turn.take() else { + return Task::ready(()); + }; + self.connection.cancel(&self.session_id, cx); + Self::flush_streaming_text(&mut self.streaming_text_buffer, cx); + self.mark_pending_tools_as_canceled(); + + // Drop instead of: cx.background_spawn(turn.send_task) + drop(turn.send_task); + Task::ready(()) +} +``` + +**History:** Fixed in `6e0e6db32b`. The previous approach (`cx.background_spawn`) worked for NativeAgent (which responds to cancel immediately) but deadlocked with Claude Code. + +**Symptom:** Phase 8 (mid-stream interrupt) times out for Claude Code agent. User pressing stop/interrupt in Zed while Claude Code is streaming causes the thread to hang permanently. + ## Environment Variables | Variable | Purpose | Default | @@ -358,7 +394,8 @@ When rebasing/merging against upstream Zed: 28. **Check `SyncEvent::MessageAdded`** — has `entry_type`, `tool_name`, `tool_status` fields 29. **Check `SyncEvent::UiStateResponse`** — has `mcp_servers` and `active_model` fields 30. **Check `NativeAgent` multi-project**: `agent.projects.values().next()` to get `ProjectState`; no more flat `agent.project` or `agent.context_server_registry()` fields/methods -31. **Run `cargo check --package zed --features external_websocket_sync`** — must compile +31. **Check `acp_thread.rs` `cancel()`** — must `drop(turn.send_task)` not `cx.background_spawn(turn.send_task)` (Critical Fix #8) +32. **Run `cargo check --package zed --features external_websocket_sync`** — must compile 32. **Run `cargo test -p external_websocket_sync`** — unit tests 33. **Run E2E test** after merge to verify all phases pass @@ -422,3 +459,5 @@ Helix-specific commits on main (oldest first): | `bfe84a2134` | Send structured tool_name and tool_status in message_added events | | `e38aad1a18` | Clear persistent subscription on unregister to fix E2E timeout | | `8b033a4451` | **Test: add Stopped emission and mid-stream interrupt E2E tests (Critical Fix #6)** | +| `85be6df7b6` | **Fix: E2E seed session, user_created_thread tracking, interaction count** | +| `6e0e6db32b` | **Fix: drop cancel task to prevent deadlock with Claude Code (Critical Fix #8)** | From b9b66835024599077e43de79513027577975afdc Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Wed, 25 Mar 2026 03:48:36 +0000 Subject: [PATCH 4/4] docs: note cancel drop fix should be kept as defensive measure Even after claude-agent-acp cancel bugs are fixed, the drop approach protects against any ACP agent that doesn't properly handle CancelNotification. Co-Authored-By: Claude Opus 4.6 (1M context) --- portingguide.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/portingguide.md b/portingguide.md index 08d14b5031392d..194c9447028748 100644 --- a/portingguide.md +++ b/portingguide.md @@ -326,6 +326,8 @@ pub fn cancel(&mut self, cx: &mut Context) -> Task<()> { **Symptom:** Phase 8 (mid-stream interrupt) times out for Claude Code agent. User pressing stop/interrupt in Zed while Claude Code is streaming causes the thread to hang permanently. +**Note:** Even if the claude-agent-acp cancel bugs (#442, #423) are fully fixed upstream, the drop approach should be kept as a defensive measure. Any ACP agent that doesn't properly respond to `CancelNotification` would cause the same deadlock. The drop approach makes Zed resilient to buggy agent implementations without changing protocol semantics (the cancel notification is still sent). + ## Environment Variables | Variable | Purpose | Default |