chore: add reviewer-feedback guardrails (CLAUDE.md, pre-commit hook, skill) #665
+209
−2
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| #!/usr/bin/env bash | ||
| # Pre-commit safety checks for common issues caught by AI code reviewers. | ||
| # | ||
| # Can be run standalone: bash scripts/pre-commit-safety.sh | ||
| # Or installed as a git pre-commit hook via dev-setup.sh. | ||
| # | ||
| # Checks staged .rs files for: | ||
| # 1. Unsafe UTF-8 byte slicing (panics on multi-byte chars) | ||
| # 2. Case-sensitive file extension comparisons | ||
| # 3. Hardcoded /tmp paths in tests (flaky in parallel runs) | ||
| # 4. Tool parameters logged without redaction (secret leaks) | ||
| # 5. Multi-step DB operations without transaction wrapping | ||
| # | ||
| # Suppress individual lines with an inline "// safety: <reason>" comment. | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| # Determine a suitable base ref for standalone diffs. | ||
| resolve_base_ref() { | ||
| local candidates=( | ||
| "@{upstream}" | ||
| "origin/HEAD" | ||
| "origin/main" | ||
| "origin/master" | ||
| "main" | ||
| "master" | ||
| ) | ||
|
|
||
| for ref in "${candidates[@]}"; do | ||
| if git rev-parse --verify --quiet "$ref" >/dev/null 2>&1; then | ||
| echo "$ref" | ||
| return 0 | ||
| fi | ||
| done | ||
|
|
||
| echo "pre-commit-safety: could not determine a base Git ref for diff (tried: ${candidates[*]})." >&2 | ||
| echo "pre-commit-safety: ensure your repository has an upstream or a local main/master branch." >&2 | ||
| exit 1 | ||
| } | ||
|
|
||
| # Support both pre-commit hook (staged files) and standalone (all changed vs base) | ||
| if git diff --cached --quiet 2>/dev/null; then | ||
| # No staged changes -- compare working tree against a resolved base ref | ||
| BASE_REF="$(resolve_base_ref)" | ||
| DIFF_OUTPUT=$(git diff "$BASE_REF" -- '*.rs' 2>/dev/null || true) | ||
| else | ||
| DIFF_OUTPUT=$(git diff --cached -U0 -- '*.rs' 2>/dev/null || true) | ||
| fi | ||
|
|
||
| # Early exit if there are no relevant .rs changes | ||
| if [ -z "$DIFF_OUTPUT" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| WARNINGS=0 | ||
|
|
||
| warn() { | ||
| if [ "$WARNINGS" -eq 0 ]; then | ||
| echo "" | ||
| echo "=== Pre-commit Safety Checks ===" | ||
| echo "" | ||
| fi | ||
| WARNINGS=$((WARNINGS + 1)) | ||
| echo " [$1] $2" | ||
| } | ||
|
|
||
| # 1. Unsafe UTF-8 byte slicing: &s[..N] or &s[..some_var] on strings | ||
| # Safe patterns: is_char_boundary, char_indices, // safety: | ||
| if echo "$DIFF_OUTPUT" | grep -nE '^\+' | grep -E '\[\.\..*\]' | grep -vE 'is_char_boundary|char_indices|// safety:|as_bytes|Vec<|&\[u8\]|\[u8\]|bytes\(\)|&bytes' | head -3 | grep -q .; then | ||
| warn "UTF8" "Possible unsafe byte-index string slicing. Use is_char_boundary() or char_indices()." | ||
| echo "$DIFF_OUTPUT" | grep -nE '^\+' | grep -E '\[\.\..*\]' | grep -vE 'is_char_boundary|char_indices|// safety:|as_bytes|Vec<|&\[u8\]|\[u8\]|bytes\(\)|&bytes' | head -3 | sed 's/^/ /' | ||
| fi | ||
|
|
||
| # 2. Case-sensitive file extension checks | ||
| # Match: .ends_with(".png") without prior to_lowercase | ||
| if echo "$DIFF_OUTPUT" | grep -nE '^\+.*ends_with\("\.([pP][nN][gG]|[jJ][pP][eE]?[gG]|[gG][iI][fF]|[wW][eE][bB][pP]|[mM][dD])"\)' | grep -vE 'to_lowercase|to_ascii_lowercase|// safety:' | head -3 | grep -q .; then | ||
| warn "CASE" "Case-sensitive file extension comparison. Normalize to lowercase first." | ||
| echo "$DIFF_OUTPUT" | grep -nE '^\+.*ends_with\("\.([pP][nN][gG]|[jJ][pP][eE]?[gG]|[gG][iI][fF]|[wW][eE][bB][pP]|[mM][dD])"\)' | grep -vE 'to_lowercase|to_ascii_lowercase|// safety:' | head -3 | sed 's/^/ /' | ||
| fi | ||
|
|
||
| # 3. Hardcoded /tmp paths in test files | ||
| if echo "$DIFF_OUTPUT" | grep -nE '^\+.*"/tmp/' | grep -vE 'tempfile|tempdir|// safety:' | head -3 | grep -q .; then | ||
| warn "TMPDIR" "Hardcoded /tmp path. Use tempfile::tempdir() for parallel-safe tests." | ||
| echo "$DIFF_OUTPUT" | grep -nE '^\+.*"/tmp/' | grep -vE 'tempfile|tempdir|// safety:' | head -3 | sed 's/^/ /' | ||
| fi | ||
|
|
||
| # 4. Logging tool parameters without redaction | ||
| if echo "$DIFF_OUTPUT" | grep -nE '^\+.*tracing::(info|debug|warn|error).*param' | grep -vE 'redact|// safety:' | head -3 | grep -q .; then | ||
| warn "REDACT" "Logging tool parameters without redaction. Use redact_params() first." | ||
| echo "$DIFF_OUTPUT" | grep -nE '^\+.*tracing::(info|debug|warn|error).*param' | grep -vE 'redact|// safety:' | head -3 | sed 's/^/ /' | ||
| fi | ||
|
|
||
| # 5. Multi-step DB operations without transaction | ||
| # Uses -W (function context) to reduce false positives from existing transactions. | ||
| # Suppressible with "// safety:" in the hunk. | ||
| DIFF_W_OUTPUT=$(git diff --cached -W -- '*.rs' 2>/dev/null || git diff "$(resolve_base_ref)" -W -- '*.rs' 2>/dev/null || true) | ||
| if [ -n "$DIFF_W_OUTPUT" ]; then | ||
| HUNK_COUNT=$(echo "$DIFF_W_OUTPUT" | awk ' | ||
| /^@@/ { | ||
| if (count >= 2 && !has_tx && !has_safety) found++ | ||
| count=0; has_tx=0; has_safety=0 | ||
| } | ||
| /^\+.*\.(execute|query)\(/ { count++ } | ||
| /^\+.*(transaction|\.tx\.|\.begin\()/ { has_tx=1 } | ||
| / .*(transaction|\.tx\.|\.begin\()/ { has_tx=1 } | ||
| /\/\/ safety:/ { has_safety=1 } | ||
| END { | ||
| if (count >= 2 && !has_tx && !has_safety) found++ | ||
| print found+0 | ||
| } | ||
| ') | ||
| if [ "$HUNK_COUNT" -gt 0 ]; then | ||
| warn "TX" "Multiple DB operations in same function without transaction. Wrap in a transaction for atomicity." | ||
| echo "$DIFF_W_OUTPUT" | awk ' | ||
| /^@@/ { | ||
| if (count >= 2 && !has_tx && !has_safety) { print buf } | ||
| buf=""; count=0; has_tx=0; has_safety=0 | ||
| } | ||
| /^\+.*\.(execute|query)\(/ { count++ } | ||
| /^\+.*(transaction|\.tx\.|\.begin\()/ { has_tx=1 } | ||
| / .*(transaction|\.tx\.|\.begin\()/ { has_tx=1 } | ||
| /\/\/ safety:/ { has_safety=1 } | ||
| { buf = buf "\n" $0 } | ||
| END { | ||
| if (count >= 2 && !has_tx && !has_safety) { print buf } | ||
| } | ||
| ' | grep -E '^\+.*\.(execute|query)\(' | head -4 | sed 's/^/ /' | ||
| fi | ||
|
Comment on lines
+93
to
+128
There was a problem hiding this comment. The transaction-safety check operates on
Member
Author
There was a problem hiding this comment. Fixed in f74ced1 — TX check now uses |
||
| fi | ||
|
|
||
| if [ "$WARNINGS" -gt 0 ]; then | ||
| echo "" | ||
| echo "Found $WARNINGS potential issue(s). Fix them or add '// safety: <reason>' to suppress." | ||
| echo "" | ||
| exit 1 | ||
| fi | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| --- | ||
| name: review-checklist | ||
| version: 0.1.0 | ||
| description: Pre-merge review checklist based on recurring AI reviewer feedback patterns | ||
| activation: | ||
| patterns: | ||
| - "review.*checklist" | ||
| - "ready to merge" | ||
| - "pre-merge check" | ||
| - "check.*before.*merge" | ||
| keywords: | ||
| - review | ||
| - checklist | ||
| - merge | ||
| - pre-merge | ||
| max_context_tokens: 1500 | ||
| --- | ||
|
|
||
| # Pre-Merge Review Checklist | ||
|
|
||
| Before merging, verify these items. They represent the most common issues caught by automated code reviewers (Copilot, Gemini) on IronClaw PRs. | ||
|
|
||
| ## Database Operations | ||
| - [ ] Multi-step DB operations are wrapped in transactions (INSERT+INSERT, UPDATE+DELETE, read-modify-write) | ||
| - [ ] Both postgres AND libsql backends updated for any new Database trait methods | ||
| - [ ] Migrations are atomic (SQL execution + version recording in same transaction) | ||
|
|
||
| ## Security & Data Safety | ||
| - [ ] Tool parameters are redacted via `redact_params()` before logging or SSE/WebSocket broadcast | ||
| - [ ] URL validation resolves DNS before checking for private/loopback IPs (anti-SSRF via DNS rebinding) | ||
| - [ ] Destructive tools have `requires_approval()` returning `Always` or `UnlessAutoApproved` | ||
| - [ ] Data from worker containers is treated as untrusted (tool domain checks, server-side nesting depth) | ||
| - [ ] No secrets or credentials in error messages, logs, or SSE events | ||
|
|
||
| ## String Safety | ||
| - [ ] No byte-index slicing (`&s[..n]`) on external/user strings -- use `is_char_boundary()` or `char_indices()` | ||
| - [ ] File extension and media type comparisons are case-insensitive (`.to_ascii_lowercase()` before matching) | ||
| - [ ] Path comparisons are case-insensitive where needed (macOS/Windows filesystems) | ||
|
|
||
| ## Trait Wrappers & Decorator Chain | ||
| - [ ] New `LlmProvider` trait methods are delegated in ALL wrapper types (grep `impl LlmProvider for`) | ||
| - [ ] New trait methods are tested through the full decorator/provider chain, not just the base impl | ||
| - [ ] Default trait method implementations are intentional -- wrappers that silently return defaults are bugs | ||
|
|
||
| ## Tests | ||
| - [ ] Temporary files/dirs use `tempfile` crate, no hardcoded `/tmp/` paths | ||
| - [ ] Tests don't mutate global statics without synchronization (use per-test state or `serial_test`) | ||
| - [ ] Tests don't make real network requests (use mocks, stubs, or RFC 5737 TEST-NET IPs like 192.0.2.1) | ||
| - [ ] Test names and comments match actual test behavior and assertions | ||
|
|
||
| ## Comments & Documentation | ||
| - [ ] Code comments match actual behavior (especially route paths, tool names, function semantics) | ||
| - [ ] Spec/README files updated if module behavior changed | ||
| - [ ] Error messages are clear and non-redundant (don't nest tool name inside tool error that already contains it) |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header comment says this hook checks 4 items, but the script also includes a 5th check for multi-step DB operations/transactions. Please update the header comment (or the implemented checks list) so it accurately reflects what the hook enforces; otherwise it’s easy for developers to miss why commits are blocked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f74ced1 — header now lists all 5 checks.