Skip to content

fix(knowledge): check an oss pick's weights before recommending it (BE-8974) #874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
annehe9 wants to merge 2 commits into from
Closed

fix(knowledge): check an oss pick's weights before recommending it (BE-8974) #874

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58a30d9
fix(knowledge): check an oss pick's weights before recommending it (B…
annehe9 Sep 15, 2026
768d83d
fix(knowledge): walk past any non-runnable pick, not just missing-models
annehe9 Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions comfy_cli/skills/comfy/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@ rather than an answer to anything. Five rules:
it resolves to. A row flagged this way also pulls in `picks[]` for the
capabilities that rank it, so the highest-ranked entry *without* the flag is
the runnable alternative. Say what is missing, then name that alternative.
The flag never looks at model files. When routing is local, run

@github-actions github-actions Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — The step is unconditional, but templates check lists /models/<folder> on the local server whenever the template declares models, so it hard-fails with server_not_running (exit 1) on the very common "installed but not launched" machine — local routing means no cloud credentials, not a running server. Give the rule a branch for the server being down, and note that templates check has no --where flag, so the global error-code table's "switch to --where cloud" advice does not apply here. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).

`comfy --json templates check <template>` on an `oss` pick's `template`

@github-actions github-actions Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High — The template value comes from the remotely fetched knowledge bundle and is only type-checked (pick_entry does _text(p.get("template"))), yet this line tells the agent to splice it straight into a shell command; a value containing ;, $(...), backticks, a leading -, or whitespace either breaks the lookup or executes arbitrary shell before templates check ever validates the name. Carry the same escaping caveat Rule 1 already has for user-supplied words: pass it as one quoted argument after --, and reject anything that is not a plain template name. Raised by 3 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).

@github-actions github-actions Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Low — Scoping the check to oss picks silently skips other locally-runnable picks: pick_entry passes route through _text (yielding None for a missing or non-string value) and the bundle also uses route: "both", neither of which matches oss, so those picks keep a template but get recommended with no model verification. Broaden the condition to any pick that is not cloud-only. Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).

before recommending it. On `missing-models` the pick is still the answer:

@github-actions github-actions Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — The procedure branches only on missing-models and runnable, but _compute_verdict also returns api-required and unknown. api-required takes precedence over everything (including for an oss pick whose template merely matches the api_ name prefix or carries an API tag, which also hides missing files), and unknown comes back for a loader-ish template that declares no models; with no branch for either, the walk can exhaust every pick with no instruction for what to report, and api-required deserves a callout since running it spends credits. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).

name it, say it is not installed, and list `models.missing[]` with each

@github-actions github-actions Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — models.missing[] is conservative, not proof of absence: templates check marks a file missing when its model folder 404s on the local server (e.g. a custom-node folder such as SEEDVR2) or when directory is not a single addressable path segment, and records that qualification in data.warnings[]. Telling the agent to flatly "say it is not installed" converts those into false negatives about files that are present — require reading warnings[] before asserting the negative. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).

file's `directory`. Any other result that is not `runnable`, including a
`template_not_found` error, means that pick is not confirmed to run here.
Either way, check the next `oss` pick down and name the first `runnable` one
as what works today, or say that none is. Ask before downloading anything. A
pick with no `template` has nothing to check.
3. **Verify before denying.** A missing `knowledge` key or a `nudge` means
nothing is curated for that query, not that it is unsupported. A `nudge` on
a block that still carries rows means your search term matched nothing
Expand Down
Loading