fix: silent error handling and defensive improvements#2450
fix: silent error handling and defensive improvements#2450AdrianBesleaga wants to merge 1 commit intoi-am-bee:mainfrom
Conversation
…anges and removals, and fix provider patch logic to correctly handle `None` and empty string updates.
dosubot
bot
added
the
size:L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces several critical fixes to improve error handling and data integrity within the system. It addresses issues where errors were silently swallowed, environment variable changes were not fully detected, and optional fields could not be properly cleared. These changes enhance the robustness and predictability of the application's behavior, particularly in how it manages provider configurations and environment variables. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 several valuable fixes for silent error handling and defensive programming. The change to raise an exception instead of returning silently in update_provider_env improves error propagation. The modification in _load_variables to track environment variable values in addition to keys ensures that value changes are correctly detected. Finally, using is not None in patch_provider allows for clearing optional fields with falsy values. The inclusion of unit tests for all three fixes is excellent. I have one suggestion to improve code consistency.
| updated_provider.watsonx_project_id = watsonx_project_id if watsonx_project_id is not None else updated_provider.watsonx_project_id | ||
| updated_provider.watsonx_space_id = watsonx_space_id if watsonx_space_id is not None else updated_provider.watsonx_space_id | ||
|
|
||
| updated_api_key = api_key or old_api_key | ||
| updated_api_key = api_key if api_key is not None else old_api_key |
There was a problem hiding this comment.
While these changes correctly use is not None for updating optional fields, there's a remaining inconsistency in this block. The updates for type and base_url still use the or operator, while name, description, and the fields you've updated now use the safer if ... is not None pattern. For consistency and to prevent potential future bugs, it would be best to apply the same pattern to type and base_url as well. While it may not be a bug now, standardizing on this safer pattern improves maintainability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38b228b700
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._all_configured_variables.update(variables) | ||
|
|
||
| if dirty := old_variables != self._all_configured_variables: | ||
| logger.info(f"Environment variables reloaded dynamically: {self._all_configured_variables}") |
There was a problem hiding this comment.
Because _all_configured_variables now stores the full {name: value} mapping, the existing logger.info(...) call emits every provider variable value on first load and whenever any variable changes. In the common case where provider variables include secrets such as API keys, this writes plaintext credentials into application logs; before this change only variable names were tracked. Please log only the keys (or a count) here.
Useful? React with 👍 / 👎.
| updated_provider.watsonx_project_id = watsonx_project_id if watsonx_project_id is not None else updated_provider.watsonx_project_id | ||
| updated_provider.watsonx_space_id = watsonx_space_id if watsonx_space_id is not None else updated_provider.watsonx_space_id |
There was a problem hiding this comment.
Switching these fields to is not None lets callers send "" to clear a WatsonX project/space ID, but updated_provider is mutated after model_copy() so ModelProvider.validate_watsonx_config() is never re-run. If a WatsonX provider is patched with watsonx_project_id="" and no replacement watsonx_space_id, uow.model_providers.update() will save an invalid row, and later reads fail when SqlAlchemyModelProviderRepository._row_to_model_provider() reconstructs the model. Normalizing empty strings back to None or revalidating before save would avoid bricking that provider.
Useful? React with 👍 / 👎.
1 participant
fix: silent error handling and defensive improvements
update_provider_envsilently swallows errors when provider lookup fails (return→raise). Callers get no error on invalid provider_id — breaks error handling contract._load_variablesdirty detection tracks values not just keys (set→dict). Value changes bypass reload logging and missing-env validation.patch_providerusesis not Noneinstead oforfor optional watsonx fields.orprevents clearing fields to falsy values like empty string.Added unit tests for all three fixes. All existing tests pass (12/12 SDK, 107/107 server).