From fe197686d13cb357dbe584e8acdc2c2d13b98701 Mon Sep 17 00:00:00 2001 From: orveth Date: Tue, 25 Aug 2026 08:50:03 -0700 Subject: [PATCH] Bind and verify a requested session model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A job may name a model. Nothing could select one, so a seat served whatever the harness defaulted to. The setter's success is not the check, and this is the whole design. Read at claude-agent-acp v0.62.0 and codex-acp main: codex accepts an unrecognised id verbatim and forwards it, rejecting only the empty string; claude-agent-acp resolves aliases like `opus` onto a canonical id and substitutes it deliberately. Both return OK having bound something other than what was named. So two guards catch two different substitutions, and neither subsumes the other. Membership, pre-write, in the driver: refuse a model the selector does not offer. This is the only guard that can see codex's echo, because there the read-back returns the request verbatim and any comparison passes. The offered set is `options[].value` plus `currentValue` — the current value is settable even when the picker omits it, so `options` alone is not the settable set. Exact comparison, post-write, in the engine: read back what the harness reports as bound and refuse anything but the requested string. This is the only guard that can see claude's canonicalisation. It lives outside the driver so a driver cannot bypass it, and it is a pure function so the policy is testable without a harness. The write addresses the option by `configId`, and ACP leaves `id` agent-chosen, so the id is discovered from the same model-category entry the read path uses. A hardcoded "model" would repeat the defect the read path exists to avoid, and sharing the lookup is what guarantees we write to the selector we then read. Refusal lands before the prompt, so no compute is spent and no delivery exists. NOT wired: nothing supplies a requested model yet. The seam is marked in seller_exec, and the value must come from the stored signed offer, never award params. That waits on #900's contract. NOT supported: a composed `model[effort]` id. #785 states that whether the settable surface is two axes or enumerated composites is a hypothesis, so parsing the composition would bake in an unverified format. A composed request fails the exact comparison and refuses. NOT verified: any of this against a running harness. The wire method `session/set_config_option` is confirmed from agent-client-protocol v0.14.0, but no live handshake was made. --- .../maxplayer-core/src/driver/acp_driver.rs | 262 +++++++++++++++++- crates/maxplayer-core/src/driver/mod.rs | 54 ++++ crates/maxplayer-core/src/engine.rs | 167 ++++++++++- crates/maxplayer-core/src/seller_exec.rs | 8 + .../maxplayer-core/tests/acp_concurrency.rs | 3 + crates/maxplayer/src/cli.rs | 4 + 6 files changed, 484 insertions(+), 14 deletions(-) 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 }], },