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/<name>/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
This commit is contained in:
parent
1dbbe39ea1
commit
bdb33c58d1
2 changed files with 48 additions and 18 deletions
|
|
@ -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-<name>-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
|
||||
|
|
|
|||
40
CLAUDE.md
40
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`):
|
||||
|
|
|
|||
Loading…
Reference in a new issue