Skip to content

Commit ea6aa1d

Browse files
steipetesteipete
authored
fix(pr): prevent same-PR operations from overlapping (#103669)
* fix(pr): serialize operations per pull request * fix(pr): tighten supervisor error handling
1 parent 505f8ae commit ea6aa1d

9 files changed

Lines changed: 2733 additions & 47 deletions

File tree

‎scripts/AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ This directory owns local tooling, script wrappers, and generated-artifact helpe
2020

2121
## PR Prepare Gates
2222

23+
- `scripts/pr` serializes review, prepare, and merge operations per PR across linked worktrees; `scripts/pr gc` skips active or indeterminate locks. A successful command return is the trusted synchronous-completion contract: every PR-state-mutating child must be joined before returning, and such work must never daemonize or explicitly escape both the operation group and lock-notification FD. Failed, interrupted, or controller-lost operations stay locked because detached children cannot be disproved; after verifying no child tools remain, use the reported exact-OID `scripts/pr lock-recover` command. Never bypass or delete these refs manually.
2324
- `scripts/pr prepare-gates` holds the heavy-check lock for its whole local gate block (`scripts/pr-gates-lock.mjs`), so concurrent gate runs across `.worktrees` queue as units instead of dying on child lock timeouts or vitest no-output watchdog kills.
2425
- `OPENCLAW_PR_GATES_REMOTE=testbox` runs the full-suite `pnpm test` gate on a Blacksmith Testbox through `scripts/crabbox-wrapper.mjs` (same delegation as `check:changed`); `pnpm build`/`pnpm check` stay local. The `tbx_` lease id and Actions run URL land in `.local/gates.env` (`REMOTE_GATES_*`) and `.local/prep.md`. Use it for reviewed trusted code when a loaded host makes the local 88-shard run stall-kill; contributor/fork code stays on secretless CI or sanitized AWS unless a maintainer explicitly approves credentialed execution.
2526

‎scripts/pr‎

Lines changed: 70 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,32 @@ if common_git_dir=$(git -C "$script_parent_dir" rev-parse --path-format=absolute
2121
fi
2222
fi
2323

24+
is_locked_pr_command() {
25+
case "$1" in
26+
review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | review-tests | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) return 0 ;;
27+
*) return 1 ;;
28+
esac
29+
}
30+
31+
is_supervised_pr_process() {
32+
[ "${OPENCLAW_PR_DEDICATED_PROCESS_GROUP:-}" = "1" ] &&
33+
[ "${OPENCLAW_PR_LOCK_NOTIFY_FD:-}" = "3" ] &&
34+
[ "${OPENCLAW_PR_LOCK_SUPERVISOR_PID:-}" = "$PPID" ]
35+
}
36+
37+
if [ "${1-}" = "gc" ] || is_locked_pr_command "${1-}"; then
38+
if is_supervised_pr_process; then
39+
# Do not leak the one-shot marker to tools or nested wrapper calls.
40+
unset OPENCLAW_PR_DEDICATED_PROCESS_GROUP
41+
else
42+
unset OPENCLAW_PR_DEDICATED_PROCESS_GROUP
43+
unset OPENCLAW_PR_LOCK_NOTIFY_FD
44+
unset OPENCLAW_PR_LOCK_SUPERVISOR_PID
45+
command -v node >/dev/null 2>&1 || { echo "Missing required command: node" >&2; exit 1; }
46+
exec node "$script_parent_dir/pr-lib/process-group-runner.mjs" "$script_parent_dir/.." "$script_self" "$@"
47+
fi
48+
fi
49+
2450
# shellcheck disable=SC1091
2551
source "$script_parent_dir/lib/plain-gh.sh"
2652

