Skip to content

[Chore] Add Libra review configuration - #10210

Merged
clintandrewhall merged 5 commits into
mainfrom
claude/epic-cannon-y8sh4w
Oct 14, 2026
Merged

clintandrewhall merged 5 commits into
mainfrom
claude/epic-cannon-y8sh4w

Conversation

@clintandrewhall

@clintandrewhall clintandrewhall commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • What: Adds Libra repository configuration under .libra/: settings.yaml, instructions.md, and four path-scoped specialists (public-api, accessibility, styles-theming, docs-and-changelog).
  • Why: Gives Libra EUI-specific review guidance, so reviews focus on consumer-facing impact (Kibana and other downstream products) rather than formatting or wording nits.
  • How:
    • Drafts aren't reviewed by default. The libra-review-draft label opts a draft in and libra-skip skips a PR.
    • Lockfiles, yearly changelogs, i18n token files, Jest snapshots, and icon SVG sources and generated assets are excluded from review.
    • Findings below P3 aren't published. Blocking stays at P1, and only the public-api specialist can request changes.

The config is based on the Libra repository configuration reference. libra config lint hasn't been run, and the Libra Config Lint status won't appear until Libra's webhook is installed on this repo (see the onboarding doc). Libra reads config from the default branch, so nothing takes effect until this merges.

API Changes

None. This adds review tooling configuration only.

Impact Assessment

  • 🔴 Breaking changes: none
  • 💅 Visual changes: none
  • 🧪 Test impact: none
  • 🔧 Hard to integrate: none

Impact level: 🟢 None

Release Readiness

  • Documentation
  • Figma
  • Migration guide
  • Adoption plan

No changelog or skip-changelog label is needed: .libra/ is outside every package, so the changelog check passes as is.

QA instructions for reviewer

  • Sanity-check the paths.exclude globs and the apply_to patterns in .libra/specialists/*.md against the repo layout.
  • Check that the label names (libra-skip, libra-review-draft) are what the team wants.

🤖 Generated with Claude Code

https://claude.ai/code/session_012GMt5khuQyoinpozhcVBDB

@clintandrewhall
clintandrewhall requested a review from a team as a code owner October 14, 2026 17:43
Copilot AI balanced review requested due to automatic review settings October 14, 2026 17:43
@cla-checker-service

cla-checker-service Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

💚 CLA has been signed

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GMt5khuQyoinpozhcVBDB
@clintandrewhall
clintandrewhall force-pushed the claude/epic-cannon-y8sh4w branch from f5499a0 to 6701343 Compare October 14, 2026 17:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Specialist scopes omit public API modules and component TypeScript files containing keyboard-navigation logic.

2 open findings
What changed in this PR

Adds Libra configuration to focus EUI reviews on downstream compatibility and consumer-facing regressions.

Changes:

  • Configures draft opt-in, exclusions, and publication thresholds.
  • Adds repository-wide review guidance linked to the wiki.
  • Defines four path-scoped specialists.
File Description
.libra/​specialists/​styles-theming.md Adds styling and theme compatibility guidance.
.libra/​specialists/​public-api.md Defines the blocking API compatibility specialist.
.libra/​specialists/​docs-and-changelog.md Adds documentation and changelog guidance.
.libra/​specialists/​accessibility.md Adds keyboard, focus, and accessibility guidance.
.libra/​settings.yaml Sets review triggers, exclusions, and thresholds.
.libra/​instructions.md Establishes shared EUI review priorities.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .libra/specialists/public-api.md Outdated
Comment thread .libra/specialists/accessibility.md
…rted utils

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GMt5khuQyoinpozhcVBDB
@Dosant

Dosant commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

@clintandrewhall, awesome thanks. i just run it through opus 5.5 high. Good stuff here.

Whatever you prefer, I can polish it up from here if you want.


Review of [elastic/eui#10210](#10210)

Verdict: nothing blocks merging. The config matches Libra's schema, which I checked by hand against elastic/libra/docs/repository-configuration.md; I couldn't run libra config lint. But a fair amount of the rules content is dead weight or contradicts the wiki.

Should fix

  1. There's no sign Libra is set up for EUI. Libra posts a Libra Config Lint status on PRs that touch .libra/. Head 54db0d9 has no such status (I checked). Either the webhook isn't installed on elastic/eui or it never ran. The PR's main QA step, "confirm Config Lint passes", can't be done. Libra also reads config from the default branch, so none of this takes effect until after merge.

  2. Many of the "rules" get filtered out before anyone sees them. Libra grades severity with its own fixed rubric; repos can't define severity. Naming, wording and doc placement are P4, and publish_min_severity: P3 drops P4. That means these will never be published:

    • the snake_case / EuiPascalCase rule
    • the PR-title rule (inline comments can't attach to a title anyway)
    • most of the docs-and-changelog specialist's checks ("past tense, start with a verb", sentence case, kebab-case)

    Either remove those rules or lower the threshold to P4.

  3. "Only public-api can block" will almost never happen. block_min_severity: P1, and Libra's P1 means data loss, widened access, or a core workflow blocked. Under that rubric a prop removal is most likely graded P2. This is my inference from the rubric.

  4. The changelog rule is wrong and duplicates CI. scripts/check_changelogs.sh already requires changelogs/upcoming/<PR>.md for every changed public package (there are 7), and supports skip-changelog-<pkg>. The instructions only cover packages/eui/src/. They also miss the wiki's exemption for test-only and story-only changes (changelogs.md), so Libra could flag a PR that only changes stories. I'd drop the "is there a changelog" rule and keep only checks on what the entry says.

  5. The Storybook-story rule isn't in the wiki. I searched the wiki and found no such requirement. It contradicts the PR's own "skip … preferences not backed by the wiki" line.

Recommendations

  • Packages other than eui aren't covered:
    • docs-and-changelog only matches packages/eui/changelogs/upcoming/**, but docusaurus-theme, test-helpers, eslint-plugin and illustrations have their own upcoming folders.
    • paths.exclude only covers eui's CHANGELOG_*.md, though the other packages have yearly changelogs too.
    • public-api leaves out @elastic/eui-test-helpers, @elastic/eslint-plugin-eui and packages/eui/package.json (peer deps and exports).
  • High contrast mode isn't mentioned in the accessibility or theming rules, although 135 files under packages/eui/src use highContrastMode (I counted). "Every shipped theme" currently means only Borealis.
  • .yarn/** also hides .yarn/patches, which are real changes to dependency code. Excluding only .yarn/releases/** and .yarn/plugins/** would keep patches reviewed.
  • Libra loads AGENTS.md automatically, after instructions.md. Two effects:
    • The pointer to it in instructions.md is redundant.
    • Its "if not a help-wanted issue, stop and point them at…" line will also steer the reviewer on community PRs. That second point is inferred.

Nits

  • packages/eui/CHANGELOG.md is a hand-written pointer file, not "generated by the release script". Excluding it does no harm.
  • The PR body says it "should carry skip-changelog". It doesn't need it: the changelog check already passed, because .libra/ isn't inside a package.

Verified fine

  • Every referenced path exists on upstream/main, and 3 *.styles.tsx files exist.
  • Both Copilot findings are fixed in 54db0d9.
  • Not reviewing drafts by default matches the wiki.
  • Excluding snapshots matches the Kibana and elastic-charts configs.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GMt5khuQyoinpozhcVBDB

clintandrewhall commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor Author

@Dosant thanks, this was a useful review. I checked each point against the repo and pushed 3e11d45.

Changed

  • Removed the rules Libra can never publish at P3: the naming rule, the PR-title rule and the prose-style checks in docs-and-changelog.
  • Dropped the "changelog exists" rule, since check_changelogs.sh already covers it. The remaining rule checks only what an entry says.
  • Dropped the Storybook-story rule, which isn't in the wiki.
  • docs-and-changelog now covers packages/*/changelogs/upcoming/**, and paths.exclude covers packages/*/changelogs/CHANGELOG_*.md.
  • public-api now covers packages/eui/package.json, test-helpers and eslint-plugin.
  • Added high contrast mode (highContrastMode) to the accessibility, styles and instructions text.
  • .yarn exclusions narrowed to releases/** and plugins/**, so .yarn/patches stays reviewed.
  • Removed the redundant AGENTS.md pointer from instructions.md.
  • Corrected the comment on the CHANGELOG.md exclusion. The new packages/*/changelogs/CHANGELOG_*.md pattern covers only the yearly changelogs.

