The tree fragment is now matched as a path segment, so an identifier that
merely carries the same letters (load_device, __device__) cannot trip the
check, and the POSIX home roots are checked outside the vendored upstream
tree. Two self-tests prove the tripwire catches a POSIX home path and does
not fire on device identifiers.
The tracked files still pointed at the development documents, which now live
in the untracked .dev tree: the usage headings promised the embedded
interpreter while the examples are interpreter agnostic, and the provenance
notes named a local document. The headings are reworded and the provenance
notes name the tool that produces the record instead.
The test asserts on the stub that records the progress calls. With
KEEP_NO_COMFY_MOCKS=1 the node uses the real comfy_api.latest API, whose
execution recorder has no calls attribute, so the test failed in the
mocks-off configuration (17 passed, 1 failed). It now skips there, with the
reason naming the configuration. The gate and the headless tier are
unchanged and green.
A tracked file must not name a local drive, a local tree or the local
interpreter, and the internal documents must not be in the index. The test
scans the tracked file list, builds its forbidden fragments from parts so it
does not trip itself, and skips binary files.
The tools named a local drive, a local interpreter and local ComfyUI trees.
They now take the tree from the command line or from the environment only -
a machine-local default would make a scan run against the wrong tree - and
the usage examples are interpreter agnostic. The exported example workflow
no longer carries the absolute path of the machine it was exported on.
The audit artifacts and the plan moved to the local .dev/docs tree, so the
references in the code, the tools and the tests follow them there. The
license file names the tool that produces the record instead of a path.
The development documents (the modernization plan, the phase reviews, the
audit artifacts, the status reports) are local working material: they live
under .dev/docs and must never be committed. docs/ leaves the index and
.gitignore keeps both trees out; the .dev pattern is anchored without a
trailing slash so it matches the .dev symlink itself.
The headless tier runs with the ComfyUI mocks switched off
(KEEP_NO_COMFY_MOCKS=1), but test_the_mock_exposes_no_symbol_the_real_api_lacks
read the mock out of sys.modules - where it is not installed in that
configuration - and died with KeyError: 'comfy_api.latest'.
The mock construction is now a module-level function (build_comfy_api_mocks)
that _install_comfy_mocks uses, so the test can build the mock itself when
the mocks are not installed. The comparison against the real ComfyUI API now
runs in the headless tier as well.
The development artifacts under docs/ are not shipped with the pack and may
be absent from a working tree (the docs folder is gitignored). The tests
that assert their content now skip with a message naming the missing
artifact instead of failing. The README no longer points at an audit file
that is not part of the package.
ComfyUI runs a V3 node's validate_inputs during the prompt validation, where
the links are not resolved yet: a connected pack arrives as None (after the
list unwrapping) or as the (None,) marker of a missing link. Both nodes
rejected that unresolved pack, so every prompt failed with the same error
once per input.
- validate_inputs now accepts an unresolved pack and only refuses a value
that is present but of the wrong type.
- execute raises the actionable error for a pack that is really wrong
(the plan already requires execute to raise instead of returning None).
- the shared message lives in src/keep_model_loader.keep_model_pack_error.
- tests cover the unresolved pack (None and (None,)) and the raising execute.
Verified against the real ComfyUI validation path: execution.get_input_data
with a linked pack returns (None,) and validate_inputs now returns True.
The five review findings of 2026-10-24 for phase P6:
1. Gate 2 (`ruff check .`) was RED on the P6 tree: F841 - the local `size`
of `_anchor_in` in tools/render_workflow_preview.py:95 was assigned but
never used, so the lint job of .github/workflows/tests.yml would have
gone red on this branch. The unused local is gone; the render output is
unchanged (the regenerated png has the same sha256 as the committed
one). The gate was recorded as "not runnable" although ruff IS
installed on this machine (0.15.22; the CI pin 0.8.6 reproduces it via
`uvx ruff@0.8.6`) - the embedded env is only one of the environments.
The gate records (audit D6, the progress doc, the status doc, the plan)
now state the truth.
2. The per-node parameter tables of the README were not checked by any
test, although the README calls the widget names, the defaults, the
ranges and the order compatibility guarantees. tools/docs_tables.py
renders what a V3 schema says (documented_parameters) and
test_the_parameter_tables_of_the_readme_match_the_node_schemas compares
the README with it. The check is mutation-checked: a wrong
`max_clip_length` default/range, a wrong `final_upscale_factor` range,
a wrong `only_center_face` default and two swapped widget rows each
fail the test (the values in the README are correct - the gap was the
coverage).
3. tools/docs_tables.py carried REPO_FILE_EXTENSIONS and
EXTERNAL_PATH_ROOTS without using them while the test re-defined them.
The test imports both from tools/docs_tables.py now - one copy, no
drift between the generator and the checker.
4. The rendered preview of the sequence workflow was never shown in the
README (only the shipped workflow's preview was). The README shows
example_workflows/keep-sequence-workflow.png in the Example workflows
section, captioned as a rendered graph diagram and not a screenshot.
5. The committed progress doc called its own review file untracked and
"stays uncommitted" although it is committed (0066001, 185c289). The
gate-7 row names `.dev` as the only untracked path now.
Checks (run on 2026-10-24): ruff check . -> All checks passed! (0.15.22)
and uvx ruff@0.8.6 check . -> All checks passed! (the version CI pins);
pytest -q -m "unit or mock_integration or headless" -> 262 passed, 6
skipped, 5 deselected; pytest -q tests/test_structure.py -> 21 passed;
pytest -q tests/test_example_workflows.py tests/test_docs_claims.py
tests/test_examples_dir.py -> 29 passed; pytest -q -m gpu_acceptance -rs
-> 5 skipped (opt-in); the headless tier against the real ComfyUI source
-> 4 passed, 1 failed (the pre-existing D5 mock-fidelity failure - P6
touched neither conftest.py nor that test).
The five review findings of 2026-10-23 for phase P5:
1. The P5.5 fallback branch (a checkpoint without flownet.* keys) died with
a bare RuntimeError at the strict load: the arch always owns the flownet
(keep_arch.py:882), so the state dict of the net always carries the
flownet.* keys - and the GMFlow weights the arch had just loaded were
discarded. The flownet tensors of the built net (the trained ones - the
arch was built with flownet_path) are now merged into the checkpoint
state dict before load_state_dict(..., strict=True). The load stays
strict (an unexpected key still raises) and the trained flow weights
survive. The stub arches of the tests behave like nn.Module now - a
no-op recorder had hidden the defect.
2. P5.4's acceptance is no longer unmet: the GPU acceptance run was
executed (real weights, real ComfyUI modules, RTX 4070 Ti SUPER) and is
green (5 passed, exit code 0; the run log is in the audit doc section 5).
KEEP_MODEL_DIR now really decides where the weights come from - it is the
models root of the run (folder_paths.models_dir), the harness fixture
no longer isolates the gpu_acceptance tier, and the run needs
KEEP_NO_COMFY_MOCKS=1 so the real ComfyUI modules are in charge.
3. The "the flow weights are not random" assertion of the acceptance test
could not distinguish random weights from trained ones. It now compares
the flownet tensors with the tensors of their source (the KEEP checkpoint
or the GMFlow file) bit for bit.
4. The superseded download helper load_file_from_url_comfy is gone
(src/keep_paths.py only resolves paths; the download lives in
src/keep_download.py since P5.2). The stale docstrings, the stale
comment in the vendored facelib and the two tests that called the dead
helper are fixed.
5. The risk list mentioned a pending deletion of assets/keep-preview.png
that does not exist - the asset is committed and present (93279 bytes).
The rule in the implementation workflow now states the truth.
Checks: the gate (pytest -q -m "unit or mock_integration") is green (234
passed, 1 skipped, 1 xfailed); the opt-in acceptance run is green (5 passed);
the acceptance tier still skips cleanly without its environment variables.
P5.0-P5.5 are implemented and committed. This commit adds the phase audit (docs/audit/2026-10-23-p5-weights.md, deviations D1-D8), the working-state review (docs/2026-10-23-p5-weights-progress.md) and marks the P5 row in the plan's progress table as done.
Two review findings of the P4 phase, both fixed at their root:
1. (medium) Structure guard (g) did not scan the node layer. Its file set
came from the import closure of tools/scan_compat.py (the three src/
entry points) plus src/** - the node layer (nodes/**) and the root
__init__.py were invisible, although guard (h) of the same phase scans
them. A torch.cuda.empty_cache() added to the shipped node code passed
the guard silently.
tests/test_structure.py: new PACKAGE_OWN_SOURCE_ROOTS (the pack's own
code without the vendored tree), _package_files() takes the trees to
walk (the default is unchanged, so guards (d) and (h) are untouched),
and guard (g) now scans the own code (13 files) plus the vendored
modules/deps closure (139 files - the vendored part stays
closure-scoped as the plan and D8 require). Two new self-tests cover
the node layer and the entry point.
Proof (old guard vs new guard on the same injected copy):
OLD guard (HEAD) -> []
NEW guard (fixed) -> ['__init__.py: 1 occurrence(s) of
torch.cuda.empty_cache() (allowlisted: 0)', 'nodes/
keep_process_image_sequence.py: 1 occurrence(s) of
torch.cuda.empty_cache() (allowlisted: 0)']
2. (low) The P4.5 "CPU-mode variant" residency test was vacuous: the
conftest mock already answers torch.device("cpu") for
get_torch_device(), so the monkeypatch was a no-op and the variant
exercised the default path.
tests/test_keep_model_loader.py: the test now runs a GPU leg and a CPU
leg - both must produce the same residency calls (load_models_gpu,
unload_all_models, soft_empty_cache, once each) and the patched device
must really reach the pack.
Proof (mutation probes against the production code):
mutation A (the pack hardcodes the cpu device and ignores ComfyUI):
the old test (from HEAD) passes (10 passed), the new test fails
(assert device(type='cpu') == device(type='cuda')).
mutation B (the pack skips load_models_gpu on a cpu device): the new
test fails (the two legs no longer match).
Docs: the plan's P4 progress row, the guard table of the P4 audit and the
P4 working state are current; the round is recorded in
docs/2026-10-23-p4-review-findings.md.
Verification (run in this round):
python_embeded/python.exe -m pytest -q -m "unit or mock_integration"
-> 189 passed, 1 skipped, 5 deselected, 2 xfailed
(baseline before the round: 187 passed)
COMFYUI_ROOT=... python_embeded/python.exe -m pytest -q -m headless
-> 5 passed, 192 deselected
ruff check . -> All checks passed!
Five review findings of the P3 phase, each fixed at its root:
1. (medium) The P3.7 metadata test imported `tomllib` at module level: the
**3.10 CI job** (the declared `requires-python >=3.10` minimum) died
during collection, so the registry-metadata assertions never ran on 3.10.
The parser import is now guarded (`tomllib` -> `tomli`, the package the
3.10 job installs); without any parser the module *skips* instead of
dying. `pyproject.toml` gained the test extra
`tomli>=2.0; python_version < '3.11'`.
Proof: a 3.10 simulation (the stdlib `tomllib` refused for the test
module, `tomli` on sys.path) -> 6 passed; the control without `tomli`
-> 6 skipped (no collection error).
2. (medium) The P3.5 "primary-root-first multi-root" resolution was not
wired into the production load path: `resolve_model_file` (the only
`get_full_path_or_raise` caller) was used by tests only, so a weight in
a secondary model root was downloaded into the primary root again.
`src/keep_paths.py` now has `find_model_file()` (the multi-root lookup
through `folder_paths.get_full_path_or_raise`) and `resolve_model_root()`
(the root the weights really live in); `load_file_from_url_comfy()` - the
production path of `src/keep_model_loader.py:107` - uses the lookup, and
the facelib root follows the weights (the facelib helper builds its
paths from a single root). New tests prove that the production path
uses a weight of an extra model root and that the primary root wins.
The `models_dir` grep guard is no longer a blanket exemption: only the
registration in `src/keep_paths.py` may use the attribute.
3. (low) `[tool.comfy].Icon` was an empty string and the test only checked
that the key exists. The preview asset **is** committed
(`git cat-file -s HEAD:assets/keep-preview.png` -> 93279 bytes); only the
working copy was missing (an uncommitted local deletion, restored from
HEAD - no asset change is in this commit). The key now points at
`assets/keep-preview.png` and the test asserts that the value is not
empty and names a file that is really in the repository.
4. (low) `tests/test_package_entrypoint.py` hard-coded the V1 state and
would have gone red (not xfail) when P4.4 flips the root `__init__.py`
to `comfy_entrypoint`. The assertions now follow the entry point that is
really there: exactly **one** mapping style (V1 XOR V3). The mocked
smoke test of `tests/test_package_import.py` got the same treatment; the
headless variant is P4.4's job (its test list names it).
5. (low) The plan's §3 rule ("`comfy.*` imports inside `src/` are lazy
(function-level) so the pack imports without a ComfyUI server") was
violated and unguarded. The `comfy` imports of `src/keep_model_loader.py`
and `src/keep_processor.py` are function-level now; the vendored facelib
helper (which imports `comfy` itself) is imported lazily through a module
`__getattr__` (PEP 562), so `loader.FaceRestoreHelper` still resolves to
the vendored class. Structure **guard (f)** (`tests/test_structure.py`)
forbids a module-level `comfy`/`comfy_api` import in `src/**` (with the
self-test the guards file demands), and the new
`tests/test_lazy_imports.py` proves at runtime that the engines import
while every `comfy` import is blocked.
Gate after the commit:
python.exe -m pytest -q tests -m "unit or mock_integration"
-> 153 passed, 1 skipped, 5 deselected, 3 xfailed
COMFYUI_ROOT=C:/_Dev/ComfyUI_dev/ComfyUI-source/ComfyUI python.exe
-m pytest -q tests -m "unit or mock_integration or headless"
-> 158 passed, 1 skipped, 3 xfailed
Details: docs/2026-10-23-p3-review-findings.md
Section 9 of the plan: the P3.1-P3.7 row is now marked done with the per-step
summary. The phase review (docs/2026-10-23-comfyui-keep-p3-phase.md) carries the final
state, the gate outcomes and the remaining risks. The nine deviations of the phase are
recorded in docs/audit/2026-10-23-p3-restructure.md.
Five review findings of the P2 phase, each fixed at its root:
1. (medium) The P2.6 guard could not see an undeclared third-party import:
the closure's imports were filtered through a hard-coded 10-name map, so
every module outside it was dropped silently (a module-level `import av`
in a copy of the repo still gave "1 passed"). The map is now a *rename*
map (cv2 -> opencv-python, PIL -> Pillow, yaml -> pyyaml); every other
module is mapped to its own name, so an unknown module shows up as an
undeclared dependency. The guard is split by import time: every
third-party module imported at import time must be a declared
distribution, every lazy (function-level) one must be allowlisted with a
reason (dlib, gdown, mc, wandb - all unreachable from the node).
2. (low) The P2.1 (c) progression test was vacuous: it returned before its
assertions and probed a second, top-level copy of the vendored tree
(`sys.path.insert(modules/deps)`). It now imports the packaged
architecture, asserts that FeedForward / AdaLayerNorm come from the
vendored _diffusers_compat and that no top-level `basicsr` shadows an
installed one.
3. (low) Two swallow-and-stub fallbacks remained in the vendored tree
(facelib/detection, facelib/parsing - their stubs called an unimported
`urlparse` and would have raised NameError). Both now import the vendored
symbol directly; a guard forbids a fallback stub in the vendored tree.
4. (low) `basicsr/utils/file_client.py` still appended to `sys.path` (the
optional `sys_path` of MemcachedBackend), which the plan's §3 rule
forbids anywhere in the package. The mutation and the parameter are gone
and guard (d) now covers the whole package (the vendored tree included).
5. (low) The unreachable body of `KEEPFaceProcessor.process_video` still
called the deleted VideoReader / VideoWriter. The 141 dead lines are
removed; an AST guard keeps the references out.
Found while fixing the findings: the CI job did not install `requests`,
which the vendored `basicsr/utils/download_util.py` imports at module level
(P2.6 added it to requirements.txt but not to the workflow) - a clean CI
runner died with "No module named 'requests'". The install list is now
completed and a test derives it from requirements.txt.
Gate: pytest -m "unit or mock_integration" -> 109 passed, 1 skipped,
4 xfailed; plan's local gate (+ headless) -> 109 passed, 3 skipped,
4 xfailed; tools/scan_compat.py -> finding_count 0 (exit 0); ruff check . ->
All checks passed. A mutation check re-introduces six defects into a copy of
the repository: the matching guard fails in 6/6 cases.
WildAi