fix(agents): clear poisoned claude cli sessions#81247
Conversation
|
/review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for a source-level path: current main can reuse a stored Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Land the scoped clear-only Claude CLI session invalidation after trimming the changelog diff and explicitly choosing it over the broader retry behavior. Do we have a high-confidence way to reproduce the issue? Yes for a source-level path: current main can reuse a stored Is this the best way to solve the issue? Yes for the scoped PR: clearing only active reused Full review comments:
Overall correctness: patch is correct Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against df23b0f86c00. |
834bb58 to
0c75041
Compare
|
/review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Applying the maintainer-proof pass from Galin's review style: this PR is mechanically narrow and CI is green, but the current proof is still seam-level/test-output proof rather than a real reused Claude CLI failure path. I converted it back to draft until we can add redacted runtime proof or a maintainer explicitly accepts the seam proof as sufficient for this failure mode. |
0c75041 to
d9125c9
Compare
|
Rebased on current origin/main, pushed to the upstream PR branch, and reran focused verification.\n\nChecks:\n- oxfmt --check changed files\n- node scripts/test-projects.mjs src/agents/command/attempt-execution.cli.test.ts --reporter=verbose\n- node scripts/check-changed.mjs\n- git diff --check\n\nStill draft pending the requested real behavior proof for the Claude CLI poisoned-session case.\n\n@clawsweeper re-review |
|
Maintainer review: not merge-ready yet, but I do not see a blocking code defect in the patch itself. This PR clears stored Findings:
Merge readiness:
Best-fix verdict:
Verification I ran against PR head git diff --check HEAD^..HEAD
pnpm exec oxfmt --check --threads=1 CHANGELOG.md src/agents/command/attempt-execution.ts src/agents/command/attempt-execution.cli.test.ts
pnpm test src/agents/command/attempt-execution.cli.test.ts src/agents/command/session-store.test.ts -- --reporter=verboseResult: targeted formatting passed; 2 test files and 49 tests passed. |
e4f1324 to
c98255b
Compare
c98255b to
18049b8
Compare
18049b8 to
e8d7231
Compare
d2aeff5 to
ade345a
Compare
ade345a to
18d8e8d
Compare
OpenClaw QA ArtifactsSummaryBefore/after Crabbox proof for PR #81247: fix(agents): clear poisoned claude cli sessions Before shows the origin/main baseline in a fresh Crabbox checkout. After shows the PR head, diff summary, and patch excerpts from the same fresh checkout. Before / AfterBefore After Evidence
|
|
Updated #81247 with a fresh deterministic before/after proof image and replaced the old text-only proof with a raw GitHub artifact. The proof compares Fresh artifact: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/pr-81247-claude-cli-session-invalidation-before-after-proof.png |
|
@galiniliev thanks for the maintainer review here — direct follow-up so this does not get lost. You were right about the remaining merge-readiness gap. Since your comment, I refreshed the branch/proof and the PR is now clean/mergeable, with a fresh deterministic before/after artifact in the PR body. That proof shows Fresh proof image: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/pr-81247-claude-cli-session-invalidation-before-after-proof.png That said, this is still deterministic source-seam/runtime-path proof, not a live after-fix reused Claude CLI poisoned-session capture. I am not claiming that it satisfies the exact live-runtime proof gap you called out unless you or another maintainer explicitly accept this proof level for this failure mode. Otherwise the remaining next step is still a redacted live Claude CLI runtime capture showing the poisoned reused binding is cleared and the following turn starts fresh. |
|
@galiniliev I got the live Claude CLI reused-session proof now. This was run through the repo's secret-bearing Run: https://github.com/openclaw/openclaw/actions/runs/25981733389 Relevant log markers: So the live path exercised a reused Claude CLI session ( |


Summary
Closes #78785.
When a reused
claude-clisession fails withAbortErroror a non-session_expiredFailoverError, OpenClaw currently keeps the stored CLI session id. The next turn can then resume the same poisoned Claude CLI transcript/session and wedge again.This PR keeps the existing
session_expiredclear-and-retry path unchanged, and adds a separate clear-only path for failed reused Claude CLI turns:AbortErrorclears the storedclaude-clibinding before rethrowing.session_expiredFailoverErrorclears the storedclaude-clibinding before rethrowing.claude-clisessions.Motivation
The issue has live logs showing
claude live session reuse, no output for the watchdog window, thenFailoverError(reason=timeout). Source inspection also showed the old code only cleared stored CLI sessions forsession_expired, leaving aborts and timeout failovers eligible for reuse on the next turn.Clearing the stored id after a failed reused turn is the smallest defensive fix: the next turn starts fresh instead of resuming the session id that just failed.
What changed
runAgentAttemptfor failed reused Claude CLI turns.clearCliSessionInStorehelper socliSessionBindings,cliSessionIds, and legacyclaudeCliSessionIdare cleared the same way as existing session cleanup.session_expiredclear-and-retry behavior;AbortErrorclear-only behavior;FailoverError(reason=timeout)clear-only behavior.Real behavior proof
claude-clisession id should be invalidated afterAbortErroror non-session_expiredFailoverErrorfailures, so the next turn cannot resume the poisoned Claude CLI session. The existingsession_expiredclear-and-retry path must remain unchanged.upstream/mainand actual PR head8cec105b7017ae9f1c39e4a58672220becb2479c.upstream/mainand the actual fix(agents): clear poisoned claude cli sessions #81247 head; in each worktree rannode ./probe-claude-cli-session-invalidation.mjs. The probe inspects the OpenClaw command/session-store seam and regression coverage: AbortError classification, non-expired FailoverError classification, preservation of the existingsession_expiredclear-and-retry path, clear-only rethrow behavior, and tests that seed reusable Claude CLI transcripts, assert one failed CLI call, and assert all storedclaude-clibindings are cleared.upstream/mainonly has the existingsession_expiredclear-and-retry path; it does not classify AbortError or non-expired FailoverError for clear-only invalidation. PR head adds the clear-only path for reused Claude CLI failures, preservessession_expiredretry behavior, rethrows the original error, and adds regressions proving AbortError/timeout failures clearcliSessionBindings,cliSessionIds, and legacyclaudeCliSessionIdwithout retrying.Risk
Low. The change is scoped to reused
claude-clisessions and uses the existing session-store clear helper. It does not alter API backends, fresh CLI starts, or the existingsession_expiredretry path.