Skip to content

[Issue 101][eval-and-fix] Add SeedVR2 decode state guards (closes pollockjj/mydevelopment#194)#40

Closed
pollockjj wants to merge 11 commits into
issue_101from
issue_194
Closed

[Issue 101][eval-and-fix] Add SeedVR2 decode state guards (closes pollockjj/mydevelopment#194)#40
pollockjj wants to merge 11 commits into
issue_101from
issue_194

Conversation

@pollockjj
Copy link
Copy Markdown
Owner

Plan: Guard SeedVR2 VideoAutoencoderKLWrapper.decode against unpopulated state

Overview

comfy.ldm.seedvr.vae.VideoAutoencoderKLWrapper.decode reads self.original_image_video and self.img_dims without validation. Both are populated only by external setters (SeedVR2InputProcessing.execute and encode(orig_dims=...)), so any workflow that wires the wrapper directly without those producers crashes inside einops.rearrange or with a bare AttributeError — no SeedVR2 context. This packet adds two presence guards plus shape/arity validation at the top of decode(), plus a one-line __init__ change that initializes self.img_dims = None so the missing-state branch is reachable from a default-constructed instance. The regression module is a structural port of Comfy-Org#119's _make_standin + _PostGuardReached sentinel pattern, exercising the five reachable input shapes (4 misuse cells + 1 valid cell).

Diagnosis Summary

This is a PLAN-mode packet. The bug mechanism is fully documented in the issue body's Executive Summary and re-verified live by the Phase 0.5 probes captured in github_issues/194/probes/results/ against base SHA 9c2b9c39. No investigation slice is needed; the fix shape is dictated by CodeRabbit thread r2962219551 and the proven _make_standin precedent from #109 / Comfy-Org#119.

§Failure Signature

Five reachable cells, each captured live from the unguarded decode() body on pollockjj/ComfyUI@9c2b9c39 (comfy/ldm/seedvr/vae.py:2245-onward). Cell labels A1–A5 are referenced verbatim by AC Pre-fix-fingerprint: sub-bullets.

  • Cell A1self.original_image_video is None, self.img_dims = (H, W) valid: decode(z) raises RuntimeError('Tensor type unknown to einops <class \'NoneType\'>') from einops/_backends.py:62 (get_backend(tensor)), called transitively from comfy/ldm/seedvr/vae.py:2273 input = rearrange(self.original_image_video, "b c t h w -> (b t) c h w"). The exception message contains neither SeedVR2 nor original_image_video. Captured: github_issues/194/probes/results/gtp_03.txt (line 29).
  • Cell A2self.original_image_video is a valid 5-D tensor, self.img_dims is None (the realistic shape after the __init__ change in this packet's §Proposed Fix initializes the attribute to None on every default-constructed instance): decode(z) raises TypeError('cannot unpack non-iterable NoneType object') at comfy/ldm/seedvr/vae.py:2285 o_h, o_w = self.img_dims (Python tuple-unpacking on None). The exception class is TypeError (not RuntimeError) and the message contains neither SeedVR2 nor img_dims. Captured: live Phase 0.5 dry-run reproducing TypeError directly. (Note: prior to the __init__ change, the same misuse on a default-constructed instance raises AttributeError("'<obj>' object has no attribute 'img_dims'") at the same line; this is the __init__-less precursor shape and is the alternative pre-fix fingerprint described informally below for AC-6's source-pin context.)
  • Cell A3self.original_image_video is a 3-D tensor, self.img_dims = (H, W) valid: decode(z) raises einops.EinopsError('Error while processing rearrange-reduction pattern "b c t h w -> (b t) c h w". Input tensor shape: torch.Size([3, 4, 4]). Additional info: {}. Wrong shape: expected 5 dims. Received 3-dim tensor.') from line 2273. The exception class is einops.EinopsError (not RuntimeError) and the message contains neither SeedVR2 nor original_image_video.
  • Cell A4aself.original_image_video valid 5-D, self.img_dims = (H,) 1-tuple: decode(z) raises ValueError('not enough values to unpack (expected 2, got 1)') at line 2285 o_h, o_w = self.img_dims. The exception message contains neither SeedVR2 nor img_dims.
  • Cell A4bself.original_image_video valid 5-D, self.img_dims = (H, W, D) 3-tuple: decode(z) raises ValueError('too many values to unpack (expected 2)') at line 2285. Same defect class as A4a; same lack of SeedVR2 context.
  • Cell A5self.original_image_video valid 5-D, self.img_dims = (H, W) valid: decode(z) does NOT raise; control reaches tiled_vae(latent, self, **self.tiled_args, encode=False) at line 2259 (when enable_tiling=True) or super().decode_(latent) at line 2267 (when enable_tiling=False). This is the valid-state cell that proves guards must NOT false-positive.

§Failure Boundary

  • Triggers cells A1–A4 — any call site that invokes VideoAutoencoderKLWrapper.decode BEFORE both SeedVR2InputProcessing.execute (the producer of self.original_image_video) AND encode(orig_dims=(H, W)) (the producer of self.img_dims). Specifically: workflows that wire the wrapper as a generic VAE without the SeedVR2 input-processing node; programmatic harness code that constructs the wrapper directly and calls decode without first calling encode(orig_dims=...); test code that exercises decode in isolation.
  • Does not trigger — the canonical SeedVR2 inference path: SeedVR2InputProcessing.executeencode(orig_dims=...)decode(z). Both required attributes are populated upstream; cells A1–A4 are unreachable on this path. Cell A5 is the canonical-path shape; guards must pass control through unchanged.

§Root Cause

__init__ initializes self.original_image_video = None (line 2221) but does NOT initialize self.img_dims. decode() accesses both attributes (lines 2273, 2285) without validation. There is no presence/shape/arity guard at the top of decode(). CodeRabbit's r2962219551 review thread on Comfy-Org#11294 identified this defect class as Minor and proposed a RuntimeError guard pattern.

§Proposed Fix

Two surgical changes on comfy/ldm/seedvr/vae.py. The original plan as authored covered only original_image_video and img_dims; PR review cycle 8 expanded the fix to also cover tiled_args after Copilot review finding comment_id=3188027986 (classified ACCEPT / P1) flagged the same defect class on a third attribute. The §Proposed Fix below describes the fully shipped delta; the cycle-8 additions are also recorded under ## PR Review Scope Additions for change-history clarity.

  1. VideoAutoencoderKLWrapper.__init__ (immediately after self.original_image_video = None at line 2221) — add two new initializers:
    • self.img_dims = None so the missing-state branch (Cell A2) is reachable as a is None check on a real default-constructed instance, not only as an AttributeError on a stand-in that omits the attribute.
    • self.tiled_args = None (added cycle 8) so the missing-tiled_args branch is reachable on a default-constructed instance, parity with the other two attributes.
  2. VideoAutoencoderKLWrapper.decode (immediately after def decode(self, z): and BEFORE b, tc, h, w = z.shape) — add six guards in this order:
    • Presence guard for self.original_image_video is Noneraise RuntimeError("SeedVR2 VideoAutoencoderKLWrapper.decode: ... original_image_video is None ...").
    • Type guard for not torch.is_tensor(self.original_image_video)raise RuntimeError("... original_image_video must be a tensor ...").
    • Shape guard for self.original_image_video.ndim != 5raise RuntimeError("... original_image_video must be a 5-D tensor (b c t h w) ...").
    • Presence guard for getattr(self, 'img_dims', None) is Noneraise RuntimeError("... img_dims is None or unset ...").
    • Type guard for not isinstance(img_dims, (tuple, list))raise RuntimeError("... img_dims must be a tuple or list ...").
    • Arity guard for len(img_dims) != 2raise RuntimeError("... img_dims must be a length-2 tuple or list (H, W) ...").
    • Presence guard for getattr(self, 'tiled_args', None) is None (added cycle 8) → raise RuntimeError("... tiled_args is None or unset ...").
    • Type guard for not isinstance(tiled_args, dict) (added cycle 8) → raise RuntimeError("... tiled_args must be a dict ...").

Every guard's RuntimeError message MUST contain (a) the substring SeedVR2 and (b) the substring of the offending attribute name (original_image_video for the first three guards; img_dims for the next three; tiled_args for the cycle-8 guards). The slicer is free to adjust message text within those constraints.

§Verification Strategy

A new test module tests-unit/comfy_test/test_seedvr_vae_decode_guards.py ports the Comfy-Org#119 pattern: a _make_standin class borrows decode = comfy.ldm.seedvr.vae.VideoAutoencoderKLWrapper.decode onto a bare class; _PostGuardReached(Exception) is the sentinel raised by unittest.mock.patch.object(comfy.ldm.seedvr.vae, "tiled_vae", side_effect=_PostGuardReached(...)); an _exercise_decode helper drives the five cells. ACs are evaluated by pytest -q exit code on independent pre-fix and post-fix source checkouts; pre-fix logs MUST FAIL with the §Failure Signature cell-specific exception, post-fix logs MUST PASS with pytest.raises(RuntimeError, match=r"SeedVR2.*<attr>") succeeding.

Affected Repositories

Repository Path Base Branch Issue Branch
pollockjj/ComfyUI /home/johnj/dev_master/ComfyUI issue_101 issue_194
pollockjj/mydevelopment /home/johnj/dev_master/mydevelopment main issue_194

pollockjj/ComfyUI:issue_194 is created from origin/issue_101 HEAD 9c2b9c39; deliverable PR targets issue_101 via --deliverable-base issue_101. pollockjj/mydevelopment:issue_194 is created from origin/main; bookkeeping PR targets main and carries the plan, gate outputs, post-mortem, and committed pre/post evidence logs under github_issues/194/slice1/.

Research and Methodology

Plan Foundations Comment URL: https://github.com/pollockjj/mydevelopment/issues/194#issuecomment-4378143678

Detected Scope: none — port of the internal _make_standin + post-guard sentinel pattern established by #109 (tests-unit/comfy_test/seedvr_model_test.py) and reused by Comfy-Org#119 (tests-unit/comfy_test/test_diffusers_metadata_guard.py); the existing precedent IS the spec for how this guard's regression module is shaped, and CodeRabbit's r2962219551 suggested-fix block IS the spec for the production guard text.

The linked comment is the load-bearing artifact for every measurement, equivalence rule, canonical-protocol, tool version, and pipeline-stage citation in this plan. This plan defines no measurable output, no metric, and no equivalence rule against a published anchor; the change class is "API-misuse hygiene guard" with binary RuntimeError-and-substring assertions per AC. No AC below cites a metric, threshold, equivalence rule, or canonical protocol; therefore no AC carries a Foundations-pin: for R&M sub-check 6 (the per-AC scope is N/A across all ACs).

Tools, Pipeline, and Measurements

Plan Foundations Comment URL: https://github.com/pollockjj/mydevelopment/issues/194#issuecomment-4378143678

The linked comment's ## Tools, Pipeline, and Measurements section enumerates: (1) Existing Tooling Inventory with REUSE status for every dep (pytest, comfy.cli_args.args, unittest.mock, contextlib, comfy.ldm.seedvr.vae.tiled_vae, inspect.getsource); (2) Pipeline = single pytest invocation against tests-unit/comfy_test/test_seedvr_vae_decode_guards.py (no data pipeline; hygiene-class change); (3) Measurements = N/A (no metric, no anchor, binary assertions only); (4) Single-probe-before-sweep = N/A (5 enumerated cells, not a sweep); (5) Boundary-bracketing-before-verdict = N/A (no characterization verdict; binary observables per cell). No KNOWN-GOOD-REF rows requiring resolution. Every AC below uses pytest as the standard test runner and comfy.ldm.seedvr.vae as the unmodified-vs-modified surface under test; neither constitutes citing a foundations entry for sub-check-6 purposes per the Phase 3 schema definition (Foundations-pin: is required iff the AC text cites the foundations comment for an equivalence rule, metric definition, canonical protocol, tooling decision, or measurement specification — none of which appear in the AC bodies below). Therefore no AC carries a Foundations-pin: for TPM sub-check 6.

Ground Truth Probes

Probe captures live in github_issues/194/probes/results/. Each row pairs the verbatim probe command and the verbatim observed output against pollockjj/ComfyUI@9c2b9c39 (origin/issue_101 HEAD) via worktree at /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree. The Python interpreter for every probe is /home/johnj/dev_master/ComfyUI/.venv/bin/python (Python 3.12.13).

Probe ID Consumed by AC(s) Probe command Observed output (verbatim)
GTP-1 Slice 1 AC-1, AC-2, AC-3, AC-4, AC-5 cd /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree && python3 -c "import comfy.ldm.seedvr.vae as v; print('decode in dir:', 'decode' in dir(v.VideoAutoencoderKLWrapper)); print('decode method object:', v.VideoAutoencoderKLWrapper.decode)" decode in dir: True / decode method object: <function VideoAutoencoderKLWrapper.decode at 0x7f4d1ac5da80>
GTP-2 Slice 1 AC-6 cd /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree && python3 -c "import inspect, comfy.ldm.seedvr.vae as v; src = inspect.getsource(v.VideoAutoencoderKLWrapper.__init__); print(src); print('---'); print('self.img_dims initialized in __init__:', 'self.img_dims' in src); print('self.original_image_video initialized in __init__:', 'self.original_image_video' in src); print('self.tiled_args initialized in __init__:', 'self.tiled_args' in src)" (full init body — see gtp_02.txt) self.img_dims initialized in __init__: False / self.original_image_video initialized in __init__: True / self.tiled_args initialized in __init__: False
GTP-3 Slice 1 AC-1, AC-2, AC-3, AC-4, AC-5 cd /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree && python3 -c "import inspect, comfy.ldm.seedvr.vae as v; src = inspect.getsource(v.VideoAutoencoderKLWrapper.decode); [print(f'{i:3d}: {line}') for i, line in enumerate(src.split(chr(10)), start=1) if line.strip().startswith('def decode') or 'rearrange(self.original_image_video' in line or 'o_h, o_w = self.img_dims' in line or 'self.tiled_args.get' in line or 'tiled_vae(' in line]" 1: def decode(self, z): / 12: self.enable_tiling = self.tiled_args.get("enable_tiling", False) / 15: x = tiled_vae(latent, self, **self.tiled_args, encode=False) / 29: input = rearrange(self.original_image_video, "b c t h w -> (b t) c h w") / 41: o_h, o_w = self.img_dims
GTP-4 Slice 1 AC-5 cd /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree && python3 -c "import comfy.ldm.seedvr.vae as v; print('tiled_vae attr present:', hasattr(v, 'tiled_vae')); print('tiled_vae callable:', callable(v.tiled_vae)); print('tiled_vae object:', v.tiled_vae)" tiled_vae attr present: True / tiled_vae callable: True / tiled_vae object: <function tiled_vae at 0x7fcfbfb87420>
GTP-5 Slice 1 AC-1, AC-2, AC-3, AC-4, AC-5, AC-6 cd /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree && python3 -c "from comfy.cli_args import args; print('cpu attr present:', hasattr(args, 'cpu')); print('cpu value:', args.cpu)" cpu attr present: True / cpu value: False
GTP-6 Slice 1 (test-module shape — informational; AC-1..AC-6 inherit the precedent) ls /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree/tests-unit/comfy_test/test_diffusers_metadata_guard.py /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree/tests-unit/comfy_test/seedvr_model_test.py Both files present — see gtp_06.txt for _make_standin precedent excerpts (lines 37–42 of test_diffusers_metadata_guard.py; lines 48–59 of seedvr_model_test.py)
GTP-7 Slice 1 AC-1, AC-2, AC-3, AC-4, AC-5, AC-6 (baseline pytest harness) cd /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree && python3 -m pytest tests-unit/comfy_test/seedvr_model_test.py tests-unit/comfy_test/test_diffusers_metadata_guard.py -q 13 passed, 2 warnings in 2.36s (precedent harness baseline on 9c2b9c39) — see gtp_09.txt

Created Surface Contract

The plan introduces literals that do not exist on the base branch. These are anchored here (not in Ground Truth Probes) because they will be created by Slice 1 itself; their post-creation existence is verified by the slice's own ACs.

Created literal Created by Verified by
tests-unit/comfy_test/test_seedvr_vae_decode_guards.py (new test module) Slice 1 implementation commit AC-1..AC-6 all assert via this module's pytest invocations
_make_standin (helper in new test module) Slice 1 AC-1..AC-5 (consumed by all behavioral cells)
_PostGuardReached(Exception) (sentinel class in new test module) Slice 1 AC-5 (consumed by valid-state behavioral cell)
_exercise_decode (helper in new test module) Slice 1 AC-1..AC-5 (consumed by all behavioral cells)
test_none_original_image_video_raises_seedvr2_runtime_error (test fn) Slice 1 AC-1
test_none_img_dims_raises_seedvr2_runtime_error (test fn) Slice 1 AC-2
test_wrong_rank_original_image_video_raises_seedvr2_runtime_error (test fn) Slice 1 AC-3
test_wrong_arity_img_dims_raises_seedvr2_runtime_error (parametrized over 1-tuple and 3-tuple) Slice 1 AC-4
test_valid_state_passes_through_guards_to_tiled_vae (test fn, combined behavioral + AST source pin) Slice 1 AC-5
test_init_initializes_img_dims_to_none (test fn, AST source pin on __init__) Slice 1 AC-6
Production guard text in comfy/ldm/seedvr/vae.py VideoAutoencoderKLWrapper.decode (4 guards, each raising RuntimeError whose message contains substring SeedVR2 AND the relevant attribute name) Slice 1 implementation commit on pollockjj/ComfyUI:issue_194 AC-1..AC-5 (behavioral) + AC-5 (AST source pin component)
self.img_dims = None line in VideoAutoencoderKLWrapper.__init__ Slice 1 implementation commit on pollockjj/ComfyUI:issue_194 AC-6
Pre-fix log artifacts at github_issues/194/slice1/<test>_pre.log (one per AC) Slice 1 (slicer runs pytest against worktree at 9c2b9c39 with the new test file dropped in but the production fix absent) AC-1..AC-6 cite the file path verbatim
Post-fix log artifacts at github_issues/194/slice1/<test>_post.log (one per AC) Slice 1 (slicer runs pytest against pollockjj/ComfyUI:issue_194 with both fixes applied) AC-1..AC-6 cite the file path verbatim

Asset Readiness Inventory

Asset Location Provenance Status
comfy/ldm/seedvr/vae.py (unguarded base) pollockjj/ComfyUI@9c2b9c39:comfy/ldm/seedvr/vae.py origin/issue_101 HEAD; PR #36 merge VERIFIED — exists, opens cleanly, decode() body matches §Failure Signature cell sites at lines 2245/2273/2285 (probe GTP-3); pre-fix opaque-exception cells reproduce live in Phase 0.5 dry-runs
tests-unit/comfy_test/seedvr_model_test.py (#109 precedent) pollockjj/ComfyUI@9c2b9c39:tests-unit/comfy_test/seedvr_model_test.py merged via PR #33 VERIFIED — 8 tests pass on baseline harness run (gtp_09.txt)
tests-unit/comfy_test/test_diffusers_metadata_guard.py (Comfy-Org#119 precedent — primary structural template) pollockjj/ComfyUI@9c2b9c39:tests-unit/comfy_test/test_diffusers_metadata_guard.py merged via PR #36 VERIFIED — 5 tests pass on baseline harness run (gtp_09.txt)
Python 3.12 venv at /home/johnj/dev_master/ComfyUI/.venv Linux slot dev_master on prosoche Maintained by Boss; same venv every prior issue_101 packet ran against VERIFIED — pytest baseline 13 passed, 2 warnings in 2.36s (gtp_09.txt)
tests-unit/comfy_test/test_seedvr_vae_decode_guards.py (new test module) pollockjj/ComfyUI:issue_194:tests-unit/comfy_test/test_seedvr_vae_decode_guards.py created by Slice 1 BROKEN-EXPECTED — does not exist on base SHA (gtp_08.txt confirms No such file or directory; exit:2); created by Slice 1 implementation commit; AC-1..AC-6 verify post-creation behavior
Production guards in decode() + self.img_dims = None in __init__ pollockjj/ComfyUI:issue_194:comfy/ldm/seedvr/vae.py created by Slice 1 BROKEN-EXPECTED — guards absent on base SHA (the entire defect being fixed); created by Slice 1 implementation commit; AC-1..AC-6 verify post-fix behavior via behavioral and AST source pin
Pre-fix and post-fix evidence logs pollockjj/mydevelopment:issue_194:github_issues/194/slice1/<test>_{pre,post}.log created by Slice 1 BROKEN-EXPECTED — written by slicer during Slice 1 evidence capture; AC-1..AC-6 cite the paths and the slicer commits the logs

Slices


Slice 1: Convert opaque decode() misuse failures into SeedVR2-specific RuntimeErrors

Kind: implementation

Objective: Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged.

Acceptance Criteria

  • AC-1: cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py::test_none_original_image_video_raises_seedvr2_runtime_error exits 0 on the post-fix pollockjj/ComfyUI:issue_194 checkout AND exits non-zero on the pre-fix origin/issue_101 (9c2b9c39) worktree with the same test module dropped in (the test's pytest.raises(RuntimeError, match=r"SeedVR2.*original_image_video") matcher fails to match the unguarded einops Tensor type unknown to einops <class 'NoneType'> message) — verified by committed artifacts github_issues/194/slice1/test_none_original_image_video_pre.log AND github_issues/194/slice1/test_none_original_image_video_post.log, each containing the verbatim pytest invocation, full stdout, and final line exit code: <N>.

    • Pre-fix-fingerprint: on origin/issue_101@9c2b9c39 the unguarded decode() raises RuntimeError('Tensor type unknown to einops <class \'NoneType\'>') from einops/_backends.py:62 (called transitively from comfy/ldm/seedvr/vae.py:2273); the pytest.raises(RuntimeError, match=r"SeedVR2.*original_image_video") matcher does NOT match because the einops message contains neither SeedVR2 nor original_image_video; the test FAILS with Failed: DID NOT RAISE matching or Failed: Pattern did not match (Diagnosis Summary §Failure Signature Cell A1).
    • Post-fix-expectation: post-fix decode() raises RuntimeError whose str(exc.value) contains both substrings SeedVR2 AND original_image_video; the matcher matches; the test PASSES with exit 0.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged."
    • Probe: GTP-1, GTP-3, GTP-5, GTP-7
  • AC-2: cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py::test_none_img_dims_raises_seedvr2_runtime_error exits 0 on the post-fix pollockjj/ComfyUI:issue_194 checkout AND exits non-zero on the pre-fix origin/issue_101 (9c2b9c39) worktree (the test's pytest.raises(RuntimeError, match=r"SeedVR2.*img_dims") matcher fails to match the TypeError('cannot unpack non-iterable NoneType object') raised by Python tuple-unpacking on None) — verified by committed artifacts github_issues/194/slice1/test_none_img_dims_pre.log AND github_issues/194/slice1/test_none_img_dims_post.log, each containing the verbatim pytest invocation, full stdout, and final line exit code: <N>.

    • Pre-fix-fingerprint: on origin/issue_101@9c2b9c39 the unguarded decode() raises TypeError('cannot unpack non-iterable NoneType object') at comfy/ldm/seedvr/vae.py:2285 o_h, o_w = self.img_dims when the stand-in sets self.img_dims = None; the matcher requires RuntimeError and does NOT accept TypeError (TypeError is not a RuntimeError subclass); the test FAILS with the TypeError propagating through pytest.raises(RuntimeError) (Diagnosis Summary §Failure Signature Cell A2).
    • Post-fix-expectation: post-fix decode() raises RuntimeError whose str(exc.value) contains both substrings SeedVR2 AND img_dims; the matcher matches; the test PASSES. The test stand-in sets self.img_dims = None explicitly (the realistic shape after the §Proposed Fix __init__ initialization), so the production guard may use either self.img_dims is None or getattr(self, 'img_dims', None) is None — both expressions evaluate True on the test stand-in and the test is implementation-agnostic.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged."
    • Probe: GTP-1, GTP-3, GTP-5, GTP-7
  • AC-3: cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py::test_wrong_rank_original_image_video_raises_seedvr2_runtime_error exits 0 on the post-fix pollockjj/ComfyUI:issue_194 checkout AND exits non-zero on the pre-fix origin/issue_101 (9c2b9c39) worktree (the test's pytest.raises(RuntimeError, match=r"SeedVR2.*original_image_video") matcher fails to match the einops.EinopsError rank-mismatch message) — verified by committed artifacts github_issues/194/slice1/test_wrong_rank_original_image_video_pre.log AND github_issues/194/slice1/test_wrong_rank_original_image_video_post.log, each containing the verbatim pytest invocation, full stdout, and final line exit code: <N>.

    • Pre-fix-fingerprint: on origin/issue_101@9c2b9c39 the unguarded decode() raises einops.EinopsError('Error while processing rearrange-reduction pattern "b c t h w -> (b t) c h w". Input tensor shape: torch.Size([3, 4, 4]). Additional info: {}. Wrong shape: expected 5 dims. Received 3-dim tensor.') from comfy/ldm/seedvr/vae.py:2273; the matcher requires RuntimeError and does NOT accept einops.EinopsError; the test FAILS (Diagnosis Summary §Failure Signature Cell A3).
    • Post-fix-expectation: post-fix decode() raises RuntimeError whose str(exc.value) contains both substrings SeedVR2 AND original_image_video; the matcher matches; the test PASSES.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged."
    • Probe: GTP-1, GTP-3, GTP-5, GTP-7
  • AC-4: cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py::test_wrong_arity_img_dims_raises_seedvr2_runtime_error (parametrized over 1-tuple (H,) AND 3-tuple (H, W, D)) exits 0 on the post-fix pollockjj/ComfyUI:issue_194 checkout AND exits non-zero on the pre-fix origin/issue_101 (9c2b9c39) worktree for both parametrized cells (the test's pytest.raises(RuntimeError, match=r"SeedVR2.*img_dims") matcher fails to match either ValueError unpack message) — verified by committed artifacts github_issues/194/slice1/test_wrong_arity_img_dims_pre.log AND github_issues/194/slice1/test_wrong_arity_img_dims_post.log, each containing the verbatim pytest invocation, full stdout (including both parametrized cell IDs), and final line exit code: <N>.

    • Pre-fix-fingerprint: on origin/issue_101@9c2b9c39 the unguarded decode() raises ValueError('not enough values to unpack (expected 2, got 1)') for the 1-tuple cell and ValueError('too many values to unpack (expected 2)') for the 3-tuple cell, both at comfy/ldm/seedvr/vae.py:2285 o_h, o_w = self.img_dims; the matcher requires RuntimeError and does NOT accept ValueError; both parametrized cells FAIL (Diagnosis Summary §Failure Signature Cells A4a, A4b).
    • Post-fix-expectation: post-fix decode() raises RuntimeError whose str(exc.value) contains both substrings SeedVR2 AND img_dims for both parametrized cells; the matcher matches; both cells PASS.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged."
    • Probe: GTP-1, GTP-3, GTP-5, GTP-7
  • AC-5: cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py::test_valid_state_passes_through_guards_to_tiled_vae exits 0 on the post-fix pollockjj/ComfyUI:issue_194 checkout AND exits non-zero on the pre-fix origin/issue_101 (9c2b9c39) worktree — the test asserts BOTH (a) mock_tiled.call_count == 1 (the patched module-level comfy.ldm.seedvr.vae.tiled_vae was reached exactly once via _PostGuardReached, proving valid input flows past the guards into the post-guard region) AND (b) inspect.getsource(comfy.ldm.seedvr.vae.VideoAutoencoderKLWrapper.decode) contains the substring raise RuntimeError( at a position BEFORE the substring b, tc, h, w = z.shape (proving the guards are placed at the top of decode() ahead of the existing first body line). On the pre-fix base SHA assertion (b) FAILS because the unguarded decode() body has no raise RuntimeError( substring before b, tc, h, w = z.shape; assertion (a) would pass vacuously, but the conjunction fails → exit non-zero — verified by committed artifacts github_issues/194/slice1/test_valid_state_passes_through_guards_pre.log AND github_issues/194/slice1/test_valid_state_passes_through_guards_post.log.

    • Pre-fix-fingerprint: on origin/issue_101@9c2b9c39 inspect.getsource(VideoAutoencoderKLWrapper.decode) shows the first executable statement is b, tc, h, w = z.shape (probe GTP-3 line 1: def decode(self, z): followed by b, tc, h, w = z.shape as the immediate body); the substring raise RuntimeError( does NOT appear before b, tc, h, w = z.shape; the AST source-position assertion FAILS even though the behavioral assertion mock_tiled.call_count == 1 would pass vacuously (no guards exist to reject valid state) (Diagnosis Summary §Failure Signature Cell A5 + §Root Cause).
    • Post-fix-expectation: post-fix inspect.getsource(VideoAutoencoderKLWrapper.decode) shows raise RuntimeError( substring(s) appear BEFORE b, tc, h, w = z.shape; the AST source-position assertion PASSES; AND mock_tiled.call_count == 1 confirms valid input still reaches the post-guard region; the conjoint test PASSES.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged."
    • Probe: GTP-1, GTP-3, GTP-4, GTP-5, GTP-7
  • AC-6: cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py::test_init_initializes_img_dims_to_none exits 0 on the post-fix pollockjj/ComfyUI:issue_194 checkout AND exits non-zero on the pre-fix origin/issue_101 (9c2b9c39) worktree — the test asserts that inspect.getsource(comfy.ldm.seedvr.vae.VideoAutoencoderKLWrapper.__init__) contains the substring self.img_dims = None. On the pre-fix base SHA the substring is absent (probe GTP-2 confirms self.img_dims initialized in __init__: False); on the post-fix branch the substring is present per the §Proposed Fix one-line addition — verified by committed artifacts github_issues/194/slice1/test_init_initializes_img_dims_to_none_pre.log AND github_issues/194/slice1/test_init_initializes_img_dims_to_none_post.log.

    • Pre-fix-fingerprint: on origin/issue_101@9c2b9c39 inspect.getsource(VideoAutoencoderKLWrapper.__init__) does NOT contain the substring self.img_dims = None (probe GTP-2 captured self.img_dims initialized in __init__: False on the live module); the test's assert "self.img_dims = None" in inspect.getsource(...) FAILS (Diagnosis Summary §Root Cause).
    • Post-fix-expectation: post-fix inspect.getsource(VideoAutoencoderKLWrapper.__init__) contains the substring self.img_dims = None exactly once between the self.original_image_video = None line and super().__init__(*args, **kwargs); the assertion PASSES.
    • Assumes-passed: none
    • In-scope-of: Slice 1 Objective "Prove that VideoAutoencoderKLWrapper.decode raises a RuntimeError whose message contains both SeedVR2 and the offending attribute name (original_image_video or img_dims) for each of the four reachable misuse shapes (None, img_dims-None, wrong-rank, wrong-arity), AND that valid state still flows through the new guards into the post-guard region unchanged."
    • Probe: GTP-2, GTP-5, GTP-7

Constraints

  • Python: python3 (resolved from cd /home/johnj/dev_master/ComfyUI to the slot venv at .venv/bin/python)
  • Runner: python3 -m pytest -q tests-unit/comfy_test/test_seedvr_vae_decode_guards.py (no debug harness; no debug/run_debug.py invocation; this is a unit-test-only packet)
  • Isolation flags: N/A (no --use-process-isolation, no --disable-cuda-malloc; the test imports the wrapper class directly from comfy.ldm.seedvr.vae with comfy.cli_args.args.cpu = True set before import per Add SeedVR2 support #109/UserWarning bug ? Comfy-Org/ComfyUI#119 precedent)
  • No packages installed into host venv (every dep is REUSE — pytest, stdlib, comfy module already present on the slot venv at /home/johnj/dev_master/ComfyUI/.venv)
  • No pkill, no rm -rf, no python main.py
  • All commits on issue branch issue_194, not on base branches: deliverable commit on pollockjj/ComfyUI:issue_194 (created from origin/issue_101@9c2b9c39); bookkeeping commit on pollockjj/mydevelopment:issue_194 (created from origin/main)
  • Relevant Existing Tooling:
  • Evidence Primitive Requirements:
    • Behavioral-change claims (AC-1..AC-5 behavioral facets) require independent pre-fix and post-fix source checkouts: pre-fix from a temporary worktree at origin/issue_101@9c2b9c39 with the new test module dropped in but the production fix absent; post-fix from pollockjj/ComfyUI:issue_194 with both fixes applied. Each AC commits both <test>_pre.log and <test>_post.log to github_issues/194/slice1/.
    • Source-pin claims (AC-5 AST facet, AC-6) require inspect.getsource substring assertions captured by the same pytest invocation; the same pre/post artifact pair satisfies the requirement (no separate evidence file needed).
    • No corpus claims (no manifest required).
    • No sweep claims (no single-probe artifact required; the 5 input-shape cells are an enumerated set, not a sweep).
    • No characterization verdict (no boundary-bracketing PASS/FAIL samples required).
  • Repository Hygiene:
    • Every dirty repo state is resolved by the slicer/agent without git stash and without external decision deferral.
    • Dirty paths are classified as committed work, precise committed .gitignore rules for safe generated bulk, or discarded generated debris.
    • Clean committed work already present on the active branch is repo provenance and is integrated, not removed as dirty-state cleanup. The issue_194 branch on both repos was created by the orchestrator before this plan was authored; the slicer integrates the pre-existing branch state.
    • Slicer and Melian must run live git status --short gates locally on /home/johnj/dev_master/ComfyUI AND /home/johnj/dev_master/mydevelopment AND any temporary worktree before submission, and must refuse to submit while any touched repo is dirty.
    • No AC requires repo_hygiene.txt, raw git status --short transcripts, or CLEAN markers as QA evidence.
    • QA verifies submitted commits, branch refs, and artifact presence through GitHub.
    • The temporary probe worktree at /home/johnj/dev_master/mydevelopment/github_issues/194/probe_worktree is removed by the slicer before submission via git worktree remove; presence of this worktree at submission is treated as dirty state.

Out of Scope

  • VideoAutoencoderKL.decode_() (the parent class's decode) — the unguarded parent method is not modified by this packet; only the wrapper's decode() gains guards. CodeRabbit thread r2962219551 flags only the wrapper.
  • The tiled_vae branch and lab_color_transfer post-guard processing — out-of-scope per the issue body's ## Non-Goals. The fix adds guards at the TOP of decode() only; downstream behavior is unmodified.
  • VideoAutoencoderKLWrapper.encode() validation — the producer-side counterpart of the same defect class (encode accepts an unvalidated orig_dims argument and stores it without shape/arity check). Tracked as a separate future issue if surfaced; this packet does not pre-empt it.
  • self.tiled_args initialization in __init__ — originally out-of-scope per issue body Non-Goals (the tiled_vae branch is excluded), but brought into scope by PR review cycle 8 in response to Copilot review finding comment_id=3188027986 (classified ACCEPT / P1). See ## PR Review Scope Additions below.
  • Dtype, positive-integer-range, or other deeper validation beyond rank-and-arity. Deferred per issue body; only the four specific shapes flagged by CodeRabbit are guarded.
  • Every non-VAE class in comfy/ldm/seedvr/vae.py — VAE-only scope per issue body Non-Goals.
  • Cross-platform Windows verification leg — the change is pure Python with no platform-specific code paths (no torch backend selection, no file-system primitives, no thread/process primitives); existing Add SeedVR2 support #109/UserWarning bug ? Comfy-Org/ComfyUI#119 precedent is Linux-only and no Windows verification was required for those CodeRabbit-driven hygiene packets either. If a future packet introduces a platform-divergence risk, the Windows leg is added then.
  • Multi-slice decomposition — this is a single-slice packet because the production change (4 guards + 1 init line) and the regression module (1 file with 6 tests) are an indivisible unit of evidence; splitting the slice would require AC artifacts on a partially-fixed tree (e.g., guards present but __init__ change absent), which has no diagnostic value distinct from the combined fix.

PR Review Scope Additions

During PR review, the scope was expanded beyond the original plan to address a related missing-state defect surfaced by Copilot review.

Cycle 8 — self.tiled_args initialization and decode-guard expansion

Copilot review finding comment_id=3188027986 on PR #37 (classified ACCEPT / P1) flagged that decode() unconditionally calls self.tiled_args.get("enable_tiling", False) but __init__ did not initialise the attribute. A default-constructed wrapper (or any instance not populated by SeedVR2InputProcessing.execute) therefore raised an opaque AttributeError: 'VideoAutoencoderKLWrapper' object has no attribute 'tiled_args' after passing the original_image_video / img_dims guards — the same defect class the original packet was guarding against, just a different attribute. The fix added (commit e6f68371 "pr review cycle 8: address findings"):

  • self.tiled_args = None in VideoAutoencoderKLWrapper.__init__ (alongside the original self.original_image_video = None and the new self.img_dims = None).
  • A presence guard (tiled_args is None) and a dict-type guard (not isinstance(tiled_args, dict)) at the top of decode() ahead of the .get() call, each raising RuntimeError whose message contains SeedVR2 and tiled_args.
  • Three additional regression-test cells in tests-unit/comfy_test/test_seedvr_vae_decode_guards.py:
    • test_init_initializes_tiled_args_to_none (AC9 — source-pin on __init__).
    • test_none_tiled_args_raises_seedvr2_runtime_error (AC10 — parametrized over set_tiled_args=True/False so both the explicit-None and attribute-unset cells are covered).
    • test_non_dict_tiled_args_raises_seedvr2_runtime_error (AC11 — parametrized over non-dict types str / int / list / tuple).

This addition brings the PR description into alignment with the shipped code per Copilot review finding comment_id=3188204623 (cycle 13). The original ## Out of Scope bullet for self.tiled_args reflected the original plan as authored; this addendum supersedes it. No other ## Out of Scope items were affected.


Tracked in pollockjj/mydevelopment Comfy-Org#194 (bookkeeping PR pollockjj/mydevelopment#202).

pollockjj added 11 commits May 5, 2026 05:07
…lformed state (closes pollockjj/mydevelopment#194)

VideoAutoencoderKLWrapper.decode() previously accessed
self.original_image_video and self.img_dims without validation, raising
opaque torch errors deep in rearrange() / attribute unpacking when a
workflow wired the wrapper directly without SeedVR2InputProcessing
populating those attributes. CodeRabbit thread r2962219551 on upstream
PR Comfy-Org#11294 flagged this as Minor.

Fix:

  1. __init__: initialise self.img_dims = None so the missing-state
     branch is reachable from a default-constructed instance (matches
     the pre-existing self.original_image_video = None initialiser).

  2. decode() top: four ordered guards raising RuntimeError with
     SeedVR2-specific messages — presence of original_image_video,
     5-D rank check on original_image_video, presence of img_dims,
     2-arity check on img_dims. Each message names the offending
     attribute and includes "SeedVR2" so misuse converts to an
     actionable signal instead of an opaque torch failure.

Regression cover: tests-unit/comfy_test/test_seedvr_vae_decode_guards.py
(7 cells across 5 AC functions) using the __new__ + nn.Module.__init__
standin pattern from Comfy-Org#189 and the _PostGuardReached halt-point sentinel
pattern from Comfy-Org#119; AC5 patches VideoAutoencoderKL.decode_ to confirm the
guards passed and control crossed into the post-guard region without
standing up the heavy decode pipeline.
…ct node ids + add AC6

Slice 1 rework for pollockjj/mydevelopment#194: align test node ids with
the qa-slice contract and add AC5/AC6 source-position invariants.

- AC1-AC4: rename the four wrapper-failure tests to
  test_none_original_image_video_raises_seedvr2_runtime_error,
  test_none_img_dims_raises_seedvr2_runtime_error,
  test_wrong_rank_original_image_video_raises_seedvr2_runtime_error,
  test_wrong_arity_img_dims_raises_seedvr2_runtime_error and switch each
  to pytest.raises(RuntimeError, match=r"SeedVR2.*<attr>") so the regex
  matcher both pins the message shape and discriminates the wrong
  exception class on a pre-fix worktree.
- AC5: replace the post-guard sentinel with the contract spelling —
  test_valid_state_passes_through_guards_to_tiled_vae now patches the
  module-level comfy.ldm.seedvr.vae.tiled_vae, asserts mock_tiled.call_count
  == 1 (proves valid input flowed past the guards into the post-guard
  region) and asserts inspect.getsource(VideoAutoencoderKLWrapper.decode)
  contains "raise RuntimeError(" before "b, tc, h, w = z.shape" (proves
  the guards are placed at the top of decode() ahead of the existing
  first body line).
- AC6: add test_init_initializes_img_dims_to_none asserting
  inspect.getsource(VideoAutoencoderKLWrapper.__init__) contains the
  literal "self.img_dims = None" so the missing-state branch is reachable
  from a default-constructed instance — a substring the unguarded base
  SHA does not carry.
- Add a _bypass_super_decode autouse-style fixture for AC1-AC4 that
  patches VideoAutoencoderKL.decode_ to return a synthesised 5-D tensor.
  Without it, the standin (built via __new__ to skip the heavy real
  __init__) lacks parent-class attributes like use_slicing, so on a
  pre-fix worktree the unguarded decode() body would die with an
  AttributeError before reaching the rearrange(self.original_image_video,
  ...) / o_h, o_w = self.img_dims sites the ACs target. With the fixture
  in place, pre-fix runs surface the contract-described einops /
  TypeError / ValueError / AttributeError fingerprints; post-fix runs
  short-circuit at the guard raises and never invoke the stub.
…ract

Copilot review-round-5 comment_id=3187927157 (load-bearing path):
the arity-failure message at comfy/ldm/seedvr/vae.py L2282 said
'must be a 2-tuple (H, W)' while the type guard one frame above
(L2274) explicitly accepts both tuples and lists. Re-wording the
arity message to 'must be a length-2 tuple or list (H, W)' brings
it into agreement with the type-guard contract so a workflow
passing a length-2 list does not see a misleading '2-tuple' error.

No behaviour change: the regex matchers in
test_wrong_arity_img_dims_raises_seedvr2_runtime_error
(SeedVR2.*img_dims) are unaffected; full module re-runs 15/15
green on the post-fix branch.
…essing.execute

Copilot review-round-7 comment_id=3187988689 (load-bearing path):
the missing-img_dims RuntimeError at comfy/ldm/seedvr/vae.py L2271
told users `img_dims` "must be populated by encode(orig_dims=...)",
but the primary in-repo producer is SeedVR2InputProcessing.execute
(comfy_extras/nodes_seedvr.py:255,265,273) which sets
`vae_model.img_dims = [o_h, o_w]` directly without going through
encode(orig_dims=...). The misleading guidance would send a user
debugging this guard down the wrong call path. Re-wording to name
SeedVR2InputProcessing.execute first (with encode(orig_dims=...) as
secondary) brings the message into agreement with the
already-mentioned `original_image_video is None` RuntimeError above
and the actual primary call path.

No behaviour change: regex matchers in
test_none_img_dims_raises_seedvr2_runtime_error and
test_non_sized_img_dims_raises_seedvr2_runtime_error use
`SeedVR2.*img_dims` and are unaffected; full module re-runs 15/15
green on the post-fix branch.
Copilot AI review requested due to automatic review settings May 5, 2026 15:33
@qa-agent-seveneves
Copy link
Copy Markdown

Codex Review -- Round 1 -- KICKOFF

Runner: scripts/run_codex_review.py
PR: #40
Reviewer model: codex review (configured codex CLI default)
Kickoff timestamp (UTC): 2026-05-05T15:35:56Z

Prompt sent to codex review

Review the latest state of https://github.com/pollockjj/ComfyUI/pull/40. Output findings using strict P0-P3 priority tags ([P0], [P1], [P2], [P3]) at the start of each finding title. Surface P0/P1/P2 -- these block merge in this repo. Surface P3 with that tag explicitly. Do not surface style nits, formatting, typos, or pre-existing bugs. End your output with a single line -- one of: 'APPROVE' (no P0/P1/P2 found), 'REVIEW CONDUCTED AND NO ISSUES FOUND' (no P0/P1/P2 found, equivalent to APPROVE for verdict-tracking purposes; use this if you are reluctant to emit a bare APPROVE token), or 'REQUEST_CHANGES' (one or more P0/P1/P2 found). Output nothing else after that line.

Awaiting codex output. The result will be posted as a separate comment on this PR when codex exits.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds explicit SeedVR2 state validation to VideoAutoencoderKLWrapper.decode() so miswired workflows fail fast with SeedVR2-context RuntimeErrors (instead of opaque einops/AttributeError/TypeError failures), and introduces regression tests that pin the guard behavior (including the round-8 tiled_args scope addition).

Changes:

  • Initialize VideoAutoencoderKLWrapper.img_dims and VideoAutoencoderKLWrapper.tiled_args to None in __init__, making “missing state” detectable on default-constructed instances.
  • Add top-of-decode() guards for original_image_video, img_dims, and tiled_args (presence + type + shape/arity), and use validated locals (img_dims, tiled_args) for subsequent reads.
  • Add a new unit test module covering valid/misuse cells and update an existing “no mutate” test to accept either self.tiled_args.get(...) or tiled_args.get(...).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
comfy/ldm/seedvr/vae.py Adds __init__ defaults and decode() state/type/shape guards; switches downstream reads to validated locals.
tests-unit/comfy_test/test_seedvr_vae_decode_guards.py New regression suite pinning the guard behavior and placement (including tiled_args cases).
tests-unit/comfy_test/test_seedvr_vae_tiled_args_no_mutate.py Adjusts regex to allow .get(...) on either self.tiled_args or validated local tiled_args.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pollockjj
Copy link
Copy Markdown
Owner Author

Closed — operator instruction not followed correctly during regeneration.

@pollockjj pollockjj closed this May 5, 2026
@qa-agent-seveneves
Copy link
Copy Markdown

Codex Review -- Round 1 -- RESULT

PR: #40
Reviewer model: codex review (configured codex CLI default)
Result timestamp (UTC): 2026-05-05T15:38:51Z
Codex exit code: 0
Duration (s): 172.6
Detected verdict: UNKNOWN

Prompt sent to codex review

Review the latest state of https://github.com/pollockjj/ComfyUI/pull/40. Output findings using strict P0-P3 priority tags ([P0], [P1], [P2], [P3]) at the start of each finding title. Surface P0/P1/P2 -- these block merge in this repo. Surface P3 with that tag explicitly. Do not surface style nits, formatting, typos, or pre-existing bugs. End your output with a single line -- one of: 'APPROVE' (no P0/P1/P2 found), 'REVIEW CONDUCTED AND NO ISSUES FOUND' (no P0/P1/P2 found, equivalent to APPROVE for verdict-tracking purposes; use this if you are reluctant to emit a bare APPROVE token), or 'REQUEST_CHANGES' (one or more P0/P1/P2 found). Output nothing else after that line.

Verbatim codex review output

This PR was reviewed using codex review with the prompt above and produced the following output:


No merge-blocking defects were identified in the latest PR diff.


Codex runner stderr (non-evidence)

OpenAI Codex v0.128.0 (research preview)
--------
workdir: /home/johnj/dev_master/mydevelopment
model: gpt-5.5
provider: openai
approval: never
sandbox: workspace-write [workdir, /tmp, /home/johnj/.codex/memories]
reasoning effort: high
reasoning summaries: none
session id: 019df8c8-253b-77d0-b029-52a434106c68
--------
user
Review the latest state of https://github.com/pollockjj/ComfyUI/pull/40. Output findings using strict P0-P3 priority tags ([P0], [P1], [P2], [P3]) at the start of each finding title. Surface P0/P1/P2 -- these block merge in this repo. Surface P3 with that tag explicitly. Do not surface style nits, formatting, typos, or pre-existing bugs. End your output with a single line -- one of: 'APPROVE' (no P0/P1/P2 found), 'REVIEW CONDUCTED AND NO ISSUES FOUND' (no P0/P1/P2 found, equivalent to APPROVE for verdict-tracking purposes; use this if you are reluctant to emit a bare APPROVE token), or 'REQUEST_CHANGES' (one or more P0/P1/P2 found). Output nothing else after that line.
exec
/bin/bash -lc 'cat /home/johnj/.codex/skills/pr-review/SKILL.md' in /home/johnj/dev_master/mydevelopment
 succeeded in 0ms:
---
name: pr-review
description: "Iterative codex + Copilot PR review loop, slice-LLM driven. ACTIVATE when: (1) user says 'review the PR', 'kick off PR review', '/pr-review', 'disposition the PR'; (2) a PR has just been opened or had a new commit pushed to its head. Fires codex review (P0-P3 priority tags + verdict line), fires Copilot review via add-reviewer, READS the full Copilot inline-comment array (never trusts the review-body summary), decides every finding REQUIRED / NOT-REQUIRED / INVALID, applies REQUIRED fixes, posts INVALID rationale on the comment thread, then loops until codex P0-P2 list is empty AND no remaining findings about newly-changed code. Codex P0/P1/P2 findings are NEVER allowed to be classified INVALID."
---

# pr-review — codex + Copilot iterative review loop

**Role.** You own the PR. You decide every finding. You apply fixes. You reply to disagreements. The user does not adjudicate review findings — you do.

## Reviewers and what they post

| Reviewer | How fired | What it posts |
|---|---|---|
| **codex** | `python scripts/run_codex_review.py {OWNER/REPO} {PR_NUMBER} {ISSUE_NUMBER} {ROUND}` | Two PR thread comments per round (kickoff + result) as `qa-agent-seveneves[bot]`. Result body has findings tagged `[P0]`/`[P1]`/`[P2]`/`[P3]` and ends with one of: `APPROVE`, `REVIEW CONDUCTED AND NO ISSUES FOUND` (verdict-equivalent to APPROVE), or `REQUEST_CHANGES`. |
| **Copilot** | Fire: `gh pr edit {PR_NUMBER} --repo {OWNER/REPO} --add-reviewer copilot-pull-request-reviewer`. Monitor: `python scripts/run_copilot_poll.py {OWNER/REPO} {PR_NUMBER} {ISSUE_NUMBER} {ROUND} {HEAD_SHA}`. | One review (state COMMENTED) at the current head SHA, plus zero-or-more **inline comments** attached to specific file lines. The review-body summary is unreliable about inline counts. |

## The ONLY way to read Copilot's inline comments

`scripts/run_copilot_poll.py` is the runner. It polls until Copilot's review at the captured `HEAD_SHA` lands, then fetches the inline comments **scoped to that specific review** (via `/pulls/{pr}/reviews/{id}/comments` — review-scoped, not the PR-wide `/pulls/{pr}/comments` endpoint, so cycle N+1 only surfaces what THAT review added). It writes evidence to:

github_issues/{ISSUE_NUMBER}/pr_review_outputs/round{ROUND}_copilot_review.md
github_issues/{ISSUE_NUMBER}/pr_review_outputs/round{ROUND}_copilot_inline.json


and prints `REVIEW_ID`, `REVIEW_STATE`, `INLINE_COUNT`, `EVIDENCE_REVIEW`, `EVIDENCE_INLINE` lines on success.

**Always read `EVIDENCE_INLINE` after the runner exits 0.** Walk every entry: `id`, `path`, `line`, `body`, `in_reply_to_id`. Do NOT report "no actionable inline findings" based on `gh pr view --json latestReviews` body summary — that summary lies (it can say "generated 2 comments" while the body field is empty). The runner's inline-JSON file is the only authoritative source.

## Decision categories — every finding gets one

| Verdict | Meaning | Action |
|---|---|---|
| **REQUIRED** | Real defect or risk inside the PR's intended scope. | Apply the fix. Commit on the PR head branch. Push. |
| **NOT-REQUIRED** | Real, but optional / cosmetic / or genuinely outside the PR's intended scope. | Do nothing. On Copilot cycle #0, no reply needed. On cycle #1+, only worry if it concerns code touched in the most recent fix cycle. |
| **INVALID** | Reviewer is wrong: misunderstands the code, asserts a non-existent behavior, suggests a change that would break a stated invariant, or proposes a fix that defeats the test it claims to strengthen. | Post an inline reply on the comment thread with the technical rationale. Do not apply the change. |

**Hard rule:** A codex `[P0]` / `[P1]` / `[P2]` finding may NOT be classified INVALID. P0/P1/P2 are by definition merge-blocking — if you believe codex is wrong, the burden is on you to either (a) reclassify yourself privately and accept the risk by marking NOT-REQUIRED with explicit written rationale on the PR thread, or (b) apply a fix. You do not get to silently dismiss a codex P0/P1/P2.

Codex `[P3]` findings may be REQUIRED, NOT-REQUIRED, or INVALID.

## Posting an INVALID reply

gh api -X POST /repos/{OWNER/REPO}/pulls/{PR_NUMBER}/comments
-F in_reply_to={comment_id}
-f body="<REJECT rationale: name the false premise; cite the language/library semantics; cite the stated invariant>"


The reply must explain WHY the reviewer is wrong, not just that you disagree. If the reviewer is misunderstanding Python/git/HTTP/etc., cite the spec or stdlib doc. If they're proposing a change that would defeat the test's purpose, name the invariant the test guards.

## Cycle structure

A **cycle** = one round of (codex + Copilot) at the current head SHA, followed by your dispositioning of every finding, followed by any pushes you make.

- **Cycle #0**: the first time both reviewers run on this PR. Every finding warrants classification. NOT-REQUIRED Copilot findings get no reply on cycle #0.
- **Cycle #1+** (re-reviews after you pushed fixes): each Copilot re-review is a **fresh scan** of the entire PR — it will surface new findings, including ones about pre-existing code it didn't mention before. **You are only required to disposition cycle #1+ findings if they concern code that was changed in the most recent fix cycle.** Pre-existing-code findings Copilot just noticed may be ignored. (You did not introduce them in this round; they were tacit-NOT-REQUIRED on cycle #0.)

## The loop

cycle = 0
while True:
1. Capture current head SHA.
2. Fire codex review (round = cycle + 1):
python scripts/run_codex_review.py {OWNER/REPO} {PR} {ISSUE} {cycle+1}
Wait for the result comment to land.
3. Fire Copilot:
gh pr edit {PR} --repo {OWNER/REPO} --add-reviewer copilot-pull-request-reviewer
4. Monitor Copilot via the runner (blocks until review at HEAD lands):
python scripts/run_copilot_poll.py {OWNER/REPO} {PR} {ISSUE} {cycle+1} {HEAD_SHA}
The runner writes evidence files and prints REVIEW_ID / REVIEW_STATE /
INLINE_COUNT / EVIDENCE_REVIEW / EVIDENCE_INLINE on stdout.
5. Read codex result body. Parse [P0]/[P1]/[P2]/[P3] findings.
6. Read EVIDENCE_INLINE (the JSON file written by the Copilot runner).
Walk every entry. The PR-wide /pulls/{PR}/comments endpoint is
allowed as a cross-check but the runner output is canonical.
7. Disposition every finding REQUIRED / NOT-REQUIRED / INVALID per the rules above.
(Cycle #1+: skip dispositioning Copilot findings unrelated to most-recent-fix code.)
8. For each REQUIRED: apply fix, commit on PR head branch.
For each INVALID: post inline reply with rationale.
9. If you committed any fixes in step 8: push to PR head branch.
10. Termination check — break when ALL hold:
a. codex has no [P0]/[P1]/[P2] findings
b. codex has no actionable [P3] (any P3 not classified INVALID/NOT-REQUIRED)
c. on the LATEST Copilot review, no findings concern code changed in the most recent f

...truncated 43720 bytes (full stderr in round1_codex_stderr.txt)...

'210,290p' comfy_extras/nodes_seedvr.py" in /home/johnj/dev_master/ComfyUI
succeeded in 0ms:
vae_model = vae.first_stage_model
scale = 0.9152
shift = 0
if images.dim() != 5: # add the t dim
images = images.unsqueeze(0)
images = images.permute(0, 1, 4, 2, 3)

    b, t, c, h, w = images.shape
    images = images.reshape(b * t, c, h, w)

    clip = Lambda(lambda x: torch.clamp(x, 0.0, 1.0))
    normalize = Normalize(0.5, 0.5)
    images = side_resize(images, resolution)

    images = clip(images)
    o_h, o_w = images.shape[-2:]
    images = div_pad(images, (16, 16))
    images = normalize(images)
    _, _, new_h, new_w = images.shape

    images = images.reshape(b, t, c, new_h, new_w)
    # Per #188: capture UNPADDED tensor before cut_videos for the B==1 trim path.
    # For B>1, store the PADDED tensor instead — VideoAutoencoderKLWrapper.decode()
    # passes `input` (derived from original_image_video) to lab_color_transfer, and
    # the decoded `x` carries B*T_padded frames; matching that with B*T_padded `input`
    # preserves the pre-#188 pairing semantics until #189 fixes the B>1 axis derivation.
    images_bcthw_unpadded = rearrange(images, "b t c h w -> b c t h w")
    images = cut_videos(images)
    images_bcthw = rearrange(images, "b t c h w -> b c t h w")
    images_bthwc = rearrange(images, "b t c h w -> b t h w c")
    original_image_video = images_bcthw_unpadded if b == 1 else images_bcthw

    # in case users a non-compatiable number for tiling
    def make_divisible(val, divisor):
        return max(divisor, round(val / divisor) * divisor)

    spatial_tile_size = make_divisible(spatial_tile_size, 32)
    spatial_overlap = make_divisible(spatial_overlap, 32)

    if spatial_overlap >= spatial_tile_size:
        spatial_overlap = max(0, spatial_tile_size - 8)

    args = {"tile_size": (spatial_tile_size, spatial_tile_size), "tile_overlap": (spatial_overlap, spatial_overlap),
            "temporal_size":temporal_tile_size}
    if enable_tiling:
        vae_model.img_dims = [o_h, o_w]
        vae_model.original_image_video = original_image_video
        latent = vae.encode_tiled(
            images_bthwc,
            tile_x=spatial_tile_size,
            tile_y=spatial_tile_size,
            overlap=spatial_overlap,
            tile_t=temporal_tile_size,
        )
    else:
        vae_model.img_dims = [o_h, o_w]
        vae_model.original_image_video = original_image_video
        latent = vae.encode(images_bthwc)

    clear_vae_memory(vae_model)
    #images = images.to(offload_device)
    #vae_model = vae_model.to(offload_device)

    vae_model.img_dims = [o_h, o_w]
    args["enable_tiling"] = enable_tiling
    vae_model.tiled_args = args
    vae_model.original_image_video = original_image_video

    latent = latent.unsqueeze(2) if latent.ndim == 4 else latent
    latent = rearrange(latent, "b c ... -> b ... c")

    latent = (latent - shift) * scale

    return io.NodeOutput({"samples": latent})

class SeedVR2Conditioning(io.ComfyNode):
@classmethod
def define_schema(cls):
return io.Schema(
node_id="SeedVR2Conditioning",
category="image/video",

exec
/bin/bash -lc 'cat tests-unit/comfy_test/test_seedvr_vae_tiled_args_no_mutate.py' in /home/johnj/dev_master/ComfyUI
succeeded in 0ms:
import re
from pathlib import Path

def test_seedvr_vae_tiled_args_uses_get_not_pop():
path = Path(file).resolve().parents[2] / "comfy" / "ldm" / "seedvr" / "vae.py"
src = path.read_text(encoding="utf-8")
assert not re.search(r"(?:self.)?tiled_args.pop\s*(", src), (
f"VideoAutoencoderKLWrapper.decode contains tiled_args.pop(...) which mutates tiled_args across calls; "
f"expected reads via .get(...) only per the upstream fix in Comfy-Org#11294 commit 3b418da. "
f"Source path: {path}"
)
enable_tiling_get_calls = re.findall(
r"self.tiled_args.get\s*(\s*["']enable_tiling["']\s*,\sFalse\s)",
src,
)
assert len(enable_tiling_get_calls) == 1, (
f"VideoAutoencoderKLWrapper.decode should contain exactly one "
f"self.tiled_args.get('enable_tiling', False) call per Slice 1 baseline; "
f"found {len(enable_tiling_get_calls)}. Source path: {path}"
)

mcp: codex_apps/github_list_pull_request_review_threads started
mcp: codex_apps/github_list_pull_request_review_threads (completed)
exec
/bin/bash -lc "sed -n '1,80p' tests-unit/comfy_test/test_diffusers_metadata_guard.py" in /home/johnj/dev_master/ComfyUI
succeeded in 0ms:
"""Regression tests for the diffusers-format guard inside comfy.sd.VAE.__init__.

Tracks pollockjj/mydevelopment#119. The guard previously indexed
metadata["keep_diffusers_format"] directly, raising KeyError when
metadata was non-None but lacked that key. The merged fix on
pollockjj/ComfyUI:issue_101 uses
metadata.get("keep_diffusers_format") != "true": a missing key flows
through to convert_vae_state_dict; the explicit "true" value bypasses it.

Five cells exercise every reachable shape of the guard input — missing key,
explicit "true", None, explicit non-"true", empty dict — and halt
the constructor at the first post-guard call (model_management.is_amd).
_make_standin borrows __init__ onto a bare class, mirroring
seedvr_model_test.py::_make_standin (#109). _exercise_guard single-
sources the patched-constructor harness so the cells stay synchronised.
"""

from comfy.cli_args import args
import torch

if not torch.cuda.is_available():
args.cpu = True

import contextlib # noqa: E402
import unittest.mock # noqa: E402

import comfy.sd # noqa: E402

_DIFFUSERS_TRIGGER_KEY = "decoder.up_blocks.0.resnets.0.norm1.weight"

class _PostGuardReached(Exception):
"""Sentinel raised by the patched is_amd to halt __init__ at the first post-guard statement."""

def _make_standin():
class _StandIn:
init = comfy.sd.VAE.init

return _StandIn

def _exercise_guard(metadata):
"""Drive VAE.__init__ with the diffusers trigger key and the supplied
metadata; halt at is_amd. Returns (mock_convert, mock_is_amd)
for branch (call_count) + reach (called) assertions per cell.
"""
StandIn = _make_standin()
sd = {_DIFFUSERS_TRIGGER_KEY: torch.zeros(1)}

with unittest.mock.patch.object(
    comfy.sd.diffusers_convert,
    "convert_vae_state_dict",
    autospec=True,
    side_effect=lambda state_dict: state_dict,
) as mock_convert, unittest.mock.patch.object(
    comfy.sd.model_management,
    "is_amd",
    autospec=True,
    side_effect=_PostGuardReached("post-guard reached"),
) as mock_is_amd:
    with contextlib.suppress(_PostGuardReached):
        StandIn(sd=sd, metadata=metadata)

return mock_convert, mock_is_amd

def test_diffusers_guard_invokes_convert_when_metadata_missing_key():
"""AC1: metadata is non-None but lacks keep_diffusers_format → convert is invoked."""
mock_convert, mock_is_amd = _exercise_guard({"unrelated_key": "value"})

assert mock_convert.call_count == 1
assert mock_is_amd.called

def test_diffusers_guard_skips_convert_when_metadata_pins_keep_true():
"""AC2: metadata pins keep_diffusers_format == "true" → convert is skipped."""
mock_convert, mock_is_amd = _exercise_guard({"keep_diffusers_format": "true"})

exec
/bin/bash -lc 'grep -n "tiled_args" comfy/ldm/seedvr/vae.py' in /home/johnj/dev_master/ComfyUI
succeeded in 0ms:
2284: self.enable_tiling = self.tiled_args.get("enable_tiling", False)
2287: x = tiled_vae(latent, self, **self.tiled_args, encode=False)

codex
No merge-blocking defects were identified in the latest PR diff.
2026-05-05T15:38:50.979571Z ERROR codex_core::session: failed to record rollout items: thread 019df8c8-2553-7f52-946b-fe1049b1b868 not found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants