feat(core): add Fireworks provider with prefix-based validation#855
feat(core): add Fireworks provider with prefix-based validation#855samin1554 wants to merge 2 commits into
Conversation
|
PR author is not in the allowed authors list. |
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@packages/notte-core/src/notte_core/common/config.py`:
- Around line 261-271: The typed SDK path still rejects prefix-based reasoning
models before Config.is_valid() can apply, so arbitrary Fireworks provider paths
are not accepted through AgentCreateRequest.reasoning_model. Update the SDK-side
validation in AgentCreateRequest and the related LlmModel coercion in types.py
so reasoning_model can accept either a known enum value or a valid prefix
provider path, matching the logic in Config.is_valid() instead of enforcing
enum-only parsing.
- Around line 262-268: The is_valid(model) check in config.py is too permissive
for prefix providers: it currently accepts a provider key alone or a trailing
slash because it only validates LlmModel.get_provider and has_apikey_in_env.
Update the logic so prefix-provider models are only valid when there is a
non-empty suffix after the provider segment, while still allowing exact enum
matches via LlmModel.valid(); use is_valid and LlmModel.get_provider as the main
points to adjust.
- Around line 130-131: The `apikey_name` logic in `notte_core.common.config` is
returning `OPENROUTER_API_KEY` before the `LlmProvider.fireworks_ai` branch can
ever run, so Fireworks models never require their own credential. Update
`apikey_name` (and any related `has_apikey_in_env` validation path) so
`fireworks_ai` resolves to `FIREWORKS_API_KEY` even when `enable_openrouter()`
is enabled, while keeping other providers on the OpenRouter key as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52d0d021-7322-44fd-b23b-a459ab9319d8
📒 Files selected for processing (2)
packages/notte-core/src/notte_core/common/config.pytests/config/test_fireworks_provider.py
- Exempt fireworks_ai from OpenRouter API key override - Reject bare provider prefix in is_valid() (require model suffix) - Update SDK AgentCreateRequest validator to accept prefix provider paths
|
2027 auto-runs evals against preview deployments of your docs. To enable this, install one of:
Once a preview is deployed, open a new PR and we'll run the eval automatically. Evaluating agent experience using 2027.dev · View dashboard |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/notte-sdk/src/notte_sdk/types.py`:
- Around line 1892-1895: The validation in the model-checking path currently
conflates malformed model identifiers with missing API key configuration. Update
the logic around LlmModel.is_valid() in the model validation flow to first
detect invalid bare-prefix model paths (for example, values like fireworks_ai
with no suffix) and raise a separate “invalid model path” error, then keep the
provider.apikey_name environment-variable message only for otherwise valid
models that still require credentials. Use the LlmModel.is_valid and
provider.apikey_name checks to split these cases cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65300ddf-bcc3-4ab9-957f-ea9da6dcad9f
📒 Files selected for processing (3)
packages/notte-core/src/notte_core/common/config.pypackages/notte-sdk/src/notte_sdk/types.pytests/config/test_fireworks_provider.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/config/test_fireworks_provider.py
- packages/notte-core/src/notte_core/common/config.py
| if not LlmModel.is_valid(value): | ||
| raise ValueError( | ||
| f"Model '{value}' requires the {provider.apikey_name} variable to be configured in the environment" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Split malformed-model errors from missing-key errors.
LlmModel.is_valid() also rejects bare prefix values with no suffix, so this message is wrong for inputs like fireworks_ai even when FIREWORKS_API_KEY is already present. Please surface an “invalid model path” error separately from the env-var check.
Suggested fix
if provider.is_prefix_provider:
- if not LlmModel.is_valid(value):
+ _, _, suffix = str(value).partition("/")
+ if not suffix:
+ raise ValueError(f"Model '{value}' must include a provider-prefixed model path")
+ if not provider.has_apikey_in_env():
raise ValueError(
f"Model '{value}' requires the {provider.apikey_name} variable to be configured in the environment"
)
return value🤖 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 `@packages/notte-sdk/src/notte_sdk/types.py` around lines 1892 - 1895, The
validation in the model-checking path currently conflates malformed model
identifiers with missing API key configuration. Update the logic around
LlmModel.is_valid() in the model validation flow to first detect invalid
bare-prefix model paths (for example, values like fireworks_ai with no suffix)
and raise a separate “invalid model path” error, then keep the
provider.apikey_name environment-variable message only for otherwise valid
models that still require credentials. Use the LlmModel.is_valid and
provider.apikey_name checks to split these cases cleanly.
1 participant
Summary
Fireworks models rotate pretty frequently and the current approach of hardcoding each one as a LlmModel enum entry means every rotation needs an SDK change and a release. This PR fixes that by validating at the provider level instead.
What changed
15 tests covering provider registration, arbitrary path validation, API key gating, response format, and making sure non-prefix providers still reject unknown models
How it addresses #854
Any fireworks_ai/* path is accepted when FIREWORKS_API_KEY is set, no SDK release needed for model rotations
No hardcoded slugs in the enum so docs and SDK can't drift independently
is_valid() confirms the string is structurally valid and the key exists, whether the model is actually deployed is on the provider at runtime
Migration note
valid() is unchanged for backwards compat. The cloud endpoint just needs to swap model in LlmModel.valid() for LlmModel.is_valid(model).
Summary by CodeRabbit