Skip to content

feat(hooks-stop-feedback-dispatch): Stop 時 skill 自動起動 (task 1-C)#71

Merged
aloekun merged 4 commits intomasterfrom
feat-hooks-stop-feedback-dispatch-task-1c
Apr 23, 2026
Merged

feat(hooks-stop-feedback-dispatch): Stop 時 skill 自動起動 (task 1-C)#71
aloekun merged 4 commits intomasterfrom
feat-hooks-stop-feedback-dispatch-task-1c

Conversation

@aloekun
Copy link
Copy Markdown
Owner

@aloekun aloekun commented Apr 23, 2026

Summary

  • 新規 Stop hook hooks-stop-feedback-dispatch を追加 (ADR-029)。.claude/post-merge-feedback-pending.json を検出して status=pending なら Claude に /post-merge-feedback skill 起動を指示、status=dispatched に atomic 更新
  • lib-pending-file crate を新設し、cli-merge-pipeline / hooks-stop-feedback-dispatch の PendingFile struct + epoch_secs_to_iso8601 + カレンダー定数 10 個 + is_valid_owner_repo を共通化 (3 callers 目の DRY 解消)
  • .claude/settings.local.json.template の Stop hook に 2 つ目エントリを追加 (実行順序: hooks-stop-quality → hooks-stop-feedback-dispatch)
  • package.json build scripts + scripts/deploy-hooks.ts の EXE_FILES に新規 exe を統合
  • 付随: 完了済み 1-B セクションを docs/todo.md から削除 + cli-merge-pipeline の doc コメント微修正

Context

Why: ADR-029 で規定した 3 コンポーネント連携の Stop hook 側を実装する。pnpm merge-pr 後に skill 呼び忘れでフィードバックループが失われていた課題を、pending file + 現セッション起動で解決する (新規 session spawn を避けセッション知見を維持)。

Trigger: docs/todo.md「マージ後フィードバックの定常化」task 1-C。PR #69 (ADR 策定) + PR #70 (CLI 側 task 1-B) の後続。

Scope decision: lib-pending-file 抽出は当初スコープ外の想定だったが、takt pre-push-review (simplicity) が DRY-001 として blocking 指摘 → 本 PR に含めた (3 callers 超で shared lib の閾値を明確に越えた)。skill 側 Phase 0 対応は task 1-E (別リポジトリ)、post_steps 有効化は task 1-D (別 PR) に分離。

Validation

  • cargo test (workspace): 全クレート pass (hooks-stop-feedback-dispatch 23件、他クレート回帰なし)
  • pnpm build:hooks-stop-feedback-dispatch: 成功 (exe が .claude/ に配置)
  • pnpm push pre-push review: verdict=APPROVE (3 iterations / 17m46s)
    • simplicity-review APPROVED (DRY-001 / W-001 / W-002 全 resolved)
    • security-review APPROVED (SEC-001: owner_repo injection テスト済)
  • E2E smoke test (7 経路): absent / stop_hook_active / pending / dispatched / consumed / corrupt / stale 全て ADR-029 仕様通り動作確認

References

Summary by CodeRabbit

  • 新機能

    • Stop フェーズでフィードバックを自動ディスパッチする新しいフック実行バイナリを追加
  • リファクタリング

    • 保留ファイルスキーマとUTC時刻ヘルパーを共有ライブラリへ統合し再利用性を向上
  • ドキュメント

    • タスク順序とチェックリストを整理・再編成(フロー調整、依存関係の簡素化)
  • その他の改善

    • ビルド・配布パイプラインへ新バイナリ統合、テスト・フォーマットの整理と安定化

… 起動指示 (task 1-C)

ADR-029 で規定した新規 Stop hook を追加。cli-merge-pipeline (task 1-B) が書く
`.claude/post-merge-feedback-pending.json` を検出し、status=pending なら
additionalContext で /post-merge-feedback skill の起動を Claude に指示、
status を dispatched に atomic 更新する。

責務分離 (ADR-022): hooks-stop-quality と並列の Stop hook として登録し、
実行順序 1 (品質ゲート) → 2 (dispatch) を settings template の array order で保証。

