diff --git a/docs/todo.md b/docs/todo.md index bde9502..ce8faad 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -27,8 +27,7 @@ | 8 | 🔧 Tier 2 | 週次レビュー (ADR-031) Phase B 実装 | todo.md | 中-高 | なし (順位 20 の compensating check 前提) | | 10 | 🔧 Tier 2 | ADR-032 PR-broken-link: broken-link-check + 内部アンカー検査 統合 | todo2.md | Small-中 | なし (clean baseline 確立済) | | 11 | 🔧 Tier 2 | `cli-pr-monitor` プロセス正常終了の integration test (PR #85 T2-2) | todo2.md | S | なし | -| 12 | 🔧 Tier 2 | **`cli-pr-monitor` ポーリング延長 + 重複起動ロック (PR #88 T2-4)** ★ rate-limit critical | todo3.md | Medium | なし (Polling anti-pattern 検出 (PR #86 T1-1, 完了済) と補完) | -| 13 | 🔧 Tier 2 | **post-pr-review に rate-limit 自動検出 + 再トリガー (PR #89 T2-1)** ★ rate-limit critical | todo3.md | Medium | なし (順位 12 と補完) | +| 13 | 🔧 Tier 2 | **post-pr-review に rate-limit 自動検出 + 再トリガー (PR #89 T2-1)** ★ rate-limit critical | todo3.md | Medium | なし (`cli-pr-monitor` ポーリング延長 + 重複起動ロック (PR #88 T2-4、完了済) と補完) | | 15 | 🔧 Tier 2 | **cli-pr-monitor 通知 Recovery 経路 (SessionStart hook 拡張)** ★ silent loss prevention | todo3.md | S/M | なし (ADR-030 L2 recovery パターンを cli-pr-monitor に適用) | | 16 | 🔧 Tier 2 | **`vitest` を devDependencies に固定 (PR #88 T2-3)** | todo3.md | Small | なし | | 17 | 🔧 Tier 2 | **`pnpm create-pr` 必須引数ヘルプ改善 (PR #88 T2-5)** | todo3.md | Small | なし | @@ -45,19 +44,23 @@ | 28 | ⏳ Tier 5 | (追って) ADR-030 の takt-test-vc 反映 | todo.md | 中 | 順位 27 Phase F | | 29 | 🚀 Tier 1 | **非 docs ファイル `docs/todo` 参照検出 lint rule (PR #94 T1-1) ★ Bundle U** | todo3.md | S | なし (PR #94 直接対策、Cross-File Reference Lifecycle の決定論的防止層) | | 30 | 💎 Tier 3 | **Cross-File Reference Lifecycle ルールに具体例追記 (PR #94 T3-2) ★ Bundle U** | todo3.md | XS | なし (Bundle U で 29 と並行 land 推奨) | +| 31 | 💎 Tier 3 | **review-security.md docs-only fast-approve から `.takt/**` と `.claude/**` を明示除外 (PR #95 T3-2) ★ Bundle V** | todo3.md | XS | なし (PR #95 直接対策) | +| 32 | 💎 Tier 3 | **docs/todo.md ヘッダの「新規タスクは追加しない」表記を実態整合 (PR #95 T3-3) ★ Bundle V** | todo3.md | XS | なし (PR #95 直接対策) | +| 33 | 💎 Tier 3 | **code-review.md に新 verdict 経路追加時の 3 点チェックリスト追記 (PR #95 T3-4) ★ Bundle V** | todo3.md | XS | なし (PR #95 直接対策、verdict path 整合性) | **戦略**: Tier 1 を 2〜3 セッションで片付け → Tier 2 で ADR-032 の前提 + rate-limit + convergence cost 削減を進める → Tier 3 で ADR-032 を land + ドキュメント整備。Tier 4-5 は cleanup / 外部展開で daily efficiency への直接効果は小さい。 **Bundle 1 完了 + post-merge-feedback 反映 (2026-04-29)**: PR #91 (Bundle 1: PowerShell + Markdown anchor lint rules) merge 後の post-merge-feedback で **4 件の新規 task を追加** (PowerShell `(?i)` 自動検証 / `.claude/` filter + ADR-030 制約 / cli-pr-monitor 通知 Recovery 経路 / takt REJECT-ESCALATE)。**前 2 件は本 PR で実証された「fix iteration の根因」に対する決定論的防止策で最優先候補**。**日付ベース見出しアンカーのグローバル明文化 task は決定論的防止 (no-mutable-anchor rule) との二重防衛として継続有効**。 -**reviewer facet 改善 (Bundle T で land 済)** + **post-pr-review fix loop の `.claude/` filter (Bundle T で land 済)** が完了し、reviewer 精度向上 + convergence cost 削減の二段構えが成立。残る Tier 2 では rate-limit critical 系 (cli-pr-monitor ポーリング延長 / post-pr-review rate-limit 自動検出) を最優先候補とする。 -**rate-limit 系の 2 タスク (cli-pr-monitor ポーリング延長 + 重複起動ロック / post-pr-review rate-limit 自動検出 + 再トリガー) は rate-limit 直撃のため Tier 2 内で最優先候補**。前者 = ポーリング頻度全体の削減、後者 = review 単位での自動再トリガー、Polling anti-pattern 検出 (PR #86 T1-1、完了済) を含む 3 層で rate-limit を抑制する設計。 +**reviewer facet 改善 (Bundle T で land 済)** + **post-pr-review fix loop の `.claude/` filter (Bundle T で land 済)** + **cli-pr-monitor ポーリング延長 + 重複起動ロック (Phase 3 で land 済)** が完了し、reviewer 精度向上 + convergence cost 削減 + ポーリング頻度削減の三段構えが成立。残る Tier 2 では rate-limit critical 系の最後の 1 つ post-pr-review rate-limit 自動検出 + 再トリガー が最優先候補。 +**rate-limit 抑制の 3 層**: (1) Polling anti-pattern 検出 (PR #86 T1-1、完了済) = Claude 側の polling 禁止 (preventive)、(2) cli-pr-monitor ポーリング延長 + 重複起動ロック (PR #88 T2-4、完了済) = tool 側のポーリング頻度削減 (corrective)、(3) post-pr-review rate-limit 自動検出 + 再トリガー (Tier 2 残) = review 単位の自動 recovery。 **cli-pr-monitor 通知 Recovery 経路 (SessionStart hook 拡張) は PR #91 の直接観測知見**。SessionStart hook で再起動跨ぎの通知ロスト防止。post-pr-review fix loop の `.claude/` filter (path-based 解決) は Bundle T で land 済。 **Stop hook の `pnpm lint:md` 統合 task は Markdown linter hook 統合 (PR #88 で merged) の gap closure**。**AI 生成一時スクリプト pattern 検出は push 前 untracked `__*` hook (PR #85 T1-4) と関連** (実装前に擦り合わせ要)。 **`.failed` marker 自己文書化 task は ADR-030 soft-fail 機構の運用負荷削減** (PR #89 セッションで recovery が機能した実証から派生、Effort S)。 **takt REJECT-ESCALATE は post-pr-review fix loop の `.claude/` filter (Bundle T で land 済) の verdict-based 一般解**。path-based 解決が完了したので、本 task 着手で補完関係を完成させる。 **T3 グローバルルール 4 件 (日付ベース見出しアンカー / jj conflict リカバリ / `__` prefix scratch / post-pr-monitor polling 禁止) は `~/.claude/` 配下への XS 追記なので並列実施推奨**。 **Bundle U (Cross-File Reference Lifecycle 強化) は PR #94 post-merge-feedback 直接対策**。非 docs ファイル `docs/todo` 参照検出 lint rule (Tier 1 = 決定論的防止) と Cross-File Reference Lifecycle ルール具体例追記 (Tier 3 = preventive guidance) を 1 PR で land 推奨。両者は同一テーマ (永続成果物→ephemeral 参照禁止の二層化) で補完関係、effort 合計 S+XS。 +**Bundle V (verdict path 整合性 + docs-only fast-approve gap 補強) は PR #95 post-merge-feedback 直接対策**。review-security.md の `.takt/**` / `.claude/**` 除外、docs/todo.md ヘッダ整合、code-review.md の 3 点チェックリスト追記の 3 件を 1 PR で land 推奨。共通テーマは「PR #95 で実証された不整合パターンへの retroactive 対策」、effort 合計 XS×3 で完結。 --- diff --git a/docs/todo3.md b/docs/todo3.md index cfaf55d..55d60b0 100644 --- a/docs/todo3.md +++ b/docs/todo3.md @@ -144,56 +144,6 @@ cmd = "pnpm lint:md" --- -### `cli-pr-monitor` ポーリング間隔延長 + 重複起動防止ロック (PR #88 T2-4) - -> **動機**: PR #88 作成後の cli-pr-monitor 監視中に、Claude Code Max (5x) のレートリミットを 1 時間で 40% 消費する事象を観測。監視セッション重複起動による累積消費が推定原因。現在の `poll_interval_secs = 120` (2分) はセッション単独では問題ないが、複数セッションで監視が重複起動すると 1 分以下の頻度で polling が走り得る。 -> -> **本タスクの位置づけ**: **既存の Polling anti-pattern 検出ルール task (PR #86 T1-1) と補完**。前者 = Claude 側の polling 禁止 (preventive)、本タスク = cli-pr-monitor (tool 側) の polling 動作改善 (corrective)。両層で rate-limit を削減する。 -> -> **参照**: `.claude/feedback-reports/88.md` の Tier 2 #4 finding -> -> **実行優先度**: 🔧 **Tier 2** — 工数 Medium。**rate-limit 直撃のため daily efficiency への影響大**。Tier 2 内では最優先候補。実装前に重複起動の根本原因 (どこで複数セッションが立つか) を特定し、ロック方式を選定する必要あり。 - -#### 背景 - -- 観測: 1 セッション内で `pnpm push` → `pnpm create-pr` の流れで 2 度 cli-pr-monitor 系の処理が走った -- post-pr-review takt workflow は内部で provider (Claude API) を呼ぶため、polling 1 サイクルが重い -- `poll_interval_secs = 120` × 監視時間 (最大 600s) = 5 サイクル/セッション。複数セッション重複で更に増える -- 重複起動の原因候補: - - VSCode 上の複数 Claude Code セッションが各々 cli-pr-monitor を起動 - - daemon と --observe / --monitor-only の組み合わせが意図せず多重化 - - state file の lockless な読み書きで race - -#### 設計決定 (案) - -- 改善 1: poll_interval_secs を `120` → `180` または `240` に延長 (config 値変更のみ) -- 改善 2: 重複起動防止 file lock (`.claude/pr-monitor.lock` など、PID + start_time 記録) -- 改善 3: lock 検出時の挙動 — 既存セッションが alive なら skip (no-op exit)、stale なら lock 奪取 -- 既存設計 (ADR-018: cli-pr-monitor takt 移行) を尊重しつつ追加 -- pr-monitor-config.toml への設定追加で柔軟性確保 - -#### 作業計画 - -- [ ] 重複起動の根本原因を実測で確認 (transcript から複数 cli-pr-monitor 起動を検出) -- [ ] file lock 機構の設計 (既存 jj 環境との互換性) -- [ ] `src/cli-pr-monitor/` の lock 取得・解放ロジック実装 -- [ ] poll_interval_secs の調整 (config 経由) -- [ ] dogfood: 複数セッションを意図的に立てて重複起動が抑制されることを確認 -- [ ] rate-limit 消費が改善前後で測定可能なら比較 -- [ ] 本 todo3.md エントリを削除 - -#### 完了基準 - -- 重複起動時に後続セッションが skip され、polling 並走が発生しない -- poll_interval_secs 延長で polling 総回数が減る -- レートリミット消費が体感で改善される (測定可能なら定量化) - -#### 詰まっている箇所 - -なし (Effort Medium、根本原因の調査が必要だが進路は明確) - ---- - ### `pnpm create-pr` 必須引数未指定時のヘルプ改善 (PR #88 T2-5) > **動機**: 引数なしで `pnpm create-pr` を実行すると `gh pr create` が `must provide --title and --body (or --fill or fill-first or --fillverbose)` エラーのみ出力し、使用例が示されない。今回 PR 作成時に手動ワークアラウンド (`pnpm prepare-pr-body` で `.tmp-pr-body.md` 生成 → `pnpm create-pr -- --title "..." --body-file .tmp-pr-body.md`) が必要になった。`gh` のエラーをそのまま流す現設計だと、Claude や人間が次の手を察するのに余計な往復が発生する。 @@ -253,7 +203,7 @@ Hint: > > **参照**: `.claude/feedback-reports/89.md` の Tier 2 #1 finding > -> **実行優先度**: 🔧 **Tier 2** — 工数 Medium。daily efficiency への影響中-大 (rate-limit 発生率 × 手動判断時間)。cli-pr-monitor ポーリング延長 + 重複起動ロック task と補完関係 (本タスクは review 単位の対応、ポーリング延長 task はポーリング頻度全体の削減)。Polling anti-pattern 検出ルール task も類似の rate-limit 削減ライン。 +> **実行優先度**: 🔧 **Tier 2** — 工数 Medium。daily efficiency への影響中-大 (rate-limit 発生率 × 手動判断時間)。cli-pr-monitor ポーリング延長 + 重複起動ロック (PR #88 T2-4、完了済) と補完関係 (本タスクは review 単位の対応、ポーリング延長 task はポーリング頻度全体の削減)。Polling anti-pattern 検出ルール task も類似の rate-limit 削減ライン。 #### 背景 @@ -300,7 +250,7 @@ Hint: > > **参照**: `.claude/feedback-reports/90.md` の Tier 2 #2 finding > -> **実行優先度**: 🔧 **Tier 2** — 工数 S。daily efficiency への影響中 (recovery 発生頻度は低いが、発生時の摩擦を低減)。rate-limit 系 task (cli-pr-monitor ポーリング延長 + post-pr-review rate-limit 自動検出) ほど critical ではないが、ADR-030 の long-term 運用品質に寄与。 +> **実行優先度**: 🔧 **Tier 2** — 工数 S。daily efficiency への影響中 (recovery 発生頻度は低いが、発生時の摩擦を低減)。rate-limit 系 task (cli-pr-monitor ポーリング延長 PR #88 T2-4、完了済 / post-pr-review rate-limit 自動検出) ほど critical ではないが、ADR-030 の long-term 運用品質に寄与。 #### 背景 @@ -595,3 +545,106 @@ prompt and prompt Claude to re-run the workflow. #### 詰まっている箇所 なし + +### review-security.md docs-only fast-approve から `.takt/**` と `.claude/**` を明示除外 (PR #95 T3-2) + +> **動機**: PR #95 (Bundle T) で `.takt/facets/instructions/review-security.md` に「Docs-only changes: trust boundary criterion」セクションを追加し、trust boundary 不変な docs 変更を APPROVE 即判定する fast path を定義した。CodeRabbit の outside-diff 指摘で、`.takt/**` 配下の md は AI 挙動を制御する code-equivalent (今 PR の verdict routing 変更がまさに該当) であり、`.claude/**` も同様に AI 設定を含むため、docs-only fast-approve から除外しないと security gap が生じることが判明。 +> +> **本タスクの位置づけ**: Bundle V (PR #95 post-merge-feedback) の 3 件 retroactive 対策の 1 つ。`.takt/**` と `.claude/**` を Markdown でもツールチェーン設定として扱い、fast-approve の対象外と明記。 +> +> **参照**: `.claude/feedback-reports/95.md` の Tier 3 #2 finding +> +> **実行優先度**: 💎 **Tier 3** — 工数 XS。既存セクション (Cross-File Reference Lifecycle と並ぶ formal-rule セクション) への追記のみ。Bundle V として 3 件並行 land 推奨。 + +#### 設計決定 (案) + +- 配置先: `.takt/facets/instructions/review-security.md` の "Docs-only changes: trust boundary criterion" セクション +- 既存「Pass criterion」「Trust boundary unchanged」「Trust boundary changed」のうち、「Pass criterion」直下に **明示除外パス** を追加: + > **Excluded paths (docs-only fast-approve は適用しない)**: `.takt/**` (AI 挙動制御 = code-equivalent) / `.claude/**` (Claude Code 設定 = code-equivalent)。これらの md 変更は trust boundary 評価を通常通り実施する +- 補足理由: `.takt/facets/instructions/` の md は LLM 行動を変える instruction、`.claude/hooks-config.toml` 等は hook 挙動を変える config — 形式上は md/config だが実質 code + +#### 作業計画 + +- [ ] `.takt/facets/instructions/review-security.md` の Docs-only セクションに excluded paths 追記 +- [ ] dogfood: 次回 `.takt/**` を含む docs PR で security review が APPROVE 即判定しないことを確認 +- [ ] 本 todo3.md エントリを削除 + +#### 完了基準 + +- review-security.md に `.takt/**` と `.claude/**` の明示除外が記載される +- 次回 `.takt/**` を含む PR で security review が docs-only fast path をスキップする + +#### 詰まっている箇所 + +なし (Effort XS、既存セクションへの 1 文追記のみ) + +### docs/todo.md ヘッダの「新規タスクは追加しない」表記を実態整合 (PR #95 T3-3) + +> **動機**: docs/todo.md の冒頭運用ルールに「**docs/todo.md**: 既存タスクの編集・完了削除専用。新規タスクは追加しない」と記述があるが、実態としては推奨実行順序サマリー table への新規行追加は継続的に行われている (PR #94 で順位 29/30、PR #95 で順位 31-33 を追加済)。CodeRabbit Major outside-diff 指摘でこの矛盾を指摘された。本文の詳細エントリ追加と table への行追加を区別する形で表記を整合させる。 +> +> **本タスクの位置づけ**: Bundle V (PR #95 post-merge-feedback) の 3 件 retroactive 対策の 1 つ。同種指摘の将来再発を防止。 +> +> **参照**: `.claude/feedback-reports/95.md` の Tier 3 #3 finding +> +> **実行優先度**: 💎 **Tier 3** — 工数 XS。docs/todo.md ヘッダの 1 行修正のみ。Bundle V として 3 件並行 land 推奨。 + +#### 設計決定 (案) + +- 配置先: `docs/todo.md` の冒頭運用ルール (line 6) +- 修正方針: + - 既存: `**docs/todo.md**: 既存タスクの編集・完了削除専用。新規タスクは追加しない (~50KB 閾値内に維持し...)` + - 修正後: `**docs/todo.md**: 既存タスクの編集・完了削除と推奨実行順序サマリー table の管理専用。新規タスクの**詳細エントリ**は追加しない (~50KB 閾値内に維持し...)。table への新規行追加は可 (詳細は docs/todo3.md に記録)` +- 同様の表記を docs/todo2.md にも展開する場合は別途検討 (todo2.md は 50KB 到達済で読み専用に近い扱い) + +#### 作業計画 + +- [ ] `docs/todo.md` ヘッダの該当行を修正 +- [ ] 必要に応じて docs/todo2.md / docs/todo3.md ヘッダにも整合化 +- [ ] dogfood: 次回 task 追加時に CodeRabbit が同じ指摘を出さないことを確認 +- [ ] 本 todo3.md エントリを削除 + +#### 完了基準 + +- docs/todo.md ヘッダの table 管理ポリシーが実態と整合 +- 同種 CodeRabbit 指摘が再発しない + +#### 詰まっている箇所 + +なし (Effort XS、ヘッダ 1 行修正) + +### code-review.md に新 verdict 経路追加時の 3 点チェックリスト追記 (PR #95 T3-4) + +> **動機**: PR #95 (Bundle T) で `analyze-coderabbit.md` に新 verdict 経路 `user_decision_path` を追加した際、(1) Step 3 の severity 取得元の記述、(2) 出力テーブルの Severity 列、(3) Verdict 判定ロジックの 3 箇所が同一 commit で整合しておらず、CodeRabbit Major 指摘 (1 件目) を受けた。さらに ADR-030 と analyze-coderabbit.md の整合性も同様に乱れて Major 2 件目 / Minor 1 件目 / 表記揺れ 1 件目を誘発。同じ「3 点同期失敗」パターンが pre-push (advisor)、first CR round、second CR round、third CR round の 4 フェーズに渡って発生しており、慣習化が必要。 +> +> **本タスクの位置づけ**: Bundle V (PR #95 post-merge-feedback) の 3 件 retroactive 対策の 1 つ。コードレビュー観点 (`~/.claude/rules/common/code-review.md`) に具体的なチェックリストを追記し、AI が編集時に意識化する。 +> +> **参照**: `.claude/feedback-reports/95.md` の Tier 3 #4 finding +> +> **実行優先度**: 💎 **Tier 3** — 工数 XS。既存 code-review.md の Common Issues to Catch セクション拡充。Bundle V として 3 件並行 land 推奨。 + +#### 設計決定 (案) + +- 配置先: `~/.claude/rules/common/code-review.md` (グローバルルール、全プロジェクト適用) +- 追記内容 (案): 新規セクション or 既存「Code Quality」サブセクション拡充 + > **新 verdict 経路 / 分類カテゴリ追加時の 3 点同期チェック**: facet 指示書 / ADR / 検証ロジックを同時編集する場合、以下 3 箇所が必ず同一 commit で整合していることを確認する: + > 1. **取得元の明示**: 新カテゴリの severity / 識別子がどこから来るか (CodeRabbit field / ADR の規定 / 計算結果) を Step 1 段階で明記 + > 2. **出力フォーマットの整合**: report テーブル列・サマリー文・指示書本文が同じ取得元の同じフィールドを参照していること + > 3. **判定ロジックの整合**: verdict rule / routing logic が新カテゴリを正しくハンドルし、severity 参照が取得元と一致していること +- PR #95 を実例として `>` blockquote で例示 (`user_decision_path` 追加時の 3 点同期失敗パターン) + +#### 作業計画 + +- [ ] `~/.claude/rules/common/code-review.md` に 3 点チェックリスト追記 +- [ ] PR #95 の実例を blockquote で例示 +- [ ] 派生プロジェクトへ deploy (グローバルルールなのでセット展開) +- [ ] dogfood: 次回 verdict 経路追加 PR でチェックリストが有効に機能するか観察 +- [ ] 本 todo3.md エントリを削除 + +#### 完了基準 + +- グローバルルール `~/.claude/rules/common/code-review.md` に 3 点チェックリストが記載される +- 同種「3 点同期失敗」パターンが将来 PR で再発しない + +#### 詰まっている箇所 + +なし (Effort XS、グローバルルール 1 セクション追記のみ) diff --git a/src/cli-pr-monitor/src/config.rs b/src/cli-pr-monitor/src/config.rs index 510e294..1b5097a 100644 --- a/src/cli-pr-monitor/src/config.rs +++ b/src/cli-pr-monitor/src/config.rs @@ -3,7 +3,11 @@ use std::path::{Path, PathBuf}; use crate::log::log_info; -pub(crate) const DEFAULT_POLL_INTERVAL: u64 = 120; +/// poll_interval_secs のデフォルト。 +/// 120s から 180s に延長 (PR #88 T2-4): rate-limit 浪費を抑えるため、 +/// セッション単独でも polling 回数を 5 → ~3 サイクル/監視に削減。 +/// max_duration_secs (600s) の維持と組み合わせて、polling 総数を 40% 削減する。 +pub(crate) const DEFAULT_POLL_INTERVAL: u64 = 180; pub(crate) const DEFAULT_MAX_DURATION: u64 = 600; pub(crate) const DEFAULT_STEP_TIMEOUT_SECS: u64 = 300; pub(crate) const DEFAULT_CHECK_TIMEOUT_SECS: u64 = 60; diff --git a/src/cli-pr-monitor/src/lock.rs b/src/cli-pr-monitor/src/lock.rs new file mode 100644 index 0000000..f035bd5 --- /dev/null +++ b/src/cli-pr-monitor/src/lock.rs @@ -0,0 +1,512 @@ +//! 重複起動防止 file lock +//! +//! `start_monitoring` の polling + takt 並走を防ぐため、`.claude/pr-monitor.lock` に +//! PID + start_time を記録する。同時に複数の cli-pr-monitor が polling を回すと +//! Claude Code Max のレートリミットを浪費するため (PR #88 dogfood で実測)、 +//! 1 リポジトリ 1 アクティブ監視 にゲートする。 +//! +//! 仕様: +//! - acquire(): lock file を atomic create。既存 lock が "fresh" (start_time が +//! `stale_threshold_secs` 以内) なら None (= 別インスタンスが走行中、skip)。 +//! stale (timeout 超過) なら overwrite して取得。 +//! - Drop: lock file を削除。プロセス crash 時は file が残るが、stale 判定で +//! `stale_threshold_secs` 経過後に次インスタンスが takeover できる。 +//! +//! `--observe` / `--mark-notified` は read-only / one-shot mutation のため guard 対象外。 +//! polling + takt を回す `start_monitoring` のみ guard する。 + +use serde::{Deserialize, Serialize}; +use std::fs::OpenOptions; +use std::io::Write; +use std::path::PathBuf; + +use crate::log::log_info; + +const LOCK_FILENAME: &str = ".claude/pr-monitor.lock"; +/// stale 判定 threshold。max_duration_secs (600s = 10min) の 3x で安全マージン。 +const DEFAULT_STALE_THRESHOLD_SECS: i64 = 1800; + +#[derive(Serialize, Deserialize)] +struct LockFile { + pid: u32, + start_time: String, + mode: String, +} + +/// Lock 取得成功時に保持する RAII guard。Drop で lock file を削除する。 +pub(crate) struct MonitorLock { + path: PathBuf, +} + +impl Drop for MonitorLock { + fn drop(&mut self) { + if let Err(e) = std::fs::remove_file(&self.path) { + // already removed (race) なら無視。それ以外は warn。 + if e.kind() != std::io::ErrorKind::NotFound { + log_info(&format!("[lock] cleanup 失敗: {}", e)); + } + } + } +} + +/// Lock 取得結果。 +pub(crate) enum LockResult { + /// 取得成功。guard が drop されるまで保持される。 + Acquired(MonitorLock), + /// 別インスタンスが fresh な lock を保持中 → skip 推奨。 + Busy { + holder_pid: u32, + holder_age_secs: i64, + }, + /// lock ファイルの作成に失敗 (権限不足等)。lock 機能なしで継続可能。 + Unavailable { reason: String }, +} + +/// `start_monitoring` 用 lock を取得する。`mode` は debug 用の人間可読ラベル。 +pub(crate) fn acquire(mode: &str) -> LockResult { + acquire_at(lock_path(), mode, DEFAULT_STALE_THRESHOLD_SECS) +} + +/// テスト用: lock path / stale threshold を引数化。 +/// +/// レース対策: `OpenOptions::create_new` で atomic create を試み、AlreadyExists の +/// 場合のみ既存 lock の stale 判定にフォールバックする。read-then-write の TOCTOU +/// race を排除する設計。stale 判定後の overwrite は仕様上 race を許容 (stale = +/// 監視者なし、複数 takeover が同時に成功しても無害)。 +pub(crate) fn acquire_at(path: PathBuf, mode: &str, stale_threshold_secs: i64) -> LockResult { + if let Some(parent) = path.parent() { + let _ = std::fs::create_dir_all(parent); + } + + let content = match build_lock_content(mode) { + Some(c) => c, + None => { + return LockResult::Unavailable { + reason: "lock content serialize 失敗".to_string(), + } + } + }; + + match OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(mut f) => { + if let Err(e) = f.write_all(content.as_bytes()) { + log_info(&format!("[lock] 新規 lock 書き込み失敗 (継続): {}", e)); + } + LockResult::Acquired(MonitorLock { path }) + } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + // fresh な lock が存在する間は後続セッションを skip してレートリミット浪費を防ぐ + if let Some((holder, age_secs)) = read_fresh_lock(&path, stale_threshold_secs) { + return LockResult::Busy { + holder_pid: holder.pid, + holder_age_secs: age_secs, + }; + } + // stale takeover: overwrite。複数の takeover が並走しても last write wins で + // どれか 1 つの guard が cleanup する (不変条件: lock file は最終的に消える)。 + if let Err(e) = std::fs::write(&path, content) { + log_info(&format!("[lock] takeover 書き込み失敗 (継続): {}", e)); + } + LockResult::Acquired(MonitorLock { path }) + } + Err(e) => { + // I/O エラー (権限不足等): lock なしで監視は継続可能。 + log_info(&format!("[lock] create_new 失敗 (lock なしで継続): {}", e)); + LockResult::Unavailable { + reason: e.to_string(), + } + } + } +} + +fn build_lock_content(mode: &str) -> Option { + let lock = LockFile { + pid: std::process::id(), + start_time: crate::util::utc_now_iso8601(), + mode: mode.to_string(), + }; + match toml::to_string(&lock) { + Ok(c) => Some(c), + Err(e) => { + log_info(&format!("[lock] serialize 失敗 (lock なしで継続): {}", e)); + None + } + } +} + +/// 既存 lock が fresh なら `Some((LockFile, age_secs))` を返す。 +/// stale (parse 失敗 / 超過) の場合は `None` (= 取得可)。 +fn read_fresh_lock(path: &PathBuf, stale_threshold_secs: i64) -> Option<(LockFile, i64)> { + let content = std::fs::read_to_string(path).ok()?; + let lock: LockFile = match toml::from_str(&content) { + Ok(l) => l, + Err(e) => { + log_info(&format!( + "[lock] 既存 lock の parse 失敗 (stale 扱い): {}", + e + )); + return None; + } + }; + let age_secs = parse_age_secs(&lock.start_time)?; + if age_secs < stale_threshold_secs { + Some((lock, age_secs)) + } else { + log_info(&format!( + "[lock] 既存 lock は stale (pid={}, age={}s > {}s threshold)、takeover", + lock.pid, age_secs, stale_threshold_secs + )); + None + } +} + +/// ISO 8601 文字列から「現在からの経過秒数」を返す。 +/// +/// stale 扱いになるケース (None を返す): +/// - parse 失敗 +/// - 未来日時 (then > now): 時計巻き戻しや破損 future timestamp の lock を +/// 永続 fresh で塩漬けにしないよう明示的に stale 化する。`saturating_sub` だと +/// age=0 で常に fresh 扱いになり crash recovery が機能しない。 +fn parse_age_secs(iso8601: &str) -> Option { + let then = parse_iso8601(iso8601)?; + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .ok()? + .as_secs() as i64; + if then > now { + return None; + } + Some(now - then) +} + +/// ISO 8601 (`2026-04-30T05:00:00Z` 形式) を Unix epoch secs にパース。 +/// chrono を依存させずに済むよう手書き parse。 +/// フィールドの値域を検証し、範囲外なら None を返す (corrupt lock → stale 扱い)。 +fn parse_iso8601(s: &str) -> Option { + let s = s.trim_end_matches('Z'); + let mut parts = s.split('T'); + let date = parts.next()?; + let time = parts.next()?; + + let mut date_parts = date.split('-'); + let year: i64 = date_parts.next()?.parse().ok()?; + let month: i64 = date_parts.next()?.parse().ok()?; + let day: i64 = date_parts.next()?.parse().ok()?; + + let mut time_parts = time.split(':'); + let hour: i64 = time_parts.next()?.parse().ok()?; + let minute: i64 = time_parts.next()?.parse().ok()?; + let second: i64 = time_parts.next()?.parse().ok()?; + + // Range checks: out-of-bounds values cause index-out-of-bounds panic in + // days_from_epoch. Returning None lets read_fresh_lock treat the lock as stale. + if !(1970..=9999).contains(&year) + || !(1..=12).contains(&month) + || !(1..=days_in_month(year, month)).contains(&day) + || !(0..=23).contains(&hour) + || !(0..=59).contains(&minute) + || !(0..=59).contains(&second) + { + return None; + } + + Some(unix_timestamp(year, month, day, hour, minute, second)) +} + +fn days_in_month(year: i64, month: i64) -> i64 { + let month_days = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; + let base = month_days[(month - 1) as usize]; + if month == 2 && is_leap(year) { + base + 1 + } else { + base + } +} + +/// 単純な Unix epoch 計算 (UTC 前提、うるう秒は無視)。 +fn unix_timestamp(year: i64, month: i64, day: i64, hour: i64, minute: i64, second: i64) -> i64 { + let days = days_from_epoch(year, month, day); + days * 86400 + hour * 3600 + minute * 60 + second +} + +fn days_from_epoch(year: i64, month: i64, day: i64) -> i64 { + let mut days: i64 = 0; + for y in 1970..year { + days += if is_leap(y) { 366 } else { 365 }; + } + let month_days = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; + for m in 1..month { + let idx = (m - 1) as usize; + days += month_days[idx]; + if m == 2 && is_leap(year) { + days += 1; + } + } + days + day - 1 +} + +fn is_leap(year: i64) -> bool { + (year % 4 == 0 && year % 100 != 0) || year % 400 == 0 +} + +fn lock_path() -> PathBuf { + PathBuf::from(LOCK_FILENAME) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + #[test] + fn acquire_in_clean_dir_succeeds() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + match acquire_at(path.clone(), "test", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Acquired(_lock) => { + assert!(path.exists(), "lock file should be created"); + } + LockResult::Busy { .. } => panic!("expected Acquired in clean dir"), + LockResult::Unavailable { reason } => { + panic!( + "expected Acquired in clean dir, got Unavailable: {}", + reason + ) + } + } + } + + #[test] + fn drop_removes_lock_file() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + { + let _lock = match acquire_at(path.clone(), "test", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Acquired(l) => l, + LockResult::Busy { .. } => panic!("expected Acquired"), + LockResult::Unavailable { reason } => { + panic!("expected Acquired, got Unavailable: {}", reason) + } + }; + assert!(path.exists()); + } + assert!(!path.exists(), "Drop should remove the lock file"); + } + + #[test] + fn fresh_lock_blocks_second_acquire() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + let _first = match acquire_at(path.clone(), "first", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Acquired(l) => l, + LockResult::Busy { .. } => panic!("expected Acquired for first"), + LockResult::Unavailable { reason } => { + panic!("expected Acquired for first, got Unavailable: {}", reason) + } + }; + + match acquire_at(path.clone(), "second", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Busy { holder_pid, .. } => { + assert_eq!(holder_pid, std::process::id()); + } + LockResult::Acquired(_) => panic!("second should be Busy while first holds"), + LockResult::Unavailable { reason } => { + panic!("second should be Busy, got Unavailable: {}", reason) + } + } + } + + #[test] + fn stale_lock_is_taken_over() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + // 古い start_time を持つ lock を仕込む (1980-01-01 = epoch+10年、確実に stale) + let stale = LockFile { + pid: 999_999, + start_time: "1980-01-01T00:00:00Z".into(), + mode: "stale-test".into(), + }; + std::fs::write(&path, toml::to_string(&stale).unwrap()).unwrap(); + + // threshold=1800s でも 1980 は stale 判定 → takeover 成功 + match acquire_at(path.clone(), "takeover", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Acquired(_lock) => { + let content = std::fs::read_to_string(&path).unwrap(); + assert!(content.contains(&format!("pid = {}", std::process::id()))); + } + LockResult::Busy { .. } => panic!("stale lock should allow takeover"), + LockResult::Unavailable { reason } => { + panic!( + "stale lock should allow takeover, got Unavailable: {}", + reason + ) + } + } + } + + #[test] + fn concurrent_acquire_only_one_wins() { + // 真の concurrency test: 8 thread が同一 path に同時 acquire を試み、 + // 1 つだけが Acquired (lock 保持) で残りは Busy になることを確認。 + // create_new による atomic create が機能していない場合、複数が + // Acquired になり test 失敗する。 + // + // 2 barrier 構成の意図: `start` で全 thread 同時に acquire_at に突入させ、 + // `finish` で全 thread が判定を終えるまで Acquired guard を保持する。 + // 1 barrier だと先行 thread の guard が判定後に即 drop され、後続 thread が + // 逐次 Acquired する flaky window が生じる (CR finding E)。 + use std::sync::{Arc, Barrier}; + use std::thread; + + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + let start = Arc::new(Barrier::new(8)); + let finish = Arc::new(Barrier::new(8)); + let mut handles = vec![]; + for _ in 0..8 { + let p = path.clone(); + let start_b = start.clone(); + let finish_b = finish.clone(); + handles.push(thread::spawn(move || { + start_b.wait(); + let result = acquire_at(p, "concurrent", DEFAULT_STALE_THRESHOLD_SECS); + let acquired = matches!(result, LockResult::Acquired(_)); + // 全 thread の判定が終わるまで result (Acquired なら guard) を保持 + finish_b.wait(); + acquired + })); + } + let acquired_count = handles + .into_iter() + .map(|h| h.join().unwrap()) + .filter(|&v| v) + .count(); + // race-safe な実装なら 1 thread のみが Acquired。 + // (Drop は thread 終了で走るため、その後の状態は不定 — 取得回数だけを検証) + assert_eq!( + acquired_count, 1, + "exactly one thread should acquire the lock under concurrency" + ); + } + + #[test] + fn lock_format_matches_util_iso8601() { + // util::utc_now_iso8601() の出力 format と本 module の parse_iso8601 が + // round-trip することを確認 (advisor 指摘の "format alignment" check)。 + let now = crate::util::utc_now_iso8601(); + let parsed = parse_iso8601(&now); + assert!( + parsed.is_some(), + "util's iso8601 format must parse: {}", + now + ); + } + + #[test] + fn future_timestamp_lock_is_taken_over() { + // 時計巻き戻し / 破損 future timestamp の lock が永続 fresh で塩漬けに + // ならず、stale 扱いで takeover されることを確認 (CR finding D)。 + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + // 9999 年は確実に未来 (parse_iso8601 上限内) + let future = LockFile { + pid: 999_999, + start_time: "9999-01-01T00:00:00Z".into(), + mode: "future-test".into(), + }; + std::fs::write(&path, toml::to_string(&future).unwrap()).unwrap(); + + match acquire_at(path.clone(), "takeover", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Acquired(_lock) => { + let content = std::fs::read_to_string(&path).unwrap(); + assert!( + content.contains(&format!("pid = {}", std::process::id())), + "lock should be overwritten with current PID" + ); + } + LockResult::Busy { + holder_age_secs, .. + } => panic!( + "future timestamp should be treated as stale, got Busy with age={}s", + holder_age_secs + ), + LockResult::Unavailable { reason } => { + panic!( + "future-stale takeover should succeed, got Unavailable: {}", + reason + ) + } + } + } + + #[test] + fn corrupt_lock_is_taken_over() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("pr-monitor.lock"); + // parse 不能な内容を仕込む + std::fs::write(&path, "this is not valid toml :::").unwrap(); + + match acquire_at(path.clone(), "takeover", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Acquired(_lock) => {} + LockResult::Busy { .. } => panic!("corrupt lock should be treated as stale"), + LockResult::Unavailable { reason } => { + panic!( + "corrupt lock should allow takeover, got Unavailable: {}", + reason + ) + } + } + } + + #[test] + fn parse_iso8601_round_trip() { + // 2026-04-30T00:00:00Z = 56 yr from epoch with leap year handling + // 単純にパースが動くことを確認 + let ts = parse_iso8601("2026-04-30T00:00:00Z").unwrap(); + // 2026-04-30 should be > 2025-01-01 (1735689600 sec) and < 2027-01-01 + assert!(ts > 1_735_689_600); + assert!(ts < 1_798_761_600); + } + + #[test] + fn is_leap_correctness() { + assert!(is_leap(2024)); + assert!(!is_leap(2025)); + assert!(!is_leap(1900)); // century non-leap + assert!(is_leap(2000)); // 400-year leap + } + + #[test] + fn parse_iso8601_rejects_out_of_range_month() { + // month=99 would cause index-out-of-bounds in days_from_epoch without bounds check + assert_eq!(parse_iso8601("2026-99-30T00:00:00Z"), None); + } + + #[test] + fn parse_iso8601_rejects_out_of_range_fields() { + assert_eq!(parse_iso8601("1969-01-01T00:00:00Z"), None); // year < 1970 + assert_eq!(parse_iso8601("2026-00-01T00:00:00Z"), None); // month = 0 + assert_eq!(parse_iso8601("2026-13-01T00:00:00Z"), None); // month > 12 + assert_eq!(parse_iso8601("2026-01-00T00:00:00Z"), None); // day = 0 + assert_eq!(parse_iso8601("2026-01-32T00:00:00Z"), None); // day > 31 + assert_eq!(parse_iso8601("2026-01-01T24:00:00Z"), None); // hour = 24 + assert_eq!(parse_iso8601("2026-01-01T00:60:00Z"), None); // minute = 60 + assert_eq!(parse_iso8601("2026-01-01T00:00:60Z"), None); // second = 60 + assert_eq!(parse_iso8601("2026-02-29T00:00:00Z"), None); // day 29 in non-leap year + } + + #[test] + fn io_error_returns_unavailable() { + let tmp = tempfile::TempDir::new().unwrap(); + // Create a regular file at the path that will be used as a parent directory. + // create_dir_all on a file path silently fails, so open() gets a non-AlreadyExists error. + let file_as_dir = tmp.path().join("notadir"); + std::fs::write(&file_as_dir, "content").unwrap(); + let path = file_as_dir.join("pr-monitor.lock"); + match acquire_at(path, "test", DEFAULT_STALE_THRESHOLD_SECS) { + LockResult::Unavailable { .. } => {} + LockResult::Acquired(_) => panic!("expected Unavailable on I/O error, got Acquired"), + LockResult::Busy { .. } => panic!("expected Unavailable on I/O error, got Busy"), + } + } +} diff --git a/src/cli-pr-monitor/src/main.rs b/src/cli-pr-monitor/src/main.rs index 2e9cc07..2a4e925 100644 --- a/src/cli-pr-monitor/src/main.rs +++ b/src/cli-pr-monitor/src/main.rs @@ -24,6 +24,7 @@ mod config; mod fix_commit; +mod lock; mod log; mod runner; mod stages; diff --git a/src/cli-pr-monitor/src/stages/monitor.rs b/src/cli-pr-monitor/src/stages/monitor.rs index 0a4c71d..977c020 100644 --- a/src/cli-pr-monitor/src/stages/monitor.rs +++ b/src/cli-pr-monitor/src/stages/monitor.rs @@ -1,5 +1,6 @@ use crate::config::load_config; use crate::fix_commit::{create_fix_commit, FixCommitState}; +use crate::lock::{acquire as acquire_lock, LockResult}; use crate::log::{log_info, truncate_safe}; use crate::stages::collect::collect_findings; use crate::stages::poll::run_poll_loop; @@ -18,6 +19,30 @@ pub(crate) fn start_monitoring(pr_info: &PrInfo) -> i32 { return 0; } + // 重複起動防止 (PR #88 T2-4): 同一リポジトリで polling + takt が並走すると + // Claude Code レートリミットを浪費するため、`.claude/pr-monitor.lock` で + // 1 アクティブ監視 にゲートする。Drop guard が cleanup 担当。 + let _lock_guard: Option = match acquire_lock("start_monitoring") { + LockResult::Acquired(lock) => Some(lock), + LockResult::Busy { + holder_pid, + holder_age_secs, + } => { + log_info(&format!( + "[lock] 別の cli-pr-monitor が走行中 (pid={}, age={}s)、本セッションは skip", + holder_pid, holder_age_secs + )); + return 0; + } + LockResult::Unavailable { reason } => { + log_info(&format!( + "[lock] lock 取得不可 (lock なしで継続): {}", + reason + )); + None + } + }; + let pr_label = pr_info .pr_number .map(|n| format!("PR #{}", n))