code-review-and-quality

Files (1)

• SKILL.md

SKILL.md (first lines)

# Code Review and Quality

## Overview

Multi-dimensional code review with quality gates. Every change gets reviewed before merge — no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance.

This skill is the methodology behind, and complements, the repo's automation:
- the built-in **`/code-review`** slash command (diff-focused correctness + cleanup pass),
- the built-in **`/security-review`** slash command (security-focused pass — see the `security-and-hardening` sibling skill for the controls),
- the **`/opsx:address-review`** loop, which applies PR-comment feedback against an OpenSpec change.

Use those for the mechanics; use this skill for the standard you hold the change to.

**The approval standard:** Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — the goal is continuous improvement. Don't block a change because it isn't exactly how you would have written it. If it improves the codebase, follows the conventions of the touched language (idiomatic Python / Go / TypeScript), and upholds the hard invariants in `openspec/project.md`, approve it.

## When to Use

- Before merging any PR or change (including before `/opsx:ship` opens a PR)
- After completing a feature implementation via `/opsx:ship-code`
- When another agent or model produced code you need to evaluate
- When refactoring existing packages
- After any bug fix (review both the fix and the regression test — see `debugging-and-error-recovery`)

## The Five-Axis Review

Every review evaluates code across these dimensions:

### 1. Correctness

Does the code do what it claims to do?

- Does it match the OpenSpec delta / task requirements?
- Are edge cases handled (None/nil, empty list/dict, zero values, boundary values)?
- Are error paths handled? Exceptions caught at the right boundary (Python); every returned `error` checked and wrapped with `%w` where inspected (Go); rejected promises handled (TS). Errors are not swallowed.
- Does it pass the resolver-selected gates? Did the DB-dependent pytest actually run against pgvector (`DATABASE_URL`/`TEST_DATABASE_URL` set), or silently skip?
- Are there off-by-one errors, **race conditions**, or state inconsistencies? For Go concurrency, check with `go test -race`. For async Python, check the `ASYNC`-lint findings and that awaited work isn't dropped.
- Does it preserve the hard invariants — `temperature == 0` on synthesize/cite/filter, `refuse_if: no_citations` in the filter stage, ACL enforced server-side in `retrieve_kb`?

### 2. Readability & Simplicity

Can another engineer (or agent) understand this code without the author explaining it?

- Are names idiomatic for the language and consistent with the package? (No `temp`, `data`, `result` without context; clear module/function names; no stutter.)
- Is the control flow straightforward? Prefer early returns and guard clauses over deep nesting.
- Is the code organized logically (related code grouped, clear package boundaries — `packages/*` vs `apps/*` vs `apps/portal`)?
- Are there "clever" tricks (reflection, metaprogramming, unsafe, channel gymnastics) that should be simplified?
- **Could this be done in fewer lines?** (1000 lines where 100 suffice is a failure.)
- **Are abstractions earning their complexity?** Don't add a generic helper or a new lobe/path until a second/third caller needs it. The BotPolicy interpreter is the one deliberate generalization — a new bot behavior is a config row + prompt file, not a new interpreter branch; match that level, don't out-abstract it.
- Would a doc comment clarify non-obvious intent? (But don't comment obvious code.)
- Any dead code: unused exports, no-op assignments, backwards-compat shims, `# removed` / `// removed` comments?

### 3. Architecture

Does the change fit the system's design?

- Does it follow existing patterns or introduce a new one? If new, is it justified?
- Does it respect the package boundaries (`packages/{rag-core,agent-core,ingest-core,llm-transport,...}`, `apps/*`, `apps/portal`)? Does the dependency flow point the right way?
- Is there code duplication that should be shared?
- Is a new bot/capability expressed as a **new `BotVersion`/`BotPolicy` row + prompt file** (not code), and a new engine capability as a **lobe/path registry row** (not an interpreter branch)? If a "new bot" needs an interpreter change, the abstraction is wrong.
- Does it honor the framework boundaries: **no LangChain/LangGraph/Haystack/LlamaIndex** (`anthropic-sdk-python` directly), **MCP as the internal tool boundary** (`kb.*` contract, embedded transport, ACP/A2A out of the retrieval loop), the **compression invariant** (only `memo_schema_ref`-shaped objects cross stage boundaries)?
- Is **OpenAPI the API contract** — backend generates the spec, portal types are *generated*, never hand-written?
- Does the change belong to an OpenSpec change? Non-trivial behavior changes should have a spec delta, not just code.

### 4. Security

For detailed security guidance, see the `security-and-hardening` sibling skill and the built-in `/security-review` command. In review specifically, check:

- Is external input (chat messages, ingested KB documents, connector payloads, portal requests) validated at the boundary before use, and treated as **untrusted data, not instructions**?
- Is **ACL enforced server-side inside `retrieve_kb`**, with caller identity inherited from the request and **never trusted from the model**?
- Is the system **multi-tenant by default** — does every query, cache key, and log line carry `tenant_id`, with no cross-tenant joins and a cache key that includes the ACL-cohort hash?
- Are SQL queries parameterized (`$1`/`%s`/bound params), never string-concatenated?
- Are secrets kept out of code, logs, and version control? No API keys, tokens, or unsealed credentials in log lines or error strings.
- Are **versions append-only** — is the change adding a new child with a parent pointer rather than mutating a prior `BotVersion`/`KBVersion`/golden set/trace in place?
- Does the **filter stage refuse rather than emit ungrounded claims** (`refuse_if: no_citations`), and is `temperature == 0` held on synthesize/cite/filter so output is deterministic and not coaxable?
- Does the **compression invariant** hold — raw KB chunks never leave a sub-agent across a stage boundary?

### 5. Performance

Does the change introduce performance problems?

- Any N+1 query patterns against pgvector/Postgres? Prefer a single query / `IN` over a loop of queries. Is the vector search bounded (top-k) and the ACL filter pushed into the query?
- Any unbounded loops or unconstrained data fetching (e.g. ingesting a whole KB into memory)?
- Any blocking call inside an async path or request handler that should have a timeout/context? Latency matters — the product gate is **p95 <= 12s**.
- Any task/worker leaks — an asyncio task or goroutine started without a way to stop it, a channel never drained?
- Any large allocation or redundant embedding computation in a hot path?
- Any missing pagination on list endpoints?

## Change Sizing

Small, focused changes are easier to review, faster to merge, and safer to deploy. Target these sizes:

```
~100 lines changed   → Good. Reviewable in one sitting.
~300 lines changed   → Acceptable if it's a single logical change.
~1000 lines changed  → Too large. Split it.
```

A single OpenSpec change should ideally map to a reviewable diff. If a change's tasks add up to a 1000-line diff, the change itself was probably too big — split the proposal.

**What counts as "one change":** A single self-contained modification that addresses one thing, includes related tests in the touched package's language, and keeps the resolver-selected gates green after submission. One part of a feature — not the whole feature.

**Splitting strategies when a change is too large:**

| Strategy | How | When |
|----------|-----|------|
| **Stack** | Submit a small change, start the next based on it | Sequential dependencies |
| **By package** | Separate changes for `packages/rag-core` vs `apps/backend` vs `apps/portal` etc. | Cross-cutting concerns |
| **Horizontal** | Add the shared contract/migration first, then consumers | Layered work (e.g. new `kb.*` MCP tool, OpenAPI schema) |
| **Vertical** | Break into smaller full-pipeline slices | Feature work (ingest → retrieve_kb → synthesize/cite/filter → API) |

**When large changes are acceptable:** Complete file deletions and automated refactoring (e.g. `ruff format` / `gofmt` / rename) where the reviewer only needs to verify intent, not every line. A new Alembic migration plus its generated touchpoints, or regenerated OpenAPI/portal types, can be large but is still one logical change.

**Separate refactoring from feature work.** A change that refactors an existing package and adds new behavior is two changes — submit them separately. Small cleanups (a rename, a format pass) can be included at reviewer discretion.

## Change Descriptions

Every change needs a description that stands alone in version-control history.

**First line:** Short, imperative, standalone. "Enforce tenant ACL server-side in retrieve_kb" not "ACL stuff." Informative enough that someone searching history understands it without the diff.

From minhlucvan/emdesign, Apache-2.0.

More from minhlucvan/emdesign