diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index e6a230c..b2419a0 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,7 @@ { "name": "ndf", "source": "./plugins/ndf-claude", - "description": "Claude Code plugin (v8.5.4): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications." + "description": "Claude Code plugin (v8.6.0): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications." }, { "name": "playwright-kit", diff --git a/AGENTS.md b/AGENTS.md index 422ac2a..b47b849 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,7 +77,7 @@ ai-plugins/ ## NDFプラグインについて -**NDFプラグイン**は、このマーケットプレイスの主要プラグインです（v8.5.4）。plugin 名は全ランタイムで `ndf` を維持し、配布物は `plugins/ndf-claude` / `plugins/ndf-codex` / `plugins/ndf-kiro` に分離しています。 +**NDFプラグイン**は、このマーケットプレイスの主要プラグインです（v8.6.0）。plugin 名は全ランタイムで `ndf` を維持し、配布物は `plugins/ndf-claude` / `plugins/ndf-codex` / `plugins/ndf-kiro` に分離しています。 - 共通編集元は `plugins/ndf-shared/` - Claude Code版は 8個の専門サブエージェント、公開Skills、SessionStart/Stopフックを提供 - Codex版は Codex向け公開Skillsと任意Slack通知hookを提供 diff --git a/CLAUDE.md b/CLAUDE.md index 6e62b1d..9cb4406 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +29,7 @@ skills/ → 実行可能なワークフロー 詳細は `docs/specifications/ndf-knowledge-and-kiro.md` を参照。 -## NDF v8.5.4 の Skill 構成 +## NDF v8.6.0 の Skill 構成 Skill は 31 個で、配布は `plugins/ndf-shared/manifests/` が唯一の基準（Claude Code 27 / Codex 25 / Kiro 26）。ブラウザ自動テストの 4 個は `playwright-kit` プラグインへ分離した（`plugins/playwright-kit-shared/`）。frontmatter の書き方は `plugins/ndf-shared/skills/README.md` の規約に従い、`python3 scripts/check-skill-frontmatter.py` で検査する。利用実績と維持・統合・削除の判定は `docs/specifications/ndf-skill-inventory.md` に記録する。 @@ -57,6 +57,8 @@ v8.5.3 で `cross-refactoring` の投稿の確認を入れた。投稿は AI 自 v8.5.4 で `cross-refactoring` の差分予算を手法別にした。新しい定義を作って呼び出し側を書き換える手法は、抽出した本体に加えて呼び出し側の書き換え・import の追加・引数の受け渡しが固定費として乗る。実測で予算超過として落ちた 4 件はいずれも `long_method` の抽出で、見積の 2.03〜2.31 倍だった。範囲の逸脱ではなく、倍率 2 の予算をわずかに超えただけである。抽出系の 7 手法だけ倍率を 3 にした（範囲外を触った実測例は見積の 4 倍なので取り逃がさない）。あわせて提案プロンプトの見積の指示へ固定費と現状固定テストを数えることを明記し、`init` が kiro の既定 `auto` を検知して「集計から分離される」ことを着手前に知らせるようにした。詳細は `plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md`。 +v8.6.0 で `cross-refactoring` のコミット粒度を 1 改善項目 = 1 コミットに変えた。手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。手ごとにテストを回すのは変わらないが、残すのは項目単位の 1 コミットだけにする（現状固定テストが要る項目のみ 2 コミット）。適用と修正の両方で検証するのは、適用側だけ揃えても指摘への対応という名目で刻んだ履歴が戻ってくるためである。テストの回数も項目の単位に合わせた。進行側が申告されたコミットごとに実行するため、実装担当にも手ごとの実行を義務づけると同じテストが手数の 2 倍だけ走る（実測 44 手で 88 回）。あわせて改修計画（なぜ直すのか・どう直すのか）を `--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出すようにした。理由と手順は提案の時点でしか残らず、状態ファイルは差分から除外されるため Pull Request からは読めなかった。公開は生成物の同期と同じコミットに乗せる。詳細は `plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md`。 + v6.0.0 の対応表（`review` → `pr-review`）は予告どおり削除済み。v6.0.0 以前から移行する場合は v6.1.0 の `ndf-policies` を参照する。 ## cross-refactoring @@ -74,6 +76,8 @@ v6.0.0 の対応表（`review` → `pr-review`）は予告どおり削除済み - 収束しない改善項目は **項目単位で取り消す**。合意済みの項目は PR に残る。ただし同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのラウンドは全件取り消しへ退避する - 生成物・配布物の同期は **進行側の責務**。実装担当にはさせない（範囲外の変更になる）。同期の手順は `--sync-command "bash scripts/build-runtime-plugins.sh"` のように渡す - 公開するのは **進行側だけ**。実装担当は push しない。進行側が検証を通した後に push するので、未検証の変更が公開されない +- 履歴に残るのは **1 改善項目 = 1 コミット**。現状固定テストが要る項目だけ 2 コミット。テストも項目の単位で 1 回だけ求める +- 改修計画は `--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出され、生成物の同期と同じコミットで公開される - `init` が参加 CLI の認証状態を確認する。誤検知するときは `NDF_SKIP_AUTH_CHECK=1` ## cross-review diff --git a/README.md b/README.md index 77f83f9..7a307b1 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ Claude Code / Codex / Kiro CLI向けのスキル・MCP設定を共有するた このマーケットプレイスは、チーム全体でAI開発ツール（Claude Code / Codex / Kiro CLI）の導入を加速するための事前設定されたプラグインを提供します。 -**NDFプラグイン v8.5.4** は、同じ `ndf@ai-plugins` という名前で Claude Code / Codex / Kiro CLI へ配布されるランタイム別プラグインです。共通ソースは `plugins/ndf-shared/` に集約し、利用者が install する配布物は `plugins/ndf-claude/` / `plugins/ndf-codex/` / `plugins/ndf-kiro/` に分かれています。 +**NDFプラグイン v8.6.0** は、同じ `ndf@ai-plugins` という名前で Claude Code / Codex / Kiro CLI へ配布されるランタイム別プラグインです。共通ソースは `plugins/ndf-shared/` に集約し、利用者が install する配布物は `plugins/ndf-claude/` / `plugins/ndf-codex/` / `plugins/ndf-kiro/` に分かれています。 - **公開Skills**: Claude Code向け core 27個、Kiro向け core 26個、Codex向け core 25個に分離。 - **元Skills（30個）**: @@ -102,9 +102,15 @@ kiro-cli chat --agent ndf | プラグイン名 | バージョン | 説明 | 詳細 | |------------|----------|------|------| -| **ndf** | 8.5.4 | Claude Code / Codex / Kiro CLI 向けに runtime 別配布物を提供する NDF プラグイン。8個の専門エージェント（Claude版）、公開Skills（Claude Code向け core 27個、Kiro向け core 26個、Codex向け core 25個）、Claude SessionStart/Stopフック、Codex/Kiro向け通知・実行補助を提供。v4.0.0 で Codex MCP サーバを廃止し、`/ndf:external-ai` skill + `corder` エージェント経由の CLI 直接実行に一本化。 | [Claude](./plugins/ndf-claude/README.md) / [Codex](./plugins/ndf-codex/README.md) / [Kiro](./plugins/ndf-kiro/README.md) | +| **ndf** | 8.6.0 | Claude Code / Codex / Kiro CLI 向けに runtime 別配布物を提供する NDF プラグイン。8個の専門エージェント（Claude版）、公開Skills（Claude Code向け core 27個、Kiro向け core 26個、Codex向け core 25個）、Claude SessionStart/Stopフック、Codex/Kiro向け通知・実行補助を提供。v4.0.0 で Codex MCP サーバを廃止し、`/ndf:external-ai` skill + `corder` エージェント経由の CLI 直接実行に一本化。 | [Claude](./plugins/ndf-claude/README.md) / [Codex](./plugins/ndf-codex/README.md) / [Kiro](./plugins/ndf-kiro/README.md) | | **playwright-kit** | 1.0.0 | Playwright による E2E テストの計画・実装・証跡管理を提供するプラグイン。ページ役割からのテスト計画、動画 / trace 付きスクリプト実装、レポート生成と Drive 保管、playwright_kit ランタイム（init、a11y / CWV スキャン）の 4 Skill。NDF v7.0.0 で分離。 | [Claude](./plugins/playwright-kit-claude/README.md) | +### NDF v8.6.0 の主な変更 + +- **1 改善項目 = 1 コミットにする**: 手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。残すのは項目単位の 1 コミットだけにした（現状固定テストが要る項目のみ 2 コミット）。適用と修正の両方で検証する +- **テストの回数を項目の単位に合わせる**: 進行側が申告されたコミットごとに実行するため、実装担当にも手ごとの実行を義務づけると同じテストが手数の 2 倍だけ走る（実測 44 手で 88 回、約 38 分）。求めるのはコミットの前に通っていることだけにした +- **改修計画を差分の中へ残す**: なぜ直すのか・どう直すのかは提案の時点でしか残らず、状態ファイルは差分から除外されるため Pull Request からは読めなかった。`--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出し、生成物の同期と同じコミットで公開する + ### NDF v8.5.4 の主な変更 - **抽出系の手法で差分予算を超える不具合を修正**: 新しい定義を作って呼び出し側を書き換える手法は、呼び出し側の書き換え・import の追加・引数の受け渡しが固定費として乗る。実測で落ちた 4 件はいずれも見積の 2.03〜2.31 倍で、範囲の逸脱ではなかった。抽出系だけ差分予算の倍率を 3 にし、提案プロンプトの見積の指示にも固定費を数えることを明記した diff --git a/plugins/ndf-claude/.claude-plugin/plugin.json b/plugins/ndf-claude/.claude-plugin/plugin.json index f7d265b..84a9c10 100644 --- a/plugins/ndf-claude/.claude-plugin/plugin.json +++ b/plugins/ndf-claude/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ndf", - "version": "8.5.4", - "description": "Claude Code plugin (v8.5.4): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications.", + "version": "8.6.0", + "description": "Claude Code plugin (v8.6.0): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications.", "author": { "name": "takemi-ohama", "url": "https://github.com/takemi-ohama" diff --git a/plugins/ndf-claude/skills/cross-refactoring/SKILL.md b/plugins/ndf-claude/skills/cross-refactoring/SKILL.md index 52c8fe6..0c44026 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-claude/skills/cross-refactoring/SKILL.md @@ -37,6 +37,8 @@ allowed-tools: | 役割の分離 | 提案・レビューは**ホストを除く 3 者**、適用は**gemini を除く 3 者**。両者は重なるが一致しない | | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | +| コミットの単位 | **1 改善項目 = 1 コミット。** テストも項目の単位で 1 回だけ求める（現状固定テストが要る項目のみ 2 コミット） | +| 改修計画 | **差分の中へ残す。** 理由と手順は提案の時点でしか残らない。公開の直前に進行側が書き出し、生成物の同期と同じコミットへ入れる | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | | 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | | 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | @@ -61,6 +63,7 @@ allowed-tools: | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | | `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | +| `--plan-file PATH` | 改修計画の書き出し先（**対象リポジトリからの相対パス**）。空文字を渡すと記録しない | `issues/refactoring-plan-rf.md` | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" @@ -128,7 +131,7 @@ flowchart TD Merge --> Empty{"採用件数 = 0 ?"} Empty -->|はい| Final([提案ラウンドの繰り返しを終了]):::ok Empty -->|いいえ| Apply - Apply["Step 4: 適用（実装担当 1 CLI）
項目ごとに 1 手 1 コミット"] + Apply["Step 4: 適用（実装担当 1 CLI）
1 改善項目 = 1 コミット"] Apply --> Review["Step 5: レビュー（2 CLI 並列）
ラウンドの差分をまとめて 1 回"] Review --> Judge{"2 者とも承認 ?"} Judge -->|いいえ| Fix["Step 6: 指摘修正（実装担当）"] @@ -275,6 +278,9 @@ done | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | +| 1 項目を複数のコミットへ刻む | 改善項目と履歴が 1 対 1 で辿れなくなり、取り消しと積み直しのコミットも件数に比例して増える | +| 実装担当に手ごとのテストを義務づける | 進行側もコミットごとに回すため、テストの実行回数が手数の 2 倍になる（実測 44 手で 88 回） | +| 改修計画を状態ファイルにだけ残す | 状態ファイルは差分から除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない | | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | 投稿に失敗したまま結果ファイルを書かずに終了する | 進行側からは「レビュー担当が動かなかった」と区別が付かない。失敗したときほど `post_error` 付きの結果ファイルが要る | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | diff --git a/plugins/ndf-claude/skills/cross-refactoring/docs/01-state-and-propose.md b/plugins/ndf-claude/skills/cross-refactoring/docs/01-state-and-propose.md index fb75178..e2a06f1 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/docs/01-state-and-propose.md +++ b/plugins/ndf-claude/skills/cross-refactoring/docs/01-state-and-propose.md @@ -46,10 +46,14 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 +7. **改修計画の書き出し先の記録** — `--plan-file` を状態ファイルへ保存する。 + 既定は `issues/refactoring-plan-rf.md` で、空文字を渡すと記録しない。 + **既定で残す**のは、指定できるだけでは誰も指定しないためである。書き出しは + 初期化時ではなく、生成物の同期と同じく**push の直前**に行う +8. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） -8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +9. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない @@ -105,7 +109,7 @@ gemini は NDF の配布先ではないため「標準の配置先」を持た | `quality-gates` | 「直し終わった」と言える条件の判定 | `ndf-policies` は**配置しない**。git 運用やコミット規約といったリポジトリ運用の方針で -あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 手 1 コミット、 +あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 改善項目 = 1 コミット、 コミットトレーラー、`--force` 禁止など）はプロンプト側で明示する。 ### 守ること diff --git a/plugins/ndf-claude/skills/cross-refactoring/docs/02-apply-and-review.md b/plugins/ndf-claude/skills/cross-refactoring/docs/02-apply-and-review.md index c311658..f9bbf23 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/docs/02-apply-and-review.md +++ b/plugins/ndf-claude/skills/cross-refactoring/docs/02-apply-and-review.md @@ -39,6 +39,7 @@ | 各コミットでテストが成功している | 実際の実行 | コミットを取り出して着手前と同じテストを走らせる。**JSON の `test_status` は読まない**。`--test-timeout`（既定 900 秒）を超えたら失敗 | | テストが無い経路は先に現状固定テスト | `git show --name-only` | `test_gap` が真の項目は、先頭コミットがテストの置き場所を触っている | | 項目の分離 | git のトレーラー | 各コミットの `Item-Id` がその項目と一致する。複数の項目を 1 コミットにまとめたら失敗 | +| コミットの粒度 | 申告されたコミットの実数 | 1 改善項目 = 1 コミット。`test_gap` が真の項目だけ 2 コミット | | 差分予算 | `git show --numstat` | 実差分の合計が `estimated_diff_lines` の 2 倍（抽出系の手法は 3 倍）を超えたら失敗（範囲の逸脱） | | 対象範囲の遵守 | `git show --name-only` | 触ったファイルが全て `--scope` の中にある。1 つでも外なら失敗 | | 機能変更の混入なし | — | 機械判定は不可能。レビュー観点に委ねる | @@ -74,6 +75,59 @@ 真のときの現状固定テストを数えることを明記した。倍率だけを広げると、見積が 楽観側へ倒れたぶんまで通してしまう。 +#### 1 改善項目 = 1 コミットにする + +**手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** 手順は +順に適用してよいが、残すのは項目単位の 1 コミットだけにする。 + +刻んだままだと 3 つの読みにくさが出る。 + +| 影響 | 内容 | +| --- | --- | +| 履歴 | Pull Request を読む側が、改善項目と履歴を 1 対 1 で辿れない | +| 取り消し | 範囲を戻して積み直すコミットが、刻んだ件数に比例して増える | +| 検証 | コミットごとにテストを実行するため、実行回数も比例して増える | + +実測では採用 12 件に対して適用が 34 コミット、取り消しと積み直しで 25 コミットだった。 + +**テストの回数も項目の単位に合わせる。** 進行側は申告されたコミットごとにテストを +実行するので、実装担当にも手ごとの実行を義務づけると、同じテストが手数の 2 倍だけ +走る。実測では 44 手に対して 88 回（1 回 26 秒として約 38 分）だった。 + +| 誰が | いつ | 6 回目の実測 | 項目単位にした後 | +| --- | --- | ---: | ---: | +| 実装担当 | コミットの前に 1 回 | 44 | 14 | +| 進行側 | 申告されたコミットごと | 44 | 14 | + +実装担当が途中で確かめる分には止めない。求めるのは**コミットの前に通っていること** +だけで、そこは進行側が実際に実行して確かめる。 + +例外は現状固定テストが要る項目（`test_gap` が真）だけで、「テスト → 実装」の +2 コミットを許す。1 つに混ぜると、テストが先行したことを履歴から確かめられない。 + +プロンプトは、途中で刻みたいときの戻し方も渡す。**その項目に着手する前の HEAD**を +控えておき、最後に `git reset --soft` で 1 コミットへまとめる。控えた地点より前へ +戻すと他の項目のコミットを巻き込むため、起点は項目の着手前に固定する。 + +#### 改修計画を差分へ残す + +**なぜ直すのか（理由）とどう直すのか（手順）は、提案の時点でしか残らない。** +状態ファイルには入っているが、そのディレクトリは差分から除外されるため、 +Pull Request を読む側からは見えない。 + +そこで `--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出す。 + +- 内容は**状態から決まる**。状態が動いていなければ差分は出ないので、公開のたびに + 書き出しても余計なコミットは積まれない +- 取り消した項目も残す。同じ提案が次のラウンドで来たときの判断材料になる +- 公開は生成物の同期と**同じコミット**に乗せる。分けると進行側のコミットが + 公開のたびに 2 つずつ積まれる +- 空文字を渡すと記録しない。差分へ入れたくないリポジトリのための逃げ道である +- **絶対パスと親へ抜ける経路は受け取った時点で拒む**（終了コード 4）。進行側は利用者の + リポジトリを触るため、作業ディレクトリの外へ書き出す余地を残さない。あわせて + `./issues/plan.md` のような表記も正規化する。git が返すパスと形が違うと、公開の + コミットメッセージが取り違えられる + #### 範囲の指定は検証にも効かせる `--scope` を必須にした目的は**提案の発散と変更の肥大を防ぐ**ことなので、指定を検証へ @@ -312,7 +366,7 @@ claude が参加する構成では実際のリファクタリング 1 件で 1.4 | --- | --- | | 項目ごとに実装者が入れ替わらない | 輪番の単位がラウンドなので、重ねれば実装者は分散する | | 指摘がどの項目に対するものか曖昧になる | 指摘に改善項目 ID を**必須**とし、未知の ID と欠落は差し戻す | -| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 手 1 コミットを保ち、**取り消しは項目単位**で行う | +| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 コミットを保ち、**取り消しは項目単位**で行う | ### 判定 diff --git a/plugins/ndf-claude/skills/cross-refactoring/docs/03-review-viewpoints.md b/plugins/ndf-claude/skills/cross-refactoring/docs/03-review-viewpoints.md index d607ea9..536799f 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/docs/03-review-viewpoints.md +++ b/plugins/ndf-claude/skills/cross-refactoring/docs/03-review-viewpoints.md @@ -14,7 +14,7 @@ | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | ## 振る舞い不変を筆頭に置く理由 diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md index ec9c8b9..f42ae6c 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md @@ -27,9 +27,18 @@ $RF_ITEMS 1. `test_gap` が真なら、**先に現状固定テストを追加してコミットする**。 これは省略できません。振る舞いが変わっていないことを示す手段が無いまま 構造を変えるのは、構造改善ではなく単なる編集です -2. `plan` の手順を **1 手ずつ**適用する -3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** -4. 通ったらコミットする（**1 手 = 1 コミット**） +2. `plan` の手順を順に適用する +3. **手順を終えてから** `$RF_BASELINE_TEST` を **1 回**実行する。落ちたら原因の手を戻す +4. 通ったら、**その項目の変更をまとめて 1 コミットにする** + +**テストもコミットも項目の単位で 1 回です。** 手ごとに回すと、進行側の検証と +合わせて手数の 2 倍だけテストが走ります（実測では 44 手で 88 回）。手ごとに +確かめたいときは自分の判断で回して構いませんが、**求めているのはコミットの前に +通っていること**だけです。 + +途中で刻んでおきたいときは、**その項目に着手する前の HEAD を控えておき**、 +最後に `git reset --soft <控えた HEAD>` してから 1 回だけコミットしてください +（控えた HEAD より前へ戻すと、他の項目のコミットを巻き込みます）。 ## コミットの規約 @@ -47,6 +56,10 @@ Impl-Runtime: $RF_RUNTIME Impl-Model: $RF_MODEL ``` +- **1 改善項目 = 1 コミット。** 2 件以上に刻むと、その項目は失敗として扱われ、 + 取り消されます。例外は `test_gap` が真の項目だけで、「現状固定テスト → 実装」の + 2 コミットを許します（テストと実装を混ぜると、テストが先行したことを + 履歴から確かめられません） - **`Item-Id` は必ずその項目のものにする。** 複数の項目を 1 コミットへまとめると、 取り消し範囲が項目単位で決まらなくなり、失敗として扱われます - `Impl-Model` には**実際に使ったモデル名**を書く。分からなければ `default` diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md index bb2180a..dd3bf4b 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md @@ -24,7 +24,8 @@ $RF_ITEMS 1. `gh api` で Pull Request の**未解決レビュースレッド**を取得する 2. 各指摘について、**修正するか・しないか**を決める - - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする + - 修正する: 直してから `$RF_BASELINE_TEST` を 1 回実行し、**その改善項目の + 修正を 1 コミットにまとめる** - 修正しない: 根拠を返信する。**黙って閉じない** 3. 対応したスレッドに返信し、`resolveReviewThread` で解決する @@ -38,6 +39,11 @@ $RF_ITEMS いること**が必要です。満たさないコミットは取り込まれず、その改善項目の指摘は 解決済みになりません（修正ラウンドの上限に達すると項目ごと取り消されます）。 +**1 改善項目 = 1 コミット。** 同じ項目のコミットが 2 件以上あると、その修正 +ラウンドの範囲ごと取り消されます。適用側だけ揃えても、指摘への対応という名目で +刻んだ履歴が戻ってくるためです。複数の項目に指摘が付いているときは、項目ごとに +1 コミットへ分けてください。 + 判定はオーケストレータが **git と実際のテスト実行**から行います。結果ファイルに 何と書いても検査結果は変わりません。 diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/review.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/review.md index ed083c9..5c8144d 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/review.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/review.md @@ -31,7 +31,7 @@ $RF_ITEMS | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | **振る舞い不変を筆頭に置く。** 疑わしければ実際に `$RF_BASELINE_TEST` を実行して diff --git a/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py index 685de46..87b018b 100755 --- a/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py @@ -145,6 +145,33 @@ def vocabulary() -> dict[str, Any]: "公開の直前に進行側がまとめて生成する。" ) +# 計画と生成物を 1 つのコミットへまとめたときのメッセージ。 +SYNC_AND_PLAN_COMMIT_MESSAGE = ( + "Chore: 生成物と改修計画を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "改修計画は提案の時点でしか残らないため、公開の直前に書き出す。\n" + "どちらも進行側の責務なので、1 つのコミットにまとめる。" +) + +# 改修計画だけを記録したコミットのメッセージ。 +PLAN_COMMIT_MESSAGE = ( + "Docs: 改修計画を記録する（cross-refactoring 進行側）\n\n" + "なぜ直すのか（理由）とどう直すのか（手順）は提案の時点でしか残らない。\n" + "状態ファイルは差分から除外されるため、Pull Request から読める場所へ置く。" +) + +# 改修計画を書き出す既定のディレクトリ。 +DEFAULT_PLAN_DIR = "issues" + +# 改善項目の状態を、Pull Request を読む側に通じる語へ置き換える。 +ITEM_STATUS_LABELS = { + "pending": "未着手", + "reviewing": "レビュー中", + "done": "採用", + "abandoned": "取り消し", + "blocked": "着手せず", +} + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -168,6 +195,19 @@ def vocabulary() -> dict[str, Any]: }) EXTRACTION_DIFF_BUDGET_FACTOR = 3 +# 1 改善項目が履歴に残せるコミット数。 +# +# **手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** +# 手ごとにテストを回して安全に進めるのは変わらないが、残すのは項目単位の +# 1 コミットだけにする。刻んだままだと Pull Request を読む側が改善項目と履歴を +# 1 対 1 で辿れず、取り消しと積み直しのコミットも件数に比例して増える +# （実測: 採用 12 件に対して適用 34 コミット、取り消しと積み直しで 25 コミット）。 +MAX_COMMITS_PER_ITEM = 1 + +# 現状固定テストが要る項目だけは 2 コミットを許す。テストと実装を 1 コミットへ +# 混ぜると、「テストを先に足した」ことを履歴から確かめられなくなる。 +MAX_COMMITS_PER_ITEM_WITH_TEST_GAP = 2 + # テスト 1 回あたりの上限（秒）。生成されたコードやテストが無限ループに入ると、 # 待ち続けて**進行全体が止まる**。打ち切って失敗として扱う。 DEFAULT_TEST_TIMEOUT = 900 @@ -579,7 +619,7 @@ def verify_apply_item( 確かめられる**。読ませ方の不確実性に対する最後の砦としてここを厚くする。 """ if not facts: - return "コミットが 1 件もありません（1 手 1 コミットの前提を満たしていません）" + return "コミットが 1 件もありません（1 改善項目 = 1 コミットの前提を満たしていません）" for commit in facts: problem = _verify_commit_basics( @@ -617,9 +657,38 @@ def verify_apply_item( f"実差分 {actual} 行が差分予算 {budget} 行" f"（見積 {estimated} 行 × {factor}）を超えました（範囲の逸脱）" ) + + # 粒度は最後に見る。トレーラーやテストの問題を粒度の失敗で覆い隠さない。 + # 数えるのは**実在するコミットの数**である。同じコミットを重ねて申告しただけの + # ときに落とすと、食い違いの無い申告を刻みすぎとして扱ってしまう。 + problem = verify_commit_granularity(item, len({c.get("sha") for c in facts})) + if problem: + return problem return None +def commit_limit_for(item: dict[str, Any]) -> int: + """その項目が履歴に残せるコミット数。""" + if item.get("test_gap"): + return MAX_COMMITS_PER_ITEM_WITH_TEST_GAP + return MAX_COMMITS_PER_ITEM + + +def verify_commit_granularity(item: dict[str, Any], count: int) -> Optional[str]: + """項目のコミット数が上限に収まっているか。超えていれば理由を返す。 + + 適用（`verify_apply_item`）と修正（`_verify_fix_commits`）で**同じ基準**を使う。 + 適用側だけ揃えると、レビュー指摘への対応という名目で刻んだ履歴が戻ってくる。 + """ + limit = commit_limit_for(item) + if count <= limit: + return None + return ( + f"項目 {item['item_id']} のコミットが {count} 件あります" + f"（残すのは 1 項目 = 1 コミット。現状固定テストが要る項目だけ 2 コミットまで）" + ) + + # ---------------- レビュー判定 ---------------- REVIEW_URL_MARKER = "#pullrequestreview-" @@ -1011,6 +1080,11 @@ def cmd_init(args: argparse.Namespace) -> None: "baseline_test": baseline, # 生成物の同期は**進行側の責務**。push の直前に実行する。 "sync_command": args.sync_command, + # 改修計画の書き出し先も同じ経路に乗せる。指定が無ければ既定のパスを使い、 + # 空文字なら記録しない。 + "plan_file": normalize_plan_file( + default_plan_file(args.pr) if args.plan_file is None else args.plan_file + ), "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -2018,7 +2092,7 @@ def cmd_abandon_items(args: argparse.Namespace) -> None: """Step 6 — 未解決の指摘に紐づく改善項目だけを取り消す。 **合意済みの項目は Pull Request に残す。** これを可能にするために、適用は - 項目ごとに 1 手 1 コミットへ分け、状態ファイルへコミットを記録している。 + 項目ごとに 1 コミットへまとめ、状態ファイルへコミットを記録している。 """ path, state = _load(args.id) entry = _round(state, args.round) @@ -2180,6 +2254,7 @@ def _verify_fix_commits( """ problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) + seen: dict[str, set[str]] = {} # item_id -> 実在するコミットの集合 for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, scope) @@ -2187,7 +2262,16 @@ def _verify_fix_commits( problems.append(problem) info(f"❌ 修正コミットが手順を満たしていません: {problem}") continue + seen.setdefault(item_id, set()).add(commit["sha"]) accepted.append((item_id, commit["sha"])) + + # 粒度は 1 件ずつの検証が済んでから見る。壊れたコミットの理由を + # 粒度の失敗で覆い隠さない。 + for item_id, shas in seen.items(): + problem = verify_commit_granularity({"item_id": item_id}, len(shas)) + if problem: + problems.append(problem) + info(f"❌ 修正コミットが手順を満たしていません: {problem}") return problems, accepted @@ -2379,6 +2463,96 @@ def cmd_status(args: argparse.Namespace) -> None: print(_round_table(state)) +def default_plan_file(pr: int) -> str: + """改修計画を書き出す既定のパス。""" + return f"{DEFAULT_PLAN_DIR}/refactoring-plan-rf{pr}.md" + + +def normalize_plan_file(value: Optional[str]) -> str: + """改修計画の書き出し先を検証して正規化する。空文字は「記録しない」。 + + **作業ディレクトリの外へ書かせない。** 進行側は利用者のリポジトリを触るので、 + 絶対パスと親へ抜ける経路は受け取った時点で拒む。 + + 正規化するのは、判定に使うパスを git の出力と揃えるためでもある。 + `./issues/plan.md` のまま持つと、`git status` が返す `issues/plan.md` と + 一致せず、公開のコミットメッセージが取り違えられる。 + """ + rel = str(value or "").strip() + if not rel: + return "" + if os.path.isabs(rel) or (len(rel) > 1 and rel[1] == ":"): + die(f"--plan-file には相対パスを指定してください: {rel}", code=4) + normalized = os.path.normpath(rel) + if normalized == ".." or normalized.startswith(".." + os.sep): + die( + f"--plan-file が作業ディレクトリの外を指しています: {rel}", + code=4, + ) + return normalized + + +def format_plan(state: dict[str, Any]) -> str: + """改修計画の本文を組み立てる。**同じ状態からは同じ本文が出る。** + + 提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から + 除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画 + だったのかも見えない。ここで差分の中へ置く。 + """ + baseline = state.get("baseline_test") or {} + lines = [ + f"# 改修計画 — {state['repo']} #{state['current_pr']}", + "", + "`/ndf:cross-refactoring` が提案し、適用した改善項目の記録である。", + "理由と手順は提案の時点でしか残らないため、公開の直前に書き出している。", + "", + f"- 対象範囲: {', '.join(state.get('target_scope') or []) or '（未指定）'}", + f"- 着手前のテスト: {baseline.get('command') or '（未指定）'}", + "", + ] + for entry in state.get("rounds") or []: + lines.extend(_plan_round_section(state, entry)) + if not (state.get("rounds") or []): + lines.append("（改善項目なし）") + return "\n".join(lines).rstrip() + "\n" + + +def _plan_round_section(state: dict[str, Any], entry: dict[str, Any]) -> list[str]: + """1 ラウンド分の見出しと、そのラウンドの改善項目を並べる。""" + reviewers = " / ".join(entry.get("reviewers") or []) or "—" + lines = [ + f"## ラウンド {entry['round']}" + f"（実装 {entry.get('impl', '—')} / レビュー {reviewers}）", + "", + ] + items = [i for i in state.get("items") or [] if i.get("round") == entry["round"]] + if not items: + lines.extend(["（採用した改善項目なし）", ""]) + return lines + for item in items: + lines.extend(_plan_item_section(item)) + return lines + + +def _plan_item_section(item: dict[str, Any]) -> list[str]: + """改善項目 1 件の見出し・要約表・理由・手順。""" + status = ITEM_STATUS_LABELS.get(item.get("status"), item.get("status") or "—") + return [ + f"### {item['item_id']} — `{item['path']}#{item['symbol']}`", + "", + "| スメル | 手法 | 重要度 | 提案元 | 状態 | コミット |", + "| --- | --- | --- | --- | --- | ---: |", + f"| {item['smell']} | {item['technique']} | {item['severity']} | " + f"{' / '.join(item.get('proposed_by') or []) or '—'} | {status} | " + f"{len(item.get('commits') or [])} |", + "", + f"**なぜ**: {item.get('rationale') or '（記録なし）'}", + "", + f"**手順**: {item.get('plan') or '（記録なし）'}", + "", + ] + + def cmd_report(args: argparse.Namespace) -> None: """Step 8 — ラウンド表・項目表・見送り項目・指標を出す。""" _, state = _load(args.id) @@ -3206,7 +3380,31 @@ def _run_sync_command(state: dict[str, Any], work: str, command: str) -> None: ) -def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: +def _write_plan_file(state: dict[str, Any], work: str, rel: str) -> None: + """改修計画を作業ディレクトリの中へ書き出す。 + + 内容は状態から決まるので、**状態が動いていなければ差分は出ない**。 + 書き出しを毎回行っても、余計なコミットは積まれない。 + """ + path = pathlib.Path(work) / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(format_plan(state), encoding="utf-8") + + +def _publish_commit_message(produced: list[str], plan_rel: str) -> str: + """公開の直前に積むコミットのメッセージを、中身に合わせて選ぶ。""" + has_plan = bool(plan_rel) and plan_rel in produced + has_generated = any(p != plan_rel for p in produced) + if has_plan and has_generated: + return SYNC_AND_PLAN_COMMIT_MESSAGE + if has_plan: + return PLAN_COMMIT_MESSAGE + return SYNC_COMMIT_MESSAGE + + +def _commit_sync_changes( + work: str, command: str, produced: list[str], plan_rel: str = "" +) -> None: """同期が作った差分を進行側のコミットとして積む。差分が無ければ何もしない。 このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 @@ -3221,11 +3419,15 @@ def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: # 確認済みだからである。 try: _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + _sh(["git", "commit", "-m", _publish_commit_message(produced, plan_rel)], + cwd=work) except SystemExit: _discard_worktree_changes(work) raise - info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + if command: + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + else: + info(f"📝 改修計画を記録しました（{len(produced)} ファイル）") def _sync_generated(state: dict[str, Any]) -> None: @@ -3243,14 +3445,22 @@ def _sync_generated(state: dict[str, Any]) -> None: 利用者のリポジトリの検査を壊したまま進むことになる。 """ command = str(state.get("sync_command") or "").strip() - if not command: + # 状態ファイルの値も受け取った時点と同じ基準で通す。旧い状態ファイルや + # 手で書き換えられた値でも、作業ディレクトリの外へは書き出さない。 + plan_rel = normalize_plan_file(state.get("plan_file")) + if not command and not plan_rel: return work = state["worktrees"]["work"] # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 # 同期が作った差分と元からあった差分を区別できない。 _require_clean_worktree(state, work) - _run_sync_command(state, work, command) - _commit_sync_changes(work, command, _dirty_paths(state, work)) + # 改修計画も生成物と同じ経路に乗せる。**別のコミットに分けない。** + # 分けると、進行側のコミットが公開のたびに 2 つずつ積まれる。 + if plan_rel: + _write_plan_file(state, work, plan_rel) + if command: + _run_sync_command(state, work, command) + _commit_sync_changes(work, command, _dirty_paths(state, work), plan_rel) def _push_head(state: dict[str, Any]) -> None: @@ -3398,6 +3608,12 @@ def main() -> None: help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" "差分があれば進行側のコミットとして積む。" "同期を実装担当にさせると範囲外の変更になるため分離している") + init.add_argument("--plan-file", default=None, + help="改修計画を書き出すパス（対象リポジトリからの相対）。" + "提案の理由と手順は状態ファイルにしか残らず、差分から" + "除外されるため、公開の直前に進行側が書き出す。" + f"既定は {DEFAULT_PLAN_DIR}/refactoring-plan-rf.md。" + "空文字を渡すと記録しない") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_commit_granularity.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_commit_granularity.py new file mode 100644 index 0000000..aeec6f7 --- /dev/null +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_commit_granularity.py @@ -0,0 +1,125 @@ +"""コミットの粒度（1 改善項目 = 1 コミット）のテスト。 + +手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。 +**残すのは項目単位の 1 コミットだけ**にして、Pull Request を読む側が +改善項目と履歴を 1 対 1 で辿れるようにする。 + +現状固定テストが要る項目（`test_gap`）だけは、テストと実装を混ぜないために +2 コミットを許す。 +""" +from __future__ import annotations + +import pytest + + +def trailers(item_id="R1-001", round_no="1", runtime="codex", model="gpt-5.5"): + return { + "Item-Id": item_id, "Round": round_no, + "Impl-Runtime": runtime, "Impl-Model": model, + } + + +def fact(sha="abc1234", **over): + base = { + "sha": sha, "exists": True, "test_status": "pass", + "touches_tests": False, "diff_lines": 30, "trailers": trailers(), + } + base.update(over) + return base + + +def item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 400, "proposed_by": ["codex"], + "status": "pending", "commits": [], + } + base.update(over) + return base + + +# ---------- 適用フェーズ ---------- + +def test_one_commit_per_item_passes(refactor): + assert refactor.verify_apply_item(item(), [fact()]) is None + + +def test_two_commits_for_one_item_fails(refactor): + """途中経過を刻むと、改善項目と履歴が 1 対 1 で対応しなくなる。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb")] + ) + assert problem is not None and "1 コミット" in problem + + +def test_the_granularity_message_names_the_item(refactor): + """どの項目が刻みすぎたのかを読めるようにする。""" + problem = refactor.verify_apply_item( + item(item_id="R2-003"), [fact(sha="aaa", trailers=trailers(item_id="R2-003")), + fact(sha="bbb", trailers=trailers(item_id="R2-003"))] + ) + assert "R2-003" in problem + + +def test_test_gap_allows_the_characterization_test_commit(refactor): + """テストと実装を 1 コミットへ混ぜないため、この項目だけ 2 コミットを許す。""" + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb")] + assert refactor.verify_apply_item(item(test_gap=True), facts) is None + + +def test_test_gap_still_rejects_three_commits(refactor): + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb"), fact(sha="ccc")] + problem = refactor.verify_apply_item(item(test_gap=True), facts) + assert problem is not None and "2 コミット" in problem + + +def test_a_broken_commit_is_reported_before_the_granularity(refactor): + """粒度は最後に見る。トレーラーやテストの問題を粒度で覆い隠さない。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb", test_status="fail")] + ) + assert problem is not None and "テストが成功していません" in problem + + +def test_the_budget_is_reported_before_the_granularity(refactor): + """差分予算の超過は原因が別なので、粒度より先に伝える。""" + problem = refactor.verify_apply_item( + item(technique="rename", estimated_diff_lines=10), + [fact(sha="aaa", diff_lines=50), fact(sha="bbb", diff_lines=50)], + ) + assert problem is not None and "差分予算" in problem + + +# ---------- 修正フェーズ ---------- + +def test_fix_accepts_one_commit_per_item(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-002"))] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] + assert accepted == [("R1-001", "aaa"), ("R1-002", "bbb")] + + +def test_fix_rejects_two_commits_for_the_same_item(refactor): + """適用側だけ揃えると、指摘への対応という名目で刻んだ履歴が戻ってくる。""" + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert problems and any("R1-001" in p and "1 コミット" in p for p in problems) + + +def test_fix_granularity_does_not_hide_a_broken_commit(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", test_status="fail", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert any("テストが成功していません" in p for p in problems) + + +@pytest.mark.parametrize("count", [1, 2, 3]) +def test_fix_allows_one_commit_for_each_distinct_item(refactor, count): + facts = [fact(sha=f"s{i}", trailers=trailers(item_id=f"R1-00{i}")) + for i in range(count)] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] and len(accepted) == count diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_init.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_init.py index 5667ae3..d5fbf92 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_init.py @@ -59,7 +59,7 @@ def _args(tmp_path, **over): "pr": 130, "scope": ["src"], "host": "claude", "max_outer_rounds": 3, "max_fix_rounds": 3, "max_items_per_round": 5, "severity_threshold": "minor", "model": None, "baseline_test": "true", - "sync_command": None, + "sync_command": None, "plan_file": None, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } @@ -429,3 +429,25 @@ def test_init_fills_the_posting_event_when_resuming_an_old_state(run_init, tmp_p assert resumed["is_own_pr"] is True assert resumed["event_downgrade"] is True assert "COMMENT" in resumed["review_post_note"] + + +# ---------- 改修計画の書き出し先 ---------- + +def test_init_records_the_default_plan_file(run_init, tmp_path): + """指定が無くても計画を残す。**既定で残らないと、誰も指定しない。**""" + run_init(_args(tmp_path)) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "issues/refactoring-plan-rf130.md" + + +def test_init_keeps_an_explicit_plan_file(run_init, tmp_path): + run_init(_args(tmp_path, plan_file="docs/plan.md")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "docs/plan.md" + + +def test_init_accepts_an_empty_plan_file_as_off(run_init, tmp_path): + """計画を差分へ入れたくないリポジトリのために、空文字で無効にできる。""" + run_init(_args(tmp_path, plan_file="")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "" diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_merge_apply.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_merge_apply.py index fa29964..d341a7f 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/tests/test_merge_apply.py +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_merge_apply.py @@ -76,7 +76,7 @@ def test_commit_that_does_not_exist_fails(refactor): assert problem is not None and "範囲にありません" in problem -# ---------- 1 手 1 コミット ---------- +# ---------- 1 改善項目 = 1 コミット ---------- def test_zero_commits_fails(refactor): problem = refactor.verify_apply_item(item(), []) diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_plan_file.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_plan_file.py new file mode 100644 index 0000000..fe6c508 --- /dev/null +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_plan_file.py @@ -0,0 +1,233 @@ +"""改修計画をリポジトリ内のファイルへ残すテスト。 + +提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から除外される。 +**Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない。** +計画を差分の中へ置き、公開は生成物の同期と同じ経路（進行側の 1 コミット）に乗せる。 +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + + +def _make_work(tmp_path): + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "1 関数が 6 段の処理を通しで行っている", + "plan": "1. 範囲の確定を切り出す 2. 検証を切り出す", + "test_gap": False, "estimated_diff_lines": 40, + "proposed_by": ["codex", "gemini"], "status": "done", "commits": ["abc1234"], + } + base.update(over) + return base + + +def _state(tmp_path, work=None, **over): + rounds = over.pop("rounds", [{ + "round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0, + }]) + items = over.pop("items", [_item()]) + worktrees = {"work": str(work or tmp_path / "work")} + for r in ("codex", "gemini", "kiro"): + worktrees[r] = str(tmp_path / r) + path = make_state(tmp_path, rounds=rounds, items=items, + worktrees=worktrees, **over) + return path, read_state(path) + + +# ---------- 計画の本文 ---------- + +def test_plan_names_the_item_and_the_target(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "R1-001" in text and "src/foo.py" in text and "Foo.handle" in text + + +def test_plan_carries_the_reason_and_the_steps(refactor, tmp_path): + """なぜ直すのか・どう直すのかは、提案の時点でしか残らない。""" + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "1 関数が 6 段の処理を通しで行っている" in text + assert "1. 範囲の確定を切り出す" in text + + +def test_plan_shows_the_smell_and_the_technique(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "long_method" in text and "extract_method" in text + + +def test_plan_records_who_proposed_it(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_marks_an_abandoned_item(refactor, tmp_path): + """取り消した項目も残す。同じ提案が再び来たときの判断材料になる。""" + _, state = _state(tmp_path, items=[_item(status="abandoned", commits=[])]) + text = refactor.format_plan(state) + assert "取り消し" in text + + +def test_plan_groups_items_by_round(refactor, tmp_path): + rounds = [ + {"round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0}, + {"round": 2, "impl": "kiro", "reviewers": ["codex", "gemini"], + "items": ["R2-001"], "reviews": [], "fix_rounds": 0}, + ] + items = [_item(), _item(item_id="R2-001", round=2)] + _, state = _state(tmp_path, rounds=rounds, items=items) + text = refactor.format_plan(state) + assert text.index("ラウンド 1") < text.index("ラウンド 2") + + +def test_plan_names_the_implementer_of_each_round(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_is_stable_for_the_same_state(refactor, tmp_path): + """同じ状態からは同じ本文が出る。差分が出続けると毎回コミットが積まれる。""" + _, state = _state(tmp_path) + assert refactor.format_plan(state) == refactor.format_plan(state) + + +# ---------- 置き場所 ---------- + +def test_the_default_plan_file_lives_under_issues(refactor): + assert refactor.default_plan_file(136) == "issues/refactoring-plan-rf136.md" + + +def test_the_plan_file_is_written_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), "issues/plan.md") + + written = (work / "issues" / "plan.md").read_text(encoding="utf-8") + assert "R1-001" in written + + +# ---------- 公開 ---------- + +def test_the_plan_lands_in_one_commit_with_the_generated_files(refactor, tmp_path): + """計画書と生成物で 2 コミットに分けない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command="printf 'x = 2\\n' > generated/out.py") + + refactor._sync_generated(state) + + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files and "generated/out.py" in files + assert subject and "cross-refactoring" in subject + + +def test_a_repository_without_a_sync_command_still_records_the_plan( + refactor, tmp_path +): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + + refactor._sync_generated(state) + + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files + + +def test_an_unchanged_plan_does_not_add_a_commit(refactor, tmp_path): + """状態が動いていないのにコミットを積まない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + refactor._sync_generated(state) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +def test_an_empty_plan_file_setting_turns_the_record_off(refactor, tmp_path): + """計画を差分へ入れたくないリポジトリのために、無効にできる。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="", sync_command=None) + + refactor._sync_generated(state) + + assert not (work / "issues").exists() + + +# ---------- 書き出し先の検証 ---------- + +def test_a_relative_path_is_kept(refactor): + assert refactor.normalize_plan_file("issues/plan.md") == "issues/plan.md" + + +def test_a_leading_dot_is_normalized(refactor): + """`./issues/plan.md` は git が返すパスと一致しない。正規化して揃える。""" + assert refactor.normalize_plan_file("./issues/plan.md") == "issues/plan.md" + + +def test_an_empty_value_stays_empty(refactor): + assert refactor.normalize_plan_file("") == "" + assert refactor.normalize_plan_file(None) == "" + + +def test_an_absolute_path_is_refused(refactor): + """作業ディレクトリの外へ書かせない。進行側は利用者のリポジトリを触る。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("/tmp/out.md") + + +def test_a_parent_traversal_is_refused(refactor): + with pytest.raises(SystemExit): + refactor.normalize_plan_file("../out.md") + + +def test_a_traversal_in_the_middle_is_refused(refactor): + """途中で外へ出る経路も拒む。正規化してから判定する。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("issues/../../out.md") + + +def test_the_written_path_stays_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), state["plan_file"]) + + assert (work / "issues" / "plan.md").exists() + assert not (tmp_path / "plan.md").exists() diff --git a/plugins/ndf-codex/.codex-plugin/plugin.json b/plugins/ndf-codex/.codex-plugin/plugin.json index 4751cf8..9a25c90 100644 --- a/plugins/ndf-codex/.codex-plugin/plugin.json +++ b/plugins/ndf-codex/.codex-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ndf", - "version": "8.5.4", - "description": "Codex plugin (v8.5.4): 25 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, external AI delegation (Codex/Gemini), and optional Slack completion notifications.", + "version": "8.6.0", + "description": "Codex plugin (v8.6.0): 25 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, external AI delegation (Codex/Gemini), and optional Slack completion notifications.", "skills": "./skills/", "hooks": "./hooks/hooks.json" } diff --git a/plugins/ndf-codex/README.md b/plugins/ndf-codex/README.md index ef34a99..a3d2219 100644 --- a/plugins/ndf-codex/README.md +++ b/plugins/ndf-codex/README.md @@ -47,7 +47,7 @@ Claude Code 専用の agents、statusline 自動設定、transcript retention ```text # 動く: 実体パスを示して読ませる -~/.codex/plugins/cache/ai-plugins/ndf/8.5.4/skills/deploy/SKILL.md を読んで、その手順どおりに qa/staging へ deploy PR を作成してください。 +~/.codex/plugins/cache/ai-plugins/ndf/8.6.0/skills/deploy/SKILL.md を読んで、その手順どおりに qa/staging へ deploy PR を作成してください。 # 動かない: 明示起動 ($ は展開されない) $deploy qa/staging @@ -69,14 +69,14 @@ marketplace 経由でインストールした場合、Skill の実体は **ワ ```text $CODEX_HOME/plugins/cache////skills//SKILL.md # 既定 ($CODEX_HOME=~/.codex) の例: -# ~/.codex/plugins/cache/ai-plugins/ndf/8.5.4/skills/deploy/SKILL.md +# ~/.codex/plugins/cache/ai-plugins/ndf/8.6.0/skills/deploy/SKILL.md ``` そのため「`deploy` の SKILL.md を探して読んで」のような曖昧な依頼は、Codex のファイル探索がワークスペース内に限られる状況では失敗しえます。**抑止した Skill は `$` が展開されない**ので、`codex plugin list` で実体パスを確認し、絶対パスを渡してください。 ```bash codex plugin list | grep 'ndf@ai-plugins' -# => ndf@ai-plugins installed, enabled 8.5.4 +# => ndf@ai-plugins installed, enabled 8.6.0 ``` 抑止していない Skill（`markdown-writing` など）はキャッシュ配下でも `$` で解決するため、そちらは `$` 起動が使えます。 diff --git a/plugins/ndf-codex/skills/cross-refactoring/SKILL.md b/plugins/ndf-codex/skills/cross-refactoring/SKILL.md index 9120ec5..812d285 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-codex/skills/cross-refactoring/SKILL.md @@ -37,6 +37,8 @@ allowed-tools: | 役割の分離 | 提案・レビューは**ホストを除く 3 者**、適用は**gemini を除く 3 者**。両者は重なるが一致しない | | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | +| コミットの単位 | **1 改善項目 = 1 コミット。** テストも項目の単位で 1 回だけ求める（現状固定テストが要る項目のみ 2 コミット） | +| 改修計画 | **差分の中へ残す。** 理由と手順は提案の時点でしか残らない。公開の直前に進行側が書き出し、生成物の同期と同じコミットへ入れる | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | | 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | | 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | @@ -61,6 +63,7 @@ allowed-tools: | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | | `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | +| `--plan-file PATH` | 改修計画の書き出し先（**対象リポジトリからの相対パス**）。空文字を渡すと記録しない | `issues/refactoring-plan-rf.md` | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" @@ -128,7 +131,7 @@ flowchart TD Merge --> Empty{"採用件数 = 0 ?"} Empty -->|はい| Final([提案ラウンドの繰り返しを終了]):::ok Empty -->|いいえ| Apply - Apply["Step 4: 適用（実装担当 1 CLI）
項目ごとに 1 手 1 コミット"] + Apply["Step 4: 適用（実装担当 1 CLI）
1 改善項目 = 1 コミット"] Apply --> Review["Step 5: レビュー（2 CLI 並列）
ラウンドの差分をまとめて 1 回"] Review --> Judge{"2 者とも承認 ?"} Judge -->|いいえ| Fix["Step 6: 指摘修正（実装担当）"] @@ -275,6 +278,9 @@ done | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | +| 1 項目を複数のコミットへ刻む | 改善項目と履歴が 1 対 1 で辿れなくなり、取り消しと積み直しのコミットも件数に比例して増える | +| 実装担当に手ごとのテストを義務づける | 進行側もコミットごとに回すため、テストの実行回数が手数の 2 倍になる（実測 44 手で 88 回） | +| 改修計画を状態ファイルにだけ残す | 状態ファイルは差分から除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない | | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | 投稿に失敗したまま結果ファイルを書かずに終了する | 進行側からは「レビュー担当が動かなかった」と区別が付かない。失敗したときほど `post_error` 付きの結果ファイルが要る | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | diff --git a/plugins/ndf-codex/skills/cross-refactoring/docs/01-state-and-propose.md b/plugins/ndf-codex/skills/cross-refactoring/docs/01-state-and-propose.md index fb75178..e2a06f1 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/docs/01-state-and-propose.md +++ b/plugins/ndf-codex/skills/cross-refactoring/docs/01-state-and-propose.md @@ -46,10 +46,14 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 +7. **改修計画の書き出し先の記録** — `--plan-file` を状態ファイルへ保存する。 + 既定は `issues/refactoring-plan-rf.md` で、空文字を渡すと記録しない。 + **既定で残す**のは、指定できるだけでは誰も指定しないためである。書き出しは + 初期化時ではなく、生成物の同期と同じく**push の直前**に行う +8. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） -8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +9. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない @@ -105,7 +109,7 @@ gemini は NDF の配布先ではないため「標準の配置先」を持た | `quality-gates` | 「直し終わった」と言える条件の判定 | `ndf-policies` は**配置しない**。git 運用やコミット規約といったリポジトリ運用の方針で -あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 手 1 コミット、 +あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 改善項目 = 1 コミット、 コミットトレーラー、`--force` 禁止など）はプロンプト側で明示する。 ### 守ること diff --git a/plugins/ndf-codex/skills/cross-refactoring/docs/02-apply-and-review.md b/plugins/ndf-codex/skills/cross-refactoring/docs/02-apply-and-review.md index c311658..f9bbf23 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/docs/02-apply-and-review.md +++ b/plugins/ndf-codex/skills/cross-refactoring/docs/02-apply-and-review.md @@ -39,6 +39,7 @@ | 各コミットでテストが成功している | 実際の実行 | コミットを取り出して着手前と同じテストを走らせる。**JSON の `test_status` は読まない**。`--test-timeout`（既定 900 秒）を超えたら失敗 | | テストが無い経路は先に現状固定テスト | `git show --name-only` | `test_gap` が真の項目は、先頭コミットがテストの置き場所を触っている | | 項目の分離 | git のトレーラー | 各コミットの `Item-Id` がその項目と一致する。複数の項目を 1 コミットにまとめたら失敗 | +| コミットの粒度 | 申告されたコミットの実数 | 1 改善項目 = 1 コミット。`test_gap` が真の項目だけ 2 コミット | | 差分予算 | `git show --numstat` | 実差分の合計が `estimated_diff_lines` の 2 倍（抽出系の手法は 3 倍）を超えたら失敗（範囲の逸脱） | | 対象範囲の遵守 | `git show --name-only` | 触ったファイルが全て `--scope` の中にある。1 つでも外なら失敗 | | 機能変更の混入なし | — | 機械判定は不可能。レビュー観点に委ねる | @@ -74,6 +75,59 @@ 真のときの現状固定テストを数えることを明記した。倍率だけを広げると、見積が 楽観側へ倒れたぶんまで通してしまう。 +#### 1 改善項目 = 1 コミットにする + +**手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** 手順は +順に適用してよいが、残すのは項目単位の 1 コミットだけにする。 + +刻んだままだと 3 つの読みにくさが出る。 + +| 影響 | 内容 | +| --- | --- | +| 履歴 | Pull Request を読む側が、改善項目と履歴を 1 対 1 で辿れない | +| 取り消し | 範囲を戻して積み直すコミットが、刻んだ件数に比例して増える | +| 検証 | コミットごとにテストを実行するため、実行回数も比例して増える | + +実測では採用 12 件に対して適用が 34 コミット、取り消しと積み直しで 25 コミットだった。 + +**テストの回数も項目の単位に合わせる。** 進行側は申告されたコミットごとにテストを +実行するので、実装担当にも手ごとの実行を義務づけると、同じテストが手数の 2 倍だけ +走る。実測では 44 手に対して 88 回（1 回 26 秒として約 38 分）だった。 + +| 誰が | いつ | 6 回目の実測 | 項目単位にした後 | +| --- | --- | ---: | ---: | +| 実装担当 | コミットの前に 1 回 | 44 | 14 | +| 進行側 | 申告されたコミットごと | 44 | 14 | + +実装担当が途中で確かめる分には止めない。求めるのは**コミットの前に通っていること** +だけで、そこは進行側が実際に実行して確かめる。 + +例外は現状固定テストが要る項目（`test_gap` が真）だけで、「テスト → 実装」の +2 コミットを許す。1 つに混ぜると、テストが先行したことを履歴から確かめられない。 + +プロンプトは、途中で刻みたいときの戻し方も渡す。**その項目に着手する前の HEAD**を +控えておき、最後に `git reset --soft` で 1 コミットへまとめる。控えた地点より前へ +戻すと他の項目のコミットを巻き込むため、起点は項目の着手前に固定する。 + +#### 改修計画を差分へ残す + +**なぜ直すのか（理由）とどう直すのか（手順）は、提案の時点でしか残らない。** +状態ファイルには入っているが、そのディレクトリは差分から除外されるため、 +Pull Request を読む側からは見えない。 + +そこで `--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出す。 + +- 内容は**状態から決まる**。状態が動いていなければ差分は出ないので、公開のたびに + 書き出しても余計なコミットは積まれない +- 取り消した項目も残す。同じ提案が次のラウンドで来たときの判断材料になる +- 公開は生成物の同期と**同じコミット**に乗せる。分けると進行側のコミットが + 公開のたびに 2 つずつ積まれる +- 空文字を渡すと記録しない。差分へ入れたくないリポジトリのための逃げ道である +- **絶対パスと親へ抜ける経路は受け取った時点で拒む**（終了コード 4）。進行側は利用者の + リポジトリを触るため、作業ディレクトリの外へ書き出す余地を残さない。あわせて + `./issues/plan.md` のような表記も正規化する。git が返すパスと形が違うと、公開の + コミットメッセージが取り違えられる + #### 範囲の指定は検証にも効かせる `--scope` を必須にした目的は**提案の発散と変更の肥大を防ぐ**ことなので、指定を検証へ @@ -312,7 +366,7 @@ claude が参加する構成では実際のリファクタリング 1 件で 1.4 | --- | --- | | 項目ごとに実装者が入れ替わらない | 輪番の単位がラウンドなので、重ねれば実装者は分散する | | 指摘がどの項目に対するものか曖昧になる | 指摘に改善項目 ID を**必須**とし、未知の ID と欠落は差し戻す | -| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 手 1 コミットを保ち、**取り消しは項目単位**で行う | +| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 コミットを保ち、**取り消しは項目単位**で行う | ### 判定 diff --git a/plugins/ndf-codex/skills/cross-refactoring/docs/03-review-viewpoints.md b/plugins/ndf-codex/skills/cross-refactoring/docs/03-review-viewpoints.md index d607ea9..536799f 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/docs/03-review-viewpoints.md +++ b/plugins/ndf-codex/skills/cross-refactoring/docs/03-review-viewpoints.md @@ -14,7 +14,7 @@ | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | ## 振る舞い不変を筆頭に置く理由 diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md index ec9c8b9..f42ae6c 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md @@ -27,9 +27,18 @@ $RF_ITEMS 1. `test_gap` が真なら、**先に現状固定テストを追加してコミットする**。 これは省略できません。振る舞いが変わっていないことを示す手段が無いまま 構造を変えるのは、構造改善ではなく単なる編集です -2. `plan` の手順を **1 手ずつ**適用する -3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** -4. 通ったらコミットする（**1 手 = 1 コミット**） +2. `plan` の手順を順に適用する +3. **手順を終えてから** `$RF_BASELINE_TEST` を **1 回**実行する。落ちたら原因の手を戻す +4. 通ったら、**その項目の変更をまとめて 1 コミットにする** + +**テストもコミットも項目の単位で 1 回です。** 手ごとに回すと、進行側の検証と +合わせて手数の 2 倍だけテストが走ります（実測では 44 手で 88 回）。手ごとに +確かめたいときは自分の判断で回して構いませんが、**求めているのはコミットの前に +通っていること**だけです。 + +途中で刻んでおきたいときは、**その項目に着手する前の HEAD を控えておき**、 +最後に `git reset --soft <控えた HEAD>` してから 1 回だけコミットしてください +（控えた HEAD より前へ戻すと、他の項目のコミットを巻き込みます）。 ## コミットの規約 @@ -47,6 +56,10 @@ Impl-Runtime: $RF_RUNTIME Impl-Model: $RF_MODEL ``` +- **1 改善項目 = 1 コミット。** 2 件以上に刻むと、その項目は失敗として扱われ、 + 取り消されます。例外は `test_gap` が真の項目だけで、「現状固定テスト → 実装」の + 2 コミットを許します（テストと実装を混ぜると、テストが先行したことを + 履歴から確かめられません） - **`Item-Id` は必ずその項目のものにする。** 複数の項目を 1 コミットへまとめると、 取り消し範囲が項目単位で決まらなくなり、失敗として扱われます - `Impl-Model` には**実際に使ったモデル名**を書く。分からなければ `default` diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md index bb2180a..dd3bf4b 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md @@ -24,7 +24,8 @@ $RF_ITEMS 1. `gh api` で Pull Request の**未解決レビュースレッド**を取得する 2. 各指摘について、**修正するか・しないか**を決める - - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする + - 修正する: 直してから `$RF_BASELINE_TEST` を 1 回実行し、**その改善項目の + 修正を 1 コミットにまとめる** - 修正しない: 根拠を返信する。**黙って閉じない** 3. 対応したスレッドに返信し、`resolveReviewThread` で解決する @@ -38,6 +39,11 @@ $RF_ITEMS いること**が必要です。満たさないコミットは取り込まれず、その改善項目の指摘は 解決済みになりません（修正ラウンドの上限に達すると項目ごと取り消されます）。 +**1 改善項目 = 1 コミット。** 同じ項目のコミットが 2 件以上あると、その修正 +ラウンドの範囲ごと取り消されます。適用側だけ揃えても、指摘への対応という名目で +刻んだ履歴が戻ってくるためです。複数の項目に指摘が付いているときは、項目ごとに +1 コミットへ分けてください。 + 判定はオーケストレータが **git と実際のテスト実行**から行います。結果ファイルに 何と書いても検査結果は変わりません。 diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/review.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/review.md index ed083c9..5c8144d 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/review.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/review.md @@ -31,7 +31,7 @@ $RF_ITEMS | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | **振る舞い不変を筆頭に置く。** 疑わしければ実際に `$RF_BASELINE_TEST` を実行して diff --git a/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py index 685de46..87b018b 100755 --- a/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py @@ -145,6 +145,33 @@ def vocabulary() -> dict[str, Any]: "公開の直前に進行側がまとめて生成する。" ) +# 計画と生成物を 1 つのコミットへまとめたときのメッセージ。 +SYNC_AND_PLAN_COMMIT_MESSAGE = ( + "Chore: 生成物と改修計画を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "改修計画は提案の時点でしか残らないため、公開の直前に書き出す。\n" + "どちらも進行側の責務なので、1 つのコミットにまとめる。" +) + +# 改修計画だけを記録したコミットのメッセージ。 +PLAN_COMMIT_MESSAGE = ( + "Docs: 改修計画を記録する（cross-refactoring 進行側）\n\n" + "なぜ直すのか（理由）とどう直すのか（手順）は提案の時点でしか残らない。\n" + "状態ファイルは差分から除外されるため、Pull Request から読める場所へ置く。" +) + +# 改修計画を書き出す既定のディレクトリ。 +DEFAULT_PLAN_DIR = "issues" + +# 改善項目の状態を、Pull Request を読む側に通じる語へ置き換える。 +ITEM_STATUS_LABELS = { + "pending": "未着手", + "reviewing": "レビュー中", + "done": "採用", + "abandoned": "取り消し", + "blocked": "着手せず", +} + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -168,6 +195,19 @@ def vocabulary() -> dict[str, Any]: }) EXTRACTION_DIFF_BUDGET_FACTOR = 3 +# 1 改善項目が履歴に残せるコミット数。 +# +# **手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** +# 手ごとにテストを回して安全に進めるのは変わらないが、残すのは項目単位の +# 1 コミットだけにする。刻んだままだと Pull Request を読む側が改善項目と履歴を +# 1 対 1 で辿れず、取り消しと積み直しのコミットも件数に比例して増える +# （実測: 採用 12 件に対して適用 34 コミット、取り消しと積み直しで 25 コミット）。 +MAX_COMMITS_PER_ITEM = 1 + +# 現状固定テストが要る項目だけは 2 コミットを許す。テストと実装を 1 コミットへ +# 混ぜると、「テストを先に足した」ことを履歴から確かめられなくなる。 +MAX_COMMITS_PER_ITEM_WITH_TEST_GAP = 2 + # テスト 1 回あたりの上限（秒）。生成されたコードやテストが無限ループに入ると、 # 待ち続けて**進行全体が止まる**。打ち切って失敗として扱う。 DEFAULT_TEST_TIMEOUT = 900 @@ -579,7 +619,7 @@ def verify_apply_item( 確かめられる**。読ませ方の不確実性に対する最後の砦としてここを厚くする。 """ if not facts: - return "コミットが 1 件もありません（1 手 1 コミットの前提を満たしていません）" + return "コミットが 1 件もありません（1 改善項目 = 1 コミットの前提を満たしていません）" for commit in facts: problem = _verify_commit_basics( @@ -617,9 +657,38 @@ def verify_apply_item( f"実差分 {actual} 行が差分予算 {budget} 行" f"（見積 {estimated} 行 × {factor}）を超えました（範囲の逸脱）" ) + + # 粒度は最後に見る。トレーラーやテストの問題を粒度の失敗で覆い隠さない。 + # 数えるのは**実在するコミットの数**である。同じコミットを重ねて申告しただけの + # ときに落とすと、食い違いの無い申告を刻みすぎとして扱ってしまう。 + problem = verify_commit_granularity(item, len({c.get("sha") for c in facts})) + if problem: + return problem return None +def commit_limit_for(item: dict[str, Any]) -> int: + """その項目が履歴に残せるコミット数。""" + if item.get("test_gap"): + return MAX_COMMITS_PER_ITEM_WITH_TEST_GAP + return MAX_COMMITS_PER_ITEM + + +def verify_commit_granularity(item: dict[str, Any], count: int) -> Optional[str]: + """項目のコミット数が上限に収まっているか。超えていれば理由を返す。 + + 適用（`verify_apply_item`）と修正（`_verify_fix_commits`）で**同じ基準**を使う。 + 適用側だけ揃えると、レビュー指摘への対応という名目で刻んだ履歴が戻ってくる。 + """ + limit = commit_limit_for(item) + if count <= limit: + return None + return ( + f"項目 {item['item_id']} のコミットが {count} 件あります" + f"（残すのは 1 項目 = 1 コミット。現状固定テストが要る項目だけ 2 コミットまで）" + ) + + # ---------------- レビュー判定 ---------------- REVIEW_URL_MARKER = "#pullrequestreview-" @@ -1011,6 +1080,11 @@ def cmd_init(args: argparse.Namespace) -> None: "baseline_test": baseline, # 生成物の同期は**進行側の責務**。push の直前に実行する。 "sync_command": args.sync_command, + # 改修計画の書き出し先も同じ経路に乗せる。指定が無ければ既定のパスを使い、 + # 空文字なら記録しない。 + "plan_file": normalize_plan_file( + default_plan_file(args.pr) if args.plan_file is None else args.plan_file + ), "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -2018,7 +2092,7 @@ def cmd_abandon_items(args: argparse.Namespace) -> None: """Step 6 — 未解決の指摘に紐づく改善項目だけを取り消す。 **合意済みの項目は Pull Request に残す。** これを可能にするために、適用は - 項目ごとに 1 手 1 コミットへ分け、状態ファイルへコミットを記録している。 + 項目ごとに 1 コミットへまとめ、状態ファイルへコミットを記録している。 """ path, state = _load(args.id) entry = _round(state, args.round) @@ -2180,6 +2254,7 @@ def _verify_fix_commits( """ problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) + seen: dict[str, set[str]] = {} # item_id -> 実在するコミットの集合 for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, scope) @@ -2187,7 +2262,16 @@ def _verify_fix_commits( problems.append(problem) info(f"❌ 修正コミットが手順を満たしていません: {problem}") continue + seen.setdefault(item_id, set()).add(commit["sha"]) accepted.append((item_id, commit["sha"])) + + # 粒度は 1 件ずつの検証が済んでから見る。壊れたコミットの理由を + # 粒度の失敗で覆い隠さない。 + for item_id, shas in seen.items(): + problem = verify_commit_granularity({"item_id": item_id}, len(shas)) + if problem: + problems.append(problem) + info(f"❌ 修正コミットが手順を満たしていません: {problem}") return problems, accepted @@ -2379,6 +2463,96 @@ def cmd_status(args: argparse.Namespace) -> None: print(_round_table(state)) +def default_plan_file(pr: int) -> str: + """改修計画を書き出す既定のパス。""" + return f"{DEFAULT_PLAN_DIR}/refactoring-plan-rf{pr}.md" + + +def normalize_plan_file(value: Optional[str]) -> str: + """改修計画の書き出し先を検証して正規化する。空文字は「記録しない」。 + + **作業ディレクトリの外へ書かせない。** 進行側は利用者のリポジトリを触るので、 + 絶対パスと親へ抜ける経路は受け取った時点で拒む。 + + 正規化するのは、判定に使うパスを git の出力と揃えるためでもある。 + `./issues/plan.md` のまま持つと、`git status` が返す `issues/plan.md` と + 一致せず、公開のコミットメッセージが取り違えられる。 + """ + rel = str(value or "").strip() + if not rel: + return "" + if os.path.isabs(rel) or (len(rel) > 1 and rel[1] == ":"): + die(f"--plan-file には相対パスを指定してください: {rel}", code=4) + normalized = os.path.normpath(rel) + if normalized == ".." or normalized.startswith(".." + os.sep): + die( + f"--plan-file が作業ディレクトリの外を指しています: {rel}", + code=4, + ) + return normalized + + +def format_plan(state: dict[str, Any]) -> str: + """改修計画の本文を組み立てる。**同じ状態からは同じ本文が出る。** + + 提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から + 除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画 + だったのかも見えない。ここで差分の中へ置く。 + """ + baseline = state.get("baseline_test") or {} + lines = [ + f"# 改修計画 — {state['repo']} #{state['current_pr']}", + "", + "`/ndf:cross-refactoring` が提案し、適用した改善項目の記録である。", + "理由と手順は提案の時点でしか残らないため、公開の直前に書き出している。", + "", + f"- 対象範囲: {', '.join(state.get('target_scope') or []) or '（未指定）'}", + f"- 着手前のテスト: {baseline.get('command') or '（未指定）'}", + "", + ] + for entry in state.get("rounds") or []: + lines.extend(_plan_round_section(state, entry)) + if not (state.get("rounds") or []): + lines.append("（改善項目なし）") + return "\n".join(lines).rstrip() + "\n" + + +def _plan_round_section(state: dict[str, Any], entry: dict[str, Any]) -> list[str]: + """1 ラウンド分の見出しと、そのラウンドの改善項目を並べる。""" + reviewers = " / ".join(entry.get("reviewers") or []) or "—" + lines = [ + f"## ラウンド {entry['round']}" + f"（実装 {entry.get('impl', '—')} / レビュー {reviewers}）", + "", + ] + items = [i for i in state.get("items") or [] if i.get("round") == entry["round"]] + if not items: + lines.extend(["（採用した改善項目なし）", ""]) + return lines + for item in items: + lines.extend(_plan_item_section(item)) + return lines + + +def _plan_item_section(item: dict[str, Any]) -> list[str]: + """改善項目 1 件の見出し・要約表・理由・手順。""" + status = ITEM_STATUS_LABELS.get(item.get("status"), item.get("status") or "—") + return [ + f"### {item['item_id']} — `{item['path']}#{item['symbol']}`", + "", + "| スメル | 手法 | 重要度 | 提案元 | 状態 | コミット |", + "| --- | --- | --- | --- | --- | ---: |", + f"| {item['smell']} | {item['technique']} | {item['severity']} | " + f"{' / '.join(item.get('proposed_by') or []) or '—'} | {status} | " + f"{len(item.get('commits') or [])} |", + "", + f"**なぜ**: {item.get('rationale') or '（記録なし）'}", + "", + f"**手順**: {item.get('plan') or '（記録なし）'}", + "", + ] + + def cmd_report(args: argparse.Namespace) -> None: """Step 8 — ラウンド表・項目表・見送り項目・指標を出す。""" _, state = _load(args.id) @@ -3206,7 +3380,31 @@ def _run_sync_command(state: dict[str, Any], work: str, command: str) -> None: ) -def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: +def _write_plan_file(state: dict[str, Any], work: str, rel: str) -> None: + """改修計画を作業ディレクトリの中へ書き出す。 + + 内容は状態から決まるので、**状態が動いていなければ差分は出ない**。 + 書き出しを毎回行っても、余計なコミットは積まれない。 + """ + path = pathlib.Path(work) / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(format_plan(state), encoding="utf-8") + + +def _publish_commit_message(produced: list[str], plan_rel: str) -> str: + """公開の直前に積むコミットのメッセージを、中身に合わせて選ぶ。""" + has_plan = bool(plan_rel) and plan_rel in produced + has_generated = any(p != plan_rel for p in produced) + if has_plan and has_generated: + return SYNC_AND_PLAN_COMMIT_MESSAGE + if has_plan: + return PLAN_COMMIT_MESSAGE + return SYNC_COMMIT_MESSAGE + + +def _commit_sync_changes( + work: str, command: str, produced: list[str], plan_rel: str = "" +) -> None: """同期が作った差分を進行側のコミットとして積む。差分が無ければ何もしない。 このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 @@ -3221,11 +3419,15 @@ def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: # 確認済みだからである。 try: _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + _sh(["git", "commit", "-m", _publish_commit_message(produced, plan_rel)], + cwd=work) except SystemExit: _discard_worktree_changes(work) raise - info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + if command: + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + else: + info(f"📝 改修計画を記録しました（{len(produced)} ファイル）") def _sync_generated(state: dict[str, Any]) -> None: @@ -3243,14 +3445,22 @@ def _sync_generated(state: dict[str, Any]) -> None: 利用者のリポジトリの検査を壊したまま進むことになる。 """ command = str(state.get("sync_command") or "").strip() - if not command: + # 状態ファイルの値も受け取った時点と同じ基準で通す。旧い状態ファイルや + # 手で書き換えられた値でも、作業ディレクトリの外へは書き出さない。 + plan_rel = normalize_plan_file(state.get("plan_file")) + if not command and not plan_rel: return work = state["worktrees"]["work"] # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 # 同期が作った差分と元からあった差分を区別できない。 _require_clean_worktree(state, work) - _run_sync_command(state, work, command) - _commit_sync_changes(work, command, _dirty_paths(state, work)) + # 改修計画も生成物と同じ経路に乗せる。**別のコミットに分けない。** + # 分けると、進行側のコミットが公開のたびに 2 つずつ積まれる。 + if plan_rel: + _write_plan_file(state, work, plan_rel) + if command: + _run_sync_command(state, work, command) + _commit_sync_changes(work, command, _dirty_paths(state, work), plan_rel) def _push_head(state: dict[str, Any]) -> None: @@ -3398,6 +3608,12 @@ def main() -> None: help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" "差分があれば進行側のコミットとして積む。" "同期を実装担当にさせると範囲外の変更になるため分離している") + init.add_argument("--plan-file", default=None, + help="改修計画を書き出すパス（対象リポジトリからの相対）。" + "提案の理由と手順は状態ファイルにしか残らず、差分から" + "除外されるため、公開の直前に進行側が書き出す。" + f"既定は {DEFAULT_PLAN_DIR}/refactoring-plan-rf.md。" + "空文字を渡すと記録しない") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_commit_granularity.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_commit_granularity.py new file mode 100644 index 0000000..aeec6f7 --- /dev/null +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_commit_granularity.py @@ -0,0 +1,125 @@ +"""コミットの粒度（1 改善項目 = 1 コミット）のテスト。 + +手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。 +**残すのは項目単位の 1 コミットだけ**にして、Pull Request を読む側が +改善項目と履歴を 1 対 1 で辿れるようにする。 + +現状固定テストが要る項目（`test_gap`）だけは、テストと実装を混ぜないために +2 コミットを許す。 +""" +from __future__ import annotations + +import pytest + + +def trailers(item_id="R1-001", round_no="1", runtime="codex", model="gpt-5.5"): + return { + "Item-Id": item_id, "Round": round_no, + "Impl-Runtime": runtime, "Impl-Model": model, + } + + +def fact(sha="abc1234", **over): + base = { + "sha": sha, "exists": True, "test_status": "pass", + "touches_tests": False, "diff_lines": 30, "trailers": trailers(), + } + base.update(over) + return base + + +def item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 400, "proposed_by": ["codex"], + "status": "pending", "commits": [], + } + base.update(over) + return base + + +# ---------- 適用フェーズ ---------- + +def test_one_commit_per_item_passes(refactor): + assert refactor.verify_apply_item(item(), [fact()]) is None + + +def test_two_commits_for_one_item_fails(refactor): + """途中経過を刻むと、改善項目と履歴が 1 対 1 で対応しなくなる。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb")] + ) + assert problem is not None and "1 コミット" in problem + + +def test_the_granularity_message_names_the_item(refactor): + """どの項目が刻みすぎたのかを読めるようにする。""" + problem = refactor.verify_apply_item( + item(item_id="R2-003"), [fact(sha="aaa", trailers=trailers(item_id="R2-003")), + fact(sha="bbb", trailers=trailers(item_id="R2-003"))] + ) + assert "R2-003" in problem + + +def test_test_gap_allows_the_characterization_test_commit(refactor): + """テストと実装を 1 コミットへ混ぜないため、この項目だけ 2 コミットを許す。""" + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb")] + assert refactor.verify_apply_item(item(test_gap=True), facts) is None + + +def test_test_gap_still_rejects_three_commits(refactor): + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb"), fact(sha="ccc")] + problem = refactor.verify_apply_item(item(test_gap=True), facts) + assert problem is not None and "2 コミット" in problem + + +def test_a_broken_commit_is_reported_before_the_granularity(refactor): + """粒度は最後に見る。トレーラーやテストの問題を粒度で覆い隠さない。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb", test_status="fail")] + ) + assert problem is not None and "テストが成功していません" in problem + + +def test_the_budget_is_reported_before_the_granularity(refactor): + """差分予算の超過は原因が別なので、粒度より先に伝える。""" + problem = refactor.verify_apply_item( + item(technique="rename", estimated_diff_lines=10), + [fact(sha="aaa", diff_lines=50), fact(sha="bbb", diff_lines=50)], + ) + assert problem is not None and "差分予算" in problem + + +# ---------- 修正フェーズ ---------- + +def test_fix_accepts_one_commit_per_item(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-002"))] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] + assert accepted == [("R1-001", "aaa"), ("R1-002", "bbb")] + + +def test_fix_rejects_two_commits_for_the_same_item(refactor): + """適用側だけ揃えると、指摘への対応という名目で刻んだ履歴が戻ってくる。""" + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert problems and any("R1-001" in p and "1 コミット" in p for p in problems) + + +def test_fix_granularity_does_not_hide_a_broken_commit(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", test_status="fail", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert any("テストが成功していません" in p for p in problems) + + +@pytest.mark.parametrize("count", [1, 2, 3]) +def test_fix_allows_one_commit_for_each_distinct_item(refactor, count): + facts = [fact(sha=f"s{i}", trailers=trailers(item_id=f"R1-00{i}")) + for i in range(count)] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] and len(accepted) == count diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_init.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_init.py index 5667ae3..d5fbf92 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_init.py @@ -59,7 +59,7 @@ def _args(tmp_path, **over): "pr": 130, "scope": ["src"], "host": "claude", "max_outer_rounds": 3, "max_fix_rounds": 3, "max_items_per_round": 5, "severity_threshold": "minor", "model": None, "baseline_test": "true", - "sync_command": None, + "sync_command": None, "plan_file": None, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } @@ -429,3 +429,25 @@ def test_init_fills_the_posting_event_when_resuming_an_old_state(run_init, tmp_p assert resumed["is_own_pr"] is True assert resumed["event_downgrade"] is True assert "COMMENT" in resumed["review_post_note"] + + +# ---------- 改修計画の書き出し先 ---------- + +def test_init_records_the_default_plan_file(run_init, tmp_path): + """指定が無くても計画を残す。**既定で残らないと、誰も指定しない。**""" + run_init(_args(tmp_path)) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "issues/refactoring-plan-rf130.md" + + +def test_init_keeps_an_explicit_plan_file(run_init, tmp_path): + run_init(_args(tmp_path, plan_file="docs/plan.md")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "docs/plan.md" + + +def test_init_accepts_an_empty_plan_file_as_off(run_init, tmp_path): + """計画を差分へ入れたくないリポジトリのために、空文字で無効にできる。""" + run_init(_args(tmp_path, plan_file="")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "" diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_merge_apply.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_merge_apply.py index fa29964..d341a7f 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/tests/test_merge_apply.py +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_merge_apply.py @@ -76,7 +76,7 @@ def test_commit_that_does_not_exist_fails(refactor): assert problem is not None and "範囲にありません" in problem -# ---------- 1 手 1 コミット ---------- +# ---------- 1 改善項目 = 1 コミット ---------- def test_zero_commits_fails(refactor): problem = refactor.verify_apply_item(item(), []) diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_plan_file.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_plan_file.py new file mode 100644 index 0000000..fe6c508 --- /dev/null +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_plan_file.py @@ -0,0 +1,233 @@ +"""改修計画をリポジトリ内のファイルへ残すテスト。 + +提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から除外される。 +**Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない。** +計画を差分の中へ置き、公開は生成物の同期と同じ経路（進行側の 1 コミット）に乗せる。 +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + + +def _make_work(tmp_path): + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "1 関数が 6 段の処理を通しで行っている", + "plan": "1. 範囲の確定を切り出す 2. 検証を切り出す", + "test_gap": False, "estimated_diff_lines": 40, + "proposed_by": ["codex", "gemini"], "status": "done", "commits": ["abc1234"], + } + base.update(over) + return base + + +def _state(tmp_path, work=None, **over): + rounds = over.pop("rounds", [{ + "round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0, + }]) + items = over.pop("items", [_item()]) + worktrees = {"work": str(work or tmp_path / "work")} + for r in ("codex", "gemini", "kiro"): + worktrees[r] = str(tmp_path / r) + path = make_state(tmp_path, rounds=rounds, items=items, + worktrees=worktrees, **over) + return path, read_state(path) + + +# ---------- 計画の本文 ---------- + +def test_plan_names_the_item_and_the_target(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "R1-001" in text and "src/foo.py" in text and "Foo.handle" in text + + +def test_plan_carries_the_reason_and_the_steps(refactor, tmp_path): + """なぜ直すのか・どう直すのかは、提案の時点でしか残らない。""" + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "1 関数が 6 段の処理を通しで行っている" in text + assert "1. 範囲の確定を切り出す" in text + + +def test_plan_shows_the_smell_and_the_technique(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "long_method" in text and "extract_method" in text + + +def test_plan_records_who_proposed_it(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_marks_an_abandoned_item(refactor, tmp_path): + """取り消した項目も残す。同じ提案が再び来たときの判断材料になる。""" + _, state = _state(tmp_path, items=[_item(status="abandoned", commits=[])]) + text = refactor.format_plan(state) + assert "取り消し" in text + + +def test_plan_groups_items_by_round(refactor, tmp_path): + rounds = [ + {"round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0}, + {"round": 2, "impl": "kiro", "reviewers": ["codex", "gemini"], + "items": ["R2-001"], "reviews": [], "fix_rounds": 0}, + ] + items = [_item(), _item(item_id="R2-001", round=2)] + _, state = _state(tmp_path, rounds=rounds, items=items) + text = refactor.format_plan(state) + assert text.index("ラウンド 1") < text.index("ラウンド 2") + + +def test_plan_names_the_implementer_of_each_round(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_is_stable_for_the_same_state(refactor, tmp_path): + """同じ状態からは同じ本文が出る。差分が出続けると毎回コミットが積まれる。""" + _, state = _state(tmp_path) + assert refactor.format_plan(state) == refactor.format_plan(state) + + +# ---------- 置き場所 ---------- + +def test_the_default_plan_file_lives_under_issues(refactor): + assert refactor.default_plan_file(136) == "issues/refactoring-plan-rf136.md" + + +def test_the_plan_file_is_written_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), "issues/plan.md") + + written = (work / "issues" / "plan.md").read_text(encoding="utf-8") + assert "R1-001" in written + + +# ---------- 公開 ---------- + +def test_the_plan_lands_in_one_commit_with_the_generated_files(refactor, tmp_path): + """計画書と生成物で 2 コミットに分けない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command="printf 'x = 2\\n' > generated/out.py") + + refactor._sync_generated(state) + + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files and "generated/out.py" in files + assert subject and "cross-refactoring" in subject + + +def test_a_repository_without_a_sync_command_still_records_the_plan( + refactor, tmp_path +): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + + refactor._sync_generated(state) + + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files + + +def test_an_unchanged_plan_does_not_add_a_commit(refactor, tmp_path): + """状態が動いていないのにコミットを積まない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + refactor._sync_generated(state) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +def test_an_empty_plan_file_setting_turns_the_record_off(refactor, tmp_path): + """計画を差分へ入れたくないリポジトリのために、無効にできる。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="", sync_command=None) + + refactor._sync_generated(state) + + assert not (work / "issues").exists() + + +# ---------- 書き出し先の検証 ---------- + +def test_a_relative_path_is_kept(refactor): + assert refactor.normalize_plan_file("issues/plan.md") == "issues/plan.md" + + +def test_a_leading_dot_is_normalized(refactor): + """`./issues/plan.md` は git が返すパスと一致しない。正規化して揃える。""" + assert refactor.normalize_plan_file("./issues/plan.md") == "issues/plan.md" + + +def test_an_empty_value_stays_empty(refactor): + assert refactor.normalize_plan_file("") == "" + assert refactor.normalize_plan_file(None) == "" + + +def test_an_absolute_path_is_refused(refactor): + """作業ディレクトリの外へ書かせない。進行側は利用者のリポジトリを触る。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("/tmp/out.md") + + +def test_a_parent_traversal_is_refused(refactor): + with pytest.raises(SystemExit): + refactor.normalize_plan_file("../out.md") + + +def test_a_traversal_in_the_middle_is_refused(refactor): + """途中で外へ出る経路も拒む。正規化してから判定する。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("issues/../../out.md") + + +def test_the_written_path_stays_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), state["plan_file"]) + + assert (work / "issues" / "plan.md").exists() + assert not (tmp_path / "plan.md").exists() diff --git a/plugins/ndf-kiro/README.md b/plugins/ndf-kiro/README.md index 4e0591e..a994ccf 100644 --- a/plugins/ndf-kiro/README.md +++ b/plugins/ndf-kiro/README.md @@ -12,7 +12,7 @@ cat plugins/ndf-kiro/VERSION # 導入済みプロジェクトの版数 python3 -c "import json;print(json.load(open('.kiro/agents/ndf.json'))['description'])" -# => NDF統合開発エージェント（Kiro CLI用 / v8.5.4） +# => NDF統合開発エージェント（Kiro CLI用 / v8.6.0） ``` `install.sh` は実行時にも `NDF バージョン: <版数>` を表示する。 diff --git a/plugins/ndf-kiro/VERSION b/plugins/ndf-kiro/VERSION index 078e20f..acd405b 100644 --- a/plugins/ndf-kiro/VERSION +++ b/plugins/ndf-kiro/VERSION @@ -1 +1 @@ -8.5.4 +8.6.0 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md b/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md index 03bc988..04d714d 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md @@ -37,6 +37,8 @@ allowed-tools: | 役割の分離 | 提案・レビューは**ホストを除く 3 者**、適用は**gemini を除く 3 者**。両者は重なるが一致しない | | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | +| コミットの単位 | **1 改善項目 = 1 コミット。** テストも項目の単位で 1 回だけ求める（現状固定テストが要る項目のみ 2 コミット） | +| 改修計画 | **差分の中へ残す。** 理由と手順は提案の時点でしか残らない。公開の直前に進行側が書き出し、生成物の同期と同じコミットへ入れる | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | | 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | | 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | @@ -61,6 +63,7 @@ allowed-tools: | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | | `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | +| `--plan-file PATH` | 改修計画の書き出し先（**対象リポジトリからの相対パス**）。空文字を渡すと記録しない | `issues/refactoring-plan-rf.md` | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" @@ -128,7 +131,7 @@ flowchart TD Merge --> Empty{"採用件数 = 0 ?"} Empty -->|はい| Final([提案ラウンドの繰り返しを終了]):::ok Empty -->|いいえ| Apply - Apply["Step 4: 適用（実装担当 1 CLI）
項目ごとに 1 手 1 コミット"] + Apply["Step 4: 適用（実装担当 1 CLI）
1 改善項目 = 1 コミット"] Apply --> Review["Step 5: レビュー（2 CLI 並列）
ラウンドの差分をまとめて 1 回"] Review --> Judge{"2 者とも承認 ?"} Judge -->|いいえ| Fix["Step 6: 指摘修正（実装担当）"] @@ -275,6 +278,9 @@ done | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | +| 1 項目を複数のコミットへ刻む | 改善項目と履歴が 1 対 1 で辿れなくなり、取り消しと積み直しのコミットも件数に比例して増える | +| 実装担当に手ごとのテストを義務づける | 進行側もコミットごとに回すため、テストの実行回数が手数の 2 倍になる（実測 44 手で 88 回） | +| 改修計画を状態ファイルにだけ残す | 状態ファイルは差分から除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない | | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | 投稿に失敗したまま結果ファイルを書かずに終了する | 進行側からは「レビュー担当が動かなかった」と区別が付かない。失敗したときほど `post_error` 付きの結果ファイルが要る | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | diff --git a/plugins/ndf-kiro/skills/cross-refactoring/docs/01-state-and-propose.md b/plugins/ndf-kiro/skills/cross-refactoring/docs/01-state-and-propose.md index fb75178..e2a06f1 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/docs/01-state-and-propose.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/docs/01-state-and-propose.md @@ -46,10 +46,14 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 +7. **改修計画の書き出し先の記録** — `--plan-file` を状態ファイルへ保存する。 + 既定は `issues/refactoring-plan-rf.md` で、空文字を渡すと記録しない。 + **既定で残す**のは、指定できるだけでは誰も指定しないためである。書き出しは + 初期化時ではなく、生成物の同期と同じく**push の直前**に行う +8. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） -8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +9. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない @@ -105,7 +109,7 @@ gemini は NDF の配布先ではないため「標準の配置先」を持た | `quality-gates` | 「直し終わった」と言える条件の判定 | `ndf-policies` は**配置しない**。git 運用やコミット規約といったリポジトリ運用の方針で -あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 手 1 コミット、 +あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 改善項目 = 1 コミット、 コミットトレーラー、`--force` 禁止など）はプロンプト側で明示する。 ### 守ること diff --git a/plugins/ndf-kiro/skills/cross-refactoring/docs/02-apply-and-review.md b/plugins/ndf-kiro/skills/cross-refactoring/docs/02-apply-and-review.md index c311658..f9bbf23 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/docs/02-apply-and-review.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/docs/02-apply-and-review.md @@ -39,6 +39,7 @@ | 各コミットでテストが成功している | 実際の実行 | コミットを取り出して着手前と同じテストを走らせる。**JSON の `test_status` は読まない**。`--test-timeout`（既定 900 秒）を超えたら失敗 | | テストが無い経路は先に現状固定テスト | `git show --name-only` | `test_gap` が真の項目は、先頭コミットがテストの置き場所を触っている | | 項目の分離 | git のトレーラー | 各コミットの `Item-Id` がその項目と一致する。複数の項目を 1 コミットにまとめたら失敗 | +| コミットの粒度 | 申告されたコミットの実数 | 1 改善項目 = 1 コミット。`test_gap` が真の項目だけ 2 コミット | | 差分予算 | `git show --numstat` | 実差分の合計が `estimated_diff_lines` の 2 倍（抽出系の手法は 3 倍）を超えたら失敗（範囲の逸脱） | | 対象範囲の遵守 | `git show --name-only` | 触ったファイルが全て `--scope` の中にある。1 つでも外なら失敗 | | 機能変更の混入なし | — | 機械判定は不可能。レビュー観点に委ねる | @@ -74,6 +75,59 @@ 真のときの現状固定テストを数えることを明記した。倍率だけを広げると、見積が 楽観側へ倒れたぶんまで通してしまう。 +#### 1 改善項目 = 1 コミットにする + +**手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** 手順は +順に適用してよいが、残すのは項目単位の 1 コミットだけにする。 + +刻んだままだと 3 つの読みにくさが出る。 + +| 影響 | 内容 | +| --- | --- | +| 履歴 | Pull Request を読む側が、改善項目と履歴を 1 対 1 で辿れない | +| 取り消し | 範囲を戻して積み直すコミットが、刻んだ件数に比例して増える | +| 検証 | コミットごとにテストを実行するため、実行回数も比例して増える | + +実測では採用 12 件に対して適用が 34 コミット、取り消しと積み直しで 25 コミットだった。 + +**テストの回数も項目の単位に合わせる。** 進行側は申告されたコミットごとにテストを +実行するので、実装担当にも手ごとの実行を義務づけると、同じテストが手数の 2 倍だけ +走る。実測では 44 手に対して 88 回（1 回 26 秒として約 38 分）だった。 + +| 誰が | いつ | 6 回目の実測 | 項目単位にした後 | +| --- | --- | ---: | ---: | +| 実装担当 | コミットの前に 1 回 | 44 | 14 | +| 進行側 | 申告されたコミットごと | 44 | 14 | + +実装担当が途中で確かめる分には止めない。求めるのは**コミットの前に通っていること** +だけで、そこは進行側が実際に実行して確かめる。 + +例外は現状固定テストが要る項目（`test_gap` が真）だけで、「テスト → 実装」の +2 コミットを許す。1 つに混ぜると、テストが先行したことを履歴から確かめられない。 + +プロンプトは、途中で刻みたいときの戻し方も渡す。**その項目に着手する前の HEAD**を +控えておき、最後に `git reset --soft` で 1 コミットへまとめる。控えた地点より前へ +戻すと他の項目のコミットを巻き込むため、起点は項目の着手前に固定する。 + +#### 改修計画を差分へ残す + +**なぜ直すのか（理由）とどう直すのか（手順）は、提案の時点でしか残らない。** +状態ファイルには入っているが、そのディレクトリは差分から除外されるため、 +Pull Request を読む側からは見えない。 + +そこで `--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出す。 + +- 内容は**状態から決まる**。状態が動いていなければ差分は出ないので、公開のたびに + 書き出しても余計なコミットは積まれない +- 取り消した項目も残す。同じ提案が次のラウンドで来たときの判断材料になる +- 公開は生成物の同期と**同じコミット**に乗せる。分けると進行側のコミットが + 公開のたびに 2 つずつ積まれる +- 空文字を渡すと記録しない。差分へ入れたくないリポジトリのための逃げ道である +- **絶対パスと親へ抜ける経路は受け取った時点で拒む**（終了コード 4）。進行側は利用者の + リポジトリを触るため、作業ディレクトリの外へ書き出す余地を残さない。あわせて + `./issues/plan.md` のような表記も正規化する。git が返すパスと形が違うと、公開の + コミットメッセージが取り違えられる + #### 範囲の指定は検証にも効かせる `--scope` を必須にした目的は**提案の発散と変更の肥大を防ぐ**ことなので、指定を検証へ @@ -312,7 +366,7 @@ claude が参加する構成では実際のリファクタリング 1 件で 1.4 | --- | --- | | 項目ごとに実装者が入れ替わらない | 輪番の単位がラウンドなので、重ねれば実装者は分散する | | 指摘がどの項目に対するものか曖昧になる | 指摘に改善項目 ID を**必須**とし、未知の ID と欠落は差し戻す | -| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 手 1 コミットを保ち、**取り消しは項目単位**で行う | +| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 コミットを保ち、**取り消しは項目単位**で行う | ### 判定 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/docs/03-review-viewpoints.md b/plugins/ndf-kiro/skills/cross-refactoring/docs/03-review-viewpoints.md index d607ea9..536799f 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/docs/03-review-viewpoints.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/docs/03-review-viewpoints.md @@ -14,7 +14,7 @@ | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | ## 振る舞い不変を筆頭に置く理由 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md index ec9c8b9..f42ae6c 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md @@ -27,9 +27,18 @@ $RF_ITEMS 1. `test_gap` が真なら、**先に現状固定テストを追加してコミットする**。 これは省略できません。振る舞いが変わっていないことを示す手段が無いまま 構造を変えるのは、構造改善ではなく単なる編集です -2. `plan` の手順を **1 手ずつ**適用する -3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** -4. 通ったらコミットする（**1 手 = 1 コミット**） +2. `plan` の手順を順に適用する +3. **手順を終えてから** `$RF_BASELINE_TEST` を **1 回**実行する。落ちたら原因の手を戻す +4. 通ったら、**その項目の変更をまとめて 1 コミットにする** + +**テストもコミットも項目の単位で 1 回です。** 手ごとに回すと、進行側の検証と +合わせて手数の 2 倍だけテストが走ります（実測では 44 手で 88 回）。手ごとに +確かめたいときは自分の判断で回して構いませんが、**求めているのはコミットの前に +通っていること**だけです。 + +途中で刻んでおきたいときは、**その項目に着手する前の HEAD を控えておき**、 +最後に `git reset --soft <控えた HEAD>` してから 1 回だけコミットしてください +（控えた HEAD より前へ戻すと、他の項目のコミットを巻き込みます）。 ## コミットの規約 @@ -47,6 +56,10 @@ Impl-Runtime: $RF_RUNTIME Impl-Model: $RF_MODEL ``` +- **1 改善項目 = 1 コミット。** 2 件以上に刻むと、その項目は失敗として扱われ、 + 取り消されます。例外は `test_gap` が真の項目だけで、「現状固定テスト → 実装」の + 2 コミットを許します（テストと実装を混ぜると、テストが先行したことを + 履歴から確かめられません） - **`Item-Id` は必ずその項目のものにする。** 複数の項目を 1 コミットへまとめると、 取り消し範囲が項目単位で決まらなくなり、失敗として扱われます - `Impl-Model` には**実際に使ったモデル名**を書く。分からなければ `default` diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md index bb2180a..dd3bf4b 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md @@ -24,7 +24,8 @@ $RF_ITEMS 1. `gh api` で Pull Request の**未解決レビュースレッド**を取得する 2. 各指摘について、**修正するか・しないか**を決める - - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする + - 修正する: 直してから `$RF_BASELINE_TEST` を 1 回実行し、**その改善項目の + 修正を 1 コミットにまとめる** - 修正しない: 根拠を返信する。**黙って閉じない** 3. 対応したスレッドに返信し、`resolveReviewThread` で解決する @@ -38,6 +39,11 @@ $RF_ITEMS いること**が必要です。満たさないコミットは取り込まれず、その改善項目の指摘は 解決済みになりません（修正ラウンドの上限に達すると項目ごと取り消されます）。 +**1 改善項目 = 1 コミット。** 同じ項目のコミットが 2 件以上あると、その修正 +ラウンドの範囲ごと取り消されます。適用側だけ揃えても、指摘への対応という名目で +刻んだ履歴が戻ってくるためです。複数の項目に指摘が付いているときは、項目ごとに +1 コミットへ分けてください。 + 判定はオーケストレータが **git と実際のテスト実行**から行います。結果ファイルに 何と書いても検査結果は変わりません。 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/review.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/review.md index ed083c9..5c8144d 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/review.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/review.md @@ -31,7 +31,7 @@ $RF_ITEMS | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | **振る舞い不変を筆頭に置く。** 疑わしければ実際に `$RF_BASELINE_TEST` を実行して diff --git a/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py index 685de46..87b018b 100755 --- a/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py @@ -145,6 +145,33 @@ def vocabulary() -> dict[str, Any]: "公開の直前に進行側がまとめて生成する。" ) +# 計画と生成物を 1 つのコミットへまとめたときのメッセージ。 +SYNC_AND_PLAN_COMMIT_MESSAGE = ( + "Chore: 生成物と改修計画を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "改修計画は提案の時点でしか残らないため、公開の直前に書き出す。\n" + "どちらも進行側の責務なので、1 つのコミットにまとめる。" +) + +# 改修計画だけを記録したコミットのメッセージ。 +PLAN_COMMIT_MESSAGE = ( + "Docs: 改修計画を記録する（cross-refactoring 進行側）\n\n" + "なぜ直すのか（理由）とどう直すのか（手順）は提案の時点でしか残らない。\n" + "状態ファイルは差分から除外されるため、Pull Request から読める場所へ置く。" +) + +# 改修計画を書き出す既定のディレクトリ。 +DEFAULT_PLAN_DIR = "issues" + +# 改善項目の状態を、Pull Request を読む側に通じる語へ置き換える。 +ITEM_STATUS_LABELS = { + "pending": "未着手", + "reviewing": "レビュー中", + "done": "採用", + "abandoned": "取り消し", + "blocked": "着手せず", +} + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -168,6 +195,19 @@ def vocabulary() -> dict[str, Any]: }) EXTRACTION_DIFF_BUDGET_FACTOR = 3 +# 1 改善項目が履歴に残せるコミット数。 +# +# **手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** +# 手ごとにテストを回して安全に進めるのは変わらないが、残すのは項目単位の +# 1 コミットだけにする。刻んだままだと Pull Request を読む側が改善項目と履歴を +# 1 対 1 で辿れず、取り消しと積み直しのコミットも件数に比例して増える +# （実測: 採用 12 件に対して適用 34 コミット、取り消しと積み直しで 25 コミット）。 +MAX_COMMITS_PER_ITEM = 1 + +# 現状固定テストが要る項目だけは 2 コミットを許す。テストと実装を 1 コミットへ +# 混ぜると、「テストを先に足した」ことを履歴から確かめられなくなる。 +MAX_COMMITS_PER_ITEM_WITH_TEST_GAP = 2 + # テスト 1 回あたりの上限（秒）。生成されたコードやテストが無限ループに入ると、 # 待ち続けて**進行全体が止まる**。打ち切って失敗として扱う。 DEFAULT_TEST_TIMEOUT = 900 @@ -579,7 +619,7 @@ def verify_apply_item( 確かめられる**。読ませ方の不確実性に対する最後の砦としてここを厚くする。 """ if not facts: - return "コミットが 1 件もありません（1 手 1 コミットの前提を満たしていません）" + return "コミットが 1 件もありません（1 改善項目 = 1 コミットの前提を満たしていません）" for commit in facts: problem = _verify_commit_basics( @@ -617,9 +657,38 @@ def verify_apply_item( f"実差分 {actual} 行が差分予算 {budget} 行" f"（見積 {estimated} 行 × {factor}）を超えました（範囲の逸脱）" ) + + # 粒度は最後に見る。トレーラーやテストの問題を粒度の失敗で覆い隠さない。 + # 数えるのは**実在するコミットの数**である。同じコミットを重ねて申告しただけの + # ときに落とすと、食い違いの無い申告を刻みすぎとして扱ってしまう。 + problem = verify_commit_granularity(item, len({c.get("sha") for c in facts})) + if problem: + return problem return None +def commit_limit_for(item: dict[str, Any]) -> int: + """その項目が履歴に残せるコミット数。""" + if item.get("test_gap"): + return MAX_COMMITS_PER_ITEM_WITH_TEST_GAP + return MAX_COMMITS_PER_ITEM + + +def verify_commit_granularity(item: dict[str, Any], count: int) -> Optional[str]: + """項目のコミット数が上限に収まっているか。超えていれば理由を返す。 + + 適用（`verify_apply_item`）と修正（`_verify_fix_commits`）で**同じ基準**を使う。 + 適用側だけ揃えると、レビュー指摘への対応という名目で刻んだ履歴が戻ってくる。 + """ + limit = commit_limit_for(item) + if count <= limit: + return None + return ( + f"項目 {item['item_id']} のコミットが {count} 件あります" + f"（残すのは 1 項目 = 1 コミット。現状固定テストが要る項目だけ 2 コミットまで）" + ) + + # ---------------- レビュー判定 ---------------- REVIEW_URL_MARKER = "#pullrequestreview-" @@ -1011,6 +1080,11 @@ def cmd_init(args: argparse.Namespace) -> None: "baseline_test": baseline, # 生成物の同期は**進行側の責務**。push の直前に実行する。 "sync_command": args.sync_command, + # 改修計画の書き出し先も同じ経路に乗せる。指定が無ければ既定のパスを使い、 + # 空文字なら記録しない。 + "plan_file": normalize_plan_file( + default_plan_file(args.pr) if args.plan_file is None else args.plan_file + ), "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -2018,7 +2092,7 @@ def cmd_abandon_items(args: argparse.Namespace) -> None: """Step 6 — 未解決の指摘に紐づく改善項目だけを取り消す。 **合意済みの項目は Pull Request に残す。** これを可能にするために、適用は - 項目ごとに 1 手 1 コミットへ分け、状態ファイルへコミットを記録している。 + 項目ごとに 1 コミットへまとめ、状態ファイルへコミットを記録している。 """ path, state = _load(args.id) entry = _round(state, args.round) @@ -2180,6 +2254,7 @@ def _verify_fix_commits( """ problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) + seen: dict[str, set[str]] = {} # item_id -> 実在するコミットの集合 for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, scope) @@ -2187,7 +2262,16 @@ def _verify_fix_commits( problems.append(problem) info(f"❌ 修正コミットが手順を満たしていません: {problem}") continue + seen.setdefault(item_id, set()).add(commit["sha"]) accepted.append((item_id, commit["sha"])) + + # 粒度は 1 件ずつの検証が済んでから見る。壊れたコミットの理由を + # 粒度の失敗で覆い隠さない。 + for item_id, shas in seen.items(): + problem = verify_commit_granularity({"item_id": item_id}, len(shas)) + if problem: + problems.append(problem) + info(f"❌ 修正コミットが手順を満たしていません: {problem}") return problems, accepted @@ -2379,6 +2463,96 @@ def cmd_status(args: argparse.Namespace) -> None: print(_round_table(state)) +def default_plan_file(pr: int) -> str: + """改修計画を書き出す既定のパス。""" + return f"{DEFAULT_PLAN_DIR}/refactoring-plan-rf{pr}.md" + + +def normalize_plan_file(value: Optional[str]) -> str: + """改修計画の書き出し先を検証して正規化する。空文字は「記録しない」。 + + **作業ディレクトリの外へ書かせない。** 進行側は利用者のリポジトリを触るので、 + 絶対パスと親へ抜ける経路は受け取った時点で拒む。 + + 正規化するのは、判定に使うパスを git の出力と揃えるためでもある。 + `./issues/plan.md` のまま持つと、`git status` が返す `issues/plan.md` と + 一致せず、公開のコミットメッセージが取り違えられる。 + """ + rel = str(value or "").strip() + if not rel: + return "" + if os.path.isabs(rel) or (len(rel) > 1 and rel[1] == ":"): + die(f"--plan-file には相対パスを指定してください: {rel}", code=4) + normalized = os.path.normpath(rel) + if normalized == ".." or normalized.startswith(".." + os.sep): + die( + f"--plan-file が作業ディレクトリの外を指しています: {rel}", + code=4, + ) + return normalized + + +def format_plan(state: dict[str, Any]) -> str: + """改修計画の本文を組み立てる。**同じ状態からは同じ本文が出る。** + + 提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から + 除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画 + だったのかも見えない。ここで差分の中へ置く。 + """ + baseline = state.get("baseline_test") or {} + lines = [ + f"# 改修計画 — {state['repo']} #{state['current_pr']}", + "", + "`/ndf:cross-refactoring` が提案し、適用した改善項目の記録である。", + "理由と手順は提案の時点でしか残らないため、公開の直前に書き出している。", + "", + f"- 対象範囲: {', '.join(state.get('target_scope') or []) or '（未指定）'}", + f"- 着手前のテスト: {baseline.get('command') or '（未指定）'}", + "", + ] + for entry in state.get("rounds") or []: + lines.extend(_plan_round_section(state, entry)) + if not (state.get("rounds") or []): + lines.append("（改善項目なし）") + return "\n".join(lines).rstrip() + "\n" + + +def _plan_round_section(state: dict[str, Any], entry: dict[str, Any]) -> list[str]: + """1 ラウンド分の見出しと、そのラウンドの改善項目を並べる。""" + reviewers = " / ".join(entry.get("reviewers") or []) or "—" + lines = [ + f"## ラウンド {entry['round']}" + f"（実装 {entry.get('impl', '—')} / レビュー {reviewers}）", + "", + ] + items = [i for i in state.get("items") or [] if i.get("round") == entry["round"]] + if not items: + lines.extend(["（採用した改善項目なし）", ""]) + return lines + for item in items: + lines.extend(_plan_item_section(item)) + return lines + + +def _plan_item_section(item: dict[str, Any]) -> list[str]: + """改善項目 1 件の見出し・要約表・理由・手順。""" + status = ITEM_STATUS_LABELS.get(item.get("status"), item.get("status") or "—") + return [ + f"### {item['item_id']} — `{item['path']}#{item['symbol']}`", + "", + "| スメル | 手法 | 重要度 | 提案元 | 状態 | コミット |", + "| --- | --- | --- | --- | --- | ---: |", + f"| {item['smell']} | {item['technique']} | {item['severity']} | " + f"{' / '.join(item.get('proposed_by') or []) or '—'} | {status} | " + f"{len(item.get('commits') or [])} |", + "", + f"**なぜ**: {item.get('rationale') or '（記録なし）'}", + "", + f"**手順**: {item.get('plan') or '（記録なし）'}", + "", + ] + + def cmd_report(args: argparse.Namespace) -> None: """Step 8 — ラウンド表・項目表・見送り項目・指標を出す。""" _, state = _load(args.id) @@ -3206,7 +3380,31 @@ def _run_sync_command(state: dict[str, Any], work: str, command: str) -> None: ) -def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: +def _write_plan_file(state: dict[str, Any], work: str, rel: str) -> None: + """改修計画を作業ディレクトリの中へ書き出す。 + + 内容は状態から決まるので、**状態が動いていなければ差分は出ない**。 + 書き出しを毎回行っても、余計なコミットは積まれない。 + """ + path = pathlib.Path(work) / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(format_plan(state), encoding="utf-8") + + +def _publish_commit_message(produced: list[str], plan_rel: str) -> str: + """公開の直前に積むコミットのメッセージを、中身に合わせて選ぶ。""" + has_plan = bool(plan_rel) and plan_rel in produced + has_generated = any(p != plan_rel for p in produced) + if has_plan and has_generated: + return SYNC_AND_PLAN_COMMIT_MESSAGE + if has_plan: + return PLAN_COMMIT_MESSAGE + return SYNC_COMMIT_MESSAGE + + +def _commit_sync_changes( + work: str, command: str, produced: list[str], plan_rel: str = "" +) -> None: """同期が作った差分を進行側のコミットとして積む。差分が無ければ何もしない。 このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 @@ -3221,11 +3419,15 @@ def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: # 確認済みだからである。 try: _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + _sh(["git", "commit", "-m", _publish_commit_message(produced, plan_rel)], + cwd=work) except SystemExit: _discard_worktree_changes(work) raise - info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + if command: + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + else: + info(f"📝 改修計画を記録しました（{len(produced)} ファイル）") def _sync_generated(state: dict[str, Any]) -> None: @@ -3243,14 +3445,22 @@ def _sync_generated(state: dict[str, Any]) -> None: 利用者のリポジトリの検査を壊したまま進むことになる。 """ command = str(state.get("sync_command") or "").strip() - if not command: + # 状態ファイルの値も受け取った時点と同じ基準で通す。旧い状態ファイルや + # 手で書き換えられた値でも、作業ディレクトリの外へは書き出さない。 + plan_rel = normalize_plan_file(state.get("plan_file")) + if not command and not plan_rel: return work = state["worktrees"]["work"] # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 # 同期が作った差分と元からあった差分を区別できない。 _require_clean_worktree(state, work) - _run_sync_command(state, work, command) - _commit_sync_changes(work, command, _dirty_paths(state, work)) + # 改修計画も生成物と同じ経路に乗せる。**別のコミットに分けない。** + # 分けると、進行側のコミットが公開のたびに 2 つずつ積まれる。 + if plan_rel: + _write_plan_file(state, work, plan_rel) + if command: + _run_sync_command(state, work, command) + _commit_sync_changes(work, command, _dirty_paths(state, work), plan_rel) def _push_head(state: dict[str, Any]) -> None: @@ -3398,6 +3608,12 @@ def main() -> None: help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" "差分があれば進行側のコミットとして積む。" "同期を実装担当にさせると範囲外の変更になるため分離している") + init.add_argument("--plan-file", default=None, + help="改修計画を書き出すパス（対象リポジトリからの相対）。" + "提案の理由と手順は状態ファイルにしか残らず、差分から" + "除外されるため、公開の直前に進行側が書き出す。" + f"既定は {DEFAULT_PLAN_DIR}/refactoring-plan-rf.md。" + "空文字を渡すと記録しない") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_commit_granularity.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_commit_granularity.py new file mode 100644 index 0000000..aeec6f7 --- /dev/null +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_commit_granularity.py @@ -0,0 +1,125 @@ +"""コミットの粒度（1 改善項目 = 1 コミット）のテスト。 + +手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。 +**残すのは項目単位の 1 コミットだけ**にして、Pull Request を読む側が +改善項目と履歴を 1 対 1 で辿れるようにする。 + +現状固定テストが要る項目（`test_gap`）だけは、テストと実装を混ぜないために +2 コミットを許す。 +""" +from __future__ import annotations + +import pytest + + +def trailers(item_id="R1-001", round_no="1", runtime="codex", model="gpt-5.5"): + return { + "Item-Id": item_id, "Round": round_no, + "Impl-Runtime": runtime, "Impl-Model": model, + } + + +def fact(sha="abc1234", **over): + base = { + "sha": sha, "exists": True, "test_status": "pass", + "touches_tests": False, "diff_lines": 30, "trailers": trailers(), + } + base.update(over) + return base + + +def item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 400, "proposed_by": ["codex"], + "status": "pending", "commits": [], + } + base.update(over) + return base + + +# ---------- 適用フェーズ ---------- + +def test_one_commit_per_item_passes(refactor): + assert refactor.verify_apply_item(item(), [fact()]) is None + + +def test_two_commits_for_one_item_fails(refactor): + """途中経過を刻むと、改善項目と履歴が 1 対 1 で対応しなくなる。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb")] + ) + assert problem is not None and "1 コミット" in problem + + +def test_the_granularity_message_names_the_item(refactor): + """どの項目が刻みすぎたのかを読めるようにする。""" + problem = refactor.verify_apply_item( + item(item_id="R2-003"), [fact(sha="aaa", trailers=trailers(item_id="R2-003")), + fact(sha="bbb", trailers=trailers(item_id="R2-003"))] + ) + assert "R2-003" in problem + + +def test_test_gap_allows_the_characterization_test_commit(refactor): + """テストと実装を 1 コミットへ混ぜないため、この項目だけ 2 コミットを許す。""" + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb")] + assert refactor.verify_apply_item(item(test_gap=True), facts) is None + + +def test_test_gap_still_rejects_three_commits(refactor): + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb"), fact(sha="ccc")] + problem = refactor.verify_apply_item(item(test_gap=True), facts) + assert problem is not None and "2 コミット" in problem + + +def test_a_broken_commit_is_reported_before_the_granularity(refactor): + """粒度は最後に見る。トレーラーやテストの問題を粒度で覆い隠さない。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb", test_status="fail")] + ) + assert problem is not None and "テストが成功していません" in problem + + +def test_the_budget_is_reported_before_the_granularity(refactor): + """差分予算の超過は原因が別なので、粒度より先に伝える。""" + problem = refactor.verify_apply_item( + item(technique="rename", estimated_diff_lines=10), + [fact(sha="aaa", diff_lines=50), fact(sha="bbb", diff_lines=50)], + ) + assert problem is not None and "差分予算" in problem + + +# ---------- 修正フェーズ ---------- + +def test_fix_accepts_one_commit_per_item(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-002"))] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] + assert accepted == [("R1-001", "aaa"), ("R1-002", "bbb")] + + +def test_fix_rejects_two_commits_for_the_same_item(refactor): + """適用側だけ揃えると、指摘への対応という名目で刻んだ履歴が戻ってくる。""" + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert problems and any("R1-001" in p and "1 コミット" in p for p in problems) + + +def test_fix_granularity_does_not_hide_a_broken_commit(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", test_status="fail", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert any("テストが成功していません" in p for p in problems) + + +@pytest.mark.parametrize("count", [1, 2, 3]) +def test_fix_allows_one_commit_for_each_distinct_item(refactor, count): + facts = [fact(sha=f"s{i}", trailers=trailers(item_id=f"R1-00{i}")) + for i in range(count)] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] and len(accepted) == count diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_init.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_init.py index 5667ae3..d5fbf92 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_init.py @@ -59,7 +59,7 @@ def _args(tmp_path, **over): "pr": 130, "scope": ["src"], "host": "claude", "max_outer_rounds": 3, "max_fix_rounds": 3, "max_items_per_round": 5, "severity_threshold": "minor", "model": None, "baseline_test": "true", - "sync_command": None, + "sync_command": None, "plan_file": None, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } @@ -429,3 +429,25 @@ def test_init_fills_the_posting_event_when_resuming_an_old_state(run_init, tmp_p assert resumed["is_own_pr"] is True assert resumed["event_downgrade"] is True assert "COMMENT" in resumed["review_post_note"] + + +# ---------- 改修計画の書き出し先 ---------- + +def test_init_records_the_default_plan_file(run_init, tmp_path): + """指定が無くても計画を残す。**既定で残らないと、誰も指定しない。**""" + run_init(_args(tmp_path)) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "issues/refactoring-plan-rf130.md" + + +def test_init_keeps_an_explicit_plan_file(run_init, tmp_path): + run_init(_args(tmp_path, plan_file="docs/plan.md")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "docs/plan.md" + + +def test_init_accepts_an_empty_plan_file_as_off(run_init, tmp_path): + """計画を差分へ入れたくないリポジトリのために、空文字で無効にできる。""" + run_init(_args(tmp_path, plan_file="")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "" diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_merge_apply.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_merge_apply.py index fa29964..d341a7f 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_merge_apply.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_merge_apply.py @@ -76,7 +76,7 @@ def test_commit_that_does_not_exist_fails(refactor): assert problem is not None and "範囲にありません" in problem -# ---------- 1 手 1 コミット ---------- +# ---------- 1 改善項目 = 1 コミット ---------- def test_zero_commits_fails(refactor): problem = refactor.verify_apply_item(item(), []) diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_plan_file.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_plan_file.py new file mode 100644 index 0000000..fe6c508 --- /dev/null +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_plan_file.py @@ -0,0 +1,233 @@ +"""改修計画をリポジトリ内のファイルへ残すテスト。 + +提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から除外される。 +**Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない。** +計画を差分の中へ置き、公開は生成物の同期と同じ経路（進行側の 1 コミット）に乗せる。 +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + + +def _make_work(tmp_path): + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "1 関数が 6 段の処理を通しで行っている", + "plan": "1. 範囲の確定を切り出す 2. 検証を切り出す", + "test_gap": False, "estimated_diff_lines": 40, + "proposed_by": ["codex", "gemini"], "status": "done", "commits": ["abc1234"], + } + base.update(over) + return base + + +def _state(tmp_path, work=None, **over): + rounds = over.pop("rounds", [{ + "round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0, + }]) + items = over.pop("items", [_item()]) + worktrees = {"work": str(work or tmp_path / "work")} + for r in ("codex", "gemini", "kiro"): + worktrees[r] = str(tmp_path / r) + path = make_state(tmp_path, rounds=rounds, items=items, + worktrees=worktrees, **over) + return path, read_state(path) + + +# ---------- 計画の本文 ---------- + +def test_plan_names_the_item_and_the_target(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "R1-001" in text and "src/foo.py" in text and "Foo.handle" in text + + +def test_plan_carries_the_reason_and_the_steps(refactor, tmp_path): + """なぜ直すのか・どう直すのかは、提案の時点でしか残らない。""" + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "1 関数が 6 段の処理を通しで行っている" in text + assert "1. 範囲の確定を切り出す" in text + + +def test_plan_shows_the_smell_and_the_technique(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "long_method" in text and "extract_method" in text + + +def test_plan_records_who_proposed_it(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_marks_an_abandoned_item(refactor, tmp_path): + """取り消した項目も残す。同じ提案が再び来たときの判断材料になる。""" + _, state = _state(tmp_path, items=[_item(status="abandoned", commits=[])]) + text = refactor.format_plan(state) + assert "取り消し" in text + + +def test_plan_groups_items_by_round(refactor, tmp_path): + rounds = [ + {"round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0}, + {"round": 2, "impl": "kiro", "reviewers": ["codex", "gemini"], + "items": ["R2-001"], "reviews": [], "fix_rounds": 0}, + ] + items = [_item(), _item(item_id="R2-001", round=2)] + _, state = _state(tmp_path, rounds=rounds, items=items) + text = refactor.format_plan(state) + assert text.index("ラウンド 1") < text.index("ラウンド 2") + + +def test_plan_names_the_implementer_of_each_round(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_is_stable_for_the_same_state(refactor, tmp_path): + """同じ状態からは同じ本文が出る。差分が出続けると毎回コミットが積まれる。""" + _, state = _state(tmp_path) + assert refactor.format_plan(state) == refactor.format_plan(state) + + +# ---------- 置き場所 ---------- + +def test_the_default_plan_file_lives_under_issues(refactor): + assert refactor.default_plan_file(136) == "issues/refactoring-plan-rf136.md" + + +def test_the_plan_file_is_written_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), "issues/plan.md") + + written = (work / "issues" / "plan.md").read_text(encoding="utf-8") + assert "R1-001" in written + + +# ---------- 公開 ---------- + +def test_the_plan_lands_in_one_commit_with_the_generated_files(refactor, tmp_path): + """計画書と生成物で 2 コミットに分けない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command="printf 'x = 2\\n' > generated/out.py") + + refactor._sync_generated(state) + + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files and "generated/out.py" in files + assert subject and "cross-refactoring" in subject + + +def test_a_repository_without_a_sync_command_still_records_the_plan( + refactor, tmp_path +): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + + refactor._sync_generated(state) + + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files + + +def test_an_unchanged_plan_does_not_add_a_commit(refactor, tmp_path): + """状態が動いていないのにコミットを積まない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + refactor._sync_generated(state) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +def test_an_empty_plan_file_setting_turns_the_record_off(refactor, tmp_path): + """計画を差分へ入れたくないリポジトリのために、無効にできる。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="", sync_command=None) + + refactor._sync_generated(state) + + assert not (work / "issues").exists() + + +# ---------- 書き出し先の検証 ---------- + +def test_a_relative_path_is_kept(refactor): + assert refactor.normalize_plan_file("issues/plan.md") == "issues/plan.md" + + +def test_a_leading_dot_is_normalized(refactor): + """`./issues/plan.md` は git が返すパスと一致しない。正規化して揃える。""" + assert refactor.normalize_plan_file("./issues/plan.md") == "issues/plan.md" + + +def test_an_empty_value_stays_empty(refactor): + assert refactor.normalize_plan_file("") == "" + assert refactor.normalize_plan_file(None) == "" + + +def test_an_absolute_path_is_refused(refactor): + """作業ディレクトリの外へ書かせない。進行側は利用者のリポジトリを触る。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("/tmp/out.md") + + +def test_a_parent_traversal_is_refused(refactor): + with pytest.raises(SystemExit): + refactor.normalize_plan_file("../out.md") + + +def test_a_traversal_in_the_middle_is_refused(refactor): + """途中で外へ出る経路も拒む。正規化してから判定する。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("issues/../../out.md") + + +def test_the_written_path_stays_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), state["plan_file"]) + + assert (work / "issues" / "plan.md").exists() + assert not (tmp_path / "plan.md").exists() diff --git a/plugins/ndf-shared/skills/cross-refactoring/SKILL.md b/plugins/ndf-shared/skills/cross-refactoring/SKILL.md index 52c8fe6..0c44026 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-shared/skills/cross-refactoring/SKILL.md @@ -37,6 +37,8 @@ allowed-tools: | 役割の分離 | 提案・レビューは**ホストを除く 3 者**、適用は**gemini を除く 3 者**。両者は重なるが一致しない | | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | +| コミットの単位 | **1 改善項目 = 1 コミット。** テストも項目の単位で 1 回だけ求める（現状固定テストが要る項目のみ 2 コミット） | +| 改修計画 | **差分の中へ残す。** 理由と手順は提案の時点でしか残らない。公開の直前に進行側が書き出し、生成物の同期と同じコミットへ入れる | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | | 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | | 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | @@ -61,6 +63,7 @@ allowed-tools: | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | | `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | +| `--plan-file PATH` | 改修計画の書き出し先（**対象リポジトリからの相対パス**）。空文字を渡すと記録しない | `issues/refactoring-plan-rf.md` | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" @@ -128,7 +131,7 @@ flowchart TD Merge --> Empty{"採用件数 = 0 ?"} Empty -->|はい| Final([提案ラウンドの繰り返しを終了]):::ok Empty -->|いいえ| Apply - Apply["Step 4: 適用（実装担当 1 CLI）
項目ごとに 1 手 1 コミット"] + Apply["Step 4: 適用（実装担当 1 CLI）
1 改善項目 = 1 コミット"] Apply --> Review["Step 5: レビュー（2 CLI 並列）
ラウンドの差分をまとめて 1 回"] Review --> Judge{"2 者とも承認 ?"} Judge -->|いいえ| Fix["Step 6: 指摘修正（実装担当）"] @@ -275,6 +278,9 @@ done | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | +| 1 項目を複数のコミットへ刻む | 改善項目と履歴が 1 対 1 で辿れなくなり、取り消しと積み直しのコミットも件数に比例して増える | +| 実装担当に手ごとのテストを義務づける | 進行側もコミットごとに回すため、テストの実行回数が手数の 2 倍になる（実測 44 手で 88 回） | +| 改修計画を状態ファイルにだけ残す | 状態ファイルは差分から除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない | | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | 投稿に失敗したまま結果ファイルを書かずに終了する | 進行側からは「レビュー担当が動かなかった」と区別が付かない。失敗したときほど `post_error` 付きの結果ファイルが要る | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | diff --git a/plugins/ndf-shared/skills/cross-refactoring/docs/01-state-and-propose.md b/plugins/ndf-shared/skills/cross-refactoring/docs/01-state-and-propose.md index fb75178..e2a06f1 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/docs/01-state-and-propose.md +++ b/plugins/ndf-shared/skills/cross-refactoring/docs/01-state-and-propose.md @@ -46,10 +46,14 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 +7. **改修計画の書き出し先の記録** — `--plan-file` を状態ファイルへ保存する。 + 既定は `issues/refactoring-plan-rf.md` で、空文字を渡すと記録しない。 + **既定で残す**のは、指定できるだけでは誰も指定しないためである。書き出しは + 初期化時ではなく、生成物の同期と同じく**push の直前**に行う +8. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） -8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +9. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない @@ -105,7 +109,7 @@ gemini は NDF の配布先ではないため「標準の配置先」を持た | `quality-gates` | 「直し終わった」と言える条件の判定 | `ndf-policies` は**配置しない**。git 運用やコミット規約といったリポジトリ運用の方針で -あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 手 1 コミット、 +あり、対象リポジトリの運用と食い違う可能性が高い。必要な規約（1 改善項目 = 1 コミット、 コミットトレーラー、`--force` 禁止など）はプロンプト側で明示する。 ### 守ること diff --git a/plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md b/plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md index c311658..f9bbf23 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md +++ b/plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md @@ -39,6 +39,7 @@ | 各コミットでテストが成功している | 実際の実行 | コミットを取り出して着手前と同じテストを走らせる。**JSON の `test_status` は読まない**。`--test-timeout`（既定 900 秒）を超えたら失敗 | | テストが無い経路は先に現状固定テスト | `git show --name-only` | `test_gap` が真の項目は、先頭コミットがテストの置き場所を触っている | | 項目の分離 | git のトレーラー | 各コミットの `Item-Id` がその項目と一致する。複数の項目を 1 コミットにまとめたら失敗 | +| コミットの粒度 | 申告されたコミットの実数 | 1 改善項目 = 1 コミット。`test_gap` が真の項目だけ 2 コミット | | 差分予算 | `git show --numstat` | 実差分の合計が `estimated_diff_lines` の 2 倍（抽出系の手法は 3 倍）を超えたら失敗（範囲の逸脱） | | 対象範囲の遵守 | `git show --name-only` | 触ったファイルが全て `--scope` の中にある。1 つでも外なら失敗 | | 機能変更の混入なし | — | 機械判定は不可能。レビュー観点に委ねる | @@ -74,6 +75,59 @@ 真のときの現状固定テストを数えることを明記した。倍率だけを広げると、見積が 楽観側へ倒れたぶんまで通してしまう。 +#### 1 改善項目 = 1 コミットにする + +**手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** 手順は +順に適用してよいが、残すのは項目単位の 1 コミットだけにする。 + +刻んだままだと 3 つの読みにくさが出る。 + +| 影響 | 内容 | +| --- | --- | +| 履歴 | Pull Request を読む側が、改善項目と履歴を 1 対 1 で辿れない | +| 取り消し | 範囲を戻して積み直すコミットが、刻んだ件数に比例して増える | +| 検証 | コミットごとにテストを実行するため、実行回数も比例して増える | + +実測では採用 12 件に対して適用が 34 コミット、取り消しと積み直しで 25 コミットだった。 + +**テストの回数も項目の単位に合わせる。** 進行側は申告されたコミットごとにテストを +実行するので、実装担当にも手ごとの実行を義務づけると、同じテストが手数の 2 倍だけ +走る。実測では 44 手に対して 88 回（1 回 26 秒として約 38 分）だった。 + +| 誰が | いつ | 6 回目の実測 | 項目単位にした後 | +| --- | --- | ---: | ---: | +| 実装担当 | コミットの前に 1 回 | 44 | 14 | +| 進行側 | 申告されたコミットごと | 44 | 14 | + +実装担当が途中で確かめる分には止めない。求めるのは**コミットの前に通っていること** +だけで、そこは進行側が実際に実行して確かめる。 + +例外は現状固定テストが要る項目（`test_gap` が真）だけで、「テスト → 実装」の +2 コミットを許す。1 つに混ぜると、テストが先行したことを履歴から確かめられない。 + +プロンプトは、途中で刻みたいときの戻し方も渡す。**その項目に着手する前の HEAD**を +控えておき、最後に `git reset --soft` で 1 コミットへまとめる。控えた地点より前へ +戻すと他の項目のコミットを巻き込むため、起点は項目の着手前に固定する。 + +#### 改修計画を差分へ残す + +**なぜ直すのか（理由）とどう直すのか（手順）は、提案の時点でしか残らない。** +状態ファイルには入っているが、そのディレクトリは差分から除外されるため、 +Pull Request を読む側からは見えない。 + +そこで `--plan-file`（既定 `issues/refactoring-plan-rf.md`）へ書き出す。 + +- 内容は**状態から決まる**。状態が動いていなければ差分は出ないので、公開のたびに + 書き出しても余計なコミットは積まれない +- 取り消した項目も残す。同じ提案が次のラウンドで来たときの判断材料になる +- 公開は生成物の同期と**同じコミット**に乗せる。分けると進行側のコミットが + 公開のたびに 2 つずつ積まれる +- 空文字を渡すと記録しない。差分へ入れたくないリポジトリのための逃げ道である +- **絶対パスと親へ抜ける経路は受け取った時点で拒む**（終了コード 4）。進行側は利用者の + リポジトリを触るため、作業ディレクトリの外へ書き出す余地を残さない。あわせて + `./issues/plan.md` のような表記も正規化する。git が返すパスと形が違うと、公開の + コミットメッセージが取り違えられる + #### 範囲の指定は検証にも効かせる `--scope` を必須にした目的は**提案の発散と変更の肥大を防ぐ**ことなので、指定を検証へ @@ -312,7 +366,7 @@ claude が参加する構成では実際のリファクタリング 1 件で 1.4 | --- | --- | | 項目ごとに実装者が入れ替わらない | 輪番の単位がラウンドなので、重ねれば実装者は分散する | | 指摘がどの項目に対するものか曖昧になる | 指摘に改善項目 ID を**必須**とし、未知の ID と欠落は差し戻す | -| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 手 1 コミットを保ち、**取り消しは項目単位**で行う | +| 1 件の失敗がラウンド全体を巻き込む | 項目ごとに 1 コミットを保ち、**取り消しは項目単位**で行う | ### 判定 diff --git a/plugins/ndf-shared/skills/cross-refactoring/docs/03-review-viewpoints.md b/plugins/ndf-shared/skills/cross-refactoring/docs/03-review-viewpoints.md index d607ea9..536799f 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/docs/03-review-viewpoints.md +++ b/plugins/ndf-shared/skills/cross-refactoring/docs/03-review-viewpoints.md @@ -14,7 +14,7 @@ | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | ## 振る舞い不変を筆頭に置く理由 diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md index ec9c8b9..f42ae6c 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md @@ -27,9 +27,18 @@ $RF_ITEMS 1. `test_gap` が真なら、**先に現状固定テストを追加してコミットする**。 これは省略できません。振る舞いが変わっていないことを示す手段が無いまま 構造を変えるのは、構造改善ではなく単なる編集です -2. `plan` の手順を **1 手ずつ**適用する -3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** -4. 通ったらコミットする（**1 手 = 1 コミット**） +2. `plan` の手順を順に適用する +3. **手順を終えてから** `$RF_BASELINE_TEST` を **1 回**実行する。落ちたら原因の手を戻す +4. 通ったら、**その項目の変更をまとめて 1 コミットにする** + +**テストもコミットも項目の単位で 1 回です。** 手ごとに回すと、進行側の検証と +合わせて手数の 2 倍だけテストが走ります（実測では 44 手で 88 回）。手ごとに +確かめたいときは自分の判断で回して構いませんが、**求めているのはコミットの前に +通っていること**だけです。 + +途中で刻んでおきたいときは、**その項目に着手する前の HEAD を控えておき**、 +最後に `git reset --soft <控えた HEAD>` してから 1 回だけコミットしてください +（控えた HEAD より前へ戻すと、他の項目のコミットを巻き込みます）。 ## コミットの規約 @@ -47,6 +56,10 @@ Impl-Runtime: $RF_RUNTIME Impl-Model: $RF_MODEL ``` +- **1 改善項目 = 1 コミット。** 2 件以上に刻むと、その項目は失敗として扱われ、 + 取り消されます。例外は `test_gap` が真の項目だけで、「現状固定テスト → 実装」の + 2 コミットを許します（テストと実装を混ぜると、テストが先行したことを + 履歴から確かめられません） - **`Item-Id` は必ずその項目のものにする。** 複数の項目を 1 コミットへまとめると、 取り消し範囲が項目単位で決まらなくなり、失敗として扱われます - `Impl-Model` には**実際に使ったモデル名**を書く。分からなければ `default` diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md index bb2180a..dd3bf4b 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md @@ -24,7 +24,8 @@ $RF_ITEMS 1. `gh api` で Pull Request の**未解決レビュースレッド**を取得する 2. 各指摘について、**修正するか・しないか**を決める - - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする + - 修正する: 直してから `$RF_BASELINE_TEST` を 1 回実行し、**その改善項目の + 修正を 1 コミットにまとめる** - 修正しない: 根拠を返信する。**黙って閉じない** 3. 対応したスレッドに返信し、`resolveReviewThread` で解決する @@ -38,6 +39,11 @@ $RF_ITEMS いること**が必要です。満たさないコミットは取り込まれず、その改善項目の指摘は 解決済みになりません（修正ラウンドの上限に達すると項目ごと取り消されます）。 +**1 改善項目 = 1 コミット。** 同じ項目のコミットが 2 件以上あると、その修正 +ラウンドの範囲ごと取り消されます。適用側だけ揃えても、指摘への対応という名目で +刻んだ履歴が戻ってくるためです。複数の項目に指摘が付いているときは、項目ごとに +1 コミットへ分けてください。 + 判定はオーケストレータが **git と実際のテスト実行**から行います。結果ファイルに 何と書いても検査結果は変わりません。 diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/review.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/review.md index ed083c9..5c8144d 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/review.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/review.md @@ -31,7 +31,7 @@ $RF_ITEMS | 手法の適合 | 宣言されたスメルに対して手法が妥当か、別の手法の方が適切でないか | | 範囲の逸脱 | 提案した手順の範囲を超えた変更が混ざっていないか、機能変更が混入していないか | | 改善の実質 | 行数が動いただけでなく、責務・依存・可読性が実際に改善しているか | -| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 手 1 コミットか | +| コミット分割 | 改名と中身の変更が同一コミットに混ざっていないか、1 改善項目 = 1 コミットか | | 性能退行 | ループの入れ替え、呼び出し回数の増加、N+1 の発生 | **振る舞い不変を筆頭に置く。** 疑わしければ実際に `$RF_BASELINE_TEST` を実行して diff --git a/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py index 685de46..87b018b 100755 --- a/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py @@ -145,6 +145,33 @@ def vocabulary() -> dict[str, Any]: "公開の直前に進行側がまとめて生成する。" ) +# 計画と生成物を 1 つのコミットへまとめたときのメッセージ。 +SYNC_AND_PLAN_COMMIT_MESSAGE = ( + "Chore: 生成物と改修計画を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "改修計画は提案の時点でしか残らないため、公開の直前に書き出す。\n" + "どちらも進行側の責務なので、1 つのコミットにまとめる。" +) + +# 改修計画だけを記録したコミットのメッセージ。 +PLAN_COMMIT_MESSAGE = ( + "Docs: 改修計画を記録する（cross-refactoring 進行側）\n\n" + "なぜ直すのか（理由）とどう直すのか（手順）は提案の時点でしか残らない。\n" + "状態ファイルは差分から除外されるため、Pull Request から読める場所へ置く。" +) + +# 改修計画を書き出す既定のディレクトリ。 +DEFAULT_PLAN_DIR = "issues" + +# 改善項目の状態を、Pull Request を読む側に通じる語へ置き換える。 +ITEM_STATUS_LABELS = { + "pending": "未着手", + "reviewing": "レビュー中", + "done": "採用", + "abandoned": "取り消し", + "blocked": "着手せず", +} + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -168,6 +195,19 @@ def vocabulary() -> dict[str, Any]: }) EXTRACTION_DIFF_BUDGET_FACTOR = 3 +# 1 改善項目が履歴に残せるコミット数。 +# +# **手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。** +# 手ごとにテストを回して安全に進めるのは変わらないが、残すのは項目単位の +# 1 コミットだけにする。刻んだままだと Pull Request を読む側が改善項目と履歴を +# 1 対 1 で辿れず、取り消しと積み直しのコミットも件数に比例して増える +# （実測: 採用 12 件に対して適用 34 コミット、取り消しと積み直しで 25 コミット）。 +MAX_COMMITS_PER_ITEM = 1 + +# 現状固定テストが要る項目だけは 2 コミットを許す。テストと実装を 1 コミットへ +# 混ぜると、「テストを先に足した」ことを履歴から確かめられなくなる。 +MAX_COMMITS_PER_ITEM_WITH_TEST_GAP = 2 + # テスト 1 回あたりの上限（秒）。生成されたコードやテストが無限ループに入ると、 # 待ち続けて**進行全体が止まる**。打ち切って失敗として扱う。 DEFAULT_TEST_TIMEOUT = 900 @@ -579,7 +619,7 @@ def verify_apply_item( 確かめられる**。読ませ方の不確実性に対する最後の砦としてここを厚くする。 """ if not facts: - return "コミットが 1 件もありません（1 手 1 コミットの前提を満たしていません）" + return "コミットが 1 件もありません（1 改善項目 = 1 コミットの前提を満たしていません）" for commit in facts: problem = _verify_commit_basics( @@ -617,9 +657,38 @@ def verify_apply_item( f"実差分 {actual} 行が差分予算 {budget} 行" f"（見積 {estimated} 行 × {factor}）を超えました（範囲の逸脱）" ) + + # 粒度は最後に見る。トレーラーやテストの問題を粒度の失敗で覆い隠さない。 + # 数えるのは**実在するコミットの数**である。同じコミットを重ねて申告しただけの + # ときに落とすと、食い違いの無い申告を刻みすぎとして扱ってしまう。 + problem = verify_commit_granularity(item, len({c.get("sha") for c in facts})) + if problem: + return problem return None +def commit_limit_for(item: dict[str, Any]) -> int: + """その項目が履歴に残せるコミット数。""" + if item.get("test_gap"): + return MAX_COMMITS_PER_ITEM_WITH_TEST_GAP + return MAX_COMMITS_PER_ITEM + + +def verify_commit_granularity(item: dict[str, Any], count: int) -> Optional[str]: + """項目のコミット数が上限に収まっているか。超えていれば理由を返す。 + + 適用（`verify_apply_item`）と修正（`_verify_fix_commits`）で**同じ基準**を使う。 + 適用側だけ揃えると、レビュー指摘への対応という名目で刻んだ履歴が戻ってくる。 + """ + limit = commit_limit_for(item) + if count <= limit: + return None + return ( + f"項目 {item['item_id']} のコミットが {count} 件あります" + f"（残すのは 1 項目 = 1 コミット。現状固定テストが要る項目だけ 2 コミットまで）" + ) + + # ---------------- レビュー判定 ---------------- REVIEW_URL_MARKER = "#pullrequestreview-" @@ -1011,6 +1080,11 @@ def cmd_init(args: argparse.Namespace) -> None: "baseline_test": baseline, # 生成物の同期は**進行側の責務**。push の直前に実行する。 "sync_command": args.sync_command, + # 改修計画の書き出し先も同じ経路に乗せる。指定が無ければ既定のパスを使い、 + # 空文字なら記録しない。 + "plan_file": normalize_plan_file( + default_plan_file(args.pr) if args.plan_file is None else args.plan_file + ), "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -2018,7 +2092,7 @@ def cmd_abandon_items(args: argparse.Namespace) -> None: """Step 6 — 未解決の指摘に紐づく改善項目だけを取り消す。 **合意済みの項目は Pull Request に残す。** これを可能にするために、適用は - 項目ごとに 1 手 1 コミットへ分け、状態ファイルへコミットを記録している。 + 項目ごとに 1 コミットへまとめ、状態ファイルへコミットを記録している。 """ path, state = _load(args.id) entry = _round(state, args.round) @@ -2180,6 +2254,7 @@ def _verify_fix_commits( """ problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) + seen: dict[str, set[str]] = {} # item_id -> 実在するコミットの集合 for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, scope) @@ -2187,7 +2262,16 @@ def _verify_fix_commits( problems.append(problem) info(f"❌ 修正コミットが手順を満たしていません: {problem}") continue + seen.setdefault(item_id, set()).add(commit["sha"]) accepted.append((item_id, commit["sha"])) + + # 粒度は 1 件ずつの検証が済んでから見る。壊れたコミットの理由を + # 粒度の失敗で覆い隠さない。 + for item_id, shas in seen.items(): + problem = verify_commit_granularity({"item_id": item_id}, len(shas)) + if problem: + problems.append(problem) + info(f"❌ 修正コミットが手順を満たしていません: {problem}") return problems, accepted @@ -2379,6 +2463,96 @@ def cmd_status(args: argparse.Namespace) -> None: print(_round_table(state)) +def default_plan_file(pr: int) -> str: + """改修計画を書き出す既定のパス。""" + return f"{DEFAULT_PLAN_DIR}/refactoring-plan-rf{pr}.md" + + +def normalize_plan_file(value: Optional[str]) -> str: + """改修計画の書き出し先を検証して正規化する。空文字は「記録しない」。 + + **作業ディレクトリの外へ書かせない。** 進行側は利用者のリポジトリを触るので、 + 絶対パスと親へ抜ける経路は受け取った時点で拒む。 + + 正規化するのは、判定に使うパスを git の出力と揃えるためでもある。 + `./issues/plan.md` のまま持つと、`git status` が返す `issues/plan.md` と + 一致せず、公開のコミットメッセージが取り違えられる。 + """ + rel = str(value or "").strip() + if not rel: + return "" + if os.path.isabs(rel) or (len(rel) > 1 and rel[1] == ":"): + die(f"--plan-file には相対パスを指定してください: {rel}", code=4) + normalized = os.path.normpath(rel) + if normalized == ".." or normalized.startswith(".." + os.sep): + die( + f"--plan-file が作業ディレクトリの外を指しています: {rel}", + code=4, + ) + return normalized + + +def format_plan(state: dict[str, Any]) -> str: + """改修計画の本文を組み立てる。**同じ状態からは同じ本文が出る。** + + 提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から + 除外される。Pull Request を読む側からは、なぜ直したのかも、どう直す計画 + だったのかも見えない。ここで差分の中へ置く。 + """ + baseline = state.get("baseline_test") or {} + lines = [ + f"# 改修計画 — {state['repo']} #{state['current_pr']}", + "", + "`/ndf:cross-refactoring` が提案し、適用した改善項目の記録である。", + "理由と手順は提案の時点でしか残らないため、公開の直前に書き出している。", + "", + f"- 対象範囲: {', '.join(state.get('target_scope') or []) or '（未指定）'}", + f"- 着手前のテスト: {baseline.get('command') or '（未指定）'}", + "", + ] + for entry in state.get("rounds") or []: + lines.extend(_plan_round_section(state, entry)) + if not (state.get("rounds") or []): + lines.append("（改善項目なし）") + return "\n".join(lines).rstrip() + "\n" + + +def _plan_round_section(state: dict[str, Any], entry: dict[str, Any]) -> list[str]: + """1 ラウンド分の見出しと、そのラウンドの改善項目を並べる。""" + reviewers = " / ".join(entry.get("reviewers") or []) or "—" + lines = [ + f"## ラウンド {entry['round']}" + f"（実装 {entry.get('impl', '—')} / レビュー {reviewers}）", + "", + ] + items = [i for i in state.get("items") or [] if i.get("round") == entry["round"]] + if not items: + lines.extend(["（採用した改善項目なし）", ""]) + return lines + for item in items: + lines.extend(_plan_item_section(item)) + return lines + + +def _plan_item_section(item: dict[str, Any]) -> list[str]: + """改善項目 1 件の見出し・要約表・理由・手順。""" + status = ITEM_STATUS_LABELS.get(item.get("status"), item.get("status") or "—") + return [ + f"### {item['item_id']} — `{item['path']}#{item['symbol']}`", + "", + "| スメル | 手法 | 重要度 | 提案元 | 状態 | コミット |", + "| --- | --- | --- | --- | --- | ---: |", + f"| {item['smell']} | {item['technique']} | {item['severity']} | " + f"{' / '.join(item.get('proposed_by') or []) or '—'} | {status} | " + f"{len(item.get('commits') or [])} |", + "", + f"**なぜ**: {item.get('rationale') or '（記録なし）'}", + "", + f"**手順**: {item.get('plan') or '（記録なし）'}", + "", + ] + + def cmd_report(args: argparse.Namespace) -> None: """Step 8 — ラウンド表・項目表・見送り項目・指標を出す。""" _, state = _load(args.id) @@ -3206,7 +3380,31 @@ def _run_sync_command(state: dict[str, Any], work: str, command: str) -> None: ) -def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: +def _write_plan_file(state: dict[str, Any], work: str, rel: str) -> None: + """改修計画を作業ディレクトリの中へ書き出す。 + + 内容は状態から決まるので、**状態が動いていなければ差分は出ない**。 + 書き出しを毎回行っても、余計なコミットは積まれない。 + """ + path = pathlib.Path(work) / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(format_plan(state), encoding="utf-8") + + +def _publish_commit_message(produced: list[str], plan_rel: str) -> str: + """公開の直前に積むコミットのメッセージを、中身に合わせて選ぶ。""" + has_plan = bool(plan_rel) and plan_rel in produced + has_generated = any(p != plan_rel for p in produced) + if has_plan and has_generated: + return SYNC_AND_PLAN_COMMIT_MESSAGE + if has_plan: + return PLAN_COMMIT_MESSAGE + return SYNC_COMMIT_MESSAGE + + +def _commit_sync_changes( + work: str, command: str, produced: list[str], plan_rel: str = "" +) -> None: """同期が作った差分を進行側のコミットとして積む。差分が無ければ何もしない。 このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 @@ -3221,11 +3419,15 @@ def _commit_sync_changes(work: str, command: str, produced: list[str]) -> None: # 確認済みだからである。 try: _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + _sh(["git", "commit", "-m", _publish_commit_message(produced, plan_rel)], + cwd=work) except SystemExit: _discard_worktree_changes(work) raise - info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + if command: + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + else: + info(f"📝 改修計画を記録しました（{len(produced)} ファイル）") def _sync_generated(state: dict[str, Any]) -> None: @@ -3243,14 +3445,22 @@ def _sync_generated(state: dict[str, Any]) -> None: 利用者のリポジトリの検査を壊したまま進むことになる。 """ command = str(state.get("sync_command") or "").strip() - if not command: + # 状態ファイルの値も受け取った時点と同じ基準で通す。旧い状態ファイルや + # 手で書き換えられた値でも、作業ディレクトリの外へは書き出さない。 + plan_rel = normalize_plan_file(state.get("plan_file")) + if not command and not plan_rel: return work = state["worktrees"]["work"] # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 # 同期が作った差分と元からあった差分を区別できない。 _require_clean_worktree(state, work) - _run_sync_command(state, work, command) - _commit_sync_changes(work, command, _dirty_paths(state, work)) + # 改修計画も生成物と同じ経路に乗せる。**別のコミットに分けない。** + # 分けると、進行側のコミットが公開のたびに 2 つずつ積まれる。 + if plan_rel: + _write_plan_file(state, work, plan_rel) + if command: + _run_sync_command(state, work, command) + _commit_sync_changes(work, command, _dirty_paths(state, work), plan_rel) def _push_head(state: dict[str, Any]) -> None: @@ -3398,6 +3608,12 @@ def main() -> None: help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" "差分があれば進行側のコミットとして積む。" "同期を実装担当にさせると範囲外の変更になるため分離している") + init.add_argument("--plan-file", default=None, + help="改修計画を書き出すパス（対象リポジトリからの相対）。" + "提案の理由と手順は状態ファイルにしか残らず、差分から" + "除外されるため、公開の直前に進行側が書き出す。" + f"既定は {DEFAULT_PLAN_DIR}/refactoring-plan-rf.md。" + "空文字を渡すと記録しない") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_commit_granularity.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_commit_granularity.py new file mode 100644 index 0000000..aeec6f7 --- /dev/null +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_commit_granularity.py @@ -0,0 +1,125 @@ +"""コミットの粒度（1 改善項目 = 1 コミット）のテスト。 + +手順を 1 手ずつ進めることと、その途中経過を履歴に残すことは別である。 +**残すのは項目単位の 1 コミットだけ**にして、Pull Request を読む側が +改善項目と履歴を 1 対 1 で辿れるようにする。 + +現状固定テストが要る項目（`test_gap`）だけは、テストと実装を混ぜないために +2 コミットを許す。 +""" +from __future__ import annotations + +import pytest + + +def trailers(item_id="R1-001", round_no="1", runtime="codex", model="gpt-5.5"): + return { + "Item-Id": item_id, "Round": round_no, + "Impl-Runtime": runtime, "Impl-Model": model, + } + + +def fact(sha="abc1234", **over): + base = { + "sha": sha, "exists": True, "test_status": "pass", + "touches_tests": False, "diff_lines": 30, "trailers": trailers(), + } + base.update(over) + return base + + +def item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 400, "proposed_by": ["codex"], + "status": "pending", "commits": [], + } + base.update(over) + return base + + +# ---------- 適用フェーズ ---------- + +def test_one_commit_per_item_passes(refactor): + assert refactor.verify_apply_item(item(), [fact()]) is None + + +def test_two_commits_for_one_item_fails(refactor): + """途中経過を刻むと、改善項目と履歴が 1 対 1 で対応しなくなる。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb")] + ) + assert problem is not None and "1 コミット" in problem + + +def test_the_granularity_message_names_the_item(refactor): + """どの項目が刻みすぎたのかを読めるようにする。""" + problem = refactor.verify_apply_item( + item(item_id="R2-003"), [fact(sha="aaa", trailers=trailers(item_id="R2-003")), + fact(sha="bbb", trailers=trailers(item_id="R2-003"))] + ) + assert "R2-003" in problem + + +def test_test_gap_allows_the_characterization_test_commit(refactor): + """テストと実装を 1 コミットへ混ぜないため、この項目だけ 2 コミットを許す。""" + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb")] + assert refactor.verify_apply_item(item(test_gap=True), facts) is None + + +def test_test_gap_still_rejects_three_commits(refactor): + facts = [fact(sha="aaa", touches_tests=True), fact(sha="bbb"), fact(sha="ccc")] + problem = refactor.verify_apply_item(item(test_gap=True), facts) + assert problem is not None and "2 コミット" in problem + + +def test_a_broken_commit_is_reported_before_the_granularity(refactor): + """粒度は最後に見る。トレーラーやテストの問題を粒度で覆い隠さない。""" + problem = refactor.verify_apply_item( + item(), [fact(sha="aaa"), fact(sha="bbb", test_status="fail")] + ) + assert problem is not None and "テストが成功していません" in problem + + +def test_the_budget_is_reported_before_the_granularity(refactor): + """差分予算の超過は原因が別なので、粒度より先に伝える。""" + problem = refactor.verify_apply_item( + item(technique="rename", estimated_diff_lines=10), + [fact(sha="aaa", diff_lines=50), fact(sha="bbb", diff_lines=50)], + ) + assert problem is not None and "差分予算" in problem + + +# ---------- 修正フェーズ ---------- + +def test_fix_accepts_one_commit_per_item(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-002"))] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] + assert accepted == [("R1-001", "aaa"), ("R1-002", "bbb")] + + +def test_fix_rejects_two_commits_for_the_same_item(refactor): + """適用側だけ揃えると、指摘への対応という名目で刻んだ履歴が戻ってくる。""" + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert problems and any("R1-001" in p and "1 コミット" in p for p in problems) + + +def test_fix_granularity_does_not_hide_a_broken_commit(refactor): + facts = [fact(sha="aaa", trailers=trailers(item_id="R1-001")), + fact(sha="bbb", test_status="fail", trailers=trailers(item_id="R1-001"))] + problems, _ = refactor._verify_fix_commits(facts, ["src"]) + assert any("テストが成功していません" in p for p in problems) + + +@pytest.mark.parametrize("count", [1, 2, 3]) +def test_fix_allows_one_commit_for_each_distinct_item(refactor, count): + facts = [fact(sha=f"s{i}", trailers=trailers(item_id=f"R1-00{i}")) + for i in range(count)] + problems, accepted = refactor._verify_fix_commits(facts, ["src"]) + assert problems == [] and len(accepted) == count diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_init.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_init.py index 5667ae3..d5fbf92 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_init.py @@ -59,7 +59,7 @@ def _args(tmp_path, **over): "pr": 130, "scope": ["src"], "host": "claude", "max_outer_rounds": 3, "max_fix_rounds": 3, "max_items_per_round": 5, "severity_threshold": "minor", "model": None, "baseline_test": "true", - "sync_command": None, + "sync_command": None, "plan_file": None, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } @@ -429,3 +429,25 @@ def test_init_fills_the_posting_event_when_resuming_an_old_state(run_init, tmp_p assert resumed["is_own_pr"] is True assert resumed["event_downgrade"] is True assert "COMMENT" in resumed["review_post_note"] + + +# ---------- 改修計画の書き出し先 ---------- + +def test_init_records_the_default_plan_file(run_init, tmp_path): + """指定が無くても計画を残す。**既定で残らないと、誰も指定しない。**""" + run_init(_args(tmp_path)) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "issues/refactoring-plan-rf130.md" + + +def test_init_keeps_an_explicit_plan_file(run_init, tmp_path): + run_init(_args(tmp_path, plan_file="docs/plan.md")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "docs/plan.md" + + +def test_init_accepts_an_empty_plan_file_as_off(run_init, tmp_path): + """計画を差分へ入れたくないリポジトリのために、空文字で無効にできる。""" + run_init(_args(tmp_path, plan_file="")) + _, state = _state_of(tmp_path) + assert state["plan_file"] == "" diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_merge_apply.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_merge_apply.py index fa29964..d341a7f 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/tests/test_merge_apply.py +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_merge_apply.py @@ -76,7 +76,7 @@ def test_commit_that_does_not_exist_fails(refactor): assert problem is not None and "範囲にありません" in problem -# ---------- 1 手 1 コミット ---------- +# ---------- 1 改善項目 = 1 コミット ---------- def test_zero_commits_fails(refactor): problem = refactor.verify_apply_item(item(), []) diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_plan_file.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_plan_file.py new file mode 100644 index 0000000..fe6c508 --- /dev/null +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_plan_file.py @@ -0,0 +1,233 @@ +"""改修計画をリポジトリ内のファイルへ残すテスト。 + +提案の理由と手順は状態ファイルにしか残らず、そのディレクトリは差分から除外される。 +**Pull Request を読む側からは、なぜ直したのかも、どう直す計画だったのかも見えない。** +計画を差分の中へ置き、公開は生成物の同期と同じ経路（進行側の 1 コミット）に乗せる。 +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + + +def _make_work(tmp_path): + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _item(**over): + base = { + "item_id": "R1-001", "round": 1, "path": "src/foo.py", "symbol": "Foo.handle", + "smell": "long_method", "technique": "extract_method", "severity": "major", + "rationale": "1 関数が 6 段の処理を通しで行っている", + "plan": "1. 範囲の確定を切り出す 2. 検証を切り出す", + "test_gap": False, "estimated_diff_lines": 40, + "proposed_by": ["codex", "gemini"], "status": "done", "commits": ["abc1234"], + } + base.update(over) + return base + + +def _state(tmp_path, work=None, **over): + rounds = over.pop("rounds", [{ + "round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0, + }]) + items = over.pop("items", [_item()]) + worktrees = {"work": str(work or tmp_path / "work")} + for r in ("codex", "gemini", "kiro"): + worktrees[r] = str(tmp_path / r) + path = make_state(tmp_path, rounds=rounds, items=items, + worktrees=worktrees, **over) + return path, read_state(path) + + +# ---------- 計画の本文 ---------- + +def test_plan_names_the_item_and_the_target(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "R1-001" in text and "src/foo.py" in text and "Foo.handle" in text + + +def test_plan_carries_the_reason_and_the_steps(refactor, tmp_path): + """なぜ直すのか・どう直すのかは、提案の時点でしか残らない。""" + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "1 関数が 6 段の処理を通しで行っている" in text + assert "1. 範囲の確定を切り出す" in text + + +def test_plan_shows_the_smell_and_the_technique(refactor, tmp_path): + _, state = _state(tmp_path) + text = refactor.format_plan(state) + assert "long_method" in text and "extract_method" in text + + +def test_plan_records_who_proposed_it(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_marks_an_abandoned_item(refactor, tmp_path): + """取り消した項目も残す。同じ提案が再び来たときの判断材料になる。""" + _, state = _state(tmp_path, items=[_item(status="abandoned", commits=[])]) + text = refactor.format_plan(state) + assert "取り消し" in text + + +def test_plan_groups_items_by_round(refactor, tmp_path): + rounds = [ + {"round": 1, "impl": "codex", "reviewers": ["gemini", "kiro"], + "items": ["R1-001"], "reviews": [], "fix_rounds": 0}, + {"round": 2, "impl": "kiro", "reviewers": ["codex", "gemini"], + "items": ["R2-001"], "reviews": [], "fix_rounds": 0}, + ] + items = [_item(), _item(item_id="R2-001", round=2)] + _, state = _state(tmp_path, rounds=rounds, items=items) + text = refactor.format_plan(state) + assert text.index("ラウンド 1") < text.index("ラウンド 2") + + +def test_plan_names_the_implementer_of_each_round(refactor, tmp_path): + _, state = _state(tmp_path) + assert "codex" in refactor.format_plan(state) + + +def test_plan_is_stable_for_the_same_state(refactor, tmp_path): + """同じ状態からは同じ本文が出る。差分が出続けると毎回コミットが積まれる。""" + _, state = _state(tmp_path) + assert refactor.format_plan(state) == refactor.format_plan(state) + + +# ---------- 置き場所 ---------- + +def test_the_default_plan_file_lives_under_issues(refactor): + assert refactor.default_plan_file(136) == "issues/refactoring-plan-rf136.md" + + +def test_the_plan_file_is_written_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), "issues/plan.md") + + written = (work / "issues" / "plan.md").read_text(encoding="utf-8") + assert "R1-001" in written + + +# ---------- 公開 ---------- + +def test_the_plan_lands_in_one_commit_with_the_generated_files(refactor, tmp_path): + """計画書と生成物で 2 コミットに分けない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command="printf 'x = 2\\n' > generated/out.py") + + refactor._sync_generated(state) + + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files and "generated/out.py" in files + assert subject and "cross-refactoring" in subject + + +def test_a_repository_without_a_sync_command_still_records_the_plan( + refactor, tmp_path +): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + + refactor._sync_generated(state) + + files = _git("show", "--name-only", "--format=", "HEAD", cwd=work).stdout.split() + assert "issues/plan.md" in files + + +def test_an_unchanged_plan_does_not_add_a_commit(refactor, tmp_path): + """状態が動いていないのにコミットを積まない。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md", + sync_command=None) + refactor._sync_generated(state) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +def test_an_empty_plan_file_setting_turns_the_record_off(refactor, tmp_path): + """計画を差分へ入れたくないリポジトリのために、無効にできる。""" + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="", sync_command=None) + + refactor._sync_generated(state) + + assert not (work / "issues").exists() + + +# ---------- 書き出し先の検証 ---------- + +def test_a_relative_path_is_kept(refactor): + assert refactor.normalize_plan_file("issues/plan.md") == "issues/plan.md" + + +def test_a_leading_dot_is_normalized(refactor): + """`./issues/plan.md` は git が返すパスと一致しない。正規化して揃える。""" + assert refactor.normalize_plan_file("./issues/plan.md") == "issues/plan.md" + + +def test_an_empty_value_stays_empty(refactor): + assert refactor.normalize_plan_file("") == "" + assert refactor.normalize_plan_file(None) == "" + + +def test_an_absolute_path_is_refused(refactor): + """作業ディレクトリの外へ書かせない。進行側は利用者のリポジトリを触る。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("/tmp/out.md") + + +def test_a_parent_traversal_is_refused(refactor): + with pytest.raises(SystemExit): + refactor.normalize_plan_file("../out.md") + + +def test_a_traversal_in_the_middle_is_refused(refactor): + """途中で外へ出る経路も拒む。正規化してから判定する。""" + with pytest.raises(SystemExit): + refactor.normalize_plan_file("issues/../../out.md") + + +def test_the_written_path_stays_inside_the_work_dir(refactor, tmp_path): + work = _make_work(tmp_path) + _, state = _state(tmp_path, work=work, plan_file="issues/plan.md") + + refactor._write_plan_file(state, str(work), state["plan_file"]) + + assert (work / "issues" / "plan.md").exists() + assert not (tmp_path / "plan.md").exists()