Skip to content

fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15#536

Draft
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-3290-tomlkit-comment-fix
Draft

fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15#536
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-3290-tomlkit-comment-fix

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

comfy node init writes a starter pyproject.toml with a couple of commented-out
"here's what you could put here" hints. It built those hints by handing tomlkit a
multi-line string. tomlkit 0.15.1 started rejecting that, so the command crashed —
and since tomlkit was unpinned, CI picked up the new release and the build check
went red on main and therefore on every open PR.

This writes each hint line as its own comment instead, which tomlkit is happy with,
and pins tomlkit so a future release can't break us silently again.

What was broken

Item.comment() now validates its argument and raises
ValueError("Comment cannot contain line breaks") on any \n (tomlkit/items.py:504).
Two call sites passed multi-line strings, so every call to create_comfynode_config()
raised — comfy node init was broken at runtime, not just in tests.

Reproduced on clean origin/main @ a732f5c with tomlkit 0.15.1: 9 failed (8 in
test_config_parser.py, 1 in test_node_init.py), matching the report exactly.

What changed

  • _add_commented_hint() emits a hint one standalone tomlkit.comment() per line.
    The helper takes the block and splits it, so the hint text stays verbatim and the
    shape that caused the bug is now structurally impossible to reintroduce.
  • The classifiers hint moved into create_comfynode_config. initialize_project_config
    reparses that file and only rewrites values, so this is the only point where the hint
    can be placed under license and before the [project.urls] sub-table.
  • Pinned tomlkit>=0.13,<0.16 (defense-in-depth, as the ticket suggested).
  • Two regression tests covering both hints: rendered position, inertness, and that
    uncommenting actually parses.

Rendered output is byte-identical to the pre-break version except two dropped
trailing spaces and one blank line (artifacts of the old inline attachment). Verified by
diffing this branch's comfy node init output against origin/main's under tomlkit 0.13.2.

Judgment calls (flagging for review)

  1. I did not use the fix the ticket proposed, because it doesn't hold up. The ticket
    suggested includes.comment('"requires-comfyui" = ...'). That renders inline:
    includes = [] # "requires-comfyui" = ... — but the stated intent (and the code's own
    comment, "uncommentable hint") is a hint beneath the field that a user can uncomment
    in place. Inline, it can't be. A standalone comment preserves the original behavior
    exactly; I went with that.

  2. The ticket's root cause was incomplete — there is a second call site. It named only
    config_parser.py:74-76, but that one is in create_comfynode_config; the 9 failing
    tests actually die at line 250 in initialize_project_config, a 17-line commented-out
    classifiers block. Fixing only the reported lines would have left CI red. Both are fixed.

  3. Placement is load-bearing, and the obvious fix is wrong. Naively appending the
    classifiers comment to the project table renders it after [project.urls] — so
    uncommenting it would define classifiers under [project.urls], the wrong table. The
    test asserts the ordering; I verified it fails against that mutation.

  4. Verified across the whole pinned range, not just the version I had installed — the
    affected tests pass under tomlkit 0.13.0 / 0.13.2 / 0.14.0 / 0.15.1. (At 0.13.0 blank
    hint lines render # vs #; cosmetic only, and the tests are not overfit to it.)

  5. Left alone (pre-existing, out of scope): the # Used by Comfy Registry comment
    renders inside [project.urls] rather than at the top of the file. Same class of
    placement bug, but it predates this change — I confirmed identical output on origin/main
    — so it isn't part of this fix. Happy to file it separately.

  6. Did not regenerate uv.lock. uv lock wants to pull in 286 lines of unrelated churn
    (main's lock is already stale w.r.t. the bench extra). CI installs via pip install -e .
    and doesn't enforce the lock, so this is a no-op for the build; keeping the diff scoped.

Verification

  • ruff check . — pass
  • ruff format --check . — pass (235 files)
  • pytest2575 passed, 37 skipped (the 9 previously-failing tests now green)
  • End-to-end: ran initialize_project_config() in a scratch git repo and diffed the real
    emitted pyproject.toml against origin/main's.

…t 0.15

tomlkit 0.15.1 made `Item.comment()` validate its argument, raising
ValueError("Comment cannot contain line breaks") on any `\n`. Two call
sites in config_parser passed multi-line strings, so every call to
`create_comfynode_config()` raised and `comfy node init` was broken at
runtime, not just in tests (9 tests red on main; tomlkit was unpinned so
CI resolved the newest release).

Emit each hint line as a standalone `tomlkit.comment()` instead, which
keeps the hints on their own lines and uncommentable in place. The
classifiers hint moves into `create_comfynode_config`, the only point
where it can be positioned under `license` and before the `[project.urls]`
sub-table. Rendered output is byte-identical to the pre-break version
apart from dropped trailing whitespace.

Also pin `tomlkit>=0.13,<0.16` to stop silent upstream breakage; the fix
is verified against 0.13.0, 0.13.2, 0.14.0 and 0.15.1.
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0d12cb5d-aa76-4f10-97e7-07384795e129

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-3290-tomlkit-comment-fix
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3290-tomlkit-comment-fix

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 1 finding(s).

Severity Count
🟢 Low 1

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 strip. Hints must be added while `table` is still being built, before any
sub-table: a comment appended after one renders *inside* that sub-table.
"""
for line in hint.strip("\n").splitlines():

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Low — The hint literals _INCLUDES_HINT/_CLASSIFIERS_HINT live in this source file, so if it is checked out with CRLF endings (the repo has no .gitattributes to normalize them, and core.autocrlf=true is the Windows default) they contain \r\n. hint.strip("\n") then leaves the leading \r, so splitlines() yields an empty string as its first element and an extra empty # comment is emitted above each hint. Use hint.strip() (or .strip("\r\n")) instead. Raised by 1 of 8 reviewers (gemini-3.1-pro edge-case).

mattmillerai added a commit that referenced this pull request Jul 17, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant