Skip to content

fix(core): require integer compaction counts - #5646

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/compaction-count-integers
Jun 23, 2026
Merged

fix(core): require integer compaction counts#5646
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/compaction-count-integers

Conversation

@tt-a1i

@tt-a1i tt-a1i commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Requires integer values for compaction tuning knobs that are used as counts: recent file retention, recent image retention, and the screenshot trigger threshold. Fractional and unsafe integer values now fall through to the next source in the normal env > settings > default order.

Why it's needed

These knobs are documented and consumed as counts, but resolveCompactionTuning previously accepted any finite number above the minimum. A value like 1.5 could therefore reach collectors that compare against integer lengths, effectively retaining 2 items instead of rejecting the invalid count. Env strings also need a lexical integer check before numeric conversion so huge fractional values cannot be rounded into an apparently safe integer.

Reviewer Test Plan

How to verify

Confirm that resolveCompactionTuning accepts valid integer values, rejects fractional count-like env/settings values, rejects unsafe integers, and still preserves the existing fallback order from env to settings to defaults.

Commands run locally:

npx vitest run src/services/compactionInputSlimming.test.ts --coverage.enabled=false
npx prettier --check packages/core/src/services/compactionInputSlimming.ts packages/core/src/services/compactionInputSlimming.test.ts
npx eslint packages/core/src/services/compactionInputSlimming.ts packages/core/src/services/compactionInputSlimming.test.ts
git diff --check
npm run build --workspace @qwen-code/qwen-code-core
npm run typecheck --workspace @qwen-code/qwen-code-core

Evidence (Before & After)

Before: fractional values such as 1.5 were accepted for count-like compaction tuning and could change the effective retention count. After: fractional and unsafe integer values are treated as invalid and fall through to settings/defaults. The new tests cover fractional env values for all three count-like knobs, a huge fractional env value that would otherwise be rounded by Number(), fractional settings values, and unsafe integers.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Local macOS arm64, Node.js v26.3.0.

Risk & Scope

  • Main risk or tradeoff: Low; this only tightens invalid count-like tuning values and keeps valid integer values unchanged.
  • Not validated / out of scope: Windows and Linux were not tested locally; CI should cover those platforms.
  • Breaking changes / migration notes: No expected breaking change for valid configs. Fractional count values now fall back instead of being accepted.

Linked Issues

Fixes #5640

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

中文说明

What this PR does

这个 PR 要求用于“计数”的 compaction tuning 必须是整数,包括最近文件保留数量、最近图片保留数量,以及截图触发阈值。小数和不安全整数现在会按正常的 env > settings > default 顺序回落到下一个来源。

Why it's needed

这些配置项在文档和下游逻辑里都是作为数量使用的,但之前 resolveCompactionTuning 会接受任何大于等于下限的有限数字。因此像 1.5 这样的值可以进入和整数长度比较的 collector,实际效果会变成保留 2 个条目,而不是拒绝这个非法数量。env 字符串也需要在数字转换前先做整数字面量校验,避免超大的小数字符串被 Number() 舍入成看起来安全的整数。

Reviewer Test Plan

How to verify

确认 resolveCompactionTuning 会接受合法整数,拒绝 count-like env/settings 小数,拒绝不安全整数,并保留现有 env 到 settings 再到默认值的回落顺序。

本地运行过的命令:

npx vitest run src/services/compactionInputSlimming.test.ts --coverage.enabled=false
npx prettier --check packages/core/src/services/compactionInputSlimming.ts packages/core/src/services/compactionInputSlimming.test.ts
npx eslint packages/core/src/services/compactionInputSlimming.ts packages/core/src/services/compactionInputSlimming.test.ts
git diff --check
npm run build --workspace @qwen-code/qwen-code-core
npm run typecheck --workspace @qwen-code/qwen-code-core

Evidence (Before & After)

修复前:1.5 这类小数会被 count-like compaction tuning 接受,并可能改变实际保留数量。修复后:小数和不安全整数都会被视为非法值,并回落到 settings/defaults。新增测试覆盖了三个 count-like 配置项的小数 env 值、一个会被 Number() 舍入的超大小数 env 值、小数 settings 值,以及不安全整数。

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

本地环境为 macOS arm64,Node.js v26.3.0。

Risk & Scope

  • 主要风险或取舍:风险较低;这个 PR 只收紧非法的 count-like tuning 值,合法整数值保持不变。
  • 未验证或不在范围内:本地没有测试 Windows 和 Linux;这些平台交给 CI 覆盖。
  • 破坏性变更或迁移说明:对合法配置没有预期破坏性影响。小数 count 值现在会回落,而不是被接受。

Linked Issues

Fixes #5640

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

@tt-a1i
tt-a1i marked this pull request as ready for review June 22, 2026 14:25
@wenshao

wenshao commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

On direction: this is a clean bug fix — resolveCompactionTuning shouldn't accept 1.5 as a count when downstream collectors compare against integer lengths. The linked issue (#5640) is well-scoped and the fix is squarely within core engine correctness. No direction concerns.

On approach: the diff is minimal and focused — adds an integer flag to the shared resolveNumber helper, threads it through the three count-like knobs, and adds targeted tests. The lexical pre-check on env strings (/^\d+$/) before Number() conversion is the right call to prevent large fractional strings from being silently rounded. No scope creep, no drive-by refactors.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:这是一个干净的 bug 修复——当下游 collector 和整数长度做比较时,resolveCompactionTuning 不应该接受 1.5 这样的值作为数量。关联的 issue (#5640) 范围明确,修复完全在核心引擎的正确性范畴内。方向没有问题。

方案:diff 很小且专注——给共享的 resolveNumber 添加 integer 标志,传递给三个 count-like 配置项,并增加了有针对性的测试。在 Number() 转换之前对 env 字符串做词法预检(/^\d+$/)是正确的做法,可以避免超大 fractional 字符串被静默舍入。没有范围蔓延,也没有夹带无关改动。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

This is a clean, minimal fix. The approach — adding an integer option flag to the shared resolveNumber helper — is exactly what I'd have done. The lexical pre-check on env strings (/^\d+$/) before Number() conversion is the right call: it prevents "9007199254740990.5" from being silently rounded to 9007199254740990 and then accepted as a safe integer. The isValid helper consolidates finiteness, integrality, and minimum checks without over-abstracting. JSDoc was updated to explain the new constraint. No scope creep, no drive-by refactors.

No blockers or concerns.

Testing

Unit Tests

All 35 tests in compactionInputSlimming.test.ts pass, including the 3 new test cases covering fractional env values, fractional settings values, and unsafe integers.

 RUN  v3.2.4 .../packages/core

 ✓ src/services/compactionInputSlimming.test.ts (35 tests) 15ms

 Test Files  1 passed (1)
      Tests  35 passed (35)
   Duration  382ms

Before / After

Demonstration of the bug and fix — same inputs, old vs new validation logic:

=== Compaction Count Validation: Before vs After ===

Test Case                      | BEFORE (bug)   | AFTER (fix)    | Verdict
-------------------------------------------------------------------------------------
fractional env "1.5"           | 1.5            | 4              | BUG FIXED
fractional env "2.5"           | 2.5            | 5              | BUG FIXED
huge fractional env            | 9007199254740990 | 6              | BUG FIXED
fractional settings 1.5        | 1.5            | 5              | BUG FIXED
unsafe integer env             | 9007199254740992 | 4              | BUG FIXED
valid integer env "10"         | 10             | 10             | unchanged
valid settings 7               | 7              | 7              | unchanged

All 5 invalid-value scenarios now correctly fall through to settings/defaults. Valid integer values continue to work as before.

Typecheck

npx tsc --noEmit  →  clean (exit 0)
中文说明

代码审查

这是一个干净、最小化的修复。在共享的 resolveNumber 辅助函数上添加 integer 选项标志的做法和我自己会选择的方案一致。对 env 字符串做词法预检(/^\d+$/)再调 Number() 是正确的:可以避免 "9007199254740990.5" 被静默舍入为 9007199254740990 后被当作安全整数接受。isValid 辅助函数将有穷性、整数性和最小值校验合在一起,没有过度抽象。JSDoc 也更新了解释新约束。没有范围蔓延,没有夹带无关改动。

没有阻塞问题。

测试

单元测试

compactionInputSlimming.test.ts 全部 35 个测试通过,包括新增的 3 个用例:fractional env 值、fractional settings 值和不安全整数。

Before / After

同样的输入,旧逻辑 vs 新逻辑的对比演示:

  • 5 个非法值场景现在正确回落到 settings/defaults
  • 合法整数值和之前一样正常工作

类型检查

npx tsc --noEmit → 无错误(exit 0)

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back: this is a textbook bug fix. The problem is real (fractional counts silently changing retention behavior), the fix is minimal (one flag, one helper, three call sites), and the evidence is clean (35/35 tests, typecheck passes, before/after demo confirms all 5 edge cases).

The approach matches what I'd have written independently. The lexical pre-check on env strings is a nice touch — it catches the "9007199254740990.5"9007199254740990 rounding trap before Number() can do damage. The fallback chain (env → settings → default) is preserved correctly for both valid and invalid inputs. No backward compatibility concerns since integer defaults to false.

No reservations. Approving. ✅

中文说明

退后一步看:这是一个教科书式的 bug 修复。问题是真实存在的(小数 count 值会静默改变保留行为),修复是最小化的(一个标志、一个辅助函数、三个调用点),证据是干净的(35/35 测试通过、类型检查通过、before/after 演示确认了全部 5 个边界场景)。

方案和我的独立提案一致。对 env 字符串的词法预检是个很好的处理——在 Number() 有机会造成损害之前捕获了 "9007199254740990.5"9007199254740990 的舍入陷阱。合法和非法输入的回落链(env → settings → default)都正确保留。因为 integer 默认为 false,没有向后兼容性问题。

没有顾虑。批准。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification — real runtime A/B build (Linux)

Verdict: verified, looks good to merge. The bug is real and reproduces end-to-end; the fix corrects every case without changing any valid configuration; the three new tests have teeth.

I didn't just run the unit suite — I built the PR head and its parent commit into two real dist artifacts and drove the actual compiled code path the app uses in a tmux session:

process.env / settings
  → resolveCompactionTuning()                         (compactionInputSlimming.js)
  → { maxRecentFiles, maxRecentImages, screenshotTriggerThreshold }
  → extractRecentFilePaths() / extractRecentImages()  (postCompactAttachments.js)

That last hop is the real harm: extractRecentFilePaths returns once seen.size >= maxFiles, so a fractional 1.5 collects 1 (1 >= 1.5 false), collects a 2nd, then stops (2 >= 1.5 true) — 2 files restored instead of the intended 1. Exactly the "retaining 2 items" claim in the PR description, observed on the shipped collector.

Bug cases — BEFORE (parent) wrong → AFTER (this PR) correct

Scenario BEFORE AFTER
B env QWEN_COMPACT_MAX_RECENT_FILES=1.5, settings 4 resolves 1.52 files restored 🐞 resolves 44 files
C env QWEN_COMPACT_MAX_RECENT_IMAGES=2.5, settings 5 resolves 2.53 images restored 🐞 resolves 55 images
D env QWEN_COMPACT_SCREENSHOT_THRESHOLD=9007199254740990.5, settings 6 Number() rounds → 9007199254740990 → screenshot trigger effectively disabled 🐞 rejected → 6
E env QWEN_COMPACT_MAX_RECENT_FILES=9007199254740992 (MAX_SAFE_INTEGER+1), settings 4 accepted → tries to restore 9e15 → 8 files (all available) 🐞 rejected → 4 files
F settings maxRecentFilesToRetain=1.5, no env resolves 1.52 files restored 🐞 rejected → default 55 files

Regression guard — valid configs are byte-for-byte identical before & after

Scenario BEFORE == AFTER
A env 2, settings 4 2 files (env wins)
G env 3, settings 7 3 files (env > settings)
H no env, settings 6 6 images (settings used)
I neither env nor settings 5 files (default)

The env > settings > default order is preserved.

Tests, teeth, and static checks (all on Linux)

Check Result
vitest run compactionInputSlimming.test.ts 35/35 pass
Mutation test — revert only the source fix, keep the new tests exactly the 3 new tests fail, other 32 pass → tests pin the fix
Consumer suites chatCompressionService + postCompactAttachments 138/138 pass — no regression
prettier --check (both files) clean
eslint (both files) clean
tsc --noEmit (core) clean
git diff --check (PR diff) clean

One non-blocking note for awareness

The new env parsing uses a strict ^\d+$ lexical check, so a few exotic strings that Number() previously accepted now fall through to settings/default. Verified delta (env QWEN_COMPACT_MAX_RECENT_FILES, settings 4):

env string BEFORE AFTER
1e2 100 4 (falls through)
0x10 16 4 (falls through)
+5 5 4 (falls through)
5.0 5 4 (falls through)
6 (whitespace) 6 6 (trimmed, unchanged)
007 7 7 (unchanged)
3 3 3 (unchanged)

This is an intentional, safe tightening for count knobs (and there's a minor, defensible asymmetry: env "5.0" is rejected while a settings value of 5.0 is accepted, since Number.isSafeInteger(5.0) === true). Flagging only so it's a conscious decision, not a surprise.

Environment: Linux (kernel 6.12), Node v22.22.2, PR head 8f840a6b7, parent caad0ff2c. This covers the 🐧 Linux row the PR marked "not tested".

中文说明(点击展开)

✅ 本地验证 —— 真实运行时 A/B 构建(Linux)

结论:已验证,建议合并。 这个 bug 是真实存在并且能端到端复现的;修复修正了所有问题场景,同时不改变任何合法配置;新增的 3 个测试是有“咬合力”的(能真正卡住回归)。

我没有只跑单元测试,而是把 PR head 和它的父提交分别构建成两份真实的 dist 产物,并在 tmux 会话里驱动应用实际使用的已编译代码路径:

process.env / settings
  → resolveCompactionTuning()                         (compactionInputSlimming.js)
  → { maxRecentFiles, maxRecentImages, screenshotTriggerThreshold }
  → extractRecentFilePaths() / extractRecentImages()  (postCompactAttachments.js)

最后这一跳才是真正的危害所在:extractRecentFilePathsseen.size >= maxFiles 时返回,所以小数 1.5 会先收集 1 个(1 >= 1.5 为假),再收集第 2 个,然后才停止(2 >= 1.5 为真)—— 实际恢复了 2 个文件,而不是本应的 1 个。这正好印证了 PR 描述里“保留了 2 个条目”的说法,并且是在真实 collector 上观测到的。

Bug 场景 —— 修复前(父提交)错误 → 修复后(本 PR)正确

场景 修复前 修复后
B env QWEN_COMPACT_MAX_RECENT_FILES=1.5,settings 4 解析为 1.5恢复 2 个文件 🐞 解析为 44 个文件
C env QWEN_COMPACT_MAX_RECENT_IMAGES=2.5,settings 5 解析为 2.5恢复 3 张图片 🐞 解析为 55 张图片
D env QWEN_COMPACT_SCREENSHOT_THRESHOLD=9007199254740990.5,settings 6 Number() 舍入为 9007199254740990 → 截图触发器实际上被禁用 🐞 被拒绝 → 6
E env QWEN_COMPACT_MAX_RECENT_FILES=9007199254740992MAX_SAFE_INTEGER+1),settings 4 被接受 → 尝试恢复 9e15 个 → 8 个文件(全部可用) 🐞 被拒绝 → 4 个文件
F settings maxRecentFilesToRetain=1.5,无 env 解析为 1.5恢复 2 个文件 🐞 被拒绝 → 默认值 55 个文件

回归保护 —— 合法配置在修复前后完全一致

场景 修复前 == 修复后
A env 2,settings 4 2 个文件(env 优先)
G env 3,settings 7 3 个文件(env > settings)
H 无 env,settings 6 6 张图片(使用 settings)
I env 和 settings 都没有 5 个文件(默认值)

env > settings > default 的优先级顺序得以保留。

测试、咬合力与静态检查(全部在 Linux 上)

检查项 结果
vitest run compactionInputSlimming.test.ts 35/35 通过
变异测试 —— 只回退源码修复、保留新测试 恰好这 3 个新测试失败,其余 32 个通过 → 测试确实卡住了修复
消费方测试 chatCompressionService + postCompactAttachments 138/138 通过 —— 无回归
prettier --check(两个文件) 干净
eslint(两个文件) 干净
tsc --noEmit(core) 干净
git diff --check(PR diff) 干净

一个不阻塞合并的提醒

新的 env 解析使用了严格的 ^\d+$ 字面量校验,因此一些 Number() 之前能接受的特殊字符串,现在会回落到 settings/default。实测差异(env QWEN_COMPACT_MAX_RECENT_FILES,settings 4):

env 字符串 修复前 修复后
1e2 100 4(回落)
0x10 16 4(回落)
+5 5 4(回落)
5.0 5 4(回落)
6(带空格) 6 6(trim 后,不变)
007 7 7(不变)
3 3 3(不变)

对于“计数类”配置项来说,这是一个有意为之、且安全的收紧(另外有一个很小、且合理的不对称:env "5.0" 会被拒绝,而 settings 里的数值 5.0 会被接受,因为 Number.isSafeInteger(5.0) === true)。这里只是提示一下,确保这是一个有意识的决定,而不是意外。

环境: Linux(内核 6.12),Node v22.22.2,PR head 8f840a6b7,父提交 caad0ff2c。本次验证覆盖了 PR 中标记为“未测试”的 🐧 Linux 一行。

@wenshao
wenshao merged commit 83caca6 into QwenLM:main Jun 23, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): compaction count tuning accepts fractional values

3 participants