travelagent-core 1.10.3

use directories::ProjectDirs;
use std::cell::RefCell;
use std::fs;
use std::path::{Path, PathBuf};
use std::time::{Duration, SystemTime};

use crate::config::SessionGcConfig;
use crate::error::{Result, TrvError};
use crate::model::ReviewSession;
use crate::model::review::{SESSION_VERSION, SessionDiffSource};
const SESSION_FILENAME_DELIMITER: &str = "--";
/// Expected component count when splitting a v2 session filename stem on the
/// delimiter: `{name}--{hex8}--{branch}--{diff_source}--{YYYYMMDD_HHMMSS}--{id}`.
const SESSION_FILENAME_PART_COUNT: usize = 6;
const SESSION_FILENAME_DATE_LEN: usize = 8;
const SESSION_FILENAME_TIME_LEN: usize = 6;
const FINGERPRINT_HEX_LEN: usize = 8;

struct SessionFilenameParts {
    repo_fingerprints: Vec<String>,
    diff_source: String,
}

/// Parse a session filename.
///
/// The canonical format uses `--` (double-dash) as the major delimiter so that
/// `diff_source` and any other component may safely contain single underscores
/// (e.g. `staged_and_unstaged`). Returns `None` for filenames that do not
/// conform to the current format — callers can treat this as "unknown
/// provenance" and decide whether to include the file when filtering.
fn parse_session_filename(filename: &str) -> Option<SessionFilenameParts> {
    let stem = filename.strip_suffix(".json")?;
    let parts: Vec<&str> = stem.split(SESSION_FILENAME_DELIMITER).collect();

    if parts.len() != SESSION_FILENAME_PART_COUNT {
        return None;
    }

    let repo_fingerprint = parts[1];
    let diff_source = parts[3];
    let date_part = parts[4].get(..SESSION_FILENAME_DATE_LEN)?;
    let time_part = parts[4].get(SESSION_FILENAME_DATE_LEN + 1..)?;

    if !matches!(
        diff_source,
        "worktree"
            | "staged"
            | "unstaged"
            | "staged_and_unstaged"
            | "commits"
            | "worktree_and_commits"
            | "staged_unstaged_and_commits"
            | "remote"
    ) {
        return None;
    }

    // Timestamp layout is `YYYYMMDD_HHMMSS`.
    if parts[4].len() != SESSION_FILENAME_DATE_LEN + 1 + SESSION_FILENAME_TIME_LEN
        || parts[4].as_bytes().get(SESSION_FILENAME_DATE_LEN) != Some(&b'_')
        || !is_timestamp_part(date_part, SESSION_FILENAME_DATE_LEN)
        || !is_timestamp_part(time_part, SESSION_FILENAME_TIME_LEN)
    {
        return None;
    }

    if !is_hex_fingerprint(repo_fingerprint) {
        return None;
    }

    Some(SessionFilenameParts {
        repo_fingerprints: vec![repo_fingerprint.to_string()],
        diff_source: diff_source.to_string(),
    })
}

fn is_timestamp_part(part: &str, len: usize) -> bool {
    part.len() == len && part.chars().all(|ch| ch.is_ascii_digit())
}

fn is_hex_fingerprint(part: &str) -> bool {
    part.len() == FINGERPRINT_HEX_LEN && part.chars().all(|ch| ch.is_ascii_hexdigit())
}

thread_local! {
    /// Per-thread override for [`get_reviews_dir`]. Tests use
    /// [`with_reviews_dir_override`] instead of mutating `std::env`, which would
    /// be `unsafe` and racy under Rust 2024's tightened env-var model.
    static REVIEWS_DIR_OVERRIDE: RefCell<Option<PathBuf>> = const { RefCell::new(None) };
}

/// Run `f` with `dir` installed as the thread-local reviews directory.
///
/// Compiled into test builds (via `cfg(test)`) and into the
/// `test-support` feature so integration tests in sibling crates can
/// redirect the reviews directory without spawning a fresh process. The
/// override is thread-local to avoid cross-test interference.
#[cfg(any(test, feature = "test-support"))]
pub fn set_reviews_dir_override(dir: PathBuf) {
    REVIEWS_DIR_OVERRIDE.with(|cell| {
        *cell.borrow_mut() = Some(dir);
    });
}

#[cfg(any(test, feature = "test-support"))]
pub fn clear_reviews_dir_override() {
    REVIEWS_DIR_OVERRIDE.with(|cell| {
        *cell.borrow_mut() = None;
    });
}

fn get_reviews_dir() -> Result<PathBuf> {
    if let Some(override_dir) = REVIEWS_DIR_OVERRIDE.with(|cell| cell.borrow().clone()) {
        fs::create_dir_all(&override_dir)?;
        return Ok(override_dir);
    }

    let proj_dirs = ProjectDirs::from("", "", "travelagent")
        .ok_or_else(|| TrvError::Io(std::io::Error::other("Could not determine data directory")))?;

    let data_dir = proj_dirs.data_dir().join("reviews");
    fs::create_dir_all(&data_dir)?;
    Ok(data_dir)
}

const MAX_FILENAME_COMPONENT_LEN: usize = 64;

fn sanitize_filename_component(value: &str) -> String {
    let mut sanitized = String::with_capacity(value.len().min(MAX_FILENAME_COMPONENT_LEN));
    for ch in value.chars() {
        if sanitized.len() >= MAX_FILENAME_COMPONENT_LEN {
            break;
        }
        let ok = ch.is_ascii_alphanumeric() || matches!(ch, '-' | '_' | '.');
        sanitized.push(if ok { ch } else { '-' });
    }

    let sanitized = sanitized.trim_matches('-');
    if sanitized.is_empty() {
        "unknown".to_string()
    } else {
        sanitized.to_string()
    }
}

fn fnv1a_64(bytes: &[u8]) -> u64 {
    const OFFSET_BASIS: u64 = 0xcbf29ce484222325;
    const PRIME: u64 = 0x100000001b3;

    let mut hash = OFFSET_BASIS;
    for byte in bytes {
        hash ^= u64::from(*byte);
        hash = hash.wrapping_mul(PRIME);
    }
    hash
}

fn repo_path_fingerprint(repo_path: &Path) -> String {
    let normalized = normalize_repo_path(repo_path);
    let hash = fnv1a_64(normalized.as_bytes());
    let hex = format!("{hash:016x}");
    hex[..FINGERPRINT_HEX_LEN].to_string()
}

fn normalize_repo_path(repo_path: &Path) -> String {
    let canonical = fs::canonicalize(repo_path).unwrap_or_else(|_| repo_path.to_path_buf());
    let normalized = canonical.to_string_lossy().to_string();

    if cfg!(windows) {
        normalized.to_lowercase()
    } else {
        normalized
    }
}

fn diff_source_slug(diff_source: SessionDiffSource) -> &'static str {
    match diff_source {
        SessionDiffSource::WorkingTree => "worktree",
        SessionDiffSource::Staged => "staged",
        SessionDiffSource::Unstaged => "unstaged",
        SessionDiffSource::StagedAndUnstaged => "staged_and_unstaged",
        SessionDiffSource::CommitRange => "commits",
        SessionDiffSource::WorkingTreeAndCommits => "worktree_and_commits",
        SessionDiffSource::StagedUnstagedAndCommits => "staged_unstaged_and_commits",
        SessionDiffSource::Remote => "remote",
    }
}

