diff --git a/crates/jp_attachment_bear_note/README.md b/crates/jp_attachment_bear_note/README.md index 23950534..4dca0c38 100644 --- a/crates/jp_attachment_bear_note/README.md +++ b/crates/jp_attachment_bear_note/README.md @@ -13,6 +13,9 @@ Fetch a single note by its unique identifier: ```sh # You can copy the note ID using ⌥⇧⌘I jp attachment add "bear://get/2356A6D7-49D7-4818-8E37-3E02D1B95146" + +# Shorthand for the same: +jp attachment add "bear:2356A6D7-49D7-4818-8E37-3E02D1B95146" ``` Fetch a list of notes based on a search query: diff --git a/crates/jp_attachment_bear_note/src/lib.rs b/crates/jp_attachment_bear_note/src/lib.rs index 7a26156f..f2ba261b 100644 --- a/crates/jp_attachment_bear_note/src/lib.rs +++ b/crates/jp_attachment_bear_note/src/lib.rs @@ -186,6 +186,9 @@ fn uri_to_query(uri: &Url) -> Result> { Query::Search { query: path, tags } } + // Shorthand: `bear:NOTE_ID`. Opaque form (no `//`) has no host; + // the path holds the note id directly. Mirrors the gh handler. + None if !path.is_empty() => Query::Get(path), _ => return Err("Invalid bear note query".into()), }; diff --git a/crates/jp_attachment_bear_note/src/lib_tests.rs b/crates/jp_attachment_bear_note/src/lib_tests.rs index babebae4..ca29c52f 100644 --- a/crates/jp_attachment_bear_note/src/lib_tests.rs +++ b/crates/jp_attachment_bear_note/src/lib_tests.rs @@ -30,6 +30,13 @@ fn test_uri_to_query() { Ok(Query::Get("123-456".to_string())), ), ("bear://get/1", Ok(Query::Get("1".to_string()))), + ( + "bear:F70FD86D-752F-403D-A517-BF020591F530", + Ok(Query::Get( + "F70FD86D-752F-403D-A517-BF020591F530".to_string(), + )), + ), + ("bear:", Err("Invalid bear note query".to_string())), ( "bear://get/tag%20%231", Ok(Query::Get("tag #1".to_string())), diff --git a/crates/jp_cli/src/cmd/conversation/print_tests.rs b/crates/jp_cli/src/cmd/conversation/print_tests.rs index 2ccc1319..207c0c72 100644 --- a/crates/jp_cli/src/cmd/conversation/print_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/print_tests.rs @@ -282,6 +282,88 @@ fn prints_structured_data() { assert!(output.contains("```json"), "got: {output}"); } +/// Regression: replay must close the structured `json` fence. Before this +/// fix, `TurnRenderer::flush()` only flushed the chat sub-renderer, so a +/// conversation ending on a `ChatResponse::Structured` printed an opening +/// ```json with no matching close. +#[test] +fn structured_fence_is_closed_at_end_of_replay() { + let data = json!({"name": "Alice"}); + let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + ChatResponse::structured(data), + ts(0, 0, 0), + )]); + + let print = Print { + target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), + last: None, + turn: None, + current_config: false, + style: None, + }; + let h = ctx.workspace.acquire_conversation(&id).unwrap(); + print.run(&mut ctx, &[h]).unwrap(); + ctx.printer.flush(); + + let output = strip_ansi(&out.lock()); + let opens = output.matches("```json").count(); + // Closing fence is a backtick triple at line start with nothing after. + let closes = output.lines().filter(|l| l.trim_end() == "```").count(); + assert_eq!( + opens, closes, + "opening and closing ``` fences must match, got opens={opens} closes={closes}, output: \ + {output:?}" + ); +} + +/// Regression: a message following a structured response in the same +/// conversation must render OUTSIDE the `json` fence — the fence is +/// closed at the role/content boundary, not left open until end-of-stream. +#[test] +fn structured_response_followed_by_message_closes_fence_first() { + let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + ConversationEvent::new(TurnStart, ts(0, 0, 0)), + ConversationEvent::new(ChatRequest::from("Extract"), ts(0, 0, 1)), + ConversationEvent::new( + ChatResponse::structured(json!({"name": "Alice"})), + ts(0, 0, 2), + ), + ConversationEvent::new(TurnStart, ts(0, 1, 0)), + ConversationEvent::new(ChatRequest::from("Thanks"), ts(0, 1, 1)), + ConversationEvent::new(ChatResponse::message("You're welcome.\n\n"), ts(0, 1, 2)), + ]); + + let print = Print { + target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), + last: None, + turn: None, + current_config: false, + style: None, + }; + let h = ctx.workspace.acquire_conversation(&id).unwrap(); + print.run(&mut ctx, &[h]).unwrap(); + ctx.printer.flush(); + + let output = strip_ansi(&out.lock()); + + // The trailing message text must not appear before the JSON fence has + // been closed. Find the index of the JSON close (a `\`\`\`` line + // standing alone) that follows the opening `\`\`\`json`, and assert + // the message body shows up only after it. + let open_idx = output.find("```json").expect("expected an opening fence"); + let close_idx = output[open_idx..] + .find("\n```") + .map(|i| open_idx + i) + .expect("expected a closing fence after the opening one"); + let welcome_idx = output + .find("You're welcome.") + .expect("expected the trailing message text"); + assert!( + welcome_idx > close_idx, + "trailing message must render after the JSON fence is closed; output: {output:?}" + ); +} + #[test] fn turn_separators_between_turns() { let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ diff --git a/crates/jp_cli/src/cmd/init.rs b/crates/jp_cli/src/cmd/init.rs index c7562972..65f432fd 100644 --- a/crates/jp_cli/src/cmd/init.rs +++ b/crates/jp_cli/src/cmd/init.rs @@ -135,17 +135,21 @@ impl Init { /// Prompt the user for their display name, pre-filled with the best /// available default (see [`detect_default_user_name`]). Returns `None` /// when the user submits an empty value. + /// + /// The default is set as an *initial value* (editable), not as an + /// `inquire` default (substituted on empty submission), so the user can + /// clear the field and submit empty to skip attribution. fn ask_user_name( writer: &mut dyn io::Write, ) -> Result, Box> { let prompt = Text::new("Your name for conversations:").with_help_message( - "Stamped onto each message you send. Press Enter to accept the default, or leave \ - blank for the generic 'user' label.", + "Stamped onto each message you send. Press Enter to accept the pre-filled value, or \ + clear the field for the generic 'user' label.", ); - let default = detect_default_user_name(); - let answer = match default.as_deref() { - Some(d) => prompt.with_default(d).prompt_with_writer(writer)?, + let initial = detect_default_user_name(); + let answer = match initial.as_deref() { + Some(v) => prompt.with_initial_value(v).prompt_with_writer(writer)?, None => prompt.prompt_with_writer(writer)?, }; @@ -319,26 +323,33 @@ impl Init { /// /// Cascades through: /// -/// 1. `git config --get user.name` — typically the user's real name, set -/// once and reused across tools. -/// 2. `$USER` (Unix) or `$USERNAME` (Windows) — the system login name, +/// 1. `git config --global --get user.name` — the user's stable global +/// identity. Preferred because the picked-up value is persisted to +/// user-global JP config and inherited by every future workspace; a +/// repo-local override would otherwise leak into unrelated workspaces. +/// 2. `git config --get user.name` (no scope) — falls back to whatever +/// git resolves in the current directory, for users who only have a +/// repo-local identity. +/// 3. `$USER` (Unix) or `$USERNAME` (Windows) — the system login name, /// last-resort fallback. /// /// Returns `None` when nothing is available so the prompt renders without /// a pre-filled value. fn detect_default_user_name() -> Option { - // 1. git config - if let Ok(out) = cmd!("git", "config", "--get", "user.name") - .stderr_null() - .read() - { - let trimmed = out.trim(); - if !trimmed.is_empty() { - return Some(trimmed.to_owned()); + // 1. git config (global), 2. git config (any scope). + for args in [ + ["config", "--global", "--get", "user.name"].as_slice(), + ["config", "--get", "user.name"].as_slice(), + ] { + if let Ok(out) = cmd("git", args).stderr_null().read() { + let trimmed = out.trim(); + if !trimmed.is_empty() { + return Some(trimmed.to_owned()); + } } } - // 2. system login name ($USER on Unix, $USERNAME on Windows). + // 3. system login name ($USER on Unix, $USERNAME on Windows). env::var("USER") .or_else(|_| env::var("USERNAME")) .ok() diff --git a/crates/jp_cli/src/cmd/query/tool/pending.rs b/crates/jp_cli/src/cmd/query/tool/pending.rs index 7384d776..581cae9e 100644 --- a/crates/jp_cli/src/cmd/query/tool/pending.rs +++ b/crates/jp_cli/src/cmd/query/tool/pending.rs @@ -17,9 +17,6 @@ //! the stream first or add a new public API method, which is the visible //! smell that earlier conventions lacked. //! -//! See also: the `JP refactor` Bear note for the prep-flow unification -//! follow-up. -//! //! [`ToolCallResponse`]: jp_conversation::event::ToolCallResponse use std::collections::HashMap; @@ -110,18 +107,21 @@ pub(crate) struct ExecutionPlan { items: Vec, /// Tool-call requests that appear in the stream's current turn without a - /// matching response AND without a matching entry in `PendingTools`. - /// Should be empty in correct operation; a non-empty vec signals a - /// contract violation (some path added a `ToolCallRequest` to the stream - /// without going through the streaming-phase preparation flow). The - /// caller decides what to do — synthesize an error response, log, etc. - orphaned: Vec, + /// matching response AND without a matching entry in `PendingTools`, + /// paired with the plan index they would have occupied in document + /// order. Should be empty in correct operation; a non-empty vec signals + /// a contract violation (some path added a `ToolCallRequest` to the + /// stream without going through the streaming-phase preparation flow). + /// The caller decides what to do — synthesize an error response, log, + /// etc. — but MUST preserve the carried index when committing a + /// response so stream order is retained. + orphaned: Vec<(usize, ToolCallRequest)>, } impl ExecutionPlan { /// Decompose the plan into its parts. Consumes the plan; there's no /// way to read it twice. - pub(crate) fn into_parts(self) -> (Vec, Vec) { + pub(crate) fn into_parts(self) -> (Vec, Vec<(usize, ToolCallRequest)>) { (self.items, self.orphaned) } @@ -177,7 +177,7 @@ pub(crate) fn build_execution_plan( request: request.clone(), work, }), - None => orphaned.push(request.clone()), + None => orphaned.push((index, request.clone())), } } diff --git a/crates/jp_cli/src/cmd/query/tool/pending_tests.rs b/crates/jp_cli/src/cmd/query/tool/pending_tests.rs index 7fe52425..2294e659 100644 --- a/crates/jp_cli/src/cmd/query/tool/pending_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/pending_tests.rs @@ -99,6 +99,44 @@ fn responded_requests_are_skipped() { assert_eq!(items[0].index, 0); } +/// Orphans interleaved with non-orphan items must keep the plan index +/// they would have occupied in document order. The downstream +/// `commit_tool_responses` sorts by that index, so any other choice would +/// reorder responses relative to their requests in the persisted stream. +#[test] +fn orphan_index_matches_stream_position() { + let mut stream = ConversationStream::new_test(); + stream.start_turn(ChatRequest::from("user")); + stream + .current_turn_mut() + .add_tool_call_request(req("a", "tool_a")) + .add_tool_call_request(req("orphan", "tool_orphan")) + .add_tool_call_request(req("b", "tool_b")) + .build() + .unwrap(); + + let mut pending = PendingTools::new(); + pending.insert_approved("a".into(), approved_executor("a", "tool_a")); + pending.insert_approved("b".into(), approved_executor("b", "tool_b")); + // No entry for `orphan` — it falls through to the orphaned vec. + + let plan = build_execution_plan(&stream, &mut pending); + let (items, orphaned) = plan.into_parts(); + + assert_eq!(items.len(), 2); + assert_eq!(items[0].request.id, "a"); + assert_eq!(items[0].index, 0); + assert_eq!(items[1].request.id, "b"); + assert_eq!(items[1].index, 2); + + assert_eq!(orphaned.len(), 1); + assert_eq!( + orphaned[0].0, 1, + "orphan must keep its document-order index" + ); + assert_eq!(orphaned[0].1.id, "orphan"); +} + /// A request in the stream without a matching pending entry is reported as /// orphaned. In normal operation this should never happen — the streaming /// phase always populates pending for every request it adds. Treating it @@ -121,7 +159,8 @@ fn unmatched_request_is_reported_as_orphaned() { assert!(items.is_empty()); assert_eq!(orphaned.len(), 1); - assert_eq!(orphaned[0].id, "ghost"); + assert_eq!(orphaned[0].0, 0); + assert_eq!(orphaned[0].1.id, "ghost"); } /// The plan walks only the current (most recent) turn. Tool calls from a diff --git a/crates/jp_cli/src/cmd/query/turn/coordinator.rs b/crates/jp_cli/src/cmd/query/turn/coordinator.rs index fbf11d64..116c444b 100644 --- a/crates/jp_cli/src/cmd/query/turn/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/turn/coordinator.rs @@ -216,7 +216,7 @@ impl TurnCoordinator { for event in self.event_builder.drain() { self.push_event(stream, event); } - self.view.flush_all(); + self.view.flush(); self.transition_from_streaming(stream, reason) } @@ -367,12 +367,19 @@ impl TurnCoordinator { self.push_event(conversation_stream, ChatResponse::message(&partial)); } - // Add user's reply as a new request - self.push_event(conversation_stream, ChatRequest { + // Add user's reply as a new request, then render it through + // the view so the live terminal gets the same labeled + // user header replay would emit for this `ChatRequest`. + // `render_user_request` also resets the assistant-header + // gate, so the next assistant chunk will print a fresh + // `── jp …` header. + let request = ChatRequest { content, schema: None, author: self.author.clone(), - }); + }; + self.view.render_user_request(&request); + self.push_event(conversation_stream, request); self.prepare_continuation(); } diff --git a/crates/jp_cli/src/cmd/query/turn/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/turn/coordinator_tests.rs index 2b156db9..2ce5c131 100644 --- a/crates/jp_cli/src/cmd/query/turn/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn/coordinator_tests.rs @@ -5,6 +5,13 @@ use jp_printer::{OutputFormat, Printer}; use serde_json::{Map, json}; use super::{super::state::TurnState, *}; +use crate::cmd::query::interrupt::InterruptAction; + +/// Strip ANSI escape codes for readable assertions on captured stdout. +fn strip_ansi(s: &str) -> String { + let bytes = strip_ansi_escapes::strip(s); + String::from_utf8(bytes).expect("valid utf-8 after stripping ANSI") +} #[test] fn test_transitions_to_executing_on_tool_call() { @@ -418,3 +425,89 @@ fn test_structured_not_routed_to_chat_renderer() { "Expected code fence, got: {output:?}" ); } + +/// Regression: if the user interrupts before any chunk has arrived and +/// chooses Continue, the next assistant event MUST emit a fresh role +/// header. The previous behaviour forced `assistant_header_rendered = +/// true` in `reset_for_continuation`, which suppressed the header +/// unconditionally and left the resumed output without a `── jp …` +/// boundary. +#[test] +fn interrupt_continue_before_first_chunk_emits_assistant_header_on_resume() { + let mut stream = ConversationStream::new_test(); + let (printer, out, _) = Printer::memory(OutputFormat::Text); + let printer = Arc::new(printer); + let mut coordinator = TurnCoordinator::new( + Arc::clone(&printer), + AppConfig::new_test().style, + None, + None, + Some("anthropic/test".into()), + ); + + coordinator.start_turn(&mut stream, ChatRequest::from("hello")); + + // No chunks have arrived yet — the assistant header has NOT been emitted. + coordinator.handle_streaming_interrupt(InterruptAction::Continue, &mut stream); + + // Resumed cycle delivers its first chunk. + coordinator.handle_event(&mut stream, Event::message(0, "hi there")); + coordinator.handle_event(&mut stream, Event::flush(0)); + coordinator.handle_event(&mut stream, Event::Finished(FinishReason::Completed)); + + printer.flush(); + let output = strip_ansi(&out.lock()); + assert!( + output.contains("\u{2500}\u{2500} jp "), + "resumed assistant content must be preceded by a `── jp` header, got: {output:?}" + ); +} + +/// Regression: a Reply interrupt inserts a new `ChatRequest` boundary, +/// which in replay would render a labeled user header AND a fresh +/// assistant header for the following content. Live mode must match. +#[test] +fn interrupt_reply_renders_user_header_for_new_request() { + let mut stream = ConversationStream::new_test(); + let (printer, out, _) = Printer::memory(OutputFormat::Text); + let printer = Arc::new(printer); + let mut coordinator = TurnCoordinator::new( + Arc::clone(&printer), + AppConfig::new_test().style, + Some("alice".into()), + None, + Some("anthropic/test".into()), + ); + + coordinator.start_turn(&mut stream, ChatRequest::from("first question")); + + // Some assistant content arrives so the assistant header is emitted. + coordinator.handle_event(&mut stream, Event::message(0, "partial answer")); + + // User interrupts with a follow-up reply. + coordinator.handle_streaming_interrupt( + InterruptAction::Reply("actually, ignore that".into()), + &mut stream, + ); + + // Resumed cycle delivers the new assistant content. + coordinator.handle_event(&mut stream, Event::message(1, "new answer")); + coordinator.handle_event(&mut stream, Event::flush(1)); + coordinator.handle_event(&mut stream, Event::Finished(FinishReason::Completed)); + + printer.flush(); + let output = strip_ansi(&out.lock()); + + // The labeled user header for the Reply must show up between the + // partial answer and the resumed assistant content. + let alice_idx = output + .find("\u{2500}\u{2500} alice ") + .unwrap_or_else(|| panic!("expected a `── alice` header for the Reply, got: {output:?}")); + + // And a fresh assistant header must follow the user boundary. + let after_alice = &output[alice_idx..]; + assert!( + after_alice.contains("\u{2500}\u{2500} jp "), + "expected a fresh `── jp` header after the Reply, got: {output:?}" + ); +} diff --git a/crates/jp_cli/src/cmd/query/turn_loop.rs b/crates/jp_cli/src/cmd/query/turn_loop.rs index 49e18d31..40fa4113 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop.rs @@ -490,10 +490,9 @@ pub(super) async fn run_turn_loop( // the unified executing path below picks up. // // The streaming and restart prep flows are still two - // separate codepaths today (see Bear note: "unify the - // streaming and restart tool-prep codepaths"). Both - // converge on `pending_tools` and `build_execution_plan`, - // which is the load-bearing invariant for this refactor. + // separate codepaths today; both converge on + // `pending_tools` and `build_execution_plan`, which is the + // load-bearing invariant for this refactor. if restart_requested { restart_requested = false; @@ -572,15 +571,14 @@ pub(super) async fn run_turn_loop( // an error response so the conversation stays valid (every // request must have a response before the next provider // call) and surface the inconsistency. - for req in orphaned { + for (idx, req) in orphaned { warn!( id = %req.id, name = %req.name, "ToolCallRequest in stream without a pending entry; synthesizing error \ response.", ); - let next_idx = approved.len() + pre_resolved.len(); - pre_resolved.push((next_idx, ToolCallResponse { + pre_resolved.push((idx, ToolCallResponse { id: req.id, result: Err( "Tool call had no prepared executor (internal inconsistency).".into(), diff --git a/crates/jp_cli/src/render/turn_view.rs b/crates/jp_cli/src/render/turn_view.rs index 3ad7bd73..dcd4274e 100644 --- a/crates/jp_cli/src/render/turn_view.rs +++ b/crates/jp_cli/src/render/turn_view.rs @@ -67,8 +67,11 @@ impl TurnView { } /// Mark the start of a new turn. The next assistant event will emit a - /// fresh role header. + /// fresh role header. Closes any open structured fence so a turn that + /// ended on a `ChatResponse::Structured` doesn't bleed into the next + /// turn's content. pub fn begin_turn(&mut self) { + self.structured.flush(); self.assistant_header_rendered = false; } @@ -76,6 +79,9 @@ impl TurnView { /// body. Resets assistant-header gating so the next assistant event /// emits a fresh header. pub fn render_user_request(&mut self, req: &ChatRequest) { + // Close any open structured fence before the user header so the + // boundary marker isn't rendered inside a `json` block. + self.structured.flush(); let label = req.author.as_deref().unwrap_or(DEFAULT_USER_LABEL); self.chat.render_role_header(label, None); self.chat.render_request(&req.content); @@ -86,13 +92,17 @@ impl TurnView { /// role header first if it hasn't been emitted yet for this turn. /// /// Dispatches structured responses to the structured renderer and - /// everything else (messages, reasoning) to the chat renderer. + /// everything else (messages, reasoning) to the chat renderer. A + /// non-structured response after structured content closes the open + /// `json` fence first; a structured response after non-structured + /// content flushes the chat buffer first. pub fn render_chat_response(&mut self, resp: &ChatResponse) { self.ensure_assistant_header(); if resp.is_structured() { self.chat.flush(); self.structured.render_chunk(resp); } else { + self.structured.flush(); self.chat.render_response(resp); } } @@ -101,6 +111,10 @@ impl TurnView { /// /// Emits the assistant header if not already shown, then flushes the /// chat buffer so surrounding messages render as distinct paragraphs. + /// Also closes any open structured fence — a tool call after + /// structured output is a content boundary that must not stay inside + /// the `json` block. + /// /// `hidden` controls whether the chat renderer transitions into the /// `ToolCall` content kind: passing `true` keeps the boundary /// invisible (suitable for hidden tool calls so the next message @@ -108,22 +122,20 @@ impl TurnView { /// where tool UI follows. pub fn enter_tool_call(&mut self, hidden: bool) { self.ensure_assistant_header(); + self.structured.flush(); self.chat.flush(); if !hidden { self.chat.transition_to_tool_call(); } } - /// Flush pending chat output. - pub fn flush(&mut self) { - self.chat.flush(); - } - - /// Flush pending output for both chat and structured renderers. + /// Flush pending output across both chat and structured renderers. /// - /// Used at the end of a streaming cycle so any unclosed JSON code - /// fence gets its closing fence before the next phase starts. - pub fn flush_all(&mut self) { + /// Closes any open `json` fence and drains any buffered chat content. + /// Safe to call at any boundary; in particular, replay's final flush + /// after the last turn relies on this to terminate a trailing + /// structured response. + pub fn flush(&mut self) { self.chat.flush(); self.structured.flush(); } @@ -131,13 +143,15 @@ impl TurnView { /// Reset internal renderer state, discarding partial buffers. /// /// Used when a streaming cycle is interrupted and a new one begins - /// (e.g. interrupt-with-prefill). Crucially, this leaves the - /// assistant-header flag set: the continuation is part of the same - /// turn, so re-emitting the header would be a duplicate. + /// (e.g. interrupt-with-prefill). Preserves the existing + /// `assistant_header_rendered` flag: if a header was already on the + /// terminal before the interrupt, the continuation is part of the + /// same assistant turn and must not re-emit it; if no header had been + /// rendered yet (the user interrupted before the first chunk), the + /// flag stays `false` and the next assistant event will emit one. pub fn reset_for_continuation(&mut self) { self.chat.reset(); self.structured.reset(); - self.assistant_header_rendered = true; } /// Replace the underlying renderers and identity. @@ -145,7 +159,9 @@ impl TurnView { /// Used by replay's per-turn config rebuild when the conversation's /// historical config differs from the workspace's current config. /// Header gating state is preserved (the rebuild itself doesn't open - /// a new turn). + /// a new turn). Flushes both sub-renderers before swapping them out so + /// any open `json` fence or buffered chat output is committed before + /// the new instances take over. pub fn reconfigure( &mut self, printer: Arc, @@ -153,6 +169,7 @@ impl TurnView { assistant_name: Option, model_id: Option, ) { + self.flush(); self.chat = ChatRenderer::new(printer.clone(), style); self.structured = StructuredRenderer::new(printer); self.assistant_name = assistant_name;