diff --git a/issues/issue-113-cross-refactoring-push-ownership.md b/issues/issue-113-cross-refactoring-push-ownership.md new file mode 100644 index 00000000..d7576c3f --- /dev/null +++ b/issues/issue-113-cross-refactoring-push-ownership.md @@ -0,0 +1,132 @@ +# issue-113: 公開の責務を進行側へ一本化し、適用失敗の項目を対象外へ記録する + +## 関連リンク + +- [issue-113-cross-refactoring-retrial.md](issue-113-cross-refactoring-retrial.md) — 不具合 10・11 を見つけた再検証（PR #120） +- [issue-113-cross-refactoring-defect-fixes.md](issue-113-cross-refactoring-defect-fixes.md) — 不具合 1〜9 の修正（PR #119） + +## モード + +`architecture`。`init` の引数（`--sync-command`）と「誰が push するか」という +役割の契約を変更し、`refactor.py` / プロンプト / 手順書にまたがる。 + +## 目的と非目的 + +達成したい状態: + +- 生成物の同期を検査する pre-push を持つリポジトリでも、収束ループが成立する +- **検証を通っていない変更が Pull Request に現れない**（不具合 4 の経路を根絶する） +- 適用の検証で失敗した提案が、次のラウンドで再び採用されない + +やらないこと: + +- 生成物を持たないリポジトリへの影響（`--sync-command` は省略可能にする） +- `--no-verify` による回避（検証を飛ばす手段は増やさない） + +## 受け入れ条件 + +- [ ] 1. 実装担当は push しない。プロンプトから push の手順が消え、禁止として明記される +- [ ] 2. `merge-apply` は成功・失敗のどちらでも、検証後に進行側が push する +- [ ] 3. `merge-fix` も検証後に進行側が push する +- [ ] 4. `--sync-command` を指定すると、**push の直前**に同期が走り、差分があれば + 進行側のコミットとして積まれる（`test_push_syncs_generated_files_first`） +- [ ] 5. 同期に失敗したら中断する（終了コード 4）。黙って push しない +- [ ] 6. `--sync-command` 未指定なら同期は走らない（既存の利用者に影響しない） +- [ ] 7. 適用の検証で失敗した項目が `deferred_items` へ理由付きで記録され、 + 次ラウンドの提案で「対象外」として渡る（`test_failed_items_are_deferred`） +- [ ] 8. 既存 430 件のテストが退行しない + +## 代替案と採否 + +### 生成物の同期を誰がどこで行うか + +| 案 | 内容 | 採否 | 理由 | +| --- | --- | --- | --- | +| A | **進行側が push の直前に同期する** | 採用 | push は `_push_head()` の 1 経路に集約されているため、同期を差し込む場所も 1 つで済む | +| B | 進行側が収束後にまとめて同期する（現行） | 不採用 | **ループ中に push が起きることを見落としていた**。実測で全 push が落ち、実装担当を範囲違反へ誘導した | +| C | 実装担当に同期させる | 不採用 | 範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） | +| D | `--no-verify` で検査を飛ばす | 不採用 | 検証を飛ばす手段を増やさないという方針に反する | + +### 誰が push するか + +| 案 | 内容 | 採否 | 理由 | +| --- | --- | --- | --- | +| A | **進行側だけが、検証を通した後に push する** | 採用 | 「未検証の変更が公開される」経路自体が無くなる。不具合 4 は緩和ではなく根絶になる | +| B | 実装担当が項目ごとに push する（現行） | 不採用 | 検証前に公開されるため、取り消しの反映漏れが Pull Request に残る | + +## 不変条件 + +- Pull Request に現れるのは、**検証を通ったコミットと進行側の同期コミットだけ**である +- `git push --force` と `--no-verify` は使わない +- 同期コミットはどの改善項目にも属さない。取り消しでは積み直さない + （次の push で作り直されるため失われても問題にならない） + +## 互換性 + +| 対象 | 変更 | 互換性の扱い | +| --- | --- | --- | +| `init` の引数 | `--sync-command` を追加 | 追加のみ。省略時は同期しない | +| 状態ファイル | `sync_command` を追加 | 追加のみ。欠けていても読める | +| 実装担当の手順 | push を禁止に変える | **破る**。プロンプトと手順書を同時に変更する | +| `merge-apply` / `merge-fix` の副作用 | 常に push するようになる | **破る**。手順書に明記する | + +## 修正対象 + +``` +plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py +plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md +plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md +plugins/ndf-shared/skills/cross-refactoring/SKILL.md +plugins/ndf-shared/skills/cross-refactoring/docs/02-apply-and-review.md +plugins/ndf-shared/skills/cross-refactoring/tests/ +plugins/ndf-{claude,codex,kiro}/skills/... # 配布物（生成） +``` + +## タスク分解 + +### Task 1: 適用で失敗した項目を「対象外」へ記録する + +- **対象ファイル:** `scripts/refactor.py`、`tests/test_merge_apply.py` +- **変更内容:** `_defer_abandoned_items()` を追加し、取り消しの完了時に呼ぶ。 + `item_id` で重複を防ぐ +- **満たす受け入れ条件:** 7 +- **進め方:** 再提案が防げることを確かめる失敗テスト → 実装 + +### Task 2: push の直前に生成物を同期する + +- **対象ファイル:** `scripts/refactor.py`、`tests/test_merge_apply.py` +- **変更内容:** `--sync-command` を `init` へ追加し状態へ保存する。 + `_sync_generated()` を追加し、`_push_head()` の冒頭で呼ぶ。差分があれば + 進行側のコミットとして積む。同期の失敗は中断（終了コード 4） +- **満たす受け入れ条件:** 4, 5, 6 +- **進め方:** 同期→コミット→push の順序を確かめる失敗テスト → 実装 + +### Task 3: 公開の責務を進行側へ移す + +- **対象ファイル:** `scripts/refactor.py`、`prompts/apply.md`、`prompts/fix.md`、 + `SKILL.md`、`docs/02-apply-and-review.md`、`tests/` +- **変更内容:** `merge-apply` / `merge-fix` が検証後に必ず push する。 + プロンプトから push の手順を消し、禁止として明記する。手順書を追従させる +- **満たす受け入れ条件:** 1, 2, 3 +- **進め方:** 成功経路でも push することを確かめる失敗テスト → 実装 → 文書追従 + +### Task 4: 配布物を生成する + +- **対象ファイル:** `plugins/ndf-{claude,codex,kiro}/` +- **変更内容:** `bash scripts/build-runtime-plugins.sh` +- **満たす受け入れ条件:** 8 + +## リスクと対処 + +| リスク | 対処 | +| --- | --- | +| 同期コマンドが遅い / 固まる | テストと同じ打ち切り時間を掛け、超えたら中断する | +| 同期コミットが取り消しで消える | 次の push で作り直される。取り消し対象にも積み直し対象にもしない | +| 実装担当が指示に反して push する | 検証は git の事実から取るため判定は変わらない。範囲外なら従来どおり失敗する | + +## 完了の定義 + +- [ ] 受け入れ条件 1〜8 をすべて満たす +- [ ] 2 つの tests ディレクトリのテストが全件成功 +- [ ] `python3 scripts/check-skill-frontmatter.py` / `claude plugin validate` が成功 +- [ ] `/ndf:cross-review` が収束 diff --git a/plugins/ndf-claude/skills/cross-refactoring/SKILL.md b/plugins/ndf-claude/skills/cross-refactoring/SKILL.md index 68fdd069..c057c122 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-claude/skills/cross-refactoring/SKILL.md @@ -38,7 +38,8 @@ allowed-tools: | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | -| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる。生成物の同期は進行側が収束後にまとめて行う | +| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | +| 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | | 検証の情報源 | **git と実際のテスト実行。** 結果ファイルの申告は検証に使わない（書き換えるだけで通る検査にしない） | | 投稿 | **AI 自身が `gh api` で投稿する。** ホストの作業文脈に差分やレビュー本文を載せない | | 状態の永続化 | `/.cross_refactoring/cross-refactoring-rf<番号>-state.json` に集約。中断・再開可能 | @@ -58,9 +59,11 @@ allowed-tools: | `--max-items-per-round N` | 1 ラウンドの採用上限 | `5` | | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | +| `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" +/ndf:cross-refactoring 130 --scope src --baseline-test "pytest -q" --sync-command "make generate" /ndf:cross-refactoring 130 --scope src --model codex=gpt-5.5 --model claude=opus-5 /ndf:cross-refactoring 130 --scope src --host codex --max-outer-rounds 1 ``` @@ -227,9 +230,8 @@ while :; do # 提案ラウンドの繰り返 rf advance "$ID" || break done -# 収束後にまとめて生成物を同期する（**進行側の責務**）。編集元から配布物を生成する -# 規約を持つリポジトリでは、実装担当に同期させると範囲外の変更が生まれる。 -# 同期が要るなら、ここで生成してから Step 7 の最終ゲートへ渡す。 +# 生成物の同期は `--sync-command` として push の直前に進行側が実行済み。 +# ここで追加の作業は要らない。 ``` ### 終了コード @@ -260,7 +262,9 @@ done | ホストのサブエージェントで適用する | ホストの作業文脈に差分が載り、実装者とレビュー担当の独立性が崩れる | | `launch-cli.sh` に「ホストなら起動しない」分岐を入れる | ホストは適用担当として起動しうる。分岐はランタイム名だけで行う | | `--scope` を省く | 提案が発散し、Pull Request が肥大する | -| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、差分予算を超える。同期は進行側が収束後にまとめて行う | +| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、範囲の検査で全件失敗する。同期は `--sync-command` で進行側が行う | +| 実装担当に push させる | 検証を通る前に公開され、取り消しの反映漏れが Pull Request に残る | +| 生成物を同期するリポジトリで `--sync-command` を省く | pre-push の検査で**あらゆる push が落ちる**。実装担当が同期に手を出し、範囲違反で全件失敗する | | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | 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 487a9b4c..45207583 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,7 +46,10 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 + 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に + すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） +8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない 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 e9b460b8..4609b02a 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 @@ -54,8 +54,44 @@ | 誰が | 何を | | --- | --- | -| 実装担当 | `--scope` の中だけを変更する。生成物・配布物の同期はしない | -| 進行側（ホスト） | 収束後にまとめて生成物を同期する | +| 実装担当 | `--scope` の中だけを変更する。生成物の同期もしないし、**push もしない** | +| 進行側 | 検証を通した後に公開する。`--sync-command` を **push の直前**に実行する | + +#### 公開するのは進行側だけである + +**実装担当に push させない。** 検証を通る前に変更が Pull Request へ現れると、 +取り消しの反映が漏れたときにそのまま残る。`merge-apply` と `merge-fix` は、 +検証が済んでから進行側として push する。 + +生成物の同期も同じ経路に乗せる。`--sync-command` を指定すると、**push の直前**に +進行側が実行し、差分があれば**どの改善項目にも属さないコミット**として積む。 + +| 案 | 結果（実測） | +| --- | --- | +| 同期しない | 同期を検査する pre-push で**あらゆる push が落ちる**。取り消しを反映できない | +| 実装担当に同期させる | 範囲外の変更になり、**採用 5 件が全件失敗**した | +| **進行側が push の直前に同期する** | 採用 | + +同期コミットは取り消しでは積み直されないが、次の push で作り直されるため失われても +問題にならない。同期に失敗したら**中断する**（終了コード 4）。同期できない状態を +公開すると、利用者のリポジトリの検査を壊したまま進むことになる。 + +**同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、同期が +作った差分と元からあった差分を区別できない。区別しようと `git status` の状態コードを +比べても足りず、次の 2 つを取りこぼす。 + +| 取りこぼし | 何が起きるか | +| --- | --- | +| 元から ` M` のファイルを同期がさらに書き換える | 状態コードが変わらず検知できない。その変更がコミットされず **push がまた落ちる** | +| 同期の前から index に staged された変更がある | `git commit` は index を丸ごと含めるため、`git add` の対象を絞っても**検証を受けない変更が公開される** | + +無視されたファイルは判定に現れない。生成物やキャッシュを `.gitignore` へ入れてあれば +止まらない。制御用ディレクトリ（状態ファイル・結果・ログ）も判定から外す。 + +**同期が途中で失敗したら、作った差分を捨ててから中断する。** 残すと次の実行は +清浄性の検査で必ず止まり、`pending_push` の再試行が永久に進まない。着手前が +綺麗だったことは確認済みなので、そこにある変更は全て同期が作ったものだと分かる。 +無視されたファイルは消さない（`git clean` に `-x` を付けない）。 判定は**前方一致だけ**で行い、除外規則は持たない。規則を書けるようにすると、 規則を 1 行足すだけで範囲の検査を骨抜きにできる。 @@ -147,9 +183,14 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 #### 取り消しは判定が出そろってからまとめて行う -**失敗した項目のコミットを Pull Request に残さない。** 実装担当は項目ごとに push して -いるため、状態を `abandoned` にするだけでは差分が残り、以後のレビュー対象にも混入する。 -何が消えるかを先に見たいときは `--dry-run` を付ける。 +**失敗した項目のコミットを Pull Request に残さない。** 公開は進行側が検証の後に行うので、 +検証で落ちた項目はそもそも公開されない。ただし取り消しはローカルの履歴にも必要である +（残すとレビュー対象と以後のラウンドに混入する）。何が消えるかを先に見たいときは +`--dry-run` を付ける。 + +取り消した項目は**「対象外」として記録する**（`deferred_items`）。記録しないと同じ提案が +次のラウンドで再び採用され、同じ理由で失敗する。実測では 3 ランタイム全員から再提案され、 +合意数が最大になって最優先で採用された。 ただし**項目ごとにその場で戻してはならない**。詳細は [取り消しは巻き戻して積み直す](#取り消しは巻き戻して積み直す)を参照する。 @@ -402,9 +443,9 @@ flowchart LR 重複率は `path` + `symbol` + `smell` の集合比較で求める。同じ提案が毎ラウンド出続けて 終わらない状態を検知するためである。 -終了後、生成物の同期が要るリポジトリでは**ここで進行側がまとめて同期する**。 -実装担当に同期させると範囲外の変更が生まれ、差分予算にも影響する（Step 4 の -「範囲の指定は検証にも効かせる」を参照）。 +**ここで追加の同期は要らない。** 生成物の同期は `--sync-command` として +各 push の直前に済んでいる（Step 4 の「公開するのは進行側だけである」を参照）。 +手で同期すると、検証を受けていない差分を作ることになる。 続けて **`/ndf:cross-review `** で Pull Request 全体を承認収束にかける。 レビューはラウンド単位なので、**ラウンドを跨いだ整合はここで見る**。 diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md index ad7fac79..bfaf491e 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md @@ -30,7 +30,6 @@ $RF_ITEMS 2. `plan` の手順を **1 手ずつ**適用する 3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** 4. 通ったらコミットする（**1 手 = 1 コミット**） -5. 項目が終わったら push する ## コミットの規約 @@ -54,13 +53,15 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 + ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます - **生成物・配布物の同期をしない。** このリポジトリに「編集元から配布物を生成する」 - 規約があっても、同期は**進行側が収束後にまとめて行う**責務です。ここで同期すると - 範囲外の変更が生まれ、差分予算も超えます + 規約があっても、同期は**進行側が公開の直前に行う**責務です。ここで同期すると + 範囲外の変更が生まれ、その項目は検証で失敗します - **機能変更を混ぜない。** 振る舞いを変える修正が必要だと分かったら、その項目は 適用せず `status` を `skipped` にして理由を書く - 提案された手順の範囲を超えない。ついでの整理をしない @@ -103,7 +104,7 @@ Impl-Model: $RF_MODEL 取り直します。ここに何と書いても検査結果は変わりません。 - `commits[].sha` は**正確に書いてください**。ここが唯一の対応付けであり、 - 検証に失敗した項目はここに書かれたコミットを取り消して push します。 + 検証に失敗した項目は、ここに書かれたコミットが取り消されます。 書き漏らすと差分が Pull Request に残ります - **このラウンドで作ったコミットは、全て `items[].commits` のどれかに入れてください。** どの項目にも割り当てられていないコミットが 1 件でもあると、**ラウンドごと diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md index b28b967d..a73f3c3e 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md @@ -26,8 +26,7 @@ $RF_ITEMS 2. 各指摘について、**修正するか・しないか**を決める - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする - 修正しない: 根拠を返信する。**黙って閉じない** -3. すべての対応が終わったら push する -4. 対応したスレッドに返信し、`resolveReviewThread` で解決する +3. 対応したスレッドに返信し、`resolveReviewThread` で解決する 指摘のうち、**振る舞いを変えないと直せないもの**は修正しないでください。 その場合は「この改善項目自体を見送るべき」と返信し、解決しないまま残します。 @@ -55,11 +54,12 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った 修正コミットがあると、その修正ラウンドの範囲ごと取り消されます -- **生成物・配布物の同期をしない。** 同期は進行側が収束後にまとめて行います +- **生成物・配布物の同期をしない。** 同期は進行側が公開の直前に行います - 指摘に無い箇所を「ついでに」直さない。ラウンドの差分が膨らみ、 どの変更がどの指摘に対応するのか追えなくなる diff --git a/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py index 3ad3134b..1a224c66 100755 --- a/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py @@ -137,6 +137,14 @@ def vocabulary() -> dict[str, Any]: # 適用で必ず配置する Skill。ここに無いものは配らない。 REQUIRED_SKILLS = ("refactoring", "tdd-cycle", "quality-gates") +# 生成物を同期したコミットのメッセージ。**どの改善項目にも属さない**ことが分かる形にする。 +SYNC_COMMIT_MESSAGE = ( + "Chore: 生成物を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "同期を検査する pre-push を持つリポジトリでも push できるよう、\n" + "公開の直前に進行側がまとめて生成する。" +) + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -468,7 +476,7 @@ def verify_scope(commit: dict[str, Any], scope: Iterable[str]) -> Optional[str]: more = f" ほか {len(outside) - 5} 件" if len(outside) > 5 else "" return ( f"コミット {commit.get('sha', '?')} が対象範囲の外を変更しています" - f"（{shown}{more}）。生成物の同期は進行側が収束後にまとめて行います。" + f"（{shown}{more}）。生成物の同期は進行側が公開の直前に行います。" "現状固定テストの置き場所が範囲外なら、`--scope` に含めてから実行してください" ) @@ -779,6 +787,8 @@ def cmd_init(args: argparse.Namespace) -> None: "max_items_per_round": args.max_items_per_round, "severity_threshold": args.severity_threshold, "baseline_test": baseline, + # 生成物の同期は**進行側の責務**。push の直前に実行する。 + "sync_command": args.sync_command, "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -1244,6 +1254,9 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: if args.dry_run: info("（dry-run）状態ファイルは更新していません") else: + # 項目別の失敗と同じく、**ここで取り消した項目も「対象外」に残す**。 + # 残さないと同じ提案が次のラウンドで再び採用される。 + _defer_abandoned_items(state, entry) statefile.save(path, state) _push_head(state) entry["pending_push"] = False @@ -1323,7 +1336,13 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: # `merged_at` は `_apply_drop` が取り消しの完了時点で立てる。 applied = _apply_drop(path, state, entry, failed) else: + # **全項目が通ったときも進行側が公開する。** 実装担当は push しないため、 + # ここで公開しないとレビュー担当が Pull Request 上の差分へ指摘を書けない。 entry["apply"]["merged_at"] = statefile.now() + entry["pending_push"] = True + statefile.save(path, state) + _push_head(state) + entry["pending_push"] = False statefile.save(path, state) if not applied: @@ -1331,6 +1350,29 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: sys.exit(2) +def _defer_abandoned_items(state: dict[str, Any], entry: dict[str, Any]) -> None: + """このラウンドで取り消した項目を「対象外」として記録する。 + + 記録しないと、**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では適用で失敗した項目が 3 ランタイム全員から再提案され、合意数が最大に + なって最優先で採用された。手順書が「同じ提案が毎ラウンド出続けて収束しない」 + として禁じている状態そのものである。 + + 除外の鍵は `path` + `symbol` + `smell` なので、その 3 つを必ず残す。 + """ + already = {d.get("item_id") for d in state["deferred_items"]} + for item_id in entry["items"]: + item = _find_item(state, item_id, required=False) + if item is None or item.get("status") != "abandoned" or item_id in already: + continue + state["deferred_items"].append({ + "item_id": item_id, + "path": item["path"], "symbol": item["symbol"], "smell": item["smell"], + "round": entry["round"], + "defer_reason": item.get("failure_reason") or "適用結果の検証を通らなかった", + }) + + def _run_drop( path: pathlib.Path, state: dict[str, Any], entry: dict[str, Any], targets: list[str], @@ -1391,6 +1433,9 @@ def _apply_drop( entry["apply_base_sha"] = _git_out(work, ["rev-parse", "HEAD"]) state["phase"] = "propose" + # 取り消した項目は「対象外」として残す。次のラウンドで同じ提案が採用され、 + # 同じ理由で失敗するのを防ぐ。 + _defer_abandoned_items(state, entry) # **取り消しが済んだことを push より先に、印の解除と同じ保存で永続化する。** # 保存せずに push して失敗すると、次の実行が適用の検証をやり直し、取り消しと # 積み直しのコミットを「未割当」と判定してラウンドごと巻き込んでしまう。 @@ -1725,7 +1770,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: # 適用フェーズの未割当コミットと同じ扱いにする。 problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) - needs_push = False for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, state.get("target_scope") or []) @@ -1762,7 +1806,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: statefile.save(path, state) # **push は保存のあと。** ここで push して失敗すると、取り消しコミットは # ローカルに残るのに起点の更新が保存されず、叩き直しで二重に取り消してしまう。 - needs_push = True info("⚠ 修正を取り消したため、解決の申告は採用しません") resolved = set() else: @@ -1785,8 +1828,9 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: + _safe_int(payload.get("elapsed_seconds")) ) statefile.save(path, state) - if needs_push: - _push_with_retry_marker(path, state, entry) + # **取り消したかどうかに関わらず公開する。** 実装担当は push しないため、 + # ここで公開しないと再レビューが Pull Request 上の差分を見られない。 + _push_with_retry_marker(path, state, entry) info(f"修正を取り込みました（解決 {len(resolved)} スレッド / 修正ラウンド {entry['fix_rounds']}）") @@ -2471,8 +2515,146 @@ def _order_newest_first(work: str, shas: list[str]) -> list[str]: return sorted(shas, key=lambda s: rank.get(resolved[s], len(rank))) +def _worktree_changes(work: str) -> dict[str, str]: + """作業ツリーの変更を `パス → 状態` で返す。同期の前後を比べるために使う。 + + 無視されているファイルは現れない（`--porcelain` の既定）。改名は移動先の + パスだけを見る。 + """ + # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ + # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 + out = _git_out( + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + ) + changes: dict[str, str] = {} + for line in (out or "").splitlines(): + if len(line) < 4: + continue + path = line[3:] + if " -> " in path: # 改名。移動先だけを対象にする + path = path.split(" -> ", 1)[1] + changes[path.strip('"')] = line[:2] + return changes + + +def _control_prefix(state: dict[str, Any], work: str) -> Optional[str]: + """作業ディレクトリから見た制御用ディレクトリの相対パス。外にあれば `None`。 + + 状態ファイル・プロンプト・結果・ログの置き場所で、**同期コミットへ入れない**。 + `prepare-worktrees.sh` が無視の設定を置くが、置き場所を環境変数で移した場合や + 配置前に同期が走った場合に備えて、ここでも明示的に外す。 + """ + tmp_dir = str(state.get("tmp_dir") or "") + if not tmp_dir: + return None + try: + relative = pathlib.Path(tmp_dir).resolve().relative_to( + pathlib.Path(work).resolve() + ) + except ValueError: + return None + return f"{relative}/" + + +def _dirty_paths(state: dict[str, Any], work: str) -> list[str]: + """作業ツリーの未コミット変更のパス。制御用ディレクトリは除く。""" + control = _control_prefix(state, work) + return sorted( + path for path in _worktree_changes(work) + if not (control and path.startswith(control)) + ) + + +def _discard_worktree_changes(work: str) -> None: + """作業ツリーと index の未コミット変更を捨てる。**着手前が綺麗なときだけ呼ぶ。** + + **index も戻す。** `git checkout -- .` は staged された差分を戻さないため、 + 同期コマンドが `git add` してから失敗すると清浄性の検査が通らないままになり、 + `pending_push` の再試行が永久に進まない。 + + 無視されたファイル（制御用ディレクトリを含む）は消さない（`git clean` に + `-x` を付けない）。 + """ + for args in (["reset", "--hard", "HEAD"], ["clean", "-fd"]): + subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) + + +def _require_clean_worktree(state: dict[str, Any], work: str) -> None: + """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 + + 汚れたまま同期すると、**同期が作った差分と元からあった差分を区別できない**。 + 区別しようと状態コードを比べても足りず、次の 2 つを取りこぼす。 + + - 元から ` M` のファイルを同期がさらに書き換えても、状態コードは ` M` のままで + 検知できない。その変更がコミットされず、**push がまた落ちる** + - `git commit` は index の内容を全て含めるため、`git add` の対象を絞っても + **先に staged だった変更が検証を受けないまま Pull Request へ入る** + + 無視されたファイルはここに現れない。生成物やキャッシュを `.gitignore` へ + 入れてあれば止まらない。 + """ + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = ", ".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + die( + f"生成物を同期する前に、作業ツリーへ未コミットの変更があります（{shown}{more}）。" + "同期が作った差分と区別できず、検証を受けていない変更を公開しかねないため" + "中断します。コミットするか `.gitignore` へ入れてから再実行してください" + ) + + +def _sync_generated(state: dict[str, Any]) -> None: + """push の直前に生成物を同期し、差分があれば進行側のコミットとして積む。 + + 同期を**実装担当の責務にすると範囲外の変更が生まれ**、範囲の検査で全件失敗する + （実測ではラウンドの採用 5 件が全て範囲外で落ちた）。かといって同期しないと、 + 生成物の同期を検査する pre-push を持つリポジトリでは push そのものが通らず、 + 取り消しを Pull Request へ反映できない。そこで**進行側が push の直前に同期する**。 + + このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 + 次の push で作り直されるので失われても問題にならない。 + + 同期に失敗したら中断する。**黙って push しない。** 同期できない状態を公開すると、 + 利用者のリポジトリの検査を壊したまま進むことになる。 + """ + command = str(state.get("sync_command") or "").strip() + if not command: + return + work = state["worktrees"]["work"] + # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 + # 同期が作った差分と元からあった差分を区別できない。 + _require_clean_worktree(state, work) + code, timed_out = _run_with_timeout( + command, work, _safe_int(state.get("test_timeout"), DEFAULT_TEST_TIMEOUT) + ) + if timed_out or code != 0: + # **途中まで書き換えた差分を残さない。** 残すと次の実行は + # `_require_clean_worktree` で必ず止まり、`pending_push` の再試行が + # 永久に進まなくなる。着手前が綺麗だったことは確認済みなので、 + # ここにある変更は全て同期が作ったものだと分かる。 + _discard_worktree_changes(work) + die( + f"生成物の同期に失敗しました（{command}）: " + + ("打ち切りました" if timed_out else f"終了コード {code}") + + "。同期が作った差分は破棄したので、原因を直せばそのまま再開できます" + ) + produced = _dirty_paths(state, work) + if not produced: + return + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + + def _push_head(state: dict[str, Any]) -> None: - """head ブランチへ push する。**`--force` は使わない。**""" + """head ブランチへ push する。**`--force` は使わない。** + + **公開するのは進行側だけである。** 実装担当に push させると、検証を通る前に + 変更が Pull Request へ現れ、取り消しの反映漏れがそのまま残る。 + """ + _sync_generated(state) _sh( ["git", "push", "origin", f"HEAD:{state['head_branch']}"], cwd=state["worktrees"]["work"], @@ -2607,6 +2789,10 @@ def main() -> None: init.add_argument("--test-timeout", type=int, default=DEFAULT_TEST_TIMEOUT, help="テスト 1 回あたりの上限秒数。超えたら失敗として扱う " f"(default: {DEFAULT_TEST_TIMEOUT})") + init.add_argument("--sync-command", default=None, + help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" + "差分があれば進行側のコミットとして積む。" + "同期を実装担当にさせると範囲外の変更になるため分離している") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_abandon_items.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_abandon_items.py index 63b2bb64..41928945 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/tests/test_abandon_items.py +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_abandon_items.py @@ -244,6 +244,8 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: resolved_facts, ) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": claimed, "elapsed_seconds": 12, @@ -396,6 +398,8 @@ def test_broken_fix_result_does_not_crash( """ state_path = _state(tmp_path, [_finding("R1-001")]) env_tmp_dir(state_path) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": broken_ids, "commits": {"sha": "辞書ではあるが配列でない"}, 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 4987b7d6..2427ed3a 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_init.py @@ -59,6 +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, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } 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 6b772a0c..28aa9cb7 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 @@ -383,7 +383,7 @@ def test_self_reported_values_cannot_pass_the_check( assert "テストが成功していません" in state["items"][0]["failure_reason"] -def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 @@ -392,6 +392,7 @@ def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): calls: list[list[str]] = [] picked: list[str] = [] reverted: list[str] = [] + statuses: list[int] = [] def fake_run(cmd, **kwargs): calls.append(list(cmd)) @@ -413,6 +414,14 @@ def fake_git_out(work, args): if picked: return f"new-{picked[-1]}" return "REVERTED_HEAD" if reverted else "HEAD_BEFORE" + if "status" in args: + # 同期の前後で 2 回呼ばれる。1 回目が同期前、2 回目以降が同期後。 + # 既定は「同期前も後も差分なし」 + statuses.append(len(statuses)) + if sync_dirty is False: + return "" + before, after = sync_dirty + return before if len(statuses) == 1 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -647,10 +656,13 @@ def test_blank_scope_entry_is_ignored(refactor): assert not refactor.path_in_scope("dist/foo.py", ["", " ", "src"]) -def test_no_push_when_nothing_was_reverted( +def test_push_happens_once_per_merge_apply( refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts ): - """全項目が通ったときに余計な push をしない。""" + """全項目が通ったときも公開するが、push は 1 回だけにすること。 + + 公開するのは進行側だけである（実装担当は push しない）。 + """ items = [item(item_id="R1-001")] state_path = _state_with_items(tmp_path, items) env_tmp_dir(state_path) @@ -666,7 +678,7 @@ def test_no_push_when_nothing_was_reverted( lambda cmd, **kw: subprocess.CompletedProcess(cmd, 0, "", ""), ) refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) - assert pushes == [] + assert len([c for c in pushes if c[:2] == ["git", "push"]]) == 1 def test_dry_run_touches_neither_git_nor_state( @@ -1205,3 +1217,330 @@ def test_push_failure_after_a_successful_drop_only_retries_the_push( entry = read_state(state_path)["rounds"][0] assert entry["pending_push"] is False assert entry["apply"]["applied"] == ["R1-001"] + + +# ---------- 適用で失敗した項目を「対象外」へ ---------- + +def test_failed_items_are_deferred( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """適用の検証で失敗した項目を「対象外」として記録すること。 + + 記録しないと**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では 3 ランタイム全員から再提案され、合意数が最大になって最優先で採用された。 + """ + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-002"], "失敗した項目だけを対象外にすること" + entry = deferred["R1-002"] + # 次ラウンドの除外は path + symbol + smell の組で行われる + assert (entry["path"], entry["symbol"], entry["smell"]) == ( + "src/foo.py", "Foo.handle", "long_method") + assert "差分予算" in entry["defer_reason"] + + +def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts): + """叩き直しても対象外の記録を重複させないこと。""" + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + args = type("A", (), {"id": 130, "round": 1, "dry_run": False})() + refactor.cmd_merge_apply(args) + refactor.cmd_merge_apply(args) + assert [d["item_id"] for d in read_state(state_path)["deferred_items"]] == ["R1-002"] + + +# ---------- push の直前に生成物を同期する ---------- + +def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["sync_command"] = command + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + return state_path + + +def test_push_syncs_generated_files_first( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """push の直前に同期し、差分があれば進行側のコミットとして積むこと。 + + 同期を実装担当にさせると範囲外の変更になり、範囲の検査で全件失敗する。 + かといって同期しないと、同期を検査する pre-push では push が通らない。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + order: list[str] = [] + _drop_env(refactor, monkeypatch, + sync_dirty=("", "?? plugins/generated/a.py")) + monkeypatch.setattr( + refactor, "_sh", + lambda cmd, **k: order.append("push" if cmd[:2] == ["git", "push"] else cmd[1]) + or "", + ) + ran: list[tuple[str, str]] = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append((command, cwd)) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert ran and ran[0][0] == "make build", "同期コマンドを実行していない" + assert "add" in order and "commit" in order, "同期の差分をコミットしていない" + assert order.index("commit") < order.index("push"), "コミットより先に push している" + + +def test_sync_failure_aborts_without_pushing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期に失敗したら中断する。黙って push しない。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_no_sync_command_runs_nothing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """`--sync-command` 未指定なら同期は走らない（既存の利用者に影響しない）。""" + _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert ran == [] + + +def test_merge_apply_pushes_even_when_every_item_passes( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """全項目が通ったときも進行側が push すること。 + + 実装担当が push しなくなったため、ここで公開しないとレビュー担当が + Pull Request 上の差分へ指摘を書けない。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + git_facts({"ok111": fact(sha="ok111")}) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + calls, pushes = _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in pushes if c[:2] == ["git", "push"]], "push していない" + for cmd in pushes: + assert "--force" not in cmd and "--no-verify" not in cmd + entry = read_state(state_path)["rounds"][0] + assert entry["pending_push"] is False + assert entry["apply"]["applied"] == ["R1-001"] + + +def test_sync_aborts_when_the_worktree_is_dirty( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期の前に作業ツリーが汚れていたら中断すること。 + + 汚れたまま同期すると、同期が作った差分と元からあった差分を区別できない。 + 状態コードを比べても、元から ` M` のファイルを同期がさらに書き換えた場合を + 取りこぼす。`git commit` は index を丸ごと含めるため、staged 済みの変更も + 検証を受けないまま公開される。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(" M src/edited.py", " M src/edited.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert ran == [], "汚れたまま同期を走らせている" + assert [c for c in staged if c[:2] == ["git", "push"]] == [] + + +def test_dirt_inside_the_control_directory_does_not_abort( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """制御用ディレクトリの中は汚れていても止めないこと。 + + 状態ファイル・結果・ログは常にそこへ書かれる。 + """ + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(f"?? {control}/codex-apply-r1-result.json", + f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert [c for c in staged if c[:2] == ["git", "add"]] == [ + ["git", "add", "--", "generated/a.py"]] + + +def test_sync_excludes_the_control_directory( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """状態ファイル・結果・ログの置き場所を同期コミットへ入れないこと。""" + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + # 既定の配置では制御用ディレクトリが作業ディレクトリの中にある + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name # `.cross_refactoring` + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=("", f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + adds = [c for c in staged if c[:2] == ["git", "add"]] + assert adds == [["git", "add", "--", "generated/a.py"]] + + +def test_sync_without_changes_makes_no_commit( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期しても差分が出なければ、空のコミットを積まないこと。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env(refactor, monkeypatch, sync_dirty=("", "")) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in staged if c[:2] == ["git", "commit"]] == [] + assert [c for c in staged if c[:2] == ["git", "push"]], "push はすること" + + +def test_whole_round_failure_also_defers_items( + refactor, tmp_path, env_tmp_dir, no_git, git_facts +): + """ラウンドごと取り消す経路でも「対象外」として記録すること。 + + 項目別の失敗だけを記録すると、未割当コミットで落ちた提案が次のラウンドで + 再び採用される。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + # 範囲に 2 件あるが、申告は 1 件だけ（未割当コミットあり） + git_facts({"ok111": fact(sha="ok111")}, in_range=["sneaky", "ok111"]) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == 2 + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-001"] + assert "割り当てられていない" in deferred["R1-001"]["defer_reason"] + + +def test_sync_failure_discards_what_it_produced( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が途中で失敗したら、作った差分を捨てて再開できる状態にすること。 + + 残すと次の実行は清浄性の検査で必ず止まり、`pending_push` の再試行が + 永久に進まなくなる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", " M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + # index も戻す。`git checkout -- .` では staged された差分が残り、 + # 清浄性の検査が通らないままになる + assert ["git", "reset", "--hard", "HEAD"] in calls, "index を戻していない" + assert ["git", "clean", "-fd"] in calls, "同期が作ったファイルを消していない" + # 無視されたファイル（制御用ディレクトリ）まで消さない + assert not any("-x" in c for c in calls if c[:2] == ["git", "clean"]) + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_status_disables_path_quoting(refactor, monkeypatch): + """`core.quotePath` の既定では非 ASCII のパスがエスケープされて `git add` が失敗する。""" + seen: list[list[str]] = [] + monkeypatch.setattr( + refactor, "_git_out", + lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + ) + assert refactor._worktree_changes("/w") == {"plugins/日本語/a.py": " M"} + assert seen[0][:2] == ["-c", "core.quotePath=false"] + + +def test_sync_failure_also_resets_the_index( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が `git add` してから失敗しても、次の実行が再開できること。 + + `git checkout -- .` は staged された差分を戻さないため、index も戻さないと + 清浄性の検査が通らないままになる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + # 同期が index へ追加してから失敗した状況（`M ` は staged） + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", "M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit): + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert ["git", "reset", "--hard", "HEAD"] in calls + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] diff --git a/plugins/ndf-codex/skills/cross-refactoring/SKILL.md b/plugins/ndf-codex/skills/cross-refactoring/SKILL.md index 9821cb8a..4ca3d399 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-codex/skills/cross-refactoring/SKILL.md @@ -38,7 +38,8 @@ allowed-tools: | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | -| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる。生成物の同期は進行側が収束後にまとめて行う | +| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | +| 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | | 検証の情報源 | **git と実際のテスト実行。** 結果ファイルの申告は検証に使わない（書き換えるだけで通る検査にしない） | | 投稿 | **AI 自身が `gh api` で投稿する。** ホストの作業文脈に差分やレビュー本文を載せない | | 状態の永続化 | `/.cross_refactoring/cross-refactoring-rf<番号>-state.json` に集約。中断・再開可能 | @@ -58,9 +59,11 @@ allowed-tools: | `--max-items-per-round N` | 1 ラウンドの採用上限 | `5` | | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | +| `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" +/ndf:cross-refactoring 130 --scope src --baseline-test "pytest -q" --sync-command "make generate" /ndf:cross-refactoring 130 --scope src --model codex=gpt-5.5 --model claude=opus-5 /ndf:cross-refactoring 130 --scope src --host codex --max-outer-rounds 1 ``` @@ -227,9 +230,8 @@ while :; do # 提案ラウンドの繰り返 rf advance "$ID" || break done -# 収束後にまとめて生成物を同期する（**進行側の責務**）。編集元から配布物を生成する -# 規約を持つリポジトリでは、実装担当に同期させると範囲外の変更が生まれる。 -# 同期が要るなら、ここで生成してから Step 7 の最終ゲートへ渡す。 +# 生成物の同期は `--sync-command` として push の直前に進行側が実行済み。 +# ここで追加の作業は要らない。 ``` ### 終了コード @@ -260,7 +262,9 @@ done | ホストのサブエージェントで適用する | ホストの作業文脈に差分が載り、実装者とレビュー担当の独立性が崩れる | | `launch-cli.sh` に「ホストなら起動しない」分岐を入れる | ホストは適用担当として起動しうる。分岐はランタイム名だけで行う | | `--scope` を省く | 提案が発散し、Pull Request が肥大する | -| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、差分予算を超える。同期は進行側が収束後にまとめて行う | +| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、範囲の検査で全件失敗する。同期は `--sync-command` で進行側が行う | +| 実装担当に push させる | 検証を通る前に公開され、取り消しの反映漏れが Pull Request に残る | +| 生成物を同期するリポジトリで `--sync-command` を省く | pre-push の検査で**あらゆる push が落ちる**。実装担当が同期に手を出し、範囲違反で全件失敗する | | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | 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 487a9b4c..45207583 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,7 +46,10 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 + 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に + すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） +8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない 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 e9b460b8..4609b02a 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 @@ -54,8 +54,44 @@ | 誰が | 何を | | --- | --- | -| 実装担当 | `--scope` の中だけを変更する。生成物・配布物の同期はしない | -| 進行側（ホスト） | 収束後にまとめて生成物を同期する | +| 実装担当 | `--scope` の中だけを変更する。生成物の同期もしないし、**push もしない** | +| 進行側 | 検証を通した後に公開する。`--sync-command` を **push の直前**に実行する | + +#### 公開するのは進行側だけである + +**実装担当に push させない。** 検証を通る前に変更が Pull Request へ現れると、 +取り消しの反映が漏れたときにそのまま残る。`merge-apply` と `merge-fix` は、 +検証が済んでから進行側として push する。 + +生成物の同期も同じ経路に乗せる。`--sync-command` を指定すると、**push の直前**に +進行側が実行し、差分があれば**どの改善項目にも属さないコミット**として積む。 + +| 案 | 結果（実測） | +| --- | --- | +| 同期しない | 同期を検査する pre-push で**あらゆる push が落ちる**。取り消しを反映できない | +| 実装担当に同期させる | 範囲外の変更になり、**採用 5 件が全件失敗**した | +| **進行側が push の直前に同期する** | 採用 | + +同期コミットは取り消しでは積み直されないが、次の push で作り直されるため失われても +問題にならない。同期に失敗したら**中断する**（終了コード 4）。同期できない状態を +公開すると、利用者のリポジトリの検査を壊したまま進むことになる。 + +**同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、同期が +作った差分と元からあった差分を区別できない。区別しようと `git status` の状態コードを +比べても足りず、次の 2 つを取りこぼす。 + +| 取りこぼし | 何が起きるか | +| --- | --- | +| 元から ` M` のファイルを同期がさらに書き換える | 状態コードが変わらず検知できない。その変更がコミットされず **push がまた落ちる** | +| 同期の前から index に staged された変更がある | `git commit` は index を丸ごと含めるため、`git add` の対象を絞っても**検証を受けない変更が公開される** | + +無視されたファイルは判定に現れない。生成物やキャッシュを `.gitignore` へ入れてあれば +止まらない。制御用ディレクトリ（状態ファイル・結果・ログ）も判定から外す。 + +**同期が途中で失敗したら、作った差分を捨ててから中断する。** 残すと次の実行は +清浄性の検査で必ず止まり、`pending_push` の再試行が永久に進まない。着手前が +綺麗だったことは確認済みなので、そこにある変更は全て同期が作ったものだと分かる。 +無視されたファイルは消さない（`git clean` に `-x` を付けない）。 判定は**前方一致だけ**で行い、除外規則は持たない。規則を書けるようにすると、 規則を 1 行足すだけで範囲の検査を骨抜きにできる。 @@ -147,9 +183,14 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 #### 取り消しは判定が出そろってからまとめて行う -**失敗した項目のコミットを Pull Request に残さない。** 実装担当は項目ごとに push して -いるため、状態を `abandoned` にするだけでは差分が残り、以後のレビュー対象にも混入する。 -何が消えるかを先に見たいときは `--dry-run` を付ける。 +**失敗した項目のコミットを Pull Request に残さない。** 公開は進行側が検証の後に行うので、 +検証で落ちた項目はそもそも公開されない。ただし取り消しはローカルの履歴にも必要である +（残すとレビュー対象と以後のラウンドに混入する）。何が消えるかを先に見たいときは +`--dry-run` を付ける。 + +取り消した項目は**「対象外」として記録する**（`deferred_items`）。記録しないと同じ提案が +次のラウンドで再び採用され、同じ理由で失敗する。実測では 3 ランタイム全員から再提案され、 +合意数が最大になって最優先で採用された。 ただし**項目ごとにその場で戻してはならない**。詳細は [取り消しは巻き戻して積み直す](#取り消しは巻き戻して積み直す)を参照する。 @@ -402,9 +443,9 @@ flowchart LR 重複率は `path` + `symbol` + `smell` の集合比較で求める。同じ提案が毎ラウンド出続けて 終わらない状態を検知するためである。 -終了後、生成物の同期が要るリポジトリでは**ここで進行側がまとめて同期する**。 -実装担当に同期させると範囲外の変更が生まれ、差分予算にも影響する（Step 4 の -「範囲の指定は検証にも効かせる」を参照）。 +**ここで追加の同期は要らない。** 生成物の同期は `--sync-command` として +各 push の直前に済んでいる（Step 4 の「公開するのは進行側だけである」を参照）。 +手で同期すると、検証を受けていない差分を作ることになる。 続けて **`/ndf:cross-review `** で Pull Request 全体を承認収束にかける。 レビューはラウンド単位なので、**ラウンドを跨いだ整合はここで見る**。 diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md index ad7fac79..bfaf491e 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md @@ -30,7 +30,6 @@ $RF_ITEMS 2. `plan` の手順を **1 手ずつ**適用する 3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** 4. 通ったらコミットする（**1 手 = 1 コミット**） -5. 項目が終わったら push する ## コミットの規約 @@ -54,13 +53,15 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 + ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます - **生成物・配布物の同期をしない。** このリポジトリに「編集元から配布物を生成する」 - 規約があっても、同期は**進行側が収束後にまとめて行う**責務です。ここで同期すると - 範囲外の変更が生まれ、差分予算も超えます + 規約があっても、同期は**進行側が公開の直前に行う**責務です。ここで同期すると + 範囲外の変更が生まれ、その項目は検証で失敗します - **機能変更を混ぜない。** 振る舞いを変える修正が必要だと分かったら、その項目は 適用せず `status` を `skipped` にして理由を書く - 提案された手順の範囲を超えない。ついでの整理をしない @@ -103,7 +104,7 @@ Impl-Model: $RF_MODEL 取り直します。ここに何と書いても検査結果は変わりません。 - `commits[].sha` は**正確に書いてください**。ここが唯一の対応付けであり、 - 検証に失敗した項目はここに書かれたコミットを取り消して push します。 + 検証に失敗した項目は、ここに書かれたコミットが取り消されます。 書き漏らすと差分が Pull Request に残ります - **このラウンドで作ったコミットは、全て `items[].commits` のどれかに入れてください。** どの項目にも割り当てられていないコミットが 1 件でもあると、**ラウンドごと diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md index b28b967d..a73f3c3e 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md @@ -26,8 +26,7 @@ $RF_ITEMS 2. 各指摘について、**修正するか・しないか**を決める - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする - 修正しない: 根拠を返信する。**黙って閉じない** -3. すべての対応が終わったら push する -4. 対応したスレッドに返信し、`resolveReviewThread` で解決する +3. 対応したスレッドに返信し、`resolveReviewThread` で解決する 指摘のうち、**振る舞いを変えないと直せないもの**は修正しないでください。 その場合は「この改善項目自体を見送るべき」と返信し、解決しないまま残します。 @@ -55,11 +54,12 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った 修正コミットがあると、その修正ラウンドの範囲ごと取り消されます -- **生成物・配布物の同期をしない。** 同期は進行側が収束後にまとめて行います +- **生成物・配布物の同期をしない。** 同期は進行側が公開の直前に行います - 指摘に無い箇所を「ついでに」直さない。ラウンドの差分が膨らみ、 どの変更がどの指摘に対応するのか追えなくなる diff --git a/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py index 3ad3134b..1a224c66 100755 --- a/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py @@ -137,6 +137,14 @@ def vocabulary() -> dict[str, Any]: # 適用で必ず配置する Skill。ここに無いものは配らない。 REQUIRED_SKILLS = ("refactoring", "tdd-cycle", "quality-gates") +# 生成物を同期したコミットのメッセージ。**どの改善項目にも属さない**ことが分かる形にする。 +SYNC_COMMIT_MESSAGE = ( + "Chore: 生成物を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "同期を検査する pre-push を持つリポジトリでも push できるよう、\n" + "公開の直前に進行側がまとめて生成する。" +) + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -468,7 +476,7 @@ def verify_scope(commit: dict[str, Any], scope: Iterable[str]) -> Optional[str]: more = f" ほか {len(outside) - 5} 件" if len(outside) > 5 else "" return ( f"コミット {commit.get('sha', '?')} が対象範囲の外を変更しています" - f"（{shown}{more}）。生成物の同期は進行側が収束後にまとめて行います。" + f"（{shown}{more}）。生成物の同期は進行側が公開の直前に行います。" "現状固定テストの置き場所が範囲外なら、`--scope` に含めてから実行してください" ) @@ -779,6 +787,8 @@ def cmd_init(args: argparse.Namespace) -> None: "max_items_per_round": args.max_items_per_round, "severity_threshold": args.severity_threshold, "baseline_test": baseline, + # 生成物の同期は**進行側の責務**。push の直前に実行する。 + "sync_command": args.sync_command, "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -1244,6 +1254,9 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: if args.dry_run: info("（dry-run）状態ファイルは更新していません") else: + # 項目別の失敗と同じく、**ここで取り消した項目も「対象外」に残す**。 + # 残さないと同じ提案が次のラウンドで再び採用される。 + _defer_abandoned_items(state, entry) statefile.save(path, state) _push_head(state) entry["pending_push"] = False @@ -1323,7 +1336,13 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: # `merged_at` は `_apply_drop` が取り消しの完了時点で立てる。 applied = _apply_drop(path, state, entry, failed) else: + # **全項目が通ったときも進行側が公開する。** 実装担当は push しないため、 + # ここで公開しないとレビュー担当が Pull Request 上の差分へ指摘を書けない。 entry["apply"]["merged_at"] = statefile.now() + entry["pending_push"] = True + statefile.save(path, state) + _push_head(state) + entry["pending_push"] = False statefile.save(path, state) if not applied: @@ -1331,6 +1350,29 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: sys.exit(2) +def _defer_abandoned_items(state: dict[str, Any], entry: dict[str, Any]) -> None: + """このラウンドで取り消した項目を「対象外」として記録する。 + + 記録しないと、**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では適用で失敗した項目が 3 ランタイム全員から再提案され、合意数が最大に + なって最優先で採用された。手順書が「同じ提案が毎ラウンド出続けて収束しない」 + として禁じている状態そのものである。 + + 除外の鍵は `path` + `symbol` + `smell` なので、その 3 つを必ず残す。 + """ + already = {d.get("item_id") for d in state["deferred_items"]} + for item_id in entry["items"]: + item = _find_item(state, item_id, required=False) + if item is None or item.get("status") != "abandoned" or item_id in already: + continue + state["deferred_items"].append({ + "item_id": item_id, + "path": item["path"], "symbol": item["symbol"], "smell": item["smell"], + "round": entry["round"], + "defer_reason": item.get("failure_reason") or "適用結果の検証を通らなかった", + }) + + def _run_drop( path: pathlib.Path, state: dict[str, Any], entry: dict[str, Any], targets: list[str], @@ -1391,6 +1433,9 @@ def _apply_drop( entry["apply_base_sha"] = _git_out(work, ["rev-parse", "HEAD"]) state["phase"] = "propose" + # 取り消した項目は「対象外」として残す。次のラウンドで同じ提案が採用され、 + # 同じ理由で失敗するのを防ぐ。 + _defer_abandoned_items(state, entry) # **取り消しが済んだことを push より先に、印の解除と同じ保存で永続化する。** # 保存せずに push して失敗すると、次の実行が適用の検証をやり直し、取り消しと # 積み直しのコミットを「未割当」と判定してラウンドごと巻き込んでしまう。 @@ -1725,7 +1770,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: # 適用フェーズの未割当コミットと同じ扱いにする。 problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) - needs_push = False for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, state.get("target_scope") or []) @@ -1762,7 +1806,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: statefile.save(path, state) # **push は保存のあと。** ここで push して失敗すると、取り消しコミットは # ローカルに残るのに起点の更新が保存されず、叩き直しで二重に取り消してしまう。 - needs_push = True info("⚠ 修正を取り消したため、解決の申告は採用しません") resolved = set() else: @@ -1785,8 +1828,9 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: + _safe_int(payload.get("elapsed_seconds")) ) statefile.save(path, state) - if needs_push: - _push_with_retry_marker(path, state, entry) + # **取り消したかどうかに関わらず公開する。** 実装担当は push しないため、 + # ここで公開しないと再レビューが Pull Request 上の差分を見られない。 + _push_with_retry_marker(path, state, entry) info(f"修正を取り込みました（解決 {len(resolved)} スレッド / 修正ラウンド {entry['fix_rounds']}）") @@ -2471,8 +2515,146 @@ def _order_newest_first(work: str, shas: list[str]) -> list[str]: return sorted(shas, key=lambda s: rank.get(resolved[s], len(rank))) +def _worktree_changes(work: str) -> dict[str, str]: + """作業ツリーの変更を `パス → 状態` で返す。同期の前後を比べるために使う。 + + 無視されているファイルは現れない（`--porcelain` の既定）。改名は移動先の + パスだけを見る。 + """ + # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ + # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 + out = _git_out( + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + ) + changes: dict[str, str] = {} + for line in (out or "").splitlines(): + if len(line) < 4: + continue + path = line[3:] + if " -> " in path: # 改名。移動先だけを対象にする + path = path.split(" -> ", 1)[1] + changes[path.strip('"')] = line[:2] + return changes + + +def _control_prefix(state: dict[str, Any], work: str) -> Optional[str]: + """作業ディレクトリから見た制御用ディレクトリの相対パス。外にあれば `None`。 + + 状態ファイル・プロンプト・結果・ログの置き場所で、**同期コミットへ入れない**。 + `prepare-worktrees.sh` が無視の設定を置くが、置き場所を環境変数で移した場合や + 配置前に同期が走った場合に備えて、ここでも明示的に外す。 + """ + tmp_dir = str(state.get("tmp_dir") or "") + if not tmp_dir: + return None + try: + relative = pathlib.Path(tmp_dir).resolve().relative_to( + pathlib.Path(work).resolve() + ) + except ValueError: + return None + return f"{relative}/" + + +def _dirty_paths(state: dict[str, Any], work: str) -> list[str]: + """作業ツリーの未コミット変更のパス。制御用ディレクトリは除く。""" + control = _control_prefix(state, work) + return sorted( + path for path in _worktree_changes(work) + if not (control and path.startswith(control)) + ) + + +def _discard_worktree_changes(work: str) -> None: + """作業ツリーと index の未コミット変更を捨てる。**着手前が綺麗なときだけ呼ぶ。** + + **index も戻す。** `git checkout -- .` は staged された差分を戻さないため、 + 同期コマンドが `git add` してから失敗すると清浄性の検査が通らないままになり、 + `pending_push` の再試行が永久に進まない。 + + 無視されたファイル（制御用ディレクトリを含む）は消さない（`git clean` に + `-x` を付けない）。 + """ + for args in (["reset", "--hard", "HEAD"], ["clean", "-fd"]): + subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) + + +def _require_clean_worktree(state: dict[str, Any], work: str) -> None: + """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 + + 汚れたまま同期すると、**同期が作った差分と元からあった差分を区別できない**。 + 区別しようと状態コードを比べても足りず、次の 2 つを取りこぼす。 + + - 元から ` M` のファイルを同期がさらに書き換えても、状態コードは ` M` のままで + 検知できない。その変更がコミットされず、**push がまた落ちる** + - `git commit` は index の内容を全て含めるため、`git add` の対象を絞っても + **先に staged だった変更が検証を受けないまま Pull Request へ入る** + + 無視されたファイルはここに現れない。生成物やキャッシュを `.gitignore` へ + 入れてあれば止まらない。 + """ + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = ", ".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + die( + f"生成物を同期する前に、作業ツリーへ未コミットの変更があります（{shown}{more}）。" + "同期が作った差分と区別できず、検証を受けていない変更を公開しかねないため" + "中断します。コミットするか `.gitignore` へ入れてから再実行してください" + ) + + +def _sync_generated(state: dict[str, Any]) -> None: + """push の直前に生成物を同期し、差分があれば進行側のコミットとして積む。 + + 同期を**実装担当の責務にすると範囲外の変更が生まれ**、範囲の検査で全件失敗する + （実測ではラウンドの採用 5 件が全て範囲外で落ちた）。かといって同期しないと、 + 生成物の同期を検査する pre-push を持つリポジトリでは push そのものが通らず、 + 取り消しを Pull Request へ反映できない。そこで**進行側が push の直前に同期する**。 + + このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 + 次の push で作り直されるので失われても問題にならない。 + + 同期に失敗したら中断する。**黙って push しない。** 同期できない状態を公開すると、 + 利用者のリポジトリの検査を壊したまま進むことになる。 + """ + command = str(state.get("sync_command") or "").strip() + if not command: + return + work = state["worktrees"]["work"] + # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 + # 同期が作った差分と元からあった差分を区別できない。 + _require_clean_worktree(state, work) + code, timed_out = _run_with_timeout( + command, work, _safe_int(state.get("test_timeout"), DEFAULT_TEST_TIMEOUT) + ) + if timed_out or code != 0: + # **途中まで書き換えた差分を残さない。** 残すと次の実行は + # `_require_clean_worktree` で必ず止まり、`pending_push` の再試行が + # 永久に進まなくなる。着手前が綺麗だったことは確認済みなので、 + # ここにある変更は全て同期が作ったものだと分かる。 + _discard_worktree_changes(work) + die( + f"生成物の同期に失敗しました（{command}）: " + + ("打ち切りました" if timed_out else f"終了コード {code}") + + "。同期が作った差分は破棄したので、原因を直せばそのまま再開できます" + ) + produced = _dirty_paths(state, work) + if not produced: + return + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + + def _push_head(state: dict[str, Any]) -> None: - """head ブランチへ push する。**`--force` は使わない。**""" + """head ブランチへ push する。**`--force` は使わない。** + + **公開するのは進行側だけである。** 実装担当に push させると、検証を通る前に + 変更が Pull Request へ現れ、取り消しの反映漏れがそのまま残る。 + """ + _sync_generated(state) _sh( ["git", "push", "origin", f"HEAD:{state['head_branch']}"], cwd=state["worktrees"]["work"], @@ -2607,6 +2789,10 @@ def main() -> None: init.add_argument("--test-timeout", type=int, default=DEFAULT_TEST_TIMEOUT, help="テスト 1 回あたりの上限秒数。超えたら失敗として扱う " f"(default: {DEFAULT_TEST_TIMEOUT})") + init.add_argument("--sync-command", default=None, + help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" + "差分があれば進行側のコミットとして積む。" + "同期を実装担当にさせると範囲外の変更になるため分離している") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_abandon_items.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_abandon_items.py index 63b2bb64..41928945 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/tests/test_abandon_items.py +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_abandon_items.py @@ -244,6 +244,8 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: resolved_facts, ) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": claimed, "elapsed_seconds": 12, @@ -396,6 +398,8 @@ def test_broken_fix_result_does_not_crash( """ state_path = _state(tmp_path, [_finding("R1-001")]) env_tmp_dir(state_path) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": broken_ids, "commits": {"sha": "辞書ではあるが配列でない"}, 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 4987b7d6..2427ed3a 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_init.py @@ -59,6 +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, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } 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 6b772a0c..28aa9cb7 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 @@ -383,7 +383,7 @@ def test_self_reported_values_cannot_pass_the_check( assert "テストが成功していません" in state["items"][0]["failure_reason"] -def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 @@ -392,6 +392,7 @@ def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): calls: list[list[str]] = [] picked: list[str] = [] reverted: list[str] = [] + statuses: list[int] = [] def fake_run(cmd, **kwargs): calls.append(list(cmd)) @@ -413,6 +414,14 @@ def fake_git_out(work, args): if picked: return f"new-{picked[-1]}" return "REVERTED_HEAD" if reverted else "HEAD_BEFORE" + if "status" in args: + # 同期の前後で 2 回呼ばれる。1 回目が同期前、2 回目以降が同期後。 + # 既定は「同期前も後も差分なし」 + statuses.append(len(statuses)) + if sync_dirty is False: + return "" + before, after = sync_dirty + return before if len(statuses) == 1 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -647,10 +656,13 @@ def test_blank_scope_entry_is_ignored(refactor): assert not refactor.path_in_scope("dist/foo.py", ["", " ", "src"]) -def test_no_push_when_nothing_was_reverted( +def test_push_happens_once_per_merge_apply( refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts ): - """全項目が通ったときに余計な push をしない。""" + """全項目が通ったときも公開するが、push は 1 回だけにすること。 + + 公開するのは進行側だけである（実装担当は push しない）。 + """ items = [item(item_id="R1-001")] state_path = _state_with_items(tmp_path, items) env_tmp_dir(state_path) @@ -666,7 +678,7 @@ def test_no_push_when_nothing_was_reverted( lambda cmd, **kw: subprocess.CompletedProcess(cmd, 0, "", ""), ) refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) - assert pushes == [] + assert len([c for c in pushes if c[:2] == ["git", "push"]]) == 1 def test_dry_run_touches_neither_git_nor_state( @@ -1205,3 +1217,330 @@ def test_push_failure_after_a_successful_drop_only_retries_the_push( entry = read_state(state_path)["rounds"][0] assert entry["pending_push"] is False assert entry["apply"]["applied"] == ["R1-001"] + + +# ---------- 適用で失敗した項目を「対象外」へ ---------- + +def test_failed_items_are_deferred( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """適用の検証で失敗した項目を「対象外」として記録すること。 + + 記録しないと**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では 3 ランタイム全員から再提案され、合意数が最大になって最優先で採用された。 + """ + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-002"], "失敗した項目だけを対象外にすること" + entry = deferred["R1-002"] + # 次ラウンドの除外は path + symbol + smell の組で行われる + assert (entry["path"], entry["symbol"], entry["smell"]) == ( + "src/foo.py", "Foo.handle", "long_method") + assert "差分予算" in entry["defer_reason"] + + +def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts): + """叩き直しても対象外の記録を重複させないこと。""" + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + args = type("A", (), {"id": 130, "round": 1, "dry_run": False})() + refactor.cmd_merge_apply(args) + refactor.cmd_merge_apply(args) + assert [d["item_id"] for d in read_state(state_path)["deferred_items"]] == ["R1-002"] + + +# ---------- push の直前に生成物を同期する ---------- + +def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["sync_command"] = command + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + return state_path + + +def test_push_syncs_generated_files_first( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """push の直前に同期し、差分があれば進行側のコミットとして積むこと。 + + 同期を実装担当にさせると範囲外の変更になり、範囲の検査で全件失敗する。 + かといって同期しないと、同期を検査する pre-push では push が通らない。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + order: list[str] = [] + _drop_env(refactor, monkeypatch, + sync_dirty=("", "?? plugins/generated/a.py")) + monkeypatch.setattr( + refactor, "_sh", + lambda cmd, **k: order.append("push" if cmd[:2] == ["git", "push"] else cmd[1]) + or "", + ) + ran: list[tuple[str, str]] = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append((command, cwd)) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert ran and ran[0][0] == "make build", "同期コマンドを実行していない" + assert "add" in order and "commit" in order, "同期の差分をコミットしていない" + assert order.index("commit") < order.index("push"), "コミットより先に push している" + + +def test_sync_failure_aborts_without_pushing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期に失敗したら中断する。黙って push しない。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_no_sync_command_runs_nothing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """`--sync-command` 未指定なら同期は走らない（既存の利用者に影響しない）。""" + _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert ran == [] + + +def test_merge_apply_pushes_even_when_every_item_passes( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """全項目が通ったときも進行側が push すること。 + + 実装担当が push しなくなったため、ここで公開しないとレビュー担当が + Pull Request 上の差分へ指摘を書けない。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + git_facts({"ok111": fact(sha="ok111")}) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + calls, pushes = _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in pushes if c[:2] == ["git", "push"]], "push していない" + for cmd in pushes: + assert "--force" not in cmd and "--no-verify" not in cmd + entry = read_state(state_path)["rounds"][0] + assert entry["pending_push"] is False + assert entry["apply"]["applied"] == ["R1-001"] + + +def test_sync_aborts_when_the_worktree_is_dirty( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期の前に作業ツリーが汚れていたら中断すること。 + + 汚れたまま同期すると、同期が作った差分と元からあった差分を区別できない。 + 状態コードを比べても、元から ` M` のファイルを同期がさらに書き換えた場合を + 取りこぼす。`git commit` は index を丸ごと含めるため、staged 済みの変更も + 検証を受けないまま公開される。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(" M src/edited.py", " M src/edited.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert ran == [], "汚れたまま同期を走らせている" + assert [c for c in staged if c[:2] == ["git", "push"]] == [] + + +def test_dirt_inside_the_control_directory_does_not_abort( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """制御用ディレクトリの中は汚れていても止めないこと。 + + 状態ファイル・結果・ログは常にそこへ書かれる。 + """ + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(f"?? {control}/codex-apply-r1-result.json", + f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert [c for c in staged if c[:2] == ["git", "add"]] == [ + ["git", "add", "--", "generated/a.py"]] + + +def test_sync_excludes_the_control_directory( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """状態ファイル・結果・ログの置き場所を同期コミットへ入れないこと。""" + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + # 既定の配置では制御用ディレクトリが作業ディレクトリの中にある + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name # `.cross_refactoring` + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=("", f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + adds = [c for c in staged if c[:2] == ["git", "add"]] + assert adds == [["git", "add", "--", "generated/a.py"]] + + +def test_sync_without_changes_makes_no_commit( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期しても差分が出なければ、空のコミットを積まないこと。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env(refactor, monkeypatch, sync_dirty=("", "")) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in staged if c[:2] == ["git", "commit"]] == [] + assert [c for c in staged if c[:2] == ["git", "push"]], "push はすること" + + +def test_whole_round_failure_also_defers_items( + refactor, tmp_path, env_tmp_dir, no_git, git_facts +): + """ラウンドごと取り消す経路でも「対象外」として記録すること。 + + 項目別の失敗だけを記録すると、未割当コミットで落ちた提案が次のラウンドで + 再び採用される。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + # 範囲に 2 件あるが、申告は 1 件だけ（未割当コミットあり） + git_facts({"ok111": fact(sha="ok111")}, in_range=["sneaky", "ok111"]) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == 2 + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-001"] + assert "割り当てられていない" in deferred["R1-001"]["defer_reason"] + + +def test_sync_failure_discards_what_it_produced( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が途中で失敗したら、作った差分を捨てて再開できる状態にすること。 + + 残すと次の実行は清浄性の検査で必ず止まり、`pending_push` の再試行が + 永久に進まなくなる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", " M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + # index も戻す。`git checkout -- .` では staged された差分が残り、 + # 清浄性の検査が通らないままになる + assert ["git", "reset", "--hard", "HEAD"] in calls, "index を戻していない" + assert ["git", "clean", "-fd"] in calls, "同期が作ったファイルを消していない" + # 無視されたファイル（制御用ディレクトリ）まで消さない + assert not any("-x" in c for c in calls if c[:2] == ["git", "clean"]) + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_status_disables_path_quoting(refactor, monkeypatch): + """`core.quotePath` の既定では非 ASCII のパスがエスケープされて `git add` が失敗する。""" + seen: list[list[str]] = [] + monkeypatch.setattr( + refactor, "_git_out", + lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + ) + assert refactor._worktree_changes("/w") == {"plugins/日本語/a.py": " M"} + assert seen[0][:2] == ["-c", "core.quotePath=false"] + + +def test_sync_failure_also_resets_the_index( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が `git add` してから失敗しても、次の実行が再開できること。 + + `git checkout -- .` は staged された差分を戻さないため、index も戻さないと + 清浄性の検査が通らないままになる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + # 同期が index へ追加してから失敗した状況（`M ` は staged） + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", "M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit): + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert ["git", "reset", "--hard", "HEAD"] in calls + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] diff --git a/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md b/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md index aab37aa8..eaac7f69 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md @@ -38,7 +38,8 @@ allowed-tools: | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | -| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる。生成物の同期は進行側が収束後にまとめて行う | +| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | +| 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | | 検証の情報源 | **git と実際のテスト実行。** 結果ファイルの申告は検証に使わない（書き換えるだけで通る検査にしない） | | 投稿 | **AI 自身が `gh api` で投稿する。** ホストの作業文脈に差分やレビュー本文を載せない | | 状態の永続化 | `/.cross_refactoring/cross-refactoring-rf<番号>-state.json` に集約。中断・再開可能 | @@ -58,9 +59,11 @@ allowed-tools: | `--max-items-per-round N` | 1 ラウンドの採用上限 | `5` | | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | +| `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" +/ndf:cross-refactoring 130 --scope src --baseline-test "pytest -q" --sync-command "make generate" /ndf:cross-refactoring 130 --scope src --model codex=gpt-5.5 --model claude=opus-5 /ndf:cross-refactoring 130 --scope src --host codex --max-outer-rounds 1 ``` @@ -227,9 +230,8 @@ while :; do # 提案ラウンドの繰り返 rf advance "$ID" || break done -# 収束後にまとめて生成物を同期する（**進行側の責務**）。編集元から配布物を生成する -# 規約を持つリポジトリでは、実装担当に同期させると範囲外の変更が生まれる。 -# 同期が要るなら、ここで生成してから Step 7 の最終ゲートへ渡す。 +# 生成物の同期は `--sync-command` として push の直前に進行側が実行済み。 +# ここで追加の作業は要らない。 ``` ### 終了コード @@ -260,7 +262,9 @@ done | ホストのサブエージェントで適用する | ホストの作業文脈に差分が載り、実装者とレビュー担当の独立性が崩れる | | `launch-cli.sh` に「ホストなら起動しない」分岐を入れる | ホストは適用担当として起動しうる。分岐はランタイム名だけで行う | | `--scope` を省く | 提案が発散し、Pull Request が肥大する | -| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、差分予算を超える。同期は進行側が収束後にまとめて行う | +| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、範囲の検査で全件失敗する。同期は `--sync-command` で進行側が行う | +| 実装担当に push させる | 検証を通る前に公開され、取り消しの反映漏れが Pull Request に残る | +| 生成物を同期するリポジトリで `--sync-command` を省く | pre-push の検査で**あらゆる push が落ちる**。実装担当が同期に手を出し、範囲違反で全件失敗する | | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | 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 487a9b4c..45207583 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,7 +46,10 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 + 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に + すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） +8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない 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 e9b460b8..4609b02a 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 @@ -54,8 +54,44 @@ | 誰が | 何を | | --- | --- | -| 実装担当 | `--scope` の中だけを変更する。生成物・配布物の同期はしない | -| 進行側（ホスト） | 収束後にまとめて生成物を同期する | +| 実装担当 | `--scope` の中だけを変更する。生成物の同期もしないし、**push もしない** | +| 進行側 | 検証を通した後に公開する。`--sync-command` を **push の直前**に実行する | + +#### 公開するのは進行側だけである + +**実装担当に push させない。** 検証を通る前に変更が Pull Request へ現れると、 +取り消しの反映が漏れたときにそのまま残る。`merge-apply` と `merge-fix` は、 +検証が済んでから進行側として push する。 + +生成物の同期も同じ経路に乗せる。`--sync-command` を指定すると、**push の直前**に +進行側が実行し、差分があれば**どの改善項目にも属さないコミット**として積む。 + +| 案 | 結果（実測） | +| --- | --- | +| 同期しない | 同期を検査する pre-push で**あらゆる push が落ちる**。取り消しを反映できない | +| 実装担当に同期させる | 範囲外の変更になり、**採用 5 件が全件失敗**した | +| **進行側が push の直前に同期する** | 採用 | + +同期コミットは取り消しでは積み直されないが、次の push で作り直されるため失われても +問題にならない。同期に失敗したら**中断する**（終了コード 4）。同期できない状態を +公開すると、利用者のリポジトリの検査を壊したまま進むことになる。 + +**同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、同期が +作った差分と元からあった差分を区別できない。区別しようと `git status` の状態コードを +比べても足りず、次の 2 つを取りこぼす。 + +| 取りこぼし | 何が起きるか | +| --- | --- | +| 元から ` M` のファイルを同期がさらに書き換える | 状態コードが変わらず検知できない。その変更がコミットされず **push がまた落ちる** | +| 同期の前から index に staged された変更がある | `git commit` は index を丸ごと含めるため、`git add` の対象を絞っても**検証を受けない変更が公開される** | + +無視されたファイルは判定に現れない。生成物やキャッシュを `.gitignore` へ入れてあれば +止まらない。制御用ディレクトリ（状態ファイル・結果・ログ）も判定から外す。 + +**同期が途中で失敗したら、作った差分を捨ててから中断する。** 残すと次の実行は +清浄性の検査で必ず止まり、`pending_push` の再試行が永久に進まない。着手前が +綺麗だったことは確認済みなので、そこにある変更は全て同期が作ったものだと分かる。 +無視されたファイルは消さない（`git clean` に `-x` を付けない）。 判定は**前方一致だけ**で行い、除外規則は持たない。規則を書けるようにすると、 規則を 1 行足すだけで範囲の検査を骨抜きにできる。 @@ -147,9 +183,14 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 #### 取り消しは判定が出そろってからまとめて行う -**失敗した項目のコミットを Pull Request に残さない。** 実装担当は項目ごとに push して -いるため、状態を `abandoned` にするだけでは差分が残り、以後のレビュー対象にも混入する。 -何が消えるかを先に見たいときは `--dry-run` を付ける。 +**失敗した項目のコミットを Pull Request に残さない。** 公開は進行側が検証の後に行うので、 +検証で落ちた項目はそもそも公開されない。ただし取り消しはローカルの履歴にも必要である +（残すとレビュー対象と以後のラウンドに混入する）。何が消えるかを先に見たいときは +`--dry-run` を付ける。 + +取り消した項目は**「対象外」として記録する**（`deferred_items`）。記録しないと同じ提案が +次のラウンドで再び採用され、同じ理由で失敗する。実測では 3 ランタイム全員から再提案され、 +合意数が最大になって最優先で採用された。 ただし**項目ごとにその場で戻してはならない**。詳細は [取り消しは巻き戻して積み直す](#取り消しは巻き戻して積み直す)を参照する。 @@ -402,9 +443,9 @@ flowchart LR 重複率は `path` + `symbol` + `smell` の集合比較で求める。同じ提案が毎ラウンド出続けて 終わらない状態を検知するためである。 -終了後、生成物の同期が要るリポジトリでは**ここで進行側がまとめて同期する**。 -実装担当に同期させると範囲外の変更が生まれ、差分予算にも影響する（Step 4 の -「範囲の指定は検証にも効かせる」を参照）。 +**ここで追加の同期は要らない。** 生成物の同期は `--sync-command` として +各 push の直前に済んでいる（Step 4 の「公開するのは進行側だけである」を参照）。 +手で同期すると、検証を受けていない差分を作ることになる。 続けて **`/ndf:cross-review `** で Pull Request 全体を承認収束にかける。 レビューはラウンド単位なので、**ラウンドを跨いだ整合はここで見る**。 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md index ad7fac79..bfaf491e 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md @@ -30,7 +30,6 @@ $RF_ITEMS 2. `plan` の手順を **1 手ずつ**適用する 3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** 4. 通ったらコミットする（**1 手 = 1 コミット**） -5. 項目が終わったら push する ## コミットの規約 @@ -54,13 +53,15 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 + ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます - **生成物・配布物の同期をしない。** このリポジトリに「編集元から配布物を生成する」 - 規約があっても、同期は**進行側が収束後にまとめて行う**責務です。ここで同期すると - 範囲外の変更が生まれ、差分予算も超えます + 規約があっても、同期は**進行側が公開の直前に行う**責務です。ここで同期すると + 範囲外の変更が生まれ、その項目は検証で失敗します - **機能変更を混ぜない。** 振る舞いを変える修正が必要だと分かったら、その項目は 適用せず `status` を `skipped` にして理由を書く - 提案された手順の範囲を超えない。ついでの整理をしない @@ -103,7 +104,7 @@ Impl-Model: $RF_MODEL 取り直します。ここに何と書いても検査結果は変わりません。 - `commits[].sha` は**正確に書いてください**。ここが唯一の対応付けであり、 - 検証に失敗した項目はここに書かれたコミットを取り消して push します。 + 検証に失敗した項目は、ここに書かれたコミットが取り消されます。 書き漏らすと差分が Pull Request に残ります - **このラウンドで作ったコミットは、全て `items[].commits` のどれかに入れてください。** どの項目にも割り当てられていないコミットが 1 件でもあると、**ラウンドごと diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md index b28b967d..a73f3c3e 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md @@ -26,8 +26,7 @@ $RF_ITEMS 2. 各指摘について、**修正するか・しないか**を決める - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする - 修正しない: 根拠を返信する。**黙って閉じない** -3. すべての対応が終わったら push する -4. 対応したスレッドに返信し、`resolveReviewThread` で解決する +3. 対応したスレッドに返信し、`resolveReviewThread` で解決する 指摘のうち、**振る舞いを変えないと直せないもの**は修正しないでください。 その場合は「この改善項目自体を見送るべき」と返信し、解決しないまま残します。 @@ -55,11 +54,12 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った 修正コミットがあると、その修正ラウンドの範囲ごと取り消されます -- **生成物・配布物の同期をしない。** 同期は進行側が収束後にまとめて行います +- **生成物・配布物の同期をしない。** 同期は進行側が公開の直前に行います - 指摘に無い箇所を「ついでに」直さない。ラウンドの差分が膨らみ、 どの変更がどの指摘に対応するのか追えなくなる diff --git a/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py index 3ad3134b..1a224c66 100755 --- a/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py @@ -137,6 +137,14 @@ def vocabulary() -> dict[str, Any]: # 適用で必ず配置する Skill。ここに無いものは配らない。 REQUIRED_SKILLS = ("refactoring", "tdd-cycle", "quality-gates") +# 生成物を同期したコミットのメッセージ。**どの改善項目にも属さない**ことが分かる形にする。 +SYNC_COMMIT_MESSAGE = ( + "Chore: 生成物を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "同期を検査する pre-push を持つリポジトリでも push できるよう、\n" + "公開の直前に進行側がまとめて生成する。" +) + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -468,7 +476,7 @@ def verify_scope(commit: dict[str, Any], scope: Iterable[str]) -> Optional[str]: more = f" ほか {len(outside) - 5} 件" if len(outside) > 5 else "" return ( f"コミット {commit.get('sha', '?')} が対象範囲の外を変更しています" - f"（{shown}{more}）。生成物の同期は進行側が収束後にまとめて行います。" + f"（{shown}{more}）。生成物の同期は進行側が公開の直前に行います。" "現状固定テストの置き場所が範囲外なら、`--scope` に含めてから実行してください" ) @@ -779,6 +787,8 @@ def cmd_init(args: argparse.Namespace) -> None: "max_items_per_round": args.max_items_per_round, "severity_threshold": args.severity_threshold, "baseline_test": baseline, + # 生成物の同期は**進行側の責務**。push の直前に実行する。 + "sync_command": args.sync_command, "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -1244,6 +1254,9 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: if args.dry_run: info("（dry-run）状態ファイルは更新していません") else: + # 項目別の失敗と同じく、**ここで取り消した項目も「対象外」に残す**。 + # 残さないと同じ提案が次のラウンドで再び採用される。 + _defer_abandoned_items(state, entry) statefile.save(path, state) _push_head(state) entry["pending_push"] = False @@ -1323,7 +1336,13 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: # `merged_at` は `_apply_drop` が取り消しの完了時点で立てる。 applied = _apply_drop(path, state, entry, failed) else: + # **全項目が通ったときも進行側が公開する。** 実装担当は push しないため、 + # ここで公開しないとレビュー担当が Pull Request 上の差分へ指摘を書けない。 entry["apply"]["merged_at"] = statefile.now() + entry["pending_push"] = True + statefile.save(path, state) + _push_head(state) + entry["pending_push"] = False statefile.save(path, state) if not applied: @@ -1331,6 +1350,29 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: sys.exit(2) +def _defer_abandoned_items(state: dict[str, Any], entry: dict[str, Any]) -> None: + """このラウンドで取り消した項目を「対象外」として記録する。 + + 記録しないと、**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では適用で失敗した項目が 3 ランタイム全員から再提案され、合意数が最大に + なって最優先で採用された。手順書が「同じ提案が毎ラウンド出続けて収束しない」 + として禁じている状態そのものである。 + + 除外の鍵は `path` + `symbol` + `smell` なので、その 3 つを必ず残す。 + """ + already = {d.get("item_id") for d in state["deferred_items"]} + for item_id in entry["items"]: + item = _find_item(state, item_id, required=False) + if item is None or item.get("status") != "abandoned" or item_id in already: + continue + state["deferred_items"].append({ + "item_id": item_id, + "path": item["path"], "symbol": item["symbol"], "smell": item["smell"], + "round": entry["round"], + "defer_reason": item.get("failure_reason") or "適用結果の検証を通らなかった", + }) + + def _run_drop( path: pathlib.Path, state: dict[str, Any], entry: dict[str, Any], targets: list[str], @@ -1391,6 +1433,9 @@ def _apply_drop( entry["apply_base_sha"] = _git_out(work, ["rev-parse", "HEAD"]) state["phase"] = "propose" + # 取り消した項目は「対象外」として残す。次のラウンドで同じ提案が採用され、 + # 同じ理由で失敗するのを防ぐ。 + _defer_abandoned_items(state, entry) # **取り消しが済んだことを push より先に、印の解除と同じ保存で永続化する。** # 保存せずに push して失敗すると、次の実行が適用の検証をやり直し、取り消しと # 積み直しのコミットを「未割当」と判定してラウンドごと巻き込んでしまう。 @@ -1725,7 +1770,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: # 適用フェーズの未割当コミットと同じ扱いにする。 problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) - needs_push = False for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, state.get("target_scope") or []) @@ -1762,7 +1806,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: statefile.save(path, state) # **push は保存のあと。** ここで push して失敗すると、取り消しコミットは # ローカルに残るのに起点の更新が保存されず、叩き直しで二重に取り消してしまう。 - needs_push = True info("⚠ 修正を取り消したため、解決の申告は採用しません") resolved = set() else: @@ -1785,8 +1828,9 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: + _safe_int(payload.get("elapsed_seconds")) ) statefile.save(path, state) - if needs_push: - _push_with_retry_marker(path, state, entry) + # **取り消したかどうかに関わらず公開する。** 実装担当は push しないため、 + # ここで公開しないと再レビューが Pull Request 上の差分を見られない。 + _push_with_retry_marker(path, state, entry) info(f"修正を取り込みました（解決 {len(resolved)} スレッド / 修正ラウンド {entry['fix_rounds']}）") @@ -2471,8 +2515,146 @@ def _order_newest_first(work: str, shas: list[str]) -> list[str]: return sorted(shas, key=lambda s: rank.get(resolved[s], len(rank))) +def _worktree_changes(work: str) -> dict[str, str]: + """作業ツリーの変更を `パス → 状態` で返す。同期の前後を比べるために使う。 + + 無視されているファイルは現れない（`--porcelain` の既定）。改名は移動先の + パスだけを見る。 + """ + # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ + # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 + out = _git_out( + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + ) + changes: dict[str, str] = {} + for line in (out or "").splitlines(): + if len(line) < 4: + continue + path = line[3:] + if " -> " in path: # 改名。移動先だけを対象にする + path = path.split(" -> ", 1)[1] + changes[path.strip('"')] = line[:2] + return changes + + +def _control_prefix(state: dict[str, Any], work: str) -> Optional[str]: + """作業ディレクトリから見た制御用ディレクトリの相対パス。外にあれば `None`。 + + 状態ファイル・プロンプト・結果・ログの置き場所で、**同期コミットへ入れない**。 + `prepare-worktrees.sh` が無視の設定を置くが、置き場所を環境変数で移した場合や + 配置前に同期が走った場合に備えて、ここでも明示的に外す。 + """ + tmp_dir = str(state.get("tmp_dir") or "") + if not tmp_dir: + return None + try: + relative = pathlib.Path(tmp_dir).resolve().relative_to( + pathlib.Path(work).resolve() + ) + except ValueError: + return None + return f"{relative}/" + + +def _dirty_paths(state: dict[str, Any], work: str) -> list[str]: + """作業ツリーの未コミット変更のパス。制御用ディレクトリは除く。""" + control = _control_prefix(state, work) + return sorted( + path for path in _worktree_changes(work) + if not (control and path.startswith(control)) + ) + + +def _discard_worktree_changes(work: str) -> None: + """作業ツリーと index の未コミット変更を捨てる。**着手前が綺麗なときだけ呼ぶ。** + + **index も戻す。** `git checkout -- .` は staged された差分を戻さないため、 + 同期コマンドが `git add` してから失敗すると清浄性の検査が通らないままになり、 + `pending_push` の再試行が永久に進まない。 + + 無視されたファイル（制御用ディレクトリを含む）は消さない（`git clean` に + `-x` を付けない）。 + """ + for args in (["reset", "--hard", "HEAD"], ["clean", "-fd"]): + subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) + + +def _require_clean_worktree(state: dict[str, Any], work: str) -> None: + """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 + + 汚れたまま同期すると、**同期が作った差分と元からあった差分を区別できない**。 + 区別しようと状態コードを比べても足りず、次の 2 つを取りこぼす。 + + - 元から ` M` のファイルを同期がさらに書き換えても、状態コードは ` M` のままで + 検知できない。その変更がコミットされず、**push がまた落ちる** + - `git commit` は index の内容を全て含めるため、`git add` の対象を絞っても + **先に staged だった変更が検証を受けないまま Pull Request へ入る** + + 無視されたファイルはここに現れない。生成物やキャッシュを `.gitignore` へ + 入れてあれば止まらない。 + """ + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = ", ".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + die( + f"生成物を同期する前に、作業ツリーへ未コミットの変更があります（{shown}{more}）。" + "同期が作った差分と区別できず、検証を受けていない変更を公開しかねないため" + "中断します。コミットするか `.gitignore` へ入れてから再実行してください" + ) + + +def _sync_generated(state: dict[str, Any]) -> None: + """push の直前に生成物を同期し、差分があれば進行側のコミットとして積む。 + + 同期を**実装担当の責務にすると範囲外の変更が生まれ**、範囲の検査で全件失敗する + （実測ではラウンドの採用 5 件が全て範囲外で落ちた）。かといって同期しないと、 + 生成物の同期を検査する pre-push を持つリポジトリでは push そのものが通らず、 + 取り消しを Pull Request へ反映できない。そこで**進行側が push の直前に同期する**。 + + このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 + 次の push で作り直されるので失われても問題にならない。 + + 同期に失敗したら中断する。**黙って push しない。** 同期できない状態を公開すると、 + 利用者のリポジトリの検査を壊したまま進むことになる。 + """ + command = str(state.get("sync_command") or "").strip() + if not command: + return + work = state["worktrees"]["work"] + # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 + # 同期が作った差分と元からあった差分を区別できない。 + _require_clean_worktree(state, work) + code, timed_out = _run_with_timeout( + command, work, _safe_int(state.get("test_timeout"), DEFAULT_TEST_TIMEOUT) + ) + if timed_out or code != 0: + # **途中まで書き換えた差分を残さない。** 残すと次の実行は + # `_require_clean_worktree` で必ず止まり、`pending_push` の再試行が + # 永久に進まなくなる。着手前が綺麗だったことは確認済みなので、 + # ここにある変更は全て同期が作ったものだと分かる。 + _discard_worktree_changes(work) + die( + f"生成物の同期に失敗しました（{command}）: " + + ("打ち切りました" if timed_out else f"終了コード {code}") + + "。同期が作った差分は破棄したので、原因を直せばそのまま再開できます" + ) + produced = _dirty_paths(state, work) + if not produced: + return + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + + def _push_head(state: dict[str, Any]) -> None: - """head ブランチへ push する。**`--force` は使わない。**""" + """head ブランチへ push する。**`--force` は使わない。** + + **公開するのは進行側だけである。** 実装担当に push させると、検証を通る前に + 変更が Pull Request へ現れ、取り消しの反映漏れがそのまま残る。 + """ + _sync_generated(state) _sh( ["git", "push", "origin", f"HEAD:{state['head_branch']}"], cwd=state["worktrees"]["work"], @@ -2607,6 +2789,10 @@ def main() -> None: init.add_argument("--test-timeout", type=int, default=DEFAULT_TEST_TIMEOUT, help="テスト 1 回あたりの上限秒数。超えたら失敗として扱う " f"(default: {DEFAULT_TEST_TIMEOUT})") + init.add_argument("--sync-command", default=None, + help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" + "差分があれば進行側のコミットとして積む。" + "同期を実装担当にさせると範囲外の変更になるため分離している") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_abandon_items.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_abandon_items.py index 63b2bb64..41928945 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_abandon_items.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_abandon_items.py @@ -244,6 +244,8 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: resolved_facts, ) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": claimed, "elapsed_seconds": 12, @@ -396,6 +398,8 @@ def test_broken_fix_result_does_not_crash( """ state_path = _state(tmp_path, [_finding("R1-001")]) env_tmp_dir(state_path) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": broken_ids, "commits": {"sha": "辞書ではあるが配列でない"}, 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 4987b7d6..2427ed3a 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_init.py @@ -59,6 +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, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } 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 6b772a0c..28aa9cb7 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 @@ -383,7 +383,7 @@ def test_self_reported_values_cannot_pass_the_check( assert "テストが成功していません" in state["items"][0]["failure_reason"] -def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 @@ -392,6 +392,7 @@ def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): calls: list[list[str]] = [] picked: list[str] = [] reverted: list[str] = [] + statuses: list[int] = [] def fake_run(cmd, **kwargs): calls.append(list(cmd)) @@ -413,6 +414,14 @@ def fake_git_out(work, args): if picked: return f"new-{picked[-1]}" return "REVERTED_HEAD" if reverted else "HEAD_BEFORE" + if "status" in args: + # 同期の前後で 2 回呼ばれる。1 回目が同期前、2 回目以降が同期後。 + # 既定は「同期前も後も差分なし」 + statuses.append(len(statuses)) + if sync_dirty is False: + return "" + before, after = sync_dirty + return before if len(statuses) == 1 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -647,10 +656,13 @@ def test_blank_scope_entry_is_ignored(refactor): assert not refactor.path_in_scope("dist/foo.py", ["", " ", "src"]) -def test_no_push_when_nothing_was_reverted( +def test_push_happens_once_per_merge_apply( refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts ): - """全項目が通ったときに余計な push をしない。""" + """全項目が通ったときも公開するが、push は 1 回だけにすること。 + + 公開するのは進行側だけである（実装担当は push しない）。 + """ items = [item(item_id="R1-001")] state_path = _state_with_items(tmp_path, items) env_tmp_dir(state_path) @@ -666,7 +678,7 @@ def test_no_push_when_nothing_was_reverted( lambda cmd, **kw: subprocess.CompletedProcess(cmd, 0, "", ""), ) refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) - assert pushes == [] + assert len([c for c in pushes if c[:2] == ["git", "push"]]) == 1 def test_dry_run_touches_neither_git_nor_state( @@ -1205,3 +1217,330 @@ def test_push_failure_after_a_successful_drop_only_retries_the_push( entry = read_state(state_path)["rounds"][0] assert entry["pending_push"] is False assert entry["apply"]["applied"] == ["R1-001"] + + +# ---------- 適用で失敗した項目を「対象外」へ ---------- + +def test_failed_items_are_deferred( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """適用の検証で失敗した項目を「対象外」として記録すること。 + + 記録しないと**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では 3 ランタイム全員から再提案され、合意数が最大になって最優先で採用された。 + """ + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-002"], "失敗した項目だけを対象外にすること" + entry = deferred["R1-002"] + # 次ラウンドの除外は path + symbol + smell の組で行われる + assert (entry["path"], entry["symbol"], entry["smell"]) == ( + "src/foo.py", "Foo.handle", "long_method") + assert "差分予算" in entry["defer_reason"] + + +def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts): + """叩き直しても対象外の記録を重複させないこと。""" + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + args = type("A", (), {"id": 130, "round": 1, "dry_run": False})() + refactor.cmd_merge_apply(args) + refactor.cmd_merge_apply(args) + assert [d["item_id"] for d in read_state(state_path)["deferred_items"]] == ["R1-002"] + + +# ---------- push の直前に生成物を同期する ---------- + +def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["sync_command"] = command + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + return state_path + + +def test_push_syncs_generated_files_first( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """push の直前に同期し、差分があれば進行側のコミットとして積むこと。 + + 同期を実装担当にさせると範囲外の変更になり、範囲の検査で全件失敗する。 + かといって同期しないと、同期を検査する pre-push では push が通らない。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + order: list[str] = [] + _drop_env(refactor, monkeypatch, + sync_dirty=("", "?? plugins/generated/a.py")) + monkeypatch.setattr( + refactor, "_sh", + lambda cmd, **k: order.append("push" if cmd[:2] == ["git", "push"] else cmd[1]) + or "", + ) + ran: list[tuple[str, str]] = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append((command, cwd)) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert ran and ran[0][0] == "make build", "同期コマンドを実行していない" + assert "add" in order and "commit" in order, "同期の差分をコミットしていない" + assert order.index("commit") < order.index("push"), "コミットより先に push している" + + +def test_sync_failure_aborts_without_pushing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期に失敗したら中断する。黙って push しない。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_no_sync_command_runs_nothing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """`--sync-command` 未指定なら同期は走らない（既存の利用者に影響しない）。""" + _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert ran == [] + + +def test_merge_apply_pushes_even_when_every_item_passes( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """全項目が通ったときも進行側が push すること。 + + 実装担当が push しなくなったため、ここで公開しないとレビュー担当が + Pull Request 上の差分へ指摘を書けない。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + git_facts({"ok111": fact(sha="ok111")}) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + calls, pushes = _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in pushes if c[:2] == ["git", "push"]], "push していない" + for cmd in pushes: + assert "--force" not in cmd and "--no-verify" not in cmd + entry = read_state(state_path)["rounds"][0] + assert entry["pending_push"] is False + assert entry["apply"]["applied"] == ["R1-001"] + + +def test_sync_aborts_when_the_worktree_is_dirty( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期の前に作業ツリーが汚れていたら中断すること。 + + 汚れたまま同期すると、同期が作った差分と元からあった差分を区別できない。 + 状態コードを比べても、元から ` M` のファイルを同期がさらに書き換えた場合を + 取りこぼす。`git commit` は index を丸ごと含めるため、staged 済みの変更も + 検証を受けないまま公開される。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(" M src/edited.py", " M src/edited.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert ran == [], "汚れたまま同期を走らせている" + assert [c for c in staged if c[:2] == ["git", "push"]] == [] + + +def test_dirt_inside_the_control_directory_does_not_abort( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """制御用ディレクトリの中は汚れていても止めないこと。 + + 状態ファイル・結果・ログは常にそこへ書かれる。 + """ + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(f"?? {control}/codex-apply-r1-result.json", + f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert [c for c in staged if c[:2] == ["git", "add"]] == [ + ["git", "add", "--", "generated/a.py"]] + + +def test_sync_excludes_the_control_directory( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """状態ファイル・結果・ログの置き場所を同期コミットへ入れないこと。""" + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + # 既定の配置では制御用ディレクトリが作業ディレクトリの中にある + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name # `.cross_refactoring` + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=("", f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + adds = [c for c in staged if c[:2] == ["git", "add"]] + assert adds == [["git", "add", "--", "generated/a.py"]] + + +def test_sync_without_changes_makes_no_commit( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期しても差分が出なければ、空のコミットを積まないこと。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env(refactor, monkeypatch, sync_dirty=("", "")) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in staged if c[:2] == ["git", "commit"]] == [] + assert [c for c in staged if c[:2] == ["git", "push"]], "push はすること" + + +def test_whole_round_failure_also_defers_items( + refactor, tmp_path, env_tmp_dir, no_git, git_facts +): + """ラウンドごと取り消す経路でも「対象外」として記録すること。 + + 項目別の失敗だけを記録すると、未割当コミットで落ちた提案が次のラウンドで + 再び採用される。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + # 範囲に 2 件あるが、申告は 1 件だけ（未割当コミットあり） + git_facts({"ok111": fact(sha="ok111")}, in_range=["sneaky", "ok111"]) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == 2 + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-001"] + assert "割り当てられていない" in deferred["R1-001"]["defer_reason"] + + +def test_sync_failure_discards_what_it_produced( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が途中で失敗したら、作った差分を捨てて再開できる状態にすること。 + + 残すと次の実行は清浄性の検査で必ず止まり、`pending_push` の再試行が + 永久に進まなくなる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", " M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + # index も戻す。`git checkout -- .` では staged された差分が残り、 + # 清浄性の検査が通らないままになる + assert ["git", "reset", "--hard", "HEAD"] in calls, "index を戻していない" + assert ["git", "clean", "-fd"] in calls, "同期が作ったファイルを消していない" + # 無視されたファイル（制御用ディレクトリ）まで消さない + assert not any("-x" in c for c in calls if c[:2] == ["git", "clean"]) + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_status_disables_path_quoting(refactor, monkeypatch): + """`core.quotePath` の既定では非 ASCII のパスがエスケープされて `git add` が失敗する。""" + seen: list[list[str]] = [] + monkeypatch.setattr( + refactor, "_git_out", + lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + ) + assert refactor._worktree_changes("/w") == {"plugins/日本語/a.py": " M"} + assert seen[0][:2] == ["-c", "core.quotePath=false"] + + +def test_sync_failure_also_resets_the_index( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が `git add` してから失敗しても、次の実行が再開できること。 + + `git checkout -- .` は staged された差分を戻さないため、index も戻さないと + 清浄性の検査が通らないままになる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + # 同期が index へ追加してから失敗した状況（`M ` は staged） + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", "M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit): + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert ["git", "reset", "--hard", "HEAD"] in calls + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] diff --git a/plugins/ndf-shared/skills/cross-refactoring/SKILL.md b/plugins/ndf-shared/skills/cross-refactoring/SKILL.md index 68fdd069..c057c122 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-shared/skills/cross-refactoring/SKILL.md @@ -38,7 +38,8 @@ allowed-tools: | レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ | | 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない | | 取り消しの単位 | **改善項目ごと（独立している範囲で）。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** | -| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる。生成物の同期は進行側が収束後にまとめて行う | +| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる | +| 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する | | 検証の情報源 | **git と実際のテスト実行。** 結果ファイルの申告は検証に使わない（書き換えるだけで通る検査にしない） | | 投稿 | **AI 自身が `gh api` で投稿する。** ホストの作業文脈に差分やレビュー本文を載せない | | 状態の永続化 | `/.cross_refactoring/cross-refactoring-rf<番号>-state.json` に集約。中断・再開可能 | @@ -58,9 +59,11 @@ allowed-tools: | `--max-items-per-round N` | 1 ラウンドの採用上限 | `5` | | `--severity-threshold LEVEL` | この重要度未満は採用しない | `minor` | | `--test-timeout SEC` | テスト 1 回あたりの上限秒数。超えたら失敗として扱う | `900` | +| `--sync-command CMD` | 生成物を同期するコマンド。**push の直前**に進行側が実行し、差分があれば進行側のコミットとして積む | なし | ```text /ndf:cross-refactoring 130 --scope src/services tests/services --baseline-test "pytest -q" +/ndf:cross-refactoring 130 --scope src --baseline-test "pytest -q" --sync-command "make generate" /ndf:cross-refactoring 130 --scope src --model codex=gpt-5.5 --model claude=opus-5 /ndf:cross-refactoring 130 --scope src --host codex --max-outer-rounds 1 ``` @@ -227,9 +230,8 @@ while :; do # 提案ラウンドの繰り返 rf advance "$ID" || break done -# 収束後にまとめて生成物を同期する（**進行側の責務**）。編集元から配布物を生成する -# 規約を持つリポジトリでは、実装担当に同期させると範囲外の変更が生まれる。 -# 同期が要るなら、ここで生成してから Step 7 の最終ゲートへ渡す。 +# 生成物の同期は `--sync-command` として push の直前に進行側が実行済み。 +# ここで追加の作業は要らない。 ``` ### 終了コード @@ -260,7 +262,9 @@ done | ホストのサブエージェントで適用する | ホストの作業文脈に差分が載り、実装者とレビュー担当の独立性が崩れる | | `launch-cli.sh` に「ホストなら起動しない」分岐を入れる | ホストは適用担当として起動しうる。分岐はランタイム名だけで行う | | `--scope` を省く | 提案が発散し、Pull Request が肥大する | -| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、差分予算を超える。同期は進行側が収束後にまとめて行う | +| 実装担当に生成物を同期させる | 範囲外の変更が生まれ、範囲の検査で全件失敗する。同期は `--sync-command` で進行側が行う | +| 実装担当に push させる | 検証を通る前に公開され、取り消しの反映漏れが Pull Request に残る | +| 生成物を同期するリポジトリで `--sync-command` を省く | pre-push の検査で**あらゆる push が落ちる**。実装担当が同期に手を出し、範囲違反で全件失敗する | | 取り消しの失敗を「全件失敗」として次のラウンドへ進む | 検証を通っていない変更が Pull Request に残る。終了コード 4 は必ず進行ごと止める | | `--dry-run` の出力を実行結果と混同する | 確認用なので git も状態ファイルも触らない。進行は 1 歩も進まない | | 複数の改善項目を 1 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる | 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 487a9b4c..45207583 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,7 +46,10 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" 6. **語彙の受け渡し** — 検証側が持つスメル・手法・重要度の集合を状態ファイルの `vocabulary` へ書く。提案プロンプトはここから**許容値をそのまま列挙する**。 定義を 1 箇所に保ったまま、読ませ方の不確実性を減らすためである -7. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 +7. **生成物の同期コマンドの記録** — `--sync-command` を状態ファイルへ保存する。 + 実行は初期化時ではなく、**push の直前**に進行側が行う。同期を実装担当の責務に + すると範囲外の変更になり、範囲の検査で全件失敗する（実測 0/5） +8. **着手前のテスト** — `--baseline-test` を実行する。**失敗していたら開始しない**。 壊れた状態から始めると、壊したのか元から壊れていたのか区別できない。 この引数は**必須**である。振る舞いが変わっていないことを示す手段が無い書き換えは、 `refactoring` Skill の定義からして構造改善ではない 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 e9b460b8..4609b02a 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 @@ -54,8 +54,44 @@ | 誰が | 何を | | --- | --- | -| 実装担当 | `--scope` の中だけを変更する。生成物・配布物の同期はしない | -| 進行側（ホスト） | 収束後にまとめて生成物を同期する | +| 実装担当 | `--scope` の中だけを変更する。生成物の同期もしないし、**push もしない** | +| 進行側 | 検証を通した後に公開する。`--sync-command` を **push の直前**に実行する | + +#### 公開するのは進行側だけである + +**実装担当に push させない。** 検証を通る前に変更が Pull Request へ現れると、 +取り消しの反映が漏れたときにそのまま残る。`merge-apply` と `merge-fix` は、 +検証が済んでから進行側として push する。 + +生成物の同期も同じ経路に乗せる。`--sync-command` を指定すると、**push の直前**に +進行側が実行し、差分があれば**どの改善項目にも属さないコミット**として積む。 + +| 案 | 結果（実測） | +| --- | --- | +| 同期しない | 同期を検査する pre-push で**あらゆる push が落ちる**。取り消しを反映できない | +| 実装担当に同期させる | 範囲外の変更になり、**採用 5 件が全件失敗**した | +| **進行側が push の直前に同期する** | 採用 | + +同期コミットは取り消しでは積み直されないが、次の push で作り直されるため失われても +問題にならない。同期に失敗したら**中断する**（終了コード 4）。同期できない状態を +公開すると、利用者のリポジトリの検査を壊したまま進むことになる。 + +**同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、同期が +作った差分と元からあった差分を区別できない。区別しようと `git status` の状態コードを +比べても足りず、次の 2 つを取りこぼす。 + +| 取りこぼし | 何が起きるか | +| --- | --- | +| 元から ` M` のファイルを同期がさらに書き換える | 状態コードが変わらず検知できない。その変更がコミットされず **push がまた落ちる** | +| 同期の前から index に staged された変更がある | `git commit` は index を丸ごと含めるため、`git add` の対象を絞っても**検証を受けない変更が公開される** | + +無視されたファイルは判定に現れない。生成物やキャッシュを `.gitignore` へ入れてあれば +止まらない。制御用ディレクトリ（状態ファイル・結果・ログ）も判定から外す。 + +**同期が途中で失敗したら、作った差分を捨ててから中断する。** 残すと次の実行は +清浄性の検査で必ず止まり、`pending_push` の再試行が永久に進まない。着手前が +綺麗だったことは確認済みなので、そこにある変更は全て同期が作ったものだと分かる。 +無視されたファイルは消さない（`git clean` に `-x` を付けない）。 判定は**前方一致だけ**で行い、除外規則は持たない。規則を書けるようにすると、 規則を 1 行足すだけで範囲の検査を骨抜きにできる。 @@ -147,9 +183,14 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 #### 取り消しは判定が出そろってからまとめて行う -**失敗した項目のコミットを Pull Request に残さない。** 実装担当は項目ごとに push して -いるため、状態を `abandoned` にするだけでは差分が残り、以後のレビュー対象にも混入する。 -何が消えるかを先に見たいときは `--dry-run` を付ける。 +**失敗した項目のコミットを Pull Request に残さない。** 公開は進行側が検証の後に行うので、 +検証で落ちた項目はそもそも公開されない。ただし取り消しはローカルの履歴にも必要である +（残すとレビュー対象と以後のラウンドに混入する）。何が消えるかを先に見たいときは +`--dry-run` を付ける。 + +取り消した項目は**「対象外」として記録する**（`deferred_items`）。記録しないと同じ提案が +次のラウンドで再び採用され、同じ理由で失敗する。実測では 3 ランタイム全員から再提案され、 +合意数が最大になって最優先で採用された。 ただし**項目ごとにその場で戻してはならない**。詳細は [取り消しは巻き戻して積み直す](#取り消しは巻き戻して積み直す)を参照する。 @@ -402,9 +443,9 @@ flowchart LR 重複率は `path` + `symbol` + `smell` の集合比較で求める。同じ提案が毎ラウンド出続けて 終わらない状態を検知するためである。 -終了後、生成物の同期が要るリポジトリでは**ここで進行側がまとめて同期する**。 -実装担当に同期させると範囲外の変更が生まれ、差分予算にも影響する（Step 4 の -「範囲の指定は検証にも効かせる」を参照）。 +**ここで追加の同期は要らない。** 生成物の同期は `--sync-command` として +各 push の直前に済んでいる（Step 4 の「公開するのは進行側だけである」を参照）。 +手で同期すると、検証を受けていない差分を作ることになる。 続けて **`/ndf:cross-review `** で Pull Request 全体を承認収束にかける。 レビューはラウンド単位なので、**ラウンドを跨いだ整合はここで見る**。 diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md index ad7fac79..bfaf491e 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md @@ -30,7 +30,6 @@ $RF_ITEMS 2. `plan` の手順を **1 手ずつ**適用する 3. 1 手ごとに `$RF_BASELINE_TEST` を実行する。落ちたら**直前の 1 手を戻す** 4. 通ったらコミットする（**1 手 = 1 コミット**） -5. 項目が終わったら push する ## コミットの規約 @@ -54,13 +53,15 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 + ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます - **生成物・配布物の同期をしない。** このリポジトリに「編集元から配布物を生成する」 - 規約があっても、同期は**進行側が収束後にまとめて行う**責務です。ここで同期すると - 範囲外の変更が生まれ、差分予算も超えます + 規約があっても、同期は**進行側が公開の直前に行う**責務です。ここで同期すると + 範囲外の変更が生まれ、その項目は検証で失敗します - **機能変更を混ぜない。** 振る舞いを変える修正が必要だと分かったら、その項目は 適用せず `status` を `skipped` にして理由を書く - 提案された手順の範囲を超えない。ついでの整理をしない @@ -103,7 +104,7 @@ Impl-Model: $RF_MODEL 取り直します。ここに何と書いても検査結果は変わりません。 - `commits[].sha` は**正確に書いてください**。ここが唯一の対応付けであり、 - 検証に失敗した項目はここに書かれたコミットを取り消して push します。 + 検証に失敗した項目は、ここに書かれたコミットが取り消されます。 書き漏らすと差分が Pull Request に残ります - **このラウンドで作ったコミットは、全て `items[].commits` のどれかに入れてください。** どの項目にも割り当てられていないコミットが 1 件でもあると、**ラウンドごと diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md index b28b967d..a73f3c3e 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md @@ -26,8 +26,7 @@ $RF_ITEMS 2. 各指摘について、**修正するか・しないか**を決める - 修正する: 1 手ずつ直し、その都度テストを実行してコミットする - 修正しない: 根拠を返信する。**黙って閉じない** -3. すべての対応が終わったら push する -4. 対応したスレッドに返信し、`resolveReviewThread` で解決する +3. 対応したスレッドに返信し、`resolveReviewThread` で解決する 指摘のうち、**振る舞いを変えないと直せないもの**は修正しないでください。 その場合は「この改善項目自体を見送るべき」と返信し、解決しないまま残します。 @@ -55,11 +54,12 @@ Impl-Model: $RF_MODEL ## 守ること +- **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った 修正コミットがあると、その修正ラウンドの範囲ごと取り消されます -- **生成物・配布物の同期をしない。** 同期は進行側が収束後にまとめて行います +- **生成物・配布物の同期をしない。** 同期は進行側が公開の直前に行います - 指摘に無い箇所を「ついでに」直さない。ラウンドの差分が膨らみ、 どの変更がどの指摘に対応するのか追えなくなる diff --git a/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py b/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py index 3ad3134b..1a224c66 100755 --- a/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py @@ -137,6 +137,14 @@ def vocabulary() -> dict[str, Any]: # 適用で必ず配置する Skill。ここに無いものは配らない。 REQUIRED_SKILLS = ("refactoring", "tdd-cycle", "quality-gates") +# 生成物を同期したコミットのメッセージ。**どの改善項目にも属さない**ことが分かる形にする。 +SYNC_COMMIT_MESSAGE = ( + "Chore: 生成物を同期する（cross-refactoring 進行側）\n\n" + "実装担当は対象範囲だけを変更するため、生成物が同期されない。\n" + "同期を検査する pre-push を持つリポジトリでも push できるよう、\n" + "公開の直前に進行側がまとめて生成する。" +) + # 実差分行数が見積りのこの倍数を超えたら範囲の逸脱とみなす。 DIFF_BUDGET_FACTOR = 2 @@ -468,7 +476,7 @@ def verify_scope(commit: dict[str, Any], scope: Iterable[str]) -> Optional[str]: more = f" ほか {len(outside) - 5} 件" if len(outside) > 5 else "" return ( f"コミット {commit.get('sha', '?')} が対象範囲の外を変更しています" - f"（{shown}{more}）。生成物の同期は進行側が収束後にまとめて行います。" + f"（{shown}{more}）。生成物の同期は進行側が公開の直前に行います。" "現状固定テストの置き場所が範囲外なら、`--scope` に含めてから実行してください" ) @@ -779,6 +787,8 @@ def cmd_init(args: argparse.Namespace) -> None: "max_items_per_round": args.max_items_per_round, "severity_threshold": args.severity_threshold, "baseline_test": baseline, + # 生成物の同期は**進行側の責務**。push の直前に実行する。 + "sync_command": args.sync_command, "test_timeout": args.test_timeout, "outer_round": 0, "phase": "init", @@ -1244,6 +1254,9 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: if args.dry_run: info("（dry-run）状態ファイルは更新していません") else: + # 項目別の失敗と同じく、**ここで取り消した項目も「対象外」に残す**。 + # 残さないと同じ提案が次のラウンドで再び採用される。 + _defer_abandoned_items(state, entry) statefile.save(path, state) _push_head(state) entry["pending_push"] = False @@ -1323,7 +1336,13 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: # `merged_at` は `_apply_drop` が取り消しの完了時点で立てる。 applied = _apply_drop(path, state, entry, failed) else: + # **全項目が通ったときも進行側が公開する。** 実装担当は push しないため、 + # ここで公開しないとレビュー担当が Pull Request 上の差分へ指摘を書けない。 entry["apply"]["merged_at"] = statefile.now() + entry["pending_push"] = True + statefile.save(path, state) + _push_head(state) + entry["pending_push"] = False statefile.save(path, state) if not applied: @@ -1331,6 +1350,29 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: sys.exit(2) +def _defer_abandoned_items(state: dict[str, Any], entry: dict[str, Any]) -> None: + """このラウンドで取り消した項目を「対象外」として記録する。 + + 記録しないと、**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では適用で失敗した項目が 3 ランタイム全員から再提案され、合意数が最大に + なって最優先で採用された。手順書が「同じ提案が毎ラウンド出続けて収束しない」 + として禁じている状態そのものである。 + + 除外の鍵は `path` + `symbol` + `smell` なので、その 3 つを必ず残す。 + """ + already = {d.get("item_id") for d in state["deferred_items"]} + for item_id in entry["items"]: + item = _find_item(state, item_id, required=False) + if item is None or item.get("status") != "abandoned" or item_id in already: + continue + state["deferred_items"].append({ + "item_id": item_id, + "path": item["path"], "symbol": item["symbol"], "smell": item["smell"], + "round": entry["round"], + "defer_reason": item.get("failure_reason") or "適用結果の検証を通らなかった", + }) + + def _run_drop( path: pathlib.Path, state: dict[str, Any], entry: dict[str, Any], targets: list[str], @@ -1391,6 +1433,9 @@ def _apply_drop( entry["apply_base_sha"] = _git_out(work, ["rev-parse", "HEAD"]) state["phase"] = "propose" + # 取り消した項目は「対象外」として残す。次のラウンドで同じ提案が採用され、 + # 同じ理由で失敗するのを防ぐ。 + _defer_abandoned_items(state, entry) # **取り消しが済んだことを push より先に、印の解除と同じ保存で永続化する。** # 保存せずに push して失敗すると、次の実行が適用の検証をやり直し、取り消しと # 積み直しのコミットを「未割当」と判定してラウンドごと巻き込んでしまう。 @@ -1725,7 +1770,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: # 適用フェーズの未割当コミットと同じ扱いにする。 problems: list[str] = [] accepted: list[tuple[str, str]] = [] # (item_id, sha) - needs_push = False for commit in facts: item_id = (commit.get("trailers") or {}).get("Item-Id") problem = verify_fix_commit(commit, state.get("target_scope") or []) @@ -1762,7 +1806,6 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: statefile.save(path, state) # **push は保存のあと。** ここで push して失敗すると、取り消しコミットは # ローカルに残るのに起点の更新が保存されず、叩き直しで二重に取り消してしまう。 - needs_push = True info("⚠ 修正を取り消したため、解決の申告は採用しません") resolved = set() else: @@ -1785,8 +1828,9 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: + _safe_int(payload.get("elapsed_seconds")) ) statefile.save(path, state) - if needs_push: - _push_with_retry_marker(path, state, entry) + # **取り消したかどうかに関わらず公開する。** 実装担当は push しないため、 + # ここで公開しないと再レビューが Pull Request 上の差分を見られない。 + _push_with_retry_marker(path, state, entry) info(f"修正を取り込みました（解決 {len(resolved)} スレッド / 修正ラウンド {entry['fix_rounds']}）") @@ -2471,8 +2515,146 @@ def _order_newest_first(work: str, shas: list[str]) -> list[str]: return sorted(shas, key=lambda s: rank.get(resolved[s], len(rank))) +def _worktree_changes(work: str) -> dict[str, str]: + """作業ツリーの変更を `パス → 状態` で返す。同期の前後を比べるために使う。 + + 無視されているファイルは現れない（`--porcelain` の既定）。改名は移動先の + パスだけを見る。 + """ + # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ + # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 + out = _git_out( + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + ) + changes: dict[str, str] = {} + for line in (out or "").splitlines(): + if len(line) < 4: + continue + path = line[3:] + if " -> " in path: # 改名。移動先だけを対象にする + path = path.split(" -> ", 1)[1] + changes[path.strip('"')] = line[:2] + return changes + + +def _control_prefix(state: dict[str, Any], work: str) -> Optional[str]: + """作業ディレクトリから見た制御用ディレクトリの相対パス。外にあれば `None`。 + + 状態ファイル・プロンプト・結果・ログの置き場所で、**同期コミットへ入れない**。 + `prepare-worktrees.sh` が無視の設定を置くが、置き場所を環境変数で移した場合や + 配置前に同期が走った場合に備えて、ここでも明示的に外す。 + """ + tmp_dir = str(state.get("tmp_dir") or "") + if not tmp_dir: + return None + try: + relative = pathlib.Path(tmp_dir).resolve().relative_to( + pathlib.Path(work).resolve() + ) + except ValueError: + return None + return f"{relative}/" + + +def _dirty_paths(state: dict[str, Any], work: str) -> list[str]: + """作業ツリーの未コミット変更のパス。制御用ディレクトリは除く。""" + control = _control_prefix(state, work) + return sorted( + path for path in _worktree_changes(work) + if not (control and path.startswith(control)) + ) + + +def _discard_worktree_changes(work: str) -> None: + """作業ツリーと index の未コミット変更を捨てる。**着手前が綺麗なときだけ呼ぶ。** + + **index も戻す。** `git checkout -- .` は staged された差分を戻さないため、 + 同期コマンドが `git add` してから失敗すると清浄性の検査が通らないままになり、 + `pending_push` の再試行が永久に進まない。 + + 無視されたファイル（制御用ディレクトリを含む）は消さない（`git clean` に + `-x` を付けない）。 + """ + for args in (["reset", "--hard", "HEAD"], ["clean", "-fd"]): + subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) + + +def _require_clean_worktree(state: dict[str, Any], work: str) -> None: + """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 + + 汚れたまま同期すると、**同期が作った差分と元からあった差分を区別できない**。 + 区別しようと状態コードを比べても足りず、次の 2 つを取りこぼす。 + + - 元から ` M` のファイルを同期がさらに書き換えても、状態コードは ` M` のままで + 検知できない。その変更がコミットされず、**push がまた落ちる** + - `git commit` は index の内容を全て含めるため、`git add` の対象を絞っても + **先に staged だった変更が検証を受けないまま Pull Request へ入る** + + 無視されたファイルはここに現れない。生成物やキャッシュを `.gitignore` へ + 入れてあれば止まらない。 + """ + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = ", ".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + die( + f"生成物を同期する前に、作業ツリーへ未コミットの変更があります（{shown}{more}）。" + "同期が作った差分と区別できず、検証を受けていない変更を公開しかねないため" + "中断します。コミットするか `.gitignore` へ入れてから再実行してください" + ) + + +def _sync_generated(state: dict[str, Any]) -> None: + """push の直前に生成物を同期し、差分があれば進行側のコミットとして積む。 + + 同期を**実装担当の責務にすると範囲外の変更が生まれ**、範囲の検査で全件失敗する + （実測ではラウンドの採用 5 件が全て範囲外で落ちた）。かといって同期しないと、 + 生成物の同期を検査する pre-push を持つリポジトリでは push そのものが通らず、 + 取り消しを Pull Request へ反映できない。そこで**進行側が push の直前に同期する**。 + + このコミットはどの改善項目にも属さない。取り消しでは積み直されないが、 + 次の push で作り直されるので失われても問題にならない。 + + 同期に失敗したら中断する。**黙って push しない。** 同期できない状態を公開すると、 + 利用者のリポジトリの検査を壊したまま進むことになる。 + """ + command = str(state.get("sync_command") or "").strip() + if not command: + return + work = state["worktrees"]["work"] + # **同期の前に作業ツリーが綺麗であることを求める。** 汚れたまま同期すると、 + # 同期が作った差分と元からあった差分を区別できない。 + _require_clean_worktree(state, work) + code, timed_out = _run_with_timeout( + command, work, _safe_int(state.get("test_timeout"), DEFAULT_TEST_TIMEOUT) + ) + if timed_out or code != 0: + # **途中まで書き換えた差分を残さない。** 残すと次の実行は + # `_require_clean_worktree` で必ず止まり、`pending_push` の再試行が + # 永久に進まなくなる。着手前が綺麗だったことは確認済みなので、 + # ここにある変更は全て同期が作ったものだと分かる。 + _discard_worktree_changes(work) + die( + f"生成物の同期に失敗しました（{command}）: " + + ("打ち切りました" if timed_out else f"終了コード {code}") + + "。同期が作った差分は破棄したので、原因を直せばそのまま再開できます" + ) + produced = _dirty_paths(state, work) + if not produced: + return + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") + + def _push_head(state: dict[str, Any]) -> None: - """head ブランチへ push する。**`--force` は使わない。**""" + """head ブランチへ push する。**`--force` は使わない。** + + **公開するのは進行側だけである。** 実装担当に push させると、検証を通る前に + 変更が Pull Request へ現れ、取り消しの反映漏れがそのまま残る。 + """ + _sync_generated(state) _sh( ["git", "push", "origin", f"HEAD:{state['head_branch']}"], cwd=state["worktrees"]["work"], @@ -2607,6 +2789,10 @@ def main() -> None: init.add_argument("--test-timeout", type=int, default=DEFAULT_TEST_TIMEOUT, help="テスト 1 回あたりの上限秒数。超えたら失敗として扱う " f"(default: {DEFAULT_TEST_TIMEOUT})") + init.add_argument("--sync-command", default=None, + help="生成物を同期するコマンド。**push の直前**に進行側が実行し、" + "差分があれば進行側のコミットとして積む。" + "同期を実装担当にさせると範囲外の変更になるため分離している") init.add_argument("--baseline-test", required=True, help="着手前と各コミットで実行するテストコマンド。" "振る舞い不変を示す手段が無い書き換えは構造改善ではないため必須") diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_abandon_items.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_abandon_items.py index 63b2bb64..41928945 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/tests/test_abandon_items.py +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_abandon_items.py @@ -244,6 +244,8 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: resolved_facts, ) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": claimed, "elapsed_seconds": 12, @@ -396,6 +398,8 @@ def test_broken_fix_result_does_not_crash( """ state_path = _state(tmp_path, [_finding("R1-001")]) env_tmp_dir(state_path) + # 修正の取り込みは取り消しの有無に関わらず push する。既定では実行させない + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: "") write_result(state_path, "codex-fix-r1", { "resolved_thread_ids": broken_ids, "commits": {"sha": "辞書ではあるが配列でない"}, 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 4987b7d6..2427ed3a 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/tests/test_init.py +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_init.py @@ -59,6 +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, "test_timeout": 60, "worktree_root": str(tmp_path / "rf130"), } 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 6b772a0c..28aa9cb7 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 @@ -383,7 +383,7 @@ def test_self_reported_values_cannot_pass_the_check( assert "テストが成功していません" in state["items"][0]["failure_reason"] -def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 @@ -392,6 +392,7 @@ def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0): calls: list[list[str]] = [] picked: list[str] = [] reverted: list[str] = [] + statuses: list[int] = [] def fake_run(cmd, **kwargs): calls.append(list(cmd)) @@ -413,6 +414,14 @@ def fake_git_out(work, args): if picked: return f"new-{picked[-1]}" return "REVERTED_HEAD" if reverted else "HEAD_BEFORE" + if "status" in args: + # 同期の前後で 2 回呼ばれる。1 回目が同期前、2 回目以降が同期後。 + # 既定は「同期前も後も差分なし」 + statuses.append(len(statuses)) + if sync_dirty is False: + return "" + before, after = sync_dirty + return before if len(statuses) == 1 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -647,10 +656,13 @@ def test_blank_scope_entry_is_ignored(refactor): assert not refactor.path_in_scope("dist/foo.py", ["", " ", "src"]) -def test_no_push_when_nothing_was_reverted( +def test_push_happens_once_per_merge_apply( refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts ): - """全項目が通ったときに余計な push をしない。""" + """全項目が通ったときも公開するが、push は 1 回だけにすること。 + + 公開するのは進行側だけである（実装担当は push しない）。 + """ items = [item(item_id="R1-001")] state_path = _state_with_items(tmp_path, items) env_tmp_dir(state_path) @@ -666,7 +678,7 @@ def test_no_push_when_nothing_was_reverted( lambda cmd, **kw: subprocess.CompletedProcess(cmd, 0, "", ""), ) refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) - assert pushes == [] + assert len([c for c in pushes if c[:2] == ["git", "push"]]) == 1 def test_dry_run_touches_neither_git_nor_state( @@ -1205,3 +1217,330 @@ def test_push_failure_after_a_successful_drop_only_retries_the_push( entry = read_state(state_path)["rounds"][0] assert entry["pending_push"] is False assert entry["apply"]["applied"] == ["R1-001"] + + +# ---------- 適用で失敗した項目を「対象外」へ ---------- + +def test_failed_items_are_deferred( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """適用の検証で失敗した項目を「対象外」として記録すること。 + + 記録しないと**同じ提案が次のラウンドで再び採用され、同じ理由で失敗する**。 + 実測では 3 ランタイム全員から再提案され、合意数が最大になって最優先で採用された。 + """ + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-002"], "失敗した項目だけを対象外にすること" + entry = deferred["R1-002"] + # 次ラウンドの除外は path + symbol + smell の組で行われる + assert (entry["path"], entry["symbol"], entry["smell"]) == ( + "src/foo.py", "Foo.handle", "long_method") + assert "差分予算" in entry["defer_reason"] + + +def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts): + """叩き直しても対象外の記録を重複させないこと。""" + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + args = type("A", (), {"id": 130, "round": 1, "dry_run": False})() + refactor.cmd_merge_apply(args) + refactor.cmd_merge_apply(args) + assert [d["item_id"] for d in read_state(state_path)["deferred_items"]] == ["R1-002"] + + +# ---------- push の直前に生成物を同期する ---------- + +def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["sync_command"] = command + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + return state_path + + +def test_push_syncs_generated_files_first( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """push の直前に同期し、差分があれば進行側のコミットとして積むこと。 + + 同期を実装担当にさせると範囲外の変更になり、範囲の検査で全件失敗する。 + かといって同期しないと、同期を検査する pre-push では push が通らない。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + order: list[str] = [] + _drop_env(refactor, monkeypatch, + sync_dirty=("", "?? plugins/generated/a.py")) + monkeypatch.setattr( + refactor, "_sh", + lambda cmd, **k: order.append("push" if cmd[:2] == ["git", "push"] else cmd[1]) + or "", + ) + ran: list[tuple[str, str]] = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append((command, cwd)) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert ran and ran[0][0] == "make build", "同期コマンドを実行していない" + assert "add" in order and "commit" in order, "同期の差分をコミットしていない" + assert order.index("commit") < order.index("push"), "コミットより先に push している" + + +def test_sync_failure_aborts_without_pushing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期に失敗したら中断する。黙って push しない。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_no_sync_command_runs_nothing( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """`--sync-command` 未指定なら同期は走らない（既存の利用者に影響しない）。""" + _two_item_apply(tmp_path, env_tmp_dir, git_facts) + _drop_env(refactor, monkeypatch) + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert ran == [] + + +def test_merge_apply_pushes_even_when_every_item_passes( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """全項目が通ったときも進行側が push すること。 + + 実装担当が push しなくなったため、ここで公開しないとレビュー担当が + Pull Request 上の差分へ指摘を書けない。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + git_facts({"ok111": fact(sha="ok111")}) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + calls, pushes = _drop_env(refactor, monkeypatch) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in pushes if c[:2] == ["git", "push"]], "push していない" + for cmd in pushes: + assert "--force" not in cmd and "--no-verify" not in cmd + entry = read_state(state_path)["rounds"][0] + assert entry["pending_push"] is False + assert entry["apply"]["applied"] == ["R1-001"] + + +def test_sync_aborts_when_the_worktree_is_dirty( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期の前に作業ツリーが汚れていたら中断すること。 + + 汚れたまま同期すると、同期が作った差分と元からあった差分を区別できない。 + 状態コードを比べても、元から ` M` のファイルを同期がさらに書き換えた場合を + 取りこぼす。`git commit` は index を丸ごと含めるため、staged 済みの変更も + 検証を受けないまま公開される。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(" M src/edited.py", " M src/edited.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + ran: list = [] + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: ran.append(command) or (0, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + assert ran == [], "汚れたまま同期を走らせている" + assert [c for c in staged if c[:2] == ["git", "push"]] == [] + + +def test_dirt_inside_the_control_directory_does_not_abort( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """制御用ディレクトリの中は汚れていても止めないこと。 + + 状態ファイル・結果・ログは常にそこへ書かれる。 + """ + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=(f"?? {control}/codex-apply-r1-result.json", + f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + assert [c for c in staged if c[:2] == ["git", "add"]] == [ + ["git", "add", "--", "generated/a.py"]] + + +def test_sync_excludes_the_control_directory( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """状態ファイル・結果・ログの置き場所を同期コミットへ入れないこと。""" + state_path = _sync_state(tmp_path, env_tmp_dir, git_facts) + # 既定の配置では制御用ディレクトリが作業ディレクトリの中にある + state = read_state(state_path) + state["worktrees"]["work"] = str(state_path.parent.parent) + state_path.write_text(__import__("json").dumps(state), encoding="utf-8") + control = state_path.parent.name # `.cross_refactoring` + staged: list[list[str]] = [] + _drop_env( + refactor, monkeypatch, + sync_dirty=("", f"?? {control}/codex-apply-r1-result.json\n M generated/a.py"), + ) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + adds = [c for c in staged if c[:2] == ["git", "add"]] + assert adds == [["git", "add", "--", "generated/a.py"]] + + +def test_sync_without_changes_makes_no_commit( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期しても差分が出なければ、空のコミットを積まないこと。""" + _sync_state(tmp_path, env_tmp_dir, git_facts) + staged: list[list[str]] = [] + _drop_env(refactor, monkeypatch, sync_dirty=("", "")) + monkeypatch.setattr(refactor, "_sh", lambda cmd, **k: staged.append(list(cmd)) or "") + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (0, False), + ) + refactor.cmd_merge_apply(type("A", (), {"id": 130, "round": 1, "dry_run": False})()) + + assert [c for c in staged if c[:2] == ["git", "commit"]] == [] + assert [c for c in staged if c[:2] == ["git", "push"]], "push はすること" + + +def test_whole_round_failure_also_defers_items( + refactor, tmp_path, env_tmp_dir, no_git, git_facts +): + """ラウンドごと取り消す経路でも「対象外」として記録すること。 + + 項目別の失敗だけを記録すると、未割当コミットで落ちた提案が次のラウンドで + 再び採用される。 + """ + items = [item(item_id="R1-001")] + state_path = _state_with_items(tmp_path, items) + env_tmp_dir(state_path) + # 範囲に 2 件あるが、申告は 1 件だけ（未割当コミットあり） + git_facts({"ok111": fact(sha="ok111")}, in_range=["sneaky", "ok111"]) + write_result(state_path, "codex-apply-r1", { + "base_sha": "aaa", + "items": [{"item_id": "R1-001", "commits": [{"sha": "ok111"}]}], + }) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == 2 + + state = read_state(state_path) + deferred = {d["item_id"]: d for d in state["deferred_items"]} + assert list(deferred) == ["R1-001"] + assert "割り当てられていない" in deferred["R1-001"]["defer_reason"] + + +def test_sync_failure_discards_what_it_produced( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が途中で失敗したら、作った差分を捨てて再開できる状態にすること。 + + 残すと次の実行は清浄性の検査で必ず止まり、`pending_push` の再試行が + 永久に進まなくなる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", " M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit) as e: + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert e.value.code == refactor.ABORT + # index も戻す。`git checkout -- .` では staged された差分が残り、 + # 清浄性の検査が通らないままになる + assert ["git", "reset", "--hard", "HEAD"] in calls, "index を戻していない" + assert ["git", "clean", "-fd"] in calls, "同期が作ったファイルを消していない" + # 無視されたファイル（制御用ディレクトリ）まで消さない + assert not any("-x" in c for c in calls if c[:2] == ["git", "clean"]) + assert [c for c in pushes if c[:2] == ["git", "push"]] == [] + + +def test_status_disables_path_quoting(refactor, monkeypatch): + """`core.quotePath` の既定では非 ASCII のパスがエスケープされて `git add` が失敗する。""" + seen: list[list[str]] = [] + monkeypatch.setattr( + refactor, "_git_out", + lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + ) + assert refactor._worktree_changes("/w") == {"plugins/日本語/a.py": " M"} + assert seen[0][:2] == ["-c", "core.quotePath=false"] + + +def test_sync_failure_also_resets_the_index( + refactor, tmp_path, env_tmp_dir, monkeypatch, git_facts +): + """同期が `git add` してから失敗しても、次の実行が再開できること。 + + `git checkout -- .` は staged された差分を戻さないため、index も戻さないと + 清浄性の検査が通らないままになる。 + """ + _sync_state(tmp_path, env_tmp_dir, git_facts) + # 同期が index へ追加してから失敗した状況（`M ` は staged） + calls, pushes = _drop_env(refactor, monkeypatch, sync_dirty=("", "M generated/a.py")) + monkeypatch.setattr( + refactor, "_run_with_timeout", + lambda command, cwd, timeout, grace=5.0: (1, False), + ) + with pytest.raises(SystemExit): + refactor.cmd_merge_apply( + type("A", (), {"id": 130, "round": 1, "dry_run": False})() + ) + assert ["git", "reset", "--hard", "HEAD"] in calls + assert [c for c in pushes if c[:2] == ["git", "push"]] == []