test(core): give the telemetry-swap client mock a getToolRegistry - #10220
Merged
Conversation
GeminiClient.initialize() now calls restoreLoadedSkillsFromHistory, which resolves the SKILL tool through this.config.getToolRegistry(). The telemetry- swap transaction tests build a minimal config mock that predates that call and does not provide getToolRegistry, so every test that awaits client.initialize() throws "this.config.getToolRegistry is not a function" and the file reds the Linux unit lane. Add getToolRegistry to the mock, returning an empty registry (no SKILL tool, so the skill restore is a no-op) — the transaction behaviour under test is unchanged.
wenshao
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin and
yiliang114
as code owners
August 27, 2026 01:17
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the quick fix! Template looks good ✓ Problem: observed and confirmed, not theoretical. The ubuntu unit lane is red on current Direction: aligned — this restores a required check that currently blocks every freshly-rebased PR. Test-only, no product surface touched. Size: touches Approach: minimal and right. The only registry call these tests can reach is Risk: no elevated risk signals — test-only change, no high-risk-path match. Moving on to code review. 🔍 中文说明感谢及时的修复！ 模板完整 ✓ 问题：已观测到并已核实，不是理论问题。当前 main 上 ubuntu 单测道确实红了，签名完全一致——例如 PR #10203（一个带着今天 main 的无关分支）的 CI 运行中，Test (ubuntu-latest, Node 22.x) 以 "Test Files 1 failed | 611 passed (613)" 失败，唯一失败的文件就是 client.telemetrySwap.test.ts：10 个用例中 9 个在 client.ts:615（restoreLoadedSkillsFromHistory）抛 TypeError: this.config.getToolRegistry is not a function。时间线也对得上：#9500（在 initialize() 中加入 restoreLoadedSkillsFromHistory 调用）于 2026-08-26 合入时 CI 是绿的，因为其分支基线早于 #9833/#9844 的 telemetry-swap 测试——两者交叉合入后 main 变红。 方向：对齐——这是恢复必选检查道，当前它阻塞所有刚 rebase 的 PR。纯测试改动，不触及产品面。 规模：触及 packages/core/src/**，但唯一改动文件是 client.telemetrySwap.test.ts（+5/−0）：生产逻辑 0 行，测试 5 行。无 Stage 0 顾虑（作者本身是维护者）。 方案：最小且正确。这些测试唯一能走到的注册表调用是 restoreLoadedSkillsFromHistory → getToolRegistry().getTool(ToolNames.SKILL)；返回空注册表让技能恢复成为 no-op，不影响被测的事务行为。没有更简路径——spy 私有方法或在生产代码里为测试缺口加防御都更差。 风险：无升级风险信号——纯测试改动，未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Code review Before reading the diff, my independent take was: the minimal fix is to give the hand-rolled config mock a Verified against the code:
No findings. ✅ CI evidence — the PR's own checks on the reviewed commit, read via the API (triage never executes PR code) Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The lane that matters is 中文说明代码审查：在读 diff 之前，我的独立方案就是给这个手搓 config mock 补一个 getToolRegistry——返回一个 getTool 取不到任何工具的对象，使技能恢复成为 no-op。其他方案（spy 私有方法、在生产代码里加防御性判断、换成完整真实 Config）都更差。PR 做的正是这个最小改动。 对照代码核实：这些测试唯一会走到注册表调用的路径是 initialize() 的 resume 分支，且仅用于 getToolRegistry().getTool(ToolNames.SKILL)（client.ts:615）。mock 的 { getTool: () => undefined } 与真实 ToolRegistry.getTool(name): AnyDeclarativeTool | undefined 形状一致，可选链使恢复成为 no-op。被测的三个 swap 方法从不触碰注册表，文件其余位置无需该 mock。10 个用例中有 1 个不带 resume 数据初始化、根本走不到该调用——正好对应红道 9/10 的失败数（PR 描述说全部 10 个失败，小出入，不影响结论）。断言无改动，生产代码无改动，新增注释把 why 讲清楚了。 无发现 ✅ CI 证据（通过 API 读取本提交上 PR 自身的检查；triage 不执行 PR 代码）：关键道 Test (ubuntu-latest, Node 22.x) 正是当前在 main 上以此 TypeError 失败的那条，在本提交上仍在运行。该道也是本 PR 的神谕：它此前唯一失败的文件就是本 PR 修补的文件，此处跑绿即可定论。尚未验证：本提交上的结果——CI 仍在进行，表格会在结束后自动更新。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — verified a real breakage with hard CI evidence, and the fix is exactly the minimal correct one. Stepping back: I confirmed the problem exists before trusting the PR's framing — the red lane log from an unrelated PR carrying today's CI is still running on this commit ( 中文说明置信度：5/5——用真实的 CI 证据核实了 main 上确有其损坏，而修复恰好是最小且正确的改法。 退一步看：在采信 PR 的表述之前，我先确认了问题真实存在——一个带着今天 main 的无关 PR 的红道日志显示：唯一失败的文件、正是这个 TypeError、别无其他。修复与我独立会采取的方案一致，我也找过更便宜的路径（spy 私有方法、在生产调用处加防御、换真实 Config）——都更差。5 行测试 fixture，满足一处注册表调用，技能恢复变为 no-op，断言原封不动。半年后维护这段代码时，mock 里的注释会告诉我它为什么在那里。唯一的小瑕疵——描述说 10 个用例全失败而红道显示 9 个——不影响修复本身。 本提交上的 CI 仍在运行（Test (ubuntu-latest, Node 22.x) 进行中），因此暂缓批准，待 CI 在该提交上全绿后自动完成批准。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
wenshao
enabled auto-merge
August 27, 2026 01:59
This was referenced Aug 27, 2026
This was referenced Aug 27, 2026
Open
yiliang114
pushed a commit
that referenced
this pull request
Aug 27, 2026
Picks up #10220's getToolRegistry mock fix, which is what the 9 failing client.telemetrySwap tests on this branch were missing. The branch was cut before that landed, so the failure was inherited, not introduced.
yiliang114
pushed a commit
that referenced
this pull request
Aug 27, 2026
#10220 and #9729 independently gave this suite's config mock a `getToolRegistry`, and merging them produced a clean Git merge and a broken object literal: the same key twice, which is TS1117 ("An object literal cannot have multiple properties with the same name"). PR CI does not catch it — it runs only `typecheck:integration`, not the workspace `npm run typecheck` — so it fails for the next person to run preflight rather than in a check anyone is watching. Keep #10220's copy: its comment names the call that needs the registry and the error an empty one avoids.
wenshao
added a commit
to AaronZ345/qwen-code
that referenced
this pull request
Aug 27, 2026
…ck (QwenLM#10252) QwenLM#10220 already added a getToolRegistry stub to the telemetry-swap client test's config mock; the QwenLM#9729 branch carried an identical copy, and the merge kept both. Duplicate property names in an object literal are a TS1117 error that breaks tsc --build (and thus npm run build) on main while vitest, which only transpiles, stayed green. Drop the duplicate; the mock behavior is unchanged.
qqqys
added a commit
to qqqys/qwen-code
that referenced
this pull request
Aug 27, 2026
The `Merge branch 'main'` in 1f670a1 brought main's TS1117 in with it: `client.telemetrySwap.test.ts` declares `getToolRegistry` twice in the same object literal, which fails `tsc` and kills `packages/core`'s build before a single test runs — the Test job dies in "Install dependencies". Not this branch's doing. Two main commits added the property independently and neither saw the other: 032b907 feat(serve): backfill session PR bindings ... (QwenLM#9729) 8241905 test(core): give the telemetry-swap client mock a getToolRegistry (QwenLM#10220) `upstream/main` at 053f17b still carries both — checking that exact file out here and running `tsc --noEmit -p packages/core` reproduces `client.telemetrySwap.test.ts(103,5): error TS1117` verbatim, so main is red on its own and every branch that merges it inherits this. Keeps QwenLM#10220's copy — it was added for this purpose and carries the explanation — and drops QwenLM#9729's incidental one. main needs the same removal; this only unblocks the branch. Verified: `tsc --noEmit -p packages/core` clean, `client.telemetrySwap.test.ts` 10 passed. Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1
qqqys
added a commit
to qqqys/qwen-code
that referenced
this pull request
Aug 27, 2026
Merging upstream/main (053f17b) clears the 9 `client.telemetrySwap` failures this branch had from predating QwenLM#10220, but main carries its own TS1117 on the same file: two commits added `getToolRegistry` to the same object literal independently and neither saw the other. 032b907 feat(serve): backfill session PR bindings ... (QwenLM#9729) 8241905 test(core): give the telemetry-swap client mock a getToolRegistry (QwenLM#10220) Checking that file out from upstream/main here and running `tsc --noEmit -p packages/core` reproduces `client.telemetrySwap.test.ts(103,5): error TS1117` verbatim, so taking the merge unmodified would have traded 9 test failures for a build that never reaches the tests at all. Removed as part of the merge rather than left for a follow-up: keeps QwenLM#10220's copy, which was added for this purpose and carries the explanation, and drops QwenLM#9729's incidental one. main still needs the same removal — this only keeps it out of the branch. Verified after the merge: `tsc --noEmit` clean for both packages/core and packages/cli; client.telemetrySwap 10 passed, Session.test.ts and acpAgent.test.ts 1260 passed together. Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1
This was referenced Aug 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds
getToolRegistryto the minimalConfigmock the telemetry-swap transaction tests build, returning an empty registry (no SKILL tool), soclient.initialize()can run without throwing.Why it's needed
GeminiClient.initialize()now callsrestoreLoadedSkillsFromHistory, which resolves the SKILL tool throughthis.config.getToolRegistry(). The telemetry-swap tests (client.telemetrySwap.test.ts, from #9833) construct a small hand-rolled config mock that predates that call and does not exposegetToolRegistry. Every test in the file awaitsclient.initialize(), so each now throwsTypeError: this.config.getToolRegistry is not a functionand the whole file fails — reddening theTest (ubuntu-latest, Node 22.x)unit lane on currentmain. This restores the mock to the surfaceinitialize()now touches, leaving the transaction behaviour under test unchanged (the empty registry means the skill restore is a no-op, which is exactly what these telemetry-only tests want).Reviewer Test Plan
How to verify
Before this change, on current
main:npm run test --workspace=packages/core -- src/core/client.telemetrySwap.test.tsfails all 10 cases withTypeError: this.config.getToolRegistry is not a functionatGeminiClient.restoreLoadedSkillsFromHistory(client.ts), reached frominitialize(). After this change the same command passes 10/10. No production code changes — only the test's config mock gains one method.Evidence (Before & After)
N/A — non-user-visible test-only change.
Tested on
Environment (optional)
Unit tests only (
vitest, packages/core).Risk & Scope
restoreLoadedSkillsFromHistoryruns duringinitialize()in these telemetry tests (that coupling is pre-existing); this only stops the mock from throwing.Linked Issues
N/A — repairs a standing failure on
mainintroduced by the interaction ofrestoreLoadedSkillsFromHistory(skill-state restore) with this test's older config mock.中文说明
GeminiClient.initialize()现在会调用restoreLoadedSkillsFromHistory,后者通过this.config.getToolRegistry()解析 SKILL 工具。而 telemetry-swap 事务测试(client.telemetrySwap.test.ts,来自 #9833)用的是一个早于该调用的最小 config mock,没有getToolRegistry。文件里每个用例都await client.initialize(),于是全部抛TypeError: this.config.getToolRegistry is not a function,把Test (ubuntu-latest, Node 22.x)单测道在当前main上打红。此改动给 mock 补上getToolRegistry(返回空注册表,无 SKILL 工具→技能恢复为 no-op),被测的事务行为不变。纯测试改动,无生产代码变更。