Skip to content

fix(spec): mark DeviceStats.index and NodeInfo.essentials_category as nullable#13706

Merged
MillerMedia merged 3 commits intomasterfrom
mattmiller/spec-nullable-device-index-essentials-category
May 5, 2026
Merged

fix(spec): mark DeviceStats.index and NodeInfo.essentials_category as nullable#13706
MillerMedia merged 3 commits intomasterfrom
mattmiller/spec-nullable-device-index-essentials-category

Conversation

@MillerMedia
Copy link
Copy Markdown
Contributor

Summary

Two fields in openapi.yaml are declared non-nullable but the Python implementation legitimately returns null for them. This PR marks both fields nullable: true so spec-driven response validators don't fail on conformant responses.

DeviceStats.index (used by GET /api/system_stats)

server.py:677 emits \"index\": device.index unconditionally. For the CPU device (--cpu mode), torch.device(\"cpu\").index is None per PyTorch convention, so the JSON response includes \"index\": null.

The field stays in required — it's always present in the response. Only the value can be null.

NodeInfo.essentials_category (used by GET /api/object_info)

Two emitters disagree on this field:

  1. The legacy V1 path in server.py:739-740 only sets the key when hasattr(obj_class, 'ESSENTIALS_CATEGORY'). Field absent for nodes that don't set it.
  2. The V3 schema path in comfy_api/latest/_io.py:1654 unconditionally passes essentials_category=self.essentials_category into NodeInfoV1, then serializes via dataclasses.asdict(). The dataclass default is None, so for V3 nodes that don't set essentials_category in define_schema (e.g. APG, in comfy_extras/nodes_apg.py) the JSON response includes \"essentials_category\": null.

The field stays optional (already not in required). Marking it nullable lets the V3-generated null validate cleanly while still allowing the V1 "key absent" shape.

Reproduction

Stand up ComfyUI in --cpu mode and validate the responses against the current spec with any OpenAPI 3.1 response validator (e.g. kin-openapi's openapi3filter.ValidateResponse):

  • GET /api/system_stats → `response body doesn't match schema #/components/schemas/SystemStatsResponse: Error at "/devices/0/index": Value is not nullable`
  • GET /api/object_info → `response body doesn't match schema: Error at "/APG/essentials_category": Value is not nullable`

After this PR both validate cleanly.

Notes

  • No Python code changes — the runtime behavior is correct, only the spec was wrong.
  • Descriptions expanded to spell out exactly when null is expected, so future readers don't have to re-derive it.
  • Spec validates under kin-openapi after the change.

… nullable

Two fields in openapi.yaml are declared as required/non-nullable but
the Python implementation legitimately returns `null` for them, so any
client that response-validates against the spec will fail.

`DeviceStats.index` (used by GET /api/system_stats):
- server.py emits `"index": device.index` unconditionally
- For the CPU device (--cpu mode), `torch.device("cpu").index` is `None`
- → JSON response includes `"index": null` for CPU devices

`NodeInfo.essentials_category` (used by GET /api/object_info):
- The V3 schema-based path (comfy_api/latest/_io.py:1654) unconditionally
  passes `essentials_category=self.essentials_category` into NodeInfoV1
  and serializes via dataclasses.asdict(), so the key is always present
- Schema's `essentials_category` defaults to `None` for nodes that
  don't set it in `define_schema` (e.g. the APG node)
- → JSON response includes `"essentials_category": null` for those nodes
- (The V1 path in server.py uses `hasattr` and so omits the key
  entirely when not set, but the V3 path is the one that produces nulls)

Both fields keep their existing `required` status — they're always
present in the response, the value is just nullable. Descriptions
expanded to spell out when `null` is expected.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7db6ec7d-f58b-49d3-959f-245cfebc7ab4

📥 Commits

Reviewing files that changed from the base of the PR and between 5a316da and fdac249.

📒 Files selected for processing (1)
  • openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • openapi.yaml

📝 Walkthrough

Walkthrough

The OpenAPI spec was updated: components.schemas.DeviceStats.index is now nullable with an expanded description stating CUDA devices return their ordinal index while CPU and other no-index devices (including --cpu mode) return null. components.schemas.NodeInfo.essentials_category is now nullable and its description was expanded to explain when the key is omitted versus present as null, noting V1 vs V3 node behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: marking two OpenAPI schema fields as nullable to fix spec validation errors.
Description check ✅ Passed The description comprehensively explains the changes, root causes, affected endpoints, and reproduction steps with concrete code references.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Review rate limit: 1/5 review remaining, refill in 45 minutes and 17 seconds.

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@openapi.yaml`:
- Around line 2512-2515: Update the description for the essentials category
field to clarify V1 behavior: change the text that currently states it is
"`null` for nodes that don't set `ESSENTIALS_CATEGORY` (V1)..." to say the key
may be omitted or set to null in V1; reference the field name
`essentials_category` (and env var `ESSENTIALS_CATEGORY`) in the sentence so
clients understand legacy V1 can omit the key entirely as well as return null.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f6f73c5f-0c61-4200-a0b1-516ee7650c98

📥 Commits

Reviewing files that changed from the base of the PR and between c33d26c and 5a316da.

📒 Files selected for processing (1)
  • openapi.yaml

Comment thread openapi.yaml Outdated
The previous description said "null for nodes that don't set
ESSENTIALS_CATEGORY (V1)" — that's wrong. server.py:739-740 uses
`hasattr` and OMITS the key when the V1 attribute isn't defined; null
only happens if the attribute is explicitly set to None. Spell out
all three legal shapes (string / null / absent) and which path
produces which.
@MillerMedia MillerMedia merged commit 35819e3 into master May 5, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants