fix(merge): synthetic _merged/* ref + function-owned ff-push (Ship Msg 3)
Phase 2 review fix #1 (architectural pushback): replace force-push of contributor's gh-pr-N/* branch with a three-step synthetic-branch flow: 1. Worktree on local branch _merged-{slug} from origin/main 2. git merge --no-ff origin/{branch} into the local branch 3. Push merge commit to origin/_merged/{branch} (synthetic audit ref) 4. Function ff-pushes merge_sha → origin/main directly Contributor's gh-pr-N/* branch on Forgejo is now never touched. Force-pushing it would have rewritten the tip with a merge commit the contributor didn't author — confusing bot force-push in Forgejo PR UI. Mirrors the _clean/* synthetic branch pattern in cherry-pick. Function now owns the push to main (was dispatch's job for cherry-pick and reweave). Returns sentinel "merged --no-ff (external PR #N, M=<sha>, audit_ref=...)" that dispatch detects to skip its ff-push and route directly to PR-close + mark_merged + audit. Audit detail JSON now includes merge_commit_sha + audit_ref + github_pr (Ship review #5). Smoke-tested in scratch repo end-to-end: - contributor branch tip unchanged ✓ - audit ref _merged/gh-pr-90/... carries merge SHA ✓ - main tip equals merge SHA (ff-push, no force) ✓ - contributor SHA ancestor of main → GitHub badge fires ✓ Sentinel return parsed via 3 regexes in dispatch (full 40-char SHA in return string for durability). Branch comment in dispatch explicitly notes contributor branch is left in place — sync-mirror keeps the GitHub PR <-> Forgejo PR link observable through it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
992b4ee36f
commit
7741c1e6de
1 changed files with 100 additions and 21 deletions
121
lib/merge.py
121
lib/merge.py
|
|
@ -444,15 +444,30 @@ async def _merge_no_ff_external(branch: str) -> tuple[bool, str]:
|
|||
|
||||
Mechanics:
|
||||
1. Fetch origin/main + origin/{branch}
|
||||
2. Detached worktree at origin/main, git merge --no-ff origin/{branch}
|
||||
with verbose message: "Merge external GitHub PR #{N}: {branch_slug}"
|
||||
3. Force-push the merge commit as origin/{branch} (replacing the branch tip).
|
||||
Dispatch's existing ff-push to main then produces the merge commit on main.
|
||||
2. Worktree on local branch _merged-{slug} from origin/main
|
||||
3. git merge --no-ff origin/{branch} with verbose message:
|
||||
"Merge external GitHub PR #{N}: {branch_slug}"
|
||||
4. Push merge commit to origin/_merged/{branch} (synthetic audit ref)
|
||||
5. ff-push merge_sha → origin/main directly (function owns the push, NOT
|
||||
dispatch — see sentinel return below)
|
||||
|
||||
The merge commit M has parents [main_sha, branch_sha]. M is a fast-forward
|
||||
descendant of main_sha (via first-parent chain), so the ff-push to main
|
||||
descendant of main_sha (via first-parent chain), so the push to main
|
||||
works without --force.
|
||||
|
||||
Synthetic branch (Ship review Apr 28): we deliberately do NOT force-push
|
||||
the contributor's gh-pr-N/* branch. Force-pushing it would rewrite the
|
||||
branch tip with a merge commit the contributor didn't author, showing as
|
||||
a confusing bot force-push in Forgejo's PR UI. The synthetic _merged/*
|
||||
audit ref lets us track the merge commit without touching the contributor's
|
||||
branch. Mirrors the _clean/* synthetic branch pattern in cherry-pick.
|
||||
|
||||
Sentinel return: function pushes merge_sha → main itself (dispatch's ff-push
|
||||
can't, since origin/{branch} is unchanged and not a descendant of main).
|
||||
Returns a "merged --no-ff" sentinel string that dispatch detects to skip
|
||||
its ff-push step and route directly to PR-close + mark_merged + audit.
|
||||
The full 40-char merge SHA is in the return string for dispatch to extract.
|
||||
|
||||
Conflict handling: same auto-resolve pattern as cherry-pick — entity-only
|
||||
conflicts take main's version (--ours = current worktree HEAD = main),
|
||||
other conflicts abort and return False with detail.
|
||||
|
|
@ -465,7 +480,10 @@ async def _merge_no_ff_external(branch: str) -> tuple[bool, str]:
|
|||
gh_pr_num = m.group(1)
|
||||
branch_slug = m.group(2)
|
||||
|
||||
worktree_path = f"/tmp/teleo-merge-{branch.replace('/', '-')}"
|
||||
slug = branch.replace("/", "-")
|
||||
worktree_path = f"/tmp/teleo-merge-{slug}"
|
||||
local_branch = f"_merged-{slug}" # local working branch in worktree
|
||||
audit_ref = f"_merged/{branch}" # remote synthetic ref (preserves hierarchy)
|
||||
|
||||
# Fetch latest state — separate calls (long branch names break combined refspec)
|
||||
rc, out = await _git("fetch", "origin", "main", timeout=15)
|
||||
|
|
@ -487,7 +505,9 @@ async def _merge_no_ff_external(branch: str) -> tuple[bool, str]:
|
|||
logger.info("External PR branch %s is descendant of main but has new content — proceeding", branch)
|
||||
|
||||
async with _bare_repo_lock:
|
||||
rc, out = await _git("worktree", "add", "--detach", worktree_path, "origin/main")
|
||||
# Clean up any stale local branch from a prior failed run
|
||||
await _git("branch", "-D", local_branch)
|
||||
rc, out = await _git("worktree", "add", "-b", local_branch, worktree_path, "origin/main")
|
||||
if rc != 0:
|
||||
return False, f"worktree add failed: {out}"
|
||||
|
||||
|
|
@ -533,30 +553,45 @@ async def _merge_no_ff_external(branch: str) -> tuple[bool, str]:
|
|||
await _git("merge", "--abort", cwd=worktree_path)
|
||||
return False, f"merge conflict on PR #{gh_pr_num}: {conflict_detail}"
|
||||
|
||||
# Force-push the merge commit as origin/{branch}, replacing the contributor's
|
||||
# branch tip. Dispatch then ff-pushes origin/{branch} → main, producing the
|
||||
# merge commit on main. --force-with-lease guards against concurrent updates.
|
||||
rc, expected_sha = await _git("rev-parse", f"origin/{branch}")
|
||||
# Capture the merge commit SHA before any pushes
|
||||
rc, merge_sha = await _git("rev-parse", "HEAD", cwd=worktree_path)
|
||||
if rc != 0:
|
||||
return False, f"rev-parse origin/{branch} failed: {expected_sha}"
|
||||
expected_sha = expected_sha.strip().split("\n")[0]
|
||||
return False, f"rev-parse merge HEAD failed: {merge_sha}"
|
||||
merge_sha = merge_sha.strip().split("\n")[0]
|
||||
|
||||
# Push to synthetic audit ref _merged/{branch} (does not touch contributor's
|
||||
# gh-pr-N/* branch). Force-with-lease against expected nothing — the audit
|
||||
# ref is per-PR and shouldn't pre-exist on a fresh merge. If it does (rare:
|
||||
# a prior failed attempt), force-push is fine since the ref is bot-owned.
|
||||
rc, out = await _git(
|
||||
"push",
|
||||
f"--force-with-lease={branch}:{expected_sha}",
|
||||
"origin",
|
||||
f"HEAD:{branch}",
|
||||
cwd=worktree_path,
|
||||
timeout=30,
|
||||
"push", "--force", "origin", f"HEAD:refs/heads/{audit_ref}",
|
||||
cwd=worktree_path, timeout=30,
|
||||
)
|
||||
if rc != 0:
|
||||
return False, f"push rejected: {out}"
|
||||
return False, f"push to audit ref {audit_ref} failed: {out}"
|
||||
|
||||
return True, f"merged --no-ff (external PR #{gh_pr_num})"
|
||||
# ff-push the merge commit to main. This is a true fast-forward (M is a
|
||||
# descendant of origin/main via its first parent), so no --force needed.
|
||||
# Forgejo's branch protection allows ff-push to main from authorized users.
|
||||
rc, out = await _git(
|
||||
"push", "origin", f"{merge_sha}:main",
|
||||
cwd=worktree_path, timeout=30,
|
||||
)
|
||||
if rc != 0:
|
||||
# Roll back audit ref if main push failed — keeps state consistent.
|
||||
await _git("push", "--delete", "origin", f"refs/heads/{audit_ref}",
|
||||
cwd=worktree_path, timeout=15)
|
||||
return False, f"ff-push to main failed: {out}"
|
||||
|
||||
# Sentinel return: "merged --no-ff" prefix triggers dispatch's external-PR
|
||||
# close path (skips ff-push, does PR-close + mark_merged + audit).
|
||||
# Full 40-char merge SHA in the message so dispatch can parse it for audit.
|
||||
return True, f"merged --no-ff (external PR #{gh_pr_num}, M={merge_sha}, audit_ref={audit_ref})"
|
||||
|
||||
finally:
|
||||
async with _bare_repo_lock:
|
||||
await _git("worktree", "remove", "--force", worktree_path)
|
||||
await _git("branch", "-D", local_branch)
|
||||
|
||||
|
||||
from .frontmatter import (
|
||||
|
|
@ -922,6 +957,50 @@ async def _merge_domain_queue(conn, domain: str) -> tuple[int, int]:
|
|||
succeeded += 1
|
||||
continue
|
||||
|
||||
# External GitHub PR (gh-pr-*): _merge_no_ff_external already pushed
|
||||
# the merge commit to origin/main + the synthetic _merged/{branch}
|
||||
# audit ref. Skip dispatch's ff-push (would fail — origin/{branch} is
|
||||
# the contributor's untouched branch, not a descendant of main).
|
||||
# Just close PR + mark_merged + audit, parsing merge SHA from sentinel.
|
||||
if pick_msg.startswith("merged --no-ff"):
|
||||
m = re.search(r"M=([a-f0-9]{40})", pick_msg)
|
||||
merge_sha = m.group(1) if m else None
|
||||
m_ref = re.search(r"audit_ref=(\S+?)\)", pick_msg)
|
||||
audit_ref = m_ref.group(1) if m_ref else None
|
||||
m_pr = re.search(r"external PR #(\d+)", pick_msg)
|
||||
gh_pr_num = m_pr.group(1) if m_pr else None
|
||||
|
||||
leo_token = get_agent_token("leo")
|
||||
comment_body = (
|
||||
f"Merged via --no-ff into main.\n"
|
||||
f"Merge commit: `{merge_sha}`\n"
|
||||
f"Audit ref: `{audit_ref}`\n"
|
||||
f"Branch: `{branch}` (preserved unchanged)"
|
||||
)
|
||||
await forgejo_api("POST", repo_path(f"issues/{pr_num}/comments"),
|
||||
{"body": comment_body})
|
||||
result = await forgejo_api("PATCH", repo_path(f"pulls/{pr_num}"),
|
||||
{"state": "closed"}, token=leo_token)
|
||||
if result is None:
|
||||
logger.error("PR #%d: Forgejo close failed (no-ff path), skipping DB update", pr_num)
|
||||
failed += 1
|
||||
continue
|
||||
mark_merged(conn, pr_num)
|
||||
db.audit(conn, "merge", "merged", json.dumps({
|
||||
"pr": pr_num, "branch": branch, "method": "no-ff",
|
||||
"merge_commit_sha": merge_sha,
|
||||
"audit_ref": audit_ref,
|
||||
"github_pr": gh_pr_num,
|
||||
}))
|
||||
# NOTE: do NOT _delete_remote_branch(branch) here. The contributor's
|
||||
# gh-pr-N/* branch is the mirror of their fork PR head — leaving it
|
||||
# in place lets sync-mirror keep the GitHub PR <-> Forgejo PR link
|
||||
# observable. The synthetic _merged/{branch} ref carries the merge.
|
||||
logger.info("PR #%d merged via --no-ff (M=%s)", pr_num,
|
||||
merge_sha[:8] if merge_sha else "?")
|
||||
succeeded += 1
|
||||
continue
|
||||
|
||||
# Local ff-push: cherry-picked branch is a descendant of origin/main.
|
||||
# Regular push = fast-forward. Non-ff rejected by default (same safety).
|
||||
# --force-with-lease removed: Forgejo categorically blocks it on protected branches.
|
||||
|
|
|
|||
Loading…
Reference in a new issue