Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/jp_attachment_bear_note/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Fetch a single note by its unique identifier:
```sh
# You can copy the note ID using <kbd>⌥⇧⌘I</kbd>
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:
Expand Down
3 changes: 3 additions & 0 deletions crates/jp_attachment_bear_note/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ fn uri_to_query(uri: &Url) -> Result<Query, Box<dyn Error + Send + Sync>> {

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()),
};

Expand Down
7 changes: 7 additions & 0 deletions crates/jp_attachment_bear_note/src/lib_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())),
Expand Down
82 changes: 82 additions & 0 deletions crates/jp_cli/src/cmd/conversation/print_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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![
Expand Down
45 changes: 28 additions & 17 deletions crates/jp_cli/src/cmd/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Option<String>, Box<dyn std::error::Error + Send + Sync>> {
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)?,
};

Expand Down Expand Up @@ -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<String> {
// 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()
Expand Down
22 changes: 11 additions & 11 deletions crates/jp_cli/src/cmd/query/tool/pending.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -110,18 +107,21 @@ pub(crate) struct ExecutionPlan {
items: Vec<PlanItem>,

/// 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<ToolCallRequest>,
/// 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<PlanItem>, Vec<ToolCallRequest>) {
pub(crate) fn into_parts(self) -> (Vec<PlanItem>, Vec<(usize, ToolCallRequest)>) {
(self.items, self.orphaned)
}

Expand Down Expand Up @@ -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())),
}
}

Expand Down
41 changes: 40 additions & 1 deletion crates/jp_cli/src/cmd/query/tool/pending_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
15 changes: 11 additions & 4 deletions crates/jp_cli/src/cmd/query/turn/coordinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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();
}

Expand Down
Loading
Loading