From cdce1489b714fb0f44800b808942b308ee3eed2b Mon Sep 17 00:00:00 2001 From: Milton Hultgren Date: Fri, 29 May 2026 23:18:40 +0200 Subject: [PATCH] Add skill to split larger Kibana change to multiple PRs --- .agents/skills/kibana-split-to-prs/SKILL.md | 69 +++++++++++++ .../references/execution.md | 57 +++++++++++ .../references/plan-template.md | 82 ++++++++++++++++ .../kibana-split-to-prs/references/review.md | 74 ++++++++++++++ .../kibana-split-to-prs/references/slicing.md | 97 +++++++++++++++++++ 5 files changed, 379 insertions(+) create mode 100644 .agents/skills/kibana-split-to-prs/SKILL.md create mode 100644 .agents/skills/kibana-split-to-prs/references/execution.md create mode 100644 .agents/skills/kibana-split-to-prs/references/plan-template.md create mode 100644 .agents/skills/kibana-split-to-prs/references/review.md create mode 100644 .agents/skills/kibana-split-to-prs/references/slicing.md diff --git a/.agents/skills/kibana-split-to-prs/SKILL.md b/.agents/skills/kibana-split-to-prs/SKILL.md new file mode 100644 index 0000000000000..2e575097728b2 --- /dev/null +++ b/.agents/skills/kibana-split-to-prs/SKILL.md @@ -0,0 +1,69 @@ +--- +name: kibana-split-to-prs +description: >- + Analyzes local Kibana git changes and proposes (or executes after approval) a + stacked chain of small, focused PRs. Each PR is a review product: one question, + one owner, planned validation, no hidden risk. CODEOWNERS-aligned, risk-tagged + hints, low-risk vs needs-scrutiny buckets, splittability edits. Diffs against + upstream/main (fork workflow). Use when splitting a branch, dirty working tree, + or large diff for faster time-to-merge. +disable-model-invocation: true +--- + +# Kibana split to PRs + +**North star:** time-to-merged. Each PR = **one review question**, reviewable in one focused pass ([references/review.md](references/review.md)). + +Turn local work into a **single stacked chain** (`upstream/main` → PR1 → PR2 → …). Deliverable is a **split plan**; git execution optional after approval. Design already reviewed. + +**Not git-only:** when hunks or compile order block clean slices, **edit the code** (shims, dark-ship, extractions)—see [references/slicing.md](references/slicing.md) §splittability. Plan in the split plan; apply only after **execute split**. + +Also read [AGENTS.md](../../../AGENTS.md) for module boundaries, plugin `server/index.ts`, testing, and mergeability guardrails. + +## Hard rules + +- **No git mutations** until the user approves the split plan or **execute split**. +- **No draft PRs** without explicit ask ([references/execution.md](references/execution.md)). +- **Never discard work.** No destructive git without explicit approval. +- **Recoverable snapshot** before Phase 5 (see table). +- **Stage only named files or hunks.** No `git add .` / `git add -A`. + +## Workflow + +```mermaid +flowchart TD + analyze[1_Analyze] + slice[2_Slice_and_audit] + plan[3_Write_plan] + gate[4_Review_gate] + execute[5_Execute] + report[6_Report] + + analyze --> slice --> plan --> gate + gate -->|execute_split| execute --> report + gate -->|revisions| slice + gate -->|plan_only| report +``` + +Phases 1–3 are **git read-only** (may propose splittability edits; apply in Phase 5). Plans record validation as `not run — slice not materialized`; after **execute split**, each slice must pass `node scripts/check` before the next ([references/execution.md](references/execution.md)). + +| Phase | Read | Do | +|-------|------|-----| +| **1 Analyze** | [slicing.md](references/slicing.md) §when-not, §hotspots | **`upstream`** = diff baseline, **`origin`** = push only—not `origin/main` unless user asks ([fork docs](../../../docs/extend/development-github.md)). `git remote -v`; if no `upstream`, stop. `git fetch upstream` && `git rev-parse --verify upstream/main`. Diff: `git diff upstream/main...HEAD`, `git diff`, `git diff --cached`, `git log upstream/main..HEAD --oneline`, `git status`. Chat history for intent. §when-not: if one PR suffices, say so. Per changed path: walk to `kibana.jsonc` (`@kbn/...`, owner, group, visibility). Record hotspots. | +| **2 Slice + audit** | [slicing.md](references/slicing.md), [review.md](references/review.md) | Slice per slicing order (CODEOWNERS → deps → mechanical/behavioral → BBA/dark-ship → noise). One chain only—no parallel PRs off `main`. Splittability edits when `git add -p` won't compile or yield one question. **Audit:** each PR passes [review.md](references/review.md) and has an answer for every Phase 4 row below. Revise or `NEEDS DECISION`. Flag mergeability per [AGENTS.md](../../../AGENTS.md) (SO: one model version/type/PR; privileges: deprecation skill ordering). | +| **3 Plan** | [plan-template.md](references/plan-template.md) | Write `split-plan-.md` (repo root or primary plugin dir—state path). Follow schema; **self-check:** every PR has all 7 fields. | +| **4 Gate** | — | **Stop.** No branches, commits, push, PRs. Present plan (see below). Ask: **Plan only** \| **Approve and execute split**. Do not bundle draft PRs. | +| **5 Execute** | [execution.md](references/execution.md) | Backup: `SHA=$(git stash create "pre-split"); [ -n "$SHA" ] && git update-ref "refs/backup/pre-split-$(date +%s)" "$SHA"`. `git fetch upstream`. Per PR in order: splittability edits → branch from `upstream/main` or prior split branch → stage planned hunks only → commit → **`node scripts/check --scope branch --base-ref upstream/main`**—stop on fail → record branch + SHA + outcome. No push/`gh` unless asked (`origin` only). Leave original branch recoverable. | +| **6 Report** | [execution.md](references/execution.md) | Plan path; branch names + SHAs; check outcomes; leftover hunks; suggest only: draft PRs on `origin`, or fix failed check before continuing. | + +## Phase 4 — Review gate (mandatory) + +Surface in chat — **lead with review questions**, not file lists: + +1. Feature context + stack diagram + PR count (or single-PR recommendation) +2. **PR # → review question → bucket → attention** +3. Merge order (foundations vs behavioral—not approval urgency) +4. Hotspots + cross-PR hunk maps +5. Highest-attention PRs: one hint each +6. Splittability edits not in current diff +7. `NEEDS DECISION` items diff --git a/.agents/skills/kibana-split-to-prs/references/execution.md b/.agents/skills/kibana-split-to-prs/references/execution.md new file mode 100644 index 0000000000000..bba0aa1d750db --- /dev/null +++ b/.agents/skills/kibana-split-to-prs/references/execution.md @@ -0,0 +1,57 @@ +# Execution and validation + +## Validation timing + +| Phase | Run check on slice? | Plan outcome | +|-------|---------------------|--------------| +| 1–3 | **No** | `not run — slice not materialized; run in Phase 5 after commit` | +| 5 | **Yes** — after commit | `pass` or `fail` (stop stack on fail) | + +Do not claim checks passed during planning. + +## Check gate (Phase 5) + +After each slice commit: + +```bash +node scripts/check --scope branch --base-ref upstream/main +``` + +Pass → proceed. Fail → stop; fix slice; re-run. Baseline: `upstream/main`. + +Supplement commands: [AGENTS.md](../../../AGENTS.md) Testing section (`check_changes`, `jest`, `type_check`, `eslint`). Prefer `scripts/check` for the gate—do not substitute "CI will catch it" without naming deferred checks. + +| PR type | Validation | +|---------|------------| +| Mechanical, unchanged | `scripts/check` | +| Behavioral | `scripts/check` + tests in PR | +| Dark-shipped (off) | `scripts/check` + tests for new path | +| Flip | `scripts/check`; often small diff | + +--- + +## Approval gates + +| Action | Requires | +|--------|----------| +| Split plan | — | +| Branches + commits | **execute split** | +| `git push` to `origin` | Explicit ask (never `upstream`) | +| Draft PRs | Explicit ask **after** slices exist | + +Do not open draft PRs as a side effect of execute split. + +## Stack bases + +PR1 base `upstream/main`; PRn base branch for PR(n−1). After PR1 merges, retarget PR2 to `upstream/main` or rebase—note in plan if merging incrementally. + +## Opening draft PRs + +Push to **`origin`**, then: + +```bash +gh pr create --draft --base --head --title "..." --body-file - +# PR2+: --base previous split branch until PR1 merges +``` + +Use review question + stack position in body. Draft until check passed on branch. diff --git a/.agents/skills/kibana-split-to-prs/references/plan-template.md b/.agents/skills/kibana-split-to-prs/references/plan-template.md new file mode 100644 index 0000000000000..5bf8351d89bbc --- /dev/null +++ b/.agents/skills/kibana-split-to-prs/references/plan-template.md @@ -0,0 +1,82 @@ +# Split plan schema + +Write `split-plan-.md` following this schema. Definitions: [review.md](review.md). Execution: [execution.md](execution.md). + +**Phase 3 self-check:** every PR block has all 7 fields below. + +## Stack-level sections + +1. **Header table** — baseline (`upstream/main`), branch, date, plan path, single chain note +2. **Feature context** — 1–2 sentences; design reviewed +3. **Hotspots** — omit if none; file, issue, resolution +4. **Ownership map** — team, files summary, PR #, primary reviewer +5. **Summary table** — PR #, title, review question, bucket, risk tags, attention, file count, merge priority, depends on +6. **Stack mermaid** + suggested merge order (foundations vs behavioral) +7. **Per-PR blocks** — repeat for each PR +8. **Leftover / deferred** — omit if empty +9. **NEEDS DECISION** — omit if empty +10. **Splittability edits summary** — stack-level code not in current diff; omit if none + +Bucket, attention, merge priority, depends-on live in **Summary** only—not duplicated per PR. + +## Per-PR block (7 fields) + +Each PR: `## PR n — ` then: + +| # | Section | Content | +|---|---------|---------| +| 1 | **Review question** | One sentence | +| 2 | **Out of scope** | What later PRs add | +| 3 | **Surprise audit** | `None identified` or bullets + hints | +| 4 | **Risk tags + hints** | Tags; 1–2 imperative hints | +| 5 | **Files** | Paths; cross-PR hunk map inline if needed | +| 6 | **Mergeability + splittability edits** | Why safe alone; flag/dark-ship state; `none` or edits in this PR; PR # removing scaffolding | +| 7 | **Validation** | Command + `Outcome: not run — slice not materialized; run in Phase 5 after commit` | + +Per-PR metadata (base, team, modules, change class) — one line under the title if needed: + +`Base: upstream/main · Team: @elastic/... · Modules: @kbn/... · Class: mechanical|behavioral|mixed` + +After Phase 5, append execution log (branch, SHA, outcome pass|fail) under Validation. + +--- + +## Example PR block (cross-PR hunk map) + +`## PR 3 — Dark-ship new backoff behind useNewRetry` + +Base: PR2 branch · Team: @elastic/kibana-management · Modules: @kbn/task-manager · Class: behavioral + +### Review question + +Is the new backoff correct when enabled, and fully inactive when `useNewRetry` is false? + +### Out of scope + +Enabling flag (PR4); type extraction (PR1). + +### Surprise audit + +`task_runner.ts` also in PR2 (wiring) and PR4 (flip)—hunk map in Files. + +### Risk tags + hints + +`concurrency`, `tests` + +- Trace `runTask()`—`useNewRetry` branch unreachable at default. +- Unit tests cover both policies without timing flakes. + +### Files + +- `packages/.../retry_policy.ts` — new backoff impl +- `packages/.../task_runner.ts` — **PR3 only:** `useNewRetry` branch (default false) +- Hunk map: `task_runner.ts` — PR2: wiring; PR3: `useNewRetry`; PR4: remove legacy + +### Mergeability + splittability edits + +Dark-ship off at default; production path unchanged. Add `const useNewRetry = false` and `NewRetryPolicy`. Scaffolding removed in PR4. + +### Validation + +- Command: `node scripts/check --scope branch --base-ref upstream/main` +- Outcome: not run — slice not materialized; run in Phase 5 after commit diff --git a/.agents/skills/kibana-split-to-prs/references/review.md b/.agents/skills/kibana-split-to-prs/references/review.md new file mode 100644 index 0000000000000..591c20e35a1f1 --- /dev/null +++ b/.agents/skills/kibana-split-to-prs/references/review.md @@ -0,0 +1,74 @@ +# Review product + +A PR is a **review product**, not a git hunk. Canonical vocabulary: + +| Term | Meaning | +|------|---------| +| **One question** | Single sentence the reviewer decides—not a file list; split if "and" joins unrelated concerns | +| **One owner** | Primary `@elastic/team`; secondary minimal and named | +| **Planned validation** | `node scripts/check --scope branch --base-ref upstream/main`; outcome only after Phase 5 commit | +| **No hidden risk** | Surprise audit empty or every surprise named + hinted | +| **Bucket** | `low risk` \| `needs scrutiny`—review surface, not approval urgency | + +Apply to each PR after slicing, before Phase 3. Fail → reslice or `NEEDS DECISION`. + +**Pass when:** scannable (title + question + tags tell the story); standalone readable (no later PR required); single-pass review (assessable without rest of stack). + +## Buckets + +| Bucket | Signals | +|--------|---------| +| **Low risk** | Attention `low`, ≤2 tags, mechanical or dark-ship off | +| **Needs scrutiny** | Attention `medium`/`high`, `privileges`, `saved-objects`, `http-api`, `concurrency`, flip PR | + +Do not hide needs scrutiny inside low risk. + +## Hidden risk + +*What would make a reviewer say "I didn't expect that"?* + +| Risk | Fix | +|------|-----| +| Trojan mechanical | Split behavioral hunk | +| Silent default | Own PR or `config` tag + hint | +| Type-only runtime change | `public-api` + call-site hint | +| Cross-stack file | Reslice, extract, hunk map ([slicing.md](slicing.md) §hotspots) | +| Stack-only readable | **Out of scope** + standalone narrative | +| Deferred validation | Name check or run on branch | +| Owner mismatch | Split or document coupling | +| Drive-by fixes / unrelated tags in one PR | Remove or split | +| Cross-PR hunks without map | Reslice or map | + +Record in **Surprise audit** (`None identified` if empty). + +## Stack self-check + +Titles + questions predict the arc? ≤5 PRs or documented why? Same file 3+ re-reads → extract or exception? No silent leftover hunks? + +## Risk tags + +Tag every applicable area. Prefer 1–3; more → split further. + +`public-api`, `http-api`, `saved-objects` (one model version per type per PR), `privileges`, `auth`, `concurrency`, `state-machine`, `config`, `ui`, `a11y`, `i18n`, `plugin-entry`, `dependencies`, `tests`, `generated`, `noise` + +## Hints + +Imperative, short; files/symbols; tie to review question. No ignore lists or diff prose. + +| Signal | Attention | +|--------|-----------| +| ~80 lines privilege mapping | high | +| ~800 lines moves | low (`noise`) | +| HTTP route + authz | high | +| `useNew = false` + new impl | medium | +| Flip PR | low–medium | + +## PR blurb (generate when opening PRs, not required in plan) + +``` +<One-line summary> +Review question: <question> +Stack: n/N +``` + +Optional: `Focus: <hint>`. If check passed: may note `node scripts/check` passed—verification only, not skip review. diff --git a/.agents/skills/kibana-split-to-prs/references/slicing.md b/.agents/skills/kibana-split-to-prs/references/slicing.md new file mode 100644 index 0000000000000..3589c126f4d5e --- /dev/null +++ b/.agents/skills/kibana-split-to-prs/references/slicing.md @@ -0,0 +1,97 @@ +# Slicing + +North star is **time-to-merged**, not maximum PR count. Splitting costs stack overhead, splittability scaffolding, context-switching, GitHub mechanics. + +## When not to split + +**One PR when:** one [review question](review.md), one owner, low surprise; small scope (one team, few files, no SO/privilege/http-api risk); heavy splittability edits for little gain; time-sensitive work where stack delays first reviewable unit. + +**Stack when:** separable CODEOWNERS teams; risk isolation (dark-ship, SO model version, privilege deprecation, HTTP+authz); mechanical noise would poison behavioral review; one file mixes mechanical + behavioral and extraction wins. + +### Decision gate + +1. Would one coherent PR stall review more than a stack stalls shipping the first slice? +2. Does each extra PR remove a **distinct review question** or just chop history? +3. Will splittability edits add more total review than they save? + +If (2) and (3) favor splitting → stack. Else → **single PR**. + +**Depth:** target ≤5 PRs; merge coherent slices or document why not. + +**Stop splitting** when compile breaks without reasonable shims, hunks won't separate, or re-read tax exceeds benefit—propose shims (§splittability) before one big PR. + +--- + +## Hotspots + +Find in Phase 1 before boundaries. + +| Signal | Action | +|--------|--------| +| Mechanical + behavioral in one file | Extract or dark-ship | +| Same file in 2+ PRs | Avoid—see cross-PR | +| Two review questions in one file | Extract or `const useNew = false` | +| Mixed team ownership | Split by file, or one PR + coupling note | +| God file | One-time extraction PR | + +**Cross-PR default:** each file in at most one PR. Prefer extract, dedicated move/rename PR, or splittability edit. + +**Acceptable** (Surprise audit + hunk map): flag/dark-ship sequence; poor prior factoring (`NEEDS DECISION`); generated + hand edit (`generated` tag). + +```markdown +- `path/task_runner.ts` — PR2: wiring; PR3: `useNewRetry` branch (default false) +``` + +Anti-patterns (trojan mechanical, hidden defaults, stack-only readable, etc.): [review.md](review.md) §hidden-risk. + +--- + +## Slice order + +0. CODEOWNERS / team boundaries +1. Module / dependency order (package API before consumer; shims if needed) +2. Mechanical vs behavioral (not line-count driven) +3. Unrelated concepts (feature/API/privilege, not directory) +4. BBA + dark-ship: types → wire legacy → dark-ship new (`const useNew = false`) → flip → remove legacy +5. Noise early: lockfile, generated mass updates, i18n JSON, Scout scaffold, bulk `kibana.jsonc` +6. See [review.md](review.md) §hidden-risk for anti-patterns + +**Exception:** prettier/eslint from the behavioral edit in the same files stays in that PR. + +**Titles:** one [review question](review.md)—not "Part 2" or file lists. Split if the title needs "and" for unrelated concerns. + +### CODEOWNERS + +1. [`.github/CODEOWNERS`](../../../.github/CODEOWNERS) · nested wins · `kibana.jsonc` `owner` · `node src/dev/stage_by_owner.ts` + +Rules: one PR per team when separable; same team + related feature → one slice; no owner → `NEEDS DECISION`. + +### Stack + +`upstream/main` → PR1 → PR2 → … only. Build: types/shims/APIs first. Merge: foundations before behavioral flips. + +### Mergeability + +Flag per [AGENTS.md](../../../AGENTS.md): plugin `server/index.ts` lazy import; Saved Objects one model version per type per PR ([validate](../../../docs/extend/saved-objects/validate.md)); privilege deprecations ([skill](../../kibana-privilege-deprecation/SKILL.md)); tests with behavior. + +--- + +## Splittability edits + +Not pure git surgery—**edit code** when hunks, compile order, or review boundaries block a clean stack. Scaffolding for review only; design approved. + +| Signal | Edit | +|--------|------| +| Consumer needs package exports | Temporary re-export/shim | +| Old + new impl in one file | BBA + dark-ship | +| Two questions in one file | Extract or `const useNew = false` | +| SO / privilege separate landing | Split schema from behavior | +| Mechanical + behavioral same hunk | Separate files or gated branches | +| PR N needs PR N+1 symbols | Stub, deprecated wrapper, `import type` | + +**In scope:** dark-ship toggles, temp re-exports/no-ops, extracted files, lazy plugin entry, test doubles, hunk moves. +**Out of scope:** unrelated fixes, permanent API you wouldn't ship as one PR, early behavior enablement. + +**Workflow:** list in plan (per-PR under Mergeability + splittability); call out at Phase 4 gate; apply in Phase 5 before staging. Plan only → no code edits. + +Record: `none` or edits **in this PR**; note PR # that removes scaffolding.