feat(web-shell): manage git remotes from the workspace branch picker - #11163
feat(web-shell): manage git remotes from the workspace branch picker#11163wenshao wants to merge 38 commits into
Conversation
The workspace git popover could pull, commit, push and switch branches, but the remotes themselves were read-only context: a user who clones from a zip (no origin), works triangular (fork + upstream), or wants to drop a stale remote had to leave the Web Shell for a terminal. Adds a Manage Remotes panel inside the popover — list, add, remove — backed by new workspace-scoped daemon routes and the SDK methods for them. The listing reads git through its structured accessors rather than the rendered "remote -v" output, which git annotates for partial-clone remotes and which a line parser silently misreads. Mutations answer with the fresh list so the panel re-renders in one round trip, and a remove also refreshes the branch listing and the tracking state of the chip, because git deletes the remote-tracking refs and upstream config together with the remote. Names and URLs are validated before git is spawned (flag injection, refname rules for add; removal stays as lenient as git itself so a hand-edited config can always be cleaned up), and rendered through a display sanitizer that strips the invisible-character set — a git config the user did not author can carry bidi marks that spoof the displayed URL. Design doc: docs/design/git-manage-remotes.md
E2E test reportEnvironment: macOS (darwin), Node 22, local build ( Web Shell UI e2e (Playwright)
Daemon routes (supertest + real temp repos) — 27/27List with push-URL override ( Core (real temp repos) — 55/55Structured-accessor listing: sorted order, push-URL override, partial-clone promisor remote (the SDK — 416/416 (2 new)URL/method/body composition for Web Shell unit — 228/228
Gates (on the merge-base tree,
|
|
Thanks for the PR — re-running the gate on the current head. Template looks good ✓ — every required heading is present, and the bilingual body, the Before/After screenshots and the OS matrix are filled in rather than stubbed. Problem: this is a feature, not a fix, so there is no reproduction to ask for. The gap it names is observable in the current code — the popover can pull, commit, push and switch branches, but remotes are read-only context, appearing only as group headers over remote-tracking branches. Anyone on a fork + upstream layout, or working from a zip clone with no The part that is theoretical hardening rather than an observed problem is the invisible-character and confusables work: no linked issue, no report of anyone actually spoofed by a homoglyph remote name. I have not treated that as a 1b blocker, because it arrived as review-round output (the R10/R11 commits) rather than as the PR's premise — but it is the right thing to question, and I come back to it under Approach. Direction: aligned. Finishing the workspace git popover is a natural extension of it, the routes follow the existing workspace-scoped precedent instead of adding a legacy unscoped form, and everything is additive — no public contract, auth, sandbox or telemetry surface touched. CHANGELOG (claude-code): no direct reference — every Size: core paths are touched (
Production logic is ~8.8x the 500-line core threshold and ~4.4x the 1000-line large-PR advisory, so both fire. The maintainer-awareness escalation is self-satisfied — you are the maintainer — but it is what caps my Stage 3 confidence at 3/5 no matter how clean the review reads. The split advisory is informational only: I am not asking for a split at this point in the PR's life, and saying so would be noise after 16 rounds. Approach: the feature itself is proportionate — three routes, matching SDK methods, one core helper reading What is not proportionate is that 6,834 of the 21,229 lines — 32% of the whole diff — exist to flag remote names that look alike: a full UTS #39 confusables table, a generator for it, and a fixed-point skeleton fold that iterates up to 8 passes because confluence is not proven. Cut all of it and the list/add/remove panel still works end to end. That is the 80/20 question the gate exists to ask, and the honest answer is the feature survives the cut intact. I am not raising that as a blocker, for two reasons: it was pulled in by review rounds rather than volunteered, and our own rule past ~5 rounds is Criticals only. But it is worth naming, because it is the clearest instance of what that rule exists to prevent — the diff has grown 6.6x since the review first measured it, and round 16 said so explicitly while asking a human to decide whether the shape is still right. That question is above my pay grade and I have not answered it. Two smaller reuse notes, both non-blocking:
Risk: no elevated risk signals — the 1e high-risk path screen matched nothing in the changed files. The genuinely risky surface is the one you flagged yourself: One description nit: test-plan item 9 lists Moving on to code review. 🔍 中文说明感谢贡献——在当前 head 上重跑准入门禁。 模板完整 ✓——必需标题齐全，双语正文、前后对比截图和 OS 矩阵都是实填而非占位。 **问题：**这是 feature 而非 fix，因此不存在「要求复现」这一说。它指出的缺口在现有代码里可直接观察：弹层能 pull、commit、push、切分支，但 remotes 只是只读上下文，仅作为远程跟踪分支的分组标题出现。fork + upstream 布局的用户、或从 zip 解压（没有 真正属于「理论性加固」而非已观测问题的，是不可见字符与 confusables 那部分：没有关联 issue，也没有任何人真的被同形字 remote 名欺骗过的报告。我没有按 1b 阻断它，因为它是评审轮次的产物（R10/R11 提交），不是这个 PR 的立项前提——但它确实值得被质疑，我在「方案」里回到这一点。 **方向：**对齐。补齐 workspace git 弹层是自然延伸；路由沿用既有 workspace 级先例，没有新增 legacy 非 scoped 形态；全部是增量——未触及公开契约、鉴权、沙箱或遥测面。CHANGELOG（claude-code）：无直接对应——那里所有 **规模：**触及了核心路径（
生产逻辑约为 500 行核心阈值的 8.8 倍、1000 行大 PR 提示线的 4.4 倍，两条都触发。维护者知会式升级在你这里自我满足——你就是维护者——但正是它把我的 Stage 3 信心分封顶在 3/5，无论评审读起来多干净。拆分建议仅供参考：在 PR 走到这个阶段我不会要求拆分，第 16 轮之后再提就是噪音。 **方案：**特性本身是相称的——三个路由、对应的 SDK 方法、一个直接读 不相称的是：21,229 行里有 6,834 行——整个 diff 的 32%——只是为了标记「长得像」的 remote 名：一张完整的 UTS #39 confusables 表、它的生成脚本，以及一个因为收敛性未被证明而最多迭代 8 轮的定点 skeleton 折叠。把这些全砍掉，列表/添加/删除面板依然端到端可用。这正是门禁该问的 80/20 问题，诚实的答案是：特性砍完仍然完整。 我没有把它作为阻断项提出，理由有二：它是被评审轮次拉进来的，不是主动加的；而我们自己在超过 ~5 轮之后的规则是只落 Critical。但它值得被点名，因为它是那条规则要防之事的最清晰样本——diff 自评审首次测量以来涨了 6.6 倍，第 16 轮明确说了这一点，并请人类判断这个改动的形态是否还合适。那个问题超出我的职权，我没有回答。 两条较小的复用提示，均非阻断：
**风险：**无升级风险信号——1e 高风险路径筛查在变更文件中没有命中。真正有风险的面是你自己点出的那个： 一处描述小疵：测试计划第 9 条把 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 3 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewI sketched my own approach before reading the diff — three workspace-scoped routes, matching SDK methods, one core helper reading The two Criticals round 16 could not settle — I read both against this commit and can give you verdicts. R15-1, One consequence worth stating rather than burying: when a surviving remote has a flat or mid-wildcard dest, the sweep deletes nothing at all, so a refspec-less removal leaves its orphaned R15-2, Residual I could not close, and am deliberately not calling Critical: the ownership probe itself spawns git with The shared classifier — the blast radius you flagged yourself. All six new arms are matched ahead of every legacy keyword branch, on the full untruncated redacted detail, anchored to line 1 or git's documented two-line lock chain. I checked each for cross-route capture:
The widened redaction. Shape arms first for the Smaller notes, none blocking:
The removal path, which carries the sweep and the refusal polarity: sequenceDiagram
participant P1 as User
participant P2 as Remotes panel
participant P3 as DaemonClient SDK
participant P4 as Remove route
participant P5 as Core gitRemoteRemove
participant P6 as git
P1->>P2: click trash twice (arm then confirm)
P2->>P3: workspaceGitRemoteRemove(cwd, name)
P3->>P4: POST workspaces/:workspace/git/remote/remove
P4->>P5: gitRemoteRemove(cwd, name, env)
P5->>P6: pre-flight reads (config -z, remote, worktree list)
P5->>P6: remote remove -- name
P5->>P6: sweep owned tracking refs, then sibling worktree keys
P5->>P6: re-verify the section across every scope
alt section still resolves somewhere
P5-->>P4: throw remote still configured after removal
P4-->>P2: 409 remote_still_configured
else clean
P5-->>P4: fresh remote list
P4-->>P2: 200 with v and remotes
end
P2->>P3: refresh branches and the chip tracking state
P2-->>P1: re-render in one round trip
Files changed (28 of 33 shown)
TestingThis is an unattended CI run, so I built and ran nothing — no
The PR's own CI is green. Every One check is red: The web-shell suite failed exactly one case out of 7,753, and it is yours: Line 3764 is Not verified, and why: I did not run the daemon, the Playwright spec, or any vitest suite — the skill forbids executing PR-derived code on this path. So the counts in your description (core 162, cli routes 100, SDK 417, web-shell 109, e2e 5) are your claim, not evidence I reproduced, and the behavioural claims in test-plan items 1 to 9 — the two-click confirm, the race re-read, the invisible-character rendering, the partial-clone push-URL tooltip — rest on the mocked-daemon Playwright spec and your local runs. Nothing here exercised a live daemon against a real repository in a browser. Sandboxed verification would settle this: Real-scenario testing (2c): N/A — unattended CI run, so tmux driving is out of scope by rule; the CI signal above and the 中文说明代码审查我在读 diff 之前先自己勾了一版方案——三个 workspace 级路由、对应的 SDK 方法、一个读 第 16 轮无法定论的两个 Critical——我对着当前 commit 都读了，可以给出结论。 **R15-1， 有一个后果值得明说而不是埋着：当存活 remote 配了扁平（ **R15-2， 我没能闭合的残留，也刻意不按 Critical 报：所有权探针本身就会在一个取自仓库可控内容的路径里 spawn git，而此时所有权尚未确立。 **共享分类器——你自己点出的爆炸半径。**六个新分支全部排在遗留关键字分支之前，对未截断的完整脱敏文本匹配，锚定在第 1 行（或 git 文档化的两行 lock 链）。我逐条查了跨路由误捕：
**加宽的脱敏。**先是对 较小的提示，均不阻断：
（删除路径的时序图见上，未重复。） 测试这是无人值守的 CI 运行，所以我什么都没构建、没跑——没有 PR 自己的 CI 是绿的。该 head 上所有 一个 check 是红的： web-shell 套件 7,753 例中恰好失败 1 例，而它是你的： 第 3764 行是 **未验证项及原因：**我没有跑 daemon、没有跑 Playwright spec、没有跑任何 vitest 套件——本路径禁止执行 PR 派生代码。所以你描述里的数字（core 162、cli 路由 100、SDK 417、web-shell 109、e2e 5）是你的声明，不是我复现出的证据；测试计划第 1–9 条的行为性主张——两段式确认、竞态重读、不可见字符渲染、partial-clone 的 push-URL tooltip——依赖 mock daemon 的 Playwright spec 和你的本地运行。这里没有任何环节在浏览器里对着真实仓库跑过真实 daemon。 沙箱化验证可以定这件事： 真实场景测试（2c）：N/A——无人值守 CI 运行，按规则不做 tmux 驱动；上面的 CI 信号与 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the code is better than I expected going in and I could settle both Criticals the last round could not, but the core-size escalation caps this at 3 by policy, and I have two real uncertainties that are not mine to resolve. ⏸️ Deferring to @qqqys — not approving, and not requesting changes either. You approved this commit at 14:12 while this run was in flight; that is your vote and it does not substitute for the gate's, and nothing I found below says your approval is wrong. It says what I could not settle, so you can decide with it in front of you. I have assigned the PR to you. On the code. I went in expecting to argue about scope and came out mostly impressed. The removal path is the load-bearing part and it is the part I would have written wrong: I would have called I also could not find a materially simpler path that solves the same problem, which is the test I actually apply. My independent sketch matched the feature and was simpler only because it was less correct. The exception is the confusables table, and I want to be precise about what I am and am not saying. I am not saying it is wrong — a homoglyph remote name in a Why I am deferring rather than approving:
What I would do in your position: answer the Windows focus question (or delete the assertion with a reason, if it is a jsdom artifact — it is your test and your call), then land it. The deferred Suggestions from rounds 11 to 16, the zh-CN depth gap, the I did not add another review. The PR already carries a gating 中文说明信心分：3/5——代码比我进来时预期的好，上一轮定不了的两个 Critical 我都能给出结论，但核心规模触发的升级按策略把分数封顶在 3，而且我还有两个不属于我职权范围的真实不确定项。 ⏸️ **转交 @qqqys——既不批准，也不请求变更。**你在这次运行进行中、于 14:12 批准了这个 commit；那是你的一票，不能替代门禁的一票，而我下面所发现的内容也不说明你的批准是错的。它说明的是我没能定下来的部分，好让你在这些信息面前做决定。我已把 PR 指派给你。 **关于代码。**我本来准备就范围问题争论，结果大部分是被说服的。删除路径是承重部分，也正是我会写错的部分：我只会调一次 我也没能找到一条能解决同样问题的、显著更简的路径——这才是我实际使用的判据。我独立勾的方案在特性部分与之一致，之所以更简，只是因为它更不正确。 例外是 confusables 表，我想把「在说什么」和「不在说什么」分清楚。我不是说它错——一个你没亲手写的 为什么是转交而不是批准：
如果我在你的位置：先把 Windows 焦点这个问题回答掉（如果确认是 jsdom 伪影，也可以带理由删掉那条断言——那是你的测试、你的决定），然后合了它。第 11 到 16 轮 deferred 的那些 Suggestion、中文文档的深度差、 我没有再提交一条评审。这个 PR 已经带着一条起门禁作用的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- DaemonGitRemotesResult.available is declared, always true, and read by no consumer — already reported (triage stage-2 comment 5557805467)
Not explored to full depth (tool budget reached): "agent 1c": none — every check above ran to completion..
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1c"：none — every check above ran to completion.。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
The config-scope read, removal verification and client refusal handling carried gaps the reverse audits surfaced: include.path/worktree scopes were under-listed yet un-removable (destructive dead ends), GIT_ALLOW_PROTOCOL was deleted outright (a fail-open swap against config-file policy), the classifier let echoed names/URLs steal config-write failures, and several guards shipped without witnesses. Fixes: - list the repository-owned scope via `config --list --show-scope -z` (local+worktree), completing worktree-scope removals git cannot edit - normalize an inherited GIT_ALLOW_PROTOCOL (strip ext/fd) instead of deleting it; keep deny-by-default force - order sendGitError's anchored config-write shapes first; add remote_config_unparsable for unparsable configured refspecs - follow git's boolean/integer grammar exactly (valueless/empty/hex/kmg) - keep the URL visible under row shrink; restore focus after in-flight mutations; disarm confirms on refusal; silent re-reads stay silent - witness every guard above in unit/e2e suites; sync the design doc
…at/git-manage-remotes
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": running packages/cli/src/serve/routes/workspace-git-remotes.test.ts with the runtime.env.effectiveEnv argument dropped, to observe the whole suite stay gree…; "agent reverse-audit (round 1)": an executed probe of the timed-out-config-read path (hanging git config --list under GIT_TIMEOUT_MS to capture the real 512-char message body) — the chain…; "agent reverse-audit (round 1)": the design doc's round-1 (unchanged) sections outside this delta's hunks, and its per-test coverage claims for git-remotes.test.ts beyond the three I confirme…; chunk 17: running the Playwright suite to measure the geometry/overflow assertions in web-shell.git-remotes.spec.ts tests 4 and 5 (dev server + browser run); reasoned f…; "agent reverse-audit (round 2)": did not execute packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts in a browser — the layout, sticky and focus-clearance assertions are verified agains….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: "agent verify (round 2)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/workspace-git-remotes.test.ts:24 — [probe] nothing pins { strict: true } on the two git-config write routespackages/web-shell/client/components/BranchPickerPopover.test.tsx:1837 — [review] two-click confirm never exercised across two rowspackages/web-shell/client/components/BranchPickerPopover.tsx:504 — [probe] reopen commits the previous session's (or another worktree's) remotes panelpackages/web-shell/client/components/BranchPickerPopover.tsx:1076 — [probe] search filter survives a mutation, hiding the remote just addedpackages/web-shell/client/components/BranchPickerPopover.tsx:91 — [probe] display sanitizer fuses git's multi-line error diagnostics
[Critical] R2-2: [certifies-falsely] [new-surface] gitIntegerValue mirrors neither git's leading-whitespace acceptance nor its INT_MAX bound, so promisor is reported false for a config value git reads as true — suppressing the partial-clone warning badge for exactly the remote it exists for — and true for a value git refuses at read time. Measured on git 2.43.0: git config remote.origin.promisor ' 1' then git config --bool --get remote.origin.promisor exits 0 with true (same for '\t1', ' 0x1', ' 1k'; only TRAILING padding dies), while the regex at git-remotes.ts:140 has no leading-whitespace arm, so gitIntegerValue(' 1') returns null and fetchGitRemotes reports promisor: false; BranchPickerPopover.tsx:1792 gates the badge on r.promisor || r.partialCloneFilter, so a promisor remote with no partialclonefilter key renders with no badge. The bound diverges the other way: git refuses |valuefactor| > 2147483647 (2048m, 2147483648, -2g all fatal) while unbounded JS arithmetic reports promisor: true, certifying a partial clone for a config every git command fatals on. Witness: git 2.43.0 — stored " 1" -> git config --bool --get exit 0 'true'; stored "1 " -> exit 128 'fatal: bad boolean config value'; 2048m/2147483648/-2g -> fatal while the copied regex plus parseInt returns true; --list --show-scope -z byte-dumped with od -c showing the padded value verbatim. Fix: mirror strtoumax's isspace() skip and git's bound — /^\s([+-]?)(0x[0-9a-f]+|0[0-7]|[1-9][0-9])([kmg]?)$/ keeping the $ anchor so trailing padding still fails as git does, and return null when |value*factor| exceeds 2147483647. (Unanchorable: the quoted constant does not appear in any hunk of git-remotes.ts at the reviewed commit, so this blocker's only copy is here.) The fix must not violate git's asymmetry, already pinned by the row [' 1 ', false] at git-remotes.test.ts:597, nor move the value.toLowerCase() at git-remotes.ts:161 that makes '0X1f'/'1G' work. The test that must go red without this fix: new rows in the promisor it.each at git-remotes.test.ts:581 — [' 1', true], ['\t1', true], [' 1k', true], ['2048m', false], ['-2g', false] — with ['1 ', false] as the counter-mutation that keeps a blanket value.trim() from passing.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：running packages/cli/src/serve/routes/workspace-git-remotes.test.ts with the runtime.env.effectiveEnv argument dropped, to observe the whole suite stay gree…；"agent reverse-audit (round 1)"：an executed probe of the timed-out-config-read path (hanging git config --list under GIT_TIMEOUT_MS to capture the real 512-char message body) — the chain…；"agent reverse-audit (round 1)"：the design doc's round-1 (unchanged) sections outside this delta's hunks, and its per-test coverage claims for git-remotes.test.ts beyond the three I confirme…；chunk 17：running the Playwright suite to measure the geometry/overflow assertions in web-shell.git-remotes.spec.ts tests 4 and 5 (dev server + browser run); reasoned f…；"agent reverse-audit (round 2)"：did not execute packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts in a browser — the layout, sticky and focus-clearance assertions are verified agains…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未审查："agent verify (round 2)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
[Critical] R2-2: [certifies-falsely] [new-surface] gitIntegerValue mirrors neither git's leading-whitespace acceptance nor its INT_MAX bound, so promisor is reported false for a config value git reads as true — suppressing the partial-clone warning badge for exactly the remote it exists for — and true for a value git refuses at read time. Measured on git 2.43.0: git config remote.origin.promisor ' 1' then git config --bool --get remote.origin.promisor exits 0 with true (same for '\t1', ' 0x1', ' 1k'; only TRAILING padding dies), while the regex at git-remotes.ts:140 has no leading-whitespace arm, so gitIntegerValue(' 1') returns null and fetchGitRemotes reports promisor: false; BranchPickerPopover.tsx:1792 gates the badge on r.promisor || r.partialCloneFilter, so a promisor remote with no partialclonefilter key renders with no badge. The bound diverges the other way: git refuses |valuefactor| > 2147483647 (2048m, 2147483648, -2g all fatal) while unbounded JS arithmetic reports promisor: true, certifying a partial clone for a config every git command fatals on. Witness: git 2.43.0 — stored " 1" -> git config --bool --get exit 0 'true'; stored "1 " -> exit 128 'fatal: bad boolean config value'; 2048m/2147483648/-2g -> fatal while the copied regex plus parseInt returns true; --list --show-scope -z byte-dumped with od -c showing the padded value verbatim. Fix: mirror strtoumax's isspace() skip and git's bound — /^\s([+-]?)(0x[0-9a-f]+|0[0-7]|[1-9][0-9])([kmg]?)$/ keeping the $ anchor so trailing padding still fails as git does, and return null when |value*factor| exceeds 2147483647. (Unanchorable: the quoted constant does not appear in any hunk of git-remotes.ts at the reviewed commit, so this blocker's only copy is here.) The fix must not violate git's asymmetry, already pinned by the row [' 1 ', false] at git-remotes.test.ts:597, nor move the value.toLowerCase() at git-remotes.ts:161 that makes '0X1f'/'1G' work. The test that must go red without this fix: new rows in the promisor it.each at git-remotes.test.ts:581 — [' 1', true], ['\t1', true], [' 1k', true], ['2048m', false], ['-2g', false] — with ['1 ', false] as the counter-mutation that keeps a blanket value.trim() from passing.
— qwen3.8-max via Qwen Code /review (v0.23.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): chunk 5: none — all planned checks completed (17 of ~33 tool calls).; chunk 12: none — I did not run vitest on BranchPickerPopover.test.tsx locally, relying instead on the passing CI Test check for that file.; "agent reverse-audit (round 1)": none — but for completeness, two checks I chose not to run rather than was cut short on: whether git can store a literal NUL byte inside a config *value* (which….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
docs/design/git-manage-remotes.md:136 — [review] The doc justifies the transport-helper write gate with…docs/design/git-manage-remotes.md:138 — [review] The design of record declares the config-file protocol…docs/design/git-manage-remotes.md:171 — [review] The specified available field is a constant — the only…packages/cli/src/serve/routes/workspace-git-branches.ts:109 — [review] The new /invalid refspec/i branch is inserted into the…packages/cli/src/serve/routes/workspace-git-branches.ts:144 — [review] ROOT-CAUSE FAMILY (three verifier incidentals merged at…packages/cli/src/serve/routes/workspace-git-remotes.test.ts:99 — [review] Nothing in the repo pins that the two new destructive POST…packages/cli/src/serve/routes/workspace-git-remotes.test.ts:156 — [review] The generation test closes the guard *before* the request,…packages/cli/src/serve/routes/workspace-git-remotes.test.ts:227 — [review] The add path pins its wrong-typed guards ( { name: 42 } …packages/cli/src/serve/routes/workspace-git-remotes.test.ts:260 — [review] Nothing in this suite distinguishes the resolved workspace…packages/cli/src/serve/routes/workspace-git-remotes.test.ts:366 — [review] The suite's only wedge covers the *config* lock; the same…packages/cli/src/serve/routes/workspace-git-remotes.test.ts:461 — [review] All three path-redaction assertions in the new real-repo…packages/cli/src/serve/routes/workspace-git-remotes.ts:66 — [review] The add route accepts a url of unbounded length and…packages/core/src/utils/git-branches.test.ts:244 — [review] The new gitEnv test table pins only https, ssh, ext, fd…packages/core/src/utils/git-branches.ts:122 — [review] The PR body's 'Breaking changes / migration notes: none —…packages/core/src/utils/git-remotes-kill.test.ts:38 — [review] Two of isNoMatchConfigError 's five conjuncts — typeof…packages/core/src/utils/git-remotes.test.ts:247 — [review] The one test that configures remote.<name>.promisor and…packages/core/src/utils/git-remotes.test.ts:257 — [review] The only test that drives customRefspec true leaves…packages/core/src/utils/git-remotes.test.ts:358 — [review] The listing deliberately survives an unparseable fetch…packages/core/src/utils/git-remotes.test.ts:588 — [review] The promisor integer-grammar table cannot reach any value…packages/core/src/utils/git-remotes.test.ts:794 — [review] The || scopes.has('local') half of the…- …and 31 more (see the run report)
Convergence: round 3 posted 7 inline comment(s), 3 of them reported for the first time; the previous round posted 7 (6 new). Findings keep coming back to the same files: packages/core/src/utils/git-remotes.ts (findings in rounds 1, 2; 3 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）：chunk 5：none — all planned checks completed (17 of ~33 tool calls).；chunk 12：none — I did not run vitest on BranchPickerPopover.test.tsx locally, relying instead on the passing CI Test check for that file.；"agent reverse-audit (round 1)"：none — but for completeness, two checks I chose not to run rather than was cut short on: whether git can store a literal NUL byte inside a config *value* (which…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 51 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 7 条行内评论，其中 3 条是首次提出；上一轮发布了 7 条（其中 6 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/git-remotes.ts（第 1、2 轮已出过发现，本轮又有 3 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
@qwen-code /takeover from 4 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This window's round counter starts at 4 (the rounds this PR spent in review before takeover), so the Critical-only brake engages after 1 more change-producing round(s) instead of a full fresh 5. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本窗口轮次计数从 4 起算（即本 PR 托管前已进行的评审轮数），因此再经过 1 个产生改动的轮次即进入 Critical-only，而非重新计满 5 轮。移除 |
Seven Critical threads from the review bot's second batch, all fixed with witnesses and a ten-round reverse audit on top: - add pre-flights all config scopes and refuses a same-name inherited section (remote_shadows_inherited), after probing repository-ness; removal verifies resolution across every scope so an inherited survivor can never be reported as removed (fail-closed on killed reads) - sendGitError shapes match git's message start only (line 1, or git's two-line lock chain): config-chosen names/URLs can carry any keyword, and config values can carry real newlines, so deeper line-initial text is attacker-controllable; classification reads the full redacted detail while the client message stays bounded - killed config reads rethrow with the all-scope stdout dump stripped (stderr diagnostics preserved) on every read path - focus restores: add submit button, remove-row button when focused, back-button fallback; restores are gated on lent focus, cleared on view exit and workspace switch; refused removal awaits the silent re-read so the restore lands on the converged list - no_such_remote remove refusals also refresh branches and status
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the new Playwright spec packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts is not run by any PR-gated CI job (e2e.yml has no pull_request trigger, and web_shell_e2e_smoke selects only @smoke-tagged tests while none of the five new tests carry the tag) and was not executed in this review.
Not reviewed: build-and-test — the test-efficacy probe measured nothing: its positive control never ran (the probe tree has no built dist/, so this repository’s vitest globalSetup guard stopped all 12 probes before collection), harnessValidated is null, and no mutant or hunk was probed, so this run says nothing about whether individual hunks are gated by a test.
Not reviewed: issue-fidelity — the closing-issue set could not be checked (the installed gh cannot resolve closingIssuesReferences; gh >= 2.72.0 required), so it is UNKNOWN rather than verified-empty; the PR body’s Linked Issues reads None and fidelity was judged against the description and the committed design doc.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": the BranchPickerPopover.test.tsx cell of the doc's test table — I did not open the test file, so "focus restore to the manage row on exit with a search-box fall…; "agent reverse-audit (round 1)": the workspace-git-remotes.test.ts and git-remotes.test.ts table cells beyond the two I grepped ("helper-URL rejection before spawn" at :490 and "stores the trim…; "agent reverse-audit (round 2)": the url.insteadOf connect-time probe hung inside the ext helper and hit the shell timeout before printing, so that layer's evidence is the hang itself rathe….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Convergence: round 4 posted 57 inline comment(s), 54 of them reported for the first time; the previous round posted 7 (3 new). Findings keep coming back to the same files: packages/web-shell/client/components/BranchPickerPopover.tsx (findings in round 2; 19 more now); packages/core/src/utils/git-remotes.ts (findings in rounds 1, 2, 3; 4 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R4-3: [certifies-falsely] [new-surface] The transport-helper write gate requires a leading alphanumeric, so git's empty-transport-name helper form ::<args> passes isValidRemoteUrl and is written into .git/config, even though git routes that URL through — A client calls the add-remote route with name evil, url ::sh -c id. isValidRemoteUrl('::sh -c id') returns true (non-empty, no leading -, EXECUTING_HELPER_URL needs one [A-Za-z0-9] before ::, no control/invisible chars), so runGit(cwd, ['remote','add','--',name,trimmedUrl]) runs — verified: git remote add -- evil '::sh -c id' exits 0 and writes url = ::sh -c id. Any later connect on that remote (panel pull/fetch, or the user's own git fetch evil in a terminal) takes git's helper path: GIT_TRACE=1 git ls-remote evil shows run_command: GIT_DIR=.git git remote- evil 'sh -c id' then git.c:749 trace: exec: git-remote- evil 'sh -c id' — git looks up the executable git-remote- in its exec path and then on PATH, passing the stored payload as argv, with no protocol.*.allow prompt or denial in a default environment. Unconditionally the planted remote is permanently broken (git: 'remote-' is not a git command); if any directory on the daemon's or user's PATH holds an executable named git-remote-, the stored argument string executes with it — persistence that outlives the session. The gate's own comment says "the write path rejects the form", and the existing table rejects unknown helper names (gcrypt::myrepo, 9p::ssh://host/repo) on precisely the rationale that "an installed git-remote-<name> runs at connect time under the default policy"; git-remote- is looked up the same way and is admitted. The fix must not violate this existing fact: the same table pins ['ssh://git@[::1]/repo.git', true] and ['ssh://git@[2001:db8::1]:22/o/r.git', true] (packages/core/src/utils/git-remotes.test.ts:134-135) — the widened pattern must keep accepting bracketed IPv6 literals, so :: must stay anchored at position 0 rather than matched anywhere in the URL. Please pin this with a test: packages/core/src/utils/git-remotes.test.ts — add rows to the isValidRemoteUrl it.each table (lines 130–166): ['::sh -c id', false] and ['::0', false]. Both go red against the current pattern (true !== false); reverting the pattern to /^[A-Za-z0-9][A-Za-z0-9_.+-]*::/ turns them red again, which is the mutation proof.
[Critical] R3-1: [certifies-falsely] [new-surface] Still stands. fetchGitRemotes receives git's -z byte stream as a lossily UTF-8-decoded JS string, so any config byte that is not valid UTF-8 becomes U+FFFD in the listed name and the row the panel renders can never be removed. — At HEAD the transport is unchanged: runGit calls execFileAsync (git-branches.ts:135-146) with no encoding option, so Node decodes git's NUL-framed byte stream as UTF-8 and replaces every invalid byte with U+FFFD. fetchGitRemotes then does raw.split('\0') over that decoded string and returns the lossy name to the client, which sends it back verbatim as the removal target. A .git/config the user did not author - a downloaded zip, a cloned repo, a hand-edited file - can hold a remote subsection name with a non-UTF-8 byte; the panel renders a row whose name no longer equals the name git holds, so gitRemoteRemove's git remote remove -- <lossy name> answers 'No such remote' and the row can never be removed through the UI. The -z framing exists precisely to make names unambiguous at the byte level, and the decode throws that away before the parser sees it. Please pin this with a test: A case in packages/core/src/utils/git-remotes.test.ts that writes a remote subsection name containing a non-UTF-8 byte directly into .git/config, asserts the listing either surfaces it removably or refuses it explicitly, and asserts the removal round trip succeeds - it goes red against the lossy decode.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — the new Playwright spec packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts is not run by any PR-gated CI job (e2e.yml has no pull_request trigger, and web_shell_e2e_smoke selects only @smoke-tagged tests while none of the five new tests carry the tag) and was not executed in this review.
未审查（原文为英文）：build-and-test — the test-efficacy probe measured nothing: its positive control never ran (the probe tree has no built dist/, so this repository’s vitest globalSetup guard stopped all 12 probes before collection), harnessValidated is null, and no mutant or hunk was probed, so this run says nothing about whether individual hunks are gated by a test.
未审查（原文为英文）：issue-fidelity — the closing-issue set could not be checked (the installed gh cannot resolve closingIssuesReferences; gh >= 2.72.0 required), so it is UNKNOWN rather than verified-empty; the PR body’s Linked Issues reads None and fidelity was judged against the description and the committed design doc.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：the BranchPickerPopover.test.tsx cell of the doc's test table — I did not open the test file, so "focus restore to the manage row on exit with a search-box fall…；"agent reverse-audit (round 1)"：the workspace-git-remotes.test.ts and git-remotes.test.ts table cells beyond the two I grepped ("helper-URL rejection before spawn" at :490 and "stores the trim…；"agent reverse-audit (round 2)"：the url.insteadOf connect-time probe hung inside the ext helper and hit the shell timeout before printing, so that layer's evidence is the hang itself rathe…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛情况：第 4 轮发布了 57 条行内评论，其中 54 条是首次提出；上一轮发布了 7 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/components/BranchPickerPopover.tsx（第 2 轮已出过发现，本轮又有 19 条）；packages/core/src/utils/git-remotes.ts（第 1、2、3 轮已出过发现，本轮又有 4 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R4-3: [certifies-falsely] [new-surface] The transport-helper write gate requires a leading alphanumeric, so git's empty-transport-name helper form ::<args> passes isValidRemoteUrl and is written into .git/config, even though git routes that URL through — A client calls the add-remote route with name evil, url ::sh -c id. isValidRemoteUrl('::sh -c id') returns true (non-empty, no leading -, EXECUTING_HELPER_URL needs one [A-Za-z0-9] before ::, no control/invisible chars), so runGit(cwd, ['remote','add','--',name,trimmedUrl]) runs — verified: git remote add -- evil '::sh -c id' exits 0 and writes url = ::sh -c id. Any later connect on that remote (panel pull/fetch, or the user's own git fetch evil in a terminal) takes git's helper path: GIT_TRACE=1 git ls-remote evil shows run_command: GIT_DIR=.git git remote- evil 'sh -c id' then git.c:749 trace: exec: git-remote- evil 'sh -c id' — git looks up the executable git-remote- in its exec path and then on PATH, passing the stored payload as argv, with no protocol.*.allow prompt or denial in a default environment. Unconditionally the planted remote is permanently broken (git: 'remote-' is not a git command); if any directory on the daemon's or user's PATH holds an executable named git-remote-, the stored argument string executes with it — persistence that outlives the session. The gate's own comment says "the write path rejects the form", and the existing table rejects unknown helper names (gcrypt::myrepo, 9p::ssh://host/repo) on precisely the rationale that "an installed git-remote-<name> runs at connect time under the default policy"; git-remote- is looked up the same way and is admitted. The fix must not violate this existing fact: the same table pins ['ssh://git@[::1]/repo.git', true] and ['ssh://git@[2001:db8::1]:22/o/r.git', true] (packages/core/src/utils/git-remotes.test.ts:134-135) — the widened pattern must keep accepting bracketed IPv6 literals, so :: must stay anchored at position 0 rather than matched anywhere in the URL. Please pin this with a test: packages/core/src/utils/git-remotes.test.ts — add rows to the isValidRemoteUrl it.each table (lines 130–166): ['::sh -c id', false] and ['::0', false]. Both go red against the current pattern (true !== false); reverting the pattern to /^[A-Za-z0-9][A-Za-z0-9_.+-]*::/ turns them red again, which is the mutation proof.
[Critical] R3-1: [certifies-falsely] [new-surface] Still stands. fetchGitRemotes receives git's -z byte stream as a lossily UTF-8-decoded JS string, so any config byte that is not valid UTF-8 becomes U+FFFD in the listed name and the row the panel renders can never be removed. — At HEAD the transport is unchanged: runGit calls execFileAsync (git-branches.ts:135-146) with no encoding option, so Node decodes git's NUL-framed byte stream as UTF-8 and replaces every invalid byte with U+FFFD. fetchGitRemotes then does raw.split('\0') over that decoded string and returns the lossy name to the client, which sends it back verbatim as the removal target. A .git/config the user did not author - a downloaded zip, a cloned repo, a hand-edited file - can hold a remote subsection name with a non-UTF-8 byte; the panel renders a row whose name no longer equals the name git holds, so gitRemoteRemove's git remote remove -- <lossy name> answers 'No such remote' and the row can never be removed through the UI. The -z framing exists precisely to make names unambiguous at the byte level, and the decode throws that away before the parser sees it. Please pin this with a test: A case in packages/core/src/utils/git-remotes.test.ts that writes a remote subsection name containing a non-UTF-8 byte directly into .git/config, asserts the listing either surfaces it removably or refuses it explicitly, and asserts the removal round trip succeeds - it goes red against the lossy decode.
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 更新了一个过期的 base —— 修复未通过验证，但本 PR 落后于 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34192434018 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse-audit rounds 1-2 reported 78 further findings that the time budget left undeduplicated and unverified — none is posted and none is counted as a blocker.
Not reviewed: verification shard 3 (6 findings) — the verifier completed but never opened its brief, so no verdict for those findings is recoverable.
Not reviewed: reverse audit of chunk 34, round 2 — the agent failed on a provider content-filter error over the generated Unicode confusables table.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": the Chinese design twin's structural and claim-level sync against EN doc lines 72-472 (my whole range) — I never opened docs/design/git-manage-remotes.zh-CN.md…; "agent reverse-audit (round 2)": line-by-line verification of the §6 test-table coverage assertions in zh:134-138 (~200 individual claims across five test files) — I spot-checked only three (th…; "agent reverse-audit (round 2)": the zh:122 CSS class list ( .remotesHeader , .backButton , .remotesTitle , .remoteRow , .remoteName , .remoteBadge , .remoteUrl , .remoteRemove , .remot…; "agent reverse-audit (round 1)": the ~10k-character coverage rows at zh:131-138 ( git-remotes.test.ts , workspace-git-remotes.test.ts , BranchPickerPopover.test.tsx , e2e spec) — I verified o…; "agent reverse-audit (round 1)": zh:119's premise that git remote remove deletes tracking refs and branch tracking config BEFORE the section write (the stated justification for refreshing bra…, and 13 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: "agent verify" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
docs/design/git-manage-remotes.md:349 — [review] EN doc attaches the HEAD refusal to the wrong predicate;…packages/cli/src/serve/routes/workspace-git-branches.test.ts:594 — [review] One 450-line it mutates shared fixtures across ~20…packages/cli/src/serve/routes/workspace-git-branches.ts:51 — [review] Filesystem root as a redaction key eats every slash in the…packages/cli/src/serve/routes/workspace-git-branches.ts:115 — [review] in file redaction arm eats remote sideband text on…packages/cli/src/serve/routes/workspace-git-branches.ts:318 — [review] ~25 full-length redaction passes over an unbounded ~20 MB…packages/core/src/utils/git-remotes-kill.test.ts:66 — [review] Kill fixture pins exit 128 where real git exits 2packages/core/src/utils/git-remotes-kill.test.ts:103 — [review] The signal half of isKillError has no witness in 26 testspackages/core/src/utils/git-remotes.test.ts:85 — [review] Teardown retry handles only EBUSY, not Node's other four…packages/core/src/utils/git-remotes.test.ts:223 — [review] The Default_Ignorable arm of INVISIBLE_CHARS has no witnesspackages/core/src/utils/git-remotes.test.ts:526 — [review] A test named for the invalid-refspec guard cannot fail…packages/core/src/utils/git-remotes.test.ts:630 — [review] Comment names a gitConfig() helper that does not exist herepackages/web-shell/client/components/BranchPickerPopover.test.tsx:4228 — [review] A never-resolving branch mock leaks past the end of its testpackages/web-shell/client/components/BranchPickerPopover.tsx:606 — [review] Remotes panel paints for a frame on reopen after dismissalpackages/web-shell/client/components/BranchPickerPopover.tsx:915 — [review] Unknown remove outcome is classified as "nothing was…packages/web-shell/client/components/BranchPickerPopover.tsx:1009 — [review] Focus restore yanks focus from a control the user moved topackages/web-shell/client/components/BranchPickerPopover.tsx:1044 — [review] A rejected add leaves a destructive remove-confirm armedpackages/web-shell/client/utils/unicodeConfusables.ts:11 — [review] 6,565-entry generated table committed with no integrity gate
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (4 Critical(s)), the rate of first-time findings is not falling (this round 3, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：reverse-audit rounds 1-2 reported 78 further findings that the time budget left undeduplicated and unverified — none is posted and none is counted as a blocker.
未审查（原文为英文）：verification shard 3 (6 findings) — the verifier completed but never opened its brief, so no verdict for those findings is recoverable.
未审查（原文为英文）：reverse audit of chunk 34, round 2 — the agent failed on a provider content-filter error over the generated Unicode confusables table.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：the Chinese design twin's structural and claim-level sync against EN doc lines 72-472 (my whole range) — I never opened docs/design/git-manage-remotes.zh-CN.md…；"agent reverse-audit (round 2)"：line-by-line verification of the §6 test-table coverage assertions in zh:134-138 (~200 individual claims across five test files) — I spot-checked only three (th…；"agent reverse-audit (round 2)"：the zh:122 CSS class list ( .remotesHeader , .backButton , .remotesTitle , .remoteRow , .remoteName , .remoteBadge , .remoteUrl , .remoteRemove , .remot…；"agent reverse-audit (round 1)"：the ~10k-character coverage rows at zh:131-138 ( git-remotes.test.ts , workspace-git-remotes.test.ts , BranchPickerPopover.test.tsx , e2e spec) — I verified o…；"agent reverse-audit (round 1)"：zh:119's premise that git remote remove deletes tracking refs and branch tracking config BEFORE the section write (the stated justification for refreshing bra…，另有 13 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未审查："agent verify"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
未审查：验证——它的 prompt 已构建，但没有 agent 用它启动，发布的发现不能算作已验证。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
收敛姿态下延后（第 13 轮，非阻断）——已记录，本轮不要求修改：共 17 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 4 条 Critical），首次发现的速率没有下降（本轮 3，上一轮 0），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.3)
…lout - R4-3: widen EXECUTING_HELPER_URL to admit git's empty-name helper form (`::payload` execs a PATH-resolved `git-remote-`); rationale corrected — the empty name is not on git's deny-by-default list, and a protocol.allow policy that would cover it is overridable from config files. Witness rows `::sh -c id`/`::0`/`::`; mutation of the pattern turns them red while the IPv6 literals stay accepted. - R12-1: restore the local upstream backups at the TOP of the removal-failure path (above the !completed throw and the converge classification), so a refusal following an already-destructive rm cannot skip the rollback; a killed restore read rethrows instead of letting git's original answer surface un-rolled-back (kill witness pins the restore read's argv, separating it from the converge gate's scope read). - R13-2: the converge arm excludes the removed name's own namespace-less top-level bare ref from sweep AND re-verify while keeping the longest-prefix ownership resolution whole — a bare ref exactly owned by a configured slashed sibling keeps that owner (first attempt at this fix regressed exactly that; witnesses: remote-HEAD symref, flat layout, slashed sibling bare ref). Accepted residuals disclosed: a formerly-configured name's bare-ref residue is orphaned, and a flat layout's slashed branch refs stay sweepable. - R13-3: mark BOTH rows when a collision group's skeleton is non-ASCII and the variance is a table fold rather than canonical equivalence (skeletonAscii/allCanonical on the group); witness öö × ةة; the all-ASCII and pure-canonical polarities keep their pinned behavior. - Audit fallout: the TR39 fold moves to client/utils/remote-name-skeleton.ts beside its table integrity gate (no self-maps, no value containing a table key, key folds to value, value is a fold fixed point, NFC); the non-discriminating lowercase-spoof completion test is removed (the exact-prefix test carries the veto witness); design docs synced EN+ZH for the third marking polarity, the error-path restore and its fail-closed masking, the converge residuals, and the new test rows.
|
R13 deferred-list dispositions (review 5184892069 recorded 17 items as "not requested in this round"; triaged as maintainer). The four posted Criticals are answered in their threads: fixed in bab8e50. Fixed in bab8e50 besides the Criticals:
Declined with evidence:
Recorded as follow-up (not landed this round, with reasons):
|
|
CI note (macOS lane): the |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] packages/core/src/utils/git-remotes.ts:504 (R13-2) — the no-such-remote converge arm sweeping a live top-level remote-tracking ref: this round confirmed a NEW Critical in the same converge arm at :546 (R14-4, refs a surviving remote owns), …
Not reviewed: reverse-audit pair findings (59 candidates from rounds 1-2, 12 of them Critical) — never verified: the time budget stopped before a verifier batch could be built, so they are reported terminal-only and are not confirmed blockers.
Not reviewed: test-efficacy probe — unmeasured: all 14 probes returned inconclusive (no-output) and the positive control never ran (harnessValidated null), so inert / mutant-survived / hunk-survived are unprobed scope, not clean results.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: chunk 29 — did not verify by execution whether any production caller mutates workspaceCwd/gitCwd while branchPickerOpen is true — stopped at the agent tool budget.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": matching row 698's ~150 named cases and row 701's ~90 one-by-one to test titles in git-remotes.test.ts / BranchPickerPopover.test.tsx — I spot-checked the e…; "agent reverse-audit (round 2)": none was cut short — the full 278-line range (diff 13642-13919) was read, and every claim above was checked against HEAD sources in the worktree.; chunk 3: none — no check in this territory was cut short.; "agent reverse-audit (round 2)": the 现状 table's approximate pre-change line numbers ( server.ts 约 L2403-2413, DaemonClient.ts 约 L6600-6770, i18n.tsx en 约 L34-80 / zh 约 L3523-3566, mockDae…; "agent reverse-audit (round 2)": the 问题陈述's inventory of pre-existing git remote readers was verified only for the two review-command paths ( packages/cli/src/commands/review/lib/remote-match…, and 6 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: "agent verify" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
1 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/workspace-git-remotes.test.ts:80 — [review] R14-1: The hermeticity pre-condition matches any key in the bare [remote] section — notably remote.pushDefault — not a [remote "<name>"] subsection, so it decla…docs/design/git-manage-remotes.md:721 (+2 locations) — [review] The "Files affected" inventory omits…packages/core/src/utils/git-remotes.test.ts:1004 — [review] The test's own stated premise — "the name still resolves…packages/core/src/utils/git-remotes.ts:1511 — [review] localBranchKeyValues parses git config --get-all …packages/core/src/utils/git-remotes.ts:1532 — [review] The rollback path pays two sequential git config process…packages/web-shell/client/components/BranchPickerPopover.tsx:2091 — [review] For a URL-side anomaly the only evidence is a hover…packages/core/src/utils/git-remotes.ts:1431 — [review] The **sibling-worktree** remote.pushDefault sweep — and…packages/core/src/utils/git-remotes.ts:560 — [review] The **converge arm's** post-sweep resolution refusal (a…packages/web-shell/client/components/BranchPickerPopover.tsx:2218 — [review] The push-only fan-out arm of the removal-consequence badge…packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts:38 — [review] The PR description's "Executable evidence" line names this…packages/cli/src/serve/routes/workspace-git-branches.test.ts:594 — [review] One 447-line it bundles ~17 independent redaction…packages/cli/src/serve/routes/workspace-git-branches.ts:101 — [review] The /etc/gitconfig token arm tokenizes the entire…packages/cli/src/serve/routes/workspace-git-branches.ts:260 — [review] readHead 's non-regular-file guard is a stat-then-open…packages/core/src/utils/git-remotes-kill.test.ts:223 — [review] The ~15 kill-site sequencing tests identify *which* read…packages/core/src/utils/git-remotes-kill.test.ts:393 — [review] The module-level runGit mock is never reset between…packages/core/src/utils/git-remotes.test.ts:497 — [review] Six mutation-path tests are filed under…packages/core/src/utils/git-remotes.test.ts:866 — [review] The otherSettings counter's one documented exception —…
Convergence: round 14 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 4 (3 new). Findings keep coming back to the same files: packages/core/src/utils/git-remotes.ts (findings in rounds 12, 13; 3 more now); packages/web-shell/client/components/BranchPickerPopover.tsx (findings in round 13; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (6 Critical(s)), the rate of first-time findings is not falling (this round 6, previous 3), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：reverse-audit pair findings (59 candidates from rounds 1-2, 12 of them Critical) — never verified: the time budget stopped before a verifier batch could be built, so they are reported terminal-only and are not confirmed blockers.
未审查（原文为英文）：test-efficacy probe — unmeasured: all 14 probes returned inconclusive (no-output) and the positive control never ran (harnessValidated null), so inert / mutant-survived / hunk-survived are unprobed scope, not clean results.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：chunk 29 — did not verify by execution whether any production caller mutates workspaceCwd/gitCwd while branchPickerOpen is true — stopped at the agent tool budget.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：matching row 698's ~150 named cases and row 701's ~90 one-by-one to test titles in git-remotes.test.ts / BranchPickerPopover.test.tsx — I spot-checked the e…；"agent reverse-audit (round 2)"：none was cut short — the full 278-line range (diff 13642-13919) was read, and every claim above was checked against HEAD sources in the worktree.；chunk 3：none — no check in this territory was cut short.；"agent reverse-audit (round 2)"：the 现状 table's approximate pre-change line numbers ( server.ts 约 L2403-2413, DaemonClient.ts 约 L6600-6770, i18n.tsx en 约 L34-80 / zh 约 L3523-3566, mockDae…；"agent reverse-audit (round 2)"：the 问题陈述's inventory of pre-existing git remote readers was verified only for the two review-command paths ( packages/cli/src/commands/review/lib/remote-match…，另有 6 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未审查："agent verify"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
未审查：验证——它的 prompt 已构建，但没有 agent 用它启动，发布的发现不能算作已验证。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
1 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 14 轮，非阻断）——已记录，本轮不要求修改：共 17 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 14 轮发布了 6 条行内评论，其中 6 条是首次提出；上一轮发布了 4 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/git-remotes.ts（第 12、13 轮已出过发现，本轮又有 3 条）；packages/web-shell/client/components/BranchPickerPopover.tsx（第 13 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 6 条 Critical），首次发现的速率没有下降（本轮 6，上一轮 3），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.3)
… fallout
- R14-2: the TR39 fold iterates to a fixed point (capped) instead of
running one pass — the closing NFC can compose a table key and a
decomposed spelling only meets the table after composition; new
remote-name-skeleton.ts carries the fold beside its integrity gate
and fixed-point corner tests.
- R14-3: redaction gains a quoted-path arm (git's die(_('%s' …))
family owns a quoted space-bearing payload whole) plus a slash-
carrying fragment arm for apostrophe-bearing paths.
- R14-4: tracking-ref ownership skips refs inside a SURVIVING remote's
fetch-dest namespace (NUL-framed get-regexp read), on sweep and
re-verify alike; closes the flat-dest slashed-branch residual.
- R14-5: the restore's presence read is NUL-framed, so an empty value
reads as present instead of absent (no more doubled pushDefault).
- R14-6: isSectionlessUpstream narrows to `.`/colon-bearing values;
dangling slashed/backslash upstreams now refuse like their bare-word
twin; win32 UNC keeps a carve (network transport, offline-share
timeout); existing paths still certify through the path probe.
- R14-7: the post-refusal branch/status refresh runs in the same
synchronous block as the staleness guard, ahead of the awaited
re-read, so a mid-await workspace switch cannot seed the new
workspace's panel with the old closure's data.
- Audit fallout: stale comments corrected (sectionless shapes, flat
dests, probe classes), win32 UNC carve, NUL-framed dest read without
the speculative JSON.parse arm, prettier-clean test files, and EN/ZH
design-doc sync for all six changes plus matrix/Files-affected
attribution of the new skeleton module and its tests.
|
R14 deferred-list dispositions (review 5186199109 recorded 11 items as "not requested in this round"; triaged as maintainer). The six posted Criticals are answered in their threads: fixed in b3aa91e. Fixed in b3aa91e besides the Criticals:
Recorded as follow-up (not landed this round, with reasons):
|
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit round 3 - not run: the review time budget would not cover another round plus compose and submit, 79 reverse-audit candidates from the round 1/2 pair (16 Critical, 63 Suggestion) - never verified: reported terminal-only, they are not confirmed blockers, test-efficacy probe - unmeasured: all 15 probes returned inconclusive (no-output) and the positive control never ran (harnessValidated null), so inert / mutant-survived / hunk-survived are unprobed scope, not clean results, the 83 blocker threads from rounds 1-13 were not individually re-ruled - the round-14 machine ledger (6 entries) is the authoritative cumulative work list and every entry in it was ruled — the agent returned no evidence of its walk twice.
Not explored to full depth (tool budget reached): chunk 23: none — the mid-wildcard repro, the worktree-scope repro and the git-behaviour probes were all executed; nothing was cut short. I did not run the package's vites…; "agent reverse-audit (round 1)": executing the new Playwright spec (tests 2-5 in particular) against a built web-shell — npm ci + npm run build + Chromium install exceeds the remaining budg…; "agent reverse-audit (round 1)": I did not verify the §1 prose in this chunk describing the sibling-worktree sweep, the merge-key-only-with-remote-key rule, and the survivor/unmask gates ( swee…; "agent reverse-audit (round 2)": end-to-end dynamic confirmation of the sibling-override sequence (running packages/core 's git-remotes.test.ts:1004 with the added non-destruction assertions…; "agent reverse-audit (round 1)": a full word-by-word bilingual read of the six ~5-6 KB coverage-table cells (zh:134-139 vs EN:707-712) — I compared them by backtick-token set, separator counts,…, and 1 more.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/serve/routes/workspace-git-remotes.test.ts:79 — [test] Critical [fails-closed] [new-surface] D15-1: The global-scope hermeticity precondition matches *any* remote.* key, while the invariant its own comment names — "an amb…docs/design/git-manage-remotes.md:295 — [probe] D15-2: The doc states the sectionless-value short-circuit set as exhaustive ( ONLY for the shapes … (the local-repository '.', URLs, scp-like 'host:path') ) and then asserts the complement — "…docs/design/git-manage-remotes.md:705 — [probe] D15-3: The coverage table puts a whole file's enumeration in one Markdown cell, so prettier pads all 9 rows to the widest cell — every line of the table is now exactly 6682 characters, and any…docs/design/git-manage-remotes.zh-CN.md:62 — [review] D15-4: The zh §1 predicate bullets drifted from their English source: the slashed-name rationale is translated into an ungrammatical fragment that drops the reason, the removal bullet lo…packages/cli/src/serve/routes/workspace-git-branches.test.ts:617 — [review] D15-5: One it() (~445 lines) bundles roughly 25 independent redaction scenarios on a single shared temp fixture that one scenario mutates in place, so they are or…packages/core/src/utils/git-branches.ts:125 — [review] D15-6: The shared gitEnv now strips ext / fd out of an operator-supplied GIT_ALLOW_PROTOCOL for **every** git consumer in the product — not the remotes feature — while preserving …packages/core/src/utils/git-remotes.test.ts:958 — [review] D15-7: The sibling-worktree fixture (derive the -wt path, register it for teardown, enable extensions.worktreeConfig , git worktree add ) is copy-pasted 26 times inside this chu…packages/core/src/utils/git-remotes.ts:985 — [review] D15-11: removeWorktreeScopeSection 's catch { return false; } — the completion spawn itself failing — is never exercised: every worktree-scope test succeeds ( completes removal of a w…packages/core/src/utils/git-remotes.ts:1794 — [review] D15-9: The win32 UNC short-circuit in isSectionlessUpstream is the only branch of the unmask/converge gate that no test reaches on any platform — nothing in git-remotes.test.ts or …packages/core/src/utils/git-remotes.ts:1803 — [review] D15-10: On the certify path this gate is a strict subset of survivingUpstreamKeys , so it can never change the accept/refuse outcome — its only effect is to throw *before* sweepSiblin…packages/web-shell/client/components/BranchPickerPopover.test.tsx:1888 — [review] D15-12: Only the *name* arm of the local dash guard is witnessed; the url.startsWith('-') arm has no test anywhere in the file (the only dash‑prefixed fixtu…packages/web-shell/client/components/BranchPickerPopover.test.tsx:3008 — [review] D15-13: The comment states the opposite of the assertion three lines below it: it says pushl "stays plain", while the test asserts pushl *does* carry (lo…packages/web-shell/client/components/BranchPickerPopover.tsx:50 — [review] D15-20: The 600 s ceiling is justified by counting only the **add** chain (5 spawns) but the same exported constant also gates the **remove** chain, whose worst-case…packages/web-shell/client/components/BranchPickerPopover.tsx:556 — [review] D15-21: The cross-workspace clear of the held sticky-warning snapshot has no witness: the five sticky-warning tests ( BranchPickerPopover.test.tsx:2056 , :2084 , …packages/web-shell/client/components/BranchPickerPopover.tsx:1114 — [review] D15-14: The removal's chip refresh is a no-op for the composer-branch-chip instance: ChatEditor.tsx:3318-3327 renders <BranchPickerPopover open onOpenChange wor…packages/web-shell/client/components/BranchPickerPopover.tsx:1923 — [review] D15-15: The two new busyAction values ( remoteAdd , remoteRemove ) render an indicator only inside RemotesView , so a mutation still in flight leaves the branc…packages/web-shell/client/components/BranchPickerPopover.tsx:1976 — [review] D15-17: The arm fires for ANY Latin + non‑Latin mix, not the "Cyrillic‑ о homoglyph shape" its comment names, so legitimate names in scripts that have no Latin co…packages/web-shell/client/components/BranchPickerPopover.tsx:1986 — [review] D15-18: The collision lookup folds the RAW name, so a name whose ink differs from its raw spelling (an invisible character, or edge/repeated whitespace CSS collaps…packages/web-shell/client/components/BranchPickerPopover.tsx:2033 — [review] D15-19: The marker is concatenated into the name text inside the same span that carries min-width: 0; overflow: hidden; text-overflow: ellipsis ( .remoteName , B…packages/web-shell/client/utils/remote-name-skeleton.test.ts:24 — [review] D15-23: No test in either new file exercises the NFKC compatibility fallback with a char the table does not list, so the branch remote-name-skeleton.ts:36-38 exist…- …and 2 more (see the run report)
Convergence: round 15 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 6 (6 new). Findings keep coming back to the same files: packages/core/src/utils/git-remotes.ts (findings in round 14; 2 more now); packages/web-shell/client/components/BranchPickerPopover.tsx (findings in round 14; 1 more now); packages/web-shell/client/utils/remote-name-skeleton.ts (findings in round 14; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit round 3 - not run: the review time budget would not cover another round plus compose and submit、79 reverse-audit candidates from the round 1/2 pair (16 Critical, 63 Suggestion) - never verified: reported terminal-only, they are not confirmed blockers、test-efficacy probe - unmeasured: all 15 probes returned inconclusive (no-output) and the positive control never ran (harnessValidated null), so inert / mutant-survived / hunk-survived are unprobed scope, not clean results、the 83 blocker threads from rounds 1-13 were not individually re-ruled - the round-14 machine ledger (6 entries) is the authoritative cumulative work list and every entry in it was ruled——该 agent 连续两次未返回任何检查过程的证据。
未探索到全部深度（达到工具调用预算）：chunk 23：none — the mid-wildcard repro, the worktree-scope repro and the git-behaviour probes were all executed; nothing was cut short. I did not run the package's vites…；"agent reverse-audit (round 1)"：executing the new Playwright spec (tests 2-5 in particular) against a built web-shell — npm ci + npm run build + Chromium install exceeds the remaining budg…；"agent reverse-audit (round 1)"：I did not verify the §1 prose in this chunk describing the sibling-worktree sweep, the merge-key-only-with-remote-key rule, and the survivor/unmask gates ( swee…；"agent reverse-audit (round 2)"：end-to-end dynamic confirmation of the sibling-override sequence (running packages/core 's git-remotes.test.ts:1004 with the added non-destruction assertions…；"agent reverse-audit (round 1)"：a full word-by-word bilingual read of the six ~5-6 KB coverage-table cells (zh:134-139 vs EN:707-712) — I compared them by backtick-token set, separator counts,…，另有 1 条。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 22 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 15 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 6 条（其中 6 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/git-remotes.ts（第 14 轮已出过发现，本轮又有 2 条）；packages/web-shell/client/components/BranchPickerPopover.tsx（第 14 轮已出过发现，本轮又有 1 条）；packages/web-shell/client/utils/remote-name-skeleton.ts（第 14 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
…at/git-manage-remotes
…lout - R15-1: foreign-namespace roots derived per wildcard position; mid- wildcard dests (refs/remotes/*/main, or*/main) fail closed. The reverse audit caught an unsatisfiable star-position conjunction that disabled the orphan sweep in any repo with another default-refspec remote; fixed and pinned by an origin + refspec-less orphan witness. - R15-2: the sibling-worktree sweep verifies the sibling shares this repository's common dir before any read or write, so a planted .git/worktrees gitdir can no longer receive its unset writes; planted-gitdir witness, real-sibling tests unchanged. - R15-3: the remotes search folds the needle in three case forms (the TR39 table is case-sensitive); Istanbul/lstanbul witness. - R15-4: the fold is invariant under canonical equivalence — two-level decomposition fallback at runtime, generator closure over NFD(key), table regenerated (6564 entries, U+021A dropped, 27 values re-classed, lunate sigma preserved); full invariance sweep pinned. - Deferred-triage fixes: hermeticity precondition regex, win32 UNC short-circuit witness, worktree-completion kill witness, dash-URL client-guard witness, NFKC-fallback pin, timeout-ceiling and mixed-scripts comment accuracy, zh predicate-bullet and UNC doc parity, pushl marker-comment accuracy.
|
R15 deferred-list dispositions (review 5189545189 recorded 21 items; triaged as maintainer). The four posted Criticals are answered in their threads: fixed in 5f3752c. Fixed in 5f3752c besides the Criticals:
Declined with reasons:
Recorded as follow-up (not landed this round):
|
- The sibling sweep skips the invoking worktree BEFORE the ownership probes, so a repo whose only worktree record is its own pays no probe spawn (just the one lazy toplevel read, reused per sibling). - The killed-sibling witness is re-aimed at the new spawn order and pins the kill to the sibling's config.worktree read; under the old queue the kill landed on an ownership probe while the test passed. - The remotes search dedupes its case-folded needles and folds each row's ink once instead of once per needle variant. - EN design doc: two stranded fragments repaired; ZH counterpart gets the missing subject in the marker sentence (parity).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D16-1 shared
gitEnvrewrite of an inherited GIT_ALLOW_PROTOCOL affects every git and gh spawn — already reported (D15-6, PR context line 2124)
Unresolved, please confirm:
- [Critical] R15-1
packages/core/src/utils/git-remotes.ts:1295(remoteTrackingRefsclassifying a surviving remote fetch-dest namespace as foreign) — could not reach a verdict: the code now carries an explicit guard whose comment names this exact sub… - [Critical] R15-2
packages/core/src/utils/git-remotes.ts:1441(sweepSiblingWorktreeKeystaking each sibling path fromgit worktree list --porcelain) — could not reach a verdict: the sweep now uses-zfield splitting with an explicit newline-saf…
Not explored to full depth (tool budget reached): chunk 3: none — no check was cut short by the tool ceiling.; "agent reverse-audit (round 2)": whether BranchPickerPopover.tsx validates the payload v field — I could not check, so finding 3 reports the v: 2 fixture as unchecked contract drift rathe…; "agent reverse-audit (round 1)": exhaustive witness-by-witness cross-check of the three multi-thousand-character §6 coverage rows ( git-remotes.test.ts , workspace-git-remotes.test.ts , Branc…; "agent reverse-audit (round 2)": reading survivingUpstreamKeys / sweptUpstreamResolving / isSectionlessUpstream ( git-remotes.ts ~1830-1916) against the doc's "resolves each branch.<b>.…; "agent reverse-audit (round 2)": verifying the main-path merge-pairing rule ("merge only when the same scope's remote key holds no entry naming a surviving remote") inside unsetUpstreamKeys —…, and 8 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:
docs/design/git-manage-remotes.md:35 — [review] Three of the five line pointers in the new "Current state"docs/design/git-manage-remotes.md:169 — [review] The doc states two incompatible behaviours for git configdocs/design/git-manage-remotes.zh-CN.md:177 — [review] The two language versions of the design doc's Security notpackages/cli/src/serve/routes/workspace-git-branches.test.ts:401 — [review] The new block exists so the file "can go red when the tablpackages/cli/src/serve/routes/workspace-git-branches.test.ts:771 — [review] The new 230-line redaction test creates a directory symlinpackages/cli/src/serve/routes/workspace-git-branches.ts:100 — [probe] Redaction runs over the FULL detail before the 512-char slpackages/cli/src/serve/routes/workspace-git-branches.ts:154 — [review] The fail-closed sweep is whitespace-token bounded, so an apackages/cli/src/serve/routes/workspace-git-branches.ts:212 — [review] A dir key is pushed without a minimum-length guard, so apackages/cli/src/serve/routes/workspace-git-remotes.test.ts:79 — [review] The ambient-global precondition is thrown from a file-widepackages/core/src/utils/git-remotes-kill.test.ts:12 — [review] The single module-scope runGit mock is never reset betwepackages/core/src/utils/git-remotes-kill.test.ts:696 — [review] Every kill fixture in this new suite sets killed: true npackages/core/src/utils/git-remotes.ts:1205 — [probe] The value-matched ( fixedValue ) sweep of remote.pushDefapackages/core/src/utils/git-remotes.ts:1581 — [probe] The re-verify matches branch keys by endsWith('.remote')/packages/core/src/utils/git-remotes.ts:1841 — [probe] isSectionlessUpstream classifies *every* colon-bearing upackages/web-shell/client/components/BranchPickerPopover.module.css:465 — [probe] The unarmed Remove control — the first click of a two-clicpackages/web-shell/client/components/BranchPickerPopover.tsx:881 — [probe] The mutation error taxonomy classifies "answer not receivepackages/web-shell/client/components/BranchPickerPopover.tsx:1013 — [probe] The add form's local-validation guard returns *before* sepackages/web-shell/client/utils/unicodeConfusables.test.ts:12 — [probe] Every guard on the committed 6564-entry confusables tablepackages/web-shell/scripts/generate-confusables.mjs:227 — [review] The entry-count floor is a fixed 5000 against a 6564-entrypackages/web-shell/client/components/BranchPickerPopover.module.css:511 — [review] .addRemoteButton and its two pseudo-state blocks are a v- …and 3 more (see the run report)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R16-1 packages/core/src/utils/git-remotes.ts:817 — The removed-name certification's resolver leg is fetch-side only, so a removal whose name a url.<base>.pushInsteadOf alias still resolves push-side is certified as "no longer resolves AT ALL" — the push twin of the insteadOf case the same gate refuses, and the module already owns the helper that answers it (pushInsteadOfAliases, :881).
[Critical] R15-4 packages/web-shell/client/utils/remote-name-skeleton.ts:36 — The fold is not a function of the canonical equivalence class of its input — it decides on the arrival spelling, so two remote names that render identically get different skeletons, splitting one ink class into two collision groups. The file's own contract ("ink-identical names share a skeleton whichever spelling they arrive in") and the generator's equation value === runtimeFold(key.normalize('NFD')) (packages/web-shell/scripts/generate-confusables.mjs:118) hold only for keys in isolation; the closing NFC recomposes across a key boundary and defeats them.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 2 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）：chunk 3：none — no check was cut short by the tool ceiling.；"agent reverse-audit (round 2)"：whether BranchPickerPopover.tsx validates the payload v field — I could not check, so finding 3 reports the v: 2 fixture as unchecked contract drift rathe…；"agent reverse-audit (round 1)"：exhaustive witness-by-witness cross-check of the three multi-thousand-character §6 coverage rows ( git-remotes.test.ts , workspace-git-remotes.test.ts , Branc…；"agent reverse-audit (round 2)"：reading survivingUpstreamKeys / sweptUpstreamResolving / isSectionlessUpstream ( git-remotes.ts ~1830-1916) against the doc's "resolves each branch.<b>.…；"agent reverse-audit (round 2)"：verifying the main-path merge-pairing rule ("merge only when the same scope's remote key holds no entry naming a surviving remote") inside unsetUpstreamKeys —…，另有 8 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未审查：验证——它的 prompt 已构建，但没有 agent 用它启动，发布的发现不能算作已验证。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
收敛姿态下延后（第 16 轮，非阻断）——已记录，本轮不要求修改：共 23 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R16-1 packages/core/src/utils/git-remotes.ts:817 — The removed-name certification's resolver leg is fetch-side only, so a removal whose name a url.<base>.pushInsteadOf alias still resolves push-side is certified as "no longer resolves AT ALL" — the push twin of the insteadOf case the same gate refuses, and the module already owns the helper that answers it (pushInsteadOfAliases, :881).
[Critical] R15-4 packages/web-shell/client/utils/remote-name-skeleton.ts:36 — The fold is not a function of the canonical equivalence class of its input — it decides on the arrival spelling, so two remote names that render identically get different skeletons, splitting one ink class into two collision groups. The file's own contract ("ink-identical names share a skeleton whichever spelling they arrive in") and the generator's equation value === runtimeFold(key.normalize('NFD')) (packages/web-shell/scripts/generate-confusables.mjs:118) hold only for keys in isolation; the closing NFC recomposes across a key boundary and defeats them.
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 287 passed · 0 failed · 287 total Flakiness gate: ✅ 10 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：287 通过 · 0 失败 · 287 总计 抖动门：✅ 10 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11163 deep verification —
|
| # | shape | what git was asked to confirm | result |
|---|---|---|---|
| S1 | plain single origin | name set, fetch/push URL, defaults | ✅ 8/8 |
| S2 | multiple remotes + a URL-less section | git remote lists it; fetchUrl/pushUrl empty |
✅ 4/4 |
| S3 | pushurl override; multi-url push fan-out | git remote -v reports 2 push lines |
✅ 7/7 |
| S4 | partial clone (--filter=blob:none) + push override |
promisor / partialclonefilter from git config | ✅ 6/6 |
| S5 | insteadOf rewriting |
listing returns the raw configured URL, not the rewrite | ✅ 4/4 |
| S6 | inherited global remote | git lists it; the panel excludes it; scope really is global |
✅ 3/3 |
| S7 | linked worktree, worktree-scope remote | listed from the worktree, absent from the main repo | ✅ 3/3 |
| S8 | custom refspec + proxy/tagOpt | customRefspec, otherSettings counts |
✅ 3/3 |
| S9 | bidi/invisible name & URL; exec vectors | raw in the listing, gated on add, lenient on remove | ✅ 12/12 |
| S10 | non-repository | git's canonical not-a-git-repository text surfaces | ✅ 2/2 |
| S11 | add round-trip | git agrees the remote exists; one round trip carries the list | ✅ 5/5 |
| S12 | duplicate add | git's own remote origin already exists; URL untouched |
✅ 3/3 |
| S13 | remove round-trip | no remote.* record in any scope, upstream keys swept, orphan refs swept, sibling survived |
✅ 9/9 |
| S14 | remove a missing remote (the panel race) | git's No such remote; nothing else destroyed |
✅ 3/3 |
| S15 | worktree-scope section | bare git remote remove fails; the helper completes it |
✅ 6/6 |
| S16 | inherited same-name section | add and remove refuse; nothing mutated | ✅ 5/5 |
| S17 | refspec-less remote | orphaned refs/remotes/origin/* swept |
✅ 5/5 |
| S18 | unparsable refspec | refuses; the row survived (git dies before mutating) | ✅ 3/3 |
| S19 | remote named after a classifier sentinel | listed raw; removal succeeds (no sentinel collision) | ✅ 3/3 |
| S20 | core error hygiene | no config dump on stdout | ✅ 2/2 |
97/97. The two shapes that most directly test the PR's "removal is verified, never trusted" claim are S15 and S16: in S15 bare git leaves the worktree-scope section behind after already destroying the tracking refs, and the helper completes the removal so that git config --show-scope finds nothing in any scope; in S16 the inherited-shadow pre-flight runs before any mutation, and the local half is confirmed still present after the refusal.
Classifier A/B — the shared error path (secondary claims 1 & 2)
h2-classifier-ab.mjs runs the identical 49-entry corpus through two esbuild bundles of workspace-git-branches.ts:
- base =
git show HEAD^1:…/workspace-git-branches.ts, withexportadded tosendGitErroras the only edit. - head = the working-tree file, bundled identically.
Both arms resolve @qwen-code/qwen-code-core to the same compiled head build. That is a clean control for this function because findGitRoot lives in gitUtils.ts, which the PR does not touch (git diff --stat HEAD^1..HEAD -- packages/core/src/utils/gitUtils.ts is empty), and GIT_ERROR_MESSAGE_MAX = 512 is identical on both sides. readlink -f node_modules/@qwen-code/qwen-code-core → /__w/qwen-code/qwen-code/packages/core (asserted, quoted here per the workspace-link rule).
Corpus: 25 entries captured from real failing git commands in real temp repos, plus 22 clearly-labelled synthesized shapes for payloads git cannot be coaxed into emitting (a server's push sideband, Windows/UNC spellings, deeper-line injection).
| class | what it asserts | head | control (base) |
|---|---|---|---|
| R (15 entries) | pre-existing route shapes classify identically | 15/15 unchanged | — |
| N (8 entries) | remote shapes reach the correct new code, and base did not already answer it | 16/16 | base wrong on all 8 |
| H (11 entries) | a config-chosen name / deeper line cannot claim the wrong code | 21/21 | base fooled on 6 |
| L (49 entries) | no absolute filesystem path survives | 0/49 leaked | 11/49 leaked |
117/117. Witness: 01-classifier-ab-base-vs-head.png.
The 16 classification deltas, all in the intended direction:
N-add-duplicate 409/branch_already_exists -> 409/remote_already_exists
N-remove-missing 500/__bridge__ -> 404/no_such_remote
N-worktree-section 500/__bridge__ -> 409/git_config_write_failed
N-config-lock-chain 500/__bridge__ -> 409/git_config_write_failed
N-invalid-refspec 500/__bridge__ -> 409/remote_config_unparsable
N-sentinel-still-configured 500/__bridge__ -> 409/remote_still_configured
N-sentinel-included-file 500/__bridge__ -> 409/remote_section_in_included_file
N-sentinel-inherited 500/__bridge__ -> 409/remote_shadows_inherited
H-name-dirty-remove-missing 409/dirty_working_tree -> 404/no_such_remote
H-name-dirty-dup-add 409/dirty_working_tree -> 409/remote_already_exists
H-name-not-a-git-repo 404/not_a_git_repository -> 404/no_such_remote
H-name-nothing-to-commit 400/nothing_to_commit -> 404/no_such_remote
H-name-detached-head 409/detached_head -> 404/no_such_remote
H-name-already-exists 409/branch_already_exists -> 404/no_such_remote
H-newline-injection 500/__bridge__ -> 409/remote_config_unparsable
L-linked-worktree-gitdir 500/__bridge__ -> 409/git_config_write_failed
The six H-name-* cells are the sharpest: a remote named after a legacy keyword makes git emit error: No such remote: '<name>', and base's unanchored keyword scan claims the name's word instead of git's own line-initial prefix. H-name-nothing-to-commit is not merely a wrong code — base answers HTTP 400 where 404 is correct. These are real git outputs, not synthesized.
H-newline-injection is a real git-emitted injection: a config-held fetch refspec carrying \n makes git unescape it into a deeper line reading fatal: No such remote: 'spoofed'. Head anchors to line 1 and answers remote_config_unparsable; the deeper line is not claimed.
Redaction: 11 base leaks → 0 at head
The sharpest single cell is L-linked-worktree-gitdir. From a linked worktree, git echoes the main repository's .git/config — a path outside the requesting workspace's tree, which the pre-existing cwd/gitRoot substitution cannot reach:
base: error: could not lock config file /tmp/h2-…/r14-mainwt/.git/config: File exists
head: error: could not lock config file <workspace>/config: File exists
The other ten base leaks are the ~/.gitconfig, $XDG_CONFIG_HOME/git/config, */etc/gitconfig, include.path-target, Windows-drive, UNC, submodule-gitdir and quoted-spacey-path shapes. Head redacts all eleven and still preserves a transport URL verbatim (L-url-survives).
Scaling of the redactor (the PR's own O(L²) claim)
h3-redactor-scaling.mjs drives the real exported sendGitError — so the measurement includes redactGitPaths — over 10 hostile payload families at 2 k / 3 k / 5 k / 20 k / 100 k characters. Untrusted writers reach this path: a malicious remote's push/fetch sideband (the code comment itself notes it "bypasses git's vreportf cap"), and config-chosen names/URLs from a repo the user did not author.
Every family is linear within noise (ratio 10.0×–28.2× for a 20× size increase). Worst single error response: 8.2 ms. The adversarial arm-7 shape (many <config>' starts with no closing quote, forcing a full scan per start) measured 13.7×. A realistic 123 041-char sideband payload cost 0.5 ms. 12/12. The per-token /etc/gitconfig decision the comment says it chose to avoid O(L²) holds up.
gitEnv() — a second shared-surface change
The PR also filters GIT_ALLOW_PROTOCOL in gitEnv(), which feeds every git invocation in core, not just the new routes. h4-allow-protocol-ab.mjs A/Bs base vs head gitEnv and then asks git what it actually does, using a repo whose .git/config pairs url = ext::<helper> with protocol.ext.allow = always — the workspace-controlled escalation the comment names. Two independent witnesses for "the helper executed": git echoing the helper's stdout, and a canary file the helper touches.
git's semantics, measured first (the oracle both arms must respect):
GIT_ALLOW_PROTOCOL |
protocol.ext.allow = always in repo config |
ext helper |
|---|---|---|
| unset | — | executes |
"" (set, empty) |
— | blocked |
https:ssh (ext absent) |
— | blocked |
So the variable is a strict allowlist: absence blocks even when the workspace config says always.
| case | base env | head env | base: helper ran | head: helper ran |
|---|---|---|---|---|
| operator allows ext explicitly | "https:ssh:ext" |
"https:ssh" |
true | false |
| operator allows only ext | "ext" |
"" |
true | false |
| operator allows ext+fd only | "ext:fd" |
"" |
true | false |
| operator list without ext | "https:ssh" |
"https:ssh" |
false | false |
operator allows arbitrary helper 7z |
"https:7z" |
"https:7z" |
false | false |
17/17. The strip is load-bearing and strictly safer, and the code comment's claim that an arbitrary operator-listed helper name is preserved is confirmed (7z untouched). All pre-existing env hardening survives at both arms (14 repo-selector / config-injection vars still cleared; LC_ALL/LANG still pinned to C).
Bounded sharp edge, fail-closed: an operator whose list is only helper names (ext:fd) now gets "", which is deny-all — so https is refused too (fatal: transport 'https' not allowed). This breaks functionality rather than opening a hole, needs an unusual operator configuration to trigger, and the code comment states the deny-all choice deliberately. Reported as an observation, not a defect.
Mutation matrix on the shared classifier
mutate-classifier.py applies 10 single-point mutants to workspace-git-branches.ts and runs the collocated suite (src/serve/routes/workspace-git-branches.test.ts) for each, restoring byte-identically afterwards (restore check: source identical to original = True; git status --porcelain -- packages/cli/src empty).
Witness: 02-mutation-matrix-7-of-10-killed.png.
| # | mutation | suite | classification |
|---|---|---|---|
| — | CONTROL, unmutated | 69/69 green | the kills below mean something |
| M1 | delete the No such remote branch |
🔴 11 failed | killed |
| M2 | un-anchor No such remote (drop ^) — positive control |
🔴 1 failed | killed |
| M3 | remove the fail-closed absolute-path sweep | 🔴 1 failed | killed |
| M4 | disable external-gitdir redaction (worktree/submodule) | 🔴 1 failed | killed |
| M5 | revert detail composition to always-join stdout\nstderr |
🔴 26 failed | killed |
| M6 | classify the loose keyword arms on the unbounded detail | 🟢 69/69 | survived → coverage gap |
| M7 | drop the two-line lock-chain arm | 🔴 6 failed | killed |
| M8 | un-anchor remote … already exists and drop $ |
🟢 69/69 | survived → coverage gap |
| M9 | drop the ~/.gitconfig double-slash spelling |
🔴 1 failed | killed |
| M10 | replace the per-token /etc/gitconfig arm with a whole-message \S* regex |
🟢 69/69 | survived → equivalent mutant |
M2 is the positive control the method requires: it is a single-character deletion of the security property, it landed in the same file as the mutants, and it turned exactly one test red — so "the suite does not cover M6/M8" and "my harness never ran the suite" are distinguishable.
Both survivors were then proven observable rather than left as impressions (h6-m6-and-homoglyphs.mjs, part (a)):
| payload | head | mutant | observable? |
|---|---|---|---|
not a git repository past the 512 cap |
500/__bridge__ |
M6 → 404/not_a_git_repository |
yes |
invalid reference past the 512 cap |
500/__bridge__ |
M6 → 404/not_a_git_repository |
yes |
benign line 1 + error: remote foo already exists. on line 2 |
409/branch_already_exists |
M8 → 409/remote_already_exists |
yes |
So M6 and M8 are genuine coverage gaps, not dead code. M10 is classified as an equivalent mutant: the .test() guard I added to it makes the replacement near-semantically-identical, so the row shows only that the per-token form is not pinned. The O(L²) property itself is measured linear by H3 but has no test — a regression to the quadratic form would pass the suite.
Display sanitizer + TR39 skeleton — sibling sweep
The PR's stated threat model is "a .git/config the user did not author (downloaded zip, cloned repo) can carry bidi/zero-width characters in remote names and URLs", closed by two mechanisms: a property arm that strips the Default_Ignorable set, and a TR39 confusables skeleton that flags ink-identical twins. Per the sibling-sweep rule, every shape was asked of both.
h5-sanitizer-sweep.mjs drives the real client sources bundled with esbuild. 33/33.
- Property arm: 25/28 probed zero-ink shapes are stripped, including every one the PR names (U+202E RLO, U+200F RLM) plus U+200B ZWSP (measured
gc=Cf, so\p{Cf}catches it — worth stating because it is not Default_Ignorable-by-itself in every mental model), the whole Tag block, variation selectors, Hangul fillers, and U+2028/U+2029. - Committed generated table vs its generator's documented invariants: all 6 564 keys are single code points (the "one
Map.getper code point" claim); the prototype closure is fully resolved (no value character is itself a key, so the runtime really is single-pass); every key settles under the 8-pass cap with a measured maximum of 2 outer passes (the comment claims ≤2 — confirmed); no self-entries; the file recordsconfusables.txt 17.0.0, is marked do-not-edit, names its generator, and carries the Unicode License V3 attribution. - Search invariant: for every name probed, the displayed text is findable by searching it, and typing the visible characters finds a bidi-laced name.
h6-m6-and-homoglyphs.mjs part (b) then replicates the component's row-marking predicate and calibrates it before trusting it: the replication reproduces the ZWSP twin marking, the whitespace-only twin marking, a lone plain-ASCII remote not marking, three ordinary distinct remotes marking nothing, and the Cyrillic-о homoglyph (the comment's own motivating shape) marking. Witness: 03-homoglyph-sweep-two-gaps.png.
Calibrated, 9 ink-identical pairs were driven through it. 7 mark; 2 do not — see Findings 1 and 2.
Findings
None is blocking. 1 is the sharpest; all four are in the PR's own stated scope.
1. 0rigin beside origin marks neither row — the skeleton fold has no casefold step (Suggestion)
The classic digit-zero / lowercase-o homoglyph sails through, while its siblings 1ist/list, Iink/link and |ink/link are all correctly flagged.
Reproduce (h6-m6-and-homoglyphs.mjs, calibrated replication of the component predicate):
NOT MARKED digit zero vs lowercase o [["origin",false],["0rigin",false]]
skeletons: ["origin","Origin"]
MARKED digit one vs lowercase L [["list",true],["1ist",true]]
skeletons: ["list","list"]
Cause, measured at the table level: remoteNameSkeleton('0rigin') === 'Origin' while remoteNameSkeleton('origin') === 'origin'. confusables.txt folds 0 to the capital O, and remoteNameSkeleton never case-folds, so the two land in different skeletonGroups keys — group.count stays 1 and skeletonCollision is false. No other arm fires either: 0rigin is all-ASCII (so mixedScripts is false, 0 being Script=Common), it sanitizes to itself, and it is NFC-stable. 1→l, I→l and |→l work only because their prototype is the lowercase l, matching the ASCII letter's own prototype.
TR39's skeleton algorithm is NFD → toCasefold → map confusables; the middle step is what makes 0rigin and origin collide. Impact is bounded: this is a display-layer defense-in-depth control, and removal requests still carry the raw configured name, so nothing is silently mis-deleted — but a user staring at two rows reading origin has no signal that one is a digit zero, which is exactly the outcome the panel's marking exists to prevent.
Minimal suggested fix (not measured against the suite — see caveat)
Compare skeletons case-insensitively for grouping while keeping the existing raw-vs-skeleton polarity for deciding which row marks:
const skeleton = remoteNameSkeleton(r.name);
const group = skeletonGroups.get(skeleton.toLowerCase());keying the map on skeleton.toLowerCase() in the same way.
Caveat, stated rather than hidden: I did not apply and drive this through the suite within budget. It has a real tradeoff — git remote names are case-sensitive, so Origin and origin would begin to collide and both would mark. Given the source's own stated preference ("the arm is script-mixing generally, in the conservative direction"), that is arguably the intended polarity, but the author should decide, and the change needs its own fixture: remoteNameSkeleton('0rigin').toLowerCase() === remoteNameSkeleton('origin').toLowerCase().
2. U+FFFC survives both mechanisms (Nice to have)
OBJECT REPLACEMENT CHARACTER is neither Cc/Cf/Default_Ignorable (measured: gc=So, DICP=false) nor present in confusables.txt, and it is Script=Common, so the mixedScripts arm does not fire either.
NOT MARKED object replacement char [["origin",false],["origin",false]]
skeletons: ["origin","origin"]
Contrast U+2800 BRAILLE PATTERN BLANK, which is also stripped by neither mechanism but is correctly marked — because it is Script=Braille, so mixedScripts catches it. That contrast is the useful part: the script-mixing arm is doing real work, and U+FFFC is the corner it cannot reach. Lower severity than Finding 1 because most fonts render U+FFFC as a visible placeholder box rather than as nothing, so the two rows do not actually present one identity.
3. "Loose keyword branches match the bounded slice" is documented but unpinned (Suggestion)
The source comment states the property explicitly ("an unbounded keyword scan reclassifies a long push/pull output by any path or URL past the cap"), and mutant M6 — switching those two arms from message to fullMessage — survives 69/69. H6 proves it is observable: a payload carrying not a git repository past char 512 flips 500/__bridge__ → 404/not_a_git_repository. The fixture that would go red is exactly that payload; it belongs beside the existing long-lock-line case in the classification table.
4. The remote … already exists arm's line-1 anchoring is unpinned (Suggestion)
Mutant M8 (drop ^ and $) survives 69/69, yet H6 shows a deeper-line injection flipping 409/branch_already_exists → 409/remote_already_exists. The suite pins the anchoring for No such remote (M2 killed) and for the lock chain (M7 killed) but not for this arm. Same class as Finding 3: the property is real and reasoned about in a comment, and nothing would notice its loss.
Corrections to the PR description
Checked, none needed. The description's "six remote-specific branches" is accurate: six arms are remote-specific (remote_already_exists, no_such_remote, remote_still_configured, remote_section_in_included_file, remote_config_unparsable, remote_shadows_inherited) and the seventh (git_config_write_failed) is not remote-specific — it also covers unset 'branch.. Its disclosure that no live-daemon curl pass was run matches what I could reach (see Not covered).
Not covered
- Playwright UI e2e (
web-shell.git-remotes.spec.ts, 5 specs) and the five screenshots in the description. Not run: no browser in this container. The React rendering, the two-click confirm state machine, the panel↔branch-list navigation and the i18n copy are therefore unverified behaviourally; H5/H6 verify the sanitizer and skeleton logic those components call, not the components. - Live-daemon curl pass. Not run. The route layer was verified at the module level (guards read and compared against the sibling branch routes: GET uses the lenient
resolveContainedCwd, both POSTs useresolveContainedCwdOrFail→ 400, exactly matching lines 826/848/881/914/947/980 of the sibling) and by the PR's own supertest suite, which I ran green — but no request crossed a real socket in this round. - Confusables generator re-run.
scripts/generate-confusables.mjsfetcheshttps://www.unicode.org/Public/security/latest/confusables.txt; this sandbox has no network. So the committedunicodeConfusables.tswas not byte-compared against a fresh generation, and I cannot say whether it was hand-edited. I substituted an audit of the generator's own documented invariants against the committed table (H5 section E, 8/8), which is weaker: it would not catch a hand-edit that preserved those invariants. What would have calibrated it: a vendoredconfusables.txt17.0.0 fixture, or network egress. - Per-commit attribution. The snapshot lists 35 commits; the checkout is shallow (
git rev-parse --is-shallow-repository→true) andgit rev-list HEAD^1..HEAD^2returns 1 — the known-plausible number a shallow boundary produces instead of erroring. Only the merge commit, the base tip and the PR head are reachable, so everything here verifies the aggregateHEAD^1..HEADdiff. No per-commit table is presented. - Repo-wide gates. Not run:
npm run lint,npm run typecheck, the full test suites,integration-tests, and theeslint-ignore claim that type-aware rules OOM on the generated table (plausible, unmeasured). Targeted workspace suites only — see below. - Windows / macOS. The description marks both
⚠️ and defers to CI. All measurements here are Linux/git 2.39.5. The Windows-drive and UNC redaction shapes were exercised only as synthesized payloads, never against real Windows git. - Base-side dependency parity. The PR touches no
package.jsonor lockfile, so reusing the rootnode_modulesfor the base arm is a clean control; no dependency-aware A/B was needed. docs/design/git-manage-remotes.md/.zh-CN.mdbilingual completeness: not audited for structural parity beyond noting both files exist (866 and 182 lines — the size asymmetry suggests the Chinese version is a summary rather than a full translation, which per the house rule is a Suggestion at most and was not pursued).
Targeted gates (run, exact counts)
| workspace | command | result |
|---|---|---|
packages/core |
vitest run src/utils/git-remotes.test.ts src/utils/git-remotes-kill.test.ts src/utils/git-branches.test.ts |
3 files, 352 tests passed |
packages/cli |
vitest run src/serve/routes/workspace-git-remotes.test.ts src/serve/routes/workspace-git-branches.test.ts src/serve/process-env-guard.test.ts |
3 files, 108 tests passed |
packages/web-shell |
vitest run client/utils/remote-name-skeleton.test.ts client/utils/unicodeConfusables.test.ts client/components/BranchPickerPopover.test.tsx |
3 files, 136 tests passed |
packages/sdk-typescript |
vitest run test/unit/DaemonClient.test.ts |
1 file, 446 tests passed |
Total 1 042 tests, 0 failures. These are my file selections and are not directly comparable to the counts in the PR description (which names different groupings); no pre-existing failure needed attributing because nothing failed. The mutation-matrix control (69/69 on workspace-git-branches.test.ts) was re-run from a known-clean state immediately before the mutants.
Methodology
Everything ran inside the CI verify container on the merge-ref checkout (HEAD = merge, HEAD^1 = base tip 35a9c5c6, HEAD^2 = head 08f5009a), with npm ci and npm run build already completed by the workflow; I rebuilt nothing except the scratch esbuild bundles named below. Six harnesses (.mjs, kept in this directory so a maintainer can rerun them verbatim) drove compiled or bundled real code against real temporary git repositories and real git subprocesses — no mock of any unit under test. H1 imported packages/core/dist/src/utils/git-remotes.js and took every expectation from an independent git invocation. H2 and H6 used two esbuild bundles of workspace-git-branches.ts (base from git show HEAD^1:, with export added to sendGitError as the sole edit; head from the working tree), both externalizing @qwen-code/qwen-code-core so they share one compiled core — a clean control because findGitRoot lives in the untouched gitUtils.ts, and the workspace symlink realpath was asserted. H4 bundled base and head packages/core/src/utils/git-branches.ts the same way and used a canary file plus git's own stdout echo as two independent witnesses for helper execution. H5 and H6 bundled the real client remote-name-skeleton.ts and unicodeConfusables.ts; H6's replication of the React component's marking predicate was calibrated against five shapes the PR's own suite pins before being trusted with an unpinned one. Every scratch source file was removed and git status --porcelain confirmed the tree clean after each bundle. mutate-classifier.py restored the mutated source byte-identically after every run. Raw per-cell output is in logs/; assertions.json counts only scripted checks that actually executed, and the expected-red base cells are encoded as passing assertions rather than counted as failures.
Flakiness gate log
e2e suite, out of gate scope: packages/web-shell/client/e2e/web-shell.git-mode.spec.ts
e2e suite, out of gate scope: packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts
rounds=5 files=10 skipped=2
file packages/cli/src/serve/process-env-guard.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/process-env-guard.test.ts
file packages/cli/src/serve/routes/workspace-git-branches.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/routes/workspace-git-branches.test.ts
file packages/cli/src/serve/routes/workspace-git-remotes.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/routes/workspace-git-remotes.test.ts
file packages/core/src/utils/git-branches.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/git-branches.test.ts
file packages/core/src/utils/git-remotes-kill.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/git-remotes-kill.test.ts
file packages/core/src/utils/git-remotes.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/git-remotes.test.ts
file packages/sdk-typescript/test/unit/DaemonClient.test.ts: (cd packages/sdk-typescript) npx --no-install vitest run ./test/unit/DaemonClient.test.ts
file packages/web-shell/client/components/BranchPickerPopover.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/BranchPickerPopover.test.tsx
file packages/web-shell/client/utils/remote-name-skeleton.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/utils/remote-name-skeleton.test.ts
file packages/web-shell/client/utils/unicodeConfusables.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/utils/unicodeConfusables.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/process-env-guard.test.ts: PPPPP
packages/cli/src/serve/routes/workspace-git-branches.test.ts: PPPPP
packages/cli/src/serve/routes/workspace-git-remotes.test.ts: PPPPP
packages/core/src/utils/git-branches.test.ts: PPPPP
packages/core/src/utils/git-remotes-kill.test.ts: PPPPP
packages/core/src/utils/git-remotes.test.ts: PPPPP
packages/sdk-typescript/test/unit/DaemonClient.test.ts: PPPPP
packages/web-shell/client/components/BranchPickerPopover.test.tsx: PPPPP
packages/web-shell/client/utils/remote-name-skeleton.test.ts: PPPPP
packages/web-shell/client/utils/unicodeConfusables.test.ts: PPPPP
verdict: pass
summary: 10 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/routes/workspace-git-branches.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/routes/workspace-git-remotes.test.ts: P (exit 0)
round 1 · packages/core/src/utils/git-branches.test.ts: P (exit 0)
round 1 · packages/core/src/utils/git-remotes-kill.test.ts: P (exit 0)
round 1 · packages/core/src/utils/git-remotes.test.ts: P (exit 0)
round 1 · packages/sdk-typescript/test/unit/DaemonClient.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/BranchPickerPopover.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/utils/remote-name-skeleton.test.ts: P (exit 0)
round 1 · packages/web-shell/client/utils/unicodeConfusables.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/routes/workspace-git-branches.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/routes/workspace-git-remotes.test.ts: P (exit 0)
round 2 · packages/core/src/utils/git-branches.test.ts: P (exit 0)
round 2 · packages/core/src/utils/git-remotes-kill.test.ts: P (exit 0)
round 2 · packages/core/src/utils/git-remotes.test.ts: P (exit 0)
round 2 · packages/sdk-typescript/test/unit/DaemonClient.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/BranchPickerPopover.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/utils/remote-name-skeleton.test.ts: P (exit 0)
round 2 · packages/web-shell/client/utils/unicodeConfusables.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/routes/workspace-git-branches.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/routes/workspace-git-remotes.test.ts: P (exit 0)
round 3 · packages/core/src/utils/git-branches.test.ts: P (exit 0)
round 3 · packages/core/src/utils/git-remotes-kill.test.ts: P (exit 0)
round 3 · packages/core/src/utils/git-remotes.test.ts: P (exit 0)
round 3 · packages/sdk-typescript/test/unit/DaemonClient.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/BranchPickerPopover.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/utils/remote-name-skeleton.test.ts: P (exit 0)
round 3 · packages/web-shell/client/utils/unicodeConfusables.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/routes/workspace-git-branches.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/routes/workspace-git-remotes.test.ts: P (exit 0)
round 4 · packages/core/src/utils/git-branches.test.ts: P (exit 0)
round 4 · packages/core/src/utils/git-remotes-kill.test.ts: P (exit 0)
round 4 · packages/core/src/utils/git-remotes.test.ts: P (exit 0)
round 4 · packages/sdk-typescript/test/unit/DaemonClient.test.ts: P (exit 0)
round 4 · packages/web-shell/client/components/BranchPickerPopover.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/utils/remote-name-skeleton.test.ts: P (exit 0)
round 4 · packages/web-shell/client/utils/unicodeConfusables.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/routes/workspace-git-branches.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/routes/workspace-git-remotes.test.ts: P (exit 0)
round 5 · packages/core/src/utils/git-branches.test.ts: P (exit 0)
round 5 · packages/core/src/utils/git-remotes-kill.test.ts: P (exit 0)
round 5 · packages/core/src/utils/git-remotes.test.ts: P (exit 0)
round 5 · packages/sdk-typescript/test/unit/DaemonClient.test.ts: P (exit 0)
round 5 · packages/web-shell/client/components/BranchPickerPopover.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/utils/remote-name-skeleton.test.ts: P (exit 0)
round 5 · packages/web-shell/client/utils/unicodeConfusables.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Round 2 · Live-daemon + real-browser verification on macOS / git 2.55Maintainer pass at head Chain under test: Chromium → PR web-shell → PR SDK → real HTTP → Verdict: merge-ready. 38/38 live checks pass; the two Environment
1. The two findings round 16 could not verdict — settled against real git
Both arms are full rebuilds driven over HTTP; the verdicts are disk reads, not log claims. Genuine siblings keep working in both arms, including one whose path carries a literal newline (§2/F). 2. Namespace ownership and the sibling sweep — 6/6, each against a plain-git control
3. Live daemon over real HTTP — 18/18
4. Inherited (global) scope — 3/3, the shapes where plain git is silently wrong
5. Real browser → real daemon → real
|
| Manage Remotes… in the live picker | The panel, listing the real repository |
|---|---|
 |
 |
Add — verified on disk with git config --get |
Duplicate add: git's own message, nothing mutated |
|---|---|
 |
 |
| Two-click confirm armed (zero requests so far) | A remote deleted behind the panel |
|---|---|
 |
 |
- Panel rows matched
git remoteexactly, before and after every mutation. - The first remove click issued zero HTTP requests and left the repository untouched; the second removed it from
.git/config. - The failed duplicate add left
remote.fork.urlsingle-valued. - The race (a terminal
git remote remove racerbehind the open panel, then confirming its removal) surfaced git's realerror: No such remote: 'racer'and the panel re-read the list instead of keeping an undeletable row.
New since round 1 — the R15-3/R15-4 confusable fold, and it works in the real UI:
Istanbul vs lstanbul: searching one finds both |
A bidi-override name: stripped, flagged, still removable |
|---|---|
 |
 |
- Typing
Istanbulmatched both rows and both carry(lookalike name)— the TR39 fold is case-folded in the UI, not only in unit tests. - The
U+202E-bearing row renders asevliat (hidden characters); the raw code point never reaches the DOM, and the remove request still carried the exact configured name on the wire (65 76 202e 6c 69 61 74), which real git accepted. - zh-CN is complete:
6. Gates re-run locally at this head (macOS, Node 24.18.1, git 2.55.0)
| Suite | Result |
|---|---|
core git-remotes + git-remotes-kill + git-branches |
352/352 |
cli workspace-git-remotes + workspace-git-branches + process-env-guard |
108/108 |
sdk-typescript DaemonClient |
435/435 |
web-shell BranchPickerPopover + remote-name-skeleton + unicodeConfusables |
136/136 |
web-shell e2e web-shell.git-remotes.spec.ts (Playwright) |
5/5 |
Round 1 flagged one git-branches failure caused by git 2.47's wording; on git 2.55 it passes, so that note is retired.
7. The red Windows check is not this PR — and the one PR-owned difference is a timing flake
Test (windows-latest, Node 22.x) on the dispatch run for this head fails. Measured against main's own nightly (run 34713863264, b5c7635ff, same self-hosted Windows runner):
| failures | files | |
|---|---|---|
main nightly |
148 | 47 |
| this head | 149 | 47 |
The file sets are otherwise identical (code-mode 24, scheduler 13, monitor-debug-store 12, git-branches 8, … in both). Only two files differ, one in each direction, and exactly one belongs to this PR:
BranchPickerPopover.test.tsx > resets the remotes view and restores no focus after a workspace switch
That test is byte-identical at b3aa91ebb, where the same Windows lane passed it — so it is not a new defect at this head. Cause, reproduced deterministically on macOS: the popover's own mount-time setTimeout(() => searchRef.current?.focus(), 50) (BranchPickerPopover.tsx:605 — pre-existing on main, untouched by this PR) lands inside the assertion window when the runner spends >50 ms between sampling activeBefore and the settle. Adding one line to the test reproduces the CI message byte-for-byte:
const activeBefore = document.activeElement;
+ await act(async () => { await new Promise((r) => setTimeout(r, 80)); });
await act(async () => { release?.({ v: 1, workspaceCwd: '/repo2', remotes: [] }); });AssertionError: expected <input …(3)></input> to be <body><div>…(2)</div></body>
Moving that same delay above the activeBefore sample turns it green again, and the test passed 20/20 unmodified here. Suggested fix (test-side, one line): flush past the 50 ms open-autofocus before sampling activeBefore. Not a product defect — the popover autofocusing its search box 50 ms after opening is the intended pre-existing behaviour.
8. What is actually gating this merge
- All 178 review threads are resolved (GraphQL, paginated) — zero unresolved.
- The blocking
CHANGES_REQUESTEDis anchored at5ad579c97(2026-09-13 04:33), i.e. two fix batches behind the current head; the bot's only review at08f5009adisCOMMENTED, which carries no vote. - Required lanes at head are green:
Test (ubuntu-latest),Lint & Static,web-shell E2E Smoke,Serve A/B,Real daemon E2E, the Java matrix;Test (macos-latest)passed on the dispatch run. - So the remaining gate is human approval, not code.
9. Non-blocking observations
- N1 (carried from round 1's N3, unchanged). The test plan's item 9 says "
?cwd=escaping the workspace → 400". True for the two mutations; the GET route is deliberately lenient and falls back to the workspace root (measured again here). Wording only. - N2. The mid-wildcard fail-closed rule (§2/C) means a removed remote's own orphan refs are also left behind in such a repo, and the removal still answers 200 — the phantom group stays listed. This is the documented trade (
docs/design/git-manage-remotes.md:344-348, "residue stays as cache — the safe polarity") and I confirmed it behaves as written; noting it only so the doc sentence and the observed behaviour are known to match. - N3. The §7 flake is worth the one-line test fix before the next Windows dispatch, otherwise the lane keeps carrying a red that looks PR-owned.
- N4 — independent confirmation of the sandboxed verification's finding 1, with one refinement. The
qwen-triagesandbox run posted while this pass was underway; its 287/287 result and mine agree, and I re-tested its sharpest item in the live UI.0riginbesideorigin: neither row is flagged, because the skeleton groups by a case-sensitive fold (0folds toO, soskeleton('0rigin') = "Origin"≠skeleton('origin')). The refinement: the search half already handles it — R15-3 folds the needle in three case forms, so typingorigindoes surface0rigin. Only the row-marking path misses the casefold, which narrows the fix toremoteSkeletonGroupsrather than the fold itself.
中文说明
第 2 轮 · 在 macOS / git 2.55 上对 live daemon 与真实浏览器的验证
维护者在 head 08f5009ad 上的验证。第 1 轮在 d1e994f、Linux / git 2.47 / Node 22 上完成；本轮针对此后合入的四个修复批次——R11 c62cecf0、R13 bab8e50b、R14 b3aa91eb、R15 5f3752cd + 08f5009a——换到另一个平台和高得多的 git 版本复验，而配置作用域、worktree 与 ref 机制恰恰最容易在这里出现差异。
受测链路：**Chromium → PR 的 web-shell → PR 的 SDK → 真实 HTTP → qwen serve（由 PR 源码构建）→ 真实 git 2.55.0 → 真实仓库。**无 mock daemon、无 page.route、无夹具；每条断言都用真实 git 回读磁盘。
**结论：可以合入。*38/38 项实测检查通过；第 16 轮评审标记为"无法得出裁决"*的两个 [Critical] 已用实测settled，并通过与 pre-R15 构建的 A/B 证明这两个守卫确实起作用（§1）。唯一的红色检查是 Windows 通道：它有 149 个失败，而 main 自身是 148 个，唯一属于本 PR 的那一条，我在本地用一行代码确定性复现为计时抖动（§7）。
环境
| Head / merge-base | 08f5009ade / ddd6eea9dd |
| 平台 | macOS 26.6.2（Darwin 25.6.0）、Node 24.18.1、git 2.55.0 |
| Daemon | node dist/cli.js serve --port 41631 --workspace <真实仓库> --require-auth，由 PR 树 npm run build && npm run bundle 产出 |
| UI | daemon 自带的 dist/web-shell（生产路径，非 dev server），Chromium 1280×900 @2x，?token=… |
| A/B 臂 | 同一棵树，仅把 packages/core/src/utils/git-remotes.ts 回退到 c1fdad2a（pre-R15），完整重新构建并重新打包 |
1. 第 16 轮无法裁决的两条 Critical —— 用真实 git settled
R15-1 · git-remotes.ts:1295 |
R15-2 · git-remotes.ts:1441 |
|
|---|---|---|
| 问题 | remoteTrackingRefs 是否会把存活 remote 的 fetch 目标命名空间判为 foreign？ |
sweepSiblingWorktreeKeys 会往谁的 .git/worktrees 条目写？ |
| 探针 | remote.keeper.fetch = +refs/heads/*:refs/remotes/*/main（中间带通配的 dest，git 接受），于是 refs/remotes/gone/main 是 keeper 的活引用；而 gone 本身无 refspec |
手工植入 host/.git/worktrees/planted/gitdir → 指向无关仓库；git worktree list --porcelain 确实把它当作 sibling 列出 |
| plain git | 引用存活（没有 refspec 可作用） | victim 不受影响（git 根本不扫 sibling） |
pre-R15 c1fdad2a |
200 —— refs/remotes/gone/main 被删除 |
200 —— victim 的 branch.main.remote 被 unset |
| 当前 head | 200 —— 引用存活（整棵树判为 foreign：fail closed） | 200 —— victim 不受影响（common-dir 校验先于任何读写） |
两臂都是完整重建并经 HTTP 驱动；裁决来自磁盘回读，而不是日志声明。两臂里真实 sibling 都照常工作，包括路径中带真实换行的那一个（§2/F）。
2. 命名空间归属与 sibling 扫除 —— 6/6，每条都配 plain git 对照
| # | 主张 | plain git（对照） | 当前 head |
|---|---|---|---|
| A | 在 origin 持有默认 refspec 的仓库里，无 refspec 的 remote 被删后其孤儿引用仍被清扫（R15-1 回归见证） |
留下孤儿 refs/remotes/noref/main |
已清扫；refs/remotes/origin/main 不受影响 |
| B | 存活 remote 的 fetch dest 命名空间（keeper → refs/remotes/gone/*）内的引用绝不清扫 |
引用存活 | 引用存活 |
| C | 中间带通配的 dest 无法界定命名空间 → fail closed | 引用存活 | 引用存活（pre-R15：被删除） |
| D | linked worktree 自身 config.worktree 中指向被删 remote 的 upstream 键被清扫 |
sibling 仍留 branch.feature.remote = gone、…merge |
两个键都已清除 |
| E | 指向无关仓库的植入 gitdir 不会被写入 | victim 不受影响 | victim 不受影响（pre-R15：被 unset） |
| F | sibling 路径带真实 \n 且伪造了一行 worktree /tmp/fake 时，按 NUL 分帧解析 |
键存活 | 正确清扫，无错乱 |
3. Live daemon 真实 HTTP —— 18/18
| 主张 | plain git（对照） | 当前 head |
|---|---|---|
分类器碰撞：删除名为 dirty-cache、already exists、not a git repository 的不存在 remote |
— | 三者都返回 404 no_such_remote，绝不落入遗留关键字分支 |
换行注入：fetch 值携带真实 LF 伪造 error: Could not remove config section … |
git 在 refspec 解析错误后死掉 | 409 remote_config_unparsable，section 存活，行仍显示 |
URL 谓词：ext::sh -c id、::sh -c id（git 的空 helper 形态）、-oProxyCommand=id |
git remote add 三者全收 |
400 invalid_remote_url |
URL 谓词：ssh://git@[::1]:22/x.git、scp 形式、含空格的相对路径 |
接受 | 接受（200） |
insteadOf 改写 |
git remote -v 显示 https://rewritten.invalid/o/r.git |
面板显示配置中的 git@example.invalid:o/r.git |
| partial clone + push URL 覆盖 | remote -v 的注解无法机读 |
promisor=true、push URL 单独上报，均读自配置 |
位于 config.worktree 的 remote |
git remote remove 失败：error: Could not remove config section 'remote.wt'，remote 存活 |
200，在 git 写不了的作用域完成删除 |
include.path 文件中的 section |
git 报成功，而 include 仍让该名字解析成功 | 列出，随后 409 remote_section_in_included_file —— remote 确实存活 |
| 守卫 | — | ?cwd= 逃逸 → 变更类 400 invalid_cwd，无 token 变更 → 401，非仓库 → 404 not_a_git_repository |
| 脱敏 | — | 本轮收集到的所有错误响应体中，绝对路径 0 次 |
4. 继承（global）作用域 —— 3/3，plain git 在这些形态上是静默错误的
| 主张 | plain git（对照） | 当前 head |
|---|---|---|
| 用户 global 配置里定义的 remote | git remote 把 shadow 当作本仓库的列出 |
不列出 —— 只列仓库自有作用域 |
| 在其之上添加同名 remote | git remote add 退出码 0，此后 git 解析出两个 URL |
409 remote_shadows_inherited，本地未写入任何内容 |
| 删除一个本地与继承作用域都存在的名字 | git remote remove 退出码 0，而该名字仍解析到 global URL |
409，本地 section 完整保留，而不是被删掉一半 |
5. 真实浏览器 → 真实 daemon → 真实 .git/config —— 11/11
| 实时分支弹层中的 Manage Remotes… | 面板列出真实仓库的 remotes |
|---|---|
|
|
添加 —— 用 git config --get 在磁盘上核对 |
重复添加：git 自己的报错，且未产生任何改动 |
|---|---|
|
|
| 两段式确认已武装（此时零请求） | 面板背后 remote 被删除的竞态 |
|---|---|
|
|
- 每次变更前后，面板的行与
git remote完全一致。 - 第一次点击删除发出了零个 HTTP 请求、仓库未被改动；第二次才从
.git/config中删除。 - 失败的重复添加没有把
remote.fork.url变成多值。 - 竞态（面板开着时在终端
git remote remove racer，再确认删除）显示 git 真实的error: No such remote: 'racer'，面板重新拉取列表，而不是留下一个删不掉的行。
相对第 1 轮的新内容 —— R15-3/R15-4 的易混字符折叠，在真实 UI 上成立：
Istanbul 与 lstanbul：搜其一，两行都命中 |
bidi 覆盖名：剥离、标记，仍可删除 |
|---|---|
|
|
- 输入
Istanbul命中两行，且两行都带(lookalike name)—— TR39 折叠在 UI 里也做了大小写处理，不只是单测里成立。 - 带
U+202E的行渲染为evliat (hidden characters)；原始码点从未进入 DOM，而删除请求在链路上仍携带配置中的原始名字（65 76 202e 6c 69 61 74），真实 git 也接受了它。 - zh-CN 完整：
6. 在当前 head 本地复跑的门禁（macOS、Node 24.18.1、git 2.55.0）
| 套件 | 结果 |
|---|---|
core git-remotes + git-remotes-kill + git-branches |
352/352 |
cli workspace-git-remotes + workspace-git-branches + process-env-guard |
108/108 |
sdk-typescript DaemonClient |
435/435 |
web-shell BranchPickerPopover + remote-name-skeleton + unicodeConfusables |
136/136 |
web-shell e2e web-shell.git-remotes.spec.ts（Playwright） |
5/5 |
第 1 轮记录的那个 git-branches 失败源于 git 2.47 的措辞；在 git 2.55 上已通过，该注记可以撤下。
7. 红色的 Windows 检查不是本 PR 引入 —— 唯一属于本 PR 的差异是计时抖动
本 head 的 dispatch 运行里 Test (windows-latest, Node 22.x) 失败。与 main 自己的 nightly（运行 34713863264、b5c7635ff，同一台 self-hosted Windows runner）对比：
| 失败数 | 文件数 | |
|---|---|---|
main nightly |
148 | 47 |
| 当前 head | 149 | 47 |
两边的文件集合在其余部分完全一致（code-mode 24、scheduler 13、monitor-debug-store 12、git-branches 8 … 两边相同）。只有两个文件不同，一边一个，其中只有一个属于本 PR：
BranchPickerPopover.test.tsx > resets the remotes view and restores no focus after a workspace switch
该测试在 b3aa91ebb 上逐字节相同，而同一条 Windows 通道当时是通过的——所以不是当前 head 新引入的缺陷。成因已在 macOS 上确定性复现：弹层自身挂载时的 setTimeout(() => searchRef.current?.focus(), 50)（BranchPickerPopover.tsx:605，main 上既有、本 PR 未触碰）会在 runner 于「采样 activeBefore」与「settle」之间花费 >50 ms 时落进断言窗口。给测试加一行即可逐字节复现 CI 的报错：
const activeBefore = document.activeElement;
+ await act(async () => { await new Promise((r) => setTimeout(r, 80)); });
await act(async () => { release?.({ v: 1, workspaceCwd: '/repo2', remotes: [] }); });AssertionError: expected <input …(3)></input> to be <body><div>…(2)</div></body>
把同样的延时挪到 activeBefore 采样之前，测试即恢复绿色；未改动的原测试在本机 20/20 通过。建议修法（测试侧，一行）：在采样 activeBefore 前先冲过那 50 ms 的 open-autofocus。这不是产品缺陷——弹层在打开 50 ms 后自动聚焦搜索框是既有的预期行为。
8. 真正卡住合并的是什么
- 178 条评审线程全部已解决（GraphQL 分页核对）—— 未解决 0 条。
- 阻塞的
CHANGES_REQUESTED锚定在5ad579c97（2026-09-13 04:33），也就是比当前 head 落后两个修复批次；机器人在08f5009ad上唯一的评审是COMMENTED，不带票。 - head 上的必需通道全绿：
Test (ubuntu-latest)、Lint & Static、web-shell E2E Smoke、Serve A/B、Real daemon E2E、Java 矩阵；Test (macos-latest)在 dispatch 运行中通过。 - 因此剩下的门是人工批准，不是代码。
9. 非阻塞观察
- **N1（沿用第 1 轮的 N3，未变）。**测试计划第 9 条写「
?cwd=逃逸 workspace → 400」。对两个变更类接口成立；GET 路由是刻意宽松的，会回退到 workspace 根（本轮再次实测）。只是措辞问题。 - **N2。**中间带通配的 fail-closed 规则（§2/C）意味着在这类仓库里，被删 remote 自己的孤儿引用也会被留下，而删除仍返回 200——幻影分组会继续列出。这是文档化的取舍（
docs/design/git-manage-remotes.md:344-348，"residue stays as cache — the safe polarity"），我确认行为与文字一致；在此记录只是为了让文档句子与实测行为的一致性有据可查。 - N3。§7 的抖动值得在下一次 Windows dispatch 前用那一行测试修法处理掉，否则该通道会一直带着一个看起来属于本 PR 的红。
- N4 —— 独立印证沙箱验证的 finding 1，并补一个细化。
qwen-triage的沙箱运行在本轮进行期间发布，其 287/287 的结论与本轮一致；我在真实 UI 里复测了它最锐利的那一条。0rigin与origin并列时：两行都没有被标记，因为分组用的 skeleton 是区分大小写的折叠（0折叠为O，于是skeleton('0rigin') = "Origin"≠skeleton('origin')）。细化之处在于：搜索那一半已经处理了 —— R15-3 会把 needle 折叠成三种大小写形态，所以输入origin确实能搜出0rigin。漏掉 casefold 的只有行标记这条路径，因此修法可以收敛到remoteSkeletonGroups，而不必动折叠本身。
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
PR 主旨 / What this PR does
这个 PR 给 web-shell 加了一套「管理 git remote」的能力：在分支/远程面板里列出、新增、删除远程，底层走 workspace 作用域的 daemon 路由，核心 helper 用 git config --list --show-scope -z（NUL 分帧）读配置并做作用域过滤。安全模型是「显示脱敏、请求原样」——每个渲染边界剥离 Default_Ignorable/bidi/零宽字符，add/remove 载荷带原始名；并用 TR39 confusables skeleton 折叠做同形碰撞检测，git argv 一律以 -- 收尾防参数注入。删除远程走「认证式」清理：先删 [remote] 段，再清扫遗留的 tracking refs / upstream key，最后用解析器复核，宁可拒绝也不谎报一个 git 仍能解析的远程已删除。
This PR adds git-remote management to the web-shell (list / add / remove) over workspace-scoped daemon routes, with a core helper that reads config via git config --list --show-scope -z (NUL-framed) and filters by scope. The security model is "display sanitized, request raw" — Default_Ignorable/bidi/zero-width stripped at every render boundary, raw name in add/remove payloads — plus a TR39 confusables-skeleton fold for homoglyph-collision detection, and ---terminated git argv to block argument injection. Removal is certifying: delete the [remote] section, sweep leftover tracking refs / upstream keys, then re-verify by resolution, preferring to refuse over certifying a remote git would still resolve.
审查方式：纯静态、逐 hunk 对照 head tree（08f5009），未运行测试/构建。CI 实质 lane 全绿，qqqys 已在同一 head approve。round-16 机器人在降级模式下把下面两条留作 Critical 未定论；我独立复核后给出校准过的结论。
Review method: static only, every hunk cross-checked against the head tree (08f5009); no tests/builds run. CI's substantive lanes are green and qqqys approved at this same head. The round-16 bot left the two items below as unverified Criticals in degraded mode; I re-checked both independently and give a calibrated verdict.
🟡 Important (1) — 删除认证的解析器腿只看 fetch 侧，漏了 pushInsteadOf 的 push 侧孪生 / removal certification's resolver leg is fetch-side only, missing the push-side pushInsteadOf twin
packages/core/src/utils/git-remotes.ts
主路径的「已删除名」认证门（:606-607）是：
if (
(await remoteSectionScopes(cwd, name, env)).size > 0 ||
(await remoteStillResolves(cwd, name, env)) // 只有 fetch 侧 ls-remote --get-url
) {
throw new Error('remote still configured after removal');
}remoteStillResolves（:817）走的是 ls-remote --get-url -- name，只解析 fetch 侧。而同一个文件里的 converge 分支（:504-528）在决定是否清扫时，显式加了一条 push 侧守卫：
!(await pushInsteadOfAliases(cwd, env)).some((alias) => name.startsWith(alias)) // :526pushInsteadOfAliases（:881）全仓只被 :526 这一处调用；:606-607 的认证门、以及其后的 deleteRemoteTrackingRefs / unsetUpstreamKeys / sweptUpstreamResolving（:619-638）都不覆盖 push 侧解析。后果：当配置里存在一条 url.<base>.pushInsteadOf = <prefix> 且被删名 startsWith(prefix) 时，[remote "name"] 段确实被删掉了，认证门也放行「已删除」，但 git push name 仍会经 pushInsteadOf 别名解析到一个 URL——正是本模块 :593-604 注释里说要拦的那类「面板说删了、fetch/push 却还能到达」的状态。这就是 insteadOf（fetch 侧，remoteStillResolves 能抓到并拒绝）与 pushInsteadOf（push 侧，认证门放行）之间的平行兄弟成员不对称守卫。
建议：给认证门补上 converge 分支已有的同一条 push 侧腿：
if (
(await remoteSectionScopes(cwd, name, env)).size > 0 ||
(await remoteStillResolves(cwd, name, env)) ||
(await pushInsteadOfAliases(cwd, env)).some((alias) => name.startsWith(alias))
) {
throw new Error('remote still configured after removal');
}诚实的另一面：pushInsteadOf 是用户独立配置的全局 URL 重写规则，不属于 [remote] 段本身；把「段已删除」判为完成、把别名重写视为正交配置，也讲得通——这很可能就是本 head 已被 approve 的原因。但按本模块自己的教义（「宁可拒绝也不认证一个 git 仍能解析的状态」）与 converge 分支的既有处理，两条腿应当对称，所以我按 Important（而非 Critical）提出，触发面窄（需要一条前缀命中被删名的 pushInsteadOf 别名）。
The main-path "removed name" certification gate (:606-607) checks remoteSectionScopes(...).size > 0 || remoteStillResolves(...). remoteStillResolves (:817) runs ls-remote --get-url -- name — fetch-side only. But the converge arm in the same file (:504-528) explicitly adds a push-side guard before deciding to sweep: !(await pushInsteadOfAliases(cwd, env)).some((alias) => name.startsWith(alias)) (:526). pushInsteadOfAliases (:881) is called from :526 and nowhere else; neither the :606-607 gate nor the later deleteRemoteTrackingRefs/unsetUpstreamKeys/sweptUpstreamResolving (:619-638) covers push-side resolution. Consequence: when config holds a url.<base>.pushInsteadOf = <prefix> and the removed name startsWith(prefix), the [remote "name"] section is genuinely deleted and the gate certifies "removed", yet git push name still resolves through the alias — exactly the "panel said removed, fetch/push still reach it" state this module's own comment (:593-604) says it refuses. This is an asymmetric sibling guard between insteadOf (fetch-side, caught by remoteStillResolves) and pushInsteadOf (push-side, certified past). Suggested fix: add the converge arm's push-side leg to the certification gate (snippet above). Honest counterpoint: pushInsteadOf is orthogonal global user config, not part of the [remote] section, so treating the section removal as complete is defensible — which likely explains the existing approval. I raise it as Important (not Critical) given the module's own doctrine and the narrow trigger (requires an alias whose prefix matches the removed name).
🟡 Important (2) — 路径脱敏的 truncatedKeys 臂重新引入了邻居臂刚消除的 O(L²) 同步正则 / the truncatedKeys redaction arm reintroduces the O(L²) sync regex its neighbor arm just removed
packages/core/src/utils/workspace-git-branches.ts
:55-63 的 truncatedKeys 臂：
for (const key of externals.truncatedKeys) {
message = message.replace(
new RegExp(`${escapeRegExp(key)}\\S*`, 'g'), // :60 无界 \S* 前缀
'<workspace>',
);
}紧挨着的 :100-103 那条 /etc/gitconfig 臂，本 PR 里被特意改写成按 token 处理，并留了明确注释（:94-99）：
Decide per whitespace-delimited TOKEN, not with an unbounded
\S*prefix: one long whitespace-free run in the payload (a rejected push's sideband data bypasses git's vreportf cap) would otherwise cost O(L²) of synchronous CPU on the daemon's single event loop, before the 512-char slice ever applies.
也就是说作者已经认识到 \S* 前缀在长无空白串上的同步 CPU 代价，并在下面一条臂里消除了它；而上面的 truncatedKeys 臂（本 PR 新增代码）又把同样的 escapeRegExp(key) + '\S*' 无界形态放了回来。truncatedKeys 来自 workspace 侧（.git-as-file 布局里的 gitdir/commondir 行，可达数千字节），redactGitPaths 在 daemon 的单线程事件循环上同步跑，一旦 payload 里出现长无空白串（如被拒 push 的 sideband 数据）就会阻塞所有会话。触发面窄（需要 .git-as-file 布局产生 truncatedKeys + 错误消息里有长无空白串），但这是共享 daemon 上的同步停顿，且与相邻臂刚立下的规约自相矛盾。
建议：沿用邻居臂的 token 习语，或对 key 长度设上限，避免 \S* 在整条未切片消息上回溯。
The truncatedKeys arm at :55-63 uses new RegExp(\${escapeRegExp(key)}\S*`, 'g')— an unbounded\S*prefix. The immediately-adjacent/etc/gitconfigarm (:100-103) was *deliberately* rewritten in this PR to be token-based, with an explicit comment (:94-99) that the\S*form "would otherwise cost O(L²) of synchronous CPU on the daemon's single event loop, before the 512-char slice ever applies." So the author already recognized the sync-CPU hazard of an unbounded\S*prefix and removed it one arm below — but the truncatedKeys arm (new code in this PR) reintroduces the same shape.truncatedKeysis workspace-controlled (gitdir/commondir lines from a.git-as-file layout, up to several KB), and redactGitPathsruns synchronously on the daemon's single-threaded event loop, so a long whitespace-free run in the payload (e.g. a rejected push's sideband data) can stall **all** sessions. The trigger is narrow, but it's a synchronous stall on a shared daemon and contradicts the convention the neighboring arm just established. Suggested fix: use the neighbor's token idiom, or cap the key length, so\S*` can't backtrack over the whole unsliced message.
🔵 Low-confidence — skeleton 折叠的合流性 / confluence of the skeleton fold
packages/web-shell/client/utils/remote-name-skeleton.ts:36
机器人（R15-4）主张：收尾的 NFC（:60）会跨 key 边界重新组合，使折叠「取决于到达拼写」而非规范等价类，从而把同一 ink 类拆成两个碰撞组；生成器方程 value === runtimeFold(key.normalize('NFD')) 只对孤立 key 成立。
我复核后降级为低置信度、非阻塞：该函数是迭代到不动点（最多 8 遍，:37），每遍都重新处理上一遍的 NFC 输出，文件注释（:26-35）恰恰举了跨边界重组的例子（0→O，NFC 组合成 Ő，其 prototype 是 Ö）来说明为什么需要多遍；生成器还会丢弃任何会循环而非收敛的条目，实测「≤2 额外遍」即收敛。因此在已提交的表内，precomposed / decomposed 两种拼写会在不动点处相遇。要把它升为阻塞项，需要一个具体的发散 witness 对（两个渲染相同、在 8 遍内落到不同 skeleton 的名字）——目前只有理论论证，没有 witness；且影响面仅限次级同形碰撞告警，主脱敏在 UI 每个渲染边界已独立核实存在。
The bot (R15-4) argues the closing NFC (:60) recomposes across key boundaries, making the fold depend on arrival spelling rather than the canonical equivalence class and splitting one ink class into two collision groups. I downgrade this to low-confidence / non-blocking: the function iterates to a fixed point (up to 8 passes, :37), re-processing the previous pass's NFC output each time, and the file's own comment (:26-35) uses exactly a cross-boundary recomposition example (0→O, NFC composes Ő, whose prototype is Ö) to justify the multi-pass design; the generator also drops any entry that would cycle rather than settle, with measured convergence "≤2 extra passes." So within the committed table, precomposed and decomposed spellings meet at the fixed point. Escalating this to blocking needs a concrete divergent witness pair (two identically-rendering names that land in different skeletons within 8 passes) — only a theoretical argument exists today, and the blast radius is a secondary homoglyph-collision warning, since the primary sanitization is independently verified present at every UI render boundary.
已核实干净 / Verified clean
- UI 脱敏层（
BranchPickerPopover.tsx+1289/-248、i18n.tsx、css）：12 个 DOM 边界全部经sanitizeRemoteDisplay/sanitizeStatusText/remoteExtras脱敏；请求载荷带原始名（remoteName.trim()/remoteUrl.trim()，remove 用原始r.name）；脱敏正则含\p{Cc}\p{Cf}\p{Default_Ignorable}\u2028\u2029；无dangerouslySetInnerHTML/href/src注入面；两步确认无法粘滞/绕过/双触发；26 个新 i18n key 中英齐全；248 行删除是 branches 视图 JSX 重排缩进，未丢守卫。
UI sanitization layer: all 12 DOM boundaries sanitized; requests carry the raw name; nodangerouslySetInnerHTML/href/srcinjection surface; two-click confirm cannot stick/bypass/double-fire; all 26 new i18n keys present in EN+ZH; the 248 removed lines are re-indented branches-view JSX with no guard dropped. - 参数注入：所有用户可控值都以
--收尾——remote add -- name url（:410）、remote remove -- name（:453）、ls-remote --get-url -- name（:823）、ls-remote -- name（:863）、ls-remote --get-url -- value（:925）。
Argument injection: every user-controlled value is---terminated.
结论 / Verdict
🔄 COMMENT（不批准）。两条独立复核为真的 Important：(1) 删除认证的 push 侧不对称守卫，(2) 路径脱敏 truncatedKeys 臂的同步 CPU 回归。二者触发面都窄，且 (1) 有正当的「正交配置」反驳——这解释了本 head 已有的 approve；但按本模块自订教义与相邻代码规约，它们值得作者确认或显式记为可接受残留。R15-4 我降级为低置信度、需具体 witness。UI 层、参数注入、CI 均干净。
🔄 COMMENT (not approving). Two independently-verified Importants: (1) the push-side asymmetric guard in removal certification, (2) the sync-CPU regression in the truncatedKeys redaction arm. Both have narrow triggers, and (1) has a legitimate "orthogonal config" counter-argument — which explains the existing approval at this head — but per the module's own doctrine and the neighboring-code convention they warrant an author fix or an explicit accepted-residual note. R15-4 I downgrade to low-confidence pending a concrete witness. The UI layer, argument injection, and CI are clean.
… entry Main's context-overview rework removed the sidebar git pill and the welcome-view branch chip this spec opened the picker through; the merge-head smoke run failed waiting for a pill that no longer renders. The spec now enters the scenario session and opens the picker from the environment panel's branch row — the same route the branch-picker visual spec uses — keeping every assertion unchanged.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review pass at head 789b42b92a3d73033534f6e28204954c4cd6aec0. Not approving: two Critical findings filed against the previous commit still stand at this head, because neither file changed after they were filed. One is confirmed in the head source below; the other is recorded as unconfirmed. The pass then stopped widening, so most of this surface was not audited to depth.
Standing: the removed-name certification resolves fetch-side only
packages/core/src/utils/git-remotes.ts, remoteStillResolves at :811-833 at this head:
// Whether git still RESOLVES the name after the section is gone — the
// certification the config-record checks cannot give ...
async function remoteStillResolves(cwd, name, env?) {
try {
const out = await runGit(cwd, ['ls-remote', '--get-url', '--', name], env);
return (out.endsWith('\n') ? out.slice(0, -1) : out) !== name;
} catch (err) { stripConfigDump(err); throw err; }
}ls-remote --get-url answers the fetch side: it applies url.<base>.insteadOf and never consults url.<base>.pushInsteadOf, which is the rewrite git uses when pushing. So a removal whose name a pushInsteadOf alias still resolves push-side is certified here as "git no longer resolves this name at all", and the push twin of the alias survives a removal the panel reports as complete and verified. That is the exact failure this PR's own design says it refuses - the description states removal is verified rather than trusted and that the route answers 409 remote_still_configured "rather than certifying a state git would still resolve" - so the certification leg is the load-bearing one and it is half as wide as the claim it backs.
The comment block is explicit that this function exists to give "the certification the config-record checks cannot give" and to fail closed on any failure; the gap is not error handling but scope, and a push-side leg (git config --get-urlmatch over url.*.pushInsteadOf for the name, or the equivalent ls-remote push query) is what closes it.
This was filed against the previous commit and the file is unchanged since: the only author commits between that review and this head are Merge remote-tracking branch 'origin/main' and test(web-shell): route the remotes e2e through the post-#11700 picker entry, and the only file either touches in this area is packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts.
Standing, unconfirmed: the confusable fold decides on arrival spelling
packages/web-shell/client/utils/remote-name-skeleton.ts:26-67 is likewise unchanged since the finding was filed. Reading it at head: the fold iterates to a fixed point under an 8-pass cap, consults CONFUSABLE_PROTOTYPES on the arrival code point first, then walks NFD parts with their own table chance, then NFKD, and NFC-closes at the end of each pass. The finding's claim is that the result is still not a function of the canonical equivalence class, so two names that render identically can take different skeletons and split one identity in the lookalike marker.
I could not settle that within this pass, and the code itself concedes the underlying question - "The cap guarantees termination (confluence is not proven)". What I can say is that nothing changed to answer it, so it is recorded as standing-unconfirmed rather than cleared. A test over the paired spellings the finding names (i\u0146fra / in\u0326fra, \u1E9B and its parts, a precomposed/decomposed pair whose composed form is itself a table key) would settle it in one direction or the other.
Gates this pass did not complete
All 178 review threads are flagged resolved, including 30 Critical threads from earlier rounds; those flags are not treated as evidence and I did not re-derive them. Once the two standing items were established the pass stopped, so the following were not audited to depth - which matters here because the change is 20,938 added lines across 33 files and three of its surfaces are security-relevant:
- The three new workspace-scoped daemon routes and their guard parity with the sibling branch routes: untrusted workspace, closed workspace generation,
?cwd=escape, non-repository, and the rule that error bodies never carry absolute paths. - The shared git error classifier's six new remote-specific branches, which the description itself identifies as the main risk: they are matched ahead of every legacy keyword branch on untruncated detail, where a config-chosen remote name can carry any keyword and a config-chosen value can carry a real newline.
- The redaction of gitdir paths outside the cwd tree - a linked worktree's shared main
.git, a submodule gitdir, symlink-canonical spellings - parsed from the.gitfile under head-bounded reads. - The removal converge path: the worktree-scope half git cannot edit, the upstream-key sweep for pointing branches, orphaned
refs/remotes/<name>/*after a refspec-less removal, and the inherited same-name survivor refusal. - The bidi and zero-width sanitization at each render boundary and its interaction with the search filter and the raw-name request path.
CI
This head has concluded failures, not just pending work: Lint & Static (ubuntu-latest, Node 22.x), Serve A/B (ubuntu-latest, Node 22.x) and Real daemon E2E / Java 11 all completed as failure, while Test (ubuntu-latest, Node 22.x), Test (macos-latest), Test (windows-latest), both integration lanes, web-shell E2E Smoke, Capture web-shell visuals, Desktop Shell and Live Host completed cancelled, and review-pr was still running. I did not establish attribution for any of them, and under this channel's rules an unattributed CI state does not by itself decide the review - the two standing Criticals do. The practical consequence is that this head currently carries no concluded green unit, lint or integration evidence, so nothing corroborates the local gate results the description reports.
Next step
Add the push-side leg to remoteStillResolves so the certification covers both rewrite directions, and settle the skeleton fold with the paired-spelling cases above - a test either closes it or reproduces the split. Then get a clean CI run on the new head before re-requesting review, since the cancelled lanes mean the current failures cannot be read as either this PR's or not. Given the size of this surface, splitting the unaudited list between reviewers would let the routes, the error classifier and the removal converge path each get a dedicated pass rather than a share of one budget.
A fresh `tsc --build` of packages/cli on the merged tree peaks just above the 3072 MiB ceiling: two consecutive CI Lint runs and a local clean-tsbuildinfo build all OOM at ~3.0/3.1 GiB, while each parent tree alone stays under the ceiling and incremental builds never showed it. Workspace build children inherit NODE_OPTIONS from the root build script, so raising it there is what fits the union program; the test ceilings are untouched.
7 participants
What this PR does
The workspace git popover in the Web Shell — opened from the left sidebar's workspace git pill or the composer branch chip — gains a Manage Remotes panel: list the repository's configured remotes with their fetch and push URLs, add a remote, and remove one behind a two-click inline confirm. It is backed by three new workspace-scoped daemon routes (list / add / remove), matching SDK client methods, and a core git helper that reads remotes from git's CONFIG (
git config --list --show-scope -z, NUL-framed records) instead ofgit remote -vor per-nameget-urllookups. Mutations answer with the fresh remote list, so the panel re-renders in one round trip; a removal also refreshes the branch listing and the branch chip's tracking state, because git deletes the remote-tracking refs and upstream configuration together with the remote.Why it's needed
The popover could pull, commit, push, and switch branches, but the remotes themselves were read-only context — remote-tracking branches were listed, grouped by a remote name the user could not change. Anyone who clones from a zip (no
origin), works a triangular fork + upstream layout, or just wants to drop a stale remote had to leave the Web Shell and open a terminal. This closes that gap with the minimal useful set (list / add / remove); URL editing, rename, and fetch/prune stay out of scope.Two details worth calling out because they shaped the implementation:
git config --list --show-scope -zread yields the full section (urls, push urls, refspecs, promisor/partial-clone) immune toinsteadOfrewriting and rendered-output annotations, and the scope filter lists the repository's own scope (local + worktree, include-sourced) — the sectionsgit remote add/removeact on — while inherited global/system sections stay out. Add pre-flights the name across ALL scopes and refuses a same-name inherited section with 409remote_shadows_inherited; removal re-reads the section across every scope, completes the worktree-scope half git cannot edit, sweeps the pointing branches' upstream keys and orphanedrefs/remotes/<name>/*a refspec-less removal leaves, and refuses 409remote_still_configuredrather than certifying a state git would still resolve..git/configthe user did not author (downloaded zip, cloned repo) can carry bidi/zero-width characters in remote names and URLs. The panel strips the full Unicode Default_Ignorable set at every render boundary (rows, tooltips, aria labels, footer messages, and the search filter, so what you can see is what you can search for), while add/remove requests always carry the exact configured name — removal validation is deliberately as lenient as git itself, so a hand-edited config can always be cleaned up through the UI. Names differing only by whitespace (which CSS collapses out of the inked text) flag the same way, with the raw name spelled out as codepoint escapes in the tooltip and aria-labels.Reviewer Test Plan
How to verify
Run the daemon against a git workspace, open the Web Shell, and expand the workspace in the left sidebar:
remote … already existserror in the footer, list unchanged.No such remotelands in the footer and the panel re-reads the list instead of keeping a row that can never be removed.git clone --filter=blob:nonerepo with a push-URL override, the panel shows the true fetch URL and the override in the tooltip.workspace_runtime_unavailable,?cwd=escaping the workspace → 400, non-repo → 404not_a_git_repository. Error bodies never contain absolute paths.Executable evidence:
packages/web-shell/client/e2e/web-shell.git-remotes.spec.tsdrives flows 1–6 end-to-end in Chromium against the mocked daemon, and the daemon layer is covered against real temporary git repositories by the route suite (27 cases) and the core suite (55 cases, including the partial-clone and invisible-character shapes).Evidence (Before & After)
Before: the popover's action list ended at Checkout Tag or Revision…; remote names appeared only as read-only group headers in the collapsed Remote branch section, with no way to add or remove one.
After: the same popover carries a Manage Remotes… row opening the panel below. Screenshots captured from the mocked-daemon Playwright run (default theme, 1280×800):
Tested on
Environment (optional)
Local daemon build + Web Shell dev server; Playwright (Chromium) for the UI e2e and the screenshots above; vitest suites against real temp repos for core/routes. Full local gates green on the current head:
npm run build,npm run typecheck,eslint --max-warnings 0on every touched file, core 162/162, cli routes 100/100 (including the sibling branch-route suite), SDK 417/417, web-shell 109/109, e2e 5/5.Risk & Scope
.gitdir, a submodule's gitdir, symlink-canonical spellings — parsed from the.gitfile with head-bounded reads. Second: removal is verified, never trusted — git's ownremote removedeletes tracking refs, upstream keys, and the section in that order, and every step the panel cannot see git complete (worktree-scope sections and upstream keys, refspec-less orphaned refs, inherited same-name survivors) is either completed in the scope git could not write or refused with a name-free 409./workspace/git/remote*form, matching the GitHub-PRs precedent); a live-daemon curl pass was not run in the authoring environment (its shell guard blocks git fixture writes) — the daemon layer is instead verified by supertest route tests against real temp repos, and the UI by the Playwright spec. Windows/Linux rely on CI.Linked Issues
None.
中文说明
这个 PR 做了什么
Web Shell 的 workspace git 弹层（从左侧栏 workspace 的 git 胶囊或输入框的分支 chip 打开）新增 管理远程仓库（Manage Remotes） 面板：列出仓库已配置的 remotes 及其 fetch/push URL、添加 remote、以及带两段式内联确认的删除。底层由三个新的 workspace 级 daemon 路由（列表 / 添加 / 删除）、对应的 SDK 客户端方法，以及一个直接读取 git 配置（
git config --list --show-scope -z，NUL 分隔记录）的 core 辅助模块支撑——不解析git remote -v，也不按名逐个get-url。变更类接口的响应直接携带最新列表，面板一次往返即可重渲染；删除还会刷新分支列表与分支 chip 的跟踪状态，因为 git 会把 remote-tracking refs 和 upstream 配置随 remote 一起删掉。为什么需要
弹层原本可以 pull、commit、push、切分支，但 remotes 本身只是只读上下文——远程跟踪分支按 remote 名分组展示，而用户无法改动这些 remote。从 zip 解压（没有
origin）、fork + upstream 三角工作流、或想删掉过期 remote 的用户，都必须离开 Web Shell 去开终端。本 PR 用最小可用集合（列表 / 添加 / 删除）补上这个缺口；改 URL、重命名、fetch/prune 不在范围内。两个影响实现方式的细节：
git config --list --show-scope -z即得完整 section（url、pushurl、refspec、promisor/partial-clone），免疫insteadOf改写与渲染注解；作用域过滤只列仓库自有作用域（local + worktree，含 include）——也就是git remote add/remove实际作用的部分——继承的 global/system section 一律不进列表。Add 预检覆盖全部作用域，发现同名继承 section 一律 409remote_shadows_inherited；删除后在 git 能解析的全部作用域复核 section，补删 git 写不了的 worktree 一半，清扫指过去分支的 upstream 键与无 refspec 删除留下的孤儿refs/remotes/<name>/*，宁可 409remote_still_configured也不把一个 git 仍能解析的状态说成成功。.git/config（下载的 zip、克隆的仓库）可能在 remote 名或 URL 里带 bidi/零宽字符。面板在每一个渲染边界（行、tooltip、aria 标签、footer 消息、以及搜索过滤器——保证「看得见的就搜得到」）剥掉完整的 Unicode Default_Ignorable 字符集，而添加/删除请求始终携带配置中的原始名字——删除侧的校验刻意与 git 本身一样宽松，手改出来的配置也总能通过 UI 清理掉。只差空白字符的名字（CSS 会折叠掉）同样标记，tooltip 与 aria 标签里以码点转义写出原始名字。评审测试计划
如何验证
对着一个 git workspace 启动 daemon，打开 Web Shell，在左侧栏展开该 workspace：
remote … already exists错误，列表不变。No such remote，面板会重新拉取列表，而不是留着一个永远删不掉的行。git clone --filter=blob:none且设置了 push URL 覆盖的仓库里，面板显示真实的 fetch URL，tooltip 里是覆盖后的 push URL。workspace_runtime_unavailable，?cwd=逃逸 workspace → 400，非 git 仓库 → 404not_a_git_repository。错误响应体不含绝对路径。可执行证据：
packages/web-shell/client/e2e/web-shell.git-remotes.spec.ts在 Chromium 里对着 mock daemon 端到端跑通第 1–6 条；daemon 层由路由套件（27 例）和 core 套件（55 例，含 partial-clone 与不可见字符形态）对着真实临时 git 仓库覆盖。前后对比
**之前：**弹层操作列表到 Checkout Tag or Revision… 为止；remote 名只作为折叠的 Remote 分支分组标题只读出现，无法增删。
**之后：**同一个弹层多了 Manage Remotes… 入口，打开下方面板。截图来自 mock daemon 的 Playwright 运行（默认主题，1280×800）：
测试环境
运行环境（可选）
本地 daemon 构建 + Web Shell dev server；UI e2e 与上述截图用 Playwright（Chromium）；core/路由用 vitest 对着真实临时仓库。当前 head 上本地门禁全绿：
npm run build、npm run typecheck、所有触及文件的eslint --max-warnings 0、core 162/162、cli 路由 100/100（含兄弟分支路由套件）、SDK 417/417、web-shell 109/109、e2e 5/5。风险与范围
.git、子模块 gitdir、符号链接规范化后的拼写——从.git文件解析、限长读取。第二点：删除是复核过的，不是听信的——git 自己的remote remove按序删除跟踪 refs、upstream 键与 section，面板看不到 git 完成的每一步（worktree 作用域的 section 与 upstream 键、无 refspec 留下的孤儿 refs、同名继承残留），要么在 git 写不了的作用域补删，要么以不含名字的 409 拒绝。/workspace/git/remote*形态，与 GitHub-PRs 先例一致）；撰写环境未跑 live-daemon 的 curl 验证（其 shell guard 禁止 git fixture 写操作）——daemon 层改由 supertest 路由测试对真实临时仓库验证，UI 由 Playwright spec 验证。Windows/Linux 依赖 CI。关联 Issue
无。