mirror of
https://github.com/Comfy-Org/comfy-skills.git
synced 2026-11-04 23:37:45 +00:00
[PR #19] fix(skills): reword partner_generate routing to guess-and-bounce (BE-8057) #19
Labels
No labels
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Comfy-Org/comfy-skills#19
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
📋 Pull Request Information
Original PR: https://github.com/Comfy-Org/comfy-skills/pull/19
Author: @mattmillerai
Created: 8/20/2026
Status: 🔄 Open
Base:
main← Head:fix/partner-generate-guess-and-bounce📝 Commits (7)
5191de1fix(skills): reword partner_generate routing to guess-and-bounceb0a019ffix(skills): retry the slug partner_generate names, and port to the plugin3e8cde6fix(skills): carry the bounce-retry qualifiers into generate-video/image/audio56f4893Merge pull request #33 from Comfy-Org/matt/be-10663-bounce-retry-qualifiersd3d0353Merge origin/main into fix/partner-generate-guess-and-bounce2c477fffix(generate): retry a bounced partner slug only within the guessed provider58eb943fix(generate): stop after the single partner_generate re-call📊 Changes
6 files changed (+7 additions, -7 deletions)
View changed files
📝
claude-code/commands/generate-audio.md(+1 -1)📝
claude-code/commands/generate-image.md(+1 -1)📝
claude-code/commands/generate-video.md(+1 -1)📝
skills/comfy-generate-audio.md(+1 -1)📝
skills/comfy-generate-image.md(+1 -1)📝
skills/comfy-generate-video.md(+2 -2)📄 Description
ELI5
The image, video and audio instructions told the assistant to pick a model from a list the paid-provider generator publishes — but that list never existed, so the assistant gave up on the fast paid route. Now it guesses a model name, and if the guess is wrong the service replies with the closest real names, so it retries once with one of those and says which one it switched to. It only retries with a name from the same provider the user asked for; if the closest names belong to a different provider, it asks the user first instead of charging them for a provider they never picked.
The false premise
skills/comfy-generate-{video,audio,image}.mdeach pointed atpartner_generate's tool description "for the currently-wired model ids". The registered description (Comfy-Org/comfy-cloud-mcp-serversrc/tools-execution.ts:1097,main@2fbbb8ae— re-read at authoring time, verbatim) does not contain one:Providers, one example slug, and an explicit instruction to guess. No model ids, and per the ticket's research none ever.
What changed
Three lines, one per file, all on
skills/:skills/comfy-generate-video.mdpartner_generate's own tool description lists" → "one of the providerspartner_generatenames, or you can guess a plausible slug for it"; "serves only the slugs it lists" → the bounce-corrects-you framing.skills/comfy-generate-audio.mdskills/comfy-generate-image.mdReview round (commit
b0a019f)Two gaps surfaced in review, both fixed:
a. Retry the bounce. The reword told the agent a wrong slug "bounces back naming the closest real slugs" but never said to use them — so the guidance stopped one step short of the behavior it argued for: guess, receive a correction, then fall through to the workflow path anyway. All three skills now say to retry once with the closest slug named, and their fall-through narrows to
"unknown model"with no close match named (the wording the video bullet already used downstream). Raised by CodeRabbit; valid and in scope.b. Ported to the canonical surface. See the revised Residual section below — the original scope call is reversed, deliberately.
Everything else in each Step 0 is byte-identical (apart from
main's later "build/create a workflow → skippartner_generate" sentence, carried through the merge described below), including the"unknown model"/"not yet implemented"fall-through in audio/image and the video bullet'sapi_*-template-via-run_templaterecovery, which is load-bearing (MiniMax H3's r2v/flf2v shapes route that way per the file's own model-facts section). The MiniMax model-facts bullets rewritten by #18 are untouched, as isdata/skills.jsonincomfy-cloud-mcp-server(the mirror — patching it is reverted byrefresh-skills.yml).Bounce-retry qualifiers (#33, merged into this branch as
3e8cde6)Stacked PR #33 extended the retry sentence in all six files (plus the Hailuo/MiniMax bullet in
skills/comfy-generate-video.md) with the three qualifiers from the mergedcomfy-cloud-mcp-serverrouting bullet: the rejection happens before queuing and names its suggestions asDid you mean: …. A tier-shaped guess must be retried with the namedparams.modeltier, because re-calling the bare slug runs a differently priced default tier. The substituted slug must be stated, since suggestions are ranked by token overlap across the whole registry. Insidesubmit_batch, only the bounced item is resent, because its siblings may already be queued and charged.generate-3dstays untouched on purpose: with an empty 3D registry the server returns its does-not-serve-type redirect before any "Did you mean" bounce. The net diff is now 6 files, 7 insertions and 7 deletions.Same-provider retry gate (
2c477ff)CodeRabbit pointed out that the
Did you meansuggestions are ranked across the whole registry, so the top one can come from a provider the user never named, and the retry is the call that queues and charges. In all six Step 0 blocks the retry now fires only when the suggested slug keeps the guessed slug's provider prefix. Otherwise the agent stops and asks which model to use. The MiniMax/Hailuo bullet carries the same limit: it retries only with aminimax/slug. This matches the server's own unknown-model routing bullet, which also gates the re-call on a suggestion for the guessed provider.One re-call, then stop (
58eb943)CodeRabbit pointed out that none of the six Step 0 blocks said what to do when the single re-call bounces again. Each block now says: do not re-call a second time, and handle it as "unknown model" with no close match named. That reuses the fallback each file already defines.
Merge with
main(d3d0353)mainhad since appended one sentence to the same Step 0 / partner-only bullet in all six files:partner_generatereturns only the output, never a workflow, so a request to BUILD or CREATE a workflow skips it. The conflict was resolved by keeping this PR's guess-and-bounce wording and appendingmain's sentence verbatim, so both intents survive. The net diff againstmainis unchanged: 6 files, 7 insertions, 7 deletions.Verification — guess-and-bounce actually works
comfy-skillshas no test or lint toolchain (nopackage.json,Makefile, or pre-commit; its only CI workflow is the SOC 2 unreviewed-merge detector), so the substantive check is the prose against the server. Probed in-process against thecomfy-cloud-mcp-serverclone at2fbbb8aeviapartnerUnknownModelMessage()— the pure message builder, no dispatch, no credits, no network:kling/kling-3-turbo(video)unknown model … **Did you mean: kling/kling-3.0-turbo-t2v, kling/kling-v3-t2v?**openai/gpt-image-2(image)unknown model … **Did you mean: openai/images-generations, bfl/flux-2-max, bfl/flux-2-pro, …?**elevenlabs/eleven-v3(audio)unknown model … **Did you mean: elevenlabs/sound-generation?**minimax/hailuo-02,openai/sora-2(video)unknown model …with no "Did you mean", falling through toDiscover registered partner models with search_models (they carry source:"partner") or search_nodes (category "partner/").So a wrong slug does cost one call and does name the closest real slugs — across all three modalities the edited files cover. Note the plural: the bounce names up to five, which is why the audio/image wording says "slugs".
Negative-claim falsification
The diff keeps a capability denial in the video bullet — "Many paid video families still are not served by it and come back
"unknown model"with no close match named" — so per the#581regression rule I went and tried to break it rather than assert it:src/partner/client/registry/, whatpartner_generateresolves against) holds 20 slugs total: 15 image, 4 video, 1 audio. The four video ones arebyteplus/seedance-2.0-t2v,kling/kling-3.0-turbo-t2v,kling/kling-v3-t2v,veo/veo-3-t2v.src/partner/workflows/coversbfl,vertexai,xaionly.unknown modelwith no close match, as tabled above.The denial is therefore true as written and bounded: it is scoped to "many … families", not to the tool, and the sentence immediately after it (unchanged) redirects to a route that does work — the family's own
api_*template viarun_template. The net direction of this PR is capability-expanding: it removes a false premise that was causing agents to skippartner_generatealtogether.One nuance I did not fold in, deliberately: on a no-close-match bounce the server still points at
search_models({source: "partner"})as a live discovery path. The tool description tells agents to "skip the lookup", and the existingrun_templateredirect already keeps the branch from being a dead end, so adding a third pointer here would both contradict the tool and exceed this ticket's scope.Residual — scope reversed: the plugin surface IS now patched
The original revision of this PR deliberately touched
skills/only, and this section argued theclaude-code/fork should be left alone because partially patching a stale file leaves it internally inconsistent. That call is reversed inb0a019f, for a reason found in review and not visible when the original scope was set:So the original three-file change landed only in the frozen copy. The fix shipped to nobody who installs the plugin.
Worse, the two commands files were not merely stale on this one span — they still gated partner routing on
`category: "api node/"`, the dead prefixcf7949dremoved fromskills/after establishing it "matches ZERO nodes — the live catalog files them underpartner/". On the installed plugin the partner shortcut therefore could never fire: every named provider failed the category check and fell through to the workflow path, which is precisely the failure mode this PR exists to fix, one layer down. Porting the wording without that retarget would have been inert.b0a019ftherefore ports Step 0 intoclaude-code/commands/generate-{audio,image,video}.mdand retargetsapi node/→partner/in the same sentence.generate-audio.md:9andgenerate-image.md:9are now byte-identical to theirskills/counterparts, so the "internally inconsistent" objection does not apply to them at all;generate-video.md:11remains a partial patch of a file still missing the#15/#17/#18material, and that is the one place the original objection still has force — flagged rather than papered over.Still deliberately unfixed, deferred to follow-ups:
claude-code/commands/generate-3d.md— still carriedapi node/3d, the same dead prefix.mainhas since retargeted it, so after the merge noapi node/reference remains anywhere in the repo.skills/↔claude-code/commands/drift — the MiniMax H3 route disambiguation, theapi_*-template recovery, the "Family exists only as OSS" bullet, and the OSS-vs-paid consent prompt. This needs a human decision, not a sync: README's authoring rule ("never hard-code … which model names are live") is in direct tension withskills/comfy-generate-video.md, which names MiniMax H3, Hailuo and specific node classes inline. Blind-syncing would push the canonical surface against its own stated rule. CodeRabbit's request to give audio/image the video skill's both-routes-and-ask treatment falls inside this same decision and is deferred with it.Refsstill stands rather thanCloses: the sync-or-delete question remains open.Judgment calls
"unknown model"in the video bullet rather than dropping the denial entirely, on the falsification evidence above.skills/" scope call in the review round, on the README evidence thatskills/is the frozen copy andclaude-code/commands/the canonical one — a skills-only fix ships to nobody. Confined to Step 0 of the three files this PR already edits; the rest of the fork's drift is deferred, not swept in.api node/→partner/sits in the very sentence being rewritten, and the ported guidance is dead without it. Splitting it into its own PR would have merged known-inert prose.Unexercised
partner_generatewas never actually called. Verification is read/pure-function only (registry contents + the message builder); dispatching a real partner generation spends credits and money, so it is out of bounds for this loop. The claim under test — what the bounce says on a wrong slug — is fully determined bypartnerUnknownModelMessage, which is what I ran.comfy-cloud-mcp-servermain@2fbbb8ae, the commit the ticket names and the currentorigin/mainat authoring time. If the tool description moves again this prose needs a re-read.Provenance
claude plugin validate ./claude-codeprints "Validation passed" on the current head (58eb943).claude-code/commands/generate-{audio,image}.md:9is byte-identical toskills/comfy-generate-{audio,image}.md:5.grep -rn "api node/"finds no hits anywhere in the repo.git diff --shortstat origin/main...HEAD= 6 files, 7 insertions, 7 deletions (in-line rewording only).npx tsxprobe ofpartnerUnknownModelMessage()and the partner registry atcomfy-cloud-mcp-server@2fbbb8ae, with no dispatch.skills/files to theirclaude-code/commands/twins. The rationale is in Residual.mainwas merged in to clear a conflict. Its new "build a workflow → skippartner_generate" sentence is kept alongside this PR's wording.partner_generatewas never actually called (see Unexercised).Refs BE-8057, BE-6922.
🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.