fix: restrict sync PR allowance to base == next draft of head#99
Conversation
Review PRs of 2nd and later drafts are themselves draft-to-draft (e.g. head 2nd-draft, base 1st-draft), so allowing any draft-to-draft PR in prevent-draft-merge disabled the merge guard for them. Only allow the sync direction: base must be exactly the next draft of the head branch (e.g. head 1st-draft -> base 2nd-draft). Also revert the review-PR lookup in sync-next-draft to "base != sync target", because review PRs of later drafts have draft bases and the draft-pattern exclusion could never find them. Refs: smkwlab/sotsuron-template#110
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| NEXT_OF_HEAD="" | ||
| if [[ "$BRANCH_NAME" =~ ^([0-9]+)(st|nd|rd|th)-draft$ ]]; then | ||
| NEXT_OF_HEAD="$(ordinal $((BASH_REMATCH[1] + 1)))-draft" |
There was a problem hiding this comment.
ordinal 関数をインラインで定義するとサブシェル問題が発生する可能性があります
run: | ブロック内でシェル関数を定義し、その後 $(ordinal ...) のようにコマンド置換で呼び出す場合、シェルの実装によっては関数がサブシェルに引き継がれないことがあります。GitHub Actions の run: ブロックは bash -e で実行されますが、$(...) はサブシェルを生成するため、ordinal 関数が未定義になるリスクがあります。
実際には bash では関数はサブシェルに継承されるため多くの場合動作しますが、より安全な実装として関数をインライン展開するか、以下のように変数に計算結果を格納する方法を検討してください:
if [[ "$BRANCH_NAME" =~ ^([0-9]+)(st|nd|rd|th)-draft$ ]]; then
n=$(( BASH_REMATCH[1] + 1 ))
# ordinal 計算をインラインで実施
last_two=$((n % 100)); last_one=$((n % 10))
if [[ $last_two -ge 11 && $last_two -le 13 ]]; then suffix="th"
elif [[ $last_one -eq 1 ]]; then suffix="st"
elif [[ $last_one -eq 2 ]]; then suffix="nd"
elif [[ $last_one -eq 3 ]]; then suffix="rd"
else suffix="th"; fi
NEXT_OF_HEAD="${n}${suffix}-draft"
fiこれにより関数定義・サブシェル起動のオーバーヘッドと潜在的なスコープ問題を回避できます。
| review_pr=$(gh pr list --head "$from" --state open \ | ||
| --json number,baseRefName \ | ||
| --jq '[.[] | select(.baseRefName | test("^[0-9]+(st|nd|rd|th)-draft$|^abstract-[0-9]+(st|nd|rd|th)$") | not)][0].number // empty') | ||
| --jq "[.[] | select(.baseRefName != \"$into\")][0].number // empty") |
There was a problem hiding this comment.
$into 変数の展開によるインジェクションリスク
変数 $into をダブルクォートで囲んだ文字列内に直接展開して jq フィルタに渡しています:
--jq "[.[] | select(.baseRefName != \"$into\")][0].number // empty"$into にブランチ名として "（ダブルクォート）や \ などの特殊文字が含まれる場合、jq の構文エラーや意図しない動作を引き起こす可能性があります。実際のブランチ名はパターンが決まっているため現実的なリスクは低いですが、--argjson や --arg を使う方がより堅牢です:
--jq '[.[] | select(.baseRefName != $into)][0].number // empty' \
--arg into "$into"これにより jq 側で安全に変数を扱えます。
| elif [[ "$BRANCH_NAME" =~ ^abstract-([0-9]+)(st|nd|rd|th)$ ]]; then | ||
| NEXT_OF_HEAD="abstract-$(ordinal $((BASH_REMATCH[1] + 1)))" | ||
| fi | ||
| if [ -n "$NEXT_OF_HEAD" ] && [ "$BASE_REF" = "$NEXT_OF_HEAD" ]; then |
There was a problem hiding this comment.
✨ [POSITIVE] [POSITIVE] 許可条件を「base が head の次稿と完全一致する場合のみ」に限定した修正は明確で正確です
旧実装の「両方が draft パターンであれば許可」という条件では、レビュー PR（逆方向）も誤って許可してしまうという regression を、序数計算による厳密な方向チェックで解決しています。コメントも方向性の違いを明確に説明しており、可読性・保守性が向上しています。
1 participant
概要
PR #98 のレビュー指摘対応で入れた「head と base が両方 draft パターンなら merge 許可」に regression があったため修正します。
問題
このエコシステムでは 2 稿目以降のレビュー PR 自体が draft→draft です (例: head
2nd-draft/ base1st-draft、sotsuron-template の docs/CLAUDE-WORKFLOWS.md「各 draft PR は直前の draft をベースとする」)。このため PR #98 の条件では、2 稿目以降のレビュー PR (マージしてはいけない) までprevent-draft-mergeが許可してしまいます。abstract-1st のレビュー PR (base が最新 draft) も同様です。修正内容
prevent-draft-merge.yml同期 PR とレビュー PR は方向で区別できます:
1st-draft→ base2nd-draft)2nd-draft→ base1st-draft)許可条件を「base が head の次稿と完全一致する場合」のみに限定しました (序数計算で next を求めて比較)。レビュー PR の方向・cross-series (abstract→Nth-draft 等) はすべてブロック継続です。
sync-next-draft.ymlコンフリクト時のレビュー PR 特定を「base が draft パターンでない」から「base ≠ 同期先ブランチ」に戻しました。2 稿目以降のレビュー PR は base が draft のため、draft パターン除外では永遠に見つけられず通知が届かないためです。
検証観点
1st-draft/ base2nd-draft(同期 PR) → 許可2nd-draft/ base1st-draft(レビュー PR) → ブロックabstract-1st/ base5th-draft(概要レビュー PR) → ブロック0th-draft/ base1st-draft(同期 PR) → 許可merge 後は
v1.25.1を発行しv1を付け替えます (非破壊のバグ修正)。Refs: smkwlab/sotsuron-template#110, #98