Skip to content

fix: restrict sync PR allowance to base == next draft of head #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
toshi0806 merged 1 commit into from
Jul 15, 2026
Merged

fix: restrict sync PR allowance to base == next draft of head #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions .github/workflows/prevent-draft-merge.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,31 @@ jobs:
fi
fi

# Allow draft-to-draft sync PRs (e.g. created by
# sync-next-draft.yml to carry accepted review suggestions
# into the next draft branch). Both head and base must match
# a draft pattern.
# Allow draft-to-draft sync PRs created by sync-next-draft.yml:
# the base must be exactly the NEXT draft of the head branch
# (e.g. head 1st-draft -> base 2nd-draft). Review PRs go in the
# opposite direction (e.g. head 2nd-draft -> base 1st-draft)
# and stay blocked.
echo "::debug::Base branch: $BASE_REF"
if { [[ "$BRANCH_NAME" =~ ^[0-9]+(st|nd|rd|th)-draft$ ]] || \
[[ "$BRANCH_NAME" =~ ^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)$ ]]; }; then
ordinal() {
local n="$1" last_two last_one
last_two=$((n % 100)); last_one=$((n % 10))
if [[ $last_two -ge 11 && $last_two -le 13 ]]; then echo "${n}th"
elif [[ $last_one -eq 1 ]]; then echo "${n}st"
elif [[ $last_one -eq 2 ]]; then echo "${n}nd"
elif [[ $last_one -eq 3 ]]; then echo "${n}rd"
else echo "${n}th"; fi
}
NEXT_OF_HEAD=""
if [[ "$BRANCH_NAME" =~ ^([0-9]+)(st|nd|rd|th)-draft$ ]]; then
NEXT_OF_HEAD="$(ordinal $((BASH_REMATCH[1] + 1)))-draft"

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [MEDIUM] [HIGH] 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

これにより関数定義・サブシェル起動のオーバーヘッドと潜在的なスコープ問題を回避できます。

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

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ [POSITIVE] [POSITIVE] 許可条件を「base が head の次稿と完全一致する場合のみ」に限定した修正は明確で正確です

旧実装の「両方が draft パターンであれば許可」という条件では、レビュー PR（逆方向）も誤って許可してしまうという regression を、序数計算による厳密な方向チェックで解決しています。コメントも方向性の違いを明確に説明しており、可読性・保守性が向上しています。

{ echo "is_draft=false"; echo "draft_type=sync"; } >> "$GITHUB_OUTPUT"
echo "Draft-to-draft sync PR - merge allowed"
echo "::notice::Head and base are both draft branches - sync PR"
echo "::notice::Base ($BASE_REF) is the next draft of head ($BRANCH_NAME) - sync PR"
exit 0
fi

Expand Down
8 changes: 5 additions & 3 deletions .github/workflows/sync-next-draft.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,14 @@ jobs:
sync_pr="${pr_url##*/}"
fi

# 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)
# The review PR of "from" is the open PR other than the sync
# PR itself. Note: its base may also be a draft branch (review
# PRs of 2nd and later drafts are based on the previous draft),
# so we only exclude the sync target here.
local review_pr
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")

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [MEDIUM] [HIGH] $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 側で安全に変数を扱えます。

if [ -n "$review_pr" ]; then
gh pr comment "$review_pr" --body "受け入れた suggestion を「$into」へ自動 merge できませんでした（コンフリクト）。同期 PR #$sync_pr を開き、「Resolve conflicts」で解決してから merge してください。"
fi
Expand Down
Loading