Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions docs/adr/adr-027-push-review-simplicity-focus.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ push 時点で見たかったのは「コードのシンプルさ」であり、

「architectural 妥当性 (cross-file, ADR 準拠, 命名規約)」→「コードのシンプルさ (diff 局所)」に責務を狭める。後者は diff だけで完結するため、reviewer が Grep/Read で探索する必要がなくなる。

### Amendment (2026-07-21): 「diff 局所」は *観点* の限定であって *コミット範囲* の限定ではない

本 ADR が狭めたのは **reviewer が使う criteria** (cross-file 探索を要求しない) であり、**レビュー対象に含めるコミットの範囲**ではない。この 2 つが混同され、`push-runner-config.toml` の `[diff] command` が `jj diff -r @` (tip コミットのみ) のまま運用された結果、祖先コミットが pre-push レビューを一度も経ずに merge される状態が生まれた (todo 順位 288、Severity High で PR #268/#300/#301/#311 と 4 回再発)。

レビュー対象は **PR 範囲 (`<base>..@`) 全体**とする。根拠:

- **速度は理由にならない (実測)**: 同一 PR に対しレビュー対象を 37 行 → 1011 行 (27 倍) に広げても所要時間は 4m32s → 4m43s の **+11 秒**。本 ADR の速度改善は arch-review facet の除去 (219-270s/iter) によるもので、diff 範囲の縮小は寄与していない。
- **検知不能性**: レビュアーは渡された diff が PR 全体かを検証できないため、範囲が狭いことによる見落としは誰にも検知されない (実際 security-review が実 diff と矛盾して "docs-only / No dependency changes" と報告した)。
- **CodeRabbit backstop では代替できない**: 第三者レビューは有用だが、セルフレビューを省いてよい理由にはならない。両者は独立した層として併用する。

実装は `Config::resolve_base_branch` に範囲解決を一本化し、`[diff] command` は `{{PR_RANGE}}` プレースホルダ経由で範囲を受け取る (config に revset を直書きできない)。加えて生成 diff が PR 範囲を網羅しているかを fail-closed で機械検査する。

### simplicity-review の criteria (diff 局所で完結)

- ネスト深さ (>4 レベルで要改善)
Expand Down
3 changes: 1 addition & 2 deletions docs/todo-summary2.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
| 255 | 💎 Tier 3 | **ADR-040 の実測値を新 GPU (RTX PRO 5000 48GB) で再 calibration (ADR-046 WP-01 スパイクで陳腐化を観測)** | todo15.md | S | なし (ADR-038/040 が前提とする RTX 3070 8GB は RTX PRO 5000 Blackwell 48GB に更新済み。27-31B Q4 モデルが 100% GPU で動き VRAM が制約でなくなったため、ADR-040 の VRAM/latency trade-off 表と「VRAM scarcity → model swap 制約」framing が陳腐化。ADR-046 で mistral:7b / gemma4 / qwen3-coder の VRAM・latency を実測済 → ADR-040 amendment に反映、num_ctx 選定 flow の memory 軸を latency 軸へ再重み付け) |
| 256 | ⏳ Tier 5 | **classifier FP 検出強化プロンプトで格上げ候補を再評価 (WP-04 見送りの follow-up、ADR-038 amendment 由来)** | todo15.md | M | なし (WP-04 実測で全候補が FP 検出未達 = 能力限界か `classify.txt` の mistral 向け tune 不適合かが未分離。FP 検出強化プロンプト版で qwen3-coder:30b 等を再測し、能力限界と確認できれば恒久見送り、プロンプト不適合なら該当モデル + 専用プロンプトで格上げ。eval 手法・gold セットは scratchpad WP-04 資産を再利用。materially better な新モデル出現時も再評価トリガー) |
| 257 | ⏳ Tier 5 | **push pipeline の `cargo test` を cargo-nextest 化 (WP-05 で Stop hook には無効と判明、push 側 follow-up)** | todo15.md | S-M | なし (WP-05 実測: Stop hook は cargo test 不在で nextest 非適用、真因は逐次実行→並列化で解決済。ただし push pipeline (cli-push-runner quality_gate) の `cargo test -- --ignored` は実測 ~80s で nextest 高速化の余地あり。ツール依存追加 = ADR-017 pinning + 派生プロジェクト配布のコスト、push が Stop より低頻度な点を踏まえた費用対効果を評価。doctest は nextest 非実行のため `cargo test --doc` 併走が必要) |
| 264 | 🔧 Tier 2 | **pre-push review-diff.txt の生成形式を `jj diff --git` に切替 — LLM レビュアーの add/delete 誤読解消 (PR #256 post-merge-feedback Tier1 #1 採用)** | todo15.md | S | なし (`push-runner-config.toml:113` の `[diff] command = "jj diff -r @"` は色+行番号2列形式で、色を落とした review-diff.txt では削除が `-` マーカー無しになり LLM レビュアーが「追加」と誤読。PR #256 で todo 25行削除を simplicity-review が false positive REJECT し ~19分浪費。`jj diff --git -r @` へ切替で解消、`templates/push-runner-config.toml:52` も同 PR で修正必須 (deploy:hooks 配布)、Adoption Risk None、memory `prepush-review-diff-plain-format-misread.md`) |
| 272 | 🚀 Tier 1 | **cli-docs-lint に ADR 重複採番 + CLAUDE.md 索引整合チェック追加 (PR #261 post-merge-feedback T1-#2 採用)** | todo15.md | S | なし (PR #261 で ADR-052/053 採番衝突が実発生、既存 cli-docs-lint の check-mode 骨格流用。順位 135 placeholder policy は todo entry 側の「ルール」で本 entry は land 済ファイルの「仕組み」検知、相補で重複ではない) |
| 275 | 🔧 Tier 2 | **層別テストテンプレート (StubOllama パターン・integration 独立性) の共有化 (PR #265 post-merge-feedback T2-1 採用)** | todo15.md | M | なし (WP-11/ADR-054 の多層防御実装で「空 StubOllama による LLM 未呼び出し証明」「tempdir+jj init+CwdRestore の integration 独立性」を都度設計。WP-17 の classifier/scope guard 拡張で同種判断が再発見込み。shared crate 化の境界は ADR-044 で判定、WP-17 着手前の実施が効果的) |
| 276 | 💎 Tier 3 | **ADR-007 に「コメント配置の意思決定フロー」を追加 (PR #265 post-merge-feedback T3-2 採用)** | todo15.md | S | なし (PR #265 で非 doc コメントの Bundle Z block が 2 回発生 = doc コメント/識別子名/マーカー付き Why の配置判断が未文書化。linter 自動化は NLP 必要で却下済み、既存 Q1-Q3 形式で人間/AI の判断補助を doc 化。バッチ PR で消化可) |
Expand All @@ -52,7 +51,7 @@
| 285 | 🔧 Tier 2 | **jj keyword を含む commit message の tokenization edge-case テスト (PR #267 post-merge-feedback T2-2 採用)** | todo15.md | S | なし (順位 283 と表裏。283 の着手有無に関わらず現行挙動を regression test で固定する価値が独立して残る。283 と同一 PR 消化が効率的) |
| 286 | 🔧 Tier 2 | **config path 解決の cwd 跨ぎ integration test (PR #267 post-merge-feedback T2-3 採用)** | todo15.md | M | なし (FIXED 済 cwd-config bug の regression guard。既存テストは pure parser のみで file-lookup 経路未カバー。Severity High、Adoption Risk = OS 依存) |
| 287 | 💎 Tier 3 | **「config 読み hook は exe-relative 解決必須」convention の明文化 (PR #267 post-merge-feedback T3-1 採用)** | todo15.md | XS | なし (順位 281 の文書層補完。**281 と同一 PR bundle 推奨**、別作業に切り出す価値は低い) |
| 288 | 🚀 Tier 1 | **pre-push review が PR 全体をカバーしない — post-merge の全 run 集約 + push-runner `[diff]` stage の tip-only 範囲修正 (PR #268/#300/#301 feedback 採用、Severity High 3連続再発)** | todo15.md | M | なし (根因は `[diff]` stage `jj diff -r @` が tip のみ = 祖先 code が AI レビュー未経由で merge。`docs_only_routing` は PR 範囲へ修正済だが `[diff]` は非対称。単一 push では全 run 集約でも救えず [diff] 範囲拡張 + bookmark_check 祖先検証が根治。ADR-027 射程はユーザー判断。独立 PR 推奨) |
| 288 | 🚀 Tier 1 | **pre-push review が PR 全体をカバーしない — post-merge の全 run 集約 + push-runner `[diff]` stage の tip-only 範囲修正 (PR #268/#300/#301 feedback 採用、Severity High 3連続再発)** | todo15.md | M | なし (【一部実装済 2026-07-21】`[diff]` stage の PR 範囲化 + 範囲カバレッジ検査 (fail-closed) + top-level default_branch 一本化 + `--git` 切替 (順位 264 同時完了) を実施。ADR-027 射程は実測 (37行 4m32s → 1011行 4m43s = +11秒) を根拠に範囲拡張を採用。**残**: post-merge feedback の全 run 集約と bookmark_check.rs の祖先未レビュー穴の検証) |
| 292 | 🔧 Tier 2 | **cli-pr-monitor の lock.rs を token 方式の所有権検証へ統一** | todo15.md | S-M | なし (PR #271 で pipeline_lock.rs に導入した token ベース所有権検証と同型の Drop 無条件削除バグが cli-pr-monitor/src/lock.rs にも残存。参照実装が既にあるため低リスク) |
| 293 | ⏳ Tier 5 | **push-runner の stack push モード (opt-in、YAGNI につき見送り継続)** | todo15.md | M | なし (stacked bookmark 運用の実績が現状なく、必要になった時点で着手する opt-in 拡張として記録のみ) |
| 294 | 💎 Tier 3 | **jj-op-verify hook の位置づけ再整理 — 並列 workspace 安全化ではなく混線緩和層として再分類** | todo15.md | S | なし (検知対象は出力混線の症状であり並列 workspace とは独立に価値を持つ。ADR-045→ADR-053 の枠組みへ紐付け直すドキュメント再整理のみ) |
Expand Down
39 changes: 5 additions & 34 deletions docs/todo15.md
Original file line number Diff line number Diff line change
Expand Up @@ -218,39 +218,6 @@

---

### pre-push review-diff.txt の生成形式を jj diff --git に切替 — LLM レビュアーの add/delete 誤読解消 (PR #256 post-merge-feedback Tier1 #1 採用)

> **動機**: `push-runner-config.toml:113` の `[diff] command = "jj diff -r @"`(jj デフォルト形式)で生成される `.takt/review-diff.txt` は、追加/削除を色 + 行番号2列(`NNN :` = 削除 / ` NNN:` = 追加)で表現する。ファイル化で色が落ちると `-`/`+` マーカーが無くなり、削除が「左列のみ行番号」でしか区別できず、pre-push の LLM レビュアー(simplicity-review 等)が削除ブロックを「追加」と誤読しうる。`--git`(標準 unified diff)は色非依存で `+`/`-` を明示するため誤読しない。PR #256(ADR-051 起票 PR)で todo エントリ25行の**削除**を simplicity-review が「追加」と誤読し stale-tracking-entry として false positive REJECT を出し、レビュー約19分を浪費した実害が発生した。
>
> **本タスクの位置づけ**: PR #256 post-merge-feedback Tier1 #1 で採用(他6提案は over-engineering として却下)。fix ステップの「hunk-polarity bug」という診断は不正確で、真因は色を落とした平文 diff の LLM 可読性問題。
>
> **参照**: `push-runner-config.toml:113`(`command = "jj diff -r @"` → `"jj diff --git -r @"`、修正対象)、`templates/push-runner-config.toml:52`(同様の変更、`pnpm deploy:hooks` で派生プロジェクトに配布されるため**両方修正必須**)、memory `prepush-review-diff-plain-format-misread.md`、PR #256 feedback report (`.claude/feedback-reports/256.md`) Tier1 #1
>
> **実行優先度**: 🔧 Tier 2 — Effort S。false positive で約19分浪費した実害が既に発生しており、config + template 各1箇所の軽微な修正で再発を防止できる。

#### 設計決定 (案)

- `[diff] command` を `jj diff --git -r @` に変更。本番 config と template の2箇所を同一 PR で修正(template 未修正だと派生プロジェクトに同じ false positive が横展開)。
- review-diff.txt を format-sensitive に parse する `.rs` 箇所は存在せず(LLM facet が読むのみ)、Adoption Risk None。

#### 作業計画

- [ ] `push-runner-config.toml:113` を `command = "jj diff --git -r @"` に変更
- [ ] `templates/push-runner-config.toml:52` も同様に変更
- [ ] review-diff.txt を参照する箇所(facet instruction / `.rs`)が `--git` 形式で問題ないか確認
- [ ] dogfood: 削除を含む diff で pre-push review が正しく削除を認識することを確認
- [ ] 本エントリ削除 + todo-summary2.md 行削除

#### 完了基準

- pre-push review が削除ブロックを「追加」と誤読しなくなり、config + template 両方が `--git` 形式、派生プロジェクトへの横展開も解消。

#### 詰まっている箇所

- なし(変更箇所・影響範囲とも確定済み、PR #256 feedback report で cross-validation 済み)。

---

### cli-docs-lint に ADR 重複採番 + CLAUDE.md 索引整合チェック追加 (PR #261 post-merge-feedback T1-#2 採用)

> **動機**: PR #261 で当方が ADR-052 として起草した ADR が、並行 land した PR #260 の ADR-052 (自律実行境界) と採番衝突し、rebase 時にファイル名 + 本文タイトル + ソース内参照 10+ 箇所の置換が発生した実例。ADR は既に 53 件、並行 PR 開発が常態化しており再発頻度 Medium。現状この衝突を機械検知する層が存在しない (発見は rebase 時の CLAUDE.md conflict 頼み)。
Expand Down Expand Up @@ -477,13 +444,17 @@
>
> **再発・優先度見直し (2026-07-19、#300/#301 feedback)**: PR-N2 (#300) / PR-N3 (#301) の feedback で同型 gap が Severity High で再発。根因は本エントリの集約範囲より上流の **push-runner `[diff]` stage** (`push-runner-config.toml` の `[diff] command = "jj diff -r @"`) が **tip コミットのみ**を AI レビュー用 diff に書き出す点。複数コミットを 1 回の `pnpm push` でまとめて送ると、tip 以外の祖先コミット (例 #300 の `resolve_main_workspace_root()` 実装、#301 の `Cargo.toml`/`main.rs` 変更) が local security/simplicity レビューを一度も経ずに merge される (security-review.md が実 diff と矛盾して "docs-only / No dependency changes" と記載)。**単一 push では pre-push run 自体が 1 回のみ**のため、本エントリの「全 run 集約」だけでは救えない。よって本エントリの実装時に、(a) `[diff]` stage の diff 範囲を `docs_only_routing` と同様に `<default_branch>..@` (PR 範囲) へ拡張し、(b) `bookmark_check.rs` の `@` 非 trunk 祖先が未レビューのまま push される穴 (T8 / PR #280 と同クラス) の検証を併せて行う。`docs_only_routing.rs` の skip 判定は既に `<default_branch>..@` に修正済みだが `[diff]` stage 自体は未修正で非対称。ADR-027 (push-time review を diff-local に限定し範囲外は CodeRabbit backstop) の trade-off 射程が security-review にも及ぶかはユーザー判断待ち。
>
> **(a) 実装済 (2026-07-21)**: PR #311 で 4 回目の再発 (695 行の PR に対しレビュー対象 37 行、security-review が実 diff と矛盾して "docs-only / No dependency changes" と記載) を観測し、`[diff]` stage を修正した。top-level `default_branch` を新設して `diff` / `docs_only_routing` / `pr_size_check` の 3 stage が同一解決を共有し、`[diff] command` は `{{PR_RANGE}}` プレースホルダ経由で範囲を受け取る (config に revset を直書きできない)。加えて生成 diff が PR 範囲の全変更ファイルを含むかを `jj diff --summary` と突き合わせる**範囲カバレッジ検査**を fail-closed で追加し、config の書き方に依存せず「レビュー範囲 < PR 範囲」を検知できるようにした (未更新の派生プロジェクト config も捕まる)。順位 264 (`--git` 形式切替) も同 PR で同時実施 (範囲検査が `diff --git` ヘッダを読む要件と重なるため)。**ADR-027 の射程についてのユーザー判断**: 範囲拡張のレビュー時間コストを実測したところ 37 行 4m32s → 1011 行 4m43s (**+11 秒**) で、ADR-027 の速度改善は arch-review facet 除去によるものであり diff 範囲縮小は寄与していないことが判明したため、範囲拡張を採用した。
>
> **残タスク**: 本エントリ本体の「post-merge feedback の全 run 集約」と、上記 (b) `bookmark_check.rs` の祖先未レビュー穴の検証は未着手。
Comment on lines +447 to +449

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

実装済みの作業計画を同期してください。

ここでは [diff] stage の PR 範囲化を「実装済み」としていますが、同じエントリ内の作業計画・完了基準には、なお未完了の作業として残っています。該当チェック項目を完了扱いに更新し、残タスクだけを記載してください。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/todo15.md` around lines 447 - 449, docs/todo15.md の該当エントリで、[diff] stage
の PR
範囲化に関する作業計画と完了基準を実装済みとして更新してください。既存の実装済み記述と整合させ、完了したチェック項目を未完了一覧から削除し、残タスクとして
post-merge feedback の全 run 集約と bookmark_check.rs の祖先未レビュー穴の検証だけを残してください。

>
> **参照**: `.claude/feedback-reports/268.md` Tier 2 #1 / `.claude/feedback-reports/300.md` Tier1 #1 / `.claude/feedback-reports/301.md` Tier1 #1、`src/cli-merge-pipeline/src/feedback/context.rs` (`find_latest_prepush_reports_dir`)、`push-runner-config.toml` (`[diff]` section)、`src/cli-push-runner/src/stages/diff.rs`・`src/cli-push-runner/src/stages/bookmark_check.rs`、`src/cli-push-runner/src/config/docs_only_routing.rs` (既に PR 範囲へ修正済の対照)、`.takt/facets/instructions/analyze-prepush-reports.md`、[ADR-027](adr/adr-027-push-review-simplicity-focus.md)
>
> **実行優先度**: 🚀 Tier 1 — Severity High (review gate の silent 覆域縮小が 3 PR 連続で再発) / Frequency Medium (複数コミットを 1 push する運用で恒常発生) / Effort M。context スキーマ変更 + facet 更新 + `[diff]` stage 修正 + テストを伴うため独立 PR 推奨 (旧 Tier 2 から昇格)。

#### 作業計画

- [ ] **`[diff]` stage の diff 範囲を `<default_branch>..@` (PR 範囲) に拡張** — 祖先コミットの code 変更も AI レビュー用 diff に含める (`docs_only_routing` の skip 判定と同基準に揃える)。
- [x] **`[diff]` stage の diff 範囲を `<default_branch>..@` (PR 範囲) に拡張** — 祖先コミットの code 変更も AI レビュー用 diff に含める (`docs_only_routing` の skip 判定と同基準に揃える)。**PR #311/#313 で実装済** (上記 (a) 参照。範囲カバレッジ検査 + config-load 時の `{{PR_RANGE}}` 必須検証込み)。
- [ ] `bookmark_check.rs` で `@` 非 trunk 祖先が未レビューのまま push される穴を検証・塞ぐ (T8 / PR #280 と同クラス)。
- [ ] 対象 PR の pre-push run dir を列挙する関数に拡張。時刻範囲のみでの絞り込みは対象外 run の混入・対象 run の欠落を招くため、対象 PR のコミット範囲や関連 bookmark 名など複数の識別根拠を突き合わせて対象 run を判定すること (`.takt/runs/*-pre-push-review`)
- [ ] context json の `prepush_reports_dir` を配列化 + facet instruction を複数 dir 対応に (スキーマ契約変更のため: 全 reader の列挙 + 旧 string 形式との後方互換 or schema versioning + 空配列時の挙動を明記)
Expand Down
Loading