Skip to content

fix(pr): stop rebased syncs from losing ancestry#98765

Closed
steipete wants to merge 2 commits into
mainfrom
codex/pr-ancestry-repair
Closed

fix(pr): stop rebased syncs from losing ancestry#98765
steipete wants to merge 2 commits into
mainfrom
codex/pr-ancestry-repair

Conversation

@steipete

@steipete steipete commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where maintainers syncing a rebased PR preparation head could publish the intended tree on the old branch ancestry, leaving the prepared topology and merge-gate evidence inconsistent.

Why This Change Was Made

The default GitHub GraphQL publication path now refuses before mutation unless the prepared head preserves both the remote lease ancestry and mainline merge base. A new explicit prepare-push --repair-ancestry mode may move only a same-repository branch ref when the verified local and remote trees are identical; it requires the recorded sync artifact, an unchanged lease, explicit git/unsigned overrides, a successful force-with-lease dry run, and post-publication SHA/tree/base/content verification. It never falls back to GraphQL.

The repair clears stale preparation evidence and cannot finalize until normal hosted gates pass for the exact published SHA. Other Testbox changed-head publication remains refused and the normal content-changing path proves or reruns local gates before publication.

User Impact

Maintainers can repair same-tree ancestry after a rebase without fabricating gate artifacts or weakening prepare-run or merge-run. Unsafe or ambiguous publication states fail closed before the branch is mutated.

Evidence

  • node scripts/run-vitest.mjs test/scripts/pr-push-topology.test.ts test/scripts/pr-wrappers.test.ts — 22 tests passed.
  • bash -n and ShellCheck on the touched shell wrappers passed.
  • Oxfmt, oxlint, and git diff --check passed.
  • Blacksmith Testbox changed-file gate passed: tbx_01kwfrjn5xkx6q8vbxf6yxvdj3 (Actions run).
  • Fresh gpt-5.6-sol xhigh autoreview reported no actionable findings on the rebased current-main diff.

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: XL maintainer Maintainer-authored PR labels Jul 1, 2026
@clawsweeper

clawsweeper Bot commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 5:34 PM ET / 21:34 UTC.

Summary
The PR hardens scripts/pr PR-head publication by adding GraphQL ancestry checks, a same-repository prepare-push --repair-ancestry mode, .local artifact validation, and topology tests.

PR surface: Tests +501, Other +558. Total +1059 across 6 files.

Reproducibility: no. live high-confidence reproduction was run in this read-only review. The source path is reproducible by inspection: current main can call GraphQL publication without an ancestry guard, and the PR adds git-topology tests for the failure mode.

Review metrics: 1 noteworthy metric.

  • PR publication modes: 1 explicit repair mode added; default GraphQL publication now fail-closed. This is the core maintainer automation behavior change owners should notice before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: scripts/pr, migration/backfill/repair: scripts/pr-lib/prepare-core.sh, migration/backfill/repair: test/scripts/pr-wrappers.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Resolve or explain the current check-test-types and cron shard CI failures.
  • [P1] Get script-owner approval for the stricter fail-closed publication behavior.

Risk before merge

  • [P1] Existing maintainer prepare-sync workflows can now fail closed unless ancestry is preserved or maintainers use the explicit git repair path with the required overrides.
  • [P1] The diff changes branch-ref publication and gate-artifact handling, so a mistake could affect PR landing automation even when unit tests pass.
  • [P1] Exact-head CI currently has failing jobs that must be resolved or proven unrelated before merge.

Maintainer options:

  1. Require owner approval plus green exact-head gates (recommended)
    Have a script workflow owner approve the stricter GraphQL/git publication semantics and wait for the current head's failing CI jobs to be resolved or proven unrelated before merge.
  2. Accept the stricter maintainer workflow
    Maintainers may intentionally accept that some rebased syncs now fail closed and require explicit git overrides or the repair mode instead of automatic GraphQL publication.
  3. Pause if fallback compatibility is still needed
    If fork or Testbox sync workflows still depend on the old GraphQL fallback shape, pause this PR and split the ancestry guard from the repair-mode rollout.

Next step before merge

  • [P1] Protected maintainer label, branch-publication semantics, and failing exact-head CI require maintainer/script-owner handling rather than cleanup close or automated repair.

Security
Cleared: The diff is security-sensitive because it touches shell artifacts and branch-ref mutation, but the reviewed changes add artifact refusals, ancestry checks, force-with-lease checks, and no concrete supply-chain concern was found.

Review details

Best possible solution:

Land the guarded publication and repair flow only after script-owner approval and green exact-head gates confirm the stricter fail-closed behavior is acceptable for maintainer PR landing workflows.

Do we have a high-confidence way to reproduce the issue?

No live high-confidence reproduction was run in this read-only review. The source path is reproducible by inspection: current main can call GraphQL publication without an ancestry guard, and the PR adds git-topology tests for the failure mode.

