feat(config): add ratio-based sibling fields for compaction token budgets#66517
feat(config): add ratio-based sibling fields for compaction token budgets#66517cheapestinference wants to merge 2 commits into
Conversation
…gets Adds `*Share` sibling fields so a single agent config behaves sensibly across heterogeneous context windows (GLM/Claude 200k, Kimi K2 1M, small-window models). When a share is set, the absolute-token budget is computed as `Math.floor(contextWindowTokens * share)`; otherwise the existing absolute field applies. Backward compatible — no existing absolute field is removed. Fields extended: - agents.defaults.compaction.reserveTokensShare - agents.defaults.compaction.keepRecentTokensShare - agents.defaults.compaction.reserveTokensFloorShare - agents.defaults.compaction.memoryFlush.softThresholdTokensShare Precedence: share (with known contextWindowTokens) > absolute > default. The resolved reserveTokensFloor is still applied as an absolute minimum on the final reserve-tokens value. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Greptile SummaryThis PR adds four ratio-based
Confidence Score: 4/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| contextWindowTokens > 0 | ||
| ) { | ||
| const computed = Math.floor(contextWindowTokens * share); | ||
| if (computed >= 0) { |
There was a problem hiding this comment.
>= 0 guard vs > 0 in sibling helper
resolveShareBasedTokenBudget uses if (computed > 0) to decide whether the share path produced a usable value and falls back to the absolute/default otherwise. Here the check is computed >= 0, so when Math.floor(contextWindowTokens * share) === 0 (e.g. a 50-token window with share = 0.01), the function returns 0 as the floor instead of falling through to reserveTokensFloor/DEFAULT_PI_COMPACTION_RESERVE_TOKENS_FLOOR. For any realistic context window this doesn't matter, but the inconsistency is surprising for future readers.
| if (computed >= 0) { | |
| if (computed > 0) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-settings.ts
Line: 99
Comment:
**Inconsistent `>= 0` guard vs `> 0` in sibling helper**
`resolveShareBasedTokenBudget` uses `if (computed > 0)` to decide whether the share path produced a usable value and falls back to the absolute/default otherwise. Here the check is `computed >= 0`, so when `Math.floor(contextWindowTokens * share) === 0` (e.g. a 50-token window with `share = 0.01`), the function returns 0 as the floor instead of falling through to `reserveTokensFloor`/`DEFAULT_PI_COMPACTION_RESERVE_TOKENS_FLOOR`. For any realistic context window this doesn't matter, but the inconsistency is surprising for future readers.
```suggestion
if (computed > 0) {
```
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: f22fd96b69
ℹ️ 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".
| >(params: { plan: TPlan; cfg?: OpenClawConfig; contextWindowTokens: number }): TPlan { | ||
| const compactionCfg = params.cfg?.agents?.defaults?.compaction; | ||
| const memoryFlushCfg = compactionCfg?.memoryFlush; | ||
| const softThresholdTokens = resolveShareBasedTokenBudget({ | ||
| share: memoryFlushCfg?.softThresholdTokensShare, |
There was a problem hiding this comment.
This helper always consumes a numeric contextWindowTokens, so callers that only have the fallback from resolveMemoryFlushContextWindowTokens (defaulting unknown models to 200k) still apply softThresholdTokensShare/reserveTokensFloorShare instead of falling back to the plan’s absolute values. In custom/unknown-model runs, that can inflate flush thresholds and defer compaction/flush decisions far past the real window size. The share override should only run when the model window is truly resolved, otherwise keep the existing absolute plan values.
Useful? React with 👍 / 👎.
Audited every site that consumes the new `*Share` fields and confirmed the context-window base is always the SESSION-active model: - `pi-embedded-runner/run/attempt.ts` reads `params.model.contextWindow`, where `params.model` is `effectiveModel` from `resolveEffectiveRuntimeModel` using the per-run `provider`/`modelId` (which already honors session model overrides via `resolveHookModelSelection` and persisted session overrides). - `pi-embedded-runner/compact.ts` reads `model?.contextWindow` from the model resolved by `resolveModelAsync(provider, modelId, …)` where provider/modelId flow from `params.provider`/`params.model` (session-scoped) through `resolveEmbeddedCompactionTarget`, with the documented `compaction.model` override taking precedence only when explicitly configured. - `auto-reply/reply/memory-flush.ts` resolves the window via `resolveMemoryFlushContextWindowTokens` from `followupRun.run.model` — the per-run model captured at queue time, which is session-scoped. No code path computes the share against a statically baked agent-default model, so no fix to the resolution logic is required. Documentation, however, was implicit. This commit: - Extends JSDoc on each `*Share` field in `types.agent-defaults.ts` to state explicitly that the base is the context window of the model active for the CURRENT SESSION, falling back to the agent's default model, then to DEFAULT_CONTEXT_TOKENS. - Adds a regression test in `reply-state.test.ts` proving that two sessions of the same agent (one inheriting the agent default glm-4.7@200k, one overriding to kimi-k2@1M) compute different `reserveTokensFloor` and `softThresholdTokens` from the same share config — locking in the session-scoped semantics so a future refactor cannot silently regress to agent-scoped resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a96f4450f8
ℹ️ 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".
| cwd: effectiveWorkspace, | ||
| agentDir, | ||
| cfg: params.config, | ||
| contextWindowTokens: params.model.contextWindow, |
There was a problem hiding this comment.
Passing params.model.contextWindow here causes share-based compaction budgets to run even when the model window is not actually known, because the model resolver synthesizes contextWindow from DEFAULT_CONTEXT_TOKENS when metadata is missing (see src/agents/pi-embedded-runner/model.ts:551-554). With this commit, that fallback value is treated as authoritative, so reserveTokensShare/keepRecentTokensShare/reserveTokensFloorShare are computed from 200k instead of falling back to absolute/default values, which can badly over-reserve on small or custom models; this same pattern is also introduced in src/agents/pi-embedded-runner/compact.ts.
Useful? React with 👍 / 👎.
|
CI note — 2 jobs failed on initial run but do not appear to be caused by this PR:
Neither test file is touched by this PR (verified via Happy to rebase or push an empty commit to re-trigger CI once a maintainer confirms. The PR itself passes locally:
|
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the same percentage/share-based compaction-config work is now tracked by the canonical feature issue and a newer current-main PR, while this older branch is dirty, still has behavioral review blockers, and lacks real behavior proof. So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Use the canonical feature issue and newer current-main PR as the merge path, preserving useful ideas from this branch only if maintainers choose that API shape. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a new public config capability, not a broken existing behavior. Source inspection gives high confidence that current main is still absolute-token-only for these compaction budgets. Is this the best way to solve the issue? No. The active current-main path in #81176 is a better place to settle the API, while this branch needs a substantial rebase, behavioral fixes, and real-run proof before it could merge. Security review: Security review cleared: The diff is limited to config schema/help/generated metadata, compaction runtime logic, and tests; no workflow, dependency, lockfile, secret-handling, publishing, or artifact-download changes were found. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 3b7d01b63f02. |
1 participant
Motivation
OpenClaw users increasingly run multiple agents side-by-side against models
with wildly different context windows — e.g. GLM 200k, Claude 200k, Kimi K2
1M, and smaller local models. The compaction config today mixes ratios
(
softTrimRatio,hardClearRatio,maxHistoryShare) with absolute-tokenbudgets (
reserveTokens,keepRecentTokens,reserveTokensFloor, andmemoryFlush.softThresholdTokens). An absolute likereserveTokens: 50000reserves 25% of a 200k window but only 5% of a 1M window, and over-reserves
aggressively against an 8k window. The same config behaves completely
differently depending on the model, so per-model configs end up fragmented.
This PR introduces additive
*Sharesibling fields so a single configcan scale correctly across heterogeneous models.
Fields that got a ratio sibling
agents.defaults.compaction.reserveTokensShareagents.defaults.compaction.keepRecentTokensShareagents.defaults.compaction.reserveTokensFloorShareagents.defaults.compaction.memoryFlush.softThresholdTokensShareAll four are validated as
numberin[0.01, 0.9]inzod-schema.agent-defaults.ts, mirroring the existingmaxHistorySharerange convention.
Precedence rules
*Shareis set and the model'scontextWindowTokensis knownat runtime →
Math.floor(contextWindowTokens * share).reserveTokens,keepRecentTokens,reserveTokensFloor,softThresholdTokens) is set → use it as before.DEFAULT_PI_COMPACTION_RESERVE_TOKENS_FLOOR,memory-core defaults, etc.).
reserveTokensFloor(resolved from share or absolute) is still applied asan absolute minimum on the final
reserveTokensvalue, so a smallshare against a large window never drops below the safety floor.
Backward compatibility
for predictability keep that option.
identical to pre-PR behavior.
contextWindowTokensis unknown (e.g. older code paths that don'tplumb it yet), the share is silently ignored and the absolute/default
path applies. No runtime error, no config error.
MemoryFlushPlanstill ships withabsolute
softThresholdTokens/reserveTokensFloorso external pluginskeep working. Share-based overrides are applied at the consumption site
(
applyMemoryFlushSharesToPlan) where the context window is resolved.Implementation notes
resolveShareBasedTokenBudget(...)centralizes the share →absolute → fallback precedence.
applyPiCompactionSettingsFromConfigtakes an optionalcontextWindowTokensand resolves shares against it. Call sites inpi-embedded-runner/run/attempt.tsandpi-embedded-runner/compact.tsnow pass
model.contextWindow.resolveCompactionReserveTokensFloorgained an optionalcontextWindowTokensarg; when present, it resolvesreserveTokensFloorSharefirst.applyMemoryFlushSharesToPlanoverrides the plugin-provided flush plan'ssoftThresholdTokensandreserveTokensFloorwith share-based valueswhen configured and a context window is known, leaving the rest of the
plan untouched.
contextWindowTokens, and matching entries inschema.help.ts/schema.labels.ts.schema.base.generated.tswas regenerated viapnpm config:schema:gen.Test coverage summary
Extended
src/agents/pi-settings.test.tsandsrc/auto-reply/reply/reply-state.test.tswith the following coverage:contextWindowTokens→ falls back to absolute (backward compat)keepRecentTokensShareresolves against the windowapplyMemoryFlushSharesToPlanpreserves a plan unchanged when no share is set,applies
softThresholdTokensShare/reserveTokensFloorShare, and scalescorrectly across 8k ↔ 1M windows
resolveShareBasedTokenBudgethandles the full share/absolute/fallback latticeTest plan
pnpm run config:schema:gen— schema.base.generated.ts regenerated cleanlynode scripts/run-vitest.mjs run --config test/vitest/vitest.agents.config.ts src/agents/— 1343 passnode scripts/run-vitest.mjs run --config test/vitest/vitest.auto-reply-reply.config.ts src/auto-reply/reply/— 1061 passnode scripts/run-vitest.mjs run --config test/vitest/vitest.config.ts src/config/— 2080 passnode scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/— 36 passnode scripts/run-vitest.mjs run --config test/vitest/vitest.extension-memory.config.ts— 564 pass, 3 skippedpnpm tsgo— no new type errors in touched files (pre-existing unrelated errors in whatsapp/ui remain)pnpm lint— no new oxlint errors in touched files