refactor(hooks): 3 hook の main.rs を module 分割 (PR-3a、file_length lint 違反解消) - #217
Merged
Conversation
…/T3-3) PR #216 (cleanup-stale-todo-weekly-enable) の post-merge-feedback で 6 提案中 4 件 (T1-1 / T3-1 / T3-2 / T3-3) をユーザー承認 (2026-06-23) し docs/todo-summary.md + docs/todo10.md に entry 化: - 順位 216 (🔧 Tier 2、Bundle 216-217): `no-workstream-seq-names-in-config` lint rule 追加 — config comment 内 `PR-[0-9]+` ephemeral workstream sequence の機械的検出。analyzer Tier 1 分類は memory feedback_tier_classification に従い project Tier 2 (mechanical = T2) に再分類。 - 順位 217 (💎 Tier 3、Bundle 216-217): coding-style.md § Cross-File Reference Lifecycle に config file comments の permanent artifact 扱い明記 + workstream sequence 禁止例追加 (順位 216 の文書層補完、2 層防御)。 - 順位 218 (💎 Tier 3): ADR-039 § Bounded Lifetime + patterns.md に provisional `enabled` 変更時の todo entry 必須化を追加 (config comment-only tracking の silent aging 防止)。 - 順位 219 (💎 Tier 3): development-workflow.md § 設計 doc/実装の同期チェック に「commit description 言及 ≠ 実装完了」明文化 (PR #216 cleanup での 順位 215 救出事例を inline cite、analyzer naïve assumption の構造的予防)。 採用されなかった T2-1 / T2-2 (analyzer cross-check / provisional auto-detect) は 🤔 様子見継続。Frequency Low 初観測 + Effort M + takt test infra 未調査のため、 2 PR 以上の再観測後に Tier 1 昇格を再評価する方針。 ファイル変更: - docs/todo-summary.md: table に 4 rows 追加 (順位 215 直後、lines 89-92) - docs/todo10.md: 詳細 entry 4 件追加 ("## 既知課題" 直前、lines 512/568/620/674)
…s-post-tool-linter を module 分割 3 hook crate の main.rs を coding-style.md § File Organization (800 行 max) 内に収まる module 構成に分割。behavior 不変な mechanical refactor で、各関数の signature / export 関係は維持し、test も co-located mod tests として各 module に分散する。 PR-3 (layered config refactor) の前置 PR (PR-3a)。順位 147 (file_length lint) が PR #202 で land、本 PR-3a で 3 hook の touch-trigger ratchet 違反を解消することで PR-3b (layered config) を clean state で進められる。 対象: - src/hooks-session-start/src/main.rs (1611 行) → 6-7 module - src/hooks-pre-tool-validate/src/main.rs (2914 行) → 5-7 module - src/hooks-post-tool-linter/src/main.rs (3316 行) → 6-8 module 完了基準: - 全 module ファイルが 800 行以下 - cargo clippy --workspace -- -D warnings clean - cargo test --workspace pass (behavior 不変) - PostToolUse comment-lint-rust の file_length lint 0 件
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough3つの Claude フックバイナリ（hooks-post-tool-linter、hooks-pre-tool-validate、hooks-session-start）を大規模に新規実装・リファクタリング。post-tool-linter にカスタムルールエンジン・UTF-8 整合性・ファイルサイズ・パイプライン lint を追加し、pre-tool-validate にブロックパターンプリセットライブラリと todo staleness 検知を追加し、session-start の nudge 生成ロジックをサブモジュールへ分離。docs の todo バックログに4件の新規タスクを追記。 Changeshooks-post-tool-linter: カスタムルールエンジン・lint レイヤ新規実装
hooks-pre-tool-validate: プリセット・保護ファイル・staleness 実装
hooks-session-start: nudge 生成ロジックをサブモジュール分離
docs・その他: todo バックログ追記・ISO8601 厳密化
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks-session-start/src/main.rs (1)
163-175: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winenv file の重複判定を exact export 行にしてください。
Line 164 の
contains(marker)だとコメントやOLD_CLAUDE_CODE_SESSION_IDでも skip し、別 session の古い export も更新されません。Line 221-235 の test も stale 値を許す期待値になっています。修正案
fn write_to_env_file(env_file: &str, session_id: &str) { let marker = "CLAUDE_CODE_SESSION_ID"; + let export_line = format!("export {}={}", marker, shell_quote(session_id)); if let Ok(content) = std::fs::read_to_string(env_file) { - if content.contains(marker) { + if content + .lines() + .any(|line| line.trim() == export_line.as_str()) + { return; } } use std::io::Write; @@ .append(true) .open(env_file) { - let _ = writeln!(f, "export {}={}", marker, shell_quote(session_id)); + let _ = writeln!(f, "{export_line}"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-session-start/src/main.rs` around lines 163 - 175, The current duplication check using contains(marker) on the file content is too broad and will match the marker string anywhere including in comments or variable names like OLD_CLAUDE_CODE_SESSION_ID, preventing proper updates of stale session values. Instead of checking if the marker exists anywhere in the content, check for the exact export line pattern (the complete export statement with the marker) to properly detect and skip re-adding the exact same export while allowing old stale session exports to be updated with new ones.
🧹 Nitpick comments (8)
docs/todo10.md (2)
549-550: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTask 216 の作業計画:
rule_test_coverage_check通過の確認手順が明記されている。Line 550 で「
cargo test -p hooks-post-tool-linterで rule_test_coverage_check が pass することを確認」と明示されており、前述の test_coverage 不整合を catch する CI gate が機能することが前提とされている（good）。ただし、上記 critical issue (#1comment で指摘) を修正してから実行する必要があることを明確にするため、作業計画の実行順序を以下に修正すると robustness が上がる:修正案:
- [ ] extension/test_coverage の不整合を fix (jsonc/json 削除 OR negative test の other_ext_tests 移動) - [ ] `.claude/custom-lint-rules.toml` に `[[rules]]` entry 追加 - [ ] `src/hooks-post-tool-linter/src/main.rs` の tests に positive/negative test 追加 - [ ] `cargo test -p hooks-post-tool-linter` で rule_test_coverage_check が pass することを確認🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/todo10.md` around lines 549 - 550, The task plan in the TODO list does not explicitly clarify the required execution order for fixing the test coverage issue. Reorder the items in the list to ensure that the extension/test_coverage inconsistency fix (either removing jsonc/json or moving negative test's other_ext_tests) appears BEFORE the steps to add tests in main.rs and BEFORE the final cargo test verification. This ensures that the rule_test_coverage_check CI gate will pass once all preceding fixes are completed, making the task sequence more robust and easier to follow.
512-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTask 216 の実装可能性: comment 行限定の段階導入。
Line 531 で「MVP は file 全体 match で開始、false positive 観測後に comment 行限定への絞り込みを判断」と明記されており、段階導入の方針は適切。ただし comment 行 only 検出の技術的課題 (TOML/YAML/JSON の comment syntax が異なる、regex では単純な
^#では充分でない) を追加で注記するとより clear。例えば:
- TOML:
#で行が始まる（シンプル）- YAML:
#で行が始まる（同様）- JSON/JSONC: comment は
//または/* */(regex 複雑化)- config comment 限定を後で実装する場合、config parser (e.g.,
tomlcrate の comment node 分別) に依存する必要が出てくる可能性Line 563-564 の「詰まっている箇所」で既に言及されているため、実装前の留意点としては十分だが、初期 MVP の「file 全体 match」が jsonc/json に対して FP を大量生成するリスク（コード内コメント、git diff, markdown 等の埋め込みで誤検出）も併記するとより helpful。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/todo10.md` around lines 512 - 567, Add technical implementation caveats to the "詰まっている箇所" section of the design decision to clarify the challenges of transitioning from file-wide match to comment-only detection. Specifically note that TOML and YAML use `#` for comments (simple regex), but JSONC and JSON use `//` or `/* */` (complex regex), and that comment-line-only filtering may require config parser support rather than regex alone. Additionally, document that the MVP file-wide match approach carries significant false positive risk for JSONC and JSON formats due to code comments, embedded diffs, and markdown blocks that could accidentally trigger the `PR-[0-9]+` pattern, and this risk should inform the decision to narrow scope after initial false positive observations.src/hooks-post-tool-linter/src/config.rs (2)
94-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueデフォルトパイプラインのツール依存性を文書化してください。
TypeScript パイプラインは
biomeとoxlintを、Python パイプラインはruffを前提としていますが、これらのツールがインストールされていない場合の動作は未定義です。npx --no-installは既存インストールを要求します。実行時にツールが見つからない場合のエラーハンドリングは
pipeline_runner.rsで行われていると思われますが、README または設定ファイルのコメントでこれらの前提条件を文書化することを推奨します。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-post-tool-linter/src/config.rs` around lines 94 - 149, The default_ts_pipeline() and default_py_pipeline() functions have undocumented tool dependencies that will cause runtime failures if the required tools are not installed. Add documentation comments to both functions clearly stating the required external tools: biome and oxlint for the TypeScript pipeline, and ruff for the Python pipeline. These comments should explain that npx --no-install requires tools to be pre-installed, and optionally reference that error handling for missing tools is handled in pipeline_runner.rs.
153-158: 📐 Maintainability & Code Quality | 🔵 Trivial
current_exe()失敗時の明示的なログ出力を追加してください。
current_exe()が失敗した場合、エラーログなしでカレントディレクトリ (".") にフォールバックします。設定ファイルが見つからない時のデバッグを容易にするため、失敗時に警告ログを出力することを検討してください。例えば、config_path()内で失敗時にeprintln!()で通知すると、設定ファイルの解決経路がより明確になります。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-post-tool-linter/src/config.rs` around lines 153 - 158, The config_path() function silently falls back to the current directory when current_exe() fails, without any notification or logging to help with debugging. Add explicit logging using eprintln!() or a similar mechanism to warn the user when current_exe() fails and the function is falling back to the default path. This will make the configuration file resolution path more transparent and aid in troubleshooting when config files cannot be found.src/hooks-post-tool-linter/src/violation.rs (1)
64-66: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winシリアライゼーション失敗時のサイレントエラーに対処してください。
serde_json::to_string(&output)がErrを返した場合、フィードバックが出力されず、エラーログも記録されません。本番環境でこの状況が発生すると、違反が検出されたにもかかわらずユーザーに通知されない可能性があります。
Err時に stderr へログを出力することを推奨します。📝 提案する修正
- if let Ok(json) = serde_json::to_string(&output) { - println!("{}", json); + match serde_json::to_string(&output) { + Ok(json) => println!("{}", json), + Err(e) => eprintln!("[post-tool-linter] Warning: Failed to serialize feedback: {}", e), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-post-tool-linter/src/violation.rs` around lines 64 - 66, The code block handling the serialization of output to JSON currently only handles the successful case with the Ok branch, but silently ignores any serialization errors when serde_json::to_string(&output) returns Err. Add an else branch or convert the if let to a match statement to handle the Err case, and log the error details to stderr using eprintln! or similar error logging mechanism so that serialization failures are visible to the user instead of being silently ignored.src/hooks-post-tool-linter/src/custom_rules/engine.rs (2)
105-114: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePowerShell の大文字小文字非区別フラグ検出ロジックを強化してください。
現在の実装は単純な部分文字列マッチ (
pattern.contains("(?i)")) を使用しています。これは(?im)や(?is)などの複合フラグを持つパターンを検出しません。より堅牢な検出のため、正規表現の先頭で
(?から始まる flags グループ内にiが含まれているかを確認することを推奨します。♻️ 提案する改善
pub(crate) fn find_powershell_rules_missing_case_insensitive_flag( rules: &[CustomRule], ) -> Vec<String> { rules .iter() .filter(|r| r.extensions.iter().any(|e| e.eq_ignore_ascii_case("ps1"))) - .filter(|r| !r.pattern.contains("(?i)")) + .filter(|r| { + // Check for (?i) or (?im) or (?is) etc. at start of pattern + !r.pattern.starts_with("(?i") && !r.pattern.contains("(?") + .then(|| r.pattern.split("(?").nth(1)) + .flatten() + .and_then(|flags| flags.split(')').next()) + .map_or(true, |flags| !flags.contains('i')) + }) .map(|r| r.id.clone()) .collect() }または、シンプルに正規表現を使用:
use regex::Regex; let flag_pattern = Regex::new(r"\(\?[a-z]*i[a-z]*\)").unwrap(); .filter(|r| !flag_pattern.is_match(&r.pattern))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-post-tool-linter/src/custom_rules/engine.rs` around lines 105 - 114, The filter condition in the find_powershell_rules_missing_case_insensitive_flag function currently checks only for the exact substring "(?i)" using pattern.contains(), which misses case-insensitive flags combined with other flags like "(?im)" or "(?is)". Replace the simple substring check with a regex pattern that matches any flag group starting with "(?", containing the letter 'i' anywhere within that group, and ending with ")" to properly detect all variations of case-insensitive flags. This ensures the function correctly identifies PowerShell rules that have case-insensitive matching enabled regardless of what other flags are present.
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueシリアライゼーション失敗時のログ出力を検討してください。
serde_json::to_string(&violation).ok()は失敗時にNoneを返しますが、エラーはログに記録されません。これはviolation.rsのemit_feedbackと同じパターンですが、デバッグ時に問題の特定が困難になる可能性があります。失敗ケースは稀ですが、一貫性のため
emit_feedbackと同様のエラーログを追加することを検討してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-post-tool-linter/src/custom_rules/engine.rs` at line 174, The `serde_json::to_string(&violation).ok()` call silently discards serialization errors without logging them, making debugging difficult. Instead of using `.ok()`, handle the error case explicitly and add logging similar to the pattern used in `emit_feedback` from violation.rs. When serialization fails, log the error before returning None to maintain consistency across the codebase and aid in troubleshooting.src/hooks-post-tool-linter/src/pipeline_runner.rs (1)
72-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Configを参照渡しにすることを検討してください。現在、
run_pipeline_layerはconfig: Configを値渡しで受け取っていますが、Config構造体のサイズによっては非効率な可能性があります。&Configに変更することで不要なコピーやムーブを回避できます。♻️ 提案される修正
-pub(crate) fn run_pipeline_layer(file: &str, config: Config) { +pub(crate) fn run_pipeline_layer(file: &str, config: &Config) { let pipelines = config .post_tool_linter + .as_ref() - .and_then(|c| c.pipelines) + .and_then(|c| c.pipelines.as_ref()) .unwrap_or_else(default_pipelines);注:
default_pipelines()の戻り値型によっては追加の調整が必要になる場合があります。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks-post-tool-linter/src/pipeline_runner.rs` at line 72, The `run_pipeline_layer` function receives `config: Config` by value, which can be inefficient if the Config struct is large. Change the parameter in the `run_pipeline_layer` function signature from `config: Config` to `config: &Config` to use a reference instead. Then update all usages of `config` within the function body to work with the reference (adding dereferences where needed), and update all call sites of `run_pipeline_layer` to pass a reference to the config argument instead of moving the value. Check that any function calls within `run_pipeline_layer` that receive config as an argument are compatible with the reference type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/todo10.md`:
- Line 532: In the docs/todo10.md file around line 532, there is a markdown
formatting inconsistency where the second quoted text has mismatched backticks.
The text currently shows 「PR `#216``」 with a closing backtick but no opening
backtick. Fix this by either adding the missing opening backtick to make it 「`PR
`#216``」 to match the formatting of 「`#216`」, or alternatively remove both
backticks entirely and use just 「PR `#216`」 to maintain consistency. Choose
whichever formatting style is more appropriate for the document's conventions.
- Line 530: The extensions list in the `extensions` declaration at line 530
includes `jsonc` and `json`, but the test coverage section in
`[rules.test_coverage.main_ext_tests]` (lines 541-543) only covers `toml`,
`yaml`, and `yml`. This mismatch will cause `check_main_ext_keys_sanity()` in
`src/hooks-post-tool-linter/src/custom_rules/coverage.rs` to fail during CI.
Additionally, the `no_workstream_seq_skips_github_pr_number` test at line 541 is
a negative test (based on `skips_*` naming convention) but is incorrectly placed
in `main_ext_tests` which expects positive tests. Either remove `jsonc` and
`json` from the extensions list to match the current test coverage, or add
corresponding positive test declarations for these formats in the
`main_ext_tests` section, and move the negative test
`no_workstream_seq_skips_github_pr_number` from `main_ext_tests` to an
`other_ext_tests` section while adding the appropriate positive tests.
In `@src/hooks-pre-tool-validate/src/config.rs`:
- Line 32: The constant TODO_STALENESS_DEFAULT_BRANCH is set to "master", but
the repository branch strategy uses "main" as the default branch (as evidenced
by jj.rs preset messages). When staleness checking is enabled without an
explicit default_branch configuration, this constant is used as the fallback
value, causing count_commits_branch_ahead to return None for the non-existent
"master" branch, which results in all todo file edits being incorrectly blocked.
Change the value of TODO_STALENESS_DEFAULT_BRANCH from "master" to "main" to
align with the actual repository branch strategy.
In `@src/hooks-pre-tool-validate/src/todo_staleness.rs`:
- Around line 53-83: The `run_jj_with_timeout` function has a deadlock risk
because it calls `try_wait()` to wait for process completion before reading
stdout with `read_to_end()`. If the `jj` command produces output exceeding the
OS pipe buffer capacity (approximately 64KB on Linux), the child process will
block on write and never exit, causing the timeout loop to reach the deadline,
kill the process, and always return None. Fix this by reading stdout
concurrently in a separate thread while the process is running, rather than
reading it after waiting for the process to exit. Alternatively, switch to using
`Command::output()` which handles concurrent reading internally.
In `@src/hooks-session-start/src/jj_helpers.rs`:
- Around line 32-58: The issue is that stdout is piped from the child process
but not read until after the process completes (in the Ok(Some(status)) branch),
which can cause a deadlock if the jj output exceeds the pipe buffer size. The
child process will block trying to write to stdout while the parent waits on
try_wait(), preventing progress. To fix this, spawn a separate thread to drain
the piped stdout in the background while the main thread continues polling with
try_wait(). Move the stdout reading logic that currently happens after
child.stdout.take() into a dedicated thread that starts immediately after
Command::new("jj") spawns, so the pipe buffer is continuously drained regardless
of when the process completes.
In `@src/hooks-session-start/src/reaper.rs`:
- Around line 58-60: The parse_iso8601_to_unix function does not properly
validate ISO 8601 timestamps before parsing them. Currently, it unconditionally
discards content after the first dot and removes the trailing 'Z', which allows
invalid timestamps like "2026-05-13T12:33:23.bad" or strings with timezone
offsets to be treated as valid UTC times, causing false positives in orphan
detection. To fix this, validate that the input string is a valid UTC ISO 8601
timestamp by confirming it ends with the 'Z' suffix and that any fractional
seconds portion (between the dot and 'Z') contains only numeric digits before
removing these parts. Reject any timestamps that have timezone offsets or
invalid fractional parts instead of silently discarding them.
- Around line 232-240: The code has a race condition (TOCTOU issue) between the
existence check on marker and success_report and the subsequent write operation.
Between the check and the std::fs::write call, another process could create the
success_report or marker, leading to false `.failed` markers on successful runs.
Fix this by writing the marker atomically: first write the body returned by
build_reaper_failed_marker_body to a temporary file in the parent directory,
then use atomic rename (std::fs::rename) to move it to the final marker path.
This ensures the marker creation is atomic and prevents race conditions where
concurrent processes could interfere with marker state.
---
Outside diff comments:
In `@src/hooks-session-start/src/main.rs`:
- Around line 163-175: The current duplication check using contains(marker) on
the file content is too broad and will match the marker string anywhere
including in comments or variable names like OLD_CLAUDE_CODE_SESSION_ID,
preventing proper updates of stale session values. Instead of checking if the
marker exists anywhere in the content, check for the exact export line pattern
(the complete export statement with the marker) to properly detect and skip
re-adding the exact same export while allowing old stale session exports to be
updated with new ones.
---
Nitpick comments:
In `@docs/todo10.md`:
- Around line 549-550: The task plan in the TODO list does not explicitly
clarify the required execution order for fixing the test coverage issue. Reorder
the items in the list to ensure that the extension/test_coverage inconsistency
fix (either removing jsonc/json or moving negative test's other_ext_tests)
appears BEFORE the steps to add tests in main.rs and BEFORE the final cargo test
verification. This ensures that the rule_test_coverage_check CI gate will pass
once all preceding fixes are completed, making the task sequence more robust and
easier to follow.
- Around line 512-567: Add technical implementation caveats to the "詰まっている箇所"
section of the design decision to clarify the challenges of transitioning from
file-wide match to comment-only detection. Specifically note that TOML and YAML
use `#` for comments (simple regex), but JSONC and JSON use `//` or `/* */`
(complex regex), and that comment-line-only filtering may require config parser
support rather than regex alone. Additionally, document that the MVP file-wide
match approach carries significant false positive risk for JSONC and JSON
formats due to code comments, embedded diffs, and markdown blocks that could
accidentally trigger the `PR-[0-9]+` pattern, and this risk should inform the
decision to narrow scope after initial false positive observations.
In `@src/hooks-post-tool-linter/src/config.rs`:
- Around line 94-149: The default_ts_pipeline() and default_py_pipeline()
functions have undocumented tool dependencies that will cause runtime failures
if the required tools are not installed. Add documentation comments to both
functions clearly stating the required external tools: biome and oxlint for the
TypeScript pipeline, and ruff for the Python pipeline. These comments should
explain that npx --no-install requires tools to be pre-installed, and optionally
reference that error handling for missing tools is handled in
pipeline_runner.rs.
- Around line 153-158: The config_path() function silently falls back to the
current directory when current_exe() fails, without any notification or logging
to help with debugging. Add explicit logging using eprintln!() or a similar
mechanism to warn the user when current_exe() fails and the function is falling
back to the default path. This will make the configuration file resolution path
more transparent and aid in troubleshooting when config files cannot be found.
In `@src/hooks-post-tool-linter/src/custom_rules/engine.rs`:
- Around line 105-114: The filter condition in the
find_powershell_rules_missing_case_insensitive_flag function currently checks
only for the exact substring "(?i)" using pattern.contains(), which misses
case-insensitive flags combined with other flags like "(?im)" or "(?is)".
Replace the simple substring check with a regex pattern that matches any flag
group starting with "(?", containing the letter 'i' anywhere within that group,
and ending with ")" to properly detect all variations of case-insensitive flags.
This ensures the function correctly identifies PowerShell rules that have
case-insensitive matching enabled regardless of what other flags are present.
- Line 174: The `serde_json::to_string(&violation).ok()` call silently discards
serialization errors without logging them, making debugging difficult. Instead
of using `.ok()`, handle the error case explicitly and add logging similar to
the pattern used in `emit_feedback` from violation.rs. When serialization fails,
log the error before returning None to maintain consistency across the codebase
and aid in troubleshooting.
In `@src/hooks-post-tool-linter/src/pipeline_runner.rs`:
- Line 72: The `run_pipeline_layer` function receives `config: Config` by value,
which can be inefficient if the Config struct is large. Change the parameter in
the `run_pipeline_layer` function signature from `config: Config` to `config:
&Config` to use a reference instead. Then update all usages of `config` within
the function body to work with the reference (adding dereferences where needed),
and update all call sites of `run_pipeline_layer` to pass a reference to the
config argument instead of moving the value. Check that any function calls
within `run_pipeline_layer` that receive config as an argument are compatible
with the reference type.
In `@src/hooks-post-tool-linter/src/violation.rs`:
- Around line 64-66: The code block handling the serialization of output to JSON
currently only handles the successful case with the Ok branch, but silently
ignores any serialization errors when serde_json::to_string(&output) returns
Err. Add an else branch or convert the if let to a match statement to handle the
Err case, and log the error details to stderr using eprintln! or similar error
logging mechanism so that serialization failures are visible to the user instead
of being silently ignored.
🪄 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: f6590c8e-78a5-4c32-ac5f-20fb0de728aa
📒 Files selected for processing (38)
docs/todo-summary.mddocs/todo10.mdsrc/hooks-post-tool-linter/src/config.rssrc/hooks-post-tool-linter/src/custom_rules/coverage.rssrc/hooks-post-tool-linter/src/custom_rules/deployed_tests.rssrc/hooks-post-tool-linter/src/custom_rules/engine.rssrc/hooks-post-tool-linter/src/custom_rules/engine_tests.rssrc/hooks-post-tool-linter/src/custom_rules/mod.rssrc/hooks-post-tool-linter/src/custom_rules/rule_tests.rssrc/hooks-post-tool-linter/src/custom_rules/rule_tests_extras.rssrc/hooks-post-tool-linter/src/custom_rules/types.rssrc/hooks-post-tool-linter/src/file_size_check.rssrc/hooks-post-tool-linter/src/main.rssrc/hooks-post-tool-linter/src/pipeline_runner.rssrc/hooks-post-tool-linter/src/utf8_integrity.rssrc/hooks-post-tool-linter/src/violation.rssrc/hooks-pre-tool-validate/src/blocked_patterns.rssrc/hooks-pre-tool-validate/src/config.rssrc/hooks-pre-tool-validate/src/handlers.rssrc/hooks-pre-tool-validate/src/main.rssrc/hooks-pre-tool-validate/src/presets/basic.rssrc/hooks-pre-tool-validate/src/presets/gh.rssrc/hooks-pre-tool-validate/src/presets/jj.rssrc/hooks-pre-tool-validate/src/presets/mod.rssrc/hooks-pre-tool-validate/src/presets/safety/mod.rssrc/hooks-pre-tool-validate/src/presets/safety/polling_exe.rssrc/hooks-pre-tool-validate/src/presets/safety/powershell.rssrc/hooks-pre-tool-validate/src/presets/safety/secret.rssrc/hooks-pre-tool-validate/src/protected_files.rssrc/hooks-pre-tool-validate/src/todo_staleness.rssrc/hooks-session-start/src/hooks_config.rssrc/hooks-session-start/src/jj_helpers.rssrc/hooks-session-start/src/main.rssrc/hooks-session-start/src/past_time.rssrc/hooks-session-start/src/pr_monitor.rssrc/hooks-session-start/src/reaper.rssrc/hooks-session-start/src/staleness.rssrc/hooks-session-start/src/weekly_review.rs
…llow-up) PR #217 (pr3a-hooks-module-split) の CodeRabbit review で検出された Critical / Major / Minor findings を takt post-pr-review の 3 iter fix で 解消した変更を land。 ## 修正内容 (CR severity 別) ### Critical (1 件 / 採用) - docs/todo10.md: 順位 216 (no-workstream-seq-names-in-config rule) の test_coverage 宣言で拡張子カバレッジ欠落と test 命名不一致を修正 (other_ext_tests に jsonc test を追加、main_ext_tests の test 名を TOML schema 規約と整合) ### Major (3 件 / 採用) - src/hooks-pre-tool-validate/src/todo_staleness.rs: run_jj_with_timeout で child stdout をブロッキング待機していたパイプバッファ枯渇デッド ロックを修正。spawn_stdout_drainer + poll_child_with_deadline 関数を 抽出してバックグラウンド drain に変更 (ADR-016 subprocess safety pattern) - src/hooks-session-start/src/jj_helpers.rs: 同型の deadlock 修正 (spawn_stdout_drainer + poll_child_with_deadline 抽出)。両 module で identical pattern を共有 - src/hooks-session-start/src/reaper.rs: .failed marker の atomic file creation を File::create_new() で保証 (TOCTOU window 解消) ### Minor (2 件 / 採用) - docs/todo10.md: line 532 markdown 引用符の閉じ括弧不一致を修正 - src/check-ci-coderabbit/src/rate_limit.rs + src/hooks-session-start/ src/reaper.rs: parse_iso8601_to_unix で Z suffix を strip_suffix で validate (trim_end_matches では invalid timestamp も accept していた) ### Minor (1 件 / 却下) - src/hooks-pre-tool-validate/src/config.rs: TODO_STALENESS_DEFAULT_BRANCH を "master" → "main" 提案。本リポジトリは master を default branch として運用しているため却下。他 module の "main" 参照は aspirational / transitional な記述で、本 module の "master" が正しい。 ## 検証 - cargo test --workspace: 437 tests pass (本 PR 関連: 71 + 221 + 145、 rate_limit.rs 周辺は別 crate test で cover) - cargo clippy --workspace -- -D warnings: clean - takt post-pr-review: 3 iterations / 39m 32s / approved (structured_output)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks-session-start/Cargo.toml`:
- Line 10: The lib-subprocess dependency in hooks-session-start Cargo.toml is
using a path-based dependency instead of workspace dependency management as
required by ADR-026. Replace the path dependency syntax `{ path =
"../lib-subprocess" }` with `{ workspace = true }` for the lib-subprocess entry
to align with the workspace-based dependency management guidelines.
🪄 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: fccccd84-96c8-4ecf-9540-f3290b2df5e2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
docs/todo10.mdsrc/check-ci-coderabbit/src/rate_limit.rssrc/hooks-pre-tool-validate/Cargo.tomlsrc/hooks-pre-tool-validate/src/todo_staleness.rssrc/hooks-session-start/Cargo.tomlsrc/hooks-session-start/src/jj_helpers.rssrc/hooks-session-start/src/main.rssrc/hooks-session-start/src/pr_monitor.rssrc/hooks-session-start/src/reaper.rssrc/hooks-session-start/src/weekly_review.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/hooks-session-start/src/jj_helpers.rs
- src/hooks-session-start/src/weekly_review.rs
- src/hooks-session-start/src/pr_monitor.rs
- docs/todo10.md
- src/hooks-session-start/src/main.rs
- src/hooks-pre-tool-validate/src/todo_staleness.rs
- src/hooks-session-start/src/reaper.rs
…L530 採用) CR Critical L530 で「Task 216 の test_coverage 宣言に拡張子カバレッジ欠落」 として指摘された不整合を解消: - extensions = ["toml", "yaml", "yml", "jsonc", "json"] のうち plain `json` は comment 構文を持たず本 rule (`no-workstream-seq-names-in-config`) の対象外 = test 未定義状態だった - jsonc が JSON-with-comments を cover するため json は rule scope から除外 - extensions = ["toml", "yaml", "yml", "jsonc"] に縮小 なお同 thread 内で指摘された「`no_workstream_seq_skips_github_pr_number` が main_ext_tests.toml に配置されている」点は coverage.rs の実装 (positive/negative semantic を強制しない、宣言 test 名の存在のみ check) と rule⑫ 既存 pattern との整合により現状維持。CR Major (path → workspace dep) と CR Minor (master → main) は rejection 理由を thread reply で記録済。
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
5 tasks
aloekun
added a commit
that referenced
this pull request
Jun 23, 2026
PR-3a (#217) の post-merge-feedback で採用された 2 件と、調査の結果判明した 800 行超 file 累積問題への改善計画を documents として land。 ## 順位 220/221 (PR #217 post-merge-feedback 採用) - 順位 220 (🔧 Tier 2): subprocess stress test (>64KB stdout) を ADR-031 weekly-review pipeline 経由で週次実行 - ユーザー判断 (2026-06-23): hooks/pre-push には組み込まず週次に分離 - `#[ignore]` 付き cargo test + ADR-031 workflow に rust-stress step 追加 - 順位 221 (💎 Tier 3): ADR-NNN (採番未確定): Safe Subprocess Stdout Pattern を ADR-016 appendix or 新 ADR で codify - 順位 220 (test 層) と 1 PR bundle 推奨 - ADR-025 CwdRestore guard pattern を precedent として cite ## ファイルサイズチェックフロー改善計画 (新規 docs/file-length-enforcement-plan.md) PR-3a 完了時に判明: 800 行超 file が 7 件存在 (lint hook 本体 1606 行を含む)。 現状の file_length lint (順位 147) は soft-nag のみで decision: block しない設計、 Stop hook quality_gate / pre-push quality_gate にも file_length check が無く、 ratchet 累積を防ぐ機構が欠落していた。 ユーザー判断 (2026-06-23): C (Stop hook gate) + E (weekly audit) の二段組を導入、 clean state 到達後に C → B (PostToolUse block) 移行を将来検討。 planning doc として docs/file-length-enforcement-plan.md を新設、6 PR (W0-W5) の作業計画 + 削除条件 (全 PR land + 0 件確認 + dogfood 通過) を明文化。 並列セッションから状況を把握できる ephemeral 計画書 (試験運用、完了で削除)。
5 tasks
aloekun
added a commit
that referenced
this pull request
Jun 24, 2026
…順位 222 採用 (#219) * docs(todo): 順位 222 採用 (PR #218 post-merge-feedback #5) PR #218 (docs PR、ファイルサイズチェックフロー改善計画 + 順位 220/221 採用) の post-merge-feedback で承認された #5 を採用: 順位 222 (💎 Tier 3、Effort XS): `~/.claude/CLAUDE.md` に「複数セッション跨ぎの計画文書作成時は AI が 先走らずユーザー確認後に方針報告し GO/NO-GO を得る」ルール追加 由来: PR #218 session 内で Plan file 作成完了報告後、AI がユーザー承認 なしに PR-W0 着手しようとして `[Request interrupted by user]` で停止 された実観測 (Severity Medium、Frequency Low 初観測、Effort XS、 Adoption Risk None)。memory `feedback_no_unauthorized_reorder` の補強 として「planning doc 作成のような大きな task 完了時は GO/NO-GO 確認待ち」 を明文化、派生プロジェクトへ `~/.claude/CLAUDE.md` 経由で自動波及。 採用しなかった項目: - #1 (weekly audit を feedback entry にも明示): 計画書 PR-W0 で既に管理 - #3 (lib-subprocess stress test): 順位 220 と完全重複 - #4 (Agent template PMF entry): 計画書 Appendix A で既に capture、却下 - #2/#6/#7: 様子見継続 * feat(weekly-review): file_length scan を pre-LLM step として追加 (PR-W0) ADR-031 weekly-review pipeline に deterministic Rust pre-step として 800 行超 file の scan を追加。LLM facet 不要、純機械測定。 順位 147 (file_length lint) は touch-trigger ratchet で「触られた file の 編集時のみ警告」設計のため、未触り state の violation を可視化できない。 本 step は毎週 1 回 master HEAD に対して 800 行超 file を全件列挙し、 aggregate-weekly facet の input に注入して watchlist として report 化する。 PR-3a (PR #217) で 7 件の 800 行超 file が判明した経緯から、Phase 1 (file split work、PR-W1 〜 W4) の進捗 dashboard としても機能する。 全 file ≤ 800 行に到達後も恒久的に監視継続。 由来: docs/file-length-enforcement-plan.md PR-W0 (PR #218 で land)、 severity = warning (block しない、健康診断目的)。
Merged
7 tasks
aloekun
added a commit
that referenced
this pull request
Jun 24, 2026
…PR-W1、self-host irony 解消) (#220) * docs(plan): PR-W0 を [x] #219 (merged at 2026-06-24T16:07:42Z) に更新 PR #219 (PR-W0、weekly-review に file_length scan facet 追加) が 2026-06-24T16:07:42Z に master へ land したことを受けて、 docs/file-length-enforcement-plan.md の進捗追跡 table の PR-W0 status を `[in progress]` → `[x] #219` に更新する。 * refactor(hooks-post-tool-comment-lint-rust): main.rs を module 分割 (PR-W1) docs/file-length-enforcement-plan.md PR-W1 を実装。 lint hook 本体 (1606 行) を coding-style.md § File Organization (800 行 max) 内に収まる module 構成に分割。behavior 不変な mechanical refactor で 関数 signature・公開 API・field 名・default 値はすべて保持。 順位 147 (file_length lint、PR #202 land) を自分自身に適用した self-host の整合性を確立。本 PR が land すれば 7 files 中 1 件 (1606 行) を 800 行 以下に解消、weekly-review file_length watchlist の件数が 7 → 6 に減少。 分割計画は計画書 PR-W1 section + Appendix A Agent prompt template 参照。 PR-3a (#217) の hooks-session-start 分割と同型 procedure を Agent 委譲で 実装、behavior 不変性は test count 不変 + cargo clippy clean で verify。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR-3 (layered config refactor) の前置 PR。3 hook の main.rs を
coding-style.md § File Organization(800 行 max) 内に収まる module 構成に分割。behavior 不変 な mechanical refactor で関数 signature・公開 API・field 名・default 値はすべて保持。順位 147 (file_length lint、PR #202 で land) が touch-trigger ratchet 違反として検出した 3 hook の触れない技術債務を解消し、PR-3b (layered config) を clean state で進められるようにする。
分割結果
hooks-session-startreaper.rs)hooks-pre-tool-validatetodo_staleness.rs)hooks-post-tool-linterrule_tests.rs)全 file ≤ 575 行 (800 行制限の 72% 以下)。tests 計 437 件、変動なし。
分割方針
hooks-session-start
reaper.rs— TaktMeta + OrphanRun + orphan scan / mark / reap (ADR-030 §L2)pr_monitor.rs— ParkedStatePartial + catch-up nudgestaleness.rs— working copy staleness 検出 (順位 136 案 A)weekly_review.rs— ADR-031 Phase C reminderhooks_config.rs— Config 構造体 + load_configjj_helpers.rs— run_jj_with_timeout / count_commits_in_revset / fetch_head_is_recentmain.rs— entry + dispatch (339 行)hooks-pre-tool-validate
presets/{basic,jj,gh}.rs+presets/safety/{polling_exe,powershell,secret}.rs— 13 BlockedPattern preset 関数blocked_patterns.rs— BlockedPattern 構造体 + validate_command + build_blocked_patternsprotected_files.rs— PROTECTED_CONFIG_FILES + is_protected_configtodo_staleness.rs— TodoStalenessConfig + check_todo_staleness + jj log helpershandlers.rs— Bash/Edit/Write/PowerShell handlersconfig.rs/main.rs— entry + dispatchhooks-post-tool-linter
custom_rules/{engine,types,coverage,deployed_tests,engine_tests,rule_tests,rule_tests_extras}.rs— regex rule engine + per-rule positive/negative testspipeline_runner.rs— lint pipeline 実行 (Biome / oxlint / ruff / markdownlint)file_size_check.rs— 50KB threshold checkutf8_integrity.rs— UTF-8 整合性 checkviolation.rs— Violation 構造体 + emit_feedbackconfig.rs/main.rs— entry + dispatchbehavior 不変性の保証
pub(crate)付与のみ、export 経路は同等cargo test --workspace: 全 crate pass (本 PR 関連の 437 件含む)cargo clippy --workspace -- -D warnings: cleancargo fmt: clean非機能的な変更点
// foo形式の非 doc コメント (見出し / inline rationale / セクション banner 等) を削除。comment-lint-rusthook の Bundle Z #B-α rule に従い識別子名 / 関数名 / doc comment に意図を集約。Pre-existing の grandfather 状態だったが、Write tool で新規 file 作成すると新規コメント扱いで block されるため、移動時に整理した。unique_temp_root/write_meta/parked_state/patterns_with_presets/is_blocked/build_todo_path/make_test_rule/compile_test_rules/write_file等の test helper は memoryfeedback_test_dry_antipatternに従い各 test module に独立コピー (合計 ~4 重複)。共有 test util module の抽出は anti-pattern。format!("{}{}", "AKIA", "...")形式を採用 (literal AWS / GitHub / OpenAI key を Edit/Write tool 経由で書き込むと secret-detection hook (順位 146) 自身が発火するため、test fixture を dogfood 経由で書く必要があり)。runtime 上は regex match と等価。rule_test_coverage_check適応: 元実装はsrc/main.rs1 file の test 関数名を抽出していたが、split 後は test が複数 module に分散するためsrc/**/*.rsを recursive walk するように rewrite。TOMLmain_ext_tests/other_ext_testsの宣言と実 test 関数名の整合性 check は引き続き機能。extract_existing_test_fn_namesの false-green guard: 関数自身がcoverage.rsに居るため、coverage check が間違って空 set を返した場合に自分自身を含まない結果になる guard は維持。pr_size_check override の理由
本 PR の jj diff は 15636 行 (= 削除 ~8000 行 + 追加 ~8000 行) で push-runner の
pr_size_checkblock_threshold 1500 を超過したが、これは「mechanical refactor で同じ code を別 file に移動」した結果で、pr_size_checkの想定する「実装 work が大きすぎる」case ではない。PR_SIZE_CHECK_OVERRIDE=1で override (= 順位 151 の override 想定 use case「大型 refactoring 等で意図的な場合」)。case A2 (hook 単位 3 PR に分割) も検討したが、3 hook 同時 split で workspace-wide 整合性を確認できるメリットを優先して 1 PR で進めた (ユーザー判断、2026-06-23)。
Test plan
cargo build --workspace: SUCCESScargo test --workspace: 全 crate pass (本 PR 関連 437 件含む)cargo clippy --workspace -- -D warnings: cleancargo fmt: cleanrule_test_coverage_checkpass (post-tool-linter の TOML meta field 整合性、split 対応 rewrite 後)PR 計画における位置
PR-1 (順位 147/151/212-214 削除 + weekly enable、merged #216) → 本 PR (PR-3a) → PR-3b (layered config + lib-hooks-config + ADR-039 amendment) → PR-2 (Stop hook todo cleanup check)
PR-3a の land 後に PR-3b で
[features].enabledallow-list pattern に移行し、各 hook のenabled: Option<bool>field を削除する。本 PR で clean state を確立したことで、PR-3b の diff は logical な refactor のみに focus できる。Summary by CodeRabbit
リリースノート