docs: CompactionConfig.keepTokens documented default value does not match source code - #3144
xingfudeshi wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Both language versions should clarify that triggerTokens: 0 enables dynamic thresholding rather than disabling token triggering.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates v2 Harness memory documentation to reflect current compaction defaults.
Changes:
- Sets
triggerTokensto0. - Sets
keepTokensto-1in English and Chinese documentation.
File summaries
| File | Summary |
|---|---|
docs/v2/en/docs/harness/memory.md |
Updates compaction defaults; clarify that triggerTokens: 0 enables dynamic thresholding. |
docs/v2/zh/docs/harness/memory.md |
Updates localized compaction defaults; clarify the dynamic threshold behavior. |
Review details
Suppressed comments (2)
docs/v2/en/docs/harness/memory.md:85
keepTokens = -1is not simply a non-zero fixed token budget: it selects dynamic mode, while0selectskeepMessagesand only values greater than zero use a fixed token budget. The current wording therefore misdescribes the documented default and can lead users to configure the wrong retention mode.
| `keepTokens` | `-1` | When non-zero, walk back by token budget; overrides `keepMessages` |
docs/v2/zh/docs/harness/memory.md:84
keepTokens = -1会进入动态模式,0才是按keepMessages保留,只有大于 0 才是固定 token 预算;当前“非 0”描述把默认值的语义写错了。请按这三种模式说明。
| `keepTokens` | `-1` | 非 0 时按 token 预算从尾部往前算,覆盖 `keepMessages` |
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Documentation-only fix in the v2 harness memory tables: CompactionConfig defaults are corrected to match CompactionConfig.Builder (triggerTokens = 0, keepTokens = -1), verified against the source at this head. The reported mismatch is real and the keepTokens correction is right.
Findings
- [Warning]
docs/v2/zh/docs/harness/memory.md:82— the zh row now shows the correct default (0) but still describes0as "关闭" (off), which is the exact misconception this PR removes; the en row in the same PR says0= dynamic mode. - [Info]
docs/v2/en/docs/harness/memory.md:85— with-1documented as the default, "when non-zero ... overrideskeepMessages" now describes the default case without saying that-1means "auto-compute fromkeepTokensRatio", i.e. that a configuredkeepMessagesis ignored unlesskeepTokens=0is set explicitly.
Suggestions
Align the zh triggerTokens description with en, and spell out the -1 = dynamic semantics in both languages, since that is the behaviour the issue is about. Non-blocking for a docs change — happy to re-review after the zh row is updated.
CI: build (ubuntu-latest) / build (windows-latest) still pending; Check License, Check Module Sync, validate, license/cla green.
Automated review by github-manager-bot
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the delta since my previous pass (f710bc84 → 23437775): only the two doc rows changed, and both non-blocking items from my earlier review are addressed — the zh triggerTokens row no longer says 0 = "关闭", and both languages now spell out the three-way keepTokens semantics (-1 dynamic / 0 = keepMessages / >0 fixed budget). Verified again against CompactionConfig.Builder at this head: triggerTokens = 0, keepMessages = 20, keepTokens = -1, and the -1 branch is documented in the javadoc as min(keepTokensMax, max(keepTokensMin, usable * keepTokensRatio)). Docs now match source in both languages.
One remaining nit is informational only: keepTokens dynamic mode is clamped by keepTokensMin/keepTokensMax (2_000/8_000) with keepTokensRatio = 0.25, so "auto-computed from the model context window" is an approximation. If a reader ever sets keepTokens(-1) expecting the whole window, the clamp is the surprise — a one-line mention would close that gap, but it does not block a docs fix.
Findings
- [Info]
docs/v2/en/docs/harness/memory.md:85— dynamic mode isusable * keepTokensRatioclamped to[keepTokensMin, keepTokensMax]; the table says only "auto-computed from the model context window". - [Info]
docs/v2/zh/docs/harness/memory.md:84— same wording gap in the zh row.
Verdict
Approving: the reported mismatch is fixed, the change is docs-only, and Check License / Check Module Sync / build (ubuntu-latest) / build (windows-latest) / validate / codecov/patch / license/cla are all green on 23437775.
Automated review by github-manager-bot
Problem
The documentation states keepTokens defaults to 0, but the actual source code (CompactionConfig.Builder) sets it to -1.
Impact
These two values produce completely different behavior:
0: determineCutoffIndex takes the message-based branch, keepMessages works as configured.
-1: Triggers dynamic mode. keepTokens is auto-computed from contextWindow * keepTokensRatio, which overrides keepMessages entirely — the user's keepMessages setting is silently ignored.
When users configure triggerMessages + keepMessages without explicitly setting keepTokens (relying on the documented default), keepMessages has no effect. Compaction barely reduces message count and re-triggers in a loop.
Fix
Correct the documented default value from 0 to -1, and clarify that -1 enables dynamic mode (auto-computed from model context window). To use keepMessages verbatim, set keepTokens=0 explicitly.