From f8f62030e312cbcba0f1d32208e1b03939c5fe93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 31 Mar 2026 10:21:25 +0200 Subject: [PATCH] Add /code-review skill for automated code quality checks (#805) * 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. --- .claude/skills/code-review/SKILL.md | 184 ++++++++++++++++++++++++++++ .gitignore | 3 +- CLAUDE.md | 49 +------- 3 files changed, 191 insertions(+), 45 deletions(-) create mode 100644 .claude/skills/code-review/SKILL.md diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md new file mode 100644 index 00000000..dc8f326f --- /dev/null +++ b/.claude/skills/code-review/SKILL.md @@ -0,0 +1,184 @@ +--- +name: code-review +description: Review changed code against project standards. Checks for missing tests, dead code, type safety, lint issues, and coding conventions. Run after completing any implementation work. +user_invocable: true +--- + +# Code Review + +Review all changed code against the project's quality standards and coding conventions. + +## Code Standards + +Read and internalize these standards before writing code. The review steps below verify compliance. + +### Python Style +- Python 3.11+, type hints required +- Async throughout (asyncpg, async FastAPI) +- 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 + +### Type Safety with Pydantic Models +**NEVER use raw `dict` types for structured data.** Always use Pydantic models: +- Use Pydantic `BaseModel` for all data structures passed between functions +- 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 + +```python +# BAD - error-prone dict access +def process(data: dict) -> str: + return data.get("name", "") # No validation, silent failures + +# GOOD - typed and validated +class UserData(BaseModel): + name: str + created_at: datetime + + @field_validator("created_at", mode="before") + @classmethod + def ensure_tz_aware(cls, v): + if isinstance(v, str): + v = datetime.fromisoformat(v.replace("Z", "+00:00")) + if v.tzinfo is None: + return v.replace(tzinfo=timezone.utc) + return v + +def process(data: UserData) -> str: + return data.name # Type-safe, validated at construction +``` + +### TypeScript Style +- Next.js App Router for control plane +- Tailwind CSS with shadcn/ui components + +### Code Comments +- **Always comment non-trivial technical decisions** with the reasoning behind the choice. If someone would ask "why is it done this way?", there should be a comment. +- **Keep comments up to date with history** — when changing an approach, update the comment to explain what was tried before and why it was changed. Comments serve as a tracker of previous implementations that likely had problems. +- Don't comment obvious code — only where the "why" isn't self-evident from the code itself. + +```python +# BAD - no context for future readers +results = await asyncio.gather(*tasks, return_exceptions=True) + +# GOOD - explains the non-obvious choice +# Use return_exceptions=True to avoid cancelling sibling tasks on failure. +# Previously we used TaskGroup but it cancelled all tasks when one failed, +# causing partial writes that left orphaned entity links (see #412). +results = await asyncio.gather(*tasks, return_exceptions=True) +``` + +### Branch Hygiene +- **Always start new feature branches from `origin/main`** — rebase to ensure a clean base. +- **Only include commits relevant to the PR/branch/feature** — no unrelated changes. If the branch contains commits that don't belong, they must be removed before merging. + +### General Principles +- Don't add features, refactor code, or make "improvements" beyond what was asked +- Don't add unnecessary error handling for impossible scenarios +- Don't create helpers or abstractions for one-time operations +- No backwards-compatibility hacks (unused vars, re-exports, "removed" comments) +- Three similar lines of code is better than a premature abstraction + +## Review Steps + +### 1. Check branch hygiene + +- Run `git log --oneline main..HEAD` to list all commits on the branch. +- Verify every commit is relevant to the feature/PR. Flag any unrelated commits. +- Check the branch is based on a recent `origin/main` (no stale base). + +### 2. Identify changed files + +Run `git diff --name-only HEAD` (unstaged) and `git diff --cached --name-only` (staged) to get all changed files. If there are no local changes, diff against the base branch using `git diff main...HEAD --name-only` and `git diff main...HEAD` to review all commits on the current branch. + +### 3. Run linters + +```bash +./scripts/hooks/lint.sh +``` + +Report any failures. Do NOT fix them yourself — just report. + +### 4. Check for dead code + +For each changed Python file, check for: +- Unused imports (Ruff should catch these, but verify) +- Functions/methods/classes that were added but are never called from anywhere +- Variables assigned but never read +- Commented-out code blocks that should be removed + +For each changed TypeScript file, check for: +- Unused imports +- Unused variables or functions +- Commented-out code + +### 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 +- **Missing type hints** on function parameters and return types +- **Missing `@field_validator`** for datetime fields that should be timezone-aware + +### 6. Check for missing tests + +For each new or significantly changed function/endpoint/class: +- Check if there is a corresponding test addition or update +- New API endpoints MUST have integration tests +- New utility functions MUST have unit tests +- Bug fixes SHOULD have a regression test + +Flag any new logic that lacks test coverage. + +### 7. Check API consistency + +If any files in `hindsight-api-slim/hindsight_api/api/` were changed: +- Were the OpenAPI specs regenerated? (`./scripts/generate-openapi.sh`) +- Were the client SDKs regenerated? (`./scripts/generate-clients.sh`) +- Were the control plane proxy routes updated? (`hindsight-control-plane/src/app/api/`) + +### 8. Check code comments + +For each non-trivial change: +- **New non-obvious logic** — is there a comment explaining the reasoning? +- **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 + +Check the diff for violations of the standards listed above: +- Python files at project root (not allowed) +- Missing async patterns (should be async throughout) +- Pydantic models for request/response +- Line length > 120 chars +- New features/code beyond what was asked (over-engineering) +- Unnecessary error handling for impossible scenarios +- Premature abstractions or speculative helpers +- Backwards-compatibility hacks (unused vars, re-exports, "removed" comments) + +### 10. Report findings + +Present a clear summary organized by severity: + +**Must fix** — issues that will break CI or violate hard project rules: +- Unrelated commits on the branch +- Lint failures +- Missing type hints on public functions +- Raw dict usage for structured data +- Missing tests for new endpoints + +**Should fix** — issues that hurt code quality: +- Dead code / unused imports missed by linter +- Missing tests for non-trivial utility functions +- Over-engineering beyond the task scope + +**Note** — observations that may or may not need action: +- API changes that might need client regeneration +- Patterns that deviate from nearby code style + +For each finding, include the file path, line number, and a brief explanation. + +Do NOT auto-fix any issues. Report all findings and let the user decide what to address. If there are no findings, confirm the code looks good. diff --git a/.gitignore b/.gitignore index 1c046c31..e980641d 100644 --- a/.gitignore +++ b/.gitignore @@ -50,7 +50,8 @@ hindsight-dev/benchmarks/perf/results/ benchmarks/results/ hindsight-cli/target hindsight-clients/rust/target -.claude +.claude/* +!.claude/skills/ whats-next.md TASK.md # Changelog is now tracked in hindsight-docs/src/pages/changelog.md diff --git a/CLAUDE.md b/CLAUDE.md index a8560c8a..66d590b1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -164,11 +164,15 @@ Key tables: `banks`, `memory_units`, `documents`, `entities`, `entity_links` ## Key Conventions ### Code Quality + +**Before writing code, read `.claude/skills/code-review/SKILL.md`** for the full coding standards (Python style, type safety, TypeScript style, general principles). + **Always run the lint script after making Python or TypeScript/Node changes:** ```bash ./scripts/hooks/lint.sh ``` -This runs the same checks as the pre-commit hook (Ruff for Python, ESLint/Prettier for TypeScript). + +**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. ### Memory Banks - Each bank is an isolated memory store (like a "brain" for one user/agent) @@ -200,49 +204,6 @@ 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 -### Python Style -- Python 3.11+, type hints required -- Async throughout (asyncpg, async FastAPI) -- 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 - -### Type Safety with Pydantic Models -**NEVER use raw `dict` types for structured data.** Always use Pydantic models: -- Use Pydantic `BaseModel` for all data structures passed between functions -- 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 - -```python -# BAD - error-prone dict access -def process(data: dict) -> str: - return data.get("name", "") # No validation, silent failures - -# GOOD - typed and validated -class UserData(BaseModel): - name: str - created_at: datetime - - @field_validator("created_at", mode="before") - @classmethod - def ensure_tz_aware(cls, v): - if isinstance(v, str): - v = datetime.fromisoformat(v.replace("Z", "+00:00")) - if v.tzinfo is None: - return v.replace(tzinfo=timezone.utc) - return v - -def process(data: UserData) -> str: - return data.name # Type-safe, validated at construction -``` - -### TypeScript Style -- Next.js App Router for control plane -- Tailwind CSS with shadcn/ui components - ### Adding New API Configuration Flags Configuration follows a hierarchical system: **Global (env vars) → Tenant (via extension) → Bank (database)**.