fix(feedback): 進行中ガードを run の状態判定へ移し run を PR で束縛する (順位 398-400/388) - #388
Merged
Conversation
順位 398: ガードは context.json の mtime だけを見ており run の完了を見ていなかった。 完了済みでも 25 分間は次の feedback を起動できず、連続マージで確実に踏んでいた。 順位 399: 同じガードが --feedback-only も塞ぎ、復旧専用コマンドが復旧に使えなかった。 順位 400: marker が stale context を読む危険な手順を第一手段として案内していた。 順位 388: report 不在判定が別 run を見ており、成功した feedback を failed marker にした。 - run_registry を新設し、takt の meta.json (task / status / reportDirectory) から post-merge-feedback の run を PR 番号で束縛して解決する - ガードの判定根拠を mtime から status: running の実在へ移す (順位 398/399) - copy_feedback_report が lex-latest ではなく対象 PR の run を選ぶ (順位 398/388) - report 不在判定の前に短い再試行を入れる (順位 388) - marker の復旧手順を --feedback-only 第一手段へ、takt 直接起動は最終手段へ (順位 400) - ADR-030 へ判定根拠の変更・PR 束縛・復旧手順を記録する status が読めない run は進行中とみなさない。壊れた meta.json 1 つで後続の feedback が 永久に起動できなくなる方が害が大きく、放置された run は orphan reaper が failed へ落とす。 順位 398 / 399 / 400 / 388 / ADR-030
📝 WalkthroughWalkthroughpost-merge feedback の並行実行判定を run metadata の状態ベースへ変更した。PR番号に一致する最新runからレポートを取得し、遅延出現を再試行する。failed marker の復旧案内と関連ドキュメントを更新した。 Changespost-merge feedback 実行管理
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MergePipeline
participant RunRegistry
participant RunMetadata
participant ReportDirectory
MergePipeline->>RunRegistry: 対象PRのrunを検索
RunRegistry->>RunMetadata: meta.jsonを読み取る
RunMetadata-->>RunRegistry: task、status、reportDirectory
RunRegistry-->>MergePipeline: 最新runとレポートパス
MergePipeline->>ReportDirectory: レポートを最大5回確認
ReportDirectory-->>MergePipeline: レポート内容
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし Applicable Findings (Medium 以下)該当なし Filtered (not applicable)該当なし 軽量サマリー (レビュー指摘が無いため)
次のアクション
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/adr/adr-030-deterministic-post-merge-feedback.md (1)
287-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value数値の直書きは同 ADR 256 行の方針と衝突します。
256 行は「実数値は
cli-merge-pipeline::feedback::TAKT_TIMEOUT_SECS/ORPHAN_THRESHOLD_SECSを参照のこと (本 ADR で数値固定するとコード変更時に drift する)」と定めています。287 行は「5 回 × 200ms」を直書きします。実装側の定数はREPORT_WAIT_ATTEMPTS/REPORT_WAIT_INTERVAL_MSです。定数名を参照する形にすると、実装変更時に本文が古くならなくなります。♻️ 定数名を参照する案
-あわせて、report 不在判定の前に短い再試行 (5 回 × 200ms) を入れた。#367 で「takt 成功扱いだが report 不在」の marker が出た後に run dir を見ると実体が**存在していた** (順位 388)。PR 束縛で「別 run を見ていた」クラスは消えるが、takt exit 直後の flush 待ちは残るため最小限だけ待つ。完了済み run が後から report を生やすことは無いので長く待つ意味は無い。 +あわせて、report 不在判定の前に短い再試行を入れた (回数と間隔は `cli-merge-pipeline::feedback::takt::REPORT_WAIT_ATTEMPTS` / `REPORT_WAIT_INTERVAL_MS` を参照)。#367 で「takt 成功扱いだが report 不在」の marker が出た後に run dir を見ると実体が**存在していた** (順位 388)。PR 束縛で「別 run を見ていた」クラスは消えるが、takt exit 直後の flush 待ちは残るため最小限だけ待つ。完了済み run が後から report を生やすことは無いので長く待つ意味は無い。適用したコーディングガイドライン: 「Follow the applicable Architecture Decision Records (ADR-001 through ADR-072)」。本 ADR 自身が定めた数値非固定の方針に合わせます。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/adr-030-deterministic-post-merge-feedback.md` at line 287, Update the retry description in ADR-030 to reference the implementation constants REPORT_WAIT_ATTEMPTS and REPORT_WAIT_INTERVAL_MS instead of hardcoding “5回 × 200ms,” while preserving the stated short retry behavior before determining that the report is absent.Source: Coding guidelines
src/cli-merge-pipeline/src/feedback/markers.rs (1)
501-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
削除の部分一致は将来の文言追加で誤検知します。このテストは本文全体から「削除」という 2 文字を禁止します。意図は「context.json の手動削除を案内しない」ことです。将来、無関係な文脈 (例: marker の後始末の説明) で「削除」を含む文言を足すと、このテストが正当な変更を落とします。判定対象を context.json に絞ると意図が固定されます。
♻️ 判定を context.json へ絞る案
fn does_not_tell_the_reader_to_delete_the_context_file() { let body = marker_body(382); + let context = crate::feedback::CONTEXT_PATH; assert!( - !body.contains("削除"), + !body + .lines() + .any(|line| line.contains(context) && line.contains("削除")), "context.json の手動削除を案内しないこと: {body}" ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli-merge-pipeline/src/feedback/markers.rs` around lines 501 - 508, Update the test does_not_tell_the_reader_to_delete_the_context_file so it checks for manual deletion guidance specifically associated with context.json, rather than rejecting every occurrence of “削除” in the full marker body. Preserve the test’s intent while allowing unrelated deletion wording elsewhere in the message.src/cli-merge-pipeline/src/feedback/takt.rs (1)
134-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueループ後の
source.is_file()は冗長です。ループの最終試行 (
attempt == REPORT_WAIT_ATTEMPTS - 1) はis_file()をチェックし、その後 sleep しません。よって 144 行の再チェックは直前のチェックとほぼ同時刻に実行されます。返り値は変わりません。sleep の条件分岐を外して構造を単純にできます。♻️ 構造を単純にする案
fn wait_for_report(source: &Path) -> bool { for attempt in 0..REPORT_WAIT_ATTEMPTS { + if attempt > 0 { + std::thread::sleep(Duration::from_millis(REPORT_WAIT_INTERVAL_MS)); + } if source.is_file() { return true; } - if attempt + 1 < REPORT_WAIT_ATTEMPTS { - std::thread::sleep(Duration::from_millis(REPORT_WAIT_INTERVAL_MS)); - } } - source.is_file() + false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli-merge-pipeline/src/feedback/takt.rs` around lines 134 - 145, wait_for_report のループ後にある冗長な source.is_file() 再チェックを削除し、ループ内の最終試行結果をそのまま返す構造に簡略化してください。最終試行では sleep せず、既存の待機回数・間隔と成功時の true を維持してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/adr/adr-030-deterministic-post-merge-feedback.md`:
- Line 287: Update the retry description in ADR-030 to reference the
implementation constants REPORT_WAIT_ATTEMPTS and REPORT_WAIT_INTERVAL_MS
instead of hardcoding “5回 × 200ms,” while preserving the stated short retry
behavior before determining that the report is absent.
In `@src/cli-merge-pipeline/src/feedback/markers.rs`:
- Around line 501-508: Update the test
does_not_tell_the_reader_to_delete_the_context_file so it checks for manual
deletion guidance specifically associated with context.json, rather than
rejecting every occurrence of “削除” in the full marker body. Preserve the test’s
intent while allowing unrelated deletion wording elsewhere in the message.
In `@src/cli-merge-pipeline/src/feedback/takt.rs`:
- Around line 134-145: wait_for_report のループ後にある冗長な source.is_file()
再チェックを削除し、ループ内の最終試行結果をそのまま返す構造に簡略化してください。最終試行では sleep せず、既存の待機回数・間隔と成功時の true
を維持してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8126c10-3f2c-4731-8d3c-0d39ce9615ba
📒 Files selected for processing (9)
docs/adr/adr-030-deterministic-post-merge-feedback.mddocs/harness-improvement-plan.mddocs/todo-summary2.mddocs/todo21.mdsrc/cli-merge-pipeline/src/feedback/context.rssrc/cli-merge-pipeline/src/feedback/markers.rssrc/cli-merge-pipeline/src/feedback/mod.rssrc/cli-merge-pipeline/src/feedback/run_registry.rssrc/cli-merge-pipeline/src/feedback/takt.rs
💤 Files with no reviewable changes (2)
- docs/todo-summary2.md
- docs/todo21.md
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし Applicable Findings (Medium 以下)
Filtered (not applicable)
次のアクション
|
aloekun
added a commit
that referenced
this pull request
Aug 11, 2026
本セッションで実施した WP-18 (2) 運用問題 5 件の対処 (#385/#386/#388/#389) について、 実走観測の記録・計画書の整理・feedback 採否の登録をまとめて行う。 ## 実走観測の記録 - ADR-072 へ定常運用 2 巡目 (PR #387) の観測を追加する。決定 15-17 投入後の 9 項目が設計どおり動いたことと、review-request の成功判定が「反応の有無」で 止まっている (拒否も success になる) ことを事実として記録する - ADR-019 へレート制限の競合が記録の翌日に実地で再現したことを追加する - 順位 386 の観測を 7 回 → 9 回へ更新する。うち 1 件は空コミットではなく 「近い revset を優先する規則」そのものが原因で、本命の対処案だけでは 解決しない可能性がある点を併記する ## 計画書の整理 (ADR-073 新設) - 完了条件の切り方 (残作業を 3 区分に分け、その WP が生んだ問題は完了条件に 含め、WP 外の派生は含めない) を ADR-073 として切り出す - WP-18 節を 71 行 → 35 行へ整理し、完了記録を削除して残作業のみにする - ローカル実行時の jj workspace 注記を ADR-072 へ移す ## post-merge feedback 採否 (順位 414-432) - 採用候補 24 件のうち 6 件は当該 PR 内で実装済みのため対象外とした (実物と照合して確認) - 採用 19 件を系統 A-G + セッション由来へ分類して登録する - SIGPIPE resilience は却下する。レポートが「実測証拠」とした recovery が 実際には発生しておらず (run 1 回・completed・marker の痕跡なし)、提案内容も ADR-030 §L1 で実装済みだった。feedback レポート自身が根拠を誤った初の実例 として記録し、順位 403 の対象へ含めるよう申し送る ## 付随 - todo21.md が 57KB (50KB 閾値超過) のため todo22.md を新設する - todo-summary.md の「現行の追加先」が todo14.md のまま stale だったので直す - cli-docs-lint が検出した preamble の数詞ずれ (23 → 24) を 9 ファイルで更新する ADR-073 / 順位 414-432
aloekun
added a commit
that referenced
this pull request
Aug 11, 2026
本セッションで実施した WP-18 (2) 運用問題 5 件の対処 (#385/#386/#388/#389) について、 実走観測の記録・計画書の整理・feedback 採否の登録をまとめて行う。 ## 実走観測の記録 - ADR-072 へ定常運用 2 巡目 (PR #387) の観測を追加する。決定 15-17 投入後の 9 項目が設計どおり動いたことと、review-request の成功判定が「反応の有無」で 止まっている (拒否も success になる) ことを事実として記録する - ADR-019 へレート制限の競合が記録の翌日に実地で再現したことを追加する - 順位 386 の観測を 7 回 → 9 回へ更新する。うち 1 件は空コミットではなく 「近い revset を優先する規則」そのものが原因で、本命の対処案だけでは 解決しない可能性がある点を併記する ## 計画書の整理 (ADR-073 新設) - 完了条件の切り方 (残作業を 3 区分に分け、その WP が生んだ問題は完了条件に 含め、WP 外の派生は含めない) を ADR-073 として切り出す - WP-18 節を 71 行 → 35 行へ整理し、完了記録を削除して残作業のみにする - ローカル実行時の jj workspace 注記を ADR-072 へ移す ## post-merge feedback 採否 (順位 414-432) - 採用候補 24 件のうち 6 件は当該 PR 内で実装済みのため対象外とした (実物と照合して確認) - 採用 19 件を系統 A-G + セッション由来へ分類して登録する - SIGPIPE resilience は却下する。レポートが「実測証拠」とした recovery が 実際には発生しておらず (run 1 回・completed・marker の痕跡なし)、提案内容も ADR-030 §L1 で実装済みだった。feedback レポート自身が根拠を誤った初の実例 として記録し、順位 403 の対象へ含めるよう申し送る ## 付随 - todo21.md が 57KB (50KB 閾値超過) のため todo22.md を新設する - todo-summary.md の「現行の追加先」が todo14.md のまま stale だったので直す - cli-docs-lint が検出した preamble の数詞ずれ (23 → 24) を 9 ファイルで更新する ADR-073 / 順位 414-432
aloekun
added a commit
that referenced
this pull request
Aug 11, 2026
本セッションで実施した WP-18 (2) 運用問題 5 件の対処 (#385/#386/#388/#389) について、 実走観測の記録・計画書の整理・feedback 採否の登録をまとめて行う。 ## 実走観測の記録 - ADR-072 へ定常運用 2 巡目 (PR #387) の観測を追加する。決定 15-17 投入後の 9 項目が設計どおり動いたことと、review-request の成功判定が「反応の有無」で 止まっている (拒否も success になる) ことを事実として記録する - ADR-019 へレート制限の競合が記録の翌日に実地で再現したことを追加する - 順位 386 の観測を 7 回 → 9 回へ更新する。うち 1 件は空コミットではなく 「近い revset を優先する規則」そのものが原因で、本命の対処案だけでは 解決しない可能性がある点を併記する ## 計画書の整理 (ADR-073 新設) - 完了条件の切り方 (残作業を 3 区分に分け、その WP が生んだ問題は完了条件に 含め、WP 外の派生は含めない) を ADR-073 として切り出す - WP-18 節を 71 行 → 35 行へ整理し、完了記録を削除して残作業のみにする - ローカル実行時の jj workspace 注記を ADR-072 へ移す ## post-merge feedback 採否 (順位 414-432) - 採用候補 24 件のうち 6 件は当該 PR 内で実装済みのため対象外とした (実物と照合して確認) - 採用 19 件を系統 A-G + セッション由来へ分類して登録する - SIGPIPE resilience は却下する。レポートが「実測証拠」とした recovery が 実際には発生しておらず (run 1 回・completed・marker の痕跡なし)、提案内容も ADR-030 §L1 で実装済みだった。feedback レポート自身が根拠を誤った初の実例 として記録し、順位 403 の対象へ含めるよう申し送る ## 付随 - todo21.md が 57KB (50KB 閾値超過) のため todo22.md を新設する - todo-summary.md の「現行の追加先」が todo14.md のまま stale だったので直す - cli-docs-lint が検出した preamble の数詞ずれ (23 → 24) を 9 ファイルで更新する ADR-073 / 順位 414-432
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
context.jsonの mtime ではなく takt run のstatusがrunningかで判定するように変えた（順位 398）--feedback-only <PR>が、手動のファイル削除なしに復旧できるようになった（順位 399）.failedmarker の復旧手順を--feedback-only第一手段に改め、stale context を読む takt 直接起動を「最終手段」へ降格した（順位 400）meta.jsonの task label で PR 番号を照合した run に変えた。あわせて report 不在判定の前に短い再試行を入れた（順位 388）run_registryを新設し、takt のmeta.json（task/status/reportDirectory）から run を解決する経路を 1 か所に集約したContext
Why（順位 398）: 2026-08-10 に #383 をマージした 4 分後に #382 をマージしたところ、#383 の run は既に完了していた（report 生成済み・takt プロセス不在）にもかかわらず #382 の feedback が refuse された。ガードは
context.jsonの mtime が 1500 秒以内かだけを見ており、run が完了したかを一切見ていない。完了済みでも 25 分間は次の feedback を起動できず、連続マージ運用では確実に踏む。Why（順位 399）: 同じガードが復旧経路も塞いでいた。
--feedback-only <PR>は PR 番号を引数で受け context.json に依存しない設計なのに、ガードだけが context の鮮度を見るため、復旧専用コマンドが復旧に使えない。実際の復旧は「進行中の takt が無いことを確認 → context.json を手動削除 → 再実行」でしか通らなかった。Why（順位 400）: marker の復旧手順は takt 直接起動を第一手段に案内していたが、これは context.json を読み直すだけなので、context が別 PR を指していると誤った PR の transcript でレポートを生成する。#382 の marker が出た時点で context は実際に #383 を指していた。「再実行前に
pr_numberの一致を確認」という警告はあったが、読み飛ばせば誤ったレポートが残る。Why（順位 388）: #367 で「takt 成功扱いだが report 不在」の marker が出たが、レポート実体は run dir に存在していた。
find_latest_run_dirの lex-latest は、連続マージ中に自分より新しい別 PR の run dir を掴む。Scope decision:
meta.jsonのtask/statusは orphan reaper (hooks-session-start::reaper) が既に読んでいる。そこへreportDirectoryを足した部分集合を読むstatusが読めない run は進行中とみなさない。壊れた meta.json 1 つで後続の feedback が永久に起動できなくなる方が害が大きく、取りこぼしの実害は「同時に 2 つ走りうる」に留まる。ただしこの取りこぼしは orphan reaper でも回収されない（reaper もパース不能な meta.json を skip する）ため、安全網があるからではなく許容している既知のギャップとして ADR とコード doc の両方に明記したValidation
cargo test -p cli-merge-pipeline: 95 pass（71 → 95、24 件追加）。cargo test --workspace全 crate greencargo clippy --workspace --all-targets: 警告 0 /pnpm lint:docs/ markdownlint: 0 errorverdict=APPROVE、指摘なしcargo fmtは全体実行せず、本 PR で書いたファイルにのみrustfmtを適用した（pipeline.rsの既存差分は未変更であることを差分一覧で確認）レビュー指摘で見つかった実バグ（いずれも実測で検出）
reportDirectoryをrepo_rootへ join する箇所に閉じ込めガードを足した際、追加したテスト自身が 2 件の実バグを検出した。is_relative()/etc/passwdtrue（Windows）has_root()で修正C:temptruePrefixかで修正is_absolute()が prefix と root の両方を要求する仕様のため、片方だけを持つ形が「相対」と判定され、joinが base を置き換える。さらに..\..\windowsは Windows でのみ区切りとして解釈されるため、テストの期待値を OS 別に分離した（Linux で実測して分離）。変異テストで、ガードを外すとテストが落ちることを確認済み。Follow-up（本 PR に含めない）
check_concurrent_run_guardが毎回.takt/runs/*を全走査する（旧実装の mtime 1 回読みに対し O(n)）。現状の run 数では問題にならないが、.takt/runs/のクリーンアップ機構が無いため長期的には増え続ける。別途 todo 登録を検討するReferences
.failedmarker の復旧手順 を追加・更新docs/todo21.md/docs/todo-summary2.mdから削除済）、WP-18 の運用問題（docs/harness-improvement-plan.md）Summary by CodeRabbit
バグ修正
ドキュメント