From 9df67b98926d2efb6f5d19c6aab3657dbb886c26 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Mon, 7 Sep 2026 17:56:33 -0700 Subject: [PATCH 1/5] test: keep ui tests out of the real feedback archive The app tests opened App against the developer's own Plannotator data directory, and the send test's Discard delivery succeeds, so every run appended a record to the real feedback archive. Point each app under test at a fresh temp data dir and assert the send lands there. --- crates/plannotator-tui/src/app/tests.rs | 43 ++++++++++++++++++------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/crates/plannotator-tui/src/app/tests.rs b/crates/plannotator-tui/src/app/tests.rs index 1b58a33..5d24ebc 100644 --- a/crates/plannotator-tui/src/app/tests.rs +++ b/crates/plannotator-tui/src/app/tests.rs @@ -4,6 +4,7 @@ #![allow(clippy::expect_used, clippy::indexing_slicing, reason = "tests assert by panicking")] use std::path::PathBuf; +use std::sync::atomic::{AtomicUsize, Ordering}; use plannotator_tui_schema::{DocumentSource, Kind, Provenance}; use ratatui::Terminal; @@ -16,12 +17,34 @@ use super::send::SendState; use super::{App, Mode}; use crate::delivery::{Delivery, Discard, HerdrAgent}; +/// A fresh, empty data directory for one test. `App::open` resolves the real one, and a +/// successful send archives into it, so every app under test is pointed here instead: +/// nothing a test does may reach the developer's own Plannotator data. +fn scratch_data_dir() -> PathBuf { + static NEXT: AtomicUsize = AtomicUsize::new(0); + let n = NEXT.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("plannotator-tui-app-{}-{n}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("scratch data dir"); + dir +} + /// A transient source: the app runs exactly as it does on a file, but nothing is written /// to the Plannotator data directory. fn app(delivery: Box) -> App { let source = DocumentSource::new("# Plan\n\nfirst thing\n".to_owned(), "plan.md", true, Provenance::Stdin); - App::open(source, 60, delivery).expect("app opens") + let mut app = App::open(source, 60, delivery).expect("app opens"); + app.data_dir = scratch_data_dir(); + app +} + +/// `App::open_message` on `candidates()`, isolated like `app`. +fn message_app(delivery: Box) -> App { + let mut app = + App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, delivery).expect("opens"); + app.data_dir = scratch_data_dir(); + app } fn agent() -> Box { @@ -73,6 +96,8 @@ fn clicking_the_send_button_sends() { }); app.handle_event(&click).expect("click"); assert_eq!(app.send_state, SendState::Sent); + let index = app.data_dir.join("feedback").join(&app.project).join("index.jsonl"); + assert!(index.is_file(), "the send was archived under the test's own data dir"); } #[test] @@ -107,8 +132,7 @@ fn candidates() -> Vec { #[test] fn the_picker_lists_newest_first_and_opens_the_chosen_message() { - let mut app = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + let mut app = message_app(Box::new(Discard)); app.clock_offset = 0; assert_eq!(app.mode, Mode::Pick, "more than one candidate asks which"); let rows = draw(&mut app); @@ -128,8 +152,7 @@ fn the_picker_lists_newest_first_and_opens_the_chosen_message() { #[test] fn a_status_leads_the_footer_so_a_narrow_pane_cannot_truncate_it_away() { - let mut app = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + let mut app = message_app(Box::new(Discard)); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); app.set_status("no session id from Herdr, showing the newest transcript for this folder".to_owned()); let rows = draw(&mut app); @@ -139,8 +162,7 @@ fn a_status_leads_the_footer_so_a_narrow_pane_cannot_truncate_it_away() { #[test] fn escaping_the_picker_keeps_the_newest_message() { - let mut app = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + let mut app = message_app(Box::new(Discard)); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); assert_eq!(app.mode, Mode::Browse); assert_eq!(app.open.doc.source, "# Third\n\nnewest message\n"); @@ -151,8 +173,7 @@ fn escaping_the_picker_keeps_the_newest_message() { #[test] fn moving_the_picker_cursor_previews_that_message() { - let mut app = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + let mut app = message_app(Box::new(Discard)); assert_eq!(app.open.doc.source, "# Third\n\nnewest message\n", "the newest opens behind the picker"); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Char('j')))).expect("j"); @@ -163,8 +184,7 @@ fn moving_the_picker_cursor_previews_that_message() { #[test] fn previewing_away_and_back_keeps_annotations() { - let mut app = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + let mut app = message_app(Box::new(Discard)); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); app.add_block_annotation(0, Kind::Comment, "keep me".to_owned()).expect("annotate"); assert_eq!(app.open.store.placed().len(), 1); @@ -214,6 +234,7 @@ fn open_path(app: &App) -> String { fn the_tree_scrolls_to_keep_the_cursor_visible_and_hit_tests_through_the_offset() { let root = folder(30); let mut app = App::open_folder(&root, 100, Box::new(Discard)).expect("folder opens"); + app.data_dir = scratch_data_dir(); // 140 columns shows the tree; 20 rows leaves 18 for the body (header + footer). draw_sized(&mut app, 140, 20); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Tab))).expect("tab"); From 68aafec3751414ed101ccaf15978474fa3e53b8a Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Mon, 7 Sep 2026 17:58:20 -0700 Subject: [PATCH 2/5] fix(last): classify pid-registered sessions as exact The Claude Code and Copilot resolvers walk a ladder whose first rung is the session registered for the agent's pid, yet every result was labelled a folder guess, so the footer said "showing the newest transcript for this folder" even when the pid had identified the session. The resolvers now report which rung matched; a pid-registered session is exact, cwd and folder rungs stay the folder's newest transcript, and Copilot's unscoped rungs read as the newest session. --- .../src/claude/ladder.rs | 24 +++++----- crates/plannotator-tui-hosts/src/copilot.rs | 25 ++++++----- crates/plannotator-tui-hosts/src/lib.rs | 14 ++++++ crates/plannotator-tui-hosts/tests/copilot.rs | 36 ++++++++++----- crates/plannotator-tui-hosts/tests/ladder.rs | 45 ++++++++++++++++--- crates/plannotator-tui/src/last/fallback.rs | 45 ++++++++++++++----- 6 files changed, 139 insertions(+), 50 deletions(-) diff --git a/crates/plannotator-tui-hosts/src/claude/ladder.rs b/crates/plannotator-tui-hosts/src/claude/ladder.rs index 12a4434..4799fa0 100644 --- a/crates/plannotator-tui-hosts/src/claude/ladder.rs +++ b/crates/plannotator-tui-hosts/src/claude/ladder.rs @@ -5,24 +5,26 @@ use std::path::{Path, PathBuf}; use std::time::SystemTime; use super::{parse_messages, parse_session_meta, project_slug}; -use crate::SessionMeta; +use crate::{Match, SessionMeta}; const MAX_ANCESTOR_HOPS: usize = 8; -/// The ladder, most precise first; the first candidate that yields a message wins: +/// The ladder, most precise first; the first candidate that yields a message wins, and +/// the returned [`Match`] says which rung it came from: /// 1. `sessions/.json` for `start_pid` and up to eight of its ancestors — with a ghost /// check: a newer transcript in the same project dir that no running session claims is a -/// `/clear` session and is preferred; -/// 2. every session whose `cwd` is ours, newest `startedAt` first; -/// 3. the project dir for our cwd (case-insensitive fallback), newest transcript first; -/// 4. the same for each parent directory of cwd. +/// `/clear` session and is preferred ([`Match::Session`]); +/// 2. every session whose `cwd` is ours, newest `startedAt` first ([`Match::Cwd`]); +/// 3. the project dir for our cwd (case-insensitive fallback), newest transcript first +/// ([`Match::Folder`]); +/// 4. the same for each parent directory of cwd ([`Match::Folder`]). pub fn find_transcript( sessions_dir: &Path, projects_dir: &Path, cwd: &Path, process_table: &[(u32, u32)], start_pid: u32, -) -> Option { +) -> Option<(PathBuf, Match)> { let sessions = registered_sessions(sessions_dir); let registered: HashSet<&str> = sessions.iter().map(|s| s.session_id.as_str()).collect(); @@ -32,7 +34,7 @@ pub fn find_transcript( if let Some(meta) = sessions.iter().find(|s| s.pid == current) && let Some(found) = transcript_for_session(projects_dir, meta, ®istered) { - return Some(found); + return Some((found, Match::Session)); } pid = process_table.iter().find(|(p, _)| *p == current).map(|(_, ppid)| *ppid).filter(|&p| p > 1); } @@ -41,11 +43,13 @@ pub fn find_transcript( same_cwd.sort_by_key(|s| std::cmp::Reverse(s.started_at)); for meta in same_cwd { if let Some(found) = transcript_for_session(projects_dir, meta, ®istered) { - return Some(found); + return Some((found, Match::Cwd)); } } - cwd.ancestors().find_map(|dir| newest_with_messages(&project_dir(projects_dir, dir)?, None)) + cwd.ancestors() + .find_map(|dir| newest_with_messages(&project_dir(projects_dir, dir)?, None)) + .map(|found| (found, Match::Folder)) } fn registered_sessions(sessions_dir: &Path) -> Vec { diff --git a/crates/plannotator-tui-hosts/src/copilot.rs b/crates/plannotator-tui-hosts/src/copilot.rs index 42f5978..7f511c7 100644 --- a/crates/plannotator-tui-hosts/src/copilot.rs +++ b/crates/plannotator-tui-hosts/src/copilot.rs @@ -11,7 +11,7 @@ use std::time::SystemTime; use serde_json::Value; -use crate::{Message, Role}; +use crate::{Match, Message, Role}; const MAX_ANCESTOR_HOPS: usize = 8; @@ -25,21 +25,23 @@ pub fn find_session_by_id( Ok(dir.join("events.jsonl").is_file().then_some(dir)) } -/// The session directory for the Copilot process we were launched from. +/// The session directory for the Copilot process we were launched from, and the +/// [`Match`] that says which rung chose it. /// /// 1. Walk `start_pid` and up to eight ancestors; the first pid that owns an /// `inuse..lock` wins, provided `is_copilot(pid)` confirms the pid still names a /// Copilot process (locks outlive sessions and pids get reused; a stale match is dropped -/// and the walk continues). -/// 2. Else by cwd, newest directory first: a locked session for `cwd`, any locked session, -/// a session for `cwd`, the newest session at all. +/// and the walk continues). [`Match::Session`]. +/// 2. Else by cwd, newest directory first: a locked session for `cwd` ([`Match::Cwd`]), +/// any locked session ([`Match::Newest`]), a session for `cwd` ([`Match::Cwd`]), the +/// newest session at all ([`Match::Newest`]). pub fn find_session( copilot_home: &Path, cwd: &Path, process_table: &[(u32, u32)], start_pid: u32, is_copilot: impl Fn(u32) -> bool, -) -> Option { +) -> Option<(PathBuf, Match)> { let state_dir = copilot_home.join("session-state"); let sessions = list_sessions(&state_dir); @@ -47,20 +49,21 @@ pub fn find_session( let locks = lock_owners(&sessions); while let Some(&pid) = chain.iter().find(|pid| locks.contains_key(pid)) { if is_copilot(pid) { - return locks.get(&pid).cloned(); + return locks.get(&pid).cloned().map(|dir| (dir, Match::Session)); } chain.retain(|&p| p != pid); } let wanted = normalize(cwd); let matches = |s: &Session| s.cwd.as_deref().is_some_and(|c| normalize(Path::new(c)) == wanted); + let pick = |session: &Session, rung: Match| (session.dir.clone(), rung); sessions .iter() .find(|s| s.locked && matches(s)) - .or_else(|| sessions.iter().find(|s| s.locked)) - .or_else(|| sessions.iter().find(|s| matches(s))) - .or_else(|| sessions.first()) - .map(|s| s.dir.clone()) + .map(|s| pick(s, Match::Cwd)) + .or_else(|| sessions.iter().find(|s| s.locked).map(|s| pick(s, Match::Newest))) + .or_else(|| sessions.iter().find(|s| matches(s)).map(|s| pick(s, Match::Cwd))) + .or_else(|| sessions.first().map(|s| pick(s, Match::Newest))) } /// Human prompts and assistant replies from `events.jsonl`, newest first, at most `n`. diff --git a/crates/plannotator-tui-hosts/src/lib.rs b/crates/plannotator-tui-hosts/src/lib.rs index f40881e..7129186 100644 --- a/crates/plannotator-tui-hosts/src/lib.rs +++ b/crates/plannotator-tui-hosts/src/lib.rs @@ -107,6 +107,20 @@ pub struct Message { pub at: Option, } +/// Which rung of a discovery ladder chose a session, so a caller can say whether the +/// result identifies the agent's own session or is the best guess for its folder. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Match { + /// The session the agent process (or one of its ancestors) registered: not a guess. + Session, + /// A session whose recorded working directory is the agent's, newest first. + Cwd, + /// The newest session filed under the agent's folder or one of its parents. + Folder, + /// The newest session the host has at all, not scoped to any directory. + Newest, +} + /// `~/.claude/sessions/.json`: one running Claude Code session. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SessionMeta { diff --git a/crates/plannotator-tui-hosts/tests/copilot.rs b/crates/plannotator-tui-hosts/tests/copilot.rs index bc93d77..f880c42 100644 --- a/crates/plannotator-tui-hosts/tests/copilot.rs +++ b/crates/plannotator-tui-hosts/tests/copilot.rs @@ -6,8 +6,8 @@ use std::fs::{self, File}; use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime}; -use plannotator_tui_hosts::Role; use plannotator_tui_hosts::copilot::{find_session, parse_messages}; +use plannotator_tui_hosts::{Match, Role}; const EVENTS: &str = include_str!("fixtures/copilot/session-state/aaaa1111-0000-4000-8000-000000000001/events.jsonl"); @@ -77,7 +77,10 @@ fn a_lock_held_by_an_ancestor_beats_every_cwd_heuristic() { let locked = home.session("locked-elsewhere", "/elsewhere", &[300], 100); // 4242 → 4000 → 300: the third hop owns the lock. let table = [(4242, 4000), (4000, 300), (300, 1)]; - assert_eq!(find_session(&home.root, Path::new("/w"), &table, 4242, always_copilot), Some(locked)); + assert_eq!( + find_session(&home.root, Path::new("/w"), &table, 4242, always_copilot), + Some((locked, Match::Session)) + ); } #[test] @@ -88,7 +91,7 @@ fn a_stale_lock_is_skipped_and_the_walk_continues() { let table = [(4242, 4000), (4000, 300), (300, 1)]; // 4000 no longer names a copilot process; 300 does. let found = find_session(&home.root, Path::new("/w"), &table, 4242, |pid| pid == 300); - assert_eq!(found, Some(live)); + assert_eq!(found, Some((live, Match::Session))); } #[test] @@ -99,10 +102,16 @@ fn the_ninth_ancestor_is_out_of_reach() { let table: Vec<(u32, u32)> = (1..=17).map(|p| (p + 1, p)).collect(); // 18 → 17 → … → 2 let only_nine_is_live = |pid: u32| pid == 9; // From 18 the walk reaches 11; the stale lock at 12 is dropped and 9 is never seen, so - // the cwd ladder decides: the newest active session. - assert_eq!(find_session(&home.root, Path::new("/none"), &table, 18, only_nine_is_live), Some(near)); + // the cwd ladder decides: the newest active session, which is not scoped to a cwd. + assert_eq!( + find_session(&home.root, Path::new("/none"), &table, 18, only_nine_is_live), + Some((near, Match::Newest)) + ); // From 16 the eighth hop is 9 and its live lock wins. - assert_eq!(find_session(&home.root, Path::new("/none"), &table, 16, only_nine_is_live), Some(far)); + assert_eq!( + find_session(&home.root, Path::new("/none"), &table, 16, only_nine_is_live), + Some((far, Match::Session)) + ); } #[test] @@ -115,15 +124,15 @@ fn without_a_lock_match_the_cwd_ladder_applies_in_order() { let no_pid_match = [(1, 1)]; let find = |cwd: &str| find_session(&home.root, Path::new(cwd), &no_pid_match, 4242, always_copilot); - assert_eq!(find("/w"), Some(cwd_locked.clone()), "an active session for the cwd wins"); + assert_eq!(find("/w"), Some((cwd_locked.clone(), Match::Cwd)), "an active session for the cwd wins"); fs::remove_file(cwd_locked.join("inuse.777.lock")).expect("unlock"); - assert_eq!(find("/w"), Some(any_locked.clone()), "then any active session"); + assert_eq!(find("/w"), Some((any_locked.clone(), Match::Newest)), "then any active session"); fs::remove_file(any_locked.join("inuse.888.lock")).expect("unlock"); // Removing locks touched those directories; restore their ages so mtime order holds. age(&cwd_locked, 40); age(&any_locked, 60); - assert_eq!(find("/w"), Some(cwd_plain), "then the newest session for the cwd"); - assert_eq!(find("/nowhere"), Some(newest_any), "then the newest session at all"); + assert_eq!(find("/w"), Some((cwd_plain, Match::Cwd)), "then the newest session for the cwd"); + assert_eq!(find("/nowhere"), Some((newest_any, Match::Newest)), "then the newest session at all"); } #[test] @@ -132,7 +141,10 @@ fn a_session_directory_without_events_is_not_a_candidate() { let with_events = home.session("with-events", "/w", &[], 50); let bare = home.session("bare", "/w", &[], 5); fs::remove_file(bare.join("events.jsonl")).expect("strip events"); - assert_eq!(find_session(&home.root, Path::new("/w"), &[], 1, always_copilot), Some(with_events)); + assert_eq!( + find_session(&home.root, Path::new("/w"), &[], 1, always_copilot), + Some((with_events, Match::Cwd)) + ); } #[test] @@ -141,7 +153,7 @@ fn cwd_comparison_ignores_case_and_slash_direction() { let win = home.session("win", "C:\\Users\\Me\\Repo", &[], 10); home.session("other", "/other", &[], 5); let found = find_session(&home.root, Path::new("c:/users/me/repo"), &[], 1, always_copilot); - assert_eq!(found, Some(win)); + assert_eq!(found, Some((win, Match::Cwd))); } #[test] diff --git a/crates/plannotator-tui-hosts/tests/ladder.rs b/crates/plannotator-tui-hosts/tests/ladder.rs index bc05bbc..984fdf7 100644 --- a/crates/plannotator-tui-hosts/tests/ladder.rs +++ b/crates/plannotator-tui-hosts/tests/ladder.rs @@ -6,6 +6,7 @@ use std::fs; use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime}; +use plannotator_tui_hosts::Match; use plannotator_tui_hosts::claude::{find_transcript, parse_ps, parse_session_meta, project_slug}; const TRANSCRIPT: &str = include_str!("fixtures/claude-code.jsonl"); @@ -83,7 +84,7 @@ fn a_direct_pid_hit_is_the_herdr_case() { home.session(500, "s500", cwd, 10); let expected = home.transcript(cwd, "s500", TRANSCRIPT, 60); let found = find_transcript(&home.sessions(), &home.projects(), cwd, &[], 500); - assert_eq!(found, Some(expected)); + assert_eq!(found, Some((expected, Match::Session))); } #[test] @@ -96,7 +97,7 @@ fn an_ancestor_within_eight_hops_is_found_and_the_ninth_is_not() { let table: Vec<(u32, u32)> = (1001..=1009).map(|p| (p, p - 1)).collect(); assert_eq!( find_transcript(&home.sessions(), &home.projects(), Path::new("/elsewhere"), &table, 1008), - Some(expected) + Some((expected, Match::Session)) ); assert_eq!( find_transcript(&home.sessions(), &home.projects(), Path::new("/elsewhere"), &table, 1009), @@ -111,7 +112,11 @@ fn a_newer_unregistered_transcript_in_the_project_is_a_clear_session_and_wins() home.session(700, "s700", cwd, 10); home.transcript(cwd, "s700", TRANSCRIPT, 600); let ghost = home.transcript(cwd, "after-clear", TRANSCRIPT, 5); - assert_eq!(find_transcript(&home.sessions(), &home.projects(), cwd, &[], 700), Some(ghost)); + assert_eq!( + find_transcript(&home.sessions(), &home.projects(), cwd, &[], 700), + Some((ghost, Match::Session)), + "the ghost stands in for the registered session, so it is still the pid's own" + ); } #[test] @@ -123,7 +128,10 @@ fn without_a_pid_hit_the_cwd_scan_prefers_the_newest_session() { home.session(12, "other", Path::new("/w/other"), 300); home.transcript(cwd, "old", TRANSCRIPT, 30); let expected = home.transcript(cwd, "new", TRANSCRIPT, 30); - assert_eq!(find_transcript(&home.sessions(), &home.projects(), cwd, &[], 9999), Some(expected)); + assert_eq!( + find_transcript(&home.sessions(), &home.projects(), cwd, &[], 9999), + Some((expected, Match::Cwd)) + ); } #[test] @@ -134,8 +142,9 @@ fn slug_and_mtime_pick_the_newest_transcript_even_in_a_lowercased_dir() { home.transcript_in(&lower, "older", TRANSCRIPT, 300); let expected = home.transcript_in(&lower, "newer", TRANSCRIPT, 30); // Case-insensitive filesystems (macOS) resolve either spelling; compare the real path. - let found = find_transcript(&home.sessions(), &home.projects(), cwd, &[], 1).expect("found"); + let (found, rung) = find_transcript(&home.sessions(), &home.projects(), cwd, &[], 1).expect("found"); assert_eq!(fs::canonicalize(found).expect("real"), fs::canonicalize(expected).expect("real")); + assert_eq!(rung, Match::Folder); } #[test] @@ -144,7 +153,7 @@ fn a_parent_directory_of_cwd_is_tried_when_cwd_has_no_project() { let expected = home.transcript(Path::new("/w/repo"), "s", TRANSCRIPT, 30); assert_eq!( find_transcript(&home.sessions(), &home.projects(), Path::new("/w/repo/deep/dir"), &[], 1), - Some(expected) + Some((expected, Match::Folder)) ); } @@ -154,5 +163,27 @@ fn a_candidate_with_no_messages_is_skipped_for_the_next_one() { let cwd = Path::new("/w/repo"); home.transcript(cwd, "empty", "{\"type\":\"progress\"}\n", 5); let expected = home.transcript(cwd, "real", TRANSCRIPT, 60); - assert_eq!(find_transcript(&home.sessions(), &home.projects(), cwd, &[], 1), Some(expected)); + assert_eq!( + find_transcript(&home.sessions(), &home.projects(), cwd, &[], 1), + Some((expected, Match::Folder)) + ); +} + +#[test] +fn the_rung_that_chose_the_transcript_is_reported() { + let home = Home::new("rungs"); + let cwd = Path::new("/w/repo"); + home.session(500, "s500", cwd, 10); + let own = home.transcript(cwd, "s500", TRANSCRIPT, 60); + let by_pid = find_transcript(&home.sessions(), &home.projects(), cwd, &[], 500); + assert_eq!(by_pid, Some((own.clone(), Match::Session)), "the pid's registered session is exact"); + + // No pid hit: a registered session for the same cwd is the folder's best guess. + let by_cwd = find_transcript(&home.sessions(), &home.projects(), cwd, &[], 9999); + assert_eq!(by_cwd, Some((own.clone(), Match::Cwd))); + + // No registered session at all: the project directory decides. + fs::remove_file(home.sessions().join("500.json")).expect("unregister"); + let by_folder = find_transcript(&home.sessions(), &home.projects(), cwd, &[], 500); + assert_eq!(by_folder, Some((own, Match::Folder))); } diff --git a/crates/plannotator-tui/src/last/fallback.rs b/crates/plannotator-tui/src/last/fallback.rs index f792dfe..6d127b0 100644 --- a/crates/plannotator-tui/src/last/fallback.rs +++ b/crates/plannotator-tui/src/last/fallback.rs @@ -7,7 +7,7 @@ use std::process::Command; use anyhow::{Context, Result, bail}; #[cfg(unix)] use plannotator_tui_hosts::copilot; -use plannotator_tui_hosts::{Host, Message, claude, droid, opencode, pi}; +use plannotator_tui_hosts::{Host, Match, Message, claude, droid, opencode, pi}; use super::LastOptions; use super::exact; @@ -17,8 +17,9 @@ use super::roots::Roots; /// How a transcript was chosen, so the UI can say when nothing identified it exactly. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Discovery { - /// An explicit path, an exact session id, a thread id, or the file the agent process - /// itself has open: this transcript is the session, not a guess. + /// An explicit path, an exact session id, a thread id, the session registered for the + /// agent's pid, or the file the agent process itself has open: this transcript is the + /// session, not a guess. Exact, /// The newest transcript filed under the agent's working directory. Sessions that share /// one directory are indistinguishable here. @@ -36,9 +37,9 @@ pub(super) fn read( ) -> Result<(PathBuf, Vec, Discovery)> { match host { Host::ClaudeCode => { - let path = find_claude_transcript(options.pid, cwd, roots)?; + let (path, rung) = find_claude_transcript(options.pid, cwd, roots)?; let messages = readers::claude_messages(&path, pick)?; - Ok((path, messages, Discovery::Folder)) + Ok((path, messages, discovery_of(rung))) } Host::Codex => { #[cfg(target_os = "linux")] @@ -53,9 +54,9 @@ pub(super) fn read( Ok((path, messages, discovery)) } Host::Copilot => { - let path = find_copilot_session(options.pid, cwd, roots)?; + let (path, rung) = find_copilot_session(options.pid, cwd, roots)?; let messages = readers::copilot_messages(&path, pick)?; - Ok((path, messages, Discovery::Folder)) + Ok((path, messages, discovery_of(rung))) } Host::Droid => { let path = find_droid_transcript(cwd, roots)?; @@ -75,6 +76,17 @@ pub(super) fn read( } } +/// What a resolver's rung means for the footer: a session registered for the agent's pid +/// is the session; a cwd or folder rung is the folder's newest transcript; an unscoped +/// rung is just the newest session the host has. +fn discovery_of(rung: Match) -> Discovery { + match rung { + Match::Session => Discovery::Exact, + Match::Cwd | Match::Folder => Discovery::Folder, + Match::Newest => Discovery::Session, + } +} + /// A selected Linux process is a stronger hint than an inherited thread id or the newest /// rollout. Missing or ambiguous descriptors must not silently select another session. /// Codex keeps its subagents' rollouts (reviews, guardians) open too; those are ignored. @@ -108,7 +120,7 @@ fn find_codex_transcript(pid: u32) -> Result { transcripts.into_iter().next().with_context(|| format!("no transcript for Codex process {pid}")) } -fn find_claude_transcript(pid: Option, cwd: &Path, roots: &Roots) -> Result { +fn find_claude_transcript(pid: Option, cwd: &Path, roots: &Roots) -> Result<(PathBuf, Match)> { let sessions_dir = roots.claude_config.join("sessions"); let projects_dir = roots.claude_config.join("projects"); let (start_pid, table) = process_context(pid); @@ -122,7 +134,7 @@ fn find_claude_transcript(pid: Option, cwd: &Path, roots: &Roots) -> Result } #[cfg(unix)] -fn find_copilot_session(pid: Option, cwd: &Path, roots: &Roots) -> Result { +fn find_copilot_session(pid: Option, cwd: &Path, roots: &Roots) -> Result<(PathBuf, Match)> { let (start_pid, table) = process_context(pid); copilot::find_session(&roots.copilot_home, cwd, &table, start_pid, is_copilot_process).ok_or_else(|| { anyhow::anyhow!( @@ -134,7 +146,7 @@ fn find_copilot_session(pid: Option, cwd: &Path, roots: &Roots) -> Result

