fix(daemon): Refresh workspace provider defaults - #5638
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ On direction: this is a well-scoped daemon/API fix — On approach: the scope feels right for the stated goal. The 1389 additions are dominated by tests (~850+ lines), and the production changes are focused: a new daemon-local provider catalog builder ( Moving on to code review and testing. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个聚焦的 daemon/API 修复—— 方案：范围与目标匹配。1389 行新增以测试为主（~850+ 行），生产代码改动聚焦：新增 daemon-local provider catalog 构建器（ 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] applyModelServiceId (used during session creation and attach-with-model-change) only broadcasts model_switched, but does not broadcast settings_changed. This creates an asymmetry with setSessionModel which now broadcasts both. If consumers start acting on settings_changed to refresh workspace provider state, model changes via attach would be silently missed. Consider adding the same settings_changed broadcast to applyModelServiceId for consistency.
— qwen3.7-max via Qwen Code /review
| options: WorkspaceProvidersStatusProviderOptions, | ||
| ): ServeWorkspaceProvidersStatus { | ||
| try { | ||
| const loaded = loadSettings(workspaceCwd); |
There was a problem hiding this comment.
[Suggestion] loadSettings() performs synchronous file I/O (fs.readFileSync, fs.realpathSync, fs.existsSync) on every GET /workspace/providers request with no caching. Combined with ModelsConfig construction, this is non-trivial per-request work on the main thread.
Consider a short-lived TTL cache (e.g., 500ms) or event-driven invalidation using the settings_changed events this PR already introduces. This preserves the "fresh" semantics without paying full I/O cost on every poll:
let cached: { at: number; result: ServeWorkspaceProvidersStatus } | undefined;
const TTL_MS = 500;
return async (workspaceCwd, acpChannelLive) => {
if (cached && Date.now() - cached.at < TTL_MS)
return { ...cached.result, acpChannelLive };
const result = buildWorkspaceProvidersStatus(workspaceCwd, acpChannelLive, options);
cached = { at: Date.now(), result };
return result;
};— qwen3.7-max via Qwen Code /review
Code ReviewIndependent proposal: to solve "workspace defaults vs session model" confusion, I'd (1) make No blockers found. Code is clean and follows project conventions. One minor observation: The The Test ResultsAll PR-specific tests pass ✅ tmux real-scenario testing: N/A — this is a daemon/API and WebUI state-source change with no TUI-visible behavior to capture. The behavior (which model appears as "current" in the WebUI connection state and which 中文说明代码审查独立方案：为解决 "workspace 默认 vs session 模型" 混淆，我会 (1) 让 未发现阻塞问题。 代码整洁，遵循项目规范。 一个小观察：
bridge 中的 测试结果所有 PR 相关测试通过 ✅ tmux 真实场景测试：N/A — 这是 daemon/API 和 WebUI 状态来源变更，无 TUI 可见行为可捕获。"哪个模型显示为 current" 和 — Qwen Code · qwen3.7-max |
|
Stepping back: this PR solves a real problem — clients seeing stale or wrong default models before a session exists, and active sessions incorrectly reflecting workspace defaults instead of their own model state. The fix is architecturally clean: daemon builds a fresh provider snapshot from settings on each request, the WebUI correctly prioritizes session context over workspace defaults, and model switches propagate a The implementation matches my independent proposal. The test suite is thorough — 87+ focused tests across 5 suites, all passing. Credential sanitization is handled consistently across every output path (provider URLs, warning messages, current selection), with a 16-case parameterized test that covers tricky edge cases like spaces, quotes, and encoded characters in credentials. The one minor duplication ( No concerns. Looks ready to ship. ✅ 中文说明回顾整体：这个 PR 解决了一个真实问题——客户端在 session 创建前看到过期或错误的默认模型，活跃 session 错误地反映 workspace 默认而非自己的模型状态。修复在架构上很干净：daemon 每次请求从 settings 构建新鲜 provider 快照，WebUI 正确优先使用 session context，model switch 时传播 实现与我的独立方案一致。测试套件充分——5 个套件共 87+ 个聚焦测试全部通过。凭据清洗在每个输出路径（provider URL、警告信息、当前选择）都一致处理，16 个参数化测试覆盖了空格、引号、编码字符等棘手边界情况。 唯一的轻微重复（ 无顾虑，可以合入 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
✅ Maintainer local verification — real build, focused tests, mutation analysis & live-daemon e2eVerified at commit 1. Build
2. Focused unit tests (your Reviewer Test Plan) — all green
3. Mutation testing — are the new tests non-vacuous? Yes.I reverted each production change one at a time (keeping the PR's tests) and re-ran. Every mutant is caught by exactly the test(s) that should catch it:
4. Typecheck — all green
5. Live-daemon e2e (the real HTTP route) — 14 / 14Booted one real A — fresh defaults + credential redaction — settings: B — fresh read, no restart — rewrote settings → C — duplicate modelId, exact persisted baseUrl — two respA (trimmed to
|
| 套件 | 结果 |
|---|---|
cli · workspace-providers-status.test.ts |
6 / 6 ✓ |
cli · workspace-service/__tests__/facade.test.ts |
44 / 44 ✓ |
cli · server.test.ts -t "…providers from daemon-local settings" |
1 / 1 ✓ |
acp-bridge · bridge.test.ts -t "setSessionModel" |
9 / 9 ✓ |
webui · DaemonSessionProvider.test.tsx -t "session context…" |
3 / 3 ✓ |
📝 小问题：测试计划里写的是
src/serve/workspaceProvidersStatus.test.ts，但实际文件是 kebab-case 的src/serve/workspace-providers-status.test.ts。照搬命令会打印 "No test files found"。纯文档问题，建议在描述里修正。
3. 变异测试 —— 这些新测试是否"非空洞"？是的。
我逐个把生产代码改动还原（保留 PR 的测试）后重跑。每个变异体都恰好被应当捕获它的测试捕获：
| 还原/改动的生产代码 | 套件 | 翻红的测试 ❌ |
|---|---|---|
从快照中删掉 acpChannelLive |
wps | 2（fresh defaults；错误路径） |
关闭 sanitizeProviderBaseUrl |
wps | 1（凭据泄露断言） |
matchesCurrentBaseUrl → return true |
wps | 2（重复 baseUrl 精确匹配） |
| 删除 daemon-local 委托分支 | facade | 1 |
删除 workspaceProvidersStatusProvider 路由接线 |
server | 1 |
删除两处 settings_changed 广播 |
bridge | 3（其中 2 个因等待永不到达的事件而超时失败 —— 证明实时订阅确实依赖它） |
还原 session-context 优先逻辑（DaemonSessionProvider + mappers） |
webui | 3 |
4. Typecheck —— 全绿
acp-bridge、webui、sdk-typescript：exit 0，0 错误。在干净的 npm ci && npm run build 树上，完整 cli 的 tsc --noEmit 同样通过（0 错误） —— Risk & Scope 里提到的 BaseTextInput.tsx Ink 子路径既有错误在此未复现，也没有任何错误涉及本 PR 的文件。
5. 真实 daemon 端到端（真实 HTTP 路由）—— 14 / 14
启动一个真实 qwen serve（loopback、免鉴权、无 model 凭据），对 GET /workspace/providers 发请求，并在同一运行中的 daemon 上于请求之间改写用户 settings.json。全程 acpChannelLive:false（从未拉起 ACP 子进程）。
A —— fresh 默认值 + 凭据脱敏 —— 设置：选中 model-a，baseUrl https://user:secret@api-a.example/v1
→ initialized:true、acpChannelLive:false、current.modelId:"model-a(openai)"、current.baseUrl:"https://api-a.example/v1"（userinfo 已剥离），整个响应体不含 secret；model-a.isCurrent:true、model-b.isCurrent:false。✓（8/8）
B —— 热读、不重启 —— 在同一 daemon 上把设置改写为 model-b
→ 路由立即返回 current.modelId:"model-b(openai)"。✓（2/2）这是核心声明：快照在每次请求时从最新 workspace 设置重建，无需 live ACP/session。
C —— 重复 modelId，精确匹配持久化的 baseUrl —— 两个 shared-model 条目，model.baseUrl = api-two
→ 仅 api-two 条目 isCurrent:true，api-one 为 isCurrent:false，无凭据泄露。✓（4/4）
结论
行为与 PR 声明的意图端到端一致；覆盖全面且非空洞；类型干净；两个微妙/安全相关的点 —— 凭据脱敏 与 重复 baseUrl 精确匹配 —— 都正确、有测试守护、并经真实运行确认。LGTM / 建议合并。 唯一小瑕疵是描述里的测试路径笔误。
Verification method: isolated git worktree of PR head + full npm ci/npm run build; mutation probes applied to production files only (PR tests kept) then reverted; e2e against a real loopback qwen serve binary. Working tree confirmed clean after all probes.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Sanitize provider warning URLs before returning workspace providers status and preserve session context-window fallback from provider catalog models. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical issues from the prior review are resolved in the current head: the no-baseUrl current-model matching no longer marks explicit baseUrl variants as current, and provider warning URLs are sanitized before they are returned. I did not find any new critical blocker in this pass.
Broadcast settings_changed inside the serialized model switch callback so it cannot be reordered behind reconcile's corrective model_switched event. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] The catch block in buildWorkspaceProvidersStatus passes error.message into the API response without the credential sanitization that the success path applies. An exception from loadSettings(), ModelsConfig, or resolveCliGenerationConfig() could embed a credential-bearing URL into the error cell, leaking credentials to connected clients.
— qwen3.7-max via Qwen Code /review
| : undefined; | ||
| const fastModelId = | ||
| typeof settings.fastModel === 'string' && settings.fastModel.length > 0 | ||
| ? settings.fastModel |
There was a problem hiding this comment.
[Suggestion] isCurrent computation reads from two independent data sources: buildExplicitModelBaseUrls(settings.modelProviders) parses raw settings to determine which models have explicit base URLs, while the main iteration loop reads from modelsConfig.getAllConfiguredModels(). If ModelsConfig ever transforms the model list differently from raw settings (e.g., deduplication, system defaults, or model merging), the explicitModelBaseUrls set membership check could disagree with the iterated model, silently breaking isCurrent accuracy. Consider deriving both from the same source.
— qwen3.7-max via Qwen Code /review
| ]); | ||
| publishModelSwitched(entry, modelId, originatorClientId); | ||
| broadcastWorkspaceEvent({ | ||
| type: 'settings_changed', |
There was a problem hiding this comment.
[Suggestion] This broadcastWorkspaceEvent({ type: 'settings_changed', ... }) block is duplicated verbatim at two call sites (here and in setSessionModel), always immediately after publishModelSwitched. Consider folding the broadcast into publishModelSwitched itself so the two events are always emitted as a pair. This also avoids the current silent asymmetry at the reconcile path (which calls publishModelSwitched without settings_changed).
— qwen3.7-max via Qwen Code /review
Sanitize provider construction error messages before returning them from /workspace/providers so credential-bearing URLs cannot leak through the error path. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
| transportClosed, | ||
| ]); | ||
| publishModelSwitched(entry, modelId, originatorClientId); | ||
| broadcastWorkspaceEvent({ |
There was a problem hiding this comment.
[Suggestion] settings_changed is broadcast here and in setSessionModel (line 3955), but two other publishModelSwitched call sites were not updated:
- Line 1220 (agent-initiated):
onModelPromotedcallback — invoked when the ACP child sendscurrent_model_update(agent changes its own model mid-conversation). OnlypublishModelSwitchedis called, nosettings_changed. - Line 1999 (reconciliation): After session reattach, if the actual model diverges from cached
currentModelId, the bridge corrects viapublishModelSwitched— but nosettings_changed.
Clients using settings_changed to refresh settings/model-name display will show stale data when the agent autonomously switches models or the bridge corrects on reconnect. The existing reconciliation test (around line 9337) only asserts settings_changed for the client-requested switch, not the corrective one.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No additional high-confidence issues found. LGTM.
-- GPT-5 Codex via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical issue from the latest review is fixed in current head: provider construction errors are sanitized before being returned from /workspace/providers, so credential-bearing URLs no longer leak through the error path. I did not find any new critical blocker in this pass.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
0639b8b to
b4c1138
Compare
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
All 5 focused test suites from the PR description green locally (219 assertions across cli, acp-bridge, webui), typechecks green in all 4 affected packages, and the implementation matches the independent design baseline exactly. The workspace-defaults-vs-session-state separation is the right architectural cut, credential stripping is applied consistently, and the new settings_changed bridge event is the minimum needed for caching clients. No scope creep, no blockers.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
✅ Maintainer verification — real local build + live
|
| Suite | Result |
|---|---|
cli workspace-providers-status + workspace-service/facade + acpModelUtils |
72 passed |
cli server.test daemon-local providers |
1 passed |
acp-bridge bridge.test setSessionModel |
9 passed |
webui DaemonSessionProvider session-context / unmatched |
4 passed |
typecheck acp-bridge / webui / sdk-typescript |
clean (tsc --noEmit) |
2. Live daemon E2E (the part unit tests can't prove)
Started a real qwen serve against an isolated QWEN_HOME, then hit GET /workspace/providers with curl:
| # | Scenario | Result |
|---|---|---|
| 1 | Before any session | initialized: true, current.modelId = model-a(openai) straight from settings; current.baseUrl sanitized — the user:SUPERSECRET@ in settings was stripped, zero credential leak in the whole payload |
| 2 | Core fix — fresh on every request | Edited settings.json model.name model-a → model-b with the daemon still running (no restart); the very next request returned current.modelId = model-b(openai) and moved isCurrent accordingly |
| 3 | Duplicate model id, matching model.baseUrl |
Only the exact-baseUrl entry (api-two) is isCurrent: true; the same-id sibling (api-one) stays false, no warning |
| 3b | Duplicate id, mismatched model.baseUrl |
Falls back to first id-match and emits the disambiguation warning — and the warning itself is sanitized (credentials never appear even in the error text) |
Check 2 is the load-bearing evidence: the ACP child is spawned at startup and does not watch settings.json, so a disk edit reflected on the next request can only come from a daemon-side re-read of fresh settings — exactly what this PR claims. (Note: the live daemon reported acpChannelLive: true because serve pre-warms an ACP channel even with zero sessions; the false path is covered by unit tests. Providers data was correct regardless of channel state — which is the whole point.)
3. Mutation tests — the new tests actually bite
Green → mutate production code → red → revert → green:
| Mutation | Effect |
|---|---|
Neutralize the settings_changed broadcast in bridge.ts |
3 setSessionModel tests go red (expected 'settings_changed_MUTANT' to be 'settings_changed') |
Make sanitizeProviderBaseUrl stop stripping credentials |
9 tests go red (7 acpModelUtils cases + 2 workspace-providers-status, incl. not.toContain('secret')) |
Both reverted; worktree clean, tests green again.
Scope / caveats
- WebUI session-context-priority and the
settings_changedbroadcast are verified via unit tests (+ mutation), not a live browser / SSE session — consistent with the PR's stated "unit-test covered, N/A screenshots" approach. - Credential sanitization held on every live path I could reach (current model, provider catalog, disambiguation warning).
Verdict
Behaves exactly as described: /workspace/providers is now a fresh, daemon-local snapshot that works before any session and never leaks credentials, and the model-switch settings_changed signal is in place. Tests are meaningful (mutation-proven). LGTM from a verification standpoint. 👍
中文版（完整对应）
✅ 维护者验证 —— 真实本地构建 + 实跑 qwen serve daemon + 变异测试
我在本 PR 的 head 上构建了真实二进制,并对 daemon 做了端到端验证(不止单测),再用变异测试证明新增用例并非空过。作为合并参考。
被测对象: PR head b4c1138e3 · qwen 0.18.5 · 隔离的 QWEN_HOME · macOS 26.5.1、Node v22.22.2、tmux 3.6a · 真实 qwen serve 监听 127.0.0.1:56649(loopback,无 bearer)。
1. Reviewer Test Plan 里的聚焦测试 —— 全绿
| 套件 | 结果 |
|---|---|
cli workspace-providers-status + workspace-service/facade + acpModelUtils |
72 通过 |
cli server.test daemon-local providers |
1 通过 |
acp-bridge bridge.test setSessionModel |
9 通过 |
webui DaemonSessionProvider session-context / unmatched |
4 通过 |
typecheck acp-bridge / webui / sdk-typescript |
干净(tsc --noEmit) |
2. 实跑 daemon 端到端(单测无法覆盖的部分)
对隔离的 QWEN_HOME 启动真实 qwen serve,再用 curl 请求 GET /workspace/providers:
| # | 场景 | 结果 |
|---|---|---|
| 1 | 任何 session 之前 | initialized: true,current.modelId = model-a(openai) 直接来自 settings;current.baseUrl 已脱敏 —— settings 里的 user:SUPERSECRET@ 被剥离,整个响应零凭据泄漏 |
| 2 | 核心修复 —— 每次请求都读最新 | 在 daemon 不重启的情况下把 settings.json 的 model.name 由 model-a 改成 model-b;下一次请求立即返回 current.modelId = model-b(openai),isCurrent 相应迁移 |
| 3 | 同名 model id、model.baseUrl 精确匹配 |
只有 baseUrl 精确匹配的条目(api-two)是 isCurrent: true,同 id 的 api-one 保持 false,无 warning |
| 3b | 同 id、model.baseUrl 不匹配 |
回退到第一个 id 匹配,并发出 disambiguation warning —— 且警告本身也脱敏(凭据连错误文本里都不出现) |
Check 2 是关键证据:ACP 子进程在启动时 spawn 且不监听 settings.json,因此磁盘改动能在下一次请求体现,只能来自 daemon 侧对最新 settings 的重新读取 —— 正是本 PR 的主张。(注:实跑 daemon 报告 acpChannelLive: true,因为 serve 即便零 session 也会预热一个 ACP channel;false 分支由单测覆盖。无论 channel 状态如何,providers 数据都正确 —— 这正是本意。)
3. 变异测试 —— 新测试真的拦得住
绿 → 改坏生产代码 → 红 → 回退 → 绿:
| 变异 | 效果 |
|---|---|
破坏 bridge.ts 里的 settings_changed 广播 |
3 个 setSessionModel 测试转红(expected 'settings_changed_MUTANT' to be 'settings_changed') |
让 sanitizeProviderBaseUrl 不再剥离凭据 |
9 个测试转红(7 个 acpModelUtils 用例 + 2 个 workspace-providers-status,含 not.toContain('secret')) |
两处变异均已回退;worktree 干净,测试重新全绿。
范围 / 说明
- WebUI 的 session-context 优先逻辑与
settings_changed广播由**单测(+ 变异)**验证,未做真实浏览器 / SSE 会话验证 —— 与 PR 自述的"单测覆盖、无需截图"一致。 - 凭据脱敏在我能触达的每条实跑路径(current model、provider catalog、disambiguation warning)上都成立。
结论
行为与描述完全一致:/workspace/providers 现在是新鲜的、daemon 本地的快照,在任何 session 之前即可用、且从不泄漏凭据;模型切换的 settings_changed 信号也已就位。测试有效(经变异验证)。从验证角度 LGTM。 👍
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
f19141c to
4d47fb5
Compare
🔁 Re-verification — new head
|
Provider baseUrl in settings |
Sanitized output | Correct? |
|---|---|---|
https://host:99999/path@domain |
https://host:99999/path@domain (unchanged) |
✅ @ is in the path + 99999 is a port-shaped segment → not credentials, so not stripped |
https://user@host:99999 |
https://host:99999 |
✅ real userinfo stripped, invalid-port host kept |
https://user:SECRET3@valid.example/v1 |
https://valid.example/v1 |
✅ normal credential strip, SECRET3 absent from whole payload |
This is exactly the regression the commit targets: the old catch logic took lastIndexOf('@'), saw host:99999/path contained a :, and stripped down to https://domain — mangling a valid URL. The new digit-port guard prevents that.
Core behaviors re-confirmed on the rebuilt binary: fresh per-request read (model.name a→b with no restart → current.modelId follows) and duplicate-id isCurrent (only the exact-baseUrl match is current, after the matchesCurrentModel simplification) — both PASS.
Mutation — the new bound is load-bearing
Removing the /^\d+$/ port check in findUnescapedUserInfoFallbackAt makes https://host:99999/path@domain collapse to https://domain, turning the new test case red (expected 'https://domain' to be 'https://host:99999/path@domain'). Reverted; clean & green again.
Verdict
The new commit does what it says and fixes a real over-stripping edge case; tests are non-vacuous (mutation-proven); no credential leaked on any live path. Prior full verification still holds. LGTM on 4d47fb54a. 👍
中文版（完整对应）
🔁 复验 —— 新 head 4d47fb54a(fix(daemon): bound invalid provider URL sanitization)
承接我之前的完整验证(针对 b4c1138e3)。作者此后又推了 1 个提交;我在同一套 live-daemon + 变异环境上重建并重跑了受影响的路径。
4d47fb54a 改了什么: 收紧 sanitizeProviderBaseUrl 的 catch 分支(new URL() 解析失败的 URL,如非法端口),使其不再过度剥离;并简化 matchesCurrentModel、新增 2 个测试用例。这两个文件之外的内容未动,所以我之前的结论(每请求新鲜快照、settings_changed 广播、WebUI session-context 优先)依然成立。
新 head 上的聚焦测试 —— 全绿
acpModelUtils 21 通过(含 2 个新非法端口用例) · workspace-providers-status + facade 53 通过 · server daemon-local providers 1 通过。
Live daemon —— 验证新的脱敏边界(真实 qwen serve + curl)
把每个棘手 URL 作为 provider baseUrl,读回脱敏后的 catalog:
settings 里的 provider baseUrl |
脱敏输出 | 是否正确 |
|---|---|---|
https://host:99999/path@domain |
https://host:99999/path@domain(不变) |
✅ @ 在 path 里、99999 形似端口 → 不是凭据,因此不剥离 |
https://user@host:99999 |
https://host:99999 |
✅ 真正的 userinfo 被剥离,非法端口 host 保留 |
https://user:SECRET3@valid.example/v1 |
https://valid.example/v1 |
✅ 常规凭据剥离,SECRET3 不出现在整个响应里 |
这正是该 commit 针对的回归:旧 catch 逻辑取 lastIndexOf('@'),看到 host:99999/path 含 : 就一路剥离成 https://domain —— 把合法 URL 弄坏了。新的数字端口判定阻止了这一点。
在重建后的二进制上再次确认核心行为:每请求读最新(model.name a→b、不重启 → current.modelId 跟随)与同 id 的 isCurrent(matchesCurrentModel 简化后,只有 baseUrl 精确匹配的条目为 current)—— 均 PASS。
变异 —— 新边界是 load-bearing 的
去掉 findUnescapedUserInfoFallbackAt 里的 /^\d+$/ 端口判定后,https://host:99999/path@domain 塌缩成 https://domain,新测试用例转红(expected 'https://domain' to be 'https://host:99999/path@domain')。已回退;重新干净且全绿。
结论
新提交名副其实,修掉了一个真实的过度剥离边界;测试非空过(经变异验证);任何 live 路径都未泄漏凭据。之前的完整验证依然成立。4d47fb54a LGTM。 👍
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| : undefined; | ||
| } | ||
|
|
||
| function stripAcpAuthSuffix(modelId: string): string { |
There was a problem hiding this comment.
[Suggestion] stripAcpAuthSuffix is functionally identical to parseAcpBaseModelId in packages/cli/src/utils/acpModelUtils.ts — both locate the last (…) pair at the end of the string and strip it. The only difference is that parseAcpBaseModelId also trims whitespace.
Having two implementations across packages means future fixes to the stripping algorithm must be applied in two places. If one drifts, baseModelId values will diverge between daemon-local and webui contexts.
Consider exporting parseAcpBaseModelId from a shared package (e.g., @qwen-code/qwen-code-core) and reusing it here, or at minimum add a cross-reference comment in both files pointing to the other implementation.
— qwen3.7-max via Qwen Code /review
| for (const rawModel of availableModels) { | ||
| const model = getRecord(rawModel); | ||
| const modelId = | ||
| getString(model, 'modelId') ?? |
There was a problem hiding this comment.
[Suggestion] mapSessionContextModels has three fallback keys for model IDs (modelId → id → value), two for current model (currentModelId → currentModel), and four for context window (_meta.contextLimit → _meta.contextWindow → model.contextLimit → model.contextWindow). The integration tests in DaemonSessionProvider.test.tsx only exercise the primary key in each chain. Additionally, the early-return edge cases (undefined input at line 83, empty models at line 126) have no tests.
These fallbacks exist for backward compatibility with older daemon session context formats. If any regress, older daemon versions would silently lose model information with no test to catch it.
Consider adding a direct unit test for mapSessionContextModels that exercises the alternate keys and edge-case returns.
— qwen3.7-max via Qwen Code /review
…M#5089) Reverts the structural changes from QwenLM#5089 back to the pre-QwenLM#5089 shape: AuthType stays a fixed enum (not `string`), the Protocol enum is removed, modelProviders is `Record<authType, ModelConfig[]>` again (not `{ protocol, models }`), and createContentGenerator dispatches on authType. The v4->v5 settings migration is removed and SETTINGS_VERSION reverts to 4. Features merged on top of QwenLM#5089 are kept and re-adapted to the old enum+array structure (not reverted): - QwenLM#5632 fastOnly/voiceOnly model flags (test fixtures reshaped to arrays) - QwenLM#5638 workspace provider defaults (readProviderModels already tolerates both shapes; test fixtures reshaped to arrays) - QwenLM#5729 active-runtime-model listing (pre-QwenLM#5089 getAllConfiguredModels already enumerates Object.values(AuthType), so the runtime model is included natively) - QwenLM#5728 ACP set_config_option deterministic provider fixture (reshaped to array; the flake fix is preserved) KNOWN DOWNGRADE CAVEAT: settings already migrated to $version:5 (shipped in v0.19.0) retain the v5 `{ protocol, models }` modelProviders shape, which the reverted ModelRegistry consumes as an array. Such settings will throw on load until re-configured. A v5->v4 downgrade guard/migration is a separate follow-up if backward compatibility for migrated users is needed.
#5745) * revert(core): revert Protocol enum & model-identity decoupling (#5089) Reverts the structural changes from #5089 back to the pre-#5089 shape: AuthType stays a fixed enum (not `string`), the Protocol enum is removed, modelProviders is `Record<authType, ModelConfig[]>` again (not `{ protocol, models }`), and createContentGenerator dispatches on authType. The v4->v5 settings migration is removed and SETTINGS_VERSION reverts to 4. Features merged on top of #5089 are kept and re-adapted to the old enum+array structure (not reverted): - #5632 fastOnly/voiceOnly model flags (test fixtures reshaped to arrays) - #5638 workspace provider defaults (readProviderModels already tolerates both shapes; test fixtures reshaped to arrays) - #5729 active-runtime-model listing (pre-#5089 getAllConfiguredModels already enumerates Object.values(AuthType), so the runtime model is included natively) - #5728 ACP set_config_option deterministic provider fixture (reshaped to array; the flake fix is preserved) KNOWN DOWNGRADE CAVEAT: settings already migrated to $version:5 (shipped in v0.19.0) retain the v5 `{ protocol, models }` modelProviders shape, which the reverted ModelRegistry consumes as an array. Such settings will throw on load until re-configured. A v5->v4 downgrade guard/migration is a separate follow-up if backward compatibility for migrated users is needed. * feat(cli): add v5->v4 settings downgrade migration for #5089 revert After reverting #5089, settings already migrated to $version:5 (shipped in v0.19.0) carry a modelProviders `{ protocol, models }` shape that the reverted v4 readers consume as arrays, throwing "models is not iterable" on load. This adds the inverse migration so those configs auto-converge to v4 on load (the user-facing "automatically migrate $version:5 to 4"). - V5ToV4Migration: unwraps each modelProviders `{ protocol, models }` back to its `models` array, drops the now-implicit protocol (warning only when the explicit protocol differs from the key-derived one), and resets $version to 4. - DOWNGRADE_MIGRATIONS keeps the downgrade out of the ascending forward ALL_MIGRATIONS chain (preserving its invariants); runMigrations and needsMigration consider both via a combined convergence set. - needsMigration now gates on `=== SETTINGS_VERSION` instead of `>=`, so a newer-but-handled version (v5) is reported as needing migration while a genuinely unknown newer version (v6+) is still left untouched. Covered by unit tests for the migration, the framework wiring, and an end-to-end loadSettings downgrade-on-load test. * fix(test): align integration settings-version constant with reverted v4 The integration suites hard-coded CURRENT_SETTINGS_VERSION = 5 (introduced by #5676), which mismatched the reverted SETTINGS_VERSION = 4 and failed the migration assertions ($version now writes 4, not 5). Revert the constant to 4 in both settings-migration and qwen-config-dir integration tests. Verified: QWEN_SANDBOX=false vitest run --root ./integration-tests cli/settings-migration.test.ts cli/qwen-config-dir.test.ts → 21 passed. * fix: harden v5-era settings handling on the #5089 revert path Addresses /qreview feedback on the revert: - vscode findOpenaiModels: restore read-side tolerance for the V5 { protocol, models } shape. The extension reads/writes settings.json without running the CLI v5->v4 migration, so a not-yet-downgraded $version:5 file would otherwise return [] and silently drop existing OpenAI models on the next write. (Critical) - modelRegistry.registerAuthTypeModels: guard against a non-array provider value (skip + warn) instead of throwing an opaque "models is not iterable" — covers hand-edited or unmigrated files the downgrade misses. - needsMigration JSDoc: update the stale ">= SETTINGS_VERSION" wording to match the "=== SETTINGS_VERSION, else fall through" logic the downgrade path depends on. - settings.test.ts: also assert the v5->v4 downgrade is persisted to disk (.tmp write-back), not just the in-memory merged result. Adds tests for the registry guard and the vscode V5 read tolerance. * fix(core): break contentGenerator import cycle + cover reverted error paths Addresses /review suggestions on the revert: - contentGenerator: import PROVIDER_SOURCED_FIELDS from constants.js (where it is actually defined) instead of modelsConfig.js, breaking the runtime import cycle contentGenerator -> modelsConfig -> contentGenerator. constants.js only references contentGenerator at the type level, which is erased at runtime, so no cycle remains. - contentGenerator.test: add coverage for the two authType error paths the revert restored (missing authType -> "must have an authType"; unknown authType -> "Unsupported authType"), which #5089's protocol-based tests had replaced. Neither was covered before. The acpAgent z.nativeEnum(AuthType).parse(methodId) suggestion is left as-is: that line is byte-identical to pre-#5089, so it is pre-existing behavior the revert faithfully restores rather than a regression of this PR.
What this PR does
This PR makes
GET /workspace/providersrepresent the workspace-level model catalog plus the default model that the next new session will use. The route now builds that snapshot daemon-side from fresh workspace settings and environment on every request instead of depending on a live ACP/session config, while still reporting whether the ACP channel is live as separate diagnostic state.Existing sessions now initialize their current model and model list from session context model state first, with workspace providers used only as a fallback. When a session model change persists a new default model, the daemon emits a workspace settings change signal so clients that cache workspace provider data can reload.
Why it's needed
The previous behavior let
/workspace/providersreflect live provider/session memory, which could be stale or unavailable before a session existed. That made landing pages and new-session entry points show the wrong default model, and it also encouraged clients to treat the workspace default as an existing session's current model. Separating workspace defaults from session context makes the pre-session default display fresh and keeps active session model state session-scoped.Reviewer Test Plan
How to verify
Call
GET /workspace/providersbefore creating a session after changing model settings and confirmcurrent.modelIdreflects the latest settings whileinitializedremains true even without a live ACP channel. In an existing session, set the session context current model to a different value from the workspace provider default and confirm the WebUI connection state shows the session context model, not the workspace default. For duplicate model IDs with different base URLs, confirm only the exact persisted base URL entry is marked current and credentials in provider base URLs are not exposed.Focused checks run locally:
cd packages/cli && npx vitest run src/serve/workspace-providers-status.test.ts src/serve/workspace-service/__tests__/facade.test.ts;cd packages/cli && npx vitest run src/serve/server.test.ts -t "returns workspace skills from the bridge and providers from daemon-local settings";cd packages/acp-bridge && npx vitest run src/bridge.test.ts -t "setSessionModel";cd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx -t "session context models|provider models when session context|unmatched session model";cd packages/acp-bridge && npm run typecheck;cd packages/webui && npm run typecheck;cd packages/sdk-typescript && npm run typecheck.Evidence (Before & After)
N/A for screenshots. This is daemon/API and WebUI state-source behavior covered by focused unit tests.
Tested on
Environment (optional)
macOS local checkout, Node v22.22.3, npm 10.9.8.
Risk & Scope
/workspace/providersnow reports workspace defaults instead of live session model state, so clients that wanted a session current model must use session context instead.BaseTextInput.tsxInk subpath type resolution errors unrelated to this change.acpChannelLiveis additive.Linked Issues
N/A
中文说明
What this PR does
这个 PR 将
GET /workspace/providers明确定义为 workspace 级模型 catalog 加下一次新建 session 会使用的默认模型。该接口现在由 daemon 侧每次请求从最新 workspace settings 和环境变量构建快照，不再依赖 live ACP/session config，同时把 ACP channel 是否在线作为独立诊断状态返回。已有 session 现在优先从 session context 的模型状态初始化当前模型和模型列表，只有缺少 context 模型信息时才 fallback 到 workspace providers。session 模型切换如果持久化了新的默认模型，daemon 会发送 workspace settings change 信号，让缓存 workspace provider 数据的客户端可以重新拉取。
Why it's needed
之前
/workspace/providers可能反映 live provider/session 内存；在还没创建 session 时，这份内存可能不可用或过期，导致首页和新建 session 入口展示错误的默认模型，也会诱导客户端把 workspace default 当成已有 session 的 current model。拆分 workspace default 和 session context 后，新建 session 前的默认模型展示会保持 fresh，已有 session 的模型状态也保持 session-scoped。Reviewer Test Plan
How to verify
修改模型 settings 后、创建 session 之前调用
GET /workspace/providers，确认current.modelId反映最新 settings，并且即使 ACP channel 不在线initialized仍为 true。对于已有 session，将 session context current model 设置成不同于 workspace provider default 的值，确认 WebUI connection state 展示 session context model，而不是 workspace default。对于相同 model id 但 base URL 不同的 provider，确认只有精确匹配持久化 base URL 的条目标记为 current，并且 provider base URL 不泄露凭据。本地已运行的聚焦检查：
cd packages/cli && npx vitest run src/serve/workspace-providers-status.test.ts src/serve/workspace-service/__tests__/facade.test.ts；cd packages/cli && npx vitest run src/serve/server.test.ts -t "returns workspace skills from the bridge and providers from daemon-local settings"；cd packages/acp-bridge && npx vitest run src/bridge.test.ts -t "setSessionModel"；cd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx -t "session context models|provider models when session context|unmatched session model"；cd packages/acp-bridge && npm run typecheck；cd packages/webui && npm run typecheck；cd packages/sdk-typescript && npm run typecheck。Evidence (Before & After)
N/A，无截图。这是 daemon/API 和 WebUI 状态来源行为变更，已由聚焦单测覆盖。
Tested on
Environment (optional)
macOS 本地 checkout，Node v22.22.3，npm 10.9.8。
Risk & Scope
/workspace/providers现在表达 workspace default，而不是 live session model state；需要 session current model 的客户端应改用 session context。BaseTextInput.tsxInk 子路径类型解析错误失败，和本次改动无关。acpChannelLive是新增可选字段。Linked Issues
N/A