mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
chore: add reviewer-feedback guardrails (CLAUDE.md, pre-commit hook, skill) (#665)
* chore: add reviewer-feedback guardrails (CLAUDE.md, pre-commit hook, skill) Analysis of ~50 PRs from the past week identified 10 recurring themes in Copilot and Gemini code review comments. This change addresses them at development time through three layers: 1. CLAUDE.md additions (7 new rules): - Transaction safety for multi-step DB operations - UTF-8 string safety (no byte-index slicing) - Case-insensitive comparisons for paths/media types - Decorator/wrapper trait method delegation - Sensitive data redaction in logs/SSE - tempfile crate for test temporary files - Trust boundaries for worker container data 2. Pre-commit hook (scripts/pre-commit-safety.sh): Mechanical checks for unsafe byte slicing, case-sensitive extension comparisons, hardcoded /tmp paths, unredacted tool parameter logging, and non-transactional DB operations. Installed via dev-setup.sh alongside existing commit-msg hook. 3. Review checklist skill (skills/review-checklist/SKILL.md): Activates on "review"/"merge" keywords. Covers the judgment-based items that can't be linted: transaction safety, SSRF validation, approval checks, decorator delegation, test quality, and doc accuracy. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address PR review feedback on pre-commit-safety.sh - Cache diff output in variable to avoid ~10 redundant git diff calls (Gemini) - Add early exit when no .rs files are changed (Gemini) - Fix header comment: list all 5 checks, not just 4 (Copilot) - Fix check 2 comment: only mentions file extensions, not media types (Copilot) - Add resolve_base_ref() with fallback candidates instead of hardcoded origin/main for standalone mode (Copilot) - TX check: use -W (function context) to reduce false positives, honor // safety: suppression, print triggering lines (Copilot) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Illia Polosukhin
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
11c5e25422
commit
3b57d5bec9
@@ -387,12 +387,27 @@ Dead code behind the wrong `#[cfg]` gate will only show up when building with a
|
||||
|
||||
**Zero clippy warnings policy:** Fix ALL clippy warnings before committing, including pre-existing ones in files you didn't change. Never leave warnings behind — treat `cargo clippy` output as a zero-tolerance gate.
|
||||
|
||||
**Transaction safety:** Multi-step database operations (INSERT+INSERT, UPDATE+DELETE, read-then-write) MUST be wrapped in a transaction. Never assume sequential calls are atomic. Before committing DB code, ask: "If this crashes between step N and N+1, is the database consistent?" If not, wrap in a transaction. This applies to both postgres and libsql backends.
|
||||
|
||||
**UTF-8 string safety:** Never use byte-index slicing (`&s[..n]`) on user-supplied or external strings — it panics on multi-byte characters. Use `is_char_boundary()` to walk backwards from the desired length, or iterate with `char_indices()`. Grep for `[..` in changed files to catch violations.
|
||||
|
||||
**Case-insensitive comparisons:** When comparing user-supplied strings (file paths, media types, extension names), always normalize to lowercase first with `.to_ascii_lowercase()`. On case-insensitive filesystems (macOS, Windows), path comparisons must be case-insensitive. File extension checks (`.png`, `.jpg`) and media type checks (`image/jpeg`) are common offenders.
|
||||
|
||||
**Decorator/wrapper trait delegation:** When adding a new method to `LlmProvider` (or any trait with decorator wrappers), you MUST update ALL wrapper types to delegate to their inner provider. Grep for `impl LlmProvider for` to find all implementations. Add a test that exercises the method through the full provider chain (`build_provider_chain()`), not just the base impl.
|
||||
|
||||
**Sensitive data in logs & events:** Tool parameters and outputs MUST be redacted before logging or broadcasting via SSE/WebSocket. Use `redact_params()` before any `tracing::info!`, `JobEvent`, or SSE emission that includes tool call data. Never log raw parameters from tool calls.
|
||||
|
||||
**Test temporary files:** Use the `tempfile` crate for test directories/files. Never hardcode `/tmp/...` paths — they collide in parallel test runs and break on non-Unix platforms.
|
||||
|
||||
**Trust boundaries in multi-process architecture:** Data from worker containers is untrusted. The orchestrator MUST validate: tool domain (never execute `Container`-domain tools on the host), nesting depth (server-side tracking, not client-supplied), and parameter sensitivity (redact before logging/broadcasting).
|
||||
|
||||
**Mechanical verification before committing:** Run these checks on changed files before committing:
|
||||
- `cargo clippy --all --benches --tests --examples --all-features` -- zero warnings
|
||||
- `grep -rnE '\.unwrap\(|\.expect\(' <files>` -- no panics in production
|
||||
- `grep -rn 'super::' <files>` -- use `crate::` imports
|
||||
- If you fixed a pattern bug, `grep` for other instances of that pattern across `src/`
|
||||
- Fix commits must include regression tests (enforced by `commit-msg` hook; bypass with `[skip-regression-check]`)
|
||||
- Run `scripts/pre-commit-safety.sh` to catch UTF-8, case-sensitivity, hardcoded /tmp, and logging issues
|
||||
|
||||
## Configuration
|
||||
|
||||
|
||||
@@ -51,9 +51,11 @@ echo "[6/6] Installing git hooks..."
|
||||
HOOKS_DIR=$(git rev-parse --git-path hooks 2>/dev/null) || true
|
||||
if [ -n "$HOOKS_DIR" ]; then
|
||||
mkdir -p "$HOOKS_DIR"
|
||||
SCRIPT_ABS="$(cd "$(dirname "$0")" && pwd)/commit-msg-regression.sh"
|
||||
ln -sf "$SCRIPT_ABS" "$HOOKS_DIR/commit-msg"
|
||||
SCRIPTS_ABS="$(cd "$(dirname "$0")" && pwd)"
|
||||
ln -sf "$SCRIPTS_ABS/commit-msg-regression.sh" "$HOOKS_DIR/commit-msg"
|
||||
echo " commit-msg hook installed (regression test enforcement)"
|
||||
ln -sf "$SCRIPTS_ABS/pre-commit-safety.sh" "$HOOKS_DIR/pre-commit"
|
||||
echo " pre-commit hook installed (UTF-8, case-sensitivity, /tmp, redaction checks)"
|
||||
else
|
||||
echo " Skipped: not a git repository"
|
||||
fi
|
||||
|
||||
Executable
+136
@@ -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
|
||||
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
|
||||
@@ -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)
|
||||
Reference in New Issue
Block a user