feat(compaction): disable thinking by default; add compaction.thinking override#33296
feat(compaction): disable thinking by default; add compaction.thinking override#33296iamcobolt wants to merge 11 commits intoopenclaw:mainfrom
Conversation
session model's thinking level. On channels with strict reply windows (Discord 30s, Telegram 240s), extended thinking during compaction can race against the channel timeout and trigger a stale-response loop (see openclaw#25272). Disabling thinking for the summarization turn eliminates this race without affecting the primary conversation model. New config key `agents.defaults.compaction.thinking` allows opting in: { "agents": { "defaults": { "compaction": { "thinking": "low" } } } } When thinking is enabled and a compaction run times out, the runner retries once without thinking automatically. The before_compaction hook is guarded to fire exactly once per compaction event (not once per attempt), keeping plugin integrations consistent.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97643aa029
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| allowedToolNames, | ||
| config: params.config, | ||
| // Thinking retry loop: on timeout with thinking enabled, retry once without thinking. | ||
| for (let thinkAttempt = 0; thinkAttempt <= 1; thinkAttempt++) { |
There was a problem hiding this comment.
Increase lock hold budget for two-attempt compaction
The new thinking retry loop keeps a single session lock across up to two compaction attempts, but the lock budget is still derived from one timeout window (resolveSessionLockMaxHoldFromTimeout in src/agents/session-write-lock.ts:111-123), which is about 420s for a 300s compaction timeout. In the timeout path this can run ~300s on attempt 1 plus a long fallback attempt, and once total hold exceeds the watchdog limit (src/agents/session-write-lock.ts:193-207) the lock is force-released while compaction is still running, allowing concurrent writers on the same session file.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces Key changes:
Minor pre-existing observations (already flagged in previous threads — not repeated here):
Confidence Score: 4/5
Last reviewed commit: 8fa5176 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baef98faec
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = await compactWithSafetyTimeout(() => | ||
| session.compact(params.customInstructions), | ||
| ); |
There was a problem hiding this comment.
Cancel timed-out compaction before starting retry
When a thinking-enabled compaction exceeds the timeout, compactWithSafetyTimeout rejects but does not actually stop session.compact (it uses a timeout race in src/node-host/with-timeout.ts and this callsite does not pass/use the abort signal), and the new retry loop immediately starts a second compaction attempt. In that timeout path, two compactions can overlap on the same session state/file, which can cause duplicate provider work and interleaved session mutations; the retry should only begin after the first attempt is truly cancelled or fully settled.
Useful? React with 👍 / 👎.
|
Tracked in discussion #33430 — split out from #33005, which originally bundled this with a |
|
The Root cause: GHSA-8mvx-p2r9-r375 — |
…en fallbackModel placeholder
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 657f3226bf
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| placeholder?: string; | ||
| itemTemplate?: unknown; | ||
| /** Force segmented button rendering even when the option count exceeds the default threshold. */ | ||
| forceSegmented?: boolean; |
There was a problem hiding this comment.
Add new uiHint key to protocol schema
forceSegmented is now emitted in config UI hints, but the gateway protocol contract still defines ConfigUiHintSchema with additionalProperties: false and no forceSegmented field (src/gateway/protocol/schema/config.ts:40-53). Any client that validates config.schema responses against that schema will reject responses containing this new hint key, so the protocol schema needs to be updated alongside this type addition.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Enum - use segmented for small, dropdown for large | ||
| // Enum - use segmented for small (or forceSegmented), dropdown for large |
There was a problem hiding this comment.
The PR description states that forceSegmented hint machinery was removed (no longer needed with only 2 options). This comment should be updated to reflect the current state and avoid confusing future contributors.
| // Enum - use segmented for small (or forceSegmented), dropdown for large | |
| // Enum - use segmented for small, dropdown for large |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/views/config-form.node.ts
Line: 445
Comment:
The PR description states that `forceSegmented` hint machinery was removed (no longer needed with only 2 options). This comment should be updated to reflect the current state and avoid confusing future contributors.
```suggestion
// Enum - use segmented for small, dropdown for large
```
How can I resolve this? If you propose a fix, please make it concise.|
Latest push — docs wording update only (no logic changes):
|
|
@greptile-apps review |
| - Agents/Compaction: compaction now runs with `thinking: off` by default, regardless of the session model's thinking level — preventing timeout races on channels with strict reply windows (Discord 30s, Telegram 240s). Set `agents.defaults.compaction.thinking: "on"` to inherit the session model's current thinking level. When a compaction run times out with thinking enabled, it retries once without thinking automatically. | ||
| - CLI/Config validation: add `openclaw config validate` (with `--json`) to validate config files before gateway startup, and include detailed invalid-key paths in startup invalid-config errors. (#31220) thanks @Sid-Qin. | ||
| - Sessions/Attachments: add inline file attachment support for `sessions_spawn` (subagent runtime only) with base64/utf8 encoding, transcript content redaction, lifecycle cleanup, and configurable limits via `tools.sessions_spawn.attachments`. (#16761) Thanks @napetrov. | ||
| - Agents/Thinking defaults: set `adaptive` as the default thinking level for Anthropic Claude 4.6 models (including Bedrock Claude 4.6 refs) while keeping other reasoning-capable models at `low` unless explicitly configured. |
There was a problem hiding this comment.
Unrelated changelog entries included
This PR adds four new entries to CHANGELOG.md, but only the first one (Agents/Compaction: compaction now runs with thinking: off by default…) is attributable to this PR's changes. The other three entries reference separate issues and contributors entirely unrelated to compaction thinking:
CLI/Config validation: add openclaw config validate … (#31220) thanks @Sid-Qin— unrelatedSessions/Attachments: add inline file attachment support … (#16761) Thanks @napetrov— unrelatedAgents/Thinking defaults: set adaptive as the default thinking level…— unrelated
These entries appear to have been accidentally picked up from another branch or staging area. If left in, they'll be misattributed to this PR in the release history and could conflict with the changelog entries in their respective PRs when those merge.
Prompt To Fix With AI
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 9-12
Comment:
**Unrelated changelog entries included**
This PR adds four new entries to `CHANGELOG.md`, but only the first one (`Agents/Compaction: compaction now runs with thinking: off by default…`) is attributable to this PR's changes. The other three entries reference separate issues and contributors entirely unrelated to compaction thinking:
- `CLI/Config validation: add openclaw config validate … (#31220) thanks @Sid-Qin` — unrelated
- `Sessions/Attachments: add inline file attachment support … (#16761) Thanks @napetrov` — unrelated
- `Agents/Thinking defaults: set adaptive as the default thinking level…` — unrelated
These entries appear to have been accidentally picked up from another branch or staging area. If left in, they'll be misattributed to this PR in the release history and could conflict with the changelog entries in their respective PRs when those merge.
How can I resolve this? If you propose a fix, please make it concise.| if (diagEnabled && preMetrics) { | ||
| log.debug( | ||
| `[compaction-diag] start runId=${runId} sessionKey=${params.sessionKey ?? params.sessionId} ` + | ||
| `diagId=${diagId} trigger=${trigger} provider=${provider}/${modelId} ` + | ||
| `attempt=${attempt} maxAttempts=${maxAttempts} ` + | ||
| `pre.messages=${preMetrics.messages} pre.historyTextChars=${preMetrics.historyTextChars} ` + | ||
| `pre.toolResultChars=${preMetrics.toolResultChars} pre.estTokens=${preMetrics.estTokens ?? "unknown"}`, | ||
| ); |
There was a problem hiding this comment.
Missing thinkAttempt/effectiveThinkLevel in diagnostic log
The [compaction-diag] start log line logs attempt, maxAttempts, provider, model, etc., but does not include the current effectiveThinkLevel or thinkAttempt. When debugging a timeout-and-retry sequence in production logs, it will be difficult to correlate which thinking level was active for each log line.
Consider adding thinkLevel=${effectiveThinkLevel} thinkAttempt=${thinkAttempt} to this log statement (and the end log below) to make post-mortem analysis easier.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compact.ts
Line: 664-671
Comment:
**Missing `thinkAttempt`/`effectiveThinkLevel` in diagnostic log**
The `[compaction-diag] start` log line logs `attempt`, `maxAttempts`, provider, model, etc., but does not include the current `effectiveThinkLevel` or `thinkAttempt`. When debugging a timeout-and-retry sequence in production logs, it will be difficult to correlate which thinking level was active for each log line.
Consider adding `thinkLevel=${effectiveThinkLevel} thinkAttempt=${thinkAttempt}` to this log statement (and the `end` log below) to make post-mortem analysis easier.
How can I resolve this? If you propose a fix, please make it concise.|
Addressed both Greptile points — pushed in commit 8fa5176:
CHANGELOG entries — the three extra entries ( @greptile-apps review |
…-limit errors
Adds agents.defaults.compaction.fallbackModel — an opt-in key that retries
compaction with a different model when the primary fails due to quota or
rate-limit errors (HTTP 429 / 402 billing). Auth errors and timeouts use
their own retry mechanisms and are unaffected.
"off" (default): no change in behaviour
"fallback": tries each model in agents.defaults.model.fallbacks in order,
skipping the current model; re-throws after exhausting candidates
Also ships the companion thinking-override change (compaction-overrides.ts)
needed for compact.ts's inner thinking-retry loop. See companion PR openclaw#33296.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Superseded by #34554 — reissued as a clean single commit on a proper feature branch ( |
Summary
Part of the compaction override work proposed in #33005.
thinking:high) can take 4+ minutes to compact — exceeding channel timeout windows (Discord 30s, Telegram 240s) and causing a stale-response retry loop.thinking=offby default regardless of session model. Newagents.defaults.compaction.thinkingconfig key —"off"(default) or"on"(inherit the session model's current thinking level). When a compaction run times out with thinking enabled, the runner retries once without thinking automatically. Thebefore_compactionhook is guarded to fire exactly once per compaction event.run.tsare unchanged; no new network calls or auth paths.Schema
agents.defaults.compaction.thinking:"off"(default) |"on""off": compaction always runs without extended thinking regardless of the session model — preventing timeout races on channels with strict reply windows."on": compaction inherits the session model's current thinking level. On timeout, automatically retries once without thinking.Renders as a two-button segmented control in the config UI.
UI — Before / After
Before — 7-option dropdown (
off · minimal · low · medium · high · xhigh · adaptive):After — clean 2-button segmented control:
Companion PR
#33396 — adds
compaction.fallbackModelto retry compaction with a cheaper/different model on quota or rate-limit errors. Can be merged in either order.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR