diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 99c5be1ecc..e2cb830a1f 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -27530,358 +27530,6 @@ async fn hydration_eviction_note_survives_a_leading_error_result() { "the note rides only the first recorded result: {succeeded}" ); } - -#[tokio::test] -async fn retrieval_admission_eviction_note_rides_the_triggering_result() { - let (mut engine, _handle) = Engine::new(EngineConfig::default(), &Config::default()); - // Two cached deferred schemas sit near the collective budget; the - // evidence receipt admits the retrieval schema on top and overflows the - // cache, so the eviction must ride the receipt's own recorded result. - let catalog = vec![ - heavy_deferred_tool("retrieve_tool_result", 3_000), - heavy_deferred_tool("cached-a", 7_000), - heavy_deferred_tool("cached-b", 7_000), - ]; - let mut active = super::tool_catalog::initial_active_tools(&catalog); - let cached = vec!["cached-a".to_string(), "cached-b".to_string()]; - engine - .session - .tool_activation_cache - .activate(&catalog, &cached); - active.extend(cached); - - let mut live_catalog = catalog; - engine - .process_tool_results( - vec![Some(synthetic_toolbox_outcome( - "call-evidence", - "evidence-tool", - Ok(ToolResult { - content: "evidence payload".to_string(), - success: true, - metadata: Some(json!({"evidence_available": true})), - }), - ))], - &mut live_catalog, - &mut active, - &HashMap::new(), - &permissive_turn_policy(), - ) - .await; - - let evidence = recorded_tool_result_content(&engine, "call-evidence"); - assert!( - evidence.contains("cached-a"), - "the retrieval admission's eviction must ride the triggering result: {evidence}" - ); - assert!( - !engine - .session - .tool_activation_cache - .names() - .any(|name| name == "cached-a"), - "the evicted name must leave the cache" - ); -} - -#[tokio::test] -async fn execution_refresh_eviction_note_rides_the_refreshed_result() { - let (mut engine, _handle) = Engine::new(EngineConfig::default(), &Config::default()); - let priming_catalog = vec![ - heavy_deferred_tool("cached-a", 7_000), - heavy_deferred_tool("executed", 7_000), - ]; - let mut active = super::tool_catalog::initial_active_tools(&priming_catalog); - let cached = vec!["cached-a".to_string(), "executed".to_string()]; - engine - .session - .tool_activation_cache - .activate(&priming_catalog, &cached); - active.extend(cached); - - // The catalog's schema for `cached-a` grew after the activation (an MCP - // refresh). Refreshing `executed` revalidates against the grown catalog, - // the collective budget overflows, and the eviction must ride the - // refreshed tool's own recorded result. - let mut grown_catalog = vec![ - heavy_deferred_tool("cached-a", 12_000), - heavy_deferred_tool("executed", 7_000), - ]; - engine - .process_tool_results( - vec![Some(synthetic_toolbox_outcome( - "call-executed", - "executed", - Ok(ToolResult::success("executed payload")), - ))], - &mut grown_catalog, - &mut active, - &HashMap::new(), - &permissive_turn_policy(), - ) - .await; - - let executed = recorded_tool_result_content(&engine, "call-executed"); - assert!( - executed.contains("cached-a"), - "the refresh's revalidation eviction must ride the refreshed result: {executed}" - ); - assert!( - !engine - .session - .tool_activation_cache - .names() - .any(|name| name == "cached-a"), - "the evicted name must leave the cache" - ); -} - -#[tokio::test] -async fn step_note_names_a_victim_the_refresh_cannot_resurrect() { - use crate::mcp::{McpConfig, McpPool}; - use std::sync::Arc; - use tokio::sync::Mutex as AsyncMutex; - - let (mut engine, _handle) = Engine::new(EngineConfig::default(), &Config::default()); - // A server whose last connect failed auth-required: the pool's refreshed - // list serves the synthetic `mcp_probe_authenticate` tool, so the - // outcome's `mcp_catalog_changed` refresh runs in the same arm whose - // touch evicted it. The refresh must not resurrect the evicted entry - // onto the wire: the step note may name it truthfully. - let mut config = McpConfig::default(); - config.servers.insert( - "probe".to_string(), - crate::mcp::McpServerConfig { - command: None, - args: Vec::new(), - env: HashMap::new(), - cwd: None, - url: Some("https://probe.example.test/mcp".to_string()), - transport: None, - connect_timeout: None, - execute_timeout: None, - read_timeout: None, - disabled: false, - enabled: true, - required: false, - enabled_tools: Vec::new(), - disabled_tools: Vec::new(), - headers: HashMap::new(), - env_headers: HashMap::new(), - bearer_token_env_var: None, - scopes: Vec::new(), - oauth: None, - oauth_resource: None, - reviewed_plugin: None, - }, - ); - let mut pool = McpPool::new(config); - pool.note_connect_failure("probe", &anyhow::Error::msg("HTTP 401 unauthorized")); - engine.mcp_pool = Some(Arc::new(AsyncMutex::new(pool))); - - let victim = "mcp_probe_authenticate"; - let priming_catalog = vec![ - heavy_deferred_tool(victim, 7_000), - heavy_deferred_tool("executed", 7_000), - ]; - let mut active = super::tool_catalog::initial_active_tools(&priming_catalog); - let cached = vec![victim.to_string(), "executed".to_string()]; - engine - .session - .tool_activation_cache - .activate(&priming_catalog, &cached); - active.extend(cached); - - // The catalog's schema for the victim grew after the activation (an MCP - // refresh), so refreshing `executed` overflow-pops it — and the same - // outcome's `mcp_catalog_changed` refresh replaces the pool slice. The - // victim is still deferred in this conversation and no longer activated, - // so the refresh must leave it off the wire and the step note may name - // it: the next request must not carry a tool the note declares - // uncallable. - let mut grown_catalog = vec![ - heavy_deferred_tool(victim, 12_000), - heavy_deferred_tool("executed", 7_000), - ]; - engine - .process_tool_results( - vec![Some(synthetic_toolbox_outcome( - "call-executed", - "executed", - Ok( - ToolResult::success("executed payload").with_metadata(json!({ - "mcp_catalog_changed": true, - })), - ), - ))], - &mut grown_catalog, - &mut active, - &HashMap::new(), - &permissive_turn_policy(), - ) - .await; - - // Premise: the refresh must have actually replaced the pool slice — - // otherwise "nothing resurrects the victim" would hold vacuously. - assert!( - grown_catalog - .iter() - .any(|tool| tool.name == "mcp_probe_authenticate"), - "premise: the refresh must have swapped the pool's slice in" - ); - assert!( - !engine - .session - .tool_activation_cache - .names() - .any(|name| name == victim), - "the victim must leave the cache before the note renders" - ); - assert!( - !active.contains(victim), - "the refresh must not resurrect the evicted victim onto the wire: {active:?}" - ); - let executed = recorded_tool_result_content(&engine, "call-executed"); - assert!( - executed.contains("no longer callable") && executed.contains(victim), - "the step note must name the victim truthfully — it is not callable again: {executed}" - ); -} - -/// The cross-outcome window: a note that rides an early outcome of a batch -/// must stay truthful when a later outcome of the same batch refreshes the -/// MCP catalog. The refresh re-advertises eager entries and still-activated -/// deferred tools — never a bounds-evicted one — so the next request cannot -/// carry a tool an earlier result in the same pass declared uncallable. -#[tokio::test] -async fn refresh_between_outcomes_cannot_resurrect_an_evicted_tool() { - use crate::mcp::{McpConfig, McpPool}; - use std::sync::Arc; - use tokio::sync::Mutex as AsyncMutex; - - let (mut engine, _handle) = Engine::new(EngineConfig::default(), &Config::default()); - let mut config = McpConfig::default(); - config.servers.insert( - "probe".to_string(), - crate::mcp::McpServerConfig { - command: None, - args: Vec::new(), - env: HashMap::new(), - cwd: None, - url: Some("https://probe.example.test/mcp".to_string()), - transport: None, - connect_timeout: None, - execute_timeout: None, - read_timeout: None, - disabled: false, - enabled: true, - required: false, - enabled_tools: Vec::new(), - disabled_tools: Vec::new(), - headers: HashMap::new(), - env_headers: HashMap::new(), - bearer_token_env_var: None, - scopes: Vec::new(), - oauth: None, - oauth_resource: None, - reviewed_plugin: None, - }, - ); - let mut pool = McpPool::new(config); - pool.note_connect_failure("probe", &anyhow::Error::msg("HTTP 401 unauthorized")); - engine.mcp_pool = Some(Arc::new(AsyncMutex::new(pool))); - - let victim = "mcp_probe_authenticate"; - // Turn start: the victim's schema grew between turns, so the entry - // revalidation overflow-pops it and parks the turn-start note — the - // exact sequence `run_turn`'s entry runs. - let priming_catalog = vec![ - heavy_deferred_tool(victim, 7_000), - heavy_deferred_tool("filler", 9_000), - ]; - let mut active = super::tool_catalog::initial_active_tools(&priming_catalog); - let cached = vec![victim.to_string(), "filler".to_string()]; - engine - .session - .tool_activation_cache - .activate(&priming_catalog, &cached); - active.extend(cached); - let mut grown_catalog = vec![ - heavy_deferred_tool(victim, 12_000), - heavy_deferred_tool("filler", 9_000), - ]; - let removals = engine - .session - .tool_activation_cache - .revalidate(&grown_catalog); - super::tool_catalog::remove_toolbox_drops( - &mut active, - removals.dropped_names().map(str::to_string), - ); - assert_eq!( - removals.bounds_evicted, - vec![victim.to_string()], - "premise: the turn-start revalidation must evict the victim" - ); - engine.park_toolbox_eviction_note( - crate::core::session::ToolboxEvictionTrigger::TurnStart, - &removals.bounds_evicted, - ); - - // The batch: a first outcome records the parked note, then a later - // outcome in the same pass carries `mcp_catalog_changed`. The refresh - // must not put the victim back onto the wire behind the note's back. - engine - .process_tool_results( - vec![ - Some(synthetic_toolbox_outcome( - "call-filler", - "filler", - Ok(ToolResult::success("filler payload")), - )), - Some(synthetic_toolbox_outcome( - "call-auth", - victim, - Ok(ToolResult::success("auth payload").with_metadata(json!({ - "mcp_catalog_changed": true, - }))), - )), - ], - &mut grown_catalog, - &mut active, - &HashMap::new(), - &permissive_turn_policy(), - ) - .await; - - // Premise: the refresh must have actually replaced the pool slice — - // otherwise "nothing resurrects the victim" would hold vacuously. - assert!( - grown_catalog - .iter() - .any(|tool| tool.name == "mcp_probe_authenticate"), - "premise: the refresh must have swapped the pool's slice in" - ); - - let filler = recorded_tool_result_content(&engine, "call-filler"); - assert!( - filler.contains("no longer callable") && filler.contains(victim), - "the parked note must ride the batch's first result: {filler}" - ); - assert!( - !active.contains(victim), - "a later outcome's refresh must not resurrect the evicted victim onto the wire: {active:?}" - ); - assert!( - !engine - .session - .tool_activation_cache - .names() - .any(|name| name == victim), - "the victim stays unactivated: a search re-activates it" - ); -} - /// The mid-turn refresh must apply the same allow/deny narrowing the turn /// build ran over its catalog: a tool the build stripped must not re-enter /// the catalog and the active set on a `mcp_catalog_changed` outcome, or a @@ -28060,73 +27708,6 @@ async fn session_sync_drops_a_parked_toolbox_eviction_note() { handle.send(Op::Shutdown).await.expect("shutdown engine"); run_task.await.expect("engine task"); } - -/// A turn-start revalidation that evicts on the collective bounds (a cached -/// schema grew between turns) must park its note so the turn's first recorded -/// result carries it — the eviction may not ride a search that has not -/// happened yet. -#[tokio::test] -async fn turn_start_revalidation_parks_an_eviction_note_for_the_turn() { - use crate::llm_client::mock::{MockLlmClient, canned}; - - let workspace = tempdir().expect("tempdir"); - let mock = std::sync::Arc::new(MockLlmClient::new(vec![canned::simple_text_turn("done")])); - let client: crate::core::model_client::SharedModelClient = mock; - let (mut engine, _handle) = Engine::new_with_model_client( - deterministic_engine_config(workspace.path()), - &Config::default(), - client, - ); - let priming_catalog = vec![ - heavy_deferred_tool("cached-a", 7_000), - heavy_deferred_tool("cached-b", 7_000), - ]; - let cached = vec!["cached-a".to_string(), "cached-b".to_string()]; - engine - .session - .tool_activation_cache - .activate(&priming_catalog, &cached); - - // The catalog's schema for `cached-a` grew between turns (an MCP - // refresh): this turn's start revalidation evicts the LRU victim, and - // the note must be parked for the turn's first result. - let grown_catalog = vec![ - heavy_deferred_tool("cached-a", 12_000), - heavy_deferred_tool("cached-b", 7_000), - ]; - let policy = policy_for_catalog( - grown_catalog, - None, - None, - crate::tui::approval::ApprovalMode::Suggest, - ); - let mut turn = crate::core::turn::TurnContext::new(4); - let _ = engine.run_turn(&mut turn, policy, None, None).await; - - assert!( - !engine - .session - .tool_activation_cache - .names() - .any(|name| name == "cached-a"), - "the turn-start eviction must drop the LRU victim from the cache" - ); - let notes = &engine.session.pending_toolbox_eviction_notes; - assert_eq!( - notes.len(), - 1, - "the turn-start eviction must park its note for the turn's first result: {notes:?}" - ); - assert_eq!( - notes[0].trigger, - crate::core::session::ToolboxEvictionTrigger::TurnStart - ); - assert!( - notes[0].evicted.iter().any(|name| name == "cached-a"), - "the parked note must name the victim: {notes:?}" - ); -} - /// The ride-time netting pin: a note parked at the turn's start must not /// name a tool that the very result carrying it re-admits. The natural /// retry — the model calling the tool the turn-start eviction just dropped @@ -28185,114 +27766,6 @@ async fn turn_start_note_does_not_name_a_tool_the_result_re_admits() { "the receipt must not declare a tool uncallable that the same result re-admits: {receipt}" ); } - -/// A refused hydration loads nothing: the receipt must answer the call with -/// the shared oversize reason, but no "Loaded deferred tool" status may -/// announce a load that did not happen, and the refused name must stay out -/// of the batch hydration order — the batch activation is never asked to -/// admit it. -#[tokio::test] -async fn refused_hydration_emits_no_load_status_and_never_enters_the_batch() { - use crate::llm_client::mock::{MockLlmClient, canned}; - - let workspace = tempdir().expect("tempdir"); - let mock = std::sync::Arc::new(MockLlmClient::new(vec![canned::simple_text_turn("unused")])); - let client: crate::core::model_client::SharedModelClient = mock.clone(); - let (mut engine, handle) = Engine::new_with_model_client( - deterministic_engine_config(workspace.path()), - &Config::default(), - client, - ); - // `notify` has a registered execution path (so the direct call passes - // the dispatch gate) and its catalog entry is deferred with an oversize - // schema, so the hydration must refuse instead of load. The sibling - // keeps the single-tool fallback from force-activating it. - let mut registry = crate::tools::ToolRegistry::new(crate::tools::ToolContext::new( - workspace.path().to_path_buf(), - )); - registry.register(std::sync::Arc::new(crate::tools::notify::NotifyTool)); - let catalog = vec![ - heavy_deferred_tool("notify", 20_000), - heavy_deferred_tool("cached-b", 100), - ]; - let surface = test_tool_surface(&engine, registry, Some(catalog.clone()), AppMode::Agent); - let tool_registry = Some(&surface.registry); - let mut tool_uses = vec![super::streaming::ToolUseState { - id: "refuse-1".to_string(), - name: "notify".to_string(), - input: json!({}), - caller: None, - thought_signature: None, - input_buffer: String::new(), - input_parse_error: None, - }]; - let mut active = HashSet::new(); - let mut budget = crate::tools::tool_call_budget::ToolCallBudget::new(surface.max_tool_calls); - let mut turn = crate::core::turn::TurnContext::new(4); - let planned = engine - .plan_tool_calls( - mock.as_ref(), - &mut turn, - &surface, - &mut tool_uses, - &catalog, - tool_registry, - &mut active, - &mut budget, - AppMode::Agent, - ) - .await; - - let plan = planned - .plans - .iter() - .find(|plan| plan.id == "refuse-1") - .expect("the refused call must be planned"); - let receipt = plan - .guard_result - .as_ref() - .expect("the refusal must answer the call as a guard result"); - assert!( - receipt.content.contains("was not loaded") - && receipt - .content - .contains(&super::tool_catalog::toolbox_oversize_reason()), - "the receipt must refuse with the shared oversize reason: {}", - receipt.content - ); - assert!( - !receipt.content.contains("has now been loaded"), - "the receipt must not promise a load: {}", - receipt.content - ); - assert!( - !active.contains("notify"), - "the refused name must stay out of the batch hydration: {active:?}" - ); - assert_eq!( - receipt - .metadata - .as_ref() - .and_then(|metadata| metadata.get("executed")) - .and_then(serde_json::Value::as_bool), - Some(false), - "the refusal must be marked unexecuted, or the working set and the audit would record a call that never happened: {:?}", - receipt.metadata - ); - - // Nothing may announce the load: drain the engine's events and require - // that no status claims the tool was loaded. - let mut rx = handle.rx_event.write().await; - while let Ok(event) = rx.try_recv() { - if let crate::core::events::Event::Status { message } = event { - assert!( - !message.contains("Loaded deferred tool"), - "a refused hydration must not announce a load: {message}" - ); - } - } -} - /// The refusal's `executed: false` marker must reach the `tool.result` audit /// payload, not just the receipt metadata: the audit is the record that a /// refused, never-executed call is not a successful execution. Driven through @@ -28395,97 +27868,6 @@ async fn refusal_audit_payload_carries_the_executed_marker() { "the receipt travels the success path — only the executed marker distinguishes it: {line}" ); } - -/// The hydration batch's eviction park is production wiring, not just the -/// shared helper: a direct call that hydrates a deferred tool can itself -/// overflow the collective bounds and evict a cached victim. Driven through -/// the real `plan_tool_calls`, the batch must park the note for the turn's -/// first recorded result. -#[tokio::test] -async fn hydration_batch_eviction_parks_its_note_through_plan_tool_calls() { - use crate::llm_client::mock::{MockLlmClient, canned}; - - let workspace = tempdir().expect("tempdir"); - let mock = std::sync::Arc::new(MockLlmClient::new(vec![canned::simple_text_turn("unused")])); - let client: crate::core::model_client::SharedModelClient = mock.clone(); - let (mut engine, _handle) = Engine::new_with_model_client( - deterministic_engine_config(workspace.path()), - &Config::default(), - client, - ); - // `notify` has a registered execution path (so the direct call passes - // the dispatch gate) and its catalog entry is deferred and admittable — - // but admitting it on top of the pre-filled cache overflows the - // collective byte budget and evicts the LRU-front victim. - let mut registry = crate::tools::ToolRegistry::new(crate::tools::ToolContext::new( - workspace.path().to_path_buf(), - )); - registry.register(std::sync::Arc::new(crate::tools::notify::NotifyTool)); - let catalog = vec![ - heavy_deferred_tool("notify", 4_000), - heavy_deferred_tool("cached-a", 7_000), - heavy_deferred_tool("cached-b", 7_000), - ]; - engine - .session - .tool_activation_cache - .activate(&catalog, &["cached-a".to_string(), "cached-b".to_string()]); - let surface = test_tool_surface(&engine, registry, Some(catalog.clone()), AppMode::Agent); - let tool_registry = Some(&surface.registry); - let mut tool_uses = vec![super::streaming::ToolUseState { - id: "hydrate-1".to_string(), - name: "notify".to_string(), - input: json!({}), - caller: None, - thought_signature: None, - input_buffer: String::new(), - input_parse_error: None, - }]; - let mut active = HashSet::new(); - let mut budget = crate::tools::tool_call_budget::ToolCallBudget::new(surface.max_tool_calls); - let mut turn = crate::core::turn::TurnContext::new(4); - engine - .plan_tool_calls( - mock.as_ref(), - &mut turn, - &surface, - &mut tool_uses, - &catalog, - tool_registry, - &mut active, - &mut budget, - AppMode::Agent, - ) - .await; - - let notes = &engine.session.pending_toolbox_eviction_notes; - assert_eq!( - notes.len(), - 1, - "the hydration batch must park its eviction note for the turn's first recorded result: {notes:?}" - ); - assert_eq!( - notes[0].trigger, - crate::core::session::ToolboxEvictionTrigger::Step, - "a hydration is a non-search activation step: {notes:?}" - ); - assert_eq!( - notes[0].evicted, - vec!["cached-a".to_string()], - "the note must name the LRU-front victim the admission evicted: {notes:?}" - ); - assert!( - !engine.session.tool_activation_cache.contains("cached-a") - && engine.session.tool_activation_cache.contains("cached-b") - && engine.session.tool_activation_cache.contains("notify"), - "the victim must leave the cache while the sibling and the hydrated tool stay" - ); - assert!( - active.contains("notify"), - "the hydrated tool must join the active set: {active:?}" - ); -} - /// Turn-start revalidation and a hydration batch can both evict in the same /// turn; the parked notes must combine, not overwrite. #[tokio::test] @@ -28558,73 +27940,6 @@ async fn turn_start_sweeps_a_stranded_eviction_note() { "a stranded note must not survive into the turn to misattribute its evictions" ); } - -/// The turn-entry sweep must run before the turn-start park: a note -/// stranded by the previous turn is dropped while this turn's fresh -/// eviction note survives. Sweeping after the park would eat the fresh -/// note and lose the eviction report entirely. -#[tokio::test] -async fn turn_entry_sweep_precedes_the_turn_start_park() { - use crate::llm_client::mock::{MockLlmClient, canned}; - - let workspace = tempdir().expect("tempdir"); - let mock = std::sync::Arc::new(MockLlmClient::new(vec![canned::simple_text_turn("done")])); - let client: crate::core::model_client::SharedModelClient = mock; - let (mut engine, _handle) = Engine::new_with_model_client( - deterministic_engine_config(workspace.path()), - &Config::default(), - client, - ); - // Stranded by an interrupted previous turn. - engine.park_toolbox_eviction_note( - crate::core::session::ToolboxEvictionTrigger::Step, - &["ghost-tool".to_string()], - ); - - // This turn's start evicts on the collective bounds (a cached schema - // grew between turns) and parks its own note. - let priming_catalog = vec![ - heavy_deferred_tool("cached-a", 7_000), - heavy_deferred_tool("cached-b", 7_000), - ]; - let cached = vec!["cached-a".to_string(), "cached-b".to_string()]; - engine - .session - .tool_activation_cache - .activate(&priming_catalog, &cached); - let grown_catalog = vec![ - heavy_deferred_tool("cached-a", 12_000), - heavy_deferred_tool("cached-b", 7_000), - ]; - let policy = policy_for_catalog( - grown_catalog, - None, - None, - crate::tui::approval::ApprovalMode::Suggest, - ); - let mut turn = crate::core::turn::TurnContext::new(4); - let _ = engine.run_turn(&mut turn, policy, None, None).await; - - let notes = &engine.session.pending_toolbox_eviction_notes; - assert_eq!( - notes.len(), - 1, - "the stranded note must be swept and the fresh turn-start note must survive: {notes:?}" - ); - assert_eq!( - notes[0].trigger, - crate::core::session::ToolboxEvictionTrigger::TurnStart - ); - assert!( - notes[0].evicted.iter().any(|name| name == "cached-a"), - "the fresh note names this turn's victim: {notes:?}" - ); - assert!( - !notes[0].evicted.iter().any(|name| name == "ghost-tool"), - "the stranded note must not combine into this turn's report: {notes:?}" - ); -} - /// A batch whose outcomes all flattened away has no result to carry the /// parked note; it must be re-parked for a later batch instead of dropped. #[tokio::test] diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index d13c13860d..7828a9bcff 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -1505,8 +1505,8 @@ pub(super) fn execute_tool_search( execute_tool_search_inner(tool_name, input, catalog, active_tools, None, None) } -/// Execute tool search while retaining activated schemas in the bounded -/// conversation cache. Kept separate from the test-only pure search helper so existing +/// Execute tool search while retaining activated schemas in the conversation +/// cache. Kept separate from the test-only pure search helper so existing /// pure catalog tests and compatibility callers do not need an engine session. /// /// `connecting_mcp_servers` carries the boot-window status (#588): while the @@ -1597,6 +1597,12 @@ fn execute_tool_search_inner( }) }) .collect::>(); + // In the Pinvou fork the toolbox size bounds never fire (see the session + // constants): discovery pre-filters deferred membership, so the only way + // a name lands here is the per-entry sentinel — a schema whose serialized + // size cannot be measured (serde failure reports usize::MAX) — which no + // real schema reaches. The reason therefore claims no bound and + // interpolates no numbers. unavailable_references.extend(cache_rejected.iter().map(|name| { json!({ "type": "unavailable_tool_reference", diff --git a/crates/tui/src/core/engine/tool_catalog/tests.rs b/crates/tui/src/core/engine/tool_catalog/tests.rs index ed3d1a2cf3..bd2fcd7076 100644 --- a/crates/tui/src/core/engine/tool_catalog/tests.rs +++ b/crates/tui/src/core/engine/tool_catalog/tests.rs @@ -1,11 +1,10 @@ use super::{ - CODE_EXECUTION_DESCRIPTION, DEFAULT_ACTIVE_NATIVE_TOOLS, DeferredHydration, + CODE_EXECUTION_DESCRIPTION, DEFAULT_ACTIVE_NATIVE_TOOLS, allowlist_is_native_file_and_shell_only, apply_mcp_tool_deferral, apply_native_tool_deferral, apply_registry_first_shell_guidance, build_model_tool_catalog_with_surface, default_synthetic_catalog_tool_names, ensure_advanced_tooling, execute_tool_search_with_cache, - initial_active_tools, is_synthetic_catalog_tool, maybe_hydrate_requested_deferred_tool, - remove_toolbox_drops, render_toolbox_eviction_notes, tool_matches_any_rule, - toolbox_oversize_reason, toolbox_oversize_search_hint, touch_cached_tool_after_execution, + initial_active_tools, is_synthetic_catalog_tool, remove_toolbox_drops, + render_toolbox_eviction_notes, tool_matches_any_rule, touch_cached_tool_after_execution, }; use crate::core::session::ToolActivationCache; use crate::models::Tool; @@ -148,7 +147,10 @@ fn cache_eviction_does_not_hide_a_tool_that_became_eager() { } #[test] -fn successful_cached_execution_updates_lru_without_granting_uncached_names() { +fn forkguard_cached_execution_keeps_every_activation_without_granting_uncached_names() { + // Pinvou fork: the toolbox bounds are disabled, so activating a ninth + // deferred tool no longer displaces the least-recently-used entry. The + // recency touch and the uncached-name guarantee are unchanged. let catalog = (0..=8) .map(|index| { let mut definition = tool(&format!("deferred-{index}")); @@ -170,8 +172,9 @@ fn successful_cached_execution_updates_lru_without_granting_uncached_names() { let delta = cache.activate(&catalog, &["deferred-8".to_string()]); remove_toolbox_drops(&mut active, delta.dropped_names().map(str::to_string)); active.extend(delta.admitted); + assert_eq!(cache.names().count(), 9); assert!(cache.names().any(|name| name == "deferred-0")); - assert!(!cache.names().any(|name| name == "deferred-1")); + assert!(cache.names().any(|name| name == "deferred-1")); assert!( !touch_cached_tool_after_execution(&catalog, &mut active, &mut cache, "never-activated") @@ -351,140 +354,6 @@ fn settled_search_result_carries_no_booting_block() { ); } } - -/// The activation cache is bounded, so a search that admits new tools can -/// revoke ones the conversation already used. The result must name those -/// evictions — a tool that silently vanishes between turns is exactly the -/// "reference returned but never callable" trap. Searches without evictions -/// keep the legacy result shape with no eviction block at all. -#[test] -fn search_result_names_cache_evictions_instead_of_dropping_them_silently() { - let catalog: Vec = (0..10) - .map(|index| { - let mut definition = tool(&format!("deferred-{index}")); - definition.defer_loading = Some(true); - definition - }) - .collect(); - let mut active = initial_active_tools(&catalog); - let mut cache = ToolActivationCache::default(); - - let first = execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "deferred", "max_results": 8}), - &catalog, - &mut active, - &mut cache, - None, - ) - .expect("first search should succeed"); - let first_payload: serde_json::Value = - serde_json::from_str(&first.content).expect("payload is JSON"); - assert_eq!( - first_payload["tool_references"].as_array().unwrap().len(), - 8 - ); - assert!( - first_payload.get("evicted_tool_references").is_none(), - "a search without evictions must keep the legacy shape: {first_payload}" - ); - assert!(active.contains("deferred-0")); - assert_eq!(cache.names().count(), 8); - - // Naming deferred-8/9 admits two new tools into the full cache and must - // evict the two least-recently-used activations. - let second = execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "deferred-8 deferred-9", "max_results": 8}), - &catalog, - &mut active, - &mut cache, - None, - ) - .expect("second search should succeed"); - let second_payload: serde_json::Value = - serde_json::from_str(&second.content).expect("payload is JSON"); - assert_eq!( - second_payload["evicted_tool_references"], - json!([ - {"type": "tool_reference", "tool_name": "deferred-0"}, - {"type": "tool_reference", "tool_name": "deferred-1"}, - ]) - ); - let note = second_payload["toolbox_note"].as_str().expect("note"); - assert!( - note.contains("deferred-0") && note.contains("no longer callable"), - "the note must name the evictions and the recovery contract: {note}" - ); - assert!(!active.contains("deferred-0")); - assert!(!active.contains("deferred-1")); - assert!(active.contains("deferred-8")); - let metadata = second.metadata.expect("search metadata"); - assert_eq!( - metadata["evicted_tool_references"], - json!(["deferred-0", "deferred-1"]) - ); - assert_eq!(metadata["toolbox_note"], second_payload["toolbox_note"]); -} - -/// The serialized-bytes bound evicts too, not just the eight-name bound, and -/// the reporting must not care which bound fired. -#[test] -fn search_result_names_byte_bound_cache_evictions_too() { - let heavy = |name: &str, description_bytes: usize| { - let mut definition = tool(name); - definition.defer_loading = Some(true); - definition.description = "x".repeat(description_bytes); - definition - }; - let catalog = vec![ - heavy("deferred-small", 10_000), - heavy("deferred-large", 8_000), - ]; - let mut active = initial_active_tools(&catalog); - let mut cache = ToolActivationCache::default(); - - execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "^deferred-small", "match": "regex"}), - &catalog, - &mut active, - &mut cache, - None, - ) - .expect("first search should succeed"); - assert_eq!(cache.names().collect::>(), vec!["deferred-small"]); - - let second = execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "^deferred-large", "match": "regex"}), - &catalog, - &mut active, - &mut cache, - None, - ) - .expect("second search should succeed"); - let payload: serde_json::Value = - serde_json::from_str(&second.content).expect("payload is JSON"); - // Both tools sit under the per-tool cap, but together they overflow the - // 16KiB cache budget, so admitting the second evicts the first. - assert_eq!( - payload["evicted_tool_references"], - json!([{"type": "tool_reference", "tool_name": "deferred-small"}]) - ); - let note = payload["toolbox_note"].as_str().expect("note"); - assert!( - note.contains("deferred-small") && note.contains("16KiB"), - "the note must name the byte-bound eviction: {note}" - ); - assert!(!active.contains("deferred-small")); - let metadata = second.metadata.expect("search metadata"); - assert_eq!( - metadata["evicted_tool_references"], - json!(["deferred-small"]) - ); -} - /// A search after the catalog shrank must report the dropped activation under /// the unavailable vocabulary — the eviction note's "re-search to /// re-activate" contract would be a false promise for a tool that is gone @@ -602,251 +471,6 @@ fn search_does_not_report_an_eager_promotion_as_unavailable() { assert!(active.contains("promoted")); assert_eq!(cache.names().count(), 0); } - -/// A cached tool whose schema grows past the collective budget (an MCP -/// refresh shipping a larger schema) is still a cataloged, re-activatable -/// deferred member: the next search must pop it under the eviction note, not -/// as an unavailability. -#[test] -fn search_reports_growth_overflows_as_re_activatable_evictions() { - let heavy = |name: &str, description_bytes: usize| { - let mut definition = tool(name); - definition.defer_loading = Some(true); - definition.description = "x".repeat(description_bytes); - definition - }; - let catalog_v1 = vec![heavy("deferred-left", 8), heavy("deferred-right", 6_000)]; - let mut active = initial_active_tools(&catalog_v1); - let mut cache = ToolActivationCache::default(); - execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "deferred", "max_results": 8}), - &catalog_v1, - &mut active, - &mut cache, - None, - ) - .expect("first search should succeed"); - assert_eq!( - cache.names().collect::>(), - vec!["deferred-left", "deferred-right"] - ); - - let catalog_v2 = vec![ - heavy("deferred-left", 12_000), - heavy("deferred-right", 6_000), - ]; - let second = execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "^deferred-right", "match": "regex"}), - &catalog_v2, - &mut active, - &mut cache, - None, - ) - .expect("second search should succeed"); - let payload: serde_json::Value = - serde_json::from_str(&second.content).expect("payload is JSON"); - assert_eq!( - payload["evicted_tool_references"], - json!([{"type": "tool_reference", "tool_name": "deferred-left"}]), - "the growth overflow must be reported as a re-activatable eviction: {payload}" - ); - let unavailable = payload["unavailable_tool_references"] - .as_array() - .expect("unavailable references"); - assert!( - !unavailable - .iter() - .any(|reference| reference["tool_name"] == "deferred-left"), - "the growth victim is still cataloged, so the unavailable vocabulary would lie: {unavailable:?}" - ); - assert!(!active.contains("deferred-left")); - assert!(active.contains("deferred-right")); -} - -/// A cached tool whose schema grows past the per-tool budget is unavailable -/// rather than evicted, and the report must say exactly that — not claim the -/// tool left the catalog. The discovery-time rejection and the revalidation -/// drop describe the same condition, so the payload carries one entry. -#[test] -fn search_reports_oversize_drops_with_the_oversize_reason() { - let heavy = |name: &str, description_bytes: usize| { - let mut definition = tool(name); - definition.defer_loading = Some(true); - definition.description = "x".repeat(description_bytes); - definition - }; - let catalog_v1 = vec![heavy("deferred-left", 8)]; - let mut active = initial_active_tools(&catalog_v1); - let mut cache = ToolActivationCache::default(); - execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "^deferred-left", "match": "regex"}), - &catalog_v1, - &mut active, - &mut cache, - None, - ) - .expect("first search should succeed"); - assert_eq!(cache.names().collect::>(), vec!["deferred-left"]); - - let catalog_v2 = vec![heavy("deferred-left", 20_000)]; - let second = execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "^deferred-left", "match": "regex"}), - &catalog_v2, - &mut active, - &mut cache, - None, - ) - .expect("second search should succeed"); - let payload: serde_json::Value = - serde_json::from_str(&second.content).expect("payload is JSON"); - let unavailable = payload["unavailable_tool_references"] - .as_array() - .expect("unavailable references"); - let entries: Vec<&serde_json::Value> = unavailable - .iter() - .filter(|reference| reference["tool_name"] == "deferred-left") - .collect(); - assert_eq!( - entries.len(), - 1, - "the oversize drop must be reported exactly once: {unavailable:?}" - ); - assert_eq!( - entries[0]["reason"], - format!( - "{}{}", - toolbox_oversize_reason(), - toolbox_oversize_search_hint() - ), - "the oversize drop must carry the oversize reason with its search-context recovery hint, not the catalog-shrink one: {entries:?}" - ); - assert!( - payload.get("evicted_tool_references").is_none() && payload.get("toolbox_note").is_none(), - "an oversize drop is not a re-activatable eviction: {payload}" - ); - assert!(!active.contains("deferred-left")); -} - -/// The searched tool itself can be the LRU-front victim of a growth -/// overflow: revalidation pops it, the admission loop re-admits it, and the -/// same payload must not name it evicted while also handing it back in -/// `tool_references`. -#[test] -fn search_does_not_name_its_own_admission_as_evicted() { - let heavy = |name: &str, description_bytes: usize| { - let mut definition = tool(name); - definition.defer_loading = Some(true); - definition.description = "x".repeat(description_bytes); - definition - }; - let catalog_v1 = vec![heavy("deferred-left", 8), heavy("deferred-right", 6_000)]; - let mut active = initial_active_tools(&catalog_v1); - let mut cache = ToolActivationCache::default(); - execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "deferred", "max_results": 8}), - &catalog_v1, - &mut active, - &mut cache, - None, - ) - .expect("first search should succeed"); - assert_eq!( - cache.names().collect::>(), - vec!["deferred-left", "deferred-right"] - ); - - let catalog_v2 = vec![ - heavy("deferred-left", 12_000), - heavy("deferred-right", 6_000), - ]; - let second = execute_tool_search_with_cache( - super::TOOL_SEARCH_NAME, - &json!({"query": "^deferred-left", "match": "regex"}), - &catalog_v2, - &mut active, - &mut cache, - None, - ) - .expect("second search should succeed"); - let payload: serde_json::Value = - serde_json::from_str(&second.content).expect("payload is JSON"); - let references = payload["tool_references"].as_array().expect("references"); - assert!( - references - .iter() - .any(|reference| reference["tool_name"] == "deferred-left"), - "the searched growth victim must be re-admitted by this very result: {references:?}" - ); - assert_eq!( - payload["evicted_tool_references"], - json!([{"type": "tool_reference", "tool_name": "deferred-right"}]), - "only the net eviction may be named: {payload}" - ); - let note = payload["toolbox_note"].as_str().expect("note"); - assert!( - note.contains("deferred-right") && !note.contains("deferred-left"), - "the note must not claim the re-admitted tool is uncallable: {note}" - ); - assert!(active.contains("deferred-left")); - assert!(!active.contains("deferred-right")); - let metadata = second.metadata.expect("search metadata"); - assert_eq!( - metadata["evicted_tool_references"], - json!(["deferred-right"]) - ); -} - -/// A direct call to an oversize deferred tool can never enter the bounded -/// toolbox, so the hydration receipt must refuse with the shared oversize -/// reason instead of promising a load the admission would refuse. -#[test] -fn hydration_of_an_oversize_tool_reports_the_rejection_not_a_load() { - let mut oversize = tool("huge-deferred"); - oversize.defer_loading = Some(true); - oversize.description = "x".repeat(20_000); - let catalog = vec![oversize]; - // An empty active set: in a real turn core tools fill it, and the - // single-tool fallback must not pre-activate the tool under test. - let active = HashSet::new(); - let mut hydrated = HashSet::new(); - - let Some(DeferredHydration::Refused(receipt)) = maybe_hydrate_requested_deferred_tool( - "huge-deferred", - &json!({}), - &catalog, - &active, - &mut hydrated, - ) else { - panic!("the oversize call must be refused, not loaded"); - }; - assert!( - receipt - .content - .contains("was not loaded and was not executed"), - "the receipt must refuse the load: {}", - receipt.content - ); - assert!( - receipt.content.contains(&toolbox_oversize_reason()), - "the refusal must carry the shared oversize reason: {}", - receipt.content - ); - assert!( - !receipt.content.contains("has now been loaded"), - "the receipt must not promise a load: {}", - receipt.content - ); - assert!( - hydrated.is_empty(), - "an oversize tool must not be offered to the batch activation: {hydrated:?}" - ); -} - /// Ride-time netting: a parked note must not name a tool the riding result /// hands back. Names back in the activation cache — or in the request's /// active tool set, which a mid-turn MCP catalog refresh can repopulate diff --git a/crates/tui/src/core/session.rs b/crates/tui/src/core/session.rs index dd8177963b..c78cc91f9a 100644 --- a/crates/tui/src/core/session.rs +++ b/crates/tui/src/core/session.rs @@ -14,9 +14,30 @@ use std::path::PathBuf; /// Maximum number of deferred schemas a conversation may keep in its active /// toolbox. The permanent `read`/`write`/`edit`/`bash`/`agent`/`tool_search` /// router surface is not counted here. -pub(crate) const TOOL_ACTIVATION_CACHE_MAX_NAMES: usize = 8; +/// +/// Pinvou fork: effectively unbounded. Upstream bounds the toolbox to 8 +/// names / 16 KiB with silent LRU eviction; under provider prompt-cache +/// pricing every eviction mutates the request tools array and re-prefills the +/// entire prefix (write premiums included), which costs orders of magnitude +/// more than the ~0.1x cached-read saving it buys (codewhale-hq/Codewhale#6969), +/// and the live incident class was a model fabricating results for tools that +/// had silently vanished mid-task. This fork keeps the admission bookkeeping +/// and the catalog-shrink revalidation exactly as upstream — a disconnected +/// server, deny rule, mode switch, or eager promotion still drops entries — +/// but the size bounds never fire, so an activated schema stays callable for +/// the conversation's lifetime. `usize::MAX` itself cannot serve as the bound: +/// comparisons against the type extreme trip clippy's deny-by-default +/// `absurd_extreme_comparisons` (rustc's `unused_comparisons` joins in only +/// when the extreme is written as a numeric literal, not through a const +/// item), so the bound is an effectively-unbounded sentinel at +/// `usize::MAX / 2` — any slightly smaller value compiles; only the exact +/// extreme is lint-rejected. +pub(crate) const TOOL_ACTIVATION_CACHE_MAX_NAMES: usize = usize::MAX / 2; /// Maximum serialized bytes added to requests by cached deferred schemas. -pub(crate) const TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES: usize = 16 * 1024; +/// +/// Pinvou fork: effectively unbounded; see [`TOOL_ACTIVATION_CACHE_MAX_NAMES`] +/// for why the bound is `usize::MAX / 2` rather than `usize::MAX`. +pub(crate) const TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES: usize = usize::MAX / 2; /// Bounded, process-local conversation cache for deferred tools activated by /// `tool_search` or loaded by a direct call (hydration, execution refresh, @@ -24,7 +45,9 @@ pub(crate) const TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES: usize = 16 * 1024; /// /// Only names are retained. Every turn revalidates them against the currently /// filtered catalog, so a disconnected MCP server, changed allow/deny rule, or -/// mode switch cannot resurrect a tool from an older authority posture. +/// mode switch cannot resurrect a tool from an older authority posture. The +/// Pinvou fork disables the size bounds (see the constants above); upstream's +/// 8-name/16KiB LRU eviction no longer fires here. #[derive(Debug, Clone, Default)] pub(crate) struct ToolActivationCache { /// Least-recently used at the front, most-recently used at the back. @@ -242,9 +265,11 @@ impl ToolActivationCache { removals } - /// Touch requested deferred tools in search-result order. An oversized - /// schema is rejected; otherwise least-recently-used entries are evicted - /// until both bounds hold. + /// Touch requested deferred tools in search-result order. Upstream + /// rejects oversized schemas and evicts least-recently-used entries until + /// both size bounds hold; the Pinvou fork disables those bounds (see the + /// constants above), so those paths never fire and every request naming a + /// deferred catalog member is admitted. pub(crate) fn activate( &mut self, catalog: &[crate::models::Tool], @@ -625,9 +650,14 @@ mod tests { } #[test] - fn tool_activation_cache_is_lru_bounded_to_eight_names() { - let catalog = (0..10) - .map(|index| deferred_tool(&format!("tool_{index}"), 8)) + fn forkguard_deferred_toolbox_bounds_are_disabled() { + // The Pinvou fork disables the bounded-toolbox eviction (see the + // constant docs): activating far more names than upstream's historical + // 8-name bound must retain every admission and evict nothing. The + // 1 KiB schemas also push the total past upstream's historical 16 KiB + // byte budget, so a byte-bound-only revert fails here too. + let catalog = (0..64) + .map(|index| deferred_tool(&format!("tool_{index}"), 1024)) .collect::>(); let requested = catalog .iter() @@ -636,20 +666,30 @@ mod tests { let mut cache = ToolActivationCache::default(); let delta = cache.activate(&catalog, &requested); - assert_eq!(cache.names().count(), TOOL_ACTIVATION_CACHE_MAX_NAMES); - assert_eq!( - cache.names().collect::>(), - vec![ - "tool_2", "tool_3", "tool_4", "tool_5", "tool_6", "tool_7", "tool_8", "tool_9" - ] - ); - assert_eq!(delta.admitted.len(), TOOL_ACTIVATION_CACHE_MAX_NAMES); - assert!(delta.evicted.contains(&"tool_0".to_string())); - assert!(delta.evicted.contains(&"tool_1".to_string())); + assert_eq!(cache.names().count(), 64); + assert_eq!(delta.admitted, requested); + assert!(delta.evicted.is_empty()); + assert!(delta.rejected.is_empty()); } #[test] - fn touching_a_cached_tool_makes_it_most_recent() { + fn forkguard_oversized_schema_is_admitted_while_the_bounds_are_disabled() { + // 17 KiB exceeds upstream's historical 16 KiB per-tool bound; with the + // bounds disabled it is admitted like any other schema. The literal is + // sized against the historical bound because no constructible schema + // can approach the fork bound (usize::MAX / 2). + let catalog = vec![deferred_tool("huge", 17 * 1024)]; + let mut cache = ToolActivationCache::default(); + let delta = cache.activate(&catalog, &["huge".to_string()]); + + assert_eq!(cache.names().count(), 1); + assert_eq!(delta.admitted, vec!["huge".to_string()]); + assert!(delta.rejected.is_empty()); + assert!(delta.evicted.is_empty()); + } + + #[test] + fn forkguard_touching_a_cached_tool_moves_it_to_the_back_and_evicts_nothing() { let catalog = (0..9) .map(|index| deferred_tool(&format!("tool_{index}"), 8)) .collect::>(); @@ -663,24 +703,14 @@ mod tests { cache.activate(&catalog, &["tool_8".to_string()]); let names = cache.names().collect::>(); + // The recency order is still maintained (most recent at the back), but + // with the bounds disabled the touched-then-re-added tool no longer + // displaces the least-recently-used entry. + assert_eq!(names.len(), 9); assert!(names.contains(&"tool_0")); - assert!(!names.contains(&"tool_1")); + assert!(names.contains(&"tool_1")); assert_eq!(names.last().copied(), Some("tool_8")); } - - #[test] - fn oversized_schema_is_never_admitted() { - let catalog = vec![deferred_tool( - "huge", - TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES + 1, - )]; - let mut cache = ToolActivationCache::default(); - let delta = cache.activate(&catalog, &["huge".to_string()]); - - assert_eq!(cache.names().count(), 0); - assert_eq!(delta.rejected, vec!["huge"]); - } - #[test] fn revalidate_classifies_catalog_shrink_and_eager_removals() { let catalog = vec![ @@ -714,28 +744,6 @@ mod tests { // and the evicted vocabulary would be false statements. assert!(removals.bounds_evicted.is_empty()); } - - #[test] - fn revalidate_reports_byte_growth_overflows_as_re_activatable_evictions() { - let small = vec![deferred_tool("left", 8), deferred_tool("right", 6 * 1024)]; - let mut cache = ToolActivationCache::default(); - cache.activate(&small, &["left".to_string(), "right".to_string()]); - // The catalog's schema for `left` grows while it is cached (an MCP - // refresh): the collective budget now overflows, but `left` is still - // a deferred catalog member under the per-tool cap, so the LRU - // victim must pop as a re-activatable eviction, not a stale - // unavailability. - let grown = vec![ - deferred_tool("left", 12 * 1024), - deferred_tool("right", 6 * 1024), - ]; - - let removals = cache.revalidate(&grown); - assert_eq!(cache.names().collect::>(), vec!["right"]); - assert!(removals.stale.is_empty()); - assert_eq!(removals.bounds_evicted, vec!["left"]); - } - #[test] fn clearing_for_session_sync_forgets_all_activated_tools() { let catalog = vec![deferred_tool("one", 8), deferred_tool("two", 8)]; diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 8b09dd1421..70e81d349c 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -7596,470 +7596,17 @@ fn synthetic_deferred_tool(name: &str, description_bytes: usize) -> Tool { } } -fn synthetic_eager_tool(name: &str) -> Tool { - Tool { - tool_type: Some("function".to_string()), - name: name.to_string(), - description: "eager always-present tool".to_string(), - input_schema: json!({"type": "object", "properties": {}}), - allowed_callers: None, - defer_loading: Some(false), - input_examples: None, - strict: None, - cache_control: None, - } -} - -/// The child hydration receipt is the hydration's only result payload, so a -/// bounds eviction caused by the load must reach the child model through it. -#[test] -fn child_hydration_receipt_carries_the_eviction_note() { - // Two large warm activations sit near the collective budget; hydrating a - // third overflows the child cache. The 8,050-byte descriptions serialize - // to ~8,170-byte tools, so the warm pair totals ~16,340 bytes — roughly - // 40 bytes under the 16,384 collective budget: loading big-c overflows - // it, but the warm pair alone must not. If a Tool field or serde - // attribute changes serialized size, re-tune these numbers before - // trusting either half of this test. - let warm_catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("big-b", 8_050), - synthetic_deferred_tool("big-c", 8), - ]; - let mut premise = ToolActivationCache::default(); - assert!( - premise - .activate(&warm_catalog, &["big-a".to_string(), "big-b".to_string()]) - .evicted - .is_empty(), - "premise: the warm pair alone must fit the collective budget" - ); - let mut surface = - SubAgentToolSurface::new(warm_catalog, &["big-a".to_string(), "big-b".to_string()]); - - let receipt = surface.hydrate("big-c").expect("third activation"); - assert!( - receipt.contains("`big-c` was deferred and has now been loaded"), - "the receipt must confirm the load: {receipt}" - ); - assert!( - receipt.contains("evicted tools from the bounded conversation toolbox") - && receipt.contains("big-a"), - "the load receipt must carry the eviction note naming the LRU victim: {receipt}" - ); - assert!(!surface.active_names.contains("big-a")); -} - -/// The oversize hydration rejection runs after the activation, so its -/// revalidation may have bounds-evicted other tools. This error is the -/// call's only result payload: the rejection must carry both the shared -/// oversize reason and those evictions instead of absorbing them. -#[test] -fn child_hydrate_rejection_carries_the_revalidations_evictions() { - let warm_catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("big-b", 8_050), - synthetic_deferred_tool("big-c", 20_000), - ]; - let mut surface = - SubAgentToolSurface::new(warm_catalog, &["big-a".to_string(), "big-b".to_string()]); - // The catalog's schema for `big-a` grew after the warm start (a - // refresh): the rejection's activation revalidates against it and the - // collective budget overflows, while `big-c` itself is per-tool - // oversize and is refused. - surface.catalog = vec![ - synthetic_deferred_tool("big-a", 12_000), - synthetic_deferred_tool("big-b", 8_050), - synthetic_deferred_tool("big-c", 20_000), - ]; - - let error = surface - .hydrate("big-c") - .expect_err("big-c is per-tool oversize"); - let message = error.to_string(); - assert!( - message.contains("could not enter the bounded child toolbox") - && message.contains(&toolbox_oversize_reason()), - "the rejection must carry the shared oversize reason: {message}" - ); - assert!( - message.contains("no longer callable") && message.contains("big-a"), - "the rejection must carry the revalidation's eviction note: {message}" - ); - assert!( - !message.contains("has now been loaded"), - "the rejection must not promise a load: {message}" - ); - assert!( - surface.pending_eviction_notes.is_empty(), - "the evictions rode the error directly; nothing may stay parked" - ); -} - -/// The child's ride channel mirrors the parent's: parked notes ride success -/// and error results alike (a note held back on `Err` would strand until a -/// later `Ok` — or forever if the child ends first), and names the ride -/// re-admits are netted out against the child's live cache. -#[test] -fn child_parked_note_rides_success_and_error_results_and_nets_re_admissions() { - // A parked eviction note names cache members, and cache members are - // catalog members — the premise the ride-time catalog check relies on. - // The eager lead defeats `initial_active_tools`' single-tool fallback, - // which would otherwise force-activate `big-a` and the ride would net - // it out of the note. - let mut lead = synthetic_deferred_tool("lead", 8); - lead.defer_loading = Some(false); - let catalog = vec![ - lead, - synthetic_deferred_tool("big-a", 8), - synthetic_deferred_tool("big-b", 8), - synthetic_deferred_tool("big-c", 8), - ]; - let mut surface = SubAgentToolSurface::new(catalog, &[]); - surface.park_eviction_note( - ToolboxEvictionTrigger::Step, - vec!["big-a".to_string(), "big-b".to_string()].as_slice(), - ); - - let mut ok = Ok(RichToolResult::plain(ToolResult::success("payload"))); - surface.ride_pending_eviction_notes(&mut ok); - let ok = ok.expect("stays ok"); - assert!( - ok.result.content.contains("payload") - && ok.result.content.contains("no longer callable") - && ok.result.content.contains("big-a") - && ok.result.content.contains("big-b"), - "the note rides a success result: {}", - ok.result.content - ); - - surface.park_eviction_note(ToolboxEvictionTrigger::Step, &["big-c".to_string()]); - let mut failure: Result = Err(anyhow::anyhow!("boom")); - surface.ride_pending_eviction_notes(&mut failure); - let message = failure.expect_err("stays an error").to_string(); - assert!( - message.contains("boom") - && message.contains("no longer callable") - && message.contains("big-c"), - "the note must ride an error result too: {message}" - ); - assert!(surface.pending_eviction_notes.is_empty()); - - // A name the ride hands back is netted out; a fully netted note does - // not ride at all. - let mut deferred = synthetic_deferred_tool("big-a", 8); - deferred.name = "big-a".to_string(); - surface.cache.activate(&[deferred], &["big-a".to_string()]); - surface.park_eviction_note(ToolboxEvictionTrigger::Step, &["big-a".to_string()]); - let mut netted = Ok(RichToolResult::plain(ToolResult::success("clean"))); - surface.ride_pending_eviction_notes(&mut netted); - let netted = netted.expect("stays ok"); - assert!( - netted.result.content == "clean", - "a note naming only re-admitted tools must be absorbed: {}", - netted.result.content - ); -} - -/// An executed tool's refresh can evict on the collective bounds when a -/// cached schema grew; the child mirror of the parent's post-execution -/// channel must append the eviction note to the recorded result. -#[test] -fn child_execution_refresh_eviction_note_rides_the_child_result() { - let warm_catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("big-b", 8_050), - ]; - let mut surface = - SubAgentToolSurface::new(warm_catalog, &["big-a".to_string(), "big-b".to_string()]); - // The catalog's schema for `big-a` grew after the warm start (a - // refresh): refreshing `big-b` revalidates against the grown catalog - // and the collective budget overflows. - surface.catalog = vec![ - synthetic_deferred_tool("big-a", 12_000), - synthetic_deferred_tool("big-b", 8_050), - ]; - - let mut result: Result = - Ok(RichToolResult::plain(ToolResult::success("payload"))); - surface.touch_after_execution("big-b", &mut result); - - let rich = result.expect("the refresh result stays ok"); - assert!( - rich.result.content.contains("big-a") - && rich - .result - .content - .contains("evicted tools from the bounded conversation toolbox"), - "the refresh result must carry the eviction note naming the LRU victim: {}", - rich.result.content - ); - assert!(!surface.active_names.contains("big-a")); - assert!( - surface.pending_eviction_notes.is_empty(), - "the touch eviction rides the result directly, not the parked slot" - ); -} - -/// A note parked outside any result payload (a catalog refresh's -/// revalidation) must ride the child's next recorded result instead of -/// vanishing. -#[test] -fn child_parked_note_rides_the_next_result_and_refresh_parks_one() { - let warm_catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("big-b", 8_050), - ]; - let mut surface = - SubAgentToolSurface::new(warm_catalog, &["big-a".to_string(), "big-b".to_string()]); - assert!( - surface.pending_eviction_notes.is_empty(), - "a warm start inside the bounds must not park anything" - ); - - // The refresh grows `big-a` past the collective bounds: request_tools - // revalidates, the LRU victim pops, and the note must park for the - // child's next recorded result. The eager lead tool keeps the - // single-tool fallback from force-activating `big-a`. - let refreshed_catalog = vec![ - synthetic_eager_tool("small-eager"), - synthetic_deferred_tool("big-a", 12_000), - synthetic_deferred_tool("big-b", 8_050), - ]; - let tools = surface.request_tools(refreshed_catalog, false); - assert!( - !tools.iter().any(|tool| tool.name == "big-a"), - "the evicted tool must leave the child's request surface" - ); - let parked = &surface.pending_eviction_notes; - assert_eq!( - parked.len(), - 1, - "the refresh eviction must park a note: {parked:?}" - ); - assert!( - parked[0].evicted.iter().any(|name| name == "big-a"), - "the parked note must name the LRU victim: {parked:?}" - ); -} - -/// The park side is covered above; this pin is the consumption side — -/// `execute_from_surface` must hand parked notes to the child's recorded -/// result, the production edge that once discarded every report. Driven -/// through the same wrapper a real child tool call takes. -#[tokio::test] -async fn parked_note_rides_through_the_execute_from_surface_glue() { - let registry = small_surface_registry(FleetRole::Builder); - let catalog = registry.deferred_catalog_for_model(&FleetRole::Builder); - let mut surface = SubAgentToolSurface::new(catalog, &["get_goal".to_string()]); - // A parked eviction note names a cache member, and a cache member is a - // catalog member — the premise the ride-time catalog check relies on. - surface - .catalog - .push(synthetic_deferred_tool("ghost-tool", 8)); - surface.park_eviction_note( - ToolboxEvictionTrigger::TurnStart, - &["ghost-tool".to_string()], - ); - - let content = execute_surface_tool(®istry, &mut surface, "get_goal", json!({})) - .await - .expect("cached read tool executes"); - assert!( - content.contains("no longer callable") && content.contains("ghost-tool"), - "the production ride glue must attach the parked note to the recorded result: {content}" - ); - assert!( - surface.pending_eviction_notes.is_empty(), - "the ride consumes the parked notes: {:?}", - surface.pending_eviction_notes - ); -} - -/// The hydrate dispatch is production wiring too: a deferred tool outside -/// the request's active set must load through `execute_from_surface` — the -/// glue edge whose regression would dead-end every first call. Driven -/// through the same wrapper a real child tool call takes. -#[tokio::test] -async fn hydrate_dispatch_loads_a_deferred_tool_through_the_execute_from_surface_glue() { - let registry = small_surface_registry(FleetRole::Builder); - let catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("big-b", 8_050), - synthetic_deferred_tool("big-c", 8), - ]; - // Premise: the warm pair alone must fit the collective budget, so the - // overflow below is attributable to the third load (see the byte-tuning - // note on the receipt test above). - let mut premise = ToolActivationCache::default(); - assert!( - premise - .activate(&catalog, &["big-a".to_string(), "big-b".to_string()]) - .evicted - .is_empty(), - "premise: the warm pair must fit the collective budget" - ); - let mut surface = - SubAgentToolSurface::new(catalog, &["big-a".to_string(), "big-b".to_string()]); - - let content = execute_surface_tool(®istry, &mut surface, "big-c", json!({})) - .await - .expect("the deferred tool must hydrate through the glue, not dead-end"); - assert!( - content.contains("`big-c` was deferred and has now been loaded"), - "the glue's hydrate dispatch must load the tool: {content}" - ); - assert!( - surface.active_names.contains("big-c"), - "the hydration must activate the tool for the rest of this request: {:?}", - surface.active_names - ); -} - -/// The refresh channel's reporting is production wiring: an executed -/// deferred tool's touch eviction must ride the recorded result through -/// `execute_from_surface` itself — deleting the dispatch there must turn -/// this pin red, the exact round-2 defect shape (the child discarding its -/// helpers' reports). -#[tokio::test] -async fn touch_glue_rides_the_refresh_eviction_through_execute_from_surface() { - let registry = small_surface_registry(FleetRole::Builder); - let catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("get_goal", 8_050), - ]; - let mut surface = - SubAgentToolSurface::new(catalog, &["big-a".to_string(), "get_goal".to_string()]); - // The catalog's schema for `get_goal` grew after the warm start (a - // refresh): the executed tool's touch revalidates against it and the - // collective budget overflows. - surface.catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("get_goal", 8_400), - ]; - - let content = execute_surface_tool(®istry, &mut surface, "get_goal", json!({})) - .await - .expect("the cached tool executes"); - assert!( - content.contains("no longer callable") && content.contains("big-a"), - "the touch eviction must ride the recorded result through the glue: {content}" - ); - assert!( - !surface.active_names.contains("big-a"), - "the evicted victim must leave the child's active set" - ); -} - -/// A mid-request bounds eviction must not leave the evicted tool -/// dispatchable for the rest of the request: the request's active-name -/// snapshot still carries it, but the surface's live active set no longer -/// does. Routing the retry through hydration re-admits it under the bounds -/// with a truthful receipt (or refuses it with the shared oversize reason) -/// instead of executing a call the surface no longer admits — the eviction -/// note an earlier result recorded would otherwise be a false statement for -/// the rest of this request. Deleting the live-set disjunct in the dispatch -/// turns this pin red. -#[tokio::test] -async fn mid_request_eviction_routes_the_retry_through_hydration() { - let registry = small_surface_registry(FleetRole::Builder); - let catalog = vec![ - synthetic_deferred_tool("big-a", 8_050), - synthetic_deferred_tool("big-b", 8_050), - synthetic_deferred_tool("big-c", 8), - ]; - let mut surface = - SubAgentToolSurface::new(catalog, &["big-a".to_string(), "big-b".to_string()]); - // The request's active-name snapshot is taken once per request; call i - // (hydrating big-c) evicts big-a behind it. - let request_active: HashSet = ["big-a".to_string(), "big-b".to_string()] - .into_iter() - .collect(); - - let load = execute_surface_tool(®istry, &mut surface, "big-c", json!({})) - .await - .expect("big-c hydrates"); - assert!( - load.contains("`big-c` was deferred and has now been loaded") && load.contains("big-a"), - "premise: the mid-request hydration evicts big-a and says so: {load}" - ); - assert!( - !surface.active_names.contains("big-a"), - "premise: big-a left the surface's live active set" - ); - - // Call j retries big-a under the stale request snapshot. - let retry = registry - .execute_from_surface( - "agent_test", - "", - &mut surface, - &request_active, - "big-a", - json!({}), - ) - .await - .expect("the retry must rehydrate, not execute behind the eviction"); - assert!( - retry - .result - .content - .contains("`big-a` was deferred and has now been loaded"), - "the evicted retry must route through hydration: {}", - retry.result.content - ); - assert!( - retry.result.content.contains("big-b"), - "the rehydration's own eviction must be reported truthfully: {}", - retry.result.content - ); - assert!(surface.active_names.contains("big-a")); - assert!(!surface.active_names.contains("big-b")); -} - -/// The warm-start overflow park is defensive today (the only production -/// spawn passes no warm names) but it is the constructor's invariant guard: -/// deleting the park must turn this pin red. -#[tokio::test] -async fn warm_start_overflow_park_rides_the_first_result() { - let registry = small_surface_registry(FleetRole::Builder); - let catalog = registry.deferred_catalog_for_model(&FleetRole::Builder); - let deferred: Vec = catalog - .iter() - .filter(|tool| tool.defer_loading == Some(true) && tool.name != "get_goal") - .map(|tool| tool.name.clone()) - .take(8) - .collect(); - // Nine warm names exceed the eight-name cache: the LRU front victim - // pops at construction and its note must park for the first result. - // `get_goal` is warmed last (most recently used) so it survives and the - // pinned ride runs down the executed-tool path. - let mut warm = deferred.clone(); - warm.push("get_goal".to_string()); - let victim = warm[0].clone(); - let mut surface = SubAgentToolSurface::new(catalog, &warm); - let parked = &surface.pending_eviction_notes; - assert!( - parked - .iter() - .any(|note| note.evicted.iter().any(|name| name == &victim)), - "premise: the warm start must overflow and park the LRU victim: {parked:?}" - ); - - let content = execute_surface_tool(®istry, &mut surface, "get_goal", json!({})) - .await - .expect("cached read tool executes"); - assert!( - content.contains("no longer callable") && content.contains(&victim), - "the warm-start overflow note must ride the child's first result: {content}" - ); - assert!(surface.pending_eviction_notes.is_empty()); -} - -#[test] -fn small_surface_caches_are_independent_bounded_and_revalidated() { +// Pinvou fork: the toolbox size bounds are disabled (see the constant docs in +// core/session.rs). Upstream tests that construct near-budget activations to +// fire bounds eviction or oversize rejection are not carried here — those +// paths never fire in this fork; the ride-channel and revalidation pins that +// do not depend on the bounds are kept. +#[test] +fn forkguard_small_surface_caches_are_independent_unbounded_and_revalidated() { + // 2 KiB schemas put nine activations past upstream's historical 16 KiB + // byte budget as well as its 8-name bound, so both reverts fail here. let mut catalog = (0..9) - .map(|index| synthetic_deferred_tool(&format!("deferred_{index}"), 8)) + .map(|index| synthetic_deferred_tool(&format!("deferred_{index}"), 2 * 1024)) .collect::>(); ensure_advanced_tooling(&mut catalog, AppMode::Agent, &HashSet::new()); catalog.retain(|tool| tool.name == TOOL_SEARCH_NAME || tool.name.starts_with("deferred_")); @@ -8069,7 +7616,14 @@ fn small_surface_caches_are_independent_bounded_and_revalidated() { let mut first = SubAgentToolSurface::new(catalog.clone(), &warm); let mut second = SubAgentToolSurface::new(catalog, &[]); let first_names = model_tool_names(model_request_tools(&mut first)); - assert!(!first_names.contains("deferred_0")); + assert_eq!( + first_names + .iter() + .filter(|name| name.starts_with("deferred_")) + .count(), + 9 + ); + assert!(first_names.contains("deferred_0")); assert!(first_names.contains("deferred_8")); assert!(!model_tool_names(model_request_tools(&mut second)).contains("deferred_8")); @@ -8078,7 +7632,8 @@ fn small_surface_caches_are_independent_bounded_and_revalidated() { first .catalog .push(synthetic_deferred_tool("oversized", 17 * 1024)); - assert!(first.hydrate("oversized").is_err()); + assert!(first.hydrate("oversized").is_ok()); + assert!(model_tool_names(model_request_tools(&mut first)).contains("oversized")); let mut byte_catalog = (0..3) .map(|index| synthetic_deferred_tool(&format!("bytes_{index}"), 6 * 1024)) @@ -8090,7 +7645,7 @@ fn small_surface_caches_are_independent_bounded_and_revalidated() { .collect::>(); let mut byte_surface = SubAgentToolSurface::new(byte_catalog, &byte_warm); let byte_names = model_tool_names(model_request_tools(&mut byte_surface)); - assert!(!byte_names.contains("bytes_0")); + assert!(byte_names.contains("bytes_0")); assert!(byte_names.contains("bytes_1") && byte_names.contains("bytes_2")); } diff --git a/docs/TOOL_SURFACE.md b/docs/TOOL_SURFACE.md index d777a3ce15..0d571b638c 100644 --- a/docs/TOOL_SURFACE.md +++ b/docs/TOOL_SURFACE.md @@ -75,9 +75,18 @@ user explicitly names them in `[tools].always_load`. ### Conversation toolbox cache A successful search activation is remembered by name for the current -conversation. The cache holds at most eight deferred names and 16 KiB of -serialized tool definitions, evicts least-recently-used entries, and -revalidates every +conversation. Upstream bounds the cache to eight deferred names and 16 KiB of +serialized schemas with least-recently-used eviction; this fork disables that +bound (effectively unbounded), so every activation stays +advertised for the conversation's lifetime — under provider prompt-cache +pricing, an eviction re-prefills the entire prefix while saving only the ~0.1x +cached-read cost of the schemas it frees (see codewhale-hq/Codewhale#6969). +No size ceiling remains: the activated set is the only clamp on the request +tools array, so a session that activates very many large schemas can approach +provider request-size or tool-count limits. Bookkeeping grows with the +activated set too: revalidation and admission re-serialize cached schemas +against the catalog on each pass, negligible at tens of tools but not free +for activation-heavy sessions. The cache still revalidates every entry against the current catalog and policy before advertising it again. A session sync clears it. The cache cannot resurrect a removed, denied, or newly-eager tool. @@ -124,7 +133,8 @@ catalog-shrink drop left the catalog entirely and no later search can name it — the model learns of those only when a search for the capability misses. Each subagent gets its own policy-filtered deferred catalog, always-present -`tool_search`, and bounded activation cache. Forked messages and instructions +`tool_search`, and activation cache (unbounded in this fork, like the parent +conversation's). Forked messages and instructions remain in context, but the child cache starts empty and discovers tools locally; neither forked context nor a cache can become a discovery allowlist. A child can still search every tool its own authority permits, including Web search/fetch