feat: add sync-next-draft reusable workflow#98
Conversation
Automatically merge pushes on a draft branch (e.g. accepted review suggestions) into the following draft branches, walking the whole chain in one run. On merge conflict, open a draft-to-draft sync PR that students can resolve in the browser and notify the review PR. Also allow draft-to-draft sync PRs in prevent-draft-merge. Refs: smkwlab/sotsuron-template#110
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # sync-next-draft.yml to carry accepted review suggestions | ||
| # into the next draft branch) | ||
| echo "::debug::Base branch: $BASE_REF" | ||
| if [[ "$BASE_REF" =~ ^[0-9]+(st|nd|rd|th)-draft$ ]] || \ |
There was a problem hiding this comment.
🚨 [HIGH] draft-to-draft 同期 PR の許可判定（BASE_REF が draft パターンかどうか）が、HEAD_REF の検証より先に実行されています。これにより、HEAD_REF が draft パターンであれば BASE_REF も draft パターンである任意の PR がブロックをバイパスできます。悪意のある操作（例: 任意のブランチを head として draft ブランチへの PR を作成）を防ぐため、HEAD_REF も draft パターンであることを条件に追加することを検討してください:
if [[ "$HEAD_REF" =~ ^[0-9]+(st|nd|rd|th)-draft$ || "$HEAD_REF" =~ ^abstract-[0-9]+(st|nd|rd|th)$ ]] && \
[[ "$BASE_REF" =~ ^[0-9]+(st|nd|rd|th)-draft$ || "$BASE_REF" =~ ^abstract-[0-9]+(st|nd|rd|th)$ ]]; thenThere was a problem hiding this comment.
対応しました (4e1b4ea)。head と base の両方が draft パターンに一致する場合のみ同期 PR として許可するようにしました。なお補足すると、head が非 draft の PR は従来も後段の分類 (draft_type=none) で許可されていたため実挙動のバイパスにはなりませんが、許可条件を必要最小限に絞る変更として妥当と判断しました。
| gh pr create --base "$into" --head "$from" \ | ||
| --title "Sync review suggestions from $from into $into" \ | ||
| --body "$(conflict_pr_body "$from" "$into")" | ||
| sync_pr=$(gh pr list --head "$from" --base "$into" --state open \ |
There was a problem hiding this comment.
handle_conflict 関数内で gh pr create 実行後に再度 gh pr list で PR 番号を取得していますが、gh pr create の出力（URL）から番号を直接取得する方が確実です。例:
local pr_url
pr_url=$(gh pr create --base "$into" --head "$from" \
--title "..." --body "...")
sync_pr=$(echo "$pr_url" | grep -oE '[0-9]+$')現在の実装では、gh pr create と2回目の gh pr list の間に別のプロセスが同じ PR を作成した場合（競合状態）に誤った PR 番号を取得する可能性があります。
There was a problem hiding this comment.
対応しました (4e1b4ea)。gh pr create の出力 URL 末尾から PR 番号を直接取得するようにしました (sync_pr="${pr_url##*/}")。
| fi | ||
|
|
||
| local review_pr | ||
| review_pr=$(gh pr list --head "$from" --state open \ |
There was a problem hiding this comment.
review_pr を検索する際、--head "$from" で open な PR を検索し、base が $into でないものを選んでいます。しかし、$from ブランチを head とする PR が複数存在する場合（例: 同一ブランチから複数の PR が open）、意図しない PR にコメントする可能性があります。また、$from が PUSHED_BRANCH ではなくチェーン途中のブランチになった場合（コンフリクトが2段目以降で発生した場合）、そのブランチのレビュー PR を正しく特定できるか確認が必要です。
There was a problem hiding this comment.
対応しました (4e1b4ea)。「base が $into でない」ではなく「base が draft パターンに一致しない」PR を選ぶように強化しました。これにより同期 PR (base が draft) はチェーン途中のものも含めて除外され、レビュー PR (base=main 等) だけが対象になります。チェーン途中のブランチでコンフリクトした場合にそのブランチのレビュー PR が存在しないケースは、-n ガードによりコメントをスキップします (同期 PR 自体は作成されるため学生への導線は確保されます)。
| # Returns 1 on merge conflict, 2 when the push keeps failing. | ||
| sync_pair() { | ||
| local from="$1" into="$2" attempt | ||
| for attempt in 1 2; do |
There was a problem hiding this comment.
ℹ️ [LOW] sync_pair 関数内でリトライ時に git merge --abort || true を実行していませんが、1回目の push 失敗後（merge は成功している状態）に2回目のループで再度 git fetch → git checkout -B → git merge を実行するため、前回の merge 結果が上書きされます。これは意図した動作ですが、コメントがないと混乱しやすいです。また、push 失敗後に git checkout -B "$into" "origin/$into" でリモートの最新状態に戻してから再 merge する点は正しい実装です。ただし、2回目のループ開始時に前回の merge 済みローカルブランチが残っている状態で git checkout -B が正しくリセットされることを明示するコメントを追加することを推奨します。
There was a problem hiding this comment.
対応しました (4e1b4ea)。リトライ時に git checkout -B が fetch 済みのリモート最新状態へリセットし、前回の merge 結果を破棄して再 merge することを明示するコメントを追加しました。あわせて actionlint (SC2034) 指摘のループ変数 attempt を _ に変更しています。
| next_branch() { | ||
| local branch="$1" | ||
| if [[ "$branch" =~ ^([0-9]+)(st|nd|rd|th)-draft$ ]]; then | ||
| # Also covers the 0th-draft -> 1st-draft special case |
There was a problem hiding this comment.
ℹ️ [LOW] コメントに「Also covers the 0th-draft -> 1st-draft special case」とありますが、0th-draft は英語の序数として不正確（0 に対する序数は通常 0th）であり、ordinal(0+1) = ordinal(1) = 1st となるため 0th-draft → 1st-draft の変換は正しく動作します。ただし、0th-draft というブランチ名が実際に使用されるかどうかをドキュメントに明記しておくと良いでしょう。現在の README には 0th-draft → 1st-draft の記載がありますが、create-next-draft.yml との整合性確認を推奨します。
There was a problem hiding this comment.
据え置きとします。0th-draft は既存運用で実際に使われるブランチ名で (create-next-draft.yml にも 0th-draft → 1st-draft の特例が実装済み)、ご指摘の通り ordinal(0+1)=1st により本実装でも正しく変換されます。README の記載 (0th-draft → 1st-draft) とも整合しているため、変更は不要と判断しました。
- Require both head and base to match draft patterns for the draft-to-draft sync PR allowance in prevent-draft-merge - Take the sync PR number from the gh pr create output instead of a second gh pr list (race-safe) - Select the review PR by excluding draft-pattern bases so sync PRs are never picked, including mid-chain conflicts - Clarify the retry behavior of sync_pair and fix SC2034 (unused loop variable) reported by actionlint Refs: smkwlab/sotsuron-template#110
|
レビュー指摘への対応をまとめます (commit 4e1b4ea):
あわせて actionlint の SC2034 (未使用ループ変数 |
|
|
||
| conflict_pr_body() { | ||
| local from="$1" into="$2" | ||
| cat <<EOF |
There was a problem hiding this comment.
🚨 [HIGH] conflict_pr_body 関数内のヒアドキュメントのインデントに注意が必要です。cat <<EOF の後の本文が8スペースインデントされており、PRボディにそのままインデントが含まれてしまいます。cat <<-EOF を使用してタブによるインデント除去を行うか、ヒアドキュメントの内容をインデントしないようにする必要があります（現状ではGitHub PRのMarkdownレンダリングに影響する可能性があります）。
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
There was a problem hiding this comment.
🚨 [HIGH] actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 のコメントに # v7.0.0 とありますが、actions/checkout の最新安定版は v4 系です。v7.0.0 は存在しないバージョンの可能性があります。コミットハッシュが正しければ動作しますが、コメントのバージョン表記を確認・修正してください。
| # Returns 1 on merge conflict, 2 when the push keeps failing. | ||
| sync_pair() { | ||
| local from="$1" into="$2" | ||
| for _ in 1 2; do |
There was a problem hiding this comment.
ℹ️ [LOW] リトライループが for _ in 1 2 の2回固定になっています。2回目の push も失敗した場合、ループを抜けて return 2 に到達しますが、ループ内で return 2 に到達するパスがないため、実際には2回目の push 失敗後にループが終了して return 2 が実行されます。これは意図通りですが、コードの意図が少し読みにくいです。
より重要な問題として、sync_pair 内で git checkout -B "$into" "origin/$into" を実行していますが、この関数はメインスクリプトと同じシェルプロセスで実行されます。リトライ時に git fetch で最新状態を取得し直してから checkout -B でリセットする設計は正しいですが、git merge --abort || true の後に continue や break がなく、そのまま return 1 しているため、ループの2回目には入りません。つまり、コンフリクト時は1回しか試行されません（これは正しい動作ですが、コメントの「retry once on push race」との整合性のため、コンフリクト時は即 return 1 することを明示するとよいでしょう）。
| # The review PR of "from" is the open PR whose base is not a | ||
| # draft branch (draft-based PRs are sync PRs, also mid-chain) | ||
| local review_pr | ||
| review_pr=$(gh pr list --head "$from" --state open \ |
There was a problem hiding this comment.
ℹ️ [LOW] handle_conflict 関数内で review_pr を検索する際の jq フィルタが複雑です。test("^[0-9]+(st|nd|rd|th)-draft$|^abstract-[0-9]+(st|nd|rd|th)$") は正規表現として正しいですが、gh pr list --head "$from" は同名 head ブランチを持つ全リポジトリのPRを返す可能性があります。--repo オプションを明示的に指定することを検討してください（GH_TOKEN と GITHUB_REPOSITORY 環境変数で自動解決されるため実害はないかもしれませんが、明示的な方が安全です）。
| } | ||
|
|
||
| current="$PUSHED_BRANCH" | ||
| synced="" |
There was a problem hiding this comment.
ℹ️ [LOW] synced 変数にブランチ名をスペース区切りで追記し、後で for b in $synced でイテレートしています。ブランチ名にスペースが含まれる場合（通常はないですが）に問題が生じます。また、先頭にスペースが付く形（synced="$synced $next"）になるため、配列を使う方がより堅牢です:
synced=()
# ...
synced+=("$next")
# ...
for b in "${synced[@]}"; do| # into the next draft branch). Both head and base must match | ||
| # a draft pattern. | ||
| echo "::debug::Base branch: $BASE_REF" | ||
| if { [[ "$BRANCH_NAME" =~ ^[0-9]+(st|nd|rd|th)-draft$ ]] || \ |
There was a problem hiding this comment.
ℹ️ [LOW] draft-to-draft 同期PRの許可チェックが、ブランチ名の空チェック（line 77付近）より前に実行されています。BRANCH_NAME が空の場合でも正規表現 ^[0-9]+... はマッチしないため実害はありませんが、ロジックの順序として空チェックを先に行う方が防御的です。
1 participant
概要
draft ブランチへの push (レビュー PR での suggestion 受け入れを含む) を、後続の draft ブランチへ連鎖的に自動 merge する再利用ワークフロー
sync-next-draft.ymlを追加します。PR 作成時に
create-next-draft.ymlが次稿ブランチを先行作成するため、その後に受け入れた suggestion が次稿ブランチに反映されない問題 (smkwlab/sotsuron-template#110) への対応です。学生の手動操作 (smkwlab/sotsuron-template#111 のスクリプト案) を不要にします。変更内容
新規:
.github/workflows/sync-next-draft.ymlworkflow_callで提供。caller 側は draft ブランチへのpushをトリガーに呼び出す (caller 例はファイルヘッダと README に記載)1st-draft→2nd-draft→3rd-draft→ …)。GITHUB_TOKEN による push は新しい workflow run を発火しないため、1 回の実行でチェーン全体を処理concurrencyで同一リポジトリ内の実行を直列化修正:
.github/workflows/prevent-draft-merge.ymlbase も draft ブランチである PR (上記の同期 PR) をマージ許可対象に追加。従来は head が draft パターンだと base を問わずブロックしていたため、同期 PR が誤ブロックされる問題への対応。既存の final-* タグ判定・draft ブロック動作は変更なし (非破壊)。
ドキュメント:
README.mdsync-next-draft.ymlを追加prevent-draft-merge.ymlの説明に同期 PR 許可を追記バージョニング
全ワークフローに対して非破壊 (新規追加 + 許可条件の追加) のため、merge 後に
v1.25.0を発行し floatingv1を付け替える想定です。今後の展開 (別 PR / 別作業)
Refs: smkwlab/sotsuron-template#110