Skip to content

fix: stop Claude tool heartbeats from hiding stuck sessions#96198

Closed
harjothkhara wants to merge 3 commits into
openclaw:mainfrom
harjothkhara:codex/96168-claude-live-heartbeat-progress
Closed

fix: stop Claude tool heartbeats from hiding stuck sessions#96198
harjothkhara wants to merge 3 commits into
openclaw:mainfrom
harjothkhara:codex/96168-claude-live-heartbeat-progress

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #96168

What Problem This Solves

Fixes an issue where Claude CLI live-session runs could keep a wedged native tool looking fresh when the backend emitted active-tool heartbeat progress. If the CLI emitted tool_use but never emitted the matching tool_result, OpenClaw's diagnostic stuck-session detection could see recent progress and delay surfacing or recovering the blocked work.

Why This Change Was Made

cli_live:tool_running is now treated as passive diagnostic progress: the heartbeat event is still emitted for observability, but it no longer creates or refreshes diagnostic session activity. Real progress, including tool start/result and run result events, still updates the activity clock.

This keeps the fix at the diagnostic activity boundary, avoids changing Claude CLI runtime behavior, and does not add config or a new timeout policy. Blast radius is limited to diagnostic run-activity bookkeeping and the existing Claude live-session regression harness.

User Impact

Wedged Claude CLI live-session turns with an active tool that never returns a tool_result can age normally in stuck-session detection instead of being kept fresh by a synthetic heartbeat.

Because the synthetic heartbeat no longer masks tool age, Claude live native-tool calls now follow the same stall/abort policy already applied to model calls and embedded runs: a tool with no real intermediate progress is surfaced as a stalled (blocked_tool_call) session past the warn threshold (default 2 min) and becomes auto-recovery/abort-eligible past the abort threshold (default 6 min, configurable via diagnostics.stuckSessionAbortMs). Previously such tools were shielded from stall detection indefinitely. Tools that stream real intermediate output keep refreshing the activity clock exactly as before, so normal long-running work is unaffected.

Evidence

AI-assisted disclosure: This PR was prepared with Codex assistance; I reviewed the change and validation.

Behavior addressed
Claude live active-tool heartbeat events still emit run.progress, but no longer refresh diagnostic session "last real progress" state.

Real environment tested
Local OpenClaw checkout on current origin/main SHA c24d266b2d0943a2376ae9ac87a2fd8e200920e1, using the production Claude live runner path up to the subprocess boundary, diagnostic event bus, timer heartbeat, and session activity snapshot.

Exact steps or command run after this patch
node scripts/run-vitest.mjs src/agents/cli-runner.spawn.test.ts -t "reports Claude live stream progress without treating active tool heartbeats as progress"

Evidence after fix
Focused run passed: Test Files 2 passed (2), Tests 2 passed | 118 skipped (120).

Supplemental validation:

  • node scripts/run-vitest.mjs src/logging/diagnostic-session-attention.test.ts - passed, 1 file, 11 tests
  • node scripts/run-vitest.mjs src/logging/diagnostic.test.ts -t "does not refresh progress age for Claude live active-tool heartbeats" - passed, 1 test | 72 skipped
  • node scripts/run-oxlint.mjs src/logging/diagnostic-run-activity.ts src/agents/cli-runner.spawn.test.ts - passed
  • git diff --check - passed
  • .agents/skills/autoreview/scripts/autoreview --mode local - clean, no accepted/actionable findings reported

Observed result after fix
After a simulated Claude tool_use starts an active tool, the 10s heartbeat still produces run.progress, but lastProgressReason remains cli_live:tool_started and lastProgressAgeMs >= 10000. The heartbeat no longer masks a wedged tool as fresh progress.

What was not tested
No live Claude account/API call or actual Claude CLI binary invocation. The proof uses the repo's deterministic live-runner harness because the bug boundary is the JSONL stream, heartbeat timer, and diagnostic activity bookkeeping.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jun 23, 2026
@harjothkhara
harjothkhara marked this pull request as ready for review June 23, 2026 21:21
@harjothkhara

harjothkhara commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a small follow-up (93054bc5a8) and clarified the PR body.

Code — refactor(diagnostic): early-return passive heartbeat progress: markDiagnosticRunProgress now early-returns for the passive cli_live:tool_running reason before calling resolveSessionActivity, instead of passing create: false and bailing afterward. Behavior-identical, but it also avoids the passive heartbeat needlessly running resolveSessionActivity's ref-registration/merge side effect. Revalidated locally:

  • diagnostic.test.ts — 73 passed (incl. the new heartbeat test)
  • diagnostic-session-attention.test.ts — 11 passed
  • cli-runner.spawn.test.ts -t "reports Claude live stream progress without treating active tool heartbeats as progress" — 2 passed | 118 skipped

Body clarification (User Impact): Made explicit that dropping the heartbeat refresh re-enables the existing blocked_tool_call stall path for Claude live native tools — they now follow the same warn (default 2 min) / auto-abort (default 6 min, configurable via diagnostics.stuckSessionAbortMs) policy already applied to model calls and embedded runs, where the heartbeat previously shielded them indefinitely. Tools that stream real intermediate output keep refreshing the activity clock, so normal long-running work is unaffected.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 24, 2026
Closed
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 6:27 PM ET / 22:27 UTC.

Summary
The PR makes Claude live active-tool heartbeat progress (cli_live:tool_running) observability-only for diagnostic activity and updates regression tests around the heartbeat snapshot.

PR surface: Source +10, Tests +23. Total +33 across 3 files.

