Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
68 changes: 61 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,35 @@ 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 +181,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 | Runtime/editor code that requires full triage |

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
83 changes: 83 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,89 @@ 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:

```text
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)
- ...
```

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
44 changes: 43 additions & 1 deletion .claude/skills/backport-management/reference/discovery.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# 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 +42,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 |
Loading
Loading