feat(model-provider): provider management (GOAL-6) - #7
Merged
Conversation
…opic catalog (GOAL-6 PR-A)
Data layer for the model-provider-management subsystem (no runtime yet):
- backend/models/provider_model.py (ProviderModel, table provider_models):
real FK provider_id -> model_providers.id ON DELETE CASCADE (unlike the
loose AIAgent.provider_id), unique (provider_id, model_id), model_type
default "llm", capabilities JSON, source {discovered,manual}, enabled.
- backend/models/model_default.py (ModelDefault, table model_defaults):
role unique {chat,executor,enrichment}, candidates JSON ordered list
(first = primary, rest = failover order).
- backend/llm/: closed-set constants + validators (VALID_MODEL_TYPES/ROLES/
SOURCES) and ANTHROPIC_CATALOG hardcoded constant (Anthropic has no
/v1/models discovery) seeded with claude-opus-4-8/sonnet-5/haiku-4-5.
- Pydantic schemas validate model_type/role/source against the closed sets.
- Migration d8e9f0a1b2c3 (down_revision a7v8w9x0y1z2), validated base->head +
downgrade round-trip on a scratch db.
Closed-set validation lives at the Pydantic/backend.llm layer only, matching
the existing provider_type/channel_type convention.
Note for PR-C/E: sqlite doesn't enforce FK by default and this repo never
issues PRAGMA foreign_keys=ON, so DB-level cascade won't fire in production —
delete provider_models explicitly or enable the pragma.
1504 -> 1530 passed, zero regression.
…ocal exemption (GOAL-6 PR-B) Self-built LLM runtime (no litellm): - backend/llm/base.py: ProviderAdapter ABC (chat/list_models/test_connection), LlmAdapterError (no secrets in message), redact_secret helper. - backend/llm/openai_compat.py: OpenAICompatAdapter (openai|local). Builds an AsyncOpenAI client with an SSRF-validated + DNS-rebind-pinned base_url, mirroring skill_channel/openai_processor. - backend/llm/anthropic.py: AnthropicAdapter (claude). list_models() returns the hardcoded ANTHROPIC_CATALOG (no /v1/models discovery upstream). - backend/llm/factory.py: get_adapter(provider) dispatches by provider_type. Decision #6 local-address resolution: url_guard had NO localhost/private-IP exemption, yet provider_type "local" (ollama loopback, model-hotel on the NetBird CGNAT mesh) legitimately lives there. Added a keyword allow_private (default False — every existing call site unchanged, all 38 url_guard tests + full suite green) threaded through every layer. unspecified/multicast/reserved stay always-blocked and DNS-rebind pinning always applies; only OpenAICompatAdapter passes allow_private=True, and only for provider_type=="local". 21 tests (both adapters, url_guard rejection before client build, api_key never in error, factory dispatch, local-loopback allowed vs openai-loopback rejected). 1530 -> 1551 passed, zero regression.
…s API (GOAL-6 PR-C) - backend/services/provider_model_service.py: sync_models upserts discovered models (decision #3 — source="manual" rows never overwritten/deleted; stale discovered rows pruned; idempotent via (provider_id, model_id) unique key), catalog CRUD, delete_provider_models, put_default (validates role closed-set + each candidate references a real provider AND a catalog row). - Endpoints on providers.py: POST /{id}/test, /{id}/models/sync (LlmAdapterError -> 502), GET/POST/PATCH/DELETE /{id}/models[/{row}]; new model_defaults router: GET /model-defaults, PUT /model-defaults/{role}. - DELETE /providers/{id} now explicitly deletes its provider_models first (sqlite never runs PRAGMA foreign_keys=ON, so the FK cascade never fires). - api_key never in any response (test_connection sanitizes; asserted). 24 tests (test success/failure + no-key-leak, sync idempotent + manual preserved + stale prune, catalog CRUD, defaults validation, provider-delete no orphans). 1551 -> 1575 passed, zero regression.
…L-6 PR-D) backend/llm/resolver.py — ProviderResolver resolves a role to a live provider: - resolve(role) returns the primary candidate (or None if unconfigured). - resolve_with_fallback(role, operation) tries candidates in order. Only a connection-level failure (LlmAdapterError.retryable=True) fails over to the next candidate and cools down the failed provider; a business/4xx error (retryable=False) is re-raised immediately — no cooldown, no fallover (decision #7: a 4xx is a config error, failing over would mask it). - In-process cooldown dict (no Redis), injectable monotonic clock for tests. Cooled providers are skipped without building their adapter. Error classification added to backend/llm/base.py (classify_retryable) + LlmAdapterError.retryable flag; adapters set it from the caught SDK exception (openai/anthropic connection/timeout/5xx -> retryable, 4xx/auth -> not). 24 tests (sequential failover + cooldown, 4xx no-failover + no-cooldown, cooldown-window skip + expiry, all-exhausted clear error no-key-leak, 20-way concurrency, classify_retryable). Adapter tests re-run clean (21/21). 1575 -> 1599 passed, zero regression.
… (GOAL-6 PR-E) Consolidate duplicated LLM client construction through backend/llm/factory helpers (build_openai_compat_adapter / build_anthropic_adapter / litellm_prefix_for) — behavior-preserving, zero regression. - factory: build_* helpers take already-resolved fields and hand the adapter a throwaway _provider_view (SimpleNamespace) — so a live-ORM provider isn't mutated with an env-derived key (autoflush would persist it) and dict-config callers work too. Adapters expose get_client() for callers that need the raw guarded client (tool-calling). - chat.py: _build_client goes through the factory; OPENAI_API_KEY env fallback, _pick_provider selection, and the tool-calling loop all preserved. It gains the SSRF guard it previously lacked (decision #6, no test covered it). - skill_channel / openai_processor / claude_processor: client construction via the factory; each keeps its own env fallback, usage logging, and defaults. - crawl4ai: litellm call/LLMConfig untouched (decision #8 exception); only the provider_type -> litellm prefix mapping is centralized in the factory. - runner: agent processor_config override precedence unchanged (locked by a new test). local_processor left as-is (ollama native protocol / timeout knob don't map to the frozen adapter — documented). Resolver (PR-D) intentionally NOT wired into consumers here: decision #8 mandates factory adoption, not resolver adoption, and wiring role-based selection would change existing provider-selection behavior — deferred to keep PR-E zero-regression. 18 tests (agent-override precedence, resolver-absent safety, consumer wiring). 1599 -> 1617 passed, zero regression (full suite verified).
…deprecation (GOAL-6 PR-F) DataSource.ai_config can now reference a ModelProvider via provider_id (the governed path); legacy inline api_key/base_url keeps working but logs a deprecation warning (decision #9 — no hard migration, no field dropped). backend/pipeline/ai_processor.py _resolve_llm_config(): - no provider_id -> ai_config returned byte-identical (same object); only a deprecation warning when inline creds are present. - provider_id resolves -> provider fields (type/api_key/base_url/default_model) win over inline; warns if inline was also supplied. - provider_id unresolvable -> warn + fall back to ai_config (fail-soft, no crash). - DB session opened only when provider_id is set (common path never hits DB). process_with_ai gains resolve_provider (default True); pipeline.py passes resolve_provider=(agent_config is None) so agent-driven runs — which resolve ai_agents.provider_id via runner separately — don't false-positive the deprecation warning. ai_agents.provider_id stays a loose string (decision #9). 5 tests (provider_id path, inline byte-identical + warn, both->provider wins, unresolvable fallback, resolve_provider gate). 1617 -> 1622 passed, zero regression.
…ts, presets (GOAL-6 PR-G) Surface the model-provider-management UI on the Next.js providers page, wiring to PR-C's endpoints (react-query hooks + shadcn/ui, no new zustand): - frontend/components/providers/provider-catalog-panel.tsx: expandable per-provider model catalog table + Sync button (shows added/updated/kept_manual/pruned) + manual add; discovered/manual source badges. - provider-form-dialog.tsx: add/edit provider with quick presets (Claude, OpenAI, model-hotel — prefills a local base_url). Surfaces the provider CRUD endpoints the frontend previously left as dead code. - model-defaults-card.tsx: chat/executor/enrichment roles, orderable candidate lists, PUT per role. - Test-connection button + status badge (ok+latency / red+error). - lib/api types/endpoints/hooks extended (has_api_key/api_key_preview replaces the bogus api_key field; ProviderModelRead/ConnectionTestResult/ModelDefault types). api_key never rendered raw (write-only password field; edit shows api_key_preview). tsc --noEmit clean, eslint clean, next build succeeds. No backend/tests touched. Live click-through needs a running stack (deferred). Completes GOAL-6.
…ider mgmt, agent runtimes, browser-act) Backfill the /loop-driven planning docs deliberately excluded from auto-commit (GOAL*/AUDIT*/GRILL*/HANDOFF* are always human-committed): GOAL.md (strangler-fig channel refactor), GOAL-2..7.md (AuthManager session affinity, api_channel contract review, pipeline reliability, agent onboarding + taxonomy, model provider management, browser-act collection pack), GOAL-agent-runtimes.md (pluggable agent runtime proposal), AUDIT-cybernetic-remediation.md (control-theory audit), GRILL-KICKOFF.md (closeout kickoff), HANDOFF-strangler-fig.md (channel refactor handoff).
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds extensive audit and execution documentation, introduces guarded LLM provider adapters with model catalogs and failover, exposes provider/model management APIs, integrates provider resolution into pipeline consumers, and adds frontend controls plus unit and integration coverage. ChangesAudit and execution documentation
Provider runtime and management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProvidersPage
participant ProviderAPI
participant ProviderModelService
participant ProviderAdapter
participant ModelCatalog
ProvidersPage->>ProviderAPI: test provider or sync models
ProviderAPI->>ProviderModelService: execute provider operation
ProviderModelService->>ProviderAdapter: test connection or list models
ProviderAdapter-->>ProviderModelService: result or discovered model IDs
ProviderModelService->>ModelCatalog: persist catalog changes
ModelCatalog-->>ProviderAPI: catalog/result response
ProviderAPI-->>ProvidersPage: status, counts, or model rows
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
|
✅ Health: 8.7 📋 At a glance Files & modules (2)
🚨 Change risk: 9.8/10 (high)
🔎 More signals (3)🔥 Hotspots touched (3)
🔗 Hidden coupling (2 files)
💀 Dead code (6 findings)
3 more
👀 Suggested reviewers @xujinghua 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-09 22:20 UTC |
![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 implements Phase 1 of the Model Provider Management system (GOAL-6), introducing a self-built model-provider runtime with OpenAI-compatible and Anthropic adapters, a failover resolver, and database models for model catalogs and defaults. It consolidates LLM client construction across existing consumers to enforce SSRF and key-exfil guards consistently, and adds a Next.js frontend interface for managing providers. The review feedback highlights a potential resource leak and connection overhead in both adapters due to the repeated instantiation of unclosed httpx.AsyncClient objects, and a usability issue in the provider form dialog where clearing optional fields during edits is impossible because empty strings are omitted from the PATCH payload.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+63
to
+106
| def __init__(self, provider: Any) -> None: | ||
| super().__init__(provider) | ||
| self._allow_private = getattr(provider, "provider_type", None) == "local" | ||
| self._client: Any = None | ||
| self._pinned_http_client: httpx.AsyncClient | None = None | ||
|
|
||
| async def _get_client(self) -> Any: | ||
| if self._client is not None: | ||
| return self._client | ||
| from openai import AsyncOpenAI | ||
|
|
||
| api_key = self.provider.api_key or "" | ||
| base_url = self.provider.base_url or None | ||
| if base_url: | ||
| try: | ||
| base_url, ips = await avalidate_public_url_and_ip( | ||
| base_url, allow_private=self._allow_private | ||
| ) | ||
| except SSRFValidationError as exc: | ||
| raise LlmAdapterError( | ||
| self._sanitize(f"provider base_url rejected: {exc}") | ||
| ) from exc | ||
| hostname = urlparse(base_url).hostname or "" | ||
| self._pinned_http_client = httpx.AsyncClient( | ||
| transport=PinnedAsyncHTTPTransport( | ||
| hostname, ips, allow_private=self._allow_private | ||
| ) | ||
| ) | ||
| self._client = AsyncOpenAI( | ||
| api_key=api_key, base_url=base_url, http_client=self._pinned_http_client | ||
| ) | ||
| return self._client | ||
|
|
||
| async def aclose(self) -> None: | ||
| """Close the pinned ``http_client`` this adapter opened, if any. | ||
|
|
||
| ``AsyncOpenAI`` does not close an externally-supplied ``http_client`` | ||
| (mirrors ``OpenAIProcessor``'s own cleanup) — callers that create an | ||
| adapter directly (rather than through a request-scoped helper) should | ||
| call this when done with it. | ||
| """ | ||
| if self._pinned_http_client is not None: | ||
| await self._pinned_http_client.aclose() | ||
| self._pinned_http_client = None |
There was a problem hiding this comment.
⚠️ High Severity: Resource Leak & Connection Overhead
Creating a new httpx.AsyncClient instance on every call to _get_client() introduces significant connection overhead (TCP handshake and TLS negotiation) and leads to a severe resource leak (unclosed sockets/clients) because AsyncOpenAI does not close externally-supplied HTTP clients, and the adapter is frequently discarded without aclose() being called (e.g., in chat.py, skill_channel.py, resolver.py, and provider_model_service.py).
To resolve both the resource leak and the connection overhead, we should cache and reuse the httpx.AsyncClient instances globally using a module-level dictionary keyed by (hostname, tuple(ips), allow_private). Since httpx.AsyncClient is thread-safe and coroutine-safe, this is the recommended pattern for persistent connection reuse.
_CLIENT_CACHE: dict[tuple[str, tuple[str, ...], bool], httpx.AsyncClient] = {}
class OpenAICompatAdapter(ProviderAdapter):
"""Adapter for provider_type in {"openai", "local"}.
Builds an openai.AsyncOpenAI client pointed at provider.base_url,
reusing the exact SSRF-guard + DNS-rebind-pinning pattern
backend.channels.skill_channel._build_model_call and
backend.processors.openai_processor.OpenAIProcessor already use for
this same SDK: validate base_url with
avalidate_public_url_and_ip and hand AsyncOpenAI an
http_client whose transport is a PinnedAsyncHTTPTransport bound
to the validated IP(s) — see backend.security.url_guard's module
docstring for the full DNS-rebind-closure mechanism. When base_url
is unset the SDK's own default endpoint is used, unvalidated and
unpinned, exactly as the existing call sites already do.
**Local-address exemption (decision #6 — flag for reviewer
confirmation)**: backend.security.url_guard had *no* existing
localhost/private-IP allowlist before this PR (confirmed by reading the
whole module + its test file — every IP-space check was unconditional).
Yet provider_type == "local" exists specifically for self-hosted
providers that live at exactly the addresses the guard blocks: ollama on
loopback (http://localhost:11434), model-hotel on the NetBird
fleet-mesh CGNAT range (100.64.0.0/10, e.g. 100.80.x.x). Rather
than leave "local" providers permanently unreachable, this adapter adds
a narrow allow_private=True opt-in (see
backend.security.url_guard.is_ip_blocked) that is threaded through
to both the initial validation call and the pinned transport's
connect-time re-check, and is used ONLY when
self.provider.provider_type == "local" — an openai provider's
base_url is always validated with
allow_private=False (the full,
unmodified guard). The connection is still IP-pinned in both cases:
allow_private only changes which addresses pass the block-list
check, not whether DNS-rebind pinning applies.
"""
def __init__(self, provider: Any) -> None:
super().__init__(provider)
self._allow_private = getattr(provider, "provider_type", None) == "local"
self._client: Any = None
async def _get_client(self) -> Any:
if self._client is not None:
return self._client
from openai import AsyncOpenAI
api_key = self.provider.api_key or ""
base_url = self.provider.base_url or None
pinned_client = None
if base_url:
try:
base_url, ips = await avalidate_public_url_and_ip(
base_url, allow_private=self._allow_private
)
except SSRFValidationError as exc:
raise LlmAdapterError(
self._sanitize(f"provider base_url rejected: {exc}")
) from exc
hostname = urlparse(base_url).hostname or ""
cache_key = (hostname, tuple(ips), self._allow_private)
global _CLIENT_CACHE
if cache_key not in _CLIENT_CACHE:
_CLIENT_CACHE[cache_key] = httpx.AsyncClient(
transport=PinnedAsyncHTTPTransport(
hostname, ips, allow_private=self._allow_private
)
)
pinned_client = _CLIENT_CACHE[cache_key]
self._client = AsyncOpenAI(
api_key=api_key, base_url=base_url, http_client=pinned_client
)
return self._client
async def aclose(self) -> None:
"""No-op because pinned clients are cached globally and reused across requests."""
pass
Comment on lines
+51
to
+87
| def __init__(self, provider: Any) -> None: | ||
| super().__init__(provider) | ||
| self._client: Any = None | ||
| self._pinned_http_client: httpx.AsyncClient | None = None | ||
|
|
||
| async def _get_client(self) -> Any: | ||
| if self._client is not None: | ||
| return self._client | ||
| import anthropic | ||
|
|
||
| api_key = self.provider.api_key or "" | ||
| base_url = getattr(self.provider, "base_url", None) or None | ||
| client_kwargs: dict[str, Any] = {"api_key": api_key} | ||
| if base_url: | ||
| # Same SSRF-guard + DNS-rebind-pinning pattern as | ||
| # OpenAICompatAdapter/skill_channel/openai_processor — | ||
| # allow_private is always False here (see class docstring). | ||
| try: | ||
| base_url, ips = await avalidate_public_url_and_ip(base_url) | ||
| except SSRFValidationError as exc: | ||
| raise LlmAdapterError( | ||
| self._sanitize(f"provider base_url rejected: {exc}") | ||
| ) from exc | ||
| hostname = urlparse(base_url).hostname or "" | ||
| self._pinned_http_client = httpx.AsyncClient( | ||
| transport=PinnedAsyncHTTPTransport(hostname, ips) | ||
| ) | ||
| client_kwargs["base_url"] = base_url | ||
| client_kwargs["http_client"] = self._pinned_http_client | ||
| self._client = anthropic.AsyncAnthropic(**client_kwargs) | ||
| return self._client | ||
|
|
||
| async def aclose(self) -> None: | ||
| """Close the pinned ``http_client`` this adapter opened, if any.""" | ||
| if self._pinned_http_client is not None: | ||
| await self._pinned_http_client.aclose() | ||
| self._pinned_http_client = None |
There was a problem hiding this comment.
⚠️ High Severity: Resource Leak & Connection Overhead
Similar to OpenAICompatAdapter, creating a new httpx.AsyncClient instance on every call to _get_client() introduces significant connection overhead and leads to a severe resource leak because AsyncAnthropic does not close externally-supplied HTTP clients, and the adapter is frequently discarded without aclose() being called.
We should cache and reuse the httpx.AsyncClient instances globally using a module-level dictionary keyed by (hostname, tuple(ips), False).
_CLIENT_CACHE: dict[tuple[str, tuple[str, ...], bool], httpx.AsyncClient] = {}
class AnthropicAdapter(ProviderAdapter):
"""Adapter for provider_type == "claude".
Uses anthropic.AsyncAnthropic (mirrors
backend.processors.claude_processor.ClaudeProcessor's SDK usage).
Unlike OpenAICompatAdapter, there is no provider_type == "local"
case here — Anthropic's endpoint is effectively fixed
(https://api.anthropic.com by SDK default), so this adapter never
passes allow_private=True to the guard: a base_url override (rare
— e.g. a proxy in front of the real API) is validated with the full,
unmodified SSRF guard, exactly like openai-type providers.
list_models() returns the hardcoded
backend.llm.catalog.anthropic_catalog model ids rather than
hitting the network — decision #5: Anthropic has no GET /v1/models-
style discovery endpoint.
"""
def __init__(self, provider: Any) -> None:
super().__init__(provider)
self._client: Any = None
async def _get_client(self) -> Any:
if self._client is not None:
return self._client
import anthropic
api_key = self.provider.api_key or ""
base_url = getattr(self.provider, "base_url", None) or None
client_kwargs: dict[str, Any] = {"api_key": api_key}
if base_url:
try:
base_url, ips = await avalidate_public_url_and_ip(base_url)
except SSRFValidationError as exc:
raise LlmAdapterError(
self._sanitize(f"provider base_url rejected: {exc}")
) from exc
hostname = urlparse(base_url).hostname or ""
cache_key = (hostname, tuple(ips), False)
global _CLIENT_CACHE
if cache_key not in _CLIENT_CACHE:
_CLIENT_CACHE[cache_key] = httpx.AsyncClient(
transport=PinnedAsyncHTTPTransport(hostname, ips)
)
client_kwargs["base_url"] = base_url
client_kwargs["http_client"] = _CLIENT_CACHE[cache_key]
self._client = anthropic.AsyncAnthropic(**client_kwargs)
return self._client
async def aclose(self) -> None:
"""No-op because pinned clients are cached globally and reused across requests."""
pass
Comment on lines
+118
to
+121
| if (form.base_url.trim()) payload.base_url = form.base_url.trim() | ||
| if (form.default_model.trim()) payload.default_model = form.default_model.trim() | ||
| if (form.notes.trim()) payload.notes = form.notes.trim() | ||
| if (form.api_key.trim()) payload.api_key = form.api_key.trim() |
There was a problem hiding this comment.
⚠️ Medium Severity: Usability / Correctness Issue
During provider editing (mode === 'edit'), if a user clears optional fields like base_url, default_model, or notes in the form, the empty strings are trimmed and then completely omitted from the PATCH payload because of the if (form.field.trim()) checks.
As a result, the backend never receives these fields and cannot clear them, making it impossible for users to remove a previously configured base_url, default_model, or notes.
We should include these fields in the payload even if they are empty strings (which the backend will treat as clearing the field), while keeping api_key conditionally omitted so we don't overwrite the existing encrypted key.
Suggested change
| if (form.base_url.trim()) payload.base_url = form.base_url.trim() | |
| if (form.default_model.trim()) payload.default_model = form.default_model.trim() | |
| if (form.notes.trim()) payload.notes = form.notes.trim() | |
| if (form.api_key.trim()) payload.api_key = form.api_key.trim() | |
| const payload: ModelProviderInput = { | |
| name: form.name.trim(), | |
| provider_type: form.provider_type, | |
| enabled: form.enabled, | |
| base_url: form.base_url.trim(), | |
| default_model: form.default_model.trim(), | |
| notes: form.notes.trim(), | |
| } | |
| if (form.api_key.trim()) { | |
| payload.api_key = form.api_key.trim() | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (5)
GOAL-3.md (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the status-update rule compatible with the staging rule.
This file is explicitly never staged, yet the DoD requires updating its state after every PR. Either allow intentional commits of control-document updates or state that these updates are local-only; otherwise the recorded checklist cannot reliably represent the branch history.
Also applies to: 39-43
🤖 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 `@GOAL-3.md` around lines 12 - 13, Update the status-update rule in GOAL-3.md and the corresponding sections at the referenced lines so it is consistent with the “never stage” rule: explicitly define control-document updates as either intentionally committable or local-only, and clarify how the checklist should represent progress under that policy.GOAL-2.md (1)
19-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the “zero behavior change” requirement.
PR-C explicitly adds an API-channel deprecation warning, while the acceptance criteria claim old-path behavior is unchanged. State that semantic behavior is unchanged but observability/logging changes, or update the DoD to avoid an ambiguous acceptance condition.
🤖 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 `@GOAL-2.md` around lines 19 - 29, 澄清 PR-C 与“行为零变”验收条件之间的歧义：在“每 PR 验收 / 停止条件”或 PR-C 描述中明确旧路径语义行为保持不变，但允许新增 api_channel 明文凭据的 deprecation warning 等可观测性变化；同步调整相关 DoD 表述，避免将日志变化误判为行为变更。backend/schemas/model_default.py (2)
18-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ModelDefaultPutlooks unused; docstring onModelDefaultCandidatesBodymisdescribes the actual flow.
ModelDefaultCandidatesBody's docstring claims the router wraps this into a role-validated ModelDefaultPut before handing off to backend.services.provider_model_service.put_default, so the same closed-set check that schema already enforces gets reused end-to-end instead of duplicated — butbackend/api/v1/model_defaults.py'sput_model_defaultnever constructs aModelDefaultPut; it validatesroleinline (if role not in VALID_ROLES) and callsprovider_model_service.put_default(db, role, body.candidates)directly.ModelDefaultPut(lines 18-30) appears to be dead code left over from an earlier design, and the docstring is now inaccurate.Either wire
ModelDefaultPutinto the router as documented, or drop the unused class and fix the docstring to describe the real flow.🤖 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/schemas/model_default.py` around lines 18 - 41, Remove the unused ModelDefaultPut schema and update ModelDefaultCandidatesBody’s docstring to describe the actual flow: put_model_default validates the path role inline against VALID_ROLES and directly calls provider_model_service.put_default with body.candidates. Ensure no references to ModelDefaultPut remain.
46-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: use
ConfigDictinstead of a plain dict formodel_config.Ruff flags line 53 as a mutable class default (RUF012). Pydantic v2 merges
model_configacross the MRO regardless of dict vs.ConfigDict, so this isn't a functional bug (UTCModel'sjson_encodersis preserved), but switching topydantic.ConfigDictis the idiomatic v2 pattern and silences the lint warning.♻️ Suggested change
-from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator ... - model_config = {"from_attributes": True} + model_config = ConfigDict(from_attributes=True)🤖 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/schemas/model_default.py` around lines 46 - 53, Replace the plain-dict model_config in ModelDefaultRead with a pydantic ConfigDict instance, adding the appropriate import, while preserving from_attributes=True and inherited configuration.Source: Linters/SAST tools
backend/api/v1/providers.py (1)
62-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valuePrune deleted providers from
model_defaults.candidatestoo
Deleting a provider still leaves itsprovider_idinside the JSON defaults, so the config can drift from the live provider table. If these defaults are meant to stay authoritative, remove or revalidate those candidates here as well.🤖 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/providers.py` around lines 62 - 74, Update delete_provider to also remove or revalidate the deleted provider_id in model_defaults.candidates before committing, using the existing model-defaults persistence/service APIs and preserving valid candidates. Ensure the cleanup occurs alongside provider_model_service.delete_provider_models and the provider deletion so defaults cannot retain references to the deleted provider.
🤖 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`:
- Around line 234-238: Ensure the adapter-owned pinned client is closed after
each chat request when provider.base_url is configured. Update _build_client and
chat to preserve the adapter or expose its cleanup, then call the adapter’s
async close method in a finally block surrounding the chat/tool loop while
retaining existing LlmAdapterError handling.
In `@backend/llm/openai_compat.py`:
- Around line 96-106: Update OpenAICompat.aclose() to also set self._client to
None after closing and clearing self._pinned_http_client, ensuring subsequent
_get_client() calls construct a fresh validated client instead of reusing a
closed AsyncOpenAI instance.
In `@backend/pipeline/ai_processor.py`:
- Around line 45-72: Update _resolve_llm_config to treat a resolved
ModelProvider with enabled=False the same as a missing provider. Check
provider.enabled immediately after get_provider; log the existing fallback
warning and return the inline ai_config without using the disabled provider’s
fields.
In `@backend/security/url_guard.py`:
- Around line 148-154: Update the URL guard logic around allow_private so
link-local addresses remain blocked regardless of that flag. Check
ip.is_link_local before the allow_private early return, preserving existing
blocking for unspecified, multicast, reserved, and link-local addresses while
still permitting legitimate loopback, RFC1918, and CGNAT addresses when
allow_private is true.
In `@backend/services/provider_model_service.py`:
- Around line 69-90: Update add_manual_model to handle IntegrityError raised by
the unique provider_id/model_id constraint during commit. Roll back the session,
then raise the API’s conflict/HTTP 409 exception with a clear duplicate-model
message; preserve other database errors instead of converting them.
- Around line 143-216: Prevent duplicate model IDs from causing multiple inserts
in sync_models(). Deduplicate discovered_ids before processing, or update
existing_by_model_id immediately after adding a new ProviderModel, while
preserving accurate added/updated/kept_manual counts and pruning behavior.
In `@frontend/components/providers/model-defaults-card.tsx`:
- Around line 115-128: Add accessible aria-labels to the icon-only reorder and
removal Buttons in the candidate controls, using distinct labels for moving a
candidate up, moving it down, and removing it; update the buttons around the
move and remove handlers.
In `@frontend/components/providers/provider-catalog-panel.tsx`:
- Around line 120-135: 添加可访问名称到模型删除按钮：在包含 Trash2 图标的 Button 上设置描述性的
aria-label（如“删除模型”），以便屏幕阅读器识别该破坏性操作。
In `@GOAL-4.md`:
- Around line 15-17: Resolve the contradictory retry design documented in
GOAL-4.md: make the locked decision and the completed PR-B record agree on one
authoritative contract. Update the relevant sections to consistently state
whether tasks.py uses autoretry_for or pipeline.py re-raises for Celery
max_retries, ensuring duplicate retry mechanisms are not prescribed.
In `@GOAL-5.md`:
- Around line 65-69: Update the CI workflow to add a backend-test step that
reruns the SKILL.md generator and fails when the generated output differs from
the committed file, or remove the claim from GOAL-5.md if this gate is
intentionally not implemented. Align the workflow’s working directory and
commands with the actual repository layout, and update the GOAL-5 documentation
to accurately describe the enforced checks.
In `@GOAL-6.md`:
- Around line 54-56: The completion claim incorrectly presents provider failover
as delivered while no consumers invoke resolve_with_fallback. Either integrate
resolve_with_fallback at the intended role/model selection boundary for chat,
skill, and processor consumers, preserving existing provider-selection behavior,
or revise GOAL-6 to explicitly mark failover integration as deferred and remove
it from the completion claim.
- Line 40: Clarify the test baseline discrepancy in the PR-A documentation:
state the exact branch/commit and test command producing 1504→1530, and the
branch/commit and command producing 1430→1622. Update the zero-regression claim
to identify which baseline is authoritative and ensure the final summary uses
consistent, auditable figures.
In `@GOAL-7.md`:
- Around line 50-52: The frontend requirement is incomplete because PackCatalog
presets are not wired into a usable source configuration flow. Implement the
browser-act source editor and use a data-fetching hook to load presets from GET
/api/v1/browser-act/packs, allowing users to select a preset and complete
one-click setup; update the channel_type union and CHANNEL_LABEL consistently.
Alternatively, explicitly mark this frontend requirement as deferred rather than
claiming it is complete.
In `@GOAL-agent-runtimes.md`:
- Around line 49-65: Replace the invalid RuntimeEvent declaration with an
enforceable tagged type using Literal for the allowed type values and TypedDict
variants (or an equivalent discriminated union) for event payloads. Update
RuntimeAdapter.invoke to return AsyncIterator[RuntimeEvent] rather than
AsyncIterator[dict], ensuring adapters yield only the defined closed event
shapes.
In `@GOAL.md`:
- Line 17: The test baseline figures and commands in GOAL.md are inconsistent:
reconcile the stated 347 baseline with the reported 325→379 progression. Update
the relevant test-count descriptions to explain which commands and test scopes
produce each count, and clearly define the baseline used for the “at least
previous baseline” DoD.
In `@HANDOFF-strangler-fig.md`:
- Around line 3-4: HANDOFF-strangler-fig.md contains obsolete instructions to
resume at PR2 with an outdated test baseline, despite later records marking
PR2–PR13 and AuthManager complete. Archive or regenerate the handoff and update
all referenced sections to reflect the current completed work, test status, and
next actionable task; remove instructions that would repeat completed work.
- Around line 75-80: 统一 HANDOFF-strangler-fig.md 中的提交授权规则：明确“仅用户明确说 commit 才提交”与
GOAL-2/GOAL-3
自动提交要求的优先级和适用范围，规定哪些场景允许或必须自动提交，并说明冲突时应遵循的规则，确保既不会未经授权提交，也不会遗漏必要提交。
In `@tests/unit/test_model_default.py`:
- Around line 105-110: Replace the mutable class-level candidates attribute in
the _Row stub with an instance attribute, such as by adding an initializer or
using SimpleNamespace, while preserving the existing id, role, and timestamp
values so each fixture instance owns its candidates list and satisfies Ruff
RUF012.
In `@tests/unit/test_provider_model.py`:
- Around line 217-226: Update the `_Row` test stub so its fields, especially
mutable `capabilities`, are initialized in an `__init__` method as instance
attributes rather than class attributes, preserving the existing default values
and avoiding Ruff RUF012.
---
Nitpick comments:
In `@backend/api/v1/providers.py`:
- Around line 62-74: Update delete_provider to also remove or revalidate the
deleted provider_id in model_defaults.candidates before committing, using the
existing model-defaults persistence/service APIs and preserving valid
candidates. Ensure the cleanup occurs alongside
provider_model_service.delete_provider_models and the provider deletion so
defaults cannot retain references to the deleted provider.
In `@backend/schemas/model_default.py`:
- Around line 18-41: Remove the unused ModelDefaultPut schema and update
ModelDefaultCandidatesBody’s docstring to describe the actual flow:
put_model_default validates the path role inline against VALID_ROLES and
directly calls provider_model_service.put_default with body.candidates. Ensure
no references to ModelDefaultPut remain.
- Around line 46-53: Replace the plain-dict model_config in ModelDefaultRead
with a pydantic ConfigDict instance, adding the appropriate import, while
preserving from_attributes=True and inherited configuration.
In `@GOAL-2.md`:
- Around line 19-29: 澄清 PR-C 与“行为零变”验收条件之间的歧义：在“每 PR 验收 / 停止条件”或 PR-C
描述中明确旧路径语义行为保持不变，但允许新增 api_channel 明文凭据的 deprecation warning 等可观测性变化；同步调整相关 DoD
表述，避免将日志变化误判为行为变更。
In `@GOAL-3.md`:
- Around line 12-13: Update the status-update rule in GOAL-3.md and the
corresponding sections at the referenced lines so it is consistent with the
“never stage” rule: explicitly define control-document updates as either
intentionally committable or local-only, and clarify how the checklist should
represent progress under that policy.
🪄 Autofix (Beta)
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
Run ID: fa988144-115d-46fd-bce4-f685ee8c732d
📒 Files selected for processing (56)
AUDIT-cybernetic-remediation.mdGOAL-2.mdGOAL-3.mdGOAL-4.mdGOAL-5.mdGOAL-6.mdGOAL-7.mdGOAL-agent-runtimes.mdGOAL.mdGRILL-KICKOFF.mdHANDOFF-strangler-fig.mdbackend/api/v1/__init__.pybackend/api/v1/chat.pybackend/api/v1/model_defaults.pybackend/api/v1/providers.pybackend/channels/crawl4ai_channel.pybackend/channels/skill_channel.pybackend/llm/__init__.pybackend/llm/anthropic.pybackend/llm/base.pybackend/llm/catalog.pybackend/llm/factory.pybackend/llm/openai_compat.pybackend/llm/resolver.pybackend/migrations/versions/d8e9f0a1b2c3_add_provider_models_and_model_defaults.pybackend/models/__init__.pybackend/models/model_default.pybackend/models/provider_model.pybackend/pipeline/ai_processor.pybackend/pipeline/pipeline.pybackend/processors/claude_processor.pybackend/processors/local_processor.pybackend/processors/openai_processor.pybackend/schemas/model_default.pybackend/schemas/provider_model.pybackend/security/url_guard.pybackend/services/provider_model_service.pyfrontend/app/(app)/providers/page.tsxfrontend/components/providers/model-defaults-card.tsxfrontend/components/providers/provider-catalog-panel.tsxfrontend/components/providers/provider-form-dialog.tsxfrontend/lib/api/endpoints.tsfrontend/lib/api/hooks.tsfrontend/lib/api/types.tstests/integration/test_model_defaults_api.pytests/integration/test_provider_models_api.pytests/unit/llm/__init__.pytests/unit/llm/test_adapters.pytests/unit/llm/test_catalog.pytests/unit/llm/test_pr_e_consumers.pytests/unit/llm/test_resolver.pytests/unit/pipeline/test_ai_processor.pytests/unit/security/test_url_guard.pytests/unit/test_model_default.pytests/unit/test_provider_model.pytests/unit/test_runner.py
Comment on lines
+234
to
+238
| adapter = build_openai_compat_adapter(base_url=provider.base_url, api_key=api_key) | ||
| try: | ||
| return await adapter.get_client() | ||
| except LlmAdapterError as exc: | ||
| raise HTTPException(status_code=502, detail=f"模型调用失败: {exc}") from exc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the pinned client lifecycle and that chat() never closes it.
ast-grep outline backend/llm/openai_compat.py --items all
rg -nP '_pinned_http_client|PinnedAsyncHTTPTransport|http_client' backend/llm/openai_compat.py
rg -nP '\b(aclose|__aexit__|async with)\b' backend/api/v1/chat.pyRepository: 2233admin/opencli-admin
Length of output: 1240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation slices.
wc -l backend/api/v1/chat.py backend/llm/openai_compat.py
echo '--- chat.py around _build_client and chat() ---'
sed -n '180,420p' backend/api/v1/chat.py
echo '--- openai_compat.py around adapter lifecycle ---'
sed -n '1,180p' backend/llm/openai_compat.pyRepository: 2233admin/opencli-admin
Length of output: 18549
Close the pinned client after the chat loop
When provider.base_url is set, build_openai_compat_adapter(...).get_client() creates an adapter-owned pinned httpx.AsyncClient, but _build_client() drops the adapter and chat() never calls aclose(). That leaves a request-scoped connection pool open on every /chat call against a configured base_url. Return the adapter (or close it in chat() after the tool loop) so the pinned client is released.
🤖 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` around lines 234 - 238, Ensure the adapter-owned
pinned client is closed after each chat request when provider.base_url is
configured. Update _build_client and chat to preserve the adapter or expose its
cleanup, then call the adapter’s async close method in a finally block
surrounding the chat/tool loop while retaining existing LlmAdapterError
handling.
Comment on lines
+96
to
+106
| async def aclose(self) -> None: | ||
| """Close the pinned ``http_client`` this adapter opened, if any. | ||
|
|
||
| ``AsyncOpenAI`` does not close an externally-supplied ``http_client`` | ||
| (mirrors ``OpenAIProcessor``'s own cleanup) — callers that create an | ||
| adapter directly (rather than through a request-scoped helper) should | ||
| call this when done with it. | ||
| """ | ||
| if self._pinned_http_client is not None: | ||
| await self._pinned_http_client.aclose() | ||
| self._pinned_http_client = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
aclose() should also clear self._client to prevent use-after-close.
aclose() nulls _pinned_http_client but leaves _client cached. If the adapter is reused after aclose(), _get_client() returns the stale AsyncOpenAI instance whose underlying http_client has been closed, causing silent transport failures. Clearing _client forces a fresh (re-validated, re-pinned) construction on next use.
🛡️ Proposed fix
async def aclose(self) -> None:
if self._pinned_http_client is not None:
await self._pinned_http_client.aclose()
self._pinned_http_client = None
+ self._client = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def aclose(self) -> None: | |
| """Close the pinned ``http_client`` this adapter opened, if any. | |
| ``AsyncOpenAI`` does not close an externally-supplied ``http_client`` | |
| (mirrors ``OpenAIProcessor``'s own cleanup) — callers that create an | |
| adapter directly (rather than through a request-scoped helper) should | |
| call this when done with it. | |
| """ | |
| if self._pinned_http_client is not None: | |
| await self._pinned_http_client.aclose() | |
| self._pinned_http_client = None | |
| async def aclose(self) -> None: | |
| """Close the pinned ``http_client`` this adapter opened, if any. | |
| ``AsyncOpenAI`` does not close an externally-supplied ``http_client`` | |
| (mirrors ``OpenAIProcessor``'s own cleanup) — callers that create an | |
| adapter directly (rather than through a request-scoped helper) should | |
| call this when done with it. | |
| """ | |
| if self._pinned_http_client is not None: | |
| await self._pinned_http_client.aclose() | |
| self._pinned_http_client = None | |
| self._client = None |
🤖 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/llm/openai_compat.py` around lines 96 - 106, Update
OpenAICompat.aclose() to also set self._client to None after closing and
clearing self._pinned_http_client, ensuring subsequent _get_client() calls
construct a fresh validated client instead of reusing a closed AsyncOpenAI
instance.
Comment on lines
+45
to
+72
| from backend.database import AsyncSessionLocal | ||
| from backend.services.provider_model_service import get_provider | ||
|
|
||
| async with AsyncSessionLocal() as session: | ||
| provider = await get_provider(session, provider_id) | ||
|
|
||
| if provider is None: | ||
| logger.warning( | ||
| "DataSource %s ai_config.provider_id=%s does not resolve to an " | ||
| "existing ModelProvider; falling back to inline config", | ||
| source_id, provider_id, | ||
| ) | ||
| return ai_config | ||
|
|
||
| if has_inline: | ||
| logger.warning( | ||
| "DataSource %s ai_config supplies both provider_id=%s and inline " | ||
| "api_key/base_url; provider_id takes precedence", | ||
| source_id, provider_id, | ||
| ) | ||
|
|
||
| resolved = dict(ai_config) | ||
| resolved["processor_type"] = provider.provider_type | ||
| resolved["api_key"] = provider.api_key | ||
| resolved["base_url"] = provider.base_url | ||
| if provider.default_model: | ||
| resolved["model"] = provider.default_model | ||
| return resolved |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## backend/pipeline/ai_processor.py\n'
ast-grep outline backend/pipeline/ai_processor.py --view expanded || true
printf '\n--- relevant slice ---\n'
sed -n '1,140p' backend/pipeline/ai_processor.py | cat -n
printf '\n## backend/models/provider.py\n'
ast-grep outline backend/models/provider.py --view expanded || true
printf '\n--- relevant slice ---\n'
sed -n '1,220p' backend/models/provider.py | cat -n
printf '\n## backend/llm/resolver.py\n'
ast-grep outline backend/llm/resolver.py --view expanded || true
printf '\n--- relevant slice ---\n'
sed -n '1,260p' backend/llm/resolver.py | cat -n
printf '\n## search for enabled checks\n'
rg -n "enabled" backend/pipeline backend/llm backend/models | sed -n '1,200p'Repository: 2233admin/opencli-admin
Length of output: 22827
Skip disabled ModelProvider rows here. _resolve_llm_config uses provider.provider_type / api_key / base_url / default_model as soon as provider_id resolves, but it never checks provider.enabled. That means a DataSource can keep using a provider an admin has explicitly disabled. Treat enabled=False the same as a missing provider and fall back to inline config.
🤖 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/pipeline/ai_processor.py` around lines 45 - 72, Update
_resolve_llm_config to treat a resolved ModelProvider with enabled=False the
same as a missing provider. Check provider.enabled immediately after
get_provider; log the existing fallback warning and return the inline ai_config
without using the disabled provider’s fields.
Comment on lines
+148
to
+154
| if ip.is_unspecified or ip.is_multicast or ip.is_reserved: | ||
| return True | ||
| if allow_private: | ||
| return False | ||
| if isinstance(ip, ipaddress.IPv4Address) and ip in _CGNAT_SHARED_SPACE: | ||
| return True | ||
| return ( | ||
| ip.is_loopback | ||
| or ip.is_private | ||
| or ip.is_link_local | ||
| or ip.is_unspecified | ||
| or ip.is_multicast | ||
| or ip.is_reserved | ||
| ) | ||
| return ip.is_loopback or ip.is_private or ip.is_link_local |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
allow_private re-opens the cloud-metadata SSRF vector via link-local.
When allow_private=True, line 151 returns False for everything except unspecified/multicast/reserved, so link-local (169.254.0.0/16 — including the cloud-metadata address 169.254.169.254, and fe80::/10) becomes reachable. Legitimate provider_type == "local" endpoints live on loopback, RFC1918, or the CGNAT shared space — never link-local — so allowing it only widens the SSRF surface to the metadata endpoint the default guard is specifically designed to block. Keep link-local blocked even under the private exemption.
🛡️ Block link-local regardless of allow_private
if ip.is_unspecified or ip.is_multicast or ip.is_reserved:
return True
+ # Link-local (169.254.0.0/16 incl. cloud-metadata 169.254.169.254,
+ # fe80::/10) is never a legitimate provider endpoint — keep it blocked
+ # even under allow_private so the metadata SSRF defense is preserved.
+ if ip.is_link_local:
+ return True
if allow_private:
return False
if isinstance(ip, ipaddress.IPv4Address) and ip in _CGNAT_SHARED_SPACE:
return True
return ip.is_loopback or ip.is_private or ip.is_link_local📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ip.is_unspecified or ip.is_multicast or ip.is_reserved: | |
| return True | |
| if allow_private: | |
| return False | |
| if isinstance(ip, ipaddress.IPv4Address) and ip in _CGNAT_SHARED_SPACE: | |
| return True | |
| return ( | |
| ip.is_loopback | |
| or ip.is_private | |
| or ip.is_link_local | |
| or ip.is_unspecified | |
| or ip.is_multicast | |
| or ip.is_reserved | |
| ) | |
| return ip.is_loopback or ip.is_private or ip.is_link_local | |
| if ip.is_unspecified or ip.is_multicast or ip.is_reserved: | |
| return True | |
| # Link-local (169.254.0.0/16 incl. cloud-metadata 169.254.169.254, | |
| # fe80::/10) is never a legitimate provider endpoint — keep it blocked | |
| # even under allow_private so the metadata SSRF defense is preserved. | |
| if ip.is_link_local: | |
| return True | |
| if allow_private: | |
| return False | |
| if isinstance(ip, ipaddress.IPv4Address) and ip in _CGNAT_SHARED_SPACE: | |
| return True | |
| return ip.is_loopback or ip.is_private or ip.is_link_local |
🤖 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/security/url_guard.py` around lines 148 - 154, Update the URL guard
logic around allow_private so link-local addresses remain blocked regardless of
that flag. Check ip.is_link_local before the allow_private early return,
preserving existing blocking for unspecified, multicast, reserved, and
link-local addresses while still permitting legitimate loopback, RFC1918, and
CGNAT addresses when allow_private is true.
Comment on lines
+69
to
+90
| async def add_manual_model(db: AsyncSession, provider_id: str, body: Any) -> ProviderModel: | ||
| """Insert a hand-entered catalog row. | ||
|
|
||
| ``source`` is always forced to ``"manual"`` regardless of what ``body`` | ||
| carries (the request schema, ``ProviderModelManualCreate``, doesn't even | ||
| expose a ``source`` field) — decision #3's manual/discovered boundary is | ||
| enforced structurally: this is the ONLY function that ever writes | ||
| ``source="manual"``, :func:`sync_models` is the only one that writes | ||
| ``source="discovered"``. | ||
| """ | ||
| row = ProviderModel( | ||
| provider_id=provider_id, | ||
| model_id=body.model_id, | ||
| model_type=body.model_type, | ||
| capabilities=body.capabilities, | ||
| source="manual", | ||
| enabled=body.enabled, | ||
| ) | ||
| db.add(row) | ||
| await db.commit() | ||
| await db.refresh(row) | ||
| return row |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Locate relevant files and inspect targeted ranges
git ls-files | rg 'backend/(services/provider_model_service\.py|api/v1/providers\.py|.*exception.*|.*error.*|.*models?\.py|.*schema.*|.*model.*\.py)$'
printf '\n--- provider_model_service.py ---\n'
wc -l backend/services/provider_model_service.py
sed -n '1,220p' backend/services/provider_model_service.py | cat -n
printf '\n--- backend/api/v1/providers.py ---\n'
wc -l backend/api/v1/providers.py
sed -n '1,260p' backend/api/v1/providers.py | cat -n
printf '\n--- search for ModelAlreadyExistsError / IntegrityError / unique constraint handling ---\n'
rg -n "ModelAlreadyExistsError|IntegrityError|UniqueConstraint|unique constraint|HTTPException\\(|409|duplicate" backendRepository: 2233admin/opencli-admin
Length of output: 50379
🏁 Script executed:
set -euo pipefail
# Inspect the data model for uniqueness constraints on provider_id/model_id/source
rg -n "ProviderModel|__table_args__|UniqueConstraint|unique=True|Index\\(" backend | sed -n '1,220p'
printf '\n--- likely model file(s) ---\n'
fd -a 'provider_model' backend
fd -a 'model' backend | head -n 50Repository: 2233admin/opencli-admin
Length of output: 9135
🏁 Script executed:
set -euo pipefail
# Find any exception handlers or middleware that map DB integrity errors to HTTP responses
rg -n "exc\.IntegrityError|IntegrityError|HTTPException|handle.*Integrity|exception_handler|ModelAlreadyExistsError" backend | sed -n '1,240p'Repository: 2233admin/opencli-admin
Length of output: 16885
🏁 Script executed:
set -euo pipefail
echo "PAUSE"Repository: 2233admin/opencli-admin
Length of output: 167
🏁 Script executed:
set -euo pipefail
printf '\n--- backend/models/provider_model.py ---\n'
wc -l backend/models/provider_model.py
sed -n '1,220p' backend/models/provider_model.py | cat -n
printf '\n--- backend/main.py exception handler section ---\n'
wc -l backend/main.py
sed -n '200,240p' backend/main.py | cat -nRepository: 2233admin/opencli-admin
Length of output: 4367
🏁 Script executed:
set -euo pipefail
printf '\n--- backend/api/v1/browsers.py duplicate handling example ---\n'
sed -n '1,80p' backend/api/v1/browsers.py | cat -n
printf '\n--- backend/api/v1/skill_record.py duplicate handling example ---\n'
sed -n '160,210p' backend/api/v1/skill_record.py | cat -nRepository: 2233admin/opencli-admin
Length of output: 5705
Handle duplicate manual models as 409
POST /providers/{provider_id}/models can still hit uq_provider_models_provider_model on (provider_id, model_id), so a duplicate manual add falls through as a 500. Catch the conflict here (or pre-check) and return 409 instead.
🤖 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/services/provider_model_service.py` around lines 69 - 90, Update
add_manual_model to handle IntegrityError raised by the unique
provider_id/model_id constraint during commit. Roll back the session, then raise
the API’s conflict/HTTP 409 exception with a clear duplicate-model message;
preserve other database errors instead of converting them.
|
|
||
| ## 坐标 | ||
| - repo: `D:\projects\opencli-admin` 分支: `refactor/thin-channel-thick-runner` | ||
| - 测试闸: `uv run pytest tests/unit --no-cov -q`（须全绿，当前基线 347） |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the baseline test count.
The document lists a current baseline of 347 but later reports completion from 325→379. Explain the differing baselines and commands so the “at least previous baseline” DoD can be verified.
Also applies to: 34-34
🤖 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 `@GOAL.md` at line 17, The test baseline figures and commands in GOAL.md are
inconsistent: reconcile the stated 347 baseline with the reported 325→379
progression. Update the relevant test-count descriptions to explain which
commands and test scopes produce each count, and clearly define the baseline
used for the “at least previous baseline” DoD.
Comment on lines
+3
to
+4
| > 用法：开一个**新 session**（本窗已过 smart zone），先读这份文件 + 读 memory | ||
| > `opencli-admin-channel-runner-refactor`，然后从 **PR2** 接着干。一口气做完一刀再 commit。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Archive or regenerate this handoff before using it.
It tells the next session to start at PR2 with a 325-test baseline, while the later goal documents record PR2–PR13 complete and 616/675 passing tests. AuthManager is also already marked complete elsewhere. A new agent following this file can redo completed work or operate from an obsolete branch state.
Also applies to: 18-22, 26-34, 70-71
🤖 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 `@HANDOFF-strangler-fig.md` around lines 3 - 4, HANDOFF-strangler-fig.md
contains obsolete instructions to resume at PR2 with an outdated test baseline,
despite later records marking PR2–PR13 and AuthManager complete. Archive or
regenerate the handoff and update all referenced sections to reflect the current
completed work, test status, and next actionable task; remove instructions that
would repeat completed work.
Comment on lines
+75
to
+80
| ## 5. 工作纪律（用户定，硬约束） | ||
|
|
||
| - 回复中文、代码/路径/commit 英文；caveman 简洁。 | ||
| - **接到明确方向就端到端做完**（自己 build/test/真验证再交），中途不一步一问、不开菜单挑下一步；只真分叉才问。 | ||
| - **只在用户说 "commit" 时提交**；stage 时显式列路径，**绝不** stage `chat.py` / `PR-DESCRIPTION.md`。 | ||
| - 不向上游 PR，自己 fork 开发。「PR1/PR2」只是每刀的叫法 = 本地 commit。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Unify the commit-authorization policy.
This file says to commit only when the user explicitly says “commit,” while GOAL-2 and GOAL-3 authorize auto-commits for each green PR. Define precedence and scope explicitly to prevent unauthorized commits or skipped required commits.
🤖 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 `@HANDOFF-strangler-fig.md` around lines 75 - 80, 统一 HANDOFF-strangler-fig.md
中的提交授权规则：明确“仅用户明确说 commit 才提交”与 GOAL-2/GOAL-3
自动提交要求的优先级和适用范围，规定哪些场景允许或必须自动提交，并说明冲突时应遵循的规则，确保既不会未经授权提交，也不会遗漏必要提交。
Comment on lines
+105
to
+110
| class _Row: | ||
| id = "md-1" | ||
| role = "chat" | ||
| candidates = [{"provider_id": "p1", "model_id": "m1"}] | ||
| created_at = datetime.now(timezone.utc) | ||
| updated_at = datetime.now(timezone.utc) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid the mutable class attribute in the schema stub.
Line 108 defines candidates as a shared mutable class attribute. Use instance attributes (or SimpleNamespace) so Ruff RUF012 passes and the fixture cannot leak state between instances.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 108-108: Mutable default value for class attribute
(RUF012)
🤖 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 `@tests/unit/test_model_default.py` around lines 105 - 110, Replace the mutable
class-level candidates attribute in the _Row stub with an instance attribute,
such as by adding an initializer or using SimpleNamespace, while preserving the
existing id, role, and timestamp values so each fixture instance owns its
candidates list and satisfies Ruff RUF012.
Source: Linters/SAST tools
Comment on lines
+217
to
+226
| class _Row: | ||
| id = "pm-1" | ||
| provider_id = "p-1" | ||
| model_id = "gpt-4o" | ||
| model_type = "llm" | ||
| capabilities = {"tools": True} | ||
| source = "manual" | ||
| enabled = True | ||
| created_at = datetime.now(timezone.utc) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make capabilities instance-scoped in the test stub.
Line 222 defines a mutable class attribute, triggering Ruff RUF012. Initialize the stub fields on an instance instead.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 222-222: Mutable default value for class attribute
(RUF012)
🤖 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 `@tests/unit/test_provider_model.py` around lines 217 - 226, Update the `_Row`
test stub so its fields, especially mutable `capabilities`, are initialized in
an `__init__` method as instance attributes rather than class attributes,
preserving the existing default values and avoiding Ruff RUF012.
Source: Linters/SAST tools
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
1 participant
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.
GOAL-6 provider管理: adapter工厂+failover+catalog/CRUD+resolver+前端providers页面。8 commits, PR-A~PR-G。