diff --git a/issues/issue-730-583-design.md b/issues/issue-730-583-design.md index 7de56e85..aa3b8d8d 100644 --- a/issues/issue-730-583-design.md +++ b/issues/issue-730-583-design.md @@ -112,13 +112,15 @@ | 種別 | 照合の鍵 | | --- | --- | -| `review-post` | 投稿者と、本文の先頭行の `## 🤖 cross-review \| round \| <席> \|` までの前方一致（判定の語を含めない） | +| `review-post` | 投稿者と、本文の先頭行の `## 🤖 cross-review \| round \| <席> \|` までの前方一致（判定の語を含めない）。ラウンドの開始時刻を持つときは、それ以降に出たレビューに限る | | `review-reply` | 返信先の指摘の識別子と、本文の先頭 80 文字 | | `thread-resolve` | スレッドの識別子と、すでに決着しているかどうか | | `pr-comment` | 投稿者と、本文の先頭 80 文字（ラウンドを含む） | 投稿者はどの席でも同じになる。ラウンドと席を先頭行に持たせることで、同じ投稿者の別の投稿と区別できる。 +**レビューの照合は、そのラウンドが始まった時刻（状態ファイルの `rounds[-1].started_at`）以降に出たレビューに限る。** ラウンドの番号は実行ごとに 1 から数え直すため、収束した PR へ回し直すと、ラウンドと席だけの鍵が前の実行のレビューに一致し、新しい指摘を送らずに前の実行の投稿を送れた先として記録する。開始時刻で絞っても同じ実行の中の送り直しは従来どおり見つかり、開始時刻かレビューの時刻を読めないときはラウンドと席だけの照合へ落とす。 + ### 決定 6: 投稿済みで記録なしの状態が起きなくなるため、投稿済みのレビューを探して記録だけの起動をする決めを取り下げる 担当が投稿しなくなるため、探す対象が無い。**GitHub からレビューを探す照会も、記録だけを行うプロンプトも作らない。** 止まった担当を起動し直すときは、初回と同じプロンプトをそのまま使う。 @@ -362,12 +364,48 @@ graph TD **偽の `gh` を使う形は既存のテストにある**（待ち行列の照合と巻き直しの投稿）。同じ仕掛けを使う。 -## 未確認のまま残ること +## 実測で決めた 4 件 + +設計の時点で未確認だった 4 件を、2026-09-22 に実物の `gh` で 1 度ずつ動かして決めた。対象は +この束の設計を載せた Pull Request #794（マージ済み、head `233f28ba`）である。拒まれた要求は +何も作らないため、確かめた跡は残っていない（レビューとレビューのコメントを引いて 0 件）。 + +### 差分の外を指す指摘が拒まれるときの応答の形 + +レビューの作成は**要求ごとに全件が拒まれる**。正しいインラインを混ぜても、一部だけが作られる +ことはない。終了コードは 1 である。 + +| 何を送ったか | 応答の `errors` | +| --- | --- | +| 差分に無いファイルのインライン 1 件 | `["Path could not be resolved"]` | +| 差分にあるファイルの、塊の外の行のインライン 1 件（正しいインライン 1 件と同時） | `["Line could not be resolved"]` | +| 塊の外・差分に無いファイル・塊の外の 3 件 | `["Line could not be resolved, Path could not be resolved, and Line could not be resolved"]` | +| 判定の値に知らない語 | `["Variable $event of type PullRequestReviewEvent was provided invalid value"]` | +| 基準のコミットに存在しない値 | `["The commitOID is not part of the pull request"]` | + +**退避の契機に使う語は `could not be resolved` である。** 判定の値の誤りと基準のコミットの誤りは +この語を含まないため、決定 9 のとおり失敗として残せる。 + +### 拒まれた応答から、どのインラインが原因かを 1 件ずつ特定できるか + +**特定できない。** 応答は語を `, ` と `and` でつないだ 1 つの文字列で、位置も識別子も持たない。 +拒まれた件数は語の数から読めるが、正しいインラインを混ぜると位置が対応しない。 + +**決定 9 の落とし先を採る。** 行やファイルを解決できないことを理由に拒まれた要求は、その要求の +インラインをすべて本文へ退避して送り直す。1 ラウンドの指摘は多くて 10 件前後、1 件の本文は +数百文字のため、すべてを退避しても数 KB に収まり、投稿の本文の上限（65536 文字）に対して余裕がある。 + +### すでに決着したスレッドをもう一度決着させたときの応答 + +**冪等である。** 決着の操作は成功し（終了コード 0）、決着済みであることを返す。失敗にならない。 +決定 5 の照合の鍵（スレッドの識別子と、すでに決着しているかどうか）が送信を止め損ねても、 +二重の決着が失敗にはならない。 + +### 投稿者のアカウントが席ごとに違う環境があるか -- **差分の外を指す指摘が拒まれるときの応答の形。** HTTP 422 が返ることは GitHub の仕様として知られている。応答の本文が行を解決できないことをどの語で示すかと、本文とインラインを同じ要求で送ったときにどちらが拒まれるかは未確認である。実装の最初の段で、偽ではない `gh` で 1 度確かめ、判定に使う語を契約へ書く -- **拒まれた応答から、どのインラインが原因かを 1 件ずつ特定できるかどうか。** 特定できなければ、拒まれた要求のインラインをすべて本文へ退避する形へ落とす。落としたときに本文が長くなりすぎないかを、実装の段で 1 度測る -- **決着の投稿を、すでに決着したスレッドへもう一度送ったときの応答。** 失敗にならないことを前提に置いているが、実行して確かめていない -- **投稿者のアカウントが席ごとに違う環境があるかどうか。** いまの作業環境では 1 つだが、別の環境で担当ごとに別の認証を使う設定があると、照合の鍵の前提が変わる +**無い。** 作業環境の `gh` は 1 アカウントだけを持ち、配布物の中に席ごとの認証を切り替える口は +無い（`plugins/ndf/` を認証の環境変数で引くと 2 行あり、どちらも別の目的の注釈である）。 +要求の前提 1（投稿者アカウントは実質 1 つ）はそのまま成り立ち、照合の鍵に投稿者を含める形を変えない。 ## 申し送り（並行する設計との境界） diff --git a/issues/issue-730-583-plan.md b/issues/issue-730-583-plan.md new file mode 100644 index 00000000..d9ab65e5 --- /dev/null +++ b/issues/issue-730-583-plan.md @@ -0,0 +1,185 @@ +# cross-review: PR に出ている指摘が記録に残らず、同じ論点が 2 つのスレッドに分かれる → GitHub と git へ書くのをレビューを回す側だけにし、途中で止まっても二度書かない（#730 #583） + +## 関連リンク + +| 文書 | 何を持つか | +| --- | --- | +| [issue-730-583-requirements.md](issue-730-583-requirements.md) | 何を満たすか（受け入れ条件 AC1〜AC32） | +| [issue-730-583-design.md](issue-730-583-design.md) | どう作るか（決定 1〜14・データ構造・入出力の契約） | +| #730 | 根本原因の親課題 | +| #583 | 投稿の重なりと起動し直しの経路 | + +## モード + +`standard`。レビューを回す仕組みの振る舞いを変え、複数の実行単位にまたがる。 + +## 目的と非目的 + +達成したい状態: + +- レビューを任された担当が途中で止まっても、PR に出ている指摘と記録が食い違わない +- 起動し直しても同じ論点のスレッドが 2 つに分かれない +- 修正を送ったという報告と、送り先のブランチの実物が一致する + +やらないこと: + +- 収束の判定・指摘の数え方・区分を変える（別の束が持つ） +- 席の決め方と再開の引数を変える（別の束が持つ） +- PR の巻き直しの締め・作り直し・再開の手順を変える（設計の決定 13） +- 子課題 #548 #350 #585 #676 の個別の受け入れ条件を立てる（根本の修正で現象が出なくなる見込みを要求へ書き、閉じるのは棚卸に任せる） + +## 用語の対応 + +**説明は業務用語で通す。** 識別子は設計文書の「用語の対応表」で引く。この計画で使う語だけを再掲する。 + +| 業務用語 | 実体 | +| --- | --- | +| レビューを回す側 | `plugins/ndf/skills/cross-review/scripts/state.py` と、それを呼ぶ `SKILL.md` の骨組み | +| 担当 | 1 ラウンドで 1 者ぶんのレビューを行う CLI | +| 席 | そのラウンドで担当が入る枠 | +| 指摘の控え | 担当が書く、指摘 1 件ごとのファイル | +| 結果ファイル | 担当が書く、判定の要約のファイル | +| 投稿の待ち行列 | `plugins/ndf/scripts/lib/post_queue.py` | +| 結果ファイルを投稿へ変える層 | 新設する `plugins/ndf/scripts/lib/result_posts.py` | +| 指摘の取り込み | `state.py read-result` | +| 修正の取り込み | `state.py merge-fix` | + +## 前提 + +- 前提 1: レビューの投稿者アカウントは 1 つである。作業環境の `gh` の認証は 1 アカウントで、席ごとに切り替える口は配布物に無い（実測で確かめた。「実測で決めたこと」の 4 番目） +- 前提 2: 担当が結果を書けずに止まったとき、その担当は何も投稿していない。投稿の手順をプロンプトから外すため、変更の後に成り立つ +- 前提 3: 修正の担当が働く作業ツリーは、取り込む側から同じパスで見える +- 前提 4: 待ち行列は作業ツリーの中にあり、巻き直しのときは作業ツリーごと捨てられる + +## 実測で決めたこと + +設計文書が「未確認のまま残ること」として残した 4 件を、実物の `gh` で 1 度ずつ動かして決めた。 +実測の記録は設計文書の同じ節へ書き戻す（同じ Pull Request に含める）。 + +| 何を | 決めたこと | +| --- | --- | +| 差分の外を指す指摘が拒まれるときの応答の形 | 要求ごとに全件が拒まれ、一部だけが作られることはない。判定に使う語は `could not be resolved` である。判定の値の誤りと基準のコミットの誤りはこの語を含まないため、失敗として残せる | +| 拒まれた応答から原因を 1 件ずつ特定できるか | できない。応答は語をつないだ 1 つの文字列で、位置も識別子も持たない。設計の落とし先（拒まれた要求のインラインをすべて本文へ退避する）を採る | +| すでに決着したスレッドをもう一度決着させたときの応答 | 冪等である。成功し、決着済みを返す。失敗にならない | +| 投稿者のアカウントが席ごとに違う環境があるか | 無い。前提 1 のとおり | + +## 受け入れ条件 + +**一覧は要求の文書が持つ**（AC1〜AC32）。この計画では、タスクごとに満たす番号を示す。 + +## 互換性 + +| 対象 | 変更 | 互換性の扱い | +| --- | --- | --- | +| 取り込み・判定・報告の終了コード | 変えない | AC23 | +| 収束の判定と報告が読む変数 | 変えない | AC23 | +| 取り込みの標準出力 | 投稿の結果を表す行と、取り込んだ指摘の件数を足す | 追加のみ | +| 結果ファイルの項目 | 担当が書く項目を減らし、投稿する側が埋める項目を増やす | 担当の側の契約が変わる。プロンプトと同じ変更に含める | +| 待ち行列の保存先 | 変えない | 前提 4 | +| 席の名前・結末の語彙・数えない指摘の区分 | 変えない | AC24 | + +## 修正対象 + +| ファイル | 扱い | +| --- | --- | +| `plugins/ndf/scripts/lib/result_posts.py` | 新設 | +| `plugins/ndf/scripts/lib/post_queue.py` | 変更 | +| `plugins/ndf/skills/cross-review/scripts/state.py` | 変更 | +| `plugins/ndf/skills/cross-review/scripts/launch-reviewer.sh` | 変更 | +| `plugins/ndf/skills/cross-review/SKILL.md` | 変更 | +| `plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md` | 変更 | +| `plugins/ndf/skills/cross-review/docs/03-review-output.md` | 変更 | +| `plugins/ndf/skills/cross-review/docs/04-contracts.md` | 変更 | +| `plugins/ndf/skills/cross-review/references/context-budget.md` | 変更 | +| `plugins/ndf/skills/fix/SKILL.md` | 変更 | +| `issues/issue-730-583-design.md` | 「未確認のまま残ること」を実測の結果へ書き直す | +| 各ランタイムの配布物 | 生成の実行で揃える | + +## タスク分解 + +機能単位で分ける。各タスクは、失敗するテストを先に書いてから通す。 + +### Task 1: 二度書かない照合を 4 種別すべてへ広げ、拒まれ方を区別して返す + +- **対象ファイル:** `plugins/ndf/scripts/lib/post_queue.py` +- **変更内容:** 先客がいるかを見る照合の鍵を、レビューの投稿・返信・決着・まとめの 4 種別それぞれに与える。レビューの投稿の鍵は本文の先頭行のうちラウンドと席までの前方一致とし、判定の語を含めない。送信が拒まれたとき、行やファイルを解決できないことを示す応答と、それ以外の拒まれ方を呼ぶ側が見分けられる形で返す +- **満たす受け入れ条件:** AC10・AC11・AC13・AC14（照合）、AC16（拒まれ方の区別） +- **進め方:** 偽の `gh` で先客を返す状態を作り、同じ項目を 2 度積んでも増えないことを見るテストを先に書く + +### Task 2: 指摘の控えと結果ファイルからレビューの投稿を組み立てて送る層を新設する + +- **対象ファイル:** `plugins/ndf/scripts/lib/result_posts.py`（新設） +- **変更内容:** 控えと結果ファイルのパスを受け取り、待ち行列へ積む項目の列を返す。本文は引数にも標準出力にも出さない。自分の Pull Request かどうかを受け取り、送った形だけを格下げする。インラインが行を解決できないことを理由に拒まれたら、その要求のインラインを本文の末尾へ移して送り直し、移した指摘の宛先を本文として記録する +- **満たす受け入れ条件:** AC5・AC15・AC16・AC17・AC32 +- **進め方:** 偽の `gh` が拒む応答を返す状態で、本文へ移した件数と宛先の値を見るテストを先に書く + +### Task 3: 指摘の取り込みが投稿を行い、申告と実数の突き合わせをやめる + +- **対象ファイル:** `plugins/ndf/skills/cross-review/scripts/state.py` +- **変更内容:** 控えを読む → 投稿を積む → 流す → 送信の応答を記録へ書き戻す → 指摘を取り込む、の順で 1 回の呼び出しの中を進める。担当の申告件数を GitHub の実数と比べて中断する処理を取り除く。記録に入る投稿の URL は送信の応答から取り、件数は送れたインラインの数から取る。標準出力へ足すのは件数・URL・状態の行だけにする +- **満たす受け入れ条件:** AC5・AC6・AC12・AC15・AC17・AC23 +- **進め方:** 標準出力に控えの本文の文字列が含まれないことと、投稿の後・記録の前で止めた状態から呼び直してもレビューが増えないことを見るテストを先に書く + +### Task 4: 担当のプロンプトから投稿の手順を外し、書き終えてから改名で公開する + +- **対象ファイル:** `plugins/ndf/skills/cross-review/scripts/launch-reviewer.sh` +- **変更内容:** プロンプトから、レビューを投稿する手順（投稿の呼び出し・判定の値の指定・インラインの組み立て）を外す。担当が書くのは指摘の控えと結果ファイルの 2 つだけにする。どちらも一時の名前で書かせ、控えを先、結果ファイルを後の順で正式の名前へ改名させる +- **満たす受け入れ条件:** AC1・AC2・AC3・AC4 +- **進め方:** 組み立てたプロンプトを取り出し、投稿の手順の語が 0 件であることを見るテストを先に書く + +### Task 5: 修正の返信・決着・まとめと送信を 1 つの層にまとめ、単独の口を与える + +- **対象ファイル:** `plugins/ndf/scripts/lib/result_posts.py`・`plugins/ndf/skills/cross-review/scripts/state.py`・`plugins/ndf/skills/fix/SKILL.md` +- **変更内容:** 修正の結果ファイルから返信・決着・まとめの投稿を組み立てる口と、現在の頭を指定して送信する口を同じ層へ置く。送信の後、報告されたコミットが送り先のブランチの履歴に含まれることを確かめ、含まれなければ失敗として止まる。修正の取り込みはこの層を呼び、単独で使うときは同じ層を部分命令として直接呼ぶ。単独で使う手順は 1 行のコマンドへ置き換える +- **満たす受け入れ条件:** AC7・AC8・AC9・AC21・AC22 +- **進め方:** 送信を偽装し、報告されたコミットが送り先に無いときに失敗することを見るテストを先に書く + +### Task 6: 起動し直しを初回と同じ経路へ通す + +- **対象ファイル:** `plugins/ndf/skills/cross-review/SKILL.md` +- **変更内容:** 骨組みの起動し直しの枝を、繰り返しの先頭へ戻す形にする。待ち行列に残りがあるときの枝は、2 度目を含む各判定の直後に残す。設計方針の表から、担当が直接投稿する記述を外し、投稿の担い手とその理由を入れる +- **満たす受け入れ条件:** AC18・AC19・AC25 +- **進め方:** 骨組みの行の並びを読む既存の検査へ条件を足す + +### Task 7: 文書の決定を書き直す + +- **対象ファイル:** `docs/02-fix-and-rotation.md`・`docs/03-review-output.md`・`docs/04-contracts.md`・`references/context-budget.md`（いずれも `plugins/ndf/skills/cross-review/` の配下）・`issues/issue-730-583-design.md` +- **変更内容:** 修正の手順から担当が送信する行を外し、取り込む側が送信することを書く。担当が直接投稿する決定を、待ち行列を通す形へ書き直す。投稿の種別ごとの契約を載せる。文脈の予算の工夫の一覧を、本文がレビューを回す側のプロセスの中だけを通る形へ書き直す。設計文書の「未確認のまま残ること」を実測の結果へ書き直す +- **満たす受け入れ条件:** AC26・AC27・AC28・AC29 +- **進め方:** 文言の検査。振る舞いを持たないためテストは書かない + +### Task 8: 生成物を揃え、全体の検査を通す + +- **対象ファイル:** 各ランタイムの配布物 +- **変更内容:** 生成の実行で配布物を揃え、全体のテストと検査を通す +- **満たす受け入れ条件:** AC23・AC24・AC30・AC31 +- **進め方:** テストと検査のコマンドを実行し、終了コードを証跡として残す + +## 影響範囲 + +| 何が | どう変わるか | +| --- | --- | +| 担当の CLI | GitHub への書き込みの権限が要らなくなる。実行時間が短くなる | +| 収束の判定 | 変わらない。読む変数と終了コードを保つ | +| 修正を単独で行う経路 | 書き込みの実装が、レビューを回す経路と 1 つになる | +| 並行する束 | 席の決め方（PR #793、マージ済み）と同じファイルを触る。マージ済みのため競合は着手の時点で解けている | + +## リスクと対処 + +| リスク | 対処 | +| --- | --- | +| 変更が 1 ファイル 4983 行の取り込みの実装に集中する | タスクごとにテストを通す。構造の整理はこの変更の後の構造改善へ回す | +| 投稿の担い手を移す間に、送る経路が 2 つ並ぶ | 担当のプロンプトから手順を外す変更（Task 4）と、投稿を行う変更（Task 3）を同じ Pull Request に入れる | +| 拒まれ方の区別が実物の応答に合わない | 実測で確かめた語を契約へ書き、偽の応答をその形で作る | +| 本文へ退避した指摘で本文が長くなる | 1 ラウンドの指摘は多くて 10 件前後で、投稿の本文の上限に対して十分小さい（実測の記録に根拠を置く） | + +## 切り戻し手順 + +Pull Request の単位で戻せる。外部の系の状態を変える移行は無い。待ち行列の保存先を変えないため、 +途中まで積まれた項目は作業ツリーごと捨てられる。 + +## 完了の定義 + +- [ ] 受け入れ条件 AC1〜AC32 をすべて満たし、条件ごとに検証手段と結果が対応している +- [ ] `uv run --with pytest pytest scripts/tests plugins/ndf -q` が通る +- [ ] `bash scripts/build-runtime-plugins.sh --check`・`claude plugin validate .`・`python3 scripts/check-skill-frontmatter.py` が終了コード 0 で終わる diff --git a/plugins/ndf/scripts/lib/post_queue.py b/plugins/ndf/scripts/lib/post_queue.py index e79511ab..39949f53 100755 --- a/plugins/ndf/scripts/lib/post_queue.py +++ b/plugins/ndf/scripts/lib/post_queue.py @@ -75,13 +75,6 @@ QUEUED = "queued" FAILED = "failed" -# レビューの判定と、GitHub 側に残る状態の対応。 -_REVIEW_STATE = { - "APPROVE": "APPROVED", - "REQUEST_CHANGES": "CHANGES_REQUESTED", - "COMMENT": "COMMENTED", -} - # 未解決のスレッドの識別子だけを読む問い合わせ。**解決の冪等はこの一覧だけで決まる** # （一覧に無ければ、既に解決されている）。 _UNRESOLVED_QUERY = """ @@ -115,6 +108,9 @@ _HTTP_RE = re.compile(r"\(HTTP (\d{3})\)") # 上限を指す語。一次・二次・GraphQL の 3 つの言い回しを拾う。 _RATE_WORDS = ("rate limit", "rate_limited", "abuse detection") +# 指した位置を差分の中に見つけられないことを表す語。実測した応答は +# `Line could not be resolved` と `Path could not be resolved` の 2 つ。 +_POSITION_WORD = "could not be resolved" class Attempt(NamedTuple): @@ -149,6 +145,10 @@ def message(self) -> str: for e in errors: if isinstance(e, dict): parts += [str(e.get("message") or ""), str(e.get("type") or "")] + elif isinstance(e, str): + # レビューの作成が拒まれたときは、語をつないだ文字列が並ぶ + # （実測: `["Line could not be resolved"]`）。 + parts.append(e) return " ".join(p for p in parts if p) def summary(self) -> str: @@ -210,6 +210,24 @@ def is_rate_limited(attempt: Attempt) -> bool: return False +def is_position_unresolved(attempt: Attempt) -> bool: + """この失敗が「指した位置を差分の中に見つけられない」ことによるものか。 + + **レビューの作成は要求ごとに全件が拒まれる。** 差分の外の行やファイルを指した + インラインが 1 件でもあると、正しいインラインも総評も作られない（実測、 + 2026-09-22）。応答は語をつないだ 1 つの文字列で、どの項目かは指さないため、 + 呼び出し側は要求のインラインをまとめて総評へ移して送り直す。 + + **同じ状態で返る別の拒まれ方と分ける。** 判定の値の誤り + （`Variable $event ... was provided invalid value`）と基準のコミットの誤り + （`The commitOID is not part of the pull request`）はこの語を持たない。 + 区別しないと、別の不具合が退避として飲み込まれる。 + """ + if attempt.ok or attempt.http != 422: + return False + return _POSITION_WORD in f"{attempt.message} {attempt.stderr}".lower() + + # ---------------- 送る内容の組み立て ---------------- @@ -233,11 +251,16 @@ def _request_review_post(repo: str, pr: int, fields: dict[str, Any]) -> dict[str body["commit_id"] = fields["commit_id"] if fields.get("comments"): body["comments"] = fields["comments"] + match = {"event": fields["event"], "body": fields.get("body", "")} + # **照合をラウンドの開始より後へ絞る。** ラウンドの番号は実行ごとに 1 から数え + # 直すため、番号と席の鍵だけでは前の実行のレビューに一致する。 + if fields.get("since"): + match["since"] = fields["since"] return { "request": {"method": "POST", "path": f"repos/{repo}/pulls/{int(pr)}/reviews", "fields": body}, - "match": {"event": fields["event"], "body": fields.get("body", "")}, + "match": match, } @@ -334,24 +357,66 @@ def _by_actor(row: dict[str, Any], actor: str | None) -> bool: return str((row.get("user") or {}).get("login") or "") == actor +def _head(body: Any) -> str: + return str(body or "")[:BODY_MATCH_CHARS] + + +def review_match_key(body: Any) -> str: + """レビューの本文から、同じ投稿かどうかを決める鍵を作る。 + + 先頭行は `## 🤖 cross-review | round | <席> | <判定>` である。**鍵に取るのは + 席までで、判定の語を含めない。** 含めると、起動し直して判定が変わったときに別の + 投稿と読まれ、同じラウンド・同じ席のレビューが 2 件になる（#730 #583）。 + + 先頭行がこの形でないときは、本文の先頭 `BODY_MATCH_CHARS` 文字へ落とす。 + """ + first = str(body or "").splitlines()[0] if str(body or "") else "" + parts = first.split("|") + if len(parts) < 4: + return _head(body) + return "|".join(parts[:3]).strip() + "|" + + def _comment_match(match: dict[str, Any], actor: str | None): - return lambda row: _by_actor(row, actor) and row.get("body") == match.get("body") + head = _head(match.get("body")) + return lambda row: _by_actor(row, actor) and _head(row.get("body")) == head + + +def _parse_time(value: Any) -> _dt.datetime | None: + try: + parsed = _dt.datetime.fromisoformat(str(value or "").replace("Z", "+00:00")) + except ValueError: + return None + return parsed if parsed.tzinfo is not None else None def _review_match(match: dict[str, Any], actor: str | None): - want = _REVIEW_STATE.get(str(match.get("event") or ""), "") - head = str(match.get("body") or "")[:BODY_MATCH_CHARS] + """同じラウンド・同じ席のレビューか。 + + **開始時刻を持つときは、それより後に出たレビューだけを見る。** 同じ実行の中の + 送り直しは見つかり、回し直す前の実行のレビューは外れる。開始時刻かレビューの + 時刻のどちらかを読めないときは、番号と席だけの照合へ落とす（二重に送る側へ + 倒さない）。 + """ + key = review_match_key(match.get("body")) + since = _parse_time(match.get("since")) + + def _in_this_run(row: dict[str, Any]) -> bool: + submitted = _parse_time(row.get("submitted_at")) + return since is None or submitted is None or submitted >= since + return lambda row: ( _by_actor(row, actor) - and str(row.get("state") or "") == want - and str(row.get("body") or "")[:BODY_MATCH_CHARS] == head + and review_match_key(row.get("body")) == key + and _in_this_run(row) ) def _reply_match(match: dict[str, Any], actor: str | None): + head = _head(match.get("body")) return lambda row: ( str(row.get("in_reply_to_id") or "") == str(match.get("in_reply_to")) - and row.get("body") == match.get("body") + and _head(row.get("body")) == head ) @@ -411,7 +476,7 @@ def already_posted(item: dict[str, Any]) -> bool | None: _SEQ_RE = re.compile(r"^(\d{4})-") -def _read_item(path: pathlib.Path) -> dict[str, Any] | None: +def read_item(path: pathlib.Path) -> dict[str, Any] | None: """待ち行列の項目を 1 件読む。読めなければ `None`。 項目は作成先の JSON ファイルへ直接書かれるため、書き込みの途中で終了すると @@ -424,6 +489,20 @@ def _read_item(path: pathlib.Path) -> dict[str, Any] | None: return item if isinstance(item, dict) else None +_read_item = read_item + + +def rejected_by_position(item: dict[str, Any]) -> bool: + """待ち行列に残った項目が、指した位置を解決できずに拒まれたものか。 + + 流した後に呼ぶ。`is_position_unresolved` と同じ判定を、項目へ残した状態と説明から + 行う（流す側は `Attempt` を返さないため）。 + """ + if int(item.get("last_status") or 0) != 422: + return False + return _POSITION_WORD in str(item.get("last_error") or "").lower() + + class FlushResult(NamedTuple): """流した結果。""" @@ -462,6 +541,20 @@ def items(self) -> list[tuple[pathlib.Path, dict[str, Any]]]: out.append((p, item)) return out + def drop(self, seq: Any) -> bool: + """連番で指した項目を 1 件取り除く。 + + 送れなかった項目を、送る内容を変えて積み直すときに使う（差分の外を指す指摘の + 退避）。**そのまま積み足すと、同じ論点の要求が 2 件並ぶ。** + """ + if seq is None: + return False + for path, item in self.items(): + if item.get("seq") == seq: + path.unlink(missing_ok=True) + return True + return False + def _next_seq(self) -> int: seqs = [int(m.group(1)) for m in (_SEQ_RE.match(p.name) for p in self.paths()) if m] @@ -487,6 +580,41 @@ def add(self, item: dict[str, Any], ident: str | int) -> pathlib.Path: json.dump(item, f, indent=2, ensure_ascii=False) return path + def _item_to_send( + self, path: pathlib.Path + ) -> tuple[dict[str, Any] | None, dict[str, Any] | None, + dict[str, Any] | None]: + """項目を読み、送る項目・既投稿・読込失敗のいずれかを返す。""" + item = _read_item(path) + if item is None: + return None, None, { + "path": str(path), + "last_error": f"待ち行列の項目を読めない ({path.name})", + } + found, row = posted_match(item) + if found is not True: + return item, None, None + if row is not None: + item["response"] = row + path.unlink(missing_ok=True) + return None, item, None + + def _send_item(self, path: pathlib.Path, item: dict[str, Any]) -> tuple[bool, Any]: + """1 項目を送り、成功時の応答または失敗情報を項目へ反映する。""" + attempt = send(item) + if attempt.ok: + try: + item["response"] = json.loads(attempt.stdout or "null") + except json.JSONDecodeError: + item["response"] = None + path.unlink(missing_ok=True) + return True, attempt + item["attempts"] = int(item.get("attempts") or 0) + 1 + item["last_error"] = attempt.summary() + item["last_status"] = attempt.http + path.write_text(json.dumps(item, indent=2, ensure_ascii=False), encoding="utf-8") + return False, attempt + def flush(self) -> FlushResult: """積んだ項目を連番の順に送る。 @@ -498,39 +626,18 @@ def flush(self) -> FlushResult: failed: dict[str, Any] | None = None rate_limited = False for path in self.paths(): - item = _read_item(path) - if item is None: - # **読めない項目を黙って飛ばさない。** `count()` はファイルを数え - # 続けるため、飛ばすと送りも失敗の報告もしないまま件数だけが残り、 - # 判定は終了コード 8 を返し続けて誰も直せない状態になる。ここで - # 止めて理由を返せば、その項目を捨てるか直すかを人が選べる。 - failed = { - "path": str(path), - "last_error": f"待ち行列の項目を読めない ({path.name})", - } + item, already_posted, read_failure = self._item_to_send(path) + if read_failure is not None: + failed = read_failure break - found, row = posted_match(item) - if found is True: - # **送った場合と同じ形で返す。** 呼び出し側は届いたことを応答から - # 確かめるため、既に届いていた項目にも見つけた投稿を積んで渡す。 - if row is not None: - item["response"] = row - path.unlink(missing_ok=True) - skipped.append(item) + if already_posted is not None: + skipped.append(already_posted) continue - attempt = send(item) - if attempt.ok: - try: - item["response"] = json.loads(attempt.stdout or "null") - except json.JSONDecodeError: - item["response"] = None - path.unlink(missing_ok=True) + assert item is not None + ok, attempt = self._send_item(path, item) + if ok: sent.append(item) continue - item["attempts"] = int(item.get("attempts") or 0) + 1 - item["last_error"] = attempt.summary() - path.write_text(json.dumps(item, indent=2, ensure_ascii=False), - encoding="utf-8") failed = item rate_limited = is_rate_limited(attempt) break diff --git a/plugins/ndf/scripts/lib/result_posts.py b/plugins/ndf/scripts/lib/result_posts.py new file mode 100644 index 00000000..010e778e --- /dev/null +++ b/plugins/ndf/scripts/lib/result_posts.py @@ -0,0 +1,550 @@ +#!/usr/bin/env python3 +"""結果ファイルを投稿へ変える層（#730 #583）。 + +**GitHub と git へ書くのは、レビューを回す側だけである。** 担当は指摘の控えと結果 +ファイルを書いて終わり、修正の担当はコミットまでを行う。この層が、その 2 種類の +結果ファイルを読んで投稿を組み立て、待ち行列を通して送り、送信の応答を返す。 + +**本文は引数にも標準出力にも出さない。** どの入口もファイルのパスを受け取り、本文は +この層の中だけを通る。収束ループを駆動している側の応答に本文が載ると、文脈の予算の +決めに反する。 + +**2 つの口を持つ。** 収束ループから呼ぶときは取り込みがこの層を呼び、単独で修正を +行うときは同じまとまりを部分命令として直接呼ぶ。入口のスクリプトを別に作らない。 + +```bash +python3 "$SCRIPTS/lib/result_posts.py" fix --repo <所有者>/<リポジトリ> --pr <番号> \\ + --result <結果ファイル> --head <ブランチ名> --worktree <作業ツリー> +``` +""" +from __future__ import annotations + +import argparse +import json +import os +import pathlib +import subprocess +import sys +from typing import Any, NamedTuple + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent)) + +import post_queue # noqa: E402 +import statefile # noqa: E402 + +# レビューの本文の先頭行。**照合の鍵はラウンドと席までの前方一致である**ため、判定の +# 語はこの行の末尾に置く（`post_queue.review_match_key`）。 +REVIEW_HEAD = "## 🤖 cross-review | round {round_no} | {seat} | {event}" +# 差分の外を指すために総評へ移した指摘の見出し。 +EVACUATED_HEAD = "### 差分の外を指す指摘" +# 修正のまとめの先頭行。ラウンドとコミットを鍵の幅（先頭 80 文字）の中へ入れる。 +FIX_HEAD = "## 🔧 /ndf:fix サマリ | round {round_no} | commit {commit}" +FIX_HEAD_NO_ROUND = "## 🔧 /ndf:fix サマリ | commit {commit}" + +TMP_DIRNAME = ".cross_review" + + +# ---------------- 読み取り ---------------- + + +def _read_json(path: pathlib.Path | str | None) -> dict[str, Any]: + if path is None: + return {} + try: + data = json.loads(pathlib.Path(path).read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + return {} + return data if isinstance(data, dict) else {} + + +def _findings(payload: dict[str, Any]) -> list[dict[str, Any]]: + items = payload.get("comments") + if not isinstance(items, list): + return [] + return [i for i in items if isinstance(i, dict)] + + +def _dict_items(raw: Any) -> list[dict[str, Any]]: + if isinstance(raw, dict): + return [raw] + if isinstance(raw, list): + return [i for i in raw if isinstance(i, dict)] + return [] + + +# ---------------- レビューの投稿 ---------------- + + +def _line_no(value: Any) -> int | None: + """行番号として読めるときだけ正の int を返す。 + + **行は担当が書き出す外部入力である。** `"L42"` や `"40-45"` を `int()` へ渡すと + 例外でレビュー全体が失われるため、読めない値は位置を持たないものとして扱う。 + """ + if isinstance(value, bool): + return None + if isinstance(value, int): + return value if value > 0 else None + if isinstance(value, str) and value.strip().isdecimal(): + return int(value.strip()) or None + return None + + +def _can_be_inline(finding: dict[str, Any]) -> bool: + """指す先を持つか。**差分に含まれるかどうかは見ない。** + + 含まれるかは送ってみた応答が決める（設計の決定 9）。ここで見るのは、そもそも + 指す位置があるかどうかだけである。行が数として読めない指摘は総評へ回す。 + """ + return bool(finding.get("path")) and _line_no(finding.get("line")) is not None + + +def _evacuated_line(finding: dict[str, Any]) -> str: + where = str(finding.get("path") or "") + line = finding.get("line") + head = f"`{where}:{line}`" if where and line is not None else ( + f"`{where}`" if where else "") + severity = str(finding.get("severity") or "") + mark = f" [{severity}]" if severity else "" + text = str(finding.get("body") or "").strip() + return f"- {head}{mark} {text}".strip() + + +def _review_body(payload: dict[str, Any], round_no: int, seat: str, intent: str, + evacuated: list[dict[str, Any]]) -> str: + parts = [REVIEW_HEAD.format(round_no=round_no, seat=seat, event=intent)] + summary = str(payload.get("summary") or "").strip() + if summary: + parts.append(summary) + if evacuated: + parts.append(EVACUATED_HEAD) + parts.append("\n".join(_evacuated_line(f) for f in evacuated)) + return "\n\n".join(parts) + "\n" + + +def review_posts(payload_path: pathlib.Path | str, result_path: pathlib.Path | str, + repo: str, pr: int, round_no: int, seat: str, + head_sha: str | None, is_own_pr: bool, + evacuate_all: bool = False, + since: str | None = None) -> list[dict[str, Any]]: + """指摘の控えと結果ファイルから、待ち行列へ積む項目の列を組み立てる。 + + **判定の格下げはこの層が決める**（設計の決定 14）。自分の Pull Request へは変更を + 求めるレビューを送れないため、送る形だけを `COMMENT` へ落とす。本来の判定は + 先頭行と `extra` に残り、収束の判定はそちらを読む。 + + `since` はそのラウンドが始まった時刻である。二度書かない照合をこの時刻より後に + 出たレビューへ絞るために、待ち行列の項目へ持たせる。 + + `evacuate_all` が真のとき、位置を持つ指摘もすべて総評へ移す。送った要求が位置を + 解決できずに拒まれた後の送り直しで使う。 + """ + payload = _read_json(payload_path) + result = _read_json(result_path) + findings = _findings(payload) + intent = str(result.get("event") or result.get("intent") or "COMMENT") + posted_as = "COMMENT" if is_own_pr else intent + + inline = [] if evacuate_all else [f for f in findings if _can_be_inline(f)] + evacuated = [f for f in findings if f not in inline] + body = _review_body(payload, round_no, seat, intent, evacuated) + + fields: dict[str, Any] = {"body": body, "event": posted_as} + if head_sha: + fields["commit_id"] = head_sha + if since: + fields["since"] = since + if inline: + fields["comments"] = [ + {"path": str(f.get("path")), "line": _line_no(f.get("line")), + "side": "RIGHT", "body": str(f.get("body") or "")} + for f in inline + ] + extra = {"ident": f"{seat}-r{round_no}", "agent": seat, "seat": seat, + "round": round_no, + "intent": intent, "posted_as": posted_as, + "inline": len(inline), "body": len(evacuated)} + return [{"kind": "review-post", "fields": fields, "extra": extra}] + + +class ReviewOutcome(NamedTuple): + """レビューを 1 件送った結果。**本文は持たない。**""" + + review_url: str | None + posted_inline: int + posted_body: int + queued: int + findings: int + failed: bool + posted_as: str + intent: str + detail: str + + +def _find(items: list[dict[str, Any]], seq: Any) -> dict[str, Any] | None: + return next((i for i in items if i.get("seq") == seq), None) + + +def _response_url(item: dict[str, Any] | None) -> str | None: + response = (item or {}).get("response") + if not isinstance(response, dict): + return None + url = response.get("html_url") + if url: + return str(url) + if response.get("id"): + return f"#pullrequestreview-{response['id']}" + return None + + +def _write_destinations(payload_path: pathlib.Path | str, inline_count: int) -> str: + """控えへ、送れた先を書き戻す。**決めるのは投稿する側である。** + + **原子的に書き、失敗は呼び出し元へ返す。** 半端な控えが残ると、再実行で読めずに + 空として扱われ、記録済みの指摘を 0 件で置き換える。戻り値は失敗の説明で、 + 書けたときは空文字である。 + """ + path = pathlib.Path(payload_path) + payload = _read_json(path) + findings = _findings(payload) + if not findings: + return "" + inline_ids = {id(f) for f in findings if _can_be_inline(f)} if inline_count else set() + for f in findings: + f["posted_to"] = "inline" if id(f) in inline_ids else "body" + try: + statefile.write_json_atomic(path, payload) + except OSError as exc: + return f"控えへ送れた先を書けない ({path.name}: {exc})" + return "" + + +def post_review(queue: post_queue.Queue, payload_path: pathlib.Path | str, + result_path: pathlib.Path | str, repo: str, pr: int, round_no: int, + seat: str, head_sha: str | None, is_own_pr: bool, + actor: str | None = None, since: str | None = None) -> ReviewOutcome: + """レビューを 1 件、待ち行列を通して送る。 + + **位置を解決できずに拒まれたら、その要求のインラインをすべて総評へ移して送り直す。** + 応答はどの項目が原因かを指さないため、1 件ずつの特定はできない（実測）。 + 同じ状態で返る別の拒まれ方（判定の値の誤り・基準のコミットの誤り）は退避せず、 + 失敗として残す。 + + **退避するのは、今回積んだ項目が拒まれたときだけである。** 先に積まれていた項目 + （先客）の拒まれ方を今回分のものと取り違えると、先客を消して今回分を二重に積む。 + 今回分が送れていない限り、控えへ送れた先を書かない。 + """ + findings = len(_findings(_read_json(payload_path))) + item = review_posts(payload_path, result_path, repo, pr, round_no, seat, + head_sha, is_own_pr, since=since)[0] + path = post_queue.enqueue(queue, item["kind"], repo, pr, item["fields"], + actor=actor, extra=item["extra"]) + seq = (post_queue.read_item(path) or {}).get("seq") + flushed = queue.flush() + + ours_failed = flushed.failed is not None and flushed.failed.get("seq") == seq + if ours_failed and post_queue.rejected_by_position(flushed.failed): + queue.drop(flushed.failed.get("seq")) + item = review_posts(payload_path, result_path, repo, pr, round_no, seat, + head_sha, is_own_pr, evacuate_all=True, since=since)[0] + path = post_queue.enqueue(queue, item["kind"], repo, pr, item["fields"], + actor=actor, extra=item["extra"]) + seq = (post_queue.read_item(path) or {}).get("seq") + flushed = queue.flush() + + done = _find(flushed.sent, seq) or _find(flushed.skipped, seq) + # 送れた後に控えを書けなければ、取り込みを止める。再実行は既投稿として照合し直す。 + note_error = _write_destinations(payload_path, item["extra"]["inline"]) if done else "" + return ReviewOutcome( + review_url=_response_url(done), + posted_inline=item["extra"]["inline"] if done else 0, + posted_body=item["extra"]["body"] if done else 0, + queued=0 if done else 1, + findings=findings, + # 先客に止められた場合も、今回分は送れていない。上限だけは待てば流れる。 + failed=bool(note_error) or bool(not done and flushed.failed is not None + and not flushed.rate_limited), + posted_as=item["extra"]["posted_as"], + intent=item["extra"]["intent"], + detail=note_error or str((flushed.failed or {}).get("last_error") or ""), + ) + + +# ---------------- 修正の投稿 ---------------- + + +def _reply(comment_id: Any, body: str) -> dict[str, Any] | None: + try: + target = int(comment_id) + except (TypeError, ValueError): + return None + return {"kind": "review-reply", "fields": {"in_reply_to": target, "body": body}, + "extra": {"ident": f"reply-{target}"}} + + +def _fix_summary_body(fix: dict[str, Any], round_no: int | None, + resolved: int, deferred: int, rejected: int) -> str: + commit = str(fix.get("fix_commit") or fix.get("commit_sha") or "(なし)") + head = (FIX_HEAD.format(round_no=round_no, commit=commit) if round_no is not None + else FIX_HEAD_NO_ROUND.format(commit=commit)) + by = fix.get("by_severity") or {} + counts = " / ".join(f"{k}={by.get(k, 0)}" for k in ("critical", "major", "minor")) + lines = [ + head, + "", + f"対応件数: {counts}（合計 {fix.get('fixed_count', fix.get('fixed', 0))} 件）", + f"決着: {resolved} 件 / 見送り: {deferred} 件 / 却下: {rejected} 件", + f"CI: {fix.get('ci_status') or 'NONE'}", + ] + note = str(fix.get("ci_note") or "").strip() + if note: + lines += ["", note] + return "\n".join(lines) + "\n" + + +def fix_posts(result_path: pathlib.Path | str, repo: str, pr: int, + round_no: int | None = None) -> list[dict[str, Any]]: + """修正の結果ファイルから、待ち行列へ積む項目の列を組み立てる。 + + 並びは「返信 → 決着 → まとめ」である。返信を先に置くのは、決着したスレッドが + 畳まれた後に返信が届くと、読み手がその返信を開かないためである。 + """ + fix = _read_json(result_path) + resolved = _dict_items(fix.get("resolved_threads")) + deferred = _dict_items(fix.get("deferred")) + rejected = _dict_items(fix.get("rejected")) + commit = str(fix.get("fix_commit") or fix.get("commit_sha") or "") + + # (要素の列, 返信の定型句, 理由を取り出すキー)。決着は理由の代わりにコミットを添える + reply_rules = ( + (resolved, "対応しました。", None), + (deferred, "見送ります。", "reason_for_deferral"), + (rejected, "この指摘は採らない判断です。", "reason_for_rejection"), + ) + items: list[dict[str, Any]] = [] + for entries, lead, reason_key in reply_rules: + for entry in entries: + if reason_key is None: + note = f"（{commit}）" if commit else "" + else: + note = str(entry.get(reason_key) or entry.get("reason") or "") + reply = _reply(entry.get("comment_id"), f"{lead}{note}".strip()) + if reply: + items.append(reply) + # 見送り・却下は既定では決着させない（次のラウンドで見直す）。最終スイープは + # スレッドを残さないため、要素の `resolve` を真にして決着まで求める。 + closing = resolved + [e for e in deferred + rejected if e.get("resolve")] + for entry in closing: + thread_id = entry.get("thread_id") + if thread_id: + items.append({"kind": "thread-resolve", + "fields": {"thread_id": str(thread_id)}, + "extra": {"ident": f"resolve-{thread_id}"}}) + items.append({ + "kind": "pr-comment", + "fields": {"body": _fix_summary_body(fix, round_no, len(resolved), + len(deferred), len(rejected))}, + "extra": {"ident": f"fix-summary-{round_no if round_no is not None else commit}"}, + }) + return items + + +class FixOutcome(NamedTuple): + """修正の投稿を送った結果。""" + + summary_url: str | None + replied: int + resolved: int + queued: int + failed: bool + detail: str + + +def post_fix(queue: post_queue.Queue, result_path: pathlib.Path | str, repo: str, + pr: int, round_no: int | None = None, + actor: str | None = None) -> FixOutcome: + """返信・決着・まとめを待ち行列へ積んで流す。""" + seqs: dict[int, str] = {} + for item in fix_posts(result_path, repo, pr, round_no): + path = post_queue.enqueue(queue, item["kind"], repo, pr, item["fields"], + actor=actor, extra=item["extra"]) + seq = (post_queue.read_item(path) or {}).get("seq") + if seq is not None: + seqs[int(seq)] = item["kind"] + flushed = queue.flush() + + done = {int(i["seq"]): i for i in (flushed.sent + flushed.skipped) + if i.get("seq") is not None} + summary_url = None + for seq, kind in seqs.items(): + if kind == "pr-comment" and seq in done: + response = done[seq].get("response") + if isinstance(response, dict): + summary_url = response.get("html_url") or response.get("url") + failed = flushed.failed is not None and not flushed.rate_limited + return FixOutcome( + summary_url=str(summary_url) if summary_url else None, + replied=sum(1 for s, k in seqs.items() if k == "review-reply" and s in done), + resolved=sum(1 for s, k in seqs.items() if k == "thread-resolve" and s in done), + queued=flushed.remaining, + failed=bool(failed), + detail=str((flushed.failed or {}).get("last_error") or ""), + ) + + +# ---------------- 修正の送信 ---------------- + + +class PushResult(NamedTuple): + """送信の結果と、報告されたコミットが送り先に載っているか。""" + + ok: bool + pushed: bool + contains: bool + detail: str + + +# 認証の退避の値は共通層 1 か所が持つ（`git-credential.sh`）。ここへ写さない。 +_CREDENTIAL_LIB = pathlib.Path(__file__).resolve().parent / "git-credential.sh" + + +def _credential_fallback_args() -> list[str]: + r = subprocess.run( + ["bash", "-c", f'. "{_CREDENTIAL_LIB}"; ndf_git_credential_fallback_args'], + capture_output=True, text=True) + return [line for line in r.stdout.split("\n") if line] if r.returncode == 0 else [] + + +def _git(worktree: pathlib.Path | str, *args: str) -> subprocess.CompletedProcess: + """git を 1 度実行し、認証で落ちたときだけ helper を退避して 1 度だけやり直す（#524）。""" + cmd = ["git", "-C", str(worktree), *args] + first = subprocess.run(cmd, capture_output=True, text=True) + if first.returncode == 0 or args[0] not in ("push", "fetch"): + return first + fallback = _credential_fallback_args() + if not fallback: + return first + return subprocess.run(["git", "-C", str(worktree), *fallback, *args], + capture_output=True, text=True) + + +def push_fix(worktree: pathlib.Path | str, head_branch: str, + fix_commit: str | None) -> PushResult: + """現在の頭を送り先のブランチへ送り、報告されたコミットが載ったことを確かめる。 + + **ブランチ名だけを指定しない。** 作業ツリーが切り離された頭で作られている場合、 + ブランチ名だけの指定では現在の頭が送られないまま終了コード 0 で終わる。 + """ + if not fix_commit: + return PushResult(True, False, True, "コミットが無いため送らない") + if not (str(worktree or "") and head_branch): + return PushResult(False, False, False, + "送る先（作業ツリーとブランチ）が分からない") + pushed = _git(worktree, "push", "origin", f"HEAD:{head_branch}") + if pushed.returncode != 0: + return PushResult(False, False, False, + (pushed.stderr or pushed.stdout or "").strip()[:300]) + fetched = _git(worktree, "fetch", "origin", head_branch) + if fetched.returncode != 0: + return PushResult(False, True, False, + (fetched.stderr or "").strip()[:300]) + contains = _git(worktree, "merge-base", "--is-ancestor", fix_commit, "FETCH_HEAD") + if contains.returncode != 0: + return PushResult(False, True, False, + f"報告されたコミット {fix_commit} が origin/{head_branch} に載っていない") + return PushResult(True, True, True, "") + + +# ---------------- 部分命令 ---------------- + + +def _sh(*cmd: str, cwd: pathlib.Path | str | None = None) -> str: + r = subprocess.run(list(cmd), capture_output=True, text=True, cwd=cwd) + return r.stdout.strip() if r.returncode == 0 else "" + + +def queue_for(worktree: pathlib.Path) -> post_queue.Queue: + """待ち行列の置き場所。**引数では渡さない。** 作業ツリーの下の決まった名前から導く。""" + base = os.environ.get("CROSS_REVIEW_TMP_DIR") or str(worktree / TMP_DIRNAME) + return post_queue.Queue(pathlib.Path(base) / post_queue.QUEUE_DIRNAME) + + +class FixInputs(NamedTuple): + worktree: pathlib.Path + repo: str + head: str + result: pathlib.Path + fix: dict[str, Any] + + +def _resolve_fix_inputs(args: argparse.Namespace) -> tuple[FixInputs | None, str]: + """単独 fix 命令の入力を引数と既定値から解決する。 + + **リポジトリと頭は作業ツリーの中で解決する。** 呼び出し元の cwd が作業ツリーの + 外だと、`gh` が別のリポジトリを読むか解決に失敗し、頭が空のまま送信を飛ばす。 + """ + worktree = pathlib.Path(args.worktree or os.getcwd()).resolve() + repo = args.repo or _sh("gh", "repo", "view", "--json", "nameWithOwner", + "-q", ".nameWithOwner", cwd=worktree) + if not repo: + return None, "リポジトリを決められない（--repo を渡す）" + head = args.head or _sh("gh", "pr", "view", str(args.pr), "-R", repo, + "--json", "headRefName", "-q", ".headRefName", + cwd=worktree) + if not head: + # 送れない修正へ「対応しました」と返信しないため、返信へ進まず止める。 + return None, "送り先のブランチを決められない（--head を渡す）" + result = pathlib.Path(args.result) if args.result else ( + pathlib.Path(os.environ.get("CROSS_REVIEW_TMP_DIR") + or str(worktree / TMP_DIRNAME)) / f"fix-pr{args.pr}-result.json") + fix = _read_json(result) + if not fix: + return None, f"修正の結果ファイルを読めない: {result}" + return FixInputs(worktree, repo, head, result, fix), "" + + +def cmd_fix(args: argparse.Namespace) -> int: + inputs, error = _resolve_fix_inputs(args) + if inputs is None: + print(error, file=sys.stderr) + return 1 + worktree, repo, head, result, fix = inputs + + pushed = push_fix(worktree, head, fix.get("fix_commit") or fix.get("commit_sha")) + print(f"PUSHED={1 if pushed.pushed else 0} " + f"COMMIT_ON_HEAD={1 if pushed.contains else 0}") + if not pushed.ok: + print(pushed.detail, file=sys.stderr) + return 1 + + actor = args.actor or _sh("gh", "api", "user", "-q", ".login") or None + outcome = post_fix(queue_for(worktree), result, repo, int(args.pr), + round_no=args.round, actor=actor) + if outcome.summary_url: + print(f"POSTED summary_url={outcome.summary_url}") + print(f"REPLIED={outcome.replied} RESOLVED={outcome.resolved} " + f"QUEUED={outcome.queued}") + if outcome.failed: + print(outcome.detail, file=sys.stderr) + return 1 + return 0 + + +def main() -> None: + p = argparse.ArgumentParser(description="結果ファイルから投稿を組み立てて送る") + sub = p.add_subparsers(dest="cmd", required=True) + f = sub.add_parser("fix", help="修正の結果ファイルの返信・決着・まとめと送信") + f.add_argument("--repo") + f.add_argument("--pr", required=True) + f.add_argument("--result") + f.add_argument("--head") + f.add_argument("--worktree") + f.add_argument("--round", type=int) + f.add_argument("--actor") + f.set_defaults(func=cmd_fix) + args = p.parse_args() + sys.exit(args.func(args)) + + +if __name__ == "__main__": + main() diff --git a/plugins/ndf/scripts/lib/statefile.py b/plugins/ndf/scripts/lib/statefile.py index 623c79a2..6e15626b 100644 --- a/plugins/ndf/scripts/lib/statefile.py +++ b/plugins/ndf/scripts/lib/statefile.py @@ -41,16 +41,26 @@ def load(path: pathlib.Path) -> dict[str, Any]: return json.loads(path.read_text(encoding="utf-8")) -def save(path: pathlib.Path, state: dict[str, Any]) -> None: - """状態ファイルを原子的に書く。 +def write_json_atomic(path: pathlib.Path, data: Any) -> None: + """JSON を原子的に書く。 同じディレクトリへ一時ファイルを書いてから `replace` する。途中で落ちても - 半端な JSON が残らないため、再開時に必ず読める。 + 半端な JSON が残らないため、再開時に必ず読める。失敗は例外で返し、一時 + ファイルは残さない。 """ path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_suffix(".json.tmp") - tmp.write_text(json.dumps(state, indent=2, ensure_ascii=False), encoding="utf-8") - tmp.replace(path) + try: + tmp.write_text(json.dumps(data, indent=2, ensure_ascii=False), encoding="utf-8") + tmp.replace(path) + except BaseException: + tmp.unlink(missing_ok=True) + raise + + +def save(path: pathlib.Path, state: dict[str, Any]) -> None: + """状態ファイルを原子的に書く（`write_json_atomic`）。""" + write_json_atomic(path, state) # **差し込み口の失敗で保存の呼び出し側を止めない。** 状態は既に書けている。 for hook in list(_AFTER_SAVE): try: diff --git a/plugins/ndf/scripts/tests/test_post_queue.py b/plugins/ndf/scripts/tests/test_post_queue.py index cb928c2e..5c1e8b08 100644 --- a/plugins/ndf/scripts/tests/test_post_queue.py +++ b/plugins/ndf/scripts/tests/test_post_queue.py @@ -128,6 +128,29 @@ def test_flush_stops_at_corrupt_json_and_keeps_following_items( ] +def test_drop_removes_only_the_item_with_the_requested_sequence( + tmp_path: pathlib.Path, +) -> None: + """現状固定。指定した連番の項目だけを取り除く。""" + paths = [_write_item(tmp_path, seq) for seq in range(1, 4)] + queue = post_queue.Queue(tmp_path) + + assert queue.drop(2) is True + assert [path.name for path in queue.paths()] == [paths[0].name, paths[2].name] + + +@pytest.mark.parametrize("seq", [None, 99]) +def test_drop_keeps_items_when_the_sequence_does_not_match( + tmp_path: pathlib.Path, seq: int | None +) -> None: + """現状固定。連番が無い場合は何も取り除かない。""" + paths = [_write_item(tmp_path, item_seq) for item_seq in range(1, 3)] + queue = post_queue.Queue(tmp_path) + + assert queue.drop(seq) is False + assert [path.name for path in queue.paths()] == [path.name for path in paths] + + def test_post_succeeds_directly_when_queue_is_empty( tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -267,3 +290,206 @@ def fake_send(item): assert len(sent_items) == 1 assert sent_items[0]["seq"] == 1 + + +# ---------------- 拒まれ方の区別（#730） ---------------- + +# 実測した応答（2026-09-22、Pull Request #794）。要求ごとに全件が拒まれ、 +# `errors` は語をつないだ 1 つの文字列で、どの項目かは指さない。 +_UNRESOLVED_LINE = json.dumps({ + "message": "Unprocessable Entity", + "errors": ["Line could not be resolved"], + "status": "422", +}) +_UNRESOLVED_MANY = json.dumps({ + "message": "Unprocessable Entity", + "errors": ["Line could not be resolved, Path could not be resolved," + " and Line could not be resolved"], + "status": "422", +}) +_BAD_EVENT = json.dumps({ + "message": "Unprocessable Entity", + "errors": ["Variable $event of type PullRequestReviewEvent" + " was provided invalid value"], + "status": "422", +}) +_BAD_COMMIT = json.dumps({ + "message": "Unprocessable Entity", + "errors": ["The commitOID is not part of the pull request"], + "status": "422", +}) +_STDERR_422 = "gh: Unprocessable Entity (HTTP 422)\n" + + +def _attempt(stdout: str, stderr: str = _STDERR_422) -> Any: + return post_queue.Attempt(1, stdout, stderr) + + +@pytest.mark.parametrize("stdout", [_UNRESOLVED_LINE, _UNRESOLVED_MANY]) +def test_a_rejection_that_cannot_resolve_the_position_is_told_apart(stdout: str) -> None: + """行やファイルを解決できない拒まれ方だけを、退避の契機として見分ける。""" + assert post_queue.is_position_unresolved(_attempt(stdout)) is True + + +@pytest.mark.parametrize("stdout", [_BAD_EVENT, _BAD_COMMIT]) +def test_another_rejection_of_the_same_status_is_not_a_reason_to_move(stdout: str) -> None: + """判定の値の誤りと基準のコミットの誤りは、退避せず失敗として残す。""" + assert post_queue.is_position_unresolved(_attempt(stdout)) is False + + +def test_a_rejection_of_another_status_is_not_a_reason_to_move() -> None: + assert post_queue.is_position_unresolved( + post_queue.Attempt(1, '{"message":"Not Found"}', "gh: Not Found (HTTP 404)") + ) is False + + +def test_a_success_is_not_a_rejection() -> None: + assert post_queue.is_position_unresolved(post_queue.Attempt(0, "{}", "")) is False + + +@pytest.mark.parametrize("stdout", [_UNRESOLVED_LINE, _BAD_EVENT]) +def test_the_words_of_the_rejection_are_readable(stdout: str) -> None: + """応答の `errors` が文字列の列でも、失敗の説明に語が残る。""" + assert "could not be resolved" in _attempt(_UNRESOLVED_LINE).message + assert _attempt(stdout).message != "" + + +def test_a_rejection_that_cannot_resolve_the_position_is_not_a_rate_limit() -> None: + assert post_queue.is_rate_limited(_attempt(_UNRESOLVED_LINE)) is False + + +@pytest.mark.parametrize("item, expected", [ + ({"last_status": 422, "last_error": "Line could not be resolved"}, True), + ({"last_status": 422, "last_error": "Invalid event"}, False), + ({"last_status": 404, "last_error": "Line could not be resolved"}, False), + ({"last_status": None, "last_error": "Line could not be resolved"}, False), +]) +def test_a_queued_item_is_told_apart_by_its_status_and_words( + item: dict[str, Any], expected: bool) -> None: + """流した後に残った項目も、422 と位置の語がそろうときだけ位置の拒否と見る。""" + assert post_queue.rejected_by_position(item) is expected + + +# ---------------- 上限のときに待って再実行する（R1-005） ---------------- + +_OK = post_queue.Attempt(0, '{"id": 1}', "") +_RATE = post_queue.Attempt(1, "", "API rate limit exceeded (HTTP 429)") +_NORMAL_FAIL = post_queue.Attempt(1, "", "permission denied (HTTP 403)") + + +def _run_returning(responses: list[Any], calls: list[list[str]]): + """`run` の代わりに、応答列を順に返す疑似実装。呼ばれた cmd を記録する。""" + queue = list(responses) + + def fake_run(cmd, stdin=None): + calls.append(cmd) + return queue.pop(0) + + return fake_run + + +def _recording_sleep(waits: list[float]): + """時間を進めず、待った秒数だけ記録する疑似 sleep。""" + + def sleep(seconds): + waits.append(seconds) + + return sleep + + +def test_retry_returns_immediately_on_success( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """現状固定。最初の実行が成功したら、待たずにその結果を返す。""" + calls: list[list[str]] = [] + waits: list[float] = [] + monkeypatch.setattr(post_queue, "run", _run_returning([_OK], calls)) + monkeypatch.setattr(post_queue, "quota_remaining", lambda: 0) + + result = post_queue.retry(["gh", "pr", "create"], sleep=_recording_sleep(waits)) + + assert result is _OK + assert calls == [["gh", "pr", "create"]] + assert waits == [] + + +def test_retry_returns_immediately_on_a_normal_failure( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """現状固定。上限でない失敗は、待たずにそのまま返す。""" + calls: list[list[str]] = [] + waits: list[float] = [] + monkeypatch.setattr(post_queue, "run", _run_returning([_NORMAL_FAIL], calls)) + monkeypatch.setattr(post_queue, "quota_remaining", lambda: 100) + + result = post_queue.retry(["gh", "pr", "create"], sleep=_recording_sleep(waits)) + + assert result is _NORMAL_FAIL + assert calls == [["gh", "pr", "create"]] + assert waits == [] + + +def test_retry_waits_and_re_runs_until_it_succeeds( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """現状固定。上限のあいだ待って再実行し、成功したらその結果を返す。""" + calls: list[list[str]] = [] + waits: list[float] = [] + monkeypatch.setattr( + post_queue, "run", _run_returning([_RATE, _RATE, _OK], calls) + ) + + result = post_queue.retry( + ["gh", "pr", "create"], + max_wait=900.0, + interval=30.0, + sleep=_recording_sleep(waits), + ) + + assert result is _OK + assert len(calls) == 3 + assert waits == [30.0, 30.0] + assert sum(waits) <= 900.0 + + +def test_retry_returns_the_last_rate_limited_attempt_when_the_wait_cap_is_reached( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """現状固定。待機の上限に達したら、最後の上限応答を返す。""" + calls: list[list[str]] = [] + waits: list[float] = [] + last_rate = post_queue.Attempt(1, "", "API rate limit exceeded (HTTP 429)") + responses = [_RATE, _RATE, _RATE, last_rate] + monkeypatch.setattr(post_queue, "run", _run_returning(responses, calls)) + + result = post_queue.retry( + ["gh", "pr", "create"], + max_wait=90.0, + interval=30.0, + sleep=_recording_sleep(waits), + ) + + assert result is last_rate + assert len(calls) == 4 + assert waits == [30.0, 30.0, 30.0] + assert sum(waits) <= 90.0 + + +def test_retry_returns_the_first_rate_limited_attempt_when_interval_is_zero( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """現状固定。待機間隔が 0 なら、待機も再実行もせず最初の応答を返す。""" + calls: list[list[str]] = [] + waits: list[float] = [] + first_rate = post_queue.Attempt(1, "", "API rate limit exceeded (HTTP 429)") + monkeypatch.setattr(post_queue, "run", _run_returning([first_rate], calls)) + + result = post_queue.retry( + ["gh", "pr", "create"], + interval=0, + sleep=_recording_sleep(waits), + ) + + assert result is first_rate + assert calls == [["gh", "pr", "create"]] + assert waits == [] diff --git a/plugins/ndf/scripts/tests/test_result_posts.py b/plugins/ndf/scripts/tests/test_result_posts.py new file mode 100644 index 00000000..117e16f6 --- /dev/null +++ b/plugins/ndf/scripts/tests/test_result_posts.py @@ -0,0 +1,749 @@ +"""結果ファイルを投稿へ変える層（#730 #583）。 + +担当が書いた指摘の控えと結果ファイルを読み、GitHub へ送る投稿を組み立てる。 +**本文は引数にも標準出力にも出さない。** 受け取るのはファイルのパスだけで、本文は +この層の中だけを通る。 + +| 何を確かめるか | 受け入れ条件 | +| --- | --- | +| 控えと結果からレビューの投稿が組み立つ | AC5 | +| 本文が引数に現れない | AC5 | +| 自分の Pull Request では送った形だけを落とす | AC32 | +| 位置を解決できない拒まれ方で、インラインを総評へ退避する | AC16 | +| 同じ状態の別の拒まれ方は退避せず失敗として残す | AC16 | +| インラインが 0 件でも結果なしにしない | AC17 | +| 返信・決着・まとめが積まれる | AC7 | +| 送信は現在の頭を指定し、載ったことを確かめる | AC8・AC9 | +""" +from __future__ import annotations + +import inspect +import json +import os +import pathlib +import subprocess +import sys + +import pytest + +LIB = pathlib.Path(__file__).resolve().parents[1] / "lib" +if str(LIB) not in sys.path: + sys.path.insert(0, str(LIB)) + +import post_queue # noqa: E402 +import result_posts # noqa: E402 + +REPO = "o/r" +PR = 730 +ROUND = 3 +SEAT = "codex" +SHA = "1" * 40 +ACTOR = "takemi" + + +# ---------------- 偽の `gh` ---------------- + +_FAKE_GH = '''#!/usr/bin/env python3 +import json, os, sys + +argv = sys.argv[1:] +joined = " ".join(argv) +stdin = "" if sys.stdin.isatty() else sys.stdin.read() +log = os.environ["GH_FAKE_LOG"] +with open(log, "a", encoding="utf-8") as f: + f.write(json.dumps({"argv": argv, "stdin": stdin}, ensure_ascii=False) + "\\n") +rules_file = os.environ.get("GH_FAKE_RULES") +rules = json.load(open(rules_file, encoding="utf-8")) if rules_file else [] +prior = 0 +with open(log, encoding="utf-8") as f: + prior = sum(1 for line in f if line.strip()) - 1 +for rule in rules: + if "calls_lt" in rule and prior >= int(rule["calls_lt"]): + continue + if rule.get("match", "") in joined: + sys.stdout.write(rule.get("stdout", "")) + sys.stderr.write(rule.get("stderr", "")) + sys.exit(int(rule.get("exit", 0))) +sys.stdout.write("[]") +''' + + +class FakeGh: + def __init__(self, log: pathlib.Path, rules: pathlib.Path, monkeypatch) -> None: + self.log, self.rules, self._mp = log, rules, monkeypatch + + def set_rules(self, rules: list[dict]) -> None: + self.rules.write_text(json.dumps(rules), encoding="utf-8") + self._mp.setenv("GH_FAKE_RULES", str(self.rules)) + + def calls(self) -> list[dict]: + if not self.log.exists(): + return [] + return [json.loads(line) for line in + self.log.read_text(encoding="utf-8").splitlines() if line.strip()] + + def joined(self) -> list[str]: + return [" ".join(c["argv"]) for c in self.calls()] + + def sent(self) -> list[dict]: + """状態を変える呼び出しの本文だけを取り出す。""" + out = [] + for c in self.calls(): + if "--method POST" in " ".join(c["argv"]) and c["stdin"]: + out.append(json.loads(c["stdin"])) + return out + + +@pytest.fixture() +def fake_gh(monkeypatch, tmp_path) -> FakeGh: + bindir = tmp_path / "fake-bin" + bindir.mkdir(exist_ok=True) + script = bindir / "gh" + script.write_text(_FAKE_GH, encoding="utf-8") + script.chmod(0o755) + monkeypatch.setenv("PATH", f"{bindir}{os.pathsep}{os.environ.get('PATH', '')}") + monkeypatch.setenv("GH_FAKE_LOG", str(tmp_path / "gh-calls.log")) + monkeypatch.delenv("GH_FAKE_RULES", raising=False) + return FakeGh(tmp_path / "gh-calls.log", tmp_path / "gh-rules.json", monkeypatch) + + +# ---------------- 控えと結果ファイル ---------------- + +def _files(tmp_path: pathlib.Path, comments: list[dict] | None = None, + summary: str = "設計の筋は通っている。", event: str = "REQUEST_CHANGES", + ) -> tuple[pathlib.Path, pathlib.Path]: + payload = tmp_path / f"{SEAT}-review-pr{PR}-round{ROUND}-payload.json" + result = tmp_path / f"{SEAT}-review-pr{PR}-result.json" + if comments is None: + comments = [ + {"path": "a.py", "line": 12, "body": "[major / 正確性] 戻り値を確かめる", + "severity": "major"}, + {"path": "b.py", "line": 34, "body": "[minor / 可読性] 名前を揃える", + "severity": "minor"}, + ] + payload.write_text(json.dumps({"summary": summary, "comments": comments}, + ensure_ascii=False), encoding="utf-8") + result.write_text(json.dumps( + {"event": event, "by_severity": {"critical": 0, "major": 1, "minor": 1, "nit": 0}}, + ensure_ascii=False), encoding="utf-8") + return payload, result + + +def _review_items(tmp_path, **kw): + payload, result = _files(tmp_path, **kw) + return result_posts.review_posts( + payload, result, repo=REPO, pr=PR, round_no=ROUND, seat=SEAT, + head_sha=SHA, is_own_pr=False) + + +# ---------------- 組み立て ---------------- + +def test_a_review_is_built_from_the_note_and_the_result(tmp_path) -> None: + items = _review_items(tmp_path) + + assert [i["kind"] for i in items] == ["review-post"] + fields = items[0]["fields"] + assert fields["event"] == "REQUEST_CHANGES" + assert fields["commit_id"] == SHA + assert [c["path"] for c in fields["comments"]] == ["a.py", "b.py"] + + +def test_the_first_line_carries_the_round_and_the_seat(tmp_path) -> None: + body = _review_items(tmp_path)[0]["fields"]["body"] + + assert body.splitlines()[0] == \ + f"## 🤖 cross-review | round {ROUND} | {SEAT} | REQUEST_CHANGES" + assert post_queue.review_match_key(body) == \ + f"## 🤖 cross-review | round {ROUND} | {SEAT}|" + + +def test_the_body_is_never_an_argument() -> None: + """本文は引数として渡さない。どの関数もファイルのパスを受け取る。""" + for fn in (result_posts.review_posts, result_posts.fix_posts): + names = list(inspect.signature(fn).parameters) + assert "body" not in names and "payload" not in names + assert "payload_path" in inspect.signature(result_posts.review_posts).parameters + + +def test_only_what_is_sent_is_downgraded_on_ones_own_pull_request(tmp_path) -> None: + """自分の Pull Request では送った形だけを落とし、本来の判定は落とさない（AC32）。""" + payload, result = _files(tmp_path) + items = result_posts.review_posts( + payload, result, repo=REPO, pr=PR, round_no=ROUND, seat=SEAT, + head_sha=SHA, is_own_pr=True) + + fields = items[0]["fields"] + assert fields["event"] == "COMMENT" + assert fields["body"].splitlines()[0].endswith("| REQUEST_CHANGES") + assert items[0]["extra"]["intent"] == "REQUEST_CHANGES" + assert items[0]["extra"]["posted_as"] == "COMMENT" + + +def test_a_finding_without_a_position_goes_to_the_summary(tmp_path) -> None: + """位置を持たない指摘は、指す先が無いので総評へ入れる。""" + items = _review_items(tmp_path, comments=[ + {"body": "[major / 設計] 層の分け方を見直す", "severity": "major"}, + {"path": "a.py", "line": 12, "body": "[minor / 可読性] 名前を揃える", + "severity": "minor"}, + ]) + + fields = items[0]["fields"] + assert [c["path"] for c in fields["comments"]] == ["a.py"] + assert "層の分け方を見直す" in fields["body"] + assert items[0]["extra"]["inline"] == 1 + assert items[0]["extra"]["body"] == 1 + + +@pytest.mark.parametrize("line", ["L42", "40-45", "", " ", True, 1.5, [12]]) +def test_a_finding_whose_line_is_not_an_integer_goes_to_the_summary( + tmp_path, line) -> None: + """行が整数にならない指摘は、例外で落とさず総評へ入れる（外部入力のため）。""" + items = _review_items(tmp_path, comments=[ + {"path": "a.py", "line": line, "body": "[major / 正確性] 行が壊れている", + "severity": "major"}, + {"path": "b.py", "line": "34", "body": "[minor / 可読性] 名前を揃える", + "severity": "minor"}, + ]) + + fields = items[0]["fields"] + assert fields["comments"] == [ + {"path": "b.py", "line": 34, "side": "RIGHT", + "body": "[minor / 可読性] 名前を揃える"}] + assert "行が壊れている" in fields["body"] + assert items[0]["extra"]["inline"] == 1 + assert items[0]["extra"]["body"] == 1 + + +# ---------------- 送信と退避 ---------------- + +def _queue(tmp_path) -> post_queue.Queue: + return post_queue.Queue(tmp_path / "pending") + + +def _post_review(tmp_path, **kw): + payload, result = _files(tmp_path, **kw) + return result_posts.post_review( + _queue(tmp_path), payload, result, repo=REPO, pr=PR, round_no=ROUND, + seat=SEAT, head_sha=SHA, is_own_pr=False, actor=ACTOR), payload + + +_REJECT_POSITION = { + "match": "pulls/730/reviews", "exit": 1, + "stdout": json.dumps({"message": "Unprocessable Entity", + "errors": ["Line could not be resolved"], "status": "422"}), + "stderr": "gh: Unprocessable Entity (HTTP 422)\n", +} +_REJECT_EVENT = { + "match": "pulls/730/reviews", "exit": 1, + "stdout": json.dumps({"message": "Unprocessable Entity", + "errors": ["Variable $event of type PullRequestReviewEvent" + " was provided invalid value"], "status": "422"}), + "stderr": "gh: Unprocessable Entity (HTTP 422)\n", +} +_ACCEPT = {"match": "pulls/730/reviews", "stdout": json.dumps( + {"id": 99, "html_url": "https://x/pull/730#pullrequestreview-99"})} +_RATE_LIMITED = { + "match": "pulls/730/reviews", "exit": 1, + "stdout": json.dumps({"message": "API rate limit exceeded"}), + "stderr": "gh: API rate limit exceeded (HTTP 429)\n", +} + + +def test_the_inlines_move_to_the_summary_when_the_position_is_not_resolved( + tmp_path, fake_gh) -> None: + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + dict(_REJECT_POSITION, calls_lt=3), + _ACCEPT, + ]) + + outcome, payload = _post_review(tmp_path) + + assert outcome.review_url == "https://x/pull/730#pullrequestreview-99" + assert outcome.posted_inline == 0 + assert outcome.posted_body == 2 + assert outcome.queued == 0 + # 2 度目の要求はインラインを持たず、指摘は総評に入る。 + last = fake_gh.sent()[-1] + assert "comments" not in last + assert "戻り値を確かめる" in last["body"] + # 控えには送れた先が残る。 + note = json.loads(payload.read_text(encoding="utf-8")) + assert [c["posted_to"] for c in note["comments"]] == ["body", "body"] + + +def test_another_rejection_of_the_same_status_is_not_moved(tmp_path, fake_gh) -> None: + """判定の値の誤りは退避の契機にしない。失敗として残す。""" + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + _REJECT_EVENT, + ]) + + outcome, _ = _post_review(tmp_path) + + assert outcome.failed is True + assert outcome.review_url is None + + +def test_a_position_rejection_of_an_earlier_item_is_not_taken_as_ours( + tmp_path, fake_gh) -> None: + """先に積まれた項目の位置エラーで、今回の分を退避しない。 + + 先客を消して今回分を二重に積むと、未投稿のまま控えへ送れた先を書き、取り込みを + 成功扱いにしてしまう。今回分が送れていない限り、失敗として残す。 + """ + earlier = post_queue.enqueue( + _queue(tmp_path), "review-post", REPO, PR, + {"body": f"## 🤖 cross-review | round {ROUND} | agy | COMMENT\n", + "event": "COMMENT", + "comments": [{"path": "c.py", "line": 9, "side": "RIGHT", "body": "先客"}]}, + actor=ACTOR, extra={"ident": f"agy-r{ROUND}"}) + earlier_seq = post_queue.read_item(earlier)["seq"] + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + _REJECT_POSITION, + ]) + + outcome, payload = _post_review(tmp_path) + + assert outcome.failed is True + assert outcome.queued == 1 + assert outcome.review_url is None + note = json.loads(payload.read_text(encoding="utf-8")) + assert all("posted_to" not in comment for comment in note["comments"]) + # 先客は残り、今回分は 1 件だけ（退避した写しを足さない）。 + queued = [item for _, item in _queue(tmp_path).items()] + assert [i["seq"] for i in queued][0] == earlier_seq + assert len(queued) == 2 + assert [i["extra"].get("agent") for i in queued] == [None, SEAT] + + +def test_a_failed_write_of_the_note_keeps_it_whole_and_stops_the_take_in( + tmp_path, fake_gh, monkeypatch) -> None: + """控えの書き戻しが途中で落ちても控えは元のまま読め、取り込みは失敗として止まる。 + + 半端な控えを残すと、再実行で読めずに空として扱われ、記録済みの指摘を 0 件で + 置き換える。 + """ + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + _ACCEPT, + ]) + payload, result = _files(tmp_path) + original = payload.read_text(encoding="utf-8") + real_write = pathlib.Path.write_text + + def half_write(self, data, *a, **kw): + if "posted_to" in data: + real_write(self, data[: len(data) // 2], *a, **kw) + raise OSError("disk full") + return real_write(self, data, *a, **kw) + + monkeypatch.setattr(pathlib.Path, "write_text", half_write) + outcome = result_posts.post_review( + _queue(tmp_path), payload, result, repo=REPO, pr=PR, round_no=ROUND, + seat=SEAT, head_sha=SHA, is_own_pr=False, actor=ACTOR) + + assert outcome.failed is True + assert "控え" in outcome.detail + assert payload.read_text(encoding="utf-8") == original + assert [p.name for p in payload.parent.iterdir() if p.name.endswith(".tmp")] == [] + + +def test_a_rate_limited_review_remains_queued_without_marking_the_note( + tmp_path, fake_gh) -> None: + """現状固定。上限時は失敗にせず、未投稿の要求と控えをそのまま残す。""" + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + _RATE_LIMITED, + ]) + + outcome, payload = _post_review(tmp_path) + + assert outcome.queued == 1 + assert outcome.failed is False + assert outcome.review_url is None + assert outcome.posted_inline == 0 + assert outcome.posted_body == 0 + note = json.loads(payload.read_text(encoding="utf-8")) + assert all("posted_to" not in comment for comment in note["comments"]) + queued = _queue(tmp_path).items() + assert len(queued) == 1 + assert queued[0][1]["kind"] == "review-post" + assert queued[0][1]["attempts"] == 1 + + +def test_a_review_that_is_already_on_github_is_not_posted_again( + tmp_path, fake_gh) -> None: + """投稿の後・記録の前に止まった実行をやり直しても、レビューは増えない（AC12）。""" + head = f"## 🤖 cross-review | round {ROUND} | {SEAT} | APPROVE" + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", + "stdout": json.dumps([{"user": {"login": ACTOR}, "state": "APPROVED", + "body": head + "\n\n先客", "id": 42, + "html_url": "https://x/pull/730#pullrequestreview-42"}])}, + ]) + + outcome, _ = _post_review(tmp_path) + + assert outcome.review_url == "https://x/pull/730#pullrequestreview-42" + assert [c for c in fake_gh.joined() if "--method POST" in c] == [] + + +_STARTED = "2026-09-22T12:00:00+09:00" + + +def _earlier_run_review(submitted_at: str) -> dict: + """同じラウンド番号・同じ席の、先に出ていたレビュー。""" + head = f"## 🤖 cross-review | round {ROUND} | {SEAT} | APPROVE" + return {"match": "pulls/730/reviews?", + "stdout": json.dumps([{"user": {"login": ACTOR}, "state": "APPROVED", + "body": head + "\n\n前の実行", "id": 42, + "submitted_at": submitted_at, + "html_url": "https://x/pull/730#pullrequestreview-42"}])} + + +def test_a_review_of_an_earlier_run_is_not_taken_as_this_one(tmp_path, fake_gh) -> None: + """回し直した実行では、ラウンドの開始より前のレビューを同じ投稿と読まない。 + + ラウンドの番号は実行ごとに 1 から数え直すため、番号と席だけでは実行をまたいで + 一意にならない。 + """ + fake_gh.set_rules([_earlier_run_review("2026-09-22T02:59:59Z"), _ACCEPT]) + + payload, result = _files(tmp_path) + outcome = result_posts.post_review( + _queue(tmp_path), payload, result, repo=REPO, pr=PR, round_no=ROUND, + seat=SEAT, head_sha=SHA, is_own_pr=False, actor=ACTOR, since=_STARTED) + + assert outcome.review_url == "https://x/pull/730#pullrequestreview-99" + assert len([c for c in fake_gh.joined() if "--method POST" in c]) == 1 + + +def test_a_review_sent_after_the_round_started_is_still_not_sent_again( + tmp_path, fake_gh) -> None: + """同じ実行の中で送った後に止まった分は、開始時刻で絞っても見つかる（AC12）。""" + fake_gh.set_rules([_earlier_run_review("2026-09-22T03:00:05Z")]) + + payload, result = _files(tmp_path) + outcome = result_posts.post_review( + _queue(tmp_path), payload, result, repo=REPO, pr=PR, round_no=ROUND, + seat=SEAT, head_sha=SHA, is_own_pr=False, actor=ACTOR, since=_STARTED) + + assert outcome.review_url == "https://x/pull/730#pullrequestreview-42" + assert [c for c in fake_gh.joined() if "--method POST" in c] == [] + + +def test_a_note_with_findings_is_not_a_missing_result(tmp_path, fake_gh) -> None: + """インラインとして送れたものが 0 件でも、その担当は結果なしにならない（AC17）。""" + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + dict(_REJECT_POSITION, calls_lt=3), + _ACCEPT, + ]) + + outcome, _ = _post_review(tmp_path) + + assert outcome.findings == 2 + assert outcome.failed is False + + +def test_post_review_falls_back_to_fragment_url_when_response_has_id_only( + tmp_path, fake_gh) -> None: + """現状固定。応答に html_url がなく id だけのとき、review_url をフラグメントで補う。""" + fake_gh.set_rules([ + {"match": "pulls/730/reviews?", "stdout": "[]"}, + {"match": "pulls/730/reviews", "stdout": json.dumps({"id": 99})}, + ]) + + outcome, _ = _post_review(tmp_path) + + assert outcome.review_url == "#pullrequestreview-99" + assert outcome.posted_inline == 2 + assert outcome.posted_body == 0 + assert outcome.queued == 0 + assert outcome.failed is False + + +# ---------------- 修正の投稿 ---------------- + +def _fix_file(tmp_path, **over) -> pathlib.Path: + data = { + "pr": PR, + "fix_commit": "abc1234", + "ci_status": "SUCCESS", + "fixed_count": 2, + "by_severity": {"critical": 0, "major": 2, "minor": 0, "nit": 0}, + "resolved_threads": [ + {"thread_id": "PRRT_a", "comment_id": 111, "path": "a.py", "line": 12}, + {"thread_id": "PRRT_b", "comment_id": 222, "path": "b.py", "line": 34}, + ], + "deferred": [ + {"comment_id": 333, "thread_id": "PRRT_c", "severity": "nit", + "summary": "末尾の書き方", "reason_for_deferral": "好みの範囲"}, + ], + "rejected": [ + {"comment_id": 444, "path": "c.py", "line": 7, "severity": "minor", + "summary": "引用の形", "reason_for_rejection": "意図して展開している"}, + ], + } + data.update(over) + path = tmp_path / f"fix-pr{PR}-result.json" + path.write_text(json.dumps(data, ensure_ascii=False), encoding="utf-8") + return path + + +def test_the_take_in_of_a_fix_builds_replies_resolves_and_a_summary(tmp_path) -> None: + items = result_posts.fix_posts(_fix_file(tmp_path), repo=REPO, pr=PR, + round_no=ROUND) + + kinds = [i["kind"] for i in items] + assert kinds.count("review-reply") == 4 # 決着 2 + 見送り 1 + 却下 1 + assert kinds.count("thread-resolve") == 2 + assert kinds.count("pr-comment") == 1 + assert kinds[-1] == "pr-comment" # まとめは最後 + + +def test_the_summary_carries_the_round_in_the_head_of_the_body(tmp_path) -> None: + """同じラウンドのまとめを 2 度積んでも増えないよう、鍵になる先頭へ入れる（AC14）。""" + items = result_posts.fix_posts(_fix_file(tmp_path), repo=REPO, pr=PR, + round_no=ROUND) + body = [i for i in items if i["kind"] == "pr-comment"][0]["fields"]["body"] + + assert f"round {ROUND}" in body[:post_queue.BODY_MATCH_CHARS] + assert "abc1234" in body[:post_queue.BODY_MATCH_CHARS] + + +def test_a_reply_points_at_the_comment_it_answers(tmp_path) -> None: + items = result_posts.fix_posts(_fix_file(tmp_path), repo=REPO, pr=PR, + round_no=ROUND) + targets = sorted(i["fields"]["in_reply_to"] for i in items + if i["kind"] == "review-reply") + + assert targets == [111, 222, 333, 444] + + +def test_a_fix_without_threads_still_posts_the_summary(tmp_path) -> None: + items = result_posts.fix_posts( + _fix_file(tmp_path, resolved_threads=[], deferred=[], rejected=[]), + repo=REPO, pr=PR, round_no=ROUND) + + assert [i["kind"] for i in items] == ["pr-comment"] + + +def test_fix_posts_skips_replies_with_non_numeric_comment_ids(tmp_path) -> None: + """現状固定。不正な返信先を飛ばしても、決着とまとめは組み立てる。""" + items = result_posts.fix_posts( + _fix_file( + tmp_path, + resolved_threads=[{"comment_id": "invalid", "thread_id": "PRRT_a"}], + deferred=[{"comment_id": "invalid", "thread_id": "PRRT_b"}], + rejected=[{"comment_id": "invalid", "thread_id": "PRRT_c"}], + ), + repo=REPO, + pr=PR, + round_no=ROUND, + ) + + assert [item["kind"] for item in items] == ["thread-resolve", "pr-comment"] + assert items[0]["fields"] == {"thread_id": "PRRT_a"} + assert items[0]["extra"] == {"ident": "resolve-PRRT_a"} + assert items[1]["extra"] == {"ident": f"fix-summary-{ROUND}"} + assert "決着: 1 件 / 見送り: 1 件 / 却下: 1 件" in items[1]["fields"]["body"] + + +# ---------------- 送信 ---------------- + +def _repo_with_remote(tmp_path) -> tuple[pathlib.Path, pathlib.Path]: + remote = tmp_path / "remote.git" + subprocess.run(["git", "init", "--bare", "-b", "main", str(remote)], + check=True, capture_output=True) + work = tmp_path / "work" + subprocess.run(["git", "clone", str(remote), str(work)], + check=True, capture_output=True) + for key, value in (("user.email", "t@example.com"), ("user.name", "t")): + subprocess.run(["git", "-C", str(work), "config", key, value], check=True) + (work / "a.txt").write_text("1\n", encoding="utf-8") + subprocess.run(["git", "-C", str(work), "add", "-A"], check=True) + subprocess.run(["git", "-C", str(work), "commit", "-m", "1"], + check=True, capture_output=True) + subprocess.run(["git", "-C", str(work), "push", "origin", "main"], + check=True, capture_output=True) + return work, remote + + +def _head(work: pathlib.Path) -> str: + return subprocess.run(["git", "-C", str(work), "rev-parse", "HEAD"], + check=True, capture_output=True, text=True).stdout.strip() + + +def test_the_push_names_the_current_head(tmp_path) -> None: + """切り離された頭でも現在の頭が送られる（AC8）。""" + work, remote = _repo_with_remote(tmp_path) + subprocess.run(["git", "-C", str(work), "checkout", "--detach"], + check=True, capture_output=True) + (work / "a.txt").write_text("2\n", encoding="utf-8") + subprocess.run(["git", "-C", str(work), "commit", "-am", "2"], + check=True, capture_output=True) + commit = _head(work) + + outcome = result_posts.push_fix(work, "main", commit) + + assert outcome.ok is True and outcome.contains is True + remote_head = subprocess.run( + ["git", "-C", str(remote), "rev-parse", "refs/heads/main"], + check=True, capture_output=True, text=True).stdout.strip() + assert remote_head == commit + + +def test_the_push_fails_when_the_reported_commit_is_not_on_the_branch(tmp_path) -> None: + """報告されたコミットが送り先に載っていなければ止まる（AC9）。""" + work, _ = _repo_with_remote(tmp_path) + + outcome = result_posts.push_fix(work, "main", "0" * 40) + + assert outcome.ok is False and outcome.contains is False + + +def test_nothing_is_pushed_when_no_commit_was_made(tmp_path) -> None: + work, _ = _repo_with_remote(tmp_path) + + outcome = result_posts.push_fix(work, "main", None) + + assert outcome.ok is True and outcome.pushed is False + + +def test_the_push_fails_without_a_destination() -> None: + outcome = result_posts.push_fix("", "", "abc1234") + + assert outcome.ok is False and outcome.pushed is False + + +def test_a_rejected_push_stops_before_checking_the_branch(tmp_path) -> None: + """送信そのものが拒まれたら、載ったかを確かめずに理由を残して止まる。""" + work, _ = _repo_with_remote(tmp_path) + subprocess.run(["git", "-C", str(work), "remote", "set-url", "origin", + str(tmp_path / "missing.git")], check=True) + + outcome = result_posts.push_fix(work, "main", _head(work)) + + assert outcome[:3] == (False, False, False) + assert outcome.detail != "" + + +# ---------------- 単独で使う口 ---------------- + +def test_the_standalone_command_pushes_and_posts_with_the_same_layer( + tmp_path, fake_gh, monkeypatch) -> None: + """単独の `fix` も同じ層を使い、1 行のコマンドで送信と投稿を終える（AC21・AC22）。""" + work, remote = _repo_with_remote(tmp_path) + (work / "a.txt").write_text("2\n", encoding="utf-8") + subprocess.run(["git", "-C", str(work), "commit", "-am", "2"], + check=True, capture_output=True) + fix = _fix_file(tmp_path, fix_commit=_head(work)) + fake_gh.set_rules([ + {"match": "issues/730/comments?", "stdout": "[]"}, + {"match": "pulls/730/comments?", "stdout": "[]"}, + {"match": "reviewThreads", "stdout": "PRRT_a\nPRRT_b\n"}, + {"match": "issues/730/comments", "stdout": json.dumps( + {"id": 7, "html_url": "https://x/pull/730#issuecomment-7"})}, + {"match": "", "stdout": "{}"}, + ]) + monkeypatch.delenv("CROSS_REVIEW_TMP_DIR", raising=False) + + r = subprocess.run( + [sys.executable, str(LIB / "result_posts.py"), "fix", "--repo", REPO, + "--pr", str(PR), "--result", str(fix), "--head", "main", + "--worktree", str(work), "--round", str(ROUND), "--actor", ACTOR], + capture_output=True, text=True, env=os.environ.copy()) + + assert r.returncode == 0, r.stderr + assert "PUSHED=1 COMMIT_ON_HEAD=1" in r.stdout + assert "POSTED summary_url=https://x/pull/730#issuecomment-7" in r.stdout + assert "REPLIED=4 RESOLVED=2 QUEUED=0" in r.stdout + # 本文は出さない。 + assert "好みの範囲" not in r.stdout + + +def test_a_deferred_thread_marked_to_resolve_is_resolved(tmp_path) -> None: + """最終スイープは見送りも決着させる。要素の `resolve` が真なら決着を積む。""" + items = result_posts.fix_posts( + _fix_file(tmp_path, resolved_threads=[], rejected=[], deferred=[ + {"comment_id": 333, "thread_id": "PRRT_c", "resolve": True, + "reason_for_deferral": "好みの範囲"}]), + repo=REPO, pr=PR) + + assert [i["kind"] for i in items] == ["review-reply", "thread-resolve", "pr-comment"] + assert items[1]["fields"]["thread_id"] == "PRRT_c" + + +def test_a_deferred_thread_is_not_resolved_by_default(tmp_path) -> None: + items = result_posts.fix_posts( + _fix_file(tmp_path, resolved_threads=[], rejected=[]), repo=REPO, pr=PR) + + assert "thread-resolve" not in [i["kind"] for i in items] + + +def test_the_standalone_command_resolves_repo_and_head_in_the_worktree( + tmp_path, monkeypatch) -> None: + """`--worktree` を渡したら、リポジトリと頭の解決も作業ツリーの中で行う。 + + 呼び出し元の cwd が作業ツリーの外でも、`gh` が別のリポジトリを読まないため。 + """ + work = tmp_path / "work" + work.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + monkeypatch.chdir(outside) + calls: list[tuple[list[str], str | None]] = [] + + def fake_run(cmd, **kw): + calls.append((list(cmd), kw.get("cwd"))) + inside = kw.get("cwd") is not None and \ + pathlib.Path(kw["cwd"]).resolve() == work.resolve() + out = "" + if inside and cmd[:3] == ["gh", "repo", "view"]: + out = REPO + elif inside and cmd[:3] == ["gh", "pr", "view"]: + out = "feat/x" + return subprocess.CompletedProcess(cmd, 0, stdout=out, stderr="") + + monkeypatch.setattr(result_posts.subprocess, "run", fake_run) + fix = _fix_file(tmp_path) + args = result_posts.argparse.Namespace( + repo=None, pr=str(PR), result=str(fix), head=None, worktree=str(work)) + + inputs, error = result_posts._resolve_fix_inputs(args) + + assert error == "" and inputs is not None + assert (inputs.repo, inputs.head) == (REPO, "feat/x") + pr_view = next(c for c, _ in calls if c[:3] == ["gh", "pr", "view"]) + assert pr_view[pr_view.index("-R") + 1] == REPO + + +def test_the_standalone_command_stops_when_the_branch_is_not_known( + tmp_path, monkeypatch, capsys) -> None: + """送り先のブランチを決められないときは、返信へ進まず止める。 + + 送っていない修正へ「対応しました」と返信しないため。 + """ + work = tmp_path / "work" + work.mkdir() + calls: list[list[str]] = [] + + def fake_run(cmd, **kw): + calls.append(list(cmd)) + return subprocess.CompletedProcess(cmd, 1, stdout="", stderr="no pr") + + monkeypatch.setattr(result_posts.subprocess, "run", fake_run) + monkeypatch.delenv("CROSS_REVIEW_TMP_DIR", raising=False) + fix = _fix_file(tmp_path) + args = result_posts.argparse.Namespace( + repo=REPO, pr=str(PR), result=str(fix), head=None, worktree=str(work), + round=ROUND, actor=ACTOR) + + assert result_posts.cmd_fix(args) == 1 + assert "ブランチ" in capsys.readouterr().err + # 返信・決着・まとめは 1 件も呼ばず、待ち行列にも積まない。 + assert [c for c in calls if c[:2] == ["gh", "api"]] == [] + assert not (work / result_posts.TMP_DIRNAME).exists() diff --git a/plugins/ndf/scripts/tests/test_statefile_emit.py b/plugins/ndf/scripts/tests/test_statefile_emit.py index 32e610a4..b260216a 100644 --- a/plugins/ndf/scripts/tests/test_statefile_emit.py +++ b/plugins/ndf/scripts/tests/test_statefile_emit.py @@ -42,3 +42,43 @@ def test_emit_joins_a_list_into_one_space_separated_word(mod, capsys) -> None: mod.emit(VALUES=["a b", "c"]) assert shlex.split(capsys.readouterr().out) == ["VALUES=a b c"] + + +def test_register_after_save_calls_the_same_hook_only_once(mod, tmp_path) -> None: + """現状固定。同じ差し込み口を 2 度登録しても保存後に 1 度だけ呼ぶ。""" + calls = [] + + def hook(path, state): + calls.append((path, state)) + + mod.register_after_save(hook) + mod.register_after_save(hook) + try: + path = tmp_path / "state.json" + state = {"round": 2} + mod.save(path, state) + finally: + mod.unregister_after_save(hook) + + assert calls == [(path, state)] + + +def test_save_writes_state_creates_parent_directory_and_calls_registered_hook(mod, tmp_path) -> None: + """現状固定。未作成の親ディレクトリを作って保存し、登録した差し込み口へ引数を渡す。""" + calls = [] + + def hook(path, state): + calls.append((path, state)) + + mod.register_after_save(hook) + try: + path = tmp_path / "nested" / "parent" / "state.json" + state = {"a": 1} + mod.save(path, state) + loaded = mod.load(path) + finally: + mod.unregister_after_save(hook) + + assert loaded == state + assert calls == [(path, state)] + diff --git a/plugins/ndf/skills/cross-review/SKILL.md b/plugins/ndf/skills/cross-review/SKILL.md index dbfac2d4..67a9eb5e 100644 --- a/plugins/ndf/skills/cross-review/SKILL.md +++ b/plugins/ndf/skills/cross-review/SKILL.md @@ -50,8 +50,8 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲 | 観点 | 方針 | |---|---| -| レビュー投稿 | **AI 自身が `gh api` で PR に直接投稿**。メインはペイロードを保持しない | -| 投稿の確認 | **申告されたコメント数を GitHub 側と突き合わせる**。投稿が届いていなければ中断する（取得できない場合は申告を採用） | +| 投稿の担い手 | **GitHub と git へ書くのはレビューを回す側だけ**（#730）。担当は指摘の控えと結果ファイルを書き、取り込み（`read-result`）が組み立てて待ち行列から送る。修正の担当はコミットまでで、送信・返信・決着・まとめは `merge-fix` が行う。書き込みと記録が同じ手順で続くため、担当が途中で止まっても投稿だけが残らない | +| 投稿の記録 | 参照は送信の応答から、件数は送れたインラインの数から取る。本文は取り込みのプロセスの中だけを通り、メインの応答に載らない | | 修正 | **必ずサブエージェント (`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 で終了**。件数は `state.py verify-sweep` が GitHub 側の実数で確認する | @@ -124,7 +124,6 @@ state.json の読み書きや AI launcher 起動・完了待ちは全て委譲 ## 前提 -- `/ndf:pr-review` が **AI 直接投稿**（外部 AI 自身が `gh api` で投稿）に対応 - `/ndf:fix` が **サブエージェント起動 + 重要度ベース自動修正 + Resolve Conversation** に対応 - `gh` CLI が認証済み。担当になる CLI は `init` が起動前に確かめ、通らない者は外して続ける（誤検知するときは `NDF_SKIP_AUTH_CHECK=1`） - `Agent(subagent_type="general-purpose", ...)` でサブエージェントを起動可能 @@ -151,11 +150,12 @@ GitHub は **自分の PR には `REQUEST_CHANGES` でレビューを投稿で ```json "codex": { "intent": "REQUEST_CHANGES", // AI の本来判定。ループ収束判定に使う - "posted_as": "COMMENT", // 422 回避でダウングレードした結果 + "posted_as": "COMMENT", // 投稿する側が送信の時点で落とした形 "comments": 5, "review_url": "..." } ``` +格下げは投稿する側（取り込み）が送信の時点で行う。担当は本来の判定だけを書く。 `state.py judge` は `intent` を見るので、ダウングレード投稿してもループは続行する。 ## 全体フロー @@ -165,13 +165,13 @@ flowchart TD Start([事前確認 / loop 開始前に 1 回だけ]):::phase --> Init["worktree 作成 + state.json 初期化
・自分の PR 判定 → event downgrade 設定
・<worktree-base>/pr<PR> を用意
・既存コメントスナップショット保存"] Init --> Round["Round N start
current_pr = PR#"]:::phase - Round -.並列バックグラウンド.-> Seats["レビュー担当 2 席（start-round が返す）
/ndf:pr-review <PR> <席> を席ごとに起動
body 先頭: cross-review / round N / 席 / intent
→ <席>-review-pr<PR>-result.json"] + Round -.並列バックグラウンド.-> Seats["レビュー担当 2 席（start-round が返す）
launch-reviewer.sh <席> を席ごとに起動
→ 指摘の控え + <席>-review-pr<PR>-result.json
read-result が組み立てて投稿（先頭: round N / 席 / intent）"] Seats --> Decide{"判定 (intent ベース)"} Decide -->|"結果なし (2 度目は final = error)"| Relaunch["結果を残さなかった側だけ
同じラウンドで 1 度起動し直す"] Relaunch --> Decide Decide -->|"両席 APPROVE / --only で外した席"| Approved([final = approved]):::ok - Decide -->|一方でも REQUEST_CHANGES| Fix["Agent (general-purpose)
/ndf:fix <PR> --defer-nit を worktree 内で実行
・critical/major/minor 修正 + push
・reply + resolveReviewThread
・deferred/rejected は reply のみ
→ $TMP_DIR/fix-pr<#>-result.json"] + Decide -->|一方でも REQUEST_CHANGES| Fix["Agent (general-purpose)
/ndf:fix <PR> --defer-nit を worktree 内で実行
・critical/major/minor 修正 + コミット（送らない）
→ $TMP_DIR/fix-pr<#>-result.json
merge-fix が送信・返信・決着・まとめ"] Fix --> Check{収束チェック} Check -->|max-rounds 到達| MaxR([final = max_rounds]):::stop @@ -182,7 +182,7 @@ flowchart TD Check -->|それ以外| Round Rotate --> Round - Approved --> Sweep["最終スイープ (必須)
Agent (general-purpose)
/ndf:fix <PR> を再実行
・残 open review thread を全て確認
・修正可能な minor/nit は修正 + push
・判断保留 nit も reply + resolveReviewThread
→ open thread 0 で終了"] + Approved --> Sweep["最終スイープ (必須)
Agent (general-purpose)
/ndf:fix <PR> を再実行
・残 open review thread を全て確認
・修正可能な minor/nit は修正 + コミット
・判断保留 nit も決着を求める
→ 共通層が送信・返信・決着 → open thread 0"] MaxR --> Sweep Osc --> Sweep Err --> Sweep @@ -229,17 +229,17 @@ while :; do # Step 2: 並列レビュー（担当は start-round が REVIEWERS / REVIEWERS_CSV で返す） # **シェル変数で絞り直さない。** 返る一覧は 1 者指定と席の埋め合わせを反映済みである。 - for r in $REVIEWERS; do - "$SCRIPTS/launch-reviewer.sh" "$r" "$STATE_PR" "$ROUND" - done + # **起動し直しも同じ経路を通す**（#583）。7 のときは名前の出た担当だけを入れて先頭へ戻る。 + AGENTS=$REVIEWERS; AGENTS_CSV=$REVIEWERS_CSV; RELAUNCHED= + while :; do + for r in $AGENTS; do "$SCRIPTS/launch-reviewer.sh" "$r" "$STATE_PR" "$ROUND"; done # 監視: 上限は上限の表（review 1200 秒 / stall は席のランタイム別 codex 180・agy 480・kiro 480・claude 900）。失敗時は kill して返す。担当は `--agents` で渡す。 # Bash の 1 回 600 秒に収まらないため背景で起動し、wait（1 回 540 秒以内）を 124 のあいだ **別の Bash の呼び出しで** 呼び直す。**繰り返しを 1 回の呼び出しへ書かない**（2 回目の待ちで合計が 600 秒を超え、ホストに打ち切られる。docs/01）。 # 監視と取り込みの終了コードは読まない。結果なしは NO_RESULT として state に残り、Step 3 が受け取る。 - "$SCRIPTS/bg-wait.sh" run "$TMP_DIR/review.rc" -- "$SCRIPTS/monitor.py" "$STATE_PR" --phase review --agents "$REVIEWERS_CSV" + "$SCRIPTS/bg-wait.sh" run "$TMP_DIR/review.rc" -- "$SCRIPTS/monitor.py" "$STATE_PR" --phase review --agents "$AGENTS_CSV" "$SCRIPTS/bg-wait.sh" wait "$TMP_DIR/review.rc" # 124 = まだ。**この 1 行を別の Bash の呼び出しとして呼び直す** - for r in $REVIEWERS; do - "$SCRIPTS/state.py" read-result "$STATE_PR" "$r" || true - done + # 取り込みがレビューを投稿する（担当は投稿しない、#730）。出力は件数と参照だけ。 + for r in $AGENTS; do "$SCRIPTS/state.py" read-result "$STATE_PR" "$r" || true; done # Step 2.5: 根拠の検証（#156）。順序と理由は docs/06-evidence.md の「走らせる順序」。 # 飛ばすと、判定が読む区分が統合も実行の結果も反映しないまま決まる。 @@ -252,17 +252,15 @@ while :; do # Step 3: 判定 (0=収束 / 2=修正へ / 7=結果なし / 8=待ち行列に残あり / 1=中断)。引き継いだ指摘が残っていれば、 # 両者が承認しても 2 を返して修正の工程へ回す。置換の終了コードは変数で受けてから読む。 JUDGE_VARS=$("$SCRIPTS/state.py" judge "$STATE_PR"); JUDGE_RC=$?; eval "$JUDGE_VARS" - if [ "$JUDGE_RC" -eq 7 ]; then # 名前の出た担当だけを、同じラウンドで 1 度起動し直す - for a in $RELAUNCH_AGENTS; do "$SCRIPTS/launch-reviewer.sh" "$a" "$STATE_PR" "$ROUND"; done - "$SCRIPTS/bg-wait.sh" run "$TMP_DIR/review.rc" -- "$SCRIPTS/monitor.py" "$STATE_PR" --phase review --agents "$RELAUNCH_AGENTS_CSV" - "$SCRIPTS/bg-wait.sh" wait "$TMP_DIR/review.rc" # 同上。124 のあいだ、別の呼び出しとして呼び直す - for a in $RELAUNCH_AGENTS; do "$SCRIPTS/state.py" read-result "$STATE_PR" "$a" || true; done - JUDGE_VARS=$("$SCRIPTS/state.py" judge "$STATE_PR"); JUDGE_RC=$?; eval "$JUDGE_VARS" - fi - if [ "$JUDGE_RC" -eq 8 ]; then # 上限で積んだ投稿が残っている。流してから判定し直す + if [ "$JUDGE_RC" -eq 8 ]; then # 上限で積んだ投稿が残っている。流してから判定し直す（7 より先に見る） "$SCRIPTS/state.py" flush "$STATE_PR" JUDGE_VARS=$("$SCRIPTS/state.py" judge "$STATE_PR"); JUDGE_RC=$?; eval "$JUDGE_VARS" fi + if [ "$JUDGE_RC" -eq 7 ] && [ -z "$RELAUNCHED" ]; then # 同じラウンドで 1 度だけ起動し直す + AGENTS=$RELAUNCH_AGENTS; AGENTS_CSV=$RELAUNCH_AGENTS_CSV; RELAUNCHED=1; continue + fi + break + done case $JUDGE_RC in 0) break ;; 2) : ;; *) exit "$JUDGE_RC" ;; esac # 1=結果なしのまま中断 # 8 のまま残るのは上限が続いているとき。state は残るので、回復後に同じ引数で再開する。 @@ -312,12 +310,12 @@ while :; do fi done -# Step 7.5: 最終スイープ (必須) — どの終了経路 (approved / max_rounds / oscillation / -# error) でも、ループを抜けた直後に **メインが Agent(general-purpose) を起動** して -# /ndf:fix $STATE_PR を再実行し、$TMP_DIR/sweep-pr$STATE_PR-result.json を書かせる -# (bash 単体では Agent ツールを呼べない。プロンプトは docs/02 の Step 7.5)。 -# 最終 APPROVE ラウンドの minor/nit はループ内 fix を経由しないため、ここで拾わないと -# PR 上に未解決スレッドが残る。sweep 結果はメインが Step 8 の報告へ折り込む。 +# Step 7.5: 最終スイープ (必須) — どの終了経路でも、ループを抜けた直後に **メインが +# Agent(general-purpose) を起動** して /ndf:fix を再実行し、$TMP_DIR/sweep-pr$STATE_PR-result.json +# を書かせる（プロンプトは docs/02 の Step 7.5）。担当はコミットまでで、送信・返信・決着は +# 次の 1 行が行う（#730）。最終 APPROVE ラウンドの minor/nit はここで拾う。 +python3 "$SCRIPTS/../../../scripts/lib/result_posts.py" fix --pr "${CURRENT_PR:-$PR}" \ + --result "$TMP_DIR/sweep-pr$STATE_PR-result.json" --worktree "$WORKTREE" || exit $? # Step 7.5 後段: 最終スイープの結果を GitHub 側の実数で検証する (必須) # exit 0 = 未解決の指摘なし / exit 6 = 残っている (件数と理由を完了報告へ含めて続行) diff --git a/plugins/ndf/skills/cross-review/docs/01-state-and-review.md b/plugins/ndf/skills/cross-review/docs/01-state-and-review.md index 7ef66606..2cd4ef6b 100644 --- a/plugins/ndf/skills/cross-review/docs/01-state-and-review.md +++ b/plugins/ndf/skills/cross-review/docs/01-state-and-review.md @@ -154,11 +154,12 @@ eval "$ROUND_VARS" `--head-branch` で受け取った値を書き戻す。ラウンドの開始時にも取り直すのは、作業ツリーの 外で行われた変更に追従するためである。 -## Step 2: レビュー担当 2 者の並列レビュー（AI 直接投稿） +## Step 2: レビュー担当 2 者の並列レビュー **要点**: メインは launcher を **並列バックグラウンド** で起動するだけ。 -各 AI が `gh api` で投稿し `$TMP_DIR/-review-pr-result.json` に -サマリを書く。**ペイロード本体はメイン context に載せない**。 +各担当は **投稿しない。** 指摘の控え（`<席>-review-pr-round-payload.json`）と +結果ファイル（`<席>-review-pr-result.json`）を一時の名前で書き、控え → 結果の順に +改名する。投稿は取り込み（`read-result`）が行う（#730）。**本文はメイン context に載せない**。 ### 2.1 launcher 起動 + monitor @@ -222,23 +223,21 @@ done `state.rounds[-1].<席の名前>` に `intent / posted_as / comments / review_url / by_severity` を分離保存する（席の名前の形は `04-contracts.md`）。 -#### 申告されたコメント数を GitHub 側と突き合わせる +#### 取り込みがレビューを投稿する -投稿は **AI 自身が `gh api` で行う**ため、失敗しても結果ファイルの申告だけは残る。申告のまま進むと、 -修正担当が読むべき指摘が GitHub 上に存在しないまま収束判定まで走る。実測では、2 件の申告に対し -スレッドが 1 つも作られていなかった。 +**書き込みと記録を 1 つの部分命令に閉じる**（#730）。`read-result` は「控えを読む → 投稿を +積む → 流す → 送信の応答を記録へ書き戻す → 指摘を取り込む」の順に進む。記録の参照は送信の +応答から、件数（`comments`）は送れたインラインの数から取る。**担当の申告を GitHub の実数と +突き合わせる処理は無い**（投稿する側と記録する側が同じになったため）。標準出力に足すのは +件数・参照・状態の行（`POSTED review_url=` / `INLINE= BODY= QUEUED=` / `FINDINGS=`）だけである。 -`read-result` は申告が 1 件以上のとき、`review_url` の識別子から -`repos//pulls//reviews//comments` を数えて突き合わせる。 - -| 申告 | GitHub 側 | 扱い | +| 止まった場所 | 待ち行列の項目 | 立て直し | | --- | --- | --- | -| 0 件 | 見に行かない | 突き合わせる相手がいない | -| n 件 | n 件以上 | 採用する。人の追記など申告以外の経路で増えうる | -| n 件 | n 件未満 | **中断する。** 投稿が届いていない | -| n 件 | 取得できない | 申告を採用し、確認できなかったことを出力へ残す | +| 送る前（上限などで送れていない） | 残る | 判定の終了コード 8 の枝が流し直す | +| 送った後・記録の前 | 残らない | 取り込みをもう一度呼ぶ。照合（ラウンドと席までの前方一致）が先客を見つけ、増えない | -**「取得できなかった」と「0 件」を区別する。** 取得の失敗で止めると、GitHub 側の一時的な不調でループが進まなくなる。 +本文の先頭行は `## 🤖 cross-review | round | <席> | <本来の判定>` である。自分の +Pull Request では送る形だけを `COMMENT` へ落とし、記録の `intent` は本来の判定のまま残す。 **誰がレビューし、いつ止めるかは [05-pool-and-convergence.md](05-pool-and-convergence.md) にある。** 母集合・担当の輪番・認証の確認と、終了基準の 3 つの層をそこで定める。 diff --git a/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md b/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md index 21a5995e..a0805eaa 100644 --- a/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md +++ b/plugins/ndf/skills/cross-review/docs/02-fix-and-rotation.md @@ -20,21 +20,21 @@ **メインセッションでは修正コードを書かない。** `/ndf:fix` を `general-purpose` サブエージェントで起動する。 -**サブエージェントの責務（必須 6 点）**: +**サブエージェントの責務（必須 4 点）**: -1. critical / major / minor の修正コミット +1. critical / major / minor の修正コミット（**送らない**） 2. 修正テストの追加・実行 -3. 修正対象の thread に **reply 投稿** + **`resolveReviewThread` で Resolve** -4. nit / 判断が割れる minor は **修正せず deferred 記録**（reply は `[deferred / nit]` ラベル付き、Resolve しない） -5. **PR レベルの Summary コメントを `gh pr comment` で投稿**（対応件数 / 重要度別 / deferred 件数 / rejected 件数 / commit SHA を含む） -6. 戻り値ファイル `$TMP_DIR/fix-pr-result.json` を必ず書き出す +3. nit / 判断が割れる minor は **修正せず deferred 記録** +4. 戻り値ファイル `$TMP_DIR/fix-pr-result.json` を必ず書き出す （`$TMP_DIR` は env `CROSS_REVIEW_TMP_DIR` > `/.cross_review/` の順で解決。 詳細は `scripts/state.py _tmp_dir()` 参照。`/tmp/` 直書きでも `state.py merge-fix` は legacy fallback で拾う） -> ⚠ inline thread への reply + Resolve **だけでは不十分**。PR ページの -> conversation タブに表示される **PR レベルコメント** がレビュアーへの -> サマリ通知として必須（`/ndf:fix` SKILL.md の手順 7 で規定）。 -> サブエージェント起動プロンプトでも明示的に指示すること。 +**送信・返信・決着・まとめは取り込み（`state.py merge-fix`）が行う**（#730）。サブエージェントは +GitHub と git へ書かない。取り込みは現在の頭を指定して送り（`git push origin HEAD:<ブランチ名>`）、 +戻り値ファイルが報告したコミットが送り先に載ったことを確かめてから、`resolved_threads` / +`deferred` / `rejected` の配列から返信と決着を、件数からまとめを組み立てて待ち行列で送る。 +載っていなければ記録も投稿もせずに止まる。担当が送ると、切り離された頭ではブランチ名だけの +送信が何も送らずに終了コード 0 で終わり、送ったという報告と実物が食い違う。 ### サブエージェント起動例 @@ -101,47 +101,15 @@ worktree 外を触ると競合します。 `total_count: 0` を返す（実測）。保留として読むと、承認されたラウンドが収束しない。 4. critical/major + 該当 minor/nit の修正コミット（worktree 内のみ） 5. `./pint-changed.sh && ./larastan-changed.sh` 等の品質チェック -6. push: `git push origin {HEAD_BRANCH}` （--force / --no-verify 禁止） -7. **CI 再実行は待たない**（push 後の `--watch` 等は行わない、`ci_status` は push 時点での既知失敗のみ反映） -8. **各 thread に reply 投稿**: - - 修正済み: 「対応しました — <ファイル>:<行> で〇〇 (commit )」 - - deferred: 「[deferred / nit] 後続 PR で対応予定」 - - rejected: 「bot 指摘は誤読です — 理由: ...」 -9. **修正済み thread を `resolveReviewThread` で Resolve**: - ```bash - # thread_id は GraphQL で取得 - gh api graphql -f query=' - query {{ repository(owner:"...", name:"...") {{ - pullRequest(number: {PR}) {{ reviewThreads(first:100) {{ - nodes {{ id isResolved path line }} - }} }} - }} }}' - # 修正済みのみ resolve - gh api graphql -f query=' - mutation($id: ID!) {{ - resolveReviewThread(input: {{threadId: $id}}) {{ thread {{ isResolved }} }} - }}' -f id="$THREAD_ID" - ``` - - deferred / rejected の thread は **Resolve しない** -10. **PR レベル Summary コメントを投稿**（必須・inline reply とは別物）: - ```bash - gh pr comment {PR} --body "$(cat <<'EOMD' - ## 🔧 /ndf:fix サマリ (round N) - - 対応件数: critical=X / major=Y / minor=Z (合計 N 件) - deferred: D 件 / rejected: R 件 - commit: - CI: SUCCESS | FAILURE | NONE - - ### 詳細 - - 各 thread の対応概要（行リンク付き） - EOMD - )" - ``` - - inline reply + Resolve だけでは「PR ページの Conversation タブ」に - まとめが出ず、レビュアー視点で見落とされる。**必ず投稿する** -11. 戻り値ファイル書き出し（下記フォーマット）。`summary_comment_url` には - 手順 10 の URL を入れる +6. コミットする。**送らない**（送信は取り込みが行う） +7. **CI 再実行は待たない**（`ci_status` はコミット時点での既知失敗のみ反映） +8. 各 thread の扱いを戻り値ファイルの配列へ入れる。**返信・決着・まとめは投稿しない** + （取り込みが配列から組み立てる）: + - 修正済み: `resolved_threads`（`thread_id` と `comment_id`） + - deferred: `deferred`（`comment_id` と `reason_for_deferral`） + - rejected: `rejected`（`comment_id` と `reason_for_rejection`） +9. 戻り値ファイル書き出し（下記フォーマット）。`summary_comment_url` は書かない + （取り込みがまとめの投稿の応答から記録へ書く） ## 戻り値ファイル $TMP_DIR/fix-pr{PR}-result.json @@ -381,13 +349,15 @@ while ループ脱出後にメインが以下のプロンプトでサブエー > > PR の **全 open review thread**（インライン / レビュー body / PR レベルコメント）を > `gh api` で洗い出し、cross-review の codex/agy が残したものを中心に **すべて解消**せよ: -> 1. 修正可能な `minor`/`nit` → コード修正 + push（同ブランチ、main へは push しない）し、 -> reply + GraphQL `resolveReviewThread` で Resolve。 -> 2. 修正しない（好み・判断保留）`nit` → 「[deferred / nit] 対応見送り: <理由>」を日本語で -> reply した上で **Resolve まで実行**（スレッドを open のまま残さない）。 -> 3. bot 誤指摘 → 却下理由を reply して Resolve。 +> 1. 修正可能な `minor`/`nit` → コード修正 + コミット（**送らない**）し、`resolved_threads` へ入れる。 +> 2. 修正しない（好み・判断保留）`nit` → 見送りの理由を添えて `deferred` へ入れ、 +> **`"resolve": true`** を付ける（スレッドを open のまま残さない）。 +> 3. bot 誤指摘 → 却下理由を添えて `rejected` へ入れ、`"resolve": true` を付ける。 +> +> **GitHub と git へ書かない。** 返信・決着・送信は、メインがこの結果ファイルを読んで +> 共通層の 1 行（`result_posts.py fix`）で行う。 > -> **修正で push した場合は、対象リポジトリの検証を 1 度通すこと。** 何を実行するかは +> **修正をコミットした場合は、対象リポジトリの検証を 1 度通すこと。** 何を実行するかは > 対象リポジトリを見て決める。**コマンドを推測して組み立てない。** > > 1. 実行手段を探す。`Makefile` の `test` / `lint` / `check` ターゲット、`package.json` の @@ -399,7 +369,7 @@ while ループ脱出後にメインが以下のプロンプトでサブエー > 3. 1 つも見つからないときは実行しない > > **終了コードが 0 でない実行を残したまま完了としない。** その修正が原因なら直して -> push し直し、もう一度実行して 0 を確かめる。修正の前から落ちていたなら直さず、 +> コミットし直し、もう一度実行して 0 を確かめる。修正の前から落ちていたなら直さず、 > 何が落ちているかを最終メッセージへ書く（この工程の範囲外である）。どちらの場合も > `commands` には**最後に実行した結果**を残す。 > @@ -408,15 +378,16 @@ while ループ脱出後にメインが以下のプロンプトでサブエー > メッセージへ書く。 > > 完了後、上の**結果ファイル**（`$TMP_DIR/sweep-pr-result.json`）に -> `{"resolved": N, "fixed_in_sweep": M, "commit": "", "remaining_open": K, +> `{"resolved": N, "fixed_in_sweep": M, "commit": "", "fix_commit": "", +> "resolved_threads": [...], "deferred": [...], "rejected": [...], "remaining_open": K, > "remaining_reason": "0 のときの理由|null>", "items": ["<1行要約>", ...], > "verification": {"commands": [{"command": "<実行したコマンド>", "exit": <終了コード>}], > "skipped_reason": "<実行しなかった理由|null>"}}` を > 書き出し、最終メッセージで内訳を日本語報告せよ。 > 検証を実行したときは `commands` に実行順で並べ、`skipped_reason` を `null` にする。 > 実行手段が見つからなかったときは `commands` を空にし、`skipped_reason` に**何を探して -> 見つからなかったか**を書く。push しなかったラウンドも `commands` を空にし、 -> `skipped_reason` に「push なし」と書く。 +> 見つからなかったか**を書く。コミットしなかったときも `commands` を空にし、 +> `skipped_reason` に「コミットなし」と書く。 > **`remaining_open` は 0 とする。** 0 にできない場合は `remaining_reason` に理由を書く。 > この値は申告であり、次の `verify-sweep` が GitHub 側の実数と突き合わせる。 diff --git a/plugins/ndf/skills/cross-review/docs/03-review-output.md b/plugins/ndf/skills/cross-review/docs/03-review-output.md index b070b56b..2927ae40 100644 --- a/plugins/ndf/skills/cross-review/docs/03-review-output.md +++ b/plugins/ndf/skills/cross-review/docs/03-review-output.md @@ -41,10 +41,11 @@ body 先頭に必ず以下を入れる: コード引用ブロック（``` ... ```）や現状説明だけのコメントは作らない。 **インラインは PR の差分に含まれる行にしか付かない。** 差分外の行を指定すると GitHub が -`HTTP 422 Line could not be resolved` を返し、**インラインだけでなくレビュー本体も投稿 -されない**（指摘が丸ごと失われ、PR 上には何も残らない）。差分に無い箇所を指摘するときは -body に「ファイル名:行 + 指摘」の形で書く。422 が返ったら該当インラインを body へ移して -再投稿する。 +`HTTP 422` を返し、**インラインだけでなくレビュー本体も作られない**（要求ごとに全件が拒まれ、 +応答はどのインラインが原因かを指さない）。担当はこれを気にせず、指す行が分かる指摘には +`path` と `line` を書く。**投稿する側（取り込み）が、拒まれた要求のインラインをすべて総評へ +移して送り直す。** 移す契機は応答が行やファイルを解決できないこと（`could not be resolved`）を +示すときだけで、判定の値や基準のコミットの誤りによる 422 は失敗として止める（#730）。 ### 3. body（総評）に書かないこと @@ -83,9 +84,11 @@ pint / larastan / test / build などは **中断** を原則とする。 ## アンチパターン - ❌ **修正をメインセッション内で行う** — context が一気に膨れる。必ずサブエージェント -- ❌ **AI に Markdown だけ返させる** — メインがパース・投稿する設計は禁物。AI 直接投稿 -- ❌ **result.json の申告だけで判定を進める** — 投稿が失敗しても件数は残る。GitHub 側の - 実数と突き合わせないと、修正担当が読むべき指摘が存在しないまま収束する +- ❌ **担当に GitHub へ投稿させる** — 投稿と記録を別の相手が行うと、担当が途中で止まった + ときに投稿だけが残り、記録には無い。起動し直した担当が同じ論点をもう一度投稿する（#583）。 + 担当は指摘の控えと結果ファイルを書くだけにし、取り込みが待ち行列を通して送る（#730） +- ❌ **本文をメインの応答へ載せて投稿する** — 投稿は結果ファイルを読んだプロセス（取り込み）の + 中で組み立てる。メインが読むのは件数と参照だけ - ❌ **nit を都度ユーザに問う** — ループ中は deferred 記録のみ。最終スイープ (Step 7.5) で Resolve - ❌ **未解決スレッドを残したまま終了する** — approved/max_rounds 等いずれの終了経路でも Step 7.5 の最終スイープを必ず実行し、open review thread 0 で終える。特に **最終 APPROVE diff --git a/plugins/ndf/skills/cross-review/docs/04-contracts.md b/plugins/ndf/skills/cross-review/docs/04-contracts.md index fae7ca2b..05e7a3ea 100644 --- a/plugins/ndf/skills/cross-review/docs/04-contracts.md +++ b/plugins/ndf/skills/cross-review/docs/04-contracts.md @@ -203,8 +203,12 @@ あいだは、両者が承認しても収束させない（[01-state-and-review.md](01-state-and-review.md) の Step 3 参照） - `viewer_login` — 自分のログイン名。一度取って持つ控えで、待ち行列の冪等の照合が 「投稿者が自分か」を見るために使う -- `rounds[].codex.queued` — その結果の投稿を待ち行列へ積んだかどうか。真のあいだは - 届いたことの照会を飛ばす（[01-state-and-review.md](01-state-and-review.md) の待ち行列の節参照） +- `rounds[].codex.queued` — 取り込みが送ったレビューが上限で送れず、待ち行列に残っているか。 + 流した直後に参照を書き戻して偽にする（[01-state-and-review.md](01-state-and-review.md) の待ち行列の節参照） +- `rounds[].codex.review_url` — 送信の応答が返した参照。流し直しで先客が見つかったときは先客の参照 +- `rounds[].codex.posted_inline` / `posted_body` — インラインとして送れた件数と、差分の外を + 理由に総評へ移した件数（#730）。`comments` は `posted_inline` と同じ値 +- `rounds[].fix.summary_comment_url` — 修正のまとめの投稿の応答が返した参照（#730） - `rounds[].verdict` の `queued` — 通ったが待ち行列に投稿が残っているラウンド。収束させない - `sweep` — 最終スイープ後の検証結果。`remaining_open` は GitHub 側で数え直した実数で、 `declared_remaining_open` は結果ファイルの申告値。両者が食い違う場合は実数を採る @@ -231,17 +235,19 @@ launcher が生成するプロンプトに以下を強制している: - **headRefOid (commit_id) を明示**: AI が自前で取得すると baseRefOid を誤って入れる事故が多発 - **作業 worktree の絶対パス**: 「ファイル読み取りは必ず worktree 配下の絶対パスを使う」（実 path は state.json の `worktree_path` を参照。`` は `NDF_WORKTREE_BASE` env > `<システム tmpdir>/ndf-worktrees` の優先順で解決） -- **event ダウングレード警告**: `event_downgrade=true` のときは payload の `event` を `COMMENT` に +- **投稿の手順を持たない**（#730）: 担当は投稿しない。判定の格下げ（`event_downgrade`）も + 担当へ渡さず、投稿する側が送信の時点で行う - **既存コメント差分**: `$TMP_DIR/cross-review-pr-existing-comments.txt` を読んで重複指摘禁止 - **自動レビュー観点**: GitHub API の `pulls//files --paginate` で変更ファイルを全件取得して分類し、`common` / `docs_only` / `code` / `db_migration` / `test` / `dependency` / `config_ci` / `api_contract` / `auth_security` / `frontend` / `performance` / `deletion_rename` / `generated` / `i18n` / `infra` の該当テンプレートを state.json の `auto_review_instructions` に保存する - **手動追加レビュー観点**: `--focus` / `--extra-instructions-file` が指定されていれば state.json の `manual_extra_review_instructions` に保存し、自動テンプレートの後ろに連結した `review_instructions` を codex / agy 両 launcher が同じ「追加レビュー観点」セクションとしてプロンプトに差し込む - **進捗マーカー**: agy には `$TMP_DIR/agy-review-pr-progress.log` へ短いフェーズ名を追記させ、monitor の heartbeat で表示する。内部推論や長文説明は書かせない -- **review body 先頭 prefix**: +- **review body 先頭 prefix**（投稿する側が組み立てる）: ``` - ## 🤖 cross-review | round | | + ## 🤖 cross-review | round | <席> | ``` `` は **本来の intent**（`posted_as` ではない）。 例: 自分PR で REQUEST_CHANGES を COMMENT にダウングロードしても、prefix は `REQUEST_CHANGES` のまま。 + 二度書かない照合はこの行の**席まで**の前方一致を鍵にする（判定の語を含めない） - **出力禁止事項**（SKILL.md「レビュー出力の制約」と一致）: - 「良い点」「Strengths」などの褒めセクションを body に書かない - 修正アクションを伴わないインラインコメントは作らない（nit はインライン化しない） @@ -250,24 +256,27 @@ launcher が生成するプロンプトに以下を強制している: ## AI が書き出すファイル契約 -各 launcher は AI に以下 2 ファイルの書き出しを指示する: +各 launcher は AI に以下 2 ファイルの書き出しを指示する。**どちらも一時の名前（末尾 +`.tmp`）で書き終えてから、控え → 結果ファイルの順に改名させる**（#730）。結果ファイルが +正式の名前で現れたことが、2 つとも書き終えた印になる。控えだけが正式の名前で結果ファイルが +無い状態は、結果なしとして扱い投稿を 0 件にする。 | ファイル | 内容 | |---|---| -| `$TMP_DIR/-review-pr-result.json` | `{event, posted_as, comments_count, review_url, by_severity}` のサマリ | -| `$TMP_DIR/-review-pr-round-payload.json` | `{comments: [{path, line, body, severity, evidence, falsification, suggested_check, posted_to}, ...]}` | +| `$TMP_DIR/<席>-review-pr-result.json` | `{event, by_severity}`。担当が書くのはこの 2 つだけ | +| `$TMP_DIR/<席>-review-pr-round-payload.json` | `{summary, comments: [{path, line, body, severity, evidence, falsification, suggested_check}, ...]}` | -**`comments[]` が持つのは、その担当が出した指摘の全件である**（#156）。投稿した -インラインの写しではない。**差分の外を指すために総評へ書いた指摘も、`HTTP 422` で総評へ -移した指摘も載る。** そのため `result.json` の `comments_count`（投稿したインラインの数） -とは一致しない。 +**`comments[]` が持つのは、その担当が出した指摘の全件である**（#156）。位置を持つ指摘は +インラインとして送られ、位置を持たない指摘と、差分の外を理由に拒まれた要求の指摘は総評へ +入る。送れた先（`posted_to`）は投稿する側が控えへ書き戻す。記録の `comments` は送れた +インラインの数で、指摘の件数とは一致しない。 | 項目 | 何を書くか | 無いときの扱い | | --- | --- | --- | | `evidence` | 根拠。対象のコードと到達経路 | 空。`has_evidence` が偽になる | | `falsification` | 反証条件。これが成り立てば棄却できる | 同上 | | `suggested_check` | 実行できる検証手順 | 空 | -| `posted_to` | `inline` / `body` のどちらへ投稿したか | `inline` として扱う | +| `posted_to` | `inline` / `body` のどちらへ送れたか。**投稿する側が書く** | `inline` として扱う | **4 項目を持たない指摘も捨てない。** 捨てると、対応していない担当の指摘が記録から消える。 @@ -291,8 +300,25 @@ launcher が生成するプロンプトに以下を強制している: **落とすのは、書き込む中身が確定した後である。** 読めなかった再実行が、一度取り込めて いた記録を消さないようにする。 -`/ndf:pr-review` の result.json 出力規約に `posted_as` フィールドを含むこと -（自分PR ダウングレード時に GitHub に実際送った event。デフォルトは `event` と同値）。 +## 投稿の種別ごとの契約 + +**GitHub へ書くのはレビューを回す側だけで、すべて待ち行列（`scripts/lib/post_queue.py`）を +通る**（#730）。組み立てと送信は共通層の `scripts/lib/result_posts.py` が持つ。送る前に同じ +ものが先にあるかを照合し、あれば送らずに先客を応答として返す。 + +| 種別 | 積む側 | 組み立ての元 | 二度書かない照合の鍵 | +| --- | --- | --- | --- | +| `review-post` | 指摘の取り込み（`read-result`） | 指摘の控えと結果ファイル | 投稿者と、本文の先頭行の `## 🤖 cross-review \| round \| <席> \|` までの前方一致（判定の語を含めない） | +| `review-reply` | 修正の取り込み（`merge-fix`）/ 単独の `fix` | 修正の結果ファイルの `resolved_threads` / `deferred` / `rejected` | 返信先の指摘の識別子と、本文の先頭 80 文字 | +| `thread-resolve` | 同上 | `resolved_threads`（と、`resolve` が真の見送り・却下） | スレッドの識別子と、すでに決着しているかどうか | +| `pr-comment` | 同上（修正のまとめ）/ 巻き直し（`rotate-pr.sh`） | 修正の結果ファイルの件数とコミット | 投稿者と、本文の先頭 80 文字（まとめはラウンドとコミットを含む） | + +**差分の外を指すインラインで拒まれたら、その要求のインラインをすべて総評へ移して送り直す。** +契機は応答の `errors` が `could not be resolved` を含むときだけで、ほかの 422 は失敗として +止める。すでに決着したスレッドの決着をもう一度送っても失敗にならない（実測）。 + +修正の送信は `git push origin HEAD:<ブランチ名>` で行い、戻り値ファイルの `fix_commit` が +送り先に載ったことを確かめる。載っていなければ取り込みは失敗として止まる。 ## 監視と計測が残すファイル diff --git a/plugins/ndf/skills/cross-review/references/context-budget.md b/plugins/ndf/skills/cross-review/references/context-budget.md index 8dd941d6..7c513dbb 100644 --- a/plugins/ndf/skills/cross-review/references/context-budget.md +++ b/plugins/ndf/skills/cross-review/references/context-budget.md @@ -20,8 +20,10 @@ state.json と result.json だけ読む 2. **サブエージェント分離**: 修正は別 context window で実行 3. **PR ローテーション**: 1 PR あたりの会話履歴を抑える -4. **AI 直接投稿**: 中間ペイロードがメインを通らない +4. **投稿はプロセスの中で組み立てる**: 投稿の本文は担当が書いたファイルから取り込み + （`state.py read-result` / `merge-fix`）のプロセスの中だけを通り、メインの応答には件数・ + 参照・状態だけが載る 5. **state.json で再開可能**: メインが落ちても次回起動時に続きから 手順を変えるときは、この 5 つのどれかを崩していないかを確かめる。特に 1 と 4 は、 -外部 AI の出力をメインが読んで整形する形へ戻すと簡単に崩れる。 +担当の出力をメインが読んで整形する形や、本文を部分命令の引数で渡す形へ戻すと簡単に崩れる。 diff --git a/plugins/ndf/skills/cross-review/scripts/launch-reviewer.sh b/plugins/ndf/skills/cross-review/scripts/launch-reviewer.sh index 036cf92b..12456e1c 100755 --- a/plugins/ndf/skills/cross-review/scripts/launch-reviewer.sh +++ b/plugins/ndf/skills/cross-review/scripts/launch-reviewer.sh @@ -46,7 +46,6 @@ STATE=$TMP_DIR/cross-review-pr$STATE_PR-state.json load_context() { WORKTREE=$(jq -r '.worktree_path' "$STATE") REPO=$(jq -r '.repo' "$STATE") -EVENT_DOWNGRADE=$(jq -r '.event_downgrade // false' "$STATE") EXTRA_REVIEW_INSTRUCTIONS=$(jq -r '.review_instructions // .extra_review_instructions // ""' "$STATE") # PR (=current_pr) は gh コマンドのレビュー対象 PR 番号として使う。 # tmp パス側は STATE_PR で固定 (monitor.py / state.py との読み書き整合のため)。 @@ -59,11 +58,15 @@ SHA=$(jq -r '(.rounds[-1].head_sha // "")' "$STATE") } prepare_prompt_context() { -# 前ラウンドの結果を残さない。投稿失敗などで今ラウンドの result.json が +# 前ラウンドの結果を残さない。担当が止まって今ラウンドの result.json が # 書かれなかったとき、state.py read-result が**前ラウンドの結果を読んで** # 同じ判定を繰り返す事故を防ぐ。 +# 一時の名前のファイルも消す。前の起動が書きかけで止まった残りを、改名の対象に +# しないため。 rm -f "$TMP_DIR/$SEAT-review-pr$STATE_PR-result.json" \ + "$TMP_DIR/$SEAT-review-pr$STATE_PR-result.json.tmp" \ "$TMP_DIR/$SEAT-review-pr$STATE_PR-round$ROUND-payload.json" \ + "$TMP_DIR/$SEAT-review-pr$STATE_PR-round$ROUND-payload.json.tmp" \ "$TMP_DIR/$SEAT-review-pr$STATE_PR-round$ROUND-api-payload.json" STEM=$TMP_DIR/$SEAT-review-pr$STATE_PR @@ -95,18 +98,16 @@ fi render_review_prompt() { cat > "$PROMPT" <\` は本来の intent を書く。 ## 既存コメントスナップショット（重複指摘禁止） workspace 外を読まなくて済むよう、以下にインライン展開する: @@ -116,28 +117,24 @@ $EXISTING_INLINE \`\`\` $EXTRA_REVIEW_BLOCK -## 出力契約 -- review body の **先頭行** に必ず以下を入れる: - \`\`\` - ## 🤖 cross-review | round $ROUND | $SEAT | - \`\`\` - - \`\` は **本来の intent** (REQUEST_CHANGES / APPROVE / COMMENT) - -### 出力に **含めてはいけないもの**（Resolve 負荷を増やすため） -- ❌ **「良い点」/「Strengths」/「評価できる点」 section** — body にも書かない -- ❌ **対応アクションが無いインラインコメント** — 観察・感想・現状説明だけは禁止 -- ❌ **nit / スタイル指摘のインライン化** — 好みの問題はコメント化しない (無視する) -- ❌ **コード引用 (\`\`\` ... \`\`\`) だけで指摘内容が無いコメント** -- ❌ **\`event=COMMENT\` での雑感投稿** — 直すべき点が無ければ \`APPROVE\` にする - -### インラインコメントの書式 +## 指摘に **含めてはいけないもの**（Resolve 負荷を増やすため） +- ❌ **「良い点」/「Strengths」/「評価できる点」** — 総評にも書かない +- ❌ **対応アクションが無い指摘** — 観察・感想・現状説明だけは禁止 +- ❌ **nit / スタイル指摘** — 好みの問題は指摘にしない (無視する) +- ❌ **コード引用 (\`\`\` ... \`\`\`) だけで指摘内容が無い指摘** +- ❌ **判定 \`COMMENT\` での雑感** — 直すべき点が無ければ \`APPROVE\` にする + +### 指摘の書式 - \`[重要度 / カテゴリ]\` プレフィックス必須 (例: \`[major / 正確性]\`) -- 重要度は \`critical\` / \`major\` / \`minor\` のみ使う (nit はインライン化しない) -- 本文は **1 コメント = 1 修正アクション** で完結させる。1〜2 文で具体的な修正提案を書く +- 重要度は \`critical\` / \`major\` / \`minor\` のみ使う (nit は指摘にしない) +- 本文は **1 指摘 = 1 修正アクション** で完結させる。1〜2 文で具体的な修正提案を書く +- 指す行が分かる指摘は \`path\` と \`line\` を埋める。**差分の外の行でもよい** + （差分の外を指す指摘は、投稿する側が総評へ移す） +- 設計レベル・PR 横断の指摘で行を指せないものは、\`path\` / \`line\` を省く -### body (総評) の書き方 +### 総評（\`summary\`）の書き方 - 設計レベル・PR 横断の **修正提案のみ** 書く -- 書くことが無ければ prefix 行 + 1 行サマリだけで良い (褒め言葉や評価文は不要) +- 書くことが無ければ 1 行サマリだけで良い (褒め言葉や評価文は不要) ### 進捗マーカー（監視用） - 無言ハングと区別できるよう、作業フェーズが進むたびに @@ -146,69 +143,53 @@ $EXTRA_REVIEW_BLOCK - \`start: review PR #$PR round $ROUND\` - \`scan: diff and existing comments\` - \`analyze: candidate findings\` - - \`post: submit review\` + - \`write: payload and result\` - \`done: result.json written\` - -### インラインコメントを付けられる行（422 対策・必須） -- インラインコメントは **この PR の差分に含まれる行にしか付けられない**。差分外の行を - 指定すると GitHub が \`HTTP 422 Line could not be resolved\` を返し、**インラインだけで - なくレビュー本体も投稿されない**（指摘が丸ごと失われる） -- 差分に無い箇所を指摘したいときは、インラインにせず **body に「ファイル名:行 + 指摘」 - の形で書く** -- それでも 422 が返ったときは、**該当インラインを body へ移して再投稿する**。 - 投稿を諦めない - -### 投稿できなかった場合（必須） -- gh api がエラーを返したら、err.log に詳細を残したうえで **result.json を必ず書いて - から終了する**。\`event\` は本来の intent、\`comments_count\` は 0、 - \`"post_error"\` に失敗理由（HTTP status とメッセージ）を入れる: - \`\`\`json - {"event": "REQUEST_CHANGES", "posted_as": "COMMENT", "comments_count": 0, - "review_url": "", "by_severity": {"critical": 0, "major": 0, "minor": 0, "nit": 0}, - "post_error": "422 Line could not be resolved"} - \`\`\` -- result.json を書かずに終了すると、収束ループは**前ラウンドの結果を使うか、結果なしで - 停止する**。エラー時ほど result.json が要る - -- 投稿後、サマリを **$STEM-result.json** に - **必ず以下のキーで** 書く: - \`\`\`json - { - "event": "APPROVE", - "posted_as": "COMMENT", - "comments_count": 3, - "review_url": "https://github.com/.../pull/$PR#pullrequestreview-...", - "by_severity": {"critical": 0, "major": 0, "minor": 0, "nit": 0} - } - \`\`\` - - \`intent\` / \`comment_count\` 等の別名は使わないこと - - \`event\` の値は \`APPROVE\` / \`REQUEST_CHANGES\` / \`COMMENT\` のいずれか - - \`event_downgrade=true\` のとき \`posted_as\` は \`COMMENT\` にダウングレード可 -- payload は **$STEM-round$ROUND-payload.json** に保存 - （\`{ "comments": [{path, line, body, severity, evidence, falsification, - suggested_check, posted_to}, ...] }\` 形式） - - **\`comments[]\` に載せるのは、あなたが出した指摘の全件である。** 投稿したインラインの - 写しではない。**差分の外を指すために body へ書いた指摘も、422 で body へ移した指摘も - 載せる**（載せないと進行側から見えない） - - \`posted_to\` は \`inline\` / \`body\` のどちらへ投稿したか - - \`path\` / \`line\` は body へ書いたときも埋める（body でも「ファイル名:行 + 指摘」の - 形で書くため、値は手元にある） - - \`evidence\` は根拠（対象のコードと到達経路）、\`falsification\` は反証条件 - （これが成り立てば棄却できる）、\`suggested_check\` は実行できる検証手順 - - **根拠と反証条件は、別の担当がその指摘を確かめるためのものである。** 確かめられない - 書き方（「一般によくない」など）は根拠にならない +## 書くファイル（2 つ） + +**どちらも一時の名前で書き終えてから、正式の名前へ改名する。改名の順序は控えが先、 +結果ファイルが後である。** 結果ファイルが正式の名前で現れたことが、2 つとも書き終えた +印になる。途中で止まったときは正式の名前のファイルを残さない。 + +1. 指摘の控えを **$STEM-round$ROUND-payload.json.tmp** に書く: + \`\`\`json + { + "summary": "総評（1〜数行）", + "comments": [ + {"path": "src/foo.py", "line": 42, "body": "[major / 正確性] ...", + "severity": "major", "evidence": "...", "falsification": "...", + "suggested_check": "..."} + ] + } + \`\`\` + - **\`comments[]\` に載せるのは、あなたが出した指摘の全件である** + - \`evidence\` は根拠（対象のコードと到達経路）、\`falsification\` は反証条件 + （これが成り立てば棄却できる）、\`suggested_check\` は実行できる検証手順 + - **根拠と反証条件は、別の担当がその指摘を確かめるためのものである。** 確かめられない + 書き方（「一般によくない」など）は根拠にならない +2. 判定を **$STEM-result.json.tmp** に **必ず以下のキーだけで** 書く: + \`\`\`json + { + "event": "REQUEST_CHANGES", + "by_severity": {"critical": 0, "major": 1, "minor": 0, "nit": 0} + } + \`\`\` + - \`event\` は本来の判定で、\`APPROVE\` / \`REQUEST_CHANGES\` / \`COMMENT\` のいずれか。 + \`intent\` などの別名は使わない +3. 改名する（**この順で**）: + \`\`\`bash + mv "$STEM-round$ROUND-payload.json.tmp" "$STEM-round$ROUND-payload.json" + mv "$STEM-result.json.tmp" "$STEM-result.json" + \`\`\` ## 守るべきこと - **発見を終えるまで、参照してよい既存コメントは起動時に渡されたスナップショットに - 限る。** 同じラウンドの他の担当が投稿した指摘・結果ファイル・進捗ログは参照しない - （指摘を出し終えて投稿するまでの間の話で、投稿の手順が既存コメントを引くことは妨げない） - - **担当は並列に起動する。** 先に投稿した担当の指摘を読むと、独立に見つけた指摘と - 区別できなくなる。同じ指摘が 2 者から出たことに意味があるのは、互いを見ていない場合 - だけである -- **リポジトリ編集禁止**。gh api での投稿のみ許可 -- worktree 外のパスは触らない -- gh api 失敗時は err.log にエラー詳細を残し、**result.json を書いてから**終了する + 限る。** 同じラウンドの他の担当の結果ファイル・進捗ログは参照しない + - **担当は並列に起動する。** 他の担当の指摘を読むと、独立に見つけた指摘と区別できなく + なる。同じ指摘が 2 者から出たことに意味があるのは、互いを見ていない場合だけである +- **リポジトリ編集禁止。PR・GitHub・git への書き込みもしない** +- worktree 外のパスは、上の 2 つのファイルと進捗マーカー以外に触らない EOF } diff --git a/plugins/ndf/skills/cross-review/scripts/state.py b/plugins/ndf/skills/cross-review/scripts/state.py index 579a2ddc..9667dd61 100755 --- a/plugins/ndf/skills/cross-review/scripts/state.py +++ b/plugins/ndf/skills/cross-review/scripts/state.py @@ -38,6 +38,7 @@ import statefile # noqa: E402 再開の反映（#727 / #648） import run_metrics # noqa: E402 実行の要約（#662） import monitor_outcome # noqa: E402 起動 1 回の結末（#729） +import result_posts # noqa: E402 結果ファイルを投稿へ変える層（#730） # 区分の定義は scripts 配下の共有モジュールに 1 か所だけ置く（#156、#732）。 # `measure.py` も同じ定義を読み、両者の一致は `test_measure.py` が固定する。 @@ -1489,8 +1490,9 @@ def _auto_flush(pr: int) -> None: 実行していない場合に流れない。明示だけだと、進行側が忘れたときに待ち行列が残った まま収束の判定へ進む。流せなくても工程は止めない。 - **入口は、書き戻し先が揃っている場所だけである。** 取り込み（`read-result`）の - 入口では、そのラウンドの担当のエントリがまだ無い。詳細は `cmd_read_result` にある。 + **入口は、書き戻し先が揃っている場所だけである。** 積むのは取り込み + （`read-result`）だけで、積んだ時点でその担当の記録を書くため、流す時点では + 書き戻し先が揃っている。 """ q = _queue(pr) if not q.count(): @@ -2260,49 +2262,36 @@ def _round_passes( return True -def _guard_previous_round(st: dict[str, Any], prev: dict[str, Any]) -> None: - """前のラウンドの後始末が終わっているかを確かめる。 - - 進行側が手で修正して次のラウンドへ進めると、修正の工程（Step 5）が担う返信と - Resolve が飛ばされる。飛ばされたまま進むと、未解決の指摘が残ったまま承認へ到達する。 - - 止めるのは次の 2 つ。 - - 1. 前のラウンドが修正必須の判定なのに、修正の記録が無い - 2. 前のラウンドで Resolve したと申告されたスレッドが、GitHub 側で未解決のまま +def _resolve_previous_verdict(st: dict[str, Any], prev: dict[str, Any]) -> str | None: + """保存されていない旧形式の判定を、ラウンドの結果から復元する。""" + verdict = prev.get("verdict") + if verdict is not None: + return verdict + reviewers = prev.get("reviewers") or _round_reviewers(st, prev.get("round") or 1) + if _no_result_agents(prev, st.get("only"), reviewers): + return "no_result" + return ("approved" if _round_passes(prev, st.get("only"), reviewers) + else "changes_requested") + + +def _require_fix_for_changes(round_no: Any, verdict: str | None, + fix: dict[str, Any] | None) -> None: + """修正必須の判定に修正記録が伴うことを確かめる。""" + if verdict != "changes_requested" or fix: + return + die( + f"round {round_no} は修正必須の判定でしたが、修正の記録がありません。" + " 返信と Resolve が飛ばされている可能性があります。" + " `/ndf:fix` を実行して戻り値ファイルを作り、`merge-fix` を通してから" + " 次のラウンドを開始してください", + code=5, + ) - 未解決の指摘を取得できないときは検査を行わず、確認できなかったことを残して進む。 - 取得の失敗で止めると、GitHub 側の一時的な不調でループが進まなくなる。 - スレッドの状態は、申告が行われた Pull Request（`prev["pr"]`）へ問い合わせる。 - ローテーションを挟んだラウンドでは Step 6 の `set-current-pr` が先に走るため、 - `current_pr` は既に新しい Pull Request を指している。そちらへ問い合わせると、 - 旧 Pull Request のスレッドが未解決のままでも一覧に現れず検査が素通りする。 - """ +def _verify_resolved_threads(st: dict[str, Any], prev: dict[str, Any], + fix: dict[str, Any] | None) -> None: + """Resolve 済みとの申告を GitHub の未解決スレッドと突き合わせる。""" round_no = prev.get("round") - verdict = prev.get("verdict") - if verdict is None: - # 判定の結果を持たない古い状態ファイルは、保存された重要度から判定し直す。 - # 項目が欠けたラウンドは結果なしであり、修正の記録を求める対象ではない。 - # **数える相手はそのラウンドの担当である**（決定 11）。`codex` / `agy` で数えると、 - # 担当が `agy` + `kiro` のラウンドで `codex` を結果なしと読み、修正の記録が - # 無いまま次のラウンドへ通す。 - reviewers = prev.get("reviewers") or _round_reviewers(st, prev.get("round") or 1) - if _no_result_agents(prev, st.get("only"), reviewers): - verdict = "no_result" - else: - verdict = ("approved" if _round_passes(prev, st.get("only"), reviewers) - else "changes_requested") - fix = prev.get("fix") - if verdict == "changes_requested" and not fix: - die( - f"round {round_no} は修正必須の判定でしたが、修正の記録がありません。" - " 返信と Resolve が飛ばされている可能性があります。" - " `/ndf:fix` を実行して戻り値ファイルを作り、`merge-fix` を通してから" - " 次のラウンドを開始してください", - code=5, - ) - claimed = (fix or {}).get("resolved_thread_ids") or [] if not claimed: return @@ -2324,6 +2313,31 @@ def _guard_previous_round(st: dict[str, Any], prev: dict[str, Any]) -> None: ) +def _guard_previous_round(st: dict[str, Any], prev: dict[str, Any]) -> None: + """前のラウンドの後始末が終わっているかを確かめる。 + + 進行側が手で修正して次のラウンドへ進めると、修正の工程（Step 5）が担う返信と + Resolve が飛ばされる。飛ばされたまま進むと、未解決の指摘が残ったまま承認へ到達する。 + + 止めるのは次の 2 つ。 + + 1. 前のラウンドが修正必須の判定なのに、修正の記録が無い + 2. 前のラウンドで Resolve したと申告されたスレッドが、GitHub 側で未解決のまま + + 未解決の指摘を取得できないときは検査を行わず、確認できなかったことを残して進む。 + 取得の失敗で止めると、GitHub 側の一時的な不調でループが進まなくなる。 + + スレッドの状態は、申告が行われた Pull Request（`prev["pr"]`）へ問い合わせる。 + ローテーションを挟んだラウンドでは Step 6 の `set-current-pr` が先に走るため、 + `current_pr` は既に新しい Pull Request を指している。そちらへ問い合わせると、 + 旧 Pull Request のスレッドが未解決のままでも一覧に現れず検査が素通りする。 + """ + fix = prev.get("fix") + verdict = _resolve_previous_verdict(st, prev) + _require_fix_for_changes(prev.get("round"), verdict, fix) + _verify_resolved_threads(st, prev, fix) + + def _sync_before_round(st: dict[str, Any], pr: int) -> HeadRef | None: """ラウンドを開く前に、レビュー用の作業ツリーを Pull Request の head へ揃える。 @@ -2413,32 +2427,6 @@ def _as_count(value: object) -> int: 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 - - # Pull Request 上の未解決の指摘（Resolve されていない review thread）を数えるための問い合わせ。 # `--paginate` に載せるため、カーソルと `pageInfo` を持たせる。 _UNRESOLVED_THREADS_QUERY = """ @@ -2462,14 +2450,13 @@ def _posted_comment_count(repo: str, pr: int, review_url: str | None) -> int | N def _review_exists(repo: str, pr: int, review_url: str | None) -> bool | None: - """申告された `review_url` の指すレビューが GitHub 側にあるか。 + """`review_url` の指すレビューが GitHub 側にあるか。 取得できなければ `None` を返す。**「取得できなかった」と「無い」を区別する。** 取得の失敗で中断すると、GitHub 側の一時的な不調でループが止まる。 - 投稿は AI 自身が `gh api` で行うため、失敗しても結果ファイルには判定が残る。 - 判定だけを採ると、修正の担当が読むべき指摘が Pull Request に無いまま修正の工程が - 起動する（実測: `review_url` が空、重要度別の件数もすべて 0）。 + 上限で積んだ投稿を後から流したとき、その直後に 1 度だけ呼ぶ（`_confirm_flushed`）。 + 取り込みが自分で送った投稿は、送信の応答をそのまま記録にするため照会しない（#730）。 """ if not repo or not review_url: return False @@ -2735,54 +2722,6 @@ def _read_review_result_file(pr: int, agent: str, rfile: pathlib.Path) -> dict[s die(f"{agent}: 使える結果が無い (reason={reason}, {rfile}): {outcome.detail}") -def _verify_review_arrival( - pr: int, agent: str, repo: str, result: dict[str, Any] -) -> bool: - """投稿が Pull Request に届いたかを確かめ、待ち行列へ積んだかどうかを返す。 - - **投稿が届いたかを先に確かめる。** 判定だけが残り、指摘の中身が Pull Request に - 無いまま修正の工程へ進む経路を塞ぐ（#261）。届いていないときは結果なしとして - 記録し、判定の側の「同じラウンドで 1 度だけ起動し直す」経路へ乗せる。修正の担当 - から見ると、結果が残らなかった場合と、結果はあるが指摘が届いていない場合は同じ - 状態である（読むべき指摘が無い）。 - - **待ち行列へ積んだ投稿は、積んだ時点では届いていない。** ここで照会すると - 結果なしになり、起動し直しで同じ内容が二重に積まれる。届いたことは流した直後に - 1 度だけ確かめる（`_confirm_flushed`）。 - """ - queued = bool(result.get("queued")) - if queued: - info( - f"⚠ {agent}: 投稿を待ち行列へ積んでいます。" - "届いたことの確認は流した直後に行います" - ) - post_error = None if queued else result.get("post_error") - if post_error: - _die_no_result( - pr, - agent, - "not_posted", - f"{agent}: レビューの投稿に失敗しています (post_error={post_error})。" - " 指摘が Pull Request に届いていないため、結果なしとして扱います", - ) - exists = None if queued else _review_exists(repo, pr, result.get("review_url")) - if exists is False: - _die_no_result( - pr, - agent, - "not_posted", - f"{agent}: 投稿されたレビューを確認できません " - f"(review_url={result.get('review_url')!r})。" - " 指摘が Pull Request に届いていないため、結果なしとして扱います", - ) - if exists is None and not queued: - info( - f"⚠ {agent}: レビューの投稿を確認できませんでした。" - "申告をそのまま採用します" - ) - return queued - - # 指摘へ既定を与える項目。**持たない指摘も捨てない**（#156）。捨てると、4 項目へ # 対応していない担当の指摘が記録から消える。 _FINDING_DEFAULTS: dict[str, Any] = { @@ -2887,70 +2826,34 @@ def _collect_review_findings( return len(items) -def _resolve_result_aliases(r: dict[str, Any]) -> tuple[str | None, str | None, Any]: - """result.json の別名フィールドを正規のキーへ解決する。 - - `intent` / `comment_count` を使う変則 JSON を書き出す既知のケースに対応する。 - 仕様としては `event` / `comments_count` が正で、そちらを優先する。 - """ - intent = r.get("event") or r.get("intent") - posted_as = r.get("posted_as") or intent - comments = r.get("comments_count") - if comments is None: - comments = r.get("comment_count") - return intent, posted_as, comments - - -def _verify_declared_comments( - repo: str, - pr: int, - agent: str, - comments: Any, - review_url: str | None, - queued: bool, -) -> None: - """**申告を GitHub 側と突き合わせる。** - - 投稿は AI 自身が行うので、失敗しても結果ファイルには件数が残る。申告のまま進むと、 - 修正担当が読むべき指摘が GitHub 上に存在しないまま収束判定まで走る - （実測: 申告 2 件に対しスレッド 0）。 - """ - declared = _as_count(comments) - if declared > 0 and not queued: - actual = _posted_comment_count(repo, pr, review_url) - if actual is None: - info( - f"⚠ {agent}: 投稿されたコメント数を確認できませんでした。" - f"申告（{declared} 件）をそのまま採用します" - ) - elif actual < declared: - die( - f"{agent}: インラインコメントの申告 {declared} 件に対し、" - f"GitHub 上には {actual} 件しかありません。投稿が届いていないため" - "中断します。レビューを投稿し直してから再実行してください" - ) - - def cmd_read_result(args: argparse.Namespace) -> None: - """Step 2.4 — codex/agy の result.json を state にマージ。 + """Step 2.4 — 担当の結果を読み、レビューを投稿して state にマージ。 使える結果が残らなかったときは、`NO_RESULT` と理由をラウンドへ残してから止める。 終了コードは現行のまま（無い・判定の値を持たないときは 1、JSON として読めない ときは 3）で、進む先を決めるのは次の判定である。 - **ここでは待ち行列を流さない。** 流すと `review-post` の書き戻し先（そのラウンドの - 担当のエントリ）がまだ無い時点で項目が消える。`_confirm_flushed` は書き戻せず、 - この後の取り込みが `queued: true` だけを保存するため、待ち行列が空で `queued` の - ままの状態ができる。判定はその状態で収束してしまい、投稿の存在も参照も確かめない。 - **両方の担当を取り込んだ後に流す**（`judge` の入口）。取り込みは判定の直前に - しかないため、流す時期が遅れるのは 1 コマンド分である。 + **「読んで記録する」と「投稿する」を 1 つに閉じる**（#730 の決定 4）。順序は + 「控えを読む → 投稿を積む → 流す → 送信の応答を記録へ書き戻す → 指摘を取り込む」 + である。分けると「投稿したが記録していない」に加えて「記録したが投稿していない」が + もう 1 つ増える。1 つに閉じれば、途中で止まった状態は「送れていない」か + 「送れたが記録が無い」の 2 つになる。 + + | 止まった場所 | 待ち行列の項目 | 立て直し | + | --- | --- | --- | + | 送る前（上限などで送れていない） | 残る | 判定の終了コード 8 の枝が流し直す | + | 送った後・記録の前 | 残らない | 取り込みをもう一度呼ぶ。照合が先客を見つける | + + **担当の申告と GitHub の実数を突き合わせない。** 投稿する側と記録する側が同じに + なるため、確かめる対象が無い。記録に入る URL は送信の応答から取り、件数は送れた + インラインの数から取る。 """ agent = args.agent pr = args.pr rfile = pathlib.Path(args.file or _resolve_tmp_dir(pr) / f"{agent}-review-pr{pr}-result.json") r = _read_review_result_file(pr, agent, rfile) - intent, posted_as, comments = _resolve_result_aliases(r) + intent = r.get("event") or r.get("intent") if intent is None: _die_no_result( @@ -2965,27 +2868,50 @@ def cmd_read_result(args: argparse.Namespace) -> None: if not st.get("rounds"): die(f"{agent}: state.rounds が空。`state.py start-round` を先に呼んでください") - repo = str(st.get("repo") or "") - - queued = _verify_review_arrival(pr, agent, repo, r) - - _verify_declared_comments(repo, pr, agent, comments, r.get("review_url"), queued) - - st["rounds"][-1][agent] = { - "intent": intent, - "posted_as": posted_as, - "comments": comments, - "review_url": r.get("review_url"), + # **先に残りを流す。** 残っているのは、前の取り込みで送れずに積んだ投稿だけで、 + # その担当の記録は積んだ時点で書いてある。流した結果をその記録へ書き戻してから、 + # この担当の投稿を後ろへ積む（Pull Request 上の順序を保つ）。 + _auto_flush(pr) + st = _load(pr) + last = st["rounds"][-1] + round_no = last.get("round") + posted = result_posts.post_review( + _queue(pr), + _payload_path(agent, pr, round_no), + rfile, + repo=str(st.get("repo") or ""), + pr=int(st.get("current_pr") or pr), + round_no=int(round_no or 1), + seat=agent, + head_sha=str(last.get("head_sha") or ""), + is_own_pr=bool(st.get("event_downgrade") or st.get("is_own_pr")), + actor=str(st.get("viewer_login") or "") or None, + since=str(last.get("started_at") or "") or None, + ) + if posted.failed: + die(f"{agent}: レビューを投稿できませんでした ({posted.detail})") + + last[agent] = { + "intent": posted.intent, + "posted_as": posted.posted_as, + "comments": posted.posted_inline, + "review_url": posted.review_url, "by_severity": r.get("by_severity", {}), - "queued": queued, + "queued": bool(posted.queued), + "posted_inline": posted.posted_inline, + "posted_body": posted.posted_body, } - # **指摘そのものは別に積む**（#156）。`comments` は投稿したインラインの数で、 - # GitHub 側の実数との突き合わせに使う。総評だけへ書いた指摘はそこに現れない。 - collected = _collect_review_findings(st, agent, pr, st["rounds"][-1]["round"]) + # **指摘そのものは別に積む**（#156）。`comments` は送れたインラインの数で、 + # 総評へ移した指摘はそこに現れない。 + collected = _collect_review_findings(st, agent, pr, round_no) _save(pr, st) - info(f"✅ {agent}: intent={intent} posted_as={posted_as} comments={comments}") - if collected: - info(f" 指摘の記録: {collected} 件") + if posted.review_url: + print(f"POSTED review_url={posted.review_url}") + print(f"INLINE={posted.posted_inline} BODY={posted.posted_body}" + f" QUEUED={posted.queued}") + print(f"FINDINGS={collected}") + info(f"✅ {agent}: intent={posted.intent} posted_as={posted.posted_as}" + f" comments={posted.posted_inline}") def _round_ci(st: dict[str, Any], last: dict[str, Any], pr: int) -> dict[str, Any]: @@ -3677,11 +3603,7 @@ def cmd_collect_critiques(args: argparse.Namespace) -> None: if not st.get("rounds"): die("state.rounds が空。`state.py start-round` を先に呼んでください") round_no = st["rounds"][-1]["round"] - findings = { - f.get("finding_id"): f - for f in st.get("review_findings") or [] - if f.get("round") == round_no - } + findings = _round_finding_index(st, round_no) reviewers = _round_reviewers(st, round_no) attached, covered = _attach_critiques(st, pr, round_no, findings, reviewers) @@ -3695,11 +3617,7 @@ def cmd_collect_critiques(args: argparse.Namespace) -> None: # **揃っていない対象は統合の後に数える。** 束ねられた側は対象から外れるため、 # 先に数えると、代表へ返された 1 件で足りる組を不足として扱う。 - missing: dict[str, list[str]] = {} - for agent in reviewers: - unmet = sorted(_critique_targets(st, round_no, agent) - covered[agent]) - if unmet: - missing[agent] = unmet + missing = _missing_critique_targets(st, round_no, reviewers, covered) if missing: _handle_incomplete_critiques(pr, st, round_no, missing) return @@ -3710,6 +3628,32 @@ def cmd_collect_critiques(args: argparse.Namespace) -> None: _save(pr, st) +def _round_finding_index( + st: dict[str, Any], round_no: int +) -> dict[Any, dict[str, Any]]: + """そのラウンドの指摘を `finding_id` で引ける索引にする。""" + return { + f.get("finding_id"): f + for f in st.get("review_findings") or [] + if f.get("round") == round_no + } + + +def _missing_critique_targets( + st: dict[str, Any], + round_no: int, + reviewers: list[str], + covered: dict[str, set[str]], +) -> dict[str, list[str]]: + """反証が揃っていない対象を、担当ごとに `finding_id` の並びで返す。""" + missing: dict[str, list[str]] = {} + for agent in reviewers: + unmet = sorted(_critique_targets(st, round_no, agent) - covered[agent]) + if unmet: + missing[agent] = unmet + return missing + + def _handle_incomplete_critiques( pr: int, st: dict[str, Any], round_no: int, missing: dict[str, list[str]] ) -> None: @@ -4232,6 +4176,12 @@ def _count(v: Any) -> int: return 0 +# 読んだ修正の結果ファイルの場所。投稿の組み立ては本文を引数に取らず、ファイルの +# パスを受け取る（#730 の決定 3）。記録へは写らない（`_normalize_fix_result` は +# 決まった鍵だけを読む）。 +FIX_SOURCE_KEY = "_source_path" + + def _read_fix_result( pr: int | str, explicit_file: str | pathlib.Path | None, @@ -4256,7 +4206,9 @@ def _read_fix_result( ] if explicit is not None: - return _read_explicit_fix_result(explicit) + fix = _read_explicit_fix_result(explicit) + fix.setdefault(FIX_SOURCE_KEY, str(explicit)) + return fix fix = _find_fallback_fix_result(fallback_candidates, pr, round_started_ts) @@ -4308,6 +4260,7 @@ def _find_fallback_fix_result( candidate, pr, round_started_ts, is_canonical=is_canonical ) if is_fresh: + parsed.setdefault(FIX_SOURCE_KEY, str(candidate)) return parsed return None @@ -4453,9 +4406,31 @@ def cmd_merge_fix(args: argparse.Namespace) -> None: fix = _read_fix_result(pr, args.file, round_started_ts) + # **送信と投稿は取り込む側が行う**（#730）。修正の担当はコミットまでで止まる。 + # 送れない・報告されたコミットが送り先に載っていないときは、記録も投稿もせずに + # 止まる。同じ取り込みをやり直せば、同じ手順を最初から通る。 + commit = fix.get("fix_commit") or fix.get("commit_sha") + pushed = result_posts.push_fix(str(st.get("worktree_path") or ""), + str(st.get("head_branch") or ""), commit) + if not pushed.ok: + die(f"修正を送れないか、報告されたコミットが送り先に載っていません: {pushed.detail}") + print(f"PUSHED={1 if pushed.pushed else 0} COMMIT_ON_HEAD={1 if pushed.contains else 0}") + round_fix = _merge_fix_records(st, fix, pr) _save(pr, st) + posted = result_posts.post_fix( + _queue(pr), fix[FIX_SOURCE_KEY], str(st.get("repo") or ""), + int(st.get("current_pr") or pr), round_no=st["rounds"][-1].get("round"), + actor=str(st.get("viewer_login") or "") or None) + st["rounds"][-1]["fix"]["summary_comment_url"] = posted.summary_url + _save(pr, st) + if posted.summary_url: + print(f"POSTED summary_url={posted.summary_url}") + print(f"REPLIED={posted.replied} RESOLVED={posted.resolved} QUEUED={posted.queued}") + if posted.failed: + die(f"返信・決着・まとめを投稿できませんでした ({posted.detail})") + # CI 分類 if (fix.get("ci_status") or "").upper() != "FAILURE": info(f"✅ fix マージ完了 (commit={round_fix['commit']} fixed={round_fix['fixed']})") diff --git a/plugins/ndf/skills/cross-review/tests/conftest.py b/plugins/ndf/skills/cross-review/tests/conftest.py index 67cd6aec..94bb32c3 100644 --- a/plugins/ndf/skills/cross-review/tests/conftest.py +++ b/plugins/ndf/skills/cross-review/tests/conftest.py @@ -131,6 +131,32 @@ def _no_github_state(request, monkeypatch) -> None: state_mod = request.getfixturevalue("state_mod") monkeypatch.setattr(state_mod, "_fetch_check_runs", lambda repo, sha: None) monkeypatch.setattr(state_mod, "_fetch_pr_metadata", lambda pr, repo=None: None) + # **取り込みはレビューを投稿する**（#730）。投稿を見ないテストでは、組み立てまでを + # 本物で通し、送信だけを「届いた」に置き換える。偽の `gh` を要求するテストは + # 送信も含めて検査するため置き換えない。 + if "fake_gh" not in request.fixturenames: + rp = state_mod.result_posts + monkeypatch.setattr(rp, "post_review", _post_review_offline(rp)) + monkeypatch.setattr(rp, "push_fix", + lambda worktree, head, commit: rp.PushResult( + True, bool(commit), True, "")) + monkeypatch.setattr(rp, "post_fix", _post_fix_offline(rp)) + + +def _post_review_offline(rp): + """送信を行わず、組み立てた内容がそのまま届いたものとして結果を返す。""" + def _post(queue, payload_path, result_path, repo, pr, round_no, seat, head_sha, + is_own_pr, actor=None, since=None): + item = rp.review_posts(payload_path, result_path, repo, pr, round_no, seat, + head_sha, is_own_pr, since=since)[0] + extra = item["extra"] + findings = len(rp._findings(rp._read_json(payload_path))) + return rp.ReviewOutcome( + review_url=f"https://github.com/{repo}/pull/{pr}#pullrequestreview-1", + posted_inline=extra["inline"], posted_body=extra["body"], queued=0, + findings=findings, failed=False, posted_as=extra["posted_as"], + intent=extra["intent"], detail="") + return _post @pytest.fixture() @@ -267,3 +293,14 @@ def fake_gh(monkeypatch, tmp_path) -> FakeGh: def queue_mod() -> types.ModuleType: """共通層の待ち行列モジュール（#291）。""" return _load_module("ndf_post_queue", _POST_QUEUE) + + +def _post_fix_offline(rp): + """送信を行わず、組み立てた返信・決着・まとめがすべて届いたものとして返す。""" + def _post(queue, result_path, repo, pr, round_no=None, actor=None): + kinds = [i["kind"] for i in rp.fix_posts(result_path, repo, pr, round_no)] + return rp.FixOutcome( + summary_url=f"https://github.com/{repo}/pull/{pr}#issuecomment-1", + replied=kinds.count("review-reply"), resolved=kinds.count("thread-resolve"), + queued=0, failed=False, detail="") + return _post diff --git a/plugins/ndf/skills/cross-review/tests/test_launch_reviewer_prompt_context.py b/plugins/ndf/skills/cross-review/tests/test_launch_reviewer_prompt_context.py index b48a024a..f5e7ded0 100644 --- a/plugins/ndf/skills/cross-review/tests/test_launch_reviewer_prompt_context.py +++ b/plugins/ndf/skills/cross-review/tests/test_launch_reviewer_prompt_context.py @@ -44,32 +44,70 @@ def test_existing_comments_are_inlined_or_replaced_with_none(tmp_path, comments, assert snapshot == expected + "\n" -@pytest.mark.parametrize("event_downgrade, expected_line", [ - (True, "- event_downgrade: true"), - (False, "- event_downgrade: false"), -], ids=["downgrade-true", "downgrade-false"]) -def test_event_downgrade_is_reflected_in_prompt(tmp_path, event_downgrade, expected_line): - """現状固定: state.json の event_downgrade がプロンプトに反映される。""" +def _prompt(tmp_path, **state_over) -> str: state = { "current_pr": PR, "repo": "o/r", "worktree_path": str(tmp_path), - "event_downgrade": event_downgrade, + "event_downgrade": True, "rounds": [{"round": 1, "head_sha": "a" * 40}], } + state.update(state_over) (tmp_path / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) bin_dir = tmp_path / "bin" - bin_dir.mkdir() + bin_dir.mkdir(exist_ok=True) stub = bin_dir / "codex" stub.write_text("#!/bin/sh\nexit 0\n") stub.chmod(0o755) - result = subprocess.run( ["bash", str(SCRIPT), "codex", str(PR), "1"], capture_output=True, text=True, env={**os.environ, "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", "CROSS_REVIEW_TMP_DIR": str(tmp_path)}, ) - assert result.returncode == 0, result.stderr - prompt = (tmp_path / f"codex-review-pr{PR}-prompt.md").read_text() - assert expected_line in prompt + return (tmp_path / f"codex-review-pr{PR}-prompt.md").read_text() + + +# ---------------- 担当は結果だけを残す（#730） ---------------- + + +@pytest.mark.parametrize("word", [ + "gh api", # 投稿の呼び出し + "event_downgrade", # 判定の値の指定（格下げは投稿する側が決める） + "posted_as", + "comments_count", + "review_url", + "post_error", + "line could not be resolved", # インラインの組み立て（422 への対処） + "post: submit review", +]) +def test_the_prompt_has_no_step_to_post(tmp_path, word) -> None: + """担当へ渡すプロンプトに、レビューを投稿する手順が 1 つも無い（AC1・AC2）。""" + assert word.lower() not in _prompt(tmp_path).lower() + + +def test_the_prompt_asks_only_for_the_note_and_the_result(tmp_path) -> None: + """書かせるのは指摘の控えと結果ファイルの 2 つだけ。結果は判定と重要度別の件数（AC2）。""" + prompt = _prompt(tmp_path) + assert f"codex-review-pr{PR}-round1-payload.json" in prompt + assert f"codex-review-pr{PR}-result.json" in prompt + assert '"event"' in prompt and '"by_severity"' in prompt + + +def test_the_prompt_asks_to_rename_the_note_before_the_result(tmp_path) -> None: + """一時の名前で書き、控えを先・結果ファイルを後に改名させる（AC3）。""" + prompt = _prompt(tmp_path) + note_tmp = f"codex-review-pr{PR}-round1-payload.json.tmp" + result_tmp = f"codex-review-pr{PR}-result.json.tmp" + assert note_tmp in prompt and result_tmp in prompt + first_mv = prompt.index(f"mv ") + assert prompt.index(note_tmp, first_mv) < prompt.index(result_tmp, first_mv) + +def test_leftover_temporary_files_are_removed_before_launch(tmp_path) -> None: + """前の起動が残した一時の名前のファイルを持ち越さない。""" + for name in (f"codex-review-pr{PR}-result.json.tmp", + f"codex-review-pr{PR}-round1-payload.json.tmp"): + (tmp_path / name).write_text("{}") + _prompt(tmp_path) + assert not (tmp_path / f"codex-review-pr{PR}-result.json.tmp").exists() + assert not (tmp_path / f"codex-review-pr{PR}-round1-payload.json.tmp").exists() diff --git a/plugins/ndf/skills/cross-review/tests/test_merge_fix_posts.py b/plugins/ndf/skills/cross-review/tests/test_merge_fix_posts.py new file mode 100644 index 00000000..7956614c --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_merge_fix_posts.py @@ -0,0 +1,102 @@ +"""修正の取り込みが、送信と返信・決着・まとめを行う（#730 #585 #676）。 + +**修正の担当はコミットまでを行い、送らない。** 取り込みが現在の頭を指定して送り、 +報告されたコミットが送り先に載ったことを確かめてから、返信・決着・まとめを +待ち行列へ積んで流す。まとめの参照は投稿の応答から記録へ書く。 + +| 何を確かめるか | 受け入れ条件 | +| --- | --- | +| 返信・決着・まとめが積まれて流れる | AC7 | +| 送信は取り込む側が行う | AC8 | +| 報告されたコミットが送り先に無ければ止まる | AC9 | +| 同じ共通層を使う | AC21 | +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + +PR = 5850 +REPO = "o/r" + + +@pytest.fixture() +def tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +def _seed(tmp_dir: pathlib.Path) -> None: + state = { + "current_pr": PR, "repo": REPO, "viewer_login": "takemi", + "worktree_path": str(tmp_dir), "head_branch": "feat/x", + "rounds": [{"round": 2, "pr": PR, "started_at": "2026-01-01T00:00:00+00:00"}], + "deferred_nits": [], "final": None, + } + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text(json.dumps(state)) + + +def _fix(tmp_dir: pathlib.Path) -> pathlib.Path: + path = tmp_dir / f"fix-pr{PR}-result.json" + path.write_text(json.dumps({ + "pr": PR, "fix_commit": "abc1234", "ci_status": "SUCCESS", "fixed_count": 1, + "by_severity": {"major": 1}, + "resolved_threads": [{"thread_id": "PRRT_a", "comment_id": 11}], + "deferred": [], "rejected": [], + }), encoding="utf-8") + return path + + +def _state(tmp_dir: pathlib.Path) -> dict: + return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text()) + + +@pytest.fixture() +def calls(monkeypatch, state_mod): + """送信と投稿の呼び出しを記録する。共通層の口をそのまま差し替える。""" + rp = state_mod.result_posts + seen: dict = {"push": [], "post": []} + + def push(worktree, head, commit): + seen["push"].append((str(worktree), head, commit)) + return rp.PushResult(seen.get("push_ok", True), True, + seen.get("push_ok", True), "") + + def post(queue, result_path, repo, pr, round_no=None, actor=None): + items = rp.fix_posts(result_path, repo, pr, round_no) + seen["post"].append([i["kind"] for i in items]) + return rp.FixOutcome("https://x/pull/5850#issuecomment-9", 1, 1, 0, False, "") + + monkeypatch.setattr(rp, "push_fix", push) + monkeypatch.setattr(rp, "post_fix", post) + return seen + + +def test_the_take_in_pushes_the_head_and_posts_the_replies(tmp_dir, state_mod, calls): + _seed(tmp_dir) + _fix(tmp_dir) + + state_mod.cmd_merge_fix(argparse.Namespace(pr=PR, file=None)) + + assert calls["push"] == [(str(tmp_dir), "feat/x", "abc1234")] + assert calls["post"] == [["review-reply", "thread-resolve", "pr-comment"]] + fix = _state(tmp_dir)["rounds"][-1]["fix"] + assert fix["summary_comment_url"] == "https://x/pull/5850#issuecomment-9" + + +def test_the_take_in_stops_when_the_commit_is_not_on_the_branch( + tmp_dir, state_mod, calls): + """報告されたコミットが送り先に載っていなければ、記録も投稿もせずに止まる（AC9）。""" + _seed(tmp_dir) + _fix(tmp_dir) + calls["push_ok"] = False + + with pytest.raises(SystemExit) as e: + state_mod.cmd_merge_fix(argparse.Namespace(pr=PR, file=None)) + + assert e.value.code != 0 + assert calls["post"] == [] + assert "fix" not in _state(tmp_dir)["rounds"][-1] diff --git a/plugins/ndf/skills/cross-review/tests/test_queue_idempotency.py b/plugins/ndf/skills/cross-review/tests/test_queue_idempotency.py index e2813e19..ba62b5d0 100644 --- a/plugins/ndf/skills/cross-review/tests/test_queue_idempotency.py +++ b/plugins/ndf/skills/cross-review/tests/test_queue_idempotency.py @@ -6,13 +6,16 @@ | 種別 | 照会 | 同じとみなす条件 | | --- | --- | --- | -| `pr-comment` | `repos/{リポジトリ}/issues/{番号}/comments` | 投稿者が自分で、本文が一致 | -| `review-post` | `repos/{リポジトリ}/pulls/{番号}/reviews` | 投稿者・判定・本文の先頭 80 文字が一致 | -| `review-reply` | `repos/{リポジトリ}/pulls/{番号}/comments` | `in_reply_to_id` と本文が一致 | +| `pr-comment` | `repos/{リポジトリ}/issues/{番号}/comments` | 投稿者が自分で、本文の先頭 80 文字が一致 | +| `review-post` | `repos/{リポジトリ}/pulls/{番号}/reviews` | 投稿者と、本文の先頭行のラウンドと席までの前方一致 | +| `review-reply` | `repos/{リポジトリ}/pulls/{番号}/comments` | 返信先の指摘の識別子と、本文の先頭 80 文字が一致 | | `thread-resolve` | 未解決のスレッドの一覧 | 識別子が一覧に無い | 本文の先頭 80 文字で比べるのは、振動の検知が指摘の同一性を測るときと同じ幅である。 **同じ判断に別々の値を持たない。** + +**レビューの照合の鍵に判定の語を含めない。** 含めると、起動し直して判定が変わったときに +別の投稿と読まれ、同じラウンド・同じ席のレビューが 2 件になる（#730 #583）。 """ from __future__ import annotations @@ -27,6 +30,12 @@ BODY = "同じ内容の本文。" * 12 # 80 文字より長い本文で、先頭の照合が効くことを見る +def _review_body(event: str, round_no: int = 3, seat: str = "codex") -> str: + """レビューの本文。先頭行がラウンドと席と判定を持つ（#730 AC10）。""" + return (f"## 🤖 cross-review | round {round_no} | {seat} | {event}\n\n" + + BODY) + + @pytest.fixture() def qdir(tmp_path) -> pathlib.Path: return tmp_path / "pending" @@ -80,12 +89,14 @@ def test_a_review_already_on_github_is_not_posted_again( q = queue_mod.Queue(qdir) queue_mod.enqueue( q, "review-post", REPO, PR, - {"body": BODY, "event": "REQUEST_CHANGES"}, actor=ACTOR) - # 末尾だけが違う本文でも、先頭 80 文字が同じなら同じ投稿とみなす。 + {"body": _review_body("REQUEST_CHANGES"), "event": "REQUEST_CHANGES"}, + actor=ACTOR) + # 末尾だけが違う本文でも、先頭行のラウンドと席までが同じなら同じ投稿とみなす。 fake_gh.set_rules([ {"match": f"pulls/{PR}/reviews", "stdout": json.dumps([{"user": {"login": ACTOR}, "state": "CHANGES_REQUESTED", - "body": BODY + "（末尾の言い回しだけが違う）", + "body": _review_body("REQUEST_CHANGES") + + "（末尾の言い回しだけが違う）", "id": 4961230016, "html_url": "https://x/#pullrequestreview-4961230016"}])}, ]) @@ -100,16 +111,42 @@ def test_a_review_already_on_github_is_not_posted_again( "https://x/#pullrequestreview-4961230016" -def test_a_review_with_a_different_verdict_is_not_the_same( +def test_a_review_with_a_different_verdict_is_still_the_same( queue_mod, fake_gh, qdir) -> None: + """判定が変わっても、同じラウンド・同じ席なら 2 件目を作らない（#730 AC11）。""" + q = queue_mod.Queue(qdir) + queue_mod.enqueue( + q, "review-post", REPO, PR, + {"body": _review_body("REQUEST_CHANGES"), "event": "REQUEST_CHANGES"}, + actor=ACTOR) + fake_gh.set_rules([ + {"match": f"pulls/{PR}/reviews?", + "stdout": json.dumps([{"user": {"login": ACTOR}, "state": "APPROVED", + "body": _review_body("APPROVE")}])}, + {"match": "", "stdout": "{}"}, + ]) + + result = q.flush() + + assert len(result.skipped) == 1 + assert _posted(fake_gh.joined()) == [] + + +@pytest.mark.parametrize("other", [ + _review_body("APPROVE", round_no=4), + _review_body("APPROVE", seat="agy"), +]) +def test_a_review_of_another_round_or_seat_is_not_the_same( + queue_mod, fake_gh, qdir, other) -> None: q = queue_mod.Queue(qdir) queue_mod.enqueue( q, "review-post", REPO, PR, - {"body": BODY, "event": "REQUEST_CHANGES"}, actor=ACTOR) + {"body": _review_body("REQUEST_CHANGES"), "event": "REQUEST_CHANGES"}, + actor=ACTOR) fake_gh.set_rules([ {"match": f"pulls/{PR}/reviews?", "stdout": json.dumps([{"user": {"login": ACTOR}, "state": "APPROVED", - "body": BODY}])}, + "body": other}])}, {"match": "", "stdout": "{}"}, ]) @@ -124,9 +161,11 @@ def test_a_review_reply_already_on_github_is_not_posted_again( queue_mod.enqueue( q, "review-reply", REPO, PR, {"body": BODY, "in_reply_to": 987654}, actor=ACTOR) + # 末尾だけが違う返信でも、先頭 80 文字が同じなら同じ返信とみなす。 fake_gh.set_rules([ {"match": f"pulls/{PR}/comments", - "stdout": json.dumps([{"user": {"login": ACTOR}, "body": BODY, + "stdout": json.dumps([{"user": {"login": ACTOR}, + "body": BODY + "（末尾だけが違う）", "in_reply_to_id": 987654}])}, ]) diff --git a/plugins/ndf/skills/cross-review/tests/test_read_result_posts.py b/plugins/ndf/skills/cross-review/tests/test_read_result_posts.py new file mode 100644 index 00000000..977a280c --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_read_result_posts.py @@ -0,0 +1,230 @@ +"""指摘の取り込みが、控えからレビューを組み立てて送る（#730 #583）。 + +**書き込みはレビューを回す側だけが行う。** 担当は指摘の控えと結果ファイルを書いて +終わり、取り込みがそこから投稿を組み立て、待ち行列を通して送り、送信の応答を記録に +する。担当の申告と GitHub の実数を突き合わせる処理は無くなる。 + +| 何を確かめるか | 受け入れ条件 | +| --- | --- | +| 控えから投稿が組み立ち、1 回の呼び出しで送られる | AC5 | +| 応答に本文が出ない | AC6 | +| 投稿の後・記録の前で止めた実行をやり直しても増えない | AC12 | +| 記録の URL と件数が送信の応答から来る | AC15 | +| インラインが 0 件でも結果なしにならない | AC17 | +| 控えが無ければ投稿を 0 件にする | AC3 | +""" +from __future__ import annotations + +import argparse +import json +import pathlib + +import pytest + +PR = 730 +AGENT = "codex" +ROUND = 2 +REPO = "o/r" +ACTOR = "takemi" +SHA = "f" * 40 +INLINE_TEXT = "[major / 正確性] 戻り値を確かめる" +SUMMARY_TEXT = "層の分け方をそろえると読みやすい" + + +def _seed(tmp_dir: pathlib.Path, **over) -> None: + state = { + "current_pr": PR, + "repo": REPO, + "viewer_login": ACTOR, + "is_own_pr": False, + "event_downgrade": False, + "rounds": [{"round": ROUND, "pr": PR, "head_sha": SHA, + "started_at": "2026-09-22T00:00:00+00:00"}], + "final": None, + } + state.update(over) + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text( + json.dumps(state), encoding="utf-8") + + +def _note(tmp_dir: pathlib.Path, comments: list[dict] | None = None) -> pathlib.Path: + if comments is None: + comments = [{"path": "a.py", "line": 12, "body": INLINE_TEXT, + "severity": "major"}] + path = tmp_dir / f"{AGENT}-review-pr{PR}-round{ROUND}-payload.json" + path.write_text(json.dumps({"summary": SUMMARY_TEXT, "comments": comments}, + ensure_ascii=False), encoding="utf-8") + return path + + +def _result(tmp_dir: pathlib.Path, **over) -> pathlib.Path: + data = {"event": "REQUEST_CHANGES", "by_severity": {"major": 1}} + data.update(over) + path = tmp_dir / f"{AGENT}-review-pr{PR}-result.json" + path.write_text(json.dumps(data, ensure_ascii=False), encoding="utf-8") + return path + + +def _args() -> argparse.Namespace: + return argparse.Namespace(pr=PR, agent=AGENT, file=None) + + +def _state(tmp_dir: pathlib.Path) -> dict: + return json.loads( + (tmp_dir / f"cross-review-pr{PR}-state.json").read_text(encoding="utf-8")) + + +def _entry(tmp_dir: pathlib.Path) -> dict: + return _state(tmp_dir)["rounds"][-1][AGENT] + + +@pytest.fixture() +def tmp_dir(monkeypatch, tmp_path, state_mod): + monkeypatch.setenv("CROSS_REVIEW_TMP_DIR", str(tmp_path)) + return tmp_path + + +_ACCEPT = {"match": f"pulls/{PR}/reviews", "stdout": json.dumps( + {"id": 555, "html_url": f"https://github.com/o/r/pull/{PR}#pullrequestreview-555"})} +_NO_PRIOR = {"match": f"pulls/{PR}/reviews?", "stdout": "[]"} +_URL = f"https://github.com/o/r/pull/{PR}#pullrequestreview-555" + + +def test_the_take_in_posts_the_review_and_records_the_response( + tmp_dir, state_mod, fake_gh, capsys) -> None: + _seed(tmp_dir) + _note(tmp_dir) + _result(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + state_mod.cmd_read_result(_args()) + + entry = _entry(tmp_dir) + assert entry["review_url"] == _URL # 送信の応答から取る（AC15） + assert entry["comments"] == 1 # 送れたインラインの数 + assert entry["intent"] == "REQUEST_CHANGES" + assert entry["queued"] is False + out = capsys.readouterr().out + assert f"POSTED review_url={_URL}" in out + assert "INLINE=1 BODY=0 QUEUED=0" in out + assert "FINDINGS=1" in out + + +def test_no_body_reaches_the_answer(tmp_dir, state_mod, fake_gh, capsys) -> None: + """取り込みの出力に、レビューの本文もインラインの本文も出ない（AC6）。""" + _seed(tmp_dir) + _note(tmp_dir) + _result(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + state_mod.cmd_read_result(_args()) + + captured = capsys.readouterr() + assert INLINE_TEXT not in captured.out and INLINE_TEXT not in captured.err + assert SUMMARY_TEXT not in captured.out and SUMMARY_TEXT not in captured.err + assert len(captured.out.splitlines()) <= 20 + + +def test_the_body_is_sent_from_the_note(tmp_dir, state_mod, fake_gh) -> None: + """本文は控えから組み立てて送る。先頭行がラウンドと席を持つ（AC5・AC10）。""" + _seed(tmp_dir) + _note(tmp_dir) + _result(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + state_mod.cmd_read_result(_args()) + + sent = [c for c in fake_gh.calls() if "--method POST" in " ".join(c["argv"])] + body = json.loads(sent[-1]["stdin"]) + assert body["body"].splitlines()[0] == \ + f"## 🤖 cross-review | round {ROUND} | {AGENT} | REQUEST_CHANGES" + assert body["commit_id"] == SHA + assert body["comments"][0]["path"] == "a.py" + + +def test_a_second_take_in_does_not_add_a_second_review( + tmp_dir, state_mod, fake_gh) -> None: + """投稿の後・記録の前で止めた実行をやり直しても、レビューは増えない（AC12・AC20）。""" + _seed(tmp_dir) + _note(tmp_dir) + _result(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + state_mod.cmd_read_result(_args()) + posted_once = len([c for c in fake_gh.joined() if "--method POST" in c]) + + # 記録を消して、投稿だけが残った状態を作る。 + st = _state(tmp_dir) + st["rounds"][-1].pop(AGENT, None) + (tmp_dir / f"cross-review-pr{PR}-state.json").write_text( + json.dumps(st), encoding="utf-8") + fake_gh.set_rules([ + {"match": f"pulls/{PR}/reviews?", "stdout": json.dumps([{ + "user": {"login": ACTOR}, "state": "CHANGES_REQUESTED", "id": 555, + "body": f"## 🤖 cross-review | round {ROUND} | {AGENT} | REQUEST_CHANGES\n", + "html_url": _URL}])}, + ]) + + state_mod.cmd_read_result(_args()) + + assert len([c for c in fake_gh.joined() if "--method POST" in c]) == posted_once + assert _entry(tmp_dir)["review_url"] == _URL + + +def test_findings_without_an_inline_are_still_a_result( + tmp_dir, state_mod, fake_gh, capsys) -> None: + """指摘があってインラインが 0 件でも、その担当は結果なしにならない（AC17）。""" + _seed(tmp_dir) + _note(tmp_dir, comments=[{"body": SUMMARY_TEXT, "severity": "major"}]) + _result(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + state_mod.cmd_read_result(_args()) + + entry = _entry(tmp_dir) + assert entry["intent"] == "REQUEST_CHANGES" + assert entry["comments"] == 0 + assert "INLINE=0 BODY=1" in capsys.readouterr().out + assert len(_state(tmp_dir)["review_findings"]) == 1 + + +def test_a_note_alone_is_treated_as_no_result(tmp_dir, state_mod, fake_gh) -> None: + """控えだけがあって結果ファイルが無ければ、投稿を 0 件にする（AC3・AC4）。""" + _seed(tmp_dir) + _note(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + with pytest.raises(SystemExit): + state_mod.cmd_read_result(_args()) + + assert [c for c in fake_gh.joined() if "--method POST" in c] == [] + + +def test_only_what_is_sent_is_downgraded_on_ones_own_pull_request( + tmp_dir, state_mod, fake_gh) -> None: + """自分の Pull Request では送った形だけを落とす（AC32）。""" + _seed(tmp_dir, is_own_pr=True, event_downgrade=True) + _note(tmp_dir) + _result(tmp_dir) + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + state_mod.cmd_read_result(_args()) + + sent = [c for c in fake_gh.calls() if "--method POST" in " ".join(c["argv"])] + assert json.loads(sent[-1]["stdin"])["event"] == "COMMENT" + entry = _entry(tmp_dir) + assert entry["intent"] == "REQUEST_CHANGES" and entry["posted_as"] == "COMMENT" + + +def test_a_reviewer_that_wrote_nothing_adds_no_review( + tmp_dir, state_mod, fake_gh) -> None: + """控えも結果も書かずに終わった担当では、レビューが 1 件も増えない（AC4）。""" + _seed(tmp_dir) + # 書きかけの一時の名前だけが残った状態も、正式の名前が無ければ結果なしである。 + (tmp_dir / f"{AGENT}-review-pr{PR}-round{ROUND}-payload.json.tmp").write_text("{") + fake_gh.set_rules([_NO_PRIOR, _ACCEPT]) + + with pytest.raises(SystemExit): + state_mod.cmd_read_result(_args()) + + assert [c for c in fake_gh.joined() if "--method POST" in c] == [] + assert _entry(tmp_dir)["intent"] == "NO_RESULT" diff --git a/plugins/ndf/skills/cross-review/tests/test_review_findings.py b/plugins/ndf/skills/cross-review/tests/test_review_findings.py index 19ca5a56..2be1f07c 100644 --- a/plugins/ndf/skills/cross-review/tests/test_review_findings.py +++ b/plugins/ndf/skills/cross-review/tests/test_review_findings.py @@ -74,8 +74,7 @@ def tmp_dir(monkeypatch, tmp_path, state_mod): @pytest.fixture(autouse=True) def _no_github_checks(monkeypatch, state_mod): - """GitHub 側の突き合わせを通す。取り込みの形だけを見る。""" - monkeypatch.setattr(state_mod, "_posted_comment_count", lambda *a, **k: None) + """流した後の実在確認を通す。取り込みの形だけを見る。""" monkeypatch.setattr(state_mod, "_review_exists", lambda *a, **k: True) @@ -203,14 +202,18 @@ def test_a_state_file_without_the_key_is_readable(tmp_dir, state_mod): assert len(_read(tmp_dir)["review_findings"]) == 1 -def test_the_comment_count_is_unchanged(tmp_dir, state_mod): - """`comments_count` は投稿したインラインの数のままにする。""" +def test_the_comment_count_is_the_number_of_inlines_sent(tmp_dir, state_mod): + """件数は担当の申告ではなく、送れたインラインの数である（#730 AC15）。 + + 送り先を決めるのは投稿する側で、控えの `posted_to` は読まない。位置を持つ指摘は + どちらもインラインとして送る。 + """ _write(tmp_dir, _state()); _result(tmp_dir, comments_count=3) _payload(tmp_dir, [FULL, {**FULL, "posted_to": "body"}]) _read_result(state_mod) - assert _read(tmp_dir)["rounds"][-1][AGENT]["comments"] == 3 + assert _read(tmp_dir)["rounds"][-1][AGENT]["comments"] == 2 def test_a_missing_payload_does_not_break_the_import(tmp_dir, state_mod): @@ -336,12 +339,18 @@ def test_a_failed_reimport_does_not_erase_what_was_taken(tmp_dir, state_mod): SKILL = pathlib.Path(__file__).resolve().parents[1] -def test_the_prompt_asks_for_the_four_items(): +def test_the_prompt_asks_for_the_three_items(): text = (SKILL / "scripts/launch-reviewer.sh").read_text(encoding="utf-8") - for key in ("evidence", "falsification", "suggested_check", "posted_to"): + for key in ("evidence", "falsification", "suggested_check"): assert key in text, key +def test_the_prompt_does_not_ask_where_it_was_posted(): + """送れた先（`posted_to`）は投稿する側が書く。担当には書かせない（#730）。""" + text = (SKILL / "scripts/launch-reviewer.sh").read_text(encoding="utf-8") + assert "posted_to" not in text + + def test_the_prompt_asks_for_body_only_findings(): """総評だけへ書いた指摘も payload へ載せることを求める。""" text = (SKILL / "scripts/launch-reviewer.sh").read_text(encoding="utf-8") diff --git a/plugins/ndf/skills/cross-review/tests/test_skill_bg_wait.py b/plugins/ndf/skills/cross-review/tests/test_skill_bg_wait.py index 10b59100..a9f2c066 100644 --- a/plugins/ndf/skills/cross-review/tests/test_skill_bg_wait.py +++ b/plugins/ndf/skills/cross-review/tests/test_skill_bg_wait.py @@ -1,7 +1,7 @@ """cross-review の骨組みは 600 秒を超える監視を区切って待つ（#598 / #537 の AC41 / AC42）。 レビューの監視の上限は 1200 秒で、Claude Code の Bash ツールの 1 回（600 秒）に収まらない。 -骨組みはレビューの監視（起動し直しを含む）と `critique-round.sh` を `bg-wait.sh run` で +骨組みはレビューの監視（起動し直しも同じ行を通る、#583）と `critique-round.sh` を `bg-wait.sh run` で 背景に回し、`bg-wait.sh wait`（1 回 540 秒以内）を 124 のあいだ繰り返す。 """ from __future__ import annotations @@ -38,15 +38,15 @@ def _long_runner_lines() -> list[tuple[int, str]]: if not line.lstrip().startswith("#") and any(r in line for r in LONG_RUNNERS)] -def test_the_skeleton_has_the_three_long_runners() -> None: - """レビューの監視・起動し直しの監視・反証の 1 ラウンド。""" +def test_the_skeleton_has_the_two_long_runners() -> None: + """レビューの監視と反証の 1 ラウンド。起動し直しは同じ監視の行を通る（#583）。""" found = _long_runner_lines() - assert len(found) == 3 - assert sum('"$SCRIPTS/monitor.py"' in line for _, line in found) == 2 + assert len(found) == 2 + assert sum('"$SCRIPTS/monitor.py"' in line for _, line in found) == 1 assert sum('"$SCRIPTS/critique-round.sh"' in line for _, line in found) == 1 -@pytest.mark.parametrize("index", range(3)) +@pytest.mark.parametrize("index", range(2)) def test_each_long_runner_is_started_by_bg_wait_run(index: int) -> None: _, line = _long_runner_lines()[index] runner = next(r for r in LONG_RUNNERS if r in line) @@ -54,7 +54,7 @@ def test_each_long_runner_is_started_by_bg_wait_run(index: int) -> None: assert re.search(r'"\$SCRIPTS/bg-wait\.sh" run "[^"]+" -- $', head), line -@pytest.mark.parametrize("index", range(3)) +@pytest.mark.parametrize("index", range(2)) def test_each_run_is_followed_by_one_wait_on_the_same_rc(index: int) -> None: """**待ちは 1 回の呼び出しに 1 つだけ書く。** 繰り返しを 1 回の Bash へ書くと、 2 回目の待ちに入った時点で合計が 600 秒を超え、ホストに打ち切られる（#683 round 2）。""" diff --git a/plugins/ndf/skills/cross-review/tests/test_skill_layout.py b/plugins/ndf/skills/cross-review/tests/test_skill_layout.py index e440a67a..e5ed590d 100644 --- a/plugins/ndf/skills/cross-review/tests/test_skill_layout.py +++ b/plugins/ndf/skills/cross-review/tests/test_skill_layout.py @@ -178,3 +178,40 @@ def test_the_evidence_doc_says_the_mark_is_removed_when_critiques_are_incomplete def test_the_specification_holds_the_same_six_classifications() -> None: """区分の表と行き先の表の両方が `unrefuted` を持つ（AC20）。""" assert SPEC.read_text(encoding="utf-8").count("unrefuted") >= 2 + + +# ---------- 起動し直しは初回と同じ経路を通る（#583 #730） ---------- + + +def _loop() -> str: + """骨組みの繰り返し（`while :; do` から `done` まで）。""" + text = SKILL.read_text(encoding="utf-8") + block = text.split("## 実行ステップ概要（メインの bash 骨組み）", 1)[1] + block = block.split("```bash", 1)[1].split("\n```", 1)[0] + return block.split("while :; do", 1)[1] + + +@pytest.mark.parametrize("step", [ + '"$SCRIPTS/launch-reviewer.sh"', + '"$SCRIPTS/state.py" read-result', + '"$SCRIPTS/state.py" verify-findings', + '"$SCRIPTS/critique-round.sh"', +]) +def test_each_step_of_a_review_is_written_once(step: str) -> None: + """起動し直しの枝が自分の起動・取り込みを持たない。経路は 1 本である（AC18）。 + + 枝が別に持つと、根拠の検証と反証を飛ばして 2 度目の判定へ進む。 + """ + assert _loop().count(step) == 1, step + + +def test_the_relaunch_goes_back_to_the_head_of_the_review() -> None: + loop = _loop() + relaunch = loop.index('"$JUDGE_RC" -eq 7') + assert "continue" in loop[relaunch:loop.index("done", relaunch)] + + +def test_the_queue_branch_is_seen_before_the_relaunch() -> None: + """待ち行列に残りがあるときの枝は、各判定の直後、7 より先に見る（AC19）。""" + loop = _loop() + assert loop.index('"$JUDGE_RC" -eq 8') < loop.index('"$JUDGE_RC" -eq 7') diff --git a/plugins/ndf/skills/cross-review/tests/test_state_not_posted.py b/plugins/ndf/skills/cross-review/tests/test_state_not_posted.py index 72c3ff55..58da6c4e 100644 --- a/plugins/ndf/skills/cross-review/tests/test_state_not_posted.py +++ b/plugins/ndf/skills/cross-review/tests/test_state_not_posted.py @@ -1,19 +1,18 @@ -"""投稿が届いていないレビューを結果なしとして扱う（#261）。 +"""投稿が届いたことを、送った後に確かめる（#261 #730）。 -レビュアーが投稿に失敗したとき、判定だけが残り、指摘の中身が Pull Request に無いまま -修正の工程へ進んでいた。修正の担当は Pull Request のコメントを読んで直すため、指摘が -無ければ直すものが無く、ラウンドだけが 1 つ増える。 +**投稿するのはレビューを回す側である**（#730）。担当は投稿せず、結果ファイルに +投稿の失敗や参照を申告しない。取り込みは送信の応答をそのまま記録にするため、 +申告を読んで結果なしにする経路は無い。 -| 結果ファイルの状態 | 扱い | +残るのは、上限で積んだ投稿を後から流したときの確かめである。流した直後に参照から +照会し、届いていなければ結果なしとして記録して、判定の「同じラウンドで 1 度だけ +起動し直す」経路へ乗せる（`_confirm_flushed`）。この文書はその照会の振る舞いを見る。 + +| 照会の結果 | 扱い | | --- | --- | -| `post_error` に値がある | 届いていない。結果なしとして記録する | -| `review_url` が空、または識別子を取り出せない | 届いていない。結果なしとして記録する | -| 識別子から照会してレビューが存在する | 届いた。これまでどおり取り込む | -| 識別子から照会できない | 申告を採用し、確認できなかったことを出力へ残す | - -結果なしとして記録すると、判定（`state.py judge`）の「同じラウンドで 1 度だけ起動し直す」 -経路へ乗る。修正の担当から見ると、結果が残らなかった場合と、結果はあるが指摘が届いて -いない場合は同じ状態である（読むべき指摘が無い）。 +| 識別子から照会してレビューが存在する | 届いた | +| 識別子を取り出せない | 届いていない | +| 照会できない・何も返らない | 分からない（届いていないとは読まない） | """ from __future__ import annotations @@ -66,54 +65,6 @@ def _round(tmp_dir: pathlib.Path) -> dict: return json.loads((tmp_dir / f"cross-review-pr{PR}-state.json").read_text())["rounds"][-1] -def test_a_post_error_is_recorded_as_no_result(tmp_dir, state_mod, monkeypatch): - """実測の形。`post_error` があり、`review_url` も空で件数もすべて 0。""" - _seed_state(tmp_dir) - monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: True) - - with pytest.raises(SystemExit) as e: - state_mod.cmd_read_result( - _args(_result(tmp_dir, review_url="", post_error="gh api failed")) - ) - - assert e.value.code == 1 - assert _round(tmp_dir)[AGENT]["intent"] == "NO_RESULT" - assert _round(tmp_dir)[AGENT]["no_result_reason"] == "not_posted" - - -def test_an_empty_review_url_is_recorded_as_no_result(tmp_dir, state_mod, monkeypatch): - _seed_state(tmp_dir) - monkeypatch.setattr(state_mod, "_sh", lambda cmd, check=True: "") - - with pytest.raises(SystemExit) as e: - state_mod.cmd_read_result(_args(_result(tmp_dir, review_url=""))) - - assert e.value.code == 1 - assert _round(tmp_dir)[AGENT]["no_result_reason"] == "not_posted" - - -def test_a_review_missing_on_github_is_recorded_as_no_result(tmp_dir, state_mod, monkeypatch): - _seed_state(tmp_dir) - monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: False) - - with pytest.raises(SystemExit) as e: - state_mod.cmd_read_result(_args(_result(tmp_dir))) - - assert e.value.code == 1 - assert _round(tmp_dir)[AGENT]["no_result_reason"] == "not_posted" - - -def test_an_unavailable_lookup_keeps_the_declaration(tmp_dir, state_mod, monkeypatch, capsys): - """照会できないときは申告を採用する。通信の失敗でループを止めない。""" - _seed_state(tmp_dir) - monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: None) - - state_mod.cmd_read_result(_args(_result(tmp_dir))) - - assert _round(tmp_dir)[AGENT]["intent"] == "REQUEST_CHANGES" - assert "確認できませんでした" in capsys.readouterr().err - - def test_a_posted_review_is_merged(tmp_dir, state_mod, monkeypatch): _seed_state(tmp_dir) monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: True) @@ -159,16 +110,3 @@ def test_the_lookup_is_none_when_the_api_returns_nothing(state_mod, monkeypatch) monkeypatch.setattr(state_mod, "_sh", lambda cmd, check=True: "") assert state_mod._review_exists("o/r", PR, REVIEW_URL) is None - - -def test_a_no_result_record_sends_the_judge_to_a_relaunch(tmp_dir, state_mod, monkeypatch): - """結果なしの記録を受けて、判定が起動し直しへ進む（終了コード 7）。""" - _seed_state(tmp_dir) - monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: False) - with pytest.raises(SystemExit): - state_mod.cmd_read_result(_args(_result(tmp_dir))) - - with pytest.raises(SystemExit) as e: - state_mod.cmd_judge(argparse.Namespace(pr=PR)) - - assert e.value.code == 7 diff --git a/plugins/ndf/skills/cross-review/tests/test_state_posted_comments.py b/plugins/ndf/skills/cross-review/tests/test_state_posted_comments.py deleted file mode 100644 index bf53ee2a..00000000 --- a/plugins/ndf/skills/cross-review/tests/test_state_posted_comments.py +++ /dev/null @@ -1,199 +0,0 @@ -"""申告されたインラインコメント数を、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 = "agy" -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(autouse=True) -def review_posted(monkeypatch, state_mod): - """投稿の実在確認は届いた前提にする。件数の突き合わせだけを見るため。""" - monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: True) - - -@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_not_posted(tmp_dir, state_mod, monkeypatch): - """投稿先の参照が無ければ、レビューが届いていないものとして扱う（#261）。 - - 件数の突き合わせより前に、投稿そのものが届いたかを見る。 - """ - _seed_state(tmp_dir) - monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: False) - - with pytest.raises(SystemExit) as e: - state_mod.cmd_read_result(_args(_result(tmp_dir, review_url=None))) - - assert e.value.code == 1 - assert _read_state(tmp_dir)["rounds"][-1][AGENT]["intent"] == "NO_RESULT" - - -# ---------------- 件数の取得 ---------------- - -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/skills/cross-review/tests/test_state_queue_judge.py b/plugins/ndf/skills/cross-review/tests/test_state_queue_judge.py index bae7a2a0..79252286 100644 --- a/plugins/ndf/skills/cross-review/tests/test_state_queue_judge.py +++ b/plugins/ndf/skills/cross-review/tests/test_state_queue_judge.py @@ -5,8 +5,8 @@ | 段階 | #261 の検査 | 待ち行列を入れた後 | | --- | --- | --- | -| 結果を取り込む | 投稿の失敗があれば結果なし | `queued` が真の結果はこの検査を通す | -| 結果を取り込む | 投稿先の参照の存在を照会 | `queued` が真のときは照会しない（識別子がまだ無い） | +| 結果を取り込む | 投稿の失敗があれば結果なし | 取り込みが自分で送る。上限で送れなければ積んで `queued` を真にする（#730） | +| 結果を取り込む | 投稿先の参照の存在を照会 | 照会しない。参照は送信の応答から取る（#730） | | 流した直後 | — | 参照を書き戻し、存在を 1 度だけ確かめる | | 判定 | 収束 | 待ち行列が空のときだけ | """ @@ -186,51 +186,28 @@ def test_a_review_that_did_not_arrive_is_recorded_as_no_result( # ---- 受け入れ条件 13 ---- -def test_a_queued_result_skips_the_arrival_check(state_mod, tmp_dir, - monkeypatch) -> None: - """積んだ時点では届いていない。照会すると結果なしになり、二重に積まれる。""" +def test_a_post_refused_by_the_limit_is_queued_and_recorded( + state_mod, queue_mod, fake_gh, tmp_dir) -> None: + """上限で送れなかった投稿は積まれ、記録は `queued` になる（#730）。""" _seed(tmp_dir, rounds=[{"round": 1, "pr": PR, "started_at": "2026-09-03T00:00:00+00:00"}]) - called: list = [] - monkeypatch.setattr(state_mod, "_review_exists", - lambda *a: called.append(a) or True) - monkeypatch.setattr(state_mod, "_posted_comment_count", - lambda *a: called.append(a) or 0) rfile = tmp_dir / "result.json" - rfile.write_text(json.dumps({ - "event": "REQUEST_CHANGES", "posted_as": "COMMENT", "comments_count": 3, - "review_url": "", "queued": True, - "post_error": "API rate limit exceeded", - "by_severity": {"major": 3}, - }), encoding="utf-8") + rfile.write_text(json.dumps({"event": "REQUEST_CHANGES", + "by_severity": {"major": 3}}), encoding="utf-8") + fake_gh.set_rules([ + {"match": f"pulls/{PR}/reviews?", "stdout": "[]"}, + {"match": f"pulls/{PR}/reviews", "exit": 1, + "stdout": '{"message":"API rate limit exceeded for user ID 1.","status":"403"}', + "stderr": "gh: API rate limit exceeded for user ID 1. (HTTP 403)\n"}, + ]) state_mod.cmd_read_result(argparse.Namespace(pr=PR, agent="codex", file=str(rfile))) - assert called == [] entry = _state(tmp_dir)["rounds"][0]["codex"] assert entry["intent"] == "REQUEST_CHANGES" assert entry["queued"] is True - - -def test_a_normal_result_still_checks_the_arrival(state_mod, tmp_dir, - monkeypatch) -> None: - """`queued` を持たない結果ファイルの扱いは変えない（#261 のまま）。""" - _seed(tmp_dir, rounds=[{"round": 1, "pr": PR, - "started_at": "2026-09-03T00:00:00+00:00"}]) - called: list = [] - monkeypatch.setattr(state_mod, "_review_exists", - lambda *a: called.append(a) or True) - rfile = tmp_dir / "result.json" - rfile.write_text(json.dumps({ - "event": "APPROVE", "comments_count": 0, "review_url": REVIEW_URL, - "by_severity": {}, - }), encoding="utf-8") - - state_mod.cmd_read_result(argparse.Namespace(pr=PR, agent="codex", - file=str(rfile))) - - assert len(called) == 1 + assert queue_mod.Queue(tmp_dir / "pending").count() == 1 # ---- 流した結果を、再開の入口の書き戻しが消さない ---- @@ -275,34 +252,40 @@ def test_the_resume_keeps_what_the_flush_wrote_to_the_state( # 書き戻す側の変更も残る。どちらか一方だけが残る直し方にしない。 assert saved["manual_extra_review_instructions"] == "重点観点" assert queue_mod.Queue(tmp_dir / "pending").count() == 0 -# ---- 取り込みの前に流さない ---- +# ---- 取り込みの入口で残りを流す ---- -def test_the_read_result_does_not_flush_the_queue( - state_mod, queue_mod, fake_gh, tmp_dir) -> None: - """取り込みの入口では流さない。**書き戻し先がまだ無い。** +def test_the_take_in_flushes_what_was_left_before_posting( + state_mod, queue_mod, fake_gh, tmp_dir, monkeypatch) -> None: + """前の取り込みで積んだ投稿を先に流し、その担当の記録へ書き戻す（#730）。 - 流すと `_confirm_flushed` は書き戻せないまま項目が消え、この後の取り込みが - `queued: true` だけを保存する。待ち行列は空になるため判定は収束させ、投稿の - 存在も参照も一度も確かめられない。 + 積むのは取り込みだけで、積んだ時点でその担当の記録を書くため、書き戻し先がある。 """ _seed(tmp_dir, rounds=[{"round": 1, "pr": PR, - "started_at": "2026-09-03T00:00:00+00:00"}]) + "started_at": "2026-09-03T00:00:00+00:00", + "agy": {"intent": "APPROVE", "queued": True, + "by_severity": {}}}]) queue_mod.enqueue( queue_mod.Queue(tmp_dir / "pending"), "review-post", REPO, PR, - {"body": "指摘の本文", "event": "APPROVE"}, - actor="me", extra={"agent": "codex", "round": 1}) + {"body": "## 🤖 cross-review | round 1 | agy | APPROVE\n", "event": "APPROVE"}, + actor="me", extra={"agent": "agy", "round": 1}) rfile = tmp_dir / "result.json" - rfile.write_text(json.dumps({ - "event": "APPROVE", "comments_count": 0, "review_url": "", - "queued": True, "by_severity": {}, - }), encoding="utf-8") + rfile.write_text(json.dumps({"event": "APPROVE", "by_severity": {}}), + encoding="utf-8") + fake_gh.set_rules([ + {"match": f"pulls/{PR}/reviews?", "stdout": "[]"}, + {"match": "", "stdout": json.dumps( + {"id": 4961230016, "html_url": REVIEW_URL})}, + ]) + monkeypatch.setattr(state_mod, "_review_exists", lambda repo, pr, url: True) state_mod.cmd_read_result(argparse.Namespace(pr=PR, agent="codex", file=str(rfile))) - assert queue_mod.Queue(tmp_dir / "pending").count() == 1 - assert fake_gh.joined() == [] + rnd = _state(tmp_dir)["rounds"][0] + assert rnd["agy"]["queued"] is False and rnd["agy"]["review_url"] == REVIEW_URL + assert rnd["codex"]["queued"] is False + assert queue_mod.Queue(tmp_dir / "pending").count() == 0 def test_the_queued_reviews_are_confirmed_once_both_results_are_taken_in( diff --git a/plugins/ndf/skills/cross-review/tests/test_state_read_result.py b/plugins/ndf/skills/cross-review/tests/test_state_read_result.py index 323bff6e..6700da5a 100644 --- a/plugins/ndf/skills/cross-review/tests/test_state_read_result.py +++ b/plugins/ndf/skills/cross-review/tests/test_state_read_result.py @@ -71,7 +71,8 @@ def test_canonical_schema(patched_tmp_dir, state_mod): assert merged["intent"] == "APPROVE" assert merged["posted_as"] == "APPROVE" assert merged["comments"] == 0 - assert merged["review_url"] == "https://example/pr/1#1" + # 参照は担当の申告ではなく送信の応答から取る（#730 AC15）。 + assert merged["review_url"].endswith("#pullrequestreview-1") assert merged["by_severity"]["critical"] == 0 @@ -93,9 +94,10 @@ def test_alias_schema_intent_and_comment_count(patched_tmp_dir, state_mod): st = _read_state(tmp_dir) merged = st["rounds"][-1][AGENT] assert merged["intent"] == "APPROVE" - # posted_as は別名 result.json には存在しないので intent と同値にフォールバック + # posted_as は投稿する側が決める。自分の Pull Request でなければ intent と同じ assert merged["posted_as"] == "APPROVE" - assert merged["comments"] == 3 + # 件数は担当の申告（comment_count）ではなく、送れたインラインの数（#730 AC15） + assert merged["comments"] == 0 def test_missing_event_and_intent_dies(patched_tmp_dir, state_mod): @@ -169,3 +171,27 @@ def test_invalid_json_result_file_dies(patched_tmp_dir, state_mod, capsys): assert e.value.code == 3 captured = capsys.readouterr() assert "parse" in captured.err.lower() or "parse" in captured.err + + +def test_the_take_in_passes_the_start_of_the_round( + patched_tmp_dir, state_mod, monkeypatch): + """取り込みは、二度書かない照合を絞るためにラウンドの開始時刻を渡す。 + + ラウンドの番号は回し直すと 1 から数え直すため、前の実行のレビューと取り違えない。 + """ + tmp_dir = patched_tmp_dir + _seed_state(tmp_dir) + rfile = tmp_dir / "result.json" + rfile.write_text(json.dumps({"event": "APPROVE", "comments_count": 0})) + seen = {} + offline = state_mod.result_posts.post_review + + def _spy(*a, **kw): + seen["since"] = kw.get("since") + return offline(*a, **kw) + + monkeypatch.setattr(state_mod.result_posts, "post_review", _spy) + + state_mod.cmd_read_result(_make_args(rfile)) + + assert seen["since"] == "2026-05-21T00:00:00+00:00" diff --git a/plugins/ndf/skills/cross-review/tests/test_writes_by_conductor_docs.py b/plugins/ndf/skills/cross-review/tests/test_writes_by_conductor_docs.py new file mode 100644 index 00000000..b9006590 --- /dev/null +++ b/plugins/ndf/skills/cross-review/tests/test_writes_by_conductor_docs.py @@ -0,0 +1,52 @@ +"""GitHub と git へ書くのをレビューを回す側だけにした決定が、文書に反映されている（#730）。 + +| 受け入れ条件 | 文書 | 消える記述 | 入る記述 | +| --- | --- | --- | --- | +| AC25 | `SKILL.md` の設計方針の表 | 担当が `gh api` で直接投稿する | 投稿の担い手とその理由 | +| AC26 | `docs/03-review-output.md` | 担当の直接投稿の決定 | 待ち行列を通す形 | +| AC27 | `docs/02-fix-and-rotation.md` | 担当の送信の行 | 取り込む側の送信 | +| AC28 | `references/context-budget.md` | 中間ペイロードがメインを通らない | 本文がプロセスの中だけを通る | +| AC29 | `docs/04-contracts.md` | — | 投稿の種別ごとの契約 | +""" +from __future__ import annotations + +import pathlib + +import pytest + +HERE = pathlib.Path(__file__).resolve().parent.parent + + +def _read(rel: str) -> str: + return (HERE / rel).read_text(encoding="utf-8") + + +def test_the_policy_table_names_who_posts() -> None: + text = _read("SKILL.md") + assert "AI 自身が `gh api` で PR に直接投稿" not in text + assert "| 投稿の担い手 |" in text + + +def test_the_review_output_goes_through_the_queue() -> None: + text = _read("docs/03-review-output.md") + assert "AI 直接投稿" not in text + assert "待ち行列" in text + + +def test_the_fix_procedure_does_not_push() -> None: + text = _read("docs/02-fix-and-rotation.md") + assert "git push origin {HEAD_BRANCH}" not in text + assert "HEAD:<ブランチ名>" in text + + +def test_the_context_budget_keeps_bodies_in_the_process() -> None: + text = _read("references/context-budget.md") + assert "中間ペイロードがメインを通らない" not in text + assert "プロセスの中だけを通り" in text + + +@pytest.mark.parametrize("kind", ["review-post", "review-reply", "thread-resolve", "pr-comment"]) +def test_the_contract_lists_each_kind_of_post(kind: str) -> None: + text = _read("docs/04-contracts.md") + section = text.split("## 投稿の種別ごとの契約", 1)[1].split("\n## ", 1)[0] + assert f"| `{kind}` |" in section diff --git a/plugins/ndf/skills/fix/SKILL.md b/plugins/ndf/skills/fix/SKILL.md index 7b1c51d0..db38d841 100644 --- a/plugins/ndf/skills/fix/SKILL.md +++ b/plugins/ndf/skills/fix/SKILL.md @@ -56,19 +56,33 @@ PR: ) ``` -サブエージェント側ではこの SKILL.md を読み込んで、自己完結で -**修正 → コミット → push → reply → Resolve Conversation** まで実行する。 -メインへの戻り値は最小限のサマリのみ。 +サブエージェント側ではこの SKILL.md を読み込んで、**修正 → コミット → 戻り値ファイル** +までを行う。メインへの戻り値は最小限のサマリのみ。 -**push が credential helper の不全で落ちたときは退避する**（#524）。`gh` が認証済みでも -`git` だけが `Authentication failed` を返す環境がある。 +**修正の担当は GitHub と git へ書かない。** 送信・返信・スレッドの決着・まとめの投稿は、 +戻り値ファイルを読んだ側が行う（#730）。担当が送ると、送ったという報告と実物が食い違う +状態（切り離された頭では、ブランチ名だけの送信が何も送らずに終了コード 0 で終わる）と、 +途中で止まったときに投稿だけが残る状態が作れる。 + +| 起動のされ方 | 書き込みを行う側 | +| --- | --- | +| `/ndf:cross-review` から | 修正の取り込み（`state.py merge-fix`）が行う | +| 単独で呼んだ | 戻り値ファイルを書いた後、次の 1 行を実行する | ```bash -git -c credential.helper= -c credential.helper='!gh auth git-credential' push +python3 "$SCRIPTS/lib/result_posts.py" fix --pr <番号> --result <戻り値ファイル> \ + [--repo <所有者>/<リポジトリ>] [--head <ブランチ名>] [--worktree <作業ツリー>] [--round ] ``` -**空の値を先に置く。** `credential.helper` は複数の値を持てる設定で、`git` は宣言された -順に問い合わせる。空の値だけが一覧を空へ戻す。 +`$SCRIPTS` の決め方は `development-workflow` の `references/scripts-lookup.md` にある。 +`--repo` / `--head` / `--worktree` を省いたときは、いまいる作業ツリーと Pull Request から引く。 +**送り先のブランチを決められないときは、返信へ進まず終了コード 1 で止まる。** 送っていない +修正へ「対応しました」と返信しないためである。 +このコマンドが現在の頭を送り先へ送り（`git push origin HEAD:<ブランチ名>`）、報告した +コミットが送り先に載ったことを確かめてから、返信・決着・まとめを待ち行列を通して送る。 +出力は件数と参照だけで、本文を出さない。 +送信が認証で落ちたときの退避（#524）もこのコマンドが行う。値は共通層の +`scripts/lib/git-credential.sh` 1 か所が持つ。 ## コメントの取得（3 ソース） @@ -203,14 +217,11 @@ GitHub MCP を使う場合は `mcp__github__get_pull_request_comments` を利用 4. 問題点を修正。**コード行数が減る方向の修正は積極的に実施**（重複排除、不要分岐除去） 5. **コミット前の再確認** — 作業中に新しいコメントが追加されていないか再取得し、CI 状態も 現時点だけ確認する（完了待ちはしない）。新しい指摘・失敗があれば手順 3 に戻る -6. コミット・プッシュ -7. **PR レベルの Summary コメントを投稿**（対応件数 + deferred 件数を明記） -8. 対応したインラインコメントに個別に返信 -9. **deferred スレッドには `[deferred / nit]` ラベル付き返信** を投稿（Resolve はしない） -10. reviewer に再レビューを依頼 -11. 対応完了したスレッドを **Resolve Conversation** にする -12. **本文の決めたことの節を設計文書に揃える**（**コミットの有無によらず**実行する。後述） -13. **戻り値ファイルを書き出す**（後述） +6. コミットする。**送らない** +7. **本文の決めたことの節を設計文書に揃える**（**コミットの有無によらず**実行する。後述） +8. **戻り値ファイルを書き出す**（後述）。対応したスレッド・見送り・却下をそれぞれの配列へ + 入れる。返信・決着・まとめはこの配列から組み立てられる +9. 単独で呼んだときだけ、「起動モード」の 1 行を実行して送信と投稿を終える ### 本文の決めたことの節を揃える @@ -278,33 +289,30 @@ review 指摘と CI エラーは**同じ PR で一緒に修正**する。同じ ## 返信と Resolve -### 返信の書き分け +**返信・決着・まとめは戻り値ファイルから組み立てる。** 担当が投稿の呼び出しを書かない。 +組み立てと送信は共通層（`lib/result_posts.py`）が 1 か所で持ち、`/ndf:cross-review` から +呼んだときも単独で呼んだときも同じ実装を通る。 -| 状況 | 返信の型 | -|---|---| -| 修正した | `対応しました — <ファイル>:<行> で〇〇 (commit )` | -| 別 PR で対応 | `別 PR で対応予定です。PR 説明の「やらないこと」に記載のとおり、<理由>` | -| deferred | `[deferred / nit] 後続 PR で対応予定` | -| rejected | `bot 指摘は誤読です — 理由: ...` | -| 対応不要 | `確認しました。<対応不要と判断した理由>` | -| 範囲外 | `範囲外と判断し、#<番号> として残しました` — `/ndf:out-of-scope` で起票してから返信する。起票先のリポジトリもその Skill が決める（flaky テスト・CI の失敗は例外で、この PR で直す） | +| 戻り値ファイルの配列 | 送られるもの | +| --- | --- | +| `resolved_threads` | 指摘への「対応しました（<コミット>）」の返信と、スレッドの決着 | +| `deferred` | 見送りの理由（`reason_for_deferral`）の返信。決着しない | +| `rejected` | 採らない理由（`reason_for_rejection`）の返信。決着しない | +| （すべて） | 対応件数・決着・見送り・却下・CI を並べた Pull Request のまとめ | -```bash -# 特定のコメントに返信（in_reply_to にコメント ID を指定） -gh api repos/{owner}/{repo}/pulls/{pr_number}/comments \ - -f body="対応しました。" -F in_reply_to={comment_id} -``` +- 返信の宛先は各要素の `comment_id`、決着の宛先は `thread_id`（`PRRT_...`）である。 + `thread_id` はレビューコメントの `node_id`（`PRRC_...`）ではない。下の query の + `nodes[].id` から取る（次の節の query） +- 同じ返信・同じまとめを 2 度送っても増えない（本文の先頭 80 文字で先客を照合する）。 + すでに決着したスレッドをもう一度決着させても失敗にならない -### Resolve Conversation +### 対応の対象は未解決の指摘を数え直して決める（必須） -**修正済みのスレッドのみ** Resolve する。`deferred` / `rejected` は次ラウンドで再評価する -ため Resolve しない。 +**投稿数を対象の数として使わない。** レビュー結果の `comments_count` は +そのラウンドで新しく投稿された件数であり、PR 上に残っている未解決の指摘の数ではない。 +前のラウンドの分や、中断の前に投稿された分がこの数の外にある。 -`resolveReviewThread` が要求するのは **review thread** の ID（`PRRT_...`）であり、 -レビューコメントの `node_id`（`PRRT_` ではなく `PRRC_...`）ではない。 -`repos/{owner}/{repo}/pulls/comments/` から引ける `node_id` はコメント側の ID -なので **Resolve には使えない**。必ず下記 query の `nodes[].id` を使い、 -`comments.nodes[].databaseId`（返信に使ったコメント ID）または本文と突き合わせて特定する。 +スレッドの一覧は次の query で読む（読み取りだけで、書き込みはしない）。 ```bash # スレッド一覧を thread ID (PRRT_...) 付きで取得 @@ -323,21 +331,9 @@ gh api graphql -f query=' }' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {thread_id: .id, path, line, comment_id: .comments.nodes[0].databaseId}' - -# 上で得た thread_id（PRRT_...）を THREAD_ID に入れて Resolve -gh api graphql -f query=' - mutation($id: ID!) { - resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } - }' -f id="$THREAD_ID" ``` -### 対応の対象は未解決の指摘を数え直して決める（必須） - -**投稿数を対象の数として使わない。** レビュー結果の `comments_count` は -そのラウンドで新しく投稿された件数であり、PR 上に残っている未解決の指摘の数ではない。 -前のラウンドの分や、中断の前に投稿された分がこの数の外にある。 - -上の query を `isResolved == false` で絞った結果が対象の全量である。 +この query を `isResolved == false` で絞った結果が対象の全量である。 `/ndf:cross-review` から呼ばれた場合は、次のコマンドでも同じ数を取れる（引数は state.json の キー、つまり最初に `init` した PR 番号を渡す。対象の PR は state.json 側で解決される）。 @@ -351,28 +347,15 @@ eval "$UNRESOLVED_VARS" `eval` 自身の終了コードが 0 になり、スクリプトの `exit 1` が消える。未解決の件数を 取得できていないのに 0 件と読んで、対象が無いものとして先へ進むことになる。 -返信と Resolve を終えたら、**同じ query をもう一度実行して残数を確認する**。 +送信と投稿を終えたら、**同じ query をもう一度実行して残数を確認する**。 deferred / rejected として意図的に残したもの以外が残っていれば、対応が漏れている。 ### PR レベル Summary コメント（必須） インラインへの返信と Resolve **だけでは不十分**。PR ページの Conversation タブに -まとめが出ないと、レビュアー視点で見落とされる。 - -```bash -gh pr comment --body "$(cat <<'EOMD' -## 🔧 /ndf:fix サマリ - -対応件数: critical=X / major=Y / minor=Z (合計 N 件) -deferred: D 件 / rejected: R 件 -commit: -CI: SUCCESS | FAILURE | NONE - -### 詳細 -- 各 thread の対応概要（行リンク付き） -EOMD -)" -``` +まとめが出ないと、レビュアー視点で見落とされる。**まとめは戻り値ファイルから組み立てて +送られる**（上の表の最後の行）。先頭行にラウンドとコミットが入るため、同じラウンドの +まとめは 1 件だけになる。 ## 戻り値フォーマット（必須） @@ -401,11 +384,12 @@ EOMD {"comment_id": 3222849090, "path": "scripts/state.py", "line": 120, "severity": "minor", "summary": "heredoc を <<'JSON' にせよ", "reason_for_rejection": "$SHA を意図的に展開する必要があり、クオート化すると逆に壊れる"} - ], - "summary_comment_url": "https://github.com/.../pull/67#issuecomment-..." + ] } ``` +- **まとめの参照（`summary_comment_url`）は書かない。** 投稿する側が、まとめの投稿の応答から + 記録へ書く - `resolved_threads` / `deferred` / `rejected` は **必ず配列**で返す（件数の int は誤り）。 該当が無ければ空配列 - **`rejected` の各要素は `path` / `line` / `severity` を持つ。** 却下した論点が次のラウンドで diff --git a/scripts/tests/test_push_fallback_docs.py b/scripts/tests/test_push_fallback_docs.py index a6835002..b5dc1053 100644 --- a/scripts/tests/test_push_fallback_docs.py +++ b/scripts/tests/test_push_fallback_docs.py @@ -19,12 +19,15 @@ REPO_ROOT = Path(__file__).resolve().parents[2] LIB = REPO_ROOT / "plugins/ndf/scripts/lib/git-credential.sh" -# 退避を案内する手順書。`cross-refactoring` は実装が退避するため、案内の形が違う。 +# 退避を案内する手順書。`cross-refactoring` と `fix` は実装が退避するため、案内の形が +# 違う（`fix` の送信は共通層の `result_posts.py` が行う、#730）。 DOCS_WITH_COMMAND = ( "plugins/ndf/skills/pr/SKILL.md", +) +DOCS_WITH_REFERENCE = ( + "plugins/ndf/skills/cross-refactoring/SKILL.md", "plugins/ndf/skills/fix/SKILL.md", ) -DOCS_WITH_REFERENCE = ("plugins/ndf/skills/cross-refactoring/SKILL.md",) def fallback_args() -> list[str]: