docs(external-contrib): v3 — locked Phase 2 decisions, verbose merge msg (Ship Msg 3)
Cleanup pass on the design doc per Ship's Msg 3 review. Two changes, no architecture shifts:
1. Merge commit message updated to locked verbose form:
"Merge external GitHub PR #{N}: {branch_slug}"
N derived from gh-pr-{N}/ regex; branch_slug = post-prefix portion. Embeds
the GitHub PR number explicitly so `git log --merges --grep "#90"` surfaces
the merge from PR number alone. Drops the v2 "{branch}"-only form.
2. "Open questions for Ship" section collapsed to "Locked decisions" —
the three Phase 2 items (backout flag, rebase-after-fix scope, merge commit
message format) were locked in Ship's reply on Msg 2 but the v2 doc still
framed them as open. Now restated as decisions so Ganymede doesn't re-debate
resolved items at line-level review.
Diffs reviewable against v2 (commit fa6cceb).
This commit is contained in:
parent
fa6cceb9cd
commit
537cfd5ed7
1 changed files with 35 additions and 28 deletions
|
|
@ -3,11 +3,12 @@
|
|||
**Author:** Epimetheus
|
||||
**Architecture review:** Ship (owns merge.py, sync-mirror.sh)
|
||||
**Code review:** Ganymede (line-level, post-design-approval)
|
||||
**Status:** Phase 1 sweep-only scope locked (Ship Msg 2/2). Awaiting code review.
|
||||
**Status:** Phase 1 sweep-only scope locked. Phase 2 architecture decisions locked (Ship Msg 3). Awaiting Phase 1 line-level code review.
|
||||
|
||||
## Revision log
|
||||
|
||||
- **v2 (this revision):** Phase 1 simplified per Ship's Msg 2 — backfill script dropped (sweep IS the backfill), sweep placement specified explicitly as the very first action after initial fetch (ahead of branch-mirror loop AND auto-create-PR block at line ~250). Phase 2 architecture unchanged.
|
||||
- **v3 (this revision):** Cleanup pass per Ship Msg 3 — merge commit message updated to locked verbose form (`"Merge external GitHub PR #{N}: {branch_slug}"`), open-questions section collapsed to "Locked Phase 2 decisions" restating the three resolved outcomes (no longer questions).
|
||||
- **v2:** Phase 1 simplified per Ship's Msg 2 — backfill script dropped (sweep IS the backfill), sweep placement specified explicitly as the very first action after initial fetch (ahead of branch-mirror loop AND auto-create-PR block at line ~250). Phase 2 architecture unchanged.
|
||||
- **v1:** Initial draft.
|
||||
|
||||
## Problem statement
|
||||
|
|
@ -90,9 +91,12 @@ async def _merge_no_ff_external(branch: str) -> tuple[bool, str]:
|
|||
Mechanics:
|
||||
1. Fetch latest origin/main and origin/{branch}
|
||||
2. Create scratch worktree at HEAD of origin/main
|
||||
3. git merge --no-ff origin/{branch} -m "Merge PR: {branch}"
|
||||
4. git push origin HEAD:main
|
||||
5. Cleanup worktree
|
||||
3. Derive: gh_pr_num = re.match(r"gh-pr-(\d+)/", branch).group(1)
|
||||
branch_slug = branch[len(f"gh-pr-{gh_pr_num}/"):]
|
||||
4. git merge --no-ff origin/{branch} \
|
||||
-m f"Merge external GitHub PR #{gh_pr_num}: {branch_slug}"
|
||||
5. git push origin HEAD:main
|
||||
6. Cleanup worktree
|
||||
|
||||
Conflict handling:
|
||||
- Entity conflicts: same auto-resolve pattern as cherry-pick
|
||||
|
|
@ -108,9 +112,12 @@ async def _merge_no_ff_external(branch: str) -> tuple[bool, str]:
|
|||
"""
|
||||
```
|
||||
|
||||
The merge commit message uses the branch name (which contains the GitHub PR
|
||||
number via the `gh-pr-{N}/...` convention) so post-merge processing can
|
||||
correlate back to the GitHub PR if needed.
|
||||
The merge commit message format `"Merge external GitHub PR #{N}: {branch_slug}"`
|
||||
embeds the GitHub PR number explicitly so `git log --merges --grep "#90"`
|
||||
surfaces the merge from PR number alone (no branch-name guessing). Branch slug
|
||||
is the post-`gh-pr-{N}/` portion of the branch (e.g.,
|
||||
`contributor/arcium-confidential-computing-challenge`) — already in scope at
|
||||
merge time, no claim-file read needed.
|
||||
|
||||
### Bug #2 fix: self-healing sweep at top of sync-mirror cycle
|
||||
|
||||
|
|
@ -340,29 +347,29 @@ next merge action.
|
|||
us contributor comments on closed-via-cherry-pick PRs (already partially solves
|
||||
the UX). Phase 2 adds the "merged" badge. Phase 3 is post-Phase-1 cleanup.
|
||||
|
||||
## Open questions for Ship
|
||||
## Locked decisions
|
||||
|
||||
**Resolved in Msg 2:**
|
||||
- ~~`.fork-pr-map` file location~~ → moot, sweep replaces map approach
|
||||
- ~~Phase 1 vs Phase 2 sequencing~~ → separate deploys, sweep-only Phase 1 confirmed
|
||||
- ~~One-shot backfill script~~ → dropped, sweep IS the backfill
|
||||
- ~~Sweep placement~~ → confirmed: first action after initial fetch, before
|
||||
branch-mirror loop AND auto-create-PR block
|
||||
**Phase 1 (resolved in Msg 2):**
|
||||
- `.fork-pr-map` file location → moot, sweep replaces map approach
|
||||
- Phase 1 vs Phase 2 sequencing → separate deploys, sweep-only Phase 1
|
||||
- One-shot backfill script → dropped, sweep IS the backfill
|
||||
- Sweep placement → first action after initial fetch, before branch-mirror loop
|
||||
AND auto-create-PR block
|
||||
|
||||
**Still open (Phase 2 architecture):**
|
||||
**Phase 2 (resolved in Msg 2 / Ship reply):**
|
||||
|
||||
1. **Backout flag necessary?** I'm including `EXTERNAL_PR_NO_FF_MERGE` config
|
||||
flag for fast cutout. Overkill given low-traffic external-PR path? Or
|
||||
prudent given Accelerate Solana pressure?
|
||||
1. **Backout flag:** `EXTERNAL_PR_NO_FF_MERGE` config flag included, default
|
||||
`True` after deploy. One config branch in dispatch is cheap insurance with
|
||||
hackathon timing risk one week out.
|
||||
|
||||
2. **Test plan #5 (rebase-after-fix) scope.** Documenting the silent-ignore as
|
||||
acceptable for now. Sufficient for hackathon, or do you want me to add the
|
||||
structured-alert path before May 5?
|
||||
2. **Rebase-after-fix scope:** silent-ignore as documented in §"Edge case".
|
||||
Eval-reset semantics already handle the recovery path — no structured
|
||||
alert in this scope. Re-evaluate after first 5-10 external contributors.
|
||||
|
||||
3. **`merge --no-ff` commit message format.** I have `"Merge PR: {branch}"`.
|
||||
Better signal to derive the GitHub PR number from the branch and write
|
||||
`"Merge external GitHub PR #{N}: {claim_title_or_branch_slug}"`? Trades
|
||||
verbosity for searchability in `git log --merges`.
|
||||
3. **Merge commit message format:** verbose with PR number —
|
||||
`"Merge external GitHub PR #{N}: {branch_slug}"`. Branch slug derived from
|
||||
the `gh-pr-{N}/` prefix strip; already in scope at merge time, no claim-file
|
||||
read needed. Searchability via `git log --merges --grep "#90"` is the value.
|
||||
|
||||
Sending revised doc directly via Pentagon. Ganymede gets line-level review of
|
||||
the Phase 1 code once Ship signs off on the revised design.
|
||||
Ganymede gets line-level review of the Phase 1 sweep code on
|
||||
`epimetheus/sync-mirror-self-heal` once Ship signs off on this revision.
|
||||
|
|
|
|||
Loading…
Reference in a new issue