Skip to content
132 changes: 132 additions & 0 deletions issues/issue-113-cross-refactoring-push-ownership.md
Original file line numberDiff line numberDiff line change
@@ -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` が収束
14 changes: 9 additions & 5 deletions plugins/ndf-claude/skills/cross-refactoring/SKILL.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -38,7 +38,8 @@ allowed-tools:
| レビューの単位 | **提案ラウンドの差分全体**に対して 1 回。項目ごとに回すと CLI 起動回数が採用件数に比例して膨らむ |
| 収束しない項目 | **捨てる。** リファクタリングは任意の作業なので、揉める提案を Pull Request に残さない |
| 取り消しの単位 | **改善項目ごと(独立している範囲で)。** 範囲を新しい順に全て戻し、残す項目を積み直す。同一ファイルの隣接行を触る項目どうしは git だけでは分離できないため、そのときは**ラウンド全件へ退避する** |
| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる。生成物の同期は進行側が収束後にまとめて行う |
| 範囲の扱い | `--scope` は**検証にも効く**。範囲外を触ったコミットを含む項目は失敗になる |
| 公開の責務 | **進行側だけが、検証を通した後に push する。** 実装担当は push しない。生成物の同期は `--sync-command` として push の直前に進行側が実行する |
| 検証の情報源 | **git と実際のテスト実行。** 結果ファイルの申告は検証に使わない(書き換えるだけで通る検査にしない) |
| 投稿 | **AI 自身が `gh api` で投稿する。** ホストの作業文脈に差分やレビュー本文を載せない |
| 状態の永続化 | `<work>/.cross_refactoring/cross-refactoring-rf<番号>-state.json` に集約。中断・再開可能 |
Expand All@@ -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
```
Expand DownExpand Up@@ -227,9 +230,8 @@ while :; do # 提案ラウンドの繰り返
rf advance "$ID" || break
done

# 収束後にまとめて生成物を同期する(**進行側の責務**)。編集元から配布物を生成する
# 規約を持つリポジトリでは、実装担当に同期させると範囲外の変更が生まれる。
# 同期が要るなら、ここで生成してから Step 7 の最終ゲートへ渡す。
# 生成物の同期は `--sync-command` として push の直前に進行側が実行済み。
# ここで追加の作業は要らない。
```

### 終了コード
Expand DownExpand Up@@ -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 コミットにまとめる | 取り消し範囲が項目単位で決まらなくなる。適用結果の検証で失敗になる |
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 の定義からして構造改善ではない
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 行足すだけで範囲の検査を骨抜きにできる。
Expand DownExpand Up@@ -147,9 +183,14 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検

#### 取り消しは判定が出そろってからまとめて行う

**失敗した項目のコミットを Pull Request に残さない。** 実装担当は項目ごとに push して
いるため、状態を `abandoned` にするだけでは差分が残り、以後のレビュー対象にも混入する。
何が消えるかを先に見たいときは `--dry-run` を付ける。
**失敗した項目のコミットを Pull Request に残さない。** 公開は進行側が検証の後に行うので、
検証で落ちた項目はそもそも公開されない。ただし取り消しはローカルの履歴にも必要である
(残すとレビュー対象と以後のラウンドに混入する)。何が消えるかを先に見たいときは
`--dry-run` を付ける。

取り消した項目は**「対象外」として記録する**(`deferred_items`)。記録しないと同じ提案が
次のラウンドで再び採用され、同じ理由で失敗する。実測では 3 ランタイム全員から再提案され、
合意数が最大になって最優先で採用された。

ただし**項目ごとにその場で戻してはならない**。詳細は
[取り消しは巻き戻して積み直す](#取り消しは巻き戻して積み直す)を参照する。
Expand DownExpand Up@@ -402,9 +443,9 @@ flowchart LR
重複率は `path` + `symbol` + `smell` の集合比較で求める。同じ提案が毎ラウンド出続けて
終わらない状態を検知するためである。

終了後、生成物の同期が要るリポジトリでは**ここで進行側がまとめて同期する**。
実装担当に同期させると範囲外の変更が生まれ、差分予算にも影響する(Step 4 の
「範囲の指定は検証にも効かせる」を参照)
**ここで追加の同期は要らない。** 生成物の同期は `--sync-command` として
各 push の直前に済んでいる(Step 4 の「公開するのは進行側だけである」を参照)。
手で同期すると、検証を受けていない差分を作ることになる

続けて **`/ndf:cross-review <PR>`** で Pull Request 全体を承認収束にかける。
レビューはラウンド単位なので、**ラウンドを跨いだ整合はここで見る**。
Expand Down
Loading
Loading