diff --git a/Cargo.lock b/Cargo.lock index 3d7d6237d7..65b1d2ea58 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3246,7 +3246,7 @@ checksum = "92ecc6618181def0457392ccd0ee51198e065e016d1d527a7ac1b6dc7c1f09d2" [[package]] name = "jcode" -version = "0.64.2" +version = "0.65.0" dependencies = [ "anyhow", "async-stream", @@ -3264,6 +3264,7 @@ dependencies = [ "jcode-provider-antigravity-runtime", "jcode-provider-claude-cli-runtime", "jcode-provider-copilot-runtime", + "jcode-provider-core", "jcode-provider-cursor-runtime", "jcode-provider-doctor", "jcode-provider-gemini-runtime", diff --git a/Cargo.toml b/Cargo.toml index a7502dc13d..4fe2fa4439 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "jcode" -version = "0.64.2" +version = "0.65.0" description = "Possibly the greatest coding agent ever built — blazing-fast TUI, multi-model, swarm coordination, 30+ tools" edition = "2024" autobins = false diff --git a/changelog/index.json b/changelog/index.json index ff7e491ac5..cf14c876cc 100644 --- a/changelog/index.json +++ b/changelog/index.json @@ -1,5 +1,9 @@ { "entries": [ + { + "version": "0.65.0", + "date": "2026-08-02" + }, { "version": "0.64.2", "date": "2026-07-30" @@ -149,4 +153,4 @@ "date": "2026-07-02" } ] -} \ No newline at end of file +} diff --git a/changelog/v0.65.0.json b/changelog/v0.65.0.json new file mode 100644 index 0000000000..ed6c18c3c9 --- /dev/null +++ b/changelog/v0.65.0.json @@ -0,0 +1,34 @@ +{ + "version": "0.65.0", + "date": "2026-08-02", + "title": "Seamless self-update, calmer TUI", + "highlights": [ + "Self-update is now seamless: a live progress bar during download and a graceful in-place reload when it finishes", + "Todos you are working on stay pinned in a band at the top of the viewport while you scroll", + "Ctrl+L is a true terminal-style clear: the screen blanks, history stays above, and the prompt sits at the top" + ], + "improvements": [ + "New display.external_sessions setting hides other CLIs' sessions from the session picker", + "The swarm gallery and snapshots now show which provider and auth route each agent is using", + "Markdown tables honour column alignment in every renderer, and copying a table gives you clean text", + "Inline math stays inline in image mode and blends with the surrounding prose colour", + "The discover_tools browse card is now a single compact line, and the sponsored-discovery notice is gone", + "Tool descriptions and parameter docs are capped, leaving more of the context window for your work", + "Ollama reports the context window it is actually serving instead of the trained window", + "Crash-resume hints now point at the session picker" + ], + "fixes": [ + "Custom OpenAI-compatible profile models route to their own profile instead of Copilot", + "OpenAI tool catalogs no longer break on MCP tools with untyped or unsupported schema properties", + "Ctrl+D forward-deletes mid-edit instead of quitting", + "Copying on X11 uses xclip/xsel so the selection survives after jcode exits", + "One bad value in [display] no longer discards the rest of config.toml", + "Reasoning text with multi-byte characters no longer crashes the TUI", + "Cursor provider uses the correct regional agent host instead of hardcoding global", + "Anthropic reasoning effort chosen with Ctrl+O in /model now persists", + "launch_hotkeys enabled = false is honoured on macOS", + "The API key login prompt visibly responds and tells you when a key already exists", + "Desktop no longer shows a false 'update ready' banner on dev builds", + "Builds on musl and Termux/aarch64 targets are fixed" + ] +} diff --git a/crates/jcode-app-core/src/agent.rs b/crates/jcode-app-core/src/agent.rs index 70eb9a1061..5160b87934 100644 --- a/crates/jcode-app-core/src/agent.rs +++ b/crates/jcode-app-core/src/agent.rs @@ -803,9 +803,20 @@ impl Agent { let mut missing_for_message = Vec::new(); for id in tool_uses { self.tool_call_ids.insert(id.clone()); - if !self.tool_result_ids.contains(&id) { - missing_for_message.push(id); + if self.tool_result_ids.contains(&id) { + continue; } + // A tool that is still executing is not an interrupted tool: + // its real result is on the way, and synthesizing a + // placeholder now produces a duplicate tool_result that + // Anthropic rejects outright. See `tool::inflight`. + if crate::tool::inflight::is_tool_in_flight(&id) { + logging::info(&format!( + "Skipping missing tool-output repair for {id}: tool is still executing" + )); + continue; + } + missing_for_message.push(id); } if !missing_for_message.is_empty() { missing_repairs.push((index, missing_for_message)); diff --git a/crates/jcode-app-core/src/agent_tests.rs b/crates/jcode-app-core/src/agent_tests.rs index 64ed7d1eca..4f1b419b89 100644 --- a/crates/jcode-app-core/src/agent_tests.rs +++ b/crates/jcode-app-core/src/agent_tests.rs @@ -1583,3 +1583,58 @@ async fn stranded_tool_use_stop_continues_instead_of_ending_the_turn() { "the recovered turn must deliver the model's real completion, got {text:?}" ); } + +/// Regression: the missing tool-output repair must not synthesize a +/// placeholder result for a tool call that is still executing. Doing so +/// produced a duplicate `tool_result` once the real output landed, and +/// Anthropic then rejected every later request for that session with +/// "unexpected `tool_use_id` found in `tool_result` blocks". +#[tokio::test] +async fn repair_skips_tool_calls_that_are_still_executing() { + let _guard = crate::storage::lock_test_env(); + let provider: Arc = Arc::new(NativeAutoCompactionProvider); + let registry = Registry::new(provider.clone()).await; + let mut agent = Agent::new(provider, registry); + + let tool_id = "toolu_still_running_regression"; + agent.session.add_message( + Role::User, + vec![ContentBlock::Text { + text: "run it".to_string(), + cache_control: None, + }], + ); + agent.session.add_message( + Role::Assistant, + vec![ContentBlock::ToolUse { + id: tool_id.to_string(), + name: "bash".to_string(), + input: serde_json::json!({"command": "sleep 300"}), + thought_signature: None, + }], + ); + + let in_flight = crate::tool::inflight::mark_tool_in_flight(tool_id).expect("guard"); + assert_eq!( + agent.repair_missing_tool_outputs(), + 0, + "a running tool must not be repaired" + ); + assert!( + !agent.session.messages.iter().any(|m| m + .content + .iter() + .any(|b| matches!(b, ContentBlock::ToolResult { .. }))), + "no synthetic tool_result may be inserted while the tool runs" + ); + + // Once the tool finishes without delivering a result (a genuine + // interruption), repair is allowed to step in. + drop(in_flight); + agent.reset_tool_output_tracking(); + assert_eq!( + agent.repair_missing_tool_outputs(), + 1, + "a finished-but-unanswered tool must still be repaired" + ); +} diff --git a/crates/jcode-app-core/src/tool/apply_patch.rs b/crates/jcode-app-core/src/tool/apply_patch.rs index 0b47795eeb..f5d4618f50 100644 --- a/crates/jcode-app-core/src/tool/apply_patch.rs +++ b/crates/jcode-app-core/src/tool/apply_patch.rs @@ -81,6 +81,11 @@ impl Tool for ApplyPatchTool { let params: ApplyPatchInput = serde_json::from_value(input)?; let hunks = parse_apply_patch(¶ms.patch_text)?; + // A patch can reach config.toml through any hunk kind (add, update, + // move), so watch the file across the whole invocation rather than + // threading before/after content through each branch. + let config_watch = super::config_edit_notice::ConfigEditWatch::begin(); + let mut results = Vec::new(); let mut touched_paths = Vec::new(); @@ -236,7 +241,9 @@ impl Tool for ApplyPatchTool { if results.is_empty() { Ok(ToolOutput::new("No changes applied")) } else { - let output = ToolOutput::new(results.join("\n")); + let mut body = results.join("\n"); + config_watch.finish(&mut body); + let output = ToolOutput::new(body); if touched_paths.len() == 1 { Ok(output.with_title(touched_paths[0].clone())) } else { diff --git a/crates/jcode-app-core/src/tool/config_edit_notice.rs b/crates/jcode-app-core/src/tool/config_edit_notice.rs new file mode 100644 index 0000000000..8202519d12 --- /dev/null +++ b/crates/jcode-app-core/src/tool/config_edit_notice.rs @@ -0,0 +1,118 @@ +//! Tell the agent (and through it, the user) what a config.toml edit did. +//! +//! When a user asks jcode to change a setting, the agent writes +//! `~/.jcode/config.toml` and then has to guess whether the change took +//! effect. That guess is where the confusion comes from. Instead, every file +//! write that lands on the active config file appends an explicit report: +//! which keys changed, and whether each one is live in running sessions or +//! needs a restart. + +use std::path::Path; + +/// Resolve a path for comparison, falling back to the path as given. +/// +/// A config file that does not exist yet cannot be canonicalized, and that is +/// a normal case here (the very first write creates it), so the unresolved +/// path is the correct answer rather than an error to report. +fn comparable(path: &Path) -> std::path::PathBuf { + match std::fs::canonicalize(path) { + Ok(resolved) => resolved, + Err(_) => path.to_path_buf(), + } +} + +/// Whether `path` is the config file the running process actually reads. +/// +/// Compares resolved paths so `~/.jcode/config.toml`, a relative path, and a +/// symlinked jcode home all resolve to the same file. +fn is_active_config_file(path: &Path) -> bool { + let Some(config_path) = crate::config::Config::path() else { + return false; + }; + comparable(path) == comparable(&config_path) +} + +/// Report appended to a tool result after a write to the active config file. +/// +/// Returns `None` for non-config files and for edits that changed no settings +/// (comments or formatting), so ordinary writes stay untouched. +pub fn config_edit_notice(path: &Path, before: &str, after: &str) -> Option { + if !is_active_config_file(path) { + return None; + } + // Force the next config() call to re-read instead of waiting out the + // staleness throttle, so "live now" is true the moment it is claimed. + crate::config::Config::invalidate_cache(); + + // A config file that no longer parses is silently ignored by + // `Config::load`, which falls back to defaults. That is the worst possible + // outcome to leave unreported: the write "succeeded" while every setting + // in the file quietly stopped applying. Surface it instead. + if let Err(error) = crate::config::Config::load_strict() { + return Some(format!( + "\n\nWARNING: {} no longer parses as TOML, so jcode is falling back to \ + default settings and every setting in this file is being ignored. \ + Fix the syntax error: {error}", + path.display() + )); + } + + let summary = crate::config::change_report::summarize_toml_change(before, after)?; + Some(format!("\n\n{summary}")) +} + +/// Append [`config_edit_notice`] to a tool output body when applicable. +pub fn append_config_edit_notice(body: &mut String, path: &Path, before: &str, after: &str) { + if let Some(notice) = config_edit_notice(path, before, after) { + body.push_str(¬ice); + } +} + +/// Read the config file, treating "absent or unreadable" as empty. +/// +/// An absent config file is the normal pre-state for the write that creates +/// it, and an unreadable one is reported by the change summary itself, so +/// there is no error here worth propagating: empty is the meaningful value. +fn read_config_text(path: &Path) -> String { + std::fs::read_to_string(path).unwrap_or_default() +} + +/// Watches the active config file across a whole tool invocation. +/// +/// Tools that touch several files, or write through several code paths (patch +/// application, moves, deletes), cannot easily thread before/after content to +/// the place that builds the result string. This captures the config file +/// content up front and re-reads it at the end, so a config edit is reported +/// no matter which path produced it. +pub struct ConfigEditWatch { + path: Option, + before: String, +} + +impl ConfigEditWatch { + /// Snapshot the active config file before a tool runs. + pub fn begin() -> Self { + let path = crate::config::Config::path(); + let before = match path.as_deref() { + Some(path) => read_config_text(path), + None => String::new(), + }; + Self { path, before } + } + + /// Append a change report if the config file changed while the tool ran. + pub fn finish(self, body: &mut String) { + let Some(path) = self.path else { + return; + }; + let after = read_config_text(&path); + if after == self.before { + return; + } + append_config_edit_notice(body, &path, &self.before, &after); + } +} + +#[cfg(test)] +#[path = "config_edit_notice_tests.rs"] +mod tests; diff --git a/crates/jcode-app-core/src/tool/config_edit_notice_tests.rs b/crates/jcode-app-core/src/tool/config_edit_notice_tests.rs new file mode 100644 index 0000000000..7ab7f51729 --- /dev/null +++ b/crates/jcode-app-core/src/tool/config_edit_notice_tests.rs @@ -0,0 +1,237 @@ +use super::*; + +/// Point the process at a temp jcode home and return it with a restore guard. +fn temp_jcode_home() -> (tempfile::TempDir, Option) { + let dir = tempfile::TempDir::new().expect("tempdir"); + let prev = std::env::var_os("JCODE_HOME"); + crate::env::set_var("JCODE_HOME", dir.path()); + crate::config::Config::invalidate_cache(); + (dir, prev) +} + +fn restore_jcode_home(prev: Option) { + if let Some(prev) = prev { + crate::env::set_var("JCODE_HOME", prev); + } else { + crate::env::remove_var("JCODE_HOME"); + } + crate::config::Config::invalidate_cache(); +} + +#[test] +fn writing_the_active_config_reports_what_changed() { + let _guard = crate::storage::lock_test_env(); + let (_dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + std::fs::write(&path, "[keybindings]\nscroll_up = \"ctrl+y\"\n").expect("write"); + + let notice = config_edit_notice( + &path, + "[keybindings]\nscroll_up = \"ctrl+k\"\n", + "[keybindings]\nscroll_up = \"ctrl+y\"\n", + ) + .expect("an active-config edit should be reported"); + + assert!(notice.contains("keybindings.scroll_up"), "{notice}"); + assert!(notice.contains("live now"), "{notice}"); + + restore_jcode_home(prev); +} + +#[test] +fn writing_an_unrelated_file_reports_nothing() { + let _guard = crate::storage::lock_test_env(); + let (dir, prev) = temp_jcode_home(); + + let other = dir.path().join("notes.toml"); + std::fs::write(&other, "[display]\ncentered = true\n").expect("write"); + + assert!( + config_edit_notice(&other, "", "[display]\ncentered = true\n").is_none(), + "only the active config file should get a change report" + ); + + restore_jcode_home(prev); +} + +#[test] +fn comment_only_config_edit_reports_nothing() { + let _guard = crate::storage::lock_test_env(); + let (_dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + let after = "# note\n[display]\ncentered = true\n"; + std::fs::write(&path, after).expect("write"); + + assert!( + config_edit_notice(&path, "[display]\ncentered = true\n", after).is_none(), + "a comment-only edit must not claim a settings change" + ); + + restore_jcode_home(prev); +} + +#[test] +fn restart_required_sections_say_so() { + let _guard = crate::storage::lock_test_env(); + let (_dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + let after = "[gateway]\nport = 8888\n"; + std::fs::write(&path, after).expect("write"); + + let notice = + config_edit_notice(&path, "[gateway]\nport = 7777\n", after).expect("report expected"); + assert!( + notice.contains("Restart required for: gateway.port"), + "{notice}" + ); + + restore_jcode_home(prev); +} + +#[test] +fn the_notice_leaves_the_config_cache_current() { + let _guard = crate::storage::lock_test_env(); + let (_dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + std::fs::write(&path, "[display]\ncentered = false\n").expect("write"); + assert!(!crate::config::config().display.centered); + + // Rewrite immediately: without the notice's explicit invalidation this can + // land inside the config cache's staleness throttle. + let after = "[display]\ncentered = true\n"; + std::fs::write(&path, after).expect("rewrite"); + let notice = + config_edit_notice(&path, "[display]\ncentered = false\n", after).expect("report expected"); + + assert!(notice.contains("live now"), "{notice}"); + assert!( + crate::config::config().display.centered, + "claiming 'live now' requires the config cache to already reflect the edit" + ); + + restore_jcode_home(prev); +} + +#[test] +fn a_config_write_that_breaks_toml_syntax_is_reported_loudly() { + let _guard = crate::storage::lock_test_env(); + let (_dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + let broken = "[display\ncentered = true\n"; + std::fs::write(&path, broken).expect("write"); + + let notice = config_edit_notice(&path, "[display]\ncentered = true\n", broken) + .expect("a config file that stopped parsing must never be silent"); + assert!(notice.contains("WARNING"), "{notice}"); + assert!(notice.contains("no longer parses"), "{notice}"); + + restore_jcode_home(prev); +} + +/// End-to-end through the real `write` tool: the path an agent actually takes +/// when a user says "change this setting". +#[tokio::test] +async fn the_write_tool_reports_config_changes_end_to_end() { + use crate::tool::{Tool, ToolContext}; + + let _guard = crate::storage::lock_test_env(); + let (dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + std::fs::write( + &path, + "[display]\ncentered = false\n\n[gateway]\nport = 7777\n", + ) + .expect("seed config"); + assert!(!crate::config::config().display.centered); + + let ctx = ToolContext { + session_id: "test".to_string(), + message_id: "test".to_string(), + tool_call_id: "test".to_string(), + working_dir: Some(dir.path().to_path_buf()), + stdin_request_tx: None, + graceful_shutdown_signal: None, + execution_mode: crate::tool::ToolExecutionMode::Direct, + }; + + let output = crate::tool::write::WriteTool + .execute( + serde_json::json!({ + "file_path": path.to_string_lossy(), + "content": "[display]\ncentered = true\n\n[gateway]\nport = 8888\n", + }), + ctx, + ) + .await + .expect("write should succeed"); + + let body = output.output; + assert!(body.contains("display.centered"), "{body}"); + assert!(body.contains("live now"), "{body}"); + assert!( + body.contains("Restart required for: gateway.port"), + "{body}" + ); + assert!( + crate::config::config().display.centered, + "the display change should be live in-process immediately after the write" + ); + + restore_jcode_home(prev); +} + +/// `apply_patch` reaches config.toml through its own write paths, so it gets +/// the same report as write/edit. +#[tokio::test] +async fn apply_patch_reports_config_changes() { + use crate::tool::{Tool, ToolContext}; + + let _guard = crate::storage::lock_test_env(); + let (dir, prev) = temp_jcode_home(); + + let path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("create parent"); + std::fs::write(&path, "[display]\ncentered = false\n").expect("seed config"); + assert!(!crate::config::config().display.centered); + + let ctx = ToolContext { + session_id: "test".to_string(), + message_id: "test".to_string(), + tool_call_id: "test".to_string(), + working_dir: Some(dir.path().to_path_buf()), + stdin_request_tx: None, + graceful_shutdown_signal: None, + execution_mode: crate::tool::ToolExecutionMode::Direct, + }; + + let patch_text = format!( + "*** Begin Patch\n*** Update File: {}\n@@\n-centered = false\n+centered = true\n*** End Patch\n", + path.display() + ); + let output = crate::tool::apply_patch::ApplyPatchTool + .execute(serde_json::json!({ "patch_text": patch_text }), ctx) + .await + .expect("patch should apply"); + + let body = output.output; + assert!(body.contains("display.centered"), "{body}"); + assert!(body.contains("live now"), "{body}"); + assert!( + crate::config::config().display.centered, + "the patched setting should be live immediately" + ); + + restore_jcode_home(prev); +} diff --git a/crates/jcode-app-core/src/tool/discover.rs b/crates/jcode-app-core/src/tool/discover.rs index 18b1c43f3d..b92a7e85bf 100644 --- a/crates/jcode-app-core/src/tool/discover.rs +++ b/crates/jcode-app-core/src/tool/discover.rs @@ -1,3 +1,4 @@ +use super::discover_secrets::contains_recognizable_secret; use super::{Tool, ToolContext, ToolExecutionMode, ToolOutput}; use anyhow::Result; use async_trait::async_trait; @@ -30,6 +31,34 @@ const DISCOVERY_QUERY_MAX_CHARS: usize = 500; const DISCOVERY_REASON_MIN_CHARS: usize = 40; const DISCOVERY_REASON_MAX_CHARS: usize = 2_000; +/// Telemetry reason for a `select` naming an entry the catalog does not carry. +/// Kept distinct from transport failures so the rate of agents committing to +/// off-catalog products is measurable rather than hidden in `http_error`. +const OFF_CATALOG_FAILURE_REASON: &str = "off_catalog_select"; + +/// True when a select response carries no usable tool entry (`{}`, +/// `{"tool": null}`, or an empty object), which endpoints use instead of 404. +fn listing_has_no_tool_entry(listing: &Value) -> bool { + match listing.get("tool") { + None | Some(Value::Null) => true, + Some(Value::Object(entry)) => entry.is_empty(), + Some(_) => false, + } +} + +/// Error shown when a select names something outside the catalog. It tells the +/// agent the two legitimate recoveries: pick a listed entry, or record the gap. +fn off_catalog_select_error(category: &str, tool_name: &str) -> anyhow::Error { + anyhow::anyhow!( + "'{tool_name}' is not in the Jcode catalog for '{category}'. Only entries returned by \ + action `browse` can be selected; this name did not come from a listing. Either select \ + one of the listed entries, or, if none fits, call action `suggest` with \ + `suggestion_kind: known_product`, `product_name: {tool_name}`, and the \ + `prior_request_id` from your browse so maintainers see the gap. Do not install or \ + configure '{tool_name}' from memory as if Discovery had vetted it." + ) +} + fn discovery_benchmark_run() -> bool { std::env::var(DISCOVERY_BENCHMARK_ENV) .ok() @@ -351,192 +380,6 @@ fn has_sufficient_detail(value: &str, field: &str) -> bool { words.len() >= min_words && unique.len() >= min_unique } -/// A deliberately high-confidence last-line defense before model-authored -/// Discovery text leaves the client. This complements, rather than replaces, -/// the schema instruction to summarize the need instead of copying user data. -fn contains_recognizable_secret(value: &str) -> bool { - let lower = value.to_ascii_lowercase(); - if (lower.contains("-----begin ") && lower.contains("private key-----")) - || contains_credential_assignment(&lower) - || contains_email_address(value) - || contains_ssn(value) - || contains_credential_url(value) - || contains_international_phone_number(value) - { - return true; - } - - if contains_prefixed_secret(value) || contains_payment_card_sequence(value) { - return true; - } - - value.split_whitespace().any(|token| { - let token = token.trim_matches(|c: char| { - matches!( - c, - '"' | '\'' | '`' | '(' | ')' | '[' | ']' | '{' | '}' | ',' | ';' - ) - }); - looks_like_jwt(token) - }) || contains_bearer_token(&lower) -} - -fn contains_prefixed_secret(value: &str) -> bool { - const SECRET_PREFIXES: &[&str] = &[ - "sk_live_", - "rk_live_", - "sk_test_", - "rk_test_", - "sk-proj-", - "ghp_", - "gho_", - "ghu_", - "ghs_", - "github_pat_", - "xoxb-", - "xoxp-", - "xoxa-", - "xoxr-", - "npm_", - "jck_live_", - ]; - value.split_whitespace().any(|token| { - let token = token.trim_matches(|c: char| !c.is_ascii_alphanumeric() && !"_-".contains(c)); - let lower = token.to_ascii_lowercase(); - SECRET_PREFIXES - .iter() - .any(|prefix| lower.starts_with(prefix) && token.len() >= prefix.len() + 8) - || (token.starts_with("AKIA") && token.len() == 20) - || (token.starts_with("AIza") && token.len() >= 35) - }) -} - -fn contains_credential_assignment(lower: &str) -> bool { - const LABELS: &[&str] = &[ - "api_key", - "api-key", - "apikey", - "access_token", - "auth_token", - "client_secret", - "secret_key", - "password", - "passwd", - ]; - LABELS.iter().any(|label| { - lower.match_indices(label).any(|(index, _)| { - let rest = &lower[index + label.len()..]; - let rest = rest.trim_start(); - let Some(rest) = rest.strip_prefix(['=', ':']) else { - return false; - }; - let candidate = - rest.trim_start_matches(|c: char| c.is_whitespace() || "'\"`".contains(c)); - candidate - .split(|c: char| c.is_whitespace() || "'\"`,;".contains(c)) - .next() - .is_some_and(|token| token.len() >= 8) - }) - }) -} - -fn contains_bearer_token(lower: &str) -> bool { - lower.match_indices("bearer ").any(|(index, _)| { - lower[index + "bearer ".len()..] - .split_whitespace() - .next() - .is_some_and(|token| token.trim_matches(|c: char| ",;.'\"`".contains(c)).len() >= 12) - }) -} - -fn contains_email_address(value: &str) -> bool { - value.split_whitespace().any(|token| { - let token = token.trim_matches(|c: char| ",;:()[]{}<>\"'`".contains(c)); - let Some((local, domain)) = token.split_once('@') else { - return false; - }; - !local.is_empty() - && domain - .rsplit_once('.') - .is_some_and(|(host, suffix)| !host.is_empty() && suffix.len() >= 2) - }) -} - -fn contains_ssn(value: &str) -> bool { - value.split_whitespace().any(|token| { - let token = token.trim_matches(|c: char| !c.is_ascii_digit() && c != '-'); - let parts: Vec<&str> = token.split('-').collect(); - parts.len() == 3 - && parts[0].len() == 3 - && parts[1].len() == 2 - && parts[2].len() == 4 - && parts - .iter() - .all(|part| part.chars().all(|c| c.is_ascii_digit())) - }) -} - -fn contains_credential_url(value: &str) -> bool { - value.split_whitespace().any(|token| { - let Some((_, rest)) = token.split_once("://") else { - return false; - }; - let authority = rest.split('/').next().unwrap_or_default(); - authority.contains('@') - && authority - .split('@') - .next() - .is_some_and(|user| user.contains(':')) - }) -} - -fn contains_international_phone_number(value: &str) -> bool { - value.split_whitespace().any(|token| { - if !token.starts_with('+') { - return false; - } - let digits = token.chars().filter(|c| c.is_ascii_digit()).count(); - (10..=15).contains(&digits) - && token - .chars() - .all(|c| c.is_ascii_digit() || "+-().".contains(c)) - }) -} - -fn looks_like_jwt(token: &str) -> bool { - token.len() >= 40 && token.starts_with("eyJ") && token.matches('.').count() == 2 -} - -fn contains_payment_card_sequence(value: &str) -> bool { - value - .split(|c: char| !c.is_ascii_digit() && c != '-' && c != ' ') - .any(|candidate| looks_like_payment_card(candidate.trim())) -} - -fn looks_like_payment_card(candidate: &str) -> bool { - let digits: String = candidate.chars().filter(|c| c.is_ascii_digit()).collect(); - if !(13..=19).contains(&digits.len()) - || candidate - .chars() - .any(|c| !c.is_ascii_digit() && c != '-' && c != ' ') - { - return false; - } - let mut sum = 0u32; - let parity = digits.len() % 2; - for (index, byte) in digits.bytes().enumerate() { - let mut digit = u32::from(byte - b'0'); - if index % 2 == parity { - digit *= 2; - if digit > 9 { - digit -= 9; - } - } - sum += digit; - } - sum.is_multiple_of(10) -} - #[async_trait] impl Tool for DiscoverToolsTool { fn name(&self) -> &str { @@ -544,9 +387,28 @@ 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." + "Find and set up an external product, service, dev tool, API, or data source you \ + do not already have a tool for. Catalog entries are vetted for one-call setup, so \ + going through this tool is usually faster than wiring a vendor up by hand.\n\ + \n\ + Call it in two places:\n\ + 1. browse, BEFORE the first step that assumes an external provider. Concretely: \ + before installing a vendor SDK or CLI, before writing vendor API calls or \ + vendor-specific config, before fetching a vendor's docs, signup, or pricing page, \ + before connecting an MCP server, and before telling the user which provider to \ + use. Browsing is cheap, read-only, and commits to nothing.\n\ + 2. select, once you settle on one entry, before you act on it. Setup instructions \ + live in the select response, so skipping it means working from guesses.\n\ + \n\ + Do not call it for work that stays inside this machine and this repo: editing or \ + refactoring code, running tests or builds, local files and local databases, git, \ + shell one-liners, writing docs or copy, or answering from knowledge. It is also \ + unnecessary when an existing tool already covers the need.\n\ + \n\ + If nothing in the listing fits, say so and continue with your own approach; \ + action=suggest records the gap. Recommend the entry that actually fits the task. \ + Some providers share revenue with Jcode, and that never makes an entry the right \ + answer." } fn parameters_schema(&self) -> Value { @@ -559,7 +421,7 @@ impl Tool for DiscoverToolsTool { "action": { "type": "string", "enum": ["browse", "select", "suggest"], - "description": "Phase. Defaults to select when `tool` is set, else browse. Suggest only after browse fails." + "description": "browse to compare; select the one you commit to (it carries setup); suggest if none fit." }, "category": { "type": "string", @@ -824,6 +686,29 @@ impl Tool for DiscoverToolsTool { let fetched = match fetch_listing(&discovery_request, Some(&tool_name)).await { Ok(result) => result, Err(err) => { + // A 404 on select means the agent committed to a name the + // catalog does not carry (usually a product it recalled + // from training, not one it saw in browse). That is a + // distinct behavior from a broken endpoint, so it gets its + // own outcome and its own recovery instruction. + if err.http_status == Some(404) { + record_discovery_telemetry( + &request_id, + started_at, + &endpoint, + "select", + Some(&category), + Some(tool_name.as_str()), + "off_catalog_select", + Some(OFF_CATALOG_FAILURE_REASON), + err.http_status, + err.response_bytes, + Some(0), + query_present, + reason_present, + ); + return Err(off_catalog_select_error(&category, &tool_name)); + } record_discovery_telemetry( &request_id, started_at, @@ -842,6 +727,26 @@ impl Tool for DiscoverToolsTool { return Err(err.into()); } }; + // Endpoints may also answer 200 with an empty entry. Same meaning: + // the selected name is not in the catalog. + if listing_has_no_tool_entry(&fetched.listing) { + record_discovery_telemetry( + &request_id, + started_at, + &endpoint, + "select", + Some(&category), + Some(tool_name.as_str()), + "off_catalog_select", + Some(OFF_CATALOG_FAILURE_REASON), + Some(fetched.http_status), + Some(fetched.response_bytes), + Some(0), + query_present, + reason_present, + ); + return Err(off_catalog_select_error(&category, &tool_name)); + } let rendered = match render_selection(&category, &tool_name, &fetched.listing) { Ok(rendered) => rendered, Err(err) => { @@ -1361,17 +1266,15 @@ fn render_listing(category: &str, listing: &Value, request_id: &str) -> Result bool { + let lower = value.to_ascii_lowercase(); + if (lower.contains("-----begin ") && lower.contains("private key-----")) + || contains_credential_assignment(&lower) + || contains_email_address(value) + || contains_ssn(value) + || contains_credential_url(value) + || contains_international_phone_number(value) + { + return true; + } + + if contains_prefixed_secret(value) || contains_payment_card_sequence(value) { + return true; + } + + value.split_whitespace().any(|token| { + let token = token.trim_matches(|c: char| { + matches!( + c, + '"' | '\'' | '`' | '(' | ')' | '[' | ']' | '{' | '}' | ',' | ';' + ) + }); + looks_like_jwt(token) + }) || contains_bearer_token(&lower) +} + +fn contains_prefixed_secret(value: &str) -> bool { + const SECRET_PREFIXES: &[&str] = &[ + "sk_live_", + "rk_live_", + "sk_test_", + "rk_test_", + "sk-proj-", + "ghp_", + "gho_", + "ghu_", + "ghs_", + "github_pat_", + "xoxb-", + "xoxp-", + "xoxa-", + "xoxr-", + "npm_", + "jck_live_", + ]; + value.split_whitespace().any(|token| { + let token = token.trim_matches(|c: char| !c.is_ascii_alphanumeric() && !"_-".contains(c)); + let lower = token.to_ascii_lowercase(); + SECRET_PREFIXES + .iter() + .any(|prefix| lower.starts_with(prefix) && token.len() >= prefix.len() + 8) + || (token.starts_with("AKIA") && token.len() == 20) + || (token.starts_with("AIza") && token.len() >= 35) + }) +} + +fn contains_credential_assignment(lower: &str) -> bool { + const LABELS: &[&str] = &[ + "api_key", + "api-key", + "apikey", + "access_token", + "auth_token", + "client_secret", + "secret_key", + "password", + "passwd", + ]; + LABELS.iter().any(|label| { + lower.match_indices(label).any(|(index, _)| { + let rest = &lower[index + label.len()..]; + let rest = rest.trim_start(); + let Some(rest) = rest.strip_prefix(['=', ':']) else { + return false; + }; + let candidate = + rest.trim_start_matches(|c: char| c.is_whitespace() || "'\"`".contains(c)); + candidate + .split(|c: char| c.is_whitespace() || "'\"`,;".contains(c)) + .next() + .is_some_and(|token| token.len() >= 8) + }) + }) +} + +fn contains_bearer_token(lower: &str) -> bool { + lower.match_indices("bearer ").any(|(index, _)| { + lower[index + "bearer ".len()..] + .split_whitespace() + .next() + .is_some_and(|token| token.trim_matches(|c: char| ",;.'\"`".contains(c)).len() >= 12) + }) +} + +fn contains_email_address(value: &str) -> bool { + value.split_whitespace().any(|token| { + let token = token.trim_matches(|c: char| ",;:()[]{}<>\"'`".contains(c)); + let Some((local, domain)) = token.split_once('@') else { + return false; + }; + !local.is_empty() + && domain + .rsplit_once('.') + .is_some_and(|(host, suffix)| !host.is_empty() && suffix.len() >= 2) + }) +} + +fn contains_ssn(value: &str) -> bool { + value.split_whitespace().any(|token| { + let token = token.trim_matches(|c: char| !c.is_ascii_digit() && c != '-'); + let parts: Vec<&str> = token.split('-').collect(); + parts.len() == 3 + && parts[0].len() == 3 + && parts[1].len() == 2 + && parts[2].len() == 4 + && parts + .iter() + .all(|part| part.chars().all(|c| c.is_ascii_digit())) + }) +} + +fn contains_credential_url(value: &str) -> bool { + value.split_whitespace().any(|token| { + let Some((_, rest)) = token.split_once("://") else { + return false; + }; + let authority = rest.split('/').next().unwrap_or_default(); + authority.contains('@') + && authority + .split('@') + .next() + .is_some_and(|user| user.contains(':')) + }) +} + +fn contains_international_phone_number(value: &str) -> bool { + value.split_whitespace().any(|token| { + if !token.starts_with('+') { + return false; + } + let digits = token.chars().filter(|c| c.is_ascii_digit()).count(); + (10..=15).contains(&digits) + && token + .chars() + .all(|c| c.is_ascii_digit() || "+-().".contains(c)) + }) +} + +fn looks_like_jwt(token: &str) -> bool { + token.len() >= 40 && token.starts_with("eyJ") && token.matches('.').count() == 2 +} + +fn contains_payment_card_sequence(value: &str) -> bool { + value + .split(|c: char| !c.is_ascii_digit() && c != '-' && c != ' ') + .any(|candidate| looks_like_payment_card(candidate.trim())) +} + +fn looks_like_payment_card(candidate: &str) -> bool { + let digits: String = candidate.chars().filter(|c| c.is_ascii_digit()).collect(); + if !(13..=19).contains(&digits.len()) + || candidate + .chars() + .any(|c| !c.is_ascii_digit() && c != '-' && c != ' ') + { + return false; + } + let mut sum = 0u32; + let parity = digits.len() % 2; + for (index, byte) in digits.bytes().enumerate() { + let mut digit = u32::from(byte - b'0'); + if index % 2 == parity { + digit *= 2; + if digit > 9 { + digit -= 9; + } + } + sum += digit; + } + sum.is_multiple_of(10) +} diff --git a/crates/jcode-app-core/src/tool/edit.rs b/crates/jcode-app-core/src/tool/edit.rs index 17a235b133..f1444706c5 100644 --- a/crates/jcode-app-core/src/tool/edit.rs +++ b/crates/jcode-app-core/src/tool/edit.rs @@ -140,11 +140,18 @@ impl Tool for EditTool { let end_line = start_line + params.new_string.lines().count().saturating_sub(1); let context = extract_context(&new_content, start_line, end_line, 3); - Ok(ToolOutput::new(format!( + let mut body = format!( "Edited {}: replaced {} occurrence(s)\n{}\n\nContext after edit (lines {}-{}):\n{}", params.file_path, occurrences, diff, context.0, context.1, context.2 - )) - .with_title(params.file_path.clone())) + ); + super::config_edit_notice::append_config_edit_notice( + &mut body, + &path, + &content, + &new_content, + ); + + Ok(ToolOutput::new(body).with_title(params.file_path.clone())) } } diff --git a/crates/jcode-app-core/src/tool/inflight.rs b/crates/jcode-app-core/src/tool/inflight.rs new file mode 100644 index 0000000000..95941b1d86 --- /dev/null +++ b/crates/jcode-app-core/src/tool/inflight.rs @@ -0,0 +1,110 @@ +//! Process-global registry of tool calls that are currently executing. +//! +//! Why this exists: the "missing tool output" repair paths treat an assistant +//! `tool_use` with no matching `tool_result` as evidence of an interrupted +//! turn and insert a synthetic placeholder result. That inference is wrong +//! while the tool is *still running*. A confirmed production wedge +//! (session_clover_1785560899476): a 106s `bash` call was mid-flight when a +//! scheduled-task wakeup drove another turn on the same session. Repair +//! injected a placeholder result, the real output landed 28s later as a second +//! `tool_result` for the same `tool_use_id`, and Anthropic then rejected every +//! subsequent request with: +//! +//! ```text +//! unexpected `tool_use_id` found in `tool_result` blocks: +//! ``` +//! +//! leaving the session permanently unsendable. Repair must therefore skip any +//! tool call that is still executing; its real result is on the way. +//! +//! `Registry::execute` registers each call here for the duration of its +//! execution via an RAII guard, so entries can never leak past a panic, +//! cancellation, or early return. + +use std::collections::HashMap; +use std::sync::{LazyLock, Mutex}; + +/// Reference counts per tool_call_id. A count (rather than a set) keeps the +/// registry correct if the same id is somehow executed concurrently, e.g. a +/// retry racing the original. +static IN_FLIGHT: LazyLock>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// RAII registration for one executing tool call. +pub struct InFlightToolGuard { + tool_call_id: String, +} + +impl Drop for InFlightToolGuard { + fn drop(&mut self) { + let Ok(mut map) = IN_FLIGHT.lock() else { + return; + }; + if let Some(count) = map.get_mut(&self.tool_call_id) { + *count = count.saturating_sub(1); + if *count == 0 { + map.remove(&self.tool_call_id); + } + } + } +} + +/// Mark `tool_call_id` as executing until the returned guard is dropped. +/// Empty ids are not tracked (nothing can match them during repair). +pub fn mark_tool_in_flight(tool_call_id: &str) -> Option { + if tool_call_id.is_empty() { + return None; + } + let mut map = IN_FLIGHT.lock().ok()?; + *map.entry(tool_call_id.to_string()).or_insert(0) += 1; + Some(InFlightToolGuard { + tool_call_id: tool_call_id.to_string(), + }) +} + +/// True while a tool call with this id is executing somewhere in this process. +pub fn is_tool_in_flight(tool_call_id: &str) -> bool { + IN_FLIGHT + .lock() + .map(|map| map.contains_key(tool_call_id)) + .unwrap_or(false) +} + +/// Number of tool calls currently executing (diagnostics only). +pub fn in_flight_tool_count() -> usize { + IN_FLIGHT.lock().map(|map| map.len()).unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn guard_tracks_and_releases() { + let id = "toolu_inflight_test_basic"; + assert!(!is_tool_in_flight(id)); + { + let _guard = mark_tool_in_flight(id).expect("guard"); + assert!(is_tool_in_flight(id)); + } + assert!(!is_tool_in_flight(id)); + } + + #[test] + fn nested_guards_release_only_after_the_last_one() { + let id = "toolu_inflight_test_nested"; + let outer = mark_tool_in_flight(id).expect("guard"); + let inner = mark_tool_in_flight(id).expect("guard"); + assert!(is_tool_in_flight(id)); + drop(inner); + assert!(is_tool_in_flight(id), "one registration still outstanding"); + drop(outer); + assert!(!is_tool_in_flight(id)); + } + + #[test] + fn empty_ids_are_not_tracked() { + assert!(mark_tool_in_flight("").is_none()); + assert!(!is_tool_in_flight("")); + } +} diff --git a/crates/jcode-app-core/src/tool/mod.rs b/crates/jcode-app-core/src/tool/mod.rs index f76b7fba78..07b61f714e 100644 --- a/crates/jcode-app-core/src/tool/mod.rs +++ b/crates/jcode-app-core/src/tool/mod.rs @@ -8,12 +8,15 @@ mod browser; mod communicate; #[cfg(target_os = "macos")] mod computer; +mod config_edit_notice; mod conversation_search; mod debug_socket; mod discover; +mod discover_secrets; mod edit; mod gmail; mod goal; +pub mod inflight; mod invalid; mod ls; pub mod mcp; @@ -542,6 +545,11 @@ impl Registry { /// Execute a tool by name pub async fn execute(&self, name: &str, input: Value, ctx: ToolContext) -> Result { + // Mark this call in-flight for the whole execution so the missing + // tool-output repair paths do not mistake a slow tool for an + // interrupted one and inject a duplicate synthetic result. See + // `tool::inflight`. + let _in_flight = inflight::mark_tool_in_flight(&ctx.tool_call_id); let tools = self.tools.read().await; let resolved_name = Self::resolve_tool_name(name); if let Some(policy) = session_tool_policy(&ctx.session_id) { diff --git a/crates/jcode-app-core/src/tool/multiedit.rs b/crates/jcode-app-core/src/tool/multiedit.rs index 7d856f988f..f845791bcc 100644 --- a/crates/jcode-app-core/src/tool/multiedit.rs +++ b/crates/jcode-app-core/src/tool/multiedit.rs @@ -157,6 +157,13 @@ impl Tool for MultiEditTool { output.push_str(&generate_diff_summary(&original_content, &content)); } + super::config_edit_notice::append_config_edit_notice( + &mut output, + &path, + &original_content, + &content, + ); + Ok(ToolOutput::new(output).with_title(params.file_path.clone())) } } diff --git a/crates/jcode-app-core/src/tool/patch.rs b/crates/jcode-app-core/src/tool/patch.rs index ecda739aaa..69ed9b617d 100644 --- a/crates/jcode-app-core/src/tool/patch.rs +++ b/crates/jcode-app-core/src/tool/patch.rs @@ -67,6 +67,9 @@ impl Tool for PatchTool { return Err(anyhow::anyhow!("No valid patches found in input")); } + // Watch config.toml across the whole invocation so an edit that lands + // on it is reported regardless of which patch produced it. + let config_watch = super::config_edit_notice::ConfigEditWatch::begin(); let mut results = Vec::new(); for patch in patches { @@ -84,7 +87,9 @@ impl Tool for PatchTool { } } - Ok(ToolOutput::new(results.join("\n\n"))) + let mut body = results.join("\n\n"); + config_watch.finish(&mut body); + Ok(ToolOutput::new(body)) } } diff --git a/crates/jcode-app-core/src/tool/todo.rs b/crates/jcode-app-core/src/tool/todo.rs index b998a82973..a140a7a777 100644 --- a/crates/jcode-app-core/src/tool/todo.rs +++ b/crates/jcode-app-core/src/tool/todo.rs @@ -822,7 +822,14 @@ mod tests { .get("description") .and_then(Value::as_str) .expect("feedback loop should describe requirement-to-check coverage"); - assert!(feedback_description.contains("requirement-to-check")); + // Case-insensitive: the schema text starts the sentence with + // "Requirement-to-check", so an exact-case check is a false negative. + assert!( + feedback_description + .to_ascii_lowercase() + .contains("requirement-to-check"), + "feedback loop description omitted requirement-to-check: {feedback_description}" + ); assert!(feedback_description.contains("explicit observation or check")); for required_concept in [ "reports back on each requirement", @@ -1173,6 +1180,150 @@ mod tests { } } + /// Issue #695, the visibly-stale case. The todos panel renders the + /// ungrouped goal unconditionally (not only as a group header), so an + /// ungrouped goal left over from a previous flat todo list is exactly what + /// the reporter saw frozen in the panel. + #[tokio::test] + async fn an_ungrouped_goal_does_not_survive_into_a_grouped_next_task() { + let _guard = crate::storage::lock_test_env(); + let previous_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + let session = "issue-695-ungrouped"; + let tool = TodoTool::new(); + + // Task one: a flat (ungrouped) list, so its goal is the ungrouped one. + tool.execute( + json!({ + "todos": [{ + "content": "flat task one", "status": "in_progress", + "priority": "high", "id": "t1", "confidence": 70, + }], + "plan": {"user_intention": "do task one", "understands_user_intent": 97}, + "goals": [{"closed_feedback_loop": 97, "feedback_loop": "ran the checks"}], + }), + test_ctx(session), + ) + .await + .expect("first write"); + let stored = load_goals(session).expect("goals"); + assert_eq!(stored.len(), 1); + assert!( + stored[0].group.is_none(), + "task one goal is the ungrouped one" + ); + + // Task two: a grouped list. The ungrouped goal now describes nothing. + tool.execute( + json!({ + "todos": [{ + "content": "task two", "status": "in_progress", "priority": "high", + "id": "t2", "group": "second task", "confidence": 70, + }], + "goals": [{"group": "second task", "closed_feedback_loop": 80, + "feedback_loop": "run the new checks"}], + }), + test_ctx(session), + ) + .await + .expect("second write"); + + let goals = load_goals(session).expect("goals"); + assert!( + !goals.iter().any(|goal| goal.group.is_none()), + "the stale ungrouped goal must not stay in the panel: {goals:?}" + ); + assert_eq!(goals.len(), 1); + assert_eq!(goals[0].group.as_deref(), Some("second task")); + + if let Some(home) = previous_home { + crate::env::set_var("JCODE_HOME", home); + } else { + crate::env::remove_var("JCODE_HOME"); + } + } + + /// Issue #695, end to end through the real tool: finish task one, then + /// start task two. What the todos panel renders (stored todos + goals) must + /// describe task two only, with no leftovers from task one. + #[tokio::test] + async fn moving_to_a_new_task_replaces_what_the_todos_panel_shows() { + let _guard = crate::storage::lock_test_env(); + let previous_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + let session = "issue-695-new-task"; + let tool = TodoTool::new(); + + // Task one, completed. `end_to_end_ownership` clears the completion + // gate so the write is actually stored. + tool.execute( + json!({ + "todos": [{ + "content": "task one", + "status": "completed", + "priority": "high", + "id": "t1", + "group": "first task", + "confidence": 90, + "completion_confidence": 97, + }], + "plan": {"user_intention": "do task one", "understands_user_intent": 97}, + "goals": [{ + "group": "first task", + "closed_feedback_loop": 97, + "end_to_end_ownership": 97, + "feedback_loop": "ran the checks", + }], + }), + test_ctx(session), + ) + .await + .expect("first task write should succeed"); + assert_eq!(load_goals(session).expect("goals").len(), 1); + + // Task two: a fresh todo list in a new group. + tool.execute( + json!({ + "todos": [{ + "content": "task two", + "status": "in_progress", + "priority": "high", + "id": "t2", + "group": "second task", + "confidence": 70, + }], + "goals": [{ + "group": "second task", + "closed_feedback_loop": 80, + "feedback_loop": "run the new checks", + }], + }), + test_ctx(session), + ) + .await + .expect("second task write should succeed"); + + let todos = load_todos(session).expect("todos"); + assert_eq!(todos.len(), 1, "panel must show only the current task"); + assert_eq!(todos[0].group.as_deref(), Some("second task")); + + let goals = load_goals(session).expect("goals"); + assert_eq!( + goals.len(), + 1, + "the finished task's goal must not linger in the panel: {goals:?}" + ); + assert_eq!(goals[0].group.as_deref(), Some("second task")); + + if let Some(home) = previous_home { + crate::env::set_var("JCODE_HOME", home); + } else { + crate::env::remove_var("JCODE_HOME"); + } + } + /// End-to-end through the real tool, which is what the model actually sees. /// A first plan write with honestly-moderate scores must come back clean: /// this is the exact case that previously returned two nudges and spent the diff --git a/crates/jcode-app-core/src/tool/write.rs b/crates/jcode-app-core/src/tool/write.rs index 223b098684..c58395eefb 100644 --- a/crates/jcode-app-core/src/tool/write.rs +++ b/crates/jcode-app-core/src/tool/write.rs @@ -103,24 +103,34 @@ impl Tool for WriteTool { detail, })); - if existed { - Ok(ToolOutput::new(format!( + let mut body = if existed { + format!( "Updated {} ({} lines){}\n{}", params.file_path, line_count, if diff.is_empty() { "" } else { ":" }, diff - )) - .with_title(params.file_path.clone())) + ) } else { // For new files, show all lines as additions let diff = generate_diff_summary("", ¶ms.content); - Ok(ToolOutput::new(format!( + format!( "Created {} ({} lines):\n{}", params.file_path, line_count, diff - )) - .with_title(params.file_path.clone())) - } + ) + }; + + // A write that lands on the active config.toml states exactly which + // settings changed and whether they are live, so neither the agent nor + // the user has to guess whether the edit took effect. + super::config_edit_notice::append_config_edit_notice( + &mut body, + &path, + old_content.as_deref().unwrap_or(""), + ¶ms.content, + ); + + Ok(ToolOutput::new(body).with_title(params.file_path.clone())) } } diff --git a/crates/jcode-base/src/auth/mod.rs b/crates/jcode-base/src/auth/mod.rs index 2ac665dda3..e2a0450718 100644 --- a/crates/jcode-base/src/auth/mod.rs +++ b/crates/jcode-base/src/auth/mod.rs @@ -928,8 +928,11 @@ fn build_auth_status_uncached(mode: AuthProbeMode) -> (AuthStatus, Vec<(&'static probe_copilot_status(&mut status) }); record_auth_probe_step(&mut timings, "antigravity", || { - status.antigravity = - token_state(antigravity::load_tokens().map(|tokens| tokens.is_expired())) + status.antigravity = refreshable_token_state( + "antigravity", + antigravity::load_tokens() + .map(|tokens| (tokens.is_expired(), tokens.refresh_token.clone())), + ) }); record_auth_probe_step(&mut timings, "gemini", || { // An official Gemini Developer API key is a static credential with no @@ -938,7 +941,11 @@ fn build_auth_status_uncached(mode: AuthProbeMode) -> (AuthStatus, Vec<(&'static status.gemini = if gemini::has_api_key() { AuthState::Available } else { - token_state(gemini::load_tokens().map(|tokens| tokens.is_expired())) + refreshable_token_state( + "gemini", + gemini::load_tokens() + .map(|tokens| (tokens.is_expired(), tokens.refresh_token.clone())), + ) } }); record_auth_probe_step(&mut timings, "cursor", || { @@ -972,6 +979,46 @@ fn token_state(result: anyhow::Result) -> AuthState { } } +/// Auth state for an OAuth credential that refreshes automatically. +/// +/// A short-lived access token is *not* a broken login. Antigravity/Gemini +/// access tokens expire roughly hourly and the provider transparently +/// refreshes them on the next request, so reporting `Expired` purely because +/// the cached access token aged out makes a perfectly working provider look +/// dead in `/login`, the header, onboarding, and `jcode auth status`. +/// +/// Only report `Expired` when the refresh token itself is missing or was +/// already permanently rejected (revoked / `invalid_grant`), which is the case +/// where the user genuinely has to log in again. +fn refreshable_token_state(provider_id: &str, result: anyhow::Result<(bool, String)>) -> AuthState { + refreshable_token_state_with(result, |refresh_token| { + crate::auth::refresh_state::refresh_token_is_known_rejected(provider_id, refresh_token) + }) +} + +/// Pure decision core of [`refreshable_token_state`], with the persisted +/// "this refresh token was permanently rejected" lookup injected so it can be +/// unit tested without touching `$HOME`. +fn refreshable_token_state_with( + result: anyhow::Result<(bool, String)>, + is_known_rejected: impl Fn(&str) -> bool, +) -> AuthState { + match result { + Ok((is_expired, refresh_token)) => { + if !is_expired { + return AuthState::Available; + } + let refresh_token = refresh_token.trim(); + if refresh_token.is_empty() || is_known_rejected(refresh_token) { + AuthState::Expired + } else { + AuthState::Available + } + } + Err(_) => AuthState::NotConfigured, + } +} + fn probe_jcode_status(status: &mut AuthStatus) { if crate::subscription_catalog::has_credentials() { status.jcode = AuthState::Available; diff --git a/crates/jcode-base/src/auth/tests.rs b/crates/jcode-base/src/auth/tests.rs index cb061eb099..5698ea2bbe 100644 --- a/crates/jcode-base/src/auth/tests.rs +++ b/crates/jcode-base/src/auth/tests.rs @@ -903,3 +903,87 @@ fn browser_suppressed_inside_test_harness_without_env_overrides() { "browser opens must be suppressed in test binaries even without --no-browser/env vars" ); } + +/// Antigravity/Gemini access tokens live about an hour and are refreshed +/// transparently on the next request. Reporting `Expired` just because the +/// cached access token aged out made a fully working provider render as broken +/// in `/login`, the header, onboarding, and `jcode auth status`, which is what +/// the "antigravity is not working" reports actually were. Only a missing or +/// permanently rejected refresh token means the user must log in again. +#[test] +fn refreshable_token_state_covers_the_full_expiry_state_space() { + let never_rejected = |_: &str| false; + let always_rejected = |_: &str| true; + + // (case, expired access token, refresh token, refresh token rejected) -> state + let cases: [(&str, bool, &str, bool, AuthState); 6] = [ + ( + "hourly access token expired but refresh works", + true, + "1//live-refresh-token", + false, + AuthState::Available, + ), + ( + "fresh access token", + false, + "1//live-refresh-token", + false, + AuthState::Available, + ), + ( + "fresh access token, no refresh token", + false, + "", + false, + AuthState::Available, + ), + ( + "expired with no refresh token needs re-login", + true, + " ", + false, + AuthState::Expired, + ), + ( + "expired with revoked refresh token needs re-login", + true, + "1//revoked", + true, + AuthState::Expired, + ), + ( + "fresh access token is trusted even if an old refresh token was rejected", + false, + "1//revoked", + true, + AuthState::Available, + ), + ]; + + for (case, expired, refresh_token, rejected, expected) in cases { + let observed = if rejected { + super::refreshable_token_state_with( + Ok((expired, refresh_token.to_string())), + always_rejected, + ) + } else { + super::refreshable_token_state_with( + Ok((expired, refresh_token.to_string())), + never_rejected, + ) + }; + assert_eq!(observed, expected, "{case}"); + } +} + +#[test] +fn missing_refreshable_credentials_are_not_configured() { + assert_eq!( + super::refreshable_token_state_with( + Err(anyhow::anyhow!("No Antigravity tokens found.")), + |_| false + ), + AuthState::NotConfigured + ); +} diff --git a/crates/jcode-base/src/config.rs b/crates/jcode-base/src/config.rs index 0379be4419..192678dc02 100644 --- a/crates/jcode-base/src/config.rs +++ b/crates/jcode-base/src/config.rs @@ -716,6 +716,7 @@ impl Default for DictationConfig { } } +pub mod change_report; mod config_file; mod default_file; mod display_summary; diff --git a/crates/jcode-base/src/config/change_report.rs b/crates/jcode-base/src/config/change_report.rs new file mode 100644 index 0000000000..6b91940f0b --- /dev/null +++ b/crates/jcode-base/src/config/change_report.rs @@ -0,0 +1,198 @@ +//! Reporting for config file changes: what changed, and whether it is live. +//! +//! Editing `~/.jcode/config.toml` (by hand, via `/config`, or by an agent +//! writing the file) is only useful if you can tell whether the running +//! process actually picked the change up. Most of the config is re-read +//! through the reloadable [`crate::config::config`] cache and takes effect +//! within its staleness check, but a few sections are consumed once during +//! process/server startup and genuinely need a restart. +//! +//! This module turns "the file changed" into a specific, checkable answer: +//! which keys changed, from what to what, and whether each one is live now. + +use serde_json::Value; +use std::collections::BTreeMap; + +/// Whether a changed config key takes effect in the running process. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Liveness { + /// Re-read through the config cache; the change is already in effect. + Live, + /// Consumed once at server/process startup; needs a restart. + NeedsRestart, +} + +impl Liveness { + pub fn label(self) -> &'static str { + match self { + Self::Live => "live now", + Self::NeedsRestart => "needs restart", + } + } +} + +/// Config sections that are read once during startup rather than through the +/// reloadable config cache. +/// +/// Keep this list small and evidence-based: add a section only when the code +/// that consumes it demonstrably snapshots the value at startup. Everything +/// else defaults to [`Liveness::Live`], which matches how `config()` works. +const RESTART_REQUIRED_SECTIONS: &[&str] = &[ + // `gateway.{enabled,port,bind_addr}` are snapshotted by + // `Server::spawn_gateway` when the listener is created. + "gateway", + // ACP adapter settings are consumed when the adapter process starts. + "acp", + // Launch hotkeys are baked into the desktop/launcher registration once. + "launch_hotkeys", +]; + +/// Liveness of a dotted config key such as `keybindings.scroll_up`. +pub fn liveness_for_key(key: &str) -> Liveness { + let section = key.split('.').next().unwrap_or(key); + if RESTART_REQUIRED_SECTIONS.contains(§ion) { + Liveness::NeedsRestart + } else { + Liveness::Live + } +} + +/// A single changed config key. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConfigChange { + /// Dotted key path, e.g. `keybindings.scroll_up`. + pub key: String, + /// Rendered previous value, or `None` when the key was absent. + pub before: Option, + /// Rendered new value, or `None` when the key was removed. + pub after: Option, + pub liveness: Liveness, +} + +impl ConfigChange { + fn render(&self) -> String { + let before = self.before.as_deref().unwrap_or("(unset)"); + let after = self.after.as_deref().unwrap_or("(unset)"); + format!( + "- `{}`: {} -> {} ({})", + self.key, + before, + after, + self.liveness.label() + ) + } +} + +/// Diff two config files' TOML text into a per-key change list. +/// +/// Both sides are parsed leniently: a side that fails to parse is treated as +/// empty, so a change report is still produced for a file that was previously +/// broken. Returns changes sorted by key. +pub fn diff_toml(before: &str, after: &str) -> Vec { + let before = flatten_toml(before); + let after = flatten_toml(after); + + let mut keys: Vec<&String> = before.keys().chain(after.keys()).collect(); + keys.sort(); + keys.dedup(); + + keys.into_iter() + .filter_map(|key| { + let old = before.get(key); + let new = after.get(key); + if old == new { + return None; + } + Some(ConfigChange { + key: key.clone(), + before: old.cloned(), + after: new.cloned(), + liveness: liveness_for_key(key), + }) + }) + .collect() +} + +/// Human/agent-readable summary of a config edit. +/// +/// Returns `None` when nothing semantically changed (formatting or comment-only +/// edits), so callers can stay quiet instead of reporting a no-op. +pub fn summarize_toml_change(before: &str, after: &str) -> Option { + let changes = diff_toml(before, after); + if changes.is_empty() { + return None; + } + Some(summarize_changes(&changes)) +} + +/// Render an already-computed change list. +pub fn summarize_changes(changes: &[ConfigChange]) -> String { + let mut out = String::from("Config changes:\n"); + for change in changes { + out.push_str(&change.render()); + out.push('\n'); + } + + let restart: Vec<&ConfigChange> = changes + .iter() + .filter(|c| c.liveness == Liveness::NeedsRestart) + .collect(); + if restart.is_empty() { + out.push_str( + "All changes are live in running jcode sessions; no restart needed.\ + \nKeybinding edits apply to the next keystroke.", + ); + } else { + let keys: Vec<&str> = restart.iter().map(|c| c.key.as_str()).collect(); + out.push_str(&format!( + "Restart required for: {}. Other changes are already live.", + keys.join(", ") + )); + } + out +} + +/// Flatten TOML text into dotted-key -> rendered-value pairs. +/// +/// Arrays and inline tables render as compact JSON so an element-level edit +/// still shows up as a change on the owning key. +fn flatten_toml(text: &str) -> BTreeMap { + let mut out = BTreeMap::new(); + let Ok(value) = toml::from_str::(text) else { + return out; + }; + flatten_value(String::new(), &value, &mut out); + out +} + +fn flatten_value(prefix: String, value: &Value, out: &mut BTreeMap) { + match value { + Value::Object(map) => { + for (key, child) in map { + let path = if prefix.is_empty() { + key.clone() + } else { + format!("{prefix}.{key}") + }; + flatten_value(path, child, out); + } + } + other => { + if prefix.is_empty() { + return; + } + out.insert(prefix, render_value(other)); + } + } +} + +fn render_value(value: &Value) -> String { + match value { + Value::String(s) => format!("\"{s}\""), + other => other.to_string(), + } +} + +#[cfg(test)] +#[path = "change_report_tests.rs"] +mod tests; diff --git a/crates/jcode-base/src/config/change_report_tests.rs b/crates/jcode-base/src/config/change_report_tests.rs new file mode 100644 index 0000000000..90e07b5350 --- /dev/null +++ b/crates/jcode-base/src/config/change_report_tests.rs @@ -0,0 +1,129 @@ +use super::*; + +#[test] +fn keybinding_edit_reports_as_live() { + let report = summarize_toml_change( + "[keybindings]\nscroll_up = \"ctrl+k\"\n", + "[keybindings]\nscroll_up = \"ctrl+y\"\n", + ) + .expect("changed key should produce a report"); + + assert!(report.contains("`keybindings.scroll_up`"), "{report}"); + assert!(report.contains("\"ctrl+k\" -> \"ctrl+y\""), "{report}"); + assert!(report.contains("live now"), "{report}"); + assert!(report.contains("no restart needed"), "{report}"); +} + +#[test] +fn gateway_edit_reports_as_needing_restart() { + let report = summarize_toml_change("[gateway]\nport = 7777\n", "[gateway]\nport = 8888\n") + .expect("changed key should produce a report"); + + assert!(report.contains("needs restart"), "{report}"); + assert!( + report.contains("Restart required for: gateway.port"), + "{report}" + ); +} + +#[test] +fn comment_and_formatting_only_edits_report_nothing() { + assert!( + summarize_toml_change( + "[display]\ncentered = true\n", + "# a comment\n[display]\ncentered = true\n", + ) + .is_none(), + "a semantically identical file should not claim a change" + ); +} + +#[test] +fn added_and_removed_keys_are_reported() { + let changes = diff_toml("[display]\ncentered = true\n", "[display]\nemoji = false\n"); + let keys: Vec<&str> = changes.iter().map(|c| c.key.as_str()).collect(); + assert_eq!(keys, vec!["display.centered", "display.emoji"]); + + let removed = &changes[0]; + assert_eq!(removed.before.as_deref(), Some("true")); + assert_eq!(removed.after, None); + + let added = &changes[1]; + assert_eq!(added.before, None); + assert_eq!(added.after.as_deref(), Some("false")); +} + +#[test] +fn unparseable_previous_content_still_reports_the_new_values() { + let report = summarize_toml_change("this is not = = toml", "[display]\ncentered = true\n") + .expect("repairing a broken config should still report the resulting keys"); + assert!(report.contains("`display.centered`"), "{report}"); +} + +#[test] +fn mixed_edits_report_restart_only_for_the_restart_keys() { + let report = summarize_toml_change( + "[gateway]\nport = 7777\n\n[display]\ncentered = true\n", + "[gateway]\nport = 8888\n\n[display]\ncentered = false\n", + ) + .expect("report"); + + assert!( + report.contains("Restart required for: gateway.port"), + "{report}" + ); + assert!( + report.contains("Other changes are already live"), + "{report}" + ); + assert_eq!(liveness_for_key("display.centered"), Liveness::Live); +} + +#[test] +fn nested_tables_and_arrays_flatten_to_dotted_keys() { + let changes = diff_toml( + "[providers.mine]\nmodels = [\"a\"]\n", + "[providers.mine]\nmodels = [\"a\", \"b\"]\n", + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].key, "providers.mine.models"); + assert_eq!(changes[0].liveness, Liveness::Live); +} + +/// The restart-required list is a claim about how the code consumes each +/// section, so it must stay small and deliberate. If a section is added or +/// removed, that is a behavioural change to every config edit report and +/// should be an explicit decision rather than a drive-by edit. +#[test] +fn the_restart_required_list_is_the_reviewed_set() { + assert_eq!( + RESTART_REQUIRED_SECTIONS, + &["gateway", "acp", "launch_hotkeys"], + "changing which sections need a restart changes what users are told; \ + confirm the consuming code really snapshots the value at startup" + ); +} + +/// Sections that are read through `config()` on every use are live by +/// definition. Spot-check the ones users change most often, so a careless +/// addition to the restart list cannot silently start telling people to +/// restart for a setting that already applies. +#[test] +fn commonly_edited_sections_are_live() { + for key in [ + "keybindings.scroll_up", + "display.centered", + "features.thinking", + "provider.openai_reasoning_effort", + "agents.swarm_spawn_mode", + "tools.profile", + "websearch.engine", + "notifications.enabled", + ] { + assert_eq!( + liveness_for_key(key), + Liveness::Live, + "{key} is re-read through the config cache and should report as live" + ); + } +} diff --git a/crates/jcode-base/src/prompt.rs b/crates/jcode-base/src/prompt.rs index 469256fd42..fc4694adbe 100644 --- a/crates/jcode-base/src/prompt.rs +++ b/crates/jcode-base/src/prompt.rs @@ -6,6 +6,31 @@ use std::process::Command; /// Default system prompt for jcode (embedded at compile time) pub const DEFAULT_SYSTEM_PROMPT: &str = include_str!("prompt/system_prompt.md"); +/// Load the base system prompt, allowing the user to fully replace the built-in +/// [`DEFAULT_SYSTEM_PROMPT`]. Precedence: project `./.jcode/system-prompt.md`, +/// then global `~/.jcode/system-prompt.md`, then the built-in default. +/// +/// This is a *replacement* hook. To merely add guidance on top of the default, +/// use `.jcode/prompt-overlay.md` instead. +pub fn load_base_system_prompt(working_dir: Option<&Path>) -> String { + let project_dir = working_dir.unwrap_or(Path::new(".")); + let candidates = [ + Some(project_dir.join(".jcode").join("system-prompt.md")), + crate::storage::jcode_dir() + .ok() + .map(|dir| dir.join("system-prompt.md")), + ]; + for path in candidates.into_iter().flatten() { + if let Ok(content) = std::fs::read_to_string(&path) { + let trimmed = content.trim(); + if !trimmed.is_empty() { + return trimmed.to_string(); + } + } + } + DEFAULT_SYSTEM_PROMPT.to_string() +} + /// Prompt guidance for the optional Mermaid rendering capability. pub const MERMAID_PROMPT: &str = "# Mermaid\n\nRender fenced `mermaid` blocks inline."; @@ -29,8 +54,11 @@ impl PromptCapabilities { } } -fn base_system_prompt_parts(capabilities: PromptCapabilities) -> Vec { - let mut parts = vec![DEFAULT_SYSTEM_PROMPT.to_string()]; +fn base_system_prompt_parts( + capabilities: PromptCapabilities, + working_dir: Option<&Path>, +) -> Vec { + let mut parts = vec![load_base_system_prompt(working_dir)]; if capabilities.mermaid { parts.push(MERMAID_PROMPT.to_string()); } @@ -379,7 +407,7 @@ pub fn build_system_prompt_full_with_capabilities( working_dir: Option<&Path>, capabilities: PromptCapabilities, ) -> (String, ContextInfo) { - let mut parts = base_system_prompt_parts(capabilities); + let mut parts = base_system_prompt_parts(capabilities, working_dir); let mut info = ContextInfo { system_prompt_chars: parts.join("\n\n").len(), ..Default::default() @@ -475,7 +503,7 @@ pub fn build_system_prompt_split_with_capabilities( working_dir: Option<&Path>, capabilities: PromptCapabilities, ) -> (SplitSystemPrompt, ContextInfo) { - let mut static_parts = base_system_prompt_parts(capabilities); + let mut static_parts = base_system_prompt_parts(capabilities, working_dir); let mut dynamic_parts = Vec::new(); let mut info = ContextInfo { system_prompt_chars: static_parts.join("\n\n").len(), diff --git a/crates/jcode-base/src/prompt_tests.rs b/crates/jcode-base/src/prompt_tests.rs index b44c8da3f9..2496d300b5 100644 --- a/crates/jcode-base/src/prompt_tests.rs +++ b/crates/jcode-base/src/prompt_tests.rs @@ -427,3 +427,32 @@ fn test_selfdev_prompt_uses_desktop2_focus_for_desktop2_working_dir() { assert!(prompt.contains("selfdev build target=desktop2")); assert!(!prompt.contains("launched from the TUI/root jcode context")); } + +#[test] +fn project_system_prompt_file_replaces_default_base_prompt() { + use crate::prompt::load_base_system_prompt; + + let dir = std::env::temp_dir().join(format!("jcode-sysprompt-{}", std::process::id())); + let jcode_dir = dir.join(".jcode"); + std::fs::create_dir_all(&jcode_dir).unwrap(); + std::fs::write( + jcode_dir.join("system-prompt.md"), + "You are a custom agent.\n", + ) + .unwrap(); + + assert_eq!( + load_base_system_prompt(Some(&dir)), + "You are a custom agent." + ); + + let (prompt, _info) = build_system_prompt_full(None, &[], false, None, Some(&dir)); + assert!(prompt.contains("You are a custom agent.")); + assert!(!prompt.contains("Jcode is open source")); + + // Empty override falls back to the built-in default. + std::fs::write(jcode_dir.join("system-prompt.md"), " \n").unwrap(); + assert_eq!(load_base_system_prompt(Some(&dir)), DEFAULT_SYSTEM_PROMPT); + + std::fs::remove_dir_all(&dir).ok(); +} diff --git a/crates/jcode-base/src/provider/catalog_routes.rs b/crates/jcode-base/src/provider/catalog_routes.rs index 412c28e932..83e6a3e819 100644 --- a/crates/jcode-base/src/provider/catalog_routes.rs +++ b/crates/jcode-base/src/provider/catalog_routes.rs @@ -1419,6 +1419,95 @@ mod tests { } } + /// Issue #694 through the real path a user hits: a custom + /// `[providers.]` profile in config.toml. The picker must route the + /// model to that profile, and must not offer it a Copilot route. + #[test] + fn custom_config_profile_model_is_routed_and_not_offered_a_copilot_route() { + let _guard = EnvGuard::new(); + let jcode_home = std::env::var_os("JCODE_HOME").expect("JCODE_HOME set"); + std::fs::write( + std::path::PathBuf::from(jcode_home).join("config.toml"), + "[providers.omlx]\ntype = \"openai-compatible\"\nbase_url = \"http://127.0.0.1:18000/v1\"\ndefault_model = \"KAT-Coder-V2.5-Dev-OptiQ-4bit\"\n", + ) + .expect("write config.toml"); + crate::config::invalidate_config_cache(); + + let model = "KAT-Coder-V2.5-Dev-OptiQ-4bit"; + let route = remote_openai_compatible_route_for_model(model) + .expect("custom config profile model must be routed to its profile"); + assert_eq!(route.provider, "omlx"); + assert_eq!(route.api_method, "openai-compatible:omlx"); + assert!( + !remote_model_should_offer_copilot_route(model), + "a custom profile's model must never be offered a Copilot route" + ); + + // The full fallback builder (what the picker renders) agrees. + let routes = remote_model_routes_fallback(Some("omlx"), &[model.to_string()]); + assert!( + routes + .iter() + .all(|route| !route.api_method.contains("copilot")), + "picker routes must not contain a copilot route: {routes:?}" + ); + assert!( + routes + .iter() + .any(|route| route.api_method == "openai-compatible:omlx"), + "picker routes must include the profile route: {routes:?}" + ); + } + + /// Issue #694 across both route sources. The picker is fed either by the + /// server-built catalog (named profile routes) or, before that frame + /// arrives, by the client-side fallback. Neither may attach a Copilot + /// route to a custom profile model, otherwise the label flickers to + /// Copilot and the selected id becomes a copilot-prefixed id. + #[test] + fn custom_config_profile_model_never_gets_a_copilot_route_from_either_source() { + let _guard = EnvGuard::new(); + let jcode_home = std::env::var_os("JCODE_HOME").expect("JCODE_HOME set"); + std::fs::write( + std::path::PathBuf::from(jcode_home).join("config.toml"), + "[providers.omlx]\ntype = \"openai-compatible\"\nbase_url = \"http://127.0.0.1:18000/v1\"\ndefault_model = \"KAT-Coder-V2.5-Dev-OptiQ-4bit\"\n", + ) + .expect("write config.toml"); + crate::config::invalidate_config_cache(); + + let model = "KAT-Coder-V2.5-Dev-OptiQ-4bit"; + + // Source 1: the named-profile routes the server contributes. + let named = named_provider_profile_routes( + "omlx", + crate::config::config() + .providers + .get("omlx") + .expect("omlx profile"), + ); + assert!( + named + .iter() + .any(|route| route.model == model && route.api_method == "openai-compatible:omlx"), + "server catalog must offer the profile route: {named:?}" + ); + + // Source 2: the client-side fallback, including the lightweight + // variant used while route details are still refreshing. + for routes in [ + remote_model_routes_fallback(Some("omlx"), &[model.to_string()]), + remote_model_routes_lightweight_fallback(Some("omlx"), &[model.to_string()], model), + ] { + assert!(!routes.is_empty(), "fallback must offer the model"); + assert!( + routes.iter().all(|route| { + !route.api_method.contains("copilot") && route.provider != "Copilot" + }), + "no source may attach a Copilot route: {routes:?}" + ); + } + } + #[test] fn remote_compatible_route_uses_live_cache_and_does_not_mark_fallback() { let guard = EnvGuard::new(); diff --git a/crates/jcode-provider-anthropic/src/duplicate_tool_result_tests.rs b/crates/jcode-provider-anthropic/src/duplicate_tool_result_tests.rs new file mode 100644 index 0000000000..a500d6a925 --- /dev/null +++ b/crates/jcode-provider-anthropic/src/duplicate_tool_result_tests.rs @@ -0,0 +1,197 @@ +//! Regression coverage for the 400 "unexpected `tool_use_id` found in +//! `tool_result` blocks" rejection. +//! +//! Production shape (session_clover_1785560899476): a long-running `bash` tool +//! was still executing when a scheduled-task wakeup drove a new turn. The +//! missing tool-output repair saw an assistant `tool_use` with no result yet +//! and inserted a synthetic placeholder result; the real tool output landed +//! ~28s later and was appended as a second `tool_result` for the same id. +//! After same-role merging, the duplicate sits in a user message whose +//! preceding assistant turn no longer contains that `tool_use`, so Anthropic +//! rejects every subsequent request and the session is permanently wedged. + +use super::*; +use jcode_message_types::{ContentBlock, Message, Role}; + +fn text_msg(role: Role, text: &str) -> Message { + Message { + role, + content: vec![ContentBlock::Text { + text: text.to_string(), + cache_control: None, + }], + timestamp: None, + tool_duration_ms: None, + } +} + +fn tool_use(id: &str) -> Message { + Message { + role: Role::Assistant, + content: vec![ContentBlock::ToolUse { + id: id.to_string(), + name: "bash".to_string(), + input: serde_json::json!({"command": "ls"}), + thought_signature: None, + }], + timestamp: None, + tool_duration_ms: None, + } +} + +fn tool_result(id: &str, content: &str, is_error: Option) -> Message { + Message { + role: Role::User, + content: vec![ContentBlock::ToolResult { + tool_use_id: id.to_string(), + content: content.to_string(), + is_error, + }], + timestamp: None, + tool_duration_ms: None, + } +} + +/// Every tool_use_id may appear at most once across all tool_result blocks. +fn assert_unique_tool_results(messages: &[ApiMessage]) { + let mut seen = std::collections::HashSet::new(); + for msg in messages { + for block in &msg.content { + if let ApiContentBlock::ToolResult { tool_use_id, .. } = block { + assert!( + seen.insert(tool_use_id.clone()), + "duplicate tool_result for {tool_use_id}" + ); + } + } + } +} + +#[test] +fn placeholder_then_real_output_keeps_only_the_real_output() { + let messages = vec![ + text_msg(Role::User, "Q"), + tool_use("toolu_1"), + tool_result("toolu_1", TOOL_OUTPUT_MISSING_TEXT, Some(true)), + text_msg(Role::User, "[Scheduled task] wakeup"), + tool_result("toolu_1", "real output", None), + ]; + + let formatted = format_messages(&messages, false); + assert_unique_tool_results(&formatted); + + let kept: Vec<&str> = formatted + .iter() + .flat_map(|m| &m.content) + .filter_map(|b| match b { + ApiContentBlock::ToolResult { + content: ToolResultContent::Text(text), + .. + } => Some(text.as_str()), + _ => None, + }) + .collect(); + assert_eq!(kept, vec!["real output"], "real output must win"); +} + +#[test] +fn real_output_then_placeholder_keeps_the_real_output() { + let messages = vec![ + text_msg(Role::User, "Q"), + tool_use("toolu_1"), + tool_result("toolu_1", "real output", None), + tool_result("toolu_1", TOOL_OUTPUT_MISSING_TEXT, Some(true)), + ]; + + let formatted = format_messages(&messages, false); + assert_unique_tool_results(&formatted); + assert!( + formatted.iter().flat_map(|m| &m.content).any(|b| matches!( + b, + ApiContentBlock::ToolResult { content: ToolResultContent::Text(t), .. } if t == "real output" + )) + ); +} + +#[test] +fn duplicate_real_outputs_keep_the_first() { + let messages = vec![ + text_msg(Role::User, "Q"), + tool_use("toolu_1"), + tool_result("toolu_1", "first", None), + tool_result("toolu_1", "second", None), + ]; + + let formatted = format_messages(&messages, false); + assert_unique_tool_results(&formatted); + assert!(formatted.iter().flat_map(|m| &m.content).any(|b| matches!( + b, + ApiContentBlock::ToolResult { content: ToolResultContent::Text(t), .. } if t == "first" + ))); +} + +#[test] +fn distinct_tool_ids_are_untouched() { + let messages = vec![ + text_msg(Role::User, "Q"), + tool_use("toolu_1"), + tool_result("toolu_1", "a", None), + tool_use("toolu_2"), + tool_result("toolu_2", "b", None), + ]; + + let formatted = format_messages(&messages, false); + assert_unique_tool_results(&formatted); + let count = formatted + .iter() + .flat_map(|m| &m.content) + .filter(|b| matches!(b, ApiContentBlock::ToolResult { .. })) + .count(); + assert_eq!(count, 2); +} + +#[test] +fn message_left_empty_by_dedupe_is_dropped_and_roles_stay_valid() { + // The duplicate is the sole block of its message; dropping it must not + // leave an empty message in the request. + let messages = vec![ + text_msg(Role::User, "Q"), + tool_use("toolu_1"), + tool_result("toolu_1", "real", None), + tool_result("toolu_1", TOOL_OUTPUT_MISSING_TEXT, Some(true)), + text_msg(Role::Assistant, "done"), + text_msg(Role::User, "next"), + ]; + + let formatted = format_messages(&messages, false); + assert_unique_tool_results(&formatted); + assert!(formatted.iter().all(|m| !m.content.is_empty())); + let roles: Vec<&str> = formatted.iter().map(|m| m.role.as_str()).collect(); + assert_eq!( + roles, + vec!["user", "assistant", "user", "assistant", "user"] + ); +} + +#[test] +fn synthetic_interrupt_placeholder_text_is_also_treated_as_a_placeholder() { + let messages = vec![ + text_msg(Role::User, "Q"), + tool_use("toolu_1"), + tool_result( + "toolu_1", + "[Session interrupted before tool execution completed]", + Some(true), + ), + tool_result("toolu_1", "real output", None), + ]; + + let formatted = format_messages(&messages, false); + assert_unique_tool_results(&formatted); + assert!( + formatted.iter().flat_map(|m| &m.content).any(|b| matches!( + b, + ApiContentBlock::ToolResult { content: ToolResultContent::Text(t), .. } if t == "real output" + )) + ); +} diff --git a/crates/jcode-provider-anthropic/src/lib.rs b/crates/jcode-provider-anthropic/src/lib.rs index 2fbd65d094..e17f203f1c 100644 --- a/crates/jcode-provider-anthropic/src/lib.rs +++ b/crates/jcode-provider-anthropic/src/lib.rs @@ -1,4 +1,6 @@ -use jcode_message_types::{ContentBlock, Message, Role, ToolDefinition, sanitize_tool_id}; +use jcode_message_types::{ + ContentBlock, Message, Role, TOOL_OUTPUT_MISSING_TEXT, ToolDefinition, sanitize_tool_id, +}; use jcode_provider_core::anthropic_map_tool_name_for_oauth as map_tool_name_for_oauth; use serde::Serialize; use serde_json::{Value, json}; @@ -16,6 +18,18 @@ pub(crate) const CONTINUATION_USER_TURN: &str = "Continue."; pub fn format_messages(messages: &[Message], is_oauth: bool) -> Vec { use std::collections::HashSet; + // Pre-pass: drop duplicate tool_results for the same tool_use_id. + // + // Anthropic rejects the whole request (400 "unexpected `tool_use_id` found + // in `tool_result` blocks") when a tool_use_id appears twice, because after + // same-role merging only the first result lines up with the tool_use in the + // preceding assistant message. Duplicates are produced by the missing + // tool-output repair racing a still-running tool: the repair inserts a + // synthetic placeholder result, then the real result lands moments later, + // and the conversation is permanently unsendable. Prefer the real output + // over the synthetic placeholder, and otherwise keep the first occurrence. + let messages = &dedupe_tool_results(messages); + // First pass: collect all tool_use IDs and tool_result IDs let mut tool_use_ids: HashSet = HashSet::new(); let mut tool_result_ids: HashSet = HashSet::new(); @@ -190,6 +204,91 @@ pub fn format_messages(messages: &[Message], is_oauth: bool) -> Vec merged } +/// Returns true when a tool_result body is one of the synthetic placeholders +/// injected by the missing tool-output repair paths rather than real output. +fn is_placeholder_tool_result(content: &str, is_error: Option) -> bool { + is_error.unwrap_or(false) + && (content.contains(TOOL_OUTPUT_MISSING_TEXT) + || content.contains("[Session interrupted before tool execution completed]")) +} + +/// Remove duplicate `tool_result` blocks so each `tool_use_id` is answered +/// exactly once, preferring real output over a synthetic placeholder. +/// Messages left with no content at all are dropped by the caller's +/// `!content.is_empty()` guard. +fn dedupe_tool_results(messages: &[Message]) -> Vec { + use std::collections::HashMap; + + // Winner position per tool_use_id: the first real result if one exists, + // otherwise the first occurrence at all. + let mut winner: HashMap<&str, (usize, usize)> = HashMap::new(); + let mut winner_is_real: HashMap<&str, bool> = HashMap::new(); + let mut duplicate_seen = false; + + for (mi, msg) in messages.iter().enumerate() { + for (bi, block) in msg.content.iter().enumerate() { + let ContentBlock::ToolResult { + tool_use_id, + content, + is_error, + } = block + else { + continue; + }; + let real = !is_placeholder_tool_result(content, *is_error); + match winner_is_real.get(tool_use_id.as_str()) { + None => { + winner.insert(tool_use_id, (mi, bi)); + winner_is_real.insert(tool_use_id, real); + } + Some(false) if real => { + // Upgrade a placeholder winner to the real output. + winner.insert(tool_use_id, (mi, bi)); + winner_is_real.insert(tool_use_id, true); + duplicate_seen = true; + } + Some(_) => duplicate_seen = true, + } + } + } + + if !duplicate_seen { + return messages.to_vec(); + } + + let dropped = std::cell::Cell::new(0usize); + let out: Vec = messages + .iter() + .enumerate() + .map(|(mi, msg)| { + let mut msg = msg.clone(); + let mut bi = 0usize; + msg.content.retain(|block| { + let index = bi; + bi += 1; + let ContentBlock::ToolResult { tool_use_id, .. } = block else { + return true; + }; + let keep = winner.get(tool_use_id.as_str()) == Some(&(mi, index)); + if !keep { + dropped.set(dropped.get() + 1); + } + keep + }); + msg + }) + .collect(); + + if dropped.get() > 0 { + jcode_logging::warn(&format!( + "[anthropic] Dropped {} duplicate tool_result block(s); each tool_use_id may be \ + answered only once", + dropped.get() + )); + } + out +} + /// Convert our ContentBlock to Anthropic API format pub fn format_content_blocks(blocks: &[ContentBlock], is_oauth: bool) -> Vec { let mut result: Vec = Vec::new(); @@ -1140,3 +1239,11 @@ mod cache_prefix_invariant_tests { #[cfg(test)] #[path = "trailing_assistant_repair_tests.rs"] mod trailing_assistant_repair_tests; + +#[cfg(test)] +#[path = "duplicate_tool_result_tests.rs"] +mod duplicate_tool_result_tests; + +#[cfg(test)] +#[path = "wedge_fixture_check.rs"] +mod wedge_fixture_check; diff --git a/crates/jcode-provider-anthropic/src/wedge_fixture_check.rs b/crates/jcode-provider-anthropic/src/wedge_fixture_check.rs new file mode 100644 index 0000000000..5f2219563e --- /dev/null +++ b/crates/jcode-provider-anthropic/src/wedge_fixture_check.rs @@ -0,0 +1,41 @@ +use super::*; +use jcode_message_types::Message; + +#[test] +fn real_wedged_session_becomes_sendable() { + let Ok(path) = std::env::var("JCODE_WEDGE_FIXTURE") else { + return; + }; + let raw = std::fs::read_to_string(path).expect("fixture"); + let messages: Vec = serde_json::from_str(&raw).expect("parse"); + let formatted = format_messages(&messages, false); + + let mut seen = std::collections::HashSet::new(); + for m in &formatted { + for b in &m.content { + if let ApiContentBlock::ToolResult { tool_use_id, .. } = b { + assert!(seen.insert(tool_use_id.clone()), "dup {tool_use_id}"); + } + } + } + // Every tool_result must be answered by a tool_use in the previous message. + for (i, m) in formatted.iter().enumerate() { + for b in &m.content { + let ApiContentBlock::ToolResult { tool_use_id, .. } = b else { + continue; + }; + let prev = formatted.get(i.wrapping_sub(1)).expect("prev message"); + assert!( + prev.content.iter().any(|pb| matches!( + pb, + ApiContentBlock::ToolUse { id, .. } if id == tool_use_id + )), + "messages.{i}: tool_result {tool_use_id} has no tool_use in the previous message" + ); + } + } + println!( + "formatted {} messages, all tool_results valid", + formatted.len() + ); +} diff --git a/crates/jcode-provider-core/src/openai_schema.rs b/crates/jcode-provider-core/src/openai_schema.rs index dde20daeaf..dd2837b3a3 100644 --- a/crates/jcode-provider-core/src/openai_schema.rs +++ b/crates/jcode-provider-core/src/openai_schema.rs @@ -231,6 +231,29 @@ fn openai_compatible_keyword(key: &str, value: &Value) -> Value { } } +/// Whether a schema declares enough type information for OpenAI strict mode. +/// An "empty" schema like `{"description": "..."}` accepts any instance in JSON +/// Schema, but OpenAI's strict subset requires a concrete type keyword. +fn schema_has_type_info(schema: &Value) -> bool { + match schema { + Value::Bool(_) => false, + Value::Object(map) => [ + "type", + "enum", + "const", + "$ref", + "anyOf", + "oneOf", + "allOf", + "properties", + "items", + ] + .iter() + .any(|key| map.contains_key(*key)), + _ => true, + } +} + pub fn schema_supports_strict(schema: &Value) -> bool { fn check_map(map: &serde_json::Map) -> bool { let is_object_typed = match map.get("type") { @@ -256,6 +279,16 @@ pub fn schema_supports_strict(schema: &Value) -> bool { } } + // A property carrying no type information at all (e.g. only a + // `description`) is valid JSON Schema, but strict normalization turns it + // into an untyped `anyOf` branch that makes OpenAI reject the entire tool + // catalog. Fall back to non-strict instead. See issue #713. + if let Some(Value::Object(props)) = map.get("properties") { + if props.values().any(|prop| !schema_has_type_info(prop)) { + return false; + } + } + map.values().all(schema_supports_strict) } @@ -594,6 +627,33 @@ mod tests { }))); } + /// Regression test for issue #713: an MCP tool property with only a + /// `description` (no type keyword) made OpenAI reject the whole tool catalog. + #[test] + fn schema_supports_strict_rejects_untyped_properties() { + assert!(!schema_supports_strict(&json!({ + "type": "object", + "properties": { + "key": { "type": "string" }, + "value": { "description": "JSON type depends on the key." } + }, + "additionalProperties": false + }))); + assert!(!schema_supports_strict(&json!({ + "type": "object", + "properties": { "value": true }, + "additionalProperties": false + }))); + assert!(schema_supports_strict(&json!({ + "type": "object", + "properties": { + "key": { "type": "string" }, + "value": { "enum": ["a", "b"] } + }, + "additionalProperties": false + }))); + } + #[test] fn openai_compatible_schema_flattens_allof_object_branches() { let schema = json!({ diff --git a/crates/jcode-tui-messages/src/message.rs b/crates/jcode-tui-messages/src/message.rs index 0ed89067a4..6aceedc76b 100644 --- a/crates/jcode-tui-messages/src/message.rs +++ b/crates/jcode-tui-messages/src/message.rs @@ -190,6 +190,21 @@ impl DisplayMessage { } } + /// Create a display-only spacer used by the terminal-style clear (Ctrl+L): + /// renders as `rows` blank lines so the prior transcript sits just above + /// the viewport, exactly like a terminal's clear-with-scrollback. Not part + /// of provider/model context. + pub fn spacer(rows: usize) -> Self { + Self { + role: "spacer".to_string(), + content: rows.to_string(), + tool_calls: Vec::new(), + duration_secs: None, + title: None, + tool_data: None, + } + } + /// Create a display-only collapsing reasoning trace ("current" mode). The /// content is sentinel-wrapped dim/italic markup; this message height-collapses /// toward a one-line summary and is excluded from provider/model context. diff --git a/crates/jcode-tui/src/tui/app/conversation_state.rs b/crates/jcode-tui/src/tui/app/conversation_state.rs index 62d171762f..051ac72e15 100644 --- a/crates/jcode-tui/src/tui/app/conversation_state.rs +++ b/crates/jcode-tui/src/tui/app/conversation_state.rs @@ -708,9 +708,19 @@ impl App { let mut missing_for_message = Vec::new(); for id in tool_uses { self.tool_call_ids.insert(id.clone()); - if !self.tool_result_ids.contains(&id) { - missing_for_message.push(id); + if self.tool_result_ids.contains(&id) { + continue; } + // Still-executing tools will deliver their own result; a + // placeholder here becomes a duplicate tool_result that + // Anthropic rejects. See `jcode_app_core::tool::inflight`. + if crate::tool::inflight::is_tool_in_flight(&id) { + crate::logging::info(&format!( + "Skipping missing tool-output repair for {id}: tool is still executing" + )); + continue; + } + missing_for_message.push(id); } if !missing_for_message.is_empty() { missing_repairs.push((index, missing_for_message)); diff --git a/crates/jcode-tui/src/tui/app/hotkey_feedback.rs b/crates/jcode-tui/src/tui/app/hotkey_feedback.rs index 6734e321ab..1ac59310a7 100644 --- a/crates/jcode-tui/src/tui/app/hotkey_feedback.rs +++ b/crates/jcode-tui/src/tui/app/hotkey_feedback.rs @@ -291,8 +291,13 @@ pub(super) fn build_registry(inputs: &RegistryInputs<'_>) -> Vec { ); out.push(KnownHotkey::new( ctrl('l'), - "clear_view", - "clear the view, keeping context (/cls)", + "clear_screen", + "clear the screen; history stays in scrollback", + )); + out.push(KnownHotkey::new( + key(KeyCode::Char('l'), KeyModifiers::SUPER), + "clear_screen", + "clear the screen; history stays in scrollback", )); // Built-in readline-style editing chords. diff --git a/crates/jcode-tui/src/tui/app/input.rs b/crates/jcode-tui/src/tui/app/input.rs index e099c66edd..fa44559ceb 100644 --- a/crates/jcode-tui/src/tui/app/input.rs +++ b/crates/jcode-tui/src/tui/app/input.rs @@ -1130,10 +1130,19 @@ pub(super) fn handle_multiline_input_navigation( code: KeyCode, modifiers: KeyModifiers, ) -> bool { - if !modifiers.is_empty() - || !matches!(code, KeyCode::Up | KeyCode::Down) - || !app.input.contains('\n') - { + if !modifiers.is_empty() || !matches!(code, KeyCode::Up | KeyCode::Down) { + return false; + } + + // Prefer true visual-row movement: with soft wrapping a single logical + // line can occupy several rows, and Up/Down should follow what the user + // sees. Falls through to history recall at the first/last visual row. + if let Some(target) = visual_line_move_in_composer(app, code) { + app.cursor_pos = target; + return true; + } + + if !app.input.contains('\n') { return false; } @@ -1176,6 +1185,36 @@ pub(super) fn handle_multiline_input_navigation( true } +/// Visual (wrapped-row) cursor movement using the composer's current render +/// width. Returns `None` when the width is unknown or the cursor is already on +/// the first/last visual row. +fn visual_line_move_in_composer(app: &App, code: KeyCode) -> Option { + use crate::tui::ui::input_ui; + + let width = composer_area_width()?; + let state: &dyn crate::tui::TuiState = app; + let next_prompt = input_ui::next_input_prompt_number(state); + let line_width = input_ui::composer_line_width(state, width, next_prompt)?; + let delta = match code { + KeyCode::Up => -1, + KeyCode::Down => 1, + _ => return None, + }; + input_ui::visual_line_move(&app.input, app.cursor_pos, line_width, delta) +} + +fn composer_area_width() -> Option { + if let Some(area) = crate::tui::ui::last_layout_snapshot().and_then(|l| l.input_area) + && area.width > 0 + { + return Some(area.width); + } + crossterm::terminal::size() + .ok() + .map(|(w, _)| w) + .filter(|w| *w > 0) +} + /// True when `modifiers` is exactly one of Ctrl, Alt(Option) or Cmd(Super), /// the set of single modifiers we treat as "recall queued prompts / browse /// history" when combined with Up/Down. Shift or any combination is excluded so @@ -1892,6 +1931,13 @@ pub(super) fn handle_super_key(app: &mut App, code: KeyCode) -> bool { paste_from_clipboard(app); true } + // Cmd+L mirrors Ctrl+L: terminal-style clear (blank spacer pushes + // the transcript up into scrollback; terminals that forward Command + // report it as Super+L). + KeyCode::Char('l') => { + app.clear_view_terminal_style(); + true + } _ => false, } } @@ -2482,11 +2528,14 @@ pub(super) fn handle_global_control_shortcuts( app.copy_chat_viewport_context_to_clipboard(); true } - // Ctrl+L: terminal-style view clear (context kept). Only reachable - // when no side pane claimed 'l' for focus (handle_diagram_ctrl_key - // runs first and wins while a diagram or diff pane is available). + // Ctrl+L: terminal-style clear - a viewport-height blank spacer + // pushes the transcript up into scrollback, leaving a clean prompt. + // Nothing is deleted; scroll up to see history, /cls actually wipes + // the view. Only reachable when no side pane claimed 'l' for focus + // (handle_diagram_ctrl_key runs first and wins while a diagram or + // diff pane is available). KeyCode::Char('l') => { - app.clear_view_keep_context(); + app.clear_view_terminal_style(); true } _ => handle_control_key(app, code), @@ -2701,6 +2750,11 @@ impl App { } pub(super) fn handle_key_press_event(&mut self, event: KeyEvent) -> Result<()> { + // Pick up config.toml keybinding edits before this key is matched. + // The idle tick refreshes too, but it can run as slowly as the 5s + // deep-idle cadence, which would leave the first keystroke after an + // edit matched against the old chords. + self.refresh_keybindings_if_config_reloaded(); self.handle_key_core( event.code, event.modifiers, diff --git a/crates/jcode-tui/src/tui/app/input_help.rs b/crates/jcode-tui/src/tui/app/input_help.rs index 2e932b82da..ad0ce99993 100644 --- a/crates/jcode-tui/src/tui/app/input_help.rs +++ b/crates/jcode-tui/src/tui/app/input_help.rs @@ -23,7 +23,7 @@ impl App { "/clear\nClear current conversation, queue, and display; starts a fresh session." } "cls" | "clear-view" => { - "/cls\nClear the rendered view only. The model keeps its full context; nothing is sent or forgotten. Also on Ctrl+L." + "/cls\nClear the rendered view only. The model keeps its full context; nothing is sent or forgotten. (Ctrl+L clears the screen but keeps history in scrollback.)" } "model" => { "/model\nOpen model picker.\n\n/model \nSwitch model.\n\n/model @\nPin OpenRouter routing (@auto clears pin)." diff --git a/crates/jcode-tui/src/tui/app/navigation.rs b/crates/jcode-tui/src/tui/app/navigation.rs index b8d337308a..2ecb65b3b9 100644 --- a/crates/jcode-tui/src/tui/app/navigation.rs +++ b/crates/jcode-tui/src/tui/app/navigation.rs @@ -1598,6 +1598,14 @@ impl App { /// "phantom" scroll once the viewport is already pinned to the top. pub(super) fn scroll_up(&mut self, amount: usize) -> bool { // Scrolling up cancels any pending overscroll rebound line immediately + // Leaving the collapsed terminal-clear screen: drop the trailing Ctrl+L + // spacer so scrolling up reveals the transcript immediately instead of + // first having to travel back through a viewport of blank rows. + if self.terminal_clear_collapsed() { + self.display_messages.pop(); + self.bump_display_messages_version(); + self.request_full_repaint(); + } // and ends the current downward gesture, so a subsequent scroll down // starts a fresh gesture evaluated from wherever the view is then. self.chat_overscroll_last = None; diff --git a/crates/jcode-tui/src/tui/app/remote.rs b/crates/jcode-tui/src/tui/app/remote.rs index 2a754d0a3c..ddf8c30595 100644 --- a/crates/jcode-tui/src/tui/app/remote.rs +++ b/crates/jcode-tui/src/tui/app/remote.rs @@ -1834,7 +1834,7 @@ fn handle_disconnected_key_internal( return Ok(()); } KeyCode::Char('l') if !app.diff_pane_visible() => { - app.clear_view_keep_context(); + app.clear_view_terminal_style(); return Ok(()); } _ => { @@ -1882,6 +1882,12 @@ fn handle_disconnected_key_internal( app.paste_from_clipboard(); return Ok(()); } + // Cmd+L mirrors Ctrl+L: terminal-style clear (blank spacer + // pushes the transcript up into scrollback). + KeyCode::Char('l') => { + app.clear_view_terminal_style(); + return Ok(()); + } _ => {} } } diff --git a/crates/jcode-tui/src/tui/app/remote/key_handling.rs b/crates/jcode-tui/src/tui/app/remote/key_handling.rs index 36910c0493..36c1be956f 100644 --- a/crates/jcode-tui/src/tui/app/remote/key_handling.rs +++ b/crates/jcode-tui/src/tui/app/remote/key_handling.rs @@ -532,6 +532,12 @@ async fn handle_remote_key_internal( app.paste_from_clipboard(); return Ok(()); } + // Cmd+L mirrors Ctrl+L: terminal-style clear (blank spacer + // pushes the transcript up into scrollback). + KeyCode::Char('l') => { + app.clear_view_terminal_style(); + return Ok(()); + } _ => {} } } @@ -632,9 +638,12 @@ async fn handle_remote_key_internal( return Ok(()); } KeyCode::Char('l') => { - // Terminal-style view clear (context kept); the diagram/diff - // focus handler above wins while a side pane is available. - app.clear_view_keep_context(); + // Terminal-style clear: a viewport-height blank spacer pushes + // the transcript up into scrollback, leaving a clean prompt. + // Nothing is deleted; /cls does the actual view wipe. The + // diagram/diff focus handler above wins while a pane is + // available. + app.clear_view_terminal_style(); return Ok(()); } KeyCode::Char('u') => { diff --git a/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs b/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs index 62e7b80c40..c6dfbc88b1 100644 --- a/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs +++ b/crates/jcode-tui/src/tui/app/state_ui_input_helpers.rs @@ -117,7 +117,7 @@ const REGISTERED_COMMANDS: &[RegisteredCommand] = &[ RegisteredCommand::hidden("/reasoning", "Alias for /thinking-display"), RegisteredCommand::public("/cancel", "Cancel the current prompt or operation"), RegisteredCommand::public("/clear", "Clear conversation history"), - RegisteredCommand::public("/cls", "Clear the view only, keeping context (Ctrl+L)"), + RegisteredCommand::public("/cls", "Clear the view only, keeping context"), RegisteredCommand::hidden("/clear-view", "Alias for /cls"), RegisteredCommand::public("/rewind", "Rewind conversation to previous message"), RegisteredCommand::public("/poke", "Poke model to resume with incomplete todos"), diff --git a/crates/jcode-tui/src/tui/app/state_ui_messages.rs b/crates/jcode-tui/src/tui/app/state_ui_messages.rs index b0111e4995..66ac6abf81 100644 --- a/crates/jcode-tui/src/tui/app/state_ui_messages.rs +++ b/crates/jcode-tui/src/tui/app/state_ui_messages.rs @@ -74,6 +74,18 @@ fn stored_message_visible_text(message: &crate::session::StoredMessage) -> Strin impl App { pub fn push_display_message(&mut self, mut message: DisplayMessage) { compact_display_message_tool_data(&mut message); + // A trailing Ctrl+L spacer only exists to keep the screen clear while + // idle. The moment real content arrives, drop it so the transcript + // stays contiguous instead of keeping a screenful of blank rows + // embedded in the scrollback. + if message.role != "spacer" + && self + .display_messages + .last() + .is_some_and(|last| last.role == "spacer") + { + self.display_messages.pop(); + } if self.try_coalesce_repeated_display_message(&message) { return; } @@ -374,10 +386,49 @@ impl App { } } - /// View-only clear (Ctrl+L, `/cls`): wipe the rendered transcript while + /// Terminal-style clear (Ctrl+L): append a viewport-height blank spacer + /// and snap to the bottom, so the screen shows a clean prompt while the + /// whole transcript stays one scroll-up away, exactly like a terminal's + /// clear-with-scrollback. Nothing is deleted; context, queue, and draft + /// are untouched. Contrast with `/cls` (`clear_view_keep_context`), which + /// actually wipes the rendered transcript. + pub(super) fn clear_view_terminal_style(&mut self) { + let rows = super::super::ui::last_chat_viewport_height(); + // Pressing Ctrl+L repeatedly (or on an already-empty screen) should + // not stack blank pages: with a trailing spacer and nothing after it, + // the viewport is already visually clear, so just re-snap. + let already_clear = self + .display_messages + .last() + .is_some_and(|message| message.role == "spacer"); + if rows > 0 && !already_clear && !self.display_messages.is_empty() { + self.push_display_message(DisplayMessage::spacer(rows)); + } + self.follow_chat_bottom(); + } + + /// Whether the view is in the terminal-style cleared state: the transcript + /// ends in a Ctrl+L spacer, the viewport is pinned to the bottom, and no + /// new output has arrived since. In that state every visible transcript row + /// is blank, so the renderer collapses the messages area and the numbered + /// prompt sits at the top of the screen like a terminal after `clear`. + /// Any new message, stream, or scroll-up immediately ends it. + pub(crate) fn terminal_clear_collapsed(&self) -> bool { + !self.auto_scroll_paused + && self.pending_history_anchor.is_none() + && !self.is_processing + && self.streaming.streaming_text.is_empty() + && self + .display_messages + .last() + .is_some_and(|message| message.role == "spacer") + } + + /// View-only clear (`/cls`): wipe the rendered transcript while /// keeping provider context, queued messages, and the input draft intact, /// so the model still remembers everything. Contrast with `/clear` - /// (`reset_current_session`), which discards context too. + /// (`reset_current_session`), which discards context too, and Ctrl+L, + /// which merely snaps to the bottom of the chat. pub(super) fn clear_view_keep_context(&mut self) { self.clear_display_messages(); // The rendered transcript is gone, so every entry in the diff --git a/crates/jcode-tui/src/tui/app/tests.rs b/crates/jcode-tui/src/tui/app/tests.rs index f73ceb2b8c..a6e503e83a 100644 --- a/crates/jcode-tui/src/tui/app/tests.rs +++ b/crates/jcode-tui/src/tui/app/tests.rs @@ -45,6 +45,7 @@ include!("tests/hotkey_feedback_e2e.rs"); include!("tests/todo_card.rs"); include!("tests/issue_496_input_routing.rs"); include!("tests/issue_544_paste_enter.rs"); +include!("tests/keybinding_hot_reload.rs"); include!("tests/terminal_setup_command.rs"); include!("tests/issue_497_copy_ctrl_c.rs"); include!("tests/issue_699_ctrl_d_delete.rs"); diff --git a/crates/jcode-tui/src/tui/app/tests/issue_699_ctrl_d_delete.rs b/crates/jcode-tui/src/tui/app/tests/issue_699_ctrl_d_delete.rs index 8c51432165..bffa2e769a 100644 --- a/crates/jcode-tui/src/tui/app/tests/issue_699_ctrl_d_delete.rs +++ b/crates/jcode-tui/src/tui/app/tests/issue_699_ctrl_d_delete.rs @@ -59,3 +59,59 @@ fn test_ctrl_d_deletes_multibyte_character() { assert_eq!(app.input, "hllo"); } + +// The normal `jcode` TUI runs as a remote client (`is_remote = true`), so the +// remote key path is the one real users hit. Cover it explicitly rather than +// trusting that it mirrors the local handler. + +#[test] +fn test_remote_ctrl_d_deletes_character_under_cursor() { + with_temp_jcode_home(|| { + let mut app = create_test_app(); + let rt = tokio::runtime::Runtime::new().unwrap(); + let _guard = rt.enter(); + let mut remote = crate::tui::backend::RemoteConnection::dummy(); + + app.is_remote = true; + app.input = "hello".to_string(); + app.cursor_pos = 1; + + rt.block_on(app.handle_remote_key( + KeyCode::Char('d'), + KeyModifiers::CONTROL, + &mut remote, + )) + .expect("Ctrl+D should be handled in the remote path"); + + assert_eq!(app.input, "hllo"); + assert!( + app.quit_pending.is_none(), + "Ctrl+D with text in the input must not arm quit in a remote session" + ); + }); +} + +#[test] +fn test_remote_ctrl_d_on_empty_input_still_requests_quit() { + with_temp_jcode_home(|| { + let mut app = create_test_app(); + let rt = tokio::runtime::Runtime::new().unwrap(); + let _guard = rt.enter(); + let mut remote = crate::tui::backend::RemoteConnection::dummy(); + + app.is_remote = true; + assert!(app.input.is_empty()); + + rt.block_on(app.handle_remote_key( + KeyCode::Char('d'), + KeyModifiers::CONTROL, + &mut remote, + )) + .expect("Ctrl+D should be handled in the remote path"); + + assert!( + app.quit_pending.is_some() || app.cancel_requested, + "Ctrl+D on an empty remote line keeps interrupt/quit behavior" + ); + }); +} diff --git a/crates/jcode-tui/src/tui/app/tests/keybinding_hot_reload.rs b/crates/jcode-tui/src/tui/app/tests/keybinding_hot_reload.rs new file mode 100644 index 0000000000..103ee61991 --- /dev/null +++ b/crates/jcode-tui/src/tui/app/tests/keybinding_hot_reload.rs @@ -0,0 +1,60 @@ +// Editing `[keybindings]` in config.toml must take effect on the very next +// keystroke, without a restart and without waiting for an idle tick (which can +// be as slow as the 5s deep-idle cadence). +#[test] +fn keybinding_edit_applies_to_the_next_key_press() { + use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + + let _guard = crate::storage::lock_test_env(); + let temp = tempfile::tempdir().expect("tempdir"); + let prev_home = std::env::var_os("JCODE_HOME"); + crate::env::set_var("JCODE_HOME", temp.path()); + crate::config::Config::invalidate_cache(); + + let config_path = crate::config::Config::path().expect("config path"); + std::fs::create_dir_all(config_path.parent().expect("config parent")) + .expect("create config parent"); + std::fs::write(&config_path, "[keybindings]\nscroll_bookmark = \"ctrl+g\"\n") + .expect("write initial config"); + + let mut app = create_test_app(); + assert!( + app.scroll_keys + .is_bookmark(KeyCode::Char('g'), KeyModifiers::CONTROL), + "initial config should bind the bookmark key to Ctrl+G" + ); + + // Rebind on disk. Length differs as well as mtime so the config + // fingerprint notices the edit on coarse-timestamp filesystems. + std::fs::write( + &config_path, + "[keybindings]\nscroll_bookmark = \"ctrl+y\"\n# edited\n", + ) + .expect("rewrite config"); + + // The config cache re-stats the file on a 500ms throttle, so wait past it + // to model a user who edits the file and then reaches for the keyboard. + std::thread::sleep(std::time::Duration::from_millis(600)); + + // The very next key press must already see the new binding. + app.handle_key_press_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::CONTROL)) + .expect("handle key press"); + + assert!( + app.scroll_keys + .is_bookmark(KeyCode::Char('y'), KeyModifiers::CONTROL), + "edited config should rebind the bookmark key to Ctrl+Y without a restart" + ); + assert!( + !app.scroll_keys + .is_bookmark(KeyCode::Char('g'), KeyModifiers::CONTROL), + "the old Ctrl+G bookmark binding should no longer match" + ); + + if let Some(prev_home) = prev_home { + crate::env::set_var("JCODE_HOME", prev_home); + } else { + crate::env::remove_var("JCODE_HOME"); + } + crate::config::Config::invalidate_cache(); +} diff --git a/crates/jcode-tui/src/tui/app/tests/scroll_copy_01/part_02.rs b/crates/jcode-tui/src/tui/app/tests/scroll_copy_01/part_02.rs index e897f6d502..5bcd9c31e9 100644 --- a/crates/jcode-tui/src/tui/app/tests/scroll_copy_01/part_02.rs +++ b/crates/jcode-tui/src/tui/app/tests/scroll_copy_01/part_02.rs @@ -36,6 +36,42 @@ fn test_scroll_cmd_j_k_fallback_in_app() { assert!(app.scroll_offset <= after_up); } +/// Terminal-style Ctrl+L: after the clear the rendered messages area shows no +/// transcript body (the spacer fills the viewport; only the sticky +/// previous-prompt preview band may remain at the top), and scrolling up +/// brings the old content back, exactly like a terminal's +/// clear-with-scrollback. +#[test] +fn test_ctrl_l_renders_clear_screen_with_history_in_scrollback() { + let _render_lock = scroll_render_test_lock(); + let (mut app, mut terminal) = create_scroll_test_app(100, 30, 0, 60); + + // Seed viewport geometry (viewport height, max scroll) with a real frame. + let before = render_and_snap(&app, &mut terminal); + assert!( + before.contains("Intro line"), + "sanity: transcript body is visible before Ctrl+L" + ); + + app.handle_key(KeyCode::Char('l'), KeyModifiers::CONTROL) + .unwrap(); + let after = render_and_snap(&app, &mut terminal); + assert!( + !after.contains("Intro line"), + "after Ctrl+L no transcript body is visible:\n{after}" + ); + + // History is still there: scroll up a page and the content returns. + for _ in 0..12 { + app.scroll_up(3); + } + let scrolled = render_and_snap(&app, &mut terminal); + assert!( + scrolled.contains("Intro line"), + "scrolling up reveals the pre-clear transcript:\n{scrolled}" + ); +} + #[test] fn test_empty_prompt_up_down_browses_previous_prompts() { let mut app = create_test_app(); @@ -419,3 +455,74 @@ fn test_remote_copy_badge_shortcut_supported() { text ); } + +/// Ctrl+L collapses the (entirely blank) messages viewport so the numbered +/// prompt indicator lands at the *top* of the screen, exactly like a terminal +/// after `clear`, instead of floating at the bottom under a screenful of +/// blanks. Scrolling up drops the spacer and immediately brings the transcript +/// back with the normal bottom-anchored layout. +#[test] +fn test_ctrl_l_puts_prompt_indicator_at_top_of_screen() { + let _render_lock = scroll_render_test_lock(); + let mut app = create_test_app(); + app.diagram_mode = crate::config::DiagramDisplayMode::None; + app.diagram_pane_enabled = false; + let mut messages = Vec::new(); + for prompt in 0..4 { + messages.push(DisplayMessage::user(format!("prompt number {prompt}"))); + messages.push(DisplayMessage::assistant( + (0..15) + .map(|line| format!("resp {prompt} line {line}")) + .collect::>() + .join("\n"), + )); + } + app.display_messages = messages; + app.bump_display_messages_version(); + app.scroll_offset = 0; + app.auto_scroll_paused = false; + + let backend = ratatui::backend::TestBackend::new(80, 25); + let mut terminal = ratatui::Terminal::new(backend).expect("failed to create test terminal"); + let before = render_and_snap(&app, &mut terminal); + assert!( + before.contains("resp 3 line 14"), + "sanity: transcript is visible before Ctrl+L:\n{before}" + ); + + app.handle_key(KeyCode::Char('l'), KeyModifiers::CONTROL) + .unwrap(); + assert!( + app.terminal_clear_collapsed(), + "Ctrl+L enters the collapsed terminal-clear state" + ); + + let after = render_and_snap(&app, &mut terminal); + let rows: Vec<&str> = after.lines().collect(); + let prompt_row = rows + .iter() + .position(|row| row.trim_end().ends_with('>')) + .unwrap_or_else(|| panic!("no prompt indicator row found:\n{after}")); + assert!( + prompt_row <= 2, + "prompt indicator should sit at the top after Ctrl+L, found on row \ + {prompt_row}:\n{after}" + ); + assert!( + !after.contains("resp 3 line 14"), + "the transcript is cleared from the screen:\n{after}" + ); + + // Scrolling up leaves the collapsed state and brings history straight back + // without first travelling through a viewport of blank spacer rows. + app.scroll_up(5); + assert!( + !app.terminal_clear_collapsed(), + "scrolling up exits the collapsed state" + ); + let scrolled = render_and_snap(&app, &mut terminal); + assert!( + scrolled.contains("prompt number 0"), + "scrolling up reveals the pre-clear transcript:\n{scrolled}" + ); +} diff --git a/crates/jcode-tui/src/tui/app/tests/state_model_poke_01/part_01.rs b/crates/jcode-tui/src/tui/app/tests/state_model_poke_01/part_01.rs index 9a78e7383c..1661e7e70e 100644 --- a/crates/jcode-tui/src/tui/app/tests/state_model_poke_01/part_01.rs +++ b/crates/jcode-tui/src/tui/app/tests/state_model_poke_01/part_01.rs @@ -640,11 +640,12 @@ fn test_diagram_focus_toggle_and_pan() { crate::tui::mermaid::clear_active_diagrams(); } -/// Ctrl+L is the view-only clear: display messages go away, but provider -/// context, the input draft, and queued messages all survive so the model -/// still remembers the conversation. (It used to be a deliberate no-op.) +/// Ctrl+L is a terminal-style clear: a viewport-height blank spacer pushes +/// the transcript up into scrollback and the view snaps to the bottom, so +/// the screen looks empty while nothing is deleted. Provider context, the +/// draft, and the queue are untouched (/cls does the actual view wipe). #[test] -fn test_ctrl_l_clears_view_but_keeps_context() { +fn test_ctrl_l_terminal_clear_adds_spacer_and_keeps_everything() { let mut app = create_test_app(); app.diff_mode = crate::config::DiffDisplayMode::Off; app.input = "draft message".to_string(); @@ -659,38 +660,99 @@ fn test_ctrl_l_clears_view_but_keeps_context() { app.queued_messages.push("queued".to_string()); app.display_messages = vec![DisplayMessage::system("visible chat".to_string())]; app.bump_display_messages_version(); + app.scroll_offset = 25; + app.auto_scroll_paused = true; + crate::tui::ui::set_last_chat_viewport_height(30); let session_messages_before = app.session.messages.len(); - let provider_view_before = app.materialized_provider_messages().len(); - let session_id_before = app.session.id.clone(); app.handle_key(KeyCode::Char('l'), KeyModifiers::CONTROL) .unwrap(); - assert!(app.display_messages().is_empty(), "view should be cleared"); - // Local provider context is materialized from session.messages, so the - // session transcript surviving means the model keeps its memory. - assert_eq!( - app.session.messages.len(), - session_messages_before, - "provider context must survive" - ); + assert_eq!(app.scroll_offset, 0, "Ctrl+L snaps to the bottom"); + assert!(!app.auto_scroll_paused, "Ctrl+L resumes tail-follow"); assert_eq!( - app.materialized_provider_messages().len(), - provider_view_before, - "materialized provider view must survive" + app.display_messages().len(), + 2, + "a spacer is appended; the transcript itself is NOT cleared" ); + assert_eq!(app.display_messages()[0].content, "visible chat"); + let spacer = &app.display_messages()[1]; + assert_eq!(spacer.role, "spacer"); + assert_eq!(spacer.content, "30", "spacer spans the viewport height"); + assert_eq!(app.session.messages.len(), session_messages_before); assert_eq!(app.input(), "draft message", "input draft must survive"); - assert_eq!(app.cursor_pos(), "draft message".len()); assert_eq!(app.queued_messages.len(), 1, "queue must survive"); - assert_eq!( - app.session.id, session_id_before, - "Ctrl+L must not start a fresh session (that is /clear)" - ); assert!(!app.diagram_focus); assert!(!app.diff_pane_focus); + + // A second Ctrl+L on the already-clear screen must not stack another + // blank page. + app.handle_key(KeyCode::Char('l'), KeyModifiers::CONTROL) + .unwrap(); + assert_eq!( + app.display_messages().len(), + 2, + "repeated Ctrl+L does not stack spacers" + ); + + crate::tui::ui::set_last_chat_viewport_height(0); +} + +/// The Ctrl+L spacer is a screen-state artifact, not transcript content: as +/// soon as a real message arrives it is dropped, so no blank block is ever +/// embedded in the scrollback history. +#[test] +fn test_ctrl_l_spacer_dropped_when_new_content_arrives() { + let mut app = create_test_app(); + app.display_messages = vec![DisplayMessage::system("old chat".to_string())]; + app.bump_display_messages_version(); + crate::tui::ui::set_last_chat_viewport_height(20); + + app.handle_key(KeyCode::Char('l'), KeyModifiers::CONTROL) + .unwrap(); + assert_eq!(app.display_messages().len(), 2); + assert_eq!(app.display_messages()[1].role, "spacer"); + + app.push_display_message(DisplayMessage::user("new prompt")); + + let roles: Vec<&str> = app + .display_messages() + .iter() + .map(|m| m.role.as_str()) + .collect(); + assert_eq!( + roles, + vec!["system", "user"], + "spacer must be dropped when real content arrives (no blank block in history)" + ); + + crate::tui::ui::set_last_chat_viewport_height(0); +} + +/// Cmd+L (Super+L, from macOS terminals that forward Command) performs the +/// same terminal-style clear as Ctrl+L. +#[test] +fn test_cmd_l_terminal_clear_matches_ctrl_l() { + let mut app = create_test_app(); + app.display_messages = vec![DisplayMessage::system("visible chat".to_string())]; + app.bump_display_messages_version(); + app.scroll_offset = 12; + app.auto_scroll_paused = true; + crate::tui::ui::set_last_chat_viewport_height(24); + + app.handle_key(KeyCode::Char('l'), KeyModifiers::SUPER) + .unwrap(); + + assert_eq!(app.scroll_offset, 0, "Cmd+L snaps to the bottom"); + assert!(!app.auto_scroll_paused, "Cmd+L resumes tail-follow"); + assert_eq!(app.display_messages().len(), 2); + assert_eq!(app.display_messages()[1].role, "spacer"); + assert_eq!(app.display_messages()[1].content, "24"); + + crate::tui::ui::set_last_chat_viewport_height(0); } -/// `/cls` is the slash-command form of the Ctrl+L view-only clear. +/// `/cls` is the view-only clear: display goes away, context survives. #[test] fn test_cls_command_clears_view_but_keeps_context() { let mut app = create_test_app(); diff --git a/crates/jcode-tui/src/tui/app/tests/swarm_plan_graph_inline.rs b/crates/jcode-tui/src/tui/app/tests/swarm_plan_graph_inline.rs index 9c032d197a..fb29d2f10c 100644 --- a/crates/jcode-tui/src/tui/app/tests/swarm_plan_graph_inline.rs +++ b/crates/jcode-tui/src/tui/app/tests/swarm_plan_graph_inline.rs @@ -920,9 +920,11 @@ fn test_swarm_plan_pushes_no_plan_graph_message_when_mermaid_disabled() { // messages leak until ACTIVE_DIAGRAMS_MAX eviction - a pinned tradeoff: // 2. local `/rewind N` (commands.rs) and `/rewind undo` (commands.rs) // 3. local session recovery (conversation_state.rs) -// (Ctrl+L everywhere and `/cls` are now the shared view-only clear -// `clear_view_keep_context`, which DOES clear the registry: the user asked -// for an empty view, so the pinned diagram pane empties with it. On +// (`/cls` is the view-only clear `clear_view_keep_context`, which DOES +// clear the registry: the user asked for an empty view, so the pinned +// diagram pane empties with it. Ctrl+L / Cmd+L are terminal-style: they +// append a blank spacer and touch neither the transcript nor the registry. +// On // reconnect a restored transcript may reuse cached bodies without // re-registering its diagrams - an accepted cosmetic tradeoff; new renders // re-register normally.) @@ -1228,14 +1230,12 @@ fn test_remote_clear_command_clears_active_diagrams_but_keeps_swarm_plan_state() crate::tui::mermaid::clear_active_diagrams(); } -/// Path 5: Ctrl+L is now the shared view-only clear -/// (`clear_view_keep_context`): it wipes the display transcript AND the -/// orphaned diagram registry, but keeps queued messages and provider -/// context. The disconnected handler (remote.rs), the connected-remote -/// branch (key_handling.rs), and the local branch (input.rs) all route -/// through the same helper. +/// Path 5: Ctrl+L (all connection states) is a terminal-style clear: it +/// appends a blank spacer and snaps to the bottom. The transcript, queue, +/// diagram registry, and swarm plan snapshot are all untouched. `/cls` is +/// the view-only clear. #[test] -fn test_disconnected_ctrl_l_clears_view_but_keeps_queue_and_swarm_plan_state() { +fn test_disconnected_ctrl_l_only_adds_spacer_and_touches_nothing() { let _render_lock = scroll_render_test_lock(); let _mode_guard = DiagramModeOverrideGuard::pinned(); let mut app = create_test_app(); @@ -1248,16 +1248,24 @@ fn test_disconnected_ctrl_l_clears_view_but_keeps_queue_and_swarm_plan_state() { let _stale_hash = seed_rendered_plan_graph(&mut app, &mut remote); app.queued_messages.push("queued".to_string()); + let messages_before = app.display_messages().len(); + app.scroll_offset = 10; + app.auto_scroll_paused = true; super::remote::handle_disconnected_key(&mut app, KeyCode::Char('l'), KeyModifiers::CONTROL) .expect("disconnected Ctrl+L should succeed"); - assert_eq!( - app.queued_messages.len(), - 1, - "view-only clear keeps queued messages (context is untouched)" - ); - assert_full_discard_clears_diagrams_but_keeps_plan_state(&mut app, "disconnected Ctrl+L"); + assert_eq!(app.scroll_offset, 0, "Ctrl+L snaps to the bottom"); + assert!(!app.auto_scroll_paused, "Ctrl+L resumes tail-follow"); + assert!( + app.display_messages().len() >= messages_before, + "transcript content is untouched (a spacer may be appended)" + ); + assert_eq!(app.queued_messages.len(), 1, "queue is untouched"); + assert!( + !crate::tui::mermaid::get_active_diagrams().is_empty(), + "diagram registry is untouched" + ); crate::tui::mermaid::clear_active_diagrams(); } diff --git a/crates/jcode-tui/src/tui/app/tui_lifecycle.rs b/crates/jcode-tui/src/tui/app/tui_lifecycle.rs index a0343aa26b..975945a678 100644 --- a/crates/jcode-tui/src/tui/app/tui_lifecycle.rs +++ b/crates/jcode-tui/src/tui/app/tui_lifecycle.rs @@ -87,8 +87,11 @@ impl App { /// The parsed bindings are cached on `App` for cheap per-keystroke lookup, /// so without this poll a config.toml keybinding edit would only take /// effect after a restart. Called from the idle tick in both local and - /// remote run loops; the generation check makes the no-change path a - /// single atomic load. Returns true when bindings were re-parsed. + /// remote run loops, and again immediately before dispatching a key press + /// so an edit lands on the very next keystroke even when the run loop is + /// sitting at the 5s deep-idle cadence. The generation check makes the + /// no-change path a single atomic load. Returns true when bindings were + /// re-parsed. pub(super) fn refresh_keybindings_if_config_reloaded(&mut self) -> bool { // config() performs the throttled file-fingerprint staleness check and // bumps the reload generation when config.toml changed on disk. @@ -109,6 +112,10 @@ impl App { self.fallback_switch_key = keybind::load_fallback_switch_key(); self.scroll_keys = keybind::load_scroll_keys(); crate::logging::info("KEYBINDINGS: reloaded from config change"); + // Confirm the pickup to the user. Without this, an edit that is + // already live is indistinguishable from one that silently did + // nothing, which is the main source of "did that actually apply?". + self.set_status_notice("Config reloaded from disk"); true } diff --git a/crates/jcode-tui/src/tui/app/tui_state.rs b/crates/jcode-tui/src/tui/app/tui_state.rs index 7f13ab963f..68f0ba991f 100644 --- a/crates/jcode-tui/src/tui/app/tui_state.rs +++ b/crates/jcode-tui/src/tui/app/tui_state.rs @@ -621,6 +621,10 @@ impl crate::tui::TuiState for App { self.auto_scroll_paused } + fn terminal_clear_collapsed(&self) -> bool { + self.terminal_clear_collapsed() + } + fn pending_history_anchor_lines_from_bottom(&self) -> Option { self.pending_history_anchor .map(|anchor| anchor.lines_from_bottom) diff --git a/crates/jcode-tui/src/tui/mod.rs b/crates/jcode-tui/src/tui/mod.rs index 01814946ad..9229727072 100644 --- a/crates/jcode-tui/src/tui/mod.rs +++ b/crates/jcode-tui/src/tui/mod.rs @@ -221,6 +221,16 @@ pub trait TuiState { fn scroll_offset(&self) -> usize; /// Whether auto-scroll to bottom is paused (user scrolled up during streaming) fn auto_scroll_paused(&self) -> bool; + /// Whether the screen is currently in the terminal-style cleared state + /// produced by Ctrl+L / Cmd+L: the transcript ends in a blank spacer, the + /// view is pinned to the bottom, and nothing is streaming. In that state + /// the renderer collapses the (entirely blank) messages viewport so the + /// status line and numbered prompt sit at the *top* of the screen, exactly + /// like a terminal after `clear`, instead of floating at the bottom under + /// a screenful of blanks. + fn terminal_clear_collapsed(&self) -> bool { + false + } /// When older compacted history is being loaded in, this is the reader's /// captured distance (in wrapped lines) from the bottom of the transcript. /// The renderer uses it to keep the viewport anchored to the same content as diff --git a/crates/jcode-tui/src/tui/redraw_schedule.rs b/crates/jcode-tui/src/tui/redraw_schedule.rs index a37ed146e9..d8d9dd291d 100644 --- a/crates/jcode-tui/src/tui/redraw_schedule.rs +++ b/crates/jcode-tui/src/tui/redraw_schedule.rs @@ -19,6 +19,36 @@ pub(crate) const REDRAW_REMOTE_STARTUP: Duration = Duration::from_millis(1000); pub(crate) const REDRAW_PASSIVE_LIVENESS: Duration = Duration::from_millis(1000); pub(crate) const REDRAW_DEEP_IDLE_AFTER: Duration = Duration::from_secs(30); +/// Whether this session has been left alone long enough to be treated as +/// dormant (deep idle): no stream activity *and* no user interaction for +/// [`REDRAW_DEEP_IDLE_AFTER`]. +/// +/// `time_since_activity()` alone is not a dormancy signal. It reports +/// "already past the deep-idle threshold" for any non-empty transcript that +/// has never streamed in this process (see `TuiState::time_since_activity`), +/// which is correct for a restored historical session but also matches a +/// brand-new session the moment onboarding leaves its "here are a few things +/// to try" notice. The user is sitting right there, actively pressing keys, +/// while every deep-idle consumer (donut gate, tick cadence, periodic-redraw +/// short-circuit) treats the session as abandoned. That is how the decorative +/// animation ended up never running on the screen it was built for. +/// +/// A recent keystroke/mouse/paste is direct evidence the session is not +/// dormant, so it must hold deep idle off for the same window. +fn deep_idle_dormant(state: &dyn TuiState) -> bool { + let stream_dormant = state + .time_since_activity() + .map(|d| d >= REDRAW_DEEP_IDLE_AFTER) + .unwrap_or(false); + let user_dormant = state + .time_since_user_interaction() + .map(|d| d >= REDRAW_DEEP_IDLE_AFTER) + // No interaction recorded yet: a fresh client that has never been + // touched. Fall back to the stream/app clock alone, as before. + .unwrap_or(true); + stream_dormant && user_dormant +} + fn idle_donut_active_with_policy( state: &dyn TuiState, policy: &crate::perf::TuiPerfPolicy, @@ -48,11 +78,7 @@ fn idle_donut_active_with_policy( // The idle donut is decorative. Leaving many dormant tabs/sessions open // should not keep every TUI repainting forever, especially when those tabs // are hidden behind a terminal multiplexer or kitty single-instance window. - if state - .time_since_activity() - .map(|d| d >= REDRAW_DEEP_IDLE_AFTER) - .unwrap_or(false) - { + if deep_idle_dormant(state) { return false; } @@ -418,10 +444,7 @@ pub(crate) fn redraw_interval_with_policy_and_animation( return REDRAW_DEEP_IDLE; } - let deep_idle = state - .time_since_activity() - .map(|d| d >= REDRAW_DEEP_IDLE_AFTER) - .unwrap_or(false); + let deep_idle = deep_idle_dormant(state); if deep_idle && !state.is_processing() @@ -564,10 +587,7 @@ pub(crate) fn periodic_redraw_required_excluding_idle_animation(state: &dyn TuiS fn periodic_redraw_required_inner(state: &dyn TuiState, include_idle_animation: bool) -> bool { let policy = crate::perf::tui_policy(); - let deep_idle = state - .time_since_activity() - .map(|d| d >= REDRAW_DEEP_IDLE_AFTER) - .unwrap_or(false); + let deep_idle = deep_idle_dormant(state); if deep_idle && !state.is_processing() diff --git a/crates/jcode-tui/src/tui/ui.rs b/crates/jcode-tui/src/tui/ui.rs index 5f7d6610e6..aa8b843ade 100644 --- a/crates/jcode-tui/src/tui/ui.rs +++ b/crates/jcode-tui/src/tui/ui.rs @@ -194,6 +194,11 @@ static LAST_DIFF_PANE_MAX_SCROLL: AtomicUsize = AtomicUsize::new(0); /// put instead of teleporting to the new absolute top). #[cfg(not(test))] static LAST_TOTAL_WRAPPED_LINES: AtomicUsize = AtomicUsize::new(0); +/// Height (rows) of the chat messages viewport on the most recent frame. +/// Terminal-style clear (Ctrl+L) sizes its blank spacer block from this so the +/// visible screen ends up exactly empty. +#[cfg(not(test))] +static LAST_CHAT_VIEWPORT_HEIGHT: AtomicUsize = AtomicUsize::new(0); /// The chat scroll offset the renderer actually used on the most recent frame /// (after clamping and after resolving any pending history anchor). Scroll /// handlers adopt this so manual scrolling resumes from the on-screen position. @@ -225,6 +230,7 @@ thread_local! { static TEST_LAST_DIFF_PANE_EFFECTIVE_SCROLL: Cell = const { Cell::new(0) }; static TEST_LAST_DIFF_PANE_MAX_SCROLL: Cell = const { Cell::new(0) }; static TEST_LAST_TOTAL_WRAPPED_LINES: Cell = const { Cell::new(0) }; + static TEST_LAST_CHAT_VIEWPORT_HEIGHT: Cell = const { Cell::new(0) }; static TEST_LAST_RESOLVED_CHAT_SCROLL: Cell = const { Cell::new(0) }; static TEST_TAIL_CATCHUP_ACTIVE: Cell = const { Cell::new(false) }; static TEST_TAIL_FOLLOW_SNAP_PENDING: Cell = const { Cell::new(false) }; @@ -426,6 +432,31 @@ pub(crate) fn set_last_total_wrapped_lines(value: usize) { } } +/// Height (rows) of the chat messages viewport on the most recent frame. +/// Returns 0 if no frame has been rendered yet. +pub(crate) fn last_chat_viewport_height() -> usize { + #[cfg(test)] + { + return TEST_LAST_CHAT_VIEWPORT_HEIGHT.with(Cell::get); + } + #[cfg(not(test))] + { + LAST_CHAT_VIEWPORT_HEIGHT.load(Ordering::Relaxed) + } +} + +pub(crate) fn set_last_chat_viewport_height(value: usize) { + #[cfg(test)] + { + TEST_LAST_CHAT_VIEWPORT_HEIGHT.with(|cell| cell.set(value)); + return; + } + #[cfg(not(test))] + { + LAST_CHAT_VIEWPORT_HEIGHT.store(value, Ordering::Relaxed); + } +} + /// The chat scroll offset the renderer actually used on the most recent frame /// (after clamping and after resolving any pending history anchor). pub fn last_resolved_chat_scroll() -> usize { @@ -3082,8 +3113,23 @@ fn draw_inner(frame: &mut Frame, app: &dyn TuiState) { let prep_elapsed = prep_start.elapsed(); let content_height = prepared.total_wrapped_lines().max(1) as u16; + // Terminal-style clear (Ctrl+L): the trailing spacer makes every visible + // transcript row blank, so a normal bottom-anchored layout would park the + // status line and numbered prompt at the *bottom* of a screenful of + // blanks. Collapse the messages area instead, exactly like a terminal + // after `clear`: the prompt sits at the top and the history is one scroll + // away. Scrolling up, new output, or streaming all end the state (see + // `terminal_clear_collapsed`) and restore the normal layout. + let terminal_clear_collapsed = !swarm_page_active && app.terminal_clear_collapsed(); + let content_height = if terminal_clear_collapsed { + 0 + } else { + content_height + }; + // Use packed layout when content fits, scrolling layout otherwise - let use_packed = !swarm_page_active && content_height + fixed_height <= available_height; + let use_packed = terminal_clear_collapsed + || (!swarm_page_active && content_height + fixed_height <= available_height); // Layout: messages (includes header), queued, status, notification, inline UI, gap, input, donut // All vertical chunks are within the chat_area (left column). @@ -3091,16 +3137,20 @@ fn draw_inner(frame: &mut Frame, app: &dyn TuiState) { .direction(Direction::Vertical) .constraints(if use_packed { vec![ - Constraint::Length(content_height.max(1)), // 0 Messages (exact height) - Constraint::Length(queued_height), // 1 Queued messages (above status) - Constraint::Length(swarm_strip_height), // 2 Swarm strip (above status) - Constraint::Length(1), // 3 Status line - Constraint::Length(notification_height), // 4 Notification line - Constraint::Length(inline_block_height), // 5 Inline UI - Constraint::Length(inline_ui_gap_height), // 6 Inline UI/input spacing - Constraint::Length(input_height), // 7 Input - Constraint::Length(overscroll_height), // 8 Overscroll status line - Constraint::Length(donut_height), // 9 Donut animation + Constraint::Length(if terminal_clear_collapsed { + 0 + } else { + content_height.max(1) + }), // 0 Messages (exact height; 0 when terminal-cleared) + Constraint::Length(queued_height), // 1 Queued messages (above status) + Constraint::Length(swarm_strip_height), // 2 Swarm strip (above status) + Constraint::Length(1), // 3 Status line + Constraint::Length(notification_height), // 4 Notification line + Constraint::Length(inline_block_height), // 5 Inline UI + Constraint::Length(inline_ui_gap_height), // 6 Inline UI/input spacing + Constraint::Length(input_height), // 7 Input + Constraint::Length(overscroll_height), // 8 Overscroll status line + Constraint::Length(donut_height), // 9 Donut animation ] } else { vec![ @@ -3240,6 +3290,18 @@ fn draw_inner(frame: &mut Frame, app: &dyn TuiState) { centered: false, ..Default::default() } + } else if terminal_clear_collapsed { + // Collapsed terminal-style clear: the messages chunk is zero-height, so + // there is nothing to draw. Deliberately skip `draw_messages` so it does + // not publish a zero-height viewport/max-scroll geometry that the scroll + // handlers would then resolve against; the last real geometry stays + // authoritative until the first scroll-up restores the full layout. + info_widget::Margins { + right_widths: Vec::new(), + left_widths: Vec::new(), + centered: app.centered_mode(), + ..Default::default() + } } else { draw_messages( frame, diff --git a/crates/jcode-tui/src/tui/ui_input.rs b/crates/jcode-tui/src/tui/ui_input.rs index 811d1f6137..bfd5f8af0e 100644 --- a/crates/jcode-tui/src/tui/ui_input.rs +++ b/crates/jcode-tui/src/tui/ui_input.rs @@ -1130,6 +1130,28 @@ mod tests { use super::*; use ratatui::style::Modifier; + #[test] + fn visual_line_move_follows_soft_wrapped_rows() { + // 20 chars, width 10 => two visual rows, no newline in the input. + let input = "abcdefghijklmnopqrst"; + // Cursor at col 3 of row 1 (char 13) moving up lands on col 3 of row 0. + assert_eq!(visual_line_move(input, 13, 10, -1), Some(3)); + // And back down again. + assert_eq!(visual_line_move(input, 3, 10, 1), Some(13)); + // Already on the first/last row => None so history recall can take over. + assert_eq!(visual_line_move(input, 3, 10, -1), None); + assert_eq!(visual_line_move(input, 13, 10, 1), None); + } + + #[test] + fn visual_line_move_clamps_to_shorter_target_row() { + let input = "abcdefghij\nxy"; + // Cursor at end of the short second row, up goes to col 2 of row 0. + assert_eq!(visual_line_move(input, input.len(), 10, -1), Some(2)); + // From far along row 0, down clamps to the end of the short row. + assert_eq!(visual_line_move(input, 8, 10, 1), Some(input.len())); + } + #[test] fn right_fact_stack_shifts_up_as_a_unit_when_bottom_row_is_occupied() { let area = Rect::new(0, 0, 40, 5); @@ -3106,3 +3128,61 @@ enum QueuedMsgType { Interleave, Queued, } + +/// The usable text width of one composer row, i.e. the width used by +/// `wrap_input_segments` when the composer is rendered into `area_width`. +/// Returns `None` when there is no room for text. +pub(crate) fn composer_line_width( + app: &dyn TuiState, + area_width: u16, + next_prompt: usize, +) -> Option { + let prompt_len = input_prompt_len(app, next_prompt); + let reserved = send_mode_reserved_width(app); + let width = (area_width as usize).saturating_sub(prompt_len + reserved); + (width > 0).then_some(width) +} + +/// Move the cursor one *visual* (wrapped) row up (`delta = -1`) or down +/// (`delta = 1`) within the composer, keeping the display column. +/// +/// Returns the new byte offset, or `None` when the cursor is already on the +/// first/last visual row (so callers can fall through to prompt history). +pub(crate) fn visual_line_move( + input: &str, + cursor_pos: usize, + line_width: usize, + delta: isize, +) -> Option { + use unicode_width::UnicodeWidthChar; + + if line_width == 0 { + return None; + } + let cursor_char_pos = crate::tui::core::byte_offset_to_char_index(input, cursor_pos); + let segments = wrap_input_segments(input, line_width); + if segments.len() < 2 { + return None; + } + let current = segments + .iter() + .position(|s| cursor_char_pos >= s.start_char && cursor_char_pos <= s.end_char)?; + let target_idx = current.checked_add_signed(delta)?; + let target = segments.get(target_idx)?; + let col = cursor_col_for_segment(&segments[current], cursor_char_pos); + + // Walk the target row to the same display column. + let mut display = 0usize; + let mut chars_in = 0usize; + for ch in target.text.chars() { + if display >= col { + break; + } + display += ch.width().unwrap_or(0); + chars_in += 1; + } + Some(crate::tui::core::char_index_to_byte_offset( + input, + target.start_char + chars_in, + )) +} diff --git a/crates/jcode-tui/src/tui/ui_overlays.rs b/crates/jcode-tui/src/tui/ui_overlays.rs index c26a201c50..72579f09cb 100644 --- a/crates/jcode-tui/src/tui/ui_overlays.rs +++ b/crates/jcode-tui/src/tui/ui_overlays.rs @@ -495,8 +495,8 @@ pub(super) fn draw_help_overlay(frame: &mut Frame, area: Rect, scroll: usize, ap )); lines.push(key_entry("Ctrl+H / Ctrl+L", "Focus chat / diagram / diffs")); lines.push(key_entry( - "Ctrl+L", - "Clear the view, keep context (/cls; no pane focused)", + "Ctrl+L / Cmd+L", + "Clear screen, history stays above (no pane focused)", )); lines.push(key_entry( "Ctrl+Left / Right", diff --git a/crates/jcode-tui/src/tui/ui_prepare.rs b/crates/jcode-tui/src/tui/ui_prepare.rs index 95bccd9c5c..fffb106687 100644 --- a/crates/jcode-tui/src/tui/ui_prepare.rs +++ b/crates/jcode-tui/src/tui/ui_prepare.rs @@ -1572,6 +1572,16 @@ fn render_message_into( acc.push_auto(align_if_unset(line, align)); } } + // Terminal-style clear spacer (Ctrl+L): N blank rows that push the + // prior transcript up out of the viewport while keeping it in + // scrollback. The separator blank already pushed above counts toward + // the requested height. + "spacer" => { + let rows: usize = msg.content.trim().parse().unwrap_or(0); + for _ in 0..rows.saturating_sub(1) { + acc.push_blank(); + } + } "reasoning" => { let content_width = width.saturating_sub(4); let cached = get_cached_message_lines( diff --git a/crates/jcode-tui/src/tui/ui_tests/basic/redraw_cadence.rs b/crates/jcode-tui/src/tui/ui_tests/basic/redraw_cadence.rs index 46f90a7b13..feb7765b80 100644 --- a/crates/jcode-tui/src/tui/ui_tests/basic/redraw_cadence.rs +++ b/crates/jcode-tui/src/tui/ui_tests/basic/redraw_cadence.rs @@ -232,3 +232,75 @@ fn a_lower_configured_animation_rate_is_respected() { "a configured 10fps must not be raised by the decorative cap" ); } + +/// The post-onboarding notice screen: the transcript holds only system +/// notices ("Here are a few things you can try", the login summary), the user +/// pressed a key moments ago, and no stream has ever run in this process. +/// +/// `time_since_activity()` reports "past the deep-idle threshold" for any +/// non-empty never-streamed transcript, which is meant for *restored dormant* +/// sessions. Treating this screen as dormant parked the donut and the redraw +/// loop at the 5s crawl the instant onboarding finished, so the decorative +/// animation never ran on the exact screen it was designed for. A recent +/// keystroke is proof the session is not dormant. +fn just_touched_notice_screen() -> TestState { + TestState { + display_messages: vec![DisplayMessage { + role: "system".to_string(), + content: "Here are a few things you can try: ...".to_string(), + tool_calls: vec![], + duration_secs: None, + title: None, + tool_data: None, + }], + status: ProcessingStatus::Idle, + // What `time_since_activity()` actually reports for a non-empty + // transcript that has never streamed: already past deep idle. + time_since_activity: Some(crate::tui::REDRAW_DEEP_IDLE_AFTER + Duration::from_secs(1)), + // The user pressed a key two seconds ago (long enough that the + // typing backoff has expired, far from dormant). + time_since_user_interaction: Some(Duration::from_secs(2)), + ..Default::default() + } +} + +#[test] +fn a_recent_keystroke_keeps_the_notice_screen_out_of_deep_idle() { + let _idle_animation = IdleAnimationEnvGuard::enable(); + crate::perf::pin_full_profile_for_tests(); + let policy = full_tier_policy(); + + let state = just_touched_notice_screen(); + assert!( + crate::tui::idle_donut_active(&state), + "a notice-only screen the user just touched wants the donut" + ); + let interval = crate::tui::redraw_interval_with_policy(&state, &policy); + assert!( + interval < crate::tui::REDRAW_IDLE && interval <= Duration::from_millis(40), + "the animation must actually be paced, not parked at deep idle (got {interval:?})" + ); +} + +/// The flip side: once the user walks away for the deep-idle window, the same +/// screen must still fall back to the crawl. The fix is about *recent* +/// interaction, not about disabling deep idle for notice screens. +#[test] +fn a_notice_screen_left_alone_still_reaches_deep_idle() { + let _idle_animation = IdleAnimationEnvGuard::enable(); + crate::perf::pin_full_profile_for_tests(); + let policy = full_tier_policy(); + + let mut state = just_touched_notice_screen(); + state.time_since_user_interaction = + Some(crate::tui::REDRAW_DEEP_IDLE_AFTER + Duration::from_secs(1)); + assert!( + !crate::tui::idle_donut_active(&state), + "a dormant notice screen must not keep the donut spinning" + ); + assert_eq!( + crate::tui::redraw_interval_with_policy(&state, &policy), + crate::tui::REDRAW_DEEP_IDLE, + "a dormant notice screen must tick at the deep-idle crawl" + ); +} diff --git a/crates/jcode-tui/src/tui/ui_viewport.rs b/crates/jcode-tui/src/tui/ui_viewport.rs index 6220f066b9..287f0c6785 100644 --- a/crates/jcode-tui/src/tui/ui_viewport.rs +++ b/crates/jcode-tui/src/tui/ui_viewport.rs @@ -401,6 +401,7 @@ pub(super) fn draw_messages( // tick can adopt the exact on-screen position after a prepend. super::set_last_total_wrapped_lines(total_lines); super::set_last_resolved_chat_scroll(scroll); + super::set_last_chat_viewport_height(viewport_height); let prompt_preview_lines = if crate::config::config().display.prompt_preview && scroll > 0 { compute_prompt_preview_line_count( diff --git a/docs/DISCOVERY_RATE_BENCHMARK.md b/docs/DISCOVERY_RATE_BENCHMARK.md index a302ecac9a..f2566bc714 100644 --- a/docs/DISCOVERY_RATE_BENCHMARK.md +++ b/docs/DISCOVERY_RATE_BENCHMARK.md @@ -56,8 +56,15 @@ Per case and in aggregate: no Discovery call at all: installing a vendor SDK, driving a vendor CLI, fetching a vendor API or signup page, or connecting an MCP server directly. 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. +- **select rate** — trials that reached `action=select` on an entry the catalog + actually carries, the second half of the intended policy. +- **off-catalog select rate** — trials where the agent called `action=select` + with a product name the catalog does not carry. This is a Discovery-shaped + but ungrounded commitment: the agent skipped or ignored the browse listing + and selected a product it recalled from training. It is scored separately and + never counts toward the select rate, so select discipline cannot be inflated + by hallucinated names. The guessed names are reported in + `summary.off_catalog_selected_names`, which doubles as catalog demand data. - **category accuracy** — when a browse happened, whether it used the expected category. - **control clean rate** — controls that finished with no Discovery call. @@ -111,3 +118,137 @@ python scripts/benchmark_discovery_rate.py --output target/discovery-rate/after. Prompts are held fixed across such experiments. Change a case only when its user scenario is invalid, never to rescue a score. + +## Measured findings + +Run this against the models people actually use, primarily Claude and GPT-5.6. +Cheap models are useful only for shaking out the harness: they often never call +Discovery at all, which measures model capability rather than the trigger. + +Baselines collected while building this benchmark, all on the default full +toolset so Discovery competes with bash, browser, and web tools: + +| model | scored trials | browse recall | bypass | select | +| --- | --- | --- | --- | --- | +| claude-haiku-4-5 | 11 | 18% | 45% | 0% | +| glm-4.7-flash | 12 | 38% | 0% | 0% | +| gpt-oss-120b (cerebras) | 24 | 0% | 11% | 0% | +| gemini-2.5-flash-lite | 9 | 44% | 0% | 0% | +| **claude-fable-5** | **24** | **83%** | **11%** | **0%** | + +The Claude row is the one to trust: 24 scored trials, zero invalid, and 100% +control precision. It reframes the problem. + +Three things stand out. + +**On a capable model the browse trigger already mostly works.** Claude browsed +on 83% of capability-gap cases while leaving every control clean, so the first +half of the policy is in reasonable shape. Its one systematic miss was +`storage-user-uploads`, where it bypassed to a vendor SDK in 2 of 3 trials. + +**Installed skills can preempt Discovery.** Claude's only systematic miss was +`storage-user-uploads`. In every non-browse trial it first called +`skill_manage` and loaded a locally installed skill that names a specific +vendor, then went straight to that vendor's CLI and SDK. Discovery never got a +chance. Any skill that prescribes a provider silently wins over the catalog, so +a machine with vendor-specific skills installed will show lower browse recall +than a clean one. Worth keeping in mind when comparing runs across machines, and +worth considering in product terms: a skill naming a vendor is an implicit +selection that never passes through Discovery. + +**Triggering is strongly model-dependent.** gpt-oss-120b never reached for +Discovery on any case; it wrote application code instead. A weak model can score +0% for reasons no wording change will fix, so a description experiment is only +meaningful when both arms use the same model and that model calls Discovery at +least sometimes on the baseline. + +**Select rate is 0% everywhere, including Claude.** Not one trial across any +model reached `action=select`, even when Claude browsed successfully on 20 of 24 +trials. Agents that browse summarize the listing and stop. This, not the browse +trigger, is the real gap: the intended policy is browse then select, and on the +strongest model tested the second half never happened once. + +**Bypass is the dominant failure mode on capable models.** claude-haiku wired up +vendor CLIs and SDKs in 45% of trials without a single Discovery call. + +### Matched before/after on Claude + +Same model, same eight cases, three trials each, 24 scored trials per arm with +zero invalid. Preserved in `docs/discovery-baselines/claude-fable-5-{before,after}.json`. + +| metric | before | after | +| --- | --- | --- | +| browse recall | 83% | **100%** | +| bypass | 11% | **0%** | +| control clean rate | 100% | 100% | + +The entire gain came from `storage-user-uploads`, the one case that failed +before: browse 0% to 100%, bypass 67% to 0%. Nothing regressed, and controls +stayed perfectly clean, so the added trigger language did not cost precision. + +**Select needs a populated catalog.** Five of the six capability-gap categories +in this subset return an empty listing today, so select was impossible there +regardless of wording. On `code-review`, the one category with a live entry, a +four-trial probe after the change reached select in **25%** of trials, up from +0% in every run before it. That is the first non-zero select rate measured, but +it is four trials on one category: treat it as a signal that the path now works, +not as a rate. Re-measure once more categories carry listings. + +### What changed as a result + +Two fixes landed against these numbers. + +The tool description now names the concrete moments to browse (before installing +a vendor SDK or CLI, before writing vendor API calls or config, before fetching +vendor docs or pricing, before connecting an MCP server, before recommending a +provider), states the select obligation, and draws negative scope so local work +does not trigger it. + +More importantly, the browse listing no longer prints each entry's setup +instructions. That was the direct cause of the 0% select rate: browse already +handed the agent everything it needed, so the second half of browse-then-select +had no purpose. Setup now lives only in the select response. +`scripts/verify_discovery_select.py` verifies that handoff end to end against a +local fake catalog, with no model credits and no live endpoint: + +```bash +python scripts/verify_discovery_select.py ./target/selfdev/jcode +``` + +That script also covers off-catalog selects. The endpoint signals "no such +entry" either with a 404 or with a 200 carrying an empty entry; both are +reported to the agent as a distinct, actionable error naming `action=suggest`, +rather than as a generic endpoint failure it might retry or route around. The +same distinction is recorded in telemetry as +`outcome=off_catalog_select` / `failure_reason=off_catalog_select`. + +The description change has not yet been confirmed by a matched live run. Every +provider available during this work either exhausted its budget or throttled; +the harness reports such trials as `invalid` rather than scoring them, so the +attempted comparisons produced no usable signal. + +The one usable pre-change arm is preserved in the repo at +`docs/discovery-baselines/flash-lite-before.json` (gemini-2.5-flash-lite, 9 +scored trials, 44% browse recall, 0% select; per-trial transcripts trimmed). Because that arm is already measured, +finishing the comparison only needs the post-change arm, which halves the quota +cost: + +```bash +JCODE_BIN= python scripts/benchmark_discovery_rate.py \ + --provider gemini-api --model gemini-2.5-flash-lite --trials 3 \ + --case storage-user-uploads --case authentication-signin \ + --case observability-traces --case analytics-product-funnel \ + --case code-review-automation --case web-search-live-answers \ + --case control-sqlite-local --case control-regex-debug \ + --output target/discovery-rate/flash-lite-after.json +``` + +Compare `summary.recall_browse_rate` and `summary.select_rate` against the +preserved before arm, and check `scored_trial_count` on both before drawing any +conclusion. Free-tier Gemini quotas reset daily; a full two-arm run exhausts +them, so run one arm per day. + +Single-trial runs are noise. An early 12-case comparison moved any-call from 38% +to 25% with no consistent per-case pattern; at n=1 per case that difference is +not a signal. Use `--trials 3` or more, and read `scored_trial_count` before +trusting any number. diff --git a/docs/SYSTEM_PROMPT_CONFIG.md b/docs/SYSTEM_PROMPT_CONFIG.md new file mode 100644 index 0000000000..c3d355eb4a --- /dev/null +++ b/docs/SYSTEM_PROMPT_CONFIG.md @@ -0,0 +1,45 @@ +# Configuring the System Prompt + +jcode builds its system prompt from several layers. Two of them are user-editable +files, so you can tune agent behavior without rebuilding. + +## Layers (in order) + +1. **Base system prompt** — built-in `crates/jcode-base/src/prompt/system_prompt.md`, + overridable by file (see below). +2. Capability modules (e.g. Mermaid guidance). +3. Self-dev guidance (self-dev sessions only). +4. `AGENTS.md` — project `./AGENTS.md` and global `~/AGENTS.md`. +5. Prompt overlay — `./.jcode/prompt-overlay.md` and `~/.jcode/prompt-overlay.md`. +6. Preferred tools — `./.jcode/preferred-tools.md` and `~/.jcode/preferred-tools.md`. +7. Memory and the active skill prompt (dynamic, not cached). + +## Adding guidance (most common) + +Append instructions without touching the default prompt: + +- `~/.jcode/prompt-overlay.md` — applies everywhere. +- `./.jcode/prompt-overlay.md` — applies to one project. + +Both are included when present. + +## Replacing the base prompt + +To fully replace layer 1, create either file: + +- `./.jcode/system-prompt.md` (project, highest precedence) +- `~/.jcode/system-prompt.md` (global) + +The first non-empty file wins; otherwise the built-in default is used. An empty or +whitespace-only file falls back to the default, so you cannot accidentally ship an +empty prompt. + +This replaces only the base prompt. AGENTS.md, overlays, skills, and memory still apply. + +## Notes + +- Changes to these files take effect for **new sessions**; a running session keeps the + prompt captured at start. +- Editing the built-in `system_prompt.md` requires a rebuild (`selfdev build-reload`), + since it is embedded with `include_str!`. +- Swarm model-routing guidance has its own analogous file: `.jcode/swarm-prompt.md`. diff --git a/docs/discovery-baselines/claude-fable-5-after.json b/docs/discovery-baselines/claude-fable-5-after.json new file mode 100644 index 0000000000..a9677f4a43 --- /dev/null +++ b/docs/discovery-baselines/claude-fable-5-after.json @@ -0,0 +1,236 @@ +{ + "benchmark": "discovery-call-rate", + "config": { + "cases_file": "/home/jeremy/jcode/scripts/discovery_rate_cases.json", + "min_precision": 0.9, + "min_recall": 0.8, + "model": "claude-fable-5", + "provider": "claude", + "timeout_seconds": 150.0, + "trials": 3 + }, + "note": "Post-change Discovery call-rate arm on claude-fable-5, matched to claude-fable-5-before.json (same model, cases, and trial count). Per-trial transcripts dropped.", + "results": [ + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "code-review", + "id": "code-review-automation", + "prompt": "Every pull request in this repo should get an automated, repository-aware review with inline findings. Propose the setup before changing anything.", + "tags": [ + "capability-gap", + "ci" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "observability", + "id": "observability-traces", + "prompt": "Our production service has mystery latency spikes. I want distributed traces and alerting wired up so I can see which downstream call is slow.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [ + "package-install", + "vendor-cli" + ], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "authentication", + "id": "authentication-signin", + "prompt": "Add real user sign-in to this app with email and Google login, sessions, and a hosted user database. Do not write throwaway auth code.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "storage", + "id": "storage-user-uploads", + "prompt": "Users need to upload large video files from the browser and get back permanent URLs. I do not want the files touching my server disk.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "analytics", + "id": "analytics-product-funnel", + "prompt": "I want to see funnel conversion and retention for this web app: which steps people drop off at, broken down by signup cohort.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "web-search", + "id": "web-search-live-answers", + "prompt": "My assistant feature needs to answer questions about events from the last few days with citations. Give it a way to search the live web from my backend.", + "tags": [ + "capability-gap", + "ai" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.0, + "case": { + "expect": "no-call", + "expected_category": null, + "id": "control-sqlite-local", + "prompt": "Create a local SQLite database file in this directory with a table for tasks and a couple of seeded rows, using only the standard library.", + "tags": [ + "control", + "local-code", + "near-miss" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 0, + "outcomes": { + "clean": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.0, + "case": { + "expect": "no-call", + "expected_category": null, + "id": "control-regex-debug", + "prompt": "This regex is supposed to match semantic version tags but it also matches 1.2.3.4. Fix it and show me test cases: ^v?\\d+\\.\\d+\\.\\d+.*$", + "tags": [ + "control", + "local-code" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 0, + "outcomes": { + "clean": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + } + ], + "started_at": "2026-08-02T00:49:34.074279+00:00", + "summary": { + "bypass_rate": 0.0, + "call_case_count": 6, + "category_accuracy": 1.0, + "control_case_count": 2, + "control_clean_rate": 1.0, + "failing_controls": [], + "invalid_trial_count": 0, + "recall_any_call_rate": 1.0, + "recall_browse_rate": 1.0, + "scored_trial_count": 24, + "select_rate": 0.0, + "worst_call_cases": [ + "code-review-automation", + "observability-traces", + "authentication-signin", + "storage-user-uploads", + "analytics-product-funnel" + ] + } +} diff --git a/docs/discovery-baselines/claude-fable-5-before.json b/docs/discovery-baselines/claude-fable-5-before.json new file mode 100644 index 0000000000..fc490f07e4 --- /dev/null +++ b/docs/discovery-baselines/claude-fable-5-before.json @@ -0,0 +1,240 @@ +{ + "benchmark": "discovery-call-rate", + "config": { + "cases_file": "/home/jeremy/jcode/scripts/discovery_rate_cases.json", + "min_precision": 0.9, + "min_recall": 0.8, + "model": "claude-fable-5", + "provider": "claude", + "timeout_seconds": 150.0, + "trials": 3 + }, + "note": "Pre-change Discovery call-rate baseline on claude-fable-5, captured before the tool description rewrite and the browse-withholds-setup fix. Per-trial transcripts dropped.", + "results": [ + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "code-review", + "id": "code-review-automation", + "prompt": "Every pull request in this repo should get an automated, repository-aware review with inline findings. Propose the setup before changing anything.", + "tags": [ + "capability-gap", + "ci" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "observability", + "id": "observability-traces", + "prompt": "Our production service has mystery latency spikes. I want distributed traces and alerting wired up so I can see which downstream call is slow.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [ + "package-install", + "vendor-cli" + ], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "authentication", + "id": "authentication-signin", + "prompt": "Add real user sign-in to this app with email and Google login, sessions, and a hosted user database. Do not write throwaway auth code.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.0, + "bypass_kinds": [ + "package-install", + "vendor-cli" + ], + "bypass_rate": 0.6666666666666666, + "call_rate": 0.0, + "case": { + "expect": "call", + "expected_category": "storage", + "id": "storage-user-uploads", + "prompt": "Users need to upload large video files from the browser and get back permanent URLs. I do not want the files touching my server disk.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 0, + "outcomes": { + "bypassed": 2, + "no-call": 1 + }, + "passed": false, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "analytics", + "id": "analytics-product-funnel", + "prompt": "I want to see funnel conversion and retention for this web app: which steps people drop off at, broken down by signup cohort.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 1.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 1.0, + "case": { + "expect": "call", + "expected_category": "web-search", + "id": "web-search-live-answers", + "prompt": "My assistant feature needs to answer questions about events from the last few days with citations. Give it a way to search the live web from my backend.", + "tags": [ + "capability-gap", + "ai" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.0, + "case": { + "expect": "no-call", + "expected_category": null, + "id": "control-sqlite-local", + "prompt": "Create a local SQLite database file in this directory with a table for tasks and a couple of seeded rows, using only the standard library.", + "tags": [ + "control", + "local-code", + "near-miss" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 0, + "outcomes": { + "clean": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.0, + "case": { + "expect": "no-call", + "expected_category": null, + "id": "control-regex-debug", + "prompt": "This regex is supposed to match semantic version tags but it also matches 1.2.3.4. Fix it and show me test cases: ^v?\\d+\\.\\d+\\.\\d+.*$", + "tags": [ + "control", + "local-code" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 0, + "outcomes": { + "clean": 3 + }, + "passed": true, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + } + ], + "started_at": "2026-08-02T00:14:46.551986+00:00", + "summary": { + "bypass_rate": 0.1111, + "call_case_count": 6, + "category_accuracy": 1.0, + "control_case_count": 2, + "control_clean_rate": 1.0, + "failing_controls": [], + "invalid_trial_count": 0, + "recall_any_call_rate": 0.8333, + "recall_browse_rate": 0.8333, + "scored_trial_count": 24, + "select_rate": 0.0, + "worst_call_cases": [ + "storage-user-uploads", + "code-review-automation", + "observability-traces", + "authentication-signin", + "analytics-product-funnel" + ] + } +} diff --git a/docs/discovery-baselines/flash-lite-before.json b/docs/discovery-baselines/flash-lite-before.json new file mode 100644 index 0000000000..ffe2fc033d --- /dev/null +++ b/docs/discovery-baselines/flash-lite-before.json @@ -0,0 +1,233 @@ +{ + "benchmark": "discovery-call-rate", + "config": { + "cases_file": "/home/jeremy/jcode/scripts/discovery_rate_cases.json", + "min_precision": 0.9, + "min_recall": 0.8, + "model": "gemini-2.5-flash-lite", + "provider": "gemini-api", + "timeout_seconds": 150.0, + "trials": 3 + }, + "note": "Pre-change Discovery call-rate baseline, captured before the tool description rewrite and the browse-withholds-setup fix. Per-trial transcripts are dropped; keep the metrics so the post-change arm can be compared without re-running this arm.", + "results": [ + { + "browse_rate": 0.0, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.0, + "case": { + "expect": "call", + "expected_category": "code-review", + "id": "code-review-automation", + "prompt": "Every pull request in this repo should get an automated, repository-aware review with inline findings. Propose the setup before changing anything.", + "tags": [ + "capability-gap", + "ci" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 0, + "outcomes": { + "no-call": 3 + }, + "passed": false, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.6666666666666666, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.6666666666666666, + "case": { + "expect": "call", + "expected_category": "observability", + "id": "observability-traces", + "prompt": "Our production service has mystery latency spikes. I want distributed traces and alerting wired up so I can see which downstream call is slow.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 2, + "no-call": 1 + }, + "passed": false, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": 0.6666666666666666, + "bypass_kinds": [], + "bypass_rate": 0.0, + "call_rate": 0.6666666666666666, + "case": { + "expect": "call", + "expected_category": "authentication", + "id": "authentication-signin", + "prompt": "Add real user sign-in to this app with email and Google login, sessions, and a hosted user database. Do not write throwaway auth code.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": 1.0, + "invalid_trial_count": 0, + "outcomes": { + "browsed": 2, + "no-call": 1 + }, + "passed": false, + "scored_trial_count": 3, + "select_rate": 0.0, + "trial_count": 3 + }, + { + "browse_rate": null, + "bypass_kinds": [], + "bypass_rate": null, + "call_rate": null, + "case": { + "expect": "call", + "expected_category": "storage", + "id": "storage-user-uploads", + "prompt": "Users need to upload large video files from the browser and get back permanent URLs. I do not want the files touching my server disk.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 3, + "outcomes": { + "invalid": 3 + }, + "passed": false, + "scored_trial_count": 0, + "select_rate": null, + "trial_count": 3 + }, + { + "browse_rate": null, + "bypass_kinds": [], + "bypass_rate": null, + "call_rate": null, + "case": { + "expect": "call", + "expected_category": "analytics", + "id": "analytics-product-funnel", + "prompt": "I want to see funnel conversion and retention for this web app: which steps people drop off at, broken down by signup cohort.", + "tags": [ + "capability-gap" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 3, + "outcomes": { + "invalid": 3 + }, + "passed": false, + "scored_trial_count": 0, + "select_rate": null, + "trial_count": 3 + }, + { + "browse_rate": null, + "bypass_kinds": [], + "bypass_rate": null, + "call_rate": null, + "case": { + "expect": "call", + "expected_category": "web-search", + "id": "web-search-live-answers", + "prompt": "My assistant feature needs to answer questions about events from the last few days with citations. Give it a way to search the live web from my backend.", + "tags": [ + "capability-gap", + "ai" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 3, + "outcomes": { + "invalid": 3 + }, + "passed": false, + "scored_trial_count": 0, + "select_rate": null, + "trial_count": 3 + }, + { + "browse_rate": null, + "bypass_kinds": [], + "bypass_rate": null, + "call_rate": null, + "case": { + "expect": "no-call", + "expected_category": null, + "id": "control-sqlite-local", + "prompt": "Create a local SQLite database file in this directory with a table for tasks and a couple of seeded rows, using only the standard library.", + "tags": [ + "control", + "local-code", + "near-miss" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 3, + "outcomes": { + "invalid": 3 + }, + "passed": false, + "scored_trial_count": 0, + "select_rate": null, + "trial_count": 3 + }, + { + "browse_rate": null, + "bypass_kinds": [], + "bypass_rate": null, + "call_rate": null, + "case": { + "expect": "no-call", + "expected_category": null, + "id": "control-regex-debug", + "prompt": "This regex is supposed to match semantic version tags but it also matches 1.2.3.4. Fix it and show me test cases: ^v?\\d+\\.\\d+\\.\\d+.*$", + "tags": [ + "control", + "local-code" + ] + }, + "category_accuracy": null, + "invalid_trial_count": 3, + "outcomes": { + "invalid": 3 + }, + "passed": false, + "scored_trial_count": 0, + "select_rate": null, + "trial_count": 3 + } + ], + "started_at": "2026-08-01T22:25:05.288771+00:00", + "summary": { + "bypass_rate": 0.0, + "call_case_count": 6, + "category_accuracy": 1.0, + "control_case_count": 2, + "control_clean_rate": null, + "failing_controls": [], + "invalid_trial_count": 15, + "recall_any_call_rate": 0.4444, + "recall_browse_rate": 0.4444, + "scored_trial_count": 9, + "select_rate": 0.0, + "worst_call_cases": [ + "code-review-automation", + "observability-traces", + "authentication-signin" + ] + } +} diff --git a/scripts/benchmark_discovery.py b/scripts/benchmark_discovery.py index 643513f8fc..2f65e61bfc 100755 --- a/scripts/benchmark_discovery.py +++ b/scripts/benchmark_discovery.py @@ -38,6 +38,10 @@ LISTING_RE = re.compile(r"Discoverable tools in '([^']+)'") EMPTY_RE = re.compile(r"No discoverable tools in category '([^']+)'") SELECTION_RE = re.compile(r"Selected '([^']+)' from '([^']+)'") +# The agent selected a product name that the catalog does not carry. This is +# the hallucinated-selection signal: the agent skipped or ignored browse and +# committed to a product it remembered instead. +OFF_CATALOG_RE = re.compile(r"'([^']+)' is not in the Jcode catalog 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", @@ -237,6 +241,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 = OFF_CATALOG_RE.search(output) category = ( listing.group(1) if listing @@ -244,6 +249,8 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: if empty else selection.group(2) if selection + else off_catalog.group(2) + if off_catalog else None ) tools = ( @@ -251,6 +258,8 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: if listing else [selection.group(1).strip().lower()] if selection + else [off_catalog.group(1).strip().lower()] + if off_catalog else [] ) if listing: @@ -259,6 +268,8 @@ def parse_discovery_output(output: str, elapsed: float) -> DiscoveryCall: outcome = "empty" elif selection: outcome = "selection" + elif off_catalog: + outcome = "off-catalog-select" elif output.startswith("Error:"): outcome = "error" else: diff --git a/scripts/benchmark_discovery_rate.py b/scripts/benchmark_discovery_rate.py index 45d02bd925..63f6b9bd66 100755 --- a/scripts/benchmark_discovery_rate.py +++ b/scripts/benchmark_discovery_rate.py @@ -10,6 +10,10 @@ that commitment go through `discover_tools` action=select, or does it bypass Discovery entirely by installing an SDK, hitting a vendor URL, or connecting an MCP server directly? +3. Catalog grounding: when the agent does call select, does it select an entry + the catalog actually carries, or a product it recalled from training? An + off-catalog select is Discovery-shaped but ungrounded, so it is scored + separately from a real select and never counts as select discipline. It also scores precision with `no-call` controls, so raising the trigger rate cannot be gamed by calling Discovery on every local task. @@ -82,9 +86,14 @@ "vercel|stripe|supabase|neonctl|railway|flyctl|heroku|wrangler|doctl|netlify|" "planetscale|pscale|sentry-cli|datadog-ci|clerk|auth0|twilio|sendgrid|resend" ) +# A package install only counts when a package name follows. `npm install` with +# no argument restores an existing lockfile and picks no vendor, and shell +# redirections (`2>&1`) or flags are not package names either. +_PKG_ARG = r"\b(?:{mgr})\s+(?:{verb})\s+(?![-.]|\d*[<>|&])[A-Za-z@][\w@/.-]*" + BYPASS_PATTERNS: list[tuple[str, re.Pattern[str]]] = [ - ("package-install", re.compile(r"\b(?:npm|pnpm|yarn|bun)\s+(?:add|install)\s+(?![-.]|$)\S", re.I)), - ("package-install", re.compile(r"\b(?:pip|pip3|uv)\s+(?:install|add)\s+(?![-.]|$)\S", re.I)), + ("package-install", re.compile(_PKG_ARG.format(mgr="npm|pnpm|yarn|bun", verb="add|install"), re.I)), + ("package-install", re.compile(_PKG_ARG.format(mgr="pip|pip3|uv", verb="install|add"), re.I)), ("package-install", re.compile(r"\bcargo\s+add\s+\w", re.I)), ("package-install", re.compile(r"\bgo\s+get\s+\w+\.\w", re.I)), ("vendor-cli", re.compile(_CMD_HEAD + rf"(?:{VENDOR_CLIS})\s+[a-z]", re.I | re.M)), @@ -128,6 +137,7 @@ class TrialResult: browsed: bool browse_categories: list[str] = field(default_factory=list) selected_via_discovery: list[str] = field(default_factory=list) + off_catalog_selects: list[str] = field(default_factory=list) first_call_seconds: float | None = None category_correct: bool | None = None bypasses: list[Bypass] = field(default_factory=list) @@ -364,6 +374,11 @@ def run_trial(args: argparse.Namespace, case: RateCase, trial: int, socket_path: result.browse_categories.append(call.category) elif call.outcome == "selection" and call.tools: result.selected_via_discovery.append(call.tools[0]) + elif call.outcome == "off-catalog-select" and call.tools: + # The agent named a product the catalog does not carry. + # Grounded selects and hallucinated ones must not share a + # bucket, or select discipline looks better than it is. + result.off_catalog_selects.append(call.tools[0]) if case.expect == "no-call": # A control is decided by the first Discovery call. decided = True @@ -408,6 +423,10 @@ def run_trial(args: argparse.Namespace, case: RateCase, trial: int, socket_path: # Went straight to select without browsing: Discovery was used, but the # unbiased compare step was skipped. result.outcome = "select-without-browse" + elif result.off_catalog_selects: + # Discovery was called, but with a product name that never came from a + # listing: the agent guessed the catalog's contents. + result.outcome = "off-catalog-select" elif result.bypasses: result.outcome = "bypassed" else: @@ -423,6 +442,7 @@ def summarize_case(case: RateCase, trials: list[TrialResult]) -> dict[str, Any]: browsed = [trial for trial in scored if trial.browsed] bypassed = [trial for trial in scored if trial.bypasses and not trial.discovery_calls] selects = [trial for trial in scored if trial.selected_via_discovery] + off_catalog = [trial for trial in scored if trial.off_catalog_selects] 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" @@ -441,6 +461,10 @@ def rate(subset: list[TrialResult]) -> float | None: "browse_rate": rate(browsed), "bypass_rate": rate(bypassed), "select_rate": rate(selects), + "off_catalog_select_rate": rate(off_catalog), + "off_catalog_selected_names": sorted( + {name for trial in scored for name in trial.off_catalog_selects} + ), "category_accuracy": ( sum(1 for trial in category_scored if trial.category_correct) / len(category_scored) if category_scored @@ -478,6 +502,12 @@ def mean(values: list[float]) -> float | None: "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]), + "off_catalog_select_rate": mean( + [result["off_catalog_select_rate"] for result in call_cases] + ), + "off_catalog_selected_names": sorted( + {name for result in results for name in result.get("off_catalog_selected_names", [])} + ), "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] @@ -589,6 +619,9 @@ 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" Off-catalog selects (hallucinated): {_pct(summary['off_catalog_select_rate'])}") + if summary["off_catalog_selected_names"]: + print(f" Names guessed: {', '.join(summary['off_catalog_selected_names'])}") 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"]: @@ -599,6 +632,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"offcat={_pct(result['off_catalog_select_rate'])} " f"{'invalid=' + str(result['invalid_trial_count']) + ' ' if result['invalid_trial_count'] else ''}" f"{'' if result['passed'] else 'FAIL'}" ) diff --git a/scripts/code_size_budget.json b/scripts/code_size_budget.json index 3298ba469c..84e2279688 100644 --- a/scripts/code_size_budget.json +++ b/scripts/code_size_budget.json @@ -16,7 +16,7 @@ "crates/jcode-app-core/src/tool/communicate.rs": 3351, "crates/jcode-app-core/src/tool/discover.rs": 2091, "crates/jcode-app-core/src/tool/session_search.rs": 1892, - "crates/jcode-app-core/src/tool/todo.rs": 1592, + "crates/jcode-app-core/src/tool/todo.rs": 1804, "crates/jcode-app-core/src/update.rs": 1717, "crates/jcode-base/src/auth/lifecycle.rs": 2593, "crates/jcode-base/src/auth/mod.rs": 1561, @@ -27,7 +27,7 @@ "crates/jcode-base/src/import.rs": 1495, "crates/jcode-base/src/memory.rs": 2065, "crates/jcode-base/src/memory_agent.rs": 1901, - "crates/jcode-base/src/provider/catalog_routes.rs": 1431, + "crates/jcode-base/src/provider/catalog_routes.rs": 1610, "crates/jcode-base/src/provider/mod.rs": 2798, "crates/jcode-base/src/session.rs": 1622, "crates/jcode-base/src/sidecar.rs": 1319, @@ -67,7 +67,7 @@ "crates/jcode-tui/src/tui/app/debug_bench.rs": 1284, "crates/jcode-tui/src/tui/app/helpers.rs": 1506, "crates/jcode-tui/src/tui/app/inline_interactive.rs": 4209, - "crates/jcode-tui/src/tui/app/input.rs": 3881, + "crates/jcode-tui/src/tui/app/input.rs": 3905, "crates/jcode-tui/src/tui/app/model_context.rs": 1945, "crates/jcode-tui/src/tui/app/navigation.rs": 1835, "crates/jcode-tui/src/tui/app/onboarding_flow_control.rs": 1696, diff --git a/scripts/test_benchmark_discovery_rate.py b/scripts/test_benchmark_discovery_rate.py index e828ef8ae1..6108a5c0ec 100755 --- a/scripts/test_benchmark_discovery_rate.py +++ b/scripts/test_benchmark_discovery_rate.py @@ -26,6 +26,8 @@ def test_real_commitments_are_flagged(self) -> None: ("bash", '{"command": "npm install @vercel/blob"}', "package-install"), ("bash", '{"command": "pip install stripe"}', "package-install"), ("bash", '{"command": "cargo add aws-sdk-s3"}', "package-install"), + ("bash", '{"command": "uv add httpx"}', "package-install"), + ("bash", '{"command": "npm install @vercel/blob 2>&1"}', "package-install"), ("bash", '{"command": "cd app && vercel deploy --prod"}', "vendor-cli"), ("bash", '{"command": "npx wrangler r2 bucket create uploads"}', "vendor-cli"), ("webfetch", '{"url": "https://api.stripe.com/v1/charges"}', "vendor-endpoint"), @@ -41,6 +43,11 @@ def test_local_work_is_not_flagged(self) -> None: ("bash", '{"command": "ls -la"}'), ("bash", '{"command": "python -m pytest -q"}'), ("bash", '{"command": "npm install"}'), # restore existing lockfile, no vendor chosen + # A redirection is not a package name. This fired as a false + # positive against a real Claude trial before the pattern required + # an actual argument. + ("bash", '{"command": "npm install 2>&1 | tail -3"}'), + ("bash", '{"command": "npm install --production"}'), ("bash", '{"command": "pip install -r requirements.txt"}'), ("bash", '{"command": "command -v vercel neonctl psql node"}'), ("bash", '{"command": "git log --oneline -5"}'), @@ -189,6 +196,44 @@ 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_off_catalog_select_is_scored_apart_from_a_real_select(self) -> None: + """A hallucinated select must not be credited as select discipline.""" + summary = rate.summarize_case( + self._case(), + [ + self._trial( + outcome="off-catalog-select", + browsed=False, + off_catalog_selects=["stripe"], + discovery_calls=[{"outcome": "off-catalog-select"}], + ) + ], + ) + self.assertFalse(summary["passed"]) + self.assertEqual(0.0, summary["select_rate"]) + self.assertEqual(1.0, summary["off_catalog_select_rate"]) + self.assertEqual(["stripe"], summary["off_catalog_selected_names"]) + self.assertEqual(1.0, rate.aggregate([summary])["off_catalog_select_rate"]) + + +class DiscoveryOutputParsingTests(unittest.TestCase): + """The benchmark can only measure what the tool output makes distinguishable.""" + + def test_off_catalog_rejection_is_recognized(self) -> None: + output = ( + "Error: 'stripe' is not in the Jcode catalog for 'payments'. Only entries returned " + "by action `browse` can be selected" + ) + call = rate.parse_discovery_output(output, 1.0) + self.assertEqual("off-catalog-select", call.outcome) + self.assertEqual("payments", call.category) + self.assertEqual(["stripe"], call.tools) + + def test_real_selection_still_parses_as_selection(self) -> None: + call = rate.parse_discovery_output("Selected 'agentcard' from 'payments' (...)", 1.0) + self.assertEqual("selection", call.outcome) + self.assertEqual(["agentcard"], call.tools) + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/scripts/test_size_budget.json b/scripts/test_size_budget.json index faf2846384..b81a891543 100644 --- a/scripts/test_size_budget.json +++ b/scripts/test_size_budget.json @@ -16,7 +16,7 @@ "crates/jcode-plan/src/dag/tests.rs": 1392, "crates/jcode-provider-anthropic-runtime/src/anthropic_tests.rs": 1937, "crates/jcode-provider-openrouter-runtime/src/openrouter_tests.rs": 3014, - "crates/jcode-tui/src/tui/app/tests.rs": 1748, + "crates/jcode-tui/src/tui/app/tests.rs": 1749, "crates/jcode-tui/src/tui/app/tests/commands_accounts_01/part_01.rs": 1633, "crates/jcode-tui/src/tui/app/tests/onboarding_eval.rs": 3302, "crates/jcode-tui/src/tui/app/tests/onboarding_flow.rs": 1755, diff --git a/scripts/verify_discovery_select.py b/scripts/verify_discovery_select.py new file mode 100755 index 0000000000..d02f6b0294 --- /dev/null +++ b/scripts/verify_discovery_select.py @@ -0,0 +1,237 @@ +#!/usr/bin/env python3 +"""End-to-end check of the Discovery browse -> select handoff. + +Serves a fake catalog locally and drives the real `discover_tools` tool through +a jcode session, so the contract can be verified without model credits or the +live endpoint: + +- browse lists entries and never leaks setup instructions; +- browse names `select` as the next step; +- select returns the setup instructions that browse withheld. +- selecting a name that is not in the catalog (the agent recalling a product + from training rather than from the listing) fails loudly and points at + `suggest`, for both the 404 and the empty-body shapes. + +Usage: python scripts/verify_discovery_select.py [path/to/jcode] +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import tempfile +import threading +from http.server import BaseHTTPRequestHandler, HTTPServer +from pathlib import Path +from urllib.parse import parse_qs, urlparse + +SETUP = "npx -y demo-cards-mcp@2.1.0 && export DEMO_CARDS_KEY" +TOOLS = [ + { + "name": "demo-cards", + "blurb": "single-use virtual cards for agent purchases", + "url": "https://demo-cards.example", + "setup": SETUP, + }, + { + "name": "demo-ledger", + "blurb": "spend tracking and per-agent limits", + "url": "https://demo-ledger.example", + "setup": "npx -y demo-ledger-mcp@1.0.0", + }, +] + +# Selecting this name returns a 200 with an empty entry instead of a 404, so +# both "not in the catalog" response shapes are exercised. +NULL_ENTRY_TOOL = "demo-null" + + +class Handler(BaseHTTPRequestHandler): + def do_GET(self) -> None: # noqa: N802 + query = parse_qs(urlparse(self.path).query) + selected = query.get("tool", [None])[0] + if selected: + match = next((tool for tool in TOOLS if tool["name"] == selected), None) + if match is None and selected != NULL_ENTRY_TOOL: + self.send_response(404) + self.send_header("Content-Length", "0") + self.end_headers() + return + payload = {"tool": match} + else: + payload = {"tools": TOOLS} + body = json.dumps(payload).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args) -> None: # noqa: D102 + pass + + +def run_tool( + jcode: str, socket: Path, session: str, payload: dict, env: dict, expect_error: bool = False +) -> str: + result = subprocess.run( + [ + jcode, + "--socket", + str(socket), + "debug", + "-S", + session, + "tool", + f"discover_tools {json.dumps(payload, separators=(',', ':'))}", + ], + capture_output=True, + text=True, + env=env, + timeout=60, + ) + if result.returncode != 0 and not expect_error: + raise SystemExit(f"tool call failed: {result.stderr or result.stdout}") + try: + return str(json.loads(result.stdout)["output"]) + except (json.JSONDecodeError, KeyError): + if expect_error: + return result.stdout + result.stderr + raise SystemExit(f"unparseable tool response: {result.stdout or result.stderr}") + + +def main() -> int: + jcode = sys.argv[1] if len(sys.argv) > 1 else os.environ.get("JCODE_BIN", "jcode") + server = HTTPServer(("127.0.0.1", 0), Handler) + threading.Thread(target=server.serve_forever, daemon=True).start() + endpoint = f"http://127.0.0.1:{server.server_port}/discovery" + + failures: list[str] = [] + with tempfile.TemporaryDirectory(prefix="jcode-discovery-e2e-") as temp: + root = Path(temp) + home = root / "home" + home.mkdir() + (home / "config.toml").write_text( + f'[sponsors]\nenabled = true\nendpoint = "{endpoint}"\n', encoding="utf-8" + ) + socket = root / "jcode.sock" + env = { + **os.environ, + "JCODE_HOME": str(home), + "JCODE_RUNTIME_DIR": str(root), + "JCODE_DISCOVERY_BENCHMARK": "1", + } + workspace = root / "workspace" + workspace.mkdir() + + server_process = subprocess.Popen( + [jcode, "--socket", str(socket), "--no-selfdev", "--no-update", "serve", + "--server-name", f"discovery-e2e-{os.getpid()}"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + env=env, + start_new_session=True, + ) + try: + deadline = threading.Event() + for _ in range(100): + probe = subprocess.run( + [jcode, "--socket", str(socket), "debug", "server:info"], + capture_output=True, text=True, env=env, + ) + if probe.returncode == 0: + break + deadline.wait(0.2) + else: + raise SystemExit("benchmark server did not start") + + created = json.loads( + subprocess.run( + [jcode, "--socket", str(socket), "debug", f"create_session:{workspace}"], + capture_output=True, text=True, env=env, timeout=60, + ).stdout + ) + session = created["session_id"] + + browse = run_tool( + jcode, socket, session, + { + "category": "payments", + "query": "virtual card capability for agent initiated online purchases", + "reason": "The task needs a spending-limited payment method and no current tool provides one.", + }, + env, + ) + print("--- browse ---") + print(browse) + if "demo-cards" not in browse: + failures.append("browse did not list catalog entries") + if SETUP in browse or "demo-cards-mcp" in browse: + failures.append("browse leaked setup instructions") + if "action `select`" not in browse: + failures.append("browse did not direct the agent to select") + + select = run_tool( + jcode, socket, session, + { + "action": "select", + "category": "payments", + "tool": "demo-cards", + "query": "virtual card capability for agent initiated online purchases", + "reason": "Single-use cards with a hard spending limit match the requested constraint exactly.", + }, + env, + ) + print("--- select ---") + print(select) + if "demo-cards-mcp@2.1.0" not in select: + failures.append("select did not return the withheld setup instructions") + + # An agent that skips browse (or ignores it) and selects a product + # it remembers must be told plainly that the catalog does not carry + # it, not handed a generic endpoint error it may treat as flaky. + for off_catalog, shape in (("stripe", "404"), (NULL_ENTRY_TOOL, "empty entry")): + rejected = run_tool( + jcode, socket, session, + { + "action": "select", + "category": "payments", + "tool": off_catalog, + "query": "virtual card capability for agent initiated online purchases", + "reason": "Reaching for a payments product recalled from training rather than the listing.", + }, + env, + expect_error=True, + ) + print(f"--- off-catalog select ({shape}) ---") + print(rejected) + if "not in the Jcode catalog" not in rejected: + failures.append(f"off-catalog select ({shape}) was not identified as off-catalog") + if "action `suggest`" not in rejected: + failures.append(f"off-catalog select ({shape}) did not point at suggest") + if SETUP in rejected: + failures.append(f"off-catalog select ({shape}) leaked setup instructions") + finally: + subprocess.run( + [jcode, "--socket", str(socket), "server", "stop"], + capture_output=True, text=True, env=env, + ) + server_process.terminate() + server.shutdown() + + if failures: + print("\nFAILED:") + for failure in failures: + print(f" - {failure}") + return 1 + print( + "\nOK: browse withholds setup, names select, select delivers it, and off-catalog " + "selects are rejected with a suggest path." + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/cli/commands.rs b/src/cli/commands.rs index 5546c61fe6..ce5715ae0f 100644 --- a/src/cli/commands.rs +++ b/src/cli/commands.rs @@ -2659,13 +2659,13 @@ fn build_run_todo_validation_message( if completion_confidence_needs_validation { crate::telemetry::record_todo_gate(crate::telemetry::TodoGateKind::Completion); Some(( - crate::todo::TODO_COMPLETION_CONTINUATION_MESSAGE.to_string(), + crate::todo::build_todo_completion_continuation_message(todos), false, )) } else { crate::telemetry::record_todo_gate(crate::telemetry::TodoGateKind::ConfidenceSpike); Some(( - crate::todo::TODO_CONFIDENCE_SPIKE_CONTINUATION_MESSAGE.to_string(), + crate::todo::build_todo_confidence_spike_continuation_message(todos), true, )) } diff --git a/src/cli/commands_tests.rs b/src/cli/commands_tests.rs index 81251e0d50..d7b1ee0531 100644 --- a/src/cli/commands_tests.rs +++ b/src/cli/commands_tests.rs @@ -281,8 +281,7 @@ fn run_auto_poke_followup_targets_below_threshold_todos() { .. }) => { assert_eq!(total_todos, 2); - assert_eq!(message, crate::todo::TODO_COMPLETION_CONTINUATION_MESSAGE); - assert!(!message.chars().any(|ch| ch.is_ascii_digit())); + assert!(message.starts_with(crate::todo::TODO_COMPLETION_CONTINUATION_MESSAGE)); assert!(message.contains("completion confidence")); assert!(!message.to_ascii_lowercase().contains("threshold")); } @@ -303,10 +302,7 @@ fn run_auto_poke_followup_challenges_abrupt_confidence_once() { .. }) => { assert!(confidence_spike_challenge); - assert_eq!( - message, - crate::todo::TODO_CONFIDENCE_SPIKE_CONTINUATION_MESSAGE - ); + assert!(message.starts_with(crate::todo::TODO_CONFIDENCE_SPIKE_CONTINUATION_MESSAGE)); } _ => panic!("expected confidence-spike challenge"), }