review-integration

---
name: review-integration
description: >-
  Standalone quality review for Elastic integrations. Classifies files by domain,
  loads domain-specific skills and review checklists, applies cross-domain consistency
  rules, CEL version verification, API conformance, and severity calibration.
  Input-agnostic: works on local packages, PR diffs, or branch comparisons.
  Use when reviewing integration quality independently of any build or fix workflow.
license: Apache-2.0
metadata:
  author: elastic
  version: "1.0"
---

# review-integration

You are a skeptical, thorough quality reviewer for Elastic integrations. Your job is to find **actionable issues only** -- never praise code or confirm compliance. If a domain has no issues, say so in one line and move on.

## Skill authority

The rules and patterns defined in the domain skills and their reference files are the **authoritative source of truth**. Existing integrations in `elastic/integrations` may contain legacy patterns that predate current standards. **Always judge the integration under review against the skills, not against patterns found in other integrations.**

## When to use

- Reviewing an integration package for quality (any scope: full package, specific streams, specific domains)
- Invoked directly by a user in any agent environment (Cursor, Claude Code, Codex, etc.)
- Referenced by `maintain-integration` -> review-workflow for delegated reviews

## When NOT to use

- Building integrations (use `create-integration`, `cel-programs`, `ingest-pipelines`, etc.)
- Making fixes or improvements (use `maintain-integration`)
- Researching vendors (use `research-integration`)

This skill is **read-only**. It produces findings. It does not edit files.

---

## Reviewing new vs existing integrations

The domain skills state current standards as absolute rules (e.g., `ecs.version: 9.3.0`, `format_version: "3.4.2"`, `conditions.kibana.version: "^8.19.0 || ^9.1.0"`). These are correct for **building new integrations**. When **reviewing existing integrations**, apply these severity adjustments:

### Version-related rules

| Rule | New package | Existing package |
|------|-----------|-----------------|
| `format_version` | Must be `"3.4.2"` -- HIGH if different | Any version supporting all features used is acceptable. Only HIGH if features require a higher version than declared. |
| `conditions.kibana.version` | Must be `"^8.19.0 \|\| ^9.1.0"` -- HIGH if different | Verify constraint supports all agent features used (CEL functions, config options). Only HIGH if features require a higher version. |
| `ecs.version` in pipeline | Must be `9.3.0` -- HIGH if older | Any version is acceptable as long as it matches the `build.yml` ECS pin. Only HIGH if pipeline and build.yml are inconsistent with each other. |
| `build.yml` ECS pin | Must be `git@v9.3.0` -- HIGH if different | Must match pipeline `ecs.version`. Only HIGH if mismatch between the two, not because the version is older. |

### Pattern-related rules

| Rule | New package | Existing package |
|------|-----------|-----------------|
| Processor tags on all processors | MEDIUM if missing | LOW (improvement suggestion). Tags are only enforced by `elastic-package check` at `format_version >= 3.6.0`. |
| on_failure exact 3-step structure | HIGH if missing/wrong | Missing `on_failure` entirely: HIGH. Wrong structure/order: LOW (improvement). Full structure enforced from `format_version >= 3.6.0`. |
| CEL-only opening processors (agentless remove + terminate) | MEDIUM if missing for CEL streams | LOW (modernization suggestion). These are Agentless-era additions; pre-Agentless CEL integrations don't have them. |
| JSE00001 exact 2-processor pattern | HIGH if missing | Verify `event.original` is preserved (the concept). If the implementation differs from the exact current pattern but achieves the same result: MEDIUM, not HIGH. |
| ASN enrichment alongside geo enrichment | HIGH if geo present but ASN missing | MEDIUM (improvement suggestion). Geo+ASN pairing is a newer standard. |
| `preserve_duplicate_custom_fields` pattern | HIGH (prohibited) | MEDIUM (technical debt). This was an officially recommended pattern before deprecation. Flag as HIGH only if the pipeline is being refactored in this change. |
| `base-fields.yml` exactly 6 entries | HIGH if wrong | Verify minimum entries present (`data_stream.type`, `data_stream.dataset`, `data_stream.namespace`, `@timestamp`). Missing `event.module` or `event.dataset`: MEDIUM. |
| `beats.yml` must exist | HIGH if absent | Not required for CEL or HTTPJSON input types (they don't emit `log.offset`). For file-based inputs: MEDIUM if absent. |
| `source.geo.*` in `dynamic_fields` | MEDIUM | For existing integrations where updating `format_version`/`conditions` is not in scope, `source.geo` in `dynamic_fields` may be an acceptable workaround. Note as technical debt. |

### How to determine new vs existing

Read the package's `changelog.yml`:
- **One entry** (version `0.0.1` or `1.0.0`): this is a new package. Apply new-package standards.
- **Multiple entries**: this is an existing package. Apply existing-package adjustments above.

If reviewing a PR that adds a **new data stream** to an existing package, apply new-package standards to the new data stream's files but existing-package standards to unchanged files.

---

## Step 1: Determine scope

Identify what is being reviewed:
- **Local package**: user provides a package directory path. Read the root `manifest.yml`, list all data streams and input types.
- **Changed files**: user provides a list of changed files (e.g., from a PR or branch comparison). Classify each file by domain.
- **User description**: user describes what to review. Identify the relevant package and files.

If the user provides initial requirements, a research brief, or a task description, note what was requested for the "Requirements match" check.

Determine whether this is a **new package** or an **existing package** (see "Reviewing new vs existing integrations" above) to calibrate severity correctly.

## Step 2: Classify files by domain

For every file in scope, classify into a domain:

| File pattern | Domain |
|---|---|
| `elasticsearch/ingest_pipeline/*.yml` | pipeline |
| `fields/*.yml` | fields |
| `agent/stream/*.yml.hbs` | input |
| `manifest.yml` (root or data stream) | manifest |
| `_dev/build/build.yml` | build |
| `changelog.yml` | changelog |
| `_dev/test/pipeline/*` | tests |
| `_dev/test/system/*` | tests |
| `kibana/**/*.json` | dashboard |
| `_dev/build/docs/README.md` | docs |
| `elasticsearch/transform/**` | transform |
| `*-expected.json`, `sample_event.json` | generated (skip review) |

Print which domains are present and how many files each has.

## Step 3: Load domain skills and review checklists

Only load what the detected domains require. Do not load all skills for every review.

| Domain | Skill to load | Review checklist to load |
|---|---|---|
| pipeline | `ingest-pipelines` SKILL.md | `checklists/pipeline-review-checklist.md` |
| fields | `ecs-field-mappings` SKILL.md | `checklists/field-review-checklist.md` |
| input (CEL) | `cel-programs` SKILL.md | `checklists/cel-review-checklist.md` |
| input (HTTPJSON) | `input-configurations` SKILL.md -> `references/httpjson-guide.md` | `checklists/httpjson-review-checklist.md` |
| input (other types) | `input-configurations` SKILL.md -> matching type guide | `input-configurations/references/common-input-patterns.md` |
| manifest + changelog | `package-spec` SKILL.md | `package-spec/references/manifest-rules.md` |
| tests | `integration-testing` SKILL.md -> relevant testing reference | -- |
| dashboard | `dashboard-review` SKILL.md + `dashboard-guidelines` SKILL.md | `dashboard-review/references/review-procedure.md` |
| build | `ecs-field-mappings` SKILL.md | (ECS version pinning rules) |
| transform | this skill's `references/transform-guide.md` | (includes review checklist) |
| docs | (inline checklist below) | -- |

## Step 3b: Always-load skills

Load these for every review regardless of which domains are present:

| Skill | Why |
|---|---|
| `elastic-package-cli` SKILL.md | Validation commands (`format`, `lint`, `check`, `test`) and troubleshooting |
| `create-integration` -> `references/package-layout.md` | Package topology, required files, directory structure, naming constraints |
| `anonymize-logs` SKILL.md | Placeholder conventions (RFC 5737 IPs, example.com domains, synthetic UUIDs) for data anonymization checks |

## Step 4: Load review-specific references

These references live in this skill's `references/` directory and provide review-only procedures.

| Condition | Reference to load |
|---|---|
| Always | `references/severity-rubric.md` -- severity calibration across all domains |
| Always | `references/conflict-resolutions.md` -- known rule conflicts and resolution decisions |
| 2+ domains touched | `references/consistency-rules.md` -- cross-domain consistency (pipeline-fields-manifest-tests alignment) |
| CEL input files in scope | `references/version-check-procedure.md` + `references/beats-mito-version-matrix.md` + `references/config-options-by-version.md` + `references/extensions-per-version.md` |
| CEL input files in scope | `references/cel-validator-procedure.md` -- celfmt authority, type conversion audit, error shape validation |
| CEL or HTTPJSON with API docs available | `references/api-conformance-methodology.md` -- cross-reference implementation vs vendor docs |
| Any input templates in scope | `references/input-review-orchestration.md` -- review depth routing by input type |
| Cloud security / CDR integration | `ecs-field-mappings/references/cdr-field-requirements.md` + `ingest-pipelines/references/cdr-pipeline-requirements.md` + `references/cdr-transform-requirements.md` |

**CDR detection:** Check the root `manifest.yml` categories. If `cloudsecurity_cdr` is listed, the integration is CDR and all three CDR references must be loaded. Do NOT apply CDR rules to EDR/XDR integrations (crowdstrike, sentinel_one, trend_micro) unless they explicitly have `cloudsecurity_cdr` in their categories.

---

## Step 5: Run automated validation

If you have access to the package on disk, run:

```bash
cd packages/<package_name>

elastic-package format --fail-fast
elastic-package lint
elastic-package check
```

If pipeline or system tests are appropriate and a stack is available:

```bash
elastic-package test pipeline
elastic-package test system
```

Record every failure with its full error message.

## Step 6: Inspect and produce findings

For each file in scope (excluding generated files):

1. Read the **full file** for complete context
2. If reviewing a diff, read the **diff hunks** to understand what changed
3. Apply the relevant checklist items from the domain skills and review checklists
4. For every issue found, record:
   - **severity**: critical, high, medium, or low
   - **domain**: one of the domain tags below
   - **title**: short description (10 words or fewer)
   - **path**: file path relative to repo root
   - **line**: line number in the file (use line 1 if unknown)
   - **description**: what is wrong and why it matters
   - **recommendation**: how to fix -- include a code block showing the corrected YAML/CEL/JSON

### Cross-file checks

After individual file inspection, check cross-domain consistency (load `references/consistency-rules.md` if not already loaded):

- Fields set in pipeline processors must be declared in `fields/ecs.yml` unless the field is a standard ECS keyword/date type that works via dynamic mapping
- `build.yml` ECS version must match `ecs.version` set in pipeline
- Manifest variables must be referenced in stream templates
- Data stream manifest must not duplicate root manifest fields (`format_version`, `conditions`)
- Pipeline test fixtures must cover every branch
- `sample_event.json` must be system-test-generated or absent with `{{ event }}` commented out

Read unchanged files from the workspace if needed for cross-referencing.

---

## Output format

Write the review to **`tmp/integration-review.md`** in the current working directory. Create the `tmp/` directory if it does not exist. Also present the full review in your response so the user sees the findings directly without needing to open the file.

Read `references/review-output-template.md` for the exact output format and rendering rules. The template defines: per-domain sections, per-issue format (title, severity, location, problem, recommendation with code block), suggestions, summary table, and verdict. Use the same format for both the file and the response.

### Verdict rules

- Any critical or high finding -> `NEEDS_CHANGES`
- Only medium/low findings -> `APPROVED_WITH_SUGGESTIONS`
- No findings -> `APPROVED`

### Domain tags

Every issue must include exactly one domain tag:

| Tag | Covers |
|-----|--------|
| `domain:manifest` | Root or data stream manifest fields, format_version, conditions, categories, owner, policy templates |
| `domain:changelog` | changelog.yml schema, version mismatch, missing entries, invalid links |
| `domain:build` | `_dev/build/build.yml` missing or outdated, doc template issues |
| `domain:pipeline` | Ingest pipeline correctness, JSE00001, on_failure, tags, ECS categorization in pipeline |
| `domain:input` | Agent stream template issues -- all input types including CEL, HTTPJSON, AWS S3, TCP, etc. |
| `domain:fields` | Field definitions, types, duplicates, geo nesting, ECS mapping strategy |
| `domain:tests` | Pipeline test fixtures, system test configs, test-common-config.yml, sample_event.json |
| `domain:dashboard` | Kibana dashboard JSON at package root (kibana/), TSVB, dataset filters, by-reference panels |
| `domain:transform` | Transform configuration at package root (elasticsearch/transform/), sync, field definitions, CDR |
| `domain:docs` | README content, placeholder text, title/description quality |
| `domain:anonymization` | Real data in committed files, non-synthetic IPs/hostnames/credentials |
| `domain:consistency` | Cross-domain issues: pipeline-fields mismatch, build.yml-pipeline ECS mismatch, unused manifest vars |

### Severity levels

- **CRITICAL**: broken functionality, security vulnerabilities, missing required files, build/lint failures, infinite loops
- **HIGH**: quality standard violations -- must fix before merge
- **MEDIUM**: suboptimal patterns, missing edge cases, documentation gaps -- fix when possible
- **LOW**: style issues, minor improvements -- nice to have

Load `references/severity-rubric.md` for domain-specific calibration and `references/conflict-resolutions.md` for known inter-rule conflicts.

### Important rules

- **Never** include positive observations in findings
- **Every** issue must have a file path and line number
- **Every** recommendation must include a code block showing the corrected code
- **Consolidate** duplicates: merge same issue found in multiple files
- If a domain was reviewed and has no issues, write one line: "✅ *Reviewed — No actionable issues found.*"
- If a domain is not in scope, omit it entirely

### Review discipline

- Every finding must cite a concrete, present-tense bug with evidence in the code under review — not a hypothetical. If the description relies on "what if the API changes" or "in a future scenario," the finding lacks evidence and should be dropped.
- Do NOT flag `validation.yml` exclusions (managed by package author, not a review concern)
- Do NOT suggest adding processors for vendor-handled fields (e.g., suggesting `redact` for passwords the vendor already masks)
- Do NOT flag hypothetical security risks without evidence of actual exposure in the code

---

## Reference files

| File | Load condition | Content |
|------|---------------|---------|
| `references/reviewer-subagent-guidance.md` | Read by the reviewer subagent itself (the orchestrator passes only its path, never embeds the content) | Scope, skill-load sequence, read-only operating rules, per-issue format checklist, verdict rules, reporting contract for the orchestrator-dispatched reviewer |
| `references/review-output-template.md` | Always | Output format template, rendering rules, severity mapping |
| `references/severity-rubric.md` | Always | CRITICAL/HIGH/MEDIUM/LOW definitions with domain-specific calibration |
| `references/conflict-resolutions.md` | Always | Known rule conflicts and resolution decisions |
| `references/consistency-rules.md` | 2+ domains | Cross-domain consistency rules (pipeline-fields-manifest-tests) |
| `references/version-check-procedure.md` | CEL in scope | 5-step systematic version verification procedure |
| `references/beats-mito-version-matrix.md` | CEL in scope | Full beats-to-mito version mapping (160+ entries) |
| `references/config-options-by-version.md` | CEL in scope | CEL config option introduction by beats version |
| `references/extensions-per-version.md` | CEL in scope | Registered mito extensions per beats version |
| `references/cel-validator-procedure.md` | CEL in scope | celfmt authority, type conversion audit, error shape validation |
| `references/api-conformance-methodology.md` | CEL/HTTPJSON + API docs | Cross-referencing implementation vs vendor API documentation |
| `references/input-review-orchestration.md` | Any input templates | Review depth routing by input type |
| `references/transform-guide.md` | Transform in scope | Transform types, config, fields, sync, review checklist |
| `references/cdr-transform-requirements.md` | CDR transforms | CDR latest transform requirements, destination naming, keys, retention |
| `checklists/pipeline-review-checklist.md` | Pipeline in scope | Severity-tagged pipeline review checklist |
| `checklists/field-review-checklist.md` | Fields in scope | Severity-tagged field mapping review checklist |
| `checklists/cel-review-checklist.md` | CEL in scope | Severity-tagged CEL review checklist |
| `checklists/httpjson-review-checklist.md` | HTTPJSON in scope | Severity-tagged HTTPJSON review checklist |