Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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