---
name: review-ansible-role
description: "QA an Ansible role (generated or existing) against the senior conventions: structure, variable completeness, naming, idempotency, secrets, compose/env or systemd correctness, healthchecks, air-gap and multi-instance readiness; lints with yamllint/ansible-lint and fixes CRITICAL/HIGH findings in place. Use when a role should be shipped or merged."
license: MIT
handoffs:
  - label: "Fix blocking issues"
    agent: "write-ansible-role"
    prompt: "The review of the role at {role_path} reported blocking findings. Run the write-ansible-role skill to apply the fixes, then re-run this review."
---

# What I do

I review an Ansible role and produce an actionable report: issues found (by
severity), required fixes, and suggested improvements. I check both mechanical
correctness (structure, lint, syntax) and the senior conventions from
`write-ansible-role/CONVENTIONS.md`.

I review both role shapes:

- `app-deploy` — Docker Compose application roles (any runtime: .NET, Go,
  Python, Node).
- `infra-service` — standalone component roles installed as binaries/packages
  with a systemd unit (kafka, clickhouse, k3s, node_exporter, minio, ...).

# When to use me

- After generating a role with `write-ansible-role` (pre-ship QA).
- Before merging changes to an existing role.
- When a role behaves oddly and you want a systematic audit.

# User Input

> You **MUST** consider the user input before proceeding (if not empty).

`$ARGUMENTS` may contain any of:

- Path to the role directory (or a file list to review).
- Optional: context from the Role Design Spec to validate defaults against it.

# Operating Constraints

- Review against `CONVENTIONS.md` as the non-negotiable constitution — a
  violation is at least **HIGH**, a breach of a security/idempotency invariant
  is **CRITICAL**.
- Never invent severity: every finding maps to a concrete file:line.
- Fix **CRITICAL** and **HIGH** findings directly in the role (the user asked
  for a review that improves the role). Leave MEDIUM/LOW as suggestions unless
  trivial to apply.
- Never modify files outside the reviewed role.

# Execution Steps

## 1. Load conventions

Read `write-ansible-role/CONVENTIONS.md` and the `CHECKLIST.md` in this folder.
Review against both.

## 2. Constitution Check

Before the detailed pass, verify the role does not violate core conventions:

- Variable prefix and naming follow Conventions §2.
- defaults/main.yml section order and required-var fail-fast follow §3.
- Static-IP `replace` scheme and zone conventions follow §4 (app-deploy).
- tasks/main.yml orchestrator order follows §5 (app-deploy) or §10
  (infra-service).
- Compose uses `x-logging` anchor, static IPs, healthchecks, one-shot jobs §7.
- infra-service: systemd unit present, handlers used, service verified
  `active` §10.

Any violation here is **CRITICAL**.

## 3. Structure check

- Standard role tree exists: `defaults/`, `tasks/`, `templates/`,
  `meta/main.yml`, `README.md`. `files/`, `handlers/`, `vars/` as needed.
- A Molecule **default** scenario exists (`molecule/default/`) per §11; its
  testinfra assertions cover the deployed state.
- `meta/main.yml` has valid `galaxy_info`.

## 4. Variable completeness

- Every `{{ var }}` referenced in tasks/templates is defined in
  `defaults/main.yml` (or `vars/` or a registered fact) — grep to verify.
- No undefined required vars; required ones abort early with a clear message.
- Naming consistent with the `<prefix>_` convention; internal vars use `_`.
- Booleans gate features and default to `false`.

## 5. Correctness & best practices

### app-deploy

- **Idempotency**: tasks are idempotent (no unconditional commands that always
  change; compose up uses `--force-recreate` deliberately).
- **Secrets**: no hardcoded secrets in tasks/templates; defaults use
  placeholders; README/playbook recommend vault.
- **Compose/env**: static-IP `replace` trick consistent; `x-logging` anchor
  used; healthcheck mounts + env (`HEALTH_APP_PORT/CHECK_PATH`) present;
  `depends_on` for one-shot jobs with `service_completed_successfully`; env
  keys match the app's real config discovery (appsettings sections,
  os.Getenv, python-dotenv, env.sample.js).
- **Registry**: login/logout balanced; skipped in air-gap.
- **Air-gap**: all required images covered by the air-gap image list.
- **Multi-instance**: suffix applied to names that must not collide.
- **Healthchecks**: `until` + `retries`/`delay` present on assertions.

### infra-service

- **Install**: download guarded by a stat check (idempotent) OR OS package via
  repo key + `apt_repository`/`yum_repos`; versions pinned, never `latest`.
- **systemd**: unit installed, `daemon-reload`, `enable`, restart via handler
  (not inline); `EnvironmentFile` wired; service verified `active`
  (`service_facts` + `until`/`retries`).
- **Config**: rendered from `.j2` templates, perms/owner set, data/log dirs
  created.
- **Cluster**: multi-node tasks gated by inventory membership; bootstrap
  ordering correct.

## 6. Molecule tests

- `molecule/default/` present with a Docker driver + systemd image,
  testinfra verifier, and a converge that includes the role (§11).
- Testinfra assertions actually check the deployed state, and the host group
  name matches `molecule.yml`.
- `group_vars/all.yml` uses placeholders only (no real secrets).
- If a Docker environment is available, run `molecule test -s default` and
  report failures; otherwise flag as deferred-to-CI.

## 7. Lint & syntax

If available, run:

```bash
yamllint -c <this folder>/.yamllint <role>/
ansible-lint <role>/
ansible-playbook --syntax-check <playbook>
```

Report every finding with file:line. Triage by severity:

| Severity | Meaning | Action |
| -------- | ------- | ------ |
| **CRITICAL** | Convention/security/idempotency violation; breaks deployment | Must fix |
| **HIGH** | Wrong or unsafe default; missing required wiring | Must fix |
| **MEDIUM** | Best-practice gap; should fix |
| **LOW** | Style / optional improvement |

## 8. Report

Produce a review report:

- Summary line (pass / pass-with-warnings / fail).
- Findings table: severity | location | issue | suggested fix.
- Fix the CRITICAL and HIGH items directly in the role. Leave MEDIUM/LOW as
  suggestions unless trivial.

# Validation

- Every reported finding has a file:line reference.
- No CRITICAL or HIGH finding remains open after the fix pass.
- **Re-verify fixes against the writer's self-checks before finishing**: (a) no
  leftover `__...__` token remains anywhere in the role; (b) every `{{ var }}`
  you introduced is defined in `defaults/main.yml` / `vars/` / a registered
  fact; (c) every file you edited still parses as YAML and its templates still
  render; (d) app-deploy: the air-gap image list still covers every image in the
  rendered compose files and the compose/healthcheck/verify wiring is unchanged.
- **Reopen previously reported MEDIUM/LOW findings on each pass** so they are
  not lost between review iterations.
- Re-run `yamllint`/`ansible-lint` after fixes to confirm they pass.
- Role is idempotent, secret-free (placeholders only), air-gap and
  multi-instance aware.

# Output

A review report (summary + findings by severity + fixes applied). The role must
pass `ansible-lint` and `yamllint` with only accepted warnings before shipping.

# Quality bar

- No CRITICAL or HIGH findings remain.
- Role is idempotent, secret-free (placeholders only), air-gap and
  multi-instance aware (app-deploy).
- infra-service: systemd unit correct, service verified `active`.
- Every variable used is defined; every template renders.