From bdb33c58d1ac505c5075bf1c13023a88c5e4e053 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 31 Mar 2026 11:09:41 +0200 Subject: [PATCH] feat: add /code-review skill with project standards (#806) * feat: add /code-review skill for automated code quality checks Adds a Claude Code skill that reviews changes against project standards: missing tests, dead code, type safety, lint, and CLAUDE.md conventions. CLAUDE.md now instructs contributors to run /code-review after implementation. * refactor: move code standards from CLAUDE.md into /code-review skill Single source of truth for coding conventions (Python style, type safety, TypeScript style) is now .claude/skills/code-review.md. CLAUDE.md points to the skill for reading before coding and running after implementation. * feat: add code comments convention to /code-review skill Require comments explaining non-trivial technical decisions, with history of previous approaches. Review step checks for missing reasoning comments, stale comments, and undocumented approach changes. * fix: move skill to directory structure for Claude Code discovery Claude Code requires .claude/skills//SKILL.md, not loose .md files. * feat: add branch hygiene checks to /code-review skill Review step 1 now verifies branch is based on recent origin/main and all commits are relevant to the feature. Unrelated commits flagged as must-fix. * feat: strengthen code review rules and fix stale CLAUDE.md references - Enforce no multi-item tuple returns and no raw dicts even for internal code - Add mandatory /code-review gate before push/PR - Add integration completeness checklist (tests, CI job, release-integration.sh) - Fix stale references: remove hindsight/ dir, update integrations list, update LLM providers, remove hardcoded file sizes, fix _HIERARCHICAL_FIELDS -> _CONFIGURABLE_FIELDS * docs: add ./scripts/dev/start.sh for local dev in CLAUDE.md --- .claude/skills/code-review/SKILL.md | 26 ++++++++++++++----- CLAUDE.md | 40 +++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index dc8f326f..926569dc 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -18,15 +18,17 @@ Read and internalize these standards before writing code. The review steps below - Pydantic models for request/response - Ruff for linting (line-length 120) - No Python files at project root - maintain clean directory structure -- **Never use multi-item tuple return values** - prefer dataclass or Pydantic model for structured returns +- **Never use multi-item tuple return values** — not even for internal/private functions. Always use a dataclass or Pydantic model. No exceptions, no "it's just two values" shortcuts. If a function returns more than one value, define a named type for it. ### Type Safety with Pydantic Models -**NEVER use raw `dict` types for structured data.** Always use Pydantic models: +**NEVER use raw `dict` types for structured data** — this applies to all code, including internal helpers and private functions. If the dict has known keys, it must be a dataclass or Pydantic model: - Use Pydantic `BaseModel` for all data structures passed between functions +- Use `@dataclass` for lightweight internal data containers when Pydantic validation isn't needed - Add `@field_validator` for type coercion (e.g., ensuring datetimes are timezone-aware) - Avoid `dict.get()` patterns - use typed model attributes instead - Parse external data (JSON, API responses) into Pydantic models at the boundary - This catches type errors at parse time, not deep in business logic +- The only acceptable `dict` usage is for truly dynamic/unknown keys (e.g., arbitrary metadata, JSON blobs with no fixed schema) ```python # BAD - error-prone dict access @@ -118,8 +120,8 @@ For each changed TypeScript file, check for: ### 5. Check type safety (Python) For each changed Python file, check for violations: -- **No raw `dict` for structured data** — should use Pydantic models -- **No multi-item tuple returns** — should use dataclass or Pydantic model +- **No raw `dict` for structured data** — must use Pydantic model or dataclass, even for internal/private functions (only exception: truly dynamic/unknown keys) +- **No multi-item tuple returns** — must use dataclass or Pydantic model, even for internal/private functions (no exceptions) - **Missing type hints** on function parameters and return types - **Missing `@field_validator`** for datetime fields that should be timezone-aware @@ -147,7 +149,15 @@ For each non-trivial change: - **Changed approach** — does the comment include what was done before and why it changed? - **Stale comments** — do existing comments near the changed code still accurately describe the behavior? -### 9. Review against other coding standards +### 9. Check integration completeness + +If any files in `hindsight-integrations/` were added or changed, verify: +- **Tests exist** — the integration must have tests that simulate/exercise the external framework (not just pure unit tests of helpers). Check for a `tests/` directory with meaningful test files. +- **CI job exists** — check `.github/workflows/test.yml` for a corresponding `test--integration` job. If missing, flag it. +- **Release process** — check that the integration name is in the `VALID_INTEGRATIONS` array in `scripts/release-integration.sh`. If missing, flag it. +- **Code standards** — the integration code must follow all Python style rules (type hints, no raw dicts, no tuple returns, etc.). + +### 10. Review against other coding standards Check the diff for violations of the standards listed above: - Python files at project root (not allowed) @@ -159,7 +169,7 @@ Check the diff for violations of the standards listed above: - Premature abstractions or speculative helpers - Backwards-compatibility hacks (unused vars, re-exports, "removed" comments) -### 10. Report findings +### 11. Report findings Present a clear summary organized by severity: @@ -167,8 +177,10 @@ Present a clear summary organized by severity: - Unrelated commits on the branch - Lint failures - Missing type hints on public functions -- Raw dict usage for structured data +- Raw dict usage for structured data (including internal code) +- Multi-item tuple returns (including internal code) - Missing tests for new endpoints +- New integration missing tests, CI job, or release-integration.sh entry **Should fix** — issues that hurt code quality: - Dead code / unused imports missed by linter diff --git a/CLAUDE.md b/CLAUDE.md index 66d590b1..10dc2265 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,9 +11,15 @@ Hindsight is an agent memory system that provides long-term memory for AI agents ## Development Commands +### Local Development (API + UI) +```bash +# Start both API server and control plane UI +./scripts/dev/start.sh +``` + ### API Server (Python/FastAPI) ```bash -# Start API server (loads .env automatically) +# Start API server only (loads .env automatically) ./scripts/dev/start-api.sh # Run all tests (parallelized with pytest-xdist) @@ -73,17 +79,16 @@ cd hindsight-control-plane && npm run dev ### Monorepo Structure - **hindsight-api-slim/**: Core FastAPI server with memory engine (Python, uv) -- **hindsight/**: Embedded Python bundle (hindsight-all package) - **hindsight-control-plane/**: Admin UI (Next.js, npm) - **hindsight-cli/**: CLI tool (Rust, cargo, uses progenitor for API client) - **hindsight-clients/**: Generated SDK clients (Python, TypeScript, Rust) - **hindsight-docs/**: Docusaurus documentation site -- **hindsight-integrations/**: Framework integrations (LiteLLM, OpenAI) +- **hindsight-integrations/**: Framework integrations (LiteLLM, CrewAI, LangGraph, Pydantic AI, AG2, Claude Code, etc.) - **hindsight-dev/**: Development tools and benchmarks ### Core Engine (hindsight-api-slim/hindsight_api/engine/) -- `memory_engine.py`: Main orchestrator (~170KB) for retain/recall/reflect operations -- `llm_wrapper.py`: LLM abstraction supporting OpenAI, Anthropic, Gemini, Groq, MiniMax, Ollama, LM Studio +- `memory_engine.py`: Main orchestrator for retain/recall/reflect operations +- `llm_wrapper.py`: LLM abstraction supporting OpenAI, Anthropic, Gemini, VertexAI, Groq, MiniMax, Ollama, LM Studio, LiteLLM, Claude Code - `embeddings.py`: Embedding generation (local sentence-transformers or TEI) - `cross_encoder.py`: Reranking (local or TEI) - `entity_resolver.py`: Entity extraction and normalization @@ -102,7 +107,7 @@ cd hindsight-control-plane && npm run dev - `reranking.py`: Cross-encoder reranking ### API Layer (hindsight-api-slim/hindsight_api/api/) -- `http.py`: FastAPI HTTP routers (~80KB) for all REST endpoints +- `http.py`: FastAPI HTTP routers for all REST endpoints - `mcp.py`: Model Context Protocol server implementation Main operations: @@ -174,6 +179,8 @@ Key tables: `banks`, `memory_units`, `documents`, `entities`, `entity_links` **After completing any implementation work, run `/code-review`** to verify your changes against project standards (missing tests, dead code, type safety, etc.). Fix any "must fix" issues before considering the task done. +**MANDATORY: Run `/code-review` before pushing code or creating a pull request.** Do not push or create a PR until all "must fix" issues are resolved. + ### Memory Banks - Each bank is an isolated memory store (like a "brain" for one user/agent) - Banks have dispositions (skepticism, literalism, empathy traits 1-5) affecting reflect @@ -204,6 +211,17 @@ When adding or modifying parameters in the dataplane API (hindsight-api), you mu - Update the client type definition in `lib/api.ts` - Update any UI components that need to use the new parameter +### Adding New Integrations + +Every new integration in `hindsight-integrations/` must satisfy all of the following before it can be merged: + +1. **Tests are required** — tests must simulate or exercise the external system (mock the framework's interfaces and verify the integration actually calls Hindsight correctly). Pure unit tests of helper functions are not sufficient. +2. **CI job** — add a test job in `.github/workflows/test.yml` following the existing pattern (e.g., `test-crewai-integration`). The job must build, install deps, and run `uv run pytest tests -v`. Also add the integration to `detect-changes` outputs so it only runs when its files change. +3. **Release process** — add the integration name to the `VALID_INTEGRATIONS` array in `scripts/release-integration.sh` so it can be released via the standard release workflow. +4. **Follow project code standards** — Python style, type safety, no raw dicts for structured data, no multi-item tuple returns (see `.claude/skills/code-review/SKILL.md`). + +If any of these are missing, the integration is incomplete and must not be pushed or merged. + ### Adding New API Configuration Flags Configuration follows a hierarchical system: **Global (env vars) → Tenant (via extension) → Bank (database)**. @@ -216,17 +234,17 @@ Fields must be categorized as either **hierarchical** (can be overridden per-ten - Add `ENV_*` constant for the environment variable name (e.g., `ENV_MY_SETTING = "HINDSIGHT_API_MY_SETTING"`) - Add `DEFAULT_*` constant for the default value - Add field to `HindsightConfig` dataclass with type annotation - - **Mark as hierarchical or static** by adding to `_HIERARCHICAL_FIELDS` set (hierarchical) or leaving it out (static) + - **Mark as configurable** by adding to `_CONFIGURABLE_FIELDS` set if the field should be overridable per-tenant/bank via API - Add initialization in `from_env()` method ```python - # Hierarchical field (can be overridden per-bank) - _HIERARCHICAL_FIELDS = { + # Configurable field (can be overridden per-tenant/bank via API) + _CONFIGURABLE_FIELDS = { ..., - "my_setting", # Add here for hierarchical + "my_setting", # Add here for configurable } - # Static field - just don't add to _HIERARCHICAL_FIELDS + # Static field - just don't add to _CONFIGURABLE_FIELDS ``` 2. **main.py** (`hindsight-api-slim/hindsight_api/main.py`):