[Issue 101][eval-and-fix] Guard diffusers metadata lookup (closes pollockjj/mydevelopment#119)#36
Conversation
…get binary regression Pin the CodeRabbit Critical fix at comfy/sd.py:443-446 with two AC-mapped binary cells in tests-unit/comfy_test/vae_diffusers_format_guard_test.py: - AC1 key-missing: state dict carries the diffusers-format trigger key and metadata is non-None but lacks 'keep_diffusers_format'. The fixed guard must call convert_vae_state_dict exactly once and not raise KeyError. - AC2 key-present-"true": metadata['keep_diffusers_format'] == 'true'. The guard must skip conversion entirely. Source thread: Comfy-Org#11294 r2959796358. Zero LOC of production code change in this packet — the safe .get form is already merged from upstream into pollockjj/ComfyUI:issue_101 (HEAD 8d5cfce).
QA gate slice 1 contract requires nodeids test_diffusers_guard_invokes_convert_when_metadata_missing_key (AC1) and test_diffusers_guard_skips_convert_when_metadata_pins_keep_true (AC2) under tests-unit/comfy_test/test_diffusers_metadata_guard.py. The previously committed cells under vae_diffusers_format_guard_test.py exercise the same guard but use names and a shape the gate's verification command does not target, so the contract paths returned 404 and exit code 4. This module adds the contract-shaped cells (unittest.mock.patch.object on comfy.sd.diffusers_convert. convert_vae_state_dict with autospec, _make_standin binding comfy.sd.VAE.__init__, contextlib.suppress around the post-guard fallthrough, and a binary mock_convert.call_count assertion) at the contract's exact path; vae_diffusers_format_guard_test.py is retained unchanged as the manifest lists both files.
Codex Review — Cycle 1Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
This PR adds regression coverage around the comfy.sd.VAE.__init__ diffusers-format metadata guard so future changes do not reintroduce the metadata["keep_diffusers_format"] KeyError path.
Changes:
- Adds a regression test module that verifies conversion is invoked when
metadatalackskeep_diffusers_format. - Adds a regression test module that verifies conversion is skipped when
metadata["keep_diffusers_format"] == "true". - Introduces a second overlapping test module covering the same guard through a different test shape.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests-unit/comfy_test/vae_diffusers_format_guard_test.py |
Adds guard regression tests by instantiating VAE with a synthetic diffusers-format state dict. |
tests-unit/comfy_test/test_diffusers_metadata_guard.py |
Adds guard regression tests using a stand-in class bound to VAE.__init__ and mocked conversion calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Regression tests for the VAE diffusers-format guard at comfy/sd.py:443-446. | ||
|
|
||
| Tracks pollockjj/mydevelopment#119 (parent #101). The guard previously | ||
| indexed ``metadata["keep_diffusers_format"]`` directly, which raised | ||
| ``KeyError`` when ``metadata`` was non-``None`` but lacked that key — | ||
| the common case for any safetensors VAE with ordinary metadata. CodeRabbit | ||
| flagged this as Critical on Comfy-Org/ComfyUI#11294 (thread r2959796358). | ||
| The merged fix on ``pollockjj/ComfyUI:issue_101`` uses | ||
| ``metadata.get("keep_diffusers_format") != "true"`` so a missing key | ||
| flows through to invoke ``convert_vae_state_dict``, while the | ||
| explicit ``"true"`` value skips the conversion. | ||
|
|
||
| The guard is inline in ``VAE.__init__`` rather than a separate method, | ||
| so the ``_make_standin`` precedent from #109's | ||
| ``seedvr_model_test.py`` (which borrows ``NaDiT`` methods onto a small | ||
| ``torch.nn.Module`` subclass) does not apply directly. Instead we drive | ||
| the guard by calling ``VAE.__init__`` with a synthetic state dict that | ||
| contains only the diffusers-format trigger key | ||
| (``decoder.up_blocks.0.resnets.0.norm1.weight``). The trigger key | ||
| satisfies the outer ``if`` of the guard but matches no other elif | ||
| branch in the post-guard model-detection chain, so ``__init__`` falls | ||
| through to the ``else: logging.warning(...); self.first_stage_model = | ||
| None; return`` path at ``comfy/sd.py:860-863``. ``convert_vae_state_dict`` | ||
| is monkey-patched to a tracker rather than allowed to execute on the | ||
| synthetic dict, since the production routine assumes a real diffusers | ||
| VAE state dict. | ||
| """ | ||
|
|
Codex Review — Cycle 2Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with contextlib.suppress(Exception): | ||
| StandIn(sd=sd, metadata=metadata) |
| with contextlib.suppress(Exception): | ||
| StandIn(sd=sd, metadata=metadata) |
| import comfy.sd # noqa: E402 | ||
|
|
||
|
|
||
| _DIFFUSERS_TRIGGER_KEY = "decoder.up_blocks.0.resnets.0.norm1.weight" |
Codex Review — Cycle 3Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codex Review — Cycle 4Overall: patch is incorrect No findings. |
Codex Review — Cycle 5Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with unittest.mock.patch.object( | ||
| comfy.sd.diffusers_convert, | ||
| "convert_vae_state_dict", | ||
| autospec=True, |
| ) as mock_is_amd: | ||
| with contextlib.suppress(Exception): | ||
| StandIn(sd=sd, metadata=metadata) | ||
|
|
||
| assert mock_convert.call_count == 0 |
| ) as mock_is_amd: | ||
| with contextlib.suppress(Exception): | ||
| StandIn(sd=sd, metadata=metadata) | ||
|
|
||
| assert mock_convert.call_count == 1 | ||
| assert mock_is_amd.called |
Codex Review — Cycle 6Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with unittest.mock.patch.object( | ||
| comfy.sd.diffusers_convert, | ||
| "convert_vae_state_dict", | ||
| autospec=True, | ||
| side_effect=lambda state_dict: state_dict, |
| * ``unittest.mock.patch.object(comfy.sd.model_management, "is_amd", | ||
| autospec=True, side_effect=_PostGuardReached(...))`` — patches | ||
| the FIRST function called after the guard at ``comfy/sd.py:448`` | ||
| (``if model_management.is_amd():``). Raising a dedicated sentinel | ||
| exception from this mock halts ``__init__`` immediately past the |
Codex Review — Cycle 7Overall: patch is incorrect No findings. |
Codex Review — Cycle 27Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_diffusers_guard_invokes_convert_when_metadata_is_empty_dict(): | ||
| """AC5: state dict carries the diffusers-format trigger key and | ||
| ``metadata`` is the empty dict ``{}``. The guard's second disjunct | ||
| (``metadata.get("keep_diffusers_format") != "true"``) must evaluate | ||
| to ``True`` because ``{}.get("keep_diffusers_format")`` returns | ||
| ``None`` and ``None != "true"`` is ``True``, driving the conversion | ||
| branch and invoking ``convert_vae_state_dict`` exactly once. The | ||
| empty dict is a real production input: ``comfy.utils.convert_old_quants`` | ||
| (``comfy/utils.py`` line 1359-1360) normalizes ``None`` to ``{}`` | ||
| before ``load_state_dict_guess_config`` constructs the VAE at | ||
| ``comfy/sd.py`` line 1739 (``vae = VAE(sd=vae_sd, metadata=metadata)``). | ||
| AC1 covers the truthy missing-key shape ``{"unrelated_key": "value"}`` | ||
| but does not pin the falsy missing-key shape: a regression of the | ||
| shape ``if metadata is None or (metadata and metadata.get(...) != "true"):`` | ||
| short-circuits the ``and`` on the empty dict and skips the | ||
| conversion, breaking the ``convert_old_quants``-normalized callers | ||
| while keeping AC1, AC2, AC3 and AC4 green (Copilot review feedback | ||
| on PR pollockjj/ComfyUI#36, comment r3185375054). The branch | ||
| witness is ``mock_convert.call_count == 1``; ``mock_is_amd.called`` | ||
| is the paired post-guard reach witness; the narrow | ||
| ``suppress(_PostGuardReached)`` keeps any other regression loud. | ||
| """ | ||
| mock_convert, mock_is_amd = _exercise_guard({}) |
| ``None``-normalized empty dict, or pass a non-``"true"`` | ||
| ``keep_diffusers_format`` value. | ||
|
|
||
| All four cells share an ``_exercise_guard`` helper so the |
Codex Review — Cycle 28Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codex Review — Cycle 29Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codex Review — Cycle 30Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ``comfy.utils.convert_old_quants`` (``comfy/utils.py`` line 1359-1360) | ||
| normalizes ``None`` to ``{}`` before ``load_state_dict_guess_config`` | ||
| constructs the VAE at ``comfy/sd.py`` line 1739 | ||
| (``vae = VAE(sd=vae_sd, metadata=metadata)``). AC1 covers the truthy |
| (``comfy/utils.py`` line 1359-1360) normalizes ``None`` to ``{}`` | ||
| before ``load_state_dict_guess_config`` constructs the VAE at | ||
| ``comfy/sd.py`` line 1739 (``vae = VAE(sd=vae_sd, metadata=metadata)``). | ||
| AC1 covers the truthy missing-key shape ``{"unrelated_key": "value"}`` | ||
| but does not pin the falsy missing-key shape: a regression of the | ||
| shape ``if metadata is None or (metadata and metadata.get(...) != "true"):`` |
Codex Review — Cycle 31Overall: patch is incorrect No findings. |
Codex Review — Cycle 32Overall: patch is incorrect No findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reduces the test module from 310 LOC to 106 LOC by collapsing the prose carried in across 31 PR-review cycles. No test logic changes: helpers, mock wiring, halt point, and all 5 AC cells (key-missing, keep-true, None, keep-false, empty-dict) are unchanged. Pytest: 5 passed in 2.20s on the prosoche dev slot venv. Removed: the multi-paragraph module-docstring narrative that recapped the slice-rework history and cited Copilot comment IDs (which rot when threads are deleted); the per-test docstrings that re-explained the helper contract already documented at `_exercise_guard`; and the 8-line sentinel-class docstring that justified its own existence. Kept: a tight module docstring naming the bug, the merged fix, the 5 input shapes covered, and the precedent link to seedvr_model_test.py; a one-sentence docstring per cell stating the input shape and the expected branch outcome.
Plan: Lock-in regression for the
comfy/sd.pydiffusers-formatmetadataguardOverview
The unsafe
metadata["keep_diffusers_format"]indexing inVAE.__init__(CodeRabbitr2959796358, Critical) is already corrected tometadata.get("keep_diffusers_format")onpollockjj/ComfyUI:issue_101(SHA7936e591) atcomfy/sd.py:444. This plan adds one regression-only test module totests-unit/comfy_test/, with five binary AC cells that pin all five input shapes the live guard'sor-disjunct must resolve: AC1 (truthy missing-key dict →convert_vae_state_dictinvoked), AC2 (key-present-"true"→ invocation skipped), AC3 (metadata is None→ invoked, additive completeness added in round 8), AC4 (key-present-"false"→ invoked, additive completeness added in round 8), AC5 (metadata = {}empty-dict → invoked, additive completeness added in round 26). Zero LOC of production code change.Round 8 Scope Expansion
The deliverable was extended in round 8 of PR review (Copilot inline finding
r3185061776on PR #36, ACCEPTED ingithub_issues/119/pr_review_round_8/copilot_decisions.json) from the issue body's two-cell binary AC contract (AC1 key-missing, AC2 key-present-"true") to a four-cell completeness contract that pins four of the five input shapes the live guard's disjunct must resolve. AC3 (metadata is None) and AC4 (key-present-"false") are strictly additive: zero edits to AC1 / AC2 nodeids, literals, or assertions; new test functions appended after AC2; the_exercise_guardhelper handles all four cells. The expansion locks the guard against the regression shapeif metadata and metadata.get(...) is None:which would keep AC1+AC2 green while breaking real callers that omitmetadataor pass a non-"true"value (full regression-coverage witness ingithub_issues/119/pr_review_round_8/triage_notes.md). PR body updated in round 9 (Copilot inline findingr3185101996ACCEPTED,github_issues/119/pr_review_round_9/copilot_decisions.json) to align the description and slice contract with the four-cell deliverable on the wire at that time.Round 26 Scope Expansion
The deliverable was further extended in round 26 of PR review (Copilot inline finding
r3185375054on PR #36, ACCEPTED ingithub_issues/119/pr_review_round_26/copilot_decisions.json) from the round-8 four-cell contract to the current five-cell completeness contract that pins all five input shapes the live guard'sor-disjunct must resolve. AC5 (metadata = {}empty-dict) is strictly additive: zero edits to AC1 / AC2 / AC3 / AC4 nodeids, literals, or assertions; new test function appended after AC4; the_exercise_guardhelper handles all five cells. The expansion locks the guard against the regression shapeif metadata is None or (metadata and metadata.get("keep_diffusers_format") != "true"):which would keep AC1+AC2+AC3+AC4 green while breaking real callers that pass an empty dict — a real production input shape produced bycomfy.utils.convert_old_quants(comfy/utils.py:1359-1360) which normalizesNoneto{}beforeload_state_dict_guess_configconstructs the VAE atcomfy/sd.py:1739(vae = VAE(sd=vae_sd, metadata=metadata)). The empty-dict short-circuits the regression'sandtoFalsebecausebool({})isFalse, while AC1's truthy missing-key mapping{"unrelated_key": "value"}evaluates toTrueand does not pin this falsy missing-key path. Full regression-coverage witness ingithub_issues/119/pr_review_round_26/triage_notes.md. PR body updated in round 27 (Copilot inline findingsr3185413283andr3185413305ACCEPTED,github_issues/119/pr_review_round_27/copilot_decisions.json) to align the description and slice contract with the five-cell deliverable now on the wire.Diagnosis Summary
Pre-merge state on
pollockjj/ComfyUI:issue_101carriedif metadata is None or metadata["keep_diffusers_format"] != "true":at thecomfy/sd.py:444guard. Whenmetadatawas a non-Nonedict that did not contain thekeep_diffusers_formatkey — the common case for any safetensors VAE with ordinary metadata — the indexing raisedKeyError("keep_diffusers_format")BEFOREdiffusers_convert.convert_vae_state_dict(sd)could rewrite the state dict, breaking otherwise-valid diffusers-format VAEs. The CodeRabbit-suggested fix swapped the unsafe[…]for.get(…), restoring the intended two-branch behavior: (1) callconvert_vae_state_dictwhenmetadataisNone, lacks the key, or has the key but pinned to a value other than the literal string"true"; (2) skip the conversion only when the caller has explicitly pinnedkeep_diffusers_format = "true". The fix landed via upstream Comfy-Org#11294 follow-up and is currently live onissue_101. This packet is regression-only: it commits the test module that would have caught the originalKeyErrorand that locks the symmetric skip-branch against future regression.Failure Signature
metadata["keep_diffusers_format"] != "true"raisesKeyError: 'keep_diffusers_format'when invoked withmetadata = {"any_other_key": "any_value"}— observable as an unhandledKeyErrortraceback rooted atcomfy/sd.pyline of the unsafe indexing.Reproduction Path
issue_101; this regression test packet asserts the post-merge.get()form via patched-mock observation. The deliverable repo's CI suite would surface a regression to the unsafe form as an AC1 test failure.Failure Boundary
if metadata is None or …:predicate atcomfy/sd.py:444. The outerif not is_seedvr2_vae and 'decoder.up_blocks.0.resnets.0.norm1.weight' in sd.keys():(line 443) and the SeedVR2 short-circuit onis_seedvr2_vae(line 442) are out of scope; AC1, AC2, AC3, AC4, and AC5 all feed only the diffusers trigger key (line 443) and never the SeedVR2 indicator (line 442) to keep the test pinned to the targeted guard.Proposed Fix
comfy/sd.py:444onissue_101SHA7936e591. This plan's deliverable is one new test module attests-unit/comfy_test/test_diffusers_metadata_guard.pyexercising the live form via patchedunittest.mock.Verification Strategy
comfy.sd.diffusers_convert.convert_vae_state_dictandcomfy.sd.model_management.is_amdvia the_exercise_guardhelper, invoke a_make_standin-boundVAE.__init__withsd={trigger_key: torch.zeros(1)}, and assert onmock_convert.call_countplusmock_is_amd.called(post-guard reach witness). AC1 invokes withmetadata={"unrelated_key":"value"}and assertsmock_convert.call_count == 1; with the pre-merge unsafe indexing, the call would raiseKeyErrorbefore reaching the patched call, leavingcall_count == 0and failing AC1. AC2 invokes withmetadata={"keep_diffusers_format":"true"}and assertsmock_convert.call_count == 0, locking the symmetric skip path; it would FAIL under a regression that removes the inner guard entirely (always-call form) producingcall_count == 1. AC3 invokes withmetadata=Noneand assertsmock_convert.call_count == 1, pinning the first disjunct (metadata is None); it would FAIL under the round-8 Copilot regression shapeif metadata and metadata.get(...) is None:which evaluatesNone and X = Noneand skips the conversion. AC4 invokes withmetadata={"keep_diffusers_format":"false"}and assertsmock_convert.call_count == 1, pinning the second disjunct against any non-"true"value; it would FAIL under the same round-8 regression shape which evaluates"false" is None = Falseand skips the conversion. AC5 invokes withmetadata={}and assertsmock_convert.call_count == 1, pinning the falsy missing-key path that real callers produce viaconvert_old_quants'sNone→{}normalization; it would FAIL under the round-26 Copilot regression shapeif metadata is None or (metadata and metadata.get(...) != "true"):which short-circuits the empty dict'sandtoFalseand skips the conversion.Affected Repositories
pollockjj/ComfyUI/home/johnj/dev_master/ComfyUIissue_101issue_119pollockjj/mydevelopment/home/johnj/dev_master/mydevelopmentmainissue_119pollockjj/ComfyUI:issue_119carries the deliverable test module;pollockjj/mydevelopment:issue_119carries the bookkeeping plan, gate outputs, evidence logs, and post-mortem. The deliverable PR targetsissue_101(per--deliverable-base issue_101melian flag); the bookkeeping PR targetsmain.Research and Methodology
Plan Foundations Comment URL: https://github.com/pollockjj/mydevelopment/issues/119#issuecomment-4374620437
Detected Scope:
none— Regression-only test packet for an already-applied upstream fix atcomfy/sd.py:444onpollockjj/ComfyUI:issue_101; mirrors the_make_standinprecedent established by #109 intests-unit/comfy_test/seedvr_model_test.py:48-58with byte-identical pattern.The linked comment is the load-bearing artifact for the equivalence rule (
behavioral-mock-callcount, proposed seed entry), the binding parent-issue directives (CodeRabbitr2959796358thread on Comfy-Org#11294 and the issue body's two-AC contract — extended to four AC cells per round-8 Copilot reviewr3185061776per the Round 8 Scope Expansion section above, then to five AC cells per round-26 Copilot reviewr3185375054per the Round 26 Scope Expansion section above), the internal-surface citations (comfy/sd.py:440-446onissue_101,comfy/utils.py:1359-1360andcomfy/sd.py:1697,1739onissue_101for the AC5 production-input path, andtests-unit/comfy_test/seedvr_model_test.py:48-58onissue_109), and the I/O contract for the binary integer measurement. Every AC below that names a tool, file path, function symbol, or measurement traces by quote or live probe to either that comment or the## Ground Truth Probessection.Tools, Pipeline, and Measurements
Plan Foundations Comment URL: https://github.com/pollockjj/mydevelopment/issues/119#issuecomment-4374620437
The linked comment's
## Tools, Pipeline, and Measurementssection enumerates: (1) Existing Tooling Inventory (pytest,unittest.mock,torch,comfy.cli_args— allREUSEwith citations); (2) Pipeline stages from pytest invocation to mock-call-count assertion; (3) Measurements table (binary integer assertions, one per AC — two ACs at plan-foundations time, four after round-8 expansion per the Round 8 Scope Expansion section above, five after round-26 expansion per the Round 26 Scope Expansion section above); (4) HR-06 declared not applicable (no sweep); (5) HR-11 declared not applicable (no characterization verdict). The five ACs below trace each tool, pipeline stage, and measurement to that section.Ground Truth Probes
git -C /home/johnj/dev_master/ComfyUI show issue_101:comfy/sd.py | sed -n '440,446p'git -C /home/johnj/dev_master/ComfyUI show issue_109:tests-unit/comfy_test/seedvr_model_test.py | sed -n '48,58p'cd /home/johnj/dev_master/ComfyUI && .venv/bin/python -c "import comfy.cli_args as c; c.args.cpu = True; import comfy.sd, inspect; print(list(inspect.signature(comfy.sd.VAE.__init__).parameters))"['self', 'sd', 'device', 'config', 'dtype', 'metadata']cd /home/johnj/dev_master/ComfyUI && .venv/bin/python -c "import comfy.cli_args as c; c.args.cpu = True; import comfy.sd as sd; import comfy.diffusers_convert; print('same_module:', sd.diffusers_convert is comfy.diffusers_convert)"same_module: Truecd /home/johnj/dev_master/ComfyUI && .venv/bin/python -c "import inspect, comfy.cli_args as c; c.args.cpu = True; import comfy.sd as sd; print(list(inspect.signature(sd.diffusers_convert.convert_vae_state_dict).parameters))"['vae_state_dict']git -C /home/johnj/dev_master/ComfyUI grep -n "decoder.up_blocks.0.resnets.0.norm1.weight" issue_101 -- comfy/sd.pyissue_101:comfy/sd.py:443: if not is_seedvr2_vae and 'decoder.up_blocks.0.resnets.0.norm1.weight' in sd.keys(): #diffusers formatgit -C /home/johnj/dev_master/ComfyUI grep -n "decoder.up_blocks.2.upsamplers.0.upscale_conv.weight" issue_101 -- comfy/sd.py | head -1issue_101:comfy/sd.py:442: is_seedvr2_vae = "decoder.up_blocks.2.upsamplers.0.upscale_conv.weight" in sdgit -C /home/johnj/dev_master/ComfyUI show issue_101:pytest.inigit -C /home/johnj/dev_master/ComfyUI show issue_101:comfy/sd.py | grep -n "from . import diffusers_convert"34:from . import diffusers_convertAsset Readiness Inventory
comfy/sd.pysource carrying the safe-.get()form atcomfy/sd.py:444pollockjj/ComfyUI:issue_101SHA7936e591, blob pathcomfy/sd.pylines 440–446r2959796358);git log issue_101 --oneline -10shows the merge commit at HEADgit show issue_101:comfy/sd.py)_make_standinprecedent filepollockjj/ComfyUI:issue_109SHA4cdaf5a5, blob pathtests-unit/comfy_test/seedvr_model_test.pylines 48–58git ls-tree issue_109 -- tests-unit/comfy_test/, the file is onissue_109only and does not exist onissue_101git show issue_109:tests-unit/comfy_test/seedvr_model_test.py); the new test onissue_119ports the pattern (does not import the file)pytest.ini,tests-unit/comfy_test/directory)pollockjj/ComfyUI:issue_101repo root +tests-unit/comfy_test/git show issue_101:pytest.ini)No external model weights, datasets, calibration corpora, or third-party services are required. The packet operates entirely on the in-tree source surface and stdlib (
unittest.mock,contextlib) plus the repo-pinnedpytestandtorch.Created Surface Contract
tests-unit/comfy_test/test_diffusers_metadata_guard.py(new test module onpollockjj/ComfyUI:issue_119)test_diffusers_guard_invokes_convert_when_metadata_missing_key(AC1, in the new module)test_diffusers_guard_skips_convert_when_metadata_pins_keep_true(AC2, in the new module)test_diffusers_guard_invokes_convert_when_metadata_is_none(AC3, in the new module; additive completeness added round 8 per Copilotr3185061776)test_diffusers_guard_invokes_convert_when_metadata_pins_keep_false(AC4, in the new module; additive completeness added round 8 per Copilotr3185061776)test_diffusers_guard_invokes_convert_when_metadata_is_empty_dict(AC5, in the new module; additive completeness added round 26 per Copilotr3185375054)_make_standin()(this packet's local port of the #109 pattern; bindscomfy.sd.VAE.__init__onto a tiny standin class so the production__init__body executes in its own module namespace without instantiating a fullVAE)_exercise_guard(metadata)(single-source harness shared by AC1, AC2, AC3, AC4, AC5; patchescomfy.sd.diffusers_convert.convert_vae_state_dictvia passthrough lambda andcomfy.sd.model_management.is_amdvia_PostGuardReachedsentinel; returns(mock_convert, mock_is_amd)for per-cell paired assertions)_PostGuardReached(raised by the patchedis_amdto haltVAE.__init__at the first call past the guard; caught by narrowcontextlib.suppress(_PostGuardReached)in_exercise_guard)_DIFFUSERS_TRIGGER_KEY = "decoder.up_blocks.0.resnets.0.norm1.weight"(in the new module; mirrors the literal atcomfy/sd.py:443per GTP-6)github_issues/119/slice1/test_AC1_key_missing.log(committed onpollockjj/mydevelopment:issue_119)github_issues/119/slice1/test_AC2_key_present_true.log(committed onpollockjj/mydevelopment:issue_119)github_issues/119/slice1/test_AC3_metadata_none.log(committed onpollockjj/mydevelopment:issue_119; round-8 additive completeness)github_issues/119/slice1/test_AC4_keep_false.log(committed onpollockjj/mydevelopment:issue_119; round-8 additive completeness)github_issues/119/slice1/test_AC5_metadata_empty_dict.log(committed onpollockjj/mydevelopment:issue_119; round-26 additive completeness)Slices
Slice 1: Lock the diffusers-format
metadataguard via five-cell regression testKind: implementation
Objective: Prove that the live
comfy/sd.py:443-446two-branch guard correctly resolves all five input shapes itsor-disjunct must handle: AC1 (metadatanon-Nonetruthy missing-key dict →convert_vae_state_dictinvoked); AC2 (metadatanon-None, key="true"→ invocation skipped); AC3 (metadata is None→ invoked, additive completeness from round 8 per Copilotr3185061776); AC4 (metadatanon-None, key="false"→ invoked, additive completeness from round 8 per Copilotr3185061776); AC5 (metadata = {}empty-dict → invoked, additive completeness from round 26 per Copilotr3185375054). All five cells exercise the guard via patchedunittest.mockobservation under a_make_standinstandin class that binds the productionVAE.__init__without instantiating a fullVAE, sharing the_exercise_guardhelper so the patched-converter / patched-is_amd/_PostGuardReached/ narrow-suppressmachinery is single-sourced.Acceptance Criteria
AC-1:
cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_diffusers_metadata_guard.py::test_diffusers_guard_invokes_convert_when_metadata_missing_keyexits 0; the test patchescomfy.sd.diffusers_convert.convert_vae_state_dictwithunittest.mock.patch.object(..., autospec=True), invokes a_make_standin-boundcomfy.sd.VAE.__init__withsd = {"decoder.up_blocks.0.resnets.0.norm1.weight": torch.zeros(1)}andmetadata = {"unrelated_key": "value"}undercontextlib.suppress(Exception), and assertsmock_convert.call_count == 1— verified by committed artifactgithub_issues/119/slice1/test_AC1_key_missing.log(onpollockjj/mydevelopment:issue_119) containing the pytest stdout line matching^1 passed(or^.* 1 passed.*$) for that nodeid and exit code 0.metadata["keep_diffusers_format"] != "true"(CodeRabbitr2959796358thread on feat: Add support For SeedVR2 (CORE-6) Comfy-Org/ComfyUI#11294, before the upstream fix landed onissue_101SHA7936e591), the same invocation raisedKeyError: 'keep_diffusers_format'BEFORE reachingdiffusers_convert.convert_vae_state_dict(sd); the patched mock therefore recordedcall_count == 0, andassert mock_convert.call_count == 1would have FAILED. (Diagnosis Summary §"Failure Signature".)comfy/sd.py:444formmetadata.get("keep_diffusers_format") != "true"onissue_101SHA7936e591, noKeyErroris raised; the innerifevaluatesTrue;diffusers_convert.convert_vae_state_dict(sd)is invoked exactly once before the suppressed downstream cascade;mock_convert.call_count == 1.AC-2:
cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_diffusers_metadata_guard.py::test_diffusers_guard_skips_convert_when_metadata_pins_keep_trueexits 0; the test patchescomfy.sd.diffusers_convert.convert_vae_state_dictwithunittest.mock.patch.object(..., autospec=True), invokes the same_make_standin-boundcomfy.sd.VAE.__init__withsd = {"decoder.up_blocks.0.resnets.0.norm1.weight": torch.zeros(1)}andmetadata = {"keep_diffusers_format": "true"}undercontextlib.suppress(Exception), and assertsmock_convert.call_count == 0— verified by committed artifactgithub_issues/119/slice1/test_AC2_key_present_true.log(onpollockjj/mydevelopment:issue_119) containing the pytest stdout line matching^1 passed(or^.* 1 passed.*$) for that nodeid and exit code 0.if metadata is None or metadata.get("keep_diffusers_format") != "true":predicate atcomfy/sd.py:444and unconditionally executessd = diffusers_convert.convert_vae_state_dict(sd)on the diffusers-format trigger branch (an "always-call" wrong correction class) would producemock_convert.call_count == 1for the key-present-"true"case, violating the explicit-skip contract documented atcomfy/sd.py:443-446and the issue body AC2. (Diagnosis Summary §"Failure Boundary".) Note that the originally-flaggedKeyErrorbug shape did NOT itself break this branch — formetadata == {"keep_diffusers_format":"true"}the unsafe indexing succeeded — so AC2's diagnostic value is regression-prevention against the wrong-correction class above, locking the skip branch for the future.comfy/sd.py:443-446two-branch guard onissue_101SHA7936e591, the inner predicate evaluatesFalse(metadatais notNone;metadata.get("keep_diffusers_format")returns"true";"true" != "true"isFalse;None or FalseisFalse);diffusers_convert.convert_vae_state_dict(sd)is NOT invoked;mock_convert.call_count == 0."true"skip branch).AC-3:
cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_diffusers_metadata_guard.py::test_diffusers_guard_invokes_convert_when_metadata_is_noneexits 0; the test invokes_exercise_guard(metadata=None)(helper definition above), which patchescomfy.sd.diffusers_convert.convert_vae_state_dictandcomfy.sd.model_management.is_amdviaunittest.mock.patch.object(..., autospec=True), drives a_make_standin-boundcomfy.sd.VAE.__init__withsd = {"decoder.up_blocks.0.resnets.0.norm1.weight": torch.zeros(1)}andmetadata = Noneundercontextlib.suppress(_PostGuardReached), and assertsmock_convert.call_count == 1andmock_is_amd.called— verified by committed artifactgithub_issues/119/slice1/test_AC3_metadata_none.log(onpollockjj/mydevelopment:issue_119) containing the pytest stdout line matching^1 passed(or^.* 1 passed.*$) for that nodeid and exit code 0.if metadata and metadata.get("keep_diffusers_format") is None:atcomfy/sd.py:444(the canonical wrong-correction class Copilot called out inr3185061776) evaluates the left operand toNone(falsy) onmetadata=Noneinput, short-circuits the conjunction toNone, and SKIPS the conversion —mock_convert.call_count == 0for the AC3 input, violating the live guard's first-disjunct contractmetadata is None → convert. Real callers that omitmetadata(the common case for any safetensors VAE without the explicit pin) would silently lose the diffusers-format rewrite under that regression.comfy/sd.py:444formif metadata is None or metadata.get("keep_diffusers_format") != "true":, the first disjunct short-circuits toTrueonmetadata=Noneinput;diffusers_convert.convert_vae_state_dict(sd)is invoked exactly once before the patchedis_amdraises_PostGuardReachedto halt the constructor at the first call past the guard;mock_convert.call_count == 1,mock_is_amd.called == True.metadata is Nonedisjunct, additive completeness from round 8).r3185061776(PR [Issue 101][eval-and-fix] Guard diffusers metadata lookup (closes pollockjj/mydevelopment#119) #36); ACCEPT decision committed ingithub_issues/119/pr_review_round_8/copilot_decisions.json.AC-4:
cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_diffusers_metadata_guard.py::test_diffusers_guard_invokes_convert_when_metadata_pins_keep_falseexits 0; the test invokes_exercise_guard(metadata={"keep_diffusers_format": "false"}), drives the same_make_standin-boundcomfy.sd.VAE.__init__under the same patchedconvert_vae_state_dict/ patchedis_amd/contextlib.suppress(_PostGuardReached)machinery, and assertsmock_convert.call_count == 1andmock_is_amd.called— verified by committed artifactgithub_issues/119/slice1/test_AC4_keep_false.log(onpollockjj/mydevelopment:issue_119) containing the pytest stdout line matching^1 passed(or^.* 1 passed.*$) for that nodeid and exit code 0.if metadata and metadata.get("keep_diffusers_format") is None:evaluatesTrue and ("false" is None)toTrue and False=Falseon AC4 input, SKIPS the conversion, and producesmock_convert.call_count == 0— violating the live guard's second-disjunct contractmetadata.get(key) != "true" → convert. Real callers that pass an explicit non-"true"value (e.g., a YAML config that pinskeep_diffusers_format: false) would silently lose the rewrite under that regression.comfy/sd.py:444form, the first disjunct evaluatesFalse(metadatais notNone); the second disjunct evaluates"false" != "true"=True;False or True=True;diffusers_convert.convert_vae_state_dict(sd)is invoked exactly once before the patchedis_amdraises_PostGuardReached;mock_convert.call_count == 1,mock_is_amd.called == True."true"disjunct, additive completeness from round 8).r3185061776(PR [Issue 101][eval-and-fix] Guard diffusers metadata lookup (closes pollockjj/mydevelopment#119) #36); ACCEPT decision committed ingithub_issues/119/pr_review_round_8/copilot_decisions.json.AC-5:
cd /home/johnj/dev_master/ComfyUI && python3 -m pytest -q tests-unit/comfy_test/test_diffusers_metadata_guard.py::test_diffusers_guard_invokes_convert_when_metadata_is_empty_dictexits 0; the test invokes_exercise_guard(metadata={}), drives the same_make_standin-boundcomfy.sd.VAE.__init__under the same patchedconvert_vae_state_dict/ patchedis_amd/contextlib.suppress(_PostGuardReached)machinery, and assertsmock_convert.call_count == 1andmock_is_amd.called— verified by committed artifactgithub_issues/119/slice1/test_AC5_metadata_empty_dict.log(onpollockjj/mydevelopment:issue_119) containing the pytest stdout line matching^1 passed(or^.* 1 passed.*$) for that nodeid and exit code 0.if metadata is None or (metadata and metadata.get("keep_diffusers_format") != "true"):short-circuits the innerandonmetadata={}becausebool({})isFalse, evaluates the entire predicate toNone or False=False, SKIPS the conversion, and producesmock_convert.call_count == 0— violating the live guard's second-disjunct contractmetadata.get(key) != "true" → convertfor the falsy missing-key shape that real callers produce. The empty-dict shape is reachable in production viacomfy.utils.convert_old_quants(comfy/utils.py:1359-1360) which normalizesNoneto{}beforeload_state_dict_guess_configconstructs the VAE atcomfy/sd.py:1739(vae = VAE(sd=vae_sd, metadata=metadata)); a caller ofload_state_dict_guess_config(sd, metadata=None)flows through that normalization toVAE(sd=vae_sd, metadata={})and would silently lose the rewrite under the round-26 regression. AC1's truthy missing-key mapping{"unrelated_key": "value"}does not pin this path becausebool({"unrelated_key": "value"})isTrueand the regression'sanddoes not short-circuit on it.comfy/sd.py:444form, the first disjunct evaluatesFalse(metadatais notNone); the second disjunct evaluates{}.get("keep_diffusers_format")=NoneandNone != "true"=True;False or True=True;diffusers_convert.convert_vae_state_dict(sd)is invoked exactly once before the patchedis_amdraises_PostGuardReached;mock_convert.call_count == 1,mock_is_amd.called == True.convert_old_quantsNone→{}normalization, additive completeness from round 26).r3185375054(PR [Issue 101][eval-and-fix] Guard diffusers metadata lookup (closes pollockjj/mydevelopment#119) #36); ACCEPT decision committed ingithub_issues/119/pr_review_round_26/copilot_decisions.json.Constraints
python3(the deliverable slot's.venv/bin/pythonresolves via the slot's pytest invocation; never hardcode a host-specific absolute interpreter path).python3 -m pytestinvoked fromcd /home/johnj/dev_master/ComfyUI(deliverable clone); evidence logs land onpollockjj/mydevelopment:issue_119undergithub_issues/119/slice1/.pkill, norm -rf, nopython main.py.issue_119of each repo, not on base branches (pollockjj/ComfyUI:issue_101andpollockjj/mydevelopment:mainare read-only PR targets for this packet).pytest(REUSE) — repo-pinned viapollockjj/ComfyUI:issue_101tests-unit/requirements.txt; invoked viapython3 -m pytestperpytest.ini(pythonpath = .,testpaths = tests tests-unit).unittest.mock(REUSE) — stdlib; precedent intests-unit/comfy_test/folder_path_test.py:6(from unittest.mock import patch).torch(REUSE) — repo-pinned viapollockjj/ComfyUI:issue_101requirements.txt; precedent for CPU-mode test bootstrap intests-unit/comfy_test/seedvr_model_test.py:33-37.comfy.cli_args(REUSE) — in-tree; precedent forargs.cpu = Truetest bootstrap intests-unit/comfy_test/seedvr_model_test.py:33-37._make_standinpattern (KNOWN-GOOD-REF) — established inpollockjj/ComfyUI:issue_109tests-unit/comfy_test/seedvr_model_test.py:48-58; this packet ports the pattern (does not import it; the precedent file does not exist onissue_101, only onissue_109).pollockjj/mydevelopment:issue_119atgithub_issues/119/slice1/test_AC{1,2,3,4,5}_*.logshowing1 passedand exit code 0 for the named nodeid.mock_convert.call_countinteger;mock_is_amd.calledboolean) asserted inside each test body; the pytest stdout log captures the assertion result by reportingpassed(both assertions held) orfailed(either assertion violated)..gitignorerules for safe generated bulk, or discarded generated debris.git status --shortgates locally and must refuse to submit while any touched repo is dirty.repo_hygiene.txt, rawgit status --shorttranscripts, orCLEANmarkers as QA evidence.Out of Scope
comfy/sd.py— production code is already correct onissue_101SHA7936e591per the upstream merge; this packet is regression-only.comfy/diffusers_convert.py— the conversion routine is out of scope; this packet tests the guard, not the conversion. The patchedconvert_vae_state_dictmock uses a passthroughlambda state_dict: state_dictside-effect specifically to avoid encoding the converter's output shape in the test (which would couple the regression to converter semantics that are not the test target).is_seedvr2_vaeatcomfy/sd.py:442); all five ACs feed a state dict that lacks the SeedVR2 indicator key (decoder.up_blocks.2.upsamplers.0.upscale_conv.weight, GTP-7) so the SeedVR2 short-circuit never fires.comfy/sd.py.metadata is Noneand key-present-non-"true"cases were initially marked out of scope at plan-foundations time per the issue body's two-cell contract. Round 8 of PR review (Copilot inliner3185061776, ACCEPT) brought both cases in scope as AC3 and AC4 — see the Round 8 Scope Expansion section above. They are no longer out of scope on the round-9 deliverable.metadata = {}empty-dict case was initially marked out of scope at plan-foundations time and remained out of scope through the round-8 four-cell contract. Round 26 of PR review (Copilot inliner3185375054, ACCEPT) brought it in scope as AC5 — see the Round 26 Scope Expansion section above. It is no longer out of scope on the round-27 deliverable.mock.call_count) with an exact-match assertion. Pertdd-planPhase 2 rule 12 and Phase 4 Check 25, Slice 0 calibration default applies only when the I/O contract names a measurement instrument; this packet'sunittest.mockcall-count assertion is not such an instrument. Documented here per the explicit-rationale-in-Out-of-Scope requirement.Tracked in pollockjj/mydevelopment Comfy-Org#119 (bookkeeping PR pollockjj/mydevelopment#201).