Skip to content

feat: I212 差分のレビューの起点を最後に合格した記録にする(失敗した回の後にセキュリティレビューを飛ばさない) - #421

Draft
osushi0404-claude[bot] wants to merge 1 commit into
developfrom
feature/I212-review-base-last-passing
Draft

osushi0404-claude[bot] wants to merge 1 commit into
developfrom
feature/I212-review-base-last-passing

Conversation

@osushi0404-claude

Copy link
Copy Markdown
Contributor

概要

I212: 差分のレビューが、組み込みの /security-review が動かなかった回の記録を起点にして、セキュリティレビューを飛ばして OK を出すのを止める

関連イシュー

Closes #419

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@osushi0404-claude osushi0404-claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

計画書レビュー [I212] ── [2026-11-08 12:00]

判定: ✅ 完了

レビュー指摘一覧

重大度 観点 指摘内容 該当箇所 対応
Warning P4 REVIEW_COMPLETE: YES は、SECURITY_VERDICT と REVIEWER_VERDICT と REVIEW_OK から決める。ところが /code-review(OFFICIAL_CODE)が失敗した回は見終えた扱いになる。参考としてレビュー役に渡すだけなので意図どおりだが、計画書には「OFFICIAL_CODE の失敗は YES の条件に含めない」と一言も書かれていない。 §4-1、§9(--append-system-prompt は /security-review だけ) 実装時に §4-1 へ 1 行足す。
Warning P4 review_mode の第 2 引数の既定が origin/develop になる。既存の test_i197_review_ledger.sh の R1 などは引数 1 個で呼ぶ。テスト用リポジトリに origin/develop が無いと、merge-base が失敗して base が空になり、full に落ちる。R1 の「見終えた記録が起点」の例は YES の記録があるので動く。ただし、見終えた記録が無い既存ケースがあれば、結果が full へ変わる恐れがある。 §4-2、§4-6 ステップ 1 で、既存の R 系テストが full へ変わらないことを確認し、必要ならテスト側で第 2 引数 develop を渡す。
Warning P4 official_security_verdict は、/security-review の出力の「最後の空でない行」だけを見る。モデルが SECURITY_FINDINGS: の行の後に締めの文を付ければ HIGH(FAILED)になる。review-gate の手順で 1 回やり直せるが、同梱 2・4 の再発経路が残る。スパイクは 1 回だけ。 §4-3、§8 M-01 に加えて、実装後の実走(M-02 の基本レビュー)で連続して出るかを観察する。外れが続くなら、行頭一致で最後に出た SECURITY_FINDINGS: を読む形へ緩めるかを検討する。
Info BP extra=() の空配列を "${extra[@]}" で展開している。bash 4.3 以前で set -u を使うと未定義エラーになる。この環境は bash 5 系なので実害はない。 §4-3 ${extra[@]+"${extra[@]}"} にすれば安全。任意。
Info P4 欄の無い古い記録を「見終えていない」扱いにするので、進行中のイシューで古い記録が残っていると見直す範囲が広がる。計画書は「進行中は無い」としている。 §4-1、§8 対応は任意。

観点別の確認

  • P1 要件適合性: 問題なし。範囲を「見終えていない回」に狭めたのは、ユーザーの判断(2026-11-08)に沿っている。台帳と周の数は、台帳のある最新の記録から引き継ぐ。全体か差分かの決め方(rebase 後は全体)は変えない。同梱 3・5 は別イシューに切り出す理由が書かれている。
  • ベストプラクティス: 問題なし。判定の本体は record_complete と official_security_verdict の 1 か所に集めている。指示の文は定数化している。消費箇所(review_mode の呼び出し、--route、SKILL.md の 14 行目と 36 行目、review-rules.md の 59 行目、gate-check.sh)は列挙されている。neutralize_machine_lines と REVIEW_CLEAN の sed への追加も書かれている。
    • コマンド衛生と破壊的取り消し: 問題なし。
    • 外部挙動(--append-system-prompt): スパイクで実証済みで、結果が計画書に記録されている。
  • セキュリティ: 認証・認可・テナントは対象外(アプリのコードを変えない)。偽の行による起点の改ざんは、本文中の行を無効化して対策している。わざと書き換えるケースは I179 の方針どおり対象外。依存の追加はない。
  • P4 テスト: 問題なし。
    • FG(失敗の注入)が各 TC に付いている。
    • 実装前に、直す前の版で落ちることを確かめる手順がある。
    • 合否は exit 0 基準。
    • 文言の有無を見る過剰な決定論テストは作らない方針も明記されている。
    • M-01 と M-02 は Claude が実施し、実行方法が具体的。
    • 「必ず確かめる項目」の節と表がある。確かめ方はすべて 実験必須、観点は 1〜3 の範囲内。
  • P3: 対象外(DB・マイグレーションなし)。
  • P5 運用性: ロールバックは revert で足りる。名義と鍵の変更は無い。GH_APP_READONLY=1 は維持する。
  • P8 コスト・保守負荷: 新しい安全装置としては「見終えたか」の行を足すだけで、文字列でシェルを読む精密な判定ではない。欠けたときは広く見直す側(fail-closed)に倒す。問題なし。
  • P9 プライバシー: 対象外。
  • Claude Code ベストプラクティス: SKILL.md の説明文の変更だけ。分岐の追加はなく、概念変更の消費箇所は §4-8 に列挙されている。問題なし。

高リスク判定

判定: No
該当条件: なし

  • 変更対象は scripts/claude/code-review.sh、そのテスト、SKILL.md、runbook、イシュー文書。
  • harness 固有の高リスク一覧(.github/*・.pre-commit-config.yaml・.secrets.baseline・pretooluse_guard.py・.claude/settings*.json)には当たらない。
  • アプリ側の条件を理由に Yes にはしない。

推奨: 実装(implement 段階)へ。

VERDICT: OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

0 participants