fix(review): repair permissions before giving up on worktree cleanup

.github/workflows/.size-baseline

@@ -36,7 +36,7 @@
 5942 qwen-autofix-fork-signal.yml
 397656 qwen-autofix.yml
 7061 qwen-ci-flaky-rerun.yml
-151937 qwen-code-pr-review.yml
+157847 qwen-code-pr-review.yml
 79041 qwen-fleet-shepherd.yml
 20525 qwen-issue-followup-bot.yml
 5760 qwen-pr-safety-precheck.yml

.github/workflows/qwen-code-pr-review.yml

@@ -1842,6 +1842,20 @@ jobs:
       # the next job on this reused runner can delete qwen-review/* branches.
       # The sweep deletes all review artifacts, not just this PR's: safe because
       # a runner executes one job at a time.
+      #
+      # The removal owns its own permission repair. A containerised job on this
+      # shared pool can leave a review worktree owned by another uid and
+      # read-only (measured, run 32577821716 / PR #9718: a leftover
+      # scratch-verify tree held files this job's user could not unlink, and
+      # the NEXT review's checkout died on them with EACCES — both the
+      # pre-checkout ownership restore and the checkout's own wipe degraded
+      # because the runner had no passwordless sudo). A removal that gives up
+      # on the first EACCES re-poisons the next job, so a failed rm gets a
+      # repair ladder instead: chmod what this user owns, then passwordless
+      # sudo chown/chmod where the pool member has it, each followed by a
+      # retry. Members without sudo still degrade to a named warning —
+      # nothing unprivileged can remove a foreign-owned tree — but the heal
+      # chain must never fail the job.
       - name: 'Clean review worktrees'
         if: 'always()'
         timeout-minutes: 5
@@ -1853,6 +1867,65 @@ jobs:
           fi
 
           GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")
+
+          # The repair ladder for one leftover tree (see the step comment).
+          # A path outside the workspace or resolving through symlinks is
+          # refused rather than repaired: the sudo leg escalates to root,
+          # and a planted link would aim a chown/chmod -R outside the
+          # workspace. Warning echoes strip newlines from the path first:
+          # leftover names are untrusted glob entries, and a fresh line on
+          # the runner's stdout would parse as a workflow command.
+          remove_review_tree() {
+            local abs="$1"
+            case "$abs" in
+              /*) : ;;
+              *) abs="$GITHUB_WORKSPACE/$abs" ;;
+            esac
+            [ -e "$abs" ] || [ -L "$abs" ] || return 0
+            rm -rf "$abs" 2>/dev/null && return 0
+            # Refuse a path that resolves through symlinks, but compare
+            # against the workspace's OWN resolved path: an ancestor the
+            # workspace itself sits under (a macOS /tmp -> /private/tmp
+            # local run) is legitimate and must not read as a redirect —
+            # only a symlink planted BELOW the workspace does. The refusal
+            # names the branch that fired so the on-call knows which case
+            # hit.
+            local ws_real rel abs_real reason=''
+            ws_real="$(realpath -- "$GITHUB_WORKSPACE" 2>/dev/null)" ||
+              ws_real="$GITHUB_WORKSPACE"
+            case "$abs" in
+              "$GITHUB_WORKSPACE"/*) rel="${abs#"$GITHUB_WORKSPACE/"}" ;;
+              *) rel='' ;;
+            esac
+            abs_real="$(realpath -- "$abs" 2>/dev/null)" || abs_real=''
+            if [ -z "$rel" ]; then
+              reason='outside the workspace'
+            elif [ -L "$abs" ]; then
+              reason='path is a symlink'
+            elif [ -z "$abs_real" ]; then
+              reason='path could not be resolved'
+            elif [ "$abs_real" != "$ws_real/$rel" ]; then
+              reason='resolves through symlinks'
+            fi
+            if [ -n "$reason" ]; then
+              echo "::warning::refusing to repair review worktree path (${reason}): ${abs//$'\n'/ }"
+              return 0
+            fi
+            chmod -R u+rwX "$abs" 2>/dev/null || true
+            rm -rf "$abs" 2>/dev/null && return 0
+            local sudo_probe='absent'
+            if command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null; then
+              sudo_probe='ok'
+              sudo -n chown -R "$(id -u):$(id -g)" "$abs" 2>/dev/null || true
+              sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true
+            fi
+            rm -rf "$abs" 2>/dev/null && return 0
+            echo "::warning::could not remove review worktree: ${abs//$'\n'/ } (permission repair failed; sudo: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk '{print $3}'))"
+            # return 0 even when the warning echo fails: the heal chain must
+            # never fail the job.
+            return 0
+          }
+
           "${GIT_SAFE[@]}" worktree prune -v || true
           "${GIT_SAFE[@]}" worktree list --porcelain \
             | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \
@@ -1873,10 +1946,21 @@ jobs:
                     continue
                     ;;
                 esac
+                # `git worktree remove` unlinks entries the same way rm does,
+                # so a foreign-owned entry defeats it too; the repair ladder
+                # retries it, and whatever git still leaves behind goes through
+                # the same ladder below (registrations are pruned afterwards).
                 "${GIT_SAFE[@]}" worktree remove --force "$worktree" ||
-                  echo "::warning::could not remove review worktree: $worktree"
+                  remove_review_tree "$worktree"
               done || true
           rm -rf .qwen/tmp/review-pr-* 2>/dev/null || true
+          # Survivors of the glob are exactly the permission-poisoned trees;
+          # run each through the repair ladder individually so one poisoned
+          # entry cannot mask its siblings.
+          for leftover in .qwen/tmp/review-pr-*; do
+            [ -e "$leftover" ] || [ -L "$leftover" ] || continue
+            remove_review_tree "$leftover"
+          done
           "${GIT_SAFE[@]}" worktree prune -v || true
           "${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
             | while read -r review_ref; do

scripts/tests/review-worktree-cleanup-workflow.test.js

@@ -181,6 +181,41 @@ describe('review worktree cleanup steps', () => {
     expect(reviewCleanStep).toContain(
       `rm -f ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json`,
     );
+    // A failed rm must not be left to poison the next job's checkout: the
+    // sweep owns its own permission repair — chmod, then passwordless sudo
+    // chown/chmod where the pool member has it — and retries the removal per
+    // leftover entry (measured, run 32577821716 / PR #9718: a foreign-owned
+    // scratch-verify tree killed the next review at checkout with EACCES).
+    // Pin the ladder's EFFECT, not mechanism substrings: those double-match
+    // (the non-sudo chmod rung hides inside the sudo line) and let a
+    // rewrite silently drop the ladder back to warn-and-leave.
+    const reviewCleanCode = stripComments(reviewCleanStep);
+    // Three removal attempts: the initial rm plus one retry after EACH
+    // repair rung, so a chmod-repaired tree never escalates to sudo.
+    expect(reviewCleanCode.match(/rm -rf "\$abs"/g)).toHaveLength(3);
+    // The non-sudo rung must exist as its own command, not just inside the
+    // sudo line.
+    expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs"/m);
+    expect(reviewCleanCode).toContain('sudo -n chown -R');
+    expect(reviewCleanCode).toContain('remove_review_tree "$leftover"');
+    // The symlink-refusal guard must survive, including the direction of
+    // its comparison and the deciding reason it now carries.
+    expect(reviewCleanCode).toContain(
+      'refusing to repair review worktree path (${reason})',
+    );
+    expect(reviewCleanCode).toContain('!= "$ws_real/$rel"');
+    // Leftover names are untrusted glob entries: both warnings must strip
+    // newlines, or a hostile name injects a workflow command into the log.
+    expect(reviewCleanCode.match(/\$\{abs\/\/\$'\\n'\/ \}/g)).toHaveLength(2);
+    // The failure warning carries the deciding state (sudo probe + owner),
+    // and the function returns 0 unconditionally: even a failed warning
+    // echo must not fail the `if: always()` job via errexit.
+    expect(reviewCleanCode).toMatch(
+      /could not remove review worktree[^\n]*sudo: \$sudo_probe[^\n]*owner:/,
+    );
+    expect(reviewCleanCode).toMatch(
+      /could not remove review worktree[^\n]*\n\s*return 0/,
+    );
   });
 
   it('keeps the pre-checkout agent-state sweep pinned to paths.ts', () => {
@@ -192,7 +227,9 @@ describe('review worktree cleanup steps', () => {
   });
 
   it('uses one identical worktree filter at every list-driven sweep', () => {
-    const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1];
+    // The step's owner-extraction awk is not a worktree filter: anchor
+    // on the filter's shape, not the first awk in the step.
+    const filter = reviewCleanStep.match(/awk '(\$1 == "worktree"[^']+)'/)?.[1];
     expect(filter).toBeTruthy();
     for (const { id, run } of ciCleanSteps) {
       expect(run, id).toContain(`awk '${filter}'`);
@@ -202,7 +239,11 @@ describe('review worktree cleanup steps', () => {
   it.skipIf(!awkAvailable)(
     'filter selects review worktrees only, never the main checkout',
     () => {
-      const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1];
+      // The step's owner-extraction awk is not a worktree filter: anchor
+      // on the filter's shape, not the first awk in the step.
+      const filter = reviewCleanStep.match(
+        /awk '(\$1 == "worktree"[^']+)'/,
+      )?.[1];
       const main = '/home/runner/work/qwen-code/qwen-code';
       const review = `${main}/.qwen/tmp/review-pr-42`;
       const out = spawnSync('awk', [filter], {