fix(core): support public GitHub extensions with older Git - #9680
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, with a concrete reproduction. #8993 documents Ubuntu 22.04 (Git 2.34.1 from the standard apt repos) failing public extension installs before cloning, with the terse Direction: aligned. The issue lists three possible resolutions, and this PR deliberately picks the "document the prerequisite and make the error actionable" one rather than adding a fail-open path that would weaken the #6825 DNS-pinning guarantee. Keeping the security boundary intact while telling users what to do about it is the right tradeoff. No direct reference in the claude-code CHANGELOG, but this is our own extension-install prerequisite, so that signal doesn't apply here. Size: core path touched ( Approach: the scope is exactly right — one error site gets the detected version and actionable guidance, one docs paragraph states the prerequisite before users attempt an install, and two regression tests cover both sides of the 2.37 boundary. Nothing to cut, no unrelated changes. Risk: no elevated risk signals — none of the revert-correlated paths are touched. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,且有具体复现。#8993 记录了 Ubuntu 22.04(标准 apt 仓库的 Git 2.34.1)在 clone 前安装公共扩展失败的情况,错误信息只有简短的 方向:对齐。issue 列出了三种可能的解决方案,本 PR 明确选择了"文档化前置要求并让错误可操作"这一种,而不是增加会削弱 #6825 DNS 固定安全保证的 fail-open 路径。在保持安全边界完整的前提下告诉用户该怎么办,这是正确的取舍。claude-code CHANGELOG 中无直接参考,但这是我们自己的扩展安装前置要求,该信号在此不适用。 规模:触及核心路径( 方案:范围恰到好处——一处错误点加上了检测到的版本和可操作的指引,一段文档在用户尝试安装前说明前置要求,两个回归测试覆盖 2.37 边界的两侧。没有可砍的内容,没有无关改动。 风险:无升级风险信号——未触及任何与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: from just the title and motivation, I would (a) enrich the single Reviewing the diff against that baseline:
TestingUnattended CI run — I do not build or execute PR code; the evidence below is the PR's own CI on The Linux unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The behavioral claim — actionable error before clone, pinning intact at the 2.37 boundary — is fully pinned by the two new unit tests (the 2.34.1 test fails against 中文说明代码审查:先独立给出方案——仅凭标题和动机,我会 (a) 在已经同时守护安装路径( 对照 diff 审查:版本比较逻辑未动,只改了抛出的错误信息;检测版本拼接 测试说明:无人值守 CI 运行——不构建、不执行 PR 代码;以上证据为通过 API 一次性获取的该 PR 自身在 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage: a real reproduced problem, exactly the resolution the issue documented, a minimal well-tested diff, nothing extraneous. Stepping back: this is what a good small fix looks like. #8993 is a real, reproduced pain point — Ubuntu 22.04 LTS users cannot install public extensions and get an error that doesn't tell them what to do — and the issue itself lists "document the prerequisite and make the error actionable" among its acceptable resolutions. This PR implements precisely that, explicitly refusing the fail-open path that would have weakened the #6825 DNS-pinning guarantee. The diff is 7 production lines plus a docs paragraph and two regression tests; every change earns its place, and my independent proposal for this problem matches what landed. The 2.34.1 test fails against Only remaining item: the Linux unit suite is still in flight on this commit. Approval is deferred until CI lands green on 中文说明回顾总结:这是一个标准的小型优秀修复。#8993 是真实且有复现的痛点——Ubuntu 22.04 LTS 用户无法安装公共扩展,且错误信息没有告诉用户该怎么办——issue 本身就列出了"文档化前置要求并让错误可操作"作为可接受的解决方案之一。本 PR 精确实现了该方案,并明确拒绝了会削弱 #6825 DNS 固定保证的 fail-open 路径。diff 为 7 行生产代码、一段文档和两个回归测试,每一处改动都有其必要,我的独立方案与最终实现一致。2.34.1 测试在 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
doudouOUC
left a comment
There was a problem hiding this comment.
Code Review for PR #9680 — fix(core): clarify Git requirement for public extensions
Overview
3 files, +57/−1. The PR addresses issue #8993 (Ubuntu 22.04 ships Git 2.34.1, which blocks public Git extension installs before cloning with a terse error) by making the prerequisite actionable without weakening the DNS-pinning security guarantee from #6825.
What I verified
Code (worktree at 2bc440b29803):
assertPinnedGitSupported()ingithub.ts: version comparison logic is untouched — the only change is the thrown error message, which now includes the detected Git version and actionable guidance. The detected version string correctly filters outundefinedpatch(via.filter(c => c !== undefined).join('.')).- The error still fires before
resolveNetworkTargetand before any clone or network activity — the too-old-Git test doesn't even need a DNS mock. - On the update-check path (
checkForExtensionUpdate), the error is caught, logged, and returnsExtensionUpdateState.ERROR— unchanged behavior, improved message. - No orphaned consumers: the old message string
'Public extension Git installs require Git 2.37 or newer.'appears in exactly one place in the tree (the site being changed). No other code or test asserts on it.
Tests (0 regressions, 2 new passing):
explains how to install public extensions when Git is too old for DNS pinning— mock Git 2.34.1, asserts the full new message, assertscloneis never called.accepts Git 2.37 while preserving public network pinning— mock Git 2.37.0 with DNS lookup, asserts the full pinned config (http.curloptResolve,http.followRedirects=false,http.proxy=,protocol.allow=never,protocol.https.allow=always).- Both tests pass on the worktree, and the first test fails against
origin/main(proving the suite genuinely pins the change).
TypeScript: tsc --noEmit passes on packages/core.
Build: packages/core builds successfully. The packages/audio-capture build failure is a pre-existing Windows infra issue (no Python for node-gyp), unrelated to the PR.
Docs: The paragraph under "From Git Repository" in introduction.md is well placed — it's the first text users see before the install command. It explains the requirement, the rationale (http.curloptResolve), and offers alternatives (upgrade Git or install from local path/archive).
Findings
No Critical issues. The diff is minimal, well-tested, and correctly scoped. The PR implements exactly the resolution documented in the issue ("document the prerequisite and make the error actionable") and explicitly avoids the fail-open compatibility path that would have weakened the #6825 DNS-pinning guarantee.
Suggestions
None. The PR is self-contained, the tests are well-structured, and the docs placement is correct. This is a model small fix.
Review performed on 2bc440b29803aa1e3a883c22f90918fb26bdf7fa
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| throw new Error( | ||
| `Public extension Git installs require Git 2.37 or newer for secure DNS pinning; found Git ${detectedVersion}. Upgrade Git, or install the extension from a local path or archive instead.`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The improved message never reaches the second of its two consumers. On the update-check path, checkForExtensionUpdate calls assertPinnedGitSupported() (github.ts:482), but its catch-all (github.ts:602-609) logs the error to debugLogger.error only and returns a bare ExtensionUpdateState.ERROR, so the update-check coverage this PR claims — and the new docs sentence "installs and update checks require Git 2.37 or newer" — is never surfaced at runtime. A user on Git < 2.37 who triggers an update check for a networkPolicy: 'public' git extension sees only Failed to check "<name>" for updates. in the TUI (the headless extensions update command even prints "already up to date" for the ERROR state) — no Git version, cause, or alternative. A probe driving the real checkForExtensionUpdate with a Git 2.34.1 extension confirmed the error does not propagate (F1-THREW: false F1-RESULT: "error"). One possible shape — mirror the ExtensionCredentialUnavailableError treatment:
class ExtensionGitTooOldError extends Error {}
// assertPinnedGitSupported: throw new ExtensionGitTooOldError(...)
// checkForExtensionUpdate's catch:
if (
error instanceof ExtensionCredentialUnavailableError ||
error instanceof ExtensionGitTooOldError
) {
throw error;
}
// note: checkAllExtensionsForUpdates has its own .catch(() => ERROR)
// (extensionManager.ts:3015-3019) that would need the same rethrow中文说明
改进后的消息没有到达它的第二个使用者。在更新检查路径上,checkForExtensionUpdate 调用了 assertPinnedGitSupported()(github.ts:482),但其 catch-all(github.ts:602-609)只把错误记录到 debugLogger.error 并返回裸的 ExtensionUpdateState.ERROR,因此 PR 声称的更新检查覆盖——以及新增文档句子"公共 Git 仓库的安装和更新检查需要 Git 2.37 或更高版本"——在运行时并不会真正呈现。使用 Git < 2.37 的用户触发 networkPolicy: 'public' git 扩展的更新检查时,TUI 只会显示 Failed to check "<name>" for updates.(headless 的 extensions update 命令甚至会对 ERROR 状态打印 "already up to date")——没有 Git 版本、原因或替代方案。用 Git 2.34.1 扩展驱动真实 checkForExtensionUpdate 的探针确认错误没有向上传播(F1-THREW: false F1-RESULT: "error")。建议参照 ExtensionCredentialUnavailableError 的处理方式:从 assertPinnedGitSupported 抛出专用错误类,并将其加入 checkForExtensionUpdate catch 中的重新抛出集合(注意 checkAllExtensionsForUpdates 有自己的 .catch(() => ERROR),extensionManager.ts:3015-3019,也需要同样的重新抛出)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| it('explains how to install public extensions when Git is too old for DNS pinning', async () => { | ||
| mockGit.version.mockResolvedValue({ major: 2, minor: 34, patch: 1 }); |
There was a problem hiding this comment.
[Suggestion] Neither new test exercises the undefined-component branch of .filter((component) => component !== undefined) — both supply major, minor, and patch. A mutation probe confirmed that deleting the filter keeps both new tests green (with inputs {2,34,1} and {2,37,0} the join output is identical either way). The discriminating input is a version result without a patch component, which the filter itself anticipates: without the filter the message degrades to found Git 2.34.. (double dot — join renders undefined as the empty string, observed in the probe), and no test fails. Add a case like:
it('renders the detected version without a patch component', async () => {
mockGit.version.mockResolvedValue({ major: 2, minor: 34 });
await expect(
cloneFromGit(
{
source: 'https://github.com/owner/repo.git',
type: 'git',
networkPolicy: 'public',
},
'/dest',
),
).rejects.toThrow('found Git 2.34. Upgrade Git');
expect(mockGit.clone).not.toHaveBeenCalled();
});The assertion found Git 2.34. Upgrade Git fails against the double-dot output, so it pins the filter's behavior.
中文说明
两个新测试都没有覆盖 .filter((component) => component !== undefined) 的 undefined 分量分支——它们都提供了 major、minor 和 patch。变异探针确认:删除该 filter 后两个新测试仍然通过(对于输入 {2,34,1} 和 {2,37,0},join 的输出在两种情况下完全相同)。判定性输入是不含 patch 分量的版本结果——这正是 filter 本身所预期的场景:若没有 filter,消息会退化为 found Git 2.34..(双点——join 把 undefined 渲染为空字符串,探针已观察到),且没有任何测试失败。建议补充如上代码块所示的用例;断言 found Git 2.34. Upgrade Git 在出现双点输出时会失败,从而钉住 filter 的行为。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const detectedVersion = [version.major, version.minor, version.patch] | ||
| .filter((component) => component !== undefined) | ||
| .join('.'); |
There was a problem hiding this comment.
[Suggestion] When Git is not installed at all, simple-git's version() resolves (does not throw) with {major: 0, minor: 0, patch: 0, installed: false}, and the new code ignores the installed flag — so the message reports found Git 0.0.0. Verified against the installed simple-git 3.36.0 with git removed from PATH and end-to-end through this PR's own cloneFromGit: the user sees "…found Git 0.0.0. Upgrade Git, or install the extension from a local path or archive instead." The diagnostic contradicts reality (nothing was found) and sends them hunting for a phantom 0.0.0 install to upgrade; the rejection itself is correct. Branch on the flag simple-git already provides:
| const detectedVersion = [version.major, version.minor, version.patch] | |
| .filter((component) => component !== undefined) | |
| .join('.'); | |
| if (!version.installed) { | |
| throw new Error( | |
| 'Public extension Git installs require Git 2.37 or newer for secure DNS pinning; no Git installation was found. Install Git 2.37 or newer, or install the extension from a local path or archive instead.', | |
| ); | |
| } | |
| const detectedVersion = [version.major, version.minor, version.patch] | |
| .filter((component) => component !== undefined) | |
| .join('.'); |
(plus a test mocking { major: 0, minor: 0, patch: 0, installed: false })
中文说明
当完全没有安装 Git 时,simple-git 的 version() 会 resolve(而不是抛错)并返回 {major: 0, minor: 0, patch: 0, installed: false},而新代码忽略了 installed 标志——因此消息显示 found Git 0.0.0。已在安装的 simple-git 3.36.0 上(将 git 从 PATH 移除)以及通过本 PR 自身的 cloneFromGit 端到端验证:用户会看到 "…found Git 0.0.0. Upgrade Git, or install the extension from a local path or archive instead."。该诊断与现实矛盾(根本没有找到 Git),会让人去找一个并不存在的 0.0.0 安装来"升级";拒绝行为本身是正确的。建议利用 simple-git 已提供的标志进行分支(如上 suggestion),并补充一个 mock { major: 0, minor: 0, patch: 0, installed: false } 的测试。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| .filter((component) => component !== undefined) | ||
| .join('.'); | ||
| throw new Error( | ||
| `Public extension Git installs require Git 2.37 or newer for secure DNS pinning; found Git ${detectedVersion}. Upgrade Git, or install the extension from a local path or archive instead.`, |
There was a problem hiding this comment.
[Suggestion] The minimum version is hardcoded as the literal "2.37" in this message, duplicating MINIMUM_PINNED_GIT_VERSION (github.ts:47); the new docs sentence and the new test expectation bake the same literal, so four sites must be kept in sync by hand. A future maintainer who bumps the constant (say, to 2.40) and updates only the comparison leaves a user on Git 2.38 with the self-contradictory error "require Git 2.37 or newer ...; found Git 2.38.1" while the docs still say 2.37 suffices — and the test keeps passing on the stale string. Derive the requirement from the constant (verified in a scratch tree: both new tests stay green, rendered string identical):
| `Public extension Git installs require Git 2.37 or newer for secure DNS pinning; found Git ${detectedVersion}. Upgrade Git, or install the extension from a local path or archive instead.`, | |
| `Public extension Git installs require Git ${MINIMUM_PINNED_GIT_VERSION.major}.${MINIMUM_PINNED_GIT_VERSION.minor} or newer for secure DNS pinning; found Git ${detectedVersion}. Upgrade Git, or install the extension from a local path or archive instead.`, |
中文说明
最低版本在这条消息中被硬编码为字面量 "2.37",与 MINIMUM_PINNED_GIT_VERSION(github.ts:47)重复;新增的文档句子和新测试的期望值也写死了同一个字面量,因此四处必须手工保持同步。若未来维护者提高该常量(例如到 2.40)而只更新比较逻辑,使用 Git 2.38 的用户会收到自相矛盾的错误 "require Git 2.37 or newer ...; found Git 2.38.1",而文档仍说 2.37 足够——测试也会因陈旧的字符串而继续通过。建议从常量派生版本要求(已在 scratch tree 验证:两个新测试仍然通过,渲染的字符串完全相同)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not explored to full depth (tool budget reached): "PR #9680 (fix/issue-8993-git-prerequisite) makes the…": none (well under the tool ceiling).; "PR #9680 (fix/issue-8993-git-prerequisite) makes the…": not-installed scenario and mutation candidates were verified statically (locked-library source + flow tracing), not by editing-and-rerunning mutants — the suite….
中文说明
未发现问题。LGTM!✅
未探索到全部深度(达到工具调用预算):"PR #9680 (fix/issue-8993-git-prerequisite) makes the…":none (well under the tool ceiling).;"PR #9680 (fix/issue-8993-git-prerequisite) makes the…":not-installed scenario and mutation candidates were verified statically (locked-library source + flow tracing), not by editing-and-rerunning mutants — the suite…。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
The previous docs/error-only scope is now a true compatibility fix. On Git <2.37, strictly anonymous public GitHub root repositories resolve the requested ref to an immutable SHA and install/update from the pinned codeload archive. Git 2.37+ keeps the existing native pinned-Git path; credentials, non-GitHub/nested sources, submodules, and Git LFS remain fail-closed. Targeted verification: 266 tests, core typecheck, changed-file ESLint/Prettier, and core build. |
|
Correction: this PR was merged at the previous docs/error-only head (2bc440b) before GitHub attached the compatibility commit. The implementation commit f94cb19 is pushed on branch fix/issue-8993-git-prerequisite but is not part of the merged PR. I am stopping here rather than opening an unrequested follow-up PR. |
|
Released in v0.22.0. |
What this PR does
Adds a secure compatibility path for older Git versions when installing or updating a strictly anonymous public GitHub repository root. Qwen Code resolves
ref ?? HEADthrough GitHub's anonymous commits API, downloads the immutable commit archive fromcodeload.github.com, preserves the originaltype: git, source, and ref metadata, and records the resolvedgitCommitfor later update checks and updates.The fallback is selected only by the top-level extension manager. Git 2.37+ continues to use the existing native Git path with
http.curloptResolve; credentialed sources, non-GitHub sources, nested marketplace content,git-subdir, submodules, and Git LFS remain on the native path or fail closed.Why it's needed
Ubuntu 22.04 ships Git 2.34.1, so the original
https://github.com/obra/superpowersdaemon/workspace install scenario failed before any clone despite being an anonymous public repository. The compatibility path restores that supported LTS scenario without removing the Git 2.37 security boundary or introducing a general proxy.Reviewer Test Plan
How to verify
npx vitest run packages/core/src/extension/archive-safety.test.ts packages/core/src/extension/github.test.ts packages/core/src/extension/extensionManager.test.ts.https://github.com/obra/superpowersthrough the archive fallback, never calls clone, and persists the immutable commit SHA.GITHUB_TOKEN.UP_TO_DATEorUPDATE_AVAILABLEby SHA and update installs the new SHA.http.curloptResolve, redirects disabled, proxies cleared, and HTTPS-only protocol configuration.Evidence (Before & After)
Before: the new Git 2.34 integration test failed with
Public extension Git installs require Git 2.37 or newer...before clone.After: 266 focused tests pass, including old-Git install, ref resolution, metadata, update check, update, network/token behavior, archive safety, submodule/LFS rejection, and the unchanged Git 2.37 native path.
Tested on
Environment (optional)
Linux host with Git 2.19.1.6; Git 2.34.1 and 2.37 behavior exercised through the existing
simple-gitboundary. Reused the repository's existingnode_modules.Risk & Scope
git-subdir; repositories that require submodules, Git LFS, or Git history; real Ubuntu 22.04 network execution outside the mocked network boundary.Linked Issues
Fixes #8993
中文说明
这个 PR 做了什么
为旧版 Git 增加严格受限的安全兼容路径,仅支持匿名公共 GitHub 根仓库的安装和更新。Qwen Code 通过匿名 GitHub commits API 将
ref ?? HEAD解析为不可变 commit SHA,再从codeload.github.com下载该 SHA 的源码归档;安装元数据继续保留原始type: git、source、ref,并写入gitCommit,用于后续更新检查和更新。fallback 只在顶层扩展管理器选择。Git 2.37+ 继续使用现有 native Git +
http.curloptResolve路径;带凭据来源、非 GitHub 来源、嵌套 marketplace 内容、git-subdir、submodule 和 Git LFS 均继续使用 native 路径或 fail closed。为什么需要
Ubuntu 22.04 自带 Git 2.34.1,因此 issue 原始的
https://github.com/obra/superpowersdaemon/workspace 安装场景会在 clone 前失败,尽管它是匿名公共仓库。该兼容路径恢复了受支持 LTS 环境的原始场景,同时不移除 Git 2.37 安全边界,也不引入通用代理。Reviewer 测试计划
如何验证
npx vitest run packages/core/src/extension/archive-safety.test.ts packages/core/src/extension/github.test.ts packages/core/src/extension/extensionManager.test.ts。https://github.com/obra/superpowers,不调用 clone,并持久化不可变 commit SHA。GITHUB_TOKEN。UP_TO_DATE或UPDATE_AVAILABLE,更新会安装新的 SHA。http.curloptResolve,并保持禁用重定向、清空代理和仅允许 HTTPS。证据(修改前与修改后)
修改前:新增的 Git 2.34 集成测试在 clone 前以
Public extension Git installs require Git 2.37 or newer...失败。修改后:266 个定向测试全部通过,覆盖旧 Git 安装、ref 解析、metadata、更新检查、更新、网络/token 行为、归档安全、submodule/LFS 拒绝和 Git 2.37 native 路径不变。
测试平台
环境(可选)
Linux 主机,Git 2.19.1.6;通过现有
simple-git边界覆盖 Git 2.34.1 和 2.37 行为。复用仓库已有node_modules。风险与范围
git-subdir;依赖 submodule、Git LFS 或 Git 历史的仓库;未在真实 Ubuntu 22.04 网络环境执行,网络部分通过现有 mock 边界验证。关联 Issue
Fixes #8993