fix(intelligence): resolve stale AI model ids instead of fatalling on /ai/generate - #3328
Open
MdAsifHossainNadim wants to merge 3 commits into
Open
fix(intelligence): resolve stale AI model ids instead of fatalling on /ai/generate#3328MdAsifHossainNadim wants to merge 3 commits into
MdAsifHossainNadim wants to merge 3 commits into
Conversation
… /ai/generate Gemini's default model id was gemini-2.0-flash, which is no longer among the registered models, so Provider::get_model() returned null and handle_request() passed that null to method_exists() -- a TypeError, and a 500 instead of an error response. The same unresolvable id was fed to the admin Model select as its `default`, which is why the dropdown rendered blank and why saving the AI Assist tab persisted the broken id. - Gemini now defaults to a model that is actually registered. - Provider::resolve_model() degrades a retired saved id to a working model of the right type rather than returning null. - The controller returns WP_Error for every misconfiguration instead of dereferencing a null provider or model, gates on is_configured() (active_engine() falls back to openai, so emptiness proved nothing), enforces the image-generation toggle at the API, and coerces the unconstrained payload members. - Saved model ids that a provider has retired are remapped at read time, so the settings select shows a real value instead of blank. - The settings Vue app copied a whole saved section inside the per-field loop, discarding defaults already applied to earlier fields. Closes #4910 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAI provider model resolution now handles saved, default, and fallback models by type. AI settings remap retired model IDs, REST generation validates configuration and capabilities, and the settings page applies per-field defaults more consistently. ChangesAI model flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AIRequestController
participant Provider
participant AIModelInterface
Client->>AIRequestController: Submit generation payload
AIRequestController->>Provider: Resolve provider and model by type
Provider->>AIModelInterface: Verify process_type support
AIModelInterface-->>Provider: Return supported model
Provider-->>AIRequestController: Return generated result or WP_Error
AIRequestController-->>Client: Return response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
includes/Intelligence/REST/AIRequestController.php (2)
90-98: 🧹 Nitpick | 🔵 TrivialOperational note: no visible throttling on paid AI generation calls.
The image-generation toggle gates a marketplace-level feature flag, but there's no visible per-vendor rate limiting/quota on this endpoint despite the inline comment noting costs are billed to the marketplace owner. Consider whether abuse protection (rate limiting, per-vendor caps) exists upstream or should be added, since REST endpoints triggering paid third-party API calls are a common cost/DoS vector.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/Intelligence/REST/AIRequestController.php` around lines 90 - 98, Add or reuse abuse protection in the REST handler around the paid image-generation path identified by Model::SUPPORTS_IMAGE and the AI request controller, enforcing a per-vendor rate limit or quota before invoking the third-party API. Reject over-limit requests with the controller’s existing configuration/error response mechanism, while preserving the marketplace toggle and configuration checks.
71-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding regression tests for the resolved bug's guard paths.
This diff fixes a production fatal error (
#4910) via several new guard clauses (instanceof AIProviderInterface,instanceof AIModelInterface, retired-model fallback,Throwablecatch). Given the criticality, unit/integration tests covering: null/invalid provider, retired saved model id, unsupportedtypepayload, and a thrownError/Exceptionduring generation would help lock in this fix.
Want me to draft PHPUnit tests for these paths?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/Intelligence/REST/AIRequestController.php` around lines 71 - 151, Add regression tests for handle_request covering invalid or null providers, retired saved model IDs falling back to a valid model, unsupported or malformed type payloads, and both Error and Exception thrown during generation. Assert each guard returns the expected configuration or service WP_Error response and that valid fallback generation remains successful.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@includes/Intelligence/REST/AIRequestController.php`:
- Around line 90-98: Add or reuse abuse protection in the REST handler around
the paid image-generation path identified by Model::SUPPORTS_IMAGE and the AI
request controller, enforcing a per-vendor rate limit or quota before invoking
the third-party API. Reject over-limit requests with the controller’s existing
configuration/error response mechanism, while preserving the marketplace toggle
and configuration checks.
- Around line 71-151: Add regression tests for handle_request covering invalid
or null providers, retired saved model IDs falling back to a valid model,
unsupported or malformed type payloads, and both Error and Exception thrown
during generation. Assert each guard returns the expected configuration or
service WP_Error response and that valid fallback generation remains successful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a76b8e7-3afb-4830-8df8-0d7f8732c03a
📒 Files selected for processing (5)
includes/Intelligence/Admin/Settings.phpincludes/Intelligence/REST/AIRequestController.phpincludes/Intelligence/Services/Provider.phpincludes/Intelligence/Services/Providers/Gemini.phpsrc/admin/pages/Settings.vue
Contributor
Author
CI note — the 2 PHPUnit failures are pre-existing on
|
…ctions - resolve_model() called get_model() twice and get_models_by_type() once, each of which re-runs the provider's models filter. Resolve the type-filtered list once and index into it; three dispatches become one and the method gets shorter. - remap_retired_model_values() declared ': array' and cast on the decline path, so for every section that is not dokan_ai it rewrote whatever an earlier callback on the same filter had returned. It now returns the value untouched, matching its three siblings on dokan_get_settings_values. - Collapse the $args['type'] write/read/write dance in handle_request(). No behaviour change: verified the resolve matrix and the full endpoint failure matrix are identical before and after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d; tidy the settings loader - move the scalar cast next to the option read it normalizes - drop a dead callback param and collapse the default-pick branch in fetchSettingValues Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All Submissions:
Changes proposed in this Pull Request:
Gemini::get_default_model_id()returnedgemini-2.0-flash, an id that is no longer among the registered Gemini models (gemini-2.5-flash,gemini-2.5-pro,gemini-2.5-flash-lite-preview-06-17).Provider::get_model()returnsnullfor an unknown id, andhandle_request()passed that null straight intomethod_exists()— the exactTypeErrorin the report.The reporter's second observation — "some dropdowns, which were previously selected, appear empty after a few days" — is the same root cause, not a separate symptom. That same unresolvable id is the
defaultof the admin Model<select>, so it is not among the option keys and the select renders blank; saving the tab then persistsgemini-2.0-flashinto thedokan_aioption, which is what later reaches/ai/generate.What changed:
Providers/Gemini.php— default now points at a registered model.Services/Provider.php— newresolve_model( $type, $model_id )degrades a retired saved id to a working model of the right type instead of returning null, plusget_default_model_id_by_type(). Both are additive; theget_default_model_id(): stringabstract signature is untouched so Pro'sBriaAikeeps compiling.REST/AIRequestController.php— returnsWP_Errorfor every misconfiguration instead of dereferencing a null provider/model. Also fixes a second, independent fatal one line earlier:$manager->get_provider()returns null for an unregistered id, so a site still holding the 4.1 defaultdokan_ai_engine = 'chatgpt'hitCall to a member function get_model() on null.Intelligence/Admin/Settings.php— type-aware default, plus a read-time remap of retired ids so the select self-heals.src/admin/pages/Settings.vue— the hydration loop assigned the whole saved section inside the per-field loop, discarding defaults already applied to earlier fields. Hoisted the copy out of the loop.Two deliberate calls worth reviewer attention:
$manager->is_configured( $type )rather thanempty( $provider_id ).active_engine()falls back toopenai, so an emptiness check would have been dead code and an unconfigured site would have attempted a keyless upstream call.AIProviderInterfacewas NOT widened.resolve_model()/get_default_model_id_by_type()are not on the public interface, so every call site guards withinstanceof Provider(ormethod_existsin Pro). Adding them to the interface would be a BC break for third-party providers.Also enforces
dokan_ai_image_gen_availabilityat the API — image generation is billed to the marketplace owner and the admin toggle previously held only in the UI.Related Pull Request(s)
method_exists-guarded so merge order does not matter.Closes
How to test the changes in this Pull Request:
Put the site into the reported configuration and call the endpoint:
Before:
PHP Fatal error: Uncaught TypeError: method_exists(): Argument #1 ... null given ... AIRequestController.php:97withmethod_exists(NULL, 'process_text')— byte-identical to the trace in the issue.After: no fatal; a clean JSON error response.
Verified matrix (all previously fatal or misleading, now clean — Lite 5.0.9 / Pro 5.0.7 / WC 10.9.4 / PHP 8.4):
gemini-2.0-flashchatgptdokan_ai_not_configured(was a fatal on line 93)dokan_ai_not_configureddokan_ai_not_configuredtype=image, toggle offpayload.field/typeas arraysTypeErrorBlank-dropdown half:
Browser check still needed from a reviewer (my local session could not reach
/wp-admin): Dokan → Settings → AI Assist, set Engine = Gemini, confirm the Model select pre-selects "Gemini 2.5 Flash" instead of rendering blank.src/changed, so runnpm run buildfirst —assets/js/is gitignored and no build artefacts are committed.PHPCS clean on all four PHP files.
src/admin/pages/Settings.vuehas a pre-existing eslint parse error at 4:52, identical ondevelop— unrelated to this change; the script block passesnode --check.Changelog entry
Fix — AI Assist: a retired model id no longer crashes generation or blanks the settings dropdown
Previously, when a provider retired a model that was already saved in settings (Gemini's default
gemini-2.0-flash), the Model dropdown rendered blank and any vendor AI generation request crashed with a PHP fatal error. Dokan now falls back to a working model of the correct type, remaps retired ids when settings load, and returns a clear error message for every misconfiguration instead of failing fatally.🤖 Generated with Claude Code
Summary by CodeRabbit