mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
3b57d5bec9
* 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]>
2.6 KiB
2.6 KiB
name, version, description, activation
| name | version | description | activation | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| review-checklist | 0.1.0 | Pre-merge review checklist based on recurring AI reviewer feedback patterns |
|
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()returningAlwaysorUnlessAutoApproved - 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 -- useis_char_boundary()orchar_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
LlmProvidertrait methods are delegated in ALL wrapper types (grepimpl 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
tempfilecrate, 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)