Ak Dev Review PR

Review a Pull Request Against Agent Kernel Practices

Use this skill when asked to review a specific pull request by number or URL. It fetches the PR with the GitHub CLI (gh), reviews the delta against Agent Kernel's documented practices, and pushes the verified findings to the PR as a single review with inline comments.

This skill is for reviewing someone else's PR, not for reviewing your own uncommitted working-tree changes.

Goal

Produce a thorough, low-noise review grounded in Agent Kernel's own standards — architecture, code quality, and testing skills — and publish it on the PR so the author can act on it, without duplicating feedback that is already on the PR.

Inputs

• PR identifier (required): a number (342), a URL, or owner/repo#number. If only a number is given, resolve the repository from the origin remote of the current checkout.
• Optional scope hints: the requester may narrow the review ("focus on the session store changes") — honor them, but still flag anything clearly dangerous outside that scope.

If no PR identifier can be determined, stop and ask for one. Do not guess.

Step 1: Fetch the PR Context

Gather everything before forming opinions:

# Metadata: title, body, author, base/head, state, linked issues
gh pr view <N> --json number,title,body,author,baseRefName,headRefName,state,files,additions,deletions,url

# The full diff
gh pr diff <N>

# Existing discussion and review comments — needed for dedupe in Step 5
gh pr view <N> --comments
gh api repos/{owner}/{repo}/pulls/<N>/comments --paginate

# CI status
gh pr checks <N>

Then make the PR head readable locally without touching the developer's working tree:

git fetch origin pull/<N>/head:refs/remotes/pr/<N>
git show pr/<N>:<path>          # read any file at the PR head

Never run gh pr checkout — it would switch the developer's branch.

Read the full content of every changed source file at the PR head, not just the diff hunks. A diff hunk without surrounding context is the main source of false-positive review comments.

While listing the changed files, check whether the PR includes spec documents — any added or modified design.md, spec.md, or plan.md under docs/specs/<issue-number>-<short-title>/ (case-insensitive; also match a bare spec.md elsewhere from before the staged layout). If any exists, Step 3 is mandatory and runs before any code is reviewed.

Step 2: Load the Standards to Review Against

Load these skills and use them as the review rubric — do not review from memory:

1. ak-dev-architecture — always. Design principles, core abstractions, execution flow, directory structure, and the House Patterns for New Features section (pluggable by default, config reuse over new knobs, classes over scripts) that every new feature is reviewed against.
2. ak-dev-code-quality — always. Formatting, typing, logging, Python style (including the classes-not-scripts and configuration-field rules), commit conventions, PR guidelines.
3. ak-dev-testing-conventions — always. Test patterns, async testing, mocking, CI workflows.

Then route from the PR's changed file paths to the specialized skills. Take the file list from Step 1 and load every skill whose paths the PR touches:

A PR can match several rows — load every matching skill. If the PR touches one of these areas only incidentally (e.g. a mechanical rename brushing trace/), a skim of the skill's checklist is enough; when the PR adds or substantially modifies that kind of component, walk the guide's checklist step by step and flag every step the PR skipped (missing factory registration, missing config section, missing optional-dependency extra, missing exports, missing tests, missing example).

These skills are the source of truth for what "complete" means in each area. When a finding concerns one of these areas, cite the specific checklist step or convention from the loaded skill, not a general impression.

Step 3: Review the Spec Documents First (When the PR Contains Any)

A change is specified in up to three staged documents under docs/specs/<issue-number>-<short-title>/ (per ak-dev-write-spec): design.md — concise point-form requirements, reviewed and approved first; spec.md — detailed implementation spec that follows the approved design; plan.md — concise iteration breakdown of the implementation. A PR may contain any subset: a design.md-only PR is a design review cycle, a later PR adds spec.md and/or plan.md, and an implementation PR may carry all three alongside the code.

If the PR adds or modifies any of these documents, review them before reading any implementation code, so the spec set is judged on its own merits rather than rationalized from the code. Load ak-dev-write-spec — its per-document structure, staged-process rules, and completeness checklists are the rubric.

Judge each document at its own stage's altitude — do not demand implementation detail from a design spec or re-litigate approved design in a plan:

• design.md: point-form, hierarchical, and complete — every requirement present, each point atomic and concrete enough to test. Prose paragraphs, implementation detail (belongs in spec.md), more than one diagram, or silent decisions on things that should be open questions are findings. This document is optimized for fast expert review — verbosity is a defect, not thoroughness.
• spec.md: the full completeness rubric applies — behavior, configuration, error handling, edge cases, and the "details specs routinely omit" checklist (exception scopes, concurrency contracts, per-operation cost, naming consistency, riskiest-consumer test coverage, absolute-claim audits, config/data compatibility). Every design.md requirement must be covered; a deviation from design.md that isn't explicitly called out is a finding.
• plan.md: orders the spec without restating it — every spec.md component appears in exactly one iteration, each iteration leaves the branch working and testable, and the tests and docs/skills-sync iterations are present. Restated spec content or missing final iterations are findings.

Check the staging itself: a PR that introduces spec.md alongside a brand-new, never-reviewed design.md skips the design review cycles the process exists for — raise it as a [question] unless the PR description says the design was approved elsewhere.

Then, across whichever documents the PR contains:

1. Verify the documents' factual claims against the base branch — file:line citations, config defaults, "appears in N places" counts, and described behaviors. An unverified claim that turns out false is a finding; a spec whose claims all verify deserves saying so in the summary.
2. Review each document itself and record findings on it like any other file:
   • Completeness at its stage's altitude (see above) — or only the happy path?
   • Consistency with Agent Kernel: does the design it describes respect the principles in ak-dev-architecture (framework-agnostic core, adapter pattern, config via AKConfig, pluggable interfaces, coupling direction)?
   • House patterns: is every external-system touchpoint pluggable (ABC + factory + dotted-path BYO, even with one backend)? Does the design name the existing AKConfig models it reuses and justify every new field, and does it let already-configured components enable the feature where that applies instead of adding enabled/type knobs? Are the components classes with one responsibility each? A design that departs from one of these without saying so is a finding; one that says so with a reason is a [question] at most.
   • Internal consistency: no contradictory requirements, undefined terms, or references to components that don't exist.
   • Cross-document consistency: spec.md covers every design.md requirement; plan.md covers every spec.md component. (Only check against documents that exist — on the base branch or in this PR.)
   • Testability: are the stated behaviors concrete enough to verify?
3. Extract a requirements checklist from the documents — every "must/should/will" statement, config key, interface, and named behavior, from design.md and spec.md alike.
4. Then use that checklist as an additional review rubric for the implementation (dimension 6 below) — this applies only when the PR contains implementation code; for spec-only PRs, dimension 6 is skipped and the report says so.

Spec findings anchor to lines of the document (design.md, spec.md, or plan.md) in the diff like any other inline comment; spec-vs-implementation gaps anchor to the implementation line where the deviation lives (or to the spec line if nothing was implemented at all).

Step 4: Review Dimensions

Evaluate the delta on each dimension. For each finding, record: file, line (in the PR head), severity, what is wrong, and why — citing the specific principle or convention it violates.

1. Architecture & design

• Framework-agnostic core: no framework-specific imports or logic in ak-py/src/agentkernel/core/ — framework code belongs in framework/<name>/ adapters.
• Coupling direction: integrations, deployment adapters, and API layers may depend on core; core must never import from them.
• Adapter pattern: new framework/provider code implements the established base interfaces (Agent, Runner, Module, AttachmentStore, BaseTrace, guardrail hooks) rather than inventing parallel abstractions.
• Config-driven behavior: new knobs go through AKConfig (Pydantic, YAML/env with AK_ prefix), not module-level constants or ad-hoc os.environ reads.
• Session lifecycle correctness: session state mutations happen inside the session context; transient per-request data uses v_cache, cross-request data uses nv_cache; no state stored on module globals.
• Plugin interfaces: pluggable components are registered through the existing factories/builders, not special-cased with if/else chains in core.
• Config surface cohesion: when a component already has a canonical settings surface (an object-typed variable — Terraform's rest_service/agent_runner/queue_config/scaling_config; an AKConfig Pydantic section; a settings dataclass; builder kwargs), new config for that component belongs on it, not as a disconnected sibling variable/parameter/env var. This fragments one component's config across two places — a defect even if the sibling wires through and plans/type-checks fine. Check every layer the PR touches: a root module and the submodule it calls can each have their own canonical object for the same service, and either can independently fragment (a root fixing its rest_service object doesn't guarantee the submodule's identically-shaped object stays cohesive too). A flat sibling is legitimate only when the PR/spec states a concrete reason it can't be a nested field; otherwise it's a finding.
• Pluggable by default: a new touchpoint with an external system, backend, or provider ships as an ABC plus adapters behind a factory in the core/util/factory.py shape (if/elif real imports for built-ins, require_extra on optional SDKs, resolve_dotted for bring-your-own). A single hard-wired implementation with no interface or BYO path, or core code branching on backend names, is a [blocker]-level architecture finding unless the spec or PR description justifies it.
• Config reuse, no excessive knobs: every new AKConfig field is checked against the existing models. Flag a new block that duplicates an existing shape instead of reusing it (_QueuesConfig, _ResponseStoreConfig, the shared connection models, or a defaults-only subclass of one); an enabled flag or type selector added where already-configured components could enable or select the feature implicitly (the session backend providing the WebSocket connection store is the model); and any field with no reader in the PR. Every surviving field needs a real description and a default that keeps existing YAML and AK_* env vars valid.
• Classes, not scripts: feature logic is in classes with one responsibility each (ABC, backends, *Factory, *Manager/*Handler/*Runner, Pydantic models) with state on instances. A chain of module-level functions passing state through arguments, a main()-style wiring function, or mutable module globals is a finding; small stateless shared utilities and the plain tool functions bound by tool builders are the accepted exceptions.

2. Correctness

• Async correctness: no blocking I/O in async paths, no forgotten await, no unguarded shared state.
• Error handling: failures surface meaningfully; no silent except: pass; resources cleaned up in finally.
• Concurrency: anything touching Runtime, session stores, or streaming respects the locking model described in ak-dev-architecture.
• Behavior matches the PR description; edge cases in the changed logic (empty inputs, missing config, store misses) are handled.