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 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/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/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..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)] @@ -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 { @@ -157,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. /// @@ -205,3 +244,45 @@ pub fn detect_host(env: impl Fn(&str) -> Option) -> Result = (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/app/mod.rs b/crates/plannotator-tui/src/app/mod.rs index e18e65c..2dc6927 100644 --- a/crates/plannotator-tui/src/app/mod.rs +++ b/crates/plannotator-tui/src/app/mod.rs @@ -148,7 +148,10 @@ pub(crate) struct App { /// rather than dropped. pick_cache: HashMap, 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 1b58a33..1f4d3fa 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,65 @@ 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(session_id: Option<&str>, delivery: Box) -> App { + let mut app = + 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 { @@ -73,6 +127,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 +163,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(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); @@ -128,8 +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 = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + 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); @@ -139,8 +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 = App::open_message("claude", "/tmp/transcript.jsonl", candidates(), 60, Box::new(Discard)) - .expect("opens"); + 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"); @@ -151,8 +204,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(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"); @@ -163,8 +215,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(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); @@ -214,6 +265,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"); 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); + } +} diff --git a/crates/plannotator-tui/src/last/locate.rs b/crates/plannotator-tui/src/last/locate.rs index 2f0bb52..e47d281 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 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, /// How the transcript was chosen; the UI says so when nothing identified it exactly. @@ -32,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. @@ -50,7 +56,18 @@ 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 = 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? @@ -151,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())); @@ -167,6 +201,40 @@ 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 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"); + 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)); + + // 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"); } } 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()), }, )