主な実装:
- src/hooks-stop-feedback-dispatch/ 新規 crate (workspace member に登録)
- pending_file.rs は cli-merge-pipeline 側から duplicate (3 callers 目での lib 切出しは
  別 PR、本 PR スコープは新規 exe + 配布統合)
- ExistingPending を Pending / Dispatched variant で分離 (cli 側の Active(String) とは差分)
- stale 判定 (24h TTL) は ISO 8601 UTC 'Z' の lex 比較性を活用
- fail-open: stdin/parse/write エラーは silent exit or stderr WARN

配布統合:
- Cargo.toml workspace members に alphabetical 順で追加
- package.json の build:hooks-stop-feedback-dispatch + build:all チェーン
- scripts/deploy-hooks.ts の EXE_FILES
- .claude/settings.local.json.template の Stop hook 2 つ目のエントリ (timeout 10s)

テスト: unit 23 件 + E2E smoke 7 経路 (absent/stop_hook_active/pending/dispatched/
consumed/corrupt/stale) 全て ADR-029 仕様通り。

付随変更 (task 1-C 完了に伴うクリーンアップ):
- docs/todo.md: 完了済み 1-B セクションを削除 + 依存関係を更新
- src/cli-merge-pipeline/src/pending_file.rs: 1 箇所の doc コメント整合性修正

Note: todo.md に 'templates/settings.json にも反映' とあるが、実際の配布テンプレートは
.claude/settings.local.json.template (deploy-hooks.ts が参照) のみで、templates/ 配下に
settings ファイルは存在しない。正しいテンプレートに反映済。
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

📝 Walkthrough

Walkthrough

Stop フェーズでペンディングファイルを読み取り・ロック・更新してフィードバック送信用 JSON を stdout に出力する新しい dispatcher 実装と共有ライブラリを追加し、ワークスペース設定・ビルド・デプロイ・既存モジュールの再エクスポートを更新しました。

Changes

Cohort / File(s) Summary
新しい共有ライブラリ
src/lib-pending-file/Cargo.toml, src/lib-pending-file/src/lib.rs
PendingFile スキーマ、ステータス定数、owner/repo 検証、epoch⇄ISO8601 と現在時刻ヘルパーを公開するライブラリを追加。
新規フィードバック・ディスパッチフック
src/hooks-stop-feedback-dispatch/Cargo.toml, src/hooks-stop-feedback-dispatch/src/main.rs, src/hooks-stop-feedback-dispatch/src/pending_file.rs
Stop フック実行ファイルを追加。stdin パース、既存ペンディングの分類(Pending/Dispatched/Consumed/Corrupt)、24h ステール判定、ロックによる重複防止、stdout JSON 出力、アトミック上書きで status→dispatched 更新を実装。多数のユニットテストあり。
ワークスペース / 依存登録
Cargo.toml, src/cli-merge-pipeline/Cargo.toml, src/cli-pr-monitor/Cargo.toml
ルート workspace に src/hooks-stop-feedback-dispatchsrc/lib-pending-file をメンバー追加。既存クレートに lib-pending-file 参照を追加。
CLI / フック設定・デプロイ
.claude/settings.local.json.template, package.json, scripts/deploy-hooks.ts
Stop フェーズへ hooks-stop-feedback-dispatch.exe を追加。npm スクリプトとデプロイスクリプトで新バイナリのビルド/コピーを追加。
pending-file の統合 (再エクスポート)
src/cli-merge-pipeline/src/pending_file.rs, src/cli-pr-monitor/src/util.rs
各モジュール内のローカル pending-file / UTC ヘルパーを削除し、lib_pending_file から pub(crate) use による再エクスポートで重複を解消。テストの一部を調整。
書式・テストの軽微変更群
src/check-ci-coderabbit/src/main.rs, src/hooks-pre-tool-validate/src/main.rs, src/hooks-session-start/src/main.rs, src/hooks-stop-quality/src/main.rs, src/lib-jj-helpers/src/lib.rs, src/lib-report-formatter/src/lib.rs, src/cli-push-pipeline/src/main.rs, src/cli-pr-monitor/src/config.rs, src/cli-push-runner/..., src/cli-push-runner/src/stages/push_jj_bookmark.rs
複数ファイルでフォーマット整形、ログ/フォーマット呼び出しの折返し変更、テストアサーションの簡略化など。基本的にロジックは不変。
ドキュメント更新
docs/todo.md
ポストマージ・フィードバック自動化タスクの依存順序を再構成し、1-B を削除する等のタスク記述を更新。

