Review Pr

package-spec PR Review (Agent Skill)

This skill conforms to the [Agent Skills](https://agentskills.io/specification.md) open format (agentskills.io).

This skill does not modify the codebase. It does not edit source files, run any git command that modifies state (no commit, push, checkout, reset, stash, merge, or rebase), or change anything on the branch under review. If an issue is found, describe it and stop — do not attempt to fix it. The only file it writes is the review report at .cursor/review-pr-output.md.

Reviews a branch or pull request against the contribution rules for this repository. See references/checklist.md for full rules and examples.

For narrative onboarding (Makefile targets, scaffolding test packages, testing against the integrations repo), see [CONTRIBUTING.md](../../../CONTRIBUTING.md) at the repository root. This skill and the checklist focus on PR completeness against those practices.

How to Use This Skill

---

Step 1 — Resolve the Target

Determine what to diff based on the input provided:

If a PR number was given (e.g., 1140):

gh pr view 1140 --repo elastic/package-spec --json headRefName,baseRefName,title,url
git fetch origin <headRefName>
# diff: FETCH_HEAD vs <baseRefName>
git diff <baseRefName>...FETCH_HEAD --stat
git diff <baseRefName>...FETCH_HEAD

If a branch name was given (e.g., my-feature):

git diff main...<branch> --stat
git diff main...<branch>

If no argument was given (review current branch):

git branch --show-current          # confirm we are not on main
git diff main...HEAD --stat
git diff main...HEAD

If the current branch is main and no argument was given, report that there is nothing to review.

---

Step 2 — Categorize Changed Files

Using the file list from --stat, bucket each changed file into one or more categories:

Use the categories internally to pick checklist sections; do not paste the full categorized file list into the report unless it clarifies an extra-review item.

---

Step 3 — Run the Checklist

Work through each applicable section from references/checklist.md.

Below is a compact version of the checks. The reference file has full rules, edge cases, and exact examples.

Contribution process (when relevant)

Consult section 0 in the reference file and CONTRIBUTING.md.

• Cross-stack or cross-product spec changes should follow the Change Proposals process before merge.
• New package categories require the Category Proposals process.
• If the change is large and the PR does not reference a proposal or tracking issue, add an extra-review note asking the author to confirm process was followed.

Always: Changelog

• Is spec/changelog.yml modified?
• If not: flag as missing (required for all non-trivial changes; dependency-bump PRs

from Dependabot are the only exception).

• If modified, verify:
• Entry is under the correct in-development version (the one with -next suffix).
• New entry is at the bottom of that version's changes list.
• description is a complete sentence ending with a period.
• type is one of: enhancement, bugfix, or breaking-change.
• link points to a valid PR URL or is TBD (not empty, not an issue URL unless the

change originated from an issue with no PR).

• If the feature is blocked on Kibana/elastic-package: a # Pending on <url> comment

appears directly above the entry.

If spec files changed

Consult the "Version patches" and "Schema reuse" sections of the reference file.

• For new properties or definitions that older format_version values must not include, add

versions[].patch remove operations (see the reference file). Not every addition needs a patch: if the change is only meaningful at the introducing spec version (e.g. a new required field or other breaking change with a clear minimum format_version), rely on version scoping instead of remove patches when appropriate.

• The remove order matters: property $ref entries must be removed before the definition

they reference.

• Shared fields used in multiple spec files must be defined once in

spec/integration/manifest.spec.yml and referenced via $ref elsewhere — not duplicated inline.

If semantic validators changed

Consult the "Semantic validators" section of the reference file.

• New validator function: check it is registered in code/go/internal/validator/spec.go

in the rulesDef slice, with appropriate fn, since, and types fields.

• Tests: at least one of the following must be present:
• A *_test.go file alongside the validator using t.TempDir() (simple/single-file rules).
• New test cases in code/go/pkg/validator/validator_test.go referencing packages in

test/packages/ (complex multi-file scenarios).

If test packages changed

Consult the "Test packages" section of the reference file.

• Prefer scaffolding with `elastic-package create package` or, for new data streams in an

existing package, `elastic-package create data-stream` (from that package’s directory), over manual copies.

• Each package under test/packages/ or compliance/testdata/packages/ must contain:

manifest.yml, changelog.yml, docs/README.md.

• Any data stream must include a valid ingest pipeline: processors need a tag field, and

there must be an on_failure block that sets event.kind: pipeline_error and error.message to a string containing the three Handlebars placeholders.

If compliance feature files changed

Consult the "Compliance features" section of the reference file.

• Every Scenario: must have a @X.Y.Z version tag immediately above it.
• A @skip tag must always be paired with a # Pending on <url> comment on the next line.
• When @skip is added, the corresponding spec/changelog.yml entry must also have a

matching # Pending on <url> comment.

If new Go files added

• Every new .go file must start with the Elastic license header:

  // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
  // or more contributor license agreements. Licensed under the Elastic License;
  // you may not use this file except in compliance with the Elastic License.

• Variable and function names must be unabbreviated (e.g., packageName not pkgName,

dataStreamManifests not dsManifests).

• Indentation must use tabs, not spaces.

---

Step 4 — Run Local Validation

If a Go toolchain and make are available in the environment, run at minimum (same order as CONTRIBUTING.md):

# Validate spec YAML syntax (fast, always do this after spec changes)
go test ./code/go/internal

# Lint and license headers
make check

Optional (broader validation): When the diff warrants it, also run make test for the full suite (CONTRIBUTING.md). For changes that should be exercised across many real packages, see Testing with integrations repository in CONTRIBUTING.md (e.g. test integrations PR comment and closing the spawned integrations PR when done).

Record validation outcomes only as described in Step 5 (short summary per command; on failure, include a small excerpt only). If the toolchain is not available, note skipped for those commands.

---

Step 5 — Write the Report

Write a short report to .cursor/review-pr-output.md, overwriting any previous run. Use plain text only (no emoji or decorative symbols). Aim for roughly one screen of content.

Include only what follows. Omit empty sections entirely.

# PR review: <branch or PR title>
<!-- reviewed: <ISO date> -->

## Scope

At most 2–4 bullets: what the diff changes, only if that context helps a reviewer. Omit this section if the PR title and `git diff --stat` are enough and nothing needs extra review.

## Extra review

Bullets **only** for items that need human attention: checklist gaps, blockers, open questions, or parts of the change that are easy to get wrong (for example new `versions` patches, new semantic validators, `spec/changelog.yml` entries, compliance `@skip`, or cross-file `$ref`). For each bullet use `path:approximate-line` (or `path` and a short region description) plus **one sentence**. Prefix with `blocker`, `suggestion`, or `question` when useful.

If there is nothing to flag here and validation passed, use a single line instead of a list:

No items flagged for extra review.

## Validation

One line per command you ran, for example: `go test ./code/go/internal: pass`. Use `fail` or `skipped` as appropriate. If a command failed, add a **short** excerpt (failing test name or error lines only), not the full log. If no commands ran, one line: `Validation: skipped (toolchain not available).`

Do not add per-topic sections (changelog, version patches, tests, etc.) when there is nothing to say. Do not paste full go test or make check output on success. Do not list files that look fine.

After writing the file, print this message to the user:

Review written to .cursor/review-pr-output.md

Validating this skill

After substantive edits to this skill, run (if installed):

skills-ref validate ./.cursor/skills/review-pr

package-spec PR Review Checklist — Detailed Reference

This file contains the full rules, rationale, and examples for each review category. It is referenced by SKILL.md and is loaded on demand when deeper detail is needed.

The human-oriented contributor guide (CONTRIBUTING.md) covers the same development workflows (Makefile targets, test packages, integrations testing) in narrative form. Prefer that document for onboarding; use this checklist for systematic PR review.

---

0. Contribution process

When it applies: Large or cross-product spec changes, or new package categories.

Review hint: If the diff is a substantial spec or validator change and the PR description does not reference a proposal or tracking issue, add an extra-review note asking the author to confirm process was followed.

---

1. Changelog

File: spec/changelog.yml

Changelog Format

- version: 3.7.0-next        # in-development version always has -next suffix
  changes:
    - description: Brief description of the change ending with a period.
      type: enhancement        # enhancement | bugfix | breaking-change
      link: https://github.com/elastic/package-spec/pull/NUMBER

Changelog Rules

Pending features

When a spec feature is complete but blocked on upstream (Kibana, elastic-package):

    # Pending on https://github.com/elastic/kibana/issues/NNNNNN
    - description: Add support for foo.
      type: enhancement
      link: https://github.com/elastic/package-spec/pull/NNN

• The # Pending on <url> comment must appear on the line directly above the entry.
• The corresponding compliance .feature file must also have a @skip tag with the same URL.
• Remove both when the blocker is resolved.

---

2. Version Patches

Why they exist: The spec supports multiple format versions. When a single .spec.yml still validates older format_version values, remove patches drop newer optional properties and definitions from the schema those older versions use. Not every change needs a remove patch: if the change applies only at the introducing version (e.g. a new required field or breaking change with a clear minimum format_version), version scoping may be enough—see the Patch Rules table below.

Where they live: In the same .spec.yml file that defines the new field, under a versions key at the top level.

Patch Format

Add each new patch block at the top of the versions list so newer spec versions come first (higher before value above lower). When you add a patch for a new release, insert a new list item at the top—do not append at the bottom.

versions:
  # Newer `before` first — insert new patch blocks here, above existing entries.
  - before: 3.7.0
    patch:
      - op: remove
        path: "/properties/recent_field"
      - op: remove
        path: "/definitions/recent_field"
  - before: 3.6.0
    patch:
      - op: remove
        path: "/properties/my_new_field"
      - op: remove
        path: "/definitions/my_new_definition"

Patch Rules

Comment convention examples

# GOOD — comment on path line, only for complex path
- op: add
  path: "/properties/policy_templates/items/required/-"  # re-add type as required
  value: type

# GOOD — no comment for simple path
- op: remove
  path: "/properties/my_field"

# BAD — comment on op line
- op: remove  # removes my_field
  path: "/properties/my_field"

# BAD — redundant comment for obvious path
- op: remove
  path: "/properties/my_field"  # removes my_field property

Version Patches Checklist

• [ ] Every new properties or definitions entry that older format_version values must not include has a matching remove patch (or the change is correctly scoped to the introducing version only, e.g. required/breaking at a minimum format_version).
• [ ] Properties are removed before their definitions in the patch list.
• [ ] _dev files are excluded.
• [ ] Comments follow the path-line convention.

---

3. Schema Reuse

Why it matters: Duplicate inline definitions drift out of sync. The spec uses JSON Schema $ref to define a field once and reference it everywhere.

When to use $ref

If the same field definition appears in two or more spec files: define it once in spec/integration/manifest.spec.yml under definitions, then reference it everywhere else.

Cross-file reference syntax

# Same file
$ref: "#/definitions/my_field"

# From spec/input/manifest.spec.yml referencing spec/integration/manifest.spec.yml
$ref: "../integration/manifest.spec.yml#/definitions/my_field"

# From spec/integration/data_stream/manifest.spec.yml
$ref: "../../integration/manifest.spec.yml#/definitions/my_field"

Always use relative paths (starting with ../). Absolute or root-relative paths break portability.

additionalProperties: false

Objects should have additionalProperties: false to prevent undeclared fields from silently passing validation. Flag if a new object type is missing this.

Schema Reuse Checklist

• [ ] No field definition appears inline in two different .spec.yml files.
• [ ] Shared definitions live in spec/integration/manifest.spec.yml.
• [ ] All $ref uses relative paths.
• [ ] New object schemas include additionalProperties: false where appropriate.

---

4. Semantic Validators

Location: code/go/internal/validator/semantic/

Registration

New validator functions must be registered in code/go/internal/validator/spec.go inside the rulesDef slice within the rules() function:

// Minimal — applies to all package types and all versions
{fn: semantic.ValidateMyRule},

// With version gate — only applies for spec >= 3.7.0
{fn: semantic.ValidateMyRule, since: semver.MustParse("3.7.0")},

// With package type filter
{fn: semantic.ValidateMyRule, types: []string{"integration", "input"}, since: semver.MustParse("3.7.0")},

// With upper bound — only applies before 3.0.0
{fn: warnOn(semantic.ValidateMyRule), until: semver.MustParse("3.0.0")},

Fields:

• fn — required, the validator function
• since — optional, minimum spec version (inclusive)
• until — optional, maximum spec version (exclusive)
• types — optional, package types this applies to; omit to apply to all types

Semantic Validators Checklist

• [ ] New ValidateXxx function is present in the rulesDef slice.
• [ ] since is set when the rule corresponds to a new spec feature (should match the spec

version being targeted).

• [ ] types is set when the rule is not universally applicable (e.g., only for integration

packages).

• [ ] warnOn() is used only for rules that are being phased in (deprecation warnings before

becoming hard errors in a future version).

---

5. Tests for Semantic Validators

Two testing strategies; choose based on complexity:

Simple: unit test with t.TempDir()

Use when the validator reads a single file or a simple directory structure.

func TestValidateMyRule(t *testing.T) {
    tests := map[string]struct {
        manifest      string
        expectError   bool
        errorContains string
    }{
        "valid": {
            manifest:    "name: test\nformat_version: 3.7.0\n",
            expectError: false,
        },
        "invalid_missing_field": {
            manifest:      "name: test\nformat_version: 3.7.0\nbad_field: value\n",
            expectError:   true,
            errorContains: "bad_field",
        },
    }
    for name, tc := range tests {
        t.Run(name, func(t *testing.T) {
            pkgRoot := t.TempDir()
            err := os.WriteFile(filepath.Join(pkgRoot, "manifest.yml"),
                []byte(tc.manifest), 0644)
            require.NoError(t, err)
            fsys := fspath.DirFS(pkgRoot)
            errs := semantic.ValidateMyRule(fsys)
            if tc.expectError {
                require.NotEmpty(t, errs)
                require.Contains(t, errs[0].Error(), tc.errorContains)
            } else {
                require.Empty(t, errs)
            }
        })
    }
}

Complex: test packages + validator_test.go

Use when the rule requires a full package structure (multiple data streams, pipelines, etc.).

1. Scaffold with `elastic-package create package` (cd test/packages first), or add a data stream with `elastic-package create data-stream` from inside the package directory. 2. Modify manifest.yml (and data stream files if applicable) to exercise the rule. 3. Add entries to code/go/pkg/validator/validator_test.go:

tests := map[string]struct {
    invalidPkgFilePath  string
    expectedErrContains []string
}{
    "good_my_feature": {},   // valid — no errors expected
    "bad_my_feature": {
        "manifest.yml",
        []string{`expected error substring here`},
    },
}

Validator Tests Checklist

• [ ] At least one of the above test strategies is present for every new validator.
• [ ] Tests cover both the valid and invalid cases.
• [ ] Error message substrings in expectedErrContains are specific enough to be meaningful.

---

6. Test Packages

Location: test/packages/ (general), compliance/testdata/packages/ (compliance-only)

Required files

The layout below is what `elastic-package` scaffolds when you create artifacts interactively: use `elastic-package create package` for a new package (integration, input, or content), or `elastic-package create data-stream` to add a data stream under an existing package. Prefer those commands over copying folders by hand so required files stay consistent.

Every package directory must contain:

my_package/
├── manifest.yml          # package manifest
├── changelog.yml         # at minimum: one version entry with at least one change
└── docs/
    └── README.md         # package documentation

Valid ingest pipeline

Any data stream that includes an ingest pipeline must follow this structure:

---
description: Pipeline description.
processors:
  - set:
      tag: set_field_name     # tag is required on every processor
      field: my.field
      value: test
on_failure:                   # on_failure block is required
  - set:
      field: event.kind
      value: pipeline_error
  - set:
      field: error.message
      value: >-
        Processor '{{{ _ingest.on_failure_processor_type }}}'
        with tag '{{{ _ingest.on_failure_processor_tag }}}'
        in pipeline '{{{ _ingest.pipeline }}}'
        failed with message '{{{ _ingest.on_failure_message }}}'

Required elements:

• Every processor must have a tag field.
• on_failure must set event.kind to pipeline_error.
• error.message must include all three Handlebars placeholders:

_ingest.on_failure_processor_type, _ingest.on_failure_processor_tag, _ingest.pipeline. (A fourth, _ingest.on_failure_message, is also included by convention.)

Transform packages (compliance only)

Transforms in compliance/testdata/packages/ that are installed and uninstalled across test runs must use:

dest:
  index: "metrics-mypackage.my_dest_default"  # no leading dot — hidden indices cause 403 on uninstall
_meta:
  managed: true
  run_as_kibana_system: false  # use logged-in user credentials, not kibana_system

Test Packages Checklist

• [ ] manifest.yml, changelog.yml, and docs/README.md all present.
• [ ] Data stream ingest pipelines have tag on each processor and a valid on_failure block.
• [ ] Compliance-only transform packages use non-hidden dest index and run_as_kibana_system: false.
• [ ] Prefer `elastic-package create package` or `elastic-package create data-stream` over

manual scaffolding when adding a test package or data stream.

---

7. Compliance Feature Files

Location: compliance/features/*.feature

Version tagging

Every Scenario: must be tagged with the minimum spec version that introduced the feature:

  @3.6.0
  Scenario: Integration package with named input can be installed
   Given the "good_v3" package is installed
   ...

The tag controls whether the scenario runs: it is skipped if the TEST_SPEC_VERSION environment variable is lower than the tag.

Skipped scenarios

When a scenario cannot pass because upstream (Kibana or elastic-package) hasn't implemented support yet:

  @3.6.0
  @skip
  # Pending support for qualified input names: https://github.com/elastic/kibana/pull/262138
  Scenario: Integration package with OTel input can be installed
   Given the "good_v3" package is installed
   ...

Rules:

• @skip must appear directly after the version tag.
• A # Pending <description>: <url> comment must appear on the line after @skip.
• The corresponding spec/changelog.yml entry must have a matching # Pending on <url>

comment above it.

• Both the @skip and the # Pending on comment in the changelog must be removed together

when the blocker is resolved.

Compliance Features Checklist

• [ ] Every Scenario: has a @X.Y.Z tag.
• [ ] Every @skip is accompanied by a # Pending ... comment with a URL.
• [ ] Changelog entry for the same feature has a matching # Pending on <url> comment.
• [ ] No scenario has @skip without a tracking issue URL.

---

8. Go Style

License header

Every new .go file must begin with:

// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

Enforced by make check. Running make -C code/go update adds headers automatically.

Naming conventions

Exception: loop variables (i, j) and widely accepted short names (err, ok) are fine.

Indentation

Go files use tabs, not spaces. gofmt enforces this — run make -C code/go format.

Error messages

When creating structured errors with file paths, use fsys.Path("relative/path") rather than file.Path() to get the full package-relative path expected by the test framework:

// Good
specerrors.NewStructuredErrorf("file %q is invalid: %s", fsys.Path("_dev/test/config.yml"), err)

// Bad — path is relative within fsys, not the full package path
specerrors.NewStructuredErrorf("file %q is invalid: %s", config.Path(), err)

Go Style Checklist

• [ ] License header present in every new .go file.
• [ ] No abbreviations in variable/function names (outside err, ok, loop vars).
• [ ] Indentation uses tabs.
• [ ] Error messages use fsys.Path() for file path references.