fix(cli): skip unusable A2UI configs - #5685
Conversation
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ — all required sections present and well-filled. On direction: this is a straightforward bug fix for #5682. The A2UI discovery path accepting garbage configs (empty commands, malformed URLs) and selecting them over valid ones is a real reliability problem — 502s instead of graceful fallback. Clearly within scope, clearly worth fixing. On approach: the scope is tight — one function tightened, tests added for exactly the bug scenarios described. The logic mirrors Moving on to code review and testing. 🔍 中文说明感谢贡献，@tt-a1i！ 模板完整 ✓ — 所有章节齐全，填写到位。 方向：这是对 #5682 的直接 bug 修复。A2UI discovery 接受垃圾配置（空 command、畸形 URL）并优先选中它们导致 502，是真实的可靠性问题。明确在范围内，明确值得修。 方案：范围紧凑——一个函数加固，测试精确覆盖描述的 bug 场景。逻辑与 进入代码审查和测试。🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe diff is clean and minimal — exactly what this bug needs.
Tests cover the right scenarios: runtime discovery with bad-then-good configs, mixed No correctness issues, no security concerns, no AGENTS.md violations. The change is focused — no drive-by refactors, no scope creep. Test ResultsApplied the PR diff to main and ran the test suite: a2ui-action.test.ts (19 tests)All serve/routes (3 files, 60 tests)ESLintAll green. Moving to final reflection. 🧠 中文说明代码审查Diff 干净且最小化——正是这个 bug 需要的。
测试 覆盖了正确的场景：runtime discovery 中坏配置在前有效配置在后、混合 没有正确性问题，没有安全隐患，没有违反 AGENTS.md。改动聚焦——没有顺手重构，没有范围膨胀。 测试结果将 PR diff 应用到 main 并运行测试套件，全部通过（19 个 a2ui-action 测试，60 个 serve/routes 测试）。ESLint 无错误。 一切正常。进入最终反思。🧠 — Qwen Code · qwen3.7-max |
|
Stepping back: this is a textbook bug fix. Issue #5682 describes a real reliability problem — stale or half-edited A2UI configs getting picked by discovery and causing 502s when a valid server sits right behind them. The fix is 12 lines of source code in the exact function that needs tightening. The logic mirrors My independent proposal before reading the diff was essentially identical: tighten The test results confirm it works: 19 tests in the affected file, 60 across all route tests, lint clean. The new tests are well-targeted — they test the actual bug scenarios (bad-before-good ordering, mixed configs, settings fallback), not just the helper function in isolation. This is a focused, minimal, well-tested fix. Ready to ship. ✅ 中文说明退一步看：这是一个教科书式的 bug 修复。Issue #5682 描述了真实的可靠性问题——过期或编辑到一半的 A2UI 配置被 discovery 选中，导致 502，而有效服务器就在后面。修复是 12 行源代码，恰好在需要加固的函数中。逻辑正确镜像了 我在读 diff 之前的独立方案基本一致：用 URL 解析 + 测试结果确认有效：受影响文件 19 个测试，所有 route 测试 60 个，lint 干净。新测试目标明确——测试实际 bug 场景（坏配置在好配置前、混合配置、settings fallback），而不只是隔离测试 helper 函数。 聚焦、最小、测试充分。可以合并。✅ — 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 — focused bug fix, clean implementation, comprehensive tests. Ready to ship. ✅
PR #5685 verification — local build + live
|
| input | base | PR | correct? |
|---|---|---|---|
{command:"node"} |
true | true | ✓ |
{command:""} |
true | false | ✓ (base wrong) |
{command:" "} |
true | false | ✓ (base wrong) |
{httpUrl:"http://x/mcp"} |
true | true | ✓ |
{httpUrl:"https://x/mcp"} |
true | true | ✓ |
{httpUrl:"not a url"} |
true | false | ✓ (base wrong) |
{httpUrl:"ftp://x/mcp"} |
true | false | ✓ (base wrong) |
{command:"node",httpUrl:"not a url"} |
true | false | ✓ (base wrong) |
{} / undefined |
false | false | ✓ |
Base wrongly accepts 5 unusable shapes; the PR rejects all of them and keeps every valid shape.
2) Route integration tests (real Express + supertest)
- merged:
a2ui-action.test.ts19/19, wholeserve/routes60/60. - Control: replaying the PR's test file on base source fails exactly the 4 new tests (the 3 skip-scenarios + the
usableServerConfighelper); the 15 pre-existing pass. So the new tests are load-bearing and isolate the change. The skip-scenarios assert that with a bad-then-good server list, discovery selects the later valid config and returns 200.
3) Bundle freshness
- base bundle:
usableServerConfig=!!cfg && (typeof cfg.command === "string" || typeof cfg.httpUrl === "string"). - merged bundle: the
new URL()+protocol+command.trim().lengthversion. (The two bundles differ; confirmed the change is shipped.)
4) Live daemon E2E — real qwen serve, real HTTP
Workspace .qwen/settings.json with a single bad A2UI server {"a2ui-bad": {"httpUrl": "not a url"}}; boot the real daemon, create a session, POST /session/:id/a2ui-action {"name":"go"}:
| HTTP | body | daemon stderr | |
|---|---|---|---|
| BASE | 502 | {"error":"a2ui action call failed"} |
a2ui-action proxy failed: Invalid URL |
| MERGED | 503 | {"error":"no a2ui MCP server found …"} |
(none — never reached buildTransport) |
This is the bug exactly: on base the unusable config is selected, buildTransport runs new URL("not a url") → throws → a misleading 502 "action call failed". On merged the config is skipped, so the client gets the honest 503 "no a2ui MCP server found". (And per §2, when a later valid server exists, merged selects it and returns 200.) Net: the fix removes the false-502 and makes discovery resilient to stale/partial A2UI configs.
Notes
- Scope is correct and matches the PR description:
httpUrlprecedence means a config with a validcommandbut invalidhttpUrlis now skipped — intended, becausebuildTransportwould fail it anyway. Legacy SSEurlremains unsupported (unchanged). - No regressions: full
serve/routessuite green; the only base-vs-merged behavior delta is the intended skip.
Verdict
Approve / merge-ready. Correct predicate, comprehensive tests, shipped in the bundle, and proven end-to-end on the real daemon (502→correct behavior). Clean, well-scoped bug fix.
中文版（点击展开）
PR #5685 验证 —— 本地构建 + 真实 qwen serve daemon E2E
对断言函数、route、以及真实发布的 daemon（走 HTTP）都做了验证。结论：修复正确、覆盖完整，在真实 qwen serve 二进制上把原本错误的 502 变成了正确行为。建议合并。
PR 内容
强化 serve/routes/a2ui-action.ts 里的 usableServerConfig，让 A2UI MCP discovery 在选择候选前跳过不可用配置。只有「非空且去空白后非空的 stdio command」或「能解析为 http:/https: 的 httpUrl」才算可用。httpUrl 优先（与 buildTransport 一致），所以 {command, 非法 httpUrl} 会被正确拒绝。+86/−4（大部分是测试）。
方法
- 合并到当前
main@2fd2104fa（干净 fast-forward，仅 2 个 PR 文件）；构建两个真实 esbuild bundle（base + merged）。 - 4 层验证：纯函数 A/B、route 集成测试、bundle 新鲜度、真实 daemon HTTP E2E。
1）纯函数 A/B（usableServerConfig，忠实复制 base 与 PR 的函数体）
| 输入 | base | PR | 正确? |
|---|---|---|---|
{command:"node"} |
true | true | ✓ |
{command:""} |
true | false | ✓（base 错） |
{command:" "} |
true | false | ✓（base 错） |
{httpUrl:"http://x/mcp"} |
true | true | ✓ |
{httpUrl:"https://x/mcp"} |
true | true | ✓ |
{httpUrl:"not a url"} |
true | false | ✓（base 错） |
{httpUrl:"ftp://x/mcp"} |
true | false | ✓（base 错） |
{command:"node",httpUrl:"not a url"} |
true | false | ✓（base 错） |
{} / undefined |
false | false | ✓ |
base 错误地把 5 种不可用配置判为可用；PR 全部拒绝，且保留所有合法配置。
2）route 集成测试（真实 Express + supertest）
- merged：
a2ui-action.test.ts19/19，整个serve/routes60/60。 - 对照：把 PR 的测试文件放到 base 源码上跑，恰好 4 个新测试失败（3 个 skip 场景 +
usableServerConfighelper）；15 个既有测试通过。所以新测试真正起作用并隔离了改动。skip 场景断言：当 server 列表是「坏的在前、好的在后」时，discovery 选中后面那个有效配置并返回 200。
3）bundle 新鲜度
- base bundle：
usableServerConfig=!!cfg && (typeof cfg.command === "string" || typeof cfg.httpUrl === "string")。 - merged bundle：
new URL()+protocol+command.trim().length版本。（两个 bundle 确实不同，确认改动已打进发布产物。）
4）真实 daemon E2E —— 真实 qwen serve、真实 HTTP
工作区 .qwen/settings.json 只放一个坏的 A2UI server {"a2ui-bad": {"httpUrl": "not a url"}}；启动真实 daemon，创建 session，POST /session/:id/a2ui-action {"name":"go"}：
| HTTP | body | daemon stderr | |
|---|---|---|---|
| BASE | 502 | {"error":"a2ui action call failed"} |
a2ui-action proxy failed: Invalid URL |
| MERGED | 503 | {"error":"no a2ui MCP server found …"} |
(无——根本没走到 buildTransport) |
这正是 bug：base 上不可用配置被选中，buildTransport 执行 new URL("not a url") → 抛错 → 误导性的 502「action call failed」。merged 上该配置被跳过，客户端得到诚实的 503「no a2ui MCP server found」。（且据 §2，当后面存在有效 server 时，merged 会选中它并返回 200。）净效果：修复消除了假 502，并让 discovery 对过期/残缺的 A2UI 配置更稳健。
备注
- 范围正确，与描述一致：
httpUrl优先意味着「有合法command但httpUrl非法」的配置现在会被跳过——这是有意的，因为buildTransport本来也会让它失败。legacy SSEurl仍不支持（未改动）。 - 无回归：整个
serve/routes套件通过；base 与 merged 唯一的行为差异就是这个有意的跳过。
结论
建议合并。 断言正确、测试完整、已打进 bundle，并在真实 daemon 上端到端验证（502→正确行为）。干净、范围清晰的 bug 修复。
Verification: merged onto main@2fd2104fa; two esbuild bundles (base + merged); pure-fn A/B of usableServerConfig (base wrong on 5/10); a2ui-action.test.ts 19/19 + serve/routes 60/60; control = 4 new tests fail on base; bundle freshness confirmed; live qwen serve E2E with a bad-only a2ui config → base 502 ("Invalid URL"), merged 503 ("no a2ui MCP server found").
3 participants
What this PR does
Makes A2UI MCP server discovery skip unusable server configs before selecting a candidate for
POST /session/:id/a2ui-action.A config is now considered usable only when it has either a non-empty stdio
commandor a validhttp:/https:streamable HTTPhttpUrl. WhenhttpUrlis present, discovery validates it the same waybuildTransport()will use it, so mixed configs with an invalidhttpUrldo not get selected just because they also include acommand.Why it's needed
Before this change, discovery treated any string
httpUrlor any stringcommandas usable. A bad A2UI server entry such as{ "httpUrl": "not a url" },{ "command": "" }, or{ "command": "node", "httpUrl": "not a url" }could be selected ahead of a later valid A2UI server. The route would then fail in the proxy path with502instead of skipping the unusable candidate.This makes runtime MCP status discovery and workspace settings fallback more robust when stale or partially edited A2UI server configs exist.
Reviewer Test Plan
How to verify
Create a runtime A2UI server list where an unusable A2UI config appears before a valid one, then call
POST /session/:id/a2ui-action. Confirm the route skips the bad candidate and uses the later valid config.Repeat through the workspace settings fallback path by making daemon MCP status unavailable and placing a bad A2UI config before a valid one in
.qwen/settings.json.Confirm helper-level behavior: empty stdio commands, invalid URLs, non-http(s) URLs, and mixed
commandplus invalidhttpUrlall return unusable.Evidence (Before & After)
Before: A bad A2UI config could be selected and cause
502, even when a later valid A2UI server was available.After:
npm test --workspace=packages/cli -- serve/routes/a2ui-action.test.tspasses with 19 tests, including runtime discovery, settings fallback, and mixed invalidhttpUrlregression coverage.After:
npm test --workspace=packages/cli -- serve/routespasses with 3 test files and 60 tests.After:
npm run lint --workspace=packages/cli --if-presentpasses.After:
npx prettier --experimental-cli --check packages/cli/src/serve/routes/a2ui-action.ts packages/cli/src/serve/routes/a2ui-action.test.tspasses.After:
git diff --checkpasses.Tested on
Environment (optional)
Local macOS workspace with the repository npm dependencies installed.
Risk & Scope
httpUrlare now skipped even if they also contain a valid stdiocommand, becausebuildTransport()giveshttpUrlprecedence and would fail the same config at call time.urlsupport, which remains intentionally unsupported in this route.Linked Issues
Fixes #5682
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
让 A2UI MCP server discovery 在为
POST /session/:id/a2ui-action选择候选 server 之前跳过不可用的 server 配置。现在只有包含非空 stdio
command，或合法http:/https:streamable HTTPhttpUrl的配置才会被视为可用。如果配置里存在httpUrl，discovery 会按buildTransport()实际使用它的方式先校验它，因此带有无效httpUrl的混合配置不会因为同时有command就被选中。Why it's needed
在这个改动之前，discovery 会把任意字符串
httpUrl或任意字符串command视为可用。像{ "httpUrl": "not a url" }、{ "command": "" }，或者{ "command": "node", "httpUrl": "not a url" }这样的坏 A2UI server entry 可能会排在后续有效 A2UI server 前面并被选中。随后 route 会在 proxy path 中返回502，而不是跳过这个不可用候选。这个改动让 runtime MCP status discovery 和 workspace settings fallback 在存在过期或编辑到一半的 A2UI server 配置时更稳健。
Reviewer Test Plan
How to verify
构造一个 runtime A2UI server 列表，让不可用的 A2UI 配置排在有效配置之前，然后调用
POST /session/:id/a2ui-action。确认 route 会跳过坏候选并使用后面的有效配置。通过 workspace settings fallback 路径重复验证：让 daemon MCP status 不可用，并在
.qwen/settings.json中把坏 A2UI 配置放在有效配置前面。确认 helper 层行为：空 stdio command、无效 URL、非 http(s) URL，以及
command加无效httpUrl的混合配置都会被判断为不可用。Evidence (Before & After)
Before：坏 A2UI config 可能被选中并导致
502，即使后面还有有效 A2UI server。After：
npm test --workspace=packages/cli -- serve/routes/a2ui-action.test.ts通过，19 个测试覆盖 runtime discovery、settings fallback，以及 mixed invalidhttpUrl回归场景。After：
npm test --workspace=packages/cli -- serve/routes通过，3 个测试文件共 60 个测试。After：
npm run lint --workspace=packages/cli --if-present通过。After：
npx prettier --experimental-cli --check packages/cli/src/serve/routes/a2ui-action.ts packages/cli/src/serve/routes/a2ui-action.test.ts通过。After：
git diff --check通过。Tested on
Environment (optional)
本地 macOS 工作区，已安装仓库 npm 依赖。
Risk & Scope
httpUrl的 A2UI 配置现在会被跳过，即使它同时包含有效 stdiocommand；这是因为buildTransport()会优先使用httpUrl，同一个配置在调用时也会失败。url支持，该 route 仍然有意不支持它。Linked Issues
Fixes #5682
AI Assistance Disclosure
我使用 Codex 来审查改动、对照现有模式做 sanity check，并帮助发现潜在边界情况。