fix(post-merge-feedback): task labeling 規約導入で run dir 検索を決定論化#78
Merged
Conversation
PR #77 の merge dogfood で発覚した `copy_feedback_report` の bug を修正する。 takt の run dir 命名は `<ts>-<sanitized-task-label>` 形式で workflow 名そのものは 含まないため、`ends_with("-<workflow>")` での suffix match が context suffix (例: `-for-77`) 付きの dir に対して fail していた。 対策: 「task label は workflow 名を必ず prefix として含む」という命名規約を ADR-030 に追記し、Rust 側のマッチングを `name.contains("-<workflow>")` の 1 ルールに統一。context suffix の有無に関わらず一律にマッチする。 主な変更: - src/cli-merge-pipeline/src/feedback.rs: find_latest_run_dir を contains match に変更、TAKT_TASK_PREFIX を workflow 名と sanitization 後一致する形に 修正、新規テスト 5 件追加 - src/cli-pr-monitor/src/config.rs + pr-monitor-config.toml + templates/pr-monitor-config.toml: post-pr-review の task を "analyze PR review comments" → "post-pr-review" に揃え、latent bug を予防 - docs/adr/adr-030-deterministic-post-merge-feedback.md: §task labeling convention セクション追加。規約・制約 (workflow 名同士の部分文字列禁止)・ 採用根拠を明記 Phase B 内の bugfix として完結し、Phase C には bug を持ち越さない。
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughADRでタスクラベルとtakt実行ディレクトリ命名の一貫性を定め、Rust実装のランディレクトリ選択ロジックをサフィックス照合からワークフロー名の包含照合（ Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/adr-030-deterministic-post-merge-feedback.md`:
- Line 181: The ADR example is inconsistent with the implementation: the logic
in find_latest_run_dir using name.contains(&format!("-{}", workflow)) will match
internal substrings (e.g., "-merge" inside "post-merge-feedback"), so update the
ADR example to mark "merge" vs "post-merge-feedback" as invalid (NG) rather than
OK; reference the implementation detail (find_latest_run_dir and
name.contains(&format!("-{}", workflow))) in the ADR text and adjust the
wording/constraints so future readers see that any workflow name that is a
substring of another (including mid-string matches) is disallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96047c5b-ac59-4514-a199-24d5258953d8
📒 Files selected for processing (5)
docs/adr/adr-030-deterministic-post-merge-feedback.mdpr-monitor-config.tomlsrc/cli-merge-pipeline/src/feedback.rssrc/cli-pr-monitor/src/config.rstemplates/pr-monitor-config.toml
CodeRabbit (PR #78) が指摘した不整合を修正。 旧記述: "merge と post-merge-feedback は OK" は誤り。 - find_latest_run_dir は name.contains("-{workflow}") で照合 - workflow=merge の needle "-merge" は "<ts>-post-merge-feedback-..." に 内部出現するため誤マッチが起きる - したがって merge も post-merge と同様 NG 修正内容: - 「部分文字列関係」の定義を実装メソッド (name.contains) と紐付けて明記 - NG 例を 2 件 (merge, post-merge) と OK 例 (build) を併記 - 実装ファイル feedback.rs へのリンクを追加
6 tasks
aloekun
added a commit
that referenced
this pull request
Apr 26, 2026
) Phase B dogfood で発見されたタイムアウト経路の残存不具合と、ボトルネック分析で 判明した sequential 構造を併せて修正する。 ## 主な変更 ### 1. workflow の並列化 (.takt/workflows/post-merge-feedback.yaml) 3 つの analyze facet (analyze-pr / analyze-session / analyze-prepush-reports) は 独立した情報源を扱うため parallel block で並列実行する。 PR #78 計測 (sequential 12m13s) → parallel 想定 ~7m30s に短縮。 ### 2. TAKT_TIMEOUT_SECS: 600s → 1200s PR #77 (14m21s)、#78 (12m13s) 観測実績を踏まえた暫定値。並列化後の想定 7m30s に対し 2x の安全係数。docstring に「band-aid」と明記し、本質解は workflow 構造で 時間を抑えることだと残した。 ### 3. caller-side reconciliation (run() 末尾) Windows の child.kill() が takt の descendants を殺せず、Rust が timeout で kill した後も takt が orphan として走り続けて report を完成させるケースを観測した (PR #78 で kill 後 2m13s で feedback-report.md 完成)。 run_takt_workflow の戻り値に関わらず copy_feedback_report を必ず試行し、report が 存在すれば success 扱いとする。既存の .failed marker は cleanup_failed_marker で 除去する。 ### 4. 並行起動 guard (CONCURRENT_RUN_GUARD_SECS = 1500s) cross-invocation context overwrite race を発生源で予防。 .takt/post-merge-feedback-context.json が 1500s 以内に書かれていれば、orphan takt が走行中の可能性が高いとみなして新規実行を refuse する。 ### 5. テスト追加 (5 件) concurrent_run_guard / context_age_secs の振る舞いを検証。 ### 6. ADR-030 更新 - レイテンシモデル (parallel 想定 = max(analyze) + aggregate) - Reconciliation 設計の根拠 - 並行起動 guard の意図と Phase B 外切り出し方針 ## 残存リスク (Phase B 外) - Windows の job object でプロセスツリー全体を kill する根本対策 - 完全な per-invocation isolation (takt context dir 連携)
9 tasks
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
PR #77 の merge dogfood で発覚した
copy_feedback_reportのバグを Phase B 内で完結修正する。Bug: takt の run dir 命名
<ts>-<sanitized-task-label>に対し、Rust 側がends_with("-<workflow>")で suffix match していた。task に context (PR 番号等) が含まれると dir suffix が-post-merge-feedback-for-77のようになり、-post-merge-feedbackends_with で fail。Fix: 命名規約を導入することで曖昧な検索を排除する。task label は workflow 名を必ず prefix として含む
"<workflow-name> [<context>]"形式とし、Rust 側のマッチングをname.contains(&format!("-{}", workflow))の 1 ルールに統一。context suffix の有無に関わらず一律にマッチする。変更内容
コード
find_latest_run_dir: 引数をsuffix→workflowに、内部実装をends_with→contains(&format!("-{}", workflow))に変更TAKT_TASK_PREFIX:"post-merge feedback for #"→"post-merge-feedback for #"(空白を除去し sanitization 後完全一致に)設定 (post-pr-review の latent bug 予防)
task = "analyze PR review comments"→"post-pr-review"ドキュメント
mergeとpost-merge-feedbackは OK、post-mergeとpost-merge-feedbackは NG)設計判断
なぜ「最新 run dir を無条件で拾う」(Option C) ではなく命名規約 (Option A) か
pre-push-review) との後方互換pre-push-reviewで偶然一致するため変更不要バグの教訓 (PR #77 dogfood で抽出された Tier 1 #3 とも整合)
「ADR 移行時、旧機構の文言・参照・命名仮定が log メッセージや match ロジックに残留する」というパターンが ADR-015/018/030 と繰り返し発生している。本 PR では log 文言ではなくマッチング規約の側で問題を解決する (PR #77 で aggregate-feedback が提案した Tier 3 #1「ADR 移行チェックリストに grep ステップ追加」とも独立補完)。
Test plan
find_latest_prepush_picks_lexicographic_max(workflow 名のみ形式) が新実装でも passfind_latest_run_dir_matches_workflow_with_context_suffix(PR 番号付き形式) が passfind_latest_run_dir_picks_lex_max_across_mixed_forms(両形式混在) が passdogfood 検証は本 PR マージ後の post-merge-feedback workflow が
.claude/feedback-reports/<pr>.mdを正しく生成することで完了する (Phase B 内で bug 発見 → 修正 → 検証のサイクルを閉じる)。References
Summary by CodeRabbit
Documentation
Bug Fixes
Chores
Tests