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",