Repository navigation
feat: I226 I225 のレビューの残り: 失効の確かめの取り違え・鍵を出す書き方の検査の狭さ - #418
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
計画書レビュー [I226] ── [2026-11-06 22:00]
判定: ✅ 完了
レビュー指摘一覧
| 重大度 | 観点 | 指摘内容 | 該当箇所 | 対応 |
|---|---|---|---|---|
| Warning | 外部挙動依存の未実証 | M-02 は「このワークツリーの版のフックが効く」を前提にしている。メモリには、許可ルールが元のリポジトリの設定で判定されるとある。フックの実行パスが元のリポジトリの版を指すと、M-02 は実装が正しくても不合格になる。この前提は稼働中のセッションで確かめられるのに、事前の確認がない。 | 手動テスト M-02 備考、計画 §4-5 | 実装ステップ 4 の前に、フックのコマンドが $CLAUDE_PROJECT_DIR のどのパスを指すかを確かめて記録する。 |
| Warning | P4 テスト | 既存の G9 d)(FAKE_NET_DOWN)は、変更後は最初の読みで失敗して DELETE 前に終わる。これまでの「DELETE の後の読みが通信失敗」の分岐は、どのケースでも検証されなくなる。V2(DELETE を送らず exit 1)は d) で足りる。 |
計画 §4-3 G9、V2 | DELETE 後の読みだけを通信失敗にするケースを G9 に足す。または意図的に省くと明記する。 |
| Warning | P4 テスト | TC-04 の grep -q G7 は、既存の記述に G7 が含まれていても通る。F4 の変更がなくても通る可能性がある。 |
自動テスト TC-04 | 追記する文の固有語(例: G7 が止める)で検査する。 |
| Info | 設計 | revoke_one の戻り値は 0/1/3 で、token_state の 0/1/2 とは意味が違う。呼び出し側で取り違えやすい。コメントは書かれている。 |
計画 §4-1 | 任意。名前付きの定数にすると読みやすい。 |
| Info | 設計 | G10 は gh auth token を --json の有無に関係なく不合格にする。この決定は妥当で、--show-token の言及は落とさない方針とも整合している。 |
計画 §4-3 | 対応不要。 |
その他の観点の確認結果:
- 要件適合性(P1): 問題なし。F5 は見送りどおり含まれておらず、仕様の追加もない。エッジケース(空の控え・通信失敗・期限切れの控え)も扱われている。
- セキュリティ: 問題なし。鍵は
GH_TOKENの環境変数で渡し、G9 g・h で出力に鍵の値がないことを確かめる。依存パッケージの追加はない。 - P3(データ整合性)、P9(プライバシー): 対象外。
- P5(運用性): 問題なし。フックは失敗しても黙って通し、
revoke.shは確かめられないとき失敗にする。入れ直しとロールバックの手順も書かれている。 - P8(コスト・保守負荷): omission-lint の判定本体は
code-review.shの 1 か所のままにしている。pre-commit に載せない理由(CI の--all-filesで古い I034 が落ちる)は実走した件数で示されており、妥当。 - コマンド衛生・破壊的取り消し: 問題なし。
- 規約是正の網羅性:
CLAUDE.mdの説明の 1 文も影響範囲に入っている。 - Claude Code ベストプラクティス(指示ファイル): 対象外。変更するのは runbook・CLAUDE.md の 1 文・スクリプトで、スキルは変えない。
- 必ず確かめる項目の節: 表はあり、観点は 1〜3、確かめ方は
実験必須/読んで可のみで、書式は適合している。 - 手動テスト: 3 件とも実施者は Claude で、期待結果は文言と exit code まで具体的に書かれている。
高リスク判定
判定: No
該当条件: なし(pretooluse_guard.py・.github/*・.pre-commit-config.yaml・.claude/settings*.json は変更しない。手元のハーネスだけの変更なので、harness 固有の規定により Yes にしない)
推奨: 実装(implement 段階)へ。
VERDICT: OK
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uto_test の宣言漏れを書いた直後に知らせる Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
決定論ゲート実行結果(code-review.sh 自動注入)
-
bash scripts/claude/tests/test_i223_gh_app.sh→ exit=0 ✅ -
bash scripts/claude/tests/test_posttooluse_check.sh→ exit=0 ✅ -
bash scripts/claude/tests/test_review_gates.sh→ exit=0 ✅ -
grep -q strict-mcp-config docs/runbooks/plan-writing-rules.md→ exit=0 ✅ -
grep -q G6・G7 docs/runbooks/plan-writing-rules.md→ exit=0 ✅ -
bash -n scripts/claude/gh-app/revoke.sh→ exit=0 ✅ -
bash -n scripts/claude/gh-app/gh→ exit=0 ✅ -
bash -n scripts/claude/code-review.sh→ exit=0 ✅ -
python3 -m py_compile scripts/claude/hooks/posttooluse_check.py→ exit=0 ✅ -
高リスク判定: NO(計画レビュー=NO / path=NO / 台帳の目印=NO)
-
台帳(full・1 周目): OK(code-reviewer の VERDICT=OK・合否に使わない)
-
omission-lint: OK
-
FINAL VERDICT(gate/omission/LLM 合成): OK
コードレビュー [I226] ── [2026-11-06 22:30]
受け入れ条件の照合
| # | 受け入れ条件 | 実装状況 | 備考 |
|---|---|---|---|
| 1 | revoke.sh が、もともと無効な鍵を「失効を確認」と表示しない(--stdin は exit 1、控えは控えを消して成功扱い) |
✅ 実装済み | token_state で失効の前に読み、401 なら rc=3 を返す。G9 g・h・i と FG があり、test_i223_gh_app.sh は exit=0 |
| 2 | gh auth token を書くと G10 で落ちる。行末の \ で分けた --json 付きは落ちない |
✅ 実装済み | g10_fg を 2 つ足してある。ただし、直後に和文が続く書き方を見落とす穴がある(F1) |
| 3 | ログインなしの gh auth status が exit 1 になる |
✅ 実装済み | sys.exit(0 if ok and n else 1) に変えた。G8 に空の hosts のケースと FG を足した |
| 4 | plan-writing-rules.md に --strict-mcp-config・tmux の PATH・G7 の記述がある |
✅ 実装済み | grep -q 2 本が exit 0 |
| 5 | omission-lint が push の前にも流れる | ✅ 実装済み | --lint-auto-test の入口を足し、PostToolUse のフックから呼ぶ。TC-10g〜i を足した。test_posttooluse_check.sh は exit=0 |
| 6 | 既存のハーネステスト・CI がすべて pass する | ✅ 実装済み | 決定論ゲート 9 本がすべて exit 0 |
決定論ゲートは、注入された実結果(上の各コマンドと exit code)を根拠にした。
台帳
FINDING: F1 SEV=Medium STATUS=open FILE=scripts/claude/tests/test_i223_gh_app.sh LINE=139 SECURITY=yes STOP=- | G10 の正規表現に足した \b が、gh auth status や gh auth token の直後に和文が続く書き方を見落とす。Python の \w は和文も含むので語の境目にならない。変更前の gh auth status(\b なし)はこの形でも止めていたので後退にあたる | 再現: 手順書に「gh auth tokenを打つ」と、バッククォートも空白も挟まずに書く → re.finditer(r"gh auth (status|token)\b") に当たらず、G10 が合格になる
FINDING: F2 SEV=Low STATUS=open FILE=docs/runbooks/plan-writing-rules.md LINE=90 SECURITY=no STOP=- | 同じ括弧の中に「付け忘れは G6 が止める」と「付け忘れは G6・G7 が止める」が重複して書かれている。次に直すときに片方だけ更新されてずれやすい | 再現: なし
FINDING: F3 SEV=Low STATUS=open FILE=scripts/claude/tests/test_i223_gh_app.sh LINE=141 SECURITY=no STOP=- | 行末の \ のつなぎが LF だけを見ている。CRLF の文書では続きの行が切り離される | 再現: gh auth status \ を CRLF で書き、次の行に --json hosts を置く → 正しい書き方なのに G10 が不合格にする
FINDING: F4 SEV=Low STATUS=open FILE=scripts/claude/hooks/posttooluse_check.py LINE=59 SECURITY=no STOP=- | code-review.sh の返り値が 1 以外(2 や途中で落ちた場合)だと黙って通すので、検査が壊れても気づけない。意図した設計(流せないときは review-gate が最後に見る)の範囲内 | 再現: なし
FINDING: F5 SEV=Low STATUS=open FILE=CLAUDE.md LINE=56 SECURITY=no STOP=- | CLAUDE.md の §3 に、フックの新しい検査の説明を 1 文足している。ルールを増やさないという方針とは緊張するが、既存の 1 行の説明を合わせただけで、新しい決まりは足していない | 再現: なし
(次の段階は review-gate 段階がスクリプトの判定で決める)
REVIEWED_COMMIT: cce1536
RISK: NO
VERDICT: OK
台帳(累積・スクリプトが書く)
LEDGER: F1 SEV=Medium STATUS=open SCOPE=delta KIND=harness STOP=- REPRO=yes RISK=- SECURITY=yes FILE=scripts/claude/tests/test_i223_gh_app.sh | G10 の正規表現に足した \b が、gh auth status や gh auth token の直後に和文が続く書き方を見落とす。Python の \w は和文も含むので語の境目にならない。変更前の gh auth status(\b なし)はこの形でも止めていたので後退にあたる
LEDGER: F2 SEV=Low STATUS=open SCOPE=delta KIND=harness STOP=- REPRO=no RISK=- SECURITY=no FILE=docs/runbooks/plan-writing-rules.md | 同じ括弧の中に「付け忘れは G6 が止める」と「付け忘れは G6・G7 が止める」が重複して書かれている。次に直すときに片方だけ更新されてずれやすい
LEDGER: F3 SEV=Low STATUS=open SCOPE=delta KIND=harness STOP=- REPRO=yes RISK=- SECURITY=no FILE=scripts/claude/tests/test_i223_gh_app.sh | 行末の \ のつなぎが LF だけを見ている。CRLF の文書では続きの行が切り離される
LEDGER: F4 SEV=Low STATUS=open SCOPE=delta KIND=harness STOP=- REPRO=no RISK=- SECURITY=no FILE=scripts/claude/hooks/posttooluse_check.py | code-review.sh の返り値が 1 以外(2 や途中で落ちた場合)だと黙って通すので、検査が壊れても気づけない。意図した設計(流せないときは review-gate が最後に見る)の範囲内
LEDGER: F5 SEV=Low STATUS=open SCOPE=delta KIND=harness STOP=- REPRO=no RISK=- SECURITY=no FILE=CLAUDE.md | CLAUDE.md の §3 に、フックの新しい検査の説明を 1 文足している。ルールを増やさないという方針とは緊張するが、既存の 1 行の説明を合わせただけで、新しい決まりは足していない
LEDGER_ROUND: 1
止める指摘(スクリプトが決めた・fix-loop が直すのはこれだけ)
(再現は本文の同じ番号の FINDING 行)
組み込みのレビューの出力(公式・参考)
組み込み /code-review の指摘
Permission allow rule "Read()" adds nothing to Read's default, which already runs without asking in the working directories. "Read" alone allows every path. Older versions of Claude Code treat "Read()" as "Read", and this version will too if Anthropic turns this change off.
Permission allow rule "Grep()" adds nothing to Grep's default, which already runs without asking in the working directories. "Grep" alone allows every path. Older versions of Claude Code treat "Grep()" as "Grep", and this version will too if Anthropic turns this change off.
Permission allow rule "Glob()" adds nothing to Glob's default, which already runs without asking in the working directories. "Glob" alone allows every path. Older versions of Claude Code treat "Glob()" as "Glob", and this version will too if Anthropic turns this change off.
[
{
"file": "scripts/claude/tests/test_i223_gh_app.sh",
"line": 139,
"summary": "G10 の正規表現に `\\b` が足されたため、`gh auth status` や `gh auth token` のすぐ後ろに和文が続く書き方を見落とす。Python の str 正規表現では和文の文字も \\w なので、そこに語の境目ができない。",
"failure_scenario": "手順書に「gh auth statusで名義を確かめる」や「gh auth tokenを打つ」のように、バッククォートも空白も挟まず和文を続けて書くと、`re.finditer(r\"gh auth (status|token)\\b\")` に当たらない。G10 は合格になり、鍵を画面に出す書き方が残る。変更前の `gh auth status`(`\\b` なし)はこの形でも止めていたので、これは後退にあたる。"
},
{
"file": "scripts/claude/tests/test_i223_gh_app.sh",
"line": 141,
"summary": "行末の `\\` で続く行をつなぐ処理が LF の改行しか見ていない。CRLF のファイル(/mnt/c 上で Windows の改行で保存されたもの)では、続きの行が切り離される。",
"failure_scenario": "`gh auth status \\` の改行が \\r\\n で、次の行が `--json hosts` の文書を考える。`(?<!\\\\)\\n` の直前は \\r なので最初の行で区切られ、`replace(\"\\\\\\n\", \" \")` も当たらない。has_json が False になり、正しい書き方なのに G10 が不合格にする。"
},
{
"file": "scripts/claude/gh-app/revoke.sh",
"line": 49,
"summary": "`--stdin` で渡した鍵が、すでに失効させ済み(例: 直前に控えの revoke.sh で失効させた同じ鍵)でも exit 1 になる。成功した後始末が失敗と同じ終了コードになる。",
"failure_scenario": "手順書の 1. で控えを失効させた後、念のため同じ鍵を `revoke.sh --stdin` に貼る。事前の確かめで 401 になり rc=3 となって exit 1 で終わる。文言では「対応は不要」と言っているのに、終了コードで成否を見るスクリプトや人は「失効できていない」と受け取り、流し直しや報告をする。終了コード 1 の意味(確かめられない)と重なっている。"
},
{
"file": "scripts/claude/gh-app/gh",
"line": 35,
"summary": "ログインしていない(hosts が空)とき、入口は標準出力に何も出さず exit 1 で終わる。「案内の文は本物の gh が出す」という前提が、--json を付けた呼び出しで成り立つかを確かめていない。",
"failure_scenario": "ログインなしの環境で `gh auth status` を打つと、本物の `gh auth status --json hosts` は `{\"hosts\":{}}` を返す。このとき標準エラーに何も出さないと、利用者に見えるのは入口の「--json の形で表示しました」の 1 行だけになる。何も表示されずに失敗し、ログインしていないことに気づけない。テストの g8empty も標準出力が空であることしか見ていない。"
},
{
"file": "scripts/claude/hooks/posttooluse_check.py",
"line": 21,
"summary": "AUTO_TEST_RE が `docs/tests/open/I<数字>_auto_test.md` だけを対象にしているので、ひな形(docs/tests/templates/auto_test_template.md)の見出しの書き違いは書いた直後に知らせられない。",
"failure_scenario": "ひな形の「## 決定論ゲート(自動実走)」の見出しを誰かが 1 語でも書き換えると、フックは黙って通る。その後に作る auto_test は新しいひな形の見出しを写すので、すべて omission_lint に当たる。結果として review-gate で毎回 HIGH になる(変更の根元は見張られていない)。"
},
{
"file": "scripts/claude/hooks/posttooluse_check.py",
"line": 59,
"summary": "returncode が 1 以外(使い方の誤りの 2、code-review.sh の途中で set -e により落ちたときなど)を黙って通すので、検査が壊れていても気づけない。",
"failure_scenario": "将来 code-review.sh の 901 行目より前の最上位のコードが失敗するように変わる(例: 先頭で引数や git を前提にする処理が足される)と、フックは毎回 exit 0 で通る。宣言漏れの即時通知は消えるが、テスト TC-10g が CI で回らない限り誰も気づかない。"
},
{
"file": "scripts/claude/code-review.sh",
"line": 421,
"summary": "根本の原因は omission_lint が見出しの文言の完全一致(`^## 決定論ゲート(自動実走)`)に頼っていることで、今回のフックはその脆い判定を書いた時点でも呼ぶようにしただけになっている。",
"failure_scenario": "「自動実行」と「自動実走」のような 1 語違いの見出しでは、ゲートは宣言されないまま HIGH になる。見出しの判定を `^## 決定論ゲート` の前方一致にして、表記が違えば注意だけ出す形にすれば、通知の仕組みを足さずに同じ事故を根元で防げる。"
},
{
"file": "docs/runbooks/plan-writing-rules.md",
"line": 90,
"summary": "同じ括弧の中に「付け忘れは test_i223_gh_app.sh の G6 が止める」と「付け忘れは test_i223_gh_app.sh の G6・G7 が止める」が重なって書かれている。",
"failure_scenario": "1 文の中で同じ趣旨の決まりが 2 回、範囲を変えて書かれている。読む人にはどちらが正しいのか分かりにくく、次に直すときにも片方だけ更新されてずれやすい。"
},
{
"file": "CLAUDE.md",
"line": 56,
"summary": "CLAUDE.md の「この CLAUDE.md にルールを増やさないこと」に対して、§3 の行にフックの新しい検査の説明を書き足している。",
"failure_scenario": "CLAUDE.md §0 に「詳細ルールは以下へ集約します(この CLAUDE.md にルールを増やさないこと)」とある。変更後の行は「hooks(PostToolUse)で Edit/Write 直後の .py/.json/.yaml 構文と、自動テスト文書の決定論ゲートの宣言漏れを検証する」で、検査が増えるたびに CLAUDE.md が伸びる形になっている。詳細は docs/runbooks/pre-commit.md などの参照先に書くべき。"
}
]組み込み /security-review の指摘
(実行していない: パス判定 NO)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
概要
I225 のレビューで残った指摘を直した。
revoke.shが、もともと無効な鍵(末尾の欠け・打ち間違い・失効済み)を「失効を確認しました」と取り違えないよう、失効の前にもその鍵が有効かを確かめるgh auth tokenを見落とし、行末の\で分けた--json付きの書き方を誤って落としていたのを直すgh auth statusが exit 0 になる件(F2)・plan-writing-rules.mdの記述漏れ(F4)・自動テスト文書の決定論ゲートの宣言漏れを、書いた直後に PostToolUse のフックで知らせる関連イシュー
Closes #417
変更点
scripts/claude/gh-app/revoke.sh:token_state(有効・401・確かめられない)を足し、失効の前後で同じ判定を使う。--stdinで最初から無効なら「最初から無効」と出して exit 1・DELETE は送らない。控えがもともと無効(期限切れなど)なら、そう出して控えを消し成功扱いscripts/claude/gh-app/gh: 置き換えたauth statusは、アカウントが 0 件のとき exit 1(案内の文は本物のghが標準エラーに出す)scripts/claude/code-review.sh:--lint-auto-test <ファイル>…(既存のomission_lintを呼ぶだけの入口)scripts/claude/hooks/posttooluse_check.py:docs/tests/open/I<数字>_auto_test.mdの Edit/Write の直後に上の入口を呼び、宣言漏れなら exit 2 で Claude に知らせるtest_i223_gh_app.sh(G8 の空の hosts・G9 の g/h/i・G10 のauth tokenと行のつなぎ・それぞれの失敗の注入)・test_posttooluse_check.sh(TC-10g〜i)docs/runbooks/github-app-setup.md(失効の新しい表示)・docs/runbooks/plan-writing-rules.md(F4)・CLAUDE.md(PostToolUse の説明の 1 文)・docs/reviews/backlog/I225.md(F2・F4 の行を消した)・docs/reviews/backlog/I226.md(今回の台帳の Low)テスト確認
docs/tests/closed/I226_manual_test.md。M-01〜M-03 をすべて Claude が実施・OK)ロールバック手順
bash scripts/claude/gh-app/install.shをもう一度流す参照ドキュメント
docs/plans/closed/plan_I226.mddocs/reviews/closed/I226_review.md・docs/reviews/closed/I226_code_review_20261106_2139.mddocs/tests/closed/I226_auto_test.md・docs/tests/closed/I226_manual_test.md🤖 Generated with Claude Code