refactor: src/ フォルダ命名を役割ベースに整理 (ADR-012) - #26
Conversation
📝 WalkthroughWalkthroughリポジトリ内でフック実行バイナリと共通ライブラリの命名規約を導入・適用し、 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/adr-003-hooks-layout-and-build-strategy.md`:
- Around line 59-63: The doc lists non-existent npm scripts
(`build:all-pre-tool-validate`, `build:all-post-tool-linter`,
`build:all-stop-quality`) which will fail; update the commands to match the
actual script names (`build:hooks-...` variants and `build:all`) used in
package.json and change the example command pattern line to the real invocation
format (`cd .claude/<dir> && cargo build --release && cp
target/release/<name>.exe ../<name>.exe`) so the section and the additional
steps (also around lines 90–94) consistently reference `build:hooks-*` and
`build:all` instead of the nonexistent `build:all-*` names.
In `@docs/adr/adr-005-hooks-path-resolution-with-template.md`:
- Line 40: The document still references the old npm script name `build:hooks`
on Line 58; update that mention to `build:all` so the text correctly matches the
package.json command names (e.g., change the sentence that currently reads
"`build:hooks` の末尾で自動実行されるため..." to use `build:all`), ensuring all occurrences
referring to the script that triggers the exe build and settings.local.json
generation are consistent with `build:all`.
In `@docs/adr/adr-010-hooks-layout-and-build-strategy-v2.md`:
- Around line 79-80: The ADR currently documents a non-existent script name
`pnpm build:all-<機能名>`; update the text to match the actual package.json scripts
by replacing references to `build:all-<機能名>` with the per-folder script format
`pnpm build:<フォルダ名>` and keep `pnpm build:all` for the aggregate build; ensure
every occurrence (including the repeated section around the later block that
currently lists `build:all-<機能名>`) is updated so examples, commands, and naming
conventions in the ADR align with the real scripts `build:<フォルダ名>` and
`build:all`.
🪄 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: cfab71ee-1d6f-4acd-a153-32f3f015fce8
⛔ Files ignored due to path filters (4)
src/check-ci-coderabbit/Cargo.lockis excluded by!**/*.locksrc/cli-pr-monitor/Cargo.lockis excluded by!**/*.locksrc/cli-push-pipeline/Cargo.lockis excluded by!**/*.locksrc/lib-report-formatter/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude/hooks-config.toml.claude/settings.local.json.template.gitignoreCLAUDE.mddocs/adr/adr-001-hooks-implementation-language.mddocs/adr/adr-003-hooks-layout-and-build-strategy.mddocs/adr/adr-005-hooks-path-resolution-with-template.mddocs/adr/adr-006-config-driven-hooks.mddocs/adr/adr-008-push-pipeline-harness.mddocs/adr/adr-009-post-pr-monitor.mddocs/adr/adr-010-hooks-layout-and-build-strategy-v2.mddocs/adr/adr-012-src-naming-convention.mddocs/todo.mdpackage.jsonscripts/deploy-hooks.tssrc/check-ci-coderabbit/Cargo.tomlsrc/check-ci-coderabbit/src/main.rssrc/cli-pr-monitor/Cargo.tomlsrc/cli-pr-monitor/src/main.rssrc/cli-push-pipeline/Cargo.tomlsrc/cli-push-pipeline/src/main.rssrc/hooks-session-start/src/main.rssrc/lib-report-formatter/Cargo.tomlsrc/lib-report-formatter/src/lib.rs
- hooks-push-pipeline → cli-push-pipeline (CLIツール) - hooks-post-pr-monitor → cli-pr-monitor (CLIツール) - hooks-report-formatter → lib-report-formatter (共有ライブラリ) - build:hooks → build:all (スクリプト名) - .gitignore を個別列挙から glob パターン (.claude/*.exe, src/*/target/) に変更 - ADR-012: src/ ディレクトリの命名規約を追加 Fix: ADR-003/005/010 の build:all-* 誤変換を修正 (CodeRabbit review) Fix: cli-pr-monitor の drain_pipe MAX_LINES=40 制限を撤廃 (JSON パースエラー対策) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cc5bbe1 to
c27442c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli-pr-monitor/src/main.rs (1)
168-180: 無制限の出力読み取りに関する軽微なメモリリスク
read_to_stringは出力全体をメモリに読み込むため、サブプロセスが大量の出力を生成した場合にメモリ問題を引き起こす可能性があります。現状の使用コンテキスト(
check-ci-coderabbit.exeの JSON 出力やghコマンド)では出力サイズが制限されており、タイムアウト(60秒)も設定されているため実際のリスクは低いです。コメントで理由が説明されている点も良いです。将来的に別のコマンドで再利用する場合は、出力サイズの上限チェックを検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/main.rs` around lines 168 - 180, The drain_pipe function reads the entire stream into memory via reader.read_to_string, which can OOM if a subprocess emits huge output; modify drain_pipe (the spawned closure) to enforce a maximum read size by wrapping the reader in std::io::Read::take (or read into a buffer and stop after a configurable MAX_BYTES), then return the truncated string (and optionally append a marker like "...(truncated)"). Keep function signature and behavior otherwise the same, and make MAX_BYTES a constant or configurable value so future callers (e.g., check-ci-coderabbit/gh) can adjust limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli-pr-monitor/src/main.rs`:
- Around line 168-180: The drain_pipe function reads the entire stream into
memory via reader.read_to_string, which can OOM if a subprocess emits huge
output; modify drain_pipe (the spawned closure) to enforce a maximum read size
by wrapping the reader in std::io::Read::take (or read into a buffer and stop
after a configurable MAX_BYTES), then return the truncated string (and
optionally append a marker like "...(truncated)"). Keep function signature and
behavior otherwise the same, and make MAX_BYTES a constant or configurable value
so future callers (e.g., check-ci-coderabbit/gh) can adjust limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0e1234c-87dc-4653-a87d-8d15b994e81a
⛔ Files ignored due to path filters (4)
src/check-ci-coderabbit/Cargo.lockis excluded by!**/*.locksrc/cli-pr-monitor/Cargo.lockis excluded by!**/*.locksrc/cli-push-pipeline/Cargo.lockis excluded by!**/*.locksrc/lib-report-formatter/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude/hooks-config.toml.claude/settings.local.json.template.gitignoreCLAUDE.mddocs/adr/adr-001-hooks-implementation-language.mddocs/adr/adr-003-hooks-layout-and-build-strategy.mddocs/adr/adr-005-hooks-path-resolution-with-template.mddocs/adr/adr-006-config-driven-hooks.mddocs/adr/adr-008-push-pipeline-harness.mddocs/adr/adr-009-post-pr-monitor.mddocs/adr/adr-010-hooks-layout-and-build-strategy-v2.mddocs/adr/adr-012-src-naming-convention.mddocs/todo.mdpackage.jsonscripts/deploy-hooks.tssrc/check-ci-coderabbit/Cargo.tomlsrc/check-ci-coderabbit/src/main.rssrc/cli-pr-monitor/Cargo.tomlsrc/cli-pr-monitor/src/main.rssrc/cli-push-pipeline/Cargo.tomlsrc/cli-push-pipeline/src/main.rssrc/hooks-session-start/src/main.rssrc/lib-report-formatter/Cargo.tomlsrc/lib-report-formatter/src/lib.rs
✅ Files skipped from review due to trivial changes (13)
- .claude/hooks-config.toml
- docs/adr/adr-001-hooks-implementation-language.md
- src/hooks-session-start/src/main.rs
- src/check-ci-coderabbit/src/main.rs
- CLAUDE.md
- .claude/settings.local.json.template
- docs/todo.md
- docs/adr/adr-006-config-driven-hooks.md
- src/lib-report-formatter/Cargo.toml
- docs/adr/adr-012-src-naming-convention.md
- docs/adr/adr-005-hooks-path-resolution-with-template.md
- docs/adr/adr-003-hooks-layout-and-build-strategy.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cli-push-pipeline/Cargo.toml
- src/cli-pr-monitor/Cargo.toml
- src/check-ci-coderabbit/Cargo.toml
- docs/adr/adr-008-push-pipeline-harness.md
- docs/adr/adr-010-hooks-layout-and-build-strategy-v2.md
- docs/adr/adr-009-post-pr-monitor.md
- package.json
Summary
hooks-push-pipeline→cli-push-pipeline、hooks-post-pr-monitor→cli-pr-monitor、hooks-report-formatter→lib-report-formatterにリネームhooks-/cli-/lib-)で、コーディング AI がsrc/一覧だけで各クレートの役割を判別可能にbuild:hooks→build:allにスクリプト名を変更(hooks 以外も含むため).gitignoreを個別列挙から glob パターン(.claude/*.exe,src/*/target/)に簡素化Test plan
pnpm pushパイプライン通過(テスト + AI レビュー + push)pnpm build:allで全 exe がビルドできることpnpm deploy:hooksで派生プロジェクトに新名 exe が配布されること🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
Chores
pnpm build:hooksからpnpm build:allに変更hooks-*からcli-*バイナリ参照へ切替(コピー・実行コマンド更新)Documentation