Skip to content

fix(scripts): declare the pull-request requirement and cover the elixir repos #138

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 3 commits into from
Aug 4, 2026
Merged

fix(scripts): declare the pull-request requirement and cover the elixir repos #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9943129
fix(scripts): declare the pull-request requirement and cover the elix…
toshi0806 Aug 4, 2026
ba6127f
docs(config): state that only the declared review fields are managed
toshi0806 Aug 4, 2026
d1544c4
fix(scripts): stop when review_settings is missing from the declaration
toshi0806 Aug 4, 2026
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
69 changes: 66 additions & 3 deletions config/dev-infra-protection.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,24 @@
"$schema-note": "開発インフラリポジトリのブランチ保護とマージ設定の desired state。方針は https://github.com/smkwlab/latex-ecosystem/blob/main/docs/DEPENDENCY-MANAGEMENT.md を参照。学生リポジトリは対象外で、student-repo-management の setup-branch-protection.sh が別に管理する。",
"branch": "main",
"invariants": {
"allow_auto_merge": "false 固定。GitHub の auto-merge はブランチ保護の要件が満たされた時点でマージするため、required status checks が空のリポジトリでは CI 完了前に入る。マージは Renovate 自前の automerge に任せる",
"allow_auto_merge": "false 固定。GitHub の auto-merge はブランチ保護の要件が満たされた時点でマージするため、required に入っていない check を待たない。マージは Renovate 自前の automerge に任せる",
"enforce_admins": "false 固定。CI が壊れた緊急時に管理者が明示的に突破できるようにする。Renovate App はブランチ保護をバイパスしないので bot 側は必ずゲートされる",
"strict": "false 固定。up-to-date 要求を付けると、1 本マージするたび全 PR が rebase と再ビルドになる",
"contexts": "その PR で必ず check run が生成されるジョブだけを列挙する。job レベルの if: で skip されたジョブは conclusion=skipped の check run が出るので指定してよい。workflow レベルの paths: / branches: フィルタで発火しない workflow は check run 自体が出ず永久 pending になるので指定してはいけない"
"strict": "latex 系は false。up-to-date 要求を付けると、1 本マージするたび全 PR が rebase と再ビルドになる。elixir 系は true のまま置いている。マージが月 0〜4 件で再ビルドの費用が出ておらず、個別には緑でも組み合わせると壊れる PR の検出を優先している",
"contexts": "その PR で必ず check run が生成されるジョブだけを列挙する。job レベルの if: で skip されたジョブは conclusion=skipped の check run が出るので指定してよい。workflow レベルの paths: / branches: フィルタで発火しない workflow は check run 自体が出ず永久 pending になるので指定してはいけない",
"require_pull_request": "true なら「Require a pull request before merging」を有効にする。これが無いと main へ直接 push できる。有効な 9 リポジトリは設定内容が完全に同じなので、形は review_settings に一度だけ書き、リポジトリごとには有無だけを宣言する。latex-environment と tenbin_cache は現在無効で、揃えるかどうかは未決。現状を写して監査対象に載せてある",
"review_settings": "ここに書いたフィールドだけを管理する。require_last_push_approval や bypass_pull_request_allowances のような書いていないフィールドは、audit では突き合わせず、apply では PUT の全項目置換によって API の既定値に戻る。現在 11 リポジトリすべてが既定値なので両者は一致している。既定から外した値を残したいなら、ここに書き足すこと"
},
"review_settings": {
"required_approving_review_count": 0,
"dismiss_stale_reviews": true,
"require_code_owner_reviews": false
},
"repositories": [
{
"name": "texlive-ja-textlint",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": false,
"contexts": ["changes", "build-alpine", "build-debian", "build-debian-arm64"]
Expand All @@ -21,6 +29,7 @@
"name": "latex-environment",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": false,
"required_status_checks": {
"strict": false,
"contexts": ["build-and-release-pdf"]
Expand All @@ -30,6 +39,7 @@
"name": "latex-release-action",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": false,
"contexts": ["yaml-lint", "test-build"]
Expand All @@ -39,6 +49,7 @@
"name": "ai-academic-paper-reviewer",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": false,
"contexts": ["test"]
Expand All @@ -48,6 +59,7 @@
"name": "student-repo-management",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": false,
"contexts": ["Validate YAML files"]
Expand All @@ -57,10 +69,61 @@
"name": ".github",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": false,
"contexts": ["actionlint"]
}
},
{
"name": "tenbin_dns",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": true,
"contexts": ["ci / Code Quality", "ci / All checks"]
}
},
{
"name": "tdig",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": true,
"contexts": ["ci / Code Quality", "ci / All checks"]
}
},
{
"name": "tenbin_ex",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": true,
"contexts": ["ci / Code Quality", "ci / All checks"]
}
},
{
"name": "tenbin_cache",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": false,

@github-actions github-actions Bot Aug 4, 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.

ℹ️ [LOW] tenbin_cache の require_pull_request: false と strict: true の組み合わせ

tenbin_cache は require_pull_request: false（PR 不要）かつ strict: true（up-to-date 必須）という設定になっています。PR が不要なら strict の設定は実質的に意味を持ちません。

invariants に「揃えるかどうかは未決」と記載されており、現状を写したものであることは理解できますが、この矛盾した組み合わせについて invariants または当該エントリのコメントで明示的に言及しておくと、将来の混乱を防げます。

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

鋭い指摘です。ただし「実質的に意味を持たない」よりもう少し込み入っています。

strict は無意味にはなりません。 required status checks は PR のマージだけでなく、保護ブランチへの push にも効きます。tenbin_cache に PR 必須が無くても、checks が緑でないコミットは push できません。

むしろ効いているのは enforce_admins: false の方です。管理者はブランチ保護を突破できるので、require_pull_request: false と組み合わさると 管理者は tenbin_cache と latex-environment の main へ直接 push できます。これらは実質的に単独メンテナのリポジトリなので、現実的にはこの経路が開いています。

それが望ましいかどうかが invariants に書いた「揃えるかどうかは未決」の中身です。この PR は現状を写して監査対象に載せるところまでで、値の是非は変えていません（変えるなら 2 リポジトリの保護を強める操作になり、別の判断が要ります）。

指摘の趣旨（この組み合わせが目に付く形で残っている）はそのとおりなので、判断が必要になったときのために、いま監査で毎週報告される状態にしてあります。値を変えれば drift として出ます。

"required_status_checks": {
"strict": true,
"contexts": ["ci / Code Quality", "ci / All checks"]
}
},
{
"name": "elixir_dnstap",
"allow_auto_merge": false,
"enforce_admins": false,
"require_pull_request": true,
"required_status_checks": {
"strict": true,
"contexts": ["ci / Code Quality", "ci / All checks"]
}
}
]
}
18 changes: 13 additions & 5 deletions scripts/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,14 @@ caller を追加したら対象リポジトリで動作を確認してくださ
[依存管理基盤（Renovate 一本化）](https://github.com/smkwlab/latex-ecosystem/blob/main/docs/DEPENDENCY-MANAGEMENT.md)
にあります。

対象は `texlive-ja-textlint` / `latex-environment` / `latex-release-action` /
`ai-academic-paper-reviewer` / `student-repo-management` / `.github` の 6 つ。学生
リポジトリは対象外で、`student-repo-management` の `setup-branch-protection.sh` が
別に管理します。
対象は 11 リポジトリです。latex 系が `texlive-ja-textlint` / `latex-environment` /
`latex-release-action` / `ai-academic-paper-reviewer` / `student-repo-management` /
`.github` の 6 つ、elixir 系が `tenbin_dns` / `tdig` / `tenbin_ex` / `tenbin_cache` /
`elixir_dnstap` の 5 つ。学生リポジトリは対象外で、`student-repo-management` の
`setup-branch-protection.sh` が別に管理します。

値は系統ごとに違うところがあります（elixir 系は `strict: true`）。理由は desired
state の `invariants` に書いてあります。

```bash
# 乖離があれば非ゼロ終了（読み取りのみ）
Expand Down Expand Up @@ -150,7 +154,11 @@ secret が未設定のときは skip せず失敗します。何も見ていな
一部フィールドが黙って落ちる事例が出ています（smkwlab/student-repo-management#577）。
管理者の PAT で手動実行してください。audit は読み取りのみなので App token でも動きます
- ブランチ保護の PUT は**全項目置換**です。desired state が宣言していない項目は消えます。
宣言を増やすときは apply スクリプトの送信ペイロードも合わせて広げること
宣言を増やすときは apply スクリプトの送信ペイロードも合わせて広げること。
実際に踏んだ例として、`required_pull_request_reviews` を宣言せず `null` で送っていた
時期があり、`--apply` すると「Require a pull request before merging」が外れて
main へ直接 push できる状態になっていました（現在は `require_pull_request` として
宣言しています）
- `contexts` には**その PR で必ず check run が生成されるジョブだけ**を並べます。
workflow レベルの `paths:` / `branches:` フィルタで発火しない workflow を required に
すると、非該当 PR が永久 pending になります（job レベルの `if:` による skip は
Expand Down
21 changes: 18 additions & 3 deletions scripts/apply-repo-protection.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ log() {

branch=$(jq -r '.branch' "$CONFIG_PATH")
count=$(jq '.repositories | length' "$CONFIG_PATH")
# require_pull_request が true のリポジトリに送る中身。有効な側は設定が全て同じ
# なので宣言では 1 箇所にまとめてある。
#
# 欠けていたら止める。null のまま進むと required_pull_request_reviews に null を
# 送ることになり、「Require a pull request before merging」を外す動作に戻る
review_settings=$(jq -c '.review_settings' "$CONFIG_PATH")

@github-actions github-actions Bot Aug 4, 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] review_settings が JSON として正しく取得できなかった場合のエラーハンドリングがありません。

review_settings=$(jq -c '.review_settings' "$CONFIG_PATH")

この時点で review_settings が null（設定ファイルにキーが存在しない場合）になっても、後続の jq --argjson reviews "$review_settings" は null を渡すことになり、require_pull_request: true のリポジトリに対して null が設定されてしまいます。これは修正前と同じ問題を引き起こします。

以下のようなガード節を追加することを検討してください：

review_settings=$(jq -c '.review_settings' "$CONFIG_PATH")
if [ -z "$review_settings" ] || [ "$review_settings" = "null" ]; then
    log "ERROR: review_settings が設定ファイルに存在しません"
    exit 1
fi

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

妥当な指摘です。ガードを入れました（d1544c4）。両スクリプトに追加しています。

review_settings が欠けると jq -c は文字列 "null" を返し、--argjson reviews null を経て required_pull_request_reviews: null を送ることになります。この PR で直している欠陥が、宣言側の削除だけで黙って戻るということなので、止める価値があります。

audit 側にも同じガードを入れました。こちらは null のまま進むと全リポジトリが drift として報告され、実際にはずれていないのに「ずれている」という報告が毎週出ることになります。

終了コードも確認しました。

$ CONFIG_PATH=<review_settings を削除した宣言> bash scripts/audit-repo-protection.sh
review_settings が宣言に無い。期待値が決まらないので中止する
exit=1

$ 同 apply
review_settings が宣言に無い。require_pull_request の適用先が決まらないので中止する
exit=1

$ 正常系
exit=0

週次 workflow は非ゼロ終了で失敗扱いにするので、宣言が壊れた状態は「何も見ていない監査が success を返す」ではなく赤として出ます。

if [ -z "$review_settings" ] || [ "$review_settings" = "null" ]; then
log "review_settings が宣言に無い。require_pull_request の適用先が決まらないので中止する"
exit 1
fi

log "desired state: $CONFIG_PATH"
log "対象: ${count} リポジトリ (org: ${ORG}, branch: ${branch})"
Expand Down Expand Up @@ -86,9 +96,10 @@ while IFS= read -r spec; do
am=$(printf '%s' "$spec" | jq -r '.allow_auto_merge')
admins=$(printf '%s' "$spec" | jq -r '.enforce_admins')
checks=$(printf '%s' "$spec" | jq -c '.required_status_checks')
pr_required=$(printf '%s' "$spec" | jq -r '.require_pull_request')

if [ "$APPLY" != "true" ]; then
log " would apply: ${name} — allow_auto_merge=${am} enforce_admins=${admins} checks=${checks}"
log " would apply: ${name} — allow_auto_merge=${am} enforce_admins=${admins} require_pull_request=${pr_required} checks=${checks}"
applied=$((applied + 1))
continue
fi
Expand All @@ -102,10 +113,14 @@ while IFS= read -r spec; do
log " ERROR: ${name} — allow_auto_merge を設定できなかった: ${err}"
fi

body=$(printf '%s' "$spec" | jq '{
# required_pull_request_reviews は宣言から組み立てる。PUT は全項目置換なので、
# ここを無条件に null にすると「Require a pull request before merging」が外れ、
# main へ直接 push できる状態になる。宣言している 11 リポジトリのうち 9 つが
# この設定を持っており、null 固定は保護を落とす操作だった
body=$(printf '%s' "$spec" | jq --argjson reviews "$review_settings" '{
required_status_checks: .required_status_checks,
enforce_admins: .enforce_admins,
required_pull_request_reviews: null,
required_pull_request_reviews: (if .require_pull_request then $reviews else null end),
restrictions: null
}')
if ! err=$(printf '%s' "$body" | gh api -X PUT \
Expand Down
25 changes: 25 additions & 0 deletions scripts/audit-repo-protection.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ log() {

branch=$(jq -r '.branch' "$CONFIG_PATH")
count=$(jq '.repositories | length' "$CONFIG_PATH")
# require_pull_request が true のリポジトリに期待する中身。
# 欠けていたら止める。null のまま進むと全リポジトリが drift として報告され、
# 実際にはずれていないのに「ずれている」と読める報告が毎週出ることになる
review_settings=$(jq -cS '.review_settings' "$CONFIG_PATH")

@github-actions github-actions Bot Aug 4, 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.

ℹ️ [LOW] jq -cS の -S フラグについて

review_settings の取得時に jq -cS を使用してキーをソートしていますが、got_reviews の取得時（148行目）も jq -cS でソートしているため、比較は正しく機能します。

ただし、apply-repo-protection.sh の review_settings 取得では jq -c（ソートなし）を使用しています（64行目）。apply 側では文字列比較ではなく jq の --argjson でオブジェクトとして渡すため実害はありませんが、両スクリプトで取得方法が異なる点は将来の混乱の元になる可能性があります。コメントで意図を明示しておくと良いでしょう。

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

据え置きます。ご指摘のとおり実害はなく、取得方法が違うのは用途が違うためです。

  • audit は jq -cS — got_reviews と文字列として比較するので、両辺でキー順を揃える必要がある
  • apply は jq -c — --argjson でオブジェクトとして渡すだけで、比較しない。キー順は payload の意味に影響しない

audit 側にだけ -S が要る、という関係です。apply に -S を足しても動きますが、「なぜソートしているのか」が読めなくなる分かえって紛らわしくなります。

コメントで意図を書く案は採りませんでした。-S の意味（sort keys）は jq の基本オプションで、直後に文字列比較が続く文脈から用途は読めます。今日この PR で入れたコメントは、コードから読み取れないこと（PUT の全項目置換で保護が外れる、宣言していないフィールドは既定に戻る）に絞っています。

if [ -z "$review_settings" ] || [ "$review_settings" = "null" ]; then
log "review_settings が宣言に無い。期待値が決まらないので中止する"
exit 1
fi

log "desired state: $CONFIG_PATH"
log "対象: ${count} リポジトリ (org: ${ORG}, branch: ${branch})"
Expand Down Expand Up @@ -130,6 +138,23 @@ while IFS= read -r spec; do
report "${name}: enforce_admins want=${want_admins} got=${got_admins}"
fi

# 「Require a pull request before merging」の有無。これが外れると main へ
# 直接 push できるようになるが、PR も check run も出ないので気付く手段が無い
want_pr=$(printf '%s' "$spec" | jq -r '.require_pull_request')
got_pr=$(printf '%s' "$prot" | jq -r '.required_pull_request_reviews != null')

@github-actions github-actions Bot Aug 4, 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.

ℹ️ [LOW] got_pr の取得に jq -r '.required_pull_request_reviews != null' を使っており、これは true または false の文字列を返します。一方 want_pr は JSON の true/false を -r で文字列化したものです。

通常は一致しますが、GitHub API が required_pull_request_reviews キー自体を省略して返す場合（キーが存在しない場合）、jq は null != null → false を返すため意図通りに動作します。ただし、API レスポンスの構造変化に対して脆弱です。より堅牢にするなら .required_pull_request_reviews // null | . != null のように明示的に書くか、has("required_pull_request_reviews") を使うことを検討してください。

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

据え置きます。ご指摘の分析どおり、現状の書き方で意図どおり動きます（キーが無い場合も null != null → false）。

提案の 2 案も挙動は同じです。

  • .required_pull_request_reviews // null | . != null — // は左辺が null か false のとき右辺を返すので、結果は現状と同一
  • has("required_pull_request_reviews") — こちらは意味が変わります。キーが存在して値が null の場合に true を返すため、「保護が無い」を「保護がある」と誤判定します

jq の != null は「キーが無い」と「値が null」を同じに扱うので、この用途では最も素直です。API がキーを省略する形に変わっても壊れません。

なお enforce_admins の比較（.enforce_admins.enabled）も同じ流儀で、こちらはキーが消えると null になって want と一致せず drift として出ます。API の構造変化は「黙って通る」ではなく「drift として出る」方に倒れるので、監査としては安全側です。

@github-actions github-actions Bot Aug 4, 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.

ℹ️ [LOW] require_pull_request が false の場合の審査が不完全な可能性があります。

現在のロジックは以下の通りです：

got_pr=$(printf '%s' "$prot" | jq -r '.required_pull_request_reviews != null')

GitHub API が required_pull_request_reviews フィールド自体を返さない場合（フィールドが存在しない）、jq は null を返し、null != null は false となるため正しく動作します。しかし、API がフィールドを返すが値が null の場合も同様に false となります。

一方、want_pr=false かつ got_pr=false の場合は一致と判定されますが、実際には required_pull_request_reviews が空オブジェクト {} で返ってくるケースなど、API の応答形式が変わった際に誤検知が起きる可能性があります。

現時点では実測で問題ないとのことですが、latex-environment や tenbin_cache（require_pull_request: false）で実際に got_pr が期待通り false になることを継続的に確認できる仕組みがあると安心です。

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

据え置きます。ご指摘の「継続的に確認できる仕組み」は、このスクリプト自体がそれです。

latex-environment と tenbin_cache は require_pull_request: false として宣言してあり、週次監査が毎週この 2 つも突き合わせます。got_pr が期待どおり false にならなくなれば（API が {} を返すようになるなど）、その週の監査が drift として報告し、workflow が赤くなります。今日の実行でも 11 リポジトリすべてが ok でした。

つまり「API の応答形式が変わったら誤検知が起きる」ではなく、変わったら報告される方に倒れています。監査対象に入れたことがそのまま継続確認になっている、という関係です。

なお {} が返る形になった場合は != null が true になるので、want=false のこの 2 つで drift として出ます。黙って通る側には倒れません。

@github-actions github-actions Bot Aug 4, 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.

ℹ️ [LOW] got_pr の判定ロジックの脆弱性

got_pr=$(printf '%s' "$prot" | jq -r '.required_pull_request_reviews != null')

この判定は GitHub API が required_pull_request_reviews フィールドを返さない場合（フィールド自体が存在しない場合）に true を返してしまいます。jq では存在しないフィールドは null として扱われるため、.required_pull_request_reviews != null は null != null = false となり正しく動作しますが、API レスポンスの構造が変わった場合のリスクがあります。

より堅牢にするなら has("required_pull_request_reviews") and .required_pull_request_reviews != null とすることを検討してください。ただし現状の GitHub API では null が返ることはなく省略されるため、現在の実装で実害はありません。

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

同じ箇所への 2 回目の指摘で、前のスレッドで回答済みです。据え置きます。

なお今回の本文は冒頭で「フィールドを返さない場合に true を返してしまいます」と書いたあと、同じ段落で「null != null = false となり正しく動作します」と結論しており、前半は誤りです（後半が正しい）。

提案の has("required_pull_request_reviews") and .required_pull_request_reviews != null は、has を足しても結果が変わりません。キーが無ければ後半が false になるためです。冗長になるだけなので採りません。

if [ "$want_pr" != "$got_pr" ]; then
report "${name}: require_pull_request want=${want_pr} got=${got_pr}"
elif [ "$want_pr" = "true" ]; then
got_reviews=$(printf '%s' "$prot" | jq -cS '.required_pull_request_reviews | {
required_approving_review_count,
dismiss_stale_reviews,
require_code_owner_reviews
}')
if [ "$review_settings" != "$got_reviews" ]; then

@github-actions github-actions Bot Aug 4, 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] GitHub API が返す required_pull_request_reviews オブジェクトには、required_approving_review_count / dismiss_stale_reviews / require_code_owner_reviews 以外にも追加フィールド（例: require_last_push_approval、bypass_pull_request_allowances など）が含まれる場合があります。

現在の実装では jq -cS で3フィールドだけを抽出して比較しているため、宣言していないフィールドが API 側でデフォルト値と異なる状態になっていても drift として検出されません。これは意図的な設計（宣言したフィールドのみ監査する）であれば問題ありませんが、その旨をコメントに明記しておくと将来の混乱を防げます。

また、apply-repo-protection.sh 側では $reviews（review_settings の内容）をそのまま PUT ペイロードに渡しているため、宣言していないフィールドが API のデフォルト値にリセットされる可能性があります。audit と apply の対称性を保つためにも、どちらのスクリプトでも「宣言したフィールドのみを扱う」という方針を明示することを推奨します。

@toshi0806 toshi0806 Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

明記しました（ba6127f）。指摘のとおり、apply 側は宣言していないフィールドを API 既定値に戻します（audit は突き合わせません）。

"review_settings": "ここに書いたフィールドだけを管理する。require_last_push_approval や
bypass_pull_request_allowances のような書いていないフィールドは、audit では突き合わせず、
apply では PUT の全項目置換によって API の既定値に戻る。現在 11 リポジトリすべてが既定値
なので両者は一致している。既定から外した値を残したいなら、ここに書き足すこと"

invariants に置いたのは、review_settings の中に $note として書くと PUT の payload にそのまま混ざるためです（実際に確認しました）。invariants は送信対象外なので安全です。

実測もしました。宣言していない 3 フィールドは 9 リポジトリすべて API 既定値です。

texlive-ja-textlint  last_push=false bypass=false dismissal=false
…（9 リポジトリすべて同じ）

したがって現時点で apply がリセットするものはありません。ただしご指摘のとおり「宣言していない = 保たれる」ではなく「宣言していない = 既定に戻る」が正しい理解なので、それを書いた形です。desired state を一方的に送るスクリプトの性質そのもので、scripts/README.md の注意書きにも同じ趣旨が既にあります。

report "${name}: review settings want=${review_settings} got=${got_reviews}"
fi
fi

want_strict=$(printf '%s' "$spec" | jq -r '.required_status_checks.strict')
# `// "なし"` は使えない。jq の // は false も空として扱うため、
# strict=false が「なし」に化ける
Expand Down