Refactor app config into modular components #690
Conversation
|
Warning Rate limit exceeded@matdev83 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ 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 |
![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.
ℹ️ 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".
| def model_is_functional(self, model_id: str) -> bool: | ||
| """Check if a model is available in any functional backend.""" | ||
| return self.backends.model_is_functional(model_id) |
There was a problem hiding this comment.
AppConfig.model_is_functional now delegates to self.backends.model_is_functional, but the refactored BackendSettings in _app_config_models.py no longer defines that method. Calls such as persistence.collect() use this path to decide whether a model can be used, so the first call will raise AttributeError: 'BackendSettings' object has no attribute 'model_is_functional'. Reintroduce the helper on BackendSettings or move the logic into AppConfig to avoid runtime crashes.
Useful? React with 👍 / 👎.
| def __init__(self, **data: Any) -> None: | ||
| known_fields = set(self.model_fields.keys()) | ||
| init_data = {key: value for key, value in data.items() if key in known_fields} | ||
| backend_data = {key: value for key, value in data.items() if key not in known_fields} | ||
| super().__init__(**init_data) | ||
|
|
||
| for backend_name, config_data in backend_data.items(): | ||
| if isinstance(config_data, dict): | ||
| self.__dict__[backend_name] = BackendConfig(**config_data) | ||
| elif isinstance(config_data, BackendConfig): | ||
| self.__dict__[backend_name] = config_data | ||
|
|
||
| for backend_name in backend_registry.get_registered_backends(): | ||
| if backend_name not in self.__dict__: | ||
| self.__dict__[backend_name] = BackendConfig() | ||
|
|
||
| self._initialization_complete = True | ||
|
|
||
| def __getitem__(self, key: str) -> BackendConfig: | ||
| if key in self.__dict__: | ||
| return cast(BackendConfig, self.__dict__[key]) | ||
| raise KeyError(f"Backend '{key}' not found") | ||
|
|
||
| def __setitem__(self, key: str, value: BackendConfig) -> None: | ||
| self.__dict__[key] = value | ||
|
|
||
| def __setattr__(self, name: str, value: Any) -> None: | ||
| if name in {"default_backend"} or name.startswith("_") or name in self.model_fields: | ||
| super().__setattr__(name, value) | ||
| return | ||
| if isinstance(value, BackendConfig): | ||
| config = value | ||
| elif isinstance(value, dict): | ||
| config = BackendConfig(**value) | ||
| else: | ||
| config = BackendConfig() | ||
| self.__dict__[name] = config | ||
|
|
||
| def get(self, key: str, default: Any = None) -> Any: | ||
| return cast(BackendConfig | None, self.__dict__.get(key, default)) | ||
|
|
||
| @property | ||
| def functional_backends(self) -> set[str]: | ||
| functional: set[str] = set() | ||
| registered = backend_registry.get_registered_backends() | ||
| for backend_name in registered: | ||
| if backend_name in self.__dict__: | ||
| config = self.__dict__[backend_name] | ||
| if isinstance(config, BackendConfig) and config.api_key: | ||
| functional.add(backend_name) | ||
|
|
||
| oauth_like: set[str] = set() | ||
| for name in registered: | ||
| if name.endswith("-oauth") or name.startswith("gemini-oauth"): | ||
| oauth_like.add(name) | ||
| if name == "gemini-cli-cloud-project": | ||
| oauth_like.add(name) | ||
|
|
||
| functional.update(oauth_like.intersection(set(registered))) | ||
|
|
||
| for name, cfg in getattr(self, "__dict__", {}).items(): | ||
| if ( | ||
| name == "default_backend" | ||
| or name.startswith("_") | ||
| or not isinstance(cfg, BackendConfig) | ||
| ): | ||
| continue | ||
| if cfg.api_key: | ||
| functional.add(name) |
There was a problem hiding this comment.
BackendSettings assigns dynamic backend entries directly into self.__dict__ for any registered backend (lines 266‑334), but the class no longer overrides model_dump as it did before the split. When persistence collects settings via app_config.backends.model_dump(exclude_defaults=True), those dynamically-added configs are omitted, dropping API keys and timeouts from the persisted configuration. Re-add a custom model_dump that serializes entries in __dict__ or move the dynamic data into pydantic fields so persistence and AppConfig.save keep the backend settings.
Useful? React with 👍 / 👎.
Summary
src/core/config/_app_config_utils.pyand import them fromapp_config.pysrc/core/config/_app_config_models.pysrc/core/config/app_config.pyby re-exporting the new helpers and models, leaving theAppConfigdefinition focused on configuration assemblyTesting
python -m pytest -n0(fails: ModuleNotFoundError: No module named 'pytest_mock')Codex Task