fix(scripts): declare the pull-request requirement and cover the elixir repos by toshi0806 · Pull Request #138 · smkwlab/.github

toshi0806 · 2026-08-04T14:17:57Z

背景

elixir 系 5 リポジトリを週次監査の対象に入れる作業をしていて、apply-repo-protection.sh に保護を落とす欠陥が見つかった。

body=$(printf '%s' "$spec" | jq '{
    required_status_checks: .required_status_checks,
    enforce_admins: .enforce_admins,
    required_pull_request_reviews: null,   # ← 固定で null
    restrictions: null
}')

ブランチ保護の PUT は全項目置換なので、--apply を実行すると「Require a pull request before merging」が外れ、main へ直接 push できる状態になる。実測すると 11 リポジトリ中 9 つがこの設定を持っている。

既定が dry-run なので暴発はしていないが、これは宣言済みの 6 リポジトリにも当てはまる既存の欠陥で、elixir 系を足すと被害範囲が 11 に広がる。先に塞ぐ。

変更内容

1. `require_pull_request` を宣言に加える

reviews を持つ 9 リポジトリは設定内容が完全に同一（approvals=0 / dismiss_stale=true / code_owner=false）だったので、形は review_settings に一度だけ書き、リポジトリごとには有無だけを宣言する。

"review_settings": {
  "required_approving_review_count": 0,
  "dismiss_stale_reviews": true,
  "require_code_owner_reviews": false
},
"repositories": [
  { "name": "tenbin_dns", "require_pull_request": true, ... }
]

apply はこの宣言から payload を組み立て、audit は有無と中身の両方を突き合わせる。保護が外れても気付く手段が無い設定だったので、監査対象に入れる価値が大きい。

latex-environment と tenbin_cache は現在無効なので false と宣言した。揃えるかどうかは未決で、現状を写して監査に載せている旨を invariants に書いた。

2. elixir 系 5 リポジトリを宣言に追加

tenbin_dns / tdig / tenbin_ex / tenbin_cache / elixir_dnstap。contexts は今日 #117 で追加した ci / Code Quality + ci / All checks。

strict は elixir 系だけ true なので、invariants の記述を「false 固定」から系統ごとの説明に改めた（elixir 系はマージが月 0〜4 件で再ビルドの費用が出ておらず、組み合わせて壊れる PR の検出を優先している）。

検証

宣言と実体が一致すること（11 リポジトリ、drift 0）:

対象: 11 リポジトリ (org: smkwlab, branch: main)
  ok: texlive-ja-textlint … ok: elixir_dnstap
drift: 0 件 / missing: 0 件 / errors: 0 件

新しい検査が実際に drift を捕まえること（負のテスト）:

$ # require_pull_request を反転させた宣言で実行
drift: tdig: require_pull_request want=false got=true

$ # review_settings の approvals を 1 に変えた宣言で実行
drift: tdig: review settings want={"required_approving_review_count":1,…} got={"required_approving_review_count":0,…}

apply は dry-run で 11 リポジトリすべてに require_pull_request が出ることを確認。shellcheck は両スクリプトとも clean。

補足

scripts/README.md の「注意」に、今回踏んだ内容を実例として追記した。全項目置換の PUT に宣言していない項目を渡す危険は既に書かれていたが、実際にそれで保護が外れる箇所が残っていたため。

ref: smkwlab/latex-ecosystem#169, #117

…ir repos

github-actions

全体的に、既存の欠陥（required_pull_request_reviews を無条件に null で送っていた問題）を適切に修正しており、audit スクリプトへの検査追加も論理的に正しい実装です。以下に気になる点を挙げます。

github-actions · 2026-08-04T14:18:35Z

+            dismiss_stale_reviews,
+            require_code_owner_reviews
+        }')
+        if [ "$review_settings" != "$got_reviews" ]; then


⚠️ [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 の対称性を保つためにも、どちらのスクリプトでも「宣言したフィールドのみを扱う」という方針を明示することを推奨します。

明記しました（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 の注意書きにも同じ趣旨が既にあります。

github-actions · 2026-08-04T14:18:35Z

+    # 「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')


ℹ️ [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") を使うことを検討してください。

据え置きます。ご指摘の分析どおり、現状の書き方で意図どおり動きます（キーが無い場合も 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

全体的に、既存の欠陥（required_pull_request_reviews を null 固定で送信していた問題）を適切に修正し、elixir 系リポジトリを監査対象に追加する変更として、設計・実装ともに妥当です。以下に気になる点を挙げます。

github-actions · 2026-08-04T14:21:43Z

 count=$(jq '.repositories | length' "$CONFIG_PATH")
+# require_pull_request が true のリポジトリに送る中身。有効な側は設定が全て同じ
+# なので宣言では 1 箇所にまとめてある
+review_settings=$(jq -c '.review_settings' "$CONFIG_PATH")


⚠️ [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

妥当な指摘です。ガードを入れました（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 を返す」ではなく赤として出ます。

github-actions · 2026-08-04T14:21:43Z

+    # 「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')


ℹ️ [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 になることを継続的に確認できる仕組みがあると安心です。

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

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

全体的に、セキュリティ上の重大な欠陥（required_pull_request_reviews を null 固定で送信していた問題）を適切に修正しており、変更の品質は高いと評価できます。以下に気になる点を挙げます。

github-actions · 2026-08-04T14:24:45Z

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


ℹ️ [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 でオブジェクトとして渡すため実害はありませんが、両スクリプトで取得方法が異なる点は将来の混乱の元になる可能性があります。コメントで意図を明示しておくと良いでしょう。

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

audit は jq -cS — got_reviews と文字列として比較するので、両辺でキー順を揃える必要がある

apply は jq -c — --argjson でオブジェクトとして渡すだけで、比較しない。キー順は payload の意味に影響しない

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

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

github-actions · 2026-08-04T14:24:45Z

+    # 「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')


ℹ️ [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 が返ることはなく省略されるため、現在の実装で実害はありません。

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

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

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

github-actions · 2026-08-04T14:24:45Z

+      "name": "tenbin_cache",
+      "allow_auto_merge": false,
+      "enforce_admins": false,
+      "require_pull_request": false,


ℹ️ [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 または当該エントリのコメントで明示的に言及しておくと、将来の混乱を防げます。

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

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 として出ます。

fix(scripts): declare the pull-request requirement and cover the elix…

9943129

…ir repos

github-actions Bot reviewed Aug 4, 2026

View reviewed changes

docs(config): state that only the declared review fields are managed

ba6127f

github-actions Bot reviewed Aug 4, 2026

View reviewed changes

fix(scripts): stop when review_settings is missing from the declaration

d1544c4

github-actions Bot reviewed Aug 4, 2026

View reviewed changes

toshi0806 merged commit 0931a25 into main Aug 4, 2026
7 checks passed

toshi0806 deleted the declare-reviews-and-elixir-repos branch August 4, 2026 16:43

toshi0806 mentioned this pull request Aug 4, 2026

feat(config): require a pull request on every declared repository #139

Merged

Uh oh!

Conversation

toshi0806 commented Aug 4, 2026

背景

変更内容

1. require_pull_request を宣言に加える

2. elixir 系 5 リポジトリを宣言に追加

検証

補足

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

toshi0806 Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

1. `require_pull_request` を宣言に加える