Sequence Diagram(s)

sequenceDiagram
    participant StopPhase as Stop Phase
    participant Dispatcher as hooks-stop-feedback-dispatch
    participant FS as Filesystem
    participant Lock as PendingLock
    participant Stdout as Stdout

    StopPhase->>Dispatcher: 起動 (stdin JSON)
    Dispatcher->>Dispatcher: parse -> check stop_hook_active
    alt stop_hook_active == true
        Dispatcher-->>StopPhase: サイレント終了
    else
        Dispatcher->>FS: locate & read pending file
        FS-->>Dispatcher: ExistingPending
        alt Pending
            Dispatcher->>Dispatcher: 24h staleness 判定
            alt not stale
                Dispatcher->>Lock: try_acquire (.lock create)
                Lock-->>Dispatcher: acquired?
                Dispatcher->>FS: re-read pending
                Dispatcher->>Stdout: emit JSON {hookEventName:"Stop", additionalContext:"/post-merge-feedback <pr>"}
                Dispatcher->>FS: write_overwrite(status="dispatched", dispatched_at=UTC)
                Lock-->>Dispatcher: drop (remove .lock)
            else stale
                Dispatcher->>FS: delete stale pending (+.lock cleanup)
                Dispatcher->>Dispatcher: stderr 警告 / 終了
            end
        else Dispatched/Consumed
            Dispatcher-->>Dispatcher: サイレント終了
        else Corrupt
            Dispatcher->>FS: delete corrupt file
            Dispatcher-->>Dispatcher: サイレント終了
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding a new Stop hook for automatic skill invocation during stop phase (task 1-C), which aligns with the substantial changes in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hooks-stop-feedback-dispatch/src/pending_file.rs`:
- Around line 85-112: write_overwrite is not providing process-level atomicity
for the pending→dispatched transition, allowing multiple Stop hooks to dispatch
concurrently; fix by implementing a lock-file based claim similar to
lib-pending-file::write_new_exclusive: attempt to create a lock file named
"{file_name}.lock" with OpenOptions::create_new(true), on success re-read the
pending via read_existing, verify pending.status == "pending", then perform the
stdout dispatch and finally write the new status (use write_overwrite or its
logic) to mark "dispatched"; if the status is no longer pending or any step
fails, release/remove the lock and do nothing (no dispatch), and always ensure
the lock file is removed on exit/failure to avoid deadlock.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f4c82a41-502f-4939-9398-43a50cb4fb09

📥 Commits

Reviewing files that changed from the base of the PR and between 4db06b0 and 4c051c9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (24)
  • .claude/settings.local.json.template
  • Cargo.toml
  • docs/todo.md
  • package.json
  • scripts/deploy-hooks.ts
  • src/check-ci-coderabbit/src/main.rs
  • src/cli-merge-pipeline/Cargo.toml
  • src/cli-merge-pipeline/src/pending_file.rs
  • src/cli-pr-monitor/Cargo.toml
  • src/cli-pr-monitor/src/config.rs
  • src/cli-pr-monitor/src/util.rs
  • src/cli-push-pipeline/src/main.rs
  • src/cli-push-runner/src/config.rs
  • src/cli-push-runner/src/stages/push_jj_bookmark.rs
  • src/hooks-pre-tool-validate/src/main.rs
  • src/hooks-session-start/src/main.rs
  • src/hooks-stop-feedback-dispatch/Cargo.toml
  • src/hooks-stop-feedback-dispatch/src/main.rs
  • src/hooks-stop-feedback-dispatch/src/pending_file.rs
  • src/hooks-stop-quality/src/main.rs
  • src/lib-jj-helpers/src/lib.rs
  • src/lib-pending-file/Cargo.toml
  • src/lib-pending-file/src/lib.rs
  • src/lib-report-formatter/src/lib.rs

Comment thread src/hooks-stop-feedback-dispatch/src/pending_file.rs
… 排他化 (CodeRabbit PR #71)

CodeRabbit Major 指摘 (src/hooks-stop-feedback-dispatch/src/pending_file.rs:112):
pending→dispatched 遷移が atomic でなく複数の Stop hook が同時 dispatch 可能。

修正: `PendingLock` (RAII struct) を追加し、handle_pending で
  lock 取得 → 再読込 → status=pending 再検証 → emit → write dispatched
の順で process-level 排他を担保。lib-pending-file::write_new_exclusive と
同じ `OpenOptions::create_new(true)` パターン。

- Lock path: `{pending}.lock` (path.with_extension("lock"))
- Stale recovery: lock mtime > 60秒 なら crash 残骸扱いで gc + 1 回再取得
  (kill -9 による permanent stuck の defense-in-depth)
- Drop guard で panic / 早期 return でも RAII 解放
- stale (24h) 判定は lock 前に実行 (GC は lock 不要)

ADR-029「ロックファイル不要」は pending file 自体の **新規作成 atomicity**
(create_new) の記述であり、pending を **read して dispatched へ write** する
遷移の同期とは別軸の問題、という区別を doc コメントに明記。

テスト (5 件追加、合計 30 件 pass):
- pending_lock_atomic_under_concurrent_acquirers: 8 スレッド → exactly 1 wins
- pending_lock_recovers_from_stale_lock: File::set_modified で 120秒前 mtime 設定 → 回復確認
- pending_lock_acquires_and_releases_on_drop / _returns_none_when_already_held
- pending_lock_is_stale_with_threshold_rejects_fresh_lock / _on_missing_file_returns_false

E2E smoke: 正常 dispatch + lock cleanup、lock 競合時の silent skip + pending status 維持 を確認。
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hooks-stop-feedback-dispatch/src/pending_file.rs`:
- Around line 502-551: The test pending_lock_atomic_under_concurrent_acquirers
is flaky because the winner only holds the lock for 50ms and thread start delays
can let a second thread win; fix it by introducing an Arc<std::sync::Barrier>
(e.g. barrier) shared with each spawned thread and have each thread wait on
barrier.wait() immediately before calling PendingLock::try_acquire(&p) so all
threads attempt acquisition simultaneously; keep the existing
success_count/none_count logic and the short sleep inside the winner branch to
ensure observation, then join threads and assert counts as before.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d190fe9f-f990-4f0c-8a4e-e19ac96d3390

