diff --git a/.config/jp/tools/src/fs/create_file.rs b/.config/jp/tools/src/fs/create_file.rs index 3cbde8fd..1ae3b790 100644 --- a/.config/jp/tools/src/fs/create_file.rs +++ b/.config/jp/tools/src/fs/create_file.rs @@ -72,13 +72,12 @@ pub(crate) async fn fs_create_file( Some(true) => {} Some(false) => return error("Path points to existing file"), None => { - return Ok(Outcome::NeedsInput { - question: Question::boolean( - "overwrite_file", - format!("File '{path}' exists. Overwrite?"), - ) - .with_default(Value::Bool(false)), - }); + let question = Question::boolean( + "overwrite_file", + format!("File '{path}' exists. Overwrite?"), + )? + .with_default(Value::Bool(false)); + return Ok(Outcome::NeedsInput { question }); } }, None => {} diff --git a/.config/jp/tools/src/fs/delete_file.rs b/.config/jp/tools/src/fs/delete_file.rs index f2b6ab8c..b1103e7c 100644 --- a/.config/jp/tools/src/fs/delete_file.rs +++ b/.config/jp/tools/src/fs/delete_file.rs @@ -52,13 +52,12 @@ pub(crate) async fn fs_delete_file( return error("File has uncommitted changes. Please stage or discard first."); } None => { - return Ok(Outcome::NeedsInput { - question: Question::boolean( - "delete_dirty_file", - format!("File '{path}' has uncommitted changes. Delete anyway?"), - ) - .with_default(Value::Bool(false)), - }); + let question = Question::boolean( + "delete_dirty_file", + format!("File '{path}' has uncommitted changes. Delete anyway?"), + )? + .with_default(Value::Bool(false)); + return Ok(Outcome::NeedsInput { question }); } } } diff --git a/.config/jp/tools/src/fs/modify_file.rs b/.config/jp/tools/src/fs/modify_file.rs index e0e2fb60..5dffc77a 100644 --- a/.config/jp/tools/src/fs/modify_file.rs +++ b/.config/jp/tools/src/fs/modify_file.rs @@ -415,12 +415,15 @@ fn guard_broad_replacement( Some(true) => None, Some(false) => Some(fail(reject_message)), None => { - let mut q = - Question::boolean("broad_replacement", text).with_default(Value::Bool(false)); + let question = match Question::boolean("broad_replacement", text) { + Ok(q) => q, + Err(e) => return Some(Err(e.into())), + }; + let mut question = question.with_default(Value::Bool(false)); if let Some(p) = pre_amble { - q = q.with_preamble(p); + question = question.with_preamble(p); } - Some(Ok(Outcome::NeedsInput { question: q })) + Some(Ok(Outcome::NeedsInput { question })) } } } @@ -751,12 +754,11 @@ fn apply_changes( return Err("File has uncommitted changes. Change discarded.".into()); } None => { - return Ok(Outcome::NeedsInput { - question: Question::boolean( - "modify_dirty_file", - format!("File '{path}' has uncommitted changes. Modify anyway?"), - ), - }); + let question = Question::boolean( + "modify_dirty_file", + format!("File '{path}' has uncommitted changes. Modify anyway?"), + )?; + return Ok(Outcome::NeedsInput { question }); } } } @@ -800,14 +802,13 @@ fn apply_changes( ); } None => { - return Ok(Outcome::NeedsInput { - question: Question::boolean( - "apply_changes", - "Do you want to apply the patch shown above?", - ) - .with_preamble(patch) - .with_default(Value::Bool(true)), - }); + let question = Question::boolean( + "apply_changes", + "Do you want to apply the patch shown above?", + )? + .with_preamble(patch) + .with_default(Value::Bool(true)); + return Ok(Outcome::NeedsInput { question }); } } diff --git a/.config/jp/tools/src/fs/move_file.rs b/.config/jp/tools/src/fs/move_file.rs index 939c9ab0..1376339a 100644 --- a/.config/jp/tools/src/fs/move_file.rs +++ b/.config/jp/tools/src/fs/move_file.rs @@ -117,7 +117,7 @@ fn fs_move_file_impl( // so. Lumping symlinks in here is consistent with the source // side, where the link entry itself is what gets renamed. Some(_) => { - if let Some(outcome) = confirm_overwrite_file(answers, target) { + if let Some(outcome) = confirm_overwrite_file(answers, target)? { return Ok(outcome); } } @@ -221,19 +221,23 @@ fn classify_source( Ok(Some(result)) } -fn confirm_overwrite_file(answers: &Map, target: &str) -> Option { +fn confirm_overwrite_file( + answers: &Map, + target: &str, +) -> Result, Error> { match answers.get("overwrite_file").and_then(Value::as_bool) { - Some(true) => None, - Some(false) => Some(error_outcome(format!( + Some(true) => Ok(None), + Some(false) => Ok(Some(error_outcome(format!( "Destination '{target}' already exists." - ))), - None => Some(Outcome::NeedsInput { - question: Question::boolean( + )))), + None => { + let question = Question::boolean( "overwrite_file", format!("Destination '{target}' exists. Overwrite?"), - ) - .with_default(Value::Bool(false)), - }), + )? + .with_default(Value::Bool(false)); + Ok(Some(Outcome::NeedsInput { question })) + } } } @@ -278,10 +282,11 @@ fn confirm_dirty_source( Some(false) => Ok(Some(error_outcome(format!( "'{source}' has uncommitted changes; please stage or discard first." )))), - None => Ok(Some(Outcome::NeedsInput { - question: Question::boolean("move_dirty_source", prompt) - .with_default(Value::Bool(false)), - })), + None => { + let question = + Question::boolean("move_dirty_source", prompt)?.with_default(Value::Bool(false)); + Ok(Some(Outcome::NeedsInput { question })) + } } } diff --git a/.config/jp/tools/src/git/stage_patch.rs b/.config/jp/tools/src/git/stage_patch.rs index 05bf78b0..0ff1ef14 100644 --- a/.config/jp/tools/src/git/stage_patch.rs +++ b/.config/jp/tools/src/git/stage_patch.rs @@ -68,14 +68,13 @@ fn git_stage_patch_impl( return Ok("Changes not staged.".into()); } None => { - return Ok(Outcome::NeedsInput { - question: Question::boolean( - "stage_changes", - "Do you want to stage the patch shown above?", - ) - .with_preamble(combined) - .with_default(Value::Bool(true)), - }); + let question = Question::boolean( + "stage_changes", + "Do you want to stage the patch shown above?", + )? + .with_preamble(combined) + .with_default(Value::Bool(true)); + return Ok(Outcome::NeedsInput { question }); } } diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index 43d371e1..ddf8f2b4 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -58,8 +58,9 @@ //! - **LLM target**: The question is formatted as a response asking the LLM to //! re-run the tool with the answer. //! The tool is marked as completed. -//! - **Static answer**: Pre-populated before first execution, so the tool -//! should never ask for this input. +//! - **Static answer**: When the tool asks a question with a configured +//! `QuestionConfig.answer`, the value is supplied without a prompt and the +//! round-trip is recorded as an inquiry request/response pair. //! //! # Non-Blocking Prompts //! @@ -84,6 +85,7 @@ use std::{ use camino::{Utf8Path, Utf8PathBuf}; use indexmap::IndexMap; +use inquire::error::InquireError; use jp_config::{ conversation::tool::{ FormatMode, QuestionTarget, ResultMode, RunMode, ToolsConfig, style::ParametersStyle, @@ -93,8 +95,8 @@ use jp_config::{ use jp_conversation::{ ConversationStream, event::{ - InquiryAnswerType, InquiryQuestion, InquiryRequest, InquiryResponse, InquirySource, - SelectOption, ToolCallRequest, ToolCallResponse, + CancellationReason, InquiryAnswerType, InquiryId, InquiryQuestion, InquiryRequest, + InquiryResponse, SelectOption, ToolCallRequest, ToolCallResponse, }, }; use jp_editor::EditorBackend; @@ -111,11 +113,15 @@ use tracing::warn; use super::{ ToolRenderer, - inquiry::{self, InquiryBackend}, - prompter::{PermissionResult, ToolPrompter, permission_inquiry_id, tool_question_inquiry_id}, + inquiry::{self, InquiryBackend, InquiryError}, + prompter::{PermissionResult, ToolPrompter}, }; use crate::{ - cmd::query::turn::{TurnCoordinator, state::TurnState}, + Error, + cmd::query::turn::{ + TurnCoordinator, + state::{PermissionCacheKey, ToolAnswerCacheKey, TurnState}, + }, render::tool::RenderOutcome, }; @@ -129,20 +135,29 @@ enum ExecutionEvent { PromptAnswer { index: usize, question_id: String, + inquiry_id: InquiryId, answer: Value, persist_level: jp_tool::PersistLevel, + /// Whether the persisted response must be recorded as `Redacted` (the + /// question's answer type is `Secret`). + redact: bool, }, PromptCancelled { index: usize, + inquiry_id: InquiryId, + /// Why the prompt closed without an answer, mapped from the prompter + /// error at the prompt site. + reason: CancellationReason, }, /// Result of a structured inquiry (LLM answering a tool question). InquiryResult { index: usize, + inquiry_id: InquiryId, question_id: String, question_text: String, - result: Result, + result: Result, }, ResultModeProcessed { @@ -181,6 +196,7 @@ enum PendingPrompt { Question { index: usize, question: Question, + inquiry_id: InquiryId, }, ResultMode { index: usize, @@ -265,6 +281,7 @@ fn tool_question_to_inquiry_question(q: &Question) -> InquiryQuestion { .collect(), }, AnswerType::Text => InquiryAnswerType::Text, + AnswerType::Secret => InquiryAnswerType::Secret, }; let mut iq = InquiryQuestion::new(q.text.clone(), answer_type); @@ -531,33 +548,6 @@ impl ToolCoordinator { }) } - pub fn static_answers_for_tool( - &self, - tool_name: &str, - ) -> indexmap::IndexMap { - let mut answers = indexmap::IndexMap::new(); - if let Some(config) = self.tools_config.get(tool_name) { - for (question_id, question_config) in config.questions() { - if let Some(answer) = &question_config.answer { - answers.insert(question_id.clone(), answer.clone()); - } - } - } - answers - } - - /// Returns pre-configured static answers for a tool's questions. - /// - /// These are answers set in the tool configuration (e.g. - /// `questions.confirm.answer = true`) that bypass both user prompts and LLM - /// inquiries. - pub(crate) fn static_answers_for_all_questions( - &self, - tool_name: &str, - ) -> IndexMap { - self.static_answers_for_tool(tool_name) - } - pub fn is_hidden(&self, tool_name: &str) -> bool { self.tools_config .get(tool_name) @@ -696,31 +686,26 @@ impl ToolCoordinator { } // Check for a persisted permission decision from earlier in this turn. - let permission_id = permission_inquiry_id(&info.tool_name); + let permission_key = PermissionCacheKey::new(&info.tool_name); let persisted = turn_state - .persisted_inquiry_responses - .get(&permission_id) - .and_then(|r| r.answer.as_str()) - .map(str::to_owned); - - if let Some(ref decision) = persisted { - match decision.as_str() { - "y" | "Y" => { - self.set_tool_state(&info.tool_id, ToolCallState::Running); - return PermissionDecision::Approved(executor); - } - "n" | "N" => { - self.set_tool_state(&info.tool_id, ToolCallState::Completed); - return PermissionDecision::Skipped(ToolCallResponse { - id: info.tool_id.clone(), - result: Ok("Tool skipped by user (remembered).".to_string()), - }); - } - _ => {} // Unknown value, fall through to prompt + .remembered_permission_decisions + .get(&permission_key) + .copied(); + + match persisted { + Some(true) => { + self.set_tool_state(&info.tool_id, ToolCallState::Running); + PermissionDecision::Approved(executor) } + Some(false) => { + self.set_tool_state(&info.tool_id, ToolCallState::Completed); + PermissionDecision::Skipped(ToolCallResponse { + id: info.tool_id.clone(), + result: Ok("Tool skipped by user (remembered).".to_string()), + }) + } + None => PermissionDecision::NeedsPrompt { executor, info }, } - - PermissionDecision::NeedsPrompt { executor, info } } /// Applies the result of an interactive permission prompt. @@ -734,15 +719,14 @@ impl ToolCoordinator { turn_state: &mut TurnState, mut executor: Box, ) -> Result, ToolCallResponse> { - let permission_id = permission_inquiry_id(&info.tool_name); + let permission_key = PermissionCacheKey::new(&info.tool_name); match result { Ok(PermissionResult::Run { arguments, persist }) => { if persist { - turn_state.persisted_inquiry_responses.insert( - permission_id.clone(), - InquiryResponse::select(permission_id, "y"), - ); + turn_state + .remembered_permission_decisions + .insert(permission_key, true); } executor.set_arguments(arguments); self.set_tool_state(&info.tool_id, ToolCallState::Running); @@ -750,10 +734,9 @@ impl ToolCoordinator { } Ok(PermissionResult::Skip { reason, persist }) => { if persist { - turn_state.persisted_inquiry_responses.insert( - permission_id.clone(), - InquiryResponse::select(permission_id, "n"), - ); + turn_state + .remembered_permission_decisions + .insert(permission_key, false); } self.set_tool_state(&info.tool_id, ToolCallState::Completed); let msg = if let Some(r) = reason { @@ -866,7 +849,10 @@ impl ToolCoordinator { for (index, executor) in executors.into_iter().enumerate() { let tool_id = executor.tool_id().to_string(); let tool_name = executor.tool_name().to_string(); - let accumulated_answers = self.static_answers_for_all_questions(&tool_name); + // No pre-seeding: static answers flow through the late + // `static_answer` path so every question round-trip is recorded as + // an inquiry pair (RFD 082). + let accumulated_answers = IndexMap::new(); let executor: Arc = Arc::from(executor); @@ -971,14 +957,18 @@ impl ToolCoordinator { ExecutionEvent::PromptAnswer { index, question_id, + inquiry_id, answer, persist_level, + redact, } => { self.handle_prompt_answer( index, question_id, + &inquiry_id, answer, persist_level, + redact, &mut executing_tools, &mut pending_prompts, &mut prompt_active, @@ -987,26 +977,23 @@ impl ToolCoordinator { root, &cancellation_token, event_tx.clone(), + conv, turn_state, ); } ExecutionEvent::InquiryResult { index, + inquiry_id, question_id, question_text, result, } => match result { Ok(answer) => { + // Close the recorded pair before the tool lookup, so an + // unknown index cannot leave the request unpaired on + // disk (sanitize() would drop it on the next load). + Self::record_inquiry_answer(conv, &inquiry_id, &answer); if let Some(tool) = executing_tools.get_mut(&index) { - let id = inquiry::tool_call_inquiry_id(&tool.tool_id, &question_id); - conv.update_events(|events| { - events - .current_turn_mut() - .add_inquiry_response(InquiryResponse::new(id, answer.clone())) - .build() - .expect("Invalid ConversationStream state"); - }); - tool.accumulated_answers.insert(question_id, answer); self.set_tool_state(&tool.tool_id, ToolCallState::Running); Self::spawn_tool_execution( @@ -1018,35 +1005,54 @@ impl ToolCoordinator { cancellation_token.child_token(), event_tx.clone(), ); + } else { + warn!(index, "Received InquiryResult for unknown tool."); } } - Err(error) => match executing_tools.get(&index) { - None => warn!(index, %error, "Received ToolResult for unknown tool."), - Some(tool) => { - self.set_tool_state(&tool.tool_id, ToolCallState::Completed); - - results[index] = Some(ToolCallResponse { - id: tool.tool_id.clone(), - result: Err(format!( - "The tool '{}' asked a follow-up question (\"{}\") that was \ - routed to a secondary assistant for resolution, but the \ - secondary assistant failed to provide a valid answer. Error: \ - {}. You may retry the tool call or end the turn.", - tool.tool_name, question_text, error, - )), - }); + Err(error) => { + Self::record_inquiry_cancelled( + conv, + &inquiry_id, + Self::cancellation_reason(&error), + ); + match executing_tools.get(&index) { + None => { + warn!(index, %error, "Received InquiryResult for unknown tool."); + } + Some(tool) => { + self.set_tool_state(&tool.tool_id, ToolCallState::Completed); + + results[index] = Some(ToolCallResponse { + id: tool.tool_id.clone(), + result: Err(format!( + "The tool '{}' asked a follow-up question (\"{}\") that \ + was routed to a secondary assistant for resolution, but \ + the secondary assistant failed to provide a valid \ + answer. Error: {}. You may retry the tool call or end \ + the turn.", + tool.tool_name, question_text, error, + )), + }); + } } - }, + } }, - ExecutionEvent::PromptCancelled { index } => { + ExecutionEvent::PromptCancelled { + index, + inquiry_id, + reason, + } => { self.handle_prompt_cancelled( index, + &inquiry_id, + reason, &mut executing_tools, &mut results, &mut pending_prompts, &mut prompt_active, prompter.clone(), event_tx.clone(), + conv, ); } ExecutionEvent::ResultModeProcessed { @@ -1211,9 +1217,86 @@ impl ToolCoordinator { }); } + /// Record an `InquiryResponse::Answered` for a request recorded earlier in + /// this turn. + fn record_inquiry_answer(conv: &ConversationMut, inquiry_id: &InquiryId, answer: &Value) { + conv.update_events(|events| { + events + .current_turn_mut() + .add_inquiry_response(InquiryResponse::answered( + inquiry_id.clone(), + answer.clone(), + )) + .build() + .expect("Invalid ConversationStream state"); + }); + } + + /// Record an `InquiryResponse::Cancelled` for a request recorded earlier in + /// this turn, closing the pair without an answer. + fn record_inquiry_cancelled( + conv: &ConversationMut, + inquiry_id: &InquiryId, + reason: CancellationReason, + ) { + conv.update_events(|events| { + events + .current_turn_mut() + .add_inquiry_response(InquiryResponse::Cancelled { + id: inquiry_id.clone(), + reason, + }) + .build() + .expect("Invalid ConversationStream state"); + }); + } + + /// Record an `InquiryResponse::Redacted` for a request recorded earlier in + /// this turn: the tool received the answer in-memory, but the persisted + /// record deliberately omits it. + fn record_inquiry_redacted(conv: &ConversationMut, inquiry_id: &InquiryId) { + conv.update_events(|events| { + events + .current_turn_mut() + .add_inquiry_response(InquiryResponse::Redacted { + id: inquiry_id.clone(), + }) + .build() + .expect("Invalid ConversationStream state"); + }); + } + + /// Map an inquiry-backend error to its persisted [`CancellationReason`]. + /// + /// `InquiryError::Cancelled` is user-initiated (the cancellation token was + /// fired by a user action); every other variant is a genuine backend + /// failure. + fn cancellation_reason(error: &InquiryError) -> CancellationReason { + match error { + InquiryError::Cancelled => CancellationReason::User, + InquiryError::Provider(_) + | InquiryError::MissingStructuredData + | InquiryError::AnswerExtraction { .. } + | InquiryError::Other(_) => CancellationReason::BackendError, + } + } + + /// Map a prompter error to its persisted [`CancellationReason`]. + /// + /// `OperationCanceled`/`OperationInterrupted` are user-initiated (Esc, + /// Ctrl-C, or EOF at the prompt); every other error is a prompt failure. + fn prompt_cancellation_reason(error: &Error) -> CancellationReason { + match error { + Error::Inquire( + InquireError::OperationCanceled | InquireError::OperationInterrupted, + ) => CancellationReason::User, + _ => CancellationReason::BackendError, + } + } + fn spawn_inquiry( index: usize, - inquiry_id: String, + inquiry_id: InquiryId, id: String, tool_name: String, question: Question, @@ -1239,18 +1322,18 @@ impl ToolCoordinator { let result = backend .inquire( events, - &inquiry_id, + inquiry_id.as_str(), &tool_name, &question, cancellation_token, ) - .await - .map_err(|error| error.to_string()); + .await; let _err = event_tx .send(ExecutionEvent::InquiryResult { index, - question_id: question.id, + inquiry_id, + question_id: question.id.to_string(), question_text: question.text, result, }) @@ -1368,31 +1451,68 @@ impl ToolCoordinator { tool_id, tool_name, question, + source, accumulated_answers, } => { tool.accumulated_answers = accumulated_answers.clone(); - let question_inq_id = tool_question_inquiry_id(&tool_name, &question.id); - let persisted_answer = turn_state - .persisted_inquiry_responses - .get(&question_inq_id) - .map(|r| r.answer.clone()); - if let Some(answer) = persisted_answer { - tool.accumulated_answers.insert(question.id.clone(), answer); - Self::spawn_tool_execution( - index, - tool.executor.clone(), - tool.accumulated_answers.clone(), - mcp_client.clone(), - root.to_path_buf(), - cancellation_token.clone(), - event_tx, - ); - return; + // Allocate the inquiry ID (incrementing the per-turn attempt + // counter) and record the `InquiryRequest` before any routing + // decision, so every question round-trip lands on the stream + // regardless of how it is answered. + let attempt = turn_state.next_inquiry_attempt(&tool_id, question.id.as_str()); + let inquiry_id = InquiryId::new(inquiry::tool_call_inquiry_id( + &tool_id, + question.id.as_str(), + attempt, + )); + let inquiry_question = tool_question_to_inquiry_question(&question); + conv.update_events(|events| { + events + .current_turn_mut() + .add_inquiry_request(InquiryRequest::new( + inquiry_id.clone(), + source, + inquiry_question, + )) + .build() + .expect("Invalid ConversationStream state"); + }); + + let is_secret = question.answer_type == AnswerType::Secret; + + // Secrets never enter or read the turn-answer cache. + if !is_secret { + let answer_key = ToolAnswerCacheKey::new(&tool_name, question.id.as_str()); + let persisted_answer = + turn_state.remembered_tool_answers.get(&answer_key).cloned(); + if let Some(answer) = persisted_answer { + Self::record_inquiry_answer(conv, &inquiry_id, &answer); + tool.accumulated_answers + .insert(question.id.to_string(), answer); + Self::spawn_tool_execution( + index, + tool.executor.clone(), + tool.accumulated_answers.clone(), + mcp_client.clone(), + root.to_path_buf(), + cancellation_token.clone(), + event_tx, + ); + return; + } } - if let Some(answer) = self.static_answer(&tool_name, &question.id) { - tool.accumulated_answers.insert(question.id.clone(), answer); + if let Some(answer) = self.static_answer(&tool_name, question.id.as_str()) { + // The tool still receives the configured value in-memory; + // only the persisted record is redacted for secrets. + if is_secret { + Self::record_inquiry_redacted(conv, &inquiry_id); + } else { + Self::record_inquiry_answer(conv, &inquiry_id, &answer); + } + tool.accumulated_answers + .insert(question.id.to_string(), answer); Self::spawn_tool_execution( index, tool.executor.clone(), @@ -1406,7 +1526,7 @@ impl ToolCoordinator { } let target = self - .question_target(&tool_name, &question.id) + .question_target(&tool_name, question.id.as_str()) .unwrap_or(QuestionTarget::User); tracing::info!( @@ -1422,32 +1542,52 @@ impl ToolCoordinator { if is_tty && target.is_user() { if *prompt_active { - pending_prompts.push_back(PendingPrompt::Question { index, question }); + pending_prompts.push_back(PendingPrompt::Question { + index, + question, + inquiry_id, + }); } else { *prompt_active = true; self.set_tool_state(&tool_id, ToolCallState::AwaitingInput); - Self::spawn_user_prompt(index, question, prompter.clone(), event_tx); + Self::spawn_user_prompt( + index, + question, + inquiry_id, + prompter.clone(), + event_tx, + ); } - } else { - // Route through the inquiry backend: either the target is - // explicitly `Assistant`, or the user can't be prompted - // (non-interactive). Record the request in the persisted - // stream, then spawn the async inquiry on a cloned - // snapshot. - let inquiry_id = inquiry::tool_call_inquiry_id(&tool_id, &question.id); - - conv.update_events(|events| { - events - .current_turn_mut() - .add_inquiry_request(InquiryRequest::new( - inquiry_id.clone(), - InquirySource::tool(tool_name.clone()), - tool_question_to_inquiry_question(&question), - )) - .build() - .expect("Invalid ConversationStream state"); + } else if is_secret { + // A secret requires a human at an interactive prompt; it + // must never route to the inquiry backend. Fail the tool + // and close the recorded inquiry with the guard's reason. + let (reason, message) = if target.is_user() { + ( + CancellationReason::NoPromptBackend, + format!( + "The tool '{tool_name}' asked for a secret value, which requires \ + an interactive prompt, but no interactive terminal is available." + ), + ) + } else { + ( + CancellationReason::AssistantRoutingDenied, + format!( + "The tool '{tool_name}' asked for a secret value, which must be \ + entered by a human and cannot be routed to the assistant." + ), + ) + }; + Self::record_inquiry_cancelled(conv, &inquiry_id, reason); + self.set_tool_state(&tool_id, ToolCallState::Completed); + *tracked_response = Some(ToolCallResponse { + id: tool_id.clone(), + result: Err(message), }); - + } else { + // The `InquiryRequest` is already recorded above; spawn the + // async inquiry on a cloned snapshot. Self::spawn_inquiry( index, inquiry_id, @@ -1470,8 +1610,10 @@ impl ToolCoordinator { &mut self, index: usize, question_id: String, + inquiry_id: &InquiryId, answer: Value, persist_level: jp_tool::PersistLevel, + redact: bool, executing_tools: &mut HashMap, pending_prompts: &mut VecDeque, prompt_active: &mut bool, @@ -1480,15 +1622,26 @@ impl ToolCoordinator { root: &Utf8Path, cancellation_token: &CancellationToken, event_tx: mpsc::Sender, + conv: &ConversationMut, turn_state: &mut TurnState, ) { *prompt_active = false; + + // Close the recorded inquiry with the user's answer; a secret answer + // is persisted as `Redacted` and never carries the value. + if redact { + Self::record_inquiry_redacted(conv, inquiry_id); + } else { + Self::record_inquiry_answer(conv, inquiry_id, &answer); + } + if let Some(tool) = executing_tools.get_mut(&index) { - if persist_level == jp_tool::PersistLevel::Turn { - let inq_id = tool_question_inquiry_id(&tool.tool_name, &question_id); + // Secrets never enter the turn-answer cache. + if persist_level == jp_tool::PersistLevel::Turn && !redact { + let answer_key = ToolAnswerCacheKey::new(&tool.tool_name, &question_id); turn_state - .persisted_inquiry_responses - .insert(inq_id.clone(), InquiryResponse::new(inq_id, answer.clone())); + .remembered_tool_answers + .insert(answer_key, answer.clone()); } tool.accumulated_answers.insert(question_id, answer); self.set_tool_state(&tool.tool_id, ToolCallState::Running); @@ -1511,22 +1664,35 @@ impl ToolCoordinator { ); } + #[allow(clippy::too_many_arguments)] fn handle_prompt_cancelled( &mut self, index: usize, + inquiry_id: &InquiryId, + reason: CancellationReason, executing_tools: &mut HashMap, results: &mut [Option], pending_prompts: &mut VecDeque, prompt_active: &mut bool, prompter: Arc, event_tx: mpsc::Sender, + conv: &ConversationMut, ) { *prompt_active = false; + + // A user cancellation (Esc / Ctrl-C / EOF at the prompt) completes the + // tool benignly; a prompt failure is a tool-level error. + let result = match reason { + CancellationReason::User => Ok("Tool input cancelled by user.".to_string()), + _ => Err("Tool input prompt failed.".to_string()), + }; + Self::record_inquiry_cancelled(conv, inquiry_id, reason); + if let Some(tool) = executing_tools.get(&index) { self.set_tool_state(&tool.tool_id, ToolCallState::Completed); results[index] = Some(ToolCallResponse { id: tool.tool_id.clone(), - result: Ok("Tool input cancelled by user.".to_string()), + result, }); } self.process_next_prompt( @@ -1541,20 +1707,36 @@ impl ToolCoordinator { fn spawn_user_prompt( index: usize, question: Question, + inquiry_id: InquiryId, prompter: Arc, event_tx: mpsc::Sender, ) { - let question_id = question.id.clone(); - tokio::task::spawn_blocking(move || { - if let Ok(result) = prompter.prompt_question(&question) { + let question_id = question.id.to_string(); + let redact = question.answer_type == AnswerType::Secret; + tokio::task::spawn_blocking(move || match prompter.prompt_question(&question) { + Ok(result) => { drop(event_tx.blocking_send(ExecutionEvent::PromptAnswer { index, question_id, + inquiry_id, answer: result.answer, persist_level: result.persist_level, + redact, + })); + } + Err(error) => { + let reason = Self::prompt_cancellation_reason(&error); + // Esc/Ctrl-C is routine; only genuine prompt failures are + // warning-worthy. The persisted record stays coarse, so this + // trace is the only place the underlying error survives. + if reason == CancellationReason::BackendError { + warn!(%error, "Tool question prompt failed."); + } + drop(event_tx.blocking_send(ExecutionEvent::PromptCancelled { + index, + inquiry_id, + reason, })); - } else { - drop(event_tx.blocking_send(ExecutionEvent::PromptCancelled { index })); } }); } @@ -1624,11 +1806,15 @@ impl ToolCoordinator { }; *prompt_active = true; match next { - PendingPrompt::Question { index, question } => { + PendingPrompt::Question { + index, + question, + inquiry_id, + } => { if let Some(tool) = executing_tools.get(&index) { self.set_tool_state(&tool.tool_id, ToolCallState::AwaitingInput); } - Self::spawn_user_prompt(index, question, prompter, event_tx); + Self::spawn_user_prompt(index, question, inquiry_id, prompter, event_tx); } PendingPrompt::ResultMode { index, diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs index 706bc42d..0ff31d77 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -2,6 +2,7 @@ use async_trait::async_trait; use camino::Utf8PathBuf; use jp_config::conversation::tool::{ToolConfig, ToolSource, style::PartialDisplayStyleConfig}; use jp_inquire::{ReplyOutcome, prompt::MockPromptBackend}; +use jp_llm::tool::executor::MockExecutor; use jp_printer::{ErrChannel, OutputFormat, Printer}; use schematic::Config as _; @@ -259,62 +260,6 @@ fn test_static_answer_with_configured_answer() { ); } -#[test] -fn test_static_answers_for_tool_empty() { - let coordinator = ToolCoordinator::new( - jp_config::AppConfig::new_test().conversation.tools, - empty_executor_source(), - ); - // Non-existent tool returns empty map - assert!( - coordinator - .static_answers_for_tool("nonexistent_tool") - .is_empty() - ); -} - -#[test] -fn test_static_answers_for_tool_collects_all_answers() { - use jp_config::conversation::tool::{QuestionTarget, ToolConfig, ToolSource}; - use schematic::Config as _; - - // Create a tool config with multiple questions, some with answers - let tool_config = ToolConfig::from_partial( - jp_config::conversation::tool::PartialToolConfig { - source: Some(ToolSource::Builtin { tool: None }), - questions: indexmap::indexmap! { - "q1".to_string() => jp_config::conversation::tool::PartialQuestionConfig { - target: Some(QuestionTarget::User), - answer: Some(serde_json::json!("answer1")), - }, - "q2".to_string() => jp_config::conversation::tool::PartialQuestionConfig { - target: Some(QuestionTarget::User), - answer: Some(serde_json::json!(42)), - }, - "q3".to_string() => jp_config::conversation::tool::PartialQuestionConfig { - target: Some(QuestionTarget::User), - answer: None, // No static answer - } - }, - ..Default::default() - }, - vec![], - ) - .expect("valid tool config"); - - let mut tools_config = jp_config::AppConfig::new_test().conversation.tools; - tools_config.insert("my_tool".to_string(), tool_config); - - let coordinator = ToolCoordinator::new(tools_config, empty_executor_source()); - let answers = coordinator.static_answers_for_tool("my_tool"); - - // Should have 2 answers (q1 and q2, but not q3) - assert_eq!(answers.len(), 2); - assert_eq!(answers.get("q1"), Some(&serde_json::json!("answer1"))); - assert_eq!(answers.get("q2"), Some(&serde_json::json!(42))); - assert!(answers.get("q3").is_none()); -} - #[tokio::test] async fn test_pre_render_for_prompt_function_call_fires_before_approval() { // Regression test for the bug where `fs_delete_file`-style tools @@ -574,15 +519,119 @@ async fn test_resolve_tool_call_decision_invalidates_prerender_on_edit() { ); } +#[test] +fn test_permission_decision_cache_is_isolated_from_answers() { + let mut coordinator = ToolCoordinator::new( + jp_config::AppConfig::new_test().conversation.tools, + empty_executor_source(), + ); + let mut turn_state = TurnState::default(); + + let info = PermissionInfo { + tool_id: "call_1".into(), + tool_name: "my_tool".into(), + tool_source: ToolSource::Builtin { tool: None }, + run_mode: RunMode::Ask, + arguments: Value::Null, + }; + + // Persisting a "run" decision lands only in the permission cache. + coordinator + .apply_permission_result( + Ok(PermissionResult::Run { + arguments: Value::Null, + persist: true, + }), + &info, + &mut turn_state, + Box::new(MockExecutor::completed("call_1", "my_tool", "ok")), + ) + .expect("run decision returns the executor"); + + assert_eq!( + turn_state + .remembered_permission_decisions + .get(&PermissionCacheKey::new("my_tool")), + Some(&true), + ); + assert!( + turn_state.remembered_tool_answers.is_empty(), + "a permission decision must not leak into the tool-answer cache" + ); + + // A later call for the same tool reuses the decision without prompting. + let executor = + Box::new(MockExecutor::completed("call_2", "my_tool", "ok").with_permission_info(info)); + let decision = coordinator.decide_permission(executor, true, &turn_state); + assert!( + matches!(decision, PermissionDecision::Approved(_)), + "a remembered `y` decision approves without prompting" + ); +} + +#[test] +fn test_cancellation_reason_mapping() { + // `InquiryError::Cancelled` is a user-initiated cancellation; every other + // variant is a genuine backend failure. + assert_eq!( + ToolCoordinator::cancellation_reason(&InquiryError::Cancelled), + CancellationReason::User + ); + assert_eq!( + ToolCoordinator::cancellation_reason(&InquiryError::MissingStructuredData), + CancellationReason::BackendError + ); + assert_eq!( + ToolCoordinator::cancellation_reason(&InquiryError::AnswerExtraction { + reason: "nope".into() + }), + CancellationReason::BackendError + ); + assert_eq!( + ToolCoordinator::cancellation_reason(&InquiryError::Other("boom".into())), + CancellationReason::BackendError + ); +} + +#[test] +fn test_prompt_cancellation_reason_mapping() { + // Esc/Ctrl-C/EOF at the prompt is a user cancellation; any other prompt + // failure (I/O, no TTY, writer errors) is a backend error. + assert_eq!( + ToolCoordinator::prompt_cancellation_reason(&Error::Inquire( + InquireError::OperationCanceled + )), + CancellationReason::User + ); + assert_eq!( + ToolCoordinator::prompt_cancellation_reason(&Error::Inquire( + InquireError::OperationInterrupted + )), + CancellationReason::User + ); + assert_eq!( + ToolCoordinator::prompt_cancellation_reason(&Error::Inquire(InquireError::NotTTY)), + CancellationReason::BackendError + ); + assert_eq!( + ToolCoordinator::prompt_cancellation_reason(&Error::Fmt(std::fmt::Error)), + CancellationReason::BackendError + ); +} + #[test] fn test_pending_prompt_question_variant() { let pending = PendingPrompt::Question { index: 0, - question: jp_tool::Question::text("q1", "Test question"), + question: jp_tool::Question::text("q1", "Test question").unwrap(), + inquiry_id: InquiryId::new("iq"), }; // Verify we can match and extract fields - let PendingPrompt::Question { index, question: q } = pending else { + let PendingPrompt::Question { + index, question: q, .. + } = pending + else { panic!("Expected Question variant"); }; assert_eq!(index, 0); @@ -629,7 +678,8 @@ fn test_pending_prompt_queue_fifo_order() { // Add a question prompt queue.push_back(PendingPrompt::Question { index: 0, - question: jp_tool::Question::text("q1", "First"), + question: jp_tool::Question::text("q1", "First").unwrap(), + inquiry_id: InquiryId::new("iq"), }); // Add a result mode prompt @@ -647,14 +697,18 @@ fn test_pending_prompt_queue_fifo_order() { // Add another question prompt queue.push_back(PendingPrompt::Question { index: 2, - question: jp_tool::Question::boolean("q2", "Third"), + question: jp_tool::Question::boolean("q2", "Third").unwrap(), + inquiry_id: InquiryId::new("iq"), }); // Verify FIFO order assert_eq!(queue.len(), 3); // First: Question at index 0 - let PendingPrompt::Question { index, question } = queue.pop_front().unwrap() else { + let PendingPrompt::Question { + index, question, .. + } = queue.pop_front().unwrap() + else { panic!("Expected Question"); }; assert_eq!(index, 0); @@ -668,7 +722,10 @@ fn test_pending_prompt_queue_fifo_order() { assert_eq!(tool_id, "call_1"); // Third: Question at index 2 - let PendingPrompt::Question { index, question } = queue.pop_front().unwrap() else { + let PendingPrompt::Question { + index, question, .. + } = queue.pop_front().unwrap() + else { panic!("Expected Question"); }; assert_eq!(index, 2); @@ -686,7 +743,8 @@ fn test_pending_prompt_mixed_types_interleaved() { // Simulate: while prompt_active, queue these in arrival order queue.push_back(PendingPrompt::Question { index: 0, - question: jp_tool::Question::text("branch", "Which branch?"), + question: jp_tool::Question::text("branch", "Which branch?").unwrap(), + inquiry_id: InquiryId::new("iq"), }); queue.push_back(PendingPrompt::ResultMode { @@ -703,7 +761,9 @@ fn test_pending_prompt_mixed_types_interleaved() { queue.push_back(PendingPrompt::Question { index: 2, question: jp_tool::Question::boolean("confirm", "Confirm action?") + .unwrap() .with_default(serde_json::json!(true)), + inquiry_id: InquiryId::new("iq"), }); // All three should be queued diff --git a/crates/jp_cli/src/cmd/query/tool/executor.rs b/crates/jp_cli/src/cmd/query/tool/executor.rs index 4e1ade44..e9c438d9 100644 --- a/crates/jp_cli/src/cmd/query/tool/executor.rs +++ b/crates/jp_cli/src/cmd/query/tool/executor.rs @@ -47,8 +47,8 @@ use std::sync::Arc; use async_trait::async_trait; use camino::Utf8Path; use indexmap::IndexMap; -use jp_config::conversation::tool::{RunMode, ToolConfigWithDefaults}; -use jp_conversation::event::{ToolCallRequest, ToolCallResponse}; +use jp_config::conversation::tool::{RunMode, ToolConfigWithDefaults, ToolSource}; +use jp_conversation::event::{InquirySource, ToolCallRequest, ToolCallResponse}; use jp_llm::{ ExecutionOutcome, tool::{ @@ -151,6 +151,26 @@ impl ToolExecutor { invocation, } } + + /// Resolve the persisted `InquirySource` recorded for a question this tool + /// emits. + /// + /// Built-in tools may override their source via + /// `BuiltinTool::inquiry_source`; local and MCP tools always attribute the + /// question to the tool by name. + fn inquiry_source(&self) -> InquirySource { + match self.config.source() { + ToolSource::Builtin { .. } => { + self.builtin_executors.get(&self.request.name).map_or_else( + || InquirySource::tool(self.request.name.as_str()), + |tool| tool.inquiry_source(&self.request.name), + ) + } + ToolSource::Local { .. } | ToolSource::Mcp { .. } => { + InquirySource::tool(self.request.name.as_str()) + } + } + } } #[async_trait] @@ -243,6 +263,7 @@ impl Executor for ToolExecutor { tool_id: self.request.id.clone(), tool_name: self.request.name.clone(), question, + source: self.inquiry_source(), accumulated_answers: answers.clone(), }, Err(e) => ExecutorResult::Completed(ToolCallResponse { diff --git a/crates/jp_cli/src/cmd/query/tool/inquiry.rs b/crates/jp_cli/src/cmd/query/tool/inquiry.rs index 560e1f4f..f3a1a4b2 100644 --- a/crates/jp_cli/src/cmd/query/tool/inquiry.rs +++ b/crates/jp_cli/src/cmd/query/tool/inquiry.rs @@ -40,14 +40,15 @@ use serde_json::{Map, Value, json}; use tokio_util::sync::CancellationToken; use tracing::info; -/// Create an inquiry ID for a tool call question. +/// Create the stream-correlation inquiry ID for a tool-call question. /// -/// Format: `.` — unique per question within a tool -/// call. +/// Format: `..`, unique per attempt within +/// the turn (`attempt` is the 1-indexed per-`(tool_call_id, question_id)` +/// counter from `TurnState`). /// The tool name is not included because it's already stored in the /// `InquirySource` of the `InquiryRequest` event. -pub fn tool_call_inquiry_id(tool_call_id: &str, question_id: &str) -> String { - format!("{tool_call_id}.{question_id}") +pub fn tool_call_inquiry_id(tool_call_id: &str, question_id: &str, attempt: usize) -> String { + format!("{tool_call_id}.{question_id}.{attempt}") } /// Create a JSON schema for a structured inquiry. @@ -62,7 +63,7 @@ pub fn create_inquiry_schema(question: &Question) -> Map { "type": "string", "enum": options }), - AnswerType::Text => json!({ + AnswerType::Text | AnswerType::Secret => json!({ "type": "string" }), }; @@ -206,7 +207,7 @@ impl InquiryBackend for LlmInquiryBackend { question: &Question, cancellation_token: CancellationToken, ) -> Result { - let config = self.config_for(tool_name, &question.id); + let config = self.config_for(tool_name, question.id.as_str()); info!( inquiry_id, @@ -332,12 +333,16 @@ impl InquiryBackend for LlmInquiryBackend { "Structured inquiry completed", ); + // A literal `null` answer is rejected here rather than recorded: it is + // never a meaningful answer, and `InquiryResponse::answered` refuses + // null values. structured_data .get_mut("answer") .map(Value::take) + .filter(|answer| !answer.is_null()) .ok_or_else(|| { let reason = format!( - "missing 'answer' field in structured response: {}", + "missing or null 'answer' field in structured response: {}", serde_json::to_string(&structured_data) .unwrap_or_else(|_| "".into()) ); 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..c7014612 100644 --- a/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs @@ -43,7 +43,7 @@ fn test_inquiry_config(provider: MockProvider) -> InquiryConfig { } fn test_question() -> Question { - Question::boolean("confirm", "Create backup?") + Question::boolean("confirm", "Create backup?").unwrap() } fn test_events() -> ConversationStream { @@ -53,21 +53,21 @@ fn test_events() -> ConversationStream { #[test] fn test_tool_call_inquiry_id() { assert_eq!( - tool_call_inquiry_id("call_abc123", "apply_changes"), - "call_abc123.apply_changes" + tool_call_inquiry_id("call_abc123", "apply_changes", 1), + "call_abc123.apply_changes.1" ); } #[test] fn test_tool_call_inquiry_id_unique_per_question() { - let id_a = tool_call_inquiry_id("call_1", "confirm"); - let id_b = tool_call_inquiry_id("call_1", "reason"); + let id_a = tool_call_inquiry_id("call_1", "confirm", 1); + let id_b = tool_call_inquiry_id("call_1", "reason", 1); assert_ne!(id_a, id_b); } #[test] fn test_create_inquiry_schema_boolean() { - let question = Question::boolean("q1", "Confirm?"); + let question = Question::boolean("q1", "Confirm?").unwrap(); let schema = create_inquiry_schema(&question); @@ -87,11 +87,9 @@ fn test_create_inquiry_schema_boolean() { #[test] fn test_create_inquiry_schema_select() { - let question = Question::select("q2", "Choose one").with_options(vec![ - "A".to_string(), - "B".to_string(), - "C".to_string(), - ]); + let question = Question::select("q2", "Choose one") + .unwrap() + .with_options(vec!["A".to_string(), "B".to_string(), "C".to_string()]); let schema = create_inquiry_schema(&question); let props = schema.get("properties").and_then(Value::as_object).unwrap(); @@ -107,7 +105,7 @@ fn test_create_inquiry_schema_select() { #[test] fn test_create_inquiry_schema_text() { - let question = Question::text("q3", "Enter text"); + let question = Question::text("q3", "Enter text").unwrap(); let schema = create_inquiry_schema(&question); let props = schema.get("properties").and_then(Value::as_object).unwrap(); @@ -122,7 +120,7 @@ fn test_create_inquiry_schema_text() { #[test] fn test_create_inquiry_schema_stable_across_ids() { - let question = Question::boolean("q1", "Confirm?"); + let question = Question::boolean("q1", "Confirm?").unwrap(); let schema_a = create_inquiry_schema(&question); let schema_b = create_inquiry_schema(&question); @@ -131,7 +129,7 @@ fn test_create_inquiry_schema_stable_across_ids() { #[tokio::test] async fn llm_backend_returns_answer() { - let inquiry_id = tool_call_inquiry_id("call_abc", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_abc", "confirm", 1); let config = InquiryConfig { system_prompt: Some("You are a helpful assistant.".to_string()), ..test_inquiry_config(structured_provider(json!({ "answer": true }))) @@ -154,7 +152,7 @@ async fn llm_backend_returns_answer() { #[tokio::test] async fn llm_backend_returns_error_on_missing_structured_data() { - let inquiry_id = tool_call_inquiry_id("call_1", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); let config = test_inquiry_config(MockProvider::with_message("I don't know")); let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); @@ -173,7 +171,7 @@ async fn llm_backend_returns_error_on_missing_structured_data() { #[tokio::test] async fn llm_backend_returns_error_on_answer_extraction_failure() { - let inquiry_id = tool_call_inquiry_id("call_1", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); let config = test_inquiry_config(structured_provider(json!({ "unrelated": true }))); let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); @@ -190,11 +188,32 @@ async fn llm_backend_returns_error_on_answer_extraction_failure() { assert!(matches!(result, Err(InquiryError::AnswerExtraction { .. }))); } +#[tokio::test] +async fn llm_backend_returns_error_on_null_answer() { + // A provider returning `{"answer": null}` must fail extraction rather + // than produce an `Answered { answer: Null }` record. + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); + let config = test_inquiry_config(structured_provider(json!({ "answer": null }))); + let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); + + let result = backend + .inquire( + test_events(), + &inquiry_id, + "test_tool", + &test_question(), + CancellationToken::new(), + ) + .await; + + assert!(matches!(result, Err(InquiryError::AnswerExtraction { .. }))); +} + #[tokio::test] async fn llm_backend_returns_cancelled_when_token_is_already_cancelled() { let config = test_inquiry_config(structured_provider(json!({ "answer": true }))); let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); - let inquiry_id = tool_call_inquiry_id("call_1", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); let token = CancellationToken::new(); token.cancel(); @@ -214,9 +233,10 @@ async fn llm_backend_returns_cancelled_when_token_is_already_cancelled() { #[tokio::test] async fn llm_backend_passes_select_question() { - let inquiry_id = tool_call_inquiry_id("call_sel", "choose"); - let question = - Question::select("choose", "Pick one").with_options(vec!["A".to_string(), "B".to_string()]); + let inquiry_id = tool_call_inquiry_id("call_sel", "choose", 1); + let question = Question::select("choose", "Pick one") + .unwrap() + .with_options(vec!["A".to_string(), "B".to_string()]); let config = test_inquiry_config(structured_provider(json!({ "answer": "B" }))); let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); @@ -235,8 +255,8 @@ async fn llm_backend_passes_select_question() { #[tokio::test] async fn llm_backend_passes_text_question() { - let inquiry_id = tool_call_inquiry_id("call_txt", "reason"); - let question = Question::text("reason", "Why?"); + let inquiry_id = tool_call_inquiry_id("call_txt", "reason", 1); + let question = Question::text("reason", "Why?").unwrap(); let config = test_inquiry_config(structured_provider(json!({ "answer": "Because reasons" }))); let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); @@ -255,7 +275,7 @@ async fn llm_backend_passes_text_question() { #[tokio::test] async fn mock_backend_returns_configured_answer() { - let inquiry_id = tool_call_inquiry_id("call_1", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); let backend = MockInquiryBackend::new(HashMap::from([(inquiry_id.clone(), json!(true))])); let result = backend @@ -290,7 +310,7 @@ async fn mock_backend_returns_error_for_unknown_inquiry() { #[tokio::test] async fn mock_backend_ignores_cancellation_token() { - let inquiry_id = tool_call_inquiry_id("call_1", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); let backend = MockInquiryBackend::new(HashMap::from([(inquiry_id.clone(), json!(42))])); // Even with a cancelled token, mock returns immediately. @@ -312,7 +332,7 @@ async fn mock_backend_ignores_cancellation_token() { #[tokio::test] async fn llm_backend_uses_per_question_override() { - let inquiry_id = tool_call_inquiry_id("call_1", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_1", "confirm", 1); let default_config = test_inquiry_config( // Default provider returns wrong data (would fail extraction). structured_provider(json!({ "unrelated": true })), @@ -548,7 +568,7 @@ fn truncate_triggers_with_overhead() { #[tokio::test] async fn dedicated_model_backend_returns_answer() { - let inquiry_id = tool_call_inquiry_id("call_dedicated", "confirm"); + let inquiry_id = tool_call_inquiry_id("call_dedicated", "confirm", 1); let config = InquiryConfig { provider: Arc::new(structured_provider(json!({ "answer": true }))), model: ModelDetails::empty(ModelIdConfig { diff --git a/crates/jp_cli/src/cmd/query/tool/prompter.rs b/crates/jp_cli/src/cmd/query/tool/prompter.rs index f825d4b2..26ad9e7b 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter.rs @@ -14,7 +14,7 @@ use std::{io::Write as _, sync::Arc}; use crossterm::style::Stylize as _; use jp_config::conversation::tool::{RunMode, ToolSource}; -use jp_conversation::event::{InquiryId, SelectOption}; +use jp_conversation::event::SelectOption; use jp_editor::{EditOutcome, EditorBackend}; use jp_inquire::{InlineOption, ReplyEditMode, ReplyOutcome, prompt::PromptBackend}; use jp_llm::tool::executor::PermissionInfo; @@ -24,27 +24,6 @@ use serde_json::Value; use crate::{Error, editor::report_editor_failure}; -/// Well-known question ID used for tool permission inquiries. -pub const PERMISSION_QUESTION_ID: &str = "__permission__"; - -/// Constructs a stable `InquiryId` for a tool's permission prompt. -/// -/// Format: `".__permission__"`. -/// This is stable across different invocations of the same tool within a turn. -#[must_use] -pub fn permission_inquiry_id(tool_name: &str) -> InquiryId { - InquiryId::new(format!("{tool_name}.{PERMISSION_QUESTION_ID}")) -} - -/// Constructs a stable `InquiryId` for a tool question. -/// -/// Format: `"."`. -/// This is stable across different invocations of the same tool within a turn. -#[must_use] -pub fn tool_question_inquiry_id(tool_name: &str, question_id: &str) -> InquiryId { - InquiryId::new(format!("{tool_name}.{question_id}")) -} - /// Result of a permission prompt. #[derive(Debug)] pub enum PermissionResult { @@ -503,6 +482,14 @@ impl ToolPrompter { .prompt_backend .text(&question.text, default_str, &mut writer)?; + Ok(QuestionResult { + answer: Value::String(answer), + persist_level: jp_tool::PersistLevel::None, + }) + } + AnswerType::Secret => { + let answer = self.prompt_backend.password(&question.text, &mut writer)?; + Ok(QuestionResult { answer: Value::String(answer), persist_level: jp_tool::PersistLevel::None, diff --git a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs index 0573a360..74051e7b 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs @@ -418,7 +418,7 @@ fn result_confirmation_cancelled_returns_false() { #[test] fn question_boolean_uses_inline_select() { let prompt = MockPromptBackend::new().with_inline_responses(['y']); - let question = jp_tool::Question::boolean("q1", "Proceed?"); + let question = jp_tool::Question::boolean("q1", "Proceed?").unwrap(); let result = prompter(prompt).prompt_question(&question).unwrap(); assert_eq!(result.answer, Value::Bool(true)); assert_eq!(result.persist_level, jp_tool::PersistLevel::None); @@ -427,7 +427,7 @@ fn question_boolean_uses_inline_select() { #[test] fn question_text_uses_backend() { let prompt = MockPromptBackend::new().with_text_responses(["user input"]); - let question = jp_tool::Question::text("q2", "Input:"); + let question = jp_tool::Question::text("q2", "Input:").unwrap(); let result = prompter(prompt).prompt_question(&question).unwrap(); assert_eq!(result.answer, Value::String("user input".to_string())); } @@ -436,6 +436,7 @@ fn question_text_uses_backend() { fn question_select_uses_backend() { let prompt = MockPromptBackend::new().with_select_responses(["Option B"]); let question = jp_tool::Question::select("q3", "Choose:") + .unwrap() .with_options(vec!["Option A".to_string(), "Option B".to_string()]); let result = prompter(prompt).prompt_question(&question).unwrap(); assert_eq!(result.answer, Value::String("Option B".to_string())); diff --git a/crates/jp_cli/src/cmd/query/turn/state.rs b/crates/jp_cli/src/cmd/query/turn/state.rs index 99b59427..62a1101a 100644 --- a/crates/jp_cli/src/cmd/query/turn/state.rs +++ b/crates/jp_cli/src/cmd/query/turn/state.rs @@ -3,7 +3,46 @@ //! See [`TurnState`] for more details. use indexmap::IndexMap; -use jp_conversation::event::{InquiryId, InquiryResponse}; +use serde_json::Value; + +/// The reserved question-id segment used for a tool's permission prompt. +const PERMISSION_QUESTION_ID: &str = "__permission__"; + +/// Turn-cache key for a tool's remembered permission decision. +/// +/// Format `".__permission__"`, stable across invocations of the same +/// tool within a turn. +/// Deliberately a distinct type from the stream-correlation inquiry ID, which +/// identifies a persisted inquiry uniquely per attempt; the two must never be +/// used interchangeably. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct PermissionCacheKey(String); + +impl PermissionCacheKey { + /// Builds the permission-decision key for `tool_name`. + #[must_use] + pub fn new(tool_name: &str) -> Self { + Self(format!("{tool_name}.{PERMISSION_QUESTION_ID}")) + } +} + +/// Turn-cache key for a remembered tool-question answer. +/// +/// Format `"."`, stable across invocations of the same +/// tool within a turn. +/// Deliberately a distinct type from the stream-correlation inquiry ID, which +/// identifies a persisted inquiry uniquely per attempt; the two must never be +/// used interchangeably. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ToolAnswerCacheKey(String); + +impl ToolAnswerCacheKey { + /// Builds the answer key for `tool_name`'s `question_id`. + #[must_use] + pub fn new(tool_name: &str, question_id: &str) -> Self { + Self(format!("{tool_name}.{question_id}")) + } +} /// State that is persisted for the duration of a turn. /// @@ -23,12 +62,18 @@ use jp_conversation::event::{InquiryId, InquiryResponse}; /// `ToolCallRequest`. #[derive(Debug, Default)] pub struct TurnState { - /// Inquiry responses remembered for the duration of the turn. + /// Tool-question answers remembered for the duration of the turn. /// - /// The caller mints the [`InquiryId`] and decides the convention. - /// For tool permission prompts this is `".__permission__"`, for - /// tool questions it's `"."`. - pub persisted_inquiry_responses: IndexMap, + /// Written when a user answers a tool question with "remember for this + /// turn", and read to auto-answer the same question on a later tool call + /// within the turn. + pub remembered_tool_answers: IndexMap, + + /// Tool-permission decisions remembered for the duration of the turn. + /// + /// Gated by the permission prompt's `persist` flag. + /// `true` runs the tool without prompting again; `false` skips it. + pub remembered_permission_decisions: IndexMap, /// The number of times we've tried a request to the assistant. /// @@ -36,4 +81,30 @@ pub struct TurnState { /// Every retry increments this counter, until a maximum number of retries /// is reached, after which the turn ends in an error. pub request_count: usize, + + /// Per-`(tool_call_id, question_id)` attempt counter for minting unique + /// three-segment inquiry IDs within the turn. + /// + /// In-memory only; a fresh `TurnState` (built per turn) resets it. + pub inquiry_attempts: IndexMap<(String, String), usize>, } + +impl TurnState { + /// Allocate the next 1-indexed attempt for a `(tool_call_id, question_id)` + /// pair within this turn. + /// + /// The first call for a key returns `1`; each subsequent call for the same + /// key returns the next integer. + pub fn next_inquiry_attempt(&mut self, tool_call_id: &str, question_id: &str) -> usize { + let attempt = self + .inquiry_attempts + .entry((tool_call_id.to_owned(), question_id.to_owned())) + .or_insert(0); + *attempt += 1; + *attempt + } +} + +#[cfg(test)] +#[path = "state_tests.rs"] +mod tests; diff --git a/crates/jp_cli/src/cmd/query/turn/state_tests.rs b/crates/jp_cli/src/cmd/query/turn/state_tests.rs new file mode 100644 index 00000000..9552aedc --- /dev/null +++ b/crates/jp_cli/src/cmd/query/turn/state_tests.rs @@ -0,0 +1,23 @@ +use super::*; + +#[test] +fn next_inquiry_attempt_increments_per_key_and_resets_per_turn() { + let mut state = TurnState::default(); + + // The first recording for a key is attempt 1; the next for the same key is + // 2 (a re-ask after an invalid answer, or a reused tool_call_id cycle). + assert_eq!(state.next_inquiry_attempt("call_1", "confirm"), 1); + assert_eq!(state.next_inquiry_attempt("call_1", "confirm"), 2); + + // A different question or tool call counts independently. + assert_eq!(state.next_inquiry_attempt("call_1", "reason"), 1); + assert_eq!(state.next_inquiry_attempt("call_2", "confirm"), 1); + + // Continuing the first key picks up where it left off rather than + // restarting (cross-cycle uniqueness within the turn). + assert_eq!(state.next_inquiry_attempt("call_1", "confirm"), 3); + + // A fresh TurnState (built per turn) resets every counter. + let mut next_turn = TurnState::default(); + assert_eq!(next_turn.next_inquiry_attempt("call_1", "confirm"), 1); +} diff --git a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs index af3215ea..fdd24d8d 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -22,7 +22,10 @@ use jp_config::{ }; use jp_conversation::{ Conversation, - event::{ChatRequest, ChatResponse, InquirySource, ToolCallRequest, TurnStart}, + event::{ + CancellationReason, ChatRequest, ChatResponse, InquiryResponse, InquirySource, + ToolCallRequest, TurnStart, + }, }; use jp_inquire::prompt::MockPromptBackend; use jp_llm::{ @@ -3229,11 +3232,12 @@ impl Executor for InquiryMockExecutor { _cancellation_token: tokio_util::sync::CancellationToken, ) -> ExecutorResult { for q in &self.questions { - if !answers.contains_key(&q.id) { + if !answers.contains_key(q.id.as_str()) { return ExecutorResult::NeedsInput { tool_id: self.tool_id.clone(), tool_name: self.tool_name.clone(), question: q.clone(), + source: InquirySource::tool(self.tool_name.clone()), accumulated_answers: answers.clone(), }; } @@ -3380,7 +3384,7 @@ async fn test_tool_with_single_inquiry() { Box::new(InquiryMockExecutor::new( &req.id, &req.name, - vec![Question::boolean("confirm", "Create backup?")], + vec![Question::boolean("confirm", "Create backup?").unwrap()], "inquiry tool output", )) }); @@ -3452,7 +3456,652 @@ async fn test_tool_with_single_inquiry() { .filter_map(|e| e.event.into_inquiry_response()) .collect(); assert_eq!(res.len(), 1, "Should have one inquiry response"); - assert_eq!(res[0].answer, json!(true)); + assert_eq!(res[0].answer(), Some(&json!(true))); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A `Secret` question with no interactive terminal fails the tool and records +/// `Cancelled(no_prompt_backend)` (RFD 082 routing guard). +#[tokio::test] +async fn test_secret_question_without_tty_fails_tool() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + // Register the tool without a question config: the question targets + // the user by default, and without a TTY it would fall back to the + // inquiry backend — which the secret guard refuses. + config + .conversation + .tools + .insert("secret_tool".to_string(), inquiry_tool_config(&[])); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::new(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Use the tool"); + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + single_tool_call_events("call_sec", "secret_tool"), + final_message_events("Understood."), + ], + call_index: AtomicUsize::new(0), + model: inquiry_mock_model(), + }); + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let (_signal_tx, signal_rx) = broadcast::channel(16); + + let executor_source = TestExecutorSource::new().with_executor("secret_tool", |req| { + Box::new(InquiryMockExecutor::new( + &req.id, + &req.name, + vec![Question::secret("passphrase", "Enter passphrase").unwrap()], + "secret tool output", + )) + }); + let tool_defs = executor_source.tool_definitions(); + + let result = run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &signal_rx, + &mcp_client, + root, + false, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request, + InvocationContext::default(), + ) + .await; + + assert!(result.is_ok(), "Turn loop should complete: {result:?}"); + + let events = lock.events().clone(); + + // The tool fails with a tool-level error. + let tool_responses: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_tool_call_response()) + .collect(); + assert_eq!(tool_responses.len(), 1); + let error = tool_responses[0].result.as_ref().unwrap_err(); + assert!(error.contains("secret value"), "unexpected error: {error}"); + + // The recorded inquiry pair closes as Cancelled(no_prompt_backend). + let req: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_inquiry_request()) + .collect(); + assert_eq!(req.len(), 1, "Should have one inquiry request"); + let res: Vec<_> = events + .into_iter() + .filter_map(|e| e.event.into_inquiry_response()) + .collect(); + assert_eq!(res.len(), 1, "Should have one inquiry response"); + assert!(matches!(&res[0], InquiryResponse::Cancelled { + reason: CancellationReason::NoPromptBackend, + .. + })); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A `Secret` question whose target is the assistant is refused and records +/// `Cancelled(assistant_routing_denied)` (RFD 082 routing guard). +#[tokio::test] +async fn test_secret_question_with_assistant_target_fails_tool() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + // Route the secret question to the assistant — the guard refuses. + config.conversation.tools.insert( + "secret_tool".to_string(), + inquiry_tool_config(&["passphrase"]), + ); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::new(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Use the tool"); + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + single_tool_call_events("call_sec", "secret_tool"), + final_message_events("Understood."), + ], + call_index: AtomicUsize::new(0), + model: inquiry_mock_model(), + }); + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let (_signal_tx, signal_rx) = broadcast::channel(16); + + let executor_source = TestExecutorSource::new().with_executor("secret_tool", |req| { + Box::new(InquiryMockExecutor::new( + &req.id, + &req.name, + vec![Question::secret("passphrase", "Enter passphrase").unwrap()], + "secret tool output", + )) + }); + let tool_defs = executor_source.tool_definitions(); + + let result = run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &signal_rx, + &mcp_client, + root, + false, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request, + InvocationContext::default(), + ) + .await; + + assert!(result.is_ok(), "Turn loop should complete: {result:?}"); + + let events = lock.events().clone(); + + let tool_responses: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_tool_call_response()) + .collect(); + assert_eq!(tool_responses.len(), 1); + assert!(tool_responses[0].result.is_err()); + + let res: Vec<_> = events + .into_iter() + .filter_map(|e| e.event.into_inquiry_response()) + .collect(); + assert_eq!(res.len(), 1, "Should have one inquiry response"); + assert!(matches!(&res[0], InquiryResponse::Cancelled { + reason: CancellationReason::AssistantRoutingDenied, + .. + })); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A `Secret` question answered at the prompter delivers the answer to the tool +/// in-memory while the persisted response is `Redacted` (RFD 082). +#[tokio::test] +async fn test_secret_prompter_answer_is_redacted() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + // Register the tool without a question config: the question targets + // the user and the TTY prompter answers it via the no-echo password + // path. + config + .conversation + .tools + .insert("secret_tool".to_string(), inquiry_tool_config(&[])); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::new(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Use the tool"); + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + single_tool_call_events("call_sec", "secret_tool"), + final_message_events("Secret used."), + ], + call_index: AtomicUsize::new(0), + model: inquiry_mock_model(), + }); + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let (_signal_tx, signal_rx) = broadcast::channel(16); + + let executor_source = TestExecutorSource::new().with_executor("secret_tool", |req| { + Box::new(InquiryMockExecutor::new( + &req.id, + &req.name, + vec![Question::secret("passphrase", "Enter passphrase").unwrap()], + "secret tool output", + )) + }); + let tool_defs = executor_source.tool_definitions(); + + let result = run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &signal_rx, + &mcp_client, + root, + true, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new().with_password_responses(["s3cret"])), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request, + InvocationContext::default(), + ) + .await; + + assert!(result.is_ok(), "Turn loop should complete: {result:?}"); + + let events = lock.events().clone(); + + // The answer reached the tool in-memory: it completed successfully. + let tool_responses: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_tool_call_response()) + .collect(); + assert_eq!(tool_responses.len(), 1); + assert_eq!(tool_responses[0].content(), "secret tool output"); + + // The persisted response is redacted and carries no answer value. + let res: Vec<_> = events + .into_iter() + .filter_map(|e| e.event.into_inquiry_response()) + .collect(); + assert_eq!(res.len(), 1, "Should have one inquiry response"); + assert!(matches!(&res[0], InquiryResponse::Redacted { .. })); + assert_eq!(res[0].answer(), None); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A `Secret` question with a configured static answer delivers the value to +/// the tool in-memory while recording `Redacted` instead of `Answered`. +#[tokio::test] +async fn test_secret_static_answer_is_redacted() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + let mut tool_config = inquiry_tool_config(&[]); + tool_config + .questions + .insert("passphrase".to_string(), QuestionConfig { + target: QuestionTarget::User, + answer: Some(json!("s3cret")), + }); + config + .conversation + .tools + .insert("secret_tool".to_string(), tool_config); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::new(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Use the tool"); + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + single_tool_call_events("call_sec", "secret_tool"), + final_message_events("Secret used."), + ], + call_index: AtomicUsize::new(0), + model: inquiry_mock_model(), + }); + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let (_signal_tx, signal_rx) = broadcast::channel(16); + + let executor_source = TestExecutorSource::new().with_executor("secret_tool", |req| { + Box::new(InquiryMockExecutor::new( + &req.id, + &req.name, + vec![Question::secret("passphrase", "Enter passphrase").unwrap()], + "secret tool output", + )) + }); + let tool_defs = executor_source.tool_definitions(); + + let result = run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &signal_rx, + &mcp_client, + root, + false, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request, + InvocationContext::default(), + ) + .await; + + assert!(result.is_ok(), "Turn loop should complete: {result:?}"); + + let events = lock.events().clone(); + + // The static answer reached the tool in-memory. + let tool_responses: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_tool_call_response()) + .collect(); + assert_eq!(tool_responses.len(), 1); + assert_eq!(tool_responses[0].content(), "secret tool output"); + + // The persisted response is redacted, not `Answered` with the value. + let res: Vec<_> = events + .into_iter() + .filter_map(|e| e.event.into_inquiry_response()) + .collect(); + assert_eq!(res.len(), 1, "Should have one inquiry response"); + assert!(matches!(&res[0], InquiryResponse::Redacted { .. })); + assert_eq!(res[0].answer(), None); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A non-secret question with a configured static answer records a full +/// `InquiryRequest`/`InquiryResponse::Answered` pair carrying the configured +/// value (RFD 082: static answers are recorded, not pre-seeded). +#[tokio::test] +async fn test_static_answer_records_answered_inquiry() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + let mut tool_config = inquiry_tool_config(&[]); + tool_config + .questions + .insert("confirm".to_string(), QuestionConfig { + target: QuestionTarget::User, + answer: Some(json!(true)), + }); + config + .conversation + .tools + .insert("static_tool".to_string(), tool_config); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::new(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Use the tool"); + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + single_tool_call_events("call_static", "static_tool"), + final_message_events("Done."), + ], + call_index: AtomicUsize::new(0), + model: inquiry_mock_model(), + }); + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let (_signal_tx, signal_rx) = broadcast::channel(16); + + let executor_source = TestExecutorSource::new().with_executor("static_tool", |req| { + Box::new(InquiryMockExecutor::new( + &req.id, + &req.name, + vec![Question::boolean("confirm", "Proceed?").unwrap()], + "static tool output", + )) + }); + let tool_defs = executor_source.tool_definitions(); + + let result = run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &signal_rx, + &mcp_client, + root, + false, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request, + InvocationContext::default(), + ) + .await; + + assert!(result.is_ok(), "Turn loop should complete: {result:?}"); + + let events = lock.events().clone(); + + // The static answer reached the tool and it completed successfully. + let tool_responses: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_tool_call_response()) + .collect(); + assert_eq!(tool_responses.len(), 1); + assert_eq!(tool_responses[0].content(), "static tool output"); + + // The round-trip is recorded as a request/response pair, with the + // response carrying the configured value. + let req: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_inquiry_request()) + .collect(); + assert_eq!(req.len(), 1, "Should have one inquiry request"); + let res: Vec<_> = events + .into_iter() + .filter_map(|e| e.event.into_inquiry_response()) + .collect(); + assert_eq!(res.len(), 1, "Should have one inquiry response"); + assert!(matches!(&res[0], InquiryResponse::Answered { .. })); + assert_eq!(res[0].answer(), Some(&json!(true))); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A "remember for this turn" prompter answer is reused for a later tool call +/// in the same turn, and the cache hit still records a fresh +/// `InquiryRequest`/`InquiryResponse::Answered` pair (RFD 082). +#[tokio::test] +async fn test_remembered_answer_cache_hit_records_new_inquiry_pair() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + // No question config: the question targets the user and is answered + // at the interactive prompter. + config + .conversation + .tools + .insert("cached_tool".to_string(), inquiry_tool_config(&[])); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::new(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Use the tool twice"); + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + single_tool_call_events("call_a", "cached_tool"), + single_tool_call_events("call_b", "cached_tool"), + final_message_events("Done."), + ], + call_index: AtomicUsize::new(0), + model: inquiry_mock_model(), + }); + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let (_signal_tx, signal_rx) = broadcast::channel(16); + + let executor_source = TestExecutorSource::new().with_executor("cached_tool", |req| { + Box::new(InquiryMockExecutor::new( + &req.id, + &req.name, + vec![Question::boolean("confirm", "Proceed?").unwrap()], + "cached tool output", + )) + }); + let tool_defs = executor_source.tool_definitions(); + + // A single queued 'Y' ("yes, and remember for this turn"): the second + // call must be satisfied from the turn cache, because another prompt + // would find the queue empty and cancel. + let prompt_backend = Arc::new(MockPromptBackend::new().with_inline_responses(['Y'])); + + let result = run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &signal_rx, + &mcp_client, + root, + true, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + prompt_backend, + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request, + InvocationContext::default(), + ) + .await; + + assert!(result.is_ok(), "Turn loop should complete: {result:?}"); + + let events = lock.events().clone(); + + // Both tool calls completed with the answer. + let tool_responses: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_tool_call_response()) + .collect(); + assert_eq!(tool_responses.len(), 2); + assert_eq!(tool_responses[0].content(), "cached tool output"); + assert_eq!(tool_responses[1].content(), "cached tool output"); + + // Each round-trip records its own pair; the cache hit records + // `Answered`, not nothing. + let req: Vec<_> = events + .clone() + .into_iter() + .filter_map(|e| e.event.into_inquiry_request()) + .collect(); + assert_eq!(req.len(), 2, "Should have two inquiry requests"); + let res: Vec<_> = events + .into_iter() + .filter_map(|e| e.event.into_inquiry_response()) + .collect(); + assert_eq!(res.len(), 2, "Should have two inquiry responses"); + for r in &res { + assert!(matches!(r, InquiryResponse::Answered { .. })); + assert_eq!(r.answer(), Some(&json!(true))); + } })) .await; @@ -3515,8 +4164,8 @@ async fn test_tool_with_multiple_inquiries() { &req.id, &req.name, vec![ - Question::boolean("confirm", "Proceed?"), - Question::text("reason", "Why?"), + Question::boolean("confirm", "Proceed?").unwrap(), + Question::text("reason", "Why?").unwrap(), ], "both questions answered", )) @@ -3664,7 +4313,7 @@ async fn test_parallel_tools_one_with_inquiry() { Box::new(InquiryMockExecutor::new( &req.id, &req.name, - vec![Question::boolean("confirm", "Proceed?")], + vec![Question::boolean("confirm", "Proceed?").unwrap()], "inquiry completed", )) }) @@ -3796,7 +4445,7 @@ async fn test_parallel_tools_both_with_inquiries() { Box::new(InquiryMockExecutor::new( &req.id, &req.name, - vec![Question::boolean("confirm_a", "Proceed A?")], + vec![Question::boolean("confirm_a", "Proceed A?").unwrap()], "tool_a done", )) }) @@ -3804,7 +4453,7 @@ async fn test_parallel_tools_both_with_inquiries() { Box::new(InquiryMockExecutor::new( &req.id, &req.name, - vec![Question::boolean("confirm_b", "Proceed B?")], + vec![Question::boolean("confirm_b", "Proceed B?").unwrap()], "tool_b done", )) }); @@ -4199,7 +4848,7 @@ async fn test_inquiry_failure_marks_tool_as_error() { Box::new(InquiryMockExecutor::new( &req.id, &req.name, - vec![Question::boolean("confirm", "Confirm?")], + vec![Question::boolean("confirm", "Confirm?").unwrap()], "should not reach this", )) }); diff --git a/crates/jp_cli/src/editor.rs b/crates/jp_cli/src/editor.rs index a03eee38..b0257460 100644 --- a/crates/jp_cli/src/editor.rs +++ b/crates/jp_cli/src/editor.rs @@ -14,7 +14,7 @@ use jp_config::{ }; use jp_conversation::{ ConversationStream, - event::{ChatResponse, EventKind}, + event::{ChatResponse, EventKind, InquiryResponse}, }; use jp_editor::{EditOutcome, EditRequest, EditorBackend, EditorError, TerminalEditorBackend}; use jp_printer::Printer; @@ -370,8 +370,7 @@ fn build_history_text(history: &ConversationStream) -> String { } EventKind::InquiryResponse(response) => { buf.push_str(&format!("\n\n## Inquiry Response on {timestamp}\n\n")); - buf.push_str("Answer: "); - buf.push_str(&response.answer.to_string()); + buf.push_str(&inquiry_answer_line(response)); } EventKind::TurnStart(_) => {} } @@ -384,6 +383,15 @@ fn build_history_text(history: &ConversationStream) -> String { text } +/// Render the answer line for an inquiry response in the history export. +fn inquiry_answer_line(response: &InquiryResponse) -> String { + match response { + InquiryResponse::Answered { answer, .. } => format!("Answer: {answer}"), + InquiryResponse::Redacted { .. } => "Answer: ".to_string(), + InquiryResponse::Cancelled { reason, .. } => format!("Cancelled ({})", reason.as_str()), + } +} + #[cfg(test)] #[path = "editor_tests.rs"] mod tests; diff --git a/crates/jp_cli/src/render/tool.rs b/crates/jp_cli/src/render/tool.rs index 7d42f53a..d904a084 100644 --- a/crates/jp_cli/src/render/tool.rs +++ b/crates/jp_cli/src/render/tool.rs @@ -720,6 +720,17 @@ async fn format_args_custom( )) } CommandResult::Cancelled => Ok(String::new()), + CommandResult::InvalidInquiry { question_id } => { + warn!( + command = %cmd, + question_id = %question_id, + "Custom parameters formatter returned an invalid inquiry" + ); + Err(format!( + "Custom parameters formatter '{cmd}' produced an invalid inquiry (question id \ + '{question_id}')" + )) + } CommandResult::RawOutput { stdout, success: true, diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index 519fe2da..089b023c 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -1203,8 +1203,12 @@ pub struct QuestionConfig { /// The fixed answer to the question. /// - /// If this is set, the question will not be presented to the target, but - /// will always be answered with the given value. + /// If set, the question is never presented to the target: when the tool + /// asks it, the configured value is supplied automatically (no prompt is + /// shown and no assistant round-trip happens) and the tool is re-invoked + /// with the answer. + /// The exchange is still recorded in the conversation as an inquiry + /// request/response pair. // TODO: We should add an enumeration of possible options: // // - Fixed answer diff --git a/crates/jp_conversation/src/event.rs b/crates/jp_conversation/src/event.rs index 4ed0ff35..9187cb93 100644 --- a/crates/jp_conversation/src/event.rs +++ b/crates/jp_conversation/src/event.rs @@ -15,8 +15,8 @@ use serde_json::{Map, Value}; pub use self::{ chat::{ChatRequest, ChatResponse}, inquiry::{ - InquiryAnswerType, InquiryId, InquiryQuestion, InquiryRequest, InquiryResponse, - InquirySource, SelectOption, + CancellationReason, InquiryAnswerType, InquiryId, InquiryQuestion, InquiryRequest, + InquiryResponse, InquirySource, SelectOption, }, tool_call::{ToolCallRequest, ToolCallResponse}, turn::TurnStart, diff --git a/crates/jp_conversation/src/event/inquiry.rs b/crates/jp_conversation/src/event/inquiry.rs index f0a288f8..b24d3c11 100644 --- a/crates/jp_conversation/src/event/inquiry.rs +++ b/crates/jp_conversation/src/event/inquiry.rs @@ -2,8 +2,8 @@ use std::fmt; -use serde::{Deserialize, Serialize}; -use serde_json::Value; +use serde::{Deserialize, Deserializer, Serialize, Serializer, de}; +use serde_json::{Map, Value}; /// Opaque identifier for an inquiry. #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -189,6 +189,9 @@ pub enum InquiryAnswerType { /// Free-form text input. Text, + + /// Free-form text input whose answer is not persisted on disk. + Secret, } /// A single option in a select inquiry. @@ -232,70 +235,239 @@ impl From<&str> for SelectOption { } } -/// An inquiry response event - the answer to an inquiry request. +/// The outcome of an [`InquiryRequest`]. /// /// This event MUST be in response to an `InquiryRequest` event, with a matching /// `id`. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct InquiryResponse { - /// ID matching the corresponding `InquiryRequest`. - pub id: InquiryId, +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(tag = "outcome", rename_all = "snake_case")] +pub enum InquiryResponse { + /// The inquiry was answered. + Answered { + /// ID matching the corresponding `InquiryRequest`. + id: InquiryId, + + /// The answer provided. + /// + /// The shape of this value depends on the `answer_type` of the + /// corresponding inquiry: + /// + /// - `Boolean`: `Value::Bool` + /// - `Select`: one of the option values + /// - `Text`: `Value::String` + answer: Value, + }, - /// The answer provided. - /// - /// The shape of this value depends on the `answer_type` of the - /// corresponding inquiry: + /// The inquiry was closed without an answer. + Cancelled { + /// ID matching the corresponding `InquiryRequest`. + id: InquiryId, + + /// Why the inquiry was closed without an answer. + reason: CancellationReason, + }, + + /// The inquiry was answered, but the answer was deliberately not persisted + /// (e.g. a secret value). /// - /// - `Boolean`: `Value::Bool` - /// - `Select`: one of the option values - /// - `Text`: `Value::String` - pub answer: Value, + /// The tool still received the answer in-memory; only the on-disk record + /// omits it. + Redacted { + /// ID matching the corresponding `InquiryRequest`. + id: InquiryId, + }, } impl InquiryResponse { - /// Creates a new inquiry response. + /// Creates an answered inquiry response. + /// + /// A null answer is never a meaningful answer for any answer type; record a + /// `Cancelled` response instead. #[must_use] - pub fn new(id: impl Into, answer: Value) -> Self { - Self { + pub fn answered(id: impl Into, answer: Value) -> Self { + debug_assert!( + !answer.is_null(), + "inquiry answers must not be null; record a cancellation instead" + ); + Self::Answered { id: id.into(), answer, } } - /// Creates a new boolean inquiry response. + /// Creates an answered boolean inquiry response. #[must_use] pub fn boolean(id: impl Into, answer: bool) -> Self { - Self::new(id, Value::Bool(answer)) + Self::answered(id, Value::Bool(answer)) } - /// Creates a new select inquiry response. + /// Creates an answered select inquiry response. #[must_use] pub fn select(id: impl Into, answer: impl Into) -> Self { - Self::new(id, answer.into()) + Self::answered(id, answer.into()) } - /// Creates a new text inquiry response. + /// Creates an answered text inquiry response. #[must_use] pub fn text(id: impl Into, answer: String) -> Self { - Self::new(id, Value::String(answer)) + Self::answered(id, Value::String(answer)) } - /// Returns the answer as a boolean, if applicable. + /// The inquiry ID, present on every variant. #[must_use] - pub fn as_bool(&self) -> Option { - self.answer.as_bool() + pub const fn id(&self) -> &InquiryId { + match self { + Self::Answered { id, .. } | Self::Cancelled { id, .. } | Self::Redacted { id } => id, + } } - /// Returns the answer as a string, if applicable. + /// The answer provided, if this response carries one. + /// + /// Returns `None` for `Cancelled` and `Redacted` responses. #[must_use] - pub fn as_str(&self) -> Option<&str> { - self.answer.as_str() + pub const fn answer(&self) -> Option<&Value> { + match self { + Self::Answered { answer, .. } => Some(answer), + Self::Cancelled { .. } | Self::Redacted { .. } => None, + } + } +} + +impl<'de> Deserialize<'de> for InquiryResponse { + /// Accepts both the tagged 082+ form and the legacy pre-082 flat form. + /// + /// The flat form (`{ "id", "answer" }`, no `outcome`) deserializes as + /// `Answered`. + /// A tagged `cancelled` event whose `reason` is missing or not a string + /// deserializes as `CancellationReason::Unknown` with the sentinel tag + /// `unspecified`: the record carries no usable reason, and no specific one + /// is fabricated for it. + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + // Deserialized through a raw map so a literal `null` answer stays + // distinguishable from an absent `answer` field: an `Option` + // field folds both into `None`, rejecting `Answered { answer: Null }` + // even though the serializer can produce it. + let mut raw = Map::deserialize(deserializer)?; + + let id = raw + .get("id") + .and_then(Value::as_str) + .map(InquiryId::new) + .ok_or_else(|| de::Error::missing_field("id"))?; + let outcome = raw + .get("outcome") + .and_then(Value::as_str) + .map(str::to_owned); + + match outcome.as_deref() { + Some("answered") => { + let answer = raw + .remove("answer") + .ok_or_else(|| de::Error::missing_field("answer"))?; + Ok(Self::Answered { id, answer }) + } + Some("cancelled") => { + let reason = raw.get("reason").and_then(Value::as_str).map_or_else( + || CancellationReason::Unknown("unspecified".to_owned()), + CancellationReason::from_tag, + ); + Ok(Self::Cancelled { id, reason }) + } + Some("redacted") => Ok(Self::Redacted { id }), + Some(other) => Err(de::Error::unknown_variant(other, &[ + "answered", + "cancelled", + "redacted", + ])), + // Legacy pre-082 flat form: `{ "id", "answer" }` is an answer. + None => { + let answer = raw.remove("answer").ok_or_else(|| { + de::Error::custom("inquiry response missing `outcome` and `answer`") + })?; + Ok(Self::Answered { id, answer }) + } + } } +} - /// Returns the answer as a string, if applicable. +/// Why an inquiry was closed without an answer. +/// +/// Recorded on [`InquiryResponse::Cancelled`] for the audit trail only; it does +/// not drive retry behavior. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CancellationReason { + /// The user explicitly cancelled (e.g. Ctrl-C at the prompt). + User, + + /// The routing backend (prompter or inquiry backend) returned an error + /// instead of an answer. + BackendError, + + /// A question that requires a human answer could not be routed because no + /// interactive terminal is available. + NoPromptBackend, + + /// A question that requires a human answer was targeted at the assistant + /// and refused to route to the inquiry backend. + AssistantRoutingDenied, + + /// A reason this build cannot interpret: a tag named by a newer JP, or a + /// `cancelled` event whose `reason` was missing or not a string (recorded + /// with the sentinel tag `unspecified`). + /// + /// The payload is the unparsed serde tag, preserved verbatim so it + /// round-trips unchanged. + /// Audit-trail only; readers MUST NOT branch on the contents. + Unknown(String), +} + +impl CancellationReason { + /// The serde tag for this reason. + /// + /// `Unknown` returns its preserved tag verbatim. #[must_use] - pub fn as_string(&self) -> Option { - self.answer.as_str().map(ToString::to_string) + pub fn as_str(&self) -> &str { + match self { + Self::User => "user", + Self::BackendError => "backend_error", + Self::NoPromptBackend => "no_prompt_backend", + Self::AssistantRoutingDenied => "assistant_routing_denied", + Self::Unknown(tag) => tag, + } + } + + /// Parses a serde tag into a reason, mapping unrecognized tags to + /// [`Self::Unknown`]. + fn from_tag(tag: &str) -> Self { + match tag { + "user" => Self::User, + "backend_error" => Self::BackendError, + "no_prompt_backend" => Self::NoPromptBackend, + "assistant_routing_denied" => Self::AssistantRoutingDenied, + other => Self::Unknown(other.to_owned()), + } + } +} + +impl Serialize for CancellationReason { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(self.as_str()) + } +} + +impl<'de> Deserialize<'de> for CancellationReason { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let tag = String::deserialize(deserializer)?; + Ok(Self::from_tag(&tag)) } } diff --git a/crates/jp_conversation/src/event/inquiry_tests.rs b/crates/jp_conversation/src/event/inquiry_tests.rs index d5970ec0..749e9555 100644 --- a/crates/jp_conversation/src/event/inquiry_tests.rs +++ b/crates/jp_conversation/src/event/inquiry_tests.rs @@ -1,3 +1,4 @@ +use serde_json::json; use test_log::test; use super::*; @@ -34,15 +35,138 @@ fn test_inquiry_request_serialization() { } #[test] -fn test_inquiry_response_serialization() { +fn test_inquiry_response_answered_serialization() { let response = InquiryResponse::boolean("test-id", true); let json = serde_json::to_value(&response).unwrap(); - assert_eq!(json["id"], "test-id"); - assert_eq!(json["answer"], true); + assert_eq!( + json, + json!({ "outcome": "answered", "id": "test-id", "answer": true }) + ); + + let deserialized: InquiryResponse = serde_json::from_value(json).unwrap(); + assert_eq!(deserialized, response); +} + +#[test] +fn test_inquiry_response_legacy_answered_deserialization() { + // Pre-082 events carry no `outcome` field. + let json = json!({ "id": "call_1.answer", "answer": true }); + let response: InquiryResponse = serde_json::from_value(json).unwrap(); + assert_eq!(response, InquiryResponse::boolean("call_1.answer", true)); +} + +#[test] +fn test_inquiry_response_null_answer_round_trips() { + // A literal `null` answer must stay distinguishable from an absent + // `answer` field: whatever the serializer produces, the deserializer + // accepts. + let response = InquiryResponse::Answered { + id: InquiryId::new("call_1.confirm.1"), + answer: Value::Null, + }; + + let json = serde_json::to_value(&response).unwrap(); + assert_eq!( + json, + json!({ "outcome": "answered", "id": "call_1.confirm.1", "answer": null }) + ); + + let deserialized: InquiryResponse = serde_json::from_value(json).unwrap(); + assert_eq!(deserialized, response); + + // The legacy flat form accepts a null answer the same way. + let legacy = json!({ "id": "call_1.confirm", "answer": null }); + let deserialized: InquiryResponse = serde_json::from_value(legacy).unwrap(); + assert_eq!(deserialized, InquiryResponse::Answered { + id: InquiryId::new("call_1.confirm"), + answer: Value::Null, + }); +} + +#[test] +fn test_inquiry_response_cancelled_serialization() { + let user = InquiryResponse::Cancelled { + id: InquiryId::new("call_1.confirm.1"), + reason: CancellationReason::User, + }; + assert_eq!( + serde_json::to_value(&user).unwrap(), + json!({ "outcome": "cancelled", "id": "call_1.confirm.1", "reason": "user" }) + ); + + let backend = InquiryResponse::Cancelled { + id: InquiryId::new("call_1.confirm.1"), + reason: CancellationReason::BackendError, + }; + assert_eq!( + serde_json::to_value(&backend).unwrap(), + json!({ "outcome": "cancelled", "id": "call_1.confirm.1", "reason": "backend_error" }) + ); +} + +#[test] +fn test_inquiry_response_cancelled_missing_reason_is_unknown() { + // A `cancelled` event without a usable `reason` carries no audit claim; + // it must not be fabricated into a specific reason like `User`. + let json = json!({ "outcome": "cancelled", "id": "call_1.confirm.1" }); + let response: InquiryResponse = serde_json::from_value(json).unwrap(); + assert_eq!(response, InquiryResponse::Cancelled { + id: InquiryId::new("call_1.confirm.1"), + reason: CancellationReason::Unknown("unspecified".to_owned()), + }); + + // A non-string reason is equally unusable and lands on the same sentinel, + // consistent with unrecognized string tags mapping to `Unknown`. + let json = json!({ "outcome": "cancelled", "id": "call_1.confirm.1", "reason": 42 }); + let response: InquiryResponse = serde_json::from_value(json).unwrap(); + assert_eq!(response, InquiryResponse::Cancelled { + id: InquiryId::new("call_1.confirm.1"), + reason: CancellationReason::Unknown("unspecified".to_owned()), + }); +} + +#[test] +fn test_inquiry_response_unknown_reason_round_trips() { + let json = json!({ + "outcome": "cancelled", + "id": "call_1.confirm.1", + "reason": "some_future_variant", + }); + let response: InquiryResponse = serde_json::from_value(json.clone()).unwrap(); + assert_eq!(response, InquiryResponse::Cancelled { + id: InquiryId::new("call_1.confirm.1"), + reason: CancellationReason::Unknown("some_future_variant".to_owned()), + }); + + // The unknown tag survives a re-serialize verbatim. + assert_eq!(serde_json::to_value(&response).unwrap(), json); +} + +#[test] +fn test_inquiry_response_redacted_serialization() { + let response = InquiryResponse::Redacted { + id: InquiryId::new("call_1.passphrase.1"), + }; + + let json = serde_json::to_value(&response).unwrap(); + assert_eq!( + json, + json!({ "outcome": "redacted", "id": "call_1.passphrase.1" }) + ); + assert!(json.get("answer").is_none()); let deserialized: InquiryResponse = serde_json::from_value(json).unwrap(); assert_eq!(deserialized, response); + assert_eq!(deserialized.answer(), None); +} + +#[test] +fn test_inquiry_response_invalid_shape_is_error() { + // No `outcome`, no `answer` — not interpretable as any variant. + let json = json!({ "id": "call_1.confirm" }); + let result: Result = serde_json::from_value(json); + assert!(result.is_err()); } #[test] @@ -99,17 +223,18 @@ fn test_select_option_serialization() { } #[test] -fn test_inquiry_response_helpers() { - let response = InquiryResponse::boolean("id", true); - assert_eq!(response.as_bool(), Some(true)); - assert_eq!(response.as_str(), None); - - let response = InquiryResponse::text("id", "hello".to_string()); - assert_eq!(response.as_str(), Some("hello")); - - let response = InquiryResponse::select("id", "option1"); - assert_eq!(response.as_str(), Some("option1")); - - let response = InquiryResponse::select("id", 42); - assert_eq!(response.answer, 42); +fn test_inquiry_response_answer_accessor() { + let answered = InquiryResponse::select("id", 42); + assert_eq!(answered.answer(), Some(&json!(42))); + + let cancelled = InquiryResponse::Cancelled { + id: InquiryId::new("id"), + reason: CancellationReason::User, + }; + assert_eq!(cancelled.answer(), None); + + let redacted = InquiryResponse::Redacted { + id: InquiryId::new("id"), + }; + assert_eq!(redacted.answer(), None); } diff --git a/crates/jp_conversation/src/stream.rs b/crates/jp_conversation/src/stream.rs index e1c72c7d..1a5a6b2e 100644 --- a/crates/jp_conversation/src/stream.rs +++ b/crates/jp_conversation/src/stream.rs @@ -1,6 +1,6 @@ //! See [`ConversationStream`]. -use std::sync::Arc; +use std::{collections::HashMap, sync::Arc}; use chrono::{DateTime, Utc}; use jp_config::{AppConfig, PartialAppConfig, PartialConfig as _}; @@ -790,51 +790,91 @@ impl ConversationStream { }); } - /// Removes [`InquiryResponse`]s whose ID doesn't match any - /// [`InquiryRequest`] in the stream. + /// Removes [`InquiryResponse`]s that have no matching [`InquiryRequest`] + /// **within the same turn**. + /// + /// The ID set is scoped to the containing turn so `InquiryId` reuse across + /// turns cannot cross-satisfy a pair. + /// When a legacy two-segment ID collides within a turn, requests and + /// responses pair by order (each request satisfies at most one response); + /// the unpaired excess is removed. /// /// [`InquiryRequest`]: crate::event::InquiryRequest /// [`InquiryResponse`]: crate::event::InquiryResponse fn remove_orphaned_inquiry_responses(&mut self) { - let request_ids: Vec = self - .events - .iter() - .filter_map(InternalEvent::as_event) - .filter_map(|e| e.as_inquiry_request()) - .map(|r| r.id.clone()) - .collect(); + let mut request_counts: HashMap<(usize, InquiryId), usize> = HashMap::new(); + let mut turn = 0; + for event in self.events.iter().filter_map(InternalEvent::as_event) { + if event.is_turn_start() { + turn += 1; + } else if let Some(request) = event.as_inquiry_request() { + *request_counts + .entry((turn, request.id.clone())) + .or_default() += 1; + } + } + let mut turn = 0; self.events.retain(|event| { - if let Some(event) = event.as_event() - && let Some(response) = event.as_inquiry_response() - { - return request_ids.contains(&response.id); + let Some(event) = event.as_event() else { + return true; + }; + if event.is_turn_start() { + turn += 1; + return true; + } + let Some(response) = event.as_inquiry_response() else { + return true; + }; + match request_counts.get_mut(&(turn, response.id().clone())) { + Some(remaining) if *remaining > 0 => { + *remaining -= 1; + true + } + _ => false, } - true }); } - /// Removes [`InquiryRequest`]s whose ID doesn't match any - /// [`InquiryResponse`] in the stream. + /// Removes [`InquiryRequest`]s that have no matching [`InquiryResponse`] + /// **within the same turn** (see + /// [`Self::remove_orphaned_inquiry_responses`] for the turn-scoping and + /// order-pairing rationale). /// /// [`InquiryRequest`]: crate::event::InquiryRequest /// [`InquiryResponse`]: crate::event::InquiryResponse fn remove_orphaned_inquiry_requests(&mut self) { - let response_ids: Vec = self - .events - .iter() - .filter_map(InternalEvent::as_event) - .filter_map(|e| e.as_inquiry_response()) - .map(|r| r.id.clone()) - .collect(); + let mut response_counts: HashMap<(usize, InquiryId), usize> = HashMap::new(); + let mut turn = 0; + for event in self.events.iter().filter_map(InternalEvent::as_event) { + if event.is_turn_start() { + turn += 1; + } else if let Some(response) = event.as_inquiry_response() { + *response_counts + .entry((turn, response.id().clone())) + .or_default() += 1; + } + } + let mut turn = 0; self.events.retain(|event| { - if let Some(event) = event.as_event() - && let Some(request) = event.as_inquiry_request() - { - return response_ids.contains(&request.id); + let Some(event) = event.as_event() else { + return true; + }; + if event.is_turn_start() { + turn += 1; + return true; + } + let Some(request) = event.as_inquiry_request() else { + return true; + }; + match response_counts.get_mut(&(turn, request.id.clone())) { + Some(remaining) if *remaining > 0 => { + *remaining -= 1; + true + } + _ => false, } - true }); } diff --git a/crates/jp_conversation/src/stream/turn_mut.rs b/crates/jp_conversation/src/stream/turn_mut.rs index 85e399d3..d84ba481 100644 --- a/crates/jp_conversation/src/stream/turn_mut.rs +++ b/crates/jp_conversation/src/stream/turn_mut.rs @@ -235,7 +235,7 @@ impl<'a> TurnMut<'a> { } } EventKind::InquiryResponse(resp) => { - let id = &resp.id; + let id = resp.id(); let requests = turn_events .iter() @@ -254,7 +254,7 @@ impl<'a> TurnMut<'a> { .filter_map(InternalEvent::as_event) .chain(events.iter()) .filter_map(ConversationEvent::as_inquiry_response) - .filter(|r| r.id == *id) + .filter(|r| r.id() == id) .count(); if responses >= requests { diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index 0d6e0877..ecafb945 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -500,6 +500,97 @@ fn test_sanitize_removes_orphaned_inquiry_request() { ); } +#[test] +fn test_sanitize_inquiry_orphan_repair_is_turn_scoped() { + // Turn 1 holds an orphaned request that shares its id with a valid pair in + // turn 2. The turn-1 orphan must be repaired within its own turn, not + // cross-satisfied by turn 2's response. + let mut stream = ConversationStream::new_test(); + + stream.start_turn("turn 1"); + stream.push(ConversationEvent::new( + InquiryRequest::new( + "call_1.confirm", + InquirySource::tool("t"), + InquiryQuestion::boolean("proceed?".into()), + ), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 1).unwrap(), + )); + + stream.start_turn("turn 2"); + stream.push(ConversationEvent::new( + InquiryRequest::new( + "call_1.confirm", + InquirySource::tool("t"), + InquiryQuestion::boolean("proceed?".into()), + ), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), + )); + stream.push(ConversationEvent::new( + InquiryResponse::boolean("call_1.confirm", true), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 3).unwrap(), + )); + + stream.sanitize(); + + let requests = stream + .iter() + .filter(|e| e.event.is_inquiry_request()) + .count(); + let responses = stream + .iter() + .filter(|e| e.event.is_inquiry_response()) + .count(); + assert_eq!(requests, 1, "turn 1's orphan request should be removed"); + assert_eq!(responses, 1, "turn 2's paired response should survive"); +} + +#[test] +fn test_sanitize_legacy_duplicate_ids_pair_by_order() { + // A single turn with two requests sharing a legacy two-segment id and one + // response: the by-order pair is preserved and the remaining request + // orphan is removed. + let mut stream = ConversationStream::new_test(); + + stream.start_turn("turn"); + stream.push(ConversationEvent::new( + InquiryRequest::new( + "call_1.confirm", + InquirySource::tool("t"), + InquiryQuestion::boolean("proceed?".into()), + ), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 1).unwrap(), + )); + stream.push(ConversationEvent::new( + InquiryRequest::new( + "call_1.confirm", + InquirySource::tool("t"), + InquiryQuestion::boolean("proceed?".into()), + ), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), + )); + stream.push(ConversationEvent::new( + InquiryResponse::boolean("call_1.confirm", true), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 3).unwrap(), + )); + + stream.sanitize(); + + let requests = stream + .iter() + .filter(|e| e.event.is_inquiry_request()) + .count(); + let responses = stream + .iter() + .filter(|e| e.event.is_inquiry_response()) + .count(); + assert_eq!( + requests, 1, + "the unpaired duplicate request should be removed" + ); + assert_eq!(responses, 1); +} + #[test] fn test_sanitize_keeps_matched_pairs_intact() { let mut stream = ConversationStream::new_test(); diff --git a/crates/jp_inquire/src/prompt.rs b/crates/jp_inquire/src/prompt.rs index 2afc673a..7d0e3f80 100644 --- a/crates/jp_inquire/src/prompt.rs +++ b/crates/jp_inquire/src/prompt.rs @@ -55,6 +55,9 @@ pub trait PromptBackend: Send + Sync { default: Option, writer: &mut dyn Write, ) -> Result; + + /// Display a single-line no-echo input prompt for a secret value. + fn password(&self, message: &str, writer: &mut dyn Write) -> Result; } /// Blanket impl for references, enabling `&dyn PromptBackend` to work. @@ -98,6 +101,10 @@ impl PromptBackend for &P { ) -> Result { (*self).select(message, options, default, writer) } + + fn password(&self, message: &str, writer: &mut dyn Write) -> Result { + (*self).password(message, writer) + } } /// Terminal prompt backend using `jp_inquire` and `inquire`. @@ -166,6 +173,12 @@ impl PromptBackend for TerminalPromptBackend { } prompt.prompt_with_writer(writer) } + + fn password(&self, message: &str, writer: &mut dyn Write) -> Result { + inquire::Password::new(message) + .without_confirmation() + .prompt_with_writer(writer) + } } /// Mock prompt backend for testing. @@ -180,6 +193,7 @@ pub struct MockPromptBackend { reply_outcomes: Mutex>, text_responses: Mutex>, select_responses: Mutex>, + password_responses: Mutex>, } impl MockPromptBackend { @@ -223,6 +237,15 @@ impl MockPromptBackend { *self.select_responses.lock() = responses.into_iter().map(Into::into).collect(); self } + + #[must_use] + pub fn with_password_responses( + self, + responses: impl IntoIterator>, + ) -> Self { + *self.password_responses.lock() = responses.into_iter().map(Into::into).collect(); + self + } } impl PromptBackend for MockPromptBackend { @@ -277,4 +300,11 @@ impl PromptBackend for MockPromptBackend { .pop_front() .ok_or(InquireError::OperationCanceled) } + + fn password(&self, _message: &str, _writer: &mut dyn Write) -> Result { + self.password_responses + .lock() + .pop_front() + .ok_or(InquireError::OperationCanceled) + } } diff --git a/crates/jp_llm/src/tool.rs b/crates/jp_llm/src/tool.rs index d87296bd..6803f2f1 100644 --- a/crates/jp_llm/src/tool.rs +++ b/crates/jp_llm/src/tool.rs @@ -27,7 +27,7 @@ use tokio::{ process::Command, }; use tokio_util::sync::CancellationToken; -use tracing::{info, trace, warn}; +use tracing::{error, info, trace, warn}; use crate::error::ToolError; @@ -281,6 +281,17 @@ pub enum CommandResult { /// Whether the process exited successfully. success: bool, }, + + /// Tool emitted a well-formed `needs_input` whose question id is invalid + /// (empty, or contains a `.`, which is reserved as the inquiry-id + /// separator). + /// + /// Surfaced as a tool-level error so the malformed inquiry is dropped + /// before any inquiry event is constructed. + InvalidInquiry { + /// The offending question id, for the diagnostic trace. + question_id: String, + }, } impl CommandResult { @@ -332,6 +343,19 @@ impl CommandResult { .to_string()) } } + Self::InvalidInquiry { question_id } => { + error!( + tool = name, + question_id = %question_id, + "tool produced an invalid inquiry: question id must be non-empty and must not \ + contain '.'" + ); + Err( + "tool produced an invalid inquiry: question id must be non-empty and must not \ + contain '.'" + .to_owned(), + ) + } Self::NeedsInput(_) => { unreachable!("NeedsInput should be handled by the caller") } @@ -572,11 +596,34 @@ fn parse_command_output(stdout: &[u8], stderr: &[u8], success: bool) -> CommandR } } Ok(Outcome::NeedsInput { question }) => CommandResult::NeedsInput(question), - Err(_) => CommandResult::RawOutput { - stdout: stdout_str.into_owned(), - stderr: String::from_utf8_lossy(stderr).into_owned(), - success, - }, + // A `needs_input` whose question id is empty or contains a `.` fails + // to deserialize (`QuestionId` rejects both). Surface that as a + // tool-level error rather than silently treating the outcome as raw + // text. Any other parse failure stays `RawOutput`. + Err(_) => { + let invalid_id = serde_json::from_str::(&stdout_str) + .ok() + .and_then(|v| { + (v.get("type").and_then(Value::as_str) == Some("needs_input")) + .then(|| { + v.get("question") + .and_then(|q| q.get("id")) + .and_then(Value::as_str) + }) + .flatten() + .filter(|id| id.is_empty() || id.contains('.')) + .map(str::to_owned) + }); + + match invalid_id { + Some(question_id) => CommandResult::InvalidInquiry { question_id }, + None => CommandResult::RawOutput { + stdout: stdout_str.into_owned(), + stderr: String::from_utf8_lossy(stderr).into_owned(), + success, + }, + } + } } } diff --git a/crates/jp_llm/src/tool/builtin.rs b/crates/jp_llm/src/tool/builtin.rs index b247c43c..538e8a28 100644 --- a/crates/jp_llm/src/tool/builtin.rs +++ b/crates/jp_llm/src/tool/builtin.rs @@ -8,6 +8,7 @@ use std::{collections::HashMap, sync::Arc}; use async_trait::async_trait; use indexmap::IndexMap; +use jp_conversation::event::InquirySource; use jp_tool::Outcome; use serde_json::Value; @@ -16,6 +17,17 @@ use serde_json::Value; pub trait BuiltinTool: Send + Sync { /// Execute the tool with the given arguments and accumulated answers. async fn execute(&self, arguments: &Value, answers: &IndexMap) -> Outcome; + + /// The persisted `InquirySource` for questions emitted by this tool. + /// + /// Default: `InquirySource::Tool { name }`. + /// Override for tools whose questions are semantically the assistant's, not + /// the tool's (e.g. `ask_user`). + fn inquiry_source(&self, name: &str) -> InquirySource { + InquirySource::Tool { + name: name.to_owned(), + } + } } /// Registry mapping builtin tool names to their executors. diff --git a/crates/jp_llm/src/tool/executor.rs b/crates/jp_llm/src/tool/executor.rs index ae2a5bf4..b53b6031 100644 --- a/crates/jp_llm/src/tool/executor.rs +++ b/crates/jp_llm/src/tool/executor.rs @@ -4,7 +4,7 @@ use async_trait::async_trait; use camino::Utf8Path; use indexmap::IndexMap; use jp_config::conversation::tool::{RunMode, ToolConfigWithDefaults, ToolSource}; -use jp_conversation::event::{ToolCallRequest, ToolCallResponse}; +use jp_conversation::event::{InquirySource, ToolCallRequest, ToolCallResponse}; use jp_mcp::Client; use jp_tool::Question; use serde_json::{Map, Value}; @@ -130,6 +130,12 @@ pub enum ExecutorResult { /// The question that needs to be answered. question: Question, + /// Resolved provenance for the persisted `InquiryRequest`. + /// + /// Built-in tools may override this via `BuiltinTool::inquiry_source`; + /// local and MCP tools attribute the question to the tool by name. + source: InquirySource, + /// Accumulated answers so far (for retry). accumulated_answers: IndexMap, }, diff --git a/crates/jp_llm/src/tool_tests.rs b/crates/jp_llm/src/tool_tests.rs index cdc19b52..a85892c2 100644 --- a/crates/jp_llm/src/tool_tests.rs +++ b/crates/jp_llm/src/tool_tests.rs @@ -26,7 +26,7 @@ fn test_execution_outcome_completed_error_into_response() { #[test] fn test_execution_outcome_needs_input_into_response() { - let question = Question::text("q1", "What is your name?"); + let question = Question::text("q1", "What is your name?").unwrap(); let outcome = ExecutionOutcome::NeedsInput { id: "call_789".to_string(), @@ -66,7 +66,7 @@ fn test_execution_outcome_id() { let needs_input = ExecutionOutcome::NeedsInput { id: "id2".to_string(), - question: Question::text("q", "?"), + question: Question::text("q", "?").unwrap(), }; assert_eq!(needs_input.id(), "id2"); @@ -96,7 +96,7 @@ fn test_execution_outcome_helper_methods() { let needs_input = ExecutionOutcome::NeedsInput { id: "3".to_string(), - question: Question::boolean("q", "?"), + question: Question::boolean("q", "?").unwrap(), }; assert!(!needs_input.is_success()); assert!(needs_input.needs_input()); @@ -110,6 +110,46 @@ fn test_execution_outcome_helper_methods() { assert!(cancelled.is_cancelled()); } +#[test] +fn parse_command_output_valid_needs_input() { + let stdout = br#"{"type":"needs_input","question":{"id":"confirm","text":"?","pre_amble":null,"answer_type":{"type":"boolean"},"default":null}}"#; + assert!(matches!( + parse_command_output(stdout, b"", true), + CommandResult::NeedsInput(_) + )); +} + +#[test] +fn parse_command_output_dotted_question_id_is_invalid_inquiry() { + let stdout = br#"{"type":"needs_input","question":{"id":"a.b","text":"?","pre_amble":null,"answer_type":{"type":"boolean"},"default":null}}"#; + let result = parse_command_output(stdout, b"", true); + assert!(matches!( + result, + CommandResult::InvalidInquiry { ref question_id } if question_id == "a.b" + )); + // Renders as a tool-level error, not raw text. + assert!(result.into_tool_result("t").is_err()); +} + +#[test] +fn parse_command_output_empty_question_id_is_invalid_inquiry() { + let stdout = br#"{"type":"needs_input","question":{"id":"","text":"?","pre_amble":null,"answer_type":{"type":"boolean"},"default":null}}"#; + let result = parse_command_output(stdout, b"", true); + assert!(matches!( + result, + CommandResult::InvalidInquiry { ref question_id } if question_id.is_empty() + )); + assert!(result.into_tool_result("t").is_err()); +} + +#[test] +fn parse_command_output_non_outcome_is_raw() { + assert!(matches!( + parse_command_output(b"plain text", b"", true), + CommandResult::RawOutput { .. } + )); +} + /// Build a minimal `ToolParameterConfig` for use in validation tests. fn param(kind: &str, required: bool) -> ToolParameterConfig { ToolParameterConfig { diff --git a/crates/jp_tool/src/lib.rs b/crates/jp_tool/src/lib.rs index 2f7c7c9a..6a884b29 100644 --- a/crates/jp_tool/src/lib.rs +++ b/crates/jp_tool/src/lib.rs @@ -1,5 +1,7 @@ +use std::{fmt, str::FromStr}; + use camino::Utf8PathBuf; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; use serde_json::Value; mod access; @@ -83,6 +85,98 @@ impl Outcome { } } +/// A validated tool-question identifier. +/// +/// A `QuestionId` is never empty and never contains a `.`: the dot is reserved +/// as the segment separator in the persisted inquiry ID +/// (`..`), and an empty id is a +/// tool-authoring bug. +/// The only ways to build one are the validating `FromStr`/`TryFrom` +/// conversions and `Deserialize`, so an invalid id cannot exist past the tool +/// boundary. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] +#[serde(transparent)] +pub struct QuestionId(String); + +impl QuestionId { + /// The id as a string slice. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl FromStr for QuestionId { + type Err = InvalidQuestionId; + + fn from_str(s: &str) -> Result { + if s.is_empty() || s.contains('.') { + return Err(InvalidQuestionId); + } + Ok(Self(s.to_owned())) + } +} + +impl TryFrom for QuestionId { + type Error = InvalidQuestionId; + + fn try_from(s: String) -> Result { + if s.is_empty() || s.contains('.') { + return Err(InvalidQuestionId); + } + Ok(Self(s)) + } +} + +impl TryFrom<&str> for QuestionId { + type Error = InvalidQuestionId; + + fn try_from(s: &str) -> Result { + s.parse() + } +} + +impl fmt::Display for QuestionId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl PartialEq for QuestionId { + fn eq(&self, other: &str) -> bool { + self.0 == other + } +} + +impl PartialEq<&str> for QuestionId { + fn eq(&self, other: &&str) -> bool { + self.0 == *other + } +} + +impl<'de> Deserialize<'de> for QuestionId { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Self::try_from(s).map_err(serde::de::Error::custom) + } +} + +/// Error returned when a string is not a valid [`QuestionId`] (it is empty or +/// contains a `.`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct InvalidQuestionId; + +impl fmt::Display for InvalidQuestionId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("question id must be non-empty and must not contain '.'") + } +} + +impl std::error::Error for InvalidQuestionId {} + /// A request for additional input. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[non_exhaustive] @@ -90,7 +184,7 @@ pub struct Question { /// The question ID. /// /// This must be passed back to the tool when answering the question. - pub id: String, + pub id: QuestionId, /// The question to ask. /// @@ -113,36 +207,60 @@ pub struct Question { impl Question { /// Create a new text question. - pub fn text(id: impl Into, text: impl Into) -> Self { - Self { - id: id.into(), + /// Fails if `id` is empty or contains a `.`. + pub fn text(id: impl Into, text: impl Into) -> Result { + Ok(Self { + id: QuestionId::try_from(id.into())?, text: text.into(), pre_amble: None, answer_type: AnswerType::Text, default: None, - } + }) } /// Create a new boolean question. - pub fn boolean(id: impl Into, text: impl Into) -> Self { - Self { - id: id.into(), + /// Fails if `id` is empty or contains a `.`. + pub fn boolean( + id: impl Into, + text: impl Into, + ) -> Result { + Ok(Self { + id: QuestionId::try_from(id.into())?, text: text.into(), pre_amble: None, answer_type: AnswerType::Boolean, default: None, - } + }) } - /// Create a new boolean question. - pub fn select(id: impl Into, text: impl Into) -> Self { - Self { - id: id.into(), + /// Create a new select question. + /// Fails if `id` is empty or contains a `.`. + pub fn select( + id: impl Into, + text: impl Into, + ) -> Result { + Ok(Self { + id: QuestionId::try_from(id.into())?, text: text.into(), pre_amble: None, answer_type: AnswerType::Select { options: vec![] }, default: None, - } + }) + } + + /// Create a new secret (no-echo, non-persisted) question. + /// Fails if `id` is empty or contains a `.`. + pub fn secret( + id: impl Into, + text: impl Into, + ) -> Result { + Ok(Self { + id: QuestionId::try_from(id.into())?, + text: text.into(), + pre_amble: None, + answer_type: AnswerType::Secret, + default: None, + }) } /// Set the preamble text. @@ -193,6 +311,7 @@ impl Question { /// The type of answer expected for a given question. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] pub enum AnswerType { /// Boolean yes/no question Boolean, @@ -202,6 +321,12 @@ pub enum AnswerType { /// Free-form text input Text, + + /// Free-form text input whose answer must not be persisted on disk. + /// + /// Prompter input is not echoed, and the persisted inquiry response is + /// recorded as redacted rather than carrying the answer. + Secret, } /// Contextual information available to a tool. @@ -287,3 +412,7 @@ impl Action { matches!(self, Self::FormatArguments) } } + +#[cfg(test)] +#[path = "lib_tests.rs"] +mod tests; diff --git a/crates/jp_tool/src/lib_tests.rs b/crates/jp_tool/src/lib_tests.rs new file mode 100644 index 00000000..b5683fa8 --- /dev/null +++ b/crates/jp_tool/src/lib_tests.rs @@ -0,0 +1,96 @@ +use super::*; + +#[test] +fn question_id_rejects_dot() { + assert_eq!("has.dot".parse::(), Err(InvalidQuestionId)); + assert_eq!( + QuestionId::try_from("has.dot".to_owned()), + Err(InvalidQuestionId) + ); + assert_eq!(QuestionId::try_from("has.dot"), Err(InvalidQuestionId)); +} + +#[test] +fn question_id_rejects_empty() { + assert_eq!("".parse::(), Err(InvalidQuestionId)); + assert_eq!(QuestionId::try_from(String::new()), Err(InvalidQuestionId)); + assert_eq!(QuestionId::try_from(""), Err(InvalidQuestionId)); + + // Rejected at the deserialization boundary too. + assert!(serde_json::from_str::("\"\"").is_err()); + + // And through every constructor. + assert!(Question::text("", "?").is_err()); + assert!(Question::boolean("", "?").is_err()); + assert!(Question::select("", "?").is_err()); + assert!(Question::secret("", "?").is_err()); +} + +#[test] +fn question_id_accepts_plain() { + let id: QuestionId = "overwrite_file".parse().unwrap(); + assert_eq!(id, "overwrite_file"); + assert_eq!(id.as_str(), "overwrite_file"); + assert_eq!(id.to_string(), "overwrite_file"); +} + +#[test] +fn question_id_deserialize_validates() { + let id: QuestionId = serde_json::from_str("\"confirm\"").unwrap(); + assert_eq!(id, "confirm"); + + // A dotted id is rejected at the deserialization boundary. + assert!(serde_json::from_str::("\"a.b\"").is_err()); +} + +#[test] +fn question_id_serializes_transparently() { + let id: QuestionId = "confirm".parse().unwrap(); + assert_eq!(serde_json::to_string(&id).unwrap(), "\"confirm\""); +} + +#[test] +fn question_constructors_reject_dotted_id() { + assert!(Question::boolean("a.b", "?").is_err()); + assert!(Question::text("a.b", "?").is_err()); + assert!(Question::select("a.b", "?").is_err()); + + let q = Question::boolean("confirm", "Proceed?").unwrap(); + assert_eq!(q.id, "confirm"); +} + +#[test] +fn answer_type_serializes_with_internal_type_tag() { + use serde_json::json; + + assert_eq!( + serde_json::to_value(AnswerType::Boolean).unwrap(), + json!({ "type": "boolean" }) + ); + assert_eq!( + serde_json::to_value(AnswerType::Text).unwrap(), + json!({ "type": "text" }) + ); + assert_eq!( + serde_json::to_value(AnswerType::Secret).unwrap(), + json!({ "type": "secret" }) + ); + assert_eq!( + serde_json::to_value(AnswerType::Select { + options: vec!["a".to_owned()] + }) + .unwrap(), + json!({ "type": "select", "options": ["a"] }) + ); + + let secret: AnswerType = serde_json::from_value(json!({ "type": "secret" })).unwrap(); + assert_eq!(secret, AnswerType::Secret); +} + +#[test] +fn question_secret_constructor() { + let q = Question::secret("passphrase", "Enter passphrase").unwrap(); + assert_eq!(q.id, "passphrase"); + assert_eq!(q.answer_type, AnswerType::Secret); + assert!(Question::secret("a.b", "?").is_err()); +} diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index d163e122..d030421a 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -12,7 +12,7 @@ "summary": "Buffer segments streaming markdown blocks; TerminalRenderer applies ANSI-aware terminal formatting; comrak chosen over pulldown-cmark." }, "005-first-class-inquiry-events.md": { - "hash": "bd19573644d77d9c4d14b7b9c8feae87c997320977d53846fb717a7b69d6397f", + "hash": "6e06d81559aa0ca48fffd041ea70d2f74c8e3a59d3a9d3b9adba1e19361226ca", "summary": "Record inquiry events in conversation stream with centralized filtering to keep them hidden from LLM providers." }, "006-turn-scoped-mutations.md": { @@ -324,7 +324,7 @@ "summary": "Split tool enable into state and allow_toggle to fix directive bugs and eliminate variant proliferation." }, "082-unified-inquiry-event-recording.md": { - "hash": "57e2a66512d7ec922bb83f8078a81177a884797ae41ec6609f8f1e75e6fb842a", + "hash": "c617de652451b00153898d130a243a2b4e46ba4f9eb99db355927ebc5073df20", "summary": "Unify inquiry event recording across all routing paths; add cancellation/redaction variants; track attempts per turn." }, "083-built-in-ask_user-tool-for-assistant-initiated-inquiries.md": { diff --git a/docs/rfd/005-first-class-inquiry-events.md b/docs/rfd/005-first-class-inquiry-events.md index d60a1816..d32d6722 100644 --- a/docs/rfd/005-first-class-inquiry-events.md +++ b/docs/rfd/005-first-class-inquiry-events.md @@ -195,20 +195,32 @@ migrate to `into_parts()`. The `ToolCoordinator` records inquiry events at two points: -1. **On inquiry spawn** (`handle_tool_result`, when a question is routed through - the inquiry backend): Push an `InquiryRequest` into the conversation stream. - -2. **On inquiry result** (`InquiryResult` handler in the event loop): Push an - `InquiryResponse` into the conversation stream. - -Inquiry events are recorded for any question routed through the -`InquiryBackend`, not only those with `QuestionTarget::Assistant`. -In non-interactive environments (no TTY), user-targeted questions also fall -through to the inquiry backend and are recorded. - -Questions answered via interactive user prompts (TTY + `QuestionTarget::User`) -are *not* recorded as inquiry events — they go through the `ToolPrompter` path, -which doesn't touch the conversation stream. +1. **On question arrival** (`handle_tool_result`, before any routing decision): + Push an `InquiryRequest` into the conversation stream. + +2. **On resolution**: Push an `InquiryResponse` into the conversation stream — + `Answered` when an answer resolves the question, `Redacted` for + `AnswerType::Secret` answers, or `Cancelled` when the question closes without + an answer. + +Inquiry events are recorded for **every** `Outcome::NeedsInput` round-trip, +regardless of routing path (amended by [RFD 082]; this section originally +excluded prompter-answered questions): + +- Questions resolved by the interactive prompter (TTY + `QuestionTarget::User`). +- Questions resolved by a cached "remember for turn" answer from earlier in the + same turn. +- Questions resolved by a static `QuestionConfig.answer` value. +- Questions routed through the `InquiryBackend` (explicit + `QuestionTarget::Assistant`, or user-targeted questions falling through in + non-interactive environments). + +When a question closes without an answer, the pair is completed with +`InquiryResponse::Cancelled { reason }` rather than left dangling: `user` for a +user cancellation (Ctrl-C at the prompt, or a cancellation token fired by a user +action), `backend_error` for an inquiry-backend failure, and `no_prompt_backend` +/ `assistant_routing_denied` for `AnswerType::Secret` questions that could not +be routed to a human. `execute_with_prompting` receives `events: &mut ConversationStream` so the coordinator has write access. diff --git a/docs/rfd/082-unified-inquiry-event-recording.md b/docs/rfd/082-unified-inquiry-event-recording.md index 980b8209..b0b3c5b9 100644 --- a/docs/rfd/082-unified-inquiry-event-recording.md +++ b/docs/rfd/082-unified-inquiry-event-recording.md @@ -1,6 +1,6 @@ # RFD 082: Unified inquiry event recording -- **Status**: Discussion +- **Status**: Accepted - **Category**: Design - **Authors**: Jean Mertz - **Date**: 2026-05-12 @@ -110,7 +110,44 @@ attempt is `1`; the next recording for the same key (the LLM gave an invalid answer and the tool re-asks; or a provider like Google Gemini reused the same `tool_call_id` in a later cycle and the same question came up again) gets attempt `2`; and so on. -IDs are unique by construction within the turn. + +`question_id` MUST NOT contain `.`. +This is enforced with a `QuestionId` newtype (in `jp_tool`, wrapping the `id` +field of `Question`) whose `FromStr`/`TryFrom` validation rejects any string +containing a dot. +Validation happens once, at the tool→JP boundary where a `Question` is +ingested: local-tool stdout is parsed into `QuestionId`, and built-in tools +construct their questions through the same validated constructor, so past that +boundary an invalid id is unrepresentable and no downstream code re-checks. +`QuestionId` is distinct from the composite stream-correlation `InquiryId` +(`..`): it validates only the +tool-author-set segment, and `attempt` is always numeric and therefore dot-free. + +When a tool emits a `NeedsInput` whose `question.id` is invalid, JP rejects it +at the boundary. +It logs an `error!` trace with the offending tool and id, and fails the tool +with a tool-level error response (`tool produced an invalid inquiry: question id +must not contain '.'`). +No separate chrome line is emitted: the tool-level error is rendered to the +terminal through the existing `ErrorStyleConfig` path like any other tool +failure, so a bespoke notice would double-print. +Because the malformed outcome is dropped before any inquiry event is +constructed, it never enters the recording pipeline, and the "every `NeedsInput` +is recorded" invariant holds without a carve-out (there is a `ToolCallRequest`/ +`ToolCallResponse` pair for the failed call, but no `InquiryRequest`). +One implementation caveat: local-tool stdout is parsed by +`serde_json::from_str::` (`crates/jp_llm/src/tool.rs`), which today +falls back to `RawOutput` on any parse error. +The ingestion path MUST distinguish a well-formed `NeedsInput` carrying an +invalid `question.id` (which becomes an `error!` plus a tool-level error) from +output that is not an `Outcome` at all (which becomes `RawOutput`), so the +invalid-inquiry diagnostic is not silently swallowed by the fallback. + +Given that constraint, the join is injective and IDs are **unique by +construction within the turn**, even when `tool_call_id` contains dots (which JP +cannot control, since providers generate it): for any joined string the last two +dot-separated tokens are unambiguously `question_id` and `attempt`, and +everything before them is `tool_call_id`. The counter is tracked per-turn (in `TurnState`) keyed by `(tool_call_id, question_id)`. @@ -138,14 +175,17 @@ IDs. To read both formats without a migration step, the pairing logic on read accepts both shapes and falls back to request/response order within the turn when IDs collide (the same strategy used for tool-call IDs today). -Only new writes use the three-segment form, and new writes do not collide -because the counter is turn-scoped. +Only new writes use the three-segment form, and new writes do not collide: the +counter is turn-scoped and the no-dot rule on `question_id` keeps the join +injective. +(Legacy streams predate the no-dot rule, which is why the read-side order +fallback remains.) This ID is the **stream-correlation** identifier for the persisted audit trail. It is distinct from two other identifiers that share some of the same parts: - The in-memory `TurnState` cache key (`.` — see - [Splitting the turn-cache state] (\#splitting-the-turn-cache-state)) dedupes + [Splitting the turn-cache state](#splitting-the-turn-cache-state)) dedupes "remember for turn" answers across tool calls within the same turn. - The tool-facing `ExecutingTool::accumulated_answers` map remains keyed by the bare `question.id`, with latest-answer-wins semantics. @@ -233,13 +273,14 @@ built-in. ### Recording lifecycle -For every `Outcome::NeedsInput { question }`: +For every `Outcome::NeedsInput { question }` that passed ingestion validation (a +valid `QuestionId` — see [Inquiry ID format](#inquiry-id-format)): 1. The coordinator reads the `InquirySource` carried on `ExecutorResult::NeedsInput`. Resolution happened at the executor boundary (see [Crossing the executor - boundary] (\#crossing-the-executor-boundary)); the coordinator does not - branch on tool type. + boundary](#crossing-the-executor-boundary)); the coordinator does not branch + on tool type. 2. The coordinator records `InquiryRequest { id, source, question }` **before any routing decision** — including the cached-answer and static-answer @@ -254,7 +295,7 @@ For every `Outcome::NeedsInput { question }`: - **Cached answer** (`remembered_tool_answers` — a previous `Y`/`N` in this turn). Skipped for `AnswerType::Secret` questions; secret answers do not enter the - cache (see [Secret questions] (\#secret-questions)). + cache (see [Secret questions](#secret-questions)). For non-`Secret` answer types: if found, record `InquiryResponse::Answered { id, answer }` with the cached value and resume tool execution. No prompt is shown. @@ -267,7 +308,7 @@ For every `Outcome::NeedsInput { question }`: 4. Otherwise, route the question (interactive prompter or inquiry backend). For `AnswerType::Secret` questions on the prompter path, the prompter uses a - no-echo input mode (see [Secret questions] (\#secret-questions) for the + no-echo input mode (see [Secret questions](#secret-questions) for the routing-scope caveat). 5. On a successful answer: @@ -275,8 +316,8 @@ For every `Outcome::NeedsInput { question }`: - Non-`Secret` answer type (from prompter or inquiry backend): record `InquiryResponse::Answered { id, answer }`. - `AnswerType::Secret` (from prompter only — the inquiry backend is - unreachable per the routing guard in [Secret questions] - (\#secret-questions)): record `InquiryResponse::Redacted { id }`. + unreachable per the routing guard in [Secret + questions](#secret-questions)): record `InquiryResponse::Redacted { id }`. The answer is delivered to the tool in-memory; only the persisted shape is redacted. @@ -288,18 +329,17 @@ For every `Outcome::NeedsInput { question }`: user cancelled would land unpaired on disk. The `reason` is determined by the originating event: - | Originating event | `reason` | | - ------------------------------------------------------ | - ------------------------ | | `ExecutionEvent::PromptCancelled` (user Ctrl-C - or EOF) | `User` | | `InquiryError::Cancelled` (cancellation token fired — | - `User` | | user restart, tool cancel, hard quit) | | | - `InquiryError::Provider` | `BackendError` | | - `InquiryError::MissingStructuredData` | `BackendError` | | - `InquiryError::AnswerExtraction { .. }` | `BackendError` | | - `InquiryError::Other(_)` (mock-backend catch-all) | `BackendError` | | - `AnswerType::Secret` and no TTY available | `NoPromptBackend` | | - `AnswerType::Secret` and `target = "assistant"` | `AssistantRoutingDenied` - |\` | + | Originating event | `reason` | + | ------------------------------------------------------------------------------------------ | ------------------------ | + | `ExecutionEvent::PromptCancelled` (Esc, Ctrl-C, or EOF at the prompt) | `User` | + | `ExecutionEvent::PromptCancelled` (prompter failure: I/O error, no TTY) | `BackendError` | + | `InquiryError::Cancelled` (cancellation token fired: user restart, tool cancel, hard quit) | `User` | + | `InquiryError::Provider` | `BackendError` | + | `InquiryError::MissingStructuredData` | `BackendError` | + | `InquiryError::AnswerExtraction { .. }` | `BackendError` | + | `InquiryError::Other(_)` (mock-backend catch-all) | `BackendError` | + | `AnswerType::Secret` and no TTY available | `NoPromptBackend` | + | `AnswerType::Secret` and `target = "assistant"` | `AssistantRoutingDenied` | `InquiryError::Cancelled` returns `Err` from the inquiry backend today (see `crates/jp_cli/src/cmd/query/tool/inquiry.rs` around @@ -307,10 +347,10 @@ For every `Outcome::NeedsInput { question }`: cancellation — the token is cancelled by `interrupt/signals.rs` in response to user actions (`InterruptAction::RestartTool`, `ToolCancelled`, hard quit). Mapping it to `User` (not `BackendError`) keeps the audit trail honest. - Prompter I/O errors (e.g. EOF on stdin mid-prompt) are indistinguishable from - Ctrl-C at the coordinator's vantage today and follow the same `User` path; if - a future change distinguishes them, the right landing is a new - `CancellationReason` variant rather than re-using `BackendError`. + `ExecutionEvent::PromptCancelled` carries a reason mapped at the prompt site: + `inquire`'s cancel/interrupt errors are user actions, and every other + prompter error (I/O, no TTY, writer failure) is a backend failure recorded as + `BackendError`. `Cancelled` records that the inquiry was closed without an answer — for the audit trail only. @@ -324,10 +364,10 @@ variant rather than overloading `CancellationReason`. ### Secret questions -Some tool questions ask for values that must not be persisted on disk — SSH -passphrases, API keys, one-time tokens. +Some tool questions ask for values the user does not want written into the +conversation stream: SSH passphrases, API keys, one-time tokens. Recording the question text without the answer keeps the audit trail honest -without leaking the secret. +while keeping the answer out of the persisted inquiry event. `jp_tool::AnswerType` gains a new variant, `Secret`, for free-form text input whose answer must not be persisted: @@ -400,6 +440,34 @@ Tool authors MUST NOT set `default` to a sensitive literal on an The same machinery serves [RFD 083]'s `exclusive: true` flag (083 reuses these variants for its routing fail-fast paths). +**What `Secret` does and does not cover.** The guarantee is scoped to JP's own +handling of the inquiry: JP does not echo the prompt input, does not write the +answer into the `InquiryResponse` (it records `Redacted { id }`), and does not +store the answer in the `TurnState` turn-answer cache. +It does **not** redact the value from anywhere else it might reach disk: + +- **Tool output.** If a tool echoes the secret back in its success or error + text, that text is persisted verbatim in `ToolCallResponse`. + JP does not scan tool output for the answer. +- **Config values.** A secret supplied through `QuestionConfig.answer` lives in + the config file the user wrote it into, and can be persisted into the + conversation stream as a config delta, independent of the `Redacted` response. +- **`Question.default`** is propagated verbatim (see above) and is not redacted. +- **Logs** and any external tool behavior are out of scope. + +A "tainted value" model that tracks a secret across `ToolCallResponse`, config +deltas, and logs is a larger redaction design and belongs in its own RFD. + +**Static-answer shape is not validated.** `QuestionConfig.answer` is an +arbitrary JSON value, and 082 does not validate that a static answer for a +`Secret` question is a string (the same looseness already applies to `Boolean`, +`Select`, and `Text`). +Whatever is configured is delivered to the tool in memory and the response is +recorded as `Redacted` regardless of shape, so no secret leaks through a +malformed static answer. +Answer-shape validation (`CancellationReason::InvalidStaticAnswer`) arrives with +[RFD 083]; 082 does not pull it forward. + ### `InquiryResponse` serialization The widened `InquiryResponse` is a Rust enum that makes invalid @@ -552,7 +620,7 @@ Defined behavior: - **Turn renderer** (`crates/jp_cli/src/render/turn.rs`): continues to skip inquiry events entirely. Pretty rendering for `jp conversation show` is a non-goal here (see - [Non-Goals] (\#non-goals)). + [Non-Goals](#non-goals)). Any other reader that pattern-matches on `InquiryResponse` MUST handle all variants. @@ -597,9 +665,9 @@ pub struct TurnState { The cache-key shapes (`.` and `.__permission__`) are deliberately distinct from the -stream-correlation `InquiryId` format defined in [Inquiry ID format] -(\#inquiry-id-format) — today's `TurnState` uses `InquiryId` for both, which -blurs the two roles. +stream-correlation `InquiryId` format defined in [Inquiry ID +format](#inquiry-id-format) — today's `TurnState` uses `InquiryId` for both, +which blurs the two roles. Implementation may introduce dedicated newtypes per map to lift this distinction into the type system; 082 does not require it. @@ -634,9 +702,14 @@ The tradeoff buys uniform recording: every question/answer exchange produces an `InquiryRequest`/`InquiryResponse` pair regardless of how the answer is resolved. -**This is a behavior change for user-supplied local tools.** Built-in and MCP -tools are unaffected (built-ins handle `answers` through the explicit two-call -`NeedsInput → Success` pattern; MCP tools never see `answers`). +**This is a behavior change for user-supplied local tools.** MCP tools are +structurally unaffected: `execute_mcp` never receives the `answers` map. +Built-in tools *do* receive the pre-seeded `answers` map today, through +`BuiltinTool::execute(&self, arguments, answers)`, so removing the pre-seed +changes what a built-in sees on its first call. +Current in-tree built-ins use the two-call `NeedsInput → Success` pattern and +do not read `answers` on first call, but that is a property to verify, not a +structural guarantee (see the built-in audit bullet in Phase 3). Local tools receive the accumulated answers in the JSON context under `"tool": { "answers": answers, ... }` (`crates/jp_llm/src/tool.rs` around line 719). Today, `QuestionConfig.answer` is documented as "the question will not be @@ -711,6 +784,18 @@ handler would require the handler to know the ID format, which conflicts with the "treat `InquiryId` as opaque" rule in [Inquiry ID format](#inquiry-id-format). +The cancellation-reason mapping in [Recording lifecycle](#recording-lifecycle) +requires the typed `InquiryError` at the recording site. +`ExecutionEvent::InquiryResult.result` is `Result` today, and +`spawn_inquiry` stringifies the error (`.map_err(|e| e.to_string())`) before the +coordinator sees it, so the variant-to-`CancellationReason` mapping is +impossible without parsing rendered text. +Widen the event to carry `Result` (or a +`CancellationReason` mapped at the `spawn_inquiry` site); the reason is derived +from the error variant, never from the message. +The model-facing `ToolCallResponse` text is rendered separately and is +unchanged. + ## Drawbacks - **Stream-size growth.** Adds two `InquiryRequest`/`InquiryResponse` events per @@ -722,13 +807,16 @@ format](#inquiry-id-format). - **Static-answer contract change for user-supplied local tools.** Removing the pre-seed optimization means tools with a configured static answer execute twice (emit `NeedsInput`, then receive the injected answer) instead of once. - JP's own built-in and MCP tools are unaffected, but local tools that today - read `tool.answers` on first invocation and proceed without emitting - `NeedsInput` will no longer see the static answer on that first call — a real - contract change for that authoring pattern. + JP's own built-ins and MCP tools are affected differently: MCP is structurally + immune (`execute_mcp` receives no `answers` map), while built-ins receive the + map today and are unaffected only because current in-tree built-ins do not + read it on first call (a property to audit, not a guarantee). + Local tools that today read `tool.answers` on first invocation and proceed + without emitting `NeedsInput` will no longer see the static answer on that + first call — a real contract change for that authoring pattern. Tools with side effects before their first `NeedsInput` will run those side effects one extra time per configured static answer. - See the [Pre-seed removal] (\#pre-seed-removal) subsection for migration + See the [Pre-seed removal](#pre-seed-removal) subsection for migration guidance. - **Touches existing tool-question call sites.** The unified recording affects any tool that emits `Outcome::NeedsInput`, not just `ask_user`. @@ -749,12 +837,25 @@ format](#inquiry-id-format). Downstream `match`es on the enum (the prompter, the editor renderer, any code that branches on answer types) need a new arm. `jp_tool::AnswerType` also gains `#[serde(tag = "type", rename_all = - "snake_case")]` to align its wire shape with the already-internally- tagged - `InquiryAnswerType` (and with this RFD's local-tool wire examples). - The persisted shape for non-secret questions is unchanged. - Existing in-tree tools build `Question` values through `jp_tool`'s - constructors rather than hand-encoding JSON, so the wire-shape change is - transparent to them. + "snake_case")]` so its wire shape matches `InquiryAnswerType` and this RFD's + local-tool wire examples. + This **changes the local-tool stdout protocol**: `answer_type` shifts from the + externally-tagged shape (`"Text"`, `{"Select": {"options": [...]}}`) to the + internally-tagged shape (`{"type": "text"}`, `{"type": "select", "options": + [...]}`). + It ships as a clean cutover with no read-both compatibility shim, which is + acceptable because JP has no external local-tool authors today; the only + consumer is the in-tree `.config/jp/tools` crate, whose tools serialize + `Question` values *through* `jp_tool` rather than hand-encoding JSON, so their + emitted shape updates with the type and stays self-consistent. + Persisted conversation streams are unaffected: the stream stores + `InquiryAnswerType` (already internally tagged), produced by converting + `AnswerType` in `tool_question_to_inquiry_question`, so `jp_tool::AnswerType` + never crosses the on-disk boundary and reading pre-082 streams does not + change. + At implementation time, grep for any persisted local-tool stdout fixtures (raw + JSON, not constructed via `Outcome`) before flipping the tag; the cutover must + update those. Old streams cannot contain `Secret` answers, so backward compat on read is trivial. - **`PromptBackend` gains a no-echo input method.** A new method on the trait @@ -868,12 +969,16 @@ counter bounded per-turn rather than universally. opt in by declaring the question's answer type as `Secret`. The persisted response is `Redacted` rather than `Answered`, the prompter does not echo input, and routing to the inquiry backend (no-TTY fallback or `target - = "assistant"`) is refused with a tool-level error. 082 does not cover every - shape of sensitive content — e.g. a text answer that happens to contain a - token without the question being declared `Secret`, or partial sensitivity - inside an otherwise-archival answer. - A future RFD may add richer redaction policies if real-world use surfaces - them. + = "assistant"`) is refused with a tool-level error. + The guarantee is scoped to JP's own handling of the inquiry; it does not + redact the value from tool output (`ToolCallResponse`), config values or + deltas (a secret set via `QuestionConfig.answer`), `Question.default`, or logs + (see [What `Secret` does and does not cover](#secret-questions)). 082 also + does not cover sensitive content that was never declared `Secret` (e.g. a text + answer that happens to contain a token, or partial sensitivity inside an + otherwise-archival answer). + A future RFD may add richer redaction policies (e.g. tainted-value tracking) + if real-world use surfaces them. - **Stream-size growth for noisy tools.** Tools that prompt frequently produce more persisted bytes after this RFD. The events are small and the growth is bounded, but conversations dominated by @@ -908,8 +1013,10 @@ variant. Implement the custom `Serialize`/`Deserialize` for `CancellationReason` so unrecognized tags round-trip as `Unknown(s)` preserving `s` verbatim. Implement the custom `Deserialize` for `InquiryResponse` that accepts the legacy -untagged form as `Answered` and defaults a missing `reason` on a tagged -`Cancelled` event to `User`. +untagged form as `Answered` and maps a missing (or non-string) `reason` on a +tagged `Cancelled` event to `Unknown` with the sentinel tag `unspecified` — +such a record carries no reason, and fabricating a specific one (e.g. `User`) +would put a false claim in the audit trail. Split `TurnState.persisted_inquiry_responses` into `remembered_tool_answers: IndexMap` and `remembered_permission_decisions: IndexMap` to + carry the typed `InquiryError` (or a `CancellationReason` mapped at the + `spawn_inquiry` site) so the cancellation reason is derived from the error + variant rather than from stringified error text (see [Coordinator + plumbing](#coordinator-plumbing)). Inquiry ID format: @@ -979,6 +1097,19 @@ Inquiry ID format: recorded under that key; first recording is attempt `1`. The counter resets only at turn boundaries (i.e. when a fresh `TurnState` is built). +- Introduce a `QuestionId` newtype in `jp_tool` (wrapping `Question.id`) whose + `FromStr`/`TryFrom` validation rejects a dot, and route local-tool stdout and + built-in question construction through it so an invalid id cannot exist past + the boundary. + On a `NeedsInput` whose `question.id` is invalid, log an `error!` trace and + fail the tool with a tool-level error response (no chrome line; the error + renders via `ErrorStyleConfig`), then drop the outcome before constructing any + inquiry event. + Ensure the `serde_json::from_str::` ingestion path distinguishes an + invalid `question.id` from non-`Outcome` output so the diagnostic is not + swallowed by the `RawOutput` fallback. + This is what makes the three-segment join injective (see [Inquiry ID + format](#inquiry-id-format)). - Construct `InquiryRequest.id` as `..` at the recording site. The matching `InquiryResponse.id` uses the same value. @@ -991,10 +1122,19 @@ Inquiry ID format: (`crates/jp_conversation/src/stream.rs` around `remove_orphaned_inquiry_responses`), which can cross-satisfy pairs across turns once `tool_call_id` (and therefore `InquiryId`) reuse becomes possible. - Orphan removal and any synthetic-response injection operate within each turn's - event window instead. + 082 keeps the existing behavior — an orphaned `InquiryRequest` or + `InquiryResponse` is *removed* — and only narrows the ID-set scope to the + containing turn's event window. 082 does not inject synthetic inquiry + responses: an orphan left by a crash, truncation, or manual edit is not a + cancellation, so writing a synthetic `Cancelled` (which would have to claim + some `reason`) would put a false record on disk. The same change applies to any other code that builds an `InquiryId` index over the full stream. + If [RFD 023] lands, incomplete-tail extraction runs before `sanitize()`, so + this per-turn orphan removal only ever applies to complete turns; the + incomplete turn's unpaired inquiry events are carried into the + `IncompleteTurn` rather than removed. (023 is Discussion; this is + forward-compatible, not a dependency.) Secret-question handling: @@ -1004,9 +1144,12 @@ Secret-question handling: `{"type": "select", "options": [...]}`, `{"type": "text"}`, `{"type": "secret"}`) matches `InquiryAnswerType`. Existing in-tree tools build `Question` values through `jp_tool`'s - constructors, so the wire-shape change is transparent. - Add a matching `Question::secret(text: String) -> Self` constructor in - `jp_tool` alongside `Question::text`/`boolean`/`select`. + constructors, so the wire-shape change is transparent to them; for external + local tools this is a breaking change to the stdout protocol, shipped without + a compat shim (see Drawbacks). + Add a matching `Question::secret(id, text) -> Result` + constructor in `jp_tool` alongside `Question::text`/`boolean`/`select` (all + four validate the id through `QuestionId`). No `secret: bool` field is added to `Question`. - Add the matching `Secret` variant to `jp_conversation::event::inquiry::InquiryAnswerType` (which is already @@ -1027,8 +1170,11 @@ Secret-question handling: In the static-answer short-circuit, apply the configured value to the in-memory `accumulated_answers` but record `InquiryResponse::Redacted { id }` instead of `Answered`. - On a successful prompter or inquiry-backend answer for a `Secret` question, - record `InquiryResponse::Redacted { id }` in place of `Answered`. + On a successful prompter answer for a `Secret` question, record + `InquiryResponse::Redacted { id }` in place of `Answered`. + The inquiry-backend path is guarded before routing (below) and must never + receive a `Secret` question; treat its arrival there as an internal invariant + violation. - Enforce the routing guard before routing a `Secret` question: - **No TTY available**: synthesize a tool-level error response and record `InquiryResponse::Cancelled { reason: NoPromptBackend }`. @@ -1119,7 +1265,10 @@ Independent of the others, lands alongside Phase 3 in the same PR. RFD 023 assumes user-targeted `InquiryRequest`/`InquiryResponse` pairs are on disk so incomplete turns blocked on inquiries can be detected and resumed. RFD 005's prompter carve-out leaves that assumption unmet; 082 closes the gap. - RFD 023's `Requires` needs updating to reference 082 at 082 promotion time. + If 023 is confirmed to depend on 082, `just rfd-require` records the link on + both sides at promotion time (082's `Required by` and 023's `Requires`); it is + deliberately not hand-written into the metadata while both RFDs are in + Discussion. - [RFD 028] — the structured inquiry system this RFD's recording layer builds on. - [RFD 034] — defines the current `QuestionTarget` shape.