refactor(review): share the probe-worktree path helper; harden the stale-tree sweep

packages/cli/src/commands/review/cleanup.ts

@@ -19,6 +19,7 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import { refExists, releaseWorktree } from './lib/git.js';
 import {
   worktreePath,
+  probeWorktreePath,
   reviewBranch,
   REVIEW_TMP_DIR,
   tmpPrefix,
@@ -30,28 +31,43 @@ interface CleanupArgs {
 
 function runCleanup(target: string): void {
   let removedAny = false;
+  // Tracked separately from `removedAny`, because a failure is neither. Without
+  // it, a run that could not delete something goes on to announce "Nothing to
+  // clean" on stdout while stderr says it failed to remove a thing that is very
+  // much still there — the two streams contradicting each other, and the stdout
+  // half being the one a script reads.
+  let failedAny = false;
 
   // --- Worktree + branch (only for PR targets) -------------------------
   const prMatch = /^pr-(\d+)$/.exec(target);
   if (prMatch) {
     const prNumber = prMatch[1];
 
+    // Report what actually happened, in both directions. Announcing "Removed …"
+    // off a path that is still on disk is a lie; saying nothing at all when we
+    // could not remove it leaves a leftover that will wedge the next run's
+    // `git worktree add` with nobody told why. Both have been shipped here.
+    const report = (label: string, path: string) => {
+      const { existed, freed, reason } = releaseWorktree(path);
+      if (freed) {
+        writeStdoutLine(`Removed ${label}: ${path}`);
+        removedAny = true;
+      } else if (existed) {
+        writeStderrLine(`Failed to remove ${label} ${path}: ${reason}`);
+        failedAny = true;
+      }
+    };
+
     const wt = worktreePath(prNumber);
     // Prunes a registration left behind by a hand-deleted directory, which is
     // also what unblocks the `git branch -D` below.
-    if (releaseWorktree(wt)) {
-      writeStdoutLine(`Removed worktree: ${wt}`);
-      removedAny = true;
-    }
+    report('worktree', wt);
 
     // The test-efficacy probe runs in a disposable sibling worktree and removes
     // it itself; sweep one a crashed probe left behind so it does not block the
-    // next run's `git worktree add` (see #6832 / test-efficacy.ts).
-    const probeWt = `${wt}-probe`;
-    if (releaseWorktree(probeWt)) {
-      writeStdoutLine(`Removed probe worktree: ${probeWt}`);
-      removedAny = true;
-    }
+    // next run's `git worktree add` (see #6832 / test-efficacy.ts). Shares the
+    // path helper with the probe so the suffix cannot drift between the two.
+    report('probe worktree', probeWorktreePath(wt));
 
     const branch = reviewBranch(prNumber);
     if (refExists(branch)) {
@@ -87,10 +103,14 @@ function runCleanup(target: string): void {
       removedAny = true;
     } catch (err) {
       writeStderrLine(`Failed to remove ${full}: ${(err as Error).message}`);
+      failedAny = true;
     }
   }
 
-  if (!removedAny) {
+  // "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.
+  if (!removedAny && !failedAny) {
     writeStdoutLine(`Nothing to clean for target "${target}".`);
   }
 }

packages/cli/src/commands/review/lib/git.integration.test.ts

@@ -71,13 +71,38 @@ describe('releaseWorktree', () => {
     git('worktree', 'add', '-q', 'wt', '-b', 'topic');
     expect(existsSync(join(repo, 'wt'))).toBe(true);
 
-    expect(releaseWorktree(join(repo, 'wt'))).toBe(true);
+    expect(releaseWorktree(join(repo, 'wt'))).toMatchObject({
+      existed: true,
+      freed: true,
+    });
 
     expect(existsSync(join(repo, 'wt'))).toBe(false);
     // Not `.not.toContain('wt')` — the fixture's own path holds that substring.
     expect(git('worktree', 'list')).not.toContain(join(repo, 'wt'));
   });
 
+  it('removes an unregistered non-empty leftover git no longer tracks', () => {
+    // A crashed run can leave a directory at the worktree path that git does not
+    // track as a worktree. `git worktree remove` says "not a working tree" and
+    // leaves it, and a non-empty one then blocks the next `worktree add` with
+    // `already exists`. releaseWorktree must still leave the path gone.
+    mkdirSync(join(repo, 'wt', 'junk'), { recursive: true });
+    writeFileSync(join(repo, 'wt', 'junk', 'f'), 'x');
+    // Negative control: it is not a registered worktree.
+    expect(git('worktree', 'list')).not.toContain(join(repo, 'wt'));
+
+    expect(releaseWorktree(join(repo, 'wt'))).toMatchObject({
+      existed: true,
+      freed: true,
+    });
+
+    expect(existsSync(join(repo, 'wt'))).toBe(false);
+    // And the path is reusable — the `already exists` wedge is gone.
+    expect(() =>
+      git('worktree', 'add', '-q', 'wt', '-b', 'topic'),
+    ).not.toThrow();
+  });
+
   it('frees a path whose directory was deleted by hand', () => {
     // What `rm -rf .qwen/tmp` does to a review worktree.
     git('worktree', 'add', '-q', 'wt', '-b', 'topic');
@@ -88,7 +113,11 @@ describe('releaseWorktree', () => {
       /missing but already registered/,
     );
 
-    expect(releaseWorktree(join(repo, 'wt'))).toBe(false); // nothing to remove
+    // Nothing was there: not an existence, and nothing to free.
+    expect(releaseWorktree(join(repo, 'wt'))).toMatchObject({
+      existed: false,
+      freed: false,
+    });
     expect(() => git('worktree', 'add', '-q', 'wt', 'topic')).not.toThrow();
   });
 
@@ -108,7 +137,10 @@ describe('releaseWorktree', () => {
   });
 
   it('is a no-op when there is nothing registered', () => {
-    expect(releaseWorktree(join(repo, 'never-existed'))).toBe(false);
+    expect(releaseWorktree(join(repo, 'never-existed'))).toMatchObject({
+      existed: false,
+      freed: false,
+    });
     expect(git('worktree', 'list').trim().split('\n')).toHaveLength(1);
   });
 

packages/cli/src/commands/review/lib/git.test.ts

@@ -0,0 +1,63 @@
+/**
+ * @license
+ * Copyright 2026 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// `releaseWorktree`'s I/O is pinned against real git in `git.integration.test.ts`.
+// What is pinned here is the one outcome real git cannot produce on demand: the
+// path was there, and we could NOT free it. That needs `rmSync` to hit EPERM or
+// EBUSY, and nothing portable forces those — as root the permission lever is
+// bypassed outright, under CI's unprivileged user it behaves differently, and a
+// `node:fs` module mock does not reach the module under this suite's config. So
+// the ruling is a pure function, and it is tested as one.
+
+import { describe, it, expect } from 'vitest';
+import { worktreeReleaseResult } from './git.js';
+
+describe('worktreeReleaseResult', () => {
+  it('reports a path that was there and is gone now', () => {
+    expect(worktreeReleaseResult(true, false)).toEqual({
+      existed: true,
+      freed: true,
+      reason: undefined,
+    });
+  });
+
+  it('reports nothing-to-do without inventing a reason', () => {
+    // Nothing was there. Not a failure, so no reason — cleanup should stay quiet
+    // rather than announce a removal it did not perform.
+    expect(worktreeReleaseResult(false, false)).toEqual({
+      existed: false,
+      freed: false,
+      reason: undefined,
+    });
+  });
+
+  it('carries the rmSync error out when the path survived', () => {
+    // The outcome a boolean return could not express, and the one that made
+    // cleanup either lie ("Removed …") or go silent — both were shipped here and
+    // caught in review. `existed && !freed` must arrive with the WHY attached:
+    // someone has to delete this tree by hand.
+    const got = worktreeReleaseResult(
+      true,
+      true,
+      new Error("EBUSY: resource busy or locked, rmdir '/w/wt'"),
+    );
+    expect(got).toMatchObject({ existed: true, freed: false });
+    expect(got.reason).toContain('EBUSY');
+  });
+
+  it('names the situation when the path survived with no exception to quote', () => {
+    // `rmSync` returned cleanly and the path is still there anyway. There is no
+    // errno to hand over, but `reason` must not come back undefined — a silent
+    // "still there" is exactly the failure mode being fixed.
+    const got = worktreeReleaseResult(true, true);
+    expect(got).toMatchObject({ existed: true, freed: false });
+    expect(got.reason).toMatch(/still there/);
+  });
+
+  it('stringifies a non-Error throw rather than dropping it', () => {
+    expect(worktreeReleaseResult(true, true, 'boom').reason).toBe('boom');
+  });
+});

packages/cli/src/commands/review/lib/git.ts

@@ -9,7 +9,7 @@
 // across platforms.
 
 import { execFileSync } from 'node:child_process';
-import { existsSync } from 'node:fs';
+import { existsSync, rmSync } from 'node:fs';
 
 /** Deadline for a single `git` invocation. Generous; a hang must still end. */
 const GIT_TIMEOUT_MS = 120_000;
@@ -84,9 +84,63 @@ export function refExists(ref: string): boolean {
   return gitOpt('rev-parse', '--verify', '--quiet', ref) !== null;
 }
 
+/** What `releaseWorktree` found at the path, and what it managed to do about it. */
+export interface WorktreeRelease {
+  /** Something was at the path when we started. */
+  existed: boolean;
+  /** The path is free now — a `git worktree add` over it will succeed. */
+  freed: boolean;
+  /**
+   * Why the path is not free, set only when `existed && !freed`. A boolean
+   * cannot say both "there is still something there" and "here is why", and a
+   * caller that has to hand the problem to a human needs the second half:
+   * without it, cleanup either lies ("Removed …") or goes silent, and both were
+   * shipped and caught in review.
+   */
+  reason?: string;
+}
+
+/**
+ * Rule on a release attempt: what was there, what is there now, what went wrong.
+ *
+ * Pure, and extracted for that reason. The interesting outcome — `existed` but
+ * not `freed` — needs `rmSync` to hit EPERM or EBUSY, and nothing portable
+ * forces those: as root the permission lever is bypassed outright, and under
+ * CI's unprivileged user it behaves differently, so a `chmod`-based test would
+ * assert one thing locally and another in CI. Mocking `node:fs` does not reach
+ * this module under the suite's config either. The composition is where the
+ * logic lives, so it is testable here on its own.
+ *
+ * `reason` is never left unset when the path survived: a caller that has to tell
+ * a human "this is still on disk" is useless without "and here is why", so when
+ * there is no exception to quote it names the situation instead.
+ */
+export function worktreeReleaseResult(
+  existed: boolean,
+  stillThere: boolean,
+  removeError?: unknown,
+): WorktreeRelease {
+  const freed = existed && !stillThere;
+  if (!existed || freed) {
+    return { existed, freed, reason: undefined };
+  }
+  return {
+    existed,
+    freed,
+    reason: removeError
+      ? removeError instanceof Error
+        ? removeError.message
+        : String(removeError)
+      : 'the path is still there after `git worktree remove --force` and `rm -rf`',
+  };
+}
+
 /**
- * Free a review worktree's path **and** its branch. Returns whether a live
- * worktree was there to remove.
+ * Free a review worktree's path **and** its branch.
+ *
+ * Never throws — see the `rmSync` below. Reports what happened through the
+ * result: `existed` (something was there), `freed` (it is gone now), and
+ * `reason` when it is still there.
  *
  * `git worktree remove` needs the directory. A user reclaiming disk with
  * `rm -rf .qwen/tmp` leaves the worktree *registered but missing*, and from then
@@ -102,13 +156,32 @@ export function refExists(ref: string): boolean {
  * registration and a no-op when nothing is stale — run it unconditionally, and
  * **before** the branch delete that depends on it.
  */
-export function releaseWorktree(worktreePath: string): boolean {
+export function releaseWorktree(worktreePath: string): WorktreeRelease {
   const existed = existsSync(worktreePath);
+  let removeError: unknown;
   if (existed) {
     gitOpt('worktree', 'remove', worktreePath, '--force');
+    // `worktree remove` only clears 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 blocks the next
+    // `worktree add` with `already exists`. So remove whatever remains. `rmSync`
+    // unlinks a symlink rather than following it, so a tampered leftover cannot
+    // redirect the delete.
+    //
+    // Not allowed to throw, like every other failure here: `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. But the reason must not be
+    // lost either — a caller that has to tell a human "this path is still there"
+    // is useless without "and here is why". So: caught, and carried out in the
+    // result.
+    try {
+      rmSync(worktreePath, { recursive: true, force: true });
+    } catch (e) {
+      removeError = e;
+    }
   }
   gitOpt('worktree', 'prune');
-  return existed;
+  return worktreeReleaseResult(existed, existsSync(worktreePath), removeError);
 }
 
 /**

packages/cli/src/commands/review/lib/paths.test.ts

@@ -5,7 +5,8 @@
  */
 
 import { describe, it, expect } from 'vitest';
-import { tmpFile } from './paths.js';
+import { resolve } from 'node:path';
+import { tmpFile, probeWorktreePath, worktreePath } from './paths.js';
 
 describe('tmpFile — target is a single safe component', () => {
   it('keeps ordinary labels intact', () => {
@@ -34,3 +35,28 @@ describe('tmpFile — target is a single safe component', () => {
     expect(p.split('.qwen/tmp/')[1]).not.toContain('/');
   });
 });
+
+describe('probeWorktreePath', () => {
+  it('appends -probe to an absolute worktree path', () => {
+    expect(probeWorktreePath('/a/b/review-pr-1')).toBe(
+      '/a/b/review-pr-1-probe',
+    );
+  });
+
+  it('resolves a relative worktree to absolute so it never depends on cwd', () => {
+    // The probe drives `git worktree add` with the shared worktree as cwd, so a
+    // relative probe path would resolve against that worktree and nest the probe
+    // tree inside it. Absolute keeps it a sibling wherever it is called from.
+    expect(probeWorktreePath('.qwen/tmp/review-pr-1')).toBe(
+      `${resolve('.qwen/tmp/review-pr-1')}-probe`,
+    );
+  });
+
+  it('is the single source of the -probe suffix both call sites share', () => {
+    // cleanup.ts sweeps `probeWorktreePath(worktreePath(n))`; the probe creates
+    // `probeWorktreePath(worktree)`. One helper, one suffix — they cannot drift.
+    expect(probeWorktreePath(worktreePath(7))).toBe(
+      `${resolve(worktreePath(7))}-probe`,
+    );
+  });
+});

packages/cli/src/commands/review/lib/paths.ts

@@ -9,7 +9,7 @@
 // when the command is invoked). Use `path.join` rather than string
 // concatenation so Windows backslashes are produced when needed.
 
-import { join } from 'node:path';
+import { join, resolve } from 'node:path';
 
 export const REVIEW_TMP_DIR = join('.qwen', 'tmp');
 export const REVIEWS_DIR = join('.qwen', 'reviews');
@@ -20,6 +20,23 @@ export function worktreePath(prNumber: string | number): string {
   return join(REVIEW_TMP_DIR, `review-pr-${prNumber}`);
 }
 
+/**
+ * The disposable worktree the test-efficacy probe runs in — a sibling of the
+ * shared review worktree, discarded wholesale when the probe finishes (#6832).
+ *
+ * The one exception to this file's "paths are relative to the project root"
+ * rule: this returns an ABSOLUTE path. The probe drives `git worktree add`/
+ * `remove` with the shared worktree as cwd, so a relative path would resolve
+ * against that worktree, not the repo root, and land the probe tree nested
+ * inside the tree it is meant to sit beside. Both call sites — the probe and
+ * `cleanup.ts`'s stale-tree sweep — go through here so the `-probe` suffix and
+ * this normalisation stay in one place; renaming the suffix in one file used to
+ * silently stop the other from sweeping.
+ */
+export function probeWorktreePath(worktree: string): string {
+  return `${resolve(worktree)}-probe`;
+}
+
 /** Local branch ref name for a fetched PR head. */
 export function reviewBranch(prNumber: string | number): string {
   return `qwen-review/pr-${prNumber}`;