fix(cli-backend): restore compaction lifecycle and rotate claude-cli sessions after compaction#68388
fix(cli-backend): restore compaction lifecycle and rotate claude-cli sessions after compaction#68388jacko2bot wants to merge 2 commits into
Conversation
…sessions after compaction CLI-backed sessions skipped both pre-turn native compaction checks and post-turn context-engine/plugin maintenance, causing context to grow unbounded (see openclaw#68329). Compacting only OpenClaw's local transcript was also insufficient for claude-cli, which maintains its own resumed session history on disk. This change: - runs pre-turn native compaction lifecycle for CLI-backed runs - runs post-turn context-engine/plugin maintenance for CLI-backed runs - rotates Claude CLI sessions after compaction by seeding a new resumed session JSONL from the compacted transcript and using the new session id - falls back to a prompt-based reseed if Claude seeded-session creation fails Closes openclaw#68329 Co-Authored-By: Claude Opus 4.7 <[email protected]>
openclaw-barnacle
Bot
added
commands
![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: a153f53aa4
ℹ️ 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".
| context.reseedPrompt = undefined; | ||
| context.forceFreshSession = false; | ||
| context.reusableCliSession = { sessionId: seededSessionId }; | ||
| await params.onResetReusableCliSession?.(); |
There was a problem hiding this comment.
After compaction, this branch clears reseedPrompt and forces a resumed Claude session with the seeded ID. The resumed path is treated as isNew=false in executePreparedCliRun, so backends configured with systemPromptWhen: "first" skip reapplying the OpenClaw system prompt; however, the seeded JSONL built here only contains user/assistant transcript messages. That means compacted claude-cli turns can proceed without the system/tool instructions, causing behavior regressions right after compaction.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR restores the compaction lifecycle for CLI-backed agent runs ( Confidence Score: 5/5Safe to merge — the new compaction lifecycle is well-guarded with a prompt-based fallback, and all remaining findings are P2 style/maintenance notes. No P0 or P1 bugs found. The implementation correctly adds pre-turn compaction, Claude CLI session rotation, and a prompt-based reseed fallback, with proper error isolation ( src/agents/cli-runner.ts — the Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 259-265
Comment:
**Hardcoded Claude CLI version fallback**
`version: "2.1.114"` is used whenever no existing session file is found (or the file read fails). This value will drift as Claude Code ships new versions, and if the JSONL schema changes structurally the seeded file could be rejected. The fallback at `createClaudeSeededSession.catch(() => undefined)` prevents a hard failure, but the stale default may produce misleading session metadata in logs. Consider extracting this to a named constant so it's easy to bump:
```suggestion
const defaultMetadata: ClaudeSeedMetadata = {
cwd: resolvedWorkspace,
version: CLAUDE_CLI_FALLBACK_SESSION_VERSION,
gitBranch: "HEAD",
entrypoint: "sdk-cli",
userType: "external",
permissionMode: "bypassPermissions",
};
```
where `const CLAUDE_CLI_FALLBACK_SESSION_VERSION = "2.1.114";` is declared near the top of the file next to the other type definitions.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 174-176
Comment:
**Encoding may diverge from Claude Code's actual project-path scheme**
`encodeClaudeProjectPath` encodes every non-alphanumeric character as `-`. Claude Code's own encoding for the project path directory is not publicly documented; if it differs (e.g. it URL-encodes or uses a different separator), the `readFile` call in `readClaudeSeedMetadata` will silently return `""` (falling back to default metadata), and `createClaudeSeededSession` will write the new JSONL to a path that Claude Code never looks at. The graceful fallback to prompt-based reseed prevents a hard failure, but compaction context would be silently lost in those cases. A comment here noting the assumption and a known-good Claude Code version it was validated against would help future maintainers detect drift.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 267-273
Comment:
**`sessionId` used as a path component without format validation**
`sessionId` (from `params.existingSessionId`) is interpolated directly into the file path as `/${sessionId}.jsonl`. Claude-generated session IDs are UUIDs, but the value ultimately originates from the session store (externally writable). A path like `../../sensitive` would resolve outside `~/.claude/projects/`. The impact is limited to reads (not writes) here, and the session store is not attacker-controlled in typical deployments, but a defensive check is cheap:
```suggestion
const safeSessionId = /^[0-9a-f-]{36}$/i.test(sessionId) ? sessionId : null;
if (!safeSessionId) {
return defaultMetadata;
}
const sessionFile = `${process.env.HOME ?? ""}/.claude/projects/${encodeClaudeProjectPath(resolvedWorkspace)}/${safeSessionId}.jsonl`;
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(cli-backend): restore compaction lif..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const defaultMetadata: ClaudeSeedMetadata = { | ||
| cwd: resolvedWorkspace, | ||
| version: "2.1.114", | ||
| gitBranch: "HEAD", | ||
| entrypoint: "sdk-cli", | ||
| userType: "external", | ||
| permissionMode: "bypassPermissions", |
There was a problem hiding this comment.
version: "2.1.114" is used whenever no existing session file is found (or the file read fails). This value will drift as Claude Code ships new versions, and if the JSONL schema changes structurally the seeded file could be rejected. The fallback at createClaudeSeededSession.catch(() => undefined) prevents a hard failure, but the stale default may produce misleading session metadata in logs. Consider extracting this to a named constant so it's easy to bump:
| const defaultMetadata: ClaudeSeedMetadata = { | |
| cwd: resolvedWorkspace, | |
| version: "2.1.114", | |
| gitBranch: "HEAD", | |
| entrypoint: "sdk-cli", | |
| userType: "external", | |
| permissionMode: "bypassPermissions", | |
| const defaultMetadata: ClaudeSeedMetadata = { | |
| cwd: resolvedWorkspace, | |
| version: CLAUDE_CLI_FALLBACK_SESSION_VERSION, | |
| gitBranch: "HEAD", | |
| entrypoint: "sdk-cli", | |
| userType: "external", | |
| permissionMode: "bypassPermissions", | |
| }; |
where const CLAUDE_CLI_FALLBACK_SESSION_VERSION = "2.1.114"; is declared near the top of the file next to the other type definitions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 259-265
Comment:
**Hardcoded Claude CLI version fallback**
`version: "2.1.114"` is used whenever no existing session file is found (or the file read fails). This value will drift as Claude Code ships new versions, and if the JSONL schema changes structurally the seeded file could be rejected. The fallback at `createClaudeSeededSession.catch(() => undefined)` prevents a hard failure, but the stale default may produce misleading session metadata in logs. Consider extracting this to a named constant so it's easy to bump:
```suggestion
const defaultMetadata: ClaudeSeedMetadata = {
cwd: resolvedWorkspace,
version: CLAUDE_CLI_FALLBACK_SESSION_VERSION,
gitBranch: "HEAD",
entrypoint: "sdk-cli",
userType: "external",
permissionMode: "bypassPermissions",
};
```
where `const CLAUDE_CLI_FALLBACK_SESSION_VERSION = "2.1.114";` is declared near the top of the file next to the other type definitions.
How can I resolve this? If you propose a fix, please make it concise.| function encodeClaudeProjectPath(cwd: string): string { | ||
| return cwd.replace(/[^A-Za-z0-9]/g, "-"); | ||
| } |
There was a problem hiding this comment.
encodeClaudeProjectPath encodes every non-alphanumeric character as -. Claude Code's own encoding for the project path directory is not publicly documented; if it differs (e.g. it URL-encodes or uses a different separator), the readFile call in readClaudeSeedMetadata will silently return "" (falling back to default metadata), and createClaudeSeededSession will write the new JSONL to a path that Claude Code never looks at. The graceful fallback to prompt-based reseed prevents a hard failure, but compaction context would be silently lost in those cases. A comment here noting the assumption and a known-good Claude Code version it was validated against would help future maintainers detect drift.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 174-176
Comment:
**Encoding may diverge from Claude Code's actual project-path scheme**
`encodeClaudeProjectPath` encodes every non-alphanumeric character as `-`. Claude Code's own encoding for the project path directory is not publicly documented; if it differs (e.g. it URL-encodes or uses a different separator), the `readFile` call in `readClaudeSeedMetadata` will silently return `""` (falling back to default metadata), and `createClaudeSeededSession` will write the new JSONL to a path that Claude Code never looks at. The graceful fallback to prompt-based reseed prevents a hard failure, but compaction context would be silently lost in those cases. A comment here noting the assumption and a known-good Claude Code version it was validated against would help future maintainers detect drift.
How can I resolve this? If you propose a fix, please make it concise.| const sessionId = params.existingSessionId?.trim(); | ||
| if (!sessionId) { | ||
| return defaultMetadata; | ||
| } | ||
|
|
||
| const sessionFile = `${process.env.HOME ?? ""}/.claude/projects/${encodeClaudeProjectPath(resolvedWorkspace)}/${sessionId}.jsonl`; | ||
| const raw = await cliRunnerDeps.readFile(sessionFile).catch(() => ""); |
There was a problem hiding this comment.
sessionId used as a path component without format validation
sessionId (from params.existingSessionId) is interpolated directly into the file path as /${sessionId}.jsonl. Claude-generated session IDs are UUIDs, but the value ultimately originates from the session store (externally writable). A path like ../../sensitive would resolve outside ~/.claude/projects/. The impact is limited to reads (not writes) here, and the session store is not attacker-controlled in typical deployments, but a defensive check is cheap:
| const sessionId = params.existingSessionId?.trim(); | |
| if (!sessionId) { | |
| return defaultMetadata; | |
| } | |
| const sessionFile = `${process.env.HOME ?? ""}/.claude/projects/${encodeClaudeProjectPath(resolvedWorkspace)}/${sessionId}.jsonl`; | |
| const raw = await cliRunnerDeps.readFile(sessionFile).catch(() => ""); | |
| const safeSessionId = /^[0-9a-f-]{36}$/i.test(sessionId) ? sessionId : null; | |
| if (!safeSessionId) { | |
| return defaultMetadata; | |
| } | |
| const sessionFile = `${process.env.HOME ?? ""}/.claude/projects/${encodeClaudeProjectPath(resolvedWorkspace)}/${safeSessionId}.jsonl`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 267-273
Comment:
**`sessionId` used as a path component without format validation**
`sessionId` (from `params.existingSessionId`) is interpolated directly into the file path as `/${sessionId}.jsonl`. Claude-generated session IDs are UUIDs, but the value ultimately originates from the session store (externally writable). A path like `../../sensitive` would resolve outside `~/.claude/projects/`. The impact is limited to reads (not writes) here, and the session store is not attacker-controlled in typical deployments, but a defensive check is cheap:
```suggestion
const safeSessionId = /^[0-9a-f-]{36}$/i.test(sessionId) ? sessionId : null;
if (!safeSessionId) {
return defaultMetadata;
}
const sessionFile = `${process.env.HOME ?? ""}/.claude/projects/${encodeClaudeProjectPath(resolvedWorkspace)}/${safeSessionId}.jsonl`;
```
How can I resolve this? If you propose a fix, please make it concise.
martingarramon
left a comment
martingarramon
left a comment
There was a problem hiding this comment.
Thanks for picking this up and for the clean write-up. Reading the diff (~770 source LOC + 242 tests), the direction holds and the pragmatic additions beyond the A/B framing are correct calls. Direct answer to your closing question: ship A, don't flip to B. Detail + 5 concerns + 1 architectural observation below.
A vs B — ship A
For this fix, right call. Users are stuck on claude-cli; B (lift compaction eval above all runners) is the correct long-term shape but is a cross-cutting refactor that would hold up this unblock for weeks. Your "A is additive, B-refactor lifts this cleanly" framing matches the code: if the preemptive-compaction + context-engine-maintenance helpers ever move to a neutral namespace, cli-runner.ts would just swap the import path.
One architectural observation — call it latent coupling, not a blocker: cli-runner.ts now imports from ./pi-embedded-runner/run/preemptive-compaction.js (102 LOC) and ./pi-embedded-runner/context-engine-maintenance.js (651 LOC). That's ~750 LOC of "shared" machinery that still lives under pi-embedded-runner/'s namespace. The name says "embedded"; the code is now used by CLI too. Worth a tracking issue ("refactor: move compaction helpers to src/agents/compaction/ for provider-neutral access") so the name and home match the actual ownership — that's the B-refactor you correctly scoped out of this PR, just made explicit and deferred.
Concerns on the claude-cli rotation path
1. Claude project-path encoding assumption (cli-runner.ts:encodeClaudeProjectPath)
function encodeClaudeProjectPath(cwd: string): string {
return cwd.replace(/[^A-Za-z0-9]/g, "-");
}This is reverse-engineering Claude CLI's project-path encoding from observed behavior — you acknowledged the version-stability concern in the PR body, but specifically on THIS function: if Claude's encoding ever changes (e.g., percent-based, or collapses consecutive separators differently), the path lookup silently misses an existing session file and you fall back to defaultMetadata. Silent degradation with no log. Would either (a) log verbose on "couldn't read existing session metadata, using defaults" so operators can diagnose, or (b) probe a couple of encoding variants before giving up. Low blast radius since you have the prompt-reseed fallback below it — but it degrades the rotation path from "preserves CLI session continuity" to "restarts from scratch" silently.
2. Silent failure on seeded-session creation (cli-runner.ts call site)
const seededSessionId = await createClaudeSeededSession({...}).catch(() => undefined);The .catch(() => undefined) swallows every error class: fs EACCES, ENOSPC, invalid JSON in metadata, HOME unset, etc. All collapse to "fall back to prompt reseed," which your test asserts — good — but operators see no log line explaining why rotation didn't happen. Suggest logVerbose(\claude-cli seeded session creation failed: ${err}`)` in the catch so the "why am I getting prompt reseed instead of session rotation" question is diagnosable.
3. HOME unset edge case (cli-runner.ts:readClaudeSeedMetadata + createClaudeSeededSession)
const sessionFile = `${process.env.HOME ?? ""}/.claude/projects/${encodeClaudeProjectPath(...)}/${sessionId}.jsonl`;With HOME unset, the path becomes /.claude/projects/... — writing there on a hardened container (rootfs read-only) throws, which lands in the swallow-all catch. On a dev machine running as a weird user, writes SUCCEED to /.claude/... which is the wrong place entirely. Both paths are wrong. os.homedir() would be more correct than process.env.HOME ?? "", and an early-return-if-not-resolved would prevent the wrong-place-writes case.
4. Session JSONL accumulation (design question)
createClaudeSeededSession writes a new <uuid>.jsonl to ~/.claude/projects/<encoded-cwd>/. After N compactions the directory has N+1 files, none of them explicitly cleaned. Claude CLI itself may or may not prune its own sessions — if not, this is a slow leak. Not blocking (users can rm them), but worth (a) flagging in the PR body, or (b) a best-effort delete of the previous rotation's file once rotation completes.
5. Runtime-context inconsistency between pre- and post-turn maintenance
- Pre-turn:
runtimeContext.currentTokenCount = preemptiveCompaction.estimatedPromptTokens(cli-runner.ts) - Post-turn (
agent-command.ts:runCliTurnContextEngineMaintenance):runtimeContext.currentTokenCount = sessionEntry.totalTokens(when present and finite)
Two different sources of "current token count" into the same runContextEngineMaintenance API, same session. If any context-engine plugin sees both calls, the reported counts will non-monotonically step (estimated → actual post-turn is usually LOWER). This is semantically fine — pre-turn is "what we're about to send", post-turn is "what the turn cost" — but worth documenting in a code comment so future plugin authors don't interpret the sequence as a bug.
Test coverage gaps
The two compaction tests are DI-synthesized and cover the control flow well, but they don't exercise the real Claude JSONL format. Two I'd add before merge:
- A test that asserts
buildClaudeSeededSessionEntriesoutput, when round-tripped throughJSON.parseline-by-line, contains the required fields Claude checks (parentUuid,isSidechain,type,message,uuid,timestamp,cwd,sessionId,version,gitBranch) with the right types. You listed these in the PR body — an assertion lock prevents drift. - A test for the fallback path where
createClaudeSeededSessionrejects, asserting the reseed-prompt branch fires andforceFreshSession: trueis set. The existing "seed success" test covers happy path only.
Minor, non-blocking
cli-runner.tsuses${process.env.HOME ?? ""}in two sites — factor to a helperclaudeProjectsRoot()that usesos.homedir()and returnsundefinedon unresolvable home, and bail early from both callers. One-line change in the helper, cleaner error semantics.- PR scope is right at the edge of "cognitive-load rank 2" (9 files, ~1K diff). The claude-cli rotation path is ~200 LOC of coherent work that could stand alone as a follow-up after the Option-A scaffolding lands — but given you've demonstrated end-to-end validation and the two halves are semantically coupled (compaction without rotation is still broken on
claude-cli), I wouldn't block on the split. shouldPreemptivelyCompactBeforePromptreturn includesroute: "compact_only"— your code ignoresrouteand only readsshouldCompact. If the pi-embedded-runner side ever adds non-compact_onlyroutes (e.g.,drop_tool_results), cli-runner would silently miss those. Worth a comment noting this is compact-only by design.
Validation work is strong — production observation (22 restarts → 0 at aggressive thresholds), deterministic tests, tsc clean. The tradeoff calls you surfaced in the PR body are the right ones.
Happy to re-review after the rotation-path logging + JSONL format assertion lands.
|
Maintainer note from the ACP/session-continuity sweep: this is the right broad fix direction for #70304/#69973-adjacent CLI context drift, but please address the hardening before landing. In particular, |
…th encoding, fix lint gate
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d9a53f14f
ℹ️ 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".
| const role = | ||
| message.role === "assistant" ? "Assistant" : message.role === "user" ? "User" : ""; | ||
| if (!role) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
buildCliHistoryReseedPrompt only serializes user/assistant roles and reads text from content, but compacted transcripts carry retained history in compactionSummary.summary entries. In sessions where compaction has replaced most earlier turns with summaries, this makes renderedHistory empty and the fallback reseed prompt degrades to just the next user message, dropping the summarized context that compaction was supposed to preserve.
Useful? React with 👍 / 👎.
| const role = message.role === "assistant" ? "assistant" : message.role === "user" ? "user" : ""; | ||
| if (!role) { | ||
| continue; | ||
| } | ||
| const text = coerceMessageText(message.content); |
There was a problem hiding this comment.
buildClaudeSeededSessionEntries drops every non-user/assistant message and extracts text only from message.content, so compactionSummary entries are never written into the seeded Claude session file. After a compaction pass, this can omit the only retained long-term context (or yield an empty seed file), so the rotated Claude session resumes without the compacted history.
Useful? React with 👍 / 👎.
|
Hey @steipete — thanks for the thorough feedback! I've addressed all three points in the latest commit (8d9a53f):
Looks like the main CI workflows are gated on first-time contributor approval — would you mind clicking "Approve and run" on the Actions tab so we can confirm the gate is green? Appreciate it! |
martingarramon
left a comment
martingarramon
left a comment
There was a problem hiding this comment.
Re-review of 8d9a53f (static; no fresh checkout).
Both hardening asks look addressed. isSafeClaudeCliSessionId at cli-runner.ts guards readClaudeSeedMetadata before the ${HOME}/.claude/projects/<encoded>/<sessionId>.jsonl interpolation, with helper tests covering ../evil, a/b, a\b, and ../../etc/passwd. encodeClaudeProjectPath JSDoc pins Claude CLI v2.1.114 and names the breakage-recovery path (ls ~/.claude/projects/), with 4 helper tests on case, digits, non-collapsing, and roundtrip.
One observation I didn't surface on the Apr 18 pass:
coerceMessageText silently drops assistant tool-call turns. Both buildClaudeSeededSessionEntries and buildCliHistoryReseedPrompt gate emission on non-empty text and only walk .text on content blocks. An assistant message whose content is ONLY tool_use / tool_result blocks returns empty string → the whole message is skipped. On the claude-cli rotation path (agent-loop heavy), that produces a reseeded session with visible gaps: User: X → [assistant turn missing] → User: Y. Worth either rendering a placeholder ([assistant tool_use: <name>]) in the text output, or mapping tool_use / tool_result blocks into the Claude CLI JSONL content shape so the assistant turn is still anchored for Claude's continuation parser.
|
Closing this as duplicate or superseded after Codex automated review. Close #68388 as superseded by merged #71916. Current main implements the canonical CLI transcript compaction path for #68329 and avoids this PR's older Claude-private JSONL session rotation design. Best possible solution: Close #68388 and treat merged #71916 as the canonical implementation for #68329. Any remaining broader CLI context-engine lifecycle gaps should be handled in narrower follow-up issues rather than by reviving this older Claude-private JSONL rotation PR. What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a; fix evidence: commit d54d2d6b9b8a. |
3 participants
Summary
Fixes #68329 — CLI-backed sessions skipped both pre-turn native compaction checks and post-turn context-engine/plugin maintenance, so context grew unbounded. For
claude-clispecifically, compacting only OpenClaw's local transcript was not enough: Claude Code maintains its own resumed session history on disk, so the next turn re-loaded the pre-compaction context.This PR:
Files changed
src/agents/cli-runner.tssrc/agents/cli-runner/execute.tssrc/agents/cli-runner/types.tssrc/agents/cli-runner/prepare.tssrc/agents/command/attempt-execution.tssrc/agents/agent-command.tssrc/agents/cli-runner.compaction.test.ts(new)src/agents/command/attempt-execution.cli.test.tssrc/commands/agent.test.tsValidation
Run locally on the patched checkout:
pnpm exec vitest run src/agents/command/attempt-execution.cli.test.ts src/agents/cli-runner.compaction.test.ts src/commands/agent.test.ts— passedNODE_OPTIONS=--max-old-space-size=8192 pnpm exec tsc -p tsconfig.json --noEmit— passedTest plan
Closes #68329