code-review

---
name: code-review
description: "Senior-level code review for the agent platform — architecture and boundaries first, then correctness, security, testability, and simplicity. Outputs a structured review."
generated: true
source: .ai/skills/code-review.md
---

<!--
This file is generated by tools/skills.
Do not edit this file directly.
Edit .ai/skills/code-review.md and run `make generate-ai`.
-->

# Skill: Code Review

## Purpose

Perform senior-level code review on changes in this repository: judge
architecture and design first, then correctness, security, testability, and
simplicity. The goal is to protect the project's boundaries and quality, not to
nitpick syntax.

## When to Use This Skill

- Reviewing a pull request, diff, or another agent's proposed change.
- Self-reviewing your own change before declaring a task complete.
- Evaluating whether a change is safe to merge or must be sent back.

## Files to Read First

- `AGENTS.md` (especially §3 non-negotiable architecture rules).
- `docs/ARCHITECTURE.md` and the relevant layer docs
  (`RUNTIME_LIFECYCLE.md`, `YAML_SPEC.md`, `PROMPT_RENDERING.md`,
  `SIDECAR_CONTEXT_AUTH.md`, `MCP_AND_TOOLS.md`).
- `docs/adr/` for any contract the change touches.
- `.ai/skills/architecture-review.md` if the change affects architecture.
- The task file in `tasks/` the change claims to implement.

## Core Principles

- **Review architecture before syntax.** A clean diff in the wrong layer is
  still wrong.
- **Boundaries are non-negotiable.** Validation, compilation, runtime, prompt
  rendering, plugin context/access, and tools stay separated.
- **Security is enforced outside prompts.** Prompt wording is never a boundary.
- **Simplicity wins.** Reject unnecessary abstraction; prefer the smallest
  design that works.
- **Behavior must be tested**, and tests must assert behavior, not internals.

## Process

1. **Understand intent.** Read the task/PR description and confirm the change is
   in scope for that task. Out-of-scope churn is a finding.
2. **Architecture & separation of concerns.** Confirm code lives in the correct
   `src/agentplatform/<layer>` package and does not cross boundaries (e.g. the
   runtime reading raw YAML, or client/business logic leaking into the runtime).
3. **Lifecycle/state.** Verify runtime/startup state is separated from
   per-request state: nothing request-scoped on `RuntimeEngine` or the compiled
   graph; per-request data lives on `ExecutionContext`.
4. **Public interface.** Check that public functions/classes have clear, typed,
   minimal signatures and stable contracts; private details are not leaked.
5. **Errors.** Confirm errors are typed/actionable and name what went wrong
   (e.g. which YAML key, which missing prompt variable), not bare exceptions.
6. **Security.** Confirm permission/tool-policy enforcement happens at the
   tool/data layer; injected params can't be overridden; no secrets hardcoded;
   secrets redacted in traces/logs.
7. **Testability & tests.** Confirm the code is testable (DI, no hidden
   globals) and that tests cover behavior and edge/negative cases, not private
   implementation details.
8. **Simplicity & abstractions.** Flag premature abstractions, clever code, and
   layers collapsed "for convenience".
9. **Backward compatibility.** If a public contract (YAML schema, plugin
   contract, API shape) changed, require an ADR and check for breakage.
10. **Write the structured report** (below).

## Checklist Before Finishing

- [ ] Change is in the correct layer and respects boundaries.
- [ ] No request state on `RuntimeEngine`/compiled graph.
- [ ] Runtime does not execute raw YAML; validation precedes compilation.
- [ ] Public interfaces are clean, typed, and minimal.
- [ ] Errors are actionable and typed.
- [ ] Security enforced outside prompts; no hardcoded secrets; secrets redacted.
- [ ] Code is testable; tests cover behavior and negatives.
- [ ] No unnecessary abstraction; code is as simple as possible.
- [ ] Contract changes have an ADR; backward compatibility considered.
- [ ] `make check` passes (or you state why it can't run).

## Common Mistakes to Avoid

- Reviewing formatting/style while missing a layer or boundary violation.
- Approving request state stored on a long-lived object.
- Accepting prompt text as a security control.
- Letting tests assert private internals (brittle) instead of behavior.
- Waving through "small" contract changes without an ADR.
- Approving speculative abstractions that aren't needed yet.

## Expected Final Report

Produce the review in exactly this structure:

1. **Summary** — what the change does and overall impression.
2. **Blocking issues** — must fix before merge.
3. **Non-blocking issues** — should fix, not gating.
4. **Architecture concerns** — boundary/layer/lifecycle observations.
5. **Security concerns** — enforcement, secrets, redaction.
6. **Testing gaps** — missing/weak tests, missing negatives.
7. **Suggested improvements** — optional polish.
8. **Final recommendation** — Approve / Approve with changes / Request changes.