post Saved Objects CI check feedback as a PR comment by hammad-nasir-elastic · Pull Request #268469 · elastic/kibana

hammad-nasir-elastic · 2026-05-08T14:42:40Z

resolves elastic/kibana-team#3141

Summary

Today, when the Saved Objects CI check runs on a PR, the only signal a developer gets is a green or red tile in Buildkite. If it fails, you have to dig through nested build logs (or ping the Core team) to figure out which rule fired and how to fix it. If it passes but you changed a Saved Object type, there's no nudge to think about whether your change needs a 2-step release.

This PR makes the check explain itself directly on the PR. After every run on a pull request, you'll see one of:

Failure — a comment listing the violations grouped by SO type, with a one-line fix hint and a link to the relevant docs section.
Success with SO changes — a comment summarizing which types were added / updated / removed, with a 2-step-release reminder when applicable.
Success with no SO changes — no comment.

The comment is idempotent: re-runs replace it instead of stacking new ones, the same pattern already used by the API contracts notifier. This closes the 4th done-criterion of elastic/kibana-team#3139.

How it works (high level)

The CLI (scripts/check_saved_objects) now emits a structured JSON report alongside its normal output (new --report-path flag).
A small Buildkite script reads that report and posts (or updates) the PR comment.
Validators have been updated to attach a stable rule ID, fix hint, and docs link to each violation, so the comment can render them as actionable items rather than raw stack traces.

How to test

Local smoke (no comment posted):

node scripts/check_saved_objects --baseline <merge-base-sha> --report-path /tmp/r.json
cat /tmp/r.json | jq

Unit tests:

node scripts/jest packages/kbn-check-saved-objects-cli
node scripts/jest .buildkite/scripts/steps/checks/notify_saved_objects_changes.test.ts

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support — N/A, CI-only output (Buildkite logs + GitHub PR markdown).
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations. — no HTTP API changes.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
Review the backport guidelines and apply applicable backport:* labels.

Identify risks

Comment spam on re-run — avoided by re-using a stable comment context, so each new run updates the existing comment in place.
Notifier failure breaks the build — the notifier is fire-and-forget; if it errors, the step still passes and prints a warning.
Soft-fail still on — .buildkite/pipelines/pull_request/check_saved_objects.yml keeps soft_fail: true for now. Flipping it to a hard fail is a separate done-criterion of elastic/kibana-team#3139.

The check_saved_objects CI step now writes a structured JSON report and upserts a single PR comment. Failures are grouped by SO type with ruleId, message, fix hint, and a deep docs link; successes that touched SO types include a 2-step release reminder. Comments use a stable context so re-runs replace rather than stack. Closes elastic/kibana-team#3141 Co-authored-by: Cursor <cursoragent@cursor.com>

infra-vault-gh-plugin-prod · 2026-05-08T18:01:55Z

Pinging @elastic/kibana-core (Team:Core)

- buildFailureBody: empty-findings fallback links to Buildkite logs - check_saved_objects.sh: run check_for_changed_files unconditionally after --fix - detect_conflicts_removed_types / update_removed_types: emit one finding per type - buildSuccessBody: only show 2-step release reminder for updated/removed types - notify_saved_objects_changes: rename errorCount -> findingCount - existing_type: fix 'deprected' -> 'deprecated' typo - types.ts: document docsAnchor format requirement - consolidate SavedObjectsCheckError to accept single or multiple findings, drop SavedObjectsCheckMultiError Co-authored-by: Cursor <cursoragent@cursor.com>

gsoldevila

LGTM!

kibanamachine · 2026-05-13T20:19:14Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 8a08aac

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #43 / discover/ccs_compatible discover search CCS cancel classic mode should show warning and results

Metrics [docs]

✅ unchanged

History

💛 Build #440904 was flaky ccd4ff6
💔 Build #440874 failed e5b566f
💚 Build #440217 succeeded cef4b30

…mas and granular diff Replaces the `validateModelVersionsChanges` / `validateNoModelVersionChanges` split with a single `validateNoStructuralModelVersionChanges` that applies granular schema diffing uniformly across all model versions and all validation contexts (local and serverless baseline). Key changes: - Store schemas as serialized Joi describe() output in snapshots instead of opaque SHA-256 hashes, enabling field-level diff comparisons. - `validateNoStructuralModelVersionChanges` classifies changes as: - Structural mutations → throw (always forbidden) - Breaking schema changes (field removed, type changed, optional→required, new required field) → throw - Undiffable changes against legacy hash baselines → throw (forces baseline regeneration) - Constraint tightening → warn (logged) - Non-breaking changes (new optional fields, loosened constraints) → allow - Backward compatibility: cross-format comparison between old hash strings and new serialized objects is supported via legacyHash matching, including correct reproduction of old hashes for function-based schemas. - Compact (no-whitespace) JSON for GCS snapshots; pretty-printed for local mock files. - Adds two new RULE_IDs (EXISTING_TYPE_SCHEMA_BREAKING_CHANGES, EXISTING_TYPE_SCHEMA_UNDIFFABLE) and converts structural-mutation throws to SavedObjectsCheckError for PR comment integration. - Picks up `validateIgnoreAboveExistingType` from main (elastic#268469). Co-authored-by: Cursor <cursoragent@cursor.com>

…mas + granular diff (#268630) ## Summary Extends the schema-only mutation tolerance introduced in #256432 with the following improvements: ### 1. Serialised schemas in snapshots Instead of a 64-byte SHA-256 hash, each model version's \`create\` and \`forwardCompatibility\` schemas are now stored as their full Joi \`describe()\` output (\`Record<string, unknown>\`). This enables field-level diffing across CI runs. > **Backward compatibility** — when the current snapshot contains a Joi object but the GCS baseline still contains the old hash string, the check computes a legacy hash on-the-fly (via \`computeSchemaHash\`) and compares it against the stored hash. This keeps cross-format comparisons correct during the transition window, including for function-based \`forwardCompatibility\` schemas (e.g. fleet, SLO), which the old code hashed as \`SHA256(fn.toString())\`. ### 2. Granular schema diff When the CI check detects a schema-only mutation, it now classifies each field change: | Change | \`create\` schema | \`forwardCompatibility\` schema | |--------|-----------------|-------------------------------| | Schema removed from model version | ❌ error | ❌ error | | Field removed | ❌ error | ❌ error | | Field type changed | ❌ error | ❌ error | | Optional → required / new required field | ❌ error | ❌ error | | New optional field added | ✅ allow | ✅ allow | | Required → optional field | ✅ allow | ✅ allow | | Constraint tightened / validator added | ⚠️ warning | ⚠️ warning | | Constraint loosened / validator removed | ✅ allow | ✅ allow | When the diff cannot determine whether a change is safe (baseline uses a legacy hash string), CI **throws** instead of warning, forcing a baseline regeneration before the PR can merge. ### 3. Complete migration to \`SavedObjectsCheckError\` All remaining \`throw new Error()\` calls in the validator have been migrated to \`SavedObjectsCheckError\` (introduced by #268469). Each violation now carries a structured \`ruleId\`, \`fixHint\`, and \`docsAnchor\` for actionable CI PR comments. New \`RULE_IDS\` added: | Rule ID | Violation | |---------|-----------| | \`existing-type/schema-breaking-changes\` | Breaking schema change in an existing model version | | \`existing-type/schema-undiffable-legacy-hash\` | Schema changed but baseline uses legacy hash — cannot diff | | \`existing-type/new-mappings-not-in-model-version\` | New mapping fields not declared in the model version | | \`existing-type/keyword-missing-ignore-above\` | Newly introduced keyword/flattened fields without \`ignore_above\` | | \`existing-type/invalid-name-title-field-type\` | New name/title field with incorrect mapping type | | \`model-version/mappings-not-in-schema\` | Mapping fields absent from the latest model version schema | | \`model-version/mapping-index-false\` | New mapping field uses \`index: false\` | | \`model-version/mapping-enabled-false\` | New mapping field uses \`enabled: false\` | ### Snapshot size | Format | On-disk size | |--------|-------------| | Old (hash strings) | ~137 KB | | Full Joi \`describe()\` output, pretty-printed (rejected) | ~15 MB | | Full Joi \`describe()\` output, compact JSON (current) | **~1.7 MB** | Snapshots are stored in GCS and not committed to the repository. Mock JSON files under \`src/snapshots/mocks/\` remain pretty-printed for readability. ## Test plan - [x] All unit tests in \`kbn-check-saved-objects-cli\` and \`so-type-serializer\` pass - [x] Tests cover: field removal (throws), schema removal (throws), required field addition (throws), optional field addition (allows), constraint tightening (warns) - [x] Tests for backward compat against hash-based baselines: schema unchanged (no false positive), function-based schema unchanged (no false positive), schema changed against hash baseline (throws) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

resolves elastic/kibana-team#3141 ## Summary Today, when the Saved Objects CI check runs on a PR, the only signal a developer gets is a green or red tile in Buildkite. If it fails, you have to dig through nested build logs (or ping the Core team) to figure out which rule fired and how to fix it. If it passes but you changed a Saved Object type, there's no nudge to think about whether your change needs a [2-step release](https://www.elastic.co/docs/extend/kibana/saved-objects#defining-model-versions). This PR makes the check explain itself directly on the PR. After every run on a pull request, you'll see one of: - **Failure** — a comment listing the violations grouped by SO type, with a one-line fix hint and a link to the relevant docs section. - **Success with SO changes** — a comment summarizing which types were added / updated / removed, with a 2-step-release reminder when applicable. - **Success with no SO changes** — no comment. The comment is idempotent: re-runs replace it instead of stacking new ones, the same pattern already used by the API contracts notifier. This closes the 4th done-criterion of elastic/kibana-team#3139. ### How it works (high level) 1. The CLI (`scripts/check_saved_objects`) now emits a structured JSON report alongside its normal output (new `--report-path` flag). 2. A small Buildkite script reads that report and posts (or updates) the PR comment. 3. Validators have been updated to attach a stable rule ID, fix hint, and docs link to each violation, so the comment can render them as actionable items rather than raw stack traces. ## How to test **Local smoke (no comment posted):** ``` node scripts/check_saved_objects --baseline <merge-base-sha> --report-path /tmp/r.json cat /tmp/r.json | jq ``` **Unit tests:** ``` node scripts/jest packages/kbn-check-saved-objects-cli node scripts/jest .buildkite/scripts/steps/checks/notify_saved_objects_changes.test.ts ``` ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) — N/A, CI-only output (Buildkite logs + GitHub PR markdown). - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. — no HTTP API changes. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks - **Comment spam on re-run** — avoided by re-using a stable comment context, so each new run updates the existing comment in place. - **Notifier failure breaks the build** — the notifier is fire-and-forget; if it errors, the step still passes and prints a warning. - **Soft-fail still on** — `.buildkite/pipelines/pull_request/check_saved_objects.yml` keeps `soft_fail: true` for now. Flipping it to a hard fail is a separate done-criterion of elastic/kibana-team#3139. --------- Co-authored-by: Cursor <cursoragent@cursor.com>

…mas + granular diff (#268630) ## Summary Extends the schema-only mutation tolerance introduced in #256432 with the following improvements: ### 1. Serialised schemas in snapshots Instead of a 64-byte SHA-256 hash, each model version's \`create\` and \`forwardCompatibility\` schemas are now stored as their full Joi \`describe()\` output (\`Record<string, unknown>\`). This enables field-level diffing across CI runs. > **Backward compatibility** — when the current snapshot contains a Joi object but the GCS baseline still contains the old hash string, the check computes a legacy hash on-the-fly (via \`computeSchemaHash\`) and compares it against the stored hash. This keeps cross-format comparisons correct during the transition window, including for function-based \`forwardCompatibility\` schemas (e.g. fleet, SLO), which the old code hashed as \`SHA256(fn.toString())\`. ### 2. Granular schema diff When the CI check detects a schema-only mutation, it now classifies each field change: | Change | \`create\` schema | \`forwardCompatibility\` schema | |--------|-----------------|-------------------------------| | Schema removed from model version | ❌ error | ❌ error | | Field removed | ❌ error | ❌ error | | Field type changed | ❌ error | ❌ error | | Optional → required / new required field | ❌ error | ❌ error | | New optional field added | ✅ allow | ✅ allow | | Required → optional field | ✅ allow | ✅ allow | | Constraint tightened / validator added | ⚠️ warning | ⚠️ warning | | Constraint loosened / validator removed | ✅ allow | ✅ allow | When the diff cannot determine whether a change is safe (baseline uses a legacy hash string), CI **throws** instead of warning, forcing a baseline regeneration before the PR can merge. ### 3. Complete migration to \`SavedObjectsCheckError\` All remaining \`throw new Error()\` calls in the validator have been migrated to \`SavedObjectsCheckError\` (introduced by #268469). Each violation now carries a structured \`ruleId\`, \`fixHint\`, and \`docsAnchor\` for actionable CI PR comments. New \`RULE_IDS\` added: | Rule ID | Violation | |---------|-----------| | \`existing-type/schema-breaking-changes\` | Breaking schema change in an existing model version | | \`existing-type/schema-undiffable-legacy-hash\` | Schema changed but baseline uses legacy hash — cannot diff | | \`existing-type/new-mappings-not-in-model-version\` | New mapping fields not declared in the model version | | \`existing-type/keyword-missing-ignore-above\` | Newly introduced keyword/flattened fields without \`ignore_above\` | | \`existing-type/invalid-name-title-field-type\` | New name/title field with incorrect mapping type | | \`model-version/mappings-not-in-schema\` | Mapping fields absent from the latest model version schema | | \`model-version/mapping-index-false\` | New mapping field uses \`index: false\` | | \`model-version/mapping-enabled-false\` | New mapping field uses \`enabled: false\` | ### Snapshot size | Format | On-disk size | |--------|-------------| | Old (hash strings) | ~137 KB | | Full Joi \`describe()\` output, pretty-printed (rejected) | ~15 MB | | Full Joi \`describe()\` output, compact JSON (current) | **~1.7 MB** | Snapshots are stored in GCS and not committed to the repository. Mock JSON files under \`src/snapshots/mocks/\` remain pretty-printed for readability. ## Test plan - [x] All unit tests in \`kbn-check-saved-objects-cli\` and \`so-type-serializer\` pass - [x] Tests cover: field removal (throws), schema removal (throws), required field addition (throws), optional field addition (allows), constraint tightening (warns) - [x] Tests for backward compat against hash-based baselines: schema unchanged (no false positive), function-based schema unchanged (no false positive), schema changed against hash baseline (throws) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

…format (#270927) ## Summary Updates `docs/extend/saved-objects/validate.md` to reflect the structured error reporting introduced in #268469 and the additional validation rules added in subsequent PRs. ### What changed **Format**: The CI check now posts a structured PR comment (`**[rule-id]** Message. _Fix:_ …`) instead of raw `❌` terminal output. The troubleshooting intro is updated to explain the new format and show how to reproduce findings locally. **New rules documented** (were missing from the original section): | Rule ID | Introduced in | |---|---| | `existing-type/schema-breaking-changes` | #268630 | | `existing-type/schema-undiffable-legacy-hash` | #268630 | | `existing-type/new-mappings-not-in-model-version` | #268630 | | `existing-type/keyword-missing-ignore-above` | #268630 | | `existing-type/invalid-name-title-field-type` | #268630 | | `new-type/missing-initial-model-version` | #268469 | | `new-type/legacy-migrations` | #268469 | | `new-type/keyword-missing-ignore-above` | #270541 | | `new-type/invalid-name-title-field-type` | #270541 | | `model-version/mappings-not-in-schema` | #268630 | | `model-version/mapping-index-false` | #268630 | | `model-version/mapping-enabled-false` | #268630 | | `model-version/fixture-missing` | #270541 | | `model-version/fixture-invalid` | #270541 | | `documents/fixture-mismatch` | #270541 | **Structure**: Rules are now grouped into categories (existing-type / new-type / model-version / documents / removed-type) with stable anchor IDs on every rule heading, so `([docs](link))` references in PR comments land directly on the right entry. ## Test plan - [ ] Visual review of rendered markdown in the Elastic docs preview Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com>

…format (elastic#270927) ## Summary Updates `docs/extend/saved-objects/validate.md` to reflect the structured error reporting introduced in elastic#268469 and the additional validation rules added in subsequent PRs. ### What changed **Format**: The CI check now posts a structured PR comment (`**[rule-id]** Message. _Fix:_ …`) instead of raw `❌` terminal output. The troubleshooting intro is updated to explain the new format and show how to reproduce findings locally. **New rules documented** (were missing from the original section): | Rule ID | Introduced in | |---|---| | `existing-type/schema-breaking-changes` | elastic#268630 | | `existing-type/schema-undiffable-legacy-hash` | elastic#268630 | | `existing-type/new-mappings-not-in-model-version` | elastic#268630 | | `existing-type/keyword-missing-ignore-above` | elastic#268630 | | `existing-type/invalid-name-title-field-type` | elastic#268630 | | `new-type/missing-initial-model-version` | elastic#268469 | | `new-type/legacy-migrations` | elastic#268469 | | `new-type/keyword-missing-ignore-above` | elastic#270541 | | `new-type/invalid-name-title-field-type` | elastic#270541 | | `model-version/mappings-not-in-schema` | elastic#268630 | | `model-version/mapping-index-false` | elastic#268630 | | `model-version/mapping-enabled-false` | elastic#268630 | | `model-version/fixture-missing` | elastic#270541 | | `model-version/fixture-invalid` | elastic#270541 | | `documents/fixture-mismatch` | elastic#270541 | **Structure**: Rules are now grouped into categories (existing-type / new-type / model-version / documents / removed-type) with stable anchor IDs on every rule heading, so `([docs](link))` references in PR comments land directly on the right entry. ## Test plan - [ ] Visual review of rendered markdown in the Elastic docs preview Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com>

hammad-nasir-elastic added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels May 8, 2026

hammad-nasir-elastic added 6 commits May 8, 2026 11:20

Merge branch 'main' into so-automated-feedback-prs

9246983

fix and testing CI

e3fb498

testing SO CI with dashboard change

c2613a1

undo dashboard

c6ef481

testing

57272f2

undo

ead2f23

hammad-nasir-elastic changed the title ~~feat: post Saved Objects CI check feedback as a PR comment~~ post Saved Objects CI check feedback as a PR comment May 8, 2026

undo

cef4b30

elastic deleted a comment from kibanamachine May 8, 2026

hammad-nasir-elastic marked this pull request as ready for review May 8, 2026 18:01

hammad-nasir-elastic requested review from a team as code owners May 8, 2026 18:01

gsoldevila reviewed May 11, 2026

View reviewed changes

hammad-nasir-elastic and others added 3 commits May 11, 2026 12:31

Merge branch 'main' into so-automated-feedback-prs

e5b566f

Merge branch 'main' into so-automated-feedback-prs

ccd4ff6

gsoldevila mentioned this pull request May 12, 2026

[SavedObjects] Improve schema-only mutation handling: serialized schemas + granular diff #268630

Merged

3 tasks

gsoldevila approved these changes May 13, 2026

View reviewed changes

Merge branch 'main' into so-automated-feedback-prs

8a08aac

tylersmalley approved these changes May 15, 2026

View reviewed changes

hammad-nasir-elastic merged commit 658427c into elastic:main May 15, 2026
33 checks passed

kibanamachine added the v9.5.0 label May 15, 2026

gsoldevila mentioned this pull request May 25, 2026

[docs] Update SO validate troubleshooting section for new PR comment format #270927

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

post Saved Objects CI check feedback as a PR comment#268469

post Saved Objects CI check feedback as a PR comment#268469
hammad-nasir-elastic merged 12 commits into
elastic:mainfrom
hammad-nasir-elastic:so-automated-feedback-prs

hammad-nasir-elastic commented May 8, 2026 •

edited

Loading

Uh oh!

infra-vault-gh-plugin-prod Bot commented May 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsoldevila left a comment

Uh oh!

kibanamachine commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

hammad-nasir-elastic commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How it works (high level)

How to test

Checklist

Identify risks

Uh oh!

infra-vault-gh-plugin-prod Bot commented May 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsoldevila left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 13, 2026

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hammad-nasir-elastic commented May 8, 2026 •

edited

Loading