SDK: more embedded mode configuration #3970
Conversation
WalkthroughThe PR adds structured LLM provider configs, extends embedded-mode APIs to accept multiple provider keys/configs and settings overrides, applies validated settings before importing the ASGI app, and wires these configurations into the embedded ASGI transport and returned AsyncClient. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Skyvern as Skyvern Client
participant Factory as Embedded Server Factory
participant Settings as Settings Module
participant ASGI as ASGITransport/App
User->>Skyvern: instantiate client with provider configs/keys + overrides
Skyvern->>Factory: create_embedded_server(..., settings_overrides)
Factory->>Settings: set ENABLE_* flags and map config fields to settings
alt settings_overrides provided
Factory->>Settings: validate override keys
note right of Settings `#f6f6f6`: invalid key -> ValueError
Factory->>Settings: apply overrides
end
Factory->>ASGI: import app (after settings applied)
Factory->>ASGI: construct ASGITransport and httpx.AsyncClient
Factory-->>Skyvern: return AsyncClient
Skyvern-->>User: client ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-14T04:27:55.404ZApplied to files:
🧬 Code graph analysis (1)skyvern/library/embedded_server_factory.py (1)
🪛 Ruff (0.14.5)skyvern/library/embedded_server_factory.py86-86: Avoid specifying long messages outside the exception class (TRY003) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 963072a in 1 minute and 51 seconds. Click for details.
- Reviewed
173lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/library/embedded_server_factory.py:55
- Draft comment:
Consider validating Azure parameters when azure_api_key is provided. If using Azure, required fields like azure_deployment, azure_api_base, and azure_api_version should be explicitly checked to avoid misconfiguration. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is suggesting that Azure configuration might be incomplete if only the API key is provided without the other parameters. However, this is speculative - it assumes that these parameters are required for Azure to work properly. The code as written allows users to provide just the API key, and the other parameters are optional. Without seeing how these settings are used elsewhere in the codebase, I cannot determine if this is actually a problem. The comment uses "should be explicitly checked" which is a suggestion, not pointing to a definite bug. This falls under "speculative comments" - it's saying "If X (using Azure), then Y (missing params) is an issue", but we don't know if it's definitely an issue. The settings object might have defaults for these values, or they might be truly optional depending on the Azure configuration. Maybe Azure really does require these parameters and the comment is catching a real bug. Perhaps the settings object doesn't have sensible defaults and this would cause runtime errors. Without seeing the rest of the codebase, I might be wrong to dismiss this. Even if Azure does require these parameters, the comment is still speculative because it says "should be explicitly checked" rather than pointing to a definite issue. The rule states "Do NOT make speculative comments, such as 'If X, then Y is an issue', Only comment if it is definitely an issue." This comment is suggesting a potential improvement but not identifying a clear bug in the changed code. Additionally, the parameters are exposed in the function signature as optional, suggesting the author intentionally made them optional. This comment should be deleted. It's a speculative suggestion about potential misconfiguration rather than identifying a definite issue with the code changes. The author explicitly made these parameters optional in the function signature, and without strong evidence that this is wrong, we should assume the design is intentional.
2. skyvern/library/embedded_server_factory.py:77
- Draft comment:
Using setattr with hasattr for settings_overrides may allow unintended modifications. Consider using a whitelist of allowed settings keys. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is suggesting a code quality improvement - using a whitelist instead of hasattr/setattr. However, this is somewhat speculative. The current implementation does have guards (hasattr check and raises ValueError for invalid keys). The comment says "may allow unintended modifications" which is speculative language ("may"). The code already validates that the setting exists before setting it. Without seeing the settings object definition or knowing what settings should/shouldn't be modifiable, it's hard to say this is definitively a problem. The comment is more of a "consider this alternative approach" suggestion rather than pointing out a clear bug. This falls into the category of speculative comments. The comment could be valid if there are certain settings that exist but shouldn't be modifiable through this API. The hasattr check only validates existence, not whether modification is appropriate. There could be internal/private settings that shouldn't be exposed through settings_overrides. Without more context about the settings object, I can't be certain this is wrong. While there could theoretically be settings that shouldn't be modified, the comment uses speculative language ("may allow") and doesn't provide concrete evidence of a problem. The code does validate that settings exist before modification. Without strong evidence that specific settings are being inappropriately exposed, this is more of a subjective code quality suggestion than a clear issue. This comment is speculative and uses "may allow" language without demonstrating a concrete problem. The code has validation (hasattr + ValueError). Without evidence of specific settings that shouldn't be modifiable, this is subjective advice rather than a clear issue. Should be deleted per the rules against speculative comments.
3. skyvern/library/skyvern_sdk.py:168
- Draft comment:
Ensure consistent parameter naming. The old 'open_api_key' has been renamed to 'openai_api_key'; verify that this change is propagated in all usages to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. skyvern/library/skyvern_sdk.py:169
- Draft comment:
The parameter 'settings' passed to create_embedded_server is mapped to 'settings_overrides'. Consider renaming it in the SDK for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This comment is suggesting a code quality improvement - renaming the parameter for consistency. However, I need to consider: 1) Is this actionable? Yes, it's suggesting to renamesettingstosettings_overrides. 2) Is it about a change? Yes, thesettingsparameter was just added in this diff. 3) Is it clear and important? The naming inconsistency could cause confusion for developers reading the code. However, the comment says "Consider renaming" which is somewhat soft language. The parameter namesettingsis actually more user-friendly thansettings_overridesfor the public API, so having different names at different layers might be intentional. The internal implementation detail (that it's "overrides") doesn't need to leak into the public API. The comment might be missing context about API design principles. It's common and often desirable to have simpler, more user-friendly parameter names in public APIs while using more specific names internally. The namesettingsis clearer for SDK users thansettings_overrides. This could be an intentional design choice rather than an oversight. While API design principles might justify different naming at different layers, the comment is still speculative and uses soft language ("Consider"). It's not pointing out a clear bug or issue, just suggesting a potential improvement that may or may not be appropriate. Without seeing thecreate_embedded_serverfunction signature, we can't determine if this is truly an issue. This comment is making a subjective suggestion about naming consistency rather than identifying a clear issue. The different names at different API layers could be intentional design. The comment uses soft language ("Consider") and doesn't demonstrate strong evidence of a problem. It should be deleted.
Workflow ID: wflow_RgH9Sk8cdQU1IQpg
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
skyvern/library/embedded_server_factory.py(2 hunks)skyvern/library/skyvern_sdk.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{skyvern,integrations,alembic,scripts}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{skyvern,integrations,alembic,scripts}/**/*.py: Use Python 3.11+ features and add type hints throughout the codebase
Follow PEP 8 with a maximum line length of 100 characters
Use absolute imports for all Python modules
Document all public functions and classes with Google-style docstrings
Use snake_case for variables and functions, and PascalCase for classes
Prefer async/await over callbacks in asynchronous code
Use asyncio for concurrency
Always handle exceptions in async code
Use context managers for resource cleanup
Use specific exception classes
Include meaningful error messages when raising or logging exceptions
Log errors with appropriate severity levels
Never expose sensitive information in error messages
Files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern_sdk.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Python code must be linted and formatted with Ruff
Use type hints throughout Python code
Prefer async/await for asynchronous Python code
Enforce a maximum line length of 120 characters in Python files
Files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern_sdk.py
skyvern/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type-check Python code in the skyvern/ package with mypy
Files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern_sdk.py
🧠 Learnings (2)
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Applies to {skyvern,integrations,alembic,scripts}/**/*.py : Use Python 3.11+ features and add type hints throughout the codebase
Applied to files:
skyvern/library/skyvern_sdk.py
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Applies to {skyvern,integrations,alembic,scripts}/**/*.py : Document all public functions and classes with Google-style docstrings
Applied to files:
skyvern/library/skyvern_sdk.py
🧬 Code graph analysis (2)
skyvern/library/embedded_server_factory.py (1)
skyvern/client/client.py (1)
AsyncSkyvern(1613-3400)
skyvern/library/skyvern_sdk.py (2)
skyvern-ts/client/src/environments.ts (2)
SkyvernEnvironment(3-7)SkyvernEnvironment(9-12)skyvern/client/environment.py (1)
SkyvernEnvironment(6-9)
🪛 Ruff (0.14.4)
skyvern/library/embedded_server_factory.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests and pre-commit hooks
- GitHub Check: Optimize new Python code in this PR
🔇 Additional comments (1)
skyvern/library/skyvern_sdk.py (1)
165-180: Embedded wiring looks good.The new provider keys and
settingsoverrides flow cleanly intocreate_embedded_server, and remote mode remains untouched. Nice job keeping the overloads and docstrings in sync.
stanislaw89
force-pushed
the
stas/sdk-embedded-settings
branch
from
963072a to
7f30515
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
skyvern/library/embedded_server_factory.py (2)
40-49: Require complete Azure configuration before enabling it.The code currently enables Azure (
ENABLE_AZURE = True) when onlyazure_api_keyis present, even ifazure_deployment,azure_api_base, orazure_api_versionare missing. Azure OpenAI requires all four parameters to function correctly. Enabling Azure with incomplete configuration will cause runtime failures when the Azure client attempts to initialize.Apply this diff to require all Azure parameters:
# Azure if azure_api_key: + if not all([azure_deployment, azure_api_base, azure_api_version]): + raise ValueError( + "Azure OpenAI requires all of: azure_api_key, azure_deployment, " + "azure_api_base, and azure_api_version" + ) settings.AZURE_API_KEY = azure_api_key - if azure_deployment: - settings.AZURE_DEPLOYMENT = azure_deployment - if azure_api_base: - settings.AZURE_API_BASE = azure_api_base - if azure_api_version: - settings.AZURE_API_VERSION = azure_api_version + settings.AZURE_DEPLOYMENT = azure_deployment + settings.AZURE_API_BASE = azure_api_base + settings.AZURE_API_VERSION = azure_api_version settings.ENABLE_AZURE = True
62-68: Address the TRY003 lint violation.Raising
ValueError(f"Invalid setting: {key}")violates Ruff's TRY003 rule, which requires exception messages to be defined inside the exception class rather than at the raise site. This will fail CI.Apply this diff to fix the violation:
+class InvalidSettingError(ValueError): + """Raised when an invalid setting key is provided.""" + + def __init__(self, key: str) -> None: + super().__init__(f"Invalid setting: {key}") + + def create_embedded_server( openai_api_key: str | None = None, # ... rest of parametersThen update the raise statement:
if hasattr(settings, key): setattr(settings, key, value) else: - raise ValueError(f"Invalid setting: {key}") + raise InvalidSettingError(key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
skyvern/library/embedded_server_factory.py(2 hunks)skyvern/library/skyvern.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{skyvern,integrations,alembic,scripts}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{skyvern,integrations,alembic,scripts}/**/*.py: Use Python 3.11+ features and add type hints throughout the codebase
Follow PEP 8 with a maximum line length of 100 characters
Use absolute imports for all Python modules
Document all public functions and classes with Google-style docstrings
Use snake_case for variables and functions, and PascalCase for classes
Prefer async/await over callbacks in asynchronous code
Use asyncio for concurrency
Always handle exceptions in async code
Use context managers for resource cleanup
Use specific exception classes
Include meaningful error messages when raising or logging exceptions
Log errors with appropriate severity levels
Never expose sensitive information in error messages
Files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Python code must be linted and formatted with Ruff
Use type hints throughout Python code
Prefer async/await for asynchronous Python code
Enforce a maximum line length of 120 characters in Python files
Files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern.py
skyvern/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type-check Python code in the skyvern/ package with mypy
Files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern.py
🧠 Learnings (1)
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Applies to {skyvern,integrations,alembic,scripts}/**/*.py : Include meaningful error messages when raising or logging exceptions
Applied to files:
skyvern/library/embedded_server_factory.py
🧬 Code graph analysis (1)
skyvern/library/skyvern.py (3)
skyvern/forge/sdk/schemas/tasks.py (1)
llm_key(293-305)skyvern/forge/sdk/schemas/task_v2.py (1)
llm_key(61-76)skyvern/client/environment.py (1)
SkyvernEnvironment(6-9)
🪛 Ruff (0.14.4)
skyvern/library/embedded_server_factory.py
68-68: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Run tests and pre-commit hooks
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
skyvern/library/embedded_server_factory.py (4)
30-33: LGTM!The OpenAI configuration is correctly implemented. When the API key is provided, it sets both the key and enables the provider.
35-38: LGTM!The Anthropic configuration follows the same correct pattern as OpenAI.
51-54: LGTM!The Gemini configuration follows the same correct pattern as OpenAI and Anthropic.
56-60: LGTM!The LLM key configuration correctly sets the primary and secondary LLM keys when provided.
skyvern/library/skyvern.py (4)
109-118: LGTM!The embedded mode overload signature correctly declares all the new provider-specific parameters with appropriate type hints.
120-135: LGTM!The embedded mode docstring is comprehensive and well-written, providing clear explanations for each parameter with helpful examples.
147-156: LGTM!The main
__init__signature correctly includes all new parameters with type hints matching the overload declaration.
175-186: LGTM!All parameters are correctly forwarded to
create_embedded_server, including the appropriate renaming ofsettingstosettings_overrides.
| def create_embedded_server( | ||
| openai_api_key: str | None, | ||
| openai_api_key: str | None = None, | ||
| anthropic_api_key: str | None = None, | ||
| azure_api_key: str | None = None, | ||
| azure_deployment: str | None = None, | ||
| azure_api_base: str | None = None, | ||
| azure_api_version: str | None = None, | ||
| gemini_api_key: str | None = None, | ||
| llm_key: str | None = None, | ||
| secondary_llm_key: str | None = None, | ||
| settings_overrides: dict[str, Any] | None = None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a comprehensive docstring for this public function.
This public function lacks documentation. According to the coding guidelines, all public functions should have Google-style docstrings explaining the purpose, parameters, return value, and ideally include usage examples.
Apply this diff to add a docstring:
def create_embedded_server(
openai_api_key: str | None = None,
anthropic_api_key: str | None = None,
azure_api_key: str | None = None,
azure_deployment: str | None = None,
azure_api_base: str | None = None,
azure_api_version: str | None = None,
gemini_api_key: str | None = None,
llm_key: str | None = None,
secondary_llm_key: str | None = None,
settings_overrides: dict[str, Any] | None = None,
) -> httpx.AsyncClient:
+ """Create an embedded Skyvern server with custom LLM provider configuration.
+
+ This factory function creates an httpx AsyncClient that runs Skyvern in embedded mode,
+ allowing you to override LLM provider settings without modifying environment variables.
+
+ Args:
+ openai_api_key: OpenAI API key to enable OpenAI provider.
+ anthropic_api_key: Anthropic API key to enable Anthropic provider.
+ azure_api_key: Azure OpenAI API key. Requires azure_deployment, azure_api_base,
+ and azure_api_version to be provided as well.
+ azure_deployment: Azure OpenAI deployment name (required with azure_api_key).
+ azure_api_base: Azure OpenAI API base URL (required with azure_api_key).
+ azure_api_version: Azure OpenAI API version (required with azure_api_key).
+ gemini_api_key: Google Gemini API key to enable Gemini provider.
+ llm_key: Primary LLM model selection (e.g., "OPENAI_GPT4_1").
+ secondary_llm_key: Secondary LLM for lightweight operations.
+ settings_overrides: Dictionary of Skyvern settings to override. Keys must match
+ existing settings attributes.
+
+ Returns:
+ An httpx AsyncClient configured for embedded mode.
+
+ Raises:
+ ValueError: If invalid settings are provided in settings_overrides.
+ """As per coding guidelines.
🤖 Prompt for AI Agents
In skyvern/library/embedded_server_factory.py around lines 7 to 17, the public
function create_embedded_server is missing a Google-style docstring; add a
comprehensive docstring immediately under the def line that states the function
purpose, documents each parameter (name, type, optional/default behavior),
describes the return value and its type, and includes a short usage example and
any important notes or side effects (e.g., environment variable precedence or
required keys). Ensure the docstring follows Google style conventions (Args:,
Returns:, Raises: if applicable, Example:) and keeps descriptions concise and
accurate to the function's behavior.
There was a problem hiding this comment.
This part is not not exposed to the customers so there is not need for docs
There was a problem hiding this comment.
@stanislaw89, understood! Thanks for clarifying that create_embedded_server is an internal factory method and not part of the customer-facing API surface. I'll note this for future reviews.
✏️ Learnings added
Learnt from: stanislaw89
Repo: Skyvern-AI/skyvern PR: 3970
File: skyvern/library/embedded_server_factory.py:7-17
Timestamp: 2025-11-14T04:27:55.396Z
Learning: The `create_embedded_server` function in `skyvern/library/embedded_server_factory.py` is an internal factory method not exposed to customers, so it does not require public documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7f30515 in 6 minutes and 25 seconds. Click for details.
- Reviewed
148lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/library/embedded_server_factory.py:63
- Draft comment:
Consider adding a docstring or inline comment for 'settings_overrides' to document the expected keys. Raising a ValueError on an invalid key is clear, but users may benefit from knowing which settings can be overridden. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. skyvern/library/skyvern.py:175
- Draft comment:
The embedded mode init now accepts a parameter named 'settings' that is forwarded as 'settings_overrides' to create_embedded_server. For clarity and consistency, consider renaming it to 'settings_overrides' in the init signature as well. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_1osIigpbe8cqifuE
You can customize
stanislaw89
force-pushed
the
stas/sdk-embedded-settings
branch
from
7f30515 to
9c254e8
Compare
| settings.ENABLE_AZURE = True | ||
|
|
||
| # Gemini | ||
| if gemini_api_key: |
There was a problem hiding this comment.
Gemini also has vertex and such. Is there a way to plumb this using LiteLLM instead of us adding all these conditions in here?
There was a problem hiding this comment.
ie create a version of LLM_KEY that just takes general LiteLLM configs and let the user specify those at init
stanislaw89
force-pushed
the
stas/sdk-embedded-settings
branch
from
9c254e8 to
5ae54a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
skyvern/library/embedded_server_factory.py (2)
46-54: Require complete Azure configuration before enabling.Azure OpenAI requires all four settings (api_key, deployment, api_base, api_version) to function correctly. Currently,
ENABLE_AZUREis set toTruewhen onlyazure_api_keyis provided, which will cause runtime failures when the Azure client is initialized with missing configuration.Apply this diff to validate all required Azure settings before enabling:
# Azure if azure_api_key: settings.AZURE_API_KEY = azure_api_key - if azure_deployment: - settings.AZURE_DEPLOYMENT = azure_deployment - if azure_api_base: - settings.AZURE_API_BASE = azure_api_base - if azure_api_version: - settings.AZURE_API_VERSION = azure_api_version - settings.ENABLE_AZURE = True + # Validate all required Azure settings are provided + if not all([azure_deployment, azure_api_base, azure_api_version]): + missing = [] + if not azure_deployment: + missing.append("azure_deployment") + if not azure_api_base: + missing.append("azure_api_base") + if not azure_api_version: + missing.append("azure_api_version") + raise ValueError(f"Azure OpenAI requires all configuration: missing {', '.join(missing)}") + + settings.AZURE_DEPLOYMENT = azure_deployment + settings.AZURE_API_BASE = azure_api_base + settings.AZURE_API_VERSION = azure_api_version + settings.ENABLE_AZURE = True
85-91: Fix TRY003 linting violation.The
ValueErrorwith an f-string message violates Ruff's TRY003 rule, which will fail CI. The validation logic itself is sound and correctly prevents invalid settings from being applied.Apply this diff to resolve the linting issue by defining the exception message within a custom exception class:
+class InvalidSettingError(ValueError): + """Raised when an invalid setting key is provided.""" + def __init__(self, key: str) -> None: + super().__init__(f"Invalid setting: {key}") + + def create_embedded_server( openai_api_key: str | None = None, ...Then update the raise statement:
if hasattr(settings, key): setattr(settings, key, value) else: - raise ValueError(f"Invalid setting: {key}") + raise InvalidSettingError(key)
🧹 Nitpick comments (1)
skyvern/library/embedded_server_factory.py (1)
35-77: Consider consolidating provider configuration with LiteLLM.The current approach of adding individual conditional blocks for each LLM provider (OpenAI, Anthropic, Azure, Gemini, Vertex AI, Groq) increases maintenance burden as more providers are added. As suchintan suggested, LiteLLM provides a unified interface that could handle all these providers through a single configuration mechanism, reducing code duplication and complexity.
This would involve:
- Accepting generic LiteLLM configuration parameters
- Letting LiteLLM handle provider-specific setup internally
- Reducing this function to a single configuration point
However, this is a larger architectural change beyond the current PR scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
skyvern/library/embedded_server_factory.py(2 hunks)skyvern/library/skyvern.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T04:27:55.396Z
Learnt from: stanislaw89
Repo: Skyvern-AI/skyvern PR: 3970
File: skyvern/library/embedded_server_factory.py:7-17
Timestamp: 2025-11-14T04:27:55.396Z
Learning: The `create_embedded_server` function in `skyvern/library/embedded_server_factory.py` is an internal factory method not exposed to customers, so it does not require public documentation.
Applied to files:
skyvern/library/embedded_server_factory.pyskyvern/library/skyvern.py
🧬 Code graph analysis (1)
skyvern/library/skyvern.py (2)
skyvern/forge/sdk/schemas/tasks.py (1)
llm_key(293-305)skyvern/forge/sdk/schemas/task_v2.py (1)
llm_key(61-76)
🪛 Ruff (0.14.4)
skyvern/library/embedded_server_factory.py
91-91: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Run tests and pre-commit hooks
🔇 Additional comments (10)
skyvern/library/embedded_server_factory.py (7)
1-1: LGTM!Adding
Anyimport to support thesettings_overridestype annotation is appropriate.
7-24: LGTM!The expanded function signature maintains backward compatibility with None defaults and follows consistent naming conventions across all provider parameters.
35-43: LGTM!OpenAI and Anthropic provider configurations are correctly implemented with straightforward API key setup and enablement.
56-59: LGTM!Gemini provider configuration follows the same clean pattern as OpenAI and Anthropic.
70-77: LGTM!Groq configuration correctly treats
groq_modelandgroq_api_baseas optional settings with reasonable defaults when only the API key is provided.
79-83: LGTM!LLM key configuration correctly allows overriding the primary and secondary model selection settings.
62-68: I need to verify whether litellm supports Vertex AI with credentials alone or if it strictly requires both project_id and location.Based on my verification, I found that Vertex AI SDK defaults location to 'us-central1', and the Skyvern codebase provides additional defaults in
vertex_cache_manager.py: project_id defaults to "skyvern-production" and location to "global". This means the code will function even when onlyvertex_credentialsis provided, as downstream code applies defensive defaults.However, relying on hardcoded defaults creates a subtle but real issue: if a user provides only credentials without explicit project_id and location values, the system silently uses "skyvern-production" as the project and "global" as the location. These defaults may not match the user's actual GCP environment, leading to authentication or resource errors at runtime rather than during configuration.
Update Vertex AI configuration to require either full credentials or explicit project/location values
Similar to Azure, Vertex AI should enforce proper configuration. While defaults exist downstream, relying on them silently is error-prone. Consider:
if vertex_credentials: settings.VERTEX_CREDENTIALS = vertex_credentials if vertex_project_id: settings.VERTEX_PROJECT_ID = vertex_project_id if vertex_location: settings.VERTEX_LOCATION = vertex_location # Add validation: ensure project_id and location are set (either explicitly or rely on documented defaults) settings.ENABLE_VERTEX_AI = TrueEither document that "skyvern-production" and "global" are fallback defaults, or require explicit configuration to avoid silent mismatches between user credentials and the hardcoded project.
skyvern/library/skyvern.py (3)
109-148: LGTM! Clear and comprehensive docstring.The embedded mode overload signature and docstring are well-structured with detailed parameter descriptions. The parameter naming uses
settings(user-facing) which is forwarded assettings_overrides(internal) to the embedded server factory, providing a cleaner public API.
150-175: LGTM!The main constructor signature correctly includes all parameters from both remote and embedded mode overloads with proper type annotations.
193-210: LGTM!All provider configuration parameters and settings are correctly forwarded from the constructor to
create_embedded_server. The parameter mapping is complete and accurate, including the translation fromsettings(public API) tosettings_overrides(internal).
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d624d98 in 4 minutes and 1 seconds. Click for details.
- Reviewed
219lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/library/embedded_server_factory.py:41
- Draft comment:
Using 'if azure_config and azure_config.api_key' cleanly checks for valid Azure config; consider raising a clear error if azure_config is provided but lacks an api_key to avoid partial configuration. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is suggesting an improvement to error handling, but it's not pointing out a bug or required change. The current code behavior is consistent with how other config objects are handled (vertex_config and groq_config follow the same pattern). This is a "consider doing X" suggestion, which is speculative and not actionable as a required change. According to the rules, I should not keep speculative comments that suggest what could be better without clear evidence of an issue. The comment also doesn't point to any actual problem - it's just suggesting defensive programming. The comment could be valid if there's evidence that partial configuration is a common user error that needs to be caught. The suggestion for better error handling could improve user experience. I might be missing context about whether this is a common pain point. While better error handling could be nice, this is a speculative "consider" suggestion without evidence of an actual problem. The current behavior is consistent across all config objects in this file, suggesting it's an intentional design pattern. Without strong evidence that this is causing issues, this is just a code quality suggestion that isn't clearly required. This comment should be deleted. It's a speculative suggestion using "consider" language, not pointing out a clear bug or required change. The current behavior is consistent with the pattern used for other config objects, and there's no evidence this is causing problems.
2. skyvern/library/embedded_server_factory.py:80
- Draft comment:
The settings_overrides loop applies arbitrary attribute overrides on settings. Consider enforcing a whitelist to prevent accidental changes to critical settings. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skyvern/library/llm_config_types.py:4
- Draft comment:
Dataclass definitions for AzureConfig, VertexConfig, and GroqConfig are clear and concise. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. skyvern/library/skyvern.py:146
- Draft comment:
Embedded mode init has been updated to use the new config dataclasses (azure_config, vertex_config, groq_config). Ensure that these changes are reflected consistently in documentation and downstream usage. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
Workflow ID: wflow_4i2fYYL5iqaPBqkx
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
skyvern/library/embedded_server_factory.py (3)
40-49: Require a complete Azure configuration before enabling it.Right now we enable Azure (
ENABLE_AZURE = True) as soon asazure_config.api_keyis present, even ifdeployment,api_base, orapi_versionare still missing. Azure OpenAI requires all four fields for a valid deployment. Enabling Azure with a partial configuration will cause runtime failures when the client tries to connect.Gate the
ENABLE_AZURE = Trueon all required fields being present:# Azure if azure_config and azure_config.api_key: - settings.AZURE_API_KEY = azure_config.api_key - if azure_config.deployment: - settings.AZURE_DEPLOYMENT = azure_config.deployment - if azure_config.api_base: - settings.AZURE_API_BASE = azure_config.api_base - if azure_config.api_version: - settings.AZURE_API_VERSION = azure_config.api_version - settings.ENABLE_AZURE = True + if not all([azure_config.deployment, azure_config.api_base, azure_config.api_version]): + missing = [ + field for field, value in [ + ("deployment", azure_config.deployment), + ("api_base", azure_config.api_base), + ("api_version", azure_config.api_version), + ] if not value + ] + raise ValueError(f"Azure configuration incomplete. Missing required fields: {', '.join(missing)}") + settings.AZURE_API_KEY = azure_config.api_key + settings.AZURE_DEPLOYMENT = azure_config.deployment + settings.AZURE_API_BASE = azure_config.api_base + settings.AZURE_API_VERSION = azure_config.api_version + settings.ENABLE_AZURE = True
80-86: Address the TRY003 lint violation.The
ValueError(f"Invalid setting: {key}")violates Ruff's TRY003 rule, which requires exception messages to be defined inside the exception class rather than as f-strings. This will fail CI.Create a custom exception class:
+class InvalidSettingError(ValueError): + """Raised when an invalid setting key is provided.""" + def __init__(self, key: str) -> None: + super().__init__(f"Invalid setting: {key}") + + def create_embedded_server( openai_api_key: str | None = None, ...Then update the raise statement:
if settings_overrides: for key, value in settings_overrides.items(): if hasattr(settings, key): setattr(settings, key, value) else: - raise ValueError(f"Invalid setting: {key}") + raise InvalidSettingError(key)
88-88: Remove unused noqa directive.The
noqa: PLC0415directive is flagged as unused by Ruff. The import placement after settings configuration is correct, but the noqa comment is unnecessary.Apply this diff:
- from skyvern.forge.api_app import app # noqa: PLC0415 + from skyvern.forge.api_app import app
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
skyvern/library/embedded_server_factory.py(2 hunks)skyvern/library/llm_config_types.py(1 hunks)skyvern/library/skyvern.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T04:27:55.396Z
Learnt from: stanislaw89
Repo: Skyvern-AI/skyvern PR: 3970
File: skyvern/library/embedded_server_factory.py:7-17
Timestamp: 2025-11-14T04:27:55.396Z
Learning: The `create_embedded_server` function in `skyvern/library/embedded_server_factory.py` is an internal factory method not exposed to customers, so it does not require public documentation.
Applied to files:
skyvern/library/skyvern.pyskyvern/library/embedded_server_factory.py
🧬 Code graph analysis (2)
skyvern/library/skyvern.py (3)
skyvern/library/llm_config_types.py (3)
AzureConfig(5-11)GroqConfig(24-29)VertexConfig(15-20)skyvern/forge/sdk/schemas/tasks.py (1)
llm_key(293-305)skyvern/forge/sdk/schemas/task_v2.py (1)
llm_key(61-76)
skyvern/library/embedded_server_factory.py (1)
skyvern/library/llm_config_types.py (3)
AzureConfig(5-11)GroqConfig(24-29)VertexConfig(15-20)
🪛 Ruff (0.14.4)
skyvern/library/embedded_server_factory.py
86-86: Avoid specifying long messages outside the exception class
(TRY003)
88-88: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Run tests and pre-commit hooks
🔇 Additional comments (12)
skyvern/library/llm_config_types.py (3)
4-11: Configuration looks good for a simple data container.The optional fields are appropriate here since validation and completeness checks happen at the usage site in
embedded_server_factory.py.
14-20: LGTM!Clean data container for Vertex AI configuration.
23-29: LGTM!Consistent with the other configuration dataclasses.
skyvern/library/embedded_server_factory.py (6)
9-19: Function signature updated appropriately.The signature now supports multiple LLM providers with both simple string keys (OpenAI, Anthropic, Gemini) and structured config objects (Azure, Vertex, Groq), along with general settings overrides.
30-33: LGTM!OpenAI configuration is straightforward and correct.
35-38: LGTM!Anthropic configuration follows the same clean pattern as OpenAI.
51-54: LGTM!Gemini configuration is correct.
65-72: Groq configuration looks reasonable.Groq configuration enables the provider when
api_keyis present, with optionalmodelandapi_baseoverrides. This pattern seems appropriate if Groq can function with just an API key (similar to OpenAI/Anthropic).
74-78: LGTM!LLM key configuration is straightforward and correct.
skyvern/library/skyvern.py (3)
14-14: LGTM!Import of configuration types is appropriate.
106-135: Well-documented embedded mode overload.The overload signature and docstring clearly document all the new configuration parameters for embedded mode. The documentation provides helpful context for each parameter.
137-184: Constructor properly forwards all configuration parameters.The main constructor correctly accepts all new parameters and forwards them to
create_embedded_server. The parameter renaming fromsettingstosettings_overrides(line 182) is appropriate to match the internal factory function's parameter name.
| # Vertex AI | ||
| if vertex_config and vertex_config.credentials: | ||
| settings.VERTEX_CREDENTIALS = vertex_config.credentials | ||
| if vertex_config.project_id: | ||
| settings.VERTEX_PROJECT_ID = vertex_config.project_id | ||
| if vertex_config.location: | ||
| settings.VERTEX_LOCATION = vertex_config.location | ||
| settings.ENABLE_VERTEX_AI = True |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure complete Vertex AI configuration before enabling.
Similar to the Azure issue, this code enables Vertex AI when only credentials is present, but Vertex AI typically requires credentials, project_id, and location to function properly. Enabling with partial configuration may cause runtime failures.
Verify whether Vertex AI can function with only credentials, or if all three fields are required. If all are required, apply similar validation as suggested for Azure:
# Vertex AI
if vertex_config and vertex_config.credentials:
- settings.VERTEX_CREDENTIALS = vertex_config.credentials
- if vertex_config.project_id:
- settings.VERTEX_PROJECT_ID = vertex_config.project_id
- if vertex_config.location:
- settings.VERTEX_LOCATION = vertex_config.location
- settings.ENABLE_VERTEX_AI = True
+ if not all([vertex_config.project_id, vertex_config.location]):
+ missing = [
+ field for field, value in [
+ ("project_id", vertex_config.project_id),
+ ("location", vertex_config.location),
+ ] if not value
+ ]
+ raise ValueError(f"Vertex AI configuration incomplete. Missing required fields: {', '.join(missing)}")
+ settings.VERTEX_CREDENTIALS = vertex_config.credentials
+ settings.VERTEX_PROJECT_ID = vertex_config.project_id
+ settings.VERTEX_LOCATION = vertex_config.location
+ settings.ENABLE_VERTEX_AI = TrueYou can also verify Vertex AI requirements by checking the documentation:
🌐 Web query:
What are the required configuration parameters for Google Vertex AI API client?
💡 Result:
Required / essential configuration parameters (typical) for a Vertex AI API client
- project (Google Cloud project ID). [1][3]
- location (region, e.g., us-central1). [1][3]
- Authentication: one of
- credentials (Google OAuth2 / service-account credentials / ADC), or
- api_key (API key) — mutually exclusive with credentials and with project+location in some SDKs. [1][2][3]
- Endpoint / base URL or vertexai flag when using SDKs that support both Gemini and Vertex backends (selects Vertex API endpoints). [1][8]
Common optional HTTP/client settings: timeout, headers, retry policy, http_options. [1][3]
Common environment variables used instead of passing values: GOOGLE_CLOUD_PROJECT, GOOGLE_CLOUD_LOCATION, GOOGLE_APPLICATION_CREDENTIALS (service-account JSON), GOOGLE_API_KEY. [2][4]
Notes: many client libraries validate that Vertex mode requires either (project + location) or an api_key and enforce mutual-exclusivity between api_key and credentials/project+location. [1][2][3]
Sources: [1] SDK client docs/README; [2] Go client configuration notes; [3] Python SDK installation & validation; [4] common env var usage examples.
🏁 Script executed:
#!/bin/bash
# Find how Vertex AI settings are used in the codebase
rg -n "ENABLE_VERTEX_AI|VERTEX_CREDENTIALS|VERTEX_PROJECT_ID|VERTEX_LOCATION" \
-C 3 --type pyLength of output: 19431
🏁 Script executed:
#!/bin/bash
# Check Azure configuration for comparison pattern
rg -n "ENABLE_AZURE|AZURE_CREDENTIALS" \
-C 3 --type pyLength of output: 12318
🏁 Script executed:
#!/bin/bash
# Look for where these settings are used to initialize Vertex AI client
rg -n "vertex" -i --type py -C 2 | head -100Length of output: 6769
Ensure Vertex AI configuration is complete before enabling.
Vertex AI requires project ID, location, and credentials as mandatory configuration parameters. The current code enables Vertex AI when only credentials is present, leaving project_id and location as optional. While downstream code in vertex_cache_manager.py provides defaults, this defers validation and risks passing None values to litellm if models are invoked before cache manager initialization.
# Vertex AI
if vertex_config and vertex_config.credentials:
- settings.VERTEX_CREDENTIALS = vertex_config.credentials
- if vertex_config.project_id:
- settings.VERTEX_PROJECT_ID = vertex_config.project_id
- if vertex_config.location:
- settings.VERTEX_LOCATION = vertex_config.location
- settings.ENABLE_VERTEX_AI = True
+ if not all([vertex_config.project_id, vertex_config.location]):
+ missing = [
+ field for field, value in [
+ ("project_id", vertex_config.project_id),
+ ("location", vertex_config.location),
+ ] if not value
+ ]
+ raise ValueError(f"Vertex AI configuration incomplete. Missing required fields: {', '.join(missing)}")
+ settings.VERTEX_CREDENTIALS = vertex_config.credentials
+ settings.VERTEX_PROJECT_ID = vertex_config.project_id
+ settings.VERTEX_LOCATION = vertex_config.location
+ settings.ENABLE_VERTEX_AI = True🤖 Prompt for AI Agents
In skyvern/library/embedded_server_factory.py around lines 56 to 63, the code
enables Vertex AI when only credentials are present; change the logic to require
credentials, project_id, and location before setting VERTEX_CREDENTIALS,
VERTEX_PROJECT_ID, VERTEX_LOCATION and ENABLE_VERTEX_AI. Concretely: check that
vertex_config.credentials, vertex_config.project_id, and vertex_config.location
are all truthy; if any are missing, do not enable Vertex AI (optionally log a
warning mentioning which fields are missing) so downstream code never receives
None for these settings.
🔧 This PR significantly expands the embedded mode configuration for the Skyvern SDK by adding comprehensive LLM provider support and flexible settings overrides, replacing the single
open_api_keyparameter with dedicated configuration options for OpenAI, Anthropic, Azure, and Gemini providers.🔍 Detailed Analysis
Key Changes
open_api_keytoopenai_api_keyfor consistency and clarity across the codebasesettings_overridesparameter allowing users to customize any Skyvern setting (e.g., browser type, max steps per run)Technical Implementation
flowchart TD A[AsyncSkyvern.__init__] --> B{Environment Check} B -->|Embedded Mode| C[create_embedded_server] C --> D[Configure LLM Providers] D --> E[OpenAI Settings] D --> F[Anthropic Settings] D --> G[Azure Settings] D --> H[Gemini Settings] I[Apply Settings Overrides] --> J[Validate Settings] J --> K[Create ASGI Transport] K --> L[Return AsyncSkyvern Instance] E --> I F --> I G --> I H --> IImpact
open_api_keyis renamed toopenai_api_key, the change maintains the same functionality with improved naming consistencyCreated with Palmier
🔧 This PR significantly expands the embedded mode configuration for the Skyvern SDK by adding comprehensive LLM provider support and flexible settings overrides, replacing the single
open_api_keyparameter with dedicated configuration options for OpenAI, Anthropic, Azure, and Gemini providers.🔍 Detailed Analysis
Key Changes
open_api_keytoopenai_api_keyfor consistency and clarity across the codebasesettings_overridesparameter allowing users to customize any Skyvern setting (e.g., browser type, max steps per run)Technical Implementation
flowchart TD A[AsyncSkyvern.__init__] --> B{Environment Check} B -->|Embedded Mode| C[create_embedded_server] C --> D[Configure LLM Providers] D --> E[OpenAI Settings] D --> F[Anthropic Settings] D --> G[Azure Settings] D --> H[Gemini Settings] I[Apply Settings Overrides] --> J[Validate Settings] J --> K[Create ASGI Transport] K --> L[Return AsyncSkyvern Instance] E --> I F --> I G --> I H --> IImpact
open_api_keyis renamed toopenai_api_key, the change maintains the same functionality with improved naming consistencyCreated with Palmier
Important
This PR expands Skyvern SDK's embedded mode with multi-provider LLM support and a flexible settings override system.
create_embedded_server()inembedded_server_factory.py.AzureConfig,VertexConfig, andGroqConfiginllm_config_types.py.settings_overridesparameter increate_embedded_server()to allow custom settings.open_api_keytoopenai_api_keyacross the codebase.skyvern.pyto include new parameters and usage examples.This description was created by
for d624d98. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit