diff --git a/Cargo.lock b/Cargo.lock index 06669c1cfe..0ab65b43cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3793,6 +3793,7 @@ dependencies = [ "jcode-logging", "jcode-message-types", "jcode-provider-core", + "jcode-schema-dialect", "serde", "serde_json", ] @@ -3935,6 +3936,7 @@ dependencies = [ "httpdate", "jcode-logging", "jcode-message-types", + "jcode-schema-dialect", "rand 0.9.3", "reqwest 0.12.28", "serde", @@ -4021,10 +4023,12 @@ dependencies = [ "chrono", "jcode-base", "jcode-message-types", + "jcode-provider-anthropic", "jcode-provider-antigravity", "jcode-provider-core", "jcode-provider-gemini", "jcode-provider-openai", + "jcode-provider-openrouter", "jcode-schema-dialect", "reqwest 0.12.28", "serde", @@ -4073,6 +4077,7 @@ dependencies = [ "jcode-message-types", "jcode-provider-core", "jcode-provider-openai", + "jcode-schema-dialect", "reqwest 0.12.28", "serde_json", "tempfile", @@ -4092,6 +4097,7 @@ dependencies = [ "jcode-core", "jcode-logging", "jcode-message-types", + "jcode-schema-dialect", "reqwest 0.12.28", "serde", "serde_json", @@ -4133,6 +4139,7 @@ name = "jcode-schema-dialect" version = "0.1.0" dependencies = [ "dirs", + "jcode-schema-dialect", "serde", "serde_json", "tempfile", diff --git a/crates/jcode-app-core/src/tool/discover.rs b/crates/jcode-app-core/src/tool/discover.rs index 035d56210f..21f307b7f3 100644 --- a/crates/jcode-app-core/src/tool/discover.rs +++ b/crates/jcode-app-core/src/tool/discover.rs @@ -232,32 +232,32 @@ struct DiscoverToolsInput { #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum DiscoveryAction { Search, - Setup, + Select, Suggest, } impl DiscoveryAction { - /// Parse the requested phase. `search`/`setup` are the current names; - /// `browse`/`select` are accepted as aliases so transcripts, benchmark + /// Parse the requested phase. `search`/`select` are the current names; + /// `browse`/`setup` are accepted as aliases so transcripts, benchmark /// baselines, and in-flight sessions recorded under the old vocabulary /// keep working. fn parse(action: Option<&str>, has_tool: bool) -> Result { match action.map(str::trim).filter(|value| !value.is_empty()) { - None => Ok(if has_tool { Self::Setup } else { Self::Search }), + None => Ok(if has_tool { Self::Select } else { Self::Search }), Some("search" | "browse") if !has_tool => Ok(Self::Search), - Some("setup" | "select") if has_tool => Ok(Self::Setup), + Some("select" | "setup") if has_tool => Ok(Self::Select), Some("suggest") if !has_tool => Ok(Self::Suggest), Some("search" | "browse") => Err(anyhow::anyhow!( - "integration action 'search' cannot include `tool`; use action 'setup'" + "integration action 'search' cannot include `tool`; use action 'select'" )), - Some("setup" | "select") => Err(anyhow::anyhow!( - "integration action 'setup' requires the chosen `tool` name" + Some("select" | "setup") => Err(anyhow::anyhow!( + "integration action 'select' requires the chosen `tool` name" )), Some("suggest") => Err(anyhow::anyhow!( "integration action 'suggest' cannot include `tool`; use `product_name` for a known product" )), Some(other) => Err(anyhow::anyhow!( - "unknown integration action '{other}'. Available: search, setup, suggest" + "unknown integration action '{other}'. Available: search, select, suggest" )), } } @@ -558,7 +558,10 @@ impl Tool for DiscoverToolsTool { fn description(&self) -> &str { "Call this before using any product, service, dev tool, API, or data source you \ don't already have a tool for. Tools in this catalog are vetted integrations \ - partnered with this harness to ensure quality and seamless one-call setup." + partnered with this harness to ensure quality and seamless one-call setup. \ + After deciding, call it again with action `select` and the product actually \ + chosen, including an off-catalog product. Off-catalog selections are recorded \ + but return no provider information or setup instructions." } fn parameters_schema(&self) -> Value { @@ -570,8 +573,8 @@ impl Tool for DiscoverToolsTool { "intent": super::intent_schema_property(), "action": { "type": "string", - "enum": ["search", "setup", "suggest"], - "description": "Phase. Defaults to setup when `tool` is set, else search. Suggest only when no result fits." + "enum": ["search", "select", "suggest"], + "description": "Phase. Defaults to select when `tool` is set, else search. Select the product actually chosen, even when it is not in the catalog. Suggest a capability gap only when no product was chosen." }, "category": { "type": "string", @@ -592,7 +595,9 @@ impl Tool for DiscoverToolsTool { }, "tool": { "type": "string", - "description": "Catalog tool name to set up when action=setup." + "minLength": 2, + "maxLength": 100, + "description": "For select: public name of the product actually chosen. Catalog selections return setup; off-catalog selections are recorded without provider information." }, "suggestion_kind": { "type": "string", @@ -762,12 +767,7 @@ impl Tool for DiscoverToolsTool { } }; - let tool_selection = params - .tool - .as_deref() - .map(str::trim) - .filter(|t| !t.is_empty()) - .map(str::to_ascii_lowercase); + let tool_selection = normalize_selection_name(params.tool.as_deref())?; let action = DiscoveryAction::parse(params.action.as_deref(), tool_selection.is_some())?; let discovery_request = DiscoveryRequestContext { client: &self.client, @@ -875,25 +875,30 @@ impl Tool for DiscoverToolsTool { return Err(err); } }; - crate::sponsors::provenance::record_discovered_setups(extract_mcp_setups_from( - fetched - .listing - .get("tool") - .map(std::slice::from_ref) - .unwrap_or(&[]), - )); + let catalog_tool = fetched.listing.get("tool").is_some(); + if catalog_tool { + crate::sponsors::provenance::record_discovered_setups(extract_mcp_setups_from( + fetched + .listing + .get("tool") + .map(std::slice::from_ref) + .unwrap_or(&[]), + )); + } let canonical_tool = fetched .listing .get("tool") .and_then(|tool| tool.get("name")) - .and_then(Value::as_str); + .and_then(Value::as_str) + .or_else(|| fetched.listing.get("selected_tool").and_then(Value::as_str)) + .unwrap_or(&tool_name); record_discovery_telemetry( &request_id, started_at, &endpoint, "select", Some(&category), - canonical_tool, + Some(canonical_tool), "success", None, Some(fetched.http_status), @@ -905,7 +910,9 @@ impl Tool for DiscoverToolsTool { return Ok(ToolOutput::new(rendered) .with_title(tool_name.to_string()) .with_metadata(json!({ - "sponsored_discovery": true, + "discovery_selection": true, + "sponsored_discovery": catalog_tool, + "catalog_tool": catalog_tool, "category": category, "selected_tool": tool_name, "disclosure_url": crate::sponsors::DISCOVERY_PARTNERS_URL, @@ -1274,6 +1281,37 @@ fn validate_suggestion_text( Ok(()) } +/// Normalize the public product name recorded by the select phase. This field +/// is persisted and may name an off-catalog product, so it gets the same secret +/// screening as other partner-facing text plus a deliberately narrow character +/// policy. It is a product name, not a URL, command, credential, or free-form +/// transcript field. +fn normalize_selection_name(value: Option<&str>) -> Result> { + let Some(value) = value.map(str::trim).filter(|value| !value.is_empty()) else { + return Ok(None); + }; + let chars = value.chars().count(); + if !(2..=100).contains(&chars) { + return Err(anyhow::anyhow!( + "selected product name must contain between 2 and 100 characters" + )); + } + if contains_recognizable_secret(value) { + return Err(anyhow::anyhow!( + "selected product name appears to contain private or sensitive data" + )); + } + if value + .chars() + .any(|ch| ch.is_control() || matches!(ch, '<' | '>' | '\\' | '`')) + { + return Err(anyhow::anyhow!( + "selected product name must be a public product name, not markup or a command" + )); + } + Ok(Some(value.to_ascii_lowercase())) +} + fn normalize_suggestion_url(value: Option<&str>) -> Result> { let Some(value) = value.map(str::trim).filter(|value| !value.is_empty()) else { return Ok(None); @@ -1358,7 +1396,7 @@ fn render_listing(category: &str, listing: &Value, request_id: &str) -> Result Result Result { - let tool = listing - .get("tool") - .ok_or_else(|| anyhow::anyhow!("discovery returned no tool entry for '{tool_name}'"))?; + let Some(tool) = listing.get("tool") else { + let selected_tool = listing + .get("selected_tool") + .and_then(Value::as_str) + .unwrap_or_default(); + if listing.get("listed").and_then(Value::as_bool) != Some(false) + || !selected_tool.eq_ignore_ascii_case(tool_name) + { + return Err(anyhow::anyhow!( + "discovery returned no selection receipt for '{tool_name}'" + )); + } + return Ok(format!( + "Selected off-catalog product '{selected_tool}' for '{category}'.\n\n\ + Selection recorded as demand data. Jcode does not list or partner with this \ + product, so no provider information, recommendation, or setup instructions \ + are provided. Continue using only information independently available to you." + )); + }; let name = tool .get("name") .and_then(|v| v.as_str()) .unwrap_or(tool_name); let blurb = tool.get("blurb").and_then(|v| v.as_str()).unwrap_or(""); let mut out = format!( - "Set up '{name}' from '{category}' (Jcode tool directory; the choice must be based only \ + "Selected '{name}' from '{category}' (Jcode tool directory; the choice must be based only \ on fit; details: {}):\n\n{name}: {blurb}", crate::sponsors::DISCOVERY_PARTNERS_URL ); @@ -1555,6 +1612,8 @@ mod tests { .unwrap(); assert!(out.contains("No integrations")); assert!(out.contains("Search request ID")); + assert!(out.contains("action `select`")); + assert!(out.contains("off-catalog")); assert!(out.contains("action `suggest`")); } @@ -1565,7 +1624,8 @@ mod tests { }); let out = render_listing("payments", &listing, "11111111-2222-4333-8444-555555555555").unwrap(); - assert!(out.contains("action `setup`")); + assert!(out.contains("action `select`")); + assert!(out.contains("off-catalog selection")); assert!(out.contains("action `suggest`")); assert!(out.contains("Search request ID")); } @@ -1581,13 +1641,43 @@ mod tests { } }); let out = render_selection("payments", "agentcard", &listing).unwrap(); - assert!(out.contains("Set up 'agentcard'")); + assert!(out.contains("Selected 'agentcard'")); assert!(out.contains("Setup: npm install -g agentcard")); assert!(out.contains("Jcode tool directory")); assert!(out.contains("the choice must be based only on fit")); assert!(render_selection("payments", "ghost", &json!({})).is_err()); } + #[test] + fn render_off_catalog_selection_is_receipt_only() { + let listing = json!({ + "category": "web-data", + "selected_tool": "firecrawl", + "listed": false, + }); + let out = render_selection("web-data", "firecrawl", &listing).unwrap(); + assert!(out.contains("Selected off-catalog product 'firecrawl'")); + assert!(out.contains("Selection recorded as demand data")); + assert!(out.contains("no provider information")); + assert!(out.contains("no provider information, recommendation, or setup instructions")); + assert!(!out.contains("http")); + assert!(render_selection("web-data", "other", &listing).is_err()); + } + + #[test] + fn selected_product_names_are_public_and_bounded() { + assert_eq!( + normalize_selection_name(Some(" Firecrawl ")).unwrap(), + Some("firecrawl".to_string()) + ); + assert_eq!(normalize_selection_name(None).unwrap(), None); + assert!(normalize_selection_name(Some("x")).is_err()); + assert!(normalize_selection_name(Some("")).is_err()); + assert!( + normalize_selection_name(Some("ghp_abcdefghijklmnopqrstuvwxyz1234567890")).is_err() + ); + } + #[test] fn agentmail_selection_preserves_signup_attribution_and_mcp_provenance() { let listing = json!({ @@ -1608,7 +1698,7 @@ mod tests { }); let rendered = render_selection("email-messaging", "agentmail", &listing).unwrap(); - assert!(rendered.contains("Set up 'agentmail'")); + assert!(rendered.contains("Selected 'agentmail'")); assert!(rendered.contains("\"source\":\"jcode\"")); assert!(rendered.contains("\"referrer\":\"https://jcode.sh/discovery-tools\"")); assert!(rendered.contains("agentmail-mcp@1.0.0")); @@ -1633,8 +1723,9 @@ mod tests { assert!(description.contains("don't already have a tool for")); assert!(description.contains("vetted integrations")); assert!(description.contains("partnered with this harness")); + assert!(description.contains("including an off-catalog product")); assert!( - description.len() < 300, + description.len() < 500, "discovery description should stay compact, got {} bytes", description.len() ); @@ -1660,6 +1751,11 @@ mod tests { assert!(schema.contains("known_product")); assert!(schema.contains("capability_gap")); assert!(schema.contains("prior_request_id")); + assert!(schema.contains("off-catalog selections are recorded")); + assert_eq!( + parameters["properties"]["action"]["enum"], + json!(["search", "select", "suggest"]) + ); assert!( schema.len() < 4_500, "discovery schema should stay compact, got {} bytes", @@ -1675,20 +1771,23 @@ mod tests { ); assert_eq!( DiscoveryAction::parse(None, true).unwrap(), - DiscoveryAction::Setup + DiscoveryAction::Select + ); + assert_eq!( + DiscoveryAction::parse(Some("select"), true).unwrap(), + DiscoveryAction::Select ); assert_eq!( DiscoveryAction::parse(Some("suggest"), false).unwrap(), DiscoveryAction::Suggest ); - assert!(DiscoveryAction::parse(Some("setup"), false).is_err()); + assert!(DiscoveryAction::parse(Some("select"), false).is_err()); assert!(DiscoveryAction::parse(Some("search"), true).is_err()); assert!(DiscoveryAction::parse(Some("suggest"), true).is_err()); } - /// The tool was renamed from discovery vocabulary to integration - /// vocabulary. Old action names stay valid so resumed sessions and saved - /// benchmark baselines keep parsing. + /// Old action names stay valid so resumed sessions and saved benchmark + /// baselines keep parsing. #[test] fn legacy_action_names_still_parse() { assert_eq!( @@ -1696,10 +1795,10 @@ mod tests { DiscoveryAction::Search ); assert_eq!( - DiscoveryAction::parse(Some("select"), true).unwrap(), - DiscoveryAction::Setup + DiscoveryAction::parse(Some("setup"), true).unwrap(), + DiscoveryAction::Select ); - assert!(DiscoveryAction::parse(Some("select"), false).is_err()); + assert!(DiscoveryAction::parse(Some("setup"), false).is_err()); assert!(DiscoveryAction::parse(Some("browse"), true).is_err()); } @@ -2104,6 +2203,65 @@ mod tests { } } + #[tokio::test] + async fn execute_records_off_catalog_selection_without_provider_information() { + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let temp = tempfile::tempdir().unwrap(); + crate::env::set_var("JCODE_HOME", temp.path()); + + let body = json!({ + "category": "web-data", + "selected_tool": "firecrawl", + "listed": false, + }) + .to_string(); + let (endpoint, server) = one_shot_server("HTTP/1.1 200 OK", body).await; + std::fs::write( + temp.path().join("config.toml"), + format!("[sponsors]\nenabled = true\nendpoint = \"{endpoint}\"\n"), + ) + .unwrap(); + crate::config::Config::invalidate_cache(); + + let output = DiscoverToolsTool::new() + .execute( + json!({ + "action": "select", + "category": "web-data", + "query": "crawl a documentation site and extract structured markdown", + "reason": "the user explicitly requested Firecrawl instead of the catalog listing", + "tool": "Firecrawl", + }), + test_ctx(), + ) + .await + .unwrap(); + + assert!( + output + .output + .contains("Selected off-catalog product 'firecrawl'") + ); + assert!(output.output.contains("no provider information")); + assert!(!output.output.contains("Setup:")); + let metadata = output.metadata.unwrap(); + assert_eq!(metadata["selected_tool"], "firecrawl"); + assert_eq!(metadata["catalog_tool"], false); + assert_eq!(metadata["sponsored_discovery"], false); + + let request = server.await.unwrap(); + assert!(request.starts_with("GET /?"), "{request}"); + assert!(request.contains("tool=firecrawl"), "{request}"); + + if let Some(prev) = prev_home { + crate::env::set_var("JCODE_HOME", prev); + } else { + crate::env::remove_var("JCODE_HOME"); + } + crate::config::Config::invalidate_cache(); + } + #[tokio::test] async fn execute_end_to_end_with_enabled_config_and_local_server() { let _guard = crate::storage::lock_test_env(); diff --git a/crates/jcode-app-core/src/tool/tests.rs b/crates/jcode-app-core/src/tool/tests.rs index c00f9b2fdb..0721cf8204 100644 --- a/crates/jcode-app-core/src/tool/tests.rs +++ b/crates/jcode-app-core/src/tool/tests.rs @@ -1525,14 +1525,14 @@ fn the_dialect_sweep_catches_the_issue_754_schema() { &jcode_schema_dialect::registry::GEMINI, ); assert!( - unnormalized.iter().any(|e| e.message.contains("propertyNames")), + unnormalized + .iter() + .any(|e| e.message.contains("propertyNames")), "the checker must flag the raw schema, got {unnormalized:?}" ); - let normalized = jcode_schema_dialect::dialect::apply( - &hostile, - &jcode_schema_dialect::registry::GEMINI, - ); + let normalized = + jcode_schema_dialect::dialect::apply(&hostile, &jcode_schema_dialect::registry::GEMINI); assert!( jcode_schema_dialect::must_not_contain_unsupported_constructs( &normalized, diff --git a/crates/jcode-base/src/gateway/control.rs b/crates/jcode-base/src/gateway/control.rs index 7081bfaec8..795c7c536f 100644 --- a/crates/jcode-base/src/gateway/control.rs +++ b/crates/jcode-base/src/gateway/control.rs @@ -13,6 +13,8 @@ use super::{DeviceRegistry, resolve_connect_host}; /// Parsed `/remote` invocation. #[derive(Debug, Clone, PartialEq, Eq)] pub enum RemoteCommand { + /// Activate or manage the subscription-backed Jcode Cloud host. + Cloud, /// Show gateway state, dial address, and paired devices. Status, /// Enable the gateway in config. @@ -39,11 +41,12 @@ pub fn parse_remote_command(input: &str) -> Option let mut parts = rest.split_whitespace(); let Some(sub) = parts.next() else { - return Some(Ok(RemoteCommand::Status)); + return Some(Ok(RemoteCommand::Cloud)); }; let command = match sub.to_ascii_lowercase().as_str() { - "status" => RemoteCommand::Status, + "cloud" | "setup" => RemoteCommand::Cloud, + "status" | "local" => RemoteCommand::Status, "on" | "enable" => RemoteCommand::On, "off" | "disable" => RemoteCommand::Off, "pair" => RemoteCommand::Pair, @@ -57,7 +60,7 @@ pub fn parse_remote_command(input: &str) -> Option } other => { return Some(Err(format!( - "Unknown /remote subcommand: {other}\nUsage: /remote [status|on|off|pair|revoke ]" + "Unknown /remote subcommand: {other}\nUsage: /remote [cloud|status|on|off|pair|revoke ]" ))); } }; @@ -65,7 +68,7 @@ pub fn parse_remote_command(input: &str) -> Option // Only `revoke` takes an argument. if !matches!(command, RemoteCommand::Revoke(_)) && parts.next().is_some() { return Some(Err(format!( - "/remote {sub} takes no arguments\nUsage: /remote [status|on|off|pair|revoke ]" + "/remote {sub} takes no arguments\nUsage: /remote [cloud|status|on|off|pair|revoke ]" ))); } @@ -304,14 +307,14 @@ mod tests { } #[test] - fn bare_remote_shows_status() { + fn bare_remote_starts_cloud_activation() { assert_eq!( parse_remote_command("/remote"), - Some(Ok(RemoteCommand::Status)) + Some(Ok(RemoteCommand::Cloud)) ); assert_eq!( parse_remote_command(" /remote "), - Some(Ok(RemoteCommand::Status)) + Some(Ok(RemoteCommand::Cloud)) ); } @@ -319,6 +322,9 @@ mod tests { fn subcommands_and_aliases_parse() { for (input, expected) in [ ("/remote status", RemoteCommand::Status), + ("/remote cloud", RemoteCommand::Cloud), + ("/remote setup", RemoteCommand::Cloud), + ("/remote local", RemoteCommand::Status), ("/remote on", RemoteCommand::On), ("/remote enable", RemoteCommand::On), ("/remote off", RemoteCommand::Off), diff --git a/crates/jcode-base/src/mcp/client.rs b/crates/jcode-base/src/mcp/client.rs index b14b24166f..3ea445052d 100644 --- a/crates/jcode-base/src/mcp/client.rs +++ b/crates/jcode-base/src/mcp/client.rs @@ -143,8 +143,12 @@ impl McpClient { name, config.command, config.args, working_dir )); - let mut env: HashMap = std::env::vars().collect(); - env.extend(config.env.clone()); + // Credentials must be opted into an MCP server explicitly through its + // config. The long-lived jcode daemon contains provider credentials in + // its process environment, and blindly inheriting them exposes those + // credentials to every configured MCP executable (issue #771). + let inherited: HashMap = std::env::vars().collect(); + let env = mcp_child_env(inherited, &config.env); let mut command = Command::new(&config.command); command @@ -364,6 +368,34 @@ impl McpClient { } } +/// Secrets that an MCP child must not receive merely because jcode has them. +/// +/// This intentionally applies only to inherited values. A server can still be +/// given any of these names through `McpServerConfig::env`. +fn is_sensitive_inherited_env_key(key: &str) -> bool { + let key = key.to_ascii_uppercase(); + key.ends_with("_API_KEY") + || key.ends_with("_ACCESS_TOKEN") + || key.ends_with("_AUTH_TOKEN") + || matches!( + key.as_str(), + "AWS_ACCESS_KEY_ID" + | "AWS_SECRET_ACCESS_KEY" + | "AWS_SESSION_TOKEN" + | "AZURE_CLIENT_SECRET" + | "GOOGLE_APPLICATION_CREDENTIALS" + ) +} + +fn mcp_child_env( + mut inherited: HashMap, + explicit: &HashMap, +) -> HashMap { + inherited.retain(|key, _| !is_sensitive_inherited_env_key(key)); + inherited.extend(explicit.clone()); + inherited +} + impl Drop for McpClient { fn drop(&mut self) { let _ = self.child.start_kill(); @@ -372,8 +404,45 @@ impl Drop for McpClient { #[cfg(all(test, unix))] mod tests { - use super::McpClient; + use super::{McpClient, is_sensitive_inherited_env_key, mcp_child_env}; use crate::mcp::protocol::McpServerConfig; + use std::collections::HashMap; + + #[test] + fn inherited_mcp_env_scrubs_provider_credentials() { + for key in [ + "ANTHROPIC_API_KEY", + "openai_api_key", + "CURSOR_ACCESS_TOKEN", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "GOOGLE_APPLICATION_CREDENTIALS", + ] { + assert!(is_sensitive_inherited_env_key(key), "must scrub {key}"); + } + for key in ["PATH", "HOME", "RUST_LOG", "JCODE_OPENROUTER_API_KEY_NAME"] { + assert!(!is_sensitive_inherited_env_key(key), "must preserve {key}"); + } + } + + #[test] + fn explicit_mcp_env_can_opt_a_credential_back_in() { + let inherited = HashMap::from([ + ("PATH".to_string(), "/bin".to_string()), + ("ANTHROPIC_API_KEY".to_string(), "daemon-secret".to_string()), + ]); + let explicit = HashMap::from([( + "ANTHROPIC_API_KEY".to_string(), + "server-specific-secret".to_string(), + )]); + + let env = mcp_child_env(inherited, &explicit); + assert_eq!(env.get("PATH").map(String::as_str), Some("/bin")); + assert_eq!( + env.get("ANTHROPIC_API_KEY").map(String::as_str), + Some("server-specific-secret") + ); + } /// A minimal fake stdio MCP server (shell script) that reports its own /// process cwd as the serverInfo name. diff --git a/crates/jcode-build-support/src/paths.rs b/crates/jcode-build-support/src/paths.rs index d77b4e27e7..af9b7c78e6 100644 --- a/crates/jcode-build-support/src/paths.rs +++ b/crates/jcode-build-support/src/paths.rs @@ -177,9 +177,20 @@ pub fn selfdev_build_command_for_target( }; let specs = match target { SelfDevBuildTarget::Tui => vec![("jcode", "jcode")], - SelfDevBuildTarget::Desktop2 => vec![("jcode-desktop2", "jcode-desktop2")], + // desktop2 launches the harness API bridge as a sibling executable. + // Building only the app leaves a fresh target directory unable to + // start its runtime because the bridge is neither beside it nor on + // PATH. + SelfDevBuildTarget::Desktop2 => vec![ + ("jcode-desktop2", "jcode-desktop2"), + ("jcode-harness-api-server", "jcode-harness-api-bridge"), + ], SelfDevBuildTarget::All | SelfDevBuildTarget::Auto => { - vec![("jcode", "jcode"), ("jcode-desktop2", "jcode-desktop2")] + vec![ + ("jcode", "jcode"), + ("jcode-desktop2", "jcode-desktop2"), + ("jcode-harness-api-server", "jcode-harness-api-bridge"), + ] } }; let wrapper = repo_dir.join("scripts").join("dev_cargo.sh"); @@ -637,10 +648,17 @@ mod tests { let repo = repo_fixture(false); let cases = [ (SelfDevBuildTarget::Tui, vec!["-p jcode "]), - (SelfDevBuildTarget::Desktop2, vec!["-p jcode-desktop2 "]), + ( + SelfDevBuildTarget::Desktop2, + vec!["-p jcode-desktop2 ", "--bin jcode-harness-api-bridge"], + ), ( SelfDevBuildTarget::All, - vec!["-p jcode ", "-p jcode-desktop2 "], + vec![ + "-p jcode ", + "-p jcode-desktop2 ", + "--bin jcode-harness-api-bridge", + ], ), ]; for (target, expected) in cases { diff --git a/crates/jcode-desktop2/scripts/reapply_image_paste.py b/crates/jcode-desktop2/scripts/reapply_image_paste.py new file mode 100644 index 0000000000..9d3e8acb5a --- /dev/null +++ b/crates/jcode-desktop2/scripts/reapply_image_paste.py @@ -0,0 +1,404 @@ +#!/usr/bin/env python3 +"""Idempotently (re)apply desktop2 clipboard-image paste wiring. + +Concurrent agents keep rewriting harness.rs/main.rs wholesale, which reverts +small cross-cutting edits. Re-running this restores them whatever shape the +surrounding file is currently in. +""" +import pathlib +import re +import sys + +root = pathlib.Path(__file__).resolve().parents[1] / "src" +changed = [] + + +def edit(name, fn): + path = root / name + before = path.read_text() + after = fn(before) + if after != before: + path.write_text(after) + changed.append(name) + + +def main_rs(s): + if "mod clipboard_image;" not in s: + s = s.replace("mod clipboard;\n", "mod clipboard;\nmod clipboard_image;\n", 1) + if "mod png;" not in s: + s = s.replace("mod place;\n", "mod place;\nmod png;\n", 1) + if "pending_images" not in s: + s = s.replace( + " clipboard: clipboard::Clipboard,\n", + """ clipboard: clipboard::Clipboard, + /// Images pasted into the composer, waiting for the next submission. + /// + /// Held on `App` rather than in the editor because an attachment is not + /// text: it has no place in the buffer, it must survive editing the message + /// written around it, and it is cleared by sending rather than by deleting + /// a character. + pending_images: Vec<(String, String)>, + /// Attachments belonging to messages typed mid-turn and waiting in the + /// transcript's queue: one entry per queued card, in the same order, so a + /// message is sent with the images it was written with rather than with + /// whatever happens to be pending when its turn comes. + queued_images: std::collections::VecDeque>, +""", + 1, + ) + s = s.replace( + " clipboard: clipboard::Clipboard::default(),\n", + """ clipboard: clipboard::Clipboard::default(), + pending_images: Vec::new(), + queued_images: std::collections::VecDeque::new(), +""", + 1, + ) + if "pub attachments: usize," not in s: + s = s.replace( + """ /// Transient one-line notice (e.g. "nothing to undo"). + pub notice: Option,""", + """ /// Transient one-line notice (e.g. "nothing to undo"). + pub notice: Option, + /// How many images are attached to the message being written. + /// + /// A count on the model rather than the payload: a frame is a pure function + /// of the model, so the composer can say "1 image attached" in a capture + /// and in a test without carrying megabytes of base64 through the layout. + /// The bytes live on `App`, beside the connection that sends them. + pub attachments: usize,""", + 1, + ) + s = s.replace(" notice: None,", " notice: None,\n attachments: 0,", 1) + if "images attached" not in s: + s = s.replace( + """ if self.scroll > 0.0 { + return Some("scrolled back".to_string()); + }""", + """ // Attachments outlive the paste notice: a notice fades, and an image + // silently attached to a message still being typed is the one thing + // that must not go invisible before it is sent. + if self.attachments > 0 { + return Some(match self.attachments { + 1 => "1 image attached".to_string(), + count => format!("{count} images attached"), + }); + } + if self.scroll > 0.0 { + return Some("scrolled back".to_string()); + }""", + 1, + ) + old = """ if self.model.editor.text().trim().is_empty() { + return; + } + if self.model.session_id.is_none() { + self.model.set_notice("not attached yet"); + return; + } + let content = self.model.editor.take_for_submit();""" + if old in s: + s = s.replace( + old, + """ // An attachment is a message: sending a screenshot with no words is a + // normal thing to do, so the composer is only empty when there is + // nothing pending either. + if self.model.editor.text().trim().is_empty() && self.pending_images.is_empty() { + return; + } + if self.model.session_id.is_none() { + self.model.set_notice("not attached yet"); + return; + } + let mut content = self.model.editor.take_for_submit(); + let images = std::mem::take(&mut self.pending_images); + self.model.attachments = 0; + // The transcript card needs something to draw and the daemon needs + // non-empty content, so an image sent on its own says so rather than + // appearing as a blank card indistinguishable from a glitch. + if content.trim().is_empty() { + content = "[image]".to_string(); + }""", + 1, + ) + if "self.queued_images.push_back" not in s: + s = s.replace( + """ if queued { + // The turn is still streaming""", + """ if queued { + // The attachments wait with their card rather than with the app: the + // next thing typed gets a fresh set, and this message keeps the + // images it was written with. + self.queued_images.push_back(images); + // The turn is still streaming""", + 1, + ) + if "self.queued_images.pop_front" not in s: + s = s.replace( + """ let Some(content) = self.model.transcript.promote_oldest_queued() else { + return; + }; + self.model.busy = true; + self.model.activity.start(std::time::Instant::now());""", + """ let Some(content) = self.model.transcript.promote_oldest_queued() else { + return; + }; + self.model.busy = true; + self.model.activity.start(std::time::Instant::now()); + // Oldest first, matching the card being promoted: the queue and this + // deque are pushed in the same order, so the front is this message's. + let images = self.queued_images.pop_front().unwrap_or_default();""", + 1, + ) + old_paste = """ Action::Paste => match self.clipboard.get() { + Some(text) => self.model.editor.insert_str(&text), + None => self.model.set_notice("clipboard is empty"), + },""" + if old_paste in s: + s = s.replace( + old_paste, + """ // An image on the clipboard outranks text, because a copied image + // usually also publishes a text flavour (a file URI, or the HTML it + // came from), and pasting that instead is exactly the bug that made + // image pasting look broken. + Action::Paste => match self.clipboard.get_image() { + Ok(Some(image)) => { + let label = image.label(); + self.pending_images + .push((image.media_type, crate::png::base64(&image.bytes))); + self.model.attachments = self.pending_images.len(); + // Said out loud because an attachment is invisible in the + // composer text: without this a paste looks like nothing + // happened, and a second looks like it replaced the first. + self.model.set_notice(match self.pending_images.len() { + 1 => format!("image attached ({label})"), + count => format!("image attached ({label}), {count} total"), + }); + } + Ok(None) => match self.clipboard.get() { + Some(text) => self.model.editor.insert_str(&text), + None => self.model.set_notice("clipboard is empty"), + }, + // The clipboard existed but refused: say so rather than paste + // nothing, which is indistinguishable from the key being + // ignored. + Err(error) => self + .model + .set_notice(format!("clipboard image unavailable: {error}")), + },""", + 1, + ) + s = s.replace("harness::Command::Send(content)", "harness::Command::Send { content, images }") + return s + + +def clipboard_rs(s): + if "from_wayland" in s: + return s + anchor = "/// Which system buffer an operation refers to." + s = s.replace( + anchor, + """/// An image read from the clipboard, ready to be sent. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Image { + /// Pixel size, when the container declares one. `None` rather than a guess: + /// the size exists only to tell the user what they attached, and a wrong + /// number would be worse than saying nothing. + pub width: Option, + pub height: Option, + /// IANA media type of [`Self::bytes`]. Not always PNG: a JPEG on the + /// clipboard is forwarded as a JPEG rather than re-encoded, because those + /// bytes are already smaller than anything this app would produce. + pub media_type: String, + /// The encoded image, exactly as it will be sent. + pub bytes: Vec, +} + +impl Image { + /// Short description for the caption that tells the user what they + /// attached: the pixel size when it is known, else the kind. + pub fn label(&self) -> String { + match (self.width, self.height) { + (Some(width), Some(height)) => format!("{width}x{height}"), + _ => self + .media_type + .strip_prefix("image/") + .unwrap_or("image") + .to_string(), + } + } +} + +""" + + anchor, + 1, + ) + get = """ /// Read the clipboard, preferring the system and falling back to the last + /// value set in-process.""" + s = s.replace( + get, + """ /// Read an image from the ordinary clipboard. + /// + /// The compositor is asked first (see [`crate::clipboard_image`]) because it + /// hands back the source's own encoded bytes; arboard only offers raw RGBA, + /// which would have to be re-encoded. `Ok(None)` means "no image on the + /// clipboard", the ordinary case for a text paste, and must not be reported + /// to the user as a failure. + pub fn get_image(&mut self) -> Result, Unavailable> { + if self.system && let Some(image) = crate::clipboard_image::from_wayland() { + let (width, height) = match image.dimensions() { + Some((width, height)) => (Some(width), Some(height)), + None => (None, None), + }; + return Ok(Some(Image { + width, + height, + media_type: image.media_type, + bytes: image.bytes, + })); + } + let Some(backend) = self.backend() else { + return Ok(None); + }; + match backend.get_image() { + Ok(image) => { + let width = image.width as u32; + let height = image.height as u32; + let bytes = crate::png::encode_rgba(width, height, image.bytes.as_ref()); + Ok(Some(Image { + width: Some(width), + height: Some(height), + media_type: "image/png".to_string(), + bytes, + })) + } + // Nothing image-shaped on the clipboard: an absence, not an error. + Err(arboard::Error::ContentNotAvailable) => Ok(None), + Err(error) => Err(Unavailable(error.to_string())), + } + } + +""" + + get, + 1, + ) + return s + + +def capture_rs(s): + if "/// Minimal PNG writer" not in s: + return s + return s[: s.index("/// Minimal PNG writer")] + """/// Write tight RGBA8 pixels out as a PNG. Encoding itself lives in `png`, so +/// the clipboard path can produce the same bytes without a file. +fn write_png(path: &std::path::Path, width: u32, height: u32, rgba: &[u8]) -> Result<()> { + std::fs::write(path, crate::png::encode_rgba(width, height, rgba))?; + Ok(()) +} +""" + + +def harness_rs(s): + if "Send(String)" in s: + s = s.replace( + " Send(String),", + """ /// A user message with any images attached to it. The images travel with + /// the text rather than as a command of their own, so a message and its + /// attachments can never be split across a reconnect. + Send { + content: String, + images: Vec<(String, String)>, + },""", + 1, + ) + s = s.replace("Command::Send(content) => {", "Command::Send { content, images } => {", 1) + # Two shapes exist depending on which agent last rewrote the worker. + s = s.replace( + """ content, + images: vec![],""", + """ content, + images,""", + 1, + ) + s = s.replace( + "client.send_message(&session, &content, vec![], None)", + "client.send_message(&session, &content, images, None)", + 1, + ) + return s + + +CLIPBOARD_IMAGE_PROBE = '''/// `--check-clipboard-image`: prove Ctrl+V's image path against the *real* +/// compositor. +/// +/// The unit tests keep the system clipboard sandboxed so they cannot read or +/// clobber a developer's clipboard, which means nothing in the suite exercises +/// Wayland image negotiation at all. That is exactly where pasting a screenshot +/// breaks without a single test failing, so this reads whatever image is on the +/// clipboard right now and reports its type, size, and payload cost. +fn check_clipboard_image() -> Result<()> { + let mut clipboard = crate::clipboard::Clipboard::system(); + let image = clipboard + .get_image() + .map_err(|error| anyhow::anyhow!("clipboard image unavailable: {error}"))? + .ok_or_else(|| anyhow::anyhow!("clipboard does not contain an image"))?; + println!( + "clipboard image ok: {} {}, {} bytes, {} base64 chars", + image.media_type, + image.label(), + image.bytes.len(), + crate::png::base64(&image.bytes).len() + ); + Ok(()) +} + +''' + +DISPATCH = ' Some("--check-primary-selection") => Some(check_primary_selection()),' + + +def cli_rs(s): + if "check_clipboard_image" not in s and DISPATCH in s: + s = s.replace( + DISPATCH, + ' Some("--check-clipboard-image") => Some(check_clipboard_image()),\n' + DISPATCH, + 1, + ) + marker = "/// `--check-primary-selection`: prove auto-copy" + s = s.replace(marker, CLIPBOARD_IMAGE_PROBE + marker, 1) + return s.replace( + "harness::Command::Send(message.to_string())", + """harness::Command::Send { + content: message.to_string(), + images: vec![], + }""", + ) + + +def delivery_tests(s): + """The queue tests match on the command; the variant gained a field.""" + s = s.replace("Ok(harness::Command::Send(_))", "Ok(harness::Command::Send { .. })") + return s.replace( + "Ok(harness::Command::Send(content))", + "Ok(harness::Command::Send { content, .. })", + ) + + +def states_rs(s): + if "attachments" in s: + return s + return re.sub( + r"(\n(\s+)notice: [^\n]*,\n)", + lambda m: m.group(1) + m.group(2) + "attachments: 0,\n", + s, + ) + + +edit("main.rs", main_rs) +edit("clipboard.rs", clipboard_rs) +edit("capture.rs", capture_rs) +edit("harness.rs", harness_rs) +edit("cli.rs", cli_rs) +edit("states.rs", states_rs) +edit("tests/delivery.rs", delivery_tests) +print("reapplied:", ", ".join(changed) if changed else "nothing (already applied)") +sys.exit(0) diff --git a/crates/jcode-desktop2/src/app_workspace.rs b/crates/jcode-desktop2/src/app_workspace.rs index 760cb48ab9..df3cbcaef9 100644 --- a/crates/jcode-desktop2/src/app_workspace.rs +++ b/crates/jcode-desktop2/src/app_workspace.rs @@ -1,14 +1,32 @@ -//! App-side clock and coordinate bridge for the horizontal workspace. +//! App-side clock and coordinate bridge for the niri-style workspace. use crate::{App, workspace}; impl App { + /// Start a horizontal camera slide toward the session the strip just + /// focused. pub(crate) fn begin_workspace_transition(&mut self, direction: workspace::Direction) { self.model.workspace.begin(direction); self.workspace_frame = Some(std::time::Instant::now()); self.request_redraw(); } + /// Start a vertical row slide. `prev_row` and `prev_focused` describe the + /// group being left, captured *before* the strip moved, so the departing + /// workspace exits exactly as it stood. + pub(crate) fn begin_row_transition( + &mut self, + direction: workspace::Direction, + prev_row: Vec, + prev_focused: usize, + ) { + self.model + .workspace + .begin_row_change(direction, prev_row, prev_focused); + self.workspace_frame = Some(std::time::Instant::now()); + self.request_redraw(); + } + pub(crate) fn tick_workspace(&mut self, now: std::time::Instant) { if !self.model.workspace.is_animating() { self.workspace_frame = None; @@ -27,20 +45,29 @@ impl App { } } - /// Size used to lay out the focused model. The GPU surface stays full-window; - /// only the child scene uses this narrower native-scale page. + /// Size used to lay out the focused model. The GPU surface stays + /// full-window; only the child scene uses this narrower native-scale page. pub(crate) fn workspace_render_size(&self, viewport: (u32, u32)) -> (u32, u32) { if !self.workspace_active() { return viewport; } + let row_len = self + .model + .strip + .focused_group() + .map(|group| group.entries.len()) + .unwrap_or(1); + let scale = self.effective_scale(); + let inset = (workspace::VERTICAL_INSET * scale * 2.0).round() as u32; ( - workspace::column_width(viewport.0, self.model.strip.len()), - viewport.1, + workspace::column_width(viewport.0, row_len), + viewport.1.saturating_sub(inset).max(1), ) } - /// Convert the window-space pointer to focused-column coordinates. Inactive - /// columns therefore never reach any of the existing hit-testing paths. + /// Convert the window-space pointer to focused-column coordinates. + /// Inactive columns therefore never reach any of the existing hit-testing + /// paths. pub(crate) fn focused_pointer(&self) -> (f64, f64) { let Some(state) = self.state.as_ref() else { // Unit tests construct the app without a surface and set pointer @@ -52,33 +79,55 @@ impl App { } let size = state.size(); let scale = self.effective_scale(); - let entries = self.model.strip.entries(); - let focused_index = entries - .iter() - .position(|entry| Some(entry.session_id.as_str()) == self.model.session_id.as_deref()) - .or_else(|| { - let focused = self.model.strip.focused_session()?; - entries.iter().position(|entry| entry.session_id == focused) - }) - .unwrap_or(0); - let width = workspace::column_width(size.0, entries.len()); - let origin = self - .model - .workspace - .layout( - entries.len(), - focused_index, - f64::from(size.0), - f64::from(width), - workspace::GAP * scale, + let origin = workspace::placement( + &self.model.strip, + &self.model.workspace, + self.model.session_id.as_deref(), + (f64::from(size.0), f64::from(size.1)), + workspace::GAP * scale, + ) + .into_iter() + .find(|column| column.focused) + .map_or((0.0, 0.0), |column| { + ( + column.x / scale, + (column.y + workspace::VERTICAL_INSET * scale) / scale, ) - .into_iter() - .find(|column| column.focused) - .map_or(0.0, |column| column.x / scale); - (self.pointer.0 - origin, self.pointer.1) + }); + (self.pointer.0 - origin.0, self.pointer.1 - origin.1) + } + + /// The focused group's session ids and focused position, for capturing a + /// row about to be left by a vertical slide. + pub(crate) fn focused_row_snapshot(&self) -> (Vec, usize) { + let ids = self + .model + .strip + .focused_group() + .map(|group| { + group + .entries + .iter() + .map(|entry| entry.session_id.clone()) + .collect() + }) + .unwrap_or_default(); + (ids, self.model.strip.index()) } - fn workspace_active(&self) -> bool { - self.model.strip.len() > 1 && !self.model.overview.is_visible() + /// Whether the workspace chrome (gutters, page rings, camera) is in play: + /// the focused row has neighbors, or a slide is still running. + pub(crate) fn workspace_active(&self) -> bool { + if self.model.overview.is_visible() { + return false; + } + if self.model.workspace.is_animating() { + return true; + } + self.model + .strip + .focused_group() + .map(|group| group.entries.len() > 1) + .unwrap_or(false) } } diff --git a/crates/jcode-desktop2/src/cli.rs b/crates/jcode-desktop2/src/cli.rs index 6ca1ed93db..432aa2afbe 100644 --- a/crates/jcode-desktop2/src/cli.rs +++ b/crates/jcode-desktop2/src/cli.rs @@ -152,11 +152,11 @@ fn run_profile_states(args: &[String]) -> Result<()> { /// /// jcode-desktop2 --script 'type:alpha beta' ctrl+a shift+right shift+right /// -/// The gesture verbs drive the same handlers the window does, so the held-Super -/// overview is checkable without a compositor: +/// The gesture verbs drive the same handlers the window does. Super+hjkl is +/// direct niri-style motion by default; the held-Super overview stays behind +/// its bench flag: /// -/// jcode-desktop2 --script 'sessions:a=jcode,b=jcode,c=site' super-down \ -/// 'settle' super+h super-up +/// jcode-desktop2 --script 'sessions:a=jcode,b=jcode,c=site' super+l super+j fn run_script(steps: &[String]) -> Result<()> { let mut app = App::default(); app.model.session_id = Some("session_script".into()); diff --git a/crates/jcode-desktop2/src/main.rs b/crates/jcode-desktop2/src/main.rs index 963415b9c4..9f8ea4bd9c 100644 --- a/crates/jcode-desktop2/src/main.rs +++ b/crates/jcode-desktop2/src/main.rs @@ -118,10 +118,12 @@ struct App { /// cancel it, so a synthetic lift is invisible and a real one still /// resolves a frame or two later. pending_super_release: Option<(std::time::Instant, bool)>, - /// Whether holding Super opens the card-strip overview. On by default: - /// the compositor muscle memory this app lives inside (niri, GNOME) puts - /// "zoom out to everything" on the Super key. The flag stays so the - /// gesture can be benched again as a flip rather than a revert. + /// Whether holding Super opens the card-strip overview. Benched: the + /// workspace now moves like niri itself, so Super+hjkl slides the camera + /// between live pages directly and a zoomed-out field of thumbnails is a + /// second spatial model fighting the first. The machinery stays behind + /// this flag (and the sessions icon) so it can return as a flip rather + /// than a revert if the direct motion proves insufficient. super_overview: bool, /// Finished session-store scans, from the picker's worker thread. /// @@ -194,7 +196,7 @@ impl Default for App { modifiers: winit::keyboard::ModifiersState::empty(), super_held_since: None, pending_super_release: None, - super_overview: true, + super_overview: false, resume_scans: Some(std::sync::mpsc::channel()), clipboard: clipboard::Clipboard::default(), pending_images: Vec::new(), @@ -1526,14 +1528,23 @@ impl App { self.request_peek(); } } + // Vertical motion is a workspace switch: the whole row slides + // off and the next one in, so the departing row is captured + // before the strip moves or it could not be drawn leaving. Action::SessionUp => { + let (prev_row, prev_focused) = self.focused_row_snapshot(); if self.model.strip.focus_up() { + self.begin_row_transition(workspace::Direction::Up, prev_row, prev_focused); self.attach_focused_session(); + self.request_peek(); } } Action::SessionDown => { + let (prev_row, prev_focused) = self.focused_row_snapshot(); if self.model.strip.focus_down() { + self.begin_row_transition(workspace::Direction::Down, prev_row, prev_focused); self.attach_focused_session(); + self.request_peek(); } } diff --git a/crates/jcode-desktop2/src/math.rs b/crates/jcode-desktop2/src/math.rs index 5a64725b2f..42dcd997d4 100644 --- a/crates/jcode-desktop2/src/math.rs +++ b/crates/jcode-desktop2/src/math.rs @@ -85,6 +85,22 @@ impl MathSystem { } } + /// Typeset math that participates in a prose line. Unlike display math it + /// keeps operator limits beside operators and stays at the body font size. + pub fn typeset_inline(&self, source: &str, font_size: f64) -> Formula { + let boxed = MathLayoutEngine::stix().layout_inline(source, font_size); + let width = boxed.width; + let height = boxed.ascent + boxed.descent; + Formula { + lines: vec![PlacedLine { + baseline: boxed.ascent, + boxed, + }], + width, + height, + } + } + /// Draw a typeset formula with its top-left corner at `origin`, in logical /// units. /// diff --git a/crates/jcode-desktop2/src/scene.rs b/crates/jcode-desktop2/src/scene.rs index 64c39b9046..a0adbb1830 100644 --- a/crates/jcode-desktop2/src/scene.rs +++ b/crates/jcode-desktop2/src/scene.rs @@ -1388,6 +1388,30 @@ fn draw_transcript( scale, revealed, ); + // Parley owns wrapping and baseline geometry for inline math. + // Its inline boxes reserve the exact formula dimensions; draw + // the corresponding native OpenType-MATH formula into each box. + for line in block.layout.lines() { + for item in line.items() { + let parley::PositionedLayoutItem::InlineBox(boxed) = item else { + continue; + }; + let Some(formula) = block.inline_math.get(boxed.id as usize) else { + continue; + }; + crate::math::shared().draw( + scene, + formula, + ( + text_left + inset_x + f64::from(boxed.x) / scale, + block_top + inset_y + f64::from(boxed.y) / scale, + ), + theme.text, + scale, + f64::INFINITY, + ); + } + } } drawn_glyphs += block.glyphs; } diff --git a/crates/jcode-desktop2/src/scene_workspace.rs b/crates/jcode-desktop2/src/scene_workspace.rs index 588057728d..1c55164643 100644 --- a/crates/jcode-desktop2/src/scene_workspace.rs +++ b/crates/jcode-desktop2/src/scene_workspace.rs @@ -4,15 +4,26 @@ //! remains the only interactive page. Other columns use the same builder with a //! read-only model made from their cached `Peek` transcript. All pages are then //! appended into one Vello scene at the camera positions supplied by -//! [`crate::workspace`]. +//! [`crate::workspace`], each clipped to a rounded window with its own border +//! ring, so where one session ends and the next begins is legible at a glance. use crate::{Model, paint, scene, strip, workspace}; use vello::Scene; -use vello::kurbo::{Affine, Rect}; +use vello::kurbo::{Affine, Rect, RoundedRect, Stroke}; -/// Build the actual window scene. The overview deliberately keeps the legacy -/// full-window path: it is already a view of every session, and nesting that -/// spatial navigator inside one workspace column would make both modes worse. +/// Corner radius of a session page, in logical units. Soft enough to read as +/// a window, square enough that the transcript inside does not lose its +/// margins to the curve. +const PAGE_CORNER: f64 = 10.0; +/// Ring weight around an unfocused page, and around the focused one. The +/// focused ring is the compositor's focus border: it is the whole signal for +/// "this is the page your keys go to", so it is unmistakably heavier. +const PAGE_RING: f64 = 1.0; +const PAGE_RING_FOCUS: f64 = 2.0; + +/// Build the actual window scene. The overview keeps the legacy full-window +/// path: it is already a view of every session, and nesting that spatial +/// navigator inside one workspace column would make both modes worse. pub fn build_workspace_scene( output: &mut Scene, painter: &mut paint::Painter, @@ -20,28 +31,25 @@ pub fn build_workspace_scene( size: (u32, u32), scale: f64, ) { - let entries = model.strip.entries(); - if entries.len() <= 1 || model.overview.is_visible() { + if model.overview.is_visible() { scene::build_scene(output, painter, model, size, scale); return; } - - let column_width = workspace::column_width(size.0, entries.len()); - let focused_index = entries - .iter() - .position(|entry| Some(entry.session_id.as_str()) == model.session_id.as_deref()) - .or_else(|| { - let focused = model.strip.focused_session()?; - entries.iter().position(|entry| entry.session_id == focused) - }) - .unwrap_or(0); - let columns = model.workspace.layout( - entries.len(), - focused_index, - f64::from(size.0), - f64::from(column_width), + let entries = model.strip.entries(); + let columns = workspace::placement( + &model.strip, + &model.workspace, + model.session_id.as_deref(), + (f64::from(size.0), f64::from(size.1)), workspace::GAP * scale, ); + // A lone page at rest is the legacy full-window layout: chrome around a + // window with no neighbors would be a picture frame on a wall with one + // painting. + if columns.len() <= 1 && !model.workspace.is_animating() { + scene::build_scene(output, painter, model, size, scale); + return; + } // The gutters belong to the workspace, not to any session. A quiet wash // makes the page boundaries legible without adding permanent chrome. @@ -53,29 +61,70 @@ pub fn build_workspace_scene( &Rect::new(0.0, 0.0, f64::from(size.0), f64::from(size.1)), ); - // Neighbors first, then the live page. They normally do not overlap, but - // this ordering keeps subpixel edge antialiasing from washing over focus. - for column in columns + let inset = workspace::VERTICAL_INSET * scale; + let page_height = (f64::from(size.1) - inset * 2.0).max(1.0) as u32; + let viewport = (f64::from(size.0), f64::from(size.1)); + + // Neighbors first, then the live page, so the focused ring is never + // washed over by a neighbor's edge antialiasing. + let mut ordered: Vec<&workspace::Column> = columns .iter() - .copied() - .filter(|column| !column.focused && column.is_visible(f64::from(size.0))) - { + .filter(|column| column.is_visible(viewport)) + .collect(); + ordered.sort_by_key(|column| column.focused); + for column in ordered { + let width = column.width.round().max(1.0) as u32; let mut child = Scene::new(); - let retained = retained_session_model(model, &entries[column.index]); - scene::build_scene( - &mut child, - painter, - &retained, - (column_width, size.1), - scale, + // The focused column is the live page: placement anchors focus to the + // attached session, falling back to the strip's focus during the + // frames before an attach resolves, exactly when the live model's + // "attaching" status is the honest thing to show. + if column.focused { + scene::build_scene(&mut child, painter, model, (width, page_height), scale); + } else { + let Some(entry) = entries.get(column.index) else { + continue; + }; + let retained = retained_session_model(model, entry); + scene::build_scene(&mut child, painter, &retained, (width, page_height), scale); + } + + let page = RoundedRect::new( + column.x, + inset + column.y, + column.x + column.width, + inset + column.y + f64::from(page_height), + PAGE_CORNER * scale, ); - output.append(&child, Some(Affine::translate((column.x, 0.0)))); - } + // The page is clipped to its rounded window so nothing it draws (a + // wide code block, a selection band) can bleed into the gutter or + // onto a neighbor: the boundary is a wall, not a suggestion. + output.push_layer( + vello::peniko::Fill::NonZero, + vello::peniko::Mix::Normal, + 1.0, + Affine::IDENTITY, + &page, + ); + output.append( + &child, + Some(Affine::translate((column.x, inset + column.y))), + ); + output.pop_layer(); - if let Some(column) = columns.iter().find(|column| column.focused) { - let mut child = Scene::new(); - scene::build_scene(&mut child, painter, model, (column_width, size.1), scale); - output.append(&child, Some(Affine::translate((column.x, 0.0)))); + // The ring sits outside the clip so it stays crisp at every corner. + let (color, weight) = if column.focused { + (model.theme.field_border_focus, PAGE_RING_FOCUS) + } else { + (model.theme.rule, PAGE_RING) + }; + output.stroke( + &Stroke::new(weight * scale), + Affine::IDENTITY, + color, + None, + &page, + ); } } @@ -210,7 +259,10 @@ mod tests { let entry = strip::Entry::new("neighbor", Some("/work/neighbor")); source.session_id = Some("live".into()); source.strip = strip::Strip::build( - vec![strip::Entry::new("live", Some("/work/live")), entry.clone()], + vec![ + strip::Entry::new("live", Some("/work/neighbor")), + entry.clone(), + ], Some("live"), ); source @@ -229,78 +281,29 @@ mod tests { build_workspace_scene(&mut output, &mut painter, &source, (1000, 720), 1.0); } + /// A vertical row slide draws both rows without panicking, including the + /// departing sessions that only exist as peeks. #[test] - #[ignore = "requires a GPU-backed capture"] - fn centered_retained_column_matches_the_full_session_scene_within_raster_tolerance() { - const VIEW_WIDTH: u32 = 1000; - const COLUMN_WIDTH: u32 = 760; - const HEIGHT: u32 = 720; - const COLUMN_X: u32 = (VIEW_WIDTH - COLUMN_WIDTH) / 2; - + fn a_row_slide_builds_with_both_rows() { let mut source = Model::default(); - let entry = strip::Entry::new("neighbor", Some("/work/neighbor")); - source.theme = crate::theme::Theme::print_light(); - source.session_id = Some("live".into()); + source.session_id = Some("b1".into()); source.strip = strip::Strip::build( - vec![strip::Entry::new("live", Some("/work/live")), entry.clone()], - Some("live"), - ); - source.peeks.insert( - "neighbor", - transcript([ - Message::user("question with **formatting**"), - Message::assistant("answer with `code` and\n\nmultiple paragraphs"), - ]), - ); - // At phase zero the page being left is centered. With two columns and - // rightward motion, that is the retained neighbor at x = 120. - source.workspace.begin(workspace::Direction::Right); - - let mut workspace_scene = Scene::new(); - build_workspace_scene( - &mut workspace_scene, - &mut paint::Painter::default(), - &source, - (VIEW_WIDTH, HEIGHT), - 1.0, + vec![ + strip::Entry::new("a1", Some("/w/jcode")), + strip::Entry::new("a2", Some("/w/jcode")), + strip::Entry::new("b1", Some("/w/site")), + ], + Some("b1"), ); - let workspace_pixels = - crate::capture::capture_scene_to_rgba(&workspace_scene, VIEW_WIDTH, HEIGHT) - .expect("capture workspace scene"); - - let retained = retained_session_model(&source, &entry); - let mut retained_scene = Scene::new(); - scene::build_scene( - &mut retained_scene, - &mut paint::Painter::default(), - &retained, - (COLUMN_WIDTH, HEIGHT), - 1.0, + source.peeks.insert("a1", transcript([Message::user("q")])); + source.workspace.begin_row_change( + workspace::Direction::Down, + vec!["a1".into(), "a2".into()], + 0, ); - let retained_pixels = - crate::capture::capture_scene_to_rgba(&retained_scene, COLUMN_WIDTH, HEIGHT) - .expect("capture retained scene"); - let mut max_channel_delta = 0; - for y in 0..HEIGHT as usize { - let workspace_start = (y * VIEW_WIDTH as usize + COLUMN_X as usize) * 4; - let workspace_end = workspace_start + COLUMN_WIDTH as usize * 4; - let retained_start = y * COLUMN_WIDTH as usize * 4; - let retained_end = retained_start + COLUMN_WIDTH as usize * 4; - for (&workspace, &retained) in workspace_pixels[workspace_start..workspace_end] - .iter() - .zip(&retained_pixels[retained_start..retained_end]) - { - max_channel_delta = max_channel_delta.max(workspace.abs_diff(retained)); - } - } - // Vello may round edge coverage a few byte values differently when the - // same vector scene is rasterized at a translated target origin. The - // observed delta stays below one sixteenth of a channel and is confined - // to antialiased edges; a structural or color mismatch is much larger. - assert!( - max_channel_delta <= 16, - "retained scene diverged by {max_channel_delta}/255", - ); + let mut painter = paint::Painter::default(); + let mut output = Scene::new(); + build_workspace_scene(&mut output, &mut painter, &source, (1000, 720), 1.0); } } diff --git a/crates/jcode-desktop2/src/scroll.rs b/crates/jcode-desktop2/src/scroll.rs index db432d6d66..a8c583377e 100644 --- a/crates/jcode-desktop2/src/scroll.rs +++ b/crates/jcode-desktop2/src/scroll.rs @@ -303,7 +303,11 @@ impl Smooth { fn edge_brake(&self) -> Option { let room = self.room.filter(|room| *room > 0.0)?; let speed = self.velocity.abs(); - (speed > MIN_VELOCITY).then(|| speed * speed / (2.0 * room)) + // The continuous solution lands exactly only with continuous integration. + // We integrate once per frame and spend the post-brake velocity, so a + // small safety margin is needed to avoid reaching the last frame with a + // visibly non-zero step that the edge clamp then cuts off. + (speed > MIN_VELOCITY).then(|| 1.2 * speed * speed / (2.0 * room)) } /// Kill the fling: the view has hit the top or the tail, and coasting into diff --git a/crates/jcode-desktop2/src/tests/overview_gesture.rs b/crates/jcode-desktop2/src/tests/overview_gesture.rs index c5f184c57a..9e4dfebbd3 100644 --- a/crates/jcode-desktop2/src/tests/overview_gesture.rs +++ b/crates/jcode-desktop2/src/tests/overview_gesture.rs @@ -52,21 +52,96 @@ fn app() -> App { weight: 1_200.0, }, ]; - let mut app = App::default(); + let mut app = App { + // The held-Super field is benched by default; these tests exercise + // the machinery behind the flag so it stays healthy while benched. + super_overview: true, + ..App::default() + }; app.model.session_id = Some("session_mushroom_2_b".into()); app.model.strip = Strip::build(entries, Some("session_mushroom_2_b")); app } -/// The gesture ships on: a default app must open the field when Super goes -/// down, because the whole point is that it costs zero configuration. +/// The gesture is benched: a default app leaves Super as a plain chord +/// modifier, because the workspace now moves like niri directly and a +/// zoomed-out field would be a second spatial model fighting the first. #[test] -fn the_super_overview_is_enabled_by_default() { +fn the_super_overview_is_benched_by_default() { let mut app = App::default(); app.on_super_changed(true, Instant::now()); assert!( - app.model.overview.is_visible(), - "the default app did not open the field on Super" + !app.model.overview.is_visible(), + "the benched field opened on a bare Super press" + ); +} + +/// Super+hjkl on a default app is direct niri motion: the session switches +/// at once and the camera slides, with no overview in between. +#[test] +fn super_hjkl_is_direct_motion_by_default() { + let mut app = App::default(); + app.model.session_id = Some("session_clover_1_a".into()); + app.model.strip = Strip::build( + vec![ + Entry { + session_id: "session_clover_1_a".into(), + title: None, + working_dir: Some("/home/j/jcode".into()), + busy: false, + weight: 1.0, + }, + Entry { + session_id: "session_mushroom_2_b".into(), + title: None, + working_dir: Some("/home/j/jcode".into()), + busy: false, + weight: 1.0, + }, + Entry { + session_id: "session_harbor_4_d".into(), + title: None, + working_dir: Some("/home/j/site".into()), + busy: false, + weight: 1.0, + }, + ], + Some("session_clover_1_a"), + ); + app.modifiers = winit::keyboard::ModifiersState::SUPER; + app.on_super_changed(true, Instant::now()); + assert!(!app.model.overview.is_visible()); + + app.key_pressed(&Key::Character(SmolStr::new("l")), Some("l")); + assert_eq!( + app.model.session_id.as_deref(), + Some("session_mushroom_2_b"), + "super+l did not switch session directly" + ); + assert!( + app.model.workspace.is_animating(), + "the direct switch did not slide the camera" + ); + + app.key_pressed(&Key::Character(SmolStr::new("j")), Some("j")); + assert_eq!( + app.model.session_id.as_deref(), + Some("session_harbor_4_d"), + "super+j did not switch to the next workspace" + ); + assert_eq!( + app.model.workspace.row_change(), + Some(crate::workspace::Direction::Down), + "the workspace switch was not a vertical row slide" + ); + let (prev_row, _) = app.model.workspace.prev_row(); + assert_eq!( + prev_row, + [ + "session_clover_1_a".to_string(), + "session_mushroom_2_b".to_string() + ], + "the departing row was not captured for the slide" ); } @@ -520,7 +595,10 @@ fn dead_axes_move_but_live_edges_clamp() { }; // Up and down could never move in a single-row field, so they cycle. for dir in [crate::overview::Dir::Up, crate::overview::Dir::Down] { - let mut app = App::default(); + let mut app = App { + super_overview: true, + ..App::default() + }; app.model.session_id = Some("session_0".into()); app.model.strip = Strip::build(entries(), Some("session_0")); let opened = hold_super(&mut app); @@ -534,7 +612,10 @@ fn dead_axes_move_but_live_edges_clamp() { } // Left at the start of the row is an edge of a live axis: it clamps // instead of wrapping to the far end. - let mut app = App::default(); + let mut app = App { + super_overview: true, + ..App::default() + }; app.model.session_id = Some("session_0".into()); app.model.strip = Strip::build(entries(), Some("session_0")); let opened = hold_super(&mut app); diff --git a/crates/jcode-desktop2/src/transcript.rs b/crates/jcode-desktop2/src/transcript.rs index 46c688545e..c5d63aafc9 100644 --- a/crates/jcode-desktop2/src/transcript.rs +++ b/crates/jcode-desktop2/src/transcript.rs @@ -857,6 +857,9 @@ pub struct LaidBlock { /// Native OpenType-MATH layout for a display equation. When present the /// scene draws this instead of the terminal-oriented Unicode fallback. pub math: Option, + /// Native formulas embedded in this block's Parley inline boxes. The box id + /// is the index into this vector, so layout and drawing share exact geometry. + pub inline_math: Vec, /// The block's flattened plain text, the same string the layout was built /// from. Kept so a pointer selection can slice it for the clipboard /// without re-parsing the markdown or reading back from the GPU. @@ -1152,6 +1155,11 @@ pub fn lay_out_message_reusing( source = latex.to_owned(); crate::math::shared().typeset(latex, f64::from(base.font_size)) }); + let inline_math: Vec<_> = spans + .iter() + .filter_map(|span| span.latex.as_deref()) + .map(|latex| crate::math::shared().typeset_inline(latex, f64::from(base.font_size))) + .collect(); // An edit card's code block is a diff, so each of its lines takes the // ink of the side it is on. Applied here, over the flattened block, // because "which side" is a property of the whole line and markdown has @@ -1237,6 +1245,7 @@ pub fn lay_out_message_reusing( style, Palette { theme, tint }, scale, + &inline_math, ); fresh += 1; let mut height = if let Some(formula) = math.as_ref() { @@ -1265,11 +1274,18 @@ pub fn lay_out_message_reusing( }; blocks.push(LaidBlock { glyphs: math.as_ref().map_or_else( - || crate::text::glyph_count(&layout), + || { + crate::text::glyph_count(&layout) + + inline_math + .iter() + .map(crate::math::Formula::glyphs) + .sum::() + }, |formula| formula.glyphs(), ), layout, math, + inline_math, source, top, height, @@ -1342,6 +1358,7 @@ fn diff_spans(source: &str, language: Option<&str>, theme: &Theme) -> Vec) -> Vec Vec { underline: false, strikethrough: false, color: Some(color), + latex: None, }); } } @@ -2132,6 +2151,9 @@ pub struct SpanStyle { /// tint. Only diff lines use it: an added line is green and a removed one /// red regardless of the role the markdown gave the text. pub color: Option, + /// Original TeX for native inline layout. The flattened source contains one + /// object-replacement character at this range rather than Unicode math. + pub latex: Option, } /// Flatten styled lines into one string plus byte-ranged styling. Parley wants @@ -2146,7 +2168,11 @@ pub fn flatten(lines: &[StyledLine]) -> (String, Vec) { } for span in &line.spans { let start = source.len(); - source.push_str(&span.text); + if span.latex.is_some() { + source.push('\u{fffc}'); + } else { + source.push_str(&span.text); + } spans.push(SpanStyle { range: start..source.len(), role: span.role, @@ -2156,6 +2182,7 @@ pub fn flatten(lines: &[StyledLine]) -> (String, Vec) { underline: span.attrs.underline || role_is_underlined(span.role), strikethrough: span.attrs.strikethrough, color: None, + latex: span.latex.clone(), }); } } @@ -2194,8 +2221,10 @@ pub fn layout_rich( style: ParagraphStyle, palette: Palette<'_>, scale: f64, + inline_math: &[crate::math::Formula], ) -> Layout { text.layout_rich(source, width as f32, style, scale, &mut |builder| { + let mut math_id = 0usize; for span in spans { if span.range.is_empty() { continue; @@ -2223,6 +2252,20 @@ pub fn layout_rich( if span.strikethrough { builder.push(StyleProperty::Strikethrough(true), span.range.clone()); } + if span.latex.is_some() { + let id = math_id; + math_id += 1; + if let Some(formula) = inline_math.get(id) { + builder.push(StyleProperty::FontSize(0.0), span.range.clone()); + builder.push_inline_box(parley::InlineBox { + id: id as u64, + kind: parley::InlineBoxKind::InFlow, + index: span.range.start, + width: (formula.width * scale) as f32, + height: (formula.height * scale) as f32, + }); + } + } } }) } @@ -3101,6 +3144,15 @@ mod tests { "inline latex was not rendered to math: {text:?}" ); assert!(!text.contains("x^2"), "raw latex source survived: {text:?}"); + + let laid = laid("the value $x^2$ grows"); + assert_eq!(laid.blocks[0].inline_math.len(), 1); + assert!( + laid.blocks[0].source.contains('\u{fffc}'), + "native inline math did not replace the Unicode fallback: {:?}", + laid.blocks[0].source + ); + assert!(!laid.blocks[0].source.contains('²')); } #[test] diff --git a/crates/jcode-desktop2/src/workspace.rs b/crates/jcode-desktop2/src/workspace.rs index 3ac2ce3ac9..18b8696fe4 100644 --- a/crates/jcode-desktop2/src/workspace.rs +++ b/crates/jcode-desktop2/src/workspace.rs @@ -1,18 +1,34 @@ -//! Pure geometry and animation state for the horizontal session workspace. +//! Pure geometry and animation state for the niri-style session workspace. //! //! The renderer consumes this module but no GPU or window types leak into it. -//! That keeps the niri-style camera behavior deterministic and makes the edge -//! cases (cyclic session order, two-column ties, interrupted transitions) cheap -//! to exercise in unit tests. +//! That keeps the camera behavior deterministic and makes the edge cases +//! (cyclic session order, two-column ties, interrupted transitions, mid-slide +//! row changes) cheap to exercise in unit tests. +//! +//! The spatial model is the compositor's own: a *row* is a working directory +//! (a niri workspace) and a *column* is a session in it (a window). Left and +//! right slide the camera along the focused row; up and down slide the whole +//! row off and the next one in, exactly the motion niri makes for a workspace +//! switch. Only the focused row is ever on screen when the camera is at rest, +//! which is what makes a column's neighbors always mean "same project". /// Focused columns occupy most, but not all, of the viewport. The remaining -/// strip is split between the adjacent sessions so they are always discoverable. +/// strip is split between the adjacent sessions so they are always +/// discoverable. const COLUMN_FRACTION: f64 = 0.76; /// Space between session pages, in logical pixels. pub const GAP: f64 = 14.0; +/// Breathing room above and below every page while the workspace chrome is +/// drawn, in logical pixels. This is what turns "one full-bleed page" into +/// "windows on a desk": without it the boundary rings would run into the +/// window edge and read as clutter rather than as borders. +pub const VERTICAL_INSET: f64 = 10.0; /// A focus change should be quick enough to feel like navigation, while long /// enough for the eye to track which neighboring page became active. pub const TRANSITION_SECONDS: f32 = 0.18; +/// A row change travels the full window height, so it gets slightly longer +/// than a column slide or the motion reads as a flash rather than a move. +pub const ROW_TRANSITION_SECONDS: f32 = 0.22; const PHASE_MAX: u16 = 1000; #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -20,32 +36,48 @@ pub enum Direction { Left, #[default] Right, + Up, + Down, } impl Direction { fn sign(self) -> f64 { match self { - Self::Left => -1.0, - Self::Right => 1.0, + Self::Left | Self::Up => -1.0, + Self::Right | Self::Down => 1.0, } } + + pub fn is_horizontal(self) -> bool { + matches!(self, Self::Left | Self::Right) + } } #[derive(Clone, Copy, Debug, PartialEq, Eq)] struct Transition { direction: Direction, phase: u16, + /// Stored per transition because a column slide and a row slide run at + /// different speeds. Milliseconds, so the model stays `Eq`. + duration_ms: u16, } -/// Horizontal camera state. Session identity and ordering remain owned by the -/// strip, so switching still uses the existing attach path. +/// Horizontal and vertical camera state. Session identity and ordering remain +/// owned by the strip, so switching still uses the existing attach path. #[derive(Clone, Debug, PartialEq, Eq)] pub struct Workspace { transition: Option, - /// Resolves the exactly-opposite column in an even-sized ring. Retaining the - /// last direction avoids a column teleporting to the other side when an - /// animation reaches its final frame. + /// Resolves the exactly-opposite column in an even-sized ring. Retaining + /// the last direction avoids a column teleporting to the other side when + /// an animation reaches its final frame. side_bias: Direction, + /// The sessions of the row being left, captured when a vertical slide + /// begins. Ids rather than indices because the session list can be + /// re-polled mid-slide, and an index into a reshuffled list would draw + /// the wrong conversations flying off screen. + prev_row: Vec, + /// Which column of the departing row was focused, so it exits centered. + prev_focused: usize, } impl Default for Workspace { @@ -53,10 +85,24 @@ impl Default for Workspace { Self { transition: None, side_bias: Direction::Right, + prev_row: Vec::new(), + prev_focused: 0, } } } +/// One row of sessions, as the camera needs to know it: which flat entries it +/// holds, which of them is focused, and how wide its columns are. +#[derive(Clone, Debug, PartialEq)] +pub struct RowSpec { + /// Indices into the strip's flat entry list, in row order. + pub indices: Vec, + /// Position of the focused column within `indices`. + pub focused_pos: usize, + /// Native pixel width of each column in this row. + pub column_width: f64, +} + /// One session page in viewport coordinates. Width is deliberately not scaled: /// the focused page is native size, while clipping at the viewport exposes its /// neighbors rather than shrinking the active model into a thumbnail. @@ -64,25 +110,58 @@ impl Default for Workspace { pub struct Column { pub index: usize, pub x: f64, + pub y: f64, pub width: f64, pub focused: bool, } impl Column { - pub fn is_visible(self, viewport_width: f64) -> bool { - self.x < viewport_width && self.x + self.width > 0.0 + /// Whether any part of this full-height page intersects the viewport. + pub fn is_visible(self, viewport: (f64, f64)) -> bool { + self.x < viewport.0 + && self.x + self.width > 0.0 + && self.y < viewport.1 + && self.y + viewport.1 > 0.0 } } impl Workspace { - /// Start a camera move after the strip has moved focus to its destination. - /// At phase zero the old neighbor is still centered and the new focused - /// page starts one pitch away; the offset then eases to zero. + /// Start a horizontal camera move after the strip has moved focus to its + /// destination. At phase zero the old neighbor is still centered and the + /// new focused page starts one pitch away; the offset then eases to zero. pub fn begin(&mut self, direction: Direction) { + if !direction.is_horizontal() { + return; + } self.side_bias = direction; + self.prev_row.clear(); self.transition = Some(Transition { direction, phase: 0, + duration_ms: (TRANSITION_SECONDS * 1000.0) as u16, + }); + } + + /// Start a vertical row slide after the strip has moved focus to another + /// group. `prev_row` is the departing row's session ids and `prev_focused` + /// which of them was centered, so the old workspace exits exactly as it + /// stood rather than snapping to some canonical arrangement first. + pub fn begin_row_change( + &mut self, + direction: Direction, + prev_row: Vec, + prev_focused: usize, + ) { + if direction.is_horizontal() { + self.begin(direction); + return; + } + self.prev_row = prev_row; + self.prev_focused = prev_focused; + self.transition = Some(Transition { + direction, + phase: 0, + duration_ms: (ROW_TRANSITION_SECONDS * 1000.0) as u16, }); } @@ -90,62 +169,122 @@ impl Workspace { self.transition.is_some() } + /// The running vertical slide's direction, if one is running. The scene + /// uses this to know the departing row still needs drawing. + pub fn row_change(&self) -> Option { + self.transition + .filter(|transition| !transition.direction.is_horizontal()) + .map(|transition| transition.direction) + } + + /// The departing row: its session ids and its focused position. + pub fn prev_row(&self) -> (&[String], usize) { + (&self.prev_row, self.prev_focused) + } + /// Advance by elapsed seconds. Returns true when the visible camera changed. pub fn advance(&mut self, dt: f32) -> bool { let Some(transition) = self.transition.as_mut() else { return false; }; - let step = (dt.max(0.0) / TRANSITION_SECONDS * f32::from(PHASE_MAX)).max(1.0) as u16; + let seconds = (f32::from(transition.duration_ms) / 1000.0).max(0.01); + let step = (dt.max(0.0) / seconds * f32::from(PHASE_MAX)).max(1.0) as u16; transition.phase = transition.phase.saturating_add(step).min(PHASE_MAX); if transition.phase == PHASE_MAX { self.transition = None; + self.prev_row.clear(); } true } - fn camera_offset(&self, pitch: f64) -> f64 { + /// Eased progress of the running transition, 1.0 at rest. + fn progress(&self) -> f64 { let Some(transition) = self.transition else { - return 0.0; + return 1.0; }; let linear = f64::from(transition.phase) / f64::from(PHASE_MAX); // Smoothstep settles at both ends without a velocity discontinuity. - let eased = linear * linear * (3.0 - 2.0 * linear); - transition.direction.sign() * pitch * (1.0 - eased) + linear * linear * (3.0 - 2.0 * linear) } - /// Lay out every session around `focused_index` in a cyclic horizontal ring. + /// Lay out the focused row (and, during a vertical slide, the departing + /// one) in viewport coordinates. pub fn layout( &self, - session_count: usize, - focused_index: usize, - viewport_width: f64, - column_width: f64, + current: &RowSpec, + previous: Option<&RowSpec>, + viewport: (f64, f64), gap: f64, ) -> Vec { - if session_count == 0 { - return Vec::new(); - } - let focused_index = focused_index.min(session_count - 1); - let pitch = column_width + gap; - let centered = (viewport_width - column_width) / 2.0; - let camera = self.camera_offset(pitch); - (0..session_count) - .map(|index| { - let relative = ring_offset(index, focused_index, session_count, self.side_bias); - Column { - index, - x: centered + relative as f64 * pitch + camera, - width: column_width, - focused: index == focused_index, + let eased = self.progress(); + match self.transition { + Some(transition) if !transition.direction.is_horizontal() => { + // The new row arrives from the direction the user moved (down + // brings the row below up into place) while the old one exits + // out the opposite edge, one full viewport height apart. + let travel = transition.direction.sign() * viewport.1; + let dy = travel * (1.0 - eased); + let mut columns = + row_columns(current, viewport, gap, 0.0, dy, self.side_bias, true); + if let Some(previous) = previous { + columns.extend(row_columns( + previous, + viewport, + gap, + 0.0, + dy - travel, + self.side_bias, + false, + )); } - }) - .collect() + columns + } + Some(transition) => { + let pitch = current.column_width + gap; + let dx = transition.direction.sign() * pitch * (1.0 - eased); + row_columns(current, viewport, gap, dx, 0.0, self.side_bias, true) + } + None => row_columns(current, viewport, gap, 0.0, 0.0, self.side_bias, true), + } + } +} + +/// Lay one row out as a cyclic horizontal ring around its focused column. +fn row_columns( + row: &RowSpec, + viewport: (f64, f64), + gap: f64, + dx: f64, + dy: f64, + bias: Direction, + carries_focus: bool, +) -> Vec { + if row.indices.is_empty() { + return Vec::new(); } + let len = row.indices.len(); + let focused_pos = row.focused_pos.min(len - 1); + let pitch = row.column_width + gap; + let centered = (viewport.0 - row.column_width) / 2.0; + row.indices + .iter() + .enumerate() + .map(|(pos, &index)| { + let relative = ring_offset(pos, focused_pos, len, bias); + Column { + index, + x: centered + relative as f64 * pitch + dx, + y: dy, + width: row.column_width, + focused: carries_focus && pos == focused_pos, + } + }) + .collect() } -/// Native pixel width used to build a session page. One session retains the -/// legacy full-window layout; a workspace reserves enough edge space for both -/// neighboring columns. +/// Native pixel width used to build a session page. A row of one session +/// keeps the legacy full-window layout; a wider row reserves enough edge +/// space for both neighboring columns. pub fn column_width(viewport_width: u32, session_count: usize) -> u32 { if session_count <= 1 { return viewport_width; @@ -153,6 +292,74 @@ pub fn column_width(viewport_width: u32, session_count: usize) -> u32 { ((f64::from(viewport_width) * COLUMN_FRACTION).round() as u32).clamp(1, viewport_width.max(1)) } +/// Resolve the strip into camera columns: the focused working-dir group as +/// the current row, plus the departing group while a vertical slide runs. +/// +/// One function shared by the renderer and by pointer conversion, for the +/// same reason [`crate::layout::Frame`] is shared: if the two ever disagreed, +/// clicks would land on a different page than the one under the cursor. +pub fn placement( + strip: &crate::strip::Strip, + workspace: &Workspace, + session_id: Option<&str>, + viewport: (f64, f64), + gap: f64, +) -> Vec { + let groups = strip.groups(); + if groups.is_empty() { + return Vec::new(); + } + // Flat entry index of each group's first session, matching + // `Strip::entries` order, so a `Column::index` addresses the same session + // everywhere. + let mut bases = Vec::with_capacity(groups.len()); + let mut base = 0usize; + for group in groups { + bases.push(base); + base += group.entries.len(); + } + let locate = |id: &str| { + groups.iter().enumerate().find_map(|(g, group)| { + group + .entries + .iter() + .position(|entry| entry.session_id == id) + .map(|i| (g, i)) + }) + }; + // The attached session anchors the camera; the strip's own focus is the + // fallback for the moments before an attach resolves. + let (group, pos) = session_id.and_then(locate).unwrap_or_else(|| { + let group = strip.group_index().min(groups.len() - 1); + let len = groups[group].entries.len(); + (group, strip.index().min(len.saturating_sub(1))) + }); + let row: Vec = (0..groups[group].entries.len()) + .map(|i| bases[group] + i) + .collect(); + let current = RowSpec { + column_width: f64::from(column_width(viewport.0.round() as u32, row.len())), + focused_pos: pos, + indices: row, + }; + let previous = workspace + .row_change() + .map(|_| { + let (ids, prev_focused) = workspace.prev_row(); + let indices: Vec = ids + .iter() + .filter_map(|id| locate(id).map(|(g, i)| bases[g] + i)) + .collect(); + RowSpec { + column_width: f64::from(column_width(viewport.0.round() as u32, indices.len())), + focused_pos: prev_focused.min(indices.len().saturating_sub(1)), + indices, + } + }) + .filter(|row| !row.indices.is_empty()); + workspace.layout(¤t, previous.as_ref(), viewport, gap) +} + fn ring_offset(index: usize, focused: usize, len: usize, bias: Direction) -> isize { if len <= 1 { return 0; @@ -167,7 +374,7 @@ fn ring_offset(index: usize, focused: usize, len: usize, bias: Direction) -> isi std::cmp::Ordering::Greater => backward, std::cmp::Ordering::Equal => match bias { Direction::Left => forward as isize, - Direction::Right => backward, + _ => backward, }, } } @@ -175,13 +382,22 @@ fn ring_offset(index: usize, focused: usize, len: usize, bias: Direction) -> isi #[cfg(test)] mod tests { use super::*; + use crate::strip::{Entry, Strip}; - const VIEW: f64 = 1000.0; + const VIEW: (f64, f64) = (1000.0, 700.0); const WIDTH: f64 = 760.0; + fn row(count: usize, focused_pos: usize) -> RowSpec { + RowSpec { + indices: (0..count).collect(), + focused_pos, + column_width: WIDTH, + } + } + #[test] fn focused_column_is_native_width_with_neighbors_visible() { - let columns = Workspace::default().layout(3, 1, VIEW, WIDTH, GAP); + let columns = Workspace::default().layout(&row(3, 1), None, VIEW, GAP); let focused = columns.iter().find(|column| column.focused).unwrap(); assert_eq!(focused.width, WIDTH); assert_eq!(focused.x, 120.0); @@ -200,17 +416,17 @@ mod tests { fn right_navigation_starts_on_the_previous_page_and_settles_on_target() { let mut workspace = Workspace::default(); workspace.begin(Direction::Right); - let start = workspace.layout(3, 1, VIEW, WIDTH, GAP); + let start = workspace.layout(&row(3, 1), None, VIEW, GAP); assert_eq!(start[0].x, 120.0); assert_eq!(start[1].x, 120.0 + WIDTH + GAP); workspace.advance(TRANSITION_SECONDS / 2.0); - let middle = workspace.layout(3, 1, VIEW, WIDTH, GAP); + let middle = workspace.layout(&row(3, 1), None, VIEW, GAP); assert!(middle[1].x > 120.0); assert!(middle[1].x < start[1].x); workspace.advance(TRANSITION_SECONDS); - let end = workspace.layout(3, 1, VIEW, WIDTH, GAP); + let end = workspace.layout(&row(3, 1), None, VIEW, GAP); assert!(!workspace.is_animating()); assert!((end[1].x - 120.0).abs() < f64::EPSILON); } @@ -219,7 +435,7 @@ mod tests { fn left_navigation_is_the_mirror_image() { let mut workspace = Workspace::default(); workspace.begin(Direction::Left); - let columns = workspace.layout(3, 1, VIEW, WIDTH, GAP); + let columns = workspace.layout(&row(3, 1), None, VIEW, GAP); assert_eq!(columns[2].x, 120.0); assert_eq!(columns[1].x, 120.0 - WIDTH - GAP); } @@ -229,22 +445,142 @@ mod tests { let mut workspace = Workspace::default(); workspace.begin(Direction::Right); workspace.advance(TRANSITION_SECONDS * 2.0); - let columns = workspace.layout(2, 1, VIEW, WIDTH, GAP); + let columns = workspace.layout(&row(2, 1), None, VIEW, GAP); assert!(columns[0].x < columns[1].x); workspace.begin(Direction::Left); workspace.advance(TRANSITION_SECONDS * 2.0); - let columns = workspace.layout(2, 1, VIEW, WIDTH, GAP); + let columns = workspace.layout(&row(2, 1), None, VIEW, GAP); assert!(columns[0].x > columns[1].x); } #[test] fn invisible_columns_are_still_stably_ordered_in_the_ring() { - let columns = Workspace::default().layout(7, 3, VIEW, WIDTH, GAP); + let columns = Workspace::default().layout(&row(7, 3), None, VIEW, GAP); assert_eq!(columns.len(), 7); assert_eq!(columns.iter().filter(|column| column.focused).count(), 1); assert_eq!(columns[3].x, 120.0); assert!(columns[2].x < columns[3].x); assert!(columns[4].x > columns[3].x); } + + /// The niri workspace switch: moving down brings the new row up from the + /// bottom while the old one exits out the top, and the slide settles with + /// only the new row on screen. + #[test] + fn a_downward_row_change_slides_the_old_row_out_the_top() { + let mut workspace = Workspace::default(); + workspace.begin_row_change(Direction::Down, vec!["old".into()], 0); + let prev = RowSpec { + indices: vec![9], + focused_pos: 0, + column_width: VIEW.0, + }; + + let start = workspace.layout(&row(2, 0), Some(&prev), VIEW, GAP); + let new_row: Vec<&Column> = start.iter().filter(|c| c.index != 9).collect(); + let old = start.iter().find(|c| c.index == 9).unwrap(); + assert_eq!(old.y, 0.0, "the departing row did not start centered"); + assert!( + new_row.iter().all(|c| (c.y - VIEW.1).abs() < 1e-9), + "the arriving row did not start one viewport below" + ); + assert!(!old.focused, "the departing row kept focus"); + assert_eq!(new_row.iter().filter(|c| c.focused).count(), 1); + + workspace.advance(ROW_TRANSITION_SECONDS / 2.0); + let middle = workspace.layout(&row(2, 0), Some(&prev), VIEW, GAP); + let old = middle.iter().find(|c| c.index == 9).unwrap(); + assert!(old.y < 0.0, "the departing row never started leaving"); + + workspace.advance(ROW_TRANSITION_SECONDS * 2.0); + assert!(!workspace.is_animating()); + assert_eq!(workspace.row_change(), None); + let end = workspace.layout(&row(2, 0), None, VIEW, GAP); + assert!(end.iter().all(|c| c.y == 0.0)); + } + + #[test] + fn an_upward_row_change_is_the_mirror_image() { + let mut workspace = Workspace::default(); + workspace.begin_row_change(Direction::Up, vec!["old".into()], 0); + let prev = RowSpec { + indices: vec![9], + focused_pos: 0, + column_width: VIEW.0, + }; + let start = workspace.layout(&row(1, 0), Some(&prev), VIEW, GAP); + let arriving = start.iter().find(|c| c.index == 0).unwrap(); + assert!( + (arriving.y + VIEW.1).abs() < 1e-9, + "up did not arrive from above" + ); + } + + fn entry(id: &str, dir: &str) -> Entry { + Entry { + session_id: id.into(), + title: None, + working_dir: Some(dir.into()), + busy: false, + weight: 0.0, + } + } + + /// Only the focused working directory's sessions are on screen at rest: + /// that is what makes a column's neighbors always mean "same project". + #[test] + fn placement_shows_only_the_focused_group_at_rest() { + let strip = Strip::build( + vec![ + entry("a1", "/w/jcode"), + entry("a2", "/w/jcode"), + entry("b1", "/w/site"), + ], + Some("a1"), + ); + let columns = placement(&strip, &Workspace::default(), Some("a1"), VIEW, GAP); + let indices: Vec = columns.iter().map(|c| c.index).collect(); + assert_eq!(indices, vec![0, 1], "another project's session leaked in"); + assert!(columns[0].focused); + } + + /// During a vertical slide both rows exist, each with its own column + /// width, and the departing row is resolved by session id so a re-polled + /// list cannot make the wrong pages fly off. + #[test] + fn placement_draws_the_departing_row_during_a_slide() { + let strip = Strip::build( + vec![ + entry("a1", "/w/jcode"), + entry("a2", "/w/jcode"), + entry("b1", "/w/site"), + ], + Some("b1"), + ); + let mut workspace = Workspace::default(); + workspace.begin_row_change(Direction::Down, vec!["a1".into(), "a2".into()], 1); + let columns = placement(&strip, &workspace, Some("b1"), VIEW, GAP); + let indices: Vec = columns.iter().map(|c| c.index).collect(); + assert_eq!(indices, vec![2, 0, 1]); + // The lone arriving column is full width; the departing pair is not. + assert_eq!(columns[0].width, VIEW.0); + assert_eq!(columns[1].width, WIDTH); + // The departing row exits as it stood: its second column centered. + let a2 = columns.iter().find(|c| c.index == 1).unwrap(); + assert_eq!(a2.x, 120.0); + assert!(!a2.focused); + } + + /// A departed session that vanished from a re-poll is simply skipped + /// rather than panicking or drawing a stranger in its place. + #[test] + fn placement_survives_a_departing_session_disappearing() { + let strip = Strip::build(vec![entry("b1", "/w/site")], Some("b1")); + let mut workspace = Workspace::default(); + workspace.begin_row_change(Direction::Down, vec!["gone".into()], 0); + let columns = placement(&strip, &workspace, Some("b1"), VIEW, GAP); + assert_eq!(columns.len(), 1); + assert_eq!(columns[0].index, 0); + } } diff --git a/crates/jcode-provider-anthropic-runtime/src/anthropic_tests.rs b/crates/jcode-provider-anthropic-runtime/src/anthropic_tests.rs index ae8f87a981..43835deba4 100644 --- a/crates/jcode-provider-anthropic-runtime/src/anthropic_tests.rs +++ b/crates/jcode-provider-anthropic-runtime/src/anthropic_tests.rs @@ -1851,6 +1851,65 @@ fn fable_quota_fallback_selects_the_best_available_opus() { assert_eq!(anthropic_model_quality_rank(&fallback), best_rank); } +#[test] +fn model_scoped_usage_routes_only_exhausted_fable_to_opus() { + let usage = jcode_base::usage::UsageData { + model_scoped: vec![jcode_base::usage::ModelScopedUsageWindow { + model_name: "Fable".to_string(), + utilization: 1.0, + resets_at: Some("2026-08-11T00:00:00Z".to_string()), + }], + ..Default::default() + }; + let fallback = AnthropicProvider::fallback_for_model_scoped_usage("claude-fable-5", &usage) + .expect("exhausted Fable should route to Opus"); + assert!( + fallback.contains("claude-opus"), + "unexpected fallback: {fallback}" + ); + assert!( + AnthropicProvider::fallback_for_model_scoped_usage("claude-opus-5", &usage).is_none(), + "an exhausted Fable scope must not reroute an explicitly selected Opus" + ); + + let available = jcode_base::usage::UsageData { + model_scoped: vec![jcode_base::usage::ModelScopedUsageWindow { + model_name: "Fable".to_string(), + utilization: 0.98, + resets_at: None, + }], + ..Default::default() + }; + assert!( + AnthropicProvider::fallback_for_model_scoped_usage("claude-fable-5", &available).is_none(), + "Fable must remain selected while its scoped quota is available" + ); +} + +#[test] +fn detects_live_fable_scoped_limit_errors_without_misrouting_other_limits() { + assert!(is_fable_scoped_limit_error( + "claude-fable-5", + r#"429 {"type":"rate_limit_error","message":"You have reached your weekly Fable limit"}"#, + )); + assert!(is_fable_scoped_limit_error( + "claude-fable-5", + "usage limit reached for the 7-day model window", + )); + assert!(!is_fable_scoped_limit_error( + "claude-opus-5", + "weekly Fable rate limit reached", + )); + assert!(!is_fable_scoped_limit_error( + "claude-fable-5", + "429 overloaded_error: service temporarily overloaded", + )); + assert!(!is_fable_scoped_limit_error( + "claude-fable-5", + "global 5-hour rate limit reached", + )); +} + #[test] fn ping_keepalive_emits_streaming_phase_event() { // Issue #451: during silent reasoning phases, `ping` events can be the diff --git a/crates/jcode-provider-anthropic-runtime/src/lib.rs b/crates/jcode-provider-anthropic-runtime/src/lib.rs index ddf76da6cc..b26c261ed2 100644 --- a/crates/jcode-provider-anthropic-runtime/src/lib.rs +++ b/crates/jcode-provider-anthropic-runtime/src/lib.rs @@ -399,6 +399,16 @@ impl AnthropicProvider { models.into_iter().next() } + fn fallback_for_model_scoped_usage( + selected_model: &str, + usage: &jcode_base::usage::UsageData, + ) -> Option { + (selected_model.to_ascii_lowercase().contains("fable") + && usage.model_scoped_exhausted(selected_model)) + .then(|| Self::best_available_opus_model(selected_model)) + .flatten() + } + async fn model_after_oauth_quota_check( &self, token: &str, @@ -411,10 +421,7 @@ impl AnthropicProvider { let Ok(usage) = jcode_base::usage::fetch_usage_for_access_token(token).await else { return selected_model; }; - if !usage.model_scoped_exhausted(&selected_model) { - return selected_model; - } - let Some(fallback) = Self::best_available_opus_model(&selected_model) else { + let Some(fallback) = Self::fallback_for_model_scoped_usage(&selected_model, &usage) else { return selected_model; }; jcode_base::logging::warn(&format!( @@ -1669,6 +1676,38 @@ async fn run_stream_with_retries( continue; } + // Anthropic OAuth can reject Fable with a model-scoped weekly + // quota error before the usage cache observes the exhausted + // window. This is terminal for Fable, not a transient 429. + if is_oauth + && !saw_output + && is_fable_scoped_limit_error(&model_name, &error_str) + && let Some(fallback) = + AnthropicProvider::best_available_opus_model(&model_name) + { + jcode_base::logging::warn(&format!( + "Anthropic Fable weekly quota is exhausted ({}); retrying with '{}'", + e, fallback + )); + let _ = tx + .send(Ok(StreamEvent::StatusDetail { + detail: format!( + "⚠ '{}' weekly limit reached; switching to '{}'", + strip_1m_suffix(&model_name), + strip_1m_suffix(&fallback) + ), + })) + .await; + request.model = strip_1m_suffix(&fallback).to_string(); + *model_state + .write() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = fallback.clone(); + tried_models.push(fallback.clone()); + model_name = fallback; + last_error = Some(e); + continue; + } + // Reasoning request rejected (e.g. a model listed with effort or // thinking capabilities that the live API does not actually // accept: "adaptive thinking is not supported on this model" or @@ -1983,6 +2022,24 @@ fn is_retryable_error(error_str: &str) -> bool { || error_str.contains("internal server error") } +fn is_fable_scoped_limit_error(model: &str, error: &str) -> bool { + let model = strip_1m_suffix(model).to_ascii_lowercase(); + if !model.contains("fable") { + return false; + } + let error = error.to_ascii_lowercase(); + let is_limit = error.contains("rate_limit") + || error.contains("rate limit") + || error.contains("usage_limit") + || error.contains("usage limit"); + let is_scoped = error.contains("fable") + || error.contains("weekly") + || error.contains("week limit") + || error.contains("7-day") + || error.contains("7 day"); + is_limit && is_scoped +} + /// Detect an Anthropic "model not found" rejection. /// /// Anthropic returns HTTP 404 with `"type":"not_found_error"` when a model id diff --git a/crates/jcode-provider-anthropic/Cargo.toml b/crates/jcode-provider-anthropic/Cargo.toml index 9df8dd5262..e10eb69cf7 100644 --- a/crates/jcode-provider-anthropic/Cargo.toml +++ b/crates/jcode-provider-anthropic/Cargo.toml @@ -4,6 +4,7 @@ version = "0.1.0" edition = "2024" [dependencies] +jcode-schema-dialect = { path = "../jcode-schema-dialect" } jcode-logging = { path = "../jcode-logging" } jcode-message-types = { path = "../jcode-message-types" } jcode-provider-core = { path = "../jcode-provider-core" } diff --git a/crates/jcode-provider-anthropic/src/lib.rs b/crates/jcode-provider-anthropic/src/lib.rs index 7a40f88b88..65e5365014 100644 --- a/crates/jcode-provider-anthropic/src/lib.rs +++ b/crates/jcode-provider-anthropic/src/lib.rs @@ -318,72 +318,19 @@ const OAUTH_BUILTIN_LOCAL_TOOLS: &[&str] = &[ "write", ]; -/// Anthropic accepts JSON Schema combinators inside object properties, but -/// rejects `oneOf`, `anyOf`, and `allOf` at the input schema's top level. Keep -/// the common object shape and widen top-level variants into one object whose -/// properties cover every branch. Runtime tool deserialization remains the -/// authority for action-specific constraints. +/// Normalize a tool schema for Anthropic's `input_schema`. +/// +/// Anthropic accepts JSON Schema combinators inside object properties but +/// rejects `oneOf`/`anyOf`/`allOf` at the top level, and requires an object +/// schema with a `properties` map. The subset and the rewrites live in +/// `jcode-schema-dialect` so every provider shares one implementation and one +/// set of regression tests. +/// +/// Widening a top-level combiner loses the per-branch constraint, which is +/// intended: runtime tool deserialization remains the authority on which +/// combination is actually valid. fn anthropic_input_schema(schema: &Value) -> Value { - let Value::Object(source) = schema else { - return json!({"type": "object", "properties": {}}); - }; - - let mut output = source.clone(); - let mut merged_properties = output - .get("properties") - .and_then(Value::as_object) - .cloned() - .unwrap_or_default(); - let mut all_of_required = Vec::new(); - - for keyword in ["oneOf", "anyOf", "allOf"] { - let Some(branches) = output - .remove(keyword) - .and_then(|value| value.as_array().cloned()) - else { - continue; - }; - for branch in branches { - let Some(branch) = branch.as_object() else { - continue; - }; - if let Some(properties) = branch.get("properties").and_then(Value::as_object) { - for (name, property) in properties { - merged_properties - .entry(name.clone()) - .or_insert_with(|| property.clone()); - } - } - if keyword == "allOf" - && let Some(required) = branch.get("required").and_then(Value::as_array) - { - for name in required.iter().filter_map(Value::as_str) { - if !all_of_required.iter().any(|existing| existing == name) { - all_of_required.push(name.to_string()); - } - } - } - } - } - - output.insert("type".to_string(), Value::String("object".to_string())); - output.insert("properties".to_string(), Value::Object(merged_properties)); - if !all_of_required.is_empty() { - let required = output - .entry("required".to_string()) - .or_insert_with(|| Value::Array(Vec::new())); - if let Value::Array(required) = required { - for name in all_of_required { - if !required - .iter() - .any(|existing| existing.as_str() == Some(&name)) - { - required.push(Value::String(name)); - } - } - } - } - Value::Object(output) + jcode_schema_dialect::normalize(schema, &jcode_schema_dialect::registry::ANTHROPIC) } pub fn format_tools(tools: &[ToolDefinition], is_oauth: bool, cache_ttl_1h: bool) -> Vec { diff --git a/crates/jcode-provider-core/Cargo.toml b/crates/jcode-provider-core/Cargo.toml index a2a434320d..770f41e338 100644 --- a/crates/jcode-provider-core/Cargo.toml +++ b/crates/jcode-provider-core/Cargo.toml @@ -13,6 +13,7 @@ async-trait = "0.1" futures = "0.3" httpdate = "1" jcode-logging = { path = "../jcode-logging" } +jcode-schema-dialect = { path = "../jcode-schema-dialect" } jcode-message-types = { path = "../jcode-message-types" } reqwest = { version = "0.12", default-features = false, features = ["json", "stream", "charset", "http2", "system-proxy", "rustls-tls", "rustls-tls-native-roots"] } serde = { version = "1", features = ["derive"] } diff --git a/crates/jcode-provider-core/src/openai_schema.rs b/crates/jcode-provider-core/src/openai_schema.rs index 085c60e6c9..9c7a541afc 100644 --- a/crates/jcode-provider-core/src/openai_schema.rs +++ b/crates/jcode-provider-core/src/openai_schema.rs @@ -1,234 +1,19 @@ use serde_json::Value; use std::collections::HashSet; -fn merge_string_sets(existing: &Value, incoming: &Value) -> Option { - fn collect_strings(value: &Value) -> Option> { - match value { - Value::String(s) => Some(vec![s.clone()]), - Value::Array(items) => items - .iter() - .map(|item| item.as_str().map(ToString::to_string)) - .collect(), - _ => None, - } - } - - let mut combined = collect_strings(existing)?; - for item in collect_strings(incoming)? { - if !combined.contains(&item) { - combined.push(item); - } - } - - if combined.len() == 1 { - Some(Value::String(combined.remove(0))) - } else { - Some(Value::Array( - combined.into_iter().map(Value::String).collect(), - )) - } -} - -fn merge_schema_objects( - target: &mut serde_json::Map, - incoming: &serde_json::Map, -) { - for (key, incoming_value) in incoming { - match key.as_str() { - "properties" | "$defs" | "definitions" | "patternProperties" => { - let Some(incoming_children) = incoming_value.as_object() else { - target.insert(key.clone(), incoming_value.clone()); - continue; - }; - - match target.get_mut(key) { - Some(Value::Object(existing_children)) => { - for (child_key, child_value) in incoming_children { - if let Some(existing_child) = existing_children.get_mut(child_key) { - merge_schema_values(existing_child, child_value.clone()); - } else { - existing_children.insert(child_key.clone(), child_value.clone()); - } - } - } - _ => { - target.insert(key.clone(), Value::Object(incoming_children.clone())); - } - } - } - "required" | "enum" | "type" => match target.get_mut(key) { - Some(existing_value) => { - if let Some(merged) = merge_string_sets(existing_value, incoming_value) { - *existing_value = merged; - } - } - None => { - target.insert(key.clone(), incoming_value.clone()); - } - }, - "description" | "title" => { - target - .entry(key.clone()) - .or_insert_with(|| incoming_value.clone()); - } - "additionalProperties" => match target.get_mut(key) { - Some(Value::Bool(existing_bool)) => { - if incoming_value == &Value::Bool(false) { - *existing_bool = false; - } - } - Some(Value::Object(existing_obj)) => { - if let Value::Object(incoming_obj) = incoming_value { - merge_schema_objects(existing_obj, incoming_obj); - } else if incoming_value == &Value::Bool(false) { - target.insert(key.clone(), Value::Bool(false)); - } - } - Some(_) => { - if incoming_value == &Value::Bool(false) { - target.insert(key.clone(), Value::Bool(false)); - } - } - None => { - target.insert(key.clone(), incoming_value.clone()); - } - }, - _ => match target.get_mut(key) { - Some(existing_value) => merge_schema_values(existing_value, incoming_value.clone()), - None => { - target.insert(key.clone(), incoming_value.clone()); - } - }, - } - } -} - -fn merge_schema_values(existing: &mut Value, incoming: Value) { - if *existing == incoming { - return; - } - - match incoming { - Value::Object(incoming_map) => { - if let Value::Object(existing_map) = existing { - merge_schema_objects(existing_map, &incoming_map); - } else { - *existing = Value::Object(incoming_map); - } - } - Value::Array(incoming_items) => { - if let Value::Array(existing_items) = existing { - if existing_items != &incoming_items { - for item in incoming_items { - if !existing_items.contains(&item) { - existing_items.push(item); - } - } - } - } else { - *existing = Value::Array(incoming_items); - } - } - incoming_value => { - *existing = incoming_value; - } - } -} - -fn flatten_all_of_schema(mut map: serde_json::Map) -> Value { - let Some(Value::Array(all_of_items)) = map.remove("allOf") else { - return Value::Object(map); - }; - - let mut merged = map; - let mut fallback_any_of = Vec::new(); - - for item in all_of_items { - match item { - Value::Object(item_map) => merge_schema_objects(&mut merged, &item_map), - other => fallback_any_of.push(other), - } - } - - if !fallback_any_of.is_empty() { - match merged.get_mut("anyOf") { - Some(Value::Array(existing_any_of)) => existing_any_of.extend(fallback_any_of), - _ => { - merged.insert("anyOf".to_string(), Value::Array(fallback_any_of)); - } - } - } - - Value::Object(merged) -} - +/// Normalize a tool-parameter schema for the OpenAI function-parameters subset. +/// +/// One construct OpenAI rejects fails the entire tool catalog rather than the +/// one tool, which is why this class of bug (#446, #495, #543, #687, #711, #713, +/// #754) has recurred: each fix appended a keyword to a deny-list, so the next +/// unlisted keyword from the next MCP server was the next outage. +/// +/// The subset is now an allow-list in `jcode-schema-dialect`, shared with every +/// other provider, so a construct nobody has seen yet is dropped rather than +/// forwarded. Strict-mode eligibility and normalization stay here: they are +/// OpenAI-specific and have no dialect equivalent. pub fn openai_compatible_schema(schema: &Value) -> Value { - match schema { - Value::Object(map) => { - let mut out = serde_json::Map::new(); - for (key, value) in map { - if is_openai_unsupported_keyword(key) { - continue; - } - let normalized_key = if key == "oneOf" { "anyOf" } else { key }; - out.insert( - normalized_key.to_string(), - openai_compatible_keyword(key, value), - ); - } - flatten_all_of_schema(out) - } - Value::Array(items) => Value::Array(items.iter().map(openai_compatible_schema).collect()), - _ => schema.clone(), - } -} - -/// JSON Schema keywords that are valid JSON Schema 2020-12 but rejected by the -/// OpenAI function-parameters subset. One unsupported keyword invalidates the -/// entire tool catalog, so they are stripped instead of failing the request -/// (see issue #687). Constraints they express (e.g. `uniqueItems`) stay -/// enforced by the tool/MCP server at execution time. -const OPENAI_UNSUPPORTED_SCHEMA_KEYWORDS: &[&str] = &[ - "uniqueItems", - "contains", - "minContains", - "maxContains", - "unevaluatedItems", - "unevaluatedProperties", - "propertyNames", - "minProperties", - "maxProperties", - "dependentSchemas", - "dependentRequired", - "if", - "then", - "else", - "not", -]; - -fn is_openai_unsupported_keyword(key: &str) -> bool { - OPENAI_UNSUPPORTED_SCHEMA_KEYWORDS.contains(&key) -} - -/// Recurse into a keyword's value while respecting whether the value is a -/// schema, a map of schemas, or plain data. Without this, a property literally -/// named `uniqueItems` inside `properties` would be stripped. -fn openai_compatible_keyword(key: &str, value: &Value) -> Value { - match key { - "properties" | "$defs" | "definitions" | "patternProperties" => match value { - Value::Object(children) => Value::Object( - children - .iter() - .map(|(child_key, child_value)| { - (child_key.clone(), openai_compatible_schema(child_value)) - }) - .collect(), - ), - other => openai_compatible_schema(other), - }, - "enum" | "const" | "examples" | "default" => value.clone(), - _ => openai_compatible_schema(value), - } + jcode_schema_dialect::normalize(schema, &jcode_schema_dialect::registry::OPENAI) } pub fn schema_supports_strict(schema: &Value) -> bool { @@ -262,7 +47,9 @@ pub fn schema_supports_strict(schema: &Value) -> bool { // `key`). Strict eligibility must fail closed here: the schema is still // sent, just without `strict: true`, so the tool stays usable. if let Some(Value::Object(properties)) = map.get("properties") - && properties.values().any(|property| !declares_a_type(property)) + && properties + .values() + .any(|property| !declares_a_type(property)) { return false; } @@ -322,7 +109,15 @@ fn declares_a_type(schema: &Value) -> bool { return schema.is_boolean(); }; const TYPE_BEARING_KEYWORDS: &[&str] = &[ - "type", "enum", "const", "anyOf", "oneOf", "allOf", "$ref", "properties", "items", + "type", + "enum", + "const", + "anyOf", + "oneOf", + "allOf", + "$ref", + "properties", + "items", ]; TYPE_BEARING_KEYWORDS .iter() @@ -821,7 +616,6 @@ mod tests { ); } - /// Issue #711, reproduced independently against master before fixing: four /// constructs from a real MCP catalog that jcode marked `strict: true` and /// OpenAI then rejected, failing the entire tool catalog. @@ -899,5 +693,4 @@ mod tests { "a well-formed schema must keep strict mode" ); } - } diff --git a/crates/jcode-provider-gemini-runtime/Cargo.toml b/crates/jcode-provider-gemini-runtime/Cargo.toml index 692026f368..07cec35af1 100644 --- a/crates/jcode-provider-gemini-runtime/Cargo.toml +++ b/crates/jcode-provider-gemini-runtime/Cargo.toml @@ -30,7 +30,9 @@ uuid = { version = "1", features = ["v4"] } [dev-dependencies] jcode-provider-antigravity = { path = "../jcode-provider-antigravity" } +jcode-provider-anthropic = { path = "../jcode-provider-anthropic" } jcode-provider-openai = { path = "../jcode-provider-openai" } +jcode-provider-openrouter = { path = "../jcode-provider-openrouter" } serde = { version = "1", features = ["derive"] } # The migrated gemini tests use jcode-base's test-env sandbox (lock_test_env). jcode-base = { path = "../jcode-base", default-features = false, features = ["test-support"] } diff --git a/crates/jcode-provider-gemini-runtime/tests/every_provider_sends_clean_schemas.rs b/crates/jcode-provider-gemini-runtime/tests/every_provider_sends_clean_schemas.rs new file mode 100644 index 0000000000..a5bf4c89f2 --- /dev/null +++ b/crates/jcode-provider-gemini-runtime/tests/every_provider_sends_clean_schemas.rs @@ -0,0 +1,370 @@ +//! Is the dialect engine actually the code path each provider uses? +//! +//! The registry sweep proves every dialect *would* produce a sendable schema. +//! It says nothing about whether the provider's request builder calls it. Three +//! providers (OpenAI, OpenRouter, Anthropic) have dialects in the registry and +//! still ship their own older sanitizers, so the sweep was passing for code +//! nothing executes. This makes that gap explicit and bounded. +//! +//! The check is behavioral, not structural: for each provider it runs a hostile +//! schema through the *real* request builder and asserts on what would go on the +//! wire. That holds whether the provider reaches the engine or its own +//! sanitizer, so it keeps working through the migration. + +use jcode_message_types::ToolDefinition; +use serde_json::Value; + +/// A schema combining the trigger from every issue in this class. +fn hostile_schema() -> Value { + serde_json::json!({ + "type": "object", + "properties": { + // #543: unsupported string format. + "url": { "type": "string", "format": "uri" }, + // #687: uniqueItems. + "ids": { "type": "array", "uniqueItems": true, "items": { "type": "string" } }, + // #754: propertyNames (+ additionalProperties). + "data": { + "type": "object", + "propertyNames": { "type": "string" }, + "additionalProperties": { "type": "string" }, + "description": "map of MIME type to value" + }, + // #713: a property with no type at all. + "value": { "description": "type depends on the sibling key" }, + // A property named like a keyword, which must never be mistaken + // for one and deleted. + "uniqueItems": { "type": "boolean", "description": "a real field" } + }, + "required": ["url"] + }) +} + +fn hostile_tool() -> Vec { + vec![ToolDefinition { + name: "mcp__hostile__probe".to_string(), + description: "probe".to_string(), + input_schema: hostile_schema(), + }] +} + +fn contains_key(value: &Value, key: &str) -> bool { + match value { + Value::Object(map) => map.iter().any(|(k, v)| k == key || contains_key(v, key)), + Value::Array(items) => items.iter().any(|i| contains_key(i, key)), + _ => false, + } +} + +/// Whatever normalization a provider uses, the prompt-visible description of a +/// surviving property must survive with it. Losing these is silent: requests +/// still succeed, the model is just told less. +fn assert_descriptions_survive(wire: &Value, provider: &str) { + assert!( + contains_key(wire, "description"), + "{provider} dropped every description: {wire}" + ); + let serialized = wire.to_string(); + assert!( + serialized.contains("map of MIME type to value"), + "{provider} dropped a nested property description: {wire}" + ); +} + +#[test] +fn gemini_sends_a_clean_schema_for_the_hostile_tool() { + let built = jcode_provider_gemini::build_tools(&hostile_tool()).expect("tools"); + let wire = serde_json::to_value(&built).expect("serialize"); + + for rejected in ["propertyNames", "additionalProperties", "uniqueItems"] { + // `uniqueItems` appears as a property NAME, so check the schema + // position rather than the whole document. + if rejected == "uniqueItems" { + let parameters = &built[0].function_declarations[0].parameters; + assert!( + parameters["properties"]["ids"].get("uniqueItems").is_none(), + "gemini kept the uniqueItems keyword: {parameters}" + ); + assert_eq!( + parameters["properties"]["uniqueItems"]["type"], "boolean", + "gemini deleted a property named like a keyword: {parameters}" + ); + continue; + } + assert!(!contains_key(&wire, rejected), "gemini kept {rejected}"); + } + assert_descriptions_survive(&wire, "gemini"); +} + +#[test] +fn every_antigravity_route_sends_a_clean_schema_for_the_hostile_tool() { + let schema = hostile_schema(); + for model in ["gemini-3-flash", "claude-sonnet-4-5", "gpt-oss-120b"] { + let normalized = jcode_provider_antigravity::antigravity_compatible_schema(&schema, model); + for rejected in ["propertyNames", "additionalProperties"] { + assert!( + !contains_key(&normalized, rejected), + "antigravity model `{model}` kept {rejected}: {normalized}" + ); + } + assert_descriptions_survive(&normalized, &format!("antigravity/{model}")); + } +} + +/// OpenAI still uses its own sanitizer rather than the engine, so this asserts +/// the *outcome* the class requires: nothing OpenAI rejects goes out, and the +/// typeless property does not get a `strict` claim jcode cannot honor. +#[test] +fn openai_sends_a_clean_schema_and_does_not_overclaim_strict() { + let built = jcode_provider_openai::request::build_tools(&hostile_tool()); + let wire = serde_json::to_value(&built).expect("serialize"); + + assert!( + !contains_key(&wire, "propertyNames"), + "openai kept propertyNames: {wire}" + ); + let parameters = &wire[0]["parameters"]; + assert!( + parameters["properties"]["ids"].get("uniqueItems").is_none(), + "openai kept the uniqueItems keyword: {parameters}" + ); + assert!( + parameters["properties"]["url"].get("format").is_none(), + "openai kept an unsupported format: {parameters}" + ); + // #713: a typeless property must force strict off, not be rewritten away. + assert_eq!( + wire[0]["strict"], false, + "openai claimed strict for a schema it rejects: {wire}" + ); + assert!( + parameters["properties"].get("value").is_some(), + "openai dropped the typeless property instead of keeping it non-strict" + ); + assert_descriptions_survive(&wire, "openai"); +} + +/// OpenRouter forwards to whichever upstream serves the model, so it must +/// satisfy the strictest: no top-level combiner and `properties` present on +/// object schemas (#446, #495). +#[test] +fn openrouter_sends_a_schema_its_strictest_upstream_accepts() { + let combiner_schema = serde_json::json!({ + "type": "object", + "properties": { "action": { "type": "string", "description": "what" } }, + "anyOf": [ + { "properties": { "label": { "type": "string" } }, "required": ["label"] }, + { "properties": { "target": { "type": "string" } } } + ] + }); + let normalized = + jcode_provider_openrouter::request::sanitize_tool_parameters_schema(&combiner_schema); + + assert!( + normalized.get("anyOf").is_none(), + "openrouter kept a top-level combiner: {normalized}" + ); + for name in ["action", "label", "target"] { + assert!( + normalized["properties"].get(name).is_some(), + "openrouter lost property `{name}`: {normalized}" + ); + } + // #446: a bare no-argument object schema must gain `properties`. + let bare = + jcode_provider_openrouter::request::sanitize_tool_parameters_schema(&serde_json::json!({ + "type": "object" + })); + assert_eq!(bare["properties"], serde_json::json!({})); +} + +/// Which provider request builders reach the dialect engine. +/// +/// A dialect in the registry that no provider executes is a sweep passing over +/// dead code, which is how OpenAI, OpenRouter and Anthropic ended up with +/// registry entries while still shipping their own older sanitizers. Pinning the +/// set turns the remaining migration into bounded, visible work: a provider +/// moving onto the engine fails this until the list is updated, and a provider +/// silently reverting off it fails too. +#[test] +fn provider_request_builders_that_reach_the_dialect_engine_are_pinned() { + // (source file, reaches the engine) + const BUILDERS: &[(&str, bool)] = &[ + ("../jcode-provider-gemini/src/lib.rs", true), + ("../jcode-provider-antigravity/src/lib.rs", true), + ("../jcode-provider-openrouter/src/request.rs", true), + ("../jcode-provider-anthropic/src/lib.rs", true), + // OpenAI's keyword subset now comes from the engine too. Strict + // eligibility and strict normalization stay in jcode-provider-core + // because they are OpenAI-specific and have no dialect equivalent, so + // the engine call lives there rather than in this request builder. + ("../jcode-provider-core/src/openai_schema.rs", true), + ("../jcode-provider-openai/src/request.rs", false), + ]; + + let manifest = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + let mut mismatches = Vec::new(); + + for (relative, should_use_engine) in BUILDERS { + let path = manifest.join(relative); + let source = std::fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("cannot read {}: {err}", path.display())); + // Ignore doc comments, so a file that only *mentions* the engine in + // prose is not counted as using it. + let uses_engine = source.lines().any(|line| { + let trimmed = line.trim_start(); + !trimmed.starts_with("//") && trimmed.contains("jcode_schema_dialect::") + }); + if uses_engine != *should_use_engine { + mismatches.push(format!( + "{relative}: pinned as engine={should_use_engine}, found engine={uses_engine}" + )); + } + } + + assert!( + mismatches.is_empty(), + "provider/engine wiring changed without updating this list:\n{}", + mismatches.join("\n") + ); +} + +/// Anthropic rejects a top-level combiner and requires an object schema with a +/// `properties` map (#495's sibling constraint). Now that it runs the shared +/// engine, its wire output needs the same behavioral pin as the others. +#[test] +fn anthropic_sends_a_schema_without_a_top_level_combiner() { + let combiner_tool = vec![ToolDefinition { + name: "multi_action".to_string(), + description: "probe".to_string(), + input_schema: serde_json::json!({ + "type": "object", + "properties": { "action": { "type": "string", "description": "what" } }, + "anyOf": [ + { "properties": { "label": { "type": "string" } }, "required": ["label"] }, + { "properties": { "target": { "type": "string" } } } + ] + }), + }]; + + let built = jcode_provider_anthropic::format_tools(&combiner_tool, false, false); + let wire = serde_json::to_value(&built).expect("serialize"); + let schema = &wire[0]["input_schema"]; + + for combiner in ["anyOf", "oneOf", "allOf"] { + assert!( + schema.get(combiner).is_none(), + "anthropic kept a top-level {combiner}: {schema}" + ); + } + // Every branch's fields are advertised, so the model can still call any + // action; runtime deserialization enforces which combination is valid. + for name in ["action", "label", "target"] { + assert!( + schema["properties"].get(name).is_some(), + "anthropic lost property `{name}`: {schema}" + ); + } + assert_eq!(schema["properties"]["action"]["description"], "what"); + // A branch-only requirement must not survive as a demand the merged object + // cannot express. + assert!( + schema + .get("required") + .and_then(|r| r.as_array()) + .is_none_or(|r| r.iter().all(|n| n.as_str() != Some("label"))), + "anthropic promoted an anyOf branch's requirement: {schema}" + ); + + // And a no-argument tool still gets the object shape Anthropic requires. + let bare = vec![ToolDefinition { + name: "noargs".to_string(), + description: "probe".to_string(), + input_schema: serde_json::json!({}), + }]; + let bare_wire = + serde_json::to_value(jcode_provider_anthropic::format_tools(&bare, false, false)) + .expect("serialize"); + assert_eq!(bare_wire[0]["input_schema"]["type"], "object"); + assert_eq!( + bare_wire[0]["input_schema"]["properties"], + serde_json::json!({}) + ); +} + +/// The property the whole system exists for: a keyword nobody has ever seen +/// cannot reach any provider. +/// +/// Every issue in this class began this way. Some MCP server emitted a construct +/// that was not on the relevant deny-list, it was forwarded verbatim, and the +/// provider 400d the entire tool catalog. A deny-list can only ever contain what +/// has already broken for somebody, so this test is the difference between the +/// fix and the system: it uses an invented keyword that appears in no list, no +/// issue, and no provider documentation. +#[test] +fn a_keyword_no_deny_list_has_ever_heard_of_reaches_no_provider() { + let novel = vec![ToolDefinition { + name: "mcp__future__probe".to_string(), + description: "probe".to_string(), + input_schema: serde_json::json!({ + "type": "object", + "properties": { + "x": { + "type": "string", + "description": "keep me", + "someKeywordFromADraftThatDoesNotExistYet": { "nested": true } + } + }, + "required": ["x"] + }), + }]; + const NOVEL: &str = "someKeywordFromADraftThatDoesNotExistYet"; + + let gemini = + serde_json::to_value(jcode_provider_gemini::build_tools(&novel).expect("gemini tools")) + .expect("serialize"); + assert!( + !contains_key(&gemini, NOVEL), + "gemini forwarded it: {gemini}" + ); + + let openai = serde_json::to_value(jcode_provider_openai::request::build_tools(&novel)) + .expect("serialize"); + assert!( + !contains_key(&openai, NOVEL), + "openai forwarded it: {openai}" + ); + + let anthropic = + serde_json::to_value(jcode_provider_anthropic::format_tools(&novel, false, false)) + .expect("serialize"); + assert!( + !contains_key(&anthropic, NOVEL), + "anthropic forwarded it: {anthropic}" + ); + + let openrouter = + jcode_provider_openrouter::request::sanitize_tool_parameters_schema(&novel[0].input_schema); + assert!( + !contains_key(&openrouter, NOVEL), + "openrouter forwarded it: {openrouter}" + ); + + for model in ["gemini-3-flash", "claude-sonnet-4-5", "gpt-oss-120b"] { + let antigravity = jcode_provider_antigravity::antigravity_compatible_schema( + &novel[0].input_schema, + model, + ); + assert!( + !contains_key(&antigravity, NOVEL), + "antigravity/{model} forwarded it: {antigravity}" + ); + } + + // Dropping the unknown keyword must not cost the tool its meaning. + assert_eq!( + gemini[0]["functionDeclarations"][0]["parameters"]["properties"]["x"]["description"], + "keep me" + ); + assert_eq!(openai[0]["parameters"]["properties"]["x"]["type"], "string"); +} diff --git a/crates/jcode-provider-gemini-runtime/tests/mcp_schema_end_to_end.rs b/crates/jcode-provider-gemini-runtime/tests/mcp_schema_end_to_end.rs index 0478fbfb58..eb5d65b087 100644 --- a/crates/jcode-provider-gemini-runtime/tests/mcp_schema_end_to_end.rs +++ b/crates/jcode-provider-gemini-runtime/tests/mcp_schema_end_to_end.rs @@ -58,9 +58,7 @@ fn playwright_tool_definitions() -> Vec { /// validator does when it reports "Unknown name X at ...". fn contains_key(value: &serde_json::Value, key: &str) -> bool { match value { - serde_json::Value::Object(map) => map - .iter() - .any(|(k, v)| k == key || contains_key(v, key)), + serde_json::Value::Object(map) => map.iter().any(|(k, v)| k == key || contains_key(v, key)), serde_json::Value::Array(items) => items.iter().any(|i| contains_key(i, key)), _ => false, } diff --git a/crates/jcode-provider-openai-runtime/Cargo.toml b/crates/jcode-provider-openai-runtime/Cargo.toml index f66881ef41..8a664cfdf5 100644 --- a/crates/jcode-provider-openai-runtime/Cargo.toml +++ b/crates/jcode-provider-openai-runtime/Cargo.toml @@ -20,6 +20,7 @@ futures = "0.3" jcode-base = { path = "../jcode-base", default-features = false } jcode-message-types = { path = "../jcode-message-types" } jcode-provider-core = { path = "../jcode-provider-core" } +jcode-schema-dialect = { path = "../jcode-schema-dialect" } jcode-provider-openai = { path = "../jcode-provider-openai" } reqwest = { version = "0.12", default-features = false, features = ["json", "stream", "charset", "http2", "system-proxy", "rustls-tls", "rustls-tls-native-roots"] } serde_json = { version = "1", features = ["raw_value"] } diff --git a/crates/jcode-provider-openai-runtime/src/openai_provider_impl.rs b/crates/jcode-provider-openai-runtime/src/openai_provider_impl.rs index 7fd1279c09..4dac502182 100644 --- a/crates/jcode-provider-openai-runtime/src/openai_provider_impl.rs +++ b/crates/jcode-provider-openai-runtime/src/openai_provider_impl.rs @@ -608,6 +608,27 @@ impl Provider for OpenAIProvider { ("elapsed_ms", elapsed_ms.to_string()), ], ); + // A tool schema OpenAI rejects fails every turn, not + // just this one, and one bad construct invalidates + // the whole catalog (#446, #543, #687, #711, #713). + // Learn what it refused so the user's next request + // omits it, instead of every request failing until + // a release adds the keyword to a list. Learning, + // not retrying: this loop owns its own retry and + // backoff, and a second retry inside it would double + // attempts against a possibly rate-limited endpoint. + let error = match jcode_schema_dialect::learn_from_error( + &error.to_string(), + &jcode_schema_dialect::registry::OPENAI, + ) { + Some(explanation) => { + jcode_base::logging::warn(&format!( + "OpenAI tool-schema rejection: {explanation}" + )); + error.context(explanation) + } + None => error, + }; let _ = tx.send(Err(error)).await; return; } diff --git a/crates/jcode-provider-openrouter/Cargo.toml b/crates/jcode-provider-openrouter/Cargo.toml index c7fee7ec57..d4f75ec917 100644 --- a/crates/jcode-provider-openrouter/Cargo.toml +++ b/crates/jcode-provider-openrouter/Cargo.toml @@ -4,6 +4,7 @@ version = "0.1.0" edition = "2024" [dependencies] +jcode-schema-dialect = { path = "../jcode-schema-dialect" } dirs = "5" serde = { version = "1", features = ["derive"] } serde_json = "1" diff --git a/crates/jcode-provider-openrouter/src/request.rs b/crates/jcode-provider-openrouter/src/request.rs index b3c7bf0d21..fc130b293e 100644 --- a/crates/jcode-provider-openrouter/src/request.rs +++ b/crates/jcode-provider-openrouter/src/request.rs @@ -4,167 +4,21 @@ use jcode_message_types::{ use serde_json::Value; use std::collections::{HashMap, HashSet}; -/// Normalize a tool `parameters` JSON schema for strict OpenAI-compatible -/// endpoints (issue #446). +/// Normalize a tool `parameters` JSON schema for whichever upstream OpenRouter +/// routes the model to. /// -/// Some backends (LM Studio being the prominent example) validate -/// `tools[].function.parameters` strictly and reject any object schema that -/// lacks a `properties` field with HTTP 400. MCP servers commonly declare -/// no-argument tools as a bare `{"type": "object"}`, and because the full tool -/// array is sent on every request, one such tool makes the provider unusable. +/// OpenRouter forwards to Anthropic, Vertex, Bedrock, LM Studio and others, so +/// it must satisfy the strictest of them: object schemas need a `properties` key +/// (LM Studio, #446) and top-level combiners are rejected by the +/// Anthropic-family backends (#495). /// -/// This recursively inserts an empty `properties: {}` into every -/// object-typed schema node that is missing it. The rewrite is semantically a -/// no-op per JSON Schema, so it is safe to apply for every OpenAI-compatible -/// endpoint rather than allow-listing strict ones. +/// The subset, the recursion, and the structural rewrites live in +/// `jcode-schema-dialect` so every provider shares one implementation and one +/// set of regression tests. Delegating here also strips constructs the previous +/// hand-written version forwarded (`propertyNames`, `uniqueItems`), which the +/// same upstreams reject when they reach them. pub fn sanitize_tool_parameters_schema(schema: &Value) -> Value { - fn walk(node: &mut Value) { - let Some(obj) = node.as_object_mut() else { - if let Some(items) = node.as_array_mut() { - for item in items { - walk(item); - } - } - return; - }; - - let is_object_type = match obj.get("type") { - Some(Value::String(ty)) => ty == "object", - Some(Value::Array(types)) => types.iter().any(|ty| ty.as_str() == Some("object")), - _ => false, - }; - if is_object_type { - obj.entry("properties") - .or_insert_with(|| Value::Object(serde_json::Map::new())); - } - - for (key, value) in obj.iter_mut() { - match key.as_str() { - // Schema maps: each value is a schema. - "properties" | "patternProperties" | "$defs" | "definitions" => { - if let Some(map) = value.as_object_mut() { - for sub in map.values_mut() { - walk(sub); - } - } - } - // Direct sub-schemas (or arrays of schemas). - "items" - | "additionalProperties" - | "anyOf" - | "oneOf" - | "allOf" - | "not" - | "if" - | "then" - | "else" - | "prefixItems" - | "contains" => walk(value), - _ => {} - } - } - } - - // A bare `{}` / non-object parameters value is also rejected by strict - // validators; OpenAI's spec models "no parameters" as an empty object - // schema. - let mut sanitized = if schema.is_object() { - schema.clone() - } else { - serde_json::json!({ "type": "object" }) - }; - if let Some(obj) = sanitized.as_object_mut() - && obj.is_empty() - { - obj.insert("type".to_string(), Value::String("object".to_string())); - } - flatten_top_level_combinators(&mut sanitized); - walk(&mut sanitized); - sanitized -} - -/// Flatten `oneOf`/`anyOf`/`allOf` at the top level of a tool parameters -/// schema into a single object schema (issue #495). -/// -/// OpenRouter forwards tool schemas to whichever upstream serves the model, -/// and Anthropic-family backends (Anthropic, Google Vertex, Amazon Bedrock) -/// reject `input_schema` combinators at the top level with HTTP 400 -/// ("input_schema does not support oneOf, allOf, or anyOf at the top level"). -/// One such tool bricks every request, so first-time OpenRouter logins fail on -/// their first message when the registry contains a multi-action tool that -/// models its action branches with top-level `anyOf`. -/// -/// Mirror the direct Anthropic provider's `anthropic_input_schema`: keep the -/// common object shape, merge branch `properties` in as optional fields, and -/// only promote `required` from `allOf` branches (whose constraints all -/// apply). Runtime tool deserialization remains the authority for -/// action-specific constraints. Nested combinators inside properties are left -/// untouched; upstreams accept those. -fn flatten_top_level_combinators(schema: &mut Value) { - let Some(output) = schema.as_object_mut() else { - return; - }; - - let mut merged_properties = output - .get("properties") - .and_then(Value::as_object) - .cloned() - .unwrap_or_default(); - let mut all_of_required = Vec::new(); - let mut saw_combinator = false; - - for keyword in ["oneOf", "anyOf", "allOf"] { - let Some(branches) = output - .remove(keyword) - .and_then(|value| value.as_array().cloned()) - else { - continue; - }; - saw_combinator = true; - for branch in branches { - let Some(branch) = branch.as_object() else { - continue; - }; - if let Some(properties) = branch.get("properties").and_then(Value::as_object) { - for (name, property) in properties { - merged_properties - .entry(name.clone()) - .or_insert_with(|| property.clone()); - } - } - if keyword == "allOf" - && let Some(required) = branch.get("required").and_then(Value::as_array) - { - for name in required.iter().filter_map(Value::as_str) { - if !all_of_required.iter().any(|existing| existing == name) { - all_of_required.push(name.to_string()); - } - } - } - } - } - - if !saw_combinator { - return; - } - - output.insert("type".to_string(), Value::String("object".to_string())); - output.insert("properties".to_string(), Value::Object(merged_properties)); - if !all_of_required.is_empty() { - let required = output - .entry("required".to_string()) - .or_insert_with(|| Value::Array(Vec::new())); - if let Value::Array(required) = required { - for name in all_of_required { - if !required - .iter() - .any(|existing| existing.as_str() == Some(&name)) - { - required.push(Value::String(name)); - } - } - } - } + jcode_schema_dialect::normalize(schema, &jcode_schema_dialect::registry::OPENROUTER) } /// Build OpenAI-compatible chat `messages` for OpenRouter/direct compatible providers. diff --git a/crates/jcode-render-core/src/markdown.rs b/crates/jcode-render-core/src/markdown.rs index 8590122ad4..9925fe66f5 100644 --- a/crates/jcode-render-core/src/markdown.rs +++ b/crates/jcode-render-core/src/markdown.rs @@ -451,6 +451,7 @@ pub fn parse_markdown(text: &str) -> Document { } spans.push(StyledSpan { text: t.to_string(), + latex: None, role: style.role(), fill: FillRole::None, attrs: style.attrs(), @@ -466,6 +467,7 @@ pub fn parse_markdown(text: &str) -> Document { } spans.push(StyledSpan { text: t.to_string(), + latex: None, role: StyleRole::Code, fill: FillRole::Code, attrs: TextAttrs::none(), @@ -481,6 +483,7 @@ pub fn parse_markdown(text: &str) -> Document { } spans.push(StyledSpan { text: crate::math::render_inline_latex(&math), + latex: Some(math.to_string()), role: StyleRole::Math, fill: FillRole::None, attrs: TextAttrs::none(), @@ -559,6 +562,7 @@ pub fn parse_markdown(text: &str) -> Document { } else { spans.push(StyledSpan { text: raw.to_string(), + latex: None, role: StyleRole::Html, fill: FillRole::None, attrs: TextAttrs { @@ -654,6 +658,7 @@ pub fn parse_markdown(text: &str) -> Document { .map(|l| { StyledLine::from_spans(vec![StyledSpan { text: l.to_string(), + latex: None, role: StyleRole::Code, fill: FillRole::Code, attrs: TextAttrs::none(), diff --git a/crates/jcode-render-core/src/model.rs b/crates/jcode-render-core/src/model.rs index 2dbbca2d28..4c5bcecb19 100644 --- a/crates/jcode-render-core/src/model.rs +++ b/crates/jcode-render-core/src/model.rs @@ -67,6 +67,10 @@ impl TextAttrs { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct StyledSpan { pub text: String, + /// Original TeX for an inline-math span. Text front-ends render `text` as a + /// readable fallback; graphical front-ends use this source for native math. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub latex: Option, pub role: StyleRole, pub fill: FillRole, pub attrs: TextAttrs, @@ -76,6 +80,7 @@ impl StyledSpan { pub fn new(text: impl Into, role: StyleRole) -> Self { Self { text: text.into(), + latex: None, role, fill: FillRole::None, attrs: TextAttrs::none(), diff --git a/crates/jcode-render-core/src/tests.rs b/crates/jcode-render-core/src/tests.rs index 4a2d308a01..fab664e001 100644 --- a/crates/jcode-render-core/src/tests.rs +++ b/crates/jcode-render-core/src/tests.rs @@ -49,6 +49,7 @@ fn parses_inline_math_into_unicode_math_span() { .find(|span| span.role == StyleRole::Math) .expect("math span"); assert_eq!(math.text, "e^(iπ) + 1 = 0"); + assert_eq!(math.latex.as_deref(), Some(r"e^{i\pi} + 1 = 0")); } #[test] diff --git a/crates/jcode-render-core/src/wrap.rs b/crates/jcode-render-core/src/wrap.rs index 902df3fd9b..1477709159 100644 --- a/crates/jcode-render-core/src/wrap.rs +++ b/crates/jcode-render-core/src/wrap.rs @@ -152,12 +152,14 @@ fn push_token(cur: &mut Vec, src: &StyledSpan, text: &str) { && last.role == src.role && last.fill == src.fill && last.attrs == src.attrs + && last.latex == src.latex { last.text.push_str(text); return; } cur.push(StyledSpan { text: text.to_string(), + latex: src.latex.clone(), role: src.role, fill: src.fill, attrs: src.attrs, diff --git a/crates/jcode-schema-dialect/Cargo.toml b/crates/jcode-schema-dialect/Cargo.toml index 1472d73b7f..21caa93997 100644 --- a/crates/jcode-schema-dialect/Cargo.toml +++ b/crates/jcode-schema-dialect/Cargo.toml @@ -13,5 +13,12 @@ dirs = "5" serde = { version = "1", features = ["derive"] } serde_json = "1" +[features] +# Exposes the per-thread quirk-store redirect so integration tests (which see +# the crate as an external dependency) can isolate themselves from the real +# ~/.jcode store. +test-support = [] + [dev-dependencies] tempfile = "3" +jcode-schema-dialect = { path = ".", features = ["test-support"] } diff --git a/crates/jcode-schema-dialect/src/conformance.rs b/crates/jcode-schema-dialect/src/conformance.rs index 0effe169d6..b9de188022 100644 --- a/crates/jcode-schema-dialect/src/conformance.rs +++ b/crates/jcode-schema-dialect/src/conformance.rs @@ -37,9 +37,19 @@ pub fn untyped_properties(schema: &Value) -> Vec { let Some(map) = schema.as_object() else { return schema.is_boolean(); }; - ["type", "enum", "const", "anyOf", "oneOf", "allOf", "$ref", "properties", "items"] - .iter() - .any(|keyword| map.contains_key(*keyword)) + [ + "type", + "enum", + "const", + "anyOf", + "oneOf", + "allOf", + "$ref", + "properties", + "items", + ] + .iter() + .any(|keyword| map.contains_key(*keyword)) } fn walk(schema: &Value, path: &str, errors: &mut Vec) { diff --git a/crates/jcode-schema-dialect/src/dialect.rs b/crates/jcode-schema-dialect/src/dialect.rs index d40cfe7f92..d67fa6d43f 100644 --- a/crates/jcode-schema-dialect/src/dialect.rs +++ b/crates/jcode-schema-dialect/src/dialect.rs @@ -53,6 +53,15 @@ pub struct DialectTransforms { pub const_as_enum: bool, /// Rewrite `oneOf` as `anyOf` for dialects that model only `anyOf`. pub one_of_as_any_of: bool, + /// Merge each `allOf` branch's `properties` into the enclosing object and + /// drop the `allOf`. + /// + /// Distinct from [`Self::flatten_top_level_combiners`], which widens + /// `anyOf`/`oneOf` alternatives at the root only. `allOf` branches all + /// apply simultaneously, so merging them is lossless, and it applies at any + /// depth. Needed by validators that accept `allOf` syntactically without + /// intersecting it. + pub merge_all_of_branches: bool, } /// A provider's accepted JSON Schema subset plus the rewrites needed to reach @@ -236,6 +245,9 @@ fn walk(schema: &Value, spec: &DialectSpec, quirks: &LearnedQuirks) -> Value { if spec.transforms.prune_dangling_required { prune_dangling_required(&mut out); } + if spec.transforms.merge_all_of_branches { + merge_all_of_branches(&mut out); + } if spec.transforms.require_properties_on_objects && is_object_typed(&out) { out.entry("properties".to_string()) .or_insert_with(|| Value::Object(Map::new())); @@ -430,3 +442,82 @@ fn flatten_all_combiners(schema: &Value) -> Value { _ => schema.clone(), } } + +/// Merge each `allOf` branch into the enclosing object and drop the `allOf`. +/// +/// Lossless in principle: `allOf` branches all apply at once, so their +/// properties and requirements are simply the object's. Needed by validators +/// that accept `allOf` syntactically without intersecting it, which otherwise +/// see a tool with no properties at all. +/// +/// Only object-shaped branches are merged. A branch expressing something else +/// (a bare `{"minLength": 1}` on a string, say) has nothing to contribute to a +/// property map, and dropping it only widens what the model may send while the +/// tool still validates the real call. +fn merge_all_of_branches(out: &mut Map) { + let Some(Value::Array(branches)) = out.remove("allOf") else { + return; + }; + + let mut properties = out + .get("properties") + .and_then(Value::as_object) + .cloned() + .unwrap_or_default(); + let mut required: Vec = out + .get("required") + .and_then(Value::as_array) + .map(|names| { + names + .iter() + .filter_map(Value::as_str) + .map(ToString::to_string) + .collect() + }) + .unwrap_or_default(); + + for branch in &branches { + let Some(branch) = branch.as_object() else { + continue; + }; + // Adopt the branch's `type` when the enclosing object declares none. + // A schema whose only `type` lives in its `allOf` branches becomes + // typeless once the `allOf` is gone, and a typeless tool schema is + // rejected outright. + if !out.contains_key("type") + && let Some(branch_type) = branch.get("type") + { + out.insert("type".to_string(), branch_type.clone()); + } + if let Some(branch_properties) = branch.get("properties").and_then(Value::as_object) { + for (name, property) in branch_properties { + // The enclosing object's own declaration wins: it is the more + // specific one, and a branch usually only narrows. + properties + .entry(name.clone()) + .or_insert_with(|| property.clone()); + } + } + if let Some(branch_required) = branch.get("required").and_then(Value::as_array) { + for name in branch_required.iter().filter_map(Value::as_str) { + if !required.iter().any(|existing| existing == name) { + required.push(name.to_string()); + } + } + } + } + + if !properties.is_empty() { + out.insert("properties".to_string(), Value::Object(properties)); + } + if required.is_empty() { + out.remove("required"); + } else { + out.insert( + "required".to_string(), + Value::Array(required.into_iter().map(Value::String).collect()), + ); + } + // A branch may have required a name no branch declared. + prune_dangling_required(out); +} diff --git a/crates/jcode-schema-dialect/src/lib.rs b/crates/jcode-schema-dialect/src/lib.rs index 565d06991b..df7e28586a 100644 --- a/crates/jcode-schema-dialect/src/lib.rs +++ b/crates/jcode-schema-dialect/src/lib.rs @@ -165,6 +165,94 @@ pub fn recover_from_error(message: &str, spec: &DialectSpec) -> RecoveryAction { } } +/// Learn from a rejection without retrying, returning a message to show the +/// user in place of the raw provider error. +/// +/// Some request paths cannot safely re-send a turn: OpenAI's streaming loop +/// owns its own retry and backoff machinery, and threading a second retry +/// through it risks doubling attempts against a rate-limited endpoint. But the +/// expensive half of recovery is *learning*, not retrying. Recording the +/// construct here means the user's next request already omits it, so a schema +/// rejection costs one failed turn instead of every turn until a release. +/// +/// Returns `None` when the error is not about tool schemas, so callers can use +/// this as a pass-through predicate. +pub fn learn_from_error(message: &str, spec: &DialectSpec) -> Option { + match recover_from_error(message, spec) { + RecoveryAction::NotSchemaRelated => None, + RecoveryAction::RetryWithoutConstruct { description } => Some(format!( + "{description}. This request failed, but the next one will not send it.", + )), + RecoveryAction::Unrecoverable { hint } => Some(hint), + } +} + +#[cfg(test)] +mod learn_tests { + use super::*; + + fn isolated(name: &str) -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + quirks::use_test_path(dir.path().join(format!("{name}.json"))); + dir + } + + /// The two OpenAI rejections that name a construct must be learned, so the + /// user's next request omits it even though this path cannot retry. + #[test] + fn openai_rejections_that_name_a_construct_are_learned() { + let _dir = isolated("learn-openai"); + + let format_rejection = "invalid_request_error (invalid_function_parameters): Invalid schema for function 'mcp__firecrawl__firecrawl_map': In context=('properties', 'url'), 'uri' is not a valid format."; + let message = learn_from_error(format_rejection, ®istry::OPENAI).expect("recognized"); + assert!(message.contains("uri"), "{message}"); + assert!( + quirks::learned_for("openai") + .rejected_formats + .iter() + .any(|f| f == "uri"), + "the format must be remembered for the next request" + ); + + let keyword_rejection = "invalid_request_error (invalid_function_parameters): Invalid schema for function 'mcp__x__y': In context=('properties', 'ids'), 'uniqueItems' is not permitted."; + let message = learn_from_error(keyword_rejection, ®istry::OPENAI).expect("recognized"); + assert!(message.contains("uniqueItems"), "{message}"); + } + + /// #713's "must have a 'type' key" names nothing strippable, so it must be + /// labelled as a schema problem rather than either retried or passed + /// through as an opaque 400. + #[test] + fn a_structural_openai_rejection_is_labelled_not_retried() { + let _dir = isolated("learn-structural"); + let structural = "invalid_request_error (invalid_function_parameters): Invalid schema for function 'mcp__cua__set_config': In context=('properties','value'), schema must have a 'type' key."; + let message = learn_from_error(structural, ®istry::OPENAI).expect("recognized"); + assert!( + message.contains("schema"), + "the user needs to know this is a tool-schema problem: {message}" + ); + assert!( + quirks::learned_for("openai").is_empty(), + "nothing is strippable here, so nothing must be recorded" + ); + } + + #[test] + fn operational_failures_pass_straight_through() { + let _dir = isolated("learn-operational"); + for message in [ + "HTTP 429 Too Many Requests", + "connection reset by peer", + "HTTP 500 internal error", + ] { + assert!( + learn_from_error(message, ®istry::OPENAI).is_none(), + "misread an operational failure as a schema problem: {message}" + ); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -358,7 +446,6 @@ mod tests { recover_from_error(message, ®istry::OPENAI), RecoveryAction::Unrecoverable { .. } )); - } #[test] @@ -390,7 +477,6 @@ mod tests { "minItems" )); let _ = schema; - } #[test] diff --git a/crates/jcode-schema-dialect/src/quirks.rs b/crates/jcode-schema-dialect/src/quirks.rs index e24b895df0..9d1705e685 100644 --- a/crates/jcode-schema-dialect/src/quirks.rs +++ b/crates/jcode-schema-dialect/src/quirks.rs @@ -33,28 +33,43 @@ fn store_path() -> Option { if let Some(override_path) = test_override() { return Some(override_path); } - if let Ok(explicit) = std::env::var("JCODE_SCHEMA_QUIRKS_PATH") { - return Some(PathBuf::from(explicit)); + // A test that forgot to isolate itself must not read or write the real + // store. This is not hypothetical: an earlier version of the test hook used + // a process-global env var, and because tests run in parallel one of them + // observed it unset and persisted a learned `minItems` rejection into the + // developer's real `~/.jcode/schema-quirks.json`, silently stripping that + // keyword from every OpenAI request on that machine afterwards. Failing + // closed here makes the whole class impossible rather than relying on every + // future test remembering to isolate. + #[cfg(any(test, feature = "test-support"))] + { + None + } + #[cfg(not(any(test, feature = "test-support")))] + { + if let Ok(explicit) = std::env::var("JCODE_SCHEMA_QUIRKS_PATH") { + return Some(PathBuf::from(explicit)); + } + let home = if let Ok(jcode_home) = std::env::var("JCODE_HOME") { + PathBuf::from(jcode_home) + } else { + dirs::home_dir()?.join(".jcode") + }; + Some(home.join("schema-quirks.json")) } - let home = if let Ok(jcode_home) = std::env::var("JCODE_HOME") { - PathBuf::from(jcode_home) - } else { - dirs::home_dir()?.join(".jcode") - }; - Some(home.join("schema-quirks.json")) } -#[cfg(test)] +#[cfg(any(test, feature = "test-support"))] thread_local! { static TEST_PATH: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; } -#[cfg(test)] +#[cfg(any(test, feature = "test-support"))] fn test_override() -> Option { TEST_PATH.with(|path| path.borrow().clone()) } -#[cfg(not(test))] +#[cfg(not(any(test, feature = "test-support")))] fn test_override() -> Option { None } @@ -63,7 +78,12 @@ fn test_override() -> Option { /// store is otherwise process-global, so redirecting per thread is what keeps /// them from racing each other (and avoids mutating process env, which is /// `unsafe` in edition 2024). -#[cfg(test)] +/// +/// Gated behind `test-support` as well as `cfg(test)` so integration tests, +/// which compile against the crate as an external dependency, can isolate +/// themselves too. Without that an integration test would read and write the +/// developer's real `~/.jcode/schema-quirks.json`. +#[cfg(any(test, feature = "test-support"))] pub fn use_test_path(path: PathBuf) { TEST_PATH.with(|slot| *slot.borrow_mut() = Some(path)); reset_cache_for_tests(); diff --git a/crates/jcode-schema-dialect/src/registry.rs b/crates/jcode-schema-dialect/src/registry.rs index 8b5c9196bd..4e33075a7c 100644 --- a/crates/jcode-schema-dialect/src/registry.rs +++ b/crates/jcode-schema-dialect/src/registry.rs @@ -55,6 +55,13 @@ pub const OPENAI: DialectSpec = DialectSpec { ], transforms: DialectTransforms { one_of_as_any_of: true, + // OpenAI accepts `allOf` but does not intersect its branches the way + // the spec requires, so a schema whose properties live only in `allOf` + // branches ends up advertising none of them. The sanitizer this + // replaces merged them into the parent object, so the dialect must too + // or migrating onto it would silently empty those tools (caught by + // diffing the two before switching). + merge_all_of_branches: true, ..DEFAULT_TRANSFORMS }, }; @@ -137,6 +144,12 @@ pub const ANTHROPIC: DialectSpec = DialectSpec { supported_string_formats: &[], transforms: DialectTransforms { flatten_top_level_combiners: true, + // Anthropic's `input_schema` must be an object schema with a + // `properties` map: the API rejects a bare `{"type":"object"}` and a + // non-object schema outright. The provider's own sanitizer guaranteed + // both, so the dialect has to as well or migrating onto it would be a + // regression (caught by diffing the two before switching). + require_properties_on_objects: true, ..DEFAULT_TRANSFORMS }, }; @@ -214,6 +227,7 @@ const DEFAULT_TRANSFORMS: DialectTransforms = DialectTransforms { prune_dangling_required: false, const_as_enum: false, one_of_as_any_of: false, + merge_all_of_branches: false, }; /// Every registered dialect, for conformance sweeps. diff --git a/crates/jcode-schema-dialect/src/rejection.rs b/crates/jcode-schema-dialect/src/rejection.rs index 7ace23612b..28f9533620 100644 --- a/crates/jcode-schema-dialect/src/rejection.rs +++ b/crates/jcode-schema-dialect/src/rejection.rs @@ -93,6 +93,20 @@ pub fn classify(message: &str) -> Option { }); } + // OpenAI (#713): "schema must have a 'type' key". Structural: nothing can be + // stripped to fix it, so it is reported as a recognized-but-unactionable + // schema error. That labels the failure for the user instead of surfacing a + // raw 400, and keeps the caller from retrying a byte-identical request. + // Prevention already handles this by declining strict mode, so a rejection + // reaching here means a schema jcode did not expect. + if message.contains("must have a 'type' key") || message.contains("is not of type") { + return Some(SchemaRejection { + keywords: Vec::new(), + format: None, + tool, + }); + } + // Anthropic / OpenRouter (#495) and the Antigravity Claude bridge. if message.contains("does not support oneOf, allOf, or anyOf") || (message.contains("input_schema") && message.contains("JSON Schema draft 2020-12")) @@ -219,10 +233,7 @@ mod tests { #[test] fn parses_the_real_gemini_dangling_required_400() { let message = "GenerateContentRequest.tools[0].function_declarations[3].parameters: required fields ['label'] are not defined in the schema properties"; - assert_eq!( - classify(message).unwrap().keyword(), - Some("required") - ); + assert_eq!(classify(message).unwrap().keyword(), Some("required")); } #[test] @@ -290,5 +301,4 @@ mod tests { "both violations must be learned from one response, deduplicated" ); } - } diff --git a/crates/jcode-schema-dialect/tests/recovery_coverage.rs b/crates/jcode-schema-dialect/tests/recovery_coverage.rs new file mode 100644 index 0000000000..353351efa7 --- /dev/null +++ b/crates/jcode-schema-dialect/tests/recovery_coverage.rs @@ -0,0 +1,228 @@ +//! Does the recovery layer actually cover the providers that need it? +//! +//! The live experiment that closed this loop exercised exactly one route +//! (Antigravity + Gemini). That proves the mechanism works, not that it is +//! *wired* everywhere it is needed, and the recovery code was written before any +//! live loop existed to check it. This runs the same question over every +//! provider dialect at once so a route without recovery is a failing test +//! instead of a discovery during an outage. + +use jcode_schema_dialect::{RecoveryAction, quirks, registry}; + +/// Providers whose runtime calls `recover_from_error`, so a construct the +/// provider rejects is learned and retried instead of failing the turn. +/// +/// Kept as data next to the assertion below rather than as prose in a commit +/// message: adding a dialect without recovery should require deliberately +/// editing this list, which is the moment to ask whether that route can 400 on +/// a schema. +const DIALECTS_WITH_RUNTIME_RECOVERY: &[&str] = &[ + "gemini", + "antigravity-claude", + "antigravity-bridge", + "openai", +]; + +/// Dialects deliberately without runtime recovery, and why. +/// +/// OpenAI-family routes reject schemas via a *validation* error naming the +/// construct, which prevention already strips, and their historical failures +/// (#446, #543, #687, #711, #713) were all fixed by not claiming `strict` +/// rather than by retrying. Wiring recovery there is still worthwhile, but it +/// is a separate change with its own live verification, so it is recorded as a +/// known gap instead of being silently absent. +const DIALECTS_WITHOUT_RUNTIME_RECOVERY: &[&str] = &["openrouter", "anthropic"]; + +#[test] +fn every_dialect_is_accounted_for_as_having_recovery_or_not() { + let mut registered: Vec<&str> = registry::ALL.iter().map(|spec| spec.id).collect(); + registered.sort_unstable(); + + let mut accounted: Vec<&str> = DIALECTS_WITH_RUNTIME_RECOVERY + .iter() + .chain(DIALECTS_WITHOUT_RUNTIME_RECOVERY) + .copied() + .collect(); + accounted.sort_unstable(); + + assert_eq!( + registered, accounted, + "a dialect was added or removed without deciding whether its runtime \ + recovers from a schema rejection" + ); +} + +/// The classifier must produce an actionable, non-looping recovery for every +/// dialect that has runtime recovery wired. +/// +/// A dialect whose `recover_from_error` returns `NotSchemaRelated` for a real +/// rejection would make the retry dead code on that route, which is exactly the +/// failure the single-route live test could not see. +#[test] +fn each_recovering_dialect_learns_retries_once_then_refuses_to_loop() { + for id in DIALECTS_WITH_RUNTIME_RECOVERY { + let spec = registry::by_id(id).expect("registered dialect"); + // Isolate per dialect so one case cannot consume another's "new + // information" signal. + let dir = tempfile::tempdir().expect("tempdir"); + quirks::use_test_path(dir.path().join(format!("{id}.json"))); + + // The real shape these routes return, from issue #754. + let rejection = format!( + "{} generateContent failed (HTTP 400 Bad Request): {{\"error\":{{\"code\":400,\ + \"message\":\"Invalid JSON payload received. Unknown name \\\"vendorThing\\\" at \ + 'request.tools[0].function_declarations[3].parameters.properties[0].value': Cannot \ + find field.\",\"status\":\"INVALID_ARGUMENT\"}}}}", + id + ); + + match jcode_schema_dialect::recover_from_error(&rejection, spec) { + RecoveryAction::RetryWithoutConstruct { description } => assert!( + description.contains("vendorThing"), + "dialect `{id}` must name what it learned: {description}" + ), + other => panic!("dialect `{id}` cannot recover from a real rejection: {other:?}"), + } + + // Learned, so normalization now strips it without a request. + assert!( + quirks::learned_for(id) + .rejected_keywords + .iter() + .any(|k| k == "vendorThing"), + "dialect `{id}` did not persist what it learned" + ); + + // And the same rejection again must not retry forever. + assert!( + matches!( + jcode_schema_dialect::recover_from_error(&rejection, spec), + RecoveryAction::Unrecoverable { .. } + ), + "dialect `{id}` would retry the same construct in a loop" + ); + } +} + +/// Recovery must never absorb a rejection that names something load-bearing. +/// +/// Stripping `type` or `properties` to make a request succeed would leave the +/// model calling a tool whose real shape it was never told, which is worse than +/// the 400 it replaced. Checked for every dialect, recovering or not. +#[test] +fn no_dialect_absorbs_a_load_bearing_rejection() { + for spec in registry::ALL { + let dir = tempfile::tempdir().expect("tempdir"); + quirks::use_test_path(dir.path().join(format!("{}-lb.json", spec.id))); + + let rejection = "GenerateContentRequest.tools[0].function_declarations[3].parameters: \ + required fields ['label'] are not defined in the schema properties"; + assert!( + matches!( + jcode_schema_dialect::recover_from_error(rejection, spec), + RecoveryAction::Unrecoverable { .. } + ), + "dialect `{}` would strip a load-bearing keyword to force a success", + spec.id + ); + } +} + +/// A transient provider failure must not be mistaken for a schema problem, or +/// recovery would swallow a rate limit and retry it as if a keyword were at +/// fault. Checked for every dialect. +#[test] +fn no_dialect_treats_an_operational_failure_as_a_schema_rejection() { + let operational = [ + "HTTP 429 Too Many Requests", + "HTTP 503 upstream unavailable", + "connection reset by peer", + "Function call is missing a thought_signature in functionCall parts", + "Antigravity generateContent failed (HTTP 400 Bad Request): Request contains an invalid argument", + ]; + for spec in registry::ALL { + for message in operational { + assert_eq!( + jcode_schema_dialect::recover_from_error(message, spec), + RecoveryAction::NotSchemaRelated, + "dialect `{}` misread an operational failure as a schema rejection: {message}", + spec.id + ); + } + } +} + +/// Runtimes that own a provider request path, and whether each is expected to +/// recover from a schema rejection. +/// +/// Per-runtime rather than per-dialect, because dialects are shared: the +/// Antigravity runtime dispatches to the `gemini` dialect too, so a +/// dialect-keyed check stays green even when the *native* Gemini route loses its +/// recovery. That flaw was found by unwiring the native route and watching an +/// earlier version of this test pass anyway. +const RUNTIME_RECOVERY_EXPECTATIONS: &[(&str, bool)] = &[ + ("../jcode-provider-gemini-runtime/src/lib.rs", true), + ("../jcode-provider-antigravity-runtime/src/lib.rs", true), + // Learns without retrying: this path owns its own retry and backoff, so a + // second retry inside it would double attempts against a possibly + // rate-limited endpoint. Learning still turns "every request fails until a + // release" into "one request fails". + ( + "../jcode-provider-openai-runtime/src/openai_provider_impl.rs", + true, + ), + // Still unhandled. Both forward to upstreams whose rejection texts jcode has + // never captured, so there is nothing to write a classifier against yet; + // inventing patterns would produce a check that cannot fail. Prevention + // covers them (their wire output is pinned by + // `every_provider_sends_clean_schemas`), so this is a real but bounded gap. + ("../jcode-provider-openrouter-runtime/src/lib.rs", false), + ("../jcode-provider-anthropic-runtime/src/lib.rs", false), +]; + +/// Every runtime's recovery wiring must match what this file claims. +/// +/// A hand-kept list of "routes that recover" goes stale the first time someone +/// adds or refactors a provider, and its staleness is invisible: the classifier +/// tests keep passing because they never touch the wiring. Reading the runtime +/// sources ties the claim to the code. +#[test] +fn each_runtime_recovery_wiring_matches_what_is_claimed() { + let manifest = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + let mut mismatches: Vec = Vec::new(); + + for (relative, should_recover) in RUNTIME_RECOVERY_EXPECTATIONS { + let path = manifest.join(relative); + let source = std::fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("cannot read runtime source {}: {err}", path.display())); + + // Count only real calls, not the mentions in doc comments explaining + // them, or an unwired runtime whose comment survived would pass. + let calls = source + .lines() + .filter(|line| { + let trimmed = line.trim_start(); + // Either mechanism counts: `recover_from_error` retries the + // turn, `learn_from_error` only records for the next one. Both + // end the "fails until a release" behavior, which is what this + // check is about. + !trimmed.starts_with("//") + && (trimmed.contains("recover_from_error(") + || trimmed.contains("learn_from_error(")) + }) + .count(); + + let recovers = calls > 0; + if recovers != *should_recover { + mismatches.push(format!( + "{relative}: claimed recovery={should_recover}, found {calls} call(s)" + )); + } + } + + assert!( + mismatches.is_empty(), + "runtime recovery wiring disagrees with this file's claims:\n{}", + mismatches.join("\n") + ); +} diff --git a/crates/jcode-tui/src/tui/app/commands_remote.rs b/crates/jcode-tui/src/tui/app/commands_remote.rs index 95beb0448a..541b8389ab 100644 --- a/crates/jcode-tui/src/tui/app/commands_remote.rs +++ b/crates/jcode-tui/src/tui/app/commands_remote.rs @@ -5,19 +5,23 @@ use crate::gateway::control::{ }; const REMOTE_HELP: &str = "\ -**`/remote`** - reach this session from another machine +**`/remote`** - use jcode from any device -The gateway lets another computer or phone drive this jcode server over the -network, using the same protocol the local UI speaks. +`/remote` opens your Jcode account, the entry point for Jcode Cloud access. +Cloud is being bundled with eligible Jcode subscriptions. The managed host +control plane is currently in early access. -- `/remote` or `/remote status` - gateway state, dial address, paired devices +- `/remote` or `/remote cloud` - activate or manage Jcode Cloud +- `/remote status` - local gateway state, dial address, paired devices - `/remote on` / `/remote off` - enable or disable the gateway - `/remote pair` - show a pairing code and QR for a new device - `/remote revoke ` - remove a paired device -Setup is: `/remote on`, restart the server, then `/remote pair`. +For self-hosting, run `/remote on`, restart the server, then `/remote pair`. "; +const CLOUD_ACCOUNT_URL: &str = "https://jcode.sh/account"; + pub(super) fn handle_remote_command(app: &mut App, trimmed: &str) -> bool { let Some(parsed) = parse_remote_command(trimmed) else { return false; @@ -29,6 +33,7 @@ pub(super) fn handle_remote_command(app: &mut App, trimmed: &str) -> bool { app.push_display_message(DisplayMessage::system(REMOTE_HELP.to_string())); app.set_status_notice("Remote help"); } + Ok(RemoteCommand::Cloud) => activate_cloud(app), Ok(RemoteCommand::Status) => show_status(app), Ok(RemoteCommand::On) => toggle(app, true), Ok(RemoteCommand::Off) => toggle(app, false), @@ -39,6 +44,38 @@ pub(super) fn handle_remote_command(app: &mut App, trimmed: &str) -> bool { true } +fn activate_cloud(app: &mut App) { + let signed_in = crate::subscription_catalog::has_credentials(); + let tier = crate::subscription_catalog::cached_tier(); + let account_line = if signed_in { + match tier { + Some(tier) => format!("Signed in on the **{}** plan.", tier.display_name()), + None => "Signed in to your Jcode account.".to_string(), + } + } else { + "Not signed in yet. The account page will guide you through passwordless sign-in and subscription setup." + .to_string() + }; + + let opened = super::helpers::open_path_or_url_detached(CLOUD_ACCOUNT_URL).is_ok(); + let open_line = if opened { + "Opened your secure Jcode account page in the browser." + } else { + "Open your secure Jcode account page:" + }; + app.push_display_message(DisplayMessage::system(format!( + "**Jcode Cloud early access**\n\n{account_line}\n\n{open_line}\n\n{CLOUD_ACCOUNT_URL}\n\n\ + The managed-host control plane is not generally available yet. Reference hosts wake on \ + demand and stop when idle. To use your own machine now, run \ + `/remote on`, restart the server, then `/remote pair`." + ))); + app.set_status_notice(if opened { + "Jcode Cloud account opened" + } else { + "Jcode Cloud account link ready" + }); +} + fn show_status(app: &mut App) { let status = RemoteStatus::load(); app.push_display_message(DisplayMessage::system(status.to_markdown())); @@ -163,7 +200,7 @@ mod tests { assert_eq!(parse_remote_command("/remote-release"), None); assert_eq!( parse_remote_command("/remote"), - Some(Ok(RemoteCommand::Status)) + Some(Ok(RemoteCommand::Cloud)) ); } @@ -171,7 +208,7 @@ mod tests { /// subcommands the parser actually accepts. #[test] fn help_documents_every_supported_subcommand() { - for sub in ["status", "on", "off", "pair", "revoke"] { + for sub in ["cloud", "status", "on", "off", "pair", "revoke"] { assert!( REMOTE_HELP.contains(sub), "help should document /remote {sub}" diff --git a/crates/jcode-tui/src/tui/app/turn_notify.rs b/crates/jcode-tui/src/tui/app/turn_notify.rs index db106835ed..d291f8e842 100644 --- a/crates/jcode-tui/src/tui/app/turn_notify.rs +++ b/crates/jcode-tui/src/tui/app/turn_notify.rs @@ -7,8 +7,8 @@ //! default it fires only while the terminal window is unfocused. use super::App; -use base64::Engine as _; use crate::todo::TodoItem; +use base64::Engine as _; #[cfg(target_os = "macos")] use std::io::Write; @@ -139,9 +139,7 @@ fn notification_text(notification: &TurnNotification) -> String { } fn osc_safe(text: &str) -> String { - text.chars() - .filter(|ch| !ch.is_control()) - .collect() + text.chars().filter(|ch| !ch.is_control()).collect() } fn kitty_notification_id(session_id: &str) -> String { @@ -150,7 +148,10 @@ fn kitty_notification_id(session_id: &str) -> String { .filter(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-' | '+' | '.')) .take(128) .collect(); - format!("jcode-turn-{}", if safe.is_empty() { "unknown" } else { &safe }) + format!( + "jcode-turn-{}", + if safe.is_empty() { "unknown" } else { &safe } + ) } fn kitty_notification_sequence(notification: &TurnNotification, session_id: &str) -> String { diff --git a/crates/jcode-tui/src/tui/ui_messages.rs b/crates/jcode-tui/src/tui/ui_messages.rs index 7d8cb0bfb8..27d8b79bba 100644 --- a/crates/jcode-tui/src/tui/ui_messages.rs +++ b/crates/jcode-tui/src/tui/ui_messages.rs @@ -3566,7 +3566,7 @@ fn render_discovery_card( MAX_DISCOVERY_DETAIL_LINES, ); } - "select" => { + "select" | "setup" => { let name = tool .input .get("tool") @@ -3575,7 +3575,14 @@ fn render_discovery_card( push_compact_discovery_header( &mut content, vec![ - Span::styled("selected ", muted_style), + Span::styled( + if tool_output.starts_with("Selected off-catalog product") { + "selected off-catalog " + } else { + "selected " + }, + muted_style, + ), Span::styled(name.to_string(), name_style), ], block_width, diff --git a/crates/jcode-tui/src/tui/ui_messages/tests.rs b/crates/jcode-tui/src/tui/ui_messages/tests.rs index f1b09b7d64..d769fe31fd 100644 --- a/crates/jcode-tui/src/tui/ui_messages/tests.rs +++ b/crates/jcode-tui/src/tui/ui_messages/tests.rs @@ -2271,6 +2271,33 @@ fn render_tool_message_shows_selected_discovery_setup() { ); } +#[test] +fn render_tool_message_marks_off_catalog_selection_without_fake_details() { + let msg = discovery_message( + "Selected off-catalog product 'firecrawl' for 'web-data'.\n\nSelection recorded as demand data. Jcode does not list or partner with this product, so no provider information, recommendation, or setup instructions are provided.", + serde_json::json!({ + "action": "select", + "category": "web-data", + "tool": "firecrawl", + "query": "crawl a documentation site and extract structured markdown", + "reason": "the user explicitly requested Firecrawl instead of the catalog listing" + }), + ); + let lines = render_tool_message(&msg, 100, crate::config::DiffDisplayMode::Off); + let plain = lines + .iter() + .map(extract_line_text) + .collect::>() + .join("\n"); + assert!(plain.contains("selected off-catalog firecrawl"), "{plain}"); + assert!( + plain.contains("why: the user explicitly requested"), + "{plain}" + ); + assert!(!plain.contains("details:"), "{plain}"); + assert!(!plain.contains("setup:"), "{plain}"); +} + #[test] fn render_tool_message_shows_catalog_suggestion_receipt_and_trust_line() { let msg = discovery_message( diff --git a/docs/DISCOVERY_RATE_BENCHMARK.md b/docs/DISCOVERY_RATE_BENCHMARK.md index a302ecac9a..a9c962f8c9 100644 --- a/docs/DISCOVERY_RATE_BENCHMARK.md +++ b/docs/DISCOVERY_RATE_BENCHMARK.md @@ -1,9 +1,9 @@ # Discovery call-rate benchmark `scripts/benchmark_discovery_rate.py` measures the policy we actually want to -hold: **the agent calls `discover_tools` whenever it reaches for an external +hold: **the agent calls `integration_tools` whenever it reaches for an external product, service, API, or data source, and it commits to a specific vendor -through `action=select` rather than around Discovery.** +through `action=select` rather than around the integration directory.** This is a different question from `docs/DISCOVERY_BENCHMARK.md`. That benchmark is catalog-locked: it verifies that each live listing is reachable from a natural @@ -16,6 +16,7 @@ broad suite, including tasks where triggering would be wrong. python scripts/benchmark_discovery_rate.py --provider jcode --model claude-haiku-4-5-20251001 python scripts/benchmark_discovery_rate.py --trials 3 # tighter confidence python scripts/benchmark_discovery_rate.py --tag control # precision only +python scripts/benchmark_discovery_rate.py --tag selection # selection accuracy only python scripts/benchmark_discovery_rate.py --case storage-user-uploads python scripts/benchmark_discovery_rate.py --list # inspect the suite ``` @@ -31,19 +32,24 @@ named baselines. ## The suite -`scripts/discovery_rate_cases.json` holds two kinds of case. +`scripts/discovery_rate_cases.json` holds three kinds of case. -- `expect: "call"` — a task that genuinely needs an external capability. There is +- `expect: "call"` - a task that genuinely needs an external capability. There is at least one per Discovery category, plus two open-category tasks (SMS, speech to text) where no category is asserted. These measure **recall**. -- `expect: "no-call"` — a nearby task that is purely local: refactoring, tests, +- `expect: "no-call"` - a nearby task that is purely local: refactoring, tests, writing copy, a Dockerfile, local SQLite. Any Discovery call here is a false positive. These measure **precision**, so recall cannot be bought by calling Discovery on everything. +- `expect: "select"` - a task where the user has already chosen a named product. + These cases require `expected_category`, `expected_tool`, and boolean + `expected_listed`. The checked-in suite covers context.dev as a listed product + and Firecrawl as an off-catalog product. These measure **selection accuracy**. -Loading the suite enforces that prompts never name `discover_tools`, never say +Loading the suite enforces that prompts never name `integration_tools`, never say "discovery", and never contain a category slug. A prompt that leaks the -mechanism measures nothing. +mechanism measures nothing. Selection prompts name the product because the +behavior under test begins after the user has made that choice. ## Metrics @@ -58,12 +64,30 @@ Per case and in aggregate: A high bypass rate is the specific failure this benchmark exists to catch. - **select rate** — trials that reached `action=select`, the second half of the intended policy. +- **selection accuracy** - on `expect: "select"` cases, the fraction of scored + trials whose actual `integration_tools` input used `action: "select"` and the + expected `tool`, included a substantive `reason` explaining the choice, and + whose output receipt reported the expected tool, category, and catalog status. + Catalog receipts such as `Selected 'context.dev' from + 'web-data' ...` count as `listed: true`; `Selected off-catalog product + 'Firecrawl' for 'web-data'.` counts as `listed: false`. Output text alone does + not prove that the agent supplied the required tool input. - **category accuracy** — when a browse happened, whether it used the expected category. - **control clean rate** — controls that finished with no Discovery call. -The run passes when aggregate browse recall clears `--min-recall` (default 0.8) -and control clean rate clears `--min-precision` (default 0.9). +The run passes when each represented family clears its gate: aggregate browse +recall clears `--min-recall` (default 0.8), control clean rate clears +`--min-precision` (default 0.9), and exact selection accuracy clears +`--min-selection-accuracy` (default 1.0). A filtered run applies only the gates +for case families it contains, so `--tag selection` can pass or fail without call +or control cases. A run with no scored trials never passes. + +Every trial stops on its first selection, whether correct or incorrect. This +makes the receipt decisive and prevents setup instructions from leading into an +install, signup, spending, or another consequential action. Existing `call` +cases still require a browse response, and `no-call` controls still fail on their +first integration-directory call. ## Bypass detection diff --git a/docs/JCODE_CLOUD_AWS.md b/docs/JCODE_CLOUD_AWS.md new file mode 100644 index 0000000000..7246c34e78 --- /dev/null +++ b/docs/JCODE_CLOUD_AWS.md @@ -0,0 +1,114 @@ +# Jcode Cloud on AWS + +Status: product and infrastructure direction, August 2026. The managed customer +control plane described here is not deployed yet. + +## Product decision + +`/remote` is the single activation entry point for Jcode Cloud. It opens the Jcode account activation page, where a user signs in, confirms the subscription entitlement, chooses a region, and creates or wakes their host. `/remote status`, `/remote on`, `/remote pair`, and `/remote revoke` remain the explicit self-hosted gateway controls. + +Cloud access is bundled into paid Jcode subscriptions rather than sold as a second product. The subscription pays for the control plane and a bounded amount of host runtime/storage. Model-token budgets remain governed by the existing subscription tier. + +### User journey + +1. Run `/remote` on desktop or select **Jcode Cloud** in a client. +2. Browser opens `https://jcode.sh/account`. During early access, the account + page handles sign-in and plan management but does not yet provision a host. +3. Sign in with the existing Jcode device/account identity. If needed, subscribe or upgrade. +4. Pick the nearest supported region. Defaults are automatic and reversible. +5. Jcode provisions an isolated host, imports only credentials or repository access the user explicitly approves, and displays progress. +6. The page returns a one-time deep link. Desktop and mobile store a revocable device credential. +7. Later connections wake the host automatically. It stops after 30 idle minutes and preserves the encrypted workspace. + +The normal path must not expose EC2, SSH, ports, pairing codes, AWS accounts, or instance types. Advanced users retain the local self-hosting commands. + +## AWS architecture + +```mermaid +flowchart LR + C[Jcode clients] -->|OAuth/device credential| CF[CloudFront] + CF --> APIGW[API Gateway HTTP + WebSocket] + APIGW --> CP[Lambda control plane] + CP --> DDB[(DynamoDB accounts, hosts, devices, jobs)] + CP --> SQS[SQS provisioning jobs] + SQS --> PROV[Provisioner Lambda or Step Functions] + PROV --> EC2[Per-user EC2 host] + EC2 --> EBS[(Encrypted EBS workspace)] + EC2 --> BR[Amazon Bedrock] + EC2 --> SSM[Systems Manager] + APIGW --> ROUTER[Subscription model router] + ROUTER --> BR + EV[EventBridge] --> REAPER[Idle and budget reaper] + REAPER --> EC2 + BILL[Stripe webhooks] --> APIGW + CP --> SES[SES transactional email] + LOG[CloudWatch + CloudTrail] --- CP +``` + +### AWS services + +- **Identity:** Cognito user pool federated from the existing Jcode account during migration. Long term, Cognito is the account identity authority. Device authorization is implemented by Lambda/API Gateway with hashed, short-lived codes. +- **API edge:** CloudFront, WAF, API Gateway HTTP APIs, and WebSocket APIs. No user host has public ingress. +- **State:** DynamoDB with on-demand capacity, point-in-time recovery, TTL for device codes, idempotency records, and host leases. +- **Provisioning:** SQS plus Step Functions for idempotent create, wake, stop, update, snapshot, and delete workflows. +- **Compute:** one EC2 instance and encrypted EBS volume per active user initially. Hosts use SSM and outbound-only networking. Move steady multi-tenant workloads to ECS only after isolation and economics are measured. +- **Model routing:** Bedrock by default. Non-Bedrock upstreams remain behind the subscription router until equivalent models are available. +- **Secrets:** Secrets Manager for service secrets. Per-user grants use short-lived scoped credentials. Never copy local plaintext credentials by default. +- **Observability:** CloudWatch structured logs/metrics, X-Ray traces, CloudTrail, GuardDuty, Security Hub, and AWS Budgets. +- **Email:** SES. **Artifacts:** versioned private S3 buckets. **Encryption:** KMS customer-managed keys with separate control-plane and host-data keys. + +## Isolation and network rules + +- Dedicated instance profile, security group, EBS volume, and host record per user. +- No public IPv4 or inbound security-group rules for production hosts. +- Clients connect through the managed WebSocket edge. The control plane routes authenticated sessions to the assigned host over an outbound tunnel. +- SSM Session Manager is the only operator shell path. CloudTrail records all administrative access. +- Device tokens are random, hashed at rest, scoped to one account/host, rotatable, and revocable. +- Every mutating API accepts an idempotency key. Provisioning workflows reconcile desired state after retries. + +## Subscription policy + +All paid tiers include Cloud activation. Limits are entitlements, not separate SKUs: + +| Tier | Included cloud shape | Included runtime policy | +|---|---|---| +| Plus | burstable 2 vCPU, 4 GiB, 20 GiB | personal interactive use, aggressive idle stop | +| Pro | 2 vCPU, 8 GiB, 40 GiB | longer monthly runtime allowance | +| Max | 4 vCPU, 16 GiB, 80 GiB | larger repos and background agents | +| Ultra | 8 vCPU, 32 GiB, 160 GiB | sustained agents and higher concurrency | +| Solo | configurable dedicated host | contract limits and priority support | + +Exact included hours must be set from measured AWS cost plus support and model margin. Hard safety behavior is required before launch: warn at 70%, stop paid overage by default at 100%, and require explicit opt-in for metered overage. + +## API contract + +Authenticated endpoints under the existing `api.jcode.sh/v1` origin: + +- `GET /cloud` returns entitlement, desired/actual host state, region, limits, client endpoint, and pending operation. +- `POST /cloud/activate` creates the desired host idempotently. +- `POST /cloud/wake` and `POST /cloud/stop` change desired state. +- `POST /cloud/devices` issues a one-time pairing/deep-link exchange. +- `DELETE /cloud/devices/{id}` revokes a client. +- `POST /cloud/transfer` creates an explicit, encrypted project import job. +- `DELETE /cloud` requires recent authentication, creates a recovery snapshot, and schedules final deletion after a retention window. + +Host lifecycle states are `absent`, `provisioning`, `stopped`, `starting`, `ready`, `stopping`, `failed`, and `deleting`. Clients poll operations and may also subscribe to WebSocket lifecycle events. + +## Rollout + +1. **Internal alpha:** keep the existing guarded `jcode-phone` EC2 deployment as the reference host. Validate wake, SSM, gateway, Bedrock, idle stop, and breaker paths. +2. **Single-account beta:** deploy AWS control-plane stacks with IaC, provision per-user hosts in `us-east-1`, and manually grant a small allowlist. +3. **Subscription beta:** connect existing account entitlements and Stripe events, enforce tier limits, and add self-service activation. +4. **General availability:** multi-region hosts, automated recovery, support tooling, cost attribution, deletion/export flows, and published SLOs. + +## Infrastructure requirements before customer provisioning + +- Use CDK or Terraform as the sole writer for production resources. Do not build the multi-user stack from ad hoc CLI commands. +- Separate `dev`, `staging`, and `prod` AWS accounts under Organizations with SCPs and IAM Identity Center. +- Require MFA for human roles and OIDC for CI. No long-lived administrator keys. +- Add per-account and per-user budget alarms, concurrency quotas, EC2 quota checks, DLQs, backup restore tests, and a tested global kill switch. +- Complete threat modeling, privacy/retention policy, incident runbook, and billing reconciliation tests. + +## Current state + +The AWS account already contains a live-tested reference deployment in `us-east-1`: EC2 host `i-08214cf66cd3f80c7`, wake and breaker Lambdas, SSM management, Bedrock access, idle shutdown, encrypted EBS, and a $10 budget guardrail. It is suitable for internal validation, not shared customer tenancy. The current subscription/account backend is owned by the private `solosystems-backend` repository and uses Cloudflare-backed services, so moving **everything** to AWS requires a coordinated backend migration rather than client-only changes. diff --git a/scripts/benchmark_discovery.py b/scripts/benchmark_discovery.py index f69fd9c7fa..4236afc04b 100755 --- a/scripts/benchmark_discovery.py +++ b/scripts/benchmark_discovery.py @@ -43,6 +43,9 @@ LISTING_RE = re.compile(r"(?:Discoverable tools|Available integrations) in '([^']+)'") EMPTY_RE = re.compile(r"No (?:discoverable tools|integrations) in category '([^']+)'") SELECTION_RE = re.compile(r"(?:Selected|Set up) '([^']+)' from '([^']+)'") +OFF_CATALOG_SELECTION_RE = re.compile( + r"Selected off-catalog product '([^']+)' for '([^']+)'" +) TOOL_RE = re.compile(r"^- ([^:\n]+):", re.MULTILINE) RUNTIME_ERROR_RE = re.compile( r"\b(error|failed|failure|timed out|timeout|did not start|exited before startup)\b", @@ -66,6 +69,7 @@ class DiscoveryCall: tools: list[str] outcome: str output: str + listed: bool | None = None @dataclass @@ -243,6 +247,7 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: listing = LISTING_RE.search(output) empty = EMPTY_RE.search(output) selection = SELECTION_RE.search(output) + off_catalog_selection = OFF_CATALOG_SELECTION_RE.search(output) category = ( listing.group(1) if listing @@ -250,6 +255,8 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: if empty else selection.group(2) if selection + else off_catalog_selection.group(2) + if off_catalog_selection else None ) tools = ( @@ -257,13 +264,15 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: if listing else [selection.group(1).strip().lower()] if selection + else [off_catalog_selection.group(1).strip().lower()] + if off_catalog_selection else [] ) if listing: outcome = "listing" elif empty: outcome = "empty" - elif selection: + elif selection or off_catalog_selection: outcome = "selection" elif output.startswith("Error:"): outcome = "error" @@ -275,6 +284,7 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: tools=tools, outcome=outcome, output=output[:4000], + listed=True if selection else False if off_catalog_selection else None, ) diff --git a/scripts/benchmark_discovery_rate.py b/scripts/benchmark_discovery_rate.py index 4568fcd256..26524e4c86 100755 --- a/scripts/benchmark_discovery_rate.py +++ b/scripts/benchmark_discovery_rate.py @@ -1,13 +1,13 @@ #!/usr/bin/env python3 -"""Call-rate benchmark for `discover_tools`. +"""Call-rate benchmark for `integration_tools`. Where `benchmark_discovery.py` asks "did the agent reach the expected catalog listing", this runner asks the two questions that define the intended policy: 1. Recall: when a task needs an external product, service, API, or data source, - does the agent call `discover_tools` at all (browse phase)? + does the agent call `integration_tools` at all (search/browse phase)? 2. Select discipline: when the agent then commits to a specific product, does - that commitment go through `discover_tools` action=select, or does it bypass + that commitment go through `integration_tools` action=select, or does it bypass Discovery entirely by installing an SDK, hitting a vendor URL, or connecting an MCP server directly? @@ -42,6 +42,7 @@ BENCHMARK_ENV, BENCHMARK_HEADER, BenchmarkError, + DiscoveryCall, benchmark_environment, load_categories, DISCOVERY_TOOL_NAMES, @@ -103,6 +104,8 @@ class RateCase: expect: str prompt: str expected_category: str | None = None + expected_tool: str | None = None + expected_listed: bool | None = None tags: tuple[str, ...] = () @@ -131,6 +134,7 @@ class TrialResult: selected_via_discovery: list[str] = field(default_factory=list) first_call_seconds: float | None = None category_correct: bool | None = None + selection_correct: bool | None = None bypasses: list[Bypass] = field(default_factory=list) discovery_calls: list[dict[str, Any]] = field(default_factory=list) other_tool_calls: list[str] = field(default_factory=list) @@ -158,6 +162,12 @@ def parse_args() -> argparse.Namespace: parser.add_argument("--output", type=Path, default=DEFAULT_OUTPUT) parser.add_argument("--min-recall", type=float, default=0.8, help="Required browse rate on `call` cases.") parser.add_argument("--min-precision", type=float, default=0.9, help="Required clean rate on `no-call` controls.") + parser.add_argument( + "--min-selection-accuracy", + type=float, + default=1.0, + help="Required exact selection rate on `select` cases.", + ) parser.add_argument("--retry-delay", type=float, default=0.5) parser.add_argument( "--invalid-retries", @@ -175,6 +185,9 @@ def parse_args() -> argparse.Namespace: args = parser.parse_args() if args.trials < 1 or args.timeout <= 0: parser.error("--trials must be >= 1 and --timeout must be positive") + for name in ("min_recall", "min_precision", "min_selection_accuracy"): + if not 0 <= getattr(args, name) <= 1: + parser.error(f"--{name.replace('_', '-')} must be between 0 and 1") return args @@ -191,12 +204,25 @@ def load_cases(path: Path, categories: list[str]) -> list[RateCase]: expect=str(raw.get("expect", "")).strip().lower(), prompt=str(raw.get("prompt", "")).strip(), expected_category=(str(raw.get("expected_category") or "").strip().lower() or None), + expected_tool=(str(raw.get("expected_tool") or "").strip().lower() or None), + expected_listed=raw.get("expected_listed"), tags=tuple(str(tag).strip().lower() for tag in raw.get("tags", [])), ) if not case.id or not case.prompt: raise BenchmarkError(f"rate case has an empty id or prompt: {raw}") - if case.expect not in {"call", "no-call"}: - raise BenchmarkError(f"case {case.id}: expect must be 'call' or 'no-call'") + if case.expect not in {"call", "no-call", "select"}: + raise BenchmarkError(f"case {case.id}: expect must be 'call', 'no-call', or 'select'") + if case.expect == "select": + if not case.expected_category or not case.expected_tool: + raise BenchmarkError( + f"case {case.id}: select cases require expected_category and expected_tool" + ) + if not isinstance(case.expected_listed, bool): + raise BenchmarkError(f"case {case.id}: select cases require boolean expected_listed") + elif case.expected_tool is not None or case.expected_listed is not None: + raise BenchmarkError( + f"case {case.id}: only select cases may declare expected_tool or expected_listed" + ) if case.expect == "no-call" and case.expected_category: raise BenchmarkError(f"case {case.id}: no-call cases must not declare a category") if case.expected_category and case.expected_category not in categories: @@ -261,6 +287,46 @@ def detect_bypasses(tool: str, text: str, elapsed: float) -> list[Bypass]: return found +def parse_tool_input(text: str) -> tuple[str | None, str | None, str | None]: + """Return normalized action, tool, and stated reason from one tool input.""" + try: + value = json.loads(text) + except (json.JSONDecodeError, TypeError): + return None, None, None + if not isinstance(value, dict): + return None, None, None + action = str(value.get("action") or "").strip().lower() or None + tool = str(value.get("tool") or "").strip().lower() or None + reason = str(value.get("reason") or "").strip() or None + return action, tool, reason + + +def selection_is_correct( + case: RateCase, + call: DiscoveryCall, + action: str | None, + tool: str | None, + reason: str | None, +) -> bool: + """Require the select input, stated reason, and receipt to agree with the case.""" + return ( + case.expect == "select" + and action == "select" + and tool == case.expected_tool + and reason is not None + and len(reason) >= 40 + and call.outcome == "selection" + and call.tools == [case.expected_tool] + and call.category == case.expected_category + and call.listed is case.expected_listed + ) + + +def discovery_call_stops_trial(case: RateCase, is_selection: bool) -> bool: + """Controls stop on any call; every case stops once a product is selected.""" + return case.expect == "no-call" or is_selection + + def _pump(stream: Any, source: str, messages: queue.Queue[tuple[str, str | None]]) -> None: try: for line in iter(stream.readline, ""): @@ -356,6 +422,10 @@ def run_trial(args: argparse.Namespace, case: RateCase, trial: int, socket_path: call = parse_discovery_output(output, elapsed) record = asdict(call) record["input"] = tool_input[:2000] + input_action, input_tool, input_reason = parse_tool_input(tool_input) + record["input_action"] = input_action + record["input_tool"] = input_tool + record["input_reason"] = input_reason result.discovery_calls.append(record) if result.first_call_seconds is None: result.first_call_seconds = round(elapsed, 3) @@ -363,10 +433,16 @@ def run_trial(args: argparse.Namespace, case: RateCase, trial: int, socket_path: result.browsed = True if call.category: result.browse_categories.append(call.category) - elif call.outcome == "selection" and call.tools: - result.selected_via_discovery.append(call.tools[0]) - if case.expect == "no-call": - # A control is decided by the first Discovery call. + is_selection = input_action == "select" or call.outcome == "selection" + if input_action == "select" and input_tool: + result.selected_via_discovery.append(input_tool) + if case.expect == "select" and is_selection: + result.selection_correct = selection_is_correct( + case, call, input_action, input_tool, input_reason + ) + if discovery_call_stops_trial(case, is_selection): + # A control is decided by its first call. A product choice + # decides every case before a consequential action can run. decided = True break else: @@ -403,6 +479,15 @@ def run_trial(args: argparse.Namespace, case: RateCase, trial: int, socket_path: if case.expect == "no-call": result.outcome = "false-positive" if result.discovery_calls else "clean" + elif case.expect == "select": + if result.selection_correct is True: + result.outcome = "selected" + elif result.selection_correct is False: + result.outcome = "incorrect-selection" + elif result.bypasses: + result.outcome = "bypassed" + else: + result.outcome = "no-selection" elif result.browsed: result.outcome = "browsed" elif result.selected_via_discovery: @@ -426,7 +511,7 @@ def summarize_case(case: RateCase, trials: list[TrialResult]) -> dict[str, Any]: selects = [trial for trial in scored if trial.selected_via_discovery] category_scored = [trial for trial in scored if trial.category_correct is not None] first_call_times = [trial.first_call_seconds for trial in scored if trial.first_call_seconds is not None] - wanted = "clean" if case.expect == "no-call" else "browsed" + wanted = {"no-call": "clean", "call": "browsed", "select": "selected"}[case.expect] passed = bool(scored) and all(trial.outcome == wanted for trial in scored) def rate(subset: list[TrialResult]) -> float | None: @@ -442,6 +527,11 @@ def rate(subset: list[TrialResult]) -> float | None: "browse_rate": rate(browsed), "bypass_rate": rate(bypassed), "select_rate": rate(selects), + "selection_accuracy": ( + sum(trial.selection_correct is True for trial in scored) / total + if case.expect == "select" and total + else None + ), "category_accuracy": ( sum(1 for trial in category_scored if trial.category_correct) / len(category_scored) if category_scored @@ -462,6 +552,7 @@ def rate(subset: list[TrialResult]) -> float | None: def aggregate(results: list[dict[str, Any]]) -> dict[str, Any]: call_cases = [result for result in results if result["case"]["expect"] == "call"] control_cases = [result for result in results if result["case"]["expect"] == "no-call"] + select_cases = [result for result in results if result["case"]["expect"] == "select"] def mean(values: list[float]) -> float | None: values = [value for value in values if value is not None] @@ -470,15 +561,20 @@ def mean(values: list[float]) -> float | None: category_scores = [ result["category_accuracy"] for result in call_cases if result["category_accuracy"] is not None ] + action_cases = call_cases + select_cases return { "call_case_count": len(call_cases), "control_case_count": len(control_cases), + "select_case_count": len(select_cases), "invalid_trial_count": sum(result["invalid_trial_count"] for result in results), "scored_trial_count": sum(result["scored_trial_count"] for result in results), "recall_browse_rate": mean([result["browse_rate"] for result in call_cases]), "recall_any_call_rate": mean([result["call_rate"] for result in call_cases]), "bypass_rate": mean([result["bypass_rate"] for result in call_cases]), - "select_rate": mean([result["select_rate"] for result in call_cases]), + "select_rate": mean([result["select_rate"] for result in action_cases]), + "selection_accuracy": mean( + [result["selection_accuracy"] for result in select_cases] + ), "category_accuracy": mean(category_scores), "control_clean_rate": mean( [1.0 - result["call_rate"] for result in control_cases if result["call_rate"] is not None] @@ -496,6 +592,25 @@ def mean(values: list[float]) -> float | None: } +def passes_gates( + summary: dict[str, Any], + min_recall: float, + min_precision: float, + min_selection_accuracy: float, +) -> bool: + """Apply only gates for case families represented by scored trials.""" + if summary["scored_trial_count"] <= 0: + return False + return all( + value is None or value >= minimum + for value, minimum in ( + (summary["recall_browse_rate"], min_recall), + (summary["control_clean_rate"], min_precision), + (summary["selection_accuracy"], min_selection_accuracy), + ) + ) + + def main() -> int: args = parse_args() started_at = datetime.now(timezone.utc) @@ -504,8 +619,12 @@ def main() -> int: if args.list: for case in cases: - marker = "call" if case.expect == "call" else "CTRL" - print(f"{marker:5} {case.id:38} {case.expected_category or '-':24} {case.prompt[:70]}") + marker = {"call": "call", "no-call": "CTRL", "select": "SEL"}[case.expect] + target = case.expected_tool or "-" + print( + f"{marker:5} {case.id:38} {case.expected_category or '-':24} " + f"{target:16} {case.prompt[:70]}" + ) print(f"\n{len(cases)} cases") return 0 @@ -549,12 +668,12 @@ def main() -> int: summary = aggregate(results) recall = summary["recall_browse_rate"] precision = summary["control_clean_rate"] - # A run with nothing scored is not a pass. Gate on real measurements only. - enough_signal = summary["scored_trial_count"] > 0 and recall is not None - passed = ( - enough_signal - and recall >= args.min_recall - and (precision is None or precision >= args.min_precision) + selection_accuracy = summary["selection_accuracy"] + passed = passes_gates( + summary, + args.min_recall, + args.min_precision, + args.min_selection_accuracy, ) report = { "benchmark": "discovery-call-rate", @@ -574,6 +693,7 @@ def main() -> int: "cases_file": str(args.cases), "min_recall": args.min_recall, "min_precision": args.min_precision, + "min_selection_accuracy": args.min_selection_accuracy, }, "summary": summary, "results": results, @@ -590,6 +710,10 @@ def main() -> int: print(f" Any Discovery call on those cases: {_pct(summary['recall_any_call_rate'])}") print(f" Bypassed Discovery entirely: {_pct(summary['bypass_rate'])}") print(f" Reached action=select: {_pct(summary['select_rate'])}") + print( + f" Exact selection accuracy: {_pct(selection_accuracy)} " + f"(gate {args.min_selection_accuracy:.0%})" + ) print(f" Correct category when browsing: {_pct(summary['category_accuracy'])}") print(f" Controls left clean: {_pct(precision)} (gate {args.min_precision:.0%})") if summary["failing_controls"]: @@ -600,6 +724,7 @@ def main() -> int: print( f" {case['id']:38} {case['expect']:8} browse={_pct(result['browse_rate'])} " f"bypass={_pct(result['bypass_rate'])} select={_pct(result['select_rate'])} " + f"accuracy={_pct(result['selection_accuracy'])} " f"{'invalid=' + str(result['invalid_trial_count']) + ' ' if result['invalid_trial_count'] else ''}" f"{'' if result['passed'] else 'FAIL'}" ) diff --git a/scripts/discovery_rate_cases.json b/scripts/discovery_rate_cases.json index e1c9f04725..71570325b8 100644 --- a/scripts/discovery_rate_cases.json +++ b/scripts/discovery_rate_cases.json @@ -1,6 +1,6 @@ { "version": 1, - "comment": "Call-rate suite for discover_tools. Unlike scripts/discovery_benchmark_cases.json this suite is not catalog-locked: it measures how often the agent reaches for Discovery at all across a broad spread of tasks, including tasks where reaching for it is wrong. `expect` is `call` or `no-call`. `expected_category` is optional and only scores category routing when a call happens. Prompts must never name a catalog tool, a category slug, or discover_tools.", + "comment": "Call-rate suite for integration_tools. Unlike scripts/discovery_benchmark_cases.json this suite is not catalog-locked: it measures how often the agent reaches for Discovery at all across a broad spread of tasks, including tasks where reaching for it is wrong. `expect` is `call`, `no-call`, or `select`. Selection cases name an already-chosen product and require `expected_category`, `expected_tool`, and `expected_listed`. Other prompts must not name a product, category slug, or integration_tools.", "cases": [ { "id": "payments-agent-purchase", @@ -140,6 +140,24 @@ "tags": ["capability-gap", "open-category"], "prompt": "I have a folder of recorded customer calls and want accurate transcripts with speaker labels, produced automatically for new recordings too." }, + { + "id": "select-context-dev", + "expect": "select", + "expected_category": "web-data", + "expected_tool": "context.dev", + "expected_listed": true, + "tags": ["selection", "catalog-selection"], + "prompt": "I've compared the options and chosen context.dev for the company website enrichment pipeline. Set up that product in this project." + }, + { + "id": "select-firecrawl", + "expect": "select", + "expected_category": "web-data", + "expected_tool": "firecrawl", + "expected_listed": false, + "tags": ["selection", "off-catalog-selection"], + "prompt": "I've compared the options and chosen Firecrawl to crawl customer websites into structured records. Set up that product in this project." + }, { "id": "control-local-refactor", "expect": "no-call", diff --git a/scripts/test_benchmark_discovery.py b/scripts/test_benchmark_discovery.py index 8b4459a869..23d394c396 100755 --- a/scripts/test_benchmark_discovery.py +++ b/scripts/test_benchmark_discovery.py @@ -157,6 +157,30 @@ def test_parse_accepts_current_integration_vocabulary(self): self.assertEqual(selection.category, "email-messaging") self.assertEqual(selection.tools, ["agentmail"]) self.assertEqual(selection.outcome, "selection") + self.assertIs(selection.listed, True) + + def test_parse_current_catalog_selection_receipt(self): + call = benchmark.parse_discovery_output( + "Selected 'context.dev' from 'web-data' (Jcode tool directory; the choice " + "must be based only on fit; details: https://jcode.sh/discovery-tools):\n\n" + "context.dev: structured extraction", + 1.25, + ) + self.assertEqual(call.category, "web-data") + self.assertEqual(call.tools, ["context.dev"]) + self.assertEqual(call.outcome, "selection") + self.assertIs(call.listed, True) + + def test_parse_off_catalog_selection_receipt(self): + call = benchmark.parse_discovery_output( + "Selected off-catalog product 'Firecrawl' for 'web-data'.\n\n" + "Selection recorded as demand data. Jcode does not list or partner with this product.", + 2.5, + ) + self.assertEqual(call.category, "web-data") + self.assertEqual(call.tools, ["firecrawl"]) + self.assertEqual(call.outcome, "selection") + self.assertIs(call.listed, False) def test_parse_selection_tracks_but_does_not_count_direct_selection(self): call = benchmark.parse_discovery_output( @@ -165,6 +189,7 @@ def test_parse_selection_tracks_but_does_not_count_direct_selection(self): self.assertEqual(call.category, "email-messaging") self.assertEqual(call.tools, ["agentmail"]) self.assertEqual(call.outcome, "selection") + self.assertIs(call.listed, True) case = benchmark.BenchmarkCase( "agentmail", "email-messaging", "agentmail", "Set up an inbox." ) diff --git a/scripts/test_benchmark_discovery_rate.py b/scripts/test_benchmark_discovery_rate.py index e828ef8ae1..9272bd76e7 100755 --- a/scripts/test_benchmark_discovery_rate.py +++ b/scripts/test_benchmark_discovery_rate.py @@ -94,8 +94,14 @@ def test_shipped_suite_is_valid_and_balanced(self) -> None: cases = rate.load_cases(rate.DEFAULT_CASES, self.categories) calls = [case for case in cases if case.expect == "call"] controls = [case for case in cases if case.expect == "no-call"] + selections = [case for case in cases if case.expect == "select"] self.assertGreaterEqual(len(calls), 15) self.assertGreaterEqual(len(controls), 8, "controls guard against over-triggering") + self.assertEqual( + {(case.expected_tool, case.expected_listed) for case in selections}, + {("context.dev", True), ("firecrawl", False)}, + ) + self.assertTrue(all(case.expected_category == "web-data" for case in selections)) # Every category with a positive case should be represented at most once # per distinct scenario, and all declared categories must be real. for case in calls: @@ -141,6 +147,148 @@ def test_control_may_not_declare_a_category(self) -> None: with self.assertRaises(rate.BenchmarkError): rate.load_cases(path, self.categories) + def test_selection_case_requires_complete_expected_receipt(self) -> None: + base = { + "id": "x", + "expect": "select", + "prompt": "I chose ExampleCo. Set it up.", + "expected_category": "web-data", + "expected_tool": "exampleco", + "expected_listed": False, + } + for missing in ("expected_category", "expected_tool", "expected_listed"): + malformed = dict(base) + malformed.pop(missing) + with self.subTest(missing=missing), self.assertRaises(rate.BenchmarkError): + rate.load_cases(self._write([malformed]), self.categories) + + def test_selection_case_requires_boolean_listed_status(self) -> None: + path = self._write( + [ + { + "id": "x", + "expect": "select", + "prompt": "I chose ExampleCo. Set it up.", + "expected_category": "web-data", + "expected_tool": "exampleco", + "expected_listed": "false", + } + ] + ) + with self.assertRaisesRegex(rate.BenchmarkError, "boolean expected_listed"): + rate.load_cases(path, self.categories) + + def test_call_cases_cannot_declare_selection_fields(self) -> None: + path = self._write( + [ + { + "id": "x", + "expect": "call", + "prompt": "Give this application an external capability.", + "expected_tool": "exampleco", + } + ] + ) + with self.assertRaisesRegex(rate.BenchmarkError, "only select cases"): + rate.load_cases(path, self.categories) + + +class SelectionTests(unittest.TestCase): + def setUp(self) -> None: + self.case = rate.RateCase( + id="select-context", + expect="select", + prompt="I chose context.dev. Set it up.", + expected_category="web-data", + expected_tool="context.dev", + expected_listed=True, + ) + self.call = rate.parse_discovery_output( + "Selected 'context.dev' from 'web-data' (Jcode tool directory):", 1.0 + ) + + def test_tool_input_parser_normalizes_selection(self) -> None: + self.assertEqual( + ( + "select", + "context.dev", + "The user chose it because it best fits the website enrichment workflow.", + ), + rate.parse_tool_input( + '{"action":"SELECT","tool":"Context.Dev","reason":"The user chose it because it best fits the website enrichment workflow."}' + ), + ) + self.assertEqual((None, None, None), rate.parse_tool_input("not json")) + self.assertEqual((None, None, None), rate.parse_tool_input("[]")) + + def test_exact_input_and_output_match_is_correct(self) -> None: + self.assertTrue( + rate.selection_is_correct( + self.case, + self.call, + "select", + "context.dev", + "The user explicitly chose context.dev for this website enrichment workflow.", + ) + ) + + def test_selection_requires_action_tool_category_and_listed_status(self) -> None: + wrong_outputs = [ + rate.parse_discovery_output( + "Selected 'context.dev' from 'web-search' (Jcode tool directory):", 1.0 + ), + rate.parse_discovery_output( + "Selected off-catalog product 'context.dev' for 'web-data'.", 1.0 + ), + rate.parse_discovery_output( + "Selected 'another-tool' from 'web-data' (Jcode tool directory):", 1.0 + ), + ] + good_reason = ( + "The user explicitly chose context.dev for this website enrichment workflow." + ) + for action, tool, reason, call in [ + ("search", "context.dev", good_reason, self.call), + ("select", "firecrawl", good_reason, self.call), + ("select", "context.dev", None, self.call), + ("select", "context.dev", "too short", self.call), + *[("select", "context.dev", good_reason, call) for call in wrong_outputs], + ]: + with self.subTest(action=action, tool=tool, reason=reason, output=call.output): + self.assertFalse( + rate.selection_is_correct(self.case, call, action, tool, reason) + ) + + def test_off_catalog_selection_can_match(self) -> None: + case = rate.RateCase( + id="select-firecrawl", + expect="select", + prompt="I chose Firecrawl. Set it up.", + expected_category="web-data", + expected_tool="firecrawl", + expected_listed=False, + ) + call = rate.parse_discovery_output( + "Selected off-catalog product 'Firecrawl' for 'web-data'.", 1.0 + ) + self.assertTrue( + rate.selection_is_correct( + case, + call, + "select", + "firecrawl", + "The user explicitly chose Firecrawl for this website enrichment workflow.", + ) + ) + + def test_any_selection_stops_all_case_kinds_immediately(self) -> None: + call_case = rate.RateCase("call", "call", "p", "payments") + control = rate.RateCase("control", "no-call", "p") + self.assertTrue(rate.discovery_call_stops_trial(self.case, True)) + self.assertTrue(rate.discovery_call_stops_trial(call_case, True)) + self.assertTrue(rate.discovery_call_stops_trial(control, False)) + self.assertFalse(rate.discovery_call_stops_trial(call_case, False)) + class ScoringTests(unittest.TestCase): def _case(self, expect: str = "call", category: str | None = "payments") -> rate.RateCase: @@ -189,6 +337,54 @@ def test_aggregate_ignores_unscored_cases(self) -> None: self.assertEqual(1.0, summary["recall_browse_rate"]) self.assertEqual(1, summary["invalid_trial_count"]) + def test_selection_accuracy_counts_missing_and_incorrect_selections(self) -> None: + case = rate.RateCase( + "selection", "select", "p", "web-data", "context.dev", True + ) + trials = [ + self._trial(outcome="selected", selection_correct=True), + self._trial(trial=2, outcome="incorrect-selection", selection_correct=False), + self._trial(trial=3, outcome="no-selection", browsed=False), + ] + summary = rate.summarize_case(case, trials) + self.assertAlmostEqual(1 / 3, summary["selection_accuracy"]) + self.assertFalse(summary["passed"]) + + def test_selection_only_aggregate_passes_and_fails_its_own_gate(self) -> None: + case = rate.RateCase( + "selection", "select", "p", "web-data", "context.dev", True + ) + result = rate.summarize_case( + case, + [ + self._trial( + outcome="selected", + selection_correct=True, + selected_via_discovery=["context.dev"], + ) + ], + ) + aggregate = rate.aggregate([result]) + self.assertIsNone(aggregate["recall_browse_rate"]) + self.assertIsNone(aggregate["control_clean_rate"]) + self.assertEqual(1.0, aggregate["select_rate"]) + self.assertEqual(1.0, aggregate["selection_accuracy"]) + self.assertTrue(rate.passes_gates(aggregate, 0.99, 0.99, 1.0)) + + aggregate["selection_accuracy"] = 0.5 + self.assertFalse(rate.passes_gates(aggregate, 0.0, 0.0, 0.75)) + + def test_no_scored_trials_never_pass_any_filtered_gate(self) -> None: + summary = rate.aggregate( + [ + rate.summarize_case( + self._case(), + [self._trial(outcome="invalid", browsed=False, invalid_reason="quota")], + ) + ] + ) + self.assertFalse(rate.passes_gates(summary, 0.0, 0.0, 0.0)) + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/src/cli/commands_tests.rs b/src/cli/commands_tests.rs index 8e32c00e19..ca7d5af15c 100644 --- a/src/cli/commands_tests.rs +++ b/src/cli/commands_tests.rs @@ -3,8 +3,8 @@ use crate::auth::{AuthState, AuthStatus, ProviderAuth}; use crate::message::{Message, StreamEvent, ToolDefinition}; use crate::provider::ModelRoute; use crate::provider::{EventStream, Provider}; -use crate::tool::Registry; use crate::todo::ConfidenceState; +use crate::tool::Registry; use async_trait::async_trait; use std::io::{Read, Write}; use std::sync::Arc; @@ -281,8 +281,20 @@ fn test_todo( #[test] fn run_auto_poke_followup_targets_below_threshold_todos() { let todos = vec![ - test_todo("a", "completed", "high", Some(ConfidenceState::Plausible), Some(ConfidenceState::Plausible)), - test_todo("b", "completed", "low", Some(ConfidenceState::Plausible), Some(ConfidenceState::Plausible)), + test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Plausible), + ), + test_todo( + "b", + "completed", + "low", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Plausible), + ), ]; let followup = build_run_auto_poke_follow_up_from_todos(&todos, false, None); @@ -305,7 +317,13 @@ fn run_auto_poke_followup_targets_below_threshold_todos() { #[test] fn run_auto_poke_followup_challenges_abrupt_confidence_once() { - let mut todo = test_todo("a", "completed", "high", Some(ConfidenceState::Speculative), Some(ConfidenceState::Verified)); + let mut todo = test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Speculative), + Some(ConfidenceState::Verified), + ); todo.confidence_history = vec![ConfidenceState::Speculative, ConfidenceState::Verified]; let todos = [todo]; @@ -332,7 +350,13 @@ fn run_auto_poke_followup_silent_when_confident_and_earned() { // summary anyway; now we spend no tokens and end the run. let todos = vec![ { - let mut todo = test_todo("a", "completed", "high", Some(ConfidenceState::Verified), Some(ConfidenceState::Verified)); + let mut todo = test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Verified), + Some(ConfidenceState::Verified), + ); todo.confidence_history = vec![ ConfidenceState::Plausible, ConfidenceState::Plausible, @@ -341,7 +365,13 @@ fn run_auto_poke_followup_silent_when_confident_and_earned() { ]; todo }, - test_todo("b", "completed", "low", Some(ConfidenceState::Validated), Some(ConfidenceState::Validated)), + test_todo( + "b", + "completed", + "low", + Some(ConfidenceState::Validated), + Some(ConfidenceState::Validated), + ), ]; assert!(build_run_auto_poke_follow_up_from_todos(&todos, false, None).is_none()); } @@ -349,8 +379,20 @@ fn run_auto_poke_followup_silent_when_confident_and_earned() { #[test] fn run_auto_poke_followup_prioritizes_incomplete_todos() { let todos = vec![ - test_todo("a", "completed", "high", Some(ConfidenceState::Plausible), Some(ConfidenceState::Plausible)), - test_todo("b", "in_progress", "medium", Some(ConfidenceState::Plausible), None), + test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Plausible), + ), + test_todo( + "b", + "in_progress", + "medium", + Some(ConfidenceState::Plausible), + None, + ), ]; let followup = build_run_auto_poke_follow_up_from_todos(&todos, false, None); @@ -371,7 +413,13 @@ fn run_auto_poke_followup_prioritizes_incomplete_todos() { /// the deferred quality review must reach that path too, not only the TUI. #[test] fn run_auto_poke_delivers_the_deferred_gate_digest_before_confidence() { - let todos = vec![test_todo("a", "completed", "high", Some(ConfidenceState::Plausible), Some(ConfidenceState::Plausible))]; + let todos = vec![test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Plausible), + )]; // Without a digest, the confidence gate is what fires. assert!(matches!( build_run_auto_poke_follow_up_from_todos(&todos, false, None), @@ -395,7 +443,13 @@ fn run_auto_poke_delivers_the_deferred_gate_digest_before_confidence() { /// turn to actually end rather than interrupting mid-flight. #[test] fn run_auto_poke_prefers_incomplete_todos_over_the_gate_digest() { - let todos = vec![test_todo("a", "in_progress", "high", Some(ConfidenceState::Plausible), None)]; + let todos = vec![test_todo( + "a", + "in_progress", + "high", + Some(ConfidenceState::Plausible), + None, + )]; assert!(matches!( build_run_auto_poke_follow_up_from_todos( &todos, @@ -428,7 +482,13 @@ fn open_todos_do_not_consume_the_pending_gate_digest() { ) .expect("append"); - let open = vec![test_todo("a", "in_progress", "high", Some(ConfidenceState::Plausible), None)]; + let open = vec![test_todo( + "a", + "in_progress", + "high", + Some(ConfidenceState::Plausible), + None, + )]; assert!(matches!( build_run_auto_poke_follow_up_from_todos( &open, @@ -445,7 +505,13 @@ fn open_todos_do_not_consume_the_pending_gate_digest() { ); // Once the work closes, the reminder is still there to deliver. - let done = vec![test_todo("a", "completed", "high", Some(ConfidenceState::Plausible), Some(ConfidenceState::Verified))]; + let done = vec![test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Verified), + )]; match build_run_auto_poke_follow_up_from_todos( &done, false, @@ -511,7 +577,13 @@ fn take_run_gate_digest_consumes_the_log_and_respects_delivery() { #[test] fn run_auto_poke_followup_rechecks_completion_confidence_until_it_passes() { - let needs_validation = vec![test_todo("a", "completed", "high", Some(ConfidenceState::Plausible), Some(ConfidenceState::Plausible))]; + let needs_validation = vec![test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Plausible), + )]; assert!(matches!( build_run_auto_poke_follow_up_from_todos(&needs_validation, false, None), Some(RunAutoPokeFollowUp::ConfidenceSummary { .. }) @@ -521,7 +593,13 @@ fn run_auto_poke_followup_rechecks_completion_confidence_until_it_passes() { Some(RunAutoPokeFollowUp::ConfidenceSummary { .. }) )); - let validated = vec![test_todo("a", "completed", "high", Some(ConfidenceState::Plausible), Some(ConfidenceState::Verified))]; + let validated = vec![test_todo( + "a", + "completed", + "high", + Some(ConfidenceState::Plausible), + Some(ConfidenceState::Verified), + )]; assert!(matches!( build_run_auto_poke_follow_up_from_todos(&validated, false, None), Some(RunAutoPokeFollowUp::ConfidenceSummary {