Skip to content

fix(claude-cli): preserve reused session on empty response#99139

Open
wyf027 wants to merge 3 commits into
openclaw:mainfrom
wyf027:fix/claude-cli-empty-response-keeps-session-99131
Open

fix(claude-cli): preserve reused session on empty response#99139
wyf027 wants to merge 3 commits into
openclaw:mainfrom
wyf027:fix/claude-cli-empty-response-keeps-session-99131

Conversation

@wyf027

@wyf027 wyf027 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #99131.

A reused Claude CLI session can produce an empty-response failover when the CLI emits a synthetic no-response result before later transcript activity. The attempt layer currently treats every FailoverError from a reused Claude CLI turn as a poisoned session and clears the stored CLI session binding, which turns the empty response into a destructive session discard.

Why This Change Was Made

This keeps the existing cleanup behavior for aborts and hard failover reasons such as auth, billing, and rate limits, but stops clearing reused Claude CLI session IDs for empty_response. That preserves the session binding for a later resume instead of orphaning the same transcript after a transient/synthetic empty result.

User Impact

Claude CLI users who hit the synthetic empty-response path no longer lose the reusable session binding as a side effect of the failed turn, reducing the background-task/session discard cascade described in the issue.

Evidence

  • node scripts/run-vitest.mjs src/agents/command/attempt-execution.cli.test.ts -- -t "keeps reused Claude CLI session IDs after empty response failover"
  • node scripts/run-vitest.mjs src/agents/command/attempt-execution.cli.test.ts
  • git diff --check

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jul 2, 2026
@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 6:12 PM ET / 22:12 UTC.

Summary
The PR retains persisted reused Claude CLI session identifiers after empty_response failovers and adds a mocked persistence regression test.

PR surface: Source 0, Tests +42. Total +42 across 2 files.

Reproducibility: yes. at source level. The cleanup path and generic empty-result classification are explicit on current main, and the linked issue supplies repeated runtime logs for the synthetic sequence; this read-only review did not execute the external Claude account scenario.

Review metrics: 1 noteworthy metric.

  • Session cleanup semantics: 1 generic failover reason exempted. The same reason covers the reported placeholder, ordinary whitespace output, and quiet exit-zero failures.

Stored data model
Persistent data-model change detected: serialized state: src/agents/command/attempt-execution.cli.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #99131
Summary: This PR is a partial candidate fix for the canonical issue's persisted-binding consequence; typed placeholder classification and live-process teardown remain interacting but distinct surfaces.

Members:

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦪 silver shellfish
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:

  • [P1] Narrow retention to a typed synthetic-placeholder outcome and add negative coverage for whitespace-only and quiet exit-zero failures.
  • [P1] Add redacted live Claude CLI terminal output or runtime logs showing successful later resume or answer delivery, with private identifiers and endpoints removed.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR supplies a mocked persistence regression but no redacted after-fix Claude CLI run showing that the retained session later resumes successfully or delivers the answer; after adding proof, update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging the generic exception can retain a genuinely poisoned Claude CLI session after an unrelated whitespace-only or quiet exit-zero completion, causing later turns to resume the same failing state.
  • [P1] Retaining the persisted identifier does not itself prove that the live process remains available or that later transcript activity will be read and delivered, so the reported user-visible recovery remains unproven.

Maintainer options:

  1. Narrow the exception (recommended)
    Propagate a typed synthetic-placeholder outcome to the cleanup decision, retain generic empty-response invalidation, and add negative coverage for quiet and whitespace-only failures.
  2. Accept generic reuse risk
    Merge the broad exception only if maintainers intentionally accept that unrelated empty-output failures may repeatedly reuse a failed Claude CLI session.
  3. Defer to typed handling
    Pause or close this branch if the overlapping typed synthetic-placeholder implementation becomes the canonical viable landing path.

Next step before merge

  • [P1] The contributor must provide real-environment recovery proof, and the required correction spans typed parser/runtime classification while an overlapping open PR already owns that surface.

Security
Cleared: The diff changes session cleanup logic and focused tests only, with no new dependency, secret, permission, downloaded code, or supply-chain surface.

Review findings

  • [P1] Preserve only typed synthetic-placeholder sessions — src/agents/command/attempt-execution.ts:88
Review details

Best possible solution:

Carry a typed synthetic-placeholder outcome through the Claude CLI parser and result path, preserve the reused binding only for that observed state, retain generic empty-response invalidation, and prove with redacted live output that the later answer is delivered.

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

Yes at source level. The cleanup path and generic empty-result classification are explicit on current main, and the linked issue supplies repeated runtime logs for the synthetic sequence; this read-only review did not execute the external Claude account scenario.

Is this the best way to solve the issue?

No. The patch exempts a generic failure reason used by unrelated empty-output states and does not prove that preserving the identifier recovers the later user-visible answer; typed classification is the safer boundary.

Full review comments:

  • [P1] Preserve only typed synthetic-placeholder sessions — src/agents/command/attempt-execution.ts:88
    empty_response also represents ordinary whitespace output and quiet exit-zero failures, so it does not identify the reported synthetic-placeholder sequence. Exempting the whole reason undoes poisoned-session cleanup for those unrelated failures and can repeatedly resume a wedged session. Propagate a typed placeholder outcome into this decision and keep generic cleanup; this prior blocker remains unchanged on the current head.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2a479bd4ac85.

Label changes

Label justifications:

  • P1: Incorrect Claude CLI session cleanup or reuse can lose active work and repeatedly break a real agent workflow.
  • merge-risk: 🚨 session-state: The PR changes whether persisted Claude CLI session identifiers survive failed reused turns.
  • merge-risk: 🚨 availability: Retaining a genuinely poisoned session can make subsequent turns repeatedly fail or stall.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR supplies a mocked persistence regression but no redacted after-fix Claude CLI run showing that the retained session later resumes successfully or delivers the answer; after adding proof, update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source 0, Tests +42. Total +42 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 1 0
Tests 1 42 0 +42
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 43 1 +42

What I checked:

Likely related people:

  • giodl73-repo: Authored and merged the failed reused-session invalidation behavior directly changed by this PR, including live proof that failed bindings were cleared. (role: introduced cleanup behavior; confidence: high; commits: 5817e478d1f8; files: src/agents/command/attempt-execution.ts, src/agents/command/attempt-execution.cli.test.ts)
  • steipete: Co-authored and merged the recent Claude CLI empty-result parser and live-session work and has the strongest accumulated history across the central agent files. (role: recent adjacent owner and merger; confidence: high; commits: 3c2316c43cd2; files: src/agents/cli-output.ts, src/agents/cli-runner.ts, src/agents/cli-runner/claude-live-session.ts)
  • totobusnello: Introduced the neighboring streamed-assistant-text preservation behavior and supplied focused real-runtime evidence for Claude CLI empty terminal results. (role: recent empty-result contributor; confidence: medium; commits: 3c2316c43cd2; files: src/agents/cli-output.ts, src/agents/cli-output.test.ts, src/agents/cli-runner/claude-live-session.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.
Review history (3 earlier review cycles)
  • reviewed 2026-07-02T15:45:43.014Z sha 4bc04db :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-11T21:51:57.091Z sha eb6dcb0 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-11T22:03:09.968Z sha 18f4d36 :: needs real behavior proof before merge. :: [P1] Preserve only typed synthetic-placeholder sessions

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jul 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Merged
@jwest75674 jwest75674 mentioned this pull request Jul 9, 2026
Closed
wyf027 added 2 commits July 12, 2026 05:41
@wyf027
Merge remote-tracking branch 'upstream/main' into fix/claude-cli-empt…
eb6dcb0 
…y-response-keeps-session-99131

# Conflicts:
#	src/agents/command/attempt-execution.cli.test.ts
@wyf027
test: format claude cli empty response regression
18f4d36
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Jul 11, 2026
@obviyus obviyus mentioned this pull request Jul 13, 2026
Closed
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: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS 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.

[Bug]: Claude CLI reused session emits synthetic empty result before real response, causing OpenClaw session discard and background task cascade

1 participant

@wyf027