refactor(core): centralize extension runtime refresh - #6152
Conversation
|
Thanks for the PR! Template looks good ✓ On direction: this is a clean, well-scoped extraction that fits squarely within the extension system improvement track (#3696). The motivation is sound — having a single orchestrator function for extension runtime refresh is a reasonable foundation for the planned follow-up work (commands, hooks, LSP, On approach: the scope is tight — one function extraction, one delegation site, one test file, plus a plan doc for the broader initiative. The 320-line plan document is substantial for what's essentially a 40-line code move, but it's explicitly referenced in the PR description as context for future PRs, so it earns its place. No scope creep, no drive-by refactors. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:这是一个范围清晰的重构,完全契合扩展系统改进方向(#3696)。动机合理——将扩展运行时刷新逻辑收进一个统一的编排函数,为后续工作(commands、hooks、LSP、 方案:范围控制得当——一个函数抽取、一个委托调用点、一个测试文件,加上一份用于描述后续 PR 序列的计划文档。320 行的计划文档对于一个本质上是 40 行代码搬移的 PR 来说看起来较多,但 PR 描述中明确引用它作为后续 PR 的上下文,因此有其存在的价值。没有范围蔓延,没有顺手重构。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewI compared the extracted The Tests cover the five key scenarios: no-config guard, happy path, partial No blockers. No AGENTS.md violations. Test ResultsNew orchestrator tests: 5/5 pass ✅ Typecheck (core package): clean ✅ (0 errors) Real-Scenario TestingN/A — pure internal refactor with no user-visible or TUI change. The PR body explicitly marks Before/After as N/A. Unit tests and typecheck are the appropriate verification here. Additionally, collaborator @wenshao independently performed a comprehensive real-binary A/B verification (detailed in their comment above) that exercised all four refresh legs with a probe extension, confirming byte-for-byte identical observables between the pre-refactor and PR binaries. That external validation reinforces the conclusion that this extraction is behavior-preserving. 中文说明代码审查逐行对比了抽取的
测试覆盖了五个关键场景:无 config 提前返回、正常路径、 无阻塞问题。无 AGENTS.md 违规。 测试结果新编排器测试:5/5 通过 ✅ 真实场景测试不适用——纯内部重构,无用户可见或 TUI 变化。PR 正文中明确标注 Before/After 为 N/A。单元测试和类型检查是此处合适的验证方式。 此外,协作人员 @wenshao 独立进行了全面的真实二进制 A/B 验证(详见其评论),使用探针扩展覆盖了全部四条刷新路径,确认重构前与 PR 二进制之间可观测项逐字节一致。该外部验证进一步证实本次抽取保持行为不变。 — Qwen Code · qwen3.7-max |
|
This is a textbook extraction refactor: the original The plan document, while lengthy, serves a real purpose — it's the roadmap for a 6-PR sequence and this PR is explicitly step 1. Two pieces of external validation reinforce the verdict: the 5 new unit tests with their tiered error-handling contract, and @wenshao's independent real-binary A/B verification exercising all four refresh legs. One note: the Verdict: clean, ready to merge. Escalating to maintainer for final approval since this is a fork refactor PR. 🙏 中文说明这是一个教科书式的抽取重构:原始 计划文档虽然较长,但有实际用途——它是一个 6 个 PR 序列的路线图,本 PR 明确是第一步。 两项外部验证支持该结论:5 个新的单元测试及其分层错误处理契约,以及 @wenshao 独立的真实二进制 A/B 验证覆盖了全部四条刷新路径。 一个备注: 结论: 干净,可以合并。因为是 fork 重构 PR,交由维护者做最终审批。🙏 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Clean extraction — the control flow, error handling, and Promise.allSettled semantics are faithfully preserved. One test improvement suggestion below.
…nce in refreshExtensionRuntime
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. Clean extraction — the control flow, error handling, and Promise.allSettled semantics are faithfully preserved from the original refreshMemory() body. Typecheck and ESLint pass; all 4 new tests pass. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
Clean extraction — control flow, error handling, and Promise.allSettled semantics are faithfully preserved from the original refreshMemory() body. Build, typecheck, and all 4 new tests pass. LGTM! ✅
— qwen3.7-max via Qwen Code /review
…onale comments Address @wenshao's review feedback: - Add test for restartMcpServers rejection (the only fatal error path) - Restore key 'why' comments about allSettled and try/catch design decisions - Logger tag change to EXTENSION_RUNTIME_REFRESH is intentional (standalone module)
DragonnZhang
left a comment
There was a problem hiding this comment.
New review. Clean extraction of the extension runtime refresh logic from ExtensionManager.refreshMemory() into a shared refreshExtensionRuntime() function in extension-runtime-refresh.ts.
Analysis:
- Control flow is faithfully preserved: MCP restart first (awaited, failure propagates), then
Promise.allSettledfor skills + subagents (failure logged but non-fatal), thenrefreshHierarchicalMemory(failure caught and logged). - The
ExtensionManager.refreshMemory()body is replaced with a single delegation call — zero behavioral change. - 4 new tests cover: undefined config early return, happy path,
refreshCacherejection resilience, andrefreshHierarchicalMemoryrejection. - The
restartMcpServersrejection test correctly verifies it blocks subsequent legs (matching the sequentialawaitbeforeallSettled). - Plan document provides useful context for the phased rollout.
No high-confidence issues found. All 7 existing inline comments are resolved Suggestions from prior reviews.
Downgraded from Approve to Comment: CI still running (Test ubuntu-latest pending).
— qwen3-coder via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— GPT-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Clean extraction — the control flow, error handling, and Promise.allSettled semantics are faithfully preserved from the original refreshMemory() body. ESLint and typecheck pass; all 5 new tests pass with full coverage. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Clean extraction — the control flow, error handling, and Promise.allSettled semantics are faithfully preserved from the original refreshMemory() body. Build passes, typecheck clean, all 5 new tests pass with good branch coverage (happy path, undefined config, allSettled rejection, hierarchical memory rejection, and fatal MCP rejection).
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
✅ Real-binary verification (merge reference)I built the actual Verification had four layers:
1 · Unit tests + mutation
To prove the new tests actually guard the control flow, I mutated the source and re-ran the suite:
P1 is exactly the 2 · Real E2E — all four legs fire live (no restart)I installed a probe extension that bundles an MCP server, a skill, a subagent and a
Leg 1 — Leg 2 — Leg 3 — Leg 4 — Disabling it (manage dialog) runs 3 · A/B equivalence — inline vs delegatedSame probe, same key sequence, on a pre-refactor binary (reverted
Identical in every row. The extraction changes nothing observable. 4 · Notes (not blockers)
Verdict: LGTM — pure, regression-tested extraction; no behavior change. 👍 🇨🇳 中文说明(点击展开)✅ 真实二进制验证(合并参考)我从本 PR 的 head( 验证分四层:
1 · 单测 + 变异
为证明新测试真的守住控制流,我对源码做变异后重跑:
P1 正是 review 阶段补的 2 · 真实 E2E —— 四条路径在运行中实时触发(无需重启)我安装的探针扩展同时打包了 MCP server、技能、子代理和
在管理弹窗里禁用它会再次运行 3 · A/B 等价 —— 内联 vs 委托同一个探针、同一段关键序列,跑在重构前二进制(把
每一行都完全相同。 这次抽取不改变任何可观测行为。 4 · 备注(非阻塞)
结论:LGTM —— 纯粹、且有变异回归保护的抽取;无行为变化。👍 Verified on macOS · Node 22 · real |
Add block comment documenting the three-tier error-handling contract (fatal/swallow/swallow) so future maintainers know which tier applies when adding new refresh steps. Addresses review feedback on QwenLM#6152.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
This change touches core infrastructure at scale (539 lines across packages/core/src/extension/). Per the project's core module protection policy, core refactors of this size must be maintainer-initiated.
Please open an issue to discuss the design first, or coordinate with a maintainer to champion this change.
中文说明
此 PR 涉及核心基础设施的大规模改动(539 行,跨越 packages/core/src/extension/)。根据项目的核心模块保护策略,此规模的核心重构必须由维护者发起。
请先开一个 issue 讨论设计方案,或与维护者协调由他们来推动此变更。
— Qwen Code · qwen3.7-max
False positive: Stage 0 counted total PR lines instead of protected core production lines. Docs/tests should not trigger the 500-line core hard block.
|
@qwen-code /review --timeout=180 |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28732595815)._ |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Clean extraction — control flow, error-handling contract (fatal/swallow/swallow tiers), and Promise.allSettled semantics are faithfully preserved. Good test coverage across all error paths. The plan doc adds useful context for the follow-up PRs in #3696.






What this PR does
Extracts the body of
ExtensionManager.refreshMemory()into a standalone, exportedrefreshExtensionRuntime()function in a newpackages/core/src/extension/extension-runtime-refresh.tsmodule. The function takesExtensionRuntimeRefreshConfig, aPick<Config, 'getToolRegistry' | 'getSkillManager' | 'getSubagentManager' | 'refreshHierarchicalMemory'>view ofConfig, so it can run without anExtensionManagerinstance.ExtensionManager.refreshMemory()now just delegates to it. This is a pure extraction: the restart-MCP-servers step, thePromise.allSettledrefresh of skills/subagents, and the awaited try/catch around hierarchical memory refresh are all preserved exactly as before, with no behavior change.Why it's needed
Part of #3696 (comprehensive hot-reload system), specifically the sub-task for a single orchestrator that refreshes subsystem caches and runtime state in a predictable order. Today that refresh logic is inlined inside
ExtensionManager, which makes it unreachable for planned future call sites: a manual/reload-pluginscommand for external file changes, and additional refresh steps for commands, hooks, and LSP servers. This PR is step 1 of the sequenced plan indocs/plans/2026-07-01-extension-runtime-refresh-implementation.md("Shared Extension Runtime Refresh Orchestrator"): move the current refresh sequence behind one function with no behavior change, so later PRs have a single place to attach the additional refresh steps and model-facing notifications.Reviewer Test Plan
How to verify
Run the existing extension manager suite plus the new orchestrator test, and confirm the project still typechecks cleanly:
Both suites pass (59 tests). Manually, toggling an extension (enable/disable/install/update/uninstall) should still refresh MCP servers, skills, subagents, and hierarchical memory exactly as before — there is no observable behavior change, since
refreshMemory()now only delegates to the extracted function.Evidence (Before & After)
N/A — internal refactor, no user-visible or TUI change.
Tested on
Environment (optional)
Local:
npm run devon macOS, Node 22.Risk & Scope
refreshMemory()body (samePromise.allSettledsemantics, same try/catch aroundrefreshHierarchicalMemory, samerestartMcpServersordering).refreshExtensionRuntimeandExtensionRuntimeRefreshConfigare new exports fromextension-runtime-refresh.ts; they are not yet re-exported from the package barrel, since no consumer outsidepackages/core/src/extension/needs them yet.Linked Issues
Part of #3696
中文说明
这个 PR 做了什么
把
ExtensionManager.refreshMemory()的方法体抽取成一个独立导出的refreshExtensionRuntime()函数,放进新文件packages/core/src/extension/extension-runtime-refresh.ts。该函数接收ExtensionRuntimeRefreshConfig,即Pick<Config, 'getToolRegistry' | 'getSkillManager' | 'getSubagentManager' | 'refreshHierarchicalMemory'>,因此可以在没有ExtensionManager实例的情况下运行。ExtensionManager.refreshMemory()现在只是委托给它。这是一次纯粹的抽取:重启 MCP 服务器的步骤、通过Promise.allSettled刷新 skills/subagents、以及对 hierarchical memory 刷新的 await + try/catch,行为都与之前完全一致,没有任何变化。为什么需要
属于 #3696(全面的热重载系统)的一部分,具体是其中"统一的缓存/运行时状态刷新编排器"子任务。目前这部分刷新逻辑写死在
ExtensionManager内部,导致未来的调用点(比如面向外部文件变更的手动/reload-plugins命令,以及 commands/hooks/LSP 的额外刷新步骤)无法复用它。这个 PR 是docs/plans/2026-07-01-extension-runtime-refresh-implementation.md中规划的第一步("共享的 Extension Runtime 刷新编排器"):把现有的刷新流程收进一个函数、不改变任何行为,这样后续 PR 就有了一个统一的地方去挂接更多刷新步骤和面向模型的通知。Reviewer Test Plan
(测试步骤同上,此处省略以保持可读性。)
风险与范围
refreshMemory()方法体在行为上完全一致(相同的Promise.allSettled语义、相同的refreshHierarchicalMemorytry/catch、相同的restartMcpServers执行顺序)。refreshExtensionRuntime和ExtensionRuntimeRefreshConfig是extension-runtime-refresh.ts新增的导出,目前还没有从包的 barrel 文件中重新导出,因为暂时没有packages/core/src/extension/之外的调用方需要它们。关联 Issues
属于 #3696 的一部分