fix(agents): use atomic store helper for CLI session clearing#70298
Conversation
Greptile SummaryThis PR fixes a bug where Confidence Score: 5/5Safe to merge — core fix is correct and well-scoped; only edge-case concern is a minor behavioral difference when the session entry is unexpectedly absent. All findings are P2 or lower. The root-cause fix (delete → undefined) is correct, the new helper mirrors an established pattern, and the dead code removal is unambiguous. The one noted behavioral change (params.sessionEntry clobbered to undefined when key is missing) is an edge case unlikely to occur during a session_expired error. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/command/attempt-execution.ts
Line: 305-310
Comment:
**Behavioral change when session key is missing**
In the old code, the update was guarded by `if (entry) { ... params.sessionEntry = updatedEntry }` — if the key was absent from the store, `params.sessionEntry` was left unchanged. The new code unconditionally assigns the result of `clearCliSessionInStore`, which returns `undefined` when the key is missing, so `params.sessionEntry` is clobbered to `undefined` in that edge case.
While a `session_expired` error almost always implies the entry exists, this is a silent state change that could affect downstream consumers of `params.sessionEntry` if the entry ever goes missing mid-run.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agents): use atomic store helper for..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| params.sessionEntry = await clearCliSessionInStore({ | ||
| provider: params.providerOverride, | ||
| sessionKey: params.sessionKey, | ||
| sessionStore: params.sessionStore, | ||
| storePath: params.storePath, | ||
| }); |
There was a problem hiding this comment.
In the old code, the update was guarded by if (entry) { ... params.sessionEntry = updatedEntry } — if the key was absent from the store, params.sessionEntry was left unchanged. The new code unconditionally assigns the result of clearCliSessionInStore, which returns undefined when the key is missing, so params.sessionEntry is clobbered to undefined in that edge case.
While a session_expired error almost always implies the entry exists, this is a silent state change that could affect downstream consumers of params.sessionEntry if the entry ever goes missing mid-run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/command/attempt-execution.ts
Line: 305-310
Comment:
**Behavioral change when session key is missing**
In the old code, the update was guarded by `if (entry) { ... params.sessionEntry = updatedEntry }` — if the key was absent from the store, `params.sessionEntry` was left unchanged. The new code unconditionally assigns the result of `clearCliSessionInStore`, which returns `undefined` when the key is missing, so `params.sessionEntry` is clobbered to `undefined` in that edge case.
While a `session_expired` error almost always implies the entry exists, this is a silent state change that could affect downstream consumers of `params.sessionEntry` if the entry ever goes missing mid-run.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44b033a9dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (isFailoverError(err)) { | ||
| const retryableSessionId = context.reusableCliSession.sessionId ?? params.cliSessionId; | ||
| // Check if this is a session expired error and we have a session to clear | ||
| if (err.reason === "session_expired" && retryableSessionId && params.sessionKey) { | ||
| // Clear the expired session ID from the session entry | ||
| // This requires access to the session store, which we don't have here | ||
| // We'll need to modify the caller to handle this case | ||
|
|
||
| // For now, retry without the session ID to create a new session | ||
| const output = await executePreparedCliRun(context, undefined); | ||
| const effectiveCliSessionId = output.sessionId; | ||
| return buildCliRunResult({ output, effectiveCliSessionId }); | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
The new if (isFailoverError(err)) { throw err; } path removes the runner-level retry for session_expired, but other production callers still rely on runCliAgent to handle that case. I checked src/cron/isolated-agent/run-executor.ts:123-145 and src/auto-reply/reply/agent-runner-execution.ts:869-923: both pass persisted CLI session IDs and do not implement their own clear-and-retry flow on session_expired. With this change, expired sessions now bubble up as hard failures (especially when no alternate fallback candidate exists) instead of retrying once with a fresh CLI session, which is a behavior regression introduced by this commit.
Useful? React with 👍 / 👎.
|
Maintainer triage: this needs fixup before merge. Current CI has real unit failures in Also, this is adjacent to #70177 but does not fix it. #70177 needs transcript-backed resume validation for Claude sessions: if a stored Claude session id has no backing JSONL transcript/content, invalidate the binding with an explicit reason instead of resuming a ghost session. |
- Bug fix: `clearCliSession` and `clearAllCliSessions` used `delete` to remove CLI session properties. When `mergeSessionEntry` later spreads the update object over the persisted store entry, deleted keys are absent from the spread, so stale session IDs survive the merge and the expired session is never actually cleared. Changed to `= undefined`. - Refactor: Extracted `clearCliSessionInStore()` in `session-store.ts` — an atomic helper that clears + persists + merges in one call, matching the existing `updateSessionStoreAfterAgentRun` pattern. - Dead code removal: Removed unreachable retry logic in `cli-runner.ts` that attempted session recovery without store access (duplicated by the caller in `attempt-execution.ts`).
Addresses CI failures and bot review feedback on PR openclaw#70298: - Restore session-expired retry in cli-runner.ts — the runner-level retry is a safety net for callers (run-executor, agent-runner-execution) that do not implement their own clear-and-retry flow. - Add nil-guard on clearCliSessionInStore result in attempt-execution.ts so params.sessionEntry is preserved when the store key is unexpectedly absent (Greptile P2).
442d535 to
44846ee
Compare
|
Landed via temp rebase onto main.
Thanks @HFConsultant! |
|
Gerne! Danke, dass du es Open Source hältst 🙂 |
| import { resolveMessageChannel } from "../../utils/message-channel.js"; | ||
| import { resolveBootstrapWarningSignaturesSeen } from "../bootstrap-budget.js"; | ||
| import { runCliAgent } from "../cli-runner.js"; | ||
| import { clearCliSession, getCliSessionBinding, setCliSessionBinding } from "../cli-session.js"; |
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
Persist stale CLI session clearing through the session-store merge path and add regression coverage for Claude binding removal.\n\nThanks @HFConsultant.
3 participants
What
Bug fix:
clearCliSessionandclearAllCliSessionsuseddeleteto remove CLI session properties. WhenmergeSessionEntrylater spreads the update object over the persisted store entry, deleted keys are absent from the spread — so stale session IDs survive the merge and the expired session is never actually cleared.Changed to
= undefinedso the key is explicitly present in the spread.Refactor: Extracted
clearCliSessionInStore()insession-store.ts— an atomic helper that clears + persists + merges in one call, matching the existingupdateSessionStoreAfterAgentRunpattern.Dead code removal: Removed unreachable retry logic in
cli-runner.tsthat attempted session recovery without store access (duplicated by the caller inattempt-execution.ts).Why
Without this fix, CLI sessions that expire mid-run are not properly cleared from the store. The agent keeps retrying with the stale session ID instead of creating a fresh session, causing repeated
session_expiredfailures.Testing
vitest --project agents— 382 files, 4000 tests)attempt-execution.cli.test.tscovers the session-expired recovery path end-to-endoxlint+tsc --noEmitcleanAI Disclosure