refactor(core): shrink the content generator interface - #9676
Conversation
|
Re-run at the updated head (main merge + the follow-up cleanup requested by the last triage round). Gate outcome unchanged — passes, with one size note below that deserves maintainer visibility. Template still fine ✓ Problem: still real dead code, re-verified at the new head. Direction: unchanged — narrowing a contract that every backend and test double must implement but nothing consumes is the right direction. Size: 1,089 production-logic lines (interface shrink 157, orphaned estimator-cluster deletion ~922, remainder comment/doc-reference cleanup) vs 932 test lines, 32 doc lines, and 2 lines of CI bookkeeping. To be straight about it: this is above the 500-production-line threshold at which a fresh fork Approach: scope remains minimal. The new delta is exactly the follow-up: the estimator cluster deleted root and branch (zero residual references), stale references in two design docs and one code comment updated with dated sync notes rather than silent edits, and Risk: Re-review complete. 🔍 中文说明基于更新后的 head(合入 main + 上一轮 triage 要求的后续清理)重新运行。门禁结论不变——通过,但下面有一条应当让维护者知悉的规模说明。 模板仍然完整 ✓ 问题:仍是真实的死代码,已在新 head 上复核。 方向:不变——收窄每个后端和测试替身都必须实现、却无消费者的协议,方向正确。 规模:1,089 行生产逻辑(接口收窄 157 行、孤儿估计器簇删除约 922 行、其余为注释/文档引用清理),测试 932 行、文档 32 行、CI 记账 2 行。坦率说明:这已超过 500 行生产逻辑阈值——一个全新的、触及核心路径的 fork 方案:范围仍然最小。新增改动恰为所需:估计器簇连根删除(零残留引用),两个设计文档与一处代码注释中的过期引用以带日期的同步注记更新而非静默改写, 风险: 复审完成 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (re-run at the merged head)My independent baseline for this round, written before reading the new delta: confirm the estimator cluster has no importers beyond the two What I verified myself against the current
No blockers. The two non-blocking findings from @wenshao's deep-verify round both stand, and I re-checked them against the base: F1 — the PR body's "embedding still has an active call path" is inaccurate ( Files changed (30 of 34 shown)
Test evidence (this PR's own CI — I never run PR code)All PR-CI runs on the reviewed commit have completed, and the unit suite is fully green: from the One correction to the previous run's wording: the macOS and Windows test legs and the CLI integration job are skipped here by design, not fail-fast — ci.yml gates all three on
Real-scenario testing: N/A — unattended run, and the change has no user-visible surface to drive. The central claim is a negative one (zero behavior change), and it is substantiated: repo-wide typecheck as the exhaustiveness oracle, a fully green 23,486-test suite on this head, and @wenshao's local A/B round showing byte-identical wire requests between base and head bundles. A sandboxed CI-side verification run for this head is also in flight and will post its own report in this thread — belt and braces rather than an open gap. 中文说明代码审查(合入后 head 的复审)本轮在读新改动之前写下的独立基线:确认估计器簇除本次移除的两个 我亲自对照当前
无阻塞项。@wenshao 深度验证轮的两条非阻塞发现均成立,且我已对照基线复核:F1——PR 正文"embedding 仍有活跃调用链路"不准确( 测试证据(来自本 PR 自己的 CI——审查过程不运行 PR 代码)受审 commit 上的所有 PR-CI 运行均已完成,单测套件全绿: 对上一轮措辞的一处更正:macOS/Windows 测试腿与 CLI 集成 job 在此为设计上跳过,而非 fail-fast——ci.yml 将三者都门禁在 真实场景测试:不适用——无人值守运行,且改动无用户可见界面可驱动。中心主张是否定性主张(零行为变化),且已被证实:全仓库 typecheck 作为完备性判定、本 head 上 23,486 个测试全绿、@wenshao 的本地 A/B 轮显示 base 与 head 构建的 wire 请求逐字节一致。针对本 head 的 CI 侧沙箱验证运行也在进行中,会在本线程另行发布报告——属于双保险,而非未闭合缺口。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean re-review at the final head; the fork-refactor guardrail keeps the approval with the maintainer, who has already exercised it on this commit (policy cap, not doubt). Stepping back: this PR grew since the last run along exactly the axis that run asked for, and it's better for it. The author took the orphaned-utility note and deleted the estimator cluster root and branch, updated every stale reference with dated sync notes instead of silent edits, merged main to clear the unrelated red leg, and kept the diff free of drive-by churn. The result: every provider, decorator, and test double now implements only what something actually calls, and the follow-up the previous comment tracked as "worth a cleanup issue" simply no longer exists. My independent baseline for the follow-up round and the PR's approach are the same plan — I found no simpler path it missed. All three original defer reasons are resolved:
So: no approval from this account — the guardrail is absolute for fork refactors — and no change request, because nothing blocking was found. The bot side of the gate is complete on this head; the merge decision rests with the maintainers. 中文说明置信度:3/5 —— 在最终 head 上的复审是干净的;fork-refactor 护栏把批准权留在维护者手中,而维护者已在此 commit 上行使了该批准(这是政策上限,而非对代码有疑虑)。 退一步看:自上一轮以来,这个 PR 恰恰沿着上一轮要求的方向生长,并且因此变得更好。作者接到孤儿工具的提示后,把估计器簇连根删除,用带日期的同步注记(而非静默修改)更新了每一处过期引用,合入 main 清除了无关的红色测试腿,且 diff 中没有顺手改动。结果是:现在每个 provider、装饰器和测试替身只实现真正被调用的东西,上一条评论里记为"值得开一个清理 issue"的后续事项已不复存在。我为本轮后续工作独立写下的基线方案与 PR 的做法就是同一个方案——我没有找到被它遗漏的更简路径。 最初转交的三个原因已全部解决:
因此:本账号不会给出批准——护栏对 fork refactor 是绝对的——也不会请求修改,因为没有发现任何阻塞项。门禁的机器人一侧在此 head 上已完成;合入决定权在维护者。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 628 passing — this review observed 22866, 20658, 1661, 1638, 495, 4026, 595 passed; 175 passing — this review observed 22866, 20658, 1661, 1638, 495, 4026, 595 passed.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan(非阻断):628 passing — this review observed 22866, 20658, 1661, 1638, 495, 4026, 595 passed; 175 passing — this review observed 22866, 20658, 1661, 1638, 495, 4026, 595 passed。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Removing countTokens from both providers deleted the last production consumers of RequestTokenEstimator. Delete the orphaned cluster: requestTokenizer.ts (330), imageTokenizer.ts (534), types.ts (36), the directory barrel (11), and both test files (608 lines). Also drop the inert vi.mock of requestTokenizer.js left in client.test.ts and the stale dimension-extractor cross-reference in review/lib/assets.ts. textTokenizer.ts and supportedImageFormats.ts stay: converter.ts, pdf.ts, and fileUtils.ts still consume them and the core barrel re-exports them.
…face countTokens and useSummarizedThinking no longer exist on ContentGenerator, so the design record for the lazy-wrapper architecture must not keep advertising them: list the three remaining shared async operations, drop the useSummarizedThinking sentence and the summarized-thinking item from the consumer audit and Verification section, and add a dated note recording the interface shrink from PR QwenLM#9676.
|
Status update (maintenance pass after stall) — new head 1. Red CI diagnosed. The only failing check (Test ubuntu-latest, Node 22.x) failed exactly 1 of 22,883 tests: 2. All three review suggestions verified and actioned (per-thread replies posted, threads resolved):
3. Verification evidence. Repo-wide typecheck on the merged head: core 4. Maintainer items (policy, unchanged): fork-refactor approval guardrail and the exported- |
Same latent main-side violation as fixed in QwenLM#9682: QwenLM#9587 grew the workflow without a baseline update; record the new size as the check message directs (precedent QwenLM#9747).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1c": none — though I did not execute the affected test suites (cross-file verification was static; runtime behavior of the edited mocks is attested by the author's C…; chunk 3: typecheck/unit-test run for the modified core test files (worktree lacks node_modules and dist; install+build exceeds budget).
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/core/__tests__/openaiTimeoutHandling.test.ts:352 — [review] Suite titled 'token estimation on timeout' outlives the deleted token-estimation test
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 1c":none — though I did not execute the affected test suites (cross-file verification was static; runtime behavior of the edited mocks is attested by the author's C…;chunk 3:typecheck/unit-test run for the modified core test files (worktree lacks node_modules and dist; install+build exceeds budget)。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…emoval Follow-up to 0ee1763/1871bb5b81 (review round 2): - supportedImageFormats.ts header and getSupportedImageFormatsString doc no longer describe a tokenizer decode/metadata-extraction stage; the list is now documented as the vision-input acceptance list, with token accounting noted as the flat DEFAULT_IMAGE_TOKEN_ESTIMATE. - web-shell-image-drag-and-drop.md's BMP rationale no longer claims ImageTokenizer parses BMP dimensions; dated sync note added stating BMP support rests on SUPPORTED_IMAGE_MIME_TYPES plus converter passthrough since PR QwenLM#9676.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Follow-up to 18f08c0: the test plan still required converter/tokenizer focused tests for image paths; the image-tokenizer estimator cluster was removed in PR QwenLM#9676 (text tokenizer is unaffected and out of scope here).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
docs/design/web-shell/web-shell-image-drag-and-drop.md:512 — [probe] Sync-note blockquote absorbs the following paragraph — missing blank line
中文说明
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
🔬 Deep verification: merge-ready — 75/75 scripted assertions passed, 0 unexpected failuresLocal maintainer round against verified head Central claim — proven by A/B"Removing Identical headless scenario (2 prompt runs, loopback OpenAI-compatible server, isolated HOME/settings) against the base bundle vs head bundle — twice, including a fresh
Supporting evidence
Findings (both non-blocking)
Not coveredPer-provider wire harnesses for Anthropic/Gemini/Qwen (covered by typecheck + unit suites only); per-commit attribution (aggregate diff verified, 7 commits); UI E2E (matches the PR's own out-of-scope note); the merit of #9587's workflow growth (only the baseline bookkeeping was checked). 📄 Full reportPR 9676 deep verification — VERDICT: merge-ready (75/75 scripted assertions passed, 0 unexpected failures)
Central claim + A/B table
Scenario: 2 headless Controls that held: no Census (9/9 scripted assertions)See summary above; raw log Mutation matrix (5/5)
Dist-level behavior checks (compiled head artifact)HEAD 4/4 ( Targeted gates
Corrections: the Reviewer Test Plan's "14 core files, 1,628 passing" reads 13 files / 1632 at the verified head — the PR itself deleted the 14th file; +4 tests from the main merge. CLI numbers match exactly. Findings (both non-blocking)F1 (Low — description accuracy; pre-existing): "embedding … still has an active call path" — F2 (Low — coverage gap on surviving behavior, verified correct): no test drives Not coveredPer-provider wire harnesses for Anthropic/Gemini/Qwen (typecheck + unit suites only); per-commit attribution (aggregate diff verified); UI E2E (matches the PR's own note); the merit of #9587's workflow growth; embedding end-to-end reachability (no production caller exists by construction — decorator- and lazy-layer forwarding verified against the compiled artifact instead). MethodologyMaintainer-local, macOS (darwin), Node v24.18.1. Two detached scratch worktrees ( 中文摘要结论:可以合并(merge-ready),75/75 脚本断言全部通过,0 个意外失败。
|
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定:❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified the removal is complete at head cf82579:
- No residual production callers: a grep over the head tree finds zero
countTokens(call sites in packages and nouseSummarizedThinkingcode references (only historical mentions in the 2026-07-22 design doc, which this PR updates). TheLazyContentGeneratorconstructor loses thesummarizedThinkingflag along with the interface member, and the StandaloneSessionPicker stub that fakeduseSummarizedThinking: () => falseis removed with it. - Embedding stays intact as claimed:
embedContentremains on the contract and on every provider implementation. - The request-tokenizer pruning is surgical: only the orphaned estimator cluster (imageTokenizer/requestTokenizer/index/types) is deleted;
textTokenizer(still used by pdf.ts and the OpenAI converter) andsupportedImageFormats(still used by fileUtils' PIPELINE_IMAGE_MIME_TYPES) stay, and the supportedImageFormats doc comment is updated to record the removal and the flat DEFAULT_IMAGE_TOKEN_ESTIMATE replacement. - Typecheck-level contract compliance is covered by CI, which is green at approval time (14 checks passing, none failing).
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the head at cf82579ac4. The contract narrowing is correct and complete — no blockers.
Verified on the head:
countTokens/useSummarizedThinking: zero residual references across production, tests, and types.RequestTokenizer/ImageTokenizerwere only consumed by the anthropic/openaicountTokensimplementations, so removing them as orphaned (along withrequest-tokenizer/index.ts+types.ts) is correct.textTokenizer/supportedImageFormatsandembedContentare retained with live call sites (converter.ts,pdf.ts,fileUtils.ts,baseLlmClient.ts).LoggingContentGeneratorstill forwards generation + embedding unchanged.
Linked this PR to #4063 (item 1) in the body.
One observation, non-blocking: on this fork PR build-cli and precheck-pr (typecheck) report skipped, so CI only exercised Test (ubuntu-latest, Node 22.x) (green). The body states build+typecheck pass locally and reference consistency checks out statically, but letting precheck-pr run green once before merge would close that gap.
|
Opened follow-up #9899 for the post-merge build failure already captured by sandboxed verification: the independently added Vertex ADC test still called the removed |


What this PR does
This removes unused token-counting and summarized-thinking capabilities from the shared content-generation contract, its lazy and logging decorators, and the provider implementations. It also removes the corresponding dead tests and mock members while leaving embedding support unchanged because it still has an active call path.
Why it's needed
The shared provider contract required every backend and test double to implement operations that no production caller used. That widened the Google SDK type surface, duplicated provider-specific code, and made the abstraction look more capable than its consumers require. Narrowing the contract reduces provider coupling and makes later protocol work smaller without changing generation behavior.
Reviewer Test Plan
How to verify
npm run buildandnpm run typecheck; both should complete successfully.packages/coreand the session-picker/AppContainer tests frompackages/cli; the verified result is 14 core files with 1,628 passing tests and 2 CLI files with 175 passing tests.Evidence (Before & After)
N/A — this is a non-user-visible contract cleanup.
Tested on
Environment (optional)
Local workspace with Node.js v24.18.0 and npm 11.16.0; no sandbox.
Risk & Scope
countTokensanduseSummarizedThinkingfrom theirContentGeneratorusage. There are no remaining production callers in this repository.Linked Issues
Refs #4063
中文说明
这个 PR 做了什么
本 PR 从共享内容生成协议、懒加载与日志装饰器以及各 provider 实现中移除了未使用的 token 计数和 summarized-thinking 查询能力,同时清理了对应的无效测试和 mock 成员。Embedding 支持仍然保留,因为目前还有真实调用链路。
为什么需要这个改动
共享 provider 协议此前要求每个后端和测试替身都实现生产代码从未调用的操作。这扩大了 Google SDK 类型的影响范围,造成了 provider 侧重复代码,也让抽象层看起来承担了超出实际消费者需求的职责。收窄协议可以降低 provider 耦合,并让后续协议改造范围更小,同时不改变内容生成行为。
Reviewer 测试计划
如何验证
npm run build和npm run typecheck,两者都应成功完成。packages/core运行受影响的 core 测试,并在packages/cli运行 session picker 与 AppContainer 测试;本地验证结果为 14 个 core 测试文件共 1,628 个用例通过,以及 2 个 CLI 测试文件共 175 个用例通过。证据(改动前后)
N/A — 这是一个无用户可见变化的协议清理。
测试平台
环境(可选)
本地工作区,Node.js v24.18.0、npm 11.16.0,未使用 sandbox。
风险与范围
ContentGenerator用法中移除countTokens和useSummarizedThinking。本仓库生产代码中已经没有这些成员的调用方。关联 Issue
Refs #4063