From cae9ecaba40a3acbc4c07dc8ce5a3f1135cd7e37 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 16 Aug 2026 10:48:26 -0700 Subject: [PATCH] fix(pr): substitute anchor-matching canonical wrapper for stale worktrees (#124710) A linked worktree whose base predates (or carries) wrapper changes relative to origin/main previously hit a hard refusal, even though the canonical checkout held exactly the trusted origin/main wrapper the refusal message told the operator to go run by hand. When the canonical checkout is clean and byte-identical to fetched refs/remotes/origin/main, exec it with a loud stderr notice instead; advisory dev-wrapper opt-in keeps precedence, and the refusal remains when no anchor-matching wrapper exists on disk. Also records the squash-merged stacked-branch rebase gotcha (git rebase --onto origin/main ) in the PR maintainer skill. --- .../skills/openclaw-pr-maintainer/SKILL.md | 1 + scripts/AGENTS.md | 2 +- scripts/pr | 10 +++ test/scripts/pr-wrappers.test.ts | 75 +++++++++++++++---- 4 files changed, 74 insertions(+), 14 deletions(-) diff --git a/.agents/skills/openclaw-pr-maintainer/SKILL.md b/.agents/skills/openclaw-pr-maintainer/SKILL.md index a44a583cbdeb..1f5bf8d4188e 100644 --- a/.agents/skills/openclaw-pr-maintainer/SKILL.md +++ b/.agents/skills/openclaw-pr-maintainer/SKILL.md @@ -359,6 +359,7 @@ gh search issues --repo openclaw/openclaw --match title,body --limit 50 \ - When an agent is landing or merging a PR targeting `main`, use only the repo-native `scripts/pr` wrapper: run `scripts/pr review-init `, follow its emitted checkout/guard guidance, initialize and complete review artifacts with `scripts/pr review-artifacts-init `, validate them with `scripts/pr review-validate-artifacts `, then run `OPENCLAW_TESTBOX=1 scripts/pr prepare-run ` and `scripts/pr merge-run `. The Testbox flag is mandatory for agents: it verifies hosted CI/Testbox on the current head or reuses a patch-identical pre-rebase run green within 24 hours instead of running full `pnpm` gates locally. `prepare-run` fails fast; invoke only after exact-head CI is complete and green, and do not idle on `auto-response` or `check-docs`. For owner-approved reviewed fork code without hosted Testbox, use `OPENCLAW_PR_GATES_REMOTE=testbox` instead. Do not rebase only because `main` advanced; behind-main drift is advisory unless strict drift is explicitly enabled, while GitHub still blocks conflicts. - `scripts/pr` gotchas: subcommands require a PR number (no subcommand `--help` placeholder). Artifacts preserve template enum values with evidence detail in summaries; validate before prepare, from PR-head mode (moving main invalidates the main-baseline guard). Review flow: checkout main baseline, then PR, before artifact validation. After every PR push, rerun `scripts/pr review-init`; checkout alone leaves a stale guard SHA. Locally unset `GITHUB_TOKEN`, `GH_TOKEN`, `HOMEBREW_GITHUB_API_TOKEN`; ambient tokens can select an exhausted or wrong identity. Review JSON: land-ready recommendation `READY FOR /prepare-pr`, `issueValidation.status=valid`; never `APPROVE`. After `scripts/pr merge-run` removes its worktree, `cd` to a persistent repo before follow-up commands. - After GitHub throttling, check core quota before `scripts/pr prepare-run` or `merge-run`. A failed operation can retain its lock; verify no child remains, then recover only with its emitted token. +- Stacked branches over a squash-merged parent: rebase with `git rebase --onto origin/main `; a plain `git rebase origin/main` replays the parent's already-squashed commits and manufactures conflicts. - Non-main PRs: do not run `scripts/pr prepare-run` or `merge-run`; they diff against `main`. Use review artifacts, exact base-head CI, revalidate `headRefOid`, then `gh pr merge --match-head-commit `. - PR-create merge-ref race recognition: the dropped/killed pull_request CI run appears as `startup_failure`/`BuildFailed` (`(Unknown event)`) and is not rerunnable — close/reopen or wait for the hourly `pr-ci-sweeper`; rerun attempts are wasted. - PR/issue media upload: 422 = unsupported type; 404 = bad repo id/no push. Video: `content_type` `video/mp4` or `video/webm`; embed the returned URL on its own bare line — GitHub renders a player, `![]()` does not; transcode Playwright webm via `ffmpeg -i in.webm -c:v libx264 -pix_fmt yuv420p out.mp4` for broad playback. Non-media artifacts or endpoint failure: Crabbox artifact publishing plus the manifest URL. diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 81beb6418648..1831ba1c24c7 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -20,7 +20,7 @@ This directory owns local tooling, script wrappers, and generated-artifact helpe ## PR Prepare Gates -- `scripts/pr` serializes review, prepare, and merge operations per PR across linked worktrees; `scripts/pr gc` skips active or indeterminate locks. Its subcommand classification table is the canonical wrapper trust boundary: a mismatched local wrapper may run only a classified `advisory` subcommand with `--dev-wrapper` or `OPENCLAW_PR_DEV_WRAPPER=1`; classified `landing` subcommands always require canonical/origin-main wrapper code. 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. Release on clean exit requires the leader's completion marker; an escaped descendant that merely holds the notify pipe then produces a loud warned release instead of retention (#124583), while all failure shapes still retain. A failed command auto-releases only while its explicit pre-side-effect validation marker remains active; failures after mutation/tool launch, interruptions, and controller loss 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. +- `scripts/pr` serializes review, prepare, and merge operations per PR across linked worktrees; `scripts/pr gc` skips active or indeterminate locks. Its subcommand classification table is the canonical wrapper trust boundary: a mismatched local wrapper may run only a classified `advisory` subcommand with `--dev-wrapper` or `OPENCLAW_PR_DEV_WRAPPER=1`; classified `landing` subcommands always require canonical/origin-main wrapper code. A worktree whose wrapper differs from origin/main (stale base or wrapper-editing branch) loudly substitutes the canonical checkout's wrapper when that checkout is clean and byte-identical to fetched `refs/remotes/origin/main`; it refuses only when no anchor-matching wrapper is available. 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. Release on clean exit requires the leader's completion marker; an escaped descendant that merely holds the notify pipe then produces a loud warned release instead of retention (#124583), while all failure shapes still retain. A failed command auto-releases only while its explicit pre-side-effect validation marker remains active; failures after mutation/tool launch, interruptions, and controller loss 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. - `scripts/pr prepare-gates` holds the heavy-check lock for its whole local gate block (`scripts/pr-gates-lock.mts`), so concurrent gate runs across `.worktrees` queue as units instead of dying on child lock timeouts or vitest no-output watchdog kills. - `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. diff --git a/scripts/pr b/scripts/pr index 9a3a087f86a0..b5fb2e205fd7 100755 --- a/scripts/pr +++ b/scripts/pr @@ -102,6 +102,16 @@ if common_git_dir=$(git -C "$script_parent_dir" rev-parse --path-format=absolute if [ "$dev_wrapper_opt_in" = "1" ] && [ -n "$requested_classification" ]; then echo "subcommand '$requested_subcommand' is classified $requested_classification; dev-wrapper opt-in is unavailable." >&2 fi + # Worktrees routinely sit on a base that predates (or carries) wrapper + # changes relative to main. When the canonical checkout is byte-identical + # to the fetched origin/main anchor, exec-ing it runs exactly the trusted + # anchor code; announce the substitution so it is never silent. + if [ -n "$anchor_wrapper_revision" ] && + [ "$canonical_wrapper_revision" = "$anchor_wrapper_revision" ] && + [ "$canonical_wrapper_clean" = "1" ]; then + echo "scripts/pr wrapper in this worktree differs from origin/main; running the canonical checkout's wrapper (matches the origin/main trust anchor): $canonical_repo_root" >&2 + exec "$canonical_self" "$@" + fi # HEAD blobs are authoritative here: the uncommitted-wrapper guard above # already exited for any staged or unstaged edit to these paths, so # the working tree matches HEAD and this list matches what was rejected. diff --git a/test/scripts/pr-wrappers.test.ts b/test/scripts/pr-wrappers.test.ts index 75cd225330f2..70c6ec7829a7 100644 --- a/test/scripts/pr-wrappers.test.ts +++ b/test/scripts/pr-wrappers.test.ts @@ -20,14 +20,8 @@ function readScript(path: string): string { return readFileSync(path, "utf8"); } -const canonicalMismatchMessage = (repo: string) => - [ - "scripts/pr implementation differs between this worktree and the canonical checkout, and does not match origin/main.", - "differing wrapper components vs origin/main: scripts/pr-lib", - `Refusing to silently substitute canonical wrapper code from: ${repo}`, - "Run scripts/pr from a checkout whose wrapper matches the canonical checkout or a fetched origin/main.", - "", - ].join("\n"); +const anchorSubstitutionNotice = (repo: string) => + `scripts/pr wrapper in this worktree differs from origin/main; running the canonical checkout's wrapper (matches the origin/main trust anchor): ${repo}`; const itPosix = process.platform === "win32" ? it.skip : it; function makeMismatchedWrapperRepo() { @@ -45,6 +39,14 @@ function makeMismatchedWrapperRepo() { writeFileSync(commandPath, "#!/bin/sh\nexit 0\n"); chmodSync(commandPath, 0o755); } + // Deterministic gh stub: main-only subcommands fail fast on the base-branch + // gate instead of reaching the network, proving which wrapper actually ran. + const ghStub = join(bin, "gh"); + writeFileSync( + ghStub, + '#!/bin/sh\nif [ "$1" = "pr" ] && [ "$2" = "view" ]; then\n echo not-main\n exit 0\nfi\nexit 0\n', + ); + chmodSync(ghStub, 0o755); const fixtureEnv = { ...process.env, @@ -81,6 +83,12 @@ function makeMismatchedWrapperRepo() { join(canonical, "scripts", "lib", "plain-gh.sh"), "resolve_plain_gh_bin() { printf '/usr/bin/true\\n'; }\ngh_plain() { :; }\n", ); + // Marker stub committed to main (the origin/main anchor), so tests can tell + // an anchor-substituted canonical run apart from a local wrapper run. + writeFileSync( + join(canonical, "scripts", "pr-lib", "gates.sh"), + 'ci_dispatch() { echo "canonical wrapper executed"; }\n', + ); chmodSync(join(canonical, "scripts", "pr"), 0o755); git(canonical, ["config", "user.name", "OpenClaw Test"]); @@ -114,8 +122,10 @@ function makeMismatchedWrapperRepo() { canonical, cleanup: () => rmSync(root, { recursive: true, force: true }), env: fixtureEnv, + git, linked, localRevision, + root, }; } @@ -290,7 +300,7 @@ describe("scripts/pr wrappers", () => { } }); - it("keeps the existing mismatch refusal for advisory commands without opt-in", () => { + it("substitutes the anchor-matching canonical wrapper for a mismatched worktree without opt-in", () => { const fixture = makeMismatchedWrapperRepo(); try { const result = spawnSync(join(fixture.linked, "scripts", "pr"), ["ci-dispatch", "123"], { @@ -298,14 +308,17 @@ describe("scripts/pr wrappers", () => { encoding: "utf8", env: fixture.env, }); - expect(result.status).toBe(1); - expect(result.stderr).toBe(canonicalMismatchMessage(fixture.canonical)); + expect(result.status, `${result.stderr}\n${result.stdout}`).toBe(0); + expect(result.stdout).toContain("canonical wrapper executed"); + expect(result.stdout).not.toContain("local wrapper executed"); + expect(result.stderr).toContain(anchorSubstitutionNotice(fixture.canonical)); + expect(result.stderr).not.toContain("Refusing to silently substitute"); } finally { fixture.cleanup(); } }); - it("refuses developer opt-in for a mismatched landing command", () => { + it("routes a mismatched landing command to the anchor-matching canonical wrapper despite opt-in", () => { const fixture = makeMismatchedWrapperRepo(); try { const result = spawnSync( @@ -317,13 +330,49 @@ describe("scripts/pr wrappers", () => { expect(result.stderr).toContain( "subcommand 'prepare-run' is classified landing; dev-wrapper opt-in is unavailable.", ); - expect(result.stderr).toContain(canonicalMismatchMessage(fixture.canonical).trim()); + expect(result.stderr).toContain(anchorSubstitutionNotice(fixture.canonical)); + // The stubbed gh reports a non-main base: reaching this gate proves the + // canonical wrapper ran instead of the mismatched local one. + expect(result.stderr).toContain( + "scripts/pr prepare and merge commands only support PRs targeting main; PR #123 targets not-main.", + ); expect(result.stdout).not.toContain("local wrapper executed"); } finally { fixture.cleanup(); } }); + it("substitutes the canonical wrapper for a stale-base worktree once main moves the wrapper", () => { + const fixture = makeMismatchedWrapperRepo(); + try { + // Stale worktree: created at the pushed main base, no local wrapper edits. + const stale = join(fixture.root, "stale"); + const baseline = fixture.git(fixture.canonical, ["rev-parse", "main"]).stdout.trim(); + fixture.git(fixture.canonical, ["worktree", "add", "-b", "stale-feature", stale, baseline]); + + // main's wrapper then advances and the canonical checkout tracks it. + writeFileSync( + join(fixture.canonical, "scripts", "pr-lib", "gates.sh"), + 'ci_dispatch() { echo "canonical v2 executed"; }\n', + ); + fixture.git(fixture.canonical, ["add", "scripts/pr-lib/gates.sh"]); + fixture.git(fixture.canonical, ["commit", "-m", "test: wrapper v2"]); + fixture.git(fixture.canonical, ["push", "origin", "main"]); + + const result = spawnSync(join(stale, "scripts", "pr"), ["ci-dispatch", "123"], { + cwd: stale, + encoding: "utf8", + env: fixture.env, + }); + expect(result.status, `${result.stderr}\n${result.stdout}`).toBe(0); + expect(result.stdout).toContain("canonical v2 executed"); + expect(result.stderr).toContain(anchorSubstitutionNotice(fixture.canonical)); + expect(result.stderr).not.toContain("Refusing to silently substitute"); + } finally { + fixture.cleanup(); + } + }); + it("keeps merge wrapper modes delegated to the main PR helper", () => { const script = readScript("scripts/pr-merge");