Refactor hybrid connector into modular package #693
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 16 minutes and 39 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 (14)
✨ 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".
| except asyncio.TimeoutError as exc: | ||
| logger.error( | ||
| "Execution phase timeout after %ss", | ||
| EXECUTION_PHASE_TIMEOUT, | ||
| extra={ | ||
| "phase": "execution", | ||
| "execution_backend": execution_backend, | ||
| "execution_model": execution_model, | ||
| "reasoning_output_length": reasoning_output_length, | ||
| "timeout_seconds": EXECUTION_PHASE_TIMEOUT, | ||
| }, | ||
| ) | ||
| raise BackendError( | ||
| message=f"Execution phase timeout after {EXECUTION_PHASE_TIMEOUT}s", | ||
| code="execution_timeout", | ||
| details={ | ||
| "phase": "execution", | ||
| "execution_backend": execution_backend, | ||
| "execution_model": execution_model, | ||
| "reasoning_output_length": reasoning_output_length, | ||
| "timeout_seconds": EXECUTION_PHASE_TIMEOUT, | ||
| }, | ||
| ) from exc | ||
|
|
||
| except BackendError: | ||
| raise | ||
|
|
||
| except Exception as exc: | ||
| logger.error( | ||
| "Execution phase failed with unexpected error: %s", | ||
| type(exc).__name__, | ||
| extra={ | ||
| "phase": "execution", | ||
| "execution_backend": execution_backend, | ||
| "execution_model": execution_model, | ||
| "reasoning_output_length": reasoning_output_length, | ||
| "error_type": type(exc).__name__, | ||
| "error": str(exc), | ||
| }, | ||
| exc_info=True, | ||
| ) | ||
| raise BackendError( | ||
| message=f"Execution phase failed: {exc}", | ||
| code="execution_phase_failed", | ||
| details={ | ||
| "phase": "execution", | ||
| "execution_backend": execution_backend, | ||
| "execution_model": execution_model, | ||
| "reasoning_output_length": reasoning_output_length, | ||
| "error_type": type(exc).__name__, | ||
| }, | ||
| ) from exc |
There was a problem hiding this comment.
The execution-phase helper now catches every non-BackendError exception and re-wraps it as a generic BackendError (code="execution_phase_failed"). In the previous implementation this block included an except AuthenticationError: raise, allowing invalid or missing credentials from the execution backend to propagate so callers could surface an auth-specific failure. With the refactor, an AuthenticationError from execution_connector.chat_completions will be converted into a BackendError, masking credential issues and preventing upstream code from applying any special handling or remediation. Consider reintroducing an explicit except AuthenticationError: raise before the broad except Exception clause.
Useful? React with 👍 / 👎.
Summary
src/connectors/hybrid.pyfile with a dedicatedsrc/connectors/hybridpackage composed of mixins for model parsing, parameter handling, request preparation, response filtering, and phase orchestrationHybridConnector,ReasoningPhaseResult, and module logger) from the package entry point while wiring helper modules together insideconnector.pyTesting
python -m pytest tests/unit/connectors/test_hybrid_connector.pypython -m pytest tests/unit/connectors/test_hybrid_connector.py -k parameter_override_loggingThe full
python -m pytestsuite still reports pre-existing snapshot, linting, and dependency failures (missingsnapshotfixture, lint/type/dependency guards, etc.).Codex Task