fix(extensions): fetch http marketplaces with http client - #5452
Conversation
wenshao
left a comment
There was a problem hiding this comment.
No issues found. The protocol-based client selection in fetchUrl is correct — new URL(url).protocol normalizes case, the two internal GitHub callers stay on https, and a malformed URL throws ERR_INVALID_URL exactly as the previous https.get(url) did (no new failure path). The added test genuinely covers the http:// route (it fails against the pre-fix https.get implementation). Downgraded from Approve to Comment: CI still running. — claude-opus-4-8[1m] via Qwen Code /qreview
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @tt-a1i — thanks for the fix! The change itself looks focused and reasonable, but the PR body doesn't follow our pull request template. Several required sections are missing:
- What this PR does / Why it's needed — there's a "Summary" but no separate motivation section
- Reviewer Test Plan (How to verify, Evidence Before & After, Tested on) — "Test Plan" lists commands but doesn't describe what a reviewer should confirm or show before/after
- Risk & Scope — not present
- Linked Issues — not present (Fixes #5451 is in the Summary, but the section is expected for traceability)
- 中文说明 — not present
Could you restructure the PR body to match the template? It helps reviewers (and our automated triage) evaluate the change consistently. Happy to re-run triage once updated.
中文说明
@tt-a1i 你好——感谢这个修复!改动本身很聚焦且合理,但 PR 正文没有遵循我们的 PR 模板,缺少几个必填章节:
- What this PR does / Why it's needed — 有 "Summary" 但没有单独的动机说明
- Reviewer Test Plan(验证方法、改动前后证据、测试平台)— "Test Plan" 列出了命令,但没有描述审查者应确认什么或展示前后对比
- Risk & Scope — 缺失
- Linked Issues — 缺失(Fixes #5451 写在 Summary 里,但需要单独章节以便追踪)
- 中文说明 — 缺失
请按模板重新组织 PR 正文,更新后我们会重新跑 triage。谢谢!
— Qwen Code · qwen3.7-max
✅ Local end-to-end verification (real HTTP/HTTPS servers, tmux)Verified the fix against a real local HTTP server and a real HTTPS server (self-signed cert, trusted via
Root cause confirmedOn BASE, feeding an A/B result
Result deterministic across repeated runs (ports randomized each run), process exit 0. Unit test + static checks
Notes
Verdict: LGTM — the fix works end-to-end with a real server and introduces no HTTPS regression. 🇨🇳 中文版本(点击展开)✅ 本地端到端验证(真实 HTTP/HTTPS 服务器,tmux)使用真实的本地 HTTP 服务器和真实的 HTTPS 服务器(自签名证书,通过
根因确认在 BASE 上,把 A/B 结果
多次重复运行结果稳定(每次随机端口),进程退出码为 0。 单元测试 + 静态检查
补充说明
结论:LGTM —— 修复在真实服务器下端到端有效,且不会造成 HTTPS 回归。 |
|
updated the PR body to match the template. |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual description included. On direction: this fixes a genuine bug. On approach: the scope is minimal and exactly right. Three lines of production code: import Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有必填章节齐全,含双语描述。 方向:修复了一个真实 bug。 方案:范围极小且恰到好处。三行生产代码:导入 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewBefore reading the diff, my independent proposal was identical: import The implementation is clean and correct:
No correctness bugs, no security issues, no convention violations found. Real-Scenario Test (tmux)Started a real Before (base/main —
|
|
Stepping back: this is a textbook minimal bugfix. The problem is real and well-documented — My independent proposal was identical to what the PR does, which is a good sign — there's no simpler path being missed. Every line in the diff earns its place: the import, the protocol check, the client swap, the test, the docstring update. Nothing extra. The tmux test confirms the fix end-to-end: base throws the exact error the issue describes, PR loads the marketplace JSON successfully. Unit tests pass (26/26), ESLint clean. No regressions in the HTTPS path. The PR is cross-repository from a first-time contributor. The PR body follows the template well, the motivation is clear, and the linked issue (#5451) provides the bug report. This is ready to ship. Approving. ✅ 中文说明总结:这是一个教科书式的最小 bug 修复。问题真实且有据可查—— 我的独立方案与 PR 完全一致——没有更简单的路径被遗漏。diff 中的每一行都有存在的必要:导入、协议检查、client 切换、测试、文档更新。没有多余内容。 tmux 测试端到端确认了修复:base 抛出与 issue 描述完全相同的错误,PR 成功加载 marketplace JSON。单元测试全部通过(26/26),ESLint 干净。HTTPS 路径无回归。 PR 来自首次贡献者的跨仓库提交。PR 内容很好地遵循了模板,动机清晰,关联的 issue (#5451) 提供了 bug 报告。可以合并。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Uses the Node HTTP client for
http://extension marketplace sources and the HTTPS client forhttps://sources.The existing timeout, body-size limit, JSON parsing, and null-on-error behavior stay the same.
Why it's needed
loadMarketplaceConfigFromSourceaccepts directhttp://.../marketplace.jsonURLs, but the shared fetch helper always calledhttps.get. Node rejects that client/protocol mismatch before making the request, so valid local or internal HTTP marketplace sources could not load.Reviewer Test Plan
How to verify
Review
fetchUrland the marketplace tests. Thehttp://case should go throughhttp.get; HTTPS and GitHub-backed sources should continue to usehttps.get.Run:
Evidence (Before & After)
Before:
http://.../marketplace.jsonfailed withProtocol "http:" not supported. Expected "https:".After: HTTP marketplace URLs are fetched with the HTTP client, while HTTPS marketplace URLs keep the previous HTTPS path.
Tested on
Environment (optional)
Local Node/npm workspace tests.
Risk & Scope
Linked Issues
Fixes #5451
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
让
http://扩展 marketplace 源使用 Node 的 HTTP client,让https://源继续使用 HTTPS client。原有的超时、响应体大小限制、JSON 解析和出错返回
null的行为不变。为什么需要
loadMarketplaceConfigFromSource支持直接传入http://.../marketplace.json,但共享 fetch helper 之前总是调用https.get。Node 会在发请求前拒绝这种 protocol/client 不匹配,因此本地或内网 HTTP marketplace 源无法加载。Reviewer Test Plan
How to verify
检查
fetchUrl和 marketplace 测试:http://用例应走http.get,HTTPS 和 GitHub 源应继续走https.get。Evidence (Before & After)
修复前:
http://.../marketplace.json会报Protocol "http:" not supported. Expected "https:"。修复后:HTTP marketplace URL 使用 HTTP client;HTTPS URL 保持原来的 HTTPS 路径。
Tested on
本地跑过相关 Node/npm 测试;Windows 和 Linux 由 CI 覆盖。
Risk & Scope
改动只影响直接 marketplace URL 的协议选择。不新增 HTTP/HTTPS 以外的协议支持,也没有破坏性变更。