feat: local model management for Ollama and LM Studio#1037
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src,tests}/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-04-01T15:36:39.993ZApplied to files:
📚 Learning: 2026-04-02T11:49:21.202ZApplied to files:
📚 Learning: 2026-04-03T17:10:42.444ZApplied to files:
🔇 Additional comments (7)
WalkthroughAdds local model management end-to-end: a streaming SSE pull endpoint ( Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request introduces local model management capabilities, primarily targeting Ollama. It adds backend support for pulling models with SSE-based progress streaming, deleting models, and configuring per-model launch parameters (such as context window and GPU layers). The frontend is updated with new UI components including a model pull dialog and a configuration drawer. Additionally, the PR includes a bug fix to ensure local presets skip static LiteLLM model discovery in favor of live discovery. A critical syntax error was identified in the exception handling logic of the provider management service.
| await manager.delete_model(model_id) | ||
| try: | ||
| await self.discover_models_for_provider(name) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
The use of a comma in the except clause is invalid syntax in Python. It should be written as except (MemoryError, RecursionError):.
| except MemoryError, RecursionError: | |
| except (MemoryError, RecursionError): |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end local model management for local providers (starting with Ollama), fixes model count mismatches by preferring live discovery for no-auth presets, and exposes new capability-gated UI controls in the dashboard.
Changes:
- Backend: introduce preset capability flags, per-model
LocalModelParams, local model manager protocol + Ollama implementation, and provider management service methods for pull/delete/config. - API: add SSE-based model pull endpoint plus delete/config endpoints; include
preset_nameand capability flags in provider responses. - Web: add model pull dialog, delete confirmation, config drawer, refresh UI, and update store/API client plumbing and tests/stories.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/stores/providers.ts | Zustand store actions/state for pull/delete/config + UI refresh integration |
| web/src/api/endpoints/providers.ts | Adds POST-based SSE pull via fetch + delete/config endpoints |
| web/src/api/types.ts | Adds LocalModelParams, PullProgressEvent, provider capability flags, preset_name |
| web/src/pages/ProviderDetailPage.tsx | Wires refresh/pull/config/delete dialogs into provider detail page |
| web/src/pages/providers/ProviderDetailHeader.tsx | Adds Refresh + Pull Model buttons (capability-gated) |
| web/src/pages/providers/ProviderModelList.tsx | Adds per-model action buttons (configure/delete) when supported |
| web/src/pages/providers/ModelPullDialog.tsx | New pull dialog with progress gauge + cancel handling |
| web/src/pages/providers/ModelConfigDrawer.tsx | New drawer to edit per-model LocalModelParams |
| web/src/pages/**.stories.tsx | Updates/adds Storybook coverage for new UI components/fields |
| web/src/tests/** | Updates tests for new provider fields + local_params |
| src/synthorg/providers/presets.py | Adds capability flags to presets (Ollama enabled) |
| src/synthorg/config/schema.py | Adds LocalModelParams and local_params on ProviderModelConfig; adds preset_name on ProviderConfig |
| src/synthorg/api/dto_providers.py | Adds DTO fields + request payloads for pull/config; resolves capability flags in responses |
| src/synthorg/api/controllers/providers.py | Adds SSE pull + delete + config endpoints |
| src/synthorg/providers/management/local_models.py | Implements LocalModelManager and OllamaModelManager (pull/delete) |
| src/synthorg/providers/management/service.py | Bug fix for local presets to skip LiteLLM DB; adds service methods for pull/delete/config |
| src/synthorg/providers/management/_helpers.py | Persists preset_name into ProviderConfig on creation |
| src/synthorg/observability/events/provider.py | Adds structured event names for local model management |
| tests/unit/providers/** | Adds unit coverage for capabilities, local model manager, and service behavior |
| docs/design/operations.md | Documents capability flags + new local model management endpoints/features |
| CLAUDE.md | Updates package structure note to include local model management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pullModel: async (name, modelName) => { | ||
| _pullAbortController = new AbortController() | ||
| set({ pullingModel: true, pullProgress: null }) | ||
| try { | ||
| await apiPullModel( | ||
| name, | ||
| modelName, | ||
| (event) => set({ pullProgress: event }), | ||
| _pullAbortController.signal, | ||
| ) | ||
| useToastStore.getState().add({ | ||
| variant: 'success', | ||
| title: `Model "${modelName}" pulled successfully`, | ||
| }) | ||
| const detail = get().selectedProvider | ||
| if (detail?.name === name) { | ||
| await get().fetchProviderDetail(name) | ||
| } | ||
| return true | ||
| } catch (err) { |
There was a problem hiding this comment.
pullModel() shows a success toast and returns true whenever apiPullModel() completes, even if the SSE stream reported an error (e.g. backend emits an error event or a progress payload with an error field). Because apiPullModel() never rejects on streamed errors, users can see “pulled successfully” for failed pulls. Track the last streamed event and treat event.error / event.done && error (or SSE event: error) as a failure (reject or return false) before emitting the success toast.
|
|
||
| while (true) { | ||
| const { done, value } = await reader.read() | ||
| if (done) break | ||
| buffer += decoder.decode(value, { stream: true }) | ||
|
|
||
| const lines = buffer.split('\n') | ||
| buffer = lines.pop() ?? '' | ||
|
|
||
| for (const line of lines) { | ||
| if (line.startsWith('data: ')) { | ||
| try { | ||
| const event = JSON.parse(line.slice(6)) as PullProgressEvent | ||
| onProgress(event) | ||
| } catch { | ||
| // Skip malformed JSON | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Process any remaining data in the buffer after the stream ends | ||
| if (buffer.startsWith('data: ')) { | ||
| try { | ||
| const event = JSON.parse(buffer.slice(6)) as PullProgressEvent | ||
| onProgress(event) | ||
| } catch { | ||
| // Skip malformed JSON | ||
| } | ||
| } |
There was a problem hiding this comment.
The SSE parser only processes lines starting with "data: " and ignores event: lines, so it can’t distinguish progress vs error events and can miss the last message if the final chunk ends with event: then data: without a trailing newline. This also means an error event payload (currently {error: ...}) will be treated as a PullProgressEvent without required fields. Consider implementing a minimal SSE parser (track current event + accumulate data: lines until blank line), and reject/throw when event === "error" or when parsed JSON contains an error flag.
| while (true) { | |
| const { done, value } = await reader.read() | |
| if (done) break | |
| buffer += decoder.decode(value, { stream: true }) | |
| const lines = buffer.split('\n') | |
| buffer = lines.pop() ?? '' | |
| for (const line of lines) { | |
| if (line.startsWith('data: ')) { | |
| try { | |
| const event = JSON.parse(line.slice(6)) as PullProgressEvent | |
| onProgress(event) | |
| } catch { | |
| // Skip malformed JSON | |
| } | |
| } | |
| } | |
| } | |
| // Process any remaining data in the buffer after the stream ends | |
| if (buffer.startsWith('data: ')) { | |
| try { | |
| const event = JSON.parse(buffer.slice(6)) as PullProgressEvent | |
| onProgress(event) | |
| } catch { | |
| // Skip malformed JSON | |
| } | |
| } | |
| let currentEvent = 'message' | |
| let currentDataLines: string[] = [] | |
| const dispatchEvent = () => { | |
| if (currentDataLines.length === 0) { | |
| currentEvent = 'message' | |
| return | |
| } | |
| const data = currentDataLines.join('\n') | |
| currentDataLines = [] | |
| try { | |
| const parsed = JSON.parse(data) as PullProgressEvent | { error?: string } | |
| if ( | |
| currentEvent === 'error' || | |
| (parsed !== null && | |
| typeof parsed === 'object' && | |
| 'error' in parsed && | |
| typeof parsed.error === 'string' && | |
| parsed.error.length > 0) | |
| ) { | |
| const message = | |
| parsed !== null && | |
| typeof parsed === 'object' && | |
| 'error' in parsed && | |
| typeof parsed.error === 'string' && | |
| parsed.error.length > 0 | |
| ? parsed.error | |
| : 'Pull failed' | |
| currentEvent = 'message' | |
| throw new Error(message) | |
| } | |
| onProgress(parsed as PullProgressEvent) | |
| } catch (error) { | |
| currentEvent = 'message' | |
| if (error instanceof Error) { | |
| throw error | |
| } | |
| // Skip malformed JSON | |
| return | |
| } | |
| currentEvent = 'message' | |
| } | |
| while (true) { | |
| const { done, value } = await reader.read() | |
| if (done) break | |
| buffer += decoder.decode(value, { stream: true }) | |
| const lines = buffer.split(/\r?\n/) | |
| buffer = lines.pop() ?? '' | |
| for (const line of lines) { | |
| if (line === '') { | |
| dispatchEvent() | |
| continue | |
| } | |
| if (line.startsWith('event:')) { | |
| currentEvent = line.slice(6).trim() || 'message' | |
| continue | |
| } | |
| if (line.startsWith('data:')) { | |
| const value = line.startsWith('data: ') ? line.slice(6) : line.slice(5) | |
| currentDataLines.push(value) | |
| } | |
| } | |
| } | |
| buffer += decoder.decode() | |
| const trailingLines = buffer.split(/\r?\n/) | |
| for (const line of trailingLines) { | |
| if (line === '') { | |
| dispatchEvent() | |
| continue | |
| } | |
| if (line.startsWith('event:')) { | |
| currentEvent = line.slice(6).trim() || 'message' | |
| continue | |
| } | |
| if (line.startsWith('data:')) { | |
| const value = line.startsWith('data: ') ? line.slice(6) : line.slice(5) | |
| currentDataLines.push(value) | |
| } | |
| } | |
| if (currentDataLines.length > 0) { | |
| dispatchEvent() | |
| } |
| async def _event_stream() -> AsyncIterator[dict[str, str]]: | ||
| try: | ||
| async for event in svc.pull_model(name, data.model_name): | ||
| yield { | ||
| "event": "complete" if event.done else "progress", | ||
| "data": _json.dumps( | ||
| event.model_dump(exclude_none=True), | ||
| ), | ||
| } | ||
| except ProviderNotFoundError: | ||
| yield { | ||
| "event": "error", | ||
| "data": _json.dumps( | ||
| {"error": f"Provider {name!r} not found"}, | ||
| ), | ||
| } | ||
| except ProviderValidationError as exc: | ||
| yield { | ||
| "event": "error", | ||
| "data": _json.dumps({"error": str(exc)}), | ||
| } |
There was a problem hiding this comment.
The SSE error path emits data shaped as { "error": "..." }, while the success/progress path emits serialized PullProgressEvent. This makes the stream payload a union type and breaks clients that expect consistent PullProgressEvent fields (e.g. status, done). Consider always emitting a PullProgressEvent-shaped payload on errors too (e.g. status="error", error=..., done=true, and include nulls) so the frontend can handle errors uniformly.
| async def update_model_config( | ||
| self, | ||
| name: str, | ||
| model_id: str, | ||
| local_params: LocalModelParams, | ||
| ) -> ProviderConfig: | ||
| """Update per-model launch parameters for a local provider. | ||
|
|
||
| Args: | ||
| name: Provider name. | ||
| model_id: Model identifier. | ||
| local_params: New launch parameters. | ||
|
|
||
| Returns: | ||
| Updated provider configuration. | ||
|
|
||
| Raises: | ||
| ProviderNotFoundError: If the provider does not exist. | ||
| ProviderValidationError: If the model is not found. | ||
| """ | ||
| async with self._lock: |
There was a problem hiding this comment.
update_model_config() updates local_params for any provider/model without checking the preset capability flag (supports_model_config). This bypasses the capability gating used for pull/delete (and the UI gating), allowing config updates on providers where it should be unsupported. Consider reusing _resolve_local_manager(..., capability="config") or at least validating the preset capability before persisting the update.
web/src/stores/providers.ts
Outdated
| pullModel: async (name, modelName) => { | ||
| _pullAbortController = new AbortController() | ||
| set({ pullingModel: true, pullProgress: null }) | ||
| try { | ||
| await apiPullModel( | ||
| name, | ||
| modelName, | ||
| (event) => set({ pullProgress: event }), | ||
| _pullAbortController.signal, |
There was a problem hiding this comment.
pullModel() overwrites the module-level _pullAbortController without aborting any in-flight pull first. If the user triggers multiple pulls (double-click, rapid reopen), the earlier fetch will keep running but becomes un-cancellable and can continue updating state. Consider aborting any existing controller before creating a new one, or guarding against concurrent pulls.
| const handleSave = async () => { | ||
| setSaving(true) | ||
| const parseIntSafe = (val: string): number | null => { | ||
| if (!val) return null | ||
| const parsed = parseInt(val, 10) | ||
| return Number.isNaN(parsed) ? null : parsed | ||
| } | ||
| const parseFloatSafe = (val: string): number | null => { | ||
| if (!val) return null | ||
| const parsed = parseFloat(val) | ||
| return Number.isNaN(parsed) ? null : parsed | ||
| } | ||
| const newParams: LocalModelParams = { | ||
| num_ctx: parseIntSafe(numCtx), | ||
| num_gpu_layers: parseIntSafe(numGpuLayers), | ||
| num_threads: parseIntSafe(numThreads), | ||
| num_batch: parseIntSafe(numBatch), | ||
| repeat_penalty: parseFloatSafe(repeatPenalty), | ||
| } |
There was a problem hiding this comment.
parseInt() / parseFloat() will accept partially-numeric inputs (e.g. "12abc" becomes 12), which can silently persist unintended values. If the intent is “numeric or empty”, consider validating the whole string (e.g. regex or Number(val) + Number.isFinite) and treating any non-strict numeric input as null/invalid before calling the API.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1037 +/- ##
==========================================
- Coverage 91.48% 91.28% -0.21%
==========================================
Files 687 688 +1
Lines 38392 38680 +288
Branches 3821 3851 +30
==========================================
+ Hits 35124 35310 +186
- Misses 2608 2701 +93
- Partials 660 669 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/api/dto_providers.py (1)
133-166:⚠️ Potential issue | 🔴 CriticalRemove
preset_namefromCreateProviderRequestor filter it out during conversion.The generic
create_provider()endpoint acceptspreset_namefrom the client viaCreateProviderRequest, and the conversion helper (_request_to_config()in_helpers.py:57) copies it directly intoProviderConfigwithout validation. This allows a client to manually setpreset_nameto an arbitrary preset, andto_provider_response()will then derivesupports_model_pull,supports_model_delete, andsupports_model_configfrom that preset. Combined with a client-suppliedbase_url, this grants local model management capabilities to any provider that was not legitimately created viacreate_from_preset(), violating the boundary enforcement required by the coding guidelines ("Validate at system boundaries").Only
create_from_preset()should setpreset_nameon the resulting config (line 375). Remove the field fromCreateProviderRequestor, if backward compatibility is required, filter it out toNoneduring conversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/dto_providers.py` around lines 133 - 166, Remove client-controlled preset injection by eliminating preset_name from the CreateProviderRequest model or by ensuring _request_to_config() ignores it: update CreateProviderRequest to no longer declare preset_name (or accept it but never map it), and modify _request_to_config() to always set ProviderConfig.preset_name = None so only create_from_preset() can assign a preset; also verify to_provider_response() continues deriving capabilities from ProviderConfig.preset_name only when set by create_from_preset().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/operations.md`:
- Line 130: Update the local model management bullet to use the full provider
API namespace: replace the relative endpoints `POST /{name}/models/pull`,
`DELETE /{name}/models/{model_id}`, and `PUT /{name}/models/{model_id}/config`
with fully qualified paths under the provider API (e.g. `POST
/api/v1/providers/{name}/models/pull`, `DELETE
/api/v1/providers/{name}/models/{model_id}`, `PUT
/api/v1/providers/{name}/models/{model_id}/config`) and keep the referenced
`LocalModelParams` fields (`num_ctx`, `num_gpu_layers`, `num_threads`,
`num_batch`, `repeat_penalty`) unchanged; update the Ollama/LM Studio note
accordingly.
In `@src/synthorg/api/controllers/providers.py`:
- Around line 663-669: The log call in providers.py uses the inline string
"api.sse.pull_model_failed"; replace this with the appropriate event constant
from synthorg.observability.events (import the domain constant, e.g. from
synthorg.observability.events.api import SSE_PULL_MODEL_FAILED) and pass that
constant to logger.exception instead of the inline string while keeping the same
context (provider=name, model=data.model_name, error=str(exc),
error_type=type(exc).__qualname__). Ensure the import is added at top of the
module and update the logger.exception invocation to use the imported constant.
In `@src/synthorg/providers/management/local_models.py`:
- Around line 275-298: The delete logic currently raises ValueError for any
response.status_code >= _HTTP_CLIENT_ERROR which gets mapped to a 404 upstream;
change it so ValueError is only raised when response.status_code ==
_HTTP_NOT_FOUND (keep the PROVIDER_MODEL_DELETE_FAILED log as-is), and for other
HTTP errors (response.status_code >= _HTTP_CLIENT_ERROR) log
PROVIDER_MODEL_DELETE_FAILED and raise a different exception type (e.g.,
RuntimeError or a custom UpstreamError) with the HTTP status and message; also
surface transport/timeout exceptions from client.request (wrap the await
client.request call to catch transport errors, log PROVIDER_MODEL_DELETE_FAILED
with the error, and re-raise as the same non-ValueError exception).
In `@src/synthorg/providers/management/service.py`:
- Around line 686-748: The update_model_config method currently persists
local_params without verifying the provider preset's capability; inside the
async with self._lock block (in update_model_config) check the provider config's
preset supports_model_config flag (e.g., config.preset.supports_model_config)
after loading config and before modifying models, and if it's False log using
PROVIDER_VALIDATION_FAILED (include provider/name and a message) and raise
ProviderValidationError to prevent persisting launch parameters for presets that
don't allow model config changes.
In `@tests/unit/providers/management/test_local_models.py`:
- Around line 28-34: The test test_defaults assumes
PullProgressEvent.progress_percent is None but the actual default is 0; update
the test_defaults assertions to match the implementation by expecting
progress_percent == 0 (keep other assertions unchanged), i.e., change the
assertion in test_defaults for PullProgressEvent.progress_percent to assert it
equals 0 so the test aligns with the PullProgressEvent class default.
In `@web/src/api/endpoints/providers.ts`:
- Around line 160-179: The JSON parse failures in the SSE handling loop and
final buffer (when parsing into PullProgressEvent) are being swallowed; update
the catch blocks in the pullModel() stream handling to signal terminal failure
by either (a) calling onProgress with a synthetic PullProgressEvent representing
an error (include an error message and a terminal status) or (b) rethrowing the
parse error so the caller can fail; ensure you reference the same
PullProgressEvent shape and use onProgress(event) so the store receives a
terminal error instead of silently treating the stream as successful.
In `@web/src/pages/providers/ModelConfigDrawer.tsx`:
- Around line 34-52: The parsing helpers inside handleSave (parseIntSafe and
parseFloatSafe) currently allow partial parses like "12abc" → 12; change them to
validate that the entire string is a valid number and otherwise return null or
mark the field invalid: use full-string checks (e.g., regex or re-parsing and
comparing string representations) for numCtx, numGpuLayers, numThreads, numBatch
and repeatPenalty before constructing LocalModelParams, and when a value is
malformed set a corresponding error state used by the InputField error prop so
the UI shows validation feedback and prevent saving until corrected.
In `@web/src/pages/providers/ModelPullDialog.stories.tsx`:
- Around line 18-21: Replace the hardcoded wrapper height class "min-h-[400px]"
around the Story render with a design-system friendly utility: use a standard
Tailwind spacing like "min-h-96" or the tokenized density variable (e.g.,
"min-h-[var(--p-card)]" or whatever project token mapping provides) so the story
uses approved spacing rules; update the div wrapping Story in
ModelPullDialog.stories (the div containing <Story />) to use the chosen class
instead of "min-h-[400px]".
In `@web/src/pages/providers/ModelPullDialog.tsx`:
- Around line 49-116: Replace the hand-rolled overlay in ModelPullDialog with
the app's accessible modal primitive (the same Dialog/Modal component used by
other UI modals) instead of a raw div so focus is trapped and restored
automatically; wire the modal's onClose to the existing handleCancel/onClose
handlers, set the modal's initialFocus to the model name InputField (add a ref
to InputField) so keyboard users start on the input, and ensure the primitive's
aria props are used rather than manual role/aria-modal/aria-label on the
wrapper; keep existing handlers (handlePull, handleCancel, onClose) and state
(pullingModel, pullProgress) but remove the outer div overlay and rely on the
imported Dialog/Modal's built-in focus management and backdrop behavior.
In `@web/src/pages/providers/ProviderModelList.tsx`:
- Around line 72-92: The two raw icon <button> elements (the Configure and
Delete actions rendered when supportsConfig and supportsDelete are true) must be
replaced with the shared Button primitive (use the project Button component) so
row actions inherit focus, disabled and variant styling; update the JSX in
ProviderModelList to render <Button> with the same onClick handlers
(onConfigure?.(model) and onDelete?.(model.id)), preserve title and aria-label
attributes, render the Settings2 and Trash2 icons as the Button children, remove
the ad-hoc className styling used on the raw buttons, and add the appropriate
Button props for an icon/ghost variant and size (so keyboard focus and visual
variants match other UI buttons); also add the Button import at the top and
remove the unused raw button markup.
In `@web/src/stores/providers.ts`:
- Around line 398-405: The module-level _pullAbortController is being
overwritten on each apiPullModel call so the finally block can clear the active
controller for a newer request; fix by creating a local controller instance
(e.g., const controller = new AbortController()), assign it to the module-level
_pullAbortController, pass controller.signal to apiPullModel, and in the finally
only clear/reset _pullAbortController and set pullingModel:false if
_pullAbortController === controller (so cleanup is tied to the specific
controller that started the request).
- Around line 401-415: The success toast is shown unconditionally after
apiPullModel resolves, but apiPullModel streams terminal payloads that may
include an in-band error; modify the caller around apiPullModel (the block that
updates pullProgress and then calls useToastStore.getState().add) to track the
last SSE event received (the value fed into set({ pullProgress: event })) and
only show the success toast and call get().fetchProviderDetail when that final
event has done === true and no error property; if the final event contains an
error, show an error toast instead and return false; use the existing symbols
apiPullModel, set({ pullProgress: ... }), _pullAbortController.signal,
get().selectedProvider and get().fetchProviderDetail to locate and implement
this conditional behavior.
---
Outside diff comments:
In `@src/synthorg/api/dto_providers.py`:
- Around line 133-166: Remove client-controlled preset injection by eliminating
preset_name from the CreateProviderRequest model or by ensuring
_request_to_config() ignores it: update CreateProviderRequest to no longer
declare preset_name (or accept it but never map it), and modify
_request_to_config() to always set ProviderConfig.preset_name = None so only
create_from_preset() can assign a preset; also verify to_provider_response()
continues deriving capabilities from ProviderConfig.preset_name only when set by
create_from_preset().
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6254bc19-45c9-49b5-bff5-98a057a6c8f7
📒 Files selected for processing (32)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/controllers/providers.pysrc/synthorg/api/dto_providers.pysrc/synthorg/config/schema.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/management/_helpers.pysrc/synthorg/providers/management/local_models.pysrc/synthorg/providers/management/service.pysrc/synthorg/providers/presets.pytests/unit/providers/management/test_local_models.pytests/unit/providers/management/test_service_local_models.pytests/unit/providers/test_presets.pyweb/src/__tests__/pages/ProviderDetailPage.test.tsxweb/src/__tests__/pages/ProvidersPage.test.tsxweb/src/__tests__/stores/setup-wizard.test.tsweb/src/__tests__/utils/providers.test.tsweb/src/__tests__/utils/setup-validation.property.test.tsweb/src/__tests__/utils/setup-validation.test.tsweb/src/api/endpoints/providers.tsweb/src/api/types.tsweb/src/pages/ProviderDetailPage.tsxweb/src/pages/providers/ModelConfigDrawer.stories.tsxweb/src/pages/providers/ModelConfigDrawer.tsxweb/src/pages/providers/ModelPullDialog.stories.tsxweb/src/pages/providers/ModelPullDialog.tsxweb/src/pages/providers/PresetPicker.stories.tsxweb/src/pages/providers/ProviderCard.stories.tsxweb/src/pages/providers/ProviderDetailHeader.tsxweb/src/pages/providers/ProviderModelList.stories.tsxweb/src/pages/providers/ProviderModelList.tsxweb/src/stores/providers.ts
| return ( | ||
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-background/80 backdrop-blur-sm" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label="Pull model" | ||
| > | ||
| <div className="w-full max-w-md rounded-lg bg-card p-card shadow-lg"> | ||
| <div className="mb-4 flex items-center justify-between"> | ||
| <h2 className="text-lg font-semibold text-foreground">Pull Model</h2> | ||
| <button | ||
| type="button" | ||
| onClick={handleCancel} | ||
| aria-label="Close" | ||
| className="rounded p-1 text-text-muted hover:bg-bg-surface hover:text-foreground transition-colors" | ||
| > | ||
| <X className="size-4" /> | ||
| </button> | ||
| </div> | ||
|
|
||
| {!pullingModel ? ( | ||
| <div className="flex flex-col gap-4"> | ||
| <InputField | ||
| label="Model name" | ||
| value={modelName} | ||
| onValueChange={setModelName} | ||
| placeholder="e.g. llama3.2:1b" | ||
| hint="Enter the model name and optional tag" | ||
| /> | ||
| <div className="flex justify-end gap-2"> | ||
| <Button variant="outline" size="sm" onClick={onClose}> | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| onClick={handlePull} | ||
| disabled={!modelName.trim()} | ||
| > | ||
| <Download className="size-3.5 mr-1.5" /> | ||
| Pull | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div className="flex flex-col gap-4"> | ||
| <div className="flex items-center justify-center py-4"> | ||
| <ProgressGauge | ||
| value={progressPercent} | ||
| variant="linear" | ||
| /> | ||
| </div> | ||
| <p className="text-center text-sm text-text-secondary truncate"> | ||
| {statusText} | ||
| </p> | ||
| {pullProgress?.error && ( | ||
| <p className="text-center text-sm text-danger"> | ||
| {pullProgress.error} | ||
| </p> | ||
| )} | ||
| <div className="flex justify-end"> | ||
| <Button variant="outline" size="sm" onClick={handleCancel}> | ||
| Cancel | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
This modal needs real focus management.
The hand-rolled overlay never traps focus, sets initial focus, or restores focus on close, so keyboard users can tab behind the dialog and lose context during a pull. Please build this on the same accessible modal primitive used elsewhere instead of a raw div overlay. As per coding guidelines, "ALWAYS reuse existing components from web/src/components/ui/ before creating new ones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/providers/ModelPullDialog.tsx` around lines 49 - 116, Replace
the hand-rolled overlay in ModelPullDialog with the app's accessible modal
primitive (the same Dialog/Modal component used by other UI modals) instead of a
raw div so focus is trapped and restored automatically; wire the modal's onClose
to the existing handleCancel/onClose handlers, set the modal's initialFocus to
the model name InputField (add a ref to InputField) so keyboard users start on
the input, and ensure the primitive's aria props are used rather than manual
role/aria-modal/aria-label on the wrapper; keep existing handlers (handlePull,
handleCancel, onClose) and state (pullingModel, pullProgress) but remove the
outer div overlay and rely on the imported Dialog/Modal's built-in focus
management and backdrop behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web/src/pages/providers/ModelConfigDrawer.tsx (1)
34-52:⚠️ Potential issue | 🟠 MajorDon't persist malformed numeric input as cleared config.
These helpers still treat bad input as something savable: typos like
4096xcollapse tonulland clear the existing setting, while whitespace-only input is parsed as a value instead of “blank”. Please validate trimmed input and block save with field errors instead of serializing malformed values intoLocalModelParams.Run this read-only check to confirm the current helper behavior. Expected result: whitespace becomes a numeric value, and malformed strings collapse to
nullrather than being rejected.#!/bin/bash set -euo pipefail sed -n '34,52p' web/src/pages/providers/ModelConfigDrawer.tsx node - <<'NODE' const parseIntStrict = (val) => { if (!val) return null const n = Number(val) return Number.isFinite(n) && Number.isInteger(n) ? n : null } const parseFloatStrict = (val) => { if (!val) return null const n = Number(val) return Number.isFinite(n) ? n : null } for (const sample of [' ', '4096x', '1.25x']) { console.log(JSON.stringify({ sample, parseIntStrict: parseIntStrict(sample), parseFloatStrict: parseFloatStrict(sample), })) } NODEBased on learnings, "Validate at system boundaries (user input, external APIs, config files)".
Also applies to: 60-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/providers/ModelConfigDrawer.tsx` around lines 34 - 52, The parseIntStrict and parseFloatStrict helpers accept whitespace-only input as valid and silently convert malformed strings to null which clears existing settings; fix by trimming input first and treating empty-after-trim as "blank" (do not change stored value) and treat any non-numeric or partially-numeric values as validation errors that prevent save. Specifically, update parseIntStrict and parseFloatStrict in ModelConfigDrawer.tsx to trim(val) and return a distinct sentinel or throw/return error state for invalid input (rather than null), then in handleSave validate all fields before constructing LocalModelParams (use the trimmed values, only include numeric properties when valid), set per-field error state for invalid inputs, and block the save when any field error exists so malformed input is not persisted.web/src/stores/providers.ts (1)
397-429:⚠️ Potential issue | 🟠 MajorTie pull cleanup to the controller that started the request.
If a second pull begins before the first one finishes unwinding, the first
finallyblock still nulls_pullAbortControllerand flipspullingModeltofalse. That leaves the newer request without a cancel handle and drops the UI back to idle mid-download.Suggested fix
pullModel: async (name, modelName) => { - _pullAbortController?.abort() - _pullAbortController = new AbortController() + const controller = new AbortController() + _pullAbortController?.abort() + _pullAbortController = controller set({ pullingModel: true, pullProgress: null }) try { await apiPullModel( name, modelName, (event) => set({ pullProgress: event }), - _pullAbortController.signal, + controller.signal, ) useToastStore.getState().add({ variant: 'success', title: `Model "${modelName}" pulled successfully`, }) @@ } finally { - _pullAbortController = null - set({ pullingModel: false }) + if (_pullAbortController === controller) { + _pullAbortController = null + set({ pullingModel: false }) + } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/providers.ts` around lines 397 - 429, The finally block is clearing the shared _pullAbortController and flipping pullingModel even when a newer pull started; fix pullModel by capturing the controller created for this invocation (e.g., const controller = _pullAbortController after new AbortController()) and use that local reference in the finally to only null _pullAbortController and set pullingModel to false if _pullAbortController === controller, so subsequent pulls retain their abort handle and UI state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 771-787: The current except block for ProviderValidationError in
controllers/providers.py treats any validation failure as HTTP 400; change it so
that when the validation error specifically indicates the target model is
missing you raise NotFoundError instead. In practice: inside the except
ProviderValidationError as exc handler, inspect exc (e.g., error
code/message/attribute that denotes "model not found" for the service) and if it
matches the missing-model condition, log API_RESOURCE_NOT_FOUND with
resource="model" and name=model_id and raise NotFoundError(f"Model {model_id!r}
not found") from exc; otherwise keep the existing API_VALIDATION_FAILED log and
raise ApiValidationError(str(exc)). Reference symbols: ProviderValidationError,
NotFoundError, ApiValidationError, API_RESOURCE_NOT_FOUND,
API_VALIDATION_FAILED, and the model_id variable.
In `@src/synthorg/providers/management/local_models.py`:
- Around line 193-225: The local_models.py loop defines an unused got_done
variable; remove got_done and its final conditional branch by relying on the
existing early return when event.done is true. In the async for over
response.aiter_lines(), keep JSON parsing and the call to
self._parse_pull_line(model_name) and yielding the event, then when the stream
ends log the same PROVIDER_MODEL_PULL_FAILED message and yield the
PullProgressEvent as currently done but without referencing or updating
got_done; remove the got_done declaration and any checks related to it to
eliminate the dead variable.
---
Duplicate comments:
In `@web/src/pages/providers/ModelConfigDrawer.tsx`:
- Around line 34-52: The parseIntStrict and parseFloatStrict helpers accept
whitespace-only input as valid and silently convert malformed strings to null
which clears existing settings; fix by trimming input first and treating
empty-after-trim as "blank" (do not change stored value) and treat any
non-numeric or partially-numeric values as validation errors that prevent save.
Specifically, update parseIntStrict and parseFloatStrict in
ModelConfigDrawer.tsx to trim(val) and return a distinct sentinel or
throw/return error state for invalid input (rather than null), then in
handleSave validate all fields before constructing LocalModelParams (use the
trimmed values, only include numeric properties when valid), set per-field error
state for invalid inputs, and block the save when any field error exists so
malformed input is not persisted.
In `@web/src/stores/providers.ts`:
- Around line 397-429: The finally block is clearing the shared
_pullAbortController and flipping pullingModel even when a newer pull started;
fix pullModel by capturing the controller created for this invocation (e.g.,
const controller = _pullAbortController after new AbortController()) and use
that local reference in the finally to only null _pullAbortController and set
pullingModel to false if _pullAbortController === controller, so subsequent
pulls retain their abort handle and UI state.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39a740b4-fd68-40f0-8c79-9d50660ae39b
📒 Files selected for processing (18)
docs/design/operations.mdsrc/synthorg/api/controllers/providers.pysrc/synthorg/api/dto_providers.pysrc/synthorg/observability/events/api.pysrc/synthorg/providers/management/local_models.pysrc/synthorg/providers/management/service.pysrc/synthorg/providers/presets.pytests/unit/providers/management/test_local_models.pytests/unit/providers/management/test_service_local_models.pyweb/src/api/endpoints/providers.tsweb/src/pages/ProviderDetailPage.tsxweb/src/pages/providers/ModelConfigDrawer.tsxweb/src/pages/providers/ModelPullDialog.tsxweb/src/pages/providers/PresetPicker.tsxweb/src/pages/providers/ProviderFormModal.tsxweb/src/pages/providers/ProviderModelList.stories.tsxweb/src/pages/providers/TestConnectionResult.tsxweb/src/stores/providers.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
web/src/stores/providers.ts (2)
439-443:⚠️ Potential issue | 🟠 MajorKeep
pullingModelcleanup tied to the active controller.Line 443 still runs even when this request has already been superseded by a newer pull. The older
finallycan therefore hide a still-running stream, and it makes it impossible to keep a terminal error state latched if the dialog wants to continue showing it.♻️ Scope the reset to the request that still owns `_pullAbortController`
} finally { if (_pullAbortController === controller) { _pullAbortController = null - } - set({ pullingModel: false }) + const finalEvent = get().pullProgress + if (!(finalEvent?.done && finalEvent.error)) { + set({ pullingModel: false }) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/providers.ts` around lines 439 - 443, The finally block unconditionally clears pullingModel which can hide a newer pull; change it so pullingModel is only reset when this finally belongs to the active request by checking that _pullAbortController === controller before calling set({ pullingModel: false }) (i.e., scope both _pullAbortController = null and set({ pullingModel: false }) behind the existing controller equality check for the _pullAbortController variable).
397-424:⚠️ Potential issue | 🔴 CriticalRequire a terminal event before reporting pull success.
apiPullModel()resolves when the SSE stream closes, solastEvent === nullordone === falsestill falls through to the success toast. This block is also what CI is failing on at Line 413 (TS2339), because TypeScript does not track the callback assignment intolastEventacross theawait.♻️ One way to fix the terminal-event check and the TS narrowing issue
- let lastEvent: PullProgressEvent | null = null + const streamState: { lastEvent: PullProgressEvent | null } = { lastEvent: null } try { await apiPullModel( name, modelName, (event) => { - lastEvent = event + streamState.lastEvent = event set({ pullProgress: event }) }, controller.signal, ) - if (lastEvent?.error) { + const finalEvent = streamState.lastEvent + if (!finalEvent || !finalEvent.done) { + useToastStore.getState().add({ + variant: 'error', + title: 'Model pull incomplete', + description: 'Pull stream ended before a terminal event was received.', + }) + return false + } + if (finalEvent.error) { useToastStore.getState().add({ variant: 'error', title: 'Model pull failed', - description: lastEvent.error, + description: finalEvent.error, }) return false }#!/bin/bash # Inspect the terminal-event handling around the failing line and the event shape it depends on. sed -n '397,424p' web/src/stores/providers.ts printf '\n--- PullProgressEvent ---\n' sed -n '700,707p' web/src/api/types.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/providers.ts` around lines 397 - 424, The pullModel flow reports success whenever apiPullModel resolves even if the SSE never emitted a terminal event; fix pullModel (and the post-await logic) to require a terminal event: after the await apiPullModel(...) check that lastEvent is non-null and that lastEvent.done === true and lastEvent.error is falsy before showing the success toast, otherwise show an error toast using lastEvent?.error or a generic message and return false; reference the pullModel function, the lastEvent variable (type PullProgressEvent | null), the PullProgressEvent.done and .error fields, and the apiPullModel callback to locate and implement this check so TypeScript narrowing is satisfied and CI TS2339 goes away.src/synthorg/providers/management/local_models.py (1)
97-106:⚠️ Potential issue | 🟡 MinorFix the
delete_model()exception contract in the protocol/docstrings.The implementation now uses
ValueErroronly for “model not found” andRuntimeErrorfor upstream/transport failures, but both docstrings still sayValueErrorcovers request failures too. That stale contract makes the nextLocalModelManagerimplementation likely to reintroduce the old 404 mis-mapping.Also applies to: 300-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/providers/management/local_models.py` around lines 97 - 106, The docstring for delete_model currently misstates the exception contract; update the Raises section for the delete_model(self, model_name: str) method to specify that ValueError is raised only when the model does not exist and that RuntimeError is raised for upstream/transport/delete request failures, and ensure any related wording in the method (and similar docstrings around lines 300-302) matches this contract so implementations (e.g., LocalModelManager) will not map 404s to the wrong exception type.src/synthorg/api/controllers/providers.py (1)
806-815:⚠️ Potential issue | 🟠 MajorReturn 404 when the updated model is missing.
This branch is a missing resource, not a validation failure. Returning
ApiValidationErrormakesPUT /models/{model_id}/configinconsistent with the delete endpoint and forces clients to treat stale model IDs as HTTP 400.🩹 Suggested fix
if model is None: - msg = f"Model {model_id!r} missing from updated config" - logger.error( - API_VALIDATION_FAILED, - resource="model", - name=model_id, - provider=name, - error=msg, - ) - raise ApiValidationError(msg) + msg = f"Model {model_id!r} not found" + logger.warning( + API_RESOURCE_NOT_FOUND, + resource="model", + name=model_id, + provider=name, + ) + raise NotFoundError(msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/providers.py` around lines 806 - 815, This branch represents a missing resource, not a validation error: replace the ApiValidationError raise and the API_VALIDATION_FAILED log with a 404-style error; update the logger call to use the not-found error code constant (e.g., API_NOT_FOUND) and raise the appropriate not-found exception (e.g., ApiNotFoundError or ResourceNotFoundError) instead of ApiValidationError when model is None, keeping the same context fields (resource="model", name=model_id, provider=name) so PUT /models/{model_id}/config returns HTTP 404 for stale/missing model IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 746-754: The except block in providers.py currently maps
RuntimeError (from local_models.py upstream/transport failures) to
ApiValidationError, causing 5xx/provider errors to be treated as client 400s;
change the handler so RuntimeError is not converted to ApiValidationError—either
re-raise the original RuntimeError or raise the controller-level provider/server
error (e.g., ApiProviderError or the existing 5xx error type used elsewhere)
instead of ApiValidationError; keep the logger.warning call
(API_VALIDATION_FAILED, resource="model", name=model_id, provider=name,
error=str(exc)) or change it to a provider-error log level if appropriate, but
ensure ApiValidationError is not used for RuntimeError so upstream failures
surface as 5xx.
- Around line 645-697: The SSE handler _event_stream currently yields error
payloads that omit nullable fields (progress_percent, total_bytes,
completed_bytes, error) because error dicts are hand-built with exclude_none
behaviour, breaking the PullProgressEvent contract; update the error branches
(ProviderNotFoundError, ProviderValidationError, generic Exception) to include
the same keys as the success/progress payloads — always emit progress_percent,
total_bytes, completed_bytes and error (set them to null when not applicable)
and keep done/status/error strings, or alternatively call event.model_dump
without exclude_none or construct a full dict matching the success shape; ensure
JSON serialization (_json.dumps) returns the same field set for progress and
error events so the frontend receives nulls instead of missing fields.
---
Duplicate comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 806-815: This branch represents a missing resource, not a
validation error: replace the ApiValidationError raise and the
API_VALIDATION_FAILED log with a 404-style error; update the logger call to use
the not-found error code constant (e.g., API_NOT_FOUND) and raise the
appropriate not-found exception (e.g., ApiNotFoundError or
ResourceNotFoundError) instead of ApiValidationError when model is None, keeping
the same context fields (resource="model", name=model_id, provider=name) so PUT
/models/{model_id}/config returns HTTP 404 for stale/missing model IDs.
In `@src/synthorg/providers/management/local_models.py`:
- Around line 97-106: The docstring for delete_model currently misstates the
exception contract; update the Raises section for the delete_model(self,
model_name: str) method to specify that ValueError is raised only when the model
does not exist and that RuntimeError is raised for upstream/transport/delete
request failures, and ensure any related wording in the method (and similar
docstrings around lines 300-302) matches this contract so implementations (e.g.,
LocalModelManager) will not map 404s to the wrong exception type.
In `@web/src/stores/providers.ts`:
- Around line 439-443: The finally block unconditionally clears pullingModel
which can hide a newer pull; change it so pullingModel is only reset when this
finally belongs to the active request by checking that _pullAbortController ===
controller before calling set({ pullingModel: false }) (i.e., scope both
_pullAbortController = null and set({ pullingModel: false }) behind the existing
controller equality check for the _pullAbortController variable).
- Around line 397-424: The pullModel flow reports success whenever apiPullModel
resolves even if the SSE never emitted a terminal event; fix pullModel (and the
post-await logic) to require a terminal event: after the await apiPullModel(...)
check that lastEvent is non-null and that lastEvent.done === true and
lastEvent.error is falsy before showing the success toast, otherwise show an
error toast using lastEvent?.error or a generic message and return false;
reference the pullModel function, the lastEvent variable (type PullProgressEvent
| null), the PullProgressEvent.done and .error fields, and the apiPullModel
callback to locate and implement this check so TypeScript narrowing is satisfied
and CI TS2339 goes away.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee6a2719-03d8-4dbc-b35d-5ecc41b689fb
📒 Files selected for processing (6)
docs/design/operations.mdsrc/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.pyweb/src/pages/providers/ModelPullDialog.stories.tsxweb/src/pages/providers/ProviderModelList.tsxweb/src/stores/providers.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
web/src/stores/providers.ts (1)
439-443:⚠️ Potential issue | 🟠 MajorGuard
pullingModelcleanup with the active controller check.Line 443 still runs when an older pull settles. If a second pull starts before the first request finishes aborting, the first
finallyflipspullingModeltofalsewhile the newer stream is still active.🔒 Scope cleanup to the request that started it
} finally { if (_pullAbortController === controller) { _pullAbortController = null + set({ pullingModel: false }) } - set({ pullingModel: false }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/providers.ts` around lines 439 - 443, The finally block currently always calls set({ pullingModel: false }) even if a newer pull replaced the original; change it to only clear pullingModel when the abort controller that started this request is still the active one. Concretely, in the finally of the pull function check _pullAbortController === controller and only then set({ pullingModel: false }) (i.e., move the set call inside the existing if that nulls _pullAbortController) so older settled requests don't flip pullingModel for a newer active pull.src/synthorg/api/controllers/providers.py (2)
650-652:⚠️ Potential issue | 🟠 MajorEmit a stable
PullProgressEventshape on every SSE frame.
PullProgressEventdefines nullableprogress_percent,total_bytes,completed_bytes, anderror, but Line 651 dropsNonevalues and the synthetic error payloads omit those keys entirely. That changes the contract from “nullable” to “sometimes missing,” so the web client getsundefinedinstead ofnull.♻️ Contract-preserving serialization
async for event in svc.pull_model(name, data.model_name): yield { "event": "complete" if event.done else "progress", - "data": _json.dumps( - event.model_dump(exclude_none=True), - ), + "data": _json.dumps(event.model_dump()), } except ProviderNotFoundError: yield { "event": "error", "data": _json.dumps( { "status": f"Provider {name!r} not found", + "progress_percent": None, + "total_bytes": None, + "completed_bytes": None, "error": f"Provider {name!r} not found", "done": True, } ), } except ProviderValidationError as exc: yield { "event": "error", "data": _json.dumps( { "status": str(exc), + "progress_percent": None, + "total_bytes": None, + "completed_bytes": None, "error": str(exc), "done": True, } ), } @@ yield { "event": "error", "data": _json.dumps( { "status": f"Internal error: {type(exc).__name__}", + "progress_percent": None, + "total_bytes": None, + "completed_bytes": None, "error": f"Internal error: {type(exc).__name__}", "done": True, } ), }Also applies to: 657-662, 669-673, 691-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/providers.py` around lines 650 - 652, The SSE payloads currently call event.model_dump(exclude_none=True) which strips keys with None and breaks the PullProgressEvent contract; change the serialization to preserve nullable keys (e.g., use event.model_dump() or build a dict that explicitly includes progress_percent, total_bytes, completed_bytes, and error with their None values) wherever you see "data": _json.dumps(event.model_dump(exclude_none=True)) (including the occurrences around the PullProgressEvent emission at the shown snippet and the similar blocks at the other noted ranges) so the emitted JSON contains the keys with null values instead of omitting them.
746-754:⚠️ Potential issue | 🟠 MajorSurface delete transport/upstream failures as 5xxs.
RuntimeErrorhere is not a client validation problem. Mapping it toApiValidationErrorturns provider-side delete failures into HTTP 4xx responses, which breaks retry/error handling for callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/providers.py` around lines 746 - 754, The RuntimeError in the provider delete path is a server-side failure and should not be mapped to ApiValidationError (a 4xx); change the except block that currently catches RuntimeError in controllers/providers.py to either re-raise the original RuntimeError or raise an ApiServerError (or equivalent 5xx exception) instead of ApiValidationError; keep the existing logger call (adjust level to error if appropriate) and include str(exc) in the log, and ensure the raised exception is ApiServerError(...) from exc (or simply raise) so callers get a 5xx response rather than a 4xx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 811-824: The missing-model branch is an internal invariant failure
(service should have raised earlier) so don't treat it as a client validation
error; update the branch that currently logs API_VALIDATION_FAILED and raises
ApiValidationError to instead log as a provider/internal error (use the
controller's provider-error/5xx log level/constant instead of
API_VALIDATION_FAILED) and raise an internal server/provider error so the
request takes the controller's 5xx/provider-error path (replace raising
ApiValidationError with the appropriate internal/provider error type used by the
controllers); locate the code around the model lookup (variables model,
updated.models, model_id) in controllers/providers.py to apply this change.
In `@src/synthorg/providers/management/local_models.py`:
- Around line 97-107: The docstring for delete_model in local_models.py is
inaccurate: OllamaModelManager.delete_model actually raises ValueError for 404
(model not found) and RuntimeError for other HTTP errors; update the
delete_model docstring to state these exact exception semantics (ValueError when
model does not exist, RuntimeError on other delete failures) or alternatively
change OllamaModelManager.delete_model to raise the same exception type for all
failures—refer to the delete_model method and OllamaModelManager.delete_model to
locate and apply the change.
- Around line 293-347: Update the delete_model method docstring to accurately
reflect its error behavior: state that ValueError is raised when the model is
not found (HTTP 404), and RuntimeError is raised for other HTTP client/server
errors (status >= 400) and transport/httpx errors; mention these conditions by
name (delete_model, httpx.HTTPError) so reviewers can verify the docstring
matches the implementation.
In `@web/src/stores/providers.ts`:
- Around line 421-429: The providers list cache isn't refreshed after a model
pull/delete (the code around useToastStore.getState().add and the
selectedProvider check), so list-level UI (provider.models counts/badges) stays
stale; after a successful mutation return, call the store method that reloads
the providers array (e.g., get().fetchProviders()) to update the cached
providers list, and do the same fix for the similar block around lines 457-465
so both pull and delete refresh the top-level providers cache as well as the
detail view (keep the existing selectedProvider/fetchProviderDetail logic).
---
Duplicate comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 650-652: The SSE payloads currently call
event.model_dump(exclude_none=True) which strips keys with None and breaks the
PullProgressEvent contract; change the serialization to preserve nullable keys
(e.g., use event.model_dump() or build a dict that explicitly includes
progress_percent, total_bytes, completed_bytes, and error with their None
values) wherever you see "data":
_json.dumps(event.model_dump(exclude_none=True)) (including the occurrences
around the PullProgressEvent emission at the shown snippet and the similar
blocks at the other noted ranges) so the emitted JSON contains the keys with
null values instead of omitting them.
- Around line 746-754: The RuntimeError in the provider delete path is a
server-side failure and should not be mapped to ApiValidationError (a 4xx);
change the except block that currently catches RuntimeError in
controllers/providers.py to either re-raise the original RuntimeError or raise
an ApiServerError (or equivalent 5xx exception) instead of ApiValidationError;
keep the existing logger call (adjust level to error if appropriate) and include
str(exc) in the log, and ensure the raised exception is ApiServerError(...) from
exc (or simply raise) so callers get a 5xx response rather than a 4xx.
In `@web/src/stores/providers.ts`:
- Around line 439-443: The finally block currently always calls set({
pullingModel: false }) even if a newer pull replaced the original; change it to
only clear pullingModel when the abort controller that started this request is
still the active one. Concretely, in the finally of the pull function check
_pullAbortController === controller and only then set({ pullingModel: false })
(i.e., move the set call inside the existing if that nulls _pullAbortController)
so older settled requests don't flip pullingModel for a newer active pull.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b79eb2ed-c0c8-47b4-a704-e87a4c8e2307
📒 Files selected for processing (3)
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.pyweb/src/stores/providers.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{tsx,ts}: ALWAYS reuse existing components fromweb/src/components/ui/before creating new ones (StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast, Skeleton, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, TagInput, MetadataGrid, ProjectStatusBadge, ContentTypeBadge)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success,bg-danger, etc.) or CSS variables (var(--so-accent)) for colors. NEVER hardcode hex values in.tsx/.tsfiles.
Usefont-sansorfont-monofor typography (maps to Geist tokens). NEVER setfontFamilydirectly.
Use density-aware tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing. NEVER hardcode pixel values for layout spacing.
Use token variables (var(--so-shadow-card-hover),border-border,border-bright) for shadows and borders. NEVER hardcode values.
Importcnfrom@/lib/utilsfor conditional class merging in new components.
Do NOT recreate status dots inline -- use<StatusBadge>component.
Do NOT build card-with-header layouts from scratch -- use<SectionCard>component.
Do NOT create metric displays withtext-metric font-bold-- use<MetricCard>component.
Do NOT render initials circles manually -- use<Avatar>component.
Do NOT create complex (>8 line) JSX inside.map()-- extract to a shared component.
TypeScript strict mode must pass type checking (runnpm --prefix web run type-check); all type errors must be resolved before proceeding.
Do NOT hardcode Framer Motion transition durations -- use@/lib/motionpresets.
Files:
web/src/stores/providers.ts
web/src/**/*.{tsx,ts,css}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Do NOT use
rgba()with hardcoded values -- use design token variables.
Files:
web/src/stores/providers.ts
web/src/**/*.{tsx,ts,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
ESLint must enforce zero warnings (run
npm --prefix web run lint); all warnings must be fixed before proceeding.
Files:
web/src/stores/providers.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Reuse existing components fromweb/src/components/ui/before creating new ones. Never hardcode hex colors, font-family, pixel spacing, or Framer Motion transitions—use design tokens and@/lib/motionpresets.
Web dashboard component updates must follow design token rules and reuse existing components. A PostToolUse hook enforces these rules on every Edit/Write toweb/src/.
Web dashboard is built with React 19, TypeScript 6.0+, and design system components from shadcn/ui and Radix UI. Use Tailwind CSS 4 for styling with design tokens.
Files:
web/src/stores/providers.ts
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations—Python 3.14 has PEP 649 native lazy annotations.
Use PEP 758 except syntax: useexcept A, B:(no parentheses)—ruff enforces this on Python 3.14.
All public functions must have type hints; mypy strict mode is required.
Docstrings must use Google style and are required on all public classes and functions—enforced by ruff D rules.
Create new objects instead of mutating existing ones—use immutability as the default pattern.
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement.
Usemodel_copy(update=...)for runtime state that evolves; separate mutable-via-copy models from frozen Pydantic models for config/identity. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict). Useallow_inf_nan=Falsein allConfigDictdeclarations to rejectNaN/Infin numeric fields at validation time.
Use@computed_fieldfor derived values instead of storing and validating redundant fields (e.g.TokenUsage.total_tokens).
UseNotBlankStrfromcore.typesfor all identifier/name fields—including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants—instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task. Existing code is being migrated incrementally.
Function line length limit: 88 characters; enforced by ruff.
Functions must be less than 50 lines; files must be less than 800 lines.
Handle errors explicitly; never silently swallow exceptions.
Validate at system boundaries (user input, external APIs, config files).
Logger variable name must always belogger(not_logger, notlog).
Always use structured logging with kwargs: `l...
Files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__).
Never useimport logging/logging.getLogger()/print()in application code. Exception:observability/setup.py,observability/sinks.py,observability/syslog_handler.py, andobservability/http_handler.pymay use stdlibloggingandprint(..., file=sys.stderr)for handler construction and bootstrap.
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:example-provider,example-large-001,example-medium-001,example-small-001. Vendor names may only appear in: (1) Operations design page, (2).claude/files, (3) third-party import paths, (4) provider presets. Tests must usetest-provider,test-small-001, etc.
Files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/providers/**/*.py: All provider calls go throughBaseCompletionProviderwhich applies retry and rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig. Retryable errors (is_retryable=True):RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError. Non-retryable errors raise immediately without retry.
Rate limiter respectsRateLimitError.retry_afterfrom providers—automatically pauses future requests.
Files:
src/synthorg/providers/management/local_models.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
🧠 Learnings (35)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/providers/**/*.py : Providers: LLM provider abstraction (LiteLLM adapter), auth types (api_key/oauth/custom_header/none), presets (PROVIDER_PRESETS), runtime CRUD (ProviderManagementService with asyncio.Lock serialization), hot-reload via AppState swap.
📚 Learning: 2026-03-30T10:41:40.176Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T10:41:40.176Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use Zustand stores for state management in the web dashboard; each domain has its own store module (auth, WebSocket, toast, analytics, setup, company, agents, budget, tasks, settings, providers, theme, per-domain stores)
Applied to files:
web/src/stores/providers.ts
📚 Learning: 2026-03-27T12:44:29.466Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T12:44:29.466Z
Learning: Applies to web/src/stores/**/*.{ts,tsx} : Use Zustand stores in web dashboard for state management (auth, WebSocket, toast, analytics, domain shells)
Applied to files:
web/src/stores/providers.ts
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/providers/**/*.py : Providers: LLM provider abstraction (LiteLLM adapter), auth types (api_key/oauth/custom_header/none), presets (PROVIDER_PRESETS), runtime CRUD (ProviderManagementService with asyncio.Lock serialization), hot-reload via AppState swap.
Applied to files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T11:18:48.128Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T11:18:48.128Z
Learning: Applies to src/synthorg/**/*.py : Retryable errors (`is_retryable=True`): `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately without retry.
Applied to files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-04-01T17:49:14.133Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T17:49:14.133Z
Learning: Applies to src/synthorg/{providers,engine}/**/*.py : Retryable errors are `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`; non-retryable errors raise immediately; `RetryExhaustedError` signals all retries failed
Applied to files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, and key function entry/exit
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, entry/exit of key functions.
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-17T06:43:14.114Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:43:14.114Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, entry/exit of key functions. Pure data models, enums, and re-exports do NOT need logging.
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-31T20:07:03.035Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T20:07:03.035Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising; all state transitions must log at INFO; DEBUG for object creation, internal flow, entry/exit of key functions
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-15T16:55:07.730Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T16:55:07.730Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Applied to files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/**/*.py : `RetryConfig` and `RateLimiterConfig` are set per-provider in `ProviderConfig`. Retryable errors: `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately.
Applied to files:
src/synthorg/providers/management/local_models.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-31T16:09:24.320Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T16:09:24.320Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from `synthorg.observability.events.<domain>` modules (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`); import directly and use in structured logging
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`); import directly rather than using string literals
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-04-03T13:34:37.835Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T13:34:37.835Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (via `model_copy(update=...)`) for runtime state that evolves
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T11:18:48.128Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T11:18:48.128Z
Learning: Applies to src/synthorg/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-31T21:07:37.469Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T21:07:37.469Z
Learning: Applies to src/synthorg/providers/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T19:13:36.562Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:13:36.562Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-04-03T13:34:37.835Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T13:34:37.835Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in `ProviderConfig`. Retryable errors (`is_retryable=True`): `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately without retry.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 BaseModel, model_validator, computed_field, ConfigDict.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T19:13:34.746Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:13:34.746Z
Learning: Applies to src/synthorg/providers/**/*.py : Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately without retry. RetryExhaustedError signals that all retries failed — the engine layer catches this to trigger fallback chains.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/providers.py
🔇 Additional comments (7)
src/synthorg/providers/management/local_models.py (7)
1-33: LGTM!Module setup follows conventions: proper observability imports, correct logger instantiation, and well-named constants for timeouts and HTTP status codes. The
TYPE_CHECKINGguard forAsyncIteratoris appropriate with PEP 649's lazy annotation evaluation in Python 3.14.
35-74: LGTM!Well-designed immutable Pydantic model with proper constraints. The cross-field validator correctly enforces that
errorevents are terminal andcompleted_bytesnever exceedstotal_bytes. Good use ofNotBlankStrfor string fields andallow_inf_nan=Falsein ConfigDict.
110-128: LGTM!Constructor properly validates
base_urlis non-empty and normalizes the trailing slash. The optionalclientinjection enables testability.
130-185: LGTM!The
_parse_pull_linemethod correctly handles error events, computes progress percentage with proper bounds (min(..., 100.0)), and logs at appropriate levels. The distinction between using float values for progress calculation while only storing integer byte counts is intentional and correct.
187-224: LGTM!Robust stream consumption: malformed JSON lines are logged and skipped without breaking the stream, early return on terminal events, and a fallback error event if the stream ends unexpectedly. Good defensive programming.
226-291: LGTM!The
pull_modelmethod properly handles client lifecycle, logs state transitions, and converts all failure modes (non-200, transport errors) into terminalPullProgressEventinstances with appropriate logging. Thefinallyblock ensures cleanup of owned clients.
350-366: LGTM!Clean factory function with proper type hints and docstring. The
Nonereturn for unsupported presets is correctly handled by_resolve_local_managerin the service layer, which raisesProviderValidationErrorin that case.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
web/src/stores/providers.ts (1)
404-410:⚠️ Potential issue | 🟠 MajorIgnore progress callbacks from superseded pull requests.
The
finallyguard fixed stale cleanup, but this callback can still flush a buffered SSE chunk afterabort()has been issued. Because Lines 407-409 always writepullProgress, an aborted/superseded request can overwrite the newer pull's state or repopulate state aftercancelPull()cleared it.🔧 Tie progress updates to the controller that started this pull
await apiPullModel( name, modelName, (event) => { + if (_pullAbortController !== controller) return if (event.error) lastError = event.error set({ pullProgress: event }) }, controller.signal, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/providers.ts` around lines 404 - 410, Tie progress updates to the controller that started the pull: when calling apiPullModel, capture the controller used for this request (e.g., const myController = controller or similar) and inside the progress callback first verify this is still the active controller (compare myController to the store's currentPullController or check myController.signal.aborted); if it’s not the active controller or is aborted, ignore the event and do not call set({ pullProgress: event }); this prevents superseded/aborted pulls from overwriting state after cancelPull()/abort() was called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 664-668: The SSE generator currently maps every terminal
PullProgressEvent to "complete"; change the logic around svc.pull_model
iteration so that when an event is terminal (event.done) and carries an error
(event.error is truthy) you emit "event": "error" instead of "complete"
(otherwise emit "complete" when done and no error, and "progress" while not
done). Update the yield in the async for over svc.pull_model to inspect
event.done and event.error and set the event type accordingly while preserving
the existing payload construction via _json.dumps(event.model_dump()).
- Around line 321-327: The controller currently strips preset_name via safe_data
= data.model_copy(update={"preset_name": None}) before calling
provider_management.create_provider, which silently creates providers missing
preset metadata used by _resolve_local_manager; instead validate the incoming
request at this boundary: if data.preset_name is not None, return a 4xx error
(HTTPException) telling callers to use POST /providers/from-preset, or
alternatively preserve preset_name (remove the model_copy update) so that
create_provider receives it; update the branch around safe_data/data.model_copy
and create_provider to implement one of these two behaviors and ensure
_resolve_local_manager will have the preset metadata it expects.
In `@src/synthorg/providers/management/local_models.py`:
- Around line 197-208: The decoded JSON chunk from json.loads may be a scalar or
list; before calling _parse_pull_line, validate that `data` is a JSON object
(e.g. isinstance(data, dict)) and if not, call `logger.warning` with the same
`PROVIDER_MODEL_PULL_FAILED` message including `provider="ollama"`,
`model=model_name`, and `error` describing a non-object payload (include a
snippet of `line[:200]!r`), then continue the loop; this ensures non-object
payloads are treated as provider pull failures rather than causing an
AttributeError inside `_parse_pull_line`.
---
Duplicate comments:
In `@web/src/stores/providers.ts`:
- Around line 404-410: Tie progress updates to the controller that started the
pull: when calling apiPullModel, capture the controller used for this request
(e.g., const myController = controller or similar) and inside the progress
callback first verify this is still the active controller (compare myController
to the store's currentPullController or check myController.signal.aborted); if
it’s not the active controller or is aborted, ignore the event and do not call
set({ pullProgress: event }); this prevents superseded/aborted pulls from
overwriting state after cancelPull()/abort() was called.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99c6ba6d-e00a-40a3-b5bb-d4d25f75a203
📒 Files selected for processing (3)
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.pyweb/src/stores/providers.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (8)
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{tsx,ts}: ALWAYS reuse existing components fromweb/src/components/ui/before creating new ones (StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast, Skeleton, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, TagInput, MetadataGrid, ProjectStatusBadge, ContentTypeBadge)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success,bg-danger, etc.) or CSS variables (var(--so-accent)) for colors. NEVER hardcode hex values in.tsx/.tsfiles.
Usefont-sansorfont-monofor typography (maps to Geist tokens). NEVER setfontFamilydirectly.
Use density-aware tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing. NEVER hardcode pixel values for layout spacing.
Use token variables (var(--so-shadow-card-hover),border-border,border-bright) for shadows and borders. NEVER hardcode values.
Importcnfrom@/lib/utilsfor conditional class merging in new components.
Do NOT recreate status dots inline -- use<StatusBadge>component.
Do NOT build card-with-header layouts from scratch -- use<SectionCard>component.
Do NOT create metric displays withtext-metric font-bold-- use<MetricCard>component.
Do NOT render initials circles manually -- use<Avatar>component.
Do NOT create complex (>8 line) JSX inside.map()-- extract to a shared component.
TypeScript strict mode must pass type checking (runnpm --prefix web run type-check); all type errors must be resolved before proceeding.
Do NOT hardcode Framer Motion transition durations -- use@/lib/motionpresets.
Files:
web/src/stores/providers.ts
web/src/**/*.{tsx,ts,css}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Do NOT use
rgba()with hardcoded values -- use design token variables.
Files:
web/src/stores/providers.ts
web/src/**/*.{tsx,ts,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
ESLint must enforce zero warnings (run
npm --prefix web run lint); all warnings must be fixed before proceeding.
Files:
web/src/stores/providers.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Reuse existing components fromweb/src/components/ui/before creating new ones. Never hardcode hex colors, font-family, pixel spacing, or Framer Motion transitions—use design tokens and@/lib/motionpresets.
Web dashboard component updates must follow design token rules and reuse existing components. A PostToolUse hook enforces these rules on every Edit/Write toweb/src/.
Web dashboard is built with React 19, TypeScript 6.0+, and design system components from shadcn/ui and Radix UI. Use Tailwind CSS 4 for styling with design tokens.
Files:
web/src/stores/providers.ts
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations—Python 3.14 has PEP 649 native lazy annotations.
Use PEP 758 except syntax: useexcept A, B:(no parentheses)—ruff enforces this on Python 3.14.
All public functions must have type hints; mypy strict mode is required.
Docstrings must use Google style and are required on all public classes and functions—enforced by ruff D rules.
Create new objects instead of mutating existing ones—use immutability as the default pattern.
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement.
Usemodel_copy(update=...)for runtime state that evolves; separate mutable-via-copy models from frozen Pydantic models for config/identity. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict). Useallow_inf_nan=Falsein allConfigDictdeclarations to rejectNaN/Infin numeric fields at validation time.
Use@computed_fieldfor derived values instead of storing and validating redundant fields (e.g.TokenUsage.total_tokens).
UseNotBlankStrfromcore.typesfor all identifier/name fields—including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants—instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task. Existing code is being migrated incrementally.
Function line length limit: 88 characters; enforced by ruff.
Functions must be less than 50 lines; files must be less than 800 lines.
Handle errors explicitly; never silently swallow exceptions.
Validate at system boundaries (user input, external APIs, config files).
Logger variable name must always belogger(not_logger, notlog).
Always use structured logging with kwargs: `l...
Files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__).
Never useimport logging/logging.getLogger()/print()in application code. Exception:observability/setup.py,observability/sinks.py,observability/syslog_handler.py, andobservability/http_handler.pymay use stdlibloggingandprint(..., file=sys.stderr)for handler construction and bootstrap.
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:example-provider,example-large-001,example-medium-001,example-small-001. Vendor names may only appear in: (1) Operations design page, (2).claude/files, (3) third-party import paths, (4) provider presets. Tests must usetest-provider,test-small-001, etc.
Files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/providers/**/*.py: All provider calls go throughBaseCompletionProviderwhich applies retry and rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig. Retryable errors (is_retryable=True):RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError. Non-retryable errors raise immediately without retry.
Rate limiter respectsRateLimitError.retry_afterfrom providers—automatically pauses future requests.
Files:
src/synthorg/providers/management/local_models.py
🧠 Learnings (35)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/providers/**/*.py : Providers: LLM provider abstraction (LiteLLM adapter), auth types (api_key/oauth/custom_header/none), presets (PROVIDER_PRESETS), runtime CRUD (ProviderManagementService with asyncio.Lock serialization), hot-reload via AppState swap.
📚 Learning: 2026-03-30T10:41:40.176Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T10:41:40.176Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use Zustand stores for state management in the web dashboard; each domain has its own store module (auth, WebSocket, toast, analytics, setup, company, agents, budget, tasks, settings, providers, theme, per-domain stores)
Applied to files:
web/src/stores/providers.ts
📚 Learning: 2026-03-27T12:44:29.466Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T12:44:29.466Z
Learning: Applies to web/src/stores/**/*.{ts,tsx} : Use Zustand stores in web dashboard for state management (auth, WebSocket, toast, analytics, domain shells)
Applied to files:
web/src/stores/providers.ts
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/providers/**/*.py : Providers: LLM provider abstraction (LiteLLM adapter), auth types (api_key/oauth/custom_header/none), presets (PROVIDER_PRESETS), runtime CRUD (ProviderManagementService with asyncio.Lock serialization), hot-reload via AppState swap.
Applied to files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-31T16:09:24.320Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T16:09:24.320Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from `synthorg.observability.events.<domain>` modules (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`); import directly and use in structured logging
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`); import directly rather than using string literals
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-04-03T13:34:37.835Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T13:34:37.835Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Applied to files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (via `model_copy(update=...)`) for runtime state that evolves
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-20T11:18:48.128Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T11:18:48.128Z
Learning: Applies to src/synthorg/**/*.py : Retryable errors (`is_retryable=True`): `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately without retry.
Applied to files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-20T11:18:48.128Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T11:18:48.128Z
Learning: Applies to src/synthorg/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-31T21:07:37.469Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T21:07:37.469Z
Learning: Applies to src/synthorg/providers/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/**/*.py : `RetryConfig` and `RateLimiterConfig` are set per-provider in `ProviderConfig`. Retryable errors: `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately.
Applied to files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-16T19:13:36.562Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:13:36.562Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 BaseModel, model_validator, computed_field, ConfigDict.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-04-03T13:34:37.835Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T13:34:37.835Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in `ProviderConfig`. Retryable errors (`is_retryable=True`): `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately without retry.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T19:13:34.746Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:13:34.746Z
Learning: Applies to src/synthorg/providers/**/*.py : Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately without retry. RetryExhaustedError signals that all retries failed — the engine layer catches this to trigger fallback chains.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-04-01T17:49:14.133Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T17:49:14.133Z
Learning: Applies to src/synthorg/{providers,engine}/**/*.py : Retryable errors are `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`; non-retryable errors raise immediately; `RetryExhaustedError` signals all retries failed
Applied to files:
src/synthorg/api/controllers/providers.pysrc/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, and key function entry/exit
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, entry/exit of key functions.
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-17T06:43:14.114Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:43:14.114Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, entry/exit of key functions. Pure data models, enums, and re-exports do NOT need logging.
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-31T20:07:03.035Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T20:07:03.035Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising; all state transitions must log at INFO; DEBUG for object creation, internal flow, entry/exit of key functions
Applied to files:
src/synthorg/providers/management/local_models.py
📚 Learning: 2026-03-15T16:55:07.730Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T16:55:07.730Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/providers/management/local_models.py
2300ba8 to
301c062
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/pages/providers/ProviderCard.stories.tsx (1)
55-60: 🧹 Nitpick | 🔵 TrivialConsider setting capability flags for the LocalProvider story.
The
LocalProviderstory represents an Ollama provider (litellm_provider: 'ollama') but inheritssupports_model_pull: false,supports_model_delete: false, andsupports_model_config: falsefrombaseProvider. Per the PR objectives, Ollama should support these local model management features. Consider overriding these flags totrueto accurately demonstrate Ollama's capabilities in the story.📖 Suggested story update
export const LocalProvider: Story = { args: { - provider: { ...baseProvider, name: 'ollama-local', litellm_provider: 'ollama', auth_type: 'none', base_url: 'http://localhost:11434', models: [], has_api_key: false }, + provider: { ...baseProvider, name: 'ollama-local', litellm_provider: 'ollama', auth_type: 'none', base_url: 'http://localhost:11434', models: [], has_api_key: false, supports_model_pull: true, supports_model_delete: true, supports_model_config: true }, health: null, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/providers/ProviderCard.stories.tsx` around lines 55 - 60, The LocalProvider story currently uses baseProvider values that leave supports_model_pull, supports_model_delete, and supports_model_config false, but since it models an Ollama provider (litellm_provider: 'ollama') these capability flags should be true; update the LocalProvider.args.provider to explicitly set supports_model_pull: true, supports_model_delete: true, and supports_model_config: true so the story accurately reflects Ollama's local model management capabilities (locate the LocalProvider Story and the provider object in ProviderCard.stories.tsx to apply the overrides).tests/unit/providers/test_presets.py (1)
76-85:⚠️ Potential issue | 🔴 CriticalReplace real vendor names with vendor-agnostic test names.
The test file uses real provider names (
"anthropic","openai","gemini", etc.) in the_CLOUD_PRESETSand_SELF_HOSTED_PRESETStuples. Per coding guidelines, tests must use vendor-agnostic names only (e.g.,"test-provider","test-large-001", etc.). This applies to all test code, even when testing actual provider preset integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/providers/test_presets.py` around lines 76 - 85, Update the test fixtures _CLOUD_PRESETS and _SELF_HOSTED_PRESETS in test_presets.py to use vendor-agnostic names instead of real vendor names: replace entries in the _CLOUD_PRESETS tuple ("anthropic","openai","gemini","mistral","groq","deepseek","openrouter") and the _SELF_HOSTED_PRESETS tuple ("ollama","lm-studio","vllm","azure") with generic test values (e.g., "test-provider-1", "test-provider-2", "test-model-1", etc.) so the tuples remain the same shape but contain no real vendor identifiers.
♻️ Duplicate comments (2)
web/src/pages/providers/ModelConfigDrawer.tsx (1)
34-52:⚠️ Potential issue | 🟠 MajorDon't collapse invalid numeric input to
null.The parsers still turn any non-empty invalid value into
null, so a typo clears the saved param instead of failing validation. That's especially confusing fornum_threads, because the"auto"placeholder implies a supported value even though typing"auto"currently clears the field. Keep blank input asnull, but surface per-field errors and block save when a value is malformed or outside the backend's allowed range. Based on learnings, "Validate at system boundaries (user input, external APIs, config files)."Also applies to: 60-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/providers/ModelConfigDrawer.tsx` around lines 34 - 52, The current handleSave implementation uses parseIntStrict and parseFloatStrict which convert any non-empty invalid string into null, unintentionally clearing values (e.g., num_threads with "auto"); change this so empty strings still map to null but malformed or out-of-range inputs do not become null—instead set per-field validation errors and prevent saving. Update parseIntStrict/parseFloatStrict (or add validateInt/validateFloat) to return a discriminated result (e.g., {value:number|null, error?:string}) or throw a validation error, then in handleSave build newParams only from valid parsed values and if any field (num_ctx, num_gpu_layers, num_threads, num_batch, repeat_penalty) has a parse/range error set corresponding UI error state and abort the save; ensure LocalModelParams remains correctly typed and that input placeholder values like "auto" are treated as invalid text (not silently nulled) so the UI can surface the error.src/synthorg/api/controllers/providers.py (1)
321-327:⚠️ Potential issue | 🟠 MajorDon't silently drop
preset_nameonPOST /providers.This makes an invalid request look successful while creating a provider that no longer carries the preset metadata
_resolve_local_manager()uses insrc/synthorg/providers/management/service.py:597-639for pull/delete/config. If/providersis not allowed to acceptpreset_name, reject it here and point callers at/providers/from-preset; otherwise preserve the field.🛡️ Fail fast instead of creating a half-configured provider
- # Strip preset_name from external requests -- only - # create_from_preset may set it (capability flag injection). - safe_data = data.model_copy(update={"preset_name": None}) + if data.preset_name is not None: + logger.warning( + API_VALIDATION_FAILED, + resource="provider", + name=data.name, + error="preset_name is only accepted by /providers/from-preset", + ) + raise ApiValidationError( + "Use /providers/from-preset when supplying preset_name", + ) try: - config = await app_state.provider_management.create_provider( - safe_data, - ) + config = await app_state.provider_management.create_provider(data)As per coding guidelines, "Validate at system boundaries (user input, external APIs, config files)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/providers.py` around lines 321 - 327, The handler currently strips preset_name from incoming POST /providers requests (using data.model_copy(...) before calling app_state.provider_management.create_provider), which lets invalid requests succeed and breaks downstream logic in _resolve_local_manager (src/synthorg/providers/management/service.py). Change the endpoint to validate at the boundary: if data.preset_name is present, return a 400/validation error that instructs callers to use /providers/from-preset; otherwise pass the original data through (do not drop preset_name) to app_state.provider_management.create_provider. Ensure the error message references the correct caller action and keep handling centered in the same request handler that currently calls create_provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 755-763: The logger in delete_model currently uses the
pull-specific event constant API_SSE_PULL_MODEL_FAILED; update the
logger.exception call in delete_model to use a delete-specific constant (e.g.,
API_SSE_DELETE_MODEL_FAILED) or a generic model failure constant (e.g.,
API_SSE_MODEL_OPERATION_FAILED), and if that constant does not exist add it to
the event constants module; ensure the same replacement is made for any related
error-raising paths in delete_model so the event name semantically matches the
delete operation.
- Around line 802-819: The current handler relies on string-matching "not found"
on ProviderValidationError (variable exc_msg) which is brittle; update the
ProviderValidationError hierarchy to include an explicit subclass (e.g.,
ProviderNotFoundError) or an error code attribute (e.g., .code == "NOT_FOUND"),
then change the except block in the controller to test that explicit signal (use
isinstance(exc, ProviderNotFoundError) or check exc.code) instead of `"not
found" in exc_msg`, and keep the existing logging calls (API_RESOURCE_NOT_FOUND
with resource="model", name=model_id, provider=name and raising NotFoundError)
for the not-found case, otherwise log API_VALIDATION_FAILED and raise
ApiValidationError as before.
In `@src/synthorg/config/schema.py`:
- Around line 45-58: Docstrings for the public config classes are out of date:
update the Google-style class docstrings for LocalModelParams and ProviderConfig
to list the new fields (for LocalModelParams mention num_ctx, num_gpu_layers,
num_threads, num_batch, repeat_penalty and their brief meanings/constraints; for
ProviderConfig mention the newly exposed preset_name and its purpose) so
generated docs/IDE hovers reflect the current public API; edit the docstring
blocks at the top of the LocalModelParams class and the ProviderConfig class to
include one-line descriptions for each added field following the existing
Google-style format.
In `@src/synthorg/providers/management/service.py`:
- Around line 715-726: The capability check done by _resolve_local_manager
before acquiring self._lock can race with concurrent provider updates; move the
capability validation into the locked section (the block that calls
self._config_resolver.get_provider_configs()) or, after re-reading config under
the lock in update_model_config, re-validate that the provider still has the
required fields (preset_name/base_url) and capability before proceeding.
Specifically, adjust the flow around _resolve_local_manager and the locked read
of get_provider_configs so that either the capability check is performed while
holding self._lock or add explicit re-validation of preset_name/base_url and the
requested capability after fetching config and before raising
ProviderNotFoundError or continuing.
In `@tests/unit/providers/management/test_local_models.py`:
- Around line 28-34: Add a test that verifies the terminal-error invariant for
PullProgressEvent: when constructing PullProgressEvent with an error (e.g.,
PullProgressEvent(status="pulling", error="some error")) but without done=True,
the model should reject it; update tests in test_defaults (or add a new test) to
assert that instantiating PullProgressEvent(status="pulling", error="msg")
raises the expected validation exception (e.g., ValueError/ValidationError)
rather than silently allowing a non-terminal error state.
In `@web/src/api/endpoints/providers.ts`:
- Around line 133-140: The code in processSseLines currently throws an Error
when state.currentEvent === 'error' or parsed.error is present before calling
onProgress, which prevents the caller from receiving the terminal error frame;
change the control flow in processSseLines so that when a terminal error is
detected (state.currentEvent === 'error' or parsed.error), you first call
onProgress(parsed) to deliver the final event (so pullProgress is persisted),
then clear/reset state.currentEvent and only after notifying the caller throw
the Error (using parsed.error || parsed.status || 'Pull failed') to preserve
behavior while ensuring the callback sees the terminal payload.
- Around line 178-184: This fetch error branch currently only clears
localStorage keys ('auth_token', 'auth_token_expires_at',
'auth_must_change_password') which leaves the app auth store and redirect state
inconsistent; replace that local-only cleanup with the shared 401 logout flow
used by apiClient interceptors (i.e., call the central logout/unauthorized
handler used elsewhere so the auth store is cleared and the redirect path/router
is updated) instead of directly removing localStorage keys in this response
handling block.
In `@web/src/pages/ProviderDetailPage.tsx`:
- Around line 91-93: The onRefresh handler calls
useProvidersStore.getState().discoverModels(decodedName) but doesn't thread the
provider's preset_name; update the call in the onRefresh callback to pass the
preset hint, e.g. call useProvidersStore.getState().discoverModels(decodedName,
provider.preset_name ?? undefined), so discoverModels(presetHint) receives
provider.preset_name for backward-compatible refresh behavior; locate the
onRefresh block in ProviderDetailPage.tsx and modify the discoverModels
invocation accordingly.
In `@web/src/pages/providers/ModelConfigDrawer.stories.tsx`:
- Around line 52-58: The EmptyParams story duplicates Default because baseModel
already has local_params: null; either remove the redundant EmptyParams export
or change its args to demonstrate a different state (e.g., provide a model with
non-null local_params or other differing fields) so it meaningfully differs from
Default; update the EmptyParams export or delete it accordingly, referencing the
EmptyParams and Default Story objects and the baseModel/local_params values.
In `@web/src/pages/providers/ModelPullDialog.stories.tsx`:
- Around line 62-70: The story currently sets useProvidersStore.pullingModel
true which simulates an in-flight/cancellable pull; change the fixture to
reflect a terminal failed pull by setting pullingModel: false and leaving
pullProgress as an error (e.g. pullProgress.status = 'Error', progress_percent =
0, done = true, error message present) so the UI renders the post-failure
retry/error state the real pullModel() finally produces; update the object in
the ModelPullDialog.stories.tsx snippet that sets
useProvidersStore.setState(...) accordingly (referencing useProvidersStore,
pullingModel, and pullProgress).
In `@web/src/pages/providers/ModelPullDialog.tsx`:
- Around line 83-99: The status and error text updates in ModelPullDialog
(statusText and pullProgress.error) are not announced to assistive tech; wrap
the status/error block (the paragraph showing statusText and the conditional
error paragraph) with the existing LiveRegion component from
web/src/components/ui/LiveRegion (or add aria-live="polite" aria-atomic="true"
to that container) so streamed SSE updates are announced; ensure you use the
LiveRegion wrapper rather than creating a new component to comply with
guidelines and keep the ProgressGauge and other layout intact.
---
Outside diff comments:
In `@tests/unit/providers/test_presets.py`:
- Around line 76-85: Update the test fixtures _CLOUD_PRESETS and
_SELF_HOSTED_PRESETS in test_presets.py to use vendor-agnostic names instead of
real vendor names: replace entries in the _CLOUD_PRESETS tuple
("anthropic","openai","gemini","mistral","groq","deepseek","openrouter") and the
_SELF_HOSTED_PRESETS tuple ("ollama","lm-studio","vllm","azure") with generic
test values (e.g., "test-provider-1", "test-provider-2", "test-model-1", etc.)
so the tuples remain the same shape but contain no real vendor identifiers.
In `@web/src/pages/providers/ProviderCard.stories.tsx`:
- Around line 55-60: The LocalProvider story currently uses baseProvider values
that leave supports_model_pull, supports_model_delete, and supports_model_config
false, but since it models an Ollama provider (litellm_provider: 'ollama') these
capability flags should be true; update the LocalProvider.args.provider to
explicitly set supports_model_pull: true, supports_model_delete: true, and
supports_model_config: true so the story accurately reflects Ollama's local
model management capabilities (locate the LocalProvider Story and the provider
object in ProviderCard.stories.tsx to apply the overrides).
---
Duplicate comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 321-327: The handler currently strips preset_name from incoming
POST /providers requests (using data.model_copy(...) before calling
app_state.provider_management.create_provider), which lets invalid requests
succeed and breaks downstream logic in _resolve_local_manager
(src/synthorg/providers/management/service.py). Change the endpoint to validate
at the boundary: if data.preset_name is present, return a 400/validation error
that instructs callers to use /providers/from-preset; otherwise pass the
original data through (do not drop preset_name) to
app_state.provider_management.create_provider. Ensure the error message
references the correct caller action and keep handling centered in the same
request handler that currently calls create_provider.
In `@web/src/pages/providers/ModelConfigDrawer.tsx`:
- Around line 34-52: The current handleSave implementation uses parseIntStrict
and parseFloatStrict which convert any non-empty invalid string into null,
unintentionally clearing values (e.g., num_threads with "auto"); change this so
empty strings still map to null but malformed or out-of-range inputs do not
become null—instead set per-field validation errors and prevent saving. Update
parseIntStrict/parseFloatStrict (or add validateInt/validateFloat) to return a
discriminated result (e.g., {value:number|null, error?:string}) or throw a
validation error, then in handleSave build newParams only from valid parsed
values and if any field (num_ctx, num_gpu_layers, num_threads, num_batch,
repeat_penalty) has a parse/range error set corresponding UI error state and
abort the save; ensure LocalModelParams remains correctly typed and that input
placeholder values like "auto" are treated as invalid text (not silently nulled)
so the UI can surface the error.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fe3f4d0-4a41-448f-bc7a-ccd06cb7e6f5
📒 Files selected for processing (36)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/controllers/providers.pysrc/synthorg/api/dto_providers.pysrc/synthorg/config/schema.pysrc/synthorg/observability/events/api.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/management/_helpers.pysrc/synthorg/providers/management/local_models.pysrc/synthorg/providers/management/service.pysrc/synthorg/providers/presets.pytests/unit/providers/management/test_local_models.pytests/unit/providers/management/test_service_local_models.pytests/unit/providers/test_presets.pyweb/src/__tests__/pages/ProviderDetailPage.test.tsxweb/src/__tests__/pages/ProvidersPage.test.tsxweb/src/__tests__/stores/setup-wizard.test.tsweb/src/__tests__/utils/providers.test.tsweb/src/__tests__/utils/setup-validation.property.test.tsweb/src/__tests__/utils/setup-validation.test.tsweb/src/api/endpoints/providers.tsweb/src/api/types.tsweb/src/pages/ProviderDetailPage.tsxweb/src/pages/providers/ModelConfigDrawer.stories.tsxweb/src/pages/providers/ModelConfigDrawer.tsxweb/src/pages/providers/ModelPullDialog.stories.tsxweb/src/pages/providers/ModelPullDialog.tsxweb/src/pages/providers/PresetPicker.stories.tsxweb/src/pages/providers/PresetPicker.tsxweb/src/pages/providers/ProviderCard.stories.tsxweb/src/pages/providers/ProviderDetailHeader.tsxweb/src/pages/providers/ProviderFormModal.tsxweb/src/pages/providers/ProviderModelList.stories.tsxweb/src/pages/providers/ProviderModelList.tsxweb/src/pages/providers/TestConnectionResult.tsxweb/src/stores/providers.ts
| except RuntimeError as exc: | ||
| logger.exception( | ||
| API_SSE_PULL_MODEL_FAILED, | ||
| resource="model", | ||
| name=model_id, | ||
| provider=name, | ||
| error=str(exc), | ||
| ) | ||
| raise ApiError(str(exc)) from exc |
There was a problem hiding this comment.
Mismatched event constant for delete errors.
API_SSE_PULL_MODEL_FAILED is used for RuntimeError in delete_model, but this endpoint handles model deletion, not pulling. Consider using a delete-specific event constant (or a generic model operation failure constant) for semantic accuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/providers.py` around lines 755 - 763, The logger
in delete_model currently uses the pull-specific event constant
API_SSE_PULL_MODEL_FAILED; update the logger.exception call in delete_model to
use a delete-specific constant (e.g., API_SSE_DELETE_MODEL_FAILED) or a generic
model failure constant (e.g., API_SSE_MODEL_OPERATION_FAILED), and if that
constant does not exist add it to the event constants module; ensure the same
replacement is made for any related error-raising paths in delete_model so the
event name semantically matches the delete operation.
| except ProviderValidationError as exc: | ||
| exc_msg = str(exc) | ||
| if "not found" in exc_msg: | ||
| logger.warning( | ||
| API_RESOURCE_NOT_FOUND, | ||
| resource="model", | ||
| name=model_id, | ||
| provider=name, | ||
| ) | ||
| raise NotFoundError(exc_msg) from exc | ||
| logger.warning( | ||
| API_VALIDATION_FAILED, | ||
| resource="provider", | ||
| name=name, | ||
| model=model_id, | ||
| error=exc_msg, | ||
| ) | ||
| raise ApiValidationError(exc_msg) from exc |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fragile string-based error classification.
Detecting "not found" via "not found" in exc_msg (line 804) is brittle—if the service's error message changes, this will silently break HTTP status code selection. Consider adding an error code or subclass to ProviderValidationError for explicit not-found cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/providers.py` around lines 802 - 819, The
current handler relies on string-matching "not found" on ProviderValidationError
(variable exc_msg) which is brittle; update the ProviderValidationError
hierarchy to include an explicit subclass (e.g., ProviderNotFoundError) or an
error code attribute (e.g., .code == "NOT_FOUND"), then change the except block
in the controller to test that explicit signal (use isinstance(exc,
ProviderNotFoundError) or check exc.code) instead of `"not found" in exc_msg`,
and keep the existing logging calls (API_RESOURCE_NOT_FOUND with
resource="model", name=model_id, provider=name and raising NotFoundError) for
the not-found case, otherwise log API_VALIDATION_FAILED and raise
ApiValidationError as before.
| await self._resolve_local_manager(name, capability="config") | ||
| async with self._lock: | ||
| providers = await self._config_resolver.get_provider_configs() | ||
| config = providers.get(name) | ||
| if config is None: | ||
| msg = f"Provider {name!r} not found" | ||
| logger.warning( | ||
| PROVIDER_NOT_FOUND, | ||
| provider=name, | ||
| error=msg, | ||
| ) | ||
| raise ProviderNotFoundError(msg) |
There was a problem hiding this comment.
Potential TOCTOU: capability check before lock, config read under lock.
_resolve_local_manager reads the provider config (via get_provider) outside the lock, then update_model_config re-reads it under the lock at line 717. If the provider is deleted or its preset_name/base_url changes between these two reads, the capability check passes but the locked read finds a different (or missing) config. The code handles the deletion case (lines 719-726), but a concurrent update that removes preset_name or base_url would bypass the capability guard.
Consider moving the capability check inside the lock, or re-validating the preset capability after re-reading the config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/providers/management/service.py` around lines 715 - 726, The
capability check done by _resolve_local_manager before acquiring self._lock can
race with concurrent provider updates; move the capability validation into the
locked section (the block that calls
self._config_resolver.get_provider_configs()) or, after re-reading config under
the lock in update_model_config, re-validate that the provider still has the
required fields (preset_name/base_url) and capability before proceeding.
Specifically, adjust the flow around _resolve_local_manager and the locked read
of get_provider_configs so that either the capability check is performed while
holding self._lock or add explicit re-validation of preset_name/base_url and the
requested capability after fetching config and before raising
ProviderNotFoundError or continuing.
| if (!response.ok || !response.body) { | ||
| if (response.status === 401) { | ||
| localStorage.removeItem('auth_token') | ||
| localStorage.removeItem('auth_token_expires_at') | ||
| localStorage.removeItem('auth_must_change_password') | ||
| } | ||
| throw new Error(`Pull failed: HTTP ${response.status}`) |
There was a problem hiding this comment.
Mirror the shared 401 logout flow on this fetch path.
This request bypasses apiClient interceptors. Clearing only localStorage here leaves the auth store and redirect path untouched, so an expired token during a pull can leave the UI looking logged in until some later axios request happens to fail.
♻️ Suggested change
if (!response.ok || !response.body) {
if (response.status === 401) {
localStorage.removeItem('auth_token')
localStorage.removeItem('auth_token_expires_at')
localStorage.removeItem('auth_must_change_password')
+ import('@/stores/auth').then(({ useAuthStore }) => {
+ useAuthStore.getState().logout()
+ }).catch(() => {
+ if (window.location.pathname !== '/login' && window.location.pathname !== '/setup') {
+ window.location.href = '/login'
+ }
+ })
}📝 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.
| if (!response.ok || !response.body) { | |
| if (response.status === 401) { | |
| localStorage.removeItem('auth_token') | |
| localStorage.removeItem('auth_token_expires_at') | |
| localStorage.removeItem('auth_must_change_password') | |
| } | |
| throw new Error(`Pull failed: HTTP ${response.status}`) | |
| import { useAuthStore } from '@/stores/auth' | |
| // ... elsewhere in the file ... | |
| if (!response.ok || !response.body) { | |
| if (response.status === 401) { | |
| localStorage.removeItem('auth_token') | |
| localStorage.removeItem('auth_token_expires_at') | |
| localStorage.removeItem('auth_must_change_password') | |
| useAuthStore.getState().logout() | |
| } | |
| throw new Error(`Pull failed: HTTP ${response.status}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/api/endpoints/providers.ts` around lines 178 - 184, This fetch error
branch currently only clears localStorage keys ('auth_token',
'auth_token_expires_at', 'auth_must_change_password') which leaves the app auth
store and redirect state inconsistent; replace that local-only cleanup with the
shared 401 logout flow used by apiClient interceptors (i.e., call the central
logout/unauthorized handler used elsewhere so the auth store is cleared and the
redirect path/router is updated) instead of directly removing localStorage keys
in this response handling block.
- Fix model count mismatch: skip models_from_litellm for local presets (auth_type=NONE), always use live discovery instead of stale static DB - Add preset capability flags: supports_model_pull/delete/config on ProviderPreset (Ollama: all true, LM Studio/vLLM: deferred) - Add LocalModelParams model for per-model launch parameters (num_ctx, num_gpu_layers, num_threads, num_batch) - Add preset_name to ProviderConfig for capability flag resolution - New OllamaModelManager: pull (streaming via /api/pull), delete (/api/delete) with LocalModelManager protocol - Service layer: pull_model, delete_model, update_model_config methods - API: SSE pull endpoint, DELETE model, PUT model config - Dashboard: model pull dialog with progress, delete with confirmation, per-model config drawer, refresh button, actions gated by preset caps - Extend ProviderResponse with capability flags derived from preset Closes #1030
- Fix ProviderModelResponse missing local_params field (API contract) - Add httpx error handling + terminal event in pull stream - Add SSE generator catch-all for unexpected exceptions - Extract _resolve_local_manager to reduce service.py under 800 lines - Extract _parse_pull_line to reduce pull_model under 50 lines - Condense LocalModelParams to reduce schema.py under 800 lines - Split local model tests to test_service_local_models.py (under 800) - Add repeat_penalty to LocalModelParams and dashboard config drawer - Fix post-delete refresh failure (catch + log, don't propagate) - Fix progress calc truthy check (isinstance + total > 0) - Fix bare next() StopIteration risk in controller - Add missing log-before-raise in 6+ error paths - Refactor ModelPullDialog: proper overlay, a11y attrs, Escape handler - Add aria-labels to model action buttons - Fix parseInt NaN validation in ModelConfigDrawer - Remove double padding in ModelConfigDrawer - Process remaining SSE buffer after stream ends - Add preset_name to frontend CreateProviderRequest - Add Storybook stories for ModelPullDialog and ModelConfigDrawer - Update CLAUDE.md and operations.md with new endpoints and features - Log malformed JSON in pull stream instead of silent discard - Change PullProgressEvent.error to NotBlankStr | None Pre-reviewed by 8 agents, 22 findings addressed
Backend: - Add API_SSE_PULL_MODEL_FAILED event constant, replace raw string - SSE error events now emit PullProgressEvent-shaped data (status + error + done) - Add MemoryError/RecursionError guard in SSE catch-all - Add model_name character-allowlist validator on PullModelRequest - Add Parameter(max_length=256) on model_id path params - Add missing log before raise in update_model_config assertion - Add capability check (supports_model_config) in update_model_config - Validate base_url is non-empty before creating local model manager - Add httpx.HTTPError handler in OllamaModelManager.delete_model - Add PullProgressEvent cross-field validators (error->done, bytes) - Add base_url validation in OllamaModelManager constructor - Split pull_model into pull_model + _consume_pull_stream (50-line rule) - Fix module docstring (Ollama only, not LM Studio) - Add ProviderPreset/ProviderResponse docstring Attributes for new fields Frontend: - Rewrite SSE parser: parse event: field, handle error events, track terminal done event, throw on stream end without completion - Fix URL encoding: use encodeModelIdPath preserving / for :path params - Handle 401 in fetch path (clear stale auth token) - Replace custom modal with Radix AlertDialog (focus trap, a11y) - Use individual Zustand selectors in ModelPullDialog and ProviderDetailPage - Use strict Number() parsing in ModelConfigDrawer (reject '12abc') - Abort existing pull controller before starting new one - Add Storybook stories for delete/config action columns Design tokens: - PresetPicker: gap-3 -> gap-grid-gap, p-4 -> p-card - ProviderFormModal: p-3 -> p-card on warning banner - TestConnectionResult: px-3 py-2 -> p-card on result banner - ModelConfigDrawer/ModelPullDialog: gap-4 -> gap-section-gap Tests: - Replace vendor name 'anthropic' with 'example-provider'/'openrouter' - Fix misleading test docstring (yields error event, not raises) Docs: - Fix event module count 56 -> 58 in operations.md
- Strip preset_name from external CreateProviderRequest at controller level to prevent capability flag injection (only create_from_preset may set preset_name) - Use RuntimeError (not ValueError) for non-404 delete errors so controller maps them to 422 instead of 404 - Add RuntimeError catch in delete_model controller - Replace raw <button> with <Button> component in ProviderModelList - Fix ModelPullDialog.stories.tsx: min-h-[400px] -> min-h-96, fix PullError story state (error requires done=true per validator) - Instance-tied abort controller cleanup in store pullModel - Track last SSE event -- show error toast if terminal event has error - Use fully qualified API paths in operations.md local model section
- Fix TS2339 in stores/providers.ts: track lastError string instead of lastEvent object (avoids CFA narrowing to never for callback- mutated variables) - Remove dead got_done variable in _consume_pull_stream (early return on event.done makes the flag unreachable) - Differentiate model-not-found from other ProviderValidationError in update_model_config controller (404 vs 422)
Backend:
- RuntimeError from upstream delete failures now raises ApiError (500)
instead of ApiValidationError (422) -- server errors should be 5xx
- Remove exclude_none=True from SSE progress events so nullable fields
(progress_percent, total_bytes, completed_bytes, error) are always
present as null instead of omitted
- Add _sse_error() helper for consistent PullProgressEvent-shaped error
dicts in all SSE error branches
- Missing-model-after-update invariant violation now raises ApiError
(500) instead of ApiValidationError (422)
- Update delete_model docstrings (protocol + impl) to distinguish
ValueError (404 not-found) from RuntimeError (upstream/transport)
Frontend:
- Move set({ pullingModel: false }) inside controller equality check
so older settled requests don't reset state for a newer active pull
- Refresh providers list cache (fetchProviders) after successful pull
and delete so list-level model counts stay current
- SSE generator now emits event type 'error' (not 'complete') when a terminal PullProgressEvent carries an error field - Guard _consume_pull_stream against non-object JSON payloads from Ollama (log warning and skip instead of AttributeError) - Progress callback ignores events after controller.signal.aborted to prevent superseded/cancelled pulls from updating store state
Backend: - Add API_MODEL_OPERATION_FAILED constant for delete/config errors (replaces misused API_SSE_PULL_MODEL_FAILED in non-SSE handlers) - Add LocalModelParams Attributes docstring (5 fields) - Add preset_name to ProviderConfig Attributes docstring - Guard _consume_pull_stream against non-dict JSON payloads Frontend: - SSE parser: call onProgress before throwing on error events so pullProgress state reflects the terminal error - Pass provider.preset_name to discoverModels on refresh - Wrap pull status/error text in LiveRegion for screen readers Tests: - Add 3 tests for PullProgressEvent cross-field invariants (error requires done, completed <= total, error+done valid) Stories: - PullError: set pullingModel=false for post-failure state - Rename EmptyParams to AllNullParams (differentiate from Default) - ProviderCard LocalProvider: add capability flags for Ollama
301c062 to
fa3da85
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
web/src/api/endpoints/providers.ts (1)
179-185:⚠️ Potential issue | 🟠 MajorReuse the shared unauthorized handler on this fetch path.
This still only clears
localStorageon 401. Because the request bypassesapiClient, the auth store and redirect state can stay stale until some later Axios request fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/api/endpoints/providers.ts` around lines 179 - 185, Replace the ad-hoc 401 cleanup that directly removes localStorage keys in this fetch path with the app's shared unauthorized handler so the auth store and redirect flow stay in sync; import and call the central handler (e.g., handleUnauthorized or authStore.handleUnauthorized) when response.status === 401 instead of removing 'auth_token', 'auth_token_expires_at', and 'auth_must_change_password' manually, then rethrow the error as before (i.e., call the shared handler before throwing `new Error(\`Pull failed: HTTP ${response.status}\`)`).src/synthorg/providers/management/service.py (2)
715-726:⚠️ Potential issue | 🟠 MajorRevalidate config capability after acquiring
self._lock.Line 715 checks
preset_nameandbase_urlbefore the critical section, but Line 717 re-reads the provider under the lock. A concurrentupdate_provider()can swap the preset or clear the base URL in between, and this method will still persistlocal_paramsonto the new config. Move the capability check into the locked section, or repeat it afterconfig = providers.get(name).🔒 Re-check the preset after loading the locked config
- await self._resolve_local_manager(name, capability="config") async with self._lock: providers = await self._config_resolver.get_provider_configs() config = providers.get(name) if config is None: msg = f"Provider {name!r} not found" @@ ) raise ProviderNotFoundError(msg) + preset = get_preset(config.preset_name) if config.preset_name else None + if preset is None or not preset.supports_model_config: + msg = f"Provider {name!r} does not support model config" + logger.warning( + PROVIDER_VALIDATION_FAILED, + provider=name, + error=msg, + ) + raise ProviderValidationError(msg) + if not config.base_url: + msg = f"Provider {name!r} has no base URL configured" + logger.warning( + PROVIDER_VALIDATION_FAILED, + provider=name, + error=msg, + ) + raise ProviderValidationError(msg) model_idx = next( (i for i, m in enumerate(config.models) if m.id == model_id), None, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/providers/management/service.py` around lines 715 - 726, The code currently validates preset_name/base_url before acquiring self._lock but reads the provider config inside the locked section, risking a race where update_provider() changes the config; to fix, revalidate the provider's "config" capability and the preset_name/base_url after fetching config under the lock (or move the original checks into the async with self._lock block) so that the local_params are only applied to the same config you just loaded; update logic around _resolve_local_manager, async with self._lock, providers = await self._config_resolver.get_provider_configs(), config = providers.get(name), and local_params to perform capability/preset/base_url checks while holding the lock.
731-739:⚠️ Potential issue | 🟡 MinorUse a dedicated not-found signal for missing models.
Raising
ProviderValidationErrorhere is what forces the controller to parse"not found"out ofstr(exc)to choose between 400 and 404. That coupling is brittle and can misclassify unrelated validation failures if the message text changes or a provider/model name contains that phrase. Raise a model-specific not-found exception, or at least a coded validation error, instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/providers/management/service.py` around lines 731 - 739, The current branch raises ProviderValidationError when model_idx is None which forces callers to parse the error text to infer a 404; instead introduce and raise a dedicated not-found exception (e.g., ProviderModelNotFound or ModelNotFoundError) from this location: replace the raise ProviderValidationError(msg) in the block that checks model_idx with raise ProviderModelNotFound(msg) (or a coded ValidationError subtype) and keep the existing logging call (PROVIDER_VALIDATION_FAILED, provider=name, model=model_id, error=msg); also add the new exception class where other provider errors are declared so the controller can catch it explicitly and map to 404.src/synthorg/api/controllers/providers.py (1)
322-328:⚠️ Potential issue | 🟠 MajorDon't silently discard
preset_nameon create.This makes a request with preset metadata look successful while creating a provider that can no longer resolve local-model capabilities, because
ProviderManagementService._resolve_local_manager()keys offconfig.preset_name. Either preserve the field or reject it with a 4xx that points callers at/providers/from-preset.As per coding guidelines, "Validate at system boundaries (user input, external APIs, config files)."🛡️ Fail fast instead of silently discarding the field
- # Strip preset_name from external requests -- only - # create_from_preset may set it (capability flag injection). - safe_data = data.model_copy(update={"preset_name": None}) + if data.preset_name is not None: + logger.warning( + API_VALIDATION_FAILED, + resource="provider", + name=data.name, + error="preset_name is only accepted by /providers/from-preset", + ) + raise ApiValidationError( + "Use /providers/from-preset when supplying preset_name", + ) try: - config = await app_state.provider_management.create_provider( - safe_data, - ) + config = await app_state.provider_management.create_provider(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/providers.py` around lines 322 - 328, The handler currently strips preset_name via safe_data = data.model_copy(update={"preset_name": None}) which silently accepts requests with preset metadata but loses it and breaks ProviderManagementService._resolve_local_manager; instead validate at the boundary: before calling data.model_copy or app_state.provider_management.create_provider, check if data.preset_name is set and return a 4xx (HTTP 400) response guiding callers to use /providers/from-preset (or document the correct flow), or alternatively only allow preset_name to be set via the create_from_preset flow; update the logic around safe_data, data.model_copy, and create_provider accordingly so preset_name is either preserved when legitimately set by create_from_preset or rejected with a clear client error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/dto_providers.py`:
- Around line 485-486: Update the docstring example for the model_name attribute
to use a neutral identifier: replace "llama3.2:1b" with "test-local-001:latest"
in the docstring that documents the model_name attribute in dto_providers (look
for the Attributes: block and the model_name example string).
In `@src/synthorg/providers/management/service.py`:
- Around line 339-343: The AuthType.NONE branch currently seeds models =
preset.default_models which prevents _maybe_discover_preset_models from probing
live; change the AuthType.NONE handling so it does not populate the local models
variable (use an empty tuple or None to force discovery) and only use
preset.default_models as a fallback after _maybe_discover_preset_models returns
no live models; update the code around the models variable and the call to
_maybe_discover_preset_models to ensure AuthType.NONE presets trigger live
discovery while preserving preset.default_models as a fallback.
In `@tests/unit/providers/management/test_local_models.py`:
- Around line 246-260: Add tests to ensure LocalModelParams rejects non-finite
numeric values: create a new test (e.g., test_non_finite_validation) that uses
pytest.raises(ValidationError) when constructing LocalModelParams with
float("nan"), float("inf"), and float("-inf") for fields num_ctx,
num_gpu_layers, num_threads, num_batch and repeat_penalty; reference the
LocalModelParams class and the repeat_penalty field so the assertions cover all
numeric fields and will fail if allow_inf_nan=False is missing from the model
ConfigDict.
In `@tests/unit/providers/management/test_service_local_models.py`:
- Around line 54-67: The fake_pull generator ignores its model_name argument so
the test doesn't verify the service passed the correct identifier; change the
setup so the manager call is asserted: either make fake_pull record the incoming
model_name (e.g., append to a captured list) and after collecting events assert
the captured value equals "test-model:7b", or assign mock_manager.pull_model =
AsyncMock(side_effect=fake_pull) and after iterating assert
mock_manager.pull_model.assert_awaited_with("test-model:7b"); reference
get_local_model_manager, mock_manager, fake_pull and service.pull_model to
locate the change.
In `@tests/unit/providers/test_presets.py`:
- Around line 113-136: Combine the three tests into a single parametrized pytest
using get_preset to assert the three capability flags per preset: replace
test_ollama_supports_local_model_management,
test_lm_studio_local_management_deferred, and
test_vllm_no_local_model_management with one test function (e.g.,
test_preset_local_management) decorated with `@pytest.mark.parametrize` that
iterates over tuples like ("ollama", True, True, True), ("lm-studio", False,
False, False), ("vllm", False, False, False) and inside call preset =
get_preset(name) then assert preset is not None and assert
preset.supports_model_pull is pull_expected, preset.supports_model_delete is
delete_expected, preset.supports_model_config is config_expected.
In `@web/src/api/endpoints/providers.ts`:
- Around line 133-147: The catch block around JSON.parse in the pull stream
handler incorrectly rethrows all Errors (including SyntaxError) so malformed
JSON frames abort the pull; update the catch to first detect parse failures
(e.g., err instanceof SyntaxError or err?.name === 'SyntaxError') and handle
them by resetting state.currentEvent, logging the malformed-JSON warning and
continuing (do not rethrow), while still rethrowing non-parse Errors; ensure
this behavior applies to the block handling parsed as PullProgressEvent and
calls to onProgress/state.currentEvent.
In `@web/src/pages/providers/ModelConfigDrawer.tsx`:
- Around line 36-45: The parsing helpers parseIntStrict and parseFloatStrict
accept whitespace-only strings because they check !val instead of trimming;
update both to trim the input first (e.g., const s = val.trim()) and treat empty
trimmed strings as null, then parse s with Number and keep the existing
Number.isFinite/Number.isInteger checks so whitespace-only input is no longer
interpreted as 0.
In `@web/src/pages/providers/ModelPullDialog.tsx`:
- Line 2: The AlertDialog import is using the wrong package; update the import
that currently references AlertDialog from 'radix-ui' to import AlertDialog from
the scoped Radix package '@radix-ui/react-alert-dialog' so the component
resolves correctly with Radix UI v1.4.3 (look for the AlertDialog import
statement to replace).
---
Duplicate comments:
In `@src/synthorg/api/controllers/providers.py`:
- Around line 322-328: The handler currently strips preset_name via safe_data =
data.model_copy(update={"preset_name": None}) which silently accepts requests
with preset metadata but loses it and breaks
ProviderManagementService._resolve_local_manager; instead validate at the
boundary: before calling data.model_copy or
app_state.provider_management.create_provider, check if data.preset_name is set
and return a 4xx (HTTP 400) response guiding callers to use
/providers/from-preset (or document the correct flow), or alternatively only
allow preset_name to be set via the create_from_preset flow; update the logic
around safe_data, data.model_copy, and create_provider accordingly so
preset_name is either preserved when legitimately set by create_from_preset or
rejected with a clear client error.
In `@src/synthorg/providers/management/service.py`:
- Around line 715-726: The code currently validates preset_name/base_url before
acquiring self._lock but reads the provider config inside the locked section,
risking a race where update_provider() changes the config; to fix, revalidate
the provider's "config" capability and the preset_name/base_url after fetching
config under the lock (or move the original checks into the async with
self._lock block) so that the local_params are only applied to the same config
you just loaded; update logic around _resolve_local_manager, async with
self._lock, providers = await self._config_resolver.get_provider_configs(),
config = providers.get(name), and local_params to perform
capability/preset/base_url checks while holding the lock.
- Around line 731-739: The current branch raises ProviderValidationError when
model_idx is None which forces callers to parse the error text to infer a 404;
instead introduce and raise a dedicated not-found exception (e.g.,
ProviderModelNotFound or ModelNotFoundError) from this location: replace the
raise ProviderValidationError(msg) in the block that checks model_idx with raise
ProviderModelNotFound(msg) (or a coded ValidationError subtype) and keep the
existing logging call (PROVIDER_VALIDATION_FAILED, provider=name,
model=model_id, error=msg); also add the new exception class where other
provider errors are declared so the controller can catch it explicitly and map
to 404.
In `@web/src/api/endpoints/providers.ts`:
- Around line 179-185: Replace the ad-hoc 401 cleanup that directly removes
localStorage keys in this fetch path with the app's shared unauthorized handler
so the auth store and redirect flow stay in sync; import and call the central
handler (e.g., handleUnauthorized or authStore.handleUnauthorized) when
response.status === 401 instead of removing 'auth_token',
'auth_token_expires_at', and 'auth_must_change_password' manually, then rethrow
the error as before (i.e., call the shared handler before throwing `new
Error(\`Pull failed: HTTP ${response.status}\`)`).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64dbca03-979b-4509-a9f9-fca8724a8687
📒 Files selected for processing (36)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/controllers/providers.pysrc/synthorg/api/dto_providers.pysrc/synthorg/config/schema.pysrc/synthorg/observability/events/api.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/management/_helpers.pysrc/synthorg/providers/management/local_models.pysrc/synthorg/providers/management/service.pysrc/synthorg/providers/presets.pytests/unit/providers/management/test_local_models.pytests/unit/providers/management/test_service_local_models.pytests/unit/providers/test_presets.pyweb/src/__tests__/pages/ProviderDetailPage.test.tsxweb/src/__tests__/pages/ProvidersPage.test.tsxweb/src/__tests__/stores/setup-wizard.test.tsweb/src/__tests__/utils/providers.test.tsweb/src/__tests__/utils/setup-validation.property.test.tsweb/src/__tests__/utils/setup-validation.test.tsweb/src/api/endpoints/providers.tsweb/src/api/types.tsweb/src/pages/ProviderDetailPage.tsxweb/src/pages/providers/ModelConfigDrawer.stories.tsxweb/src/pages/providers/ModelConfigDrawer.tsxweb/src/pages/providers/ModelPullDialog.stories.tsxweb/src/pages/providers/ModelPullDialog.tsxweb/src/pages/providers/PresetPicker.stories.tsxweb/src/pages/providers/PresetPicker.tsxweb/src/pages/providers/ProviderCard.stories.tsxweb/src/pages/providers/ProviderDetailHeader.tsxweb/src/pages/providers/ProviderFormModal.tsxweb/src/pages/providers/ProviderModelList.stories.tsxweb/src/pages/providers/ProviderModelList.tsxweb/src/pages/providers/TestConnectionResult.tsxweb/src/stores/providers.ts
| elif preset.auth_type == AuthType.NONE: | ||
| # Local providers: skip static LiteLLM DB, rely on live | ||
| # discovery in _maybe_discover_preset_models below. | ||
| models = preset.default_models | ||
| else: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don't let default_models short-circuit live discovery for no-auth presets.
_maybe_discover_preset_models() returns immediately when models is truthy. Seeding models = preset.default_models here means any AuthType.NONE preset with built-in models will skip the live probe and can fall back to stale static counts again. Pass an empty tuple into discovery and keep preset.default_models only as fallback if you still want seeded models.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/providers/management/service.py` around lines 339 - 343, The
AuthType.NONE branch currently seeds models = preset.default_models which
prevents _maybe_discover_preset_models from probing live; change the
AuthType.NONE handling so it does not populate the local models variable (use an
empty tuple or None to force discovery) and only use preset.default_models as a
fallback after _maybe_discover_preset_models returns no live models; update the
code around the models variable and the call to _maybe_discover_preset_models to
ensure AuthType.NONE presets trigger live discovery while preserving
preset.default_models as a fallback.
| def test_all_none_by_default(self) -> None: | ||
| params = LocalModelParams() | ||
| assert params.num_ctx is None | ||
| assert params.num_gpu_layers is None | ||
| assert params.num_threads is None | ||
| assert params.num_batch is None | ||
|
|
||
| def test_positive_validation(self) -> None: | ||
| from pydantic import ValidationError | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| LocalModelParams(num_ctx=0) # gt=0 | ||
|
|
||
| def test_gpu_layers_allows_zero(self) -> None: | ||
| params = LocalModelParams(num_gpu_layers=0) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cover the non-finite LocalModelParams cases too.
These tests never assert that the new numeric fields reject NaN/Inf, and repeat_penalty is still unpinned here. If the schema ever loses its non-finite guard, this file stays green while the API starts accepting invalid persisted model config values.
As per coding guidelines, "In all ConfigDict declarations, use allow_inf_nan=False to reject NaN/Inf in numeric fields at validation time."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/providers/management/test_local_models.py` around lines 246 - 260,
Add tests to ensure LocalModelParams rejects non-finite numeric values: create a
new test (e.g., test_non_finite_validation) that uses
pytest.raises(ValidationError) when constructing LocalModelParams with
float("nan"), float("inf"), and float("-inf") for fields num_ctx,
num_gpu_layers, num_threads, num_batch and repeat_penalty; reference the
LocalModelParams class and the repeat_penalty field so the assertions cover all
numeric fields and will fail if allow_inf_nan=False is missing from the model
ConfigDict.
| def test_ollama_supports_local_model_management(self) -> None: | ||
| """Ollama preset supports pull, delete, and config.""" | ||
| preset = get_preset("ollama") | ||
| assert preset is not None | ||
| assert preset.supports_model_pull is True | ||
| assert preset.supports_model_delete is True | ||
| assert preset.supports_model_config is True | ||
|
|
||
| def test_lm_studio_local_management_deferred(self) -> None: | ||
| """LM Studio preset flags are False until API stabilizes.""" | ||
| preset = get_preset("lm-studio") | ||
| assert preset is not None | ||
| assert preset.supports_model_pull is False | ||
| assert preset.supports_model_delete is False | ||
| assert preset.supports_model_config is False | ||
|
|
||
| def test_vllm_no_local_model_management(self) -> None: | ||
| """vLLM has no pull/delete/config API.""" | ||
| preset = get_preset("vllm") | ||
| assert preset is not None | ||
| assert preset.supports_model_pull is False | ||
| assert preset.supports_model_delete is False | ||
| assert preset.supports_model_config is False | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parameterizing the three local-capability tests to remove duplication.
The assertions are correct, but these cases can be expressed as one parametrized test for easier extension.
♻️ Suggested refactor
- def test_ollama_supports_local_model_management(self) -> None:
- """Ollama preset supports pull, delete, and config."""
- preset = get_preset("ollama")
- assert preset is not None
- assert preset.supports_model_pull is True
- assert preset.supports_model_delete is True
- assert preset.supports_model_config is True
-
- def test_lm_studio_local_management_deferred(self) -> None:
- """LM Studio preset flags are False until API stabilizes."""
- preset = get_preset("lm-studio")
- assert preset is not None
- assert preset.supports_model_pull is False
- assert preset.supports_model_delete is False
- assert preset.supports_model_config is False
-
- def test_vllm_no_local_model_management(self) -> None:
- """vLLM has no pull/delete/config API."""
- preset = get_preset("vllm")
- assert preset is not None
- assert preset.supports_model_pull is False
- assert preset.supports_model_delete is False
- assert preset.supports_model_config is False
+ `@pytest.mark.parametrize`(
+ ("name", "supports_pull", "supports_delete", "supports_config"),
+ [
+ ("ollama", True, True, True),
+ ("lm-studio", False, False, False),
+ ("vllm", False, False, False),
+ ],
+ )
+ def test_local_model_management_capabilities(
+ self,
+ name: str,
+ supports_pull: bool,
+ supports_delete: bool,
+ supports_config: bool,
+ ) -> None:
+ """Local model management capability flags match preset behavior."""
+ preset = get_preset(name)
+ assert preset is not None
+ assert preset.supports_model_pull is supports_pull
+ assert preset.supports_model_delete is supports_delete
+ assert preset.supports_model_config is supports_configAs per coding guidelines: "Prefer @pytest.mark.parametrize for testing similar cases."
📝 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.
| def test_ollama_supports_local_model_management(self) -> None: | |
| """Ollama preset supports pull, delete, and config.""" | |
| preset = get_preset("ollama") | |
| assert preset is not None | |
| assert preset.supports_model_pull is True | |
| assert preset.supports_model_delete is True | |
| assert preset.supports_model_config is True | |
| def test_lm_studio_local_management_deferred(self) -> None: | |
| """LM Studio preset flags are False until API stabilizes.""" | |
| preset = get_preset("lm-studio") | |
| assert preset is not None | |
| assert preset.supports_model_pull is False | |
| assert preset.supports_model_delete is False | |
| assert preset.supports_model_config is False | |
| def test_vllm_no_local_model_management(self) -> None: | |
| """vLLM has no pull/delete/config API.""" | |
| preset = get_preset("vllm") | |
| assert preset is not None | |
| assert preset.supports_model_pull is False | |
| assert preset.supports_model_delete is False | |
| assert preset.supports_model_config is False | |
| `@pytest.mark.parametrize`( | |
| ("name", "supports_pull", "supports_delete", "supports_config"), | |
| [ | |
| ("ollama", True, True, True), | |
| ("lm-studio", False, False, False), | |
| ("vllm", False, False, False), | |
| ], | |
| ) | |
| def test_local_model_management_capabilities( | |
| self, | |
| name: str, | |
| supports_pull: bool, | |
| supports_delete: bool, | |
| supports_config: bool, | |
| ) -> None: | |
| """Local model management capability flags match preset behavior.""" | |
| preset = get_preset(name) | |
| assert preset is not None | |
| assert preset.supports_model_pull is supports_pull | |
| assert preset.supports_model_delete is supports_delete | |
| assert preset.supports_model_config is supports_config |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/providers/test_presets.py` around lines 113 - 136, Combine the
three tests into a single parametrized pytest using get_preset to assert the
three capability flags per preset: replace
test_ollama_supports_local_model_management,
test_lm_studio_local_management_deferred, and
test_vllm_no_local_model_management with one test function (e.g.,
test_preset_local_management) decorated with `@pytest.mark.parametrize` that
iterates over tuples like ("ollama", True, True, True), ("lm-studio", False,
False, False), ("vllm", False, False, False) and inside call preset =
get_preset(name) then assert preset is not None and assert
preset.supports_model_pull is pull_expected, preset.supports_model_delete is
delete_expected, preset.supports_model_config is config_expected.
| @@ -0,0 +1,115 @@ | |||
| import { useState } from 'react' | |||
| import { AlertDialog } from 'radix-ui' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Radix UI version and import patterns used in the project
echo "=== package.json radix dependencies ==="
cat web/package.json | jq '.dependencies | with_entries(select(.key | test("radix")))'
echo -e "\n=== Existing radix imports in codebase ==="
rg -n "from ['\"](`@radix-ui`|radix-ui)" web/src --type ts --type tsx -g '*.tsx' -g '*.ts' | head -20Repository: Aureliolo/synthorg
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Check the actual import in ModelPullDialog.tsx
echo "=== ModelPullDialog.tsx imports (first 20 lines) ==="
head -20 web/src/pages/providers/ModelPullDialog.tsx | cat -n
echo -e "\n=== Search for `@radix-ui` imports in the codebase ==="
rg "@radix-ui" web/src --max-count 15Repository: Aureliolo/synthorg
Length of output: 1221
Fix the Radix UI import for AlertDialog.
The project uses Radix UI v1.4.3, which requires scoped imports. Line 2 should be import { AlertDialog } from '@radix-ui/react-alert-dialog' instead of from 'radix-ui'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/providers/ModelPullDialog.tsx` at line 2, The AlertDialog
import is using the wrong package; update the import that currently references
AlertDialog from 'radix-ui' to import AlertDialog from the scoped Radix package
'@radix-ui/react-alert-dialog' so the component resolves correctly with Radix UI
v1.4.3 (look for the AlertDialog import statement to replace).
- Replace 'llama3.2:1b' with 'test-local-001:latest' in PullModelRequest docstring (vendor-agnostic) - SSE parser: catch SyntaxError (JSON parse) separately from Error (intentional error events) to avoid aborting on malformed frames - ModelConfigDrawer: trim whitespace before Number() parse to prevent whitespace-only strings becoming 0 - Add parametrized test for LocalModelParams non-finite rejection (NaN, Inf, -Inf across all numeric fields) - Verify model_name argument passed to pull manager in service test
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/api/dto_providers.py (1)
407-433: 🧹 Nitpick | 🔵 TrivialConsider logging when preset lookup fails for a non-None preset_name.
If
config.preset_nameis set butget_preset()returnsNone(e.g., due to a typo or removed preset), the capabilities silently default toFalse. This may mask configuration errors.♻️ Optional: Add debug logging for unknown preset
+ from synthorg.observability import get_logger + logger = get_logger(__name__) + preset = get_preset(config.preset_name) if config.preset_name else None + if config.preset_name and preset is None: + logger.debug( + "preset_lookup_miss", + preset_name=config.preset_name, + msg="Unknown preset; capabilities will be False", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/dto_providers.py` around lines 407 - 433, When config.preset_name is provided but get_preset(config.preset_name) returns None the code silently falls back to False for capabilities; update the logic around get_preset and the preset variable to detect this case and emit a debug or warning log (including the requested preset name) before constructing the ProviderResponse (functions/variables: get_preset, config.preset_name, preset, ProviderResponse) so mis-typed or missing presets are visible in logs while preserving the existing fallback behavior.
♻️ Duplicate comments (2)
web/src/api/endpoints/providers.ts (1)
181-188:⚠️ Potential issue | 🟠 MajorMirror the shared 401 logout flow for consistency.
The 401 handling here only clears localStorage keys, but the axios interceptor in
client.ts(lines 40-55) also dynamically imports and callsuseAuthStore.getState().logout()to sync Zustand state. This inconsistency can leave the UI appearing logged in after a 401 during a pull operation.🔧 Suggested fix
if (!response.ok || !response.body) { if (response.status === 401) { localStorage.removeItem('auth_token') localStorage.removeItem('auth_token_expires_at') localStorage.removeItem('auth_must_change_password') + // Sync Zustand auth state -- mirrors apiClient interceptor behavior + import('@/stores/auth') + .then(({ useAuthStore }) => { + useAuthStore.getState().logout() + }) + .catch((importErr: unknown) => { + console.error('Auth store cleanup failed during 401 handling:', importErr) + }) } throw new Error(`Pull failed: HTTP ${response.status}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/api/endpoints/providers.ts` around lines 181 - 188, The 401 branch in the pull response handling currently only clears localStorage keys; update it to mirror the shared logout flow used in client.ts by also dynamically importing and calling useAuthStore.getState().logout() so Zustand auth state is cleared; keep the existing removals of 'auth_token', 'auth_token_expires_at', and 'auth_must_change_password', and ensure this logic is placed in the same 401 conditional inside the response handling in providers.ts so the UI state and storage are synchronized on 401.web/src/pages/providers/ModelConfigDrawer.tsx (1)
34-58: 🧹 Nitpick | 🔵 TrivialConsider adding validation feedback for invalid numeric input.
The strict parsing functions correctly reject invalid input (e.g., "12abc" →
null), but users receive no visual feedback when their input is silently discarded. An empty field and an invalid field both result innull, making it unclear whether the value was intentionally cleared or rejected.Consider tracking parse errors and using
InputField'serrorprop to display validation messages. This is a UX improvement rather than a bug, sincenullvalues are valid per theLocalModelParamsschema.♻️ Optional enhancement with validation errors
+ const [errors, setErrors] = useState<Record<string, string | null>>({}) + const handleSave = async () => { + const newErrors: Record<string, string | null> = {} const parseIntStrict = (val: string, field: string): number | null => { const s = val.trim() if (!s) return null const n = Number(s) - return Number.isFinite(n) && Number.isInteger(n) ? n : null + if (Number.isFinite(n) && Number.isInteger(n)) return n + newErrors[field] = 'Must be a valid integer' + return null } // ... similar for parseFloatStrict + setErrors(newErrors) + if (Object.values(newErrors).some(Boolean)) { + setSaving(false) + return + }Then pass
error={errors.numCtx}to eachInputField.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/providers/ModelConfigDrawer.tsx` around lines 34 - 58, The handleSave logic currently uses parseIntStrict/parseFloatStrict to turn invalid numeric strings into null without user feedback; update handleSave to collect per-field parse errors (e.g., errors.numCtx, errors.numGpuLayers, errors.numThreads, errors.numBatch, errors.repeatPenalty) when parseIntStrict/parseFloatStrict returns null for non-empty input, set those errors into component state, and prevent calling updateModelConfig until errors are cleared; then pass the corresponding error state into each InputField's error prop so users see validation messages; keep the newParams creation and updateModelConfig call but only proceed when there are no validation errors and still allow null for intentionally blank fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/providers/management/test_local_models.py`:
- Around line 246-251: The test test_all_none_by_default is missing an assertion
that the LocalModelParams.repeat_penalty field defaults to None; update the
test_all_none_by_default to include an assertion that params.repeat_penalty is
None so the test verifies all fields on LocalModelParams (e.g., num_ctx,
num_gpu_layers, num_threads, num_batch, repeat_penalty) are None by default.
- Around line 88-196: Add a new unit test that verifies
OllamaModelManager.pull_model handles httpx transport errors by yielding a
single terminal error event: create a test named test_pull_transport_error that
mocks httpx.AsyncClient.stream to raise an httpx.ConnectError (or
httpx.HTTPError) via MagicMock/side_effect, instantiate OllamaModelManager with
that client, collect events from manager.pull_model("test-model"), and assert
there's exactly one event with a non-None error and done is True; reference the
existing test helpers _make_stream_cm/_make_aiter and the class
OllamaModelManager to place the test alongside the other
TestOllamaModelManagerPull methods.
---
Outside diff comments:
In `@src/synthorg/api/dto_providers.py`:
- Around line 407-433: When config.preset_name is provided but
get_preset(config.preset_name) returns None the code silently falls back to
False for capabilities; update the logic around get_preset and the preset
variable to detect this case and emit a debug or warning log (including the
requested preset name) before constructing the ProviderResponse
(functions/variables: get_preset, config.preset_name, preset, ProviderResponse)
so mis-typed or missing presets are visible in logs while preserving the
existing fallback behavior.
---
Duplicate comments:
In `@web/src/api/endpoints/providers.ts`:
- Around line 181-188: The 401 branch in the pull response handling currently
only clears localStorage keys; update it to mirror the shared logout flow used
in client.ts by also dynamically importing and calling
useAuthStore.getState().logout() so Zustand auth state is cleared; keep the
existing removals of 'auth_token', 'auth_token_expires_at', and
'auth_must_change_password', and ensure this logic is placed in the same 401
conditional inside the response handling in providers.ts so the UI state and
storage are synchronized on 401.
In `@web/src/pages/providers/ModelConfigDrawer.tsx`:
- Around line 34-58: The handleSave logic currently uses
parseIntStrict/parseFloatStrict to turn invalid numeric strings into null
without user feedback; update handleSave to collect per-field parse errors
(e.g., errors.numCtx, errors.numGpuLayers, errors.numThreads, errors.numBatch,
errors.repeatPenalty) when parseIntStrict/parseFloatStrict returns null for
non-empty input, set those errors into component state, and prevent calling
updateModelConfig until errors are cleared; then pass the corresponding error
state into each InputField's error prop so users see validation messages; keep
the newParams creation and updateModelConfig call but only proceed when there
are no validation errors and still allow null for intentionally blank fields.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77223a6c-8665-4946-a4d4-06091fd587d2
📒 Files selected for processing (5)
src/synthorg/api/dto_providers.pytests/unit/providers/management/test_local_models.pytests/unit/providers/management/test_service_local_models.pyweb/src/api/endpoints/providers.tsweb/src/pages/providers/ModelConfigDrawer.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
🧰 Additional context used
📓 Path-based instructions (7)
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{tsx,ts}: ALWAYS reuse existing components fromweb/src/components/ui/before creating new ones (StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast, Skeleton, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, TagInput, MetadataGrid, ProjectStatusBadge, ContentTypeBadge)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success,bg-danger, etc.) or CSS variables (var(--so-accent)) for colors. NEVER hardcode hex values in.tsx/.tsfiles.
Usefont-sansorfont-monofor typography (maps to Geist tokens). NEVER setfontFamilydirectly.
Use density-aware tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing. NEVER hardcode pixel values for layout spacing.
Use token variables (var(--so-shadow-card-hover),border-border,border-bright) for shadows and borders. NEVER hardcode values.
Importcnfrom@/lib/utilsfor conditional class merging in new components.
Do NOT recreate status dots inline -- use<StatusBadge>component.
Do NOT build card-with-header layouts from scratch -- use<SectionCard>component.
Do NOT create metric displays withtext-metric font-bold-- use<MetricCard>component.
Do NOT render initials circles manually -- use<Avatar>component.
Do NOT create complex (>8 line) JSX inside.map()-- extract to a shared component.
TypeScript strict mode must pass type checking (runnpm --prefix web run type-check); all type errors must be resolved before proceeding.
Do NOT hardcode Framer Motion transition durations -- use@/lib/motionpresets.
Files:
web/src/pages/providers/ModelConfigDrawer.tsxweb/src/api/endpoints/providers.ts
web/src/**/*.{tsx,ts,css}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Do NOT use
rgba()with hardcoded values -- use design token variables.
Files:
web/src/pages/providers/ModelConfigDrawer.tsxweb/src/api/endpoints/providers.ts
web/src/**/*.{tsx,ts,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
ESLint must enforce zero warnings (run
npm --prefix web run lint); all warnings must be fixed before proceeding.
Files:
web/src/pages/providers/ModelConfigDrawer.tsxweb/src/api/endpoints/providers.ts
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations.
Useexcept A, B:(no parentheses) for PEP 758 except syntax on Python 3.14.
Type hints required on all public functions; mypy strict mode enforced.
Docstrings required on public classes and functions using Google style; enforced by ruff D rules.
Create new objects instead of mutating existing ones. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves (e.g. agent execution state, task progress). Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict). In allConfigDictdeclarations, useallow_inf_nan=Falseto rejectNaN/Infin numeric fields at validation time.
Use@computed_fieldfor derived values instead of storing and validating redundant fields (e.g.TokenUsage.total_tokens).
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length maximum is 88 characters, enforced by ruff.
Functions must be < 50 lines; files < 800 lines.
Handle errors explicitly; never silently swallow exceptions.
Validate at system boundaries (use...
Files:
tests/unit/providers/management/test_service_local_models.pysrc/synthorg/api/dto_providers.pytests/unit/providers/management/test_local_models.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowmarkers on test functions.
Global test timeout: 30 seconds per test (configured inpyproject.toml). Do not add per-filepytest.mark.timeout(30)markers; non-default overrides liketimeout(60)are allowed.
Prefer@pytest.mark.parametrizefor testing similar cases.
Tests must usetest-provider,test-small-001, etc. instead of real vendor names.
Use Hypothesis for property-based testing in Python:@given+@settings. Profiles configured intests/conftest.py:ci(deterministic,max_examples=10+derandomize=True— fixed seed per test),dev(1000 examples),fuzz(10,000 examples, no deadline). Controlled viaHYPOTHESIS_PROFILEenv var.
Files:
tests/unit/providers/management/test_service_local_models.pytests/unit/providers/management/test_local_models.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/providers/management/test_service_local_models.pytests/unit/providers/management/test_local_models.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__).
Never useimport logging/logging.getLogger()/print()in application code. Exceptions:observability/setup.py,observability/sinks.py,observability/syslog_handler.py, andobservability/http_handler.pymay use stdlibloggingandprint(..., file=sys.stderr)for handler construction, bootstrap, and error reporting.
Use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Always use structured kwargs in logging:logger.info(EVENT, key=value)— neverlogger.info("msg %s", val).
All error paths must log at WARNING or ERROR with context before raising.
All state transitions must log at INFO level.
DEBUG logging for object creation, internal flow, and entry/exit of key functions.
Retryable errors (is_retryable=True):RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError. Non-retryable errors raise immediately without retry.
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases.
Files:
src/synthorg/api/dto_providers.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/api/dto_providers.py
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/providers/**/*.py : Providers: LLM provider abstraction (LiteLLM adapter), auth types (api_key/oauth/custom_header/none), presets (PROVIDER_PRESETS), runtime CRUD (ProviderManagementService with asyncio.Lock serialization), hot-reload via AppState swap.
📚 Learning: 2026-04-01T14:22:06.315Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T14:22:06.315Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
web/src/pages/providers/ModelConfigDrawer.tsx
📚 Learning: 2026-04-03T10:54:21.472Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-04-03T10:54:21.472Z
Learning: Applies to web/src/**/*.{tsx,ts} : Use density-aware tokens (`p-card`, `gap-section-gap`, `gap-grid-gap`) or standard Tailwind spacing. NEVER hardcode pixel values for layout spacing.
Applied to files:
web/src/pages/providers/ModelConfigDrawer.tsx
📚 Learning: 2026-03-27T22:32:26.927Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T22:32:26.927Z
Learning: Applies to web/src/**/*.{tsx,ts} : Use density-aware tokens (p-card, gap-section-gap, gap-grid-gap) or standard Tailwind spacing; never hardcode pixel values for layout spacing
Applied to files:
web/src/pages/providers/ModelConfigDrawer.tsx
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to src/synthorg/providers/**/*.py : Providers: LLM provider abstraction (LiteLLM adapter), auth types (api_key/oauth/custom_header/none), presets (PROVIDER_PRESETS), runtime CRUD (ProviderManagementService with asyncio.Lock serialization), hot-reload via AppState swap.
Applied to files:
tests/unit/providers/management/test_service_local_models.pysrc/synthorg/api/dto_providers.py
📚 Learning: 2026-04-02T21:38:30.127Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T21:38:30.127Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Handle errors explicitly; never silently swallow exceptions
Applied to files:
web/src/api/endpoints/providers.ts
📚 Learning: 2026-03-31T21:07:37.469Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T21:07:37.469Z
Learning: Applies to src/synthorg/providers/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-04-03T16:11:48.610Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T16:11:48.610Z
Learning: Applies to src/synthorg/providers/**/*.py : `RetryConfig` and `RateLimiterConfig` are set per-provider in `ProviderConfig`.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-03-20T11:18:48.128Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T11:18:48.128Z
Learning: Applies to src/synthorg/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-04-03T16:11:48.610Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T16:11:48.610Z
Learning: Applies to src/synthorg/**/*.py : Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names: `example-provider`, `example-large-001`, `example-medium-001`, `example-small-001`, `large`/`medium`/`small` as aliases.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-04-01T15:36:39.993Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T15:36:39.993Z
Learning: Applies to {src/synthorg/**/*.py,tests/**/*.py,web/src/**/*.{ts,tsx}} : NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples — use generic names: `example-provider`, `example-large-001`, `example-medium-001`, `example-small-001`, `large`/`medium`/`small`
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-04-01T15:36:39.993Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T15:36:39.993Z
Learning: Applies to {src,tests}/**/*.py : Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names: `example-provider`, `example-large-001`, `example-medium-001`, `example-small-001`, `large`/`medium`/`small` as aliases
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to {src,tests,web,cli,site}/**/*.{py,ts,tsx,go,astro} : Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names: example-provider, example-large-001, example-medium-001, example-small-001. Vendor names may only appear in: (1) Operations design page provider list (docs/design/operations.md), (2) .claude/ skill/agent files, (3) third-party import paths/module names.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 BaseModel, model_validator, computed_field, ConfigDict.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-03-15T18:42:17.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:42:17.990Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-04-03T16:11:48.610Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T16:11:48.610Z
Learning: Applies to **/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). In all `ConfigDict` declarations, use `allow_inf_nan=False` to reject `NaN`/`Inf` in numeric fields at validation time.
Applied to files:
src/synthorg/api/dto_providers.pytests/unit/providers/management/test_local_models.py
📚 Learning: 2026-04-02T11:49:21.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T11:49:21.202Z
Learning: Applies to **/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`) with `allow_inf_nan=False` in all `ConfigDict` declarations
Applied to files:
src/synthorg/api/dto_providers.pytests/unit/providers/management/test_local_models.py
📚 Learning: 2026-03-16T19:13:36.562Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:13:36.562Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/dto_providers.py
📚 Learning: 2026-04-01T15:36:39.993Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T15:36:39.993Z
Learning: Applies to src/**/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `allow_inf_nan=False` in all `ConfigDict` declarations to reject `NaN`/`Inf` in numeric fields at validation time
Applied to files:
src/synthorg/api/dto_providers.pytests/unit/providers/management/test_local_models.py
🔇 Additional comments (29)
web/src/api/endpoints/providers.ts (4)
118-121: LGTM!The
encodeModelIdPathhelper correctly preserves/delimiters for wildcard path params while encoding individual segments, enabling model IDs likellama3:latestororg/modelto be safely included in URL paths.
123-153: LGTM!The SSE parsing logic correctly:
- Calls
onProgress(parsed)before throwing on error events (line 138), ensuring the store receives terminal error payloads- Handles
SyntaxErrorseparately (lines 144-147), allowing malformed JSON frames to be logged and skipped without aborting the stream- Rethrows other
Errorinstances to propagate intentional errors
225-230: LGTM!The
deleteModelfunction correctly usesencodeModelIdPathfor model IDs containing/and follows the same pattern asdeleteProvider.
232-242: LGTM!The
updateModelConfigfunction correctly wraps parameters as{ local_params: params }, matching theUpdateModelConfigRequestschema expected by the backend controller.web/src/pages/providers/ModelConfigDrawer.tsx (3)
1-14: LGTM!Clean imports reusing existing UI components, and the props interface correctly allows
modelto be null for the closed drawer state.
60-107: LGTM!The form follows coding guidelines: uses
gap-section-gapdensity token, reusesInputFieldandButtoncomponents, and properly handles the saving state with disabled buttons and loading text.
109-126: LGTM!The drawer wrapper correctly uses
key={model.id}to reset form state when switching between models, and conditionally renders the form only when a model is selected.src/synthorg/api/dto_providers.py (7)
12-15: LGTM!Import additions for
LocalModelParamsandProviderModelConfigare correctly placed with thenoqa: TC001annotation for runtime type checking needs.
68-71: LGTM!The
local_paramsfield is properly typed as optional with a sensibleNonedefault, allowing models without local launch parameters to remain unaffected.
165-165: LGTM!The
preset_namefield correctly usesNotBlankStr | Nonefor optional preset association.
305-308: LGTM!The capability flags default to
False, which is the safe and correct default for providers without explicit capability declarations.
463-463: LGTM!The
local_paramsfield is correctly passed through from the model config to the response.
479-505: LGTM!The
PullModelRequestDTO has proper validation:
- Regex pattern covers common model identifier formats (tags, org prefixes, versions)
NotBlankStrensures non-empty inputmax_length=256provides reasonable bounds- Docstring example uses a vendor-agnostic identifier
508-517: LGTM!The
UpdateModelConfigRequestcorrectly requireslocal_paramsas a mandatory field, ensuring the update always has configuration data.tests/unit/providers/management/test_local_models.py (5)
1-17: LGTM!File setup is correct with proper imports, unit test marker applied at module level, and vendor-agnostic test identifiers.
20-63: LGTM!Comprehensive test coverage for
PullProgressEvent:
- Immutability (frozen model)
- Default values
- Terminal-error invariant (error requires
done=True)- Bytes ordering invariant (
completed_bytes <= total_bytes)
66-85: LGTM!Factory tests cover all expected cases: Ollama returns a manager, LM Studio/vLLM return
None(not yet implemented), and unknown/Nonepresets safely returnNone.
199-235: LGTM!Delete tests verify correct request construction (method, URL, JSON body, timeout) and proper error handling for 404 responses.
263-278: The review comment is based on a misunderstanding.num_batchis typed asint | None, notfloat. Theallow_inf_nan=FalseConfigDict setting applies specifically to float field validation; integer fields cannot holdNaN/Infby definition. The test already covers integer field type coercion failure vianum_ctx,num_gpu_layers, andnum_threads. Addingnum_batchwould not meaningfully testallow_inf_nan=False— it would only duplicate existing int field behavior. The float fieldrepeat_penaltyis the only field whereallow_inf_nan=Falsevalidation actually applies, and it is already tested.> Likely an incorrect or invalid review comment.tests/unit/providers/management/test_service_local_models.py (10)
1-17: LGTM!File setup is correct with proper imports and unit test marker. Uses vendor-agnostic provider and model names.
22-38: LGTM!The helper method cleanly encapsulates Ollama provider creation with mocked discovery, avoiding code duplication across tests.
40-74: LGTM!Test correctly verifies that
pull_modeldelegates to the local model manager with the correct model name, and that events are properly yielded.
76-90: LGTM!Test verifies that providers without pull capability (no
preset_nameor unsupported preset) correctly raiseProviderValidationError. This confirms capability gating happens at the service layer.
92-115: LGTM!Test correctly verifies that
delete_model:
- Delegates to the local model manager with the correct model ID
- Triggers model discovery refresh afterward
117-136: LGTM!Test verifies that
update_model_configcorrectly persistsLocalModelParamson the target model configuration.
138-152: LGTM!Test correctly verifies that updating configuration for a nonexistent model raises
ProviderValidationError.
155-194: LGTM!Comprehensive tests for the bug fix ensuring local presets (auth_type=NONE) skip
models_from_litellm:
- Parametrized test covers all local presets (ollama, lm-studio, vllm)
- Verifies models come from discovery instead
- Patch target correctly references the service module's import
196-217: LGTM!Test correctly verifies that cloud presets (with auth_type != NONE) continue using
models_from_litellmfor model population.
219-243: LGTM!Test correctly verifies that user-provided models bypass both
models_from_litellmanddiscover_models, ensuring explicit configuration is honored.
- Assert repeat_penalty is None in test_all_none_by_default - Add test_pull_transport_error: httpx.ConnectError yields terminal error event with done=True and error message
🤖 I have created a release *beep* *boop* --- ## [0.6.0](v0.5.9...v0.6.0) (2026-04-03) ### Features * dashboard UI for ceremony policy settings ([#1038](#1038)) ([865554c](865554c)), closes [#979](#979) * implement tool-based memory retrieval injection strategy ([#1039](#1039)) ([329270e](329270e)), closes [#207](#207) * local model management for Ollama and LM Studio ([#1037](#1037)) ([e1b14d3](e1b14d3)), closes [#1030](#1030) * workflow execution -- instantiate tasks from WorkflowDefinition ([#1040](#1040)) ([e9235e3](e9235e3)), closes [#1004](#1004) ### Maintenance * shared Hypothesis failure DB + deterministic CI profile ([#1041](#1041)) ([901ae92](901ae92)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Local providers (Ollama, LM Studio) have management APIs that SynthOrg didn't use. The setup wizard showed wrong model counts (LiteLLM static DB vs live discovery), and there was no way to download, delete, or configure local models from the dashboard.
Bug fix
auth_type=NONE(Ollama, LM Studio, vLLM), skipmodels_from_litellm()and always use live discovery. The static DB returned stale/wrong models (Ollama got old entries; LM Studio/vLLM got OpenAI cloud models vialitellm_provider="openai").Backend
supports_model_pull,supports_model_delete,supports_model_configonProviderPreset(Ollama: all true; LM Studio/vLLM: deferred)LocalModelParams: Per-model launch parameters (num_ctx,num_gpu_layers,num_threads,num_batch,repeat_penalty) stored onProviderModelConfigpreset_nameonProviderConfigfor capability flag resolution in API responsesOllamaModelManager: Pull via streaming/api/pull(newline-delimited JSON), delete via/api/delete, withLocalModelManagerprotocolpull_model(async generator),delete_model(with auto-refresh),update_model_config(immutable update under lock)POST /{name}/models/pull(SSE streaming),DELETE /{name}/models/{model_id}(204),PUT /{name}/models/{model_id}/config(returns updated model)Dashboard
supports_model_pull/delete/config)Documentation
Test plan
Visual testing
curl -X POST localhost:3001/api/v1/providers/ollama/models/pull -H "Content-Type: application/json" -d '{"model_name":"llama3.2:1b"}' -NReview coverage
Pre-reviewed by 8 agents (code-reviewer, conventions-enforcer, frontend-reviewer, silent-failure-hunter, type-design-analyzer, api-contract-drift, docs-consistency, issue-resolution-verifier). 22 findings identified and fixed.
Closes #1030