Merge feature/ticket97-opencode-provider-mutual-exclusion into develop
This commit is contained in:
@ -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 })
|
||||
|
||||
@ -134,6 +134,7 @@ impl From<StoreError> 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()),
|
||||
}
|
||||
}
|
||||
|
||||
@ -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`].
|
||||
|
||||
@ -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();
|
||||
|
||||
@ -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<ProfilesDoc, StoreError> {
|
||||
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();
|
||||
|
||||
@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user