📥 Commits

Reviewing files that changed from the base of the PR and between 4c051c9 and bf0b2d8.

📒 Files selected for processing (2)
  • src/hooks-stop-feedback-dispatch/src/main.rs
  • src/hooks-stop-feedback-dispatch/src/pending_file.rs

Comment thread src/hooks-stop-feedback-dispatch/src/pending_file.rs
aloekun added a commit that referenced this pull request Apr 23, 2026
…abbit PR #71)

CodeRabbit Minor 指摘: pending_lock_atomic_under_concurrent_acquirers が負荷の高い CI で flaky
になりうる。winner が 50ms しか lock を保持しないため、スレッド起動遅延が 50ms を超える環境
(slow CI / Windows Defender 常駐スキャン下等) では winner drop 後に後発スレッドが再取得して
success_count=2 になる可能性があった。

修正: std::sync::Barrier で THREAD_COUNT=8 の同期を追加。全スレッドが try_acquire 直前まで
barrier.wait() で揃ってから一斉発火するため、起動遅延に依らず race が決定論化される。

- 50ms 保持は維持 (scheduling が極端に歪んでも後発が lock 存在を観測できる余裕)
- 成功条件を THREAD_COUNT - 1 に一般化 (スレッド数を変えても追従)
- ローカルで 30 回連続 run → 全 pass 確認
@aloekun
Copy link
Copy Markdown
Owner Author

