diff --git a/design/2026-07-07-spec-task-attachment-upload-race.md b/design/2026-07-07-spec-task-attachment-upload-race.md new file mode 100644 index 0000000000..6174e5ed02 --- /dev/null +++ b/design/2026-07-07-spec-task-attachment-upload-race.md @@ -0,0 +1,93 @@ +# Spec-task attachment lost: upload races start-planning, never staged into the repo + +**Date:** 2026-07-07 +**Task that surfaced it:** `spt_01kwxcg8vk3fsy35x247brmr46` (Helix project) +**Attachment:** `att_01kwxcgae…` — `Screenshot 2026-07-07 at 05.13.29.png` (2.25 MB) +**Instance:** meta.helix.ml (localhost dev stack) + +## Symptom + +The agent reported it could not find an attachment the user is sure they added. +The attachment row and filestore blob exist, but the file never appears in the +agent's workspace at `design/tasks//attachments/`. + +## Root cause — a race between three independent client requests + +`CreateTaskFromPrompt` deliberately does **not** auto-start planning ("start in +backlog, wait for explicit user action"). So the UI create-with-attachment flow +issues three separate HTTP requests: + +| Time (UTC) | Request | Effect | +|---|---|---| +| 04:13:42.643 | `POST /spec-tasks/from-prompt` | task created | +| 04:13:42.716 | `POST /spec-tasks/{id}/start-planning` | planning begins (**73 ms later**) | +| 04:13:42.774 | — | planning kickoff prompt **built**; `stageAttachmentsAndBuildPromptSection` runs here | +| 04:13:44.293 | `POST /spec-tasks/{id}/attachments` | attachment upload completes — **1.5 s too late** | + +The 2.25 MB PNG upload is slower than the `start-planning` call that was fired +right behind the create. `start-planning` is the **only** place attachments get +staged into the repo (`stageAttachmentsAndBuildPromptSection` → +`commitAttachmentsToHelixSpecs`, in `pkg/services/spec_task_attachments.go`). At +staging time `ListSpecTaskAttachments` returned **zero** rows, so: + +- nothing was committed to the helix-specs branch → `committed_sha` stays `""`; +- the prompt's "attachments" section was empty → the agent was never told; +- the file never reached `design/tasks/002234_we-basically-need-to/attachments/`. + +### Evidence + +- `spec_task_attachments.committed_sha = ''` for `att_01kwxcgae…`. +- No `Committed attachments to helix-specs branch` log for this task (there **is** + one for `spt_01kwwcrew…`, proving the mechanism works when ordering is right). +- helix-specs branch has `design/tasks/002234_we-basically-need-to/{design,requirements,tasks}.md` + and a `screenshots/` dir, but **no `attachments/` dir**. +- `uploadSpecTaskAttachments` writes to filestore + creates the DB row only — it + does **not** stage into the repo. Staging is exclusively at planning time. + +### Why the CLI is immune + +`helix spectask start --attach` runs `createSpecTask` → `uploadSpecTaskAttachments` +→ **then** `triggerStartPlanning` (synchronous, in that order). The attachment +always exists before staging runs. So CLI-attached tasks work (that's why the +de-dup task's attachment staged fine). The bug lives in the UI create+attach +flow, where the three requests race and the slow upload loses. + +## Fix direction + +Two complementary fixes; the second is the robust one: + +1. **Ordering (UI):** upload attachments *before* calling `start-planning`, or + have `start-planning` wait until in-flight uploads for the task have settled. + Fragile on its own (depends on client sequencing) but cheap. + +2. **Server-side robustness (preferred):** make attachment staging independent of + the planning race. Two sub-parts: + - **`uploadSpecTaskAttachments` stages the blob into helix-specs immediately** + (reuse `commitAttachmentsToHelixSpecs`), so the file lands in + `design/tasks//attachments/` regardless of when it's uploaded — even + after planning started. `commitAttachmentsToHelixSpecs` is already idempotent + (skips rows whose `CommittedSHA` is set), so double-staging is safe. + - **Make the agent aware of late arrivals:** if an attachment lands after the + planning prompt was built, either re-inject an "attachment added" note into + the session (queue path — see #2808 / the sender-unification work) or ensure + the agent is instructed to always check the attachments dir. Without this, the + file is in the workspace but the agent isn't told to look. + +Prefer (2). It removes the race entirely: once uploaded, the file is in the repo, +and the agent is notified — no dependence on request ordering. + +## Files + +| File | Role | +|---|---| +| `api/pkg/server/*` `uploadSpecTaskAttachments` | upload handler — currently filestore + row only; should stage into repo | +| `api/pkg/services/spec_task_attachments.go` | `stageAttachmentsAndBuildPromptSection`, `commitAttachmentsToHelixSpecs` (idempotent) | +| `api/pkg/server/spec_driven_task_handlers.go` | `startPlanning` (where staging currently happens) | +| frontend create+attach flow | issues create / start-planning / upload as separate races | + +## Note on testing this fix + +Reproduce the UI ordering (create → start-planning → slow upload) and assert the +file lands in the repo and the agent sees it. The **CLI `--attach` path will NOT +reproduce the bug** (correct order) — so a fix verified only via CLI attach is not +verified. Drive the racing UI/API sequence directly. diff --git a/design/2026-07-08-we-find-ai-custom-domain-prod-cutover.md b/design/2026-07-08-we-find-ai-custom-domain-prod-cutover.md new file mode 100644 index 0000000000..54086f5df6 --- /dev/null +++ b/design/2026-07-08-we-find-ai-custom-domain-prod-cutover.md @@ -0,0 +1,123 @@ +# we-find.ai custom-domain cutover onto prod SaaS (app.helix.ml) + +Date: 2026-07-08. Goal: serve the **Find AI** web service at the customer apex **we-find.ai** +(+ www) from the **prod** SaaS, cutting it over from its current Replit host. + +## Topology facts (verified live) +- Project (prod): **`prj_01kvz0e7b401545376fyyfxtta`** "Find AI", org `org_01kvzf9s7tarpm9pmg7vmwhfn6`, + web service **enabled**, backend sandbox `sbx_01kwf3fghqychvyahmxs7d3zh5` port 8080, host `code-for-app`. + (The `prj_01kv5j…` the user first linked is the **meta** copy — NOT this one.) +- Prod is `helix-cloud-london` (GCE europe-west2-a). Public ingress IP **34.39.116.64** = `ingress.helix.ml`. +- Prod version **2.11.45** (has TLS-ALPN-01 fallback, commit `eac0aab1f`). #2813 self-serve-acme is + NOT needed for the direct-A approach; it only adds the `_acme-challenge` UI helper. (2.11.46 cut, + CD pending, but not required here.) +- Prod `.env`: `HELIX_VHOST_TLS_MODE=auto`, `HELIX_VHOST_ACME_DNS_PROVIDER=cloudflare`, + `HELIX_VHOST_CNAME_TARGET=ingress.helix.ml`. certmagic uses **production** LE + (find-ai.apps.helix.ml cert issuer = CN=YE1). Staging lines in logs are just certmagic's + post-failure fallback — red herring. + +## nginx edge (prod, /etc/nginx/nginx.conf, monolithic) +- `:443` = **stream ssl_preread** map by SNI: legacy names → 127.0.0.1:8443 (nginx http), + **`default` → 127.0.0.1:8444** = helix-api-1 :443 (certmagic). So unknown SNI (custom domains) + DOES reach certmagic. `8444` confirmed = `127.0.0.1:8444->443/tcp` on helix-api-1. +- `:80` = per-host server blocks (return 301 → https). **No default_server / catch-all** → + unknown host (we-find.ai) gets nginx **404**. + +## DNS-01 vs ALPN (why no _acme-challenge record needed) +`vhost_tls.go` configures two issuers, tried in order: (1) DNS-01 via Cloudflare — only works for +names in the CF zone or with `_acme-challenge.` CNAME delegation; (2) **TLS-ALPN-01 fallback** +over :443 — works for domains pointed DIRECTLY at the origin. we-find.ai will be a plain A → prod +(no CF proxy), so ALPN issues the cert. **No `_acme-challenge` record required.** +Confirmed by logs: DNS-01 failed (`expected 1 zone, got 0` — we-find.ai not in CF), ALPN-01 failed +ONLY because public DNS still points at Replit (34.111.179.208) — self-heals on DNS flip. + +## The one real gap: domain verification over :80 +`webservice/verifier.go` polls every 60s → GET `http:///.well-known/helix-domain-verify/` +(port 80, no redirect-follow). Dispatch (`vhost_middleware.go:149`) returns **503 "domain not yet +verified"** until `verified_at` is set. Prod nginx :80 → 404 for we-find.ai → auto-verify can never +succeed. Cert issuance does NOT need verification (gate allows any vhost_routes row), but **serving +the app does**. + +### Resolution taken +- Added routes via prod API (owner key from DB): `we-find.ai` = `vhr_01kwy2e4bdsved5qqy7gmmzthm`, + `www.we-find.ai` = `vhr_01kwy2e4bvhbvq84s22ynk4rm3`. +- **Manually set `verified_at=now()`** on both (we own the domain; HTTP ownership proof is redundant + for an operator cutover). Reversible: `UPDATE vhost_routes SET verified_at=NULL WHERE id IN (...)`. +- **Pre-flight PASSED**: `curl -H 'Host: we-find.ai' http://localhost:8001/` → 200, 142KB, + `Find AI — AI Talent, Matched with Purpose`, identical to find-ai baseline. +- Proper fix (still TODO, optional): nginx :80 `default_server` proxying + `/.well-known/helix-domain-verify/` → helix (localhost:8001) + `return 301 https://$host` — makes + auto-verify + http→https work for ALL future custom domains. Touches shared prod edge (gate on `nginx -t`). + +## Remaining step (NOT done — needs explicit go: live customer-domain cutover) +123-reg DNS for we-find.ai (registrar; nameservers ns45/ns46.domaincontrol.com): +- apex **A `@`**: `34.111.179.208` → **`34.39.116.64`** (staged in the 123-reg edit form, UNSAVED). +- `www` CNAME → `we-find.ai` already (resolves to prod after apex change) — leave as-is. +- Leave NS, MX (smtp.google.com email), google-site-verification TXT, replit-verify TXT. +- TTL 600s. **Rollback** = set apex A back to `34.111.179.208`. + +## INCIDENT 2026-07-09: find-ai.apps.helix.ml down (blocks switchover) +Discovered while pre-flighting: `find-ai.apps.helix.ml` returning 502 (backend down), +NOT caused by the nginx change (the `:8001` backend path 502s too). + +**Root cause (app):** `.helix/startup.sh` (helix-specs branch, line 56) ran +`exec docker compose -f docker-compose.prod.yml up`, but **`docker-compose.prod.yml` never +existed** in helixml/find-ai (`git log --all` empty). The repo only tracks `docker-compose.yml` +(dev stack: Go `api` on 8080 proxying Next.js `npm run dev` frontend + Postgres). A redeploy +~06:30 UTC killed the old `docker compose up` (→ stack stopped), then the new startup.sh failed +`open docker-compose.prod.yml: no such file` → app never bound :8080 → health-monitor looped +a failed redeploy every ~11 min; rollback also failed (the broken ref is in startup.sh, not the +app commit, so every SHA fails identically). + +**Fix (restore):** committed one-liner to helixml/find-ai@helix-specs (`be2c45d`): +`docker-compose.prod.yml` → `docker-compose.yml`. Triggered +`POST /projects/prj_01kvz0e7b401545376fyyfxtta/web-service/deploy` → deploy `live`, all 3 +containers healthy, `https://find-ai.apps.helix.ml` → 200/0.27s. Recovery loop stopped. +(Follow-up for find-ai team: if a static prod build was intended, add a real +`docker-compose.prod.yml` and re-point startup.sh.) + +**Platform bug (Helix — "make it not happen again", NOT yet done):** `webservice/controller.go` +`runDeploy`/`deployInPlace` (~L173-229, L516-549) kills the running `docker compose up` before the +new startup proves healthy, and on readiness failure `rollback()` (L604-618) re-runs the SAME +startup.sh → also fails → site stays down indefinitely + retries destructively. The header comment +(L9-10) intentionally accepts a *brief* restart window (single /data DB writer ⇒ not trivially +blue-green). Proposed hardening options (needs Luke's call, then code + release + prod deploy): + 1. **Pre-teardown validation** — before killing the running stack, run `docker compose config` + (or check the referenced compose file exists) in the sandbox; abort the deploy and KEEP the + old stack if invalid. Cheap, directly prevents this class (missing/invalid compose). + 2. **Rollback-to-last-KNOWN-GOOD + stop-loop** — if rollback also fails readiness, stop the + auto-retry, mark degraded, and alert (Slack/janitor) instead of looping every 11 min. + 3. (Bigger) true keep-old-until-new-healthy, constrained by the single-DB-writer design. +Recommend 1 + 2 together (small, targeted). we-find.ai switchover HELD until this ships. + +## FOLLOW-UP 2026-07-09: proper prod-build fix (supersedes the dev fallback) +The dev fallback (be2c45d) was a stopgap. Real intent: spec task **#2242 "Serve Production +Build in Helix Web-Service Mode"** (`spt_01kwyaer8v1t9122dr0cfpmyn9`, meta, still status +`implementation`) built the prod compose but was never merged. Its startup.sh half landed on +helix-specs; its app half (docker-compose.prod.yml + api/Dockerfile.prod + /api/version) sat +unmerged on `feature/002242-serve-production-build`. +- Merged that branch → main via **find-ai PR #18** (main now `42e8407`). +- Smoke-built the prod image in the sandbox (isolated `-p ftprodtest build`) — clean, warmed cache. +- Re-pointed startup.sh → docker-compose.prod.yml (helix-specs `bc483dd`), redeployed. +- **Verified live:** `mode=static`, `Serving static frontend from /www`, listening :8080, + `https://find-ai.apps.helix.ml` → 200 (77KB static vs 142KB dev). Prod app+db containers only + (no dev frontend server). Rollback = startup.sh → docker-compose.yml. +- Nit: `/api/version` shows version/gitSha/buildTime = "unknown" (ldflags not injected in the + web-service deploy path) — cosmetic, follow-up. +- Renamed meta's find-ai project (`prj_01kv5j…`) → "Find AI (DEPRECATED – use SaaS)"; its web + service is still enabled at find-ai.meta.helix.ml (disable pending). + +## STILL TODO (user's sequence: harden Helix → THEN we-find.ai switchover) +Helix observability feature (branch `feature/web-service-deploy-logs`, not finished): surface the +sandbox deploy log (`/data/.helix-webservice.log`, read via hydra exec) in the Web Service tab; +friendly "stack didn't bind to port N — view logs" errors (deploy.Error already stored); stop the +public leak (hydra server.go:584 passes the app-down 502 with internal IP straight through — +api proxyToContainer only catches transport errors, not hydra's 5xx passthrough). Do NOT assume +startup.sh uses compose. Then release + deploy to prod, THEN flip we-find.ai DNS. + +## Browser note +chrome-devtools MCP connects to `--browserUrl 127.0.0.1:9222`. Original Chrome there was headless +(invisible on RDP). Fixed by killing it and relaunching **headful on the Wayland session**: +`XDG_RUNTIME_DIR=/run/user/1000 WAYLAND_DISPLAY=wayland-0 DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus +google-chrome-stable --remote-debugging-port=9222 --user-data-dir=/tmp/chrome-visible --ozone-platform=wayland +--no-sandbox ...`. (pkill footgun: `pkill -f 'chrome-mcp'` matched its own shell — use PIDs.) diff --git a/design/2026-07-21-brief-preserve-thread-on-provider-switch.md b/design/2026-07-21-brief-preserve-thread-on-provider-switch.md new file mode 100644 index 0000000000..6d71088520 --- /dev/null +++ b/design/2026-07-21-brief-preserve-thread-on-provider-switch.md @@ -0,0 +1,82 @@ +# Fix: switching an agent's model/provider/credential must NOT discard the Zed conversation thread + +## Problem (real incident, meta prod, 2026-07-21) + +A long-running spec-task session (`spt_01kvtnrkgp5t2a7n4pwcv2cb8j`, "LinkedIn +Outreach", owned by Chris) had a healthy Claude Code (ACP) conversation thread +`bd5abc10-…` — a 569 MB jsonl at +`~/.claude-state/projects/-home-retro-work/.jsonl` on the workspace +volume, ~869 messages deep. A user opened the agent settings and switched it from +Anthropic **API key** mode to **subscription** mode (and picked a different +model, opus 4.8). Immediately after, the session's `config.zed_thread_id` pointer +was **cleared to empty**. The next messages therefore dispatched with an empty +`acp_thread_id` / `first_message=true`, so Zed **forked brand-new empty threads** +(`2c1b6724`, `1151c086`) — total, silent loss of the agent's working context (the +Helix UI transcript still showed the old messages, but the agent had forgotten +everything). Recovery required manually repointing `config.zed_thread_id` back to +`bd5abc10` in Postgres. + +## Root cause to confirm and fix + +There are multiple code paths that set `session.Metadata.ZedThreadID = ""`: +- `api/pkg/server/session_handlers.go` `restartSessionContainer` — **already fixed** + by PR #2860 (https://github.com/helixml/helix/pull/2860): it now preserves a + healthy thread via `lastInteractionCompletedCleanly()` and only resets when the + thread looks wedged. +- `api/pkg/server/session_switch_agent_handlers.go:~237` — still sets + `session.Metadata.ZedThreadID = ""` **unconditionally**. Comment at ~line 231 + ("Repoint the session's agent in place. Clearing ZedThreadID makes the …") + and ~325 ("a successful switch always ends with a fresh thread id"). +- `api/pkg/server/session_clear.go` — the explicit /clear (leave as-is; that's + intentional). + +**Your first job: confirm exactly which path fired during a +provider/model/credential change** (add logging / reproduce). The restart-preserve +fix (#2860) was already deployed on meta, and the last interaction before the loss +was `complete` (healthy), so a plain restart would have *preserved* the thread — +which means the clear almost certainly came from the **switch-agent path** (or an +app-config-edit path that re-provisions the session), not the restart path. +Reproduce it: create a zed_external claude_code spec task, send a couple of +messages so a thread exists (`config->>'zed_thread_id'` is a non-empty UUID), then +change the agent's model / provider / credential_type in settings and observe +whether `zed_thread_id` gets cleared. + +## The fix + +Switching the **LLM model, provider, or credential type** (api_key ⇄ subscription) +within the **same** `code_agent_runtime` (e.g. claude_code → claude_code) must +**preserve** the existing Zed thread — the conversation is model-agnostic ACP +state and there is no reason to discard it. Apply the same principle as #2860: +only clear `ZedThreadID` when either +1. the **agent kind genuinely changes** such that the old thread state is + incompatible (e.g. `zed-agent` ⇄ `claude_code`, different ACP agent binaries / + thread stores), OR +2. the thread is **wedged** (last interaction not in a clean terminal state — reuse + `lastInteractionCompletedCleanly` or equivalent). + +For a pure model/provider/credential change, keep the thread and let the reconnect +`open_thread` (`websocket_external_agent_sync.go:~439`) re-attach. Note the token +is injected at desktop-start (`external_agent_handlers.go` `subscriptionEnvForSession`), +so a model/provider switch that needs new env should still recreate the desktop — +but **preserve the thread pointer** across that recreate. + +## Acceptance criteria (must test live in the inner Helix — this is a lifecycle change) + +Per the repo's testing rules, lifecycle changes MUST be tested against a LIVE, +connected Zed, not seeded DB rows. Create a spec task, get a live thread +(`config->>'zed_thread_id'` = non-empty UUID), then: +1. Change **model** (e.g. opus → sonnet) → send a message → agent still has prior + context, `zed_thread_id` unchanged, no new empty thread forked. +2. Change **credential type** (api_key ⇄ subscription) → same: context preserved. +3. Change **agent kind** (claude_code ⇄ zed-agent) → thread MAY reset (that's + allowed) — verify it comes up cleanly. +4. Regression: a genuinely **wedged** thread (kill the ACP agent mid-turn) still + resets and recovers. + +Report the exact clear-path you found, the gate you added, and paste the live +test output (the `last_zed_message_id` climbing on the SAME thread across a +model switch is the key evidence). Do NOT claim "covered by unit tests" — unit +tests that assert the field value are not evidence the conversation survived. + +Related design docs: `design/2026-07-20-restart-clears-zed-thread-context-loss.md` +(the #2860 writeup — mirror its approach). diff --git a/design/2026-07-21-brief-subscription-owner-ux.md b/design/2026-07-21-brief-subscription-owner-ux.md new file mode 100644 index 0000000000..a2cafa576a --- /dev/null +++ b/design/2026-07-21-brief-subscription-owner-ux.md @@ -0,0 +1,85 @@ +# Fix: Claude-subscription UX — whose sub is used, cross-user agent edits, and legible auth errors + +## Problem (real incident, meta prod, 2026-07-21) + +User A (Luke) opened User B's (Chris's) agent settings and switched the agent from +Anthropic **API key** mode to **Claude subscription** mode. Luke had a valid Claude +subscription connected to **his own** account, so he reasonably expected it to be +used. It wasn't. Every turn then failed, and the user-visible error was the +useless generic string: + +> "agent turn aborted: the ACP agent process exited mid-turn or hit max tokens +> (see Zed.log 'Error in run turn' for the cause)" + +The actual cause (only visible by SSHing into the container and reading Zed.log) +was `Error in run turn: … API Error: 401 OAuth access token is invalid. +errorKind: authentication_failed`. + +## Root cause (confirmed in code) + +`api/pkg/server/external_agent_handlers.go` `subscriptionEnvForSession` resolves +the Claude token via: + +```go +sub, err := apiServer.Store.GetEffectiveClaudeSubscription(ctx, session.Owner, session.OrganizationID) +``` + +i.e. it uses the **session owner's** subscription (user-level first, then +org-level fallback), NOT the editing user's, and NOT the agent-app owner's +necessarily. So when Luke flips Chris's agent to subscription mode, it silently +means "use **Chris's** Claude subscription". Chris's stored token was invalid → +401. There is **no UI affordance** telling the editor whose subscription will be +used, **no validation** that the resolved owner even has a working subscription, +and **no legible surfacing** of the auth failure. The token is injected as +`CLAUDE_CODE_OAUTH_TOKEN` at desktop-start, so failures only appear at first turn. + +## Scope of this task — three improvements + +### 1. Make "whose subscription" explicit in the agent-settings UI +When a user selects **subscription** credential mode for an agent/assistant, the +UI must state, in plain language, **which account's Claude subscription will +authenticate the agent** — the session owner's — and that it is NOT the editing +user's. E.g. a callout: *"Sessions from this agent authenticate with the session +owner's connected Claude subscription. If someone else runs this agent, their own +subscription is used — not yours."* If editing another user's agent, name that +owner and whether they currently have an active subscription connected. + +### 2. Validate at save time (and/or at session start) +When subscription mode is selected/saved, check that the account whose sub will be +used has an **active, non-expired, actually-valid** Claude subscription. If not, +**block or warn clearly**: *" has no working Claude subscription connected — +the agent will fail to authenticate. Connect one, or use API-key mode."* A cheap +liveness probe: call `https://api.anthropic.com/v1/messages` with +`Authorization: Bearer ` + `anthropic-beta: oauth-2025-04-20`; **401 = +invalid**, 429/200 = accepted (429 is just a throttle, still "valid"). Consider +recording `last_error` / `last_validated_at` on the `claude_subscriptions` row and +showing it in settings. + +### 3. Surface the real auth error to the user +Propagate the underlying ACP failure reason instead of the generic +"agent process exited". When Zed reports `errorKind: authentication_failed` / +`401 OAuth access token is invalid`, the Helix session error shown in the UI must +say something like *"Claude subscription authentication failed for (invalid +or expired token). Reconnect the subscription in Settings."* Trace where the +generic string is produced (search `agent turn aborted` / `exited mid-turn or hit +max tokens` in `api/pkg/server/` and the Zed +`external_websocket_sync` `chat_response_error` emission) and pass the specific +`authentication_failed` reason through. + +## Optional (call out, don't necessarily build): let the agent specify a sub owner +Today the sub is strictly `session.Owner`. Consider whether an agent config should +be able to pin an explicit subscription (e.g. an **org-level** shared Claude +subscription) so cross-user agents don't silently depend on who runs them. If you +add this, keep it minimal and data-driven. Get review before expanding scope. + +## Acceptance criteria +Test in the inner Helix browser end-to-end: +1. As user A, edit user B's agent → switch to subscription mode → the UI clearly + shows B's subscription is what's used, and warns if B has none/invalid. +2. Trigger an auth failure (owner with an invalid token) → the **session error in + the UI** names it as a subscription-auth failure, not "process exited". +3. Happy path still works (owner with a valid token authenticates). + +Do not report "done" from unit tests alone — show the actual UI states +(screenshots/DOM) for the warning and the legible error. Related incident notes: +the session-owner resolution and recovery are documented in the 2026-07-21 memory. diff --git a/design/2026-07-29-acp-agent-silent-prompt-wedge.md b/design/2026-07-29-acp-agent-silent-prompt-wedge.md index ec026bb86b..9bed11f9c0 100644 --- a/design/2026-07-29-acp-agent-silent-prompt-wedge.md +++ b/design/2026-07-29-acp-agent-silent-prompt-wedge.md @@ -134,12 +134,17 @@ future merge conflict surface) at zero. *every* `AcpThreadEvent` (thinking, text, tool call, entry update, Stopped). Any event is proof of life. - `wait_for_first_agent_activity()` — races a freshly dispatched prompt against a - budget (`HELIX_ACP_FIRST_EVENT_TIMEOUT_SECS`, default **120s**, `0` disables). -- If the agent emits **anything**, the watchdog disarms for the rest of the turn, so - arbitrarily long tool calls and slow generations are never interrupted. This is the - key design point: a blanket turn timeout would be wrong (the documented - long-single-tool-call false positive), but "zero events since dispatch" is - unambiguous — a healthy agent emits its first event within seconds. + budget (`HELIX_ACP_SILENCE_TIMEOUT_SECS`, default **120s**, `0` disables). +- A blanket turn timeout would be wrong (the documented long-single-tool-call false + positive), so busy-ness is decided by **thread state, not a clock**: + `has_outstanding_work()` reports true while any tool call is `Pending` / + `InProgress` / `WaitingForConfirmation`. A running tool or a pending permission + prompt is exempt for exactly as long as the work genuinely takes — there is no + "longest plausible tool call" constant anywhere, because that number is both + unknowable and wrong the first time someone runs a 40-minute build. +- The budget therefore only has to cover model think-time between one event and the + next (normally seconds), which is what makes a single modest 120s default + defensible. - On expiry the send task is dropped (same rationale as Critical Fix #8: never block on a non-responding agent) and an error carrying `helix_silent_prompt_wedge` is returned. @@ -170,23 +175,47 @@ disablable. `waiting` interaction with no agent events for N minutes on a live connection as a wedge, so Restart can reset a genuinely poisoned thread instead of preserving it. - Not verified end-to-end against a live wedge (not reproducible on demand). -- **The watchdog does not cover "emitted output, then went silent before completing".** - It is a time-to-FIRST-event watchdog by design (that shape is unambiguous and cannot - false-positive on a long tool call). A turn that streams some tokens and *then* stalls - before `Stopped` disarms the watchdog and is still unbounded. The E2E claude round was - observed failing in exactly that shape on 2026-07-29 (three events including a correct - assistant answer, then no `message_completed`), so it is a real behaviour, not - hypothetical. Catching it needs a separate idle-since-last-event budget, sized well - above the longest plausible tool call. -- **The E2E harness cannot attribute a claude-round failure.** `e2e-test/run_e2e.sh:204` - reports the agent version with - `npm view @anthropic-ai/claude-agent-acp version`, but the package Zed actually - installs is **`@agentclientprotocol/claude-agent-acp`** (confirmed in +- ~~The watchdog does not cover "emitted output, then went silent before completing".~~ + **FIXED** (`e36d6f47ee`). Originally the watchdog disarmed permanently on the first + event, so a turn that streamed tokens and *then* stalled before `Stopped` stayed + unbounded — observed in the E2E claude round on 2026-07-29 (three events including a + correct assistant answer, then no `message_completed`). Rather than add a second idle + timer sized to "the longest plausible tool call" (unknowable, and wrong the first time + someone runs a 40-minute build), busy-ness is now read from thread state via + `has_outstanding_work()`. Both shapes now fall out of one rule: + *generating + no events for `budget` + nothing outstanding ⇒ wedged.* +- ~~The E2E harness cannot attribute a claude-round failure.~~ **FIXED** + (`e36d6f47ee`). `run_e2e.sh` queried `npm view @anthropic-ai/claude-agent-acp`, which + **404s**; Zed installs `@agentclientprotocol/claude-agent-acp` (confirmed in `crates/agent_servers/`, and matching the `ps` output of the wedged production - container). The wrong scope means the query always returns `unknown`, so the log line - that exists precisely to distinguish "our regression" from "the agent package changed - under us" is silently useless. The package is also unpinned in the npm path, so the - claude round is not reproducible across time. Worth fixing both. + container — today it resolves to **0.63.0**, the very version that wedged). The wrong + scope silently returned `unknown`, disabling the one signal that distinguishes "our + regression" from "the agent package changed under us". Now queries the correct scope, + warns loudly if it cannot resolve, and states explicitly that the install is unpinned. + +### Decision: do NOT pin `claude-agent-acp` in the E2E + +Tempting, because an unpinned install makes the claude round non-reproducible across +time. Rejected deliberately: + +- **Production is unpinned too.** Zed auto-installs the latest + `@agentclientprotocol/claude-agent-acp` in every desktop container. Pinning CI would + make CI test something we do not ship. +- **Tracking latest is essential, not incidental.** The Anthropic API and Claude Code + move fast, and keeping up with them is the point of this integration. Freezing the + agent would mean discovering breakage in production instead of in CI. +- Consequently, an agent-package regression breaking the claude round is a **true + positive**, not noise. Pinning would convert a real signal into silence. + +The correct remedy is therefore *attribution and resilience*, not determinism: +1. Always log the resolved agent version (fixed above) so a failure can be attributed. +2. Make the system tolerate a misbehaving agent rather than assume a well-behaved one — + which is exactly what the silence watchdog and Critical Fix #8 (`cancel()` drops + `send_task`) do. + +**Practical consequence for flake triage:** a claude-round failure is not automatically +"our bug". Check the logged agent version first, and prefer hardening Zed against the +misbehaviour over chasing a deterministic repro that may not exist. - The `model_not_found` trigger has been fixed on the serving side (model availability in GCP), so the specific path into this wedge is closed. The wedge handling still matters: any future provider-side error can re-enter it. diff --git a/sandbox-versions.txt b/sandbox-versions.txt index 27eb869a28..e2823a900f 100644 --- a/sandbox-versions.txt +++ b/sandbox-versions.txt @@ -1,3 +1,3 @@ -ZED_COMMIT=d1dffb81161ce79abf76206df03bd44b0e90e3cc +ZED_COMMIT=473a1c7cc2c5fec2f8657497b060f7c0ec606bd5 QWEN_COMMIT=36fae6014a1e520c8e5c3aa0d50cd1a72319457e GOOSE_COMMIT=ca26f01d3acd9871691fa8981f05d19aed9a3b82