From 57285a94f1cb05d199fa2eba3fe29938df9e79d3 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 23 Aug 2026 14:27:46 +0000 Subject: [PATCH] fix(review): repair permissions before giving up on worktree cleanup (#9748) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(review): repair permissions before giving up on worktree cleanup The review job's end-of-job sweep gave up on the first EACCES and left foreign-owned leftovers in the shared runner workspace; the next review's checkout then died on them (run 32577821716, PR #9718: a scratch-verify tree whose contents this job's user could not unlink, on a pool member without passwordless sudo). Give the removal a repair ladder — chmod what this user owns, then passwordless sudo chown/chmod where the pool member has it, each followed by a retry — and refuse the ladder on paths that resolve through symlinks, since its sudo leg escalates to root. Members without sudo still degrade to a named warning: nothing unprivileged can remove a foreign-owned tree, but the sweep must never fail the job. Pin the ladder in the cleanup contract test so a rewrite cannot silently drop it back to warn-and-leave. * fix(ci): record qwen-code-pr-review.yml's shipped size in the workflow size baseline The permission-repair ladder added to the review cleanup step (repair before giving up on a worktree removal, refuse the sudo leg through symlinks) plus its incident comments grew the file past its recorded size plus allowance. The growth is the fix itself — the repair logic and the rationale a future reader needs — not drift, so record the shipped size rather than trimming the rationale. * fix(review): pin the repair ladder by effect and enrich its failure warnings Review feedback on the permission-repair ladder: - Pin the ladder's effect in the contract test (three removal attempts, isolated non-sudo chmod rung, refusal-comparison direction) — the old mechanism substrings stayed green when the post-repair retry was deleted, when the non-sudo rung was deleted, and when the refusal comparison was inverted (all reproduced by mutation before the fix). - Retry the removal after the chmod rung so a chmod-repaired tree never escalates to passwordless sudo; the step comment's "each followed by a retry" is now literally true. - Strip newlines from leftover paths before echoing: leftover names are untrusted glob entries, and a fresh line on the runner's stdout would parse as a workflow command. - Both warnings now carry the deciding state: the refusal names the branch that fired; the failure warning reports the sudo probe result and the survivor's owner. - Return 0 unconditionally so a failed warning echo can never fail the if: always() job via errexit. * fix(review): close the remaining command-injection entrances in worktree warnings (#9748) Co-authored-by: Qwen-Coder * test(review): execute remove_review_tree against fixtures and pin its sudo ok-state (#9748) * test(review): gate the removal-failure fixture on realpath and pin the ladder's guards (#9748) * test(review): execute the ladder's unpinned arms against behavioral fixtures (#9748) Co-authored-by: Qwen-Coder * fix(review): strip CR and LF from registered-worktree skip warnings (#9748) --------- Co-authored-by: qwen-code-ci-bot Co-authored-by: Qwen-Coder Co-authored-by: qwen-code-dev-bot --- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-code-pr-review.yml | 95 +++- .../review-worktree-cleanup-workflow.test.js | 469 +++++++++++++++++- 3 files changed, 555 insertions(+), 11 deletions(-) diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 849c5c3be4..9bfcc18fb4 100644 --- a/.github/workflows/.size-baseline +++ b/.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 +158010 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 386dda209c..1e437bf94f 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.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,67 @@ 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 CR and LF from every path + # expansion first: leftover names are untrusted glob entries, and a + # fresh line on the runner's stdout — which it splits on bare CR as + # well as LF — 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//[$'\r\n']/ }" + return 0 + fi + chmod -R u+rwX "$abs" 2>/dev/null || true + rm -rf "$abs" 2>/dev/null && return 0 + local sudo_probe='password-gated' + command -v sudo >/dev/null 2>&1 || 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//[$'\r\n']/ } (permission repair failed; sudo: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk 'NR==1 {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 }' \ @@ -1861,22 +1936,34 @@ jobs: # Registered paths come from leftover git metadata and are # untrusted: the awk filter above matched by substring, so reject # `..` traversal and re-anchor to the review prefix before the - # destructive remove. + # destructive remove. The skip warnings strip CR/LF from the + # path for the same reason the ladder's warnings do (above). case "$worktree" in */../*|../*|*/..) - echo "::warning::skipping suspicious review worktree path: $worktree" + echo "::warning::skipping suspicious review worktree path: ${worktree//[$'\r\n']/ }" continue ;; "$GITHUB_WORKSPACE/.qwen/tmp/review-pr-"*) : ;; *) - echo "::warning::skipping unexpected review worktree path: $worktree" + echo "::warning::skipping unexpected review worktree path: ${worktree//[$'\r\n']/ }" 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 diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index 281257875e..d8baf7e0a1 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -5,7 +5,19 @@ */ import { spawnSync } from 'node:child_process'; -import { readFileSync } from 'node:fs'; +import { + chmodSync, + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { parse } from 'yaml'; import { @@ -67,6 +79,12 @@ const reviewCleanStep = reviewCleanSteps[reviewCleanIndex].run; const agentStateCleanStep = reviewCleanSteps.find( (s) => s.name === 'Clean stale agent state', ).run; +// The step's owner-extraction awk is not a worktree filter: anchor on the +// filter's shape, not the first awk in the step. Derive it once here so the +// pinning test and the behavioral test always execute the same filter. +const worktreeFilter = reviewCleanStep.match( + /awk '(\$1 == "worktree"[^']+)'/, +)?.[1]; // Comments may name the recipe pieces out of order when explaining them, so // the order and isolation assertions below cover the commands only. @@ -124,6 +142,90 @@ function expectHardenedGit(run) { const awkAvailable = spawnSync('awk', ['BEGIN { exit 0 }']).status === 0; +// Substring/order pins cannot see parse or runtime behavior: a dropped +// closing quote fails `bash -n` on the whole `if: always()` step, and a +// dropped `]` in the existence check turns the ladder into a silent no-op — +// both mutants survive every pin above (mutation-probed). Execute the +// extracted function against fixture workspaces to catch that class. +const removeTreeFnStart = reviewCleanStep.indexOf('remove_review_tree() {'); +const removeReviewTreeFn = reviewCleanStep.slice( + removeTreeFnStart, + reviewCleanStep.indexOf('\n}\n', removeTreeFnStart) + 2, +); +const bashAvailable = spawnSync('bash', ['-c', 'exit 0']).status === 0; +// The fixtures defeat rm with a chmod-555 parent, which needs POSIX +// permission semantics: Git Bash on Windows resolves `bash` but not chmod, +// and root ignores the bits entirely. +const permissionFixturesAvailable = + bashAvailable && process.platform !== 'win32' && process.geteuid?.() !== 0; +// The ladder's failure path resolves the leftover with `realpath --`; on a +// host without it (the merge_group macOS lane ships none) the function +// refuses as `path could not be resolved` instead, so the test asserting +// the removal-failure warning gates on it separately — the plain-leftover +// fixture returns before resolution, and the symlink fixture's refusal +// branch fires before the resolved value is consumed. +const realpathAvailable = + spawnSync('realpath', ['--', '/'], { stdio: 'ignore' }).status === 0; +const runRemoveReviewTree = (workspace, ...args) => + spawnSync( + 'bash', + [ + '-c', + // Mirror the runner's flags: Actions runs the step under errexit, and + // the step's own `set -uo pipefail` first line does not turn it back + // off — an unguarded failing command inside the function must fail + // these tests exactly as it fails the `if: always()` step. + `set -euo pipefail\n${removeReviewTreeFn}\nremove_review_tree "$@"`, + 'remove_review_tree', + ...args, + ], + { env: { ...process.env, GITHUB_WORKSPACE: workspace }, encoding: 'utf8' }, + ); + +// The skip-warning fixture executes the whole step body with `git` +// stubbed to a function whose `worktree list --porcelain` returns +// hostile registrations: the echoes under test sit in the loop, not in +// git, and the stub keeps the fixture free of real worktree state. +const runReviewCleanStep = (workspace, hostileRegistrations) => + spawnSync( + 'bash', + [ + '-c', + [ + 'set -euo pipefail', + 'git() {', + ' case " $* " in', + ' *" worktree list "*) printf \'%s\\n\' "$HOSTILE_REGISTRATIONS" ;;', + ' esac', + '}', + reviewCleanStep, + ].join('\n'), + 'clean-review-worktrees', + ], + { + cwd: workspace, + env: { + ...process.env, + GITHUB_WORKSPACE: workspace, + HOSTILE_REGISTRATIONS: hostileRegistrations + .map((path) => `worktree ${path}`) + .join('\n'), + }, + encoding: 'utf8', + }, + ); + +// existsSync follows the link: a dangling leftover reports as absent while +// the link itself still survives, so link presence is asserted via lstat. +const linkExists = (path) => { + try { + lstatSync(path); + return true; + } catch { + return false; + } +}; + describe('review worktree cleanup steps', () => { it('keeps every shared-pool ci.yml checkout sweep pinned to paths.ts', () => { expect(ciCleanSteps.map(({ id }) => id)).toEqual( @@ -176,11 +278,149 @@ describe('review worktree cleanup steps', () => { expectHardenedGit(reviewCleanStep); // Fallback for worktree directories Git no longer knows about. expect(reviewCleanStep).toContain(`rm -rf ${worktreePrefix}*`); + // The leftover loop's glob is the only call site that feeds surviving + // permission-poisoned trees into the ladder: a rename here matches + // nothing, and every pin and fixture stays green while the sweep + // silently skips the trees it exists to heal. + expect(reviewCleanStep).toContain(`for leftover in ${worktreePrefix}*; do`); // Leases are session+prompt scoped so a stale one is inert, but the glob // must stay in sync with LEASE_PREFIX or it silently never matches. 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 first rm must run BEFORE the refusal guard: a guard-first rewrite + // refuses a symlinked leftover that the plain rm would simply have + // unlinked (measured: one spurious refusal, documented behavior gone). + const guardPos = reviewCleanCode.indexOf('if [ -n "$reason" ]'); + expect(reviewCleanCode.indexOf('rm -rf "$abs"')).toBeLessThan(guardPos); + // The non-sudo rung must exist as its own command, not just inside the + // sudo line, with its errexit guard intact: the leftover loop calls the + // function bare under the runner's -e, so an unguarded failing rung + // would kill the `if: always()` step mid-ladder. + expect(reviewCleanCode).toMatch( + /^\s*chmod -R u\+rwX "\$abs" 2>\/dev\/null \|\| true$/m, + ); + // The rung's retry rm must sit directly under it: hoisting the sudo + // block between the two escalates every chmod-repaired tree to sudo, + // breaking the never-escalates ordering the rm count pins above. + expect(reviewCleanCode).toMatch( + /^\s*chmod -R u\+rwX "\$abs"[^\n]*\n\s*rm -rf "\$abs"/m, + ); + // Both sudo rungs pinned in full: dropping the chmod leg or chowning to + // root leaves a foreign-owned tree owned-but-locked, so the retry rm + // still fails and the leftover survives the ladder built to heal it. + expect(reviewCleanCode).toContain( + 'sudo -n chown -R "$(id -u):$(id -g)" "$abs" 2>/dev/null || true', + ); + expect(reviewCleanCode).toContain( + 'sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true', + ); + 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"'); + // The realpath fallbacks keep the assignments errexit-safe: a leftover + // realpath cannot resolve (a symlink loop, or a dangling link with + // missing target ancestry, under a locked parent) must warn and + // continue, not die at the assignment and skip the trailing sweeps. + expect(reviewCleanCode).toContain( + 'abs_real="$(realpath -- "$abs" 2>/dev/null)" || abs_real=\'\'', + ); + expect(reviewCleanCode).toMatch( + /ws_real="\$\(realpath -- "\$GITHUB_WORKSPACE" 2>\/dev\/null\)" \|\|\n\s*ws_real="\$GITHUB_WORKSPACE"/, + ); + // Order and derivation are load-bearing too, not just presence + // (mutation-probed): with the refusal guard below the rungs, a rewrite + // chmod/chowns through a planted link before the check runs; with the + // sudo block above the chmod rung, a chmod-repaired tree escalates to + // sudo anyway; with rel blanked, every leftover refuses as "outside the + // workspace" and the incident this PR exists for recurs. + const chmodPos = reviewCleanCode.indexOf('chmod -R u+rwX "$abs"'); + const sudoPos = reviewCleanCode.indexOf('sudo -n chown -R'); + expect(guardPos).toBeGreaterThan(-1); + expect(guardPos).toBeLessThan(chmodPos); + expect(guardPos).toBeLessThan(sudoPos); + expect(chmodPos).toBeLessThan(sudoPos); + expect(reviewCleanCode).toContain('rel="${abs#"$GITHUB_WORKSPACE/"}"'); + // Leftover names are untrusted glob entries: every direct expansion in + // both warnings must strip CR as well as LF — the runner splits step + // stdout on bare CR too — and the owner enrichment must read only ls's + // first line, or a newline-bearing name's later lines are emitted + // standalone and a `::` among them parses as a workflow command. + const warningLines = removeReviewTreeFn + .split('\n') + .filter((line) => line.includes('::warning::')); + expect(warningLines).toHaveLength(2); + for (const line of warningLines) { + // Command substitutions pass the path as an argument, never to the + // log line; only direct interpolations reach stdout. + const direct = line.replace(/\$\([^()]*\)/g, ''); + expect(direct).not.toMatch(/\$\{abs[^/]|\$abs\b/); + } + expect( + reviewCleanCode.match(/\$\{abs\/\/\[\$'\\r\\n'\]\/ \}/g), + ).toHaveLength(2); + // The registered-worktree loop's two skip warnings reach the same + // stdout with an untrusted registered path, so the identical strip + // protects them: a bare `$worktree` there injects a standalone + // workflow-command line on the runner's stdout (executed by the + // CR-bearing-registration fixture below). + const skipWarningLines = reviewCleanCode + .split('\n') + .filter( + (line) => + line.includes('skipping suspicious review worktree') || + line.includes('skipping unexpected review worktree'), + ); + expect(skipWarningLines).toHaveLength(2); + for (const line of skipWarningLines) { + expect(line).not.toMatch(/\$worktree\b/); + } + expect( + reviewCleanCode.match(/\$\{worktree\/\/\[\$'\\r\\n'\]\/ \}/g), + ).toHaveLength(2); + expect(reviewCleanCode).toContain("awk 'NR==1 {print $3}'"); + // 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/, + ); + // The probe must keep all three sudo states apart: the incident this + // ladder exists for was a runner WITH sudo and no NOPASSWD entry, and + // "absent" sends the on-call to install a package instead of writing a + // sudoers rule. The 'ok' assignment is pinned for the mirror case: + // without it a working passwordless sudo reports as password-gated and + // sends the on-call to add a sudoers rule that already exists. + expect(reviewCleanCode).toContain("local sudo_probe='password-gated'"); + expect(reviewCleanCode).toContain( + "command -v sudo >/dev/null 2>&1 || sudo_probe='absent'", + ); + // The ok/password-gated split lives in the `sudo -n true` predicate: + // dropping it reports `sudo: ok` on exactly the NOPASSWD-less runners + // this ladder exists for, and mis-triages the on-call. + expect(reviewCleanCode).toContain( + 'command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null', + ); + expect(reviewCleanCode).toContain("sudo_probe='ok'"); }); it('keeps the pre-checkout agent-state sweep pinned to paths.ts', () => { @@ -192,20 +432,18 @@ describe('review worktree cleanup steps', () => { }); it('uses one identical worktree filter at every list-driven sweep', () => { - const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; - expect(filter).toBeTruthy(); + expect(worktreeFilter).toBeTruthy(); for (const { id, run } of ciCleanSteps) { - expect(run, id).toContain(`awk '${filter}'`); + expect(run, id).toContain(`awk '${worktreeFilter}'`); } }); it.skipIf(!awkAvailable)( 'filter selects review worktrees only, never the main checkout', () => { - const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; const main = '/home/runner/work/qwen-code/qwen-code'; const review = `${main}/.qwen/tmp/review-pr-42`; - const out = spawnSync('awk', [filter], { + const out = spawnSync('awk', [worktreeFilter], { input: [ `worktree ${main}`, `worktree ${review}`, @@ -218,4 +456,223 @@ describe('review worktree cleanup steps', () => { expect(out.stdout.trim()).toBe(review); }, ); + + it.skipIf(!permissionFixturesAvailable)( + 'remove_review_tree actually removes a plain leftover', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + try { + const leftover = join(fixture, '.qwen/tmp/review-pr-101'); + mkdirSync(leftover, { recursive: true }); + writeFileSync(join(leftover, 'leftover.txt'), 'x'); + // Relative input: the leftover loop's glob entries are relative. + const out = runRemoveReviewTree(fixture, '.qwen/tmp/review-pr-101'); + expect(out.status).toBe(0); + expect(out.stdout).toBe(''); + expect(existsSync(leftover)).toBe(false); + } finally { + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!permissionFixturesAvailable)( + 'remove_review_tree refuses a symlinked leftover without touching its target', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + try { + const target = join(fixture, 'target'); + mkdirSync(target); + writeFileSync(join(target, 'keep.txt'), 'x'); + const leftoverDir = join(fixture, '.qwen/tmp'); + mkdirSync(leftoverDir, { recursive: true }); + const link = join(leftoverDir, 'review-pr-102'); + symlinkSync(target, link); + // Make rm fail so the ladder reaches the refusal branch: with a + // writable parent the first rung unlinks the link itself, which is + // correct but never exercises the guard. + chmodSync(leftoverDir, 0o555); + const out = runRemoveReviewTree(fixture, link); + expect(out.status).toBe(0); + const warnings = out.stdout + .split('\n') + .filter((line) => line.startsWith('::warning::')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain( + 'refusing to repair review worktree path (path is a symlink)', + ); + expect(readFileSync(join(target, 'keep.txt'), 'utf8')).toBe('x'); + } finally { + chmodSync(join(fixture, '.qwen/tmp'), 0o755); + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!permissionFixturesAvailable || !realpathAvailable)( + 'remove_review_tree keeps a newline-bearing leftover name on one warning line', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + try { + const leftoverDir = join(fixture, '.qwen/tmp'); + mkdirSync(leftoverDir, { recursive: true }); + const hostile = join(leftoverDir, 'review-pr-\n::error::injected'); + mkdirSync(hostile); + chmodSync(leftoverDir, 0o555); + const out = runRemoveReviewTree(fixture, hostile); + expect(out.status).toBe(0); + expect(existsSync(hostile)).toBe(true); + // The runner parses every stdout line as a possible workflow + // command: the stripped name must stay inside the single warning + // line, never surface `::error::` on its own line. + const lines = out.stdout.split(/\r?\n/).filter((line) => line); + expect(lines).toHaveLength(1); + expect( + lines[0].startsWith('::warning::could not remove review worktree'), + ).toBe(true); + } finally { + chmodSync(join(fixture, '.qwen/tmp'), 0o755); + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!permissionFixturesAvailable)( + 'remove_review_tree unlinks a symlinked leftover over a writable parent', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + try { + const target = join(fixture, 'target'); + mkdirSync(target); + writeFileSync(join(target, 'keep.txt'), 'x'); + const leftoverDir = join(fixture, '.qwen/tmp'); + mkdirSync(leftoverDir, { recursive: true }); + const link = join(leftoverDir, 'review-pr-103'); + symlinkSync(target, link); + // The parent stays writable, so the first rm must unlink the link + // before the guard runs: deadening that rm routes the leftover to + // the symlink refusal instead, warning and leaving it behind. + const out = runRemoveReviewTree(fixture, link); + expect(out.status).toBe(0); + expect(out.stdout).toBe(''); + expect(linkExists(link)).toBe(false); + expect(readFileSync(join(target, 'keep.txt'), 'utf8')).toBe('x'); + } finally { + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!permissionFixturesAvailable)( + 'remove_review_tree removes a dangling symlink via the -L existence arm', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + try { + const leftoverDir = join(fixture, '.qwen/tmp'); + mkdirSync(leftoverDir, { recursive: true }); + const link = join(leftoverDir, 'review-pr-104'); + symlinkSync(join(fixture, 'missing-target'), link); + // -e follows the link and is false here: only the -L arm keeps the + // remove-or-named-warning contract for dangling links. + const out = runRemoveReviewTree(fixture, link); + expect(out.status).toBe(0); + expect(out.stdout).toBe(''); + expect(linkExists(link)).toBe(false); + } finally { + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!permissionFixturesAvailable)( + 'remove_review_tree refuses a path outside the workspace', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + const outside = mkdtempSync(join(tmpdir(), 'review-tree-outside-')); + const leftover = join(outside, 'review-pr-105'); + try { + mkdirSync(leftover); + writeFileSync(join(leftover, 'keep.txt'), 'x'); + // Lock the tree AND its parent: rm may run before the guard and + // unlinks anything it can, so both locks are needed to observe the + // refusal leaving a foreign tree completely untouched. + chmodSync(leftover, 0o555); + chmodSync(outside, 0o555); + const out = runRemoveReviewTree(fixture, leftover); + expect(out.status).toBe(0); + const warnings = out.stdout + .split('\n') + .filter((line) => line.startsWith('::warning::')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain( + 'refusing to repair review worktree path (outside the workspace)', + ); + expect(existsSync(join(leftover, 'keep.txt'))).toBe(true); + } finally { + chmodSync(outside, 0o755); + chmodSync(leftover, 0o755); + rmSync(fixture, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!permissionFixturesAvailable || !realpathAvailable)( + 'remove_review_tree repairs a permission-locked tree and then removes it', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-tree-fixture-')); + const leftover = join(fixture, '.qwen/tmp/review-pr-106'); + try { + mkdirSync(leftover, { recursive: true }); + writeFileSync(join(leftover, 'locked.txt'), 'x'); + // Lock the tree itself, not the parent: the rungs repair $abs only, + // so the chmod-555-parent fixtures never reach the repair-success + // path this exercises — first rm fails, chmod rung heals, retry rm + // removes. + chmodSync(leftover, 0o555); + const out = runRemoveReviewTree(fixture, '.qwen/tmp/review-pr-106'); + expect(out.status).toBe(0); + expect(out.stdout).toBe(''); + expect(existsSync(leftover)).toBe(false); + } finally { + if (existsSync(leftover)) chmodSync(leftover, 0o755); + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); + it.skipIf(!bashAvailable || !awkAvailable)( + 'skip warnings keep a CR-bearing registered path on one runner line', + () => { + const fixture = mkdtempSync(join(tmpdir(), 'review-skip-echo-fixture-')); + try { + // The step exits early without a checkout. + mkdirSync(join(fixture, '.git')); + const hostile = [ + // `..` routes to the suspicious-skip echo; the other two fail + // the workspace prefix check and route to the unexpected-skip + // echo. + `${fixture}/.qwen/tmp/review-pr-1/../pwn\r::stop-commands::pwned`, + `/elsewhere/.qwen/tmp/review-pr-2\r::endgroup::`, + `/elsewhere/.qwen/tmp/review-pr-3\r::notice::forged/git`, + ]; + const out = runReviewCleanStep(fixture, hostile); + expect(out.status).toBe(0); + // The runner splits step stdout on bare CR as well as LF and + // parses every line for workflow commands: the stripped path must + // stay inside its warning line, never surface a standalone `::` + // line. + const lines = out.stdout.split(/[\r\n]/).filter((line) => line); + expect( + lines.filter((line) => line.startsWith('::warning::skipping')), + ).toHaveLength(3); + expect( + lines.filter( + (line) => line.startsWith('::') && !line.startsWith('::warning::'), + ), + ).toEqual([]); + } finally { + rmSync(fixture, { recursive: true, force: true }); + } + }, + ); });