diff --git a/crates/application/src/agent/usecases.rs b/crates/application/src/agent/usecases.rs index fc45422..a459701 100644 --- a/crates/application/src/agent/usecases.rs +++ b/crates/application/src/agent/usecases.rs @@ -361,8 +361,12 @@ impl SaveOpenCodeProviderProfile { provider = provider.with_custom(custom); } - let mut profile = input.profile; - profile.opencode_provider = Some(provider); + // Route through the builder (ticket #97): `with_opencode_provider` clears + // any stale `opencode` (llamacpp) carried by the input profile, so the + // persisted profile honours the mutual-exclusion invariant instead of + // keeping both backends — which previously forced a wrong llamacpp + // fallback even when the user chose a cloud provider. + let profile = input.profile.with_opencode_provider(provider); self.profile_store.save(&profile).await?; Ok(SaveOpenCodeProviderProfileOutput { profile }) diff --git a/crates/application/src/error.rs b/crates/application/src/error.rs index 779851d..7fcdc09 100644 --- a/crates/application/src/error.rs +++ b/crates/application/src/error.rs @@ -134,6 +134,7 @@ impl From for AppError { fn from(e: StoreError) -> Self { match e { StoreError::NotFound => Self::NotFound("store item".to_owned()), + StoreError::Invalid(message) => Self::Invalid(message), other => Self::Store(other.to_string()), } } diff --git a/crates/domain/src/ports.rs b/crates/domain/src/ports.rs index b74e1c8..5cccfcd 100644 --- a/crates/domain/src/ports.rs +++ b/crates/domain/src/ports.rs @@ -796,6 +796,12 @@ pub enum StoreError { /// Underlying I/O error. #[error("store io failed: {0}")] Io(String), + /// A persisted item violated a domain invariant — surfaced by defense-in-depth + /// guards (e.g. [`crate::profile::AgentProfile::opencode_backend_is_consistent`], + /// ticket #97). Mapped upstream to an *invalid input* error (not a generic + /// store failure) so callers can distinguish invariant violations. + #[error("invalid persisted item: {0}")] + Invalid(String), } /// Errors from the [`MemoryStore`]. diff --git a/crates/domain/src/profile.rs b/crates/domain/src/profile.rs index cde96e3..8c49285 100644 --- a/crates/domain/src/profile.rs +++ b/crates/domain/src/profile.rs @@ -1127,18 +1127,28 @@ impl AgentProfile { self } - /// Builder : fixe la configuration OpenCode process-backed. + /// Builder : fixe la configuration OpenCode process-backed (provider custom + /// `llamacpp`). Garantit l'invariant [`Self::opencode_backend_is_consistent`] + /// (ticket #97) : positionner le backend local évacue tout backend cloud + /// précédemment fixé — les deux configurent le même champ `opencode.json` + /// `provider`/`model` et n'ont aucun sens combinées. Dernier appel gagnant. #[must_use] pub fn with_opencode(mut self, config: OpenCodeConfig) -> Self { self.opencode = Some(config); + self.opencode_provider = None; self } /// Builder : fixe la configuration OpenCode **cloud** (provider BUILT-IN, - /// ticket #92, lot B1). + /// ticket #92, lot B1). Garantit l'invariant + /// [`Self::opencode_backend_is_consistent`] (ticket #97) : positionner le + /// backend cloud évacue tout backend local (`llamacpp`) précédemment fixé — + /// sans quoi le stale `opencode` forcerait un repli sur llamacpp au lieu du + /// provider cloud choisi par l'utilisateur. Dernier appel gagnant. #[must_use] pub fn with_opencode_provider(mut self, config: OpenCodeProviderConfig) -> Self { self.opencode_provider = Some(config); + self.opencode = None; self } @@ -1474,13 +1484,47 @@ mod mcp_tests { let neither = profile_without_mcp().with_structured_adapter(StructuredAdapter::OpenCode); assert!(neither.opencode_backend_is_consistent()); - let both = profile_without_mcp() + let mut both = profile_without_mcp() .with_structured_adapter(StructuredAdapter::OpenCode) - .with_opencode(local) - .with_opencode_provider(cloud); + .with_opencode(local.clone()); + // The builders now enforce mutual exclusion (ticket #97), so an + // inconsistent profile is only reachable via direct mutation or a + // corrupted on-disk file — simulate that to exercise the predicate. + both.opencode_provider = Some(cloud.clone()); assert!(!both.opencode_backend_is_consistent()); } + #[test] + fn opencode_builders_enforce_mutual_exclusion_last_wins() { + let local = OpenCodeConfig::new( + "http://localhost:8080/v1", + None, + "qwen3-coder-30b", + None, + None, + ) + .unwrap(); + let cloud = OpenCodeProviderConfig::new("anthropic", "claude-sonnet-5", crate::ports::SecretRef::new("secret-cloud")).unwrap(); + + // Setting the cloud provider clears a previously-set local one. + let cloud_wins = profile_without_mcp() + .with_structured_adapter(StructuredAdapter::OpenCode) + .with_opencode(local.clone()) + .with_opencode_provider(cloud.clone()); + assert!(cloud_wins.opencode_backend_is_consistent()); + assert!(cloud_wins.opencode.is_none(), "stale local backend dropped"); + assert_eq!(cloud_wins.opencode_provider.as_ref().unwrap().provider_id, "anthropic"); + + // Setting the local provider clears a previously-set cloud one. + let local_wins = profile_without_mcp() + .with_structured_adapter(StructuredAdapter::OpenCode) + .with_opencode_provider(cloud) + .with_opencode(local); + assert!(local_wins.opencode_backend_is_consistent()); + assert!(local_wins.opencode_provider.is_none(), "stale cloud backend dropped"); + assert!(local_wins.opencode.is_some()); + } + #[test] fn opencode_provider_config_serialises_no_local_model_server_id_leak() { let config = OpenCodeProviderConfig::new("openrouter", "some-model", crate::ports::SecretRef::new("secret-openrouter")).unwrap(); diff --git a/crates/infrastructure/src/store/profile.rs b/crates/infrastructure/src/store/profile.rs index 59f454a..77c2828 100644 --- a/crates/infrastructure/src/store/profile.rs +++ b/crates/infrastructure/src/store/profile.rs @@ -81,10 +81,23 @@ impl FsProfileStore { } /// Reads and parses the doc, returning an empty default if the file is absent. + /// + /// Runs a one-pass **repair** (ticket #97): a profile corrupted on disk may + /// carry both `opencode` (llamacpp) and `opencodeProvider` (cloud). The cloud + /// provider is the user's actual intent (the bug duplicated the local backend + /// even when a provider was chosen), so the stale `opencode` is dropped. This + /// makes the on-disk state converge with the now-enforced builder invariant. async fn read_doc(&self) -> Result { match self.fs.read(&self.path()).await { Ok(bytes) => { - serde_json::from_slice(&bytes).map_err(|e| StoreError::Serialization(e.to_string())) + let mut doc: ProfilesDoc = + serde_json::from_slice(&bytes).map_err(|e| StoreError::Serialization(e.to_string()))?; + for profile in &mut doc.profiles { + if !profile.opencode_backend_is_consistent() { + profile.opencode = None; + } + } + Ok(doc) } Err(domain::ports::FsError::NotFound(_)) => Ok(ProfilesDoc::default()), Err(e) => Err(StoreError::Io(e.to_string())), @@ -114,6 +127,16 @@ impl ProfileStore for FsProfileStore { } async fn save(&self, profile: &AgentProfile) -> Result<(), StoreError> { + // Defense-in-depth (ticket #97): refuse to persist a profile that violates + // the OpenCode backend mutual-exclusion invariant. The application layer + // now routes through the builders (which enforce exclusion), so this guard + // only ever fires for a future/buggy caller that mutates fields directly. + if !profile.opencode_backend_is_consistent() { + return Err(StoreError::Invalid(format!( + "profile `{}` carries both `opencode` and `opencodeProvider` (mutually exclusive)", + profile.name + ))); + } let mut doc = self.read_doc().await?; if let Some(slot) = doc.profiles.iter_mut().find(|p| p.id == profile.id) { *slot = profile.clone(); diff --git a/crates/infrastructure/tests/profile_store.rs b/crates/infrastructure/tests/profile_store.rs index ca462d6..e0c191e 100644 --- a/crates/infrastructure/tests/profile_store.rs +++ b/crates/infrastructure/tests/profile_store.rs @@ -6,8 +6,8 @@ use std::path::PathBuf; use std::sync::Arc; use domain::ids::ProfileId; -use domain::ports::{FileSystem, ProfileStore, RemotePath, StoreError}; -use domain::profile::{AgentProfile, ContextInjection}; +use domain::ports::{FileSystem, ProfileStore, RemotePath, SecretRef, StoreError}; +use domain::profile::{AgentProfile, ContextInjection, OpenCodeConfig, OpenCodeProviderConfig, StructuredAdapter}; use infrastructure::{FsProfileStore, LocalFileSystem}; use uuid::Uuid; @@ -168,3 +168,88 @@ async fn profiles_file_is_camelcase_versioned() { assert_eq!(entry["contextInjection"]["strategy"], "conventionFile"); assert_eq!(entry["contextInjection"]["target"], "CLAUDE.md"); } + +/// Fixture pair for ticket #97: the local llama.cpp backend (`opencode`) and the +/// cloud provider (`opencodeProvider`) — the two mutually exclusive OpenCode +/// backends. A profile carrying both is inconsistent. +fn opencode_backends() -> (OpenCodeConfig, OpenCodeProviderConfig) { + let local = + OpenCodeConfig::new("http://localhost:8080/v1", None, "qwen3-coder-30b", None, None).unwrap(); + let cloud = + OpenCodeProviderConfig::new("anthropic", "claude-sonnet-5", SecretRef::new("secret-cloud")) + .unwrap(); + (local, cloud) +} + +/// Ticket #97 — read repair (`read_doc`, profile.rs:90-105): a profile corrupted +/// on disk (both `opencode` and `opencodeProvider` present) is silently repaired +/// on read by dropping the stale `opencode`, converging with the enforced +/// builder invariant. The cloud provider — the user's actual intent — is kept. +#[tokio::test] +async fn read_doc_repairs_stale_opencode_when_both_sections_present() { + let tmp = TempDir::new(); + let (local, cloud) = opencode_backends(); + + // The `save` guard would refuse this, so plant the corruption directly on + // disk (bypassing the guard), exactly like a stale file from an older build. + let mut corrupted = sample(1, "OpenCode Cloud", "opencode") + .with_structured_adapter(StructuredAdapter::OpenCode) + .with_opencode(local); + corrupted.opencode_provider = Some(cloud.clone()); + assert!( + !corrupted.opencode_backend_is_consistent(), + "fixture must violate the invariant" + ); + + let fs = LocalFileSystem::new(); + let doc = serde_json::json!({ "version": 1, "profiles": [corrupted] }); + fs.write(&tmp.child("profiles.json"), &serde_json::to_vec_pretty(&doc).unwrap()) + .await + .unwrap(); + + let store = store(&tmp); + let listed = store.list().await.expect("read repairs instead of failing"); + assert_eq!(listed.len(), 1, "corrupted profile preserved as an entry"); + let repaired = &listed[0]; + assert!(repaired.opencode.is_none(), "stale local `opencode` dropped on read"); + assert_eq!( + repaired.opencode_provider.as_ref().unwrap().provider_id, + "anthropic", + "cloud `opencodeProvider` preserved" + ); + assert!( + repaired.opencode_backend_is_consistent(), + "read repair restored the mutual-exclusion invariant" + ); +} + +/// Ticket #97 — write guard (`save`, profile.rs:129-139): persisting a profile +/// that violates the OpenCode backend mutual-exclusion invariant is rejected +/// with [`StoreError::Invalid`], and nothing is written to disk. +#[tokio::test] +async fn save_rejects_profile_violating_backend_exclusion() { + let tmp = TempDir::new(); + let store = store(&tmp); + let (local, cloud) = opencode_backends(); + + // Only reachable via direct mutation: the builders enforce exclusion + // (last-wins), so an inconsistent profile cannot be built through the API. + let mut inconsistent = sample(1, "OpenCode Both", "opencode") + .with_structured_adapter(StructuredAdapter::OpenCode) + .with_opencode(local); + inconsistent.opencode_provider = Some(cloud); + assert!(!inconsistent.opencode_backend_is_consistent()); + + let err = store + .save(&inconsistent) + .await + .expect_err("save must reject an invariant-violating profile"); + assert!( + matches!(err, StoreError::Invalid(_)), + "defense-in-depth guard maps to Invalid; got {err:?}" + ); + assert!( + !store.is_configured().await.unwrap(), + "rejected save must not materialise profiles.json" + ); +}