feat(registry): resolve PEP 621 dynamic version from [tool.comfy.version].path#438
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PEP 621 dynamic-version support and stricter publishing pre-checks: publishing validation aborts earlier for missing node config or empty/whitespace project.version; config parser can resolve version from a referenced source file (safe path checks, regex extraction, warnings). A tiny rhyme: new version-finding logic — no more blind guessing, just precise detecting. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client (CLI)
participant Cmd as Publish Validator
participant Parser as Config Parser
participant FS as Filesystem
participant Echo as User Output
CLI->>Cmd: run `app validate` / publish
Cmd->>Parser: call extract_node_configuration()
alt node config is None
Parser-->>Cmd: returns None
Cmd->>Echo: echo error (escaped Rich)
Cmd-->>CLI: exit code 1
else node config present
Cmd->>Parser: request resolved version
Parser->>Parser: check project.version
alt static version present
Parser-->>Cmd: return static version
else dynamic declares "version"
Parser->>FS: resolve `[tool.comfy.version].path` (safe relative)
FS-->>Parser: file content or read error
alt file readable
Parser->>Parser: regex extract __version__/VERSION
alt match found
Parser-->>Cmd: return extracted version
else no match
Parser->>Echo: warn (no assignment found)
Parser-->>Cmd: return ""
end
else unreadable/invalid path
Parser->>Echo: warn (path/config issue)
Parser-->>Cmd: return ""
end
end
Cmd->>Cmd: if version empty -> echo error and exit 1
Cmd-->>CLI: proceed or exit depending on checks
end
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f36dfca to
75b3e15
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #438 +/- ##
==========================================
+ Coverage 78.02% 78.40% +0.37%
==========================================
Files 35 35
Lines 4319 4399 +80
==========================================
+ Hits 3370 3449 +79
- Misses 949 950 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/registry/config_parser.py`:
- Around line 358-365: The current logic reads comfy_data.get("version", {})
into version_cfg and treats non-dict values the same as a missing path, which
produces a misleading warning; update the check around version_cfg and
path_value so that if version_cfg is not a dict (i.e., a scalar like "1.2.3")
you emit a dedicated warning that `[tool.comfy].version` is a scalar and the
user should define a `[tool.comfy.version]` table with a `path` field, otherwise
keep the existing missing-path warning when version_cfg is a dict but path is
empty; reference the variables version_cfg and path_value in config_parser.py
and return the same empty string after emitting the more accurate warning.
- Around line 324-328: The current try/except around
resolved.read_text(encoding="utf-8") only catches OSError but UnicodeDecodeError
(a ValueError) can escape and crash the parser; update the exception handler for
the read into text to also catch UnicodeDecodeError (or ValueError) so that on
decode failures you call the same typer.echo warning (including rel_path and the
exception) and return ""—i.e., change the except clause that currently
references OSError as e to catch OSError and UnicodeDecodeError (or ValueError)
for the resolved.read_text call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19836d1d-f777-44d1-bc5e-c6eda5898f4a
📒 Files selected for processing (5)
.github/workflows/publish_package.ymlcomfy_cli/command/custom_nodes/command.pycomfy_cli/registry/config_parser.pytests/comfy_cli/command/nodes/test_publish.pytests/comfy_cli/registry/test_config_parser.py
|
Thank you! 🙏🏻 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/registry/config_parser.py`:
- Line 329: The current code reads the file into text using
resolved.read_text(encoding="utf-8") which preserves a leading UTF-8 BOM and can
break the `^__version__` anchor; change the read call to use "utf-8-sig" so any
BOM is stripped transparently (i.e., replace the
resolved.read_text(encoding="utf-8") call with
resolved.read_text(encoding="utf-8-sig") where the variable `resolved` is used
to load the `_version.py` content).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c94ac12-e051-4279-a3a5-21596f333b38
📒 Files selected for processing (4)
comfy_cli/command/custom_nodes/command.pycomfy_cli/registry/config_parser.pytests/comfy_cli/command/nodes/test_publish.pytests/comfy_cli/registry/test_config_parser.py
75b3e15 to
3f9b45c
Compare
|
Though, when it will be merged and documented, I strongly suggest encouraging node authors to keep a version in its own file ( |
3f9b45c to
db7ad9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/registry/config_parser.py`:
- Around line 30-37: Add a short note to the docstring near _VERSION_RE (or the
surrounding function/module docstring in config_parser.py) explaining the
empty-string-literal edge case: because _VERSION_RE uses [^"\n]+ and [^'\n]+ it
requires at least one character inside quotes so a declaration like __version__
= "" will not match and will trigger the "could not find" warning; state that
this is intentional as empty version literals are rejected as a safety measure
to aid debugging.
In `@tests/comfy_cli/registry/test_config_parser.py`:
- Around line 222-413: Add a test that ensures look-alike names (e.g.,
VERSIONED, MY_VERSION) do not trigger a partial regex match before a real
version assignment: create a tmp file via tmp_path containing look-alike
assignments first and then a real __version__ or VERSION assignment later, call
extract_node_configuration(path) and assert result.project.version equals the
real later value; place the test alongside the other tests (e.g., name it
test_dynamic_version_ignores_lookalike_names) to pin the exact-match behavior of
the regex used by _resolve_dynamic_version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93372bd4-c017-4685-99e5-196e7c16281f
📒 Files selected for processing (4)
comfy_cli/command/custom_nodes/command.pycomfy_cli/registry/config_parser.pytests/comfy_cli/command/nodes/test_publish.pytests/comfy_cli/registry/test_config_parser.py
2364cde to
cf4e854
Compare
Good point! |
d04cdc0 to
7be4bbd
Compare
…ion].path Custom nodes using `dynamic = ["version"]` in pyproject.toml were silently getting an empty version in registry publishes. Add a comfy-native `[tool.comfy.version]` section with a `path` key that points at a source file containing `__version__` or `VERSION`; parse it with an anchored regex (no Python execution, scanning-friendly per earlier discussion with @ltdrdata, @Lex-DRL). - `config_parser.py`: add `_VERSION_RE`, `_resolve_dynamic_version`, `_extract_version`. Resolution runs only when `dynamic` contains `"version"` and `project.version` is absent; all failure modes (no config, absolute path, path escaping project dir, missing file, no regex match) warn and fall back to empty so scanning contexts stay graceful. Type-check `project.version` up front — running BEFORE the truthy gate — so both truthy and falsy non-strings (`1`, `0`, `0.0`, `false`, `["1","2"]`, `[]`, `{path="_v.py"}`, `{}`) are rejected at parse time instead of silently being coerced to their Python repr and POSTed to the registry. The quoted-value regex excludes `\` from the value class entirely — escape sequences (`\n`, `\t`, `\"`, ...) cause the regex to fail to match and surface as a "could not find" warning rather than being silently misinterpreted; PEP 440 versions are ASCII-only so this is a clean fail-closed contract, and the single-alternative character class also makes catastrophic backtracking impossible. Open pyproject.toml with `encoding="utf-8-sig"` (strips Windows editor BOMs that otherwise yield a cryptic `Empty key at line 1 col 0`) and add `UnicodeDecodeError` to the parse except tuple so non-UTF-8 bytes degrade to the same friendly error as other unreadable files. Detect adjacent-string-literal concatenation (`__version__ = "1." "2.3"`, with or without whitespace between, quote styles freely mixed) via a same-line look-ahead after the closing quote: if the first non-whitespace char is a quote, Python would evaluate the two literals together (`"1.2.3"` here) but the regex captures only the first literal, so returning `"1."` to the registry would silently publish a wrong version. Warning scoped narrowly to a quote byte so `;` (statement separator) and `#` (comment) following a valid literal continue to resolve cleanly. - `command.py`: fail fast in `validate_node_for_publishing()` when the resolved version is empty, so we never POST an empty `version` to the registry. - tests: 19 new config_parser cases using real tmp_path files (typed- version rejection covering both truthy and falsy non-strings; backslash-in-value rejection pinning the fail-closed contract; adjacent-literal concatenation detection across double-quote, single-quote, mixed-quote, and no-whitespace forms, plus a negative control pinning that `; x = 1` after a valid literal still resolves), plus a publish-layer guard test. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
7be4bbd to
bd20ef7
Compare
Fixes #294
Summary
Custom node authors using
dynamic = ["version"]inpyproject.toml(PEP 621) previously got a silent failure — the version was read as""and POSTed to the registry anyway. This PR adds a comfy-native[tool.comfy.version]section that tells us where to read the version from, and a publish-time guard so empty versions never reach the registry.The file is text-read and a single anchored regex extracts
__version__orVERSION. No Python execution — this was the hard constraint agreed with @ltdrdata in the issue thread (env-sensitive, slow, breaks the Manager's "quick scan of installed packs" use case). The schema matches @Lex-DRL's proposal that got a 👍 in the thread.Behavior
project.versionpresent → used as-is (existing behavior, no change)."version"inproject.dynamic→ resolve via[tool.comfy.version].path.typer.echoand fall back to empty version. This keeps scanning contexts graceful — they never crash on malformed pack configs.validate_node_for_publishing()now exits1with a clear message if the resolved version is empty, so we never silently POST an empty version to the registry. This is the behavior change that actually fixes the bug.Regex
Start-of-line anchored (MULTILINE) so single-line comments are skipped. Horizontal whitespace only — no cross-line matching. Both quote styles. PEP 526 type annotation tolerated.
Documented limitations (unchanged from @Lex-DRL's proposal):
__version__(f-strings,get_distribution().version, etc.) doesn't.__version__andVERSIONnames.Non-goals (punted to follow-ups)
[tool.hatch.version]passthrough: zero-config for existing hatch users, ~10 LOC. Easy follow-up if the community asks for it; kept out of v1 to avoid coupling to a second schema.[tool.setuptools.dynamic] attr = "...": requires Python module loading. Explicitly out of scope per the issue discussion.Test plan
New tests in
tests/comfy_cli/registry/test_config_parser.py(12 cases, all using realtmp_pathfiles):__version__(double-quoted, single-quoted, with PEP 526 annotation).VERSIONname.# __version__ = "x"line correctly ignored (^anchor).__version__lines.project.versionbeats[tool.comfy.version]when both present (no accidental resolution).dynamic = ["version"]with no[tool.comfy.version]→ warn + empty.../../etc/passwd) → warn + empty.Plus in
tests/comfy_cli/command/nodes/test_publish.py:comfy node validateexits1with the helpful error whenproject.versionis empty.CI:
pytest tests/comfy_cli/— 687 passed, 6 skipped (+13 new).ruff check .andruff format --check .— clean.Risk / blast radius
"").registry/types.py.