fix(backend): exclusion mutuelle des backends OpenCode (llamacpp vs cloud) (#97)
Les builders `with_opencode` / `with_opencode_provider` s'évacuent réciproquement (dernier appel gagnant) pour honorer l'invariant `opencode_backend_is_consistent`. Le use case SaveOpenCodeProviderProfile route désormais via le builder au lieu de muter le champ directement — c'est ce qui dupliquait `opencode` + `opencodeProvider` et forçait un repli sur llamacpp même quand l'utilisateur choisissait un provider cloud. Défense en profondeur côté store : - `read_doc` répare les profils corrompus sur disque (drop du stale `opencode`) - `save` refuse de persister un profil violant l'invariant via le nouveau variant `StoreError::Invalid` (mappé vers `AppError::Invalid`) Tests verts : builders last-wins (domain), save rejets + read repair (infra, profile_store 10/10). Refs #97
This commit is contained in:
@ -361,8 +361,12 @@ impl SaveOpenCodeProviderProfile {
|
|||||||
provider = provider.with_custom(custom);
|
provider = provider.with_custom(custom);
|
||||||
}
|
}
|
||||||
|
|
||||||
let mut profile = input.profile;
|
// Route through the builder (ticket #97): `with_opencode_provider` clears
|
||||||
profile.opencode_provider = Some(provider);
|
// 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?;
|
self.profile_store.save(&profile).await?;
|
||||||
Ok(SaveOpenCodeProviderProfileOutput { profile })
|
Ok(SaveOpenCodeProviderProfileOutput { profile })
|
||||||
|
|||||||
@ -134,6 +134,7 @@ impl From<StoreError> for AppError {
|
|||||||
fn from(e: StoreError) -> Self {
|
fn from(e: StoreError) -> Self {
|
||||||
match e {
|
match e {
|
||||||
StoreError::NotFound => Self::NotFound("store item".to_owned()),
|
StoreError::NotFound => Self::NotFound("store item".to_owned()),
|
||||||
|
StoreError::Invalid(message) => Self::Invalid(message),
|
||||||
other => Self::Store(other.to_string()),
|
other => Self::Store(other.to_string()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -796,6 +796,12 @@ pub enum StoreError {
|
|||||||
/// Underlying I/O error.
|
/// Underlying I/O error.
|
||||||
#[error("store io failed: {0}")]
|
#[error("store io failed: {0}")]
|
||||||
Io(String),
|
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`].
|
/// Errors from the [`MemoryStore`].
|
||||||
|
|||||||
@ -1127,18 +1127,28 @@ impl AgentProfile {
|
|||||||
self
|
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]
|
#[must_use]
|
||||||
pub fn with_opencode(mut self, config: OpenCodeConfig) -> Self {
|
pub fn with_opencode(mut self, config: OpenCodeConfig) -> Self {
|
||||||
self.opencode = Some(config);
|
self.opencode = Some(config);
|
||||||
|
self.opencode_provider = None;
|
||||||
self
|
self
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Builder : fixe la configuration OpenCode **cloud** (provider BUILT-IN,
|
/// 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]
|
#[must_use]
|
||||||
pub fn with_opencode_provider(mut self, config: OpenCodeProviderConfig) -> Self {
|
pub fn with_opencode_provider(mut self, config: OpenCodeProviderConfig) -> Self {
|
||||||
self.opencode_provider = Some(config);
|
self.opencode_provider = Some(config);
|
||||||
|
self.opencode = None;
|
||||||
self
|
self
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -1474,13 +1484,47 @@ mod mcp_tests {
|
|||||||
let neither = profile_without_mcp().with_structured_adapter(StructuredAdapter::OpenCode);
|
let neither = profile_without_mcp().with_structured_adapter(StructuredAdapter::OpenCode);
|
||||||
assert!(neither.opencode_backend_is_consistent());
|
assert!(neither.opencode_backend_is_consistent());
|
||||||
|
|
||||||
let both = profile_without_mcp()
|
let mut both = profile_without_mcp()
|
||||||
.with_structured_adapter(StructuredAdapter::OpenCode)
|
.with_structured_adapter(StructuredAdapter::OpenCode)
|
||||||
.with_opencode(local)
|
.with_opencode(local.clone());
|
||||||
.with_opencode_provider(cloud);
|
// 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());
|
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]
|
#[test]
|
||||||
fn opencode_provider_config_serialises_no_local_model_server_id_leak() {
|
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();
|
let config = OpenCodeProviderConfig::new("openrouter", "some-model", crate::ports::SecretRef::new("secret-openrouter")).unwrap();
|
||||||
|
|||||||
@ -81,10 +81,23 @@ impl FsProfileStore {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Reads and parses the doc, returning an empty default if the file is absent.
|
/// 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<ProfilesDoc, StoreError> {
|
async fn read_doc(&self) -> Result<ProfilesDoc, StoreError> {
|
||||||
match self.fs.read(&self.path()).await {
|
match self.fs.read(&self.path()).await {
|
||||||
Ok(bytes) => {
|
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(domain::ports::FsError::NotFound(_)) => Ok(ProfilesDoc::default()),
|
||||||
Err(e) => Err(StoreError::Io(e.to_string())),
|
Err(e) => Err(StoreError::Io(e.to_string())),
|
||||||
@ -114,6 +127,16 @@ impl ProfileStore for FsProfileStore {
|
|||||||
}
|
}
|
||||||
|
|
||||||
async fn save(&self, profile: &AgentProfile) -> Result<(), StoreError> {
|
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?;
|
let mut doc = self.read_doc().await?;
|
||||||
if let Some(slot) = doc.profiles.iter_mut().find(|p| p.id == profile.id) {
|
if let Some(slot) = doc.profiles.iter_mut().find(|p| p.id == profile.id) {
|
||||||
*slot = profile.clone();
|
*slot = profile.clone();
|
||||||
|
|||||||
@ -6,8 +6,8 @@ use std::path::PathBuf;
|
|||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
use domain::ids::ProfileId;
|
use domain::ids::ProfileId;
|
||||||
use domain::ports::{FileSystem, ProfileStore, RemotePath, StoreError};
|
use domain::ports::{FileSystem, ProfileStore, RemotePath, SecretRef, StoreError};
|
||||||
use domain::profile::{AgentProfile, ContextInjection};
|
use domain::profile::{AgentProfile, ContextInjection, OpenCodeConfig, OpenCodeProviderConfig, StructuredAdapter};
|
||||||
use infrastructure::{FsProfileStore, LocalFileSystem};
|
use infrastructure::{FsProfileStore, LocalFileSystem};
|
||||||
use uuid::Uuid;
|
use uuid::Uuid;
|
||||||
|
|
||||||
@ -168,3 +168,88 @@ async fn profiles_file_is_camelcase_versioned() {
|
|||||||
assert_eq!(entry["contextInjection"]["strategy"], "conventionFile");
|
assert_eq!(entry["contextInjection"]["strategy"], "conventionFile");
|
||||||
assert_eq!(entry["contextInjection"]["target"], "CLAUDE.md");
|
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"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user