fix(registry): emit pyproject hint blocks as single-line tomlkit comments#533
fix(registry): emit pyproject hint blocks as single-line tomlkit comments#533mattmillerai wants to merge 3 commits into
Conversation
…ents tomlkit 0.15.0 started enforcing that a comment cannot contain line breaks, so the two multi-line `.comment()` calls in registry/config_parser.py now raise `ValueError: Comment cannot contain line breaks`. tomlkit is unpinned, so every fresh install resolves 0.15.x — this breaks `comfy node init` for new users and fails 9 tests on every PR's CI. Emit both hint blocks (the `requires-comfyui` hint on [tool.comfy].includes and the `classifiers` hint on [project].license) as one `tomlkit.comment()` item per line instead, which is within tomlkit's contract on both 0.13.x and 0.15.x. The generated pyproject.toml is unchanged apart from comment whitespace trivia: two trailing spaces and one blank line are dropped, and the blank line before [project.urls] now precedes the classifiers hint. Add tests covering the rendered hint blocks — their content, their placement relative to the fields they document, and that the classifiers hint is valid TOML once uncommented, which is its whole purpose.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTOML hint generation now uses line-safe helper infrastructure for ChangesTOML hint generation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've examined the code carefully. Let me verify the one panel finding against the actual code path.
The single non-empty finding (gemini-3.1-pro edge-case) claims: *"If the existing `pyproject.toml` contains other sub-tables (like `[project.scripts]` or `[project.optional-dependencies]`), the hint will be rendered after them."*
Checking the actual code path:
- `initialize_project_config()` (line 234) calls `create_comfynode_config()` **first**, which overwrites `pyproject.toml` in `"w"` mode (
Re-trigger by removing and re-adding the cursor-review label.
tomlkit.comment("") renders "# ", leaving trailing whitespace on every
blank separator line of the generated pyproject.toml. User repos commonly
run a trailing-whitespace pre-commit hook over that file.
Emit the separators as a bare "#" instead, which keeps the hint one
contiguous comment block (a real newline would split it visually).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Commit ee4ccdd inadvertently swept in a uv.lock re-resolution that adds the bench extra's dependency tree (anthropic, pydantic, pydantic-core, jiter, annotated-types, docstring-parser, typing-inspection; +285/-2). That churn is unrelated to the tomlkit comment fix and contradicts this PR's own description, which states uv.lock is deliberately untouched because the bench-extra resolution "belongs in its own change". CI installs via `pip install -e .` and never reads uv.lock, so the lock has no bearing on the fix. Restored to main's version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review resolution pass1.
|
Root cause of the
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
✅ No high-signal findings.
Panel: 5/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), claude-opus-4-8-thinking-xhigh:edge-case (parse_error), kimi-k2.5:edge-case (empty)
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
f91cab6 accidentally carried a 287-line uv.lock regeneration that has nothing to do with the HTTP oversize fix. The drift is real but pre-existing: #490 added the `bench` optional-dependency extra (anthropic>=0.40) to pyproject.toml without relocking, so main's lock has been stale since it merged — `uv lock --check` fails on main today. Any bare `uv run` in the repo silently re-locks and re-dirties the file, which is how it got picked up here. Re-locking is not this PR's job, and carrying it here invites conflicts with the dependency PRs already in flight (#533/#535/#536). No CI job consumes uv.lock, so reverting to main's copy is behavior-neutral. Filed as a follow-up instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ELI-5
comfy node initwrites apyproject.tomlwith two blocks of commented-out TOML in it, as hints you can uncomment (arequires-comfyuiline, and a bigclassifiersblock). We built those hints by handing tomlkit one long string with newlines in it. tomlkit 0.15 started rejecting that — a comment isn't allowed to contain line breaks — so the hint code now throws. We build the same hints one comment line at a time instead, which is what tomlkit always meant you to do.Problem
registry/config_parser.pycalled.comment()with multi-line strings in two places:create_comfynode_config— therequires-comfyuihint on[tool.comfy].includesinitialize_project_config— the ~15-lineclassifiershint on[project].licensetomlkit is unpinned, and tomlkit 0.15.0+ rejects comments containing line breaks, so both calls raise
ValueError: Comment cannot contain line breaks. These multi-line comments were always outside tomlkit's contract; 0.15 just started enforcing it.This is not only a CI break. Any user who installs comfy-cli fresh today resolves tomlkit 0.15.1 and gets a hard failure from
comfy node init/ project-config initialization. It also fails 9 tests on thebuildjob of every open PR, since CI installs withpip install -e .and resolves tomlkit fresh.Reproduced on this branch's base: fresh
uv pip install -e '.[dev]'resolves tomlkit 0.15.1, andpytest tests/comfy_cli/registry/test_config_parser.pygives8 failed, 73 passed.Fix
Emit each hint block as one
tomlkit.comment()item per line, appended to the containing table — no embedded\n, so it is within tomlkit's contract on 0.13.x and 0.15.x. The two hint blocks move to module constants (_REQUIRES_COMFYUI_HINT,_CLASSIFIERS_HINT) with a shared_append_hint()helper.No pin was added. The ticket proposed pinning
tomlkit<0.15as an immediate unblock and dropping the pin once the code was fixed. The code fix landed cleanly, so the pin would have been added and removed in the same PR —pyproject.tomlis untouched. Note CI installs viapip install -e .and never readsuv.lock, so the code fix alone is what unblocks it.Verification
2576 passed, 18 skipped, 0 failed(pytest tests/ -q --deselect tests/e2e).tests/comfy_cli/registry/test_config_parser.py→84 passed. The fix is correct on both, which is why the pin is unnecessary.ruff check .→ all checks passed;ruff format --check .→ 235 files already formatted..comment()call sites. The one remainingtomlkit.comment(...)call was already single-line and is untouched.Tests added
The generated hint blocks had no test coverage at all — the 8 failing tests only failed because the call raised, not because they assert on the hints. Three tests now cover the rendered output directly: hint content, placement relative to the field each hint documents, and that the
classifiershint parses as valid TOML once uncommented (its entire purpose).Judgment call — rendered output is content-identical, whitespace differs
The ticket asked to keep the rendered output byte-identical, and said that if step 2 required changing the output, to do the pin alone and open a child ticket. The generated file is content-identical, but three whitespace-trivia bytes differ, so calling this out explicitly rather than silently:
Concretely: (1) the trailing space after
includes = []andlicense = "MIT"is gone — it was an artifact of the multi-line trivia hack; (2) the blank line that separatedlicensefrom[project.urls]now sits above the classifiers hint rather than below it; (3) the trailing blank line after therequires-comfyuihint is gone.I read the ticket's fallback as guarding against a change to the generated file's content — every hint line is byte-for-byte what it was, in the same place, and both the existing tests and the new ones are green. Dropping back to the pin alone would leave
comfy node initbroken for fresh installs, which seemed strictly worse than losing two trailing spaces. Happy to be overruled if byte-identity was meant literally.Resolved (was disclosed as a caveat): blank hint lines previously rendered as
#(trailing space) on tomlkit <0.15, becausetomlkit.comment("")prefixes"# "unconditionally. Fixed inee4ccddby emitting bare#separators viaComment(Trivia(...)). Verified: the generated file now carries bare#separators on both 0.13.3 and 0.15.1, matchingmainbyte-for-byte on those lines.uv.lockis deliberately untouched:uv lock --upgrade-package tomlkitalso pulls in ~280 lines resolving thebenchextra that the lock onmainis already stale on. That's unrelated churn and belongs in its own change.Self-review notes
del project["urls"]before appending the hint, then re-adding it.Table.add()appends to the end, so appending the hint whileurlswas present would have rendered it after the[project.urls]sub-table instead of underlicense. It's safe becauseprojecthere is always parsed back fromcreate_comfynode_config's own output, whereurlsis the last key — so delete-and-re-append reproduces the identical key order, which the rendered-output diff above confirms. Theif "urls" in projectguard covers the absent case.