fn session_filename(session: &ReviewSession) -> String {
    let repo_name = session
        .repo_path
        .file_name()
        .and_then(|n| n.to_str())
        .unwrap_or("unknown");

    let repo_name = sanitize_filename_component(repo_name);
    let repo_fingerprint = repo_path_fingerprint(&session.repo_path);

    let branch = session.branch_name.as_deref().unwrap_or("detached");
    let branch = sanitize_filename_component(branch);

    let diff_source = diff_source_slug(session.diff_source);

    let timestamp = session.created_at.format("%Y%m%d_%H%M%S");
    let id_fragment = session.id.split('-').next().unwrap_or(&session.id);

    format!(
        "{repo_name}{SESSION_FILENAME_DELIMITER}{repo_fingerprint}{SESSION_FILENAME_DELIMITER}{branch}{SESSION_FILENAME_DELIMITER}{diff_source}{SESSION_FILENAME_DELIMITER}{timestamp}{SESSION_FILENAME_DELIMITER}{id_fragment}.json",
    )
}

pub fn save_session(session: &ReviewSession) -> Result<PathBuf> {
    let reviews_dir = get_reviews_dir()?;
    let filename = session_filename(session);
    let path = reviews_dir.join(&filename);

    // Write atomically: emit the serialized JSON to `<target>.tmp` on the
    // same directory, then `fs::rename` it into place. Rename is atomic on
    // the same filesystem on POSIX and on modern Windows NTFS, so a crash
    // mid-write leaves either the old contents or the full new contents —
    // never a half-written file.
    if let Some(parent) = path.parent() {
        fs::create_dir_all(parent)?;
    }
    let tmp_path = {
        let mut p = path.clone().into_os_string();
        p.push(".tmp");
        PathBuf::from(p)
    };

    // Compact JSON (Phase B): pretty-printing was ~30% overhead on large
    // sessions. Sessions are machine-written / machine-read only; the
    // `--debug-session` escape hatch (pending) exposes pretty output
    // when needed. The single-line JSON still round-trips losslessly.
    let json = save_session_to_string(session)?;
    fs::write(&tmp_path, json)?;
    if let Err(err) = fs::rename(&tmp_path, &path) {
        // Best-effort cleanup so a failed rename does not leave stray
        // `<target>.tmp` files littering the reviews directory.
        let _ = fs::remove_file(&tmp_path);
        return Err(err.into());
    }

    Ok(path)
}

/// Serialize a session to its on-disk JSON form without touching the
/// filesystem. Symmetric counterpart to [`load_session_from_str`]; exists
/// so property/fuzz tests can round-trip through the serde boundary.
pub fn save_session_to_string(session: &ReviewSession) -> Result<String> {
    Ok(serde_json::to_string(session)?)
}

pub fn load_session(path: &PathBuf) -> Result<ReviewSession> {
    let contents = fs::read_to_string(path)?;
    load_session_from_str(&contents)
}

/// Parse a session body from JSON without touching the filesystem. Runs
/// the same version check + tour / legacy-marker migrations as
/// [`load_session`]. Exists so property/fuzz tests can drive the loader
/// with arbitrary inputs.
///
/// Invariant under test: every input either returns `Err(TrvError)` or
/// `Ok(ReviewSession)` with zero panics — malformed JSON, unsupported
/// version strings, and future-versioned tours all flow through
/// `TrvError::CorruptedSession` with a caller-facing message.
pub fn load_session_from_str(contents: &str) -> Result<ReviewSession> {
    let mut session: ReviewSession =
        serde_json::from_str(contents).map_err(|e| TrvError::CorruptedSession(e.to_string()))?;

    // Refuse to load sessions written by a newer schema. Parse both versions
    // as dotted numeric tuples so "1.10" correctly compares as newer than
    // "1.2"; malformed versions are treated as incompatible and rejected.
    let file_version = parse_version(&session.version).ok_or_else(|| {
        TrvError::CorruptedSession(format!(
            "session version {} is not a valid dotted numeric version",
            session.version
        ))
    })?;
    let supported_version = parse_version(SESSION_VERSION)
        .expect("SESSION_VERSION constant must be a valid dotted numeric version");
    if file_version > supported_version {
        return Err(TrvError::CorruptedSession(format!(
            "session version {} is newer than supported {}",
            session.version, SESSION_VERSION
        )));
    }

    // Roll the tour forward through any pending schema migrations (H11).
    // Independent of `SESSION_VERSION` so a tour-shape change doesn't
    // force a whole-session version bump. A future-versioned tour is
    // treated as a corrupted session so the user sees the same refusal
    // they'd get for a newer `SESSION_VERSION` — don't silently interpret
    // unknown shape.
    if let Some(tour) = session.tour.as_mut() {
        crate::model::tour::migrate_tour(tour)
            .map_err(|e| TrvError::CorruptedSession(e.to_string()))?;
    }

    // Migrate 1.3-era agent comments. The legacy marker is idempotent so
    // running this on a 1.4-native session is a no-op: only bodies that
    // still end with the exact marker are rewritten.
    migrate_legacy_mcp_markers_in_place(&mut session);

    Ok(session)
}

/// Strip the legacy `\n\n_(via MCP agent)_` suffix from every comment in
/// `session` and promote `author_kind` to `McpAgent`. Idempotent.
fn migrate_legacy_mcp_markers_in_place(session: &mut ReviewSession) {
    use crate::model::comment::migrate_legacy_mcp_author_marker;
    for comment in session.review_comments.iter_mut() {
        migrate_legacy_mcp_author_marker(comment);
    }
    for file in session.files.values_mut() {
        for comment in file.file_comments.iter_mut() {
            migrate_legacy_mcp_author_marker(comment);
        }
        for comments in file.line_comments.values_mut() {
            for comment in comments.iter_mut() {
                migrate_legacy_mcp_author_marker(comment);
            }
        }
        for comment in file.orphaned_comments.iter_mut() {
            migrate_legacy_mcp_author_marker(comment);
        }
    }
}

/// Parse a dotted numeric version string like `"1.10"` into a vector of u32
/// components that can be compared numerically. Returns `None` for empty
/// strings or any component that fails to parse as `u32`.
fn parse_version(s: &str) -> Option<Vec<u32>> {
    if s.is_empty() {
        return None;
    }
    s.split('.').map(|part| part.parse::<u32>().ok()).collect()
}

/// Summary of a single GC pass over the reviews directory. Zero-valued
/// counters mean "that bound didn't trigger" rather than "the scan failed";
/// `scanned` is the total number of `.json` entries inspected before any
/// deletions.
#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub struct GcReport {
    pub scanned: usize,
    pub removed_age: usize,
    pub removed_size: usize,
    pub removed_count: usize,
    pub remaining_files: usize,
    pub remaining_bytes: u64,
}

impl GcReport {
    pub fn total_removed(&self) -> usize {
        self.removed_age + self.removed_size + self.removed_count
    }
}

/// Run session GC against a specific directory with the given config. Used
/// by the transparent per-load purge (age-only defaults) and by
/// `run_session_gc` (all three bounds). `dry_run` counts what would be
/// removed without mutating the filesystem.
///
/// Errors during deletion or metadata inspection are logged but never
/// abort the scan; clock skew (mtime in the future) is ignored so a
/// freshly-written file is not accidentally purged.
fn purge_sessions(reviews_dir: &Path, cfg: &SessionGcConfig, dry_run: bool) -> GcReport {
    let mut report = GcReport::default();

    let Ok(read_dir) = fs::read_dir(reviews_dir) else {
        return report;
    };

    struct Entry {
        path: PathBuf,
        modified: SystemTime,
        size: u64,
    }

    let mut entries: Vec<Entry> = Vec::new();
    for entry in read_dir.flatten() {
        let path = entry.path();
        if !path
            .extension()
            .and_then(|ext| ext.to_str())
            .is_some_and(|ext| ext.eq_ignore_ascii_case("json"))
        {
            continue;
        }

        let metadata = match entry.metadata() {
            Ok(m) => m,
            Err(err) => {
                eprintln!(
                    "travelagent: failed to stat {} during session GC: {err}",
                    path.display()
                );
                continue;
            }
        };
        let modified = match metadata.modified() {
            Ok(t) => t,
            Err(err) => {
                eprintln!(
                    "travelagent: failed to read mtime for {} during session GC: {err}",
                    path.display()
                );
                continue;
            }
        };
        entries.push(Entry {
            path,
            modified,
            size: metadata.len(),
        });
    }
    report.scanned = entries.len();

    let remove = |path: &Path| -> bool {
        if dry_run {
            return true;
        }
        match fs::remove_file(path) {
            Ok(()) => true,
            Err(err) => {
                eprintln!(
                    "travelagent: failed to remove session {}: {err}",
                    path.display()
                );
                false
            }
        }
    };

    if cfg.max_age_days > 0 {
        // saturating_mul mirrors the size-pass cap below; prevents silent
        // wrap on pathological user values from producing a tiny max_age
        // that would evict everything.
        let max_age = Duration::from_secs(cfg.max_age_days.saturating_mul(24 * 60 * 60));
        let now = SystemTime::now();
        entries.retain(|e| match now.duration_since(e.modified) {
            Ok(age) if age > max_age => {
                if remove(&e.path) {
                    report.removed_age += 1;
                    false
                } else {
                    // Couldn't delete; keep it so downstream bounds still
                    // reason about it accurately.
                    true
                }
            }
            // Fresh file or clock-skewed mtime in the future. Leave it alone.
            Ok(_) | Err(_) => true,
        });
    }

    // Oldest first for subsequent size / count passes so we evict the least-
    // recently-used files.
    entries.sort_by_key(|e| e.modified);

    if cfg.max_size_mb > 0 {
        let cap = cfg.max_size_mb.saturating_mul(1024 * 1024);
        let mut total: u64 = entries.iter().map(|e| e.size).sum();
        let mut kept = Vec::with_capacity(entries.len());
        for entry in entries {
            if total > cap {
                let size = entry.size;
                if remove(&entry.path) {
                    report.removed_size += 1;
                    total = total.saturating_sub(size);
                    continue;
                }
                // Delete failed: subtract the size anyway so the next
                // iteration's `total > cap` check reflects the files we can
                // actually evict, not ghosts we can't. Keep the entry in
                // `kept` so the count pass still sees it.
                total = total.saturating_sub(size);
            }
            kept.push(entry);
        }
        entries = kept;
    }

    if cfg.max_count > 0 {
        // `max_count` keeps the newest N — entries are currently oldest
        // first, so drop from the front.
        let cap = cfg.max_count as usize;
        if entries.len() > cap {
            let drop_n = entries.len() - cap;
            let mut kept = Vec::with_capacity(cap);
            for (i, entry) in entries.into_iter().enumerate() {
                if i < drop_n && remove(&entry.path) {
                    report.removed_count += 1;
                    continue;
                }
                kept.push(entry);
            }
            entries = kept;
        }
    }

    report.remaining_files = entries.len();
    report.remaining_bytes = entries.iter().map(|e| e.size).sum();
    report
}

/// Public entry point for explicit `trv --session-gc` invocations. Returns
/// the full [`GcReport`] so the caller can print a human-readable summary.
pub fn run_session_gc(cfg: &SessionGcConfig, dry_run: bool) -> Result<GcReport> {
    let reviews_dir = get_reviews_dir()?;
    Ok(purge_sessions(&reviews_dir, cfg, dry_run))
}

pub fn load_latest_session_for_context(
    repo_path: &Path,
    branch_name: Option<&str>,
    head_commit: &str,
    diff_source: SessionDiffSource,
    commit_range: Option<&[String]>,
) -> Result<Option<(PathBuf, ReviewSession)>> {
    let current_repo_path = normalize_repo_path(repo_path);
    let current_fingerprint = repo_path_fingerprint(repo_path);
    let current_diff_source = diff_source_slug(diff_source);

    let reviews_dir = get_reviews_dir()?;
    // Transparent startup GC uses SessionGcConfig defaults (age-only,
    // preserving historical behaviour). Explicit `trv --session-gc`
    // runs go through `run_session_gc` with the effective config.
    let _ = purge_sessions(&reviews_dir, &SessionGcConfig::default(), false);

    // Collect `(path, mtime)` pairs once so the subsequent sort does not re-stat
    // every entry. Files that survive the filename filter but whose metadata we
    // cannot read fall back to `UNIX_EPOCH` — they sort last and will be tried
    // only if nothing else matches.
    let mut session_files: Vec<(PathBuf, SystemTime)> = fs::read_dir(&reviews_dir)?
        .filter_map(std::result::Result::ok)
        .filter_map(|entry| {
            let path = entry.path();

            if !path
                .extension()
                .and_then(|ext| ext.to_str())
                .is_some_and(|ext| ext.eq_ignore_ascii_case("json"))
            {
                return None;
            }

            let filename = path.file_name().and_then(|f| f.to_str())?;

            if let Some(parts) = parse_session_filename(filename) {
                if !parts
                    .repo_fingerprints
                    .iter()
                    .any(|fingerprint| fingerprint == &current_fingerprint)
                {
                    return None;
                }

                if parts.diff_source != current_diff_source {
                    return None;
                }
            }
            // Filenames that do not parse (e.g. legacy single-dash files) are
            // kept and fully verified by loading their JSON below.

            let modified = entry
                .metadata()
                .and_then(|m| m.modified())
                .unwrap_or(SystemTime::UNIX_EPOCH);
            Some((path, modified))
        })
        .collect();

    // Newest first; tiebreak on path so results are deterministic when two
    // files share an mtime (common on coarse-grained filesystems).
    session_files.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(&b.0)));

    let mut legacy_candidate = None;

    for (path, _modified) in session_files {
        let Ok(session) = load_session(&path) else {
            continue;
        };

        if normalize_repo_path(&session.repo_path) != current_repo_path {
            continue;
        }

        if session.diff_source != diff_source {
            continue;
        }

        if matches!(
            diff_source,
            SessionDiffSource::CommitRange
                | SessionDiffSource::WorkingTreeAndCommits
                | SessionDiffSource::StagedUnstagedAndCommits
        ) && let Some(expected_range) = commit_range
            && session.commit_range.as_deref() != Some(expected_range)
        {
            continue;
        }

        let session_branch = session.branch_name.as_deref();
        if session_branch == branch_name {
            if branch_name.is_none() && session.base_commit != head_commit {
                continue;
            }

            // Files are iterated newest-first, so the first full match is the
            // winner and we can stop parsing the rest of the directory.
            return Ok(Some((path, session)));
        }

        let eligible_legacy = branch_name.is_some()
            && legacy_candidate.is_none()
            && commit_range.is_none()
            && session_branch.is_none()
            && session.base_commit == head_commit;
        if eligible_legacy {
            legacy_candidate = Some((path, session));
        }
    }

    Ok(legacy_candidate)
}

#[cfg(test)]
fn delete_session(path: &PathBuf) -> Result<()> {
    fs::remove_file(path)?;
    Ok(())
}

#[cfg(test)]
mod tests {
    use super::*;
    use crate::model::FileStatus;
    use std::path::PathBuf;
    use std::time::Duration;

    const TEST_MTIME_RETRIES: usize = 40;
    const TEST_MTIME_SLEEP_MS: u64 = 100;

    fn create_test_session() -> ReviewSession {
        let mut session = ReviewSession::new(
            PathBuf::from("/tmp/test-repo"),
            "abc1234def".to_string(),
            Some("main".to_string()),
            SessionDiffSource::WorkingTree,
        );
        session.add_file(PathBuf::from("src/main.rs"), FileStatus::Modified);
        session
    }

    /// RAII guard that installs a fresh per-test reviews directory on the
    /// current thread and wipes it on drop. Tests run on separate threads
    /// under `cargo test`, so the thread-local override gives us isolation
    /// without the `unsafe` env-var mutation the previous implementation
    /// relied on.
    struct TestReviewsDirGuard {
        path: PathBuf,
    }

    impl TestReviewsDirGuard {
        fn new() -> Self {
            let path =
                std::env::temp_dir().join(format!("trv-reviews-test-{}", uuid::Uuid::new_v4()));
            fs::create_dir_all(&path).unwrap();
            set_reviews_dir_override(path.clone());
            Self { path }
        }
    }

    impl Drop for TestReviewsDirGuard {
        fn drop(&mut self) {
            clear_reviews_dir_override();
            let _ = fs::remove_dir_all(&self.path);
        }
    }

    fn with_test_reviews_dir() -> TestReviewsDirGuard {
        TestReviewsDirGuard::new()
    }

    fn create_session(
        repo_path: PathBuf,
        base_commit: &str,
        branch_name: Option<&str>,
        diff_source: SessionDiffSource,
        commit_range: Option<Vec<String>>,
    ) -> ReviewSession {
        let mut session = ReviewSession::new(
            repo_path,
            base_commit.to_string(),
            branch_name.map(std::string::ToString::to_string),
            diff_source,
        );
        session.commit_range = commit_range;
        session.add_file(PathBuf::from("src/main.rs"), FileStatus::Modified);
        session
    }

    fn save_legacy_session(reviews_dir: &Path, session: &ReviewSession) -> PathBuf {
        let mut value = serde_json::to_value(session).unwrap();
        let obj = value.as_object_mut().unwrap();
        obj.remove("branch_name");
        obj.remove("diff_source");
        obj.remove("commit_range");
        obj.insert(
            "version".to_string(),
            serde_json::Value::String("1.0".to_string()),
        );

        let id_fragment = session.id.split('-').next().unwrap_or(&session.id);
        let path = reviews_dir.join(format!("legacy_{id_fragment}.json"));
        fs::write(&path, serde_json::to_string_pretty(&value).unwrap()).unwrap();
        path
    }

    fn ensure_newer_mtime(newer: &Path, older: &Path) {
        let older_time = fs::metadata(older)
            .ok()
            .and_then(|m| m.modified().ok())
            .unwrap_or(std::time::SystemTime::UNIX_EPOCH);

        for _ in 0..TEST_MTIME_RETRIES {
            let newer_time = fs::metadata(newer)
                .ok()
                .and_then(|m| m.modified().ok())
                .unwrap_or(std::time::SystemTime::UNIX_EPOCH);

            if newer_time > older_time {
                return;
            }

            std::thread::sleep(Duration::from_millis(TEST_MTIME_SLEEP_MS));
            let contents = fs::read_to_string(newer).unwrap();
            fs::write(newer, contents).unwrap();
        }

        let newer_time = fs::metadata(newer)
            .ok()
            .and_then(|m| m.modified().ok())
            .unwrap_or(std::time::SystemTime::UNIX_EPOCH);

        assert!(
            newer_time > older_time,
            "failed to produce newer mtime for {}",
            newer.display()
        );
    }

    #[test]
    fn should_generate_correct_filename() {
        let session = create_test_session();
        let filename = session_filename(&session);
        assert!(filename.starts_with("test-repo--"));
        assert!(filename.contains("--main--worktree--"));
        assert!(filename.ends_with(".json"));
    }

    #[test]
    fn should_generate_filename_for_staged_unstaged() {
        let session = create_session(
            PathBuf::from("/tmp/test-repo"),
            "abc1234def",
            Some("main"),
            SessionDiffSource::StagedAndUnstaged,
            None,
        );
        let filename = session_filename(&session);
        assert!(filename.contains("--staged_and_unstaged--"));
    }

    #[test]
    fn should_roundtrip_session() {
        let _guard = with_test_reviews_dir();
        let session = create_test_session();
        let path = save_session(&session).unwrap();
        let loaded = load_session(&path).unwrap();
        assert_eq!(session.id, loaded.id);
        assert_eq!(session.base_commit, loaded.base_commit);
        assert_eq!(session.branch_name, loaded.branch_name);
        assert_eq!(session.diff_source, loaded.diff_source);
        assert_eq!(session.files.len(), loaded.files.len());
        let _ = delete_session(&path);
    }

    #[test]
    fn should_sanitize_branch_name_in_filename() {
        let session = create_session(
            PathBuf::from("/tmp/test-repo"),
            "abc1234def",
            Some("feature/login"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let filename = session_filename(&session);
        assert!(!filename.contains('/'));
        assert!(filename.contains("feature-login"));
    }

    #[test]
    fn should_select_latest_session_for_branch() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let session1 = create_session(
            repo_path.clone(),
            "commit-1",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let path1 = save_session(&session1).unwrap();

        let session2 = create_session(
            repo_path.clone(),
            "commit-2",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let path2 = save_session(&session2).unwrap();
        ensure_newer_mtime(&path2, &path1);
        let (selected_path, selected) = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "head-does-not-matter-for-branch",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap()
        .unwrap();
        assert_eq!(selected_path, path2);
        assert_ne!(selected_path, path1);
        assert_eq!(selected.base_commit, "commit-2");
    }

    #[test]
    fn should_match_branch_even_when_head_commit_differs() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let session = create_session(
            repo_path.clone(),
            "old-head",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let _ = save_session(&session).unwrap();
        let loaded = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "new-head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        assert!(loaded.is_some());
    }

    #[test]
    fn should_load_session_with_underscore_branch_name() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let session = create_session(
            repo_path.clone(),
            "head-commit",
            Some("feature/with_underscores"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let _ = save_session(&session).unwrap();
        let loaded = load_latest_session_for_context(
            &repo_path,
            Some("feature/with_underscores"),
            "new-head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        assert!(loaded.is_some());
    }

    #[test]
    fn should_load_session_with_hex_like_branch_segment() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let session = create_session(
            repo_path.clone(),
            "head-commit",
            Some("feature/deadbeef_fix"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let _ = save_session(&session).unwrap();
        let loaded = load_latest_session_for_context(
            &repo_path,
            Some("feature/deadbeef_fix"),
            "new-head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        assert!(loaded.is_some());
    }

    #[test]
    fn should_prefer_branch_match_over_legacy_candidate() {
        let guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let branch_session = create_session(
            repo_path.clone(),
            "branch-base",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let branch_path = save_session(&branch_session).unwrap();

        let legacy_source = create_session(
            repo_path.clone(),
            "head-commit",
            None,
            SessionDiffSource::WorkingTree,
            None,
        );
        let legacy_path = save_legacy_session(&guard.path, &legacy_source);
        let (selected_path, _selected) = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "head-commit",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap()
        .unwrap();
        assert_eq!(selected_path, branch_path);
        assert_ne!(selected_path, legacy_path);
    }

    #[test]
    fn should_fallback_to_legacy_session_when_no_branch_session_exists() {
        let guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let legacy_source = create_session(
            repo_path.clone(),
            "head-commit",
            None,
            SessionDiffSource::WorkingTree,
            None,
        );
        let legacy_path = save_legacy_session(&guard.path, &legacy_source);
        let (selected_path, selected) = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "head-commit",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap()
        .unwrap();
        assert_eq!(selected_path, legacy_path);
        assert_eq!(selected.branch_name, None);
        assert_eq!(selected.diff_source, SessionDiffSource::WorkingTree);
    }

    #[test]
    fn should_not_select_legacy_session_when_head_commit_differs() {
        let guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let legacy_source = create_session(
            repo_path.clone(),
            "old-head",
            None,
            SessionDiffSource::WorkingTree,
            None,
        );
        let _legacy_path = save_legacy_session(&guard.path, &legacy_source);
        let loaded = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "new-head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        assert!(loaded.is_none());
    }

    #[test]
    fn should_require_commit_match_in_detached_head() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let session = create_session(
            repo_path.clone(),
            "detached-head",
            None,
            SessionDiffSource::WorkingTree,
            None,
        );
        let _ = save_session(&session).unwrap();
        let mismatch = load_latest_session_for_context(
            &repo_path,
            None,
            "different-head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        let match_ = load_latest_session_for_context(
            &repo_path,
            None,
            "detached-head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        assert!(mismatch.is_none());
        assert!(match_.is_some());
    }

    #[test]
    fn should_ignore_sessions_with_different_diff_source() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let commit_range = vec!["commit-2".to_string(), "commit-1".to_string()];
        let commits_session = create_session(
            repo_path.clone(),
            "commit-2",
            Some("main"),
            SessionDiffSource::CommitRange,
            Some(commit_range.clone()),
        );
        let _ = save_session(&commits_session).unwrap();
        let worktree = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap();
        let commits = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "head",
            SessionDiffSource::CommitRange,
            Some(commit_range.as_slice()),
        )
        .unwrap();
        assert!(worktree.is_none());
        assert!(commits.is_some());
    }

    #[test]
    fn should_match_commit_range_session() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let commit_range_a = vec!["commit-a2".to_string(), "commit-a1".to_string()];
        let commit_range_b = vec!["commit-b2".to_string(), "commit-b1".to_string()];

        let session_a = create_session(
            repo_path.clone(),
            "commit-a2",
            Some("main"),
            SessionDiffSource::CommitRange,
            Some(commit_range_a.clone()),
        );
        let path_a = save_session(&session_a).unwrap();

        let session_b = create_session(
            repo_path.clone(),
            "commit-b2",
            Some("main"),
            SessionDiffSource::CommitRange,
            Some(commit_range_b.clone()),
        );
        let path_b = save_session(&session_b).unwrap();
        let (selected_path, selected) = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "commit-b2",
            SessionDiffSource::CommitRange,
            Some(commit_range_b.as_slice()),
        )
        .unwrap()
        .unwrap();
        assert_eq!(selected_path, path_b);
        assert_ne!(selected_path, path_a);
        assert_eq!(
            selected.commit_range.as_deref(),
            Some(commit_range_b.as_slice())
        );
    }

    #[test]
    fn should_roundtrip_commit_range_session() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let commit_range = vec!["commit-2".to_string(), "commit-1".to_string()];
        let session = create_session(
            repo_path,
            "commit-2",
            Some("main"),
            SessionDiffSource::CommitRange,
            Some(commit_range.clone()),
        );
        let path = save_session(&session).unwrap();
        let loaded = load_session(&path).unwrap();
        assert_eq!(loaded.commit_range, Some(commit_range));
        assert_eq!(loaded.diff_source, SessionDiffSource::CommitRange);
        let _ = delete_session(&path);
    }

    #[test]
    fn should_require_commit_range_order_match() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let commit_range = vec!["commit-2".to_string(), "commit-1".to_string()];
        let reversed_range = vec!["commit-1".to_string(), "commit-2".to_string()];

        let session = create_session(
            repo_path.clone(),
            "commit-2",
            Some("main"),
            SessionDiffSource::CommitRange,
            Some(commit_range),
        );
        let _ = save_session(&session).unwrap();
        let loaded = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "commit-2",
            SessionDiffSource::CommitRange,
            Some(reversed_range.as_slice()),
        )
        .unwrap();
        assert!(loaded.is_none());
    }

    #[test]
    fn should_skip_commit_sessions_without_range_match() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let commit_range = vec!["commit-2".to_string(), "commit-1".to_string()];

        let session = create_session(
            repo_path.clone(),
            "commit-2",
            Some("main"),
            SessionDiffSource::CommitRange,
            None,
        );
        let _ = save_session(&session).unwrap();
        let loaded = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "commit-2",
            SessionDiffSource::CommitRange,
            Some(commit_range.as_slice()),
        )
        .unwrap();
        assert!(loaded.is_none());
    }

    #[test]
    fn should_load_legacy_session_without_tour_fields() {
        let guard = with_test_reviews_dir();
        let session = create_test_session();
        let path = save_legacy_session(&guard.path, &session);
        let loaded = load_session(&path).unwrap();
        assert!(loaded.tour.is_none());
        assert!(loaded.tour_comment_meta.is_empty());
        assert!(loaded.tour_triage.is_empty());
    }

    #[test]
    fn should_roundtrip_session_with_tour_state() {
        use crate::model::tour::{
            CommentTriage, NewCommentLocation, TourCommentMeta, TourState, TourStop,
            TourTriageVerdict,
        };
        let _guard = with_test_reviews_dir();
        let mut session = create_test_session();
        session.tour = Some(TourState {
            stops: vec![
                TourStop {
                    commit_ids: vec!["abc".into()],
                    summary: "first commit".into(),
                    risk: crate::risk::RiskScore::MIN,
                },
                TourStop {
                    commit_ids: vec!["def".into(), "ghi".into()],
                    summary: "batched".into(),
                    risk: crate::risk::RiskScore::MIN,
                },
            ],
            index: 1,
            threshold: crate::risk::RiskScore::MIN,
            tour_schema_version: crate::model::tour::TOUR_SCHEMA_VERSION,
        });
        session.tour_comment_meta.insert(
            "cid-1".to_string(),
            TourCommentMeta {
                stop_index: 0,
                stop_commit_shas: vec!["abc".into()],
                file: "src/a.rs".into(),
                line: 42,
            },
        );
        session.tour_triage.insert(
            "cid-1".to_string(),
            CommentTriage {
                verdict: TourTriageVerdict::Moved,
                reasoning: "renamed".into(),
                new_location: Some(NewCommentLocation {
                    file: "src/a2.rs".into(),
                    line: 50,
                }),
            },
        );

        let path = save_session(&session).unwrap();
        let loaded = load_session(&path).unwrap();
        assert_eq!(loaded.tour.as_ref().unwrap().index, 1);
        assert_eq!(loaded.tour.as_ref().unwrap().stops.len(), 2);
        assert_eq!(loaded.tour_comment_meta.len(), 1);
        let meta = loaded.tour_comment_meta.get("cid-1").unwrap();
        assert_eq!(meta.file, "src/a.rs");
        assert_eq!(meta.line, 42);
        let triage = loaded.tour_triage.get("cid-1").unwrap();
        assert_eq!(triage.verdict, TourTriageVerdict::Moved);
        assert_eq!(triage.new_location.as_ref().unwrap().line, 50);
        let _ = delete_session(&path);
    }

    #[test]
    fn load_session_migrates_legacy_tour_to_current_schema_version() {
        // Regression (H11): a session whose on-disk tour JSON predates
        // the H11 tour_schema_version field must load successfully and
        // have its tour rolled forward via migrate_tour.
        let guard = with_test_reviews_dir();
        let legacy = format!(
            r#"{{
              "id": "abc",
              "version": "{SESSION_VERSION}",
              "repo_path": "/repo",
              "base_commit": "deadbeef",
              "created_at": "2024-01-01T00:00:00Z",
              "updated_at": "2024-01-01T00:00:00Z",
              "files": {{}},
              "tour": {{
                "stops": [{{"commit_ids": ["sha1"], "summary": "first"}}],
                "index": 0
              }}
            }}"#
        );
        let path = guard.path.join("legacy-tour-session.json");
        fs::write(&path, legacy).unwrap();

        let loaded = load_session(&path).expect("legacy tour session loads");
        let tour = loaded.tour.expect("tour preserved");
        assert_eq!(
            tour.tour_schema_version,
            crate::model::tour::TOUR_SCHEMA_VERSION,
            "migrate_tour rolled the legacy v0 tour up to current"
        );
        assert_eq!(tour.stops.len(), 1);
        assert_eq!(tour.stops[0].commit_ids, vec!["sha1".to_string()]);
    }

    #[test]
    fn load_session_migrates_legacy_mcp_author_marker_to_author_kind() {
        // Regression (Phase B): 1.3-era sessions tagged agent-authored
        // comments by appending `\n\n_(via MCP agent)_` to the body. 1.4
        // introduced `AuthorKind`; `load_session` must strip the legacy
        // suffix and promote `author_kind` on load.
        use crate::model::AuthorKind;
        let guard = with_test_reviews_dir();
        let legacy = r#"{
              "id": "abc",
              "version": "1.3",
              "repo_path": "/repo",
              "base_commit": "deadbeef",
              "created_at": "2024-01-01T00:00:00Z",
              "updated_at": "2024-01-01T00:00:00Z",
              "files": {
                "src/foo.rs": {
                  "path": "src/foo.rs",
                  "reviewed": false,
                  "status": "modified",
                  "file_comments": [
                    {
                      "id": "fc1",
                      "content": "plain human file comment",
                      "comment_type": "note",
                      "created_at": "2024-01-01T00:00:00Z",
                      "line_context": null
                    }
                  ],
                  "line_comments": {
                    "10": [
                      {
                        "id": "lc1",
                        "content": "agent said\n\n_(via MCP agent)_",
                        "comment_type": "note",
                        "created_at": "2024-01-01T00:00:00Z",
                        "line_context": null,
                        "side": "new"
                      },
                      {
                        "id": "lc2",
                        "content": "human said",
                        "comment_type": "note",
                        "created_at": "2024-01-01T00:00:00Z",
                        "line_context": null,
                        "side": "new"
                      }
                    ]
                  }
                }
              },
              "review_comments": [
                {
                  "id": "rc1",
                  "content": "agent review comment\n\n_(via MCP agent)_",
                  "comment_type": "note",
                  "created_at": "2024-01-01T00:00:00Z",
                  "line_context": null
                }
              ]
            }"#;
        let path = guard.path.join("legacy-mcp-marker.json");
        fs::write(&path, legacy).unwrap();

        let loaded = load_session(&path).expect("legacy marker session loads");

        // Agent line comment: suffix stripped, kind promoted.
        let file = loaded.files.get(&PathBuf::from("src/foo.rs")).unwrap();
        let line10 = file.line_comments.get(&10).unwrap();
        assert_eq!(line10[0].content, "agent said");
        assert_eq!(line10[0].author_kind, AuthorKind::McpAgent);
        // Human line comment: untouched.
        assert_eq!(line10[1].content, "human said");
        assert_eq!(line10[1].author_kind, AuthorKind::Human);
        // Human file comment: untouched.
        assert_eq!(file.file_comments[0].content, "plain human file comment");
        assert_eq!(file.file_comments[0].author_kind, AuthorKind::Human);
        // Agent review comment: also migrated.
        assert_eq!(loaded.review_comments[0].content, "agent review comment");
        assert_eq!(loaded.review_comments[0].author_kind, AuthorKind::McpAgent);
    }

    #[test]
    fn should_disambiguate_repos_with_same_folder_name() {
        let _guard = with_test_reviews_dir();
        let base = std::env::temp_dir().join(format!("trv-repos-{}", uuid::Uuid::new_v4()));
        let repo_a = base.join("a").join("same-repo");
        let repo_b = base.join("b").join("same-repo");
        fs::create_dir_all(&repo_a).unwrap();
        fs::create_dir_all(&repo_b).unwrap();

        let session_a = create_session(
            repo_a.clone(),
            "head-a",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let _ = save_session(&session_a).unwrap();

        let session_b = create_session(
            repo_b.clone(),
            "head-b",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let _ = save_session(&session_b).unwrap();
        let (_path, selected) = load_latest_session_for_context(
            &repo_a,
            Some("main"),
            "head",
            SessionDiffSource::WorkingTree,
            None,
        )
        .unwrap()
        .unwrap();
        assert_eq!(selected.base_commit, "head-a");
        assert_eq!(
            normalize_repo_path(&selected.repo_path),
            normalize_repo_path(&repo_a)
        );
    }

    // ---- New tests covering the bug fixes ----

    /// C1: every `SessionDiffSource` variant must round-trip through the
    /// filename writer/parser pair. Previously `staged_and_unstaged`,
    /// `worktree_and_commits`, and `staged_unstaged_and_commits` tripped the
    /// parser because it assumed `diff_source` was a single token.
    #[test]
    fn should_roundtrip_filename_for_all_diff_source_variants() {
        let repo_path = PathBuf::from("/tmp/test-repo");
        let commit_range = vec!["c1".to_string(), "c2".to_string()];
        let variants = [
            (SessionDiffSource::WorkingTree, "worktree", None),
            (SessionDiffSource::Staged, "staged", None),
            (SessionDiffSource::Unstaged, "unstaged", None),
            (
                SessionDiffSource::StagedAndUnstaged,
                "staged_and_unstaged",
                None,
            ),
            (
                SessionDiffSource::CommitRange,
                "commits",
                Some(commit_range.clone()),
            ),
            (
                SessionDiffSource::WorkingTreeAndCommits,
                "worktree_and_commits",
                Some(commit_range.clone()),
            ),
            (
                SessionDiffSource::StagedUnstagedAndCommits,
                "staged_unstaged_and_commits",
                Some(commit_range.clone()),
            ),
            (SessionDiffSource::Remote, "remote", None),
        ];

        for (variant, expected_slug, range) in variants {
            let session = create_session(
                repo_path.clone(),
                "base-commit",
                Some("main"),
                variant,
                range,
            );
            let filename = session_filename(&session);

            assert!(
                filename.contains(&format!("--{expected_slug}--")),
                "filename {filename} should contain slug {expected_slug}"
            );

            let parts =
                parse_session_filename(&filename).expect("filename must parse for every variant");
            assert_eq!(
                parts.diff_source, expected_slug,
                "parser drift for {variant:?}"
            );
            assert_eq!(parts.repo_fingerprints.len(), 1);
            assert!(is_hex_fingerprint(&parts.repo_fingerprints[0]));
        }
    }

    /// C1: filter must not crash or silently drop sessions for multi-token
    /// diff sources once the session is round-tripped through disk.
    #[test]
    fn should_filter_multi_token_diff_source_sessions() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        let commit_range = vec!["c1".to_string(), "c2".to_string()];
        let session = create_session(
            repo_path.clone(),
            "c2",
            Some("main"),
            SessionDiffSource::StagedUnstagedAndCommits,
            Some(commit_range.clone()),
        );
        let path = save_session(&session).unwrap();

        let (selected_path, selected) = load_latest_session_for_context(
            &repo_path,
            Some("main"),
            "c2",
            SessionDiffSource::StagedUnstagedAndCommits,
            Some(commit_range.as_slice()),
        )
        .unwrap()
        .unwrap();
        assert_eq!(selected_path, path);
        assert_eq!(
            selected.diff_source,
            SessionDiffSource::StagedUnstagedAndCommits
        );
    }

    /// H3: sessions whose schema version is newer than this binary must be
    /// rejected rather than silently loaded with missing fields.
    #[test]
    fn should_reject_session_with_newer_version() {
        let _guard = with_test_reviews_dir();
        let reviews_dir = get_reviews_dir().unwrap();

        let session = create_test_session();
        let mut value = serde_json::to_value(&session).unwrap();
        value["version"] = serde_json::Value::String("9.9".to_string());
        let path = reviews_dir.join("newer-version.json");
        fs::write(&path, serde_json::to_string_pretty(&value).unwrap()).unwrap();

        let err = load_session(&path).unwrap_err();
        match err {
            TrvError::CorruptedSession(msg) => {
                assert!(
                    msg.contains("9.9"),
                    "error message should mention version: {msg}"
                );
                assert!(
                    msg.contains(SESSION_VERSION),
                    "error message should mention supported version"
                );
            }
            other => panic!("expected CorruptedSession, got {other:?}"),
        }
    }

    /// H3: sessions at or below the supported version must still load.
    #[test]
    fn should_accept_session_with_current_version() {
        let _guard = with_test_reviews_dir();
        let session = create_test_session();
        let path = save_session(&session).unwrap();
        let loaded = load_session(&path).unwrap();
        assert_eq!(loaded.version, SESSION_VERSION);
    }

    /// Version comparison must be numeric, not lexicographic: `"1.10"` is
    /// newer than `"1.2"`.
    #[test]
    fn should_compare_versions_numerically_not_lexicographically() {
        let v1_10 = parse_version("1.10").expect("1.10 must parse");
        let v1_2 = parse_version("1.2").expect("1.2 must parse");
        assert!(v1_10 > v1_2, "1.10 must sort newer than 1.2 numerically");
    }

    /// Sanity check that a larger major beats a large minor.
    #[test]
    fn should_treat_major_version_bumps_as_newer() {
        let v2_0 = parse_version("2.0").expect("2.0 must parse");
        let v1_99 = parse_version("1.99").expect("1.99 must parse");
        assert!(v2_0 > v1_99, "2.0 must sort newer than 1.99");
    }

    /// Malformed version strings are rejected rather than silently accepted.
    #[test]
    fn should_reject_malformed_version() {
        assert!(parse_version("").is_none());
        assert!(parse_version("1.x").is_none());
        assert!(parse_version("abc").is_none());
        assert!(parse_version("1..2").is_none());
    }

    /// Equal versions must be accepted by `load_session`.
    #[test]
    fn should_accept_session_with_equal_version() {
        let _guard = with_test_reviews_dir();
        let reviews_dir = get_reviews_dir().unwrap();

        let session = create_test_session();
        let mut value = serde_json::to_value(&session).unwrap();
        value["version"] = serde_json::Value::String(SESSION_VERSION.to_string());
        let path = reviews_dir.join("equal-version.json");
        fs::write(&path, serde_json::to_string_pretty(&value).unwrap()).unwrap();

        let loaded = load_session(&path).expect("equal version must load");
        assert_eq!(loaded.version, SESSION_VERSION);
    }

    /// `load_session` must reject `"1.10"` as newer than `"1.2"` / `"1.0"`
    /// (whichever the current SESSION_VERSION is) when numerics say so.
    #[test]
    fn should_reject_newer_version_by_numeric_comparison() {
        let _guard = with_test_reviews_dir();
        let reviews_dir = get_reviews_dir().unwrap();

        // Build a version that is guaranteed numerically newer than the
        // current SESSION_VERSION regardless of what that value is today.
        let current = parse_version(SESSION_VERSION).unwrap();
        let newer_major = current[0] + 1;
        let newer = format!("{newer_major}.0");

        let session = create_test_session();
        let mut value = serde_json::to_value(&session).unwrap();
        value["version"] = serde_json::Value::String(newer.clone());
        let path = reviews_dir.join("numerically-newer.json");
        fs::write(&path, serde_json::to_string_pretty(&value).unwrap()).unwrap();

        let err = load_session(&path).unwrap_err();
        match err {
            TrvError::CorruptedSession(msg) => {
                assert!(
                    msg.contains(&newer),
                    "message should mention version: {msg}"
                );
            }
            other => panic!("expected CorruptedSession, got {other:?}"),
        }
    }

    /// `save_session` must produce a file whose contents equal the most
    /// recent save — not an append or a half-written intermediate. This
    /// guards the atomic-rename path in `save_session`.
    #[test]
    fn should_overwrite_session_file_atomically_on_resave() {
        let _guard = with_test_reviews_dir();
        let repo_path = std::env::temp_dir().join(format!("trv-repo-{}", uuid::Uuid::new_v4()));
        fs::create_dir_all(&repo_path).unwrap();

        // First save: branch `main`, one file.
        let session1 = create_session(
            repo_path.clone(),
            "base-1",
            Some("main"),
            SessionDiffSource::WorkingTree,
            None,
        );
        let path1 = save_session(&session1).unwrap();

        // Second save with the same id (and therefore same filename) but a
        // distinct payload, so we can prove the file reflects the second
        // write in full rather than a concatenation or truncated remnant.
        let mut session2 = session1.clone();
        session2.base_commit = "base-2".to_string();
        session2.add_file(PathBuf::from("src/other.rs"), FileStatus::Added);

        let path2 = save_session(&session2).unwrap();
        assert_eq!(
            path1, path2,
            "save path must be stable for a session with stable id"
        );

        // File must be valid JSON (not garbled) and must match session2.
        let loaded = load_session(&path2).expect("resaved file must be valid JSON");
        assert_eq!(loaded.base_commit, "base-2");
        assert_eq!(
            loaded.files.len(),
            2,
            "second save must be the full payload, not appended"
        );

        // No leftover `.tmp` sidecar should remain in the reviews dir.
        let reviews_dir = get_reviews_dir().unwrap();
        for entry in fs::read_dir(&reviews_dir).unwrap().flatten() {
            let name = entry.file_name();
            let name_str = name.to_string_lossy();
            assert!(
                !name_str.ends_with(".tmp"),
                "stale tmp file left behind: {name_str}"
            );
        }
    }

    /// `load_session` must reject sessions whose `version` field is not a
    /// valid dotted numeric string.
    #[test]
    fn should_reject_session_with_malformed_version() {
        let _guard = with_test_reviews_dir();
        let reviews_dir = get_reviews_dir().unwrap();

        let session = create_test_session();
        let mut value = serde_json::to_value(&session).unwrap();
        value["version"] = serde_json::Value::String("not-a-version".to_string());
        let path = reviews_dir.join("malformed-version.json");
        fs::write(&path, serde_json::to_string_pretty(&value).unwrap()).unwrap();

        let err = load_session(&path).unwrap_err();
        match err {
            TrvError::CorruptedSession(msg) => {
                assert!(
                    msg.contains("not-a-version"),
                    "message should mention version: {msg}"
                );
            }
            other => panic!("expected CorruptedSession, got {other:?}"),
        }
    }

    /// H2: the thread-local override must route writes without touching the
    /// process-wide environment. We assert the env var stays untouched.
    #[test]
    fn should_use_thread_local_override_without_env_var() {
        let key = "TRV_REVIEWS_DIR";
        let before = std::env::var_os(key);

        let _guard = with_test_reviews_dir();
        let session = create_test_session();
        let saved = save_session(&session).unwrap();
        assert!(
            saved.exists(),
            "session should be written under thread-local override"
        );

        let after = std::env::var_os(key);
        assert_eq!(before, after, "tests must not mutate TRV_REVIEWS_DIR");
    }

    /// H2: resolved paths should match the override directory exactly.
    #[test]
    fn should_resolve_reviews_dir_from_thread_local_override() {
        let guard = with_test_reviews_dir();
        let resolved = get_reviews_dir().unwrap();
        assert_eq!(resolved, guard.path);
    }

    /// H2: clearing the override must fall back to the default production path.
    /// We only assert that the override is no longer honored (the real path may
    /// legitimately vary by platform / user profile).
    #[test]
    fn should_clear_override_on_guard_drop() {
        let temp = {
            let guard = with_test_reviews_dir();
            let path = get_reviews_dir().unwrap();
            assert_eq!(path, guard.path);
            guard.path.clone()
        };
        // After drop the override should be gone: resolving again should not
        // return the (now-deleted) temp directory.
        let resolved = get_reviews_dir().unwrap();
        assert_ne!(resolved, temp, "override must be cleared after guard drop");
    }

    // ---- Phase G: session GC (age / size / count bounds) ----

    /// Write `content` to a session file under `dir` and stamp its mtime
    /// `age_days` days in the past so the age pass sees it as stale.
    fn write_aged_session(dir: &Path, name: &str, content: &str, age_days: u64) -> PathBuf {
        let path = dir.join(name);
        fs::write(&path, content).unwrap();
        let age = Duration::from_secs(age_days * 24 * 60 * 60);
        let modified = std::time::SystemTime::now()
            .checked_sub(age)
            .unwrap_or(std::time::SystemTime::UNIX_EPOCH);
        let ft = filetime::FileTime::from_system_time(modified);
        filetime::set_file_mtime(&path, ft).unwrap();
        path
    }

    #[test]
    fn gc_age_eviction_preserves_historical_behavior() {
        // Regression guard: the default SessionGcConfig (7-day age, no
        // size / count cap) must evict files older than 7 days and leave
        // fresher files alone — matching pre-Phase-G behaviour.
        let guard = with_test_reviews_dir();
        let stale = write_aged_session(&guard.path, "stale.json", "{}", 30);
        let fresh = write_aged_session(&guard.path, "fresh.json", "{}", 1);

        let report = purge_sessions(&guard.path, &SessionGcConfig::default(), false);
        assert_eq!(report.removed_age, 1);
        assert_eq!(report.removed_size, 0);
        assert_eq!(report.removed_count, 0);
        assert_eq!(report.remaining_files, 1);
        assert!(!stale.exists(), "30-day-old file should be purged");
        assert!(fresh.exists(), "1-day-old file should be preserved");
    }

    #[test]
    fn gc_size_cap_evicts_oldest_first() {
        let guard = with_test_reviews_dir();
        // 1 MB-ish bodies so the size cap is easy to reason about.
        let body = "x".repeat(1024 * 1024);
        let oldest = write_aged_session(&guard.path, "oldest.json", &body, 2);
        let middle = write_aged_session(&guard.path, "middle.json", &body, 1);
        let newest = write_aged_session(&guard.path, "newest.json", &body, 0);

        let cfg = SessionGcConfig {
            max_age_days: 0,
            max_size_mb: 2,
            max_count: 0,
        };
        let report = purge_sessions(&guard.path, &cfg, false);
        assert_eq!(report.removed_age, 0);
        assert!(
            report.removed_size >= 1,
            "size cap must evict at least one file"
        );
        assert!(!oldest.exists(), "oldest file should be evicted first");
        assert!(middle.exists(), "middle file should survive a 2 MB cap");
        assert!(newest.exists(), "newest file should always survive");
    }

    #[test]
    fn gc_count_cap_keeps_newest_n() {
        let guard = with_test_reviews_dir();
        let f1 = write_aged_session(&guard.path, "f1.json", "{}", 5);
        let f2 = write_aged_session(&guard.path, "f2.json", "{}", 4);
        let f3 = write_aged_session(&guard.path, "f3.json", "{}", 3);
        let f4 = write_aged_session(&guard.path, "f4.json", "{}", 2);

        let cfg = SessionGcConfig {
            max_age_days: 0,
            max_size_mb: 0,
            max_count: 2,
        };
        let report = purge_sessions(&guard.path, &cfg, false);
        assert_eq!(report.removed_count, 2);
        assert_eq!(report.remaining_files, 2);
        assert!(!f1.exists(), "oldest pruned");
        assert!(!f2.exists(), "second oldest pruned");
        assert!(f3.exists(), "second newest kept");
        assert!(f4.exists(), "newest kept");
    }

    #[test]
    fn gc_dry_run_reports_without_deleting() {
        let guard = with_test_reviews_dir();
        let stale = write_aged_session(&guard.path, "stale.json", "{}", 30);
        let fresh = write_aged_session(&guard.path, "fresh.json", "{}", 1);

        let report = purge_sessions(&guard.path, &SessionGcConfig::default(), true);
        assert_eq!(report.removed_age, 1);
        assert!(stale.exists(), "dry run must not delete the stale file");
        assert!(fresh.exists());
    }

    #[test]
    fn gc_zero_means_unbounded_for_each_cap() {
        // All three caps set to 0 => full no-op (scanned only, nothing removed).
        let guard = with_test_reviews_dir();
        let f1 = write_aged_session(&guard.path, "a.json", "{}", 365);
        let f2 = write_aged_session(&guard.path, "b.json", "{}", 365);

        let cfg = SessionGcConfig {
            max_age_days: 0,
            max_size_mb: 0,
            max_count: 0,
        };
        let report = purge_sessions(&guard.path, &cfg, false);
        assert_eq!(report.scanned, 2);
        assert_eq!(report.removed_age, 0);
        assert_eq!(report.removed_size, 0);
        assert_eq!(report.removed_count, 0);
        assert!(f1.exists());
        assert!(f2.exists());
    }

    #[test]
    fn gc_config_round_trips_through_toml() {
        let toml_src = r#"
[session_gc]
max_age_days = 14
max_size_mb = 100
max_count = 50
"#;
        let cfg: SessionGcConfig = toml::from_str::<toml::Table>(toml_src)
            .unwrap()
            .get("session_gc")
            .unwrap()
            .clone()
            .try_into()
            .unwrap();
        assert_eq!(cfg.max_age_days, 14);
        assert_eq!(cfg.max_size_mb, 100);
        assert_eq!(cfg.max_count, 50);

        // Defaults: empty section preserves historical (age-only, 7-day) behaviour.
        let empty: SessionGcConfig = toml::from_str("").unwrap();
        assert_eq!(empty, SessionGcConfig::default());
        assert_eq!(empty.max_age_days, 7);
        assert_eq!(empty.max_size_mb, 0);
        assert_eq!(empty.max_count, 0);
    }
}