Skip to content

fix(pr-monitor): post-pr-review の docs-only 判定を決定論層へ引き上げる (順位 233) #435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aloekun merged 1 commit into from
Aug 21, 2026
Merged

fix(pr-monitor): post-pr-review の docs-only 判定を決定論層へ引き上げる (順位 233) #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .takt/facets/instructions/analyze-coderabbit.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Read `.takt/review-comments.json`. This file contains the output from `check-ci-
- `findings`: Array of structured findings (severity, file, line, issue, suggestion, source)
- `action`: Terminal action from the monitor ("action_required", "stop_monitoring_success", etc.)
- `summary`: Human-readable summary
- `docs_only`: Boolean ADR-035 verdict for the **whole PR**, decided deterministically by `cli-pr-monitor` from GitHub's PR-wide file list (never re-derive it from a diff)

## Task

Expand All @@ -19,7 +20,7 @@ Read `.takt/review-comments.json`. This file contains the output from `check-ci-
CodeRabbit sometimes raises findings that are not applicable to this project. Before classifying severity, evaluate each finding against the project context:

1. Read `CLAUDE.md` to understand the project's architecture decisions and constraints
2. **Determine if the PR is docs-only** under [ADR-035](../../../docs/adr/adr-035-doc-evaluation-policy.md): inspect the diff in `.takt/review-diff.txt`. The PR is docs-only when **all** changed files are `docs/**` / `*.md` / source-code doc comments / yaml comment-only, **and** no executable code logic changes. Excluded paths (`.takt/facets/instructions/**`, `.claude/**`, `.takt/workflows/**.yaml` structural changes, `docs/claude-code-web-tasks.md` = the nightly loop's task ledger per ADR-072) disqualify docs-only treatment even when the file extension is `.md`/`.yaml`
2. **Read the docs-only verdict** for [ADR-035](../../../docs/adr/adr-035-doc-evaluation-policy.md) from the `docs_only` field of `.takt/review-comments.json`. It is a boolean already decided by the deterministic layer (`cli-pr-monitor` classifies GitHub's PR-wide file list through `lib-docs-policy`, the single implementation of ADR-035's path criteria). **Do not derive it yourself, and do not read `.takt/review-diff.txt`** — that file is a pre-push artifact that can hold a tip-only diff or a leftover from a different PR, which is exactly how a code-bearing PR got misclassified as docs-only (PR #227). If `docs_only` is absent, treat it as `false` (an old exe wrote the file; fail-closed = apply findings normally)
3. For each finding, check:
- **Platform scope**: This project targets Windows only. Findings about cross-platform compatibility (e.g., `.exe` hardcoding) are NOT applicable -- downgrade to `Info`
- **Intentional design**: Check if the finding contradicts an ADR decision. If so, mark as `not_applicable`
Expand Down
7 changes: 6 additions & 1 deletion .takt/facets/instructions/fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,12 @@ New files (post-only) and files absent from `@-` are reported `metrics_check: sk

After completing all fixes (Edit/Write operations) AND before emitting the `convergence_verdict` line, refresh `.takt/review-diff.txt` so the next reviewer iteration (if `convergence_verdict: partial`) sees the post-fix state:

jj diff -r @ > .takt/review-diff.txt
jj diff --git -r 'trunk()..@' > .takt/review-diff.txt

Both parts of that command are load-bearing — it must reproduce the scope and format `cli-push-runner` used when it first wrote the file, not a narrower one:

- **`trunk()..@` (the whole PR), not `-r @`.** `-r @` writes the tip commit only, so on a multi-commit PR the ancestors' changes vanish from the file the next iteration reads. Reviewers then judge a PR they can only partly see, and the `analyze-coderabbit` step used to misread such a diff as a docs-only PR (todo 順位 233 / PR #227). `trunk()` is a jj builtin that resolves the remote trunk bookmark, so this works unchanged in derived projects whose default branch is `main`
- **`--git` (unified diff), not jj's default format.** The default format loses `+`/`-` markers once colour is stripped, and LLM reviewers read deletions as additions (todo 順位 264)

This refresh is **unconditional**:

Expand Down
1 change: 0 additions & 1 deletion docs/todo-summary2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
| 228 | 🔧 Tier 2 | **`evaluate_rate_limit_shortcut` の cr_clean regression test (PR #224 post-merge-feedback T2-1 採用)** | todo13.md | S | なし (Fix 3 で拡張した `unresolved_threads` / `new_comments` / `actionable_comments` 3 field の clean 判定の回帰防止、None/境界ケース網羅、silent-clean 誤認の保護。2026-08-17: 対象パスを実体 `poll/rate_limit/tests.rs` へ修正 — 旧記載 `rate_limit_signal.rs` は module 分割で消滅) |
| 231 | 💎 Tier 3 | **ADR-022 拡張 — pre-create cleanup flow 例 + agent fmt スコープ指針 (PR #224 post-merge-feedback T3-1 採用)** | todo13.md | S | なし (CodeRabbit が `create_fix_commit` の空 findings 設計を bug 誤判定=却下 CR#2、agent 無差別 fmt の 2 事象を ADR-022 責務分離で codify、doc-only) |
| 232 | 🔧 Tier 2 | **post-merge-feedback / workflow agent の repo 作業ツリー書込禁止 + 検知安全網 (PR #224 セッション合意)** | todo13.md | S-M | なし (merge 時に analyze-session agent が repo root に throwaway script (parse_transcript.py) を残した、日常工程ゆえ累積リスク = コンテキスト汚染。(1) feedback facets に repo 書込禁止 + jq/scratch 使用を明記 (2) post_steps/Stop hook で root 新規 untracked を warning 検知 (3) gitignore は補助) |
| 233 | 🔧 Tier 2 | **post-pr-review (takt) の diff scope を PR 全体に修正 — `@` 限定による docs-only 誤判定解消 (PR #227 観測)** | todo13.md | M | なし (PR #227 で post-pr-review analyze が `@` コミット (docs のみ) の diff を見て PR を docs-only 誤判定し、CodeRabbit が PR 全体で出した finding (create_pr.rs:208) を ADR-035 docs-only filter で誤って適用外化。今回は finding も false positive (composition root) だったため実害なしだが有効 finding 見逃しリスク。根因は ADR-027 pre-push-review の `jj diff -r @` 由来 review-diff.txt 流用 or post-pr-review 独自 @ 限定 diff 生成の疑い。diff scope を PR 全体 (base..head) に修正 or 分類を CodeRabbit findings file path 基準に変更) |
| 234 | 💎 Tier 3 | **memory `feedback-di-over-ambient-global-tests` に serialization primitive 例外境界 + PR #227 具体例を追記 (PR #227 post-merge-feedback T3-1 採用)** | todo13.md | XS | なし (PR #227 が memory 原則「DI over ambient global」の 2 例目 = PR #224 env_override_lock も同根、Frequency Medium。(a) `PR_MONITOR_STATE_FILE_OVERRIDE` race → `state_path: &Path` DI 解消の具体例 (b) serialization primitive `OnceLock<Mutex<()>>` は複製禁止だが通常 test helper は複製推奨という例外境界、を追記。「DI over ambient global」と「helper 複製推奨」の見かけの矛盾を解消。順位 235 と相補) |
| 235 | 💎 Tier 3 | **ADR-022 に Serialization Primitive Single-Instance Rule の Appendix 追加 (PR #227 post-merge-feedback T3-2 採用)** | todo13.md | S | なし (PR #224 T2-2 共有 env_override_lock helper 抽出 + PR #227 で同根の serialization primitive 単一化問題 2 PR 観測 = Frequency Medium。`OnceLock<Mutex<()>>` 等を複製すると各々独立した Mutex になり競合排除が破壊される特殊ケースを ADR-022 Appendix で明文化、通常 helper 複製推奨 (DRY) との例外境界を codify。ADR-046 独立化 (feedback T3-3) との overlap は着手時判断、順位 234 と相補) |
| 236 | 🚀 Tier 1 | **tempfile mandate + PID+ms 命名 block の custom lint (PR #229 post-merge-feedback T1-1 採用)** | todo13.md | S | なし (#227 で修正した temp file collision flaky の再導入防止。#229 で本 flaky が push pipeline の `cargo test` を 3 回ブロックした実害。custom lint で `tempfile::Builder` / `NamedTempFile` を mandate + `gh-pr-body-{PID}-{ms}` 形式の手動命名を block。順位 237 = 検出層と二層防御) |
Expand Down
33 changes: 0 additions & 33 deletions docs/todo13.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,39 +162,6 @@

---

### post-pr-review (takt) の diff scope を PR 全体に修正 — `@` コミット限定による docs-only 誤判定の解消 (PR #227 観測)

> **動機**: PR #227 (cli-pr-monitor flaky 修正 2 件 + docs 整理、3 commit) の post-pr monitor で、takt `post-pr-review` の analyze が PR を **docs-only と誤判定**した。実際は spvtqwor (create_pr.rs の tempfile 化) / qzpwsyzr (state path DI) の Rust 変更を含むが、analyze が見た diff は `@` コミット (`docs/todo*.md` のみ) だった。その結果、CodeRabbit が PR 全体 (`create_pr.rs:208`) を見て出した finding を ADR-035 docs-only filter で「適用外」と**誤フィルタ**した。今回は finding 自体も false positive (composition root のため DI 不要) だったため実害はなかったが、**有効な finding を見逃すリスク**がある。
>
> **本タスクの位置づけ**: PR #227 セッション観測 (2026-06-30)、ユーザー判断で todo 登録。CodeRabbit が PR 全体 (base..head) を見るのに対し takt の判定 diff が `@` 限定で、findings と local diff scope が構造的に不整合になる点が核心。
>
> **参照**: PR #227、`.takt/review-comments.json` (findings = `create_pr.rs:208`)、push runner ログ `[diff] 実行: jj diff -r @` / `review-diff.txt (68 行)`、ADR-027 (push-time review は `@` の simplicity 限定、architectural review は post-PR CodeRabbit に委ねる)、ADR-035 (docs-only 評価ポリシー — classify の入力 diff scope を誤ると誤適用)、cli-pr-monitor の `post-pr-review` 起動箇所 (`stages/takt.rs` 周辺)。
>
> **実行優先度**: 🔧 **Tier 2** — Effort M。診断 (diff scope の生成箇所特定) + 修正。実害は CodeRabbit がフル PR を見るため現状限定的だが、自動フィルタの信頼性に関わる。

#### 設計決定 (案)

- **(A)** post-pr-review の analyze に渡す diff を PR 全体 (`master..@` または PR base..head) に変更する。`@` 限定の pre-push-review (ADR-027) とは射程が異なる (post-PR は PR 全体を評価すべき) ことを明示。
- **(B)** または docs-only 分類を local diff でなく **CodeRabbit findings の file path 基準** に切り替える (findings が code file を指すなら docs-only にしない)。
- pre-push-review (ADR-027 = `@` 限定 simplicity) と diff 生成を共有しているなら、post-pr-review 専用に分離する。

#### 作業計画

- [ ] post-pr-review が docs-only 判定に使う diff の生成箇所を特定 (`review-diff.txt` 流用 or 独自生成)
- [ ] diff scope を PR 全体に修正、or 分類基準を findings file path に変更
- [ ] dogfood: code + docs 混在 PR で docs-only 誤判定しないことを確認
- [ ] 本 entry 削除 + todo-summary2.md 行削除

#### 完了基準

- code 変更を含む PR が post-pr-review で docs-only と誤判定されず、code file を指す CodeRabbit finding が ADR-035 filter で誤って適用外にされない。

#### 詰まっている箇所

- diff scope を PR 全体にする際、pre-push-review (ADR-027 = `@` 限定 simplicity) との設定/生成共有部分に影響しないか。post-pr-review 専用に diff 生成を分離する必要があるか。

---

### memory `feedback-di-over-ambient-global-tests` に serialization primitive 例外境界 + PR #227 具体例を追記 (PR #227 post-merge-feedback T3-1 採用)

> **動機**: PR #227 (cli-pr-monitor 並列テスト flaky 修正) の post-merge-feedback で採用候補 (T3-1) として浮上。既存 memory `feedback-di-over-ambient-global-tests` の「DI over ambient global」原則が、直感的には「通常 test helper は複製推奨」原則と矛盾するように見える問題を解消する。PR #227 は本原則の 2 例目 (PR #224 の env_override_lock 関連も同根)。
Expand Down
154 changes: 150 additions & 4 deletions src/cli-pr-monitor/src/stages/collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,60 @@
//! Collect stage — poll 結果を `.takt/review-comments.json` に書き出す。
//!
//! ## docs-only 判定を決定論層で行う理由 (順位 233)
//!
//! 以前は post-pr-review の `analyze-coderabbit` facet が `.takt/review-diff.txt` を
//! **目視して** PR が docs-only かを分類していた。この入力は 2 通りに壊れる:
//!
//! - `.takt/facets/instructions/fix.md` の refresh が tip 限定 diff で上書きするため、
//! fix を挟んだ iteration では祖先コミットの code 変更が消える
//! - post-pr-review は push の後に走るので、pre-push run が diff を書かなかった場合
//! (`DiffResult::Empty` で takt skip 等) **別 PR の残骸**がそのまま残る
//!
//! どちらでも「code を含む PR」を docs-only と誤判定し、CodeRabbit が PR 全体を見て
//! 出した finding を ADR-035 filter で誤って適用外にする (PR #227 実観測)。
//!
//! そこで判定を決定論層へ引き上げ、**GitHub が持つ PR 全体の変更ファイル一覧**を
//! 真実源にして `lib_docs_policy` で分類し、結果を JSON の `docs_only` として渡す。
//! facet は値を読むだけで diff を分類しない。ローカルの working copy 状態に一切
//! 依存しないため、上の 2 経路とも構造的に消える。
//!
//! fail-closed ([ADR-043]): 一覧を取得できなければ `docs_only: false` を書く。
//! docs-only 扱いは ADR-035 filter を緩める方向なので、取得失敗を `true` に倒すと
//! 「検証できなかった」が「docs だけだった」に化けて finding を握り潰す。
//!
//! **一覧が全件そろっていることも検証する**: 取得した件数と PR が申告する
//! `changedFiles` が一致しなければ `false` に倒す。「取得できた」と「全部取得できた」は
//! 別物で、後者を確かめずに分類すると、見えていないファイルが source かどうかを
//! 判断できないまま docs-only 側へ倒れる。

use std::path::Path;

use crate::log::log_info;
use crate::runner::run_gh_quiet;
use crate::stages::poll::PollResult;
use crate::util::PrInfo;

const OUTPUT_PATH: &str = ".takt/review-comments.json";

/// PollResult を .takt/review-comments.json に書き出す
///
/// instruction (analyze-coderabbit.md) が期待するフィールド:
/// action, summary, ci, coderabbit, findings
pub(crate) fn collect_findings(result: &PollResult) -> bool {
// instruction が期待するスキーマに合わせたラッパーを構築
/// action, summary, ci, coderabbit, findings, docs_only
pub(crate) fn collect_findings(result: &PollResult, pr_info: &PrInfo) -> bool {
let docs_only = resolve_docs_only(pr_info);

let wrapper = serde_json::json!({
"action": result.action,
"summary": result.summary,
"ci": result.ci,
"coderabbit": result.coderabbit,
"findings": result.findings,
"check_output": result.check_output,
"docs_only": docs_only,
});

let output_path = Path::new(OUTPUT_PATH);

// .takt/ ディレクトリが存在しない場合は作成
if let Some(parent) = output_path.parent() {
if !parent.exists() {
if let Err(e) = std::fs::create_dir_all(parent) {
Expand Down Expand Up @@ -55,3 +87,117 @@ pub(crate) fn collect_findings(result: &PollResult) -> bool {
}
}
}

/// PR 全体の変更ファイル一覧と、PR が申告する変更ファイル数。
///
/// 2 つを別々に取得して**件数の一致を要求する**のが本 struct の存在理由で、
/// 「一覧が全件そろっている」ことを取得元の挙動に依存せず検証するための対になっている
/// ([`classify_docs_only`])。
struct PrFileList {
paths: Vec<String>,
expected_count: usize,
}

/// PR 全体の変更ファイル一覧から docs-only 判定を得る (順位 233、module doc 参照)。
fn resolve_docs_only(pr_info: &PrInfo) -> bool {
let files = fetch_pr_files(pr_info);
let docs_only = classify_docs_only(files.as_ref());
log_info(&format!(
"[docs_only] PR 全体の path 基準判定 (ADR-035): {} ({})",
docs_only,
describe_file_list(files.as_ref())
));
docs_only
}

fn describe_file_list(files: Option<&PrFileList>) -> String {
match files {
None => "ファイル一覧を取得できませんでした".to_string(),
Some(f) => format!(
"取得 {} 件 / PR 申告 {} 件",
f.paths.len(),
f.expected_count
),
}
}

/// 取得済みのファイル一覧を分類する。gh 実行から切り離して単体テスト可能にする。
///
/// **件数の一致を先に要求する** (PR #435 CodeRabbit Major): 一覧が PR の申告数に
/// 満たなければ、見えていないファイルが source かどうかを確かめる術が無い。
/// 欠けた分を「無かった」と扱うと docs-only 側へ倒れて ADR-035 filter が有効な
/// finding を落とすため、不一致は必ず `false` にする ([ADR-043])。
/// 取得元の上限値 (`gh pr view --json files` の 100 件、REST の 3,000 件) を
/// 定数として持たないのは、上限が変わっても本検査が成立し続けるようにするため。
///
/// `None` (取得失敗) と `Some(空)` (ファイルが 1 件も無い) はどちらも `false`。
/// 前者は fail-closed、後者は `lib_docs_policy` 側の「空は docs-only ではない」と
/// 同じ規則で、判定規則を本関数に写し取らない (ADR-035 の drift 防止)。
fn classify_docs_only(files: Option<&PrFileList>) -> bool {
let Some(files) = files else {
return false;
};
if files.paths.len() != files.expected_count {
return false;
}
lib_docs_policy::is_docs_only_paths(files.paths.iter().map(String::as_str))
}

/// PR 全体の変更ファイル一覧を GitHub から取得する。
///
/// `jj diff` ではなく GitHub を真実源にするのは、post-pr-review 時点のローカル
/// working copy が PR の内容と一致している保証が無いため (fix step の途中経過や、
/// 別ブランチへ移動した後の `--monitor-only` 再実行)。
///
/// `repo` が解決できない場合も `None` (fail-closed): endpoint を組み立てられない。
fn fetch_pr_files(pr_info: &PrInfo) -> Option<PrFileList> {
let pr_number = pr_info.pr_number?;
let repo = pr_info.repo.as_deref()?;
let expected_count = fetch_changed_file_count(pr_number, repo)?;
let paths = fetch_all_file_paths(pr_number, repo)?;
Some(PrFileList {
paths,
expected_count,
})
}

/// PR が申告する変更ファイル数 (`changedFiles`)。一覧の全件性を検証する対照値。
fn fetch_changed_file_count(pr_number: u64, repo: &str) -> Option<usize> {
let number = pr_number.to_string();
let raw = run_gh_quiet(&[
"pr",
"view",
&number,
"--repo",
repo,
"--json",
"changedFiles",
"-q",
".changedFiles",
])?;
raw.trim().parse::<usize>().ok()
}

/// 変更ファイルのパスを **全ページ**取得する。
///
/// `gh pr view --json files` を使わないのは、100 件で**無言で切り捨てる**ため
/// (実測: 185 ファイルの PR で 100 件しか返らない / cli/cli#13338)。切り捨てられた
/// 一覧をそのまま分類すると、101 件目以降の source file が見えず docs-only へ
/// 誤って倒れる。REST の files endpoint を `--paginate` で辿る。
fn fetch_all_file_paths(pr_number: u64, repo: &str) -> Option<Vec<String>> {
let endpoint = format!("repos/{}/pulls/{}/files", repo, pr_number);
let raw = run_gh_quiet(&["api", "--paginate", &endpoint, "--jq", ".[].filename"])?;
let paths: Vec<String> = raw
.lines()
.map(str::trim)
.filter(|l| !l.is_empty())
.map(str::to_string)
.collect();
if paths.is_empty() {
return None;
}
Some(paths)
}

#[cfg(test)]
mod tests;
Loading
Loading