docs(skill): improve backport-management with tiered triage, path pre-filter, and public-API conflict review#11868
Conversation
…ict review to backport-management Synthesizes lessons from the v1.43.16 backport session into the existing skill. ## What's new ### SKILL.md - Quick Start expanded from 7 to 12 steps, surfacing the pre-filter and target-file-existence check that should run BEFORE per-PR triage - New gotcha: 'Cherry-Picked Tests Can Reference Files Added By Earlier Unbackported PRs' — drop the test, document why - New gotcha: 'Backport-Only Compatibility Shims' — when a refactor-style fix's mechanism relies on changes that aren't on the older branch, a literal cherry-pick can recreate the bug for extensions still using the old contract. Real example from #11541 + LGraphNode.vue handleDrop - New 'Path Pre-Filter' table under Auto-Skip Categories — auto-skip PRs touching only apps/website/, browser_tests/, .github/, design-system, generated types, .claude/, docs/, *.stories.ts. Removes 30-50% of candidates without reading their bodies ### reference/analysis.md - New 'Verify Target File Existence' subsection — git cat-file -e before cherry-pick to skip PRs targeting features the branch doesn't ship, faster than letting cherry-pick fail with modify/delete - New 'Tiered Triage' section — Tier 1 (core editor must-haves), Tier 2 (cloud-distribution only), Tier 3 (skip) before per-PR Y/N. Surfaces release-engineering decisions a flat MUST/SHOULD list obscures ### reference/discovery.md - Reconciliation workflow combining Slack bot list + git gap, subtracting already-backported PRs (extracted via 'Backport of #' grep on the branch) - Clarifies that no single source is authoritative ### reference/execution.md - New 'Step 0: Test-Then-Resolve Pre-Pass' — moved the dry-run loop to the top of the workflow so you classify clean vs conflict before triggering automation - New 'Public-API conflict review' guard inline in the manual cherry-pick loop — consult oracle BEFORE pushing if resolution touches LiteGraph callbacks, node.* methods, or extension-API surfaces - Per-PR validation block (typecheck + targeted unit tests + ESLint + oxfmt) before push, in addition to wave verification - Three new lessons learned (19-21) covering the above - New 'PR Body Template' for manual cherry-picks — non-negotiable conflict-resolution section so reviewers don't have to re-derive resolution logic from the diff six months later ## Why Lessons from the v1.43.16 backport session (PRs #11856-#11862): 1. Path pre-filtering caught 35 of 61 candidates as 'skip' before any per-PR analysis was needed (apps/website/, CI, tests, design-system). The previous flat MUST/SHOULD/SKIP rubric meant reading every PR. 2. Oracle review on PR #11856 (#11541 backport) caught a regression in LGraphNode.vue's handleDrop where the upstream PR's removal of the legacy 'handled === true' sync-return path would silently break custom nodes still using the old onDragDrop contract — recreating the very duplicate-node bug the PR was fixing. The skill had no guidance for this class of issue. 3. Two separate backports (#11180, #11541) hit the same modify/delete conflict pattern: a test file added on main by an earlier unbackported PR. The skill's Conflict Triage table covered modify/delete generally but didn't surface this specific anti-pattern of smuggling in test scaffolding without its prerequisites. 4. The skill assumed Slack bot + git gap as inputs but didn't show how to reconcile them with already-backported PRs, leading to potential double-cherry-picking. No code or workflow changes — skill documentation only.
📝 WalkthroughWalkthroughThis PR expands the backport-management playbook: adds path pre-filtering, a target-file-existence pre-check with auto-skip, a dry-run "test-then-resolve" cherry-pick classification, tiered triage, public-API conflict review guidance (LiteGraph/extension/node.*), richer discovery reconciliation, and standardized PR body templates and logging. ChangesBackport-Management Playbook Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/backport-management/reference/discovery.md (1)
3-83:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUpstream-mirrored skill content appears locally diverged
These edits modify content inside
.claude/skills/, which is documented as upstream-mirrored content. Please confirm this was first updated upstream (or explicitly approved as a local exception); otherwise this should be moved to repo-local docs/metadata instead of changing the mirrored skill body.Based on learnings: Treat all files under
.claude/skills/as verbatim upstream content sourced from CloudAI-X/threejs-skills, preserving content unless upstream updates are requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/backport-management/reference/discovery.md around lines 3 - 83, The PR edits upstream-mirrored skill content (the "Run all sources, then reconcile." section in discovery.md), which must either be updated upstream or treated as a local exception; do not leave divergent changes under mirrored content. Revert or remove the changes in discovery.md that live under the upstream-mirrored skill (the "Run all sources, then reconcile." section), then either (a) open/track an upstream update in the source repo to apply these edits there, or (b) move your local edits into a repo-local doc (e.g., docs/ or metadata files) outside the mirrored skill area and reference them from the mirrored file if needed; ensure the mirrored file remains identical to upstream unless an explicit local-exception approval is recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/backport-management/reference/analysis.md:
- Around line 104-118: The fenced code block that begins with ``` and contains
the "Tier 1 (N PRs) — strong backport candidates" list is missing a language
identifier; update the opening fence to include a language (e.g., change ``` to
```text or ```md) so markdownlint MD040 stops flagging it and the block renders
correctly.
In @.claude/skills/backport-management/reference/execution.md:
- Around line 140-144: The per-PR scripted commands (the pnpm test:unit -- run
$(git diff --name-only HEAD~1 | grep -E "\.test\.ts$"), pnpm exec eslint $(git
diff --name-only HEAD~1 | grep -E "\.(ts|vue)$"), and pnpm exec oxfmt --check
$(git diff --name-only HEAD~1 | grep -E "\.(ts|vue)$") invocations) must be
guarded against empty globs: capture each git diff result into a variable (e.g.,
changed_tests, changed_ts_vue), check if the variable is non-empty before
running the corresponding pnpm command, and otherwise skip with a clear message
so the commands don't run with empty args or unintended scopes.
In @.claude/skills/backport-management/SKILL.md:
- Around line 185-196: The table row for the "apps/desktop-ui/" entry is
malformed (it currently has only two pipe-separated columns), which violates the
"Path prefix | Bucket | Reason" structure and triggers MD056; edit the
.claude/skills/backport-management/SKILL.md table so every row has three columns
by fixing the "apps/desktop-ui/" row to include the missing third column
(Reason) text (e.g., "Desktop app, separate release cadence") and ensure proper
pipe separators around the `core/*` code span so the row reads: |
apps/desktop-ui/ | SKIP for `core/*` | Desktop app, separate release cadence |.
---
Outside diff comments:
In @.claude/skills/backport-management/reference/discovery.md:
- Around line 3-83: The PR edits upstream-mirrored skill content (the "Run all
sources, then reconcile." section in discovery.md), which must either be updated
upstream or treated as a local exception; do not leave divergent changes under
mirrored content. Revert or remove the changes in discovery.md that live under
the upstream-mirrored skill (the "Run all sources, then reconcile." section),
then either (a) open/track an upstream update in the source repo to apply these
edits there, or (b) move your local edits into a repo-local doc (e.g., docs/ or
metadata files) outside the mirrored skill area and reference them from the
mirrored file if needed; ensure the mirrored file remains identical to upstream
unless an explicit local-exception approval is recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8418278-cd41-465b-9fe7-0debc367683e
📒 Files selected for processing (4)
.claude/skills/backport-management/SKILL.md.claude/skills/backport-management/reference/analysis.md.claude/skills/backport-management/reference/discovery.md.claude/skills/backport-management/reference/execution.md
- SKILL.md: complete the malformed final row of the Path Pre-Filter table
('src/' (core editor)) so it has 3 columns matching the header (MD056)
- reference/analysis.md: add 'text' language identifier to the Tier-results
presentation example fence (MD040)
- reference/execution.md: guard the per-PR validation block against empty
file matches. Without this, 'pnpm test:unit -- run' with no args runs the
full suite, and 'pnpm exec eslint' with no args errors. Use mapfile +
array length check, with explicit skip messages
Did NOT address the upstream-mirror outside-diff comment — verified that
.claude/skills/ in this repo is repo-authored content (PRs #9619, #10164,
#10927 in the Comfy-Org/ComfyUI_frontend history), not a mirror of any
upstream skills repository. The reviewer's 'learning' about CloudAI-X /
threejs-skills doesn't apply here.
glary-bot
Bot
force-pushed
the
glary/skill-backport-management-improvements
branch
from
cceb79a to
62d79e7
Compare
|
@christian-byrne — addressed all 4 review comments in On 3177812247 (blocker — missing helpers): You're right. Verified Picked option 2 (trim to the actual #11779 fix and inline minimal helpers):
Production code ( Sandbox doesn't have a display server for On 3177812249 (scope creep): Agreed, dropped both #9714/#10620 tests. PR description updated. On 3177812250 (test name doesn't exercise setGraph path): Good point — kept your existing test name since the trimmed version is still the document-keydown path, and the unit test in On 3177812252 (loose >20px assertion): N/A now — that test was dropped (originated from #9714, not #11779). On 3177812254 ( On 3177812256 (afterEach ordering in |
|
@christian-byrne — replies (inline-reply API returned 404 on the parent comment IDs, so consolidating): On 3177815230 ( // Awaited so async onDragDrop rejections propagate via Vue's errorHandler
await resultGood catch — this exact "looks redundant, delete it" trap is the kind of thing that would absolutely get refactored away in 6 months without a comment. On 3177815232 (sync→async timing window for thrown exceptions): Acknowledged. The shift from On 3177815234 (no direct test for the legacy |
|
@christian-byrne — thanks for the review and approval. On 3177815742 (wrap stale invocation in On 3177815745 (other autogrow-map destructure sites at lines 371/384/433 don't have the guard): Agreed, those are sync paths where the group is guaranteed present, not part of the rAF race fixed in #11180. Out of scope for this backport. I'll leave that follow-up to you since you have the full context on which sites are sync-safe vs need the guard — let me know if you want me to file an issue instead. |
|
@christian-byrne — thanks for the review and approval. On 3177815533 (cross-reference comment + imprecise wording): Both fair — the Leaving the cherry-pick byte-faithful to upstream |
|
Updated state of this PR after addressing review: The Trimmed the spec in
Production code ( Net diff vs upstream: spec is +12/-123 lines smaller. The trimmed test is functionally equivalent to upstream's #11779 case (loads basic-subgraph, enters subgraph, opens v2 search box, picks a node to enter ghost mode, presses Escape, asserts ghost is cancelled and we're still in the subgraph). |
dosubot
Bot
added
the
size:L
4 participants
PR Created by the Glary-Bot Agent
Synthesizes lessons from the recent
v1.43.16backport session (PRs #11856–#11862) into the.claude/skills/backport-management/skill.Documentation only — no code, no workflow, no automation changes.
What's new
SKILL.mdLGraphNode.vuehandleDropapps/website/,browser_tests/,.github/,packages/design-system/, generated types,.claude/,docs/, or*.stories.ts. Removes 30–50% of candidates without reading their bodiesreference/analysis.mdgit cat-file -ebefore cherry-pick to skip PRs targeting features the branch doesn't ship, faster than letting cherry-pick fail with modify/deletereference/discovery.mdBackport of #grep on the branch)reference/execution.mdnode.*methods, or extension-API surfacesWhy
Path pre-filtering caught 35 of 61 candidates as
skipbefore any per-PR analysis was needed during thev1.43.16session (apps/website/, CI, tests, design-system). The previous flat MUST/SHOULD/SKIP rubric meant reading every PR.Oracle review on PR [backport core/1.43] fix: stop duplicate node creation when dropping image on Vue nodes (#11541) #11856 (fix: stop duplicate node creation when dropping image on Vue nodes #11541 backport) caught a regression in
LGraphNode.vue:823where the upstream PR's removal of the legacyhandled === truesync-return path would silently break custom nodes still using the oldonDragDropcontract — recreating the very duplicate-node bug the PR was fixing. The skill had no guidance for this class of issue.Two separate backports (fix cloud frontend runtime guard regressions #11180, fix: stop duplicate node creation when dropping image on Vue nodes #11541) hit the same modify/delete conflict pattern: a test file added on
mainby an earlier unbackported PR. The skill's Conflict Triage table covered modify/delete generally but didn't surface this specific anti-pattern of smuggling in test scaffolding without its prerequisites.Discovery sources — the skill assumed Slack bot + git gap as inputs but didn't show how to reconcile them with already-backported PRs, leading to potential double-cherry-picking.
Verification
git diff --checkcleanreference/*.mdfiles validated by line-count checkOut of scope (intentionally not changed)
pr-backport.yamlGitHub Action — the skill describes this but doesn't own it[backport TARGET] ...— kept as-is (CodeRabbit's auto-skip filter relies on it)┆Issue is synchronized with this Notion page by Unito