docs(todo): PR #268/#269 post-merge feedback 採用 4 件を順位 288-291 に登録 - #270
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Changespost-merge feedback 改善タスク
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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)(該当なし — レビュー指摘自体がまだ 0 件) 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/todo13.md`:
- Around line 1061-1067: Update the TODO entry describing pre-push run discovery
to require stable association with the target PR, such as PR number, bookmark,
or commit ID, rather than relying only on the time range. Extend the completion
criteria to include a regression test proving concurrent PR runs are excluded
while all relevant runs for the target PR are analyzed.
- Around line 1090-1105: Clarify the ownership definition used by the bookmark
detection plan in docs/todo13.md: ancestor membership via `bookmarks() & ::@ ~
trunk()` alone must not be treated as proof that a bookmark belongs to the
current workspace. Specify the repository/revset mechanism that identifies
current-workspace bookmarks, and add tests covering bookmarks on shared
ancestors or merged history from concurrent workspaces, ensuring push targets
include only bookmarks owned by the current workspace.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9401eebd-3364-482d-a9c8-0e70c040d35a
📒 Files selected for processing (2)
docs/todo-summary.mddocs/todo13.md
Comment on lines
+1061
to
+1067
| - [ ] 対象 PR の時刻 range 内の pre-push run dir を列挙する関数に拡張 (`.takt/runs/*-pre-push-review`) | ||
| - [ ] context json の `prepush_reports_dir` を配列化 + facet instruction を複数 dir 対応に | ||
| - [ ] 本エントリ削除 + todo-summary.md 行削除 | ||
|
|
||
| #### 完了基準 | ||
|
|
||
| - 複数 push した PR の feedback が全 pre-push run のレポートを分析対象にすること。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
時刻範囲だけで対象PRのrunを集約しないでください。
同じ時間帯に並行PRのpre-push runが存在すると、対象PRに紐づかないレポートまで取り込む可能性があります。PR番号、bookmark、commit IDなどの安定した関連付け条件を明記し、並行PRのrunが混在する回帰テストも完了基準に追加してください。
🤖 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/todo13.md` around lines 1061 - 1067, Update the TODO entry describing
pre-push run discovery to require stable association with the target PR, such as
PR number, bookmark, or commit ID, rather than relying only on the time range.
Extend the completion criteria to include a regression test proving concurrent
PR runs are excluded while all relevant runs for the target PR are analyzed.
Comment on lines
+1090
to
+1105
| ### cli-push-runner の bookmark 検出を `::@` (自 workspace 祖先) に限定 (PR #269 post-merge-feedback T1-1 採用) | ||
|
|
||
| > **動機**: bookmark_check の検出 (`jj bookmark list`) はリポジトリ全体を対象とするため、並行 workspace の bookmark も拾い、push の `-b` 付与対象に含めてしまう。本セッションの実 push で `-b <PR#268の bookmark> -b <PR#269の bookmark>` と複数 bookmark が付与された実観測あり (両方自分のもので無害だったが、並行 workspace では他者の作業中 bookmark を巻き込む余地)。`--all` 廃止 (PR #267) の仕上げとして、検出を `::@ ~ trunk()` 等の revset で自 workspace の祖先に限定する。feedback pipeline とセッション内 dogfood が独立に同一問題を検出 (相互裏付け)。 | ||
| > | ||
| > **参照**: `.claude/feedback-reports/269.md` Tier 1 #1、`src/cli-push-runner/src/stages/bookmark_check.rs`、ADR-045 § Known operational risks (bookmark conflicts) | ||
| > | ||
| > **実行優先度**: 🚀 Tier 1 — Effort S。**順位 280 の実装 PR で消化予定** (並列安全化の仕上げとして同一テーマ)。 | ||
|
|
||
| #### 作業計画 | ||
|
|
||
| - [ ] bookmark 検出を revset ベース (`jj log -r 'bookmarks() & ::@ ~ trunk()'` 等) に変更 + テスト | ||
| - [ ] 本エントリ削除 + todo-summary.md 行削除 | ||
|
|
||
| #### 完了基準 | ||
|
|
||
| - push の `-b` 付与対象が自 workspace の祖先にある bookmark に限定されること。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
::@ だけでは「自workspace所有」の保証になりません。
bookmarks() & ::@ ~ trunk() はコミット祖先を絞る条件であり、並行workspaceのbookmarkが共有祖先やマージ済み履歴上にある場合まで除外できるとは限りません。「自workspace」の定義を明確化し、そのケースを含むテストを追加してください。
🤖 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/todo13.md` around lines 1090 - 1105, Clarify the ownership definition
used by the bookmark detection plan in docs/todo13.md: ancestor membership via
`bookmarks() & ::@ ~ trunk()` alone must not be treated as proof that a bookmark
belongs to the current workspace. Specify the repository/revset mechanism that
identifies current-workspace bookmarks, and add tests covering bookmarks on
shared ancestors or merged history from concurrent workspaces, ensuring push
targets include only bookmarks owned by the current workspace.
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)(該当なし) Applicable Findings (Medium 以下)(該当なし) Filtered (not applicable)
次のアクション
|
Resolved findings: - [Major] docs/todo13.md:1067 時刻範囲だけで対象PRのrunを集約しないでください。 - [Major] docs/todo13.md:1105 `::@` だけでは「自workspace所有」の保証になりません。
This was referenced Jul 19, 2026
4 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.
概要
PR #268 / #269 の post-merge feedback で採用が確定した 4 件を、順位 288-291 として todo 系列に登録する (ADR-033 方式)。
登録内容
::@(自 workspace 祖先) に限定 (#269 T1-1)補足
🤖 Generated with Claude Code
Summary by CodeRabbit