hive-agent: warn once on a model-list mismatch; test the ACP set-model/effort branch
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.
This commit is contained in:
parent
8a8da5ec8a
commit
9de5ba4279
2 changed files with 97 additions and 15 deletions
|
|
@ -148,8 +148,8 @@ pub(super) async fn post_set_model(
|
||||||
}
|
}
|
||||||
let text = if state.bus.session_choices().is_some() {
|
let text = if state.bus.session_choices().is_some() {
|
||||||
let offered = super::state::pickers(&state.bus).available_models;
|
let offered = super::state::pickers(&state.bus).available_models;
|
||||||
if !offered.iter().any(|m| m == name) {
|
if let Some(response) = accept_or_refuse("model", name, &offered) {
|
||||||
return not_offered("model", &offered);
|
return response;
|
||||||
}
|
}
|
||||||
format!("operator: /model — model set to '{name}' from the next turn")
|
format!("operator: /model — model set to '{name}' from the next turn")
|
||||||
} else {
|
} else {
|
||||||
|
|
@ -180,8 +180,8 @@ pub(super) async fn post_set_effort(
|
||||||
let level = form.effort.trim();
|
let level = form.effort.trim();
|
||||||
let text = if state.bus.session_choices().is_some() {
|
let text = if state.bus.session_choices().is_some() {
|
||||||
let offered = super::state::pickers(&state.bus).available_efforts;
|
let offered = super::state::pickers(&state.bus).available_efforts;
|
||||||
if !offered.iter().any(|e| e == level) {
|
if let Some(response) = accept_or_refuse("effort", level, &offered) {
|
||||||
return not_offered("effort", &offered);
|
return response;
|
||||||
}
|
}
|
||||||
format!("operator: /effort — effort set to '{level}' from the next turn")
|
format!("operator: /effort — effort set to '{level}' from the next turn")
|
||||||
} else if crate::harness_state::is_valid_effort(level) {
|
} 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()
|
(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<Response> {
|
||||||
|
if offered.iter().any(|o| o == name) {
|
||||||
|
None
|
||||||
|
} else {
|
||||||
|
Some(not_offered(what, offered))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Refuses a pick the ACP session does not offer.
|
/// Refuses a pick the ACP session does not offer.
|
||||||
fn not_offered(what: &str, offered: &[String]) -> Response {
|
fn not_offered(what: &str, offered: &[String]) -> Response {
|
||||||
let message = if offered.is_empty() {
|
let message = if offered.is_empty() {
|
||||||
|
|
@ -253,3 +265,31 @@ pub(super) async fn post_mark_todos_done(Form(form): Form<MarkTodosDoneForm>) ->
|
||||||
struct MarkTodosDoneBody {
|
struct MarkTodosDoneBody {
|
||||||
acked: u64,
|
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);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,7 @@
|
||||||
//! `/api/state` + `/api/dashboard-state` snapshot builders.
|
//! `/api/state` + `/api/dashboard-state` snapshot builders.
|
||||||
|
|
||||||
|
use std::sync::atomic::{AtomicBool, Ordering};
|
||||||
|
|
||||||
use axum::extract::State;
|
use axum::extract::State;
|
||||||
use serde::Serialize;
|
use serde::Serialize;
|
||||||
|
|
||||||
|
|
@ -460,14 +462,27 @@ fn configured_models() -> Option<Vec<String>> {
|
||||||
(!models.is_empty()).then_some(models)
|
(!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
|
/// 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
|
/// its own order) that `configured` names. `configured: None` (the env is
|
||||||
/// unset) or a `configured` list matching none of `offered` both leave
|
/// unset) or a `configured` list matching none of `offered` both leave
|
||||||
/// `offered` unfiltered — the latter is what an ACP agent that has never
|
/// `offered` unfiltered — the latter is what an ACP agent that has never
|
||||||
/// set `services.hyperhive.agent.availableModels` hits, since the option's
|
/// set `services.hyperhive.agent.availableModels` hits, since the option's
|
||||||
/// default is the claude names, so it must show every model the session
|
/// 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.
|
/// offers rather than an empty picker. `warned` gates the mismatch warning
|
||||||
fn filter_offered_models(offered: Vec<String>, configured: Option<&[String]>) -> Vec<String> {
|
/// 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<String>,
|
||||||
|
configured: Option<&[String]>,
|
||||||
|
warned: &AtomicBool,
|
||||||
|
) -> Vec<String> {
|
||||||
let Some(configured) = configured else {
|
let Some(configured) = configured else {
|
||||||
return offered;
|
return offered;
|
||||||
};
|
};
|
||||||
|
|
@ -477,11 +492,13 @@ fn filter_offered_models(offered: Vec<String>, configured: Option<&[String]>) ->
|
||||||
.cloned()
|
.cloned()
|
||||||
.collect();
|
.collect();
|
||||||
if matching.is_empty() {
|
if matching.is_empty() {
|
||||||
tracing::warn!(
|
if !warned.swap(true, Ordering::Relaxed) {
|
||||||
configured = configured.join(", "),
|
tracing::warn!(
|
||||||
"HIVE_AVAILABLE_MODELS matches none of the models this ACP session offers; \
|
configured = configured.join(", "),
|
||||||
showing every model it offers instead"
|
"HIVE_AVAILABLE_MODELS matches none of the models this ACP session offers; \
|
||||||
);
|
showing every model it offers instead"
|
||||||
|
);
|
||||||
|
}
|
||||||
offered
|
offered
|
||||||
} else {
|
} else {
|
||||||
matching
|
matching
|
||||||
|
|
@ -546,7 +563,7 @@ fn offered_pickers(
|
||||||
effort: shown(effort, effort_choice.as_ref()),
|
effort: shown(effort, effort_choice.as_ref()),
|
||||||
available_models: offered
|
available_models: offered
|
||||||
.model
|
.model
|
||||||
.map(|c| filter_offered_models(c.values, configured))
|
.map(|c| filter_offered_models(c.values, configured, &MODEL_MISMATCH_WARNED))
|
||||||
.unwrap_or_default(),
|
.unwrap_or_default(),
|
||||||
available_efforts: effort_choice.map(|c| c.values).unwrap_or_default(),
|
available_efforts: effort_choice.map(|c| c.values).unwrap_or_default(),
|
||||||
}
|
}
|
||||||
|
|
@ -554,6 +571,8 @@ fn offered_pickers(
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
|
use std::sync::atomic::{AtomicBool, Ordering};
|
||||||
|
|
||||||
use hive_runtime::{Choice, SessionChoices};
|
use hive_runtime::{Choice, SessionChoices};
|
||||||
|
|
||||||
use super::{Pickers, filter_offered_models, offered_pickers};
|
use super::{Pickers, filter_offered_models, offered_pickers};
|
||||||
|
|
@ -622,8 +641,9 @@ mod tests {
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn unconfigured_model_list_leaves_the_session_offer_untouched() {
|
fn unconfigured_model_list_leaves_the_session_offer_untouched() {
|
||||||
|
let warned = AtomicBool::new(false);
|
||||||
assert_eq!(
|
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"])
|
strings(&["m/think", "m/plain"])
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
@ -631,8 +651,9 @@ mod tests {
|
||||||
#[test]
|
#[test]
|
||||||
fn configured_model_list_keeps_only_the_matches_in_the_session_s_order() {
|
fn configured_model_list_keeps_only_the_matches_in_the_session_s_order() {
|
||||||
let configured = strings(&["m/plain", "m/other"]);
|
let configured = strings(&["m/plain", "m/other"]);
|
||||||
|
let warned = AtomicBool::new(false);
|
||||||
assert_eq!(
|
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"])
|
strings(&["m/plain"])
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
@ -641,10 +662,31 @@ mod tests {
|
||||||
fn configured_model_list_matching_nothing_falls_back_to_every_offered_model() {
|
fn configured_model_list_matching_nothing_falls_back_to_every_offered_model() {
|
||||||
// The claude names on an ACP agent that never set `availableModels`.
|
// The claude names on an ACP agent that never set `availableModels`.
|
||||||
let configured = strings(&["haiku", "sonnet", "opus"]);
|
let configured = strings(&["haiku", "sonnet", "opus"]);
|
||||||
|
let warned = AtomicBool::new(false);
|
||||||
assert_eq!(
|
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"])
|
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]
|
#[test]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue