diff --git a/crates/maxplayer-core/src/driver/acp_driver.rs b/crates/maxplayer-core/src/driver/acp_driver.rs index 7853dee1e..e38577be5 100644 --- a/crates/maxplayer-core/src/driver/acp_driver.rs +++ b/crates/maxplayer-core/src/driver/acp_driver.rs @@ -56,6 +56,24 @@ pub struct AcpDriver { /// either wire shape by [`session_model_from_result`]. Folded into [`Self::usage`] so a run's /// exec-metadata carries the resolved model (#455). `None` when the harness reported no model. session_model: Option, + /// The `id` of this session's model selector, captured from the `session/new` `configOptions` + /// so [`Driver::select_model`] can address `session/set_config_option`. + /// + /// Captured rather than assumed: ACP leaves the option `id` to the agent, so the only way to + /// write to the selector we also READ is to remember the id off the same entry. `None` when the + /// harness published no model selector — which makes a model request unservable rather than a + /// fault, hence [`DriverError::ModelSelectorAbsent`]. + model_config_id: Option, + /// Every model id this session's selector says it can be set to, captured at `session/new`. + /// + /// Checked BEFORE writing, because the post-write read-back cannot see one of the two + /// substitution directions: `codex-acp` accepts an unrecognised id verbatim and echoes it back, + /// so request == bound and an exact comparison passes on a model the harness never had. See + /// [`model_option_offered_values`]. + /// + /// `None` when no selector was published; `Some(empty)` is a selector offering nothing, which is + /// a different fact and refuses every request rather than accepting any. + model_offered_values: Option>, } impl AcpDriver { @@ -76,6 +94,8 @@ impl AcpDriver { next_request_id: AtomicU64::new(1), last_usage: None, session_model: None, + model_config_id: None, + model_offered_values: None, } } @@ -251,9 +271,61 @@ impl Driver for AcpDriver { // `session_model_from_result` reads (#896). Absent-stays-absent: a harness that reports no // model leaves this `None`, and nothing downstream fabricates one. self.session_model = session_model_from_result(&result); + // Capture the model selector's id from the same response, so a later `select_model` writes + // to the selector this session actually published rather than a hardcoded name (#785). + self.model_config_id = model_config_option_id(&result); + self.model_offered_values = model_option_offered_values(&result); session_id_from_result(&result) } + async fn select_model( + &mut self, + session_id: &SessionId, + model: &str, + ) -> Result, DriverError> { + // No selector in this session's `configOptions` ⇒ there is no id to address, and guessing + // one would be the hardcoded-name defect `model_config_option_id` exists to avoid. A harness + // that published no model selector cannot serve a model request; say that, do not retry it. + let config_id = self + .model_config_id + .clone() + .ok_or(DriverError::ModelSelectorAbsent)?; + // FIRST GUARD, and it catches the direction the read-back structurally cannot: refuse a + // model the selector does not offer, BEFORE writing anything. `codex-acp` accepts an + // unrecognised id verbatim and echoes it back, so request == bound and the exact comparison + // downstream would PASS on a model the harness never had. Refusing pre-write also means a + // rejected request leaves the session's model untouched. + if let Some(offered) = &self.model_offered_values + && !offered.iter().any(|value| value == model) + { + return Err(DriverError::ModelNotOffered { + requested: model.to_owned(), + }); + } + let id = self.send_request( + "session/set_config_option", + json!({ + "sessionId": session_id, + "configId": config_id, + "value": model, + }), + )?; + let result = self.wait_response(id).await?; + // The setter returns the FULL updated `configOptions`, so its own response is the read-back + // channel — same array shape as `session/new`, so the same first-entry fail-closed rule + // applies unchanged. Returning the harness's REPORT, not a verdict: the caller compares it + // against what was requested, because a successful set is not evidence the requested model + // was bound (see `Driver::select_model`). + let bound = config_option_session_model(&result); + // Keep the advertised model consistent with what is now bound. Without this, a successful + // rebind would leave `usage()` reporting the model captured at session start — an + // exec-metadata attribution that names a model the run did not use. + if let Some(bound) = bound.clone() { + self.session_model = Some(bound); + } + Ok(bound) + } + async fn prompt( &mut self, session_id: &SessionId, @@ -568,7 +640,7 @@ fn session_model_from_result(result: &Value) -> Option { /// Shape 1: the legacy top-level `models.currentModelId` (camelCase on the wire). fn legacy_session_model(result: &Value) -> Option { - non_blank_model(result.get("models")?.get("currentModelId")?) + non_blank_str(result.get("models")?.get("currentModelId")?) } /// Shape 2: the FIRST model-category `configOptions` entry, value in its `currentValue`. @@ -589,25 +661,85 @@ fn legacy_session_model(result: &Value) -> Option { /// A `configOptions` that is not an array, entries that are not objects, and entries of any other /// category never yield a value. fn config_option_session_model(result: &Value) -> Option { - let first_model_option = result + non_blank_str(first_model_config_option(result)?.get("currentValue")?) +} + +/// The FIRST `configOptions` entry declaring the model category, or `None`. +/// +/// Shared by the read path and the write path deliberately. The value we ADVERTISE and the option +/// we WRITE TO must be the same selector: two independent "find the model option" implementations +/// could drift, and the failure that produces is setting one selector while reporting another — a +/// silent success with the wrong model bound, which is the exact class this module exists to refuse. +fn first_model_config_option(result: &Value) -> Option<&Value> { + result .get("configOptions")? .as_array()? .iter() .find(|option| { option.get("category").and_then(Value::as_str) == Some(MODEL_CONFIG_CATEGORY) - })?; - non_blank_model(first_model_option.get("currentValue")?) + }) +} + +/// The `id` of the model selector this session published, for addressing `session/set_config_option`. +/// +/// The read path keys on `category`; the write path must address the option by `id`, because that is +/// what the setter takes. Those are different fields, and the id is NOT hardcodable: ACP documents +/// `id` as the agent's own identifier and does not standardise it, so writing a literal `"model"` +/// would bind this to one adapter's naming — precisely the defect [`MODEL_CONFIG_CATEGORY`] +/// documents avoiding on the read side. Taking the id from the SAME entry the read path uses is also +/// what guarantees we write to the selector we subsequently read back. +fn model_config_option_id(result: &Value) -> Option { + non_blank_str(first_model_config_option(result)?.get("id")?) +} + +/// Every model id this session's selector says it can be set to. +/// +/// ⛔ This exists because the post-write read-back CANNOT catch one of the two substitution +/// directions, and it is worth being exact about which. `claude-agent-acp` resolves an alias onto a +/// canonical id, so the value it reports back DIFFERS from the request and an exact comparison +/// catches it. `codex-acp` does the opposite: an unrecognised id is accepted verbatim +/// (`unwrap_or_else(|| model_id.to_string())`, rejecting only the empty string) and forwarded to +/// Codex, so the read-back faithfully ECHOES the nonsense and request == bound. The comparison +/// passes and the job proceeds on a model the harness never had. +/// +/// So membership is a SECOND guard against a different failure, not a cheaper version of the first: +/// refuse a model the selector does not offer, before writing anything. +/// +/// The set is `options[].value` PLUS the current `currentValue`. The current value belongs even when +/// absent from `options`: `claude-agent-acp` treats it as always-settable, because a session resumed +/// onto an allowlist-excluded model reports a `currentValue` that is not in its own picker, and a +/// client round-tripping it must not be refused. So `options` alone is NOT the settable set. +/// +/// `None` when the session published no model selector — distinct from `Some(empty)`, which is a +/// selector offering nothing. +fn model_option_offered_values(result: &Value) -> Option> { + let option = first_model_config_option(result)?; + let mut offered: Vec = option + .get("options") + .and_then(Value::as_array) + .map(|values| { + values + .iter() + .filter_map(|entry| non_blank_str(entry.get("value")?)) + .collect() + }) + .unwrap_or_default(); + if let Some(current) = option.get("currentValue").and_then(non_blank_str) + && !offered.contains(¤t) + { + offered.push(current); + } + Some(offered) } -/// A model id is a non-blank JSON string, taken verbatim. +/// A usable wire string: non-blank, taken verbatim. /// -/// Anything else — a blank or whitespace-only string, a number, bool, null, array or object — is not -/// a model id and yields `None` rather than a coerced, trimmed or fabricated value. This also -/// applies to the legacy shape: a present-but-blank `currentModelId` is not a model, so it falls -/// through to the config-option shape rather than advertising an empty string. -fn non_blank_model(value: &Value) -> Option { - let model = value.as_str()?; - (!model.trim().is_empty()).then(|| model.to_owned()) +/// Anything else — a blank or whitespace-only string, a number, bool, null, array or object — yields +/// `None` rather than a coerced, trimmed or fabricated value. Used for both the model id and the +/// selector id, because the rule is identical: a blank string is not an identifier. +fn non_blank_str(value: &Value) -> Option { + let text = value.as_str()?; + (!text.trim().is_empty()).then(|| text.to_owned()) } /// Fold the `session/new` model into a run's captured usage. The `session/prompt` result never @@ -1051,6 +1183,112 @@ mod tests { assert_eq!(session_model_from_result(&unknown_category), None); } + #[test] + fn the_model_selector_id_is_read_from_the_model_category_entry() { + // #785: the WRITE addresses an option by `id`, and the id must come from the same entry the + // read path keys on by `category` — never a hardcoded "model". Here the selector deliberately + // calls itself something else, so a hardcoded literal would find nothing. + let result = json!({ + "sessionId": "abc", + "configOptions": [ + {"id": "session-mode", "category": "mode", "currentValue": "default"}, + {"id": "vendor.model.picker", "category": "model", "currentValue": "m-1"} + ] + }); + assert_eq!( + model_config_option_id(&result).as_deref(), + Some("vendor.model.picker") + ); + } + + #[test] + fn the_model_selector_id_follows_the_same_first_entry_and_absence_rules() { + // Same conservatism as the read path, for the same reason: writing to a LATER model-category + // selector than the one we report from would set one option and advertise another. + let two = json!({ + "sessionId": "abc", + "configOptions": [ + {"id": "first", "category": "model", "currentValue": "m-1"}, + {"id": "second", "category": "model", "currentValue": "m-2"} + ] + }); + assert_eq!(model_config_option_id(&two).as_deref(), Some("first")); + + // No category ⇒ we decline to infer, exactly as on the read side. No selector, no write. + let no_category = json!({ + "sessionId": "abc", + "configOptions": [{"id": "model", "currentValue": "m-1"}] + }); + assert_eq!(model_config_option_id(&no_category), None); + + // A blank or non-string id is not an id. + for bad in [json!(""), json!(" "), json!(7), json!(null)] { + let result = json!({ + "sessionId": "abc", + "configOptions": [{"id": bad, "category": "model", "currentValue": "m-1"}] + }); + assert_eq!(model_config_option_id(&result), None); + } + } + + #[test] + fn offered_values_include_the_options_list_and_the_current_value() { + // The current value belongs even when absent from `options`: claude-agent-acp treats it as + // always-settable, because a session resumed onto an allowlist-excluded model reports a + // currentValue outside its own picker and a client round-tripping it must not be refused. + // So `options` alone is NOT the settable set. + let result = json!({ + "sessionId": "abc", + "configOptions": [{ + "id": "model", + "category": "model", + "currentValue": "out-of-picker-model", + "options": [{"value": "m-1"}, {"value": "m-2"}] + }] + }); + let offered = model_option_offered_values(&result).expect("selector present"); + assert!(offered.contains(&"m-1".to_string())); + assert!(offered.contains(&"m-2".to_string())); + assert!( + offered.contains(&"out-of-picker-model".to_string()), + "the current value must be settable even when the picker omits it" + ); + assert_eq!(offered.len(), 3, "no duplicates, nothing invented: {offered:?}"); + } + + #[test] + fn offered_values_distinguish_no_selector_from_a_selector_offering_nothing() { + // Two different facts that a bare `Vec` would flatten. `None` = nothing published, so a + // model request is unservable. `Some(empty)` = a selector that offers nothing, which refuses + // every request rather than accepting any — and must never be read as "unconstrained". + assert_eq!( + model_option_offered_values(&json!({"sessionId": "abc"})), + None + ); + let empty = json!({ + "sessionId": "abc", + "configOptions": [{"id": "model", "category": "model"}] + }); + assert_eq!(model_option_offered_values(&empty), Some(Vec::new())); + } + + #[test] + fn offered_values_skip_malformed_entries_without_inventing_any() { + // A non-array `options`, entries that are not objects, and blank or non-string values all + // contribute nothing. An invented offer would let an unofferable model through the guard. + let result = json!({ + "sessionId": "abc", + "configOptions": [{ + "id": "model", + "category": "model", + "currentValue": "m-real", + "options": [{"value": ""}, {"value": 7}, {"value": null}, "m-bare", {"value": "m-ok"}] + }] + }); + let offered = model_option_offered_values(&result).expect("selector present"); + assert_eq!(offered, vec!["m-ok".to_string(), "m-real".to_string()]); + } + #[test] fn merge_session_model_or_fills_only_a_missing_model() { // Prompt usage carries tokens but never a model (parse_acp_usage); the session-start model diff --git a/crates/maxplayer-core/src/driver/mod.rs b/crates/maxplayer-core/src/driver/mod.rs index 89b3100af..6865d40c4 100644 --- a/crates/maxplayer-core/src/driver/mod.rs +++ b/crates/maxplayer-core/src/driver/mod.rs @@ -92,6 +92,27 @@ pub enum DriverError { /// job's absolute deadline; flattening it into [`Self::Other`] would discard that attribution /// and make a healthy harness look broken. ResponseTimeout { request_id: u64 }, + /// This driver has no model-selection route, so a model request cannot be honoured. + /// + /// Typed rather than folded into [`Self::Other`] because the caller must distinguish "this + /// harness cannot select a model at all" from "the route exists and failed". The first is a + /// dispatch fact about the harness; the second is a fault. Collapsing them would let a + /// harness that never had the capability look like one that broke. + ModelSelectionUnsupported, + /// The harness advertises no model selector, so there is no option id to write to. + /// + /// Distinct from [`Self::ModelSelectionUnsupported`], which is about OUR driver: this one says + /// the driver can write config options but THIS session published nothing identifying a model + /// selector. Absence of a selector is not a fault to retry, it is a harness that cannot serve + /// a model request. + ModelSelectorAbsent, + /// The harness's model selector does not offer the requested model, so it is refused unwritten. + /// + /// This is the guard for the substitution the post-write read-back CANNOT see. `codex-acp` + /// accepts an unrecognised model verbatim and forwards it, so a read-back echoes the nonsense + /// straight back and an exact comparison PASSES on a model the harness never had. Checking + /// membership first is a different question from "did the write take", and both are needed. + ModelNotOffered { requested: String }, Other(String), } @@ -105,6 +126,15 @@ impl Display for DriverError { Self::ResponseTimeout { request_id } => { write!(f, "ACP request {request_id} timed out waiting for response") } + Self::ModelSelectionUnsupported => { + write!(f, "driver cannot select a session model") + } + Self::ModelSelectorAbsent => { + write!(f, "harness advertises no model selector for this session") + } + Self::ModelNotOffered { requested } => { + write!(f, "harness does not offer model {requested}") + } Self::Other(message) => write!(f, "{message}"), } } @@ -127,6 +157,30 @@ pub trait Driver { async fn cancel(&mut self, session_id: &SessionId) -> Result<(), DriverError>; async fn shutdown(&mut self) -> Result<(), DriverError>; + /// Ask the harness to bind `model` for this session, and return the model the harness then + /// REPORTS as bound — `None` when it reports nothing usable. + /// + /// ⚠ This returns a REPORT, never a verdict. A driver must not decide whether the binding was + /// acceptable, because the harness's own success is not evidence that the requested model was + /// bound: `codex-acp` accepts an unrecognised model verbatim and forwards it (rejecting only + /// the empty string), and `claude-agent-acp` fuzzy-resolves aliases like `opus` onto a + /// canonical id and returns success. Both paths return OK having bound something other than + /// what was named. The comparison is therefore the caller's, in one place, where a driver bug + /// cannot bypass it — see `engine::bind_session_model`. + /// + /// Default: [`DriverError::ModelSelectionUnsupported`]. Fail-closed on purpose. A default that + /// returned `Ok(None)`, or echoed the request back, would read downstream as a successful + /// binding and would make every driver that never implemented this silently claim to honour + /// model requests. + async fn select_model( + &mut self, + session_id: &SessionId, + model: &str, + ) -> Result, DriverError> { + let _ = (session_id, model); + Err(DriverError::ModelSelectionUnsupported) + } + /// Execution usage captured from the most recent prompt, if the harness surfaced any. /// Default `None` keeps **absent-stays-absent** for drivers that expose nothing — only a /// driver that actually reads usage overrides this. diff --git a/crates/maxplayer-core/src/engine.rs b/crates/maxplayer-core/src/engine.rs index bf578f61e..65973334b 100644 --- a/crates/maxplayer-core/src/engine.rs +++ b/crates/maxplayer-core/src/engine.rs @@ -1,6 +1,6 @@ use crate::driver::{ Artifact, ContentBlock, Driver, DriverError, PermissionOutcome, PermissionRequest, PromptTurn, - SessionConfig, SessionUpdate, StopReason, UsageMetadata, + SessionConfig, SessionId, SessionUpdate, StopReason, UsageMetadata, }; use crate::event::{ArtifactId, Envelope, Event, JobExecutionStatus, JobId}; use crate::log::{EventLog, LogError}; @@ -29,6 +29,18 @@ pub struct RunOutcome { pub struct RunParams { pub session_config: SessionConfig, pub prompt: PromptTurn, + /// The model this job NAMED, if it named one, verbatim as the buyer signed it. + /// + /// `None` means no model was requested and the run takes whatever the harness defaults to — + /// today's behaviour for every caller, unchanged. `Some` means the run is only allowed to + /// proceed on that exact model: [`bind_session_model`] asks the harness to bind it and refuses + /// the job if the harness reports anything else. + /// + /// Carried verbatim on purpose. Normalising it anywhere en route would hide the substitution + /// the comparison exists to catch — `claude-agent-acp` resolves `opus` onto a canonical id and + /// reports success, so the requested string and the bound string must stay separately + /// observable all the way to the comparison. + pub requested_model: Option, } impl RunParams { @@ -39,6 +51,8 @@ impl RunParams { mcp_servers: Vec::new(), env: Vec::new(), }, + // No model requested: the default path, and the one every caller takes today. + requested_model: None, prompt: PromptTurn { input: vec![ContentBlock::Text { text: "do the work".into(), @@ -53,6 +67,16 @@ pub enum EngineError { Driver(DriverError), Log(LogError), MissingTerminal, + /// The job named a model and the harness bound something else. The run is refused. + /// + /// Carries BOTH strings because the pair is the evidence: a report saying only "model mismatch" + /// cannot distinguish an alias the harness canonicalised from a value it ignored outright, and + /// those want different follow-ups. `bound: None` means the harness reported no usable model at + /// all after the write. + ModelBindMismatch { + requested: String, + bound: Option, + }, } impl Display for EngineError { @@ -61,6 +85,16 @@ impl Display for EngineError { Self::Driver(error) => write!(f, "{error}"), Self::Log(error) => write!(f, "{error}"), Self::MissingTerminal => write!(f, "mock update stream ended without turn_ended"), + Self::ModelBindMismatch { requested, bound } => match bound { + Some(bound) => write!( + f, + "job requested model {requested} but the harness bound {bound}" + ), + None => write!( + f, + "job requested model {requested} but the harness reported no bound model" + ), + }, } } } @@ -70,7 +104,9 @@ impl Error for EngineError { match self { Self::Driver(error) => Some(error), Self::Log(error) => Some(error), - Self::MissingTerminal => None, + // No source: the mismatch IS the fault, not a wrapper around a lower-level one. The + // harness succeeded at everything it was asked; the refusal is ours. + Self::MissingTerminal | Self::ModelBindMismatch { .. } => None, } } } @@ -122,6 +158,46 @@ pub async fn run_job( } } +/// Bind `requested` as the session's model and PROVE the harness took it, or refuse the run. +/// +/// ⛔ The setter's success is not the check. Measured by reading both adapters' sources: `codex-acp` +/// accepts an unrecognised model verbatim and forwards it to Codex, rejecting only the empty string; +/// `claude-agent-acp` fuzzy-resolves aliases like `opus` onto a canonical id, then deliberately +/// substitutes "the canonical option value so downstream code always receives the model ID rather +/// than the caller-supplied alias". Both return OK having bound something other than what was named, +/// so a caller that trusted the return would run a job on a model the buyer did not ask for. +/// +/// The comparison is EXACT and lives here rather than in the driver — one place, so a driver cannot +/// bypass it, and testable without a harness. No aliasing forgiveness on our side: this crate's own +/// rule is that a named request is exact or nothing, with no nearest-match fallback, because +/// silently running a job on something the buyer did not ask for is the failure the registry exists +/// to prevent. A harness that canonicalises `opus` to `claude-opus-4-6` has bound a DIFFERENT STRING +/// than the one signed, and the buyer filtered and paid on the string. +/// +/// Returns the bound model on success, for callers that want to log what was proven. +async fn bind_session_model( + driver: &mut D, + session_id: &SessionId, + requested: &str, +) -> Result { + verify_bound_model(requested, driver.select_model(session_id, requested).await?) +} + +/// The comparison itself: EXACT, or refuse. +/// +/// Split out from [`bind_session_model`] so the policy is a pure function — no driver, no runtime, +/// no session. The whole value of this change is which pairs it accepts, and that is worth testing +/// directly rather than through a stub whose own behaviour would need trusting. +fn verify_bound_model(requested: &str, bound: Option) -> Result { + match bound { + Some(bound) if bound == requested => Ok(bound), + bound => Err(EngineError::ModelBindMismatch { + requested: requested.to_owned(), + bound, + }), + } +} + /// The fallible body of [`run_job`]: readiness through usage capture, shutdown excluded. async fn run_turn( driver: &mut D, @@ -139,6 +215,12 @@ async fn run_turn( append_execution(log, job_id, JobExecutionStatus::Running)?; let session_id = driver.start_session(params.session_config).await?; + // A named model is bound and PROVEN bound before any work happens. Refusing here costs nothing: + // the session exists but no prompt has been sent, so nothing has been spent on compute and the + // job fails without a delivery. Ordering matters — this sits before `prompt`, never after. + if let Some(requested) = params.requested_model.as_deref() { + bind_session_model(driver, &session_id, requested).await?; + } let mut stream = match driver.prompt(&session_id, params.prompt).await { Ok(stream) => stream, Err(error) => { @@ -270,6 +352,87 @@ impl AgentMessageCapture { } } +#[cfg(test)] +mod model_binding_tests { + use super::{EngineError, verify_bound_model}; + + #[test] + fn an_exactly_matching_bound_model_is_accepted() { + // The only accepting case, and the control for every refusal below: if this were not Ok the + // rest of the module would pass for the wrong reason. + assert_eq!( + verify_bound_model("claude-opus-4-8", Some("claude-opus-4-8".into())).unwrap(), + "claude-opus-4-8" + ); + } + + #[test] + fn a_fuzzily_resolved_alias_is_refused() { + // THE case #785 names. `claude-agent-acp` resolves `opus` onto a canonical id and returns + // SUCCESS, deliberately substituting "the canonical option value so downstream code always + // receives the model ID rather than the caller-supplied alias". The set succeeded; a + // different model is bound. The buyer named `opus`, filtered on `opus` and paid on `opus`, + // so a run on `claude-opus-4-6` is a different product and this must refuse. + let error = verify_bound_model("opus", Some("claude-opus-4-6".into())).unwrap_err(); + assert!( + matches!( + &error, + EngineError::ModelBindMismatch { requested, bound } + if requested == "opus" && bound.as_deref() == Some("claude-opus-4-6") + ), + "both strings must survive into the error, or a reader cannot tell a canonicalised \ + alias from an ignored value: {error}" + ); + } + + #[test] + fn a_harness_reporting_no_model_is_refused() { + // Absence is not agreement. A harness that wrote something and then reported nothing usable + // has not shown us the requested model is bound, so the run does not proceed. + let error = verify_bound_model("claude-opus-4-8", None).unwrap_err(); + assert!(matches!( + error, + EngineError::ModelBindMismatch { bound: None, .. } + )); + } + + #[test] + fn the_comparison_is_exact_and_forgives_nothing() { + // No trimming, no case-folding, no prefix or suffix tolerance. Each of these is a real + // near-miss shape and every one of them is a DIFFERENT model id than the one requested. + // Forgiving any of them re-introduces nearest-match dispatch through the back door. + for bound in [ + "claude-opus-4-8 ", // trailing space + " claude-opus-4-8", // leading space + "Claude-Opus-4-8", // case + "claude-opus-4-8[medium]", // composed: the effort axis appended + "claude-opus-4", // prefix + "claude-opus-4-80", // the requested id is a prefix of this one + ] { + assert!( + verify_bound_model("claude-opus-4-8", Some(bound.into())).is_err(), + "bound {bound:?} differs from the request and must be refused" + ); + } + } + + #[test] + fn an_echoed_unknown_model_is_not_caught_here_and_that_is_deliberate() { + // ⛔ THE LIMIT OF THIS CHECK, pinned so nobody later reads it as total coverage. + // `codex-acp` accepts an unrecognised id VERBATIM and forwards it, so the read-back echoes + // the nonsense and request == bound. This comparison therefore ACCEPTS it — correctly, on + // its own terms, because the harness did report exactly what was asked for. + // + // The guard for that direction is membership, checked pre-write in the driver + // (`DriverError::ModelNotOffered`). Two different failures need two different guards, and + // this test exists so a future reader cannot mistake the exact comparison for both. + assert_eq!( + verify_bound_model("gpt-9-does-not-exist", Some("gpt-9-does-not-exist".into())).unwrap(), + "gpt-9-does-not-exist" + ); + } +} + #[cfg(test)] mod tests { use std::future::Future; diff --git a/crates/maxplayer-core/src/seller_exec.rs b/crates/maxplayer-core/src/seller_exec.rs index 0bf7bfc6d..be82880a4 100644 --- a/crates/maxplayer-core/src/seller_exec.rs +++ b/crates/maxplayer-core/src/seller_exec.rs @@ -2227,6 +2227,14 @@ pub async fn run_agent_job( mcp_servers: Vec::new(), env: identity.git_env(), }, + // NOT WIRED YET, deliberately, and this is the seam where it lands (#785). The mechanism + // that binds and proves a model exists in `engine::bind_session_model`; what is missing is + // the model itself, which must arrive from the STORED SIGNED OFFER — never from award + // params, or the value a seat runs on would not be the value the buyer signed and filtered + // against. That plumbing waits on #900's contract (a requested model rides the signed + // preset), so passing `None` here keeps today's behaviour byte-for-byte: no request, no + // binding, no refusal. + requested_model: None, prompt: PromptTurn { input: vec![ContentBlock::Text { text: prompt.to_owned(), diff --git a/crates/maxplayer-core/tests/acp_concurrency.rs b/crates/maxplayer-core/tests/acp_concurrency.rs index 325253639..97ca0a33e 100644 --- a/crates/maxplayer-core/tests/acp_concurrency.rs +++ b/crates/maxplayer-core/tests/acp_concurrency.rs @@ -77,6 +77,9 @@ async fn run_stub_job( mcp_servers: Vec::new(), env: Vec::new(), }, + // This test is about concurrency, not model selection: no request, so no binding step runs + // and the harness default applies exactly as before. + requested_model: None, prompt: PromptTurn { input: vec![ContentBlock::Text { text: "do the work".into(), diff --git a/crates/maxplayer/src/cli.rs b/crates/maxplayer/src/cli.rs index e225d6f4c..f3b8ebcb3 100644 --- a/crates/maxplayer/src/cli.rs +++ b/crates/maxplayer/src/cli.rs @@ -166,6 +166,10 @@ fn run_agent(args: &[String], out: &mut dyn Write, err: &mut dyn Write) -> i32 { mcp_servers: Vec::new(), env: Vec::new(), }, + // The local run path takes no model flag, so there is nothing to bind and the harness + // default applies — today's behaviour, unchanged. A `--model` here would be a separate + // change with its own argument surface, not a side effect of #785. + requested_model: None, prompt: PromptTurn { input: vec![ContentBlock::Text { text: options.task }], },