feat(models): auto-suggest abilities from curated catalog#353
feat(models): auto-suggest abilities from curated catalog#353martinma51 wants to merge 1 commit into
Conversation
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Review Summary
This PR is well-designed overall — the three-tier matching logic (exact → wildcard_provider → provider fallback) correctly handles the common case of third-party models exposed through OpenAI-compatible endpoints. The lazy loading with double-checked locking and frontend graceful degradation are also solid.
However, after verifying each model against official documentation and checking the frontend integration, I found 3 critical data errors in the YAML catalog, 2 functional frontend gaps, and 3 minor issues that need addressing before merge.
Issues best discussed in the review body (span unmodified code)
Major: Auto-suggest missing in non-Connect mode
applyAbilitySuggestion() is only called in the Connect Wizard (Step 3), but the dialog also supports a direct "Create/Edit Model" form mode where users manually fill in model_name. In that mode, changing model_name never triggers a catalog lookup, so the auto-fill feature is completely unavailable to users who don't go through the wizard. Please add applyAbilitySuggestion() calls to the model_name onChange/onValueChange handlers in the form-mode UI as well.
Minor: Missing unit tests for model_catalog.py
The lookup() function is the core of this feature and currently has zero test coverage. Please add tests for:
- Exact provider + model match
- Wildcard provider match
- Provider fallback (
*) match - No-match (
source="none") case - Case-insensitive matching
fnmatchwildcard behavior (e.g.,gpt-4o*matchinggpt-4o-mini)- YAML reload via
reload_catalog()
|
pre-commit failed, you can run |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Re-review findings
Re-reviewed the full PR after all fix commits (aa1b5ff). The original 8 review issues are all properly resolved. Found 2 additional minor issues:
- Form mode missing ability suggestion hint —
applyAbilitySuggestionis called andabilitySuggestionstate is set in form mode (viahandleEditand model_name change handlers), but the hint text ("Pre-selected based on..." / "We don't have ability info...") is only rendered in the Connect wizard section (hunk@@ -722,6 +776,22 @@), never in form mode. - Redundant YAML rule —
dashscope/qwen2.5-vl*at file line 162 is dead code, always shadowed bydashscope/qwen*-vl*at file line 156.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
All previous review issues resolved. Minor fix for ability suggestion clearing on provider change pushed. LGTM.
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
Review findings:
Finding 1 (docker-compose.override.yml:1-26) [added]
[P1] Don’t commit an auto-loaded Compose override
docker-compose.override.yml is automatically merged by Docker Compose, and the README’s quick start tells users to run docker compose up -d. With this file checked in, every fresh clone/deployment now builds xagent-backend:local and xagent-frontend:local from the checkout instead of pulling the pinned release images from docker-compose.yml, which changes the default install/upgrade path and can fail or take much longer on machines that only expect to run the published images. Please move this to an explicitly named dev file such as docker-compose.dev.yml and document docker compose -f docker-compose.yml -f docker-compose.dev.yml ..., or keep the override untracked.
Finding 2 (frontend/src/app/tools/page.tsx:871-880) [added]
[P2] Keep the tool policy toggle admin-only
This button is now rendered for every user, but the backing PUT /api/tools/{tool_name}/enabled endpoint still rejects non-admins with 403. Normal users will see an enable/disable control that always fails with an admin-privileges error. Please restore the isAdmin guard around the toggle button, or otherwise hide/disable it for non-admin users.
Finding 3 (src/xagent/web/api/tools.py:229-233) [added]
[P2] Don’t filter disabled tools out of the listing path
ToolFactory.create_all_tools(tool_config) now applies get_user_tool_overrides() before /api/tools/available builds the response, so a per-user disabled tool is removed from all_tools entirely. The later override loop cannot mark that tool as enabled=false/status=disabled, which regresses the display/listing behavior and hides policy-disabled tools instead of showing their disabled state. Please keep the execution-path filtering, but use an unfiltered listing path here before applying the display status.
martinma51
force-pushed
the
feature/ability-auto-fill
branch
2 times, most recently
from
3b716d0 to
de4ee36
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an ability auto-fill feature for the model management dialog, which suggests and pre-selects model capabilities based on a curated backend catalog. The implementation spans a new YAML-based catalog, a backend suggestion service with a three-tier resolution strategy, and frontend integration with multi-language support. Feedback points out a potential race condition in the suggestion fetching logic that could result in stale UI updates and notes a minor typo in the catalog documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a curated catalog of LLM abilities to provide auto-fill suggestions and UI hints when adding or editing models. It includes a new backend service and API endpoint to lookup model capabilities based on provider and model name patterns, along with frontend integration in the model management dialog. Feedback focuses on reducing code duplication in the React component by extracting repeated state reset and change handler logic, and improving debuggability by logging errors in the API wrapper instead of silently swallowing them.
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
I found two frontend state issues in the ability auto-fill flow.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a curated ability catalog for LLM models, enabling automatic suggestion and auto-filling of capabilities like vision and tool calling during model setup. The changes encompass a YAML-based catalog, a backend lookup service with tiered resolution, and a new API endpoint. The frontend is updated to consume these suggestions, providing UI hints and auto-filling forms while respecting manual user overrides and managing asynchronous request ordering. Review feedback recommends enhancing type safety in the frontend by reusing shared interfaces and validating API responses, and suggests optimizing backend performance by pre-partitioning catalog rules into tiers during the loading process.
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
Found one remaining frontend state issue in the direct form provider flow.
| */ | ||
| const handleModelNameChange = (newModelName: string) => { | ||
| setFormData(prev => { | ||
| void applyAbilitySuggestion(prev.model_provider, newModelName, prev.category) |
There was a problem hiding this comment.
[P2] Recompute suggestions when the form provider changes
This helper keeps suggestions in sync when model_name changes, but the direct form's provider selector still only updates model_provider. If a user selects OpenAI/gpt-4o and gets auto-filled abilities, then switches the provider to another LLM provider while keeping the same model name, the old OpenAI hint and abilities remain in the form. Please add the same kind of provider-change path, either resetting the suggestion/model abilities or re-running applyAbilitySuggestion(newProvider, prev.model_name, prev.category) so the submitted abilities match the current provider/model pair.
|
resolve the conflict |
martinma51
force-pushed
the
feature/ability-auto-fill
branch
from
8a1516e to
413cfd4
Compare
|
Rebased onto current upstream/main (we were -65/+17 behind) and squashed all 17 review-iteration commits into a single coherent commit @qinxuye — re-checked your last open feedback (P2 "Recompute suggestions when the form provider changes"). The upstream provider-Select handler we now merge with already does Diff vs upstream/main: 9 files / +1352 / −9 (same content footprint as before the rebase, no functional drift). Local checks:
PR is now |
martinma51
force-pushed
the
feature/ability-auto-fill
branch
3 times, most recently
from
94efb75 to
51b31d5
Compare
Add a backend YAML catalog (``src/xagent/core/model/abilities_catalog.yaml``)
mapping (provider, model_name_pattern) to a curated set of abilities, plus
a backend lookup service and a ``GET /api/models/ability_suggestion``
endpoint. On the frontend, the model-management dialog now auto-fills
ability checkboxes from the catalog as the user picks/types a model, with
a hint chip showing the matched rule.
Backend
* ``model_catalog`` service with three-tier matching: exact provider+model,
wildcard-provider, then provider-default fallback
* YAML covers OpenAI GPT-4o/4.1/5/5.5, Claude 3.5/4/4.5/5, Gemini 2/3,
DeepSeek V3/R1/R2/V4, Qwen 2.5/3, plus cross-provider rules for
\"-vl\"/\"-4v\" coding-plan model names that imply vision support
* Lazy load with double-checked locking; structured fall-throughs so
unknown providers degrade gracefully to category defaults
Frontend
* ``model-management-dialog.tsx`` calls the suggestion endpoint as the
user picks (wizard) or types (form mode) a model name and as they
switch provider; the response auto-fills abilities only when the user
hasn't manually touched them since this wizard run
* Stale-response guard via monotonic request counter so out-of-order
network responses can't overwrite a newer one
* Manual ability edits are tracked via a ref (not closure value) so
in-flight responses can't clobber them
* Provider/category resets invalidate any in-flight lookup before
clearing state to prevent old-model auto-fill bleeding into the
reset form
Review feedback addressed (rogercloud APPROVED 5/9, qinxuye 5/13):
* P2 \"recompute suggestions when form provider changes\" — the form
provider Select now calls ``resetAbilitySuggestionState()`` and the
upstream-merged ``model_name=\"\"`` reset together; subsequent
model_name input retriggers a fresh catalog lookup
* Frontend state race conditions (stale closure of ``userTouchedAbilities``,
in-flight requests surviving state resets) fixed via refs + counter
martinma51
force-pushed
the
feature/ability-auto-fill
branch
from
51b31d5 to
de8e6e4
Compare
4 participants
No description provided.