feat: 品質ツールの包括的な改善 - #555
Conversation
## 変更内容 ### Hooks改善 - pre_git_quality_gates.py にリトライロジックを追加 - ネットワークエラー・一時的エラーに対するリトライ機能 - Biome と ESLint/Prettier の競合検出警告を追加 ### セキュリティスキャン強化 - AI サービスキー検出パターン追加（Anthropic, OpenAI, Gemini等） - プラットフォームキー検出追加（Slack, Stripe, Supabase, Vercel, Linear等） - Bearer Token, Basic Auth パターン追加 - 重大度判定の改善 ### ドキュメント修正 - Git署名鍵設定を正しい形式に修正（ファイルパスを使用） - README.md, SECURITY.md, credentials/README.md を更新 ### テスト追加 - security-scripts.bats: セキュリティスクリプトの統合テスト - test/python/test_hooks.py: Python Hooks の基本テスト ### その他 - npm パッケージ更新 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
🔍 AI Code Review (Local Hook)Models: Codex (default) / Gemini (default) 🤖 Codex Review指摘事項（重要度順）
判定: patch is incorrect ✨ Gemini ReviewMCP server 'supabase' requires authentication using: /mcp auth supabaseMCP server 'vercel' requires authentication using: /mcp auth vercelレビューの結果、以下の問題点を指摘します。 指摘事項
全体的な正確性の判定
理由: 主要な新機能であるリンター競合検出ロジックに、パッケージ名のタイポという単純なバグが含まれており、この機能が意図通りに動作しません。リトライ処理の導入やドキュメントの改善は適切ですが、中核的な機能不全があるため、このパッチは不正確です。 信頼度: 1.0
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29b75a7584
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [[ "$pattern_name" == *"AWS"* ]] || [[ "$pattern_name" == *"GitHub Token"* ]] || [[ "$pattern_name" == *"Private Key"* ]]; then | ||
| # Critical patterns: cloud credentials, API keys that provide full access | ||
| if [[ "$pattern_name" == *"AWS"* ]] || \ | ||
| [[ "$pattern_name" == *"GitHub Token"* ]] || \ |
There was a problem hiding this comment.
The strict-mode gate classifies only names matching *GitHub Token* as critical, so the newly added patterns (GitHub OAuth, GitHub App Token, GitHub Server-to-Server, GitHub Refresh Token) are downgraded to WARNING. In --strict runs this means leaked gho_/ghu_/ghs_/ghr_ credentials do not fail the scan (exit code stays 0), which weakens the commit/push protection this script is intended to enforce.
Useful? React with 👍 / 👎.
## 修正内容 ### pre_git_quality_gates.py - タイムアウト時のリトライを無効化（最大待ち時間の回帰を防止） - タイムアウトは即座に失敗として扱う（5分→15分の待ち時間増加を回避） ### security-credential-scan.sh - Gemini API Key パターンを削除（Google API Keyと重複） - Vercel Token パターンを削除（24桁英数字は誤検知が多すぎる） 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.claude/hooks/pre_git_quality_gates.py (1)
17-20: Consider making timeout and retry settings configurable via environment variables.The hardcoded values are reasonable defaults, but some environments may need longer timeouts (slow CI) or different retry strategies.
♻️ Optional: Environment variable overrides
# 設定 -DEFAULT_TIMEOUT = 300 # 5分 -MAX_RETRIES = 2 # 最大リトライ回数 -RETRY_DELAY = 2 # リトライ間隔（秒） +DEFAULT_TIMEOUT = int(os.environ.get("QUALITY_GATES_TIMEOUT", 300)) # 5分 +MAX_RETRIES = int(os.environ.get("QUALITY_GATES_MAX_RETRIES", 2)) # 最大リトライ回数 +RETRY_DELAY = int(os.environ.get("QUALITY_GATES_RETRY_DELAY", 2)) # リトライ間隔（秒）🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/pre_git_quality_gates.py around lines 17 - 20, Replace hardcoded DEFAULT_TIMEOUT, MAX_RETRIES, and RETRY_DELAY with values read from environment variables (e.g., CLAUDE_TIMEOUT, CLAUDE_MAX_RETRIES, CLAUDE_RETRY_DELAY) while preserving the current defaults; in the module where DEFAULT_TIMEOUT, MAX_RETRIES, and RETRY_DELAY are defined, read os.environ.get for each, attempt safe int conversion with fallback to the existing constants, and handle invalid values by falling back and optionally logging a warning; ensure any code using DEFAULT_TIMEOUT, MAX_RETRIES, and RETRY_DELAY continues to reference these symbols so behavior is unchanged when env vars are absent.script/security-credential-scan.sh (1)
103-103: Supabase Key pattern matches generic JWTs.The pattern
eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9\\.matches the standard JWT header for HS256 tokens, which is used by many services beyond Supabase. This may generate findings labeled "Supabase Key" for unrelated JWTs.Consider renaming to "JWT (HS256)" or adding context-based detection (e.g., checking for
supabasein the surrounding text).♻️ Suggested: Rename for accuracy
-PATTERNS["Supabase Key"]="eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9\\.[a-zA-Z0-9_-]+\\.[a-zA-Z0-9_-]+" +# Generic HS256 JWT - commonly used by Supabase, Auth0, and other services +PATTERNS["JWT Token (HS256)"]="eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9\\.[a-zA-Z0-9_-]+\\.[a-zA-Z0-9_-]+"Note: You already have a generic "JWT Token" pattern on line 110. Consider whether both are needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/security-credential-scan.sh` at line 103, The "Supabase Key" regex stored in PATTERNS["Supabase Key"] is too generic (it just matches HS256 JWT headers) so update detection to avoid false positives: either rename the key to "JWT (HS256)" or enhance the pattern to require Supabase context (e.g., lookaround or require the word "supabase" near the token), and if you keep both patterns reconcile with the existing "JWT Token" entry (remove or differentiate one) so you don't duplicate generic JWT detection; update PATTERNS["Supabase Key"] accordingly and adjust any tests or references that rely on that key name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/hooks/pre_git_quality_gates.py:
- Around line 158-175: The function is_retryable_error currently declares an
unused returncode parameter; remove that parameter from the function signature
(change def is_retryable_error(stderr: str) -> bool) and update every call site
that passes a return code to instead call is_retryable_error(stderr) so the
function only inspects stderr; ensure you update import/type hints if any and
run tests to confirm all callers compile.
In `@script/security-credential-scan.sh`:
- Line 97: PATTERNS["Gemini API Key"] duplicates PATTERNS["Google API Key"]
(both use `AIza[0-9A-Za-z\\-_]{35}`); remove the duplicate entry or consolidate
it under the Google pattern by deleting the PATTERNS["Gemini API Key"] line, or
if you must keep a separate Gemini entry, replace the Gemini regex with the
correct distinctive pattern or add a clear comment above PATTERNS["Gemini API
Key"] indicating it intentionally reuses the Google regex to avoid duplicate
findings; update any test cases or documentation referencing the Gemini key
accordingly.
- Line 104: The PATTERNS["Vercel Token"] entry is too generic and causes false
positives; update the PATTERNS["Vercel Token"] regex to a stricter form (for
example require a known prefix like "vercel_" or other canonical Vercel token
structure) or remove the pattern and add a comment documenting the limitation;
change the value assigned to PATTERNS["Vercel Token"] (noting the symbol name in
the diff) to a tighter regex that includes the prefix and appropriate
length/charset, or delete the entry and add a brief comment explaining why a
reliable regex could not be determined.
In `@test/integration/security-scripts.bats`:
- Around line 65-66: The test currently writes a literal-looking AWS key into
the repo (echo 'const key = "AKIAIOSFODNN7EXAMPLE";' > "$temp_dir/test.js"),
which triggers the credential scanner; instead, compose the fake key at runtime
from non-matching fragments (e.g. concatenate prefix "AKIA" with other
substrings) and write that assembled string into "$temp_dir/test.js" so no
literal AKIA... token appears in source; apply the same change to the other
occurrence around lines 77-78 in the same test file and ensure the assembled key
still matches the intended test value when evaluated by the test harness.
In `@test/python/test_hooks.py`:
- Around line 33-45: The test currently just greps text in test_hooks.py and
must be replaced with real verification: parse pre_git_quality_gates.py with the
AST module to assert the presence of real top-level function definitions (e.g.,
detect_package_manager, has_biome, get_package_scripts, run_with_retry,
detect_linter_conflicts) rather than string matches, and add behavioral unit
tests that import the module and execute the new logic paths for run_with_retry
and detect_linter_conflicts (mocking/simulating failures and conflicting linter
configs) to ensure retry behavior and conflict detection run as expected; update
test names and assertions to validate actual API signatures and outcomes instead
of text presence.
---
Nitpick comments:
In @.claude/hooks/pre_git_quality_gates.py:
- Around line 17-20: Replace hardcoded DEFAULT_TIMEOUT, MAX_RETRIES, and
RETRY_DELAY with values read from environment variables (e.g., CLAUDE_TIMEOUT,
CLAUDE_MAX_RETRIES, CLAUDE_RETRY_DELAY) while preserving the current defaults;
in the module where DEFAULT_TIMEOUT, MAX_RETRIES, and RETRY_DELAY are defined,
read os.environ.get for each, attempt safe int conversion with fallback to the
existing constants, and handle invalid values by falling back and optionally
logging a warning; ensure any code using DEFAULT_TIMEOUT, MAX_RETRIES, and
RETRY_DELAY continues to reference these symbols so behavior is unchanged when
env vars are absent.
In `@script/security-credential-scan.sh`:
- Line 103: The "Supabase Key" regex stored in PATTERNS["Supabase Key"] is too
generic (it just matches HS256 JWT headers) so update detection to avoid false
positives: either rename the key to "JWT (HS256)" or enhance the pattern to
require Supabase context (e.g., lookaround or require the word "supabase" near
the token), and if you keep both patterns reconcile with the existing "JWT
Token" entry (remove or differentiate one) so you don't duplicate generic JWT
detection; update PATTERNS["Supabase Key"] accordingly and adjust any tests or
references that rely on that key name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de71a8d9-8a2e-4a02-a4db-42710adbe024
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.claude/hooks/pre_git_quality_gates.pyREADME.mdSECURITY.mdcredentials/README.mdgit/gitconfigscript/import.shscript/lib/config.shscript/security-credential-scan.shtest/integration/security-scripts.batstest/python/test_hooks.py
| def is_retryable_error(returncode: int, stderr: str) -> bool: | ||
| """リトライ可能なエラーかどうか判定""" | ||
| # ネットワーク系エラー | ||
| network_errors = [ | ||
| "ECONNREFUSED", | ||
| "ETIMEDOUT", | ||
| "ENOTFOUND", | ||
| "network error", | ||
| "socket hang up", | ||
| "EAI_AGAIN", | ||
| ] | ||
| for err in network_errors: | ||
| if err in stderr: | ||
| return True | ||
| # 一時的なファイルロック | ||
| if "EBUSY" in stderr or "resource busy" in stderr.lower(): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Unused returncode parameter.
The returncode parameter is declared but never used in the function body. The current implementation only checks stderr content for retryable error patterns.
Consider either removing the parameter or using it to identify additional retryable conditions (e.g., specific exit codes that indicate transient failures).
🔧 Option 1: Remove unused parameter
-def is_retryable_error(returncode: int, stderr: str) -> bool:
+def is_retryable_error(stderr: str) -> bool:
"""リトライ可能なエラーかどうか判定"""Then update the call site at line 205-206:
- if attempt < max_retries and is_retryable_error(
- result.returncode, result.stderr
- ):
+ if attempt < max_retries and is_retryable_error(result.stderr):🔧 Option 2: Use returncode for additional checks
def is_retryable_error(returncode: int, stderr: str) -> bool:
"""リトライ可能なエラーかどうか判定"""
+ # Transient exit codes (e.g., signal interrupts)
+ if returncode in (130, 137, 143): # SIGINT, SIGKILL, SIGTERM
+ return True
+
# ネットワーク系エラー
network_errors = [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def is_retryable_error(returncode: int, stderr: str) -> bool: | |
| """リトライ可能なエラーかどうか判定""" | |
| # ネットワーク系エラー | |
| network_errors = [ | |
| "ECONNREFUSED", | |
| "ETIMEDOUT", | |
| "ENOTFOUND", | |
| "network error", | |
| "socket hang up", | |
| "EAI_AGAIN", | |
| ] | |
| for err in network_errors: | |
| if err in stderr: | |
| return True | |
| # 一時的なファイルロック | |
| if "EBUSY" in stderr or "resource busy" in stderr.lower(): | |
| return True | |
| return False | |
| def is_retryable_error(returncode: int, stderr: str) -> bool: | |
| """リトライ可能なエラーかどうか判定""" | |
| # Transient exit codes (e.g., signal interrupts) | |
| if returncode in (130, 137, 143): # SIGINT, SIGKILL, SIGTERM | |
| return True | |
| # ネットワーク系エラー | |
| network_errors = [ | |
| "ECONNREFUSED", | |
| "ETIMEDOUT", | |
| "ENOTFOUND", | |
| "network error", | |
| "socket hang up", | |
| "EAI_AGAIN", | |
| ] | |
| for err in network_errors: | |
| if err in stderr: | |
| return True | |
| # 一時的なファイルロック | |
| if "EBUSY" in stderr or "resource busy" in stderr.lower(): | |
| return True | |
| return False |
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 158-158: Unused function argument: returncode
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/pre_git_quality_gates.py around lines 158 - 175, The function
is_retryable_error currently declares an unused returncode parameter; remove
that parameter from the function signature (change def
is_retryable_error(stderr: str) -> bool) and update every call site that passes
a return code to instead call is_retryable_error(stderr) so the function only
inspects stderr; ensure you update import/type hints if any and run tests to
confirm all callers compile.
| # Create file with fake AWS key pattern | ||
| echo 'const key = "AKIAIOSFODNN7EXAMPLE";' > "$temp_dir/test.js" |
There was a problem hiding this comment.
Avoid committing a literal AWS-looking key in the test source.
script/security-credential-scan.sh matches AKIA[0-9A-Z]{16} and classifies AWS hits as CRITICAL, so these checked-in literals make the repository fail its own credential scan when the repo is scanned. Build the fake key from split fragments at runtime instead.
Suggested fix
- echo 'const key = "AKIAIOSFODNN7EXAMPLE";' > "$temp_dir/test.js"
+ fake_aws_key='AKIA'"IOSFODNN7EXAMPLE"
+ printf 'const key = "%s";\n' "$fake_aws_key" > "$temp_dir/test.js"- echo 'const key = "AKIAIOSFODNN7EXAMPLE";' > "$temp_dir/test.js"
+ fake_aws_key='AKIA'"IOSFODNN7EXAMPLE"
+ printf 'const key = "%s";\n' "$fake_aws_key" > "$temp_dir/test.js"Also applies to: 77-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/security-scripts.bats` around lines 65 - 66, The test
currently writes a literal-looking AWS key into the repo (echo 'const key =
"AKIAIOSFODNN7EXAMPLE";' > "$temp_dir/test.js"), which triggers the credential
scanner; instead, compose the fake key at runtime from non-matching fragments
(e.g. concatenate prefix "AKIA" with other substrings) and write that assembled
string into "$temp_dir/test.js" so no literal AKIA... token appears in source;
apply the same change to the other occurrence around lines 77-78 in the same
test file and ensure the assembled key still matches the intended test value
when evaluated by the test harness.
| content = hook_file.read_text() | ||
|
|
||
| required_functions = [ | ||
| "detect_package_manager", | ||
| "has_biome", | ||
| "get_package_scripts", | ||
| "run_with_retry", | ||
| "detect_linter_conflicts", | ||
| ] | ||
|
|
||
| for func in required_functions: | ||
| assert f"def {func}" in content, \ | ||
| f"Missing function: {func} in pre_git_quality_gates.py" |
There was a problem hiding this comment.
This only greps source text; it doesn't verify the hook API or behavior.
A comment or string containing def run_with_retry would satisfy this test, and none of the new retry/conflict paths are executed. Parse the module AST for real top-level definitions, then add behavioral tests for the newly added logic.
Suggested fix
+import ast
+
...
- content = hook_file.read_text()
+ tree = ast.parse(hook_file.read_text(encoding="utf-8"))
+ defined_functions = {
+ node.name for node in tree.body if isinstance(node, ast.FunctionDef)
+ }
required_functions = [
"detect_package_manager",
"has_biome",
"get_package_scripts",
"run_with_retry",
"detect_linter_conflicts",
]
for func in required_functions:
- assert f"def {func}" in content, \
+ assert func in defined_functions, \
f"Missing function: {func} in pre_git_quality_gates.py"As per coding guidelines, "Follow TDD methodology: Red → Green → Refactor with minimum 70% line code coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/python/test_hooks.py` around lines 33 - 45, The test currently just
greps text in test_hooks.py and must be replaced with real verification: parse
pre_git_quality_gates.py with the AST module to assert the presence of real
top-level function definitions (e.g., detect_package_manager, has_biome,
get_package_scripts, run_with_retry, detect_linter_conflicts) rather than string
matches, and add behavioral unit tests that import the module and execute the
new logic paths for run_with_retry and detect_linter_conflicts
(mocking/simulating failures and conflicting linter configs) to ensure retry
behavior and conflict detection run as expected; update test names and
assertions to validate actual API signatures and outcomes instead of text
presence.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR レビュー: feat: 品質ツールの包括的な改善全体的によくまとまった改善です。各変更について詳細なフィードバックをまとめます。 ✅ 良い点
🔴 要修正: バグ・誤検知リスク1.
|
|
🎉 This PR is included in version 1.93.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
詳細
Hooks改善
pre_git_quality_gates.pyにリトライロジックを追加セキュリティスキャン強化
新たに検出可能なパターン:
ドキュメント修正
SSH署名鍵の設定方法を修正:
$(cat ~/.ssh/id_ed25519.pub)(内容を展開)~/.ssh/id_ed25519.pub(パスを直接使用)テスト追加
test/integration/security-scripts.bats: セキュリティスクリプトの統合テストtest/python/test_hooks.py: Python Hooks の基本テストTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests