diff --git a/Cargo.lock b/Cargo.lock index 55234090..50770d55 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2409,6 +2409,7 @@ dependencies = [ "eventsource-stream", "futures", "gemini_client_rs", + "http", "indexmap", "infer", "insta", diff --git a/Cargo.toml b/Cargo.toml index e8da4424..b163f594 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ grep-printer = { version = "0.3", default-features = false } grep-regex = { version = "0.1", default-features = false } grep-searcher = { version = "0.1", default-features = false } htmd = { version = "0.5", default-features = false } +http = { version = "1", default-features = false } httpmock = { git = "https://github.com/JeanMertz/httpmock", branch = "tweaks", default-features = false } humantime = { version = "2", default-features = false } ignore = { version = "0.4", default-features = false } diff --git a/crates/jp_cli/src/cmd.rs b/crates/jp_cli/src/cmd.rs index a48e0914..df61beb9 100644 --- a/crates/jp_cli/src/cmd.rs +++ b/crates/jp_cli/src/cmd.rs @@ -490,6 +490,18 @@ impl From for Error { ), ] .into(), + TitleGeneration { model, reason } => [ + ("message", "Title generation failed".to_owned()), + ("model", model), + ("reason", reason), + ( + "suggestion", + "Retry with a different model, e.g. `jp conversation title --model `, \ + or set the title directly with `jp conversation edit --title \"...\"`." + .to_owned(), + ), + ] + .into(), CliConfig(error) => { [("message", "CLI Config error".to_owned()), ("error", error)].into() } diff --git a/crates/jp_cli/src/cmd/conversation.rs b/crates/jp_cli/src/cmd/conversation.rs index 7e723ed2..92759fa5 100644 --- a/crates/jp_cli/src/cmd/conversation.rs +++ b/crates/jp_cli/src/cmd/conversation.rs @@ -15,6 +15,7 @@ mod print; mod rm; mod show; pub(crate) mod summarize; +mod title; mod unarchive; mod use_; @@ -32,6 +33,7 @@ impl Conversation { Commands::Edit(args) => args.run(ctx, handles).await, Commands::Fork(args) => args.run(ctx, &handles).await, Commands::Compact(args) => args.run(ctx, handles).await, + Commands::Title(args) => args.run(ctx, handles).await, Commands::Grep(args) => args.run(ctx, handles), Commands::Print(args) => args.run(ctx, &handles), Commands::Path(args) => args.run(ctx, handles), @@ -49,6 +51,7 @@ impl Conversation { Commands::Edit(args) => args.conversation_load_request(), Commands::Fork(args) => args.conversation_load_request(), Commands::Compact(args) => args.conversation_load_request(), + Commands::Title(args) => args.conversation_load_request(), Commands::Grep(args) => args.conversation_load_request(), Commands::Print(args) => args.conversation_load_request(), Commands::Path(args) => args.conversation_load_request(), @@ -69,6 +72,7 @@ impl IntoPartialAppConfig for Conversation { ) -> std::result::Result> { match &self.command { Commands::Compact(args) => args.apply_cli_config(workspace, partial, merged_config), + Commands::Title(args) => args.apply_cli_config(workspace, partial, merged_config), Commands::Show(_) | Commands::Remove(_) | Commands::Edit(_) @@ -119,6 +123,14 @@ enum Commands { #[command(name = "compact")] Compact(compact::Compact), + /// Generate a conversation title with an LLM. + /// + /// Offers the generated candidates as a picker, and applies the chosen one + /// as the conversation's title. + /// Use `edit --title "TITLE"` to set a title without involving a model. + #[command(name = "title", visible_alias = "t")] + Title(title::Title), + /// Search through conversation history. #[command(name = "grep", alias = "rg", visible_alias = "g")] Grep(grep::Grep), diff --git a/crates/jp_cli/src/cmd/conversation/edit.rs b/crates/jp_cli/src/cmd/conversation/edit.rs index 994d5e9f..0905df1e 100644 --- a/crates/jp_cli/src/cmd/conversation/edit.rs +++ b/crates/jp_cli/src/cmd/conversation/edit.rs @@ -4,30 +4,15 @@ use camino::Utf8PathBuf; use chrono::Utc; use crossterm::style::Stylize as _; use inquire::Confirm; -use jp_config::{ - AppConfig, PartialAppConfig, ToPartial as _, model::id::PartialModelIdOrAliasConfig, -}; -use jp_conversation::{ - ConversationEvent, ConversationId, ConversationStream, - event::{ChatRequest, ChatResponse}, - thread::ThreadBuilder, -}; +use jp_conversation::ConversationId; use jp_editor::{EditOutcome, EditRequest}; -use jp_llm::{ - event::Event, - event_builder::EventBuilder, - provider, - retry::{RetryConfig, collect_with_retry}, - title, -}; -use jp_printer::PrinterWriter; use jp_storage::{ LoadError, backend::{FsStorageBackend, LoadBackend, Projection}, }; use jp_workspace::ConversationHandle; -use super::path::resolve_paths; +use super::{path::resolve_paths, title}; use crate::{ cmd::{ ConversationLoadRequest, Output, @@ -74,7 +59,11 @@ pub(crate) struct Edit { #[arg(long = "no-tmp", group = "property")] no_expires_at: bool, - /// Edit the title of the conversation. + /// Set the title of the conversation. + /// + /// Without a value, generates one with an LLM and offers the candidates as + /// a picker — the same as `conversation title`, which additionally exposes + /// the model and candidate count. #[arg(long, group = "property", conflicts_with = "no_title")] title: Option>, @@ -271,14 +260,15 @@ impl Edit { }); } - if let Some(ref title) = self.title { - let events = conv.events().clone(); - let title = match title { - Some(title) => title.clone(), - None => { - generate_titles(&ctx.config(), ctx.printer.out_writer(), events, vec![]) - .await? - } + if let Some(ref new_title) = self.title { + // A bare `--title` asks for a generated one; `conversation + // title` owns that path and its flags. + let title = if let Some(title) = new_title { + title.clone() + } else { + let cfg = ctx.config(); + let events = conv.events().clone(); + title::select(ctx, &cfg, events, title::DEFAULT_COUNT, false).await? }; conv.update_metadata(|m| m.title = Some(title)); @@ -308,115 +298,6 @@ impl Edit { } } -async fn generate_titles( - config: &AppConfig, - mut writer: PrinterWriter<'_>, - mut events: ConversationStream, - mut rejected: Vec, -) -> Result> { - let count = 3; - let model = config - .conversation - .title - .generate - .model - .clone() - .unwrap_or_else(|| config.assistant.model.clone()); - - let model_id = model.id.resolved(); - - let mut partial = PartialAppConfig::empty(); - partial.assistant.model.id = PartialModelIdOrAliasConfig::Id(model_id.to_partial()); - events.add_config_delta(partial); - - let provider = provider::get_provider(model_id.provider, &config.providers.llm)?; - let model_details = provider.model_details(&model_id.name).await?; - - let sections = title::title_instructions(count, &rejected); - let schema = title::title_schema(count); - - let thread = ThreadBuilder::default() - .with_events(events.clone()) - .with_sections(sections) - .build()?; - - let mut thread_events = thread.events.clone(); - thread_events.start_turn(ChatRequest { - content: "Generate titles for this conversation.".into(), - schema: Some(schema), - author: None, - }); - - let query = jp_llm::query::ChatQuery { - thread: jp_conversation::thread::Thread { - events: thread_events, - ..thread - }, - tools: vec![], - tool_choice: jp_config::assistant::tool_choice::ToolChoice::default(), - }; - - let retry_config = RetryConfig::default(); - let llm_events = - collect_with_retry(provider.as_ref(), &model_details, query, &retry_config).await?; - - // Pipe raw streaming events through the EventBuilder so that structured - // JSON chunks are concatenated and parsed into a proper Value (rather than - // individual Value::String fragments). - let mut builder = EventBuilder::new(); - let mut flushed = Vec::new(); - for event in llm_events { - match event { - Event::Part { - index, - part, - metadata, - } => { - builder.handle_part(index, part, metadata); - } - Event::Flush { index, metadata } => { - flushed.extend(builder.handle_flush(index, metadata)); - } - Event::Finished(_) => flushed.extend(builder.drain()), - Event::Patch(_) | Event::KeepAlive => {} - } - } - - let structured_data = flushed - .into_iter() - .filter_map(ConversationEvent::into_chat_response) - .find_map(ChatResponse::into_structured_data); - - let titles = structured_data - .as_ref() - .map(title::extract_titles) - .unwrap_or_default(); - - if titles.is_empty() { - return Err("No titles generated".into()); - } - - let mut choices = titles.clone(); - choices.extend(rejected.clone()); - choices.push("More...".to_owned()); - choices.push("Manually enter a title".to_owned()); - - let result = - inquire::Select::new("Conversation Title", choices).prompt_with_writer(&mut writer)?; - - match result.as_str() { - "More..." => { - rejected.extend(titles); - Box::pin(generate_titles(config, writer, events, rejected)).await - } - "Manually enter a title" => { - let title = inquire::Text::new("Title").prompt_with_writer(&mut writer)?; - Ok(title.trim().to_owned()) - } - choice => Ok(choice.to_owned()), - } -} - /// Duration value for `--tmp`, supporting `now` as an alias for zero duration. #[derive(Debug, Clone, Copy)] struct ExpirationDuration(Duration); diff --git a/crates/jp_cli/src/cmd/conversation/summarize.rs b/crates/jp_cli/src/cmd/conversation/summarize.rs index 80a2d0fa..6ba2b7b2 100644 --- a/crates/jp_cli/src/cmd/conversation/summarize.rs +++ b/crates/jp_cli/src/cmd/conversation/summarize.rs @@ -16,6 +16,7 @@ use jp_llm::{ model::ModelDetails, provider, retry::{RetryConfig, collect_with_retry}, + window, }; use tracing::debug; @@ -89,6 +90,25 @@ pub async fn generate_summary( let provider = provider::get_provider(model_id.provider, &app_cfg.providers.llm)?; let model_details = provider.model_details(&model_id.name).await?; + // The instructions ride in the system prompt and the request in its own + // turn; both share the window with the range. + let overhead = + window::estimate_overhead_chars(Some(instructions), &[], &[], &[]) + user_message.len(); + + if let Some(overflow) = window_overflow(&stream, model_details.context_window, overhead) { + return Err(Error::Summarize { + model: model_id.to_string(), + // Stored indices are 0-based; turn numbers shown to the user are + // 1-based. + reason: format!( + "turns {}..{} {overflow}; compact a smaller range (`--from`/`--to`) or summarize \ + with a larger-window model", + range_from + 1, + range_to + 1, + ), + }); + } + summarize_stream( provider.as_ref(), &model_details, @@ -100,6 +120,33 @@ pub async fn generate_summary( .await } +/// Describe why `stream` does not fit `context_window`, or `None` when it does. +/// +/// A summary stands in for every turn it covers, so a range that doesn't fit is +/// rejected rather than shortened: summarizing only the tail would leave a +/// compaction that claims a range it never read, and the projected conversation +/// would quietly lose the rest. +/// +/// `overhead_chars` is the size of everything else sharing the window (see +/// [`window::estimate_overhead_chars`]). +/// An unknown window always fits — there is no budget to measure against. +fn window_overflow( + stream: &ConversationStream, + context_window: Option, + overhead_chars: usize, +) -> Option { + let context_window = context_window?; + let budget = window::budget_chars(context_window, overhead_chars); + let needed = window::estimate_chars(stream); + + (needed > budget).then(|| { + format!( + "are roughly {needed} characters, which exceeds the ~{budget} that fit in the model's \ + {context_window} token context window" + ) + }) +} + /// Request a summary of `stream`, honouring provider rebuild requests. /// /// A provider that answers with [`FinishReason::Retry`] supplies patches that diff --git a/crates/jp_cli/src/cmd/conversation/summarize_tests.rs b/crates/jp_cli/src/cmd/conversation/summarize_tests.rs index 5b7d2119..252044b0 100644 --- a/crates/jp_cli/src/cmd/conversation/summarize_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/summarize_tests.rs @@ -11,6 +11,7 @@ use jp_llm::{ use super::{ StreamOutcome, collect_range_events, failure_reason, summarize_events, summarize_stream, + window_overflow, }; /// A stream that produced `text` and then stopped for `reason`. @@ -85,6 +86,55 @@ fn chat_request_texts(events: &[jp_conversation::ConversationEvent]) -> Vec, + + /// How many candidate titles to generate. + /// + /// The candidates are offered as a picker, which can also generate a fresh + /// batch or take a hand-written title. + /// Without a terminal to pick with, the first candidate is applied. + #[arg(long, short = 'n', default_value_t = DEFAULT_COUNT, value_parser = parse_count)] + count: usize, + + /// The model to generate the title with. + /// + /// Accepts a model alias or a full `provider/name` ID, the same values as + /// `jp query --model`. + /// Overrides `conversation.title.generate.model` for this invocation. + #[arg(long, short = 'm')] + model: Option, + + /// Print the candidate titles without applying one. + #[arg(long)] + dry_run: bool, +} + +fn parse_count(s: &str) -> std::result::Result { + match s.parse::() { + Ok(0) => Err("at least one title must be generated".to_owned()), + Ok(count) => Ok(count), + Err(error) => Err(error.to_string()), + } +} + +impl Title { + pub(crate) fn conversation_load_request(&self) -> ConversationLoadRequest { + ConversationLoadRequest::explicit_or_session(&self.target) + } + + pub(crate) async fn run(self, ctx: &mut Ctx, handles: Vec) -> Output { + for handle in handles { + self.title_one(ctx, handle).await?; + } + + Ok(()) + } + + async fn title_one(&self, ctx: &mut Ctx, handle: ConversationHandle) -> Output { + let lock = match acquire_lock(LockRequest::from_ctx(handle, ctx)).await? { + LockOutcome::Acquired(lock) => lock, + LockOutcome::NewConversation | LockOutcome::ForkConversation(_) => { + unreachable!("title does not allow new/fork on contention") + } + }; + + let cfg = ctx.config(); + let conv = lock.into_mut(); + let events = conv.events().clone(); + + if self.dry_run { + let candidates = + generate(&cfg, events, self.count, self.model.is_some(), vec![]).await?; + for candidate in candidates { + ctx.printer.println(candidate); + } + return Ok(()); + } + + let title = select(ctx, &cfg, events, self.count, self.model.is_some()).await?; + ctx.printer.println(title.clone()); + conv.update_metadata(|m| m.title = Some(title)); + + Ok(()) + } +} + +impl IntoPartialAppConfig for Title { + fn apply_cli_config( + &self, + _: Option<&Workspace>, + mut partial: PartialAppConfig, + merged_config: Option<&PartialAppConfig>, + ) -> std::result::Result> { + apply_model(&mut partial, self.model.as_deref(), merged_config); + + Ok(partial) + } +} + +/// Generate candidate titles and let the user pick one. +/// +/// The picker offers a fresh batch (carrying the discarded candidates forward +/// so the model avoids repeating them) and a hand-written title. +/// Without a terminal there is nothing to pick with, so the first candidate is +/// returned as-is. +/// +/// `override_model` reads the model from `assistant.model` instead of +/// `conversation.title.generate.model`; see [`generate`]. +pub(super) async fn select( + ctx: &Ctx, + cfg: &AppConfig, + events: ConversationStream, + count: usize, + override_model: bool, +) -> Result { + let mut rejected: Vec = vec![]; + + loop { + let candidates = + generate(cfg, events.clone(), count, override_model, rejected.clone()).await?; + + if !ctx.term.is_tty { + return Ok(candidates + .into_iter() + .next() + .expect("`generate` rejects an empty batch")); + } + + // Discarded candidates stay selectable: a user who asked for more may + // still prefer one of the earlier suggestions. + let mut choices = candidates.clone(); + choices.extend(rejected.iter().cloned()); + choices.push(MORE.to_owned()); + choices.push(MANUAL.to_owned()); + + let mut writer = ctx.printer.prompt_writer(); + let choice = + inquire::Select::new("Conversation Title", choices).prompt_with_writer(&mut writer)?; + + match choice.as_str() { + MORE => rejected.extend(candidates), + MANUAL => { + let title = inquire::Text::new("Title").prompt_with_writer(&mut writer)?; + return Ok(title.trim().to_owned()); + } + _ => return Ok(choice), + } + } +} + +/// Generate `count` candidate titles for a conversation. +/// +/// The model comes from `conversation.title.generate.model`, falling back to +/// the assistant model. +/// With `override_model` it comes from `assistant.model` unconditionally, which +/// is where the config pipeline puts a `--model` flag. +/// +/// `rejected` names titles the model must avoid. +/// +/// # Errors +/// +/// Returns [`Error::TitleGeneration`] if the model produces no titles, and the +/// underlying LLM error if the request itself fails. +async fn generate( + cfg: &AppConfig, + events: ConversationStream, + count: usize, + override_model: bool, + rejected: Vec, +) -> Result> { + let override_id = override_model.then(|| cfg.assistant.model.id.clone()); + let model = title::resolve_model(cfg, override_id.as_ref()); + let model_id = model.id.resolved().clone(); + + let provider = provider::get_provider(model_id.provider, &cfg.providers.llm)?; + let details = provider.model_details(&model_id.name).await?; + + let titles = title::generate(provider.as_ref(), &details, TitleRequest { + events, + model, + count, + rejected, + }) + .await?; + + if titles.is_empty() { + return Err(Error::TitleGeneration { + model: model_id.to_string(), + reason: "the model returned no titles".to_owned(), + }); + } + + Ok(titles) +} + +#[cfg(test)] +#[path = "title_tests.rs"] +mod tests; diff --git a/crates/jp_cli/src/cmd/conversation/title_tests.rs b/crates/jp_cli/src/cmd/conversation/title_tests.rs new file mode 100644 index 00000000..ee02add4 --- /dev/null +++ b/crates/jp_cli/src/cmd/conversation/title_tests.rs @@ -0,0 +1,63 @@ +use clap::Parser as _; +use jp_config::{PartialAppConfig, model::id::PartialModelIdOrAliasConfig}; + +use super::{DEFAULT_COUNT, IntoPartialAppConfig as _, Title}; + +/// Parse a `Title` from `jp conversation title ` for flag tests. +fn parse_title(args: &[&str]) -> Title { + #[derive(clap::Parser)] + struct TestCli { + #[command(flatten)] + title: Title, + } + + let mut argv = vec!["title"]; + argv.extend_from_slice(args); + TestCli::try_parse_from(argv).unwrap().title +} + +#[test] +fn count_defaults_to_three() { + assert_eq!(parse_title(&[]).count, DEFAULT_COUNT); +} + +/// Generating zero titles has no meaningful outcome, so it is rejected at parse +/// time rather than producing an empty picker. +#[test] +fn count_of_zero_is_rejected() { + #[derive(clap::Parser)] + struct TestCli { + #[command(flatten)] + title: Title, + } + + assert!(TestCli::try_parse_from(["title", "--count", "0"]).is_err()); +} + +/// `--model` reaches the title generator through `assistant.model`, which is +/// where `resolve_model`'s override argument reads it from. +#[test] +fn model_flag_lands_on_the_assistant_model() { + let title = parse_title(&["--model", "anthropic/claude-haiku-4-5"]); + let partial = title + .apply_cli_config(None, PartialAppConfig::empty(), None) + .unwrap(); + + assert_eq!( + partial.assistant.model.id, + PartialModelIdOrAliasConfig::from("anthropic/claude-haiku-4-5") + ); +} + +#[test] +fn without_the_model_flag_the_assistant_model_is_untouched() { + let title = parse_title(&[]); + let partial = title + .apply_cli_config(None, PartialAppConfig::empty(), None) + .unwrap(); + + assert_eq!( + partial.assistant.model.id, + PartialAppConfig::empty().assistant.model.id + ); +} diff --git a/crates/jp_cli/src/cmd/query/tool/inquiry.rs b/crates/jp_cli/src/cmd/query/tool/inquiry.rs index 560e1f4f..a37e8c55 100644 --- a/crates/jp_cli/src/cmd/query/tool/inquiry.rs +++ b/crates/jp_cli/src/cmd/query/tool/inquiry.rs @@ -21,19 +21,15 @@ use async_trait::async_trait; use indexmap::IndexMap; use jp_attachment::Attachment; use jp_config::assistant::{sections::SectionConfig, tool_choice::ToolChoice}; -use jp_conversation::{ - ConversationEvent, ConversationStream, EventKind, - event::{ChatRequest, ChatResponse}, - thread::Thread, -}; +use jp_conversation::{ConversationStream, event::ChatRequest, thread::Thread}; use jp_llm::{ Provider, - event::Event, - event_builder::EventBuilder, + event_builder::structured_data, model::ModelDetails, query::ChatQuery, retry::{RetryConfig, collect_with_retry}, tool::ToolDefinition, + window, }; use jp_tool::{AnswerType, Question}; use serde_json::{Map, Value, json}; @@ -227,15 +223,15 @@ impl InquiryBackend for LlmInquiryBackend { // Truncate older events if the inquiry model has a smaller context // window than what the conversation has accumulated. - if let Some(max_tokens) = config.model.context_window { - let overhead = estimate_fixed_overhead_chars( + if let Some(context_window) = config.model.context_window { + let overhead = window::estimate_overhead_chars( config.system_prompt.as_deref(), &config.sections, &self.attachments, &self.tools, ); - truncate_to_fit(&mut events, max_tokens, overhead); + window::truncate_to_fit(&mut events, context_window, overhead); } // Tag the second-to-last provider-visible event with a cache @@ -298,33 +294,8 @@ impl InquiryBackend for LlmInquiryBackend { } }; - // Pipe raw streaming events through the EventBuilder so that - // structured JSON chunks are concatenated and parsed into a - // proper Value (rather than individual Value::String fragments). - let mut builder = EventBuilder::new(); - let mut flushed = Vec::new(); - for event in llm_events { - match event { - Event::Part { - index, - part, - metadata, - } => { - builder.handle_part(index, part, metadata); - } - Event::Flush { index, metadata } => { - flushed.extend(builder.handle_flush(index, metadata)); - } - Event::Finished(_) => flushed.extend(builder.drain()), - Event::Patch(_) | Event::KeepAlive => {} - } - } - - let mut structured_data = flushed - .into_iter() - .filter_map(ConversationEvent::into_chat_response) - .find_map(ChatResponse::into_structured_data) - .ok_or(InquiryError::MissingStructuredData)?; + let mut structured_data = + structured_data(llm_events).ok_or(InquiryError::MissingStructuredData)?; info!( inquiry_id, @@ -385,54 +356,6 @@ impl InquiryBackend for MockInquiryBackend { } } -/// Estimated chars-per-token ratio used for estimation. -const CHARS_PER_TOKEN: usize = 3; - -/// Safety margin for tokenization imprecision (the 3 chars/token ratio varies -/// by content type) and provider framing overhead (JSON wrapping, role tags, -/// structured output injection, etc.). -/// -/// The system prompt, sections, attachments, and tool definitions are now -/// measured explicitly via `estimate_fixed_overhead_chars`, so this factor only -/// needs to cover the remaining approximation error. -const OVERHEAD_FACTOR: usize = 90; // percent - -/// When truncation is needed, target this fraction of the context window. -/// Leaves headroom so subsequent inquiries in the same turn don't re-truncate -/// (which would bust the prompt cache). -const TARGET_FACTOR: usize = 80; // percent - -fn estimate_chars(events: &ConversationStream) -> usize { - events.iter().map(|e| estimate_event_chars(e.event)).sum() -} - -fn token_budget(max_tokens: u32, overhead_chars: usize) -> usize { - let total = (max_tokens as usize) * CHARS_PER_TOKEN * OVERHEAD_FACTOR / 100; - total.saturating_sub(overhead_chars) -} - -fn token_target(max_tokens: u32, overhead_chars: usize) -> usize { - let total = (max_tokens as usize) * CHARS_PER_TOKEN * TARGET_FACTOR / 100; - total.saturating_sub(overhead_chars) -} - -fn estimate_event_chars(event: &ConversationEvent) -> usize { - match &event.kind { - EventKind::ChatRequest(r) => r.content.len(), - EventKind::ChatResponse(ChatResponse::Message { message }) => message.len(), - EventKind::ChatResponse(ChatResponse::Reasoning { reasoning }) => reasoning.len(), - EventKind::ChatResponse(ChatResponse::Structured { data }) => data.to_string().len(), - EventKind::ToolCallRequest(r) => { - r.name.len() + serde_json::to_string(&r.arguments).map_or(0, |s| s.len()) - } - EventKind::ToolCallResponse(r) => { - r.result.as_ref().map_or(0, String::len) - + r.result.as_ref().err().map_or(0, String::len) - } - _ => 0, - } -} - /// Find the iteration index of the second-to-last provider-visible event. /// /// Returns `None` if there are fewer than two visible events. @@ -450,125 +373,6 @@ fn second_last_visible_event_index(events: &ConversationStream) -> Option second_last } -/// Char-based estimate of the fixed overhead that shares the model's context -/// window with conversation events: system prompt, sections, attachments, and -/// tool definitions. -fn estimate_fixed_overhead_chars( - system_prompt: Option<&str>, - sections: &[SectionConfig], - attachments: &[Attachment], - tools: &[ToolDefinition], -) -> usize { - let mut chars = 0; - - if let Some(prompt) = system_prompt { - chars += prompt.len(); - } - - for section in sections { - chars += section.render().len(); - } - - for attachment in attachments { - if let Some(text) = attachment.as_text() { - chars += text.len(); - } - // Binary attachments also consume tokens (base64, etc.) but we can't - // easily measure them. The OVERHEAD_FACTOR margin covers this. - } - - for tool in tools { - chars += tool.name.len(); - if let Some(desc) = tool.docs.schema_description() { - chars += desc.len(); - } - // Parameter schemas are serialized as JSON by providers. - chars += serde_json::to_string(&tool.to_parameters_schema()).map_or(0, |s| s.len()); - } - - chars -} - -/// Drop older events so the conversation fits within the model's context -/// window. -/// -/// The budget is calculated from the model's context window minus the measured -/// fixed overhead (system prompt, sections, attachments, tool definitions). -/// When the estimated char count of conversation events exceeds this budget, -/// events are dropped from the **start** until enough chars have been removed -/// to bring the total under the target. -/// -/// Dropping from the start (oldest events) produces a stable cutoff across -/// multiple inquiry calls within the same turn. -/// Since `ConversationStream` is append-only, the same K oldest events are -/// dropped regardless of how many new events were appended at the end. -/// This preserves the prompt cache prefix across inquiries. -/// -/// After truncation, `sanitize()` restores structural invariants (orphaned tool -/// calls, leading non-user events, etc.). -fn truncate_to_fit(events: &mut ConversationStream, max_tokens: u32, overhead_chars: usize) { - let budget = token_budget(max_tokens, overhead_chars); - let total_chars = estimate_chars(events); - - if total_chars <= budget { - return; - } - - let target = token_target(max_tokens, overhead_chars); - - // Round must_drop up to the nearest 10% of target so that small - // additions at the end of the stream (e.g. a tool response from a - // previous inquiry) don't shift the cutoff point. This keeps the - // prefix stable across multiple inquiry calls in the same turn, - // preserving prompt cache hits on the conversation messages. - let granularity = target / 10; - let raw_drop = total_chars.saturating_sub(target); - let must_drop = match granularity { - 0 => raw_drop, - g => raw_drop.div_ceil(g) * g, - }; - - // Walk from the start (oldest), accumulating chars to drop. - let char_counts: Vec = events - .iter() - .map(|e| estimate_event_chars(e.event)) - .collect(); - - let mut dropped_chars = 0; - let mut dropped_events = 0; - - for count in &char_counts { - if dropped_chars >= must_drop { - break; - } - dropped_chars += count; - dropped_events += 1; - } - - let mut idx = 0; - events.retain(|_| { - let keep = idx >= dropped_events; - idx += 1; - keep - }); - - events.sanitize(); - - // If truncation removed all ChatRequests, the remaining events (assistant - // responses, tool calls) cannot form a valid provider message sequence — - // providers require the first message to be a user message. Clear the - // stream so the inquiry's own ChatRequest (added by the caller via - // `start_turn`) becomes the only content. - if !events.has_chat_request() { - events.clear(); - } - - info!( - max_tokens, - dropped_events, "Truncated inquiry context to fit model window", - ); -} - #[cfg(test)] #[path = "inquiry_tests.rs"] mod tests; diff --git a/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs b/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs index 43c61018..29cc924d 100644 --- a/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs @@ -2,13 +2,15 @@ use std::collections::HashMap; use jp_config::model::id::{ModelIdConfig, ProviderId}; use jp_conversation::{ - ConversationStream, - event::{InquiryQuestion, InquiryRequest, InquirySource, ToolCallRequest, ToolCallResponse}, + ConversationStream, EventKind, + event::{ + ChatResponse, InquiryQuestion, InquiryRequest, InquirySource, ToolCallRequest, + ToolCallResponse, + }, }; use jp_llm::{ event::{Event, FinishReason}, provider::mock::MockProvider, - tool::ToolDocs, }; use super::*; @@ -408,144 +410,6 @@ fn visible_index_skips_inquiry_request() { )); } -#[test] -fn overhead_empty_inputs() { - assert_eq!(estimate_fixed_overhead_chars(None, &[], &[], &[]), 0); -} - -#[test] -fn overhead_system_prompt() { - let prompt = "You are a helpful assistant."; - let result = estimate_fixed_overhead_chars(Some(prompt), &[], &[], &[]); - assert_eq!(result, prompt.len()); -} - -#[test] -fn overhead_sections() { - let section = SectionConfig::default() - .with_tag("instruction") - .with_title("Testing") - .with_content("Do the thing."); - let rendered_len = section.render().len(); - - let result = estimate_fixed_overhead_chars(None, &[section], &[], &[]); - assert_eq!(result, rendered_len); -} - -#[test] -fn overhead_text_attachments() { - let attachment = Attachment::text("file.rs", "fn main() {}"); - let result = estimate_fixed_overhead_chars(None, &[], &[attachment], &[]); - assert_eq!(result, "fn main() {}".len()); -} - -#[test] -fn overhead_binary_attachments_ignored() { - let attachment = Attachment::binary("img.png", vec![0u8; 1000], "image/png"); - let result = estimate_fixed_overhead_chars(None, &[], &[attachment], &[]); - assert_eq!(result, 0); -} - -#[test] -fn overhead_tool_definitions() { - let tool = ToolDefinition { - name: "grep_files".to_string(), - docs: ToolDocs { - summary: Some("Search files.".to_string()), - ..Default::default() - }, - parameters: IndexMap::new(), - }; - let result = estimate_fixed_overhead_chars(None, &[], &[], &[tool]); - // name + description + serialized schema - assert!(result > 0); - assert!(result > "grep_files".len() + "Search files.".len()); -} - -#[test] -fn overhead_combines_all_sources() { - let prompt = "Be helpful."; - let section = SectionConfig::default().with_content("Rule 1."); - let attachment = Attachment::text("f.txt", "hello world"); - let tool = ToolDefinition { - name: "t".to_string(), - docs: ToolDocs::default(), - parameters: IndexMap::new(), - }; - - let combined = estimate_fixed_overhead_chars( - Some(prompt), - std::slice::from_ref(§ion), - std::slice::from_ref(&attachment), - std::slice::from_ref(&tool), - ); - - let sum = estimate_fixed_overhead_chars(Some(prompt), &[], &[], &[]) - + estimate_fixed_overhead_chars(None, std::slice::from_ref(§ion), &[], &[]) - + estimate_fixed_overhead_chars(None, &[], std::slice::from_ref(&attachment), &[]) - + estimate_fixed_overhead_chars(None, &[], &[], std::slice::from_ref(&tool)); - - assert_eq!(combined, sum); -} - -#[test] -fn budget_subtracts_overhead() { - let no_overhead = token_budget(1000, 0); - let with_overhead = token_budget(1000, 500); - assert_eq!(no_overhead - 500, with_overhead); -} - -#[test] -fn budget_saturates_at_zero() { - // Overhead larger than total budget shouldn't underflow. - assert_eq!(token_budget(100, 999_999), 0); -} - -#[test] -fn target_subtracts_overhead() { - let no_overhead = token_target(1000, 0); - let with_overhead = token_target(1000, 500); - assert_eq!(no_overhead - 500, with_overhead); -} - -#[test] -fn truncate_no_op_when_within_budget() { - let mut events = ConversationStream::new_test().with_turn("short"); - let count_before = events.len(); - // Large context window, no overhead => no truncation. - truncate_to_fit(&mut events, 100_000, 0); - assert_eq!(events.len(), count_before); -} - -#[test] -fn truncate_triggers_with_overhead() { - // Build a stream that fits in the raw budget but not after subtracting - // overhead. Each turn adds ~20 chars ("message N" is ~9 chars for - // request + response). - let mut events = ConversationStream::new_test(); - for i in 0..50 { - events = events.with_turn(format!("message {i} with some padding text here")); - } - - let total_chars = estimate_chars(&events); - let count_before = events.len(); - - // Pick a context window where total_chars fits at 90% but not after - // subtracting a large overhead. - #[expect(clippy::cast_possible_truncation)] - let max_tokens = ((total_chars * 100) / (CHARS_PER_TOKEN * OVERHEAD_FACTOR) + 100) as u32; - - // Without overhead, no truncation. - let mut no_overhead = events.clone(); - truncate_to_fit(&mut no_overhead, max_tokens, 0); - assert_eq!(no_overhead.len(), count_before); - - // With overhead eating most of the budget, truncation should happen. - let overhead = token_budget(max_tokens, 0) - 100; - truncate_to_fit(&mut events, max_tokens, overhead); - assert!(events.len() < count_before); -} - #[tokio::test] async fn dedicated_model_backend_returns_answer() { let inquiry_id = tool_call_inquiry_id("call_dedicated", "confirm"); diff --git a/crates/jp_cli/src/error.rs b/crates/jp_cli/src/error.rs index e2dfa150..1b3a607a 100644 --- a/crates/jp_cli/src/error.rs +++ b/crates/jp_cli/src/error.rs @@ -179,4 +179,13 @@ pub(crate) enum Error { /// different model. #[error("Summarization failed for {model}: {reason}")] Summarize { model: String, reason: String }, + + /// Title generation produced no usable title. + /// + /// A dedicated variant so the failure names the model that produced + /// nothing: a refusal or a schema the model couldn't satisfy is a property + /// of the model, not of the conversation, and the fix is to point + /// `conversation.title.generate.model` somewhere else. + #[error("Title generation failed for {model}: {reason}")] + TitleGeneration { model: String, reason: String }, } diff --git a/crates/jp_llm/Cargo.toml b/crates/jp_llm/Cargo.toml index 9004853f..e732b76c 100644 --- a/crates/jp_llm/Cargo.toml +++ b/crates/jp_llm/Cargo.toml @@ -53,6 +53,7 @@ url = { workspace = true } assert_matches = { workspace = true } tokio = { workspace = true, features = ["test-util"] } eventsource-stream = { workspace = true } +http = { workspace = true } infer = { workspace = true, features = ["std"] } insta = { workspace = true, features = ["json"] } jp_test = { workspace = true } diff --git a/crates/jp_llm/src/error.rs b/crates/jp_llm/src/error.rs index 2279407f..f13c9574 100644 --- a/crates/jp_llm/src/error.rs +++ b/crates/jp_llm/src/error.rs @@ -72,6 +72,12 @@ impl StreamError { Self::new(StreamErrorKind::Transient, message) } + /// Create a context-window-exceeded error. + #[must_use] + pub fn context_window_exceeded(message: impl Into) -> Self { + Self::new(StreamErrorKind::ContextWindowExceeded, message) + } + /// Returns the human-readable error message (without the source chain). #[must_use] pub fn message(&self) -> &str { @@ -125,9 +131,16 @@ impl StreamError { impl fmt::Display for StreamError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.message)?; + f.write_str(&self.message)?; + + // Providers commonly build the message out of the source's own + // rendering (`StreamError::other(err.to_string()).with_source(err)`), + // which would otherwise print the same sentence twice. if let Some(ref source) = self.source { - write!(f, ": {source}")?; + let source = source.to_string(); + if !self.message.contains(&source) { + write!(f, ": {source}")?; + } } Ok(()) @@ -219,12 +232,25 @@ impl StreamError { let retry_after = extract_retry_after(&headers); let code = status.as_u16(); + // An oversized prompt is the same size on the next attempt, so + // it is classified before the retry heuristics below. A 429 is + // exempt: it is an authoritative rate-limit signal, and + // token-per-minute limits are phrased close enough to a window + // overflow that the text check would misread them as fatal. + if code != 429 && looks_like_context_window_error(&body) { + return Self::context_window_exceeded(display); + } + // Non-standard `x-should-retry` header overrides // status-code heuristics. let retryable = match header_str(&headers, "x-should-retry") { Some("true") => true, Some("false") => false, - _ => matches!(code, 408 | 409 | 429 | _ if code >= 500), + // Timeout, conflict, rate limit, and any server error. + // Written as two checks rather than one `matches!`: a guard + // attaches to the whole or-pattern, so `408 | 409 | 429 | _ + // if code >= 500` silently reduces to `code >= 500`. + _ => matches!(code, 408 | 409 | 429) || code >= 500, }; if !retryable { @@ -288,6 +314,11 @@ pub enum StreamErrorKind { /// This is not retryable — the user needs to top up or change plans. InsufficientQuota, + /// The request is larger than the model's context window. + /// This is not retryable — the request has to shrink, or move to a model + /// with a larger window. + ContextWindowExceeded, + /// Other errors that are not categorized. /// These may or may not be retryable depending on the specific error. Other, @@ -303,6 +334,7 @@ impl StreamErrorKind { Self::RateLimit => "Rate limited", Self::Transient => "Server error", Self::InsufficientQuota => "Insufficient API quota", + Self::ContextWindowExceeded => "Context window exceeded", Self::Other => "Stream Error", } } @@ -513,6 +545,28 @@ pub(crate) fn looks_like_quota_error(text: &str) -> bool { || lower.contains("resource_exhausted") } +/// Heuristic check for a request exceeding the model's context window, based on +/// error text. +/// +/// Providers report this as a plain invalid-request error, indistinguishable +/// from any other 400 without reading the message: +/// +/// - Anthropic: `"prompt is too long: 531500 tokens > 200000 maximum"` +/// - OpenAI: `"context_length_exceeded"`, `"maximum context length is ..."` +/// - Google: `"The input token count ... exceeds the maximum number of tokens +/// allowed"` +/// - llama.cpp: `"the request exceeds the available context size"` +pub(crate) fn looks_like_context_window_error(text: &str) -> bool { + let lower = text.to_ascii_lowercase(); + lower.contains("context_length_exceeded") + || lower.contains("context length") + || lower.contains("context window") + || lower.contains("context size") + || lower.contains("prompt is too long") + || lower.contains("exceeds the maximum number of tokens") + || lower.contains("too many tokens") +} + /// Heuristic check for transient network/proxy failures based on error text. /// /// Catches upstream failures reported by proxies and gateways between us and diff --git a/crates/jp_llm/src/error_tests.rs b/crates/jp_llm/src/error_tests.rs index 3bb6cb7c..54ac3c6e 100644 --- a/crates/jp_llm/src/error_tests.rs +++ b/crates/jp_llm/src/error_tests.rs @@ -1,5 +1,21 @@ +use std::io; + use super::*; +/// A `reqwest_eventsource::Error::InvalidStatusCode` carrying `status` and +/// `body`, as the provider stream paths receive it. +fn invalid_status(status: u16, body: &str) -> reqwest_eventsource::Error { + let response = http::Response::builder() + .status(status) + .body(body.to_owned()) + .expect("valid response"); + + reqwest_eventsource::Error::InvalidStatusCode( + reqwest::StatusCode::from_u16(status).expect("valid status"), + reqwest::Response::from(response), + ) +} + #[tokio::test] async fn stream_ended_classifies_as_retryable() { // A stream that ends without a terminal event is the disconnect case (e.g. @@ -9,6 +25,66 @@ async fn stream_ended_classifies_as_retryable() { assert!(err.is_retryable()); } +#[tokio::test] +async fn oversized_prompt_body_is_a_context_window_error() { + let err = StreamError::from_eventsource(invalid_status( + 400, + r#"{"error":{"message":"prompt is too long: 531500 tokens > 200000 maximum"}}"#, + )) + .await; + + assert_eq!(err.kind, StreamErrorKind::ContextWindowExceeded); + assert!(!err.is_retryable()); +} + +/// A plain 429 is a rate limit, and its `Retry-After` survives. +#[tokio::test] +async fn a_429_is_a_rate_limit() { + let err = StreamError::from_eventsource(invalid_status(429, "slow down")).await; + + assert_eq!(err.kind, StreamErrorKind::RateLimit); + assert!(err.is_retryable()); +} + +/// A request timeout and a conflict are retryable without being rate limits. +#[tokio::test] +async fn timeout_and_conflict_are_transient() { + for code in [408, 409] { + let err = StreamError::from_eventsource(invalid_status(code, "try again")).await; + + assert_eq!(err.kind, StreamErrorKind::Transient, "HTTP {code}"); + assert!(err.is_retryable(), "HTTP {code}"); + } +} + +/// A 400 carries no retry signal and no recognized phrasing, so it stays fatal. +#[tokio::test] +async fn a_plain_400_is_not_retryable() { + let err = StreamError::from_eventsource(invalid_status(400, "malformed request")).await; + + assert_eq!(err.kind, StreamErrorKind::Other); + assert!(!err.is_retryable()); +} + +/// A 429 whose body reads like a window overflow must stay a retryable rate +/// limit. +/// +/// The status code is an authoritative rate-limit signal; the body heuristic is +/// a fallback for the 4xx responses that carry no such signal. +/// Letting the text win would turn a wait-and-retry into a fatal error and +/// discard the retry timing with it. +#[tokio::test] +async fn a_429_outranks_a_context_window_phrasing_in_the_body() { + let err = StreamError::from_eventsource(invalid_status( + 429, + r#"{"error":{"message":"Rate limit reached: too many tokens per minute."}}"#, + )) + .await; + + assert_eq!(err.kind, StreamErrorKind::RateLimit); + assert!(err.is_retryable()); +} + #[test] fn extract_retry_after_from_retry_after_ms() { let mut headers = reqwest::header::HeaderMap::new(); @@ -286,3 +362,63 @@ fn other_error_without_transient_pattern_is_not_retryable() { let error = StreamError::other("unknown error: something exploded"); assert!(!error.is_retryable()); } + +#[test] +fn context_window_error_detects_provider_messages() { + // The exact shape of the reported Anthropic failure. + assert!(looks_like_context_window_error( + "api error: invalid_request_error: prompt is too long: 531500 tokens > 200000 maximum" + )); + assert!(looks_like_context_window_error("context_length_exceeded")); + assert!(looks_like_context_window_error( + "This model's maximum context length is 8192 tokens. However, your messages resulted in \ + 10000 tokens." + )); + assert!(looks_like_context_window_error( + "The input token count (1200000) exceeds the maximum number of tokens allowed (1048576)." + )); + assert!(looks_like_context_window_error( + "the request exceeds the available context size, try increasing it" + )); +} + +#[test] +fn context_window_error_ignores_unrelated_errors() { + assert!(!looks_like_context_window_error("invalid request")); + assert!(!looks_like_context_window_error( + "api error: rate_limit_error: too many requests" + )); + assert!(!looks_like_context_window_error("")); +} + +#[test] +fn context_window_error_is_not_retryable() { + // Retrying sends the same oversized prompt; the request has to shrink. + let error = StreamError::context_window_exceeded("prompt is too long"); + assert!(!error.is_retryable()); + assert_eq!(error.kind, StreamErrorKind::ContextWindowExceeded); +} + +#[test] +fn display_does_not_repeat_a_source_already_in_the_message() { + // Providers build the message from the source's own rendering; the source + // must not then be appended a second time. + let source = io::Error::other("prompt is too long: 531500 tokens > 200000 maximum"); + let error = StreamError::other(source.to_string()).with_source(source); + + assert_eq!( + error.to_string(), + "prompt is too long: 531500 tokens > 200000 maximum" + ); +} + +#[test] +fn display_appends_a_source_that_adds_information() { + let source = io::Error::other("connection reset by peer"); + let error = StreamError::other("request failed").with_source(source); + + assert_eq!( + error.to_string(), + "request failed: connection reset by peer" + ); +} diff --git a/crates/jp_llm/src/event_builder.rs b/crates/jp_llm/src/event_builder.rs index 5349bd3d..350a2288 100644 --- a/crates/jp_llm/src/event_builder.rs +++ b/crates/jp_llm/src/event_builder.rs @@ -40,7 +40,40 @@ use jp_conversation::{ use serde_json::{Map, Value}; use tracing::warn; -use crate::event::{EventPart, ToolCallPart}; +use crate::event::{Event, EventPart, ToolCallPart}; + +/// Extract the structured JSON payload from a completed list of stream events. +/// +/// Structured output arrives as separate chunks; running the list through an +/// [`EventBuilder`] concatenates and parses them into a single value rather +/// than leaving a sequence of string fragments. +/// +/// Returns `None` when the response carried no structured data. +#[must_use] +pub fn structured_data(events: Vec) -> Option { + let mut builder = EventBuilder::new(); + let mut flushed = Vec::new(); + + for event in events { + match event { + Event::Part { + index, + part, + metadata, + } => builder.handle_part(index, part, metadata), + Event::Flush { index, metadata } => { + flushed.extend(builder.handle_flush(index, metadata)); + } + Event::Finished(_) => flushed.extend(builder.drain()), + Event::Patch(_) | Event::KeepAlive => {} + } + } + + flushed + .into_iter() + .filter_map(ConversationEvent::into_chat_response) + .find_map(ChatResponse::into_structured_data) +} /// Accumulates streamed events into complete [`ConversationEvent`]s. pub struct EventBuilder { diff --git a/crates/jp_llm/src/lib.rs b/crates/jp_llm/src/lib.rs index 92d64a42..45856e02 100644 --- a/crates/jp_llm/src/lib.rs +++ b/crates/jp_llm/src/lib.rs @@ -8,6 +8,7 @@ pub mod retry; mod stream; pub mod title; pub mod tool; +pub mod window; #[cfg(test)] pub(crate) mod test; diff --git a/crates/jp_llm/src/provider/anthropic.rs b/crates/jp_llm/src/provider/anthropic.rs index bf621b92..d2cb0e54 100644 --- a/crates/jp_llm/src/provider/anthropic.rs +++ b/crates/jp_llm/src/provider/anthropic.rs @@ -34,7 +34,7 @@ use super::{Provider, trace_to_tmpfile}; use crate::{ error::{ Error, Result, StreamError, StreamErrorKind, extract_retry_from_text, - looks_like_quota_error, + looks_like_context_window_error, looks_like_quota_error, }, event::{Event, EventMatcher, EventPart, EventPatch, FinishReason, PatchAction, ToolCallPart}, event_builder::EventBuilder, @@ -2160,6 +2160,17 @@ impl From for StreamError { .with_source(error) } + // An oversized prompt is the same size on the next attempt, so it + // is classified rather than left to the generic fallback below. + E::Api(ref api_error) + if api_error + .message + .as_deref() + .is_some_and(looks_like_context_window_error) => + { + StreamError::context_window_exceeded(error.to_string()).with_source(error) + } + error => StreamError::other(error.to_string()).with_source(error), } } diff --git a/crates/jp_llm/src/provider/google.rs b/crates/jp_llm/src/provider/google.rs index b1ef5909..5f16d363 100644 --- a/crates/jp_llm/src/provider/google.rs +++ b/crates/jp_llm/src/provider/google.rs @@ -27,7 +27,7 @@ use tracing::{debug, trace}; use super::{EventStream, Provider, trace_to_tmpfile}; use crate::{ StreamErrorKind, - error::{Error, Result, StreamError, looks_like_quota_error}, + error::{Error, Result, StreamError, looks_like_context_window_error, looks_like_quota_error}, event::{Event, EventMatcher, EventPatch, FinishReason, PatchAction}, model::{ModelDeprecation, ModelDetails, ReasoningDetails}, query::ChatQuery, @@ -1029,6 +1029,15 @@ impl From for StreamError { .or_else(|| value.pointer("/error/code")) .and_then(Value::as_u64); + // An oversized prompt is the same size on the next attempt, so + // it is classified before the status codes below. A 429 is + // exempt: it is an authoritative rate-limit signal, and + // token-per-minute limits are phrased close enough to a window + // overflow that the text check would misread them as fatal. + if status != Some(429) && looks_like_context_window_error(&msg) { + return StreamError::context_window_exceeded(msg).with_source(err); + } + match status { Some(429) => StreamError::rate_limit(None).with_source(err), Some(500 | 502 | 503 | 504) => StreamError::transient(msg).with_source(err), diff --git a/crates/jp_llm/src/provider/google_tests.rs b/crates/jp_llm/src/provider/google_tests.rs index 57c50a6b..fa82ec1b 100644 --- a/crates/jp_llm/src/provider/google_tests.rs +++ b/crates/jp_llm/src/provider/google_tests.rs @@ -627,3 +627,48 @@ mod transform_schema { ); } } + +mod stream_error_classification { + use gemini_client_rs::GeminiError; + use serde_json::json; + + use crate::error::{StreamError, StreamErrorKind}; + + /// A Gemini API error carrying `status` and an error message. + fn api_error(status: u64, message: &str) -> GeminiError { + GeminiError::Api(json!({ + "status": status, + "message": {"error": {"code": status, "message": message}}, + })) + } + + #[test] + fn oversized_input_is_a_context_window_error() { + let error = StreamError::from(api_error( + 400, + "The input token count (1200000) exceeds the maximum number of tokens allowed \ + (1048576).", + )); + + assert_eq!(error.kind, StreamErrorKind::ContextWindowExceeded); + assert!(!error.is_retryable()); + } + + /// A 429 whose message reads like a window overflow must stay a retryable + /// rate limit: the status code is authoritative and the message heuristic + /// is only a fallback. + /// + /// The wording deliberately avoids the quota vocabulary, so the only thing + /// standing between this error and a fatal `ContextWindowExceeded` is the + /// status check. + #[test] + fn a_429_outranks_a_context_window_phrasing() { + let error = StreamError::from(api_error( + 429, + "Rate limit reached: too many tokens per minute for this project. Retry shortly.", + )); + + assert_eq!(error.kind, StreamErrorKind::RateLimit); + assert!(error.is_retryable()); + } +} diff --git a/crates/jp_llm/src/provider/mock.rs b/crates/jp_llm/src/provider/mock.rs index ec699778..2f208564 100644 --- a/crates/jp_llm/src/provider/mock.rs +++ b/crates/jp_llm/src/provider/mock.rs @@ -59,6 +59,9 @@ pub struct MockProvider { /// `None` means every request replays `events`. batches: Option>>>>, + /// Requests received so far, when capture is enabled. + requests: Option>>>, + /// Model details to return. model: ModelDetails, } @@ -74,6 +77,7 @@ impl MockProvider { Self { events, batches: None, + requests: None, model: Self::default_model(), } } @@ -95,6 +99,7 @@ impl MockProvider { Self { events: vec![], batches: Some(Arc::new(Mutex::new(batches.into()))), + requests: None, model: Self::default_model(), } } @@ -158,6 +163,17 @@ impl MockProvider { ]) } + /// Record every request this provider receives. + /// + /// Returns the provider alongside the shared log, so a test can assert on + /// what was actually sent rather than only on what came back. + #[must_use] + pub fn capturing_requests(mut self) -> (Self, Arc>>) { + let requests = Arc::new(Mutex::new(vec![])); + self.requests = Some(Arc::clone(&requests)); + (self, requests) + } + /// Set custom model details for this provider. #[must_use] pub fn with_model(mut self, model: ModelDetails) -> Self { @@ -202,8 +218,12 @@ impl Provider for MockProvider { async fn chat_completion_stream( &self, _model: &ModelDetails, - _query: ChatQuery, + query: ChatQuery, ) -> Result { + if let Some(requests) = &self.requests { + requests.lock().expect("mock requests lock").push(query); + } + let events = match &self.batches { None => self.events.clone(), Some(batches) => batches diff --git a/crates/jp_llm/src/provider/openai.rs b/crates/jp_llm/src/provider/openai.rs index b969e22b..41c4415c 100644 --- a/crates/jp_llm/src/provider/openai.rs +++ b/crates/jp_llm/src/provider/openai.rs @@ -33,7 +33,7 @@ use super::{EventStream, ModelDetails, Provider}; use crate::{ error::{ Error, Result, StreamError, StreamErrorKind, extract_retry_from_text, - looks_like_quota_error, + looks_like_context_window_error, looks_like_quota_error, }, event::{Event, FinishReason}, model::{ModelDeprecation, ReasoningDetails}, @@ -1491,12 +1491,25 @@ fn classify_stream_error(error: types::response::Error) -> StreamError { ); } + // An oversized prompt is the same size on the next attempt, so the typed + // signal is checked before the retryable categories below. + if code == Some("context_length_exceeded") || type_ == "context_length_exceeded" { + return StreamError::context_window_exceeded(error.message); + } + // Rate-limits may signal via either `type` or `code`. OpenAI's TPM/RPM // in-stream limits use `type=tokens|requests` with `code=rate_limit_exceeded`. + // This outranks the message sniff below: a token-per-minute limit reads much + // like a window overflow, and only the typed code tells them apart. if code == Some("rate_limit_exceeded") || type_ == "rate_limit_exceeded" { return StreamError::rate_limit(retry_after); } + // No typed signal said window or rate limit, so fall back to the message. + if looks_like_context_window_error(&error.message) { + return StreamError::context_window_exceeded(error.message); + } + // Server-side transient errors. Match on either type or code: OpenAI emits // generic types (`server_error`, `api_error`) but also more specific overload // signals where the discriminator lives in `code` (e.g. diff --git a/crates/jp_llm/src/provider/openai_tests.rs b/crates/jp_llm/src/provider/openai_tests.rs index 9b98a01a..31ef6a05 100644 --- a/crates/jp_llm/src/provider/openai_tests.rs +++ b/crates/jp_llm/src/provider/openai_tests.rs @@ -1933,4 +1933,51 @@ mod classify_stream_error { assert_eq!(classified.kind, StreamErrorKind::Other); assert_eq!(classified.retry_after, None); } + + #[test] + fn context_window_via_typed_code() { + let e = err( + "invalid_request_error", + Some("context_length_exceeded"), + "the request is too large", + ); + let classified = classify_stream_error(e); + + assert_eq!(classified.kind, StreamErrorKind::ContextWindowExceeded); + assert!(!classified.is_retryable()); + } + + /// With no typed signal the message is all there is, so the text heuristic + /// still catches a window overflow. + #[test] + fn context_window_falls_back_to_the_message() { + let e = err( + "invalid_request_error", + None, + "This model's maximum context length is 8192 tokens.", + ); + let classified = classify_stream_error(e); + + assert_eq!(classified.kind, StreamErrorKind::ContextWindowExceeded); + assert!(!classified.is_retryable()); + } + + /// A token-per-minute limit whose message reads like a window overflow must + /// still be a retryable rate limit. + /// + /// `code` is authoritative and the message heuristic is only a fallback; + /// letting the text win would turn a wait-and-retry into a fatal error. + #[test] + fn typed_rate_limit_outranks_a_context_window_phrasing() { + let e = err( + "tokens", + Some("rate_limit_exceeded"), + "Rate limit reached: too many tokens requested. Please try again in 2.398s.", + ); + let classified = classify_stream_error(e); + + assert_eq!(classified.kind, StreamErrorKind::RateLimit); + assert_eq!(classified.retry_after, Some(Duration::from_secs(3))); + assert!(classified.is_retryable()); + } } diff --git a/crates/jp_llm/src/title.rs b/crates/jp_llm/src/title.rs index 6eced3f8..af13c714 100644 --- a/crates/jp_llm/src/title.rs +++ b/crates/jp_llm/src/title.rs @@ -1,12 +1,213 @@ -//! Shared title generation helpers. +//! Conversation title generation. //! -//! Provides the JSON schema and instructions used by background callers (e.g. -//! `TitleGeneratorTask`, `conversation edit --title`) to request conversation -//! titles from an LLM via structured output. +//! [`generate`] is the entry point: it shapes a conversation into a structured +//! output request and returns the candidate titles the model produced. +//! [`resolve_model`] picks the model that request runs on. +//! +//! The schema and instruction helpers are public so callers can measure or +//! inspect the request they are about to make. + +use std::sync::Arc; -use jp_config::assistant::{instructions::InstructionsConfig, sections::SectionConfig}; +use jp_config::{ + AppConfig, + assistant::{ + instructions::InstructionsConfig, sections::SectionConfig, tool_choice::ToolChoice, + }, + model::{ + ModelConfig, + id::ModelIdOrAliasConfig, + parameters::{CustomReasoningConfig, ParametersConfig, ReasoningEffort}, + }, +}; +use jp_conversation::{ConversationStream, event::ChatRequest, thread::ThreadBuilder}; use serde_json::{Map, Value, json}; +use crate::{ + Provider, + error::Result, + event_builder, + model::ModelDetails, + query::ChatQuery, + retry::{RetryConfig, collect_with_retry}, + window, +}; + +/// A request for LLM-generated conversation titles. +#[derive(Debug)] +pub struct TitleRequest { + /// The conversation to generate titles for. + /// + /// Taken by value: the request appends its own turn and may drop older + /// events to fit the model's context window, so callers pass a clone of the + /// stream they want to keep. + pub events: ConversationStream, + + /// The model the request runs on, with the parameters it runs under. + /// + /// These parameters are the only ones the request carries: reasoning + /// effort, max tokens, temperature and provider-specific values are taken + /// from here and never inherited from the conversation. + /// See [`resolve_model`]. + pub model: ModelConfig, + + /// How many candidate titles to ask for. + pub count: usize, + + /// Titles the user already rejected, which the model must avoid. + pub rejected: Vec, +} + +/// Resolve the model that conversation-title generation runs on. +/// +/// `override_id` wins when set; otherwise `conversation.title.generate.model` +/// is used, falling back to the assistant model's ID. +/// Parameters come from `conversation.title.generate.model` when it is +/// configured and start fresh otherwise, so a title request never inherits the +/// conversation's own parameters. +/// +/// Reasoning defaults to low effort with the trace excluded: a title is a short +/// factual summary, and deep reasoning on every new conversation rarely earns +/// its cost. +/// An explicit reasoning setting on the title model is preserved. +#[must_use] +pub fn resolve_model( + config: &AppConfig, + override_id: Option<&ModelIdOrAliasConfig>, +) -> ModelConfig { + let mut model = config + .conversation + .title + .generate + .model + .clone() + .unwrap_or_else(|| ModelConfig { + id: config.assistant.model.id.clone(), + parameters: ParametersConfig::default(), + }); + + if let Some(id) = override_id { + model.id = id.clone(); + } + + if model.parameters.reasoning.is_none() { + model.parameters.reasoning = Some( + CustomReasoningConfig { + effort: ReasoningEffort::Low, + exclude: true, + } + .into(), + ); + } + + model +} + +/// Generate candidate titles for a conversation. +/// +/// `details` describes the model named by `request.model`; its context window +/// bounds the request, with older events dropped when the conversation doesn't +/// fit (see [`window::truncate_to_fit`]). +/// +/// Returns the titles in the order the model produced them, or an empty vec +/// when the response carried no structured data — callers decide whether that +/// is an error. +/// +/// # Errors +/// +/// Returns an error if the thread cannot be built or the provider request fails +/// after exhausting its retries. +pub async fn generate( + provider: &dyn Provider, + details: &ModelDetails, + request: TitleRequest, +) -> Result> { + let TitleRequest { + mut events, + model, + count, + rejected, + } = request; + + let sections = title_instructions(count, &rejected); + + // Resolve compaction overlays up front: `rebase_on_model` carries + // conversation events only, so a stored summary has to already be part of + // the event list by the time it runs. + events.apply_projection(); + + // The request carries no system prompt and no attachments, so the + // instruction sections are the only fixed overhead sharing the window. + if let Some(context_window) = details.context_window { + let overhead = window::estimate_overhead_chars(None, §ions, &[], &[]); + window::truncate_to_fit(&mut events, context_window, overhead); + } + + let mut thread = ThreadBuilder::default() + .with_events(rebase_on_model(&events, model)?) + .with_sections(sections) + .build()?; + + thread.events.start_turn(ChatRequest { + content: if count == 1 { + "Generate a title for this conversation.".into() + } else { + "Generate titles for this conversation.".into() + }, + schema: Some(title_schema(count)), + author: None, + }); + + let query = ChatQuery { + thread, + tools: vec![], + tool_choice: ToolChoice::default(), + }; + + let events = collect_with_retry(provider, details, query, &RetryConfig::default()).await?; + + Ok(event_builder::structured_data(events) + .as_ref() + .map(extract_titles) + .unwrap_or_default()) +} + +/// Rebuild `events` so `model` is the only assistant model the request sees. +/// +/// A config delta cannot express "unset": merging one carries every parameter +/// the conversation had set but `model` leaves unset, so an assistant +/// `max_tokens` outlives the switch to a smaller title model and is sent to it. +/// Collapsing the conversation's effective config into the base and replacing +/// `assistant.model` outright is the only way to drop those parameters. +/// +/// Everything the conversation configured other than the model is preserved: +/// the new base is the *merged* config, so settings the provider reads +/// alongside the model (`assistant.request.cache`, for one) keep the values the +/// conversation arrived at rather than reverting to its creation-time defaults. +/// +/// `created_at` carries over too. +/// Providers derive a per-conversation prompt cache identity from it, so a +/// fresh timestamp would cost a cache miss on every request and orphan this one +/// from the conversation whose prefix it shares. +/// +/// Only conversation events are carried over, so `events` must already be +/// projected (see [`ConversationStream::apply_projection`]) or its compaction +/// overlays are lost. +/// +/// # Errors +/// +/// Returns an error if the conversation's own config deltas do not merge into a +/// valid config. +fn rebase_on_model(events: &ConversationStream, model: ModelConfig) -> Result { + let mut base = events.config()?; + base.assistant.model = model; + + let mut rebased = ConversationStream::new(Arc::new(base)).with_created_at(events.created_at); + rebased.extend(events.iter().map(|e| e.event.clone())); + + Ok(rebased) +} + /// JSON schema for the title generation structured output. /// /// Returns a schema requiring an object with a `titles` array of exactly diff --git a/crates/jp_llm/src/title_tests.rs b/crates/jp_llm/src/title_tests.rs index 034709d1..88ae94f3 100644 --- a/crates/jp_llm/src/title_tests.rs +++ b/crates/jp_llm/src/title_tests.rs @@ -1,4 +1,367 @@ +use std::sync::{Arc, Mutex}; + +use chrono::{DateTime, Utc}; +use jp_config::{ + PartialAppConfig, + assistant::request::CachePolicy, + model::{ + id::{ModelIdConfig, Name, ProviderId}, + parameters::ReasoningConfig, + }, +}; +use jp_conversation::{Compaction, EventKind, SummaryPolicy, event::ChatResponse}; + use super::*; +use crate::{ + event::{Event, FinishReason}, + provider::mock::MockProvider, +}; + +fn model_id(name: &str) -> ModelIdConfig { + ModelIdConfig { + provider: ProviderId::Test, + name: name.parse().expect("valid model name"), + } +} + +fn model_config(name: &str) -> ModelConfig { + ModelConfig { + id: ModelIdOrAliasConfig::Id(model_id(name)), + parameters: ParametersConfig::default(), + } +} + +/// A provider that answers with one title, plus the log of what it received. +fn title_provider(context_window: Option) -> (MockProvider, Arc>>) { + MockProvider::new(vec![ + Event::structured(0, json!({"titles": ["A Title"]}).to_string()), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ]) + .with_model(ModelDetails { + context_window, + ..ModelDetails::empty(model_id("mock")) + }) + .capturing_requests() +} + +async fn details(provider: &MockProvider) -> ModelDetails { + provider + .model_details(&"mock".parse::().unwrap()) + .await + .unwrap() +} + +/// A conversation of `turns` turns, each roughly 1000 chars. +fn long_conversation(turns: usize) -> ConversationStream { + let mut events = ConversationStream::new_test(); + for i in 0..turns { + events = events.with_turn(format!("turn {i}: {}", "x".repeat(1000))); + } + events +} + +/// The reported failure: a conversation grown on a large-window model, titled +/// by a model with a small one. +/// +/// The assertion is on the size of what was sent rather than on a turn count: +/// fitting shrinks to a target fraction of the window, and which turn the +/// cutoff lands on shifts with the size of the instruction sections. +/// Without fitting this request carries ~200k chars into a 3k char window. +#[tokio::test] +async fn generate_shrinks_a_conversation_past_the_window() { + const WINDOW: u32 = 1000; + + let (provider, requests) = title_provider(Some(WINDOW)); + let details = details(&provider).await; + let conversation = long_conversation(200); + let original_chars = window::estimate_chars(&conversation); + + let titles = title_generate(&provider, &details, conversation).await; + assert_eq!(titles, ["A Title"]); + + let sent = requests.lock().unwrap(); + assert_eq!(sent.len(), 1); + + let sent_chars = window::estimate_chars(&sent[0].thread.events); + let window_chars = WINDOW as usize * window::CHARS_PER_TOKEN; + assert!( + original_chars > window_chars * 50, + "fixture must be far past the window, was {original_chars} chars" + ); + assert!( + sent_chars < window_chars, + "sent {sent_chars} chars into a {window_chars} char window" + ); +} + +/// A conversation that accumulated `max_tokens` must not impose it on a title +/// model that leaves the parameter unset. +/// +/// A config delta cannot express "unset", so overriding the model through one +/// would carry the conversation's 64000 into a request the title model may not +/// accept. +/// The assertion is on the effective config of what was sent, not on the +/// `ModelConfig` handed in. +#[tokio::test] +async fn generate_does_not_inherit_conversation_parameters() { + let (provider, requests) = title_provider(Some(1_000_000)); + let details = details(&provider).await; + + let mut events = long_conversation(2); + let mut delta = PartialAppConfig::empty(); + delta.assistant.model.parameters.max_tokens = Some(64_000); + delta.assistant.model.parameters.temperature = Some(1.5); + events.add_config_delta(delta); + + title_generate(&provider, &details, events).await; + + let sent = requests.lock().unwrap(); + let parameters = sent[0] + .thread + .events + .config() + .expect("merged config") + .assistant + .model + .parameters; + + assert_eq!(parameters.max_tokens, None); + assert_eq!(parameters.temperature, None); +} + +/// Replacing the model must not revert everything else the conversation +/// configured. +/// +/// Providers read more than the model off this config: both Anthropic and +/// OpenAI read `assistant.request.cache`, so a conversation that turned caching +/// off would silently start writing to the provider cache again. +#[tokio::test] +async fn generate_keeps_non_model_settings_from_the_conversation() { + let (provider, requests) = title_provider(Some(1_000_000)); + let details = details(&provider).await; + + let mut events = long_conversation(2); + let mut delta = PartialAppConfig::empty(); + delta.assistant.request.cache = Some(CachePolicy::Off); + events.add_config_delta(delta); + + title_generate(&provider, &details, events).await; + + let sent = requests.lock().unwrap(); + let config = sent[0].thread.events.config().expect("merged config"); + assert_eq!(config.assistant.request.cache, CachePolicy::Off); +} + +/// The same guarantee on the path where fitting empties the stream. +/// +/// A single oversized turn against a small-window title model leaves no chat +/// request, and `truncate_to_fit` then drops every conversation event. +/// The request is still built from the stream's effective config, so the delta +/// has to survive that emptying. +#[tokio::test] +async fn generate_keeps_non_model_settings_when_fitting_empties_the_stream() { + let (provider, requests) = title_provider(Some(1000)); + let details = details(&provider).await; + + let mut events = ConversationStream::new_test().with_turn("q".repeat(3000)); + let mut delta = PartialAppConfig::empty(); + delta.assistant.request.cache = Some(CachePolicy::Off); + events.add_config_delta(delta); + + title_generate(&provider, &details, events).await; + + let sent = requests.lock().unwrap(); + let config = sent[0].thread.events.config().expect("merged config"); + assert_eq!(config.assistant.request.cache, CachePolicy::Off); + + // The turn really was dropped, so this is the emptying path and not a + // request that happened to fit. + assert_eq!(sent[0].thread.events.turn_count(), 1); +} + +/// The prompt cache identity providers derive from `created_at` survives the +/// rebase. +/// +/// A fresh timestamp would miss the cache on every request, including each +/// `More...` batch re-sending the same conversation prefix. +#[tokio::test] +async fn generate_preserves_the_conversation_cache_identity() { + let (provider, requests) = title_provider(Some(1_000_000)); + let details = details(&provider).await; + + let created_at = "2026-01-02T03:04:05Z" + .parse::>() + .expect("valid timestamp"); + let events = long_conversation(2).with_created_at(created_at); + + title_generate(&provider, &details, events).await; + + let sent = requests.lock().unwrap(); + assert_eq!(sent[0].thread.events.created_at, created_at); +} + +/// The title model's own parameters do reach the request. +#[tokio::test] +async fn generate_applies_the_title_model_parameters() { + let (provider, requests) = title_provider(Some(1_000_000)); + let details = details(&provider).await; + + let mut model = model_config("mock"); + model.parameters.max_tokens = Some(256); + + generate(&provider, &details, TitleRequest { + events: long_conversation(2), + model, + count: 1, + rejected: vec![], + }) + .await + .expect("title generation succeeds"); + + let sent = requests.lock().unwrap(); + let config = sent[0].thread.events.config().expect("merged config"); + assert_eq!(config.assistant.model.parameters.max_tokens, Some(256)); + assert_eq!(config.assistant.model.id.resolved(), &model_id("mock")); +} + +#[tokio::test] +async fn generate_keeps_a_conversation_that_fits() { + let (provider, requests) = title_provider(Some(1_000_000)); + let details = details(&provider).await; + + title_generate(&provider, &details, long_conversation(3)).await; + + // The three original turns plus the appended title request. + let sent = requests.lock().unwrap(); + assert_eq!(sent[0].thread.events.turn_count(), 4); +} + +/// A stored summary reaches the provider even when no window forces fitting. +/// +/// The rebuild that replaces the assistant model carries conversation events +/// only, so the overlay has to be resolved before it runs or the summary is +/// silently swapped back for the raw turns it covers. +#[tokio::test] +async fn generate_keeps_a_summary_when_the_window_is_unknown() { + let (provider, requests) = title_provider(None); + let details = details(&provider).await; + + let mut events = long_conversation(20); + events.add_compaction(Compaction::new(0, 18).with_summary(SummaryPolicy { + summary: "A short summary of the first 19 turns.".to_owned(), + })); + + title_generate(&provider, &details, events).await; + + let sent = requests.lock().unwrap(); + let messages: Vec = sent[0] + .thread + .events + .iter() + .filter_map(|e| match &e.event.kind { + EventKind::ChatResponse(ChatResponse::Message { message }) => Some(message.clone()), + _ => None, + }) + .collect(); + + assert!( + messages + .iter() + .any(|m| m == "A short summary of the first 19 turns."), + "the summary must reach the provider, got {messages:?}" + ); +} + +/// Providers that don't report a window (local llama.cpp, Ollama) leave the +/// conversation untouched: there is no budget to measure against. +#[tokio::test] +async fn generate_keeps_everything_when_the_window_is_unknown() { + let (provider, requests) = title_provider(None); + let details = details(&provider).await; + + title_generate(&provider, &details, long_conversation(200)).await; + + let sent = requests.lock().unwrap(); + assert_eq!(sent[0].thread.events.turn_count(), 201); +} + +async fn title_generate( + provider: &MockProvider, + details: &ModelDetails, + events: ConversationStream, +) -> Vec { + generate(provider, details, TitleRequest { + events, + model: model_config("mock"), + count: 1, + rejected: vec![], + }) + .await + .expect("title generation succeeds") +} + +#[test] +fn resolve_model_falls_back_to_the_assistant_model() { + let mut config = AppConfig::new_test(); + config.assistant.model.id = ModelIdOrAliasConfig::Id(model_id("big-model")); + config.conversation.title.generate.model = None; + + let model = resolve_model(&config, None); + assert_eq!(model.id.resolved(), &model_id("big-model")); +} + +#[test] +fn resolve_model_prefers_the_configured_title_model() { + let mut config = AppConfig::new_test(); + config.assistant.model.id = ModelIdOrAliasConfig::Id(model_id("big-model")); + config.conversation.title.generate.model = Some(model_config("cheap-model")); + + let model = resolve_model(&config, None); + assert_eq!(model.id.resolved(), &model_id("cheap-model")); +} + +#[test] +fn resolve_model_override_outranks_the_configured_title_model() { + let mut config = AppConfig::new_test(); + config.conversation.title.generate.model = Some(model_config("cheap-model")); + + let override_id = ModelIdOrAliasConfig::Id(model_id("chosen-model")); + let model = resolve_model(&config, Some(&override_id)); + assert_eq!(model.id.resolved(), &model_id("chosen-model")); +} + +/// A title is a short factual summary; without an explicit setting it must not +/// inherit the conversation's reasoning budget. +#[test] +fn resolve_model_defaults_reasoning_to_low_effort() { + let mut config = AppConfig::new_test(); + config.assistant.model.parameters.reasoning = + Some(ReasoningConfig::Custom(CustomReasoningConfig { + effort: ReasoningEffort::Max, + exclude: false, + })); + + let model = resolve_model(&config, None); + assert_eq!( + model.parameters.reasoning, + Some(ReasoningConfig::Custom(CustomReasoningConfig { + effort: ReasoningEffort::Low, + exclude: true, + })) + ); +} + +#[test] +fn resolve_model_keeps_an_explicit_reasoning_setting() { + let mut config = AppConfig::new_test(); + let mut title_model = model_config("cheap-model"); + title_model.parameters.reasoning = Some(ReasoningConfig::Off); + config.conversation.title.generate.model = Some(title_model); + + let model = resolve_model(&config, None); + assert_eq!(model.parameters.reasoning, Some(ReasoningConfig::Off)); +} #[test] fn title_schema_has_correct_structure() { diff --git a/crates/jp_llm/src/window.rs b/crates/jp_llm/src/window.rs new file mode 100644 index 00000000..18eb5538 --- /dev/null +++ b/crates/jp_llm/src/window.rs @@ -0,0 +1,228 @@ +//! Fitting a conversation into a model's context window. +//! +//! [`truncate_to_fit`] is the entry point: it applies compaction projection, +//! then drops the oldest events from a stream until a char-based estimate of +//! its size fits the budget derived from the model's context window. +//! Callers measure the fixed overhead sharing that window (system prompt, +//! instruction sections, attachments, tool definitions) with +//! [`estimate_overhead_chars`] and pass it in. +//! +//! Estimation is deliberately crude: a fixed chars-per-token ratio with a +//! safety margin absorbing the error. +//! Everything here is pure — no provider calls, no tokenizer. + +use jp_attachment::Attachment; +use jp_config::assistant::sections::SectionConfig; +use jp_conversation::{ConversationEvent, ConversationStream, EventKind, event::ChatResponse}; +use tracing::info; + +use crate::tool::ToolDefinition; + +/// Estimated chars-per-token ratio used for estimation. +pub const CHARS_PER_TOKEN: usize = 3; + +/// Safety margin for tokenization imprecision (the chars-per-token ratio varies +/// by content type) and provider framing overhead (JSON wrapping, role tags, +/// structured output injection, etc.). +/// +/// System prompt, sections, attachments, and tool definitions are measured +/// explicitly via [`estimate_overhead_chars`], so this factor only needs to +/// cover the remaining approximation error. +const OVERHEAD_FACTOR: usize = 90; // percent + +/// When truncation is needed, target this fraction of the context window. +/// Leaves headroom so a later request against the same window doesn't +/// re-truncate at a different cutoff (which would bust the prompt cache). +const TARGET_FACTOR: usize = 80; // percent + +/// Char-based estimate of the fixed overhead that shares a model's context +/// window with conversation events. +/// +/// Binary attachments consume tokens too (base64 framing, image tiles) but +/// can't be measured in chars; the module's safety margin absorbs them. +#[must_use] +pub fn estimate_overhead_chars( + system_prompt: Option<&str>, + sections: &[SectionConfig], + attachments: &[Attachment], + tools: &[ToolDefinition], +) -> usize { + let mut chars = 0; + + if let Some(prompt) = system_prompt { + chars += prompt.len(); + } + + for section in sections { + chars += section.render().len(); + } + + for attachment in attachments { + if let Some(text) = attachment.as_text() { + chars += text.len(); + } + } + + for tool in tools { + chars += tool.name.len(); + if let Some(desc) = tool.docs.schema_description() { + chars += desc.len(); + } + // Parameter schemas are serialized as JSON by providers. + chars += serde_json::to_string(&tool.to_parameters_schema()).map_or(0, |s| s.len()); + } + + chars +} + +/// Drop the oldest events until the stream fits the model's context window. +/// +/// `overhead_chars` is the measured size of everything else sharing the window +/// (see [`estimate_overhead_chars`]) and is subtracted from the budget. +/// Returns the number of events dropped, which is zero when the stream already +/// fits. +/// +/// The stream is left projected: compaction overlays are resolved into the +/// events they imply (see [`ConversationStream::apply_projection`]), which is +/// what the provider receives either way. +/// +/// Events are dropped from the start, which keeps the cutoff stable across +/// repeated calls on a growing stream: streams are append-only, so the same K +/// oldest events are dropped no matter how many new events arrive at the end, +/// and a provider's prompt cache prefix survives. +/// +/// Structural invariants are restored afterwards via +/// [`ConversationStream::sanitize`] (orphaned tool calls, leading non-user +/// events). +/// If truncation removes every chat request, every remaining conversation event +/// is dropped too: what is left cannot form a valid provider message sequence, +/// since providers require the first message to come from the user. +/// Callers append their own request after fitting, so an emptied stream is +/// still valid input. +/// Config deltas survive that emptying — they are global, provider-invisible, +/// and callers read the stream's effective config to build the request. +pub fn truncate_to_fit( + events: &mut ConversationStream, + context_window: u32, + overhead_chars: usize, +) -> usize { + // Measure the events the provider will actually receive. A stored summary + // can stand in for a long range, so the raw stream both over-estimates the + // request and is the wrong thing to cut: dropping a prefix invalidates every + // compaction overlay it touches (see `ConversationStream::retain`), which + // would hand the provider the raw tail a summary was covering. + events.apply_projection(); + + let budget = budget_chars(context_window, overhead_chars); + let total_chars = estimate_chars(events); + + if total_chars <= budget { + return 0; + } + + let target = target_chars(context_window, overhead_chars); + + // Round must_drop up to the nearest 10% of target so that small + // additions at the end of the stream don't shift the cutoff point. This + // keeps the prefix stable across repeated calls against the same window, + // preserving prompt cache hits on the conversation messages. + let granularity = target / 10; + let raw_drop = total_chars.saturating_sub(target); + let must_drop = match granularity { + 0 => raw_drop, + g => raw_drop.div_ceil(g) * g, + }; + + // Walk from the start (oldest), accumulating chars to drop. + let char_counts: Vec = events + .iter() + .map(|e| estimate_event_chars(e.event)) + .collect(); + + let mut dropped_chars = 0; + let mut dropped_events = 0; + + for count in &char_counts { + if dropped_chars >= must_drop { + break; + } + dropped_chars += count; + dropped_events += 1; + } + + let mut idx = 0; + events.retain(|_| { + let keep = idx >= dropped_events; + idx += 1; + keep + }); + + events.sanitize(); + + if !events.has_chat_request() { + // Drop the conversation events, keep the configuration. `retain` + // preserves global entries, so the config deltas the request is built + // from outlive the emptying; `clear` would take them with it. + events.retain(|_| false); + } + + info!( + context_window, + dropped_events, "Truncated conversation to fit the model's context window.", + ); + + dropped_events +} + +/// Char-based estimate of the conversation events in a stream. +/// +/// Counts provider-visible payloads only; turn markers, config deltas and +/// compaction overlays contribute nothing. +/// The stream is measured as given: to size what a provider would receive, +/// project it first (see [`ConversationStream::apply_projection`]). +#[must_use] +pub fn estimate_chars(events: &ConversationStream) -> usize { + events.iter().map(|e| estimate_event_chars(e.event)).sum() +} + +/// Char-based estimate of a single event's contribution to the window. +/// +/// Events with no provider-visible payload count as zero. +fn estimate_event_chars(event: &ConversationEvent) -> usize { + match &event.kind { + EventKind::ChatRequest(r) => r.content.len(), + EventKind::ChatResponse(ChatResponse::Message { message }) => message.len(), + EventKind::ChatResponse(ChatResponse::Reasoning { reasoning }) => reasoning.len(), + EventKind::ChatResponse(ChatResponse::Structured { data }) => data.to_string().len(), + EventKind::ToolCallRequest(r) => { + r.name.len() + serde_json::to_string(&r.arguments).map_or(0, |s| s.len()) + } + EventKind::ToolCallResponse(r) => { + r.result.as_ref().map_or(0, String::len) + + r.result.as_ref().err().map_or(0, String::len) + } + _ => 0, + } +} + +/// Chars available to conversation events before truncation kicks in. +/// +/// This is the model's window converted to chars, discounted by the safety +/// margin and reduced by the caller's measured overhead. +/// Saturates at zero when the overhead alone fills the window. +#[must_use] +pub fn budget_chars(context_window: u32, overhead_chars: usize) -> usize { + let total = (context_window as usize) * CHARS_PER_TOKEN * OVERHEAD_FACTOR / 100; + total.saturating_sub(overhead_chars) +} + +/// Chars a truncated stream is shrunk down to, leaving headroom below the +/// budget. +fn target_chars(context_window: u32, overhead_chars: usize) -> usize { + let total = (context_window as usize) * CHARS_PER_TOKEN * TARGET_FACTOR / 100; + total.saturating_sub(overhead_chars) +} + +#[cfg(test)] +#[path = "window_tests.rs"] +mod tests; diff --git a/crates/jp_llm/src/window_tests.rs b/crates/jp_llm/src/window_tests.rs new file mode 100644 index 00000000..b4dc34bf --- /dev/null +++ b/crates/jp_llm/src/window_tests.rs @@ -0,0 +1,239 @@ +use indexmap::IndexMap; +use jp_config::{PartialAppConfig, assistant::request::CachePolicy}; +use jp_conversation::{Compaction, ConversationStream, SummaryPolicy, event::ChatResponse}; + +use super::*; +use crate::tool::ToolDocs; + +fn tool(name: &str, summary: Option<&str>) -> ToolDefinition { + ToolDefinition { + name: name.to_owned(), + docs: ToolDocs { + summary: summary.map(str::to_owned), + ..Default::default() + }, + parameters: IndexMap::new(), + } +} + +#[test] +fn overhead_empty_inputs() { + assert_eq!(estimate_overhead_chars(None, &[], &[], &[]), 0); +} + +#[test] +fn overhead_system_prompt() { + let prompt = "You are a helpful assistant."; + let result = estimate_overhead_chars(Some(prompt), &[], &[], &[]); + assert_eq!(result, prompt.len()); +} + +#[test] +fn overhead_sections() { + let section = SectionConfig::default() + .with_tag("instruction") + .with_title("Testing") + .with_content("Do the thing."); + let rendered_len = section.render().len(); + + let result = estimate_overhead_chars(None, &[section], &[], &[]); + assert_eq!(result, rendered_len); +} + +#[test] +fn overhead_text_attachments() { + let attachment = Attachment::text("file.rs", "fn main() {}"); + let result = estimate_overhead_chars(None, &[], &[attachment], &[]); + assert_eq!(result, "fn main() {}".len()); +} + +#[test] +fn overhead_binary_attachments_ignored() { + let attachment = Attachment::binary("img.png", vec![0u8; 1000], "image/png"); + let result = estimate_overhead_chars(None, &[], &[attachment], &[]); + assert_eq!(result, 0); +} + +#[test] +fn overhead_tool_definitions() { + let result = + estimate_overhead_chars(None, &[], &[], &[tool("grep_files", Some("Search files."))]); + // name + description + serialized schema + assert!(result > "grep_files".len() + "Search files.".len()); +} + +#[test] +fn overhead_combines_all_sources() { + let prompt = "Be helpful."; + let section = SectionConfig::default().with_content("Rule 1."); + let attachment = Attachment::text("f.txt", "hello world"); + let tool = tool("t", None); + + let combined = estimate_overhead_chars( + Some(prompt), + std::slice::from_ref(§ion), + std::slice::from_ref(&attachment), + std::slice::from_ref(&tool), + ); + + let sum = estimate_overhead_chars(Some(prompt), &[], &[], &[]) + + estimate_overhead_chars(None, std::slice::from_ref(§ion), &[], &[]) + + estimate_overhead_chars(None, &[], std::slice::from_ref(&attachment), &[]) + + estimate_overhead_chars(None, &[], &[], std::slice::from_ref(&tool)); + + assert_eq!(combined, sum); +} + +#[test] +fn budget_subtracts_overhead() { + let no_overhead = budget_chars(1000, 0); + let with_overhead = budget_chars(1000, 500); + assert_eq!(no_overhead - 500, with_overhead); +} + +#[test] +fn budget_saturates_at_zero() { + // Overhead larger than total budget shouldn't underflow. + assert_eq!(budget_chars(100, 999_999), 0); +} + +#[test] +fn target_subtracts_overhead() { + let no_overhead = target_chars(1000, 0); + let with_overhead = target_chars(1000, 500); + assert_eq!(no_overhead - 500, with_overhead); +} + +#[test] +fn truncate_no_op_when_within_budget() { + let mut events = ConversationStream::new_test().with_turn("short"); + let count_before = events.len(); + // Large context window, no overhead => no truncation. + assert_eq!(truncate_to_fit(&mut events, 100_000, 0), 0); + assert_eq!(events.len(), count_before); +} + +#[test] +fn truncate_triggers_with_overhead() { + // Build a stream that fits in the raw budget but not after subtracting + // overhead. + let mut events = ConversationStream::new_test(); + for i in 0..50 { + events = events.with_turn(format!("message {i} with some padding text here")); + } + + let total_chars = estimate_chars(&events); + let count_before = events.len(); + + // Pick a context window where total_chars fits at 90% but not after + // subtracting a large overhead. + #[expect(clippy::cast_possible_truncation)] + let context_window = ((total_chars * 100) / (CHARS_PER_TOKEN * OVERHEAD_FACTOR) + 100) as u32; + + // Without overhead, no truncation. + let mut no_overhead = events.clone(); + assert_eq!(truncate_to_fit(&mut no_overhead, context_window, 0), 0); + assert_eq!(no_overhead.len(), count_before); + + // With overhead eating most of the budget, truncation should happen. + let overhead = budget_chars(context_window, 0) - 100; + let dropped = truncate_to_fit(&mut events, context_window, overhead); + assert!(dropped > 0); + assert!(events.len() < count_before); +} + +/// A conversation whose bulk is already covered by a stored summary fits a +/// small window once projected, so nothing is dropped. +/// +/// Measuring the raw events instead would both over-estimate the request and +/// destroy the overlay: dropping a prefix invalidates every compaction it +/// touches, handing the provider the raw tail the summary was standing in for. +#[test] +fn truncate_measures_the_projected_stream() { + let mut events = ConversationStream::new_test(); + for i in 0..50 { + events = events.with_turn(format!("turn {i}: {}", "x".repeat(1000))); + } + events.add_compaction(Compaction::new(0, 48).with_summary(SummaryPolicy { + summary: "A short summary of the first 49 turns.".to_owned(), + })); + + // ~50k chars raw, but the projection is the summary pair plus the last turn. + let raw_chars = estimate_chars(&events); + assert!( + raw_chars > budget_chars(10_000, 0), + "fixture must not fit raw" + ); + + assert_eq!(truncate_to_fit(&mut events, 10_000, 0), 0); + assert!( + message_texts(&events) + .iter() + .any(|t| t == "A short summary of the first 49 turns."), + "the summary must survive into the fitted stream" + ); +} + +/// Assistant message text in the stream, in order. +fn message_texts(events: &ConversationStream) -> Vec { + events + .iter() + .filter_map(|e| match &e.event.kind { + EventKind::ChatResponse(ChatResponse::Message { message }) => Some(message.clone()), + _ => None, + }) + .collect() +} + +/// A cutoff that drops every chat request but leaves an assistant response +/// behind cannot form a valid provider message sequence, so every conversation +/// event is dropped. +/// +/// The sizes are picked so the drop loop stops right after the request: a +/// 3000-char request against a 1000-token window needs 720 chars dropped, which +/// the request alone satisfies, leaving the 100-char response as the only +/// survivor. +#[test] +fn truncate_empties_stream_when_no_chat_request_survives() { + let mut events = ConversationStream::new_test().with_turn("q".repeat(3000)); + events + .current_turn_mut() + .add_chat_response(ChatResponse::message("a".repeat(100))) + .build() + .unwrap(); + + assert_eq!(truncate_to_fit(&mut events, 1000, 0), 2); + assert_eq!(events.len(), 0); +} + +/// Emptying the stream must not take the conversation's configuration with it. +/// +/// Callers build the provider request from the stream's effective config, so a +/// delta lost here silently reverts settings the conversation had chosen +/// (`assistant.request.cache`, for one) to their creation-time defaults. +#[test] +fn truncate_keeps_config_deltas_when_the_stream_is_emptied() { + let mut events = ConversationStream::new_test().with_turn("q".repeat(3000)); + events + .current_turn_mut() + .add_chat_response(ChatResponse::message("a".repeat(100))) + .build() + .unwrap(); + + let mut delta = PartialAppConfig::empty(); + delta.assistant.request.cache = Some(CachePolicy::Off); + events.add_config_delta(delta); + + assert_eq!(truncate_to_fit(&mut events, 1000, 0), 2); + assert_eq!(events.len(), 0, "conversation events must be gone"); + assert_eq!( + events + .config() + .expect("merged config") + .assistant + .request + .cache, + CachePolicy::Off, + "the config delta must outlive the emptied stream" + ); +} diff --git a/crates/jp_task/src/task/title_generator.rs b/crates/jp_task/src/task/title_generator.rs index 708261f0..cac3a386 100644 --- a/crates/jp_task/src/task/title_generator.rs +++ b/crates/jp_task/src/task/title_generator.rs @@ -1,26 +1,11 @@ use std::error::Error; use async_trait::async_trait; -use jp_config::{ - AppConfig, - model::{ - ModelConfig, - id::ModelIdConfig, - parameters::{CustomReasoningConfig, ParametersConfig, ReasoningEffort}, - }, - providers::llm::LlmProviderConfig, -}; -use jp_conversation::{ - ConversationEvent, ConversationId, ConversationStream, - event::{ChatRequest, ChatResponse}, - thread::ThreadBuilder, -}; +use jp_config::{AppConfig, model::ModelConfig, providers::llm::LlmProviderConfig}; +use jp_conversation::{ConversationId, ConversationStream}; use jp_llm::{ - event::Event, - event_builder::EventBuilder, provider, - retry::{RetryConfig, collect_with_retry}, - title, + title::{self, TitleRequest}, }; use jp_workspace::Workspace; use tokio_util::sync::CancellationToken; @@ -31,7 +16,7 @@ use crate::Task; #[derive(Debug)] pub struct TitleGeneratorTask { pub conversation_id: ConversationId, - pub model_id: ModelIdConfig, + pub model: ModelConfig, pub providers: LlmProviderConfig, pub events: ConversationStream, pub title: Option, @@ -48,42 +33,17 @@ impl TitleGeneratorTask { config: &AppConfig, is_tty: bool, ) -> Result> { - // Prefer the title generation model id, otherwise use the assistant - // model id. - let mut model = config - .conversation - .title - .generate - .model - .clone() - .unwrap_or_else(|| ModelConfig { - id: config.assistant.model.id.clone(), - parameters: ParametersConfig::default(), - }); - - let model_id = model.id.resolved().clone(); + let model = title::resolve_model(config, None); // Fail fast on a misconfigured title provider (e.g. a missing API // key environment variable). Without this, the failure only surfaces // inside the spawned task, after the query has already committed to // waiting for it at teardown. - provider::preflight(model_id.provider, &config.providers.llm)?; - - // If reasoning is explicitly enabled for title generation, use it, - // otherwise limit it to low effort. - if model.parameters.reasoning.is_none() { - model.parameters.reasoning = Some( - CustomReasoningConfig { - effort: ReasoningEffort::Low, - exclude: true, - } - .into(), - ); - } + provider::preflight(model.id.resolved().provider, &config.providers.llm)?; Ok(Self { conversation_id, - model_id, + model, providers: config.providers.llm.clone(), events, title: None, @@ -94,68 +54,25 @@ impl TitleGeneratorTask { async fn update_title(&mut self) -> Result<(), Box> { trace!(conversation_id = %self.conversation_id, "Updating conversation title."); - let provider = provider::get_provider(self.model_id.provider, &self.providers)?; - let model = provider.model_details(&self.model_id.name).await?; - - let sections = title::title_instructions(1, &[]); - let thread = ThreadBuilder::default() - .with_events(self.events.clone()) - .with_sections(sections) - .build()?; - - let schema = title::title_schema(1); - let mut events = thread.events.clone(); - events.start_turn(ChatRequest { - content: "Generate a title for this conversation.".into(), - schema: Some(schema), - author: None, - }); - - let query = jp_llm::query::ChatQuery { - thread: jp_conversation::thread::Thread { events, ..thread }, - tools: vec![], - tool_choice: jp_config::assistant::tool_choice::ToolChoice::default(), - }; - - let retry_config = RetryConfig::default(); - let llm_events = - collect_with_retry(provider.as_ref(), &model, query, &retry_config).await?; - - // Pipe raw streaming events through the EventBuilder so that - // structured JSON chunks are concatenated and parsed into a - // proper Value (rather than individual Value::String fragments). - let mut builder = EventBuilder::new(); - let mut flushed = Vec::new(); - for event in llm_events { - match event { - Event::Part { - index, - part, - metadata, - } => { - builder.handle_part(index, part, metadata); - } - Event::Flush { index, metadata } => { - flushed.extend(builder.handle_flush(index, metadata)); - } - Event::Finished(_) => flushed.extend(builder.drain()), - Event::Patch(_) | Event::KeepAlive => {} - } - } - - let structured_data = flushed - .into_iter() - .filter_map(ConversationEvent::into_chat_response) - .find_map(ChatResponse::into_structured_data); + let model_id = self.model.id.resolved().clone(); + let provider = provider::get_provider(model_id.provider, &self.providers)?; + let details = provider.model_details(&model_id.name).await?; - if let Some(data) = structured_data { - let titles = title::extract_titles(&data); - trace!(titles = ?titles, "Received conversation titles."); - if let Some(t) = titles.into_iter().next() { - self.title = Some(t); - } - } else { - warn!(conversation_id = %self.conversation_id, "No structured data in title response."); + let titles = title::generate(provider.as_ref(), &details, TitleRequest { + events: self.events.clone(), + model: self.model.clone(), + count: 1, + rejected: vec![], + }) + .await?; + + trace!(?titles, "Received conversation titles."); + self.title = titles.into_iter().next(); + if self.title.is_none() { + warn!( + conversation_id = %self.conversation_id, + "No title in the generation response." + ); } Ok(()) diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 731e64e9..5cf582cb 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -204,7 +204,7 @@ "summary": "Introduces Workspace::sanitize() method that validates .jp data store, moves corrupt conversations to .trash/ with error explanations, and ensures four invariants before CLI commands run." }, "053-auto-refresh-conversation-titles.md": { - "hash": "245eac64105b376afb1148a74c06983f4b26258cc6281300f0ea90292ed8f20b", + "hash": "00f211df950bf5981ba941b21a785f5264fb1770207d97709c1abd52cd7ed0ba", "summary": "Auto-refresh stale conversation titles periodically by re-running LLM generation when accumulated turns exceed a configured threshold." }, "054-split-conversation-config-and-events.md": { diff --git a/docs/rfd/053-auto-refresh-conversation-titles.md b/docs/rfd/053-auto-refresh-conversation-titles.md index 33bd024e..fa74c672 100644 --- a/docs/rfd/053-auto-refresh-conversation-titles.md +++ b/docs/rfd/053-auto-refresh-conversation-titles.md @@ -24,8 +24,8 @@ exchange. The user is left with a list of conversations whose titles no longer reflect what they contain. -The user can already run `conversation edit --title` to manually regenerate a -title, but this requires noticing the problem and taking action. +The user can already run `conversation title` to manually regenerate a title, +but this requires noticing the problem and taking action. Periodic automatic refresh should be transparent. The fix needs to be careful about cost. @@ -139,7 +139,7 @@ The migration policy is conservative: The user (or a previous automatic generation) produced this title; without provenance the safe default is to leave it alone. A conversation enrolls into auto-refresh only after an explicit `conversation - edit --title` (no argument) records a watermark. + title` (or the bare `conversation edit --title`) records a watermark. - `title_generated_at_turn = None` and `title = None`: eligible. There is no title to overwrite, so auto-refresh proceeds with baseline `0`. @@ -165,10 +165,12 @@ The affected surfaces: | Surface | Behavior | | ------------------------------------ | --------------------------------------- | -| `conversation edit --title` (no arg) | Regenerates via LLM. Sets | +| `conversation title` | Regenerates via LLM. Sets | | | `title_generated_at_turn = Some(turn)`. | | | No disable-delta — the user opted into | | | LLM-driven titling. | +| `conversation edit --title` (no arg) | Delegates to `conversation title`, so | +| | it behaves identically. | | `conversation edit --title "T"` | Sets `title = Some("T")`, writes | | | disable-delta. | | `conversation edit --no-title` | Clears title, writes disable-delta. | @@ -380,7 +382,7 @@ keeping its perfectly good title. The `title_schema` and `title_instructions` helpers in `jp_llm::title` are shared by initial generation (`TitleGeneratorTask`), interactive regeneration -(`conversation edit --title`), and the new refresh path. +(`conversation title`), and the new refresh path. Adding `retain_current` unconditionally would let the LLM respond with "keep current" to the interactive regeneration path — which is the opposite of what the user asked for. @@ -399,19 +401,17 @@ This scoping happens before any token-level checks. The title generation model may still have a smaller context window than those N turns require. -The inquiry system already solves this problem: it estimates char-based token -counts and drops older events to fit the model's context window. - -The core truncation logic — estimate chars, compare to budget, drop oldest -events, re-sanitize — is extracted from `jp_cli::cmd::query::tool::inquiry` -into a shared utility (in `jp_llm` or `jp_conversation`) that both the inquiry -backend and the title generator can use. -Each caller computes its own overhead (the inquiry system accounts for tools, -attachments, and cache-preserving granularity; the title generator only needs -system prompt and title instructions). - +`jp_llm::window` handles that: `truncate_to_fit` estimates the stream's size in +chars, compares it against a budget derived from the model's context window, and +drops the oldest events until it fits. +Each caller measures its own overhead with `estimate_overhead_chars`: the +inquiry backend accounts for tools and attachments, while title generation only +needs its instruction sections. + +`jp_llm::title::generate` applies this to every title request, so the refresh +path inherits it by calling that function rather than repeating the logic. The pipeline for each candidate is: scope to last `turn_context` turns \> -estimate chars \> truncate if over budget \> send to LLM. +truncate if over budget \> send to LLM. #### Sync (main thread) @@ -569,14 +569,20 @@ may shift focus as the conversation evolves. ## Implementation Plan -### Phase 0: Shared truncation utility (independent) +### Phase 0: Shared truncation utility (implemented) + +Shipped ahead of the rest of this RFD. +The fitting logic lives in `jp_llm::window` (`truncate_to_fit`, +`estimate_overhead_chars`, `estimate_chars`, `budget_chars`), and the inquiry +backend, `jp_llm::title`, and the compaction summarizer all measure against it. + +Title generation itself moved into `jp_llm::title::generate`, shared by +`TitleGeneratorTask` and `jp conversation title`, so Phase 5's refresh path gets +context window fitting by calling that function. -- Extract the core truncation logic (estimate chars, compare to budget, drop - oldest events, re-sanitize) from `jp_cli::cmd::query::tool::inquiry` into a - shared utility in `jp_llm` or `jp_conversation`. -- Update the inquiry backend to use the shared utility. -- Update `TitleGeneratorTask::update_title` to truncate the event stream when - the title model's context window is smaller than the conversation. +The summarizer uses the same measurements but not the same remedy: a summary +stands in for every turn it covers, so `jp conversation compact --summarize` +rejects a range that exceeds the window instead of shortening it. ### Phase 1: Configuration (independent) @@ -610,8 +616,9 @@ may shift focus as the conversation evolves. - `query --title "..."` - `query --no-title` - `conversation fork --title "..."` -- Update `conversation edit --title` (no argument) to set - `title_generated_at_turn = Some(current_turn_count)` after LLM generation. +- Update `conversation title` to set `title_generated_at_turn = + Some(current_turn_count)` after LLM generation. + The bare `conversation edit --title` delegates there and inherits it. ### Phase 4: Title retention schema (independent) @@ -622,7 +629,7 @@ may shift focus as the conversation evolves. - Add a companion function (or extend `extract_titles`) that returns the `retain_current` flag alongside the title list. -### Phase 5: Task and spawn (depends on Phase 0, 1, 2, 3, 4) +### Phase 5: Task and spawn (depends on Phase 1, 2, 3, 4) - Implement `TitleRefreshTask` with the full background pipeline: scan conversation IDs via `LoadBackend`, read metadata and turn counts, sort @@ -648,6 +655,8 @@ Coverage for the high-risk paths, paired with the phases that introduce them: - `query --title`, `query --no-title`, `conversation edit --title "..."`, `conversation edit --no-title`, and `conversation fork --title "..."` each write the disable-delta and prevent future refresh. +- `conversation title` does not write the disable-delta; the conversation stays + enrolled in refresh. - `conversation fork --last N` clamps `title_generated_at_turn` to the new turn count. - `--no-persist` does not spawn `TitleRefreshTask`. @@ -656,7 +665,7 @@ Coverage for the high-risk paths, paired with the phases that introduce them: - A locked candidate is skipped at preflight without an LLM call. - `retain_current = true` advances the watermark without changing `title`. -Phases 0, 1, 2, and 4 can be reviewed and merged independently. +Phases 1, 2, and 4 can be reviewed and merged independently. Phase 3 depends on Phase 1. Phase 5 depends on all earlier phases. Phase 6 (tests) is paired with each phase as it lands.