mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-25 16:44:36 +00:00
fix(review): repair permissions before giving up on worktree cleanup (#9748)
* 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 <qwen-coder@alibabacloud.com> * 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 <qwen-coder@alibabacloud.com> * fix(review): strip CR and LF from registered-worktree skip warnings (#9748) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
This commit is contained in:
Shaojin Wen
GitHub
parent
be891657f7
commit
57285a94f1
3 changed files with 555 additions and 11 deletions
2
.github/workflows/.size-baseline
vendored
2
.github/workflows/.size-baseline
vendored
|
|
@ -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
|
||||
|
|
|
|||
95
.github/workflows/qwen-code-pr-review.yml
vendored
95
.github/workflows/qwen-code-pr-review.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
}
|
||||
},
|
||||
);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue