diff --git a/.takt/facets/instructions/review-simplicity.md b/.takt/facets/instructions/review-simplicity.md index 528cbc19..21d85ae7 100644 --- a/.takt/facets/instructions/review-simplicity.md +++ b/.takt/facets/instructions/review-simplicity.md @@ -22,7 +22,7 @@ Read the diff straight through. Note any pattern that prompted "this looks unusu - **Unexplained complexity**: Logic choices with no obvious motivation given the surrounding code; algorithm complexity that seems disproportionate to the problem - **Inconsistent style**: Naming or structural patterns that diverge from neighboring code without rationale -- **Dead-on-arrival code**: Branches, parameters, or abstractions with no apparent caller or use site +- **Dead-on-arrival code**: Branches, parameters, or abstractions with no apparent caller or use site (before flagging, check "PR chain declarations" below -- a declared successor PR is a legitimate caller-to-be) - **Hidden coupling**: Changes that silently depend on global state, environment, ordering, or undocumented invariants - **Missing failure paths**: Operations that can fail (I/O, parse, network, optional unwrap) with no visible error handling - **Non-obvious magic values**: Numeric or string literals whose meaning isn't clear from context @@ -38,10 +38,22 @@ Review primarily within the changed diff. **Limited** cross-file lookups are per The DRY and YAGNI dimensions in anomaly detection apply **only to executable code logic**. - **DRY scope**: Flag duplicated *code logic* (copy-paste functions, repeated control flow, redundant computations). Do NOT flag duplication that is documentation, doc-vs-code restatement, or test independence. -- **YAGNI scope**: Flag *speculative code abstractions* (unused parameters, premature interfaces, over-engineered patterns in production code). Do NOT flag planning-document "future candidates" / "Phase 2 検討" / ADR rejected-alternative sections, or comments documenting known constraints. +- **YAGNI scope**: Flag *speculative code abstractions* (unused parameters, premature interfaces, over-engineered patterns in production code). Do NOT flag planning-document "future candidates" / "Phase 2 検討" / ADR rejected-alternative sections, or comments documenting known constraints. For abstractions whose consumer is a **declared successor PR**, see "PR chain declarations" below. If a finding cannot be tied to executable code logic, it is out of scope. See [ADR-035: docs-only PR 評価ポリシー](../../../docs/adr/adr-035-doc-evaluation-policy.md) for the full list of criteria that do NOT apply to docs-only diffs (mutation / error handling / test coverage / function length / DRY / YAGNI all fall under this). +## PR chain declarations (ADR-069) + +Multi-PR chains (the PR size gate forces features >1500 lines to split) necessarily produce **leading PRs that introduce infrastructure whose consumer arrives in a successor PR**. Treating every missing consumer as blocking would structurally reject every chain's leading PR, so chain-declared items get a narrower treatment: + +- **When the declaration is valid**, missing-consumer findings (dead-on-arrival code, premature interfaces / abstractions) against the **declared items** are **non-blocking warnings**, not REJECT grounds. Still record them in Warnings so the chain's tail stays auditable (if the successor never lands, the next reviewer sees the trail). +- **A valid declaration** must satisfy all of: + 1. It lives in a **planning document inside this diff** (e.g. the plan doc / a `docs/todoN.md` entry updated in the same PR). Planning documents outside the diff do NOT qualify, even when module docs reference them -- an out-of-diff document was not reviewed as part of this change, so it can be stale or self-servingly pre-written, and accepting it would let an unreviewed file relax this review. + 2. It names the **successor PR and the specific pairing**: which new crate / exe / function will be consumed by which planned change. Generic "will be used later" does not qualify. + 3. The declared names **match the code**: the crate / exe / function names in the declaration must equal those in the diff. A declaration that contradicts the diff (the 2026-08-02 case: the in-diff plan doc described wiring to a *different* exe than the one the comments promised) does NOT downgrade -- flag it as blocking, citing the contradiction. +- **Fail-closed**: no declaration found, pairing not specific, or names mismatch → the finding stays blocking as usual. This section narrows nothing for undeclared speculation. +- **Fix Suggestion ordering (ADR-068 carry-over)**: when you do raise a finding that admits multiple remedies, list the **least destructive remedy first** (e.g. "correct the declaration/comment" before "defer/remove the abstraction"). The fix step follows suggestion order, and a most-destructive-first ordering caused a gut-revert incident (2026-08-02). + ## Calibration: avoid over-narrowing The shift to anomaly detection is meant to remove the duplicative checklist work, not to skip review. If reading the diff leaves you with a concrete unease that you can articulate, raise it — even if it doesn't fit a named criterion. Conversely, if you can only flag something by mechanically applying a rule, the deterministic layer already handles that case. diff --git a/CLAUDE.md b/CLAUDE.md index 573a19e3..ec51f411 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,10 +68,11 @@ - [ADR-065: CI matrix による移植退行防止 — 両 OS で同一スイートを回す](docs/adr/adr-065-ci-matrix-cross-os-regression.md) *(試験運用)* - [ADR-066: 自律実行の全体 kill-switch — 正極性単一フラグと「欠損 → 安全状態」原則](docs/adr/adr-066-autonomy-global-kill-switch.md) *(試験運用)* - [ADR-068: pre-push fix step の権限境界 — 後退検知 backstop と設計級 remedy の human routing](docs/adr/adr-068-fix-step-authority-boundary.md) *(試験運用)* +- [ADR-069: PR chain 宣言規約 — 分割チェーンと missing-consumer 検査の両立](docs/adr/adr-069-pr-chain-declaration.md) *(試験運用)* ## 開発 convention / チェックリスト -- [開発 convention / チェックリスト](docs/dev-conventions.md) — spike 見送り (negative result) 永続化 convention (順位261)、外部 SaaS 無料枠 / 制限の調査チェックリスト (順位262)、外部 fixture 参照テストは値まで assert (順位274) +- [開発 convention / チェックリスト](docs/dev-conventions.md) — spike 見送り (negative result) 永続化 convention (順位261)、外部 SaaS 無料枠 / 制限の調査チェックリスト (順位262)、外部 fixture 参照テストは値まで assert (順位274)、PR chain の分割と宣言 (ADR-069) ## Build diff --git a/docs/adr/adr-069-pr-chain-declaration.md b/docs/adr/adr-069-pr-chain-declaration.md new file mode 100644 index 00000000..80c1a191 --- /dev/null +++ b/docs/adr/adr-069-pr-chain-declaration.md @@ -0,0 +1,92 @@ +# ADR-069: PR chain 宣言規約 — 分割チェーンと missing-consumer 検査の両立 + +## ステータス + +試験運用 (2026-08-03) + +> 本 ADR は [ADR-056](adr-056-review-policy-anomaly-shadow.md)（pre-push review の anomaly policy 層）の精緻化と、PR 分割の切断点規約を 1 枚で記録する。[ADR-050](adr-050-iteration-aware-decision-criteria.md)（decision criteria の scope 明示）と同じ「判定基準に文脈を与える」パターンの適用である。 + +## コンテキスト + +### 3 つのゲートの合成デッドロック + +2026-08-02 の WP-17 PR 2a incident（[ADR-068](adr-068-fix-step-authority-boundary.md) コンテキスト参照）の根本原因は、個別には正しい 3 つの機構が合成で矛盾することだった: + +1. **PR size gate** は 1500 行超の PR を block し、分割を強制する（warning 800 / block 1500）。 +2. **Multi-PR chaining 規約**（git-workflow.md、1 PR 250〜800 行推奨）はチェーン分割を推奨する。 +3. **simplicity review の missing-consumer 検査**（dead-on-arrival / premature abstraction）は「呼び手のいない抽象」を blocking とする。 + +内部レイヤリング（lib 抽出 + 呼び手、exe + 配線）を持つ大型機能では、**チェーンの先頭 PR は必ず「消費者がまだ存在しない何か」を導入する**。3 つを同時に満たす分割は存在せず、どう切っても先頭 PR が REJECT される。incident では、この REJECT への fix が PR を空洞化させた（fix 側の対策は ADR-068）。 + +### incident で実際に起きた宣言の欠落 + +PR 2a の diff 内の計画書は「workflow を既存 exe（`cli-autonomy-gate`）へ接続する」と読める記述のままで、code コメントが約束する新 exe（`cli-fix-push-gate`）をどこも名指ししていなかった。レビュアーは計画書まで突き合わせた上で「存在しないインフラを事実として主張している」と正しく判定した。**欠けていたのはレビュアーの寛容さではなく、チェーンの宣言**である。 + +## 決定 (試験運用) + +### 1. PR chain 宣言規約 + +複数 PR に分割するチェーンでは、**先頭（および中間）PR の diff 自身に**、次を満たす宣言を含める: + +| 要件 | 内容 | +|---|---| +| 置き場所 | **diff 内の計画文書のみ**（plan doc / `docs/todoN.md` エントリを同一 PR で更新する）。diff 外の計画文書は module doc が参照していても不可 — この PR でレビューされていない文書は stale や自己都合の事前記述でありえ、未レビューのファイルにレビューを緩和させることになる | +| 具体性 | 後続 PR と**抽出↔呼び手のペアリング**を具体名で書く（どの crate / exe / 関数を、どの後続変更が消費するか）。「将来使う」だけの宣言は無効 | +| 名前一致 | 宣言中の名前は diff 内の実名と一致していること。矛盾する宣言は降格根拠にならない（incident の形） | + +分割時は「各 PR の diff 内文書がその PR の真実を語っているか」を再検証する。分割操作は diff の境界だけでなく**文書とコードの整合の境界**も動かす。 + +### 2. chain-aware review 降格 + +simplicity review は、上記の**有効な宣言がある項目に限り**、missing-consumer findings（dead-on-arrival code / premature interfaces・abstractions）を REJECT 根拠ではなく **non-blocking warning に降格**する。 + +- Warnings への記録は残す。後続 PR が land しないままチェーンが放置された場合、次のレビューが監査痕跡として辿れる。 +- **fail-closed**: 宣言なし / ペアリング非具体 / 名前不一致 → 従来どおり blocking。未宣言の投機的抽象への検査は一切緩まない。 +- 実装は `review-simplicity.md`（pre-push の blocking 経路）のみ。whole-tree variant（weekly review）は push を block しないため対象外。 + +### 3. 切断点ヒューリスティクス + +PR size gate に当たった際の分割判断: + +1. **抽出と最初の呼び手の間で切らない**。ADR-044 層 1 の正当化（呼び手の存在）が diff から消え、本 ADR の宣言でしか救えなくなる。呼び手と同じ PR に抽出を入れれば宣言は不要で、判定も自明になる。 +2. 切断点は**関心の境界**（機能 vs 配線、実装 vs docs バッチ）に置く。 +3. **良い関節が無ければ `PR_SIZE_CHECK_OVERRIDE=1` + 理由の明記が正当**。size gate の override は「大型 refactoring 等で意図的な場合」のために存在する正規の経路であり、悪い関節で切った分割はチェーン全体のコスト（レビュー回数・宣言管理・矛盾リスク）で上限超過 1 回分を上回り得る。incident の初回分割（1613 行 = block の 8% 超過を 2 分割し、抽出と呼び手を分離）はこの判断を誤った実例。 + +### 4. fix suggestion の記述規約（ADR-068 残課題の引き取り） + +複数の remedy がありうる finding では、**最も破壊的でない処置を Fix Suggestion の先頭に書く**。fix step は先頭候補に従う傾向があり、incident では最破壊処置（"defer the extraction" = revert）が先頭だったことが gut-revert の一因になった。処置の実行側の防御は ADR-068（最小処置原則 + backstop）が持ち、本規約は発生源側の手当てである。 + +## 試験運用判断基準 + +instruction / 規約層のみの変更のため config opt-in は無い（kill-switch は instruction の revert）。次を観測して本採用 / 改訂を判断する: + +- **decision trigger**: 宣言付き chain PR が 3〜5 本流れた時点で、(a) 有効な宣言を持つ先頭 PR が missing-consumer で REJECT されないこと、(b) 未宣言の投機的抽象が引き続き REJECT されること、(c) 名前不一致が blocking のままであること、(d) 宣言が欠落・非具体（「将来使う」レベル）のケースが blocking のままであること、を確認する（(b)〜(d) で fail-closed 3 条件の全てを検証対象にする）。 +- **期限**: 2026-11-03 までに判定材料が集まらなければ、chain 分割の発生頻度に照らして延長 / 却下を決める。 +- 直近の検証機会: WP-17 の再分割チェーン（2a / 2b / 2c）が最初の宣言付き chain になる。 + +## 帰結 + +### 利点 + +- size gate・chaining 規約・missing-consumer 検査が両立可能になり、「分割したら REJECT される」構造矛盾が解消する。 +- 宣言は要求が具体的（ペアリング + 名前一致）なため、「後で使うつもり」と書くだけの逃げ道にはならない。 +- レビュアーが実際に diff 内計画書を読むことは incident で実証済みで、宣言の検証コストは既存のレビュー動作に乗る。 + +### 欠点 / 留意点 + +- 宣言の維持コスト: チェーン構成が変わったら宣言も更新が要る。stale な宣言は名前不一致で fail-closed に倒れる（安全側だが手戻り）。 +- 降格は LLM instruction 層であり決定論的ではない。降格の誤適用（無効な宣言を有効と誤読）が起きた場合、**投機的抽象が blocking レビューを受けずに land するリスクは残る** — [ADR-068](adr-068-fix-step-authority-boundary.md) backstop が守るのは「fix step による PR の後退」だけ、quality gate が守るのは「ビルド・テストの成立」だけで、どちらも未消費抽象の設計妥当性は検証しない。残る防御は Warnings 記録の監査痕跡（後続 PR が land しないまま放置されたチェーンを次のレビューが辿れる）に限られる。 +- 中間 PR（呼び手はあるが自分も次への供給を含む）は宣言を両方向に書く必要がある。 + +### 残課題 + +- 宣言の機械検証（宣言中の名前が diff の実名と一致するかの決定論チェック）は未実装。降格の誤適用が観測されたら検討する。 + +## 関連 + +- [ADR-068](adr-068-fix-step-authority-boundary.md) — fix step の権限境界。incident の fix 側対策。fix suggestion 記述規約（決定 4）の残課題元 +- [ADR-056](adr-056-review-policy-anomaly-shadow.md) — pre-push review policy 層。本 ADR はその anomaly 判定に文脈（chain 宣言）を与える +- [ADR-050](adr-050-iteration-aware-decision-criteria.md) — decision criteria の scope 明示パターンの先行例 +- [ADR-044](adr-044-subprocess-utility-extraction-boundary.md) — lib 抽出の境界基準。切断点ヒューリスティクス 1 の根拠 +- [ADR-035](adr-035-doc-evaluation-policy.md) — docs-only 判定。YAGNI 検査の scope 境界の先行定義 +- 開発 convention: [dev-conventions.md](../dev-conventions.md) § PR chain の分割と宣言 diff --git a/docs/dev-conventions.md b/docs/dev-conventions.md index 8f99682d..9ab58bec 100644 --- a/docs/dev-conventions.md +++ b/docs/dev-conventions.md @@ -65,3 +65,14 @@ integration test で外部バイナリを spawn する場合、**無期限 wait 3. **lint ではなく convention** — fixture ごとにスキーマが異なり regex での自動検知は非現実的なため、機械 lint 化せず convention として運用する (ADR-042 の役割分担)。 **由来** (PR #261 T3-#2、[ADR-041](adr/adr-041-test-isolation-patterns.md)): `hooks-stop-tool-call-leak` の E2E (`tests/e2e.rs`) が実 config を隣にコピーする際、`[stop_tool_call_leak]` section の存在しか assert しておらず、`enabled = true` / `max_consecutive_blocks = 3` の値変更が cap 境界テスト (`consecutive_leaks_at_cap_fail_open` 等) を原因の見えない形で silent break させるリスクを CodeRabbit / session / pre-push simplicity の 3 ソースが独立指摘した。順位 273 で実例側 (値まで assert) を修正し、本 convention でパターンを一般化した。 + +## PR chain の分割と宣言 (ADR-069) + +PR size gate (block 1500 行) に当たって PR を分割する場合の規約 (詳細は [ADR-069](adr/adr-069-pr-chain-declaration.md)): + +1. **抽出と最初の呼び手の間で切らない** — ADR-044 層 1 の正当化 (呼び手の存在) が diff から消え、simplicity review の missing-consumer 検査 (dead-on-arrival / premature abstraction) に構造的に REJECT される。切断点は関心の境界 (機能 vs 配線、実装 vs docs バッチ) に置く。 +2. **良い関節が無ければ `PR_SIZE_CHECK_OVERRIDE=1` + 理由の明記が正当** — 悪い関節で切った分割はチェーン全体のコスト (レビュー回数・宣言管理・矛盾リスク) で上限超過 1 回分を上回り得る。 +3. **チェーンの先頭 / 中間 PR は diff 内の計画文書で宣言する** — 後続 PR と抽出↔呼び手のペアリングを具体名で書く (「将来使う」は無効)。宣言済み項目への missing-consumer findings は non-blocking warning へ降格される。宣言なし / 名前不一致は従来どおり blocking (fail-closed)。 +4. **分割後は各 PR の diff 内文書がその PR の真実を語っているか再検証する** — 分割は diff の境界だけでなく文書とコードの整合の境界も動かす。 + +**由来** (2026-08-02 WP-17 PR 2a incident、[ADR-068](adr/adr-068-fix-step-authority-boundary.md) / [ADR-069](adr/adr-069-pr-chain-declaration.md)): size gate 強制の 2 分割が抽出 (lib 2 crate) と呼び手 (cli-fix-push-gate) を分離し、宣言の無い先頭 PR が simplicity REJECT → fix の gut-revert → gate 全 PASS のまま空洞化 push という連鎖が発生した。 diff --git a/docs/todo-summary2.md b/docs/todo-summary2.md index 15a6bf29..d82d386b 100644 --- a/docs/todo-summary2.md +++ b/docs/todo-summary2.md @@ -119,6 +119,7 @@ | 361 | 🔧 Tier 2 | **JJ_VERSION の ci.yml / cloud-setup.sh 一致検証テスト (#342 post-merge feedback 採用)** | todo14.md | S | なし (ADR-051 型の 2 ファイル結合「上げるときは必ず揃える」を機械検証へ。順位 360 と同一 PR 可) | | 362 | 🔧 Tier 2 | **git subprocess のブランチ名依存引数を検出する custom lint rule (#343 post-merge feedback 採用)** | todo14.md | S | なし (detached HEAD で CI 観測が silent pending 化した PR #343 incident 由来。ADR-049 fixture 3 点セット整備込み。順位 363 と同一 PR 可) | | 363 | 🔧 Tier 2 | **check-ci-coderabbit の detached HEAD 回帰統合テスト (#343 post-merge feedback 採用)** | todo14.md | M | なし (修正済み silent pending バグの regression test が皆無。順位 362 と同一 PR 可) | +| 364 | 🔧 Tier 2 | **ADR-054 scope guard の pre-push 展開 — fix diff の allowlist 照合 (ADR-068 残課題)** | todo14.md | M | WP-17 再分割 PR (lib-scope-guard land) 後が効率的 (ADR-068 の後退検知は削除系のみの暫定。追加系 injection と backstop 自己弱体化の経路を閉じる) | **戦略**: Tier 1 を 2〜3 セッションで片付け → Tier 2 で ADR-032 の前提 + rate-limit + convergence cost 削減を進める → Tier 3 で ADR-032 を land + ドキュメント整備。Tier 4-5 は cleanup / 外部展開で daily efficiency への直接効果は小さい。 diff --git a/docs/todo14.md b/docs/todo14.md index 3d765758..51790103 100644 --- a/docs/todo14.md +++ b/docs/todo14.md @@ -790,3 +790,28 @@ #### 完了基準 - detached HEAD 環境で CI 状態が pending に固着する回帰が、テスト失敗として land 前に検出されること。 + +--- + +### ADR-054 scope guard の pre-push 展開 — fix diff の allowlist 照合 (ADR-068 残課題) + +> **動機**: pre-push の takt fix step には「finding 由来 allowlist との fix diff 照合」の決定論層が無く、instruction (fix.md の scope allowlist) 頼み。2026-08-02 の WP-17 PR 2a incident (fix が finding 対象外の lib crate 2 つを丸ごと削除し gate 全 PASS で push) で顕在化した。[ADR-068](adr/adr-068-fix-step-authority-boundary.md) の後退検知 backstop は削除系 (ファイル脱落 / 追加行削減) のみ検知する 80/20 の暫定で、**追加系の injection (finding 対象外ファイルへの書き込み・config 書き換え) は検知できない**。PR #348 security review の non-blocking 注記 (fix step が push-runner-config.toml の `max_added_line_shrink_pct` / `enabled` を書き換えて backstop 自体を自己弱体化できる経路が instruction 頼み) もこれで閉じる。 +> +> **対処案**: `cli-push-runner` の post_takt_regate 段 (または直前の専用 stage) で、`.takt/runs/` の最新 findings レポートから `Location` 列を抽出して allowlist を導出し、takt 前後の diff 差分の変更ファイルを照合する。判定コアは `lib-scope-guard` (WP-17 再分割 PR で land 予定) を再利用し、cli-pr-monitor の post-pr 経路と判定の同一性を保つ (ADR-054 の drift 防止)。violation は ADR-068 の `[FIX_REGRESSION]` と同様の loud block + 独立 kill-switch。findings レポートのパース失敗は fail-closed。 +> +> **参照**: [ADR-068](adr/adr-068-fix-step-authority-boundary.md) § 決定 3 / 残課題、[ADR-054](adr/adr-054-prompt-injection-trust-boundary-defense.md) § 欠点 (pre-push 展開の予告元)、`src/cli-pr-monitor/src/stages/scope_guard.rs` (post-pr 側の先行実装)、PR #348 security review 注記。依存: WP-17 再分割 PR (lib-scope-guard の land) 後が効率的。 +> +> **実行優先度**: 🔧 Tier 2 — Severity High (injection 防御の穴) / Frequency Low (fix 発生時のみ) / Effort M / Adoption Risk Low (既存 stage への追加、kill-switch つき)。 + +#### 作業計画 + +- [ ] findings レポート (.takt/runs/ 最新 run) から Location 列を抽出する parser (fail-closed) +- [ ] lib-scope-guard で allowlist 照合、violation は loud block + 独立 kill-switch +- [ ] incident 再現テスト: (a) finding 対象外ファイルへの変更が**変更種別 3 種 (追加 = 新規ファイル作成 / 書き換え = 既存ファイル編集、config 自己弱体化含む / 削除) のいずれでも** block されること、(b) `ALWAYS_ALLOWED` 対象ファイル (`.takt/review-diff.txt` 等) への変更は finding allowlist 外でも block されないこと、の両方を固定する (完了基準の「追加・書き換え・削除いずれも」に対応) +- [ ] fix.md / fix-supervisor.md の「pre-push は後退検知のみ」記述を更新 +- [ ] 本エントリ削除 + todo-summary2.md 行削除 + +#### 完了基準 + +- fix step が finding 対象外ファイルを変更 (追加・書き換え・削除いずれも) した push が、決定論的に block されること。ADR-068 の後退検知では通ってしまう「追加系 injection」ケースがテストで固定されていること。 +- `ALWAYS_ALLOWED` (post-pr 側の先行実装 `src/cli-pr-monitor/src/stages/scope_guard.rs` で定義済み、現状 `.takt/review-diff.txt` のみ) は、fix step が本 instruction (fix.md 「Pre-completion diff refresh」) に従って正当に書き換える中間ファイルの例外リストである。pre-push 側の実装も finding allowlist に加えてこのリストを常に許可し、post-pr 側と同一のリストを共有すること (ADR-054 の drift 防止)。この例外により (a) の block 判定が誤って `.takt/review-diff.txt` 自体の正当な refresh まで block しないことをテストで固定する。