gravitee-io/gravitee-access-management/code-review

Code Review

Purpose

Perform a systematic code review of the current changeset and report actionable findings. By default this is review-only: do not modify code unless explicitly requested.

When to use

Apply this skill:

• After implementing changes, before opening a PR / requesting review / merging
• When explicitly asked to review code
• As the "validation" step at the end of a task

Scope: Identify the current 'staged' changes or the diff between the current branch and the main branch before reviewing.

Priority: If the changeset is large, prioritize Global Standards and Architecture findings first.

Review mode

• Review-only by default: identify issues and propose fixes; stop for confirmation before making changes.
• Cite evidence: For every finding, provide the line number and a snippet of the code in question.
• Prefer project commands from @docs/agent-standards/commands.md (if present); otherwise suggest likely commands and ask before adding/updating commands.md.

Output format

Return results in this structure:

1) Verdict: PASS / NEEDS_CHANGES 2) Top risks (0–3 bullets) 3) Findings

• Blockers (must fix) - Security, blocking calls, missing audit
• Warnings (should fix) - Pattern violations, missing tests
• Nits (optional) - Style, naming, comments

4) Recommended validation commands (reference commands.md if available) 5) Next steps (Task List)

• [ ] Task 1: (e.g., Fix the blockingGet in UserService.java:42)
• [ ] Task 2: (e.g., Run mvn openapi-generator:generate)
• [ ] Task 3: (e.g., Add audit log to deleteUser)

Review checklist

1. Global Standards (Apply to ALL changes)

Security & Compliance:

• [ ] No secrets/tokens/PII in logs (prefer structured logs: key=value)
• [ ] No full request/response bodies logged
• [ ] Authorization/validation fail closed (deny by default)
• [ ] RFC compliance verified for OAuth/OIDC/IAM work (RFC 6749, 7519, 7636, 8252, 8707)
• [ ] No exposure of internal implementation details in error messages

Diff Integrity:

• [ ] Small, reviewable diffs; no unrelated refactors/reformatting
• [ ] Changes focused on stated goal
• [ ] No commented-out code or debug statements

Code Quality:

• [ ] US English for comments, logs, user-facing text
• [ ] Method/class visibility not widened without justification
• [ ] Error handling preserves business intent (expected 4xx not wrapped into 5xx)
• [ ] No redundant null checks (prefer Optional or @NotNull)
• [ ] No log pollution (avoid logging entire DTOs/entities with PII)

2. Architecture & Patterns (The "How")

Layering:

• [ ] Respects layering: Repository → Service → Resource
• [ ] Resources handle HTTP + permissions; Services handle business logic
• [ ] No business logic leaking into Resource layer

Reactive Integrity:

• [ ] No blockingGet() or other blocking calls in reactive paths
• [ ] RxJava used correctly: Single/Maybe for I/O, Completable for side-effects
• [ ] Reactive chains properly composed

Audit Trail (MANDATORY for C/U/D):

• [ ] Audit logging present for all CREATE/UPDATE/DELETE operations
• [ ] Audit logging implemented in Service layer (not Resource)
• [ ] Resources pass authenticated User principal to service methods
• [ ] Uses dedicated AuditBuilder per entity type

Pattern Adherence:

• [ ] Follows established patterns (cite similar implementations found)
• [ ] No invented endpoints, config keys, or behaviours
• [ ] Reuses existing utilities/helpers where applicable

3. Data & API Contracts (The "Mechanics")

API Contracts:

• [ ] OpenAPI spec updated when API behaviour/endpoints/DTOs change
• [ ] SDK regenerated when required
• [ ] Backward compatible OR breaking change explicitly approved
• [ ] Contract/API tests cover success + error paths
• [ ] HTTP status semantics match existing patterns

Data & Persistence:

• [ ] Repository interfaces extend CrudRepository<T, String> (unless established pattern differs)
• [ ] Repository methods are domain-scoped (avoid wide fetch + in-code filtering)
• [ ] If a Repository is changed, check for the corresponding implementation in BOTH the MongoDB and JDBC modules (`gravitee-am-repository-mongodb`/`-jdbc`, `gravitee-am-dataplane-mongodb`/`-jdbc`)
• [ ] Both MongoDB and JDBC implementations provided when applicable
• [ ] Liquibase changesets grouped logically (table + indexes + constraints together)
• [ ] Mongo indexes follow established initials + sort-order naming convention

4. Testing & Validation (The "Proof")

Test Coverage:

• [ ] Success and error paths covered
• [ ] Bottom-up coverage: Repository → Service → Resource
• [ ] CREATE/UPDATE/DELETE tests must verify audit logging
• [ ] Tests actually verify audit events are emitted (not just "audit was called")

Implementation Parity:

• [ ] Both MongoDB and JDBC tested via shared test suite (when applicable)
• [ ] Integration tests updated where needed

Validation Commands:

• [ ] Validation commands referenced from commands.md or proposed
• [ ] Tests pass, lint is clean

Documentation & Traceability:

• [ ] Complex logic explained (JavaDoc or inline comments)
• [ ] Files cited that informed implementation (pattern-following)
• [ ] Breaking changes clearly called out for PR description

Self-review prompts (quick)

1) Pattern adherence: what existing pattern was followed, where is it? 2) Search-first: what similar implementations were inspected? 3) Risk: what could break, and which edge cases exist? 4) Reviewability: is the diff focused? 5) Completeness: does it satisfy acceptance criteria? 6) Testing: do tests verify behaviour (not just exercise code)?

Common issues to catch

Architectural:

• Audit logging implemented in Resource instead of Service layer
• Business logic in Resource layer
• Blocking calls in reactive paths (blockingGet())

Data:

• Wide queries + in-code filtering instead of scoped queries
• Missing Mongo/JDBC parity where required
• Repository changes without checking both the MongoDB and JDBC modules

API:

• Missing SDK regeneration after API changes
• Breaking changes without migration documentation

Testing:

• Tests that don't verify audit events were emitted
• Only testing one storage implementation (Mongo or JDBC, not both)

Code Quality (Agent-isms):

• Redundant null checks when using Optional or @NotNull
• Log pollution: logging entire DTOs/entities instead of IDs
• Unrelated refactors mixed into feature changes
• Commented-out code or debug statements left behind

References (optional)

• @docs/agent-standards/commands.md
• @docs/agent-standards/templates/openapi_change_checklist.template.md
• @docs/agent-standards/templates/migration_checklist.template.md

Exit criteria

• Verdict issued with evidence (line numbers and snippets)
• Findings prioritized by severity (Blockers > Warnings > Nits)
• Actionable task list provided
• Suggested validation commands included
• No code changes made unless explicitly requested