fix: resolve circular imports, bump litellm, fix release tag format#285
Closed
Aureliolo wants to merge 2 commits into
Closed
fix: resolve circular imports, bump litellm, fix release tag format#285Aureliolo wants to merge 2 commits into
Aureliolo wants to merge 2 commits into
Conversation
- Fix Release Please tag format: add `include-component-in-tag: false`
to produce `vX.Y.Z` tags matching Docker workflow's `v*` trigger
- Bump litellm 1.82.0 → 1.82.1 and fix streaming test mocks:
use `ModelResponseStream` instead of `ModelResponse(stream=True)`
- Break circular import chain (config.schema ↔ providers):
- Extract RetryConfig/RateLimiterConfig to core/resilience_config.py
- Extract budget errors to budget/errors.py
- Move routing module imports to TYPE_CHECKING blocks (PEP 649)
- Delete providers/resilience/config.py re-export module
- Remove all backward-compatibility re-exports from engine/errors.py,
engine/__init__.py, and providers/__init__.py
- Update all consumers to import from canonical locations
- Move TYPE_CHECKING blocks to end of import sections in routing modules - Fix inaccurate docstrings in budget/errors.py - Update DESIGN_SPEC.md §15.3 project structure (add budget/errors.py, core/resilience_config.py; remove deleted providers/resilience/config.py) - Update CLAUDE.md package structure descriptions for budget/ and core/ - Add tests/unit/budget/test_errors.py for budget error hierarchy - Add QuotaExhaustedError to engine budget handler parametrize Pre-reviewed by 9 agents, 11 findings addressed
Contributor
Dependency ReviewThe following issues were found:
License Issuesuv.lock
OpenSSF Scorecard
Scanned Files
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR reorganizes module structure by centralizing budget-related exceptions into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Contributor
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 primarily focuses on improving the project's architectural integrity by resolving several circular import issues, which enhances modularity and maintainability. It also includes a minor dependency update for Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes circular import pressure between config/provider/engine layers by relocating shared configuration and budget exceptions into leaf modules, while also updating LiteLLM to 1.82.1 (including streaming test type fixes) and adjusting release-please tagging to match CI’s v* trigger.
Changes:
- Move
RetryConfig/RateLimiterConfigtoai_company.core.resilience_configand update imports; remove the old resilience config re-export shim. - Move budget exhaustion exceptions to
ai_company.budget.errors, update engine/budget/tests accordingly, and add unit coverage for the new budget error hierarchy. - Bump
litellmto1.82.1, update streaming test helpers toModelResponseStream, and fix release-please tag format.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updates locked LiteLLM version to 1.82.1. |
| pyproject.toml | Bumps direct dependency litellm==1.82.1. |
| .github/release-please-config.json | Forces tags to be vX.Y.Z by disabling component-in-tag. |
| DESIGN_SPEC.md | Updates project structure docs for new/moved/removed modules. |
| CLAUDE.md | Updates package structure notes for budget/ and core/. |
| src/ai_company/core/resilience_config.py | New canonical home for retry/rate-limiter config models to avoid cycles. |
| src/ai_company/config/schema.py | Imports and re-exposes resilience config models from core/. |
| src/ai_company/providers/resilience/retry.py | Updates RetryConfig type import to core.resilience_config. |
| src/ai_company/providers/resilience/rate_limiter.py | Switches RateLimiterConfig import to core.resilience_config. |
| src/ai_company/providers/resilience/config.py | Removes the re-export shim module. |
| src/ai_company/providers/resilience/init.py | Stops exporting RetryConfig/RateLimiterConfig from resilience package. |
| src/ai_company/providers/init.py | Stops re-exporting resilience config types at top-level providers API. |
| src/ai_company/providers/routing/strategies.py | Moves RoutingConfig import behind TYPE_CHECKING to break cycles. |
| src/ai_company/providers/routing/router.py | Moves ProviderConfig/RoutingConfig imports behind TYPE_CHECKING. |
| src/ai_company/providers/routing/resolver.py | Moves ProviderConfig import behind TYPE_CHECKING. |
| src/ai_company/providers/routing/_strategy_helpers.py | Moves routing config imports behind TYPE_CHECKING. |
| src/ai_company/engine/errors.py | Removes budget exceptions from engine error hierarchy. |
| src/ai_company/engine/agent_engine.py | Imports/catches BudgetExhaustedError from budget.errors. |
| src/ai_company/engine/init.py | Removes budget exceptions from engine public exports. |
| src/ai_company/budget/errors.py | New leaf-module budget error hierarchy. |
| src/ai_company/budget/enforcer.py | Imports budget exceptions from budget.errors. |
| src/ai_company/budget/init.py | Re-exports budget exceptions from the budget package. |
| tests/unit/providers/resilience/test_retry.py | Updates RetryConfig import to core.resilience_config. |
| tests/unit/providers/resilience/test_rate_limiter.py | Updates RateLimiterConfig import to core.resilience_config. |
| tests/unit/providers/resilience/test_config.py | Updates resilience config imports to core.resilience_config. |
| tests/unit/providers/resilience/conftest.py | Updates resilience config fixture imports to core.resilience_config. |
| tests/unit/providers/drivers/conftest.py | Splits schema imports vs resilience config imports to avoid cycles. |
| tests/integration/providers/test_retry_integration.py | Splits schema imports vs resilience config imports to avoid cycles. |
| tests/integration/providers/conftest.py | Updates streaming helper typing from ModelResponse to ModelResponseStream. |
| tests/unit/engine/test_errors.py | Adjusts expectations: budget errors are no longer EngineError. |
| tests/unit/engine/test_agent_engine_budget.py | Adds QuotaExhaustedError case and updates imports to budget.errors. |
| tests/unit/config/conftest.py | Updates resilience config imports to core.resilience_config. |
| tests/unit/budget/test_errors.py | Adds unit tests for the new budget error hierarchy. |
| tests/unit/budget/test_enforcer_quota.py | Updates QuotaExhaustedError import to budget.errors. |
| tests/unit/budget/test_enforcer.py | Updates budget exception imports to budget.errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR is a well-scoped structural refactoring that resolves circular import chains by relocating two sets of classes to better-positioned leaf modules —
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph core["core/"]
RC["resilience_config.py\nRetryConfig\nRateLimiterConfig"]
end
subgraph budget["budget/"]
BE["errors.py\nBudgetExhaustedError\nDailyLimitExceededError\nQuotaExhaustedError"]
ENF["enforcer.py"]
BINIT["__init__.py"]
end
subgraph config["config/"]
SCH["schema.py\nProviderConfig\nRootConfig"]
end
subgraph engine["engine/"]
AE["agent_engine.py"]
EE["errors.py\nEngineError hierarchy"]
end
subgraph providers["providers/resilience/"]
RH["retry.py\nRetryHandler"]
RL["rate_limiter.py\nRateLimiter"]
end
subgraph routing["providers/routing/"]
ROU["router.py\nresolver.py\nstrategies.py"]
end
RC -->|imported by| SCH
RC -->|TYPE_CHECKING| RH
RC -->|runtime import| RL
BE -->|imported by| ENF
BE -->|imported by| AE
BE -->|re-exported by| BINIT
SCH -->|TYPE_CHECKING| ROU
EE -->|independent of| BE
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import math | ||
| import time | ||
|
|
||
| from ai_company.core.resilience_config import RateLimiterConfig # noqa: TC001 |
There was a problem hiding this comment.
Inconsistent TYPE_CHECKING pattern with retry.py
retry.py (the companion module in this same PR) correctly places RetryConfig inside a TYPE_CHECKING guard, consistent with the project's Python 3.14 / PEP 649 stance (lazy annotations — no from __future__ import annotations needed). rate_limiter.py keeps RateLimiterConfig at module-level and suppresses the ruff TC001 warning with a # noqa comment instead.
Both imports are used only as constructor parameter annotations; neither needs to be resolved at runtime under PEP 649. Aligning rate_limiter.py with retry.py removes the noqa suppression and keeps the two resilience modules consistent:
Suggested change
| from ai_company.core.resilience_config import RateLimiterConfig # noqa: TC001 | |
| from ai_company.observability import get_logger | |
| from ai_company.observability.events.provider import ( | |
| PROVIDER_RATE_LIMITER_PAUSED, | |
| PROVIDER_RATE_LIMITER_THROTTLED, | |
| ) | |
| if TYPE_CHECKING: | |
| from ai_company.core.resilience_config import RateLimiterConfig |
(Add TYPE_CHECKING to the from typing import line above as well.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/providers/resilience/rate_limiter.py
Line: 7
Comment:
**Inconsistent TYPE_CHECKING pattern with `retry.py`**
`retry.py` (the companion module in this same PR) correctly places `RetryConfig` inside a `TYPE_CHECKING` guard, consistent with the project's Python 3.14 / PEP 649 stance (lazy annotations — no `from __future__ import annotations` needed). `rate_limiter.py` keeps `RateLimiterConfig` at module-level and suppresses the ruff `TC001` warning with a `# noqa` comment instead.
Both imports are used only as constructor parameter annotations; neither needs to be resolved at runtime under PEP 649. Aligning `rate_limiter.py` with `retry.py` removes the noqa suppression and keeps the two resilience modules consistent:
```suggestion
from ai_company.observability import get_logger
from ai_company.observability.events.provider import (
PROVIDER_RATE_LIMITER_PAUSED,
PROVIDER_RATE_LIMITER_THROTTLED,
)
if TYPE_CHECKING:
from ai_company.core.resilience_config import RateLimiterConfig
```
(Add `TYPE_CHECKING` to the `from typing import` line above as well.)
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request effectively resolves circular import issues by refactoring resilience configuration and budget-related error classes into separate, lower-level modules. The changes are well-executed, with corresponding updates to imports, documentation, and tests. The use of TYPE_CHECKING guards for routing module imports is a good practice. Additionally, the pull request includes a version bump for litellm with necessary adjustments to streaming test types, and a configuration fix for release tags. The new tests for the budget error hierarchy are a valuable addition, ensuring the new structure is correct. Overall, this is a high-quality contribution that improves the project's architecture and maintainability.
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring to resolve circular import issues by restructuring core configuration and error models. The changes are clean, consistent, and improve the overall architecture of the codebase. The dependency bump for litellm and the associated test fixes are also correctly handled. I have one suggestion to improve the docstring for the new BudgetExhaustedError to make it more accurate and comprehensive.
Comment on lines
+10
to
+20
| """Budget exhaustion signal. | ||
|
|
||
| Used in two contexts: | ||
|
|
||
| 1. Raised directly by :meth:`BudgetEnforcer.check_can_execute` | ||
| when pre-flight budget checks fail (monthly hard stop or daily | ||
| limit exceeded). | ||
| 2. Caught by the engine layer (``AgentEngine.run``) and converted | ||
| into an ``ExecutionResult`` with | ||
| ``TerminationReason.BUDGET_EXHAUSTED``. | ||
| """ |
Contributor
There was a problem hiding this comment.
The docstring for BudgetExhaustedError is slightly incomplete. It mentions that it's raised for 'monthly hard stop or daily limit exceeded', but it's also the base class for QuotaExhaustedError, which is also raised in check_can_execute. To improve clarity and accuracy, I suggest updating the docstring to reflect that it's a base exception for all budget exhaustion signals, including quota limits.
Suggested change
| """Budget exhaustion signal. | |
| Used in two contexts: | |
| 1. Raised directly by :meth:`BudgetEnforcer.check_can_execute` | |
| when pre-flight budget checks fail (monthly hard stop or daily | |
| limit exceeded). | |
| 2. Caught by the engine layer (``AgentEngine.run``) and converted | |
| into an ``ExecutionResult`` with | |
| ``TerminationReason.BUDGET_EXHAUSTED``. | |
| """ | |
| """Base exception for budget exhaustion signals. | |
| This exception and its subclasses are used in two contexts: | |
| 1. Raised by :meth:`BudgetEnforcer.check_can_execute` | |
| when pre-flight budget checks fail (e.g., monthly hard stop, daily | |
| limit, or provider quota exceeded). | |
| 2. Caught by the engine layer (``AgentEngine.run``) and converted | |
| into an ``ExecutionResult`` with | |
| ``TerminationReason.BUDGET_EXHAUSTED``. | |
| """ |
Aureliolo
added a commit
that referenced
this pull request
May 10, 2026
- scripts/check_baseline_growth.py:_count_json_entries fallback to len(payload) when 'locations' key is missing, so flat-dict baselines surface growth instead of returning sentinel 0 (gemini) - scripts/check_baseline_growth.py: validate resolved baseline path stays under REPO_ROOT before reading; closes CodeQL alert #285 (py/path-injection) - .pre-commit-config.yaml: hook regex now accepts underscore-prefixed JSON/TXT baselines like scripts/_workflow_shell_git_commits_baseline.json (CodeRabbit) - scripts/check_no_em_dashes_hook.sh: normalise backslashes before changelog exemption case-match so Windows paths hit the same skip path (CodeRabbit) - Tests: update json fallback expectation, add path-traversal guard test, add Windows-path changelog exemption parametrized cases
Aureliolo
added a commit
that referenced
this pull request
May 10, 2026
- scripts/check_baseline_growth.py: replace _safe_baseline_path resolve()/is_relative_to() with regex-validated basename approach so the user-supplied directory is discarded and only a known-safe REPO_ROOT/scripts/<basename> path is constructed; closes CodeQL alerts #285 (line 151) and #286 (line 134) - scripts/check_baseline_growth.py: emit stderr WARNING when InvalidBaselineError is raised parsing HEAD content instead of silently falling back to head_count=0 (coderabbit) - scripts/check_no_edit_baseline.sh: normalise backslashes before case-match so Windows paths like C:\repo\scripts\foo_baseline.txt hit the same protection as POSIX (coderabbit) - Tests: rewrite path-injection test for regex sanitisation, add corrupt-HEAD warning test, new tests/unit/scripts/test_check_no_edit_baseline.py covering POSIX + Windows-path + non-baseline + malformed-envelope cases
Aureliolo
added a commit
that referenced
this pull request
May 10, 2026
- scripts/check_baseline_growth.py:_count_json_entries fallback to len(payload) when 'locations' key is missing, so flat-dict baselines surface growth instead of returning sentinel 0 (gemini) - scripts/check_baseline_growth.py: validate resolved baseline path stays under REPO_ROOT before reading; closes CodeQL alert #285 (py/path-injection) - .pre-commit-config.yaml: hook regex now accepts underscore-prefixed JSON/TXT baselines like scripts/_workflow_shell_git_commits_baseline.json (CodeRabbit) - scripts/check_no_em_dashes_hook.sh: normalise backslashes before changelog exemption case-match so Windows paths hit the same skip path (CodeRabbit) - Tests: update json fallback expectation, add path-traversal guard test, add Windows-path changelog exemption parametrized cases
Aureliolo
added a commit
that referenced
this pull request
May 10, 2026
- scripts/check_baseline_growth.py: replace _safe_baseline_path resolve()/is_relative_to() with regex-validated basename approach so the user-supplied directory is discarded and only a known-safe REPO_ROOT/scripts/<basename> path is constructed; closes CodeQL alerts #285 (line 151) and #286 (line 134) - scripts/check_baseline_growth.py: emit stderr WARNING when InvalidBaselineError is raised parsing HEAD content instead of silently falling back to head_count=0 (coderabbit) - scripts/check_no_edit_baseline.sh: normalise backslashes before case-match so Windows paths like C:\repo\scripts\foo_baseline.txt hit the same protection as POSIX (coderabbit) - Tests: rewrite path-injection test for regex sanitisation, add corrupt-HEAD warning test, new tests/unit/scripts/test_check_no_edit_baseline.py covering POSIX + Windows-path + non-baseline + malformed-envelope cases
Aureliolo
added a commit
that referenced
this pull request
May 10, 2026
- scripts/check_baseline_growth.py: unify _is_baseline_path and _safe_baseline_path under a single _BASELINE_BASENAME_RE (canonical regex matching txt/json with optional leading underscore + py with required leading underscore); remove _safe_baseline_path entirely so the gate never touches the user-controlled filesystem path (coderabbit) - scripts/check_baseline_growth.py: read staged content via 'git show :<path>' instead of Path.read_text(); index reads reflect what is actually staged and remove the path-injection sink, closing CodeQL alert #285 (coderabbit + codeql) - tests/unit/scripts/test_check_baseline_growth.py: switch tmp_path/REPO_ROOT-monkeypatched fixtures to mocks of _read_staged + _read_head; broaden test_is_baseline_path with traversal/Windows-separator/uppercase/extension cases; add _read_staged subprocess.run tests - tests/unit/scripts/test_check_no_edit_baseline.py: add test_blocks_edit_of_python_baseline_posix and a Windows-path .py case (coderabbit)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RetryConfig/RateLimiterConfigfromconfig.schemato newcore/resilience_config.py, and moveBudgetExhaustedError/DailyLimitExceededError/QuotaExhaustedErrorfromengine/errors.pyto newbudget/errors.py. Delete theproviders/resilience/config.pyre-export shim. Move routing module imports ofconfig.schemabehindTYPE_CHECKINGguards (placed at end of import sections per convention).ModelResponse→ModelResponseStream)"include-component-in-tag": falseto release-please config so tags arevX.Y.Z(notai-company-vX.Y.Z), matching the Docker CIv*triggerbudget/andcore/tests/unit/budget/test_errors.pyfor budget error hierarchy assertions;QuotaExhaustedErroradded to engine budget handler parametrizeTest plan
ruff check— all checks passedruff format— no changes neededmypy src/ tests/— no issues (848 files)pytest tests/ -n auto --cov=ai_company --cov-fail-under=80— 6653 passed, 8 skipped, 94.80% coverageReview coverage
Pre-reviewed by 9 agents (code-reviewer, python-reviewer, pr-test-analyzer, comment-analyzer, type-design-analyzer, logging-audit, resilience-audit, security-reviewer, docs-consistency). 11 findings identified and addressed. No CRITICAL code issues found — all findings were import ordering, docstring accuracy, docs drift, and test coverage gaps.