Reproducibility: yes. at source level: current main leaves Claude live activeTools populated until a matching tool_result, emits cli_live:tool_running, and diagnostic activity currently records that as fresh progress. I did not run a live Claude CLI reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Progress Semantics Changed: 1 run.progress reason demoted. cli_live:tool_running changes from a freshness signal into an observability-only tick, which directly affects stuck-session recovery behavior before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96168
Summary: This PR is a candidate fix for the linked Claude live active-tool heartbeat wedge, but it partially overlaps the previously merged heartbeat protection for valid long-running tools.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Rework the heartbeat handling so valid quiet Claude-native tools remain protected while truly wedged tools can age and recover.
  • [P1] Add redacted live Claude CLI or packaged-runtime proof for both the wedged-tool case and the valid quiet-tool boundary.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports focused tests, lint, and a deterministic live-runner harness, but explicitly no actual Claude CLI binary or live turn; contributor should add redacted terminal/log proof and update the PR body for a fresh review.

Risk before merge

  • [P1] Merging as-is can make legitimate quiet Claude-native tools stale and eligible for stuck-session recovery or abort after the configured threshold, reversing the shipped heartbeat behavior from Fix Claude live tool progress for watchdog recovery #87546.
  • [P1] The PR still lacks live Claude CLI or packaged-runtime proof showing both the wedged-tool fix and the valid quiet-tool false-positive boundary.

Maintainer options:

  1. Preserve Both Heartbeat Invariants (recommended)
    Fix before merge by narrowing stale-tool detection so wedged active tools can age while valid quiet Claude-native tools still satisfy the shipped heartbeat contract.
  2. Accept Earlier Quiet-Tool Recovery
    Maintainers could intentionally accept that quiet Claude-native tools may become abort-eligible after the stuck-session threshold, but that needs explicit policy approval and proof.
  3. Pause For A Narrow Replacement
    If this branch cannot reconcile the two diagnostic invariants, pause or close it and request a smaller replacement with live proof for both scenarios.

Next step before merge

  • [P1] A maintainer needs to choose the diagnostic policy and require proof for both the wedged-tool and valid quiet-tool cases; this is not a safe automated repair from the current branch.

Security
Cleared: The diff only changes diagnostic activity logic and focused tests; I found no concrete security or supply-chain concern.

Review findings

  • [P1] Preserve heartbeat freshness for valid Claude tools — src/logging/diagnostic-run-activity.ts:275
Review details

Best possible solution:

Rework the fix so wedged Claude tools can age and recover without removing heartbeat freshness for valid quiet foreground tools, then prove both cases with redacted live Claude CLI or packaged-runtime logs.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main leaves Claude live activeTools populated until a matching tool_result, emits cli_live:tool_running, and diagnostic activity currently records that as fresh progress. I did not run a live Claude CLI reproduction in this read-only review.

Is this the best way to solve the issue?

No. The branch is a plausible diagnostic-boundary fix for the wedge, but it is too broad because it disables the shipped heartbeat freshness path that protects valid long-running Claude-native tools.

Full review comments:

  • [P1] Preserve heartbeat freshness for valid Claude tools — src/logging/diagnostic-run-activity.ts:275
    This makes every cli_live:tool_running heartbeat passive. That also reverses the shipped behavior from Fix Claude live tool progress for watchdog recovery #87546, where the heartbeat keeps valid silent foreground Claude-native tools from being treated as stale; with the existing blocked_tool_call recovery gate, those tools can now become abortable once activeToolAgeMs and lastProgressAgeMs cross the stuck-session threshold. Please narrow the wedged-tool fix so the earlier long-running-tool protection remains covered.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3811001d2783.

Label changes

Label justifications:

  • P1: The linked bug affects real Claude live-session recovery, and the proposed fix can regress an existing agent watchdog path.
  • merge-risk: 🚨 compatibility: The diff changes shipped Claude live heartbeat semantics that valid long-running tool sessions rely on to avoid watchdog recovery.
  • merge-risk: 🚨 availability: If merged as-is, valid quiet Claude CLI tool calls can be classified as stale and aborted after the stuck-session threshold.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused tests, lint, and a deterministic live-runner harness, but explicitly no actual Claude CLI binary or live turn; contributor should add redacted terminal/log proof and update the PR body for a fresh review.
Evidence reviewed

PR surface:

Source +10, Tests +23. Total +33 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 10 0 +10
Tests 2 32 9 +23
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 42 9 +33

What I checked:

Likely related people:

  • joshavant: Authored and merged the prior Claude live watchdog-progress repair that added active-tool heartbeat freshness and its regression coverage. (role: introduced shipped heartbeat behavior; confidence: high; commits: 980a643df26b, 5b73eeb9cdcf, 4c3a0292ff9e; files: src/agents/cli-runner/claude-live-session.ts, src/agents/cli-runner.spawn.test.ts)
  • Agustin Rivera: Current shallow blame on the central Claude live and diagnostic files resolves to the latest main boundary commit, so this is a weak routing signal rather than feature authorship. (role: recent current-main carry-forward signal; confidence: low; commits: 3811001d2783; files: src/agents/cli-runner/claude-live-session.ts, src/logging/diagnostic-run-activity.ts, src/logging/diagnostic.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 24, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jul 1, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Closed
@yetval yetval mentioned this pull request Jul 10, 2026
Merged
@harjothkhara

harjothkhara commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Closing — #102160 landed evidence-based liveness and lists #96168 among the issues it fixes, so this is covered on main now.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

claude-cli backend: active-tool heartbeat masks wedged subprocesses, defeats stuck-session detection

1 participant

@harjothkhara