fix(review): harden relay_overrides parser + polish (PR #178 review)
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
54c035afd0
commit
3ffb7b75aa
+14
-3
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user