Skip to content

fix(extensions): accept uppercase marketplace source schemes - #5435

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/extension-marketplace-uppercase-source
Jun 21, 2026
Merged

fix(extensions): accept uppercase marketplace source schemes#5435
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/extension-marketplace-uppercase-source

Conversation

@tt-a1i

@tt-a1i tt-a1i commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Accepts uppercase HTTP:// / HTTPS:// schemes in the extension marketplace paths that were still doing case-sensitive prefix checks. The source registry, marketplace config loader, and Claude marketplace plugin string-source resolver now lowercase only for scheme comparison while preserving the original source string for parsing, fetching, and install metadata.

It also makes marketplace direct-JSON fetches reuse the shared clientForUrl() helper introduced for npm registry URLs, so marketplace and npm extension downloads now choose http.get vs https.get from one parsed-URL protocol helper instead of carrying duplicate client-selection logic.

Why it's needed

URL schemes are case-insensitive. #5429 fixed this for extension install sources, but marketplace sources still misclassified HTTPS://... as local and skipped the remote loading path. The same gap existed for string source entries inside Claude marketplace plugins.

Reviewer Test Plan

How to verify

Run the focused extension tests and core validation commands below. The important regression cases are uppercase GitHub marketplace sources, uppercase direct JSON marketplace sources, uppercase HTTP direct JSON marketplace sources, uppercase marketplace plugin string sources, and uppercase npm registry/tarball URLs continuing to use the shared helper.

Evidence (Before & After)

Before: the new uppercase scheme tests failed because HTTPS://... was classified as local or skipped the HTTP(S) loader. HTTP://... could also enter a remote path that still called https.get, and npm/marketplace carried separate protocol-to-client helpers.

After: npx vitest run packages/core/src/extension/npm.test.ts packages/core/src/extension/marketplace.test.ts packages/core/src/extension/sourceRegistry.test.ts packages/core/src/extension/claude-converter.test.ts passes with 120 tests.

Additional local checks passed:

npm run typecheck --workspace=packages/core
npm run lint --workspace=packages/core
npm run build --workspace=packages/core
git diff --check

Tested on

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

Environment (optional)

Node.js v26.3.0, npm 11.16.0.

Risk & Scope

  • Main risk or tradeoff: http:// direct marketplace JSON sources now use the HTTP client instead of accidentally going through the HTTPS client path; npm and marketplace now share one URL-client helper.
  • Not validated / out of scope: end-to-end interactive /extensions TUI testing; this is covered at the core parser/loader level.
  • Breaking changes / migration notes: none.

Linked Issues

Fixes #5434

中文说明

这个 PR 做了什么

让 extension marketplace 相关路径接受大写 HTTP:// / HTTPS:// scheme。source registry、marketplace config loader、Claude marketplace 插件里的字符串 source 解析,现在只在比较 scheme 时做 lowercase,后续解析、请求和安装 metadata 仍保留用户输入的原始字符串。

同时,marketplace direct-JSON fetch 复用 npm registry URL 已引入的共享 clientForUrl() helper,让 marketplace 和 npm extension download 都通过同一个 parsed-URL protocol helper 选择 http.gethttps.get,避免重复实现 client-selection 逻辑。

为什么需要

URL scheme 按规范是大小写不敏感的。#5429 已经修了 extension install source,但 marketplace source 仍会把 HTTPS://... 误判成本地路径,从而跳过远程加载。Claude marketplace 插件内部的字符串 source 也有同类问题。

Reviewer Test Plan

如何验证

运行上面的 focused extension tests 和 core validation commands。关键回归用例覆盖大写 GitHub marketplace source、大写 direct JSON marketplace source、大写 HTTP direct JSON marketplace source、大写 marketplace plugin string source,以及大写 npm registry/tarball URL 继续使用共享 helper。

证据(Before & After)

Before:新增的大写 scheme 测试失败,因为 HTTPS://... 会被归类为 local 或跳过 HTTP(S) loader。HTTP://... 也可能进入远程路径后仍调用 https.get,并且 npm/marketplace 各自有一份 protocol-to-client helper。

After:npx vitest run packages/core/src/extension/npm.test.ts packages/core/src/extension/marketplace.test.ts packages/core/src/extension/sourceRegistry.test.ts packages/core/src/extension/claude-converter.test.ts 通过,共 120 个测试。

额外本地检查也已通过:

npm run typecheck --workspace=packages/core
npm run lint --workspace=packages/core
npm run build --workspace=packages/core
git diff --check

Tested on

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

Environment (optional)

Node.js v26.3.0, npm 11.16.0.

风险和范围

  • 主要风险或取舍:http:// direct marketplace JSON source 现在会使用 HTTP client,而不是误走 HTTPS client;npm 和 marketplace 现在共用一个 URL-client helper。
  • 未验证 / 不在范围内:未做 /extensions TUI 端到端交互验证;本 PR 在 core parser/loader 层覆盖。
  • Breaking changes / migration notes:无。

Linked Issues

Fixes #5434

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

@wenshao

wenshao commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

✅ Local real-build verification — LGTM

I built this PR at its head (dc9e9380f, base 61dcf865de) in an isolated worktree (npm ci && npm run build) and verified it on three independent layers: a mutation test, a built-dist function A/B against a real local HTTP marketplace server, and a real-binary qwen extensions sources A/B in tmux. Pre-fix vs fixed was produced by reverting only the 3 production files and rebuilding @qwen-code/qwen-code-core (the CLI resolves core/dist at runtime, so the same cli/dist binary picks up each variant). Environment: macOS, Node v22.22.2.

Verdict: the fix is correct, the new tests genuinely guard it, there is no regression, and it incidentally closes a latent crash. Safe to merge.

1. Mutation test — the 7 new tests are non-vacuous

Reverted only sourceRegistry.ts / marketplace.ts / claude-converter.ts to base, kept the PR's tests, ran the 3 suites:

Tests  7 failed | 85 passed (92)
  • sourceRegistry.test.ts — the 3 new uppercase rows fail; every lowercase row still passes → bug is uppercase-only.
  • marketplace.test.ts — the 3 new uppercase loader tests fail.
  • claude-converter.test.ts — fails with the smoking gun: Plugin source not found at …/plugin-uppercase-url/HTTPS:/github.com/owner/plugin → pre-fix treats HTTPS://… as a local path.

On the fixed source all 92 pass (matches the PR's claim).

2. Built-dist function A/B (real fetch, no mocks)

parseExtensionSourceType (pure):

Source Pre-fix Fixed
https://github.com/owner/repo github github
HTTPS://github.com/owner/repo local github
Https://GitHub.com/owner/repo local github
HTTPS://example.com/marketplace.json local http
HTTP://example.com/marketplace.json local http
GIT@github.com:owner/repo.git local git
SSO://team/repo local git
lowercase https/http/git@/sso + owner/repo + ./local correct correct

loadMarketplaceConfigFromSource against a real local HTTP server (127.0.0.1, served valid marketplace.json):

Source Pre-fix Fixed
http://127.0.0.1:PORT/marketplace.json throws ERR_INVALID_PROTOCOL "Protocol http: not supported. Expected https:" loaded ✅
HTTP://127.0.0.1:PORT/marketplace.json null (Priority-2 skipped) loaded ✅

The server logged the real http.get hits on the fixed run, confirming the protocol routing actually takes effect.

3. Real-binary A/B in tmux — qwen extensions sources

Same cli/dist/index.js, isolated HOME, against the local server:

Command Pre-fix Fixed
sources add HTTP://…/marketplace.json (uppercase) ✗ exit 1 — "looks like a single extension, not a marketplace" Added marketplace "mp-uppercase"Type: http
sources add http://…/marketplace.json (http routing) ✗ exit 1 — "Protocol http: not supported. Expected https:" Added marketplace "mp-lowercase"Type: http
sources list No marketplace sources added yet. both listed, Type: http
tmux pane transcript (fixed)
===== [FIXED] add UPPERCASE scheme:  HTTP://127.0.0.1:38573/marketplace.json?n=mp-uppercase =====
Added marketplace "mp-uppercase".
  EXIT=0
===== [FIXED] add lowercase http (http-vs-https routing):  http://127.0.0.1:38573/marketplace.json?n=mp-lowercase =====
Added marketplace "mp-lowercase".
  EXIT=0
===== [FIXED] sources list =====
mp-uppercase
 Source: HTTP://127.0.0.1:38573/marketplace.json?n=mp-uppercase (Type: http)
mp-lowercase
 Source: http://127.0.0.1:38573/marketplace.json?n=mp-lowercase (Type: http)
  EXIT=0

4. Bonus: the PR also closes a latent timer crash 🎁

When https.get threw synchronously on an http:// URL, the pre-fix fetchUrl left its 10s hardDeadline timer armed (never cleared), and the timer body referenced the const req that was never initialized. ~10s later it crashes the process:

ReferenceError: Cannot access 'req' before initialization
    at Timeout._onTimeout (…/marketplace.js:115:13)

This is masked in the one-shot CLI by the immediate process.exit(1), but a long-lived flow (Discover → discoverPlugins) would hit the uncaught timer. The PR's rework (let req: ClientRequest | undefined, req?.destroy(), try/catch around client.getdone(null)) eliminates it — the fixed harness exits cleanly.

5. Minor, non-blocking notes

  • http:// marketplace sources now genuinely fetch over plaintext HTTP. This is the intended behavior and only when the user explicitly supplies an http:// URL; the body-size cap and wall-clock timeout guards are preserved through the refactor.
  • The git@ / sso:// uppercase paths (lowerTrimmed.startsWith('git@'), regex /i) and the claude-converter HTTP:// (vs the tested HTTPS://) variant are covered by the fix but have no dedicated unit row — both verified green here, just a small coverage suggestion for a follow-up.
🇨🇳 中文版(完整对应)

✅ 本地真实构建验证 —— 可以合并

我在隔离 worktree 中以 PR 最新提交(dc9e9380f,base 61dcf865de)完整构建(npm ci && npm run build),并从三个相互独立的层面做了验证:变异测试针对真实本地 HTTP marketplace 服务器的构建产物函数 A/B、以及 tmux 中真实 qwen extensions sources 二进制 A/B。pre-fix 与 fixed 的对比是通过「只回退 3 个生产文件并重新构建 @qwen-code/qwen-code-core」得到的(CLI 在运行时解析 core/dist,所以同一个 cli/dist 二进制会自动加载各自的变体)。环境:macOS,Node v22.22.2。

结论:修复正确,新增测试确实能守住该修复,无回归,并且顺带消除了一个潜在崩溃。可以合并。

1. 变异测试 —— 7 个新测试非空过

只把 sourceRegistry.ts / marketplace.ts / claude-converter.ts 回退到 base,保留 PR 的测试,跑 3 个测试套件:

Tests  7 failed | 85 passed (92)
  • sourceRegistry.test.ts —— 3 个新增的大写用例失败,所有小写用例仍通过 → 该 bug 只影响大写
  • marketplace.test.ts —— 3 个新增的大写 loader 测试失败。
  • claude-converter.test.ts —— 失败信息是铁证:Plugin source not found at …/plugin-uppercase-url/HTTPS:/github.com/owner/plugin → pre-fix 把 HTTPS://… 当成了本地路径

在修复后的源码上 92 个全部通过(与 PR 描述一致)。

2. 构建产物函数 A/B(真实请求,无 mock)

parseExtensionSourceType(纯函数):

源串 Pre-fix Fixed
https://github.com/owner/repo github github
HTTPS://github.com/owner/repo local github
Https://GitHub.com/owner/repo local github
HTTPS://example.com/marketplace.json local http
HTTP://example.com/marketplace.json local http
GIT@github.com:owner/repo.git local git
SSO://team/repo local git
小写 https/http/git@/sso + owner/repo + ./local 正确 正确

loadMarketplaceConfigFromSource 针对真实本地 HTTP 服务器127.0.0.1,返回合法 marketplace.json):

源串 Pre-fix Fixed
http://127.0.0.1:PORT/marketplace.json ERR_INVALID_PROTOCOL「Protocol http: not supported. Expected https:」 成功加载 ✅
HTTP://127.0.0.1:PORT/marketplace.json null(跳过 Priority-2) 成功加载 ✅

fixed 这一侧服务器记录到真实的 http.get 请求,证明协议路由确实生效。

3. tmux 中真实二进制 A/B —— qwen extensions sources

同一个 cli/dist/index.js,隔离 HOME,对本地服务器:

命令 Pre-fix Fixed
sources add HTTP://…/marketplace.json(大写) ✗ 退出码 1 —— "looks like a single extension, not a marketplace" Added marketplace "mp-uppercase" —— Type: http
sources add http://…/marketplace.json(http 路由) ✗ 退出码 1 —— "Protocol http: not supported. Expected https:" Added marketplace "mp-lowercase" —— Type: http
sources list No marketplace sources added yet. 两条都在,Type: http

4. 额外收获:本 PR 还顺带修掉一个潜在的定时器崩溃 🎁

https.get 在遇到 http:// URL 时同步抛错,pre-fix 的 fetchUrl 会让那个 10s 的 hardDeadline 定时器一直挂着(从未 clear),而定时器体里引用了那个从未被初始化的 const req。约 10s 后进程崩溃:

ReferenceError: Cannot access 'req' before initialization
    at Timeout._onTimeout (…/marketplace.js:115:13)

在一次性 CLI 里,它被紧随其后的 process.exit(1) 掩盖了;但长期存活的流程(Discover → discoverPlugins)会真正触发这个未捕获的定时器异常。PR 的改写(let req: ClientRequest | undefinedreq?.destroy()、在 client.get 外包 try/catchdone(null))消除了它 —— fixed 的 harness 干净退出。

5. 非阻塞的小提示

  • http:// marketplace 源现在会真正走明文 HTTP 请求。这是预期行为,且仅当用户显式提供 http:// URL 时发生;重构过程中保留了响应体大小上限与墙钟超时这两个保护。
  • 大写 git@ / sso:// 路径(lowerTrimmed.startsWith('git@')、正则 /i)以及 claude-converterHTTP://(相对已测的 HTTPS://)变体都被修复覆盖,但没有专门的单测用例 —— 这里都已验证为绿,仅作为后续补测的小建议。

Verified locally by a maintainer on a real build (worktree npm ci && npm run build); pre-fix/fixed via source revert + core rebuild. Not an automated CI run.

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

The case-insensitive scheme handling is correct and consistent across claude-converter.ts / marketplace.ts / sourceRegistry.ts; the fetchUrl http/https dispatch is sound (timers cleaned up, req? null-safe, no regression vs the https-only version — it even removes a latent TDZ); and the new tests are non-vacuous (mutation-checked: each fails when its fix is reverted). Lint, types, and the 3 affected suites (92 tests) pass locally; CI green.

Two non-blocking suggestions inline. Verdict: Comment.

中文版

大小写不敏感的 scheme 处理在 claude-converter.ts / marketplace.ts / sourceRegistry.ts 三处都正确且一致;fetchUrl 的 http/https 分发也是稳的(定时器有清理、req? 空安全、相比只支持 https 的旧版无回归,还顺手消除了一个潜在的 TDZ);新增测试非空过(做了变异测试:回退对应修复时各自会失败)。本地 lint、类型、3 个相关测试套件(92 个用例)均通过;CI 全绿。

两条非阻塞建议见行内评论。结论:Comment

— claude-opus-4-8 via Claude Code /qreview

try {
const protocol = new URL(url).protocol.toLowerCase();
if (protocol === 'http:') {
client = http;

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] Nice — this makes plaintext http:// marketplace fetches actually work (previously fetchUrl was https-only, so an http:// source failed at the TLS layer; this closes the deferred follow-up from #4850). The refactor itself is solid: hardDeadline is still cleared in done(), req?.destroy() is null-safe, and the client.get try/catch handles a synchronous throw.

One security consideration now that it's functional: marketplace.json drives the pre-consent Discover list and the per-plugin installSource that flows into cloneFromGit / downloadFromGitHubRelease. Fetched over plaintext http://, an on-path attacker can rewrite that body and redirect an install to an attacker-controlled repo. (http.get doesn't follow redirects, so this is metadata-MITM, not redirect-SSRF; and it's bounded — the source is user-added and install still requires consent.) Worth considering a guard: restrict http:// to loopback / RFC-1918 hosts, or a one-line "insecure http source" warning at add time, so a network MITM can't silently hijack install routing. Likely a maintainer call — not a blocker.

中文

这次让明文 http:// 的 marketplace 拉取真正生效了(之前 fetchUrl 只支持 https,http:// 源会在 TLS 层失败;补上了 #4850 里延后的后续项)。重构本身没问题:done() 仍会清掉 hardDeadline,req?.destroy() 是空安全的,client.get 也用 try/catch 兜住了同步抛错。

既然现在能用了,有个安全考量:marketplace.json 既驱动安装前的 Discover 列表,也提供流向 cloneFromGit / downloadFromGitHubRelease 的每个插件 installSource。在明文 http:// 上,链路中间人可以改写响应体,把安装重定向到攻击者仓库。(http.get 默认不跟随重定向,所以是元数据 MITM 而非重定向 SSRF;且有边界——源由用户手动添加,安装仍需确认。)建议加个限制:把 http:// 限定在 loopback / 内网地址,或在添加源时给一行"不安全的 http 源"警告,避免网络 MITM 静默劫持安装路由。更像维护者的取舍——不阻塞。

— claude-opus-4-8 via Claude Code /qreview

['sso://team/repo', 'git'],
['https://example.com/marketplace.json', 'http'],
['HTTPS://example.com/marketplace.json', 'http'],
['HTTP://example.com/marketplace.json', 'http'],

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 table now covers uppercase HTTPS:// / HTTP:// well, but parseExtensionSourceType also made the git@ / sso:// branch case-insensitive (lower.startsWith('git@') || lower.startsWith('sso://')) and that half is only tested in lowercase (rows 33-34). Reverting just that branch to trimmed.startsWith(...) leaves every test green — so the uppercase git@/sso:// classification is unguarded. Two rows close it:

Suggested change
['HTTP://example.com/marketplace.json', 'http'],
['HTTP://example.com/marketplace.json', 'http'],
['GIT@github.com:owner/repo.git', 'git'],
['SSO://team/repo', 'git'],
中文

这个表对大写 HTTPS:// / HTTP:// 覆盖得很好,但 parseExtensionSourceType 同样把 git@ / sso:// 分支改成了大小写不敏感(lower.startsWith('git@') || lower.startsWith('sso://')),而这一半只测了小写(第 33-34 行)。把该分支退回 trimmed.startsWith(...) 时所有测试仍全绿——说明大写 git@/sso:// 的分类没有被测试守住。补两行即可。

— claude-opus-4-8 via Claude Code /qreview

@tt-a1i
tt-a1i marked this pull request as ready for review June 20, 2026 11:57
@wenshao

wenshao commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, reviewer test plan included.

On direction: this is a straightforward RFC 3986 compliance fix — URL schemes are defined as case-insensitive, and the marketplace source paths were still doing case-sensitive prefix checks. It's the direct follow-up to #5429 (install sources) covering the remaining marketplace code paths. Clearly aligned with the project's extension system, no safety or public-contract concerns.

On approach: the scope is tight and focused. Three production files, each applying the same minimal change — lowercase the prefix for comparison, preserve the original string for parsing/fetching. The clientForUrl extraction from npm.ts into a shared http-client.ts is the right DRY move since marketplace.ts now also needs protocol-aware client selection. The fetchUrl refactor (wrapping client.get in try/catch, let req instead of const req) is a necessary consequence — http:// marketplace URLs can now actually reach fetchUrl, and the old code had a latent TDZ crash when https.get threw synchronously on an http:// URL. No scope creep, no drive-by refactors.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必需章节齐全,双语,包含 reviewer test plan。

方向:这是一个直截了当的 RFC 3986 合规修复 — URL scheme 按规范是大小写不敏感的,而 marketplace source 路径仍在做大小写敏感的前缀检查。它是 #5429(install source)的直接后续,覆盖了 marketplace 代码中剩余的路径。与项目的 extension 系统完全对齐,无安全或公共接口方面的问题。

方案:范围紧凑且聚焦。三个生产文件,每个都做同样的最小改动 — 对前缀做 lowercase 比较,保留原始字符串用于解析和请求。clientForUrlnpm.ts 提取到共享的 http-client.ts 是正确的 DRY 做法,因为 marketplace.ts 现在也需要基于 protocol 选择 client。fetchUrl 的重构(在 client.get 外包 try/catch,let req 替代 const req)是必要的 — http:// marketplace URL 现在能真正到达 fetchUrl,而旧代码在 https.gethttp:// URL 同步抛错时存在潜在的 TDZ 崩溃。无范围蔓延,无顺手重构。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (before reading the diff): the fix is mechanical — lowercase the input (or just the scheme prefix) before comparing against http://, https://, git@, sso:// in parseExtensionSourceType, loadMarketplaceConfigFromSource, and resolvePluginSource. Add test cases for uppercase variants. Extract the existing clientForUrl from npm.ts into a shared module since marketplace.ts now also needs protocol-aware client selection. The fetchUrl function needs to handle http:// URLs correctly and shouldn't crash when client.get throws synchronously.

Comparison with the diff: the PR's approach matches my independent proposal exactly. It lowercases only the prefix for comparison while preserving the original string — the correct pattern since new URL() already handles case normalization internally. The clientForUrl extraction is clean (identical logic, generalized error message). The fetchUrl refactor correctly wraps clientForUrl and client.get in try/catch, uses let req with optional chaining, and preserves the existing timeout/size guards. The git@ regex gets the /i flag for case-insensitive matching. No correctness bugs, no security issues, no AGENTS.md violations found.

One observation: resolvePluginSource in claude-converter.ts already uses lowerSource for scheme comparison (likely from #5429), so no production change was needed there — the PR correctly adds only a test case for that path. The http-client.ts module is a clean extraction with the only change being a generalized error message ("Unsupported URL protocol" vs "Unsupported npm registry URL protocol").

Verification

Unit Tests (PR branch)

 ✓ src/extension/marketplace.test.ts    (30 tests)
 ✓ src/extension/sourceRegistry.test.ts (22 tests)
 ✓ src/extension/claude-converter.test.ts (42 tests)

 Test Files  3 passed (3)
      Tests  94 passed (94)

Mutation Test

Reverted only the 3 production files (sourceRegistry.ts, marketplace.ts, npm.ts) to main, kept the PR's tests:

 Test Files  2 failed | 1 passed (3)
      Tests  6 failed | 88 passed (94)

The 6 failures confirm the new tests genuinely guard the fix — sourceRegistry.test.ts loses 3 uppercase cases, marketplace.test.ts loses 3 uppercase loader tests.

Typecheck & Lint

tsc --noEmit -p packages/core/tsconfig.json  ✅ (clean)
eslint (4 production files)                   ✅ (clean)

Before/After Probe — parseExtensionSourceType

===== BEFORE (main branch) =====
✅  github   https://github.com/owner/repo
❌  local    HTTPS://github.com/owner/repo
❌  local    Https://GitHub.com/owner/repo
❌  local    HTTPS://example.com/marketplace.json
❌  local    HTTP://example.com/marketplace.json
✅  git      git@github.com:owner/repo.git
❌  local    GIT@github.com:owner/repo.git
✅  git      sso://team/repo
❌  local    SSO://team/repo
✅  github   anthropics/skills
✅  local    ./local/marketplace

===== AFTER (PR #5435) =====
✅  github   https://github.com/owner/repo
✅  github   HTTPS://github.com/owner/repo
✅  github   Https://GitHub.com/owner/repo
✅  http     HTTPS://example.com/marketplace.json
✅  http     HTTP://example.com/marketplace.json
✅  git      git@github.com:owner/repo.git
✅  git      GIT@github.com:owner/repo.git
✅  git      sso://team/repo
✅  git      SSO://team/repo
✅  github   anthropics/skills
✅  local    ./local/marketplace

Before: 5/11 misclassified (all uppercase scheme variants fall through to local). After: all 11 correct. Lowercase inputs unchanged — zero regression surface.

中文说明

代码审查

独立方案(读 diff 前):修复是机械性的 — 在 parseExtensionSourceTypeloadMarketplaceConfigFromSourceresolvePluginSource 中,对输入(或仅 scheme 前缀)做 lowercase 后再与 http://https://git@sso:// 比较。添加大写变体的测试用例。将 npm.ts 中已有的 clientForUrl 提取到共享模块,因为 marketplace.ts 现在也需要基于 protocol 选择 client。fetchUrl 需要正确处理 http:// URL 并在 client.get 同步抛错时不崩溃。

与 diff 对比: PR 的方案与我的独立提案完全一致。只对前缀做 lowercase 比较,保留原始字符串 — 这是正确的模式,因为 new URL() 内部已经处理大小写规范化。clientForUrl 的提取干净(逻辑相同,泛化了错误消息)。fetchUrl 的重构正确地在 clientForUrlclient.get 外包 try/catch,使用 let req 加可选链,并保留了现有的超时和响应体大小保护。git@ 正则加上了 /i 标志做大小写不敏感匹配。未发现正确性 bug、安全问题或 AGENTS.md 违规。

一个观察:claude-converter.ts 中的 resolvePluginSource 已经使用 lowerSource 做 scheme 比较(可能来自 #5429),因此不需要生产代码改动 — PR 正确地为该路径只添加了一个测试用例。

验证

单元测试: 94 个全部通过(3 个文件)。

变异测试: 仅回退 3 个生产文件到 main → 6 failed | 88 passed (94)。新测试确实能守护修复。

Typecheck & Lint: 全部通过。

Before/After 探测: Before 5/11 误分类,After 全部 11 个正确。小写输入无变化 — 零回归面。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Stepping back: this is a clean, well-scoped compliance fix. URL schemes are case-insensitive per RFC 3986, the marketplace source paths weren't honoring that, and the fix applies the minimum change needed — lowercase prefix comparison in three functions, protocol-aware client selection via a shared clientForUrl, and seven regression tests that genuinely guard the fix (confirmed by mutation: 6 failures on reverted code).

The implementation matches my independent proposal. It preserves the original source string for parsing/fetching (correct — new URL() handles normalization internally), extracts clientForUrl into a shared module instead of duplicating it, and the fetchUrl refactor closes a latent TDZ crash as a bonus. No over-engineering, no scope creep, no drive-by refactors. The diff is exactly the size it needs to be.

The before/after probe and mutation test both confirm the fix works and the tests are non-vacuous. All existing lowercase inputs behave identically — zero regression surface. Typecheck and lint are clean.

Note on the maintainer's DRY concern about folding in #5437: this PR already does the DRY work by extracting clientForUrl into http-client.ts. Whether #5437 should be merged into this PR is a process decision for the maintainer — the code in this PR stands on its own.

LGTM. Approving. ✅

中文说明

退一步看:这是一个干净、范围合理的合规修复。URL scheme 按 RFC 3986 是大小写不敏感的,marketplace source 路径没有遵守这一点,修复应用了所需的最小改动 — 三个函数中的小写前缀比较、通过共享 clientForUrl 实现基于 protocol 的 client 选择、以及七个确实能守护修复的回归测试(通过变异测试确认:回退代码后 6 个失败)。

实现与我的独立提案一致。保留原始 source 字符串用于解析和请求(正确 — new URL() 内部处理规范化),将 clientForUrl 提取到共享模块而不是重复实现,fetchUrl 的重构还顺带堵住了一个潜在的 TDZ 崩溃。没有过度工程,没有范围蔓延,没有顺手重构。diff 的大小恰好是它需要的。

Before/After 探测和变异测试都确认了修复有效且测试非空过。所有现有的小写输入行为不变 — 零回归面。Typecheck 和 lint 全部通过。

关于维护者提出的将 #5437 合并的 DRY 建议:本 PR 已经通过将 clientForUrl 提取到 http-client.ts 完成了 DRY 工作。#5437 是否应该合并到这个 PR 是维护者的流程决定 — 本 PR 的代码本身是完整可用的。

可以合并。✅

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

@wenshao

wenshao commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

📌 Same maintainer note — full version on #5509.

This is the case-insensitive URL-scheme series. #5437 is the same idea in npm.ts; ideally fold it (and the other scheme PRs) into this one and share a single clientForUrl-style helper instead of per-file copies (see also the hold note on #5437). The code itself looks fine — this is about grouping + DRY, not correctness.

中文说明

📌 同一条维护者说明 —— 完整版见 #5509

这是URL scheme 大小写不敏感系列。#5437 是同一个点子在 npm.ts 里的版本;最好把它(以及其他 scheme 相关 PR)并到这一个,并共用一个 clientForUrl 之类的 helper,而不是每个文件各写一份(另见 #5437 上的 hold 备注)。代码本身没问题 —— 这条是关于归并 + DRY,不涉及正确性。

@tt-a1i
tt-a1i force-pushed the fix/extension-marketplace-uppercase-source branch from dc9e938 to 60a974b Compare June 21, 2026 05:06
@tt-a1i

tt-a1i commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

@qwen-code /triage

1 similar comment
@wenshao

wenshao commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@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 commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification report — uppercase marketplace source schemes (#5435)

Verified locally in tmux: reproduced the PR's test plan, ran a revert-proof against base, then drove the real exported helpers — including a real localhost HTTP server fetched over an uppercase HTTP:// scheme (the E2E the PR marks out of scope). Recommendation: safe to merge. Scheme comparison is correctly case-insensitive while the original source string is preserved for fetching/parsing/metadata, and the shared clientForUrl() consolidation is behavior-preserving.

Environment

Tested tree clean merge of origin/main (106074188) + PR head (60a974b) → 19523a2 (no conflicts)
Toolchain Linux x86_64 · Node v22.22.2 · Vitest 3.2.4

1. The PR's stated test plan — reproduced

Step Result
vitest run npm + marketplace + sourceRegistry + claude-converter 120 / 120 pass (incl. all new uppercase cases)
typecheck --workspace=packages/core (tsc --noEmit) 0 errors
lint --workspace=packages/core ✅ clean
git diff --check ✅ clean
build --workspace=packages/core ⚠️ fails on a pre-existing, unrelated issue — see note¹

¹ The build fails on src/providers/__tests__/presets/custom-provider.test.ts: Property 'mergeModelsByIdentity' does not exist on type 'ProviderConfig' — a provider test the PR never touches (PR is src/extension/* only). It's an artifact of my isolated worktree: that test self-imports @qwen-code/qwen-code-core, which through the symlinked node_modules resolves to the host checkout that predates #5404 (the host has 0 mergeModelsByIdentity). I confirmed a base build of origin/main (no PR) in the same setup fails with the identical error, so it is environmental, not caused by this PR. The PR's real compile gate — tsc --noEmit — passes with 0 errors, and all 120 extension tests run.

2. Revert-proof — the new tests are coupled to the fix

Reverting the source files to base (removing http-client.ts, restoring the case-sensitive checks) makes 6 of the new tests fail, exactly the uppercase-scheme cases:

  • sourceRegistry: HTTPS://github.com…, HTTPS://example.com…, HTTP://example.com… (3 fail)
  • marketplace: uppercase HTTPS GitHub source, uppercase HTTPS direct-JSON, uppercase HTTP direct-JSON (3 fail)

All pass on the merged code.


3. Adversarial + real-server verification (19/19)

Drove the real exported helpers; for the loader I stood up a real http server on localhost and fetched it via an uppercase scheme.

clientForUrl()http/https/HTTP/HTTPS/HtTpS all map to the correct client; ftp://, file:// throw. ✓

parseExtensionSourceType() — NEW vs the OLD case-sensitive startsWith:

source OLD NEW (PR)
HTTPS://github.com/owner/repo misclassified (→ shorthand/local) ⚠️ github
HTTP://example.com/m.json misclassified ⚠️ http
HtTpS://example.com/m.json misclassified ⚠️ http
GIT@github.com:owner/repo misclassified ⚠️ git
SSO://corp/repo misclassified ⚠️ git
lowercase https://…, git@… correct correct ✅ (no regression)

Real marketplace load over the network:

  • loadMarketplaceConfigFromSource('HTTP://127.0.0.1:<port>/marketplace.json')loads the config end-to-end (the old case-sensitive classifier would have skipped HTTP:// as non-remote → null). ✅
  • lowercase http://… still loads (regression). ✅
  • case preserved: requesting HTTP://…/Marketplace.JSON, the server received the exact-case path /Marketplace.JSON, not a lowercased one — confirming the fix lowercases only for the scheme comparison and leaves the rest of the URL untouched for fetching. ✅

Notes (non-blocking)

  • The shared helper changes npm's error text from Unsupported npm registry URL protocol to Unsupported URL protocol. No test or caller depends on that string; harmless.
  • The fetchUrl() refactor is also a small robustness win: clientForUrl() and client.get() are now wrapped so a malformed/unsupported URL resolves to null instead of throwing synchronously.

Verdict

Approve. The fix correctly treats URL schemes as case-insensitive (compare lowercased, fetch/parse/store the original), consolidates HTTP-client selection into one tested helper, preserves all lowercase behavior, and is confirmed end-to-end against a real server. The only red — npm run build — is a pre-existing environmental artifact reproduced identically on base.

🇨🇳 中文版验证报告(点击展开)

✅ 本地验证报告 —— marketplace source 大写 scheme(#5435

tmux 中本地验证:复现 PR 测试计划,对 base 做 revert-proof,然后驱动真实导出的 helper —— 包括用真实 localhost HTTP server 通过大写 HTTP:// scheme 拉取(PR 标为 out of scope 的 E2E)。结论:可以合并。 scheme 比较已正确做到大小写不敏感,同时保留用户输入的原始 source 字符串用于 fetch/解析/metadata;共享的 clientForUrl() 整合是行为保持的。

环境

被测代码树 origin/main106074188)与 PR head(60a974b)的干净合并 → 19523a2(无冲突)
工具链 Linux x86_64 · Node v22.22.2 · Vitest 3.2.4

1. PR 自述测试计划 —— 复现

步骤 结果
vitest run npm + marketplace + sourceRegistry + claude-converter 120 / 120 通过(含全部新增大写用例)
typecheck --workspace=packages/coretsc --noEmit 0 报错
lint --workspace=packages/core ✅ 干净
git diff --check ✅ 干净
build --workspace=packages/core ⚠️ 失败于一个既有的、无关的问题 —— 见注¹

¹ build 失败在 src/providers/__tests__/presets/custom-provider.test.ts: Property 'mergeModelsByIdentity' does not exist on type 'ProviderConfig' —— 这是一个provider 测试,本 PR 根本没碰(PR 只改 src/extension/*)。这是我隔离 worktree 的假象:该测试 self-import @qwen-code/qwen-code-core,经由 symlink 的 node_modules 解析到了宿主 checkout,而宿主早于 #5404(宿主的 mergeModelsByIdentity 计数为 0)。我确认在相同环境下对 origin/main(无 PR)做 base build 会报完全相同的错,故属环境问题,与本 PR 无关。PR 真正的编译门禁 tsc --noEmit 0 报错,且 120 个 extension 测试全部运行。

2. Revert-proof —— 新测试与修复强绑定

把源文件回退到 base(删除 http-client.ts、恢复大小写敏感判断)后,6 个新测试失败,恰好是大写 scheme 用例:

  • sourceRegistryHTTPS://github.com…HTTPS://example.com…HTTP://example.com…(3 个失败)
  • marketplace:大写 HTTPS GitHub source、大写 HTTPS direct-JSON、大写 HTTP direct-JSON(3 个失败)

在合并后的代码上全部通过。

3. 对抗性 + 真实 server 验证(19/19)

驱动真实导出的 helper;loader 部分启动了真实的 localhost http server 并用大写 scheme 拉取。

clientForUrl() —— http/https/HTTP/HTTPS/HtTpS 都映射到正确的 client;ftp://file:// 抛错。✓

parseExtensionSourceType() —— NEW vs 旧的大小写敏感 startsWith

source 新(PR)
HTTPS://github.com/owner/repo 误判(→ shorthand/local)⚠️ github
HTTP://example.com/m.json 误判 ⚠️ http
HtTpS://example.com/m.json 误判 ⚠️ http
GIT@github.com:owner/repo 误判 ⚠️ git
SSO://corp/repo 误判 ⚠️ git
小写 https://…git@… 正确 正确 ✅(无回归)

真实网络 marketplace 加载:

  • loadMarketplaceConfigFromSource('HTTP://127.0.0.1:<port>/marketplace.json')端到端成功加载 config(旧的大小写敏感分类会把 HTTP:// 当作非远程而跳过 → null)。✅
  • 小写 http://… 仍可加载(回归)。✅
  • 大小写保留: 请求 HTTP://…/Marketplace.JSON 时,server 收到的是原样大小写的路径 /Marketplace.JSON,没有被转小写 —— 证明修复只对 scheme 比较做 lowercase,URL 其余部分原样用于 fetch。✅

提示(非阻塞)

  • 共享 helper 把 npm 的错误文案从 Unsupported npm registry URL protocol 改为 Unsupported URL protocol。没有测试或调用方依赖该字符串;无害。
  • fetchUrl() 重构还顺带增强了健壮性:clientForUrl()client.get() 现在被包裹,畸形/不支持的 URL 会 resolve 成 null 而不是同步抛错。

结论

赞成合并。 修复正确地把 URL scheme 当作大小写不敏感(比较时转小写,fetch/解析/存储用原始串),把 HTTP client 选择整合进一个有测试的 helper,保留了所有小写行为,并已用真实 server 端到端确认。唯一的红 —— npm run build —— 是既有环境假象,在 base 上完全复现。


Verification method: merged worktree; PR test plan reproduced (120/120 + typecheck + lint); revert-proof (6 uppercase tests fail on base); adversarial battery of clientForUrl + parseExtensionSourceType; real localhost HTTP server fetched over an uppercase HTTP:// scheme with exact-case path preservation; npm run build red proven pre-existing via an identical base build.

@wenshao
wenshao merged commit f1a5b70 into QwenLM:main Jun 21, 2026
31 of 32 checks passed
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.

Extension marketplace sources misclassify uppercase HTTP(S) schemes

3 participants