From 3ffb7b75aa19bc72207bd6bcf71715c8f6cc04a9 Mon Sep 17 00:00:00 2001 From: malkuthro <166633396+malkuthro@users.noreply.github.com> Date: Thu, 28 May 2026 23:30:50 +0200 Subject: [PATCH] fix(review): harden relay_overrides parser + polish (PR #178 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses verified findings from the /review-pr pass on PR #178. H1 — relay_overrides values are now type-checked - Allowlist of recognised knobs (`video_strength`, `video_window_scale`, `audio_strength`, `audio_window_scale`, `audio_epsilon`) with explicit float-coercion. Numeric strings ("10") coerce silently; non-numeric values (e.g. "ten") now raise a ValueError naming the offending knob and showing a minimal correct example — instead of a TypeError deep inside `build_segments` arithmetic, or a SUCCESS render at upstream defaults that polluted the A/B iteration log. H2 — malformed JSON no longer silently falls back to defaults - `json.JSONDecodeError`, non-dict JSON (arrays, strings, bare numbers, null), and non-numeric values for recognised knobs all now raise `ValueError` with operator-actionable messages. The only silent fallback is the explicit empty/whitespace case — "use upstream defaults" is the operator's intent there. - Typo'd keys (e.g. `viedo_strength`) log a warning naming the unknown keys + the recognised set, then drop the typo. Previously they were silently retained in the cleaned dict but never queried by `build_segments`, indistinguishable from a default render. Architectural side-effect — shrinks the diff vs vendored upstream - `_parse_relay_overrides` extracted into a stdlib-only sibling module `_relay_overrides.py`. `ltx_director.py` shrinks closer to upstream baseline (one import + one alias instead of a 30-line function body) and is easier to diff against `e81223a`. The parser is also now unit-testable without ComfyUI's `comfy_api` runtime. L5 — tests added (31 cases, all passing) - New `tests/forks/__init__.py` + `tests/forks/test_whatdreamscost_v1_3_2_relay_overrides.py`. - Covers every confirmed failure mode (H1 + H2), the happy path, the comment-key convention, the unknown-key warning, and an allowlist- consistency assertion that fails loudly if `build_segments` ever grows a knob without the parser being updated. - Test module uses `importlib.util.spec_from_file_location` so it can load the parser without walking through the package hierarchy (which would trigger the `comfy_api` import inside `ltx_director.py`). L3 — `_build_line` promoted to public `build_line` - `scripts/sync_to_dev.py` renames the private helper. The underscore alias is kept for one release cycle as a deprecation slot. - `scripts/sync_to_dev_audio.py` switches to `_dev.build_line()`. No behaviour change; chat-report shape is bit-identical to before. L2 — `source_ref` clarified in pin metadata - `forks/whatdreamscost_koolook/versions/v1_3_2/UPSTREAM_PIN.yaml` and `forks/forks_manifest.yaml` both now spell out that `e81223a` is the post-v1.3.2 README-only commit on upstream's `main`, not the v1.3.2 release tag itself. Code is byte-identical to the tagged release; the pin differs only in trailing docs. L1, L4 are explicitly left alone (L1 is dead upstream cruft, kept for minimum-diff; L4 is helper duplication tracked for a separate follow-up issue when a third fork lands). Verification - ruff check . → clean - pytest → 204 passed (173 prior + 31 new parser tests) - tools/preflight_release.py --skip manager-meta → all PASS (static AST extraction still picks up LTXDirector from the fork) - scripts/sync_to_dev_audio.py --dry-run → two-line chat report still emits via the renamed `build_line` helper Co-Authored-By: Claude Opus 4.7 (1M context) --- forks/forks_manifest.yaml | 2 +- .../versions/v1_3_2/UPSTREAM_PIN.yaml | 7 +- .../versions/v1_3_2/_relay_overrides.py | 126 ++++++++++ .../versions/v1_3_2/ltx_director.py | 36 +-- scripts/sync_to_dev.py | 17 +- scripts/sync_to_dev_audio.py | 2 +- tests/forks/__init__.py | 0 ...t_whatdreamscost_v1_3_2_relay_overrides.py | 215 ++++++++++++++++++ 8 files changed, 368 insertions(+), 37 deletions(-) create mode 100644 forks/whatdreamscost_koolook/versions/v1_3_2/_relay_overrides.py create mode 100644 tests/forks/__init__.py create mode 100644 tests/forks/test_whatdreamscost_v1_3_2_relay_overrides.py diff --git a/forks/forks_manifest.yaml b/forks/forks_manifest.yaml index 450b5f8..bd1371d 100644 --- a/forks/forks_manifest.yaml +++ b/forks/forks_manifest.yaml @@ -42,7 +42,7 @@ entries: - id: "whatdreamscost_koolook_v1_3_2_ltx_director" name: "WhatDreamsCost Koolook v1.3.2 LTX Director subset" source_repo: "https://github.com/WhatDreamsCost/WhatDreamsCost-ComfyUI.git" - source_ref: "v1.3.2" + source_ref: "v1.3.2 (pinned to post-tag README-only commit on main)" pinned_commit: "e81223a2add687555a6371e311b325880bf7c3c9" external_checkout: relative_path_from_forks_root: "whatdreamscost-v1.3.2-koolook" diff --git a/forks/whatdreamscost_koolook/versions/v1_3_2/UPSTREAM_PIN.yaml b/forks/whatdreamscost_koolook/versions/v1_3_2/UPSTREAM_PIN.yaml index 889ffb7..ddd1be0 100644 --- a/forks/whatdreamscost_koolook/versions/v1_3_2/UPSTREAM_PIN.yaml +++ b/forks/whatdreamscost_koolook/versions/v1_3_2/UPSTREAM_PIN.yaml @@ -1,5 +1,10 @@ source_repo: "https://github.com/WhatDreamsCost/WhatDreamsCost-ComfyUI.git" -source_ref: "v1.3.2" +# source_ref names the release. pinned_commit is the latest `main` SHA +# at the time the fork was cut — a README-only update sitting one commit +# beyond the actual `v1.3.2` release tag on upstream's history. Code is +# byte-identical to the tagged release; the SHA differs because upstream +# pushed a docs touch-up afterwards. +source_ref: "v1.3.2 (pinned to post-tag README-only commit on main)" pinned_commit: "e81223a2add687555a6371e311b325880bf7c3c9" external_checkout_relative_to_forks_root: "whatdreamscost-v1.3.2-koolook" upstream_tracking_relative_to_forks_root: "whatdreamscost-main-upstream" diff --git a/forks/whatdreamscost_koolook/versions/v1_3_2/_relay_overrides.py b/forks/whatdreamscost_koolook/versions/v1_3_2/_relay_overrides.py new file mode 100644 index 0000000..5b93189 --- /dev/null +++ b/forks/whatdreamscost_koolook/versions/v1_3_2/_relay_overrides.py @@ -0,0 +1,126 @@ +# SPDX-License-Identifier: GPL-3.0-or-later +""" +Pure-stdlib parser for the LTXDirector node's ``relay_overrides`` widget. + +Lives in its own module (not in ``ltx_director.py``) for two reasons: + +1. ``ltx_director.py`` imports ``comfy_api`` at module load, so it can't be + imported outside a running ComfyUI process. This module imports only + stdlib + logging, so unit tests can exercise the parser directly. +2. Keeping this Koolook addition out of ``ltx_director.py`` shrinks the + diff against vendored upstream — easier to spot the two upstream + modifications in ``ltx_director.py`` when comparing against the pinned + upstream commit. + +Parser contract — see :func:`parse_relay_overrides`. +""" +from __future__ import annotations + +import json +import logging +from typing import Optional + +log = logging.getLogger(__name__) + + +# Allowlist of recognised override keys and their coercion type. Anything +# outside this set (other than ``_``-prefixed comment keys) is dropped +# with a warning so typos like ``viedo_strength`` don't silently render +# at upstream defaults — they'd look identical to a clean default render +# in the iteration log, polluting any A/B comparison. +RELAY_OVERRIDE_KEYS: dict[str, type] = { + "video_strength": float, + "video_window_scale": float, + "audio_strength": float, + "audio_window_scale": float, + "audio_epsilon": float, +} + + +def parse_relay_overrides(s: str) -> Optional[dict]: + """Parse the LTXDirector ``relay_overrides`` multiline-string input. + + The widget value is JSON. Empty / whitespace input means "use upstream + defaults" — the only silent fallback. Anything else that doesn't + decode to a JSON object of numeric knob values is treated as + operator error and raises ``ValueError`` so the render fails fast + instead of silently producing a default render the maintainer would + later mistake for a real "the override had no effect" result. + + Args: + s: The widget's text body. + + Returns: + A dict of validated ``{knob_name: float}`` overrides, or ``None`` + when the input is empty/whitespace (meaning "use upstream + defaults"). The dict only contains keys from + :data:`RELAY_OVERRIDE_KEYS`; unknown keys are dropped after + logging a warning. + + Raises: + ValueError: when the input is non-empty but doesn't parse as a + JSON object, or when a recognised knob's value can't be + coerced to ``float``. The error message names the offending + knob and shows a minimal correct example so the operator can + fix the widget without consulting the upstream paper. + """ + if not s or not s.strip(): + return None + + try: + opts = json.loads(s) + except (json.JSONDecodeError, TypeError) as exc: + raise ValueError( + "relay_overrides input is not valid JSON " + f"({type(exc).__name__}: {exc}). Empty the field to use " + 'upstream defaults, or paste a JSON object like ' + '{"video_strength": 10.0}.' + ) from exc + + if not isinstance(opts, dict): + raise ValueError( + "relay_overrides must be a JSON object, got " + f"{type(opts).__name__}. Empty the field to use upstream " + 'defaults, or paste a JSON object like ' + '{"video_strength": 10.0}.' + ) + + cleaned: dict[str, float] = {} + unknown: list[str] = [] + + for key, value in opts.items(): + if key.startswith("_"): + # `_`-prefixed keys are reserved as inline JSON comments — + # carried in the widget value, ignored at parse time. + continue + if key not in RELAY_OVERRIDE_KEYS: + unknown.append(key) + continue + + coerce = RELAY_OVERRIDE_KEYS[key] + try: + cleaned[key] = coerce(value) + except (TypeError, ValueError) as exc: + raise ValueError( + f"relay_overrides[{key!r}] must be a number, got " + f"{value!r} ({type(value).__name__}). Example: " + f'{{"{key}": 10.0}}' + ) from exc + + if unknown: + log.warning( + "[PromptRelay] relay_overrides has unknown keys (typo?): %s. " + "Recognised keys: %s. Use `_`-prefixed keys for inline " + "comments.", + sorted(unknown), + sorted(RELAY_OVERRIDE_KEYS), + ) + + if not cleaned: + return None + + log.info("[PromptRelay] Loaded relay_overrides from node input: %s", cleaned) + return cleaned + + +__all__ = ["RELAY_OVERRIDE_KEYS", "parse_relay_overrides"] diff --git a/forks/whatdreamscost_koolook/versions/v1_3_2/ltx_director.py b/forks/whatdreamscost_koolook/versions/v1_3_2/ltx_director.py index 4b438b7..1f7705f 100644 --- a/forks/whatdreamscost_koolook/versions/v1_3_2/ltx_director.py +++ b/forks/whatdreamscost_koolook/versions/v1_3_2/ltx_director.py @@ -311,37 +311,11 @@ def _convert_to_latent_lengths(pixel_lengths, temporal_stride, latent_frames): return result -def _parse_relay_overrides(s): - """KOOLOOK PATCH (Koolook issue #177, 2026-05-28): - Parse Prompt Relay per-stream overrides from a JSON string typed into the - LTX Director's `relay_overrides` widget. No disk I/O — values live entirely - inside the workflow JSON and travel with it. - - Returns a dict or None. None == upstream behaviour. - - Supported keys (see prompt_relay.build_segments): - video_strength (float, default 1.0) — multiplies attn2 penalty - video_window_scale (float, default 1.0) — scales flat-top width - audio_strength (float, default 1.0) — multiplies audio_attn2 penalty - audio_window_scale (float, default 1.0) — scales flat-top width (audio) - audio_epsilon (float, None) — independent sigma for audio path - Underscore-prefixed keys are ignored (use for inline comments). - """ - if not s or not s.strip(): - return None - try: - opts = json.loads(s) - except Exception as e: - log.warning("[PromptRelay] relay_overrides input is not valid JSON, ignoring: %s", e) - return None - if not isinstance(opts, dict): - log.warning("[PromptRelay] relay_overrides must be a JSON object, got %s. Ignoring.", type(opts).__name__) - return None - cleaned = {k: v for k, v in opts.items() if not k.startswith("_")} - if not cleaned: - return None - log.info("[PromptRelay] Loaded relay_overrides from node input: %s", cleaned) - return cleaned +# KOOLOOK PATCH (issue #177, 2026-05-28): +# The relay_overrides parser lives in a stdlib-only sibling module so it +# can be unit-tested without ComfyUI. See _relay_overrides.py for the +# parser contract (allowlist, type coercion, fail-fast on malformed JSON). +from ._relay_overrides import parse_relay_overrides as _parse_relay_overrides def _encode_relay(model, clip, latent, global_prompt, local_prompts, segment_lengths, epsilon, relay_overrides=""): diff --git a/scripts/sync_to_dev.py b/scripts/sync_to_dev.py index 26c48ea..7c5887e 100644 --- a/scripts/sync_to_dev.py +++ b/scripts/sync_to_dev.py @@ -110,7 +110,7 @@ def _get_worktree_name() -> str: return REPO_ROOT.name -def _build_line() -> str: +def build_line() -> str: """Composes the two-piece header line consumed by the chat-report convention defined in project CLAUDE.md: @@ -118,11 +118,22 @@ def _build_line() -> str: SHA falls back to ``unknown`` if git is unreachable (we always need SOMETHING in slot 1 — the line shape is part of the convention). - Worktree name comes from ``REPO_ROOT.name`` and is always present.""" + Worktree name comes from ``REPO_ROOT.name`` and is always present. + + Public — consumed by scoped per-module wrappers like + ``sync_to_dev_audio.py`` so every dev-sync variant emits the same + chat-report header. + """ sha = _get_short_sha() or "unknown" return f"{sha} - {_get_worktree_name()}" +# Backwards-compatible alias (the function was private until the audio +# wrapper landed). Drop after the next release cycle once we're sure no +# downstream caller imports the underscore name. +_build_line = build_line + + def trigger_restart(url: str, timeout: float = 2.0) -> tuple[bool, str]: """Best-effort POST/GET to ComfyUI-Manager's reboot endpoint. @@ -431,7 +442,7 @@ def main() -> int: # chat-report convention that consumes this output. Header first so # the maintainer's eye lands on the build identifier before the # mechanical sync details. - print(_build_line()) + print(build_line()) print(f"{verb} {n} entries -> {target}") if not args.dry_run: write_build_info(target, args.scope) diff --git a/scripts/sync_to_dev_audio.py b/scripts/sync_to_dev_audio.py index 63d5cc3..32e0f62 100644 --- a/scripts/sync_to_dev_audio.py +++ b/scripts/sync_to_dev_audio.py @@ -148,7 +148,7 @@ def main() -> int: paths=AUDIO_PATHS, ) verb = "would sync" if args.dry_run else "synced" - print(_dev._build_line()) + print(_dev.build_line()) print(f"{verb} {n} entries -> {target} (dev-sync-audio)") if not args.dry_run: _dev.write_build_info(target, args.scope) diff --git a/tests/forks/__init__.py b/tests/forks/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/forks/test_whatdreamscost_v1_3_2_relay_overrides.py b/tests/forks/test_whatdreamscost_v1_3_2_relay_overrides.py new file mode 100644 index 0000000..599f0e5 --- /dev/null +++ b/tests/forks/test_whatdreamscost_v1_3_2_relay_overrides.py @@ -0,0 +1,215 @@ +"""Tests for the LTXDirector ``relay_overrides`` widget parser. + +These guard the failure modes flagged in the PR #178 verified review +(H1 — values were never type-checked; H2 — malformed JSON silently fell +back to upstream defaults and polluted A/B iteration logs). The parser +lives in +``forks/whatdreamscost_koolook/versions/v1_3_2/_relay_overrides.py`` — +deliberately stdlib-only so we can import it here without pulling in +ComfyUI's ``comfy_api`` runtime (which the rest of ``ltx_director.py`` +needs and which can't run inside pytest). +""" +from __future__ import annotations + +import importlib.util +import json +import logging +from pathlib import Path + +import pytest + + +# Load the parser module by file path. We avoid the normal package import +# (``from forks.whatdreamscost_koolook... import ...``) because walking +# into that package triggers ``forks/whatdreamscost_koolook/versions/ +# v1_3_2/__init__.py``, which imports ``ltx_director`` → ``comfy_api`` — +# ComfyUI-only and unavailable inside pytest. The parser module itself +# is pure stdlib + logging, so loading it in isolation is fine. +_RELAY_PATH = ( + Path(__file__).resolve().parents[2] + / "forks" + / "whatdreamscost_koolook" + / "versions" + / "v1_3_2" + / "_relay_overrides.py" +) +_spec = importlib.util.spec_from_file_location( + "whatdreamscost_v1_3_2_relay_overrides", _RELAY_PATH +) +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) + +RELAY_OVERRIDE_KEYS = _mod.RELAY_OVERRIDE_KEYS +parse_relay_overrides = _mod.parse_relay_overrides + + +# --------------------------------------------------------------------------- +# Silent fallback: only the empty/whitespace case should return None. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("blank", ["", " ", "\n", "\t \n"]) +def test_empty_or_whitespace_means_use_upstream_defaults(blank): + assert parse_relay_overrides(blank) is None + + +# --------------------------------------------------------------------------- +# Happy path: known knobs with numeric values get coerced to float. +# --------------------------------------------------------------------------- + + +def test_single_known_knob(): + out = parse_relay_overrides('{"video_strength": 10.0}') + assert out == {"video_strength": 10.0} + assert isinstance(out["video_strength"], float) + + +def test_all_known_knobs(): + payload = { + "video_strength": 2.5, + "video_window_scale": 0.7, + "audio_strength": 0.5, + "audio_window_scale": 1.2, + "audio_epsilon": 0.001, + } + assert parse_relay_overrides(json.dumps(payload)) == payload + + +@pytest.mark.parametrize( + "raw,expected", + [ + ('{"video_strength": 10}', 10.0), # int coerced to float + ('{"video_strength": "10"}', 10.0), # numeric string coerced + ('{"video_strength": "10.5"}', 10.5), # decimal string coerced + ('{"video_strength": 1e2}', 100.0), # scientific notation + ('{"video_strength": -1.5}', -1.5), # negatives flow through + ], +) +def test_numeric_values_are_coerced_to_float(raw, expected): + out = parse_relay_overrides(raw) + assert out == {"video_strength": expected} + assert isinstance(out["video_strength"], float) + + +# --------------------------------------------------------------------------- +# Comment keys: underscore-prefixed entries are silently dropped. +# --------------------------------------------------------------------------- + + +def test_underscore_prefixed_keys_are_dropped_silently(caplog): + raw = '{"_doc": "trial 3", "_owner": "daisy", "video_strength": 5}' + with caplog.at_level(logging.WARNING): + out = parse_relay_overrides(raw) + assert out == {"video_strength": 5.0} + # No warning was emitted for the comment keys. + assert "unknown keys" not in caplog.text + + +def test_only_comment_keys_returns_none(): + assert parse_relay_overrides('{"_doc": "no overrides yet"}') is None + + +# --------------------------------------------------------------------------- +# Unknown keys: warn-and-drop, don't silently misroute the operator's +# typo into the default-render bucket. +# --------------------------------------------------------------------------- + + +def test_unknown_key_logs_warning_and_drops_it(caplog): + raw = '{"viedo_strength": 10, "video_strength": 2.5}' + with caplog.at_level(logging.WARNING): + out = parse_relay_overrides(raw) + assert out == {"video_strength": 2.5} + assert "unknown keys" in caplog.text.lower() + assert "viedo_strength" in caplog.text + + +def test_only_unknown_keys_returns_none(caplog): + with caplog.at_level(logging.WARNING): + out = parse_relay_overrides('{"bogus": 1, "also_bogus": 2}') + assert out is None + assert "unknown keys" in caplog.text.lower() + + +# --------------------------------------------------------------------------- +# H2 — malformed JSON / non-object JSON must raise (not silently use +# upstream defaults, which previously polluted A/B comparisons). +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "raw", + [ + '{"video_strength":10.0', # missing closing brace + '{"video_strength": 10,}', # trailing comma + '{video_strength: 10}', # unquoted key + "not json at all", + ], +) +def test_malformed_json_raises(raw): + with pytest.raises(ValueError, match="not valid JSON"): + parse_relay_overrides(raw) + + +@pytest.mark.parametrize( + "raw", + [ + '[1, 2, 3]', # array + '"video_strength"', # string + "42", # number + "true", # bool + "null", # null + ], +) +def test_non_object_json_raises(raw): + with pytest.raises(ValueError, match="must be a JSON object"): + parse_relay_overrides(raw) + + +# --------------------------------------------------------------------------- +# H1 — a known knob with a non-numeric value must raise at parse time, +# not blow up deep inside the sampler arithmetic. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "raw", + [ + '{"video_strength": "ten"}', + '{"video_strength": null}', + '{"video_strength": [1, 2]}', + '{"video_strength": {"nested": 1}}', + '{"audio_epsilon": "very small"}', + ], +) +def test_non_numeric_known_knob_raises(raw): + with pytest.raises(ValueError, match="must be a number"): + parse_relay_overrides(raw) + + +def test_error_message_names_offending_knob_and_shows_example(): + """The error message must let the operator fix the widget without + consulting upstream Prompt-Relay source.""" + with pytest.raises(ValueError) as exc: + parse_relay_overrides('{"video_window_scale": "narrow"}') + msg = str(exc.value) + assert "video_window_scale" in msg + assert "Example:" in msg + + +# --------------------------------------------------------------------------- +# Allowlist sanity: documented keys match implementation. +# --------------------------------------------------------------------------- + + +def test_documented_knob_set_matches_implementation(): + """If anyone adds a knob in build_segments without updating the + allowlist, this test fails — preventing silent typos-disguised-as- + knobs that the H1 fix is meant to catch.""" + assert RELAY_OVERRIDE_KEYS == { + "video_strength": float, + "video_window_scale": float, + "audio_strength": float, + "audio_window_scale": float, + "audio_epsilon": float, + }