From 3b57d5bec96fe66ca096b84429021608f71ba6b5 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Sat, 7 Mar 2026 21:20:37 +0000 Subject: [PATCH] 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 * 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 --------- Co-authored-by: Claude Opus 4.6 --- CLAUDE.md | 15 ++++ scripts/dev-setup.sh | 6 +- scripts/pre-commit-safety.sh | 136 +++++++++++++++++++++++++++++++ skills/review-checklist/SKILL.md | 54 ++++++++++++ 4 files changed, 209 insertions(+), 2 deletions(-) create mode 100755 scripts/pre-commit-safety.sh create mode 100644 skills/review-checklist/SKILL.md diff --git a/CLAUDE.md b/CLAUDE.md index d0e726ce..249bc903 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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\(' ` -- no panics in production - `grep -rn 'super::' ` -- 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 diff --git a/scripts/dev-setup.sh b/scripts/dev-setup.sh index 7293f8d1..faa5aa2c 100755 --- a/scripts/dev-setup.sh +++ b/scripts/dev-setup.sh @@ -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 diff --git a/scripts/pre-commit-safety.sh b/scripts/pre-commit-safety.sh new file mode 100755 index 00000000..3fddc3b8 --- /dev/null +++ b/scripts/pre-commit-safety.sh @@ -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: " 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: ' to suppress." + echo "" + exit 1 +fi diff --git a/skills/review-checklist/SKILL.md b/skills/review-checklist/SKILL.md new file mode 100644 index 00000000..feafd52f --- /dev/null +++ b/skills/review-checklist/SKILL.md @@ -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)