From 8139163121a1d7d0204c6bc64f06c126c192a524 Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Thu, 30 Jul 2026 08:16:52 +0100 Subject: [PATCH 1/3] build(sandbox): pin state-based ACP silence watchdog Bumps ZED_COMMIT to e36d6f47ee, which replaces the first-event-only silence watchdog with a state-based one, and repairs the E2E agent-version diagnostic. The original watchdog disarmed permanently on the agent's first event, so it caught the production wedge (prompt accepted, zero events) but missed the shape seen in the E2E claude round (correct answer streamed, then no message_completed). Rather than hard-code a second timeout sized to the longest plausible tool call, busy-ness is now read from thread state: a Pending/InProgress/WaitingForConfirmation tool call is exempt for as long as the work genuinely takes, and silence is only judged when nothing is outstanding. Also updates the incident write-up for both fixes. --- ...-07-07-spec-task-attachment-upload-race.md | 93 +++++++++++++ ...8-we-find-ai-custom-domain-prod-cutover.md | 123 ++++++++++++++++++ ...rief-preserve-thread-on-provider-switch.md | 82 ++++++++++++ .../2026-07-21-brief-subscription-owner-ux.md | 85 ++++++++++++ ...026-07-29-acp-agent-silent-prompt-wedge.md | 51 ++++---- sandbox-versions.txt | 2 +- 6 files changed, 413 insertions(+), 23 deletions(-) create mode 100644 design/2026-07-07-spec-task-attachment-upload-race.md create mode 100644 design/2026-07-08-we-find-ai-custom-domain-prod-cutover.md create mode 100644 design/2026-07-21-brief-preserve-thread-on-provider-switch.md create mode 100644 design/2026-07-21-brief-subscription-owner-ux.md 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..c6d1afc802 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,25 @@ 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. + **Still open:** the npm install remains unpinned, so the claude round is not + reproducible across time. - 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..b14d7af57a 100644 --- a/sandbox-versions.txt +++ b/sandbox-versions.txt @@ -1,3 +1,3 @@ -ZED_COMMIT=d1dffb81161ce79abf76206df03bd44b0e90e3cc +ZED_COMMIT=e36d6f47ee57696a97255be59d7b65efee69fe8b QWEN_COMMIT=36fae6014a1e520c8e5c3aa0d50cd1a72319457e GOOSE_COMMIT=ca26f01d3acd9871691fa8981f05d19aed9a3b82 From 8de0ae3f3e1cfc48642f58306939271aed7b6f5c Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Thu, 30 Jul 2026 08:20:20 +0100 Subject: [PATCH 2/3] docs: record decision not to pin claude-agent-acp in the E2E MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unpinned install makes the claude round non-reproducible, but pinning is the wrong fix. Production auto-installs the latest @agentclientprotocol/claude-agent-acp in every desktop container, and tracking latest is essential rather than incidental — the Anthropic API and Claude Code move fast, and keeping up with them is the point of this integration. Pinning CI would make CI test something we do not ship, and would convert a real signal (agent-package regression) into silence. The remedy is attribution plus resilience: log the resolved agent version so a failure can be attributed, and harden Zed against a misbehaving agent rather than assuming a well-behaved one. Notes the practical consequence for flake triage. --- ...026-07-29-acp-agent-silent-prompt-wedge.md | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) 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 c6d1afc802..9bed11f9c0 100644 --- a/design/2026-07-29-acp-agent-silent-prompt-wedge.md +++ b/design/2026-07-29-acp-agent-silent-prompt-wedge.md @@ -192,8 +192,30 @@ disablable. 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. - **Still open:** the npm install remains unpinned, so the claude round is not - reproducible across time. + +### 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. From 3797a39a91908694d7946a83752bac8d2f2d7ad4 Mon Sep 17 00:00:00 2001 From: Luke Marsden Date: Thu, 30 Jul 2026 11:56:20 +0100 Subject: [PATCH 3/3] build(sandbox): bump ZED_COMMIT to zed main after PR #74 Re-pins to 473a1c7cc2 (helixml/zed main, merge of PR #74) rather than the intermediate branch commit the PR was opened with. The merge commit is content-identical to the tree the E2E validated (git diff f5c77cf39a..473a1c7cc2 is empty), so the validation transfers exactly. Carries, on top of the silence watchdog already pinned by #2896: - interrupt cancels are now targeted, so a cancel racing the creation loop can no longer kill a newer turn (root cause of intermittent Phase 17 failures, and of a user pressing stop-then-retype landing in a stuck interaction) - the silence watchdog judges by thread state rather than a tool-duration guess - three E2E observability repairs: broken grep -c counters, a missing turn-lifecycle/cancel-ordering dump, and a round filter that discarded real Helix interaction ids while reporting them as 'wrong agent' --- sandbox-versions.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sandbox-versions.txt b/sandbox-versions.txt index b14d7af57a..e2823a900f 100644 --- a/sandbox-versions.txt +++ b/sandbox-versions.txt @@ -1,3 +1,3 @@ -ZED_COMMIT=e36d6f47ee57696a97255be59d7b65efee69fe8b +ZED_COMMIT=473a1c7cc2c5fec2f8657497b060f7c0ec606bd5 QWEN_COMMIT=36fae6014a1e520c8e5c3aa0d50cd1a72319457e GOOSE_COMMIT=ca26f01d3acd9871691fa8981f05d19aed9a3b82