diff --git a/crates/cli/src/metrics.rs b/crates/cli/src/metrics.rs index 179f4f1c18..694dba560c 100644 --- a/crates/cli/src/metrics.rs +++ b/crates/cli/src/metrics.rs @@ -1,10 +1,17 @@ //! `codewhale metrics` — reads the audit log and session/task stores and prints //! a human-readable usage rollup. //! -//! Data sources: -//! - `~/.deepseek/audit.log` — one JSON line per event (approvals, credentials) -//! - `~/.deepseek/sessions/` — saved session JSON files (tool call history) -//! - `~/.deepseek/tasks/runtime/events/` — runtime thread JSONL event streams +//! Data sources (bases resolve `CODEWHALE_HOME` → `DEEPSEEK_HOME` → +//! `~/.deepseek`): +//! - `audit.log` under the base — one JSON line per event (approvals, +//! credentials). The sensitive-audit writer resolves to +//! `CODEWHALE_HOME` → `~/.codewhale/audit.log` and never consults +//! `DEEPSEEK_HOME`, so without `CODEWHALE_HOME` set its events land in a +//! file this rollup does not read. +//! - `$CODEWHALE_TOOL_AUDIT_LOG` (opt-in) — engine tool receipts +//! (`tool.result`), written top-level with no `ts` stamp +//! - `sessions/` under the base — saved session JSON files (tool call history) +//! - `tasks/runtime/events/` under the base — runtime thread JSONL event streams use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -31,7 +38,15 @@ pub fn run(args: MetricsArgs) -> Result<()> { // Collect data from every source; treat missing files as empty. let mut rollup = Rollup::default(); - read_audit_log(&base.join("audit.log"), args.since, &mut rollup); + // The engine's only in-repo `tool.result` writer is the opt-in + // tool-audit log; it has no default path, so it is read only when the + // variable is set. Precedence mirrors the writer's: CODEWHALE first, + // then the legacy DEEPSEEK spelling. + let tool_audit_log = std::env::var_os("CODEWHALE_TOOL_AUDIT_LOG") + .or_else(|| std::env::var_os("DEEPSEEK_TOOL_AUDIT_LOG")); + for path in audit_log_paths(&base, tool_audit_log.as_deref()) { + read_audit_log(&path, args.since, &mut rollup); + } read_session_files(&base.join("sessions"), args.since, &mut rollup); read_runtime_events( &base.join("tasks").join("runtime").join("events"), @@ -240,6 +255,23 @@ impl Rollup { // Source readers // ────────────────────────────────────────────────────────────────────────────── +/// Audit-log files the rollup reads: the default `base/audit.log` first, +/// then the engine's opt-in tool-audit log when one is configured. That +/// writer (`CODEWHALE_TOOL_AUDIT_LOG`, falling back to +/// `DEEPSEEK_TOOL_AUDIT_LOG`) is the only in-repo source of `tool.result` +/// receipts. An opt-in path equal to the default collapses to one entry — +/// reading the same file twice would double-count every line. +fn audit_log_paths(base: &Path, opt_in: Option<&std::ffi::OsStr>) -> Vec { + let default = base.join("audit.log"); + let mut paths = vec![default.clone()]; + if let Some(opt_in) = opt_in.map(PathBuf::from) + && opt_in != default + { + paths.push(opt_in); + } + paths +} + /// Read one-JSON-line-per-event audit log. fn read_audit_log(path: &Path, since: Option>, rollup: &mut Rollup) { let content = match std::fs::read_to_string(path) { @@ -310,6 +342,7 @@ fn read_audit_log(path: &Path, since: Option>, rollup: &mut Rollup let tool_name = v .pointer("/details/tool_name") .or_else(|| v.pointer("/payload/tool_name")) + .or_else(|| v.get("tool_name")) .and_then(|t| t.as_str()) .unwrap_or("unknown"); let stats = rollup.tool_mut(tool_name); @@ -330,19 +363,30 @@ fn read_audit_log(path: &Path, since: Option>, rollup: &mut Rollup // oversize-hydration refusal) answered the call without the // tool ever running, so it counts as neither — counting it // as a success would inflate the tool's real execution rate. + // The engine's opt-in tool-audit log + // (`CODEWHALE_TOOL_AUDIT_LOG`) — the only in-repo + // `tool.result` writer — writes these fields at the top + // level of the line. The nested `details`/`payload` + // placements are tolerated for foreign or hand-written + // lines; no in-repo writer nests them. let success = v .pointer("/details/success") .or_else(|| v.pointer("/payload/success")) + .or_else(|| v.get("success")) .and_then(|b| b.as_bool()) .unwrap_or(true); let executed = v .pointer("/details/executed") .or_else(|| v.pointer("/payload/executed")) + .or_else(|| v.get("executed")) .and_then(|b| b.as_bool()) .unwrap_or(true); if success && executed { stats.successes += 1; - } else if !success { + } else if executed { + // Only a receipt whose tool actually ran can be a + // failure; an unexecuted one is neither, whatever its + // success flag says. stats.failures += 1; } } @@ -935,6 +979,45 @@ mod tests { assert_eq!(fmt_num(1_000_000), "1,000,000"); } + // ── audit_log_paths ── + + #[test] + fn audit_log_paths_unset_reads_default_only() { + let paths = audit_log_paths(Path::new("/home/user/.codewhale"), None); + assert_eq!( + paths, + vec![PathBuf::from("/home/user/.codewhale/audit.log")] + ); + } + + #[test] + fn audit_log_paths_opt_in_appends_after_default() { + let paths = audit_log_paths( + Path::new("/home/user/.codewhale"), + Some(std::ffi::OsStr::new("/tmp/tool-audit.log")), + ); + assert_eq!( + paths, + vec![ + PathBuf::from("/home/user/.codewhale/audit.log"), + PathBuf::from("/tmp/tool-audit.log"), + ] + ); + } + + #[test] + fn audit_log_paths_opt_in_equal_to_default_is_read_once() { + // Reading the same file twice would double-count every line. + let paths = audit_log_paths( + Path::new("/home/user/.codewhale"), + Some(std::ffi::OsStr::new("/home/user/.codewhale/audit.log")), + ); + assert_eq!( + paths, + vec![PathBuf::from("/home/user/.codewhale/audit.log")] + ); + } + // ── Rollup from audit log ── fn make_audit_line(event: &str, tool: &str, ts: &str) -> String { @@ -1013,15 +1096,25 @@ mod tests { ); writeln!(tmp, "{line_old}").unwrap(); writeln!(tmp, "{line_new}").unwrap(); + // The engine's opt-in tool-audit log writes `tool.result` lines with + // no `ts`; a line that cannot be dated is never excluded by + // `--since` — it counts whatever the cutoff. + writeln!( + tmp, + r#"{{"event":"tool.result","tool_name":"flat_undated","success":true}}"# + ) + .unwrap(); let cutoff: DateTime = "2026-01-01T00:00:00Z".parse().unwrap(); let mut rollup = Rollup::default(); read_audit_log(tmp.path(), Some(cutoff), &mut rollup); - // Only the newer line should be counted. - assert_eq!(rollup.parsed_lines, 1); + // Only the newer dated line and the undatable flat line are counted. + assert_eq!(rollup.parsed_lines, 2); assert!(!rollup.tools.contains_key("exec_shell")); assert_eq!(rollup.tools["read_file"].calls, 1); + assert_eq!(rollup.tools["flat_undated"].calls, 1); + assert_eq!(rollup.tools["flat_undated"].successes, 1); } #[test] @@ -1036,6 +1129,15 @@ mod tests { r#"{"event":"tool.result","ts":"2026-04-01T10:01:00+00:00","details":{"tool_name":"exec_shell","success":false}}"#, r#"{"event":"tool.result","ts":"2026-04-01T10:02:00+00:00","details":{"tool_name":"deferred_tool","success":true,"executed":false}}"#, r#"{"event":"tool.result","ts":"2026-04-01T10:03:00+00:00","details":{"tool_name":"other_tool","success":true,"executed":true}}"#, + // The engine's opt-in tool-audit log + // (`CODEWHALE_TOOL_AUDIT_LOG`) writes the same fields at the top + // level of the line instead of under `details`, and stamps no + // `ts` — the fixtures mirror that writer shape. + r#"{"event":"tool.result","tool_name":"flat_tool","success":true}"#, + r#"{"event":"tool.result","tool_name":"flat_refusal","success":true,"executed":false}"#, + // An unexecuted receipt is neither success nor failure whatever + // its success flag says — the tool never ran. + r#"{"event":"tool.result","tool_name":"flat_failed_refusal","success":false,"executed":false}"#, ]; for line in lines { writeln!(tmp, "{line}").unwrap(); @@ -1055,6 +1157,16 @@ mod tests { assert_eq!(rollup.tools["deferred_tool"].failures, 0); assert_eq!(rollup.tools["other_tool"].calls, 1); assert_eq!(rollup.tools["other_tool"].successes, 1); + // The flat placement is read: dropping the top-level fallback + // defaults these to executed successes. + assert_eq!(rollup.tools["flat_tool"].calls, 1); + assert_eq!(rollup.tools["flat_tool"].successes, 1); + assert_eq!(rollup.tools["flat_refusal"].successes, 0); + assert_eq!(rollup.tools["flat_refusal"].failures, 0); + // Deleting the `executed` guard on the failure arm turns this pin + // red: an unexecuted receipt is never a failure. + assert_eq!(rollup.tools["flat_failed_refusal"].successes, 0); + assert_eq!(rollup.tools["flat_failed_refusal"].failures, 0); } #[test] diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 68d4e3bf79..d7cb6cfcd4 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -3462,7 +3462,9 @@ impl Engine { // generated-ID new-session path), so never carry the // previous conversation's toolbox across this edge — // including a parked eviction note whose batch never - // recorded a result. + // recorded a result. The note clear is redundant with + // the turn-entry sweep and is kept so no future + // non-turn reader inherits cross-conversation state. self.session.tool_activation_cache.clear(); self.session.pending_toolbox_eviction_notes.clear(); let plugin_workspace_changed = diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index d8bfe86a8f..e8bf6e8584 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -9494,6 +9494,71 @@ fn runtime_mcp_refresh_replaces_the_pool_slice() { !active.contains("mcp_old_name") && active.contains("mcp_new_name"), "the renamed surface must be reflected in the active set: {active:?}" ); + + // A same-name refresh that changes what the tool *says* (a description + // or schema update) moves wire bytes without moving any name: the C5 + // content fingerprint sees drift the name-set identity cannot. When + // the name set ties, the declaration must fall back to the replaced + // entries' serialized bytes. + let mut stale_definition = api_tool("mcp_static_read"); + stale_definition.description = "a stale description".to_string(); + let mut catalog = vec![stale_definition, api_tool("exec_shell")]; + let mut active = HashSet::new(); + let universe: HashSet = ["mcp_static_read"] + .into_iter() + .map(str::to_string) + .collect(); + let wire_changed = replace_runtime_mcp_tools( + &mut catalog, + &mut active, + &universe, + vec![api_tool("mcp_static_read")], + &cache, + ); + assert!( + wire_changed, + "a same-name refresh that changes the served definition must be declared" + ); + let names: Vec<&str> = catalog.iter().map(|tool| tool.name.as_str()).collect(); + assert_eq!( + names, + vec!["mcp_static_read", "exec_shell"], + "the content-drift declaration still splices in place" + ); + + // The same content change on a tool the conversation never ships must + // NOT declare: a deferred tool that was never activated is invisible on + // the wire, so its description drift moves no wire bytes. The deferral + // flag survives the splice (the refresh restores the prior flag), and + // the empty activation cache keeps it out of the active set. + let mut deferred_stale = api_tool("mcp_static_read"); + deferred_stale.description = "another stale description".to_string(); + deferred_stale.defer_loading = Some(true); + let mut catalog = vec![deferred_stale, api_tool("exec_shell")]; + let mut active = HashSet::new(); + let wire_changed = replace_runtime_mcp_tools( + &mut catalog, + &mut active, + &universe, + vec![api_tool("mcp_static_read")], + &cache, + ); + assert!( + !wire_changed, + "content drift on a wire-invisible tool moves no wire bytes and must stay silent" + ); + assert!( + !active.contains("mcp_static_read"), + "the deferred entry must stay off the wire: {active:?}" + ); + assert_eq!( + catalog + .iter() + .find(|tool| tool.name == "mcp_static_read") + .and_then(|tool| tool.defer_loading), + Some(true), + "the splice restores the pre-existing deferral flag" + ); } #[test] @@ -28325,6 +28390,55 @@ async fn mid_turn_refresh_honors_the_turn_policy() { !active.contains("mcp_probe_authenticate"), "a build-stripped name must not re-enter the active set on a refresh: {active:?}" ); + + // The arm's positive side, against the same pool: under a policy that + // does not strip the tool, the refresh must actually admit it — into + // the catalog with the pool's own definition and into the active set. + // The deny asserts above hold vacuously if the `mcp_catalog_changed` + // arm never runs at all; these do not. + let mut admitted_catalog = Vec::new(); + let mut admitted_active = HashSet::new(); + engine + .process_tool_results( + vec![Some(synthetic_toolbox_outcome( + "call-auth-allowed", + "unrelated", + Ok(ToolResult::success("payload").with_metadata(json!({ + "mcp_catalog_changed": true, + }))), + ))], + &mut admitted_catalog, + &mut admitted_active, + &HashMap::new(), + &permissive_turn_policy(), + ) + .await; + + assert!( + admitted_catalog + .iter() + .any(|tool| tool.name == "mcp_probe_authenticate"), + "an allowed pool tool must enter the catalog on a refresh: {admitted_catalog:?}" + ); + let expected = { + let pool = engine.mcp_pool.as_ref().expect("pool").lock().await; + pool.to_api_tools() + .into_iter() + .find(|tool| tool.name == "mcp_probe_authenticate") + .expect("premise: the needs-auth pool serves the synthetic login tool") + }; + let entered = admitted_catalog + .iter() + .find(|tool| tool.name == "mcp_probe_authenticate") + .expect("premise: the tool entered the catalog"); + assert_eq!( + entered.description, expected.description, + "the admitted entry must carry the pool's definition, not a stale one" + ); + assert!( + admitted_active.contains("mcp_probe_authenticate"), + "the eager synthetic login tool must be re-advertised onto the wire: {admitted_active:?}" + ); } #[tokio::test] @@ -28471,13 +28585,14 @@ 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}" ); } -/// 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 -/// the real refusal receipt this commit's marker reads, the audit line must -/// carry the marker verbatim. +/// The hydration receipt — the `DeferredHydration::Loaded` arm; the oversize +/// `Refused` arm is unreachable in this fork because the per-tool budget is +/// the disabled `usize::MAX / 2` sentinel — answers the call without +/// executing the tool. Its `executed: false` marker must reach the +/// `tool.result` audit payload verbatim, not just the receipt metadata, so +/// the audit cannot record a never-executed call as a successful execution. #[tokio::test] -async fn refusal_audit_payload_carries_the_executed_marker() { +async fn hydration_receipt_audit_payload_carries_the_executed_marker() { use crate::llm_client::mock::{MockLlmClient, canned}; let _guard = lock_test_env(); @@ -28502,7 +28617,7 @@ async fn refusal_audit_payload_carries_the_executed_marker() { 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-audit".to_string(), + id: "hydrate-audit".to_string(), name: "notify".to_string(), input: json!({}), caller: None, @@ -28530,19 +28645,19 @@ async fn refusal_audit_payload_carries_the_executed_marker() { let plan = planned .plans .iter() - .find(|plan| plan.id == "refuse-audit") - .expect("the refused call must be planned"); + .find(|plan| plan.id == "hydrate-audit") + .expect("the hydrated call must be planned"); let receipt = plan .guard_result .as_ref() - .expect("the refusal must answer the call as a guard result") + .expect("the hydration receipt must answer the call as a guard result") .clone(); // The results phase consumes the receipt exactly as production does. engine .process_tool_results( vec![Some(synthetic_toolbox_outcome( - "refuse-audit", + "hydrate-audit", "notify", Ok(receipt), ))], @@ -28558,14 +28673,14 @@ async fn refusal_audit_payload_carries_the_executed_marker() { .lines() .find_map(|line| { let value: serde_json::Value = serde_json::from_str(line).ok()?; - (value.get("event")? == "tool.result" && value.get("tool_id")? == "refuse-audit") + (value.get("event")? == "tool.result" && value.get("tool_id")? == "hydrate-audit") .then_some(value) }) - .expect("the refusal's tool.result audit line must exist"); + .expect("the hydration receipt's tool.result audit line must exist"); assert_eq!( line.get("executed").and_then(serde_json::Value::as_bool), Some(false), - "the audit must record the refused call as never executed: {line}" + "the audit must record the hydration-answered call as never executed: {line}" ); assert_eq!( line.get("success").and_then(serde_json::Value::as_bool), diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index 7828a9bcff..0de5af83f5 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -430,11 +430,22 @@ const TOOLBOX_CATALOG_SHRINK_REASON: &str = "Previously active in this conversat /// Why a deferred tool stays out of the bounded conversation toolbox: its /// serialized form — name, description, and schema measured together — /// exceeds the per-tool budget, so no activation can admit it while it -/// stays that large. Shared by discovery-time rejections and revalidation -/// drops so the prose cannot drift from the real bounds. Search-context -/// callers append the recovery hint via [`toolbox_oversize_search_hint`]; -/// a direct-call refusal has no query to narrow and states the bounds only. +/// stays that large. In the Pinvou fork the bounds never fire (see the +/// session constants): the only reachable trigger is the serde-failure +/// measurement sentinel, and the prose then claims no bound and +/// interpolates no numbers. Shared by discovery-time rejections and +/// revalidation drops so the prose cannot drift from the real bounds. +/// Search-context callers append the recovery hint via +/// [`toolbox_oversize_search_hint`]; a direct-call refusal has no query +/// to narrow and carries the reason alone. pub(crate) fn toolbox_oversize_reason() -> String { + if crate::core::session::TOOL_ACTIVATION_CACHE_MAX_NAMES == usize::MAX / 2 + && crate::core::session::TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES == usize::MAX / 2 + { + return "The serialized tool definition could not be measured for admission to the \ + conversation toolbox." + .to_string(); + } format!( "The serialized tool exceeds the bounded conversation toolbox ({} \ cached tools, {}KiB of serialized tool definitions).", @@ -451,25 +462,40 @@ pub(crate) fn toolbox_oversize_search_hint() -> &'static str { } /// [`toolbox_oversize_reason`] with the search-context recovery hint, for -/// the `tool_search` payload's unavailable references. +/// the `tool_search` payload's unavailable references. The hint rides only +/// a real budget rejection: under the fork's disabled sentinel the reason +/// reports a measurement failure, which no narrower query can fix. fn search_oversize_reason() -> String { - format!( - "{}{}", - toolbox_oversize_reason(), - toolbox_oversize_search_hint() - ) + let reason = toolbox_oversize_reason(); + if crate::core::session::TOOL_ACTIVATION_CACHE_MAX_NAMES == usize::MAX / 2 + && crate::core::session::TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES == usize::MAX / 2 + { + return reason; + } + format!("{}{}", reason, toolbox_oversize_search_hint()) } /// The model-facing sentence naming a bounds-driven toolbox eviction and the /// recovery contract. `trigger` names the step that surfaced it; the capacity -/// label is derived from the session constants so the prose cannot drift from -/// the real bounds. The eviction may come from the admission itself or from -/// revalidation after a cached schema grew, so the wording stays neutral -/// about the cause. +/// label renders only when the bounds are real — under the fork's disabled +/// sentinel bounds there is no meaningful capacity to quote, so the +/// parenthetical is omitted. The eviction may come from the admission itself +/// or from revalidation after a cached schema grew, so the wording stays +/// neutral about the cause. pub(crate) fn bounded_toolbox_eviction_note( trigger: ToolboxEvictionTrigger, evicted: &[String], ) -> String { + if crate::core::session::TOOL_ACTIVATION_CACHE_MAX_NAMES == usize::MAX / 2 + && crate::core::session::TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES == usize::MAX / 2 + { + return format!( + "This {} evicted tools from the conversation toolbox; the evicted tools \ + are no longer callable until a later search re-activates them: {}", + trigger.label(), + evicted.join(", ") + ); + } format!( "This {} evicted tools from the bounded conversation toolbox \ ({} cached tools, {}KiB of serialized tool definitions); the evicted tools \ @@ -482,9 +508,10 @@ pub(crate) fn bounded_toolbox_eviction_note( } /// Park a bounds-driven toolbox eviction for a later result to carry, -/// combining with any note already parked — turn-start revalidation and a -/// hydration batch can both evict before a result records. Shared by the -/// parent engine and the child surface so the combine contract is stated +/// queueing beside any note already parked — turn-start revalidation and a +/// hydration batch can both evict before a result records, and names +/// repeated across notes are netted when the notes render. Shared by the +/// parent engine and the child surface so the queueing contract is stated /// and pinned once. pub(crate) fn park_toolbox_eviction( notes: &mut Vec, @@ -1601,8 +1628,9 @@ fn execute_tool_search_inner( // 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. + // real schema reaches. Under that disabled-bounds posture the reason + // claims no bound and interpolates no numbers; were the bounds real, + // toolbox_oversize_reason would quote them instead. unavailable_references.extend(cache_rejected.iter().map(|name| { json!({ "type": "unavailable_tool_reference", @@ -1654,11 +1682,14 @@ fn execute_tool_search_inner( "tool_references": references, "unavailable_tool_references": unavailable_references.clone(), }); - // The activation cache is bounded (8 cached tools, 16KiB of serialized - // tool definitions), so admitting new tools can silently revoke ones this - // conversation already used. Naming the evictions in the result keeps - // the model's picture of its callable toolbox honest — otherwise it sees - // a previously working tool disappear between turns with no explanation. + // Upstream bounds the activation cache (8 cached tools, 16KiB of + // serialized tool definitions), so admitting new tools can silently + // revoke ones this conversation already used. This fork disables the + // bounds (see the session constants), so in-fork the eviction block + // below stays empty and the reporting exists for upstream parity: + // naming the evictions in the result keeps the model's picture of its + // callable toolbox honest — otherwise it sees a previously working tool + // disappear between turns with no explanation. if !cache_evicted.is_empty() { payload["evicted_tool_references"] = json!( cache_evicted diff --git a/crates/tui/src/core/engine/tool_catalog/tests.rs b/crates/tui/src/core/engine/tool_catalog/tests.rs index bd2fcd7076..e919ef0ea1 100644 --- a/crates/tui/src/core/engine/tool_catalog/tests.rs +++ b/crates/tui/src/core/engine/tool_catalog/tests.rs @@ -378,6 +378,14 @@ fn search_reports_catalog_stale_evictions_as_unavailable_not_evicted() { None, ) .expect("first search should succeed"); + assert_eq!( + cache.names().count(), + 3, + "premise: the first search must admit all three deferred tools into the cache — the \ + initial_active_tools empty-scan fallback already put deferred-0 into the active set \ + before any search ran, so the active-set assert below is vacuous and only the cache \ + proves admission" + ); assert!(active.contains("deferred-0")); let catalog_v2: Vec = catalog_v1 @@ -441,6 +449,13 @@ fn search_does_not_report_an_eager_promotion_as_unavailable() { None, ) .expect("first search should succeed"); + assert_eq!( + cache.names().count(), + 1, + "premise: the first search must activate the tool into the cache — the active set \ + already contains it via the initial_active_tools fallback, so only the cache proves \ + admission" + ); assert!(active.contains("promoted")); catalog[0].defer_loading = Some(false); @@ -514,7 +529,7 @@ fn toolbox_note_rendering_nets_re_admitted_names_at_ride_time() { ); assert_eq!( rendered - .matches("evicted tools from the bounded conversation toolbox") + .matches("evicted tools from the conversation toolbox") .count(), 1, "a companion note the netting empties must contribute no prose: {rendered}" @@ -574,6 +589,14 @@ fn toolbox_note_rendering_nets_re_admitted_names_at_ride_time() { 1, "a name already rendered by an earlier note must not be repeated: {rendered}" ); + assert!( + rendered.contains("This turn's start evicted"), + "first-note-wins: the surviving mention keeps the earlier note's attribution: {rendered}" + ); + assert!( + !rendered.contains("This step evicted"), + "first-note-wins: the deduped later note contributes no attribution: {rendered}" + ); assert!(notes.is_empty()); // A name that left the catalog between its eviction and this ride can @@ -622,3 +645,35 @@ fn toolbox_note_rendering_nets_re_admitted_names_at_ride_time() { "a fully unrecoverable note must drop the recovery promise: {rendered}" ); } + +/// Pinvou fork: the toolbox bounds are sentinel-disabled +/// (`TOOL_ACTIVATION_CACHE_MAX_*` are `usize::MAX / 2`), so no activation can +/// ever hit them and the oversize and eviction prose must claim no bound, +/// numeric or otherwise — a bound that never fires is a false statement +/// about the runtime. +#[test] +fn forkguard_toolbox_prose_claims_no_bound_under_the_disabled_sentinel() { + let reason = super::toolbox_oversize_reason(); + assert_eq!( + reason, + "The serialized tool definition could not be measured for admission \ + to the conversation toolbox." + ); + assert!( + !reason.chars().any(|ch| ch.is_ascii_digit()), + "the disabled-sentinel prose must name no numeric bound: {reason}" + ); + + use crate::core::session::ToolboxEvictionTrigger; + let note = + super::bounded_toolbox_eviction_note(ToolboxEvictionTrigger::Step, &["x".to_string()]); + assert_eq!( + note, + "This step evicted tools from the conversation toolbox; the evicted tools are \ + no longer callable until a later search re-activates them: x" + ); + assert!( + !note.contains("bounded"), + "the disabled-sentinel note must not claim a bound the fork disabled: {note}" + ); +} diff --git a/crates/tui/src/core/engine/tool_execution.rs b/crates/tui/src/core/engine/tool_execution.rs index 19709a145a..6517b3f2cf 100644 --- a/crates/tui/src/core/engine/tool_execution.rs +++ b/crates/tui/src/core/engine/tool_execution.rs @@ -199,9 +199,13 @@ pub(crate) fn emit_tool_audit(event: serde_json::Value) { emit_tool_audit_to_path(&PathBuf::from(path), event); } -/// Preserve only non-private identity fields for restricted embedded turns. -/// Tool inputs, outputs, paths, ids, policy details, and future payload keys -/// are omitted by construction instead of relying on every caller to redact. +/// Preserve only non-private fields for restricted embedded turns. Tool +/// inputs, outputs, paths, ids, policy details, and future payload keys are +/// omitted by construction instead of relying on every caller to redact. +/// `success` and `executed` are non-private booleans and survive: the +/// `codewhale metrics` rollup defaults a missing flag to `true`, so +/// stripping them would bucket every restricted receipt — failures +/// included — as an executed success. pub(crate) fn tool_audit_event_for_policy( restricted: bool, event: serde_json::Value, @@ -216,6 +220,11 @@ pub(crate) fn tool_audit_event_for_policy( if let Some(tool_name) = event.get("tool_name").and_then(serde_json::Value::as_str) { projected["tool_name"] = serde_json::json!(tool_name); } + for flag in ["success", "executed"] { + if let Some(value) = event.get(flag).and_then(serde_json::Value::as_bool) { + projected[flag] = serde_json::json!(value); + } + } projected } @@ -1039,18 +1048,27 @@ mod tests { let event = tool_audit_event_for_policy( true, json!({ - "event": "tool.error", + "event": "tool.result", "tool_name": "File", "tool_input": private, "error": format!("failed at C:/private/{private}"), + "path": format!("/private/{private}"), + "success": false, + "executed": false, }), ); let encoded = serde_json::to_string(&event).expect("audit json"); assert!(!encoded.contains(private)); - assert_eq!(event["event"], "tool.error"); + assert_eq!(event["event"], "tool.result"); assert_eq!(event["tool_name"], "File"); assert_eq!(event["restricted"], true); + // Non-private booleans survive: the metrics rollup defaults a + // missing flag to `true`, so dropping them would misbucket this + // restricted failure as an executed success. + assert_eq!(event["success"], false); + assert_eq!(event["executed"], false); assert!(event.get("tool_input").is_none()); assert!(event.get("error").is_none()); + assert!(event.get("path").is_none()); } } diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 3330fcf7e9..999896168c 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1060,15 +1060,22 @@ fn registered_tool_requires_non_bypassable_approval(tool_name: &str) -> bool { /// marking across the refresh: adopting the pool's flag would flip deferred /// tools back onto the head of the wire list and bust the cached prefix. /// A name the conversation has never seen (the synthetic login tool a -/// mid-turn 401 just started offering, or a server-side rename) arrives -/// with the pool's eager flag and joins the active set: the 401 result's -/// auth hint points straight at that tool, so deferring it would strand -/// the recovery path behind a search. Replaced entries are spliced back -/// into their own catalog slot, so a refresh that serves the same names -/// reorders nothing and the caller's declaration (this return value: did -/// the model-visible wire list change?) stays false — a same-count -/// refresh moves no wire bytes and must not trip the C5 undeclared-drift -/// guard as a false positive. +/// mid-turn 401 just started offering, a server-side rename, or a server's +/// full real tool set after a completed login) arrives with the pool's +/// eager flag and joins the active set. The auth-hint case needs it: the +/// 401 result's auth hint points straight at that tool, so deferring it +/// would strand the recovery path behind a search. Bulk arrivals after a +/// login follow the same rule deliberately: the graceful direct-call +/// hydration path bounds what deferring would cost, but a fresh login's +/// tools are advertised at once. Replaced entries are spliced back into +/// their own catalog slot, so a refresh that serves the same names reorders +/// nothing. The caller's declaration (this return value: did the +/// model-visible wire list change?) then rests on content, not on the name +/// count: a same-name refresh with byte-identical served definitions moves +/// no wire bytes and stays false; one whose served definition changed under +/// the same name splices new wire bytes and declares — the wire-JSON +/// compare below exists so that drift is declared rather than tripping the +/// C5 undeclared-drift guard. pub(super) fn replace_runtime_mcp_tools( tool_catalog: &mut Vec, active_tool_names: &mut std::collections::HashSet, @@ -1083,8 +1090,24 @@ pub(super) fn replace_runtime_mcp_tools( .collect(); // The wire list is the eager head plus the activated deferred tail; // that identity — not the catalog's length — is what a declared re-pin - // answers for. + // answers for. Membership alone is still not the whole story: a + // same-name refresh can change what a served tool *says* (description, + // schema), which the C5 content fingerprint sees as wire bytes without + // any name moving. When the visible name set ties, the wire-visible + // replaced entries' JSON — the exact bytes the fingerprint hashes — + // decides; a tool the conversation never ships is skipped so its drift + // cannot declare what no request will see. let wire_before = wire_visible_tool_names(tool_catalog, active_tool_names); + let previous_wire_json: std::collections::HashMap> = tool_catalog + .iter() + .filter(|tool| universe.contains(&tool.name)) + .map(|tool| { + ( + tool.name.clone(), + crate::prefix_cache::tool_to_api_json(tool), + ) + }) + .collect(); let mut slot_by_name: std::collections::HashMap = tool_catalog .iter() .enumerate() @@ -1115,7 +1138,30 @@ pub(super) fn replace_runtime_mcp_tools( } tool_catalog.retain(|tool| !departed.contains(&tool.name)); tool_catalog.extend(arrivals); - wire_visible_tool_names(tool_catalog, active_tool_names) != wire_before + let wire_after = wire_visible_tool_names(tool_catalog, active_tool_names); + if wire_after != wire_before { + return true; + } + // Same visible membership: declare only when a wire-visible tool's + // served JSON changed under its name. Because the visible sets tie, + // every visible-after name was visible before and so has a snapshot + // entry; a name outside the visible set never ships, so its content + // drift moves no wire bytes and must not declare. A name without a + // predecessor is a new arrival — an eager arrival already moved the + // name set above, and a deferred one is invisible on the wire. + // `tool_to_api_json` is the exact serializer the fingerprint hashes + // for the non-strict request shape; under strict mode the fingerprint + // hashes sanitized copies, and raw-identical definitions stay + // sanitized-identical, so this compare can only over-declare there, + // never miss. + tool_catalog + .iter() + .filter(|tool| universe.contains(&tool.name) && wire_after.contains(&tool.name)) + .any(|tool| { + previous_wire_json + .get(&tool.name) + .is_some_and(|before| *before != crate::prefix_cache::tool_to_api_json(tool)) + }) } /// Every name the next request would serialize: the eager head plus the @@ -5144,8 +5190,10 @@ impl Engine { } /// Park a bounded-toolbox eviction note for the next recorded tool - /// result, combining with any note already parked — turn-start + /// result, queueing beside any note already parked — turn-start /// revalidation and a hydration batch can both evict in the same turn. + /// Names repeated across notes are netted when the notes render onto a + /// result. pub(super) fn park_toolbox_eviction_note( &mut self, trigger: crate::core::session::ToolboxEvictionTrigger, diff --git a/crates/tui/src/core/session.rs b/crates/tui/src/core/session.rs index 8ef5a86b4b..5079b3c9c8 100644 --- a/crates/tui/src/core/session.rs +++ b/crates/tui/src/core/session.rs @@ -33,21 +33,22 @@ use std::path::PathBuf; /// `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. +/// Maximum serialized bytes added to requests by cached deferred tool +/// definitions. /// /// 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 +/// Process-local conversation cache for deferred tools activated by /// `tool_search` or loaded by a direct call (hydration, execution refresh, -/// retrieval admission). +/// retrieval admission). Bounded upstream; the Pinvou fork disables the size +/// bounds (see the constants above), so in-fork the cache is unbounded and +/// upstream's 8-name/16KiB LRU eviction never fires. /// /// 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. The -/// Pinvou fork disables the size bounds (see the constants above); upstream's -/// 8-name/16KiB LRU eviction no longer fires here. +/// mode switch cannot resurrect a tool from an older authority posture. #[derive(Debug, Clone, Default)] pub(crate) struct ToolActivationCache { /// Least-recently used at the front, most-recently used at the back. @@ -117,7 +118,9 @@ pub(crate) enum ToolboxEvictionTrigger { /// A `tool_search` result naming the evictions in its own payload. ToolSearch, /// A non-search activation step: a hydration batch, a retrieval - /// admission, or an executed tool's refresh. + /// admission, an executed tool's refresh, or — on the child surface — + /// a request-rebuild revalidation or a warm-start overflow, which have + /// no turn boundary of their own to attribute. Step, /// The turn-entry revalidation. TurnStart, @@ -216,6 +219,17 @@ impl ToolActivationCache { /// Shared by revalidation and admission so the bounds condition and the /// LRU-front-first policy live in exactly one place. fn pop_collective_overflow(&mut self, catalog: &[crate::models::Tool]) -> Vec { + // Under the fork's disabled bounds the loop condition can never hold + // for real schemas — the serde-failure sentinel is removed as stale + // by revalidation before this runs — so skip the full-cache + // re-serialization each condition evaluation would cost. Without the + // early return every `activate` pays it once per requested name and + // every `revalidate` pays it once. + if TOOL_ACTIVATION_CACHE_MAX_NAMES == usize::MAX / 2 + && TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES == usize::MAX / 2 + { + return Vec::new(); + } let mut popped = Vec::new(); while self.names.len() > TOOL_ACTIVATION_CACHE_MAX_NAMES || self.total_serialized_bytes(catalog) > TOOL_ACTIVATION_CACHE_MAX_SCHEMA_BYTES @@ -232,7 +246,9 @@ impl ToolActivationCache { /// filtered catalog and enforce both cache bounds, classifying every /// removal by recoverability. A tool that became eager is forgotten /// from the cache without being reported: it is directly callable, and - /// the eager catalog entry must stay active. + /// the eager catalog entry must stay active. In the Pinvou fork the + /// bounds-enforcement half never fires (see the constants above); only + /// the membership/eager/oversize-sentinel classification is live. pub(crate) fn revalidate(&mut self, catalog: &[crate::models::Tool]) -> ToolActivationRemovals { let mut removals = ToolActivationRemovals::default(); self.names.retain(|name| { @@ -269,7 +285,10 @@ impl ToolActivationCache { /// 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. + /// deferred catalog member is admitted. The one admission rejection left + /// is the serde-failure measurement sentinel (see + /// `toolbox_oversize_reason`'s doc) — unreachable for the concrete + /// `Tool` type, whose derived serialization cannot fail. pub(crate) fn activate( &mut self, catalog: &[crate::models::Tool], diff --git a/crates/tui/src/prefix_cache.rs b/crates/tui/src/prefix_cache.rs index 7d23f99982..d72af7a04e 100644 --- a/crates/tui/src/prefix_cache.rs +++ b/crates/tui/src/prefix_cache.rs @@ -711,7 +711,7 @@ impl PrefixStabilityManager { /// Serialize a tool to the same JSON shape the chat API receives, /// excluding internal-only fields like `allowed_callers`, `defer_loading`, /// `input_examples`, and `cache_control` that are never sent to DeepSeek. -fn tool_to_api_json(tool: &Tool) -> Option { +pub(crate) fn tool_to_api_json(tool: &Tool) -> Option { let mut value = serde_json::json!({ "type": "function", "function": { diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index a92f87054f..bdc0bda33b 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -1304,6 +1304,15 @@ fn sanitize_kimi_parameters_candidate( /// and only its values are visited. `oneOf` needs no visit: this runs after /// `normalize_kimi_compatibility` has converted every non-root `oneOf` to /// `anyOf`, and root composition was already flattened before that. +/// Schema values under keywords MFJS rejects (`contains`, `prefixItems`, +/// `patternProperties`, `not`, `if`/`then`/`else`, `propertyNames`, +/// `unevaluatedProperties`, `unevaluatedItems`, nested `allOf`, tuple-form +/// `items`) need no visit: the keyword itself fails closed — +/// `UnsupportedKeyword`, `UnsupportedNestedAllOf` for a nested `allOf`, or +/// `InvalidSchemaNode` for tuple-form `items` — before a stranded empty +/// `items` node inside one could matter. `dependentSchemas` is the one +/// exception in shape: `strip_dependent_keywords` removes it (with its +/// subtree) rather than failing closed, so it cannot strand one either. /// /// Children are visited before the node's own `items` check so a nested /// repair can empty the parent (`"items": {"items": {}}` is vacuously true @@ -2014,6 +2023,10 @@ fn mfjs_schema_can_terminate( Ok(true) } Some("array") => { + // An absent or non-integer `minItems` reads as 0. Non-integer + // values are unreachable here only because `validate_mfjs_bounds` + // rejects them (InvalidRange) on every node before the terminator + // runs — keep that call order if the walkers are reordered. if obj.get("minItems").and_then(Value::as_u64).unwrap_or(0) == 0 { return Ok(true); } @@ -2623,6 +2636,37 @@ mod kimi_tests { nested_positions["properties"]["choice"]["anyOf"][1], json!({"type": "array"}) ); + + // The root's own object-form `additionalProperties` is a schema-value + // position too: an empty `items` there repairs to the keyword being + // absent and the tool ships. + let mut root_additional = json!({ + "type": "object", + "properties": {"name": {"type": "string"}}, + "additionalProperties": {"type": "array", "items": {}} + }); + sanitize_for_kimi_parameters(&mut root_additional) + .expect("root additionalProperties empty items repair"); + assert_eq!( + root_additional["additionalProperties"], + json!({"type": "array"}) + ); + + // An empty `items` beside a `$ref` drops the same way, leaving the + // clean `$ref` node — MFJS admits no sibling beyond + // title/description next to a reference, so without the repair the + // whole tool would fail closed with InvalidReference. + let mut ref_with_items = json!({ + "type": "object", + "properties": {"a": {"$ref": "#/$defs/a", "items": {}}}, + "$defs": {"a": {"type": "string"}} + }); + sanitize_for_kimi_parameters(&mut ref_with_items) + .expect("$ref with an empty items sibling repairs"); + assert_eq!( + ref_with_items["properties"]["a"], + json!({"$ref": "#/$defs/a"}) + ); } #[test] @@ -2696,6 +2740,33 @@ mod kimi_tests { "the anyOf branch's variant is InvalidSchemaNode: {error:?}" ); + // The anyOf check is branch-order dependent: the terminator stops + // at the first terminating branch, so a terminating branch ahead + // of the offending one lets the tool ship — the repaired schema is + // instance-set-equivalent (the empty `items` was a no-op + // constraint), which is the desired outcome, but it means the + // fail-closed guarantee for anyOf holds only when the offending + // branch is reached. + let mut schema = json!({ + "type": "object", + "properties": { + "a": { + "anyOf": [ + {"type": "string"}, + {"type": "array", "items": {}, "minItems": 1} + ] + } + }, + "required": ["a"] + }); + sanitize_for_kimi_parameters(&mut schema) + .expect("a terminating branch ahead of the offending one ships the tool"); + assert_eq!( + schema["properties"]["a"]["anyOf"][1], + json!({"type": "array", "minItems": 1}), + "the shipped branch is the repaired, instance-set-equivalent form" + ); + let mut schema = json!({ "type": "object", "$defs": {"a": {"type": "array", "items": {}, "minItems": 1}}, diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index cc9a107193..54f8268d63 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -14265,7 +14265,10 @@ impl SubAgentToolSurface { }; match result.as_mut() { Ok(rich) => rich.result.content.push_str(&format!("\n\n{rendered}")), - Err(error) => *error = anyhow!("{error}\n\n{rendered}"), + // `{error:#}` renders the source chain into the message: the + // rebuild replaces the error wholesale, and a bare `{error}` + // would silently drop everything but its top cause. + Err(error) => *error = anyhow!("{error:#}\n\n{rendered}"), } } @@ -14347,16 +14350,30 @@ impl SubAgentToolSurface { } return Ok(message); } - // The only rejection here is the per-tool budget: a narrower search - // cannot help, so the recovery hint must say why the tool cannot - // enter instead of promising a narrower query would. The rejection - // runs after the activation, so revalidation may have bounds-evicted - // other tools on the way; this error is the call's only result - // payload, so the note rides it instead of being absorbed. - let mut message = format!( - "Tool {name} could not enter the bounded child toolbox: {}", - toolbox_oversize_reason() - ); + // Pick the truthful rejection cause. Only the per-tool budget keeps + // the oversize prose: a narrower search cannot help there, so the + // reason says why the tool cannot enter instead of promising a + // narrower query would. A name absent from the child's filtered + // catalog has nothing to activate, and a name that is not deferred + // is directly callable already — neither is a budget rejection. + // The rejection runs after the activation, so revalidation may have + // bounds-evicted other tools on the way; this error is the call's + // only result payload, so the note rides it instead of being + // absorbed. + let mut message = match self.catalog.iter().find(|tool| tool.name == name) { + None => format!( + "Tool {name} could not enter the child toolbox: no such deferred tool in the \ + child catalog." + ), + Some(tool) if !tool.defer_loading.unwrap_or(false) => format!( + "Tool {name} could not enter the child toolbox: it is not deferred, so it is \ + directly callable and hydration has nothing to load." + ), + Some(_) => format!( + "Tool {name} could not enter the child toolbox: {}", + toolbox_oversize_reason() + ), + }; if !activation.evicted.is_empty() { message.push_str(&format!( "\n\n{}", diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 4d58ebaa3f..b9821fc494 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -7669,6 +7669,108 @@ async fn small_surface_successful_cached_use_touches_lru() { assert!(model_tool_names(model_request_tools(&mut surface)).contains("get_goal")); } +/// 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()]); + // The error rebuild renders the full source chain (`{error:#}`): the + // context is the top level and "boom" its source, so a bare-display + // rebuild would keep "outer" and silently drop "boom". + use anyhow::Context as _; + let mut failure: Result = Err(anyhow::anyhow!("boom")).context("outer"); + surface.ride_pending_eviction_notes(&mut failure); + let message = failure.expect_err("stays an error").to_string(); + assert!( + message.contains("outer") + && message.contains("boom") + && message.contains("no longer callable") + && message.contains("big-c"), + "the note must ride an error result with its full source chain: {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 deferred = synthetic_deferred_tool("big-a", 8); + 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 + ); +} + +/// 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 + ); +} + #[test] fn small_surface_depth_cap_removes_only_agent() { let mut runtime = diff --git a/docs/TOOL_SURFACE.md b/docs/TOOL_SURFACE.md index 0d571b638c..6d9fa62362 100644 --- a/docs/TOOL_SURFACE.md +++ b/docs/TOOL_SURFACE.md @@ -75,17 +75,17 @@ user explicitly names them in `[tools].always_load`. ### Conversation toolbox cache A successful search activation is remembered by name for the current -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 +conversation. Upstream bounds the cache to eight deferred names and 16 KiB +of serialized tool definitions 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 +activated set too: revalidation and admission re-serialize cached tool +definitions 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 @@ -98,7 +98,9 @@ error), a retrieval activation or a refresh of an executed tool rides the triggering result, and a turn's start revalidation parks its note on the session so the turn's first recorded result carries it (a refresh only evicts when a cached tool's serialized definition grew since its last -activation enough that the collective budget overflows). A note that never reaches a recorded result is dropped rather +activation enough that the collective budget overflows). With the size bounds +disabled in this fork no bounds eviction ever fires, so this channel stays +silent — it is upstream-parity machinery. A note that never reaches a recorded result is dropped rather than riding an unrelated later result: a session sync clears it at the conversation edge, and a turn that ends without a recorded result has it swept at the next turn's entry. Child agents report the same way on their @@ -125,12 +127,19 @@ the model cannot get back by searching are reported differently: whether the tool left the catalog (a disconnect, a changed allow/deny rule, a mode switch — any catalog-shrink cause) or outgrew the per-tool serialized-size budget. A cached tool that became -eager is not reported at all — it stays directly callable. Unavailable drops -observed by non-search paths are absorbed silently: an oversize drop is -re-reported with its reason the next time a search returns the tool (a -match ranked past the result cap re-reports nothing), while a -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. +eager is not reported at all — it stays directly callable. A catalog-shrink +drop is reported only when a search observes it first: every search +revalidates the cache before admitting its matches, so a cached-but-departed +name surfaces in that result's unavailable references with the +catalog-shrink reason, whether or not the query would have matched it. Any +other first observer — a turn boundary's revalidation, a hydration batch, +an executed tool's refresh, a retrieval admission, or the child surface's +request rebuild — absorbs the drop silently: the name leaves the cache +there, so no later search can name it and the model learns of it only when +a search for the capability misses. An oversize drop observed by a +non-search path is likewise silent in the moment but re-reported with its +reason the next time a search returns the tool (a match ranked past the +result cap re-reports nothing). Each subagent gets its own policy-filtered deferred catalog, always-present `tool_search`, and activation cache (unbounded in this fork, like the parent