feat: auto-select embedding model + fine-tuning pipeline wiring#999
feat: auto-select embedding model + fine-tuning pipeline wiring#999
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (33)
WalkthroughAdds embedding-model auto-selection and override plumbing and an offline fine-tuning scaffold. New package Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. OpenSSF ScorecardScorecard details
Scanned Files
|
There was a problem hiding this comment.
Code Review
This pull request introduces a framework for embedding model selection and fine-tuning, featuring an LMEB-ranked auto-selection system that optimizes model choice based on deployment tiers. It adds a MemoryAdminController for pipeline control, updates the Mem0 backend to support fine-tuned checkpoints, and implements a priority-based configuration resolution chain. Feedback identifies critical syntax errors in exception handling across several files where multiple exceptions are caught without using a tuple. Additionally, the reviewer pointed out that the auto-selection logic incorrectly assigns model identifiers to provider fields, which will likely cause initialization failures in the Mem0 backend.
| "model": model_sv.value, | ||
| "dims": int(dims_sv.value) if dims_sv.value else None, | ||
| } | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
This syntax for catching multiple exceptions is invalid in Python 3. It will treat RecursionError as the name of the variable holding the caught MemoryError instance, and it won't catch RecursionError at all. Use a tuple to catch multiple exceptions.
| except MemoryError, RecursionError: | |
| except (MemoryError, RecursionError): |
| str(model.id) for pc in configs.values() for model in pc.models | ||
| ] | ||
| return tuple(ids) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
This syntax for catching multiple exceptions is invalid in Python 3. It will treat RecursionError as the name of the variable holding the caught MemoryError instance. Use a tuple instead.
except (MemoryError, RecursionError):| settings_svc=settings_svc, | ||
| available_model_ids=model_ids, | ||
| ) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
This syntax for catching multiple exceptions is invalid in Python 3. Use a tuple to catch both MemoryError and RecursionError correctly.
except (MemoryError, RecursionError):| await settings_svc.set( | ||
| "memory", | ||
| "embedder_provider", | ||
| ranking.model_id, | ||
| ) |
There was a problem hiding this comment.
The embedder_provider setting is being set to the model ID (ranking.model_id). In the Mem0 adapter, the provider field usually expects a provider name (e.g., "ollama", "openai", "huggingface"), not the model identifier itself. Setting the provider to the model name will likely cause initialization to fail unless a provider with that exact name is registered. The auto-selection logic should ideally track which provider offered the selected model.
| if ranking is None: | ||
| ranking = select_embedding_model(available_models) | ||
| if ranking is not None: | ||
| auto_provider = ranking.model_id |
There was a problem hiding this comment.
Setting auto_provider to ranking.model_id is likely incorrect. The provider field in Mem0EmbedderConfig (and the corresponding setting) expects a provider name like "ollama" or "openai". Using the model ID as the provider name will cause the Mem0 backend to look for a non-existent provider.
| if provider is None: | ||
| provider = model |
There was a problem hiding this comment.
This fallback assumes that the provider name is the same as the model name if the provider is not specified. For the Mem0 backend, this is generally not true (e.g., for Ollama, the provider is "ollama" regardless of the model). This may lead to configuration errors at runtime.
There was a problem hiding this comment.
Pull request overview
This PR integrates LMEB-based embedding model auto-selection into SynthOrg’s memory subsystem and wires fine-tuning checkpoint handling into the Mem0 adapter, along with admin API stubs and settings/template support.
Changes:
- Adds LMEB ranking data + selector/resolver to auto-pick an embedding model with tier inference and override priority (settings > YAML > auto).
- Wires fine-tune checkpoint path selection into Mem0 config generation and adds fine-tuning pipeline/API stubs.
- Extends settings, templates, docs, and tests to cover new configuration surfaces and behaviors.
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds fine-tune optional dependency lock entries (torch, sentence-transformers, and transitive deps). |
| pyproject.toml | Introduces fine-tune optional dependency group. |
| docs/reference/embedding-evaluation.md | Updates reference behavior for checkpoint handling and admin trigger endpoint. |
| docs/design/memory.md | Updates design spec to reflect implemented checkpoint wiring and admin trigger. |
| CLAUDE.md | Updates repo structure docs to mention new memory/embedding/ area. |
| tests/unit/memory/test_init.py | Verifies new EmbedderOverrideConfig is exported. |
| tests/unit/memory/test_config.py | Adds unit tests for EmbedderOverrideConfig and CompanyMemoryConfig.embedder. |
| tests/unit/memory/embedding/test_selector.py | Adds tests for LMEB model selection + tier inference. |
| tests/unit/memory/embedding/test_resolve.py | Adds tests for override priority chain and auto-selection behavior. |
| tests/unit/memory/embedding/test_rankings.py | Adds integrity tests for LMEB ranking dataset. |
| tests/unit/memory/embedding/test_fine_tune.py | Adds tests for fine-tune stage stubs input validation + NotImplemented behavior. |
| tests/unit/memory/embedding/init.py | Establishes test package namespace for embedding tests. |
| tests/unit/memory/backends/mem0/test_config.py | Updates tests for enabled fine-tune config and checkpoint model override behavior. |
| tests/unit/api/controllers/test_setup_embedding.py | Adds tests for setup-time embedder auto-selection persistence to settings. |
| tests/unit/api/controllers/test_memory_admin.py | Adds tests for fine-tune request/status models and controller existence. |
| src/synthorg/templates/schema.py | Adds memory override dictionary on CompanyTemplate schema. |
| src/synthorg/settings/definitions/memory.py | Registers advanced settings for embedder provider/model/dims overrides. |
| src/synthorg/observability/events/memory.py | Adds event constants for embedder selection and fine-tune/admin flows. |
| src/synthorg/memory/embedding/selector.py | Implements LMEB intersection-based model selection + deployment tier inference. |
| src/synthorg/memory/embedding/resolve.py | Implements embedder config resolution priority chain and LMEB auto-selection. |
| src/synthorg/memory/embedding/rankings.py | Adds frozen Pydantic LMEB ranking dataset and tier enum. |
| src/synthorg/memory/embedding/fine_tune.py | Adds fine-tuning pipeline stage stubs with validation + status enum. |
| src/synthorg/memory/embedding/fine_tune_models.py | Adds request/status Pydantic models for fine-tune API. |
| src/synthorg/memory/embedding/init.py | Exposes embedding selection/resolution public API from the package. |
| src/synthorg/memory/config.py | Adds EmbedderOverrideConfig and CompanyMemoryConfig.embedder. |
| src/synthorg/memory/backends/mem0/config.py | Adds checkpoint-path-as-model wiring when fine-tune is enabled; updates fine-tune config docs/validation. |
| src/synthorg/memory/init.py | Re-exports EmbedderOverrideConfig from synthorg.memory. |
| src/synthorg/api/controllers/setup.py | Wires setup completion to collect model ids and call embedder auto-selection (best-effort). |
| src/synthorg/api/controllers/setup_helpers.py | Implements auto_select_embedder() that persists selection into settings. |
| src/synthorg/api/controllers/memory.py | Adds MemoryAdminController with fine-tune + status + embedder endpoints (stub behavior). |
| src/synthorg/api/controllers/init.py | Registers/exposes MemoryAdminController. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from synthorg.observability import get_logger | ||
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
| # Provider preset names that indicate local/self-hosted deployment. |
There was a problem hiding this comment.
logger = get_logger(__name__) is declared but never used in this module, which will trip Ruff F841 (pyflakes) and fail linting. Either remove the logger import/assignment or add a log statement that uses it (e.g., when no match is found or when a model is selected).
| if provider is None: | ||
| provider = model | ||
|
|
||
| return Mem0EmbedderConfig( | ||
| provider=provider, | ||
| model=model, | ||
| dims=dims, | ||
| ) |
There was a problem hiding this comment.
resolve_embedder_config() defaults provider to model when no provider override is supplied. This conflicts with Mem0EmbedderConfig, where provider is the Mem0 embedder provider identifier and model is the model identifier; using a model id as the provider is likely to produce an invalid Mem0 config. Consider requiring an explicit provider (from settings/YAML), or deriving it from the connected provider context (e.g., provider preset) instead of falling back to model.
| logger.info( | ||
| MEMORY_EMBEDDER_AUTO_SELECTED, | ||
| model_id=ranking.model_id, | ||
| tier=tier.value, | ||
| overall_score=ranking.overall, | ||
| dims=ranking.output_dims, | ||
| ) | ||
| await settings_svc.set( | ||
| "memory", | ||
| "embedder_provider", | ||
| ranking.model_id, | ||
| ) | ||
| await settings_svc.set( | ||
| "memory", | ||
| "embedder_model", | ||
| ranking.model_id, | ||
| ) |
There was a problem hiding this comment.
auto_select_embedder() persists embedder_provider as ranking.model_id, but embedder_provider is defined as the embedding provider override (and Mem0EmbedderConfig.provider is the Mem0 provider id). This will save a model name into the provider setting and likely misconfigure the Mem0 embedder. Suggest persisting the actual provider identifier (e.g. provider_preset_name, or a resolved Mem0 provider id) to embedder_provider, and only persisting ranking.model_id to embedder_model/dims.
| """Extract model IDs from provider configs for embedding selection. | ||
|
|
||
| Best-effort: returns an empty tuple if config resolver is not | ||
| available or provider configs cannot be read. | ||
| """ | ||
| if not app_state.has_config_resolver: | ||
| return () | ||
| try: | ||
| configs = await app_state.config_resolver.get_provider_configs() | ||
| ids: list[str] = [ | ||
| str(model.id) for pc in configs.values() for model in pc.models | ||
| ] | ||
| return tuple(ids) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
_collect_model_ids() gathers model ids from ProviderConfig.models, which is documented in src/synthorg/config/schema.py as "LLM model" configuration. If embedder auto-selection is intended to use embedding models (LMEB rankings), this source is likely wrong unless provider configs also include embedding models. Consider using a dedicated embedding-model discovery mechanism (or filtering for embedding-capable models) so auto-selection can actually find LMEB matches.
| """Extract model IDs from provider configs for embedding selection. | |
| Best-effort: returns an empty tuple if config resolver is not | |
| available or provider configs cannot be read. | |
| """ | |
| if not app_state.has_config_resolver: | |
| return () | |
| try: | |
| configs = await app_state.config_resolver.get_provider_configs() | |
| ids: list[str] = [ | |
| str(model.id) for pc in configs.values() for model in pc.models | |
| ] | |
| return tuple(ids) | |
| except MemoryError, RecursionError: | |
| """Extract embedding model IDs from provider configs for embedder selection. | |
| Best-effort: returns an empty tuple if config resolver is not | |
| available or provider configs cannot be read. | |
| The function prefers any embedding-specific collection on the provider | |
| config (e.g. ``embedding_models``) when available. If such a | |
| collection is not present, it falls back to filtering ``models`` for | |
| identifiers that look like embedding models. | |
| """ | |
| if not app_state.has_config_resolver: | |
| return () | |
| try: | |
| configs = await app_state.config_resolver.get_provider_configs() | |
| ids: list[str] = [] | |
| for pc in configs.values(): | |
| # Prefer a dedicated embedding model collection if available. | |
| embedding_models = getattr(pc, "embedding_models", None) | |
| if embedding_models is not None: | |
| candidate_models = embedding_models | |
| filter_by_name = False | |
| else: | |
| candidate_models = getattr(pc, "models", ()) or () | |
| # When we only have generic models, filter by name so we | |
| # don't pass obviously non-embedding LLMs to the selector. | |
| filter_by_name = True | |
| for model in candidate_models: | |
| model_id = getattr(model, "id", None) | |
| if model_id is None: | |
| continue | |
| model_id_str = str(model_id) | |
| if not model_id_str: | |
| continue | |
| if filter_by_name: | |
| lower_id = model_id_str.lower() | |
| if "embed" not in lower_id and "embedding" not in lower_id: | |
| continue | |
| ids.append(model_id_str) | |
| return tuple(ids) | |
| except (MemoryError, RecursionError): |
| ML dependencies (torch, sentence-transformers) are optional and | ||
| guarded with ``ImportError`` messages pointing to the ``fine-tune`` | ||
| extra. Actual training logic is not yet implemented -- functions | ||
| validate inputs and raise ``NotImplementedError``. |
There was a problem hiding this comment.
Module docstring says ML deps are "guarded with ImportError messages pointing to the fine-tune extra", but the stage functions currently don’t attempt any optional imports and instead always raise NotImplementedError after validation. Either adjust the docstring to match the current behavior, or add the described ImportError guards when the extra isn’t installed.
| ML dependencies (torch, sentence-transformers) are optional and | |
| guarded with ``ImportError`` messages pointing to the ``fine-tune`` | |
| extra. Actual training logic is not yet implemented -- functions | |
| validate inputs and raise ``NotImplementedError``. | |
| ML dependencies (torch, sentence-transformers) are optional, and the | |
| actual training logic is not yet implemented. At present, these | |
| functions only validate inputs and then raise ``NotImplementedError``. |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 37-38: The fine-tune optional dependencies currently use open
ranges; change them to exact pinned versions to follow the repo policy by
replacing "torch>=2.6" and "sentence-transformers>=4.0" with exact pins (e.g.,
torch==2.6.0 and sentence-transformers==4.0.0) or the exact versions recorded in
your lockfile/CI matrix; update the dependency entries for "torch" and
"sentence-transformers" in the fine-tune optional-dependencies list accordingly.
In `@src/synthorg/api/controllers/memory.py`:
- Around line 59-70: The POST /fine-tune handler currently always returns
FineTuneStage.FAILED (using ApiResponse and FineTuneStatus), so replace the
hardcoded failure with code that enqueues/starts the actual fine-tune pipeline
(e.g., call a pipeline starter like start_fine_tune_pipeline or
enqueue_fine_tune_job with the request payload), return a success status
(FineTuneStage.INITIATED or FineTuneStage.RUNNING) and an ApiResponse (HTTP 202
Accepted) containing the job id or tracking info; on exceptions catch the error,
log it, and return FineTuneStage.FAILED with the error message. Ensure you
reference and update the response construction around ApiResponse,
FineTuneStatus, and FineTuneStage rather than leaving the static error string.
- Around line 120-122: The except block that currently reads "except
MemoryError, RecursionError: raise" should log contextual details before
re-raising: in the except handler for MemoryError and RecursionError inside
src/synthorg/api/controllers/memory.py, call the module logger (e.g., logger or
logging.getLogger(__name__)) to emit a WARNING or ERROR containing context
(function name, relevant request/user IDs or object identifiers available in
that scope) and include exception traceback via exc_info=True, then re-raise the
exception; update the handler for the exact exception tuple (MemoryError,
RecursionError) so the log runs immediately before the raise.
In `@src/synthorg/api/controllers/setup_helpers.py`:
- Around line 678-692: The code sets "embedder_provider" to ranking.model_id
instead of the provider name; update the selection flow so auto_select_embedder
(or its caller) returns or preserves the provider name and use that when calling
settings_svc.set for "memory", "embedder_provider" (e.g., set to provider_name
variable rather than ranking.model_id). Specifically, modify
auto_select_embedder/EmbeddingModelRanking usage to include provider (or accept
an explicit provider parameter), then replace the first settings_svc.set call to
use that provider identifier, keep "embedder_model" as ranking.model_id and
"embedder_dims" as str(ranking.output_dims).
In `@src/synthorg/memory/backends/mem0/config.py`:
- Around line 250-257: When embedder.fine_tune (ft) is present and ft.enabled is
True, validate that ft.checkpoint_path exists before returning it in
_resolve_effective_model; if the path is missing, raise a clear ValueError or
log an error and abort so the Mem0 SDK is not given a non-existent path. You can
add this check either inside
EmbeddingFineTuneConfig._validate_required_when_enabled (validate
os.path.exists(ft.checkpoint_path) and raise) or immediately in the branch in
_resolve_effective_model/build_mem0_config_dict where ft.checkpoint_path is
returned, using a descriptive error message referencing checkpoint_path and
base_model.
In `@src/synthorg/memory/embedding/fine_tune.py`:
- Around line 77-83: Before each raise of NotImplementedError that constructs
the msg (e.g., the "Synthetic training data generation is not yet
implemented..." message and the other stage-specific messages), add a logging
call that records the failure context at WARNING or ERROR level (for example
logger.warning(...) or logger.error(...)) including the msg and any relevant
local variables/state; then raise the same NotImplementedError(msg). Apply this
pattern to every stage function that currently builds a msg and immediately
raises (the occurrences that build msg and do raise NotImplementedError) so all
error paths log context before raising.
- Around line 43-45: Replace the hard-coded event name string in the
logger.warning call inside src/synthorg/memory/embedding/fine_tune.py with the
domain event constant imported from synthorg.observability.events.memory; add an
import for the appropriate event constant (the memory fine-tune validation
failure event) from synthorg.observability.events.memory and pass that constant
to logger.warning (keeping the field=name argument unchanged) so the code uses
the canonical domain event identifier instead of the literal
"memory.fine_tune.validation_failed".
In `@src/synthorg/memory/embedding/rankings.py`:
- Around line 99-105: The file exposes real vendor/model names in comments and
as model_id values; replace them with project-safe aliases (e.g.,
test-provider/* style aliases) and add a small mapping/resolution layer so the
code references aliases only. Specifically: replace the literal model
identifiers in the constants and comments with aliases and create a
MODEL_ALIAS_MAP and a resolve_model_id(alias) helper (or use an existing
get_embedding_dimensions or similar symbol) that looks up the real model id from
environment/config (not checked into the repo); update all usages in rankings.py
(and the block spanning the earlier 109-179 region) to call
resolve_model_id(alias) where model_id is required and update comments to list
only aliases and their embedding dimensions. Ensure no real vendor names remain
in repo-owned comments, constants, tests, or default config.
In `@src/synthorg/memory/embedding/resolve.py`:
- Around line 52-148: The function resolve_embedder_config is too large; split
its responsibilities into small helpers: extract tier inference into a helper
(e.g., infer_tier_from(provider_preset_name, has_gpu) calling
infer_deployment_tier), extract the LMEB auto-selection logic (calling
select_embedding_model) into select_auto_embedder(available_models, tier) that
returns (auto_provider, auto_model, auto_dims) and logs
MEMORY_EMBEDDER_AUTO_SELECTED, and extract the final validation/normalization
into finalize_embedder_config(provider, model, dims) which raises
MemoryConfigError and returns the Mem0EmbedderConfig; then make
resolve_embedder_config orchestrate these helpers and reuse _merge_override for
YAML and settings overrides.
In `@src/synthorg/memory/embedding/selector.py`:
- Around line 72-75: The docstring for the parameter provider_preset_name is
inconsistent with the implementation: it currently says unknown names default to
GPU_CONSUMER but the code returns GPU_FULL for non-local providers (see the
GPU_FULL return in the function handling provider_preset_name). Update the
docstring to state that non-local/unknown provider names default to GPU_FULL
(referencing GPU_FULL and GPU_CONSUMER) OR change the function to return
GPU_CONSUMER for unknown providers; pick one approach and make the docstring and
the implementation in the provider_preset_name handling function consistent.
- Around line 55-60: The matching logic in the loop using available_lower and
ranking.model_id is too permissive because it checks both "ranking_id_lower in
available" and "available in ranking_id_lower", which lets short discovered tags
(e.g., "e5") match unrelated entries; update the condition to only allow one-way
containment so that only the discovered/available tag contains the ranking ID
(i.e., keep the check that the ranked ID is inside the available tag) — modify
the loop over available_lower and the conditional using ranking.model_id.lower()
so it only returns ranking when ranking_id_lower in available (remove the
available in ranking_id_lower branch) to prevent false positives when selecting
from candidates.
In `@src/synthorg/templates/schema.py`:
- Around line 388-395: The memory Field currently typed as dict[str, Any] in
schema.py lets malformed template overrides pass; replace it with a typed
Pydantic model (e.g., MemoryConfig) that explicitly defines allowed keys
(embedder.provider: str | None, embedder.model: str | None, embedder.dims: int |
None, etc.) and use that model as the field type for memory (or validate
incoming dicts into MemoryConfig in a root_validator) so parsing of template
YAML fails fast on invalid keys/types; update any functions referencing memory
(the template parsing/instantiation path) to construct/validate via MemoryConfig
so errors are raised at parse time.
In `@tests/unit/api/controllers/test_setup_embedding.py`:
- Around line 28-33: The tests and review show embedder_provider is being saved
with the model_id instead of the provider; update auto_select_embedder in
setup_helpers.py to save the provider identifier when calling settings_svc.set
for the ("memory","embedder_provider") key rather than ranking.model_id. Since
EmbeddingModelRanking lacks a provider field, have the selector infer or accept
the provider (e.g., via an additional parameter or by mapping the chosen
model_id back to its provider) and pass that provider string into
settings_svc.set; ensure settings_svc.set calls that set
("memory","embedder_model") still use ranking.model_id and
("memory","embedder_dims") uses top.output_dims as before.
In `@tests/unit/memory/embedding/test_selector.py`:
- Around line 42-43: The test uses next(r for r in LMEB_RANKINGS if r.tier ==
DeploymentTier.GPU_FULL) to set gpu_model which will raise StopIteration if no
matching entry exists; change the lookup to a safe pattern (e.g., use next(...,
None)) and then either assert that the result is not None or call pytest.skip
with a clear message so the test fails/ skips with context; update both places
where gpu_model is obtained and similarly guard any next(...) usages over
LMEB_RANKINGS to reference the symbols gpu_model, LMEB_RANKINGS, and
DeploymentTier.GPU_FULL.
🪄 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: 8af81733-ea42-446b-ab68-7f1314c2a73b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
CLAUDE.mddocs/design/memory.mddocs/reference/embedding-evaluation.mdpyproject.tomlsrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/memory.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/api/controllers/setup_helpers.pysrc/synthorg/memory/__init__.pysrc/synthorg/memory/backends/mem0/config.pysrc/synthorg/memory/config.pysrc/synthorg/memory/embedding/__init__.pysrc/synthorg/memory/embedding/fine_tune.pysrc/synthorg/memory/embedding/fine_tune_models.pysrc/synthorg/memory/embedding/rankings.pysrc/synthorg/memory/embedding/resolve.pysrc/synthorg/memory/embedding/selector.pysrc/synthorg/observability/events/memory.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/templates/schema.pytests/unit/api/controllers/test_memory_admin.pytests/unit/api/controllers/test_setup_embedding.pytests/unit/memory/backends/mem0/test_config.pytests/unit/memory/embedding/__init__.pytests/unit/memory/embedding/test_fine_tune.pytests/unit/memory/embedding/test_rankings.pytests/unit/memory/embedding/test_resolve.pytests/unit/memory/embedding/test_selector.pytests/unit/memory/test_config.pytests/unit/memory/test_init.py
📜 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)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Agent
- GitHub Check: Dependency Review
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
All versions are pinned with
==inpyproject.toml
Files:
pyproject.toml
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations-- Python 3.14 has PEP 649
Useexcept A, B:syntax (no parentheses) for exception handling -- PEP 758 except syntax, enforced by ruff on Python 3.14
All public functions must have type hints; enforce with mypy strict mode
Use Google-style docstrings, required on public classes and functions; enforced by ruff D rules
Create new objects for immutability; never mutate existing objects. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction plusMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config and identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict); adoptallow_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)
UseNotBlankStr(fromcore.types) for all identifier and 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
Maintain line length of 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, conf...
Files:
src/synthorg/memory/__init__.pysrc/synthorg/api/controllers/__init__.pytests/unit/memory/test_init.pysrc/synthorg/templates/schema.pytests/unit/api/controllers/test_setup_embedding.pysrc/synthorg/api/controllers/setup_helpers.pysrc/synthorg/memory/embedding/__init__.pytests/unit/api/controllers/test_memory_admin.pytests/unit/memory/embedding/test_fine_tune.pysrc/synthorg/memory/config.pytests/unit/memory/embedding/test_rankings.pytests/unit/memory/embedding/test_resolve.pysrc/synthorg/memory/backends/mem0/config.pytests/unit/memory/backends/mem0/test_config.pytests/unit/memory/test_config.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/memory/embedding/fine_tune_models.pytests/unit/memory/embedding/test_selector.pysrc/synthorg/memory/embedding/selector.pysrc/synthorg/api/controllers/memory.pysrc/synthorg/memory/embedding/resolve.pysrc/synthorg/memory/embedding/rankings.pysrc/synthorg/observability/events/memory.pysrc/synthorg/memory/embedding/fine_tune.pysrc/synthorg/settings/definitions/memory.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerfollowed bylogger = get_logger(__name__)
Never useimport logging,logging.getLogger(), orprint()in application code; exceptions:observability/setup.py,observability/sinks.py,observability/syslog_handler.py, andobservability/http_handler.pymay use stdlibloggingandprint(..., file=sys.stderr)
Always use variable namelogger(not_logger, notlog)
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 domain modules
Use structured kwargs in logging: alwayslogger.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
Use DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry and rate limiting automatically; never implement retry logic in driver subclasses or calling code
SetRetryConfigandRateLimiterConfigper-provider inProviderConfig
Retryable errors (is_retryable=True):RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry
Rate limiter respectsRateLimitError.retry_afterfrom providers and automatically pauses future requests
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples; use generic names likeexample-provider,example-large-001,example-medium-001,large/medium/smallaliases; vendor names allowed only in: (1) Operations design page (docs/design/operations.md), (2).claude/skill/agent files, (3) third-party import paths, (4) provid...
Files:
src/synthorg/memory/__init__.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/templates/schema.pysrc/synthorg/api/controllers/setup_helpers.pysrc/synthorg/memory/embedding/__init__.pysrc/synthorg/memory/config.pysrc/synthorg/memory/backends/mem0/config.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/memory/embedding/fine_tune_models.pysrc/synthorg/memory/embedding/selector.pysrc/synthorg/api/controllers/memory.pysrc/synthorg/memory/embedding/resolve.pysrc/synthorg/memory/embedding/rankings.pysrc/synthorg/observability/events/memory.pysrc/synthorg/memory/embedding/fine_tune.pysrc/synthorg/settings/definitions/memory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Maintain 80% code coverage minimum; enforced in CI
Useasyncio_mode = "auto"in pytest configuration; no manual@pytest.mark.asyncioneeded
Global test timeout: 30 seconds per test (configured inpyproject.toml); non-default overrides liketimeout(60)are allowed but do not add per-filepytest.mark.timeout(30)markers
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python with@givenand@settingsdecorators; profiles:ci(50 examples, default) anddev(1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
NEVER skip, dismiss, or ignore flaky tests -- always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()for determinism; for tasks that must block indefinitely until cancelled, useasyncio.Event().wait()instead ofasyncio.sleep(large_number)
Files:
tests/unit/memory/test_init.pytests/unit/api/controllers/test_setup_embedding.pytests/unit/api/controllers/test_memory_admin.pytests/unit/memory/embedding/test_fine_tune.pytests/unit/memory/embedding/test_rankings.pytests/unit/memory/embedding/test_resolve.pytests/unit/memory/backends/mem0/test_config.pytests/unit/memory/test_config.pytests/unit/memory/embedding/test_selector.py
🧠 Learnings (45)
📓 Common learnings
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/memory/**/*.py : Memory package (memory/): pluggable MemoryBackend protocol, backends/ (Mem0 adapter), retrieval pipeline (ranking, RRF fusion, injection, formatting, non-inferable filtering), shared org memory (org/), consolidation/archival (density-aware: DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy)
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to pyproject.toml : Docs group in `pyproject.toml` includes: `zensical`, `mkdocstrings[python]`, `griffe-pydantic`
Applied to files:
pyproject.toml
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to pyproject.toml : Required runtime dependencies: `mem0ai` (default and currently only memory backend), `cryptography` (Fernet encryption), `faker` (multi-locale name generation)
Applied to files:
pyproject.toml
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {pyproject.toml,src/synthorg/__init__.py} : Version locations: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`)
Applied to files:
pyproject.toml
📚 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/memory/**/*.py : Memory package (memory/): pluggable MemoryBackend protocol, backends/ (Mem0 adapter), retrieval pipeline (ranking, RRF fusion, injection, formatting, non-inferable filtering), shared org memory (org/), consolidation/archival (density-aware: DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy)
Applied to files:
src/synthorg/memory/__init__.pysrc/synthorg/api/controllers/__init__.pyCLAUDE.mdtests/unit/memory/test_init.pysrc/synthorg/templates/schema.pysrc/synthorg/memory/embedding/__init__.pydocs/design/memory.mdsrc/synthorg/memory/backends/mem0/config.pytests/unit/memory/test_config.pysrc/synthorg/memory/embedding/selector.pysrc/synthorg/api/controllers/memory.pysrc/synthorg/memory/embedding/resolve.pysrc/synthorg/memory/embedding/rankings.pysrc/synthorg/observability/events/memory.pysrc/synthorg/memory/embedding/fine_tune.pysrc/synthorg/settings/definitions/memory.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/memory/**/*.py : Use MemoryBackend protocol with pluggable backends (Mem0 adapter available at backends/mem0/) for persistent agent memory
Applied to files:
src/synthorg/memory/__init__.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/memory/backends/mem0/config.pytests/unit/memory/test_config.pysrc/synthorg/api/controllers/memory.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/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
src/synthorg/api/controllers/__init__.pyCLAUDE.mdsrc/synthorg/memory/embedding/__init__.pysrc/synthorg/api/controllers/memory.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/__init__.pysrc/synthorg/api/controllers/memory.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: Engine: Agent orchestration, execution loops, parallel execution, task decomposition, routing, task assignment, centralized single-writer task state engine (TaskEngine), task lifecycle, recovery, shutdown, workspace isolation, coordination (multi-agent pipeline: TopologyDispatcher protocol, 4 dispatchers — SAS/centralized/decentralized/context-dependent, wave execution, workspace lifecycle integration, CoordinationSectionConfig company config bridge, build_coordinator factory), coordination error classification, prompt policy validation, checkpoint recovery (checkpoint/, per-turn persistence, heartbeat detection, CheckpointRecoveryStrategy), approval gate (escalation detection, context parking/resume, EscalationInfo/ResumePayload models), stagnation detection (stagnation/, StagnationDetector protocol, ToolRepetitionDetector, dual-signal analysis, corrective prompt injection), agent runtime state (AgentRuntimeState, lightweight per-agent execution status for dashboard queries and recove...
Applied to files:
CLAUDE.md
📚 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/engine/**/*.py : Engine package (engine/): agent orchestration, parallel execution, task decomposition, routing, TaskEngine (centralized single-writer), task lifecycle/recovery/shutdown, workspace isolation, coordination (4 dispatchers: SAS/centralized/decentralized/context-dependent, wave execution), approval gates (escalation detection, context parking/resume), stagnation detection (ToolRepetitionDetector, corrective prompt injection), AgentRuntimeState (execution status), context budget management, conversation compaction (oldest-turns summarizer)
Applied to files:
CLAUDE.md
📚 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/core/**/*.py : Core module must contain shared domain models, base classes, resilience config (RetryConfig, RateLimiterConfig)
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:43:05.601Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:43:05.601Z
Learning: Applies to docs/** : Docs source in docs/ (Markdown, built with Zensical); design spec in docs/design/ (7 pages: index, agents, organization, communication, engine, memory, operations)
Applied to files:
CLAUDE.md
📚 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: Documentation source in `docs/` (Markdown, built with Zensical). Design spec in `docs/design/` (7 pages: index, agents, organization, communication, engine, memory, operations). Architecture in `docs/architecture/` (overview, tech-stack, decision log). Roadmap in `docs/roadmap/`. Security in `docs/security.md`. Licensing in `docs/licensing.md`. Reference in `docs/reference/`. REST API reference in `docs/rest-api.md`. Library reference in `docs/api/` (auto-generated from docstrings). Custom templates in `docs/overrides/`. Config in `mkdocs.yml`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/hr/**/*.py : HR package (hr/): hiring, firing, onboarding, offboarding, agent registry, performance tracking (task metrics, collaboration scoring, LLM calibration, collaboration overrides, trend detection), promotion/demotion (criteria evaluation, approval strategies, model mapping)
Applied to files:
CLAUDE.md
📚 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 docs/design/*.md : Design spec pages: 7 pages in `docs/design/` — index, agents, organization, communication, engine, memory, operations
Applied to files:
CLAUDE.md
📚 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/hr/**/*.py : HR engine must provide: hiring, firing, onboarding, offboarding, agent registry, performance tracking (task metrics, collaboration scoring, trend detection), promotion/demotion
Applied to files:
CLAUDE.md
📚 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/templates/**/*.py : Templates: pre-built company templates, personality presets, and builder.
Applied to files:
src/synthorg/templates/schema.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/memory/config.pysrc/synthorg/memory/backends/mem0/config.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/memory/embedding/fine_tune_models.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/memory/config.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/memory/embedding/fine_tune_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/memory/config.pysrc/synthorg/memory/embedding/fine_tune_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 : 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/memory/config.pysrc/synthorg/memory/embedding/fine_tune_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 Pydantic v2 BaseModel, model_validator, computed_field, ConfigDict.
Applied to files:
src/synthorg/memory/config.pysrc/synthorg/memory/embedding/fine_tune_models.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/memory/config.pysrc/synthorg/memory/embedding/fine_tune_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 : For dict/list fields in frozen Pydantic models, rely on frozen=True for field reassignment prevention and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Applied to files:
src/synthorg/memory/config.pysrc/synthorg/memory/embedding/fine_tune_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/memory/config.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to src/synthorg/providers/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`
Applied to files:
src/synthorg/memory/config.py
📚 Learning: 2026-04-02T13:12:42.085Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T13:12:42.085Z
Learning: Applies to **/*.py : Use frozen Pydantic models for config and 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/memory/config.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/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/memory/backends/mem0/config.pysrc/synthorg/observability/events/memory.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability includes structured logging via `get_logger(__name__)`, correlation tracking, and log sinks.
Applied to files:
src/synthorg/memory/backends/mem0/config.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/memory/backends/mem0/config.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/memory/backends/mem0/config.py
📚 Learning: 2026-03-26T15:18:16.848Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:18:16.848Z
Learning: Applies to src/synthorg/api/**/*.py : Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Applied to files:
src/synthorg/api/controllers/setup.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to **/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models with `model_copy(update=...)` for runtime state that evolves
Applied to files:
src/synthorg/memory/embedding/fine_tune_models.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/memory/embedding/fine_tune_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 Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/memory/embedding/fine_tune_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 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/observability/events/memory.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/observability/events/memory.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 : 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/observability/events/memory.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/observability/events/memory.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/observability/events/memory.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/observability/events/memory.py
📚 Learning: 2026-04-02T13:12:42.085Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T13:12:42.085Z
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 domain modules
Applied to files:
src/synthorg/observability/events/memory.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
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/observability/events/memory.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/settings/**/*.py : Settings use runtime-editable persistence with precedence: DB > env > YAML > code defaults. 8 namespaces with Fernet encryption for sensitive values.
Applied to files:
src/synthorg/settings/definitions/memory.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/settings/**/*.py : Settings package (settings/): runtime-editable settings persistence (DB > env > YAML > code defaults), typed definitions (9 namespaces), Fernet encryption for sensitive values, config bridge (JSON serialization for Pydantic/collections), ConfigResolver (typed accessors), validation, registry, change notifications via message bus, SettingsSubscriber protocol, SettingsChangeDispatcher (polls `#settings` channel, routes to subscribers, restart_required filtering)
Applied to files:
src/synthorg/settings/definitions/memory.py
🔇 Additional comments (26)
src/synthorg/observability/events/memory.py (1)
73-85: Good event taxonomy extension for embedder/fine-tune flows.The new constants are consistent with the existing
memory.<entity>.<action>pattern and improve observability coverage for the new feature paths.src/synthorg/memory/embedding/fine_tune_models.py (1)
9-58: Models are well-structured and validation-safe.
ConfigDict(frozen=True, allow_inf_nan=False)plus constrained fields are correctly applied for request/status payloads.src/synthorg/memory/__init__.py (1)
19-19: Public re-export wiring is correct.
EmbedderOverrideConfigis consistently imported and exposed via__all__, keeping the package API coherent.Also applies to: 87-87
src/synthorg/api/controllers/__init__.py (1)
20-20: Controller registration is complete.
MemoryAdminControlleris correctly imported, routed, and exported, so the new admin endpoints are discoverable and mounted.Also applies to: 61-61, 84-84
tests/unit/memory/test_init.py (1)
20-20: Good test sync with public API change.Adding
EmbedderOverrideConfigto the expected export set keepsmemory.__all__coverage aligned with the module change.CLAUDE.md (1)
100-100: LGTM!The memory package documentation is correctly updated to reflect the new
embedding/subpackage with its three responsibilities: LMEB-ranked model selection, embedder config resolution, and fine-tuning pipeline.docs/reference/embedding-evaluation.md (1)
217-225: LGTM!The documentation clearly explains the updated Mem0 adapter behavior: when
fine_tune.enabledis true with acheckpoint_path, that path is used directly as the model identifier; when disabled, the base model is used. This aligns with the_resolve_effective_model()implementation.tests/unit/memory/backends/mem0/test_config.py (2)
118-131: LGTM!The renamed test correctly validates that
fine_tune.enabled=Trueis now accepted (previously rejected). Good coverage of the checkpoint path preservation.
244-275: LGTM!Comprehensive test coverage for the three fine-tune model resolution cases:
- Enabled → uses
checkpoint_pathas model identifier- Explicitly disabled → uses base model
fine_tuneisNone→ uses base modelThese tests directly verify the
_resolve_effective_model()behavior.src/synthorg/memory/embedding/__init__.py (1)
1-26: LGTM!Clean package initialization with explicit
__all__constraining the public API surface. The module correctly re-exports ranking data, selection functions, and resolution logic without containing business logic itself.tests/unit/memory/embedding/test_rankings.py (1)
1-124: LGTM!Thorough data integrity tests for the LMEB ranking system. The tests validate:
- Enum correctness and exhaustiveness
- Model immutability and field validation
- Dataset constraints (no duplicates, valid score ranges, tier-to-params consistency, sorted order, all tiers present)
These provide strong baseline guarantees for the selection logic.
src/synthorg/memory/config.py (2)
139-184: LGTM!The
EmbedderOverrideConfigmodel follows project conventions:
- Frozen Pydantic model with
allow_inf_nan=FalseNotBlankStr | Nonefor identifier fields- Proper validation that
dimsis required whenmodelis set- Structured logging with event constant before raising
227-233: LGTM!The
embedderfield is correctly added toCompanyMemoryConfigas optional with clear documentation.src/synthorg/api/controllers/setup.py (2)
116-139: LGTM!The
_collect_model_idshelper correctly:
- Guards against missing config resolver
- Extracts model IDs from all providers using a flat comprehension
- Returns empty tuple on failure (best-effort, non-blocking)
- Uses DEBUG level logging appropriate for non-critical failures
Based on the context snippets,
pc.modelsdefaults to()andmodel.idis alwaysNotBlankStr, so the access patterns are safe.
777-792: LGTM!The auto-select embedding integration follows the established best-effort pattern:
MemoryErrorandRecursionErrorare re-raised (correct per project conventions)- Other exceptions are logged as warnings without blocking setup completion
- Clear comment explaining the best-effort nature
tests/unit/memory/embedding/test_fine_tune.py (1)
1-139: LGTM!Comprehensive validation tests for fine-tuning pipeline stage functions. Good coverage of:
- Enum value assertions
- Blank input rejection for all required parameters
- Boundary validation (
epochs=0)NotImplementedErrorplaceholder behaviortests/unit/memory/test_config.py (2)
156-221: LGTM!Thorough test coverage for
EmbedderOverrideConfig:
- Default behavior validation
- Provider/model/dims coupling rules
- Immutability enforcement
- Boundary conditions (blank strings, zero/negative dims)
- Edge case for
dimswithoutmodelbeing valid
274-286: LGTM!Good integration tests verifying
CompanyMemoryConfig.embedderfield behavior with the newEmbedderOverrideConfigtype.tests/unit/api/controllers/test_memory_admin.py (2)
58-64: Good validation coverage forallow_inf_nan=False.These tests properly verify that
FineTuneStatusrejectsNaNandInfvalues forprogress, ensuring compliance with theallow_inf_nan=FalseConfigDict requirement.
74-83: LGTM!Controller metadata tests verify the correct path and tags are defined.
docs/design/memory.md (1)
362-381: LGTM!Documentation accurately reflects the implemented fine-tuning workflow:
- Admin API trigger (
POST /admin/memory/fine-tune)- Checkpoint path as model identifier behavior
- Required fields when
enabled=True- Security constraints (path traversal, Windows path rejection)
src/synthorg/memory/backends/mem0/config.py (1)
236-258: LGTM!Clean implementation of checkpoint resolution:
- Properly checks both
fine_tune is not Noneandenabledflag- Logs activation with relevant context using event constants
- Type ignore is justified since model validator enforces
checkpoint_pathwhenenabled=Truesrc/synthorg/settings/definitions/memory.py (1)
48-89: LGTM!Well-structured embedding override settings:
- Appropriately marked as
ADVANCEDlevel- Correct type definitions (
STRINGfor provider/model,INTEGERfor dims)min_value=1constraint onembedder_dimsprevents invalid dimensions- YAML paths correctly map to
memory.embedder.*hierarchyThe settings service handles string ↔ int conversion and range validation, and consumer code (per relevant code snippets from
memory.py:111-119) correctly coercesdimstointon read.tests/unit/memory/embedding/test_selector.py (1)
84-126: LGTM!Good parametrized test coverage for
infer_deployment_tier:
- Local providers with/without GPU
- Unknown GPU status defaulting behavior
- Cloud provider assumption
- Case-insensitivity
Nonepreset handlingtests/unit/api/controllers/test_setup_embedding.py (1)
54-65: 🧹 Nitpick | 🔵 TrivialConsider consolidating duplicate test logic.
test_persists_providerandtest_selects_best_modelshare identical setup and invoke the same function with the same inputs. The provider persistence check could be added as an additional assertion intest_selects_best_modelto reduce duplication.⛔ Skipped due to learnings
Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T18:28:13.207Z Learning: Applies to tests/**/*.py : Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data.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.tests/unit/memory/embedding/test_resolve.py (1)
11-141: Good coverage of resolver precedence and fallback behavior.These tests exercise the critical resolution paths and failure modes comprehensively.
| # Pipeline stages are not yet implemented -- return status | ||
| # indicating the pipeline is idle with a descriptive error. | ||
| return ApiResponse( | ||
| data=FineTuneStatus( | ||
| stage=FineTuneStage.FAILED, | ||
| error=( | ||
| "Fine-tuning pipeline stages are not yet " | ||
| "implemented. Install synthorg[fine-tune] " | ||
| "and check back in a future release." | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
POST /fine-tune is hardcoded to fail and does not start any pipeline work.
Line 63 always returns FineTuneStage.FAILED, so this endpoint cannot fulfill its admin trigger role. This is a functional blocker for fine-tuning operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/memory.py` around lines 59 - 70, The POST
/fine-tune handler currently always returns FineTuneStage.FAILED (using
ApiResponse and FineTuneStatus), so replace the hardcoded failure with code that
enqueues/starts the actual fine-tune pipeline (e.g., call a pipeline starter
like start_fine_tune_pipeline or enqueue_fine_tune_job with the request
payload), return a success status (FineTuneStage.INITIATED or
FineTuneStage.RUNNING) and an ApiResponse (HTTP 202 Accepted) containing the job
id or tracking info; on exceptions catch the error, log it, and return
FineTuneStage.FAILED with the error message. Ensure you reference and update the
response construction around ApiResponse, FineTuneStatus, and FineTuneStage
rather than leaving the static error string.
| except MemoryError, RecursionError: | ||
| raise | ||
| except Exception: |
There was a problem hiding this comment.
Log context before re-raising MemoryError/RecursionError.
At Line 120-122, those exceptions are re-raised without any log record. Add a warning/error log with context before raising to preserve diagnosability.
🛠️ Minimal logging fix
except MemoryError, RecursionError:
+ logger.error(
+ MEMORY_EMBEDDER_SETTINGS_READ_FAILED,
+ reason="settings_service_read_fatal",
+ exc_info=True,
+ )
raiseAs per coding guidelines: "All error paths must log at WARNING or ERROR with context before raising".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/memory.py` around lines 120 - 122, The except
block that currently reads "except MemoryError, RecursionError: raise" should
log contextual details before re-raising: in the except handler for MemoryError
and RecursionError inside src/synthorg/api/controllers/memory.py, call the
module logger (e.g., logger or logging.getLogger(__name__)) to emit a WARNING or
ERROR containing context (function name, relevant request/user IDs or object
identifiers available in that scope) and include exception traceback via
exc_info=True, then re-raise the exception; update the handler for the exact
exception tuple (MemoryError, RecursionError) so the log runs immediately before
the raise.
| await settings_svc.set( | ||
| "memory", | ||
| "embedder_provider", | ||
| ranking.model_id, | ||
| ) | ||
| await settings_svc.set( | ||
| "memory", | ||
| "embedder_model", | ||
| ranking.model_id, | ||
| ) | ||
| await settings_svc.set( | ||
| "memory", | ||
| "embedder_dims", | ||
| str(ranking.output_dims), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check EmbeddingModelRanking for provider field
ast-grep --pattern $'class EmbeddingModelRanking($_) {
$$$
}'
# Check if select_embedding_model has access to provider info
rg -nA 20 'def select_embedding_model'Repository: Aureliolo/synthorg
Length of output: 1825
🏁 Script executed:
# Find EmbeddingModelRanking class definition
rg -n 'class EmbeddingModelRanking' --type pyRepository: Aureliolo/synthorg
Length of output: 146
🏁 Script executed:
# View the auto_select_embedder function with full context
sed -n '623,692p' src/synthorg/api/controllers/setup_helpers.py | cat -nRepository: Aureliolo/synthorg
Length of output: 2632
🏁 Script executed:
# Check if ranking object contains provider info or if provider is passed separately
rg -n 'EmbeddingModelRanking' src/synthorg/memory/embedding/selector.py -A 10 -B 2Repository: Aureliolo/synthorg
Length of output: 943
🏁 Script executed:
# View the complete EmbeddingModelRanking class definition
cat -n src/synthorg/memory/embedding/rankings.pyRepository: Aureliolo/synthorg
Length of output: 6633
embedder_provider is set to model_id instead of the actual provider name.
Both embedder_provider and embedder_model are set to ranking.model_id. The EmbeddingModelRanking class has no provider field, and the function receives only model IDs without provider context. The provider information needed to correctly populate embedder_provider is lost during selection.
To fix this, auto_select_embedder needs access to the provider name (e.g., from caller context or as a separate parameter), or the selection flow must be redesigned to preserve provider-to-model mappings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/setup_helpers.py` around lines 678 - 692, The
code sets "embedder_provider" to ranking.model_id instead of the provider name;
update the selection flow so auto_select_embedder (or its caller) returns or
preserves the provider name and use that when calling settings_svc.set for
"memory", "embedder_provider" (e.g., set to provider_name variable rather than
ranking.model_id). Specifically, modify
auto_select_embedder/EmbeddingModelRanking usage to include provider (or accept
an explicit provider parameter), then replace the first settings_svc.set call to
use that provider identifier, keep "embedder_model" as ranking.model_id and
"embedder_dims" as str(ranking.output_dims).
| # Should have stored provider, model, and dims | ||
| calls = settings_svc.set.call_args_list | ||
| values = {(c.args[0], c.args[1]): c.args[2] for c in calls} | ||
| assert values[("memory", "embedder_model")] == top.model_id | ||
| assert values[("memory", "embedder_dims")] == str(top.output_dims) | ||
| assert values[("memory", "embedder_provider")] == top.model_id |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check LMEB_RANKINGS structure for provider vs model_id fields
ast-grep --pattern $'class $_(BaseModel):
$$$
model_id: $_
$$$
'
# Also check auto_select_embedder implementation
rg -nA 20 'def auto_select_embedder' --type pyRepository: Aureliolo/synthorg
Length of output: 25261
🏁 Script executed:
# Get the full auto_select_embedder implementation
sed -n '625,700p' src/synthorg/api/controllers/setup_helpers.py
# Check the test file to understand the test setup
cat -n tests/unit/api/controllers/test_setup_embedding.py | head -50Repository: Aureliolo/synthorg
Length of output: 4123
Fix implementation: embedder_provider should store provider name, not model ID.
The implementation incorrectly stores ranking.model_id in both "embedder_provider" and "embedder_model" settings (setup_helpers.py:676–677, 681–683). The embedder_provider field should hold a provider identifier (e.g., "ollama", "example-provider"), not the model ID. The EmbeddingModelRanking class lacks a provider field, so the selector must infer or pass the provider separately when storing embedder config. Update auto_select_embedder to populate the provider setting correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/controllers/test_setup_embedding.py` around lines 28 - 33, The
tests and review show embedder_provider is being saved with the model_id instead
of the provider; update auto_select_embedder in setup_helpers.py to save the
provider identifier when calling settings_svc.set for the
("memory","embedder_provider") key rather than ranking.model_id. Since
EmbeddingModelRanking lacks a provider field, have the selector infer or accept
the provider (e.g., via an additional parameter or by mapping the chosen
model_id back to its provider) and pass that provider string into
settings_svc.set; ensure settings_svc.set calls that set
("memory","embedder_model") still use ranking.model_id and
("memory","embedder_dims") uses top.output_dims as before.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #999 +/- ##
==========================================
- Coverage 91.69% 91.66% -0.03%
==========================================
Files 649 656 +7
Lines 35488 35719 +231
Branches 3493 3512 +19
==========================================
+ Hits 32539 32742 +203
- Misses 2330 2357 +27
- Partials 619 620 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
src/synthorg/memory/embedding/rankings.py (1)
103-183:⚠️ Potential issue | 🟠 MajorReplace checked-in vendor/model branding with project-safe aliases.
The dimension notes and
model_idliterals still embed real vendor/model names, so this module violates the repo's vendor-agnostic policy. Please keep only internal aliases here and translate to provider-specific identifiers at the integration boundary.As per coding guidelines: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples; use generic names like
example-provider,example-large-001,example-medium-001,large/medium/smallaliases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/embedding/rankings.py` around lines 103 - 183, The file currently embeds real vendor/model branding in the top comment block and in LMEB_RANKINGS via model_id strings (e.g., "bge-multilingual-gemma2", "NV-Embed-v2", etc.); replace those literal vendor/model names with project-safe aliases (e.g., "example-large-3584", "example-xlarge-4096", "example-medium-2560", etc.) inside LMEB_RANKINGS and change the comment lines above to generic dimension notes (remove vendor references); keep the EmbeddingModelRanking entries, output_dims and numeric scores unchanged, and ensure any lookup that needs provider-specific IDs is performed at the integration boundary (translate aliases to real provider IDs where the system calls actual provider SDKs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/dependency-review.yml:
- Around line 119-121: The workflow adds broad per-package exemptions for
proprietary NVIDIA CUDA packages (e.g., cuda-bindings, nvidia-cublas,
nvidia-cudnn-cu13 and other NVIDIA entries in the dependency allow-list) but
lacks explicit policy/legal approval; update the PR by adding an explicit
required sign-off from Legal/Policy (or a named approver or team) before merge,
reference these package names in the PR description and add a CI guard or
checklist item requiring confirmation (e.g., a labelled approval comment or
CODEOWNERS entry) that Legal has reviewed the proprietary CUDA EULA exemption,
so merges cannot proceed without that sign-off.
- Around line 116-123: The folded-scalar value for allow-dependencies-licenses
currently contains embedded comment text (the descriptive lines about fine-tune
dependencies) which breaks the comma-separated PURL list parsing; edit the
workflow to remove/move those descriptive lines out of the folded scalar and
place them as proper YAML comments above or below the scalar so that the
allow-dependencies-licenses scalar contains only the intended comma-separated
license identifiers (refer to the allow-dependencies-licenses folded scalar in
.github/workflows/dependency-review.yml and ensure no extra lines from the
fine-tune comment are inside that scalar).
In `@src/synthorg/api/controllers/memory.py`:
- Around line 142-145: When constructing ActiveEmbedderResponse in memory.py,
normalize sentinel empty-string setting values to None so they aren't treated as
valid truthy values: check provider_sv.value and model_sv.value and convert ""
to None before passing to ActiveEmbedderResponse (e.g., set provider = None if
provider_sv.value == "" else provider_sv.value, same for model), ensuring
downstream consumers receive None for unset settings instead of an empty string.
In `@src/synthorg/api/controllers/setup_helpers.py`:
- Around line 641-647: The catch block currently logs using the generic
SETUP_COMPLETE_CHECK_ERROR constant; replace that with a dedicated event
constant (e.g. MEMORY_MODEL_DISCOVERY_FAILED) in the except block inside the
collect_model_ids / model ID collection flow so the log reflects embedding model
discovery failure specifically, and add/import the new
MEMORY_MODEL_DISCOVERY_FAILED constant from your events/constants module (or
define it where other event constants live) so observability shows this distinct
failure type.
- Around line 650-656: The auto_select_embedder function's provider_preset_name
and has_gpu params are never forwarded from its call site, causing
infer_deployment_tier(None, None) to default to GPU_CONSUMER; update the
setup.py call that invokes auto_select_embedder to pass the actual
provider_preset_name and has_gpu values (or variables available in that scope)
so auto_select_embedder -> infer_deployment_tier receives real inputs; if those
values aren't available yet, add a short TODO comment in the call site and/or in
auto_select_embedder documenting the current best-effort behavior and add a
follow-up issue to implement proper hardware detection and parameter
propagation.
In `@src/synthorg/memory/backends/mem0/config.py`:
- Around line 252-263: The code currently falls back to embedder.model when a
fine-tune (ft) checkpoint is missing or its checkpoint_path is None; change both
fallbacks to return the fine-tuned base model (ft.base_model) instead of
embedder.model so the behavior matches the warning/contract (references: ft,
checkpoint_path, embedder.model, MEMORY_EMBEDDER_CHECKPOINT_MISSING).
In `@src/synthorg/memory/config.py`:
- Line 153: The EmbedderOverrideConfig currently allows unknown keys to be
silently ignored; update its Pydantic config so unknown fields are rejected at
parse time (set model_config/Config to forbid extra fields, e.g., extra="forbid"
/ Extra.forbid) so typos like "providre" raise a validation error; modify the
EmbedderOverrideConfig class (and any related override config classes) to
include this extra-forbid setting and ensure any existing model_config lines
(e.g., model_config = ConfigDict(...)) include extra="forbid" so invalid keys
fail fast with a clear validation error.
In `@src/synthorg/memory/embedding/fine_tune_models.py`:
- Around line 36-55: The _reject_path_traversal model_validator currently
concatenates PureWindowsPath(val).parts and PurePosixPath(val).parts into the
parts variable causing duplicates; change the logic in _reject_path_traversal to
deduplicate those path parts (e.g., build a set from both
PureWindowsPath(...).parts and PurePosixPath(...).parts) before checking for
"..", keeping the same checks for backslashes and drive letters on source_dir
and output_dir so behavior remains unchanged but avoids redundant entries.
In `@src/synthorg/memory/embedding/resolve.py`:
- Around line 114-150: The code currently synthesizes provider from model by
passing fallback_provider=None to the first _merge_override and later doing
provider = model; change it to pass the active connected provider as the
fallback (use the existing provider_preset_name / connected provider context)
when calling _merge_override after _auto_select_from_lmeb so provider only
changes if explicitly overridden, and remove the unconditional provider = model
assignment; instead, if you need provider to default to model only for
local-adapter setups, implement an explicit check (e.g., "if
is_local_adapter(provider_preset_name) or adapter_is_local(provider): provider =
model") in that branch. Ensure you update calls around infer_deployment_tier,
_auto_select_from_lmeb, memory_config.embedder, and the two _merge_override
invocations to reflect this change.
In `@src/synthorg/memory/embedding/selector.py`:
- Around line 17-19: The code is emitting MEMORY_EMBEDDER_AUTO_SELECT_FAILED
with reason="match_found" (a success path); change those emits in
src/synthorg/memory/embedding/selector.py so they either emit a matching success
event (e.g., MEMORY_EMBEDDER_AUTO_SELECT_SUCCEEDED — create this constant if it
doesn't exist) with the same payload, or simply remove the failure-event emit
entirely; make the same change for the other occurrence around the 63-68 block
so success flows no longer generate failure metrics.
In `@tests/unit/api/controllers/test_setup_embedding.py`:
- Around line 54-66: The test test_persists_model_and_dims duplicates assertions
in test_selects_best_model; either remove this redundant test or change it to
verify only persistence behavior: keep the call to
auto_select_embedder(settings_svc=..., available_model_ids=(top.model_id,)) and
then assert that settings_svc.set was called with keys
("memory","embedder_model") and ("memory","embedder_dims") and the correct
values (e.g., embedder model id and dims) instead of rechecking selection logic
already covered by test_selects_best_model; update the test name or add a short
comment to clarify it focuses solely on persistence via settings_svc.set and
reference auto_select_embedder and settings_svc.set when editing.
---
Duplicate comments:
In `@src/synthorg/memory/embedding/rankings.py`:
- Around line 103-183: The file currently embeds real vendor/model branding in
the top comment block and in LMEB_RANKINGS via model_id strings (e.g.,
"bge-multilingual-gemma2", "NV-Embed-v2", etc.); replace those literal
vendor/model names with project-safe aliases (e.g., "example-large-3584",
"example-xlarge-4096", "example-medium-2560", etc.) inside LMEB_RANKINGS and
change the comment lines above to generic dimension notes (remove vendor
references); keep the EmbeddingModelRanking entries, output_dims and numeric
scores unchanged, and ensure any lookup that needs provider-specific IDs is
performed at the integration boundary (translate aliases to real provider IDs
where the system calls actual provider SDKs).
🪄 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: 21d3c957-cd48-4f5e-b62c-6a67411a23d1
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/dependency-review.ymlCLAUDE.mddocs/architecture/decisions.mddocs/roadmap/open-questions.mdpyproject.tomlsrc/synthorg/api/controllers/memory.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/api/controllers/setup_helpers.pysrc/synthorg/memory/backends/mem0/config.pysrc/synthorg/memory/config.pysrc/synthorg/memory/embedding/fine_tune.pysrc/synthorg/memory/embedding/fine_tune_models.pysrc/synthorg/memory/embedding/rankings.pysrc/synthorg/memory/embedding/resolve.pysrc/synthorg/memory/embedding/selector.pysrc/synthorg/observability/events/memory.pysrc/synthorg/templates/schema.pytests/unit/api/controllers/test_memory_admin.pytests/unit/api/controllers/test_setup_embedding.pytests/unit/memory/backends/mem0/test_config.pytests/unit/memory/embedding/test_resolve.pytests/unit/memory/test_config.py
| # installed when users run `pip install synthorg[fine-tune]`. License | ||
| # metadata is missing from PyPI for NVIDIA CUDA packages (proprietary | ||
| # but freely redistributable under NVIDIA EULA). torch is BSD-style. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For these PyPI packages/versions — cuda-bindings 13.2.0, cuda-toolkit 13.0.2, nvidia-cublas 13.1.0.3, nvidia-cudnn-cu13 9.19.0.56, torch 2.11.0 — what licenses are declared in package metadata/classifiers?
💡 Result:
cuda-bindings 13.2.0: LicenseRef-NVIDIA-SOFTWARE-LICENSE cuda-toolkit 13.0.2: No license declared in visible metadata (meta-package) nvidia-cublas 13.1.0.3: LicenseRef-NVIDIA-Proprietary (observed in similar version 13.0.0.19; likely same for 13.1.0.3) nvidia-cudnn-cu13 9.19.0.56: No license declared in visible metadata (runtime libraries package) torch 2.11.0: BSD-3-Clause
Citations:
- 1: https://pypi.org/project/cuda-bindings/
- 2: https://pypi.org/project/cuda-python/
- 3: https://pypi.org/project/cuda-toolkit/13.0.2/
- 4: https://pypi.org/project/nvidia-cublas/13.0.0.19/
- 5: https://pypi.org/project/nvidia-cudnn-cu13/
- 6: https://pypi.org/project/torch/
🏁 Script executed:
cat -n .github/workflows/dependency-review.yml | sed -n '115,145p'Repository: Aureliolo/synthorg
Length of output: 1830
Request explicit policy/legal sign-off for proprietary CUDA package exemptions.
This adds broad per-package exemptions for proprietary NVIDIA CUDA packages (cuda-bindings, nvidia-cublas, nvidia-cudnn-cu13, and others at lines 127-144), which exceeds the documented CI allow-list baseline of permissive + weak-copyleft licenses. While these are optional dependencies (only installed for pip install synthorg[fine-tune]), the proprietary NVIDIA EULA rationale needs explicit policy/legal approval before merge to align with your baseline governance.
The included permissive-licensed packages (torch at BSD-3-Clause, scikit-learn at BSD-3-Clause) fit the baseline correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/dependency-review.yml around lines 119 - 121, The workflow
adds broad per-package exemptions for proprietary NVIDIA CUDA packages (e.g.,
cuda-bindings, nvidia-cublas, nvidia-cudnn-cu13 and other NVIDIA entries in the
dependency allow-list) but lacks explicit policy/legal approval; update the PR
by adding an explicit required sign-off from Legal/Policy (or a named approver
or team) before merge, reference these package names in the PR description and
add a CI guard or checklist item requiring confirmation (e.g., a labelled
approval comment or CODEOWNERS entry) that Legal has reviewed the proprietary
CUDA EULA exemption, so merges cannot proceed without that sign-off.
| except Exception: | ||
| logger.debug( | ||
| SETUP_COMPLETE_CHECK_ERROR, | ||
| check="collect_model_ids", | ||
| exc_info=True, | ||
| ) | ||
| return () |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more specific event constant for model ID collection failure.
SETUP_COMPLETE_CHECK_ERROR is used for general setup check failures, but this is specifically about collecting model IDs for embedding selection. A dedicated event constant like MEMORY_MODEL_DISCOVERY_FAILED would provide better observability granularity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/setup_helpers.py` around lines 641 - 647, The
catch block currently logs using the generic SETUP_COMPLETE_CHECK_ERROR
constant; replace that with a dedicated event constant (e.g.
MEMORY_MODEL_DISCOVERY_FAILED) in the except block inside the collect_model_ids
/ model ID collection flow so the log reflects embedding model discovery failure
specifically, and add/import the new MEMORY_MODEL_DISCOVERY_FAILED constant from
your events/constants module (or define it where other event constants live) so
observability shows this distinct failure type.
| async def auto_select_embedder( | ||
| *, | ||
| settings_svc: SettingsService, | ||
| available_model_ids: tuple[str, ...], | ||
| provider_preset_name: str | None = None, | ||
| has_gpu: bool | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Tier inference parameters are never passed from the call site.
The auto_select_embedder function accepts provider_preset_name and has_gpu parameters, but the call site in setup.py (lines 759-762 per context snippet 1) only passes settings_svc and available_model_ids. Both parameters default to None, causing infer_deployment_tier(None, has_gpu=None) to always return GPU_CONSUMER (per context snippet 3).
On CPU-only systems, this may select models that require GPU resources. Consider either:
- Passing
provider_preset_nameandhas_gpufrom the call site - Adding a TODO/follow-up issue to implement hardware detection
- Documenting this as intentional "best-effort" behavior
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/setup_helpers.py` around lines 650 - 656, The
auto_select_embedder function's provider_preset_name and has_gpu params are
never forwarded from its call site, causing infer_deployment_tier(None, None) to
default to GPU_CONSUMER; update the setup.py call that invokes
auto_select_embedder to pass the actual provider_preset_name and has_gpu values
(or variables available in that scope) so auto_select_embedder ->
infer_deployment_tier receives real inputs; if those values aren't available
yet, add a short TODO comment in the call site and/or in auto_select_embedder
documenting the current best-effort behavior and add a follow-up issue to
implement proper hardware detection and parameter propagation.
| @model_validator(mode="after") | ||
| def _reject_path_traversal(self) -> Self: | ||
| """Reject parent-directory traversal and Windows paths.""" | ||
| for field_name in ("source_dir", "output_dir"): | ||
| val = getattr(self, field_name) | ||
| if val is None: | ||
| continue | ||
| parts = PureWindowsPath(val).parts + PurePosixPath(val).parts | ||
| if ".." in parts: | ||
| msg = f"{field_name} must not contain parent-directory traversal (..)" | ||
| raise ValueError(msg) | ||
| if "\\" in val or ( | ||
| len(val) >= 2 and val[1] == ":" # noqa: PLR2004 | ||
| ): | ||
| msg = ( | ||
| f"{field_name} must be a POSIX path (no backslashes " | ||
| "or drive letters)" | ||
| ) | ||
| raise ValueError(msg) | ||
| return self |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Path traversal detection is thorough but has redundant part concatenation.
The validation concatenates parts from both PureWindowsPath and PurePosixPath, which works but creates duplicate entries for valid POSIX paths. Consider using a set for deduplication:
- parts = PureWindowsPath(val).parts + PurePosixPath(val).parts
+ parts = set(PureWindowsPath(val).parts) | set(PurePosixPath(val).parts)However, the current implementation is functionally correct—this is a minor optimization.
📝 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.
| @model_validator(mode="after") | |
| def _reject_path_traversal(self) -> Self: | |
| """Reject parent-directory traversal and Windows paths.""" | |
| for field_name in ("source_dir", "output_dir"): | |
| val = getattr(self, field_name) | |
| if val is None: | |
| continue | |
| parts = PureWindowsPath(val).parts + PurePosixPath(val).parts | |
| if ".." in parts: | |
| msg = f"{field_name} must not contain parent-directory traversal (..)" | |
| raise ValueError(msg) | |
| if "\\" in val or ( | |
| len(val) >= 2 and val[1] == ":" # noqa: PLR2004 | |
| ): | |
| msg = ( | |
| f"{field_name} must be a POSIX path (no backslashes " | |
| "or drive letters)" | |
| ) | |
| raise ValueError(msg) | |
| return self | |
| `@model_validator`(mode="after") | |
| def _reject_path_traversal(self) -> Self: | |
| """Reject parent-directory traversal and Windows paths.""" | |
| for field_name in ("source_dir", "output_dir"): | |
| val = getattr(self, field_name) | |
| if val is None: | |
| continue | |
| parts = set(PureWindowsPath(val).parts) | set(PurePosixPath(val).parts) | |
| if ".." in parts: | |
| msg = f"{field_name} must not contain parent-directory traversal (..)" | |
| raise ValueError(msg) | |
| if "\\" in val or ( | |
| len(val) >= 2 and val[1] == ":" # noqa: PLR2004 | |
| ): | |
| msg = ( | |
| f"{field_name} must be a POSIX path (no backslashes " | |
| "or drive letters)" | |
| ) | |
| raise ValueError(msg) | |
| return self |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/embedding/fine_tune_models.py` around lines 36 - 55, The
_reject_path_traversal model_validator currently concatenates
PureWindowsPath(val).parts and PurePosixPath(val).parts into the parts variable
causing duplicates; change the logic in _reject_path_traversal to deduplicate
those path parts (e.g., build a set from both PureWindowsPath(...).parts and
PurePosixPath(...).parts) before checking for "..", keeping the same checks for
backslashes and drive letters on source_dir and output_dir so behavior remains
unchanged but avoids redundant entries.
| tier = infer_deployment_tier(provider_preset_name, has_gpu=has_gpu) | ||
| auto_model, auto_dims = _auto_select_from_lmeb(available_models, tier) | ||
|
|
||
| # Apply YAML config override (second priority). | ||
| provider, model, dims = _merge_override( | ||
| memory_config.embedder, | ||
| fallback_provider=None, | ||
| fallback_model=auto_model, | ||
| fallback_dims=auto_dims, | ||
| ) | ||
|
|
||
| # Apply settings override (highest priority). | ||
| provider, model, dims = _merge_override( | ||
| settings_override, | ||
| fallback_provider=provider, | ||
| fallback_model=model, | ||
| fallback_dims=dims, | ||
| ) | ||
|
|
||
| if model is None or dims is None: | ||
| logger.warning( | ||
| MEMORY_EMBEDDER_AUTO_SELECT_FAILED, | ||
| available_models=len(available_models), | ||
| tier=tier.value, | ||
| reason="no LMEB-ranked model available and no override", | ||
| ) | ||
| msg = ( | ||
| "Could not resolve embedding model configuration: " | ||
| "no LMEB-ranked model found in available models " | ||
| "and no manual override provided" | ||
| ) | ||
| raise MemoryConfigError(msg) | ||
|
|
||
| # Provider defaults to model for local embedder setups | ||
| # (e.g. Ollama serves models by name). | ||
| if provider is None: | ||
| provider = model |
There was a problem hiding this comment.
Don't synthesize provider from model.
Line 114 already has the active provider context, but the merge starts with fallback_provider=None and the no-override path later collapses to provider = model. That can produce Mem0EmbedderConfig(provider=<model_id>, ...), and provider-only overrides can likewise swap providers while keeping a model/dims pair discovered from a different backend. Use the connected provider as the fallback provider, and only fall back to provider=model inside an explicit local-adapter branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/embedding/resolve.py` around lines 114 - 150, The code
currently synthesizes provider from model by passing fallback_provider=None to
the first _merge_override and later doing provider = model; change it to pass
the active connected provider as the fallback (use the existing
provider_preset_name / connected provider context) when calling _merge_override
after _auto_select_from_lmeb so provider only changes if explicitly overridden,
and remove the unconditional provider = model assignment; instead, if you need
provider to default to model only for local-adapter setups, implement an
explicit check (e.g., "if is_local_adapter(provider_preset_name) or
adapter_is_local(provider): provider = model") in that branch. Ensure you update
calls around infer_deployment_tier, _auto_select_from_lmeb,
memory_config.embedder, and the two _merge_override invocations to reflect this
change.
| async def test_persists_model_and_dims(self) -> None: | ||
| top = LMEB_RANKINGS[0] | ||
| settings_svc = _mock_settings_svc() | ||
|
|
||
| await auto_select_embedder( | ||
| settings_svc=settings_svc, | ||
| available_model_ids=(top.model_id,), | ||
| ) | ||
|
|
||
| calls = settings_svc.set.call_args_list | ||
| keys_set = {(c.args[0], c.args[1]) for c in calls} | ||
| assert ("memory", "embedder_model") in keys_set | ||
| assert ("memory", "embedder_dims") in keys_set |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating with test_selects_best_model.
test_persists_model_and_dims validates a subset of what test_selects_best_model already asserts (lines 28-33 check the same keys exist with correct values). Consider removing this test or differentiating its purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/controllers/test_setup_embedding.py` around lines 54 - 66, The
test test_persists_model_and_dims duplicates assertions in
test_selects_best_model; either remove this redundant test or change it to
verify only persistence behavior: keep the call to
auto_select_embedder(settings_svc=..., available_model_ids=(top.model_id,)) and
then assert that settings_svc.set was called with keys
("memory","embedder_model") and ("memory","embedder_dims") and the correct
values (e.g., embedder model id and dims) instead of rechecking selection logic
already covered by test_selects_best_model; update the test name or add a short
comment to clarify it focuses solely on persistence via settings_svc.set and
reference auto_select_embedder and settings_svc.set when editing.
7890712 to
52273b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
src/synthorg/api/controllers/setup_helpers.py (1)
625-637:⚠️ Potential issue | 🟠 MajorPreserve provider identity through model discovery and persistence.
collect_model_ids()collapses provider configs to bare model IDs, soauto_select_embedder()has no way to persistmemory.embedder_provideralongsideembedder_modelandembedder_dims. Since the settings schema still carries a separate provider field, setup can save a model/dims pair that later resolves with a stale provider orNone. Return provider/model pairs from discovery and persist all three fields atomically.Also applies to: 650-656, 703-713
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/setup_helpers.py` around lines 625 - 637, collect_model_ids currently flattens provider configs to model IDs losing provider identity; change collect_model_ids(app_state: AppState) to return tuples of (provider_name, model_id) (or a typed pair) by iterating config_resolver.get_provider_configs() and emitting the provider key with each model.id, then update callers such as auto_select_embedder to consume provider/model pairs and persist memory.embedder_provider together with embedder_model and embedder_dims atomically (ensure any persistence/setting logic that previously only stored embedder_model/embedder_dims also sets embedder_provider). Also update any other usages that assumed a plain list of IDs to handle the new pair shape (e.g., the code paths around memory.embedder_provider and embedder_dims)..github/workflows/dependency-review.yml (1)
94-98:⚠️ Potential issue | 🟠 MajorRequire an explicit approval path for these proprietary CUDA exemptions.
This block extends dependency-review exceptions beyond the repo’s stated “permissive + weak-copyleft” baseline by allowlisting proprietary NVIDIA runtime packages for the
fine-tuneextra. Because these are optional runtime dependencies rather than dev-only tooling, they need an explicit policy/legal approval gate before merge instead of being silently folded into the default allow-list.Based on learnings: Dependency allow-list in CI: permissive + weak-copyleft licenses, per-package GPL exemptions for dev-only tool deps (golangci-lint).
Also applies to: 123-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dependency-review.yml around lines 94 - 98, The dependency-review exceptions block that allowlists proprietary NVIDIA CUDA packages for the fine-tune extra must require an explicit legal/engineering approval gate before merge: update the dependency-review.yml logic that currently extends the allow-list for the "fine-tune" extra (the CUDA/NVIDIA runtime exemptions) to instead mark those packages as needing a required approval step (e.g., add a required-check status, a dedicated CI approval job, or require a repo label/PR review from the legal/approval team) and ensure the same pattern is applied to the similar block referenced (lines ~123-142); locate the block referencing "fine-tune", "NVIDIA CUDA packages", or the dependency allow-list and change it from silently allowlisting to gating on an explicit approval artifact (approval label, job output, or required reviewer).src/synthorg/api/controllers/setup.py (1)
755-764:⚠️ Potential issue | 🟠 MajorDon't persist a tier-filtered embedder choice until real tier inputs are wired.
auto_select_embedder()supportsprovider_preset_nameandhas_gpu, but this call still passes neither and the TODO confirms it. That means setup can persist a model based on a generic fallback tier instead of the actual host/provider, so CPU-only installs can save an incompatible embedder override into the settings DB.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/setup.py` around lines 755 - 764, auto_select_embedder is being called without real tier inputs so it may persist a tier-filtered embedder unsuitable for the host; change the call in this setup path to either pass the actual provider_preset_name and has_gpu from the setup context (so auto_select_embedder(settings_svc=settings_svc, available_model_ids=model_ids, provider_preset_name=..., has_gpu=...)) or, if those values are not yet available, invoke auto_select_embedder in non-persistent/dry-run mode (or add a persist=False flag) so it does not write a tier-filtered choice into settings_svc until real inputs are wired; update the call site that uses _collect_model_ids and auto_select_embedder accordingly.src/synthorg/api/controllers/memory.py (2)
147-148:⚠️ Potential issue | 🟡 MinorLog fatal settings-read failures before re-raising.
Lines 147-148 re-raise
MemoryErrorandRecursionErrorwithout any record, which drops the only context for a crash in this path. Emit anERRORwithexc_info=Trueimmediately beforeraise.As per coding guidelines: "All error paths must log at WARNING or ERROR with context before raising".Minimal fix
except MemoryError, RecursionError: + logger.error( + MEMORY_EMBEDDER_SETTINGS_READ_FAILED, + reason="settings_service_fatal", + exc_info=True, + ) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/memory.py` around lines 147 - 148, The except block that currently re-raises MemoryError and RecursionError needs to log the failure first: before the existing "except MemoryError, RecursionError: raise" line, call the module/class logger's error method with a descriptive message and exc_info=True (e.g., logger.error("Fatal settings-read failure", exc_info=True)) so the exception context is recorded, then re-raise the exception; update the except handler around that MemoryError/RecursionError clause in memory.py to perform the log call immediately prior to raise.
59-91:⚠️ Potential issue | 🟠 Major
/fine-tuneand/fine-tune/statusnever report a live run.
POST /fine-tunealways returnsFineTuneStage.FAILED, andGET /fine-tune/statusalways returnsIDLE, so the admin API cannot ever start or track a pipeline run. Wire both handlers to a real job launcher/state store and return an accepted/running state once work has been enqueued.Also applies to: 93-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/memory.py` around lines 59 - 91, The start_fine_tune handler (start_fine_tune) should not always return FineTuneStage.FAILED; instead enqueue a fine-tune job via the project's job launcher/state store (e.g., call the fine-tune job launcher or job_store API used elsewhere, such as a create_run/enqueue_fine_tune method) and persist the run id and initial state; return an ApiResponse containing FineTuneStatus with stage=FineTuneStage.RUNNING (or an appropriate ACCEPTED state) and the run identifier. Likewise, update the /fine-tune/status handler to lookup the run state from the same state store/job status API (e.g., get_run_status or job_store.get) and return the real FineTuneStage and any error/message from that store instead of always returning IDLE. Ensure both handlers reference the same state keys/IDs so the POST-produced run can be tracked by the GET.tests/unit/memory/embedding/test_selector.py (1)
36-53:⚠️ Potential issue | 🟡 MinorGuard the
GPU_FULLfixture lookup.The
next(...)calls at Lines 42 and 53 will raiseStopIterationwith no context ifLMEB_RANKINGSever drops itsGPU_FULLentries. Usenext(..., None)and assert/skip with a clear message so the failure explains the missing test precondition.Safer setup pattern
- gpu_model = next(r for r in LMEB_RANKINGS if r.tier == DeploymentTier.GPU_FULL) + gpu_model = next( + (r for r in LMEB_RANKINGS if r.tier == DeploymentTier.GPU_FULL), + None, + ) + if gpu_model is None: + pytest.skip("LMEB_RANKINGS has no GPU_FULL entry")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/memory/embedding/test_selector.py` around lines 36 - 53, The tests use next(...) without a default when looking up a GPU model in LMEB_RANKINGS (in test_respects_tier_filter and test_tier_filter_no_match), which will raise StopIteration if no GPU_FULL entry exists; change those lookups to next(..., None), then explicitly assert or skip with a clear message (e.g., "no DeploymentTier.GPU_FULL entry in LMEB_RANKINGS") so the test fails/returns with a helpful precondition error instead of an uninformative exception.src/synthorg/memory/embedding/rankings.py (1)
99-183:⚠️ Potential issue | 🟠 MajorReplace checked-in vendor/model branding with project-safe aliases.
This module stores real vendor/model names directly in both the comments and
LMEB_RANKINGS, so the ranking data itself violates the repo's vendor-agnostic policy. Introduce internal aliases plus a resolution layer at the system boundary instead of committing branded identifiers here.As per coding guidelines: "NEVER use real vendor names ... in project-owned code, docstrings, comments, tests, or config examples; ... vendor names allowed only in ... provider presets (
src/synthorg/providers/presets.py)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/embedding/rankings.py` around lines 99 - 183, The LMEB_RANKINGS tuple currently embeds real vendor/model names (in comments and model_id fields) which violates the vendor-agnostic policy; replace each concrete model_id string in LMEB_RANKINGS with a project-safe alias (e.g., "embed_gemma_v2", "embed_nv_v2", "embed_qwen3_4b", etc.) and remove vendor-branded names from nearby comments, keep all numeric metrics and fields (params_billions, episodic, procedural, dialogue, semantic, overall, use_instructions, output_dims) unchanged; then add a resolution mapping at the system boundary (in the provider presets module mentioned by the policy, e.g., src/synthorg/providers/presets.py) that maps each new alias back to the real vendor model identifier used by runtimes, ensuring code that consumes LMEB_RANKINGS uses aliases internally while only the presets layer performs alias→vendor resolution.src/synthorg/memory/embedding/resolve.py (1)
118-150:⚠️ Potential issue | 🟠 MajorDon't synthesize
providerfrom the resolvedmodel.At Line 120 the first merge discards the connected provider by forcing
fallback_provider=None, and Lines 149-150 then replace a missing provider with the model id. That producesMem0EmbedderConfig(provider=<model_id>, ...)on the auto-selected path, and a provider-only override can also leave you with a provider/model pair discovered from different backends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/embedding/resolve.py` around lines 118 - 150, The code incorrectly forces fallback_provider=None in the first _merge_override and then synthesizes provider=model when provider is missing; update the first merge call to preserve any provider already present on memory_config.embedder (e.g., pass fallback_provider=memory_config.embedder.provider or otherwise do not discard an existing provider) and remove the final fallback that assigns provider = model so we do not synthesize a provider from the resolved model; adjust any tests/consumers that expect provider to remain None when truly unspecified.src/synthorg/memory/embedding/fine_tune.py (1)
80-85:⚠️ Potential issue | 🟠 MajorLog each stage failure before raising
NotImplementedError.Lines 80-85, 117-122, 165-171, and 192-197 build a stage-specific failure message and raise immediately, so failed fine-tune invocations leave no observable breadcrumb. Emit a stage-specific WARNING/ERROR, using a memory event constant, before each raise.
As per coding guidelines: "All error paths must log at WARNING or ERROR with context before raising".
Also applies to: 117-122, 165-171, 192-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/embedding/fine_tune.py` around lines 80 - 85, Before raising NotImplementedError at the stage-specific blocks (the msg variable followed by raise NotImplementedError), emit a stage-specific log and memory event with context: call the module's memory event emitter (e.g., emit_memory_event or memory_events.emit) with a constant like MEMORY_EVENT_STAGE_FAILURE (or define MEMORY_EVENT_FINETUNE_STAGE_FAILURE) and the msg, then log the msg at WARNING or ERROR using the file's logger (e.g., logger.warning or logger.error) immediately before the raise; apply the same pattern to the other three locations (the blocks at the other provided line ranges) so every error path logs the stage-specific message and emits the memory event before raising.
🤖 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/memory.py`:
- Around line 33-40: Change the types of the embedding identifier fields to use
the shared NotBlankStr type so whitespace-only strings are rejected: replace the
annotations for provider and model (the Field definitions currently typed as str
| None) with NotBlankStr | None from core.types and keep the existing Field
defaults/descriptions intact; ensure you import NotBlankStr from core.types at
the top of the file if not already present.
In `@src/synthorg/memory/embedding/selector.py`:
- Around line 23-29: Replace the hardcoded vendor-branded preset ids in
_LOCAL_PRESETS with a reference to the canonical preset metadata from the shared
provider presets: import the preset constants or metadata (from the module that
defines provider presets) and build the local/self-hosted set from that source
instead of embedding "ollama", "lm-studio", "vllm"; update any other occurrences
(the similar block around the symbols referenced at lines ~80-83) to derive
locality from the shared preset definitions so there is a single source of truth
(use the exported preset names/flags from the presets module and construct the
frozenset for _LOCAL_PRESETS from those).
In `@src/synthorg/settings/definitions/memory.py`:
- Around line 77-89: The SettingDefinition for namespace SettingNamespace.MEMORY
with key "embedder_dims" is declared as SettingType.INTEGER but the setup code
stores it using str(ranking.output_dims); fix by storing the raw integer (remove
str(...) so the value written is an int) where ranking.output_dims is saved, or
alternatively change the SettingDefinition type to SettingType.STRING if you
intentionally want strings; prefer storing the integer so SettingDefinition
(embedder_dims), its min_value validation, and any consumers no longer need to
parse/convert the value.
In `@src/synthorg/templates/schema.py`:
- Around line 404-407: The template loader is not passing YAML memory overrides
into CompanyTemplate so memory.embedder is ignored; update the loader (the code
that builds kwargs for CompanyTemplate) to detect if data.get("memory") exists
and include it in the kwargs by constructing/validating a TemplateMemoryConfig
(or letting Pydantic coerce a dict) and assigning it to the "memory" key before
instantiating CompanyTemplate; ensure the "memory.embedder" value from the YAML
ends up inside the TemplateMemoryConfig used by CompanyTemplate so defaults are
only used when memory is absent.
In `@tests/unit/memory/backends/mem0/test_config.py`:
- Around line 262-274: Update the test_falls_back_when_checkpoint_missing test
to use distinct model names so it verifies fallback comes from
fine_tune.base_model rather than embedder.model: create Mem0EmbedderConfig with
embedder.model set to one value (e.g., "embedder-model-v1") and set
EmbeddingFineTuneConfig.base_model to a different value (e.g., "base-model-v2")
with checkpoint_path pointing to a nonexistent path, call
build_mem0_config_dict(config) and assert that
result["embedder"]["config"]["model"] equals the fine_tune.base_model value;
referencing the test function name, Mem0EmbedderConfig, EmbeddingFineTuneConfig,
Mem0BackendConfig, and build_mem0_config_dict to locate and update the test.
In `@tests/unit/memory/embedding/test_fine_tune.py`:
- Around line 42-47: The test currently asserts generate_training_data raises
NotImplementedError which locks in stub behavior; update tests in
tests/unit/memory/embedding/test_fine_tune.py to exercise the full four-stage
pipeline happy path by orchestrating generate_training_data (and any helper
functions it calls) to run successfully: replace the pytest.raises checks with a
test that mocks ML internals (e.g., training/evaluation/inference calls, file
I/O) to return deterministic outputs and assert the expected artifacts and
orchestration steps are produced/triggered (for example, verify calls to data
preprocessing, model training, checkpointing, and output write behaviors), and
similarly update the other affected test cases (previously asserting
NotImplementedError) to validate successful stage transitions rather than stub
exceptions.
---
Duplicate comments:
In @.github/workflows/dependency-review.yml:
- Around line 94-98: The dependency-review exceptions block that allowlists
proprietary NVIDIA CUDA packages for the fine-tune extra must require an
explicit legal/engineering approval gate before merge: update the
dependency-review.yml logic that currently extends the allow-list for the
"fine-tune" extra (the CUDA/NVIDIA runtime exemptions) to instead mark those
packages as needing a required approval step (e.g., add a required-check status,
a dedicated CI approval job, or require a repo label/PR review from the
legal/approval team) and ensure the same pattern is applied to the similar block
referenced (lines ~123-142); locate the block referencing "fine-tune", "NVIDIA
CUDA packages", or the dependency allow-list and change it from silently
allowlisting to gating on an explicit approval artifact (approval label, job
output, or required reviewer).
In `@src/synthorg/api/controllers/memory.py`:
- Around line 147-148: The except block that currently re-raises MemoryError and
RecursionError needs to log the failure first: before the existing "except
MemoryError, RecursionError: raise" line, call the module/class logger's error
method with a descriptive message and exc_info=True (e.g., logger.error("Fatal
settings-read failure", exc_info=True)) so the exception context is recorded,
then re-raise the exception; update the except handler around that
MemoryError/RecursionError clause in memory.py to perform the log call
immediately prior to raise.
- Around line 59-91: The start_fine_tune handler (start_fine_tune) should not
always return FineTuneStage.FAILED; instead enqueue a fine-tune job via the
project's job launcher/state store (e.g., call the fine-tune job launcher or
job_store API used elsewhere, such as a create_run/enqueue_fine_tune method) and
persist the run id and initial state; return an ApiResponse containing
FineTuneStatus with stage=FineTuneStage.RUNNING (or an appropriate ACCEPTED
state) and the run identifier. Likewise, update the /fine-tune/status handler to
lookup the run state from the same state store/job status API (e.g.,
get_run_status or job_store.get) and return the real FineTuneStage and any
error/message from that store instead of always returning IDLE. Ensure both
handlers reference the same state keys/IDs so the POST-produced run can be
tracked by the GET.
In `@src/synthorg/api/controllers/setup_helpers.py`:
- Around line 625-637: collect_model_ids currently flattens provider configs to
model IDs losing provider identity; change collect_model_ids(app_state:
AppState) to return tuples of (provider_name, model_id) (or a typed pair) by
iterating config_resolver.get_provider_configs() and emitting the provider key
with each model.id, then update callers such as auto_select_embedder to consume
provider/model pairs and persist memory.embedder_provider together with
embedder_model and embedder_dims atomically (ensure any persistence/setting
logic that previously only stored embedder_model/embedder_dims also sets
embedder_provider). Also update any other usages that assumed a plain list of
IDs to handle the new pair shape (e.g., the code paths around
memory.embedder_provider and embedder_dims).
In `@src/synthorg/api/controllers/setup.py`:
- Around line 755-764: auto_select_embedder is being called without real tier
inputs so it may persist a tier-filtered embedder unsuitable for the host;
change the call in this setup path to either pass the actual
provider_preset_name and has_gpu from the setup context (so
auto_select_embedder(settings_svc=settings_svc, available_model_ids=model_ids,
provider_preset_name=..., has_gpu=...)) or, if those values are not yet
available, invoke auto_select_embedder in non-persistent/dry-run mode (or add a
persist=False flag) so it does not write a tier-filtered choice into
settings_svc until real inputs are wired; update the call site that uses
_collect_model_ids and auto_select_embedder accordingly.
In `@src/synthorg/memory/embedding/fine_tune.py`:
- Around line 80-85: Before raising NotImplementedError at the stage-specific
blocks (the msg variable followed by raise NotImplementedError), emit a
stage-specific log and memory event with context: call the module's memory event
emitter (e.g., emit_memory_event or memory_events.emit) with a constant like
MEMORY_EVENT_STAGE_FAILURE (or define MEMORY_EVENT_FINETUNE_STAGE_FAILURE) and
the msg, then log the msg at WARNING or ERROR using the file's logger (e.g.,
logger.warning or logger.error) immediately before the raise; apply the same
pattern to the other three locations (the blocks at the other provided line
ranges) so every error path logs the stage-specific message and emits the memory
event before raising.
In `@src/synthorg/memory/embedding/rankings.py`:
- Around line 99-183: The LMEB_RANKINGS tuple currently embeds real vendor/model
names (in comments and model_id fields) which violates the vendor-agnostic
policy; replace each concrete model_id string in LMEB_RANKINGS with a
project-safe alias (e.g., "embed_gemma_v2", "embed_nv_v2", "embed_qwen3_4b",
etc.) and remove vendor-branded names from nearby comments, keep all numeric
metrics and fields (params_billions, episodic, procedural, dialogue, semantic,
overall, use_instructions, output_dims) unchanged; then add a resolution mapping
at the system boundary (in the provider presets module mentioned by the policy,
e.g., src/synthorg/providers/presets.py) that maps each new alias back to the
real vendor model identifier used by runtimes, ensuring code that consumes
LMEB_RANKINGS uses aliases internally while only the presets layer performs
alias→vendor resolution.
In `@src/synthorg/memory/embedding/resolve.py`:
- Around line 118-150: The code incorrectly forces fallback_provider=None in the
first _merge_override and then synthesizes provider=model when provider is
missing; update the first merge call to preserve any provider already present on
memory_config.embedder (e.g., pass
fallback_provider=memory_config.embedder.provider or otherwise do not discard an
existing provider) and remove the final fallback that assigns provider = model
so we do not synthesize a provider from the resolved model; adjust any
tests/consumers that expect provider to remain None when truly unspecified.
In `@tests/unit/memory/embedding/test_selector.py`:
- Around line 36-53: The tests use next(...) without a default when looking up a
GPU model in LMEB_RANKINGS (in test_respects_tier_filter and
test_tier_filter_no_match), which will raise StopIteration if no GPU_FULL entry
exists; change those lookups to next(..., None), then explicitly assert or skip
with a clear message (e.g., "no DeploymentTier.GPU_FULL entry in LMEB_RANKINGS")
so the test fails/returns with a helpful precondition error instead of an
uninformative exception.
🪄 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: 2f035a91-ce3e-45ca-b869-fe201daf4a4e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.github/workflows/dependency-review.ymlCLAUDE.mddocs/architecture/decisions.mddocs/design/memory.mddocs/reference/embedding-evaluation.mddocs/roadmap/open-questions.mdpyproject.tomlsrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/memory.pysrc/synthorg/api/controllers/setup.pysrc/synthorg/api/controllers/setup_helpers.pysrc/synthorg/memory/__init__.pysrc/synthorg/memory/backends/mem0/config.pysrc/synthorg/memory/config.pysrc/synthorg/memory/embedding/__init__.pysrc/synthorg/memory/embedding/fine_tune.pysrc/synthorg/memory/embedding/fine_tune_models.pysrc/synthorg/memory/embedding/rankings.pysrc/synthorg/memory/embedding/resolve.pysrc/synthorg/memory/embedding/selector.pysrc/synthorg/observability/events/memory.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/templates/schema.pytests/unit/api/controllers/test_memory_admin.pytests/unit/api/controllers/test_setup_embedding.pytests/unit/memory/backends/mem0/test_config.pytests/unit/memory/embedding/__init__.pytests/unit/memory/embedding/test_fine_tune.pytests/unit/memory/embedding/test_rankings.pytests/unit/memory/embedding/test_resolve.pytests/unit/memory/embedding/test_selector.pytests/unit/memory/test_config.pytests/unit/memory/test_init.py
| # Provider preset names that indicate local/self-hosted deployment. | ||
| _LOCAL_PRESETS: frozenset[str] = frozenset( | ||
| { | ||
| "ollama", | ||
| "lm-studio", | ||
| "vllm", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Stop hardcoding vendor-branded preset ids in memory code.
_LOCAL_PRESETS and the public examples reintroduce vendor identifiers into src/synthorg/memory/**, and they create another place that can drift from the canonical preset definitions. Drive locality from shared preset metadata or imported constants instead of embedding "ollama", "lm-studio", and "vllm" here.
As per coding guidelines: "NEVER use real vendor names ... in project-owned code, docstrings, comments, tests, or config examples; ... vendor names allowed only in ... provider presets (src/synthorg/providers/presets.py)."
Also applies to: 80-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/embedding/selector.py` around lines 23 - 29, Replace the
hardcoded vendor-branded preset ids in _LOCAL_PRESETS with a reference to the
canonical preset metadata from the shared provider presets: import the preset
constants or metadata (from the module that defines provider presets) and build
the local/self-hosted set from that source instead of embedding "ollama",
"lm-studio", "vllm"; update any other occurrences (the similar block around the
symbols referenced at lines ~80-83) to derive locality from the shared preset
definitions so there is a single source of truth (use the exported preset
names/flags from the presets module and construct the frozenset for
_LOCAL_PRESETS from those).
| _r.register( | ||
| SettingDefinition( | ||
| namespace=SettingNamespace.MEMORY, | ||
| key="embedder_dims", | ||
| type=SettingType.INTEGER, | ||
| default=None, | ||
| description="Override embedding vector dimensions (advanced)", | ||
| group="Embedding", | ||
| level=SettingLevel.ADVANCED, | ||
| min_value=1, | ||
| yaml_path="memory.embedder.dims", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how settings_svc.set handles type coercion and how embedder_dims is read
rg -nA 10 'embedder_dims' --type py -g '!test*'Repository: Aureliolo/synthorg
Length of output: 3367
Type mismatch: embedder_dims defined as INTEGER but stored as STRING.
The setting is defined as SettingType.INTEGER in memory.py:81, but stored as a string in setup_helpers.py:712 via str(ranking.output_dims). While the consuming code in memory.py:134 converts the string back to an int with explicit error handling, this workaround masks a type contract violation. Fix by either storing the raw integer in setup_helpers.py or changing the definition to SettingType.STRING.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/definitions/memory.py` around lines 77 - 89, The
SettingDefinition for namespace SettingNamespace.MEMORY with key "embedder_dims"
is declared as SettingType.INTEGER but the setup code stores it using
str(ranking.output_dims); fix by storing the raw integer (remove str(...) so the
value written is an int) where ranking.output_dims is saved, or alternatively
change the SettingDefinition type to SettingType.STRING if you intentionally
want strings; prefer storing the integer so SettingDefinition (embedder_dims),
its min_value validation, and any consumers no longer need to parse/convert the
value.
| memory: TemplateMemoryConfig = Field( | ||
| default_factory=TemplateMemoryConfig, | ||
| description="Memory configuration overrides.", | ||
| ) |
There was a problem hiding this comment.
memory.embedder template overrides are still ignored.
CompanyTemplate.memory is typed now, but src/synthorg/templates/loader.py:448-520 never copies data["memory"] into the kwargs passed to CompanyTemplate. As written, YAML memory.embedder values are silently dropped and every template falls back to TemplateMemoryConfig().
Suggested follow-up in src/synthorg/templates/loader.py
result: dict[str, Any] = {
"metadata": metadata,
"variables": data.get("variables", ()),
"agents": data.get("agents", ()),
"departments": data.get("departments", ()),
"workflow": data.get("workflow", "agile_kanban"),
"workflow_config": data.get("workflow_config", {}),
"communication": data.get("communication", "hybrid"),
"budget_monthly": _to_float(company.get("budget_monthly", 50.0)),
"autonomy": company.get("autonomy", {"level": "semi"}),
"workflow_handoffs": data.get("workflow_handoffs", ()),
"escalation_paths": data.get("escalation_paths", ()),
+ "memory": data.get("memory", {}),
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/templates/schema.py` around lines 404 - 407, The template loader
is not passing YAML memory overrides into CompanyTemplate so memory.embedder is
ignored; update the loader (the code that builds kwargs for CompanyTemplate) to
detect if data.get("memory") exists and include it in the kwargs by
constructing/validating a TemplateMemoryConfig (or letting Pydantic coerce a
dict) and assigning it to the "memory" key before instantiating CompanyTemplate;
ensure the "memory.embedder" value from the YAML ends up inside the
TemplateMemoryConfig used by CompanyTemplate so defaults are only used when
memory is absent.
| async def test_raises_not_implemented(self) -> None: | ||
| with pytest.raises(NotImplementedError, match="training"): | ||
| await generate_training_data( | ||
| source_dir="/source", | ||
| output_dir="/output", | ||
| ) |
There was a problem hiding this comment.
These tests lock in stubbed behavior instead of the required pipeline behavior.
#966 calls for a runnable four-stage flow, but the expected outcome for every stage here is still NotImplementedError. That keeps CI green while the pipeline remains unimplemented. Please add happy-path/orchestration coverage for the stage flow (mocking ML internals if needed) and treat these stub exceptions as temporary scaffolding, not the contract.
Also applies to: 76-82, 120-126, 135-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/memory/embedding/test_fine_tune.py` around lines 42 - 47, The test
currently asserts generate_training_data raises NotImplementedError which locks
in stub behavior; update tests in tests/unit/memory/embedding/test_fine_tune.py
to exercise the full four-stage pipeline happy path by orchestrating
generate_training_data (and any helper functions it calls) to run successfully:
replace the pytest.raises checks with a test that mocks ML internals (e.g.,
training/evaluation/inference calls, file I/O) to return deterministic outputs
and assert the expected artifacts and orchestration steps are produced/triggered
(for example, verify calls to data preprocessing, model training, checkpointing,
and output write behaviors), and similarly update the other affected test cases
(previously asserting NotImplementedError) to validate successful stage
transitions rather than stub exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/memory/embedding/test_selector.py`:
- Around line 84-90: Replace the loose non-None assertion with a strict check
that the selector returns the highest-ranked candidate: after calling
select_embedding_model(available) assert the returned value matches the top
ranking (compare result == top.model_id; if select_embedding_model returns an
object compare result.model_id == top.model_id) so the test verifies the correct
ranking behavior using LMEB_RANKINGS[0] (top) and the select_embedding_model
function.
🪄 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: 9b67fe75-b727-4c2a-b6de-dfebe4d07631
📒 Files selected for processing (3)
src/synthorg/api/controllers/memory.pytests/unit/memory/backends/mem0/test_config.pytests/unit/memory/embedding/test_selector.py
📜 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). (9)
- GitHub Check: Deploy Preview
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations-- Python 3.14 has PEP 649
Useexcept A, B:syntax (no parentheses) for exception handling -- PEP 758 except syntax, enforced by ruff on Python 3.14
All public functions must have type hints; enforce with mypy strict mode
Use Google-style docstrings, required on public classes and functions; enforced by ruff D rules
Create new objects for immutability; never mutate existing objects. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction plusMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config and identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict); adoptallow_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)
UseNotBlankStr(fromcore.types) for all identifier and 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
Maintain line length of 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, conf...
Files:
tests/unit/memory/backends/mem0/test_config.pytests/unit/memory/embedding/test_selector.pysrc/synthorg/api/controllers/memory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Maintain 80% code coverage minimum; enforced in CI
Useasyncio_mode = "auto"in pytest configuration; no manual@pytest.mark.asyncioneeded
Global test timeout: 30 seconds per test (configured inpyproject.toml); non-default overrides liketimeout(60)are allowed but do not add per-filepytest.mark.timeout(30)markers
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python with@givenand@settingsdecorators; profiles:ci(50 examples, default) anddev(1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
NEVER skip, dismiss, or ignore flaky tests -- always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()for determinism; for tasks that must block indefinitely until cancelled, useasyncio.Event().wait()instead ofasyncio.sleep(large_number)
Files:
tests/unit/memory/backends/mem0/test_config.pytests/unit/memory/embedding/test_selector.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerfollowed bylogger = get_logger(__name__)
Never useimport logging,logging.getLogger(), orprint()in application code; exceptions:observability/setup.py,observability/sinks.py,observability/syslog_handler.py, andobservability/http_handler.pymay use stdlibloggingandprint(..., file=sys.stderr)
Always use variable namelogger(not_logger, notlog)
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 domain modules
Use structured kwargs in logging: alwayslogger.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
Use DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry and rate limiting automatically; never implement retry logic in driver subclasses or calling code
SetRetryConfigandRateLimiterConfigper-provider inProviderConfig
Retryable errors (is_retryable=True):RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry
Rate limiter respectsRateLimitError.retry_afterfrom providers and automatically pauses future requests
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples; use generic names likeexample-provider,example-large-001,example-medium-001,large/medium/smallaliases; vendor names allowed only in: (1) Operations design page (docs/design/operations.md), (2).claude/skill/agent files, (3) third-party import paths, (4) provid...
Files:
src/synthorg/api/controllers/memory.py
🧠 Learnings (26)
📓 Common learnings
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/memory/**/*.py : Memory package (memory/): pluggable MemoryBackend protocol, backends/ (Mem0 adapter), retrieval pipeline (ranking, RRF fusion, injection, formatting, non-inferable filtering), shared org memory (org/), consolidation/archival (density-aware: DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy)
📚 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/memory/**/*.py : Memory package (memory/): pluggable MemoryBackend protocol, backends/ (Mem0 adapter), retrieval pipeline (ranking, RRF fusion, injection, formatting, non-inferable filtering), shared org memory (org/), consolidation/archival (density-aware: DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy)
Applied to files:
src/synthorg/api/controllers/memory.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/memory/**/*.py : Use MemoryBackend protocol with pluggable backends (Mem0 adapter available at backends/mem0/) for persistent agent memory
Applied to files:
src/synthorg/api/controllers/memory.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/memory.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 **/*.py : Use `except A, B:` syntax (without parentheses) per PEP 758 for exception handling in Python 3.14
Applied to files:
src/synthorg/api/controllers/memory.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to **/*.py : Use `except A, B:` (no parentheses) per PEP 758 exception syntax on Python 3.14
Applied to files:
src/synthorg/api/controllers/memory.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 **/*.py : Use PEP 758 except syntax with `except A, B:` (no parentheses) for multiple exceptions—ruff enforces this on Python 3.14.
Applied to files:
src/synthorg/api/controllers/memory.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 **/*.py : Use `except A, B:` syntax (no parentheses) for exception handling — PEP 758 exception syntax enforced by ruff on Python 3.14
Applied to files:
src/synthorg/api/controllers/memory.py
📚 Learning: 2026-04-02T13:12:42.085Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T13:12:42.085Z
Learning: Applies to **/*.py : Use `except A, B:` syntax (no parentheses) for exception handling -- PEP 758 except syntax, enforced by ruff on Python 3.14
Applied to files:
src/synthorg/api/controllers/memory.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/api/controllers/memory.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 : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
src/synthorg/api/controllers/memory.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/api/controllers/memory.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
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/api/controllers/memory.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/api/controllers/memory.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 : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/memory.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/api/controllers/memory.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/memory.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 `NotBlankStr` from `core.types` for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Applied to files:
src/synthorg/api/controllers/memory.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to **/*.py : 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/memory.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/controllers/memory.py
📚 Learning: 2026-04-02T13:12:42.085Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T13:12:42.085Z
Learning: Applies to **/*.py : Use `NotBlankStr` (from `core.types`) for all identifier and name fields -- including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants -- instead of manual whitespace validators
Applied to files:
src/synthorg/api/controllers/memory.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 **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values 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/memory.py
📚 Learning: 2026-03-26T15:18:16.848Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:18:16.848Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. Use `computed_field` for derived values instead of storing redundant fields. Use `NotBlankStr` for all identifier/name fields.
Applied to files:
src/synthorg/api/controllers/memory.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 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/memory.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/memory.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/memory.py
🔇 Additional comments (9)
tests/unit/memory/backends/mem0/test_config.py (2)
118-131: LGTM!Clear test validating that
fine_tune.enabled=Trueis now accepted after removing the fail-fast validator. The docstring accurately documents the behavior change.
244-295: LGTM!Comprehensive test coverage for the fine-tune checkpoint resolution logic:
- Checkpoint exists (lines 244-260): Correctly mocks
Path.exists()and verifiescheckpoint_pathis used as the model identifier.- Checkpoint missing (lines 262-275): Now uses distinct model names (
"test-embedding-001"vs"test-base-model-v2") to verify fallback comes fromfine_tune.base_model—this addresses the prior feedback.- Fine-tune disabled (lines 277-285): Confirms
embedder.modelis used whenenabled=False.- Fine-tune None (lines 287-294): Confirms default behavior uses
embedder.model.The tests align well with the
_resolve_effective_model()implementation and cover all branches of the decision tree.src/synthorg/api/controllers/memory.py (7)
80-92: Placeholder endpoint returnsFAILED— intentional per comments.The previous review flagged this as a blocker. The current implementation documents the limitation with comments referencing issue
#1001and returns a descriptive error message. This is an acceptable stub while the pipeline stages remain unimplemented infine_tune.py.Consider returning HTTP 501 (Not Implemented) or 503 (Service Unavailable) instead of 200 with
FAILEDto better signal this to API clients.
148-149: Log context before re-raisingMemoryError/RecursionError.These fatal exceptions are re-raised without any log record, violating the guideline that all error paths must log at WARNING or ERROR with context before raising.
🛠️ Minimal logging fix
except MemoryError, RecursionError: + logger.error( + MEMORY_EMBEDDER_SETTINGS_READ_FAILED, + reason="fatal_exception", + exc_info=True, + ) raiseAs per coding guidelines: "All error paths must log at WARNING or ERROR with context before raising".
1-26: LGTM!Module setup follows all guidelines: proper logger instantiation with
get_logger(__name__), event constants imported from domain-specific observability module, and no prohibited imports.
29-46: LGTM!Frozen config model correctly uses
NotBlankStr | Nonefor identifier fields,allow_inf_nan=False, and properge=1constraint ondims.
49-58: LGTM!Controller class is properly guarded with CEO/SYSTEM role requirements and follows Litestar controller conventions.
94-109: LGTM!Simple status endpoint with correct typing and documentation.
111-155: LGTM for overall endpoint structure.The
get_active_embedderimplementation correctly:
- Handles missing settings service gracefully
- Normalizes empty strings to
None- Uses PEP 758 exception syntax
- Logs warnings on parse failures with structured context
Note: Per context snippets,
embedder_provideris never written bycomplete_setup()(onlyembedder_modelandembedder_dimsare), soproviderwill always beNoneunless manually configured. This is technically correct behavior but may surprise API consumers.
…#966) - Add LMEB ranking data + selector for auto-selecting best embedding model from connected providers (deployment tier inference included) - Add EmbedderOverrideConfig to CompanyMemoryConfig for YAML/settings overrides (model requires dims validator) - Add 3 embedder settings (advanced level) to memory namespace - Add memory dict field to CompanyTemplate for template-level overrides - Add resolve_embedder_config() priority chain: settings > YAML > auto - Wire auto_select_embedder() into setup wizard complete_setup() - Remove _reject_unimplemented_fine_tune validator from Mem0EmbedderConfig - Add _resolve_effective_model() to build_mem0_config_dict() for checkpoint lookup when fine_tune.enabled=True - Add MemoryAdminController with fine-tune + embedder endpoints - Add fine-tuning pipeline stage stubs (4 stages, NotImplementedError) - Add fine-tune optional dependency group in pyproject.toml - Update docs to remove 'not yet implemented' language - Add 4 embedding event constants to observability Closes #965 Closes #966
Pre-reviewed by 4 agents, 15 findings addressed: - Add event constants for bare string log calls in MemoryAdminController - Add MemoryError/RecursionError re-raise guards in exception handlers - Add logging + exc_info to _collect_model_ids and complete_setup handlers - Fix _merge_override to use 'is not None' instead of truthiness - Add logger to selector.py (business logic module) - Add logging before raises in fine_tune._require_not_blank - Update CLAUDE.md Package Structure with embedding/ subpackage - Update design spec with validation constraint docs - Add MemoryAdminController endpoint + model tests - Add missing blank-input tests for pipeline stages - Add settings+YAML priority test for resolve - Verify actual persisted values in setup embedding tests
…nd CodeRabbit - Fix provider/model conflation: stop persisting embedder_provider from auto-selection (only persist model + dims) - Add MEMORY_FINE_TUNE_VALIDATION_FAILED event constant, replace bare string - Implement checkpoint fallback: _resolve_effective_model checks Path.exists(), falls back to base model with MEMORY_EMBEDDER_CHECKPOINT_MISSING warning - Replace type:ignore with explicit guard in _resolve_effective_model - Add dims-requires-model validator to EmbedderOverrideConfig - Replace dict[str,Any] with typed TemplateMemoryConfig in CompanyTemplate - Add path traversal validation to FineTuneRequest (source_dir, output_dir) - Add FineTuneStatus stage/progress/error invariant validator - Change FineTuneStatus.error to NotBlankStr | None - Create ActiveEmbedderResponse DTO for get_active_embedder endpoint - Fix separate dims ValueError handling in get_active_embedder - Remove _ = state stubs, use ARG002 noqa - Fix bidirectional matching in selector (remove too-permissive branch) - Fix infer_deployment_tier docstring (GPU_FULL for unknown, not GPU_CONSUMER) - Add DEBUG logging to selector for match traceability - Split resolve_embedder_config into smaller helpers (_auto_select_from_lmeb) - Rename _merge_override params from auto_* to fallback_* - Move _collect_model_ids from setup.py to setup_helpers.py (file under 800 lines) - Guard auto_select_embedder set() calls with try/except per docstring contract - Fix fine_tune.py docstring (remove false ImportError guard claim) - Add logging before epochs validation ValueError - Pin fine-tune deps to == (torch==2.11.0, sentence-transformers==5.3.0) - Add estimated-scores note to LMEB rankings comment block - Update stale docs (decisions.md, open-questions.md, CLAUDE.md api/ description) - Add tests: checkpoint fallback, tier-filtered miss, path traversal, stage invariants, ActiveEmbedderResponse, dims-requires-model rejection - Create follow-up issue #1001 for pipeline implementation
scikit-learn (BSD-3-Clause AND scancode artifact), torch (BSD-style), and NVIDIA CUDA packages (proprietary, freely redistributable) all have null or compound license metadata that the dependency-review action cannot parse. These are optional fine-tune deps only installed via synthorg[fine-tune].
YAML folded scalars (>-) treat # as literal text, not comments. Move the fine-tune dep explanation to a proper YAML comment above the allow-dependencies-licenses key.
- Fix selector.py: use MEMORY_EMBEDDER_AUTO_SELECTED (not _FAILED) for match-found debug log -- success path was emitting failure event - Normalize empty-string settings to None in get_active_embedder - Use ft.base_model (not embedder.model) for checkpoint-missing fallback - Add extra="forbid" to EmbedderOverrideConfig to catch key typos - Add TODO(#1001) for provider_preset_name/has_gpu forwarding
- Use NotBlankStr for ActiveEmbedderResponse provider/model fields - Use distinct model names in checkpoint fallback test to verify ft.base_model is used (not embedder.model) - Guard next() with default in test_selector.py tier lookups
dbfdd7c to
908bb87
Compare
🤖 I have created a release *beep* *boop* --- ## [0.5.8](v0.5.7...v0.5.8) (2026-04-03) ### Features * auto-select embedding model + fine-tuning pipeline wiring ([#999](#999)) ([a4cbc4e](a4cbc4e)), closes [#965](#965) [#966](#966) * ceremony scheduling batch 3 -- milestone strategy, template defaults, department overrides ([#1019](#1019)) ([321d245](321d245)) * five-pillar evaluation framework for HR performance tracking ([#1017](#1017)) ([5e66cbd](5e66cbd)), closes [#699](#699) * populate comparison page with 53 competitor entries ([#1000](#1000)) ([5cb232d](5cb232d)), closes [#993](#993) * throughput-adaptive and external-trigger ceremony scheduling strategies ([#1003](#1003)) ([bb5c9a4](bb5c9a4)), closes [#973](#973) [#974](#974) ### Bug Fixes * eliminate backup service I/O from API test lifecycle ([#1015](#1015)) ([08d9183](08d9183)) * update run_affected_tests.py to use -n 8 ([#1014](#1014)) ([3ee9fa7](3ee9fa7)) ### Performance * reduce pytest parallelism from -n auto to -n 8 ([#1013](#1013)) ([43e0707](43e0707)) ### CI/CD * bump docker/login-action from 4.0.0 to 4.1.0 in the all group ([#1027](#1027)) ([e7e28ec](e7e28ec)) * bump wrangler from 4.79.0 to 4.80.0 in /.github in the all group ([#1023](#1023)) ([1322a0d](1322a0d)) ### Maintenance * bump github.com/mattn/go-runewidth from 0.0.21 to 0.0.22 in /cli in the all group ([#1024](#1024)) ([b311694](b311694)) * bump https://github.com/astral-sh/ruff-pre-commit from v0.15.8 to 0.15.9 in the all group ([#1022](#1022)) ([1650087](1650087)) * bump node from `71be405` to `387eebd` in /docker/sandbox in the all group ([#1021](#1021)) ([40bd2f6](40bd2f6)) * bump node from `cf38e1f` to `ad82eca` in /docker/web in the all group ([#1020](#1020)) ([f05ab9f](f05ab9f)) * bump the all group in /web with 3 updates ([#1025](#1025)) ([21d40d3](21d40d3)) * bump the all group with 2 updates ([#1026](#1026)) ([36778de](36778de)) * enable additional eslint-react rules and fix violations ([#1028](#1028)) ([80423be](80423be)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
Implements embedding model auto-selection from LMEB rankings (#965) and wires the fine-tuning checkpoint lookup into the Mem0 adapter (#966).
#965: Auto-Select Embedding Model
memory/embedding/rankings.py): 6 models with per-memory-type NDCG@10 scores, deployment tier classification, and output dimensionsmemory/embedding/selector.py):select_embedding_model()intersects available models with LMEB rankings (substring + case-insensitive match for Ollama tags);infer_deployment_tier()maps provider presets to GPU_FULL/GPU_CONSUMER/CPUmemory/embedding/resolve.py):resolve_embedder_config()priority chain -- settings DB > YAML config override > auto-selection with tier-filtered fallbackEmbedderOverrideConfigonCompanyMemoryConfig(model requires dims validator); 3 new ADVANCED-level settings in memory namespace;memorydict field onCompanyTemplateauto_select_embedder()wired intocomplete_setup()-- best-effort, does not block setup on failure#966: Fine-Tuning Pipeline Wiring
_reject_unimplemented_fine_tunevalidator; added_resolve_effective_model()tobuild_mem0_config_dict()-- whenfine_tune.enabled=True, uses checkpoint path as model identifiermemory/embedding/fine_tune.py): 4-stage async functions (generate data, mine negatives, contrastive train, deploy) with input validation andNotImplementedError-- actual ML logic deferred to whensynthorg[fine-tune]extra is installedapi/controllers/memory.py):MemoryAdminControllerat/admin/memory/withPOST /fine-tune,GET /fine-tune/status,GET /embedderendpoints (CEO/SYSTEM role guard)[project.optional-dependencies]fine-tune group (torch, sentence-transformers)Test Plan
Review Coverage
All 15 valid findings implemented. Key fixes: event constants for log calls, MemoryError/RecursionError re-raise guards, explicit
is not Nonemerge logic, missing logger in selector, test coverage for controller/models/edge cases.Closes #965
Closes #966