@@ -29,6 +55,7 @@ usage() {
2955
Usage:
3056
scripts/pr ls
3157
scripts/pr gc [--dry-run]
58+
scripts/pr lock-recover <PR> <OWNER_OID> --confirmed-no-running-tools
3259
scripts/pr review-init <PR>
3360
scripts/pr review-checkout-main <PR>
3461
scripts/pr review-checkout-pr <PR>
@@ -75,6 +102,8 @@ gh() {
75102
# shellcheck disable=SC1091
76103
source "$script_parent_dir/pr-lib/worktree.sh"
77104
# shellcheck disable=SC1091
105+
source "$script_parent_dir/pr-lib/operation-lock.sh"
106+
# shellcheck disable=SC1091
78107
source "$script_parent_dir/pr-lib/common.sh"
79108
# shellcheck disable=SC1091
80109
source "$script_parent_dir/pr-lib/changelog.sh"
@@ -95,18 +124,56 @@ main() {
95124
exit 2
96125
fi
97126

98-
require_cmds
99-
100127
local cmd="${1-}"
101128
shift || true
102129

130+
if [ "$cmd" = "lock-recover" ]; then
131+
local pr="${1-}"
132+
local owner_oid="${2-}"
133+
local confirmation="${3-}"
134+
[ -n "$pr" ] && [ -n "$owner_oid" ] && [ "$#" -eq 3 ] || { usage; exit 2; }
135+
recover_pr_operation_lock "$pr" "$owner_oid" "$confirmation"
136+
return
137+
fi
138+
139+
case "$cmd" in
140+
ls) ;;
141+
gc)
142+
[ "$#" -eq 0 ] || { [ "$#" -eq 1 ] && [ "$1" = "--dry-run" ]; } || {
143+
usage
144+
exit 2
145+
}
146+
;;
147+
review-tests)
148+
[ "$#" -ge 2 ] || { usage; exit 2; }
149+
;;
150+
review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run)
151+
[ "$#" -ge 1 ] || { usage; exit 2; }
152+
;;
153+
*)
154+
usage
155+
exit 2
156+
;;
157+
esac
158+
159+
require_cmds
160+
161+
if is_locked_pr_command "$cmd"; then
162+
local locked_pr="${1-}"
163+
acquire_pr_operation_lock "$locked_pr"
164+
trap 'exit 129' HUP
165+
trap 'exit 130' INT
166+
trap 'exit 131' QUIT
167+
trap 'exit 143' TERM
168+
fi
169+
103170
case "$cmd" in
104171
ls)
105172
list_pr_worktrees
106173
;;
107174
gc)
108175
local dry_run=false
109-
if [ "${1-}" = "--dry-run" ]; then
176+
if [ "$#" -eq 1 ]; then
110177
dry_run=true
111178
fi
112179
gc_pr_worktrees "$dry_run"

‎scripts/pr-lib/common.sh‎

Lines changed: 31 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -185,38 +185,30 @@ common_repo_root() {
185185
worktree_path_for_branch() {
186186
local branch="$1"
187187
local ref="refs/heads/$branch"
188-
189-
git worktree list --porcelain | awk -v ref="$ref" '
190-
/^worktree / {
191-
worktree=$2
192-
next
193-
}
194-
/^branch / {
195-
if ($2 == ref) {
196-
print worktree
197-
found=1
198-
}
199-
}
200-
END {
201-
if (!found) {
202-
exit 1
203-
}
204-
}
205-
'
188+
local field worktree=""
189+
while IFS= read -r -d '' field; do
190+
case "$field" in
191+
worktree\ *) worktree="${field#worktree }" ;;
192+
"branch $ref")
193+
[ -n "$worktree" ] || return 1
194+
printf '%s\n' "$worktree"
195+
return 0
196+
;;
197+
"") worktree="" ;;
198+
esac
199+
done < <(git worktree list --porcelain -z)
200+
return 1
206201
}
207202

208203
worktree_is_registered() {
209204
local path="$1"
210-
git worktree list --porcelain | awk -v target="$path" '
211-
/^worktree / {
212-
if ($2 == target) {
213-
found=1
214-
}
215-
}
216-
END {
217-
exit found ? 0 : 1
218-
}
219-
'
205+
local field
206+
while IFS= read -r -d '' field; do
207+
case "$field" in
208+
worktree\ *) [ "${field#worktree }" = "$path" ] && return 0 ;;
209+
esac
210+
done < <(git worktree list --porcelain -z)
211+
return 1
220212
}
221213

222214
resolve_existing_dir_path() {
@@ -263,20 +255,27 @@ remove_worktree_if_present() {
263255
return 0
264256
fi
265257

266-
if worktree_is_registered "$path"; then
267-
git worktree remove "$path" --force >/dev/null 2>&1 || true
258+
if [ -L "$path" ] || ! is_repo_pr_worktree_dir "$path"; then
259+
echo "Warning: refusing to remove non-canonical PR-worktree path $path"
260+
return 0
261+
fi
262+
263+
local registered_path
264+
registered_path="$(resolve_existing_dir_path "$(dirname "$path")")/$(basename "$path")"
265+
if [ -n "$registered_path" ] && worktree_is_registered "$registered_path"; then
266+
git worktree remove "$registered_path" --force >/dev/null 2>&1 || true
268267
fi
269268

270269
if [ ! -e "$path" ]; then
271270
return 0
272271
fi
273272

274-
if worktree_is_registered "$path"; then
273+
if [ -n "$registered_path" ] && worktree_is_registered "$registered_path"; then
275274
echo "Warning: failed to remove registered worktree $path"
276275
return 0
277276
fi
278277

279-
if ! is_repo_pr_worktree_dir "$path"; then
278+
if [ -L "$path" ] || ! is_repo_pr_worktree_dir "$path"; then
280279
echo "Warning: refusing to trash non-PR-worktree path $path"
281280
return 0
282281
fi

0 commit comments

Comments
 (0)