Skip to content

refactor(core): remove unused LruCache utility - #9926

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
AaronZ345:remove-dead-lru-cache
Aug 26, 2026
Merged

refactor(core): remove unused LruCache utility#9926
wenshao merged 1 commit into
QwenLM:mainfrom
AaronZ345:remove-dead-lru-cache

Conversation

@AaronZ345

Copy link
Copy Markdown
Contributor

What this PR does

Removes the unused core LruCache utility and its isolated unit test, then drops the corresponding legacy filename allowlist entry.

Why it's needed

LruCache is no longer referenced outside its own test, so keeping it around adds dead code and an unnecessary legacy filename exception. Removing it keeps the core utility surface smaller and the filename allowlist more accurate.

Reviewer Test Plan

  • npm ci (completed successfully and ran the repository prepare/build flow)
  • npm run typecheck --workspace packages/core
  • npx eslint eslint.legacy-filenames.mjs --max-warnings 0
  • npx prettier --check eslint.legacy-filenames.mjs
中文说明

删除未被生产代码引用的 LruCache 工具及其单测,同时移除旧文件名白名单里的 LruCache 条目,减少 core 包里的死代码。

Drop the unreferenced LruCache helper and its isolated tests so the legacy filename allowlist no longer tracks dead core utility code.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good for the essentials — What this PR does, Why it's needed, and Reviewer Test Plan are all filled in. One small note: the Risk & Scope and Linked Issues sections (and the Tested on table) from the template are missing. For a deletion-only change those would mostly read "none", so it's not blocking — worth adding if you do another pass.

Problem: this is a verified, observed fact rather than a theoretical concern. I grepped the whole repo (case-insensitive) and LruCache has zero references outside its own file and its test, and it is not exported from the core package's public entry point. The one real LRU cache in the codebase (web-fetch.ts) uses mnemonist's LRUCache, an unrelated npm package. So this is genuinely dead code plus a now-unneeded legacy-filename exception — exactly as the PR claims.

Direction: aligned. Removing dead code keeps the core utility surface smaller and the filename allowlist accurate; it's pure hygiene with no user-visible behavior and no public-API impact. No CHANGELOG signal is expected for a dead-code removal.

Size: core paths are touched. Production logic is 42 deleted lines (LruCache.ts) plus 1 allowlist line; the other 91 deleted lines are the unit test (*.test.ts, excluded from the production count). Well under the 500-line refactor threshold, so no core-module size block.

Approach: the scope is exactly right and already minimal — delete the utility, delete its isolated test, drop the matching allowlist entry. There's nothing here to cut and no simpler path; no drive-by changes or unrelated churn in the diff.

Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板的核心部分齐全——What this PR doesWhy it's neededReviewer Test Plan 都有内容。一个小提示:模板里的 Risk & ScopeLinked Issues 部分(以及 Tested on 表格)没有填。对纯删除改动来说这些基本都会是"无",所以不阻塞——如果再有下一轮可以补上。

问题:这是已核实的事实,而非理论性担忧。全仓库(不区分大小写)搜索确认 LruCache 在它自己的文件和单测之外没有任何引用,也没有从 core 包的公共入口导出。代码库里唯一真正用到的 LRU 缓存(web-fetch.ts)用的是 mnemonistLRUCache,是无关的 npm 包。所以这确实是死代码加一个已不再需要的旧文件名白名单例外——与 PR 描述一致。

方向:对齐。删除死代码能让 core 工具面更小、文件名白名单更准确;纯卫生改动,无用户可见行为,也不影响公共 API。死代码删除本就不需要 CHANGELOG 信号。

规模:触及核心路径。生产逻辑为删除 42 行(LruCache.ts)加 1 行白名单;另外删除的 91 行是单测(*.test.ts,不计入生产行数)。远低于 500 行 refactor 阈值,不触发核心模块规模阻塞。

方案:范围恰好且已是最小——删除工具、删除其独立单测、移除对应白名单条目。没有可砍的部分,也没有更简路径;diff 里没有顺手改动或无关变更。

风险:无升级风险信号——改动的文件均未命中与 revert 相关的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 5ca021fbd563da8db17e39405d887589dffd0688 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff, my own plan for "remove the unused LruCache" was: confirm nothing references it, then delete packages/core/src/utils/LruCache.ts, its colocated test, and the 'LruCache' entry in the legacy-filename allowlist. The PR does exactly that — no more, no less. The diff is three deletions with no hidden edits, the allowlist stays alphabetically ordered after the removal, and since the class was never exported from the package entry point there is no public surface to break. No correctness, security, or convention issues found — nothing blocking.

Testing

Evidence below is the PR's own CI on the reviewed commit, fetched via the API (per triage policy, I do not build or run fork code). The primary Linux unit suite is green, which is the meaningful signal for a deletion-only change: typecheck and tests both survive the removal. The macOS/Windows and integration jobs show as skipped on this run, and the web-shell E2E smoke is still queued; the finalize job refreshes the table once CI settles. No check is red.

Final CI results for 5ca021f (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Not verified: real-scenario/TUI testing — N/A by design here. This removes unreferenced code with no user-visible behavior, so there is nothing to drive in a terminal, and no sandboxed /verify or /tmux lane is needed either (no behavioural claim to settle; the public surface is unchanged).

中文说明

代码审查:在读 diff 之前,我对"删除未使用的 LruCache"的独立方案是:先确认无引用,然后删除 packages/core/src/utils/LruCache.ts、其同目录单测,以及旧文件名白名单中的 'LruCache' 条目。PR 的做法与此完全一致——不多不少。diff 只有三处删除、没有夹带改动,白名单移除条目后仍保持字母序;由于该类从未从包入口导出,也不存在可破坏的公共面。未发现正确性、安全性或规范问题——无阻塞项。

测试:以上证据来自该 PR 在被审提交上的自有 CI(通过 API 获取;按审查策略,不构建或运行 fork 代码)。主要的 Linux 单测套件为绿,这是对纯删除改动最有意义的信号:删除后类型检查与测试均通过。macOS/Windows 与集成测试在本次运行中为 skipped,web-shell E2E 冒烟仍在排队;CI 结束后 finalize 任务会刷新该表。没有红色检查。

未验证:真实场景/终端测试——此处按设计即为 N/A。删除的是无引用代码,无用户可见行为,没有可在终端中操作的对象,也无需 /verify/tmux 沙箱通道(没有需要证实的行为性声明,公共面未变化)。

Qwen Code · qwen3.8-max

Reviewed at 5ca021fbd563da8db17e39405d887589dffd0688 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review at every stage; the 3/5 is the fork-refactor guardrail capping the score, not real doubt about the change.

Stepping back: this is exactly the kind of PR you want to see from a contributor — a small, honest, verifiable cleanup. The motivation checks out (I confirmed independently that LruCache is referenced nowhere and was never exported), the implementation is the minimal possible diff, the description is accurate, and the primary CI suite is green. If I had to maintain this in six months, I'd thank the author for deleting dead weight rather than letting it linger.

The one thing I'd flag for whoever merges: CI is not fully settled yet — the main Qwen Code CI run was still queued at review time, with the Linux unit suite already green and macOS/Windows/integration jobs skipped on this run. Worth a glance at the final state before merging, though for a three-file deletion the risk is about as low as it gets.

Why I'm not approving: this PR comes from a fork and its title is a refactor type. That combination trips the approval guardrail — such PRs always need a human maintainer's eye, regardless of how clean the automated review is. So this defers to a maintainer rather than approving; nothing in the diff needs changing.

中文说明

退一步看:这正是我们希望看到的贡献——小而诚实、可验证的清理。动机成立(我独立确认了 LruCache 没有任何引用、也从未被导出),实现是最小 diff,描述准确,主要 CI 套件为绿。如果六个月后由我来维护,我会感谢作者删掉了这些死代码。

唯一提醒合并者注意的:CI 尚未完全结束——审查时主 Qwen Code CI 运行仍在排队,Linux 单测已绿,macOS/Windows/集成任务本次为跳过。合并前值得看一眼最终状态,不过对三文件删除来说风险已经低到不能再低。

不批准的原因:该 PR 来自 fork 且标题为 refactor 类型。这一组合触发审批保护规则——此类 PR 无论自动审查多干净,都需要人类维护者把关。因此转交维护者决定,而不是批准;diff 本身无需任何修改。

Qwen Code · qwen3.8-max

Reviewed at 5ca021fbd563da8db17e39405d887589dffd0688 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @yiliang114 — this is a fork PR with a refactor-type title, which the approval guardrail keeps out of auto-approval: it needs a human maintainer's sign-off regardless of review outcome. The review itself was clean at every stage (dead code independently verified, minimal diff, primary CI green), so this is a policy defer, not a doubt defer — nothing in the diff needs changing. Needs a human call on this one.

@yiliang114

Copy link
Copy Markdown
Collaborator

Maintainer verification: confirmed LruCache has no remaining consumers — the only references are its own file and test; other repo matches are the unrelated npm lru-cache dependency. The removal claim checks out.

@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.

No issues found. LGTM! ✅

中文说明

未发现问题。LGTM!✅

— qwen3.8-max via Qwen Code /review (v0.22.0)

@wenshao
wenshao added this pull request to the merge queue Aug 26, 2026
Merged via the queue into QwenLM:main with commit 06600b3 Aug 26, 2026
112 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.22.2.

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.

5 participants