From 5fb53a78dd6cbe37fad95e08cb79c32af50cacb7 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 14:23:00 +0800 Subject: [PATCH 1/5] refactor(permission): share path normalization and simplify rules --- crates/oxide-code/src/permission.rs | 95 +++++++++++++----------- crates/oxide-code/src/permission/rule.rs | 52 ++++++------- 2 files changed, 75 insertions(+), 72 deletions(-) diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 67ba1c9f..e832f042 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -20,12 +20,11 @@ use serde::{Deserialize, Serialize}; use unicode_normalization::UnicodeNormalization; use crate::permission::rule::{MatchDiscipline, Rule}; -use crate::util::path::xdg_dir; +use crate::util::path::{lexical_normalize, xdg_dir}; // ── Mode ── -/// The standing permission posture, shaped like [`crate::config::Effort`] so it threads through -/// config and a future `/permission` control the same way. +/// The standing permission posture. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub(crate) enum Mode { @@ -40,9 +39,6 @@ pub(crate) enum Mode { } impl Mode { - /// Every mode in display order, for the `/permission` picker. - #[cfg(test)] - pub(crate) const ALL: [Self; 3] = [Self::Auto, Self::Plan, Self::Yolo]; pub(crate) const VALID_VALUES: &str = "auto, plan, yolo"; pub(crate) const fn as_str(self) -> &'static str { @@ -434,22 +430,6 @@ fn enqueue(path: &Path, resolved: &mut PathBuf, pending: &mut Vec) { })); } -/// Resolves `.` and `..` components in `path` without touching the filesystem. A leading `..` that -/// would escape the root is dropped, matching how the OS clamps traversal at `/`. -fn lexical_normalize(path: &Path) -> PathBuf { - let mut out = PathBuf::new(); - for component in path.components() { - match component { - Component::ParentDir => { - _ = out.pop(); - } - Component::CurDir => {} - other => out.push(other), - } - } - out -} - /// Whether an edit to `path` must ask: a protected name where it takes effect, or a path under /// one of the [`protected_roots`]. Names compare by [`fold_name`], since a case-insensitive file /// system such as APFS opens `.VSCODE` and `.vſcode` as `.vscode`. @@ -676,8 +656,7 @@ impl Policy { }) } - /// The user and project `read` deny rules, for a tool that reaches files below the root the gate - /// checked. + /// The user and project `read` deny rules for files below a search tool's root. pub(crate) fn read_deny(&self) -> ReadDeny { ReadDeny( self.deny @@ -689,7 +668,6 @@ impl Policy { } /// Resolves a call to a [`Decision`]. Pure and synchronous: no I/O, no async, no classifier. - /// A classifier slots in where this returns [`Decision::Ask`] from the last step. /// /// Order, stopping at the first match: /// 1. A user or project deny rule denies, in every mode. @@ -809,9 +787,9 @@ pub(crate) struct WalkDeny<'a> { } impl WalkDeny<'_> { - /// Whether a rule denies reading `path`, a file the walk found below its base, as the gate denies - /// a read of it. The walk follows no symlink, so `path` is where the file resolves, and the same - /// names below the requested base spell it the way the call does. + /// Whether a rule denies reading `path`, a file found below the walk's base. The walk follows + /// no symlink, so `path` is where the file resolves. The same names below the requested base + /// spell it the way the call does. pub(crate) fn denies(&self, path: &Path) -> bool { if self.rules.is_empty() { return false; @@ -925,7 +903,7 @@ mod tests { #[test] fn mode_parses_all_valid_values() { - for mode in Mode::ALL { + for mode in [Mode::Auto, Mode::Plan, Mode::Yolo] { assert_eq!(mode.as_str().parse::().unwrap(), mode); } } @@ -1043,7 +1021,7 @@ mod tests { #[test] fn for_path_cwd_containing_home_is_not_a_workspace() { // Launched from `$HOME` (or any ancestor of it, the root included), "inside cwd" spans - // every dotfile, so edits there must not auto-allow. Relative rule globs still see the path. + // every dotfile, so edits there must not auto-allow. Relative globs still see the path. let (_dir, root) = temp_cwd(); let home = root.join("home"); std::fs::create_dir(&home).unwrap(); @@ -1073,6 +1051,30 @@ mod tests { assert_eq!((relative, zone), (None, Zone::Outside)); } + #[test] + fn for_path_relative_cwd_parent_traversal_preserves_permission_decisions() { + let policy = policy(Mode::Auto, &["edit(**)"], &[]); + let absolute_cwd = std::env::current_dir().unwrap(); + for cwd in ["..", "../..", "missing/../.."] { + for raw in ["notes.md", ".git/config"] { + let target = GateTarget::for_path(raw, Path::new(cwd)); + let absolute = GateTarget::for_path(raw, &absolute_cwd.join(cwd)); + let (_, relative, zone) = resolved(&target); + let (_, absolute_relative, absolute_zone) = resolved(&absolute); + assert_eq!( + (relative, zone), + (absolute_relative, absolute_zone), + "{cwd:?}/{raw}", + ); + assert_eq!( + policy.decide(PermissionClass::Edit, &target.as_target()), + policy.decide(PermissionClass::Edit, &absolute.as_target()), + "{cwd:?}/{raw}", + ); + } + } + } + #[test] fn for_path_deep_existing_file_stays_a_workspace_target() { // Windows canonicalizes a path past 260 characters to a `\\?\` spelling the cwd lacks. @@ -1337,20 +1339,6 @@ mod tests { ); } - // ── lexical_normalize ── - - #[test] - fn lexical_normalize_resolves_dots_without_escaping_root() { - // A leading `..` past root is clamped, matching the OS so a traversal can't escape. - for (raw, expected) in [("/a/../../b", "/b"), ("./a/../b", "b")] { - assert_eq!( - lexical_normalize(Path::new(raw)), - PathBuf::from(expected), - "{raw}" - ); - } - } - // ── is_protected ── #[test] @@ -1924,6 +1912,25 @@ mod tests { // ── WalkDeny::denies ── + #[test] + fn walk_deny_parent_traversal_matches_the_gate_decision() { + let (_dir, cwd) = temp_cwd(); + let root = cwd.ancestors().last().unwrap(); + let absolute_rule = format!("read({}/**)", slash_separated(&cwd)); + let policy = policy(Mode::Auto, &[], &[&absolute_rule]); + let read_deny = policy.read_deny(); + let file = cwd.join("secret.txt"); + let target = GateTarget::for_path(file.to_str().unwrap(), root); + assert_eq!( + policy.decide(PermissionClass::Read, &target.as_target()), + Decision::Deny, + ); + for requested in [Path::new(".."), Path::new("../.."), Path::new(".")] { + let walk = read_deny.walk(root, requested, root); + assert!(walk.denies(&file), "{requested:?}"); + } + } + #[cfg(unix)] #[test] fn walk_deny_denies_a_file_by_the_spelling_the_call_used() { diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index df4ff3d7..ec2fd820 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -18,13 +18,13 @@ use std::fmt; use std::path::{Path, PathBuf}; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result, anyhow, bail, ensure}; use globset::{GlobBuilder, GlobMatcher}; use regex::Regex; use serde::{Deserialize, Serialize}; -use crate::permission::{PermissionClass, Target, fold_name, lexical_normalize, resolve}; -use crate::util::path::{expand_user, slash_separated}; +use crate::permission::{PermissionClass, Target, fold_name, resolve}; +use crate::util::path::{expand_user, lexical_normalize, slash_separated}; use crate::util::text::is_hidden; // ── Rule ── @@ -72,8 +72,8 @@ enum BashSpec { /// Which rule set a match is being evaluated for, selecting how compound `bash` commands, symlinked /// paths, and name spellings match. Allow widens, so it matches conservatively. Deny revokes, so it -/// matches aggressively. Carrying this as a type keeps the security-critical asymmetry legible at the two -/// call sites in [`crate::permission::Policy::decide`]. +/// matches aggressively. This keeps the security-critical asymmetry legible at the call sites in +/// [`crate::permission::Policy::decide`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum MatchDiscipline { /// A single non-compound command must match the whole rule, and a path matches only where it @@ -96,11 +96,11 @@ impl Rule { let (class, spec_str) = match (raw.find('('), raw.strip_suffix(')')) { (Some(open), Some(_)) => (&raw[..open], &raw[open + 1..raw.len() - 1]), (None, None) => (raw, ""), - _ => anyhow::bail!("permission rule {raw:?} has an unbalanced parenthesis"), + _ => bail!("permission rule {raw:?} has an unbalanced parenthesis"), }; let class = class.trim().to_lowercase(); - anyhow::ensure!( + ensure!( !class.is_empty(), "permission rule {raw:?} has no class name" ); @@ -276,7 +276,7 @@ fn parse_path_glob(spec: &str) -> Result { let pattern = slash_separated(&expand_user(spec)?); let pattern = pattern.strip_prefix("./").unwrap_or(&pattern); let path = Path::new(pattern); - anyhow::ensure!( + ensure!( path.is_absolute() || !path.has_root(), "a rooted path glob needs a drive on Windows, as in `C:/...`" ); @@ -330,9 +330,8 @@ impl BashSpec { fn matches(&self, command: &str, discipline: MatchDiscipline) -> bool { if discipline == MatchDiscipline::Deny { - // Test the whole command before splitting so a deny pattern that itself contains a chain - // operator (the shipped `* | sh` and fork-bomb defaults) still fires, then each segment - // so a danger chained behind a safe head (`ls && rm -rf`) is caught too. + // Test the whole command so deny patterns containing chain operators still fire, + // then each segment so a danger behind a safe head (`ls && rm -rf`) is caught too. return self.matches_segment(command.trim()) || split_segments(command).any(|seg| self.matches_segment(seg)); } @@ -381,16 +380,13 @@ fn split_segments(command: &str) -> impl Iterator { /// Converts a bash wildcard specifier to an anchored regex: literal text is escaped and `*` becomes /// `.*`, so `git *` matches `git status` but not `cargo git`. fn glob_to_regex(glob: &str) -> Regex { - let mut pattern = String::with_capacity(glob.len() + 4); - pattern.push('^'); - for part in glob.split('*') { - pattern.push_str(®ex::escape(part)); - pattern.push_str(".*"); - } - // Each segment appended a trailing `.*`, so drop the final one so the regex ends at the last - // literal unless the glob itself ended in `*`. - pattern.truncate(pattern.len() - 2); - pattern.push('$'); + let pattern = format!( + "^{}$", + glob.split('*') + .map(regex::escape) + .collect::>() + .join(".*") + ); Regex::new(&pattern).expect("escaped glob is always valid regex") } @@ -860,7 +856,7 @@ mod tests { // ── BashSpec::matches ── #[test] - fn bash_exact_matches_only_identical_command() { + fn matches_exact_matches_only_identical_command() { let rule = Rule::parse("bash(cargo build)").unwrap(); assert!(rule.matches( PermissionClass::Bash, @@ -875,7 +871,7 @@ mod tests { } #[test] - fn bash_prefix_matches_command_and_its_arguments() { + fn matches_prefix_matches_command_and_its_arguments() { let rule = Rule::parse("bash(cargo test:*)").unwrap(); assert!(rule.matches( PermissionClass::Bash, @@ -896,7 +892,7 @@ mod tests { } #[test] - fn bash_wildcard_anchors_both_ends() { + fn matches_wildcard_anchors_both_ends() { let rule = Rule::parse("bash(git *)").unwrap(); assert!(rule.matches( PermissionClass::Bash, @@ -911,7 +907,7 @@ mod tests { } #[test] - fn allow_prefix_refuses_compound_command() { + fn matches_allow_prefix_refuses_compound_command() { // The load-bearing safety property: `cargo test:*` must not allow a chained `rm` or a // redirect that clobbers a file. let rule = Rule::parse("bash(cargo test:*)").unwrap(); @@ -930,7 +926,7 @@ mod tests { } #[test] - fn deny_prefix_matches_any_segment_of_compound_command() { + fn matches_deny_prefix_matches_any_segment_of_compound_command() { // The mirror property: a deny must fire even when the danger is chained behind a safe head. let rule = Rule::parse("bash(rm -rf:*)").unwrap(); for command in ["rm -rf /", "ls && rm -rf /tmp/x", "echo hi; rm -rf ."] { @@ -942,7 +938,7 @@ mod tests { } #[test] - fn deny_pattern_with_a_chain_operator_matches_the_whole_command() { + fn matches_deny_pattern_with_a_chain_operator_matches_the_whole_command() { // Segmenting first would split `curl x | sh` into `curl x` and `sh`, so neither piece holds // the `|` the rule targets. The shipped `* | sh` / `* | bash` defaults rely on the whole // command being tested before the split. @@ -960,7 +956,7 @@ mod tests { } #[test] - fn deny_exact_fork_bomb_matches_the_whole_command() { + fn matches_deny_exact_fork_bomb_matches_the_whole_command() { // The fork-bomb default is an exact rule whose own text spans chain operators, so it only // matches when the unsplit command is tested. let rule = Rule::parse("bash(:(){ :|:& };:)").unwrap(); From c7b7e45d1635697eb7fb1c4e84beab06acbeb3c6 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 14:24:44 +0800 Subject: [PATCH 2/5] refactor(util): share mutex recovery and tracker verification --- crates/oxide-code/src/file_tracker.rs | 911 +++++++++++--------------- crates/oxide-code/src/util.rs | 3 +- crates/oxide-code/src/util/fs.rs | 22 +- crates/oxide-code/src/util/path.rs | 20 +- crates/oxide-code/src/util/sync.rs | 37 ++ crates/oxide-code/src/util/text.rs | 28 +- crates/oxide-code/src/util/time.rs | 19 - crates/oxide-code/src/util/tls.rs | 26 +- docs/design/architecture.md | 2 +- 9 files changed, 478 insertions(+), 590 deletions(-) create mode 100644 crates/oxide-code/src/util/sync.rs diff --git a/crates/oxide-code/src/file_tracker.rs b/crates/oxide-code/src/file_tracker.rs index 99a2e76a..43a135ef 100644 --- a/crates/oxide-code/src/file_tracker.rs +++ b/crates/oxide-code/src/file_tracker.rs @@ -4,13 +4,15 @@ use std::collections::HashMap; use std::fs::Metadata; use std::future::Future; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, MutexGuard}; use serde::{Deserialize, Serialize}; use time::OffsetDateTime; use tracing::warn; use xxhash_rust::xxh64::xxh64; +use crate::util::sync::lock_or_recover; + const HASH_SEED: u64 = 0; pub(crate) const MAX_TRACKED_FILE_SIZE: u64 = 10 * 1024 * 1024; @@ -57,114 +59,6 @@ struct FileState { tool_use_id: Option, } -/// Lines Read has shown of the tracked content, accumulated across Reads of that content. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(tag = "kind", rename_all = "snake_case")] -pub(crate) enum LastView { - Full, - /// Sorted, disjoint, inclusive 1-indexed line windows out of `total_lines`. - Partial { - ranges: Vec<(usize, usize)>, - total_lines: usize, - }, -} - -impl LastView { - /// The view of a Read that showed lines `first..=last` of `total_lines`. - pub(crate) fn lines(first: usize, last: usize, total_lines: usize) -> Self { - Self::from_ranges(vec![(first, last)], total_lines) - } - - fn union(self, other: Self) -> Self { - match (self, other) { - ( - Self::Partial { - mut ranges, - total_lines, - }, - Self::Partial { ranges: more, .. }, - ) => { - ranges.extend(more); - Self::from_ranges(ranges, total_lines) - } - _ => Self::Full, - } - } - - fn from_ranges(mut ranges: Vec<(usize, usize)>, total_lines: usize) -> Self { - ranges.sort_unstable(); - let mut merged: Vec<(usize, usize)> = Vec::with_capacity(ranges.len()); - for (first, last) in ranges { - match merged.last_mut() { - Some(prev) if first <= prev.1 + 1 => prev.1 = prev.1.max(last), - _ => merged.push((first, last)), - } - } - if merged == [(1, total_lines)] { - Self::Full - } else { - Self::Partial { - ranges: merged, - total_lines, - } - } - } -} - -/// Which mutating tool is asking for the gate. Carried into [`GateError`] so rendered errors can -/// name the blocked action. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) enum GatePurpose { - Edit, - Write, -} - -impl GatePurpose { - fn verb(self) -> &'static str { - match self { - Self::Edit => "editing", - Self::Write => "writing to", - } - } -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) enum TrackableFileError { - Directory, - NonRegular, - TooLarge { size: u64, max: u64 }, -} - -impl std::fmt::Display for TrackableFileError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Directory => f.write_str("not a regular file: directory"), - Self::NonRegular => f.write_str("not a regular file"), - Self::TooLarge { size, max } => { - write!(f, "too large to verify ({size} bytes, max {max} bytes)") - } - } - } -} - -pub(crate) fn validate_trackable_file(metadata: &Metadata) -> Result<(), TrackableFileError> { - let file_type = metadata.file_type(); - if file_type.is_dir() { - return Err(TrackableFileError::Directory); - } - if !file_type.is_file() { - return Err(TrackableFileError::NonRegular); - } - let size = metadata.len(); - if size > MAX_TRACKED_FILE_SIZE { - return Err(TrackableFileError::TooLarge { - size, - max: MAX_TRACKED_FILE_SIZE, - }); - } - Ok(()) -} - impl FileTracker { /// Returns `CacheHit` when a file already tracked as `Full` is read in full again unchanged. /// Reads of unchanged content accumulate their line windows, and changed content starts over. @@ -299,39 +193,9 @@ impl FileTracker { } for snap in latest.into_values() { - let metadata = match std::fs::metadata(&snap.path) { - Ok(metadata) => metadata, - Err(e) => { - warn!( - "dropping tracked file {} (stat failed, will require fresh Read): {e}", - snap.path.display() - ); - dropped.push(snap.path); - continue; - } - }; - if let Err(err) = validate_trackable_file(&metadata) { - warn!( - "dropping tracked file {} ({err}, will require fresh Read)", - snap.path.display() - ); - dropped.push(snap.path); - continue; - } - let current_bytes = match std::fs::read(&snap.path) { - Ok(bytes) => bytes, - Err(e) => { - warn!( - "dropping tracked file {} (read failed, will require fresh Read): {e}", - snap.path.display() - ); - dropped.push(snap.path); - continue; - } - }; - if xxh64(¤t_bytes, HASH_SEED) != snap.content_hash { + if let Err(reason) = verify_on_disk(&snap) { warn!( - "dropping tracked file {} (content drift, will require fresh Read)", + "dropping tracked file {} ({reason}, will require fresh Read)", snap.path.display() ); dropped.push(snap.path); @@ -356,17 +220,125 @@ impl FileTracker { dropped } - fn lock(&self) -> std::sync::MutexGuard<'_, HashMap> { - match self.by_path.lock() { - Ok(guard) => guard, - Err(e) => { - tracing::warn!("FileTracker mutex poisoned, recovering"); - e.into_inner() + fn lock(&self) -> MutexGuard<'_, HashMap> { + lock_or_recover(&self.by_path, "FileTracker") + } +} + +fn verify_on_disk(snapshot: &FileSnapshot) -> Result<(), String> { + let metadata = + std::fs::metadata(&snapshot.path).map_err(|error| format!("stat failed: {error}"))?; + validate_trackable_file(&metadata).map_err(|error| error.to_string())?; + let bytes = std::fs::read(&snapshot.path).map_err(|error| format!("read failed: {error}"))?; + if xxh64(&bytes, HASH_SEED) != snapshot.content_hash { + return Err("content drift".to_owned()); + } + Ok(()) +} + +// ── LastView ── + +/// Lines Read has shown of the tracked content, accumulated across Reads of that content. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case")] +pub(crate) enum LastView { + Full, + /// Sorted, disjoint, inclusive 1-indexed line windows out of `total_lines`. + Partial { + ranges: Vec<(usize, usize)>, + total_lines: usize, + }, +} + +impl LastView { + /// The view of a Read that showed lines `first..=last` of `total_lines`. + pub(crate) fn lines(first: usize, last: usize, total_lines: usize) -> Self { + Self::from_ranges(vec![(first, last)], total_lines) + } + + fn union(self, other: Self) -> Self { + match (self, other) { + ( + Self::Partial { + mut ranges, + total_lines, + }, + Self::Partial { ranges: more, .. }, + ) => { + ranges.extend(more); + Self::from_ranges(ranges, total_lines) + } + _ => Self::Full, + } + } + + fn from_ranges(mut ranges: Vec<(usize, usize)>, total_lines: usize) -> Self { + ranges.sort_unstable(); + let mut merged: Vec<(usize, usize)> = Vec::with_capacity(ranges.len()); + for (first, last) in ranges { + match merged.last_mut() { + Some(prev) if first <= prev.1 + 1 => prev.1 = prev.1.max(last), + _ => merged.push((first, last)), + } + } + if merged == [(1, total_lines)] { + Self::Full + } else { + Self::Partial { + ranges: merged, + total_lines, } } } } +// ── File Gates ── + +/// Which mutating tool is asking for the gate. Carried into [`GateError`] so rendered errors can +/// name the blocked action. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum GatePurpose { + Edit, + Write, +} + +impl GatePurpose { + fn verb(self) -> &'static str { + match self { + Self::Edit => "editing", + Self::Write => "writing to", + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] +pub(crate) enum TrackableFileError { + #[error("not a regular file: directory")] + Directory, + #[error("not a regular file")] + NonRegular, + #[error("too large to verify ({size} bytes, max {max} bytes)")] + TooLarge { size: u64, max: u64 }, +} + +pub(crate) fn validate_trackable_file(metadata: &Metadata) -> Result<(), TrackableFileError> { + let file_type = metadata.file_type(); + if file_type.is_dir() { + return Err(TrackableFileError::Directory); + } + if !file_type.is_file() { + return Err(TrackableFileError::NonRegular); + } + let size = metadata.len(); + if size > MAX_TRACKED_FILE_SIZE { + return Err(TrackableFileError::TooLarge { + size, + max: MAX_TRACKED_FILE_SIZE, + }); + } + Ok(()) +} + // ── FileSnapshot ── /// JSONL-persisted record of one tracked file. Written into the session log on `finish` and read @@ -449,12 +421,21 @@ mod tests { use std::collections::BTreeMap; #[cfg(unix)] use std::os::unix::fs::PermissionsExt as _; - use std::sync::Arc; use std::thread; use std::time::Duration; use super::*; + fn snapshot(path: &Path, content: &[u8], recorded_at: OffsetDateTime) -> FileSnapshot { + FileSnapshot { + path: path.to_path_buf(), + content_hash: xxh64(content, HASH_SEED), + last_view: LastView::Full, + recorded_at, + tool_use_id: None, + } + } + // ── attribute_to ── #[tokio::test] @@ -514,167 +495,30 @@ mod tests { Ok((path, bytes.to_vec())) } - // ── LastView::lines ── + // ── record_read ── #[test] - fn lines_covering_every_line_is_full() { - assert_eq!(LastView::lines(1, 3, 3), LastView::Full); - assert_eq!( - LastView::lines(2, 3, 3), - LastView::Partial { - ranges: vec![(2, 3)], - total_lines: 3, - }, - ); + fn record_read_first_full_read_inserts() { + let tracker = FileTracker::default(); + let path = Path::new("/tmp/a.rs"); + let outcome = tracker.record_read(path, b"hello", LastView::Full); + assert_eq!(outcome, RecordRead::Inserted); + let stored = tracker.lock().get(path).cloned().unwrap(); + assert_eq!(stored.content_hash, xxh64(b"hello", HASH_SEED)); + assert_eq!(stored.last_view, LastView::Full); } - // ── LastView::union ── - - #[test] - fn union_merges_overlapping_and_adjacent_windows_until_full() { - let view = LastView::lines(5, 6, 10) - .union(LastView::lines(1, 2, 10)) - .union(LastView::lines(3, 3, 10)) - .union(LastView::lines(8, 9, 10)) - .union(LastView::lines(6, 6, 10)); - assert_eq!( - view, - LastView::Partial { - ranges: vec![(1, 3), (5, 6), (8, 9)], - total_lines: 10, - }, - ); - - assert_eq!(view.union(LastView::lines(4, 10, 10)), LastView::Full); - } - - #[test] - fn union_with_full_is_full() { - assert_eq!( - LastView::Full.union(LastView::lines(1, 1, 5)), - LastView::Full - ); - assert_eq!( - LastView::lines(1, 1, 5).union(LastView::Full), - LastView::Full - ); - } - - // ── TrackableFileError Display ── - - #[test] - fn trackable_file_error_display_names_directory() { - let err = TrackableFileError::Directory; - assert_eq!(err.to_string(), "not a regular file: directory"); - } - - #[test] - fn trackable_file_error_display_names_non_regular_file() { - let err = TrackableFileError::NonRegular; - assert_eq!(err.to_string(), "not a regular file"); - } - - #[test] - fn trackable_file_error_display_names_size_limit() { - let err = TrackableFileError::TooLarge { size: 12, max: 10 }; - assert_eq!( - err.to_string(), - "too large to verify (12 bytes, max 10 bytes)" - ); - } - - // ── validate_trackable_file ── - - #[test] - fn validate_trackable_file_accepts_regular_file_at_limit() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("tracked.rs"); - let f = std::fs::File::create(&path).unwrap(); - f.set_len(MAX_TRACKED_FILE_SIZE).unwrap(); - let metadata = std::fs::metadata(&path).unwrap(); - - assert_eq!(validate_trackable_file(&metadata), Ok(())); - } - - #[test] - fn validate_trackable_file_rejects_directory() { - let dir = tempfile::tempdir().unwrap(); - let metadata = std::fs::metadata(dir.path()).unwrap(); - - assert_eq!( - validate_trackable_file(&metadata), - Err(TrackableFileError::Directory) - ); - } - - #[cfg(unix)] - #[test] - fn validate_trackable_file_rejects_non_regular_file() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("socket"); - let _listener = std::os::unix::net::UnixListener::bind(&path).unwrap(); - let metadata = std::fs::metadata(&path).unwrap(); - - assert_eq!( - validate_trackable_file(&metadata), - Err(TrackableFileError::NonRegular) - ); - } - - #[test] - fn validate_trackable_file_rejects_oversized_file() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("huge.rs"); - let f = std::fs::File::create(&path).unwrap(); - f.set_len(MAX_TRACKED_FILE_SIZE + 1).unwrap(); - let metadata = std::fs::metadata(&path).unwrap(); - - assert_eq!( - validate_trackable_file(&metadata), - Err(TrackableFileError::TooLarge { - size: MAX_TRACKED_FILE_SIZE + 1, - max: MAX_TRACKED_FILE_SIZE, - }), - ); - } - - // ── record_read ── - - #[test] - fn record_read_first_full_read_inserts() { - let tracker = FileTracker::default(); - let path = Path::new("/tmp/a.rs"); - let outcome = tracker.record_read(path, b"hello", LastView::Full); - assert_eq!(outcome, RecordRead::Inserted); - let stored = tracker.lock().get(path).cloned().unwrap(); - assert_eq!(stored.content_hash, xxh64(b"hello", HASH_SEED)); - assert_eq!(stored.last_view, LastView::Full); - } - - #[test] - fn record_read_redundant_full_read_is_cache_hit() { - let tracker = FileTracker::default(); - let path = Path::new("/tmp/a.rs"); - _ = tracker.record_read(path, b"hello", LastView::Full); - let outcome = tracker.record_read(path, b"hello", LastView::Full); - assert_eq!( - outcome, - RecordRead::CacheHit, - "second full read of unchanged file is a cache hit", - ); - } - - #[test] - fn record_read_cache_hit_preserves_recorded_at() { - let tracker = FileTracker::default(); - let path = Path::new("/tmp/a.rs"); - _ = tracker.record_read(path, b"hello", LastView::Full); - let first = tracker.lock().get(path).cloned().unwrap(); - let outcome = tracker.record_read(path, b"hello", LastView::Full); - let second = tracker.lock().get(path).cloned().unwrap(); - assert_eq!(outcome, RecordRead::CacheHit); - assert_eq!(first.recorded_at, second.recorded_at); - } + #[test] + fn record_read_cache_hit_preserves_recorded_at() { + let tracker = FileTracker::default(); + let path = Path::new("/tmp/a.rs"); + _ = tracker.record_read(path, b"hello", LastView::Full); + let first = tracker.lock().get(path).cloned().unwrap(); + let outcome = tracker.record_read(path, b"hello", LastView::Full); + let second = tracker.lock().get(path).cloned().unwrap(); + assert_eq!(outcome, RecordRead::CacheHit); + assert_eq!(first.recorded_at, second.recorded_at); + } #[test] fn record_read_changed_content_inserts_not_cache_hit() { @@ -746,6 +590,23 @@ mod tests { ); } + #[test] + fn record_read_concurrent_files_preserve_every_entry() { + let tracker = Arc::new(FileTracker::default()); + thread::scope(|s| { + for t in 0..8u32 { + let tracker = Arc::clone(&tracker); + s.spawn(move || { + for i in 0..100u32 { + let path = PathBuf::from(format!("/tmp/t{t}/p{i}")); + _ = tracker.record_read(&path, &t.to_le_bytes(), LastView::Full); + } + }); + } + }); + assert_eq!(tracker.lock().len(), 8 * 100); + } + // ── verify_current_content ── #[test] @@ -760,31 +621,18 @@ mod tests { } #[test] - fn verify_current_content_no_entry_errors_never_read() { + fn verify_current_content_unread_file_reports_the_blocked_action() { let tracker = FileTracker::default(); let path = Path::new("/tmp/a.rs"); - let result = tracker.verify_current_content(path, b"", GatePurpose::Edit); - assert_eq!( - result, - Err(GateError::NeverRead { - path: path.to_path_buf(), - purpose: GatePurpose::Edit, - }), - ); - } - - #[test] - fn verify_current_content_no_entry_carries_write_purpose() { - let tracker = FileTracker::default(); - let path = Path::new("/tmp/a.rs"); - let result = tracker.verify_current_content(path, b"", GatePurpose::Write); - assert_eq!( - result, - Err(GateError::NeverRead { - path: path.to_path_buf(), - purpose: GatePurpose::Write, - }), - ); + for purpose in [GatePurpose::Edit, GatePurpose::Write] { + assert_eq!( + tracker.verify_current_content(path, b"", purpose), + Err(GateError::NeverRead { + path: path.to_path_buf(), + purpose + }), + ); + } } #[test] @@ -829,56 +677,6 @@ mod tests { ); } - // ── GateError Display ── - - #[test] - fn gate_error_never_read_renders_with_path_and_edit_verb() { - let err = GateError::NeverRead { - path: PathBuf::from("/tmp/a.rs"), - purpose: GatePurpose::Edit, - }; - let msg = err.to_string(); - assert!(msg.contains("/tmp/a.rs"), "path is named: {msg}"); - assert!(msg.contains("has not been read")); - assert!(msg.contains("editing")); - } - - #[test] - fn gate_error_never_read_renders_with_write_verb() { - let err = GateError::NeverRead { - path: PathBuf::from("/tmp/a.rs"), - purpose: GatePurpose::Write, - }; - let msg = err.to_string(); - assert!(msg.contains("has not been read")); - assert!(msg.contains("writing to")); - } - - #[test] - fn gate_error_partial_read_renders_with_path_and_remaining_lines() { - let err = GateError::PartialRead { - path: PathBuf::from("/tmp/a.rs"), - purpose: GatePurpose::Write, - }; - let msg = err.to_string(); - assert!(msg.contains("/tmp/a.rs"), "path is named: {msg}"); - assert!(msg.contains("only been read in part")); - assert!(msg.contains("Read the remaining lines")); - assert!(msg.contains("writing to")); - } - - #[test] - fn gate_error_content_drifted_renders_with_path_and_modified_externally() { - let err = GateError::ContentDrifted { - path: PathBuf::from("/tmp/a.rs"), - purpose: GatePurpose::Edit, - }; - let msg = err.to_string(); - assert!(msg.contains("/tmp/a.rs"), "path is named: {msg}"); - assert!(msg.contains("modified externally")); - assert!(msg.contains("Re-read")); - } - // ── write_and_record ── #[tokio::test] @@ -998,13 +796,7 @@ mod tests { let path = dir.path().join("a.rs"); std::fs::write(&path, b"persisted bytes").unwrap(); - let snap = FileSnapshot { - path: path.clone(), - content_hash: xxh64(b"persisted bytes", HASH_SEED), - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }; + let snap = snapshot(&path, b"persisted bytes", OffsetDateTime::now_utc()); let tracker = FileTracker::default(); tracker.restore_verified(vec![snap.clone()]); @@ -1021,13 +813,7 @@ mod tests { let path = dir.path().join("a.rs"); std::fs::write(&path, b"new").unwrap(); - let snap = FileSnapshot { - path: path.clone(), - content_hash: xxh64(b"old", HASH_SEED), - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }; + let snap = snapshot(&path, b"old", OffsetDateTime::now_utc()); let tracker = FileTracker::default(); let dropped = tracker.restore_verified(vec![snap]); @@ -1035,56 +821,12 @@ mod tests { assert!(tracker.lock().get(&path).is_none()); } - #[test] - fn restore_verified_missing_file_drops_snapshot() { - let snap = FileSnapshot { - path: PathBuf::from("/nonexistent/path/a.rs"), - content_hash: 0, - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }; - - let tracker = FileTracker::default(); - tracker.restore_verified(vec![snap]); - assert!(tracker.lock().is_empty()); - } - - #[cfg(unix)] - #[test] - fn restore_verified_unreadable_file_drops_snapshot() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("a.rs"); - std::fs::write(&path, b"content").unwrap(); - let snap = FileSnapshot { - path: path.clone(), - content_hash: xxh64(b"content", HASH_SEED), - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }; - - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); - let tracker = FileTracker::default(); - let dropped = tracker.restore_verified(vec![snap]); - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); - - assert_eq!(dropped, vec![path.clone()]); - assert!(tracker.lock().is_empty()); - } - #[test] fn restore_verified_non_regular_file_drops_snapshot() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("subdir"); std::fs::create_dir(&path).unwrap(); - let snap = FileSnapshot { - path: path.clone(), - content_hash: 0, - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }; + let snap = snapshot(&path, b"", OffsetDateTime::now_utc()); let tracker = FileTracker::default(); let dropped = tracker.restore_verified(vec![snap]); @@ -1099,13 +841,7 @@ mod tests { let path = dir.path().join("huge.rs"); let f = std::fs::File::create(&path).unwrap(); f.set_len(MAX_TRACKED_FILE_SIZE + 1).unwrap(); - let snap = FileSnapshot { - path: path.clone(), - content_hash: 0, - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }; + let snap = snapshot(&path, b"", OffsetDateTime::now_utc()); let tracker = FileTracker::default(); let dropped = tracker.restore_verified(vec![snap]); @@ -1115,7 +851,7 @@ mod tests { } #[test] - fn restore_verified_returns_paths_of_dropped_snapshots() { + fn restore_verified_reports_dropped_paths_in_sorted_order() { let dir = tempfile::tempdir().unwrap(); let kept_path = dir.path().join("kept.rs"); let drifted_path = dir.path().join("drifted.rs"); @@ -1124,39 +860,21 @@ mod tests { let missing_path = PathBuf::from("/nonexistent/x.rs"); let snaps = vec![ - FileSnapshot { - path: kept_path.clone(), - content_hash: xxh64(b"alpha", HASH_SEED), - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }, - FileSnapshot { - path: drifted_path.clone(), - content_hash: 0, - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }, - FileSnapshot { - path: missing_path.clone(), - content_hash: 0, - last_view: LastView::Full, - recorded_at: OffsetDateTime::now_utc(), - tool_use_id: None, - }, + snapshot(&kept_path, b"alpha", OffsetDateTime::now_utc()), + snapshot(&drifted_path, b"", OffsetDateTime::now_utc()), + snapshot(&missing_path, b"", OffsetDateTime::now_utc()), ]; let tracker = FileTracker::default(); - let mut dropped = tracker.restore_verified(snaps); - dropped.sort(); + let dropped = tracker.restore_verified(snaps); let mut expected = vec![drifted_path, missing_path]; expected.sort(); assert_eq!( dropped, expected, "both the drifted and the missing snapshots must be reported", ); - assert!(tracker.lock().contains_key(&kept_path)); + let stored_paths: Vec<_> = tracker.lock().keys().cloned().collect(); + assert_eq!(stored_paths, vec![kept_path]); } #[test] @@ -1164,22 +882,16 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("a.rs"); std::fs::write(&path, b"x").unwrap(); - let content_hash = xxh64(b"x", HASH_SEED); let older = FileSnapshot { - path: path.clone(), - content_hash: xxh64(b"stale", HASH_SEED), last_view: LastView::lines(1, 1, 2), - recorded_at: OffsetDateTime::UNIX_EPOCH, - tool_use_id: None, - }; - let newer = FileSnapshot { - path: path.clone(), - content_hash, - last_view: LastView::Full, - recorded_at: OffsetDateTime::UNIX_EPOCH + Duration::from_mins(1), - tool_use_id: None, + ..snapshot(&path, b"stale", OffsetDateTime::UNIX_EPOCH) }; + let newer = snapshot( + &path, + b"x", + OffsetDateTime::UNIX_EPOCH + Duration::from_mins(1), + ); let tracker = FileTracker::default(); let dropped = tracker.restore_verified(vec![older.clone(), newer.clone()]); @@ -1198,38 +910,165 @@ mod tests { ); } - // ── Concurrency ── + #[cfg(unix)] + #[test] + fn restore_verified_unreadable_file_drops_snapshot() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("a.rs"); + std::fs::write(&path, b"content").unwrap(); + let snap = snapshot(&path, b"content", OffsetDateTime::now_utc()); + + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); + let tracker = FileTracker::default(); + let dropped = tracker.restore_verified(vec![snap]); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + assert_eq!(dropped, vec![path.clone()]); + assert!(tracker.lock().is_empty()); + } + + // ── LastView::lines ── #[test] - fn concurrent_record_read_does_not_corrupt_map() { - let tracker = Arc::new(FileTracker::default()); - thread::scope(|s| { - for t in 0..8u32 { - let tracker = Arc::clone(&tracker); - s.spawn(move || { - for i in 0..100u32 { - let path = PathBuf::from(format!("/tmp/t{t}/p{i}")); - _ = tracker.record_read(&path, &t.to_le_bytes(), LastView::Full); - } - }); - } - }); - assert_eq!(tracker.lock().len(), 8 * 100); + fn lines_covering_every_line_is_full() { + assert_eq!(LastView::lines(1, 3, 3), LastView::Full); + assert_eq!( + LastView::lines(2, 3, 3), + LastView::Partial { + ranges: vec![(2, 3)], + total_lines: 3, + }, + ); } + // ── LastView::union ── + #[test] - fn lock_recovers_from_poisoned_mutex() { - let tracker = Arc::new(FileTracker::default()); - let t = Arc::clone(&tracker); - _ = std::thread::spawn(move || { - let _guard = t.by_path.lock().unwrap(); - panic!("deliberate poison"); - }) - .join(); - // Mutex is now poisoned. record_read must recover via lock(). - let result = tracker.record_read(Path::new("/tmp/poison_test"), b"hello", LastView::Full); - assert_eq!(result, RecordRead::Inserted); - assert_eq!(tracker.lock().len(), 1); + fn union_merges_overlapping_and_adjacent_windows_until_full() { + let view = LastView::lines(5, 6, 10) + .union(LastView::lines(1, 2, 10)) + .union(LastView::lines(3, 3, 10)) + .union(LastView::lines(8, 9, 10)) + .union(LastView::lines(6, 6, 10)); + assert_eq!( + view, + LastView::Partial { + ranges: vec![(1, 3), (5, 6), (8, 9)], + total_lines: 10, + }, + ); + + assert_eq!(view.union(LastView::lines(4, 10, 10)), LastView::Full); + } + + #[test] + fn union_with_full_is_full() { + assert_eq!( + LastView::Full.union(LastView::lines(1, 1, 5)), + LastView::Full + ); + assert_eq!( + LastView::lines(1, 1, 5).union(LastView::Full), + LastView::Full + ); + } + + // ── LastView ── + + #[test] + fn last_view_full_serializes_kind_only() { + let json = serde_json::to_value(LastView::Full).unwrap(); + assert_eq!(json, serde_json::json!({"kind": "full"})); + } + + #[test] + fn last_view_partial_round_trips_through_json() { + let view = LastView::lines(3, 5, 9); + let json = serde_json::to_value(&view).unwrap(); + assert_eq!( + json, + serde_json::json!({"kind": "partial", "ranges": [[3, 5]], "total_lines": 9}), + ); + let parsed: LastView = serde_json::from_value(json).unwrap(); + assert_eq!(parsed, view); + } + + // ── TrackableFileError Display ── + + #[test] + fn trackable_file_error_display_names_directory() { + let err = TrackableFileError::Directory; + assert_eq!(err.to_string(), "not a regular file: directory"); + } + + #[test] + fn trackable_file_error_display_names_non_regular_file() { + let err = TrackableFileError::NonRegular; + assert_eq!(err.to_string(), "not a regular file"); + } + + #[test] + fn trackable_file_error_display_names_size_limit() { + let err = TrackableFileError::TooLarge { size: 12, max: 10 }; + assert_eq!( + err.to_string(), + "too large to verify (12 bytes, max 10 bytes)" + ); + } + + // ── validate_trackable_file ── + + #[test] + fn validate_trackable_file_accepts_regular_file_at_limit() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("tracked.rs"); + let f = std::fs::File::create(&path).unwrap(); + f.set_len(MAX_TRACKED_FILE_SIZE).unwrap(); + let metadata = std::fs::metadata(&path).unwrap(); + + assert_eq!(validate_trackable_file(&metadata), Ok(())); + } + + #[test] + fn validate_trackable_file_rejects_directory() { + let dir = tempfile::tempdir().unwrap(); + let metadata = std::fs::metadata(dir.path()).unwrap(); + + assert_eq!( + validate_trackable_file(&metadata), + Err(TrackableFileError::Directory) + ); + } + + #[test] + fn validate_trackable_file_rejects_oversized_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("huge.rs"); + let f = std::fs::File::create(&path).unwrap(); + f.set_len(MAX_TRACKED_FILE_SIZE + 1).unwrap(); + let metadata = std::fs::metadata(&path).unwrap(); + + assert_eq!( + validate_trackable_file(&metadata), + Err(TrackableFileError::TooLarge { + size: MAX_TRACKED_FILE_SIZE + 1, + max: MAX_TRACKED_FILE_SIZE, + }), + ); + } + + #[cfg(unix)] + #[test] + fn validate_trackable_file_rejects_non_regular_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("socket"); + let _listener = std::os::unix::net::UnixListener::bind(&path).unwrap(); + let metadata = std::fs::metadata(&path).unwrap(); + + assert_eq!( + validate_trackable_file(&metadata), + Err(TrackableFileError::NonRegular) + ); } // ── FileSnapshot ── @@ -1260,23 +1099,53 @@ mod tests { assert_eq!(parsed, snap); } - // ── LastView ── + // ── GateError Display ── #[test] - fn last_view_full_serializes_kind_only() { - let json = serde_json::to_value(LastView::Full).unwrap(); - assert_eq!(json, serde_json::json!({"kind": "full"})); + fn gate_error_never_read_renders_with_path_and_edit_verb() { + let err = GateError::NeverRead { + path: PathBuf::from("/tmp/a.rs"), + purpose: GatePurpose::Edit, + }; + let msg = err.to_string(); + assert!(msg.contains("/tmp/a.rs"), "path is named: {msg}"); + assert!(msg.contains("has not been read")); + assert!(msg.contains("editing")); } #[test] - fn last_view_partial_round_trips_through_json() { - let view = LastView::lines(3, 5, 9); - let json = serde_json::to_value(&view).unwrap(); - assert_eq!( - json, - serde_json::json!({"kind": "partial", "ranges": [[3, 5]], "total_lines": 9}), - ); - let parsed: LastView = serde_json::from_value(json).unwrap(); - assert_eq!(parsed, view); + fn gate_error_never_read_renders_with_write_verb() { + let err = GateError::NeverRead { + path: PathBuf::from("/tmp/a.rs"), + purpose: GatePurpose::Write, + }; + let msg = err.to_string(); + assert!(msg.contains("has not been read")); + assert!(msg.contains("writing to")); + } + + #[test] + fn gate_error_partial_read_renders_with_path_and_remaining_lines() { + let err = GateError::PartialRead { + path: PathBuf::from("/tmp/a.rs"), + purpose: GatePurpose::Write, + }; + let msg = err.to_string(); + assert!(msg.contains("/tmp/a.rs"), "path is named: {msg}"); + assert!(msg.contains("only been read in part")); + assert!(msg.contains("Read the remaining lines")); + assert!(msg.contains("writing to")); + } + + #[test] + fn gate_error_content_drifted_renders_with_path_and_modified_externally() { + let err = GateError::ContentDrifted { + path: PathBuf::from("/tmp/a.rs"), + purpose: GatePurpose::Edit, + }; + let msg = err.to_string(); + assert!(msg.contains("/tmp/a.rs"), "path is named: {msg}"); + assert!(msg.contains("modified externally")); + assert!(msg.contains("Re-read")); } } diff --git a/crates/oxide-code/src/util.rs b/crates/oxide-code/src/util.rs index 350b73d4..bab3314a 100644 --- a/crates/oxide-code/src/util.rs +++ b/crates/oxide-code/src/util.rs @@ -1,11 +1,12 @@ //! Shared utilities: env var lookup, private file writes, git probes, tracing init, path display, -//! text width, the local-offset cache, and extra TLS trust anchors. +//! mutex recovery, text width, the local-offset cache, and extra TLS trust anchors. pub(crate) mod env; pub(crate) mod fs; pub(crate) mod git; pub(crate) mod log; pub(crate) mod path; +pub(crate) mod sync; pub(crate) mod text; pub(crate) mod time; pub(crate) mod tls; diff --git a/crates/oxide-code/src/util/fs.rs b/crates/oxide-code/src/util/fs.rs index 7eff0aad..4ae46348 100644 --- a/crates/oxide-code/src/util/fs.rs +++ b/crates/oxide-code/src/util/fs.rs @@ -77,7 +77,7 @@ pub(crate) fn atomic_write_private(path: &Path, bytes: &[u8]) -> Result<()> { #[cfg(test)] mod tests { - use std::fs; + use std::ffi::OsString; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; @@ -85,6 +85,14 @@ mod tests { use super::*; + fn tmp_leftovers(dir: &Path) -> Vec { + fs::read_dir(dir) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .filter(|name| name.to_string_lossy().contains(".tmp.")) + .collect() + } + // ── create_private_dir_all ── #[test] @@ -183,11 +191,7 @@ mod tests { let dir = tempdir().unwrap(); let path = dir.path().join("file"); atomic_write_private(&path, b"x").unwrap(); - let leftovers: Vec<_> = fs::read_dir(dir.path()) - .unwrap() - .map(|e| e.unwrap().file_name()) - .filter(|n| n.to_string_lossy().contains(".tmp.")) - .collect(); + let leftovers = tmp_leftovers(dir.path()); assert!(leftovers.is_empty(), "tmp leftovers: {leftovers:?}"); } @@ -227,11 +231,7 @@ mod tests { chain.contains("failed to install file"), "actionable error: {chain}" ); - let leftovers: Vec<_> = fs::read_dir(dir.path()) - .unwrap() - .map(|e| e.unwrap().file_name()) - .filter(|n| n.to_string_lossy().contains(".tmp.")) - .collect(); + let leftovers = tmp_leftovers(dir.path()); assert!(leftovers.is_empty(), "tmp cleaned up: {leftovers:?}"); } } diff --git a/crates/oxide-code/src/util/path.rs b/crates/oxide-code/src/util/path.rs index c169acc7..59d04d34 100644 --- a/crates/oxide-code/src/util/path.rs +++ b/crates/oxide-code/src/util/path.rs @@ -103,8 +103,6 @@ pub(crate) mod testing { #[cfg(test)] mod tests { - use std::path::PathBuf; - use super::testing::{HOME_VAR, abs}; use super::*; @@ -208,11 +206,14 @@ mod tests { // `~//foo` and `~///foo` should land at $HOME/foo, not at /foo. PathBuf::join replaces // the receiver when the argument is absolute, so the tail must be trimmed first. temp_env::with_var(HOME_VAR, Some("/tmp/oxide-fake-home"), || { - for raw in ["~//foo", "~///foo/bar"] { - let got = expand_user(raw).unwrap(); - assert!( - got.starts_with("/tmp/oxide-fake-home"), - "{raw:?} -> {got:?}", + for (raw, expected) in [ + ("~//foo", "/tmp/oxide-fake-home/foo"), + ("~///foo/bar", "/tmp/oxide-fake-home/foo/bar"), + ] { + assert_eq!( + expand_user(raw).unwrap(), + PathBuf::from(expected), + "{raw:?}" ); } }); @@ -244,11 +245,6 @@ mod tests { ); } - // The `expand_user` home-unset error branch is unreachable from a Linux unit test: - // `std::env::home_dir()` falls back to `getpwuid_r` when `$HOME` is empty, so the `None` - // arm only fires in exotic environments (no passwd entry, Windows without a profile). - // The `Result` signature is still the right contract for those cases. - // ── lexical_normalize ── #[test] diff --git a/crates/oxide-code/src/util/sync.rs b/crates/oxide-code/src/util/sync.rs new file mode 100644 index 00000000..e2c4d959 --- /dev/null +++ b/crates/oxide-code/src/util/sync.rs @@ -0,0 +1,37 @@ +//! Mutex recovery for shared application state. + +use std::sync::{Mutex, MutexGuard}; + +use tracing::warn; + +pub(crate) fn lock_or_recover<'a, T>(mutex: &'a Mutex, label: &str) -> MutexGuard<'a, T> { + mutex.lock().unwrap_or_else(|error| { + warn!("{label} mutex poisoned, recovering"); + error.into_inner() + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + // ── lock_or_recover ── + + #[test] + fn lock_or_recover_preserves_poisoned_state_and_allows_mutation() { + let mutex = Mutex::new(vec![1]); + std::thread::scope(|scope| { + let result = scope + .spawn(|| { + let mut state = mutex.lock().unwrap(); + state.push(2); + panic!("deliberate poison"); + }) + .join(); + assert!(result.is_err()); + }); + assert!(mutex.is_poisoned()); + lock_or_recover(&mutex, "test").push(3); + assert_eq!(*lock_or_recover(&mutex, "test"), [1, 2, 3]); + } +} diff --git a/crates/oxide-code/src/util/text.rs b/crates/oxide-code/src/util/text.rs index 8cfb3f40..809c3399 100644 --- a/crates/oxide-code/src/util/text.rs +++ b/crates/oxide-code/src/util/text.rs @@ -1,12 +1,27 @@ -//! Display-width-aware text utilities. +//! Text helpers: display-width-aware truncation and count-based pluralization. use unicode_segmentation::UnicodeSegmentation; use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; pub(crate) const ELLIPSIS: &str = "..."; - pub(crate) const ELLIPSIS_WIDTH: usize = 3; +#[expect( + clippy::needless_pass_by_value, + reason = "callers pass integer counts directly" +)] +pub(crate) fn plural<'a, N: PartialEq + From>( + count: N, + singular: &'a str, + plural: &'a str, +) -> &'a str { + if count == N::from(1) { + singular + } else { + plural + } +} + /// Truncates `s` to `max_width` display columns, appending [`ELLIPSIS`] when shortened. /// /// Width is measured in terminal columns (CJK and emoji take 2), and cuts fall between grapheme @@ -97,6 +112,15 @@ pub(crate) fn escape_controls(text: &str) -> String { mod tests { use super::*; + // ── plural ── + + #[test] + fn plural_uses_singular_only_for_one() { + for (count, expected) in [(0, "files"), (1, "file"), (2, "files")] { + assert_eq!(plural(count, "file", "files"), expected); + } + } + // ── truncate_to_width ── #[test] diff --git a/crates/oxide-code/src/util/time.rs b/crates/oxide-code/src/util/time.rs index 731e4cd4..03796f06 100644 --- a/crates/oxide-code/src/util/time.rs +++ b/crates/oxide-code/src/util/time.rs @@ -16,8 +16,6 @@ pub(crate) fn init_local_offset() { _ = LOCAL_OFFSET.set(resolve_offset(UtcOffset::current_local_offset())); } -/// Pure fallback for the offset lookup — split from `init_local_offset` so the warn-and-fallback -/// arm can be tested without depending on the host's TZ database state. fn resolve_offset(result: Result) -> UtcOffset { result.unwrap_or_else(|e| { warn!("cannot read local timezone offset, falling back to UTC: {e}"); @@ -35,23 +33,6 @@ pub(crate) fn local_offset() -> UtcOffset { mod tests { use super::*; - // ── init_local_offset / local_offset ── - - #[test] - fn init_is_idempotent_and_local_offset_returns_a_stable_cached_value() { - // OnceLock is process-global, so we can only assert the contract that holds regardless of - // OnceLock state: init must not panic when called more than once, and `local_offset` must - // return the same value across repeat reads. - init_local_offset(); - init_local_offset(); - let first = local_offset(); - let second = local_offset(); - assert_eq!( - first, second, - "local_offset must return a stable cached value across reads", - ); - } - // ── resolve_offset ── #[test] diff --git a/crates/oxide-code/src/util/tls.rs b/crates/oxide-code/src/util/tls.rs index 67fcaf01..ba7e285c 100644 --- a/crates/oxide-code/src/util/tls.rs +++ b/crates/oxide-code/src/util/tls.rs @@ -4,10 +4,10 @@ use std::path::Path; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use reqwest::{Certificate, ClientBuilder}; -/// Merges the PEM bundle at `path` into `builder`'s trust store. `None` returns `builder` unchanged. +/// Merges the PEM bundle into `builder`'s trust store. `None` returns `builder` unchanged. pub(crate) fn apply_extra_ca_certs( builder: ClientBuilder, path: Option<&Path>, @@ -27,7 +27,7 @@ pub(crate) fn load_extra_ca_certs(path: &Path) -> Result> { let certs = Certificate::from_pem_bundle(&bytes) .with_context(|| format!("failed to parse PEM bundle at {}", path.display()))?; if certs.is_empty() { - anyhow::bail!( + bail!( "no PEM certificates found in {} (expected one or more `-----BEGIN CERTIFICATE-----` blocks)", path.display(), ); @@ -69,17 +69,6 @@ mod tests { // ── apply_extra_ca_certs ── - #[test] - fn apply_extra_ca_certs_is_a_noop_for_none() { - // Builder must survive the no-op path; `reqwest::Client::builder().build()` confirms - // the returned builder is still valid. - let builder = reqwest::Client::builder(); - let builder = apply_extra_ca_certs(builder, None).expect("None must not error"); - builder - .build() - .expect("None path must produce a buildable client"); - } - #[test] fn apply_extra_ca_certs_surfaces_loader_error_with_path() { let missing = Path::new("/definitely/does/not/exist.pem"); @@ -124,15 +113,6 @@ mod tests { assert!(msg.contains("no PEM certificates found"), "{msg}"); } - #[test] - fn load_extra_ca_certs_reports_filename_on_read_failure() { - let missing = Path::new("/definitely/does/not/exist.pem"); - let err = load_extra_ca_certs(missing).expect_err("missing path must error"); - let msg = format!("{err:#}"); - assert!(msg.contains("failed to read extra CA bundle"), "{msg}"); - assert!(msg.contains("/definitely/does/not/exist.pem"), "{msg}"); - } - #[test] fn load_extra_ca_certs_reports_filename_on_parse_failure() { let malformed = write_pem(indoc::indoc! {" diff --git a/docs/design/architecture.md b/docs/design/architecture.md index 2d5a16f4..6a75f9e3 100644 --- a/docs/design/architecture.md +++ b/docs/design/architecture.md @@ -14,7 +14,7 @@ Layers run from the foundation to the frontends. | ---------- | -------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | | Foundation | `file_tracker` | Per-session Read-before-Edit gate with xxh64 staleness detection | | Foundation | `message` | Conversation message and content-block types shared by the client, the session transcript, and the TUI | -| Foundation | `util` | Shared helpers for environment variables, the filesystem, git probes, logging, paths, text width, local time, and TLS | +| Foundation | `util` | Shared helpers for environment variables, the filesystem, git probes, logging, paths, mutex recovery, text width, local time, and TLS | | Domain | `config` | Layered configuration with per-value provenance, credentials, gateway settings, and stored OAuth logins with browser sign-in | | Domain | `model` | Supported-model catalogue, per-model request capabilities, and pricing | | Domain | `permission` | Tool permission gate: modes, allow / ask / deny rules, and path resolution | From f0ccb5cdc4fed4f9d825c25125eebd7ad78b306a Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 14:25:34 +0800 Subject: [PATCH 3/5] refactor(tool): clarify write modes and share search helpers --- crates/oxide-code/src/tool.rs | 72 +++++++++----- crates/oxide-code/src/tool/bash.rs | 71 ++++++-------- crates/oxide-code/src/tool/edit.rs | 146 +++++++++++----------------- crates/oxide-code/src/tool/glob.rs | 61 ++++++------ crates/oxide-code/src/tool/grep.rs | 95 +++++++++--------- crates/oxide-code/src/tool/read.rs | 38 ++++---- crates/oxide-code/src/tool/write.rs | 50 +++++----- 7 files changed, 255 insertions(+), 278 deletions(-) diff --git a/crates/oxide-code/src/tool.rs b/crates/oxide-code/src/tool.rs index edbf5a7e..f0bcd2cf 100644 --- a/crates/oxide-code/src/tool.rs +++ b/crates/oxide-code/src/tool.rs @@ -23,7 +23,7 @@ use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _}; use crate::file_tracker::{FileTracker, TrackableFileError, validate_trackable_file}; use crate::permission::{GateTarget, PermissionClass}; use crate::util::path::{lexical_normalize, slash_separated}; -use crate::util::text::ELLIPSIS; +use crate::util::text::{ELLIPSIS, plural}; // ── Tool Definition ── @@ -236,8 +236,7 @@ pub(crate) trait Tool: Send + Sync { /// Pulls the tool's primary argument out of `input` for terse `Name(arg)` labels in the UI. /// Returning `None` falls back to the bare name. - fn summarize_input<'a>(&self, input: &'a serde_json::Value) -> Option<&'a str> { - _ = input; + fn summarize_input<'a>(&self, _input: &'a serde_json::Value) -> Option<&'a str> { None } @@ -430,6 +429,11 @@ pub(crate) fn parse_input(raw: serde_json::Value) -> Result // ── Path Utilities ── +fn search_root_target(input: &serde_json::Value, cwd: &Path) -> GateTarget { + let path = extract_input_field(input, "path").unwrap_or("."); + GateTarget::for_path(path, cwd) +} + /// The canonical working directory, so hits under a canonical search base render relative to it. pub(crate) fn working_dir() -> Result { std::env::current_dir() @@ -574,18 +578,22 @@ pub(crate) async fn read_validated( // ── File Writing ── -/// Writes `bytes` to `path` through [`open_no_follow`]. `create_new` refuses any existing entry, a -/// dangling symlink included, and otherwise the file is truncated only once the open succeeds. -pub(crate) async fn write_no_follow( - path: &Path, - bytes: &[u8], - create_new: bool, -) -> std::io::Result<()> { +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum WriteMode { + CreateNew, + Overwrite, +} + +/// Writes `bytes` through [`open_no_follow`], refusing existing entries in `CreateNew` mode and +/// truncating only after a successful open in `Overwrite` mode. +async fn write_no_follow(path: &Path, bytes: &[u8], mode: WriteMode) -> std::io::Result<()> { let mut options = std::fs::OpenOptions::new(); - options.write(true).create_new(create_new); + options + .write(true) + .create_new(matches!(mode, WriteMode::CreateNew)); let mut file = open_no_follow(path, options).await?; - if !create_new { + if matches!(mode, WriteMode::Overwrite) { file.set_len(0).await?; } file.write_all(bytes).await?; @@ -594,16 +602,16 @@ pub(crate) async fn write_no_follow( /// Writes `bytes` like [`write_no_follow`] and records them under the path's [`tracker_key`] as one /// unit, so a caller dropped mid-write cannot leave the file written but unrecorded. -pub(crate) async fn write_tracked( +async fn write_tracked( tracker: &Arc, path: &str, bytes: Vec, - create_new: bool, + mode: WriteMode, ) -> std::io::Result<()> { let path = path.to_owned(); tracker .write_and_record(async move { - write_no_follow(Path::new(&path), &bytes, create_new).await?; + write_no_follow(Path::new(&path), &bytes, mode).await?; Ok((tracker_key(&path).await, bytes)) }) .await @@ -755,7 +763,7 @@ pub(crate) fn push_withheld_note(content: &mut String, count: usize) { if count == 0 { return; } - let word = if count == 1 { "file" } else { "files" }; + let word = plural(count, "file", "files"); _ = write!(content, "\n\n({count} {word} {WITHHELD_NOTE_END}"); } @@ -1124,7 +1132,7 @@ mod tests { } #[test] - fn get_unknown_tool() { + fn get_unknown_tool_has_no_entry() { let registry = ToolRegistry::new(vec![Box::new(BashTool)]); assert!(registry.get("nonexistent").is_none()); } @@ -1663,7 +1671,9 @@ mod tests { let path = dir.path().join("file.txt"); std::fs::write(&path, "a much longer original").unwrap(); - write_no_follow(&path, b"short", false).await.unwrap(); + write_no_follow(&path, b"short", WriteMode::Overwrite) + .await + .unwrap(); assert_eq!(std::fs::read_to_string(&path).unwrap(), "short"); } @@ -1676,9 +1686,17 @@ mod tests { let link = dir.path().join("link.txt"); symlink_file(&target, &link); - assert!(write_no_follow(&existing, b"x", true).await.is_err()); + assert!( + write_no_follow(&existing, b"x", WriteMode::CreateNew) + .await + .is_err() + ); assert_eq!(std::fs::read_to_string(&existing).unwrap(), "original"); - assert!(write_no_follow(&link, b"x", true).await.is_err()); + assert!( + write_no_follow(&link, b"x", WriteMode::CreateNew) + .await + .is_err() + ); assert!(!target.exists(), "the link target must not be created"); } @@ -1690,7 +1708,9 @@ mod tests { let link = dir.path().join("link.txt"); symlink_file(&target, &link); - let err = write_no_follow(&link, b"x", false).await.unwrap_err(); + let err = write_no_follow(&link, b"x", WriteMode::Overwrite) + .await + .unwrap_err(); #[cfg(unix)] assert_eq!(err.raw_os_error(), Some(nix::errno::Errno::ELOOP as i32)); #[cfg(windows)] @@ -1705,7 +1725,7 @@ mod tests { let fifo = dir.path().join("pipe"); nix::unistd::mkfifo(&fifo, nix::sys::stat::Mode::S_IRWXU).unwrap(); - let write = write_no_follow(&fifo, b"x", false); + let write = write_no_follow(&fifo, b"x", WriteMode::Overwrite); let result = tokio::time::timeout(std::time::Duration::from_secs(5), write).await; assert!(result.expect("the open must not block").is_err()); } @@ -1735,7 +1755,9 @@ mod tests { .await .unwrap(); assert_eq!(bytes, content.as_bytes()); - write_no_follow(&path, b"rewritten", false).await.unwrap(); + write_no_follow(&path, b"rewritten", WriteMode::Overwrite) + .await + .unwrap(); assert_eq!(std::fs::read(&path).unwrap(), b"rewritten"); } @@ -1749,7 +1771,7 @@ mod tests { } #[test] - fn truncate_line_long_gets_truncated() { + fn truncate_line_long_text_ends_with_ellipsis() { let long_line = "x".repeat(MAX_CHARS + 100); let result = truncate_line(&long_line, MAX_CHARS); let suffix = format!("... [{} chars]", MAX_CHARS + 100); @@ -1759,7 +1781,7 @@ mod tests { } #[test] - fn truncate_line_multibyte_safe() { + fn truncate_line_multibyte_text_preserves_character_boundaries() { let mut line = "a".repeat(MAX_CHARS - 2); line.push('🦀'); line.push_str(&"b".repeat(100)); diff --git a/crates/oxide-code/src/tool/bash.rs b/crates/oxide-code/src/tool/bash.rs index d0613f4c..90f92c0b 100644 --- a/crates/oxide-code/src/tool/bash.rs +++ b/crates/oxide-code/src/tool/bash.rs @@ -15,7 +15,7 @@ use tokio::io::{AsyncRead, AsyncReadExt}; use tokio::process::{ChildStderr, ChildStdout}; use super::{Tool, ToolOutput, extract_input_field, title_case}; -use crate::permission::PermissionClass; +use crate::permission::{GateTarget, PermissionClass}; const DEFAULT_TIMEOUT: Duration = Duration::from_mins(2); const MAX_TIMEOUT: Duration = Duration::from_mins(10); @@ -45,13 +45,9 @@ impl Tool for BashTool { PermissionClass::Bash } - fn gate_target( - &self, - input: &serde_json::Value, - _cwd: &std::path::Path, - ) -> crate::permission::GateTarget { + fn gate_target(&self, input: &serde_json::Value, _cwd: &Path) -> GateTarget { extract_input_field(input, "command") - .map(|c| crate::permission::GateTarget::Command(c.to_owned())) + .map(|c| GateTarget::Command(c.to_owned())) .unwrap_or_default() } @@ -261,11 +257,13 @@ fn git_bash_on(path: &std::ffi::OsStr) -> Option { fn render_streams(stdout: &PipeCapture, stderr: &PipeCapture) -> String { let (stdout_budget, stderr_budget) = split_budget(stdout.kept(), stderr.kept(), OUTPUT_BUDGET); - let stdout = render_pipe(stdout, stdout_budget, true); - let stderr = render_pipe(stderr, stderr_budget, false); + let stdout = render_pipe(stdout, stdout_budget); + let stdout = stdout.trim_start_matches('\n'); + let stderr = render_pipe(stderr, stderr_budget); + let stderr = stderr.trim_start(); match (stdout.is_empty(), stderr.is_empty()) { - (_, true) => stdout, - (true, false) => stderr, + (_, true) => stdout.to_owned(), + (true, false) => stderr.to_owned(), (false, false) => format!("{stdout}\n{stderr}"), } } @@ -285,7 +283,7 @@ fn split_budget(first: usize, second: usize, budget: usize) -> (usize, usize) { } /// Renders at most `budget` bytes of `pipe`, folding everything it drops into the omission count. -fn render_pipe(pipe: &PipeCapture, budget: usize, trim_start_newlines: bool) -> String { +fn render_pipe(pipe: &PipeCapture, budget: usize) -> String { let tail: Vec = pipe.tail.iter().copied().collect(); let (head_len, tail_len) = split_budget(pipe.head.len(), tail.len(), budget); let mut head = &pipe.head[..head_len]; @@ -314,13 +312,7 @@ fn render_pipe(pipe: &PipeCapture, budget: usize, trim_start_newlines: bool) -> } }; - let text = text.trim_end(); - let text = if trim_start_newlines { - text.trim_start_matches('\n') - } else { - text.trim_start() - }; - text.to_owned() + text.trim_end().to_owned() } // ── Pipe Capture ── @@ -446,16 +438,8 @@ mod tests { use tokio::io::AsyncWriteExt as _; use super::*; - use crate::permission::GateTarget; use crate::tool::MAX_OUTPUT_BYTES; - // ── permission_class ── - - #[test] - fn permission_class_is_bash() { - assert_eq!(BashTool.permission_class(), PermissionClass::Bash); - } - // ── gate_target ── #[test] @@ -476,14 +460,14 @@ mod tests { // ── run ── #[tokio::test] - async fn run_valid_command() { + async fn run_valid_command_executes_successfully() { let output = run(serde_json::json!({"command": "echo hello"})).await; assert!(!output.is_error); assert_eq!(output.content, "hello"); } #[tokio::test] - async fn run_missing_command_field() { + async fn run_missing_command_field_reports_input_error() { let output = run(serde_json::json!({})).await; assert!(output.is_error); assert!(output.content.contains("Invalid input")); @@ -539,14 +523,14 @@ mod tests { } #[tokio::test] - async fn execute_stderr_output() { + async fn execute_stderr_output_preserves_content() { let output = run_default("echo err >&2").await; assert!(!output.is_error); assert_eq!(output.content, "err"); } #[tokio::test] - async fn execute_combined_stdout_and_stderr() { + async fn execute_combined_stdout_and_stderr_renders_both_streams() { let output = run_default("echo out && echo err >&2").await; assert!(!output.is_error); assert_eq!(output.content, "out\nerr"); @@ -560,7 +544,7 @@ mod tests { } #[tokio::test] - async fn execute_output_with_nonzero_exit() { + async fn execute_nonzero_exit_preserves_output() { let output = run_default("echo partial; false").await; assert!(!output.is_error); assert_eq!( @@ -573,7 +557,7 @@ mod tests { } #[tokio::test] - async fn execute_no_output() { + async fn execute_no_output_renders_the_no_output_marker() { let output = run_default("true").await; assert!(!output.is_error); assert_eq!(output.content, "(no output)"); @@ -778,6 +762,13 @@ mod tests { } } + #[test] + fn render_streams_trims_stdout_newlines_and_stderr_whitespace() { + let stdout = capture(b"\n output\n", b"", 0); + let stderr = capture(b"\n error\n", b"", 0); + assert_eq!(render_streams(&stdout, &stderr), " output\nerror"); + } + // ── split_budget ── #[test] @@ -800,9 +791,9 @@ mod tests { #[test] fn render_pipe_marks_the_omitted_middle_between_head_and_tail() { - let pipe = capture(b"\nhello\n", b"world\n", 7); + let pipe = capture(b"hello\n", b"world\n", 7); assert_eq!( - render_pipe(&pipe, OUTPUT_BUDGET, true), + render_pipe(&pipe, OUTPUT_BUDGET), indoc! {" hello @@ -820,7 +811,7 @@ mod tests { 5, ); assert_eq!( - render_pipe(&pipe, OUTPUT_BUDGET, true), + render_pipe(&pipe, OUTPUT_BUDGET), indoc! {" ab ... [10 bytes omitted] ... @@ -832,7 +823,7 @@ mod tests { fn render_pipe_over_budget_drops_the_middle_and_counts_it() { let pipe = capture(b"0123456789", b"abcdef4321", 0); assert_eq!( - render_pipe(&pipe, 8, true), + render_pipe(&pipe, 8), indoc! {" 0123 ... [12 bytes omitted] ... @@ -845,13 +836,13 @@ mod tests { // A multibyte char straddling the head / tail split must not decode as two replacements. let crab = "🦀".as_bytes(); let pipe = capture(&crab[..2], &crab[2..], 0); - assert_eq!(render_pipe(&pipe, OUTPUT_BUDGET, true), "🦀"); + assert_eq!(render_pipe(&pipe, OUTPUT_BUDGET), "🦀"); } #[test] - fn render_pipe_trims_stderr_edges_and_keeps_internal_newlines() { + fn render_pipe_trims_the_end_and_keeps_leading_and_internal_newlines() { let pipe = capture(b"\nerr\nmore\n\n", b"", 0); - assert_eq!(render_pipe(&pipe, OUTPUT_BUDGET, false), "err\nmore"); + assert_eq!(render_pipe(&pipe, OUTPUT_BUDGET), "\nerr\nmore"); } // ── PipeCapture::drain ── diff --git a/crates/oxide-code/src/tool/edit.rs b/crates/oxide-code/src/tool/edit.rs index f95f1b71..741cc06c 100644 --- a/crates/oxide-code/src/tool/edit.rs +++ b/crates/oxide-code/src/tool/edit.rs @@ -9,7 +9,7 @@ use serde::Deserialize; use super::{ ApprovalBody, ApprovalPreview, DiffChunk, DiffLine, Tool, ToolMetadata, ToolOutput, - ToolResultView, extract_input_field, summarize_path_call, + ToolResultView, WriteMode, extract_input_field, summarize_path_call, }; use crate::file_tracker::{FileTracker, GatePurpose}; use crate::permission::PermissionClass; @@ -45,15 +45,6 @@ impl Tool for EditTool { Some("file_path") } - fn approval_preview(&self, input: &serde_json::Value) -> ApprovalPreview { - let old = extract_input_field(input, "old_string").unwrap_or_default(); - let new = extract_input_field(input, "new_string").unwrap_or_default(); - ApprovalPreview { - title: self.summarize_call(input), - body: ApprovalBody::Diff(vec![synthesize_chunk(old, new)]), - } - } - fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", @@ -119,6 +110,15 @@ impl Tool for EditTool { }) } + fn approval_preview(&self, input: &serde_json::Value) -> ApprovalPreview { + let old = extract_input_field(input, "old_string").unwrap_or_default(); + let new = extract_input_field(input, "new_string").unwrap_or_default(); + ApprovalPreview { + title: self.summarize_call(input), + body: ApprovalBody::Diff(vec![synthesize_chunk(old, new)]), + } + } + fn run( &self, input: serde_json::Value, @@ -172,7 +172,7 @@ async fn run(raw: serde_json::Value, tracker: Arc) -> ToolOutput { /// - `old_string` must occur verbatim in the file (whitespace and indentation included). Without /// `replace_all`, ambiguous matches are rejected so the model cannot silently edit the wrong /// site. -/// - The Read-before-Edit gate refuses files not fully read in this session and files whose +/// - The Read-before-Edit gate refuses files with no Read of their current content and files whose /// content changed since that Read. /// - CRLF is preserved: the file is normalized to LF for matching, the dominant EOL is restored /// on write so we don't flip line endings just because the caller used `\n` in `old_string`. @@ -240,7 +240,7 @@ async fn edit_file( }; let updated = apply_eol(updated, eol); - super::write_tracked(tracker, path, updated.into_bytes(), false) + super::write_tracked(tracker, path, updated.into_bytes(), WriteMode::Overwrite) .await .map_err(|e| format!("Failed to write {path}: {e}"))?; @@ -395,49 +395,8 @@ mod tests { }; use crate::tool::testing::canonical_tempdir; - // ── permission_class ── - - #[test] - fn permission_class_is_edit() { - assert_eq!( - EditTool::new(tracker()).permission_class(), - PermissionClass::Edit - ); - } - - // ── approval_preview ── - - #[test] - fn approval_preview_renders_old_and_new_as_a_diff() { - let tool = EditTool::new(Arc::new(FileTracker::default())); - let preview = tool.approval_preview(&serde_json::json!({ - "file_path": "src/main.rs", - "old_string": "let a = 1;", - "new_string": "let a = 2;", - })); - - assert!(preview.title.contains("src/main.rs")); - let ApprovalBody::Diff(chunks) = preview.body else { - panic!("edit preview should be a diff"); - }; - let chunk = &chunks[0]; - assert!(chunk.old.iter().any(|l| l.text.contains("let a = 1;"))); - assert!(chunk.new.iter().any(|l| l.text.contains("let a = 2;"))); - } - // ── result_view ── - #[test] - fn result_view_is_none_when_new_string_missing_with_old_string_present() { - let tool = EditTool::new(Arc::new(FileTracker::default())); - let view = tool.result_view( - &serde_json::json!({"old_string": "x"}), - "Successfully edited /tmp/x.", - &ToolMetadata::default(), - ); - assert!(view.is_none()); - } - #[test] fn result_view_prefers_structured_chunks_from_metadata_on_live_path() { let input = serde_json::json!({ @@ -584,43 +543,52 @@ mod tests { } #[test] - fn result_view_is_none_when_required_inputs_missing() { - let input = serde_json::json!({"file_path": "/tmp/x"}); - assert!( - EditTool::new(tracker()) - .result_view(&input, "edited", &ToolMetadata::default()) - .is_none(), - ); + fn result_view_missing_or_mistyped_required_inputs_uses_plain_output() { + for input in [ + serde_json::json!({"old_string": "x"}), + serde_json::json!({"file_path": "/tmp/x"}), + serde_json::json!({"file_path": "/tmp/x", "old_string": 42, "new_string": "b"}), + serde_json::json!({"file_path": "/tmp/x", "old_string": "a", "new_string": 42}), + ] { + assert_eq!( + EditTool::new(tracker()).result_view(&input, "edited", &ToolMetadata::default()), + None, + "{input}", + ); + } } + // ── approval_preview ── + #[test] - fn result_view_is_none_when_field_type_is_wrong() { - let bad_old = serde_json::json!({ - "file_path": "/tmp/x", - "old_string": 42, - "new_string": "b", - }); - assert!( - EditTool::new(tracker()) - .result_view(&bad_old, "edited", &ToolMetadata::default()) - .is_none(), - ); - let bad_new = serde_json::json!({ - "file_path": "/tmp/x", - "old_string": "a", - "new_string": 42, - }); - assert!( - EditTool::new(tracker()) - .result_view(&bad_new, "edited", &ToolMetadata::default()) - .is_none(), + fn approval_preview_renders_old_and_new_as_a_diff() { + let tool = EditTool::new(tracker()); + let preview = tool.approval_preview(&serde_json::json!({ + "file_path": "src/main.rs", + "old_string": "let a = 1;", + "new_string": "let a = 2;", + })); + + assert!(preview.title.contains("src/main.rs")); + assert_eq!( + preview.body, + ApprovalBody::Diff(vec![DiffChunk { + old: vec![DiffLine { + number: 1, + text: "let a = 1;".to_owned(), + }], + new: vec![DiffLine { + number: 1, + text: "let a = 2;".to_owned(), + }], + }]), ); } // ── run ── #[tokio::test] - async fn run_valid_edit() { + async fn run_valid_edit_updates_file() { let dir = canonical_tempdir(); let path = dir.path().join("test.txt"); std::fs::write(&path, "hello world").unwrap(); @@ -643,7 +611,7 @@ mod tests { } #[tokio::test] - async fn run_missing_required_fields() { + async fn run_missing_required_fields_reports_input_error() { let output = run( serde_json::json!({ "file_path": "/tmp/x", @@ -715,7 +683,7 @@ mod tests { } #[tokio::test] - async fn edit_file_replace_all() { + async fn edit_file_replace_all_updates_every_match() { let dir = canonical_tempdir(); let path = dir.path().join("test.txt"); std::fs::write(&path, "aaa bbb aaa").unwrap(); @@ -740,7 +708,7 @@ mod tests { } #[tokio::test] - async fn edit_file_replace_all_single_match() { + async fn edit_file_replace_all_single_match_updates_content() { let dir = canonical_tempdir(); let path = dir.path().join("test.txt"); std::fs::write(&path, "hello world").unwrap(); @@ -1513,7 +1481,7 @@ mod tests { } #[test] - fn common_boundaries_empty_inputs_return_zero() { + fn common_boundaries_empty_inputs_have_zero_boundaries() { let empty: [&str; 0] = []; assert_eq!(common_boundaries(&empty, &empty), (0, 0)); } @@ -1527,12 +1495,12 @@ mod tests { // ── dominant_eol ── #[test] - fn dominant_eol_lf_only() { + fn dominant_eol_lf_only_selects_lf() { assert_eq!(dominant_eol("a\nb\n"), "\n"); } #[test] - fn dominant_eol_crlf_only() { + fn dominant_eol_crlf_only_selects_crlf() { assert_eq!(dominant_eol("a\r\nb\r\n"), "\r\n"); } @@ -1548,7 +1516,7 @@ mod tests { } #[test] - fn dominant_eol_no_newlines() { + fn dominant_eol_no_newlines_defaults_to_lf() { assert_eq!(dominant_eol("no newlines"), "\n"); } @@ -1576,7 +1544,7 @@ mod tests { } #[test] - fn apply_eol_lf_unchanged() { + fn apply_eol_lf_preserves_content() { assert_eq!(apply_eol("a\nb\n".into(), "\n"), "a\nb\n"); } diff --git a/crates/oxide-code/src/tool/glob.rs b/crates/oxide-code/src/tool/glob.rs index e04a75ec..e8b5298b 100644 --- a/crates/oxide-code/src/tool/glob.rs +++ b/crates/oxide-code/src/tool/glob.rs @@ -8,7 +8,11 @@ use std::pin::Pin; use serde::Deserialize; use super::{Tool, ToolMetadata, ToolOutput, ToolResultView, extract_input_field}; -use crate::permission::{PermissionClass, ReadDeny}; +use crate::permission::{GateTarget, PermissionClass, ReadDeny}; +use crate::util::text::plural; + +const NO_FILES_FOUND: &str = "No files found"; +const SHOWING_PREFIX: &str = "(Showing "; const MAX_RESULTS: usize = 100; @@ -37,15 +41,8 @@ impl Tool for GlobTool { PermissionClass::Read } - fn gate_target( - &self, - input: &serde_json::Value, - cwd: &std::path::Path, - ) -> crate::permission::GateTarget { - // The gated target is the search root (the `path` arg, defaulting to cwd), so a path-scoped - // deny can block listing under a protected directory. - let path = extract_input_field(input, "path").unwrap_or("."); - crate::permission::GateTarget::for_path(path, cwd) + fn gate_target(&self, input: &serde_json::Value, cwd: &Path) -> GateTarget { + super::search_root_target(input, cwd) } fn input_schema(&self) -> serde_json::Value { @@ -131,12 +128,12 @@ async fn run(raw: serde_json::Value, read_deny: ReadDeny) -> ToolOutput { } fn glob_title(output: &str) -> String { - if output == "No files found" { + if output == NO_FILES_FOUND { return output.to_owned(); } let body = glob_output_body(output).unwrap_or(output.trim_end()); let count = body.lines().filter(|l| !l.trim().is_empty()).count(); - let word = if count == 1 { "file" } else { "files" }; + let word = plural(count, "file", "files"); format!("Found {count} {word}") } @@ -190,7 +187,7 @@ fn glob_files( matches.sort_by_key(|entry| std::cmp::Reverse(entry.1)); if matches.is_empty() { - let mut content = "No files found".to_owned(); + let mut content = NO_FILES_FOUND.to_owned(); super::push_withheld_note(&mut content, withheld); return Ok(GlobOutput { content, @@ -202,18 +199,16 @@ fn glob_files( let truncated = total > MAX_RESULTS; matches.truncate(MAX_RESULTS); - let mut content = String::new(); - for (i, (p, _)) in matches.iter().enumerate() { - if i > 0 { - content.push('\n'); - } - content.push_str(p); - } + let mut content = matches + .iter() + .map(|(path, _)| path.as_str()) + .collect::>() + .join("\n"); if truncated { _ = write!( content, - "\n\n(Showing {MAX_RESULTS} of {total} matches. Use a more specific pattern.)" + "\n\n{SHOWING_PREFIX}{MAX_RESULTS} of {total} matches. Use a more specific pattern.)" ); } super::push_withheld_note(&mut content, withheld); @@ -255,7 +250,7 @@ fn build_files_view( ) -> Option { let pattern = extract_input_field(input, "pattern")?.to_owned(); let trimmed = content.trim_end(); - if trimmed == "No files found" { + if trimmed == NO_FILES_FOUND { return Some(ToolResultView::GlobFiles { pattern, files: Vec::new(), @@ -285,7 +280,7 @@ fn build_files_view( } fn is_truncation_footer(footer: &str) -> bool { - footer.starts_with("(Showing ") + footer.starts_with(SHOWING_PREFIX) } fn glob_output_body(content: &str) -> Option<&str> { @@ -324,12 +319,12 @@ mod tests { #[test] fn gate_target_defaults_the_search_root_to_cwd() { - // With no `path`, the search root is cwd, so a deny scoped to the project root still applies. + // A deny scoped to the project root must apply when `path` is omitted. let dir = tempfile::tempdir().unwrap(); let cwd = dunce::canonicalize(dir.path()).unwrap(); let target = GlobTool::default().gate_target(&serde_json::json!({"pattern": "*.rs"}), &cwd); - let crate::permission::GateTarget::Path { relative, .. } = target else { + let GateTarget::Path { relative, .. } = target else { panic!("expected a path target"); }; assert_eq!(relative.as_deref(), Some("")); @@ -338,7 +333,7 @@ mod tests { // ── run ── #[tokio::test] - async fn run_finds_files() { + async fn run_matching_pattern_reports_files() { let dir = canonical_tempdir(); std::fs::write(dir.path().join("a.txt"), "").unwrap(); std::fs::write(dir.path().join("b.rs"), "").unwrap(); @@ -357,7 +352,7 @@ mod tests { } #[tokio::test] - async fn run_missing_pattern() { + async fn run_missing_pattern_reports_input_error() { let output = GlobTool::default().run(serde_json::json!({})).await; assert!(output.is_error); assert!(output.content.contains("Invalid input")); @@ -418,7 +413,7 @@ mod tests { // ── glob_files ── #[test] - fn glob_files_basic() { + fn glob_files_plain_pattern_finds_matching_paths() { let dir = canonical_tempdir(); std::fs::write(dir.path().join("foo.txt"), "").unwrap(); std::fs::write(dir.path().join("bar.txt"), "").unwrap(); @@ -432,7 +427,7 @@ mod tests { } #[test] - fn glob_files_recursive() { + fn glob_files_recursive_pattern_finds_nested_paths() { let dir = canonical_tempdir(); let sub = dir.path().join("sub"); std::fs::create_dir(&sub).unwrap(); @@ -504,7 +499,7 @@ mod tests { } #[test] - fn glob_files_sorted_by_mtime() { + fn glob_files_modification_time_orders_newest_first() { let dir = canonical_tempdir(); std::fs::write(dir.path().join("old.txt"), "old").unwrap(); @@ -516,7 +511,7 @@ mod tests { } #[test] - fn glob_files_no_matches() { + fn glob_files_unmatched_pattern_reports_no_files() { let dir = canonical_tempdir(); std::fs::write(dir.path().join("a.txt"), "").unwrap(); @@ -526,7 +521,7 @@ mod tests { } #[test] - fn glob_files_invalid_pattern() { + fn glob_files_invalid_pattern_reports_error() { let dir = canonical_tempdir(); let err = glob_files("[invalid", None, dir.path(), &ReadDeny::default()).unwrap_err(); assert!(err.contains("Invalid glob pattern")); @@ -546,7 +541,7 @@ mod tests { } #[test] - fn glob_files_invalid_directory() { + fn glob_files_invalid_directory_reports_error() { let dir = canonical_tempdir(); let err = glob_files("*.txt", Some("missing"), dir.path(), &ReadDeny::default()).unwrap_err(); diff --git a/crates/oxide-code/src/tool/grep.rs b/crates/oxide-code/src/tool/grep.rs index cbaa66f3..5cec6f46 100644 --- a/crates/oxide-code/src/tool/grep.rs +++ b/crates/oxide-code/src/tool/grep.rs @@ -12,11 +12,17 @@ use super::{ GrepFileGroup, GrepMatchLine, Tool, ToolMetadata, ToolOutput, ToolResultView, extract_input_field, }; -use crate::permission::{PermissionClass, ReadDeny, WalkDeny}; +use crate::permission::{GateTarget, PermissionClass, ReadDeny, WalkDeny}; +use crate::util::text::plural; + +const NO_MATCHES_FOUND: &str = "No matches found"; +const NO_FILES_FOUND: &str = "No files found"; +const RESULTS_LIMIT_PREFIX: &str = "(Results limited to "; const DEFAULT_HEAD_LIMIT: usize = 250; const MAX_LINE_CHARS: usize = 500; const MAX_GREP_FILE_SIZE: u64 = 1024 * 1024; +const REGEX_SIZE_LIMIT: usize = 1 << 20; /// Withholds each file a `read` deny rule matches from the results. #[cfg_attr(test, derive(Default))] @@ -43,15 +49,8 @@ impl Tool for GrepTool { PermissionClass::Read } - fn gate_target( - &self, - input: &serde_json::Value, - cwd: &std::path::Path, - ) -> crate::permission::GateTarget { - // The gated target is the search root (the `path` arg, defaulting to cwd), so a path-scoped - // deny can block searching under a protected directory. - let path = extract_input_field(input, "path").unwrap_or("."); - crate::permission::GateTarget::for_path(path, cwd) + fn gate_target(&self, input: &serde_json::Value, cwd: &Path) -> GateTarget { + super::search_root_target(input, cwd) } fn input_schema(&self) -> serde_json::Value { @@ -198,8 +197,8 @@ async fn run(raw: serde_json::Value, read_deny: ReadDeny) -> ToolOutput { } fn grep_title(output: &str) -> String { - if matches!(output, "No matches found" | "No files found") { - return "No matches found".into(); + if matches!(output, NO_MATCHES_FOUND | NO_FILES_FOUND) { + return NO_MATCHES_FOUND.into(); } // Count / files-with-matches modes lead with "Found N ...". if let Some(line) = output.lines().find(|l| l.starts_with("Found ")) { @@ -210,7 +209,7 @@ fn grep_title(output: &str) -> String { .filter_map(parse_match_line) .filter(|(_, m)| m.is_match) .count(); - let word = if match_count == 1 { "match" } else { "matches" }; + let word = plural(match_count, "match", "matches"); format!("{match_count} {word}") } @@ -236,8 +235,8 @@ fn grep_files(params: &GrepParams<'_>) -> Result { // size caps keep adversarial patterns (e.g., catastrophic alternations) from OOMing. let re = regex::RegexBuilder::new(params.pattern) .case_insensitive(params.case_insensitive) - .size_limit(1 << 20) - .dfa_size_limit(1 << 20) + .size_limit(REGEX_SIZE_LIMIT) + .dfa_size_limit(REGEX_SIZE_LIMIT) .build() .map_err(|e| format!("Invalid regex: {e}"))?; @@ -409,7 +408,7 @@ fn format_content( } if output_lines.is_empty() { - return "No matches found".into(); + return NO_MATCHES_FOUND.into(); } let truncated = output_lines.len() > head_limit; @@ -417,7 +416,7 @@ fn format_content( let mut output = output_lines.join("\n"); if truncated { - _ = write!(output, "\n\n(Results limited to {head_limit} lines)"); + _ = write!(output, "\n\n{RESULTS_LIMIT_PREFIX}{head_limit} lines)"); } output @@ -521,13 +520,13 @@ fn format_files_with_matches( } if matching_files.is_empty() { - return "No files found".into(); + return NO_FILES_FOUND.into(); } let truncated = matching_files.len() > head_limit; matching_files.truncate(head_limit); let num_files = matching_files.len(); - let file_word = if num_files == 1 { "file" } else { "files" }; + let file_word = plural(num_files, "file", "files"); let mut output = formatdoc! {" Found {num_files} {file_word} @@ -536,7 +535,7 @@ fn format_files_with_matches( }; if truncated { - _ = write!(output, "\n\n(Results limited to {head_limit} files)"); + _ = write!(output, "\n\n{RESULTS_LIMIT_PREFIX}{head_limit} files)"); } output @@ -561,7 +560,7 @@ fn format_count(files: &[PathBuf], cwd: &Path, re: ®ex::Regex, head_limit: us } if counts.is_empty() { - return "No matches found".into(); + return NO_MATCHES_FOUND.into(); } let total_files = counts.len(); @@ -570,18 +569,14 @@ fn format_count(files: &[PathBuf], cwd: &Path, re: ®ex::Regex, head_limit: us let mut output = format!( "Found {total_matches} total {} across {total_files} {}", - if total_matches == 1 { - "occurrence" - } else { - "occurrences" - }, - if total_files == 1 { "file" } else { "files" }, + plural(total_matches, "occurrence", "occurrences"), + plural(total_files, "file", "files"), ); for (p, c) in &counts { _ = write!(output, "\n{p}:{c}"); } if truncated { - _ = write!(output, "\n\n(Results limited to {head_limit} files)"); + _ = write!(output, "\n\n{RESULTS_LIMIT_PREFIX}{head_limit} files)"); } output @@ -596,7 +591,7 @@ fn parse_content_view(content: &str) -> Option { } let trimmed = content.trim_end(); - if trimmed == "No matches found" { + if trimmed == NO_MATCHES_FOUND { return Some(ToolResultView::GrepMatches { groups: Vec::new(), truncated: false, @@ -610,7 +605,7 @@ fn parse_content_view(content: &str) -> Option { if line.is_empty() || line == "--" { continue; } - if line.starts_with("(Results limited to ") && line.ends_with(')') { + if line.starts_with(RESULTS_LIMIT_PREFIX) && line.ends_with(')') { truncated = true; continue; } @@ -694,12 +689,12 @@ mod tests { #[test] fn gate_target_defaults_the_search_root_to_cwd() { - // With no `path`, the search root is cwd, so a deny scoped to the project root still applies. + // A deny scoped to the project root must apply when `path` is omitted. let dir = tempfile::tempdir().unwrap(); let cwd = dunce::canonicalize(dir.path()).unwrap(); let target = GrepTool::default().gate_target(&serde_json::json!({"pattern": "x"}), &cwd); - let crate::permission::GateTarget::Path { relative, .. } = target else { + let GateTarget::Path { relative, .. } = target else { panic!("expected a path target"); }; assert_eq!(relative.as_deref(), Some("")); @@ -708,7 +703,7 @@ mod tests { // ── run ── #[tokio::test] - async fn run_finds_pattern() { + async fn run_matching_pattern_reports_matches() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("test.rs"), @@ -731,7 +726,7 @@ mod tests { } #[tokio::test] - async fn run_missing_pattern() { + async fn run_missing_pattern_reports_input_error() { let output = GrepTool::default().run(serde_json::json!({})).await; assert!(output.is_error); assert!(output.content.contains("Invalid input")); @@ -797,7 +792,7 @@ mod tests { // ── grep_files (content mode) ── #[test] - fn grep_files_basic() { + fn grep_files_plain_pattern_finds_matching_lines() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("a.rs"), @@ -814,7 +809,7 @@ mod tests { } #[test] - fn grep_files_regex() { + fn grep_files_regex_finds_matching_lines() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("test.txt"), @@ -831,7 +826,7 @@ mod tests { } #[test] - fn grep_files_case_insensitive() { + fn grep_files_case_insensitive_matches_mixed_case() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("test.txt"), @@ -850,7 +845,7 @@ mod tests { } #[test] - fn grep_files_with_context() { + fn grep_files_with_context_includes_surrounding_lines() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("test.txt"), @@ -998,7 +993,7 @@ mod tests { } #[test] - fn grep_files_single_file() { + fn grep_files_single_file_finds_matching_lines() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("test.txt"), @@ -1154,7 +1149,7 @@ mod tests { } #[test] - fn grep_files_no_matches() { + fn grep_files_unmatched_pattern_reports_no_matches() { let dir = canonical_tempdir(); std::fs::write(dir.path().join("test.txt"), "hello world\n").unwrap(); @@ -1163,13 +1158,13 @@ mod tests { } #[test] - fn grep_files_invalid_regex() { + fn grep_files_invalid_regex_reports_error() { let err = grep_files(¶ms("[invalid", Path::new("/"))).unwrap_err(); assert!(err.contains("Invalid regex")); } #[test] - fn grep_files_nonexistent_path() { + fn grep_files_nonexistent_path_reports_error() { let dir = canonical_tempdir(); let mut p = params("test", dir.path()); p.search_path = Some("missing"); @@ -1203,7 +1198,7 @@ mod tests { // ── grep_files (head_limit) ── #[test] - fn grep_files_head_limit() { + fn grep_files_head_limit_truncates_matching_lines() { let dir = canonical_tempdir(); let content = (0..20).fold(String::new(), |mut s, i| { _ = writeln!(s, "match line {i}"); @@ -1272,7 +1267,7 @@ mod tests { // ── grep_files (files_with_matches mode) ── #[test] - fn grep_files_files_with_matches_mode() { + fn grep_files_files_with_matches_mode_lists_matching_paths() { let dir = canonical_tempdir(); std::fs::write(dir.path().join("a.rs"), "fn foo() {}\n").unwrap(); std::fs::write(dir.path().join("b.rs"), "fn bar() {}\n").unwrap(); @@ -1336,7 +1331,7 @@ mod tests { // ── grep_files (count mode) ── #[test] - fn grep_files_count_mode() { + fn grep_files_count_mode_reports_occurrences() { let dir = canonical_tempdir(); std::fs::write( dir.path().join("test.txt"), @@ -1403,6 +1398,16 @@ mod tests { ); } + #[test] + fn grep_files_count_mode_without_matches_reports_no_matches() { + let dir = canonical_tempdir(); + std::fs::write(dir.path().join("test.txt"), "other\n").unwrap(); + + let mut p = params("absent", dir.path()); + p.output_mode = OutputMode::Count; + assert_eq!(grep_files(&p).unwrap(), "No matches found"); + } + // ── result_view ── #[test] diff --git a/crates/oxide-code/src/tool/read.rs b/crates/oxide-code/src/tool/read.rs index 14a15040..518659cc 100644 --- a/crates/oxide-code/src/tool/read.rs +++ b/crates/oxide-code/src/tool/read.rs @@ -159,7 +159,7 @@ async fn read_file( let total_lines = lines.len(); if total_lines == 0 { let outcome = tracker.record_read(&key, &bytes, LastView::Full); - return Ok(stub_or(outcome, EMPTY_FILE_MARKER).into_owned()); + return Ok(stub_or(outcome, EMPTY_FILE_MARKER).to_owned()); } let offset_n = offset.unwrap_or(1).max(1); @@ -201,13 +201,13 @@ async fn read_file( let view = LastView::lines(offset_n, last_shown, total_lines); let outcome = tracker.record_read(&key, &bytes, view); - Ok(stub_or(outcome, &output).into_owned()) + Ok(stub_or(outcome, &output).to_owned()) } -fn stub_or(outcome: RecordRead, body: &str) -> std::borrow::Cow<'_, str> { +fn stub_or(outcome: RecordRead, body: &str) -> &str { match outcome { - RecordRead::CacheHit => std::borrow::Cow::Borrowed(CACHE_HIT_STUB), - RecordRead::Inserted => std::borrow::Cow::Borrowed(body), + RecordRead::CacheHit => CACHE_HIT_STUB, + RecordRead::Inserted => body, } } @@ -223,10 +223,10 @@ fn read_excerpt_view(path: String, content: &str) -> Option { } let (body, footer) = split_read_footer(content); - let mut lines = Vec::new(); - for line in body.lines() { - lines.push(parse_read_line(line)?); - } + let lines = body + .lines() + .map(parse_read_line) + .collect::>>()?; let total_lines = footer .and_then(parse_total_lines_footer) @@ -274,7 +274,7 @@ mod tests { // ── run ── #[tokio::test] - async fn run_reads_file() { + async fn run_existing_file_renders_content() { let dir = canonical_tempdir(); let path = dir.path().join("test.txt"); std::fs::write(&path, "hello\nworld\n").unwrap(); @@ -359,7 +359,7 @@ mod tests { } #[tokio::test] - async fn run_missing_file_path() { + async fn run_missing_file_path_reports_input_error() { let output = run(serde_json::json!({}), tracker()).await; assert!(output.is_error); assert!(output.content.contains("Invalid input")); @@ -382,7 +382,7 @@ mod tests { // ── read_file ── #[tokio::test] - async fn read_file_with_line_numbers() { + async fn read_file_existing_file_numbers_lines() { let dir = canonical_tempdir(); let path = dir.path().join("test.txt"); std::fs::write( @@ -554,7 +554,7 @@ mod tests { } #[tokio::test] - async fn read_file_empty() { + async fn read_file_empty_file_renders_marker() { let dir = canonical_tempdir(); let path = dir.path().join("empty.txt"); std::fs::write(&path, "").unwrap(); @@ -566,7 +566,7 @@ mod tests { } #[tokio::test] - async fn read_file_not_found() { + async fn read_file_missing_file_reports_error() { let err = read_file("/nonexistent/path.txt", None, None, &FileTracker::default()) .await .unwrap_err(); @@ -574,7 +574,7 @@ mod tests { } #[tokio::test] - async fn read_file_directory() { + async fn read_file_directory_reports_error() { let dir = canonical_tempdir(); let err = read_file( dir.path().to_str().unwrap(), @@ -588,7 +588,7 @@ mod tests { } #[tokio::test] - async fn read_file_too_large() { + async fn read_file_oversized_file_reports_error() { let dir = canonical_tempdir(); let path = dir.path().join("huge.txt"); let f = std::fs::File::create(&path).unwrap(); @@ -621,7 +621,7 @@ mod tests { } #[tokio::test] - async fn read_file_binary_detected() { + async fn read_file_binary_content_reports_error() { let dir = canonical_tempdir(); let path = dir.path().join("binary.bin"); std::fs::write(&path, b"hello\x00world").unwrap(); @@ -633,7 +633,7 @@ mod tests { } #[tokio::test] - async fn read_file_offset_beyond_end() { + async fn read_file_offset_beyond_end_reports_error() { let dir = canonical_tempdir(); let path = dir.path().join("test.txt"); std::fs::write(&path, "a\nb\n").unwrap(); @@ -665,7 +665,7 @@ mod tests { assert!(!err.contains("SECRET"), "{err}"); } - // ── read_excerpt_view ── + // ── result_view ── #[test] fn result_view_builds_read_excerpt() { diff --git a/crates/oxide-code/src/tool/write.rs b/crates/oxide-code/src/tool/write.rs index ffc47874..9e283bce 100644 --- a/crates/oxide-code/src/tool/write.rs +++ b/crates/oxide-code/src/tool/write.rs @@ -8,7 +8,8 @@ use std::sync::Arc; use serde::Deserialize; use super::{ - ApprovalBody, ApprovalPreview, Tool, ToolOutput, extract_input_field, summarize_path_call, + ApprovalBody, ApprovalPreview, Tool, ToolOutput, WriteMode, extract_input_field, + summarize_path_call, }; use crate::file_tracker::{FileTracker, GatePurpose}; use crate::permission::PermissionClass; @@ -44,17 +45,6 @@ impl Tool for WriteTool { Some("file_path") } - fn approval_preview(&self, input: &serde_json::Value) -> ApprovalPreview { - let content = extract_input_field(input, "content").unwrap_or_default(); - let current = extract_input_field(input, "file_path") - .map(current_text) - .unwrap_or_default(); - ApprovalPreview { - title: self.summarize_call(input), - body: ApprovalBody::Diff(vec![super::edit::synthesize_chunk(¤t, content)]), - } - } - fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", @@ -84,6 +74,17 @@ impl Tool for WriteTool { summarize_path_call(self.name(), input, "file_path") } + fn approval_preview(&self, input: &serde_json::Value) -> ApprovalPreview { + let content = extract_input_field(input, "content").unwrap_or_default(); + let current = extract_input_field(input, "file_path") + .map(current_text) + .unwrap_or_default(); + ApprovalPreview { + title: self.summarize_call(input), + body: ApprovalBody::Diff(vec![super::edit::synthesize_chunk(¤t, content)]), + } + } + fn run( &self, input: serde_json::Value, @@ -157,7 +158,12 @@ async fn write_file( .map_err(|e| format!("Failed to create directory {}: {e}", parent.display()))?; } - super::write_tracked(tracker, path, content.as_bytes().to_vec(), is_new) + let mode = if is_new { + WriteMode::CreateNew + } else { + WriteMode::Overwrite + }; + super::write_tracked(tracker, path, content.as_bytes().to_vec(), mode) .await .map_err(|e| format!("Failed to write file: {e}"))?; @@ -200,16 +206,6 @@ mod tests { use crate::tool::read::ReadTool; use crate::tool::testing::{canonical_tempdir, symlink_file}; - // ── permission_class ── - - #[test] - fn permission_class_is_edit() { - assert_eq!( - WriteTool::new(tracker()).permission_class(), - PermissionClass::Edit - ); - } - // ── approval_preview ── #[test] @@ -274,7 +270,7 @@ mod tests { // ── run ── #[tokio::test] - async fn run_creates_file() { + async fn run_new_file_reports_creation() { let dir = canonical_tempdir(); let path = dir.path().join("new.txt"); @@ -292,7 +288,7 @@ mod tests { } #[tokio::test] - async fn run_missing_required_fields() { + async fn run_missing_required_fields_reports_input_error() { let output = run(serde_json::json!({"file_path": "/tmp/x"}), tracker()).await; assert!(output.is_error); assert!(output.content.contains("Invalid input")); @@ -350,7 +346,7 @@ mod tests { // ── write_file ── #[tokio::test] - async fn write_file_creates_new() { + async fn write_file_new_path_creates_content() { let dir = canonical_tempdir(); let path = dir.path().join("new.txt"); @@ -473,7 +469,7 @@ mod tests { } #[tokio::test] - async fn write_file_empty_content() { + async fn write_file_empty_content_creates_empty_file() { let dir = canonical_tempdir(); let path = dir.path().join("empty.txt"); From 9aa773d2ca5cce4cae40bfb07ff982c180af8c86 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 14:31:38 +0800 Subject: [PATCH 4/5] refactor(session): clarify actor state and listing scope --- crates/oxide-code/src/main.rs | 51 ++- crates/oxide-code/src/session/actor.rs | 321 +++++++++--------- crates/oxide-code/src/session/chain.rs | 23 +- crates/oxide-code/src/session/display.rs | 17 +- crates/oxide-code/src/session/handle.rs | 197 +++-------- .../oxide-code/src/session/handle/testing.rs | 27 +- crates/oxide-code/src/session/history.rs | 140 ++++---- crates/oxide-code/src/session/list_view.rs | 133 ++++---- crates/oxide-code/src/session/resolver.rs | 12 +- crates/oxide-code/src/session/sanitize.rs | 69 ++-- crates/oxide-code/src/session/state.rs | 69 ++-- crates/oxide-code/src/session/store.rs | 107 +----- .../oxide-code/src/session/store/listing.rs | 117 +++++-- .../oxide-code/src/session/store/testing.rs | 2 +- .../oxide-code/src/session/title_generator.rs | 37 +- crates/oxide-code/src/slash/resume.rs | 12 +- crates/oxide-code/src/util/git.rs | 22 +- crates/oxide-code/tests/cli.rs | 84 ++++- docs/design/slash/resume.md | 6 +- 19 files changed, 709 insertions(+), 737 deletions(-) diff --git a/crates/oxide-code/src/main.rs b/crates/oxide-code/src/main.rs index f50f11a4..d7dafb04 100644 --- a/crates/oxide-code/src/main.rs +++ b/crates/oxide-code/src/main.rs @@ -38,7 +38,7 @@ use session::handle::ResumedSession; use session::list_view::render_list; use session::resolver::resolve_session; use session::store::SessionStore; -use session::store::listing::DEFAULT_SESSION_LIST_LIMIT; +use session::store::listing::{DEFAULT_SESSION_LIST_LIMIT, ListScope}; use slash::ResumePicker; use tool::{ ToolRegistry, bash::BashTool, edit::EditTool, glob::GlobTool, grep::GrepTool, read::ReadTool, @@ -135,8 +135,10 @@ async fn async_main() -> Result { if cli.list { list_sessions( &mut std::io::stdout().lock(), - cli.all, - cli.archived, + ListScope { + all: cli.all, + archived: cli.archived, + }, cli.limit, )?; return Ok(Exit::Success); @@ -203,17 +205,12 @@ async fn async_main() -> Result { // ── Session Helpers ── -fn list_sessions( - out: &mut dyn std::io::Write, - all: bool, - archived: bool, - limit: usize, -) -> Result<()> { +fn list_sessions(out: &mut dyn std::io::Write, scope: ListScope, limit: usize) -> Result<()> { let store = SessionStore::open()?; let local_offset = util::time::local_offset(); let term_width = detect_terminal_width(); let cap = (limit > 0).then_some(limit); - render_list(out, &store, all, archived, local_offset, term_width, cap) + render_list(out, &store, scope, local_offset, term_width, cap) } fn detect_terminal_width() -> Option { @@ -306,14 +303,22 @@ mod tests { ); let mut output = Vec::new(); - list_sessions(&mut output, false, false, 0).unwrap(); + list_sessions(&mut output, ListScope::default(), 0).unwrap(); let local = String::from_utf8(output).unwrap(); assert!(local.contains("Local session")); assert!(!local.contains("Other session")); assert!(!local.contains("Project")); let mut output = Vec::new(); - list_sessions(&mut output, true, false, 0).unwrap(); + list_sessions( + &mut output, + ListScope { + all: true, + ..ListScope::default() + }, + 0, + ) + .unwrap(); let all = String::from_utf8(output).unwrap(); assert!(all.contains("Local session")); assert!(all.contains("Other session")); @@ -321,7 +326,15 @@ mod tests { assert!(!all.contains("... more")); let mut output = Vec::new(); - list_sessions(&mut output, true, false, 1).unwrap(); + list_sessions( + &mut output, + ListScope { + all: true, + ..ListScope::default() + }, + 1, + ) + .unwrap(); let capped = String::from_utf8(output).unwrap(); assert!(capped.contains("Other session")); assert!(!capped.contains("Local session")); @@ -329,7 +342,15 @@ mod tests { set_archived_at(&other.find_session_path("other-session").unwrap(), true).unwrap(); let mut output = Vec::new(); - list_sessions(&mut output, true, true, 0).unwrap(); + list_sessions( + &mut output, + ListScope { + all: true, + archived: true, + }, + 0, + ) + .unwrap(); let archived = String::from_utf8(output).unwrap(); assert!(archived.contains("Other session")); assert!(!archived.contains("Local session")); @@ -340,7 +361,7 @@ mod tests { fn list_sessions_storage_error() { let file = tempfile::NamedTempFile::new().unwrap(); temp_env::with_var("XDG_DATA_HOME", Some(file.path()), || { - assert!(list_sessions(&mut Vec::new(), false, false, 0).is_err()); + assert!(list_sessions(&mut Vec::new(), ListScope::default(), 0).is_err()); }); } diff --git a/crates/oxide-code/src/session/actor.rs b/crates/oxide-code/src/session/actor.rs index 65006495..3a300db6 100644 --- a/crates/oxide-code/src/session/actor.rs +++ b/crates/oxide-code/src/session/actor.rs @@ -11,7 +11,7 @@ use tokio::sync::{mpsc, oneshot}; use tracing::warn; use uuid::Uuid; -use super::entry::{Entry, UsageRecord}; +use super::entry::{Entry, TitleSource, UsageRecord}; use super::handle::{CompactOutcome, Outcome, RecordOutcome, SharedState}; use super::state::SessionState; use crate::file_tracker::FileSnapshot; @@ -102,32 +102,16 @@ pub(super) async fn run( shared: Arc, ) { while let Some(first) = rx.recv().await { - let mut entries: Vec = Vec::new(); - let mut acks: Vec = Vec::new(); - let mut should_exit = false; - let mut flow = absorb( - first, - &mut entries, - &mut acks, - &mut state, - &shared, - &mut should_exit, - ); + let mut batch = Batch::default(); + let mut flow = batch.absorb(first, &mut state, &shared); // Compact and archive are batch barriers. If either fails, its entries stay at the queue // tail for `settle_barriers` to drop. Records after a compact see its synthetic root. while matches!(flow, BatchFlow::Continue) && let Ok(next) = rx.try_recv() { - flow = absorb( - next, - &mut entries, - &mut acks, - &mut state, - &shared, - &mut should_exit, - ); + flow = batch.absorb(next, &mut state, &shared); } - let failure = match state.flush_entries(entries) { + let failure = match state.flush_entries(batch.entries) { Err(e) => { let msg = format!("{e:#}"); warn!("session write batch failed: {msg}"); @@ -136,148 +120,155 @@ pub(super) async fn run( } Ok(()) => None, }; - settle_barriers(&acks, &mut state, failure.is_none()); + settle_barriers(&batch.acks, &mut state, failure.is_none()); deliver_acks( - acks, + batch.acks, failure.as_deref(), state.persistence_stopped(), &shared, ); - if should_exit { + if batch.should_exit { break; } } } -fn absorb( - cmd: SessionCmd, - entries: &mut Vec, - acks: &mut Vec, - state: &mut SessionState, - shared: &SharedState, - should_exit: &mut bool, -) -> BatchFlow { - let now = OffsetDateTime::now_utc(); - match cmd { - SessionCmd::Record { msg, context, ack } => { - let (msg_entries, ai_title_seed) = - state.queue_message_entries(&msg, now, shared.manual_title_set()); - if let Some(context) = context { - entries.push(Entry::Context { - context: Box::new(context), - timestamp: now, - }); +#[derive(Default)] +struct Batch { + entries: Vec, + acks: Vec, + should_exit: bool, +} + +impl Batch { + fn absorb( + &mut self, + cmd: SessionCmd, + state: &mut SessionState, + shared: &SharedState, + ) -> BatchFlow { + let now = OffsetDateTime::now_utc(); + match cmd { + SessionCmd::Record { msg, context, ack } => { + let (msg_entries, ai_title_seed) = + state.queue_message_entries(&msg, now, shared.manual_title_set()); + if let Some(context) = context { + self.entries.push(Entry::Context { + context: Box::new(context), + timestamp: now, + }); + } + self.entries.extend(msg_entries); + self.acks.push(PendingAck::Record { ack, ai_title_seed }); + BatchFlow::Continue } - entries.extend(msg_entries); - acks.push(PendingAck::Record { ack, ai_title_seed }); - BatchFlow::Continue - } - SessionCmd::ToolMetadata { items, ack } => { - absorb_tool_metadata(items, ack, now, entries, acks); - BatchFlow::Continue - } - SessionCmd::Usage { records, ack } => { - entries.extend(records.into_iter().map(|usage| Entry::Usage { - uuid: Uuid::new_v4(), - usage, - timestamp: now, - })); - acks.push(PendingAck::Outcome(ack)); - BatchFlow::Continue - } - SessionCmd::AppendAiTitle { title, ack } => { - // Re-check: a `/rename` can flip the latch after the generator already queued this. - if shared.manual_title_set() { - _ = ack.send(Outcome { failure: None }); - return BatchFlow::Continue; + SessionCmd::ToolMetadata { items, ack } => { + self.absorb_tool_metadata(items, ack, now); + BatchFlow::Continue } - entries.push(Entry::Title { - title, - source: super::entry::TitleSource::AiGenerated, - updated_at: now, - }); - acks.push(PendingAck::Outcome(ack)); - BatchFlow::Continue - } - SessionCmd::SetManualTitle { title, ack } => { - shared.mark_manual_title_set(); - match state.try_defer_title(title) { - None => _ = ack.send(Outcome { failure: None }), - Some(title) => { - entries.push(Entry::Title { - title, - source: super::entry::TitleSource::UserProvided, - updated_at: now, - }); - acks.push(PendingAck::Outcome(ack)); + SessionCmd::Usage { records, ack } => { + self.entries + .extend(records.into_iter().map(|usage| Entry::Usage { + uuid: Uuid::new_v4(), + usage, + timestamp: now, + })); + self.acks.push(PendingAck::Outcome(ack)); + BatchFlow::Continue + } + SessionCmd::AppendAiTitle { title, ack } => { + // Re-check: a `/rename` can flip the latch after the generator already queued this. + if shared.manual_title_set() { + _ = ack.send(Outcome { failure: None }); + return BatchFlow::Continue; } + self.entries.push(Entry::Title { + title, + source: TitleSource::AiGenerated, + updated_at: now, + }); + self.acks.push(PendingAck::Outcome(ack)); + BatchFlow::Continue } - BatchFlow::Continue - } - SessionCmd::Archive { ack } => { - entries.push(Entry::Archive { - archived: true, - at: now, - }); - acks.push(PendingAck::Archive(ack)); - BatchFlow::FlushNow - } - SessionCmd::Finish { snapshots, ack } => { - entries.extend(state.finish_entries(snapshots, now)); - acks.push(PendingAck::Outcome(ack)); - BatchFlow::Continue - } - SessionCmd::Compact { - summary, - instructions, - synthetic_message, - ack, - } => { - let pre_count = state.message_count(); - let (compact_entries, synthetic_uuid) = - state.compact_entries(&summary, instructions, synthetic_message, now); - entries.extend(compact_entries); - acks.push(PendingAck::Compact { + SessionCmd::SetManualTitle { title, ack } => { + shared.mark_manual_title_set(); + match state.try_defer_title(title) { + None => _ = ack.send(Outcome { failure: None }), + Some(title) => { + self.entries.push(Entry::Title { + title, + source: TitleSource::UserProvided, + updated_at: now, + }); + self.acks.push(PendingAck::Outcome(ack)); + } + } + BatchFlow::Continue + } + SessionCmd::Archive { ack } => { + self.entries.push(Entry::Archive { + archived: true, + at: now, + }); + self.acks.push(PendingAck::Archive(ack)); + BatchFlow::FlushNow + } + SessionCmd::Finish { snapshots, ack } => { + self.entries.extend(state.finish_entries(snapshots, now)); + self.acks.push(PendingAck::Outcome(ack)); + BatchFlow::Continue + } + SessionCmd::Compact { + summary, + instructions, + synthetic_message, ack, - pre_count, - synthetic_uuid, - }); - BatchFlow::FlushNow - } - SessionCmd::Shutdown { ack } => { - acks.push(PendingAck::Shutdown(ack)); - *should_exit = true; - BatchFlow::Continue + } => { + let pre_count = state.message_count(); + let (compact_entries, synthetic_uuid) = + state.compact_entries(&summary, instructions, synthetic_message, now); + self.entries.extend(compact_entries); + self.acks.push(PendingAck::Compact { + ack, + pre_count, + synthetic_uuid, + }); + BatchFlow::FlushNow + } + SessionCmd::Shutdown { ack } => { + self.acks.push(PendingAck::Shutdown(ack)); + self.should_exit = true; + BatchFlow::Continue + } + #[cfg(test)] + SessionCmd::Panic => panic!("deliberate actor panic for testing"), } - #[cfg(test)] - SessionCmd::Panic => panic!("deliberate actor panic for testing"), } -} -/// Queues one sidecar per item, skipping default metadata since it adds no display fields. Acks at -/// once when nothing needs a flush. -fn absorb_tool_metadata( - items: Vec<(String, ToolMetadata)>, - ack: oneshot::Sender, - now: OffsetDateTime, - entries: &mut Vec, - acks: &mut Vec, -) { - let queued = entries.len(); - entries.extend( - items - .into_iter() - .filter(|(_, metadata)| *metadata != ToolMetadata::default()) - .map(|(tool_use_id, metadata)| Entry::ToolResultMetadata { - tool_use_id, - metadata, - timestamp: now, - }), - ); - if entries.len() > queued { - acks.push(PendingAck::Outcome(ack)); - } else { - _ = ack.send(Outcome { failure: None }); + /// Queues one sidecar per item, skipping default metadata since it adds no display fields. Acks at + /// once when nothing needs a flush. + fn absorb_tool_metadata( + &mut self, + items: Vec<(String, ToolMetadata)>, + ack: oneshot::Sender, + now: OffsetDateTime, + ) { + let queued = self.entries.len(); + self.entries.extend( + items + .into_iter() + .filter(|(_, metadata)| *metadata != ToolMetadata::default()) + .map(|(tool_use_id, metadata)| Entry::ToolResultMetadata { + tool_use_id, + metadata, + timestamp: now, + }), + ); + if self.entries.len() > queued { + self.acks.push(PendingAck::Outcome(ack)); + } else { + _ = ack.send(Outcome { failure: None }); + } } } @@ -322,7 +313,7 @@ fn deliver_acks(acks: Vec, failure: Option<&str>, stopped: bool, sha } => { // Every compact failure reaches the agent, which keeps its old transcript only when // told the write failed. - _ = ack.send(super::handle::CompactOutcome { + _ = ack.send(CompactOutcome { pre_count, failure: failure.map(str::to_owned), }); @@ -349,16 +340,12 @@ fn surface_failure(failure: Option<&str>, stopped: bool, shared: &SharedState) - #[cfg(test)] mod tests { - use std::sync::Arc; - use tempfile::tempdir; - use super::super::handle::SharedState; - use super::super::store::SessionWriter; - use super::super::store::testing::test_store; use super::*; - use crate::message::Message; - use crate::session::state::SessionState; + use crate::session::state::MAX_UNWRITTEN_BYTES; + use crate::session::store::SessionWriter; + use crate::session::store::testing::{test_project_dir, test_session_file, test_store}; /// Drives the actor against `cmds` until the receiver closes (we drop `tx` after sending), /// returning `SharedState` so callers can assert flush-failure flags. @@ -415,7 +402,7 @@ mod tests { drive(state, vec![rec, fin]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); let entries: Vec = content .lines() @@ -489,7 +476,7 @@ mod tests { let outcome = meta_rx.await.unwrap(); assert!(outcome.failure.is_none()); - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); assert!( !content.contains(r#""type":"tool_result_metadata""#), @@ -570,7 +557,7 @@ mod tests { drive(state, vec![manual_cmd, rec, fin]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); let lines: Vec<&str> = content.lines().collect(); let header_idx = lines @@ -626,7 +613,7 @@ mod tests { drive(state, vec![a, b, rec, fin]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); let title_lines: Vec<&str> = content .lines() @@ -668,7 +655,7 @@ mod tests { drive(state, vec![rec, manual_cmd, ai_cmd, fin]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); let title_lines: Vec<&str> = content .lines() @@ -743,7 +730,7 @@ mod tests { drive(state, vec![rec, f1, f2]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); let summary_count = content .lines() @@ -770,7 +757,7 @@ mod tests { drive(state, vec![rec_a, rec_b, compact_cmd]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let content = std::fs::read_to_string(path).unwrap(); let compact_count = content .lines() @@ -858,7 +845,7 @@ mod tests { let session_id = state.session_id.to_string(); let (tx, rx) = mpsc::channel(4); let actor = tokio::spawn(run(state, rx, Arc::new(SharedState::default()))); - let project_dir = super::super::store::testing::test_project_dir(dir.path()); + let project_dir = test_project_dir(dir.path()); std::fs::remove_dir_all(&project_dir).unwrap(); let (rec, rec_rx) = record_cmd("before compact"); @@ -904,7 +891,7 @@ mod tests { let session_id = original.session_id.to_string(); let (rec, _) = record_cmd("before archive"); drive(original, vec![rec]).await; - let path = super::super::store::testing::test_session_file(dir.path(), &session_id); + let path = test_session_file(dir.path(), &session_id); let last_uuid = store.load_session_data(&session_id).unwrap().last_uuid; let state = SessionState::resumed( store.clone(), @@ -945,8 +932,7 @@ mod tests { let state = SessionState::fresh(test_store(dir.path()), "m"); let (tx, rx) = mpsc::channel(4); let actor = tokio::spawn(run(state, rx, Arc::new(SharedState::default()))); - std::fs::remove_dir_all(super::super::store::testing::test_project_dir(dir.path())) - .unwrap(); + std::fs::remove_dir_all(test_project_dir(dir.path())).unwrap(); let (rec, rec_rx) = record_cmd("unsaved"); tx.send(rec).await.unwrap(); @@ -971,7 +957,7 @@ mod tests { let store = test_store(dir.path()); let state = SessionState::fresh(store.clone(), "m"); - let project_dir = super::super::store::testing::test_project_dir(dir.path()); + let project_dir = test_project_dir(dir.path()); std::fs::remove_dir_all(&project_dir).unwrap(); let (cmd, rx) = record_cmd("hello"); @@ -990,11 +976,10 @@ mod tests { let state = SessionState::fresh(test_store(dir.path()), "m"); let (tx, rx) = mpsc::channel(1); let actor = tokio::spawn(run(state, rx, Arc::new(SharedState::default()))); - std::fs::remove_dir_all(super::super::store::testing::test_project_dir(dir.path())) - .unwrap(); + std::fs::remove_dir_all(test_project_dir(dir.path())).unwrap(); let mut failures = Vec::new(); - let huge = "x".repeat(super::super::state::MAX_UNWRITTEN_BYTES); + let huge = "x".repeat(MAX_UNWRITTEN_BYTES); for text in ["small", huge.as_str(), "later"] { let (rec, rec_rx) = record_cmd(text); tx.send(rec).await.unwrap(); diff --git a/crates/oxide-code/src/session/chain.rs b/crates/oxide-code/src/session/chain.rs index e7675963..6c1a0bba 100644 --- a/crates/oxide-code/src/session/chain.rs +++ b/crates/oxide-code/src/session/chain.rs @@ -16,6 +16,7 @@ use crate::prompt::SessionContext; /// The DAG can fork when concurrent resumes (e.g. two `ox -c` processes pointed at the same /// session) each append their own follow-ups onto the same parent. `resolve` picks one tip /// deterministically rather than returning every branch. +#[derive(Default)] pub(super) struct ChainBuilder { nodes: HashMap, /// UUIDs claimed as some other message's `parent_uuid`. Leaves = keys − referenced. @@ -40,10 +41,7 @@ pub(super) struct Chain { impl ChainBuilder { pub(super) fn new() -> Self { - Self { - nodes: HashMap::new(), - referenced: HashSet::new(), - } + Self::default() } /// Last-append-wins on duplicate UUIDs (retry / partial-write replay). `context` is the prompt @@ -377,23 +375,6 @@ mod tests { assert_eq!(chain.messages, []); } - #[test] - fn resolve_self_loop_breaks_at_repeat_visit() { - let mut b = ChainBuilder::new(); - let only = Uuid::new_v4(); - b.insert( - only, - Some(only), - user_message("ouroboros"), - datetime!(2026-04-16 12:00:01 UTC), - None, - ); - - let chain = b.resolve(); - assert!(chain.tip.is_none()); - assert_eq!(chain.messages, []); - } - #[test] fn resolve_duplicate_uuid_keeps_latest_insertion() { let mut b = ChainBuilder::new(); diff --git a/crates/oxide-code/src/session/display.rs b/crates/oxide-code/src/session/display.rs index 686a4601..d9aa5017 100644 --- a/crates/oxide-code/src/session/display.rs +++ b/crates/oxide-code/src/session/display.rs @@ -6,9 +6,10 @@ use std::fmt::Write as _; use time::{OffsetDateTime, UtcOffset}; use super::entry::SessionInfo; +use crate::util::text::plural; const SEPARATOR: &str = " · "; -pub(crate) const ID_PREFIX_WIDTH: usize = 8; +const ID_PREFIX_WIDTH: usize = 8; pub(crate) const UNTITLED_MARKER: &str = "(untitled)"; /// First [`ID_PREFIX_WIDTH`] bytes of `session_id`, falling back to the full id when shorter. @@ -26,7 +27,7 @@ pub(crate) fn display_title(info: &SessionInfo) -> String { /// Compact "N units ago" with an ISO-date fallback past 30 days. Negative deltas (clock skew or /// future stamps) collapse to 0 so the singular / plural axis stays sane. -pub(crate) fn format_relative_time(ts: OffsetDateTime, now: OffsetDateTime) -> String { +fn format_relative_time(ts: OffsetDateTime, now: OffsetDateTime) -> String { let secs = (now - ts).whole_seconds().max(0); let (n, unit) = if secs < 60 { (secs, "second") @@ -41,8 +42,8 @@ pub(crate) fn format_relative_time(ts: OffsetDateTime, now: OffsetDateTime) -> S .format(time::macros::format_description!("[year]-[month]-[day]")) .expect("static `[year]-[month]-[day]` description never fails on a valid ts"); }; - let plural = if n == 1 { "" } else { "s" }; - format!("{n} {unit}{plural} ago") + let suffix = plural(n, "", "s"); + format!("{n} {unit}{suffix} ago") } /// `{id_prefix} · {when} · {N msgs} · {branch} · {project}`. Trailing components are omitted when @@ -61,7 +62,7 @@ pub(crate) fn format_metadata_line( let prefix = id_prefix(session_id); let mut meta = format!("{prefix}{SEPARATOR}{when}"); if message_count > 0 { - let unit = if message_count == 1 { "msg" } else { "msgs" }; + let unit = plural(message_count, "msg", "msgs"); _ = write!(meta, "{SEPARATOR}{message_count} {unit}"); } if let Some(branch) = git_branch { @@ -89,15 +90,15 @@ pub(crate) fn archive_notice(session_id: &str, archived: bool) -> String { #[cfg(test)] mod tests { + use time::macros::datetime; + use super::*; use crate::session::entry::TitleInfo; - use time::macros::datetime; - // ── id_prefix ── #[test] - fn id_prefix_returns_first_eight_bytes_or_full_id_when_shorter() { + fn id_prefix_uses_first_eight_bytes_or_full_short_id() { // Pin both the long-id slicing and the short-id fallback so a future change to direct // `&id[..8]` slicing would panic on short ids and break this test. assert_eq!(id_prefix("abcdefghij"), "abcdefgh"); diff --git a/crates/oxide-code/src/session/handle.rs b/crates/oxide-code/src/session/handle.rs index 48bad9d3..2ac73c5b 100644 --- a/crates/oxide-code/src/session/handle.rs +++ b/crates/oxide-code/src/session/handle.rs @@ -22,6 +22,7 @@ use crate::file_tracker::{FileSnapshot, FileTracker}; use crate::message::Message; use crate::prompt::SessionContext; use crate::tool::ToolMetadata; +use crate::util::sync::lock_or_recover; #[cfg(test)] pub(crate) mod testing; @@ -58,13 +59,7 @@ pub(super) struct SharedState { impl SharedState { pub(super) fn record_flush_failure(&self, msg: &str) { - match self.last_flush_failure.lock() { - Ok(mut slot) => *slot = Some(msg.to_owned()), - Err(e) => { - tracing::warn!("last_flush_failure mutex poisoned, recovering"); - *e.into_inner() = Some(msg.to_owned()); - } - } + *lock_or_recover(&self.last_flush_failure, "last_flush_failure") = Some(msg.to_owned()); } /// `true` on the first call after [`Self::record_flush_failure`], then sticky `false`. @@ -92,13 +87,7 @@ impl SharedState { /// Most recent flush error, threaded into actor-gone messages. pub(super) fn last_flush_failure(&self) -> Option { - match self.last_flush_failure.lock() { - Ok(guard) => guard.clone(), - Err(e) => { - tracing::warn!("last_flush_failure mutex poisoned, recovering"); - e.into_inner().clone() - } - } + lock_or_recover(&self.last_flush_failure, "last_flush_failure").clone() } } @@ -148,21 +137,12 @@ impl SessionHandle { context: Option, ) -> RecordOutcome { let (ack, rx) = oneshot::channel(); - if self - .cmd_tx - .send(SessionCmd::Record { msg, context, ack }) + self.request(SessionCmd::Record { msg, context, ack }, rx) .await - .is_err() - { - return RecordOutcome { + .unwrap_or_else(|| RecordOutcome { ai_title_seed: None, failure: self.actor_gone_failure(), - }; - } - rx.await.unwrap_or_else(|_| RecordOutcome { - ai_title_seed: None, - failure: self.actor_gone_failure(), - }) + }) } /// Record sidecar metadata for a tool round in one cmd. Default-metadata items are skipped. @@ -218,15 +198,12 @@ impl SessionHandle { synthetic_message, ack, }; - if self.cmd_tx.send(cmd).await.is_ok() - && let Ok(outcome) = rx.await - { - return outcome; - } - CompactOutcome { - pre_count: 0, - failure: Some(self.actor_stopped_failure()), - } + self.request(cmd, rx) + .await + .unwrap_or_else(|| CompactOutcome { + pre_count: 0, + failure: Some(self.actor_stopped_failure()), + }) } /// Marks the session archived, reporting every failure since the caller rolls away only once @@ -234,14 +211,11 @@ impl SessionHandle { /// otherwise create a file holding nothing else. pub(crate) async fn archive(&self) -> Outcome { let (ack, rx) = oneshot::channel(); - if self.cmd_tx.send(SessionCmd::Archive { ack }).await.is_ok() - && let Ok(outcome) = rx.await - { - return outcome; - } - Outcome { - failure: Some(self.actor_stopped_failure()), - } + self.request(SessionCmd::Archive { ack }, rx) + .await + .unwrap_or_else(|| Outcome { + failure: Some(self.actor_stopped_failure()), + }) } /// Write the session summary and finalize. Idempotent no-op on fresh sessions that never @@ -273,13 +247,7 @@ impl SessionHandle { _ = rx.await; } drop(cmd_tx); - let join = match actor_join.lock() { - Ok(mut g) => g.take(), - Err(e) => { - tracing::warn!("actor_join mutex poisoned, recovering"); - e.into_inner().take() - } - }; + let join = lock_or_recover(&actor_join, "actor_join").take(); if let Some(j) = join && let Err(e) = j.await { @@ -293,16 +261,16 @@ impl SessionHandle { /// Sends a cmd and awaits the `Outcome` ack, falling back to actor-gone on failure. async fn dispatch_outcome(&self, cmd: SessionCmd, rx: oneshot::Receiver) -> Outcome { - if self.cmd_tx.send(cmd).await.is_err() { - return Outcome { - failure: self.actor_gone_failure(), - }; - } - rx.await.unwrap_or_else(|_| Outcome { + self.request(cmd, rx).await.unwrap_or_else(|| Outcome { failure: self.actor_gone_failure(), }) } + async fn request(&self, cmd: SessionCmd, rx: oneshot::Receiver) -> Option { + self.cmd_tx.send(cmd).await.ok()?; + rx.await.ok() + } + /// The actor-gone failure for a write the caller must know failed, in full the first time. fn actor_stopped_failure(&self) -> String { self.actor_gone_failure() @@ -518,15 +486,17 @@ mod tests { use time::format_description::well_known::Rfc3339; use time::macros::datetime; - use super::super::entry::{SessionInfo, UsageRecord}; - use super::super::store::testing::{test_project_dir, test_session_file, test_store}; use super::*; use crate::file_tracker::{ - self, FileTracker, GateError, GatePurpose, LastView, + self, GateError, GatePurpose, LastView, testing::{record_tracked_file, seed_full_read}, }; use crate::message::{ContentBlock, Role}; use crate::prompt::PromptParts; + use crate::session::entry::{Entry, SessionInfo}; + use crate::session::store::testing::{ + seed_test_session, test_project_dir, test_session_file, test_store, + }; use crate::tool::{DiffChunk, DiffLine}; /// Resumes the session whose file name carries `session_id`. @@ -569,35 +539,6 @@ mod tests { } } - // ── SharedState ── - - #[test] - fn shared_state_record_flush_failure_recovers_from_poisoned_mutex() { - let shared = Arc::new(SharedState::default()); - let s = Arc::clone(&shared); - _ = std::thread::spawn(move || { - let _guard = s.last_flush_failure.lock().unwrap(); - panic!("deliberate poison"); - }) - .join(); - shared.record_flush_failure("disk full"); - assert_eq!(shared.last_flush_failure(), Some("disk full".to_owned())); - } - - #[test] - fn shared_state_last_flush_failure_recovers_from_poisoned_mutex() { - let shared = Arc::new(SharedState::default()); - let s = Arc::clone(&shared); - _ = std::thread::spawn(move || { - let mut guard = s.last_flush_failure.lock().unwrap(); - *guard = Some("pre-poison".to_owned()); - panic!("deliberate poison"); - }) - .join(); - // Recovery path must still return the value written pre-panic. - assert_eq!(shared.last_flush_failure(), Some("pre-poison".to_owned()),); - } - // ── record_message ── #[tokio::test] @@ -1057,7 +998,7 @@ mod tests { handle.finish(Vec::new()).await; drop(handle); - let project_dir = super::super::store::testing::test_project_dir(dir.path()); + let project_dir = test_project_dir(dir.path()); assert!( std::fs::read_dir(&project_dir).unwrap().next().is_none(), "empty session must not write a file", @@ -1191,44 +1132,24 @@ mod tests { } #[tokio::test] - async fn shutdown_is_safe_to_call_after_dead_handle() { + async fn shutdown_dead_actor_completes_without_panicking() { // Smoke check: empty join slot must not panic. let handle = testing::dead("dead"); handle.shutdown().await; } #[tokio::test] - async fn shutdown_logs_actor_panic_instead_of_silently_discarding() { - // An actor that panics mid-batch must surface via tracing::error, - // not disappear into `_ = j.await`. + async fn shutdown_actor_panic_completes_without_panicking() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); - let handle = start(&store, "m"); - handle - .cmd_tx - .send(super::super::actor::SessionCmd::Panic) - .await - .unwrap(); - // shutdown must not panic itself — it handles the JoinError. - handle.shutdown().await; - } - - #[tokio::test] - async fn shutdown_surfaces_panic_from_stand_in_actor() { - // Exercises the Panic arm in acks_then_drops (testing.rs), - // which panics the stand-in actor task. shutdown must catch the - // JoinError and log via tracing::error. - let handle = testing::acks_then_drops("test", 1); - handle - .cmd_tx - .send(super::super::actor::SessionCmd::Panic) - .await - .unwrap(); - handle.shutdown().await; + for handle in [start(&store, "m"), testing::acks_then_drops("test", 1)] { + handle.cmd_tx.send(SessionCmd::Panic).await.unwrap(); + handle.shutdown().await; + } } #[tokio::test] - async fn shutdown_logs_cancellation_when_actor_task_aborted() { + async fn shutdown_cancelled_actor_completes_without_panicking() { // Abort produces a JoinError where is_panic() == false, // exercising the "cancelled" branch in shutdown. let dir = tempfile::tempdir().unwrap(); @@ -1240,22 +1161,6 @@ mod tests { handle.shutdown().await; } - #[tokio::test] - async fn shutdown_recovers_from_poisoned_actor_join_mutex() { - let dir = tempfile::tempdir().unwrap(); - let store = test_store(dir.path()); - let handle = start(&store, "m"); - // Poison the actor_join mutex. - let join_ref = Arc::clone(&handle.actor_join); - _ = std::thread::spawn(move || { - let _guard = join_ref.lock().unwrap(); - panic!("deliberate poison"); - }) - .join(); - // shutdown must recover (take the JoinHandle) and not panic. - handle.shutdown().await; - } - // ── start ── #[tokio::test] @@ -1264,7 +1169,7 @@ mod tests { let store = test_store(dir.path()); let handle = start(&store, "test-model"); - let project_dir = super::super::store::testing::test_project_dir(dir.path()); + let project_dir = test_project_dir(dir.path()); assert!( std::fs::read_dir(&project_dir).unwrap().next().is_none(), "fresh session must not create a file before the first record", @@ -1502,7 +1407,7 @@ mod tests { // ── resume_from_path ── #[tokio::test] - async fn resume_loads_messages_and_keeps_session_id() { + async fn resume_from_path_loads_messages_and_keeps_session_id() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let original = start(&store, "m"); @@ -1522,7 +1427,7 @@ mod tests { } #[tokio::test] - async fn resume_works_on_unfinished_session() { + async fn resume_from_path_works_on_unfinished_session() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let original = start(&store, "m"); @@ -1536,12 +1441,12 @@ mod tests { } #[tokio::test] - async fn resume_title_only_session_keeps_title_on_next_message() { + async fn resume_from_path_title_only_session_keeps_title_on_next_message() { // Title-only resume must not push a duplicate `FirstPrompt` on the next message. let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let session_id = "title-only-fixture"; - super::super::store::testing::seed_test_session( + seed_test_session( &store, session_id, Some("User-named"), @@ -1585,7 +1490,7 @@ mod tests { } #[tokio::test] - async fn resume_empty_session_errors() { + async fn resume_from_path_empty_session_errors() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let original = start(&store, "m"); @@ -1597,7 +1502,7 @@ mod tests { } #[tokio::test] - async fn resume_all_messages_sanitized_errors() { + async fn resume_from_path_all_messages_sanitized_errors() { // Sanitize drops the lone unresolved tool_use, leaving an empty chain — resume must bail. let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); @@ -1625,7 +1530,7 @@ mod tests { } #[tokio::test] - async fn resume_drops_assistant_message_with_only_unresolved_tool_use() { + async fn resume_from_path_drops_assistant_message_with_only_unresolved_tool_use() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let original = start(&store, "m"); @@ -1653,7 +1558,7 @@ mod tests { } #[tokio::test] - async fn resume_preserves_parent_chain_on_next_record() { + async fn resume_from_path_preserves_parent_chain_on_next_record() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let original = start(&store, "m"); @@ -1669,7 +1574,7 @@ mod tests { drop(resumed); let content = std::fs::read_to_string(test_session_file(dir.path(), &session_id)).unwrap(); - let entries: Vec = content + let entries: Vec = content .lines() .filter(|l| !l.is_empty()) .filter_map(|l| serde_json::from_str(l).ok()) @@ -1678,7 +1583,7 @@ mod tests { let msg_uuids: Vec<_> = entries .iter() .filter_map(|e| match e { - super::super::entry::Entry::Message { + Entry::Message { uuid, parent_uuid, .. } => Some((*uuid, *parent_uuid)), _ => None, @@ -1695,7 +1600,7 @@ mod tests { } #[tokio::test] - async fn resume_restores_unchanged_snapshots_whose_tool_result_survives() { + async fn resume_from_path_restores_unchanged_snapshots_whose_tool_result_survives() { let dir = tempfile::tempdir().unwrap(); let files_dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); @@ -1729,7 +1634,7 @@ mod tests { } #[tokio::test] - async fn resume_drops_drifted_snapshots() { + async fn resume_from_path_drops_drifted_snapshots() { let dir = tempfile::tempdir().unwrap(); let files_dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); @@ -1765,7 +1670,7 @@ mod tests { } #[tokio::test] - async fn resume_drops_snapshots_for_missing_files() { + async fn resume_from_path_drops_snapshots_for_missing_files() { let dir = tempfile::tempdir().unwrap(); let files_dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); diff --git a/crates/oxide-code/src/session/handle/testing.rs b/crates/oxide-code/src/session/handle/testing.rs index 3acbbd62..4798de6a 100644 --- a/crates/oxide-code/src/session/handle/testing.rs +++ b/crates/oxide-code/src/session/handle/testing.rs @@ -5,20 +5,16 @@ use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; use tokio::sync::mpsc; +use tokio::task::JoinHandle; -use super::super::actor::SessionCmd; use super::{CompactOutcome, Outcome, RecordOutcome, SessionHandle, SharedState}; +use crate::session::actor::SessionCmd; /// Handle whose actor channel is already closed — every write surfaces actor-gone immediately, /// so failure surfacing can be asserted without a real actor. pub(crate) fn dead(session_id: &str) -> SessionHandle { let (cmd_tx, _) = mpsc::channel(1); - SessionHandle { - cmd_tx, - session_id: Arc::from(session_id), - shared: Arc::new(SharedState::default()), - actor_join: Arc::new(std::sync::Mutex::new(None)), - } + handle(session_id, cmd_tx, None) } /// Stand-in actor that fails every `AppendAiTitle` ack with `message` — exercises the @@ -38,12 +34,7 @@ pub(crate) fn acks_append_ai_title_with_failure( } }); - SessionHandle { - cmd_tx, - session_id: Arc::from(session_id), - shared: Arc::new(SharedState::default()), - actor_join: Arc::new(std::sync::Mutex::new(Some(join))), - } + handle(session_id, cmd_tx, Some(join)) } /// Stand-in actor that acks the first `succeed` non-Shutdown cmds, then drops cmds without @@ -91,10 +82,18 @@ pub(crate) fn acks_then_drops(session_id: &str, succeed: usize) -> SessionHandle } }); + handle(session_id, cmd_tx, Some(join)) +} + +fn handle( + session_id: &str, + cmd_tx: mpsc::Sender, + join: Option>, +) -> SessionHandle { SessionHandle { cmd_tx, session_id: Arc::from(session_id), shared: Arc::new(SharedState::default()), - actor_join: Arc::new(std::sync::Mutex::new(Some(join))), + actor_join: Arc::new(std::sync::Mutex::new(join)), } } diff --git a/crates/oxide-code/src/session/history.rs b/crates/oxide-code/src/session/history.rs index 78624b23..d11fc429 100644 --- a/crates/oxide-code/src/session/history.rs +++ b/crates/oxide-code/src/session/history.rs @@ -5,11 +5,11 @@ use std::collections::HashMap; +use super::entry::CompactInfo; use crate::agent::compaction::strip_synthetic_post_compact_prefix; use crate::message::{ContentBlock, Message, Role, progress_update_before}; use crate::prompt::is_system_reminder; use crate::protocol::{Interaction, ToolCall, ToolResult}; -use crate::session::entry::CompactInfo; use crate::tool::{ToolMetadata, ToolRegistry, ToolResultView}; /// Projects a resumed transcript for display, opening with its compaction boundary when `compact` @@ -55,7 +55,7 @@ fn walk_transcript( tool_metadata: &HashMap, tools: &ToolRegistry, ) -> Vec { - let mut pairs: HashMap<&str, (&str, bool)> = messages + let pairs: HashMap<&str, (&str, bool)> = messages .iter() .flat_map(|m| m.content.iter()) .filter_map(|b| match b { @@ -68,8 +68,27 @@ fn walk_transcript( }) .collect(); - let mut out = Vec::new(); - for msg in messages { + let mut walker = TranscriptWalker { + tools, + tool_metadata, + pairs, + out: Vec::new(), + }; + for message in messages { + walker.push_message(message); + } + walker.out +} + +struct TranscriptWalker<'a> { + tools: &'a ToolRegistry, + tool_metadata: &'a HashMap, + pairs: HashMap<&'a str, (&'a str, bool)>, + out: Vec, +} + +impl TranscriptWalker<'_> { + fn push_message(&mut self, msg: &Message) { let mut text_buf = String::new(); for (index, block) in msg.content.iter().enumerate() { match block { @@ -83,27 +102,20 @@ fn walk_transcript( } ContentBlock::ToolUse { id, name, input } | ContentBlock::ServerToolUse { id, name, input } => { - flush_text(&mut out, &mut text_buf, msg.role); + self.flush_text(&mut text_buf, msg.role); if let Some(text) = progress_update_before(&msg.content, index) { - out.push(Interaction::ProgressUpdate(text.to_owned())); + self.out.push(Interaction::ProgressUpdate(text.to_owned())); } - out.push(Interaction::ToolCall(ToolCall { + self.out.push(Interaction::ToolCall(ToolCall { id: id.clone(), name: name.clone(), input: input.clone(), - icon: tools.icon(name).to_owned(), - label: tools.label(name, input), + icon: self.tools.icon(name).to_owned(), + label: self.tools.label(name, input), })); - if let Some((content, is_error)) = pairs.remove(id.as_str()) { + if let Some((content, is_error)) = self.pairs.remove(id.as_str()) { let call = Some((name.as_str(), input)); - out.push(tool_result( - tools, - tool_metadata, - id, - call, - content, - is_error, - )); + self.push_tool_result(id, call, content, is_error); } } // Paired results were already emitted inline, so only orphans reach here. @@ -111,66 +123,60 @@ fn walk_transcript( tool_use_id, content, is_error, - } if pairs.remove(tool_use_id.as_str()).is_some() => { - flush_text(&mut out, &mut text_buf, msg.role); - out.push(tool_result( - tools, - tool_metadata, - tool_use_id, - None, - content, - *is_error, - )); + } if self.pairs.remove(tool_use_id.as_str()).is_some() => { + self.flush_text(&mut text_buf, msg.role); + self.push_tool_result(tool_use_id, None, content, *is_error); } ContentBlock::Thinking { thinking, .. } if !thinking.trim().is_empty() => { - flush_text(&mut out, &mut text_buf, msg.role); - out.push(Interaction::AssistantThinking(thinking.clone())); + self.flush_text(&mut text_buf, msg.role); + self.out + .push(Interaction::AssistantThinking(thinking.clone())); } _ => {} } } - flush_text(&mut out, &mut text_buf, msg.role); + self.flush_text(&mut text_buf, msg.role); } - out -} -/// Resolves a result's view through the tool its `call` names, or as plain text when the -/// transcript lost the call. -fn tool_result( - tools: &ToolRegistry, - tool_metadata: &HashMap, - id: &str, - call: Option<(&str, &serde_json::Value)>, - content: &str, - is_error: bool, -) -> Interaction { - let default = ToolMetadata::default(); - let metadata = tool_metadata.get(id).unwrap_or(&default); - let view = match call { - Some((name, input)) => tools.result_view(name, input, content, metadata, is_error), - None => ToolResultView::Text { + /// Resolves a result's view through the tool its `call` names, or as plain text when the + /// transcript lost the call. + fn push_tool_result( + &mut self, + id: &str, + call: Option<(&str, &serde_json::Value)>, + content: &str, + is_error: bool, + ) { + let default = ToolMetadata::default(); + let metadata = self.tool_metadata.get(id).unwrap_or(&default); + let view = match call { + Some((name, input)) => self + .tools + .result_view(name, input, content, metadata, is_error), + None => ToolResultView::Text { + content: content.to_owned(), + }, + }; + self.out.push(Interaction::ToolResult(ToolResult { + id: id.to_owned(), content: content.to_owned(), - }, - }; - Interaction::ToolResult(ToolResult { - id: id.to_owned(), - content: content.to_owned(), - is_error, - title: metadata.title.clone(), - view, - }) -} + is_error, + title: metadata.title.clone(), + view, + })); + } -/// Flushes `text_buf` as a role-tagged text interaction. No-op on empty. -fn flush_text(out: &mut Vec, text_buf: &mut String, role: Role) { - if text_buf.is_empty() { - return; + /// Flushes `text_buf` as a role-tagged text interaction. No-op on empty. + fn flush_text(&mut self, text_buf: &mut String, role: Role) { + if text_buf.is_empty() { + return; + } + let text = std::mem::take(text_buf); + self.out.push(match role { + Role::User => Interaction::UserText(text), + Role::Assistant => Interaction::AssistantText(text), + }); } - let text = std::mem::take(text_buf); - out.push(match role { - Role::User => Interaction::UserText(text), - Role::Assistant => Interaction::AssistantText(text), - }); } #[cfg(test)] diff --git a/crates/oxide-code/src/session/list_view.rs b/crates/oxide-code/src/session/list_view.rs index 60425362..f89cfa73 100644 --- a/crates/oxide-code/src/session/list_view.rs +++ b/crates/oxide-code/src/session/list_view.rs @@ -1,5 +1,6 @@ //! Text rendering for `ox --list`, of either active or archived sessions. +use std::fmt::Write as _; use std::io::Write; use std::path::Path; @@ -9,13 +10,14 @@ use unicode_width::UnicodeWidthStr; use super::display::{UNTITLED_MARKER, id_prefix}; use super::store::SessionStore; -use super::store::listing::ListPage; +use super::store::listing::{ListPage, ListScope}; use crate::util::path::tildify; use crate::util::text::truncate_to_width; -/// `ID(10) + ' ' + LastActive(19) + ' ' + Msgs(6) + ' '` — fixed prefix before any optional -/// `Project` column and the final `Title`. -const FIXED_PREFIX_WIDTH: usize = 10 + 1 + 19 + 1 + 6 + 1; +const ID_COL: usize = 10; +const LAST_ACTIVE_COL: usize = 19; +const MSGS_COL: usize = 6; +const FIXED_PREFIX_WIDTH: usize = ID_COL + 1 + LAST_ACTIVE_COL + 1 + MSGS_COL + 1; /// Skip title truncation under this width — wrap rather than chop everything to `F...`. const MIN_TITLE_BUDGET: usize = 12; @@ -27,33 +29,31 @@ const PROJECT_COL_MIN: usize = 8; /// title column for every row in the listing. const PROJECT_COL_MAX: usize = 40; -/// Renders `--list` output to `out`. `all` spans every project, and `archived` lists archived -/// sessions in place of active ones. A `term_width` of `None` skips title truncation for piped -/// output. `limit` caps the rows printed, unbounded for `None`. +/// Renders `--list` output to `out`. `scope.all` spans every project, and `scope.archived` +/// selects archived sessions. A `term_width` of `None` skips title truncation for piped output. +/// `limit` caps the rows printed, unbounded for `None`. pub(crate) fn render_list( out: &mut dyn Write, store: &SessionStore, - all: bool, - archived: bool, + scope: ListScope, local_offset: UtcOffset, term_width: Option, limit: Option, ) -> Result<()> { - let page = store.list_paged(limit, all, archived)?; - render_sessions(out, &page, all, archived, local_offset, term_width) + let page = store.list_paged(limit, scope)?; + render_sessions(out, &page, scope, local_offset, term_width) } -/// Pure formatter, split from [`render_list`] so tests can skip building a real store. `all` -/// inserts a `Project` column to disambiguate cross-project rows. Appends a `... more` footer -/// when sessions follow the page. +/// `scope.all` inserts a `Project` column to disambiguate cross-project rows. +/// Appends a `... more` footer when sessions follow the page. fn render_sessions( out: &mut dyn Write, page: &ListPage, - all: bool, - archived: bool, + scope: ListScope, local_offset: UtcOffset, term_width: Option, ) -> Result<()> { + let ListScope { all, archived } = scope; let sessions = page.sessions(); if sessions.is_empty() { let kind = if archived { "archived " } else { "" }; @@ -80,25 +80,14 @@ fn render_sessions( (budget >= MIN_TITLE_BUDGET).then_some(budget) }); + let mut header = format!( + "{: truncate_to_width(raw_title, budget), None => raw_title.to_owned(), }; + let mut row = format!("{id: SessionInfo { SessionInfo { @@ -172,8 +158,10 @@ mod tests { render_sessions( &mut buf, &ListPage::new(sessions.to_vec(), false), - all, - false, + ListScope { + all, + ..ListScope::default() + }, UtcOffset::UTC, term_width, ) @@ -187,20 +175,50 @@ mod tests { fn render_list_empty_store_shows_no_sessions_notice() { // Covers the `render_list → render_sessions` glue. let dir = tempfile::tempdir().unwrap(); - let store = super::super::store::testing::test_store(dir.path()); + let store = test_store(dir.path()); let mut buf = Vec::new(); - render_list(&mut buf, &store, false, false, UtcOffset::UTC, None, None).unwrap(); + render_list( + &mut buf, + &store, + ListScope::default(), + UtcOffset::UTC, + None, + None, + ) + .unwrap(); assert_eq!( String::from_utf8(buf).unwrap(), "No sessions found in this project.\n", ); let mut buf = Vec::new(); - render_list(&mut buf, &store, true, false, UtcOffset::UTC, None, None).unwrap(); + render_list( + &mut buf, + &store, + ListScope { + all: true, + ..ListScope::default() + }, + UtcOffset::UTC, + None, + None, + ) + .unwrap(); assert_eq!(String::from_utf8(buf).unwrap(), "No sessions found.\n",); let mut buf = Vec::new(); - render_list(&mut buf, &store, false, true, UtcOffset::UTC, None, None).unwrap(); + render_list( + &mut buf, + &store, + ListScope { + archived: true, + ..ListScope::default() + }, + UtcOffset::UTC, + None, + None, + ) + .unwrap(); assert_eq!( String::from_utf8(buf).unwrap(), "No archived sessions found in this project.\n", @@ -210,9 +228,9 @@ mod tests { #[test] fn render_list_archived_lists_only_archived_sessions() { let dir = tempfile::tempdir().unwrap(); - let store = super::super::store::testing::test_store(dir.path()); + let store = test_store(dir.path()); for id in ["aaaa1111", "bbbb2222"] { - super::super::store::testing::seed_test_session( + seed_test_session( &store, id, Some(id), @@ -221,14 +239,16 @@ mod tests { ); } let archived = store.find_session_path("bbbb2222").unwrap(); - super::super::store::set_archived_at(&archived, true).unwrap(); + set_archived_at(&archived, true).unwrap(); let render = |archived: bool| { let mut buf = Vec::new(); render_list( &mut buf, &store, - false, - archived, + ListScope { + archived, + ..ListScope::default() + }, UtcOffset::UTC, None, None, @@ -248,9 +268,9 @@ mod tests { #[test] fn render_list_appends_more_footer_only_when_capped() { let dir = tempfile::tempdir().unwrap(); - let store = super::super::store::testing::test_store(dir.path()); + let store = test_store(dir.path()); for id in ["aaaa1111", "bbbb2222"] { - super::super::store::testing::seed_test_session( + seed_test_session( &store, id, Some(id), @@ -263,8 +283,7 @@ mod tests { render_list( &mut buf, &store, - false, - false, + ListScope::default(), UtcOffset::UTC, None, Some(limit), diff --git a/crates/oxide-code/src/session/resolver.rs b/crates/oxide-code/src/session/resolver.rs index c964c66e..51027e96 100644 --- a/crates/oxide-code/src/session/resolver.rs +++ b/crates/oxide-code/src/session/resolver.rs @@ -13,7 +13,7 @@ use super::store::SessionStore; use crate::util::text::ELLIPSIS; /// Normalized form of the CLI `--continue` argument. -pub(crate) enum ResumeMode<'a> { +enum ResumeMode<'a> { /// No `--continue` flag — start a new session. Fresh, /// Bare `--continue` — resume the most recent unarchived session in scope. @@ -71,7 +71,7 @@ pub(crate) async fn resolve_session( /// Trims and classifies a `--continue` argument into a [`ResumeMode`]. Empty / whitespace /// prefixes are rejected explicitly to avoid silently collapsing into "resume latest" — bare /// `--continue` already expresses that. -pub(crate) fn normalize_resume_arg(resume: Option<&Option>) -> Result> { +fn normalize_resume_arg(resume: Option<&Option>) -> Result> { match resume { None => Ok(ResumeMode::Fresh), Some(None) => Ok(ResumeMode::Latest), @@ -175,13 +175,13 @@ fn format_session_id_preview(ids: impl IntoIterator) -> String { #[cfg(test)] mod tests { - use super::super::store::set_archived_at; - use super::super::store::testing::{ + use super::*; + use crate::message::Message; + use crate::session::store::set_archived_at; + use crate::session::store::testing::{ seed_test_session, test_project_dir, test_session_file, test_store, write_newest_corrupt_session, }; - use super::*; - use crate::message::Message; // ── resolve_session ── diff --git a/crates/oxide-code/src/session/sanitize.rs b/crates/oxide-code/src/session/sanitize.rs index d48e5e3a..5222de69 100644 --- a/crates/oxide-code/src/session/sanitize.rs +++ b/crates/oxide-code/src/session/sanitize.rs @@ -37,14 +37,7 @@ pub(super) fn retain_visible_file_snapshots( snapshots: &mut Vec, messages: &[Message], ) { - let visible: HashSet<&str> = messages - .iter() - .flat_map(|m| m.content.iter()) - .filter_map(|b| match b { - ContentBlock::ToolResult { tool_use_id, .. } => Some(tool_use_id.as_str()), - _ => None, - }) - .collect(); + let visible = tool_result_ids(messages); snapshots.retain(|snap| { snap.tool_use_id .as_deref() @@ -57,14 +50,7 @@ pub(super) fn retain_visible_file_snapshots( /// Drops assistant `tool_use` blocks whose id has no matching `tool_result`. Ids are owned /// because the `HashSet` is built from a shared borrow before mutating below. fn drop_unresolved_tool_uses(messages: &mut [Message]) { - let resolved: HashSet = messages - .iter() - .flat_map(|m| m.content.iter()) - .filter_map(|b| match b { - ContentBlock::ToolResult { tool_use_id, .. } => Some(tool_use_id.clone()), - _ => None, - }) - .collect(); + let resolved = tool_result_ids(messages); for msg in messages { if msg.role != Role::Assistant { @@ -82,16 +68,7 @@ fn drop_unresolved_tool_uses(messages: &mut [Message]) { /// Drops user `tool_result` blocks with no surviving assistant `tool_use`, then strips /// messages that prior passes emptied so role-collapse sees no stubs. fn drop_orphan_tool_results(messages: &mut Vec) { - let surviving: HashSet = messages - .iter() - .flat_map(|m| m.content.iter()) - .filter_map(|b| match b { - ContentBlock::ToolUse { id, .. } | ContentBlock::ServerToolUse { id, .. } => { - Some(id.clone()) - } - _ => None, - }) - .collect(); + let surviving = tool_use_ids(messages); for msg in &mut *messages { if msg.role != Role::User { @@ -139,6 +116,30 @@ fn insert_resume_sentinels(messages: &mut Vec) { } } +fn tool_result_ids(messages: &[Message]) -> HashSet { + messages + .iter() + .flat_map(|m| &m.content) + .filter_map(|block| match block { + ContentBlock::ToolResult { tool_use_id, .. } => Some(tool_use_id.clone()), + _ => None, + }) + .collect() +} + +fn tool_use_ids(messages: &[Message]) -> HashSet { + messages + .iter() + .flat_map(|m| &m.content) + .filter_map(|block| match block { + ContentBlock::ToolUse { id, .. } | ContentBlock::ServerToolUse { id, .. } => { + Some(id.clone()) + } + _ => None, + }) + .collect() +} + #[cfg(test)] mod tests { use std::path::PathBuf; @@ -181,7 +182,7 @@ mod tests { // ── sanitize_resumed_messages ── #[test] - fn sanitize_noop_for_clean_transcript() { + fn sanitize_resumed_messages_noop_for_clean_transcript() { let mut messages = vec![ Message::user("hello"), Message::assistant("hi"), @@ -197,7 +198,7 @@ mod tests { } #[test] - fn sanitize_pairs_tool_use_with_result_and_appends_continuation_sentinel() { + fn sanitize_resumed_messages_pairs_tool_use_with_result_and_appends_continuation_sentinel() { let mut messages = vec![ Message::user("do X"), Message { @@ -229,7 +230,7 @@ mod tests { } #[test] - fn sanitize_drops_orphan_tool_result_block_and_keeps_siblings() { + fn sanitize_resumed_messages_drops_orphan_tool_result_block_and_keeps_siblings() { let mut messages = vec![ Message::user("do X"), Message::assistant("done, no tool needed"), @@ -248,7 +249,7 @@ mod tests { } #[test] - fn sanitize_drops_user_message_with_only_orphan_tool_result() { + fn sanitize_resumed_messages_drops_user_message_with_only_orphan_tool_result() { let mut messages = vec![ Message::user("do X"), Message::assistant("all clear"), @@ -265,7 +266,7 @@ mod tests { } #[test] - fn sanitize_collapses_adjacent_users_after_empty_assistant_drop() { + fn sanitize_resumed_messages_collapses_adjacent_users_after_empty_assistant_drop() { let mut messages = vec![ Message::user("do X"), Message { @@ -286,7 +287,7 @@ mod tests { } #[test] - fn sanitize_collapses_adjacent_assistants_after_orphan_user_drop() { + fn sanitize_resumed_messages_collapses_adjacent_assistants_after_orphan_user_drop() { let mut messages = vec![ Message::user("do X"), Message::assistant("first answer"), @@ -308,7 +309,7 @@ mod tests { } #[test] - fn sanitize_drops_orphan_when_assistant_tool_use_was_dropped() { + fn sanitize_resumed_messages_drops_orphan_when_assistant_tool_use_was_dropped() { // Mismatched ids: pass 1 strips the assistant's unresolved t1, // leaving no surviving ids, so pass 2 drops the user's t2. let mut messages = vec![ @@ -333,7 +334,7 @@ mod tests { } #[test] - fn sanitize_prepends_head_sentinel_when_leading_user_is_dropped() { + fn sanitize_resumed_messages_prepends_head_sentinel_when_leading_user_is_dropped() { let mut messages = vec![ Message { role: Role::User, diff --git a/crates/oxide-code/src/session/state.rs b/crates/oxide-code/src/session/state.rs index 0b557735..99c03f55 100644 --- a/crates/oxide-code/src/session/state.rs +++ b/crates/oxide-code/src/session/state.rs @@ -1,6 +1,7 @@ //! In-memory session state owned by the actor. Pure data with no tokio. I/O hides behind //! [`WriterStatus`] so lifecycle transitions test without a runtime. +use std::path::Path; use std::sync::Arc; use anyhow::{Result, bail}; @@ -12,16 +13,22 @@ use super::entry::{CURRENT_VERSION, Entry, TitleSource}; use super::store::{SessionStore, SessionWriter, encode_lines}; use crate::file_tracker::FileSnapshot; use crate::message::{ContentBlock, Message, Role}; -use crate::util::text::{ELLIPSIS, ELLIPSIS_WIDTH}; +use crate::util::git::current_branch; +use crate::util::text::truncate_to_width; -/// Char cap for first-prompt-derived titles. Sized so a 120-col `--list` row still has room +/// Terminal-column cap for first-prompt titles, leaving room in a 120-col `--list` row /// after the fixed `ID / Last Active / Msgs` prefix. const MAX_TITLE_LEN: usize = 80; -/// Retry-queue size past which a failed flush stops persistence for the rest of the session. -pub(super) const MAX_UNWRITTEN_BYTES: usize = 8 << 20; -const PERSISTENCE_STOPPED: &str = - "session persistence stopped after more than 8 MiB of entries failed to write"; +/// Retry-queue size in MiB past which a failed flush stops persistence for the rest of the session. +const MAX_UNWRITTEN_MIB: usize = 8; +pub(super) const MAX_UNWRITTEN_BYTES: usize = MAX_UNWRITTEN_MIB << 20; + +fn persistence_stopped() -> String { + format!( + "session persistence stopped after more than {MAX_UNWRITTEN_MIB} MiB of entries failed to write" + ) +} // ── SessionState ── @@ -253,7 +260,7 @@ impl SessionState { /// with more than [`MAX_UNWRITTEN_BYTES`] to write drops the queue and stops persistence. pub(super) fn flush_entries(&mut self, entries: Vec) -> Result<()> { if self.persistence_stopped { - bail!(PERSISTENCE_STOPPED); + bail!("{}", persistence_stopped()); } self.unwritten.extend(entries); if self.unwritten.is_empty() { @@ -284,7 +291,7 @@ impl SessionState { if lines.len() > MAX_UNWRITTEN_BYTES { self.persistence_stopped = true; self.unwritten = Vec::new(); - return Err(e.context(PERSISTENCE_STOPPED)); + return Err(e.context(persistence_stopped())); } return Err(e); } @@ -304,7 +311,7 @@ fn new_header(model: &str) -> (String, Entry) { let git_branch = if cfg!(test) { None } else { - crate::util::git::current_branch_str(&cwd) + current_branch(Path::new(&cwd)) }; let header = Entry::Header { session_id: session_id.clone(), @@ -342,30 +349,17 @@ pub(super) fn extract_user_text(message: &Message) -> Option<&str> { }) } -/// Truncates a title to `max_len` characters, appending [`ELLIPSIS`] when truncated. -fn truncate_title(s: &str, max_len: usize) -> String { - debug_assert!( - max_len > ELLIPSIS_WIDTH, - "truncate_title: max_len must exceed ELLIPSIS_WIDTH", - ); - let trimmed = s.lines().next().unwrap_or(s).trim(); - if trimmed.chars().count() <= max_len { - trimmed.to_owned() - } else { - let boundary = trimmed - .char_indices() - .nth(max_len - ELLIPSIS_WIDTH) - .map_or(trimmed.len(), |(i, _)| i); - format!("{}{ELLIPSIS}", &trimmed[..boundary]) - } +/// Truncates the trimmed first line to `max_width` terminal columns. +fn truncate_title(s: &str, max_width: usize) -> String { + truncate_to_width(s.lines().next().unwrap_or(s).trim(), max_width) } #[cfg(test)] mod tests { - use super::super::store::load_session_data_from_path; - use super::super::store::testing::{test_project_dir, test_session_file, test_store}; use super::*; use crate::file_tracker::{FileTracker, LastView}; + use crate::session::store::load_session_data_from_path; + use crate::session::store::testing::{test_project_dir, test_session_file, test_store}; // ── try_defer_title ── @@ -818,13 +812,13 @@ mod tests { let (entries, _) = state.queue_message_entries(&huge, now, false); let err = state.flush_entries(entries).unwrap_err(); assert!( - format!("{err:#}").contains(PERSISTENCE_STOPPED), + format!("{err:#}").contains(&persistence_stopped()), "got: {err:#}" ); let (entries, _) = state.queue_message_entries(&Message::assistant("after"), now, false); let err = state.flush_entries(entries).unwrap_err(); - assert_eq!(err.to_string(), PERSISTENCE_STOPPED); + assert_eq!(err.to_string(), persistence_stopped()); assert!(state.persistence_stopped()); assert_eq!( std::fs::read(&path).unwrap(), @@ -850,7 +844,7 @@ mod tests { // ── extract_user_text ── #[test] - fn extract_user_text_returns_first_non_empty_text_from_user_role_only() { + fn extract_user_text_selects_first_nonempty_user_text() { let tool_result = ContentBlock::ToolResult { tool_use_id: "t1".to_owned(), content: "output".to_owned(), @@ -889,7 +883,7 @@ mod tests { // ── truncate_title ── #[test] - fn truncate_title_returns_at_most_max_len_chars_after_first_line_trim() { + fn truncate_title_trims_first_line_and_caps_terminal_width() { assert_eq!(truncate_title("hello world", 60), "hello world"); assert_eq!(truncate_title("", 60), ""); assert_eq!(truncate_title("first line\nsecond line", 60), "first line"); @@ -903,8 +897,17 @@ mod tests { } #[test] - fn truncate_title_multibyte_respects_character_count() { - // Exact char count: 57 é + "..." = 60, not "<= 60". + fn truncate_title_cjk_title_fits_terminal_column_cap() { + use unicode_width::UnicodeWidthStr; + + let result = truncate_title(&"界".repeat(MAX_TITLE_LEN), MAX_TITLE_LEN); + assert_eq!(result, format!("{}...", "界".repeat(38))); + assert_eq!(result.width(), 79); + } + + #[test] + fn truncate_title_multibyte_respects_terminal_width() { + // Each é is one column, so the cap keeps 57 of them plus "...". let s = "\u{00e9}".repeat(61); let result = truncate_title(&s, 60); assert_eq!(result.chars().count(), 60); diff --git a/crates/oxide-code/src/session/store.rs b/crates/oxide-code/src/session/store.rs index 43d5da16..752d8b1e 100644 --- a/crates/oxide-code/src/session/store.rs +++ b/crates/oxide-code/src/session/store.rs @@ -89,7 +89,7 @@ impl SessionStore { } } -// ── Path-keyed primitives ── +// ── Path-Keyed Primitives ── /// Opens an existing session file in append mode by explicit path. pub(crate) fn open_append_at(path: &Path) -> Result { @@ -337,9 +337,11 @@ pub(crate) fn set_archived_at(path: &Path, archived: bool) -> Result<()> { Ok(()) } +const MAX_SESSION_ID_LEN: usize = 64; + fn validate_session_id(session_id: &str) -> Result<()> { let ok = !session_id.is_empty() - && session_id.len() <= 64 + && session_id.len() <= MAX_SESSION_ID_LEN && session_id .bytes() .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_'); @@ -422,9 +424,9 @@ pub(crate) struct SessionData { // ── File Opening ── -/// Options for a readable handle that [`Appender`] writes through. On Windows, `append(true)` strips -/// the `FILE_WRITE_DATA` access that `set_len` needs to roll back a partial write, so the handle -/// opens for plain writing there and each write targets the end of the file. +/// Options for a readable handle that [`Appender`] writes through. On Windows, `append(true)` +/// strips the `FILE_WRITE_DATA` access that `set_len` needs to roll back a partial write, so +/// the handle opens for plain writing there and each write targets the end of the file. fn append_options() -> OpenOptions { let mut options = OpenOptions::new(); options.read(true); @@ -1482,7 +1484,7 @@ mod tests { } #[tokio::test] - async fn load_session_data_picks_newest_leaf_on_fork() { + async fn load_session_data_resolves_transcript_through_newest_chain_leaf() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); let mut writer = store.create(&sample_header("fork"), &[]).unwrap(); @@ -1535,99 +1537,6 @@ mod tests { ); } - #[tokio::test] - async fn load_session_data_terminates_at_orphan_parent_reference() { - let dir = tempfile::tempdir().unwrap(); - let store = test_store(dir.path()); - let mut writer = store.create(&sample_header("orphan"), &[]).unwrap(); - - let only = Uuid::new_v4(); - writer - .append(&[sample_message_at( - only, - Some(Uuid::new_v4()), // points at a missing ancestor - datetime!(2026-04-16 12:00:01 UTC), - "orphan tip", - )]) - .unwrap(); - drop(writer); - - let data = store.load_session_data("orphan").unwrap(); - assert_eq!(data.last_uuid, Some(only)); - assert_eq!(data.messages.len(), 1); - assert!( - matches!(&data.messages[0].content[0], ContentBlock::Text { text } if text == "orphan tip") - ); - } - - #[tokio::test] - async fn load_session_data_breaks_chain_walk_on_cycle() { - let dir = tempfile::tempdir().unwrap(); - let store = test_store(dir.path()); - let mut writer = store.create(&sample_header("cycle"), &[]).unwrap(); - - let a = Uuid::new_v4(); - let b = Uuid::new_v4(); - writer - .append(&[sample_message_at( - a, - Some(b), // a points at b — part of the cycle - datetime!(2026-04-16 12:00:01 UTC), - "A", - )]) - .unwrap(); - writer - .append(&[sample_message_at( - b, - Some(a), // b points back at a — completes the cycle - datetime!(2026-04-16 12:00:02 UTC), - "B", - )]) - .unwrap(); - drop(writer); - - let data = store.load_session_data("cycle").unwrap(); - assert!( - data.messages.is_empty(), - "cycle with no leaf should yield an empty chain, got: {:?}", - data.messages, - ); - } - - #[tokio::test] - async fn load_session_data_prefers_later_duplicate_uuid() { - let dir = tempfile::tempdir().unwrap(); - let store = test_store(dir.path()); - let mut writer = store.create(&sample_header("dup"), &[]).unwrap(); - - let u = Uuid::new_v4(); - writer - .append(&[sample_message_at( - u, - None, - datetime!(2026-04-16 12:00:01 UTC), - "first copy", - )]) - .unwrap(); - writer - .append(&[sample_message_at( - u, - None, - datetime!(2026-04-16 12:00:05 UTC), - "second copy", - )]) - .unwrap(); - drop(writer); - - let data = store.load_session_data("dup").unwrap(); - assert_eq!(data.last_uuid, Some(u)); - assert_eq!(data.messages.len(), 1); - assert!( - matches!(&data.messages[0].content[0], ContentBlock::Text { text } if text == "second copy"), - "latest duplicate should win" - ); - } - #[test] fn load_session_data_recovers_from_truncated_utf8_at_eof() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/oxide-code/src/session/store/listing.rs b/crates/oxide-code/src/session/store/listing.rs index c796ab2d..42023d07 100644 --- a/crates/oxide-code/src/session/store/listing.rs +++ b/crates/oxide-code/src/session/store/listing.rs @@ -8,11 +8,20 @@ use anyhow::{Context, Result}; use time::OffsetDateTime; use tracing::warn; -use super::super::entry::{Entry, ExitInfo, SessionInfo, TitleInfo}; use super::{HeaderInfo, JsonlLines, SessionStore, file_session_id, read_header}; +use crate::session::entry::{Entry, ExitInfo, SessionInfo, TitleInfo}; pub(crate) const DEFAULT_SESSION_LIST_LIMIT: usize = 30; +// ── ListScope ── + +/// Which sessions a listing walks: every project or only the home project, and archived or active ones. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub(crate) struct ListScope { + pub(crate) all: bool, + pub(crate) archived: bool, +} + // ── ListPage ── /// One page of sessions, and whether more matching sessions follow it. @@ -46,21 +55,16 @@ impl SessionStore { /// Every readable unarchived session in the home project, most recently active first. #[cfg(test)] pub(crate) fn list(&self) -> Result> { - Ok(self.list_paged(None, false, false)?.into_sessions()) + Ok(self.list_paged(None, ListScope::default())?.into_sessions()) } - /// Up to `limit` most recently active readable sessions whose archive state is `archived`, - /// unbounded for `None`. `all` widens the walk from the home project to every project. Archive + /// Up to `limit` most recently active readable sessions matching `scope`, + /// unbounded for `None`. `scope.all` widens the walk from the home project to every project. Archive /// state lives in each file, so the walk parses sessions until it finds one past the page. - pub(crate) fn list_paged( - &self, - limit: Option, - all: bool, - archived: bool, - ) -> Result { + pub(crate) fn list_paged(&self, limit: Option, scope: ListScope) -> Result { let limit = limit.unwrap_or(usize::MAX); - let mut sessions: Vec<_> = read_session_infos(self.session_paths(all)?.into_iter()) - .filter(|info| info.archived == archived) + let mut sessions: Vec<_> = read_session_infos(self.session_paths(scope.all)?.into_iter()) + .filter(|info| info.archived == scope.archived) .take(limit.saturating_add(1)) .collect(); let more = sessions.len() > limit; @@ -225,14 +229,14 @@ mod tests { use time::macros::datetime; use uuid::Uuid; - use super::super::set_archived_at; - use super::super::testing::{ + use super::*; + use crate::session::entry::{CURRENT_VERSION, TitleSource}; + use crate::session::store::set_archived_at; + use crate::session::store::testing::{ sample_header, sample_message_entry, sample_summary_entry, sample_title_entry, seed_test_session, test_project_dir, test_session_file, test_store, write_newest_corrupt_session, }; - use super::*; - use crate::session::entry::{CURRENT_VERSION, TitleSource}; // ── list ── @@ -446,6 +450,24 @@ mod tests { assert_eq!(sessions[0].session_id, "own"); } + #[test] + fn list_missing_project_reports_directory_error() { + let dir = tempfile::tempdir().unwrap(); + let store = test_store(dir.path()); + let project_dir = test_project_dir(dir.path()); + fs::remove_dir(&project_dir).unwrap(); + + let error = store.list().unwrap_err(); + assert_eq!( + error.to_string(), + format!("cannot read {}", project_dir.display()) + ); + assert_eq!( + error.downcast_ref::().unwrap().kind(), + std::io::ErrorKind::NotFound, + ); + } + // ── list_paged ── /// Builds `count` sessions whose mtimes and file names both ascend in seed order. @@ -479,7 +501,7 @@ mod tests { let store = test_store(dir.path()); seed_n_sessions(&store, 10); - let page = store.list_paged(Some(3), false, false).unwrap(); + let page = store.list_paged(Some(3), ListScope::default()).unwrap(); assert_eq!(page.sessions().len(), 3, "cap honored"); assert!(page.has_more(), "seven sessions follow the page"); assert_eq!(page.sessions()[0].session_id, "session-0009"); @@ -487,12 +509,12 @@ mod tests { } #[tokio::test] - async fn list_paged_unbounded_returns_every_session_with_none_more() { + async fn list_paged_unbounded_includes_every_session_without_more() { let dir = tempfile::tempdir().unwrap(); let store = test_store(dir.path()); seed_n_sessions(&store, 4); - let page = store.list_paged(None, false, false).unwrap(); + let page = store.list_paged(None, ListScope::default()).unwrap(); assert_eq!(page.sessions().len(), 4); assert!(!page.has_more()); } @@ -510,7 +532,16 @@ mod tests { .unwrap(); drop(foreign_store); - let all = store.list_paged(None, true, false).unwrap().into_sessions(); + let all = store + .list_paged( + None, + ListScope { + all: true, + ..ListScope::default() + }, + ) + .unwrap() + .into_sessions(); let mut ids: Vec<_> = all.iter().map(|s| s.session_id.as_str()).collect(); ids.sort_unstable(); assert_eq!(ids, vec!["foreign", "own"]); @@ -528,7 +559,13 @@ mod tests { set_archived_at(&path("session-0003"), false).unwrap(); let ids = |archived: bool| -> Vec { let mut ids: Vec<_> = store - .list_paged(None, false, archived) + .list_paged( + None, + ListScope { + archived, + ..ListScope::default() + }, + ) .unwrap() .into_sessions() .into_iter() @@ -540,9 +577,29 @@ mod tests { assert_eq!(ids(false), ["session-0000", "session-0003"]); assert_eq!(ids(true), ["session-0001", "session-0002"]); - assert!(store.list_paged(Some(1), false, true).unwrap().has_more()); assert!( - !store.list_paged(Some(2), false, true).unwrap().has_more(), + store + .list_paged( + Some(1), + ListScope { + archived: true, + ..ListScope::default() + } + ) + .unwrap() + .has_more() + ); + assert!( + !store + .list_paged( + Some(2), + ListScope { + archived: true, + ..ListScope::default() + } + ) + .unwrap() + .has_more(), "active sessions past the page are not more archived ones", ); } @@ -554,7 +611,7 @@ mod tests { seed_n_sessions(&store, 3); write_newest_corrupt_session(dir.path()); - let page = store.list_paged(Some(2), false, false).unwrap(); + let page = store.list_paged(Some(2), ListScope::default()).unwrap(); let ids: Vec<_> = page .sessions() .iter() @@ -563,7 +620,7 @@ mod tests { assert_eq!(ids, ["session-0002", "session-0001"]); assert!(page.has_more()); - let full = store.list_paged(Some(3), false, false).unwrap(); + let full = store.list_paged(Some(3), ListScope::default()).unwrap(); assert_eq!(full.sessions().len(), 3); assert!( !full.has_more(), @@ -578,7 +635,7 @@ mod tests { let store = test_store(dir.path()); seed_n_sessions(&store, 3); - let page = store.list_paged(Some(0), false, false).unwrap(); + let page = store.list_paged(Some(0), ListScope::default()).unwrap(); assert!(page.sessions().is_empty()); assert!(page.has_more()); } @@ -596,7 +653,13 @@ mod tests { fs::create_dir(&locked).unwrap(); fs::set_permissions(&locked, fs::Permissions::from_mode(0o000)).unwrap(); - let page = store.list_paged(None, true, false); + let page = store.list_paged( + None, + ListScope { + all: true, + ..ListScope::default() + }, + ); fs::set_permissions(&locked, fs::Permissions::from_mode(0o700)).unwrap(); let ids: Vec<_> = page diff --git a/crates/oxide-code/src/session/store/testing.rs b/crates/oxide-code/src/session/store/testing.rs index ed6299c2..9d3393ee 100644 --- a/crates/oxide-code/src/session/store/testing.rs +++ b/crates/oxide-code/src/session/store/testing.rs @@ -9,12 +9,12 @@ use time::OffsetDateTime; use time::macros::datetime; use uuid::Uuid; -use super::super::entry::{CURRENT_VERSION, Entry, TitleSource}; use super::{ SessionData, SessionStore, SessionWriter, encode_lines, file_session_id, load_session_data_from_path, open_append_at, }; use crate::message::Message; +use crate::session::entry::{CURRENT_VERSION, Entry, TitleSource}; pub(crate) const TEST_PROJECT: &str = "test-project"; diff --git a/crates/oxide-code/src/session/title_generator.rs b/crates/oxide-code/src/session/title_generator.rs index 4f0f17e1..b3ed18de 100644 --- a/crates/oxide-code/src/session/title_generator.rs +++ b/crates/oxide-code/src/session/title_generator.rs @@ -6,13 +6,15 @@ use anyhow::{Context, Result, bail}; use indoc::indoc; use serde::Deserialize; use tracing::warn; +use unicode_width::UnicodeWidthStr; +use super::handle::SessionHandle; use crate::client::anthropic::Client; use crate::client::anthropic::wire::OutputFormat; use crate::config::Effort; use crate::model::ModelSpec; use crate::protocol::{Event, EventSink}; -use crate::session::handle::SessionHandle; +use crate::util::text::{ELLIPSIS, ELLIPSIS_WIDTH, truncate_to_width}; // ── Constants ── @@ -154,11 +156,11 @@ fn parse_title(response: &str) -> Result { /// Cap a string for log / error messages so long Haiku refusals stay readable. fn truncate_for_log(s: &str) -> String { const LOG_CAP: usize = 120; - if s.chars().count() <= LOG_CAP { - return s.to_owned(); + let prefix: String = s.chars().take(LOG_CAP).collect(); + if prefix.len() == s.len() && s.width() <= LOG_CAP { + return prefix; } - let head: String = s.chars().take(LOG_CAP).collect(); - format!("{head}...") + truncate_to_width(&format!("{prefix}{ELLIPSIS}"), LOG_CAP + ELLIPSIS_WIDTH) } /// Strips a surrounding triple-backtick code fence (with optional language tag). Leaves @@ -198,6 +200,7 @@ mod tests { use crate::config::auth::{Auth, AuthSource}; use crate::message::Message; use crate::protocol::CapturingSink; + use crate::session::handle::testing::{acks_append_ai_title_with_failure, dead}; use crate::session::store::testing::test_store; // ── Fixtures ── @@ -436,10 +439,7 @@ mod tests { .mount(&server) .await; - let session = super::super::handle::testing::acks_append_ai_title_with_failure( - "live-session", - "disk full", - ); + let session = acks_append_ai_title_with_failure("live-session", "disk full"); let client = title_client(server.uri()); let sink = CapturingSink::new(); @@ -472,7 +472,7 @@ mod tests { .mount(&server) .await; - let session = super::super::handle::testing::dead("dead-session"); + let session = dead("dead-session"); let client = title_client(server.uri()); let sink = CapturingSink::new(); @@ -603,10 +603,19 @@ mod tests { #[test] fn truncate_for_log_caps_long_strings_with_ellipsis() { - let long = "a".repeat(500); - let out = truncate_for_log(&long); - assert!(out.ends_with("..."), "got: {out:?}"); - assert_eq!(out.chars().count(), 123, "got: {out:?}"); + let cap = "a".repeat(120); + assert_eq!(truncate_for_log(&cap), cap); + for len in [121, 123, 500] { + assert_eq!(truncate_for_log(&"a".repeat(len)), format!("{cap}...")); + } + } + + #[test] + fn truncate_for_log_caps_multiline_and_zero_width_responses() { + for text in [format!("x{}y", "\n".repeat(500)), "\u{200b}".repeat(500)] { + let expected = format!("{}{ELLIPSIS}", text.chars().take(120).collect::()); + assert_eq!(truncate_for_log(&text), expected); + } } // ── strip_code_fence ── diff --git a/crates/oxide-code/src/slash/resume.rs b/crates/oxide-code/src/slash/resume.rs index 59f10894..e490f688 100644 --- a/crates/oxide-code/src/slash/resume.rs +++ b/crates/oxide-code/src/slash/resume.rs @@ -19,7 +19,7 @@ use crate::session::display::{display_title, format_metadata_line}; use crate::session::entry::SessionInfo; use crate::session::resolver::resolve_prefix_to_info; use crate::session::store::SessionStore; -use crate::session::store::listing::{DEFAULT_SESSION_LIST_LIMIT, ListPage}; +use crate::session::store::listing::{DEFAULT_SESSION_LIST_LIMIT, ListPage, ListScope}; use crate::tui::action::Action; use crate::tui::modal::chrome::{FOOTER_HEIGHT, render_footer}; use crate::tui::modal::searchable_list::{SearchableItem, SearchableList}; @@ -186,9 +186,13 @@ impl ResumePicker { } fn reload(&mut self) { - let result = - self.store - .list_paged(Some(DEFAULT_SESSION_LIST_LIMIT), self.all, self.archived); + let result = self.store.list_paged( + Some(DEFAULT_SESSION_LIST_LIMIT), + ListScope { + all: self.all, + archived: self.archived, + }, + ); let page = match result { Ok(p) => { self.load_error = None; diff --git a/crates/oxide-code/src/util/git.rs b/crates/oxide-code/src/util/git.rs index e8ccbae9..20c1f88d 100644 --- a/crates/oxide-code/src/util/git.rs +++ b/crates/oxide-code/src/util/git.rs @@ -29,11 +29,6 @@ pub(crate) async fn current_branch_async(cwd: &Path) -> Option { parse_branch(&output.stdout) } -/// `&str` overload for callers that already hold a string-shaped cwd. -pub(crate) fn current_branch_str(cwd: &str) -> Option { - current_branch(Path::new(cwd)) -} - fn branch_command(cwd: &str) -> Command { let mut command = Command::new("git"); command.args(["-C", cwd, "--no-optional-locks", "branch", "--show-current"]); @@ -267,15 +262,6 @@ mod tests { ); } - // ── current_branch_str ── - - #[test] - fn current_branch_str_outside_a_repo_is_absent() { - let dir = tempfile::tempdir().unwrap(); - let cwd = dir.path().to_str().unwrap(); - assert_eq!(with_isolated_git_env(|| current_branch_str(cwd)), None); - } - // ── parse_branch ── #[test] @@ -332,7 +318,7 @@ mod tests { // ── cwd_to_str ── #[test] - fn cwd_to_str_returns_path_when_utf8() { + fn cwd_to_str_utf8_path_is_preserved() { assert_eq!(cwd_to_str(Path::new("/tmp/a"), "p"), Some("/tmp/a")); } @@ -346,7 +332,7 @@ mod tests { // ── check_output ── #[test] - fn check_output_returns_output_on_success() { + fn check_output_success_preserves_stdout() { let output = check_output( "test", "/tmp/cwd", @@ -385,8 +371,6 @@ mod tests { } fn status_with_code(code: i32) -> std::process::ExitStatus { - // `Command::new("sh").arg("-c").arg(format!("exit {code}"))` is the portable way to mint - // an `ExitStatus` with a specific code from tests. Command::new("sh") .arg("-c") .arg(format!("exit {code}")) @@ -418,7 +402,7 @@ mod tests { } #[test] - fn stderr_summary_returns_empty_when_every_line_is_blank() { + fn stderr_summary_blank_lines_have_no_summary() { assert_eq!(stderr_summary(b"\n \n\t\n"), ""); } } diff --git a/crates/oxide-code/tests/cli.rs b/crates/oxide-code/tests/cli.rs index dd7a1544..3ea63f40 100644 --- a/crates/oxide-code/tests/cli.rs +++ b/crates/oxide-code/tests/cli.rs @@ -1,8 +1,10 @@ -//! CLI startup and session-list output. +//! CLI startup, headless session persistence, and session-list output. use std::process::{Command, Stdio}; use indoc::indoc; +use wiremock::matchers::{method, path}; +use wiremock::{Mock, MockServer, ResponseTemplate}; #[test] fn list_empty_project() { @@ -95,3 +97,83 @@ fn resume_without_a_terminal_fails_before_writing_anything() { ); assert!(std::fs::read_dir(dir.path()).unwrap().next().is_none()); } + +#[tokio::test] +async fn headless_rejected_request_preserves_branch_and_user_message() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/messages")) + .respond_with(ResponseTemplate::new(400).set_body_json(serde_json::json!({ + "type": "error", + "error": { + "type": "invalid_request_error", + "message": "fixture rejection", + }, + }))) + .expect(1) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let init = Command::new("git") + .args(["init", "--initial-branch", "fixture-branch"]) + .current_dir(dir.path()) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_INDEX_FILE") + .output() + .unwrap(); + assert!(init.status.success(), "{init:?}"); + + let data_dir = dir.path().join("data"); + let output = tokio::process::Command::new(env!("CARGO_BIN_EXE_ox")) + .args(["-p", "hello"]) + .current_dir(dir.path()) + .env("HOME", dir.path()) + .env("XDG_CONFIG_HOME", dir.path().join("config")) + .env("XDG_DATA_HOME", &data_dir) + .env("XDG_STATE_HOME", dir.path().join("state")) + .env("ANTHROPIC_AUTH_TOKEN", "test-token") + .env("ANTHROPIC_BASE_URL", server.uri()) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_INDEX_FILE") + .stdin(Stdio::null()) + .output() + .await + .unwrap(); + + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!(stderr.contains("fixture rejection"), "{stderr}"); + + let projects: Vec<_> = std::fs::read_dir(data_dir.join("ox/sessions")) + .unwrap() + .map(|entry| entry.unwrap().path()) + .collect(); + assert_eq!(projects.len(), 1, "session projects: {projects:?}"); + let sessions: Vec<_> = std::fs::read_dir(&projects[0]) + .unwrap() + .map(|entry| entry.unwrap().path()) + .collect(); + assert_eq!(sessions.len(), 1, "session files: {sessions:?}"); + let transcript = std::fs::read_to_string(&sessions[0]).unwrap(); + let entries: Vec = transcript + .lines() + .map(|line| serde_json::from_str(line).unwrap()) + .collect(); + assert_eq!(entries[0]["type"], "header"); + assert_eq!(entries[0]["git_branch"], "fixture-branch"); + let user_messages: Vec<_> = entries + .iter() + .filter(|entry| entry["type"] == "message" && entry["message"]["role"] == "user") + .map(|entry| entry["message"].clone()) + .collect(); + assert_eq!( + user_messages, + vec![serde_json::json!({ + "role": "user", + "content": [{"type": "text", "text": "hello"}], + })], + ); +} diff --git a/docs/design/slash/resume.md b/docs/design/slash/resume.md index b207f9e1..4628d469 100644 --- a/docs/design/slash/resume.md +++ b/docs/design/slash/resume.md @@ -51,7 +51,7 @@ The picker wraps [`SearchableList`](modals.md) and adds these controls and displ 12. **Default to a 30-row page with no background load-more.** Codex's near-bottom prefetch is genuinely nice but adds a non-trivial state machine for marginal gain at typical session counts. More than 30 sessions in a project is search territory anyway. -13. **Listing API centralizes pagination in `list_paged(limit, all, archived)`.** The page, the archive filter, and whether more sessions follow come from one walk that stops one session past the page, so a listing's cost scales with the page. Listings report no exact total, since counting the sessions in one archive state would parse every file in scope. Bare `ox -c` stops at the first unarchived session through `latest`, and a prefix lookup parses only matching files. +13. **Listing API centralizes pagination in `list_paged(limit, scope)`.** The page, the archive filter, and whether more sessions follow come from one walk that stops one session past the page, so a listing's cost scales with the page. Listings report no exact total, since counting the sessions in one archive state would parse every file in scope. Bare `ox -c` stops at the first unarchived session through `latest`, and a prefix lookup parses only matching files. 14. **`--list --limit 0` opts back into unbounded listing.** Scripts piping output need the full set, so `0` is the explicit unbounded value and no magic large default is needed. @@ -73,7 +73,7 @@ The picker wraps [`SearchableList`](modals.md) and adds these controls and displ - **`SessionRow`**: Carries the 8-char id prefix, full id, session file path, title-or-`(untitled)`, `last_active_at`, message count, optional git branch, and (in all-projects mode) the tildified project path. `haystack` joins id + title plus project when visible. `render` paints a two-line title plus dim metadata block under a fixed width budget. The metadata line is built via the shared `session::display::format_metadata_line` helper so the confirm-delete modal can reuse the same shape. -- **`SessionStore::list_paged`**: Stats every candidate, sorts by mtime desc, then parses files in order, keeping readable sessions in the requested archive state until it holds one past the page. `ListPage::has_more()` reports whether a matching session follows the page, for the `--list` footer and the picker's `more not loaded` note. +- **`SessionStore::list_paged`**: Takes a `ListScope` with project and archive flags. Stats every candidate, sorts by mtime desc, then parses files in order, keeping readable sessions in the requested archive state until it holds one past the page. `ListPage::has_more()` reports whether a matching session follows the page, for the `--list` footer and the picker's `more not loaded` note. - **`/resume ` resolver**: `resolve_prefix_to_info` runs the prefix lookup against the current project, then retries across all projects on miss. Returns the matched session or a user-readable error. @@ -109,7 +109,7 @@ The picker wraps [`SearchableList`](modals.md) and adds these controls and displ - `crates/oxide-code/src/session/history.rs`: `interactions`, the display projection `SessionResumed` carries. - `crates/oxide-code/src/session/list_view.rs`: `render_list` over `list_paged`. - `crates/oxide-code/src/session/resolver.rs`: `resolve_prefix_to_info` and its `match_in_scope` helper, shared with `/delete`. -- `crates/oxide-code/src/session/store/listing.rs`: `list_paged`, `latest`, `list_matching`, `ListPage`. +- `crates/oxide-code/src/session/store/listing.rs`: `list_paged`, `latest`, `list_matching`, `ListPage`, `ListScope`. - `crates/oxide-code/src/slash/confirm.rs`: `ConfirmDeleteSessionModal` pushed by Ctrl+D. - `crates/oxide-code/src/slash/registry.rs`: `BUILT_INS` entry `&ResumeCmd`. - `crates/oxide-code/src/slash/resume.rs`: `ResumeCmd`, `ResumePicker` and its `at_startup` variant, `SessionRow`, and `resolve_prefix`. From 98b4cd1e1cc7d621108b089ace4ad66ba5d825b2 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 15:22:06 +0800 Subject: [PATCH 5/5] docs(session): rewrap listing and actor docs to 100 columns --- crates/oxide-code/src/main.rs | 2 +- crates/oxide-code/src/session/actor.rs | 4 ++-- crates/oxide-code/src/session/store/listing.rs | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/oxide-code/src/main.rs b/crates/oxide-code/src/main.rs index d7dafb04..a8563363 100644 --- a/crates/oxide-code/src/main.rs +++ b/crates/oxide-code/src/main.rs @@ -412,7 +412,7 @@ mod tests { let dir = canonical_tempdir(); std::fs::write(dir.path().join("notes.txt"), "token").unwrap(); std::fs::write(dir.path().join("secret.txt"), "token").unwrap(); - // An absolute rule, since the tools search from the process cwd, which the dir sits outside. + // An absolute rule, since the tools search from the process cwd, which is outside the dir. let deny = [format!("read({}/secret.txt)", slash_separated(dir.path()))]; let read_deny = Policy::resolve(Mode::Auto, &[], &deny, dir.path()) .unwrap() diff --git a/crates/oxide-code/src/session/actor.rs b/crates/oxide-code/src/session/actor.rs index 3a300db6..ff1142de 100644 --- a/crates/oxide-code/src/session/actor.rs +++ b/crates/oxide-code/src/session/actor.rs @@ -245,8 +245,8 @@ impl Batch { } } - /// Queues one sidecar per item, skipping default metadata since it adds no display fields. Acks at - /// once when nothing needs a flush. + /// Queues one sidecar per item, skipping default metadata since it adds no display fields. + /// Acks at once when nothing needs a flush. fn absorb_tool_metadata( &mut self, items: Vec<(String, ToolMetadata)>, diff --git a/crates/oxide-code/src/session/store/listing.rs b/crates/oxide-code/src/session/store/listing.rs index 42023d07..8c931c4b 100644 --- a/crates/oxide-code/src/session/store/listing.rs +++ b/crates/oxide-code/src/session/store/listing.rs @@ -15,7 +15,7 @@ pub(crate) const DEFAULT_SESSION_LIST_LIMIT: usize = 30; // ── ListScope ── -/// Which sessions a listing walks: every project or only the home project, and archived or active ones. +/// Which sessions a listing walks: the home project or every project, and active or archived ones. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub(crate) struct ListScope { pub(crate) all: bool, @@ -58,9 +58,9 @@ impl SessionStore { Ok(self.list_paged(None, ListScope::default())?.into_sessions()) } - /// Up to `limit` most recently active readable sessions matching `scope`, - /// unbounded for `None`. `scope.all` widens the walk from the home project to every project. Archive - /// state lives in each file, so the walk parses sessions until it finds one past the page. + /// Up to `limit` most recently active readable sessions matching `scope`, unbounded for `None`. + /// `scope.all` widens the walk from the home project to every project. Archive state lives in + /// each file, so the walk parses sessions until it finds one past the page. pub(crate) fn list_paged(&self, limit: Option, scope: ListScope) -> Result { let limit = limit.unwrap_or(usize::MAX); let mut sessions: Vec<_> = read_session_infos(self.session_paths(scope.all)?.into_iter())