refactor: シェルスクリプトの技術的負債を解消 - #214
Conversation
コードベースの静的解析で特定された技術的負債を解消するリファクタリング。 ## 変更内容 ### High Priority - #206: import.sh/export.sh の重複コードを lib/config.sh に統合 - export.sh: 105行 → 66行 (37%削減) - import.sh: 130行 → 105行 (19%削減) - #207: setup-claude.sh を関数分割して lib/claude_plugins.sh に抽出 - setup-claude.sh: 352行 → 101行 (71%削減) - 新規 lib/claude_plugins.sh: 332行 (再利用可能なライブラリ) ### Medium Priority - #208: lib/output.sh と lib/errors.sh を統合 - 2ファイル → 1ファイル (output.sh: 149行) - errors:: エイリアスで後方互換性を維持 - #209: マーケットプレイスURLのハードコード重複を解消 - known_marketplaces.json.template と同期 - URL形式のマーケットプレイスにも対応 - #210: setup-env.sh を credentials.sh のラッパーに変更 - setup-env.sh: 127行 → 51行 (60%削減) ### Low Priority - #211: シェルスクリプトのシバン統一 - #!/bin/bash → #!/usr/bin/env bash ## 成果 - 差引349行削減 - コード重複率: 25% → <5% - ファイル当たりの最大行数: 352行 → 149行 - テスト可能性の向上 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR refactors shell scripts to improve modularity and maintainability. It consolidates config handling (import/export) into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (7)
script/lib/output.sh (1)
142-149: Good backward compatibility approach.The
errors::namespace aliases ensure existing scripts using the old errors.sh API continue to work without modification. Consider adding a deprecation notice in comments for future cleanup.script/setup-claude.sh (1)
74-99: Consider consolidating with plugins::sync_repo_content.
verify_important_commandspartially duplicates the sync logic fromplugins::sync_repo_content. Since sync is called before verify, the important commands should already be in place. This function's re-copy logic is defensive but adds redundancy.Consider moving the "important commands" list to the plugin library or relying solely on the sync function. However, keeping this as a verification step is acceptable for ensuring critical files are present.
script/lib/claude_plugins.sh (5)
78-84: Silent failure may hide real errors.The
|| trueon line 80 suppresses all copy failures, including permission errors or disk full conditions. Unlike the pattern-based branch (lines 68-72) which logs individual failures, this branch silently ignores errors and then reports a file count that may not reflect what was actually copied.Consider logging warnings for copy failures here as well:
🔎 Proposed improvement
else # パターン指定なし：ディレクトリ全体をコピー - cp -r "${source_dir}"/* "${target_dir}/" 2>/dev/null || true + if ! cp -r "${source_dir}"/* "${target_dir}/" 2>/dev/null; then + log_warn " ${name}のコピー中にエラーが発生しました" + fi local count count=$(find "$target_dir" -maxdepth 1 -type f 2>/dev/null | wc -l) log_success "${name}を同期しました: ${count} ファイル" fi
160-166: Command failure may not always mean "already added".Lines 161, 164 assume that any failure from
claude plugin marketplace addindicates the marketplace is already added. However, failures could also occur due to network issues, invalid URLs, or authentication problems. This could lead to misleading log messages.Consider checking the actual error output to distinguish between "already added" and other failures, similar to how
plugins::install_from_manifesthandles it (lines 221-228).
248-254: Minor: Redundant directory name check.Line 250's condition
[[ "$cache_dir" == *"/hookify" ]]is redundant sincefindalready filters with-name "hookify". The-d "$cache_dir"check is also unnecessary as-type densures only directories are returned.🔎 Simplified version
if [[ -d "${claude_dir}/plugins/cache" ]]; then while IFS= read -r -d '' cache_dir; do - if [[ -d "$cache_dir" ]] && [[ "$cache_dir" == *"/hookify" ]]; then - hookify_paths+=("$cache_dir") - fi + hookify_paths+=("$cache_dir") done < <(find "${claude_dir}/plugins/cache" -type d -name "hookify" -print0 2>/dev/null) fi
303-317: Potential file permission loss when prepending shebang.When prepending a shebang (lines 310-314),
mktempcreates a file with restrictive permissions (typically0600). The subsequentmvreplaces the original file, andchmod +xonly adds execute bits. Original permissions like group/other read access will be lost.🔎 Proposed fix to preserve permissions
plugins::_fix_shebang() { local py_file="$1" if head -n1 "$py_file" | grep -q "^#!"; then perl -i -pe 's|^#!.*python.*|#!/usr/bin/env python3|' "$py_file" else local tmp_file tmp_file=$(mktemp) + # Preserve original file permissions + chmod --reference="$py_file" "$tmp_file" 2>/dev/null || true echo '#!/usr/bin/env python3' > "$tmp_file" cat "$py_file" >> "$tmp_file" mv "$tmp_file" "$py_file" fi chmod +x "$py_file" }
1-8: Consider documenting or validating the dependency onoutput.sh.This library uses
log_info,log_success, andlog_warnfunctions (fromoutput.sh) but doesn't source that file directly. While this is acceptable for a library meant to be sourced by other scripts, consider adding a comment documenting this dependency or a runtime check.🔎 Optional: Add dependency documentation
#!/usr/bin/env bash # ============================================================================ # Claude Code Plugin Management Library # プラグイン管理の共通機能を提供 +# +# Dependencies: +# - lib/output.sh (provides log_info, log_success, log_warn) # ============================================================================
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
script/commit_changes.shscript/export.shscript/import.shscript/lib/claude_plugins.shscript/lib/errors.shscript/lib/output.shscript/setup-claude.shscript/setup-env.shscript/version.shtest/integration/lib_functions.bats
💤 Files with no reviewable changes (1)
- script/lib/errors.sh
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-01T03:45:17.253Z
Learnt from: CR
Repo: keito4/config PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T03:45:17.253Z
Learning: Applies to .github/workflows/docker-image.yml : Build DevContainer images automatically with semantic versioning and multi-platform support in .github/workflows/docker-image.yml
Applied to files:
script/version.sh
📚 Learning: 2025-12-01T03:45:17.253Z
Learnt from: CR
Repo: keito4/config PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T03:45:17.253Z
Learning: Publish DevContainer images to ghcr.io/keito4/config-base with semantic versioning
Applied to files:
script/version.sh
📚 Learning: 2025-12-01T03:45:17.253Z
Learnt from: CR
Repo: keito4/config PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T03:45:17.253Z
Learning: Applies to .github/workflows/claude.yml : Trigger automatic AI assistance on claude mentions in issues, PRs, and comments using .github/workflows/claude.yml
Applied to files:
script/setup-claude.sh
📚 Learning: 2025-12-09T08:39:14.049Z
Learnt from: CR
Repo: keito4/config PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T08:39:14.049Z
Learning: Follow development quality standards defined in `CLAUDE.md` when using Claude Code for development assistance
Applied to files:
script/setup-claude.shscript/import.shscript/export.sh
🧬 Code graph analysis (6)
script/setup-claude.sh (2)
script/lib/output.sh (3)
log_info(138-138)log_warn(140-140)log_success(139-139)script/lib/claude_plugins.sh (5)
plugins::sync_repo_content(23-41)plugins::copy_config_files(89-116)plugins::detect_and_add_marketplaces(120-147)plugins::install_from_manifest(193-233)plugins::apply_hookify_patch(237-272)
script/import.sh (1)
script/lib/config.sh (1)
config::import_claude(12-50)
script/lib/claude_plugins.sh (1)
script/lib/output.sh (3)
log_info(138-138)log_warn(140-140)log_success(139-139)
script/export.sh (1)
script/lib/config.sh (3)
config::filter_gitconfig(95-107)config::filter_credentials(110-123)config::export_claude(53-92)
test/integration/lib_functions.bats (1)
script/lib/output.sh (7)
output::require_command(83-94)output::info(49-52)output::warning(44-47)output::success(39-42)errors::require_command(147-147)errors::info(145-145)errors::success(146-146)
script/setup-env.sh (1)
script/lib/output.sh (3)
print_info(137-137)print_success(135-135)print_error(134-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (29)
script/version.sh (1)
1-1: LGTM!Shebang standardization to
#!/usr/bin/env bashimproves portability across different Unix-like systems, aligning with PR objective #211.script/commit_changes.sh (1)
1-1: LGTM!Shebang standardization consistent with the PR-wide portability improvement.
script/export.sh (4)
7-7: Good consolidation of config handling.Sourcing
lib/config.shcentralizes the configuration management logic, reducing code duplication as intended by PR objective #206.
33-35: LGTM!Delegation to
config::filter_gitconfigremoves duplicate sed logic and improves maintainability.
46-48: LGTM!Using
config::filter_credentialscentralizes the credential filtering pattern, eliminating duplication with import.sh.
64-66: LGTM!Delegation to
config::export_claudeconsolidates Claude config export logic into the shared library.script/lib/output.sh (3)
2-9: Good documentation of the consolidation.Clear header explaining the merged functionality from errors.sh improves maintainability.
75-80: LGTM!
output::fatalcorrectly outputs to stderr and exits with status 1, providing consistent fatal error handling.
82-94: LGTM!
output::require_commandproperly validates command existence with optional install hint, providing helpful error messages.test/integration/lib_functions.bats (4)
3-3: LGTM!Test file description updated to reflect the consolidated library testing scope.
39-66: LGTM!Function existence tests properly verify the new
output::namespace functions.
68-79: Good backward compatibility coverage.Testing the
errors::aliases ensures the migration doesn't break existing scripts.
167-181: LGTM!Runtime tests for
errors::aliases verify actual function behavior, not just existence.script/import.sh (2)
8-8: Good consolidation.Sourcing
lib/config.shenables use of centralized config helpers.
88-91: LGTM!Delegation to
config::import_claudeeliminates duplicate Claude config handling between import.sh and export.sh, achieving the PR objective #206 goal.script/setup-env.sh (2)
2-7: Good documentation of the wrapper pattern.Clear header explains the backward compatibility purpose while recommending direct credentials.sh usage for new code.
18-49: LGTM!Verified that credentials.sh exists and properly supports the
fetchsubcommand (line 103). Clean delegation with solid error handling, user-friendly guidance, and correct chmod 600 for credential files.script/setup-claude.sh (4)
19-24: Good modularization.Sourcing the dedicated plugin library enables the significant code reduction (352 → 101 lines) targeted by PR objective #207.
37-39: Good defensive coding for cross-device operations.Setting TMPDIR to a subdirectory of CLAUDE_DIR prevents cross-device link errors that can occur when /tmp is on a different filesystem.
41-45: LGTM!Early exit with informative warning when Claude CLI is missing prevents confusing errors later in the script.
47-69: Clean orchestration flow.The main function now clearly shows the setup sequence: sync → verify → copy config → detect marketplaces → install plugins → apply patches. This is much more readable than the previous inline implementation.
script/lib/claude_plugins.sh (8)
1-19: Well-structured library header and constants.The shebang standardization to
#!/usr/bin/env bashaligns with PR objective #211, and theset -euo pipefailensures strict error handling. The fallback marketplace array with clear format documentation is a good pattern for maintainability.
23-41: LGTM!Clean orchestrator function with proper parameter validation using
${1:?...}syntax. The delegation to_sync_directorywith appropriate parameters for each content type is well-organized.
89-116: LGTM!Clean implementation for config file management. The separation between template and generated file with
sedsubstitution for{{HOME}}is a good pattern. Proper logging for each operation provides good visibility.
120-147: LGTM!Good use of associative array for deduplication of marketplace names. The regex
@([^[:space:]]+)correctly extracts marketplace identifiers, and the file reading pattern with|| [[ -n "$line" ]]properly handles files without trailing newlines.
193-233: LGTM!Robust plugin installation with proper error handling. The distinction between success, already-installed (skipped), and actual failures provides good user feedback. The summary at the end is helpful for quick status assessment.
274-281: LGTM!The Perl regex substitutions correctly patch the import statements. Using
find -execwithperl -i -peis an effective approach for batch processing.
283-301: LGTM!Proper handling of glob patterns with the
[[ -f "$py_file" ]] || continueguard, which correctly handles the case when no.pyfiles exist. The separation ofhooks/directory and root directory processing is clear.
319-334: LGTM!Clean idempotent function that only creates
__init__.pywhen missing. The quoted heredoc'INIT_EOF'correctly prevents variable expansion, and the generated content is appropriate for a Python package initializer.
PR Review: シェルスクリプトの技術的負債を解消総合評価このリファクタリングは、コードベースの保守性を大幅に向上させる優れた取り組みです。349行の削減とコード重複率の25%→5%への改善は素晴らしい成果です。 ✅ 良い点1. 優れた構造化と関数分割
2. 統合による一貫性の向上
3. DRY原則の徹底
4. シバン統一（
|
|
🎉 This PR is included in version 1.26.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
コードベースの静的解析で特定された技術的負債を解消するリファクタリング。
High Priority
Medium Priority
Low Priority
Results
Test plan
Closes #206, #207, #208, #209, #210, #211
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.