From c47974ccf4cb780a59dabcf47cbdb35f60a5c516 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 27 Aug 2026 13:43:14 +0800 Subject: [PATCH] fix(pr): release locks after linked worktree teardown (#130524) * fix(pr): release locks after linked worktree teardown * test(pr): verify native cleanup and retained lock ownership Initialize the existing script-local supervisor root before canonical Git resolution, and replace overlapping teardown fixtures with the real native merge and gc lifecycle. Co-authored-by: Vincent Koc --------- Co-authored-by: Peter Steinberger --- scripts/pr | 7 +- test/scripts/pr-operation-lock.test.ts | 225 ++++++++++++++++++------- 2 files changed, 164 insertions(+), 68 deletions(-) diff --git a/scripts/pr b/scripts/pr index 2d9572fbb7bc..8976ff1b284a 100755 --- a/scripts/pr +++ b/scripts/pr @@ -39,10 +39,11 @@ if [ "${1-}" = "--dev-wrapper" ]; then fi requested_subcommand="${1-}" -# If invoked from a linked worktree copy of this script, re-exec the canonical -# script from the repository root so behavior stays consistent across worktrees. +# Select trusted wrapper code independently from the canonical repository root; +# a linked wrapper may be removed by merge-run or gc before supervision ends. script_self="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/$(basename "${BASH_SOURCE[0]}")" script_parent_dir="$(dirname "$script_self")" +canonical_repo_root="$script_parent_dir/.." pr_wrapper_components=( scripts/pr scripts/pr-lib @@ -173,7 +174,7 @@ if [ "${1-}" = "gc" ] || is_locked_pr_command "${1-}"; then unset OPENCLAW_PR_LOCK_NOTIFY_FD unset OPENCLAW_PR_LOCK_SUPERVISOR_PID command -v node >/dev/null 2>&1 || { echo "Missing required command: node" >&2; exit 1; } - exec node "$script_parent_dir/pr-lib/process-group-runner.mjs" "$script_parent_dir/.." "$script_self" "$@" + exec node "$script_parent_dir/pr-lib/process-group-runner.mjs" "$canonical_repo_root" "$script_self" "$@" fi fi diff --git a/test/scripts/pr-operation-lock.test.ts b/test/scripts/pr-operation-lock.test.ts index d2a79b37ece6..a1b5415048db 100644 --- a/test/scripts/pr-operation-lock.test.ts +++ b/test/scripts/pr-operation-lock.test.ts @@ -207,6 +207,8 @@ function installPrCliFixture(repoDir: string) { "scripts/pr", "scripts/watch-pr-ci.mjs", "scripts/watch-pr-ci.mts", + "scripts/verify-pr-hosted-gates.mjs", + "scripts/verify-pr-hosted-gates.mts", "scripts/lib/plain-gh.sh", "scripts/lib/plain-gh.mjs", "scripts/lib/direct-run.mjs", @@ -250,9 +252,7 @@ function installRequiredPrCommandStubs(binDir: string) { interface SupervisedFixtureOptions { accelerateTimeouts?: boolean; - cwd?: string; env?: NodeJS.ProcessEnv; - runner?: string; } async function runSupervisedFixture( @@ -264,12 +264,12 @@ async function runSupervisedFixture( process.execPath, [ ...(options.accelerateTimeouts ? ["--require", createProcessGroupTimingPreload()] : []), - options.runner ?? processGroupRunner, + processGroupRunner, repoDir, fixture, ], { - cwd: options.cwd ?? repoDir, + cwd: repoDir, env: { ...process.env, ...options.env }, stdio: ["ignore", "pipe", "pipe"], }, @@ -1214,66 +1214,164 @@ describePosix("scripts/pr per-PR operation lock", () => { expect(result.stderr).not.toContain("Retaining the operation lock"); } }); - it("releases the lock after the operation deletes its runner worktree", async () => { - const repoDir = createRepo(); - const doomedDir = tempDirs.make("openclaw-pr-self-deleting-runner-"); - const copiedLibDir = join(doomedDir, "pr-lib"); - mkdirSync(copiedLibDir, { recursive: true }); - for (const file of ["operation-lock.sh", "process-group-runner.mjs"]) { - cpSync(join(repoRoot, "scripts/pr-lib", file), join(copiedLibDir, file)); - } - const copiedRunner = join(copiedLibDir, "process-group-runner.mjs"); - const fixture = join(doomedDir, "delete-own-worktree.sh"); - writeFileSync( - fixture, - [ + it.each([ + { wrapper: "canonical", command: "merge-run", failure: "none" }, + { wrapper: "linked", command: "merge-run", failure: "none" }, + { wrapper: "linked", command: "gc", failure: "none" }, + { wrapper: "linked", command: "merge-run", failure: "merge" }, + { wrapper: "linked", command: "merge-run", failure: "release" }, + ])( + "finishes native cleanup with $wrapper wrapper ($command, failure=$failure)", + ({ wrapper, command, failure }) => { + const repoDir = createRepo(); + const { binDir, cli } = installPrCliFixture(repoDir); + const worktreeDir = join(repoDir, ".worktrees", "pr-42"); + const lifecycle = join(repoDir, "lifecycle.log"); + const ownerFile = join(repoDir, "owner-oid"); + const releaseCwd = join(repoDir, "release-cwd"); + const refLock = join(repoDir, ".git/refs/openclaw/pr-operation-locks/42.lock"); + const git = (...args: string[]) => + execFileSync("git", args, { cwd: repoDir, encoding: "utf8" }).trim(); + git("config", "commit.gpgSign", "false"); + git("config", "core.hooksPath", "/dev/null"); + git("config", "core.filesRefLockTimeout", "0"); + // Gate policy has its own merge tests. Keep the real wrapper, worktree + // entry/removal, completion marker, supervisor, and exact-owner release. + const mergeScript = join(repoDir, "scripts/pr-lib/merge.sh"); + writeFileSync( + mergeScript, + `${readFileSync(mergeScript, "utf8")}\n` + + "validate_review_artifact_data() { :; }\n" + + "merge_verify() { mark_pr_operation_side_effects_started; }\n", + ); + git("add", "scripts"); + git("commit", "-qm", "test: native cleanup fixture"); + const preparedHead = git("rev-parse", "HEAD"); + git("update-ref", "refs/remotes/origin/main", preparedHead); + git("worktree", "add", "-q", "-b", "temp/pr-42", worktreeDir); + if (wrapper === "linked") { + // origin/main still names the linked wrapper; canonical code must not + // be substituted merely to obtain the persistent supervisor cwd. + writeFileSync( + mergeScript, + "merge_run() { echo 'wrong canonical wrapper' >&2; exit 91; }\n", + ); + git("add", "scripts/pr-lib/merge.sh"); + git("commit", "-qm", "test: canonical wrapper drift"); + } + const canonicalHead = git("rev-parse", "HEAD"); + const localDir = join(worktreeDir, ".local"); + mkdirSync(localDir); + for (const artifact of ["review.md", "pr-meta.env", "pr-meta.json", "prep.md"]) { + writeFileSync(join(localDir, artifact), "fixture\n"); + } + writeFileSync(join(localDir, "review.json"), '{"recommendation":"READY FOR /prepare-pr"}\n'); + writeFileSync(join(localDir, "prep.env"), `PREP_HEAD_SHA=${preparedHead}\n`); + // Both cached and plain gh routes use this executable. Unknown requests + // fail locally; neither this fixture nor its origin can contact GitHub. + const gh = writeFixtureFile(binDir, "gh", [ "#!/usr/bin/env bash", "set -euo pipefail", - `source '${join(copiedLibDir, "operation-lock.sh")}'`, - `repo_root() { printf '%s\\n' '${repoDir}'; }`, - "acquire_pr_operation_lock 42", - "echo 'fixture: lock acquired'", - `rm -rf '${doomedDir}'`, - "echo 'fixture: runner worktree deleted'", - ].join("\n"), - ); - chmodSync(fixture, 0o755); - const result = await runSupervisedFixture(repoDir, fixture, { - cwd: doomedDir, - runner: copiedRunner, - }); - expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); - expect(result.stdout).toContain("fixture: lock acquired"); - expect(result.stdout).toContain("fixture: runner worktree deleted"); - expect(result.stderr).not.toContain("Retaining the operation lock"); - expect(refExists(repoDir)).toBe(false); - }); - it("retains the exact owner when supervisor release cannot take the ref lock", async () => { - const repoDir = createRepo(); - // Retry counts are covered above; this integration case only needs the real ref-lock failure. - execFileSync("git", ["config", "core.filesRefLockTimeout", "0"], { cwd: repoDir }); - const binDir = join(repoDir, "fast-release-bin"); - mkdirSync(binDir); - const sleepPath = join(binDir, "sleep"); - writeFileSync(sleepPath, "#!/bin/sh\nexit 0\n"); - chmodSync(sleepPath, 0o755); - const refLock = join(repoDir, ".git/refs/openclaw/pr-operation-locks/42.lock"); - const result = await runSupervisedOperation( - repoDir, - "blocked-release.sh", - ["acquire_pr_operation_lock 42", `: >'${refLock}'`], - { env: { PATH: `${binDir}${delimiter}${process.env.PATH ?? ""}` } }, - ); - const ownerOid = refOid(repoDir); - expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); - expect(result.stderr).toContain("Unable to release the operation lock for 42"); - expect(result.stderr).toContain( - `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, - ); - expect(refOid(repoDir)).toBe(ownerOid); - unlinkSync(refLock); - recoverOperationLock(repoDir, ownerOid); - }); + 'case "$*" in', + ' "auth token") exit 1 ;;', + ' "api graphql "*) printf "fixture-user\\n" ;;', + ' "pr view 42 --json baseRefName")', + ' printf "invocation\\t%s\\n" "$PWD" >> "$OPENCLAW_TEST_LIFECYCLE"', + ` printf '%s\\n' '{"baseRefName":"main"}' ;;`, + ` "pr view 42 --json state,isDraft") printf '%s\\n' '{"state":"OPEN","isDraft":false}' ;;`, + ' "pr merge 42 "*)', + ' git rev-parse refs/openclaw/pr-operation-locks/42 > "$OPENCLAW_TEST_OWNER"', + ' if [ "$OPENCLAW_TEST_FAILURE" = merge ]; then echo "fixture merge failed" >&2; exit 7; fi', + ' printf "merged\\n" >> "$OPENCLAW_TEST_LIFECYCLE" ;;', + ' "pr view 42 --json state --jq .state") printf "MERGED\\n" ;;', + ` "pr view 42 --json mergeCommit "*) printf '%s\\n' '${preparedHead}' ;;`, + ' "repo view "*) printf "fixture/repo\\n" ;;', + ' "api --method POST repos/{owner}/{repo}/issues/42/comments "*)', + ' printf "comment\\n" >> "$OPENCLAW_TEST_LIFECYCLE"', + ' printf "https://example.invalid/comment\\n" ;;', + ` "pr view 42 --json headRefName,"*) printf '%s\\n' '{"headRefName":""}' ;;`, + ' "pr view 42 --json url --jq .url") printf "https://example.invalid/42\\n" ;;', + ' *) echo "unexpected fixture gh call: $*" >&2; exit 99 ;;', + "esac", + ]); + chmodSync(gh, 0o755); + const realGit = realpathSync(join(binDir, "git")); + unlinkSync(join(binDir, "git")); + const gitShim = writeFixtureFile(binDir, "git", [ + "#!/usr/bin/env bash", + "set -euo pipefail", + 'case "$*" in', + ' "fetch "*|"-C "*" fetch "*) exit 0 ;;', + ' "worktree remove "*)', + ' "$OPENCLAW_TEST_REAL_GIT" "$@"', + ' printf "removed\\n" >> "$OPENCLAW_TEST_LIFECYCLE"', + ' if [ "$OPENCLAW_TEST_FAILURE" = release ]; then : > "$OPENCLAW_TEST_REF_LOCK"; fi', + " exit 0 ;;", + ' *"update-ref --no-deref -d refs/openclaw/pr-operation-locks/42 "*)', + ' pwd -P > "$OPENCLAW_TEST_RELEASE_CWD"', + ' "$OPENCLAW_TEST_REAL_GIT" "$@"', + ' printf "released\\n" >> "$OPENCLAW_TEST_LIFECYCLE"', + " exit 0 ;;", + "esac", + 'exec "$OPENCLAW_TEST_REAL_GIT" "$@"', + ]); + chmodSync(gitShim, 0o755); + const result = spawnSync( + wrapper === "linked" ? join(worktreeDir, "scripts/pr") : cli, + command === "gc" ? [command] : [command, "42"], + { + cwd: worktreeDir, + encoding: "utf8", + timeout: 15_000, + env: { + ...process.env, + OPENCLAW_GH_BIN: gh, + OPENCLAW_PR_AUTO_MERGE: "0", + OPENCLAW_PR_MERGE_METHOD: "squash", + OPENCLAW_TEST_FAILURE: failure, + OPENCLAW_TEST_LIFECYCLE: lifecycle, + OPENCLAW_TEST_OWNER: ownerFile, + OPENCLAW_TEST_REAL_GIT: realGit, + OPENCLAW_TEST_REF_LOCK: refLock, + OPENCLAW_TEST_RELEASE_CWD: releaseCwd, + PATH: `${binDir}${delimiter}${process.env.PATH ?? ""}`, + }, + }, + ); + const output = `${result.stdout}\n${result.stderr}`; + const events = readFileSync(lifecycle, "utf8"); + expect(result.error, output).toBeUndefined(); + expect(git("rev-parse", "HEAD")).toBe(canonicalHead); + expect(existsSync(worktreeDir)).toBe(failure === "merge"); + if (failure === "merge") { + expect(result.status, output).toBe(1); + expect(output).toContain("fixture merge failed"); + expect(events).toBe(`invocation\t${worktreeDir}\n`); + } else { + const completedEvents = + command === "gc" ? "removed\n" : `invocation\t${worktreeDir}\nmerged\ncomment\nremoved\n`; + expect(events, output).toBe(completedEvents + (failure === "none" ? "released\n" : "")); + expect(readFileSync(releaseCwd, "utf8").trim(), output).toBe(repoDir); + expect(result.status, output).toBe(failure === "none" ? 0 : 1); + expect(result.stdout).toContain( + command === "gc" ? "removed .worktrees/pr-42" : "merge-run complete for PR #42", + ); + } + if (failure === "none") { + expect(refExists(repoDir), output).toBe(false); + expect(result.stderr).not.toContain("Retaining the operation lock"); + } else { + const ownerOid = readFileSync(ownerFile, "utf8").trim(); + expect(refOid(repoDir)).toBe(ownerOid); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + if (failure === "release") { + expect(result.stderr).toContain("Unable to release the operation lock for 42"); + } + } + }, + ); it("reports exact recovery when lock notification fails", () => { const repoDir = createRepo(); const result = runLockShell(repoDir, [ @@ -1991,9 +2089,6 @@ describePosix("scripts/pr per-PR operation lock", () => { ); expect(script).toContain('recover_pr_operation_lock "$pr" "$owner_oid" "$confirmation"'); expect(script).toContain('source "$script_parent_dir/pr-lib/operation-lock.sh"'); - expect(script).toContain( - 'pr-lib/process-group-runner.mjs" "$script_parent_dir/.." "$script_self" "$@"', - ); expect(script).toContain('prepare_run "$pr"'); expect(runner).toContain('process.platform === "win32"'); expect(runner).toContain("requires a POSIX process group");