refactor(review): share the probe-worktree path helper; harden the stale-tree sweep - #6841
Merged
Conversation
…ale-tree sweep Follow-up to the disposable-probe-worktree change (#6836), addressing review suggestions on it. - Extract `probeWorktreePath(worktree)` into `lib/paths.ts` and call it from both the probe (`test-efficacy.ts`) and `cleanup.ts`. The `-probe` suffix was constructed independently in the two files; renaming it in one and missing the other would silently stop cleanup from sweeping. The helper also settles the normalisation: it returns an absolute path, because the probe drives `git worktree add`/`remove` with the shared worktree as cwd, where a relative path would resolve against that worktree and nest the probe tree inside it. - Make the pre-`add` sweep clear an UNREGISTERED leftover too. `git worktree remove --force` only unregisters a tree git still tracks; a directory left at the probe path after metadata loss or a partial cleanup is reported as "not a working tree" and left in place, and a non-empty one then makes `worktree add` fail `already exists` — wedging every probe as `inconclusive` until it is cleared by hand. Follow the unregister with an `rmSync` of whatever remains (which unlinks a symlink rather than following it), so `add` always gets a clean path. The sweep's stderr is now kept and appended to the create-failure detail so a genuine `add` failure explains itself. Tests: `probeWorktreePath` unit tests (suffix, relative→absolute, shared source); integration tests that a stale REGISTERED probe tree is swept and the probe still runs, and that an UNREGISTERED non-empty leftover is cleared rather than wedging the probe. Full review suite green (416).
Collaborator
|
Thanks for the follow-up PR! (Re-run at new commit Template: the body uses custom headings ("What", "Verification", "One thing deliberately not tested") instead of the template's standard sections ("Why it's needed", "Reviewer Test Plan", "Risk & Scope"). Content is comprehensive and covers everything the template asks for — not blocking, but matching the template headings would help humans scanning the body. Problem: clearly observed. Direction: aligned — hardening the review tooling's worktree lifecycle is squarely in scope. No CHANGELOG precedent needed for internal plumbing. Size: 144 production lines, 379 test lines across 9 files. All in Approach: scope is tight. The shared Moving on to code review. 🔍 中文说明感谢后续 PR！（在新提交 模板：正文使用了自定义标题（"What"、"Verification"、"One thing deliberately not tested"），而非模板的标准章节（"Why it's needed"、"Reviewer Test Plan"、"Risk & Scope"）。内容全面，覆盖了模板要求的所有方面——不会阻塞，但匹配模板标题能让审查者扫读时更顺畅。 问题：已明确观测到。 方向：对齐——加固 review 工具的 worktree 生命周期完全在范围内。内部管道不需要 CHANGELOG 先例。 规模：144 行生产代码，379 行测试代码，跨 9 个文件。全部在 方案：范围紧凑。共享的 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal: Given the PR title, I'd extract a shared Comparison: The PR's approach matches and exceeds this proposal across all points. The The Reuse check: no existing utility covers the probe path or the two-step unregister+rmSync. No duplication found. No critical issues. No AGENTS.md violations. TestingUnit Tests (37 passed)Integration Tests (12 passed, real git worktrees)LintManual verification (tmux)Internal plumbing refactor — no user-visible CLI changes. The integration tests exercise real git worktrees and cover the exact failure scenarios. The The 中文说明代码审查独立方案： 给定 PR 标题，我会把共享的 比较： PR 的方案在所有方面都匹配并超越了此方案。新提交（ 无关键问题。无 AGENTS.md 违规。 测试49 项测试通过（37 项单元 + 12 项真实 git worktree 集成测试）。ESLint 干净。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage; focused follow-up that does exactly what it promises. This is a textbook follow-up PR: takes review feedback from #6836, addresses each point with minimal, well-reasoned changes. The new commit ( Every change in the full PR earns its place. The 49 tests pass (37 unit + 12 integration with real git worktrees). ESLint clean. The integration tests reproduce the exact scenarios (registered stale tree, unregistered non-empty leftover) and confirm the fix works. Minor nit: PR body uses custom headings instead of the template's sections — not blocking given the comprehensive content. Approving. ✅ 中文说明信心：5/5 — 每个阶段都干净；专注的后续 PR，完全兑现了承诺。 这是一个教科书式的后续 PR：接收 #6836 的审查反馈，用最小化、经过深思熟虑的改动逐条处理。新提交（ 整个 PR 中的每个改动都有存在的理由。 49 项测试通过（37 项单元 + 12 项真实 git worktree 集成测试）。ESLint 干净。批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
…tover Follow-up review on #6841 pointed out that the `rmSync` fallback added to the probe's own sweep was missing from `cleanup.ts`, which sweeps the probe worktree through `releaseWorktree`. That helper returns `existed` (true when the path was there on entry) but only runs `git worktree remove --force` — which does nothing to a directory git no longer tracks as a worktree. So an unregistered non-empty leftover survives while `cleanup.ts` prints "Removed probe worktree", and it still blocks the next `git worktree add` with `already exists`. Fix it at the root: `releaseWorktree` now `rmSync`s whatever remains after the unregister, so a `true` return means the path is actually gone. This covers the probe sweep and the main review-worktree sweep in one place, rather than duplicating the fallback at each call site. `rmSync` unlinks a symlink instead of following it, so a tampered leftover cannot redirect the delete. Test: `git.integration.test.ts` — an unregistered non-empty leftover is removed and the path is add-able again.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
…y too More review on #6841. Two accuracy/consistency fixes: - The probe's post-run `finally` still discarded the tree with the throwing `git()` wrapper only — the one sweep site that had not learned the unregistered/non-empty-leftover lesson the pre-sweep and `releaseWorktree` already did. Give it the same two steps (`worktree remove` best-effort, then `rmSync` whatever remains) and key `cleanupFailure` off whether the path still exists afterward, which is the honest signal. - `releaseWorktree`'s JSDoc still said it returns "whether a live worktree was there to remove"; after the rmSync fallback it also returns true for an untracked leftover it clears, and a true return now means the path is gone. Reworded to match. Also noted at the probe pre-sweep why it keeps `releaseWorktree`'s two-step inline rather than calling it: the probe must target this repo via `cwd: worktree`, and it keeps the sweep's stderr for the create-failure detail. No behaviour change on the happy path (full review suite green, 417).
Closes the last open thread on #6841: the `sweepErr` diagnostic had no coverage. The branch it lives on fires only when `git worktree add` fails, and there is no portable way to force that in a real-git test. The one lever — making `.git/worktrees` unwritable — is bypassed by root and behaves differently under CI's unprivileged user, so a test built on it would assert one thing locally and another in CI, which is worse than no test. So extract the part that actually has logic in it. `probeCreateFailureDetail` is pure: it names the `add` failure and folds in the stale-sweep's stderr, which is usually what explains it. Unit tests pin the fold, the omission of an empty sweep clause (a dangling "(stale-tree sweep also reported: )" would be report noise), and a non-Error throw. What stays untested is only the try/catch wiring.
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
qwen-code-ci-bot
previously approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Downgraded from Request changes because this is a self-PR (GitHub does not allow REQUEST_CHANGES on your own PR).
Reviewed — no unreported blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
…s reason
Three findings on the last round, two of them regressions I introduced.
- **`releaseWorktree` must not throw.** The `rmSync` I added gave it a throw path
it never had: `force` suppresses ENOENT but not EPERM or EBUSY, and this runs
on the cleanup path, where an exception masks the error that got us there —
which is exactly what its `gitOpt`-only body was avoiding, and what its
"does not throw when git itself fails" test pins. The remove is now swallowed
like every other failure here, and the outcome is reported through the return
value instead: `true` now means the path is GONE (`existed && !existsSync`),
not merely unregistered, so a caller can print "Removed …" without lying.
- **The discard lost its reason.** Keying `cleanupFailure` off "does the path
still exist" was right, but it dropped the `: ${e.message}` the old code
carried. A bare "could not remove <path>" tells whoever has to delete the tree
by hand nothing about why they must. Restored, preferring the `rmSync`
exception and falling back to what git said when it refused to unregister.
- **The two-step was duplicated within the file.** Pre-sweep and post-run discard
now share `discardWorktree()`, whose doc carries the reason it is not a call to
`releaseWorktree` (it needs `cwd: worktree` and the sweep's stderr).
`probeCleanupFailureDetail` is extracted and unit-tested for the same reason its
sibling `probeCreateFailureDetail` was: the branch cannot be forced portably, but
the composition is where the logic is. Full review suite green (423).
Collaborator
Author
|
@qewn-code /triage |
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Two findings on the last round, and the first is a regression I shipped. **A leftover we could not delete went silent.** Making `releaseWorktree` return "the path is free now" stopped it lying — cleanup no longer prints "Removed …" over a directory that is still on disk — but it swapped one failure for another: `cleanup.ts` only prints on `true`, so a path it could NOT free now produces no output at all. The leftover survives, wedges the next `git worktree add`, and nobody is told. A boolean cannot say both "there is still something there" and "here is why", and the caller needs both. So it returns a result: `existed`, `freed`, and `reason` when it is still there (the `rmSync` errno if there was one — EPERM/EBUSY, which `force` does not suppress — otherwise a plain statement of the situation). `cleanup.ts` now reports in both directions: "Removed …" only when the path is actually gone, and "Failed to remove … : <reason>" on stderr when it is not. **And the failure branch is now tested.** It needs `rmSync` to hit EPERM/EBUSY, which nothing portable forces: root bypasses the permission lever, CI's unprivileged user behaves differently, and a `node:fs` module mock does not reach this module under the suite's config (verified — the mock applies to the fs namespace but `git.ts`'s binding stays real). So the ruling is a pure function, `worktreeReleaseResult`, tested for all five shapes — same treatment the two probe-detail helpers got, for the same reason. Full review suite green (428).
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
reviewed
Jul 14, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the full diff against 8542b489e. Logic is correct across all three sweep sites; the boolean → WorktreeRelease change has no other consumer that reads the return value (fetch-pr.ts discards it); and the two new integration tests are genuine regression tests — they fail without the rmSync fallback. The deliberately-untested add-failure branch is justified, and the logic in it is extracted into pure, unit-tested helpers.
One doc-placement issue worth fixing before merge (inline on git.ts), plus one rare cosmetic nit on cleanup.ts. Everything else LGTM.
… contradicting stderr Two from review, both mine. - Extracting `worktreeReleaseResult` left `releaseWorktree`'s doc comment stranded: the block describing it (the `prune`-before-`branch -D` deadlock, the registered-but-missing worktree) ended up above the new pure helper, and `releaseWorktree` itself was left undocumented. Moved back where it belongs. - `cleanup.ts` treated "failed to remove" as if nothing had happened: the stderr line went out, but `removedAny` stayed false, so the run went on to announce `Nothing to clean for target "…"` on stdout — the two streams contradicting each other, and the stdout half being the one a script reads. "Nothing to clean" is a claim about the tree, not about this run's luck: it is only true when there was nothing there, not when there was and we could not get rid of it. Tracked with a separate `failedAny`, which also closes the same latent contradiction on the temp-file removal path, where it predates this PR.
doudouOUC
approved these changes
Jul 14, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Re-reviewed at 86e4df16a: the doc-comment un-stranding and the failedAny fix both landed correctly. The core logic across all three sweep sites, the WorktreeRelease return-type change (no other consumer reads the return value), the extracted pure helpers, and the two new regression tests all hold up. Approving.
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #6836 (the disposable-probe-worktree change), addressing the review left on it. Everything here is one theme:
git worktree removealone does not free a path, and every sweep site had to learn that.Share the probe-worktree path. Extract
probeWorktreePath(worktree)intolib/paths.ts; both the probe (test-efficacy.ts) andcleanup.tscall it. The-probesuffix was constructed independently in the two files — rename it in one and miss the other, and cleanup silently stops sweeping. The helper also settles the absolute-vs-relative divergence: it returns absolute, because the probe drivesgit worktree add/removewith the shared worktree as cwd, where a relative path would resolve against that worktree and nest the probe tree inside it.Clear an unregistered leftover, everywhere.
git worktree remove --forceonly unregisters a tree git still tracks. A directory left at the path after metadata loss or a partial cleanup is reported "not a working tree" and left in place — and a non-empty one then makesgit worktree addfailalready exists, wedging every probe asinconclusiveuntil someone clears it by hand (confirmed against real git 2.47; an empty leftover git reuses fine). Three sites needed the same two-step (unregister, thenrmSyncwhatever remains):worktree add;releaseWorktreeinlib/git.ts— fixed at the root, socleanup.ts's probe sweep and the main review-worktree sweep both get it. Itstruereturn now means the path is gone, not merely unregistered (JSDoc updated to say so); previouslycleanup.tsprinted "Removed probe worktree" while the directory was still there;finally, whosecleanupFailureis now keyed off whether the path still exists rather than off a git throw — the honest signal.rmSyncunlinks a symlink instead of following it, so a tampered leftover cannot redirect the delete.Explain the
addfailure when it happens. The pre-sweep's stderr is captured and folded into the create-failure detail — whenaddfails on a leftover the sweep could not clear, the sweep is what says why.Verification
probeWorktreePath(suffix, relative→absolute, single shared source);probeCreateFailureDetail(sweep stderr folded in, no dangling empty clause, non-Errorthrow).rmSyncfallback);releaseWorktreeremoves an untracked leftover and leaves the pathworktree add-able again.prettier/eslint/tsc --noEmitclean; full review suite green (420, +9).One thing deliberately not tested
The
catchthat fires whengit worktree additself fails. There is no portable way to force it: the only lever — making.git/worktreesunwritable — is bypassed by root and behaves differently under CI's unprivileged user, so a test built on it would assert one thing locally and another in CI, which is worse than no test. Rather than leave the branch bare, the part that actually has logic in it was extracted (probeCreateFailureDetail) and unit-tested. What remains uncovered is only the try/catch wiring.Addresses the review comments on #6836.
中文说明（点击展开）
这个 PR 做什么
#6836（把 test-efficacy probe 放进一次性 worktree）的后续，处理其上的审查意见。所有改动围绕同一个主题：
git worktree remove单独用并不能真正释放路径，而每一处清扫点都得学会这件事。1. 共享 probe worktree 路径
把
probeWorktreePath(worktree)抽到lib/paths.ts，probe（test-efficacy.ts）和cleanup.ts都调它。此前-probe后缀在两个文件里各写一遍 —— 改一处漏一处，cleanup 就会静默停止清扫。这个 helper 同时定了路径规范化：返回绝对路径。因为 probe 执行
git worktree add/remove时 cwd 是共享 worktree，相对路径会以那棵树为基准解析，把 probe 树嵌套进它内部。2. 清掉「未注册的残留目录」—— 三处都要
git worktree remove --force只能注销 git 仍在跟踪的树。若因元数据丢失或清理中断，路径上留下一个 git 不再跟踪的目录，git 会报 "not a working tree" 并原样留着 —— 而非空的残留会让接下来的git worktree add失败于already exists，把每次 probe 都卡成inconclusive，直到有人手工清理。（已用真实 git 2.47 复现确认；顺带发现空目录 git 会正常复用，只有非空才卡。）三个地方需要同样的两步（先注销，再
rmSync掉残留的任何东西）：worktree add之前的预清扫；lib/git.ts的releaseWorktree—— 修在根上，这样cleanup.ts的 probe 清扫和主 review worktree 清扫一起受益。它返回true现在意味着「路径确实没了」，而不只是「已注销」（JSDoc 已同步更新）；此前cleanup.ts会打印 "Removed probe worktree"，而目录其实还在 —— 这是句谎报；finally，其cleanupFailure现在依据「路径是否仍存在」判定，而不是依据 git 是否抛异常 —— 后者才是诚实的信号。rmSync对符号链接是解链接而非跟随，所以即便残留被做了手脚，也无法把删除重定向到树外。3.
add失败时要能解释原因预清扫的 stderr 被捕获并折入创建失败的 detail —— 当
add因清扫没能清掉的残留而失败时，正是清扫的输出在说明原因。验证
probeWorktreePath（后缀、相对→绝对、两处调用同源）；probeCreateFailureDetail（stderr 被折入、为空时不留悬空子句、非Error抛出）。rmSync兜底此测试即失败）；releaseWorktree能移除未跟踪的残留，路径重新可worktree add。prettier/eslint/tsc --noEmit均干净；review 全套测试通过（420，+9）。一处刻意不测
git worktree add自身失败时进入的catch。没有可移植的办法强制触发它：唯一的手段 —— 让.git/worktrees不可写 —— 在 root 下会被直接绕过，而 CI 跑的是非特权用户，两边行为不同。这样的测试会「本地断言一套、CI 断言另一套」，比没有测试更糟。因此没有硬造这个测试，而是把其中真正有逻辑的部分抽了出来（
probeCreateFailureDetail）并做了单元测试。剩下未覆盖的，只有 try/catch 的接线本身，那里没有逻辑。