fix: セッション自身が作った PR の状態を番号で追跡する - #120
Conversation
一覧の PR セルに `#109` とだけ出て、マージ済み/CI の状態(グリフ)も 「確認中(⋯)」「確認できず(?)」も**一生表示されない**ケースがあった。 原因は問い合わせがブランチ名だけを鍵にしていたこと。セッションが自分で `gh pr create` した PR(`extraPrs`)の head は使い捨ての `feat/…` で、 worktree に checkout されていないことが多い。すると `gh pr view <branch>` は毎回「PR なし」と答え、`absent`(= 権威ある答え)として扱われるので `prStatus` は永久に埋まらず、`answered` 済みなので読み込み中の印も二度と 出ない。番号は `extraPrs` 由来で描かれるため、番号だけが裸で並び続ける。 - `lookupPr` に `PrLookupOptions.knownPr` を追加し、ブランチ候補が全滅したら `gh pr view <番号>` で聞く(`gh pr view` は番号も受ける)。番号は**最後**に 試す — セッションブランチに新しく出た PR を古い番号で上書きしないため。 - 渡す番号は `primaryPr`(= `pr ?? extraPrs.at(-1)`)。解決できた PR は `pr` に昇格し、reducer が `extraPrs` から畳むのでグリフ・`+n` が整合する。 - バッチ(`gh pr list`)側も、既知 PR が突き合わせに失敗したら番号で確認する。 従来は「ページが truncated のときだけ」だったので、head ブランチが見えない PR は永久に `absent` になっていた(`truncated` の判定自体が不要になった)。 - ready 化は `gh pr ready <番号>`。解決した PR がセッションブランチのものとは 限らないので、`state.branch` で ready にすると失敗するか別の PR を触る。 - 番号が分かっていても状態が未取得/取得失敗なら、グリフの位置に `⋯` / `?` を 出す(`⋯ #12` / `? #12`)。裸の番号は「状態が無い=健全」と区別が付かない。 併せて、番号だけ分かっている行(復元直後)も初回ポーリング前は `loading`。
takecchi
left a comment
There was a problem hiding this comment.
1件、修正が必要な問題があります。
[High] 別リポジトリで作成した PR を、現在のリポジトリの同番号 PR と取り違えます。
extraPrs の検出は https://<host>/<owner>/<repo>/pull/<number> を受理しており、pr-detect.ts のコメントでも GitHub Enterprise を含めホストや owner/repo を固定しない設計です。そのため、セッションが gh pr create -R owner/other などで別リポジトリに作った PR も正しく PrRef.url として保存されます。
しかし今回の knownPrOf() / PrLookupOptions.knownPr は URL を捨てて番号だけを渡し、lookupPr() は worktree の cwd で gh pr view <番号> を実行します。PR番号はリポジトリごとの番号なので、例えば保存された参照が https://github.com/acme/other/pull/42 でも、現在の codiva リポジトリに #42 が存在すればそちらを found として採用し、元の URL・状態を無関係な PR で上書きします。さらにその PR が draft かつ checks passing なら、applyResult() が gh pr ready 42 を同じ cwd で実行し、無関係な現在リポジトリの #42 を ready 化します。現在側に同番号が無い場合も、別リポジトリの既知 PR を absent として消してしまいます。
knownPr を番号ではなく PrRef(少なくとも repo identity を含む値)として渡し、フォールバックの gh pr view と auto-ready の両方を完全な pr.url で実行するのが安全です。次の回帰ケースも必要です。
- worktree の repo A に #42 があり、
extraPrsは repo B の/pull/42を指す - repo B の URLを問い合わせ、repo A の #42を採用/ready化しない
- repo A に #42がなくても repo B の参照を
absentとして消さない
確認結果: 変更対象の utils/pr.spec.ts、core/pr-coordinator.spec.ts、ui/pr-cell.spec.tsx は計110件成功し、GitHub CIも成功しています。
レビュー指摘(別リポジトリの PR を取り違える)の修正。 `extraPrs` は `https://<host>/<owner>/<repo>/pull/<n>` を受理するので、 セッションが `gh pr create -R owner/other` で**別リポジトリ**に作った PR も 保存される。ところが `knownPr` を番号で渡していたため、worktree の cwd で `gh pr view 42` を実行すると**現在リポジトリの #42** が返る: - 無関係な PR の状態・URL で `pr` を上書きする - それが draft かつ checks passing なら `gh pr ready 42` で無関係な PR を ready 化する - 現在リポジトリに同番号が無ければ、既知 PR を `absent` として消す `PrLookupOptions.knownPr` / `PrLookupTarget.knownPr` を `number` から `PrRef` に変え、フォールバックの `gh pr view` と auto-ready の両方を **PR の URL** で実行する(`gh` は URL でリポジトリごと解決する)。 回帰テストは utils / core の両方に追加: - 単体: 既知 PR が別リポジトリの `/pull/42` でも、その URL で問い合わせ、 **番号(`42`)では一度も聞かない** - バッチ: 現在リポジトリの `pr list` に同番号 #42 の行があっても、 cross-repo の既知 PR はその URL で確認し、行に引きずられない - coordinator: `markReady` が解決した PR の URL で呼ばれる
|
ご指摘のとおりでした。番号ではなく PR の URL で問い合わせるように直しました(bae0b1b)。
変更
追加した回帰テスト
確認lint / typecheck / test(2633 passed)/ build 済み。 |
takecchi
left a comment
There was a problem hiding this comment.
再レビューで、前回指摘した別リポジトリの同番号PR取り違えは修正されていることを確認しました。PrRef全体をlookupへ渡し、問い合わせとauto-readyの双方でURLを使う実装になっており、cross-repoの回帰テストも追加されています。変更対象を含む既存テスト165件は成功しました。
そのうえで、別の修正が必要な問題が1件あります。
[Medium] URLで確認した extraPr が本当に absent だった場合、今回解消するはずの裸のPR番号表示が永久に残ります。
extraPrs だけに #109 がある状態で、ブランチ候補も既知URLも absent を返すと、PrCoordinator.applyResult() は session.setPr(undefined) を呼びます。しかし pr reducer は event.pr が undefined のとき withoutPrRef(state.extraPrs, ref) に ref = undefined を渡すため、extraPrs は削除されません。その結果:
primaryPr(state)は引き続きextraPrs.at(-1)の #109 を返すprStatusとprLookupはどちらもundefinedになるansweredに入るので、以降markLooking()はloadingを付けないPrCellはグリフなしの裸の#109を描画する
つまり、PRが削除された/URLが権威的に存在しないケースでは、PR本文で説明されている元の症状(状態も ⋯ / ? も付かない裸の番号)がそのまま残ります。再ポーリングしても同じ extraPr を既知URLとして問い合わせ続けるだけです。
PR head に extraPrs: [gone]、lookup結果 absent の回帰ケースを追加し、refresh後に参照が除去されることを期待すると、配列がそのまま残って失敗することを確認しました(同ファイルの既存49件は成功)。
既知URLまで absent と確認できた場合は、その PrRef を extraPrs から除去できるイベント/Session APIを用意するか、少なくとも裸の番号にならない明示的な状態を保持する必要があります。ブランチにPRが無いだけの通常の absent と、既知URLも存在しない absent を区別できるようにすると扱いやすいと思います。
GitHub CIは最新commitで成功しています。
レビュー指摘([Medium] URL で確認した extraPr が本当に absent だと裸の PR 番号が永久に残る)の修正。 `extraPrs` だけに #109 があり、ブランチ候補も既知 URL も `absent` を返した 場合、`applyResult` は `setPr(undefined)` を呼ぶが `pr` reducer の `withoutPrRef(state.extraPrs, ref)` は `ref === undefined` なので何も落ちない。 結果 `primaryPr` は #109 を返し続け、`prStatus`・`prLookup` はどちらも undefined、`answered` 済みで `loading` も付かない = **このPRで直したはずの 「グリフも ⋯/? も無い裸の番号」に戻る**。再ポーリングしても同じ URL を 聞き続けるだけ。 - `CodivaEvent` に `pr_gone`(`PrRef`)を追加。reducer は `extraPrs` と `pr`/`prStatus` の**両方**から参照を落とす(`primaryPr` はどちらでも読むので 片方だけでは裸の番号が残る)。持っていない参照なら同一参照を返す。 - `Session.dropPr(ref)` / `SessionHandle.dropPr` を追加。 - `PrCoordinator.applyResult` は「`absent` かつ `knownPr` を渡していた」ときだけ `dropPr` する。`absent` は全候補が答えたときにしか返らないので、これは GitHub が「その PR は存在しない」と答えた場合に限られる。`unavailable` (レート制限・オフライン)では流さない = 参照は保持したまま `?` を出す。 テスト: - reducer: extraPrs から落とす / 唯一なら undefined に / 追跡中の pr と一致 したら pr+prStatus も消す / 持っていない参照は no-op(同一参照) - coordinator: 自作 PR が absent なら `dropPr` → `setPr(none)` の順で消える / `unavailable` では消さない / 既知 PR が無ければ `dropPr` を呼ばない - app: 実 reducer 経由の統合ケース(一覧から `#109` が消える)
|
ご指摘のとおりで、このPRが直したはずの症状にそのまま戻る穴でした。 原因の確認
修正「ブランチに PR が無いだけの
追加テスト
確認lint / typecheck / test(2641 passed)/ build 済み。 |
takecchi
left a comment
There was a problem hiding this comment.
再レビューで、前回指摘した「absentになった extraPr が裸の番号として永久に残る」問題は、pr_gone で pr / extraPrs の両方から参照を除去する形に修正されていることを確認しました。unavailable では削除しないテストと、実UIから番号が消える統合テストも追加されています。
そのうえで、複数PRの切り替わりに1件問題があります。
[Medium] 消えた追跡PRの次に extraPr が残っていると、新しい代表が次回ポーリングまで裸の番号になります。
例えば pr = #108、extraPrs = [#109] の状態で、#108のURLまで含めたlookupが absent になると、今回の処理は dropPr(#108) → setPr(undefined) と進みます。これにより #109 が primaryPr() の新しい代表へ即座に繰り上がりますが、prLookup は undefined のままです。同時に answered と lastFetched は今回の#108への回答で更新されるため、markLooking() はもう走らず、#109をその場で問い合わせ直すこともありません。
結果として、今回避けたい「状態も ⋯ / ? もない裸の #109」が次の更新まで表示されます。次回は state.pr が無い扱いなので、完了セッションでは最大60秒、稼働中では最大180秒待ちます。消失した参照が複数続く場合は、この待ちが1件ごとに繰り返されます。
PR head に次の回帰ケースを追加して再現しました。
pr: #108(statusあり)とextraPrs: [#109]を持たせる- lookupを
absentにする - refresh後、#109が残るなら
prLookup === "loading"を期待する
実際は prLookup === undefined となり、追加テストが失敗しました(同ファイルの既存52件は成功)。
参照を落とした後にも別の primaryPr が残る場合は、その場で次の既知URLを解決するか、少なくとも prLookup: "loading" にして lastFetched / answered を新代表向けにリセットする必要があります。新代表が未取得なのに、旧代表への回答をそのままキャッシュ済みとして扱わないことが重要です。
確認結果:
- 変更対象を含む既存5ファイルのテストと
npm run typecheck: 成功 - GitHub CI check: 成功
レビュー指摘([Medium] 消えた追跡 PR の次に extraPr が残ると、新しい代表が 次回ポーリングまで裸の番号になる)の修正。 `pr = #108` / `extraPrs = [#109]` で #108 が absent だと `dropPr(#108)` → `setPr(undefined)` で #109 が即座に `primaryPr` へ繰り上がるが、`prLookup` は undefined のままで、`answered` / `lastFetched` は #108 への回答で更新済み。 その結果 `markLooking()` も走らず、#109 は次の陳腐化(完了 60 秒 / 稼働中 180 秒)まで「状態も ⋯/? も無い裸の番号」で表示される。参照が複数続けば 1 件ごとに繰り返す。 `applyResult` は参照を落としたあとに `primaryPr` がまだ残っていれば、 その行を回答済みにせず(`answered` / `lastFetched` を更新せず) `prLookup: 'loading'` を立てて抜ける。今回の答えは落とした PR のもので 新代表については何も言っていないため、キャッシュ扱いにしてはいけない。 `lastFetched` が無いので次の tick(20 秒)で新代表を問い合わせ、それまでは `⋯` が出る。 テスト: 指摘のケース(`['dropPr:#108','setPr:none','prLookup:loading']` → 続く refresh で #109 を knownPr として問い合わせ、status が付いて loading が 消える)を追加。
|
ご指摘のとおりでした。修正しました(2475a55)。 原因
→ 新代表 #109 が「状態も 修正
if (!pr && known && primaryPr(session.getState())) {
session.setPrLookup('loading');
return undefined; // キャッシュに刻まない
}
this.answered.add(id);
this.lastFetched.set(id, this.now());今回の答えは落とした PR のもので新代表については何も言っていないので、キャッシュ扱いにしないのが要点です。 追加テスト
確認lint / typecheck / test(2642 passed)/ build 済み。 |
takecchi
left a comment
There was a problem hiding this comment.
再レビュー完了です。追加の修正指摘はありません。
前回指摘した「消えた追跡PRの次に extraPr が残ると、新代表が次回ポーリングまで裸の番号になる」問題は、次の処理で解消されていることを確認しました。
- 消えた旧代表を
dropPr/setPr(undefined)で除去 - 次の
primaryPrが残る場合は、旧代表への回答をanswered/lastFetchedに記録しない - 新代表には
prLookup: "loading"を設定して裸の番号を防ぐ - 次のrefreshで新代表のURLを
knownPrとして問い合わせ、取得後にloadingを解除
この構造なら、消失した参照が複数連続する場合も1件ずつloadingを維持しながら次へ進めます。参照が尽きた場合は通常のanswered状態になり、unavailableの場合は既知参照を削除しない点も維持されています。
確認結果:
- PR用worktreeで変更対象を含む関連5ファイルのテスト: 成功
npm run typecheck: 成功- GitHub CI check: 成功
これまで指摘したcross-repoの同番号取り違え、存在しない extraPr の残留、代表繰り上がり時の裸番号について、いずれも回帰テスト付きで修正されています。
症状(報告)
原因
PR の問い合わせがブランチ名だけを鍵にしていたため。
セッションが自分で
gh pr createした PR(extraPrs)の head は使い捨てのfeat/…などで、worktree に checkout されていないことが多い。するとgh pr view <branch>は毎回「PR なし」と答え、それはabsent(権威ある答え)として扱われるのでprStatusが永久に埋まらない → グリフ(⑂マージ済み /✓/●/✗)が付かない⋯(確認中)も二度と出ないextraPrsから描かれるので、裸の#109だけが残る実データで確認(報告者の
state.json):prextraPrs[109]codiva/agent-codex-model-claude-codex-claudecodiva/agent-codex-model-catalog-display、MERGED)#109は実際にはマージ済みだが、その head ブランチはどの worktree にも無いので、ブランチ名で聞ける限り永久に解決できなかった。修正
PrLookupOptions.knownPrを追加し、ブランチ候補(HEAD → 記録ブランチ)が全滅したらgh pr view <番号>で問い合わせる(gh pr viewは番号も受ける)。番号は最後に試す — セッションブランチに新しく出た PR を古い番号で上書きしないため。primaryPr(=pr ?? extraPrs.at(-1))。解決できた PR はprに昇格し、reducer がextraPrsから畳むのでグリフと+nが整合する。gh pr list)側も同じ: 既知 PR が突き合わせに失敗したら番号で確認する。従来は「ページが truncated のときだけ」だったので、head ブランチが見えない PR は永久にabsentだった(truncatedの判定自体が不要になった)。gh pr ready <番号>)。解決した PR がセッションブランチのものとは限らないので、state.branchで ready にすると失敗するか別の PR を触る。⋯/?を出す(⋯ #12/? #12)。裸の番号は「状態が無い=健全」と区別が付かない。併せて、番号だけ分かっている行(復元直後)も初回ポーリング前はloadingにする。API コストは増やしていない: 追加の
gh pr viewが走るのは「PR があると分かっていて、どのブランチでも解決できなかった」行だけで、その行の再取得間隔は 60〜180 秒。マージ済みになれば以後問い合わせない。テスト計画
npm run lintnpm run typechecknpm test(2630 passed。utils/pr.spec.ts/core/pr-coordinator.spec.tsに番号フォールバックのケース、新規src/ui/pr-cell.spec.tsxに⋯ #12/? #12の表示ケースを追加)npm run build補足(このPRには含めない)
調査中に見つけた別の弱点:
utils/pr.tsのgh呼び出しにタイムアウトが無く、PrCoordinator.refreshPrsの多重実行ガード(refreshing)にも期限が無い。スリープ復帰などでghが 1 回ハングすると、以後 PR 情報がプロセス寿命のあいだ一切更新されなくなる(新しいセッションは⋯すら出ない)。別 PR で直すのが良さそうです。