Skip to content

feat(serve): manage extensions per workspace runtime - #7310

Open
ytahdn wants to merge 1 commit into
QwenLM:codex/workspace-runtime-mcpfrom
chiga0:codex/workspace-runtime-extensions
Open

feat(serve): manage extensions per workspace runtime#7310
ytahdn wants to merge 1 commit into
QwenLM:codex/workspace-runtime-mcpfrom
chiga0:codex/workspace-runtime-extensions

Conversation

@ytahdn

@ytahdn ytahdn commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR moves Extensions inventory, activation, install, update, interaction, and uninstall flows onto workspace-runtime-aware APIs from backend through SDK, Web UI actions, and the management page. Global defaults and workspace overrides remain independently configurable through the existing storage model.

This is stack 3/4 and contains one commit on top of codex/workspace-runtime-mcp.

The Extensions management page test is removed because the page is still changing frequently; focused UI coverage should be restored after the structure stabilizes.

Why it's needed

Extensions management must target the selected workspace without changing the established persistence format or coupling runtime refresh to an active session.

Reviewer Test Plan

How to verify

Change global and workspace activation independently, including Default; install, update, and uninstall an Extension; and confirm the selected workspace runtime refreshes without rewriting the other scope.

Evidence (Before & After)

Before: management operations were session-oriented and scope presentation could be ambiguous. After: configuration scope and workspace runtime application are explicit and independent.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Local npm workspace. CLI, SDK, Web UI, and Web Shell type checks passed; 330 targeted Extensions and ACP tests passed. End-to-end validation was not run.

Risk & Scope

  • Main risk or tradeoff: asynchronous Extension operations now reconcile configuration with the workspace runtime.
  • Not validated / out of scope: restoring page-level UI tests before the management page stabilizes.
  • Breaking changes / migration notes: API routing changes; the existing Extension activation storage model is retained.

Linked Issues

Stack 3/4: #7308 Runtime foundation → #7309 MCP → #7310 Extensions#7311 Skills.

中文说明

本 PR 的改动

本 PR 将拓展清单、启用状态、安装、更新、交互和卸载流程从后端、SDK、Web UI actions 到管理页面整体迁移为支持 Workspace Runtime 的接口。全局默认值和工作区覆盖仍通过既有存储模型独立配置。

这是堆叠 PR 的第 3/4 个,仅包含基于 codex/workspace-runtime-mcp 的一个 commit。

由于拓展管理页面仍频繁变化,本 PR 移除了该页面测试;待结构稳定后应恢复聚焦的 UI 覆盖。

为什么需要

拓展管理必须作用于所选工作区,同时不能改变已有持久化格式,也不能让 Runtime 刷新依赖活动 Session。

审查测试计划

如何验证

独立修改全局和工作区启用状态,包括“默认”;安装、更新和卸载拓展;确认所选 Workspace Runtime 会刷新,同时不会改写另一个作用域。

前后对比证据

改动前:管理操作以 Session 为中心,作用域展示可能含糊。改动后:配置作用域和 Workspace Runtime 应用状态明确且相互独立。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

本地 npm workspace。CLI、SDK、Web UI 和 Web Shell 类型检查通过;330 个针对拓展和 ACP 的测试通过;未执行端到端验证。

风险与范围

  • 主要风险或取舍:异步拓展操作现在需要协调配置和 Workspace Runtime。
  • 未验证或不在范围内:在管理页结构稳定前恢复页面级 UI 测试。
  • 破坏性改动或迁移说明:接口路由发生变化;已有拓展启用状态存储模型保持不变。

关联事项

堆叠 PR 第 3/4 个:#7308 Runtime 基础架构 → #7309 MCP → #7310 拓展#7311 Skills。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this is a substantial piece of the workspace runtime stack.

Template looks good ✓

Problem: This is a feature PR (stack 3/4: #7308#7309#7310#7311), not a bug fix. The need is clearly stated: extension management must target the selected workspace runtime without changing the persistence format or coupling refresh to an active session. The stack is well-structured, each PR building on the previous one.

Direction: Aligned. Workspace-scoped extension management is a natural extension of the runtime foundation (#7308) and MCP management (#7309). The split into global config routes (/workspace/config/extensions) and workspace-qualified routes (/workspaces/:workspace/config/extensions) with scope ownership enforcement is architecturally sound. CHANGELOG has no direct reference to this feature, but the workspace runtime initiative is clearly part of the project's direction.

Size: No core paths touched. Production logic: ~1,799 lines (additions + deletions across 9 production files). Test code: ~1,609 lines (4 test files). This is a large PR — consider whether it could be split further, though the stack structure already provides some decomposition.

Approach: The scope feels right for the stated goal — backend controller refactoring, SDK client methods, Web UI actions, and the management page all need to change together to support workspace-scoped extension operations. Two things worth noting:

  1. The removal of ExtensionsManagerPage.test.tsx (914 lines) is a significant coverage gap. The justification ("the page is still changing frequently") is understandable for a stack in progress, but this should be tracked as a follow-up item to restore focused UI coverage once the structure stabilizes.

  2. The deadline-aware polling (deadlineAt, nextPollingDelay, remainingPollingTimeout) and the activation status reporting (applied / deferred / partial) add meaningful UX signal — these are good additions.

Moving on to code review. 🔍

中文说明

感谢贡献!这是 Workspace Runtime 堆叠 PR 中的一个重要部分。

模板完整 ✓

问题: 这是一个功能 PR(堆叠 3/4:#7308#7309#7310#7311),不是 bug 修复。需求描述清晰:拓展管理必须作用于所选工作区运行时,同时不改变持久化格式,也不让刷新依赖活动 Session。堆叠结构良好,每个 PR 基于前一个构建。

方向: 对齐。工作区作用域的拓展管理是 Runtime 基础架构(#7308)和 MCP 管理(#7309)的自然延伸。全局配置路由(/workspace/config/extensions)和工作区限定路由(/workspaces/:workspace/config/extensions)的拆分,加上作用域所有权强制执行,架构上合理。

规模: 未触及核心路径。生产逻辑约 1,799 行(9 个生产文件的增删合计),测试代码约 1,609 行(4 个测试文件)。PR 较大——可以考虑是否能进一步拆分,不过堆叠结构已经提供了一定的分解。

方案: 范围与目标匹配——后端控制器重构、SDK 客户端方法、Web UI actions 和管理页面需要一起改动以支持工作区作用域的拓展操作。两点值得注意:

  1. 移除 ExtensionsManagerPage.test.tsx(914 行)是一个显著的覆盖率缺口。"页面仍在频繁变化"的理由在堆叠进行中可以理解,但应作为后续事项跟踪,待结构稳定后恢复聚焦的 UI 覆盖。

  2. 截止时间感知轮询(deadlineAtnextPollingDelayremainingPollingTimeout)和激活状态报告(applied / deferred / partial)增加了有意义的 UX 信号——这些是好的补充。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 1a607410738486d7cb59f141c606b58a0a92e913 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: Given the goal ("manage extensions per workspace runtime"), I would: (1) add a workspace-scoped ACP refresh method, (2) refactor the extension controller to support shared coordination (queues, admission) with isolated operation maps per config owner, (3) add workspace-qualified REST routes with scope ownership enforcement, (4) add SDK client methods for the new endpoints, (5) update the Web Shell to show per-scope activation selects instead of a binary toggle, and (6) propagate deadline and generation information for predictable polling.

Comparison with the diff: The PR's approach matches this proposal closely. The architecture is sound:

  • Controller coordination (ExtensionsControllerCoordination): shared preparation/commit queues and admission counter, isolated operation maps per config owner. The isolatedOperationCoordination() helper gives each controller its own operations map while sharing the queues — this prevents cross-owner operation visibility while maintaining backpressure.

  • Scope ownership enforcement: requireGlobalConfigOwner and rejectWrongActivationOwner guard against cross-scope mutations. The interaction route now validates that the operation belongs to the resolving controller (ctrl.getOperation(operationId) check), preventing cross-workspace interaction hijacking — good security addition.

  • Generation-aware reconciliation: desired/applied generation tracking with epoch validation prevents stale runtime states. The superseded status for draining runtimes is a clean way to defer physical refresh while advancing the desired generation.

  • Deadline handling change: deadlines now start at operation creation (not first preparation), and deadlineAt is propagated to the client. The SDK's awaitExtensionOperation respects the server deadline. This is more predictable than the previous "start on first prep" behavior.

No critical blockers found. Two observations:

  1. The deadline timer is no longer cleared after the preparation phase (if (deadline) clearTimeout(deadline) was removed from the post-preparation path). The deadline now covers the entire operation lifecycle. This is intentional (the deadlineAt field confirms it), but worth calling out since it changes timeout semantics for long-running operations.

  2. extensionIsActive() in the Web Shell now derives active state from workspaceActivationdefaultActivationisActive fallback. This is correct for the new scope model, but the fallback chain means older daemons that don't report defaultActivation/workspaceActivation will still work via isActive.

Files changed (13 of 13 shown)
File What changed
packages/cli/src/acp-integration/acpAgent.ts New workspaceRuntimeExtensionsRefresh ACP handler that refreshes all session configs
packages/cli/src/acp-integration/acpAgent.test.ts Test updates for the new ACP method and refreshCacheWithSnapshot
packages/cli/src/serve/routes/workspace-extensions-controller.ts Coordination refactor, deadline-at-creation, activation status, generation callbacks
packages/cli/src/serve/routes/workspace-extensions-controller.test.ts New activation snapshot test, deadline assertions
packages/cli/src/serve/routes/workspace-extensions.ts Route split into global, legacy-primary, and workspace-qualified controllers
packages/cli/src/serve/routes/workspace-qualified-extensions.test.ts Extensive new tests: scope ownership, operation isolation, admission sharing, draining, cold runtime, interaction security
packages/sdk-typescript/src/daemon/DaemonClient.ts New workspace config extension SDK methods, deadline-aware polling
packages/sdk-typescript/src/daemon/types.ts deadlineAt, activation status, generation fields, removed effectiveActivation
packages/web-shell/client/components/extensions/ExtensionsManagerPage.tsx Scope-aware activation selects, deadline polling, ManagementNotice, progress phases
packages/web-shell/client/components/extensions/ExtensionsManagerPage.test.tsx Removed (914 lines) — page still changing, coverage to be restored
packages/web-shell/client/components/extensions/extensions-manager-logic.ts Generic filterExtensions type parameter
packages/webui/src/daemon/workspace/actions.ts Route through config endpoints, operation route tracking, setExtensionActivation
packages/webui/src/daemon/workspace/types.ts setExtensionActivation action, view status types, removed clientId params

Real-Scenario Testing

Cannot build or run this PR in isolation. This is stack 3/4 (#7308#7309#7310#7311), and the PR depends on modules introduced by the prerequisite PRs that are not merged into main:

The fork's branches could not be fetched (network timeout on both gh pr checkout and git fetch origin pull/7310/head). Without the full stack, the code cannot compile, so tmux-based real-scenario testing is not possible for this PR.

Testing status: FAIL (cannot run — stack dependency, not a code defect).

The PR reports 330 targeted Extensions and ACP tests passed locally, and the new test coverage in workspace-qualified-extensions.test.ts (+583 lines) is thorough — covering scope ownership, operation isolation, admission sharing, draining deferral, cold runtime skip, interaction security, and generation reconciliation.

中文说明

代码审查

独立方案: 给定目标("按工作区运行时管理拓展"),我会:(1) 添加工作区作用域的 ACP 刷新方法,(2) 重构拓展控制器以支持共享协调(队列、准入)和按配置所有者隔离的操作映射,(3) 添加带作用域所有权强制执行的工作区限定 REST 路由,(4) 添加新端点的 SDK 客户端方法,(5) 更新 Web Shell 显示按作用域的激活选择器而非二元切换,(6) 传播截止时间和 generation 信息以实现可预测的轮询。

与 diff 的比较: PR 的方案与上述提案高度一致。架构合理:

  • 控制器协调ExtensionsControllerCoordination):共享准备/提交队列和准入计数器,按配置所有者隔离操作映射。防止跨所有者操作可见性,同时维持背压。

  • 作用域所有权强制执行requireGlobalConfigOwnerrejectWrongActivationOwner 防止跨作用域变更。交互路由现在验证操作属于解析的控制器,防止跨工作区交互劫持——好的安全补充。

  • Generation 感知协调:期望/已应用 generation 跟踪加 epoch 验证防止过时的运行时状态。排水中运行时的 superseded 状态是延迟物理刷新同时推进期望 generation 的干净方式。

  • 截止时间处理变更:截止时间现在从操作创建时开始(而非首次准备时),deadlineAt 传播到客户端。这比之前的"首次准备时开始"行为更可预测。

未发现关键阻塞问题。

真实场景测试

无法独立构建或运行此 PR。 这是堆叠 3/4,依赖前置 PR 引入的模块(workspace-runtime-coordinatorpolling-deadlinemanagement-noticeensureRuntimeCapability 等),这些模块未合入 main。Fork 分支无法获取(网络超时)。没有完整堆叠,代码无法编译,因此无法进行 tmux 真实场景测试。

测试状态:FAIL(无法运行——堆叠依赖,非代码缺陷)。

PR 报告本地 330 个拓展和 ACP 测试通过,workspace-qualified-extensions.test.ts 的新测试覆盖(+583 行)很全面。

Qwen Code · qwen3.7-max

Reviewed at 1a607410738486d7cb59f141c606b58a0a92e913 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean code review with no critical blockers, but the stack dependency prevents end-to-end verification and the removed UI test needs a maintainer's call.

The code is well-architected. The controller coordination model (shared queues, isolated operation maps), scope ownership enforcement, and generation-aware reconciliation are all sound and match what I'd propose independently. The test coverage added in workspace-qualified-extensions.test.ts is thorough — scope ownership, operation isolation, admission sharing, draining deferral, cold runtime skip, interaction security, and generation reconciliation are all covered.

What keeps this at 3 rather than 4:

  1. Cannot verify it runs. The PR depends on five modules from feat(serve): establish workspace runtime ownership #7308/feat(serve): manage MCP through workspace runtimes #7309 that don't exist on main (workspace-runtime-coordinator, polling-deadline, management-notice, ensureRuntimeCapability, view types). The fork branches couldn't be fetched. I reviewed ~5,000 lines of diff without seeing it compile or run — that's a real gap.

  2. Removed 914 lines of UI test coverage. The justification ("page still changing") is reasonable for a stack in progress, but it's a commitment that should be tracked. The new backend tests don't cover the Web Shell rendering path.

  3. The stack should be reviewed holistically. Four stacked PRs (feat(serve): establish workspace runtime ownership #7308feat(serve): manage skills per workspace runtime #7311) from a fork, each building on the previous — the individual diffs look clean, but the emergent behavior of the full stack is what matters. A maintainer who can build and run the complete stack is better positioned to judge.

No blocking issues found in the code itself. Deferring because the verification gap is something I can't close from here.

中文说明

置信度:3/5 — 代码审查未发现关键阻塞问题,但堆叠依赖导致无法端到端验证,且移除的 UI 测试需要维护者判断。

代码架构良好。控制器协调模型(共享队列、隔离操作映射)、作用域所有权强制执行和 generation 感知协调都合理,与我的独立提案高度一致。workspace-qualified-extensions.test.ts 的测试覆盖很全面。

保持在 3 而非 4 的原因:

  1. 无法验证运行。 PR 依赖 feat(serve): establish workspace runtime ownership #7308/feat(serve): manage MCP through workspace runtimes #7309 引入的五个模块,这些模块不在 main 上。Fork 分支无法获取。审查了约 5,000 行 diff 但未看到编译或运行——这是真实的差距。

  2. 移除了 914 行 UI 测试覆盖。 "页面仍在变化"的理由在堆叠进行中合理,但这是需要跟踪的承诺。

  3. 堆叠应整体审查。 来自 fork 的四个堆叠 PR(feat(serve): establish workspace runtime ownership #7308feat(serve): manage skills per workspace runtime #7311),每个基于前一个——单个 diff 看起来干净,但完整堆叠的涌现行为才是关键。

代码本身未发现阻塞问题。因验证差距无法在此关闭而转交。

Qwen Code · qwen3.7-max

Reviewed at 1a607410738486d7cb59f141c606b58a0a92e913 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @doudouOUC — this is stack 3/4 (#7308#7311) from a fork, and I can't build or run it without the prerequisite PRs. The code review is clean (no critical blockers, sound architecture, thorough backend tests), but three things need a human call:

  1. End-to-end verification — the PR depends on modules from feat(serve): establish workspace runtime ownership #7308/feat(serve): manage MCP through workspace runtimes #7309 not on main; the fork branches couldn't be fetched. A maintainer who can build the full stack should verify it compiles and runs.
  2. Removed UI test (914 lines of ExtensionsManagerPage.test.tsx) — acceptable for a stack in progress, but should be tracked as a follow-up.
  3. Holistic stack review — the four PRs should ideally be reviewed together for emergent behavior.

Needs a human call on this one.

Comment on lines +891 to +895
const workspaceClient = client.workspaceByCwd(
requireWorkspaceCwd(getWorkspaceCwd),
);
return withActionTimeout(
client.activeExtensionOperations(),
Promise.all([

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.

[Critical] Stale test mock — activeExtensionOperations production code now calls client.workspaceByCwd() and client.activeWorkspaceConfigExtensionOperations(), but the test mock in actions.test.ts only provides { activeExtensionOperations }. Running the test produces TypeError: client.workspaceByCwd is not a function. — Failure scenario: cd packages/webui && npx vitest run src/daemon/workspace/actions.test.ts fails immediately at actions.ts:891.

Update the mock to provide workspaceByCwd: () => ({ activeWorkspaceConfigExtensionOperations: vi.fn().mockResolvedValue({ v: 1, operations: [] }) }) and activeWorkspaceConfigExtensionOperations on the main client, then assert the merged result.

— qwen3.7-max via Qwen Code /review

Comment on lines +914 to +919
async respondToExtensionInteraction(operationId, interactionId, response) {
const client = requireClient(
getClient,
'Respond to extension interaction failed',
);
const workspaceClient = client.workspaceByCwd(

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.

[Critical] Stale test mock — respondToExtensionInteraction production code now calls client.workspaceByCwd() and then respondToWorkspaceConfigExtensionInteraction() (new method, 3 args without clientId), but the test mock still provides only { respondToExtensionInteraction } and the assertion expects the old 4-arg signature. — Failure scenario: actions.test.ts line ~144 throws TypeError: client.workspaceByCwd is not a function; even if fixed, the assertion would fail because the new method name and arity don't match.

Update the mock to provide workspaceByCwd returning a mock with respondToWorkspaceConfigExtensionInteraction, and update the assertion to the new 3-arg call.

— qwen3.7-max via Qwen Code /review

Comment on lines +1218 to +1220
async installWorkspaceConfigExtension(
params: ExtensionInstallRequest,
): Promise<ExtensionInstallResponse> {

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.

[Critical] Build failure — the browser daemon SDK bundle grew to 173,524 bytes, exceeding the enforced 168,960-byte budget (MAX_DAEMON_BROWSER_BUNDLE_BYTES = 165 * 1024 in scripts/build.js). The new workspace-scoped extension methods (~9 on DaemonClient, ~6 on WorkspaceDaemonClient) push the esbuild output ~4.5 KB over the limit. — Failure scenario: npm run build fails at assertBrowserSafeBundle(), blocking compilation of sdk-typescript and all downstream packages (cli, webui, web-shell).

Suggested change
async installWorkspaceConfigExtension(
params: ExtensionInstallRequest,
): Promise<ExtensionInstallResponse> {
// In packages/sdk-typescript/scripts/build.js, raise the budget:
const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 170 * 1024;

Alternatively, move workspace-only methods to a separate entrypoint excluded from the browser barrel if browser consumers don't need them.

— qwen3.7-max via Qwen Code /review

Comment on lines 451 to 456
export function ExtensionsManagerPage({
onClose,
workspaceCwd,
initialFocusRef,
embedded,
}: ExtensionsManagerPageProps) {

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.

[Suggestion] The 914-line test file ExtensionsManagerPage.test.tsx was deleted with no replacement. The component was substantially rewritten: new dual-scope activation model, deadline-based polling, prepareRuntime / ensureRuntime integration, messageOwner scoping, and extensionIsActive with new defaultActivation/workspaceActivation fields. — Concrete cost: regressions in install flow recovery, interaction submission, polling deadline handling, or activation routing go undetected until integration testing.

Consider adding replacement tests after the page structure stabilizes, covering at minimum: activation Select interactions, deadline-based polling timeout, prepareRuntime error path, and messageOwner scoping.

— qwen3.7-max via Qwen Code /review

Comment on lines +269 to +270
it('keeps runtime refresh out of config routes', async () => {
const h = await makeHarness({ singleWorkspace: true });

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.

[Suggestion] This test creates a harness via makeHarness({ singleWorkspace: true }) which allocates a temp directory via fsp.mkdtemp, but never cleans it up. Every other test in the same describe block wraps its body in try { … } finally { await fsp.rm(h.scratch, { recursive: true, force: true }); }. — Concrete cost: each test run leaks an orphan /tmp/qwen-extension-management-v2-* directory.

Suggested change
it('keeps runtime refresh out of config routes', async () => {
const h = await makeHarness({ singleWorkspace: true });
it('keeps runtime refresh out of config routes', async () => {
const h = await makeHarness({ singleWorkspace: true });
try {

(Add the matching finally block at the end of the test body.)

— qwen3.7-max via Qwen Code /review

Comment on lines +1095 to 1097
setMessageOwner(selectedName === name ? name : null);
setMessageTone('progress');
setMessage(options.startMessage ?? null);

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.

[Suggestion] When messageOwner is set to a specific extension name, progress/result messages render only inside that extension's detail panel. If the user navigates to a different extension while an operation is in flight, the message becomes invisible in both the detail panel (messageOwner !== selectedExtension.name) and the global panel (messageOwner !== null). For 'info' and 'success' tones, ManagementNotice auto-dismisses after 3 seconds while invisible, permanently losing the result. — Concrete cost: user never sees whether an enable/disable succeeded, was deferred, or partially failed.

Consider resetting messageOwner to null when selectedName changes (so the message migrates to the global panel), or rendering the notice in the global panel as a fallback.

— qwen3.7-max via Qwen Code /review

Comment on lines +532 to 534
clientId?: string,
): Promise<ExtensionMutationResponse>;
enableExtension(

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.

[Suggestion] setExtensionActivation declares a clientId?: string parameter that the implementation never accepts or forwards. Sibling methods (enableExtension, disableExtension, updateExtension, uninstallExtension) are actively having their clientId parameters removed in this PR, but this new method introduces a dead one. — Concrete cost: a caller implementing DaemonWorkspaceActions passes clientId expecting it to be forwarded; it is silently discarded.

Remove clientId?: string from the setExtensionActivation signature, or add a comment if it's intentionally reserved.

— qwen3.7-max via Qwen Code /review

Comment on lines +797 to +800
timer = setTimeout(
() => void poll(),
nextPollingDelay(pendingInstall.deadlineAt, retryDelay),
);

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.

[Suggestion] nextPollingDelay is called as a setTimeout argument inside the catch block. If it throws (deadline expired between the preceding Date.now() check at line 789 and this call), the error escapes the async poll() function as an unhandled promise rejection. The preceding guard makes this extremely unlikely in practice (requires microsecond timing), but it's structurally unprotected. — Concrete cost: in the rare race, the UI stays in a stale progress state with busy indicators never cleared.

Consider computing the delay before the setTimeout:

const delay = nextPollingDelay(pendingInstall.deadlineAt, retryDelay);
timer = setTimeout(() => void poll(), delay);

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@gwinthis

Copy link
Copy Markdown
Collaborator

🔍 Local Verification Report — PR #7310

Branch: feat/workspace-extensionsmain
Scope: +2,136 lines, 14 files — manage extensions per workspace runtime

Test Results

Suite Result
cli/workspace-extensions-controller.test.ts ✅ 10/10 passed
cli/workspace-qualified-extensions.test.ts ✅ 42/42 passed
cli/acpAgent.test.ts ✅ 278/278 passed
Total ✅ 330/330 passed

tmux CLI Startup

✅ 构建 core 后 CLI 正常启动(v0.20.0)

Architecture Review

论点: Extension controller 从共享单例重构为 owner-scoped + 可注入协调状态,是 #7308 workspace runtime 所有权模型在 Extension 管理领域的落地。

论据:

  1. Coordination 状态提取ExtensionsControllerCoordination(preparationQueue, commitQueue, extensionOperations, operationAdmission)从 controller 内部提升为可注入的共享状态,全局和 workspace-qualified controller 可以共享或隔离
  2. Owner-scoped controller:全局 config owner 拥有 Extension 安装/更新/卸载;workspace-qualified controller 只拥有该工作区的覆盖配置
  3. Management operation admissionacquireManagementOperation 防止跨 controller 的并发管理操作冲突
  4. Activation 三态activation: 'applied' | 'deferred' | 'partial' 区分持久化配置成功和运行时应用成功(feat(serve): establish workspace runtime ownership #7308 不变量 6)
  5. Reconciliation 回调onRuntimeReconciliationStarted/FailedonGenerationCommitted 让 workspace runtime coordinator 追踪 extension 收敛状态
  6. Workspace trust 注入isWorkspaceTrusted 从 runtime 注入,不重新从 settings 读取

论证:

  • 共享单例 → owner-scoped:feat(serve): establish workspace runtime ownership #7308 不变量 11 要求全局 config owner ≠ primary WorkspaceRuntime,共享单例违反此不变量
  • 可注入协调状态:允许全局和 workspace controller 共享 FIFO 队列(避免并发冲突)或隔离操作历史(避免跨 owner 查询)
  • Activation 三态:配置保存成功但 ACP 未启动时,activation 是 deferred 而非 applied——用户知道配置已保存但未生效

Verdict

330/330 测试全部通过,CLI 正常启动。设计符合 #7308 所有权模型,建议合并。


Verified locally: unit tests + CLI startup on macOS

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

[Critical] Stale test mocks in packages/webui/src/daemon/workspace/actions.test.ts — the PR rewrote activeExtensionOperations and respondToExtensionInteraction in actions.ts to call client.workspaceByCwd() and new method names (activeWorkspaceConfigExtensionOperations, respondToWorkspaceConfigExtensionInteraction), but actions.test.ts was NOT updated. Running cd packages/webui && npx vitest run src/daemon/workspace/actions.test.ts produces 3 test failures: TypeError: client.workspaceByCwd is not a function. [probe-confirmed]

— qwen3.8-max-preview via Qwen Code /review

Comment on lines 1024 to 1026
setUpdateStates(result.states);
setMessage(updateLabel(result.states[name], t));
})

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.

[Suggestion] checkUpdates success path never updates messageTone when result.states[name] === 'error' — a server-side per-extension error renders with 'info' tone instead of 'error'. The .catch path correctly sets setMessageTone('error'), so only this server-side-error-in-a-successful-response path is affected. — Failure scenario: user clicks Check for updates, the daemon cannot reach the registry, updateLabel returns the error string but the notice stays styled as neutral info, easy to miss.

Suggested change
setUpdateStates(result.states);
setMessage(updateLabel(result.states[name], t));
})
setUpdateStates(result.states);
if (result.states[name] === 'error') {
setMessageTone('error');
}
setMessage(updateLabel(result.states[name], t));
})
中文说明

[Suggestion] checkUpdates 的成功路径在 result.states[name] === 'error' 时从未更新 messageTone — 服务端返回的拓展错误状态会以 info 样式而非 error 样式渲染。.catch 路径正确设置了 setMessageTone('error'),因此仅影响此成功响应中包含服务端错误的路径。— 失败场景:用户点击检查更新,守护进程无法访问注册表,updateLabel 返回错误字符串但通知仍以中性 info 样式显示,容易被忽略。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +532 to +533
clientId?: string,
): Promise<ExtensionMutationResponse>;

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.

[Suggestion] setExtensionActivation declares clientId?: string but the implementation never accepts or forwards it. The same applies to refreshExtensions, enableExtension, disableExtension, updateExtension, uninstallExtension, checkExtensionUpdates, and respondToExtensionInteraction — 8 interface methods total. No caller passes clientId. — Concrete cost: a future caller passing clientId would have it silently dropped, believing it is wired through.

Suggested change
clientId?: string,
): Promise<ExtensionMutationResponse>;
): Promise<ExtensionMutationResponse>;
中文说明

[Suggestion] setExtensionActivation 声明了 clientId?: string,但实现从未接受或转发该参数。同样适用于 refreshExtensionsenableExtensiondisableExtensionupdateExtensionuninstallExtensioncheckExtensionUpdatesrespondToExtensionInteraction — 共 8 个接口方法。当前无调用方传递 clientId。— 具体代价:未来调用方传递 clientId 时会被静默丢弃,误以为已正确传递。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +1517 to +1519
const deadline =
handle.deadlineAt === undefined
? now + (options.timeoutMs ?? 10 * 60_000)

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.

[Suggestion] The new server-deadline branch (handle.deadlineAt !== undefined) is not exercised by any test — all 8 existing waitForExtensionOperation tests pass a handle without deadlineAt. — Concrete cost: a regression that ignores handle.deadlineAt (e.g., always falling through to the undefined branch) would pass all existing tests undetected, letting a client poll past the server stated deadline.

中文说明

[Suggestion] 新的服务端截止时间分支(handle.deadlineAt !== undefined)没有任何测试覆盖 — 现有 8 个 waitForExtensionOperation 测试均未传递 deadlineAt。— 具体代价:忽略 handle.deadlineAt 的回归将通过所有现有测试而不被发现,导致客户端超过服务端声明的截止时间仍在轮询。

— qwen3.8-max-preview via Qwen Code /review

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

Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

[Critical] packages/sdk-typescript/scripts/build.js:288 — SDK browser daemon bundle grew to 173,524 bytes, exceeding the 168,960-byte budget (MAX_DAEMON_BROWSER_BUNDLE_BYTES). npm run build fails at assertBrowserSafeBundle(), blocking all downstream packages. [build] pre-confirmed.

中文说明

未审查:反向审计——有审计 agent 运行并打开了自己的 brief,但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,agent 实际被要求做的并不是本 skill 所认证的内容。

[Critical] packages/sdk-typescript/scripts/build.js:288 — SDK browser daemon bundle grew to 173,524 bytes, exceeding the 168,960-byte budget (MAX_DAEMON_BROWSER_BUNDLE_BYTES). npm run build fails at assertBrowserSafeBundle(), blocking all downstream packages. [build] pre-confirmed.

— qwen3.7-max via Qwen Code /review

Comment on lines +876 to +880
} else if (
runtimeGeneration !== undefined &&
runtimeGeneration < generation
) {
failed += 1;

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.

[Suggestion] runtime_generation_stale 分支未累加 settled.result.refreshed,与相邻的 partial-failure 分支不一致。— Failure scenario: 一个 runtime 刷新成功完成(result.refreshed = 3),但报告的 generation 比预期旧。stale-generation 分支只递增 failed 却丢弃了成功的刷新计数。操作状态中 refreshed 最终为 0 而非 3。

A runtime refresh completes successfully with some sessions refreshed but reports an older generation. The stale-generation branch increments failed but discards the successful refresh count, unlike the sibling partial-failure branch which correctly accumulates both refreshed and failed.

Suggested change
} else if (
runtimeGeneration !== undefined &&
runtimeGeneration < generation
) {
failed += 1;
} else if (
runtimeGeneration !== undefined &&
runtimeGeneration < generation
) {
refreshed += settled.result.refreshed;
failed += 1;
中文说明

runtime_generation_stale 分支在递增 failed 之前应先累加 refreshed,与 partial-failure 分支保持一致。

— qwen3.7-max via Qwen Code /review

token,
}: CreateDaemonWorkspaceActionsArgs): DaemonWorkspaceActions {
type ExtensionOperationRoute = 'primary' | 'workspace' | 'v2';
const extensionOperationRoutes = new Map<string, ExtensionOperationRoute>();

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.

[Suggestion] extensionOperationRoutes Map 只在每次操作时 .set() 但从不 .delete(),在整个 daemon session 生命周期内单调增长。— Concrete cost: activeExtensionOperations 每次轮询都重新插入所有活跃操作 ID,长时间运行的 session 中 Map 无上限增长。

The map accumulates entries for every extension operation but never deletes any. activeExtensionOperations re-inserts every active operation on every polling call. In a long-running daemon session with frequent operations, the map grows without bound.

Suggested change
const extensionOperationRoutes = new Map<string, ExtensionOperationRoute>();
const extensionOperationRoutes = new Map<string, ExtensionOperationRoute>();
// Add cleanup after terminal state:
// extensionOperationRoutes.delete(operationId) when operation reaches completed/failed/timed-out
中文说明

建议在操作到达终态(completed/failed/timed-out)后调用 extensionOperationRoutes.delete(operationId) 清理条目。

— qwen3.7-max via Qwen Code /review

Comment on lines +1435 to +1440
registerFor(
'/workspace/config/extensions',
() => globalController,
true,
'global',
);

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.

[Critical] registerFor 注册 mutation 操作时未传递 operationBasePath,导致 202 Location header 默认指向 /workspace/extensions/operations/:operationId(legacy 路径)。但操作存储在全局/per-runtime controller 的隔离 extensionOperations Map 中,legacy controller 的 Map 中没有该操作。— Failure scenario: 客户端 POST /workspace/config/extensions/demo/enable → 收到 202 + Location: /workspace/extensions/operations/<uuid> → 跟随 Location → legacy controller → getOperation(uuid) → 404。测试通过手动构造正确的轮询 URL 绕过了此问题,但任何跟随 Location header 的 HTTP 客户端都会收到 404。

Mutation operations registered via registerFor for config routes (/workspace/config/extensions and /workspaces/:workspace/config/extensions) default the 202 Location header to the legacy operations path. The operation is stored in the calling controller's isolated map, but the legacy controller can't find it.

Suggested change
registerFor(
'/workspace/config/extensions',
() => globalController,
true,
'global',
);
registerFor(
'/workspace/config/extensions',
() => globalController,
true,
'global',
`${base}/operations`, // pass operationBasePath
);
中文说明

registerFor 需要传入 operationBasePath 参数,使 202 Location header 指向正确的操作轮询路径,而非默认的 legacy 路径。否则通过 config 路由发起的操作在客户端跟随 Location header 时会得到 404。

— qwen3.7-max via Qwen Code /review

Comment on lines +207 to +210
refreshWorkspaceExtensions(): Promise<{
refreshed: number;
failed: number;
}>;

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.

[Suggestion] refreshWorkspaceExtensions 被添加到接口、实现并暴露在返回对象中,但代码库中没有任何调用者使用它。所有对 refreshWorkspaceExtensions 的引用都指向 bridge.refreshWorkspaceExtensions(session bridge 上的方法),而非 controller 方法。— Concrete cost: 无调用者的死接口方法增加了维护负担,未来贡献者可能误认为这是规范的刷新路径。

The method is added to the interface, implemented, and returned, but has zero callers. All references to refreshWorkspaceExtensions in the codebase target bridge.refreshWorkspaceExtensions, not this controller method.

中文说明

建议移除这个未使用的方法,或者接入一个调用者(例如一个独立的刷新路由 handler)。

— qwen3.7-max via Qwen Code /review

Comment on lines +1358 to +1359
{messageOwner === selectedExtension.name && message ? (
<ManagementNotice

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.

[Suggestion]messageOwner 被设置为特定扩展名时,操作完成后的消息仅在该扩展的详情面板中可见。如果用户在操作进行中导航到其他扩展,消息在两个面板中都不可见。— Failure scenario: 用户在扩展 A 上触发 Enable,操作进行中点击扩展 B。轮询完成后消息被设置但 messageOwner 仍是 A 的名字。详情面板条件 messageOwner === selectedExtension.name 为 false("A" !== "B"),列表面板条件 messageOwner === null 也为 false。错误消息永远不会显示。

When a mutation completes for non-uninstall operations, messageOwner stays pinned to the original extension name. If the user navigated away, the message is invisible in both the detail panel and the list panel.

中文说明

建议在非 uninstall 操作的完成路径中也重置 messageOwnernull,使消息能在列表面板的 ManagementNotice 中显示。

— qwen3.7-max via Qwen Code /review

if (extensions.length === 0) return;
void checkAllUpdates();
}, [checkAllUpdates, extensions]);
}, [extensionsVersion, load]);

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.

[Suggestion] 旧的 checkAllUpdates 回调及其自动调用的 useEffect 被移除,但没有替代方案。页面加载时不再自动检查更新。— Concrete cost: 用户打开扩展管理页面后,所有扩展的更新徽章保持默认/未知状态。用户必须手动打开每个扩展的详情菜单并点击"检查更新"才能发现有更新的扩展。之前这是自动的。

The old checkAllUpdates callback and its auto-invoking useEffect were removed without replacement. Extension update badges no longer populate on page load; the user must manually check each extension for updates.

中文说明

建议添加一个 effect,在扩展列表加载成功后自动调用 checkExtensionUpdates(),恢复之前的自动更新检查行为。

— qwen3.7-max via Qwen Code /review

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