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 <landed-branch>) in the PR maintainer skill.
This commit is contained in:
Peter Steinberger
2026-08-16 10:48:26 -07:00
committed by GitHub
parent 0e7deeec45
commit cae9ecaba4
4 changed files with 74 additions and 14 deletions
@@ -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 <PR>`, follow its emitted checkout/guard guidance, initialize and complete review artifacts with `scripts/pr review-artifacts-init <PR>`, validate them with `scripts/pr review-validate-artifacts <PR>`, then run `OPENCLAW_TESTBOX=1 scripts/pr prepare-run <PR>` and `scripts/pr merge-run <PR>`. 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 <landed-branch>`; 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 <verified-sha>`.
- 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.
+1 -1
View File
@@ -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.
+10
View File
@@ -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.
+62 -13
View File
@@ -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");