Still open

  • Blocking threshold. block_min_severity is still P1. I agree a prop removal probably grades P2, so public-api would rarely block. Lowering it to P2 would make that specialist block on most real breakages. I'd like the team's call.
  • Libra setup. Libra isn't posting a Libra Config Lint status because I haven't installed it on elastic/eui yet. Libra reads config from the default branch, so none of this takes effect until after merge.
  • Skip-changelog note. The note in the PR description about needing the skip-changelog label is wrong. I'll leave the description as is unless you'd like it corrected.

Generated by Claude Code

@Dosant

Dosant commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

@clintandrewhall pls switch from sonnet to opus! :D

Fixes look good

  • public-api still misses packages/illustrations/src/**. @elastic/eui-illustrations is a public package too.
  • Blocking threshold: I think we should keep P1 for now. 👍
  • Libra setup: the missing Libra Config Lint status is expected for now. Per the onboarding doc, we still need elastic/catalog-info#4823 plus the generated elastic/infra PR, and then the webhook in EUI repo settings (admin only). Could you add the webhook once the token PRs land?
  • Description: yes, please fix the skip-changelog note so it doesn't confuse whoever merges this 🙏

@Dosant

Dosant commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

🤖 I ran libra config lint on b6cf309 (built from elastic/libra main): valid, no diagnostics, no policy adjustments. I also checked which specialists trigger per path and the routing looks right.

A few small things left:

  • AGENTS.md is loaded as trusted reviewer guidance, so its "external contributors … stop and point them at" line may make Libra flag community PRs. I think a line in instructions.md saying those eligibility rules are for authoring agents, not review, would cover it.
  • public-api should also require the breaking change label. The wiki makes it mandatory and it triggers the Kibana due-diligence comment.
  • @elastic/eui-docusaurus-theme isn't covered by any specialist, and packages/eui-theme-*/package.json (peer deps) only hits styles-theming, not public-api.
  • Nit: nothing enforces import order (no import/order rule in any ESLint config), so "Prettier and ESLint enforce these" is only half true.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GMt5khuQyoinpozhcVBDB

Copy link
Copy Markdown
Contributor Author

@Dosant thanks for running the lint and checking the routing. I verified each point and pushed 4c9a69b:

  • AGENTS.md: instructions.md now says its contributor-eligibility guidance is for authoring agents, and reviewers shouldn't flag a PR based on who opened it or which issue it references.
  • breaking change label: public-api now requires it alongside the documentation and changelog entry, as the wiki does.
  • Coverage: public-api now covers packages/docusaurus-theme/src/**, packages/docusaurus-preset/src/**, and the package.json files of eui-theme-common and eui-theme-borealis.
  • Import order: I confirmed nothing enforces it, so the instructions now say Prettier enforces formatting and that import order shouldn't be flagged.

Earlier, illustrations was added (b6cf309) and the description was updated. The webhook still needs an admin once the token PRs land.


Generated by Claude Code

@Dosant Dosant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤞 lgtm

@clintandrewhall
clintandrewhall enabled auto-merge (squash) October 14, 2026 18:29
@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

History

@clintandrewhall

Copy link
Copy Markdown
Contributor Author

Webhook created. Once this merges, we'll be set up. 🎉

@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

@clintandrewhall
clintandrewhall merged commit 8dc3b94 into main Oct 14, 2026
7 checks passed
@clintandrewhall
clintandrewhall deleted the claude/epic-cannon-y8sh4w branch October 14, 2026 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants