Skip to content

fix(1624): a repo filed under a Comfy Registry id is refused, not swapped for another author's - #1631

Draft
artokun wants to merge 4 commits into
mainfrom
fix/1624-rekeyed-unreachable
Draft

fix(1624): a repo filed under a Comfy Registry id is refused, not swapped for another author's#1631
artokun wants to merge 4 commits into
mainfrom
fix/1624-rekeyed-unreachable

Conversation

@artokun

@artokun artokun commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Closes #1624.

ComfyUI-Manager v4 does not resolve a from-source install by the URL you pass. It files
each channel entry under the pack's Comfy Registry id when the repo is registered, and
under the bare repo name only when it is not — so a repository registered under an id that
is not its own name is unreachable by that name, and something else answers instead.

#1619 refuses this only where the bare name is contested across the six channel lists.
The hazard needs no contest at all, which is what #1624 was split out to say.

The mechanism, read at tag 4.2.2

Read at 4.2.2, not at main. main is the 3.41 line — its install_by_id has
no CNR fallback whatsoever, so quoting main here would have described code that does not
run on any v4 host. (The channel URLs in channels.list.template do point at main,
which is why the generator still fetches the lists from there.)

# get_custom_nodes — comfyui_manager/glob/manager_core.py @ 4.2.2
cnr = self.get_cnr_by_repo(v['files'][0])      # repo_cnr_map: a PLAIN dict on a
if cnr:                                        # case-PRESERVING normalize_url
    v['id'] = cnr['id']; node_id = v['id']     # <- the CNR ID
else:
    node_id = v['files'][0].split('/')[-1]     # <- the bare repo name
res[node_id] = v                               # NormalizedKeyDict; get() lowercases

# install_by_id, nightly branch
the_node = custom_nodes.get(node_id)
if the_node is None and version_spec == 'nightly':
    cnr_fallback = self.cnr_map.get(node_id)
    repo_url = cnr_fallback['repository']      # <- CLONES THE REGISTRY'S REPO

The panel sends version: "nightly" for every git URL on the v2/v4 dialect
(manager-install.js), so that fallback is reachable from this tool, not hypothetical.

What was measured

Replaying that resolution over the six published channel lists plus the whole Comfy
Registry
(5117 repositories, fetched 2026-08-15):

repositories re-keyed (registered under an id ≠ their bare name) 1366
…of those, resolve to nothing — Manager answers "not found" 1268
…of those, resolve to a different repository 98
…on the channel this code defaults to 84
missed by #1619's contested-name table 56

The 1268 are deliberately not recorded. Manager returns Node '<name>@nightly' not found, installs nobody's code, and turning a clean failure into a refusal is a different
bug from this one.

Concrete examples of the 56, none of which needs a channel collision:

Mattabyte/ComfyUI-GGUF        registered "ComfyUI-GGUF_Forked"  -> city96/ComfyUI-GGUF
hekmon/comfyui-openai-api     registered "openai-api"           -> bgreene2/ComfyUI-OpenAI-API
1038lab/KittenTTS             registered "ComfyUI-KittenTTS"    -> neverbiasu/ComfyUI-KittenTTS
pamparamm/ComfyUI_IPAdapter_plus  registered "…_fork"           -> cubiq/ComfyUI_IPAdapter_plus

Why a precomputed verdict, and not the map #1624 asked for

#1624 proposed mirroring the registry's repository -> id and id -> repository maps
into the snapshot and simulating at call time, and costed that at ~5163 rows churning
continuously. The simulation is the right idea; shipping its input is not:

  • both directions of a 5117-row table exist to answer one yes/no question per repository,
    and that answer never varies per caller;
  • a runtime simulation would be a second implementation of Manager's keying, free to
    drift from the generator's.

So the generator runs the simulation once — against registry data it already fetched in
the same pass, for REKEYED_REPOS — and emits only the repositories whose answer is
"someone else": 98 records, not 5117 rows. Same verdicts, one implementation, and the
losing 1268 never enter the snapshot.

What it does

REKEYED_SUBSTITUTIONS is keyed by the caller's lowercased owner/repo and records the
id it is filed under, what each channel's list does answer to the name, and what
cnr_map[name] clones on a miss.

Where both tables know a repository (wildminder/ComfyUI-Chatterbox is in both), #1619's
message stays in charge; the two are pinned to agree on the landing.

What it deliberately does not cover

Keyed by the caller's repository, so it fires only for repositories the registry knows.
An unregistered, unlisted fork named ComfyUI-GGUF hits exactly the same substitution and
is not in here — that set is not enumerable from any snapshot. Those keep today's
behaviour and the standing dispatch note. Stated in the module, not implied.

Verification

  • Mutation-tested, six mutations, all killed. Two survived the first pass and got tests
    written for them: dropping the !ambiguity ordering guard (free on the refusal path,
    which returns at the first conflict — load-bearing on the warning path, where both
    messages ride one note), and dropping the "resolves to nothing" branch. The three
    wiring deletions each kill a distinct call-site test, so the one-line ifs are not
    invisible to the suite.
  • Full suite green: 517 files, 9696 passed, 3 skipped.
  • tsc --noEmit clean — and verified non-vacuous by planting a type error in the edited
    file and watching it fail.
  • Registry and channel data re-fetched live; the regenerated data file is pure
    additions
    (127 lines) — AMBIGUOUS_BARE_NAMES, REKEYED_REPOS and REGISTRY_TARGETS
    are byte-identical to what fix(1616): refuse the ambiguous from-source install instead of picking a repo #1619 shipped.
  • No browser verification, and none is implied. Nothing in the panel repo changes;
    this is a pre-dispatch decision in the orchestrator that either returns a conflict or
    the same args as before.

…pped for someone else's

ComfyUI-Manager v4 does not resolve a from-source install by the URL passed. It
files each channel entry under the pack's Comfy Registry id when the repo is
registered, and under the bare repo name only when it is not:

  get_custom_nodes  cnr = self.get_cnr_by_repo(v['files'][0])
                    if cnr: v['id'] = cnr['id']; node_id = v['id']
                    else:   node_id = v['files'][0].split('/')[-1]

So a repository registered under an id that is NOT its own name is unreachable by
that name from any channel, and on a nightly spec the miss falls back to the
registry pack whose id IS the name:

  install_by_id     if the_node is None and version_spec == 'nightly':
                        repo_url = self.cnr_map.get(node_id)['repository']

The caller names one author and gets another, reported as a success.

#1619 refuses this only where the bare name is CONTESTED across the six channel
lists. The hazard needs no contest. Replaying 4.2.2's resolution over the six
lists plus the whole Comfy Registry (5117 repositories): 1366 repos are re-keyed,
1268 of them resolve to nothing (Manager answers "not found" and installs nobody's
code — deliberately not recorded), and 98 resolve to a DIFFERENT repository, 84 on
the defaulted channel. 56 of those are invisible to #1619's table.

The generator now runs that simulation once and emits the verdicts —
REKEYED_SUBSTITUTIONS, 98 records — rather than the 5117-row map #1624 proposed
mirroring, so there is no second implementation of Manager's keying to drift.

Read at tag 4.2.2, not `main`: `main` is the 3.41 line and has no CNR fallback.

Refs #1624
Two mutations survived the first pass and now do not:

  * dropping `!ambiguity` from the from-source wiring — free on the refusal
    path, which returns at the first conflict, but on the WARNING path both
    messages ride the same `note` and a caller reads the finding twice with
    two different remedies;
  * dropping the "resolves to nothing" branch — that is Manager's "not found",
    which installs nobody's code, and refusing it would widen this past what
    was measured.

Refs #1624
Copilot AI balanced review requested due to automatic review settings August 15, 2026 22:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

artokun and others added 2 commits August 15, 2026 16:02
…annel

Gate finding against the first commit. The two registry-route returns above do
not leave "necessarily from-source" true for the re-keyed table the way they do
for the contested one: `rekeyedRegistryVersionAmbiguity` requires a
`registryTarget`, and 12 of the 98 re-keyed records have none. For those, an
explicit version fell through to the from-source refusal, which told the caller
the channel's list was what would be cloned -- on a route where Manager calls
`cnr_install(bare, version)`, reads no channel, finds no such id and fails.

Refusing was directionally defensible and the remedy it named was correct, which
is exactly how a fabricated mechanism survives review. Scoped to the route it
describes; Manager's own "not found" (which substitutes nobody's code) stands.

The nightly/unknown route test is now one exported predicate, `isFromSourceSpec`,
shared by all three call sites -- two copies that drifted would not fail loudly,
they would each describe the wrong route to somebody.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cords

The gate finding had two sides and the first commit only pinned one. The warning
is what a caller who NAMED the channel gets; it describes the same channel
resolution, so on a route where no channel is read it is exactly as fabricated as
the refusal -- and worse placed, because it rides a call that DISPATCHES, where
nothing later contradicts it.

Uses RUFFY-369/ComfyUI-StreamDiffusion on `dev` deliberately: the first URL tried
here was also a CONTESTED name, so #1616's registry-version check refused it a
step earlier and the test passed while exercising nothing of the warning path.

Counts corrected against the table rather than repeated: 26 of the 98 records
have no `registryTarget` (not 12). 12 of them are reachable on the defaulted
channel -- that is where the 12 came from -- and the other 14 answer only on a
channel the caller names explicitly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@artokun

artokun commented Aug 15, 2026

Copy link
Copy Markdown
Owner Author

Status: CI green, gate INDETERMINATE — holding, not merging

A gate finding was open and unfixed

The branch carried a regression test for a codex finding that had never been fixed — it failed against the committed code. Confirmed and closed here.

The finding. The two registry-route returns do not leave "necessarily from-source" true for the re-keyed table the way they do for the contested one. registryVersionAmbiguity covers every contested name a channel can answer to, but rekeyedRegistryVersionAmbiguity requires a registryTarget — and 26 of the 98 re-keyed records have none. For those, an explicit version fell through to the from-source refusal, which told the caller "the default channel's list DOES answer to X — so that is what would be cloned", on a route where Manager calls cnr_install(bare, version), reads no channel at all, finds no such id, and fails.

Refusing was directionally defensible and the remedy it named was even correct — which is exactly how a fabricated mechanism survives review.

The fix. The from-source check is scoped to the route it describes, via one exported isFromSourceSpec shared by all three call sites (two drifting copies would not fail loudly — each would just describe the wrong route to somebody). Manager's own "not found", which substitutes nobody's code, now stands.

Both sides of the split are pinned

The first pass covered only the refusal. The warning — what a caller who named the channel gets — states the same channel resolution, so it was equally fabricated on that route and worse placed, because it rides a call that dispatches, where nothing later contradicts it.

The first URL I picked for that test was also a contested name, so #1616 refused it a step earlier and the test passed while exercising nothing. It now uses RUFFY-369/ComfyUI-StreamDiffusion on dev, which is re-keyed, has no registry entry under its bare name, and is not contested.

Counts measured, not repeated

The inherited comment said "12 of the 98". Measured against the table: 26 have no registryTarget; 12 of those are reachable on the defaulted channel (that is where the 12 came from), and the other 14 answer only on an explicitly named channel.

Verification

  • Reverting both #1624 refusal returns → 4 tests fail.
  • Reverting only the new route gate → exactly the 2 new tests fail, nothing else.
  • Call site is reachable in production: panel_install_nodenodesInstallCommandArgs, short-circuiting on conflict.
  • tsc --noEmit clean; full suite 517 files / 9698 passing; CI green on 39541be.

Why this is not merged

node .claude/autopilot/codex-gate.mjs --worktree .claude/worktrees/wt-1624 --base origin/main --name 1631
[gate] INDETERMINATE — codex account quota exhausted. Merge HOLDS.   exit 2

Verified directly rather than taken from the gate's own detector — codex exec returns You've hit your usage limit … try again at Aug 19th, 2026 8:43 PM. So this is a genuine INDETERMINATE, not a stale-harness misread, and exit 2 does not authorize a merge. Left open as a draft; re-gate after the quota resets and merge on exit 0.

#1624 stays open — it is fixed on this branch, not on main.

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.

Manager v4 CNR re-keying + nightly fallback installs another author's repo, even when the channel lists yours

2 participants