aloekun commented Apr 23, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hooks-stop-feedback-dispatch/src/pending_file.rs`:
- Around line 161-168: The stale-lock recovery currently unconditionally removes
and re-creates the lock (using Self::is_stale and Self::try_create), which can
race and delete another process's newly-created lock and cause duplicate
ownership and double dispatch; change the strategy to (1) serialize GC with a
separate GC-guard create_new (e.g., "{lock}.gc") so only one process performs
cleanup, (2) make lock creation write a unique owner token (UUID/PID+nonce) into
the lock file in try_create/create_new and return that token as part of the
guard, and (3) modify Drop to only remove the lock file if the on-disk token
matches the guard's token (so a process never removes a lock it doesn't own).
Update the codepaths that call Self::is_stale/Self::try_create (and the similar
block at the other spot) to use the GC-guard flow and token-aware create/remove
logic to eliminate the race.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f67f5c66-6d25-4a95-bf5d-2ca4ff488d1b

📥 Commits

Reviewing files that changed from the base of the PR and between bf0b2d8 and 3e857a3.

📒 Files selected for processing (2)
  • src/hooks-stop-feedback-dispatch/src/main.rs
  • src/hooks-stop-feedback-dispatch/src/pending_file.rs
✅ Files skipped from review due to trivial changes (1)
  • src/hooks-stop-feedback-dispatch/src/main.rs

Comment thread src/hooks-stop-feedback-dispatch/src/pending_file.rs Outdated
aloekun added 2 commits April 24, 2026 02:09
…abbit PR #71)

CodeRabbit Minor 指摘: pending_lock_atomic_under_concurrent_acquirers が負荷の高い CI で flaky
になりうる。winner が 50ms しか lock を保持しないため、スレッド起動遅延が 50ms を超える環境
(slow CI / Windows Defender 常駐スキャン下等) では winner drop 後に後発スレッドが再取得して
success_count=2 になる可能性があった。

修正: std::sync::Barrier で THREAD_COUNT=8 の同期を追加。全スレッドが try_acquire 直前まで
barrier.wait() で揃ってから一斉発火するため、起動遅延に依らず race が決定論化される。

- 50ms 保持は維持 (scheduling が極端に歪んでも後発が lock 存在を観測できる余裕)
- 成功条件を THREAD_COUNT - 1 に一般化 (スレッド数を変えても追従)
- ローカルで 30 回連続 run → 全 pass 確認
@aloekun aloekun force-pushed the feat-hooks-stop-feedback-dispatch-task-1c branch from 3e857a3 to bfb7b57 Compare April 23, 2026 17:21
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hooks-stop-feedback-dispatch/src/main.rs`:
- Around line 127-135: The current stale-GC branch calls
is_stale(&pending.created_at, utc_now_epoch_secs()) before any lock check and
unconditionally removes path.with_extension("lock"), which can delete an active
lock held by another process; instead, acquire or inspect the lock atomically
before deleting: either perform the stale check while holding the pending-file
lock (use the same lock acquisition routine your codebase uses), or
read/validate the lock owner token/GC guard in the lock file and only remove the
.lock if the owner token indicates it is safe for this GC to reclaim it (i.e.,
we own it or it's explicitly expired); update the branch around is_stale /
std::fs::remove_file(path.with_extension("lock")) to enforce this
ownership/lock-held check before deleting the lock.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7bd5da7c-aaef-4bce-9696-a92ceb3bd7ee

📥 Commits

Reviewing files that changed from the base of the PR and between 3e857a3 and bfb7b57.

📒 Files selected for processing (2)
  • src/hooks-stop-feedback-dispatch/src/main.rs
  • src/hooks-stop-feedback-dispatch/src/pending_file.rs

Comment on lines +127 to +135
if is_stale(&pending.created_at, utc_now_epoch_secs()) {
eprintln!(
"[stop-feedback-dispatch] Warning: stale pending file removed (created_at={})",
pending.created_at
);
let _ = std::fs::remove_file(path);
// leak した可能性がある lock file も backstop で削除
let _ = std::fs::remove_file(path.with_extension("lock"));
return;
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

stale GC が稼働中の lock を削除し得ます。

Line 127 の stale 判定が lock 取得前に走り、Line 134 で {pending}.lock を無条件削除しています。24h 境界付近で別プロセスが既に lock を取得して dispatch 中だと、この branch がその active lock を消せるため、第三プロセスが再取得して二重 dispatch できます。

stale pending の削除も lock 保有下で行うか、leaked lock 回収が必要なら owner token / GC guard などで「自分が消してよい lock」だけを削除する形にしてください。Based on learnings, Auto-trigger Post-Merge Feedback using pending file and current session startup (ADR-029).

修正方向の例
 fn handle_pending(path: &Path, pending: PendingFile) {
     if is_stale(&pending.created_at, utc_now_epoch_secs()) {
-        eprintln!(
-            "[stop-feedback-dispatch] Warning: stale pending file removed (created_at={})",
-            pending.created_at
-        );
-        let _ = std::fs::remove_file(path);
-        // leak した可能性がある lock file も backstop で削除
-        let _ = std::fs::remove_file(path.with_extension("lock"));
+        // Active lock を消さないため、stale cleanup も lock 取得後に行う。
+        // try_acquire が None の場合は別プロセスが処理中、または leaked lock の可能性があるため
+        // token-aware GC なしでは lock file を削除しない。
+        let _lock = match PendingLock::try_acquire(path) {
+            Ok(Some(lock)) => lock,
+            Ok(None) => return,
+            Err(e) => {
+                eprintln!(
+                    "[stop-feedback-dispatch] Warning: lock acquisition failed during stale cleanup ({}); skip cleanup",
+                    e
+                );
+                return;
+            }
+        };
+        if matches!(read_existing(path), ExistingPending::Pending(ref p) if is_stale(&p.created_at, utc_now_epoch_secs())) {
+            eprintln!(
+                "[stop-feedback-dispatch] Warning: stale pending file removed (created_at={})",
+                pending.created_at
+            );
+            let _ = std::fs::remove_file(path);
+        }
         return;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks-stop-feedback-dispatch/src/main.rs` around lines 127 - 135, The
current stale-GC branch calls is_stale(&pending.created_at,
utc_now_epoch_secs()) before any lock check and unconditionally removes
path.with_extension("lock"), which can delete an active lock held by another
process; instead, acquire or inspect the lock atomically before deleting: either
perform the stale check while holding the pending-file lock (use the same lock
acquisition routine your codebase uses), or read/validate the lock owner
token/GC guard in the lock file and only remove the .lock if the owner token
indicates it is safe for this GC to reclaim it (i.e., we own it or it's
explicitly expired); update the branch around is_stale /
std::fs::remove_file(path.with_extension("lock")) to enforce this
ownership/lock-held check before deleting the lock.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved: 理論的には正しい指摘ですが、現時点では 既知の受容可能なトレードオフ として対応を見送ります。

race が成立する条件:

  1. pending の created_at が 24h 境界を越えた瞬間
  2. 別プロセスが同時に active に dispatch 中 (dispatch 処理は 10-100ms オーダー)
  3. 別の第三プロセスも同時に handle_pending の stale branch に到達

dispatch の処理時間と 24h 境界の重なりから、race window は実効的に 1 秒 / 86400 秒 ≈ 0.001% 未満です。さらに複数の Stop hook が同時発火する前提も加わります。

提案 fix のトレードオフ: 提案通り lock 明示削除を撤去すると、try_acquire が None を返した leaked lock (kill -9 等で Drop が走らなかった場合) を回収する手段がなくなり、permanent stuck 状態になります。これは前回の CodeRabbit Major 指摘 (stale-recovery の TOCTOU race) を簡潔版で解決した際にユーザーと合意した方向性 (撤去 + pending 24h TTL backstop) と衝突します。

本 PR のスタンス: シンプルな実装で運用を開始し、実運用で race が顕在化したら owner token + GC guard の堅牢版へ段階的に発展させる方針。現時点では実務的リスクが極めて低いため、複雑化を避けます。

もし将来この finding を再度提案する場合は、次の条件のいずれかが満たされた時点でお願いします:

  1. 実運用で double dispatch が観測された
  2. より短い stale TTL (例: 1h) を採用する必要が生じた
  3. 敵対的 process が前提の環境に移植する場合

それまでは本トレードオフで運用します。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aloekun、詳細なトレードオフ分析をありがとうございます。論点を整理します。

  • race window の評価: 24h TTL × 10–100ms dispatch window ≈ 0.001% という算定は妥当です。さらに同一 session で複数の Stop hook が並行発火する前提が必要なため、実効確率はさらに低くなります。
  • 提案 fix の副作用: lock を stale cleanup から除外すると kill -9 後の leaked lock を回収する手段が失われ、permanent stuck というより深刻な問題を生じさせます。これは前回の合意(撤去 + 24h TTL backstop)と整合しており、今回の設計判断は一貫しています。
  • 再提案の条件: 指定いただいた 3 条件(実観測 / TTL 短縮 / 敵対的環境移植)を記憶します。

設計判断を承りました。今後のレビューでは上記条件に達するまでこの finding を再提起しません。


✏️ Learnings added
Learnt from: aloekun
Repo: aloekun/claude-code-hook-test PR: 71
File: src/hooks-stop-feedback-dispatch/src/main.rs:127-135
Timestamp: 2026-04-23T17:49:41.315Z
Learning: In `aloekun/claude-code-hook-test`, `src/hooks-stop-feedback-dispatch/src/main.rs`, the stale-GC branch in `handle_pending()` unconditionally deletes `{pending}.lock` before acquiring the lock. This is an accepted tradeoff: the race window is ~0.001% (24h TTL vs 10–100ms dispatch), and acquiring the lock before stale cleanup was rejected because it would eliminate the only recovery path for leaked locks (from kill -9 etc.), causing permanent stuck state. The design relies on the 24h TTL as a backstop for leaked locks. Re-raise this finding only if: (1) double dispatch is observed in production, (2) a shorter stale TTL (e.g., 1h) is needed, or (3) the code is ported to a hostile multi-process environment.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: aloekun/claude-code-hook-test PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T03:47:18.222Z
Learning: Implement Stop hook quality gates (ADR-004)

Learnt from: CR
Repo: aloekun/claude-code-hook-test PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T03:47:18.222Z
Learning: Implement hooks in Rust (ADR-001)

Learnt from: aloekun
Repo: aloekun/claude-code-hook-test PR: 38
File: src/cli-pr-monitor/src/runner.rs:109-115
Timestamp: 2026-04-15T05:44:11.960Z
Learning: The `cli-pr-monitor` tool in `aloekun/claude-code-hook-test` is Windows-only. Hardcoded `.exe` suffixes (e.g., in `checker_exe_path()` in `src/cli-pr-monitor/src/runner.rs` and error messages in `src/cli-pr-monitor/src/stages/daemon.rs`) are intentional. Cross-platform support is not required.

@aloekun aloekun merged commit dbc95a4 into master Apr 23, 2026
1 check passed
@aloekun aloekun deleted the feat-hooks-stop-feedback-dispatch-task-1c branch April 23, 2026 17:49
aloekun added a commit that referenced this pull request Apr 24, 2026
…72)

- .claude/hooks-config.toml: プレースホルダの post_merge_learnings
  スタブを外し、type="ai" / prompt="post-merge-feedback" の
  post_steps を有効化。本プロジェクトで dogfood 開始。
- templates/hooks-config-{typescript,python}.toml: 派生プロジェクト
  向けに同ブロックをコメントアウト状態で追記。post-merge-feedback
  skill が deploy されるまで opt-in。
- docs/todo.md: 1-D 削除 + section ヘッダー / 前文を除去。
  1-E (skill 側 Phase 0) と 1-F (ADR-014 本採用化) は
  ADR-029 参照付きの独立 section に昇格。

ADR-029 試験運用の有効化。config のみの変更で exe 再ビルド不要
(cli-merge-pipeline.exe / hooks-stop-feedback-dispatch.exe は
PR #70 / #71 で deploy 済み)。実地検証は次の pnpm merge-pr で発火。

Refs: ADR-029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant