feat(chat): wire chat role into ProviderResolver.resolve_with_fallback (#55) - #75
feat(chat): wire chat role into ProviderResolver.resolve_with_fallback (#55)#75lunnynight wants to merge 1 commit into
Conversation
2233admin#55) The failover resolver (PR-D) had no production caller: chat.py still picked the first enabled provider directly, so model_defaults.candidates order and cooldown were dead UI surface. - chat endpoint: when the chat role has model_defaults.candidates, run through resolve_with_fallback() — connection-level failures (connect error/timeout/5xx, decision 2233admin#7) fail over to the next candidate, business failures (4xx) re-raise immediately. Explicit provider_id and roles with no candidates keep the legacy single-provider path unchanged. - tool loops (JSON + XML) now raise LlmAdapterError with classify_retryable instead of a bare 502, so the resolver can distinguish failover-worthy failures; 502 conversion stays at the endpoint boundary. - resolver: skip disabled providers in resolve()/resolve_with_fallback() (governance choice, not liveness failure — no cooldown), add public has_candidates() for the legacy fallback branch. - tests: unit coverage for disabled-skip + has_candidates; new tests/integration/test_chat_failover_api.py exercises the production chain through POST /api/v1/chat (legacy path, retryable failover, business-error no-failover, all-unavailable 502, disabled-skip, explicit-provider bypass). - fix latent patch leak in test_resolver concurrency test (per-task unittest.mock.patch around an await races under asyncio.gather and leaked a get_adapter mock into later tests).
|
✅ Health of changed files: 6.3 → 6.8 (+0.6) 📋 At a glance Files & modules (2)
✅ Health gate: passed 📌 Before you merge
🔎 More signals (3)🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (2 with dependents)"]
f_backend_api_v1_chat_py[".../v1/chat.py 🔥"]:::changed
f_backend_llm_resolver_py["backend/llm/resolver.py 🔥"]:::changed
end
f_backend_api_v1___init___py[".../v1/__init__.py"]
f_backend_api_v1_chat_py --> f_backend_api_v1___init___py
f_backend_llm___init___py["backend/llm/__init__.py"]
f_backend_llm_resolver_py --> f_backend_llm___init___py
t_tests_integration_test_chat_api_py(["✅ tests/integration/test_chat_api.py"]):::guard
t_tests_integration_test_chat_api_py -.-> f_backend_api_v1_chat_py
t_tests_unit_llm_test_pr_e_consumers_py(["✅ .../llm/test_pr_e_consumers.py"]):::guard
t_tests_unit_llm_test_pr_e_consumers_py -.-> f_backend_llm_resolver_py
classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Solid arrows: code that imports the changed files (2 direct dependents, from the last indexed snapshot). Dashed: history/tests. 🔥 Hotspots touched (3)
💀 Dead code (1 finding)
👀 Suggested reviewers @2233admin 📊 See the full report for this PR |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe chat endpoint now supports resolver-based provider failover with classified LLM errors. Disabled providers are skipped without adapter creation or cooldown state. Integration and unit tests cover legacy routing, failover, error handling, and explicit provider selection. ChangesChat provider failover
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatEndpoint
participant ProviderResolver
participant ChatClient
ChatEndpoint->>ProviderResolver: Resolve chat candidates
ProviderResolver->>ChatClient: Create adapter for enabled candidate
ChatEndpoint->>ChatClient: Run shared tool loop
ChatClient-->>ChatEndpoint: Response or classified LLM error
ChatEndpoint->>ProviderResolver: Try next candidate after retryable failure
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/v1/chat.py`:
- Line 445: Remove the hard-coded "gpt-4o-mini" fallback from the legacy model
selection at backend/api/v1/chat.py lines 445-445 and resolve missing
provider.default_model through the Codex/OMX runtime, or reject missing
configuration. Apply the same runtime resolution to candidate failover at
backend/api/v1/chat.py lines 473-473, ensuring both paths avoid hard-coded model
IDs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 602717ea-6110-4602-accc-776c34c466c7
📒 Files selected for processing (4)
backend/api/v1/chat.pybackend/llm/resolver.pytests/integration/test_chat_failover_api.pytests/unit/llm/test_resolver.py
| """ | ||
| provider = await _pick_provider(db, provider_id) | ||
| client = await _build_client(provider) | ||
| model = provider.default_model or "gpt-4o-mini" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Remove the hard-coded provider model fallback.
Both paths select "gpt-4o-mini" when configuration has no model ID. This bypasses the required Codex/OMX runtime model resolution.
backend/api/v1/chat.py#L445-L445: resolve the legacy-path fallback through the runtime, or reject missing model configuration.backend/api/v1/chat.py#L473-L473: use the same runtime resolution for candidate failover.
As per coding guidelines, “Never hardcode configured model IDs in application code, scripts, or committed runtime configuration; resolve them through the Codex/OMX runtime.”
📍 Affects 1 file
backend/api/v1/chat.py#L445-L445(this comment)backend/api/v1/chat.py#L473-L473
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/api/v1/chat.py` at line 445, Remove the hard-coded "gpt-4o-mini"
fallback from the legacy model selection at backend/api/v1/chat.py lines 445-445
and resolve missing provider.default_model through the Codex/OMX runtime, or
reject missing configuration. Apply the same runtime resolution to candidate
failover at backend/api/v1/chat.py lines 473-473, ensuring both paths avoid
hard-coded model IDs.
Source: Coding guidelines
1 participant
Closes #55 — 选择「接入路径」：把
chatrole 接入ProviderResolver.resolve_with_fallback()。做了什么
model_defaults.candidates为chatrole 配置了候选时，走resolve_with_fallback()—— 连接级失败（connect error/timeout/5xx，decision feat(model-provider): provider management (GOAL-6) #7）按顺序故障转移到下一候选；业务级失败（4xx）立即重抛不转移。显式provider_id与无候选 role 保持原有单 provider 行为（兼容既有安装）。LlmAdapterError（classify_retryable区分），由 resolver 决定是否转移；502 转换收敛在端点边界。resolve()/resolve_with_fallback()跳过enabled=False的 provider（治理选择，非活性故障，不进 cooldown）；新增公开has_candidates()供 legacy 回退分支使用。tests/integration/test_chat_failover_api.py，通过真实POST /api/v1/chat链路验证：legacy 路径、可重试故障转移、4xx 不转移、全部不可用 502、disabled 跳过、显式 provider 绕过。test_resolver.py并发测试的 patch 泄漏（asyncio.gather内逐任务unittest.mock.patch围绕await导致属性还原竞争，把 mock 泄漏到后续测试）。验证证据
pip install socksio消除环境性 flaky，见下）。mypy变更文件通过；ruff无新增错误（仅存量 E501/UP045）。POST /{id}/test连接测试 ok:true、模型 sync 发现 2 个模型、PUT/GET /model-defaults默认顺序持久化、chat 错误反馈（SSRF guard 502、无 provider 400、非法 role/未注册 model 400）。遗留（需人工）
127.0.0.1:7897），沙箱偶发注入ALL_PROXY=socks5://...，httpx 0.28.1 在AsyncClient()构造时急切构建 SOCKS transport → 无socksio时抛 ImportError，导致一批真实 HTTP 测试偶发失败（与本次改动无关，tests/integration单独跑不包含本 PR 文件时同样复现 17 个）。建议 dev 依赖补httpx[socks]。