api-review — pekral/ai-olympus

## Constraints - Apply `@rules/api/general.md` — this skill is the focused review lens for that rule. - Apply `@rules/php/core-standards.md` - Apply `@rules/security/backend.md` — for the error-text and authorization-leak surface of API responses (401/403/404 wording, no internal-detail leak). - If the current project uses Laravel, also apply `@rules/laravel/architecture.md` and `@rules/laravel/laravel.md` — validation belongs in FormRequest / Data Validator, controllers stay slim. - Apply `@rules/reports/general.md` — when the findings are folded into the **GitHub PR comment** by a CR wrapper they stay in canonical English per the rule's *Exception — technical CR findings on the GitHub PR*; a non-technical mirror on a linked issue / JIRA ticket follows the language of the source assignment. A manifest `language.github` overrides both on GitHub (the rule's *Project override — `language.github` in the manifest*). HTTP verbs, status codes, header names, and code identifiers stay verbatim regardless of the surrounding prose language. - Output findings only — no praise, no summary of what was checked. - **Read-only skill** — never modify code, never stage / commit / push, and never run any git write operation. Switching to the relevant branch and `git pull` to read the latest diff are allowed; mutating the working tree or pushing is not. ## Use when - A PR or change set adds or modifies HTTP endpoints, routes, controllers, API Resources, request/response payloads, or status-code handling. - Run as part of every code review via `@skills/code-review/SKILL.md` (Specialized Reviews → Always run). - A consumer-facing API contract needs a design check before release. ## Scope Review only the API surface on the **diff** — never untouched endpoints. Detect the surface from any of: route definitions, controller/`__invoke` request handlers, API Resources / DTOs serialized into responses, FormRequests, `response()` / `abort()` / status-code calls, and `Idempotency-Key` handling. If the diff touches no API surface, return no findings. **This lens owns the generic HTTP contract of an MPP-gated endpoint, never the protocol behind it.** When the CR's payment trigger fires on the same diff on a project that implements MPP (`@skills/code-review/references/specialized-reviews.md` *Specialized Reviews*), `@skills/machine-payments-protocol/SKILL.md` with `MODE=cr` runs alongside this lens and owns protocol conformance — the challenge and receipt shape on the wire, single-use proof, request and body binding, expiry, no side effect before payment, idempotent settlement, and server-side pricing. This lens keeps status-code choice outside the `402` challenge itself, the response envelope, versioning, and idempotency-key mechanics, and never restates a protocol finding in its own words. The two divide the *dimensions* of a payment change, never its lines: a gated endpoint that both settles a replayed receipt a second time and answers a client retry with no `Idempotency-Key` handling carries one protocol finding from the payment lens and one idempotency finding here — a different defect, never this lens's finding restated. ## Core Checks Walk the diff against each pillar of `@rules/api/general.md` and raise one finding per match. ### 1. Contract & consumer orientation - Response leaks internal DB structure — raw column names, surrogate/internal keys, join tables, enum integers, or storage-only fields serialized without a DTO / API Resource boundary. - Inconsistent contract shape for the same concept across endpoints (field casing, date format, pagination shape, error envelope). ### 2. Resource-oriented REST - Action/verb in the endpoint path (`/getUser`, `/createUser`, `/users/{id}/delete`, `/doPayment`) instead of a resource noun + HTTP method. ### 3. HTTP methods & idempotence - Method whose side effects violate its contract — `GET` that mutates state, `PUT`/`DELETE` not idempotent on repetition. - `PUT` used for a partial update or `PATCH` used for a full replacement. ### 4. Idempotency keys - Critical, retry-prone, state-changing operation (payment, transfer, order placement) with no `Idempotency-Key` handling, so a client retry can double-execute. ### 5. Status codes - Imprecise success code — `200` for a creation (`201`), for an async hand-off (`202`), or where `204` (no body) is correct; a body returned alongside `204`. - Error code collapsed into a generic one where a narrower code applies (`400`/`401`/`403`/`404`/`409`/`422`/`429`). - 401-vs-403 inversion — `401` for an authorization failure or `403` for a missing/invalid credential. ### 6. Validation at the trust boundary - Input reaching business logic or the database before schema/business validation runs (trust-boundary bypass). - Validation inlined in the action/controller/model instead of the dedicated boundary layer (FormRequest / Data Validator). - Authorization check missing or running before the input is known to be well-formed. - Error responses that leak identity, resource existence, or internal detail — defer to `@rules/security/backend.md` *Safe Validation & Error Messages* and do not duplicate a finding `@skills/security-review/SKILL.md` already owns. ## Prioritization - Focus on contract defects a consumer would feel: double-charges, wrong status branching, breaking payload shapes, bypassed validation. - Do not report purely cosmetic naming nits. The review has no tier below Moderate (`@rules/code-review/general.md` *Minor findings are not detected*). - Do not propose API features the current scope does not require (YAGNI per `@rules/php/core-standards.md`). ## Report ### Real-Code Grounding for Every Finding (issue #97) Apply the contract in `@rules/code-review/general.md` *Real-Code Grounding for Every Finding (issue #97)* to every finding — **Critical and Moderate alike; no severity is exempt**. On this skill's surface the context to re-read is the enclosing route / controller / FormRequest / API Resource, plus any Service or DTO the Suggested Fix depends on — a contract claim is grounded only when the real route definition and the real response shape were both read. The requirement holds equally for a standalone run (e.g. a pre-release API design check). Findings from this skill fold into the core CR's severity buckets; the Assignment-Declared Test-Only Conditions — Exclusion Gate (`@rules/code-review/general.md` *Assignment-Declared Test-Only Conditions — Exclusion Gate (issue #17)*) is applied by `@skills/code-review/SKILL.md`, not here — trust-boundary / authorization findings from Core Check 6 fall under the gate's security carve-out and are never excludable. Use the severity scale of `@skills/code-review/SKILL.md` so findings fold cleanly into the code review: - **Critical** / **Moderate** — apply the severity declared in `@rules/api/general.md` *CR Severity Rules*. An entry that rule grades below Moderate is not reported. Each finding includes: - location (`file:line`) - risk/impact (the consumer-facing consequence) - the cited rule reference (e.g. `@rules/api/general.md#Resource-Oriented REST`) - concrete fix Each **Critical** and **Moderate** finding additionally includes: - **Faulty Example** — minimal endpoint / route / payload snippet that reproduces the issue (redact secrets/PII) - **Expected Behavior** — single assertable statement (status code, response shape, idempotent outcome, rejection before side effect) - **Test Hint** — one sentence pointing at the test layer (feature/HTTP, integration) and the entry point - **Suggested Fix** — minimal corrected snippet that complies with `@rules/api/general.md`, `@rules/php/core-standards.md`, and on Laravel projects `@rules/laravel/architecture.md`. Use `n/a — <reason>` only when a snippet adds nothing over the one-line fix. These fields exist so `@skills/process-code-review/SKILL.md` can turn each finding into a reproducer test and apply the fix without re-deriving context. ## Output Format Use the template defined in `templates/review-output.md`. Omit any severity section that has no findings; never emit `None.` / `n/a` placeholders. ## Done when - Every API-surface change on the diff has been walked against the six Core Checks. - Findings are grouped by severity with the mandatory reproducer fields on every Critical and Moderate item. - Every published finding was re-grounded in the real, current file per Real-Code Grounding (issue #97). - No code, git, or remote state was modified (read-only).

Constraints

Apply @rules/api/general.md — this skill is the focused review lens for that rule.
Apply @rules/php/core-standards.md
Apply @rules/security/backend.md — for the error-text and authorization-leak surface of API responses (401/403/404 wording, no internal-detail leak).
If the current project uses Laravel, also apply @rules/laravel/architecture.md and @rules/laravel/laravel.md — validation belongs in FormRequest / Data Validator, controllers stay slim.
Apply @rules/reports/general.md — when the findings are folded into the GitHub PR comment by a CR wrapper they stay in canonical English per the rule's Exception — technical CR findings on the GitHub PR; a non-technical mirror on a linked issue / JIRA ticket follows the language of the source assignment. A manifest language.github overrides both on GitHub (the rule's Project override — language.github in the manifest). HTTP verbs, status codes, header names, and code identifiers stay verbatim regardless of the surrounding prose language.
Output findings only — no praise, no summary of what was checked.
Read-only skill — never modify code, never stage / commit / push, and never run any git write operation. Switching to the relevant branch and git pull to read the latest diff are allowed; mutating the working tree or pushing is not.

Use when

A PR or change set adds or modifies HTTP endpoints, routes, controllers, API Resources, request/response payloads, or status-code handling.
Run as part of every code review via @skills/code-review/SKILL.md (Specialized Reviews → Always run).
A consumer-facing API contract needs a design check before release.

Scope

Review only the API surface on the diff — never untouched endpoints. Detect the surface from any of: route definitions, controller/__invoke request handlers, API Resources / DTOs serialized into responses, FormRequests, response() / abort() / status-code calls, and Idempotency-Key handling. If the diff touches no API surface, return no findings.

This lens owns the generic HTTP contract of an MPP-gated endpoint, never the protocol behind it. When the CR's payment trigger fires on the same diff on a project that implements MPP (@skills/code-review/references/specialized-reviews.md Specialized Reviews), @skills/machine-payments-protocol/SKILL.md with MODE=cr runs alongside this lens and owns protocol conformance — the challenge and receipt shape on the wire, single-use proof, request and body binding, expiry, no side effect before payment, idempotent settlement, and server-side pricing. This lens keeps status-code choice outside the 402 challenge itself, the response envelope, versioning, and idempotency-key mechanics, and never restates a protocol finding in its own words. The two divide the dimensions of a payment change, never its lines: a gated endpoint that both settles a replayed receipt a second time and answers a client retry with no Idempotency-Key handling carries one protocol finding from the payment lens and one idempotency finding here — a different defect, never this lens's finding restated.

Core Checks

Walk the diff against each pillar of @rules/api/general.md and raise one finding per match.

1. Contract & consumer orientation

Response leaks internal DB structure — raw column names, surrogate/internal keys, join tables, enum integers, or storage-only fields serialized without a DTO / API Resource boundary.
Inconsistent contract shape for the same concept across endpoints (field casing, date format, pagination shape, error envelope).

2. Resource-oriented REST

Action/verb in the endpoint path (/getUser, /createUser, /users/{id}/delete, /doPayment) instead of a resource noun + HTTP method.

3. HTTP methods & idempotence

Method whose side effects violate its contract — GET that mutates state, PUT/DELETE not idempotent on repetition.
PUT used for a partial update or PATCH used for a full replacement.

4. Idempotency keys

Critical, retry-prone, state-changing operation (payment, transfer, order placement) with no Idempotency-Key handling, so a client retry can double-execute.

5. Status codes

Imprecise success code — 200 for a creation (201), for an async hand-off (202), or where 204 (no body) is correct; a body returned alongside 204.
Error code collapsed into a generic one where a narrower code applies (400/401/403/404/409/422/429).
401-vs-403 inversion — 401 for an authorization failure or 403 for a missing/invalid credential.

6. Validation at the trust boundary

Input reaching business logic or the database before schema/business validation runs (trust-boundary bypass).
Validation inlined in the action/controller/model instead of the dedicated boundary layer (FormRequest / Data Validator).
Authorization check missing or running before the input is known to be well-formed.
Error responses that leak identity, resource existence, or internal detail — defer to @rules/security/backend.md Safe Validation & Error Messages and do not duplicate a finding @skills/security-review/SKILL.md already owns.

Prioritization

Focus on contract defects a consumer would feel: double-charges, wrong status branching, breaking payload shapes, bypassed validation.
Do not report purely cosmetic naming nits. The review has no tier below Moderate (@rules/code-review/general.md Minor findings are not detected).
Do not propose API features the current scope does not require (YAGNI per @rules/php/core-standards.md).

Report

Real-Code Grounding for Every Finding (issue #97)

Apply the contract in @rules/code-review/general.md Real-Code Grounding for Every Finding (issue #97) to every finding — Critical and Moderate alike; no severity is exempt. On this skill's surface the context to re-read is the enclosing route / controller / FormRequest / API Resource, plus any Service or DTO the Suggested Fix depends on — a contract claim is grounded only when the real route definition and the real response shape were both read. The requirement holds equally for a standalone run (e.g. a pre-release API design check).

Findings from this skill fold into the core CR's severity buckets; the Assignment-Declared Test-Only Conditions — Exclusion Gate (@rules/code-review/general.md Assignment-Declared Test-Only Conditions — Exclusion Gate (issue #17)) is applied by @skills/code-review/SKILL.md, not here — trust-boundary / authorization findings from Core Check 6 fall under the gate's security carve-out and are never excludable.

Use the severity scale of @skills/code-review/SKILL.md so findings fold cleanly into the code review:

Critical / Moderate — apply the severity declared in @rules/api/general.md CR Severity Rules. An entry that rule grades below Moderate is not reported.

Each finding includes:

location (file:line)
risk/impact (the consumer-facing consequence)
the cited rule reference (e.g. @rules/api/general.md#Resource-Oriented REST)
concrete fix

Each Critical and Moderate finding additionally includes:

Faulty Example — minimal endpoint / route / payload snippet that reproduces the issue (redact secrets/PII)
Expected Behavior — single assertable statement (status code, response shape, idempotent outcome, rejection before side effect)
Test Hint — one sentence pointing at the test layer (feature/HTTP, integration) and the entry point
Suggested Fix — minimal corrected snippet that complies with @rules/api/general.md, @rules/php/core-standards.md, and on Laravel projects @rules/laravel/architecture.md. Use n/a — <reason> only when a snippet adds nothing over the one-line fix.

These fields exist so @skills/process-code-review/SKILL.md can turn each finding into a reproducer test and apply the fix without re-deriving context.

Output Format

Use the template defined in templates/review-output.md. Omit any severity section that has no findings; never emit None. / n/a placeholders.

Done when

Every API-surface change on the diff has been walked against the six Core Checks.
Findings are grouped by severity with the mandatory reproducer fields on every Critical and Moderate item.
Every published finding was re-grounded in the real, current file per Real-Code Grounding (issue #97).
No code, git, or remote state was modified (read-only).