feat(workflow): add comfy workflow notes — read Note/MarkdownNote text offline (BE-4776) - #611
Conversation
…ext offline (BE-4776)
Extracts documentation-note content from a frontend-format workflow so agents
can read a template's authored notes (LoRA trigger words, model links, usage
caveats) without cat/grep-ing raw JSON.
A separate verb rather than an extension of `slots`: notes are UI-only virtual
nodes with no schema, so extraction is pure JSON parsing — no object_info, no
running server — and `slots` payloads stay free of multi-KB markdown blobs.
Scans the top-level graph plus every `definitions.subgraphs[]` entry, emitting
{id, type, title, text, pos, size, subgraph} per note. Reuses
`_load_workflow_or_fail` for the missing-file / bad-JSON / API-format envelope
errors; API-format rejection is correct because the conversion strips note
nodes entirely.
Registers the command in COMMAND_SCHEMAS (the repo's discovery test enforces
this) and documents the `notes[]` shape in schemas/workflow.json. Pretty-mode
output routes note text/titles through _safe_console_text() so untrusted file
content can't emit ANSI escapes or rich markup, matching the existing
_strip_terminal_controls convention in this module.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 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)
📝 WalkthroughWalkthroughThe CLI adds ChangesWorkflow Notes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant WorkflowFile
participant NoteExtractor
participant Renderer
CLI->>WorkflowFile: read frontend workflow JSON
WorkflowFile-->>CLI: parsed workflow
CLI->>NoteExtractor: collect top-level and subgraph notes
NoteExtractor-->>CLI: note payload
CLI->>Renderer: sanitize and render pretty output
Renderer-->>CLI: safe console output
🚥 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.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…776) Addresses the six cursor-review panel findings on PR #611. Malformed-but-parseable input must degrade to an envelope, never a crash. `_is_frontend_format` only vouches for the top-level `nodes` list, so `definitions.subgraphs`, a subgraph's own `nodes`, and each node's `type` are all attacker-shaped. Previously `for x in v or []` over a truthy non-iterable (`"subgraphs": 1`) raised an uncaught TypeError, and an unhashable `type` (list/dict) blew up the frozenset membership test. Each container is now isinstance-checked before it is walked. `notes[].title` and `notes[].subgraph.name` are declared `string|null` in schemas/workflow.json but were emitted verbatim, so a non-string value produced a payload failing the command's own schema. Both now coerce via `_str_or_none`. `_strip_terminal_controls` promised to block output spoofing but kept `\r` and every codepoint >= U+00A0 — so a lone carriage return could overwrite already-printed text, and bidi overrides/isolates could reorder it Trojan-Source style. It now drops `\r` (lossless for CRLF, which keeps its `\n`) plus the invisible bidi/zero-width codepoints. This is a pretty -console path only; the `--json` envelope still carries note text exactly as authored. Pretty mode no longer renders a literal `#None` / `(subgraph None)` when a note has no id or a subgraph def has neither name nor id.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/command/workflow.py`:
- Around line 594-623: Update _strip_terminal_controls to use
unicodedata.category(ch) and reject all Unicode format characters (category
“Cf”), while explicitly excluding U+2028 and U+2029 as line and paragraph
separators. Remove the now-redundant _SPOOFING_CODEPOINTS filtering and add the
required unicodedata import, preserving the existing tab/newline and
printable-character handling.
In `@tests/comfy_cli/command/test_workflow_notes.py`:
- Around line 319-324: Add an assertion in test_rejects_invalid_json that the
invocation exit_code equals 1, matching the sibling workflow-notes tests while
preserving the existing envelope and error-code assertions.
🪄 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 Plus
Run ID: 249b7a05-8785-46bc-b8b8-36be3febd524
📒 Files selected for processing (4)
comfy_cli/command/workflow.pycomfy_cli/discovery.pycomfy_cli/schemas/workflow.jsontests/comfy_cli/command/test_workflow_notes.py
CodeRabbit review follow-ups on #611. Replace the hand-rolled _SPOOFING_CODEPOINTS set with a category-based filter over Cf/Zl/Zp. Verified against unicodedata 16.0.0 that the new rule is a strict superset of the old set (nothing previously stripped is now kept), and that it additionally catches U+061C (Arabic Letter Mark), U+2028/U+2029 line+paragraph separators, the invisible math operators, and the U+E0020-U+E007F tag block -- 155 codepoints in all. The category lookup is gated behind the existing ord(ch) >= 0xA0 branch, so ASCII still settles on the range check alone and large `workflow get` payloads (the helper's other call site) don't pay a per-char lookup. Both call sites are pretty-renderer only: JSON mode emits note text verbatim, confirmed empirically, so the broader sweep costs no fidelity for machine consumers -- it only stops invisible characters from lying about what the text says on a terminal. Tests: 5 new spoofing params for the newly-covered codepoints, a guard that visible non-ASCII (Zs/Pd/Mn) survives the sweep, and the missing exit_code == 1 assertion on test_rejects_invalid_json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bigcat88
left a comment
There was a problem hiding this comment.
Approving.
CI failure is not yours. test_model / test_run fail on model download ... failed after 3 attempts: the server returned HTTP 429 Too Many Requests — HuggingFace rate-limiting the e2e checkpoint fetch. Same failure is showing on #609, #596 and #574 right now, and main is green. Nothing to fix; a re-run when HF calms down should clear it.
Feature works, verified against a real in-repo template. comfy workflow notes comfy_cli/bench/fixtures/txt2img_seed.json returns count: 4 with all four MarkdownNote bodies intact — including the one with fenced code and box-drawing characters — and note keys are exactly {id, type, title, text, pos, size, subgraph}.
Subgraph traversal is right. Hand-built a workflow with one top-level Note and one MarkdownNote inside definitions.subgraphs[]: both come back, the inner one carrying subgraph: {"id": "sg-abc", "name": "My Subgraph"} and the top-level one null. That's the shape you documented.
The pretty-mode hardening you added beyond the plan is the right call, and it holds. I fed it a note whose body is <ESC>[2J<ESC>]0;PWNED<BEL><ESC>[31mRED<ESC>[0m and [bold]markup[/bold] and [/]:
raw ESC bytes: 0 | CSI-2J: False | MarkupError: False
evil #1
[2J]0;PWNED[31mRED[0m and [bold]markup[/bold] and [/]
Renders literally, no screen clear, no crash on the unbalanced [/]. And JSON mode still carries the raw bytes (raw ESC preserved: True, [/] preserved) — which is the correct split: strip at the terminal, never mutate what an agent parses. Worth saying plainly: template JSON is untrusted input that arrives over the network, so rprint(n["text"]) as the plan had it would have been a real defect, not a hypothetical one.
API-format rejection verified, and the premise behind it too. comfy workflow notes on an API-format file returns ok: false / workflow_not_frontend_format. I confirmed the reason independently: workflow_to_api._UI_ONLY_NODE_TYPES is {"Note", "MarkdownNote", "PrimitiveNode", "GetNode", "SetNode", "Reroute"} and the converter continues past all of them at line 158, so note text genuinely does not survive conversion. The dead-end is correct rather than an oversight.
Design call — separate verb over extending slots — is right. slots needs an object_info graph because slots derive from node schemas; notes are UI-only virtual nodes with no schema, so the parse is pure JSON. Folding them together would have made an offline-capable read require a live server, and pushed multi-KB markdown into every slots payload. Keeping them apart is the better factoring.
Leaving pos / size type-unconstrained in the schema is also correct and worth the comment you gave it — litegraph really has serialized pos as both [x, y] and {"0": x, "1": y}, so an array constraint would fail real templates.
Full suite: 2900 passed, 7 skipped, 1 failed — test_non_fast_deps_uses_global_python, the sandbox artifact that fails on plain main here too. ruff check clean.
One forward-looking note, not blocking: the _safe_console_text() treatment you applied here is exactly what command/run/execution.py:203 is missing — I've flagged that on #614, where a hostile server body still reaches the terminal with live escape sequences. Same pattern, same fix; worth reusing this helper's approach there.
|
Re-verified against current Two failures in that run, neither yours:
Your red CI check is still just the HuggingFace |
ELI-5
ComfyUI template authors leave sticky notes on the canvas — "use
ohwx manto trigger this LoRA", "download the model from here", "steps=30 for finals". Until now the only way for an agent (or a person) to read those was tocatthe workflow JSON and eyeball it. This addscomfy workflow notes <file>, which just prints them.What
A new read-only subcommand:
comfy workflow notes <file>. It pulls theNote/MarkdownNotenodes out of a frontend-format workflow and emits{workflow, count, notes[]}, where each note carries{id, type, title, text, pos, size, subgraph}. It scans the top-level graph and everydefinitions.subgraphs[]definition (subgraph notes get asubgraph: {id, name}ref; top-level ones getnull).Origin: the QA report row "Read a template's LoRA 'trigger words' note" (#pillar5-mcp, 2026-07-27) — no CLI/MCP surface exposed note text.
Why a separate verb instead of extending
slotsslotsneeds an object_info graph (_get_graph— a live server or an--inputdump) because slots derive from node schemas. Notes are UI-only virtual nodes with no schema at all, so reading them is pure JSON parsing. Keeping them separate meansnotesworks fully offline, andslotspayloads stay free of multi-KB markdown blobs.Verified
ruff check/ruff format --checkclean repo-wide (with the CI-pinnedruff==0.15.15), and the fullpytestsuite is green: 2798 passed, 37 skipped. 24 new tests intests/comfy_cli/command/test_workflow_notes.pycover the payload shape, subgraph notes, the six malformed-widgets_valuesshapes, the three envelope error codes, pretty-mode rendering, and schema conformance.Live smoke against a real in-repo template fixture (
comfy_cli/bench/fixtures/txt2img_seed.json, no ComfyUI running, no object_info cache):comfy --json workflow noteson that file returnscount: 4with all fourMarkdownNotebodies intact.Two things I added beyond the ticket plan, and why
1. Schema registration. The plan didn't mention it, but the repo's own
test_every_emitted_command_registers_a_schemafails on anyrenderer.emit(command=...)that isn't inCOMMAND_SCHEMAS. Socomfy workflow notesis registered (→workflow), and I documented thenotes[]shape incomfy_cli/schemas/workflow.json. Both additions are additive — norequired/additionalPropertiesconstraints changed, so existingslots/set-slot/varypayload validation is untouched.posandsizeare deliberately left type-unconstrained in the schema (description only): we pass them through verbatim, and litegraph has serializedposas both[x, y]and{"0": x, "1": y}across versions. Constraining toarraywould have made a real-world template fail its own schema.2. Pretty-mode output hardening. The plan's snippet did
rprint(n["text"])directly. Note text is untrusted file content, and this module already has a_strip_terminal_controlshelper for exactly that reason (used on/userdataresponses). A note containing a literal[/]would raiseMarkupErrorand crash the command; one containing\x1b[…could drive the terminal. So every interpolated field goes through a new_safe_console_text()(strip control chars →rich.markup.escape). Verified: a note whose body is\x1b[31mRED\x1b[0m and [bold]markup[/bold] and [/]renders literally, with no ESC bytes reaching the terminal. JSON mode is unaffected — it emits the raw text.Negative-claim falsification (API-format rejection)
This diff reuses
_load_workflow_or_fail, so an API-format file getsworkflow_not_frontend_formatand exit 1, and I added a test asserting that dead-end. Per the falsification rule I went and checked the premise empirically rather than trusting it:The conversion output contains none of
'ohwx','Note','MarkdownNote','inner doc','Trigger words', or'steps=30'— every note node and all note text is dropped (workflow_to_api._UI_ONLY_NODE_TYPESincludes both note types, and there is no_meta/extracarve-out that preserves them). I also grepped for any other comfy-cli surface that reads note text from an API file: none exists. So there is genuinely nothing to read from an API export — the rejection is correct, not a wrongly-shipped dead-end, and the user is already pointed atFile > Save (As)by the existing hint.Scope guards honored
Only
NoteandMarkdownNoteare surfaced —PrimitiveNode/GetNode/SetNode/Rerouteandgroups[]titles are explicitly excluded (both covered by tests). The command never writes the file (also tested), and never touches object_info or the network: no_get_graph, no--host/--port/--input.Unmet criteria
None.