From e834c3506086c9a5cb0958b9fafb7c708d2f4020 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Tue, 18 Aug 2026 21:34:13 +0000 Subject: [PATCH] =?UTF-8?q?Fix:=20cross-refactoring=20=E3=81=AE=E9=80=B2?= =?UTF-8?q?=E8=A1=8C=E3=81=8C=E6=AD=A2=E3=81=BE=E3=82=8B=E4=B8=8D=E5=85=B7?= =?UTF-8?q?=E5=90=88=E3=81=A8=20cross-review=20=E3=81=AE=E6=8A=95=E7=A8=BF?= =?UTF-8?q?=E7=A2=BA=E8=AA=8D=E3=82=92=E7=9B=B4=E3=81=99=EF=BC=88v8.5.0?= =?UTF-8?q?=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 再々検証（PR #125）で見つかった 4 件と、投稿の成否を突き合わせない課題を直した。 - 生成物の同期コミットが必ず失敗する 固定幅で読む `git status --porcelain` の出力を `strip()` していたため、 変更パスの先頭 1 文字が欠けて `git add` が落ちていた。固定幅で読む出力には `strip` を適用しない - 同期の後段で落ちると差分が作業ツリーに残る `git add` / `git commit` の失敗でも、同期が作った差分を捨ててから中断する - 実装担当がコミットを作れない条件が生じる 生成物の検査を通す必要がないことを手順書へ明示し、迂回の手段を 1 つに定めた。 取り込みの前に、コミットされなかった変更を捨てる - 見送りの後に読み取り用の作業ディレクトリを同期していない 提案の直前に毎回同期する。あわせて提案とレビューの打ち切り時間を明示した - cross-review が投稿の成否を突き合わせていない 申告されたコメント数を GitHub 側の実数と照合し、届いていなければ中断する。 取得できなかった場合は申告を採用する Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WxxF27RXyg3QMjayii5dp1 --- .claude-plugin/marketplace.json | 2 +- AGENTS.md | 2 +- CLAUDE.md | 4 +- README.md | 12 +- plugins/ndf-claude/.claude-plugin/plugin.json | 4 +- .../skills/cross-refactoring/SKILL.md | 11 +- .../docs/01-state-and-propose.md | 21 +- .../docs/02-apply-and-review.md | 26 ++ .../skills/cross-refactoring/prompts/apply.md | 7 + .../skills/cross-refactoring/prompts/fix.md | 7 + .../cross-refactoring/scripts/refactor.py | 59 ++++- .../tests/test_abandon_items.py | 16 +- .../tests/test_judge_review.py | 2 +- .../tests/test_merge_apply.py | 60 +++-- .../tests/test_sync_generated.py | 223 ++++++++++++++++++ .../ndf-claude/skills/cross-review/SKILL.md | 3 + .../cross-review/docs/01-state-and-review.md | 19 ++ .../skills/cross-review/scripts/state.py | 58 +++++ .../tests/test_state_posted_comments.py | 191 +++++++++++++++ plugins/ndf-codex/.codex-plugin/plugin.json | 4 +- plugins/ndf-codex/README.md | 6 +- .../skills/cross-refactoring/SKILL.md | 11 +- .../docs/01-state-and-propose.md | 21 +- .../docs/02-apply-and-review.md | 26 ++ .../skills/cross-refactoring/prompts/apply.md | 7 + .../skills/cross-refactoring/prompts/fix.md | 7 + .../cross-refactoring/scripts/refactor.py | 59 ++++- .../tests/test_abandon_items.py | 16 +- .../tests/test_judge_review.py | 2 +- .../tests/test_merge_apply.py | 60 +++-- .../tests/test_sync_generated.py | 223 ++++++++++++++++++ .../ndf-codex/skills/cross-review/SKILL.md | 3 + .../cross-review/docs/01-state-and-review.md | 19 ++ .../skills/cross-review/scripts/state.py | 58 +++++ .../tests/test_state_posted_comments.py | 191 +++++++++++++++ plugins/ndf-kiro/README.md | 2 +- plugins/ndf-kiro/VERSION | 2 +- .../skills/cross-refactoring/SKILL.md | 11 +- .../docs/01-state-and-propose.md | 21 +- .../docs/02-apply-and-review.md | 26 ++ .../skills/cross-refactoring/prompts/apply.md | 7 + .../skills/cross-refactoring/prompts/fix.md | 7 + .../cross-refactoring/scripts/refactor.py | 59 ++++- .../tests/test_abandon_items.py | 16 +- .../tests/test_judge_review.py | 2 +- .../tests/test_merge_apply.py | 60 +++-- .../tests/test_sync_generated.py | 223 ++++++++++++++++++ plugins/ndf-kiro/skills/cross-review/SKILL.md | 3 + .../cross-review/docs/01-state-and-review.md | 19 ++ .../skills/cross-review/scripts/state.py | 58 +++++ .../tests/test_state_posted_comments.py | 191 +++++++++++++++ .../skills/cross-refactoring/SKILL.md | 11 +- .../docs/01-state-and-propose.md | 21 +- .../docs/02-apply-and-review.md | 26 ++ .../skills/cross-refactoring/prompts/apply.md | 7 + .../skills/cross-refactoring/prompts/fix.md | 7 + .../cross-refactoring/scripts/refactor.py | 59 ++++- .../tests/test_abandon_items.py | 16 +- .../tests/test_judge_review.py | 2 +- .../tests/test_merge_apply.py | 60 +++-- .../tests/test_sync_generated.py | 223 ++++++++++++++++++ .../ndf-shared/skills/cross-review/SKILL.md | 3 + .../cross-review/docs/01-state-and-review.md | 19 ++ .../skills/cross-review/scripts/state.py | 58 +++++ .../tests/test_state_posted_comments.py | 191 +++++++++++++++ 65 files changed, 2680 insertions(+), 170 deletions(-) create mode 100644 plugins/ndf-claude/skills/cross-refactoring/tests/test_sync_generated.py create mode 100644 plugins/ndf-claude/skills/cross-review/tests/test_state_posted_comments.py create mode 100644 plugins/ndf-codex/skills/cross-refactoring/tests/test_sync_generated.py create mode 100644 plugins/ndf-codex/skills/cross-review/tests/test_state_posted_comments.py create mode 100644 plugins/ndf-kiro/skills/cross-refactoring/tests/test_sync_generated.py create mode 100644 plugins/ndf-kiro/skills/cross-review/tests/test_state_posted_comments.py create mode 100644 plugins/ndf-shared/skills/cross-refactoring/tests/test_sync_generated.py create mode 100644 plugins/ndf-shared/skills/cross-review/tests/test_state_posted_comments.py diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 05e73c30..a08f781a 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,7 @@ { "name": "ndf", "source": "./plugins/ndf-claude", - "description": "Claude Code plugin (v8.4.0): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications." + "description": "Claude Code plugin (v8.5.0): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications." }, { "name": "playwright-kit", diff --git a/AGENTS.md b/AGENTS.md index f4ed2bc7..5a4141ad 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,7 +77,7 @@ ai-plugins/ ## NDFプラグインについて -**NDFプラグイン**は、このマーケットプレイスの主要プラグインです（v8.4.0）。plugin 名は全ランタイムで `ndf` を維持し、配布物は `plugins/ndf-claude` / `plugins/ndf-codex` / `plugins/ndf-kiro` に分離しています。 +**NDFプラグイン**は、このマーケットプレイスの主要プラグインです（v8.5.0）。plugin 名は全ランタイムで `ndf` を維持し、配布物は `plugins/ndf-claude` / `plugins/ndf-codex` / `plugins/ndf-kiro` に分離しています。 - 共通編集元は `plugins/ndf-shared/` - Claude Code版は 8個の専門サブエージェント、公開Skills、SessionStart/Stopフックを提供 - Codex版は Codex向け公開Skillsと任意Slack通知hookを提供 diff --git a/CLAUDE.md b/CLAUDE.md index a7648130..8258081b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +29,7 @@ skills/ → 実行可能なワークフロー 詳細は `docs/specifications/ndf-knowledge-and-kiro.md` を参照。 -## NDF v8.4.0 の Skill 構成 +## NDF v8.5.0 の Skill 構成 Skill は 31 個で、配布は `plugins/ndf-shared/manifests/` が唯一の基準（Claude Code 27 / Codex 25 / Kiro 26）。ブラウザ自動テストの 4 個は `playwright-kit` プラグインへ分離した（`plugins/playwright-kit-shared/`）。frontmatter の書き方は `plugins/ndf-shared/skills/README.md` の規約に従い、`python3 scripts/check-skill-frontmatter.py` で検査する。利用実績と維持・統合・削除の判定は `docs/specifications/ndf-skill-inventory.md` に記録する。 @@ -47,6 +47,8 @@ v8.3.0 で `cross-refactoring` の公開の責務を進行側へ一本化した v8.4.0 で `markdown-writing` に「敬意と節度のある表現で書く」（ルール 4）を追加し、以降のルール番号を 1 つ繰り下げた。強い否定語・過剰な装飾語・根拠の曖昧な断定の 3 種を扱い、セルフチェックの grep も 3 種に分けた。あわせて `01-diagram-guide.md` を図表ルールの冒頭から手順として読ませ、上限値や記法は SKILL.md へ書かずガイド側に置く構成にした（実測で読み込み挙動を確認した結果）。`pr` は完了報告を `### 6. 完了報告` として手順に組み込み、テンプレートと PR URL の書き方（生の URL を書く）を定めた。 +v8.5.0 で `cross-refactoring` の再々検証（PR #125）で見つかった不具合 4 件と、`cross-review` の投稿確認を直した。進行を止めていたのは生成物の同期で、`git status --porcelain` を固定幅で読む箇所が出力全体を `strip()` していたため、変更パスの先頭 1 文字が欠けていた。あわせて実装担当が残した未コミット変更を取り込みの前に捨てるようにし、提案の直前に読み取り用の作業ディレクトリを同期するようにした。`cross-review` は申告されたコメント数を GitHub 側の実数と突き合わせる。詳細は `issues/issue-113-cross-refactoring-re-retrial.md`。 + v6.0.0 の対応表（`review` → `pr-review`）は予告どおり削除済み。v6.0.0 以前から移行する場合は v6.1.0 の `ndf-policies` を参照する。 ## cross-refactoring diff --git a/README.md b/README.md index 74bccfed..3d4d9b2e 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ Claude Code / Codex / Kiro CLI向けのスキル・MCP設定を共有するた このマーケットプレイスは、チーム全体でAI開発ツール（Claude Code / Codex / Kiro CLI）の導入を加速するための事前設定されたプラグインを提供します。 -**NDFプラグイン v8.4.0** は、同じ `ndf@ai-plugins` という名前で Claude Code / Codex / Kiro CLI へ配布されるランタイム別プラグインです。共通ソースは `plugins/ndf-shared/` に集約し、利用者が install する配布物は `plugins/ndf-claude/` / `plugins/ndf-codex/` / `plugins/ndf-kiro/` に分かれています。 +**NDFプラグイン v8.5.0** は、同じ `ndf@ai-plugins` という名前で Claude Code / Codex / Kiro CLI へ配布されるランタイム別プラグインです。共通ソースは `plugins/ndf-shared/` に集約し、利用者が install する配布物は `plugins/ndf-claude/` / `plugins/ndf-codex/` / `plugins/ndf-kiro/` に分かれています。 - **公開Skills**: Claude Code向け core 27個、Kiro向け core 26個、Codex向け core 25個に分離。 - **元Skills（30個）**: @@ -102,9 +102,17 @@ kiro-cli chat --agent ndf | プラグイン名 | バージョン | 説明 | 詳細 | |------------|----------|------|------| -| **ndf** | 8.4.0 | Claude Code / Codex / Kiro CLI 向けに runtime 別配布物を提供する NDF プラグイン。8個の専門エージェント（Claude版）、公開Skills（Claude Code向け core 27個、Kiro向け core 26個、Codex向け core 25個）、Claude SessionStart/Stopフック、Codex/Kiro向け通知・実行補助を提供。v4.0.0 で Codex MCP サーバを廃止し、`/ndf:external-ai` skill + `corder` エージェント経由の CLI 直接実行に一本化。 | [Claude](./plugins/ndf-claude/README.md) / [Codex](./plugins/ndf-codex/README.md) / [Kiro](./plugins/ndf-kiro/README.md) | +| **ndf** | 8.5.0 | Claude Code / Codex / Kiro CLI 向けに runtime 別配布物を提供する NDF プラグイン。8個の専門エージェント（Claude版）、公開Skills（Claude Code向け core 27個、Kiro向け core 26個、Codex向け core 25個）、Claude SessionStart/Stopフック、Codex/Kiro向け通知・実行補助を提供。v4.0.0 で Codex MCP サーバを廃止し、`/ndf:external-ai` skill + `corder` エージェント経由の CLI 直接実行に一本化。 | [Claude](./plugins/ndf-claude/README.md) / [Codex](./plugins/ndf-codex/README.md) / [Kiro](./plugins/ndf-kiro/README.md) | | **playwright-kit** | 1.0.0 | Playwright による E2E テストの計画・実装・証跡管理を提供するプラグイン。ページ役割からのテスト計画、動画 / trace 付きスクリプト実装、レポート生成と Drive 保管、playwright_kit ランタイム（init、a11y / CWV スキャン）の 4 Skill。NDF v7.0.0 で分離。 | [Claude](./plugins/playwright-kit-claude/README.md) | +### NDF v8.5.0 の主な変更 + +- **生成物の同期が止まる不具合を修正**: `git status --porcelain` を固定幅で読む箇所が出力全体を `strip()` していたため、変更パスの先頭 1 文字が欠けて `git add` が失敗していた +- **実装担当の置き土産を捨ててから取り込む**: コミットされなかった変更は検証を受けていないため、`merge-apply` / `merge-fix` が取り込みの前に捨てる +- **実装担当のコミットとフックの関係を明示**: 生成物の同期を検査するリポジトリでも、そのコミットだけフックを外してよいことを手順書に定めた +- **提案の直前に読み取り用を同期**: 取り消しで進んだ HEAD が届かず、消えたコードへの提案が返る問題を解消 +- **cross-review が投稿の成否を確認**: 申告されたコメント数を GitHub 側の実数と突き合わせ、届いていなければ中断する + ### NDF v8.4.0 の主な変更 **`/ndf:markdown-writing` に敬意ある表現のルールを追加し、図表ガイドを読む位置を明示しました。** diff --git a/plugins/ndf-claude/.claude-plugin/plugin.json b/plugins/ndf-claude/.claude-plugin/plugin.json index bfb423d5..690c8d58 100644 --- a/plugins/ndf-claude/.claude-plugin/plugin.json +++ b/plugins/ndf-claude/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ndf", - "version": "8.4.0", - "description": "Claude Code plugin (v8.4.0): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications.", + "version": "8.5.0", + "description": "Claude Code plugin (v8.5.0): 8 specialized agents and 27 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, statusline, external AI delegation (Codex/Gemini), transcript retention guard, and optional Slack notifications.", "author": { "name": "takemi-ohama", "url": "https://github.com/takemi-ohama" diff --git a/plugins/ndf-claude/skills/cross-refactoring/SKILL.md b/plugins/ndf-claude/skills/cross-refactoring/SKILL.md index c057c122..ac855f65 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-claude/skills/cross-refactoring/SKILL.md @@ -193,11 +193,17 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" while :; do # 提案ラウンドの繰り返し rf_eval start-round "$ID" || break # 終了コード 1 = 繰り返し終了 + # **提案の直前に読み取り用を同期する。** 前ラウンドの取り消しで HEAD が進んで + # いるため、同期しないと**消えたコードに対する提案**が返る（実測: 取り消しで + # 消えた関数へ 2 件）。HEAD が変わっていなければ何も起きない。 + "$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done + # 提案の所要は参加ランタイムと回線状況で振れる（実測 90〜285 秒）。既定の + # 打ち切りに任せず、明示する。 "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 rf merge-proposals "$ID" || break # 終了コード 2 = 採用 0 件 "$SCRIPTS/launch-cli.sh" "$IMPL" apply "$ID" "$ROUND" @@ -213,7 +219,7 @@ while :; do # 提案ラウンドの繰り返 "$SCRIPTS/launch-cli.sh" "$r" review "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$REVIEWERS_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-review-r$ROUND" + --stem-template "{agent}-review-r$ROUND" --timeout 900 rf judge-review "$ID" "$ROUND"; rc=$? [ $rc -eq 0 ] && break # 2 者とも承認 [ $rc -eq 3 ] && continue # 形式不正 — 差し戻して再レビュー @@ -271,6 +277,7 @@ done | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | | `git push --force` / `--no-verify` を使う | 他者の作業を消す。検証を飛ばす | +| 実装担当のコミットでフックの通し方を決めない | 生成物の同期を検査するリポジトリでは、同期の禁止と両立せずコミットを作れなくなる。迂回してよい手段を 1 つ定める | | 提案とレビューにホストを混ぜる | 実装者と評価者が同一モデルになりうる。初期化時に検査して失敗させている | | kiro を既定モデルのまま計測する | `auto` は実際に選ばれたモデルを取得できない | | 提案フェーズでコードを直す | 提案は読むだけ。直すのは実装担当 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 45207583..fb751789 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 @@ -182,11 +182,12 @@ claude 17 本 / codex 1 メソッド / kiro 0 本と揃わなかった。最後 ## Step 2: 提案 ```bash +"$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 ``` 3 CLI を並列で起動し、同一のプロンプトで提案させる。**提案フェーズにホストは現れない** @@ -195,6 +196,24 @@ done 提出形式は [prompts/propose.md](../prompts/propose.md) にある。 +### 提案の直前に読み取り用を同期する + +**同期が要るのは HEAD が進んだときであって、特定のフェーズの後ではない。** +適用と修正の直後だけを同期していると、取り消しで進んだ HEAD が読み取り用へ +届かない。実測では、ラウンドを全件取り消した次の提案で、**取り消しによって +消えた関数**に対する提案が 2 件返った。統合は対象の実在を検査しないため、 +そのまま採用され、適用で必ず失敗する。 + +提案の直前に同期しておけば、どのフェーズを経ていても読み取り用は最新になる。 +HEAD が変わっていなければ何も起きないので、重ねて呼んでも無駄がない。 + +### 打ち切りまでの時間を明示する + +提案の所要はランタイムと回線状況で振れる（実測 90〜285 秒）。既定の打ち切りに +任せると、分析そのものは進んでいるのに時間切れで結果を捨てることがある。 +結果ファイルには `idle_seconds` が残るので、**止まっていたのか間に合わなかったのか**は +後から読める。 + ### 結果ファイル名にラウンド番号を入れる CLI の起動時に同名の結果ファイルを消すため、**提案の結果ファイル名にもラウンド番号が 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 4609b02a..c5d18be3 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 @@ -199,6 +199,32 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 終わると、Pull Request 側には未検証の差分が残るのに、次の実行は処理済みガードで 素通りしてしまう。印があれば、次の実行が判定より先に再送信する。 +### 実装担当のコミットはフックの検査を通さなくてよい + +**生成物の同期を pre-commit で検査するリポジトリでは、そのままだとコミットを作れない。** +実装担当は範囲内だけを変更するので生成物は必ず古くなり、同期は禁じられている。 + +公開は進行側が検証を通してから行うため、**実装担当のコミット時点で生成物が古いのは +設計どおり**である。そこで、フックが原因でコミットできないときは +`git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外させる。 + +- 禁止は `--no-verify` だけを名指しにしない。手段の名前で書き分けると、同じことが + 別の名前で起こる。実測では、同じ実装担当が適用フェーズではフックを迂回し、 + 修正フェーズでは迂回せず 0 コミットで終えた +- **直した内容は必ずコミットさせる。** 作業ツリーに置いたまま終えると、検証を + 受けていない変更として `merge-apply` / `merge-fix` が捨てる + +### 取り込みの前に置き土産を捨てる + +`merge-apply` と `merge-fix` は、実装担当が残した未コミットの変更を捨ててから +結果を読む。コミットされなかった変更はどの検証も受けておらず、公開する道が無い。 + +残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正フェーズで +コミットを作れなかった実装担当が直しかけの差分を置いたまま終え、続く `merge-fix` が +「修正 0 件」として先へ進むこともできなくなった。 + +制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、消えない。 + ### コミットトレーラーの形式 適用と修正のコミットメッセージ本文の末尾に、**git のトレーラー形式**で実行主体を残す。 diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md index bfaf491e..ec9c8b94 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/apply.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます diff --git a/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md index a73f3c3e..bb2180a9 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-claude/skills/cross-refactoring/prompts/fix.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$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 1a224c66..2aa3c0d2 100755 --- a/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-claude/skills/cross-refactoring/scripts/refactor.py @@ -1099,6 +1099,7 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: path, state = _load(args.id) entry = _round(state, args.round) if not args.dry_run: + _discard_impl_leftovers(state, state["worktrees"]["work"]) _resume_incomplete_apply(path, state, entry) # **叩き直しても同じ判定を返す。** 取り込み済みで再実行すると、前回作った @@ -1675,6 +1676,7 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: """Step 6 — 修正結果を取り込み、修正ラウンドを 1 つ進める。""" path, state = _load(args.id) entry = _round(state, args.round) + _discard_impl_leftovers(state, state["worktrees"]["work"]) _flush_pending_push(path, state, entry) impl = entry["impl"] result = _result_path(state, impl, stem_for(impl, "fix", state["id"], args.round)) @@ -2018,10 +2020,18 @@ def _reported_shas(reported: Any) -> list[str]: return shas -def _git_out(work: str, args: list[str]) -> Optional[str]: - """`git` を実行して標準出力を返す。失敗したら `None`。""" +def _git_out(work: str, args: list[str], strip: bool = True) -> Optional[str]: + """`git` を実行して標準出力を返す。失敗したら `None`。 + + **固定幅で読む出力には `strip=False` を渡す。** `git status --porcelain` の + 状態コードは未 stage の変更で ` M` と先頭が空白になるため、`strip()` すると + 1 行目だけ 1 文字ずれ、切り出したパスの先頭が欠ける。欠けたパスは + `git add` で `pathspec ... did not match any files` になり、同期が止まる。 + """ r = subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) - return r.stdout.strip() if r.returncode == 0 else None + if r.returncode != 0: + return None + return r.stdout.strip() if strip else r.stdout.rstrip("\n") def commits_in_range(work: str, base: Optional[str], head: str) -> Optional[list[str]]: @@ -2524,7 +2534,8 @@ def _worktree_changes(work: str) -> dict[str, str]: # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 out = _git_out( - work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"], + strip=False, ) changes: dict[str, str] = {} for line in (out or "").splitlines(): @@ -2579,6 +2590,33 @@ def _discard_worktree_changes(work: str) -> None: subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) +def _discard_impl_leftovers(state: dict[str, Any], work: str) -> None: + """実装担当が残した未コミットの変更を捨てる。取り込みの前に呼ぶ。 + + **公開は進行側が検証を通してから行う**ので、コミットされなかった変更は + どの検証も受けていない。Pull Request へ出す道が無い以上、残す意味がない。 + + 残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正 + フェーズでコミットを作れなかった実装担当が直しかけの差分を置いたまま終え、 + 続く `merge-fix` が「修正 0 件」として先へ進むこともできなくなった。 + + 制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、 + `git clean` に `-x` を付けないため消えない。 + """ + if not pathlib.Path(work).is_dir(): + return + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = "、".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + _discard_worktree_changes(work) + info( + f"🧹 コミットされなかった変更を捨てました（{shown}{more}）。" + "検証を受けていないため公開しません" + ) + + def _require_clean_worktree(state: dict[str, Any], work: str) -> None: """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 @@ -2643,8 +2681,17 @@ def _sync_generated(state: dict[str, Any]) -> None: produced = _dirty_paths(state, work) if not produced: return - _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + # **後段で落ちたときも差分を残さない。** `git add` / `git commit` の失敗で + # 作業ツリーを汚したまま中断すると、次の実行は `_require_clean_worktree` で + # 必ず止まり、`pending_push` の再試行が永久に進まない。捨ててよい根拠は + # 同期コマンド自身が失敗したときと同じで、着手前が綺麗だったことを + # 確認済みだからである。 + try: + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + except SystemExit: + _discard_worktree_changes(work) + raise info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") 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 41928945..5101513d 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 @@ -98,7 +98,7 @@ def test_deferred_entry_records_the_reason(refactor, tmp_path, env_tmp_dir, no_g def _history(refactor, monkeypatch, newest_first): """`git rev-list HEAD` の結果（新しい順）と SHA 解決を差し替える。""" - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:1] == ["rev-list"]: return "\n".join(newest_first) if args[:2] == ["rev-parse", "--verify"]: @@ -232,7 +232,7 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, # 未申告コミットの判定がこの解決を通るため。 monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr( @@ -406,7 +406,7 @@ def test_broken_fix_result_does_not_crash( }) monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: set()) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: []) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD") monkeypatch.setattr( refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: [], @@ -436,7 +436,7 @@ def test_merge_fix_uses_the_recorded_range(refactor, tmp_path, env_tmp_dir, monk ) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -452,7 +452,7 @@ def test_merge_fix_fails_when_the_range_cannot_be_determined( ): state_path = _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, ["PRRT_a"]) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: None) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD_NOW") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD_NOW") monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: {"PRRT_a"}) with pytest.raises(SystemExit) as e: @@ -470,7 +470,7 @@ def test_merge_fix_rejects_unreported_commits( refactor, "commits_in_range", lambda work, base, head: ["sneaky", "fix111"]) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" + lambda work, args, **_kw: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" else "HEAD_NOW", ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -691,7 +691,7 @@ def test_merge_fix_is_idempotent_after_a_revert( calls.clear() monkeypatch.setattr( refactor, "_git_out", - lambda work, args: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] + lambda work, args, **_kw: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] else args[-1].replace("^{commit}", "")), ) refactor.cmd_merge_fix(args) @@ -799,7 +799,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_judge_review.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_judge_review.py index 0105fa34..a3121ef2 100644 --- a/plugins/ndf-claude/skills/cross-refactoring/tests/test_judge_review.py +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_judge_review.py @@ -273,7 +273,7 @@ def test_judge_command_records_the_fix_base(refactor, tmp_path, env_tmp_dir, mon """修正コミットの実在を確かめるため、変更要求の時点の HEAD を残すこと。""" state_path = _state(tmp_path) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "FIX_BASE") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "FIX_BASE") write_result(state_path, "gemini-review-r1", review("REQUEST_CHANGES", [finding()])) write_result(state_path, "kiro-review-r1", review()) 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 28aa9cb7..743712c9 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 @@ -5,6 +5,7 @@ """ from __future__ import annotations +import pathlib import subprocess import pytest @@ -149,7 +150,7 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): """結果ファイルではなく実際のコミットメッセージから読む。""" monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "Item-Id: R1-001\nRound: 1\n" + lambda work, args, **_kw: "Item-Id: R1-001\nRound: 1\n" "Impl-Runtime: codex\nImpl-Model: gpt-5.5", ) assert refactor.commit_trailers("/w", "abc") == { @@ -159,31 +160,31 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): def test_commit_trailers_are_empty_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commit_trailers("/w", "abc") == {} def test_diff_lines_come_from_numstat(refactor, monkeypatch): monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", + lambda work, args, **_kw: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", ) assert refactor.commit_diff_lines("/w", "abc") == 20 def test_touches_tests_detects_test_paths(refactor, monkeypatch): monkeypatch.setattr(refactor, "_git_out", - lambda work, args: "src/a.py\ntests/test_a.py") + lambda work, args, **_kw: "src/a.py\ntests/test_a.py") assert refactor.commit_touches_tests("/w", "abc") is True - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "src/a.py") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "src/a.py") assert refactor.commit_touches_tests("/w", "abc") is False def test_commits_in_range_uses_rev_list(refactor, monkeypatch): calls = [] - def fake(work, args): + def fake(work, args, **_kw): calls.append(args) return "aaa\nbbb" @@ -198,7 +199,7 @@ def test_commits_in_range_is_none_without_base(refactor): def test_commits_in_range_is_none_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commits_in_range("/w", "base", "head") is None @@ -206,7 +207,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): """テストは実際に走らせる。実行後は必ず元のブランチへ戻す。""" git_calls = [] monkeypatch.setattr( - refactor, "_git_out", lambda work, args: git_calls.append(args) or "") + refactor, "_git_out", lambda work, args, **_kw: git_calls.append(args) or "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: (git_calls.append(a[0]) if isinstance(a[0], list) else None) @@ -220,7 +221,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): def test_run_test_at_reports_failure(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: subprocess.CompletedProcess(a[0], 0, "", ""), @@ -239,7 +240,7 @@ def fake_run(cmd, **kw): return subprocess.CompletedProcess(cmd, 0, "", "") raise OSError("テスト実行が壊れた") - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr(refactor.subprocess, "run", fake_run) with pytest.raises(OSError): refactor.run_test_at("/w", "abc", "pytest -q", "main") @@ -247,13 +248,13 @@ def fake_run(cmd, **kw): def test_collect_facts_marks_unknown_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) facts = refactor.collect_commit_facts("/w", ["ghost"], {"aaa"}, "true", "main") assert facts == [{"sha": "ghost", "exists": False}] def test_collect_facts_marks_out_of_range_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "zzz") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "zzz") facts = refactor.collect_commit_facts("/w", ["zzz"], {"aaa"}, "true", "main") assert facts[0]["exists"] is False @@ -289,7 +290,7 @@ def _set(mapping, in_range=None): # SHA をそのまま返す形にしておく monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", ""), + lambda work, args, **_kw: args[-1].replace("^{commit}", ""), ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -383,11 +384,21 @@ 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, sync_dirty=False): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False, + leftover=""): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 積み直しで SHA が変わることを、状態の更新まで含めて確かめられるようにする。 + + 作業ツリーの状態は 3 段階で返す。取り込みの前に実装担当の置き土産を捨てる + ため、同期の前後だけでは足りない。 + + | 呼ばれる場面 | 返す値 | + | --- | --- | + | 取り込みの前（置き土産の確認） | `leftover` | + | 同期の前（清浄性の検査） | `sync_dirty[0]` | + | 同期の後（生成された差分） | `sync_dirty[1]` | """ calls: list[list[str]] = [] picked: list[str] = [] @@ -407,7 +418,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: @@ -415,13 +426,13 @@ def fake_git_out(work, args): 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 len(statuses) == 1: + return leftover if sync_dirty is False: return "" before, after = sync_dirty - return before if len(statuses) == 1 else after + return before if len(statuses) == 2 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -842,7 +853,7 @@ def test_apply_base_is_recorded_by_the_orchestrator( "durations": {}, "reviews": [], }]) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "BASE_HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "BASE_HEAD") for rt in ("codex", "gemini", "kiro"): write_result(state_path, f"{rt}-propose-rf130", {"items": []}) with pytest.raises(SystemExit): @@ -1001,7 +1012,7 @@ def test_short_and_full_sha_are_seen_as_the_same_commit( # 短縮 SHA も完全 SHA も同じコミットへ解決される monkeypatch.setattr( refactor, "_git_out", - lambda work, args: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", + lambda work, args, **_kw: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -1256,9 +1267,16 @@ def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, g # ---------- push の直前に生成物を同期する ---------- def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + """同期コマンドを持つ状態を作る。 + + 書き込み用の作業ディレクトリを実在させる。取り込みの前に置き土産を確認する + 経路は、ディレクトリが無ければ何もせずに戻るため、実在しないと + `_drop_env` の 3 段階（置き土産 / 同期前 / 同期後）が 1 つずれる。 + """ state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) state = read_state(state_path) state["sync_command"] = command + pathlib.Path(state["worktrees"]["work"]).mkdir(parents=True, exist_ok=True) state_path.write_text(__import__("json").dumps(state), encoding="utf-8") return state_path @@ -1517,7 +1535,7 @@ def test_status_disables_path_quoting(refactor, monkeypatch): seen: list[list[str]] = [] monkeypatch.setattr( refactor, "_git_out", - lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + lambda work, args, **_kw: 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"] diff --git a/plugins/ndf-claude/skills/cross-refactoring/tests/test_sync_generated.py b/plugins/ndf-claude/skills/cross-refactoring/tests/test_sync_generated.py new file mode 100644 index 00000000..008259ac --- /dev/null +++ b/plugins/ndf-claude/skills/cross-refactoring/tests/test_sync_generated.py @@ -0,0 +1,223 @@ +"""生成物の同期と、実装担当が残した未コミット変更の扱いを**実際の git** で確かめる。 + +同期は `--sync-command` を持つリポジトリで push の直前に走る、進行側の責務である。 +ここが落ちると取り消しを Pull Request へ反映できないため、進行そのものが止まる。 + +| 確かめること | なぜ | +| --- | --- | +| 変更のパスを 1 文字も欠かさず拾う | `git status --porcelain` は固定幅。先頭の空白を削ると 1 行目がずれる | +| 同期の後段で落ちても差分を残さない | 残すと次の実行が清浄性の検査で必ず止まる | +| 実装担当の置き土産を捨ててから取り込む | 検証を受けていない変更なので公開しない。止まる理由にもしない | +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state, write_result + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + return _git("rev-parse", "HEAD", cwd=repo).stdout.strip() + + +def _make_work(tmp_path): + """`work` を本物のリポジトリとして作り、状態ファイルを添えて返す。""" + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _state_with_sync(tmp_path, work, command="true"): + return make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + sync_command=command, + ) + + +# ---------- 変更のパスを 1 文字も欠かさず拾う ---------- + +def test_unstaged_change_on_first_line_keeps_full_path(refactor, tmp_path): + """先頭が空白の状態コード（` M`）でも、パスの先頭文字が消えない。 + + `git status --porcelain` は「状態 2 文字 + 空白 + パス」の固定幅で、 + 未 stage の変更は 1 文字目が空白になる。出力全体を `strip()` してから + 固定幅で切り出すと、**1 行目だけ**パスが 1 文字短くなる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + + changes = refactor._worktree_changes(str(work)) + + assert "src.py" in changes + + +def test_every_changed_path_is_addable(refactor, tmp_path): + """拾ったパスは、そのまま `git add` に渡して通る。""" + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 2\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + paths = refactor._dirty_paths(state, str(work)) + + assert paths == ["generated/out.py", "src.py"] + _git("add", "--", *paths, cwd=work) + + +# ---------- 同期コミット ---------- + +def test_sync_commits_generated_changes(refactor, tmp_path): + """同期コマンドが作った差分は、進行側のコミットとして積まれる。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + assert subject == refactor.SYNC_COMMIT_MESSAGE.splitlines()[0] + + +def test_sync_without_changes_makes_no_commit(refactor, tmp_path): + """差分が出ない同期はコミットを作らない。""" + work = _make_work(tmp_path) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state = read_state(_state_with_sync(tmp_path, work, command="true")) + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +# ---------- 同期の後段で落ちたとき ---------- + +def test_failure_after_sync_discards_produced_changes(refactor, tmp_path, monkeypatch): + """`git add` / `git commit` が落ちても、同期が作った差分を残さない。 + + 残すと次の実行は清浄性の検査で必ず止まり、保留中の push を再試行できない。 + """ + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + monkeypatch.setattr(refactor, "_sh", + lambda *a, **k: refactor.die("commit に失敗しました")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_failed_sync_command_discards_partial_changes(refactor, tmp_path): + """同期コマンド自身が落ちたときも、途中まで書き換えた差分を残さない。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py; exit 1")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +# ---------- 実装担当が残した未コミット変更 ---------- + +def test_leftover_changes_are_discarded_before_merge(refactor, tmp_path): + """実装担当が残した未コミット変更は、取り込みの前に捨てる。 + + 公開は進行側が検証を通してから行うので、コミットされなかった変更は + **検証を受けていない**。残したまま進むと、清浄性の検査で進行が止まる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + refactor._discard_impl_leftovers(state, str(work)) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert (work / "src.py").read_text(encoding="utf-8") == "x = 1\n" + + +def test_discard_keeps_control_directory(refactor, tmp_path): + """制御用ディレクトリ（状態・結果・ログ）は捨てない。""" + work = _make_work(tmp_path) + control = work / ".cross_refactoring" + control.mkdir() + (control / "keep.json").write_text("{}", encoding="utf-8") + (work / ".gitignore").write_text(".cross_refactoring/\n", encoding="utf-8") + _commit(work, "ignore control dir") + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(make_state( + tmp_path, + worktrees={"work": str(work)}, + tmp_dir=str(control), + )) + + refactor._discard_impl_leftovers(state, str(work)) + + assert (control / "keep.json").exists() + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_merge_fix_continues_when_impl_left_changes( + refactor, tmp_path, env_tmp_dir, monkeypatch +): + """修正フェーズの置き土産があっても、`merge-fix` は中断しない。 + + 実装担当がコミットを作れずに終えると作業ツリーへ差分が残る。これを理由に + 止めると、修正 0 件として先へ進むこともできなくなる。 + """ + work = _make_work(tmp_path) + head = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state_path = make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + rounds=[{ + "round": 1, "impl": "codex", "impl_model": None, + "reviewers": ["gemini", "kiro"], "reviewer_models": {}, + "items": ["R1-001"], "adopted": 1, "proposed": 1, "merged": 1, + "apply": {"merged_at": "2026-08-18T00:00:00", "applied": ["R1-001"], + "failed": []}, + "apply_base_sha": head, "apply_progress": [], "drops": [], + "reviews": [], "fix_rounds": 0, "fix_attempts": 1, + "fix_base_sha": head, "deferred": [], "durations": {}, + "proposal_keys": [], "pending_drop": [], "pending_push": False, + "started_at": "2026-08-18T00:00:00", + }], + items=[{"item_id": "R1-001", "round": 1, "path": "src.py", + "symbol": "f", "smell": "long_method", "technique": "extract_method", + "severity": "major", "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 10, "proposed_by": ["codex"], + "status": "applied", "commits": []}], + ) + env_tmp_dir(state_path) + monkeypatch.setattr(refactor, "_push_head", lambda state: None) + write_result(state_path, "codex-fix-r1", + {"resolved_thread_ids": [], "unresolved": [], "commits": []}) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + + refactor.cmd_merge_fix(type("A", (), {"id": 130, "round": 1})()) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert read_state(state_path)["rounds"][0]["fix_rounds"] == 1 diff --git a/plugins/ndf-claude/skills/cross-review/SKILL.md b/plugins/ndf-claude/skills/cross-review/SKILL.md index f2eeb2f1..cc8b704f 100644 --- a/plugins/ndf-claude/skills/cross-review/SKILL.md +++ b/plugins/ndf-claude/skills/cross-review/SKILL.md @@ -41,6 +41,7 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲 | 観点 | 方針 | |---|---| | レビュー投稿 | **AI 自身が `gh api` で PR に直接投稿**。メインはペイロードを保持しない | +| 投稿の確認 | **申告されたコメント数を GitHub 側と突き合わせる**。投稿が届いていなければ中断する（取得できない場合は申告を採用） | | 修正 | **必ずサブエージェント (`general-purpose`) で実行**。メイン context に diff は載せない | | ユーザ問い合わせ | 自動判断を最大化（`critical`/`major`/`minor` は自動修正、ループ中の `nit` は deferred） | | 取りこぼし防止 | **ループ終了時（approved / max_rounds / oscillation / error いずれも）に最終スイープを必須実行**。`/ndf:fix` を再実行し、残った open review thread（最終 APPROVE ラウンドの minor/nit インラインコメント含む）を **全て解消**。修正可能なものは修正 + push、判断保留 nit も reply + resolveReviewThread して **open thread 0 で終了** | @@ -394,6 +395,8 @@ pint / larastan / test / build などは **中断** を原則とする。 - ❌ **修正をメインセッション内で行う** — context が一気に膨れる。必ずサブエージェント - ❌ **AI に Markdown だけ返させる** — メインがパース・投稿する設計は禁物。AI 直接投稿 +- ❌ **result.json の申告だけで判定を進める** — 投稿が失敗しても件数は残る。GitHub 側の + 実数と突き合わせないと、修正担当が読むべき指摘が存在しないまま収束する - ❌ **nit を都度ユーザに問う** — ループ中は deferred 記録のみ。最終スイープ (Step 7.5) で Resolve - ❌ **未解決スレッドを残したまま終了する** — approved/max_rounds 等いずれの終了経路でも Step 7.5 の最終スイープを必ず実行し、open review thread 0 で終える。特に **最終 APPROVE diff --git a/plugins/ndf-claude/skills/cross-review/docs/01-state-and-review.md b/plugins/ndf-claude/skills/cross-review/docs/01-state-and-review.md index a1e088fd..6e782731 100644 --- a/plugins/ndf-claude/skills/cross-review/docs/01-state-and-review.md +++ b/plugins/ndf-claude/skills/cross-review/docs/01-state-and-review.md @@ -216,6 +216,25 @@ launcher が生成するプロンプトに以下を強制している: `state.rounds[-1].` に `intent / posted_as / comments / review_url / by_severity` を分離保存する。 +#### 申告されたコメント数を GitHub 側と突き合わせる + +投稿は **AI 自身が `gh api` で行う**ため、失敗しても結果ファイルの申告だけは残る。 +申告のまま進むと、修正担当が読むべき指摘が GitHub 上に存在しないまま収束判定まで走る。 +実測では、2 件の申告に対しスレッドが 1 つも作られていなかった。 + +`read-result` は申告が 1 件以上のとき、`review_url` の識別子から +`repos//pulls//reviews//comments` を数えて突き合わせる。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 突き合わせる相手がいない | +| n 件 | n 件以上 | 採用する。人の追記など申告以外の経路で増えうる | +| n 件 | n 件未満 | **中断する。** 投稿が届いていない | +| n 件 | 取得できない | 申告を採用し、確認できなかったことを出力へ残す | + +**「取得できなかった」と「0 件」を区別する。** 取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 + ## Step 3: 判定（intent ベース） ```bash diff --git a/plugins/ndf-claude/skills/cross-review/scripts/state.py b/plugins/ndf-claude/skills/cross-review/scripts/state.py index 9e91f11f..c001d2aa 100755 --- a/plugins/ndf-claude/skills/cross-review/scripts/state.py +++ b/plugins/ndf-claude/skills/cross-review/scripts/state.py @@ -29,6 +29,7 @@ import json import os import pathlib +import re import shlex import subprocess import sys @@ -967,6 +968,44 @@ def cmd_start_round(args: argparse.Namespace) -> None: print(f"ROTATE_AFTER={st['rotate_after']}") +def _as_count(value: object) -> int: + """申告された件数を整数として読む。読めない値は 0 として扱う。 + + 相手は LLM なので、文字列や `null` が入ることがある。読めない申告を + 「件数あり」と見なすと、突き合わせる相手が決まらないまま中断してしまう。 + """ + try: + return max(0, int(value)) # type: ignore[arg-type] + except (TypeError, ValueError): + return 0 + + +def _posted_comment_count(repo: str, pr: int, review_url: str | None) -> int | None: + """レビューに実際にぶら下がっているインラインコメントの数。 + + 取得できなければ `None` を返す。**「取得できなかった」と「0 件」を区別する。** + 取得の失敗で中断すると、GitHub 側の一時的な不調でループが止まる。 + + 投稿は AI 自身が `gh api` で行うため、失敗しても結果ファイルの申告だけは残る。 + 数え直す先は、申告された `review_url` の末尾にある識別子から決める。 + """ + if not repo or not review_url: + return None + m = re.search(r"pullrequestreview-(\d+)", str(review_url)) + if not m: + return None + try: + out = _sh( + ["gh", "api", f"repos/{repo}/pulls/{pr}/reviews/{m.group(1)}/comments", + "--paginate", "--jq", "length"], + check=False, + ) + except Exception: + return None + counts = [int(line) for line in str(out).split() if line.strip().isdigit()] + return sum(counts) if counts else None + + def cmd_read_result(args: argparse.Namespace) -> None: """Step 2.5 — codex/gemini の result.json を state にマージ。""" agent = args.agent @@ -1007,6 +1046,25 @@ def cmd_read_result(args: argparse.Namespace) -> None: st = _load(pr) if not st.get("rounds"): die(f"{agent}: state.rounds が空。`state.py start-round` を先に呼んでください") + + # **申告を GitHub 側と突き合わせる。** 投稿は AI 自身が行うので、失敗しても + # 結果ファイルには件数が残る。申告のまま進むと、修正担当が読むべき指摘が + # GitHub 上に存在しないまま収束判定まで走る（実測: 申告 2 件に対しスレッド 0）。 + declared = _as_count(comments) + if declared > 0: + actual = _posted_comment_count(str(st.get("repo") or ""), pr, r.get("review_url")) + if actual is None: + info( + f"⚠ {agent}: 投稿されたコメント数を確認できませんでした。" + f"申告（{declared} 件）をそのまま採用します" + ) + elif actual < declared: + die( + f"{agent}: インラインコメントの申告 {declared} 件に対し、" + f"GitHub 上には {actual} 件しかありません。投稿が届いていないため" + "中断します。レビューを投稿し直してから再実行してください" + ) + st["rounds"][-1][agent] = { "intent": intent, "posted_as": posted_as, diff --git a/plugins/ndf-claude/skills/cross-review/tests/test_state_posted_comments.py b/plugins/ndf-claude/skills/cross-review/tests/test_state_posted_comments.py new file mode 100644 index 00000000..b57ffc95 --- /dev/null +++ b/plugins/ndf-claude/skills/cross-review/tests/test_state_posted_comments.py @@ -0,0 +1,191 @@ +"""申告されたインラインコメント数を、GitHub 側の実数と突き合わせる。 + +レビューの投稿は AI 自身が `gh api` で行うため、**投稿に失敗しても結果ファイルの +申告だけは残る**。申告を信じて先へ進むと、修正担当が読むべき指摘が GitHub 上に +存在しないまま収束判定まで走る。実測では 2 件の申告に対しスレッドが 1 つも +作られていなかった。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 投稿が無いので突き合わせる相手がいない | +| 2 件 | 2 件 | そのまま採用する | +| 2 件 | 0 件 | 投稿が届いていないので中断する | +| 2 件 | 取得できない | 申告を採用し、確認できなかったことを残す | + +「取得できなかった」と「0 件」を混同しない。取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + +PR = 4242 +AGENT = "gemini" +REVIEW_URL = f"https://github.com/o/r/pull/{PR}#pullrequestreview-4961230016" + + +def _seed_state(tmp_dir: pathlib.Path) -> None: + state = { + "current_pr": PR, + "repo": "o/r", + "rounds": [{"round": 1, "pr": PR, "started_at": "2026-08-18T00:00:00+00:00"}], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _result(tmp_dir: pathlib.Path, **over) -> pathlib.Path: + payload = { + "event": "REQUEST_CHANGES", + "posted_as": "REQUEST_CHANGES", + "comments_count": 2, + "review_url": REVIEW_URL, + "by_severity": {"major": 2}, + } + payload.update(over) + rfile = tmp_dir / "result.json" + rfile.write_text(json.dumps(payload)) + return rfile + + +def _args(rfile: pathlib.Path) -> argparse.Namespace: + return argparse.Namespace(pr=PR, agent=AGENT, file=str(rfile)) + + +def _read_state(tmp_dir: pathlib.Path) -> dict: + return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text()) + + +@pytest.fixture() +def tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +@pytest.fixture() +def posted(monkeypatch, state_mod): + """GitHub 側の件数を差し替える。`None` は取得できなかったことを表す。""" + def _set(count): + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda repo, pr, review_url: count, + ) + return _set + + +def test_declared_count_matching_github_is_accepted(tmp_dir, state_mod, posted): + _seed_state(tmp_dir) + posted(2) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_declared_comments_missing_on_github_aborts(tmp_dir, state_mod, posted): + """申告があるのに GitHub 側へ届いていなければ中断する。 + + そのまま進むと、修正担当が読むべき指摘が存在しないまま収束判定まで走る。 + """ + _seed_state(tmp_dir) + posted(0) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert e.value.code == 1 + assert AGENT not in _read_state(tmp_dir)["rounds"][-1] + + +def test_partially_posted_comments_abort(tmp_dir, state_mod, posted): + """一部しか届いていない場合も中断する。取りこぼしは全件欠落と同じ扱いにする。""" + _seed_state(tmp_dir) + posted(1) + + with pytest.raises(SystemExit): + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + +def test_more_comments_on_github_is_accepted(tmp_dir, state_mod, posted): + """GitHub 側が多い分には通す。人の追記など、申告以外の経路で増えうる。""" + _seed_state(tmp_dir) + posted(3) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_zero_declared_skips_the_check(tmp_dir, state_mod, monkeypatch): + """申告 0 件なら GitHub を見に行かない。""" + _seed_state(tmp_dir) + called: list = [] + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda *a, **k: called.append(a) or 0, + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, event="APPROVE", comments_count=0))) + + assert called == [] + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 0 + + +def test_unavailable_github_count_keeps_the_declaration(tmp_dir, state_mod, posted): + """GitHub 側を取得できなければ申告を採用する。取得失敗で止めない。""" + _seed_state(tmp_dir) + posted(None) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_missing_review_url_is_treated_as_unavailable(tmp_dir, state_mod, monkeypatch): + """投稿先の参照が無ければ、突き合わせる相手を決められないので申告を採用する。""" + _seed_state(tmp_dir) + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("参照が無いのに GitHub を呼んでいる"), + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, review_url=None))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +# ---------------- 件数の取得 ---------------- + +def test_posted_count_reads_the_review_id_from_the_url(state_mod, monkeypatch): + calls: list[list[str]] = [] + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: calls.append(list(cmd)) or "2", + ) + + count = state_mod._posted_comment_count("o/r", PR, REVIEW_URL) + + assert count == 2 + assert calls and "repos/o/r/pulls/4242/reviews/4961230016/comments" in calls[0] + + +def test_posted_count_is_none_when_the_url_has_no_review_id(state_mod, monkeypatch): + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("識別子が無いのに GitHub を呼んでいる"), + ) + + assert state_mod._posted_comment_count("o/r", PR, "https://example.test/") is None + + +def test_posted_count_is_none_when_the_api_fails(state_mod, monkeypatch): + def boom(cmd, check=True): + raise RuntimeError("network") + + monkeypatch.setattr(state_mod, "_sh", boom) + + assert state_mod._posted_comment_count("o/r", PR, REVIEW_URL) is None diff --git a/plugins/ndf-codex/.codex-plugin/plugin.json b/plugins/ndf-codex/.codex-plugin/plugin.json index e9832e9a..52bc7084 100644 --- a/plugins/ndf-codex/.codex-plugin/plugin.json +++ b/plugins/ndf-codex/.codex-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ndf", - "version": "8.4.0", - "description": "Codex plugin (v8.4.0): 25 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, external AI delegation (Codex/Gemini), and optional Slack completion notifications.", + "version": "8.5.0", + "description": "Codex plugin (v8.5.0): 25 focused NDF skills for PR/review workflows, cross-review, implementation planning, plan-to-spec, Docker container access, external AI delegation (Codex/Gemini), and optional Slack completion notifications.", "skills": "./skills/", "hooks": "./hooks/hooks.json" } diff --git a/plugins/ndf-codex/README.md b/plugins/ndf-codex/README.md index 8c235dec..67126456 100644 --- a/plugins/ndf-codex/README.md +++ b/plugins/ndf-codex/README.md @@ -47,7 +47,7 @@ Claude Code 専用の agents、statusline 自動設定、transcript retention ```text # 動く: 実体パスを示して読ませる -~/.codex/plugins/cache/ai-plugins/ndf/8.4.0/skills/deploy/SKILL.md を読んで、その手順どおりに qa/staging へ deploy PR を作成してください。 +~/.codex/plugins/cache/ai-plugins/ndf/8.5.0/skills/deploy/SKILL.md を読んで、その手順どおりに qa/staging へ deploy PR を作成してください。 # 動かない: 明示起動 ($ は展開されない) $deploy qa/staging @@ -69,14 +69,14 @@ marketplace 経由でインストールした場合、Skill の実体は **ワ ```text $CODEX_HOME/plugins/cache////skills//SKILL.md # 既定 ($CODEX_HOME=~/.codex) の例: -# ~/.codex/plugins/cache/ai-plugins/ndf/8.4.0/skills/deploy/SKILL.md +# ~/.codex/plugins/cache/ai-plugins/ndf/8.5.0/skills/deploy/SKILL.md ``` そのため「`deploy` の SKILL.md を探して読んで」のような曖昧な依頼は、Codex のファイル探索がワークスペース内に限られる状況では失敗しえます。**抑止した Skill は `$` が展開されない**ので、`codex plugin list` で実体パスを確認し、絶対パスを渡してください。 ```bash codex plugin list | grep 'ndf@ai-plugins' -# => ndf@ai-plugins installed, enabled 8.4.0 +# => ndf@ai-plugins installed, enabled 8.5.0 ``` 抑止していない Skill（`markdown-writing` など）はキャッシュ配下でも `$` で解決するため、そちらは `$` 起動が使えます。 diff --git a/plugins/ndf-codex/skills/cross-refactoring/SKILL.md b/plugins/ndf-codex/skills/cross-refactoring/SKILL.md index 4ca3d399..68efacbd 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-codex/skills/cross-refactoring/SKILL.md @@ -193,11 +193,17 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" while :; do # 提案ラウンドの繰り返し rf_eval start-round "$ID" || break # 終了コード 1 = 繰り返し終了 + # **提案の直前に読み取り用を同期する。** 前ラウンドの取り消しで HEAD が進んで + # いるため、同期しないと**消えたコードに対する提案**が返る（実測: 取り消しで + # 消えた関数へ 2 件）。HEAD が変わっていなければ何も起きない。 + "$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done + # 提案の所要は参加ランタイムと回線状況で振れる（実測 90〜285 秒）。既定の + # 打ち切りに任せず、明示する。 "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 rf merge-proposals "$ID" || break # 終了コード 2 = 採用 0 件 "$SCRIPTS/launch-cli.sh" "$IMPL" apply "$ID" "$ROUND" @@ -213,7 +219,7 @@ while :; do # 提案ラウンドの繰り返 "$SCRIPTS/launch-cli.sh" "$r" review "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$REVIEWERS_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-review-r$ROUND" + --stem-template "{agent}-review-r$ROUND" --timeout 900 rf judge-review "$ID" "$ROUND"; rc=$? [ $rc -eq 0 ] && break # 2 者とも承認 [ $rc -eq 3 ] && continue # 形式不正 — 差し戻して再レビュー @@ -271,6 +277,7 @@ done | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | | `git push --force` / `--no-verify` を使う | 他者の作業を消す。検証を飛ばす | +| 実装担当のコミットでフックの通し方を決めない | 生成物の同期を検査するリポジトリでは、同期の禁止と両立せずコミットを作れなくなる。迂回してよい手段を 1 つ定める | | 提案とレビューにホストを混ぜる | 実装者と評価者が同一モデルになりうる。初期化時に検査して失敗させている | | kiro を既定モデルのまま計測する | `auto` は実際に選ばれたモデルを取得できない | | 提案フェーズでコードを直す | 提案は読むだけ。直すのは実装担当 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 45207583..fb751789 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 @@ -182,11 +182,12 @@ claude 17 本 / codex 1 メソッド / kiro 0 本と揃わなかった。最後 ## Step 2: 提案 ```bash +"$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 ``` 3 CLI を並列で起動し、同一のプロンプトで提案させる。**提案フェーズにホストは現れない** @@ -195,6 +196,24 @@ done 提出形式は [prompts/propose.md](../prompts/propose.md) にある。 +### 提案の直前に読み取り用を同期する + +**同期が要るのは HEAD が進んだときであって、特定のフェーズの後ではない。** +適用と修正の直後だけを同期していると、取り消しで進んだ HEAD が読み取り用へ +届かない。実測では、ラウンドを全件取り消した次の提案で、**取り消しによって +消えた関数**に対する提案が 2 件返った。統合は対象の実在を検査しないため、 +そのまま採用され、適用で必ず失敗する。 + +提案の直前に同期しておけば、どのフェーズを経ていても読み取り用は最新になる。 +HEAD が変わっていなければ何も起きないので、重ねて呼んでも無駄がない。 + +### 打ち切りまでの時間を明示する + +提案の所要はランタイムと回線状況で振れる（実測 90〜285 秒）。既定の打ち切りに +任せると、分析そのものは進んでいるのに時間切れで結果を捨てることがある。 +結果ファイルには `idle_seconds` が残るので、**止まっていたのか間に合わなかったのか**は +後から読める。 + ### 結果ファイル名にラウンド番号を入れる CLI の起動時に同名の結果ファイルを消すため、**提案の結果ファイル名にもラウンド番号が 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 4609b02a..c5d18be3 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 @@ -199,6 +199,32 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 終わると、Pull Request 側には未検証の差分が残るのに、次の実行は処理済みガードで 素通りしてしまう。印があれば、次の実行が判定より先に再送信する。 +### 実装担当のコミットはフックの検査を通さなくてよい + +**生成物の同期を pre-commit で検査するリポジトリでは、そのままだとコミットを作れない。** +実装担当は範囲内だけを変更するので生成物は必ず古くなり、同期は禁じられている。 + +公開は進行側が検証を通してから行うため、**実装担当のコミット時点で生成物が古いのは +設計どおり**である。そこで、フックが原因でコミットできないときは +`git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外させる。 + +- 禁止は `--no-verify` だけを名指しにしない。手段の名前で書き分けると、同じことが + 別の名前で起こる。実測では、同じ実装担当が適用フェーズではフックを迂回し、 + 修正フェーズでは迂回せず 0 コミットで終えた +- **直した内容は必ずコミットさせる。** 作業ツリーに置いたまま終えると、検証を + 受けていない変更として `merge-apply` / `merge-fix` が捨てる + +### 取り込みの前に置き土産を捨てる + +`merge-apply` と `merge-fix` は、実装担当が残した未コミットの変更を捨ててから +結果を読む。コミットされなかった変更はどの検証も受けておらず、公開する道が無い。 + +残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正フェーズで +コミットを作れなかった実装担当が直しかけの差分を置いたまま終え、続く `merge-fix` が +「修正 0 件」として先へ進むこともできなくなった。 + +制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、消えない。 + ### コミットトレーラーの形式 適用と修正のコミットメッセージ本文の末尾に、**git のトレーラー形式**で実行主体を残す。 diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md index bfaf491e..ec9c8b94 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/apply.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます diff --git a/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md index a73f3c3e..bb2180a9 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-codex/skills/cross-refactoring/prompts/fix.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$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 1a224c66..2aa3c0d2 100755 --- a/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-codex/skills/cross-refactoring/scripts/refactor.py @@ -1099,6 +1099,7 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: path, state = _load(args.id) entry = _round(state, args.round) if not args.dry_run: + _discard_impl_leftovers(state, state["worktrees"]["work"]) _resume_incomplete_apply(path, state, entry) # **叩き直しても同じ判定を返す。** 取り込み済みで再実行すると、前回作った @@ -1675,6 +1676,7 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: """Step 6 — 修正結果を取り込み、修正ラウンドを 1 つ進める。""" path, state = _load(args.id) entry = _round(state, args.round) + _discard_impl_leftovers(state, state["worktrees"]["work"]) _flush_pending_push(path, state, entry) impl = entry["impl"] result = _result_path(state, impl, stem_for(impl, "fix", state["id"], args.round)) @@ -2018,10 +2020,18 @@ def _reported_shas(reported: Any) -> list[str]: return shas -def _git_out(work: str, args: list[str]) -> Optional[str]: - """`git` を実行して標準出力を返す。失敗したら `None`。""" +def _git_out(work: str, args: list[str], strip: bool = True) -> Optional[str]: + """`git` を実行して標準出力を返す。失敗したら `None`。 + + **固定幅で読む出力には `strip=False` を渡す。** `git status --porcelain` の + 状態コードは未 stage の変更で ` M` と先頭が空白になるため、`strip()` すると + 1 行目だけ 1 文字ずれ、切り出したパスの先頭が欠ける。欠けたパスは + `git add` で `pathspec ... did not match any files` になり、同期が止まる。 + """ r = subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) - return r.stdout.strip() if r.returncode == 0 else None + if r.returncode != 0: + return None + return r.stdout.strip() if strip else r.stdout.rstrip("\n") def commits_in_range(work: str, base: Optional[str], head: str) -> Optional[list[str]]: @@ -2524,7 +2534,8 @@ def _worktree_changes(work: str) -> dict[str, str]: # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 out = _git_out( - work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"], + strip=False, ) changes: dict[str, str] = {} for line in (out or "").splitlines(): @@ -2579,6 +2590,33 @@ def _discard_worktree_changes(work: str) -> None: subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) +def _discard_impl_leftovers(state: dict[str, Any], work: str) -> None: + """実装担当が残した未コミットの変更を捨てる。取り込みの前に呼ぶ。 + + **公開は進行側が検証を通してから行う**ので、コミットされなかった変更は + どの検証も受けていない。Pull Request へ出す道が無い以上、残す意味がない。 + + 残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正 + フェーズでコミットを作れなかった実装担当が直しかけの差分を置いたまま終え、 + 続く `merge-fix` が「修正 0 件」として先へ進むこともできなくなった。 + + 制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、 + `git clean` に `-x` を付けないため消えない。 + """ + if not pathlib.Path(work).is_dir(): + return + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = "、".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + _discard_worktree_changes(work) + info( + f"🧹 コミットされなかった変更を捨てました（{shown}{more}）。" + "検証を受けていないため公開しません" + ) + + def _require_clean_worktree(state: dict[str, Any], work: str) -> None: """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 @@ -2643,8 +2681,17 @@ def _sync_generated(state: dict[str, Any]) -> None: produced = _dirty_paths(state, work) if not produced: return - _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + # **後段で落ちたときも差分を残さない。** `git add` / `git commit` の失敗で + # 作業ツリーを汚したまま中断すると、次の実行は `_require_clean_worktree` で + # 必ず止まり、`pending_push` の再試行が永久に進まない。捨ててよい根拠は + # 同期コマンド自身が失敗したときと同じで、着手前が綺麗だったことを + # 確認済みだからである。 + try: + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + except SystemExit: + _discard_worktree_changes(work) + raise info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") 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 41928945..5101513d 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 @@ -98,7 +98,7 @@ def test_deferred_entry_records_the_reason(refactor, tmp_path, env_tmp_dir, no_g def _history(refactor, monkeypatch, newest_first): """`git rev-list HEAD` の結果（新しい順）と SHA 解決を差し替える。""" - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:1] == ["rev-list"]: return "\n".join(newest_first) if args[:2] == ["rev-parse", "--verify"]: @@ -232,7 +232,7 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, # 未申告コミットの判定がこの解決を通るため。 monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr( @@ -406,7 +406,7 @@ def test_broken_fix_result_does_not_crash( }) monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: set()) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: []) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD") monkeypatch.setattr( refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: [], @@ -436,7 +436,7 @@ def test_merge_fix_uses_the_recorded_range(refactor, tmp_path, env_tmp_dir, monk ) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -452,7 +452,7 @@ def test_merge_fix_fails_when_the_range_cannot_be_determined( ): state_path = _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, ["PRRT_a"]) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: None) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD_NOW") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD_NOW") monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: {"PRRT_a"}) with pytest.raises(SystemExit) as e: @@ -470,7 +470,7 @@ def test_merge_fix_rejects_unreported_commits( refactor, "commits_in_range", lambda work, base, head: ["sneaky", "fix111"]) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" + lambda work, args, **_kw: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" else "HEAD_NOW", ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -691,7 +691,7 @@ def test_merge_fix_is_idempotent_after_a_revert( calls.clear() monkeypatch.setattr( refactor, "_git_out", - lambda work, args: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] + lambda work, args, **_kw: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] else args[-1].replace("^{commit}", "")), ) refactor.cmd_merge_fix(args) @@ -799,7 +799,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_judge_review.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_judge_review.py index 0105fa34..a3121ef2 100644 --- a/plugins/ndf-codex/skills/cross-refactoring/tests/test_judge_review.py +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_judge_review.py @@ -273,7 +273,7 @@ def test_judge_command_records_the_fix_base(refactor, tmp_path, env_tmp_dir, mon """修正コミットの実在を確かめるため、変更要求の時点の HEAD を残すこと。""" state_path = _state(tmp_path) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "FIX_BASE") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "FIX_BASE") write_result(state_path, "gemini-review-r1", review("REQUEST_CHANGES", [finding()])) write_result(state_path, "kiro-review-r1", review()) 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 28aa9cb7..743712c9 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 @@ -5,6 +5,7 @@ """ from __future__ import annotations +import pathlib import subprocess import pytest @@ -149,7 +150,7 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): """結果ファイルではなく実際のコミットメッセージから読む。""" monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "Item-Id: R1-001\nRound: 1\n" + lambda work, args, **_kw: "Item-Id: R1-001\nRound: 1\n" "Impl-Runtime: codex\nImpl-Model: gpt-5.5", ) assert refactor.commit_trailers("/w", "abc") == { @@ -159,31 +160,31 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): def test_commit_trailers_are_empty_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commit_trailers("/w", "abc") == {} def test_diff_lines_come_from_numstat(refactor, monkeypatch): monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", + lambda work, args, **_kw: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", ) assert refactor.commit_diff_lines("/w", "abc") == 20 def test_touches_tests_detects_test_paths(refactor, monkeypatch): monkeypatch.setattr(refactor, "_git_out", - lambda work, args: "src/a.py\ntests/test_a.py") + lambda work, args, **_kw: "src/a.py\ntests/test_a.py") assert refactor.commit_touches_tests("/w", "abc") is True - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "src/a.py") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "src/a.py") assert refactor.commit_touches_tests("/w", "abc") is False def test_commits_in_range_uses_rev_list(refactor, monkeypatch): calls = [] - def fake(work, args): + def fake(work, args, **_kw): calls.append(args) return "aaa\nbbb" @@ -198,7 +199,7 @@ def test_commits_in_range_is_none_without_base(refactor): def test_commits_in_range_is_none_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commits_in_range("/w", "base", "head") is None @@ -206,7 +207,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): """テストは実際に走らせる。実行後は必ず元のブランチへ戻す。""" git_calls = [] monkeypatch.setattr( - refactor, "_git_out", lambda work, args: git_calls.append(args) or "") + refactor, "_git_out", lambda work, args, **_kw: git_calls.append(args) or "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: (git_calls.append(a[0]) if isinstance(a[0], list) else None) @@ -220,7 +221,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): def test_run_test_at_reports_failure(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: subprocess.CompletedProcess(a[0], 0, "", ""), @@ -239,7 +240,7 @@ def fake_run(cmd, **kw): return subprocess.CompletedProcess(cmd, 0, "", "") raise OSError("テスト実行が壊れた") - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr(refactor.subprocess, "run", fake_run) with pytest.raises(OSError): refactor.run_test_at("/w", "abc", "pytest -q", "main") @@ -247,13 +248,13 @@ def fake_run(cmd, **kw): def test_collect_facts_marks_unknown_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) facts = refactor.collect_commit_facts("/w", ["ghost"], {"aaa"}, "true", "main") assert facts == [{"sha": "ghost", "exists": False}] def test_collect_facts_marks_out_of_range_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "zzz") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "zzz") facts = refactor.collect_commit_facts("/w", ["zzz"], {"aaa"}, "true", "main") assert facts[0]["exists"] is False @@ -289,7 +290,7 @@ def _set(mapping, in_range=None): # SHA をそのまま返す形にしておく monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", ""), + lambda work, args, **_kw: args[-1].replace("^{commit}", ""), ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -383,11 +384,21 @@ 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, sync_dirty=False): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False, + leftover=""): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 積み直しで SHA が変わることを、状態の更新まで含めて確かめられるようにする。 + + 作業ツリーの状態は 3 段階で返す。取り込みの前に実装担当の置き土産を捨てる + ため、同期の前後だけでは足りない。 + + | 呼ばれる場面 | 返す値 | + | --- | --- | + | 取り込みの前（置き土産の確認） | `leftover` | + | 同期の前（清浄性の検査） | `sync_dirty[0]` | + | 同期の後（生成された差分） | `sync_dirty[1]` | """ calls: list[list[str]] = [] picked: list[str] = [] @@ -407,7 +418,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: @@ -415,13 +426,13 @@ def fake_git_out(work, args): 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 len(statuses) == 1: + return leftover if sync_dirty is False: return "" before, after = sync_dirty - return before if len(statuses) == 1 else after + return before if len(statuses) == 2 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -842,7 +853,7 @@ def test_apply_base_is_recorded_by_the_orchestrator( "durations": {}, "reviews": [], }]) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "BASE_HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "BASE_HEAD") for rt in ("codex", "gemini", "kiro"): write_result(state_path, f"{rt}-propose-rf130", {"items": []}) with pytest.raises(SystemExit): @@ -1001,7 +1012,7 @@ def test_short_and_full_sha_are_seen_as_the_same_commit( # 短縮 SHA も完全 SHA も同じコミットへ解決される monkeypatch.setattr( refactor, "_git_out", - lambda work, args: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", + lambda work, args, **_kw: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -1256,9 +1267,16 @@ def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, g # ---------- push の直前に生成物を同期する ---------- def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + """同期コマンドを持つ状態を作る。 + + 書き込み用の作業ディレクトリを実在させる。取り込みの前に置き土産を確認する + 経路は、ディレクトリが無ければ何もせずに戻るため、実在しないと + `_drop_env` の 3 段階（置き土産 / 同期前 / 同期後）が 1 つずれる。 + """ state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) state = read_state(state_path) state["sync_command"] = command + pathlib.Path(state["worktrees"]["work"]).mkdir(parents=True, exist_ok=True) state_path.write_text(__import__("json").dumps(state), encoding="utf-8") return state_path @@ -1517,7 +1535,7 @@ def test_status_disables_path_quoting(refactor, monkeypatch): seen: list[list[str]] = [] monkeypatch.setattr( refactor, "_git_out", - lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + lambda work, args, **_kw: 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"] diff --git a/plugins/ndf-codex/skills/cross-refactoring/tests/test_sync_generated.py b/plugins/ndf-codex/skills/cross-refactoring/tests/test_sync_generated.py new file mode 100644 index 00000000..008259ac --- /dev/null +++ b/plugins/ndf-codex/skills/cross-refactoring/tests/test_sync_generated.py @@ -0,0 +1,223 @@ +"""生成物の同期と、実装担当が残した未コミット変更の扱いを**実際の git** で確かめる。 + +同期は `--sync-command` を持つリポジトリで push の直前に走る、進行側の責務である。 +ここが落ちると取り消しを Pull Request へ反映できないため、進行そのものが止まる。 + +| 確かめること | なぜ | +| --- | --- | +| 変更のパスを 1 文字も欠かさず拾う | `git status --porcelain` は固定幅。先頭の空白を削ると 1 行目がずれる | +| 同期の後段で落ちても差分を残さない | 残すと次の実行が清浄性の検査で必ず止まる | +| 実装担当の置き土産を捨ててから取り込む | 検証を受けていない変更なので公開しない。止まる理由にもしない | +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state, write_result + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + return _git("rev-parse", "HEAD", cwd=repo).stdout.strip() + + +def _make_work(tmp_path): + """`work` を本物のリポジトリとして作り、状態ファイルを添えて返す。""" + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _state_with_sync(tmp_path, work, command="true"): + return make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + sync_command=command, + ) + + +# ---------- 変更のパスを 1 文字も欠かさず拾う ---------- + +def test_unstaged_change_on_first_line_keeps_full_path(refactor, tmp_path): + """先頭が空白の状態コード（` M`）でも、パスの先頭文字が消えない。 + + `git status --porcelain` は「状態 2 文字 + 空白 + パス」の固定幅で、 + 未 stage の変更は 1 文字目が空白になる。出力全体を `strip()` してから + 固定幅で切り出すと、**1 行目だけ**パスが 1 文字短くなる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + + changes = refactor._worktree_changes(str(work)) + + assert "src.py" in changes + + +def test_every_changed_path_is_addable(refactor, tmp_path): + """拾ったパスは、そのまま `git add` に渡して通る。""" + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 2\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + paths = refactor._dirty_paths(state, str(work)) + + assert paths == ["generated/out.py", "src.py"] + _git("add", "--", *paths, cwd=work) + + +# ---------- 同期コミット ---------- + +def test_sync_commits_generated_changes(refactor, tmp_path): + """同期コマンドが作った差分は、進行側のコミットとして積まれる。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + assert subject == refactor.SYNC_COMMIT_MESSAGE.splitlines()[0] + + +def test_sync_without_changes_makes_no_commit(refactor, tmp_path): + """差分が出ない同期はコミットを作らない。""" + work = _make_work(tmp_path) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state = read_state(_state_with_sync(tmp_path, work, command="true")) + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +# ---------- 同期の後段で落ちたとき ---------- + +def test_failure_after_sync_discards_produced_changes(refactor, tmp_path, monkeypatch): + """`git add` / `git commit` が落ちても、同期が作った差分を残さない。 + + 残すと次の実行は清浄性の検査で必ず止まり、保留中の push を再試行できない。 + """ + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + monkeypatch.setattr(refactor, "_sh", + lambda *a, **k: refactor.die("commit に失敗しました")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_failed_sync_command_discards_partial_changes(refactor, tmp_path): + """同期コマンド自身が落ちたときも、途中まで書き換えた差分を残さない。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py; exit 1")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +# ---------- 実装担当が残した未コミット変更 ---------- + +def test_leftover_changes_are_discarded_before_merge(refactor, tmp_path): + """実装担当が残した未コミット変更は、取り込みの前に捨てる。 + + 公開は進行側が検証を通してから行うので、コミットされなかった変更は + **検証を受けていない**。残したまま進むと、清浄性の検査で進行が止まる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + refactor._discard_impl_leftovers(state, str(work)) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert (work / "src.py").read_text(encoding="utf-8") == "x = 1\n" + + +def test_discard_keeps_control_directory(refactor, tmp_path): + """制御用ディレクトリ（状態・結果・ログ）は捨てない。""" + work = _make_work(tmp_path) + control = work / ".cross_refactoring" + control.mkdir() + (control / "keep.json").write_text("{}", encoding="utf-8") + (work / ".gitignore").write_text(".cross_refactoring/\n", encoding="utf-8") + _commit(work, "ignore control dir") + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(make_state( + tmp_path, + worktrees={"work": str(work)}, + tmp_dir=str(control), + )) + + refactor._discard_impl_leftovers(state, str(work)) + + assert (control / "keep.json").exists() + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_merge_fix_continues_when_impl_left_changes( + refactor, tmp_path, env_tmp_dir, monkeypatch +): + """修正フェーズの置き土産があっても、`merge-fix` は中断しない。 + + 実装担当がコミットを作れずに終えると作業ツリーへ差分が残る。これを理由に + 止めると、修正 0 件として先へ進むこともできなくなる。 + """ + work = _make_work(tmp_path) + head = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state_path = make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + rounds=[{ + "round": 1, "impl": "codex", "impl_model": None, + "reviewers": ["gemini", "kiro"], "reviewer_models": {}, + "items": ["R1-001"], "adopted": 1, "proposed": 1, "merged": 1, + "apply": {"merged_at": "2026-08-18T00:00:00", "applied": ["R1-001"], + "failed": []}, + "apply_base_sha": head, "apply_progress": [], "drops": [], + "reviews": [], "fix_rounds": 0, "fix_attempts": 1, + "fix_base_sha": head, "deferred": [], "durations": {}, + "proposal_keys": [], "pending_drop": [], "pending_push": False, + "started_at": "2026-08-18T00:00:00", + }], + items=[{"item_id": "R1-001", "round": 1, "path": "src.py", + "symbol": "f", "smell": "long_method", "technique": "extract_method", + "severity": "major", "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 10, "proposed_by": ["codex"], + "status": "applied", "commits": []}], + ) + env_tmp_dir(state_path) + monkeypatch.setattr(refactor, "_push_head", lambda state: None) + write_result(state_path, "codex-fix-r1", + {"resolved_thread_ids": [], "unresolved": [], "commits": []}) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + + refactor.cmd_merge_fix(type("A", (), {"id": 130, "round": 1})()) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert read_state(state_path)["rounds"][0]["fix_rounds"] == 1 diff --git a/plugins/ndf-codex/skills/cross-review/SKILL.md b/plugins/ndf-codex/skills/cross-review/SKILL.md index a189e767..90492152 100644 --- a/plugins/ndf-codex/skills/cross-review/SKILL.md +++ b/plugins/ndf-codex/skills/cross-review/SKILL.md @@ -41,6 +41,7 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲 | 観点 | 方針 | |---|---| | レビュー投稿 | **AI 自身が `gh api` で PR に直接投稿**。メインはペイロードを保持しない | +| 投稿の確認 | **申告されたコメント数を GitHub 側と突き合わせる**。投稿が届いていなければ中断する（取得できない場合は申告を採用） | | 修正 | **必ずサブエージェント (`general-purpose`) で実行**。メイン context に diff は載せない | | ユーザ問い合わせ | 自動判断を最大化（`critical`/`major`/`minor` は自動修正、ループ中の `nit` は deferred） | | 取りこぼし防止 | **ループ終了時（approved / max_rounds / oscillation / error いずれも）に最終スイープを必須実行**。`/ndf:fix` を再実行し、残った open review thread（最終 APPROVE ラウンドの minor/nit インラインコメント含む）を **全て解消**。修正可能なものは修正 + push、判断保留 nit も reply + resolveReviewThread して **open thread 0 で終了** | @@ -394,6 +395,8 @@ pint / larastan / test / build などは **中断** を原則とする。 - ❌ **修正をメインセッション内で行う** — context が一気に膨れる。必ずサブエージェント - ❌ **AI に Markdown だけ返させる** — メインがパース・投稿する設計は禁物。AI 直接投稿 +- ❌ **result.json の申告だけで判定を進める** — 投稿が失敗しても件数は残る。GitHub 側の + 実数と突き合わせないと、修正担当が読むべき指摘が存在しないまま収束する - ❌ **nit を都度ユーザに問う** — ループ中は deferred 記録のみ。最終スイープ (Step 7.5) で Resolve - ❌ **未解決スレッドを残したまま終了する** — approved/max_rounds 等いずれの終了経路でも Step 7.5 の最終スイープを必ず実行し、open review thread 0 で終える。特に **最終 APPROVE diff --git a/plugins/ndf-codex/skills/cross-review/docs/01-state-and-review.md b/plugins/ndf-codex/skills/cross-review/docs/01-state-and-review.md index 65da8c98..c702a8ef 100644 --- a/plugins/ndf-codex/skills/cross-review/docs/01-state-and-review.md +++ b/plugins/ndf-codex/skills/cross-review/docs/01-state-and-review.md @@ -216,6 +216,25 @@ launcher が生成するプロンプトに以下を強制している: `state.rounds[-1].` に `intent / posted_as / comments / review_url / by_severity` を分離保存する。 +#### 申告されたコメント数を GitHub 側と突き合わせる + +投稿は **AI 自身が `gh api` で行う**ため、失敗しても結果ファイルの申告だけは残る。 +申告のまま進むと、修正担当が読むべき指摘が GitHub 上に存在しないまま収束判定まで走る。 +実測では、2 件の申告に対しスレッドが 1 つも作られていなかった。 + +`read-result` は申告が 1 件以上のとき、`review_url` の識別子から +`repos//pulls//reviews//comments` を数えて突き合わせる。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 突き合わせる相手がいない | +| n 件 | n 件以上 | 採用する。人の追記など申告以外の経路で増えうる | +| n 件 | n 件未満 | **中断する。** 投稿が届いていない | +| n 件 | 取得できない | 申告を採用し、確認できなかったことを出力へ残す | + +**「取得できなかった」と「0 件」を区別する。** 取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 + ## Step 3: 判定（intent ベース） ```bash diff --git a/plugins/ndf-codex/skills/cross-review/scripts/state.py b/plugins/ndf-codex/skills/cross-review/scripts/state.py index 9e91f11f..c001d2aa 100755 --- a/plugins/ndf-codex/skills/cross-review/scripts/state.py +++ b/plugins/ndf-codex/skills/cross-review/scripts/state.py @@ -29,6 +29,7 @@ import json import os import pathlib +import re import shlex import subprocess import sys @@ -967,6 +968,44 @@ def cmd_start_round(args: argparse.Namespace) -> None: print(f"ROTATE_AFTER={st['rotate_after']}") +def _as_count(value: object) -> int: + """申告された件数を整数として読む。読めない値は 0 として扱う。 + + 相手は LLM なので、文字列や `null` が入ることがある。読めない申告を + 「件数あり」と見なすと、突き合わせる相手が決まらないまま中断してしまう。 + """ + try: + return max(0, int(value)) # type: ignore[arg-type] + except (TypeError, ValueError): + return 0 + + +def _posted_comment_count(repo: str, pr: int, review_url: str | None) -> int | None: + """レビューに実際にぶら下がっているインラインコメントの数。 + + 取得できなければ `None` を返す。**「取得できなかった」と「0 件」を区別する。** + 取得の失敗で中断すると、GitHub 側の一時的な不調でループが止まる。 + + 投稿は AI 自身が `gh api` で行うため、失敗しても結果ファイルの申告だけは残る。 + 数え直す先は、申告された `review_url` の末尾にある識別子から決める。 + """ + if not repo or not review_url: + return None + m = re.search(r"pullrequestreview-(\d+)", str(review_url)) + if not m: + return None + try: + out = _sh( + ["gh", "api", f"repos/{repo}/pulls/{pr}/reviews/{m.group(1)}/comments", + "--paginate", "--jq", "length"], + check=False, + ) + except Exception: + return None + counts = [int(line) for line in str(out).split() if line.strip().isdigit()] + return sum(counts) if counts else None + + def cmd_read_result(args: argparse.Namespace) -> None: """Step 2.5 — codex/gemini の result.json を state にマージ。""" agent = args.agent @@ -1007,6 +1046,25 @@ def cmd_read_result(args: argparse.Namespace) -> None: st = _load(pr) if not st.get("rounds"): die(f"{agent}: state.rounds が空。`state.py start-round` を先に呼んでください") + + # **申告を GitHub 側と突き合わせる。** 投稿は AI 自身が行うので、失敗しても + # 結果ファイルには件数が残る。申告のまま進むと、修正担当が読むべき指摘が + # GitHub 上に存在しないまま収束判定まで走る（実測: 申告 2 件に対しスレッド 0）。 + declared = _as_count(comments) + if declared > 0: + actual = _posted_comment_count(str(st.get("repo") or ""), pr, r.get("review_url")) + if actual is None: + info( + f"⚠ {agent}: 投稿されたコメント数を確認できませんでした。" + f"申告（{declared} 件）をそのまま採用します" + ) + elif actual < declared: + die( + f"{agent}: インラインコメントの申告 {declared} 件に対し、" + f"GitHub 上には {actual} 件しかありません。投稿が届いていないため" + "中断します。レビューを投稿し直してから再実行してください" + ) + st["rounds"][-1][agent] = { "intent": intent, "posted_as": posted_as, diff --git a/plugins/ndf-codex/skills/cross-review/tests/test_state_posted_comments.py b/plugins/ndf-codex/skills/cross-review/tests/test_state_posted_comments.py new file mode 100644 index 00000000..b57ffc95 --- /dev/null +++ b/plugins/ndf-codex/skills/cross-review/tests/test_state_posted_comments.py @@ -0,0 +1,191 @@ +"""申告されたインラインコメント数を、GitHub 側の実数と突き合わせる。 + +レビューの投稿は AI 自身が `gh api` で行うため、**投稿に失敗しても結果ファイルの +申告だけは残る**。申告を信じて先へ進むと、修正担当が読むべき指摘が GitHub 上に +存在しないまま収束判定まで走る。実測では 2 件の申告に対しスレッドが 1 つも +作られていなかった。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 投稿が無いので突き合わせる相手がいない | +| 2 件 | 2 件 | そのまま採用する | +| 2 件 | 0 件 | 投稿が届いていないので中断する | +| 2 件 | 取得できない | 申告を採用し、確認できなかったことを残す | + +「取得できなかった」と「0 件」を混同しない。取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + +PR = 4242 +AGENT = "gemini" +REVIEW_URL = f"https://github.com/o/r/pull/{PR}#pullrequestreview-4961230016" + + +def _seed_state(tmp_dir: pathlib.Path) -> None: + state = { + "current_pr": PR, + "repo": "o/r", + "rounds": [{"round": 1, "pr": PR, "started_at": "2026-08-18T00:00:00+00:00"}], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _result(tmp_dir: pathlib.Path, **over) -> pathlib.Path: + payload = { + "event": "REQUEST_CHANGES", + "posted_as": "REQUEST_CHANGES", + "comments_count": 2, + "review_url": REVIEW_URL, + "by_severity": {"major": 2}, + } + payload.update(over) + rfile = tmp_dir / "result.json" + rfile.write_text(json.dumps(payload)) + return rfile + + +def _args(rfile: pathlib.Path) -> argparse.Namespace: + return argparse.Namespace(pr=PR, agent=AGENT, file=str(rfile)) + + +def _read_state(tmp_dir: pathlib.Path) -> dict: + return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text()) + + +@pytest.fixture() +def tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +@pytest.fixture() +def posted(monkeypatch, state_mod): + """GitHub 側の件数を差し替える。`None` は取得できなかったことを表す。""" + def _set(count): + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda repo, pr, review_url: count, + ) + return _set + + +def test_declared_count_matching_github_is_accepted(tmp_dir, state_mod, posted): + _seed_state(tmp_dir) + posted(2) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_declared_comments_missing_on_github_aborts(tmp_dir, state_mod, posted): + """申告があるのに GitHub 側へ届いていなければ中断する。 + + そのまま進むと、修正担当が読むべき指摘が存在しないまま収束判定まで走る。 + """ + _seed_state(tmp_dir) + posted(0) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert e.value.code == 1 + assert AGENT not in _read_state(tmp_dir)["rounds"][-1] + + +def test_partially_posted_comments_abort(tmp_dir, state_mod, posted): + """一部しか届いていない場合も中断する。取りこぼしは全件欠落と同じ扱いにする。""" + _seed_state(tmp_dir) + posted(1) + + with pytest.raises(SystemExit): + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + +def test_more_comments_on_github_is_accepted(tmp_dir, state_mod, posted): + """GitHub 側が多い分には通す。人の追記など、申告以外の経路で増えうる。""" + _seed_state(tmp_dir) + posted(3) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_zero_declared_skips_the_check(tmp_dir, state_mod, monkeypatch): + """申告 0 件なら GitHub を見に行かない。""" + _seed_state(tmp_dir) + called: list = [] + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda *a, **k: called.append(a) or 0, + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, event="APPROVE", comments_count=0))) + + assert called == [] + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 0 + + +def test_unavailable_github_count_keeps_the_declaration(tmp_dir, state_mod, posted): + """GitHub 側を取得できなければ申告を採用する。取得失敗で止めない。""" + _seed_state(tmp_dir) + posted(None) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_missing_review_url_is_treated_as_unavailable(tmp_dir, state_mod, monkeypatch): + """投稿先の参照が無ければ、突き合わせる相手を決められないので申告を採用する。""" + _seed_state(tmp_dir) + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("参照が無いのに GitHub を呼んでいる"), + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, review_url=None))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +# ---------------- 件数の取得 ---------------- + +def test_posted_count_reads_the_review_id_from_the_url(state_mod, monkeypatch): + calls: list[list[str]] = [] + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: calls.append(list(cmd)) or "2", + ) + + count = state_mod._posted_comment_count("o/r", PR, REVIEW_URL) + + assert count == 2 + assert calls and "repos/o/r/pulls/4242/reviews/4961230016/comments" in calls[0] + + +def test_posted_count_is_none_when_the_url_has_no_review_id(state_mod, monkeypatch): + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("識別子が無いのに GitHub を呼んでいる"), + ) + + assert state_mod._posted_comment_count("o/r", PR, "https://example.test/") is None + + +def test_posted_count_is_none_when_the_api_fails(state_mod, monkeypatch): + def boom(cmd, check=True): + raise RuntimeError("network") + + monkeypatch.setattr(state_mod, "_sh", boom) + + assert state_mod._posted_comment_count("o/r", PR, REVIEW_URL) is None diff --git a/plugins/ndf-kiro/README.md b/plugins/ndf-kiro/README.md index ff43e33f..a3f11072 100644 --- a/plugins/ndf-kiro/README.md +++ b/plugins/ndf-kiro/README.md @@ -12,7 +12,7 @@ cat plugins/ndf-kiro/VERSION # 導入済みプロジェクトの版数 python3 -c "import json;print(json.load(open('.kiro/agents/ndf.json'))['description'])" -# => NDF統合開発エージェント（Kiro CLI用 / v8.4.0） +# => NDF統合開発エージェント（Kiro CLI用 / v8.5.0） ``` `install.sh` は実行時にも `NDF バージョン: <版数>` を表示する。 diff --git a/plugins/ndf-kiro/VERSION b/plugins/ndf-kiro/VERSION index a2f28f43..6d289079 100644 --- a/plugins/ndf-kiro/VERSION +++ b/plugins/ndf-kiro/VERSION @@ -1 +1 @@ -8.4.0 +8.5.0 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md b/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md index eaac7f69..5daa2b50 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/SKILL.md @@ -193,11 +193,17 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" while :; do # 提案ラウンドの繰り返し rf_eval start-round "$ID" || break # 終了コード 1 = 繰り返し終了 + # **提案の直前に読み取り用を同期する。** 前ラウンドの取り消しで HEAD が進んで + # いるため、同期しないと**消えたコードに対する提案**が返る（実測: 取り消しで + # 消えた関数へ 2 件）。HEAD が変わっていなければ何も起きない。 + "$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done + # 提案の所要は参加ランタイムと回線状況で振れる（実測 90〜285 秒）。既定の + # 打ち切りに任せず、明示する。 "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 rf merge-proposals "$ID" || break # 終了コード 2 = 採用 0 件 "$SCRIPTS/launch-cli.sh" "$IMPL" apply "$ID" "$ROUND" @@ -213,7 +219,7 @@ while :; do # 提案ラウンドの繰り返 "$SCRIPTS/launch-cli.sh" "$r" review "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$REVIEWERS_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-review-r$ROUND" + --stem-template "{agent}-review-r$ROUND" --timeout 900 rf judge-review "$ID" "$ROUND"; rc=$? [ $rc -eq 0 ] && break # 2 者とも承認 [ $rc -eq 3 ] && continue # 形式不正 — 差し戻して再レビュー @@ -271,6 +277,7 @@ done | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | | `git push --force` / `--no-verify` を使う | 他者の作業を消す。検証を飛ばす | +| 実装担当のコミットでフックの通し方を決めない | 生成物の同期を検査するリポジトリでは、同期の禁止と両立せずコミットを作れなくなる。迂回してよい手段を 1 つ定める | | 提案とレビューにホストを混ぜる | 実装者と評価者が同一モデルになりうる。初期化時に検査して失敗させている | | kiro を既定モデルのまま計測する | `auto` は実際に選ばれたモデルを取得できない | | 提案フェーズでコードを直す | 提案は読むだけ。直すのは実装担当 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 45207583..fb751789 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 @@ -182,11 +182,12 @@ claude 17 本 / codex 1 メソッド / kiro 0 本と揃わなかった。最後 ## Step 2: 提案 ```bash +"$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 ``` 3 CLI を並列で起動し、同一のプロンプトで提案させる。**提案フェーズにホストは現れない** @@ -195,6 +196,24 @@ done 提出形式は [prompts/propose.md](../prompts/propose.md) にある。 +### 提案の直前に読み取り用を同期する + +**同期が要るのは HEAD が進んだときであって、特定のフェーズの後ではない。** +適用と修正の直後だけを同期していると、取り消しで進んだ HEAD が読み取り用へ +届かない。実測では、ラウンドを全件取り消した次の提案で、**取り消しによって +消えた関数**に対する提案が 2 件返った。統合は対象の実在を検査しないため、 +そのまま採用され、適用で必ず失敗する。 + +提案の直前に同期しておけば、どのフェーズを経ていても読み取り用は最新になる。 +HEAD が変わっていなければ何も起きないので、重ねて呼んでも無駄がない。 + +### 打ち切りまでの時間を明示する + +提案の所要はランタイムと回線状況で振れる（実測 90〜285 秒）。既定の打ち切りに +任せると、分析そのものは進んでいるのに時間切れで結果を捨てることがある。 +結果ファイルには `idle_seconds` が残るので、**止まっていたのか間に合わなかったのか**は +後から読める。 + ### 結果ファイル名にラウンド番号を入れる CLI の起動時に同名の結果ファイルを消すため、**提案の結果ファイル名にもラウンド番号が 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 4609b02a..c5d18be3 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 @@ -199,6 +199,32 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 終わると、Pull Request 側には未検証の差分が残るのに、次の実行は処理済みガードで 素通りしてしまう。印があれば、次の実行が判定より先に再送信する。 +### 実装担当のコミットはフックの検査を通さなくてよい + +**生成物の同期を pre-commit で検査するリポジトリでは、そのままだとコミットを作れない。** +実装担当は範囲内だけを変更するので生成物は必ず古くなり、同期は禁じられている。 + +公開は進行側が検証を通してから行うため、**実装担当のコミット時点で生成物が古いのは +設計どおり**である。そこで、フックが原因でコミットできないときは +`git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外させる。 + +- 禁止は `--no-verify` だけを名指しにしない。手段の名前で書き分けると、同じことが + 別の名前で起こる。実測では、同じ実装担当が適用フェーズではフックを迂回し、 + 修正フェーズでは迂回せず 0 コミットで終えた +- **直した内容は必ずコミットさせる。** 作業ツリーに置いたまま終えると、検証を + 受けていない変更として `merge-apply` / `merge-fix` が捨てる + +### 取り込みの前に置き土産を捨てる + +`merge-apply` と `merge-fix` は、実装担当が残した未コミットの変更を捨ててから +結果を読む。コミットされなかった変更はどの検証も受けておらず、公開する道が無い。 + +残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正フェーズで +コミットを作れなかった実装担当が直しかけの差分を置いたまま終え、続く `merge-fix` が +「修正 0 件」として先へ進むこともできなくなった。 + +制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、消えない。 + ### コミットトレーラーの形式 適用と修正のコミットメッセージ本文の末尾に、**git のトレーラー形式**で実行主体を残す。 diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md index bfaf491e..ec9c8b94 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/apply.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます diff --git a/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md index a73f3c3e..bb2180a9 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-kiro/skills/cross-refactoring/prompts/fix.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$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 1a224c66..2aa3c0d2 100755 --- a/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/scripts/refactor.py @@ -1099,6 +1099,7 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: path, state = _load(args.id) entry = _round(state, args.round) if not args.dry_run: + _discard_impl_leftovers(state, state["worktrees"]["work"]) _resume_incomplete_apply(path, state, entry) # **叩き直しても同じ判定を返す。** 取り込み済みで再実行すると、前回作った @@ -1675,6 +1676,7 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: """Step 6 — 修正結果を取り込み、修正ラウンドを 1 つ進める。""" path, state = _load(args.id) entry = _round(state, args.round) + _discard_impl_leftovers(state, state["worktrees"]["work"]) _flush_pending_push(path, state, entry) impl = entry["impl"] result = _result_path(state, impl, stem_for(impl, "fix", state["id"], args.round)) @@ -2018,10 +2020,18 @@ def _reported_shas(reported: Any) -> list[str]: return shas -def _git_out(work: str, args: list[str]) -> Optional[str]: - """`git` を実行して標準出力を返す。失敗したら `None`。""" +def _git_out(work: str, args: list[str], strip: bool = True) -> Optional[str]: + """`git` を実行して標準出力を返す。失敗したら `None`。 + + **固定幅で読む出力には `strip=False` を渡す。** `git status --porcelain` の + 状態コードは未 stage の変更で ` M` と先頭が空白になるため、`strip()` すると + 1 行目だけ 1 文字ずれ、切り出したパスの先頭が欠ける。欠けたパスは + `git add` で `pathspec ... did not match any files` になり、同期が止まる。 + """ r = subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) - return r.stdout.strip() if r.returncode == 0 else None + if r.returncode != 0: + return None + return r.stdout.strip() if strip else r.stdout.rstrip("\n") def commits_in_range(work: str, base: Optional[str], head: str) -> Optional[list[str]]: @@ -2524,7 +2534,8 @@ def _worktree_changes(work: str) -> dict[str, str]: # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 out = _git_out( - work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"], + strip=False, ) changes: dict[str, str] = {} for line in (out or "").splitlines(): @@ -2579,6 +2590,33 @@ def _discard_worktree_changes(work: str) -> None: subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) +def _discard_impl_leftovers(state: dict[str, Any], work: str) -> None: + """実装担当が残した未コミットの変更を捨てる。取り込みの前に呼ぶ。 + + **公開は進行側が検証を通してから行う**ので、コミットされなかった変更は + どの検証も受けていない。Pull Request へ出す道が無い以上、残す意味がない。 + + 残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正 + フェーズでコミットを作れなかった実装担当が直しかけの差分を置いたまま終え、 + 続く `merge-fix` が「修正 0 件」として先へ進むこともできなくなった。 + + 制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、 + `git clean` に `-x` を付けないため消えない。 + """ + if not pathlib.Path(work).is_dir(): + return + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = "、".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + _discard_worktree_changes(work) + info( + f"🧹 コミットされなかった変更を捨てました（{shown}{more}）。" + "検証を受けていないため公開しません" + ) + + def _require_clean_worktree(state: dict[str, Any], work: str) -> None: """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 @@ -2643,8 +2681,17 @@ def _sync_generated(state: dict[str, Any]) -> None: produced = _dirty_paths(state, work) if not produced: return - _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + # **後段で落ちたときも差分を残さない。** `git add` / `git commit` の失敗で + # 作業ツリーを汚したまま中断すると、次の実行は `_require_clean_worktree` で + # 必ず止まり、`pending_push` の再試行が永久に進まない。捨ててよい根拠は + # 同期コマンド自身が失敗したときと同じで、着手前が綺麗だったことを + # 確認済みだからである。 + try: + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + except SystemExit: + _discard_worktree_changes(work) + raise info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") 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 41928945..5101513d 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 @@ -98,7 +98,7 @@ def test_deferred_entry_records_the_reason(refactor, tmp_path, env_tmp_dir, no_g def _history(refactor, monkeypatch, newest_first): """`git rev-list HEAD` の結果（新しい順）と SHA 解決を差し替える。""" - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:1] == ["rev-list"]: return "\n".join(newest_first) if args[:2] == ["rev-parse", "--verify"]: @@ -232,7 +232,7 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, # 未申告コミットの判定がこの解決を通るため。 monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr( @@ -406,7 +406,7 @@ def test_broken_fix_result_does_not_crash( }) monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: set()) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: []) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD") monkeypatch.setattr( refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: [], @@ -436,7 +436,7 @@ def test_merge_fix_uses_the_recorded_range(refactor, tmp_path, env_tmp_dir, monk ) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -452,7 +452,7 @@ def test_merge_fix_fails_when_the_range_cannot_be_determined( ): state_path = _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, ["PRRT_a"]) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: None) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD_NOW") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD_NOW") monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: {"PRRT_a"}) with pytest.raises(SystemExit) as e: @@ -470,7 +470,7 @@ def test_merge_fix_rejects_unreported_commits( refactor, "commits_in_range", lambda work, base, head: ["sneaky", "fix111"]) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" + lambda work, args, **_kw: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" else "HEAD_NOW", ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -691,7 +691,7 @@ def test_merge_fix_is_idempotent_after_a_revert( calls.clear() monkeypatch.setattr( refactor, "_git_out", - lambda work, args: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] + lambda work, args, **_kw: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] else args[-1].replace("^{commit}", "")), ) refactor.cmd_merge_fix(args) @@ -799,7 +799,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_judge_review.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_judge_review.py index 0105fa34..a3121ef2 100644 --- a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_judge_review.py +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_judge_review.py @@ -273,7 +273,7 @@ def test_judge_command_records_the_fix_base(refactor, tmp_path, env_tmp_dir, mon """修正コミットの実在を確かめるため、変更要求の時点の HEAD を残すこと。""" state_path = _state(tmp_path) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "FIX_BASE") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "FIX_BASE") write_result(state_path, "gemini-review-r1", review("REQUEST_CHANGES", [finding()])) write_result(state_path, "kiro-review-r1", review()) 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 28aa9cb7..743712c9 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 @@ -5,6 +5,7 @@ """ from __future__ import annotations +import pathlib import subprocess import pytest @@ -149,7 +150,7 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): """結果ファイルではなく実際のコミットメッセージから読む。""" monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "Item-Id: R1-001\nRound: 1\n" + lambda work, args, **_kw: "Item-Id: R1-001\nRound: 1\n" "Impl-Runtime: codex\nImpl-Model: gpt-5.5", ) assert refactor.commit_trailers("/w", "abc") == { @@ -159,31 +160,31 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): def test_commit_trailers_are_empty_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commit_trailers("/w", "abc") == {} def test_diff_lines_come_from_numstat(refactor, monkeypatch): monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", + lambda work, args, **_kw: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", ) assert refactor.commit_diff_lines("/w", "abc") == 20 def test_touches_tests_detects_test_paths(refactor, monkeypatch): monkeypatch.setattr(refactor, "_git_out", - lambda work, args: "src/a.py\ntests/test_a.py") + lambda work, args, **_kw: "src/a.py\ntests/test_a.py") assert refactor.commit_touches_tests("/w", "abc") is True - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "src/a.py") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "src/a.py") assert refactor.commit_touches_tests("/w", "abc") is False def test_commits_in_range_uses_rev_list(refactor, monkeypatch): calls = [] - def fake(work, args): + def fake(work, args, **_kw): calls.append(args) return "aaa\nbbb" @@ -198,7 +199,7 @@ def test_commits_in_range_is_none_without_base(refactor): def test_commits_in_range_is_none_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commits_in_range("/w", "base", "head") is None @@ -206,7 +207,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): """テストは実際に走らせる。実行後は必ず元のブランチへ戻す。""" git_calls = [] monkeypatch.setattr( - refactor, "_git_out", lambda work, args: git_calls.append(args) or "") + refactor, "_git_out", lambda work, args, **_kw: git_calls.append(args) or "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: (git_calls.append(a[0]) if isinstance(a[0], list) else None) @@ -220,7 +221,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): def test_run_test_at_reports_failure(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: subprocess.CompletedProcess(a[0], 0, "", ""), @@ -239,7 +240,7 @@ def fake_run(cmd, **kw): return subprocess.CompletedProcess(cmd, 0, "", "") raise OSError("テスト実行が壊れた") - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr(refactor.subprocess, "run", fake_run) with pytest.raises(OSError): refactor.run_test_at("/w", "abc", "pytest -q", "main") @@ -247,13 +248,13 @@ def fake_run(cmd, **kw): def test_collect_facts_marks_unknown_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) facts = refactor.collect_commit_facts("/w", ["ghost"], {"aaa"}, "true", "main") assert facts == [{"sha": "ghost", "exists": False}] def test_collect_facts_marks_out_of_range_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "zzz") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "zzz") facts = refactor.collect_commit_facts("/w", ["zzz"], {"aaa"}, "true", "main") assert facts[0]["exists"] is False @@ -289,7 +290,7 @@ def _set(mapping, in_range=None): # SHA をそのまま返す形にしておく monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", ""), + lambda work, args, **_kw: args[-1].replace("^{commit}", ""), ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -383,11 +384,21 @@ 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, sync_dirty=False): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False, + leftover=""): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 積み直しで SHA が変わることを、状態の更新まで含めて確かめられるようにする。 + + 作業ツリーの状態は 3 段階で返す。取り込みの前に実装担当の置き土産を捨てる + ため、同期の前後だけでは足りない。 + + | 呼ばれる場面 | 返す値 | + | --- | --- | + | 取り込みの前（置き土産の確認） | `leftover` | + | 同期の前（清浄性の検査） | `sync_dirty[0]` | + | 同期の後（生成された差分） | `sync_dirty[1]` | """ calls: list[list[str]] = [] picked: list[str] = [] @@ -407,7 +418,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: @@ -415,13 +426,13 @@ def fake_git_out(work, args): 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 len(statuses) == 1: + return leftover if sync_dirty is False: return "" before, after = sync_dirty - return before if len(statuses) == 1 else after + return before if len(statuses) == 2 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -842,7 +853,7 @@ def test_apply_base_is_recorded_by_the_orchestrator( "durations": {}, "reviews": [], }]) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "BASE_HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "BASE_HEAD") for rt in ("codex", "gemini", "kiro"): write_result(state_path, f"{rt}-propose-rf130", {"items": []}) with pytest.raises(SystemExit): @@ -1001,7 +1012,7 @@ def test_short_and_full_sha_are_seen_as_the_same_commit( # 短縮 SHA も完全 SHA も同じコミットへ解決される monkeypatch.setattr( refactor, "_git_out", - lambda work, args: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", + lambda work, args, **_kw: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -1256,9 +1267,16 @@ def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, g # ---------- push の直前に生成物を同期する ---------- def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + """同期コマンドを持つ状態を作る。 + + 書き込み用の作業ディレクトリを実在させる。取り込みの前に置き土産を確認する + 経路は、ディレクトリが無ければ何もせずに戻るため、実在しないと + `_drop_env` の 3 段階（置き土産 / 同期前 / 同期後）が 1 つずれる。 + """ state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) state = read_state(state_path) state["sync_command"] = command + pathlib.Path(state["worktrees"]["work"]).mkdir(parents=True, exist_ok=True) state_path.write_text(__import__("json").dumps(state), encoding="utf-8") return state_path @@ -1517,7 +1535,7 @@ def test_status_disables_path_quoting(refactor, monkeypatch): seen: list[list[str]] = [] monkeypatch.setattr( refactor, "_git_out", - lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + lambda work, args, **_kw: 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"] diff --git a/plugins/ndf-kiro/skills/cross-refactoring/tests/test_sync_generated.py b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_sync_generated.py new file mode 100644 index 00000000..008259ac --- /dev/null +++ b/plugins/ndf-kiro/skills/cross-refactoring/tests/test_sync_generated.py @@ -0,0 +1,223 @@ +"""生成物の同期と、実装担当が残した未コミット変更の扱いを**実際の git** で確かめる。 + +同期は `--sync-command` を持つリポジトリで push の直前に走る、進行側の責務である。 +ここが落ちると取り消しを Pull Request へ反映できないため、進行そのものが止まる。 + +| 確かめること | なぜ | +| --- | --- | +| 変更のパスを 1 文字も欠かさず拾う | `git status --porcelain` は固定幅。先頭の空白を削ると 1 行目がずれる | +| 同期の後段で落ちても差分を残さない | 残すと次の実行が清浄性の検査で必ず止まる | +| 実装担当の置き土産を捨ててから取り込む | 検証を受けていない変更なので公開しない。止まる理由にもしない | +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state, write_result + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + return _git("rev-parse", "HEAD", cwd=repo).stdout.strip() + + +def _make_work(tmp_path): + """`work` を本物のリポジトリとして作り、状態ファイルを添えて返す。""" + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _state_with_sync(tmp_path, work, command="true"): + return make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + sync_command=command, + ) + + +# ---------- 変更のパスを 1 文字も欠かさず拾う ---------- + +def test_unstaged_change_on_first_line_keeps_full_path(refactor, tmp_path): + """先頭が空白の状態コード（` M`）でも、パスの先頭文字が消えない。 + + `git status --porcelain` は「状態 2 文字 + 空白 + パス」の固定幅で、 + 未 stage の変更は 1 文字目が空白になる。出力全体を `strip()` してから + 固定幅で切り出すと、**1 行目だけ**パスが 1 文字短くなる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + + changes = refactor._worktree_changes(str(work)) + + assert "src.py" in changes + + +def test_every_changed_path_is_addable(refactor, tmp_path): + """拾ったパスは、そのまま `git add` に渡して通る。""" + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 2\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + paths = refactor._dirty_paths(state, str(work)) + + assert paths == ["generated/out.py", "src.py"] + _git("add", "--", *paths, cwd=work) + + +# ---------- 同期コミット ---------- + +def test_sync_commits_generated_changes(refactor, tmp_path): + """同期コマンドが作った差分は、進行側のコミットとして積まれる。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + assert subject == refactor.SYNC_COMMIT_MESSAGE.splitlines()[0] + + +def test_sync_without_changes_makes_no_commit(refactor, tmp_path): + """差分が出ない同期はコミットを作らない。""" + work = _make_work(tmp_path) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state = read_state(_state_with_sync(tmp_path, work, command="true")) + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +# ---------- 同期の後段で落ちたとき ---------- + +def test_failure_after_sync_discards_produced_changes(refactor, tmp_path, monkeypatch): + """`git add` / `git commit` が落ちても、同期が作った差分を残さない。 + + 残すと次の実行は清浄性の検査で必ず止まり、保留中の push を再試行できない。 + """ + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + monkeypatch.setattr(refactor, "_sh", + lambda *a, **k: refactor.die("commit に失敗しました")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_failed_sync_command_discards_partial_changes(refactor, tmp_path): + """同期コマンド自身が落ちたときも、途中まで書き換えた差分を残さない。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py; exit 1")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +# ---------- 実装担当が残した未コミット変更 ---------- + +def test_leftover_changes_are_discarded_before_merge(refactor, tmp_path): + """実装担当が残した未コミット変更は、取り込みの前に捨てる。 + + 公開は進行側が検証を通してから行うので、コミットされなかった変更は + **検証を受けていない**。残したまま進むと、清浄性の検査で進行が止まる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + refactor._discard_impl_leftovers(state, str(work)) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert (work / "src.py").read_text(encoding="utf-8") == "x = 1\n" + + +def test_discard_keeps_control_directory(refactor, tmp_path): + """制御用ディレクトリ（状態・結果・ログ）は捨てない。""" + work = _make_work(tmp_path) + control = work / ".cross_refactoring" + control.mkdir() + (control / "keep.json").write_text("{}", encoding="utf-8") + (work / ".gitignore").write_text(".cross_refactoring/\n", encoding="utf-8") + _commit(work, "ignore control dir") + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(make_state( + tmp_path, + worktrees={"work": str(work)}, + tmp_dir=str(control), + )) + + refactor._discard_impl_leftovers(state, str(work)) + + assert (control / "keep.json").exists() + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_merge_fix_continues_when_impl_left_changes( + refactor, tmp_path, env_tmp_dir, monkeypatch +): + """修正フェーズの置き土産があっても、`merge-fix` は中断しない。 + + 実装担当がコミットを作れずに終えると作業ツリーへ差分が残る。これを理由に + 止めると、修正 0 件として先へ進むこともできなくなる。 + """ + work = _make_work(tmp_path) + head = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state_path = make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + rounds=[{ + "round": 1, "impl": "codex", "impl_model": None, + "reviewers": ["gemini", "kiro"], "reviewer_models": {}, + "items": ["R1-001"], "adopted": 1, "proposed": 1, "merged": 1, + "apply": {"merged_at": "2026-08-18T00:00:00", "applied": ["R1-001"], + "failed": []}, + "apply_base_sha": head, "apply_progress": [], "drops": [], + "reviews": [], "fix_rounds": 0, "fix_attempts": 1, + "fix_base_sha": head, "deferred": [], "durations": {}, + "proposal_keys": [], "pending_drop": [], "pending_push": False, + "started_at": "2026-08-18T00:00:00", + }], + items=[{"item_id": "R1-001", "round": 1, "path": "src.py", + "symbol": "f", "smell": "long_method", "technique": "extract_method", + "severity": "major", "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 10, "proposed_by": ["codex"], + "status": "applied", "commits": []}], + ) + env_tmp_dir(state_path) + monkeypatch.setattr(refactor, "_push_head", lambda state: None) + write_result(state_path, "codex-fix-r1", + {"resolved_thread_ids": [], "unresolved": [], "commits": []}) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + + refactor.cmd_merge_fix(type("A", (), {"id": 130, "round": 1})()) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert read_state(state_path)["rounds"][0]["fix_rounds"] == 1 diff --git a/plugins/ndf-kiro/skills/cross-review/SKILL.md b/plugins/ndf-kiro/skills/cross-review/SKILL.md index 8f4118a1..081e66b4 100644 --- a/plugins/ndf-kiro/skills/cross-review/SKILL.md +++ b/plugins/ndf-kiro/skills/cross-review/SKILL.md @@ -41,6 +41,7 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲 | 観点 | 方針 | |---|---| | レビュー投稿 | **AI 自身が `gh api` で PR に直接投稿**。メインはペイロードを保持しない | +| 投稿の確認 | **申告されたコメント数を GitHub 側と突き合わせる**。投稿が届いていなければ中断する（取得できない場合は申告を採用） | | 修正 | **必ずサブエージェント (`general-purpose`) で実行**。メイン context に diff は載せない | | ユーザ問い合わせ | 自動判断を最大化（`critical`/`major`/`minor` は自動修正、ループ中の `nit` は deferred） | | 取りこぼし防止 | **ループ終了時（approved / max_rounds / oscillation / error いずれも）に最終スイープを必須実行**。`/ndf:fix` を再実行し、残った open review thread（最終 APPROVE ラウンドの minor/nit インラインコメント含む）を **全て解消**。修正可能なものは修正 + push、判断保留 nit も reply + resolveReviewThread して **open thread 0 で終了** | @@ -394,6 +395,8 @@ pint / larastan / test / build などは **中断** を原則とする。 - ❌ **修正をメインセッション内で行う** — context が一気に膨れる。必ずサブエージェント - ❌ **AI に Markdown だけ返させる** — メインがパース・投稿する設計は禁物。AI 直接投稿 +- ❌ **result.json の申告だけで判定を進める** — 投稿が失敗しても件数は残る。GitHub 側の + 実数と突き合わせないと、修正担当が読むべき指摘が存在しないまま収束する - ❌ **nit を都度ユーザに問う** — ループ中は deferred 記録のみ。最終スイープ (Step 7.5) で Resolve - ❌ **未解決スレッドを残したまま終了する** — approved/max_rounds 等いずれの終了経路でも Step 7.5 の最終スイープを必ず実行し、open review thread 0 で終える。特に **最終 APPROVE diff --git a/plugins/ndf-kiro/skills/cross-review/docs/01-state-and-review.md b/plugins/ndf-kiro/skills/cross-review/docs/01-state-and-review.md index 4e9cda50..619d4756 100644 --- a/plugins/ndf-kiro/skills/cross-review/docs/01-state-and-review.md +++ b/plugins/ndf-kiro/skills/cross-review/docs/01-state-and-review.md @@ -216,6 +216,25 @@ launcher が生成するプロンプトに以下を強制している: `state.rounds[-1].` に `intent / posted_as / comments / review_url / by_severity` を分離保存する。 +#### 申告されたコメント数を GitHub 側と突き合わせる + +投稿は **AI 自身が `gh api` で行う**ため、失敗しても結果ファイルの申告だけは残る。 +申告のまま進むと、修正担当が読むべき指摘が GitHub 上に存在しないまま収束判定まで走る。 +実測では、2 件の申告に対しスレッドが 1 つも作られていなかった。 + +`read-result` は申告が 1 件以上のとき、`review_url` の識別子から +`repos//pulls//reviews//comments` を数えて突き合わせる。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 突き合わせる相手がいない | +| n 件 | n 件以上 | 採用する。人の追記など申告以外の経路で増えうる | +| n 件 | n 件未満 | **中断する。** 投稿が届いていない | +| n 件 | 取得できない | 申告を採用し、確認できなかったことを出力へ残す | + +**「取得できなかった」と「0 件」を区別する。** 取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 + ## Step 3: 判定（intent ベース） ```bash diff --git a/plugins/ndf-kiro/skills/cross-review/scripts/state.py b/plugins/ndf-kiro/skills/cross-review/scripts/state.py index 9e91f11f..c001d2aa 100755 --- a/plugins/ndf-kiro/skills/cross-review/scripts/state.py +++ b/plugins/ndf-kiro/skills/cross-review/scripts/state.py @@ -29,6 +29,7 @@ import json import os import pathlib +import re import shlex import subprocess import sys @@ -967,6 +968,44 @@ def cmd_start_round(args: argparse.Namespace) -> None: print(f"ROTATE_AFTER={st['rotate_after']}") +def _as_count(value: object) -> int: + """申告された件数を整数として読む。読めない値は 0 として扱う。 + + 相手は LLM なので、文字列や `null` が入ることがある。読めない申告を + 「件数あり」と見なすと、突き合わせる相手が決まらないまま中断してしまう。 + """ + try: + return max(0, int(value)) # type: ignore[arg-type] + except (TypeError, ValueError): + return 0 + + +def _posted_comment_count(repo: str, pr: int, review_url: str | None) -> int | None: + """レビューに実際にぶら下がっているインラインコメントの数。 + + 取得できなければ `None` を返す。**「取得できなかった」と「0 件」を区別する。** + 取得の失敗で中断すると、GitHub 側の一時的な不調でループが止まる。 + + 投稿は AI 自身が `gh api` で行うため、失敗しても結果ファイルの申告だけは残る。 + 数え直す先は、申告された `review_url` の末尾にある識別子から決める。 + """ + if not repo or not review_url: + return None + m = re.search(r"pullrequestreview-(\d+)", str(review_url)) + if not m: + return None + try: + out = _sh( + ["gh", "api", f"repos/{repo}/pulls/{pr}/reviews/{m.group(1)}/comments", + "--paginate", "--jq", "length"], + check=False, + ) + except Exception: + return None + counts = [int(line) for line in str(out).split() if line.strip().isdigit()] + return sum(counts) if counts else None + + def cmd_read_result(args: argparse.Namespace) -> None: """Step 2.5 — codex/gemini の result.json を state にマージ。""" agent = args.agent @@ -1007,6 +1046,25 @@ def cmd_read_result(args: argparse.Namespace) -> None: st = _load(pr) if not st.get("rounds"): die(f"{agent}: state.rounds が空。`state.py start-round` を先に呼んでください") + + # **申告を GitHub 側と突き合わせる。** 投稿は AI 自身が行うので、失敗しても + # 結果ファイルには件数が残る。申告のまま進むと、修正担当が読むべき指摘が + # GitHub 上に存在しないまま収束判定まで走る（実測: 申告 2 件に対しスレッド 0）。 + declared = _as_count(comments) + if declared > 0: + actual = _posted_comment_count(str(st.get("repo") or ""), pr, r.get("review_url")) + if actual is None: + info( + f"⚠ {agent}: 投稿されたコメント数を確認できませんでした。" + f"申告（{declared} 件）をそのまま採用します" + ) + elif actual < declared: + die( + f"{agent}: インラインコメントの申告 {declared} 件に対し、" + f"GitHub 上には {actual} 件しかありません。投稿が届いていないため" + "中断します。レビューを投稿し直してから再実行してください" + ) + st["rounds"][-1][agent] = { "intent": intent, "posted_as": posted_as, diff --git a/plugins/ndf-kiro/skills/cross-review/tests/test_state_posted_comments.py b/plugins/ndf-kiro/skills/cross-review/tests/test_state_posted_comments.py new file mode 100644 index 00000000..b57ffc95 --- /dev/null +++ b/plugins/ndf-kiro/skills/cross-review/tests/test_state_posted_comments.py @@ -0,0 +1,191 @@ +"""申告されたインラインコメント数を、GitHub 側の実数と突き合わせる。 + +レビューの投稿は AI 自身が `gh api` で行うため、**投稿に失敗しても結果ファイルの +申告だけは残る**。申告を信じて先へ進むと、修正担当が読むべき指摘が GitHub 上に +存在しないまま収束判定まで走る。実測では 2 件の申告に対しスレッドが 1 つも +作られていなかった。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 投稿が無いので突き合わせる相手がいない | +| 2 件 | 2 件 | そのまま採用する | +| 2 件 | 0 件 | 投稿が届いていないので中断する | +| 2 件 | 取得できない | 申告を採用し、確認できなかったことを残す | + +「取得できなかった」と「0 件」を混同しない。取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + +PR = 4242 +AGENT = "gemini" +REVIEW_URL = f"https://github.com/o/r/pull/{PR}#pullrequestreview-4961230016" + + +def _seed_state(tmp_dir: pathlib.Path) -> None: + state = { + "current_pr": PR, + "repo": "o/r", + "rounds": [{"round": 1, "pr": PR, "started_at": "2026-08-18T00:00:00+00:00"}], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _result(tmp_dir: pathlib.Path, **over) -> pathlib.Path: + payload = { + "event": "REQUEST_CHANGES", + "posted_as": "REQUEST_CHANGES", + "comments_count": 2, + "review_url": REVIEW_URL, + "by_severity": {"major": 2}, + } + payload.update(over) + rfile = tmp_dir / "result.json" + rfile.write_text(json.dumps(payload)) + return rfile + + +def _args(rfile: pathlib.Path) -> argparse.Namespace: + return argparse.Namespace(pr=PR, agent=AGENT, file=str(rfile)) + + +def _read_state(tmp_dir: pathlib.Path) -> dict: + return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text()) + + +@pytest.fixture() +def tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +@pytest.fixture() +def posted(monkeypatch, state_mod): + """GitHub 側の件数を差し替える。`None` は取得できなかったことを表す。""" + def _set(count): + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda repo, pr, review_url: count, + ) + return _set + + +def test_declared_count_matching_github_is_accepted(tmp_dir, state_mod, posted): + _seed_state(tmp_dir) + posted(2) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_declared_comments_missing_on_github_aborts(tmp_dir, state_mod, posted): + """申告があるのに GitHub 側へ届いていなければ中断する。 + + そのまま進むと、修正担当が読むべき指摘が存在しないまま収束判定まで走る。 + """ + _seed_state(tmp_dir) + posted(0) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert e.value.code == 1 + assert AGENT not in _read_state(tmp_dir)["rounds"][-1] + + +def test_partially_posted_comments_abort(tmp_dir, state_mod, posted): + """一部しか届いていない場合も中断する。取りこぼしは全件欠落と同じ扱いにする。""" + _seed_state(tmp_dir) + posted(1) + + with pytest.raises(SystemExit): + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + +def test_more_comments_on_github_is_accepted(tmp_dir, state_mod, posted): + """GitHub 側が多い分には通す。人の追記など、申告以外の経路で増えうる。""" + _seed_state(tmp_dir) + posted(3) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_zero_declared_skips_the_check(tmp_dir, state_mod, monkeypatch): + """申告 0 件なら GitHub を見に行かない。""" + _seed_state(tmp_dir) + called: list = [] + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda *a, **k: called.append(a) or 0, + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, event="APPROVE", comments_count=0))) + + assert called == [] + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 0 + + +def test_unavailable_github_count_keeps_the_declaration(tmp_dir, state_mod, posted): + """GitHub 側を取得できなければ申告を採用する。取得失敗で止めない。""" + _seed_state(tmp_dir) + posted(None) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_missing_review_url_is_treated_as_unavailable(tmp_dir, state_mod, monkeypatch): + """投稿先の参照が無ければ、突き合わせる相手を決められないので申告を採用する。""" + _seed_state(tmp_dir) + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("参照が無いのに GitHub を呼んでいる"), + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, review_url=None))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +# ---------------- 件数の取得 ---------------- + +def test_posted_count_reads_the_review_id_from_the_url(state_mod, monkeypatch): + calls: list[list[str]] = [] + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: calls.append(list(cmd)) or "2", + ) + + count = state_mod._posted_comment_count("o/r", PR, REVIEW_URL) + + assert count == 2 + assert calls and "repos/o/r/pulls/4242/reviews/4961230016/comments" in calls[0] + + +def test_posted_count_is_none_when_the_url_has_no_review_id(state_mod, monkeypatch): + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("識別子が無いのに GitHub を呼んでいる"), + ) + + assert state_mod._posted_comment_count("o/r", PR, "https://example.test/") is None + + +def test_posted_count_is_none_when_the_api_fails(state_mod, monkeypatch): + def boom(cmd, check=True): + raise RuntimeError("network") + + monkeypatch.setattr(state_mod, "_sh", boom) + + assert state_mod._posted_comment_count("o/r", PR, REVIEW_URL) is None diff --git a/plugins/ndf-shared/skills/cross-refactoring/SKILL.md b/plugins/ndf-shared/skills/cross-refactoring/SKILL.md index c057c122..ac855f65 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/SKILL.md +++ b/plugins/ndf-shared/skills/cross-refactoring/SKILL.md @@ -193,11 +193,17 @@ export CROSS_REFACTORING_TMP_DIR="$TMP_DIR" while :; do # 提案ラウンドの繰り返し rf_eval start-round "$ID" || break # 終了コード 1 = 繰り返し終了 + # **提案の直前に読み取り用を同期する。** 前ラウンドの取り消しで HEAD が進んで + # いるため、同期しないと**消えたコードに対する提案**が返る（実測: 取り消しで + # 消えた関数へ 2 件）。HEAD が変わっていなければ何も起きない。 + "$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done + # 提案の所要は参加ランタイムと回線状況で振れる（実測 90〜285 秒）。既定の + # 打ち切りに任せず、明示する。 "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 rf merge-proposals "$ID" || break # 終了コード 2 = 採用 0 件 "$SCRIPTS/launch-cli.sh" "$IMPL" apply "$ID" "$ROUND" @@ -213,7 +219,7 @@ while :; do # 提案ラウンドの繰り返 "$SCRIPTS/launch-cli.sh" "$r" review "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$REVIEWERS_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-review-r$ROUND" + --stem-template "{agent}-review-r$ROUND" --timeout 900 rf judge-review "$ID" "$ROUND"; rc=$? [ $rc -eq 0 ] && break # 2 者とも承認 [ $rc -eq 3 ] && continue # 形式不正 — 差し戻して再レビュー @@ -271,6 +277,7 @@ done | 結果ファイルの申告を検証の材料にする | 実装担当は報告する側。JSON を書き換えるだけで通る検査は機械検証ではない | | レビューの指摘に項目 ID を付けない | 同上。差し戻して再レビューになる | | `git push --force` / `--no-verify` を使う | 他者の作業を消す。検証を飛ばす | +| 実装担当のコミットでフックの通し方を決めない | 生成物の同期を検査するリポジトリでは、同期の禁止と両立せずコミットを作れなくなる。迂回してよい手段を 1 つ定める | | 提案とレビューにホストを混ぜる | 実装者と評価者が同一モデルになりうる。初期化時に検査して失敗させている | | kiro を既定モデルのまま計測する | `auto` は実際に選ばれたモデルを取得できない | | 提案フェーズでコードを直す | 提案は読むだけ。直すのは実装担当 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 45207583..fb751789 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 @@ -182,11 +182,12 @@ claude 17 本 / codex 1 メソッド / kiro 0 本と揃わなかった。最後 ## Step 2: 提案 ```bash +"$SCRIPTS/prepare-worktrees.sh" "$ID" sync "$(git -C "$WORK" rev-parse HEAD)" for a in $RUNTIMES; do "$SCRIPTS/launch-cli.sh" "$a" propose "$ID" "$ROUND" done "$LIB/monitor.py" "$ID" --agents "$RUNTIMES_CSV" --tmp-dir "$TMP_DIR" \ - --stem-template "{agent}-propose-rf{id}-r$ROUND" + --stem-template "{agent}-propose-rf{id}-r$ROUND" --timeout 900 ``` 3 CLI を並列で起動し、同一のプロンプトで提案させる。**提案フェーズにホストは現れない** @@ -195,6 +196,24 @@ done 提出形式は [prompts/propose.md](../prompts/propose.md) にある。 +### 提案の直前に読み取り用を同期する + +**同期が要るのは HEAD が進んだときであって、特定のフェーズの後ではない。** +適用と修正の直後だけを同期していると、取り消しで進んだ HEAD が読み取り用へ +届かない。実測では、ラウンドを全件取り消した次の提案で、**取り消しによって +消えた関数**に対する提案が 2 件返った。統合は対象の実在を検査しないため、 +そのまま採用され、適用で必ず失敗する。 + +提案の直前に同期しておけば、どのフェーズを経ていても読み取り用は最新になる。 +HEAD が変わっていなければ何も起きないので、重ねて呼んでも無駄がない。 + +### 打ち切りまでの時間を明示する + +提案の所要はランタイムと回線状況で振れる（実測 90〜285 秒）。既定の打ち切りに +任せると、分析そのものは進んでいるのに時間切れで結果を捨てることがある。 +結果ファイルには `idle_seconds` が残るので、**止まっていたのか間に合わなかったのか**は +後から読める。 + ### 結果ファイル名にラウンド番号を入れる CLI の起動時に同名の結果ファイルを消すため、**提案の結果ファイル名にもラウンド番号が 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 4609b02a..c5d18be3 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 @@ -199,6 +199,32 @@ Pull Request に残る。**都合の悪い変更を申告しないだけで検 終わると、Pull Request 側には未検証の差分が残るのに、次の実行は処理済みガードで 素通りしてしまう。印があれば、次の実行が判定より先に再送信する。 +### 実装担当のコミットはフックの検査を通さなくてよい + +**生成物の同期を pre-commit で検査するリポジトリでは、そのままだとコミットを作れない。** +実装担当は範囲内だけを変更するので生成物は必ず古くなり、同期は禁じられている。 + +公開は進行側が検証を通してから行うため、**実装担当のコミット時点で生成物が古いのは +設計どおり**である。そこで、フックが原因でコミットできないときは +`git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外させる。 + +- 禁止は `--no-verify` だけを名指しにしない。手段の名前で書き分けると、同じことが + 別の名前で起こる。実測では、同じ実装担当が適用フェーズではフックを迂回し、 + 修正フェーズでは迂回せず 0 コミットで終えた +- **直した内容は必ずコミットさせる。** 作業ツリーに置いたまま終えると、検証を + 受けていない変更として `merge-apply` / `merge-fix` が捨てる + +### 取り込みの前に置き土産を捨てる + +`merge-apply` と `merge-fix` は、実装担当が残した未コミットの変更を捨ててから +結果を読む。コミットされなかった変更はどの検証も受けておらず、公開する道が無い。 + +残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正フェーズで +コミットを作れなかった実装担当が直しかけの差分を置いたまま終え、続く `merge-fix` が +「修正 0 件」として先へ進むこともできなくなった。 + +制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、消えない。 + ### コミットトレーラーの形式 適用と修正のコミットメッセージ本文の末尾に、**git のトレーラー形式**で実行主体を残す。 diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md index bfaf491e..ec9c8b94 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/apply.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います。 ここで公開すると、検証を通っていない変更が Pull Request に残ります - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$RF_SCOPE`）の外にあるファイルを 1 つも変更しない。** 範囲外を触った コミットを含む項目は検証で失敗し、取り消されます diff --git a/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md b/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md index a73f3c3e..bb2180a9 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md +++ b/plugins/ndf-shared/skills/cross-refactoring/prompts/fix.md @@ -56,6 +56,13 @@ Impl-Model: $RF_MODEL - **push しない。** 公開するのは進行側だけで、**検証を通した後**に行います - **`git push --force` と `--no-verify` を使わない** +- **コミット時のフックが生成物の同期を求めても、同期はしない。** + 同期は進行側が公開の直前に行うため、ここでは生成物が古いのが正しい状態です。 + フックが原因でコミットできないときは + `git -c core.hooksPath=/dev/null commit ...` で**そのコミットだけ**フックを外します。 + 公開するのは進行側だけなので、検証を受けていない変更が Pull Request へ出ることはありません +- **直した内容は必ずコミットする。** 作業ツリーに置いたまま終えると、 + 検証を受けていない変更として捨てられます - 作業ディレクトリの外を触らない - **対象範囲（`$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 1a224c66..2aa3c0d2 100755 --- a/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py +++ b/plugins/ndf-shared/skills/cross-refactoring/scripts/refactor.py @@ -1099,6 +1099,7 @@ def cmd_merge_apply(args: argparse.Namespace) -> None: path, state = _load(args.id) entry = _round(state, args.round) if not args.dry_run: + _discard_impl_leftovers(state, state["worktrees"]["work"]) _resume_incomplete_apply(path, state, entry) # **叩き直しても同じ判定を返す。** 取り込み済みで再実行すると、前回作った @@ -1675,6 +1676,7 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: """Step 6 — 修正結果を取り込み、修正ラウンドを 1 つ進める。""" path, state = _load(args.id) entry = _round(state, args.round) + _discard_impl_leftovers(state, state["worktrees"]["work"]) _flush_pending_push(path, state, entry) impl = entry["impl"] result = _result_path(state, impl, stem_for(impl, "fix", state["id"], args.round)) @@ -2018,10 +2020,18 @@ def _reported_shas(reported: Any) -> list[str]: return shas -def _git_out(work: str, args: list[str]) -> Optional[str]: - """`git` を実行して標準出力を返す。失敗したら `None`。""" +def _git_out(work: str, args: list[str], strip: bool = True) -> Optional[str]: + """`git` を実行して標準出力を返す。失敗したら `None`。 + + **固定幅で読む出力には `strip=False` を渡す。** `git status --porcelain` の + 状態コードは未 stage の変更で ` M` と先頭が空白になるため、`strip()` すると + 1 行目だけ 1 文字ずれ、切り出したパスの先頭が欠ける。欠けたパスは + `git add` で `pathspec ... did not match any files` になり、同期が止まる。 + """ r = subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) - return r.stdout.strip() if r.returncode == 0 else None + if r.returncode != 0: + return None + return r.stdout.strip() if strip else r.stdout.rstrip("\n") def commits_in_range(work: str, base: Optional[str], head: str) -> Optional[list[str]]: @@ -2524,7 +2534,8 @@ def _worktree_changes(work: str) -> dict[str, str]: # `core.quotePath` の既定（true）では、非 ASCII を含むパスが `"` で囲まれ # `\343` の形へエスケープされる。そのまま `git add` へ渡すと見つからない。 out = _git_out( - work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"] + work, ["-c", "core.quotePath=false", "status", "--porcelain", "-uall"], + strip=False, ) changes: dict[str, str] = {} for line in (out or "").splitlines(): @@ -2579,6 +2590,33 @@ def _discard_worktree_changes(work: str) -> None: subprocess.run(["git", *args], cwd=work, capture_output=True, text=True) +def _discard_impl_leftovers(state: dict[str, Any], work: str) -> None: + """実装担当が残した未コミットの変更を捨てる。取り込みの前に呼ぶ。 + + **公開は進行側が検証を通してから行う**ので、コミットされなかった変更は + どの検証も受けていない。Pull Request へ出す道が無い以上、残す意味がない。 + + 残したまま進むと、push の直前の清浄性の検査で中断する。実測では、修正 + フェーズでコミットを作れなかった実装担当が直しかけの差分を置いたまま終え、 + 続く `merge-fix` が「修正 0 件」として先へ進むこともできなくなった。 + + 制御用ディレクトリ（状態・結果・ログ）は無視の設定で守られており、 + `git clean` に `-x` を付けないため消えない。 + """ + if not pathlib.Path(work).is_dir(): + return + dirty = _dirty_paths(state, work) + if not dirty: + return + shown = "、".join(dirty[:5]) + more = f" ほか {len(dirty) - 5} 件" if len(dirty) > 5 else "" + _discard_worktree_changes(work) + info( + f"🧹 コミットされなかった変更を捨てました（{shown}{more}）。" + "検証を受けていないため公開しません" + ) + + def _require_clean_worktree(state: dict[str, Any], work: str) -> None: """同期の前に作業ツリーが綺麗であることを求める。汚れていたら中断する。 @@ -2643,8 +2681,17 @@ def _sync_generated(state: dict[str, Any]) -> None: produced = _dirty_paths(state, work) if not produced: return - _sh(["git", "add", "--", *produced], cwd=work) - _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + # **後段で落ちたときも差分を残さない。** `git add` / `git commit` の失敗で + # 作業ツリーを汚したまま中断すると、次の実行は `_require_clean_worktree` で + # 必ず止まり、`pending_push` の再試行が永久に進まない。捨ててよい根拠は + # 同期コマンド自身が失敗したときと同じで、着手前が綺麗だったことを + # 確認済みだからである。 + try: + _sh(["git", "add", "--", *produced], cwd=work) + _sh(["git", "commit", "-m", SYNC_COMMIT_MESSAGE], cwd=work) + except SystemExit: + _discard_worktree_changes(work) + raise info(f"🔧 生成物を同期しました（{command} / {len(produced)} ファイル）") 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 41928945..5101513d 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 @@ -98,7 +98,7 @@ def test_deferred_entry_records_the_reason(refactor, tmp_path, env_tmp_dir, no_g def _history(refactor, monkeypatch, newest_first): """`git rev-list HEAD` の結果（新しい順）と SHA 解決を差し替える。""" - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:1] == ["rev-list"]: return "\n".join(newest_first) if args[:2] == ["rev-parse", "--verify"]: @@ -232,7 +232,7 @@ def _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, claimed, # 未申告コミットの判定がこの解決を通るため。 monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr( @@ -406,7 +406,7 @@ def test_broken_fix_result_does_not_crash( }) monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: set()) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: []) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD") monkeypatch.setattr( refactor, "collect_commit_facts", lambda work, shas, rng, cmd, branch, timeout=None: [], @@ -436,7 +436,7 @@ def test_merge_fix_uses_the_recorded_range(refactor, tmp_path, env_tmp_dir, monk ) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: (args[-1].replace("^{commit}", "") + lambda work, args, **_kw: (args[-1].replace("^{commit}", "") if args[:2] == ["rev-parse", "--verify"] else "HEAD_NOW"), ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -452,7 +452,7 @@ def test_merge_fix_fails_when_the_range_cannot_be_determined( ): state_path = _prepare_fix(refactor, tmp_path, env_tmp_dir, monkeypatch, ["PRRT_a"]) monkeypatch.setattr(refactor, "commits_in_range", lambda work, base, head: None) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "HEAD_NOW") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "HEAD_NOW") monkeypatch.setattr(refactor, "resolved_threads_on_github", lambda repo, pr: {"PRRT_a"}) with pytest.raises(SystemExit) as e: @@ -470,7 +470,7 @@ def test_merge_fix_rejects_unreported_commits( refactor, "commits_in_range", lambda work, base, head: ["sneaky", "fix111"]) monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" + lambda work, args, **_kw: args[-1].replace("^{commit}", "") if args[0] == "rev-parse" else "HEAD_NOW", ) monkeypatch.setattr(refactor, "resolved_threads_on_github", @@ -691,7 +691,7 @@ def test_merge_fix_is_idempotent_after_a_revert( calls.clear() monkeypatch.setattr( refactor, "_git_out", - lambda work, args: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] + lambda work, args, **_kw: ("HEAD_AFTER_REVERT" if args[:1] == ["rev-parse"] else args[-1].replace("^{commit}", "")), ) refactor.cmd_merge_fix(args) @@ -799,7 +799,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_judge_review.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_judge_review.py index 0105fa34..a3121ef2 100644 --- a/plugins/ndf-shared/skills/cross-refactoring/tests/test_judge_review.py +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_judge_review.py @@ -273,7 +273,7 @@ def test_judge_command_records_the_fix_base(refactor, tmp_path, env_tmp_dir, mon """修正コミットの実在を確かめるため、変更要求の時点の HEAD を残すこと。""" state_path = _state(tmp_path) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "FIX_BASE") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "FIX_BASE") write_result(state_path, "gemini-review-r1", review("REQUEST_CHANGES", [finding()])) write_result(state_path, "kiro-review-r1", review()) 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 28aa9cb7..743712c9 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 @@ -5,6 +5,7 @@ """ from __future__ import annotations +import pathlib import subprocess import pytest @@ -149,7 +150,7 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): """結果ファイルではなく実際のコミットメッセージから読む。""" monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "Item-Id: R1-001\nRound: 1\n" + lambda work, args, **_kw: "Item-Id: R1-001\nRound: 1\n" "Impl-Runtime: codex\nImpl-Model: gpt-5.5", ) assert refactor.commit_trailers("/w", "abc") == { @@ -159,31 +160,31 @@ def test_commit_trailers_are_read_from_git(refactor, monkeypatch): def test_commit_trailers_are_empty_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commit_trailers("/w", "abc") == {} def test_diff_lines_come_from_numstat(refactor, monkeypatch): monkeypatch.setattr( refactor, "_git_out", - lambda work, args: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", + lambda work, args, **_kw: "10\t5\tsrc/a.py\n3\t2\tsrc/b.py\n-\t-\tbin.png", ) assert refactor.commit_diff_lines("/w", "abc") == 20 def test_touches_tests_detects_test_paths(refactor, monkeypatch): monkeypatch.setattr(refactor, "_git_out", - lambda work, args: "src/a.py\ntests/test_a.py") + lambda work, args, **_kw: "src/a.py\ntests/test_a.py") assert refactor.commit_touches_tests("/w", "abc") is True - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "src/a.py") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "src/a.py") assert refactor.commit_touches_tests("/w", "abc") is False def test_commits_in_range_uses_rev_list(refactor, monkeypatch): calls = [] - def fake(work, args): + def fake(work, args, **_kw): calls.append(args) return "aaa\nbbb" @@ -198,7 +199,7 @@ def test_commits_in_range_is_none_without_base(refactor): def test_commits_in_range_is_none_when_git_fails(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) assert refactor.commits_in_range("/w", "base", "head") is None @@ -206,7 +207,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): """テストは実際に走らせる。実行後は必ず元のブランチへ戻す。""" git_calls = [] monkeypatch.setattr( - refactor, "_git_out", lambda work, args: git_calls.append(args) or "") + refactor, "_git_out", lambda work, args, **_kw: git_calls.append(args) or "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: (git_calls.append(a[0]) if isinstance(a[0], list) else None) @@ -220,7 +221,7 @@ def test_run_test_at_checks_out_and_restores(refactor, monkeypatch): def test_run_test_at_reports_failure(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr( refactor.subprocess, "run", lambda *a, **kw: subprocess.CompletedProcess(a[0], 0, "", ""), @@ -239,7 +240,7 @@ def fake_run(cmd, **kw): return subprocess.CompletedProcess(cmd, 0, "", "") raise OSError("テスト実行が壊れた") - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "") monkeypatch.setattr(refactor.subprocess, "run", fake_run) with pytest.raises(OSError): refactor.run_test_at("/w", "abc", "pytest -q", "main") @@ -247,13 +248,13 @@ def fake_run(cmd, **kw): def test_collect_facts_marks_unknown_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: None) + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: None) facts = refactor.collect_commit_facts("/w", ["ghost"], {"aaa"}, "true", "main") assert facts == [{"sha": "ghost", "exists": False}] def test_collect_facts_marks_out_of_range_sha_as_missing(refactor, monkeypatch): - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "zzz") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "zzz") facts = refactor.collect_commit_facts("/w", ["zzz"], {"aaa"}, "true", "main") assert facts[0]["exists"] is False @@ -289,7 +290,7 @@ def _set(mapping, in_range=None): # SHA をそのまま返す形にしておく monkeypatch.setattr( refactor, "_git_out", - lambda work, args: args[-1].replace("^{commit}", ""), + lambda work, args, **_kw: args[-1].replace("^{commit}", ""), ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -383,11 +384,21 @@ 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, sync_dirty=False): +def _drop_env(refactor, monkeypatch, revert_rc=0, pick_rc=0, sync_dirty=False, + leftover=""): """取り消しと積み直しを実際には走らせず、順序と引数を記録する。 `git rev-parse HEAD` は**直前に積み直したコミット**に応じた値を返す。 積み直しで SHA が変わることを、状態の更新まで含めて確かめられるようにする。 + + 作業ツリーの状態は 3 段階で返す。取り込みの前に実装担当の置き土産を捨てる + ため、同期の前後だけでは足りない。 + + | 呼ばれる場面 | 返す値 | + | --- | --- | + | 取り込みの前（置き土産の確認） | `leftover` | + | 同期の前（清浄性の検査） | `sync_dirty[0]` | + | 同期の後（生成された差分） | `sync_dirty[1]` | """ calls: list[list[str]] = [] picked: list[str] = [] @@ -407,7 +418,7 @@ def fake_run(cmd, **kwargs): picked.append(cmd[-1]) return subprocess.CompletedProcess(cmd, rc, "", "conflict" if rc else "") - def fake_git_out(work, args): + def fake_git_out(work, args, **_kw): if args[:2] == ["rev-parse", "--verify"]: return args[-1].replace("^{commit}", "") if args == ["rev-parse", "HEAD"]: @@ -415,13 +426,13 @@ def fake_git_out(work, args): 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 len(statuses) == 1: + return leftover if sync_dirty is False: return "" before, after = sync_dirty - return before if len(statuses) == 1 else after + return before if len(statuses) == 2 else after return "HEAD_BEFORE" monkeypatch.setattr(refactor.subprocess, "run", fake_run) @@ -842,7 +853,7 @@ def test_apply_base_is_recorded_by_the_orchestrator( "durations": {}, "reviews": [], }]) env_tmp_dir(state_path) - monkeypatch.setattr(refactor, "_git_out", lambda work, args: "BASE_HEAD") + monkeypatch.setattr(refactor, "_git_out", lambda work, args, **_kw: "BASE_HEAD") for rt in ("codex", "gemini", "kiro"): write_result(state_path, f"{rt}-propose-rf130", {"items": []}) with pytest.raises(SystemExit): @@ -1001,7 +1012,7 @@ def test_short_and_full_sha_are_seen_as_the_same_commit( # 短縮 SHA も完全 SHA も同じコミットへ解決される monkeypatch.setattr( refactor, "_git_out", - lambda work, args: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", + lambda work, args, **_kw: full if args[:2] == ["rev-parse", "--verify"] else "HEAD", ) monkeypatch.setattr( refactor, "collect_commit_facts", @@ -1256,9 +1267,16 @@ def test_deferring_is_idempotent(refactor, tmp_path, env_tmp_dir, monkeypatch, g # ---------- push の直前に生成物を同期する ---------- def _sync_state(tmp_path, env_tmp_dir, git_facts, command="make build"): + """同期コマンドを持つ状態を作る。 + + 書き込み用の作業ディレクトリを実在させる。取り込みの前に置き土産を確認する + 経路は、ディレクトリが無ければ何もせずに戻るため、実在しないと + `_drop_env` の 3 段階（置き土産 / 同期前 / 同期後）が 1 つずれる。 + """ state_path = _two_item_apply(tmp_path, env_tmp_dir, git_facts) state = read_state(state_path) state["sync_command"] = command + pathlib.Path(state["worktrees"]["work"]).mkdir(parents=True, exist_ok=True) state_path.write_text(__import__("json").dumps(state), encoding="utf-8") return state_path @@ -1517,7 +1535,7 @@ def test_status_disables_path_quoting(refactor, monkeypatch): seen: list[list[str]] = [] monkeypatch.setattr( refactor, "_git_out", - lambda work, args: seen.append(list(args)) or " M plugins/日本語/a.py", + lambda work, args, **_kw: 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"] diff --git a/plugins/ndf-shared/skills/cross-refactoring/tests/test_sync_generated.py b/plugins/ndf-shared/skills/cross-refactoring/tests/test_sync_generated.py new file mode 100644 index 00000000..008259ac --- /dev/null +++ b/plugins/ndf-shared/skills/cross-refactoring/tests/test_sync_generated.py @@ -0,0 +1,223 @@ +"""生成物の同期と、実装担当が残した未コミット変更の扱いを**実際の git** で確かめる。 + +同期は `--sync-command` を持つリポジトリで push の直前に走る、進行側の責務である。 +ここが落ちると取り消しを Pull Request へ反映できないため、進行そのものが止まる。 + +| 確かめること | なぜ | +| --- | --- | +| 変更のパスを 1 文字も欠かさず拾う | `git status --porcelain` は固定幅。先頭の空白を削ると 1 行目がずれる | +| 同期の後段で落ちても差分を残さない | 残すと次の実行が清浄性の検査で必ず止まる | +| 実装担当の置き土産を捨ててから取り込む | 検証を受けていない変更なので公開しない。止まる理由にもしない | +""" +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import make_state, read_state, write_result + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git が必要") + + +def _git(*args, cwd): + return subprocess.run(["git", *args], cwd=cwd, capture_output=True, + text=True, check=True) + + +def _commit(repo, message): + _git("add", "-A", cwd=repo) + _git("-c", "user.email=t@e.st", "-c", "user.name=test", + "commit", "-qm", message, cwd=repo) + return _git("rev-parse", "HEAD", cwd=repo).stdout.strip() + + +def _make_work(tmp_path): + """`work` を本物のリポジトリとして作り、状態ファイルを添えて返す。""" + work = tmp_path / "work" + (work / "generated").mkdir(parents=True) + _git("init", "-q", "-b", "main", str(work), cwd=tmp_path) + (work / "src.py").write_text("x = 1\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 1\n", encoding="utf-8") + _commit(work, "init") + return work + + +def _state_with_sync(tmp_path, work, command="true"): + return make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + sync_command=command, + ) + + +# ---------- 変更のパスを 1 文字も欠かさず拾う ---------- + +def test_unstaged_change_on_first_line_keeps_full_path(refactor, tmp_path): + """先頭が空白の状態コード（` M`）でも、パスの先頭文字が消えない。 + + `git status --porcelain` は「状態 2 文字 + 空白 + パス」の固定幅で、 + 未 stage の変更は 1 文字目が空白になる。出力全体を `strip()` してから + 固定幅で切り出すと、**1 行目だけ**パスが 1 文字短くなる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + + changes = refactor._worktree_changes(str(work)) + + assert "src.py" in changes + + +def test_every_changed_path_is_addable(refactor, tmp_path): + """拾ったパスは、そのまま `git add` に渡して通る。""" + work = _make_work(tmp_path) + (work / "src.py").write_text("x = 2\n", encoding="utf-8") + (work / "generated" / "out.py").write_text("x = 2\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + paths = refactor._dirty_paths(state, str(work)) + + assert paths == ["generated/out.py", "src.py"] + _git("add", "--", *paths, cwd=work) + + +# ---------- 同期コミット ---------- + +def test_sync_commits_generated_changes(refactor, tmp_path): + """同期コマンドが作った差分は、進行側のコミットとして積まれる。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + subject = _git("log", "-1", "--format=%s", cwd=work).stdout.strip() + assert subject == refactor.SYNC_COMMIT_MESSAGE.splitlines()[0] + + +def test_sync_without_changes_makes_no_commit(refactor, tmp_path): + """差分が出ない同期はコミットを作らない。""" + work = _make_work(tmp_path) + before = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state = read_state(_state_with_sync(tmp_path, work, command="true")) + + refactor._sync_generated(state) + + assert _git("rev-parse", "HEAD", cwd=work).stdout.strip() == before + + +# ---------- 同期の後段で落ちたとき ---------- + +def test_failure_after_sync_discards_produced_changes(refactor, tmp_path, monkeypatch): + """`git add` / `git commit` が落ちても、同期が作った差分を残さない。 + + 残すと次の実行は清浄性の検査で必ず止まり、保留中の push を再試行できない。 + """ + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py")) + monkeypatch.setattr(refactor, "_sh", + lambda *a, **k: refactor.die("commit に失敗しました")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_failed_sync_command_discards_partial_changes(refactor, tmp_path): + """同期コマンド自身が落ちたときも、途中まで書き換えた差分を残さない。""" + work = _make_work(tmp_path) + state = read_state(_state_with_sync( + tmp_path, work, command="printf 'x = 2\\n' > generated/out.py; exit 1")) + + with pytest.raises(SystemExit): + refactor._sync_generated(state) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +# ---------- 実装担当が残した未コミット変更 ---------- + +def test_leftover_changes_are_discarded_before_merge(refactor, tmp_path): + """実装担当が残した未コミット変更は、取り込みの前に捨てる。 + + 公開は進行側が検証を通してから行うので、コミットされなかった変更は + **検証を受けていない**。残したまま進むと、清浄性の検査で進行が止まる。 + """ + work = _make_work(tmp_path) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(_state_with_sync(tmp_path, work)) + + refactor._discard_impl_leftovers(state, str(work)) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert (work / "src.py").read_text(encoding="utf-8") == "x = 1\n" + + +def test_discard_keeps_control_directory(refactor, tmp_path): + """制御用ディレクトリ（状態・結果・ログ）は捨てない。""" + work = _make_work(tmp_path) + control = work / ".cross_refactoring" + control.mkdir() + (control / "keep.json").write_text("{}", encoding="utf-8") + (work / ".gitignore").write_text(".cross_refactoring/\n", encoding="utf-8") + _commit(work, "ignore control dir") + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + state = read_state(make_state( + tmp_path, + worktrees={"work": str(work)}, + tmp_dir=str(control), + )) + + refactor._discard_impl_leftovers(state, str(work)) + + assert (control / "keep.json").exists() + assert _git("status", "--porcelain", cwd=work).stdout == "" + + +def test_merge_fix_continues_when_impl_left_changes( + refactor, tmp_path, env_tmp_dir, monkeypatch +): + """修正フェーズの置き土産があっても、`merge-fix` は中断しない。 + + 実装担当がコミットを作れずに終えると作業ツリーへ差分が残る。これを理由に + 止めると、修正 0 件として先へ進むこともできなくなる。 + """ + work = _make_work(tmp_path) + head = _git("rev-parse", "HEAD", cwd=work).stdout.strip() + state_path = make_state( + tmp_path, + worktrees={"work": str(work), "codex": str(tmp_path / "codex"), + "gemini": str(tmp_path / "gemini"), "kiro": str(tmp_path / "kiro")}, + rounds=[{ + "round": 1, "impl": "codex", "impl_model": None, + "reviewers": ["gemini", "kiro"], "reviewer_models": {}, + "items": ["R1-001"], "adopted": 1, "proposed": 1, "merged": 1, + "apply": {"merged_at": "2026-08-18T00:00:00", "applied": ["R1-001"], + "failed": []}, + "apply_base_sha": head, "apply_progress": [], "drops": [], + "reviews": [], "fix_rounds": 0, "fix_attempts": 1, + "fix_base_sha": head, "deferred": [], "durations": {}, + "proposal_keys": [], "pending_drop": [], "pending_push": False, + "started_at": "2026-08-18T00:00:00", + }], + items=[{"item_id": "R1-001", "round": 1, "path": "src.py", + "symbol": "f", "smell": "long_method", "technique": "extract_method", + "severity": "major", "rationale": "", "plan": "", "test_gap": False, + "estimated_diff_lines": 10, "proposed_by": ["codex"], + "status": "applied", "commits": []}], + ) + env_tmp_dir(state_path) + monkeypatch.setattr(refactor, "_push_head", lambda state: None) + write_result(state_path, "codex-fix-r1", + {"resolved_thread_ids": [], "unresolved": [], "commits": []}) + (work / "src.py").write_text("直しかけ\n", encoding="utf-8") + + refactor.cmd_merge_fix(type("A", (), {"id": 130, "round": 1})()) + + assert _git("status", "--porcelain", cwd=work).stdout == "" + assert read_state(state_path)["rounds"][0]["fix_rounds"] == 1 diff --git a/plugins/ndf-shared/skills/cross-review/SKILL.md b/plugins/ndf-shared/skills/cross-review/SKILL.md index f2eeb2f1..cc8b704f 100644 --- a/plugins/ndf-shared/skills/cross-review/SKILL.md +++ b/plugins/ndf-shared/skills/cross-review/SKILL.md @@ -41,6 +41,7 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲 | 観点 | 方針 | |---|---| | レビュー投稿 | **AI 自身が `gh api` で PR に直接投稿**。メインはペイロードを保持しない | +| 投稿の確認 | **申告されたコメント数を GitHub 側と突き合わせる**。投稿が届いていなければ中断する（取得できない場合は申告を採用） | | 修正 | **必ずサブエージェント (`general-purpose`) で実行**。メイン context に diff は載せない | | ユーザ問い合わせ | 自動判断を最大化（`critical`/`major`/`minor` は自動修正、ループ中の `nit` は deferred） | | 取りこぼし防止 | **ループ終了時（approved / max_rounds / oscillation / error いずれも）に最終スイープを必須実行**。`/ndf:fix` を再実行し、残った open review thread（最終 APPROVE ラウンドの minor/nit インラインコメント含む）を **全て解消**。修正可能なものは修正 + push、判断保留 nit も reply + resolveReviewThread して **open thread 0 で終了** | @@ -394,6 +395,8 @@ pint / larastan / test / build などは **中断** を原則とする。 - ❌ **修正をメインセッション内で行う** — context が一気に膨れる。必ずサブエージェント - ❌ **AI に Markdown だけ返させる** — メインがパース・投稿する設計は禁物。AI 直接投稿 +- ❌ **result.json の申告だけで判定を進める** — 投稿が失敗しても件数は残る。GitHub 側の + 実数と突き合わせないと、修正担当が読むべき指摘が存在しないまま収束する - ❌ **nit を都度ユーザに問う** — ループ中は deferred 記録のみ。最終スイープ (Step 7.5) で Resolve - ❌ **未解決スレッドを残したまま終了する** — approved/max_rounds 等いずれの終了経路でも Step 7.5 の最終スイープを必ず実行し、open review thread 0 で終える。特に **最終 APPROVE diff --git a/plugins/ndf-shared/skills/cross-review/docs/01-state-and-review.md b/plugins/ndf-shared/skills/cross-review/docs/01-state-and-review.md index a1e088fd..6e782731 100644 --- a/plugins/ndf-shared/skills/cross-review/docs/01-state-and-review.md +++ b/plugins/ndf-shared/skills/cross-review/docs/01-state-and-review.md @@ -216,6 +216,25 @@ launcher が生成するプロンプトに以下を強制している: `state.rounds[-1].` に `intent / posted_as / comments / review_url / by_severity` を分離保存する。 +#### 申告されたコメント数を GitHub 側と突き合わせる + +投稿は **AI 自身が `gh api` で行う**ため、失敗しても結果ファイルの申告だけは残る。 +申告のまま進むと、修正担当が読むべき指摘が GitHub 上に存在しないまま収束判定まで走る。 +実測では、2 件の申告に対しスレッドが 1 つも作られていなかった。 + +`read-result` は申告が 1 件以上のとき、`review_url` の識別子から +`repos//pulls//reviews//comments` を数えて突き合わせる。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 突き合わせる相手がいない | +| n 件 | n 件以上 | 採用する。人の追記など申告以外の経路で増えうる | +| n 件 | n 件未満 | **中断する。** 投稿が届いていない | +| n 件 | 取得できない | 申告を採用し、確認できなかったことを出力へ残す | + +**「取得できなかった」と「0 件」を区別する。** 取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 + ## Step 3: 判定（intent ベース） ```bash diff --git a/plugins/ndf-shared/skills/cross-review/scripts/state.py b/plugins/ndf-shared/skills/cross-review/scripts/state.py index 9e91f11f..c001d2aa 100755 --- a/plugins/ndf-shared/skills/cross-review/scripts/state.py +++ b/plugins/ndf-shared/skills/cross-review/scripts/state.py @@ -29,6 +29,7 @@ import json import os import pathlib +import re import shlex import subprocess import sys @@ -967,6 +968,44 @@ def cmd_start_round(args: argparse.Namespace) -> None: print(f"ROTATE_AFTER={st['rotate_after']}") +def _as_count(value: object) -> int: + """申告された件数を整数として読む。読めない値は 0 として扱う。 + + 相手は LLM なので、文字列や `null` が入ることがある。読めない申告を + 「件数あり」と見なすと、突き合わせる相手が決まらないまま中断してしまう。 + """ + try: + return max(0, int(value)) # type: ignore[arg-type] + except (TypeError, ValueError): + return 0 + + +def _posted_comment_count(repo: str, pr: int, review_url: str | None) -> int | None: + """レビューに実際にぶら下がっているインラインコメントの数。 + + 取得できなければ `None` を返す。**「取得できなかった」と「0 件」を区別する。** + 取得の失敗で中断すると、GitHub 側の一時的な不調でループが止まる。 + + 投稿は AI 自身が `gh api` で行うため、失敗しても結果ファイルの申告だけは残る。 + 数え直す先は、申告された `review_url` の末尾にある識別子から決める。 + """ + if not repo or not review_url: + return None + m = re.search(r"pullrequestreview-(\d+)", str(review_url)) + if not m: + return None + try: + out = _sh( + ["gh", "api", f"repos/{repo}/pulls/{pr}/reviews/{m.group(1)}/comments", + "--paginate", "--jq", "length"], + check=False, + ) + except Exception: + return None + counts = [int(line) for line in str(out).split() if line.strip().isdigit()] + return sum(counts) if counts else None + + def cmd_read_result(args: argparse.Namespace) -> None: """Step 2.5 — codex/gemini の result.json を state にマージ。""" agent = args.agent @@ -1007,6 +1046,25 @@ def cmd_read_result(args: argparse.Namespace) -> None: st = _load(pr) if not st.get("rounds"): die(f"{agent}: state.rounds が空。`state.py start-round` を先に呼んでください") + + # **申告を GitHub 側と突き合わせる。** 投稿は AI 自身が行うので、失敗しても + # 結果ファイルには件数が残る。申告のまま進むと、修正担当が読むべき指摘が + # GitHub 上に存在しないまま収束判定まで走る（実測: 申告 2 件に対しスレッド 0）。 + declared = _as_count(comments) + if declared > 0: + actual = _posted_comment_count(str(st.get("repo") or ""), pr, r.get("review_url")) + if actual is None: + info( + f"⚠ {agent}: 投稿されたコメント数を確認できませんでした。" + f"申告（{declared} 件）をそのまま採用します" + ) + elif actual < declared: + die( + f"{agent}: インラインコメントの申告 {declared} 件に対し、" + f"GitHub 上には {actual} 件しかありません。投稿が届いていないため" + "中断します。レビューを投稿し直してから再実行してください" + ) + st["rounds"][-1][agent] = { "intent": intent, "posted_as": posted_as, diff --git a/plugins/ndf-shared/skills/cross-review/tests/test_state_posted_comments.py b/plugins/ndf-shared/skills/cross-review/tests/test_state_posted_comments.py new file mode 100644 index 00000000..b57ffc95 --- /dev/null +++ b/plugins/ndf-shared/skills/cross-review/tests/test_state_posted_comments.py @@ -0,0 +1,191 @@ +"""申告されたインラインコメント数を、GitHub 側の実数と突き合わせる。 + +レビューの投稿は AI 自身が `gh api` で行うため、**投稿に失敗しても結果ファイルの +申告だけは残る**。申告を信じて先へ進むと、修正担当が読むべき指摘が GitHub 上に +存在しないまま収束判定まで走る。実測では 2 件の申告に対しスレッドが 1 つも +作られていなかった。 + +| 申告 | GitHub 側 | 扱い | +| --- | --- | --- | +| 0 件 | 見に行かない | 投稿が無いので突き合わせる相手がいない | +| 2 件 | 2 件 | そのまま採用する | +| 2 件 | 0 件 | 投稿が届いていないので中断する | +| 2 件 | 取得できない | 申告を採用し、確認できなかったことを残す | + +「取得できなかった」と「0 件」を混同しない。取得の失敗で止めると、GitHub 側の +一時的な不調でループが進まなくなる。 +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + +PR = 4242 +AGENT = "gemini" +REVIEW_URL = f"https://github.com/o/r/pull/{PR}#pullrequestreview-4961230016" + + +def _seed_state(tmp_dir: pathlib.Path) -> None: + state = { + "current_pr": PR, + "repo": "o/r", + "rounds": [{"round": 1, "pr": PR, "started_at": "2026-08-18T00:00:00+00:00"}], + "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _result(tmp_dir: pathlib.Path, **over) -> pathlib.Path: + payload = { + "event": "REQUEST_CHANGES", + "posted_as": "REQUEST_CHANGES", + "comments_count": 2, + "review_url": REVIEW_URL, + "by_severity": {"major": 2}, + } + payload.update(over) + rfile = tmp_dir / "result.json" + rfile.write_text(json.dumps(payload)) + return rfile + + +def _args(rfile: pathlib.Path) -> argparse.Namespace: + return argparse.Namespace(pr=PR, agent=AGENT, file=str(rfile)) + + +def _read_state(tmp_dir: pathlib.Path) -> dict: + return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text()) + + +@pytest.fixture() +def tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +@pytest.fixture() +def posted(monkeypatch, state_mod): + """GitHub 側の件数を差し替える。`None` は取得できなかったことを表す。""" + def _set(count): + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda repo, pr, review_url: count, + ) + return _set + + +def test_declared_count_matching_github_is_accepted(tmp_dir, state_mod, posted): + _seed_state(tmp_dir) + posted(2) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_declared_comments_missing_on_github_aborts(tmp_dir, state_mod, posted): + """申告があるのに GitHub 側へ届いていなければ中断する。 + + そのまま進むと、修正担当が読むべき指摘が存在しないまま収束判定まで走る。 + """ + _seed_state(tmp_dir) + posted(0) + + with pytest.raises(SystemExit) as e: + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert e.value.code == 1 + assert AGENT not in _read_state(tmp_dir)["rounds"][-1] + + +def test_partially_posted_comments_abort(tmp_dir, state_mod, posted): + """一部しか届いていない場合も中断する。取りこぼしは全件欠落と同じ扱いにする。""" + _seed_state(tmp_dir) + posted(1) + + with pytest.raises(SystemExit): + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + +def test_more_comments_on_github_is_accepted(tmp_dir, state_mod, posted): + """GitHub 側が多い分には通す。人の追記など、申告以外の経路で増えうる。""" + _seed_state(tmp_dir) + posted(3) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_zero_declared_skips_the_check(tmp_dir, state_mod, monkeypatch): + """申告 0 件なら GitHub を見に行かない。""" + _seed_state(tmp_dir) + called: list = [] + monkeypatch.setattr( + state_mod, "_posted_comment_count", + lambda *a, **k: called.append(a) or 0, + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, event="APPROVE", comments_count=0))) + + assert called == [] + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 0 + + +def test_unavailable_github_count_keeps_the_declaration(tmp_dir, state_mod, posted): + """GitHub 側を取得できなければ申告を採用する。取得失敗で止めない。""" + _seed_state(tmp_dir) + posted(None) + + state_mod.cmd_read_result(_args(_result(tmp_dir))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +def test_missing_review_url_is_treated_as_unavailable(tmp_dir, state_mod, monkeypatch): + """投稿先の参照が無ければ、突き合わせる相手を決められないので申告を採用する。""" + _seed_state(tmp_dir) + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("参照が無いのに GitHub を呼んでいる"), + ) + + state_mod.cmd_read_result(_args(_result(tmp_dir, review_url=None))) + + assert _read_state(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 + + +# ---------------- 件数の取得 ---------------- + +def test_posted_count_reads_the_review_id_from_the_url(state_mod, monkeypatch): + calls: list[list[str]] = [] + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: calls.append(list(cmd)) or "2", + ) + + count = state_mod._posted_comment_count("o/r", PR, REVIEW_URL) + + assert count == 2 + assert calls and "repos/o/r/pulls/4242/reviews/4961230016/comments" in calls[0] + + +def test_posted_count_is_none_when_the_url_has_no_review_id(state_mod, monkeypatch): + monkeypatch.setattr( + state_mod, "_sh", + lambda cmd, check=True: pytest.fail("識別子が無いのに GitHub を呼んでいる"), + ) + + assert state_mod._posted_comment_count("o/r", PR, "https://example.test/") is None + + +def test_posted_count_is_none_when_the_api_fails(state_mod, monkeypatch): + def boom(cmd, check=True): + raise RuntimeError("network") + + monkeypatch.setattr(state_mod, "_sh", boom) + + assert state_mod._posted_comment_count("o/r", PR, REVIEW_URL) is None