[codex] Unify model provider profiles#38
Conversation
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
|
Head SHA: 245adbc SummaryThis PR introduces a unified profile system for managing LLM, embedding, and STT configurations. It adds Findings1. STT options lost during migration if model/base_url not set (Data Loss Risk)In if settings.mistral_transcription_model or settings.mistral_transcription_base_url:
_upsert_profile(...)Users who configured only STT options (max_bytes, chunk_max_seconds, max_chunks, etc.) without explicitly setting model or base_url will have those options silently discarded during migration. The options are captured in Impact: Custom STT timeout/byte limit configurations will be lost on upgrade. Suggestion: Create the STT profile if any STT-related setting is non-None: if (settings.mistral_transcription_model or settings.mistral_transcription_base_url
or any(v is not None for v in stt_options.values())):2. Profile ID collision possible (Correctness)
Impact: Profile switching could target the wrong profile if IDs collide. Suggestion: Add collision detection in 3. Breaking change:
|
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
|
Head SHA: 22104dc No blocking findings found. Review SummaryThis PR introduces a unified provider profile system across LLM/chat, embedding/retrieval, and audio/STT models. The changes are well-architected and maintain backward compatibility through automatic migration of legacy settings. What's Changed (Python surface)
Validation Per PR Description
Observations
Test CoverageNew tests verify:
Verification: Reviewed diff statically against project AGENTS.md conventions (dataclasses with slots, modern type hints, no relative imports). All changes follow existing patterns. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22104dc97b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return "https://api.mistral.ai/v1/embeddings" | ||
| if self.base_url.endswith("/embeddings"): | ||
| return self.base_url | ||
| return f"{self.base_url}/v1/embeddings" |
There was a problem hiding this comment.
If a profile or env var sets OPENPLANTER_EMBEDDINGS_BASE_URL to a provider URL that already includes /v1 (for example https://api.voyageai.com/v1), this appends another /v1 and sends requests to .../v1/v1/embeddings, which will fail with 404/connection errors and disables retrieval for otherwise valid configs. The endpoint builder should treat both root URLs and versioned base URLs as valid inputs.
Useful? React with 👍 / 👎.
| self.base_url.clone() | ||
| } else { | ||
| "https://api.mistral.ai/v1/embeddings" | ||
| format!("{}/v1/embeddings", self.base_url) |
There was a problem hiding this comment.
The desktop/core retrieval client has the same endpoint construction issue: when embeddings_base_url already contains /v1, this path construction produces .../v1/v1/embeddings. Any saved embedding profile that uses a versioned base URL will break retrieval in the desktop app despite valid credentials.
Useful? React with 👍 / 👎.
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
Review SummaryHead SHA: 0908cc4 This PR introduces a unified provider profile system across LLM, embedding, and STT model configurations. The implementation is well-structured, maintains backward compatibility through automatic migration, and consistently updates Python, Rust, and TypeScript codebases. Findings✅ Strengths
|
|
@codex review |
…rofiles # Conflicts: # .github/workflows/mistral-vibe-pr-review.yml # .github/workflows/mistral-vibe-review-gate.yml
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
|
Head SHA: f1e1486 SummaryThis PR introduces a unified profile system for managing model configurations across three modalities: LLM, embedding, and speech-to-text (STT). The change consolidates scattered configuration settings into structured Key ChangesCore Architecture (Python)
TUI (Python)
Desktop (Rust + TypeScript)
CI Workflows
Risk Assessment✅ No Blocking Issues FoundCorrectness: The profile migration logic is well-designed. Legacy settings are preserved and converted to profiles, with active profiles taking precedence over legacy defaults. The Data Loss: Migration is one-way but non-destructive. Existing settings are read and converted to profiles. The Backward Compatibility: The changes are backward compatible:
Concurrency: No new concurrency concerns. Profile switching happens synchronously in the TUI and updates the engine config before use. Security: No new security risks identified. Profile IDs are slugified to prevent injection, and all values are validated through existing normalizers. Minor Observations (Non-Blocking)
Verification
No blocking findings found. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1e1486d8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if args.embeddings_provider is None and not os.getenv("OPENPLANTER_EMBEDDINGS_PROVIDER"): | ||
| active_id = settings.active_profiles.get("embedding") | ||
| profile = settings.active_profile("embedding") | ||
| if active_id and profile: | ||
| _apply_embedding_profile_to_config(cfg, active_id, profile) |
There was a problem hiding this comment.
The embeddings profile gate only checks OPENPLANTER_EMBEDDINGS_PROVIDER, so an active embedding profile is still applied even when OPENPLANTER_EMBEDDINGS_MODEL or OPENPLANTER_EMBEDDINGS_BASE_URL is set. Because AgentConfig.from_env(...) runs before settings hydration, those env-provided model/base URL values get overwritten by the profile, which breaks expected env precedence for headless/runtime deployments that pin a custom embeddings endpoint or model. The STT branch already guards against this pattern by checking all related env vars.
Useful? React with 👍 / 👎.
| pools.setdefault(modality, {}) | ||
| pools[modality].setdefault(selected_id, normalized) | ||
| if make_active or not active.get(modality): |
There was a problem hiding this comment.
_upsert_profile uses setdefault, so once a legacy profile ID (for example openai-default) exists, later changes to default_model_openai/other default_model_* fields do not update that profile during migration. Since default_model_for_provider() now prefers profile models over legacy fields, users can change a legacy default flag and still keep using the stale model from the old profile, making those legacy default settings effectively stop working after first migration.
Useful? React with 👍 / 👎.
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
|
Head SHA: 572a6cc SummaryThis PR introduces a unified provider profile system that consolidates LLM, embedding, and STT configuration into a shared FindingsStrengths
Issues Found1. Security: Duplicate default base URLs in config ✅ Non-blockingThe PR adds default embeddings base URLs in multiple places:
These are constants and safe, but represent duplication. This is acceptable for cross-language boundaries but worth noting for future maintenance. 2. Potential data loss: Profile migration is one-way ✅ Non-blockingThe migration from legacy settings to profiles happens in 3. Edge case: Empty profile pools ✅ Non-blockingWhen no profiles exist for a modality, the code handles this gracefully with empty dicts and 4. Code duplication between Python and Rust ✅ Non-blockingThe profile logic (slugify, infer_provider, default_base_url, default_model) is duplicated across Python and Rust. This is necessary for the architecture but creates a maintenance burden. Consider generating shared TypeScript/JSON schemas in the future, though this is not a blocker for this PR. 5. GitHub workflow fix included ✅ Non-blockingThe PR includes fixes to Verification
AssessmentNo blocking findings found. This is a well-engineered, comprehensive refactoring that maintains backward compatibility while adding significant new functionality. The test coverage is extensive, the migration path is automatic and safe, and the implementation is consistent across the codebase's multiple language boundaries. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 572a6ccce1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| if !has_env_value(&["OPENPLANTER_EMBEDDINGS_PROVIDER"]) { |
There was a problem hiding this comment.
apply_settings_to_config only checks OPENPLANTER_EMBEDDINGS_PROVIDER before applying the active embedding profile, so users who set OPENPLANTER_EMBEDDINGS_MODEL or OPENPLANTER_EMBEDDINGS_BASE_URL without setting provider still get profile values overwritten at startup. This breaks environment-variable precedence and makes model/base URL overrides ineffective whenever an embedding profile is active.
Useful? React with 👍 / 👎.
| .entry(modality.to_string()) | ||
| .or_default() | ||
| .entry(selected_id.clone()) | ||
| .or_insert(normalized); |
There was a problem hiding this comment.
Using or_insert in upsert_profile prevents legacy-migrated profiles like openai-default from updating when default_model_* settings change later. Because default_model_for_provider now prefers profile entries, stale migrated profiles can keep serving an old model even after the corresponding default field is updated.
Useful? React with 👍 / 👎.
| base_url: DEFAULT_STT_BASE_URL, | ||
| auth_ref: "mistral", | ||
| options: { | ||
| max_bytes: 104857600, |
There was a problem hiding this comment.
Saving /stt ... --save writes a hard-coded base URL and fixed chunk/timeout options instead of persisting the currently active STT configuration. If a workspace uses a custom STT endpoint or tuned limits, running this save path silently resets those values and future sessions will transcribe with the wrong settings.
Useful? React with 👍 / 👎.
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
Review SummaryThis PR introduces a unified provider profile system across LLM, embedding, and STT configurations. The changes are substantial and well-structured, with comprehensive test coverage. No blocking issues found. Key Changes Reviewed
Verification
AssessmentThe PR is well-architected with:
No blocking findings found. Head SHA: 741c8a5 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741c8a54db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| args.provider is None | ||
| and args.model is None | ||
| and not os.getenv("OPENPLANTER_PROVIDER") | ||
| and not os.getenv("OPENPLANTER_MODEL") | ||
| ): |
There was a problem hiding this comment.
This guard only checks OPENPLANTER_PROVIDER/OPENPLANTER_MODEL, but _apply_llm_profile_to_config also mutates reasoning_effort, zai_plan, and provider base URLs from profile options. As a result, users who set env overrides like OPENPLANTER_REASONING_EFFORT, OPENPLANTER_ZAI_PLAN, or OPENPLANTER_ZAI_BASE_URL can still have those values silently replaced by the active profile during startup, which breaks the expected env-precedence behavior.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| pub fn apply_settings_to_config(cfg: &mut AgentConfig, settings: &PersistentSettings) { | ||
| if !has_env_value(&["OPENPLANTER_PROVIDER"]) && !has_env_value(&["OPENPLANTER_MODEL"]) { |
There was a problem hiding this comment.
The desktop hydration path applies the active LLM profile whenever provider/model env vars are absent, but apply_llm_profile also sets reasoning/ZAI fields and base URLs. That means explicit env settings for those fields can be overwritten by profile data on launch, diverging from the rest of the config loader’s per-field env-override semantics and causing hard-to-debug config drift.
Useful? React with 👍 / 👎.
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
|
Head SHA: fd37af1 SummaryThis PR introduces a unified provider-profile system across LLM, embedding, and STT modalities. It's a substantial cross-stack refactoring touching Python agent, Rust core/tauri, and TypeScript frontend. The changes are well-tested (69 Python + 21+26+82 Rust/JS tests passing) and maintain backward compatibility through legacy settings migration. FindingsData Loss / Migration RiskFINDING: Legacy settings migration is one-way and irreversible
SecurityFINDING: Credentials are properly separated from profiles
Consistency IssuesFINDING: Duplicate profile application logic across layers
FINDING: Profile ID slugification differs between Python and Rust
Environment Variable PrecedenceFINDING: Inconsistent env var checking in profile application
Frontend State ManagementFINDING: Profile state may desync between frontend and backend
TestingFINDING: Test coverage is comprehensive but could be deeper
Verification
No blocking findings found. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd37af1fad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ctx.cfg.llm_profile_id = profile_id | ||
| ctx.cfg.llm_profile_name = f"{provider} {new_model}" |
There was a problem hiding this comment.
After a saved profile is selected, /model <name> without --save updates ctx.cfg.model but never resets ctx.cfg.llm_profile_id/llm_profile_name, so the TUI continues to report an active profile that no longer matches the running model/provider. This creates misleading /model and status output and can cause users to believe profile-backed settings are still in effect when they are not.
Useful? React with 👍 / 👎.
| ctx.cfg.embedding_profile_id = profile_id | ||
| ctx.cfg.embedding_profile_name = f"{provider.title()} embeddings" |
There was a problem hiding this comment.
The embeddings handler sets embedding_profile_id/name only in the --save path, but a later /embeddings <provider> without --save never clears those fields. As a result, the UI can show a stale embedding profile while runtime settings have moved to an unsaved provider/model pair, which is inconsistent and confusing during retrieval debugging.
Useful? React with 👍 / 👎.
| ctx.cfg.stt_profile_id = profile_id | ||
| ctx.cfg.stt_profile_name = profile_name |
There was a problem hiding this comment.
When /stt <model> is used without --save, the transcription model changes but stt_profile_id/name remains whatever was last saved or selected, because those fields are only assigned in the save branch. This leaves status/profile displays out of sync with the active STT configuration and makes profile troubleshooting error-prone.
Useful? React with 👍 / 👎.
ThomsenDrake
temporarily deployed
to
MISTRAL_API_KEY
GitHub Actions
Inactive
Review SummaryNo blocking findings found. This PR introduces a unified provider profile system across LLM, embedding, and STT modalities. The implementation is comprehensive and well-tested. What Changed
Strengths
Verification
Head SHA: c3c2c0f |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3c2c0f960
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| provider, | ||
| adapter: provider === "anthropic" ? "anthropic" : "openai-compatible", | ||
| model, | ||
| auth_ref: provider, | ||
| options: {}, |
There was a problem hiding this comment.
Saving a model profile here writes only provider/adapter/model/auth_ref and an empty options, so any existing profile with the same ID loses fields like base_url (and option metadata) when save_settings merges by replacing map entries. This breaks profile fidelity for custom endpoints: after saving and later switching back to that profile, requests can go to the wrong default URL instead of the user’s configured endpoint.
Useful? React with 👍 / 👎.
| provider: config.provider, | ||
| model: config.model, | ||
| llmProfileId: config.llm_profile_id, | ||
| llmProfileName: config.llm_profile_name, | ||
| zaiPlan: config.zai_plan, |
There was a problem hiding this comment.
Profile switching can change reasoning_effort via backend profile application, but this state update ignores config.reasoning_effort. As a result, frontend status surfaces that read appState.reasoningEffort can display stale reasoning until a full config reload, even though the backend is already using the new profile’s reasoning setting.
Useful? React with 👍 / 👎.
|
Head SHA: 3ea6fbb Review SummaryThis PR introduces a unified provider profile system across LLM, embedding, and STT modalities. The changes are extensive (34 files, +3534/-64 lines) and touch Python, Rust, TypeScript, and workflow files. The core architecture is sound and the migration strategy is well-executed. Strengths
Findings1. Security: Credential handling is correct ✅
2. Data Migration: Legacy settings preserved ✅
3. Race/CONCURRENCY: No issues found ✅
4. User-Facing: Command surface expanded ✅
5. Potential Issue: Profile ID collision resolution
|
1 similar comment
|
Head SHA: 3ea6fbb Review SummaryThis PR introduces a unified provider profile system across LLM, embedding, and STT modalities. The changes are extensive (34 files, +3534/-64 lines) and touch Python, Rust, TypeScript, and workflow files. The core architecture is sound and the migration strategy is well-executed. Strengths
Findings1. Security: Credential handling is correct ✅
2. Data Migration: Legacy settings preserved ✅
3. Race/CONCURRENCY: No issues found ✅
4. User-Facing: Command surface expanded ✅
5. Potential Issue: Profile ID collision resolution
|
|
@codex review |
|
@codex review latest head |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Overhauls model configuration into a shared provider-profile system across LLM/chat, embedding/retrieval, and audio/STT models.
What Changed
llm,embedding, andstt./model,/embeddings, and/sttprofiles across CLI/TUI/Desktop command surfaces.auth_ref.Validation
PYTHONPATH=. uv run pytest tests/test_settings.py tests/test_retrieval.py tests/test_audio_transcribe.py-> 69 passed/opt/homebrew/bin/ruff check agent/tui.py agent/settings.py tests/test_settings.pycargo fmt --checkcargo test -p op-core settings-> 21 passedcargo test -p op-core retrieval-> 16 passedcargo test -p op-tauri config-> 26 passednpm test -- --run src/commands/model.test.ts src/commands/slash.test.ts src/components/StatusBar.test.ts-> 82 passednpm run buildgit diff --checkNotes
uvrewroteuv.lockduring test runs because of the global exclude-newer setting; the lockfile was restored afterward and is not included in this PR.