From 9de5ba4279b68c4bf3643ac09be6153a44ab2ee3 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 30 Sep 2026 10:16:11 +0200 Subject: [PATCH] hive-agent: warn once on a model-list mismatch; test the ACP set-model/effort branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit filter_offered_models logged its "no match" warning on every pickers() call; pickers() runs on every /api/state poll (every 4s per open dashboard tab) and on every post_set_model/post_set_effort call. Guard it with a process-lifetime AtomicBool, threaded in explicitly so tests can use their own instead of racing each other on a shared static. Extract post_set_model/post_set_effort's ACP accept/refuse decision into a pure accept_or_refuse() helper and add tests for it: an offered value is accepted, an unoffered one is refused with 400, and an empty offered list (before the first turn, or effort while a model switch is pending) refuses with 400 too. Not fixed here: a session refusing session/set_config_option leaves the picker showing the refused value as pending indefinitely. Fixing it needs hive-runtime to expose a real refused/pending distinction that Choice and SessionChoices don't carry today, threaded through to hive-agent's model_pending computation, plus a fake-agent test fixture that can simulate an RPC refusal — a cross-crate change, not a small one. --- hive-agent/src/web_ui/actions.rs | 48 ++++++++++++++++++++++-- hive-agent/src/web_ui/state.rs | 64 ++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/hive-agent/src/web_ui/actions.rs b/hive-agent/src/web_ui/actions.rs index 5df7b98f..a09488fd 100644 --- a/hive-agent/src/web_ui/actions.rs +++ b/hive-agent/src/web_ui/actions.rs @@ -148,8 +148,8 @@ pub(super) async fn post_set_model( } let text = if state.bus.session_choices().is_some() { let offered = super::state::pickers(&state.bus).available_models; - if !offered.iter().any(|m| m == name) { - return not_offered("model", &offered); + if let Some(response) = accept_or_refuse("model", name, &offered) { + return response; } format!("operator: /model — model set to '{name}' from the next turn") } else { @@ -180,8 +180,8 @@ pub(super) async fn post_set_effort( let level = form.effort.trim(); let text = if state.bus.session_choices().is_some() { let offered = super::state::pickers(&state.bus).available_efforts; - if !offered.iter().any(|e| e == level) { - return not_offered("effort", &offered); + if let Some(response) = accept_or_refuse("effort", level, &offered) { + return response; } format!("operator: /effort — effort set to '{level}' from the next turn") } else if crate::harness_state::is_valid_effort(level) { @@ -201,6 +201,18 @@ pub(super) async fn post_set_effort( (axum::http::StatusCode::OK, "ok").into_response() } +/// The ACP branch of `post_set_model`/`post_set_effort`: `None` when `name` +/// is one `offered` (the session's current pickers list) names, else the +/// [`not_offered`] refusal to return. Before a session has attached, +/// `offered` is empty and every name is refused. +fn accept_or_refuse(what: &str, name: &str, offered: &[String]) -> Option { + if offered.iter().any(|o| o == name) { + None + } else { + Some(not_offered(what, offered)) + } +} + /// Refuses a pick the ACP session does not offer. fn not_offered(what: &str, offered: &[String]) -> Response { let message = if offered.is_empty() { @@ -253,3 +265,31 @@ pub(super) async fn post_mark_todos_done(Form(form): Form) -> struct MarkTodosDoneBody { acked: u64, } + +#[cfg(test)] +mod tests { + use super::accept_or_refuse; + + #[test] + fn an_offered_value_is_accepted() { + let offered = vec!["m/think".to_owned(), "m/plain".to_owned()]; + assert!(accept_or_refuse("model", "m/plain", &offered).is_none()); + } + + #[test] + fn a_value_the_session_does_not_offer_is_refused_with_400() { + let offered = vec!["m/think".to_owned()]; + let response = accept_or_refuse("model", "m/plain", &offered).unwrap(); + assert_eq!(response.status(), axum::http::StatusCode::BAD_REQUEST); + } + + #[test] + fn nothing_offered_yet_refuses_with_400() { + // Before a session has attached (or on the effort picker while a + // model switch is pending), `pickers()` returns an empty list — + // every name is refused, not just ones the session once offered + // and later dropped. + let response = accept_or_refuse("effort", "high", &[]).unwrap(); + assert_eq!(response.status(), axum::http::StatusCode::BAD_REQUEST); + } +} diff --git a/hive-agent/src/web_ui/state.rs b/hive-agent/src/web_ui/state.rs index d6e058af..3d4c0f6e 100644 --- a/hive-agent/src/web_ui/state.rs +++ b/hive-agent/src/web_ui/state.rs @@ -1,5 +1,7 @@ //! `/api/state` + `/api/dashboard-state` snapshot builders. +use std::sync::atomic::{AtomicBool, Ordering}; + use axum::extract::State; use serde::Serialize; @@ -460,14 +462,27 @@ fn configured_models() -> Option> { (!models.is_empty()).then_some(models) } +/// Set the first time [`filter_offered_models`] logs its "no match" warning. +/// `pickers()` runs on every `/api/state` poll (every 4s per open dashboard +/// tab) and on every `post_set_model`/`post_set_effort` call, and all of them +/// funnel through the one [`filter_offered_models`] call site in +/// [`offered_pickers`], so one flag guards every caller. +static MODEL_MISMATCH_WARNED: AtomicBool = AtomicBool::new(false); + /// Keep only the entries of `offered` (an ACP session's own model list, in /// its own order) that `configured` names. `configured: None` (the env is /// unset) or a `configured` list matching none of `offered` both leave /// `offered` unfiltered — the latter is what an ACP agent that has never /// set `services.hyperhive.agent.availableModels` hits, since the option's /// default is the claude names, so it must show every model the session -/// offers rather than an empty picker; a warning names the mismatch once. -fn filter_offered_models(offered: Vec, configured: Option<&[String]>) -> Vec { +/// offers rather than an empty picker. `warned` gates the mismatch warning +/// to once per process; tests pass their own instead of [`MODEL_MISMATCH_WARNED`] +/// so they don't observe each other's state. +fn filter_offered_models( + offered: Vec, + configured: Option<&[String]>, + warned: &AtomicBool, +) -> Vec { let Some(configured) = configured else { return offered; }; @@ -477,11 +492,13 @@ fn filter_offered_models(offered: Vec, configured: Option<&[String]>) -> .cloned() .collect(); if matching.is_empty() { - tracing::warn!( - configured = configured.join(", "), - "HIVE_AVAILABLE_MODELS matches none of the models this ACP session offers; \ - showing every model it offers instead" - ); + if !warned.swap(true, Ordering::Relaxed) { + tracing::warn!( + configured = configured.join(", "), + "HIVE_AVAILABLE_MODELS matches none of the models this ACP session offers; \ + showing every model it offers instead" + ); + } offered } else { matching @@ -546,7 +563,7 @@ fn offered_pickers( effort: shown(effort, effort_choice.as_ref()), available_models: offered .model - .map(|c| filter_offered_models(c.values, configured)) + .map(|c| filter_offered_models(c.values, configured, &MODEL_MISMATCH_WARNED)) .unwrap_or_default(), available_efforts: effort_choice.map(|c| c.values).unwrap_or_default(), } @@ -554,6 +571,8 @@ fn offered_pickers( #[cfg(test)] mod tests { + use std::sync::atomic::{AtomicBool, Ordering}; + use hive_runtime::{Choice, SessionChoices}; use super::{Pickers, filter_offered_models, offered_pickers}; @@ -622,8 +641,9 @@ mod tests { #[test] fn unconfigured_model_list_leaves_the_session_offer_untouched() { + let warned = AtomicBool::new(false); assert_eq!( - filter_offered_models(strings(&["m/think", "m/plain"]), None), + filter_offered_models(strings(&["m/think", "m/plain"]), None, &warned), strings(&["m/think", "m/plain"]) ); } @@ -631,8 +651,9 @@ mod tests { #[test] fn configured_model_list_keeps_only_the_matches_in_the_session_s_order() { let configured = strings(&["m/plain", "m/other"]); + let warned = AtomicBool::new(false); assert_eq!( - filter_offered_models(strings(&["m/think", "m/plain"]), Some(&configured)), + filter_offered_models(strings(&["m/think", "m/plain"]), Some(&configured), &warned), strings(&["m/plain"]) ); } @@ -641,10 +662,31 @@ mod tests { fn configured_model_list_matching_nothing_falls_back_to_every_offered_model() { // The claude names on an ACP agent that never set `availableModels`. let configured = strings(&["haiku", "sonnet", "opus"]); + let warned = AtomicBool::new(false); assert_eq!( - filter_offered_models(strings(&["m/think", "m/plain"]), Some(&configured)), + filter_offered_models(strings(&["m/think", "m/plain"]), Some(&configured), &warned), strings(&["m/think", "m/plain"]) ); + assert!( + warned.load(Ordering::Relaxed), + "the mismatch warning should have fired once" + ); + } + + #[test] + fn a_second_no_match_call_does_not_warn_again() { + // `tracing::warn!` output isn't captured in this crate's tests (no + // subscriber fixture exists), so this pins the guard itself: once + // `warned` is set, a second no-match call leaves it set rather than + // toggling or re-arming, which is what keeps the log line from + // repeating on every `/api/state` poll. + let configured = strings(&["haiku"]); + let warned = AtomicBool::new(false); + filter_offered_models(strings(&["m/think"]), Some(&configured), &warned); + assert!(warned.load(Ordering::Relaxed)); + let second = filter_offered_models(strings(&["m/think"]), Some(&configured), &warned); + assert_eq!(second, strings(&["m/think"])); + assert!(warned.load(Ordering::Relaxed)); } #[test]