Skip to content

fix(model): keep provider/model in sync when switching models#2873

Open
chindris-mihai-alexandru wants to merge 5 commits intoantinomyhq:mainfrom
chindris-mihai-alexandru:fix/model-provider-switch-sync
Open

fix(model): keep provider/model in sync when switching models#2873
chindris-mihai-alexandru wants to merge 5 commits intoantinomyhq:mainfrom
chindris-mihai-alexandru:fix/model-provider-switch-sync

Conversation

@chindris-mihai-alexandru
Copy link
Copy Markdown

@chindris-mihai-alexandru chindris-mihai-alexandru commented Apr 6, 2026

Summary

This PR fixes a model switching bug where /model could select a model from a different provider but keep the previous provider active, leading to invalid provider/model pairs and runtime 404s.

Changes

  • crates/forge_main/src/ui.rs

    • select_model now returns (ProviderId, ModelId) instead of only ModelId
    • /model now updates provider+model atomically when the selected model belongs to a different provider
    • preserves same-provider behavior (model-only update)
    • improves cursor restore by matching provider+model pair
  • crates/forge_services/src/app_config.rs

    • set_default_provider_and_model now updates the in-memory session cache after persisting
    • adds test: test_set_default_provider_and_model_updates_session_cache
  • crates/forge_services/src/agent_registry.rs

    • agent registry config is now refreshable instead of a fixed startup snapshot
    • reload_agents() refreshes config from disk/env before rebuilding agents
    • adds test: test_reload_agents_refreshes_provider_model_from_config_file

Repro (before)

  1. Set default provider to Modal
  2. Use /model to select a model from another provider (e.g. Claude)
  3. UI reports model switched, but requests are sent with stale provider/model pairing
  4. Modal returns 404 Unknown model

Behavior (after)

  • Selecting a cross-provider model via /model now switches both provider and model together
  • Active agent/default config stay in sync with persisted config

Validation

  • cargo check -p forge_main -p forge_services
  • cargo test -p forge_main -- --nocapture
  • cargo test -p forge_services -- --nocapture
  • cargo test -p forge_api -- --nocapture

Co-Authored-By: ForgeCode noreply@forgecode.dev

chindris-mihai-alexandru and others added 4 commits April 6, 2026 21:34
Add AdaL CLI by SylphAI as a new provider option in ForgeCode. AdaL is
registered as an OpenAI-compatible provider with api.sylph.ai endpoints,
enabling ForgeCode users to access AdaL's models via API key auth.

Changes:
- Add 'adal' entry to provider.json with OpenAI response type
- Add ProviderId::ADAL constant to forge_domain
- Wire up display_name ("AdaL"), FromStr, and built_in_providers()
- Add missing FromStr arms for vertex_ai_anthropic, bedrock, opencode_zen
- Add unit tests for ADAL display name, from_str, and built_in_providers

Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Add Modal as a new provider in ForgeCode, enabling access to Z.ai's
GLM-5 745B parameter model hosted on Modal's infrastructure. Modal
offers an OpenAI-compatible API endpoint with free GLM-5 access
until April 30th, 2026.

Changes:
- Add 'modal' entry to provider.json with GLM-5-FP8 model
- Add ProviderId::MODAL constant to forge_domain
- Wire up display_name ("Modal"), FromStr, and built_in_providers()
- Add unit tests for MODAL display name, from_str, and built_in_providers

Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Combines both provider branches to enable AdaL (SylphAI) and Modal (GLM-5) support.

Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Ensure /model selection updates provider+model atomically when the chosen model
belongs to a different provider, and refresh agent defaults from latest config
on reload.

Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Copilot AI review requested due to automatic review settings April 6, 2026 22:08
@github-actions github-actions bot added the type: fix Iterations on existing features or infrastructure. label Apr 6, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug where selecting a model from a different provider could leave the previous provider active, producing invalid provider/model pairs and resulting request failures.

Changes:

  • Update model selection flow to return (ProviderId, ModelId) and switch provider+model together when crossing providers.
  • Keep in-memory session config cache in sync after persisting provider/model changes.
  • Make the agent registry refresh its config on reload so provider/model changes apply immediately.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/forge_services/src/app_config.rs Updates the in-memory session cache after persisting provider/model changes; adds test coverage.
crates/forge_services/src/agent_registry.rs Stores config in a runtime RwLock and refreshes config from disk/env during reload_agents; adds test.
crates/forge_repo/src/provider/provider.json Adds new provider definitions for adal and modal (including a Modal-hosted model).
crates/forge_main/src/ui.rs Changes model picker to select provider+model pairs and applies atomic updates when switching providers.
crates/forge_domain/src/provider.rs Adds built-in provider IDs for adal and modal and updates parsing/display tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +43 to +48
/// Reloads Forge config from defaults, global file, legacy file, and env.
fn load_current_config(&self) -> anyhow::Result<ForgeConfig> {
ConfigReader::default()
.read_defaults()
.read_global()
.read_legacy()
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_current_config layers config sources in a different precedence order than the rest of the codebase (e.g. ForgeConfig::read() uses legacy -> defaults -> global -> env). Calling read_legacy() after read_global() means legacy values can override the primary TOML config during reload_agents, leading to surprising provider/model resolution after switching. Align the read order with crates/forge_config/src/config.rs:279-285 (or call ForgeConfig::read()/a shared helper) so reload semantics match startup semantics.

Suggested change
/// Reloads Forge config from defaults, global file, legacy file, and env.
fn load_current_config(&self) -> anyhow::Result<ForgeConfig> {
ConfigReader::default()
.read_defaults()
.read_global()
.read_legacy()
/// Reloads Forge config using the same precedence as startup:
/// legacy file, defaults, global file, and env.
fn load_current_config(&self) -> anyhow::Result<ForgeConfig> {
ConfigReader::default()
.read_legacy()
.read_defaults()
.read_global()

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@chindris-mihai-alexandru chindris-mihai-alexandru Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 069e75a. load_current_config() now delegates to ForgeConfig::read() so reload order matches startup precedence exactly (legacy -> defaults -> global -> env).

Comment on lines +220 to +223
let fixture_original_home = std::env::var("HOME").ok();
unsafe {
std::env::set_var("HOME", fixture_home.path());
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test mutates the process-wide HOME env var but only restores it at the end of the function. If an assertion/unwrap panics, HOME won’t be restored, which can cascade into unrelated test failures despite the mutex. Prefer an RAII guard (similar to EnvGuard in crates/forge_config/src/reader.rs) or scopeguard so env restoration happens in Drop even on early returns/panics.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@chindris-mihai-alexandru chindris-mihai-alexandru Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 069e75a. Added a HomeEnvGuard RAII helper in the test module that captures and restores HOME in Drop, and also holds the mutex lock for serialized env mutation. This guarantees cleanup even on panic.

…est guard

Addresses Copilot review feedback by using ForgeConfig::read() for reload
semantics and restoring HOME via RAII in tests.

Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix Iterations on existing features or infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants