From ba39cb36586fa1ca486c524ff40a2b30f1cae2d6 Mon Sep 17 00:00:00 2001 From: malkuthro <166633396+malkuthro@users.noreply.github.com> Date: Sat, 20 Jun 2026 11:16:38 +0200 Subject: [PATCH] Claude/goofy faraday d23e98 (#255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(nodes): drop keyframe_batch alias from Easy Image Batch The keyframe_batch input was a deprecated alias of keyframes_insert, added in 0.4.1 during PR #246 review to keep pre-rename workflows loading. The old name only ever existed in unreleased dev builds before the rename, so no published workflow depends on it and the alias only cluttered the node with a second IMAGE input. Remove the input, its parameter, and the alias-routing fallback; the node now exposes a single keyframes_insert insert input. Tests and the behavior spec updated; CHANGELOG notes the removal under [Unreleased]. Co-Authored-By: Claude Opus 4.8 * docs(policy): make backward compatibility opt-in for Koolook nodes Invert the default: backward compatibility is no longer a standing constraint for Koolook-created nodes (the root k_*.py custom nodes). By default, make the cleanest change — rename/drop/reorder IDs, inputs, and outputs freely, no deprecation aliases or _v2 suffixes. Compatibility with saved workflows is only preserved when the maintainer explicitly says "check backward compatibility". Forks are explicitly excluded: anything under forks/ (including the Koolook-original nodes exposed via SKIP_VERSION_SUFFIX) keeps full back-compat discipline by default, no request needed. The __koolook_vX_Y_Z collision suffix always applies. Scoped across the binding instruction files and skills: CLAUDE.md, copilot-instructions, node-versioning.md, versioning.md, and the preflight-release / add-external-fork / tdd skills. The preflight workflows check is reframed as a drift detector for Koolook IDs and a hard back-compat gate for fork IDs. Co-Authored-By: Claude Opus 4.8 * docs: clarify keyframe_batch alias removal * fix: retain keyframe_batch compatibility alias --------- Co-authored-by: AI Machine Co-authored-by: Claude Opus 4.8 --- .claude/skills/add-external-fork/SKILL.md | 9 ++++- .claude/skills/preflight-release/SKILL.md | 25 ++++++++---- .claude/skills/tdd/SKILL.md | 13 +++++-- .cursor/skills/add-external-fork/SKILL.md | 2 +- .github/copilot-instructions.md | 12 ++++-- CLAUDE.md | 22 ++++++++++- docs/designs/easy-image-batch-behavior.md | 4 +- docs/maintainers/node-versioning.md | 46 +++++++++++++++++------ docs/reference/versioning.md | 9 +++++ k_easy_image_batch.py | 5 +-- tests/nodes/test_easy_image_batch.py | 7 ++-- 11 files changed, 116 insertions(+), 38 deletions(-) diff --git a/.claude/skills/add-external-fork/SKILL.md b/.claude/skills/add-external-fork/SKILL.md index 8768037..d0f32b9 100644 --- a/.claude/skills/add-external-fork/SKILL.md +++ b/.claude/skills/add-external-fork/SKILL.md @@ -42,8 +42,13 @@ retroactive relicense to GPL-3.0 in v0.1.2. Don't repeat it. match the version folder). - **Single source of truth:** `forks/forks_manifest.yaml`. Every external repo must have an entry there. -- **Never rename existing namespaced node IDs** that already appear in saved - workflows. New versions get a new suffix. +- **Node IDs carry the `__koolook_vX_Y_Z` suffix** so a new fork version can + coexist with the old one and never collides with a separately-installed + upstream copy. **Never rename an existing fork node ID that appears in saved + workflows; new versions get a new suffix.** Fork nodes keep full + backward-compat discipline by **default** — the opt-in "clean rename" default + in [`CLAUDE.md`](../../../CLAUDE.md) → *Change management* applies to + Koolook-created nodes only, **not** forks. ## Phase 1 — Discover diff --git a/.claude/skills/preflight-release/SKILL.md b/.claude/skills/preflight-release/SKILL.md index 60ab69c..c8565cb 100644 --- a/.claude/skills/preflight-release/SKILL.md +++ b/.claude/skills/preflight-release/SKILL.md @@ -18,8 +18,10 @@ This one is about **code/metadata correctness**. - **Before cutting a release.** Run it after the release-prep PR is open, before triggering the publish workflow. - **Before merging any PR that touches node IDs or fork structure.** A - changed `NODE_CLASS_MAPPINGS` or a renamed fork directory is exactly - the kind of edit that can silently break saved workflows. + changed `NODE_CLASS_MAPPINGS` or a renamed fork directory surfaces here + so the workflow breakage is a *conscious* choice, not an accident. + (Breaking a **Koolook** node is allowed by default; breaking a **fork** + node is not — see the `workflows` check below for the split.) - **After running [`docs-sync`](../docs-sync/SKILL.md) on a fork-version bump.** The string-sweep can leave references stale if anything is missed; pre-flight catches it before merge. @@ -105,10 +107,19 @@ from the CI job and let it become a release blocker. - Asserts every Koolook ID found in the fixtures is still in the AST-extracted set from check 1. -If a fixture references a node ID we no longer register, **the saved -workflow would break for any user who has it**. The pre-flight fails -loudly so the rename gets either reverted, aliased for back-compat, or -treated as a `_v2`-suffix breaking change with a deprecation cycle. +If a fixture references a node ID we no longer register, this check fails +loudly. How to read the failure depends on which node it is (see +[`CLAUDE.md`](../../../CLAUDE.md) → *Change management*): + +- **Koolook-created node** (root `k_*.py`, e.g. `easy_ImageBatch`): treat + this as a **drift detector**, not a back-compat gate. Back-compat is + opt-in for Koolook nodes, so an *intentional* rename is fine — the fix is + to **update the fixture** to the new ID. The value here is catching an + *unintended* rename you didn't mean to make. +- **Fork node** (anything under `forks/`, e.g. the `Easy_hdr_VAE_*` or a + `*__koolook_vX_Y_Z` ID): this is a real **back-compat gate**. Fork IDs + must stay stable — add the old ID back as an alias or version it; do + **not** just edit the fixture. Adding more fixtures over time grows coverage without code changes: drop a new `*.json` into `tests/workflows/`, the script picks it up @@ -124,7 +135,7 @@ convention. | dispatch | Assertion fails on one of the five branches | Look at the changed file in `forks/radiance_koolook/versions/v2_3_3/`. Trace the dispatch logic in `nodes_vae.py:Easy_hdr_VAE_{encode,decode}`. The branch that broke tells you which rank/VAE combination regressed. | | manager-meta (phantom IDs) | Upstream Manager metadata has IDs we don't register | File / update issue #44; not a code fix on our side. Skip the check. | | manager-meta (missing IDs) | Our code has IDs upstream Manager doesn't list yet | Wait for the next upstream "update DB" run, or open an upstream PR. Skip the check until it propagates. | -| workflows | Fixture references a node ID no longer registered | (a) Add the old ID back as a `NODE_CLASS_MAPPINGS` alias pointing at the renamed class, OR (b) update the fixture if the rename was intentional + announced + the user was warned, OR (c) introduce a `_v2` suffix on the new node and start a deprecation cycle. | +| workflows | Fixture references a node ID no longer registered | **Koolook node (`k_*.py`):** update the fixture to the new ID — an intentional rename is fine. **Fork node (under `forks/`), or any node after `check backward compatibility`:** (a) add the old ID back as a `NODE_CLASS_MAPPINGS` alias pointing at the renamed class, OR (b) introduce a `_v2` suffix and start a deprecation cycle. | ## Companion docs diff --git a/.claude/skills/tdd/SKILL.md b/.claude/skills/tdd/SKILL.md index 210e967..6b22783 100644 --- a/.claude/skills/tdd/SKILL.md +++ b/.claude/skills/tdd/SKILL.md @@ -173,10 +173,15 @@ fine in code review but cause incidents in this codebase: - **Vendored upstream code into MAIN** instead of into `forks/`. Project CLAUDE.md is explicit: third-party code stays outside MAIN; MAIN holds wrapper loaders + tracking docs only. -- **Renamed a namespaced node ID** that already appears in saved - workflows. Node IDs are external API — never rename `Easy_*`, - `*__koolook_v2_3_3`, or any other ID present in `tests/workflows/*.json` - without a deprecation + alias. +- **Renamed a namespaced node ID.** For a **Koolook-created node** (root + `k_*.py`, e.g. `easy_ImageBatch`) this is fine by default — backward + compatibility is opt-in (project CLAUDE.md → *Change management*); just + update the matching `tests/workflows/*.json` fixtures to the new ID. **Fork + node IDs are the exception** — anything under `forks/` (the `Easy_hdr_VAE_*` + / `*__koolook_v2_3_3` IDs) keeps full back-compat discipline by default: + never rename those in saved workflows without a deprecation alias. The trap + to avoid for Koolook nodes is an *accidental* rename that leaves fixtures + stale. - **Modified a `forks//versions/vX_Y_Z/`** file expecting upstream to also change — those are frozen baseline pins (per Radiance fork policy). diff --git a/.cursor/skills/add-external-fork/SKILL.md b/.cursor/skills/add-external-fork/SKILL.md index 02b12e3..5c91fc1 100644 --- a/.cursor/skills/add-external-fork/SKILL.md +++ b/.cursor/skills/add-external-fork/SKILL.md @@ -22,7 +22,7 @@ Use this skill whenever the user wants to: - **Local version paths in MAIN:** `forks//versions/v__/` - **Node ID namespace suffix:** `__koolook_v__` (must match the version folder). - **Single source of truth:** `forks/forks_manifest.yaml`. Every external repo must have an entry there. -- **Never rename existing namespaced node IDs** that already appear in saved workflows. New versions get a new suffix. +- **Never rename existing namespaced node IDs** that already appear in saved workflows. New versions get a new suffix. Fork nodes keep full backward-compat discipline by **default** — the opt-in "clean rename" default in `CLAUDE.md` → *Change management* applies to Koolook-created nodes only, **not** forks. ## Phase 1 — Discover diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 7ffef6d..241fba3 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -44,9 +44,15 @@ ComfyUI-Koolook is a custom-node pack for ComfyUI. Two shipped surfaces: ## Things that bite if you don't know them -- **Node IDs in saved workflows are stable.** Never rename a registered node - ID without a back-compat alias — see - [`docs/maintainers/node-versioning.md`](../docs/maintainers/node-versioning.md). +- **Backward compatibility is opt-in for Koolook-created nodes only** (the root + `k_*.py` custom nodes). For those, make the cleanest change by default — + rename/drop/reorder IDs, inputs, and outputs freely, no aliases or `_v2` + suffixes — unless the maintainer says `check backward compatibility`. + **Fork nodes are different: anything under `forks/` keeps full back-compat + discipline by default** (never rename a fork ID in saved workflows, version + via new suffix/namespace, keep the `__koolook_vX_Y_Z` collision suffix). See + [`docs/maintainers/node-versioning.md`](../docs/maintainers/node-versioning.md) + and [`CLAUDE.md`](../CLAUDE.md) → *Change management*. - **Third-party code stays out of MAIN.** Modified fork code lives under `forks//versions//`; raw upstream checkouts live in `../ComfyUI-Forks/`. See [`forks/README.md`](../forks/README.md) and diff --git a/CLAUDE.md b/CLAUDE.md index 48a19ce..87c4650 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -78,7 +78,27 @@ Two things still need your attention, and only these: ## Change management - When upgrading external forks (v2, v3, etc.), add a new wrapper/version namespace. -- Never rename existing namespaced node IDs that already appear in saved workflows. +- **Backward compatibility is opt-in for Koolook-created nodes ONLY.** This + means the custom nodes authored in MAIN — the root `k_*.py` files + (`easy_ImageBatch`, `EasyResize_Koolook`, etc.), **not** anything under + `forks/`. For those Koolook nodes, default to the cleanest change: rename + inputs/outputs/node IDs, drop fields, reorder combos *without* deprecation + aliases or `_v2` suffixes, and don't treat saved-workflow breakage as a + blocker. Only preserve compatibility when the maintainer explicitly asks. +- **Forks are the exception — they KEEP full backward-compat discipline by + default, no request needed.** Anything under `forks/` (upstream + ports/wrappers, *including* the Koolook-original nodes exposed via + `SKIP_VERSION_SUFFIX` like `Easy_hdr_VAE_encode`) still follows + [`docs/maintainers/node-versioning.md`](docs/maintainers/node-versioning.md) + and [`docs/reference/versioning.md`](docs/reference/versioning.md) without + being asked: never rename a fork node ID that appears in saved workflows, + version via a new suffix/namespace, keep the `__koolook_vX_Y_Z` suffix to + avoid colliding with separately-installed upstream copies, treat + `RETURN_TYPES` as immutable, etc. +- **Trigger phrase: `check backward compatibility`** (or an equivalent — + "don't break saved workflows", "keep it compatible") raises a *Koolook* + node to the same fork-grade discipline for that change. Absent the phrase, + Koolook-node breakage is acceptable; fork breakage is never the default. ## `dev-sync` — copy runtime files into a live ComfyUI install diff --git a/docs/designs/easy-image-batch-behavior.md b/docs/designs/easy-image-batch-behavior.md index 56700b9..4094c5b 100644 --- a/docs/designs/easy-image-batch-behavior.md +++ b/docs/designs/easy-image-batch-behavior.md @@ -13,8 +13,8 @@ supersedes the generated demo removed here. This captures the agreed changes: -1. `keyframe_batch` → **`keyframes_insert`** (rename; the old name is kept as a - deprecated alias input so pre-rename workflows keep loading). +1. `keyframe_batch` → **`keyframes_insert`** (rename; the old name remains as a + deprecated alias input so workflows saved on 0.4.1/0.4.2 keep loading). 2. **No image source connected → clean placeholder batch** sized by new `width`/`height` widgets (no error). 3. **Insert-over-source mode** — `keyframes_insert` + `source_batch` composite diff --git a/docs/maintainers/node-versioning.md b/docs/maintainers/node-versioning.md index 26c94db..92283d1 100644 --- a/docs/maintainers/node-versioning.md +++ b/docs/maintainers/node-versioning.md @@ -1,9 +1,21 @@ # Node versioning — how to change a node without breaking saved workflows +> **⚠️ Scope — OPT-IN for Koolook nodes, MANDATORY for forks.** +> Backward compatibility is **not** a default constraint for Koolook-created +> nodes (the root `k_*.py` custom nodes); for those, **ignore everything below** +> and make the cleanest change — rename/drop/reorder freely — *unless* the +> maintainer says `check backward compatibility`. **For anything under +> `forks/`** (upstream ports/wrappers, including the Koolook-original nodes +> exposed via `SKIP_VERSION_SUFFIX`) **these rules still apply by default** — +> forks keep full back-compat discipline with no request needed. See +> [`CLAUDE.md`](../../CLAUDE.md) → *Change management*. + This is a hard-won list. ComfyUI's workflow format silently breaks in several non-obvious ways when you change a node's inputs, outputs, or -class name. The rules below are what to follow whenever you touch -`INPUT_TYPES`, `RETURN_TYPES`, or `NODE_CLASS_MAPPINGS`. +class name. The rules below are what to follow whenever you touch a +**fork** node's `INPUT_TYPES` / `RETURN_TYPES` / `NODE_CLASS_MAPPINGS`, +or a **Koolook** node's once `check backward compatibility` has been +requested. ## How ComfyUI loads a saved workflow @@ -151,17 +163,29 @@ NODE_CLASS_MAPPINGS = { } ``` -## Anti-patterns to refuse +## Anti-patterns to refuse — *for forks, or after `check backward compatibility`* + +These are anti-patterns for **any fork node**, and for a **Koolook node only +once the maintainer has asked** to preserve backward compatibility. For a +Koolook node with no such request, every one of them is the *correct* clean +change — append-vs-insert is the lone exception below. - ❌ "I'll just add the field in the middle for clarity, the dict order - doesn't matter." It does. Append. -- ❌ "I'll rename the input to be clearer." Don't. Add a new optional - input with the clearer name; deprecate the old one in display only. -- ❌ "It's a small change, no need to bump the node version." If the - input/output surface changes in a way that could break a saved - workflow → new node ID. Period. -- ❌ "Nobody's using the old node yet." You don't actually know. Default - to alias-then-deprecate; cost is one extra dict entry. + doesn't matter." It does. Append. *(This one is worth following even + without a back-compat request — a mid-list widget insertion silently + scrambles the `widgets_values` of any workflow already on disk, including + your own dev graphs, for zero benefit. Appending is free.)* +- ❌ "I'll rename the input to be clearer." For a fork node, or any node + under a back-compat request: add a new optional input with the clearer + name and deprecate the old one in display only. For a Koolook node with + no request, **just rename it.** +- ❌ "It's a small change, no need to bump the node version." For a fork + node, or under a back-compat request, a surface change that could break a + saved workflow → new node ID. For a Koolook node with no request, change + in place and note it in the CHANGELOG. +- ❌ "Nobody's using the old node yet." For a fork node, or under a + back-compat request, you can't assume that. For a Koolook node with no + request you *do* assume it — clean change, move on. ## Tooling — what we have, what's still missing diff --git a/docs/reference/versioning.md b/docs/reference/versioning.md index c6f5585..f07fc37 100644 --- a/docs/reference/versioning.md +++ b/docs/reference/versioning.md @@ -68,6 +68,15 @@ new wrapper version too. ## Public node-ID versioning (the `_v2` rule) +> **Scope.** This section is **mandatory for fork nodes** (anything under +> `forks/`, like the `Easy_hdr_VAE_*` example used below) — they keep full +> back-compat discipline by default. For **Koolook-created nodes** (the root +> `k_*.py` custom nodes) it is **opt-in**: by default a breaking change is made +> **in place** — rename or alter the existing ID, note it in the CHANGELOG, +> done — with no `_v2` suffix and no deprecation cycle, unless the maintainer +> asked to `check backward compatibility`. See [`CLAUDE.md`](../../CLAUDE.md) → +> *Change management*. + Independent of the three axes above, individual node IDs can carry a public version suffix when a breaking change forces it: diff --git a/k_easy_image_batch.py b/k_easy_image_batch.py index cc705b8..e803bb9 100644 --- a/k_easy_image_batch.py +++ b/k_easy_image_batch.py @@ -366,9 +366,8 @@ class easy_ImageBatch: "tooltip": ( "Deprecated alias for keyframes_insert (the input was " "renamed before release). Use keyframes_insert; this alias " - "keeps pre-rename workflows loading and will be removed in " - "a future release. Ignored when keyframes_insert is also " - "connected." + "keeps pre-rename workflows loading. Ignored when " + "keyframes_insert is also connected." ), }), "source_batch": ("IMAGE", ), diff --git a/tests/nodes/test_easy_image_batch.py b/tests/nodes/test_easy_image_batch.py index d3fd233..9e9fe98 100644 --- a/tests/nodes/test_easy_image_batch.py +++ b/tests/nodes/test_easy_image_batch.py @@ -247,8 +247,8 @@ def test_slot_overwrites_empty_input_places_nothing(): def test_input_types_exposes_keyframes_insert_with_tooltip(): spec = easy_ImageBatch.INPUT_TYPES() assert "keyframes_insert" in spec["optional"], "insert-mode input must be declared" - # `keyframe_batch` is retained as a deprecated alias so pre-rename workflows - # keep loading (unreleased, but origin/main exposed it). + # `keyframe_batch` is retained as a deprecated alias so workflows saved on + # v0.4.1/v0.4.2 keep loading. assert "keyframe_batch" in spec["optional"] assert "deprecated" in spec["optional"]["keyframe_batch"][1]["tooltip"].lower() entry = spec["optional"]["keyframes_insert"] @@ -538,8 +538,7 @@ def test_parse_frame_tokens_range_span_boundary(): @requires_fake_torch def test_keyframe_batch_alias_routes_to_insert_mode(): - # A pre-rename workflow wires `keyframe_batch` (not `keyframes_insert`); it - # must still behave like an insert. + # A workflow saved with the deprecated alias still behaves like an insert. node = easy_ImageBatch() _img, _alpha, _sel, frames = node.create_batch( total_frames=24, cut_start_frame=1, placeholder_color="Gray",