Skip to content

Throw when no requested model preference is available#245

Open
alewolf wants to merge 3 commits into
WordPress:trunkfrom
alewolf:fix/241-model-preference-no-silent-fallback
Open

Throw when no requested model preference is available#245
alewolf wants to merge 3 commits into
WordPress:trunkfrom
alewolf:fix/241-model-preference-no-silent-fallback

Conversation

@alewolf

@alewolf alewolf commented Jun 4, 2026

Copy link
Copy Markdown

Summary

Fixes #241.

Requesting a specific model id via usingModelPreference() that the resolved provider doesn't expose previously did not error — model resolution silently fell through to the first discovered model. A typo, a stale id, or a non-dated alias would silently route traffic to a potentially far more expensive model (e.g. Haiku → Opus) with no signal to the caller.

Root cause

In PromptBuilder::getConfiguredModel(), preferences are matched against the candidate map by exact string. When no preference matched, the code fell through to reset($candidateMap) — the first model discovered for the prompt's capability — with no error or warning.

Fix

When explicit model preferences were provided but none are available from the resolved provider(s), generation now throws an InvalidArgumentException instead of silently substituting a model. The message names the requested preference(s) (and the provider, if one was locked in via usingProvider()) so a typo or stale id is obvious.

Unchanged behavior:

  • No preference set → automatic discovery still picks a suitable model.
  • Partial match → when at least one requested preference is available, it is used in priority order (preference fallback among the stated preferences is still honored).
  • usingModel() → the explicit path already validates the id and is unaffected.

Behavior change note

This changes resolution from silent-fallback to throw for the unmatched-preference case. The unit test that codified the old silent fallback (testUsingModelPreferenceFallsBackToDiscovery) has been updated accordingly. Maintainers may want to reflect this in the version bump (currently 1.3.1).

Out of scope (per the issue)

  • Alias resolution (claude-haiku-4-5 → latest dated model) — this mirrors provider-specific behavior and belongs in the individual provider plugins, not the core client.
  • "Cheapest available" fallbackModelMetadata carries no pricing/cost data, so models can't be ranked by cost in core.

Testing

  • composer test:unit — 1090 tests pass (added testUsingModelPreferenceThrowsWhenNoPreferenceAvailable, testUsingModelPreferenceThrowsWhenNoPreferenceAvailableForProvider, and testFallsBackToDiscoveryWhenNoPreferenceSet).
  • composer phpcs — clean.
  • composer phpstan — no errors.

🤖 Generated with Claude Code

Requesting a specific model id via usingModelPreference() that no resolved
provider exposes previously fell through to the first discovered model with no
signal, silently routing the request to an arbitrary (potentially far more
expensive) model. A typo, stale id, or non-dated alias could route Haiku-priced
traffic to Opus with no error.

Generation now throws an InvalidArgumentException when explicit preferences were
provided but none are available, naming the requested preferences (and provider,
if locked) so the cause is obvious. Automatic discovery when no preference is set
is unchanged, as is preference fallback among available preferences.

Fixes WordPress#241.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 4, 2026 08:46
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: alewolf <alekv@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: lezama <migueluy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.17%. Comparing base (99a65e8) to head (24e2707).

Files with missing lines Patch % Lines
src/Builders/PromptBuilder.php 96.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk     #245      +/-   ##
============================================
+ Coverage     88.12%   88.17%   +0.04%     
- Complexity     1213     1220       +7     
============================================
  Files            60       60              
  Lines          3934     3958      +24     
============================================
+ Hits           3467     3490      +23     
- Misses          467      468       +1     
Flag Coverage Δ
unit 88.17% <96.66%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR changes PromptBuilder::usingModelPreference() behavior to fail loudly when explicit model preferences don’t match any available models, preventing silent fallback to discovery and potentially more expensive models (per issue #241).

Changes:

  • Add unit tests asserting usingModelPreference() throws when no requested preference is available (with and without a locked provider).
  • Update PromptBuilder selection logic to throw InvalidArgumentException when explicit preferences are provided but none match.
  • Add helper to build a clearer exception message listing requested preferences.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
tests/unit/Builders/PromptBuilderTest.php Adds/updates tests to verify throwing behavior and keeps discovery behavior when no preference is set.
src/Builders/PromptBuilder.php Implements “fail loudly” behavior for unmatched preferences and adds an exception-message builder.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* Reconstructs a human-readable list of the requested preferences from their internal
* keys so a typo or stale model identifier is obvious to the caller.
*
* @since n.e.x.t

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks, but @since n.e.x.t is the project's documented convention for unreleased code — see CONTRIBUTING.md: "Use @since n.e.x.t for new code (will be replaced with actual version on release)." It's used consistently throughout the codebase and gets swapped for the real version at release time, so I've kept it as-is here for consistency.

Comment on lines +1548 to +1555
foreach ($this->modelPreferenceKeys as $preferenceKey) {
if (strpos($preferenceKey, 'providerModel::') === 0) {
[$providerId, $modelId] = explode('::', substr($preferenceKey, strlen('providerModel::')), 2);
$requested[] = sprintf('"%s" (provider "%s")', $modelId, $providerId);
} else {
$requested[] = sprintf('"%s"', substr($preferenceKey, strlen('model::')));
}
}
Comment thread src/Builders/PromptBuilder.php Outdated
Comment on lines +1559 to +1561
'None of the requested model preferences (%s) are available from provider "%s" for %s. '
. 'Use a model identifier that the provider exposes, or call usingModelPreference() with an '
. 'available model.',
alewolf and others added 2 commits June 4, 2026 10:57
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…le branch.

- Make buildUnmatchedPreferenceMessage() defensive against unexpected key
  shapes, falling back to the raw key instead of a misleading substring.
- Reword the exception to point callers at removing usingModelPreference()
  for automatic discovery, instead of the circular "call it with an
  available model".
- Add a unit test for an unavailable provider/model tuple preference,
  covering the provider-attributed message branch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@felixarntz felixarntz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alewolf Thank you for the PR.

While I understand your concern here, the current behavior is by design. Since e.g. WordPress plugins aren't in control which providers and models a site has configured, the PHP AI Client is intended to work at the largest vector of sites - hence this API is about "model preferences", not "model requirements". So this is not a bug, it's intended behavior.

That being said, your concern is certainly valid. However, rather than addressing it as a bug, it needs to be an enhancement. The current change here would be breaking, so it can't be merged either way. What we could do instead is:

  • either allow passing a $strict flag to usingModelPreference()
  • or add a separate method usingModelRequirement() (or maybe there's a better name to clarify the strict nature of this method) which is the same as usingModelPreference() but errors if no match is found

@alewolf

alewolf commented Jun 9, 2026

Copy link
Copy Markdown
Author

Thanks @felixarntz

That makes sense. I agree that for a provider-agnostic SDK the preference API should stay best-effort, and that this is a breaking change as written.

usingModelPreference() is variadic, and threading a $strict flag through it gets awkward (a trailing boolean would be ambiguous against the preference list).

I'd lean toward the second approach you mentioned, a separate method that mirrors usingModelPreference() but throws when nothing matches. I'd avoid usingModelRequirement() because we already have a ModelRequirements class in the codebase that represents a model's required capabilities/options (built from the prompt to filter the catalog). Maybe requireModelPreference() or usingModelPreferenceOrFail()? Do you have a preference?

I'll revert usingModelPreference() to its original best-effort behavior (no signature change) and add the strict behavior as a separate new method, once you approve this way forward, and let me know what preference you have for the new function name.

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.

usingModelPreference() with an unrecognized model id silently falls back to the provider default (cost risk)

3 participants