Skip to content

fix(review): screen content filters before the probe tree's restore too (#9558) - #9741

Closed
wenshao wants to merge 22 commits into
mainfrom
fix/review-filter-screen-v2
Closed

fix(review): screen content filters before the probe tree's restore too (#9558)#9741
wenshao wants to merge 22 commits into
mainfrom
fix/review-filter-screen-v2

Conversation

@wenshao

@wenshao wenshao commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Replaces #9566 at its original size. Same single commit, rebased onto current main, nothing else.

scratch-tree refuses to create or reset a tree while the repository's local config defines a content filter: a checkout executes filter.<name>.smudge whenever it rewrites a file, and pointing core.hooksPath at nothing covers hooks, not filters. test-efficacy's per-run restore is the same checkout one directory over — checkout --force HEAD -- . rewrites every tracked file in the probe tree, twice per probe run — and it screened nothing.

Three changes: the screen runs before the probe tree's restore; it moves to lib/worktree.ts so both callers share one implementation instead of one growing a corner the other lacks; and filter.<name>.process joins smudge and clean as the third executable key.

Why a new PR

#9566 was this commit plus six more that grew it from 5 files / +160 to 13 files / +3 054 — into base-tree.ts, fetch-pr.ts and lib/git.ts, none of which this change is about. The point of splitting the #9221 follow-ups was that each one stay small enough to read. Opening a replacement rather than force-pushing keeps #9566's review history intact for anyone who wants it.

Why repo-local only — the load-bearing part

git lfs install writes filter.lfs.clean into the user's global config. A screen over merged config would put every contributor with git-lfs into permanent refusal — the same failure as a tripwire that fires on every healthy run, which this pipeline has already shipped once (#9221 round 15: git status reported nothing while the tripwire reported 3 957 paths).

Known gap, deliberately not fixed here

git config --file does not expand include.path / includeIf — that is git's documented default for --file — while the checkouts this screen authorises read merged config, which does. A filter reached through an include directive therefore passes the screen.

The finding is real. The obvious fix is not: adding --includes makes the screen follow an include.path that names the user's own global config, which drags filter.lfs.clean back in and reproduces exactly the permanent-refusal failure the paragraph above exists to avoid. Doing it correctly means resolving each hit's origin file (--show-origin) and refusing only when that origin is repo-local — which is its own change, with its own tests, and does not belong in a PR whose purpose is to be small again.

Reviewer Test Plan

How to verify

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

New coverage in test-efficacy.test.ts: a repo-local filter.evil.smudge makes the mutant run inconclusive naming the filter; the same fixture with the filter moved to the user's global config runs through without a filter refusal. scratch-tree.test.ts gains the filter.evil.process arm.

Evidence (Before & After)

Each half verified by reverting it alone and watching the test go red:

mutation result
remove the screen from test-efficacy red — the local filter no longer refuses
widen the screen to merged config (--file dropped) red — the global git-lfs filter now refuses
drop process from the key regex red — filter.evil.process no longer refuses

The middle row is the one worth reading: it is the false-positive direction, and without a test it is the change a later round would make while "tightening" the screen.

Tested on

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

Risk & Scope

Small and confined to packages/cli/src/commands/review. One new refusal path in the efficacy probe: a repository whose local config carries a content filter reports inconclusive for that run instead of running the filter. No user-facing surface.

Linked Issues

Closes #9558. Replaces #9566. Follow-up from #9221; the class it belongs to is #9556.

中文说明

这个 PR 做了什么

原始尺寸替代 #9566。同一个提交,重新基于当前 main,没有别的东西。

scratch-tree 在仓库本地配置定义了内容过滤器时拒绝创建或重置树:checkout 每次重写文件都会执行 filter.<name>.smudge,而把 core.hooksPath 指向空处只覆盖钩子、覆盖不到过滤器。test-efficacy 每次运行前的恢复是同一种 checkout 挪了一个目录——checkout --force HEAD -- . 会重写探针树里每个已跟踪文件、每次探针运行两次——而它什么也没屏蔽。

三处改动:屏蔽在探针树恢复之前执行;移到 lib/worktree.ts 让两个调用方共用一份实现,而不是其中一个长出另一个没有的角落;filter.<name>.process 作为第三个可执行键与 smudgeclean 并列。

为什么新开 PR

#9566 是这个提交外加另外六个,把它从 5 文件 / +160 撑到 13 文件 / +3 054,摊进了 base-tree.tsfetch-pr.tslib/git.ts——都不是本改动要谈的东西。拆 #9221 后续项的意义,就在于每一个都小到能读完。开替代 PR 而不是 force-push,是为了让 #9566 的评审历史对需要的人完整保留。

为什么只看仓库本地——这是承重部分

git lfs install 会把 filter.lfs.clean 写进用户的全局配置。对合并后配置做屏蔽,会让每个装了 git-lfs 的贡献者永久处于拒绝态——与"每次健康运行都触发的绊线"是同一种失败,而本流水线已经犯过一次(#9221 第 15 轮:git status 一条没有,绊线报 3 957 条路径)。

已知缺口,本 PR 有意不修

git config --file 不会展开 include.path / includeIf——这是 git 对 --file 的文档化默认——而本屏蔽所授权的 checkout 读的是合并后配置,是会展开的。因此经由 include 指令到达的过滤器能通过屏蔽。

这条发现成立。但显而易见的那个修法不成立:加上 --includes 会让屏蔽跟随一条指向用户自己全局配置的 include.path,从而把 filter.lfs.clean 拖回来,精确复刻上一段所要避免的永久拒绝。要做对,需要解析每个命中的来源文件(--show-origin)、仅当来源是仓库本地时才拒绝——那是它自己的一次改动、有它自己的测试,不属于一个"目的就是重新变小"的 PR。

审查者验证方案

如何验证

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

test-efficacy.test.ts 新增覆盖:仓库本地的 filter.evil.smudge 让突变体运行判为 inconclusive 并点名该过滤器;同一夹具把过滤器改放到用户全局配置后,运行照常通过、不产生过滤器拒绝。scratch-tree.test.ts 增加 filter.evil.process 一臂。

证据(Before & After)

每一半都以"单独回退该项 → 对应测试变红"验证(见上表)。中间那一行最值得看:它是误报方向;没有这条测试,它正是后来某一轮在"收紧"屏蔽时会做出的改动。

测试环境

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

风险与范围

小,且限于 packages/cli/src/commands/review。效力探针新增一条拒绝路径:本地配置带内容过滤器的仓库,该次运行报 inconclusive 而不是执行过滤器。无用户可见面。

关联 Issue

Closes #9558。替代 #9566。来自 #9221 的后续;所属类别是 #9556

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: extend the content-filter screen to test-efficacy, scoped to repo-local config (not global)

4 participants