fix: collapse duplicate server_died error-code registry entry (BE-5024)

[mattmillerai]

ELI-5

Two different pull requests each taught the CLI the same new error name, server_died, and neither one noticed the other. Git merged both without complaining (they touched different lines), so the list of error names ended up with the same name written twice. A test whose whole job is "no name appears twice" started failing — on main itself, and therefore on every open PR, regardless of what that PR changed. This deletes one of the two and merges the useful wording from both into a single entry.

What was wrong

comfy_cli/error_codes.py registered ErrorCode("server_died", ...) twice, so tests/comfy_cli/output/test_error_code_registry.py::test_no_duplicate_codes failed on main (e6965d0) with AssertionError: Duplicate codes in registry: ['server_died']. Every PR's build job inherited that failure — observed on #614, whose diff does not touch error_codes.py at all.

It was a semantic conflict between two cleanly-merged PRs: #605 (comfy run --wait writes job state at submit) and #604 (jobs: detect a dead local server). Neither touched the other's line, so nothing conflicted textually and the duplicate only surfaced at runtime.

The duplicate was also a live self-contradiction in the public contract, not just a red test: error_codes.get("server_died") returned the first entry while comfy discover emitted both, so the lookup and the published registry disagreed about what the code means.

What this changes

Collapses the two entries into one. Both describe the same scenario — a local ComfyUI server dying while a job is in flight — so the union is straightforward:

• Description: keeps fix(jobs): detect a dead local server and record a terminal server_died error (BE-4751) #604's fuller text (it names the crash / OOM / restart-without-the-job cases) and folds in fix: comfy run --wait writes job state at submit, names prompt_id on disconnect (BE-4750) #605's nuance, that the code is raised on the foreground --wait path too and is recorded on the job state file.
• Hint: merged so both actionable pointers survive — check the server log (it may have been OOM-killed), then comfy launch and re-submit; the prompt_id is in comfy jobs status <id>.

The error code string itself is unchanged, so nothing agents branch on moves. Only the human-facing meaning / hint prose changed.

test_no_duplicate_codes is deliberately not relaxed — the assertion is correct and is exactly what caught this contract collision.

Verification

• pytest tests/comfy_cli/output/test_error_code_registry.py — 6 passed (was failing on main).
• Full suite: 3074 passed, 37 skipped. On main the same suite is red on this one assertion.
• Both emitting call sites still resolve, checked explicitly: tests/comfy_cli/command/test_run.py (the --wait disconnect path in command/run/__init__.py, which sets wait_state.error = {"code": "server_died", ...}) and tests/comfy_cli/jobs/test_jobs.py (the comfy jobs watcher path from fix(jobs): detect a dead local server and record a terminal server_died error (BE-4751) #604, via _finalize_server_died in command/job_watcher.py) — 308 passed together with the output/ suite. The registry's own test_every_registered_code_is_raised / test_every_raised_code_is_registered pair independently confirms the code is still both registered and raised.
• Public surface checked empirically rather than assumed: load_error_codes() now yields exactly one server_died row carrying the merged text, and the registry holds 113 codes, 113 unique.
• ruff check + ruff format --diff clean at the version CI pins (0.15.15).

Judgment calls

• Kept the entry at fix(jobs): detect a dead local server and record a terminal server_died error (BE-4751) #604's position, after watcher_poll_error, rather than fix: comfy run --wait writes job state at submit, names prompt_id on disconnect (BE-4750) #605's slot. That groups the three watcher_* codes together and leaves server_died next to them. REGISTRY order is cosmetic (the module comments it as "ordered roughly by subsystem"; nothing indexes into it), so this is presentation only.
• No docs/json-output.md change. The ticket asked me to check for a stale second listing there — there is none. That table documents codes raised by comfy run, and server_died was never in it: on the run path the code is written to the state file while the emitted envelope code is ws_disconnected, which is already documented on line 348. The doc points at comfy discover for the full registry, and that output is now correct.
• Local ruff (unpinned, newer than CI's) reports 16 lint findings plus one file needing reformat. All of them reproduce identically on unmodified main with my change stashed, none are in a file I touched, and CI's pinned 0.15.15 is clean. Left alone as out of scope — flagging it because it will confuse anyone who runs a current ruff locally.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 38 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8e376e82-a2e5-4d78-bd9d-6513969b39e3

📥 Commits

Reviewing files that changed from the base of the PR and between e6965d0 and e3256ca.

📒 Files selected for processing (1)

• comfy_cli/error_codes.py

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

✅ No high-signal findings.

Panel: 8/8 reviewers contributed findings.

[bigcat88]

Approving, and this should go in ahead of everything else in the queue — main is red on this right now, so every open PR inherits the failure.

I'd independently found and root-caused this while re-verifying #614 and #611 yesterday/today, and reached the same conclusion you did: a semantic conflict between #605 and #604 that git merged cleanly because the two ErrorCode(...) blocks sit at different offsets in the tuple. Confirmed on a clean main checkout at e6965d0 with error_codes.py untouched.

Verified on this branch:

registry size: 113 | unique: 113 | dupes: none
server_died entries: 1
comfy discover: 113 codes, server_died rows: 1   (was 2)

The merged entry keeps every nuance from both — I checked each one rather than eyeballing the prose:

nuance	from	present
crash / OOM / killed	#604	✅
foreground --wait path	#605	✅
recorded on the job state file	#605	✅
hint: comfy launch + re-submit	#604	✅
hint: prompt_id in comfy jobs status	#605	✅

Both emitting call sites still resolve: command/job_watcher.py:315 (the #604 watcher path) and command/run/__init__.py:480 (the #605 --wait path). is_registered("server_died") is still True, so the registered/raised invariant pair holds from both directions.

Full suite: 2977 passed, 7 skipped, 1 failed — test_non_fast_deps_uses_global_python, the sandbox artifact that fails on plain main here too. test_no_duplicate_codes passes. ruff check / ruff format --check clean over 258 files.

Agreed on the two judgment calls. Not relaxing test_no_duplicate_codes is the right instinct — that assertion is the only thing that caught a contract collision two clean merges produced, and weakening it would have removed the guard rather than the bug. And keeping the entry at #604's position next to the watcher_* group is purely cosmetic since nothing indexes into REGISTRY.

One small correction to the description, for the record. It says error_codes.get("server_died") returned the first entry — it returned the last. _BY_CODE is built as {ec.code: ec for ec in REGISTRY}, so the later definition wins; I measured it on main and get() resolved to #604's "became unreachable / OOM" text, meaning #605's wording was the one being silently shadowed. Doesn't change the fix (you merged both), but it flips which description users have actually been seeing since #604 landed, so worth having right in the record.

Also confirming your docs/json-output.md call independently: server_died is genuinely absent from that table because the run path emits ws_disconnected in the envelope and writes server_died only to the state file. Nothing stale to clean up there.

The local-vs-CI ruff discrepancy you flagged reproduces for me too and is unrelated to this diff — right to leave it alone.