test(integration): assert successful /compress token reduction - #11259
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR — this one is easy to orient on, because it picks up a piece of work the maintainers explicitly asked someone to redo. Template looks good ✓ — every required heading is present and filled in with real content rather than placeholders. Problem: observed and documented, not theoretical. Direction: aligned. #11213's first bullet names exactly three things worth keeping from the abandoned half of #11094 — Size: not applicable — no core paths are touched. For the record, the split is 12 lines of test-infrastructure code ( Approach: the scope feels right, and I could not find a materially simpler path. Two observations worth putting on the table rather than blocking on:
Risk: no elevated risk signals. None of the changed files match the revert-correlated paths we track, and the only settings change is scoped to this suite's fixture, not to any product default. Moving on to code review. 🔍 中文说明感谢贡献 —— 这个 PR 很好定位，因为它接手的正是维护者明确要求重做的那部分工作。 模板完整 ✓ —— 所有必需小节都在，且填写的是真实内容而非占位符。 问题：是已观测且有记录的问题，不是理论性加固。 方向：一致。#11213 的第一条明确列出了 #11094 被放弃的那一半中值得保留的三样东西 —— 以"请求序号确定性"重新论证的 规模：不适用 —— 未触及核心路径。作为记录，行数构成是 12 行测试基础设施代码（ 方案：范围合理，我没有找到明显更简单的路径。有两点想摆出来讨论，但不构成阻塞：
风险：无升级风险信号。改动文件均未命中我们跟踪的与 revert 相关的路径，唯一的 settings 变更也只作用于本套件的测试配置，不影响任何产品默认值。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo critical blockers and no AGENTS.md violations. The diff does one thing, does it in the idiom the file already uses, and carries no drive-by edits. Before reading the diff I wrote down what I would do from the title and the "why" alone: add a synchronous telemetry accessor next to the existing last-api-request reader, assert The parts I checked rather than assumed:
One non-blocking note: Test evidenceThis is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI, read through the API for commit The leg that would actually exercise these tests is Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified, and why:
Sandboxed verification would settle the first gap: No real-scenario tmux capture on this run: that path is local-invocation only, and this change has no user-visible product surface to drive. 中文说明代码审查没有阻塞性问题，也没有违反 AGENTS.md 的地方。这个 diff 只做一件事，用的是文件里已有的写法，没有夹带顺手改动。 在读 diff 之前，我先只根据标题和"为什么需要"写下了自己的方案：在已有的"读取最后一条 api_request"方法旁边加一个同步的遥测读取方法；在已有的等待之后断言 以下是我实际核对过、而不是想当然的部分：
一条非阻塞的观察： 测试证据这是一次无人值守的 CI 运行，因此我没有构建或执行本 PR 的任何代码 —— 下面的证据来自 PR 自己的 CI，通过 API 读取提交 真正会跑到这些测试的是 未验证项及原因：
沙箱化验证可以补上第一个缺口： 本次运行没有真实场景的 tmux 抓取：该路径仅适用于本地调用，而且这个改动没有可供驱动的用户可见产品界面。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the change is right and I found nothing to object to; the missing point is CI on this commit, which had not landed when I reviewed. This is the unusual follow-up PR that arrives carrying its own mandate. #11213 is a maintainer-verified deferral out of #11094 that named exactly three survivors from the abandoned half — the memory setting re-justified as index determinism, a telemetry reader plus a real reduction gate, and a shared settings constant — and this diff is those three and nothing else. I went looking for scope creep, drive-by refactors, and the two inert timeout constants the deferral said to leave behind, and found none of them. The one place the author could have quietly widened the diff, folding the existing last-api-request reader into the new generic one, they correctly left alone. What I would normally be suspicious of in a PR like this is an assertion on numbers that move. That is handled the right way: the gate is relational rather than exact, and the volatile values go into the failure message. The gap between the author's observed My one real reservation is that nobody has yet seen the new gate fail on a genuine inflation, because the single case that would exercise it is still skipped — and this PR correctly declines to un-skip it, since that case predates the diff and needs its live-model path rethought. So the reservation is a follow-up, not a defect here: against main's presence-only assertion this is a strict improvement either way, and I named the sponsored On maintenance: I also checked whether I was being worn down by volume rather than judging merit — the author has two other open PRs, on unrelated surfaces, roughly a week apart. Nothing that reads as churn, and this one stands on its own evidence. Verdict: approve. CI on this commit was still running at review time ( 中文说明信心度：4/5 —— 改动是对的，我没有找到可反对之处；差的那一分是本次审查时该提交上的 CI 还没出结果。 这是一个少见的、自带授权的后续 PR。#11213 是从 #11094 遗留下来、经维护者核实过的遗留项，明确点名了被放弃的那一半中应当保留的三样东西 —— 以序号确定性重新论证的记忆开关、一个遥测读取方法加一条真实的缩减断言、以及一个共享的 settings 常量 —— 而这个 diff 正好是这三样，不多不少。我特意去找了范围蔓延、顺手重构，以及遗留项要求丢弃的那两个已失效的超时常量，都没有找到。作者本来可以悄悄扩大 diff 的那个地方（把已有的"读取最后一条 api_request"方法折进新的通用方法里），也正确地没有去动。 这类 PR 我通常会怀疑的一点，是对会变动的数值下断言。这里处理方式是对的：断言是关系型的而非精确值，且把易变的数值放进了失败信息。作者实测的 我唯一真正的保留意见是：还没有人见过这个新断言在真实 token 膨胀时失败，因为唯一能触发它的用例仍然是跳过状态 —— 而本 PR 正确地没有去取消跳过，因为那个用例早于本 diff，且需要重新设计其真实模型路径。所以这条保留意见属于后续工作，而不是本 diff 的缺陷：相比 main 上"只断言事件存在"的写法，无论如何这都是严格改进。我在上面的审查中也点名了由维护者担保的 关于后续维护： 我也检查了自己是否是被数量磨软了判断、而非基于实质：该作者另有两个开放 PR，主题互不相关，间隔约一周。看不出刷量迹象，本 PR 也是凭自身证据独立判断的。 结论：批准。审查时该提交上的 CI 仍在运行（ — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 readLastApiRequest / readTelemetryEvent duplication — already reported (comment 5565290448)
Not explored to full depth (tool budget reached): "agent 6a": did not execute the two /compress integration cases or the new test-helper.test.ts cases (needs npm run build && npm run bundle in this shared worktree); …; "agent 3b": none — every check above completed inside the tool budget (~16 of 33 calls).; "agent 1c": did not verify whether the managed-auto-dream background task ( getManagedAutoDreamEnabled() , packages/core/src/config/config.ts:8194-8198 , left enabled by ….
中文说明
已审查。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6a"：did not execute the two /compress integration cases or the new test-helper.test.ts cases (needs npm run build && npm run bundle in this shared worktree); …；"agent 3b"：none — every check above completed inside the tool budget (~16 of 33 calls).；"agent 1c"：did not verify whether the managed-auto-dream background task ( getManagedAutoDreamEnabled() , packages/core/src/config/config.ts:8194-8198 , left enabled by …。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| expect( | ||
| typeof tokensBefore === 'number' && | ||
| typeof tokensAfter === 'number' && | ||
| tokensBefore > tokensAfter, |
There was a problem hiding this comment.
[Suggestion] R1-7: This oracle means what it says only because the fake server reports zero usage, and nothing records that dependency. The /compress handler returns just { content }, so FakeOpenAIServer falls back to DEFAULT_USAGE (all zeros) and ChatCompressionService takes its local-estimator branch — which is the only reason tokens_before is an estimate of the seeded history at all. That makes the assertion fragile in both directions at once. Below one boundary, a maintainer who makes the fixture "realistic" by adding a usage block reddens two healthy cases with a message that names product token accounting while the real cause is a fixture field two files away. Above it, the oracle goes vacuous: tokens_before becomes the fixture's own declared number and tokens_after the estimator's count for the fixed 66-character summary, so shrinking the seeded model response a hundredfold moves neither value and the assertion reduces to comparing two fixture constants. That edit idiom is common in this tree — four of the five distinct prompt_tokens literals under integration-tests/ (20, 24, 32, 50) sit below the measured boundary of 71, so copying the nearest sibling breaks both cases. To be clear about the present state: the committed oracle is sound today. It is genuinely seed-sensitive, and it does catch a real inflated summary, so this is a latent fragility rather than a live defect — but it is one field-edit away, and the failure message would point the next debugger at the wrong subsystem.
Witness:
QWEN_SANDBOX=false npx vitest run --root ./integration-tests \
./interactive/context-compress-interactive.test.ts
CONTROL (as committed): Tests 2 passed | 1 skipped
tokens_before=1864 tokens_after=71 input=0 output=0 cache_sharing=False
BELOW the boundary -- fixture usage {prompt_tokens: 20, completion_tokens: 5}:
Tests 2 failed | 1 skipped
AssertionError: chat_compression event recorded no token reduction:
tokens_before=20, tokens_after=71, output=5, cache_sharing=true: expected false to be true
boundary sweep: 20 RED 50 RED 100 GREEN 1500/200 GREEN
ABOVE the boundary -- the oracle stops measuring the conversation:
A usage 100/20, seed INTACT -> 2 passed, tokens_before=100 tokens_after=71
B usage 100/20, seed shortened 100x -> 2 passed, tokens_before=100 tokens_after=71
^ bit-identical to A: the seed no longer matters
D usage 100/20, seed turn DELETED -> 1 failed, 'telemetry event was not found'
^ so the vacuity is the seed's magnitude, not its existence
F committed zero-usage + seed shortened -> 2 passed, tokens_before=1663 (was 1864)
^ the committed oracle IS seed-sensitive
Bounding arm -- does the gate bite on a genuine inflation?
E1 '<state_snapshot>' padded with 'padded '.repeat(2000):
Tests 2 failed | 1 skipped
AssertionError: chat_compression event recorded no token reduction:
tokens_before=1864, tokens_after=3571, output=0, cache_sharing=false: expected false to be true
The cheapest half of the fix is to record the dependency where the oracle lives, and to make the zeros deliberate rather than inherited:
// The reduction below is only meaningful because the fake server reports zero
// usage (DEFAULT_USAGE), which sends ChatCompressionService down its
// local-estimator branch so tokens_before estimates the seeded history.
// Any non-zero usage changes what tokens_before means: below ~71 prompt tokens
// both cases fail as INFLATED, and above it tokens_before is just the fixture's
// own number and the assertion stops measuring the conversation.plus usage: { prompt_tokens: 0, completion_tokens: 0, total_tokens: 0 } in the handler, and optionally an assertion on the two attributes that actually identify the branch (compression_input_token_count === 0, cache_sharing_used === false).
The boundary to respect is originalTokenCount >= 71 — the estimator's count for the fixture's fixed 66-character <state_snapshot> string — not the cold-path compressionInputTokenCount - 1000 arithmetic at chatCompressionService.ts:1205-1208, which is never reached here because a non-zero provider count also flips hasProviderTokenCount true (:800-802), takes the cache-sharing path (:808, :889) and routes into the estimator at :1243; note too that usage: message.usage ?? DEFAULT_USAGE (fake-openai-server.ts:420-424, consumed at :392) makes explicit zeros byte-identical to today's wire bytes.
If you add the branch-identifying assertion rather than only the comment, it must go red on the vacuity arm and stay green as committed — measured both ways: green on the committed fixture, and oracle no longer measures the seeded history ... tokens_before=100 on the prompt_tokens: 100 arm with a shortened seed.
中文说明
这条断言之所以成立，完全是因为假服务器返回的 usage 全为零，而这个依赖没有任何地方记录下来。/compress 的处理函数只返回 { content }，于是 FakeOpenAIServer 回退到 DEFAULT_USAGE（全零），ChatCompressionService 走进本地估算分支 —— 这也正是 tokens_before 会成为种子历史估算值的唯一原因。这让断言在两个方向上同时变脆。在某个边界之下，维护者若为了让 fixture「更真实」而加上 usage，两个本来健康的用例就会变红，而报错信息指向的是产品的 token 计算，真正的原因却是两个文件之外的一个 fixture 字段。在边界之上，断言会变成空转：tokens_before 变成 fixture 自己声明的数字，tokens_after 变成估算器对固定 66 字符摘要的计数，于是把种子模型响应缩短一百倍两个数字都不变，断言退化为比较两个 fixture 常量。这种改法在本仓库里很常见 —— integration-tests/ 下五个不同的 prompt_tokens 字面量中有四个（20、24、32、50）都低于实测的 71 边界，所以照抄最近的同类文件就会让两个用例都失败。需要说明当前状态：已提交的断言今天是可靠的。它确实对种子敏感，也确实能抓到真实的摘要膨胀，所以这是一个潜在脆弱性而不是现存缺陷 —— 但距离触发只差一次字段修改，而且失败信息会把下一个排查的人指向错误的子系统。
最省事的修法是把依赖记录在断言所在处，并把「零」变成显式选择而不是继承来的默认值（见上方英文代码块中的注释与显式 usage 写法），也可以再加上真正标识该分支的两个属性断言（compression_input_token_count === 0、cache_sharing_used === false）。
需要尊重的边界是 originalTokenCount >= 71 —— 即估算器对 fixture 固定 66 字符 <state_snapshot> 字符串的计数 —— 而不是 chatCompressionService.ts:1205-1208 中冷路径的 compressionInputTokenCount - 1000 算式；后者在这里根本不会走到，因为非零的 provider 计数同时会把 hasProviderTokenCount 置为真（:800-802），走缓存共享路径（:808、:889），并在 :1243 转入估算分支。另外，usage: message.usage ?? DEFAULT_USAGE（fake-openai-server.ts:420-424，在 :392 被消费）意味着显式写零与今天的实际字节完全一致。
如果选择加分支标识断言而不只是加注释，那么它必须在空转那一侧变红、在已提交状态下保持绿色 —— 两个方向都已实测：committed fixture 下绿色，prompt_tokens: 100 且种子缩短时输出 oracle no longer measures the seeded history ... tokens_before=100。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // Earlier unit tests cover prompt composition; this pins the end-to-end | ||
| // argument path and a successful compression result. |
There was a problem hiding this comment.
[Suggestion] R1-1: This rewrite strengthens the comment to say the test "pins the end-to-end argument path", but nothing in the test observes the argument. The fake-server handler dispatches on requestIndex alone and never reads the request body, and the case's only assertions are telemetry-event presence plus the new token reduction — both satisfied by any successful compression. So if a future change stops forwarding the trailing argument into the compression system prompt, this case stays green while the behaviour its own name advertises (should forward /compress instructions through to the side-query) is broken, and the comment now tells the next reader the path is covered, which is what stops someone adding the assertion. One correction that bounds the cost, because it matters: this would not be an undetected regression. Both hops are pinned elsewhere — compressCommand.test.ts:346-367 asserts 'focus on auth bug' reaches tryCompressChat, and chatCompressionService.test.ts:4384-4406 asserts the Additional Instructions: block is built. What is actually wrong is narrower: a comment in this file records coverage this file does not have. Either add the assertion or narrow the comment.
Witness:
grep -n "focus on the scientist\|requests\|body" \
integration-tests/interactive/context-compress-interactive.test.ts
-> exactly one hit, line 210, the keystroke itself:
await type(ptyProcess, '/compress focus on the scientist mentioned');
-> zero of the file's 10 expect() calls reference fakeServer.requests or a request body
handler discards the body:
({ requestIndex }) => ({ content: requestIndex === 0 ? ... : '<state_snapshot>...' })
witness (mutation arm): not run — the mutation lives in production source under
packages/cli and packages/core; the scratch tree's unit harness stops at its own
build guard (missing packages/acp-bridge/dist/index.js and 12 more) and the
interactive case spawns dist/cli.js, which the scratch tree does not have.
Survival rests on the sweep above plus the reply being index-keyed and body-independent.
The fake server already records what it received, and siblings assert on it (file-system-interactive.test.ts:137, mid-turn-submit-interactive.test.ts:270). Searching rather than indexing keeps the assertion from inheriting the request-index premise:
expect(
fakeServer!.requests.some((r) =>
JSON.stringify(r.body).includes('focus on the scientist mentioned'),
),
'the /compress instruction never reached the side-query',
).toBe(true);Assert on the instruction substring rather than the assembled prompt shape or a byte offset: the instruction travels in the side-query's system prompt as an Additional Instructions: block (chatCompressionService.ts:377) and the CLI clips it with rawArgs.slice(0, MAX_COMPRESS_INSTRUCTIONS_CHARS), cap 2000 (compressCommand.ts:71-72, itself pinned by compressCommand.test.ts:425-437).
Whichever way you resolve it, the new body assertion must go red when the instruction stops reaching the side-query — drop customInstructions at compressCommand.ts:89 or parts.push(userInstructions.trim()) at chatCompressionService.ts:371 and confirm it fails; neither mutation reddens any assertion in this file today.
中文说明
这次改写把注释强化为「pins the end-to-end argument path」（钉住端到端参数路径），但测试里没有任何地方观察这个参数。假服务器的处理函数只按 requestIndex 分派，从不读取请求体；而该用例仅有的断言是遥测事件存在，加上新增的 token 缩减 —— 任何一次成功的压缩都能同时满足这两条。因此如果将来有改动不再把尾随参数转发进压缩系统提示词，这个用例仍然会是绿色，而它自己名字所声称的行为（should forward /compress instructions through to the side-query）已经坏了；更糟的是注释现在会告诉后来的读者这条路径已被覆盖，从而阻止别人补上断言。有一个限定代价的更正很重要：这不会是一个无人察觉的回归。两处环节都已被别处钉住 —— compressCommand.test.ts:346-367 断言 'focus on auth bug' 到达了 tryCompressChat，chatCompressionService.test.ts:4384-4406 断言 Additional Instructions: 块被构造出来。真正有问题的地方更窄：本文件里的注释记录了本文件并不具备的覆盖。要么补上断言，要么把注释收窄。
假服务器已经记录了它收到的内容，同类测试也是这样断言的（file-system-interactive.test.ts:137、mid-turn-submit-interactive.test.ts:270）。用查找而不是固定序号，可以避免断言继承请求序号这个前提（见上方英文代码块）。
请断言指令子串，而不是拼装后的提示词形状或字节偏移：指令是以 Additional Instructions: 块的形式进入侧查询的系统提示词（chatCompressionService.ts:377），并且 CLI 会用 rawArgs.slice(0, MAX_COMPRESS_INSTRUCTIONS_CHARS) 截断，上限 2000（compressCommand.ts:71-72，该上限本身由 compressCommand.test.ts:425-437 钉住）。
无论选择哪种处理方式，新增的请求体断言都必须在指令不再到达侧查询时变红 —— 去掉 compressCommand.ts:89 的 customInstructions，或去掉 chatCompressionService.ts:371 的 parts.push(userInstructions.trim())，确认它失败；今天这两个变异都不会让本文件任何断言变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| rig.createFile( | ||
| 'telemetry.log', |
There was a problem hiding this comment.
[Suggestion] R1-6: This fixture writes compact one-record-per-line JSON, but the exporter that produces a real telemetry.log writes indent-2 pretty-printed records — so the rig's only fast unit coverage of the record-splitting path is keyed to a format production never emits. FileExporter.serialize is safeJsonStringify(data, 2) + '\n', and a real log from this very suite measured 1870 lines across 20 records. _readAndParseTelemetryLog's split(/}\n{/) brace reassembly exists precisely for that multi-line shape. Both shapes parse today, so the tests are green — but that is the problem: replacing the reassembly with line-based JSONL survives this fixture while returning null for every record of a real log. The fixture therefore blesses exactly the simplification that would break every consumer of the parser (waitForTelemetryEvent, readLastApiRequest, readToolLogs, readMetric), and defers the catch to the slow E2E suite. To be fair about severity, the mutation is not silent at suite level — those consumers read real logs and the slow cases would fail — and the parser itself is pre-existing unchanged code. The gap is specifically that a reviewer simplifying the parser would see this fast fixture stay green, and grep confirms it is the only createFile('telemetry.log', ...) fixture in integration-tests/.
Witness:
production shape, read:
packages/core/src/telemetry/file-exporters.ts:45-47
protected serialize(data: unknown): string {
return safeJsonStringify(data, 2) + '\n';
}
real log from this suite (kept run, interactive-compress-test/telemetry.log):
{"firstLineOfFile":"{","fileLineCount":1870,"recordCount":20}
INTACT parser, real log -> {"eventName":"qwen-code.chat_compression",
"tokens_before":1845,"tokens_after":71}
MUTANT line-based JSONL, PR fixture -> Tests 9 passed (9) <- mutation SURVIVES
MUTANT line-based JSONL, same real log -> {"recordCount":120,"recordsWithoutAttributes":120}
readTelemetryEvent('chat_compression') -> {"returnedNull":true} <- breaks every consumer
fixture emitted as JSON.stringify(obj, null, 2):
INTACT -> Tests 9 passed (9) <- fix is safe
MUTANT -> FAILED expected undefined to be 28891 <- fix kills the mutant
Emitting each record with the exporter's own shape is a fixture-only change — build them as JSON.stringify(obj, null, 2) and keep the .join('\n'). The indent must match serialize's safeJsonStringify(data, 2) (packages/core/src/telemetry/file-exporters.ts:45-47), and the parser's content.split(/}\n{/) at test-helper.ts:788 must keep working for both shapes.
With the pretty-printed fixture, replacing the split(/}\n{/) reassembly in _readAndParseTelemetryLogwith per-lineJSON.parsemust turn the latest-match case red — measured:expected undefined to be 28891`. Against today's compact fixture that same mutation stays green.
中文说明
这个固定数据写的是紧凑的「一行一条记录」JSON，而真正生成 telemetry.log 的导出器写的是缩进 2 的美化多行记录 —— 因此测试夹具中唯一快速覆盖记录切分路径的单测，绑定的是一个生产环境永远不会输出的格式。FileExporter.serialize 是 safeJsonStringify(data, 2) + '\n'，而本套件产生的一份真实日志实测为 1870 行、20 条记录。_readAndParseTelemetryLog 里的 split(/}\n{/) 补括号逻辑正是为这种多行形状而存在。两种形状今天都能解析，所以测试是绿色的 —— 但问题恰恰在这里：把补括号逻辑换成按行 JSONL 解析，在这个固定数据上依然通过，而对真实日志的每一条记录都返回 null。于是这份固定数据恰好为那个会破坏解析器所有消费者（waitForTelemetryEvent、readLastApiRequest、readToolLogs、readMetric）的「简化」背书，并把发现时机推迟到慢速 E2E 套件。为公允地判断严重程度：这个变异在套件层面并不是无声的 —— 那些消费者读取真实日志，慢速用例会失败 —— 而且解析器本身是本次未改动的既有代码。缺口具体在于：一个想简化解析器的审查者会看到这个快速 fixture 仍然绿色，而 grep 确认它是 integration-tests/ 中唯一一处 createFile('telemetry.log', ...) 固定数据。
按导出器自身的形状生成每条记录只涉及固定数据本身的改动 —— 用 JSON.stringify(obj, null, 2) 构造，并保留 .join('\n')。缩进必须与 serialize 的 safeJsonStringify(data, 2) 一致（packages/core/src/telemetry/file-exporters.ts:45-47），并且 test-helper.ts:788 的 content.split(/}\n{/) 必须对两种形状都继续有效。
改用美化格式后，把 _readAndParseTelemetryLog 中的 split(/}\n{/) 补括号逻辑换成逐行 JSON.parse，必须让「最新匹配」用例变红 —— 实测输出 expected undefined to be 28891。而在今天的紧凑固定数据下，同一个变异仍然绿色。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| expect(event?.attributes?.['tokens_before']).toBe(28891); | ||
| expect(event?.attributes?.['tokens_after']).toBe(27128); |
There was a problem hiding this comment.
[Suggestion] R1-4: Neither new test pins the reader's exact event-name match. The interleaved api_request records prove that non-matching names are skipped, but no record's name extends the queried one — so the exactness the method's contract depends on has no witness. That matters because the relaxation is a natural future edit: letting callers pass a fully-qualified name means changing === to startsWith, and that mutant survives both tests and typechecks clean, so it could ship past the gate CI runs. The telemetry namespace already contains prefix-colliding pairs, so the consequence is concrete rather than hypothetical — readTelemetryEvent('slash_command') would silently start returning slash_command.model events, and a caller would assert on the wrong event's attributes with nothing failing anywhere. Note that neither the method name nor its comment says "exact"; only the PR description does. The prior review on this PR traced a different mutant — deleting the name predicate, which this fixture does kill — so this gap is a distinct one.
Witness:
vitest run --root ./integration-tests ./test-helper.test.ts (baseline: 9 passed)
arm | new unit tests | probe: readTelemetryEvent('slash_command')
INTACT ===, PR fixture | Tests 9 passed (9) | {"returnedNull":true}
MUTANT ?.startsWith, PR fixture | Tests 9 passed (9) SURVIVES | {"returnedNull":false,
"eventName":"qwen-code.slash_command.model"}
MUTANT + proposed .details record | FAILED expected 1 to be 28891 | -
INTACT + proposed .details record | Tests 9 passed (9) | -
npm run typecheck:integration under the mutant: no test-helper.ts error
-> the relaxation is type-admissible, so no gate catches it
colliding pairs verified at HEAD:
packages/core/src/telemetry/constants.ts:31 EVENT_SLASH_COMMAND = 'qwen-code.slash_command'
packages/core/src/telemetry/constants.ts:49 EVENT_MODEL_SLASH_COMMAND = 'qwen-code.slash_command.model'
packages/core/src/telemetry/constants.ts:89 'qwen-code.memory.recall'
packages/core/src/telemetry/constants.ts:90 'qwen-code.memory.recall.delivery'
Appending one record as the last line whose name extends the queried one gives exactness a witness at no cost to correct code:
JSON.stringify({
attributes: {
'event.name': 'qwen-code.chat_compression.details',
tokens_before: 1,
tokens_after: 0,
},
}),The new record must carry the full qwen-code. prefix, because the filter builds the name as `qwen-code.${eventName}` (test-helper.ts:900), matching waitForTelemetryEvent (:559) and readMetric (:912).
The latest-match case must then go red when === at test-helper.ts:900 becomes .startsWith(...); today that mutation is invisible (measured: 9 passed either way).
中文说明
两个新测试都没有钉住读取方法的精确事件名匹配。中间插入的 api_request 记录只能证明不匹配的名字会被跳过，但没有任何一条记录的名字延伸了被查询的名字 —— 于是该方法契约所依赖的「精确性」没有任何证据。这一点之所以重要，是因为放宽匹配是一个很自然的后续改动：想让调用方可以传入完整限定名，就要把 === 改成 startsWith，而这个变异体在两个测试中都能存活，并且类型检查也干净通过，因此可能绕过 CI 所运行的关卡直接进入代码库。遥测命名空间里本来就存在前缀冲突的事件对，所以后果是具体的而不是假设性的 —— readTelemetryEvent('slash_command') 会静默开始返回 slash_command.model 事件，调用方于是对错误事件的属性下断言，而任何地方都不会报错。需要注意的是，方法名和它的注释都没有写明「精确」，只有 PR 描述写了。本 PR 之前的审查推演的是另一个变异体 —— 删除事件名条件（这个固定数据确实能抓到），所以这里的缺口是另一个不同的问题。
在数组最后追加一条名字延伸了被查询名字的记录，就能为精确性提供证据，且对正确代码没有任何代价（见上方英文代码块）。
新记录必须带上完整的 qwen-code. 前缀，因为过滤条件是按 `qwen-code.${eventName}` 构造名字的（test-helper.ts:900），与 waitForTelemetryEvent（:559）和 readMetric（:912）一致。
加上之后，把 test-helper.ts:900 的 === 改成 .startsWith(...) 必须让「最新匹配」用例变红；今天这个变异是不可见的（实测两种情况都是 9 passed）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Verification report at head No tmux e2e run, deliberately. This PR is zero production code: all three changed files sit under What the new assertions actually pin
CI, verified by lane name rather than by rollup stateComplete census at this head: 121/121 check-runs fetched ( Approval leg: One coverage fact worth recording — not a blockerThe green That is a gap in lane coverage, not a defect in the change: the assertion reads correctly, it is the strengthening the PR set out to make, and nothing here can affect shipped behaviour. It does mean the new FindingsNo Critical from me. 中文说明在 head 未做 tmux e2e，是有意为之。 本 PR 零生产代码：三个改动文件全部位于 **新断言实际钉住了什么：**① CI 按 lane 名核验，而非按 rollup 状态。 本 head 完整普查 121/121 条 check-run（已断言 一条值得记录、但不构成阻塞的覆盖事实。 绿灯的 结论： 我没有发现 Critical。ci-bot 在本 head 的评审（ |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approve at head 083a94bd03d583e6d0290595d07a93b0ff5dc0c3.
Verification report above; its conclusion is mergeable. Zero production code (all three files under integration-tests/), so the report cites the harness's own executed assertions rather than a tmux run. Complete CI census 121/121 with 0 non-green live lanes by lane name; qwen-code-ci-bot APPROVED anchored at this head with a +3102 s re-anchor delta. No Critical from me, and ci-bot's 5 at-head findings are all Suggestion-level by its own ledger.
One non-blocking coverage note is recorded in the report: the headline expectSuccessfulCompression() in interactive/context-compress-interactive.test.ts is not in the no-AK lane's explicit file list, so it is typechecked but not executed by this PR's CI.
4 participants
What this PR does
/compressfake-server fixture deterministic by disabling managed auto-memory, so the side-query remains at the expected request index./compressend-to-end cases to require an actual token reduction (tokens_before > tokens_after) and reports the compression output-token count and cache-sharing state when the assertion fails.Why it's needed
The deterministic fake-server coverage added in #11175 currently proves that a
chat_compressionevent landed, but event presence alone does not prove that compression succeeded. The same event contains the result needed to distinguish a genuine reduction from a failed or token-inflating path. Reading the latest exact-name match also prevents a trailing unrelated telemetry record from being mistaken for the target event. This completes the/compressfollow-up deferred from #11094 and tracked in #11213.Reviewer Test Plan
How to verify
npx cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests ./test-helper.test.ts; all 9 tests should pass.QWEN_CODE_LANG=en QWEN_SANDBOX=false npx vitest run --root ./integration-tests interactive/context-compress-interactive.test.ts; both active/compresscases should pass and the existing token-inflation case should remain skipped.npm run typecheck:integration, targeted ESLint, and targeted Prettier checks; each should pass.api_requestfixture has no compression attributes; restoring the predicate makes it pass.Observed fake-server telemetry was deterministic: the basic case made 2 API requests and reduced tokens from 1825 to 71; the instruction-forwarding case made 2 API requests and reduced tokens from 1832 to 71. Neither case emitted a managed-memory event. Both reported
compression_output_token_count=0andcache_sharing_used=false. The exacttokens_beforevalues vary with the environment (the tracked finding observed 3607), so the assertion intentionally pins the invariant reduction rather than literal counts.Evidence (Before & After)
N/A (test-infrastructure-only change). Before, the suite accepted any landed
chat_compressionevent. After, it accepts only a matching event whose numeric token counts demonstrate a reduction, with actionable diagnostics on failure.Tested on
Environment (optional)
macOS, Node.js v25.9.0,
QWEN_SANDBOX=false, localFakeOpenAIServerfixture.Risk & Scope
/compressintegration suite so background extraction cannot shift fake-server request indexes.Linked Issues
Refs #11213
中文说明
本 PR 的改动
/compress的假服务器测试配置中禁用托管自动记忆，使压缩侧查询稳定地位于预期请求序号。/compress端到端用例，要求压缩后确实减少 token（tokens_before > tokens_after）；断言失败时同时输出压缩结果 token 数和缓存共享状态。为什么需要这项改动
#11175 新增的确定性假服务器测试目前只能证明
chat_compression事件已经写入，但事件存在本身不能证明压缩成功。该事件已经包含区分真实压缩、压缩失败或 token 膨胀所需的结果数据。读取事件名精确匹配的最新记录，也可避免把尾部无关遥测记录误当成目标事件。本 PR 完成了 #11094 延后处理、并由 #11213 跟踪的/compress后续工作。审阅者测试计划
验证方法
npx cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests ./test-helper.test.ts；9 个测试应全部通过。QWEN_CODE_LANG=en QWEN_SANDBOX=false npx vitest run --root ./integration-tests interactive/context-compress-interactive.test.ts；两个启用中的/compress用例应通过，现有 token 膨胀用例仍保持跳过。npm run typecheck:integration、针对改动文件的 ESLint 和 Prettier 检查；均应通过。api_request固定数据不含压缩属性，“最新匹配项”测试应失败；恢复条件后测试通过。实际观察到的假服务器遥测结果保持确定性：基础用例发起 2 次 API 请求，token 从 1825 降至 71；指令转发用例发起 2 次 API 请求，token 从 1832 降至 71。两个用例都未产生托管记忆事件，并且都报告
compression_output_token_count=0、cache_sharing_used=false。具体的tokens_before数值会随环境变化（跟踪项中观测到的是 3607），因此断言有意固定“压缩后减少”这一不变量，而不写死具体数值。前后对比证据
不适用（仅改动测试基础设施）。改动前，测试接受任意已写入的
chat_compression事件；改动后，只有事件名匹配且数值型 token 计数表明确实减少时才会通过，并在失败时提供可操作的诊断信息。测试平台
环境（可选）
macOS、Node.js v25.9.0、
QWEN_SANDBOX=false、本地FakeOpenAIServer固定测试环境。风险与范围
/compress集成测试套件中有意禁用托管自动记忆，避免后台记忆抽取改变假服务器请求序号。关联 Issue
Refs #11213