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

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.