From ee44d9b2c0079eb18d2c3df265536d35bb8bc5a3 Mon Sep 17 00:00:00 2001 From: Fini Date: Thu, 30 Jul 2026 08:57:17 +0800 Subject: [PATCH] fix(agent): default ACP agents to basic capability tier --- .../src/chat_design_request.rs | 43 +++++++--- .../src/chat_session_launch.rs | 39 ++++++--- .../src/chat_session_launch_tests.rs | 85 ++++++++++++++++++- .../op-host-services/src/chat_provider_llm.rs | 9 ++ .../src/validation_providers.rs | 35 +++++++- crates/op-orchestrator/src/lib.rs | 2 +- crates/op-orchestrator/src/model_profile.rs | 35 +++++++- 7 files changed, 222 insertions(+), 26 deletions(-) diff --git a/crates/op-host-desktop/src/chat_design_request.rs b/crates/op-host-desktop/src/chat_design_request.rs index b81e9766e..d76fe42a6 100644 --- a/crates/op-host-desktop/src/chat_design_request.rs +++ b/crates/op-host-desktop/src/chat_design_request.rs @@ -1,16 +1,24 @@ use op_editor_core::EditorState; use op_orchestrator::{AppendContext, DesignRequest}; -/// Resolve the selected chat model's id for the orchestrator. Only -/// built-in (API-key) agents expose a concrete model id; CLI/ACP agents -/// pick their own model internally and yield `None` (the CLI-side -/// selection rides `ChatProviderLlmClient::with_model` instead). The id -/// feeds model-aware orchestrator policy — tier-gated skill filtering, -/// the element-manifest routing gate, and the M3 thinking policy — and -/// matches the configuration the ab-v9 benchmarks ran with (op-smoke -/// has always passed `OPENPENCIL_ORCHESTRATOR_MODEL` through). -fn selected_builtin_model(state: &EditorState) -> Option { +/// Resolve the selected chat model's capability id for the orchestrator. +/// Built-in (API-key) agents expose their concrete model id. ACP entries +/// preserve their `acp:` catalog identity so the model-profile resolver +/// can choose its conservative weak-agent default instead of treating a +/// missing id as Full tier. The ACP marker is not a transport model override: +/// `selected_cli_model_id` still yields `None` for ACP providers. +/// +/// Fixed CLI agents keep choosing their own model internally and yield `None` +/// here (the CLI-side selection rides `ChatProviderLlmClient::with_model` +/// instead). The returned id feeds model-aware orchestrator policy — +/// tier-gated skill filtering, the element-manifest routing gate, and the M3 +/// thinking policy — and matches the configuration the ab-v9 benchmarks ran +/// with (op-smoke has always passed `OPENPENCIL_ORCHESTRATOR_MODEL` through). +fn selected_orchestrator_model(state: &EditorState) -> Option { let entry = state.chat.selected_model_entry()?; + if entry.acp_agent_id().is_some() { + return Some(entry.value.clone()); + } let id = entry.builtin_provider_id.as_deref()?; state .editor_ui @@ -29,7 +37,7 @@ pub(crate) fn build_design_request( ) -> DesignRequest { DesignRequest { prompt, - model: selected_builtin_model(state), + model: selected_orchestrator_model(state), provider: None, design_md: state.doc.design_md.clone(), // Detected by `chat_intent::detect_append_intent` when the @@ -160,4 +168,19 @@ mod tests { // M3 thinking policy — must match the agent the session will call. assert_eq!(req.model.as_deref(), Some("MiniMax-M3")); } + + #[test] + fn selected_acp_agent_reaches_the_orchestrator_as_basic_tier() { + let mut state = EditorState::new(); + state.chat.available_models = vec![ModelEntry::acp("custom/vendor", "Custom ACP")]; + state.chat.selected_model = 0; + + let req = build_design_request("draw a dashboard".into(), &state, None); + + assert_eq!(req.model.as_deref(), Some("acp:custom/vendor")); + assert_eq!( + op_orchestrator::resolve_model_profile(req.model.as_deref().unwrap()).tier, + op_orchestrator::ModelTier::Basic + ); + } } diff --git a/crates/op-host-desktop/src/chat_session_launch.rs b/crates/op-host-desktop/src/chat_session_launch.rs index ecd06c4ea..726b83a8f 100644 --- a/crates/op-host-desktop/src/chat_session_launch.rs +++ b/crates/op-host-desktop/src/chat_session_launch.rs @@ -26,6 +26,23 @@ use super::ChatSession; mod chat_design_request; use chat_design_request::build_design_request; +/// Build the model-aware request while the live chat selection is still +/// attached, then take the narrowed worker snapshot. +/// +/// `narrowed_snapshot` deliberately detaches `EditorState::chat`; reversing +/// these two operations therefore erases the selected builtin/ACP identity +/// and makes the orchestrator resolve `model=None` as Full tier. +fn prepare_design_request_and_snapshot( + host: &mut WidgetHostNative, + prompt: String, + append_context: Option, +) -> (op_orchestrator::DesignRequest, EditorState) { + let request = build_design_request(prompt, host.editor_state(), append_context); + let initial_state = + op_editor_core::request_snapshot::narrowed_snapshot(host.editor_state_mut()); + (request, initial_state) +} + // Design-agent-loop helpers (flag gate + provider builder + turn launcher) // split out at the 800-line cap; see module docs there. #[path = "chat_session_launch_design.rs"] @@ -115,14 +132,17 @@ pub fn launch_if_pending( host.editor_state(), &effective_user_text, ); + let (request, initial_state) = prepare_design_request_and_snapshot( + host, + effective_user_text.clone(), + append_context, + ); // Narrowed clone — this becomes the design worker's // `RemoteDocSink` mirror, which is only ever read through // `DocSink::state()` (`active_children` / `doc` / `components`). // See `op_editor_core::request_snapshot` for the field audit. - let initial_state = - op_editor_core::request_snapshot::narrowed_snapshot(host.editor_state_mut()); - let request = - build_design_request(effective_user_text.clone(), &initial_state, append_context); + // The request above must be built first because that snapshot + // intentionally detaches chat/model-selection state. // Persist the request onto the turn's assistant bubble (already // pushed by `begin_send`) BEFORE it moves into the worker — the // manual per-subtask "Retry" button needs it to re-run a failed @@ -420,14 +440,13 @@ fn launch_cli_standard_turn( let system_prompt = build_chat_system_prompt(state, user_text); let modify_plan = op_host_services::chat_intent::build_modify_plan(state, user_text); let append_context = op_host_services::chat_intent::detect_append_intent(state, user_text); + let (design_request, initial_state) = + prepare_design_request_and_snapshot(host, user_text.to_string(), append_context); // Narrowed clone — `CliTurnPlan::initial_state` ends up as the design // worker's `RemoteDocSink` mirror, read only through `DocSink::state()`. - // See `op_editor_core::request_snapshot` for the field audit. Takes the - // mutable borrow, so it must come after the last read of `state`. - let initial_state = - op_editor_core::request_snapshot::narrowed_snapshot(host.editor_state_mut()); - let design_request = - build_design_request(user_text.to_string(), &initial_state, append_context); + // See `op_editor_core::request_snapshot` for the field audit. Preparation + // takes the mutable borrow, so it must come after the last read of `state`; + // it builds the model-aware request before detaching chat for the snapshot. // Same stash as the builtin/design-intent path above — this turn may or // may not actually classify as `DesignIntent::New` on the worker (the // classifier runs async), but setting it unconditionally is harmless: diff --git a/crates/op-host-desktop/src/chat_session_launch_tests.rs b/crates/op-host-desktop/src/chat_session_launch_tests.rs index 8bebd9dfc..b9b02f813 100644 --- a/crates/op-host-desktop/src/chat_session_launch_tests.rs +++ b/crates/op-host-desktop/src/chat_session_launch_tests.rs @@ -2,7 +2,10 @@ //! 800-line-per-file ceiling (mirrors `chat_session_launch_selection_tests.rs`). use super::*; -use op_editor_core::pen_node_ext::PenNodeExt; +use op_editor_core::{ + pen_node_ext::PenNodeExt, AgentProvider, BuiltinAgentConfig, BuiltinAgentKind, + BuiltinAgentPresetKey, ModelEntry, +}; fn frame( id: &str, @@ -97,6 +100,86 @@ fn stash_design_request_for_retry_writes_json_onto_the_last_message() { assert_eq!(restored.prompt, "design a login page"); } +#[test] +fn design_launch_preparation_captures_acp_tier_before_detaching_chat() { + let mut host = WidgetHostNative::new(); + host.editor_state_mut().chat.available_models = + vec![ModelEntry::acp("custom/vendor", "Custom ACP")]; + host.editor_state_mut().chat.selected_model = 0; + let expected_revision = host.editor_state().document_revision(); + let expected_root_count = host.editor_state().active_children().len(); + + let (request, initial_state) = + prepare_design_request_and_snapshot(&mut host, "draw a dashboard".into(), None); + + assert_eq!(request.model.as_deref(), Some("acp:custom/vendor")); + assert_eq!( + op_orchestrator::resolve_model_profile(request.model.as_deref().unwrap()).tier, + op_orchestrator::ModelTier::Basic + ); + assert!( + initial_state.chat.selected_model_entry().is_none(), + "the worker snapshot must stay narrowed" + ); + assert_eq!(initial_state.document_revision(), expected_revision); + assert_eq!(initial_state.active_children().len(), expected_root_count); + assert_eq!( + host.editor_state() + .chat + .selected_model_entry() + .and_then(ModelEntry::acp_agent_id), + Some("custom/vendor"), + "snapshot preparation must restore the live chat selection" + ); +} + +#[test] +fn design_launch_preparation_captures_builtin_model_before_detaching_chat() { + let mut host = WidgetHostNative::new(); + host.editor_state_mut() + .editor_ui + .agent_settings + .builtin_agents + .push(BuiltinAgentConfig { + id: "builtin-1".into(), + preset: BuiltinAgentPresetKey::Custom, + display_name: "MiniMax".into(), + kind: BuiltinAgentKind::OpenAiCompat, + api_key: "sk-test".into(), + model: "MiniMax-M3".into(), + base_url: "http://localhost:9".into(), + enabled: true, + }); + host.editor_state_mut().chat.available_models = vec![ModelEntry::builtin( + AgentProvider::ClaudeCode, + "builtin-1", + "builtin:builtin-1:MiniMax-M3", + "MiniMax M3", + )]; + host.editor_state_mut().chat.selected_model = 0; + + let (request, initial_state) = + prepare_design_request_and_snapshot(&mut host, "draw a dashboard".into(), None); + + assert_eq!(request.model.as_deref(), Some("MiniMax-M3")); + assert_eq!( + op_orchestrator::resolve_model_profile(request.model.as_deref().unwrap()).tier, + op_orchestrator::ModelTier::Full + ); + assert!( + initial_state.chat.selected_model_entry().is_none(), + "the worker snapshot must stay narrowed" + ); + assert_eq!( + host.editor_state() + .chat + .selected_model_entry() + .and_then(|entry| entry.builtin_provider_id.as_deref()), + Some("builtin-1"), + "snapshot preparation must restore the live builtin selection" + ); +} + /// Regression lock for a real bug a user hit: the CLI-standard route /// (`launch_cli_standard_turn`, reached whenever no builtin/ACP model is /// selected — the common case) never stashed `design_request_json_for_retry` diff --git a/crates/op-host-services/src/chat_provider_llm.rs b/crates/op-host-services/src/chat_provider_llm.rs index bbb14e92f..3e7c23158 100644 --- a/crates/op-host-services/src/chat_provider_llm.rs +++ b/crates/op-host-services/src/chat_provider_llm.rs @@ -98,6 +98,7 @@ impl LlmClient for ChatProviderLlmClient { let m3_keeps_thinking = req .model .as_deref() + .filter(|model| !op_orchestrator::is_acp_capability_marker(model)) .map(|m| m.to_ascii_lowercase().contains("minimax-m3")) .unwrap_or(false); let chat_req = ChatRequest { @@ -227,6 +228,14 @@ mod tests { let unknown = call_with_model(None); assert_eq!(unknown.thinking, ThinkingMode::Disabled); + + let acp_marker = call_with_model(Some("acp:minimax-m3-wrapper")); + assert_eq!(acp_marker.thinking, ThinkingMode::Disabled); + assert_eq!(acp_marker.max_output_tokens, 16384); + assert_eq!( + acp_marker.model, None, + "ACP capability identity must not become a transport model" + ); } // ── Provider-shape agnosticism ─────────────────────────────────────── diff --git a/crates/op-host-services/src/validation_providers.rs b/crates/op-host-services/src/validation_providers.rs index 990ffe979..03933e947 100644 --- a/crates/op-host-services/src/validation_providers.rs +++ b/crates/op-host-services/src/validation_providers.rs @@ -153,7 +153,7 @@ impl ChatVisionLlmClient { /// Attach the vision model id to every request this client issues. pub fn with_model(mut self, model: Option) -> Self { - self.model = model; + self.model = Self::transport_model(model); self } @@ -171,6 +171,13 @@ impl ChatVisionLlmClient { data, }) } + + /// Keep ACP catalog identities inside orchestrator capability policy. + /// They name an agent, not a provider model, and therefore must collapse + /// to the provider default at this transport boundary. + fn transport_model(model: Option) -> Option { + model.filter(|id| !op_orchestrator::is_acp_capability_marker(id)) + } } impl VisionLlmClient for ChatVisionLlmClient { @@ -207,7 +214,8 @@ impl VisionLlmClient for ChatVisionLlmClient { thinking: ThinkingMode::Disabled, effort: EffortLevel::Low, attachments: vec![attachment], - model: self.model.clone().or_else(|| req.model.clone()), + model: Self::transport_model(self.model.clone()) + .or_else(|| Self::transport_model(req.model.clone())), }; // `provider.send` is a blocking delta iterator (the same shape the @@ -401,6 +409,29 @@ mod tests { assert_eq!(r.model.as_deref(), Some("vision-model")); } + #[test] + fn chat_vision_client_does_not_forward_acp_capability_marker_as_model() { + let seen = Arc::new(Mutex::new(Vec::new())); + let provider = Arc::new(RecordingVisionProvider { + seen: seen.clone(), + reply: r#"{"issues":[],"fixes":[],"qualityScore":9}"#.into(), + }); + let client = + ChatVisionLlmClient::new(provider).with_model(Some("acp:custom/vendor".to_string())); + let mut request = vision_req(&b64_png()); + request.model = Some("acp:custom/vendor".to_string()); + + let response = client.validate(request); + + assert!(matches!(response, VisionResponse::Text(_))); + let requests = seen.lock().unwrap(); + assert_eq!( + requests.first().expect("provider was called").model, + None, + "ACP catalog identity is a capability marker, not a transport model" + ); + } + /// A non-base64 screenshot string can't drive a vision call → Skipped. #[test] fn chat_vision_client_skips_on_bad_base64() { diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index 7a560d200..238ff30a5 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -127,7 +127,7 @@ pub use design_type::{detect_design_type, DesignType, DesignTypePreset}; pub use intent::classify_intent; pub use loop_finalize::{apply_loop_finalize, apply_loop_finalize_counted}; pub use mobile_reflow::repair_mobile_trailing_nav_reflow; -pub use model_profile::{resolve_model_profile, ModelProfile, ModelTier}; +pub use model_profile::{is_acp_capability_marker, resolve_model_profile, ModelProfile, ModelTier}; pub use prompt::build_orchestrator_prompt; pub use repair_summary::{CheckCategory, RepairSummary}; pub use run::Orchestrator; diff --git a/crates/op-orchestrator/src/model_profile.rs b/crates/op-orchestrator/src/model_profile.rs index 1a757c0d0..fb11719e8 100644 --- a/crates/op-orchestrator/src/model_profile.rs +++ b/crates/op-orchestrator/src/model_profile.rs @@ -48,6 +48,26 @@ const DEFAULT_PROFILE: ModelProfile = ModelProfile { label: "Unknown model", }; +/// ACP agents do not expose their backing model to OpenPencil. Treat their +/// catalog identity conservatively instead of promoting a missing model id to +/// the Full-tier default. +const ACP_PROFILE: ModelProfile = ModelProfile { + tier: ModelTier::Basic, + thinking_disabled: true, + timeout_multiplier: 1.0, + label: "ACP agent", +}; + +/// Whether `model_id` is the catalog identity of an ACP agent rather than a +/// concrete provider model. Hosts may carry this marker through +/// [`crate::DesignRequest`] for capability policy, but must not forward it as a +/// transport model override. +pub fn is_acp_capability_marker(model_id: &str) -> bool { + model_id + .get(..4) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case("acp:")) +} + /// 模型表 —— verbatim 移植自 `model-profiles.ts:22-95`,首个命中胜出。 const MODEL_PROFILES: &[Entry] = &[ // Full tier @@ -215,8 +235,10 @@ const fn e(matcher: Match, tier: ModelTier, thinking_disabled: bool, label: &'st } } -/// 解析模型 id → profile。strip `provider/` 前缀 → 小写 → 首个命中。 -/// 空 id → 强制 `Full`(TS 行为);无命中 → `DEFAULT_PROFILE`。 +/// Resolve a model id to its capability profile. ACP catalog ids use the +/// conservative `Basic` default. Other ids strip a `provider/` prefix, then +/// match the lower-cased model table. An empty id keeps the legacy forced +/// `Full` behavior; an unmatched non-empty id uses [`DEFAULT_PROFILE`]. pub fn resolve_model_profile(model_id: &str) -> ModelProfile { if model_id.is_empty() { return ModelProfile { @@ -226,6 +248,11 @@ pub fn resolve_model_profile(model_id: &str) -> ModelProfile { label: "Default (no model)", }; } + // Check before stripping a provider prefix: ACP ids are opaque and may + // themselves contain `/` (for example `acp:vendor/custom-agent`). + if is_acp_capability_marker(model_id) { + return ACP_PROFILE; + } let normalized = match model_id.find('/') { Some(i) => &model_id[i + 1..], None => model_id, @@ -284,6 +311,10 @@ mod tests { assert_eq!(resolve_model_profile("minimax-01").tier, ModelTier::Basic); assert_eq!(resolve_model_profile("glm-4-plus").tier, ModelTier::Basic); assert_eq!(resolve_model_profile("qwen-max").tier, ModelTier::Basic); + assert_eq!( + resolve_model_profile("acp:vendor/custom-agent").tier, + ModelTier::Basic + ); } #[test]