From d9ff86d7e0595c528b00b490872ae3c32a8b9494 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Fri, 13 Feb 2026 20:25:53 -0800 Subject: [PATCH] docs: Add review discipline guidelines to CLAUDE.md (#68) * docs: Add review discipline guidelines to CLAUDE.md Codifies lessons learned from Illia's review fixes on the libSQL backend PR -- patterns we missed that should be caught systematically going forward. - Ban .expect() alongside .unwrap() in production code - Add mechanical grep checks before committing - New "Review & Fix Discipline" section covering: - Fix all instances of a pattern, not just the one flagged - Propagate architectural changes to satellite types - Schema translation must include indexes and seed data - Feature flag testing with each feature in isolation Co-Authored-By: Claude Opus 4.6 * Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: Illia Polosukhin Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- CLAUDE.md | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index aeaf1dd2..5664469b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -198,8 +198,9 @@ When designing new features or systems, always prefer generic/extensible archite ### Error Handling - Use `thiserror` for error types in `error.rs` -- Never use `.unwrap()` in production code (tests are fine) +- Never use `.unwrap()` or `.expect()` in production code (tests are fine) - Map errors with context: `.map_err(|e| SomeError::Variant { reason: e.to_string() })?` +- Before committing, grep for `.unwrap()` and `.expect(` in changed files to catch violations mechanically ### Async - All I/O is async with tokio @@ -637,6 +638,37 @@ RUST_LOG=ironclaw=debug,tower_http=debug cargo run - Keep functions focused, extract helpers when logic is reused - Comments for non-obvious logic only +## Review & Fix Discipline + +Hard-won lessons from code review -- follow these when fixing bugs or addressing review feedback. + +### Fix the pattern, not just the instance +When a reviewer flags a bug (e.g., TOCTOU race in INSERT + SELECT-back), search the entire codebase for all instances of that same pattern. A fix in `SecretsStore::create()` that doesn't also fix `WasmToolStore::store()` is half a fix. + +### Propagate architectural fixes to satellite types +If a core type changes its concurrency model (e.g., `LibSqlBackend` switches to connection-per-operation), every type that was handed a resource from the old model (e.g., `LibSqlSecretsStore`, `LibSqlWasmToolStore` holding a single `Connection`) must also be updated. Grep for the old type across the codebase. + +### Schema translation is more than DDL +When translating a database schema between backends (PostgreSQL to libSQL, etc.), check for: +- **Indexes** -- diff `CREATE INDEX` statements between the two schemas +- **Seed data** -- check for `INSERT INTO` in migrations (e.g., `leak_detection_patterns`) +- **Semantic differences** -- document where SQL functions behave differently (e.g., `json_patch` vs `jsonb_set`) + +### Feature flag testing +When adding feature-gated code, test compilation with each feature in isolation: +```bash +cargo check # default features +cargo check --no-default-features --features libsql # libsql only +cargo check --all-features # all features +``` +Dead code behind the wrong `#[cfg]` gate will only show up when building with a single feature. + +### Mechanical verification before committing +Run these checks on changed files before committing: +- `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/` + ## Workspace & Memory System Inspired by [OpenClaw](https://github.com/openclaw/openclaw), the workspace provides persistent memory for agents with a flexible filesystem-like structure.