Skip to content

fix(review): pin the verified git identity across the residue probe (#9557) - #9572

Closed
wenshao wants to merge 9 commits into
mainfrom
fix/review-pin-git-identity
Closed

fix(review): pin the verified git identity across the residue probe (#9557)#9572
wenshao wants to merge 9 commits into
mainfrom
fix/review-pin-git-identity

Conversation

@wenshao

@wenshao wenshao commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

worktreeResidue — the tripwire every wave of review agents is built from — verified a tree's identity once and then ran five commands that each re-discovered the repository from cwd, through the same .git file the check had read. That file is writable by anything running as this user, so the gate and the measurement were a check-then-use pair.

Two halves, closing different windows:

  • The gate now requires the gitfile's admin entry to point BACK at this tree. A repository whose core.worktree names this path answers --show-toplevel with this path — so the old gate saw itself, while every command after it measured the plant's index, which already holds the contamination as committed content. A planted standalone repo has no admin entry to round-trip. scratch-tree gates its own reset on exactly this check; the reading side did not have it.
  • The verified --git-dir / --work-tree are pinned onto every later spawn, so a swap landing after the gate cannot redirect them either.

Why it's needed

The recorded reason for not doing this (#9221, round 17) was that an explicit pin changes what ls-files resolves, so it needs its own round of measurement. The "own round" part was right. The rest was wrong, and this is the measurement:

shape pinned vs unpinned
standalone checkout byte-identical
linked worktree (the production shape) byte-identical
superproject with an initialised submodule byte-identical
worktree reached through a symlinked ancestor byte-identical

across all five commands the probe runs (status --porcelain -uall -z, ls-files --others -z, ls-files -s -z, ls-files -v -z, check-ignore -z -v --stdin).

And it does something. With the identity captured, then the gitfile swapped at a repository whose index already holds the mutant:

genuine, discovery      : M a.ts   ?? __probe__.test.ts
after the swap, discovery: (empty)          <- the mutant certified clean
after the swap, pinned   : M a.ts   ?? __probe__.test.ts

Reviewer Test Plan

How to verify

cd packages/cli && npx vitest run src/commands/review3 941 passed, 1 skipped, 0 failed.

New: worktreeResidue reports unmeasured naming "does not point back" for a tree whose .git was swapped at a repo that answers for this path. The fixture asserts the genuine reading first, so it cannot pass by being unmeasurable for some other reason.

Evidence (Before & After)

  • Backpointer gate — removing it alone turns the new test red.
  • The pinnot pinned by a test, and I would rather say so than imply otherwise: it closes a window that needs the swap to land between two spawns, which the suite cannot open deterministically. Its evidence is the repro above, run against real git; both scripts are reproducible in a scratch dir with git init, a linked worktree, and a second repo whose core.worktree points at the first.

Tested on

macOS 26.6 (Darwin 25.6.0), git 2.51, Node 24, packages/cli vitest 3.2.4.

Risk & Scope

Confined to packages/cli/src/commands/review/lib/worktree.ts. One new refusal path (a gitfile whose admin entry does not round-trip is unmeasured, not clean), and an argument prefix on five commands that was measured to change nothing.

The shape it refuses is one a healthy pipeline never produces: fetch-pr builds review worktrees with git worktree add, which always writes the round-tripping admin entry.

Linked Issues

Closes #9557. Follow-up from #9221; the class it belongs to is #9556. Stacked with #9566 in the sense that both touch lib/worktree.ts, but the regions are disjoint and they merge cleanly in either order.

中文说明

这个 PR 做了什么

worktreeResidue——每一波审查 agent 构建时都会用到的那条绊线——只校验一次树的身份,随后五条命令各自从 cwd 重新发现仓库,经由校验时读过的那同一个 .git 文件。而该文件对任何以此用户身份运行的东西都可写,因此"门"与"测量"构成了一对 check-then-use。

两半,各自关掉不同的窗口:

  • 门现在要求 gitfile 所指的 admin 条目反向指回本树。 一个把 core.worktree 指向本路径的仓库,会让 --show-toplevel 回答本路径——于是旧的门看到的是它自己,而其后每条命令测量的是植入仓库的索引,而那个索引里早已把污染当作已提交内容。植入的独立仓库根本没有可往返的 admin 条目。scratch-tree 对自己的重置正是用这道检查把关;读取侧此前没有。
  • 把校验通过的 --git-dir / --work-tree 固定到其后每一个 spawn,使门之后才落地的掉包同样无法重定向它们。

为什么需要

此前"不做"的记录理由(#9221 第 17 轮)是:显式固定会改变 ls-files 的解析对象,因此需要单独一轮测量。"需要单独一轮"是对的,其余是错的——这就是那次测量:

形态 固定 vs 不固定
独立 checkout 逐字节相同
linked worktree(生产形态) 逐字节相同
含已初始化 submodule 的父工程 逐字节相同
经由符号链接祖先到达的工作树 逐字节相同

覆盖探针运行的全部五条命令。

而且它确实有用。在身份已被捕获之后,把 gitfile 掉包到一个索引里已含突变体的仓库:

真实,发现       : M a.ts   ?? __probe__.test.ts
掉包后,发现     :(空)              <- 突变体被认证为干净
掉包后,固定     : M a.ts   ?? __probe__.test.ts

审查者验证方案

如何验证

cd packages/cli && npx vitest run src/commands/review3 941 通过、1 跳过、0 失败

新增:对于 .git 被掉包到"能为本路径作答"的仓库的树,worktreeResidue 返回 unmeasured 并点名 "does not point back"。夹具先断言真实读数,因此它不会因为"碰巧不可测量"而通过。

证据(Before & After)

  • backpointer 门 —— 单独移除即让新测试变红。
  • 固定本身 —— 没有测试钉住,我宁可写明也不愿含糊:它关掉的窗口要求掉包恰好落在两次 spawn 之间,而测试套件无法确定性地打开它。它的证据是上面那份对真实 git 的复现。

测试环境

macOS 26.6(Darwin 25.6.0)、git 2.51、Node 24、packages/cli vitest 3.2.4。

风险与范围

限于 packages/cli/src/commands/review/lib/worktree.ts。新增一条拒绝路径(admin 条目不能往返的 gitfile 判为 unmeasured 而非 clean),以及五条命令上一段经测量证明零影响的参数前缀。

它拒绝的形态是健康流水线永远不会产生的:fetch-prgit worktree add 创建 review 工作树,而该命令总会写入可往返的 admin 条目。

关联 Issue

Closes #9557。来自 #9221 的后续;所属类别是 #9556。与 #9566 都触及 lib/worktree.ts,但区域不相交,任意顺序都能干净合并。

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review: pin the resolved git identity for a worktree's whole measurement, not just its first spawn

6 participants