Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/todo-summary.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
| 212 | 🔧 Tier 2 | **`hooks-pre-tool-validate` に PowerShell dispatch + `powershell-destructive-write-block` preset 追加 (PR #213 post-merge-feedback feedback-T1-1 + session 派生統合)** | todo10.md | M | なし (PR #213 で PowerShell スクリプトの連鎖失敗 `IndexOf -1 → Substring 例外 → null content → WriteAllText で空 byte 書込` で main.rs 2369 行を 0 byte 化した事故、memory `feedback_no_powershell_inplace_edit` の機械化、現行 dispatch table の盲点 (PowerShell 未対応) を埋めて `WriteAllText`/`Out-File`/`Set-Content` の destructive write を PreToolUse で block、`__*` scratch prefix は exception で allow、session 派生提案と analyzer feedback-T1-1 の独立収束で高い妥当性。**Tier 列との不整合補足**: analyzer feedback report の `Tier 1: Hooks/Linter 改善` カテゴリは project の Tier 1 (🚀 high-impact urgent) ではなく、memory `feedback_tier_classification` の re-classification rule に従い実体 (mechanical enforcement = Rust 新規実装) に基づき project の Tier 2 (🔧 tooling improvements) に再分類している) |
| 213 | 🔧 Tier 2 | **`parse_coderabbit_status` の API ordering テスト追加 (PR #213 post-merge-feedback T2-1 採用)** | todo10.md | S | なし (PR #213 takt-fix iter 2 で `.last()` → `.first()` の semantic bug が修正された実観測、GitHub statuses API の reverse-chronological 返却 implicit assumption を test fixture で固定、doc comment + assert message で ordering semantics を明示、refactor 時の意味喪失を防ぐ defensive test、memory `feedback_test_dry_antipattern` 適用で独立 setup) |
| 214 | 🔧 Tier 2 | **`parse_actionable_comments` の境界条件テスト追加 (PR #213 post-merge-feedback T2-2 採用)** | todo10.md | S | なし (PR #213 takt-fix iter 2 で `t > push_time` → `t >= push_time` の境界 bug が修正された実観測、`submitted_at == push_time` 完全一致ケースで inclusive 比較を保証、既存 rule⑦ `no-time-field-strict-greater` が変数名抽出後を catch 不可なため test による second defense layer、`actionable_includes_at_exact` + `actionable_excludes_before` の 2 件で境界の inclusive/exclusive を pin) |
| 215 | 💎 Tier 3 | **`~/.claude/rules/common/coding-style.md` に「Defensive State Reset in State Machines」section 追加 (PR #214 post-merge-feedback T3-1 採用)** | todo10.md | S | なし (PR #214 round 2 で `finalize_initial_review_park` の `state.pr` / `state.repo` / `state.started_at` を `read_state()` 後に無条件上書きする pattern が CR Major #4 の fix として land、CR Major #1 (`head_commit`) + CR Major #2 (`review_recheck_count`) と同型の defensive reset が既に 3 field 適用済 = `review_recheck.rs` 内で複数 instance あり、将来の reviewer が「redundant」と誤判定して削除すると prior cycle の stale state が混入して silent bug 化、global rules への docs 追記で派生プロジェクト (techbook-ledger / auto-review-fix-vc) に自動波及、simplicity-review LLM が同 file を読むため "enforced via review" として機能し memory `feedback_no_unenforced_rules` 例外を満たす、`feedback_global_config_backup` 適用必須) |

**戦略**: Tier 1 を 2〜3 セッションで片付け → Tier 2 で ADR-032 の前提 + rate-limit + convergence cost 削減を進める → Tier 3 で ADR-032 を land + ドキュメント整備。Tier 4-5 は cleanup / 外部展開で daily efficiency への直接効果は小さい。

Expand Down
42 changes: 42 additions & 0 deletions docs/todo10.md
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,48 @@ ADR-039 (Experimental Feature 標準パターン) は「behavior の妥当性が

---

### `~/.claude/rules/common/coding-style.md` に「Defensive State Reset in State Machines」section 追加 (PR #214 post-merge-feedback T3-1 採用)

> **動機**: PR #214 round 2 で CR Major #4 (`既存 state 再利用時も現在の push 情報に更新してください`) の fix として `finalize_initial_review_park` 内で `read_state()` 後に `state.pr` / `state.repo` / `state.started_at` を `ctx` 値で **無条件上書き** する pattern を land した。この pattern は同 function 内の既存 reset と同型 (CR Major #1 fix で `head_commit` 上書き、CR Major #2 fix で `review_recheck_count = 0`) で、現時点で 3 field に適用済の確立された defensive pattern。
>
> ただしこの「無条件上書き」は新規 reader / reviewer から見ると一見「冗長 (= `unwrap_or_else(|| ::new(...))` で既に同値を設定済だから不要)」に見える危険性がある。同型コードを future PR で reviewer (人間 / AI 両方) が「redundant resets は削除すべき」と誤判定して削除した場合、prior cycle の stale state (古い PR 番号 / repo / 開始時刻) が混入する silent bug を導入するリスクが顕在化する。
>
> **本タスクの位置づけ**: PR #214 post-merge-feedback Tier 3 #1 採用 (Severity Medium / Frequency Medium / Effort S / Adoption Risk None、2026-06-20 ユーザー承認)。analyzer rationale: 「PR #214 の `review_recheck.rs` で positive pattern として land (lines 185–187)。同型コード (`review_recheck_count`, `head_commit` 上書き) との一貫性がある確立されたパターン。Frequency Medium = cli-pr-monitor には複数の state machine があり再発確実。Effort S、Adoption Risk None → ✅ 採用候補と判定」。pre-push:simplicity + pre-push:security の独立 2 ソース検出。
>
> **参照**: `.claude/feedback-reports/214.md` Tier 3 #1、[src/cli-pr-monitor/src/stages/poll/review_recheck.rs:177-193](../src/cli-pr-monitor/src/stages/poll/review_recheck.rs#L177-L193) (現行 `finalize_initial_review_park` の defensive reset block)、memory `feedback_no_unenforced_rules.md` (enforcement 要件)、memory `feedback_global_config_backup.md` (snapshot 必須)。
>
> **実行優先度**: 💎 **Tier 3** — Effort S。global rules への docs 追記 ~30 行で完結、`feedback_global_config_backup` snapshot を忘れない。

#### 設計決定 (案)

- **追加先**: `~/.claude/rules/common/coding-style.md` の末尾 (`## Code Quality Checklist` の直前) または `## Error Handling` 直後に新 section「Defensive State Reset in State Machines」を追加
- **rule 内容**: 「State machine 内で `read_state()` / `load_state()` 等の persisted state を再利用する場合、`new()` で設定される identity field と同等の **無条件上書き reset** を `read_state()` 後に明示的に書く。これは『冗長』に見えるが、prior cycle の stale state (古い PR 番号 / repo / session ID 等) が再利用 path で混入する silent bug を防ぐ defensive pattern。reviewer (人間 / AI) は redundant 削除を提案しないこと」
- **anti-pattern 警告**: `let state = read_state().unwrap_or_else(|| State::new(id, repo, time));` だけで identity field を `ctx` で上書きしないと、prior cycle の値が残留する
- **good pattern 例**: PR #214 `review_recheck.rs:177-193` を inline cite (`state.pr` / `state.repo` / `state.started_at` / `state.review_recheck_count` / `state.head_commit` の 5 field reset)
- **由来 cite**: PR #214 の CR Major #4 が「既存 state 再利用時も現在の push 情報に更新してください」として独立検出した実証
- **enforcement layer**: 機械 lint は困難 (`read_state` pattern の構文認識 + identity field 列挙が必要) だが、simplicity-review LLM が `coding-style.md` を読むため "enforced via review" として機能、memory `feedback_no_unenforced_rules` 例外を満たす
- **派生プロジェクト波及**: `~/.claude/rules/common/` 配下のため techbook-ledger / auto-review-fix-vc に自動

#### 作業計画

- [ ] `~/.claude/` snapshot 取得 (memory `feedback_global_config_backup` per)
- [ ] `~/.claude/rules/common/coding-style.md` に新 section「Defensive State Reset in State Machines」追記 (anti-pattern + good pattern + PR #214 由来 cite、約 30 行)
- [ ] markdownlint clean
- [ ] 本エントリ削除 + todo-summary.md 行削除

#### 完了基準

- `~/.claude/rules/common/coding-style.md` に「Defensive State Reset in State Machines」section が追加される
- 派生プロジェクト (techbook-ledger / auto-review-fix-vc) に global rule として自動波及
- 由来 cite (PR #214 CR Major #4 + `review_recheck.rs:177-193`) で reviewer / Claude が rule 背景を理解可能
- simplicity-review LLM が future PR で同型 `read_state()` を含む state machine 編集を review する際、本 section の anti-pattern 警告を参照可能

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

なし。Effort S、global rules への docs 追記のみ、`feedback_global_config_backup` snapshot を忘れない。

---

## 既知課題 (記録のみ、本セッションで未対応)

(現時点で本ファイルへの既知課題は無し。docs/todo9.md 末尾を参照。)
61 changes: 61 additions & 0 deletions src/check-ci-coderabbit/src/parsers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,28 @@ mod tests {
assert_eq!(parse_coderabbit_status(json), "success");
}

/// 順位 213 (PR #213 post-merge-feedback T2-1 採用): GitHub commit statuses API は
/// reverse-chronological 返却 (新しい→古い順) であり、`parse_coderabbit_status` は
/// `.first()` で「最新 state」を取得する。この semantics を test 名 + doc comment +
/// assert message で explicit に固定し、refactor 時 (例: `.first()` → `.last()`) に
/// 意図しない意味変更を機械的に検出する。
///
/// 由来: PR #213 takt-fix iter 2 で `.last()` (= 最古 state を選んでいた) → `.first()`
/// の semantic fix が行われたが、元 production code (main.rs 時代) のコメントが
/// 「最新エントリ」と書きながら `.last()` を使っていた drift 事例。test 表現で固定する。
#[test]
fn cr_status_reverse_chronological_picks_first() {
let json = r#"[
{"context": "CodeRabbit", "state": "success"},
{"context": "CodeRabbit", "state": "pending"}
]"#;
assert_eq!(
parse_coderabbit_status(json),
"success",
"GitHub statuses API は reverse-chronological 返却のため、配列先頭 (`.first()`) が最新 state。`.last()` (古い state) を返すと CR レビュー終了を見逃す"
);
}

#[test]
fn walkthrough_clean_detected_when_marker_present_with_header() {
let json = r#"[
Expand Down Expand Up @@ -466,6 +488,45 @@ mod tests {
);
}

/// 順位 214 (PR #213 post-merge-feedback T2-2 採用): `parse_actionable_comments` の
/// `submitted_at >= push_time` inclusive 比較を境界で固定する。`>` (exclusive) に
/// 戻された際にこの test が落ちる構造で、direct push 直後の CR review が同時刻
/// イベントとして報告される edge case の retest 取りこぼしを機械的に防ぐ。
///
/// 由来: PR #213 takt-fix iter 2 で `t > push_time` → `t >= push_time` の境界 fix。
/// 既存 rule⑦ `no-time-field-strict-greater` は `submitted_at` 名直接使用 ケースのみ
/// catch し、変数名 `t` に抽出された後は static lint 不能。test による second defense layer。
#[test]
fn actionable_includes_review_at_exact_push_time() {
let json = r#"[
{"user": {"login": "coderabbitai[bot]"}, "body": "Actionable comments posted: 3", "submitted_at": "2026-04-01T12:00:00Z"}
]"#;
assert_eq!(
parse_actionable_comments(json, "2026-04-01T12:00:00Z"),
Some(3),
"submitted_at == push_time の review は inclusive 比較で含むべき (`>=` が `>` に戻されると取りこぼす)"
);
}

/// 順位 214 (negative sentinel): 配列 latest 位置に「push 以前の sentinel 99」を
/// 置き、time filter が壊れた場合 `rfind` がそれを先に返す構造にする。time filter が
/// 正しく動くと sentinel は除外され、配列前方の `>=` 適合 review (= 5) が選ばれる。
/// 既存 `actionable_filters_by_time` は単一 review のみで test するが、本 test は
/// 「複数 review 中で boundary 適合のみを選ぶ」exclusion 挙動を sentinel pre-populate
/// で固定する (memory `feedback_test_dry_antipattern.md` 適用、独立 setup)。
#[test]
fn actionable_excludes_review_before_push_time() {
let json = r#"[
{"user": {"login": "coderabbitai[bot]"}, "body": "Actionable comments posted: 5", "submitted_at": "2026-04-01T12:30:00Z"},
{"user": {"login": "coderabbitai[bot]"}, "body": "Actionable comments posted: 99", "submitted_at": "2026-04-01T11:00:00Z"}
]"#;
assert_eq!(
parse_actionable_comments(json, "2026-04-01T12:00:00Z"),
Some(5),
"submitted_at < push_time の sentinel review (Actionable: 99) は time filter で除外され、push_time 以降の review (Actionable: 5) が選ばれるべき。99 が返れば time filter が機能していない"
);
}

#[test]
fn extract_count_from_body() {
assert_eq!(
Expand Down
Loading