fix(build): a pull no longer deletes definition data silently

CHANGELOG.md

@@ -32,6 +32,13 @@ history.
   the closest pack the registry named for each one, every model the graph loads
   (a workflow import carries none of them), the classes served by a partner API,
   and whether a ComfyUI version still has to be pinned.
+- `comfy build pull` names what it would change before changing it: the same
+  definition diff `comfy build update` prints, echoed in the confirmation and
+  carried in the `--json` payload as `summary` and `diff`. A fetched Build that
+  omits `models` or `customNodes` drops the local entries, and the diff is where
+  that is now visible. `comfy build pull --dry-run` prints the diff and writes
+  nothing — with `--yes` the payload only arrives after the write, so this is
+  how a non-interactive caller reads the diff before deciding.
 - `CONTRIBUTING.md` (renamed from `DEV_README.md`) and this changelog.
 - `comfy deploy` — run a Build release as a serverless endpoint: `up`, `status`,
   `ls`, `show`, `logs`, `events`, `scale`, `stop`, `start`, `delete`, `run`, and

comfy_cli/command/build.py

@@ -61,7 +61,7 @@
     resolve_build_paths,
     resolve_local_path,
 )
-from comfy_cli.command.build_pull import merge_pulled_spec
+from comfy_cli.command.build_pull import UnsyncedDefinitionError, merge_pulled_spec
 from comfy_cli.command.build_push import (
     SkippedSymlink,
     pending_uploads,
@@ -2038,6 +2038,10 @@ def pull_cmd(
     ] = None,
     build_id: Annotated[str | None, typer.Option("--id", help="Pull this Build id instead of the spec's id.")] = None,
     yes: Annotated[bool, typer.Option("--yes", "-y", help="Overwrite the local spec without confirming.")] = False,
+    dry_run: Annotated[
+        bool,
+        typer.Option("--dry-run", help="Fetch the Build, print the diff, and leave the spec file untouched."),
+    ] = False,
     models_dir: Annotated[
         str | None,
         typer.Option("--models-dir", help="Models folder used to recompute model content identities."),
@@ -2074,28 +2078,55 @@ def pull_cmd(
         pulled = merge_pulled_spec(prepared.spec, remote, target_id)
     except NodePackageError as error:
         _raise_node_package_error(renderer, error)
+    except UnsyncedDefinitionError as error:
+        renderer.error(
+            code=error.code,
+            message=str(error),
+            details={"path": str(paths.spec_file), "id": target_id, "fields": list(error.fields)},
+        )
+        raise typer.Exit(code=1) from error
     except BuildSpecInvalidError as error:
         renderer.error(code=error.code, message=str(error), details={"path": str(paths.spec_file)})
         raise typer.Exit(code=1) from error
 
+    # Baselined on the definition on disk, never `prepared`'s: `prepare_push`
+    # recomputes model `sha256` and node `localDigest`, and this write lands
+    # those too, so diffing the reconciled copy would hide them.
+    diff = diff_definitions(spec["definition"], pulled.definition)
+    summary = summarize_definition_diff(diff)
+    if renderer.is_pretty():
+        render_definition_diff(renderer, diff)
+
     payload = {
         "spec_file": str(paths.spec_file),
         "id": target_id,
-        "syncedRevision": pulled["syncedRevision"],
+        "syncedRevision": pulled.spec["syncedRevision"],
+        "dry_run": dry_run,
         "written": False,
-        "definition": pulled["definition"],
+        "summary": summary,
+        "diff": diff.as_json(),
+        "definition": pulled.definition,
     }
     # `pull` carries the local localDigest forward into the spec it writes
     # (`build_pull._NODE_LOCAL_FIELDS`), so it mints the same committed identity
     # `init`, `update` and `push` do and owes the same skip report — but only
     # for the nodes that survive the merge, renumbered into the merged order.
     reported = _warn_skipped_symlinks(
-        renderer, _relocate_skipped_symlinks(prepared.skipped_symlinks, pulled["definition"])
+        renderer, _relocate_skipped_symlinks(prepared.skipped_symlinks, pulled.definition)
     )
     if reported:
         payload["skipped_symlinks"] = reported
+
+    # Before the confirmation, not after: --dry-run promises to write nothing,
+    # and a prompt whose only outcome is a write it will not perform is noise.
+    if dry_run:
+        if renderer.is_pretty():
+            renderer.info(f"--dry-run: {paths.spec_file} left untouched.")
+        renderer.emit(payload, command="build pull", changed=False)
+        return
+
     if not confirm(
-        f"Pull build {target_id} and overwrite the local spec at {paths.spec_file}?",
+        f"Pull build {target_id} and overwrite the local spec at {paths.spec_file} ({summary})?",
         yes=yes,
         error_code="build_pull_needs_confirm",
         ctx=ctx,
@@ -2106,7 +2137,7 @@ def pull_cmd(
         renderer.info("Aborted.")
         return
 
-    _write_spec(renderer, paths.spec_file, pulled)
+    _write_spec(renderer, paths.spec_file, pulled.spec)
     payload["written"] = True
     if renderer.is_pretty():
         renderer.success(f"Pulled build {target_id} → {paths.spec_file}")

comfy_cli/command/build_pull.py

@@ -9,7 +9,7 @@
 from typing_extensions import assert_never
 
 from comfy_cli.command.build_push import normalize_repository_identity
-from comfy_cli.command.build_spec import BuildSpecInvalidError, JsonObject, JsonValue
+from comfy_cli.command.build_spec import BuildSpecError, BuildSpecInvalidError, JsonObject, JsonValue
 from comfy_cli.command.build_validation import MODEL_SOURCES, NODE_SOURCES
 
 IdentityTier: TypeAlias = Literal["sha256", "model_location", "blobId", "id", "repository", "name"]
@@ -47,8 +47,28 @@
         "environment",
         "modelPolicy",
         "partnerNodePolicy",
+        "customNodePolicy",
     }
 )
+# Authoring-only fields. The builder has no typed field for either — zero
+# references in its `definition/` package — so no builder-owned write path can
+# produce one, and their absence says nothing about build state. It *can* echo
+# them: `Definition` is a free-form map stored and returned verbatim, so a Build
+# last written by `comfy build push` carries both back. The exemption is for the
+# other case — a Build last written by a client that has no field for them, from
+# which they return absent and would otherwise read as a missed round trip.
+_UNSYNCED_DEFINITION_FIELDS: Final = frozenset({"schema", "environment"})
+
+
+class UnsyncedDefinitionError(BuildSpecError):
+    code = "build_pull_unsynced_definition"
+
+    def __init__(self, fields: tuple[str, ...]) -> None:
+        self.fields = fields
+        super().__init__(
+            f"the fetched Build's definition omits {', '.join(fields)}, which the local spec sets; "
+            "pulling would delete them"
+        )
 
 
 def _entries(definition: JsonObject, collection: str, *, side: str) -> list[JsonObject]:
@@ -295,6 +315,28 @@ def _merge_collection(
     ]
 
 
+def _carries_data(value: JsonValue) -> bool:
+    """Whether losing *value* would actually lose anything.
+
+    An empty value is not evidence the build was never synced. The two create
+    paths that build a definition server-side — ``from_snapshot`` and
+    ``from_workflow`` — store it through ``ToMap``, which is ``json.Marshal`` of
+    a typed struct with ``omitempty``, so a pin-less snapshot's empty
+    ``pipDependencies`` is stored *absent* rather than blank. A local ``""``
+    against that Build then read as a missed round trip while there was nothing
+    to lose.
+
+    Non-empty values still refuse, which is the guard doing its job: scan-
+    captured pins are real data, and a Build that lacks them has not carried
+    them. Adopting such a Build still requires pushing first.
+    """
+    if value is None:
+        return False
+    if isinstance(value, (str, list, dict, tuple)):
+        return bool(value)
+    return True
+
+
 def merge_pull_definition(local: JsonObject, server: JsonObject) -> JsonObject:
     """Merge a server-owned definition onto local authoring/cache fields by identity."""
     merged = deepcopy(server)
@@ -303,28 +345,48 @@ def merge_pull_definition(local: JsonObject, server: JsonObject) -> JsonObject:
             merged[key] = deepcopy(value)
     merged["models"] = _merge_collection(local, server, "models")
     merged["customNodes"] = _merge_collection(local, server, "customNodes")
+    for key in _UNSYNCED_DEFINITION_FIELDS & set(local):
+        merged.setdefault(key, deepcopy(local[key]))
+    dropped = tuple(sorted(key for key in set(local) - set(merged) if _carries_data(local[key])))
+    if dropped:
+        raise UnsyncedDefinitionError(dropped)
     return merged
 
 
-def merge_pulled_spec(local_spec: JsonObject, remote: JsonObject, build_id: str) -> JsonObject:
+@dataclass(frozen=True, slots=True)
+class PulledSpec:
+    """The spec `pull` writes. ``definition`` is the very object under
+    ``spec["definition"]``, carried alongside so a caller that has to diff or
+    project it does not have to re-narrow it out of a ``JsonValue``."""
+
+    spec: JsonObject
+    definition: JsonObject
+
+
+def merge_pulled_spec(local_spec: JsonObject, remote: JsonObject, build_id: str) -> PulledSpec:
     """Return the atomically writable local spec for one fetched Build."""
     local_definition = local_spec.get("definition")
     server_definition = remote.get("definition")
     if not isinstance(local_definition, dict) or not isinstance(server_definition, dict):
         raise BuildSpecInvalidError("both the local spec and fetched Build need a definition mapping")
     name = remote.get("name")
+    # `description` is `*string` + `omitempty` on the builder, so an empty one
+    # arrives absent, not as `""`. Same state on the wire; default, don't refuse.
     description = remote.get("description")
+    if description is None:
+        description = ""
     revision = remote.get("updatedAt")
     if not isinstance(name, str) or not isinstance(description, str) or not isinstance(revision, str) or not revision:
         raise BuildSpecInvalidError("the fetched Build needs string name, description and updatedAt fields")
+    definition = merge_pull_definition(local_definition, server_definition)
     merged = deepcopy(local_spec)
     merged.update(
         {
             "id": build_id,
             "name": name,
             "description": description,
             "syncedRevision": revision,
-            "definition": merge_pull_definition(local_definition, server_definition),
+            "definition": definition,
         }
     )
-    return merged
+    return PulledSpec(merged, definition)

comfy_cli/error_codes.py

@@ -1146,7 +1146,21 @@ class ErrorCode:
         "build_pull_needs_confirm",
         "`comfy build pull` was run without `--yes` in a non-interactive context. Pull discards local "
         "definition edits in favor of the fetched Build, so the rewrite is refused without explicit consent.",
-        "pass `--yes` to overwrite the local spec with the fetched Build",
+        "pass `--yes` to overwrite the local spec with the fetched Build, or `--dry-run` to read the diff "
+        "without writing anything",
+    ),
+    ErrorCode(
+        "build_pull_unsynced_definition",
+        "`comfy build pull` refused a merge that would silently delete definition fields the local spec "
+        "sets to a non-empty value and the fetched Build omits. `details.fields` names them. A Build that "
+        "carries the field as an empty value is an intentional clear and is applied normally; so is an "
+        "absent field whose local value is already empty, because the builder's server-side create paths drop "
+        "empty fields on store and their absence is therefore not evidence of a missed round trip. "
+        "`definition.schema` and `definition.environment` are exempt: the builder has no typed field for "
+        "either, so no builder-owned write path can produce one. The check covers definition fields other "
+        "than `models` and `customNodes`, which are reconciled entry by entry -- a Build that omits a "
+        "collection still empties it locally.",
+        "run `comfy build push` so the Build carries these fields, or delete them from the spec if the Build is authoritative",
     ),
     ErrorCode(
         "build_spec_stale",

comfy_cli/schemas/build_pull.json

@@ -2,14 +2,53 @@
   "$schema": "https://json-schema.org/draft/2020-12/schema",
   "$id": "https://comfy.org/schemas/build_pull.json",
   "title": "comfy build pull --json data payload",
+  "description": "The definition the fetched Build would write or wrote, and the diff against the definition on disk.",
   "type": "object",
-  "required": ["spec_file", "id", "syncedRevision", "written", "definition"],
+  "required": ["spec_file", "id", "syncedRevision", "dry_run", "written", "summary", "diff", "definition"],
   "additionalProperties": false,
+  "$defs": {
+    "collection": {
+      "type": "object",
+      "required": ["added", "removed", "changed", "entries"],
+      "additionalProperties": false,
+      "properties": {
+        "added": {"type": "integer"},
+        "removed": {"type": "integer"},
+        "changed": {"type": "integer"},
+        "entries": {
+          "type": "array",
+          "items": {
+            "type": "object",
+            "required": ["change", "name", "fields"],
+            "additionalProperties": false,
+            "properties": {
+              "change": {"enum": ["added", "removed", "changed"]},
+              "name": {"type": "string"},
+              "fields": {"type": "array", "items": {"type": "string"}}
+            }
+          }
+        }
+      }
+    },
+    "status": {"enum": ["changed", "unchanged"]}
+  },
   "properties": {
     "spec_file": {"type": "string"},
     "id": {"type": "string", "minLength": 1},
     "syncedRevision": {"type": "string", "minLength": 1},
+    "dry_run": {"type": "boolean"},
     "written": {"type": "boolean"},
+    "summary": {"type": "string"},
+    "diff": {
+      "type": "object",
+      "description": "What this pull changes in the local definition, measured against the copy on disk. Per-category counts plus the affected entries, so an agent never parses the table. Collections carry counts and entries; every other definition key carries a bare changed/unchanged status. A `models` or `customNodes` entry the fetched Build omits is reported as `removed`: the collections follow the server's list rather than being merged with the local one.",
+      "required": ["models", "customNodes"],
+      "additionalProperties": {"$ref": "#/$defs/status"},
+      "properties": {
+        "models": {"$ref": "#/$defs/collection"},
+        "customNodes": {"$ref": "#/$defs/collection"}
+      }
+    },
     "skipped_symlinks": {
       "description": "Symlinks excluded from a packaged custom node; present only when packaging skipped at least one. `location` points into this payload's definition, `localPath` names the node under custom_nodes/, and `member` is the symlink's path inside that node.",
       "type": "array",