npx skills add AGCO-Global/org-skills --skill aem-code-review -a github-copilot
# or with the org installer (adds .github/skills/aem-code-review):
npx -y github:AGCO-Global/org-skills add skill aem-code-review

npx skills add AGCO-Global/org-skills --skill aem-code-review
# user-level instead of project-level:
npx skills add AGCO-Global/org-skills --skill aem-code-review -g

/plugin marketplace add AGCO-Global/org-skills
/plugin install aem-quality-skills@org-skills
# or just this skill, in this repository:
npx skills add AGCO-Global/org-skills --skill aem-code-review -a claude-code

npx skills add AGCO-Global/org-skills --skill aem-code-review -a codex

npx skills add AGCO-Global/org-skills --skill aem-code-review -a cursor

npx skills add AGCO-Global/org-skills --skill aem-code-review -a gemini-cli

try (ResourceResolver rr = factory.getServiceResourceResolver(
        Map.of(ResourceResolverFactory.SUBSERVICE, "content-reader"))) {
    ...
}

## Findings
| Severity | File:line | Issue | Why it matters | Fix |
|---|---|---|---|---|
| [blocker] | core/.../TeaserModel.java:42 | Service resolver never closed | Leaks sessions until pod degrades | try-with-resources around getServiceResourceResolver |
| [major] | ui.apps/.../teaser.html:18 | `context='html'` on dialog text | Stored XSS if field isn't RTE-filtered | Remove context or sanitise via XSSAPI.filterHTML |

## Summary
Verdict: Approve | Approve with minor changes | Request changes
Blockers: <n>  Majors: <n>  Minors: <n>
Top 3 to address: 1. … 2. … 3. …
What's good: <genuine praise>

---
name: aem-code-review
description: Review AEM code changes (pull requests, diffs, packages) with a severity-ranked checklist covering security, resource handling, performance, Sling Models, OSGi services, components and dialogs, Cloud Service compatibility, content package filters, dispatcher/CDN rules, tests and accessibility. Use this whenever the user asks to review an AEM PR or diff; pastes HTL, a Sling Model, servlet, OSGi service, `.cfg.json`, `filter.xml`, `_cq_dialog`, clientlib or dispatcher `.any`/rewrite file and asks "is this OK"; mentions `context='unsafe'`, `ResourceResolverFactory`, `loginAdministrative`, `QueryBuilder`, `@Model`, `@Reference`, `sling.servlet.paths`, SonarQube or Cloud Manager code quality failures; or wants a pre-merge self-review. Also use it when auditing an inherited AEM codebase, even if they only ask about one file.
metadata:
  technology: AEM
  type: code-review
---

# AEM Code Review

AEM bugs hide in the gaps between layers: an HTL context, a leaked resolver, a filter root, a dispatcher rule. Review each change against the runtime it will hit — cached publish, multiple pods, immutable `/apps` — cite the line, explain the impact, and give the fix.

## 1. Before reading the diff

- Get the intent (ticket or one sentence). Identify what runs where: author-only, publish, both; request-time vs background.
- List touched layers: `core` Java, HTL/components, `ui.config`, `ui.content`/filters, `dispatcher`, `ui.frontend`, tests. Review each layer with its section below.
- Diff > 400 non-generated lines → ask for a split, or review only the riskiest layer in depth.

## 2. Severity scale

| Label | Meaning | Blocks merge? |
|---|---|---|
| **[blocker]** | Security hole, data/content loss, resource leak, Cloud Service incompatibility, prod outage risk | Yes |
| **[major]** | Performance regression, cache-busting, likely bug, missing test for new logic, a11y failure on a primary flow | Yes, unless deferred with a ticket |
| **[minor]** | Maintainability, non-idiomatic AEM usage, small duplication | No |
| **[nit]** | Style not caught by tooling | No |
| **[question]** | Need intent before judging | — |
| **[praise]** | Worth copying | — |

## 3. Checklist — run all, report only findings

### Security
- HTL: `context='unsafe'` or `context='html'` on author/user input → blocker unless the value is sanitised. URLs in `href`/`src` must keep the `uri` context; inline JS/CSS values need `scriptString`/`styleString`.
- Java writing markup (`response.getWriter().write(...)`) without `XSSAPI`/encoding; JSON built by string concatenation instead of a serializer.
- Servlets bound by `sling.servlet.paths` → prefer `resourceTypes` + selectors/extensions so resource ACLs and dispatcher rules apply. Check method restrictions and CSRF for POST.
- `loginAdministrative`/`getAdministrativeResourceResolver` → blocker. Service users must have least-privilege ACLs from repoinit, one per concern.
- Secrets, tokens, internal hosts in `.cfg.json`, HTL or JS → move to `$[secret:...]`/`$[env:...]`.
- Dispatcher `/filter` allowing broad paths (`/content/*`, `*.json`, `*.infinity.json`, `/bin/*`, query strings) or `/allowedClients` changes; CDN rules that disable auth.
- Logging PII, tokens, or full request bodies.

### Resource handling
- `ResourceResolver` from `ResourceResolverFactory` not closed → blocker. Use try-with-resources:

```java
try (ResourceResolver rr = factory.getServiceResourceResolver(
        Map.of(ResourceResolverFactory.SUBSERVICE, "content-reader"))) {
    ...
}
```

- Never close the request's resolver; never store a resolver or `Session` in a field of a singleton service.
- Streams/`InputStream` from `Rendition`/`Binary` closed; JCR `Session.logout()` for sessions you opened.

### Performance
- JCR-SQL2/QueryBuilder queries without a matching index (traversal warnings), without `p.limit`, or with `p.guessTotal` missing on large sets. Ask for the query plan.
- Repository traversal (`listChildren` recursion, `Resource.getChildren` over large trees) on request path.
- Queries or JCR lookups inside loops during render, especially in Sling Models used in lists.
- Responses made uncacheable: `Set-Cookie`, per-user data in cached pages, random selectors/query strings, missing `Cache-Control` on JSON endpoints; personalisation that should be client-side or edge-side.
- Heavy clientlibs: all-in-one categories on every page, un-minified vendor bundles, `embed` duplication, sync scripts in `<head>`.

### Sling Models
- `adaptables` correct (`SlingHttpServletRequest` when request/`currentPage` is needed, `Resource` otherwise); `adapters` set to an interface for exporter/proxy use.
- `defaultInjectionStrategy = OPTIONAL` with explicit null handling, or `@ValueMapValue(injectionStrategy = REQUIRED)` where absence is a bug. Prefer specific injectors (`@ValueMapValue`, `@ChildResource`, `@OSGiService`, `@ScriptVariable`) over `@Inject`.
- Heavy work in `@PostConstruct` only; getters are cheap and side-effect free.
- JSON exporter (`@Exporter(name = "jackson", extensions = "json")`) doesn't leak internal fields; `@JsonIgnore` where needed; `resourceType` set for component models.

### OSGi
- Configs via `@Designate` + `@ObjectClassDefinition`; no `PropertiesUtil`/Felix SCR annotations.
- `@Reference` cardinality/policy deliberate; dynamic references are `volatile`.
- Singleton services with mutable instance state → thread-safety bug. Use immutable config captured in `@Activate`/`@Modified`, or concurrent structures.
- Schedulers: `scheduler.concurrent=false`, `scheduler.runOn` appropriate; jobs idempotent.

### Components and dialogs
- Core Components extended via **proxy** (`sling:resourceSuperType`) — not copied into `/apps`.
- Content policies and the Style System instead of design dialogs or per-component CSS class fields.
- Dialog fields validated, `required` where needed, `name="./prop"` consistent with the model; no business logic in HTL beyond presentation.
- Editable templates: allowed components set via policy; `cq:isContainer`/responsive grid used correctly.

### Cloud Service compatibility
- Runtime writes to `/apps` or `/libs`, local filesystem writes, long-running threads, `System.exit`, custom replication agents, classic UI, `/etc` config lookups → blocker.
- Deprecated/removed APIs flagged by `aem-sdk-api` or code quality rules.
- Custom runmodes; OSGi configs outside `config.<supported runmode>`.

### Content packages and filters
- Package types correct (`application` for `/apps`, `content` for mutable paths); no mixed packages.
- `filter.xml` roots narrow; `mode="merge"`/`update` for author-owned content. A `replace` filter on `/content/<site>` or `/conf/<site>` wipes author edits on every deploy → blocker.
- Service users/ACLs via repoinit, not `rep:policy` nodes in packages. Index changes carry a bumped `-custom-<n>` version.

### Dispatcher / CDN
- Validated with the dispatcher SDK; filters deny-by-default with narrow allows; cache rules and `/invalidate` scoped; rewrites don't create redirect loops; TTL headers explicit.

### Tests and quality gates
- New logic has unit tests (AEM Mocks / `AemContext`) asserting outputs, not just coverage. Bug fixes include a regression test.
- Changes won't trip Cloud Manager rules (coverage threshold, SonarQube security/reliability, OakPAL). Suppressions need a justification comment.

### Accessibility and i18n
- Semantic markup in HTL, `alt` from asset metadata or dialog, headings level configurable, keyboard-operable interactions, visible focus.
- User-facing strings via `i18n` (`${'Read more' @ i18n}`) or dictionaries; no hardcoded language in markup; `lang` attributes correct.

## 4. Deliverable format

```
## Findings
| Severity | File:line | Issue | Why it matters | Fix |
|---|---|---|---|---|
| [blocker] | core/.../TeaserModel.java:42 | Service resolver never closed | Leaks sessions until pod degrades | try-with-resources around getServiceResourceResolver |
| [major] | ui.apps/.../teaser.html:18 | `context='html'` on dialog text | Stored XSS if field isn't RTE-filtered | Remove context or sanitise via XSSAPI.filterHTML |

## Summary
Verdict: Approve | Approve with minor changes | Request changes
Blockers: <n>  Majors: <n>  Minors: <n>
Top 3 to address: 1. … 2. … 3. …
What's good: <genuine praise>
```

Verdict rules: any blocker → Request changes; an untracked major → Request changes; otherwise Approve (with minor changes if minors exist).

## Anti-patterns to reject (in reviews)

- Approving because Cloud Manager passed — the gates miss filter modes, HTL contexts and cache impact.
- Nitpicking Java style while missing an unclosed resolver or a `replace` filter on `/content`.
- "Use Core Components" with no concrete proxy path or reason.
- Ignoring dispatcher and `ui.config` diffs because they "aren't code".
- Requesting a rewrite to personal patterns when the repo convention is sound.
- Blocking on theoretical issues without citing a line and a realistic impact.