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

[guill]

Stack of 7 — review bottom-up (this is #810)

1. feat(build): add packaging, sync and Builder client primitives #801 — feat(build): packaging, sync and Builder client primitives — base main
2. feat(build)!: replace the legacy build and distribution surface #802 — feat(build)!: replace the legacy build and distribution surface (breaking)
3. feat(deploy): add the deployment control plane #803 — feat(deploy): the deployment control plane
4. feat(deploy): add the asset and job data-plane clients #804 — feat(deploy): the asset and job data-plane clients
5. feat(deploy): add comfy deploy run #805 — feat(deploy): comfy deploy run
6. fix(build): a pull no longer deletes definition data silently #810 — fix(build): a pull no longer deletes definition data silently   ← this PR
7. fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10 #824 — fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10

Each PR is based on the branch below it, so its own diff is only its commits. Merge in order.

TL;DR: comfy build pull deleted definition data the fetched Build did not carry, without a word. Two commits: it now refuses the case it can prove is data loss, and reports everything else it is about to change. Stacked on #805.

The bug

merge_pull_definition starts merged as a copy of the server definition and only carries back local keys outside _DEFINITION_KNOWN_FIELDS. A known-but-absent field — environment, pipDependencies, a policy — was dropped from the spec silently. So were models and customNodes, by a different route (below).

---

1. 148199e — refuse a pull that would silently delete definition fields

merge_pull_definition diffs the local key set against the merged one and raises UnsyncedDefinitionError when a top-level scalar would disappear, naming the fields in details.fields. New code build_pull_unsynced_definition; pull_cmd renders it and exits 1, before any write.

• definition.schema is exempt. It names the spec's own file format, not build state, so the builder has no concept of it and its absence server-side says nothing. Restored from local rather than refused — the one member of _UNSYNCED_DEFINITION_FIELDS.
• description gets the opposite treatment, and this is a straight bug fix. It is *string + omitempty on the builder, which collapses "" to a nil pointer, so an empty description genuinely arrives absent. It now defaults to ""; previously the isinstance(description, str) check rejected the None outright, so pulling any Build with no description failed before reaching the merge at all.
• customNodePolicy joins _DEFINITION_KNOWN_FIELDS, which already held modelPolicy and partnerNodePolicy. Missing from the set, it was carried back from local unconditionally, so a policy the server cleared came straight back on the next pull — and it alone among the three would have escaped the new check.

Correction to an earlier revision of this description. It claimed the builder "stores a Build's definition verbatim, so the typed struct's omitempty tags never touch it," and therefore that an absent field is always one that never round-tripped. That is only true of two of the server's write paths. CreateBuild and UpdateBuild do store the raw map[string]interface{}, but workflows.go:56, snapshots.go:62, from_snapshot.go:64 and from_workflow.go:60 all store via definition.ToMap, which is json.Marshal of the typed struct — every field omitempty. A build imported from a workflow can therefore store as little as {"customNodes":[...]}.

Two consequences, both real and both verified against the builder source:

1. Absence is ambiguous for any omitempty field — "never carried" and "deliberately cleared" are byte-identical on the wire. The refusal above will have false positives, and the "an empty value is an intentional clear, only an absent key is refused" rule does not hold for a definition that came through a ToMap door.
2. Adopting such a build into a scan-initialized spec (comfy build init always writes environment and pipDependencies) hits the refusal, and the hint's recovery does not work: a plain comfy build push creates a new build, and push --id X sends expectedUpdatedAt: null, which unmarshals to Go's zero time.Time and can never match UpdateTimeEQ — a guaranteed 409 STALE whose own hint points back at pull.

Not fixed here. Narrowing the refusal is a separate change; this PR's second commit makes the loss visible, which is the half that does not depend on resolving the ambiguity. Filed for follow-up.

---

2. 39abbe1 — say what a pull changes before it changes it

The check above can only ever see top-level scalars. merge_pull_definition assigns merged["models"] and merged["customNodes"] unconditionally, and only then computes dropped — so the two collections are structurally incapable of reaching it. A fetched Build whose definition omits them still wiped every local entry, dropping the local localPath, source, sha256 and localDigest rows and exiting 0.

-y is not what made that silent. The guard runs ~30 lines before confirm() is consulted, so the scalar half already refuses regardless of --yes. And the interactive path asked only "Pull build X and overwrite the local spec at PATH?" — naming no consequence at all. There was no warning for -y to suppress.

pull now reports, the way build update already does:

• diff_definitions runs against the definition on disk — deliberately not prepare_push's reconciled copy, which recomputes model sha256 and node localDigest, values this write also lands, so diffing it would hide them.
• render_definition_diff prints the change table in pretty mode.
• The summary is echoed inside the confirmation: Pull build X and overwrite … (models +0 -1 ~0, customNodes +0 -1 ~0, baseComfyVersion)?
• summary and diff ride in the --json payload; build_pull.json gains them plus dry_run.
• --dry-run, for the reason update has one: with --yes the payload only arrives after the write, so a non-interactive caller had no way to read the diff while it could still act on it. It short-circuits ahead of the confirmation, and build_pull_needs_confirm now names it alongside --yes.

merge_pulled_spec returns a PulledSpec carrying the merged definition beside the spec — indexing a JsonObject yields JsonValue, which is not a Mapping[str, JsonValue], and the function already knows the type it built.

This reports the collection drop rather than refusing it. Whether an absent collection should also be refused is a separate question from whether it should be visible, and the answer to the second is not "no".

Validation

• Full suite: 6802 passed, 38 skipped. tests/comfy_cli/command/ + tests/comfy_cli/output/: 3749 passed, 3 skipped.
• ruff check . / ruff format --check . clean at CI's pin (0.15.15).
• Pyright clean on both changed modules (build.py:893 is pre-existing, from e4f661f).
• Manual end-to-end, real binary against a stub builder returning a ToMap-shaped definition: --dry-run renders the table and leaves the file byte-identical; the prompt carries the summary; declining aborts without writing; the --json payload validates against the shipped schema.
• Nine tests on pull total — four new: the collection wipe is reported, the confirmation names the change, --dry-run writes nothing, and --dry-run does not prompt a TTY caller.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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: e5a7903f-71bb-445a-ad33-994ed68a505e

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

---

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

[skishore23]

Re-reviewed against the builder source and I need to walk back the earlier approval — the guard's premise only holds for one of the server's write paths.

1. (high) build_pull.py:44,243-245 — the guard refuses every adopt/rebind pull from a scan-initialized spec, and the hint's recovery duplicates or 409s. comfy build init (scan path, build.py:552-558) always writes pipDependencies and environment. The builder has no concept of environment at all (zero non-comment references in comfy-builder Go). And "omitempty never touches the definition" is true for UpdateBuild/create (verbatim map[string]interface{}, builds_update.go:64, create.go:104) but not for the four other doors — workflows.go:56, snapshots.go:62, from_snapshot.go:64, from_workflow.go:60 all store via definition.ToMap, which is json.Marshal of the typed struct with omitempty, so an empty pipDependencies (definition.go:70) or policy is stored absent. Scenario: comfy build init → comfy build pull --id X -y to adopt a build created from the web/snapshot/workflow (the cross-id flow test_cross_id_pull_rebinds... pins) → exit 1, fields: [environment, pipDependencies]. Following the hint: comfy build push with id: null creates a new build instead of adopting X; push --id X sends expectedUpdatedAt: null → Go zero time → UpdateTimeEQ matches nothing → 409 stale, whose hint says pull again. The only non-clobbering exit is hand-deleting the keys. Fix direction: exempt environment alongside schema (both authoring-only from the server's view), and don't treat absence as "never synced" for a definition that can come from a ToMap door — or gate the refusal on the build having been last written by the CLI.

2. (medium) build_pull.py:239-240 — an absent models/customNodes still silently wipes every local entry, the same data-loss class this PR fixes. Reproduced: local spec with one local model + one local node, server definition {"baseComfyVersion": "v0.3.0"} (what a from-workflow build with no models looks like after ToMap) → pull -y exits 0, written: true, models: [], customNodes: [], and the local localPath/sha256/localDigest authoring data is gone with no warning. You flagged this as a follow-up, but the registry text for build_pull_unsynced_definition tells agents the check refuses "definition fields the local spec sets and the fetched Build omits" without the collections caveat — at minimum the text should say so.

Everything else verified: description really does arrive absent (*string,omitempty, toBuild sets it only when non-empty), so the "" default is right; all three policies are consumed at freeze (freeze.go:247-249) so customNodePolicy belongs in _DEFINITION_KNOWN_FIELDS; the refusal happens before any write; details.fields is a list; the push/pull bounce is not a deadlock (push --force or deleting the field exits it).

Tests on the branch (typer < 0.22 venv): 521 passed, 1 skipped; ruff clean.