, _cwd: &Path, _roots: &Roots) -> Result { +fn find_copilot_session(_pid: Option, _cwd: &Path, _roots: &Roots) -> Result<(PathBuf, Match)> { bail!("Copilot session discovery without --session-id is unsupported on Windows; pass --session-id") } @@ -205,3 +217,16 @@ fn opencode_for_cwd(cwd: &Path, roots: &Roots, pick: usize) -> Result<(PathBuf, let messages = opencode::messages_for_session(&database, &found.id, found.schema, pick)?; Ok((database, messages, Discovery::Folder)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn only_a_pid_registered_session_counts_as_exact() { + assert_eq!(discovery_of(Match::Session), Discovery::Exact); + assert_eq!(discovery_of(Match::Cwd), Discovery::Folder); + assert_eq!(discovery_of(Match::Folder), Discovery::Folder); + assert_eq!(discovery_of(Match::Newest), Discovery::Session); + } +} From f18dd7e78edf45bd7949dc237ad10f0fa035ef62 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Mon, 7 Sep 2026 18:00:00 -0700 Subject: [PATCH 3/5] fix(archive): record the host session id, not the transcript path message_source put the transcript path into Provenance::AgentMessage's session, so an archived message review carried the path in both target.agent.session and target.agent.transcript, where the shared feedback-archive contract wants the host-assigned session id in session. Carry the real id instead: the one Herdr or --session-id gave, else the one a uuid-named Claude Code, Droid, Codex or Copilot transcript carries in its name, else nothing. The transcript path stays in the app's own field for the archive's transcript. --- crates/plannotator-tui-hosts/src/codex.rs | 2 +- crates/plannotator-tui-hosts/src/lib.rs | 69 ++++++++++++++++++++++- crates/plannotator-tui/src/app/mod.rs | 4 ++ crates/plannotator-tui/src/app/pick.rs | 9 ++- crates/plannotator-tui/src/app/tests.rs | 45 ++++++++++++--- crates/plannotator-tui/src/last/locate.rs | 32 ++++++++++- crates/plannotator-tui/src/last/mod.rs | 18 ++++-- 7 files changed, 162 insertions(+), 17 deletions(-) diff --git a/crates/plannotator-tui-hosts/src/codex.rs b/crates/plannotator-tui-hosts/src/codex.rs index d67e682..d296dad 100644 --- a/crates/plannotator-tui-hosts/src/codex.rs +++ b/crates/plannotator-tui-hosts/src/codex.rs @@ -45,7 +45,7 @@ fn all_rollouts(dir: &Path) -> Vec { } /// The thread id is the trailing uuid of `rollout--.jsonl`. -fn thread_of(path: &Path) -> Option { +pub(crate) fn thread_of(path: &Path) -> Option { let stem = path.file_stem()?.to_str()?; let parts: Vec<&str> = stem.rsplitn(6, '-').collect(); (parts.len() == 6).then(|| parts.iter().take(5).rev().copied().collect::>().join("-")) diff --git a/crates/plannotator-tui-hosts/src/lib.rs b/crates/plannotator-tui-hosts/src/lib.rs index 7129186..c80c2a0 100644 --- a/crates/plannotator-tui-hosts/src/lib.rs +++ b/crates/plannotator-tui-hosts/src/lib.rs @@ -16,7 +16,7 @@ pub mod pi; pub(crate) mod sqlite; pub(crate) mod time; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// A supported agent host. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -171,6 +171,31 @@ impl From for HostError { } } +/// The host-assigned session id a transcript's name carries, when the host's naming +/// scheme makes it unambiguous: Claude Code and Droid file a session as `.jsonl`, +/// a Codex rollout ends in its thread uuid, and a Copilot session is a directory named by +/// its uuid. Anything that is not uuid-shaped yields `None`: an arbitrary path handed in +/// with `--session` must never be mistaken for an id, and a path is never one. +pub fn session_id_of(host: Host, transcript: &Path) -> Option { + let candidate = match host { + Host::ClaudeCode | Host::Droid => transcript.file_stem()?.to_str()?.to_owned(), + Host::Codex => codex::thread_of(transcript)?, + Host::Copilot => transcript.file_name()?.to_str()?.to_owned(), + Host::Pi | Host::Omp | Host::Hermes | Host::OpenCode => return None, + }; + is_uuid(&candidate).then_some(candidate) +} + +/// `8-4-4-4-12` hex groups, any case. +fn is_uuid(text: &str) -> bool { + let bytes = text.as_bytes(); + bytes.len() == 36 + && bytes.iter().enumerate().all(|(i, b)| match i { + 8 | 13 | 18 | 23 => *b == b'-', + _ => b.is_ascii_hexdigit(), + }) +} + /// Which host launched us, from the environment. `PLANNOTATOR_TUI_HOST` overrides when it /// names a known host; then the hosts' own markers, in Plannotator's order; then Claude Code. /// @@ -219,3 +244,45 @@ pub fn detect_host(env: impl Fn(&str) -> Option) -> Result, message_host: String, + /// The transcript path, for the archive's `transcript`; never the session id. message_transcript: String, + /// The host-assigned session id, for the archive's `session`; never a path. + message_session: Option, compose: Compose, /// Whether the terminal reports Shift+Enter distinctly (kitty keyboard protocol). pub(super) shift_enter: bool, @@ -208,6 +211,7 @@ impl App { pick_cache: HashMap::new(), message_host: String::new(), message_transcript: String::new(), + message_session: None, compose: Compose::default(), shift_enter: false, last_click: None, diff --git a/crates/plannotator-tui/src/app/pick.rs b/crates/plannotator-tui/src/app/pick.rs index 47dbaab..ef566f8 100644 --- a/crates/plannotator-tui/src/app/pick.rs +++ b/crates/plannotator-tui/src/app/pick.rs @@ -21,18 +21,21 @@ const PICK_MAX_WIDTH: u16 = 90; impl App { /// Open with `messages` (newest first) as candidates; the picker shows when there is a - /// choice to make. + /// choice to make. `transcript` is the path shown and archived; `session_id` is the + /// host's own id for the session, when known. pub(crate) fn open_message( host: &str, transcript: &str, + session_id: Option<&str>, messages: Vec, width: usize, delivery: Box, ) -> Result { let Some(newest) = messages.first() else { anyhow::bail!("no message to open") }; - let mut app = Self::open(message_source(host, transcript, newest), width, delivery)?; + let mut app = Self::open(message_source(host, session_id, newest), width, delivery)?; host.clone_into(&mut app.message_host); transcript.clone_into(&mut app.message_transcript); + app.message_session = session_id.map(str::to_owned); app.candidates = messages; if app.candidates.len() > 1 { app.mode = Mode::Pick; @@ -53,7 +56,7 @@ impl App { open } else { let Some(message) = self.candidates.get(index) else { return Ok(()) }; - let source = message_source(&self.message_host, &self.message_transcript, message); + let source = message_source(&self.message_host, self.message_session.as_deref(), message); Open::new(source, self.open.layout.width, &self.data_dir, &self.project)? }; let leaving = std::mem::replace(&mut self.open, next); diff --git a/crates/plannotator-tui/src/app/tests.rs b/crates/plannotator-tui/src/app/tests.rs index 5d24ebc..1f4d3fa 100644 --- a/crates/plannotator-tui/src/app/tests.rs +++ b/crates/plannotator-tui/src/app/tests.rs @@ -40,13 +40,44 @@ fn app(delivery: Box) -> App { } /// `App::open_message` on `candidates()`, isolated like `app`. -fn message_app(delivery: Box) -> App { +fn message_app(session_id: Option<&str>, delivery: Box) -> App { let mut app = - App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, delivery).expect("opens"); + App::open_message("claude", "/tmp/transcript.jsonl", session_id, candidates(), 60, delivery) + .expect("opens"); app.data_dir = scratch_data_dir(); app } +/// Send the open message review through `Discard` and return the archive's one record. +fn archived_message_review(app: &mut App) -> serde_json::Value { + app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); + app.add_block_annotation(0, Kind::Comment, "x".to_owned()).expect("annotation"); + app.send_feedback().expect("send"); + assert_eq!(app.send_state, SendState::Sent); + let index = app.data_dir.join("feedback").join(&app.project).join("index.jsonl"); + let text = std::fs::read_to_string(&index).expect("index written under the test's data dir"); + serde_json::from_str(text.trim()).expect("one json record") +} + +#[test] +fn a_message_review_archives_the_session_id_and_the_transcript_path_separately() { + let id = "01a04583-a848-7b21-a890-f3ed0c9fef05"; + let mut app = message_app(Some(id), Box::new(Discard)); + let record = archived_message_review(&mut app); + assert_eq!(record["surface"], "annotate-last"); + assert_eq!(record["target"]["agent"]["host"], "claude-code"); + assert_eq!(record["target"]["agent"]["session"], id); + assert_eq!(record["target"]["agent"]["transcript"], "/tmp/transcript.jsonl"); +} + +#[test] +fn a_message_review_without_a_session_id_archives_only_the_transcript_path() { + let mut app = message_app(None, Box::new(Discard)); + let record = archived_message_review(&mut app); + assert!(record["target"]["agent"].get("session").is_none(), "no id means no session, never the path"); + assert_eq!(record["target"]["agent"]["transcript"], "/tmp/transcript.jsonl"); +} + fn agent() -> Box { Box::new(HerdrAgent::new(PathBuf::from("/nonexistent/herdr"), "w1:p1".into(), Some("claude".into()))) } @@ -132,7 +163,7 @@ fn candidates() -> Vec { #[test] fn the_picker_lists_newest_first_and_opens_the_chosen_message() { - let mut app = message_app(Box::new(Discard)); + let mut app = message_app(None, Box::new(Discard)); app.clock_offset = 0; assert_eq!(app.mode, Mode::Pick, "more than one candidate asks which"); let rows = draw(&mut app); @@ -152,7 +183,7 @@ fn the_picker_lists_newest_first_and_opens_the_chosen_message() { #[test] fn a_status_leads_the_footer_so_a_narrow_pane_cannot_truncate_it_away() { - let mut app = message_app(Box::new(Discard)); + let mut app = message_app(None, Box::new(Discard)); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); app.set_status("no session id from Herdr, showing the newest transcript for this folder".to_owned()); let rows = draw(&mut app); @@ -162,7 +193,7 @@ fn a_status_leads_the_footer_so_a_narrow_pane_cannot_truncate_it_away() { #[test] fn escaping_the_picker_keeps_the_newest_message() { - let mut app = message_app(Box::new(Discard)); + let mut app = message_app(None, Box::new(Discard)); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); assert_eq!(app.mode, Mode::Browse); assert_eq!(app.open.doc.source, "# Third\n\nnewest message\n"); @@ -173,7 +204,7 @@ fn escaping_the_picker_keeps_the_newest_message() { #[test] fn moving_the_picker_cursor_previews_that_message() { - let mut app = message_app(Box::new(Discard)); + let mut app = message_app(None, Box::new(Discard)); assert_eq!(app.open.doc.source, "# Third\n\nnewest message\n", "the newest opens behind the picker"); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Char('j')))).expect("j"); @@ -184,7 +215,7 @@ fn moving_the_picker_cursor_previews_that_message() { #[test] fn previewing_away_and_back_keeps_annotations() { - let mut app = message_app(Box::new(Discard)); + let mut app = message_app(None, Box::new(Discard)); app.handle_event(&Event::Key(KeyEvent::from(KeyCode::Esc))).expect("esc"); app.add_block_annotation(0, Kind::Comment, "keep me".to_owned()).expect("annotate"); assert_eq!(app.open.store.placed().len(), 1); diff --git a/crates/plannotator-tui/src/last/locate.rs b/crates/plannotator-tui/src/last/locate.rs index 2f0bb52..64796b0 100644 --- a/crates/plannotator-tui/src/last/locate.rs +++ b/crates/plannotator-tui/src/last/locate.rs @@ -15,6 +15,9 @@ pub(crate) struct Located { pub(crate) host: Host, /// The transcript file (Claude) or the newest thread file (Codex); for the label. pub(crate) transcript: PathBuf, + /// The host-assigned session id: the one given, else the one the transcript's name + /// carries unambiguously. Never a path. + pub(crate) session_id: Option, /// Assistant messages, newest first, at most `options.pick`. pub(crate) messages: Vec, /// How the transcript was chosen; the UI says so when nothing identified it exactly. @@ -50,7 +53,9 @@ pub(crate) fn locate(options: &LastOptions) -> Result { if messages.is_empty() { bail!("transcript {} has no assistant messages yet", transcript.display()); } - Ok(Located { host, transcript, messages, discovery }) + let session_id = + options.session_id.clone().or_else(|| plannotator_tui_hosts::session_id_of(host, &transcript)); + Ok(Located { host, transcript, session_id, messages, discovery }) } /// Was a host named explicitly, by flag or by the launcher? @@ -167,6 +172,31 @@ mod tests { assert_eq!(located.discovery, Discovery::Exact); assert_eq!(located.messages.first().map(|m| m.text.as_str()), Some("reply")); + assert_eq!(located.session_id, None, "`session.jsonl` is not a session id"); + std::fs::remove_dir_all(&dir).expect("cleanup"); + } + + #[test] + fn a_uuid_named_transcript_yields_its_id_and_an_explicit_id_wins() { + let dir = std::env::temp_dir().join(format!("plannotator locate id-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("temp dir"); + let id = "01a04583-a848-7b21-a890-f3ed0c9fef05"; + let named = dir.join(format!("{id}.jsonl")); + std::fs::copy(transcript(&dir), &named).expect("copy"); + let options = LastOptions { + host: Some("claude".to_owned()), + session: Some(named), + pick: 25, + ..LastOptions::default() + }; + + let located = locate(&options).expect("the named transcript is read"); + assert_eq!(located.session_id.as_deref(), Some(id)); + + let explicit = LastOptions { session_id: Some("given-by-herdr".to_owned()), ..options }; + let located = locate(&explicit).expect("the named transcript is read"); + assert_eq!(located.session_id.as_deref(), Some("given-by-herdr")); std::fs::remove_dir_all(&dir).expect("cleanup"); } } diff --git a/crates/plannotator-tui/src/last/mod.rs b/crates/plannotator-tui/src/last/mod.rs index c2d709e..1c0e08c 100644 --- a/crates/plannotator-tui/src/last/mod.rs +++ b/crates/plannotator-tui/src/last/mod.rs @@ -80,10 +80,18 @@ pub(crate) fn run(options: &LastOptions) -> Result<()> { } let label = located.host.label(); let transcript = located.transcript.display().to_string(); + let session_id = located.session_id; let messages = located.messages; let note = discovery_note(located.discovery, crate::herdr::context::HerdrEnv::from_env().in_herdr); cli::run_ui(|width| { - let mut app = App::open_message(label, &transcript, messages, width, cli::delivery(true))?; + let mut app = App::open_message( + label, + &transcript, + session_id.as_deref(), + messages, + width, + cli::delivery(true), + )?; if let Some(note) = note { app.set_status(note); } @@ -105,15 +113,17 @@ fn discovery_note(discovery: Discovery, in_herdr: bool) -> Option { Some(if in_herdr { format!("no session id from Herdr, {shown}") } else { shown.to_owned() }) } -/// A message as a document: transient, provenance names the host, transcript and message. -pub(crate) fn message_source(host: &str, transcript: &str, message: &Message) -> DocumentSource { +/// A message as a document: transient, provenance names the host, the host-assigned +/// session id when one is known (never the transcript path; the app keeps that +/// separately), and the message. +pub(crate) fn message_source(host: &str, session_id: Option<&str>, message: &Message) -> DocumentSource { DocumentSource::new( message.text.clone(), format!("{host} · last message"), true, Provenance::AgentMessage { host: host.to_owned(), - session: Some(transcript.to_owned()), + session: session_id.map(str::to_owned), message_id: Some(message.id.clone()), }, ) From c1e81510c07b0ea33b955593653357e200e09276 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Mon, 7 Sep 2026 18:00:12 -0700 Subject: [PATCH 4/5] docs: describe the feedback archive and its opt-out The README said reply reviews are never written to disk, which stopped being true when the feedback archive landed: a successful send or copy appends the submitted feedback to the shared archive for every review kind. Say what is written, where, when, and how to turn it off. --- README.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index c615c68..d32c860 100644 --- a/README.md +++ b/README.md @@ -90,7 +90,8 @@ Droid, Hermes CLI, OpenCode (1 and 2). `--host`, `--pid`, `--session ` (Hermes, OpenCode) override detection; `--stdin` reads a document; `--print` writes the newest reply to stdout and always exits 0 (for hooks and scripts). -Reply reviews are never written to disk. +A reply review keeps its annotations in memory only; nothing about it survives the run, but +the feedback you send or copy is archived like any other (see Feedback archive below). On Linux, an explicit Codex `--pid` selects the rollout opened by that process. If it cannot be identified uniquely, `last` reports the failure instead of choosing an unrelated @@ -114,6 +115,18 @@ its path: Plannotator's own layout, so both tools see one record per file. The J Plannotator Workspaces wire shape; any agent can read it. Nothing is written next to your files. `PLANNOTATOR_DATA_DIR` relocates the directory. +### Feedback archive + +A successful Send or Copy also appends what was submitted (the feedback text, the quoted +selections and their annotations, and the file, folder or agent session it was about) to +`{data_dir}/feedback//index.jsonl`, with a Markdown copy under `records/`. The data +dir is `PLANNOTATOR_DATA_DIR`, else an existing `~/.plannotator`, else +`$XDG_DATA_HOME/plannotator`, else `~/.plannotator`. File, folder and reply reviews are all +archived; a send that fails or is refused is not. The format is the one the Plannotator +browser app writes, so both tools share one history. To turn it off, set +`PLANNOTATOR_FEEDBACK_HISTORY=0` (once the variable is set, only `1` or `true` enable) or put +`"feedbackHistory": false` in `{data_dir}/config.json`; the variable wins over the file. + ## Headless ```sh From c2ff734cff1fc983e197852023a8c199780326fd Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Mon, 7 Sep 2026 18:23:37 -0700 Subject: [PATCH 5/5] fix(last): let the transcript name outrank a supplied session id A session id given next to a transcript path is validated before any reader runs, and it is recorded only for hosts whose transcript names carry no id of their own. A uuid-named transcript is the authority for its own id, so a contradicting id can no longer be archived as the reviewed message's session. --- crates/plannotator-tui/src/last/locate.rs | 56 +++++++++++++++++++---- 1 file changed, 47 insertions(+), 9 deletions(-) diff --git a/crates/plannotator-tui/src/last/locate.rs b/crates/plannotator-tui/src/last/locate.rs index 64796b0..e47d281 100644 --- a/crates/plannotator-tui/src/last/locate.rs +++ b/crates/plannotator-tui/src/last/locate.rs @@ -15,8 +15,8 @@ pub(crate) struct Located { pub(crate) host: Host, /// The transcript file (Claude) or the newest thread file (Codex); for the label. pub(crate) transcript: PathBuf, - /// The host-assigned session id: the one given, else the one the transcript's name - /// carries unambiguously. Never a path. + /// The host-assigned session id: the one the transcript's name carries, else a valid + /// supplied one for hosts without such names. Never a path. pub(crate) session_id: Option, /// Assistant messages, newest first, at most `options.pick`. pub(crate) messages: Vec, @@ -35,7 +35,10 @@ pub(crate) fn locate(options: &LastOptions) -> Result { let pick = options.pick.max(1); let roots = Roots::from_env(); let (transcript, messages, discovery) = if let Some(path) = session { - let (transcript, messages) = readers::explicit(host, &path, options.session_id.as_deref(), pick)?; + // An id given next to a path is validated like any other before a reader sees it. + let supplied = + options.session_id.as_deref().map(plannotator_tui_hosts::validate_session_id).transpose()?; + let (transcript, messages) = readers::explicit(host, &path, supplied, pick)?; (transcript, messages, Discovery::Exact) } else if let Some(id) = options.session_id.as_deref() { // Validation precedes cwd lookup and every resolver filesystem access. @@ -53,11 +56,20 @@ pub(crate) fn locate(options: &LastOptions) -> Result { if messages.is_empty() { bail!("transcript {} has no assistant messages yet", transcript.display()); } - let session_id = - options.session_id.clone().or_else(|| plannotator_tui_hosts::session_id_of(host, &transcript)); + let session_id = session_id_for(host, &transcript, options.session_id.as_deref()); Ok(Located { host, transcript, session_id, messages, discovery }) } +/// The id to record for a transcript. A transcript whose name carries its own id is the +/// authority, so an id supplied next to a path cannot label the file as another session. +/// Only hosts without such names (pi, omp, Hermes, `OpenCode`) take the supplied id, and only +/// when it is shaped like one; anything else is left unknown rather than recorded wrong. +fn session_id_for(host: Host, transcript: &Path, supplied: Option<&str>) -> Option { + plannotator_tui_hosts::session_id_of(host, transcript).or_else(|| { + supplied.and_then(|id| plannotator_tui_hosts::validate_session_id(id).ok()).map(str::to_owned) + }) +} + /// Was a host named explicitly, by flag or by the launcher? fn host_named(options: &LastOptions) -> bool { options.host.as_deref().is_some_and(|host| !host.trim().is_empty()) @@ -156,6 +168,23 @@ mod tests { path } + #[test] + fn a_transcript_without_an_id_in_its_name_keeps_a_valid_supplied_id() { + let dir = std::env::temp_dir().join(format!("plannotator locate keep-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("temp dir"); + let options = LastOptions { + host: Some("claude".to_owned()), + session: Some(transcript(&dir)), + session_id: Some("given-by-herdr".to_owned()), + pick: 25, + ..LastOptions::default() + }; + let located = locate(&options).expect("the named transcript is read"); + assert_eq!(located.session_id.as_deref(), Some("given-by-herdr")); + std::fs::remove_dir_all(&dir).expect("cleanup"); + } + #[test] fn a_transcript_named_on_the_command_line_is_never_reported_as_a_guess() { let dir = std::env::temp_dir().join(format!("plannotator locate ü-{}", std::process::id())); @@ -177,7 +206,7 @@ mod tests { } #[test] - fn a_uuid_named_transcript_yields_its_id_and_an_explicit_id_wins() { + fn the_transcript_name_is_the_authority_over_a_supplied_id() { let dir = std::env::temp_dir().join(format!("plannotator locate id-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).expect("temp dir"); @@ -194,9 +223,18 @@ mod tests { let located = locate(&options).expect("the named transcript is read"); assert_eq!(located.session_id.as_deref(), Some(id)); - let explicit = LastOptions { session_id: Some("given-by-herdr".to_owned()), ..options }; - let located = locate(&explicit).expect("the named transcript is read"); - assert_eq!(located.session_id.as_deref(), Some("given-by-herdr")); + // An id that contradicts the file's own name must not relabel the file. + let other = LastOptions { + session_id: Some("22222222-2222-4222-8222-222222222222".to_owned()), + ..options.clone() + }; + let located = locate(&other).expect("the named transcript is read"); + assert_eq!(located.session_id.as_deref(), Some(id)); + + // A path-shaped id is rejected before any reader runs. + let bogus = LastOptions { session_id: Some("/etc/passwd".to_owned()), ..options }; + let err = locate(&bogus).err().expect("a path is not a session id"); + assert!(err.to_string().contains("invalid session id"), "{err:#}"); std::fs::remove_dir_all(&dir).expect("cleanup"); } }