Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 58 additions & 7 deletions .claude/skills/backport-management/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@ Cherry-pick backport management for Comfy-Org/ComfyUI_frontend stable release br

## Quick Start

1. **Discover** — Collect candidates from Slack bot + git log gap (`reference/discovery.md`)
2. **Analyze** — Categorize MUST/SHOULD/SKIP, check deps (`reference/analysis.md`)
3. **Human Review** — Present candidates in batches for interactive approval (see Interactive Approval Flow)
4. **Plan** — Order by dependency (leaf fixes first), group into waves per branch
5. **Execute** — Label-driven automation → worktree fallback for conflicts (`reference/execution.md`)
6. **Verify** — After each wave, verify branch integrity before proceeding
7. **Log & Report** — Generate session report (`reference/logging.md`)
1. **Discover** — Collect candidates from Slack bot + git log gap, then **reconcile both lists** (`reference/discovery.md`)
2. **Pre-filter by path** — Auto-skip PRs whose changed files are entirely under `apps/website/`, `browser_tests/`, `.github/`, `packages/design-system/`, `packages/{cloud,registry}-types/`, `.claude/`, `docs/`. Don't read PR bodies for these — they don't ship to core ComfyUI users (`reference/analysis.md`)
3. **Verify target file existence** — For each surviving candidate, run `git cat-file -e origin/$TARGET:$path` for primary changed files. If they don't exist on the target, auto-mark SKIP with reason `feature-not-on-branch`
4. **Tiered triage** — Bucket into **Tier 1 (core editor must-haves)**, **Tier 2 (cloud-distribution only)**, **Tier 3 (skip)** before reviewing individually (`reference/analysis.md`)
5. **Analyze** — Categorize remaining MUST/SHOULD, check deps (`reference/analysis.md`)
6. **Human Review** — Present candidates in batches for interactive approval, with tier context attached (see Interactive Approval Flow)
7. **Plan** — Order by dependency (leaf fixes first), group into waves per branch
8. **Test-then-resolve dry-run** — Classify clean vs conflict before committing time (`reference/execution.md`)
9. **Execute** — Label-driven automation for clean PRs → worktree fallback for conflicts (`reference/execution.md`)
10. **Public-API conflict review** — If conflict resolution touches a public LiteGraph callback, extension API, or `node.*` method, consult oracle for compat-regression review BEFORE pushing (`reference/execution.md`)
11. **Verify** — Per-PR validation (typecheck + targeted tests + lint on changed files) AND per-wave verification (full typecheck + test:unit on branch HEAD)
12. **Log & Report** — Generate session report + author accountability report + Slack status update (`reference/logging.md`)

## System Context

Expand Down Expand Up @@ -107,6 +112,32 @@ Husky hooks fail in worktrees (can't find lint-staged config). Always use `git p

In the 2026-04-06 session: core/1.42 got 18/26 auto-PRs, cloud/1.42 got only 1/25. The cloud branch has more divergence. **Always plan for manual fallback** — don't assume automation will handle most PRs.

### Cherry-Picked Tests Can Reference Files Added By Earlier Unbackported PRs

A common conflict: PR A on main modifies a test file that was *added* on main by an earlier PR B (not backported to the target). The cherry-pick of A reports "modify/delete" on B's test file because the file doesn't exist on the target. Adding the new file would smuggle in B's test scaffolding without B's runtime changes.

**Detection:** Conflict says `deleted in HEAD and modified in <PR>`. Verify with:
```bash
git log --diff-filter=A --oneline origin/main -- path/to/test.ts
```
If the introducing commit is **not** on the target branch, the test file isn't a real prerequisite for the runtime fix.

**Fix:** `git rm` the test file (drop it from the backport). Document in the commit body which PR introduced it on main and why dropping it is safe. The runtime fix itself usually doesn't depend on these tests — coverage exists at the integration layer.

### Backport-Only Compatibility Shims

When a PR's *mechanism* relies on changes upstream that aren't on the older branch, a literal cherry-pick can recreate the original bug for any consumer still using the old contract. This is most dangerous for **public LiteGraph callbacks, extension APIs, and `node.*` methods** that custom-node packages depend on.

**Real example (#11541, core/1.43 backport):** The PR removed `LGraphNode.vue`'s legacy `handled === true` sync-return check from `handleDrop`, replacing it with `await node.onDragDrop(event, true)`. Safe on `main` because all in-repo `onDragDrop` handlers had migrated to participate in the new `claimEvent` flag. On `core/1.43`, `onDragDrop` is a public callback — custom-node packages with synchronous `onDragDrop` returning `true` would no longer have their event claimed, recreating the duplicate-node-creation bug the PR was fixing.

**Detection:** The PR's diff modifies a file that is part of a public extension API surface. Look for:
- `node.onXxx` callback assignments
- Methods on `LGraphNode`, `LGraphCanvas`, `LGraph`, `Subgraph`
- Public exports from `src/lib/litegraph/`
- Type changes affecting `litegraph-augmentation.d.ts`

**Fix:** Add a backport-only compatibility shim that preserves the old contract while keeping the new fix. Document it explicitly in the commit body under a `## Backport-only compatibility fix` heading. Consult oracle for review before pushing — a bad shim is worse than no fix.

## Conflict Triage

**Always categorize before deciding to skip. High conflict count ≠ hard conflicts.**
Expand Down Expand Up @@ -147,6 +178,26 @@ Skip these without discussion:
- **Features not on target branch** — e.g., Painter, GLSLShader, appModeStore on core/1.40
- **Cloud-only PRs on core/\* branches** — Team workspaces, cloud queue, cloud-only login. (Note: app mode and Firebase auth are NOT cloud-only — see Branch Scope Rules)

### Path Pre-Filter (run BEFORE reading PR bodies)

For 50+ candidate PRs, classify by changed paths first to skip the unproductive ones without spending time on triage. Run `git show --stat $SHA` (or `gh pr view --json files`) and bucket:

| Path prefix | Bucket | Reason |
| --- | --- | --- |
| `apps/website/` | SKIP | Marketing/platform site, not core ComfyUI bundle |
| `apps/desktop-ui/` | SKIP for `core/*` | Desktop app, separate release cadence |
| `browser_tests/` only (no `src/`) | SKIP | Test-only |
| `.github/workflows/` only | SKIP | CI/release infra |
| `packages/design-system/` only | SKIP | Design tokens, not core |
| `packages/{cloud,registry,ingest}-types/` only | SKIP | Generated types |
| `.claude/`, `.agents/`, `docs/` | SKIP | Agent / documentation |
| `*.stories.ts` only | SKIP | Storybook only |
| `src/` (core editor) | KEEP — analyze further |

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
A PR touches multiple paths? Keep it if **any** changed file is under `src/` (or other core paths) and run normal analysis. Auto-skip is conservative — only skip when *all* paths match the SKIP buckets.

This filter alone removes ~30-50% of candidates in a typical session, leaving only the PRs that need real triage.

## Wave Verification

After merging each wave of PRs to a target branch, verify branch integrity before proceeding:
Expand Down
80 changes: 80 additions & 0 deletions .claude/skills/backport-management/reference/analysis.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,86 @@ Check before backporting — these don't exist on older branches:
- **App builder** — check per branch
- **appModeStore.ts** — not on core/1.40

### Verify Target File Existence (Run Before Cherry-Pick)

Before cherry-picking any PR, confirm the files it modifies actually exist on the target branch. If they don't, the PR's runtime fix is for a feature that hasn't been added yet — skip cleanly without attempting cherry-pick:

```bash
# For each file the PR changes
for f in $(gh pr view $PR --json files --jq '.files[].path' | grep -v "^browser_tests/\|\.test\." ); do
if ! git cat-file -e origin/$TARGET:$f 2>/dev/null; then
echo "MISSING on $TARGET: $f"
fi
done
```

If the *primary* changed files (the runtime ones, not tests) are missing, mark the PR `SKIP / feature-not-on-branch`. This is faster than letting cherry-pick fail with modify/delete conflicts and gives a clean signal.

This check is the first thing that runs after the path pre-filter and BEFORE you spend time reading PR descriptions.

## Tiered Triage (Recommended for 30+ Candidates)

Before the interactive Y/N approval flow, bucket all surviving candidates into three tiers. This surfaces release-engineering decisions that a flat MUST/SHOULD list obscures:

### Tier 1 — Core Editor Must-Haves

User-facing bugs, crashes, data corruption, or security issues in code paths that exist on the target branch. These are the strongest backport candidates.

Indicators:
- `fix:` prefix and the bug is reproducible on the target branch
- Crash guards, runtime null checks, race-condition fixes
- Data-loss bugs (state not persisted, duplicates, drops)
- Security hardening (CSRF, XSS, auth)
- Vue Nodes 2.0 regression cluster (if the target ships Vue Nodes 2.0)
- Subgraph correctness fixes
- Public-API extension callback fixes

Recommend `Y` to user.

### Tier 2 — Cloud-Distribution Only

Bugs that only manifest on cloud-hosted distributions (Secrets panel, subscription flows, cloud signup, workspace tracking, etc.). Whether to backport depends on whether cloud ships from the target `core/*` branch in your release matrix.

Indicators:
- Files under `src/platform/secrets/`, `src/platform/subscription/`, signup flows
- PR description mentions cloud staging issues
- Fix gated behind cloud feature flags

Default: ask the cloud release rotation owner. If unsure, defer.

### Tier 3 — Skip

Path pre-filter caught most of these. The rest are PRs where the diff *touches* `src/` but the practical impact is non-user-facing or scoped to features the target doesn't ship.

Indicators:
- All changes in test files even if the PR touched `src/` test files
- Storybook stories only
- Lint config / lint rule additions
- Documentation comments
- Internal refactors with no behavior change

### Presentation Format

When showing tier results to the user, format as:

```
Tier 1 (N PRs) — strong backport candidates
- #11541 fix: stop duplicate node creation when dropping image on Vue nodes
Why: Vue Nodes 2.0 regression — async onDragDrop bypassed handled-check, drops bubble to document, spawns extra LoadImage nodes
- #10849 fix: store promoted widget values per SubgraphNode instance
Why: Multiple instances overwriting each other's promoted widget values — data loss

Tier 2 (N PRs) — cloud-distribution release rotation should decide
- #11636 fix: enable Chrome password autofill on signup form
- ...

Tier 3 (N PRs) — skip recommended
- #11586 fix: website polish (apps/website/ only)
- ...
```
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Then run interactive Y/N over Tier 1 and Tier 2; Tier 3 gets confirmed-skip without per-PR review.

## Dep Refresh PRs

Always SKIP on stable branches. Risk of transitive dependency regressions outweighs audit cleanup benefit. If a specific CVE fix is needed, cherry-pick that individual fix instead.
Expand Down
43 changes: 42 additions & 1 deletion .claude/skills/backport-management/reference/discovery.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Discovery — Candidate Collection

**Run all sources, then reconcile.** No single source is authoritative:
- Slack bot may flag PRs that have already been backported (false positive)
- Git gap may include PRs that don't need backport (test-only, design-system, website)
- Bot can also miss PRs that landed without the right labels

## Source 1: Slack Backport-Checker Bot

Use `slackdump` skill to export `#frontend-releases` channel (C09K9TPU2G7):
Expand Down Expand Up @@ -36,7 +41,43 @@ gh pr view $PR --json mergeCommit,title --jq '"Title: \(.title)\nMerge: \(.merge
gh pr view $PR --json files --jq '.files[].path'
```

## Source 4: Already-Backported PRs (cross-reference)

When the target branch already has some cherry-picks on it (e.g., partway through a release window), extract the originals to avoid re-backporting:

```bash
# Get all original PR numbers already backported to TARGET since the last release tag
git log --format="%H%n%B" $LAST_TAG..origin/$TARGET \
| grep -oiE "(backport of|cherry.picked) #?[0-9]+" \
| grep -oE "[0-9]+" \
| sort -un > /tmp/already-backported.txt
```

Subtract this list from your candidates.

## Reconciliation Workflow

```bash
# 1. Slack bot list (parse from export)
# /tmp/bot-flagged.txt — one PR# per line, sorted

# 2. Git gap fix/perf only
MB=$(git merge-base origin/main origin/$TARGET)
git log --format="%h|%s" $MB..origin/main \
| grep -iE "^[a-f0-9]+\|(fix|perf)" \
| grep -oE "#[0-9]+\)" | grep -oE "[0-9]+" \
| sort -un > /tmp/gap-fixes.txt

# 3. Already backported (Source 4 above)

# 4. Candidates = (gap-fixes ∪ bot-flagged) − already-backported
sort -u /tmp/gap-fixes.txt /tmp/bot-flagged.txt > /tmp/union.txt
comm -23 /tmp/union.txt /tmp/already-backported.txt > /tmp/candidates.txt
```

The result is the input to the path pre-filter (`SKILL.md` Quick Start step 2).

## Output: candidate_list.md

Table per target branch:
| PR# | Title | Category | Flagged by Bot? | Decision |
| PR# | Title | Source (bot/gap/both) | Path bucket | Tier | Decision |
91 changes: 91 additions & 0 deletions .claude/skills/backport-management/reference/execution.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,42 @@
2. Medium gap next (quick win)
3. Largest gap last (main effort)

## Step 0: Test-Then-Resolve Pre-Pass (Recommended)

Before triggering label-driven automation, run a dry-run cherry-pick loop to classify candidates. This is much faster than discovering conflicts after-the-fact across automation, manual cherry-picks, and CI failures.

```bash
git fetch origin TARGET_BRANCH
git worktree add /tmp/dryrun-TARGET origin/TARGET_BRANCH
cd /tmp/dryrun-TARGET

CLEAN=()
CONFLICT=()
for pr in "${CANDIDATES[@]}"; do
SHA=$(gh pr view $pr --json mergeCommit --jq '.mergeCommit.oid')
git checkout -b dryrun-$pr origin/TARGET_BRANCH 2>/dev/null
if git cherry-pick -m 1 $SHA 2>/dev/null; then
CLEAN+=($pr)
else
CONFLICT+=($pr)
git cherry-pick --abort
fi
git checkout --detach HEAD 2>/dev/null
git branch -D dryrun-$pr 2>/dev/null
done

echo "CLEAN (${#CLEAN[@]}): ${CLEAN[*]}"
echo "CONFLICT (${#CONFLICT[@]}): ${CONFLICT[*]}"

cd -
git worktree remove /tmp/dryrun-TARGET --force
```

Use the result to:
- Send CLEAN PRs through label-driven automation (Step 1) — they'll typically self-merge
- Reserve manual worktree time (Step 3) for CONFLICT PRs only
- Surface PRs likely to need backport-only compat shims (CONFLICT files in `src/lib/litegraph/` or `src/scripts/app.ts`)

## Step 1: Label-Driven Automation (Batch)

```bash
Expand Down Expand Up @@ -88,6 +124,25 @@ for PR in ${CONFLICT_PRS[@]}; do
git add .
GIT_EDITOR=true git cherry-pick --continue

# ── Public-API conflict review (REQUIRED for extension-API surfaces) ──
# If the conflict resolution touched any of these surfaces, consult oracle
# BEFORE pushing. A bad shim is worse than no fix:
# - node.onXxx callback assignments (onDragDrop, onConnectionsChange, onRemoved, onConfigure, etc.)
# - Methods on LGraphNode, LGraphCanvas, LGraph, Subgraph
# - Public exports from src/lib/litegraph/
# - Type changes in litegraph-augmentation.d.ts
# If a public callback's signature/contract changed: add a backport-only
# compatibility shim that preserves the OLD contract while keeping the
# new fix. Document it in the commit body under
# "## Backport-only compatibility fix". See SKILL.md gotcha section.
# ───────────────────────────────────────────────────────────────────────

# Per-PR validation BEFORE push (catches issues earlier than wave verification):
pnpm typecheck && \
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)$") && \
pnpm exec oxfmt --check $(git diff --name-only HEAD~1 | grep -E "\.(ts|vue)$")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

git push origin backport-$PR-to-TARGET --no-verify
NEW_PR=$(gh pr create --base TARGET_BRANCH --head backport-$PR-to-TARGET \
--title "[backport TARGET] TITLE (#$PR)" \
Expand Down Expand Up @@ -243,6 +298,9 @@ gh pr checks $PR --watch --fail-fast && gh pr merge $PR --squash --admin
16. **Use `--no-verify` in worktrees** — husky hooks fail in `/tmp/` worktrees. Always push/commit with `--no-verify`.
17. **Automation success varies by branch** — core/1.42 got 18/26 auto-PRs (69%), cloud/1.42 got 1/25 (4%). Cloud branches diverge more. Plan for manual fallback.
18. **Test-then-resolve pattern** — for branches with low automation success, run a dry-run loop to classify clean vs conflict PRs before processing. This is much faster than resolving conflicts serially.
19. **Public-API conflict resolutions need oracle review** — when a conflict touches `node.onXxx` callbacks, `LGraphNode`/`LGraphCanvas`/`LGraph`/`Subgraph` methods, or types in `litegraph-augmentation.d.ts`, consult oracle BEFORE pushing. Custom-node packages depend on these contracts. A literal cherry-pick of a refactor-style fix can silently break extensions still using the old contract — sometimes recreating the very bug the PR was fixing. Document any backport-only compatibility shim explicitly in the commit body.
20. **Cherry-picked tests can require unbackported test scaffolding** — when a PR modifies a test file that was *added* on main by an earlier unbackported PR, the cherry-pick reports modify/delete on that file. Drop it from the backport (`git rm`) and document which PR introduced it. Don't smuggle in test infrastructure without its runtime prerequisites.
21. **Per-PR validation catches issues earlier than wave verification** — for high-stakes branches, run `pnpm typecheck && pnpm exec eslint <changed files> && pnpm exec oxfmt --check` per PR before pushing. Wave verification still matters (it catches cross-PR interactions), but per-PR makes attribution trivial when something fails.

## CI Failure Triage

Expand All @@ -268,3 +326,36 @@ Common failure categories:
| Type error | Interface changed on main but not branch | May need manual adaptation |

**Never assume a failure is safe to skip.** Present all failures to the user with analysis.

## PR Body Template (Manual Cherry-Picks)

Manual cherry-pick PRs need detail beyond the automation's terse default. Use this template — reviewers will look here before re-deriving conflict-resolution logic from the diff.

```markdown
Manual backport of #ORIG to `TARGET` for inclusion in `vX.Y.Z`.

Cherry-picked from upstream merge commit `SHORT_SHA`.

## Why
[1-2 sentences from the original PR's "Summary" — what bug, what fix mechanism]

## Conflict resolution
- **`path/to/file`** — [what conflicted on this branch] → [resolution chosen + why]
- **`path/to/dropped-test.test.ts`** — added on main by unrelated PR #XXXX (not backported). Dropped from this backport; runtime fix intact.
- [...]

## Backport-only compatibility fix (if applicable)
[If you added a shim that wasn't in the upstream PR, document it here — what extension surface, what contract, what the shim preserves, why the upstream version would have regressed it]

## Validation
- `pnpm typecheck` ✅
- `pnpm test:unit -- run <targeted suites>` ✅ (N/N passing)
- `pnpm exec eslint <changed files>` ✅ (0 errors)
- `pnpm exec oxfmt --check` ✅ (clean)

[If manual e2e was skipped, explain why — e.g., requires live backend, headless not feasible. State that source is byte-identical to upstream + how long it's been baking on main.]

Original PR: #ORIG / Original commit: `FULL_SHA`
```

The conflict-resolution section is non-negotiable — every conflict you resolved by hand needs a one-liner. This makes archaeology trivial six months later when someone asks "why does this look slightly different from main?"
Loading