Skip to content

fix: reset web-chat worktree status from owned claude -p run - #273

Merged
centdix merged 1 commit into
mainfrom
fix-reconnect-idle
Jun 19, 2026
Merged

fix: reset web-chat worktree status from owned claude -p run#273
centdix merged 1 commit into
mainfrom
fix-reconnect-idle

Conversation

@centdix

@centdix centdix commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

The Claude streaming PR (#262) added a busy-gate to the web chat: it rejects a message with "Claude is already running in the terminal. Wait for it to finish…" whenever worktree.status is running/starting. That status is derived from the worktree's agent.lifecycle, which is driven only by best-effort, async fire-and-forget hooks (UserPromptSubmit/PostToolUse → running, Stop → stopped). Those POSTs get lost/reordered, so the lifecycle sticks at running and the gate then blocks web chat even though nothing is running.

Repro (web→web): send a first web-chat message → the owned claude -p run fires UserPromptSubmit (running); the turn ends but the Stop hook's POST never lands → status stuck running → the next web-chat message is blocked.

The backend already owns the claude -p subprocess, so it has a reliable end-of-turn signal (the stream result line / process exit). This PR uses that owned signal to drive the worktree lifecycle directly instead of trusting the lossy hook.

Changes

  • claude-conversation-stream-service.ts: add an onRunSettled? callback to the run input; it fires exactly once from finishRun (covers normal completion, error, and interrupt — all funnel through finishRun, which already early-returns on completed).
  • server.ts: add setWorktreeAgentLifecycle(worktree, "running" | "stopped") (guarded getWorktreeByBranchapplyEvent, mirroring setAgentTerminalStale). In sendClaudeStreamingMessage, set running when the owned run starts and pass onRunSettled to set stopped when it settles. stopped maps to the UI done state — the same state a terminal turn finishing produces — not idle/waiting.
  • Tests: onRunSettled contract (fires once on complete/error/interrupt) + an integration repro proving the worktree flips runningstopped on completion so the next web message passes the gate.

Test plan

  • bun test (backend) — 461 pass / 0 fail; 3 new tests added (failed first, pass after the fix)
  • bun run check (backend tsc --noEmit) — exit 0
  • Manual: in the web UI, send a web-chat message to a Claude worktree, wait for it to finish, then send a second one — it should go through (status shows done, not working/waiting)

Known limitation / follow-up

The web-chat claude -p subprocess still runs the worktree's .claude hooks. For a turn that ends on a tool call (no trailing text), a late async PostToolUse → running POST can land after our onComplete → stopped and re-stick running. Text-ending turns (the common case) are unaffected. Full robustness = suppress the status hooks for owned claude -p runs (keep PR-detection) so the owned signal is the sole driver. Tracked as a follow-up.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 19, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
webmux 02951db Commit Preview URL

Branch Preview URL
Jun 19 2026, 11:01 PM

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 1m 46s —— View job


Code Review: PR #273 — reset web-chat worktree status from owned claude -p run

  • Read the diff against origin/main
  • Review claude-conversation-stream-service.ts
  • Review server.ts
  • Review tests
  • Post review feedback

Solid fix. The core insight is right: the backend owns the claude -p subprocess, so it has an authoritative end-of-turn signal and shouldn't depend on the lossy fire-and-forget hooks to clear the busy gate. The implementation is minimal and routes every settle path through a single chokepoint.

What's good

  • Single firing point. onRunSettled?.() lives in finishRun (claude-conversation-stream-service.ts:259), which guards on run.completed at the top (:245). Every terminal path — onComplete, onErrorfailRun, interrupt, and the handle.completion.finally fallback — funnels through finishRun, so the callback fires exactly once regardless of how the run ends. The "fires once / on error / on interrupt" tests correctly cover this contract.
  • Correct target state. Mapping to stopped (→ UI done) rather than idle/waiting matches what a finished terminal turn produces, so web and terminal converge on the same end state.
  • Guarded mutation. setWorktreeAgentLifecycle (server.ts:714) early-returns when getWorktreeByBranch misses, mirroring setAgentTerminalStale, and agent_status_changed + "stopped" are valid per domain/events.ts:19 and applyStatusChanged (project-runtime.ts:226). Wiring is consistent with the existing event model.
  • Honest known-limitation section — the late PostToolUse → running re-stick on tool-ending turns is real and correctly called out as a follow-up.

Minor notes (non-blocking)

  1. Start/settle ordering is safe, but worth a comment. In sendClaudeStreamingMessage, onRunSettled (→ stopped) is registered before the explicit setWorktreeAgentLifecycle(input.worktree, "running") at server.ts:803. This is fine because the run's completion callbacks are async (subprocess onComplete/completion.finally can't fire synchronously within startRun), so the synchronous running line always wins the initial ordering. It's a subtle invariant though — if a future change ever made startRun able to settle synchronously, you'd get a stuck running. A one-line comment noting "running must be set after startRun returns; settle is always async" would protect this.

  2. isBusy duplicated in the integration test. claude-streaming-lifecycle.test.ts:28 re-implements isBusyAgentStatus (server.ts:761) as a local const isBusy = ... rather than importing the real one, so the two can drift (e.g. if starting/running/idle semantics change, the test won't notice). Similarly setLifecycle re-implements setWorktreeAgentLifecycle's body. The test therefore verifies the concept of the wiring, not the actual server.ts functions. That's an understandable consequence of server.ts being the I/O boundary with a module-global projectRuntime (hard to unit-test per backend/CLAUDE.md). If isBusyAgentStatus can be exported without triggering server side effects on import, importing it here would remove the drift risk. Low priority.

  3. Terminal-stale flag isn't reset on settle. setAgentTerminalStale(input.worktree, true) is set on start but onRunSettled only resets the lifecycle, not the stale flag. This looks intentional (stale tracks that web chat wrote into the session, independent of run lifecycle) — flagging only to confirm it's deliberate and not an oversight.

Verdict

The fix is correct, minimal, and the tests pin the important contract (settle fires once; running → stopped reopens the gate). The remaining gap (tool-ending turns) is acknowledged and follow-up-tracked. 👍
fix-reconnect-idle

@centdix
centdix marked this pull request as ready for review June 19, 2026 23:06
@centdix
centdix merged commit 5753794 into main Jun 19, 2026
3 checks passed
@centdix
centdix deleted the fix-reconnect-idle branch June 19, 2026 23:07
@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 2m 0s —— View job


Code Review: PR #273 — reset web-chat worktree status from owned claude -p run

  • Read the diff against origin/main
  • Review claude-conversation-stream-service.ts
  • Review server.ts
  • Review tests
  • Post review feedback

The diagnosis is correct and the fix is well-targeted: the backend owns the claude -p subprocess, so its stream result line / process exit is an authoritative end-of-turn signal that doesn't depend on the lossy fire-and-forget Stop hook. Routing every terminal path through one chokepoint to clear the busy gate is the right shape.

Verified correct

  • Single firing point. I traced every settle path: onComplete (:98), onError → failRun (:101, :270), interrupt (:135), and the handle.completion.finally fallback (:111) all funnel through finishRun, which guards on run.completed at the top (:245) before setting it true (:246). So run.onRunSettled?.() at :259 fires exactly once regardless of how the run ends. The duplicate-completion test (:316-318) pins this.
  • Correct target state. stopped maps to UI done — same end state a finished terminal turn produces (applyStatusChanged, project-runtime.ts:226), so web and terminal converge. "stopped" is a valid agent_status_changed lifecycle.
  • Guarded mutation. setWorktreeAgentLifecycle (server.ts:714) early-returns on a getWorktreeByBranch miss, mirroring setAgentTerminalStale. Consistent with the event model.
  • Honest known-limitation. The late PostToolUse → running re-stick on tool-ending turns is real and correctly tracked as follow-up.

Minor notes (non-blocking; PR already merged — for follow-up)

  1. Start/settle ordering is safe but undocumented. onRunSettled (→ stopped) is registered at server.ts:797 before the synchronous setWorktreeAgentLifecycle(..., "running") at :803. This works only because the run's completion callbacks are async — the real claude-cli adapter spawns a subprocess, so onComplete / completion.finally can't fire within startRun, and the synchronous running always wins. It's a subtle invariant: if a future change ever let startRun settle synchronously, you'd get a stuck running. A one-line comment ("running set after startRun; settle is always async") would protect it. Fix this →

  2. isBusy/setLifecycle duplicated in the integration test. claude-streaming-lifecycle.test.ts:28 re-implements isBusyAgentStatus (server.ts:761) and :39 re-implements setWorktreeAgentLifecycle's body, so the test verifies the concept of the wiring rather than the real server.ts functions — they can drift if starting/running semantics change. Understandable given server.ts is the I/O boundary with a module-global projectRuntime; if isBusyAgentStatus can be exported without import side effects, importing it here removes the drift risk. Low priority.

  3. stopped-on-failure clears lastError. When a run fails, failRun → finishRun → onRunSettled → setWorktreeAgentLifecycle(..., "stopped"), and applyStatusChanged (project-runtime.ts:231) unconditionally sets state.agent.lastError = null. So the worktree badge shows done, not error, after a failed web-chat turn. This is probably acceptable — the error still reaches the conversation UI via failRun's subscriber.sendError (:271-272), and clearing the gate is the goal — but worth confirming it's intended rather than an accidental masking of error state on the worktree.

  4. Terminal-stale flag not reset on settle. setAgentTerminalStale(..., true) is set on start (:804) but onRunSettled only resets the lifecycle, not the stale flag. Looks deliberate (stale tracks that web chat wrote into the session, independent of run lifecycle) — flagging only to confirm.

Verdict

Correct, minimal, and the tests pin the contract that matters (settle fires once; running → stopped reopens the gate). The remaining gap (tool-ending turns) is acknowledged and tracked. 👍
fix-reconnect-idle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant