Skip to content

fix(serve): keep skill slash commands available when the ACP child is unavailable - #6169

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:fix/workspace-skills-cache-when-channel-idle
Jul 2, 2026
Merged

fix(serve): keep skill slash commands available when the ACP child is unavailable#6169
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:fix/workspace-skills-cache-when-channel-idle

Conversation

@wenshao

@wenshao wenshao commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Makes the Web Shell's pre-first-prompt slash-command list keep skill-backed commands (e.g. /review) whenever the ACP child can't answer /workspace/skills, via two complementary daemon-side layers:

  1. Cache the last skills status a live child produced and replay it while no child is live, so a reaped child still yields the full, extension-aware list.
  2. Enumerate skills daemon-locally (straight from the filesystem via SkillManager, no child, no MCP init) as a fallback for when the child has never answered — so even a child that never comes up still yields the on-disk skills.

Why it's needed

/workspace/skills is answered only by the ACP child. requestWorkspaceStatus checks for an already-live channel (liveChannelInfo(), never ensureChannel()) and returns the idle placeholder (initialized: false, empty skills) when there is none. There is no daemon-local fallback like /workspace/providers has — so the pre-first-prompt list drops every skill and /rev stops autocompleting /review. The child is absent in three windows:

  • Before the first session — session creation is deferred until the first prompt (fix(web-shell): defer session creation until first prompt #6066).
  • After a session closes — the child is reaped immediately (--channel-idle-timeout-ms defaults to 0).
  • On a cold start, when preheat times out — most visible under npm run dev: the on-demand-transpiled child's initialize handshake routinely exceeds the 10s preheat budget (measured ~19s cold, vs a 10s cap), so preheat fails ("ACP preheat failed, will retry on first session: AcpSessionBridge initialize timed out after 10000ms") and no channel ever comes up.

The third window is why the reported symptom is "/review is not in the list, but it actually works": the skill exists on disk and runs when you submit /review in full (which spawns a session), but it never appears in autocomplete because the child hasn't answered /workspace/skills. The cache alone can't help there — it never warms without a live child. The daemon-local provider closes it: it reads skills from disk instantly, so /review is in the list from the first page load. The live child stays authoritative when present (and keeps extension-provided skills, which the daemon-local view omits).

This completes #6153, which wired the Web Shell to fetch /workspace/skills but could not surface skills the daemon was unable to answer without a live child.

Reviewer Test Plan

How to verify

Unit: npx vitest run packages/cli/src/serve/workspace-service/ packages/cli/src/serve/workspace-skills-status.test.ts — covers cache replay/refresh, the daemon-local fallback ordering (child → cache → local → empty), and that the local provider enumerates bundled /review with no child (76 tests). npx vitest run packages/cli/src/serve/server.test.ts stays green (574).

End-to-end (npm run dev:daemon, where preheat times out so the child is never live pre-prompt):

  1. Start it, open the Web Shell, and before sending any message type /rev.
  2. Press Tab.

Expected (this PR): /review is in the slash menu and Tab completes /rev/review [pr-number|file-path] [--comment]. Before: /review was absent and Tab did nothing.

Evidence (Before & After)

Real daemon, GET /workspace/skills before any session:

State Before (main / cache-only) After (this PR)
Reaped child, cache warm hasReview=true (replayed) hasReview=true
Cold start, preheat timed out (child never live) initialized=false skillsCount=0 initialized=true skillsCount=25 hasReview=true

Real npm run dev:daemon + Playwright (child's preheat still timing out): the browser's /workspace/skills returns initialized=true count=25 hasReview=true immediately, the slash menu shows /review, and /rev+Tab → /review [pr-number|file-path] [--comment].

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local: node scripts/dev.js serve / npm run dev:daemon (tsx source, loopback), driven with curl and Playwright; plus vitest unit tests.

Risk & Scope

  • Main risk or tradeoff: the daemon-local view omits extension-provided skills (there is no active-extension context outside the child) and does not apply safe-mode restriction; it is a best-effort fallback for the pre-child window only. The live child stays authoritative when present, and the cache preserves the child's full extension-aware list across a reap, so extension skills are only absent in the never-yet-preheated cold-start window. Skills are listed for autocomplete only — the child still gates execution.
  • Not validated / out of scope: not addressing why the dev-mode child init exceeds 10s (a separate preheat/timeout concern); this PR makes skills available regardless.
  • Breaking changes / migration notes: none. Behavior is unchanged whenever a child answer or a cached answer is available; only the no-child, no-cache path gains the daemon-local fallback.

Linked Issues

Completes the fix started in #6153.

中文说明

这个 PR 做了什么

让 Web Shell 首个 prompt 之前的斜杠命令列表在 ACP 子进程无法回答 /workspace/skills 时,依然保留技能类命令(如 /review),通过两层互补的 daemon 侧机制:

  1. 缓存子进程最近一次答出的技能状态,在无活子进程时回放——这样被回收的子进程仍能给出完整、含扩展的列表。
  2. daemon 本地枚举技能(直接经 SkillManager 读文件系统,不启子进程、不做 MCP 初始化)作为"子进程从未答复"时的兜底——即便子进程一直起不来,也能给出磁盘上的技能。

为什么需要

/workspace/skills 只由 ACP 子进程回答。requestWorkspaceStatus 只被动查已存活通道(liveChannelInfo(),从不 ensureChannel()),无通道就返回空 idle 占位。它没有 /workspace/providers 那样的 daemon 本地兜底——于是首个 prompt 前列表丢光技能、/rev 补不出 /review。子进程在三个窗口内缺席:

  • 首个 session 之前——session 创建延迟到首个 prompt(fix(web-shell): defer session creation until first prompt #6066)。
  • session 关闭后——子进程被立即回收(--channel-idle-timeout-ms 默认 0)。
  • 冷启动 preheat 超时——npm run dev 下最明显:按需 tsx 转译的子进程 initialize 握手经常超过 10s 的 preheat 预算(实测冷启动 ~19s,超时上限 10s),preheat 失败(日志 ACP preheat failed ... initialize timed out after 10000ms),通道永不就绪。

第三个窗口正是你报的现象——"/review 不在列表里,但真实可用":技能在磁盘上、你完整输入 /review 提交(会建 session)就能跑,但它从不出现在补全里,因为子进程没答 /workspace/skills。仅靠缓存救不了——没有活子进程它永远不被填充。daemon 本地 provider 堵上了这个窗口:直接从磁盘即时读技能,/review 从首次打开就在列表里。子进程在线时仍为权威(并保留本地视图省略的扩展技能)。

这补全了 #6153#6153 让 Web Shell 去拉 /workspace/skills,但当 daemon 无活子进程答不出时它无能为力。

Reviewer Test Plan(评审验证)

如何验证

单测:npx vitest run packages/cli/src/serve/workspace-service/ packages/cli/src/serve/workspace-skills-status.test.ts —— 覆盖缓存回放/刷新、兜底顺序(子进程→缓存→本地→空)、以及本地 provider 无子进程枚举出 bundled /review(76 个测试)。server.test.ts 保持全绿(574)。

端到端(npm run dev:daemon,其 preheat 超时、首个 prompt 前子进程从不在线):

  1. 启动、打开 Web Shell,发任何消息之前输入 /rev
  2. 按 Tab。

预期(本 PR):/review 在斜杠菜单里,Tab 把 /rev 补成 /review [pr-number|file-path] [--comment]。修复前:/review 缺失、Tab 无反应。

证据(Before & After)

真实 daemon,建 session 之前 GET /workspace/skills

状态 修复前(main / 仅缓存) 修复后(本 PR)
子进程被回收、缓存已暖 hasReview=true(回放) hasReview=true
冷启动、preheat 超时(子进程从未在线) initialized=false skillsCount=0 initialized=true skillsCount=25 hasReview=true

真实 npm run dev:daemon + Playwright(子进程 preheat 仍在超时):浏览器 /workspace/skills 立即返回 initialized=true count=25 hasReview=true,斜杠菜单显示 /review/rev+Tab → /review [pr-number|file-path] [--comment]

测试平台

仅本地 macOS 验证(单测 + curl + Playwright 真实 UI);Windows/Linux 未测,交给 CI。

运行环境(可选)

本地:node scripts/dev.js serve / npm run dev:daemon(tsx 源码、loopback),用 curl 和 Playwright 驱动;外加 vitest 单测。

风险与范围

  • 主要风险/取舍:daemon 本地视图省略扩展提供的技能(子进程外没有活动扩展上下文),也不施加 safe-mode 限制;它只是"子进程就绪前"这个窗口的尽力兜底。子进程在线时仍为权威,缓存又在回收后保留子进程的完整含扩展列表,所以扩展技能只在"从未 preheat 的冷启动窗口"缺席。技能仅用于补全列表——执行仍由子进程把关。
  • 未验证/范围之外:不处理 dev 模式子进程 init 为何超过 10s(另一个 preheat/超时议题);本 PR 让技能无论如何都可用。
  • 破坏性变更/迁移:无。有子进程答复或缓存答复时行为完全不变;只有"无子进程且无缓存"这条路径新增了 daemon 本地兜底。

关联 Issue

补全 #6153 开始的修复。

…s reaped

`GET /workspace/skills` is answered exclusively by the ACP child — the
daemon has no local SkillManager. `requestWorkspaceStatus` only checks for
an already-live channel (`liveChannelInfo()`, never `ensureChannel()`), so
when no child is running it returns the idle placeholder
(`initialized: false`, empty `skills`).

That is the norm before the first session, and — crucially — again after
the child is reaped on session close, which happens immediately by default
(`--channel-idle-timeout-ms` defaults to 0 = immediate kill). Unlike
`/workspace/providers`, skills have no daemon-local status provider to fall
back on. So once a user has created and closed a session, every subsequent
pre-first-prompt `/workspace/skills` query returns empty, the Web Shell's
slash-command list falls back to the hardcoded built-ins (which omit
skills), and `/rev` stops autocompleting `/review`.

Retain the last skills status a live child produced and replay it while no
channel is live, so skill-backed slash commands keep autocompleting; the
next live query refreshes the cache. `initialized` cleanly separates a real
child answer (always `true`) from the idle placeholder (always `false`).

Completes QwenLM#6153, which wired the Web Shell to fetch `/workspace/skills` in
the deferred-connect path but could not surface skills the daemon was
unable to answer without a live child.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual body, clear before/after evidence table.

On direction: this fixes a real and well-scoped UX bug — skill-backed slash commands (/review, etc.) stop autocompleting in the Web Shell the moment a user has opened and closed one session, because the ACP child is immediately reaped (--channel-idle-timeout-ms defaults to 0) and the daemon has no local SkillManager. That's a "worked once, then broke" regression that hits normal usage, not an edge case. Clearly aligned with the project's serve/Web Shell surface.

On approach: the scope is tight — 94 additions, 1 deletion, 2 files. The caching strategy mirrors the existing workspaceProvidersStatusProvider fallback pattern already in the codebase. I don't see a materially simpler path: a daemon-local SkillManager would be heavier and duplicate child logic; eagerly spawning a child on every /workspace/skills query contradicts the idle-reap design. The stale-cache window is benign (autocomplete, not a security boundary) and self-heals on the next live query. No unrelated changes or drive-by refactors in the diff.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需章节齐全,双语正文,清晰的 Before/After 证据表。

方向:这修的是一个真实且范围明确的 UX bug —— 用户只要开关过一个 session,ACP 子进程就被立刻回收(--channel-idle-timeout-ms 默认 0),daemon 又没有本地 SkillManager,于是 Web Shell 里技能类斜杠命令(/review 等)就补不出来了。这是"用过一次就坏"的回归,不是边缘场景,明显属于项目 serve/Web Shell 方向之内。

方案:范围很紧 —— 94 行新增、1 行删除、2 个文件。缓存策略复用了代码库中已有的 workspaceProvidersStatusProvider 兜底模式。没有看到更简的路径:daemon 本地 SkillManager 更重且与子进程逻辑重复;每次 /workspace/skills 查询都主动拉起子进程违背了 idle-reap 设计。缓存短暂陈旧是良性的(补全而非安全边界),下次 live 查询自愈。diff 中没有无关改动或顺手重构。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (before reading the diff): The bug is that getWorkspaceSkillsStatus returns the idle placeholder when no ACP child is live. The minimal fix: add a closure-scoped variable (lastSkillsStatus) inside createDaemonWorkspaceService, update it when queryWorkspaceStatus returns a live answer (initialized: true), and return it as fallback when the answer is idle. No new abstractions, no new files.

Comparison with the diff: the PR's approach matches this exactly. One closure variable (lastWorkspaceSkillsStatus), one type import added, cache updated on initialized === true, fallback on idle. The implementation is as tight as the proposal — no over-engineering, no speculative features.

Correctness: the initialized flag cleanly separates real child answers from the idle placeholder, so the cache condition is unambiguous. The variable is closure-scoped to a single service instance, so no cross-workspace leakage. Cold-start (first query ever, no cache) correctly falls through to the idle placeholder via ??.

Reuse check: the pattern mirrors the existing workspaceProvidersStatusProvider fallback — consistent with how the codebase already handles "daemon can't answer without a child."

Tests: two new test cases cover the key scenarios — cache replay when channel goes idle, and cache refresh when a newer live answer arrives. The existing idle-fallback test still passes, confirming no regression. All 58 tests in facade.test.ts pass.

Comments: the inline comments in getWorkspaceSkillsStatus are detailed (7 lines of prose for ~10 lines of code), but they explain the non-obvious daemon architecture constraint — why skills can only come from the child and why initialized is the discriminator. Appropriate for this context.

No blockers. No AGENTS.md violations.

E2E Test

Ran a real qwen serve daemon (built from the PR diff, node dist/cli.js serve --hostname 127.0.0.1 --no-web --channel-idle-timeout-ms 0 --port 18923) and drove the scenario with curl:

qwen serve listening on http://127.0.0.1:18923 (mode=http-bridge, workspace=/home/github-runner/actions-runner-24/_work/qwen-code/qwen-code)
qwen serve: bound to workspace "/home/github-runner/actions-runner-24/_work/qwen-code/qwen-code"
qwen serve: startup timing: processToListenMs=99 runQwenServeToListenMs=26
qwen serve: bearer auth disabled (loopback default). Set QWEN_SERVER_TOKEN to enable.
2026-07-02T06:48:08.868Z [INFO] [DAEMON] deferred runtime: scheduling fallback start in 1000ms
2026-07-02T06:48:09.869Z [INFO] [DAEMON] deferred runtime: fallback timer fired, starting
qwen serve: session reaper started (interval 60000ms, idle threshold 1800000ms)
qwen serve: /acp WebSocket transport enabled on /acp

Before (main — not reproducible in this environment without a separate build, using PR author's evidence)

Per PR evidence table: after DELETE /session/:id, GET /workspace/skills returns initialized=false skillsCount=0 hasReview=false

After (this PR)

Step 1 — Initial skills probe (cache warm via preheat child):

GET /workspace/skills → initialized=True skillsCount=25 hasReview=True

Step 2 — Create a session:

POST /session → 200
sessionId=2dde2519-7bee-47d4-8148-ce5d5250e0f6

Step 3 — Delete the session (triggers channel exit):

DELETE /session/2dde2519-7bee-47d4-8148-ce5d5250e0f6 → 204

Daemon log:
qwen serve: closing session "2dde2519-7bee-47d4-8148-ce5d5250e0f6" (reason: client_close)
qwen serve: channel exited (code=0, signal=none, 0 session(s) torn down)
2026-07-02T06:53:46.385Z [INFO] [DAEMON] route=DELETE /session/2dde2519-7bee-47d4-8148-ce5d5250e0f6 durationMs=19 status=204

Step 4 — Post-delete skills probe (channel reaped, no live child):

GET /workspace/skills → initialized=True skillsCount=25 hasReview=True
2026-07-02T06:54:53.328Z [INFO] [DAEMON] route=GET /workspace/skills durationMs=0 status=200

✅ Cache replay works — skills remain available after the ACP child is reaped. The durationMs=0 response time confirms it's served from cache, not a child query.

中文说明

代码审查

独立方案(读 diff 之前): bug 在于无存活 ACP 子进程时 getWorkspaceSkillsStatus 返回 idle 占位。最小修复:在 createDaemonWorkspaceService 内加一个闭包变量(lastSkillsStatus),queryWorkspaceStatus 返回 live 答案(initialized: true)时更新它,idle 时作为兜底返回。不加新抽象、不加新文件。

与 diff 对比: PR 方案与此完全一致。一个闭包变量(lastWorkspaceSkillsStatus),加一个类型导入,initialized === true 时更新缓存,idle 时兜底。实现和方案一样紧凑——没有过度工程,没有投机特性。

正确性: initialized 标志干净地区分了真实子进程答案和 idle 占位,缓存条件无歧义。变量是单个服务实例的闭包作用域,不会跨 workspace 泄漏。冷启动(首次查询,无缓存)通过 ?? 正确地降级到 idle 占位。

复用检查: 模式复用了已有的 workspaceProvidersStatusProvider 兜底——与代码库中"daemon 没有子进程就答不了"的处理方式一致。

测试: 两个新用例覆盖关键场景——通道空闲时回放缓存、更新的 live 答案刷新缓存。既有的 idle 兜底测试仍通过,确认无回归。facade.test.ts 的 58 个测试全部通过。

注释: getWorkspaceSkillsStatus 中的行内注释较详细(约 10 行代码配 7 行散文),但解释了非显而易见的 daemon 架构约束——为什么技能只能来自子进程、为什么 initialized 是区分器。在这个上下文中合适。

无阻塞问题。无 AGENTS.md 违规。

端到端测试

用 PR diff 构建的真实 qwen serve daemon(node dist/cli.js serve --hostname 127.0.0.1 --no-web --channel-idle-timeout-ms 0 --port 18923),用 curl 驱动场景:

关键结果:子进程被回收后(channel exited (code=0, signal=none, 0 session(s) torn down)),GET /workspace/skills 仍返回 initialized=True skillsCount=25 hasReview=True,响应时间 durationMs=0 确认来自缓存而非子进程查询。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a textbook bug fix — well-scoped, minimal, and the implementation matches exactly what I would have written independently.

Going back to my Stage 2a proposal: the PR's approach is identical. A single closure-scoped cache variable, updated on live answers, replayed on idle. No simpler path was missed.

The E2E result is the proof: channel exited (code=0, signal=none, 0 session(s) torn down) followed by GET /workspace/skills → initialized=True skillsCount=25 hasReview=True at durationMs=0. The cache works, the response is instant, and the slash commands keep autocompleting.

What I like about this PR:

  • The author clearly understands the daemon architecture — the fix works with the existing design, not against it. No attempt to add a daemon-local SkillManager or force child spawning.
  • The risk analysis is honest: stale cache is benign for autocomplete, the cold-start race is a known pre-existing window and explicitly out of scope.
  • The diff is pure — no drive-by refactors, no formatting churn, no "while I'm here" changes.

If I had to maintain this in six months, I'd thank the author. The inline comments explain the why (daemon can't answer skills without a child), the condition is obvious (initialized flag), and the fallback is safe (?? status).

LGTM — approving. ✅

中文说明

这是一个教科书级的 bug 修复——范围清晰、改动最小,实现与我独立提出的方案完全一致。

回到 Stage 2a 的独立方案:PR 的方案与之完全相同。一个闭包作用域的缓存变量,live 答案时更新,idle 时回放。没有更简的路径被遗漏。

E2E 结果就是证据:channel exited (code=0, signal=none, 0 session(s) torn down) 之后 GET /workspace/skills → initialized=True skillsCount=25 hasReview=True,响应时间 durationMs=0。缓存生效,响应即时,斜杠命令继续补全。

喜欢这个 PR 的地方:

  • 作者显然理解 daemon 架构——修复顺着既有设计来,而不是对着干。没有试图加 daemon 本地 SkillManager 或强制拉起子进程。
  • 风险分析诚实:缓存短暂陈旧对补全是良性的,冷启动竞态是既有的已知窗口,明确标为范围外。
  • diff 纯净——没有顺手重构、没有格式抖动、没有"既然都改了"的额外改动。

如果六个月后我要维护这段代码,我会感谢作者。行内注释解释了为什么(daemon 没有子进程就答不了技能),条件显而易见(initialized 标志),兜底安全(?? status)。

LGTM — 批准 ✅

Qwen Code · qwen3.7-max

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

LGTM, looks ready to ship. ✅

Comment thread packages/cli/src/serve/workspace-service/index.ts Outdated
…unavailable

The cache from the previous commit keeps the last child answer alive across
a reap, but it never warms when the child never answers at all — most
visibly under `npm run dev`, where the on-demand-transpiled child's
`initialize` handshake routinely exceeds the 10s preheat budget, so preheat
times out and no channel ever comes up. `/workspace/skills` then stays empty
until the first prompt, dropping `/review` and every other skill from the
Web Shell's pre-first-prompt autocomplete even though the skills exist on
disk (typing `/review` in full still runs it, since submitting spawns a
session — hence "not in the list, but usable").

Add a daemon-local skills provider that enumerates skills straight from the
filesystem via SkillManager (a lightweight Config shim — no child, no MCP
init), mirroring the existing daemon-local providers-status provider. The
facade falls back to it only after both a live child answer and the cached
last answer are unavailable, so the live child stays authoritative (and
keeps extension-provided skills) while a never-preheated child still yields
the on-disk skills — `/review` included.
@wenshao wenshao changed the title fix(serve): keep skill slash commands available after the ACP child is reaped fix(serve): keep skill slash commands available when the ACP child is unavailable Jul 2, 2026
…ows mid-flight

Addresses review feedback on QwenLM#6169: the channel can die after
`liveChannelInfo()` returns a valid channel but before the RPC completes, so
`queryWorkspaceStatus` rejects. Previously that exception propagated even
though the cache or the daemon-local provider could still answer. Wrap the
query in try/catch (logging via writeStderrLine, matching
getWorkspaceEnvStatus / getWorkspacePreflightStatus) and treat a mid-flight
failure as "no live child", so the request degrades to the cached last answer
or daemon-local enumeration instead of failing.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: self-PR; CI failing: Post Coverage Comment, Test (ubuntu-latest, Node 22.x).

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/workspace-skills-status.ts
Comment thread packages/cli/src/serve/workspace-skills-status.ts
@wenshao

wenshao commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

DragonnZhang
DragonnZhang previously approved these changes Jul 2, 2026

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

This PR adds a daemon-local skill enumeration fallback so skill-backed slash commands (e.g. /review) remain available in the Web Shell when the ACP child is unavailable (cold start, channel reaped, preheat timeout). The fallback logic is well-structured: cache the last live child answer, fall back to daemon-local SkillManager enumeration only when needed, and handle mid-flight channel deaths gracefully. Comprehensive test coverage across all fallback paths. Looks correct.

— qwen3-coder via Qwen Code /review

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

发现一个需要修正的问题:daemon-local skills fallback 现在写死了 isSafeMode: () => falsegetBareMode: () => false。这会导致 ACP child 不可用时,/workspace/skills 通过本地枚举把 project/user skills 暴露到 Web Shell 补全里,即使 daemon 是用 --safe-mode--bare 启动的。

这和真实 ACP child 的行为不一致:safe mode 下 SkillManager.refreshCache() 只加载 bundled skills;bare mode 下 listSkillsAtLevel() 会返回空。因此在这个 PR 要修复的“child 不可用”窗口里,补全列表可能展示真实 session 不会展示/不应该展示的 skill。尤其 safe mode 的语义是禁用自定义能力,包括 skills,这里会绕开该限制。

建议把当前有效的 safe/bare 状态传给 createWorkspaceSkillsStatusProvider(),或者用其他方式确保 daemon-local fallback 与 child 侧 SkillManager 行为一致。

其余主逻辑我认为方向是合理的:live child 优先,其次缓存,最后 daemon-local fallback;child 查询中途失败时回退到缓存/本地枚举也合理。

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

⚠️ Downgraded from Approve to Comment: CI failing (Post Coverage Comment, Test (ubuntu-latest, Node 22.x)).

Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
- Extract the SkillConfig → ServeWorkspaceSkillStatus mapping into a shared
  workspace-skills-mapping module used by both the ACP child's
  buildWorkspaceSkillsStatus and the daemon-local provider, so the two skill
  listings can't drift; cover it (including the disable-model-invocation
  branch) with a unit test.
- Memoize the SkillManager per workspace so repeat queries reuse its in-memory
  cache instead of re-scanning every skill level on each call.
- Honor the safe-mode env (isSafeModeEnv, as Config does) instead of hardcoding
  isSafeMode to false; keep bareMode off (the daemon never runs `--bare`).
- Log daemon-local enumeration failures via writeStderrLine, matching the rest
  of the workspace-service error handling.
@wenshao

wenshao commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@ytahdn thanks — addressed in 8eb98ab. isSafeMode now delegates to isSafeModeEnv() (the same source Config uses when no explicit flag is passed), so an operator running in safe mode gets the same bundled-only listing the child would produce, instead of a hardcoded false. getBareMode stays false on purpose: bare mode is the interactive --bare CLI flag, which the daemon never sets — there is no daemon "bare" value to read, so this is correct rather than a stubbed-out default.

中文

@ytahdn 多谢 — 已在 8eb98ab 处理。isSafeMode 现在委托给 isSafeModeEnv()(与 Config 在无显式 flag 时的取值同源),所以开启 safe mode 的操作者会拿到和子进程一致的"仅 bundled"列表,而非写死的 falsegetBareMode 有意保持 false:bare mode 是交互式 --bare CLI flag,daemon 从不设置——没有 daemon 的 "bare" 值可读,所以这是正确取值而非占位默认。

@wenshao

wenshao commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual body, clear before/after evidence table.

On direction: this fixes a real and well-scoped bug — skill-backed slash commands (e.g. /review) vanish from the Web Shell's autocomplete when the ACP child is unavailable, which happens in three distinct windows (pre-session, post-reap, cold-start preheat timeout). Claude Code's CHANGELOG has multiple entries about skill slash-command correctness ("Fixed user-level skills appearing multiple times in slash-command autocomplete", "Fixed claude -p being slow...while waiting for the slash-command/skill scan"), confirming this area matters. Aligned.

On approach: the two-layer fallback (child → cache → daemon-local → empty) is the minimal correct fix. The shared mapSkillConfigToStatus extraction prevents the two paths from drifting in shape. Scope is tight — 609 additions, ~300 of which are tests. No unrelated changes. The latest commits (8eb98ab addressing safe-mode delegation, d7ccd96 adding error-path test coverage) respond cleanly to prior review feedback. Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必填部分齐全,中英双语正文,清晰的修复前后对比表。

方向:修复了一个真实且范围明确的 bug —— 技能类斜杠命令(如 /review)在 ACP 子进程不可用时从 Web Shell 的自动补全列表中消失,这发生在三个不同的窗口(首 session 前、子进程回收后、冷启动 preheat 超时)。Claude Code 的 CHANGELOG 有多条关于技能斜杠命令正确性的条目,确认这个方向重要。对齐。

方案:两层兜底(子进程 → 缓存 → daemon 本地 → 空)是最小且正确的修复。共享的 mapSkillConfigToStatus 提取防止两条路径在结构上漂移。范围紧凑 —— 609 行新增中约 300 行是测试。最新提交(8eb98ab 处理 safe-mode 委托、d7ccd96 补充错误路径测试)干净地回应了之前的审查反馈。进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (before reading the diff): The bug is that getWorkspaceSkillsStatus returns the idle placeholder when no ACP child is live. I would: (1) cache the last live child response and replay it when the channel is idle, (2) add a daemon-local fallback using SkillManager.listSkills() directly from disk for cold-start scenarios where the cache never warms, (3) extract the shared skill-mapping logic to prevent divergence between the child path and the daemon-local path.

Comparison with the diff: The PR's implementation matches this proposal exactly, and does it well:

  • Fallback chain in getWorkspaceSkillsStatus (child → cache → daemon-local → empty) is correct, with proper error handling for mid-flight channel deaths.
  • mapSkillConfigToStatus extraction is clean — single function, single responsibility, shared by both paths.
  • SkillManager instances are cached per-workspace in the daemon-local provider, avoiding repeated filesystem scans.
  • The SkillManagerConfigShim type uses Pick<Config, ...> for compile-time drift detection — a good defensive pattern.
  • isSafeMode correctly delegates to isSafeModeEnv() (addressed from prior review feedback in 8eb98ab).
  • Error path in the daemon-local provider returns a non-initialized status with the error surfaced via errors array, and the facade additionally guards the injected provider call with its own try/catch (d7ccd96).

No correctness bugs, security holes, or regressions found. No AGENTS.md violations.

Unit Tests

All pass (this rerun):

  • workspace-skills-mapping.test.ts — 3/3 ✓
  • workspace-service/__tests__/facade.test.ts — 64/64 ✓ (includes 8 fallback-ordering tests: cache replay, cache refresh, daemon-local fallback, cache-over-local preference, child-live skips local, mid-flight throw uses cache, throw-with-no-cache uses local, local-throw degrades to idle)
  • workspace-skills-status.test.ts — 4/4 ✓ (enumeration, workspace path, error handling, SkillManager memoization)
  • workspace-service/__tests__/integration.test.ts — 13/13 ✓
  • server.test.ts — 574/574 ✓ (no regressions)

Total: 84 + 574 = 658 tests, all green.

Real-Scenario Testing

Started the daemon with npm run dev -- serve on both main and the PR branch, then queried GET /workspace/skills via curl.

Before (main, 6509e8d)

qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge, workspace=/home/github-runner/actions-runner-5/_work/qwen-code/qwen-code)
qwen serve: bound to workspace "/home/github-runner/actions-runner-5/_work/qwen-code/qwen-code"
qwen serve: startup timing: processToListenMs=653 runQwenServeToListenMs=35
2026-07-02T11:25:54.345Z [INFO] [DAEMON] deferred runtime: scheduling fallback start in 1000ms
2026-07-02T11:25:55.346Z [INFO] [DAEMON] deferred runtime: fallback timer fired, starting
qwen serve: session reaper started (interval 60000ms, idle threshold 1800000ms)

$ curl -s http://127.0.0.1:4170/workspace/skills | python3 ...
initialized=True skillsCount=26 hasReview=True

On this Linux CI box, the ACP child initializes within the preheat budget, so the cold-start race doesn't reproduce here (it's a macOS npm run dev timing issue where tsx on-demand transpilation pushes child init past 10s).

After (this PR)

qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge, workspace=/home/github-runner/actions-runner-5/_work/qwen-code/qwen-code/.qwen/worktrees/triage)
qwen serve: bound to workspace "/home/github-runner/actions-runner-5/_work/qwen-code/qwen-code/.qwen/worktrees/triage"
qwen serve: startup timing: processToListenMs=639 runQwenServeToListenMs=35
2026-07-02T11:29:36.950Z [INFO] [DAEMON] deferred runtime: scheduling fallback start in 1000ms
2026-07-02T11:29:37.952Z [INFO] [DAEMON] deferred runtime: fallback timer fired, starting
qwen serve: session reaper started (interval 60000ms, idle threshold 1800000ms)
2026-07-02T11:30:16.021Z [INFO] [DAEMON] route=GET /workspace/skills durationMs=6 status=200 request completed

$ curl -s http://127.0.0.1:4170/workspace/skills | python3 ...
initialized=True skillsCount=25 hasReview=True

No regression when the child is live — the child path remains authoritative. The daemon-local fallback is exercised only in the cold-start window that doesn't reproduce on this CI hardware, but the unit tests comprehensively cover that code path (8 dedicated fallback-ordering tests).

中文说明

代码审查

独立方案(读 diff 前): bug 在于无活 ACP 子进程时 getWorkspaceSkillsStatus 返回空闲占位。我的方案:(1) 缓存最后一次活子进程响应并在通道空闲时回放,(2) 用 SkillManager.listSkills() 直接从磁盘枚举作为冷启动兜底,(3) 提取共享的技能映射逻辑防漂移。

与 diff 对比: PR 实现完全匹配这个方案,且做得很好:兜底链正确、含中途通道断开错误处理;mapSkillConfigToStatus 提取干净;SkillManager 实例按工作区缓存;SkillManagerConfigShimPick<Config, ...> 做编译期漂移检测;isSafeMode 正确委托到 isSafeModeEnv()8eb98ab 已处理);daemon 本地 provider 的错误路径返回非初始化状态并通过 errors 数组上报,facade 额外用自己的 try/catch 保护注入的 provider 调用(d7ccd96)。无正确性 bug、安全漏洞或回归。

单测

全部通过:84 + 574 = 658 个测试全绿。

真实场景测试

在 Linux CI 上子进程在 preheat 预算内完成初始化,冷启动竞态无法复现(这是 macOS npm run dev 下 tsx 按需转译导致子进程 init 超过 10s 的问题)。修复前后均返回 initialized=True hasReview=True,无回归。兜底路径由 8 个专门的单测覆盖。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

This is a textbook bug fix — well-scoped, minimal, and the implementation matches exactly what I would have written independently.

Going back to my Stage 2a proposal: the PR's fallback chain (child → cache → daemon-local → empty) is the same three-layer approach I would have taken, and it's implemented cleanly. The shared mapSkillConfigToStatus extraction is the right call — it's not over-abstraction for a single call site, it's preventing two parallel skill-mapping paths from drifting in shape. The SkillManagerConfigShim using Pick<Config, ...> for compile-time safety is a nice touch.

Since the last triage run, two commits landed:

  • 8eb98abisSafeMode now delegates to isSafeModeEnv() instead of hardcoding false. This is the correct fix: an operator running in safe mode gets the same bundled-only listing the child would produce. getBareMode stays false on purpose — bare mode is the interactive --bare CLI flag, which the daemon never sets.
  • d7ccd96 — adds error-path test coverage for the daemon-local provider throwing, and guards the facade's injected provider call with its own try/catch. This closes the last gap in the fallback chain's test coverage.

658 tests pass (84 skills-specific + 574 server), no regressions. The real-scenario test confirms no regression when the child is live — the cold-start race is a macOS dev-mode timing issue that doesn't reproduce on this Linux CI box, but 8 dedicated unit tests comprehensively cover the fallback chain.

Every change in the diff serves the stated goal. No drive-by refactors, no scope creep. If I had to maintain this in six months, I'd thank the author.

LGTM, approving. ✅

中文说明

这是一个教科书级的 bug 修复 —— 范围精准、实现最小化,且与我独立构思的方案完全一致。

自上次 triage 以来新增了两个提交:8eb98abisSafeMode 委托到 isSafeModeEnv()(safe mode 操作者获得与子进程一致的仅 bundled 列表);d7ccd96 补充了 daemon 本地 provider 抛错时的测试覆盖,并在 facade 层额外加了 try/catch。658 个测试全部通过,无回归。真实场景测试确认子进程在线时无回归。diff 中每一处改动都服务于既定目标,无顺手重构、无范围蔓延。LGTM ✅

Qwen Code · qwen3.7-max

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

LGTM, looks ready to ship. ✅

Comment thread packages/cli/src/serve/workspace-skills-status.ts
ytahdn
ytahdn previously approved these changes Jul 2, 2026

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

Incremental commit addresses all prior review feedback: shared mapSkillConfigToStatus with tests, isSafeMode reads actual env, SkillManager memoization, writeStderrLine in catch. LGTM ✅

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 2, 2026
Comment thread packages/cli/src/serve/workspace-skills-status.ts
Comment thread packages/cli/src/serve/workspace-service/index.ts
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 2, 2026
…rovider call

- Test the previously-uncovered `buildWorkspaceSkillsStatus` catch branch:
  when enumeration fails it returns `{ initialized: false, skills: [],
  errors: [{ kind: 'skills', status: 'error', error }] }` and logs to stderr.
  Also cover the per-workspace SkillManager memoization.
- Wrap the facade's `workspaceSkillsStatusProvider` call in try/catch so a
  throwing injected provider degrades to the idle placeholder instead of
  failing the request (matching getWorkspaceEnvStatus / getWorkspacePreflightStatus),
  with a facade test for the throw path.

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

Incremental commit (+82/-3) addresses all remaining review feedback: try/catch on daemon-local provider call in facade, error-path test for buildWorkspaceSkillsStatus, memoization verification test, and missing writeStderrLine assertion in throw-path tests. Clean, well-scoped additions. LGTM ✅

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

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

LGTM — cache → daemon-local fallback 的分层策略清晰,safe/bare mode 透传和 SkillManager memoization 的增量修复也很到位。

@wenshao
wenshao enabled auto-merge July 2, 2026 11:27

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

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 2, 2026
Merged via the queue into QwenLM:main with commit 76addf4 Jul 2, 2026
77 checks passed
// Live child unavailable. Prefer the last answer it produced (keeps the
// full, extension-aware list available across a reap)...
if (lastWorkspaceSkillsStatus) {
return lastWorkspaceSkillsStatus;

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 cache replay path (return lastWorkspaceSkillsStatus) has no log output. The response is byte-identical to a live child answer (initialized: true, full skills array), so an operator debugging why skills look stale after a child reap has no stderr signal to distinguish "child is alive and skills are current" from "child died and we are replaying the last answer."

Neither getWorkspaceEnvStatus nor getWorkspacePreflightStatus have a comparable cache mechanism, so this is a new observability gap introduced by this PR. A single writeStderrLine before the cache return would close it:

Suggested change
return lastWorkspaceSkillsStatus;
if (lastWorkspaceSkillsStatus) {
writeStderrLine(
'qwen serve: getWorkspaceSkillsStatus replaying cached skills status (child unavailable)',
);
return lastWorkspaceSkillsStatus;
}

— qwen3.7-max 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.

Well-structured 3-tier fallback (child → cache → daemon-local → idle) with comprehensive error handling and thorough test coverage (84 tests). Build passes, CI 30/30 green. No blocking issues found. LGTM ✅

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

6 participants