Is this the best way to solve the issue?

Yes, the guard is placed at the publication boundary and the same-tree repair path is narrowly constrained. The remaining concern is upgrade safety for maintainer sync flows, not a better code location for the fix.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against eb417fa206e6.

Label changes

Label justifications:

  • P2: This is a normal-priority maintainer automation bug fix with meaningful but bounded blast radius in PR landing scripts.
  • merge-risk: 🚨 compatibility: Existing prepare-sync publication flows can now fail closed unless ancestry is preserved or explicit git repair mode is used.
  • merge-risk: 🚨 automation: The PR changes branch-ref publication, gate artifact handling, and merge-prep scripts that are part of repository landing automation.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor proof gate is not applied because this is a same-repository PR with the protected maintainer label; the body still includes local test, shell/lint, Testbox, and autoreview evidence.
Evidence reviewed

PR surface:

Tests +501, Other +558. Total +1059 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 2 501 0 +501
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 4 587 29 +558
Total 6 1088 29 +1059

What I checked:

  • Repository policy read: Root AGENTS.md and scoped scripts/AGENTS.md were read fully; the applicable guidance requires deep script review, protected-label handling, and compatibility/automation risk treatment for maintainer workflows. (AGENTS.md:13, eb417fa206e6)
  • Protected live PR state: Live PR metadata shows a same-repository head at d78ef37289dbc786256aafd571c328176aee77df, mergeable=true, mergeable_state=unstable, and the protected maintainer label, so this workflow should keep the PR open for maintainer handling. (d78ef37289db)
  • Current-main behavior: Current main can route PR branch publication through graphql_push_to_fork when head-owner metadata is present and git mode is not forced, without checking that the prepared head preserves the remote lease ancestry before calling GraphQL. (scripts/pr-lib/push.sh:221, eb417fa206e6)
  • Current-main sync context: Current prepare_sync_head records that GraphQL publication can create a remote commit with the same tree but the old branch parent, which matches the PR's central ancestry-loss concern. (scripts/pr-lib/prepare-core.sh:288, eb417fa206e6)
  • PR ancestry guard: The PR-head push.sh adds require_graphql_push_preserves_ancestry and calls it before createCommitOnBranch, refusing non-descendant or mainline-base-changing GraphQL publication before mutation. (scripts/pr-lib/push.sh:28, d78ef37289db)
  • PR repair boundary: The PR-head prepare-core.sh limits ancestry repair to same-repository branches with a matching sync artifact, explicit git/unsigned overrides, unchanged lease/tree/base checks, force-with-lease dry run, and post-publication verification. (scripts/pr-lib/prepare-core.sh:359, d78ef37289db)

Likely related people:

  • jalehman: Authored the merged fork PR push work that introduced the GitHub GraphQL fallback behavior now being guarded. (role: introduced adjacent behavior; confidence: high; commits: 99535cf15240, 81be42339f62, 3a8133d5874b; files: scripts/pr, scripts/pr-lib/push.sh)
  • shakkernerd: Merged the GraphQL fallback PR and authored the nearby hardening commit that added prepare-sync-head. (role: adjacent workflow owner; confidence: high; commits: dc3108d3f8cb, 3a8133d5874b; files: scripts/pr, scripts/pr-lib/prepare-core.sh, scripts/pr-lib/push.sh)
  • steipete: Authored the earlier push_prep_head_to_pr_branch dedupe refactor and this same-repository PR, so the person is relevant beyond only opening the branch. (role: recent area contributor; confidence: high; commits: fa00b1d0ca26, be5bf0a9c2f7, d78ef37289db; files: scripts/pr, scripts/pr-lib/push.sh, scripts/pr-lib/prepare-core.sh)
  • gumadeiras: Split the monolithic PR workflow script into the current scripts/pr-lib/* modules where this PR makes its changes. (role: module refactor author; confidence: medium; commits: 31a4b45db09f; files: scripts/pr, scripts/pr-lib/prepare-core.sh, scripts/pr-lib/push.sh)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels Jul 1, 2026
@steipete steipete self-assigned this Jul 2, 2026
This was referenced Jul 9, 2026
Closed
Merged
Merged
@steipete

steipete commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #108018, landed as 2e73cff.

This PR repaired the old eager-rebase design by adding an explicit ancestry-repair mode. The landed fix removes that root condition: prepare sync preserves the hosted PR ancestry, rejects rewritten local topology before mutation, and delegates mainline drift to merge verification. That is one smaller canonical path and does not require force-moving a contributor branch.

The landed regression suite covers the ancestry refusal and exact merge-parent CI diff, and exact-head CI passed. Thank you for carrying the original failure analysis; closing this larger repair branch as obsolete.

@steipete steipete closed this Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. scripts Repository scripts size: XL status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete